fix: extend pending-block RPC filter to state-query methods - #340
fix: extend pending-block RPC filter to state-query methods#340operagxoksana wants to merge 1 commit into
Conversation
eth_getBalance, eth_getTransactionCount, eth_getCode, eth_getStorageAt, eth_call and eth_estimateGas accept a block-tag param that can be "pending", but were not covered by is_pending_block_method(). Their block-tag is not the first positional param, so a new extract_param_at() helper is added to locate it.
10de00c to
f448ea4
Compare
osr21
left a comment
There was a problem hiding this comment.
Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer. I have no write access to this repository, so the review state I set carries no merge authority and is advisory only. Please treat this as one contributor's technical assessment, and defer to Circle maintainers for the binding review. Rust findings below are source-review only (no
cargo/rustcin my environment); everything else is executed or probed live and labelled as such.
The premise is real and I verified it against the live network, but I think the response shape blocks this as written: null is not a representable answer for these six methods, and one of them sits on the hot path of every viem transaction.
What I verified and agree with
The gap is genuine and reachable today. Against https://rpc.testnet.arc.network (chainId 0x4cef52, head 0x399f129), the methods this PR adds all answer a "pending" tag right now:
eth_getTransactionCount [addr,"pending"] -> 0x0
eth_getBalance [addr,"pending"] -> 0xd14311c7eb2f752958d
eth_call [{to},"pending"] -> 0x
eth_estimateGas [{to},"pending"] -> 0x5f12
eth_getStorageAt [addr,"0x0","pending"] -> 0x0000...
The existing filter is confirmed live on the same endpoint, so this is a true gap in a deployed control rather than a theoretical one — eth_getBlockReceipts ["pending"] and eth_getBlockTransactionCountByNumber ["pending"] both return null, which is the middleware's null_response, not an upstream error.
The object keys are correct. I checked every signature against the pinned reth-rpc-eth-api (Cargo.lock → tag=v2.2.0, 88505c7), crates/rpc/rpc-eth-api/src/core.rs. All six bind block_number, and the positional indices in state_query_block_param_position match exactly, including eth_getStorageAt at index 2:
async fn balance(&self, address: Address, block_number: Option<BlockId>) -> RpcResult<U256>;
async fn storage_at(&self, address: Address, index: JsonStorageKey, block_number: Option<BlockId>) -> RpcResult<B256>;
async fn call(&self, request: TxReq, block_number: Option<BlockId>, state_overrides: ..., block_overrides: ...) -> RpcResult<Bytes>;BlockId is also the right type — it picks up the EIP-1898 object form ({"blockNumber":"pending"}) as well as string tags. And the existing test_enabled_allows_non_pending_methods case survives, because it passes [] for eth_getBalance/eth_call, so index 1 is absent and optional_next yields None.
Blocking: null is not a valid response for any of these six
Every method the filter covered before this PR returns an Option in reth, so null is the method's own canonical "not found" value:
async fn block_receipts(...) -> RpcResult<Option<Vec<R>>>;
async fn header_by_number(...) -> RpcResult<Option<H>>;
async fn uncle_by_block_number_and_index(...) -> RpcResult<Option<B>>;None of the six added here are optional — they are U256, Bytes, and B256. Returning null puts a value on the wire that the method's own schema cannot express, so clients don't degrade gracefully, they misparse.
I ran this against viem 2.52.2 with a transport stubbed to return JSON-RPC success with result: null:
getTransactionCount(pending) => THREW TypeError: Cannot convert null to a BigInt
getBalance(pending) => THREW TypeError: Cannot convert null to a BigInt
estimateGas(pending) => THREW EstimateGasExecutionError: An error occurred.
call(pending) => RESOLVED: {"data":null}
getCode(pending) => RESOLVED: null
getStorageAt(pending) => RESOLVED: null
Two distinct failure modes, both bad: a raw TypeError that names nothing an operator could act on, and — worse — three methods that silently succeed with a null payload.
The consequential one is eth_getTransactionCount. A pending nonce is how wallets pick the next nonce, and in viem it is not an edge case — it is the default write path:
_esm/actions/wallet/prepareTransactionRequest.js:250 blockTag: 'pending' <- every sendTransaction/writeContract without an explicit nonce
_esm/utils/nonceManager.js:76 blockTag: 'pending'
_esm/actions/wallet/prepareAuthorization.js:74 blockTag: 'pending' (EIP-7702)
_esm/actions/public/verifyHash.js:123 blockTag: 'pending' (ERC-6492)
filter_pending_txs defaults to true (node.rs:150, rpc_middleware.rs:109), so on a default-configured node this turns "send a transaction from a viem dapp" into TypeError: Cannot convert null to a BigInt. That is a much larger blast radius than the block-content methods the filter covered previously, none of which sit on a signing path.
Severity of the leak itself, calibrated honestly
Worth weighing against that cost: on the live node the exposure is currently nil in steady state. For an active address at head 0x399f129, pending and latest are byte-identical:
eth_getBalance pending=0x16c0a26072333adc08ba latest=0x16c0a26072333adc08ba same
eth_getTransactionCount pending=0x0 latest=0x0 same
That is consistent with eth_getBlockByNumber ["pending"] returning -32014 requested data not available — with no pending block, reth's state provider already falls back to latest. So the real exposure is the narrow, racy window your doc comment describes, when the consensus engine briefly publishes a proposed block. Real, worth closing, but not a standing disclosure — which argues for closing it in a way that costs clients nothing.
Suggested alternative: coerce pending → latest instead of nulling
Since reth already resolves pending state to latest whenever no pending block exists (demonstrated above), making that mapping explicit for these six methods would:
- close the transient window deterministically, which is the actual goal;
- leak nothing, since with pending transactions hidden by default a
pendingnonce could never have included other senders' transactions anyway; - keep every value schema-valid, so viem, ethers, and wallets keep working unchanged.
In other words the observable behaviour for clients stays exactly what it already is in the common case, and the pre-finalization read disappears. If you prefer to reject rather than coerce, an explicit JSON-RPC error — mirroring PENDING_TX_SUBSCRIPTION_ERROR_CODE in the subscription path — is still far better than null, because at least it surfaces an actionable message instead of a TypeError deep inside a client library. What I'd avoid is null, which is the one option that is both invalid per schema and silent for three of the six.
This is a product call as much as a technical one, so I'd defer to maintainers on which of the two to take.
Coverage gaps, if the intent is to close the class
From the same pinned core.rs, these also take a block parameter and remain uncovered after this PR:
| method | index | object key | note |
|---|---|---|---|
eth_getProof |
2 | block_number |
state proof at pending state |
eth_createAccessList |
1 | block_number |
|
eth_simulateV1 |
1 | block_number |
|
eth_getStorageValues |
1 | block_number |
|
eth_getAccount |
1 | block |
different key |
eth_getAccountInfo |
1 | block |
different key |
eth_feeHistory |
1 | newest_block |
BlockNumberOrTag, not BlockId |
eth_getUncleByBlockNumberAndIndex |
0 | number |
block-content class, missed by is_pending_block_method |
eth_getBlockAccessListByBlockNumber |
0 | number |
same |
eth_getBlockAccessListRaw |
0 | block |
same |
Calibrating that down: eth_getProof, eth_createAccessList, and eth_getHeaderByNumber all return -32601 method not supported on the public endpoints I probed, so they are not exposed there. That is provider namespace configuration though, not something arc-node enforces, so a self-hosted node with the eth namespace enabled would still expose them. The block and newest_block keys are worth noting because a mechanical copy of the block_number entry would silently miss them — your helper returns the key per method, which is exactly the right shape to extend.
Smaller points
- Named-param key list is narrower than its neighbour.
extract_param_atis called with&[key]only, while the adjacenteth_getBlockReceiptsbranch passes bothBLOCK_ID_OBJECT_KEY_SNAKEandBLOCK_ID_OBJECT_KEY_CAMEL. jsonrpsee binds snake-case proc-macro field names, soblockNumbershouldn't occur — but the file already chose to be defensive one branch above, and the inconsistency will read as an oversight later. Either add the camel variant or drop a comment saying why it isn't needed here. - Changelog. Repo convention covers this exact class — v0.8.0 carries "[EL] Complete the pending-block RPC filter…" under
### Fixes. This PR adds no entry, and given the client-visible impact above it may warrant aBREAKING_CHANGES.mdnote too, depending on which response strategy you land on. - Match guards are unnecessary.
const &strvalues are valid match patterns, soETH_GET_BALANCE_METHOD => Some((1, "block_number")),works directly without them if m == …guard. - Test gap. The new tests cover array form only. Given
extract_param_athas a distinct object-params branch, an object-form case ({"address":"0x…","block_number":"pending"}) and an EIP-1898 case ([addr, {"blockNumber":"pending"}]) would pin the two paths that are easiest to regress.
Structurally the change is sound — the helper is the right abstraction, the indices and keys are right, and the doc comment on filter_pending_txs was kept in sync. My concern is only the value it returns.
eth_getBalance, eth_getTransactionCount, eth_getCode, eth_getStorageAt, eth_call and eth_estimateGas accept a block-tag param that can be "pending", but were not covered by is_pending_block_method(). Their block-tag is not the first positional param, so a new extract_param_at() helper is added to locate it.