feat: centralized-sequencer EL support (V2 engine API + reorg + next_l1_msg_index hardening)#124
Open
panos-xyz wants to merge 3 commits into
Open
feat: centralized-sequencer EL support (V2 engine API + reorg + next_l1_msg_index hardening)#124panos-xyz wants to merge 3 commits into
panos-xyz wants to merge 3 commits into
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR implements Jade fork L1 message index validation with fork-dependent exactness rules and adds v2 engine APIs for reorg-capable block management. L1 message header checks now require exact matches under Jade but allow lower-bound skipping pre-Jade. The payload types are extended with ChangesJade L1 Validation & Engine V2 APIs
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
Comment on lines
+898
to
+921
| let mut expected = parent_block.next_l1_msg_index; | ||
| for tx in block.body().transactions() { | ||
| if !tx.is_l1_msg() { | ||
| break; | ||
| } | ||
|
|
||
| let queue_index = tx.queue_index().ok_or_else(|| { | ||
| ConsensusError::msg("L1 message transaction is missing queue index") | ||
| })?; | ||
| expected = queue_index.checked_add(1).ok_or_else(|| { | ||
| ConsensusError::msg(format!( | ||
| "invalid block.NextL1MsgIndex: expected {}, got {}", | ||
| u64::MAX, | ||
| block.header().next_l1_msg_index | ||
| )) | ||
| })?; | ||
| } | ||
|
|
||
| let actual = block.header().next_l1_msg_index; | ||
| if actual != expected { | ||
| return Err(ConsensusError::msg(format!( | ||
| "invalid block.NextL1MsgIndex: expected {expected}, got {actual}" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Enforce parent-stream queue continuity in Jade parent-aware validation.
The loop derives expected from each tx’s queue_index + 1 but never checks that queue_index equals the parent-derived expected value. That allows skipped queue indices to pass post-Jade (e.g., parent.next=2, first L1 tx queue=5, header.next=6).
🔧 Proposed fix
let mut expected = parent_block.next_l1_msg_index;
for tx in block.body().transactions() {
if !tx.is_l1_msg() {
break;
}
let queue_index = tx.queue_index().ok_or_else(|| {
ConsensusError::msg("L1 message transaction is missing queue index")
})?;
- expected = queue_index.checked_add(1).ok_or_else(|| {
+ if queue_index != expected {
+ return Err(ConsensusError::msg(format!(
+ "invalid block.NextL1MsgIndex: expected {expected}, got {queue_index}"
+ )));
+ }
+ expected = expected.checked_add(1).ok_or_else(|| {
ConsensusError::msg(format!(
"invalid block.NextL1MsgIndex: expected {}, got {}",
u64::MAX,
block.header().next_l1_msg_index
))
})?;
}📝 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.
Suggested change
| let mut expected = parent_block.next_l1_msg_index; | |
| for tx in block.body().transactions() { | |
| if !tx.is_l1_msg() { | |
| break; | |
| } | |
| let queue_index = tx.queue_index().ok_or_else(|| { | |
| ConsensusError::msg("L1 message transaction is missing queue index") | |
| })?; | |
| expected = queue_index.checked_add(1).ok_or_else(|| { | |
| ConsensusError::msg(format!( | |
| "invalid block.NextL1MsgIndex: expected {}, got {}", | |
| u64::MAX, | |
| block.header().next_l1_msg_index | |
| )) | |
| })?; | |
| } | |
| let actual = block.header().next_l1_msg_index; | |
| if actual != expected { | |
| return Err(ConsensusError::msg(format!( | |
| "invalid block.NextL1MsgIndex: expected {expected}, got {actual}" | |
| ))); | |
| } | |
| let mut expected = parent_block.next_l1_msg_index; | |
| for tx in block.body().transactions() { | |
| if !tx.is_l1_msg() { | |
| break; | |
| } | |
| let queue_index = tx.queue_index().ok_or_else(|| { | |
| ConsensusError::msg("L1 message transaction is missing queue index") | |
| })?; | |
| if queue_index != expected { | |
| return Err(ConsensusError::msg(format!( | |
| "invalid block.NextL1MsgIndex: expected {expected}, got {queue_index}" | |
| ))); | |
| } | |
| expected = expected.checked_add(1).ok_or_else(|| { | |
| ConsensusError::msg(format!( | |
| "invalid block.NextL1MsgIndex: expected {}, got {}", | |
| u64::MAX, | |
| block.header().next_l1_msg_index | |
| )) | |
| })?; | |
| } | |
| let actual = block.header().next_l1_msg_index; | |
| if actual != expected { | |
| return Err(ConsensusError::msg(format!( | |
| "invalid block.NextL1MsgIndex: expected {expected}, got {actual}" | |
| ))); | |
| } |
🤖 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/engine-tree-ext/src/payload_validator.rs` around lines 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.
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.
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
1 participant
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
Release Notes
New Features
engine_assembleL2BlockV2andengine_newL2BlockV2JSON-RPC methods enabling explicit parent selection for block assembly and importBug Fixes
Tests