chore: update reth 2.4#266
Conversation
Update reth to v2.3 and add e2e fork tests
📝 WalkthroughWalkthroughThe PR upgrades the Rust/reth dependency alignment, changes CI Cargo behavior, updates EIP-8037 batch gas accounting, and adds Amsterdam-era block access list propagation across payload building, execution, consensus validation, and Engine API tests. ChangesProtocol and CI alignment
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant EngineAPI
participant EvolvePayloadBuilder
participant PayloadService
participant ExecutionEnvelope
EngineAPI->>EvolvePayloadBuilder: request payload with fork attributes
EvolvePayloadBuilder->>PayloadService: return block and block access list
PayloadService->>ExecutionEnvelope: attach access list and construct payload
ExecutionEnvelope-->>EngineAPI: return fork-specific execution data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Mirror revm's failed top-level CREATE handling in the batch path: when a batch starting with CREATE reverts, return the intrinsic create_state_gas charged at tx entry to the reservoir. Also assert that payloads carrying EIP-7685 requests have a parent beacon block root so requests are not silently dropped from the sidecar. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/ev-revm/src/handler.rs`:
- Around line 309-312: Update the batch CREATE handling around the
FrameResult::Create outcome to treat a missing address (address == None),
including InstructionResult::Return, as a failed creation rather than committing
its checkpoint. Ensure this failure path performs the intrinsic state-gas
refill, and add a regression test covering a leading CREATE with no address
through the batch path.
In `@crates/node/src/payload_types.rs`:
- Around line 282-305: Enforce the requests and parent_beacon_block_root
invariant in release builds within the payload conversion flow containing the
current debug_assert! and sidecar construction. Replace the debug-only check and
silent map_or fallback with fallible validation that returns an appropriate
error when requests are present without a parent beacon block root; only
construct the v4 ExecutionPayloadSidecar when both values exist.
In `@crates/tests/src/test_evolve_engine_api.rs`:
- Around line 233-245: Update the test flow around
EngineApiClient::<EvolveEngineTypes>::new_payload_v4 so a successful RPC
response with a non-Valid PayloadStatusEnum is returned or converted into an
error instead of triggering the assert! panic. Preserve the existing
Valid-status assertion behavior while allowing the surrounding error handling to
observe the expected Amsterdam rejection.
🪄 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: 47b31fb8-5999-4664-8552-66e196afaa7c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/e2e.yml.github/workflows/ev_deployer.yml.github/workflows/integration.yml.github/workflows/lint.yml.github/workflows/unit.ymlCargo.tomlcrates/ev-revm/src/factory.rscrates/ev-revm/src/handler.rscrates/evolve/src/consensus.rscrates/node/Cargo.tomlcrates/node/src/attributes.rscrates/node/src/builder.rscrates/node/src/config.rscrates/node/src/evm_executor.rscrates/node/src/node.rscrates/node/src/payload_service.rscrates/node/src/payload_types.rscrates/tests/src/common.rscrates/tests/src/e2e_tests.rscrates/tests/src/test_evolve_engine_api.rs
| reservoir, | ||
| total_state_gas_spent, | ||
| 0, | ||
| create_state_gas_refill, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C5 \
'FrameResult::Create|address\.is_none\(\)|create_failed|InstructionResult::Return' \
crates Cargo.lockRepository: evstack/ev-reth
Length of output: 1744
🏁 Script executed:
#!/bin/bash
set -eu
# Map the file and inspect the relevant sections around the reported lines and test helpers.
ast-grep outline crates/ev-revm/src/handler.rs --view expanded || true
printf '\n--- handler.rs around the reported lines ---\n'
sed -n '260,340p' crates/ev-revm/src/handler.rs
printf '\n--- handler.rs test helpers and CREATE-related tests ---\n'
sed -n '1600,1725p' crates/ev-revm/src/handler.rsRepository: evstack/ev-reth
Length of output: 9738
🏁 Script executed:
#!/bin/bash
set -eu
# Look for any other CREATE outcome handling in this file and related frame semantics in the workspace.
rg -n -C4 'FrameResult::Create|CreateOutcome|address\.is_none\(\)|InstructionResult::Return|create_state_gas_refill|finalize_batch_gas' crates/ev-revm crates -g '!target'Repository: evstack/ev-reth
Length of output: 15302
🏁 Script executed:
#!/bin/bash
set -eu
# Check upstream-style frame semantics references in vendored docs/comments if present in repo.
rg -n -C4 'address.*None|failed.*CREATE|FrameResult::Create|CreateOutcome' .Repository: evstack/ev-reth
Length of output: 8742
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the exact test added around finalize_batch_gas and whether it exercises a real batch path.
sed -n '1660,1715p' crates/ev-revm/src/handler.rsRepository: evstack/ev-reth
Length of output: 1088
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the batch gas finalization logic around the reported lines.
sed -n '300,340p' crates/ev-revm/src/handler.rs
printf '\n--- references to finalization helpers ---\n'
rg -n -C6 'fn finalize_batch_gas|create_state_gas_refill|total_state_gas_spent|reservoir' crates/ev-revm/src/handler.rsRepository: evstack/ev-reth
Length of output: 10195
🌐 Web query:
revm CreateOutcome address None failed Return semantics FrameResult::Create docs.rs
💡 Result:
In the revm ecosystem, a CreateOutcome with an address of None indicates that a contract creation operation has failed or was unable to produce a new contract address [1][2]. The CreateOutcome struct contains an InstructionResult (representing the execution outcome, such as success, revert, or halt) and an Option<Address> [3][2]. When the address field is None, it signifies that no new contract was deployed [3][2]. This state commonly occurs in several scenarios: 1. Early-fail paths: Before full execution, certain conditions—such as nonce overflows, insufficient balance, or exceeding the call stack depth—result in a failure where address is set to None [1]. 2. Reverts or Halts: If the creation operation is executed but ultimately fails (e.g., due to a revert or halt instruction), the resulting CreateOutcome will reflect this failure status [1]. 3. EIP-8037 Handling: In newer implementations, the handler uses the presence of None in the address field to distinguish failed creations, specifically to manage gas refunds (e.g., refunding upfront create_state_gas) when a child frame fails to deploy a contract [1]. The FrameResult::Create variant wraps this CreateOutcome to propagate the result of the create operation back through the interpreter's frame-handling logic [4][5]. When processing this result, systems check outcome.address.is_none() alongside the InstructionResult to determine if the creation was unsuccessful and to apply necessary state updates or gas adjustments [1].
Citations:
- 1: https://docs.rs/revm-handler/latest/src/revm_handler/frame.rs.html
- 2: https://docs.rs/revm-interpreter/latest/revm_interpreter/interpreter_action/struct.CreateOutcome.html
- 3: https://docs.rs/revm/latest/revm/interpreter/struct.CreateOutcome.html
- 4: https://docs.rs/revm/latest/revm/handler/enum.FrameResult.html
- 5: https://docs.rs/revm-handler/latest/revm_handler/enum.FrameResult.html
Treat missing CREATE addresses as failures. A leading CREATE can still return InstructionResult::Return with address == None; this path would commit the checkpoint and skip the intrinsic state-gas refill. Inspect the FrameResult::Create outcome and add a regression test that exercises the batch path.
🤖 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/ev-revm/src/handler.rs` around lines 309 - 312, Update the batch
CREATE handling around the FrameResult::Create outcome to treat a missing
address (address == None), including InstructionResult::Return, as a failed
creation rather than committing its checkpoint. Ensure this failure path
performs the intrinsic state-gas refill, and add a regression test covering a
leading CREATE with no address through the batch path.
| // Requests imply a post-Prague payload, which always carries a parent | ||
| // beacon block root; without one the requests cannot be attached to | ||
| // the sidecar and would be silently dropped. | ||
| debug_assert!( | ||
| requests.is_none() || block.header.parent_beacon_block_root.is_some(), | ||
| "payload has EIP-7685 requests but no parent beacon block root" | ||
| ); | ||
| let sidecar = if let Some(requests) = requests { | ||
| block | ||
| .header | ||
| .parent_beacon_block_root | ||
| .map_or(sidecar, |parent_beacon_block_root| { | ||
| ExecutionPayloadSidecar::v4( | ||
| CancunPayloadFields { | ||
| parent_beacon_block_root, | ||
| versioned_hashes: block | ||
| .body | ||
| .blob_versioned_hashes_iter() | ||
| .copied() | ||
| .collect(), | ||
| }, | ||
| PraguePayloadFields::new(requests), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce the request/root invariant in release builds.
debug_assert! is removed in release mode. If requests is present without parent_beacon_block_root, map_or retains the original sidecar and silently drops the requests. Enforce this through a fallible conversion or payload-construction validation.
🤖 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/payload_types.rs` around lines 282 - 305, Enforce the
requests and parent_beacon_block_root invariant in release builds within the
payload conversion flow containing the current debug_assert! and sidecar
construction. Replace the debug-only check and silent map_or fallback with
fallible validation that returns an appropriate error when requests are present
without a parent beacon block root; only construct the v4
ExecutionPayloadSidecar when both values exist.
| let new_payload_status = EngineApiClient::<EvolveEngineTypes>::new_payload_v4( | ||
| &engine_client, | ||
| execution_payload.clone(), | ||
| vec![], | ||
| B256::ZERO, | ||
| RequestsOrHash::default(), | ||
| ) | ||
| .await?; | ||
| assert!( | ||
| matches!(new_payload_status.status, PayloadStatusEnum::Valid), | ||
| "expected pre-Amsterdam ev-node payload to be valid, got {:?}", | ||
| new_payload_status.status | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return non-Valid payload statuses instead of panicking.
A successful RPC response with Invalid status triggers this assertion, so the outer let Err(error) cannot observe the expected Amsterdam rejection. Convert non-Valid statuses into an error or return the status to the caller.
Proposed fix
- assert!(
- matches!(new_payload_status.status, PayloadStatusEnum::Valid),
- "expected pre-Amsterdam ev-node payload to be valid, got {:?}",
- new_payload_status.status
- );
+ if !matches!(new_payload_status.status, PayloadStatusEnum::Valid) {
+ eyre::bail!(
+ "Invalid payload attributes/status: {:?}",
+ new_payload_status.status
+ );
+ }📝 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.
| let new_payload_status = EngineApiClient::<EvolveEngineTypes>::new_payload_v4( | |
| &engine_client, | |
| execution_payload.clone(), | |
| vec![], | |
| B256::ZERO, | |
| RequestsOrHash::default(), | |
| ) | |
| .await?; | |
| assert!( | |
| matches!(new_payload_status.status, PayloadStatusEnum::Valid), | |
| "expected pre-Amsterdam ev-node payload to be valid, got {:?}", | |
| new_payload_status.status | |
| ); | |
| let new_payload_status = EngineApiClient::<EvolveEngineTypes>::new_payload_v4( | |
| &engine_client, | |
| execution_payload.clone(), | |
| vec![], | |
| B256::ZERO, | |
| RequestsOrHash::default(), | |
| ) | |
| .await?; | |
| if !matches!(new_payload_status.status, PayloadStatusEnum::Valid) { | |
| eyre::bail!( | |
| "Invalid payload attributes/status: {:?}", | |
| new_payload_status.status | |
| ); | |
| } |
🤖 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/tests/src/test_evolve_engine_api.rs` around lines 233 - 245, Update
the test flow around EngineApiClient::<EvolveEngineTypes>::new_payload_v4 so a
successful RPC response with a non-Valid PayloadStatusEnum is returned or
converted into an error instead of triggering the assert! panic. Preserve the
existing Valid-status assertion behavior while allowing the surrounding error
handling to observe the expected Amsterdam rejection.
1 participant
Description
Update reth to v2.3 and add e2e fork tests
e2e tests wont pass until ev-node is merged
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Maintenance