feat(l1): default omitted block to latest on eth state methods#6780
feat(l1): default omitted block to latest on eth state methods#6780MysticRyuujin wants to merge 8 commits into
Conversation
…orageValues The state methods (eth_getBalance, eth_getCode, eth_getStorageAt, eth_getTransactionCount, eth_getProof) required an exact param count, so omitting the block returned -32000 'Expected N params'. Make the block parameter optional and default to latest (per execution-apis), via a new Default for BlockIdentifierOrHash (latest). Also implement eth_getStorageValues (previously -32601 method not found): batch storage-slot reads for multiple accounts at a block, mirroring eth_getStorageAt, with the same optional-block default and a 1024-slot cap.
…s parse Parse-layer tests proving eth_getBalance/getCode/getStorageAt/ getTransactionCount/getProof resolve an omitted block to latest, and that eth_getStorageValues parses a request (and rejects an empty one).
MysticRyuujin
changed the title
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR aligns several state-related RPC methods with execution-apis by making the block parameter optional (defaulting to latest) and adds a new eth_getStorageValues RPC for batch storage reads.
Changes:
- Add
DefaultforBlockIdentifierOrHashto represent the omitted-block default (latest). - Update state RPC request parsing to accept an omitted block parameter and default it to
latest. - Introduce
eth_getStorageValuesrequest/handler with slot-count limiting and tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/networking/rpc/types/block_identifier.rs | Adds Default for BlockIdentifierOrHash to support omitted block params defaulting to latest. |
| crates/networking/rpc/rpc.rs | Wires new eth_getStorageValues method into the request router and imports. |
| crates/networking/rpc/eth/account.rs | Makes block param optional for state methods; adds GetStorageValuesRequest implementation and tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let raw: HashMap<String, Vec<U256>> = serde_json::from_value(params[0].clone())?; | ||
| let mut requests = HashMap::with_capacity(raw.len()); | ||
| let mut total_slots = 0usize; | ||
| for (address, slots) in raw { | ||
| total_slots += slots.len(); | ||
| if total_slots > MAX_STORAGE_VALUES_SLOTS { | ||
| return Err(RpcErr::BadParams(format!( | ||
| "too many slots (max {MAX_STORAGE_VALUES_SLOTS})" | ||
| ))); | ||
| } | ||
| let address: Address = serde_json::from_value(Value::String(address))?; | ||
| let slots = slots.iter().map(H256::from_uint).collect(); | ||
| requests.insert(address, slots); | ||
| } |
| let Some(block_number) = self.block.resolve_block_number(&context.storage).await? else { | ||
| return Err(RpcErr::Internal( | ||
| "Could not resolve block number".to_owned(), | ||
| )); | ||
| }; |
Greptile SummaryThis PR aligns ethrex with execution-apis#812 by making the block parameter optional (defaulting to
Confidence Score: 4/5The core optional-block-default logic is correct and well-tested; the new eth_getStorageValues handler works end-to-end but has two surface-area rough edges around slot format parity and response address key casing. The Default chain and all five amended parse methods are straightforward and consistent. The two open questions — decimal slot values and checksummed address key matching — are usability concerns that don't break existing callers but may trip up new consumers of eth_getStorageValues. crates/networking/rpc/eth/account.rs — specifically the GetStorageValuesRequest parse and handle implementations
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/eth/account.rs | Makes block parameter optional (defaulting to latest) for 5 state methods and adds new eth_getStorageValues handler; slot parsing and response address key format may diverge from eth_getStorageAt conventions |
| crates/networking/rpc/types/block_identifier.rs | Adds Default impl for BlockIdentifierOrHash (delegates to BlockIdentifier::default() → BlockTag::Latest); correct and safe |
| crates/networking/rpc/rpc.rs | Registers eth_getStorageValues in the eth dispatch table; minimal one-line change, straightforward |
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/account.rs:300
**Slot deserialization may reject decimal values accepted by `eth_getStorageAt`**
`eth_getStorageAt` uses `serde_utils::u256::deser_hex_or_dec_str` for the storage slot, which accepts both `"0x1"` (hex) and `"1"` (decimal). Here `slots` are deserialized via `serde_json::from_value::<Vec<U256>>`, which relies on `U256`'s built-in serde. If a client sends decimal-formatted slot indices (e.g. `{"0xaddr": ["1", "2"]}`), this call will fail while the equivalent `eth_getStorageAt(addr, "1", ...)` would succeed — a silent behavioural asymmetry between the two methods.
### Issue 2 of 2
crates/networking/rpc/eth/account.rs:348
**Response address keys use lowercase hex — callers sending checksummed or mixed-case addresses can't do a direct key lookup**
`format!("{address:#x}")` renders the address as lowercase `0x`-prefixed hex. A client that sends `{"0xDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEf": ["0x0"]}` receives a response keyed on `"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"`. Any code that naively looks up its original request-address string against the response map will miss the entry. Returning the address in EIP-55 checksummed form (or normalising to lowercase at parse time and documenting it) would make the correlation unambiguous.
Reviews (1): Last reviewed commit: "test: state methods default omitted bloc..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return Err(RpcErr::BadParams("Expected 1 or 2 params".to_owned())); | ||
| }; | ||
| // params[0] is an object mapping address -> array of storage slots. | ||
| let raw: HashMap<String, Vec<U256>> = serde_json::from_value(params[0].clone())?; |
There was a problem hiding this comment.
eth_getStorageAt
eth_getStorageAt uses serde_utils::u256::deser_hex_or_dec_str for the storage slot, which accepts both "0x1" (hex) and "1" (decimal). Here slots are deserialized via serde_json::from_value::<Vec<U256>>, which relies on U256's built-in serde. If a client sends decimal-formatted slot indices (e.g. {"0xaddr": ["1", "2"]}), this call will fail while the equivalent eth_getStorageAt(addr, "1", ...) would succeed — a silent behavioural asymmetry between the two methods.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/account.rs
Line: 300
Comment:
**Slot deserialization may reject decimal values accepted by `eth_getStorageAt`**
`eth_getStorageAt` uses `serde_utils::u256::deser_hex_or_dec_str` for the storage slot, which accepts both `"0x1"` (hex) and `"1"` (decimal). Here `slots` are deserialized via `serde_json::from_value::<Vec<U256>>`, which relies on `U256`'s built-in serde. If a client sends decimal-formatted slot indices (e.g. `{"0xaddr": ["1", "2"]}`), this call will fail while the equivalent `eth_getStorageAt(addr, "1", ...)` would succeed — a silent behavioural asymmetry between the two methods.
How can I resolve this? If you propose a fix, please make it concise.| .unwrap_or_default(); | ||
| values.push(format!("{:#x}", H256::from_uint(&value))); | ||
| } | ||
| result.insert(format!("{address:#x}"), values); |
There was a problem hiding this comment.
format!("{address:#x}") renders the address as lowercase 0x-prefixed hex. A client that sends {"0xDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEf": ["0x0"]} receives a response keyed on "0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef". Any code that naively looks up its original request-address string against the response map will miss the entry. Returning the address in EIP-55 checksummed form (or normalising to lowercase at parse time and documenting it) would make the correlation unambiguous.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/account.rs
Line: 348
Comment:
**Response address keys use lowercase hex — callers sending checksummed or mixed-case addresses can't do a direct key lookup**
`format!("{address:#x}")` renders the address as lowercase `0x`-prefixed hex. A client that sends `{"0xDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEfDeAdBeEf": ["0x0"]}` receives a response keyed on `"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"`. Any code that naively looks up its original request-address string against the response map will miss the entry. Returning the address in EIP-55 checksummed form (or normalising to lowercase at parse time and documenting it) would make the correlation unambiguous.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Thanks Copilot — reviewed both:
|
|
Thanks Greptile — both are accurate observations; my take on each:
|
| let mut total_slots = 0usize; | ||
| for (address, slots) in raw { | ||
| total_slots += slots.len(); | ||
| if total_slots > MAX_STORAGE_VALUES_SLOTS { |
There was a problem hiding this comment.
The MAX_STORAGE_VALUES_SLOTS = 1024 cap is the security boundary on this endpoint — it's the only thing stopping a single request from asking for arbitrary-many slot reads. Worth a unit test that asserts the cap fires:
#[test]
fn test_get_storage_values_request_rejects_over_cap() {
let mut slots = Vec::with_capacity(MAX_STORAGE_VALUES_SLOTS + 1);
for i in 0..=MAX_STORAGE_VALUES_SLOTS {
slots.push(format!("{i:#x}"));
}
let params = Some(vec![json!({
"0xdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef": slots
})]);
let err = GetStorageValuesRequest::parse(¶ms).unwrap_err();
assert!(matches!(err, RpcErr::BadParams(ref m) if m.contains("too many slots")));
}And a complementary one for the multi-account-totaling case (e.g. 600 slots on each of two accounts → over cap) since the cap is total-across-accounts, not per-account. The current parse test only exercises the happy path + the empty-request rejection; the cap is the un-tested security control. Non-blocking.
| .get(1) | ||
| .map(|b| BlockIdentifierOrHash::parse(b.clone(), 1)) | ||
| .transpose()? | ||
| .unwrap_or_default(), |
There was a problem hiding this comment.
The optional-block-defaults-to-latest pattern is now duplicated 6 times in this file:
params
.get(N)
.map(|b| BlockIdentifierOrHash::parse(b.clone(), N))
.transpose()?
.unwrap_or_default()…with only the index N varying. A small helper would centralize the spec-aligned behavior (and the next state method that lands in this file gets it for free):
/// Parse an optional block parameter at `index`, defaulting to `latest`
/// when omitted (per execution-apis: Block parameter is optional).
fn parse_optional_block(params: &[Value], index: usize) -> Result<BlockIdentifierOrHash, RpcErr> {
params
.get(index)
.map(|b| BlockIdentifierOrHash::parse(b.clone(), index))
.transpose()
.map(Option::unwrap_or_default)
}Non-blocking — the duplication is mechanical and reads fine inline.
There was a problem hiding this comment.
Do you want me to add a helper? I can, up to you.
There was a problem hiding this comment.
You can leave inline, your call but the duplication is local enough that a helper isn't load-bearing.
|
Added the requested |
…tech#21586) ## Summary Make the `Block` parameter optional (defaulting to `latest` when omitted) on the six state-reading methods: - `eth_getBalance` - `eth_getCode` - `eth_getStorageAt` - `eth_getTransactionCount` - `eth_getProof` - `eth_getStorageValues` Each took a value-type `rpc.BlockNumberOrHash`, so omitting the block parameter failed with `-32602 missing value for required argument N` — Erigon's `rpc` package (like go-ethereum's) only permits omitting a *trailing* argument when it is a pointer. This changes the six handlers (and the `EthAPI` interface) to `*rpc.BlockNumberOrHash` and defaults `nil` to `latest` via a small `orLatest` helper, reusing the existing `latestNumOrHash` var. `eth_call` already behaves this way. Internal callers of the interface are updated accordingly (`rpc/contracts/direct_backend.go`, `rpc/mcp/*`) along with the affected tests. ## Motivation This aligns Erigon with **ethereum/execution-apis#812** by **@manusw7**, which marks the `Block` parameter `required: false` (default `latest`) on these methods. Full credit for the proposal and rationale to @manusw7. ## Testing - `go vet ./rpc/jsonrpc/` passes (handlers + updated tests compile). - Verified against the `hive` `rpc-compat` simulator with default-block conformance fixtures (block parameter omitted): a locally-built Erigon with this change returns the latest-block values and all six pass (`tests=7 failed=0`). Before the change, all six failed. ## Related - Source proposal: ethereum/execution-apis#812 (by @manusw7) - execution-apis spec + conformance tests: ethereum/execution-apis#814 - go-ethereum implementation: ethereum/go-ethereum#35100 - Nethermind `eth_getStorageValues` fix: NethermindEth/nethermind#11883 - Besu implementation: besu-eth/besu#10587 - ethrex implementation: lambdaclass/ethrex#6780 --------- Co-authored-by: lupin012 <58134934+lupin012@users.noreply.github.com>
|
If there's anything left to do with this PR please let me know, we plan to merge the execution-apis PR soon, this behavior is merged in all other EL clients. |
…est-state-methods # Conflicts: # crates/networking/rpc/eth/account.rs
|
bump |
Summary
Two related JSON-RPC changes aligning ethrex with ethereum/execution-apis#812 by @manusw7 (Block parameter optional, default
latest):lateston the state methods that previously required it.eth_getBalance,eth_getCode,eth_getStorageAt,eth_getTransactionCount, andeth_getProofdid a strictparams.len() != Ncheck and returned-32000 "Expected N params"when the block was omitted. They now accept the block as optional and default it tolatestvia a newimpl Default for BlockIdentifierOrHash(latest).eth_getStorageValues(previously-32601 method not found): batches storage-slot reads for multiple accounts at a block, mirroringeth_getStorageAt, with the same optional-block default and a 1024-slot cap. Registered in theethdispatch.Motivation
Aligns ethrex with execution-apis#812 (@manusw7) and matches the other clients implementing the same change. Full credit to @manusw7 for the proposal.
Testing
cargo test -p ethrex-rpc --lib— 64 passed, 0 failed. New parse-layer tests assert each state method resolves an omitted block tolatest(test_state_methods_default_block_to_latest_when_omitted) and thateth_getStorageValuesparses (and rejects an empty request).rpc-compatsimulator that the omitted-block request now produces the same response as an explicit"latest"request for every method (previously the omitted form errored).Note on hive rpc-compat value fixtures (out of scope)
ethrex does not currently pass the rpc-compat value fixtures for these state methods in the hive default chain — but this is pre-existing and unrelated to this change: a stock ethrex build returns the same results for an explicit
"latest"request (e.g.eth_getBalance(addr, "latest")returns0x0where other clients return the funded balance), and*-blockhashrequests error with "Could not resolve block number". That points to an ethrex/hive default-chain state-serving issue, which is out of scope here. This PR makes the omitted-block path behave identically to the explicit-latestpath (the spec change), proven by the unit tests above.Related