debug system config #1179
debug system config #1179
Conversation
WalkthroughThis update introduces two main changes. First, in the consensus system contract logic, the method responsible for fetching an address from L1 storage now disregards the actual storage value and assigns a fixed, hardcoded Ethereum address instead. Second, in the Ethereum handler, the broadcasting of newly mined blocks is disabled by commenting out the relevant loop, stopping the propagation and announcement of mined blocks to peers via this mechanism. Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
consensus/system_contract/system_contract.go (1)
96-98: Remove commented-out code alternativesThese commented-out address alternatives are adding noise to the codebase without providing value.
- // bAddress := common.HexToAddress("0x756ea06bdee36de11f22dcca45a31d8a178ef3c6") - // bAddress := common.HexToAddress("0xD9E635beA2Ed2813eD1c550429FB965dEE58109B") - // bAddress := common.Address{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
consensus/system_contract/system_contract.go(1 hunks)eth/handler.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
consensus/system_contract/system_contract.go (1)
common/types.go (1)
HexToAddress(218-218)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
| _, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get signer address from L1 System Contract: %w", err) | ||
| } | ||
| bAddress := common.BytesToAddress(address) | ||
|
|
||
| // bAddress := common.HexToAddress("0x756ea06bdee36de11f22dcca45a31d8a178ef3c6") | ||
| // bAddress := common.HexToAddress("0xD9E635beA2Ed2813eD1c550429FB965dEE58109B") | ||
| // bAddress := common.Address{} | ||
| bAddress := common.HexToAddress("0xfe3Ba6Fac4Fdd5C975A61089AF2a3508a86D6a8B") |
There was a problem hiding this comment.
Hardcoded address bypasses dynamic L1 storage resolution
This change fundamentally alters the system's behavior - the function still retrieves L1 storage data but then ignores it, instead assigning a hardcoded Ethereum address. This approach bypasses the dynamic address resolution mechanism that appears to be the original design intent.
Consider either:
- Properly using the retrieved storage value:
- _, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil)
+ storage, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil)
if err != nil {
return fmt.Errorf("failed to get signer address from L1 System Contract: %w", err)
}
- // bAddress := common.HexToAddress("0x756ea06bdee36de11f22dcca45a31d8a178ef3c6")
- // bAddress := common.HexToAddress("0xD9E635beA2Ed2813eD1c550429FB965dEE58109B")
- // bAddress := common.Address{}
- bAddress := common.HexToAddress("0xfe3Ba6Fac4Fdd5C975A61089AF2a3508a86D6a8B")
+ bAddress := common.BytesToAddress(storage)- If this change is intentional, modify the function to skip making the unnecessary L1 call entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil) | |
| if err != nil { | |
| return fmt.Errorf("failed to get signer address from L1 System Contract: %w", err) | |
| } | |
| bAddress := common.BytesToAddress(address) | |
| // bAddress := common.HexToAddress("0x756ea06bdee36de11f22dcca45a31d8a178ef3c6") | |
| // bAddress := common.HexToAddress("0xD9E635beA2Ed2813eD1c550429FB965dEE58109B") | |
| // bAddress := common.Address{} | |
| bAddress := common.HexToAddress("0xfe3Ba6Fac4Fdd5C975A61089AF2a3508a86D6a8B") | |
| storage, err := s.client.StorageAt(s.ctx, s.config.SystemContractAddress, s.config.SystemContractSlot, nil) | |
| if err != nil { | |
| return fmt.Errorf("failed to get signer address from L1 System Contract: %w", err) | |
| } | |
| bAddress := common.BytesToAddress(storage) |
| // for obj := range h.minedBlockSub.Chan() { | ||
| // if ev, ok := obj.Data.(core.NewMinedBlockEvent); ok { | ||
| // h.BroadcastBlock(ev.Block, true) // First propagate block to peers | ||
| // h.BroadcastBlock(ev.Block, false) // Only then announce to the rest | ||
| // } | ||
| // } |
There was a problem hiding this comment.
Block broadcasting is completely disabled
This change completely disables the broadcasting of newly mined blocks to network peers. This will prevent nodes running this code from contributing to block propagation in the network, which could significantly impact network health and performance.
If this is temporary debugging code, please add a prominent comment explaining why it's disabled and when it should be re-enabled. If this is intended to be a permanent change, consider:
- Either removing the minedBroadcastLoop function entirely.
- Or explaining the rationale for disabling block broadcasting with clear comments.
func (h *handler) minedBroadcastLoop() {
defer h.wg.Done()
+ // Block broadcasting disabled intentionally because:
+ // [EXPLANATION HERE]
// for obj := range h.minedBlockSub.Chan() {
// if ev, ok := obj.Data.(core.NewMinedBlockEvent); ok {
// h.BroadcastBlock(ev.Block, true) // First propagate block to peers
// h.BroadcastBlock(ev.Block, false) // Only then announce to the rest
// }
// }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // for obj := range h.minedBlockSub.Chan() { | |
| // if ev, ok := obj.Data.(core.NewMinedBlockEvent); ok { | |
| // h.BroadcastBlock(ev.Block, true) // First propagate block to peers | |
| // h.BroadcastBlock(ev.Block, false) // Only then announce to the rest | |
| // } | |
| // } | |
| func (h *handler) minedBroadcastLoop() { | |
| defer h.wg.Done() | |
| // Block broadcasting disabled intentionally because: | |
| // [EXPLANATION HERE] | |
| // for obj := range h.minedBlockSub.Chan() { | |
| // if ev, ok := obj.Data.(core.NewMinedBlockEvent); ok { | |
| // h.BroadcastBlock(ev.Block, true) // First propagate block to peers | |
| // h.BroadcastBlock(ev.Block, false) // Only then announce to the rest | |
| // } | |
| // } | |
| } |
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Bug Fixes
Chores