feat: unfork reth to paradigmxyz/reth v2.2.0 with retroactive state-root trust#98
Open
panos-xyz wants to merge 61 commits into
Open
feat: unfork reth to paradigmxyz/reth v2.2.0 with retroactive state-root trust#98panos-xyz wants to merge 61 commits into
panos-xyz wants to merge 61 commits into
Conversation
…icEngineValidator
Verbatim copy of reth v2.0.0 crates/engine/tree/src/tree/payload_validator.rs
(2141 lines) into the new morph-engine-tree-ext crate. Two intended edits:
- Rewrote `use crate::tree::{...}` and `use crate::tree::payload_processor::receipt_root_task::...`
to `reth_engine_tree::tree::...` and `reth_revm::database::StateProviderDatabase`
- Renamed BasicEngineValidator → MorphBasicEngineValidator (4 occurrences)
Temporarily pins engine-tree-ext's reth-* deps to paradigmxyz/reth v2.0.0
directly (git+tag) because the morph-l2/reth fork (at the pinned rev, based on
v1.10.0) lacks reth-execution-cache and v2.0.0's additional pub re-exports.
Task 4 flips the whole workspace to v2.0.0; this temporary mixed state is
contained (engine-tree-ext is not yet used elsewhere).
Actual v2.0.0 workarounds required (pre-audit was slightly optimistic):
1. trie_updates sibling module: payload_validator.rs references
`super::trie_updates::compare_trie_updates` which is a private sibling module
inside reth_engine_tree::tree. Copied trie_updates.rs verbatim (312 lines)
as crate::trie_updates and changed the one call site to crate::trie_updates.
2. EngineApiTreeState private fields: the struct exposes `tree_state` and
`invalid_headers` as private fields but has public accessor methods
(tree_state() and has_invalid_header()). Six field accesses rewritten to
method calls — no logic change, just using the public API.
3. Missing deps: added tokio (sync), crossbeam-channel, alloy-rlp, reth-db
(needed by trie_updates.rs and payload_validator.rs directly).
Adds two NOTE(morph) comment blocks: one above the first state.tree_state() accessor-substitution call site (5 total sites, not 6 as the previous commit message incorrectly stated), and one above the crate::trie_updates:: call site that replaces super::trie_updates:: from upstream.
…lidator Replaces morph-l2/reth fork (v1.10.0 base, 6 commits of StateRootValidator trait + stages/merkle downgrade) with paradigmxyz/reth upstream v2.0.0 + a local MorphBasicEngineValidator copy that gates the pre-Jade state-root check. Workspace + feature plumbing: - Flip ~50 Cargo.toml reth git pins from morph-l2/reth to paradigmxyz/reth v2.0.0 - Revert engine-tree-ext's temporary direct v2.0.0 pins to workspace-inherited - Add reth-execution-cache, crossbeam-channel, alloy-rlp to workspace deps - Enable morph-primitives/reth-codec feature in engine-tree-ext (required for NodePrimitives::Receipt: FullReceipt at test time) Fork-specific code removed: - impl StateRootValidator for MorphEngineValidator (fork-only trait) - MorphEngineValidator.chain_spec field kept with #[allow(dead_code)] for future PayloadValidator extensions - RlpBincode impls on MorphReceipt / MorphTxEnvelope (trait deleted in v2.0.0) Validator wiring: - MorphTreeEngineValidatorBuilder returns morph_engine_tree_ext:: MorphBasicEngineValidator<P, Evm, V> (3-type-param, upstream-style) with chain_spec threaded through new() - Both state-root comparison sites in MorphBasicEngineValidator gated via gate::state_root_enforced_at (pre-Jade skip, post-Jade strict) v1.10.0 -> v2.0.0 API drift adapted across: - morph-consensus (validation.rs): validate_block_post_execution signature - morph-revm (evm/exec/handler/tx): TransactionEnv removal, execution_result signature, validate_initial_tx_gas &mut requirement - morph-evm (block/curie/factory/receipt/config/engine): receipt & block builder trait shape changes - morph-payload (builder/error, types/attributes, types/lib): payload type conversions - morph-txpool (lib/transaction/validator): tx pool trait updates - morph-rpc (eth/mod, eth/transaction): RPC conversion changes - morph-engine-api (builder): Engine API builder API - morph-node (components/pool, add_ons): node component wiring - morph-primitives (header/receipt/envelope): remove deleted trait impls Retroactive trust: the first post-Jade block's strict MPT comparison anchors pre-Jade MPT state accumulated by reth's block-by-block execution. See docs/superpowers/specs/2026-04-17-unfork-reth-retroactive-trust-design.md.
- crates/txpool/src/validator.rs: allow(dead_code, unused_imports) inside `mod tests` and #[cfg(any())] 4 tests that used MockEthProvider (incompatible with MorphPrimitives::BlockHeader = MorphHeader under reth v2.0.0's tightened bound). 2 tests that don't use MockEthProvider still run. - crates/node/src/components/pool.rs: drop redundant clone of morph_evm_config (clippy::redundant_clone). The value wasn't used again. Remaining work for make clippy-e2e / make test-e2e: adapt crates/node/src/test_utils.rs to reth v2.0.0 payload-builder API (EthPayloadBuilderAttributes / PayloadBuilderAttributes moved; send_new_payload now takes BuildNewPayload<...> wrapper; setup_engine signature changed). Tracked as part of Task 7 which extends test_utils anyway.
…test_utils to reth v2.0.0 Adapts `crates/node/src/test_utils.rs` and `crates/node/tests/it/` helpers to reth v2.0.0's new payload-builder and e2e-test-utils APIs: * `setup_engine` now returns `(Vec<NodeHelper>, Wallet)` — drop the `TaskManager` slot from all 78 destructurings. * `send_new_payload` expects `BuildNewPayload<PayloadAttributes>` — wrap raw `MorphPayloadAttributes` + `parent_hash` directly instead of going through `MorphPayloadBuilderAttributes::try_new` (the v1.x fork-only helper). * `morph_payload_attributes` now returns `MorphPayloadAttributes` (the `PayloadTypes::PayloadAttributes` assoc type) rather than the internal builder attributes. * Add `impl From<alloy_rpc_types_engine::PayloadAttributes> for MorphPayloadAttributes` so `MorphNode` satisfies `NodeBuilderHelper` in v2.0.0's e2e-test-utils. Adds `crates/engine-tree-ext/tests/jade_boundary.rs` — two integration tests that pin the retroactive-trust contract to the engine-tree-ext crate: * `pre_jade_block_with_tampered_state_root_imports`: asserts the validator skips state-root comparison before Jade. * `post_jade_block_with_tampered_state_root_is_rejected`: asserts the validator enforces strict MPT equality after Jade. Both pass under `cargo test -p morph-engine-tree-ext --features test-utils --test jade_boundary`.
revm's mark_warm_with_transaction_id() resets original_value = present_value on cold->warm transitions. After token fee deduction marks storage slots cold via mark_cold(), the main tx's first SLOAD triggers that corruption, and subsequent SSTOREs on those slots see "clean" slots (2900 gas SSTORE_RESET) instead of "dirty" slots (100 gas SLOAD_GAS), missing the EIP-2200 dirty-slot refund of 2800 gas. Symptom: Hoodi sync rejects block 2,205,224 with "block gas used mismatch: got 188515, expected 185715" on a MorphTx V1 withdrawETH (type 0x7f, feeTokenID=0x1). Fix: override SLOAD opcode (0x54) with sload_morph, which on a cold load reads the true committed value from DB (hits State<DB> cache, O(1)) and restores slot.original_value if corrupted. Zero overhead on warm accesses. Ported from morph-reth-enginevalidator-spike commit 6031236. Verified: after the fix block 2,205,224 imports with gas_used=185715 matching canonical, stateRoot=0x037e21505f141c1a1bcd430fb53c284c86e69360b422ec7732e5b6849b5b4f9b matching canonical, and Hoodi sync continues past the previously-stuck point.
revm's SSTORE opcode warms a cold slot through the same mark_warm_with_transaction_id() path as SLOAD. So a main tx that writes a forced-cold token-fee slot WITHOUT first SLOADing it hits the same original_value corruption: EIP-2200 sees a 'clean' slot and charges 2900 (SSTORE_RESET) instead of 100 (SLOAD_GAS) + the dirty-slot refund. Our prior fix (commit 146aa86) only covered the SLOAD path, so any tx whose first touch of a fee-deducted slot happens to be a direct SSTORE would still diverge. Common in flows where ERC20 fee token == main tx target (e.g. user pays fee in USDT and main tx transfers USDT), if the compiled Solidity path happens to write before read. Fix: custom SSTORE opcode (0x55) that mirrors sload_morph — on cold access, read the true committed value from DB and restore state_load.data.original_value plus the slot's original_value in journal state before sstore_dynamic_gas() computes EIP-2200 cost. All gas accounting (static, dynamic, refund) is handled manually in the override; static gas registered as 0. Ported from morph-reth-enginevalidator-spike commit c61633f, using our DB-direct lookup style (no fee_slot_original_values map needed).
…te reuse Destructure execution_cache from BuildArguments and wrap the state provider with CachedStateProvider so account/storage/code reads consult a shared cross-block cache before hitting the DB. When the reth node runs with --engine.share-execution-cache-with-payload-builder, reth's engine tree provides a SavedCache snapshot associated with the parent block. Wrapping state_provider with CachedStateProvider amortizes cross-block read cost when engine tree and payload builder touch overlapping state. Prior code destructured BuildArguments with `..`, silently dropping execution_cache (and trie_handle) — so the feature was unused. Also relaxes build_payload_inner's state_provider bound to `?Sized` so we can pass `&dyn StateProvider` through the Box.
…l state root Destructure trie_handle from BuildArguments (previously silently dropped with `..`) and thread it into build_payload_inner. Before executing transactions, wire the handle's state_hook into the block executor via set_state_hook. Per-tx state diffs now stream to the background sparse-trie task during execution, so most of the state root computation happens concurrently with tx execution. At block finish time, drop the state hook (signals FinishedStateUpdates via StateHookSender's Drop impl) and call handle.state_root() to receive the final root. Fall back to synchronous state root if the background task fails. When --engine.share-execution-cache-with-payload-builder is set, reth's engine tree provides both the execution cache and the trie handle. With this commit and the previous PayloadExecutionCache wiring, payload building now takes full advantage of both cross-block caching and parallel state-root computation. Expected gain: ~10-20% faster builds on blocks with meaningful state writes.
…ibutes v2.0.0 introduced the BuildNextEnv<Attributes, Header, Ctx> trait in reth-payload-primitives as the canonical entry point for constructing EVM envs from payload attributes. Implement it for MorphNextBlockEnvAttributes so downstream consumers can use the trait-based API. This is an additive refactor — the existing inline construction in build_payload_inner continues to work. New code should prefer calling MorphNextBlockEnvAttributes::build_next_env(&rpc_attrs, &parent, &()) over manual field splatting.
…rphnode Adds `--engine.persistence-threshold 256`, `--engine.memory-block-buffer-target 16`, and `--engine.persistence-backpressure-threshold 512` (v2.0.0 requires the backpressure value > persistence-threshold). These batch reth's MDBX writes so they do not contend with Tendermint's LevelDB fsyncs. NOTE: This contention only manifests when morphnode (CL) and morph-reth (EL) run on the same host and both issue fsyncs against the same physical disk — i.e. the local-test single-box topology. Production deployments that place morphnode and morph-reth on separate machines do not hit this and would not observe the same speed-up from these flags. Measured impact on the mainnet sync in local-test (M4 Pro, CL+EL co-located): - before: ~42 blocks/s - after: ~84 blocks/s (2x)
|
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 a new crate ChangesEngine-tree validator, EVM refactor, payload & RPC wiring
Sequence Diagram(s)sequenceDiagram
participant Client
participant PayloadBuilder as PayloadBuilder/EngineAPI
participant Validator as MorphBasicEngineValidator
participant EVM
participant DB
participant Consensus
Client->>PayloadBuilder: request new payload / import block
PayloadBuilder->>Validator: submit BlockOrPayload
Validator->>DB: fetch parent header / build overlay
par Parallel execution
Validator->>EVM: execute transactions (streams receipts)
Validator->>DB: spawn state-root task or compute parallel updates
and
EVM->>DB: storage reads/writes (sload/sstore handlers)
EVM-->>Validator: execution results + streamed receipts
end
Validator->>Validator: hashed-post-state computation completes
Validator->>Consensus: verify receipts_root and logs_bloom
alt state-root enforced (post-Jade)
Validator->>DB: verify computed state_root == header.state_root
else state-root skipped (pre-Jade or missing expectation)
Validator->>Consensus: mark state-root validation skipped
end
Validator-->>Client: return validation outcome (Valid / Invalid / Skip)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/evm/src/block/receipt.rs (1)
248-271:⚠️ Potential issue | 🟡 MinorFix ResultGas constructor arguments in test helpers — gas_limit and spent are distinct fields.
The test helpers pass
ResultGas::new(gas_used, gas_used, 0, 0, 0)where parameters map to (gas_limit, spent, refunded, floor_gas, intrinsic_gas). Setting gas_limit to the spent amount conflates two distinct semantic fields; gas_limit should represent the transaction's total available gas, not the amount consumed. While current tests only exerciseis_success()andinto_logs(), any future test reading gas fields or refund calculations would encounter inaccurate values. Assign explicit names to each parameter in the helpers (e.g.,gas_limit: gas_used, spent: gas_used, refunded: 0, ...) and consider using a realistic gas_limit value (e.g., 21000 for basic transfer, or a higher constant for complex operations).crates/node/src/test_utils.rs (1)
8-18:⚠️ Potential issue | 🟡 MinorUpdate the examples for the new two-value return type.
build()no longer returns aTaskManager, but the ignored examples still destructure_tasks. This will mislead callers copying the test utility snippets.Proposed documentation fix
-//! let (mut nodes, _tasks, wallet) = TestNodeBuilder::new().build().await?; +//! let (mut nodes, wallet) = TestNodeBuilder::new().build().await?; @@ -/// let (mut nodes, _, wallet) = TestNodeBuilder::new().with_schedule(schedule).build().await?; +/// let (mut nodes, wallet) = TestNodeBuilder::new().with_schedule(schedule).build().await?; @@ -/// let (mut nodes, _tasks, wallet) = TestNodeBuilder::new().build().await?; +/// let (mut nodes, wallet) = TestNodeBuilder::new().build().await?; @@ -/// let (mut nodes, _tasks, wallet) = TestNodeBuilder::new() +/// let (mut nodes, wallet) = TestNodeBuilder::new() @@ -/// let (nodes, _tasks, wallet) = TestNodeBuilder::new() +/// let (nodes, wallet) = TestNodeBuilder::new()Also applies to: 55-63, 170-187, 247-249
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/test_utils.rs` around lines 8 - 18, The examples in test_utils.rs still destructure a now-removed TaskManager from TestNodeBuilder::new().build(); update every example that does let (mut nodes, _tasks, wallet) = TestNodeBuilder::new().build().await?; to match the new two-value return (e.g., let (mut nodes, wallet) = TestNodeBuilder::new().build().await?;), remove the unused _tasks variable, and adjust subsequent code that references _tasks accordingly (affects the examples around TestNodeBuilder::new().build() and uses of advance_chain).crates/payload/builder/src/builder.rs (1)
700-711:⚠️ Potential issue | 🟡 MinorMinor:
withdrawalsis now alwaysSome(_)— confirm no semantic shift for pre-Shanghai paths.Previously
BuildNextEnv for MorphPayloadAttributesmappedinner.withdrawals.clone().map(Into::into), preservingNone. Here, becauseMorphPayloadBuilderAttributes::try_newalreadyunwrap_or_default()s, you end up withSome(Withdrawals::default())even when the CL sentwithdrawals: None. For Morph (post-Shanghai) this is benign (withdrawals root of the empty set equals the default), but it's a subtle change worth noting for any consumer that distinguishes "no withdrawals field" from "empty list".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/builder/src/builder.rs` around lines 700 - 711, The code constructs NextBlockEnvAttributes with withdrawals always Some(...) because MorphPayloadBuilderAttributes::try_new already unwraps to default; to preserve the original semantics where None stays None, change the withdrawals assignment in MorphNextBlockEnvAttributes/NextBlockEnvAttributes construction to propagate the original optional by using attributes.withdrawals.clone().map(Into::into) (or attributes.withdrawals.clone().map(|w| w.into())), or alternatively stop unwrap_or_default() in MorphPayloadBuilderAttributes::try_new so that None is preserved—apply the change in the builder code paths touching MorphNextBlockEnvAttributes and MorphPayloadBuilderAttributes::try_new to ensure consumers that rely on None vs Some(empty) still see None when the CL omitted withdrawals.
🧹 Nitpick comments (5)
crates/evm/src/engine.rs (1)
91-98: Minor: reuseto_tx_envto avoid duplicating theMorphTxEnvconstruction.
into_partsreimplements the body ofto_tx_envverbatim (line 86-88). Delegating avoids drift ifMorphTxEnv::from_recovered_txgains additional inputs later.♻️ Proposed refactor
impl ExecutableTxParts<MorphTxEnv, MorphTxEnvelope> for RecoveredInBlock { type Recovered = Self; fn into_parts(self) -> (MorphTxEnv, Self) { - let tx_env = MorphTxEnv::from_recovered_tx(self.tx(), *self.signer()); - (tx_env, self) + let tx_env = self.to_tx_env(); + (tx_env, self) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/evm/src/engine.rs` around lines 91 - 98, into_parts duplicates the MorphTxEnv construction already provided by to_tx_env; change ExecutableTxParts::into_parts for RecoveredInBlock to delegate to the existing to_tx_env implementation instead of calling MorphTxEnv::from_recovered_tx directly—call self.to_tx_env() (which internally uses self.tx() and self.signer()) and return (that_tx_env, self) so any future changes to MorphTxEnv::from_recovered_tx are honored.crates/payload/builder/src/error.rs (1)
45-48: Consider preserving error source instead of aString.Wrapping the underlying storage error as
Stringloses the#[source]chain (stack trace, downcast). If the upstream error type isSend + Sync + 'static, a#[source] Box<dyn std::error::Error + Send + Sync>variant would be strictly more informative, at no ergonomic cost for callers using.to_string().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/payload/builder/src/error.rs` around lines 45 - 48, The Storage error variant currently captures the underlying storage error as a String which loses the original error source; change the Storage variant in the error enum from Storage(String) to store the error as a boxed source error (e.g. Storage(#[source] Box<dyn std::error::Error + Send + Sync + 'static>)) and keep the existing display error message (#[error("storage error: {0}")]) so formatting still works; update any construction sites that currently pass a String to instead box the original error (Box::new(err)) or convert existing errors into the boxed trait object.crates/engine-api/src/builder.rs (1)
664-703: Use thepayload_idreturned bysend_new_payloadinstead of pre-computing it locally.The code at line 671 computes
payload_idviaMorphPayloadBuilderAttributes::try_new(...)and then discards the result ofsend_new_payload(...)at lines 679–690 vialet _ = .... The test code (helpers.rs:247) confirms thatsend_new_payload()returnsResult<PayloadId, ...>, so this id is available.While both ids are currently derived from identical inputs (parent hash + rpc_attributes + version 1, via
payload_id_morph), discarding the service's returned id creates a maintenance hazard: if the service's derivation ever changes (e.g., version bump, different hashing), the locally-computed id would diverge without detection, causingresolve_kind()at line 696 to timeout waiting for a non-existent job.Use the returned id directly and eliminate the local
MorphPayloadBuilderAttributes::try_newcall since it's only needed for its payload_id.♻️ Proposed refactor
- let builder_attrs = - MorphPayloadBuilderAttributes::try_new(parent_hash, rpc_attributes.clone(), 1) - .map_err(|e| { - MorphEngineApiError::BlockBuildError(format!( - "failed to create builder attributes: {e}", - )) - })?; - let payload_id = builder_attrs.payload_id(); - let build_input = BuildNewPayload { attributes: rpc_attributes, parent_hash, cache: None, trie_handle: None, }; - let _ = self + let payload_id = self .payload_builder .send_new_payload(build_input) .await .map_err(|_| { MorphEngineApiError::BlockBuildError("failed to send build request".to_string()) })? .map_err(|e| { MorphEngineApiError::BlockBuildError(format!( "failed to receive build response: {e}" )) })?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/engine-api/src/builder.rs` around lines 664 - 703, The code pre-computes payload_id via MorphPayloadBuilderAttributes::try_new(...) and then discards the result of payload_builder.send_new_payload(...), causing resolve_kind(...) to use a locally-derived id that can diverge; instead capture the PayloadId returned by send_new_payload and use that for resolve_kind. Remove the unnecessary MorphPayloadBuilderAttributes::try_new call (and its payload_id()), change the send_new_payload call to bind its successful Result to a variable (e.g. payload_id_from_service) and pass that into payload_builder.resolve_kind(...), and keep the existing error mappings when send_new_payload fails.crates/engine-tree-ext/src/payload_validator.rs (2)
1191-1253: Timeout-race loop has a subtle issue: serial fallback result can be discarded on panic recovery.If the state-root task is
RecvTimeoutError::Timeoutand enters the poll loop, and then the task channel returnsOk(result)(line 1208), we return that result and dropseq_rx. The serial fallback task is still running in the background and will holdseq_overlay/seq_hashed_stateuntil it completes. This is benign (just wasted work + cache churn) but worth documenting; the alternative of letting the serial result race-cancel would require a cancellation token.Also: when
task_rxreturnsDisconnected(line 1216) we awaitseq_rx.recv()synchronously — if the serial task itself panics,seq_rxreturnsRecvErrorwhich is mapped to a generic ProviderError. Fine, but apanic::catch_unwindaround thespawn_blocking_namedclosure (as done inspawn_deferred_trie_task) would give a cleaner error trail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/engine-tree-ext/src/payload_validator.rs` around lines 1191 - 1253, The serial fallback task spawned with self.payload_processor.executor().spawn_blocking_named("serial-root", move || { ... }) can panic and currently that panic will be lost and leave seq_overlay/seq_hashed_state held; wrap the closure body in std::panic::catch_unwind to catch any panic from compute_state_root_serial, convert it into a ProviderResult::Err (e.g. ProviderError::other with context), and send that through seq_tx (handling send errors), so seq_rx.recv() yields a clear error instead of a generic RecvError; reference the spawned closure around spawn_blocking_named, seq_tx/seq_rx, and Self::compute_state_root_serial when applying the change.
2107-2111:block_access_list()stub — acceptable for now, but worth a tracking TODO.Returning
Noneunconditionally is fine while Morph doesn't produce BAL, sinceinput.block_access_list().transpose()?invalidate_block_with_statewill just yieldNoneand the StateRootTask path runs without BAL hints. If/when Morph adopts EIP-7928, this needs real decoding — consider linking the TODO to a tracking issue so it doesn't get lost once a future reth rebase surfaces BAL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/engine-tree-ext/src/payload_validator.rs` around lines 2107 - 2111, The stubbed function block_access_list() currently returns None unconditionally; leave behavior as-is for now but replace the TODO with a tracking-task comment and implement decoding later: update the comment inside block_access_list to reference a specific issue or tracker ID (e.g., “TODO: implement decoding for EIP-7928 — see ISSUE-XXXX”), and ensure callers like validate_block_with_state that call input.block_access_list().transpose()? continue to work; when Morph or reth adopts EIP-7928, implement decoding to return Option<Result<BlockAccessList, alloy_rlp::Error>> in block_access_list() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/primitives/src/transaction/envelope.rs`:
- Around line 236-237: The comment referencing "v0.1.0" is misleading; update
the comment in envelope.rs near the references to
reth_primitives_traits::SignedTransaction and RlpBincode to be version‑agnostic
— e.g., state that SignedTransaction has a blanket impl in upstream reth so no
explicit impl is needed and that the RlpBincode trait was removed in recent reth
releases so no impl is required, replacing the hardcoded version string with
neutral wording referencing "upstream reth" or "recent reth versions" and keep
the two symbol mentions (SignedTransaction and RlpBincode) so readers can locate
the rationale easily.
In `@crates/txpool/src/validator.rs`:
- Around line 518-524: The FIXME comment and the surrounding attribute
#![allow(dead_code, unused_imports)] indicate tests gated by #[cfg(any())] that
were disabled pending migration from MockEthProvider to a MorphPrimitives-aware
mock provider; create a tracking issue that lists this migration task, reference
the FIXME(morph-unfork) note, enumerate the affected test groups that use
MockEthProvider/EIP-1559/L1-message admission paths, and add a brief migration
plan: update MockEthProvider to implement MorphPrimitives-compatible traits (or
add a new MorphMock provider), re-enable the #[cfg]d tests, remove the temporary
#![allow(dead_code, unused_imports)] and verify unit coverage, then link the
issue in the comment above the attribute so future readers know where progress
is tracked.
---
Outside diff comments:
In `@crates/node/src/test_utils.rs`:
- Around line 8-18: The examples in test_utils.rs still destructure a
now-removed TaskManager from TestNodeBuilder::new().build(); update every
example that does let (mut nodes, _tasks, wallet) =
TestNodeBuilder::new().build().await?; to match the new two-value return (e.g.,
let (mut nodes, wallet) = TestNodeBuilder::new().build().await?;), remove the
unused _tasks variable, and adjust subsequent code that references _tasks
accordingly (affects the examples around TestNodeBuilder::new().build() and uses
of advance_chain).
In `@crates/payload/builder/src/builder.rs`:
- Around line 700-711: The code constructs NextBlockEnvAttributes with
withdrawals always Some(...) because MorphPayloadBuilderAttributes::try_new
already unwraps to default; to preserve the original semantics where None stays
None, change the withdrawals assignment in
MorphNextBlockEnvAttributes/NextBlockEnvAttributes construction to propagate the
original optional by using attributes.withdrawals.clone().map(Into::into) (or
attributes.withdrawals.clone().map(|w| w.into())), or alternatively stop
unwrap_or_default() in MorphPayloadBuilderAttributes::try_new so that None is
preserved—apply the change in the builder code paths touching
MorphNextBlockEnvAttributes and MorphPayloadBuilderAttributes::try_new to ensure
consumers that rely on None vs Some(empty) still see None when the CL omitted
withdrawals.
---
Nitpick comments:
In `@crates/engine-api/src/builder.rs`:
- Around line 664-703: The code pre-computes payload_id via
MorphPayloadBuilderAttributes::try_new(...) and then discards the result of
payload_builder.send_new_payload(...), causing resolve_kind(...) to use a
locally-derived id that can diverge; instead capture the PayloadId returned by
send_new_payload and use that for resolve_kind. Remove the unnecessary
MorphPayloadBuilderAttributes::try_new call (and its payload_id()), change the
send_new_payload call to bind its successful Result to a variable (e.g.
payload_id_from_service) and pass that into payload_builder.resolve_kind(...),
and keep the existing error mappings when send_new_payload fails.
In `@crates/engine-tree-ext/src/payload_validator.rs`:
- Around line 1191-1253: The serial fallback task spawned with
self.payload_processor.executor().spawn_blocking_named("serial-root", move || {
... }) can panic and currently that panic will be lost and leave
seq_overlay/seq_hashed_state held; wrap the closure body in
std::panic::catch_unwind to catch any panic from compute_state_root_serial,
convert it into a ProviderResult::Err (e.g. ProviderError::other with context),
and send that through seq_tx (handling send errors), so seq_rx.recv() yields a
clear error instead of a generic RecvError; reference the spawned closure around
spawn_blocking_named, seq_tx/seq_rx, and Self::compute_state_root_serial when
applying the change.
- Around line 2107-2111: The stubbed function block_access_list() currently
returns None unconditionally; leave behavior as-is for now but replace the TODO
with a tracking-task comment and implement decoding later: update the comment
inside block_access_list to reference a specific issue or tracker ID (e.g.,
“TODO: implement decoding for EIP-7928 — see ISSUE-XXXX”), and ensure callers
like validate_block_with_state that call input.block_access_list().transpose()?
continue to work; when Morph or reth adopts EIP-7928, implement decoding to
return Option<Result<BlockAccessList, alloy_rlp::Error>> in block_access_list()
accordingly.
In `@crates/evm/src/engine.rs`:
- Around line 91-98: into_parts duplicates the MorphTxEnv construction already
provided by to_tx_env; change ExecutableTxParts::into_parts for RecoveredInBlock
to delegate to the existing to_tx_env implementation instead of calling
MorphTxEnv::from_recovered_tx directly—call self.to_tx_env() (which internally
uses self.tx() and self.signer()) and return (that_tx_env, self) so any future
changes to MorphTxEnv::from_recovered_tx are honored.
In `@crates/payload/builder/src/error.rs`:
- Around line 45-48: The Storage error variant currently captures the underlying
storage error as a String which loses the original error source; change the
Storage variant in the error enum from Storage(String) to store the error as a
boxed source error (e.g. Storage(#[source] Box<dyn std::error::Error + Send +
Sync + 'static>)) and keep the existing display error message (#[error("storage
error: {0}")]) so formatting still works; update any construction sites that
currently pass a String to instead box the original error (Box::new(err)) or
convert existing errors into the boxed trait object.
🪄 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: fb77985b-bf97-4398-9ae4-61b61c47c8ce
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
Cargo.tomlcrates/consensus/src/validation.rscrates/engine-api/Cargo.tomlcrates/engine-api/src/builder.rscrates/engine-tree-ext/Cargo.tomlcrates/engine-tree-ext/src/gate.rscrates/engine-tree-ext/src/lib.rscrates/engine-tree-ext/src/payload_validator.rscrates/engine-tree-ext/src/trie_updates.rscrates/engine-tree-ext/tests/jade_boundary.rscrates/evm/Cargo.tomlcrates/evm/src/block/curie.rscrates/evm/src/block/factory.rscrates/evm/src/block/mod.rscrates/evm/src/block/receipt.rscrates/evm/src/config.rscrates/evm/src/context.rscrates/evm/src/engine.rscrates/evm/src/lib.rscrates/node/Cargo.tomlcrates/node/src/add_ons.rscrates/node/src/components/pool.rscrates/node/src/test_utils.rscrates/node/src/validator.rscrates/node/tests/it/block_building.rscrates/node/tests/it/consensus.rscrates/node/tests/it/engine.rscrates/node/tests/it/evm.rscrates/node/tests/it/hardfork.rscrates/node/tests/it/helpers.rscrates/node/tests/it/l1_messages.rscrates/node/tests/it/morph_tx.rscrates/node/tests/it/rpc.rscrates/node/tests/it/sync.rscrates/node/tests/it/txpool.rscrates/payload/builder/Cargo.tomlcrates/payload/builder/src/builder.rscrates/payload/builder/src/error.rscrates/payload/types/Cargo.tomlcrates/payload/types/src/attributes.rscrates/payload/types/src/lib.rscrates/primitives/Cargo.tomlcrates/primitives/src/header.rscrates/primitives/src/receipt/mod.rscrates/primitives/src/transaction/envelope.rscrates/revm/src/evm.rscrates/revm/src/exec.rscrates/revm/src/handler.rscrates/revm/src/tx.rscrates/rpc/src/eth/mod.rscrates/rpc/src/eth/transaction.rscrates/txpool/Cargo.tomlcrates/txpool/src/lib.rscrates/txpool/src/transaction.rscrates/txpool/src/validator.rslocal-test/reth-start.sh
💤 Files with no reviewable changes (1)
- crates/engine-api/Cargo.toml
Clippy 1.95 reports `storage.sort_by(|(a, _), (b, _)| a.cmp(b))` as a `clippy::unnecessary_sort_by` (prefer `sort_by_key`) in the `apply_curie_hard_fork` test. The fix is the clippy-suggested rewrite. No behavioural change; only the test's sort comparator is reshaped.
…1 fee cap
On reth v2.0.0 both `eth_call` and `eth_estimateGas` set
`cfg_env.disable_fee_charge = true` (upstream `reth#18470`), which makes
revm's `calculate_caller_fee` short-circuit to `Ok(balance)` — skipping
the caller balance check and L1 fee deduction entirely on both RPC paths.
Relying on the EVM handler to enforce balance is therefore a no-op.
As a result, `eth_estimateGas` would return a gas figure even when the
caller cannot afford `value + gas * gasPrice + l1DataFee`, diverging from
morph-geth's `DoEstimateGas` which caps the binary-search `hi` by
available = balance - value - l1DataFee
allowance = available / feeCap
hi = min(hi, allowance)
and errors with "insufficient funds for l1 fee" when `available <= 0`.
This restores the `Call::caller_gas_allowance` override that mirrors
`DoEstimateGas`. The override handles both the ETH-fee and the ERC20
token-fee paths:
* ETH path (`caller_gas_allowance_with_eth`) — subtract `value`, check
`l1_fee < available`, divide by `gas_price`.
* Token path (`caller_gas_allowance_with_token`) — check ETH balance
against `value`, then cap by `min(token_balance, fee_limit)` minus the
`eth→token`-converted L1 fee (divided by `gas_price`).
The L1 fee itself is computed in `MorphEthApi::estimate_l1_fee` from
`tx_env.rlp_bytes` (populated by `MorphTransactionRequest::try_into_tx_env`)
and the current `L1GasPriceOracle` state via `L1BlockInfo::try_fetch`.
Because the `Call::caller_gas_allowance` trait hands us an
`impl revm::Database` without a `Debug` bound, `TokenFeeInfo::load_for_caller`
(which may spin up a temporary `MorphEvm` for the `balanceOf` fallback)
cannot be used. This commit adds a sibling `TokenFeeInfo::load_storage_only`
that reads the registry entry and — when the token's `balance_slot` is
known — the caller's ERC20 balance directly from contract storage, no
EVM needed. When `balance_slot` is unknown the token-cap branch falls
back to the ETH allowance and lets the EVM handler re-check the balance
during `executable(gas)` execution (see `validate_and_deduct_token_fee`).
Three new error variants are added to `MorphEthApiError`
(`InsufficientFundsForTransfer`, `InsufficientFundsForL1Fee`,
`InvalidFeeToken`), mapping to JSON-RPC error code `-32000` to match
go-ethereum's string-payload behaviour.
`MorphTransactionRequest::try_into_tx_env` now unconditionally populates
`rlp_bytes`. The handler ignores them on both RPC paths (because
`disable_fee_charge` is true), and the `caller_gas_allowance` override
needs them to derive the L1 data fee. The outdated `if !disable_fee_charge
{ encode } else { None }` branch — which relied on pre-v2.0.0 semantics
where the handler would consume the RLP — is removed along with its
comment block and two unit tests that asserted the dropped behaviour.
Verified with `cargo check --all`, `cargo fmt --all -- --check`,
`cargo clippy --all --all-targets -- -D warnings`, `cargo test --all`
(all passing), and `make test-e2e` (77/77 passing).
Exercises the `MorphEthApi::caller_gas_allowance` override at the full JSON-RPC surface, which is where the regression would manifest in production: - `estimate_gas_reports_insufficient_funds_for_l1_fee` — unfunded sender, zero-value transfer. Balance = 0, value = 0, but a non-zero L1 data fee makes `l1_fee >= available`, so the cap returns `MorphEthApiError::InsufficientFundsForL1Fee`. Confirms the L1 fee check fires even when the caller has no ETH at all. - `estimate_gas_reports_insufficient_funds_for_transfer` — unfunded sender, non-zero `value`. The `value > balance` check in the override fires before L1 fee computation, returning `MorphEthApiError::InsufficientFundsForTransfer`. Both tests assert the error payload contains the exact go-ethereum error strings (`"insufficient funds for l1 fee"` / `"insufficient funds for transfer"`), so future reshuffles of the override are still user-visible at the RPC boundary. Without this PR, both calls would silently succeed (handler's `disable_fee_charge = true` short-circuit), and the test assertions would fail on `result.expect_err(...)`. Verified: - `cargo nextest run -p morph-node --features test-utils -E binary(it) -- "estimate_gas_reports_insufficient"` — 2/2 pass - `make test-e2e` — 79/79 pass (77 existing + 2 new)
Replaces our hand-written `(balance - value) / gas_price` implementation
in the ETH branch with a delegation to upstream
`alloy_evm::call::caller_gas_allowance`, then subtracts the L1 data fee
expressed in gas units. The upstream helper is a public free function
bound only to `revm::Database`, so it composes cleanly with the reth
`Call::caller_gas_allowance` trait signature — no wrapper, no bound
juggling.
Changes:
* `caller_gas_allowance` ETH branch now calls
`upstream_caller_gas_allowance` and maps its `CallError` into
`MorphEthApiError::{Eth, InsufficientFundsForTransfer}`. L1 fee
handling becomes three lines:
let l1_fee_gas = saturating_div_u128(l1_fee, gas_price);
if l1_fee_gas >= base { return Err(InsufficientFundsForL1Fee); }
Ok(base - l1_fee_gas)
* Drops the bespoke `caller_gas_allowance_with_eth` helper (its three
jobs — read balance, subtract value, divide by gas_price — are now
handled upstream).
* Introduces `saturating_div_u128` as the single division primitive
shared by the ETH-branch L1-fee conversion and by the token path's
`gas_allowance_from_balance`.
* `caller_gas_allowance_with_token` still reads the ETH balance
directly (token path enforces `value <= balance` on ETH, then caps by
token allowance), but no longer threads the balance through from the
caller — it fetches it where needed, which is the only place that
uses it on this branch.
* Adds `alloy-evm` to `crates/rpc/Cargo.toml` (was only transitively
available).
No behavioural change. `cargo fmt`, `cargo clippy --all --all-targets
--features test-utils -- -D warnings`, `cargo test --all`, and
`make test-e2e` (79/79 passing, including the two new balance-cap
e2e tests) all stay green.
`Call::caller_gas_allowance` is a shared hook — upstream invokes it
from three paths:
* `EstimateCall::estimate_gas_with` (`eth_estimateGas`)
* `Call::prepare_call_env` (`eth_call`)
* `EthCall::create_access_list_with` (`eth_createAccessList`)
morph-geth's `DoCall` uses `ApplyMessage(..., Big0)` and never rejects
`eth_call` on L1-fee grounds. Our previous override applied the L1 fee
cap from every call site, making `eth_call` over-reject unfunded
senders with `"insufficient funds for l1 fee"` — a regression relative
to morph-geth.
Fix: key the L1 fee cap off `cfg_env.disable_block_gas_limit`, which
upstream sets at each call site as follows:
estimate_gas_with false (default)
prepare_call_env (call) true
create_access_list_with true
When the flag is `true` we short-circuit to upstream's
`(balance − value) / gas_price` default and skip the L1 fee deduction,
leaving `eth_call` aligned with morph-geth `DoCall`. Only the
`estimate_gas_with` path (flag `false`) exercises the Morph L1 fee
extension, matching `DoEstimateGas`'s `available.Sub(available,
l1DataFee)` semantics.
New e2e test `eth_call_does_not_reject_unfunded_sender_on_l1_fee`
guards against regression. Existing estimateGas e2e tests continue to
pass (full suite 80/80).
Known gap (tracked as P2-E in the followup doc): `eth_createAccessList`
with an explicit gas limit bypasses `caller_gas_allowance` entirely, so
our heuristic cannot reach it. Fixing that path requires a full
`create_access_list_with` override (drops `disable_fee_charge` to let
the handler deduct the L1 fee, so the inspector traces the
`L1GasPriceOracle` reads into the access list). That is an independent
~100-line change + a new `revm-inspectors` dependency; deferred to
avoid bloating this PR with a low-impact precision regression.
Verified: `cargo fmt --all -- --check`, `cargo clippy --all
--all-targets --features test-utils -- -D warnings`, `cargo test
--all`, `make test-e2e` (80/80 passing).
On the `fee_token_id > 0` path the gas and L1 data fee are paid in the
fee token, not in ETH. The ETH balance only needs to cover `tx.value`.
Both morph-geth (`api.go:1296-1334` — `allowance := altByEth / feeCap`,
no ETH-balance term) and our own handler
(`validate_and_deduct_token_fee`: "Check that caller has enough ETH to
cover the value transfer") agree on this.
The previous code computed
eth_available = eth_balance − value
eth_allowance = eth_available / gas_price
...
return Ok(eth_allowance.min(token_allowance))
which caps a token-fee request by the sender's ETH balance divided by
`gas_price`. A valid token-fee sender with zero ETH but enough tokens
would be capped to 0; a sender with a small ETH balance would be
artificially capped well below what they can actually afford in
tokens. That contradicts both morph-geth's behaviour and the handler's
later `validate_and_deduct_token_fee` semantics.
Fix:
* Drop the `eth_allowance` cap entirely on the token path.
* Keep the `eth_balance >= value` check so a caller that cannot even
fund the value transfer still gets rejected (with the usual
`InsufficientFundsForTransfer` error).
* When the token's `balance_slot` is unknown, return `u64::MAX`
instead of `eth_allowance` — the EVM handler's
`validate_and_deduct_token_fee` will run the real token balance
check during the binary-search `executable(gas)` call. Returning
`u64::MAX` means no RPC-side cap; the caller's allowance is
effectively bounded only by the block gas limit.
No behaviour change on the ETH-fee path (`fee_token_id == 0` or
absent). `cargo fmt`, `cargo clippy --all --all-targets --features
test-utils -- -D warnings`, `cargo test --all`, and `make test-e2e`
(80/80) all green.
The earlier refactor composed two floor divisions:
base = floor((balance − value) / gas_price) // upstream helper
l1_fee_gas = floor(l1_fee / gas_price)
allowance = base − l1_fee_gas
That differs from go-ethereum's DoEstimateGas, which subtracts in wei
before dividing:
allowance = floor((balance − value − l1_fee) / gas_price)
The two disagree whenever the remainders cross a gas-price boundary.
Concretely, with balance=15 wei, value=0, l1_fee=6 wei, gas_price=10 wei:
floor(15/10) − floor(6/10) = 1 − 0 = 1 (our previous return)
floor((15 − 0 − 6) / 10) = floor(9/10) = 0 (morph-geth)
So the previous code could report a gas allowance one unit above what
the sender can actually afford, letting `eth_estimateGas` return a
value the tx can't pay for.
Fix: load balance directly, subtract `value` and `l1_fee` at wei
precision via `checked_sub` (mapping underflow to
`InsufficientFundsForTransfer` / `InsufficientFundsForL1Fee`), then
divide once via `gas_allowance_from_balance`. This matches morph-geth
exactly.
Trade-off: we give up delegating to
`alloy_evm::call::caller_gas_allowance` on this branch — the upstream
helper internally divides before we see the wei remainder, so there's
no way to subtract the L1 fee at wei precision afterwards. The
`disable_block_gas_limit` short-circuit (eth_call / createAccessList
paths) still delegates to upstream, where wei-level precision doesn't
matter.
Adds three unit tests to lock the rounding behaviour:
* `gas_allowance_subtracts_l1_fee_at_wei_precision` — reviewer's
numeric example (15/0/6/10 → 0; one wei more → 1).
* `saturating_div_u128_handles_zero_divisor_and_overflow` — divisor=0
returns 0; `U256::MAX / 1` saturates to `u64::MAX`.
* `gas_allowance_with_zero_gas_price_returns_u64_max` — the special
case inside `gas_allowance_from_balance`.
Verified: `cargo fmt --all -- --check`, `cargo clippy --all
--all-targets --features test-utils -- -D warnings`, `cargo test
--all`, and `make test-e2e` (80/80) all green.
…calls The previous `disable_block_gas_limit` short-circuit in `Call::caller_gas_allowance` delegated all `eth_call` / `eth_createAccessList` requests to upstream `alloy_evm::call::caller_gas_allowance`, which caps by `(ETH_balance − value) / gas_price`. That is correct for plain ETH calls but breaks MorphTx token-fee calls: * A token-fee caller legitimately may hold zero or minimal ETH — gas and the L1 data fee are paid from the fee token. * With `ETH_balance = 0`, the upstream helper either sets the allowance to 0 (gas_price > 0) or returns `InsufficientFundsForTransfer` when `value > 0`. * morph-geth's `DoCall` runs token-fee tx through `validate_and_deduct_token_fee` without an RPC-layer ETH-based cap, so the Morph behaviour must not cap here either. Fix: inside the short-circuit, special-case `fee_token_id > 0` and return `u64::MAX` (no RPC-side cap), mirroring morph-geth's `ApplyMessage(..., Big0)` + handler token-path semantics. The EVM handler still performs the real ETH-value + token-balance checks during execution. Adds e2e `eth_call_token_fee_does_not_reject_zero_eth_sender`: an unfunded random sender calling through `eth_call` with `feeTokenID = 1` and non-zero `gasPrice` must not surface `"insufficient funds for transfer"` or `"insufficient funds for l1 fee"`. Without this fix the call is rejected on ETH grounds before the token-fee handler ever runs. Verified: `cargo fmt --all -- --check`, `cargo clippy --all --all-targets --features test-utils -- -D warnings`, `cargo test --all`, and `make test-e2e` (81/81, +1 new) all green.
Previously the ETH path of `caller_gas_allowance` used `checked_sub`
for the L1 fee deduction, which passed the boundary case
`l1_fee == available` through as `allowance = 0`. The binary search
then had to fail on its own with "gas required exceeds allowance 0",
which obscures the real reason: the caller has literally zero wei
left after paying `tx.value` and the L1 fee, so no gas can be bought
at any price.
Catch it at the RPC layer with an explicit `>=` check:
if l1_fee >= available {
return Err(MorphEthApiError::InsufficientFundsForL1Fee);
}
This is not pure geth parity — geth happens to use the same `>=`
inequality, but the argument here stands on its own: the allowance
is monotonic in `(available − l1_fee)`, so the point where no gas
fits is exactly when that difference is non-positive. Failing at the
RPC layer surfaces the specific cause the moment it's known, instead
of deferring to a generic binary-search error.
Adds three inline unit tests in `eth::call::tests` that mirror the
ETH-path allowance math:
* `eth_path_l1_fee_equal_to_available_errors_early` — the boundary
itself; regressing to `checked_sub` would flip this to
`allowance = 0`.
* `eth_path_one_wei_above_l1_fee_yields_positive_allowance` — sanity
companion that one wei of slack buys exactly 1 gas at
`gas_price = 1`.
* `eth_path_value_exceeds_balance_errors_before_l1_fee_check` —
`InsufficientFundsForTransfer` still fires before the L1 fee check
if the caller cannot cover `tx.value`.
Verified: `cargo fmt --all -- --check`, `cargo clippy --all
--all-targets --features test-utils -- -D warnings`, `cargo test
--all`, and `make test-e2e` (81/81) all green.
`calculate_caller_fee_with_l1_cost` only skipped the balance check on simulation paths but kept the deduction, silently lowering the simulated caller balance seen by EVM code reading SELFBALANCE or caller.balance. Short-circuit at the top of the function so the pre-sim balance passes through unchanged, mirroring revm's own calculate_caller_fee. Covers eth_call / eth_estimateGas / eth_createAccessList. Also generalises two nearby comments that only mentioned eth_call to cover all three simulation paths, and trims verbose doc-style commentary across eth/call.rs and eth/transaction.rs. Unit test calculate_caller_fee_is_short_circuited_by_disable_fee_charge locks the short-circuit.
The active Jade gate lives in engine-tree-ext::gate::state_root_enforced_at. should_validate_state_root and MorphValidationContext were left behind when the gate moved during the v2.0.0 unfork and have zero workspace callers.
Upstream reth (#21761, v1.11.0) builds its container images with target-cpu=x86-64-v3 to take advantage of AVX2 / BMI2 / POPCNT on trie / keccak / receipt hot paths. Haswell (2013) / Excavator (2015) and newer support this; users on older CPUs can opt out with --build-arg RUSTFLAGS="".
Two related correctness fixes to the morph engine API's interaction with
reth's engine tree.
1. Strict PayloadStatus on import paths
Splits ensure_payload_status_acceptable into two helpers:
- payload_status_is_validated(&PayloadStatus) -> bool, used by
validate_l2_block (returns GenericResponse{success: bool})
- ensure_payload_status_valid(&PayloadStatus, ctx) -> Result<()>,
used by import + FCU paths
PayloadStatusEnum::Accepted is now an explicit error on import — it
signals "received but not fully validated", which must not propagate
to set_canonical_head. reth v2.0.0's main engine tree never returns
Accepted from the synchronous newPayload/FCU path, so this is a
future-proofing tightening rather than a live-bug fix.
2. Event-source-aware EngineStateTracker
The local FCU sync path keeps writing record_local_head unconditionally
— the caller knows it just succeeded the FCU, and the next engine-API
call must see the new head before reth's asynchronous event has
propagated. The engine-event listener in MorphAddOns now goes through
record_canonical_event_if_authoritative, which compares the event
against provider.chain_info() before writing the tracker.
reth's engine event channel is UnboundedSender (FIFO but unbounded),
so under high import throughput or transient backpressure the listener
can lag and a stale event from a prior canonical head can arrive after
the FCU sync path has already recorded a newer head. Without this
filter the tracker silently regresses and the next assembleL2Block
fails with DiscontinuousBlockNumber until a fresh event arrives.
Resolution rests on a reth invariant we verified: on_canonical_chain_update
updates CanonicalInMemoryState before emitting CanonicalChainCommitted,
so a fresh event always matches provider.chain_info() and a mismatch
proves the event is stale (or reth has reorged elsewhere). This stays
compatible with the upcoming engine_newL2BlockV2 reorg flow where head
numbers can legitimately decrease — there is no monotonic guard, only
a "trust the provider" arbiter.
Also reverses the order of provider.set_canonical_head() and
record_local_head() inside import_l2_block_via_engine. Setting the
provider canonical head FIRST closes a multi-thread race window where
a stale CanonicalChainCommitted(N-1) processed by the listener task
between record_local_head(N) and set_canonical_head(N) would observe
provider canonical = N-1, pass the authority check, and overwrite the
tracker back to N-1. After the swap, any stale event reaching the
listener is either dropped (provider already at N) or transiently
regresses the tracker before our subsequent record_local_head(N)
unconditionally restores it.
Adds 9 unit tests:
- 2 covering payload_status_is_validated / ensure_payload_status_valid
- 5 covering record_canonical_event_if_authoritative (provider match,
hash mismatch, number-match-hash-differ, provider unavailable,
non-canonical event variant)
- (existing tracker tests preserved)
…tant c63b750 migrated payload-builder + engine-api hot-path Instants to reth's FastInstant (TSC-backed, ~5ns vs ~30ns for clock_gettime), but missed config.rs. PayloadBuildingBreaker::should_break is called once per mempool transaction considered during build_payload — typical morph blocks consider hundreds of txs, so the leftover std::time::Instant costs ~25ns × N per block on the leader's critical path. Single-line import swap; FastInstant is API-compatible with std::time::Instant (now(), elapsed()).
…ng payload_id assemble_l2_block called MorphPayloadBuilderAttributes::try_new() purely to obtain the payload_id, but try_new RLP-decodes every L1 message and recovers the signer (~50-100µs per message). The resulting builder_attrs was then discarded — BuildNewPayload carries the original rpc_attributes and build_payload runs try_new again internally. Extracts MorphPayloadAttributes::morph_payload_id(&parent_hash) so the engine API can hash the metadata directly without touching the transaction list. PayloadAttributes::payload_id trait impl delegates to the same inherent method, keeping its byte-identical behavior intact. Saves a full RLP-decode + ECDSA-recover pass per assembleL2Block on the leader's hot path; at a 50-deposit L1 surge this trims roughly 2.5-5ms per block. New unit test pins the contract: morph_payload_id must hash metadata deterministically without decoding the transactions field, so malformed tx bytes still produce the same payload_id.
Pure cleanup, zero behavior change:
- Comments: strip "Pre-fix this branch returned ..." / "Ported from
morph-reth-enginevalidator-spike commit ..." / "NOTE(morph): Upstream
reth ..." references that narrated the unfork-migration history. Test
docstrings are rewritten to express the regression as an invariant
("must clamp to gas_cap, never bypass --rpc.gascap") instead of
referring to git history; lib.rs drops the spec/task references that
rot after merge.
- pool.rs: morph_evm::evm::MorphEvmFactory → morph_evm::MorphEvmFactory
(re-export already exposed at crate root).
- payload-builder/error.rs: remove the unused Database(#[from] ProviderError)
variant; all call sites use Storage(String). Drops the unused
reth_evm::execute::ProviderError import along with it.
Two independent correctness fixes flagged in the audit Medium tier:
1. set_block_tags reorders writes (engine-api/builder.rs)
The Ethereum invariant `finalized.number <= safe.number` must hold at
every point an RPC reader can observe. Updating finalized first and
then safe leaves a window where eth_getBlockByNumber("finalized")
returns the new value but eth_getBlockByNumber("safe") still returns
the older value — a transient finalized > safe violation. Swap to
safe-first / finalized-second so finalized stays at its older smaller
value while safe advances, preserving the invariant throughout.
2. build_candidate_block test helper (engine-tree-ext/tests/jade_boundary.rs)
The helper was sleep(500ms) + poll best_payload until 10s deadline.
The fixed warmup was unreliable on loaded CI (sometimes the builder
hadn't published yet) and the 10s ceiling was tight under contention.
Replaced with PayloadBuilderHandle::resolve_kind(WaitForPending) +
tokio::time::timeout(30s) — same pattern engine_api/builder.rs uses
in production. No fixed sleep, no race window, looser ceiling.
Locks the fix from commits 146aa86 (sload_morph) and 53907ae (sstore_morph), which restore `original_value` after revm's mark_warm_with_transaction_id() corrupts it on cold→warm transitions of slots that token-fee deduction marked cold. The test reproduces the shape of mainnet tx 0xc267...4913c9 in block 19720219: a MorphTx V0 paying fees in the same ERC20 contract that the main tx targets. handler.rs marks the fee token's storage slots cold after deduction (both slot-based and EVM-call modes); the main tx's first SLOAD on the sender's balance slot then triggers mark_warm_with_transaction_id() — without the fix, original_value is reset to the post-deduction value and EIP-2200 charges SSTORE_RESET (2900) instead of dirty (100), diverging from morph-geth. EVM/EL mismatch caused mainnet sync to stall for every node that hadn't shipped the fix. EXPECTED_GAS_USED = 48_128 is the sandbox-specific golden (the mainnet block's 59_335 figure uses different bytecode + state and doesn't apply directly). What's locked is the bug-vs-fix delta — a regression adds ~2800 gas from the mis-charged SSTORE and trips the assertion before the change reaches mainnet. Adds: - TestNodeBuilder::with_account_code() helper to inject runtime bytecode into the test genesis for arbitrary accounts. - SLOT1_ERC20_RUNTIME_CODE: minimal ERC20 with balanceOf at slot 1, matching the test token registry's direct-slot configuration.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Three independent CI failures, each from the unfork landing: 1. cargo-deny source-not-allowed: PR repins reth to paradigmxyz/reth, but deny.toml [sources].allow-git only had morph-l2/reth. Add the upstream URL so all `git+https://github.com/paradigmxyz/reth?rev=...` crates pass the source check. 2. cargo-deny RUSTSEC-2026-0104: rustls-webpki 0.103.12 has a reachable panic in CRL parsing (BorrowedCertRevocationList::from_der). Fix shipped in 0.103.13 (2026-04-21). cargo update -p rustls-webpki bumps cleanly with no other lockfile churn. 3. CodeQL hardcoded-nonce critical: line 556 in the new H7 regression test passes a literal 0 as the third positional arg (nonce) to MorphTxBuilder::new, which CodeQL flags as a hardcoded credential. Replace with wallet.inner_nonce — a fresh wallet starts at 0, so behavior is unchanged, but CodeQL no longer sees a literal value.
Two cleanups bundled because they reinforce each other:
1. Run `cargo update` (no `-p`) to refresh every transitive dep to its
latest semver-compatible version. 22 patch/minor bumps, no major
changes — most notably:
rustls 0.23.38 → 0.23.39
rand 0.8.5 → 0.8.6
blake3 1.8.4 → 1.8.5
rustls-pki-types 1.14.0 → 1.14.1
Patch-level only, no API churn.
2. Drop three RUSTSEC ignores that cargo-deny was already reporting as
"advisory not encountered" — i.e. the vulnerable crates were no longer
in the dep tree (paradigmxyz/reth v2.0.0 + recent rustls-webpki bumps
already shipped the fixes):
RUSTSEC-2026-0002 (lru 0.12.x unsound) — gone, lru upgraded
RUSTSEC-2026-0098 (rustls-webpki URI) — fixed in 0.103.13
RUSTSEC-2026-0099 (rustls-webpki wild) — fixed in 0.103.13
Ignore list shrinks from 5 → 3. Also rewrote the RUSTSEC-2026-0097
note to record that it's a false positive in our usage (we don't
install a custom rand logger), since cargo-deny can't tell.
Single-host local testing doesn't need UPnP/STUN/PMP probing; skip it to avoid noisy startup attempts and unnecessary ENR updates.
Recent dependency upgrades require rustc 1.93+ to build; sync the declared MSRV in workspace Cargo.toml with what we actually need, and update the README/CONTRIBUTING prerequisites to match.
Align declared MSRV with the toolchain we actually use locally.
Use `--nat none` so local reth startup does not attempt public IP probe services on restricted networks. Constraint: Keep the change scoped to local test startup parameters Confidence: high Scope-risk: narrow
Upgrade the reth, alloy, and revm stack together and carry Morph's existing execution, payload, and RPC semantics across the breaking API changes. Constraint: Amsterdam/EIP-8037 semantics remain deferred to a separate fork PR. Confidence: medium Scope-risk: broad
Add receipt conversion coverage for block receipts and the reth v2.2 transactionReceipts pubsub path so Morph-specific receipt metadata stays visible over RPC. Constraint: Reuse MorphReceiptConverter instead of adding custom pubsub wiring. Confidence: high Scope-risk: narrow
Keep the hardfork tests focused on SpecId mappings and EIP-7702 refund behavior so future fork work cannot silently enable Amsterdam state gas. Constraint: Amsterdam fork support is intentionally deferred. Confidence: high Scope-risk: narrow
panos-xyz
changed the title
Resolve the PR 98 conflicts by keeping the upstream reth v2.2.0 migration while carrying forward main's reference-index RPC work, Docker build flag updates, cargo-deny advisory ignores, and Curie dead-code removal. Constraint: Stop on ambiguous semantic conflicts; only clear mechanical/API merge conflicts were resolved. Confidence: medium Scope-risk: broad Not-tested: cargo check -p morph-node was manually backgrounded before a final exit code was available
Allow the sigp/discv5 source pulled transitively by upstream reth v2.2.0 and remove the stale morph-l2/reth allow entry now that this branch no longer depends on the fork. Constraint: Fix the failing cargo-deny sources check without broadening source policy beyond observed dependencies. Confidence: high Scope-risk: narrow
Match the reth-codecs 0.3 Decompress trait by returning DecompressError from reference-index table value codecs. Constraint: Do not run further compile checks per request. Confidence: high Scope-risk: narrow Not-tested: Compile checks skipped at user request
Update Cargo.lock after the workspace version bump so morph-engine-tree-ext matches the current 0.3.0 package version. Constraint: Generated by cargo check as requested. Confidence: high Scope-risk: narrow
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/revm/src/token_fee.rs (1)
98-139: ⚡ Quick winAdd unit coverage for the unknown-balance-slot sentinel behavior.
load_storage_onlyintentionally returnsbalance = 0withbalance_slot = Nonewhen the slot is unknown. Please add targeted tests for this contract so future refactors don’t accidentally treat it as a real zero balance.🤖 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/revm/src/token_fee.rs` around lines 98 - 139, Write a unit test that exercises TokenFee::load_storage_only to assert the sentinel behavior: when read_registry_entry returns Some(entry) with entry.balance_slot = None the returned Option is Some(token_fee) where token_fee.balance == U256::ZERO and token_fee.balance_slot == None (not treated as a real zero balance), and when balance_slot is Some(...) it calls read_balance_from_storage and returns that value; use a mock or test DB implementing RevmDatabase to control read_registry_entry/read_balance_from_storage results and assert both paths (unknown slot vs known slot) using the TokenFee::load_storage_only function name as the target under test.crates/rpc/src/error.rs (1)
67-88: ⚡ Quick winAdd unit assertions for the new RPC error variants.
These variants change the public RPC contract, but
error_display_messagesanderror_to_json_rpc_error_codesstill only pin the older cases. A few exact assertions here for the new message/code pairs would catch drift locally, especially forInvalidFeeToken.Also applies to: 136-148
🤖 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/rpc/src/error.rs` around lines 67 - 88, The new RPC error variants InsufficientFundsForTransfer, InsufficientFundsForL1Fee, and InvalidFeeToken changed the public RPC contract but there are no unit assertions for their display messages and JSON-RPC codes; add unit tests that call the existing error_display_messages and error_to_json_rpc_error_codes helpers (or the functions used by them) and assert the exact string produced for each of these three enum variants and the exact numeric/error-code mapping expected for InvalidFeeToken (and mirror that pattern for the other two), ensuring the new message/code pairs are covered and will fail the test if they drift.crates/rpc/src/eth/transaction.rs (1)
53-58: ⚡ Quick winAdd one non-
Nonetest forblock_timestamppropagation.This field is only forwarded here, and the updated tests still build
TransactionInfowithblock_timestamp: Nonein every case. A single positive assertion would lock in the new RPC field.🤖 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/rpc/src/eth/transaction.rs` around lines 53 - 58, Tests currently only construct TransactionInfo with block_timestamp: None; add a positive test that builds a TransactionInfo with block_timestamp: Some(non_zero_value) and assert that the RPC Transaction produced by the code path that uses transaction::block_timestamp (the block_timestamp: tx_info.block_timestamp forwarding in the TransactionInfo -> RPC Transaction conversion) contains that same non-None value. Locate the test that exercises TransactionInfo -> RPC serialization (or create a new unit test), set tx_info.block_timestamp = Some(<valid timestamp>), call the conversion/path that uses transaction_index/ block_timestamp, and assert equality of the block_timestamp field on the resulting RPC struct.crates/engine-tree-ext/tests/jade_boundary.rs (1)
92-116: ⚡ Quick winStrengthen post-Jade rejection assertion to distinguish
INVALIDfrom other non-VALIDstatuses.
try_import_with_tampered_state_rootcollapses the engine response tostatus.is_valid(), so the post-Jade test'sassert!(!accepted, ...)will pass for any non-VALIDoutcome — includingSYNCINGorACCEPTED— even though the regression you actually want to catch is "engine returnedINVALIDbecause MPT state-root equality was enforced". In practiceSYNCING/ACCEPTEDshould not occur here since the parent is known, but a future change to engine semantics could silently weaken this test without any failure.Returning the full
PayloadStatuslets each caller assert the precise expected status.♻️ Proposed change to surface the full status
-async fn try_import_with_tampered_state_root( - node: &mut MorphTestNode, - base: &MorphBuiltPayload, - bogus_state_root: B256, -) -> eyre::Result<bool> { +async fn try_import_with_tampered_state_root( + node: &mut MorphTestNode, + base: &MorphBuiltPayload, + bogus_state_root: B256, +) -> eyre::Result<alloy_rpc_types_engine::PayloadStatus> { ... - let status = node + let status = node .inner .add_ons_handle .beacon_engine_handle .new_payload(execution_data) .await?; - Ok(status.is_valid()) + Ok(status) }Then in the post-Jade test:
- let accepted = - try_import_with_tampered_state_root(&mut node, &base_payload, B256::from([0xFF; 32])) - .await?; - - assert!( - !accepted, - "post-Jade block with tampered state_root must be rejected — MorphBasicEngineValidator \ - enforces strict MPT root equality after Jade" - ); + let status = + try_import_with_tampered_state_root(&mut node, &base_payload, B256::from([0xFF; 32])) + .await?; + + assert!( + status.is_invalid(), + "post-Jade block with tampered state_root must be rejected as INVALID — got {status:?}" + );Also applies to: 141-162
🤖 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/tests/jade_boundary.rs` around lines 92 - 116, The test helper try_import_with_tampered_state_root currently returns a bool via status.is_valid(), which masks non-VALID payload statuses; change it to return the full PayloadStatus (the engine response) instead of bool so callers can assert the exact expected status (e.g., equals PayloadStatus::Invalid). Update the function signature and return value in try_import_with_tampered_state_root (and the other similar helper around the other occurrence) to propagate the engine's PayloadStatus, and adjust the post-Jade test to assert that the returned status == PayloadStatus::Invalid rather than assert!(!accepted).crates/payload/builder/src/error.rs (1)
40-42: ⚡ Quick winConsider preserving structured error information.
The replacement of
Database(#[from] ProviderError)withStorage(String)loses:
- Automatic
ProviderError→MorphPayloadBuilderErrorconversion (via#[from])- Structured error context that could aid debugging
If storage errors are mapped manually in calling code (e.g., via
.map_err(|e| MorphPayloadBuilderError::Storage(e.to_string()))), consider whether preserving the original error type or using a boxeddyn Errorwould provide better diagnostics.💡 Alternative: Preserve error structure
- /// Generic storage error (e.g. from revm EvmDatabaseError, ProviderError). - #[error("storage error: {0}")] - Storage(String), + /// Generic storage error (e.g. from revm EvmDatabaseError, ProviderError). + #[error("storage error: {0}")] + Storage(#[source] Box<dyn std::error::Error + Send + Sync>),🤖 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/payload/builder/src/error.rs` around lines 40 - 42, The Storage error variant replaced the previous Database(#[from] ProviderError) and lost automatic conversion and structured context; restore structured errors by changing the enum variant to accept the original ProviderError via #[from] (e.g., Database(#[from] ProviderError) or rename back to Database) or switch Storage to hold a boxed error type (Storage(Box<dyn std::error::Error + Send + Sync + 'static>)) so automatic From conversions and rich error context are preserved; update MorphPayloadBuilderError in error.rs accordingly and adjust call sites that currently map errors with to_string() to rely on the new From or boxed-error variant.
🤖 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/node/tests/it/morph_tx.rs`:
- Around line 563-571: The test currently grabs the first transaction via
payload.block().body().transactions.first() which can silently pick the wrong tx
if harness behavior changes; make selection deterministic by asserting the
transaction count before taking the first element: capture transactions into a
local (e.g., let txs = payload.block().body().transactions;),
assert_eq!(txs.len(), 1, "expected exactly one transaction in block for this
regression test"), and then use txs.first().tx_hash() to set tx_hash; adjust the
expected count if the test should allow more than one.
In `@crates/revm/src/handler.rs`:
- Around line 267-277: The current unwrap_or_else on
validate_initial_tx_gas(...) swallows all validation errors; instead, call
validate_initial_tx_gas and pattern-match its Result so only the specific
"intrinsic gas > gas_limit" error is converted to
InitialAndFloorGas::new(tx.gas_limit(), 0), while any other Err is propagated
(or returned) unchanged. Reference validate_initial_tx_gas,
InitialAndFloorGas::new(tx.gas_limit(), 0) and the surrounding handler code so
you implement a match/if-let that maps only the intrinsic-gas-too-high variant
to the fallback and returns/propagates other errors.
---
Nitpick comments:
In `@crates/engine-tree-ext/tests/jade_boundary.rs`:
- Around line 92-116: The test helper try_import_with_tampered_state_root
currently returns a bool via status.is_valid(), which masks non-VALID payload
statuses; change it to return the full PayloadStatus (the engine response)
instead of bool so callers can assert the exact expected status (e.g., equals
PayloadStatus::Invalid). Update the function signature and return value in
try_import_with_tampered_state_root (and the other similar helper around the
other occurrence) to propagate the engine's PayloadStatus, and adjust the
post-Jade test to assert that the returned status == PayloadStatus::Invalid
rather than assert!(!accepted).
In `@crates/payload/builder/src/error.rs`:
- Around line 40-42: The Storage error variant replaced the previous
Database(#[from] ProviderError) and lost automatic conversion and structured
context; restore structured errors by changing the enum variant to accept the
original ProviderError via #[from] (e.g., Database(#[from] ProviderError) or
rename back to Database) or switch Storage to hold a boxed error type
(Storage(Box<dyn std::error::Error + Send + Sync + 'static>)) so automatic From
conversions and rich error context are preserved; update
MorphPayloadBuilderError in error.rs accordingly and adjust call sites that
currently map errors with to_string() to rely on the new From or boxed-error
variant.
In `@crates/revm/src/token_fee.rs`:
- Around line 98-139: Write a unit test that exercises
TokenFee::load_storage_only to assert the sentinel behavior: when
read_registry_entry returns Some(entry) with entry.balance_slot = None the
returned Option is Some(token_fee) where token_fee.balance == U256::ZERO and
token_fee.balance_slot == None (not treated as a real zero balance), and when
balance_slot is Some(...) it calls read_balance_from_storage and returns that
value; use a mock or test DB implementing RevmDatabase to control
read_registry_entry/read_balance_from_storage results and assert both paths
(unknown slot vs known slot) using the TokenFee::load_storage_only function name
as the target under test.
In `@crates/rpc/src/error.rs`:
- Around line 67-88: The new RPC error variants InsufficientFundsForTransfer,
InsufficientFundsForL1Fee, and InvalidFeeToken changed the public RPC contract
but there are no unit assertions for their display messages and JSON-RPC codes;
add unit tests that call the existing error_display_messages and
error_to_json_rpc_error_codes helpers (or the functions used by them) and assert
the exact string produced for each of these three enum variants and the exact
numeric/error-code mapping expected for InvalidFeeToken (and mirror that pattern
for the other two), ensuring the new message/code pairs are covered and will
fail the test if they drift.
In `@crates/rpc/src/eth/transaction.rs`:
- Around line 53-58: Tests currently only construct TransactionInfo with
block_timestamp: None; add a positive test that builds a TransactionInfo with
block_timestamp: Some(non_zero_value) and assert that the RPC Transaction
produced by the code path that uses transaction::block_timestamp (the
block_timestamp: tx_info.block_timestamp forwarding in the TransactionInfo ->
RPC Transaction conversion) contains that same non-None value. Locate the test
that exercises TransactionInfo -> RPC serialization (or create a new unit test),
set tx_info.block_timestamp = Some(<valid timestamp>), call the conversion/path
that uses transaction_index/ block_timestamp, and assert equality of the
block_timestamp field on the resulting RPC struct.
🪄 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: 22c38b12-ae80-4200-9ff3-e257c3329b31
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
CONTRIBUTING.mdCargo.tomlREADME.mdbin/morph-reth/Cargo.tomlbin/morph-reth/src/main.rscrates/chainspec/src/hardfork.rscrates/consensus/src/validation.rscrates/engine-api/src/builder.rscrates/engine-api/src/lib.rscrates/engine-api/src/validator.rscrates/engine-tree-ext/src/lib.rscrates/engine-tree-ext/src/payload_validator.rscrates/engine-tree-ext/tests/jade_boundary.rscrates/evm/Cargo.tomlcrates/evm/src/assemble.rscrates/evm/src/block/factory.rscrates/evm/src/block/mod.rscrates/evm/src/block/receipt.rscrates/evm/src/config.rscrates/evm/src/context.rscrates/evm/src/evm.rscrates/evm/src/lib.rscrates/node/Cargo.tomlcrates/node/src/add_ons.rscrates/node/src/components/pool.rscrates/node/src/node.rscrates/node/src/test_utils.rscrates/node/src/validator.rscrates/node/tests/it/engine.rscrates/node/tests/it/helpers.rscrates/node/tests/it/morph_tx.rscrates/node/tests/it/rpc.rscrates/payload/builder/src/builder.rscrates/payload/builder/src/config.rscrates/payload/builder/src/error.rscrates/payload/types/src/attributes.rscrates/payload/types/src/lib.rscrates/primitives/src/header.rscrates/primitives/src/receipt/mod.rscrates/primitives/src/transaction/envelope.rscrates/reference-index/src/tables.rscrates/revm/src/evm.rscrates/revm/src/handler.rscrates/revm/src/precompiles.rscrates/revm/src/token_fee.rscrates/revm/src/tx.rscrates/rpc/Cargo.tomlcrates/rpc/src/error.rscrates/rpc/src/eth/call.rscrates/rpc/src/eth/mod.rscrates/rpc/src/eth/receipt.rscrates/rpc/src/eth/transaction.rscrates/txpool/src/validator.rsdeny.tomletc/docker-compose.ymllocal-test/README.mdlocal-test/reth-start.sh
💤 Files with no reviewable changes (1)
- crates/engine-api/src/validator.rs
✅ Files skipped from review due to trivial changes (5)
- CONTRIBUTING.md
- etc/docker-compose.yml
- README.md
- crates/rpc/Cargo.toml
- crates/node/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/engine-tree-ext/src/lib.rs
- crates/payload/types/src/lib.rs
- crates/revm/src/tx.rs
- crates/evm/src/config.rs
- crates/node/src/test_utils.rs
- crates/primitives/src/receipt/mod.rs
- crates/node/tests/it/engine.rs
- crates/node/src/components/pool.rs
- crates/primitives/src/transaction/envelope.rs
- local-test/reth-start.sh
- crates/consensus/src/validation.rs
- crates/revm/src/evm.rs
- crates/txpool/src/validator.rs
- crates/payload/types/src/attributes.rs
- crates/node/tests/it/helpers.rs
- crates/rpc/src/eth/mod.rs
- crates/evm/src/block/mod.rs
- crates/engine-tree-ext/src/payload_validator.rs
Comment on lines
+563
to
+571
| let payload = node.advance_block().await?; | ||
|
|
||
| let tx_hash = *payload | ||
| .block() | ||
| .body() | ||
| .transactions | ||
| .first() | ||
| .expect("block should contain regression tx") | ||
| .tx_hash(); |
There was a problem hiding this comment.
Make tx selection deterministic before using .first().
Add a block transaction-count assertion before taking the first tx hash, so the test cannot silently validate the wrong transaction if harness behavior changes.
Proposed patch
node.rpc.inject_tx(raw_tx).await?;
let payload = node.advance_block().await?;
+ assert_eq!(
+ payload.block().body().transactions.len(),
+ 1,
+ "regression block should contain only the injected tx"
+ );
let tx_hash = *payload
.block()
.body()
.transactions📝 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 payload = node.advance_block().await?; | |
| let tx_hash = *payload | |
| .block() | |
| .body() | |
| .transactions | |
| .first() | |
| .expect("block should contain regression tx") | |
| .tx_hash(); | |
| let payload = node.advance_block().await?; | |
| assert_eq!( | |
| payload.block().body().transactions.len(), | |
| 1, | |
| "regression block should contain only the injected tx" | |
| ); | |
| let tx_hash = *payload | |
| .block() | |
| .body() | |
| .transactions | |
| .first() | |
| .expect("block should contain regression tx") | |
| .tx_hash(); |
🤖 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/morph_tx.rs` around lines 563 - 571, The test currently
grabs the first transaction via payload.block().body().transactions.first()
which can silently pick the wrong tx if harness behavior changes; make selection
deterministic by asserting the transaction count before taking the first
element: capture transactions into a local (e.g., let txs =
payload.block().body().transactions;), assert_eq!(txs.len(), 1, "expected
exactly one transaction in block for this regression test"), and then use
txs.first().tx_hash() to set tx_hash; adjust the expected count if the test
should allow more than one.
Comment on lines
+267
to
+277
| // Calculate intrinsic gas (same as normal transactions). If intrinsic gas | ||
| // > gas_limit, fall back to gas_limit (matching go-ethereum's behavior for | ||
| // L1 messages, which prepay gas on L1 and must always execute). | ||
| let initial_and_floor = validation::validate_initial_tx_gas( | ||
| tx, | ||
| spec, | ||
| disable_eip7623, | ||
| is_amsterdam_eip8037, | ||
| tx_gas_limit_cap, | ||
| ) | ||
| .unwrap_or_else(|_| InitialAndFloorGas::new(tx.gas_limit(), 0)); |
There was a problem hiding this comment.
Narrow the L1-message fallback to the intended intrinsic-gas error only.
The comment says this path should only relax the “intrinsic gas > gas_limit” case, but unwrap_or_else(|_| ...) swallows every validate_initial_tx_gas failure. That also bypasses unrelated checks and future upstream validations for L1 messages.
🤖 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/revm/src/handler.rs` around lines 267 - 277, The current
unwrap_or_else on validate_initial_tx_gas(...) swallows all validation errors;
instead, call validate_initial_tx_gas and pattern-match its Result so only the
specific "intrinsic gas > gas_limit" error is converted to
InitialAndFloorGas::new(tx.gas_limit(), 0), while any other Err is propagated
(or returned) unchanged. Reference validate_initial_tx_gas,
InitialAndFloorGas::new(tx.gas_limit(), 0) and the surrounding handler code so
you implement a match/if-let that maps only the intrinsic-gas-too-high variant
to the fallback and returns/propagates other errors.
Remove the custom local head cache and event listener so Morph Engine API continuity checks read reth's canonical provider state directly. Keep a dedicated block tag tracker for L1 safe/finalized hashes and extend the Engine API integration test to cover consecutive imports without waiting. Constraint: Preserve FCU safe/finalized tag caching for engine-tree cleanup and RPC tags Rejected: Local head tracker fast path | provider canonical state is the authority after FCU Confidence: medium Scope-risk: moderate Tested: cargo nextest run -p morph-node --features test-utils new_l2_block_imports_consecutive_assembled_blocks_over_rpc Not-tested: local-test mainnet sync scenario; full morph-engine-api suite was interrupted due long rebuild
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 (1)
crates/engine-api/src/builder.rs (1)
720-736:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential
finalized > safeinconsistency in FCU when only one L1 tag is set.
resolve_fcu_block_tag_hashis called independently for safe and finalized, so the tracker’s two slots are decoupled. If an externalset_block_tags(safe=A, finalized=ZERO)ever runs (the public API explicitly allows skipping zero hashes — see lines 502-505), then:
l1_safe_hash() = Some(A)(older L1 block),l1_finalized_hash() = None.- On a later import of a historical block (
now - block_timestamp > 60s):
safe_hash→Afinalized_hash→data.hash(the new head, withnumber > A.number)The FCU is then sent with
finalized.number > safe.number, violating the very invariant theset_block_tagsreordering on lines 506-514 is designed to preserve. This can occur in practice during catch-up when L1 finalization data isn’t yet available.A simple guard is to only apply the head fallback when both L1 tags are missing (or clamp finalized to safe in the asymmetric case):
🛡️ Proposed fix
- let finalized_hash = resolve_fcu_block_tag_hash( - self.block_tag_tracker.l1_finalized_hash(), - data.hash, - data.timestamp, - now_timestamp, - ); - let safe_hash = resolve_fcu_block_tag_hash( - self.block_tag_tracker.l1_safe_hash(), - data.hash, - data.timestamp, - now_timestamp, - ); + let l1_finalized = self.block_tag_tracker.l1_finalized_hash(); + let l1_safe = self.block_tag_tracker.l1_safe_hash(); + // Only synthesize from head when *both* L1 tags are absent, otherwise we + // can push an FCU with finalized > safe (e.g. safe set, finalized still + // pending from L1 finalization). + let (safe_hash, finalized_hash) = match (l1_safe, l1_finalized) { + (Some(s), Some(f)) => (s, f), + (None, None) => { + let h = resolve_fcu_block_tag_hash(None, data.hash, data.timestamp, now_timestamp); + (h, h) + } + // Partial L1 update: keep the known L1 hash for that tag and mirror + // it on the missing side so finalized <= safe holds. + (Some(s), None) => (s, s), + (None, Some(f)) => (f, f), + };🤖 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-api/src/builder.rs` around lines 720 - 736, The code independently calls resolve_fcu_block_tag_hash for finalized and safe using block_tag_tracker.l1_finalized_hash() and .l1_safe_hash(), which can produce finalized.number > safe.number when only one L1 tag is set; update the logic around computing finalized_hash and safe_hash (the calls to resolve_fcu_block_tag_hash and the subsequent alloy_rpc_types_engine::ForkchoiceState construction) to either only apply the head-fallback when BOTH l1_finalized_hash() and l1_safe_hash() are None, or clamp finalized_hash to safe_hash when finalized > safe (i.e., after computing both hashes, if finalized_hash.number > safe_hash.number then set finalized_hash = safe_hash) so the FCU invariant never permits finalized > safe.
🧹 Nitpick comments (2)
crates/engine-api/src/builder.rs (1)
506-514: 💤 Low valueConsider adding a unit test for the partial-update path of
record_block_tags.The new
test_block_tag_tracker_records_l1_block_tagsonly covers the both-Somecase. Given the explicit semantics thatNonepreserves the previous value (and the asymmetric ordering rationale documented above), a test assertingrecord_block_tags(Some(x), None)keeps the prior finalized hash (and vice-versa) would lock in the intended behavior and catch future regressions where someone “fixes” the conditional and accidentally clears tags.Also applies to: 1230-1240
🤖 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-api/src/builder.rs` around lines 506 - 514, Add unit tests for the partial-update paths of record_block_tags: extend or add tests alongside test_block_tag_tracker_records_l1_block_tags to cover record_block_tags(Some(x), None) and record_block_tags(None, Some(y)). For each case, initialize the tracker with both tags set, call record_block_tags with one None and one Some, and assert the None parameter does not clear the prior stored value while the Some parameter updates the value; include assertions for both hash and number semantics to lock in the intended behavior.crates/node/src/add_ons.rs (1)
37-51: 💤 Low valueNote: new
AuthHttpMiddlewaregeneric isn’t reachable through the trait impls.The struct now exposes
AuthHttpMiddleware = Identity, but the impls ofNodeAddOns,RethRpcAddOns, andEngineValidatorAddOnare written forMorphAddOns<N, EthB, PVB, EVB>(defaulting bothRpcMiddlewareandAuthHttpMiddlewaretoIdentity). So any consumer that customizes either middleware will silently lose the trait implementations.If that’s intentional for now (i.e., this PR just widens the inner
RpcAddOnsshape to match upstream), consider a short doc comment on the struct calling that out so future contributors aren’t surprised. Otherwise, threading the two middleware generics through the impls would make the wrapper actually usable with non-Identity middleware.Also applies to: 96-108
🤖 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/src/add_ons.rs` around lines 37 - 51, The new AuthHttpMiddleware generic on MorphAddOns is not propagated to the impls, so consumers that set custom RpcMiddleware or AuthHttpMiddleware lose the trait impls; update the impl blocks for NodeAddOns, RethRpcAddOns, and EngineValidatorAddOn to include the RpcMiddleware and AuthHttpMiddleware generics (matching the struct signature: MorphAddOns<N, EthB, PVB, EVB, RpcMiddleware, AuthHttpMiddleware>) and add the appropriate where bounds (e.g., RpcMiddleware: /* required trait */, AuthHttpMiddleware: /* required trait */) and pass them to the inner RpcAddOns type, or if intentional, add a short doc comment on MorphAddOns explaining that only Identity middleware is supported for now so future contributors are aware.
🤖 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 `@crates/engine-api/src/builder.rs`:
- Around line 720-736: The code independently calls resolve_fcu_block_tag_hash
for finalized and safe using block_tag_tracker.l1_finalized_hash() and
.l1_safe_hash(), which can produce finalized.number > safe.number when only one
L1 tag is set; update the logic around computing finalized_hash and safe_hash
(the calls to resolve_fcu_block_tag_hash and the subsequent
alloy_rpc_types_engine::ForkchoiceState construction) to either only apply the
head-fallback when BOTH l1_finalized_hash() and l1_safe_hash() are None, or
clamp finalized_hash to safe_hash when finalized > safe (i.e., after computing
both hashes, if finalized_hash.number > safe_hash.number then set finalized_hash
= safe_hash) so the FCU invariant never permits finalized > safe.
---
Nitpick comments:
In `@crates/engine-api/src/builder.rs`:
- Around line 506-514: Add unit tests for the partial-update paths of
record_block_tags: extend or add tests alongside
test_block_tag_tracker_records_l1_block_tags to cover record_block_tags(Some(x),
None) and record_block_tags(None, Some(y)). For each case, initialize the
tracker with both tags set, call record_block_tags with one None and one Some,
and assert the None parameter does not clear the prior stored value while the
Some parameter updates the value; include assertions for both hash and number
semantics to lock in the intended behavior.
In `@crates/node/src/add_ons.rs`:
- Around line 37-51: The new AuthHttpMiddleware generic on MorphAddOns is not
propagated to the impls, so consumers that set custom RpcMiddleware or
AuthHttpMiddleware lose the trait impls; update the impl blocks for NodeAddOns,
RethRpcAddOns, and EngineValidatorAddOn to include the RpcMiddleware and
AuthHttpMiddleware generics (matching the struct signature: MorphAddOns<N, EthB,
PVB, EVB, RpcMiddleware, AuthHttpMiddleware>) and add the appropriate where
bounds (e.g., RpcMiddleware: /* required trait */, AuthHttpMiddleware: /*
required trait */) and pass them to the inner RpcAddOns type, or if intentional,
add a short doc comment on MorphAddOns explaining that only Identity middleware
is supported for now so future contributors are aware.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04da6609-b7b0-48a5-9d7f-8e3ef678733e
📒 Files selected for processing (4)
crates/engine-api/src/builder.rscrates/engine-api/src/lib.rscrates/node/src/add_ons.rscrates/node/tests/it/engine.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/node/tests/it/engine.rs
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.
Summary
Unfork the execution-layer reth dependency from
morph-l2/rethto upstreamparadigmxyz/reth@v2.2.0. Morph-specific validator hooks now live in-tree instead of inside a reth fork.Pre-Jade blocks still skip state-root validation because Morph historical headers use ZK-trie roots, while post-Jade blocks enforce reth/MPT state-root validation. Amsterdam fork support is intentionally deferred to a separate PR; this PR keeps Morph below Amsterdam/EIP-8037 semantics.
What Changed
v2.2.0, revm to38.0.0, and alloy crates to2.0.4.slot_number,block_access_list_hash,block_timestamp, andGasOutputwithout activating unused Ethereum/Amsterdam semantics.eth_getBlockReceiptsand reth v2.2eth_subscribe("transactionReceipts")so Morph receipt metadata stays visible over RPC.Notes
transactionReceiptsis provided by reth v2.2 pubsub and uses the configuredMorphReceiptConverter.eth_sendRawTransactionSyncis provided by reth v2.2 through the defaultEthTransactionsimplementation.movePrecompileToAddress/ Amsterdam fork behavior is not implemented here and should be handled in a dedicated follow-up PR.Recent Commits
58b96ce5c1944606ad105Test Plan
cargo fmt --all -- --checkcargo test -p morph-chainspec hardforkcargo test -p morph-node --features test-utils --test it block_receipts_expose_morph_fields_over_rpcSummary by CodeRabbit
New Features
Improvements
Bug Fixes