feat: unfork reth to paradigmxyz/reth v2.0.0 with retroactive state-root trust#98
Draft
feat: unfork reth to paradigmxyz/reth v2.0.0 with retroactive state-root trust#98
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)
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine as MorphBasicEngineValidator
participant Consensus
participant EVM
participant StateRootCompute as StateRootCompute<br/>(Sparse/Parallel/Sync)
participant PostExec as PostExecution<br/>Validator
participant Trie as TrieUpdates<br/>Task
Client->>Engine: validate_block_with_state(block/payload)
Engine->>Engine: convert_to_block()
Engine->>Consensus: validate header & pre-exec
alt Consensus OK
Consensus-->>Engine: ✓
Engine->>EVM: execute_transactions(evm_env)
EVM-->>Engine: result + state_changes
Engine->>Engine: compute receipts (stream)
par State Root Computation
Engine->>StateRootCompute: compute_state_root(sparse/parallel/sync)
StateRootCompute-->>Engine: state_root (or fallback)
and Trie Updates Task
Engine->>Trie: spawn deferred trie sort/merge
end
Engine->>Engine: check state_root_enforced_at(timestamp)
alt Jade Active
Engine->>Engine: strict MPT equality check
else Pre-Jade
Engine->>Engine: skip state-root validation
end
Engine->>PostExec: validate post-execution
PostExec-->>Engine: ✓
Engine->>Client: VALID
else Consensus Error
Consensus-->>Client: INVALID
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
Comment on lines
+236
to
+237
| // reth_primitives_traits::SignedTransaction has a blanket impl in v0.1.0; no explicit impl needed. | ||
| // RlpBincode trait was removed in reth v2.0.0; no impl needed. |
There was a problem hiding this comment.
Avoid the misleading version reference in this comment.
This PR targets reth v2.0.0, so v0.1.0 reads stale even if the blanket impl statement is correct. Consider making the comment version-agnostic.
Suggested wording
-// reth_primitives_traits::SignedTransaction has a blanket impl in v0.1.0; no explicit impl needed.
+// reth_primitives_traits::SignedTransaction has a blanket impl; no explicit impl needed.
// RlpBincode trait was removed in reth v2.0.0; no impl needed.📝 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
| // reth_primitives_traits::SignedTransaction has a blanket impl in v0.1.0; no explicit impl needed. | |
| // RlpBincode trait was removed in reth v2.0.0; no impl needed. | |
| // reth_primitives_traits::SignedTransaction has a blanket impl; no explicit impl needed. | |
| // RlpBincode trait was removed in reth v2.0.0; no impl needed. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/primitives/src/transaction/envelope.rs` around lines 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.
Comment on lines
+518
to
+524
| // FIXME(morph-unfork): several tests below are #[cfg(any())]-disabled pending | ||
| // migration from MockEthProvider to a MorphPrimitives-aware mock provider | ||
| // (reth v2.0.0 tightened the Provider::BlockHeader == EvmConfig::BlockHeader bound). | ||
| // The shared imports and helpers remain used by those tests so silence dead-code | ||
| // lints until the migration lands. | ||
| #![allow(dead_code, unused_imports)] | ||
|
|
There was a problem hiding this comment.
Disabled tests leave a gap in unit coverage for L1-message/EIP-1559/MorphTx admission paths — track re-enable.
Several admission tests are now gated behind #[cfg(any())] pending MockEthProvider → MorphPrimitives-aware mock migration. The comments are clear and scoped, and e2e coverage (77/77) plus the retroactive-trust Hoodi sync substantially mitigates regression risk, so this is not a blocker. Consider creating a tracking issue so the FIXME-migration doesn't drift.
Want me to open a tracking issue for migrating these to a MorphPrimitives-aware mock provider?
Also applies to: 612-615, 661-661, 717-717, 771-771
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/txpool/src/validator.rs` around lines 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Unfork the execution-layer reth dependency from
morph-l2/reth@1b07025to upstreamparadigmxyz/reth@v2.0.0. All Morph-specific validator hooks now live in a new in-tree crate rather than in a fork.Pre-Jade blocks still skip state-root validation (ZK-trie era); post-Jade blocks enforce full MPT state-root. The switch is driven by a single gate helper inspected at block import time.
Why
Architectural highlights
crates/engine-tree-ext/(new)payload_validator.rs—MorphBasicEngineValidator, a near-verbatim copy of reth v2.0.0BasicEngineValidatorwith one injection point for Morph's retroactive-trust gate. Annotated withNOTE(morph)on every deviation.gate.rs—state_root_enforced_at(timestamp)helper. Returnsfalsepre-Jade (skips validation to tolerate historical ZK-trie blocks) andtruepost-Jade.trie_updates.rs— trie-update helpers for the MPT path.tests/jade_boundary.rs— 2 e2e tests (pre-Jade tampered state-root accepted, post-Jade tampered state-root rejected).revm layer
crates/revm/src/evm.rs— SLOAD (0x54) and SSTORE (0x55) opcode overrides that restoreoriginal_valueto the DB-committed value on cold→warm transitions. Fixes a +2800 gas divergence triggered by MorphTx V1 +mark_cold()on mainnet block 2,205,224 (also reproduced on Hoodi).6031236/ reimburse-save work.c61633fdefensively so that later SSTOREs inside the same tx don't re-corruptoriginal_valuevia revm's cold-path write.payload-builder perf refactors (v2.0.0 API adoption)
PayloadExecutionCache— wire cross-block state reuse (Phase 2.1).StateRootHandle+OnStateHook— run state-root computation in parallel with execution, streaming updates into a background task (Phase 2.2/2.3).BuildNextEnvtrait implemented forMorphNextBlockEnvAttributes(Phase 3.1).Local-box openloop benchmark (M4 Pro, 200k target, 6 runs × 120s):
New 2σ range does not overlap baseline 2σ range.
DB-contention fix (local-test only)
Last commit adds
--engine.persistence-threshold 256,--engine.memory-block-buffer-target 16,--engine.persistence-backpressure-threshold 512tolocal-test/reth-start.sh. Batches MDBX writes so they don't contend with morphnode's LevelDB fsyncs when CL and EL co-locate on the same host — doubled local mainnet sync speed from ~42 to ~84 blocks/s. Does not apply to production deployments where morphnode and morph-reth run on separate machines.Commits (15)
927f23d5e0f043a6656e80fb52cf2427e9b7782afbac7629fb1049a4146aa8653907ae75f8408430c372ff511f44c588a5c72fba2Test plan
make fmt— cleanmake clippy— clean with-D warningsmake clippy-e2e— clean withtest-utilsfeaturecargo test --doc --all— all doc tests passmake test— 608 unit/lib/bin tests passmake test-e2e— 77/77 integration tests pass (incl. 2 new Jade-boundary tests)Follow-ups (not in this PR)
bin/bench-block-exec,local-test/bench-contracts,run_full_benchmark.sh) was reused fromfeat/max-tps-benchmarkto produce the perf numbers, but intentionally excluded from this PR to keep it focused.Summary by CodeRabbit
Release Notes
New Features
Improvements
Chores