Skip to content

Conversation

@lakshya-sky
Copy link
Contributor

Motivation

Description

Checklist

  • Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

Closes #issue_number

lakshya-sky and others added 30 commits December 22, 2025 21:04
…ambdaclass#5706)

**Motivation**

* `./hive --sim ethereum/eels/execute-blobs --client ethrex` fails with
`execution_testing.rpc.rpc_types.JSONRPCError: JSONRPCError(code=-32603,
message=Internal Error: Parent block not found)`.
* GetBlobBaseFee is using parents header which causes to get blob base
fee for genesis blocks. According to the specs `excess_blob_gas` is
**Running total of blob gas consumed in excess of the target, prior to
this block.**. That means we need to use latest block header only.

**Description**
- use latest block header and remove accessing parent header.

Co-authored-by: lakshya-sky <lakshya-sky@users.noreply.github.com>
Co-authored-by: Edgar <git@edgl.dev>
Copilot AI review requested due to automatic review settings January 8, 2026 17:45
@lakshya-sky lakshya-sky marked this pull request as draft January 8, 2026 17:45
@lakshya-sky lakshya-sky changed the title feature(l1): add BlockAccessListTracer and debug_getBlockAccessList. feat(l1): add BlockAccessListTracer and debug_getBlockAccessList. Jan 8, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request adds support for block access list tracing and the Amsterdam fork. The key changes include:

  • Introduction of a flexible Tracer trait with multiple implementations (BlockAccessListTracer, LevmCallTracer, NoOpTracer)
  • New debug_getBlockAccessList RPC endpoint for retrieving block access lists
  • Addition of the Amsterdam fork with block_access_list_hash in block headers
  • New engine_newPayloadV5 RPC method supporting block access lists
  • Refactoring of VM tracer integration from concrete LevmCallTracer to trait object Rc<RefCell<dyn Tracer>>

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
crates/vm/levm/src/tracing/mod.rs Defines the new Tracer trait with hooks for state changes and account accesses
crates/vm/levm/src/tracing/block_access_list_tracer.rs Implements tracer that tracks all state changes for generating block access lists
crates/vm/levm/src/tracing/call_tracer.rs Refactors LevmCallTracer to implement the Tracer trait
crates/vm/levm/src/vm.rs Updates VM to use trait object for tracer, adds new call() method
crates/vm/levm/src/opcode_handlers/*.rs Adds tracer notifications throughout opcode handlers
crates/vm/levm/src/db/gen_db.rs Integrates tracer hooks into database operations
crates/common/types/block_access_list.rs Defines data structures for block access lists with RLP encoding/decoding
crates/networking/rpc/debug/block_access_list.rs Implements RPC handler for debug_getBlockAccessList
crates/networking/rpc/engine/payload.rs Adds engine_newPayloadV5 handler with block access list validation
crates/common/types/genesis.rs Adds Amsterdam fork configuration and ordering of fork-checking methods
crates/vm/backends/mod.rs Adds generate_bal flag to control block access list generation
crates/blockchain/tracing.rs Implements trace_block_access_list method for blockchain

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +123 to +126
if self.idx == 0 {
self.idx = self.idx.saturating_add(1);
}
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tracer index initialization in txn_start is inconsistent. When idx is 0, it increments to 1, but then txn_end always increments. This means the first transaction will have idx=1, but if there are no transactions in a block, idx stays at 0. Consider starting at idx=1 in the constructor or documenting why idx=0 should be skipped.

Copilot uses AI. Check for mistakes.
Comment on lines 432 to +483
let acc = self.get_account_mut(address)?;
let old_hash = acc.info.code_hash;
let code_hash = new_bytecode.hash;
acc.info.code_hash = new_bytecode.hash;
self.db.codes.entry(code_hash).or_insert(new_bytecode);
self.db
.codes
.entry(code_hash)
.or_insert(new_bytecode.clone());
let old = self.db.get_code(old_hash).cloned()?;
self.tracer
.borrow_mut()
.on_code_change(address, old, new_bytecode, self.db);
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential issue: on_code_change is called in update_account_bytecode, but there's no corresponding on_account_access call for the address before accessing/modifying the account. This is inconsistent with other methods like increase_account_balance and decrease_account_balance which call on_account_access first.

Copilot uses AI. Check for mistakes.

#[derive(Default, Debug, Serialize, Deserialize, Clone, PartialEq)]
pub struct CodeChange {
#[serde(rename = "TxIdx")]
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent serialization naming: CodeChange uses TxIdx (line 139) while other similar structs like NonceChange (line 106), BalanceChange (line 73), and StorageChange (line 14) use txIndex. This should be standardized to txIndex for consistency.

Suggested change
#[serde(rename = "TxIdx")]
#[serde(rename = "txIndex")]

Copilot uses AI. Check for mistakes.
Comment on lines +130 to +136
// // Obtain the block's parent state
// let mut vm = self
// .rebuild_parent_state(block.header.parent_hash, reexec)
// .await?;
// // Run anything necessary before executing the block's transactions (system calls, etc)
// vm.rerun_block(&block, Some(0))?;

Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code: There are commented-out lines that reference rebuild_parent_state and rerun_block. If this code is intended for future use, it should be documented with a TODO comment explaining why it's commented out. Otherwise, it should be removed to improve code clarity.

Suggested change
// // Obtain the block's parent state
// let mut vm = self
// .rebuild_parent_state(block.header.parent_hash, reexec)
// .await?;
// // Run anything necessary before executing the block's transactions (system calls, etc)
// vm.rerun_block(&block, Some(0))?;

Copilot uses AI. Check for mistakes.
Fork::BPO3 => SpecId::OSAKA,
Fork::BPO4 => SpecId::OSAKA,
Fork::BPO5 => SpecId::OSAKA,
Fork::Amsterdam => todo!(),
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing implementation: The Fork::Amsterdam mapping to SpecId is left as todo!(). This will cause a panic at runtime if Amsterdam fork tests are executed. The appropriate SpecId should be mapped or at a minimum this should return a proper error rather than panicking.

Copilot uses AI. Check for mistakes.
let chain_config = context.storage.get_chain_config();

if !chain_config.is_amsterdam_activated(block.header.timestamp) {
return Err(RpcErr::UnsuportedFork(format!(
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in error message: "UnsuportedFork" should be "UnsupportedFork" (missing 'p').

Suggested change
return Err(RpcErr::UnsuportedFork(format!(
return Err(RpcErr::UnsupportedFork(format!(

Copilot uses AI. Check for mistakes.
Comment on lines +636 to +637
// This method follows the same specification as `engine_newPayloadV4` additionally
// rejects payload without block access list
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment in validate_execution_payload_v4 mentions "This method follows the same specification as engine_newPayloadV4" but the function is actually validating payload requirements for V5 (Amsterdam fork with block_access_list). The comment should clarify that this validates V4/V5 payloads, or the function should be renamed to better reflect its purpose.

Suggested change
// This method follows the same specification as `engine_newPayloadV4` additionally
// rejects payload without block access list
// This method builds on the `engine_newPayloadV3` specification and additionally
// enforces the Amsterdam (V5) requirement that `block_access_list` is present,
// i.e. it validates payloads suitable for Engine API V4/V5.

Copilot uses AI. Check for mistakes.
Comment on lines 488 to 503
pub fn increment_account_nonce(&mut self, address: Address) -> Result<u64, InternalError> {
let account = self.get_account_mut(address)?;
let old = account.info.nonce;
account.info.nonce = account
.info
.nonce
.checked_add(1)
.ok_or(InternalError::Overflow)?;
Ok(account.info.nonce)
let new = account.info.nonce;
if old != new {
self.tracer
.borrow_mut()
.on_nonce_change(address, old, new, self.db);
}
Ok(new)
}
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing tracer notification: increment_account_nonce doesn't call on_account_access before accessing the account, which is inconsistent with other account modification methods like increase_account_balance and decrease_account_balance.

Copilot uses AI. Check for mistakes.
fn txn_end(
&mut self,
_gas_used: u64,
err: Option<String>,
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The err parameter in txn_end is not prefixed with underscore but is unused. This should either be renamed to _err for consistency with other unused parameters, or the error should be handled (e.g., to roll back changes on transaction failure).

Suggested change
err: Option<String>,
_err: Option<String>,

Copilot uses AI. Check for mistakes.
Comment on lines +125 to +127
// if self.generate_bal {
// tracer = Rc::new(RefCell::new());
// }
Copy link

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code: Commented-out code that suggests generating a BlockAccessList tracer based on self.generate_bal. This should either be implemented properly or removed. The current implementation always uses NoOpTracer which means generate_bal has no effect in this method.

Suggested change
// if self.generate_bal {
// tracer = Rc::new(RefCell::new());
// }

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant