feat(l1): move getBlockAccessList from debug to eth namespace#6709
feat(l1): move getBlockAccessList from debug to eth namespace#6709edg-l wants to merge 2 commits into
Conversation
execution-apis defines this method under `eth_` (src/eth/block.yaml), not `debug_`. Move the dispatch and module accordingly and return the structured JSON shape from the spec instead of an RLP-encoded hex string. - Rename RPC method `debug_getBlockAccessList` -> `eth_getBlockAccessList`. - Move `rpc/debug/block_access_list.rs` -> `rpc/eth/block_access_list.rs`. - Return the spec-defined JSON (array of AccountAccess with camelCase fields, hash32/quantity hex encodings per execution-apis schema). - Read from the existing BAL store before re-executing the block. - Return `null` (notFound) on unknown blocks instead of an Internal error. - Update REPL command listing. No back-compat alias for the old `debug_` name.
|
🤖 Kimi Code ReviewThe PR moves Code Quality & Correctness
Minor Observations
Security & Safety
Conclusion The PR correctly implements the EIP-7928 specification with efficient caching and proper JSON-RPC formatting. The code is idiomatic Rust and follows the project's patterns. No changes required. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Codex Code ReviewFindings
I did not run a full build: Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR moves
Confidence Score: 3/5The response serialisation and routing changes are correct, but the parameter parser rejects plain 32-byte block hash strings, which the test plan explicitly exercises. The parameter parsing uses BlockIdentifier::parse, which calls u64::from_str_radix on the hex payload. A 64-character block hash overflows u64 and returns BadHexFormat instead of a result. The PR's own test plan includes 'call against an unknown block hash; expect null (not an error)', which would fail against the current implementation. All other mechanical changes — dispatch wiring, module moves, REPL catalog updates — are correct. crates/networking/rpc/eth/block_access_list.rs — parameter parsing and storage-change value encoding both need a second look.
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/eth/block_access_list.rs | New handler for eth_getBlockAccessList with fast-path (BAL store) + slow-path (block re-execution); has a parameter-parsing bug that rejects plain 32-byte block hash inputs and a potential format mismatch for storage-change values. |
| crates/networking/rpc/debug/block_access_list.rs | Deleted file — old debug_getBlockAccessList handler removed. No issues. |
| crates/networking/rpc/rpc.rs | Routes eth_getBlockAccessList in map_eth_requests and removes debug_getBlockAccessList from map_debug_requests. Clean dispatch change. |
| tooling/repl/src/commands/eth.rs | Adds eth_getBlockAccessList to the REPL command catalog. Straightforward addition. |
| tooling/repl/src/commands/debug.rs | Removes debug_getBlockAccessList from the REPL command catalog. Straightforward deletion. |
Sequence Diagram
sequenceDiagram
participant Client
participant RPC as map_eth_requests
participant Handler as BlockAccessListRequest
participant Storage
participant Blockchain
Client->>RPC: eth_getBlockAccessList(param)
RPC->>Handler: call(req, context)
Handler->>Storage: resolve_block_number(block_id)
alt block not found
Storage-->>Handler: None
Handler-->>Client: null
end
Handler->>Storage: get_block_header(block_number)
alt header not found
Storage-->>Handler: None
Handler-->>Client: null
end
Handler->>Storage: get_block_access_list(block_hash)
alt fast path: BAL in store
Storage-->>Handler: Some(bal)
Handler-->>Client: JSON array of AccountAccess
else slow path: re-execute
Handler->>Storage: get_block_by_hash(block_hash)
Storage-->>Handler: Some(block)
Handler->>Blockchain: generate_bal_for_block(block)
alt post-Amsterdam block
Blockchain-->>Handler: Some(bal)
Handler-->>Client: JSON array of AccountAccess
else pre-Amsterdam block
Blockchain-->>Handler: None
Handler-->>Client: null
end
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
crates/networking/rpc/eth/block_access_list.rs:18
**Block hash inputs silently rejected**
`BlockIdentifier::parse` attempts `u64::from_str_radix` on the hex payload. Any 32-byte (64-char) block hash string overflows `u64` and returns `RpcErr::BadHexFormat(0)` — so a caller doing `eth_getBlockAccessList("0xd7b7fa3c...")` gets a "Bad hex format" error instead of a result.
The third test in the PR's test plan reads "call against an unknown block hash; expect `null` (not an error)", which implies the spec parameter is a plain `hash32`. Using `BlockIdentifierOrHash::parse` (which supports EIP-1898 `{"blockHash":"0x..."}`) still won't fix a *bare* hash string; a dedicated hash-first parser (or switching to `BlockIdentifierOrHash` that also accepts bare 32-byte strings) is needed to match that test expectation.
### Issue 2 of 2
crates/networking/rpc/eth/block_access_list.rs:73-76
**`storageChanges.value` format inconsistency with other change types**
`post_value` (a `U256` storage slot value) is serialised with 32-byte zero-padding (`"0x{:064x}"`), while `post_balance` and `post_nonce` use quantity encoding (`"{:#x}"`). EIP-7928 defines `StorageValue = uint256`, which in Ethereum JSON-RPC is a `uint` (no leading zeros), consistent with how balance and nonce are encoded. If the execution-apis YAML defines `value` here as `uint`, clients performing strict schema validation will reject the padded form.
```suggestion
json!({
"index": format!("{:#x}", c.block_access_index),
"value": format!("{:#x}", c.post_value),
})
```
Reviews (1): Last reviewed commit: "Merge branch 'main' into rpc/move-block-..." | Re-trigger Greptile
🤖 Claude Code ReviewI have enough information to write a thorough review. Here it is: Code Review: PR #6709 —
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if params.is_empty() { | ||
| return Err(RpcErr::BadParams("Expected 1 param".to_owned())); | ||
| } | ||
| let block_id = BlockIdentifier::parse(params[0].clone(), 0)?; |
There was a problem hiding this comment.
BlockIdentifier::parse attempts u64::from_str_radix on the hex payload. Any 32-byte (64-char) block hash string overflows u64 and returns RpcErr::BadHexFormat(0) — so a caller doing eth_getBlockAccessList("0xd7b7fa3c...") gets a "Bad hex format" error instead of a result.
The third test in the PR's test plan reads "call against an unknown block hash; expect null (not an error)", which implies the spec parameter is a plain hash32. Using BlockIdentifierOrHash::parse (which supports EIP-1898 {"blockHash":"0x..."}) still won't fix a bare hash string; a dedicated hash-first parser (or switching to BlockIdentifierOrHash that also accepts bare 32-byte strings) is needed to match that test expectation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/block_access_list.rs
Line: 18
Comment:
**Block hash inputs silently rejected**
`BlockIdentifier::parse` attempts `u64::from_str_radix` on the hex payload. Any 32-byte (64-char) block hash string overflows `u64` and returns `RpcErr::BadHexFormat(0)` — so a caller doing `eth_getBlockAccessList("0xd7b7fa3c...")` gets a "Bad hex format" error instead of a result.
The third test in the PR's test plan reads "call against an unknown block hash; expect `null` (not an error)", which implies the spec parameter is a plain `hash32`. Using `BlockIdentifierOrHash::parse` (which supports EIP-1898 `{"blockHash":"0x..."}`) still won't fix a *bare* hash string; a dedicated hash-first parser (or switching to `BlockIdentifierOrHash` that also accepts bare 32-byte strings) is needed to match that test expectation.
How can I resolve this? If you propose a fix, please make it concise.| json!({ | ||
| "index": format!("{:#x}", c.block_access_index), | ||
| "value": format!("0x{:064x}", c.post_value), | ||
| }) |
There was a problem hiding this comment.
storageChanges.value format inconsistency with other change types
post_value (a U256 storage slot value) is serialised with 32-byte zero-padding ("0x{:064x}"), while post_balance and post_nonce use quantity encoding ("{:#x}"). EIP-7928 defines StorageValue = uint256, which in Ethereum JSON-RPC is a uint (no leading zeros), consistent with how balance and nonce are encoded. If the execution-apis YAML defines value here as uint, clients performing strict schema validation will reject the padded form.
| json!({ | |
| "index": format!("{:#x}", c.block_access_index), | |
| "value": format!("0x{:064x}", c.post_value), | |
| }) | |
| json!({ | |
| "index": format!("{:#x}", c.block_access_index), | |
| "value": format!("{:#x}", c.post_value), | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/block_access_list.rs
Line: 73-76
Comment:
**`storageChanges.value` format inconsistency with other change types**
`post_value` (a `U256` storage slot value) is serialised with 32-byte zero-padding (`"0x{:064x}"`), while `post_balance` and `post_nonce` use quantity encoding (`"{:#x}"`). EIP-7928 defines `StorageValue = uint256`, which in Ethereum JSON-RPC is a `uint` (no leading zeros), consistent with how balance and nonce are encoded. If the execution-apis YAML defines `value` here as `uint`, clients performing strict schema validation will reject the padded form.
```suggestion
json!({
"index": format!("{:#x}", c.block_access_index),
"value": format!("{:#x}", c.post_value),
})
```
How can I resolve this? If you propose a fix, please make it concise.
| /// camelCase fields and per-spec hex encodings (hash32 = full 32-byte hex, | ||
| /// quantities = no-leading-zero hex). | ||
| fn bal_to_json(bal: &BlockAccessList) -> Value { | ||
| Value::Array(bal.accounts().iter().map(account_to_json).collect()) |
There was a problem hiding this comment.
Worth a unit test against the BlockAccessList → JSON conversion. The whole point of this PR is wire-shape conformance to execution-apis, and bal_to_json is where any drift would land. A small test would catch every format mistake before deploy:
#[test]
fn bal_to_json_matches_spec_shape() {
use ethrex_common::types::block_access_list::{AccountChanges, BalanceChange, BlockAccessList, NonceChange, SlotChange, StorageChange};
let mut acc = AccountChanges::new(Address::from_low_u64_be(0x42));
acc.balance_changes.push(BalanceChange::new(1, U256::from(1_000)));
acc.nonce_changes.push(NonceChange::new(2, 7));
acc.storage_reads.push(U256::from(5));
let mut sc = SlotChange::new(U256::from(3));
sc.slot_changes.push(StorageChange::new(4, U256::from(0xdead)));
acc.storage_changes.push(sc);
let bal = BlockAccessList::new(vec![acc]);
let j = bal_to_json(&bal);
assert_eq!(j[0]["address"], "0x0000000000000000000000000000000000000042");
assert_eq!(j[0]["balanceChanges"][0], json!({"index": "0x1", "value": "0x3e8"}));
assert_eq!(j[0]["nonceChanges"][0], json!({"index": "0x2", "value": "0x7"}));
assert_eq!(j[0]["storageReads"][0], "0x0000000000000000000000000000000000000000000000000000000000000005");
assert_eq!(j[0]["storageChanges"][0]["key"], "0x0000000000000000000000000000000000000000000000000000000000000003");
assert_eq!(j[0]["storageChanges"][0]["changes"][0]["value"], "0x000000000000000000000000000000000000000000000000000000000000dead");
}This is the cheap version of every checkbox in the PR test plan. Re-runs forever and doesn't need a devnet.
| let bal = context | ||
| .blockchain | ||
| .generate_bal_for_block(&block) | ||
| .map_err(|e| RpcErr::Internal(format!("Failed to generate BAL: {e}")))?; |
There was a problem hiding this comment.
nit: spec asymmetry. generate_bal_for_block returning Ok(None) is treated as Value::Null (the match bal arm below); returning Err becomes RpcErr::Internal. The boundary between the two is generate_bal_for_block's implementation detail — pre-Amsterdam is Ok(None), but "post-Amsterdam block whose state is missing from disk" might be either depending on how the function fails.
If the intent is "known block, can't generate → also null" (which matches the spirit of notFound), this branch could swallow more error variants. If the intent is "known block, real error → Internal", a one-line doc-comment here explaining the split would save the next reader from having to chase down generate_bal_for_block. Non-blocking; just calling out the boundary.
| let Some(header) = context.storage.get_block_header(block_number)? else { | ||
| return Ok(Value::Null); | ||
| }; | ||
| let block_hash = header.hash(); |
There was a problem hiding this comment.
nit on the hash round-trip. For a BlockIdentifier::Hash(h) input, this code does resolve_block_number(h) → get_block_header(number) → header.hash() — three lookups to reproduce the hash the caller already gave us. For BlockIdentifier::Number/Tag it's the natural path.
Not worth restructuring just for this method, but if BlockIdentifier ever grows a resolve_block_hash(&self, storage) -> Result<Option<H256>> helper that short-circuits the Hash variant, this handler (and any others doing the same dance) gets cheaper for free. Filing under "future-work" rather than "change now."
| GetTransactionByBlockHashAndIndexRequest::call(req, context).await | ||
| } | ||
| "eth_getBlockReceipts" => GetBlockReceiptsRequest::call(req, context).await, | ||
| "eth_getBlockAccessList" => BlockAccessListRequest::call(req, context).await, |
There was a problem hiding this comment.
Worth a one-line CHANGELOG.md entry for the breaking RPC rename. The PR is correct in not adding a debug_getBlockAccessList alias (the body explains why), but any external tooling that integrated against the debug_ name will fail with MethodNotFound after upgrade. A "Breaking: debug_getBlockAccessList renamed to eth_getBlockAccessList (now spec-conformant per execution-apis EIP-7928)" line would surface this for downstream integrators reading release notes.
2 participants
Summary
execution-apis defines
getBlockAccessListunder theeth_namespace, notdebug_. This PR aligns ethrex with the spec.debug_getBlockAccessList→eth_getBlockAccessList.rpc/debug/block_access_list.rs→rpc/eth/block_access_list.rs; dispatch moves frommap_debug_requeststomap_eth_requests.BlockAccessList(array ofAccountAccesswith camelCase fields,hash32/quantity hex per spec) instead of an RLP-encoded hex string.storage.get_block_access_list(hash)(populated at block import inblockchain.rs:1936) before falling back togenerate_bal_for_block.null(notFoundschema) instead of an Internal RPC error.No back-compat alias for the old
debug_name.Spec references
execution-apis/src/eth/block.yaml:245execution-apis/src/schemas/block-access-list.yamlPerf notes
The previous handler always re-executed the full block, which is O(block-execution-time) — hundreds of ms to several seconds per call for a full block. The new fast-path is an RLP decode of the stored BAL when the block was imported in this process lifetime (the common case for post-Amsterdam tip lookups since
add_blockwrites the BAL on import).The slow re-execute path still applies for:
blockchain.add_blockpath, which does not currently callstore_block_access_list(separate follow-up).A small LRU on top is a possible future optimization but kept out of scope here.
Test plan
cargo check -p ethrex-rpc -p ethrex-replcargo clippy -p ethrex-rpc -p ethrex-repl --no-depscargo fmt --all -- --checketh_getBlockAccessListagainst a post-Amsterdam block on a devnet and check JSON shape matches the spec example.null.null(not an error).