perf(evm): memoize deliver GetCode (+ giga); fix pointer upsert Multistore write target (CON-361) - #3843
Conversation
Cache bytecode within deliver StateDB transactions to avoid repeated KV GetCode reads on CALL-family paths. Leave codeCache nil for simulation/RPC. Keep GetCodeSize == len(GetCode) coverage on keeper and StateDB aligned with EIP-7702 size gates and Sei's separate code-size metadata. Co-authored-by: Cursor <[email protected]>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3843 +/- ##
==========================================
- Coverage 61.52% 60.67% -0.86%
==========================================
Files 2360 2268 -92
Lines 199370 188967 -10403
==========================================
- Hits 122661 114653 -8008
+ Misses 65749 64205 -1544
+ Partials 10960 10109 -851
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 memoization is carefully wired for snapshot revert, account recreate, Copy(), and the simulation/RPC opt-out, and the new invariant tests are a welcome addition — but the cache is only kept coherent by DBImpl.SetCode, and a production deliver-tx path (UpsertERCPointer → k.SetCode, reachable from the pointer precompile) writes code straight through the keeper, so GetCode can return stale bytecode while GetCodeSize/GetCodeHash return fresh values — breaking the very invariant this PR pins.
Findings: 4 blocking | 9 non-blocking | 7 posted inline
Blockers
- Cache-coherency hole makes this change observable in deliver-tx execution, which conflicts with the
non-app-hash-breakinglabel: afterUpsertERCPointer's existing-pointer branch writes code viak.SetCode, a laterCALL/EXTCODESIZE/EXTCODEHASHon that pointer in the same tx produces different results than before this PR. Either make the memo fully transparent (invalidate/write-through on every keeper code write) or gate the behavior change; as written the memo is not semantics-preserving. - No test covers the failure mode that the fix needs to close: add a regression test that exercises the pointer-precompile upsert path (or, minimally,
k.SetCodebypass followed byGetCode/GetCodeSizeon a deliver statedb) and asserts they stay consistent. Today the only test in that area (x/evm/state/code_test.go:152) asserts the stale value is returned. - 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review is the merge of Codex's single finding and my own pass. - No benchmark or measurement is included to back the perf claim. The defensive copy means the first
GetCodefor every address is now strictly more expensive than before (up to a 24KB alloc+copy), so the change is only a win when an address's code is read at least twice in a tx. The repo has abenchmark/harness — a before/after number on a CALL-heavy tx would justify the added state and the copy. Cleanup()clears the cache but no test pins that; consider coveringCleanup/CleanupForTracer/ResetForTracerclearing alongside the existing revert test.- The keeper-level test (
x/evm/keeper/code_size_invariant_test.go) shares oneaddrand onectxacross all subtests, so later subtests (overwrite shrinks size metadata,empty slice clears to zero size) silently depend on earlier ones having run. Fine today since subtests are sequential, but a fresh address per subtest would make each independently meaningful. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
Comments that couldn't be anchored to the diff
x/evm/state/code.go:63-- [suggestion]GetCodeSize(andGetCodeHashabove) still read the store on every call whileGetCodeis memoized, which is what lets the two drift apart in the scenario above, and it also leaves theEXTCODESIZE/EIP-7702 size-gate path un-optimized even though the bytes are already in memory. After invalidation is fixed, consider serving size from the cached entry (len(cached)) — same result by construction, one less store read, and the pinned invariant becomes structurally true rather than incidentally true.
| s.codeCache[addr] = nil | ||
| return nil | ||
| } | ||
| cached := make([]byte, len(code)) |
There was a problem hiding this comment.
[nit] make+copy here and again at lines 56-58 with the same empty→nil normalization; bytes.Clone(code) (nil-safe, returns nil for empty input) collapses both branches to a one-line helper, e.g. func (s *DBImpl) memoCode(addr common.Address, code []byte).
| journal: []journalEntry{}, | ||
| coinbaseEvmAddress: feeCollector, | ||
| } | ||
| if !simulation { |
There was a problem hiding this comment.
[suggestion] !simulation is broader than the field comment on lines 26-31 suggests: it also enables the cache for the ante/CheckTx statedbs (x/evm/ante/fee.go:73, app/ante/evm_checktx.go:302) and for BeginBlock/EndBlock (x/evm/keeper/abci.go:41,56) and the one-off pointer EVM (x/evm/keeper/pointer_upgrade.go:20). Harmless for the ante paths (they only touch balance/nonce), but the comment reads as "deliver only" and the pointer-upsert case is where the coherency problem lives — worth stating the actual set.
| s.tempState = nil | ||
| s.logger = nil | ||
| s.snapshottedCtxs = nil | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] Cleanup nils out every other field (tempState, logger, snapshottedCtxs); s.codeCache = nil would match that style and also release the map's buckets, whereas clear retains them. (It also keeps the "nil means disabled" invariant intact for a post-Cleanup statedb instead of leaving an enabled-but-empty cache behind.)
| } | ||
| // Deliver copies start with an empty cache (reload on demand). If the parent | ||
| // had caching disabled (nil), keep it disabled — do not allocate. | ||
| if s.codeCache != nil { |
There was a problem hiding this comment.
[suggestion] Worth calling out in the comment: starting the copy with an empty map changes Copy() semantics, because the child's cache now shadows writes the parent makes to the shared underlying store after the copy was taken (code_test.go:131 pins this — child sees initial where pre-PR it would read through to updated). Latent today since no caching-enabled statedb calls Copy() (only evmrpc/block_trace_profiled.go:238, which is simulation=true), but it's a behavior change waiting for the first deliver-path Copy() caller.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Keep deliver codeCache coherent when UpsertERCPointer redeploys an existing pointer; DBImpl.SetCode already updates the keeper store. Co-authored-by: Cursor <[email protected]>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
The deliver-only codeCache is soundly scoped and its invalidation set covers every in-tx writer of the code prefixes that I could find, and the new tests are a genuine improvement in coverage of the size/len invariant. No blocking correctness or security issue; findings are a comment/behavior mismatch on the returned slice, a behavior change in the pointer-upgrade error path, and some latent-invariant hardening/documentation.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Latent staleness across nested StateDBs:
Keeper.CallEVM(x/evm/keeper/evm.go:100) builds a second deliver-modeDBImplover the same multistore as an outer liveDBImpl(reachable as EVM tx → wasmd precompile → CW →HandleInternalEVMCall). ItsFinalize()flushes code writes into the outer statedb's store without touching the outercodeCache. Today the outer frame is just the top-level precompile call and does noGetCodeafterwards, so it isn't observable — but nothing enforces that. Worth a comment onCallEVM(or invalidating the parent) so a future change doesn't quietly turn this into a wrong-code-executed bug. WithCtx(x/evm/state/statedb.go:273) swaps the whole ctx/multistore but leavescodeCachepopulated. OnlyevmrpcPrepareTx/PrepareTxNoFlushcall it, and those statedbs aresimulation=true(nil cache), so it's currently safe. A one-lineclear(s.codeCache)inWithCtxwould make that safety structural rather than incidental.GetCodeSize/GetCodeHashstill hit the KV store on every call whileGetCodeis memoized. Given this PR explicitly pinsGetCodeSize(addr) == len(GetCode(addr)), servingGetCodeSizefrom a warm cache entry would both extend the perf win to EXTCODESIZE (arguably the hotter opcode) and make the pinned invariant structural instead of test-enforced.- Test coverage gaps for the invalidation contract itself: no test exercises the nested-
CallEVMscenario above, and no test asserts that asimulation=truestatedb genuinely leavescodeCachenil (the current test only observes store-follow behavior, which a correctly-invalidated cache would also satisfy). A benchmark backing theperf(evm)claim would also help justify the added invalidation surface. - The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| if s.codeCache == nil { | ||
| return code | ||
| } | ||
| // Cache a copy so callers cannot mutate the keeper/store-backed slice into |
There was a problem hiding this comment.
[suggestion] The comment doesn't match what the code guarantees. Copying decouples the memo from the store-backed slice, but GetCode then hands that same memo slice back to callers (line 34) — so a caller that mutates the returned slice does corrupt the tx memo, which is exactly what the comment claims is prevented. Before this PR every GetCode returned a fresh store read, so mutation was harmless.
No current caller mutates it (geth's EXTCODECOPY/CODECOPY go through getData, and vm.Contract.Code is read-only), so this isn't a live bug. Either reword to state the actual contract ("callers must treat the result as read-only") or return a copy on the hit path.
| } | ||
|
|
||
| // Store was rewound via CacheMultiStore; drop any cached code that may now be stale. | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[suggestion] Correct and appropriately conservative, but blunt for a perf-motivated change: every reverted CALL frame (including ordinary require-style reverts) wipes the whole memo, so a revert-heavy tx may see little of the intended benefit. Since SetCode already knows which addresses it touched, a journal entry recording the pre-SetCode address would let the revert drop only the affected keys. Fine to defer, but consider noting the tradeoff in the comment so it isn't mistaken for a hard requirement.
| // for the statedb lifetime. Non-nil (deliver) is cleared on snapshot revert | ||
| // and Cleanup, and kept in sync by SetCode. Copy() starts empty when the | ||
| // parent had caching enabled. | ||
| codeCache map[common.Address][]byte |
There was a problem hiding this comment.
[nit] The doc comment gives memory retention as the reason simulation/RPC/trace DBs stay nil, but there's a second, load-bearing reason worth recording: with a nil cache GetCode performs no writes to DBImpl, which is what keeps the parallel trace path safe (block_trace_profiled.go:238 hands statedb.Copy() to worker goroutines, and ResetForTracer's own comment documents concurrent reads of copies). Flipping any of those construction sites to simulation=false would introduce a concurrent map write, and nothing here says so.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
The codeCache staleness bug I flagged in my previous review (UpsertERCPointer's redeploy path bypassing DBImpl.SetCode) is fixed in the latest commit — the write now routes through state.GetDBImpl(evm.StateDB).SetCode, and pointer_upgrade_test.go/code_test.go were updated to assert the corrected (coherent) behavior instead of pinning the stale-cache read. I also checked for other direct k.SetCode(ctx, ...) call sites in x/evm/keeper that could bypass a live deliver cache — the only remaining one is the nil-DBImpl fallback branch in the same function, which is not reachable while a caching StateDB is live.
Extended reasoning...
Verified the fix commit (write pointer redeploy code through StateDB.SetCode) directly addresses the blocker I previously reported: UpsertERCPointer's exists-branch now calls sdb.SetCode(contractAddr, ret) when a DBImpl is live, keeping codeCache coherent with the keeper store. The previously-mispinning test in code_test.go (which asserted a keeper-level write stays invisible to a warm deliver cache) was corrected to assert the write is now visible. New coverage (TestUpsertERCNativePointerKeepsCodeCacheCoherent) exercises the exact mid-tx redeploy scenario from my original report. No other direct keeper SetCode callers remain reachable against a live deliver statedb.
Write redeploy bytecode via the StateDB Multistore with the caller's gas meter, then RefreshCodeCache so deliver memo stays coherent without under-charging the precompile path. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
The codeCache memo is invalidated on SetCode, snapshot revert, account clear, and the pointer-redeploy path, but not for writes made by a nested DBImpl (EVM → wasmd precompile → CW → MsgInternalEVMCall → Keeper.CallEVM), whose Finalize() flushes directly into the outer statedb's live store layer — leaving the outer GetCode stale while GetCodeSize/GetCodeHash stay fresh. The rest of the change (including the genuine frozen-layer fix in UpsertERCPointer) looks sound and is well covered by the new tests.
Findings: 1 blocking | 9 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
GetCodeSizestill hits the KV store on every call even whencodeCacheholds the bytecode. Since the PR's stated motivation is the EIP-7702 CALL-family size gate (which goes throughGetCodeSize), returninglen(cached)on a hit would both complete the optimization and make the newly-pinnedGetCodeSize == len(GetCode)invariant structurally guaranteed rather than incidental on two independent store reads.- The new tests verify cache coherence but nothing verifies the cache is actually used — every assertion would still pass with
codeCacheremoved entirely (e.g. the "served from the tx code cache" comment inTestCodeCacheHitAndSetCodeUpdateis not actually distinguishable from a store read). A keeper read counter/spy, or a benchmark inx/evm/state, would make the perf claim in the PR title verifiable and guard against a future regression that silently disables the memo. TestCodeCacheDisabledForSimulationis misnamed: roughly two thirds of it exercises deliver-DB behavior (memo refresh onSetCode,RefreshCodeCacheafter a keeper write) and only the last three lines test the simulation DB. Worth splitting intoTestCodeCacheRefreshOnKeeperWriteandTestCodeCacheDisabledForSimulation.- The invariant "any keeper-level code write against a ctx in a live
DBImpl's store chain must callRefreshCodeCache" is now load-bearing but only documented in a comment at the one call site. A doc comment onKeeper.SetCodepointing atDBImpl.RefreshCodeCachewould make the requirement discoverable for the next caller. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only Codex (which reported no material issues and could not run tests) plus my own analysis. Neither automated pass was able to execute the PR's test plan in-sandbox, so CI green on this PR is the only evidence the new tests pass. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) | ||
| return s.codeCache[addr] |
There was a problem hiding this comment.
[suggestion] Two small things on this return:
- Outbound aliasing. The comment above covers inbound aliasing ("store a copy so the memo is not aliased to the keeper/store-backed slice"), but the memo slice is now handed out to every caller — previously each
GetCodereturned a fresh store read, so a mutation was contained to one caller. go-ethereum treatsContract.Codeas read-only today (AnalyseCodebuilds a separate bitvec), so this is fine in practice, but it's now a load-bearing assumption. Worth stating it in the comment so nobody adds an in-place transform later. - Redundant map lookup.
putCodeCachealready built the slice; having it return the stored value (return s.putCodeCache(addr, code)) avoids the second hash ofaddron every cold read, on a path whose whole point is shaving per-call cost.
| s.tempState = nil | ||
| s.logger = nil | ||
| s.snapshottedCtxs = nil | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] clear() empties the map but keeps its bucket array alive, while every neighbouring line in Cleanup() nils its field precisely so the GC can reclaim it — and this is the one field that can be holding tens of KB per touched contract. s.codeCache = nil matches the intent here. (CleanupForTracer/ResetForTracer are different: those reuse the DBImpl, so clear() is correct there because it preserves the enabled/disabled distinction.)
There was a problem hiding this comment.
No new bugs found in the current revision. This PR touches core EVM state/gas-metering code (deliver-tx code cache coherence and pointer-upgrade gas billing), and given the history of real blocking issues raised and fixed across the last two commits on this exact code path, I'd like a human to confirm the final combined fix (routing the store write through sdb.Ctx() while billing gas via ctx.GasMeter(), plus RefreshCodeCache) before merge.
Extended reasoning...
Overview
This PR memoizes bytecode reads in the deliver-tx StateDB (codeCache in x/evm/state/statedb.go/code.go) to avoid re-reading the KV store on repeated GetCode calls within a CALL-heavy transaction, and adds tests pinning GetCodeSize(addr) == len(GetCode(addr)). It also touches x/evm/keeper/pointer_upgrade.go's UpsertERCPointer redeploy path, since that is the one place in production code that writes bytecode straight through the keeper while a live, caching-enabled deliver StateDB exists.
Security / correctness risk
The main risk class here is state-coherence, not a classic security vuln: if the new codeCache memo diverges from the underlying KV store within a single transaction, a contract could execute stale bytecode after a mid-tx pointer upgrade, which is exactly what my own earlier review round on this PR flagged as a blocker (x/evm/keeper/pointer_upgrade.go's exists branch calling k.SetCode directly, bypassing DBImpl.SetCode). A second review round (cursor[bot]) then found that the fix for that (routing the write through the StateDB) introduced a gas-undercharge, since it billed against the StateDB's own meter instead of the precompile's finite caller meter. The current head (e8f3cb2) addresses both by writing through sdb.Ctx().WithGasMeter(ctx.GasMeter()) (correct store, correct billing) and explicitly calling sdb.RefreshCodeCache to keep the memo in sync. I traced this through Keeper.SetCode/PrefixStore and the precompile ctx derivation (pcommon.Precompile.Prepare) and the combined fix looks correct to me, but this is exactly the kind of subtle cross-cutting fix (store routing + gas metering + cache invalidation, three properties that all have to hold together) that benefits from a second set of eyes given it's consensus-critical EVM code.
Level of scrutiny
High. This is core EVM state/gas code that runs on every deliver-tx CALL and on pointer contract upgrades; correctness bugs here can cause execution divergence or gas-metering discrepancies across nodes. The PR is labeled `non-app-hash-breaking", but the review thread itself surfaced a real state-behavior change (RunWithOneOffEVMInstance's error-path write semantics) worth a maintainer's explicit sign-off.
Other factors
Test coverage was added for the cache hit/invalidation paths (revert, cleanup, account recreation, Copy semantics) and a dedicated TestUpsertERCNativePointerKeepsCodeCacheCoherent" test now exercises the exact mid-tx redeploy scenario that was previously broken. Several non-blocking suggestions/nits from prior review rounds (comment wording, bytes.Clone" simplification, per-address journal instead of whole-cache-clear-on-revert) remain open but are explicitly optional per those same reviewers.
Wasmd-entry can nest a CallEVM DBImpl that Finalizes into the same Multistore; leaving the outer memo nil avoids a stale GetCode until wasm is decommissioned. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
The codeCache memo itself is carefully scoped (nil for simulation/RPC/trace and wasmd-entry, cleared on revert/cleanup, invalidated on account recreate) and well covered by new tests, but the PR also quietly changes where pointer bytecode is written in UpsertERCPointer — a state-transition change on a user-reachable precompile path that is unrelated to memoization and rides in under a non-app-hash-breaking label. Secondary concerns: GetCodeSize/GetCodeHash still read the store while GetCode is memoized, so the invariant this PR claims to pin depends on every future bypass write remembering to call RefreshCodeCache.
Findings: 2 blocking | 12 non-blocking | 7 posted inline
Blockers
x/evm/keeper/pointer_upgrade.gochanges the store layer that pointer bytecode is written to on the pointer re-upsert path. This is a state-transition behavior change on a user-reachable precompile (addNativePointer/addCW20/... for an already-registered pointee), independent of the memoization the PR is about. It needs an explicit app-hash-breaking determination (and, if breaking, an upgrade gate) rather than shipping under the currentnon-app-hash-breakinglabel — or it should be split into its own PR. See the inline comment for the mechanism.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty); Codex reported no material findings. So this synthesis rests on one independent pass. - Memory: the memo retains a second copy of every contract's bytecode for the whole tx, on top of the copy cachekv already holds from the store read. It is unbounded in the number of contracts a tx touches (worst case ~24 KiB × thousands of cold accounts within one tx's gas budget). Consider a size/entry cap, or skipping the defensive copy in
SetCode(geth hands over a freshly-allocated slice there). - No benchmark numbers are attached. Cosmos
cachekvalready memoizes reads within a tx, so the saving here is prefix-key construction, gas metering, and cachekv map lookups — not actual store I/O. Given the coherence surface this adds, a benchmark showing the win on a CALL-heavy workload would justify it. Cleanup()usesclear(s.codeCache), which keeps the map's bucket array alive. SinceCleanupis terminal (it nilstempState),s.codeCache = nilwould actually release the retained bytecode.giga/deps/xevm/state/code.gocarries a parallel copy ofx/evm/state/code.gothat was not updated. Worth confirming that divergence is intentional.TestCodeCacheDisabledForSimulationAndWasmdEntryalso asserts deliver-pathRefreshCodeCachebehavior; the name undersells what it covers.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // RefreshCodeCache updates the deliver-tx code memo after a keeper store write | ||
| // that bypassed SetCode (so gas can be charged against a different ctx meter). | ||
| func (s *DBImpl) RefreshCodeCache(addr common.Address, code []byte) { | ||
| s.putCodeCache(addr, code) |
There was a problem hiding this comment.
[suggestion] GetCode is now memoized but GetCodeSize (line 72) and GetCodeHash (line 10) still read the store, so the very invariant this PR adds tests for (GetCodeSize(addr) == len(GetCode(addr))) now depends on every keeper-level code write remembering to call RefreshCodeCache. RefreshCodeCache is opt-in and easy to forget: a future k.SetCode(sdb.Ctx(), ...) anywhere silently leaves GetCode stale while size/hash are fresh, and no existing test would catch it.
Serving size from the memo when it's warm closes the divergence by construction and extends the win to EXTCODESIZE:
func (s *DBImpl) GetCodeSize(addr common.Address) int {
s.k.PrepareReplayedAddr(s.ctx, addr)
if s.codeCache != nil {
if code, ok := s.codeCache[addr]; ok {
return len(code)
}
}
return s.k.GetCodeSize(s.ctx, addr)
}Alternatively, route all code writes through DBImpl.SetCode so the bypass hook isn't needed at all.
| // warm outer memo would then disagree with store. Leave nil until wasm is | ||
| // decommissioned and that nest path is gone. Nested CallEVM itself clears | ||
| // EVMEntryViaWasmdPrecompile before NewDBImpl, so the inner DB still memos. | ||
| if !simulation && !ctx.EVMEntryViaWasmdPrecompile() { |
There was a problem hiding this comment.
[suggestion] The gating reasoning holds today, but only because of a guard in a different file: Keeper.CallEVM rejects EVM->CW->EVM unless ctx.EVMEntryViaWasmdPrecompile() is set, and PrepareCtxForEVMTransaction sets that flag only when tx.To() is the wasmd precompile. So an ordinary EVM tx that internally CALLs the wasmd precompile keeps a warm memo, and its CW callback into CallEVM errors out rather than nesting a second deliver DBImpl. If that guard is ever relaxed, this memo goes stale silently with no test coverage. Worth naming CallEVM's guard explicitly in this comment (and/or clearing the memo in CallEVM defensively) so the coupling is discoverable from both sides.
| } | ||
|
|
||
| // Store was rewound via CacheMultiStore; drop any cached code that may now be stale. | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[suggestion] Correct but blunt: reverting one deep frame discards the memo for every address in the tx. Reverting the CacheMultiStore only discards writes, so only addresses whose code was written since rev can be stale. Contracts that probe with reverting sub-calls (try/catch, staticcall probing) will repeatedly nuke and re-fill the cache, which can make revert-heavy txs slower than the pre-PR baseline (extra make+copy per re-fill). A journal entry per code write would make invalidation precise and fits the existing journalEntry/revert machinery already used here.
| // nil codeCache means caching is disabled (simulation/RPC/trace/wasmd-entry); never allocate here. | ||
| if s.codeCache != nil { | ||
| if code, ok := s.codeCache[addr]; ok { | ||
| return code |
There was a problem hiding this comment.
[nit] The cache-hit path hands the caller the memo's own slice, so every GetCode in the tx shares one mutable backing array. putCodeCache copies on the way in, which suggests aliasing was a concern — it applies on the way out too. geth's interpreter doesn't mutate contract code, so this is latent rather than live, but a HookedStateDB hook consumer or precompile that writes into the returned slice would now corrupt every subsequent read in the tx instead of just its own copy.
| updated := []byte{9, 9, 9} | ||
| parent.SetCode(addr, updated) | ||
| require.Equal(t, updated, parent.GetCode(addr)) | ||
| require.Equal(t, initial, child.GetCode(addr)) |
There was a problem hiding this comment.
[nit] This assertion pins a semantic change worth calling out: after Copy(), the child no longer observes parent code writes for addresses it has already warmed (without the memo it would read updated through the parent's CacheMultiStore). Harmless today — the only DBImpl.Copy() caller is the parallel block tracer in evmrpc/block_trace_profiled.go, whose statedbs are built with simulation=true and so have a nil memo — but the comment on line 124 ("child loads from Multistore then caches") reads as if fall-through still works. Worth stating in the Copy() doc that a deliver copy gets a point-in-time code view.
There was a problem hiding this comment.
I didn't find any new bugs — the three follow-up commits (273b946, e8f3cb2, 0d35c5e) appear to correctly resolve the blockers raised earlier in this thread (stale pointer-redeploy cache, gas undercharge, and the nested wasmd-entry CallEVM reentry). Given the complexity of this change and its history of subtle correctness issues in consensus-critical deliver-tx code, I'd still like a human to sign off before merge.
Extended reasoning...
Beyond the previously-reported findings, I traced the EVMEntryViaWasmdPrecompile gating (x/evm/keeper/msg_server.go, x/evm/keeper/evm.go) to check whether the 0d35c5e fix (disabling codeCache when this flag is set) actually closes the nested-DBImpl staleness hole seidroid flagged. It does: CallEVM refuses to run at all unless ctx.EVMEntryViaWasmdPrecompile() is true, and that flag is only ever set true when the top-level EVM tx target is the wasmd precompile (PrepareCtxForEVMTransaction) — so the only statedb that can have a nested nested CallEVM DBImpl flush writes into its store is exactly the one whose cache this PR now disables. The nested CallEVM statedb itself resets the flag to false and cannot recurse further, so it never becomes the target of the same aliasing problem.
Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
The codeCache memo is soundly scoped (nil for simulation/RPC/trace and wasmd-entry deliver txs, fresh map on Copy(), cleared on revert/cleanup, deleted on account clear), and the pointer_upgrade.go change fixes a real store-layer mismatch. I found no live correctness bug, but the whole design rests on an unenforced invariant — every keeper-level code write during a memo-enabled deliver tx must refresh the memo — and the test suite doesn't actually prove the memo is consulted, so there's no regression guard.
Findings: 0 blocking | 15 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty, so that pass produced no output. Codex (codex-review.md) reported "No material issues found." Findings below are mine alone. - No test proves the memo is actually consulted. Every "cache hit" assertion in
code_test.gopasses identically withcodeCacheremoved (the store returns the same bytes); only the sim/wasmd negative assertions and oneCopy()assertion depend on the memo existing.state.EVMKeeperis an interface (x/evm/state/expected_keepers.go:13), so a counting fake that recordsGetCodecalls would let you assert exactly one store read per address per tx — that's both the proof of the perf win and the regression guard if someone accidentally disables it. - This is a perf PR with no measurement in the description. The repo has a
benchmark/harness (seebenchmark/CLAUDE.md); a before/after number on a CALL-heavy workload would justify the added invalidation surface and let a reviewer weigh it against the coarse revert invalidation. - The correctness invariant introduced here — "any keeper-level code write during a memo-enabled deliver tx must refresh the memo, or
GetCodesilently diverges from the store and fromGetCodeSize" — belongs inx/evm/AGENTS.md, which already has a "StateDB Bridge" section (line 64) but says nothing about code caching. Without it, the next contributor adding ak.SetCodecall site has no signal. - No test exercises the real nested wasmd path.
TestCodeCacheDisabledForSimulationAndWasmdEntryfabricates the flag withctx.WithEVMEntryViaWasmdPrecompile(true)rather than driving an actual EVM tx → wasmd precompile → CW →CallEVMnest. Since that nest is the sole reason the memo is disabled for wasmd-entry txs, an end-to-end test would pin the behavior against future changes toPrepareCtxForEVMTransaction/CallEVM. - The PR carries the
app-hash-breakinglabel but contains no upgrade/version gate. The state-visible change is inUpsertERCPointer: which cache layer receives the pointer bytecode, and therefore whether the write survives a failed redeploy (previously it landed inctxand could persist even when the one-off runner errored and skippedFinalize; now it is dropped with the statedb). Please confirm this is covered by a coordinated upgrade-height rollout rather than needing a code-level gate. giga/deps/xevm/stateis a parallel copy of this StateDB (its ownNewDBImpl,Copy,CleanupForTracer,code.go) and was not updated. Correctness-wise that's fine — no memo means no staleness — and its keeper has no pointer-upsert path, so thepointer_upgrade.gochange can't diverge between the two engines. Worth confirming the omission is deliberate rather than a missed sync.- 8 suggestion(s)/nit(s) flagged inline on specific lines.
| // billing KV gas to the caller's meter (finite precompile meter in deliver). | ||
| if sdb := state.GetDBImpl(evm.StateDB); sdb != nil { | ||
| k.SetCode(sdb.Ctx().WithGasMeter(ctx.GasMeter()), contractAddr, ret) | ||
| sdb.RefreshCodeCache(contractAddr, ret) |
There was a problem hiding this comment.
[suggestion] This call site is now load-bearing for consensus: a keeper-level code write during a memo-enabled deliver tx that forgets RefreshCodeCache leaves GetCode returning stale bytecode while GetCodeSize reads the store — breaking the very invariant the new tests pin, and diverging the app hash.
I checked the tree and this is the only such site today (the others are x/evm/genesis.go:20 and app/eth_replay.go:156, neither of which has a live statedb). But RefreshCodeCache is exported and silently no-ops, so nothing stops a future call site from getting it wrong. Consider folding both operations into one keeper helper — e.g. k.SetCodeWithMeter(sdb, addr, code, meter) that resolves the ctx, writes, and refreshes — so the pairing can't be split.
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) | ||
| return s.codeCache[addr] |
There was a problem hiding this comment.
[suggestion] putCodeCache defensively copies on write, but this returns the cache-owned slice on every read (as does the cache-hit branch at line 18). The defense is therefore one-sided: a caller that mutates the returned bytes poisons the memo for the rest of the tx, and unlike before the PR that corruption now survives subsequent reads.
go-ethereum treats GetCode results as read-only, so this isn't a live bug. But pick one side and document it: either copy on read too, or drop the copy-on-write. Note that the underlying cachekv layer already retains its own copy of read values, so the write-side copy roughly doubles bytecode retention per touched contract without buying isolation from the store.
| } | ||
| // Keep empty code as nil to match Keeper.GetCode, and store a copy so the | ||
| // memo is not aliased to the keeper/store-backed slice. | ||
| if len(code) == 0 { |
There was a problem hiding this comment.
[nit] This empty-code branch duplicates the identical len(code) == 0 handling already inside putCodeCache, and the miss path then does a redundant third map operation (putCodeCache stores, line 32 reads back). Having putCodeCache return the stored slice collapses lines 22–32 to:
if s.codeCache == nil {
return code
}
return s.putCodeCache(addr, code)One fewer place for the nil/empty normalization to drift out of sync.
|
|
||
| func (s *DBImpl) GetCodeSize(addr common.Address) int { | ||
| s.k.PrepareReplayedAddr(s.ctx, addr) | ||
| return s.k.GetCodeSize(s.ctx, addr) |
There was a problem hiding this comment.
[suggestion] GetCodeSize (and GetCodeHash above) still hit the KV store on every call, so EXTCODESIZE/EXTCODEHASH-heavy contracts and delegation resolution get no benefit from the memo — the hot path is only partly covered.
The new tests pin exactly the invariant that would let this be served from a warm memo (GetCodeSize(addr) == len(GetCode(addr))), so it's a natural follow-up. Worth being explicit either way: if you're deliberately leaving size/hash on the store as a cross-check against memo staleness, that's a reasonable call and a one-line comment here would record it, since otherwise the next reader will just assume it was an oversight.
| } | ||
|
|
||
| // Store was rewound via CacheMultiStore; drop any cached code that may now be stale. | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] Correct but very coarse: a single reverted inner frame — a probing CALL, a Solidity try/catch, a failed transfer — discards bytecode memoized for every contract in the tx, and the next call re-reads and re-copies all of them. Real CALL-heavy workloads revert inner frames routinely, which is the case this PR is optimizing for.
Since there's already a journal with watermarks, a codeChange entry (or clearing only addresses written since rev) would keep the memo warm across reverts. Fine as a first cut given the correctness stakes; worth a TODO so the coarseness is a recorded choice rather than a forgotten one.
| // warm outer memo would then disagree with store. Leave nil until wasm is | ||
| // decommissioned and that nest path is gone. Nested CallEVM itself clears | ||
| // EVMEntryViaWasmdPrecompile before NewDBImpl, so the inner DB still memos. | ||
| if !simulation && !ctx.EVMEntryViaWasmdPrecompile() { |
There was a problem hiding this comment.
[suggestion] This gate is subtler than the comment conveys, and the reason it's sufficient is non-local: CallEVM (x/evm/keeper/evm.go:82) rejects EVM->CW->EVM outright unless EVMEntryViaWasmdPrecompile is set, and the only place that sets it is PrepareCtxForEVMTransaction for a top-level tx.To() == wasmd. That's what rules out the case I initially worried about — an EVM tx to an ordinary contract that CALLs the wasmd precompile mid-execution, where this DB would have a warm memo but the flag is false. That call is rejected, so the memo can't go stale.
Worth citing evm.go:82 here explicitly, because the safety of this line depends entirely on that rejection staying in place. If anyone ever relaxes it, memoization silently becomes app-hash-breaking with no failing test to catch it.
| } | ||
| // Deliver copies start with an empty cache (reload on demand). If the parent | ||
| // had caching disabled (nil), keep it disabled — do not allocate. | ||
| if s.codeCache != nil { |
There was a problem hiding this comment.
[nit] Note this quietly changes Copy() semantics for any deliver-path copy: the child's ctx is a CacheMultiStore branched off the parent's, so before the PR a child read-through would observe the parent's later SetCode. Now, once the child has warmed an address, it won't — while storage, balance, and nonce still read through. TestCodeCacheCopyStartsEmptyAndIndependent enshrines that as intended.
Arguably closer to geth's snapshot semantics, and moot today since the only production Copy() caller is evmrpc/block_trace_profiled.go:238 on a simulation=true statedb (memo nil). But it makes copy isolation address- and read-order-dependent, which is a trap if a deliver-path Copy() ever appears. A sentence in the comment noting that only code is isolated would help.
Superseded: latest AI review found no blocking issues.
Avoid writing nil/revert-data over live pointer bytecode on a failed redeploy; cover with a gas-constrained preservation test. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
No new bugs found this run. The consensus-affecting issue I flagged in a prior review (codeCache going stale on pointer redeploy) has since been fixed and resolved in the thread — given the app-hash-breaking label and how many subtle staleness bugs this exact code path has already produced across review rounds, I'd still like a human to do a final pass over the four follow-up fix commits before merge.
Extended reasoning...
Overview
This PR memoizes EVM bytecode reads (codeCache) in the deliver-tx StateDB (x/evm/state/statedb.go, code.go, state.go) and adds a keeper/StateDB test pinning GetCodeSize(addr) == len(GetCode(addr)). It also touches x/evm/keeper/pointer_upgrade.go's pointer-redeploy path, which now routes its keeper-level SetCode through the live StateDB's Multistore/gas meter and refreshes the memo, instead of writing straight to the keeper's ctx.
Security risks
No injection/auth-bypass surface. The real risk here is consensus divergence: this PR is labeled app-hash-breaking, and the review thread (cursor bugbot, seidroid[bot], and my own prior pass) found multiple real staleness/gas-accounting bugs in earlier revisions — stale codeCache on pointer redeploy, gas undercharge from writing on the wrong meter, a nested wasmd-entry DBImpl re-entry path that could leave a parent memo stale, and a write-before-error-check ordering issue. All four are now addressed by the last four commits (273b946, e8f3cb2, 0d35c5e, eac208a), and the wasmd-entry fix has dedicated test coverage (TestCodeCacheDisabledForSimulationAndWasmdEntry).
Level of scrutiny
High. This is core EVM StateDB code on the hot deliver-tx path, gated by subtle invariants (cache coherence with the underlying Multistore across snapshot/revert, nested DBImpl re-entry, gas metering). The fact that several independent reviewers found real bugs in successive revisions of the same ~150 line diff is itself a signal that this class of change is easy to get subtly wrong, even after fixes land.
Other factors
No bugs were found by the bug hunting system in this run, and the fixes for prior findings look correct on inspection of the current tree. The three most recent commits (wasmd-entry gating, error-check ordering, no-op test cleanup) landed after the last human/bot comment in the thread, so no one has explicitly signed off on them yet — that, combined with the app-hash-breaking label, is why I'm deferring rather than approving.
There was a problem hiding this comment.
A well-tested code memo for deliver-tx GetCode whose invalidation surface I verified as complete — every CodeKeyPrefix mutation and every NewDBImpl/Copy() site is either covered or on a nil-cache path, and the EVMEntryViaWasmdPrecompile carve-out correctly closes the only reachable nesting hazard. No blockers; the notes are a mis-stated rationale on the (genuinely load-bearing) pointer_upgrade.go fix, an adjacent registry write that still uses the stale ctx, and two spots where the memo's copy-on-read and wipe-on-revert give back some of the perf it is meant to buy.
Findings: 0 blocking | 13 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
x/evm/keeper/pointer_upgrade.go:128—setter(ctx, pointee, contractAddr)still writes the pointer-registry entry through the capturedctx, i.e. the same layer the code write was just moved off of. If the frozen-empty-skip hazard is the reason for movingSetCode(see inline note at line 116), the registry write has identical exposure: a same-txgetter(ctx, pointee)on a second upsert, or any pointer lookup from a later precompile/contract call in the same tx, can read through a memoized skip that bypasses that layer and miss the new registration. Either move it tosdb.Ctx()too, or add a comment on why the registry write is safe where the code write was not.- A
perf(evm)PR with no numbers.Keeper.GetCodereads through a cachekv layer that already retains the value for the tx, so the memo's win is mainly avoiding the O(snapshot-depth) walk up the cache stack plus the prefix-key concat — real, but modest, and partly given back by the read-path copy and the full wipe on revert (inline notes oncode.go:65andstate.go:175). The repo hasbenchmark/; a CALL-heavy before/after would confirm the win and rule out a regression on revert-heavy txs. x/evm/AGENTS.md"StateDB Bridge → Key design choices" should gain a bullet for the code memo.nilmeans disabled (simulation/RPC/trace/wasmd-entry), it is invalidated on revert /CreateAccount/ destruct, and it is kept in sync bySetCodeplusRefreshCodeCache. That is now a load-bearing correctness invariant with a non-obvious rationale, and the guide already documents the snapshot/journal design next to it.- The
app-hash-breakinglabel with no version gate: perREVIEW_GUIDELINES.md§1/§2 I am not flagging the absence of a tag or handler. Worth an explicit confirmation, though, that the state-visible part of thepointer_upgrade.gochange is intended: moving the code write from the capturedctxlayer tosdb.Ctx()changes its revert scope, so a revert to the precompile's own snapshot level now discards the pointer bytecode where previously it survived. - Second-opinion passes: Codex reported "No material issues found in the pull request changes."
cursor-review.mdis empty, so the Cursor pass produced no output for this PR — no findings were merged from it. - 8 suggestion(s)/nit(s) flagged inline on specific lines.
| s.codeCache[addr] = nil | ||
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) |
There was a problem hiding this comment.
[suggestion] The len(code) == 0 branch just above duplicates logic putCodeCache already has (it stores nil for empty code), and return s.codeCache[addr] re-looks-up the key that was just written. Since Keeper.GetCode already normalizes empty to nil, the whole tail collapses to:
code := s.k.GetCode(s.ctx, addr)
s.putCodeCache(addr, code)
return codeOne behavioral path, one lookup, and the empty-vs-nil normalization lives in exactly one place.
| s.codeCache[addr] = nil | ||
| return | ||
| } | ||
| cached := make([]byte, len(code)) |
There was a problem hiding this comment.
[suggestion] On the read-miss path this copy is likely unnecessary, and it works against the PR's goal. Keeper.GetCode returns the slice the underlying cachekv layer already holds for the rest of the tx, so the memo copy roughly doubles retained bytecode per touched contract (up to MaxCodeSize each) and adds a memcpy to the exact path being optimized. Nothing in the EVM mutates a code slice in place, and pre-change GetCode already handed the store-owned slice to the VM.
Consider keeping the copy in SetCode (where code is caller-owned and could legitimately be reused) but storing the keeper's slice directly on the read miss — e.g. an internal putCodeCacheNoCopy, or have GetCode assign s.codeCache[addr] = code itself. Worth measuring alongside the benchmark, since the copy could plausibly eat the saved store lookup for large contracts.
| } | ||
|
|
||
| // Store was rewound via CacheMultiStore; drop any cached code that may now be stale. | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[suggestion] Correct but blunt: geth takes a snapshot per call frame and reverts on every failed sub-call, so revert-heavy txs (multicall, router try/catch, probing calls) will wipe the whole memo repeatedly and re-warm it from scratch — paying the putCodeCache copy again each time (see note on code.go:65). For those txs the memo can plausibly be net-negative versus the pre-change behavior.
A targeted invalidation would keep the win: this file already has a journal with revert(s) entries, so a codeChange entry (or a per-snapshot-level set of addresses written since that level) would let revert evict only what can actually be stale. Reads never need invalidating — only addresses written above the revert point do.
| // warm outer memo would then disagree with store. Leave nil until wasm is | ||
| // decommissioned and that nest path is gone. Nested CallEVM itself clears | ||
| // EVMEntryViaWasmdPrecompile before NewDBImpl, so the inner DB still memos. | ||
| if !simulation && !ctx.EVMEntryViaWasmdPrecompile() { |
There was a problem hiding this comment.
[suggestion] This guard is the load-bearing correctness invariant for the whole memo, but nothing tests the scenario it exists for. I traced it and the reasoning holds — the only nesting that reaches a second deliver DBImpl is CW→EVM→CW→EVM (flag set in PrepareCtxForEVMTransaction when tx.To() is the wasmd precompile), and evm.go:82 rejects the EVM→CW→EVM variant where the outer memo would be warm — but that conclusion depends on two call sites in another package that a future refactor could quietly change.
TestCodeCacheDisabledForSimulationAndWasmdEntry only simulates the staleness with a direct k.SetCode on wasmdEntry.Ctx(); it never runs a nested CallEVM. A regression test that actually drives the nest (or, cheaper, asserts NewDBImpl(ctx.WithEVMEntryViaWasmdPrecompile(true), k, false) leaves codeCache nil and that CallEVM still errors for IsEVM() && !EVMEntryViaWasmdPrecompile()) would pin the invariant to the thing that actually protects it.
| s.tempState = nil | ||
| s.logger = nil | ||
| s.snapshottedCtxs = nil | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] Cleanup nils out tempState, logger, and snapshottedCtxs, so clear is inconsistent here: it drops the entries but keeps the map's bucket array alive for as long as the DBImpl is referenced, which after a contract-heavy tx can be a non-trivial retained allocation. Since Cleanup is terminal (defer stateDB.Cleanup() in msg_server), s.codeCache = nil is safe and matches the surrounding lines. CleanupForTracer/ResetForTracer should keep using clear, since those reuse the DB.
| 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[:]) | ||
| if s.codeCache != nil { |
There was a problem hiding this comment.
[nit] The nil check is redundant — delete on a nil map is a documented no-op — and it reads inconsistently against the bare clear(s.codeCache) calls added in RevertToSnapshot, Cleanup, CleanupForTracer, and ResetForTracer. delete(s.codeCache, acc) unguarded is enough.
After GetDeploymentCode/Create freeze the Prepare Multistore layer, exists lookup, SetCode, and registry writes must target sdb.Ctx so same-tx readers do not skip them. Alias testutil/keeper as testkeeper for consistency. Co-authored-by: Cursor <[email protected]>
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 dce056c. Configure here.
There was a problem hiding this comment.
The GetCode memoization design is sound — I traced every store-code writer and every DBImpl construction site and the enable/disable gating (simulation, wasmd-entry, Copy, revert, CreateAccount, replay-prepare) is coherent. However, the PR also bundles an undocumented consensus-relevant change to UpsertERCPointer, where the exists-lookup is moved onto the StateDB's infinite gas meter, silently un-metering those reads and inflating suppliedGas in direct contradiction of the comment added right above it.
Findings: 1 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Scope/description mismatch: the PR summary only describes memoization plus tests, but the diff also changes gas metering and the store-write target for
UpsertERCPointer— a consensus-relevant path shared by everyprecompiles/pointer/legacy/v*version. That is presumably why theapp-hash-breakinglabel is set, but it should be called out explicitly in the description so reviewers and release notes don't miss it. - No measurements for a perf PR. The repo has a
benchmark/harness (seebenchmark/CLAUDE.md); before/after numbers for a CALL-heavy workload would substantiate the win and justify the added coherence surface. - Cursor's second-opinion pass (
cursor-review.md) is empty — that review produced no output, so only Codex's findings were available to merge. - Consider serving
GetCodeSizefrom the memo when it is warm (x/evm/state/code.go:70). Today it is a store read on every call even on a cache hit, and it would make theGetCodeSize == len(GetCode)invariant the PR is pinning structurally true rather than only test-asserted. TestCodeCacheHitAndSetCodeUpdate,TestCodeCacheMissHitFromPreSeededKeeperCode, and thedeliver/RefreshCodeCachesections ofTestCodeCacheDisabledForSimulationAndWasmdEntrywould all pass identically with the memo removed — they assert values, not provenance.TestCodeCacheCopyStartsEmptyAndIndependentis the only test that actually proves caching is active for deliver DBs. Not a gap in coverage of behavior, just worth knowing which test is load-bearing.TestCodeCacheCopyStartsEmptyAndIndependentpins the child returning staleinitialafter the parent writesupdatedto a store layer the child reads through. That is only safe becauseCopy()is currently reached solely fromevmrpc/block_trace_profiled.go:238, whose parent statedb issimulation=true(nil memo, so the copy is nil too). IfCopy()is ever used on a deliver DB in live execution this becomes a real divergence — worth a comment onCopy()recording that constraint.giga/deps/xevm/state/{code,statedb,state}.gocarries a parallelDBImplthat does not get the memo. If that tree is meant to trackx/evm/state, note whether the divergence is intentional.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| s.tempState = nil | ||
| s.logger = nil | ||
| s.snapshottedCtxs = nil | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[suggestion] Cleanup() nils out tempState, logger, and snapshottedCtxs but only clear()s the memo, leaving an allocated (possibly large-capacity) map bucket array reachable for as long as the DBImpl is. Setting s.codeCache = nil matches the surrounding fields and actually releases it; GetCode/putCodeCache already treat nil as "disabled", so nothing after Cleanup can repopulate it either — which is the desired post-cleanup state. (The clear() calls in CleanupForTracer/ResetForTracer are correct as-is, since those DBs are reused.)
| // nil codeCache means caching is disabled (simulation/RPC/trace/wasmd-entry); never allocate here. | ||
| if s.codeCache != nil { | ||
| if code, ok := s.codeCache[addr]; ok { | ||
| return code |
There was a problem hiding this comment.
[suggestion] On a hit this hands out the memo's own backing array, and every subsequent hit hands out the same array — whereas before the change each call returned a fresh slice from the store read. Any caller that writes into the returned slice now silently corrupts the memo for the rest of the tx. go-ethereum treats GetCode results as read-only today, so this is latent rather than broken, but given that putCodeCache goes out of its way to copy on the way in, the read-only contract on the way out deserves an explicit comment here (or a copy, if you'd rather not depend on caller discipline).
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) | ||
| return s.codeCache[addr] |
There was a problem hiding this comment.
[nit] Redundant second map lookup — putCodeCache just stored the copy. Having putCodeCache return the slice it cached lets this be return s.putCodeCache(addr, code) and also collapses the special-cased len(code) == 0 branch above (lines 27-30), which duplicates logic putCodeCache already implements.
| // Finite cosmos meter large enough for the pointer-registry getter reads, but | ||
| // small enough that GetDeploymentCode OOGs (normalizer is 1 in tests). StateDB | ||
| // KV metering stays on the infinite RunWithOneOff meter. | ||
| lowGasCtx := ctx.WithGasMeter(sdk.NewGasMeterWithMultiplier(ctx, 50_000)) |
There was a problem hiding this comment.
[nit] The 50,000 magic number makes this test sensitive to gas-schedule changes from both sides: raise the pointer-deployment cost and it stops being an OOG at all (require.Error fails); lower the budget relative to the registry reads and getter OOG-panics out of UpsertERCPointer before GetDeploymentCode is even reached, so the test would pass for the wrong reason. Deriving the limit from the observed successful-upsert cost (e.g. measure it in the first upsert, then use a fraction), or injecting a GetDeploymentCode failure directly, would pin the intended failure mode instead of a gas coincidence.
There was a problem hiding this comment.
No bugs found in this pass, but this PR shouldn't be auto-approved — it changes consensus-critical EVM state-transition behavior (labeled app-hash-breaking) and its correctness rests on subtle cachekv Freeze/Unfreeze and snapshot-layer reasoning.
What was reviewed: the deliver-tx codeCache memo and its invalidation points (SetCode, revert, Cleanup, account-recreate, Copy()), the wasmd-entry nesting guard that keeps a second deliver DBImpl from Finalizing stale code into a warm outer memo, and the UpsertERCPointer rewrite that moves writes onto the live unfrozen sdb.Ctx() instead of the frozen precompile-Prepare ctx. Several earlier blocker-level findings on these exact points (nested-DBImpl staleness, writes landing in a frozen layer, SetCode running before the deployment-code error check) were fixed across the latest commits.
A number of non-blocking suggestions from the review thread remain open — coarse whole-memo invalidation on every revert, GetCodeSize/GetCodeHash still bypassing the memo (so the invariant the new tests pin depends on every future keeper-level code write remembering RefreshCodeCache), and the event-manager/flag scoping implications of writing through sdb.Ctx().WithGasMeter(...) in the precompile path. None of these looked like correctness bugs to me, but given the stakes of getting this path wrong, I'd rather a human make the final call on whether they need to be addressed before merge.
Extended reasoning...
Overview
This PR adds a deliver-tx-scoped bytecode memo (codeCache) to x/evm/state/DBImpl so repeated GetCode calls avoid re-reading the KV store, plus tests pinning GetCodeSize(addr) == len(GetCode(addr)). Along the way it changes UpsertERCPointer (x/evm/keeper/pointer_upgrade.go) to read/write pointer bytecode and the pointer registry through the live sdb.Ctx() rather than the precompile's Prepare-time ctx, because GetDeploymentCode/Create snapshot and Freeze() that earlier layer. The PR is labeled app-hash-breaking, correctly — the pointer-write-target change is an intentional, acknowledged state-transition change, not just a caching optimization.
Security risks
No injection/auth-surface changes. The risk here is consensus correctness: a wrong invalidation rule for codeCache, or a write landing in the wrong cachekv layer, would cause validators to diverge on the resulting app hash rather than any classic OWASP-style vulnerability. That risk is real given the subtlety of the Freeze()/readThroughParent skip-memo reasoning underpinning this change.
Level of scrutiny
High. This touches the EVM state-transition hot path used by every deliver tx, is explicitly app-hash-breaking, and interacts with non-obvious invariants in sei-cosmos/store/cachekv (frozen layers, skip-memoization) and the EVM↔CW re-entry model. This is exactly the kind of change that warrants a human with deep familiarity of the store/cachekv layer and the EVM↔CW nesting rules, not just a review-bot sign-off.
Other factors
The PR went through several rounds of thorough review (from what looks like another automated reviewer bot) that surfaced genuine blocker-level issues — a second deliver DBImpl (CW→EVM re-entry) that could Finalize code into a warm outer memo, and the pointer-upgrade write landing in a frozen cachekv layer — both of which were fixed in the latest commits (0d35c5e, e8f3cb2, eac208a, dce056c) with corresponding tests added. Several nit/suggestion-level items from that same thread remain open (coarse revert invalidation, GetCodeSize/GetCodeHash not sharing the memo, event-manager scoping of the new write ctx) but did not read as live bugs on my own pass through the diff and current tree. Test coverage is solid for the scenarios exercised (revert, account recreate, Copy(), wasmd-entry gating, failed-redeploy). Given the combination of app-hash impact and the residual open suggestions, I'm deferring rather than approving.
Use live Multistore with the finite precompile meter for getter as well as SetCode/registry writes so registry reads are not infinite-metered. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
No new bugs found in this run. This PR is labeled app-hash-breaking and touches EVM state-transition code (deliver-tx code caching, pointer-upgrade store writes, gas metering on precompile reads) that has already gone through several rounds of subtle correctness fixes (frozen cache-layer writes, gas-meter bypass, code-cache staleness) — including one I flagged in an earlier run that has since been fixed via RefreshCodeCache. Given the consensus-critical nature and history of non-obvious bugs here, this warrants a human sign-off rather than a bot approval.
What was reviewed: the deliver-tx codeCache lifecycle (SetCode/GetCode/RefreshCodeCache, revert/cleanup/copy invalidation), the pointer-upgrade redeploy path's live-ctx vs frozen-ctx writes and gas metering, and the wasmd-entry/CallEVM nesting guard that keeps the memo from going stale. Checked that the previously-flagged code-cache staleness on pointer redeploy is now fixed (write + RefreshCodeCache go through the live sdb.Ctx()).
Extended reasoning...
Overview
This PR memoizes bytecode reads in the deliver-tx StateDB (a new codeCache map on DBImpl) to avoid re-reading the KV store on repeated GetCode calls within a transaction, and adds tests pinning GetCodeSize(addr) == len(GetCode(addr)). Touched files span x/evm/state/{code,state,statedb}.go (the cache itself, invalidation on revert/cleanup/copy) and x/evm/keeper/pointer_upgrade.go (the pointer-redeploy path, which needed to move its writes from a Prepare-time ctx — frozen once EVM snapshots — to the live sdb.Ctx(), both for cache coherency and for correctness against sei-cosmos's frozen-cachekv-layer semantics). The PR carries the app-hash-breaking label.
Security risks
No auth/crypto/permissions surface is touched. The main risk category is consensus correctness: any divergence between what different nodes compute for post-state (gas consumed, code visible to a same-tx CALL, pointer-registry entries) would be a fork risk. The review thread already surfaced and fixed several instances of exactly this class of bug (frozen-layer writes bypassing cachekv's write contract, a dropped gas meter on the exists-lookup, code-cache staleness on redeploy), which is a strong signal this diff sits closer to "consensus-critical" than "perf cleanup."
Level of scrutiny
High. This is EVM state-transition code labeled app-hash-breaking, with multiple rounds of reviewer-caught correctness bugs (including one from my own earlier pass) that were non-obvious and required tracing cachekv's Freeze/skip semantics and precompile gas-meter plumbing. That history, combined with the blast radius (any CALL-family path, any pointer upgrade), argues for a human sign-off on the final state even though this run's bug hunt came back clean.
Other factors
The PR has substantial new test coverage (code_test.go, code_size_invariant_test.go, pointer_upgrade_test.go) directly pinning the invariants that were previously broken, and the author (wen-coding) has been responsive, resolving each round of feedback with cited commits. Several open threads remain as suggestions/nits (aliasing of returned code slices, coarse cache invalidation on revert, minor redundant lookups) that are not correctness-blocking and are already visible as unresolved/resolved inline comments — I did not re-raise them.
There was a problem hiding this comment.
The codeCache memo looks correct: I traced every path that writes EVM code and each one either goes through DBImpl.SetCode, RefreshCodeCache, clearAccountCodeAndNonce, or a full clear() on revert/reset, and the wasmd-entry nesting is genuinely closed off because PrepareCtxForEVMTransaction sets the flag before NewDBImpl and CallEVM rejects EVM→CW→EVM otherwise. No blocking defect found, but the PR silently bundles a consensus-visible UpsertERCPointer change that the title/description don't mention, and there are several simplification, memory, and test-brittleness cleanups worth doing.
Findings: 0 blocking | 14 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Scope/description gap: the PR body only describes the
GetCodememo and the size/len invariant tests, butx/evm/keeper/pointer_upgrade.goalso changes (a) which Multistore layer the pointer registry and pointer bytecode are written to and (b) whetherSetCoderuns at all whenGetDeploymentCodefails. Both are state-machine-visible (hence theapp-hash-breakinglabel). Please describe them in the PR body so the release notes and the app-hash-breaking sign-off cover the actual change, and confirm the pointer-precompile behavior change is intended to apply to allprecompiles/pointer/legacy/v*versions (they all route through this one shared keeper method). - Nothing in
x/evm/AGENTS.mdrecords the new invariant. The "StateDB Bridge" section is the right place to state it: while a deliver-txDBImplis live, any keeper-level code write againstsdb.Ctx()must be paired withRefreshCodeCache, andGetCodeSize/GetCodeHashremain unmemoized store reads. Without that note the next person who adds ak.SetCodecall site has no way to know the rule exists. - This is a perf PR with no measurement.
benchmark/already exists; a before/after number for a CALL-heavy workload would justify the added invalidation surface (and would show how much the blanketclear()on revert costs). - Design:
GetCodeis now memoized whileGetCodeSizeandGetCodeHashstill hit the store on every call, so there are two sources of truth that the new invariant tests exist to keep aligned. Worth considering servingGetCodeSizefrom the memo when present (same tests still pin it), which both removes the divergence risk and picks up the size-gate reads that EIP-7702 CALL paths do. giga/deps/xevm/state/code.gois a parallel copy ofx/evm/state/code.goand did not receive the memo. Presumably intentional since giga is a separate fork, but worth a one-line confirmation.- Second-opinion passes were thin:
cursor-review.mdis empty (that pass produced no output at all), andcodex-review.mdreports no material issues while stating it could not run the focused tests because the Go toolchain was unavailable. So neither pass independently validated this change. I was likewise unable to rungo build/go testin this environment, so the new tests were reviewed statically only — theTest plancheckboxes in the description are still unchecked and should be confirmed green in CI before merge. - 8 suggestion(s)/nit(s) flagged inline on specific lines.
| return code | ||
| } | ||
| // Keep empty code as nil to match Keeper.GetCode, and store a copy so the | ||
| // memo is not aliased to the keeper/store-backed slice. |
There was a problem hiding this comment.
[suggestion] This empty-code branch duplicates logic putCodeCache already implements (lines 61-64), and line 31 then re-reads the map to get the slice that was just stored. The whole tail collapses to:
code := s.k.GetCode(s.ctx, addr)
if s.codeCache == nil {
return code
}
s.putCodeCache(addr, code)
return s.codeCache[addr]or, cleaner, have putCodeCache return the stored slice and return s.putCodeCache(addr, code). Two copies of the "normalize empty to nil" rule is exactly the kind of thing that drifts later.
| s.codeCache[addr] = nil | ||
| return | ||
| } | ||
| cached := make([]byte, len(code)) |
There was a problem hiding this comment.
[suggestion] The defensive copy is necessary for SetCode/RefreshCodeCache (the caller owns code), but on the GetCode miss path it doubles peak allocation for precisely the hot path this PR optimizes: Keeper.GetCode returns a cachekv-owned value that nothing in the store layer mutates in place, so the memo could alias it directly. For a 24 KB contract that's 24 KB saved per cold read, and a tx touching many contracts retains a full second copy of every one of them for the tx lifetime.
If you want the copy anyway for safety, consider splitting the read path (putCodeCacheNoCopy) from the write path so the choice is explicit rather than incidental.
| s.tempState = nil | ||
| s.logger = nil | ||
| s.snapshottedCtxs = nil | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] clear() empties the map but keeps its bucket array allocated. Cleanup() is terminal (it nils tempState, logger, and snapshottedCtxs right above), and its whole purpose is releasing memory after a tx, so s.codeCache = nil is the consistent choice here and actually frees the retained bytecode.
clear() is right in CleanupForTracer/ResetForTracer, where the DB is reused — no change needed there.
| } | ||
|
|
||
| // Store was rewound via CacheMultiStore; drop any cached code that may now be stale. | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[suggestion] Correct but maximally pessimistic: this drops every entry, including code read from the base layer that provably cannot be stale (committed before this tx, and the rewind can't touch it). Revert-heavy patterns — a contract that probes several callees with reverting CALLs, or any try/catch loop — will re-read the store for all of them after each revert, which can erase most of the memo's benefit on exactly the CALL-family paths this PR targets.
Since code writes already flow through a small number of choke points (SetCode, RefreshCodeCache, clearAccountCodeAndNonce), you could record the touched addresses per snapshot version (or add a journal entry alongside the existing watermark) and invalidate only those on revert. Not required for correctness — flagging it because the perf claim in the description is measured against the no-revert case.
| } | ||
| ctx.GasMeter().ConsumeGas(k.GetCosmosGasLimitFromEVMGas(ctx, suppliedGas-remainingGas), "ERC pointer deployment") | ||
| if err = setter(ctx, pointee, contractAddr); err != nil { | ||
| if err = setter(liveCtx(), pointee, contractAddr); err != nil { |
There was a problem hiding this comment.
[suggestion] This is the consensus-visible part of the PR and it isn't mentioned in the title or description. I agree the change is a real fix — after GetDeploymentCode/Create snapshot, the Prepare-time ctx layer is Freeze()d, and cachekv's readParent skip memo is only sound because "a frozen layer never gains writes," so a Set on it can be invisible to a deeper same-tx reader. Same for not calling SetCode on the failure path.
Two asks:
- Describe both changes in the PR body so the
app-hash-breakingsign-off covers what actually changed, not just the read-side memo. - Confirm the intent is for this to apply to every historical pointer-precompile version —
precompiles/pointer/legacy/v575…v640all call this same keeper method, so the new write-layer and failure-path semantics apply retroactively to all of them.
Gas accounting looks unchanged, for what it's worth: Prepare returns ctxer.Ctx(), so liveCtx() is the same layer with the same meter re-attached on the first call.
| writeCtx := liveCtx() | ||
| k.SetCode(writeCtx, contractAddr, ret) | ||
| if sdb != nil { | ||
| sdb.RefreshCodeCache(contractAddr, ret) |
There was a problem hiding this comment.
[suggestion] This k.SetCode(writeCtx, ...) + RefreshCodeCache(...) pairing is the one place in the tree where correctness depends on a caller remembering to poke the memo, and nothing enforces it — a future keeper-level code write inside a live statedb will silently desync GetCode from GetCodeSize/GetCodeHash.
Since the only reason not to use sdb.SetCode is to bill the write to ctx's meter rather than the statedb's infinite one, you could keep it inside the StateDB instead — e.g. a SetCodeWithCtx(addr, code, meter) on DBImpl that swaps the meter for the duration — so the memo update can't be forgotten. At minimum, a comment on Keeper.SetCode pointing at this requirement would help.
| // Finite cosmos meter large enough for the pointer-registry getter reads, but | ||
| // small enough that GetDeploymentCode OOGs (normalizer is 1 in tests). StateDB | ||
| // KV metering stays on the infinite RunWithOneOff meter. | ||
| lowGasCtx := ctx.WithGasMeter(sdk.NewGasMeterWithMultiplier(ctx, 50_000)) |
There was a problem hiding this comment.
[suggestion] The hard-coded 50_000 makes this test's mechanism fragile in a way that's worth avoiding. The window has to satisfy two competing constraints (enough for the getter KV reads, not enough for GetDeploymentCode), and only one side fails cleanly: if artifact size or read costs grow, the getter(liveCtx(), ...) reads exhaust the finite cosmos meter first, which panics with ErrorOutOfGas rather than returning through require.Error — RunWithOneOffEVMInstance doesn't recover. The failure would be a confusing panic, not "expected an error."
Either derive the limit from the actual cost (measure the getter's consumption on a probe meter, then set the limit just above it), or assert on the error identity so a drift shows up as a clear mismatch:
require.ErrorIs(t, err, vm.ErrOutOfGas) // or whatever GetDeploymentCode surfaces| func TestGetCodeSizeMatchesGetCodeLength(t *testing.T) { | ||
| k := &testkeeper.EVMTestApp.EvmKeeper | ||
| ctx := testkeeper.EVMTestApp.GetContextForDeliverTx([]byte{}) | ||
| _, addr := testkeeper.MockAddressPair() |
There was a problem hiding this comment.
[nit] All seven subtests share this one addr and ctx and mutate it in sequence, which makes them order-dependent — -run 'TestGetCodeSizeMatchesGetCodeLength/overwrite_shrinks_size_metadata' no longer tests shrinking, because the preceding max code size write never happened. That defeats the usual reason for splitting into subtests.
A fresh _, addr := testkeeper.MockAddressPair() inside each t.Run (with the max-code-size case explicitly seeding a large value first where shrinking is the point) makes each case independently meaningful and self-documenting.
Superseded: latest AI review found no blocking issues.
Upsert now writes the registry via StateDB Multistore, so the mock must RunWithOneOffEVMInstance (Finalize) for TestBlockBloom synthetic receipts. Co-authored-by: Cursor <[email protected]>
PR SummaryHigh Risk Overview
Tests pin Reviewed by Cursor Bugbot for commit 5a1c560. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
I don't have anything new to add beyond what's already in the thread — the codeCache/keeper-write desync I flagged earlier in this run was resolved via RefreshCodeCache (9db1e74), and the gas-meter and error-path issues cursor/seidroid raised were also addressed in follow-up commits. This PR is labeled app-hash-breaking and changes same-tx read/write visibility semantics for the pointer-registry Multistore layer plus gas accounting on the exists-lookup path, so it warrants a human sign-off rather than an automated approval.
Extended reasoning...
This PR is app-hash-breaking: it changes which Multistore layer pointer-upgrade code writes and reads go through (live sdb.Ctx() vs. the Prepare-time ctx, which may be frozen after GetDeploymentCode/Create snapshots), and adds a deliver-tx bytecode memo (codeCache) to the EVM StateDB that must stay coherent with the KV store across SetCode, snapshot revert, account recreation, and out-of-band keeper writes. All of this sits directly on the consensus-critical execution path (state root determinism, gas metering for precompile calls).\n\nDuring this run I re-verified the specific concern I raised earlier in the thread (keeper-side SetCode in UpsertERCPointer's redeploy branch bypassing the StateDB memo) and confirmed it's fixed: the code now calls sdb.RefreshCodeCache right after the keeper write. I also checked that the gas-meter bug cursor flagged (exists-lookup reading through the infinite sdb.Ctx() meter instead of the caller's finite precompile meter) is fixed via the hoisted liveCtx() helper that reattaches ctx.GasMeter(), and that the failed-GetDeploymentCode path now returns before any SetCode/RefreshCodeCache call. No bugs were found by the automated bug-hunting system on this run.\n\nGiven the app-hash-breaking label and the density of subtle correctness reasoning this thread already contains (frozen-layer semantics, wasmd-entry nesting guards, revert-time cache invalidation), a human should still sign off on the final state even though the specific issues raised so far have been resolved.
There was a problem hiding this comment.
The deliver-tx codeCache looks coherent: every store-side code writer (DBImpl.SetCode, clearAccountCodeAndNonce, the keeper write in UpsertERCPointer) has a matching memo hook, revert/cleanup flush the memo, Copy() starts empty, and the nesting hazard is correctly narrowed by CallEVM's EVM→CW→EVM rejection plus the wasmd-entry opt-out. The UpsertERCPointer switch to sdb.Ctx() is a genuine fix (writing to a Freeze()d layer can be skipped by descendants' memoized readParent), and the failure-path early return is a real improvement. No blockers found; remaining notes are gas-accounting/memory/documentation and test-robustness nits.
Findings: 0 blocking | 15 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty); Codex reported "No material issues found". So only this pass contributed findings. - Gas accounting: a warm
GetCodeno longer performs the gas-metered KV read, so total consumed Cosmos gas drops for any memo-enabled statedb whose ctx carries a finite meter. Deliver/CallEVMare fine (both install an infinite meter), butRunWithOneOffEVMInstancebuilds itsDBImplfrom the caller's ctx —x/evm/gov.go(gov handler meter) andx/evm/migrations/migrate_all_pointers.gopass a non-infinite meter toNewDBImpl. Worth confirming this is intended and in scope of theapp-hash-breakinglabel, since it isn't mentioned in the PR description. - Memory:
putCodeCachestores a defensive copy, so a touched contract's bytecode is retained twice for the statedb lifetime (once in the cachekv read cache, once in the memo) — up to ~2×24KB per distinct contract in a code-heavy tx. cachekv values are already treated as immutable everywhere else, so the copy may be droppable; if kept, consider noting the bound. Relatedly,Cleanup()could sets.codeCache = nilrather thanclear()(consistent with the other fields it nils) so the bucket array is released too. - The ante statedbs (
x/evm/ante/fee.go:73,app/ante/evm_checktx.go:302) now allocate acodeCachemap per tx even though they only touch balances/nonces. Minor, but the enablement predicate could be narrowed if per-tx allocations matter there. - Docs: the memo is a non-obvious cross-cutting invariant with five invalidation hooks. Consider adding it to the "Key design choices" list under StateDB Bridge in
x/evm/AGENTS.md, next to snapshots/transient state. - Future-proofing: nothing prevents a new
Keeper.SetCode/CodeKeyPrefixwrite on a deliver ctx from silently desyncing the memo (this PR had to retrofitRefreshCodeCachefor exactly that). A short comment onKeeper.SetCodeinx/evm/keeper/code.gopointing atDBImpl.RefreshCodeCachewould make the requirement discoverable. - Test coverage gap:
TestCodeCacheDisabledForSimulationAndWasmdEntryapproximates the nesting hazard with a directk.SetCode(wasmdEntry.Ctx(), ...), not with an actual nestedCallEVMwhose innerDBImplFinalizes code into the outer Multistore — the scenario the opt-out exists for. An integration test through the wasmd precompile would pin the real invariant. giga/deps/xevm/stateis a hand-fork ofx/evm/state(identicalGetCode/SetCodeshape) that gets neither the memo nor the size/len invariant test. Presumably intentional, but worth confirming the two aren't expected to stay in sync.- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| // for the statedb lifetime. Wasmd-entry deliver txs also leave it nil (see | ||
| // NewDBImpl) so a nested CallEVM DBImpl cannot Finalize code into the same | ||
| // Multistore while this memo stays warm. Non-nil (ordinary deliver) is | ||
| // cleared on snapshot revert and Cleanup, and kept in sync by SetCode. |
There was a problem hiding this comment.
[suggestion] This list of invalidation points omits WithCtx (statedb.go:280), which repoints the statedb at a different store hierarchy and would leave the memo describing the old store. It's safe today only because both production callers (Backend.PrepareTx / PrepareTxNoFlush in evmrpc/simulate.go) call CleanupForTracer/ResetForTracer immediately before, and those tracer statedbs are simulation=true (memo nil) anyway. Since WithCtx is exported and used in tests with simulation=false (e.g. precompiles/common/precompiles_test.go, precompiles/wasmd/wasmd_test.go), a defensive clear(s.codeCache) inside WithCtx would close the hazard for ~free and keep the invariant list here complete.
| s.codeCache[addr] = nil | ||
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) |
There was a problem hiding this comment.
[nit] The len(code) == 0 branch above duplicates the normalization putCodeCache already does, and the return s.codeCache[addr] here costs a second map lookup. Having putCodeCache return the stored slice collapses the whole tail to:
return s.putCodeCache(addr, code)(with the s.codeCache == nil early return above kept so the uncached path still returns the store slice).
|
|
||
| func (s *DBImpl) GetCodeSize(addr common.Address) int { | ||
| s.k.PrepareReplayedAddr(s.ctx, addr) | ||
| return s.k.GetCodeSize(s.ctx, addr) |
There was a problem hiding this comment.
[suggestion] GetCodeSize still hits the KV store on every call while GetCode is memoized. The EIP-7702 CALL-family gate this PR is optimizing for calls GetCodeSize and GetCode, so half the reads on that hot path remain un-memoized. Now that GetCodeSize(addr) == len(GetCode(addr)) is pinned by the two new invariant tests, a warm-memo fast path here (if c, ok := s.codeCache[addr]; ok { return len(c) }) would be cheap and consistent — worth doing in this PR or noting as deliberate follow-up.
| // caller's gas meter (finite precompile meter in deliver) — sdb.Ctx() alone | ||
| // carries the infinite EVM meter. | ||
| sdb := state.GetDBImpl(evm.StateDB) | ||
| liveCtx := func() sdk.Context { |
There was a problem hiding this comment.
[nit] liveCtx() inherits sdb.Ctx()'s EventManager and drops the precompile-scoped one (plus EVMPrecompileCalledFromDelegateCall) that RunAndCalculateGas installs — only the gas meter is carried over. That's benign for the current writes (on the redeploy path SetAddressMapping is a no-op since the mapping already exists, so k.SetCode emits nothing, and setter is a plain store write), and the explicit ctx.EventManager().EmitEvent below still uses the scoped manager. Worth calling out in this comment though: any future keeper write added under writeCtx would emit into the outer manager and bypass the precompile's success-only event scoping.
| } | ||
|
|
||
| // Store was rewound via CacheMultiStore; drop any cached code that may now be stale. | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] Correct and appropriately conservative, but this flushes the entire memo on every revert, and reverts are common in call-heavy txs — so the CALL-family hot path this PR targets can end up repeatedly cold. Since code writes are rare, a codeDirty bool set by putCodeCache/clearAccountCodeAndNonce and checked here (clear only if any code write happened since the DB was created) would preserve nearly all the benefit with the same safety. Fine as a follow-up.
| Symbol: "after", | ||
| Decimals: 8, | ||
| }) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
[nit] A bare require.Error means this test still passes if the failure comes from something other than the intended GetDeploymentCode OOG — and the 50_000 figure is tuned against current artifact size, pointer-getter read costs, and "normalizer is 1 in tests", any of which can drift and silently move the failure earlier (e.g. into the getter reads, which would no longer exercise the "don't clobber code" path at all). Asserting on the error (e.g. require.ErrorContains(t, err, "out of gas"), or whatever GetDeploymentCode surfaces) would keep the test honest about why it failed.
| _, err := a.EvmKeeper.UpsertERCCW20Pointer(ctx, e, contractAddr.String(), utils.ERCMetadata{Name: "test", Symbol: "test"}) | ||
| return err | ||
| }, func(step, msg string) { | ||
| panic(fmt.Sprintf("UpsertERCCW20Pointer %s: %s", step, msg)) |
There was a problem hiding this comment.
[nit] RunWithOneOffEVMInstance also invokes this callback for the purely informational logger("non-zero surplus", ...) case and then returns nil, so a non-zero surplus now hard-panics test setup instead of being ignored. Unlikely for a zero-value, zero-gas-price pointer deploy, but since the err != nil check below already covers the real failure modes, gating the panic (e.g. skip when step == "non-zero surplus") or just logging would be less surprising.
wen-coding
changed the title
Avoid wholesale memo clear on nested RevertToSnapshot so unrelated warmed bytecode survives; only SetCode/Refresh/clear are journaled. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
The deliver-tx codeCache design is sound: every store-level code write in a live-DBImpl path (DBImpl.SetCode, clearAccountCodeAndNonce, and the keeper write in UpsertERCPointer via RefreshCodeCache) updates the memo and is journaled per address, and the "disable on wasmd entry" gate is backed by the existing CallEVM / wasmd-precompile reentrancy guards, so no nested cache-enabled DBImpl can Finalize under a warm outer memo. No blockers found; the notes below are hardening, documentation, and a gating question on the app-hash-breaking pointer change.
Findings: 0 blocking | 12 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review is the merge of my findings with Codex's (which reported "no material issues found").- Coherence of the memo now rests on an unwritten invariant:
keeper.SetCodemust never be called against a live cache-enabledDBImpl's ctx without a matchingRefreshCodeCache. Today onlyx/evm/genesis.go(no live StateDB) andx/evm/keeper/pointer_upgrade.godo so, but nothing enforces it and a violation desyncsGetCodefromGetCodeSize/GetCodeHashsilently. Consider a doc note onKeeper.SetCode. - The PR is a perf change but carries no benchmark or measured numbers for the
GetCodememo on the CALL-family hot path — worth attaching before/after figures given the added state-coherence surface. - No end-to-end test for the wasmd-entry nesting scenario the gate exists for (outer deliver DB with caching disabled + inner
CallEVMDB with caching enabled Finalizing into the outer Multistore).TestCodeCacheDisabledForSimulationAndWasmdEntryonly asserts the flag's direct effect on a standalone DB. x/evm/keeper/code_size_invariant_test.goandx/evm/state/code_size_invariant_test.gooverlap substantially; not a problem, just noting the duplication if the invariant ever changes.- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| return sdb.Ctx().WithGasMeter(ctx.GasMeter()) | ||
| } | ||
| existingAddr, _, exists := getter(liveCtx(), pointee) |
There was a problem hiding this comment.
[suggestion] This consensus-behavior change is not version-gated, even though the repo maintains precompiles/pointer/legacy/v575…v640 specifically to preserve historical pointer-precompile behavior. All of those legacy versions call straight into this shared UpsertERCPointer, so replaying/tracing pre-upgrade blocks through them will now take the new write-target (sdb.Ctx() instead of the Prepare-time ctx) and the new skip-SetCode-on-error path. That's fine for forward execution if it lands with a coordinated release cut, but please confirm that's the plan (and that historical debug_trace* reproduction isn't a requirement here) rather than leaving it implicit in the app-hash-breaking label.
Note this is not the "tag doesn't exist yet" false positive REVIEW_GUIDELINES §1/§2 rules out — the concern is the absence of a gate for a deliberate behavior change on a code path that has an established versioning mechanism.
| // Only write on success: a failed GetDeploymentCode can leave ret as nil or | ||
| // revert data, which must not clobber live pointer bytecode (even transiently). | ||
| writeCtx := liveCtx() | ||
| k.SetCode(writeCtx, contractAddr, ret) |
There was a problem hiding this comment.
[suggestion] Redeploy writes (and the registry write on line 139) now land on the StateDB's live Multistore layer, so they are lost unless the caller eventually calls stateDB.Finalize(). That's a real contract change for external callers — it's exactly why evmrpc/tests/mock_contracts.go had to be rewritten to use RunWithOneOffEVMInstance. I checked the remaining callers (x/evm/gov.go, x/evm/migrations/migrate_all_pointers.go, precompiles/pointer/**) and they all Finalize, but please state this requirement in the doc comment on UpsertERCPointer so the next caller doesn't hit a silent no-op.
| s.codeCache[addr] = nil | ||
| return | ||
| } | ||
| cached := make([]byte, len(code)) |
There was a problem hiding this comment.
[suggestion] The fill path copies every bytecode read into an unbounded per-tx map. Because the underlying cachekv layer already retains the read value for the tx, this roughly doubles peak retained bytecode: a tx that CALLs many distinct near-MaxCodeSize contracts (cold access is only 2600 gas) can hold hundreds of MB of duplicated code. Consider either (a) skipping the defensive copy on the fill path — neither geth's vm.Contract nor Sei mutates the returned code, and the store-backed slice already has tx lifetime — or (b) tracking total cached bytes and stopping insertion past a cap. Copying on SetCode/RefreshCodeCache (caller-owned slices) is still worth keeping.
|
|
||
| // RefreshCodeCache updates the deliver-tx code memo after a keeper store write | ||
| // that bypassed SetCode (so gas can be charged against a different ctx meter). | ||
| func (s *DBImpl) RefreshCodeCache(addr common.Address, code []byte) { |
There was a problem hiding this comment.
[suggestion] RefreshCodeCache is exported but silently assumes the caller already wrote the same bytes to s.Ctx()'s Multistore. If a future caller writes to a different ctx/layer (or forgets the call entirely), GetCode diverges from GetCodeSize/GetCodeHash with no signal. Worth spelling that precondition out here, or folding the keeper write into a single SetCodeWithMeter(addr, code, gasMeter) helper so the store write and the memo update can't be separated.
| // warm outer memo would then disagree with store. Leave nil until wasm is | ||
| // decommissioned and that nest path is gone. Nested CallEVM itself clears | ||
| // EVMEntryViaWasmdPrecompile before NewDBImpl, so the inner DB still memos. | ||
| if !simulation && !ctx.EVMEntryViaWasmdPrecompile() { |
There was a problem hiding this comment.
[nit] The soundness of enabling the memo here depends on two guards in other packages: x/evm/keeper/evm.go:82 ("sei does not support EVM->CW->EVM call pattern") and precompiles/wasmd/wasmd.go:90 ("sei does not support CW->EVM->CW call pattern"). Together they are what guarantee no second cache-enabled DBImpl can be nested under a cache-enabled one — the EVMEntryViaWasmdPrecompile check alone doesn't cover the CW-originated CallEVM entry (ctx.IsEVM() is false there, so the memo is on). Please name those two guards in this comment; if either is ever relaxed, the memo goes stale silently.
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) | ||
| return s.codeCache[addr] |
There was a problem hiding this comment.
[nit] Redundant map lookup — have putCodeCache return the slice it stored (or assign it to a local) and return that instead of re-reading s.codeCache[addr].
| // Finite cosmos meter large enough for the pointer-registry getter reads, but | ||
| // small enough that GetDeploymentCode OOGs (normalizer is 1 in tests). StateDB | ||
| // KV metering stays on the infinite RunWithOneOff meter. | ||
| lowGasCtx := ctx.WithGasMeter(sdk.NewGasMeterWithMultiplier(ctx, 50_000)) |
There was a problem hiding this comment.
[nit] The 50_000 gas budget is tuned to sit between "enough for the registry getter reads" and "OOG inside GetDeploymentCode". If the pointer artifact bin or the gas config shifts, this silently stops exercising the intended failure mode — or starts failing the earlier getter and passing for the wrong reason. require.Error catches the first case but not the second; consider asserting the error is the out-of-gas one from the deployment step rather than any error.
There was a problem hiding this comment.
No bugs found by this pass. This PR is labeled app-hash-breaking and changes same-tx state visibility and gas metering in core EVM paths, so it warrants a human sign-off regardless.
Beyond the reported findings, I independently traced the wasmd-entry nesting invariant the codeCache-disable logic depends on (CW→EVM→CW→EVM via CallEVM) — a second re-entry attempt is rejected by the EVM->CW->EVM guard in CallEVM (x/evm/keeper/evm.go:82), so only two DBImpls can ever coexist against the same Multistore. This confirms the reasoning already discussed and resolved on this thread; I did not find a gap beyond it.
Extended reasoning...
Traced x/evm/keeper/evm.go's CallEVM guard (ctx.IsEVM() && !ctx.EVMEntryViaWasmdPrecompile()) and msg_server.go's PrepareCtxForEVMTransaction/executionCtx flag handling to verify that only a single level of CW→EVM re-entry is reachable within a wasmd-entry deliver tx, which is the invariant the new codeCache-disable-on-wasmd-entry logic relies on for correctness. It holds. Given the PR is app-hash-breaking (changes UpsertERCPointer write/read targets and gas metering) and touches the deliver-tx StateDB hot path, I'm deferring rather than approving even though no new bugs surfaced this run.
Match x/evm codeCache + per-address journal invalidation so GigaExecutorEnabled deliver txs get the same GetCode memoization. Co-authored-by: Cursor <[email protected]>
wen-coding
changed the title
There was a problem hiding this comment.
The deliver-tx codeCache memo is carefully built — per-address journaling, nil-disabled for simulation/RPC/trace/wasmd-entry, and the only keeper-level SetCode bypass (UpsertERCPointer) is refreshed — and I could not find a staleness path that reaches the store behind a warm memo. No blockers; the notes below are polish, parity, and a few semantics worth confirming given the app-hash-breaking label.
Findings: 0 blocking | 13 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage is partial:
cursor-review.mdis empty (that pass produced no output). Only Codex's pass is represented, and it reported "no material issues". GetCodeSize/GetCodeHashstill hit the KV store on every call even when the memo is warm.EXTCODESIZEand the EIP-7702 CALL-family size gate are on the same hot path this PR targets, and the newGetCodeSize(addr) == len(GetCode(addr))tests establish exactly the invariant that would make serving size from the memo safe. Worth a follow-up (or a note on why it was left out).- Memory shape changed: the memo retains a copy of every bytecode read for the tx, and
journalCodeCacheMutationnow retains every superseded bytecode blob in the journal until revert/end-of-tx. This mirrors geth's per-object code cache so it is probably fine, but there is no bound — a sanity measurement on a worst-case call-heavy / repeated-SetCodetx would be reassuring. TestCodeCacheClearedOnRevert(bothx/evm/stateandgiga/deps/xevm/state) is named for the wholesale-clear behavior the PR explicitly moved away from; it actually asserts per-address restore. Rename to match (...RestoredOnRevert).clear(s.codeCache)inCleanupForTracer/ResetForTraceris unreachable today: every tracingDBImplis constructed viaevmrpcwithsimulation=true, so the memo is nil. Harmless defensive code, but a one-line comment would stop a future reader concluding trace DBs memoize.- Test coverage gap on the revert-scope change: the new tests cover "failed
GetDeploymentCodewrites nothing", but not "upsert succeeds, then the surrounding EVM frame reverts". That is the case whose behavior actually changed — on main the registry/code write landed on the Prepare-time layer and survivedRevertToSnapshot, whereas now it lands on the live top and is discarded. Given theapp-hash-breakinglabel, that transition deserves an explicit test. - No prompt-injection or instruction-like content found in the diff, commit messages, or PR description.
- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| if sdb == nil { | ||
| return ctx | ||
| } | ||
| return sdb.Ctx().WithGasMeter(ctx.GasMeter()) |
There was a problem hiding this comment.
[suggestion] liveCtx() deliberately re-attaches ctx.GasMeter() but silently drops everything else ctx carried — notably the precompile-scoped EventManager that DynamicGasPrecompile.RunAndCalculateGas installs (ctx = ctx.WithEventManager(sdk.NewEventManager()), propagated to the parent only on success), plus WithEVMPrecompileCalledFromDelegateCall.
That matters for k.SetCode(writeCtx, ...): it calls SetAddressMapping, which emits EventTypeAddressAssociated on whatever ctx it is handed. Previously that event went into the precompile's scoped manager and was dropped if the precompile later errored; now it lands on the DBImpl snapshot layer's manager, which Finalize() flushes for every surviving layer. In practice the redeploy branch hits an already-mapped pointer so this is usually a no-op, but the semantics changed unintentionally.
Suggest preserving it explicitly so only the store target moves:
return sdb.Ctx().WithGasMeter(ctx.GasMeter()).WithEventManager(ctx.EventManager())The comment above reasons carefully about the gas meter; it should say the same about the event manager either way.
| sdb := state.GetDBImpl(evm.StateDB) | ||
| liveCtx := func() sdk.Context { | ||
| if sdb == nil { | ||
| return ctx |
There was a problem hiding this comment.
[suggestion] When state.GetDBImpl(evm.StateDB) returns nil, this silently falls back to the exact pre-fix behavior: writes go to the Prepare-time ctx, which is the layer the comment above says may already be frozen — and RefreshCodeCache is skipped too. So the nil case is precisely the bug this PR is fixing, just unreported.
Today no production path reaches it (GetDBImpl unwraps HookedStateDB, and giga's precompile table fail-fasts the pointer precompile so those txs fall back to v2), but that is a non-obvious invariant. Worth either a comment stating why nil is unreachable in deliver, or returning an error instead of a quiet downgrade.
| journal: []journalEntry{}, | ||
| coinbaseEvmAddress: feeCollector, | ||
| } | ||
| // Enable the memo only for ordinary deliver txs (parity with x/evm/state). |
There was a problem hiding this comment.
[suggestion] The comment claims "parity with x/evm/state", but the guard is not the same: x/evm/state uses if !simulation && !ctx.EVMEntryViaWasmdPrecompile(), and its doc comment explains a concrete hazard (a nested CallEVM DBImpl can Finalize code into this same Multistore while the outer memo stays warm).
The divergence looks safe today — giga/executor/precompiles.AllCustomPrecompilesFailFast maps the wasmd precompile to FailFastSingleton, so a giga tx that would nest aborts and falls back to v2, and executeEVMTxWithGigaExecutor never sets EVMEntryViaWasmdPrecompile — but nothing here records that, and the "parity" wording actively misleads. Either add the guard for real parity, or replace the comment with why giga doesn't need it.
|
|
||
| // RefreshCodeCache updates the deliver-tx code memo after a keeper store write | ||
| // that bypassed SetCode (so gas can be charged against a different ctx meter). | ||
| func (s *DBImpl) RefreshCodeCache(addr common.Address, code []byte) { |
There was a problem hiding this comment.
[suggestion] RefreshCodeCache(addr, code) is an exported method that trusts the caller to pass bytes matching what it actually wrote to the store — a mismatch is a silent, tx-scoped divergence between GetCode and GetCodeSize/GetCodeHash with no way to detect it.
An invalidate-only shape (InvalidateCodeCache(addr): journal the prior entry, then delete) can't be desynchronized, is a smaller API, and costs at most one re-read at the single call site in UpsertERCPointer. The codeCacheChange journal entry already supports the had:false → delete direction, so the revert path needs no change.
| return nil | ||
| } | ||
| s.putCodeCache(addr, code) | ||
| return s.codeCache[addr] |
There was a problem hiding this comment.
[nit] putCodeCache already has the copy in hand; return s.codeCache[addr] re-hashes the key for no reason. Having putCodeCache return the stored slice would drop the extra lookup here and in the giga/deps/xevm/state/code.go:30 twin.
Worth noting in the comment while you're here: a warm GetCode now hands out the memo's backing array on every call, so a caller that mutates the returned slice corrupts the memo for the rest of the tx while the store stays correct. geth doesn't mutate GetCode results, so this is a latent hazard rather than a bug — but it's a new invariant the copy-on-insert comment doesn't spell out.
| s.tempState = nil | ||
| s.logger = nil | ||
| s.snapshottedCtxs = nil | ||
| clear(s.codeCache) |
There was a problem hiding this comment.
[nit] Every other line in Cleanup nils its field; clear drops the entries but keeps the map's bucket array alive and leaves codeCache != nil, i.e. still reading as "caching enabled". s.codeCache = nil matches the intent of the surrounding lines. (The clear calls in CleanupForTracer/ResetForTracer are correct as-is — those must preserve the enabled/disabled state.)
There was a problem hiding this comment.
This PR is complex and consensus-critical (labeled app-hash-breaking), and there are still unresolved review comments from this round — e.g. the version-gating question on UpsertERCPointer for legacy pointer-precompile versions (v575…v640) raised on 2026-08-05 — so it's worth a human look before merging.
What was reviewed:
- The deliver-tx
codeCachememo invalidation paths (SetCode,RefreshCodeCache, account recreate, per-address snapshot revert) across bothx/evm/stateandgiga/deps/xevm/state. - The pointer-upsert Multistore fix: confirmed the earlier finding from a prior run (keeper-bypass
SetCodeinUpsertERCPointerdesyncingcodeCache) is now fixed via the addedRefreshCodeCachecall. - Gas-meter handling in
liveCtx()— the caller's finite precompile meter is now correctly attached to the exists-lookup, addressing the earlier "uncharged read" concern.
Extended reasoning...
Overview
The PR memoizes deliver-tx bytecode reads (codeCache) in both x/evm/state and the giga executor's xevm/state StateDB, and separately changes UpsertERCPointer to write pointer redeploy code and registry entries through the live, unfrozen sdb.Ctx() Multistore layer instead of the (potentially frozen) Prepare-time ctx. The latter is explicitly labeled app-hash-breaking since it changes same-tx visibility/revert scope of those writes versus main.
Security risks
The main risk surface is consensus-correctness rather than classic injection/auth issues: an incorrect cache invalidation could cause a deliver-tx to execute stale bytecode (a state-divergence bug), and the pointer-upsert write-target change alters what a precompile call observes/persists mid-transaction. Both are the kind of subtle, hard-to-test-exhaustively bugs that historically cause chain halts or app-hash mismatches rather than an exploitable security hole in the traditional sense.
Level of scrutiny
This warrants the highest level of scrutiny the review process affords. It touches the EVM StateDB bridge (a core, shared abstraction), is explicitly app-hash-breaking, and modifies behavior in a shared helper (UpsertERCPointer) that is called by every historical version of the pointer precompiles. The PR has already been through multiple rounds of automated review (seidroid, cursor) surfacing several real issues (gas-meter bypass, cache desync on keeper writes, failure-path code clobbering) that the author fixed — but the most recent round of comments, including a substantive one about the change not being version-gated for legacy precompile replay/tracing, has not yet been addressed or resolved in the thread.
Other factors
My own prior run flagged the same class of bug (codeCache not invalidated on the keeper-bypass SetCode in UpsertERCPointer) and that has since been fixed via the new RefreshCodeCache call — a good sign the author is responsive. However, given the unresolved version-gating question, the app-hash-breaking label, and the density of prior findings on this PR, I don't think this is safe to wave through without a human confirming the intended rollout plan (coordinated release cut vs. version gate) for the pointer-upsert behavior change.
3 participants
Summary
StateDB(codeCache) so repeatedGetCodecalls avoid re-reading the KV store on CALL-family paths.giga/deps/xevm/statesoGigaExecutorEnableddeliver txs get the win (simulation still leaves the memo nil).codeCachenil for simulation/RPC/trace and wasmd-entry deliver statedbs inx/evm(nestedCallEVMcan Finalize into the same Multistore).Copy()starts empty when caching is enabled and stays nil when disabled.SetCode, account recreate, and viaRefreshCodeCachefor keeper bypasses. Snapshot revert restores only journaled code mutations (not a wholesaleclear), so unrelated warmed entries survive nested reverts.GetCodeSize(addr) == len(GetCode(addr))on keeper and StateDB (EIP-7702 size-gate expectations with Sei’s separate code-size metadata).Pointer upsert Multistore fix (
app-hash-breaking)UpsertERCPointerrunsGetDeploymentCode/Create, which snapshot andFreeze()Multistore layers after precompile Prepare. Exists-lookup, redeploySetCode, and registry writes now go through the live unfrozen top (sdb.Ctx()) with the caller’s finite precompile gas meter, instead of the Prepare-timectxthat may already be frozen. FailedGetDeploymentCodereturns before any code write. This changes same-tx visibility / revert scope of those writes vs main and is why the PR is labeledapp-hash-breaking.Test plan
GOWORK=off go test ./x/evm/state/ -run 'TestCode|TestStateDBGetCodeSize' -count=1GOWORK=off go test ./x/evm/keeper/ -run 'TestGetCodeSizeMatchesGetCodeLength|TestUpsertERCNativePointer' -count=1GOWORK=off go test ./giga/deps/xevm/state/ -run 'TestCode|TestCodeCache' -count=1