feat: add system config consensus to deprecate clique #1102
Conversation
WalkthroughThis pull request updates various components to enhance consensus mechanisms and network interactions. Key modifications include updating function signatures (e.g., adding an extra nil or L1 client parameter in several services), integrating new consensus modules such as the SystemContract and UpgradableEngine, and adjusting block header handling for network compatibility. Several new files and tests introduce a proof-of-authority consensus engine with error handling improvements and dynamic consensus selection. Minor formatting and dependency updates are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UpgradableEngine
participant CliqueEngine
participant SystemContractEngine
Client->>UpgradableEngine: Call Author/Seal method
UpgradableEngine->>UpgradableEngine: Invoke chooseEngine(header)
alt Block requires upgrade
UpgradableEngine->>SystemContractEngine: Delegate call
SystemContractEngine-->>UpgradableEngine: Return result
else
UpgradableEngine->>CliqueEngine: Delegate call
CliqueEngine-->>UpgradableEngine: Return result
end
UpgradableEngine-->>Client: Return final result
sequenceDiagram
participant SC as SystemContract
participant Ticker
participant L1 as L1 Client
SC->>Ticker: Start sync loop
Ticker->>SC: Tick event
SC->>L1: fetchAddressFromL1()
L1-->>SC: Return signer address
SC->>SC: Update internal signer if changed
SC-->>Ticker: Continue sync
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
d87b55d to
f438d7b
Compare
There was a problem hiding this comment.
- this seems nicely testable in unit tests by mocking the
EthClientand returning the values you expect for signing and verifying so you should be able to cover most of the code. - what about the block hash? don't we need to exclude the
ExtraDatafrom the block hash calculation so that we can create the same hashes when reconstructing blocks from L1? - how does an upgrade work with the new consensus mechanism? this is a mandatory upgrade, so node operators need to upgrade before a hard fork. but how exactly do we support and switch the consensus mechanism from clique to system contract based on a fork given in the config?
|
Thanks for the comments @jonastheis , addressed them except for the one on failing the node on Start (commented there in the convo) |
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
7774ca6 to
a77524f
Compare
|
Bringing back the implementation with only one signer, see discussion here. |
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
cc0c2d9 to
4047f24
Compare
There was a problem hiding this comment.
Looks good to me. These 2 questions remain though:
- what about the block hash? don't we need to exclude the ExtraData from the block hash calculation so that we can create the same hashes when reconstructing blocks from L1?
- how does an upgrade work with the new consensus mechanism? this is a mandatory upgrade, so node operators need to upgrade before a hard fork. but how exactly do we support and switch the consensus mechanism from clique to system contract based on a fork given in the config?
ranchalp
force-pushed
the
feat-system-config-consensus
branch
3 times, most recently
from
4e0c302 to
7cbad55
Compare
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
7cbad55 to
b68d2d6
Compare
|
In order to only verify the current signature if the block was not requested, I modify the Then the Thoughts? @jonastheis @Thegaram |
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
21c43e3 to
0e1fb13
Compare
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
0e1fb13 to
ec2e3d0
Compare
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
8f785f6 to
20185f2
Compare
|
Thanks @jonastheis. Addressed the comments. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
miner/scroll_worker.go (1)
362-366: Extended error handling for unauthorized signer errors.The error handling has been enhanced to include specific handling for
system_contract.ErrUnauthorizedSigner, introducing a 5-second sleep to allow time for the system contract consensus to update the signer information locally.This approach makes sense as a retry mechanism for transient authorization issues.
Consider extracting the 5-second duration into a named constant to improve code maintainability and make the value more visible:
+const unauthorizedSignerRetryDelay = 5 * time.Second // mainLoop is a standalone goroutine to regenerate the sealing task based on the received event. func (w *worker) mainLoop() { // ... if errors.Is(err, system_contract.ErrUnauthorizedSigner) { - time.Sleep(5 * time.Second) // half the time it takes for the system contract consensus to read and update the address locally. + time.Sleep(unauthorizedSignerRetryDelay) // half the time it takes for the system contract consensus to read and update the address locally. } // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
les/downloader/downloader.go(1 hunks)les/downloader/queue.go(3 hunks)les/downloader/resultstore.go(1 hunks)miner/scroll_worker.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- les/downloader/resultstore.go
- les/downloader/downloader.go
- les/downloader/queue.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (1)
miner/scroll_worker.go (1)
22-22: Import added for the new system contract package.This adds the import for the newly introduced system contract package, which is part of the new consensus mechanism being implemented.
ranchalp
force-pushed
the
feat-system-config-consensus
branch
3 times, most recently
from
41b85b9 to
280f428
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/types/block.go (4)
111-113: Consider documenting the use of theRequestedfield.The
Requestedfield is used internally by the downloader queue to track if a header was requested, but this is only documented in a brief comment. Consider adding a more detailed comment explaining its specific purpose and why it's excluded from network serialization.
190-196: Consider null checks for BlockSignature before assignment.When preparing for network transmission, you assign
h.Extra = h.BlockSignaturewithout checking ifBlockSignatureis nil. While likely not an issue in practice if the code path is only executed for valid EuclidV2 blocks, adding a null check would improve robustness.func (h *Header) PrepareForNetwork() { if h.IsEuclidV2 { h.IsEuclidV2 = false - h.Extra = h.BlockSignature + if h.BlockSignature != nil { + h.Extra = h.BlockSignature + } else { + h.Extra = nil + } h.BlockSignature = nil } }
198-204: Similar null check recommendation for PrepareFromNetwork.Similarly to the previous comment, when preparing from network, you assign
h.BlockSignature = h.Extrawithout checking ifExtrais nil. Consider adding a null check here as well for consistency and robustness.func (h *Header) PrepareFromNetwork(isEuclidV2 bool) { if isEuclidV2 { h.IsEuclidV2 = true - h.BlockSignature = h.Extra + if h.Extra != nil { + h.BlockSignature = h.Extra + } h.Extra = nil } }
506-520: Clarify the comment about header copying.The comments in the implementation indicate that only the slice header is copied for
unclesandtransactionswhile the underlying arrays are shared. However, the function still performsCopyHeader(header)which creates a deep copy. Consider clarifying the comment to explain that the header is being deep copied while other fields have different copy semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/types/block.go(8 hunks)eth/backend.go(4 hunks)eth/downloader/downloader.go(1 hunks)eth/downloader/queue.go(2 hunks)eth/protocols/eth/handlers.go(3 hunks)les/downloader/queue.go(2 hunks)miner/scroll_worker.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- eth/protocols/eth/handlers.go
- eth/downloader/queue.go
- eth/downloader/downloader.go
- les/downloader/queue.go
- eth/backend.go
🔇 Additional comments (7)
miner/scroll_worker.go (3)
22-22: Added import for the system_contract package.The new import adds the necessary reference to the system contract consensus mechanism that is being introduced to replace clique consensus.
362-363: Enhanced error handling to support system contract consensus.The condition is expanded to also check for
system_contract.ErrUnauthorizedSignererrors, ensuring that unauthorized signer errors from the new consensus mechanism can trigger the retry logic.
364-366: Added delay for unauthorized signer errors.A 5-second sleep is introduced when encountering an unauthorized signer error. This provides time for the system contract consensus to read and update the address locally, increasing the chance of success on retry.
Can we verify that 5 seconds is an appropriate delay? The comment suggests it's "half the time it takes for the system contract consensus to read and update the address locally," but it would be good to confirm this timing is optimal for the expected update frequency.
core/types/block.go (4)
89-94: New fields for EuclidV2 consensus engine look good.The addition of
BlockSignatureandIsEuclidV2fields to theHeaderstruct enables the new consensus mechanism while properly excluding them from JSON representation and marking them as optional in RLP encoding, which keeps them from being included in network transmissions and block hashing.
129-137: Robust approach for conditional hashing of headers.The updated
Hash()method properly ensures that EuclidV2-specific fields are excluded from the block hash, which is crucial for network compatibility. This implementation aligns with the prior discussions in the PR comments about handling block signatures.
171-177: Good validation of network compatibility.This validation method ensures that EuclidV2 fields can't be transmitted over the network, preventing potential misuse and maintaining separation between internal and network representation of headers.
304-307: Good deep copy implementation for BlockSignature.The update to the
CopyHeaderfunction properly handles the deep copying of theBlockSignaturefield, which is necessary for maintaining data integrity when copying headers.
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
f871d94 to
d1e4790
Compare
jonastheis
force-pushed
the
feat-system-config-consensus
branch
from
c9bf298 to
67be798
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
eth/backend.go (1)
115-115: 💡 Verification agent🧩 Analysis chain
Function signature update requires changes in all callers.
The
Newfunction signature has been updated to accept an additionall1Clientparameter, which is needed for the new consensus mechanism. This is a breaking change that requires all callers to be updated.
🏁 Script executed:
#!/bin/bash # Find all places where eth.New is called to verify they've been updated with the new parameter rg "eth\.New\(" --type goLength of output: 937
Action Required: Update All Callers of
eth.Newto Pass a Validl1ClientThe function signature for
eth.Newnow requires an additionall1Clientparameter for the new consensus mechanism. While some call sites (e.g., incmd/utils/flags.go) have been correctly updated to pass a properl1Client, many other callers—particularly within various test files (such as inconsole/console_test.go,eth/catalyst/api_test.go,ethclient/ethclient_test.go,ethclient/gethclient_test.go,les/api_test.go,graphql/graphql_test.go) and miner stress tests—are still passingnil.Please ensure that all callers are updated to either provide a valid
l1Clientimplementation or, ifnilis intentional for test scenarios, confirm that this behavior is acceptable with the new consensus mechanism. This is a breaking change, so double-check each usage to avoid potential runtime issues.
♻️ Duplicate comments (1)
eth/backend.go (1)
161-161:⚠️ Potential issueFix l1Client type compatibility issue.
The l1.Client type must implement sync_service.EthClient interface, which requires the StorageAt method.
This is the same issue identified in the previous review. The l1Client needs to implement the StorageAt method to be compatible with sync_service.EthClient interface.
🧹 Nitpick comments (1)
core/types/block.go (1)
111-113: Consider documenting the Requested field more thoroughlyThis field appears to be used internally by the downloader queue, but the comment is somewhat vague. Consider adding more detailed documentation explaining:
- When this field is set/unset
- Which components interact with it
- Any invariants that must be maintained
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
core/types/block.go(7 hunks)eth/backend.go(4 hunks)eth/downloader/downloader.go(2 hunks)eth/downloader/queue.go(2 hunks)eth/downloader/resultstore.go(1 hunks)eth/protocols/eth/handlers.go(3 hunks)les/downloader/queue.go(2 hunks)miner/scroll_worker.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- eth/downloader/resultstore.go
- eth/downloader/queue.go
- eth/protocols/eth/handlers.go
- les/downloader/queue.go
- eth/downloader/downloader.go
- miner/scroll_worker.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (9)
core/types/block.go (6)
89-94: New Header fields look well-structuredThe new fields
BlockSignatureandIsEuclidV2are correctly tagged withjson:"-"andrlp:"optional"to ensure they're excluded from JSON serialization and made optional in RLP encoding. This aligns with the previous discussion about handling these fields properly for hashing and network compatibility.
129-137: Hash method implementation ensures proper block identityThe implementation correctly handles the Euclid V2 fields to ensure they don't affect the block hash, which is crucial for maintaining consistent block identity across the network. The approach of creating a copy and clearing specific fields before hashing aligns with the discussed design.
171-177: Good security check for network compatibilityThe
NetworkCompatibleEuclidV2Fieldsmethod properly enforces that the internal fields aren't present when blocks are received from the network, which is an important security measure.
190-204: Network preparation methods handle the field transformations correctlyThe implementation correctly manages the state transitions between internal representation (using
BlockSignature) and network representation (usingExtra). This ensures compatibility while maintaining the desired hashing behavior.
304-307: Proper deep copy of BlockSignature in CopyHeaderThe implementation correctly creates a deep copy of the
BlockSignaturefield, which is essential for maintaining isolation when headers are copied.
506-520: CopyBlockDeepWithHeader implementation needs careful handlingThe method correctly performs a deep copy of the header but shares references to other fields (transactions, uncles). This means modifications to those fields in the original block will affect the copy too.
The code comments explain this behavior, which is good, but be cautious when using this method since it's not a complete deep copy of the block.
Ensure that any code calling this method understands the shared references and doesn't rely on complete isolation from the original block.
eth/backend.go (3)
35-36: LGTM! New consensus mechanism imports added.The new imports for
system_contractandwrapperconsensus packages align with the PR objective to introduce a new consensus mechanism to replace clique.
284-285: LGTM! Good documentation of consensus transition.The comment clearly explains the transition from Clique consensus to SystemContract consensus and how extraData is handled differently after EuclidV2.
534-555: Support for new consensus engines properly added.The StartMining method now appropriately handles both new consensus engine types: UpgradableEngine and SystemContract. The error handling and wallet authorization follow the same pattern as the existing Clique implementation.
Note that for UpgradableEngine, the same SignData function is passed twice - I assume this is intentional for the wrapper to have separate signing functions for different purposes.
…r handling when fetching address
jonastheis
force-pushed
the
feat-system-config-consensus
branch
from
67be798 to
3c2347e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
consensus/system_contract/system_contract_test.go (1)
24-173: Consider adding negative test casesWhile the current tests cover the happy path and authorization mismatch scenarios well, consider adding tests for other edge cases:
- Error handling when L1 client fails (returning errors from the fake client)
- Testing the
Close()method behavior- Testing with nil client or configuration values
These would further strengthen the test coverage of the system contract implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
consensus/system_contract/system_contract.go(1 hunks)consensus/system_contract/system_contract_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- consensus/system_contract/system_contract.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (6)
consensus/system_contract/system_contract_test.go (6)
1-20: Well-structured test file with appropriate importsThe imports and package structure look good. The file uses a mix of standard library and project-specific imports that support the test functionality properly.
22-22: Good practice: Interface compliance verificationExcellent use of the blank identifier to verify that
fakeEthClientimplements thesync_service.EthClientinterface at compile time.
24-50: Test case is well-structured with proper cleanupThis test is well-designed with proper context handling, clear expected values, and appropriate assertions. I like the use of
deferfor cleanup and the explicit timeout for the context.
95-173: Comprehensive integration test for signer updatesThis test thoroughly verifies the core functionality of the system contract consensus mechanism by:
- Starting with an incorrect signer
- Updating the authorized signer
- Verifying the update is detected
- Confirming signature operations succeed with the correct signer
The implementation includes appropriate timeouts and clearly documents the test's purpose.
175-223: Thread-safe mock client implementationGood implementation of the mock client with proper mutex protection for concurrent access to the shared
valuefield. The implementation satisfies the interface while keeping test-focused simplicity.
84-84:❓ Verification inconclusive
Verify that
extraSealis defined and accessibleThe variable
extraSealis used but not defined in this file. Please ensure it's properly exported from the main package and accessible to this test file.
🏁 Script executed:
#!/bin/bash # Check for the definition of extraSeal in the system_contract package rg "var\s+extraSeal|const\s+extraSeal" consensus/system_contract/ -A 2Length of output: 72
Attention: Verify the Declaration of
extraSealThe search within the
consensus/system_contractfolder did not reveal any definition forextraSeal. This suggests that either the variable is defined (and possibly exported under a different name or in another package) or it was inadvertently omitted.Please double-check that the variable is declared and properly exported from the appropriate package (likely from the main package) so that it can be accessed in the test. If it should be exported, consider renaming it (e.g., to
ExtraSeal) and update the corresponding references in the test file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/handler_eth_test.go (1)
475-477: Update comment to clarify the behavior change in EuclidV2The comment "This don't match anymore in EuclidV2 I think" contains a grammatical error and expresses uncertainty. Since this test case has been deliberately modified to drop peers in FastSync mode even when checkpoints match (changing from
falsetotrue), a clearer explanation of this behavior change would be helpful.-// This don't match anymore in EuclidV2 I think +// In EuclidV2, we drop the peer even when checkpoints match in FastSync mode due to the +// new consensus mechanism requiring stricter validation {downloader.FastSync, true, false, false, true, true},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/handler_eth_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
ranchalp
force-pushed
the
feat-system-config-consensus
branch
from
fe427c2 to
3c2347e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/backend.go (1)
534-555: Consider refactoring duplicate wallet retrieval code.There's duplicate code for wallet retrieval and error handling across the different consensus engine types. Consider extracting this common logic to reduce code duplication.
func (s *Ethereum) StartMining(threads int) error { // ... existing code ... + getWallet := func(eb common.Address) (accounts.Wallet, error) { + wallet, err := s.accountManager.Find(accounts.Account{Address: eb}) + if wallet == nil || err != nil { + log.Error("Etherbase account unavailable locally", "err", err) + return nil, fmt.Errorf("signer missing: %v", err) + } + return wallet, nil + } if !s.IsMining() { // ... existing code ... if wrapper, ok := s.engine.(*wrapper.UpgradableEngine); ok { - wallet, err := s.accountManager.Find(accounts.Account{Address: eb}) - if wallet == nil || err != nil { - log.Error("Etherbase account unavailable locally", "err", err) - return fmt.Errorf("signer missing: %v", err) - } + wallet, err := getWallet(eb) + if err != nil { + return err + } wrapper.Authorize(eb, wallet.SignData, wallet.SignData) } else if clique, ok := s.engine.(*clique.Clique); ok { - wallet, err := s.accountManager.Find(accounts.Account{Address: eb}) - if wallet == nil || err != nil { - log.Error("Etherbase account unavailable locally", "err", err) - return fmt.Errorf("signer missing: %v", err) - } + wallet, err := getWallet(eb) + if err != nil { + return err + } clique.Authorize(eb, wallet.SignData) } else if systemContract, ok := s.engine.(*system_contract.SystemContract); ok { - wallet, err := s.accountManager.Find(accounts.Account{Address: eb}) - if wallet == nil || err != nil { - log.Error("Etherbase account unavailable locally", "err", err) - return fmt.Errorf("signer missing: %v", err) - } + wallet, err := getWallet(eb) + if err != nil { + return err + } systemContract.Authorize(eb, wallet.SignData) } // ... rest of the function ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/backend.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: check
🔇 Additional comments (6)
eth/backend.go (6)
35-36: LGTM: Added imports for new consensus mechanisms.The new imports for system contract and wrapper consensus engines align with the PR objective to replace the Clique consensus mechanism.
115-115: Function signature updated to include L1 client.The New function now accepts an L1 client parameter, which is needed for the new consensus mechanism to read allowed signers from Layer 1.
161-161: Ensure L1Client implements the required interface.The pipeline previously failed because l1.Client did not implement sync_service.EthClient (missing StorageAt method).
#!/bin/bash # Check if l1.Client implements sync_service.EthClient ast-grep --pattern $'type EthClient interface { $$$ StorageAt($$$) $$$ }' # Check l1.Client implementation ast-grep --pattern $'type Client struct { $$$ }' # Check for StorageAt implementation ast-grep --pattern $'func (c *Client) StorageAt($$$) $$$'
284-285: LGTM: Clear documentation of consensus transition.Good comment explaining the transition from Clique consensus (before EuclidV2) to SystemContract consensus, and how it affects the extraData field.
534-541: New support for UpgradableEngine consensus.Added support for the wrapper.UpgradableEngine consensus type, which requires two SignData functions for authorization. This aligns with the PR goal of supporting a new consensus mechanism.
548-555: New support for SystemContract consensus.Added support for the system_contract.SystemContract consensus type, which implements the consensus.Engine interface. This is the core of the PR's objective to replace Clique consensus.
|
Fixed the tests, the issue was 2 fold:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eth/handler_eth_test.go (1)
66-71: Prefer a testing framework approach to handle chain creation errors instead of panicking.
Usingt.Fatalf(...)or a similar testing function explicitly flags test failures and makes them more visible than a rawpanic(...).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/handler_eth_test.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
eth/handler_eth_test.go (3)
21-21: No issues with the new import.
The newly introducedcryptoimport is consistent with the usage for ECDSA operations at line 59.
528-528: Confirm removal of 'Extra' field.
RemovingExtramight reduce visibility or debugging options in some scenarios. If you no longer need the data it holds, then this is fine. Otherwise, consider reinstating it for clarity.
588-589: Mismatch in block number effectively simulates negative test coverage.
This is a good strategy to confirm that mismatched headers lead to the peer being dropped as expected.
| func (h *testEthHandler) Chain() *core.BlockChain { | ||
| chainConfig := ¶ms.ChainConfig{} | ||
| engine := ethash.NewFaker() | ||
| chaindb := rawdb.NewMemoryDatabase() | ||
|
|
||
| // Import the canonical chain | ||
| cacheConfig := &core.CacheConfig{} | ||
| key1, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle ECDSA key creation error and define proper chain config.
Ignoring the error from crypto.HexToECDSA can mask potential issues if the hex key is invalid. Also, consider specifying relevant fork blocks or chain parameters in ChainConfig if you intend to test particular chain forks.
Here’s a suggested fix:
- key1, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
+ key1, err := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291")
+ if err != nil {
+ panic(fmt.Sprintf("Error generating ECDSA key: %v", err))
+ }📝 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.
| func (h *testEthHandler) Chain() *core.BlockChain { | |
| chainConfig := ¶ms.ChainConfig{} | |
| engine := ethash.NewFaker() | |
| chaindb := rawdb.NewMemoryDatabase() | |
| // Import the canonical chain | |
| cacheConfig := &core.CacheConfig{} | |
| key1, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") | |
| func (h *testEthHandler) Chain() *core.BlockChain { | |
| chainConfig := ¶ms.ChainConfig{} | |
| engine := ethash.NewFaker() | |
| chaindb := rawdb.NewMemoryDatabase() | |
| // Import the canonical chain | |
| cacheConfig := &core.CacheConfig{} | |
| key1, err := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") | |
| if err != nil { | |
| panic(fmt.Sprintf("Error generating ECDSA key: %v", err)) | |
| } | |
| // ... (rest of the code) | |
| } |
|
One question regarding the design. Based on my understanding, when blocks are actively requested from peers, we do not verify the signatures. In this scenario, if it's possible a malicious node deceive others by fraudulently broadcasting that he is in a high totalDifficulty block, and lure other nodes to request those blocks from him. And nodes will not check the signature, so those unauthorized and malicious blocks will be added? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
eth/handler_eth_test.go (1)
60-60: 🛠️ Refactor suggestionHandle ECDSA key creation error.
The error from
crypto.HexToECDSAis being ignored, which could mask potential issues if the hex key is invalid.Apply this fix:
- key1, _ := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") + key1, err := crypto.HexToECDSA("b71c71a67e1177ad4e901695e1b4b9ee17ae16c6668d313eac2f96dbcda3f291") + if err != nil { + panic(fmt.Sprintf("failed to create ECDSA key: %v", err)) + }
🧹 Nitpick comments (4)
miner/scroll_worker.go (1)
365-367: Consider implementing exponential backoff for retry delaysWhile the 5-second sleep is a reasonable approach to handle unauthorized signer errors, a more robust implementation might use an exponential backoff strategy with a maximum retry limit to prevent indefinite retries.
if errors.Is(err, system_contract.ErrUnauthorizedSigner) { - time.Sleep(5 * time.Second) // half the time it takes for the system contract consensus to read and update the address locally. + // Use exponential backoff with a maximum of 5 retries + retryCount := atomic.AddUint32(&w.unauthorizedSignerRetryCount, 1) + if retryCount > 5 { + log.Warn("Maximum retry attempts reached for unauthorized signer", "retries", retryCount) + atomic.StoreUint32(&w.unauthorizedSignerRetryCount, 0) + } else { + backoffTime := time.Duration(math.Min(float64(5*time.Second)*math.Pow(1.5, float64(retryCount-1)), float64(30*time.Second))) + log.Info("Waiting for system contract consensus to update", "retry", retryCount, "sleep", backoffTime) + time.Sleep(backoffTime) + } }You would need to add a new field to the worker struct:
unauthorizedSignerRetryCount uint32consensus/wrapper/consensus.go (3)
185-191: Improve error handling in Close method.The current implementation closes the Clique engine first and returns if there's an error. This could leave the System engine unclosed if Clique's Close returns an error, potentially causing resource leaks.
Consider using a multi-error approach to ensure both engines are closed regardless of errors:
func (ue *UpgradableEngine) Close() error { - // Always close both engines. - if err := ue.clique.Close(); err != nil { - return err - } - return ue.system.Close() + // Close both engines and collect errors + cliqueErr := ue.clique.Close() + systemErr := ue.system.Close() + + // Return the first non-nil error, if any + if cliqueErr != nil { + return cliqueErr + } + return systemErr }
38-44: Add logging for consensus engine transitions.When a block transitions from one consensus engine to another, this is a significant event that should be logged to facilitate debugging and monitoring. Consider adding logging statements when the engine changes.
func (ue *UpgradableEngine) chooseEngine(header *types.Header) consensus.Engine { if ue.isUpgraded(header.Time) { + // Log only when transitioning from clique to system + parent := header.ParentHash + if parent != (common.Hash{}) && !ue.isUpgraded(header.Time-1) { + log.Info("Transitioning to SystemContract consensus engine", "number", header.Number, "hash", header.Hash()) + } return ue.system } return ue.clique }
59-136: Enhance error reporting in VerifyHeaders method.The current implementation merges errors from two verification streams without indicating which engine produced which error. This could make debugging more difficult when verification fails.
Consider wrapping errors with the source engine's name:
go func() { defer wg.Done() _, cliqueResults := ue.clique.VerifyHeaders(chain, cliqueHeaders, cliqueSeals) for err := range cliqueResults { + // Wrap the error with the engine name + wrappedErr := err + if err != nil { + wrappedErr = fmt.Errorf("clique: %w", err) + } select { case <-abort: return - case out <- err: + case out <- wrappedErr: } } }()And similarly for the system engine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
consensus/system_contract/consensus.go(1 hunks)consensus/wrapper/consensus.go(1 hunks)eth/downloader/downloader.go(2 hunks)eth/downloader/queue.go(3 hunks)eth/downloader/resultstore.go(1 hunks)eth/handler_eth_test.go(5 hunks)eth/protocols/snap/sync_test.go(1 hunks)eth/tracers/js/internal/tracers/assets.go(1 hunks)eth/tracers/native/4byte.go(1 hunks)miner/scroll_worker.go(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- eth/tracers/js/internal/tracers/assets.go
- eth/protocols/snap/sync_test.go
- eth/tracers/native/4byte.go
🚧 Files skipped from review as they are similar to previous changes (4)
- eth/downloader/resultstore.go
- eth/downloader/queue.go
- eth/downloader/downloader.go
- consensus/system_contract/consensus.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
miner/scroll_worker.go (2)
28-29: New system_contract package importThis import introduces the system_contract package which is part of the new consensus mechanism that's replacing the Clique consensus.
363-363: Enhanced error handling for the new consensus mechanismAdding the check for
system_contract.ErrUnauthorizedSignerensures that unauthorized signer errors from the new consensus mechanism are treated as retryable errors, maintaining system resilience.eth/handler_eth_test.go (3)
53-72: LGTM: Implementation of Chain() method provides proper test infrastructure.The new implementation replaces the previous "no backing chain" panic with a proper blockchain implementation for testing. This better facilitates tests involving the new consensus mechanism.
529-529: Removal of Extra field aligns with consensus changes.Removing the
Extrafield from the header is consistent with the PR's objective to ensure theextrafield is empty in the new consensus mechanism.
589-590: Changed mismatch verification approach.Previously, the mismatch test used an invalid
Extrafield. Now it creates a mismatch by incrementing the block number instead, which aligns with the change in consensus verification approach.consensus/wrapper/consensus.go (3)
19-20: Use block number instead of timestamp for fork checks.The comment mentions "forkBlock is the block number" but the code uses
header.Timewhich is a timestamp. Block timestamps can be manipulated and differ from a stable block height. Consider using block number for fork detection instead, or update the comment to reflect the actual implementation.
148-151:❓ Verification inconclusive
Potential CalcDifficulty issue for blocks near fork transition.
The
CalcDifficultymethod uses the parent header to choose the engine, but this might cause issues around the fork transition. If a parent block is pre-fork but the new block should be post-fork, you'll use the wrong engine to calculate difficulty.
🏁 Script executed:
#!/bin/bash # Check if there's any special handling for difficulty calculation around fork transitions # Look for any handling of difficulty transitions rg -A 5 "CalcDifficulty|difficulty" --type go | grep -v "consensus/wrapper/consensus.go" | grep -E "fork|transition|upgrade" # Check how fork transitions are handled in system_contract consensus rg -A 10 "CalcDifficulty" --type go --glob "consensus/system_contract/**"Length of output: 3384
Action Required: Verify Consensus Engine Selection During Fork Transition
The current implementation in
consensus/wrapper/consensus.go(lines 148–151) directly uses the parent header to determine the consensus engine viachooseEngine. Our investigation confirms that similar fork/transition logic is handled elsewhere (e.g., in tests withinconsensus/cliqueand transition-related code incmd/evm/internal/t8ntool/transition.go), but there’s no indication that additional checks exist in this wrapper. This raises a concern: for blocks near a fork transition, a parent block might be pre-fork while the new block should apply post-fork rules, meaning the wrong engine could be selected.Recommendations:
- Review
chooseEngineImplementation: Ensure that it accounts for the scenario where the new block’s consensus may have advanced beyond the parent’s state.- Align with Existing Transition Logic: Compare with how transitions are managed in other modules (e.g.,
consensus/clique, tests, and the transition handling incmd/evm/internal/t8ntool/transition.go) to confirm consistency.- Add/Review Tests: Verify that there are adequate test cases covering blocks straddling the fork transition to catch any miscalculations in difficulty.
87-93:❓ Verification inconclusive
Verify one-way transition from Clique to System.
The code finds the first header that uses the system engine, but doesn't verify that subsequent headers continue to use it. This implementation assumes headers only transition one-way from Clique to System without switching back, which could lead to issues if this assumption is violated.
🏁 Script executed:
#!/bin/bash # Check if any logic outside this file relies on the one-way transition assumption # Look for any code that switches between consensus engines based on conditions rg -A 3 -B 3 "consensus.Engine|UpgradableEngine" --type go | grep -v "consensus/wrapper/consensus.go" # Check for any potential implementations that might switch back to Clique from System rg -A 5 "isUpgraded|forkBlock" --type go | grep -v "consensus/wrapper/consensus.go"Length of output: 42219
Action: Confirm and Enforce Monotonic Transition to the System Engine
The current logic in
consensus/wrapper/consensus.go(lines 87–93) locates the first header that switches to the System engine by breaking the loop as soon as it’s found. However, this approach implicitly assumes that once a header uses the System engine, every subsequent header will also do so. Our repository search did not reveal any checks or safeguards ensuring that headers following the transition continue to adhere to the System engine.
- Verify that headers after the detected transition do not revert to using the Clique engine.
- Consider adding an explicit verification or safeguard ensuring a one-way, monotonically increasing transition to the System engine to prevent potential issues if the assumption is violated.
| // APIs returns any RPC APIs exposed by the consensus engine. | ||
| func (ue *UpgradableEngine) APIs(chain consensus.ChainHeaderReader) []rpc.API { | ||
| // Determine the current chain head. | ||
| head := chain.CurrentHeader() | ||
| if head == nil { | ||
| // Fallback: return the clique APIs (or an empty slice) if we don't have a header. | ||
| return ue.clique.APIs(chain) | ||
| } | ||
|
|
||
| // Choose engine based on whether the chain head is before or after the fork block. | ||
| if ue.isUpgraded(head.Time) { | ||
| return ue.system.APIs(chain) | ||
| } | ||
| return ue.clique.APIs(chain) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add unit tests for APIs method.
The APIs method has special logic to determine which engine's APIs to return based on the current chain head. This logic is complex enough to warrant dedicated unit tests to ensure it behaves correctly in different scenarios, especially when the chain head is nil or around fork transitions.
🏁 Script executed:
#!/bin/bash
# Check if there are any existing tests for UpgradableEngine
# Look for test files for wrapper consensus
fd ".*test.*\.go" consensus/wrapper
# Check for tests of similar consensus wrappers
rg -l "func Test.*Engine" --type go consensusLength of output: 85
Missing Unit Tests for UpgradableEngine.APIs Method – Please Cover Edge Cases
It appears that there are currently no unit tests covering the APIs method in the consensus engine wrapper. Please add dedicated tests to ensure that the method behaves correctly in all scenarios. In particular, your tests should verify that:
- When
chain.CurrentHeader()returnsnil, the method falls back to callingue.clique.APIs(chain). - For a valid chain header with a timestamp before the fork, the method returns
ue.clique.APIs(chain). - For a valid chain header with a timestamp after the fork, the method returns
ue.system.APIs(chain).
Code snippet for context (unchanged):
// APIs returns any RPC APIs exposed by the consensus engine.
func (ue *UpgradableEngine) APIs(chain consensus.ChainHeaderReader) []rpc.API {
// Determine the current chain head.
head := chain.CurrentHeader()
if head == nil {
// Fallback: return the clique APIs (or an empty slice) if we don't have a header.
return ue.clique.APIs(chain)
}
// Choose engine based on whether the chain head is before or after the fork block.
if ue.isUpgraded(head.Time) {
return ue.system.APIs(chain)
}
return ue.clique.APIs(chain)
}Please add these tests to improve our confidence in the edge-case handling of the API logic.
| func (ue *UpgradableEngine) Authorize(signer common.Address, signFn clique.SignerFn, signFn2 system_contract.SignerFn) { | ||
| if cliqueEngine, ok := ue.clique.(*clique.Clique); ok { | ||
| cliqueEngine.Authorize(signer, signFn) | ||
| } | ||
| if sysContractEngine, ok := ue.system.(*system_contract.SystemContract); ok { | ||
| sysContractEngine.Authorize(signer, signFn2) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle type assertion failures in Authorize method.
The current implementation uses type assertions without checking if they succeed. If either engine is wrapped by another middleware, these assertions could fail silently, leading to unexpected behavior where block signing doesn't work.
Add error handling for the type assertions:
func (ue *UpgradableEngine) Authorize(signer common.Address, signFn clique.SignerFn, signFn2 system_contract.SignerFn) {
if cliqueEngine, ok := ue.clique.(*clique.Clique); ok {
cliqueEngine.Authorize(signer, signFn)
+ } else {
+ // Log warning or handle appropriately if clique engine is not of expected type
+ log.Warn("Clique engine is not of type *clique.Clique, Authorize call ignored")
}
if sysContractEngine, ok := ue.system.(*system_contract.SystemContract); ok {
sysContractEngine.Authorize(signer, signFn2)
+ } else {
+ // Log warning or handle appropriately if system engine is not of expected type
+ log.Warn("System engine is not of type *system_contract.SystemContract, Authorize call ignored")
}
}📝 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.
| func (ue *UpgradableEngine) Authorize(signer common.Address, signFn clique.SignerFn, signFn2 system_contract.SignerFn) { | |
| if cliqueEngine, ok := ue.clique.(*clique.Clique); ok { | |
| cliqueEngine.Authorize(signer, signFn) | |
| } | |
| if sysContractEngine, ok := ue.system.(*system_contract.SystemContract); ok { | |
| sysContractEngine.Authorize(signer, signFn2) | |
| } | |
| } | |
| func (ue *UpgradableEngine) Authorize(signer common.Address, signFn clique.SignerFn, signFn2 system_contract.SignerFn) { | |
| if cliqueEngine, ok := ue.clique.(*clique.Clique); ok { | |
| cliqueEngine.Authorize(signer, signFn) | |
| } else { | |
| // Log warning or handle appropriately if clique engine is not of expected type | |
| log.Warn("Clique engine is not of type *clique.Clique, Authorize call ignored") | |
| } | |
| if sysContractEngine, ok := ue.system.(*system_contract.SystemContract); ok { | |
| sysContractEngine.Authorize(signer, signFn2) | |
| } else { | |
| // Log warning or handle appropriately if system engine is not of expected type | |
| log.Warn("System engine is not of type *system_contract.SystemContract, Authorize call ignored") | |
| } | |
| } |
1. Purpose or design rationale of this PR
This PR adds the
SystemContractconsensus to replaceCliqueand effectively fixes the block hash mismatch issue. It periodically reads the allowed signer from L1 and gossiped blocks (not requested) are verified against this address. Project docWe make some specific changes to the consensus/block header validation:
difficultyneeds to be1extraneeds to be emptySpecifically, this implementation uses a workaround to enable this within l2geth:
json:"-" rlp="optional"fields (BlockSignatureandIsEuclidV2) are introduced that are only used internally within l2geth. These fields are ignored when hashing the header and are not directly send to peers over the wire.json:"-" rlp:"-"field (Requested) is introduced to track locally whether a block has been requested or was received via gossip: we only verify signatures of gossiped blocks. This is to enable nodes to sync if the signer on L1 changed in the meantime.To ensure this works properly some changes within the network stack are made:
EuclidV2):BlockSignatureandIsEuclidV2otherwise the sending peer is droppedextratoBlockSignatureand setsisEuclidV2accordinglyRequestedand subsequently the signature is not verifiedBlockSignatureandIsEuclidV2and moves the signature toextra2. 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
New Features:
EthClientandClientinterfaces for improved data retrieval capabilities.Chores: