fix(evmrpc): mask state overrides with a simulation-local storage overlay#3722
Conversation
…. Install a simulation-local storage overlay with committed/current views, journal revert support, and configurable caps on override accounts and slots. Added/updated tests.
PR SummaryMedium Risk Overview RPC limits: new Reviewed by Cursor Bugbot for commit 1ac7010. Bugbot is set up for automated code reviews on this repo. Configure here. |
amir-deris
changed the title
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3722 +/- ##
==========================================
- Coverage 59.83% 58.94% -0.90%
==========================================
Files 2278 2191 -87
Lines 189132 179386 -9746
==========================================
- Hits 113173 105738 -7435
+ Misses 65881 64370 -1511
+ Partials 10078 9278 -800
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The overlay-based masking of persisted storage is a solid, well-tested fix, and the configurable override caps are cleanly plumbed and validated. However, SetStorage still deletes the account's code hash (and clears code/size/nonce), which makes an eth_call/estimateGas against a state-overridden contract execute as if it had no bytecode — the same class of correctness bug the PR set out to fix.
Findings: 1 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- cursor-review.md is empty — the Cursor second-opinion pass produced no output; only Codex's review was available to merge.
- validateStateOverrides bounds slots per-account and accounts separately, so a request can still carry up to MaxStateOverrideAccounts × MaxStateOverrideSlots slots (100 × 1000 = 100k by default) plus the same again via StateDiff. This is acceptable but worth noting if a global slot budget was intended.
- No test exercises executing an overridden contract's code after a
stateoverride (only storage read/mask behavior is covered), so the code/nonce-wipe regression is not caught by the suite — consider adding an eth_call-against-overridden-contract test.
| func (s *DBImpl) SetStorage(addr common.Address, states map[common.Hash]common.Hash) { | ||
| s.clearAccountState(addr) | ||
| s.k.PrepareReplayedAddr(s.ctx, addr) | ||
| if deleteIfExists(s.k.PrefixStore(s.ctx, types.CodeHashKeyPrefix), addr[:]) { |
There was a problem hiding this comment.
[blocker] SetStorage still deletes the account's code hash and (via clearAccountCodeAndNonce) its code, code size, and nonce. This defeats the primary use of a state storage override: go-ethereum's StateOverride.Apply sets Nonce/Code before calling SetStorage for the state field, so:
- A
state-only override on an existing contract wipes its persisted bytecode/nonce, so a subsequent eth_call/eth_estimateGas executes the contract as if it had no code (returns empty / wrong gas). - An override combining
code(ornonce) withstatealso loses the just-applied code/nonce, since SetCode writes CodeHashKeyPrefix (keeper/code.go:32) which this deleteIfExists then removes.
Geth's SetStorage only replaces storage; it does not touch code or nonce. Since the overlay now masks storage without purging it, this branch should no longer clear code/nonce — drop the code-hash delete + clearAccountCodeAndNonce here (or scope it so it doesn't undo code/nonce overrides) so overridden contracts still execute their bytecode. As-is the PR only half-fixes the state-override correctness problem it targets.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR replaces the purge-and-rewrite state-override path with a simulation-local storage overlay that masks (rather than deletes) an account's persisted storage while preserving its code/nonce/balance, fixing a real bug and matching go-ethereum semantics; it also adds configurable size caps. The change is correct and well-tested, with only minor non-blocking notes.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
debug_traceCall(DebugAPI.TraceCall→tracersAPI.TraceCall) also acceptsStateOverridesand routes through the sameSetStorageoverlay path, butvalidateStateOverridesis only wired intoeth_call/eth_estimateGas/eth_estimateGasAfterCalls. Consider applying the same account/slot caps there so the DoS-hardening is uniform across all override-accepting endpoints.- Semantic nuance for awareness (not a bug): the overlay freezes
committedatSetStoragetime and only mutatescurrenton subsequentSetState, soGetCommittedStatefor a masked account keeps returning the override-time value. go-ethereum'sfakeStorageis a single map whereGetCommittedStatereflects laterSetStatewrites. This can produce slightly different SSTORE gas metering (EIP-2200/3529) undereth_estimateGasfor overridden accounts that are then written during execution. The PR's frozen-committed model is arguably more correct as a tx-start view; just confirm this is intended. - Cursor's second-opinion review (
cursor-review.md) was empty — that pass produced no output. Codex's review produced one finding (the independent State/StateDiff slot counting), captured as an inline comment below. - Doc nit: the PR description says
clearAccountCodeAndNonceis "still used to clear code/nonce when masking storage on an existing account," but the newSetStorageno longer clears code/nonce (that's the point of the fix);clearAccountCodeAndNonceis only called fromclearAccountState. The description slightly misstates the refactor. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| return fmt.Errorf("state override has too many accounts (%d > %d)", len(*overrides), maxAccounts) | ||
| } | ||
| if maxSlots > 0 { | ||
| for addr, account := range *overrides { |
There was a problem hiding this comment.
[suggestion] State and StateDiff are capped independently, so a single account can supply up to maxSlots entries in each map and allocate/apply up to 2× the configured per-account slot cap (and go-ethereum's Apply will process both when both are present). If the cap is meant to bound total override storage per account, consider checking len(account.State) + len(account.StateDiff) against maxSlots. Low severity since the total is still bounded, but worth aligning with intent. (Also raised by Codex.)
There was a problem hiding this comment.
@seidroid check the go ethereum error when both State and StateDiff are provided (which is the same for SEI's fork which this repo uses):
https://github.com/ethereum/go-ethereum/blob/master/internal/ethapi/override/override.go#L107
https://github.com/sei-protocol/go-ethereum/blob/main/internal/ethapi/override/override.go#L100
So in this case, they are not going to get processed.
There was a problem hiding this comment.
Claude finished @amir-deris's task in 2m 8s —— View job
Analysis: State + StateDiff both provided → rejected before either is applied
You're right, and this refutes the Cursor Bugbot "double slot cap" finding.
What go-ethereum's override.Apply does. In internal/ethapi/override/override.go (upstream master L107 / the sei-protocol/go-ethereum fork at v1.15.7-sei-17 L100), StateOverride.Apply rejects any account that supplies both state and stateDiff before touching the StateDB:
// Replace entire state if caller requires.
if account.State != nil && account.StateDiff != nil {
return fmt.Errorf("account %s has both 'state' and 'stateDiff'", addr.Hex())
}
if account.State != nil {
statedb.SetStorage(addr, account.State) // full-replace path -> overlay
}
if account.StateDiff != nil { // else-only, additive path
for key, value := range account.StateDiff {
statedb.SetState(addr, key, value)
}
}Because that error returns early, SetStorage (which allocates the new simulation-local committed/current overlay) and the SetState loop are mutually exclusive per account — at most one of the two maps is ever applied. So the per-account overlay can never approach 2 × MaxStateOverrideSlots; the "up to roughly twice the limit" scenario is unreachable at execution time.
Why the independent caps in validateStateOverrides are fine (evmrpc/simulate.go:490-499):
if len(account.State) > maxSlots { ... }
if len(account.StateDiff) > maxSlots { ... }A request that puts maxSlots in each map passes this pre-check, but it is then rejected by Apply during simulation before any oversized overlay is allocated. The memory bound the caps exist to protect therefore still holds: only one map per account is ever materialized, and each is individually capped at maxSlots.
Conclusion: The Bugbot finding is not valid — an account with both maps populated is rejected outright, so no code change is needed for correctness or for the overlay memory bound. Your reply on the inline thread is correct.
One optional (non-blocking) nicety: validateStateOverrides currently relies on Apply to reject the both-present case mid-simulation rather than failing fast up front. If you want a cleaner/earlier error on the RPC boundary, you could add an explicit if len(account.State) > 0 && len(account.StateDiff) > 0 check to validateStateOverrides mirroring geth's message — but this is purely cosmetic, since the request is already rejected before any oversized allocation occurs.
--- · branch amir/plt-365-rpc-state-override-issue
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Replaces the purge-and-rewrite state-override behavior with a simulation-local storage overlay and adds configurable DoS caps on override size. The change is well-tested and correct for the common path; the only substantive concern is that the overlay is keyed to the address in TemporaryState and is not cleared when an overridden account is recreated (CreateAccount) or self-destructs mid-simulation, an edge-case semantic divergence.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdandREVIEW_GUIDELINES.mdare both empty — the Cursor second-opinion pass produced no output and there are no repo-specific review guidelines to apply, so those inputs contributed nothing to this review.- The overlay lives in
TemporaryState.storageOverrideskeyed by address and is independent of account lifecycle. If simulated bytecode self-destructs an overridden account (SelfDestruct) or recreates one (CreateAccount→clearAccountState), storage reads still route through the stale overlay instead of returning empty storage, unlike go-ethereum where storage follows the state object. Both are rare ineth_call/debug_traceCall(CREATE2 collision / same-tx recreate), but consider dropping the overlay entry in those paths for full parity — this generalizes Codex's finding. - Only full
stateoverrides go through the overlay (SetStorage);stateDiffoverrides still write directly to the working keeper store viaSetState. This matches go-ethereum's additive semantics and the PR's stated scope, but is worth a brief comment so future readers don't assumestateDiffis also masked/non-mutating. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| deleteIfExists(s.k.PrefixStore(s.ctx, types.CodeKeyPrefix), acc[:]) | ||
| deleteIfExists(s.k.PrefixStore(s.ctx, types.CodeSizeKeyPrefix), acc[:]) | ||
| deleteIfExists(s.k.PrefixStore(s.ctx, types.NonceKeyPrefix), acc[:]) | ||
| s.clearAccountCodeAndNonce(acc) |
There was a problem hiding this comment.
[suggestion] clearAccountState (called from CreateAccount and clearAccountStateIfDestructed) purges only the persisted store; it does not remove s.tempState.storageOverrides[acc.Hex()]. If simulated execution recreates or self-destructs an account that carries a state override, subsequent GetState/GetCommittedState still read the frozen overlay rather than empty storage, so eth_call/debug_traceCall can report behavior that wouldn't occur on-chain (e.g. CREATE2 collision or same-tx recreation). Consider deleting the overlay entry here (and on self-destruct) to match go-ethereum's storage-follows-account semantics. Edge case, non-blocking.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2e4925d. Configure here.
| } | ||
|
|
||
| func (s *DBImpl) GetCommittedState(addr common.Address, hash common.Hash) common.Hash { | ||
| if ov, ok := s.tempState.storageOverrides[addr.Hex()]; ok { |
There was a problem hiding this comment.
Tracing skips overlay drop
High Severity
Simulation RPC contexts use tracing mode, so CreateAccount skips clearAccountState. Storage override removal now lives only in that path, so a recreated account (e.g. CREATE2 collision) can keep serving masked override slots instead of empty storage during eth_call, eth_estimateGas, and related simulations.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2e4925d. Configure here.
There was a problem hiding this comment.
Not a regression — eth_call/eth_estimateGas don't run in tracing mode.
The claim assumes simulation RPCs set IsTracing(), but they don't. eth_call/eth_estimateGas → DoCall/DoEstimateGas → StateAndHeaderByNumberOrHash, which builds the ctx at simulate.go:323 with .WithIsEVM(true) but no .WithIsTracing(true). IsTracing() is only ever set in tracers.go:599 and trace_profile.go:143 (the debug_trace* APIs). So for these APIs IsTracing()==false, CreateAccount runs clearAccountState, and the overlay drop executes as intended.
The only context where the drop is skipped is genuine tracing (debug_traceCall), and there it's pre-existing/accepted behavior: that branch already skips all storage clearing on recreation (persisted store isn't purged either), per the documented caveat at state.go:19-20. My change doesn't touch that path or make it worse — it restores go-ethereum parity for exactly the non-tracing simulation paths the comment flags as broken.
There was a problem hiding this comment.
The PR replaces the purge-and-rewrite state-override behavior with a simulation-local storage overlay (aligning with go-ethereum semantics) and adds bounded caps on override size — a clean, well-tested change. However, the new storageOverrideRemove journal entry holds a raw pointer to an overlay object, and because Copy() shallow-copies the journal, a copied StateDB that reverts over such an entry re-installs the original DB's overlay into the copy, breaking the copy isolation that DeepCopy was added to guarantee.
Findings: 2 blocking | 4 non-blocking | 1 posted inline
Blockers
- Copy isolation break (confirmed, matches Codex):
DBImpl.Copy()deep-copiesTemporaryState.storageOverridesbut shallow-copies the journal slice, so the copy shares the same*storageOverrideRemoveentries as the original. Each such entry'sprev *storageOverridepoints to the original DB's overlay object; when the copy reverts past that entry (storageOverrideRemove.revert), it installs the original's overlay pointer into the copy'stempState, after which overlay writes viasetOverrideStatemutate a shared object and bleed between the trace/simulation copies. Fix by deep-copying the overlay carried instorageOverrideRemove.prev(either when journaling inclearAccountState, or when copying the journal inCopy), or by not storing a mutable overlay pointer in the entry. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Missing test for the exact isolation scenario behind the blocker: no test exercises
Copy()after aclearAccountState/storageOverrideRemoveon a masked account and then reverts both original and copy to observe cross-contamination. Adding one would catch regressions of the shared-pointer fix. - Second-opinion inputs:
cursor-review.mdwas empty (Cursor produced no output) andREVIEW_GUIDELINES.mdwas empty/missing, so no repo-specific review standards were applied. Codex produced exactly one finding, which is the confirmed blocker above. validateStateOverridescheckslen(State)andlen(StateDiff)independently againstmaxSlotsrather than their combined total; a request supplying both could allocate up to 2×maxSlots slots for one account. Likely acceptable, but worth a comment or a combined check if the cap is meant to bound total overlay memory per account.- Note: during tracing,
CreateAccountskipsclearAccountState(pre-existing!s.ctx.IsTracing()guard), so a recreated account keeps its frozen overlay in trace mode — consistent with the existing 'tracing could be incorrect in theory' comment, but now also applies to storage overrides.
| // storage. prev holds the removed overlay for restoration on revert. | ||
| storageOverrideRemove struct { | ||
| account common.Address | ||
| prev *storageOverride |
There was a problem hiding this comment.
[blocker] storageOverrideRemove stores a raw pointer to the overlay (prev *storageOverride). Combined with Copy() shallow-copying the journal (copy(journal, s.journal) in statedb.go), this breaks copy isolation: a copied StateDB shares this entry with the original, and when it reverts past this entry, revert installs the original's overlay pointer into the copy's tempState.storageOverrides. Subsequent setOverrideState writes on the copy then mutate an overlay object still referenced by the original's journal, so overlay state can bleed between trace/simulation copies — defeating the isolation that TemporaryState.DeepCopy was added to provide. Deep-copy the overlay stored in prev (when journaling in clearAccountState or when copying the journal), or avoid persisting a mutable overlay pointer here.
| transientAccounts map[string][]byte | ||
| transientModuleStates map[string][]byte | ||
| transientAccessLists *accessList | ||
| storageOverrides map[string]*storageOverride | ||
| surplus sdk.Int // in wei | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 debug_traceCall silently drops the 'state' state-override field: overlays installed in tempState.storageOverrides via SetStorage are wiped by Backend.PrepareTx → CleanupForTracer (statedb.go:98, s.tempState = NewTemporaryState()) before ApplyTransactionWithEVM runs, so the trace returns the persisted on-chain value with no error. This is a regression introduced by this PR — pre-PR SetStorage wrote through the multistore, which flushCtxs() preserved across the reset. The 'stateDiff' / 'code' / 'nonce' / 'balance' fields still work because they go through multistore-backed writes; only 'state' regresses. Fix: carry storageOverrides (and journal entries) across the tempState reset in CleanupForTracer / ResetForTracer, or apply overrides after PrepareTx.
Extended reasoning...
What the bug is
The new overlay-based storage override is stored exclusively in tempState.storageOverrides (installed by SetStorage at x/evm/state/state.go:220-232, initialized in NewTemporaryState at statedb.go:284). Nothing is written to the multistore. This works fine for eth_call / eth_estimateGas / eth_estimateGasAfterCalls, which never invoke PrepareTx. But debug_traceCall unconditionally routes through PrepareTx before executing the traced message, and PrepareTx calls CleanupForTracer which reallocates tempState — destroying every overlay that StateOverrides.Apply just installed.
The specific code path
Traced end-to-end through the vendored sei-protocol/go-ethereum@v1.15.7-sei-17:
DebugAPI.TraceCall(evmrpc/tracers.go:534) →api.tracersAPI.TraceCall(upstreameth/tracers/api.go:972).- Upstream
TraceCallat line 1034:config.StateOverrides.Apply(statedb, precompiles). override.Apply(internal/ethapi/override/override.go:104-105): foraccount.State != nil, callsstatedb.SetStorage(addr, account.State).- Sei's new
SetStorage(x/evm/state/state.go:216-232) allocates a*storageOverride{committed, current}and stores it ins.tempState.storageOverrides[addr.Hex()]. No multistore write. - Upstream
TraceCallat line 1059 callsapi.traceTx(...). traceTxat line 1134:api.backend.PrepareTx(statedb, tx).- Sei's
Backend.PrepareTx(evmrpc/simulate.go:771-773) unconditionally callstypedStateDB.CleanupForTracer(). CleanupForTracer(x/evm/state/statedb.go:91-102) at line 98 executess.tempState = NewTemporaryState()— the freshTemporaryStatehasstorageOverrides: make(map[string]*storageOverride), so every overlay installed in step 4 is orphaned.core.ApplyTransactionWithEVMruns at line 1137.GetState/GetCommittedState(state.go:26-38) look ups.tempState.storageOverrides[addr.Hex()], find no entry (empty map), and fall through tos.getState(s.ctx, addr, hash)— reading the persisted keeper store rather than the override.
Why existing code doesn't prevent it
- The PR's own tests (
TestSetStorageOverlay,TestSetStoragePreservesCodeAndNonce,TestSetStorageEmptyMasksAll,TestCreateAccountDropsStorageOverride) all callSetStoragedirectly on a rawDBImpland never route throughPrepareTx, so they can't observe the wipe. TestTraceCallinevmrpc/tracers_test.godoesn't pass anystateOverrides.flushCtxs()at the top ofCleanupForTracerpreserves multistore-backed changes (SetNonce/SetCode/SetBalanceandstateDiff-triggeredSetStateon a non-masked account all write to the multistore). But the overlay-backedstatefield lives purely intempStateand is destroyed by the reallocation.PrepareTxNoFlush(simulate.go:803-805) is equally affected —ResetForTracer(statedb.go:108-114) also doess.tempState = NewTemporaryState().
Impact
Silent wrong result on the exact endpoint this PR advertises support for. debug_traceCall-with-state-overrides is a primary tracing use case (masking a token balance / allowance slot to simulate a hypothetical scenario), and it now returns the on-chain persisted value with no error signal.
Pre-PR this worked: SetStorage used to call clearAccountState(addr) + SetState(...) per key, both of which wrote through s.k.PurgePrefix / s.k.SetState to the multistore. Those writes survived CleanupForTracer because flushCtxs() walks the cache multistore chain and writes each layer down before switching to snapshottedCtxs[0]. Post-PR the overlay lives only in tempState and vanishes.
Step-by-step proof
Concrete failure: a caller sends
{
"method": "debug_traceCall",
"params": [
{ "to": "0xTOKEN", "data": "balanceOf(user)" },
"latest",
{ "stateOverrides": { "0xTOKEN": { "state": { "<balanceSlot>": "0x00...3635c9adc5dea00000" } } } }
]
}(overriding the ERC-20 balance slot to 1000e18).
DebugAPI.TraceCallaccepts the config, forwards totracersAPI.TraceCall.config.StateOverrides.Apply(statedb, ...)callsstatedb.SetStorage(0xTOKEN, {slot: 1000e18})→ allocates overlay, stores intempState.storageOverrides["0xTOKEN"].traceTxcallsPrepareTx→CleanupForTracer→tempState = NewTemporaryState()→storageOverridesis now an empty map.- EVM executes
balanceOf(user);GetState(0xTOKEN, balanceSlot)finds no overlay entry, reads persisted keeper store → returns the real on-chain balance (typically 0 or the actual balance), NOT 1000e18. - Trace completes successfully, returning the wrong result.
Contrast: the identical payload sent via eth_call on the same node correctly returns 1000e18 because DoCall never invokes PrepareTx. That divergence between eth_call and debug_traceCall on the same override input is the observable symptom.
Fix
Simplest is to preserve storageOverrides (and any journal entries that reference them) across the tempState reset in both CleanupForTracer and ResetForTracer — e.g. capture the current overlays before reallocation and copy them onto the fresh TemporaryState. Alternatively, hoist StateOverrides.Apply to run after PrepareTx (but that would require overriding the upstream trace flow), or back the overlay by the multistore rather than tempState.
There was a problem hiding this comment.
Solid, well-tested fix that replaces the destructive purge-and-rewrite state-override path with a simulation-local storage overlay (correctly preserving code/nonce per go-ethereum semantics) and adds bounded, configurable caps validated on all override-accepting endpoints. No blocking issues; a few minor observations.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex flagged that
validateStateOverridescapsStateandStateDiffindependently, so an account could theoretically pass withmaxSlotsin each (up to 2x). Practical impact is low: in go-ethereum'sOverrideAccount.Apply,stateandstateDiffare mutually exclusive per account (Apply errors if both are set), so the double-overlay is normally unreachable, and even worst-case memory is a bounded 2x of a small config default. Consider capping the combined/union slot count for defense-in-depth, or at least confirm the sei go-ethereum fork's Apply rejects both (this could not be byte-verified in the review environment). - Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output. REVIEW_GUIDELINES.md was also empty, so no repo-specific guidelines were applied.
- Minor/pre-existing:
CreateAccountskipsclearAccountState(and thus the new override-drop) whens.ctx.IsTracing(), so a CREATE2 re-creation of a masked account duringdebug_traceCallwould keep reading the frozen overlay instead of empty storage. This matches the existing documented "tracing could be incorrect in theory" caveat rather than being newly introduced, but is worth noting since the new overlay extends that surface. - No state-layer test exercises the
stateDiff(partial override, un-masked account) path or theOnStorageChangetracing hook on an overridden slot; consider adding coverage. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if len(account.State) > maxSlots { | ||
| return fmt.Errorf("state override for %s has too many slots (%d > %d)", addr.Hex(), len(account.State), maxSlots) | ||
| } | ||
| if len(account.StateDiff) > maxSlots { |
There was a problem hiding this comment.
[nit] State and StateDiff slot counts are checked independently, so an account can pass with up to maxSlots in each (2x the intended per-account bound). In standard go-ethereum these two fields are mutually exclusive (Apply errors if both are set), which makes this unreachable in practice — but if you want defense-in-depth against a fork that applies both, cap the combined len(account.State) + len(account.StateDiff) (or the union) instead of each field separately.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Solid, well-tested fix that replaces the storage-purging state-override implementation with a simulation-local overlay, correctly journaling overlay writes/removals and deep-copying on Copy/revert. No blocking issues found; a few minor backward-compatibility and edge-case notes.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Backward-compat: the new default caps (max_state_override_accounts=100, max_state_override_slots=1000) reject overrides that were previously accepted (they were effectively unbounded before). Worth calling out in release/upgrade notes so operators who rely on large overrides can raise the limits.
- validateStateOverrides checks len(account.State) and len(account.StateDiff) independently against maxSlots. Since go-ethereum treats State and StateDiff as mutually exclusive this is fine in practice, but if the fork ever allowed both, an account could carry up to 2*maxSlots overridden slots. A combined check (or an explicit assumption comment) would be more defensive.
- Tracing edge case (pre-existing): CreateAccount only calls clearAccountState when !ctx.IsTracing(), so during tracing a recreated/colliding account keeps its frozen storage override instead of reading empty. This matches the existing documented 'tracing could be incorrect in theory' caveat, but the new overlay extends that caveat to storage overrides.
- Second-opinion passes produced no actionable findings: codex-review.md reported no material findings (and could not run Go tests due to a sandboxed network); cursor-review.md was empty. REVIEW_GUIDELINES.md was also empty.
2 participants
Summary
The
state/stateDiffstate overrides accepted byeth_call,eth_estimateGas, andeth_estimateGasAfterCallswere applied by clearing the target account's persisted contract storage (SetStorage→clearAccountState) and re-writing the override slots. During simulation this mutated the working state's view of a contract's storage, so slots that were not named in the override read back as empty instead of retaining their persisted values.This PR replaces that purge-and-rewrite behavior with a simulation-local storage overlay that masks the account's storage for the duration of the simulation, and adds configurable caps on override size.
Changes
x/evm/state):SetStoragenow installs a per-accountstorageOverridein the transaction'sTemporaryStateinstead of purging persisted storage. The overlay keeps acommittedview (frozen at override time) and acurrentview (mutable during execution). While an account is masked:GetState/GetCommittedStateread from the overlay; slots not present in the override read as zero rather than from the persisted store.SetStatewrites to the overlay'scurrentview and journals the change.storageOverrideChangeso overlay writes are correctly rolled back on EVM snapshot revert.TemporaryState.DeepCopycopies the overlay maps so snapshots remain independent.clearAccountCodeAndNonce(still used to clear code/nonce when masking storage on an existing account).evm.max_state_override_accounts(default 100) andevm.max_state_override_slots(default 1000) config options, wired throughConfig, the backend, and both HTTP and WebSocket servers.validateStateOverridesrejects oversized overrides up front on all three simulation endpoints.Testing
x/evm/state/state_test.go— overlay masking, zero reads for unset slots, and journal revert behavior.evmrpc/simulate_test.go/evmrpc/state_override_validate_test.go— override size validation.evmrpc/config/config_test.go— config parsing for the new options.