Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpgrades workspace dependencies (Reth → v2.1.0, Alloy → v2.0.0, revm bumps) and implements Amsterdam slot_number propagation, separates state vs total gas with per-batch reservoir propagation, refactors handler/executor gas validation and accounting, and changes precompile error handling to distinguish halting vs fatal outputs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Evm as EVM/Frame
participant StateDB
participant Finalizer as finalize_batch_gas
Client->>Handler: submit batch (txs, attributes, initial gas)
Handler->>Handler: compute initial_total_gas + initial_state_gas (reservoir)
Handler->>Evm: execute tx1 (pass reservoir, state gas tracking)
Evm->>StateDB: apply state changes (accumulate state gas)
Evm-->>Handler: tx result (tx gas used, state gas used, reservoir delta)
Handler->>Evm: execute next tx... (loop)
Handler->>Finalizer: finalize_batch_gas(total_gas, state_gas_spent, reservoir)
Finalizer->>StateDB: commit or revert state based on outcome
Finalizer-->>Client: return BlockExecutionResult (gas split, reservoir)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/ev-revm/src/handler.rs (1)
358-372:⚠️ Potential issue | 🟠 MajorRefund reservoir gas in the sponsored reimbursement path.
finalize_batch_gasnow stores unused reservoir onGas, but sponsored transactions still reimburse using onlyremaining + refunded. Any sponsored EIP-8037 transaction with unused reservoir can overcharge the sponsor unless this path includesgas.reservoir()too.Proposed fix
- let reimbursement = U256::from( - effective_gas_price - .saturating_mul((gas.remaining() + gas.refunded() as u64) as u128), - ); + let refundable_gas = gas + .remaining() + .saturating_add(gas.refunded() as u64) + .saturating_add(gas.reservoir()); + let reimbursement = + U256::from(effective_gas_price.saturating_mul(refundable_gas as u128));Also applies to: 595-615
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/ev-revm/src/handler.rs` around lines 358 - 372, In reimburse_caller (ev-revm::handler::reimburse_caller) update the sponsored reimbursement calculation to include the unused reservoir: when computing reimbursement use gas.remaining() + gas.refunded() as u64 + gas.reservoir() (or equivalent reservoir() amount converted to u64/appropriate type) before multiplying by effective_gas_price, so the reimbursement equals effective_gas_price * (remaining + refunded + reservoir); apply the same change in the other sponsored path referenced (the block around lines 595-615) to avoid overcharging sponsors.crates/node/src/executor.rs (1)
157-175:⚠️ Potential issue | 🟠 MajorUse the header slot when constructing
BlockEnvfor existing blocks.The new context paths pass
slot_number, butevm_env(&Header)still forcesslot_num: 0. Re-executing or validating a post-Amsterdam block from its header can therefore run slot-dependent logic with the wrong slot.Proposed fix
- slot_num: 0, // EL client — CL slot tracking not applicable + slot_num: header.slot_number.unwrap_or_default(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/executor.rs` around lines 157 - 175, The BlockEnv constructed in evm_env(&Header) sets slot_num: 0 which causes slot-dependent logic to run incorrectly for existing post-Amsterdam blocks; change the slot_num assignment to use the header's slot value (e.g., slot_num: header.slot) so BlockEnv reflects the actual block slot when building from a Header in the evm_env function.
🧹 Nitpick comments (2)
crates/tests/src/e2e_tests.rs (1)
120-131: Add a positiveslot_numberEngine API path test.
build_block_with_transactionsnow hard-codesslot_number: None, so these e2e tests still do not exercise the new non-empty slot propagation path. Consider parameterizing this helper or adding a dedicated test withSome(slot)and asserting the resulting payload/header carries it. Based on learnings, Implement unit tests for individual components and integration tests incrates/tests/covering Engine API interactions, payload building with transactions, state execution validation, and Evolve-specific scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tests/src/e2e_tests.rs` around lines 120 - 131, The helper build_block_with_transactions currently hard-codes slot_number: None in EvolveEnginePayloadAttributes, so update the helper signature (build_block_with_transactions) to accept an optional slot parameter (e.g., Option<Slot> or Option<u64>) and pass that into EvolveEnginePayloadAttributes.slot_number instead of None; then add an E2E test in crates/tests/src/e2e_tests.rs that calls build_block_with_transactions(Some(slot_value)), builds the payload/header, and asserts the resulting payload/header (the EvolveEnginePayloadAttributes and produced block header) contains the same slot_number value to exercise the non-empty slot propagation path.crates/node/src/payload_service.rs (1)
418-425: Add one positiveslot_number: Some(...)regression test.The updated tests only construct
slot_number: None, so a future regression dropping non-empty slot propagation would still pass. A small test that builds attributes withslot_number: Some(123)and verifies the built header/payload carries it would cover the new feature directly. Based on learnings, implement unit tests and integration tests covering Engine API interactions and payload building.Also applies to: 515-522, 602-609
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/node/src/payload_service.rs` around lines 418 - 425, Add a positive regression test that constructs EvolveEnginePayloadAttributes with RpcPayloadAttributes.slot_number: Some(123) (instead of None) and asserts that the resulting header/payload built by the payload builder/serialize path (the functions that consume EvolveEnginePayloadAttributes and produce the engine header/payload in payload_service.rs) preserves that slot number; do the same in the other two test sites referenced (around the other diffs) so you don't only test None, and add an integration test exercising the Engine API payload build path to verify propagation end-to-end (construct attributes with Some(123), call the payload-building function, and assert the produced header/payload.slot_number == Some(123)).
🤖 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/node/src/attributes.rs`:
- Around line 84-87: The code currently hardcodes slot_number to Some(0) when
chain_spec.is_amsterdam_active_at_timestamp(timestamp) is true; change this so
you do not emit a constant 0 — instead derive the slot from the parent header if
available or leave slot_number unset (None) when the builder has no
authoritative slot source. Locate the slot_number assignment (using symbols
slot_number, chain_spec, is_amsterdam_active_at_timestamp, timestamp) and
replace the then_some(0) behavior with logic that reads the parent header's slot
(e.g., from self.parent_header or equivalent) and wraps that in Some(),
otherwise return None.
In `@crates/node/src/evm_executor.rs`:
- Around line 167-179: The admission check uses self.block_regular_gas_used for
Amsterdam but must use the true final block gas (max of regular and state);
replace the branch that sets block_gas_used when
self.evm.cfg_env().enable_amsterdam_eip8037 to call and use
self.max_block_gas_used() instead of self.block_regular_gas_used so
block_available_gas = self.evm.block().gas_limit() - block_gas_used correctly
reflects Amsterdam semantics (references:
self.evm.cfg_env().enable_amsterdam_eip8037, self.block_regular_gas_used,
self.max_block_gas_used(), block_available_gas).
In `@crates/node/src/payload_types.rs`:
- Line 3: ExecutionPayloadV4 is being populated with
header.block_access_list_hash (32-byte hash) and EMPTY_BLOCK_ACCESS_LIST_HASH,
but the Engine API requires the encoded access list itself: replace uses of
header.block_access_list_hash and EMPTY_BLOCK_ACCESS_LIST_HASH when setting
ExecutionPayloadV4.block_access_list with the actual encoded access list (an
empty Vec for no entries) instead of hashes; also stop defaulting missing
slot_number to 0—use the header's slot field (or propagate an error/Option)
rather than silently using 0 to avoid masking construction errors in
post-Amsterdam headers (look for ExecutionPayloadV4 construction and variables
named block_access_list, header.block_access_list_hash,
EMPTY_BLOCK_ACCESS_LIST_HASH, and slot_number).
---
Outside diff comments:
In `@crates/ev-revm/src/handler.rs`:
- Around line 358-372: In reimburse_caller (ev-revm::handler::reimburse_caller)
update the sponsored reimbursement calculation to include the unused reservoir:
when computing reimbursement use gas.remaining() + gas.refunded() as u64 +
gas.reservoir() (or equivalent reservoir() amount converted to u64/appropriate
type) before multiplying by effective_gas_price, so the reimbursement equals
effective_gas_price * (remaining + refunded + reservoir); apply the same change
in the other sponsored path referenced (the block around lines 595-615) to avoid
overcharging sponsors.
In `@crates/node/src/executor.rs`:
- Around line 157-175: The BlockEnv constructed in evm_env(&Header) sets
slot_num: 0 which causes slot-dependent logic to run incorrectly for existing
post-Amsterdam blocks; change the slot_num assignment to use the header's slot
value (e.g., slot_num: header.slot) so BlockEnv reflects the actual block slot
when building from a Header in the evm_env function.
---
Nitpick comments:
In `@crates/node/src/payload_service.rs`:
- Around line 418-425: Add a positive regression test that constructs
EvolveEnginePayloadAttributes with RpcPayloadAttributes.slot_number: Some(123)
(instead of None) and asserts that the resulting header/payload built by the
payload builder/serialize path (the functions that consume
EvolveEnginePayloadAttributes and produce the engine header/payload in
payload_service.rs) preserves that slot number; do the same in the other two
test sites referenced (around the other diffs) so you don't only test None, and
add an integration test exercising the Engine API payload build path to verify
propagation end-to-end (construct attributes with Some(123), call the
payload-building function, and assert the produced header/payload.slot_number ==
Some(123)).
In `@crates/tests/src/e2e_tests.rs`:
- Around line 120-131: The helper build_block_with_transactions currently
hard-codes slot_number: None in EvolveEnginePayloadAttributes, so update the
helper signature (build_block_with_transactions) to accept an optional slot
parameter (e.g., Option<Slot> or Option<u64>) and pass that into
EvolveEnginePayloadAttributes.slot_number instead of None; then add an E2E test
in crates/tests/src/e2e_tests.rs that calls
build_block_with_transactions(Some(slot_value)), builds the payload/header, and
asserts the resulting payload/header (the EvolveEnginePayloadAttributes and
produced block header) contains the same slot_number value to exercise the
non-empty slot propagation path.
🪄 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: 8814c093-39af-4c80-8294-11e88acc6324
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlcrates/ev-precompiles/src/mint.rscrates/ev-primitives/Cargo.tomlcrates/ev-revm/src/evm.rscrates/ev-revm/src/factory.rscrates/ev-revm/src/handler.rscrates/evolve/src/types.rscrates/node/src/attributes.rscrates/node/src/builder.rscrates/node/src/evm_executor.rscrates/node/src/executor.rscrates/node/src/payload_service.rscrates/node/src/payload_types.rscrates/node/src/validator.rscrates/tests/src/e2e_tests.rscrates/tests/src/test_evolve_engine_api.rs
| slot_number: self | ||
| .chain_spec | ||
| .is_amsterdam_active_at_timestamp(timestamp) | ||
| .then_some(0), |
There was a problem hiding this comment.
Avoid hard-coding every Amsterdam local payload to slot 0.
When Amsterdam is active, this builder emits Some(0) for every locally-built payload, which can create repeated/incorrect slot values in block headers and EVM context. Derive it from the parent header or leave it unset unless the builder has a real slot source.
Possible direction
- slot_number: self
- .chain_spec
- .is_amsterdam_active_at_timestamp(timestamp)
- .then_some(0),
+ slot_number: self
+ .chain_spec
+ .is_amsterdam_active_at_timestamp(timestamp)
+ .then(|| parent.slot_number.unwrap_or_default().saturating_add(1)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/attributes.rs` around lines 84 - 87, The code currently
hardcodes slot_number to Some(0) when
chain_spec.is_amsterdam_active_at_timestamp(timestamp) is true; change this so
you do not emit a constant 0 — instead derive the slot from the parent header if
available or leave slot_number unset (None) when the builder has no
authoritative slot source. Locate the slot_number assignment (using symbols
slot_number, chain_spec, is_amsterdam_active_at_timestamp, timestamp) and
replace the then_some(0) behavior with logic that reads the parent header's slot
(e.g., from self.parent_header or equivalent) and wraps that in Some(),
otherwise return None.
| let block_gas_used = if self.evm.cfg_env().enable_amsterdam_eip8037 { | ||
| self.block_regular_gas_used | ||
| } else { | ||
| self.cumulative_tx_gas_used | ||
| }; | ||
| let block_available_gas = self.evm.block().gas_limit() - block_gas_used; | ||
|
|
||
| if tx.tx().gas_limit() > block_available_gas { | ||
| let mut max_tx_gas_usage = tx.tx().gas_limit(); | ||
| if let Some(tx_gas_limit_cap) = self.evm.cfg_env().tx_gas_limit_cap { | ||
| max_tx_gas_usage = min(max_tx_gas_usage, tx_gas_limit_cap); | ||
| } | ||
|
|
||
| if max_tx_gas_usage > block_available_gas { |
There was a problem hiding this comment.
Use max_block_gas_used() when checking Amsterdam block capacity.
The final block gas for Amsterdam is max(regular, state), but admission only subtracts block_regular_gas_used. A block whose state gas already exceeds the remaining limit can still accept more transactions, then finish with gas_used > gas_limit.
Proposed fix
let block_gas_used = if self.evm.cfg_env().enable_amsterdam_eip8037 {
- self.block_regular_gas_used
+ self.max_block_gas_used()
} else {
self.cumulative_tx_gas_used
};
- let block_available_gas = self.evm.block().gas_limit() - block_gas_used;
+ let block_available_gas = self.evm.block().gas_limit().saturating_sub(block_gas_used);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/evm_executor.rs` around lines 167 - 179, The admission check
uses self.block_regular_gas_used for Amsterdam but must use the true final block
gas (max of regular and state); replace the branch that sets block_gas_used when
self.evm.cfg_env().enable_amsterdam_eip8037 to call and use
self.max_block_gas_used() instead of self.block_regular_gas_used so
block_available_gas = self.evm.block().gas_limit() - block_gas_used correctly
reflects Amsterdam semantics (references:
self.evm.cfg_env().enable_amsterdam_eip8037, self.block_regular_gas_used,
self.max_block_gas_used(), block_available_gas).
| use std::sync::Arc; | ||
|
|
||
| use alloy_eips::eip7685::Requests; | ||
| use alloy_eips::{eip7685::Requests, eip7928::EMPTY_BLOCK_ACCESS_LIST_HASH}; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For alloy-rpc-types-engine ExecutionPayloadV4 / ExecutionPayloadEnvelopeV6, is the block_access_list field expected to contain the encoded block access list or the header's block_access_list_hash? What value is required for an empty block access list?
💡 Result:
For alloy-rpc-types-engine ExecutionPayloadV4 and ExecutionPayloadEnvelopeV6, the block_access_list field is expected to contain the encoded block access list itself, not the header's block_access_list_hash. This is evidenced by the ExecutionPayloadBodyV2 struct added for EIP-7928, which explicitly includes a block_access_list field alongside transactions and withdrawals, indicating the full list is included in payload bodies. ExecutionPayloadEnvelopeV6, as the return type for engine_getPayloadV6, likely wraps ExecutionPayloadV4 and related body data containing this field. The distinction from the header's hash field (present in block headers for Merkle proof verification) confirms the payload carries the encoded list. For an empty block access list, use an empty list/array: [] (empty Vec in Rust), as standard for access lists when none are present.
Citations:
- 1: feat(rpc-types-engine): add ExecutionPayloadBodyV2 for EIP-7928 alloy-rs/alloy#3636
- 2: https://docs.rs/alloy/latest/alloy/rpc/types/engine/index.html
- 3: https://docs.rs/alloy-rpc-types-engine/latest/alloy_rpc_types_engine/all.html
🏁 Script executed:
#!/bin/bash
# Description: Find all local constructions/usages of block_access_list and block_access_list_hash.
# Expected: V6 payload construction should use the field according to Alloy's ExecutionPayloadV4 semantics.
rg -n -C3 'ExecutionPayloadV4|block_access_list|block_access_list_hash'Repository: evstack/ev-reth
Length of output: 2542
Fix: block_access_list must contain the encoded access list, not the hash.
At lines 229–240, ExecutionPayloadV4's block_access_list field is being populated with header.block_access_list_hash (a 32-byte hash). According to the Engine API contract, this field should contain the encoded block access list itself. For an empty access list, use an empty Vec instead of EMPTY_BLOCK_ACCESS_LIST_HASH.
Additionally, defaulting a missing slot_number to 0 at line 234 can mask construction errors in post-Amsterdam headers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/node/src/payload_types.rs` at line 3, ExecutionPayloadV4 is being
populated with header.block_access_list_hash (32-byte hash) and
EMPTY_BLOCK_ACCESS_LIST_HASH, but the Engine API requires the encoded access
list itself: replace uses of header.block_access_list_hash and
EMPTY_BLOCK_ACCESS_LIST_HASH when setting ExecutionPayloadV4.block_access_list
with the actual encoded access list (an empty Vec for no entries) instead of
hashes; also stop defaulting missing slot_number to 0—use the header's slot
field (or propagate an error/Option) rather than silently using 0 to avoid
masking construction errors in post-Amsterdam headers (look for
ExecutionPayloadV4 construction and variables named block_access_list,
header.block_access_list_hash, EMPTY_BLOCK_ACCESS_LIST_HASH, and slot_number).
Co-authored-by: Randy Grok <98407738+randygrok@users.noreply.github.com>
2 participants
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Refactor
Tests