Feat: Sequencer Final PR#331
Conversation
Add engine_newL2BlockV2 which relaxes the parent constraint from "must be currentHead" to "must exist", allowing SetCanonical to automatically handle chain reorganization internally. - NewL2BlockV2(params, isSafe): only validates parent exists and block number == parent+1; reorg detection delegated to SetCanonical - Fix reorg() canonical hash deletion start point (commonBlock.Number) to correctly handle short-chain reorgs where len(newChain) <= 1 - Add authclient wrapper for engine_newL2BlockV2 RPC call - Unit tests: 6 subtests covering sequential, reorg, parent-not-found, wrong-number, safe-path, consecutive-reorg Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reject NewL2BlockV2 when the declared hash differs from the header hash recomputed from the assembled block. Reject in writeBlockStateWithoutHead when header.NextL1MsgIndex differs from the value derived from the L1 message stream. Companion changes in tendermint and morph/node. Made-with: Cursor
…L2BlockV2 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds NewL2BlockV2 and parent-pinning for safe blocks, validates header.NextL1MsgIndex before persisting, fixes short-chain reorg canonical-hash cleanup, updates AssembleL2BlockV2/SafeL2Data JSON, and adds client RPC wiring plus tests. ChangesL2 Reorg API and Security Validation
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@eth/catalyst/l2_api.go`:
- Around line 499-502: The verified-cache hit path returns nil for the header
causing callers to receive a nil parent; change the return in the isVerified
branch to return the block's header instead of nil so both paths return a valid
*types.Header. Specifically, in the block where api.isVerified(block.Hash()) is
true, replace the current "return nil, bc.WriteStateAndSetHead(block,
bas.receipts, bas.state, bas.procTime)" with "return block.Header(),
bc.WriteStateAndSetHead(block, bas.receipts, bas.state, bas.procTime)" so
api.isVerified, UpdateBlockProcessMetrics and bc.WriteStateAndSetHead keep
behavior but the header is non-nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 326bd641-3ae4-4db9-8889-2fe29a058223
📒 Files selected for processing (5)
core/blockchain.gocore/blockchain_l2.goeth/catalyst/l2_api.goeth/catalyst/l2_api_test.goethclient/authclient/engine.go
Consolidates the safe-write API into NewSafeL2Block by adding optional ParentHash to SafeL2Data. NewSafeL2Block now executes the block against that parent (when supplied) and lets WriteStateAndSetHead's SetCanonical auto-reorg the chain. Caller (derivation.deriveForce) only knows block contents from L1 batch data — not pre-computed execution roots — so this path is the right home for it: ProcessBlock fills StateRoot / GasUsed / Bloom / ReceiptHash / NextL1MsgIndex into the header from the resulting state, instead of trusting caller-supplied (zero) values. NewL2BlockV2 drops the isSafe parameter and the v2 isSafe branch. It is now sequencer-signed-only — caller (executor.ApplyBlockV2) supplies all execution-result fields and ProcessBlock + ValidateState verify them. The hash-mismatch and verifyBlock checks become unconditional. Removes TC-05 SafeBlock test (the path it covered now lives under TestNewSafeL2Block) and the dual-call-shape pattern in remaining tests. * api_types.SafeL2Data: add ParentHash *common.Hash (nil = legacy currentHead+1 semantics; non-nil = pin parent for reorg case) * l2_api.NewSafeL2Block: branch on ParentHash to look up parent vs fall back to CurrentBlock; rest of the flow (ProcessBlock with safe=true, header reconstruction, NextL1MsgIndex backfill from L1 message stream) unchanged * l2_api.NewL2BlockV2: signature drops isSafe bool; verifyBlock and hash check unconditional; ProcessBlock pinned to safe=false * authclient.NewL2BlockV2: client wrapper drops isSafe arg * l2_api_test.go: drop TC-05 (covered by TestNewSafeL2Block); strip isSafe arg from remaining 7 NewL2BlockV2 calls Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
eth/catalyst/l2_api.go (1)
242-262: ⚡ Quick winNew caller-pinned parent / reorg branch is untested for safe blocks.
The
params.ParentHash != nilbranch (the reorg path that relies onSetCanonicalto switch chains) has no test.TestNewSafeL2Blockonly constructsSafeL2DatawithoutParentHash, so only the legacycurrentHead+1path is covered. The equivalent reorg path forNewL2BlockV2is covered byTestNewL2BlockV2, but this safe-block path is the headline feature of the PR and exercisesProcessBlock(..., true)against a non-head parent.Consider mirroring the
Reorgsubtest fromTestNewL2BlockV2forNewSafeL2Block(pin an older parent and assert head switchover + stale canonical-hash cleanup).Want me to draft a reorg subtest for
NewSafeL2Block?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth/catalyst/l2_api.go` around lines 242 - 262, Add a new "Reorg" subtest mirroring TestNewL2BlockV2 for NewSafeL2Block: exercise the params.ParentHash != nil branch by pinning an older parent via the test blockchain (use the same header hash used in TestNewL2BlockV2), call NewSafeL2Block with ParentHash set and then call ProcessBlock(..., true) against that non-head parent, and assert that SetCanonical was applied (head switched to the new chain) and any stale canonical-hash entries were cleaned up; update TestNewSafeL2Block to include this subtest and reuse the same assertions used in the Reorg subtest of TestNewL2BlockV2 for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@eth/catalyst/l2_api.go`:
- Around line 242-262: Add a new "Reorg" subtest mirroring TestNewL2BlockV2 for
NewSafeL2Block: exercise the params.ParentHash != nil branch by pinning an older
parent via the test blockchain (use the same header hash used in
TestNewL2BlockV2), call NewSafeL2Block with ParentHash set and then call
ProcessBlock(..., true) against that non-head parent, and assert that
SetCanonical was applied (head switched to the new chain) and any stale
canonical-hash entries were cleaned up; update TestNewSafeL2Block to include
this subtest and reuse the same assertions used in the Reorg subtest of
TestNewL2BlockV2 for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08b4fa89-e28e-46f6-ab21-454203291caf
📒 Files selected for processing (5)
eth/catalyst/api_types.goeth/catalyst/gen_l2_sd.goeth/catalyst/l2_api.goeth/catalyst/l2_api_test.goethclient/authclient/engine.go
✅ Files skipped from review due to trivial changes (1)
- eth/catalyst/gen_l2_sd.go
|
Heads-up on the new The check is:
Concrete case: morph mainnet block 628697 has L1 messages with queue indices
morph-reth ran into this same case during sync earlier this year and had to relax its check from Two options worth considering:
Either way, the threat model in the doc-comment (signature replay tampering this field, since it's not covered by |
|
A few things worth a closer look: 1.
|
|
Thanks for the review. Walking through each point: 1.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
eth/catalyst/api_types.go (1)
147-160:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
SafeL2Datastill omits the canonical L1 queue cursor.
NewSafeL2Blocknow has to reconstructheader.NextL1MsgIndexfrom local block contents because this payload does not carry it. That breaks on valid batches with trailing skipped queue indices, and it also turns missing parent queue metadata into a fail-open unset header field. Please carry the authoritative next index (or equivalent total popped value) inSafeL2Datainstead of deriving it fromNumL1MessagesProcessed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth/catalyst/api_types.go` around lines 147 - 160, SafeL2Data currently omits the canonical L1 queue cursor, forcing NewSafeL2Block to reconstruct header.NextL1MsgIndex from local contents (NumL1MessagesProcessed) which breaks on batches with trailing skipped indices; add an explicit field to SafeL2Data (e.g. NextL1MsgIndex or TotalPopped) that carries the authoritative next index/total-popped value, surface it through JSON tags like the other fields, and update NewSafeL2Block to read header.NextL1MsgIndex from this new SafeL2Data field instead of deriving it from NumL1MessagesProcessed so skipped indices and missing parent metadata are handled correctly.eth/catalyst/l2_api.go (2)
509-526:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce
params.Hashbefore the verified-cache fast path.A cache hit currently skips the declared-hash invariant entirely. Any request that reproduces a cached block body will be committed even when it carries a different non-zero
params.Hash, so the anti-tamper/signature-replay check only runs on cache misses.💡 Minimal fix
+ // Defense against signature-replay: ensure the declared block hash matches the + // hash recomputed from the canonical fields. Without this check, an attacker + // could keep params.Hash from a legitimately signed block while tampering with + // other content fields, and have the signature verification on the consensus + // side accept the tampered body. This is the last line of defense before the + // block is committed to the chain; the tendermint side performs the same check + // earlier in VerifyBlockSignature to reject tampered blocks before propagation. + if (params.Hash != common.Hash{}) && block.Hash() != params.Hash { + log.Error("NewL2BlockV2 hash mismatch (signature replay or tampering)", + "declared", params.Hash.Hex(), "computed", block.Hash().Hex(), "number", params.Number) + return nil, fmt.Errorf("block hash mismatch: declared %s, computed %s", + params.Hash.Hex(), block.Hash().Hex()) + } + if bas, verified := api.isVerified(block.Hash()); verified { bc.UpdateBlockProcessMetrics(bas.state, bas.procTime) return block.Header(), bc.WriteStateAndSetHead(block, bas.receipts, bas.state, bas.procTime) } - - // Defense against signature-replay: ensure the declared block hash matches the - // hash recomputed from the canonical fields. Without this check, an attacker - // could keep params.Hash from a legitimately signed block while tampering with - // other content fields, and have the signature verification on the consensus - // side accept the tampered body. This is the last line of defense before the - // block is committed to the chain; the tendermint side performs the same check - // earlier in VerifyBlockSignature to reject tampered blocks before propagation. - if (params.Hash != common.Hash{}) && block.Hash() != params.Hash { - log.Error("NewL2BlockV2 hash mismatch (signature replay or tampering)", - "declared", params.Hash.Hex(), "computed", block.Hash().Hex(), "number", params.Number) - return nil, fmt.Errorf("block hash mismatch: declared %s, computed %s", - params.Hash.Hex(), block.Hash().Hex()) - }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth/catalyst/l2_api.go` around lines 509 - 526, The declared-hash anti-tamper check must run before any cache-fast-path return: currently api.isVerified(...) can return a cached bas and cause an early return without validating params.Hash, allowing signature-replay; ensure that the (params.Hash != common.Hash{}) && block.Hash() != params.Hash check is executed before accepting a cached result or, at minimum, before returning from the verified-cache branch (the branch that calls bc.UpdateBlockProcessMetrics(bas.state, bas.procTime) and bc.WriteStateAndSetHead). Concretely, move or duplicate the declared-hash comparison so that the code compares params.Hash to block.Hash() (using block.Hash().Hex() for logging) prior to returning on a successful api.isVerified(...) hit, referencing api.isVerified, bas/verified, block.Hash(), params.Hash, UpdateBlockProcessMetrics and WriteStateAndSetHead to locate where to insert the check.
242-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject pinned parents below finalized.
This branch accepts any known parent hash and then canonicalizes through
WriteStateAndSetHead. If that parent sits belowbc.CurrentFinalizedBlock(), a caller can rewrite canonical history under the finalized boundary and leave finalized queries pointing at a non-canonical chain. Add an explicit finalized-height guard before processing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eth/catalyst/l2_api.go` around lines 242 - 255, The branch handling params.ParentHash must reject parent headers at or below the finalized boundary to prevent rewriting finalized history: after obtaining parentHeader via bc.GetHeaderByHash(*params.ParentHash) (inside the params.ParentHash != nil branch) add a check against bc.CurrentFinalizedBlock()—compare parentHeader.Number.Uint64() to bc.CurrentFinalizedBlock().Number.Uint64() and return an error (e.g. "parent below finalized") if the parent is at or below the finalized height—do this before proceeding to bc.GetBlock(...) and before any call to WriteStateAndSetHead to ensure finalized guards are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@eth/catalyst/api_types.go`:
- Around line 147-160: SafeL2Data currently omits the canonical L1 queue cursor,
forcing NewSafeL2Block to reconstruct header.NextL1MsgIndex from local contents
(NumL1MessagesProcessed) which breaks on batches with trailing skipped indices;
add an explicit field to SafeL2Data (e.g. NextL1MsgIndex or TotalPopped) that
carries the authoritative next index/total-popped value, surface it through JSON
tags like the other fields, and update NewSafeL2Block to read
header.NextL1MsgIndex from this new SafeL2Data field instead of deriving it from
NumL1MessagesProcessed so skipped indices and missing parent metadata are
handled correctly.
In `@eth/catalyst/l2_api.go`:
- Around line 509-526: The declared-hash anti-tamper check must run before any
cache-fast-path return: currently api.isVerified(...) can return a cached bas
and cause an early return without validating params.Hash, allowing
signature-replay; ensure that the (params.Hash != common.Hash{}) && block.Hash()
!= params.Hash check is executed before accepting a cached result or, at
minimum, before returning from the verified-cache branch (the branch that calls
bc.UpdateBlockProcessMetrics(bas.state, bas.procTime) and
bc.WriteStateAndSetHead). Concretely, move or duplicate the declared-hash
comparison so that the code compares params.Hash to block.Hash() (using
block.Hash().Hex() for logging) prior to returning on a successful
api.isVerified(...) hit, referencing api.isVerified, bas/verified, block.Hash(),
params.Hash, UpdateBlockProcessMetrics and WriteStateAndSetHead to locate where
to insert the check.
- Around line 242-255: The branch handling params.ParentHash must reject parent
headers at or below the finalized boundary to prevent rewriting finalized
history: after obtaining parentHeader via bc.GetHeaderByHash(*params.ParentHash)
(inside the params.ParentHash != nil branch) add a check against
bc.CurrentFinalizedBlock()—compare parentHeader.Number.Uint64() to
bc.CurrentFinalizedBlock().Number.Uint64() and return an error (e.g. "parent
below finalized") if the parent is at or below the finalized height—do this
before proceeding to bc.GetBlock(...) and before any call to
WriteStateAndSetHead to ensure finalized guards are enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3acc122f-8988-40a9-8a87-03d59bd30b50
📒 Files selected for processing (4)
eth/catalyst/api_types.goeth/catalyst/gen_l2blockv2params.goeth/catalyst/l2_api.goethclient/authclient/engine.go
✅ Files skipped from review due to trivial changes (1)
- eth/catalyst/gen_l2blockv2params.go
🚧 Files skipped from review as they are similar to previous changes (1)
- ethclient/authclient/engine.go
…l1_msg_index hardening) (#124) * feat(engine-api): add reorg-capable L2 engine V2 API for centralized sequencer Ports the EL surface the centralized-sequencer consensus client requires (morph-l2/go-ethereum#331, morph feat/sequencer-final): - engine_newL2BlockV2: select the parent by hash instead of requiring it to equal the current head, so the forkchoice update reorgs the canonical chain onto the imported block. - engine_assembleL2BlockV2: build on an explicitly given parent hash. Positional params (parentHash, timestamp as a bare JSON number, txs); txs are base64-encoded to match go-ethereum's raw [][]byte and are decoded via AssembleV2Transactions (which also accepts 0x-hex for robustness). - engine_newSafeL2Block: optional SafeL2Data.parentHash pins a non-head parent for the derivation reorg path (deriveForce). build_l2_payload now resolves the parent from an override rather than always the head. e2e tests cover the happy path, sibling reorg, safe-block reorg and the V2 assemble path. * feat(consensus): enforce exact next_l1_msg_index from Jade onward Mirrors go-ethereum PR #331's writeBlockStateWithoutHead anti-tampering check. next_l1_msg_index is not covered by the block hash, so from Jade it must exactly match the value derived from the canonical L1 message stream: - Blocks with L1 messages: header.next == last_queue_index + 1, enforced in the stateless consensus check. Pre-Jade keeps the lenient lower bound to preserve the legacy "early L1 msg skip" behavior. - Empty blocks / parent-aware exactness: header.next == parent.next + 0, enforced in the engine-tree payload validator (validate_post_execution), the only point with both the parent header and the block body available. The error message carries the "invalid block.NextL1MsgIndex" substring so the consensus client's retry classifier treats it as a permanent (non-retryable) error. Stale "Tendermint instant finality / no reorgs" comments are corrected now that the V2 path is reorg-capable. * refactor(engine-api): use a struct param for assembleL2BlockV2 Replaces the three positional params (parentHash, bare-number timestamp, base64 txs) with a single AssembleL2BlockV2Params struct, mirroring V1's engine_assembleL2Block parameter style: - params: [{ parentHash, transactions, timestamp }] instead of three positional values - transactions use the normal 0x-hex bytes encoding (drops the base64 AssembleV2Transactions shim and its dependency) - timestamp is a hex quantity, consistent with the other engine types This unifies the V1/V2 wire shape and removes the base64 quirk. The consensus client (go-ethereum authclient AssembleL2BlockV2 + morph node retryable_client) and the cross-client contract must adopt the same struct shape in lockstep. * fix(consensus): reject L1 message forward-skip in parent-aware next_l1_msg_index check The Jade parent-aware validator derived `expected` from each leading L1 message's queue_index + 1 but never checked that the queue_index continued the parent's stream. A block whose first L1 message skipped ahead (e.g. parent.next=2, first queue=5, header.next=6) passed: the in-block messages were contiguous and header.next == last+1, so neither the stateless consensus check nor the trailing-skip check caught the silently dropped queue indices 2,3,4. go-ethereum avoids this by deriving the index from its canonical L1 queue; reth must check first == parent.next explicitly. Now each leading L1 message must equal the running expected index (first == parent.next, rest contiguous); pre-Jade behavior is unchanged. Adds post-Jade reject + pre-Jade allow e2e tests that craft a self-consistent block (queue index does not affect execution, so the state root stays valid, isolating the continuity violation). Reported by CodeRabbit on PR #124. * test(payload-types): assert assembleL2BlockV2 decodes explicit null timestamp geth's gencodec MarshalJSON always emits the `timestamp` field, writing `null` (not omitting it) when the sequencer passes no timestamp. Lock in that the reth server decodes that production payload to None rather than erroring, guarding the geth<->reth wire contract for the struct param. * fix(consensus): align L1 forward-skip handling with geth Match go-ethereum PR #331 by deriving post-Jade NextL1MsgIndex from the last leading L1 queue index, so forward skips are counted as processed while trailing skips remain rejected. Constraint: Preserve geth cross-client consensus behavior Confidence: high Scope-risk: narrow
2 participants
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
New Features
Tests