feat: centralized-sequencer EL support (V2 engine API + reorg + next_l1_msg_index hardening)#124
Merged
Merged
Conversation
…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.
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.
![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.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Jade-gated exactness for header.next_l1_msg_index, updates error text/tests, introduces parent-pinned engine v2 assemble/import APIs and RPCs, extends payload types with optional parent_hash, and adds post-execution parent-aware validation. ChangesJade L1 Validation & Engine V2 APIs
sequenceDiagram
participant Client
participant RpcHandler
participant EngineAPI
participant PayloadBuilder
participant EngineTree
participant Storage
Client->>RpcHandler: engine_assembleL2BlockV2(params)
RpcHandler->>EngineAPI: assemble_l2_block_v2(params)
EngineAPI->>PayloadBuilder: build_l2_payload(parent_override=parent_hash)
PayloadBuilder->>Storage: lookup parent by hash
PayloadBuilder-->>EngineAPI: ExecutableL2Data
Client->>RpcHandler: engine_newL2BlockV2(data)
RpcHandler->>EngineAPI: new_l2_block_v2(data)
EngineAPI->>EngineTree: import_l2_block_via_engine (resolve parent from data.parent_hash)
EngineTree->>Storage: apply block / update head
EngineAPI-->>RpcHandler: MorphHeader
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
crates/node/tests/it/consensus.rs (1)
322-324: ⚡ Quick winPin Jade schedule explicitly in this test.
This test currently depends on the default schedule being AllActive. Please set
with_schedule(HardforkSchedule::AllActive)explicitly (as done in nearby tests) so the fork-boundary intent can’t drift if defaults change.Proposed diff
- // Default schedule is AllActive (Jade on). - let (mut nodes, _wallet) = TestNodeBuilder::new().build().await?; + let (mut nodes, _wallet) = TestNodeBuilder::new() + .with_schedule(HardforkSchedule::AllActive) + .build() + .await?;🤖 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 `@crates/node/tests/it/consensus.rs` around lines 322 - 324, Test relies on the default hardfork schedule; explicitly pin it by updating the TestNodeBuilder invocation to use with_schedule(HardforkSchedule::AllActive) before build(), e.g. call TestNodeBuilder::new().with_schedule(HardforkSchedule::AllActive).build().await? so the test’s fork-boundary assumptions remain stable; locate the builder usage around TestNodeBuilder::new().build().await? and replace/augment it with with_schedule(HardforkSchedule::AllActive) prior to build().
🤖 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 `@crates/engine-tree-ext/src/payload_validator.rs`:
- Around line 898-921: The loop computing expected from
parent_block.next_l1_msg_index over block.body().transactions() currently only
sets expected = queue_index + 1 and never verifies continuity against the
parent-derived expected, allowing gaps; change the logic in the for loop that
handles tx.is_l1_msg() to first compare queue_index()? (from tx.queue_index())
with the current expected and return a ConsensusError if they differ, then set
expected = queue_index.checked_add(1)... as before (use the same error
formatting referencing block.header().next_l1_msg_index and expected/actual),
ensuring you still handle the tx.queue_index() None case and overflow with the
same ok_or_else checks.
---
Nitpick comments:
In `@crates/node/tests/it/consensus.rs`:
- Around line 322-324: Test relies on the default hardfork schedule; explicitly
pin it by updating the TestNodeBuilder invocation to use
with_schedule(HardforkSchedule::AllActive) before build(), e.g. call
TestNodeBuilder::new().with_schedule(HardforkSchedule::AllActive).build().await?
so the test’s fork-boundary assumptions remain stable; locate the builder usage
around TestNodeBuilder::new().build().await? and replace/augment it with
with_schedule(HardforkSchedule::AllActive) prior to build().
🪄 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: 325b247a-059f-4850-8f6f-6a62049ee2af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
crates/consensus/src/error.rscrates/consensus/src/validation.rscrates/engine-api/src/api.rscrates/engine-api/src/builder.rscrates/engine-api/src/rpc.rscrates/engine-tree-ext/src/payload_validator.rscrates/node/src/validator.rscrates/node/tests/it/consensus.rscrates/node/tests/it/engine.rscrates/payload/types/Cargo.tomlcrates/payload/types/src/lib.rscrates/payload/types/src/params.rscrates/payload/types/src/safe_l2_data.rs
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.
…1_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.
…imestamp 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.
tomatoishealthy
approved these changes
Jun 8, 2026
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Morph is switching to a centralized sequencer, which (unlike the Tendermint instant-finality model) introduces reorgs. morph-reth's L2 Engine API was built on a "never reorg" assumption, so it is missing the EL surface the new consensus client drives.
This ports the EL changes from morph-l2/go-ethereum#331 and aligns with the CL contract on
morph-l2/morph: feat/sequencer-final:executor.ApplyBlockV2 → engine_newL2BlockV2(reorg viaSetCanonical)deriveForce → engine_newSafeL2BlockwithSafeL2Data.ParentHash(L1-canonical self-heal reorg)engine_assembleL2BlockV2What
feat(engine-api)— reorg-capable V2 methodsengine_newL2BlockV2— selects the parent by hash instead of requiring it to equal the current head; the forkchoice update reorgs the canonical chain onto the imported block. Returns the header. TakesExecutableL2Data(unchanged shape).engine_newSafeL2Block— optionalSafeL2Data.parentHashpins a non-head parent for the derivation reorg path;build_l2_payloadresolves the parent from an override.engine_assembleL2BlockV2— builds on an explicit parent hash. Takes a singleAssembleL2BlockV2Paramsstruct ({ parentHash, transactions, timestamp }), mirroring V1'sengine_assembleL2Blockstyle:transactionsare0x-hex bytes andtimestampis a hex quantity.feat(consensus)— exactnext_l1_msg_indexfrom Jade (anti-tamper, geth PR #331)next_l1_msg_indexis not covered by the block hash, so from Jade it must exactly match the value derived from the canonical L1 message stream:header.next == last_queue_index + 1(stateless consensus check; pre-Jade keeps the lenient lower bound for the legacy "early L1 msg skip").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.invalid block.NextL1MsgIndexsubstring so the CL's retry classifier (retryable_client.go) treats it as permanent (non-retryable).Stale "Tendermint instant finality / no reorgs possible" comments are corrected.
Tests
fmt+clippy --all --all-targets -D warnings+cargo test --all+ e2e all green. New coverage:new_l2_block_v2_imports_block_on_current_head,new_l2_block_v2_reorgs_onto_sibling_block,new_safe_l2_block_with_parent_hash_reorgs_onto_non_head_parent,assemble_l2_block_v2_builds_on_explicit_parentnext_l1_msg_index_skip_past_included_messages_rejected_post_jade,..._can_skip_past_included_messages_pre_jade,next_l1_msg_index_empty_block_cannot_advance_post_jadeAssembleL2BlockV2Params/SafeL2Data.parentHashserde, consensus Jade-exact + error-stringNotes / design
newL2BlockV2near-wall-clock blocks usefinalized=ZERO, so the engine tree permits reorgs; the divergent block in aderiveForcereorg comes via the live (non-finalized) path. Verified by the reorg e2e tests.reorg()short-chain fix is not ported directly (reth uses its own engine tree); the 1-block sibling-reorg scenario it guards is covered by e2e.authclient.AssembleL2BlockV2+l2_api.go, morph noderetryable_client, andmorph-cross-client-testsadopt theAssembleL2BlockV2Paramsstruct shape (hex txs, hex-quantity timestamp).next_l1_msg_index == last_queue_index + 1(no trailing skips). Consensus-safe by geth-parity (PR #331 hard-fails the same blocks), but the previous reth behavior allowed trailing skips, so a real-chain spot check is prudent.Depends on the CL companion branch
morph: feat/sequencer-final(still unmerged) for the call sites.Summary by CodeRabbit
New Features
Bug Fixes
Tests