Improve eth_call state overrides#1882
Improve eth_call state overrides#1882arturgontijo wants to merge 5 commits intopolkadot-evm:masterfrom
eth_call state overrides#1882Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Geth-style per-account EVM state override support across RPC, runtime API, pallet runner, and executor: client-side validation and v7 payload encoding, runtime API bumped to v7 with Changes
Sequence DiagramsequenceDiagram
participant Client as RPC Client
participant Validator as Request Validator
participant Builder as StateOverride Builder
participant RuntimeAPI as Runtime API (v7)
participant EVMRunner as EVM Runner
participant Storage as Storage Backend
Client->>Validator: eth_call(..., state_overrides)
Validator->>Validator: validate_state_overrides()
alt invalid
Validator-->>Client: error
else
Validator->>Builder: build_state_override_payload()
Builder-->>RuntimeAPI: encoded state_override
Client->>RuntimeAPI: call(..., state_override)
RuntimeAPI->>EVMRunner: execute(..., state_override)
EVMRunner->>Storage: read(account, slot)
alt override exists
Storage-->>EVMRunner: return override value (missing -> 0)
else
Storage-->>EVMRunner: return on-chain value
end
EVMRunner->>Storage: write(account, slot, value)
alt account has active override
Storage-->>EVMRunner: update in-memory override only
else
Storage-->>EVMRunner: write to trie
end
EVMRunner-->>RuntimeAPI: ExecutionInfoV2
RuntimeAPI-->>Client: execution result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
template/runtime/src/lib.rs (1)
813-884:⚠️ Potential issue | 🟠 MajorAccount for
state_overridein the proof-size base estimate.The new payload is forwarded into execution, but
estimated_transaction_lenonly includes calldata/access-list/authorization-list sizes. Largestateoverrides should contribute to the base cost so the v7 path remains proportional to caller-supplied payload.🛠️ Proposed fix
if authorization_list.is_some() { estimated_transaction_len += authorization_list.encoded_size(); } + + if state_override.is_some() { + estimated_transaction_len += state_override.encoded_size(); + } let gas_limit = if gas_limit > U256::from(u64::MAX) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@template/runtime/src/lib.rs` around lines 813 - 884, The estimated_transaction_len currently omits the size of the caller-provided fp_evm::StateOverride, so large state overrides are not contributing to proof_size_base_cost; update the code that builds estimated_transaction_len (the local variable estimated_transaction_len) to include the encoded size of the state_override parameter (when non-empty) before computing (weight_limit, proof_size_base_cost), so the proof_size_base_cost passed into <Runtime as pallet_evm::Config>::Runner::call reflects the state_override payload as well.client/rpc/src/eth/execute.rs (1)
927-981:⚠️ Potential issue | 🔴 CriticalCritical: Both v7 branches encode the wrong number of runtime-API arguments.
The runtime-API v7
callsignature takes exactly 11 parameters (from, to, data, value, gas_limit, max_fee_per_gas, max_priority_fee_per_gas, nonce, estimate, access_list, authorization_list). There is nostate_overrideparameter in v7; it was added in the current (unsigned) version, which takes 12 parameters.estimate_gas v7 branch (lines 927–981): Encodes 13 arguments. This includes two extra parameters (
&None::<Vec<(sp_core::H160, H256, H256)>>and&None::<Vec<sp_core::H160>>). Additionally, the 12th parameter's type shape (Vec<(H160, H256, H256)>) does not match the intendedStateOverridetype. The moment this becomesSome(..)the payload will silently become incompatible with the runtime.call() v7 branch (lines 374–398): Also encodes 12 arguments, including
&state_override_payload. This exceeds the v7 API signature which takes only 11 parameters. The v7 call API does not accept state overrides; the branch should encode exactly 11 parameters without state_override.Both branches must be corrected to encode precisely 11 parameters matching the v7 API signature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/rpc/src/eth/execute.rs` around lines 927 - 981, The v7 branches are encoding too many/runtime-mismatched arguments; update both the estimate_gas v7 branch (where Encode::encode is called for estimate) and the call() v7 branch to encode exactly 11 parameters matching the v7 runtime API signature: (from, to, data, value, gas_limit, max_fee_per_gas, max_priority_fee_per_gas, nonce as Option<U256>, estimate_mode, access_list as Option<Vec<(H160, Vec<H256>)>> or Some(access_list), authorization_list). Remove the extra &None::<Vec<(sp_core::H160, H256, H256)>> and &None::<Vec<sp_core::H160>> (or state_override_payload) parameters, ensure the access_list is wrapped in an Option with the correct element types, and keep the Encode::encode call arguments and types exactly in that order so the CallApiAtParams->function "EthereumRuntimeRPCApi_call" receives the 11 expected args.
🧹 Nitpick comments (1)
frame/evm/src/runner/stack.rs (1)
1039-1048: Minor: helper name suggests trie-only read but also serves overrides.
read_persisted_storagereturns the override value when the account has an active override, so the name is a little misleading for readers who encounter the call sites instorage,original_storage, andset_storage. Consider renaming to something likeeffective_storage/read_current_slotto make the override-aware behavior obvious at the call site.No functional concern — override precedence, zero-on-missing, and the SSTORE → SLOAD observation path all look correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frame/evm/src/runner/stack.rs` around lines 1039 - 1048, The helper read_persisted_storage is override-aware but its name implies a trie-only read; rename it to a clearer name like effective_storage or read_current_slot and update all call sites (storage, original_storage, set_storage) to use the new name, keeping the implementation logic unchanged (i.e., still check self.state_override and fall back to <AccountStorages<T>>::get(address, index)); ensure references to state_override and AccountStorages<T> remain intact so override precedence and zero-default behavior are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@client/rpc/src/eth/execute.rs`:
- Around line 927-981: The v7 branches are encoding too many/runtime-mismatched
arguments; update both the estimate_gas v7 branch (where Encode::encode is
called for estimate) and the call() v7 branch to encode exactly 11 parameters
matching the v7 runtime API signature: (from, to, data, value, gas_limit,
max_fee_per_gas, max_priority_fee_per_gas, nonce as Option<U256>, estimate_mode,
access_list as Option<Vec<(H160, Vec<H256>)>> or Some(access_list),
authorization_list). Remove the extra &None::<Vec<(sp_core::H160, H256, H256)>>
and &None::<Vec<sp_core::H160>> (or state_override_payload) parameters, ensure
the access_list is wrapped in an Option with the correct element types, and keep
the Encode::encode call arguments and types exactly in that order so the
CallApiAtParams->function "EthereumRuntimeRPCApi_call" receives the 11 expected
args.
In `@template/runtime/src/lib.rs`:
- Around line 813-884: The estimated_transaction_len currently omits the size of
the caller-provided fp_evm::StateOverride, so large state overrides are not
contributing to proof_size_base_cost; update the code that builds
estimated_transaction_len (the local variable estimated_transaction_len) to
include the encoded size of the state_override parameter (when non-empty) before
computing (weight_limit, proof_size_base_cost), so the proof_size_base_cost
passed into <Runtime as pallet_evm::Config>::Runner::call reflects the
state_override payload as well.
---
Nitpick comments:
In `@frame/evm/src/runner/stack.rs`:
- Around line 1039-1048: The helper read_persisted_storage is override-aware but
its name implies a trie-only read; rename it to a clearer name like
effective_storage or read_current_slot and update all call sites (storage,
original_storage, set_storage) to use the new name, keeping the implementation
logic unchanged (i.e., still check self.state_override and fall back to
<AccountStorages<T>>::get(address, index)); ensure references to state_override
and AccountStorages<T> remain intact so override precedence and zero-default
behavior are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee9ed914-e1ff-4ec0-8a5f-11e4b466c696
📒 Files selected for processing (11)
client/rpc/src/eth/execute.rsframe/ethereum/src/lib.rsframe/evm/src/lib.rsframe/evm/src/runner/mod.rsframe/evm/src/runner/stack.rsframe/evm/src/tests.rsprimitives/evm/src/lib.rsprimitives/rpc/src/lib.rstemplate/fuzz/src/main.rstemplate/runtime/src/lib.rsts-tests/tests/test-state-override.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frame/evm/src/runner/stack.rs (1)
1447-1555:⚠️ Potential issue | 🟠 MajorDo not charge trie proof/storage-growth costs for overridden storage slots.
For an address present in
state_override, SLOAD/SSTORE is served from the in-memory map, but Line 1447 still records storage-meter costs and Lines 1543/1552 still charge account-storage proof size. This can make v7 override calls fail or scale with touched slots instead of on-chain proof work.🛠️ Proposed fix: skip external storage accounting for overridden slots
) -> Result<(), ExitError> { + // Storage for overridden accounts is served from the caller-provided + // in-memory map, so there is no trie proof or persistent storage growth + // to account for here. The EVM gasometer still charges normal SLOAD/SSTORE gas. + if matches!( + target, + StorageTarget::Slot(address, _) if self.state_override.contains_key(&address) + ) { + return Ok(()); + } + if let Some(storage_meter) = self.storage_meter.as_mut() { storage_meter .record_dynamic_opcode_cost(opcode, gas_cost, target)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frame/evm/src/runner/stack.rs` around lines 1447 - 1555, The code currently charges storage-meter costs unconditionally (via storage_meter.record_dynamic_opcode_cost) before checking the overlay/state override, causing charges for overridden slots; change the logic so the overlay check runs first (the match that sets accessed_storage using StorageTarget and recorded().account_codes / recorded().account_storages) and only call record_dynamic_opcode_cost when accessed_storage.is_some() (i.e., the slot/code is not overridden), leaving the existing proof-size recording for Opcode::SLOAD and Opcode::SSTORE unchanged because they already gate on accessed_storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frame/evm/src/runner/stack.rs`:
- Around line 991-995: The in-memory BTreeMap state_override is being mutated
directly during EVM frames (e.g., where SSTORE or SELFDESTRUCT call paths update
or clear entries) but those mutations aren't rolled back on
exit_revert/exit_discard, so reverted inner calls can leak into subsequent
SLOADs during the same eth_call; to fix, make state_override frame-transactional
by snapshotting or journaling changes at frame entry and restoring on
non-successful exits: take a copy or push a change-log for state_override at the
start of enter/execute of an EVM frame and apply commits only on successful exit
paths, and on exit_revert/exit_discard rollback by restoring the prior snapshot
or replaying inverse journal entries; reference the state_override field and the
exit_revert/exit_discard/execute frame code paths to locate where to push/pop
snapshots or journals.
---
Outside diff comments:
In `@frame/evm/src/runner/stack.rs`:
- Around line 1447-1555: The code currently charges storage-meter costs
unconditionally (via storage_meter.record_dynamic_opcode_cost) before checking
the overlay/state override, causing charges for overridden slots; change the
logic so the overlay check runs first (the match that sets accessed_storage
using StorageTarget and recorded().account_codes / recorded().account_storages)
and only call record_dynamic_opcode_cost when accessed_storage.is_some() (i.e.,
the slot/code is not overridden), leaving the existing proof-size recording for
Opcode::SLOAD and Opcode::SSTORE unchanged because they already gate on
accessed_storage.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3af0eab-d54b-494a-92bd-0881166b68b1
📒 Files selected for processing (2)
client/rpc/src/eth/execute.rsframe/evm/src/runner/stack.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frame/evm/src/runner/stack.rs (1)
1213-1243: Consider short-circuiting the per-frame snapshot when there are no overrides.Correctness-wise the journaling looks right and the paired
.expect(...)onexit_revert/exit_discardmirrors the existing substate pairing invariant. One small perf note:enter()clones the entirestate_overridemap on every EVM frame, even when no override is active (the typical transactional/eth_call-without-override path) or when the frame never mutates overrides. For the common empty-map case this is effectively free, but for a large override payload across a deeply nested call graph it scales asO(depth × n)of pointless allocations.A cheap improvement is to skip the snapshot when the map is empty and only journal on mutation (SSTORE against an overridden account /
reset_storage). That keeps Geth snapshot parity without paying for it on non-override calls.♻️ Sketch: skip empty snapshots
fn enter(&mut self, gas_limit: u64, is_static: bool) { - // Journal the override alongside the Substrate storage transaction that - // `SubstrateStackSubstate::enter` opens, so an inner frame's SSTORE or - // SELFDESTRUCT-driven `reset_storage` against an overridden account can - // be rolled back on revert/discard (Geth StateDB parity). - self.state_override_journal - .push(self.state_override.clone()); + // Journal the override alongside the Substrate storage transaction + // `SubstrateStackSubstate::enter` opens. Skip the clone when there + // is nothing to roll back. + let snapshot = if self.state_override.is_empty() { + BTreeMap::new() + } else { + self.state_override.clone() + }; + self.state_override_journal.push(snapshot); self.substate.enter(gas_limit, is_static) }A fuller copy-on-write / inverse-journal design (record only the mutated
(address, slot)pairs inside each frame) would scale better still for large override payloads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frame/evm/src/runner/stack.rs` around lines 1213 - 1243, The enter/exit snapshot currently clones state_override on every frame; change enter to only push a snapshot when state_override is non-empty (e.g., push Some(self.state_override.clone()) or push None for empty) to avoid unnecessary clones, and update exit_revert/exit_discard to pop the Option and only restore self.state_override when the popped value is Some(snapshot) while exit_commit should still pop and ignore the snapshot; adjust uses of state_override_journal, enter, exit_commit, exit_revert, and exit_discard accordingly to preserve the pairing invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frame/evm/src/runner/stack.rs`:
- Around line 1213-1243: The enter/exit snapshot currently clones state_override
on every frame; change enter to only push a snapshot when state_override is
non-empty (e.g., push Some(self.state_override.clone()) or push None for empty)
to avoid unnecessary clones, and update exit_revert/exit_discard to pop the
Option and only restore self.state_override when the popped value is
Some(snapshot) while exit_commit should still pop and ignore the snapshot;
adjust uses of state_override_journal, enter, exit_commit, exit_revert, and
exit_discard accordingly to preserve the pairing invariant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fd76ffb-dd38-4c8f-a06e-24aaa411d75c
📒 Files selected for processing (2)
frame/evm/src/runner/stack.rsframe/evm/src/tests.rs
1 participant
Summary
eth_callstate overrides previously forced the RPC layer to enumerate every on-chain storage key for the target address when the caller used thestatefield (full storage replacement). Cost scaled with on-chain storage size, not caller payload — a DoS vector on public RPC endpoints and an outright failure mode for the"state": {}wipe case.This PR mirrors Geth's approach: the account is treated as "destructed and recreated" with the caller's override as its fresh state. Cost becomes O(caller payload) regardless of on-chain size.
Approach
EthereumRuntimeRPCApiv7, adding astate_overrideparameter tocall.fp_evm::StateOverridethroughpallet-evm'sRunner::callintoSubstrateStackState.SLOADagainst an overridden account reads from the in-memory map only; missing slots return zero (Geth "fresh state object" semantics).SELFDESTRUCTclears the override for the affected address (reset_storage).stateoverrides;stateDiffstill flows through the overlay (bounded by caller).state+stateDiffon the same account (Geth-parity).Impact on RPC clients
JSON-RPC wire contract is unchanged — no client library needs updating.
eth_callwith no overrideseth_callwithbalance/nonce/code/stateDiffeth_callwith smallstateoverrideeth_callwith largestateoverride or"state": {}state+stateDiffon same accountTests
frame/evm/src/tests.rs— 8 runner-level tests covering persisted-slot replacement,"state": {}wipe, per-account isolation, "missing slot reads zero", SSTORE-wins-over-override, non-persistence,Nonebaseline, and directreset_storagecoverage for the SELFDESTRUCT-parity fix.client/rpc/src/eth/execute.rs— 12 unit tests covering the extractedvalidate_state_overridesandbuild_state_override_payloadhelpers, including the empty-statefull-wipe marker.ts-tests/tests/test-state-override.ts— 2 new end-to-end cases for the"state": {}wipe and thestate+stateDiffrejection.