perf(evm): cache BLOCKHASH via 32-byte hash ring (CON-372)#3811
perf(evm): cache BLOCKHASH via 32-byte hash ring (CON-372)#3811wen-coding wants to merge 6 commits into
Conversation
Store recent parent block hashes in the EVM store and an in-memory cache so repeated BLOCKHASH lookups avoid reloading staking HistoricalInfo. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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 #3811 +/- ##
==========================================
- Coverage 60.17% 59.23% -0.95%
==========================================
Files 2327 2237 -90
Lines 194617 184199 -10418
==========================================
- Hits 117106 109102 -8004
+ Misses 66943 65378 -1565
+ Partials 10568 9719 -849
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
sync.Map by value trips govet copylocks wherever Keeper is copied. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 d8b85c7. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The on-chain 256-entry hash ring is a sound design and correctly replaces the expensive HistoricalInfo header-rehash on the BLOCKHASH path, but the accompanying process-local sync.Map is unbounded: it is evicted one key per block at a fixed height-257 offset, so entries populated by the historical-info fallback (reachable from untrusted eth_call/debug_trace* at arbitrary heights) are never evicted, and giga's prune is skipped entirely for blocks with no EVM txs. Replacing the sync.Map with the already-vendored golang-lru/v2 cache resolves all three inline blockers.
Findings: 3 blocking | 10 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass (
cursor-review.md) produced no output (file is empty), so this synthesis merges only Claude's and Codex's findings. Codex's single finding (unbounded process-memory cache) is confirmed and reported inline. - Test gap: the new
TestGetHashFnpins the KV path and the HistoricalInfo path to unrelated synthetic hashes (kvHashat height 7 vs. a hand-built header at height 6), so nothing asserts the invariant that actually matters for an app-hash-breaking change — thatLastBlockId.Hashrecorded at height H+1 equalsHistoricalInfo(H).Header.Hash(). During the first 256 blocks after the upgrade, BLOCKHASH switches source mid-window; a test that drives a real block through both paths and asserts equality would guard against a silent BLOCKHASH semantic change. - No test covers the
!ctx.IsTracing()gating ofTrackBlockHash(i.e. that tracing/replay does not write the ring), and giga has no end-to-end coverage —TestPruneBlockHashCacheonly exercises the mem-cache prune with hand-written KV entries. BlockHashPrefixis not added to the prefix lists inx/evm/genesis.go(lines ~69-76 and ~145-152) that driveExportGenesis/ExportGenesisStream. After an export/import the ring starts empty, so BLOCKHASH falls back toHistoricalInfo(also not exported) and returns zero for up to 256 blocks. That is consistent with today's behavior and probably intended — worth confirming as a deliberate choice rather than an omission.TrackBlockHashwrites to the EVM KV store unconditionally on every BeginBlock with no upgrade-name gate. Per this repo's review guidelines I am not flagging the absence of a registered tag, and theapp-hash-breakinglabel indicates awareness — just confirm this ships in a coordinated app-hash-breaking release rather than a rolling one.MaxBlockHashHistory = 256is duplicated inx/evm/keeper/block_hash.go:11andgiga/deps/xevm/keeper/block_hash.go:11. Because x/evm owns the KV window and giga's prune offset must match it, any future drift between the two constants would make giga serve mem-cached hashes for heights already pruned from the shared KV ring. Consider having giga reference the x/evm constant (or add a cross-package assertion).giga.SetBlockHash/giga.DeleteBlockHashare referenced only from tests (production giga never writes the ring, by design — x/evm owns it). Consider unexporting them or adding a doc comment stating they exist for tests only, so a future caller doesn't assume giga maintains the ring.- Minor storage cost worth noting: one
Set+ oneDeleteper block against the EVM store means ~32 bytes/block retained permanently in the versioned state-store changelog (order of a GB over tens of millions of blocks). Inherent to an on-chain ring, but worth stating explicitly given the PR's perf framing. - The PR is labeled
perfbut includes no benchmark numbers. The repo has abenchmark/harness (seebenchmark/CLAUDE.md); a before/after figure for the BLOCKHASH path would substantiate the change, especially since it is app-hash-breaking. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| func (k *Keeper) getHistoricalHash(ctx sdk.Context, h int64) common.Hash { | ||
| height := uint64(h) //nolint:gosec | ||
| if cached, ok := k.blockHashCache.Load(height); ok { |
There was a problem hiding this comment.
[blocker] Unbounded process-memory cache. blockHashCache is a plain sync.Map whose only eviction is the single Delete(height-1-MaxBlockHashHistory) in TrackBlockHash. Because that prune pointer advances monotonically one key per block and never revisits, any entry stored for a height below the current pointer is retained for the lifetime of the process.
The fallback branch at line 351 is exactly that case: it caches hashes for arbitrary historical heights, and it is reachable from untrusted RPC input. A single eth_call/debug_traceCall at historical block X can execute BLOCKHASH across the whole [X-256, X-1] window, caching up to 256 permanent entries per request; iterating over distinct heights (archive nodes, or an attacker walking history) grows the map without bound. At ~100 bytes/entry, ~40k requests spread over distinct heights is ~1 GB of unreclaimable heap.
Secondary effect: the prune also never runs while ctx.IsTracing() (it lives inside the !ctx.IsTracing() block in BeginBlock), so a long trace/replay session grows the map monotonically too.
github.com/hashicorp/golang-lru/v2 is already a direct dependency — swapping the sync.Map for lru.Cache[uint64, common.Hash] sized a bit above MaxBlockHashHistory (e.g. 512) bounds memory, removes the need for manual eviction in TrackBlockHash entirely, and eliminates the stale-entry semantics noted below. Alternatively, simply don't cache the HistoricalInfo fallback result.
Related (latent, not currently exploitable): once a height is in this map it is returned even after its KV ring entry is pruned — giga/deps/xevm/keeper/block_hash_test.go asserts precisely that behavior. Today GetHashFn is only consumed by vm.BlockContext.GetHash, and geth's opBlockhash enforces the 256-block window, so contracts can't observe it. But GetHashFn is an exported keeper API returning node-local values for out-of-window heights; a future caller (precompile, RPC helper) that doesn't enforce the window would get non-deterministic answers. Bounding the cache or adding an explicit h < ctx.BlockHeight()-MaxBlockHashHistory → common.Hash{} guard in GetHashFn would close this off.
|
|
||
| func (k *Keeper) getHistoricalHash(ctx sdk.Context, h int64) common.Hash { | ||
| height := uint64(h) //nolint:gosec | ||
| if cached, ok := k.blockHashCache.Load(height); ok { |
There was a problem hiding this comment.
[blocker] Same unbounded-cache issue as x/evm/keeper/keeper.go:335 (mirrored code, mirrored bug), and worse here because giga's eviction hook is less reliable — see the comment on app/app.go:368. Entries stored by the HistoricalInfo fallback at line 344 for heights below ctx.BlockHeight()-257 are never evicted by PruneBlockHashCache.
Fixing both keepers by replacing blockHashCache *sync.Map with a bounded lru.Cache[uint64, common.Hash] (hashicorp/golang-lru/v2, already a direct dependency) would let you delete PruneBlockHashCache, the app/app.go hook, and the manual blockHashCache.Delete in TrackBlockHash altogether.
|
|
||
| func newGigaBlockCache(ctx sdk.Context, keeper *gigaevmkeeper.Keeper) (*gigaBlockCache, error) { | ||
| if !ctx.IsTracing() { | ||
| keeper.PruneBlockHashCache(ctx) |
There was a problem hiding this comment.
[blocker] This prune is not guaranteed to run once per block, and the eviction scheme requires that it does. PruneBlockHashCache deletes exactly one key (ctx.BlockHeight()-1-MaxBlockHashHistory), so any block for which it is skipped leaks that height's entry permanently.
newGigaBlockCache has two call sites: app/app.go:1539 (ProcessTxsSynchronousGiga) and app/app.go:1826, where it sits inside if len(evmEntries) > 0. Blocks containing no EVM transactions therefore never prune — a common case, and one an operator has no control over. Over time giga's blockHashCache retains an entry for every EVM-tx-free height.
Beyond the leak, cache maintenance in a block-cache constructor on the tx-processing path is a surprising place for it, and the two call sites make it easy for a future third path to miss. Either move the prune to a per-block hook that always runs (mirroring EvmKeeper.BeginBlock, which does run unconditionally via app/legacyabci/begin_block.go:73), or — preferably — make the cache a bounded LRU so no explicit prune hook is needed at all.
| const MaxBlockHashHistory = 256 | ||
|
|
||
| func (k *Keeper) GetBlockHash(ctx sdk.Context, height int64) (common.Hash, bool) { | ||
| store := ctx.KVStore(k.GetStoreKey()) |
There was a problem hiding this comment.
[suggestion] These three functions reach for ctx.KVStore(k.GetStoreKey()) directly, but every other store accessor in this package goes through k.GetKVStore(ctx) (keeper.go:241), which routes to ctx.GigaKVStore(...) unless UseRegularStore is set — see address.go, code.go, fee.go, nonce.go, log.go, receipt.go, state.go.
I believe this is deliberate: EvmKeeper.TrackBlockHash writes the ring via ctx.KVStore, so the reader must match the writer. But that reasoning isn't stated anywhere, and the file-level comment on PruneBlockHashCache only says "shared store". Please add a short comment explaining why this bypasses GetKVStore, so a future refactor that moves giga EVM state fully behind GigaKVStore doesn't silently turn GetBlockHash into an always-miss (which would degrade to the HistoricalInfo path and quietly undo this PR's perf win in giga mode, with no test catching it).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Replace the unbounded sync.Map with a fixed-size LRU and only insert from DeliverTx, so process memory stays bounded while tip lookups stay hot. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The 256-entry on-chain hash ring is a sound design and the pruning window (H-256..H-1 after BeginBlock at H) exactly matches what the EVM's BLOCKHASH opcode can ask for; the earlier round's blocking finding (unbounded sync.Map) is resolved by the bounded, thread-safe golang-lru/v2 cache with deliver-only inserts. Remaining items are non-blocking: the mem-cache read isn't bounded to the on-chain window, debug_trace* now serves BLOCKHASH(parent) from a different source than consensus does, and no test asserts the two sources agree.
Findings: 0 blocking | 14 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes:
cursor-review.mdis empty (Cursor produced no output for this synthesis, even though Cursor Bugbot posted a PR-level comment claiming 1 issue), andcodex-review.mdreports "No material issues found." So this review is essentially a single-pass synthesis. - Verified resolved from the previous review round: the unbounded process-local
sync.Mapis gone, replaced bylru.Cache[uint64, common.Hash](thread-safe, so concurrent RPC reads are fine) held behind a pointer so the by-valueapp.EvmKeeper = *NewKeeper(...)copy still shares one cache. No action needed. - Missing test for the invariant this PR now depends on: that
BlockHeader().LastBlockId.Hashat height H+1 equalsHistoricalInfo(H).Header.Hash(). BothTestGetHashFnvariants pin the two paths to unrelated synthetic hashes, so nothing would catch a divergence. Atestutil/processblock-driven test that runs a real block and asserts ring == HistoricalInfo ==ctx.HeaderHash()would cover this, the first-256-blocks-after-activation window, and the trace path in one go. - No test covers the
!ctx.IsTracing()gate onTrackBlockHash(i.e. that trace/replay never writes the ring), and giga has no end-to-end coverage that the ring it reads is actually populated byx/evm's BeginBlock — giga'sSetBlockHash/DeleteBlockHashare only exercised from tests. TrackBlockHashruns unconditionally on every BeginBlock with no upgrade-name gate. Per REVIEW_GUIDELINES §1 I am not flagging a missing tag/handler, and theapp-hash-breakinglabel shows awareness — just confirm this ships in a coordinated app-hash-breaking release, since the new store writes change the app hash from the first block the binary executes.- Storage/perf trade-off worth stating explicitly given the
perfframing: oneSetplus oneDeleteper block against the EVM store forever, i.e. ~32-41 bytes/block retained permanently in the versioned state-store changelog. Inherent to an on-chain ring, but it is a real cost against the saved header rehash. - No benchmark numbers for a
perf-labeled, app-hash-breaking change. The repo has abenchmark/harness (benchmark/CLAUDE.md); a before/after figure for the BLOCKHASH path would substantiate the trade-off above. - I could not execute
go test/go vetin this sandbox, so the new tests are reviewed by reading only — please confirm CI is green on./x/evm/keeper/...and./giga/deps/xevm/keeper/.... - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| func (k *Keeper) getHistoricalHash(ctx sdk.Context, h int64) common.Hash { | ||
| height := uint64(h) //nolint:gosec | ||
| // Peek keeps LRU order driven by DeliverTx inserts only. | ||
| if cached, ok := k.blockHashCache.Peek(height); ok { |
There was a problem hiding this comment.
[suggestion] The mem-cache is consulted before the consensus ring and its contents are not bounded to the ring window: capacity is MaxBlockHashHistory * 2 (512) and eviction is by recency, not by height, so an entry can survive long after TrackBlockHash pruned that height from the KV ring (e.g. a height looked up once and then no BLOCKHASH traffic for thousands of blocks). This is a DeliverTx path, so a long-running node and a freshly restarted node returning different answers for the same height would be a consensus divergence.
Today this is unreachable only because geth's opBlockhash (and evmone's blockhash) clamp the argument to [H-256, H-1], which is exactly what the ring holds — i.e. safety depends on an invariant enforced outside this package. Cheap defense-in-depth: only consult/insert the cache when h >= ctx.BlockHeight()-MaxBlockHashHistory, and/or size blockHashCacheSize to MaxBlockHashHistory rather than 2x. Same applies to the mirror at giga/deps/xevm/keeper/keeper.go:331.
| if !ctx.IsTracing() { | ||
| k.SetMsgs([]*types.MsgEVMTransaction{}) | ||
| k.SetTxResults([]*abci.ExecTxResult{}) | ||
| k.TrackBlockHash(ctx) |
There was a problem hiding this comment.
[suggestion] Gating TrackBlockHash on !ctx.IsTracing() is right (the replay ctx must not write the ring), but it introduces a source asymmetry between consensus and tracing that didn't exist before this PR.
evmrpc Backend.initializeBlock builds the replay ctx from ctxProvider(H-1) — state at version H-1 — and then calls legacyabci.BeginBlock. At version H-1 the ring holds up to H-2, and the write for H-1 is skipped here, so BLOCKHASH(H-1) during debug_trace* falls back to HistoricalInfo, while consensus execution of block H serves it from the ring. BLOCKHASH(block.number-1) is the most common usage, so if the two sources ever disagree, traces silently diverge from what the chain executed — with no test asserting they agree (see the PR-level note).
Note the naive fix is wrong: initializeBlock only overrides height and time on the base ctx, so ctx.BlockHeader().LastBlockId there is not the traced block's parent — moving the call out of the guard would record a bogus hash. If you want trace fidelity from the ring, the traced block's LastBlockId has to be threaded in explicitly.
| EvmOnlyBlockBloomPrefix = []byte{0x1d} | ||
| ZeroStorageCleanupCheckpointKey = []byte{0x1e} | ||
| NonceBumpPrefix = []byte{0x1f} // transient | ||
| BlockHashPrefix = []byte{0x20} |
There was a problem hiding this comment.
[suggestion] BlockHashPrefix isn't added to the prefix lists that drive ExportGenesis/ExportGenesisStream in x/evm/genesis.go (~lines 69-75 and ~146-151), so after an export/import the ring starts empty and BLOCKHASH falls back to HistoricalInfo (also not exported) — zero for up to 256 blocks. That matches today's behavior and is plausibly deliberate; worth confirming it's a choice rather than an omission.
|
|
||
| // MaxBlockHashHistory is the number of recent block hashes retained for BLOCKHASH | ||
| // (Yellow Paper / EVM: only the previous 256 blocks are available). | ||
| const MaxBlockHashHistory = 256 |
There was a problem hiding this comment.
[nit] MaxBlockHashHistory is duplicated here and in x/evm/keeper/block_hash.go:11. x/evm owns the KV window; giga only reads it, so if the two ever drift, giga would serve mem-cached hashes for heights already pruned from the shared ring. Consider referencing the x/evm constant (or adding a compile-time assertion that they match).
| return common.BytesToHash(bz), true | ||
| } | ||
|
|
||
| func (k *Keeper) SetBlockHash(ctx sdk.Context, height int64, hash common.Hash) { |
There was a problem hiding this comment.
[nit] SetBlockHash/DeleteBlockHash have no production callers on the giga side — x/evm's TrackBlockHash owns the ring (both keepers share store key "evm" and UseRegularStore is set to true in app.go, so the shared-store comment above checks out). Worth a doc comment saying these exist for tests only, or unexporting them, so a future caller doesn't assume giga maintains the ring.
| require.Equal(t, fallbackHash, f(uint64(histHeight))) | ||
| _, found := k.GetBlockHash(ctx, histHeight) | ||
| require.False(t, found) | ||
| require.Equal(t, fallbackHash, k.GetHashFn(ctx)(uint64(histHeight))) |
There was a problem hiding this comment.
[nit] This assertion is vacuous as written: HistoricalInfo(6) is still set, so GetHashFn returns fallbackHash whether or not the previous call populated the mem-cache. To actually pin the insert, remove the HistoricalInfo entry (or query from a keeper whose store lacks it) before re-querying — the way TestBlockHashCacheDeliverOnly does with DeleteBlockHash.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
No bugs found in the current state of this PR. Given the app-hash-breaking label and that this touches consensus-critical EVM opcode handling (BLOCKHASH), plus the number of blocker-level findings raised earlier in review, I'd still like a human to take a final look before merge.
Beyond the inline findings from this run, I verified that the earlier blocker-level concerns from this thread (unbounded blockHashCache growth, and the giga-side prune not running on EVM-tx-free blocks) were resolved by commit 90ab9f5 ("bound BLOCKHASH mem cache with deliver-only LRU"), which replaced the manual sync.Map + single-key eviction with a bounded LRU that only inserts from DeliverTx, and removed the now-unnecessary PruneBlockHashCache call sites in app/app.go entirely.
Extended reasoning...
This PR adds a 256-block BLOCKHASH ring persisted in the EVM store (new BlockHashPrefix key, populated in BeginBlock via TrackBlockHash), backed by a process-local bounded LRU cache for hot lookups, with a mirrored implementation in giga/deps/xevm. It changes state written every block in a way that affects the app hash (per the PR's own label), and changes BLOCKHASH resolution behavior for both the sync.Map-era paths (fixed already) and the historical-info fallback used by RPC/trace.
I confirmed via git history that the earlier blocker-severity issues raised in this thread (unbounded process-memory growth from caching arbitrary historical heights, and giga's prune being skipped on EVM-tx-free blocks) were addressed in the latest commit by switching to a bounded, deliver-only LRU cache and removing the manual prune hooks. I did not find new issues in the current diff beyond what was already discussed and resolved.
Given the app-hash-breaking label, the consensus-critical code path (BLOCKHASH opcode + BeginBlock state writes), and the fact multiple independent reviewers (including this bot) previously caught real blocker-level bugs in this same PR, I think this warrants a human's final sign-off rather than an automated approval, even though the current diff looks correct to me.
Remove the fallback entry before the second lookup so the hit can only come from the process cache. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The BLOCKHASH ring is correctly implemented and value-preserving: I verified the new 0x20 prefix doesn't collide, the pruning arithmetic keeps the KV ring exactly aligned with geth's [N-256, N-1] window, query contexts are isCheckTx=true (so RPC never poisons the mem cache), and LastBlockId.Hash provably equals the legacy HistoricalInfo[h].Header.Hash() because sei-tendermint passes the full block header in RequestFinalizeBlock. No blockers; the notes below are about test rigor, cache sizing headroom, and the duplicated x/evm ↔ giga code.
Findings: 0 blocking | 9 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported "No material issues found." - No test asserts the new ring value equals the legacy
HistoricalInfo-derived value for the same height. BothTestGetHashFnvariants use an arbitrarykvHashfor the ring and a synthetic header for the fallback, so nothing guards the equivalence. SinceDefaultHistoricalEntries = 10000, both sources cover the entire 256-block BLOCKHASH window and this is a silent source swap for every in-window height. I verified analytically that they agree (sei-tendermint passesblock.Header.ToProto()inRequestFinalizeBlock, soHistoricalInfo[h].Header.Hash()== block h's ID hash == block h+1'sLastBlockId.Hash), but an integration test that produces real blocks and comparesk.GetBlockHash(ctx, h)againstgetHistoricalHash's old path would lock that in. - State-machine-breaking with no in-code version gate:
TrackBlockHashwrites new keys from the first block the binary runs, unlike e.g.x/evm/keeper/params.gowhich gates onctx.ClosestUpgradeName(). Per REVIEW_GUIDELINES §1 this is fine if it ships in a coordinated upgrade release (theapp-hash-breakinglabel suggests it will) — flagging only so release coordination is explicit, not as a defect. MaxBlockHashHistory,blockHashCacheSize,newBlockHashCache,cacheBlockHash, and the three ring accessors are copy-pasted betweenx/evm/keeperandgiga/deps/xevm/keeper. The two copies have already drifted in their tests (see the inline note on the gigaTestGetHashFn); consider a shared helper or at least a cross-reference comment.- This is a perf PR with no benchmark numbers and no cache hit-rate metric. A
benchmark/run or a counter on the LRU hit/miss path would make the win measurable and detectable if it regresses. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| require.Equal(t, fallbackHash, f(uint64(histHeight))) | ||
| _, found := k.GetBlockHash(ctx, histHeight) | ||
| require.False(t, found) | ||
| require.Equal(t, fallbackHash, k.GetHashFn(ctx)(uint64(histHeight))) |
There was a problem hiding this comment.
[suggestion] This assertion is vacuous here. The x/evm twin deletes the historical entry first:
// Remove HistoricalInfo so a second lookup can only succeed via the mem-cache.
testApp.StakingKeeper.DeleteHistoricalInfo(ctx, histHeight)Without that call, the second GetHashFn(ctx)(histHeight) succeeds via the HistoricalInfo fallback again, so it proves nothing about the mem-cache. sei-cosmos/x/staking/keeper/historical_info.go:30 exposes DeleteHistoricalInfo, so the same two lines work here — please mirror them to keep the giga and x/evm tests in sync.
| const MaxBlockHashHistory = 256 | ||
|
|
||
| // blockHashCacheSize bounds the process-local BLOCKHASH cache (tip window plus headroom). | ||
| const blockHashCacheSize = MaxBlockHashHistory * 2 |
There was a problem hiding this comment.
[suggestion] Sizing the LRU at 2× the ring means the cache can serve heights the KV ring has already pruned — block_hash_test.go explicitly asserts this (DeleteBlockHash(ctx, 7) then still expects hash back). That's safe today only because geth's opBlockhash clamps to [N-256, N-1] before ever calling GetHash, so getHistoricalHash is never invoked outside the ring window during execution.
That invariant is load-bearing but implicit, and GetHashFn is exported and handed straight to vm.BlockContext.GetHash. Two long-running nodes with different cache contents would still agree today (the fallback covers the same heights, DefaultHistoricalEntries = 10000), but any future caller outside the 256-block window would get a stale entry that outranks an authoritative "pruned" store state.
Suggest either sizing the cache to MaxBlockHashHistory so it can't outlive the ring, or spelling out the geth-clamp invariant in the comment here.
| } | ||
|
|
||
| // Block-hash ring accessors use ctx.KVStore (not GetKVStore / GigaKVStore) so | ||
| // they read the same store EvmKeeper.TrackBlockHash writes on BeginBlock. |
There was a problem hiding this comment.
[nit] Worth stating the asymmetry explicitly: this package never writes or prunes the ring — it depends entirely on x/evm's TrackBlockHash running in the evm module's BeginBlock (which it does; app.GigaEvmKeeper shares keys[evmtypes.StoreKey] and UseRegularStore = true in production, so ctx.KVStore here is the same store). If a giga-only execution path ever bypassed that BeginBlock, this would silently degrade to the HistoricalInfo fallback forever with no signal.
Relatedly, MaxBlockHashHistory on line 12 is only used for cache sizing here (nothing prunes), and DeleteBlockHash is only reachable from tests — a one-line note would prevent someone assuming this file owns the ring lifecycle.
| require.False(t, found) | ||
| parent, found := k.GetBlockHash(ctx, 256) | ||
| require.True(t, found) | ||
| require.Equal(t, nextParent, parent) |
There was a problem hiding this comment.
[nit] The pruning test verifies that height 0 is removed and 256 is written, but not that the entries in between survive. Adding _, found = k.GetBlockHash(ctx, 9); require.True(t, found) after the second TrackBlockHash would pin down that pruning removes exactly one key rather than over-deleting — that's the property most likely to break if the height - 1 - MaxBlockHashHistory arithmetic is ever touched.
There was a problem hiding this comment.
Beyond the reported nit, I checked two other candidates and ruled them out: the x/evm/AGENTS.md ABCI Lifecycle section not mentioning the new TrackBlockHash BeginBlock step (a doc-lag issue, not a behavioral bug), and BlockHashPrefix being omitted from ExportGenesis/ExportGenesisStream (this mirrors HistoricalInfo's own non-export today, so BLOCKHASH just falls back to empty for up to 256 blocks post-import — the same as current behavior, not a regression).
Extended reasoning...
This PR is consensus-critical (app-hash-breaking) and had several blocker-level findings from other reviewers about an unbounded process-memory cache and unreliable giga-side pruning; those appear to have been addressed in the later commits (bounded LRU cache, deliver-only inserts). The only new finding from this run is a nit: the giga copy of TestGetHashFn still has the vacuous mem-cache assertion that was already fixed on the x/evm side. Given the severity and history of findings on this PR, it still warrants a human's sign-off rather than an AI approval.
Mirror the HistoricalInfo delete in giga's TestGetHashFn and assert a mid-window ring entry survives prune. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
The design is sound — the ring/prune arithmetic is correct, the LRU guard genuinely excludes CheckTx and all evmrpc contexts (which are trace-mode), and the 0x20 prefix is free in both key tables. However, this app-hash-breaking PR switches BLOCKHASH to a new data source without any test asserting the new ring value equals the HistoricalInfo-derived value it replaces, plus some test/design cleanups.
Findings: 2 blocking | 8 non-blocking | 5 posted inline
Blockers
cursor-review.mdis empty — the Cursor pass produced no output, so this PR effectively has only one third-party automated review (Codex, which reported no material issues).- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Justify the KV ring vs. mem-cache-only.
DefaultHistoricalEntriesis 10000, soHistoricalInfoalready covers the entire 256-block BLOCKHASH window — the ring stores data the staking module already has. The process-local LRU alone would capture most of the win (skipping the proto decode +header.Hash()Merkle computation) at zero state cost and with no app-hash break. The ring adds a Set+Delete per block forever and forces a coordinated upgrade. Worth including the benchmark numbers that motivated the on-chain half. TrackBlockHashhas no version/upgrade gate, so the new BeginBlock writes change the app hash from the first block the new binary produces. Per REVIEW_GUIDELINES §2 this is fine under Sei's coordinated-upgrade model and theapp-hash-breakinglabel — just confirm the release cuts an upgrade handler so all validators flip at the same height rather than shipping as a rolling release.- Consider a counter/metric distinguishing ring hits, mem-cache hits, and
HistoricalInfofallbacks, so the ring's hit rate (and therefore the perf claim) is observable in production and regressions after the upgrade boundary are visible. - No prompt-injection or instruction-like content was found in the diff, title, or description.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| testApp.StakingKeeper.SetHistoricalInfo(ctx, histHeight, &hi) | ||
|
|
||
| kvHeight := int64(7) | ||
| kvHash := common.HexToHash("0x0102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f20") |
There was a problem hiding this comment.
[blocker] Nothing in this PR asserts that the two sources of truth agree.
getHistoricalHash now prefers the ring (LastBlockId.Hash recorded in BeginBlock) over HistoricalInfo (tmtypes.HeaderFromProto(histInfo.Header).Hash()). Since DefaultHistoricalEntries is 10000, HistoricalInfo already covers the full 256-block BLOCKHASH window — so post-upgrade the ring replaces the value the old path returned for every in-window height, rather than filling a gap.
This test uses an arbitrary kvHash at height 7 and a synthetic header at height 6, so the two paths are never compared. If a proto round-trip or an unpopulated ABCI header field ever makes HeaderFromProto(...).Hash() != LastBlockId.Hash, BLOCKHASH silently returns a different value than it does today — a consensus-visible semantic change the PR description doesn't claim.
Please add a test that advances a real block (so staking's TrackHistoricalInfo and EvmKeeper.TrackBlockHash both run for the same height) and asserts k.GetBlockHash(ctx, h) equals the HistoricalInfo-derived hash for that h. Cheap to write, and it's the one thing that makes the swap safe.
| func (k *Keeper) getHistoricalHash(ctx sdk.Context, h int64) common.Hash { | ||
| height := uint64(h) //nolint:gosec | ||
| // Peek keeps LRU order driven by DeliverTx inserts only. | ||
| if cached, ok := k.blockHashCache.Peek(height); ok { |
There was a problem hiding this comment.
[suggestion] Peek short-circuits ahead of the ctx-scoped store read, so a process-local map now sits in front of state on the DeliverTx path. Correctness rests entirely on the unstated invariant "a given height's hash is immutable and identical across cache, ring, and HistoricalInfo" — if that ever breaks, two nodes with different cache contents diverge on the app hash.
I believe the invariant holds today (only canonical hashes are inserted, zero hashes are never cached, and geth/evmone bound BLOCKHASH to 256 so the 512-entry LRU can't over-serve). Two cheap hardening steps:
- Bound the cache read to
[ctx.BlockHeight()-MaxBlockHashHistory, ctx.BlockHeight())so an out-of-window entry can never be served regardless of how it got in. - Spell the invariant out in a comment here — it's the load-bearing assumption of the whole change.
|
|
||
| _, found = k.GetBlockHash(ctx, 0) | ||
| require.False(t, found) | ||
| _, found = k.GetBlockHash(ctx, 9) |
There was a problem hiding this comment.
[suggestion] This jumps the header from height 10 straight to 257 and then asserts height 9 survives — which codifies the fact that TrackBlockHash prunes exactly one key per block and leaks the rest on any height gap.
That's harmless in production (Tendermint heights are contiguous, and BeginBlock runs for every one), but the test isn't checking the invariant that actually matters. Prefer looping over contiguous heights past 256 and asserting the ring holds exactly MaxBlockHashHistory entries, with h-256 present and h-257 absent. That would also catch an off-by-one in height - 1 - MaxBlockHashHistory if the constant or the stored offset ever changes.
|
|
||
| // Block-hash ring accessors use ctx.KVStore (not GetKVStore / GigaKVStore) so | ||
| // they read the same store EvmKeeper.TrackBlockHash writes on BeginBlock. | ||
| func (k *Keeper) GetBlockHash(ctx sdk.Context, height int64) (common.Hash, bool) { |
There was a problem hiding this comment.
[suggestion] The cross-module contract here is comment-only. I confirmed it holds — app.go:798 constructs GigaEvmKeeper with keys[evmtypes.StoreKey], the same key EvmKeeper writes through — but nothing tests it: giga/deps/xevm/keeper/block_hash_test.go writes via the giga keeper's own SetBlockHash, so it would still pass if the two keepers were pointed at different stores.
A test that calls EvmKeeper.TrackBlockHash(ctx) and then reads through GigaEvmKeeper.GetBlockHash(ctx, h) would pin the contract down. Related: MaxBlockHashHistory is declared in this file but only feeds blockHashCacheSize (giga never prunes the KV ring), and SetBlockHash/DeleteBlockHash have no non-test callers here — worth a note saying the write/prune side deliberately lives only in x/evm.
| NextBaseFeePerGasPrefix = []byte{0x1c} | ||
| EvmOnlyBlockBloomPrefix = []byte{0x1d} | ||
| ZeroStorageCleanupCheckpointKey = []byte{0x1e} | ||
| BlockHashPrefix = []byte{0x20} |
There was a problem hiding this comment.
[nit] This table skips 0x1f (taken by NonceBumpPrefix in x/evm/types/keys.go) and jumps to 0x20. 0x20 is free in both tables today, but the numbering has already drifted between the two copies and the next person adding a prefix here has no way to know 0x1f is spoken for. Add a // 0x1f reserved: NonceBumpPrefix in x/evm/types/keys.go (transient) line so the two stay allocatable in lockstep.
There was a problem hiding this comment.
No bugs found in this pass, but given this is an app-hash-breaking change to consensus-critical BLOCKHASH handling that already went through several rounds of blocker-level fixes, I think a human should review before merge rather than relying on shadow-approval.
What was reviewed: confirmed the earlier "unbounded sync.Map" blocker findings are resolved by the switch to a fixed-size lru.Cache in both x/evm and giga keepers, with inserts gated to DeliverTx only (!IsCheckTx && !IsReCheckTx && !IsTracing). Verified the giga OCC-path prune-reliability concern (conditional call site skipping zero-EVM-tx blocks) is now moot since giga no longer does manual single-key eviction — the LRU handles it. Also confirmed the previously-flagged vacuous assertion in giga's TestGetHashFn (mem-cache lookup succeeding via HistoricalInfo rather than the cache) is fixed in the current diff via DeleteHistoricalInfo before the final lookup, mirroring the x/evm test.
Still-open, lower-severity items from the thread that a human may want to weigh in on: the LRU is sized 2x the ring window so it can serve heights already pruned from the KV ring (safe today only because of an external invariant in geth's opBlockhash clamp, not enforced in this package); a trace-vs-consensus BLOCKHASH source asymmetry around ctx.IsTracing() with no test pinning agreement; and BlockHashPrefix not included in genesis export/import (plausibly deliberate, unconfirmed).
Extended reasoning...
Overview
This PR adds a 256-entry on-chain KV ring (BlockHashPrefix) populated in EvmKeeper.BeginBlock via TrackBlockHash, plus a bounded process-local LRU cache (hashicorp/golang-lru/v2, deliver-only inserts) to serve the EVM BLOCKHASH opcode, falling back to staking HistoricalInfo for older heights. The change is mirrored in giga/deps/xevm/keeper for the Giga executor, sharing the same underlying KV store. It carries the app-hash-breaking label since it changes how/where a consensus-relevant value is sourced.
Security / correctness risks
The main risk class here is consensus divergence: if the mem cache or KV ring ever returned a different value than a node computed fresh from HistoricalInfo, or if trace/RPC paths mutated state read during DeliverTx, two honest nodes (or a live node vs. a replay) could disagree on execution results. The PR history shows this was a real, not hypothetical, concern — the original implementation used an unbounded sync.Map with manual single-key eviction that leaked memory and (via the OCC-Giga path) could leave giga's cache un-pruned indefinitely. That was subsequently reworked into a bounded LRU with deliver-only inserts (confirmed in the current diff), which resolves the blocker-severity findings. No injection/auth-bypass surface is introduced; this is pure internal state/caching logic.
Level of scrutiny
This warrants a high level of scrutiny: it is an app-hash-breaking change to a chain-critical opcode implementation (BLOCKHASH), with a nontrivial cache-eviction and fallback-source design, mirrored across two keeper implementations that must stay in sync. Even though no new bugs surfaced in this pass, correctness here depends on subtle invariants (e.g., geth's opBlockhash clamp being the only thing preventing the LRU from serving stale pruned heights) that are enforced outside this package, not defended-in-depth within it. That is the kind of design tradeoff a human reviewer should explicitly sign off on rather than have shadow-approved.
Other factors
The PR already has substantial review history (Cursor bugbot, seidroid, and prior claude runs) that drove several rounds of fixes, and the author has responded to at least one finding as an intentional design choice (Autobahn's sparse header). New unit tests were added for both keepers covering the ring, prune boundary, and deliver-only cache semantics. Given the criticality of the code path and the amount of back-and-forth already needed to reach the current state, this is a case for a human final look rather than a fully mechanical review.
Cover the source swap by running TrackHistoricalInfo and TrackBlockHash for the same height and requiring equal hashes. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-tested change: the prune window is exactly [H-256, H-1] matching opBlockhash's bounds with no off-by-one, prefix 0x20 is collision-free in both key files, the shared-store assumption between x/evm and giga holds, and TestBlockHashRingMatchesHistoricalInfo covers the equivalence that makes the app-hash change safe. My main note is that the process-local LRU is consulted before the store with no height bound, so its determinism rests on an invariant enforced only by geth's opcode clamp — worth making local.
Findings: 0 blocking | 9 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues. BlockHashPrefixwas not added to the export/import prefix list inx/evm/genesis.go(lines ~69-76 and ~146-152), unlike the sibling height-keyed prefixesBlockBloomPrefixandTxHashesPrefix. After an export-genesis restart the ring would be empty for 256 blocks with no HistoricalInfo to fall back on. Likely acceptable, but worth confirming it's deliberate.- giga has no writer or pruner for the ring —
giga/deps/xevm/keeper/block_hash.godefines Get/Set/Delete but onlyx/evm'sBeginBlockever populates it, andSetBlockHash/DeleteBlockHashthere are called only from tests. This works because both keepers are constructed withkeys[evmtypes.StoreKey](app.go:727 and app.go:798), but nothing enforces that coupling. Consider a test that runsEvmKeeper.TrackBlockHashand then reads throughGigaEvmKeeper.GetHashFn, so a future store-key split fails loudly. - Test gap:
cacheBlockHash'sIsCheckTx()/IsReCheckTx()skip branch is never exercised — bothTestBlockHashCacheDeliverOnlyvariants only coverWithTraceMode(true). Also missing a boundary test throughGetHashFnthat a height at exactlyH-MaxBlockHashHistoryresolves from the ring whileH-257does not; that boundary is the entire basis for the cache being safe. - This is labeled app-hash-breaking and
TrackBlockHashruns unconditionally, with nosemver.Compare(ctx.ClosestUpgradeName(), ...)gate — unlike neighboring consensus-affecting EVM logic (x/evm/keeper/params.go,pointer.go). Per REVIEW_GUIDELINES §1/§2 I'm not treating the absent tag/handler as a finding; just confirming the intent is a plain coordinated binary swap rather than a version gate. - Perf-motivated change with no telemetry: consider a counter for ring hit / mem-cache hit / HistoricalInfo fallback so the win (and the post-activation warm-up window) is measurable in production.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| func (k *Keeper) getHistoricalHash(ctx sdk.Context, h int64) common.Hash { | ||
| height := uint64(h) //nolint:gosec | ||
| // Peek keeps LRU order driven by DeliverTx inserts only. | ||
| if cached, ok := k.blockHashCache.Peek(height); ok { |
There was a problem hiding this comment.
[suggestion] The process-local LRU is consulted before the KV store and has no height-window bound, so on a consensus path the answer can depend on node-local memory state. TestBlockHashCacheDeliverOnly asserts this explicitly: after DeleteBlockHash(ctx, 7), GetHashFn(ctx)(7) still returns the hash.
That is safe today only because of an invariant enforced entirely outside this function — geth's opBlockhash clamps lookups to [BlockNumber-256, BlockNumber), which happens to coincide with the ring's retention window. Nothing here documents or enforces it. If a future caller reaches GetHashFn outside that window (EIP-2935 ProcessParentBlockHash, a precompile, a custom RPC path), a long-running validator with a warm cache and a freshly-restarted one would return different values for the same height.
Suggest making the invariant local, e.g. skip the cache entirely when h < ctx.BlockHeight()-MaxBlockHashHistory || h >= ctx.BlockHeight(), so the ring/HistoricalInfo remain the only sources of truth outside the window.
Same applies to the mirrored code at giga/deps/xevm/keeper/keeper.go:331.
| // Block-hash ring accessors use ctx.KVStore (not GetKVStore / GigaKVStore) so | ||
| // they read the same store EvmKeeper.TrackBlockHash writes on BeginBlock. | ||
| func (k *Keeper) GetBlockHash(ctx sdk.Context, height int64) (common.Hash, bool) { | ||
| store := ctx.KVStore(k.GetStoreKey()) |
There was a problem hiding this comment.
[suggestion] This bypasses k.GetKVStore(ctx), which every other giga accessor in this keeper goes through. The comment above explains the intent (read what EvmKeeper.TrackBlockHash writes), and it's equivalent today since app.go:802 sets UseRegularStore = true unconditionally — but the bypass also means these reads never enter giga's OCC/multiversion read set. Harmless right now because the ring is only written in BeginBlock, before parallel tx execution, but that's a second unstated invariant.
Since GetKVStore already resolves to ctx.KVStore under UseRegularStore, using it here would be equivalent today and stay correct if the flag is ever flipped. If you keep the direct ctx.KVStore, consider extending the comment to say why OCC tracking is not needed.
| return | ||
| } | ||
| parent := common.BytesToHash(ctx.BlockHeader().LastBlockId.Hash) | ||
| if parent != (common.Hash{}) { |
There was a problem hiding this comment.
[nit] Skipping the write on a zero LastBlockId.Hash is correct at height 1, but at any other height it silently leaves a permanent hole in the ring (the prune below still advances), and BLOCKHASH for that height quietly falls back to HistoricalInfo or returns zero. A ctx.Logger().Error(...) for height > 1 would make that observable rather than invisible.
Superseded: latest AI review found no blocking issues.
1 participant
Summary
LastBlockId.Hash) in the EVM store on BeginBlock, with a sliding prune of the oldest entryBLOCKHASHfrom that ring (plus a process-local mem cache), falling back to stakingHistoricalInfowhen a height is not yet in the ringgiga/deps/xevm(shared store; giga only prunes its mem cache)Made with Cursor