fix: provide parent header during EuclidV2 transition verification #1187
Conversation
WalkthroughThe signatures of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant UpgradableEngine
participant Clique
participant SystemContract
Caller->>UpgradableEngine: VerifyHeaders(chain, headers, seals, parent)
alt Headers belong to Clique consensus
UpgradableEngine->>Clique: VerifyHeaders(chain, headers, seals, parent)
else Headers belong to SystemContract consensus
UpgradableEngine->>SystemContract: VerifyHeaders(chain, headers, seals, parent)
end
Clique-->>UpgradableEngine: (abortCh, resultCh)
SystemContract-->>UpgradableEngine: (abortCh, resultCh)
UpgradableEngine-->>Caller: (abortCh, resultCh)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (2)
consensus/wrapper/consensus.go (2)
61-66: Document the newparentparameter
VerifyHeaders’s godoc comment has not been updated to mention the newparent *types.Headerhint. Future readers (and ‑ve tools that rely on comments) will have no clue what the extra argument is for.Please add a short sentence such as “parent — (optional) previously-verified header that immediately precedes the first element of
headers.”
110-113: Consider re-using the outerparentfor the Clique segmentAlthough Clique currently ignores the hint, passing it through keeps the API usage symmetrical and avoids ad-hoc
nils sprinkled through the code:- abortClique, cliqueResults := ue.clique.VerifyHeaders(chain, cliqueHeaders, cliqueSeals, nil) + abortClique, cliqueResults := ue.clique.VerifyHeaders(chain, cliqueHeaders, cliqueSeals, parent)Purely cosmetic today, but future Clique upgrades (or test doubles) might appreciate the extra context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
consensus/clique/clique.go(1 hunks)consensus/consensus.go(1 hunks)consensus/ethash/consensus.go(1 hunks)consensus/system_contract/consensus.go(1 hunks)consensus/wrapper/consensus.go(4 hunks)core/block_validator_test.go(3 hunks)core/blockchain.go(1 hunks)core/headerchain.go(1 hunks)params/version.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
consensus/system_contract/consensus.go (2)
consensus/consensus.go (1)
ChainHeaderReader(32-47)core/types/block.go (1)
Header(69-122)
consensus/clique/clique.go (2)
consensus/consensus.go (1)
ChainHeaderReader(32-47)core/types/block.go (1)
Header(69-122)
consensus/wrapper/consensus.go (2)
consensus/consensus.go (1)
ChainHeaderReader(32-47)core/types/block.go (1)
Header(69-122)
consensus/ethash/consensus.go (2)
consensus/consensus.go (1)
ChainHeaderReader(32-47)core/types/block.go (1)
Header(69-122)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test
- GitHub Check: build-mock-ccc-geth
- GitHub Check: check
- GitHub Check: semgrep/ci
- GitHub Check: zizmor
- GitHub Check: goimports-lint
- GitHub Check: Analyze (go)
🔇 Additional comments (10)
params/version.go (1)
27-27: Version bump for patch release.The patch version has been incremented from 46 to 47, reflecting the non-breaking changes introduced in this PR for the EuclidV2 transition verification.
core/headerchain.go (1)
342-342: Added new parent header parameter to VerifyHeaders call.The call to
VerifyHeadershas been updated to include the newparentparameter (currentlynil), aligning with the updated method signature in the consensus engine interface. This will enable parent header context during verification when needed.core/blockchain.go (1)
1549-1549: Added new parent header parameter to VerifyHeaders call.The call to
VerifyHeadershas been updated to include the newparentparameter (currentlynil), aligning with the modified consensus engine interface. This addresses the race condition described in the PR objectives, ensuring consistency across both code paths that invoke header verification.core/block_validator_test.go (1)
54-54: Updated test calls to VerifyHeaders with new parent parameter.All test calls to
VerifyHeadershave been updated to include the newparentparameter (passed asnil), ensuring tests remain compatible with the modified consensus engine interface. No changes to the test logic or verification workflow were required.Also applies to: 57-57, 110-110, 114-114, 179-179
consensus/consensus.go (1)
74-74: Interface correctly updated to support parent header context.The
VerifyHeadersmethod in theEngineinterface has been appropriately updated to include aparent *types.Headerparameter. This change enables consensus engines to access the parent header during batch verification, which is particularly useful for the first header in a batch when ancestor headers may not be available in the chain yet.consensus/system_contract/consensus.go (3)
89-89: Method signature correctly updated to match interface changes.The
VerifyHeadersmethod signature now includes the newparent *types.Headerparameter to match the updatedEngineinterface.
95-98: Appropriate utilization of parent header parameter.This logic now properly handles the case where the first header in the batch needs verification but has no preceding headers in the batch itself. By using the provided
parentparameter as a fallback, the method can now correctly verify the first header in scenarios where ancestor headers may not have been inserted into the chain yet.
100-100: Correctly passes parents to verifyHeader.The updated line correctly uses the potentially modified
parentsslice that may now include the provided parent header parameter.consensus/clique/clique.go (1)
229-229: Method signature updated but parameter not utilized.The
VerifyHeadersmethod signature has been properly updated to include the newparent *types.Headerparameter to match the Engine interface. However, the implementation doesn't actually use this parameter in the verification logic. This is acceptable since the clique engine already has a working mechanism to handle parent headers through theverifyHeadermethod's existing functionality.Consider reviewing whether the Clique implementation should also use the parent parameter in certain scenarios for consistency with the SystemContract implementation, particularly for the first header in a batch when no preceding headers exist.
consensus/ethash/consensus.go (1)
122-122: Method signature updated but parameter not utilized.The
VerifyHeadersmethod signature has been properly updated to include the newparent *types.Headerparameter to match the Engine interface. However, the implementation doesn't actually use this parameter in the verification logic. This is acceptable since the ethash verification process already handles parent header retrieval through theverifyHeaderWorkermethod.Consider reviewing whether the Ethash implementation should also use the parent parameter in certain scenarios for consistency with the SystemContract implementation, particularly when verifying the first header in a batch.
…ransition-verification
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
consensus/system_contract/system_contract_test.go (1)
130-133: Prefer the new setter for updating the mock clientAfter introducing
FakeEthClient.SetValue, replace the manual lock manipulation:-fakeClient.mu.Lock() -fakeClient.Value = updatedSigner -fakeClient.mu.Unlock() +fakeClient.SetValue(updatedSigner)This keeps tests concise and lock-error-free.
🧹 Nitpick comments (1)
miner/scroll_worker_test.go (1)
230-238: Avoid aliasing a global when selecting the worker config
testWorkerassignsconfig := testConfig, meaning the worker receives the shared pointer.
IfnewWorkeror the worker mutates it, every other test that reusestestConfiginherits those changes.After introducing
newTestConfig, this block simplifies to:- config := testConfig - if allowEmpty { - config = testConfigAllowEmpty - } +config := newTestConfig(allowEmpty)This guarantees each worker works with its own copy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
consensus/system_contract/system_contract.go(2 hunks)consensus/system_contract/system_contract_test.go(4 hunks)miner/scroll_worker_test.go(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
consensus/system_contract/system_contract.go (2)
interfaces.go (1)
FilterQuery(144-162)core/types/block.go (2)
Header(69-122)Block(223-241)
consensus/system_contract/system_contract_test.go (4)
rollup/sync_service/types.go (1)
EthClient(14-23)consensus/system_contract/system_contract.go (1)
FakeEthClient(148-153)log/handler.go (1)
DiscardHandler(333-337)common/types.go (1)
HexToAddress(218-218)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
miner/scroll_worker_test.go (1)
1411-1469: Re-use of the same consensus engine across two blockchains can leak state
engine(anUpgradableEngineinstance) is passed to two independentcore.NewBlockChaininstances (chainand the secondchainon lines 1460-1464).
If the engine caches chain-specific data (e.g., header snapshots, signer sets), the second blockchain may read stale data, giving false-negative results or masking bugs.Creating a fresh engine for the second import isolates the test cases:
-chain, err := core.NewBlockChain(chainDb, nil, b.chain.Config(), engine, vm.Config{}, nil, nil) +freshClique := clique.New(chainConfig.Clique, chainDb) +freshSys := system_contract.New(context.Background(), chainConfig.SystemContract, + &system_contract.FakeEthClient{Value: testBankAddress}) +freshEngine := wrapper.NewUpgradableEngine(chainConfig.IsEuclidV2, freshClique, freshSys) +chain, err := core.NewBlockChain(chainDb, nil, b.chain.Config(), freshEngine, vm.Config{}, nil, nil)This keeps the test hermetic and closer to real-world usage where separate nodes hold independent engine instances.
Thegaram
deleted the
fix-provide-parent-header-during-euclid-transition-verification
branch
1. Purpose or design rationale of this PR
This is a follow-up PR to #1186.
While that PR correctly handled a race condition, it did not address the real root cause, which is that
engine.VerifyHeaderscan be triggered from two different code paths:BlockChain.insertChain, which would do header verification and insertion in parallel (pipelined).BlockChain.InsertHeaderChain, which does blocking verification, and then inserts the header chain.The upgradable engine fails in (2), because calling
SystemConfig.VerifyHeadersassumes that the ancestor of this chain had already been inserted.To address this edge case, we pass an optional
parenthint toVerifyHeaders, which should work in both cases.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
New Features
Bug Fixes
Chores