feature(sei-db) LTHash refactory for adding per module hash and stats#3758
Conversation
* main: fix(evm): bound store-cache depth amplification from stacked EVM snapshots (#3713) Fix address distro bug (#3716) fix(giga): route EVM validation failures to v2 fallback (CON-368) (#3753) feat(seidb): composite + replay digest modes and migration-boundary omission for evm-logical-digest (#3711) feat(scripts): add SC read-path RPC probe (consistency check + load) (#3751) fix(feegrant): bound DenomsSubsetOf cost and cap allowance denoms (#3710)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
An organization admin can view or raise the cap at claude.ai/admin-settings/claude-code. The cap resets at the start of the next billing period.
Once the cap resets or is raised, push a new commit or reopen this pull request to trigger a review.
PR SummaryHigh Risk Overview LtHash is refactored around a new
Extensive new tests cover per-module hash/stats invariants, persistence, import/restart, and metadata validation. Reviewed by Cursor Bugbot for commit e7c7e33. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A large but mostly mechanical Legacy→Misc rename plus a well-tested feature: per-module LtHash decomposition and per-module stats, refactored into a new shared-pool HashCalculator. The new code is sound for fresh stores (homomorphic invariants preserved, pool nesting deadlock-free); the only notable concerns are two upgrade-path correctness risks that the author has explicitly declared out of scope (FlatKV is pre-production, fresh stores only).
Findings: 0 blocking | 5 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Upgrade risk — per-module hash bootstrap (raised as P0 by Codex): on load, an existing store that has a per-DB root LtHash but no persisted per-module hashes gets an empty perDBModuleWorkingLtHash. On the first post-upgrade write, Compute()/importer flush replace the per-DB root with SumModuleHashes(newPerModule[dir]), which only reflects the current block's delta and silently drops all previously-committed entries, changing the consensus AppHash. This is correct for fresh stores and the PR explicitly states fresh-only/no-migration, but there is no on-load guard that enforces this — an old-format store would diverge silently. Consider refusing to load (or logging loudly) when a data DB has a non-empty per-DB root but no per-module hashes.
- Upgrade risk — directory rename (raised as P0 by Codex): renaming the on-disk legacy/ DB dir to misc/ has no migration or fallback, so an upgraded node opens a fresh empty misc/ DB and abandons existing non-EVM/misc state. Again consistent with the stated fresh-only scope, but there is no detection of a pre-existing legacy/ directory to fail fast. Same recommendation: add a guard or a one-time rename-on-open.
- VerifyLtHash (verifyLtHashInternal) still only recomputes and checks the per-DB roots; it does not verify the new per-module hash decomposition against a full scan. The per-module hashes are consensus-irrelevant bookkeeping and the test suite covers the invariant, but a production self-check that the per-module hashes sum to the per-DB root would catch drift in the new metadata.
- Cursor's second-opinion review file (cursor-review.md) is empty — that pass produced no output. REVIEW_GUIDELINES.md is also empty, so the review proceeded without repo-specific guidelines.
- Minor: in HashCalculator.Compute, the defensive branch that handles a delta whose Dir is not in c.dbDirs sets newPerDB[dir] via the touched loop but the global-sum loop only iterates c.dbDirs, so such a dir's hash would be excluded from the global hash. Currently unreachable (dirs always come from dataDBDirs), but the defensive code is subtly inconsistent.
There was a problem hiding this comment.
A clean, well-tested refactor that renames Legacy→Misc and adds per-module LtHash/stats bookkeeping without changing the consensus AppHash for fresh stores. However, the load path silently accepts a pre-PR on-disk store (per-DB root present, no per-module metadata) and then corrupts its per-DB root / global hash on the first write, so a loud-failure guard is needed before merge.
Findings: 2 blocking | 3 non-blocking | 1 posted inline
Blockers
- Silent AppHash corruption for pre-PR on-disk stores (Codex P1, confirmed).
loadGlobalMetadata(store.go:755) accepts a store whose per-DB root (meta.LtHash) is non-identity but whoseModuleLtHashesmap is empty, andHashCalculator.Compute(hash_calculator.go:205-206) then rebuilds each touched DB's root asSumModuleHashes(...)of the module map, which for such a store contains only this block's delta — discarding all pre-existing keys and producing a wrong per-DB root and global hash with no error. The PR deliberately excludes migration ("fresh stores only"), but nothing enforces that; the fix is to fail loudly on load when a per-DB root is present without per-module metadata (or reconstruct the module map via a full scan), rather than silently corrupting the consensus hash. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The on-disk directory rename
legacy/→misc/(store.go, snapshot.go) means any existing store's populatedlegacy/dir is ignored and a fresh emptymisc/is created. This is part of the same no-migration story as the blocker above; a loud check on startup would cover both cases. - Cursor's second-opinion review (cursor-review.md) and REVIEW_GUIDELINES.md were both empty, so no additional signal was available from those sources.
- Minor:
Computeclones the per-module hash/stats maps for all four data DBs on everyApplyChangeSetscall (potentially several per block), even for untouched DBs. Maps are small so impact is limited, but the clone-all-then-overwrite-touched pattern is slightly wasteful on the hot path.
| } else { | ||
| s.perDBWorkingLtHash[dbDir] = lthash.New() | ||
| } | ||
| if meta != nil { |
There was a problem hiding this comment.
[blocker] Silent AppHash corruption for pre-PR on-disk stores (confirmed). When an existing store has a per-DB root (meta.LtHash != nil, set just above) but no per-module metadata, meta.ModuleLtHashes loads as an empty map here. On the first write, HashCalculator.Compute recomputes each touched DB's root via SumModuleHashes(newPerModule[dir]) (hash_calculator.go:205-206), which contains only this block's delta — the pre-existing keys' contribution to the per-DB root (and thus the global store hash / AppHash) is silently discarded. The PR states migration is intentionally unsupported (fresh stores only), but this path doesn't enforce that; it corrupts instead of failing. Please fail loudly when a per-DB root is present without per-module metadata (e.g. meta.LtHash is non-identity but meta.ModuleLtHashes is empty), or reconstruct the module decomposition from a full scan on load.
There was a problem hiding this comment.
A large, well-tested refactor: a mechanical Legacy→Misc rename across FlatKV plus a new per-module LtHash/stats feature extracted into lthash.HashCalculator. The core logic (homomorphic decomposition preserving the global hash, deadlock-free shared worker pool, stats predicates matching the hash) is sound and heavily covered by tests; the only issues found are a gofmt violation that would fail CI and minor cosmetic/process notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Both second-opinion passes produced essentially no findings: Cursor's file was empty, and Codex reported no material issues but noted its focused tests could not run because the Go 1.25.6 toolchain download is network-blocked. I was likewise unable to execute the Go test suite in this environment, so the correctness assessment of the new lthash/stats logic is based on code reading plus the PR's (comprehensive) in-tree tests rather than a fresh test run.
- Cosmetic only: several diagnostic printf/format strings in sei-db/tools/cmd/seidb/operations/evm_logical_digest.go were left with the old column widths after the rename (e.g.
misc count=%dandFINAL_DIGEST account+code+storage+misc), so the tool's aligned stdout columns are now slightly misaligned ("misc" is shorter than "legacy"). No functional impact. - Optional: LtHashThreadsPerCore is added to flatkv config but (like the existing MiscPoolThreadsPerCore/ReaderThreadsPerCore) is not surfaced to any app.toml wiring, so it is only tunable via DefaultConfig(). This matches the existing convention and is not a regression, but if operators are expected to tune the LtHash pool it will need explicit config plumbing.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| "code": {lines: 1}, // 1 code | ||
| "storage": {lines: 3}, // 3 storage slots | ||
| "legacy": {lines: 1}, // 1 bank row | ||
| "misc": {lines: 1}, // 1 bank row |
There was a problem hiding this comment.
[suggestion] gofmt violation: after renaming "legacy": to "misc": the composite-literal values are no longer aligned — "misc": {lines: 1} has 2 spaces where gofmt requires 4 (to align with "account":/"storage":). golangci-lint's gofmt formatter lints test files too, so CI (.github/workflows/golangci.yml) will fail on this. Run gofmt -s -w on the file to fix.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
A large, well-tested refactor renaming Legacy→Misc and adding per-module LtHash/stats decomposition plus a shared HashCalculator worker pool; the core logic is sound, but a gofmt misalignment in a test file will fail the golangci-lint CI gate.
Findings: 2 blocking | 4 non-blocking | 1 posted inline
Blockers
- sei-db/tools/cmd/seidb/operations/dump_flatkv_test.go is not gofmt-clean (the
"misc":map entry is misaligned), so the golangci-lint v2.8.0 gofmt formatter in .github/workflows/golangci.yml will fail. Rungofmt -s -w/goimports -won the touched files. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- REVIEW_GUIDELINES.md is empty and cursor-review.md produced no output; Codex reported no material issues. No repo-specific standards were available to apply beyond AGENTS.md.
- On-disk directory rename legacy/ → misc/ has no migration path: an existing store's legacy/ dir would be silently ignored and a fresh misc/ created. This is acknowledged in the PR (FlatKV not yet in production) and is acceptable, but worth confirming no non-production store relies on it.
- ss/evm/types.go keeps the exported constant name StoreLegacy while pointing it at commonevm.EVMKeyMisc; leaving the old public name is fine but slightly inconsistent with the rest of the rename — consider a follow-up if that symbol is meant to track the Misc naming.
- loadModuleLtHashes and loadModuleStats each open a separate iterator over the identical
_meta/x:range and filter by suffix; a single pass could populate both maps, though the current cost is negligible at startup.
There was a problem hiding this comment.
A large, well-structured FlatKV refactor that renames the Legacy* catch-all DB to Misc*, extracts the lattice-hash pipeline into a shared HashCalculator with a fixed worker pool, and adds crash-recoverable per-module LtHash + key/byte stats decomposition. The bulk is mechanical rename churn plus new decomposition logic backed by strong "incremental == full-scan" / persistence / import-restart tests; no blocking issues found, with one hardening suggestion.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage:
cursor-review.mdandREVIEW_GUIDELINES.mdare both empty — the Cursor pass produced no output and there were no repo-specific guidelines to apply. Codex produced a single finding (incomplete per-module load validation), addressed inline. - The per-module root is recomputed from the module map only for DBs touched in a block (
HashCalculator.Compute→SumModuleHashes(newPerModule[dir])), which makes the persisted per-DB root ↔ module-map consistency invariant load-bearing for consensus (a drifted map silently rebuilds a wrong root, hence a wrong global/AppHash on the next write). This is well guarded for the migration case and covered byVerifyLtHash, but that verifier is a full scan not run at startup — consider documenting/asserting the invariant more defensively (see inline note on store_meta.go). LtHashThreadsPerCoreis added to the internal flatkv config with a default of 1.0; confirm it is (or intentionally is not) surfaced in the operator-facing app config so the new pool can be tuned in production.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if meta == nil || meta.LtHash == nil || meta.LtHash.IsZero() { | ||
| return nil | ||
| } | ||
| if len(meta.ModuleLtHashes) == 0 { |
There was a problem hiding this comment.
[suggestion] validatePerModuleMetadata only rejects the empty module map (the pre-per-module migration case). It does not verify that the persisted per-module hashes actually reconstruct the persisted per-DB root, i.e. SumModuleHashes(meta.ModuleLtHashes).Equal(meta.LtHash). If a subset of module /hash keys were lost while the root key survived, len(...) != 0 passes here, and the next write that touches this DB rebuilds its root as SumModuleHashes(perModule[dir]) over an incomplete map — silently dropping a module's contribution to the per-DB root and thus the global store hash / AppHash. This merges Codex's finding: because writeLocalMetaToBatch writes the root and all module hashes in one atomic Pebble batch, silent partial loss is not reachable under normal operation, so this is a hardening suggestion rather than a live bug. But the check is essentially free (SumModuleHashes already exists) and would also catch any drift bug in the new decomposition code at load time, guarding a consensus-critical invariant. Consider adding the sum-equality assertion (and optionally the same for ModuleStats) alongside the existing non-empty check.
seidroid
Bot
dismissed
their stale review
Superseded: latest AI review found no blocking issues.
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
One big missing element is HashLogger integration. But I think it would be ok if we did the HashLogger as a follow up PR since it's not going to be a 5 line fix. If you'd prefer to do it later, we should probably file a ticket.
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
Giving my "Approve" review now since none of my comments are blocking. Might want to give @blindchaser an opportunity to review if he wants, since I think he was the original author of the hashing logic.
| // TODO:In theory, we could offload lattice hash calculation to a work pool and get parallelism between DB operations and hash calculations. Cryptosim performance makes me think we could probably get a 2-3x speedup from this, assuming receiving data from the network isn't the bottleneck. | ||
| newHash, _ := lthash.ComputeLtHash(w.ltHash, w.ltPairs) | ||
| w.ltHash = newHash | ||
| // Per-module hashes are the primitive: distribute this batch's pairs across |
There was a problem hiding this comment.
Not triggered by any current feeder, both the flatkv→flatkv export (RawGlobalIterator over committed Pebble rows) and the memiavl importer (translator merges accounts, injective per kind) emit each physical key once.
But the importer's LtHash accumulation is non-idempotent by construction (assumes empty target, only MixIn, never MixOut), so a duplicate key would double-mix the hash and double-count stats while Pebble keeps one row, and FinalizeImport persists it without VerifyLtHash. (not introduced by this PR, it's the existing behavior)
Suggest a cheap fail-loud dedup check in dbWorker.run() to guard the invariant against future feeders / malformed snapshots. (Pre-existing on main; the refactor widens it to per-module hashes + stats.)
There was a problem hiding this comment.
Under what scenario would we export duplicate keys to the snapshot? If we add a dedup logic, the concern is that we have to fit every key into memory during the import process, which could fill up memory overhead and cause OOM
There was a problem hiding this comment.
If we hit a bad snapshot today which does contains duplicate keys, then the calculated lattice hash should detect the difference and error out the import at the end right?
yzang2019
changed the title
| s.phaseTimer.SetPhase("apply_change_compute_lt_hash") | ||
| res, err := s.ltCalc.Compute(pairSets, s.perDBWorkingLtHash, s.perDBModuleWorkingLtHash, s.perDBModuleWorkingStats) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Pending writes before LtHash compute
High Severity
ApplyChangeSets runs prepareWrites (which maps.Copys into the pending-write maps) before ltCalc.Compute. If Compute returns an error, the call exits without updating working LtHash, per-module metadata, or pendingChangeSets, but the pending maps already hold the new values. A later Commit can persist those rows while metadata still reflects the pre-failure hash, diverging AppHash from on-disk state.
Reviewed by Cursor Bugbot for commit c0cad32. Configure here.
There was a problem hiding this comment.
A large but carefully-executed refactor of the not-yet-production FlatKV subsystem: a complete Legacy→Misc rename plus a new per-module lattice-hash calculator and per-module stats. The consensus-relevant global hash is provably unchanged (homomorphic decomposition) and is backed by extensive invariant tests; no correctness, security, or performance defects found.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion coverage: OpenAI Codex reported "No material issues found"; the Cursor review file (cursor-review.md) was empty, so that pass produced no output. My independent review aligns with Codex — no blockers.
- Naming drift (nit): sei-db/state_db/ss/evm/types.go keeps the constant name
StoreLegacywhile repointing it tocommonevm.EVMKeyMisc. Given the underlying store is now consistently called "misc" everywhere else, leaving theStoreLegacyalias may confuse future readers. Intentional/harmless, but worth a follow-up rename for consistency. - Config edge (nit): Config.Validate() only rejects LtHashThreadsPerCore < 0, so a value of 0 passes validation and is silently clamped to a single worker by lthashWorkerCount(). This is documented and reasonable, but a 0 value producing 1 worker (rather than an error or an explicit 'disabled' meaning) is slightly surprising.
- Verified the rename is symbol-complete: grep found no remaining EVMKeyLegacy/LegacyData/legacyDB references in production code, and the seidb dump tool now emits a
miscbucket file matching the updated integration-test digest scripts.
| func validatePerModuleMetadata(dbDir string, meta *ktype.LocalMeta) error { | ||
| if meta == nil || meta.LtHash == nil { | ||
| return nil | ||
| } | ||
| if len(meta.ModuleLtHashes) == 0 { | ||
| if meta.LtHash.IsZero() { | ||
| return nil | ||
| } | ||
| return fmt.Errorf( | ||
| "flatkv: %s carries a non-identity per-DB LtHash root but no "+ | ||
| "per-module metadata; this store predates per-module hashing "+ | ||
| "and cannot be opened (migration is intentionally unsupported — "+ | ||
| "recreate the store from a snapshot or genesis)", | ||
| dbDir, | ||
| ) | ||
| } | ||
| sum := lthash.SumModuleHashes(meta.ModuleLtHashes) | ||
| if !meta.LtHash.Equal(sum) { | ||
| return fmt.Errorf( | ||
| "flatkv: %s per-module hashes do not sum to per-DB root (root=%x sum=%x)", | ||
| dbDir, meta.LtHash.Checksum(), sum.Checksum(), | ||
| ) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🟡 validatePerModuleMetadata only guards one direction of the root/per-module invariant: when meta.LtHash == nil it returns success immediately without checking whether meta.ModuleLtHashes is non-empty/non-zero. If a DB's _meta/hash key is lost while its _meta/x:<module>/hash keys survive, the store loads with perDBWorkingLtHash reset to identity while the surviving non-zero per-module hashes are still loaded into perDBModuleWorkingLtHash — breaking the root==sum(modules) invariant silently. The next write touching that DB rebuilds the root via SumModuleHashes over the stale module map, corrupting the per-DB root and the global AppHash with no error raised. Fix: in the meta.LtHash == nil branch, also reject when lthash.SumModuleHashes(meta.ModuleLtHashes) is non-zero, mirroring the existing check for the opposite direction.
Extended reasoning...
validatePerModuleMetadata (sei-db/state_db/sc/flatkv/store_meta.go:186-210) is meant to enforce the load-time invariant that a persisted per-DB root LtHash is consistent with its per-module decomposition. It already guards one direction of this invariant, added during this same PR's review: a non-identity root with an empty/incomplete module map is rejected, and a non-empty module map that doesn't homomorphically sum to the root is rejected. However, the very first branch — if meta == nil || meta.LtHash == nil { return nil } — returns success immediately whenever the per-DB root is nil, without ever inspecting meta.ModuleLtHashes in that branch.
loadLocalMeta (store_meta.go:46-56) sets meta.LtHash only when the _meta/hash key is found on disk, while loadModuleLtHashes (lines 58-62) independently populates meta.ModuleLtHashes from whatever _meta/x:<module>/hash keys survive. These are two separate reads of two disjoint key ranges. So a partial loss of _meta/* keys — where _meta/hash is gone but one or more _meta/x:<module>/hash keys survive with non-zero values — produces exactly meta.LtHash == nil with a non-empty, non-zero ModuleLtHashes map. The current code passes this straight through with no error.
The consequence shows up in loadGlobalMetadata (store.go, ~lines 758-774): when meta.LtHash == nil it resets s.perDBWorkingLtHash[dbDir] to lthash.New() (the identity hash), but a separate if meta != nil block unconditionally still loads s.perDBModuleWorkingLtHash[dbDir] = cloneModuleHashes(meta.ModuleLtHashes) — the surviving non-zero per-module hashes. Immediately after startup, the invariant root == sum(modules) is violated in memory (root=identity, modules=non-zero), with no error raised anywhere in the load path.
Step-by-step proof of impact:
- A store's storageDB commits normally, giving it a non-identity per-DB root and a non-zero
_meta/x:evm/hashentry (both written atomically bywriteLocalMetaToBatch). - Simulate corruption: delete only the
_meta/hashkey on disk (e.g. bit rot, a botched manual edit, or a partial write outside FlatKV's normal atomic-batch path), leaving_meta/x:evm/hashintact. - On reopen,
loadLocalMetasetsmeta.LtHash = nil(key not found) butmeta.ModuleLtHashes = {"evm": <non-zero>}(key found). validatePerModuleMetadatahits the first branch (meta.LtHash == nil) and returnsnil— no error — without looking atModuleLtHashes.loadGlobalMetadatasetsperDBWorkingLtHash[storageDBDir] = lthash.New()(identity) butperDBModuleWorkingLtHash[storageDBDir] = {"evm": <non-zero>}. The invariant is now broken in memory, silently.- The next
ApplyChangeSets/Computethat touches storageDB recomputes its root asSumModuleHashes(perModule[storageDBDir]), which now incorporates the stale/leftover module hash that has nothing to do with the DB's actual current contents relative to the (lost) prior root — silently corrupting the per-DB root and thus the globalevm_latticestore hash / consensus AppHash going forward. If that DB dir happens to never be touched again,perDBWorkingLtHashsimply stays at identity forever, permanently dropping that DB's contribution from the global hash — with no error, until someone happens to run the offlineVerifyLtHashtool, which is not run automatically on startup.
This is precisely the same corruption threat model (partial loss of _meta/* keys) that motivated the sibling checks added to this same function during PR review (the seidroid/cursor/blindchaser comments on this exact function) — just applied in the mirror direction (root lost vs. modules lost). writeLocalMetaToBatch writes the root and every module hash/stats key in the same atomic Pebble batch, so this genuinely cannot happen under normal FlatKV operation; it requires independent on-disk corruption of specific keys. That's the same reasoning that was already accepted as sufficient justification for the existing checks in this function, so by that same standard this is worth closing too.
Fix: in the meta.LtHash == nil branch, also reject when len(meta.ModuleLtHashes) != 0 (or more precisely, when lthash.SumModuleHashes(meta.ModuleLtHashes) is non-zero), mirroring the existing SumModuleHashes(...).Equal(root) check already present for the opposite direction.
There was a problem hiding this comment.
Confirmed unaddressed: store_meta.go's validatePerModuleMetadata still returns nil immediately whenever meta.LtHash == nil, without ever inspecting meta.ModuleLtHashes in that branch (store_meta.go:186-189). Verified against the current file on disk — the function body is unchanged from what's shown in the diff. cursor[bot] independently flagged the exact same gap later in the PR thread ('Nil root skips module hash validation', 2026-07-16T00:11) and it was never revised. The fix described in the original comment (reject when SumModuleHashes(meta.ModuleLtHashes) is non-zero, mirroring the existing check in the len(meta.ModuleLtHashes) == 0 branch) has not been applied.
There was a problem hiding this comment.
I didn't find any new issues in this pass, but this is a large, consensus-critical refactor of FlatKV's lattice-hash pipeline (per-module hash/stats decomposition, a new concurrent HashCalculator, across 65 files), and there's still an open, unaddressed Cursor Bugbot High-severity finding (pending-writes-before-Compute-error path) on the latest commit, so it's worth a human look.
What was reviewed: the HashCalculator's ReadOldValues/Compute/ComputeModuleHashInfos pipeline for parallel-chunk merge correctness (MixIn/addition are commutative, so chunking/order doesn't affect the result); the store_apply.go pending-write-mutation-before-Compute-error path flagged as a candidate issue this run and ruled out; and the per-module residue/orphan-stats checks in verify.go and store_meta.go from earlier review rounds.
Extended reasoning...
Overview
This PR refactors FlatKV's lattice-hash (LtHash) subsystem: it renames the LegacyDB to MiscDB, adds per-module LtHash and key/byte-stats decomposition persisted per data DB, and extracts the per-block hash pipeline into a new lthash.HashCalculator backed by a dedicated fixed worker pool shared by the live-commit path and the state-sync importer. It touches 65 files, most within sei-db/state_db/sc/flatkv, plus config, tooling (dump/digest CLIs), and integration test scripts.
Security risks
No injection/auth/data-exposure surface here -- this is internal state-storage/consensus-bookkeeping code, not request-facing. The main risk category is correctness/consensus drift: an error in the hash pipeline could silently corrupt the per-DB or per-module LtHash without affecting the global AppHash (per-module hashes are documented as consensus-irrelevant bookkeeping), or in the worst case corrupt the per-DB root that does feed into AppHash. Several such drift scenarios were already caught by other reviewers in this thread (orphan per-module stats bypassing VerifyLtHash, incomplete per-module metadata validation on load, a partial-mutation-before-error path in ApplyChangeSets) -- some fixed, one (the pending-writes-before-Compute-error path, High severity per Cursor Bugbot on the latest commit) still open without an author response.
Level of scrutiny
This warrants a high level of scrutiny: it's a substantial refactor of a consensus-adjacent subsystem (LtHash feeds the AppHash for FlatKV-enabled nodes) with new concurrency (a shared worker pool used by both live commit and a per-DB-goroutine importer) and several load-time invariant checks whose failure modes are subtle (e.g. partial per-module metadata loss surviving undetected). FlatKV is explicitly pre-production (no on-disk migration/backward-compat is provided), which lowers the blast radius somewhat, but the density of real findings already surfaced by cursor bot, seidroid, and prior review passes in this same thread indicates the code is still accumulating edge-case fixes rather than being settled.
Other factors
Test coverage is extensive (incremental-equals-full-scan invariants, add/update/delete transitions, persistence-after-reopen, state-sync-import-survives-restart, for both hashes and stats), which is reassuring, but the still-open High-severity Cursor finding on the most recent commit ("harden verification") means the review thread is not yet settled. This run's bug hunters investigated a related concern in store_apply.go (pending-write maps mutated before Compute can fail) and did not find it currently exploitable, but that is a narrower question than Cursor's finding and does not resolve it. Given the size, consensus-adjacency, and the open unresolved finding, I'm deferring rather than approving.
| "flatkv: %s per-module hashes do not sum to per-DB root (root=%x sum=%x)", | ||
| dbDir, meta.LtHash.Checksum(), sum.Checksum(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Nil root skips module hash validation
Medium Severity
validatePerModuleMetadata returns success whenever LtHash is nil, even if ModuleLtHashes is non-empty. Load then installs a zero per-DB root while still rehydrating those module hashes. The next commit can persist that split and drop the DB’s contribution from the global store hash / AppHash — the failure mode this guard was added to prevent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 4baa0b2. Configure here.
There was a problem hiding this comment.
A large but well-structured FlatKV refactor: extracts the per-block LtHash pipeline into a shared, pool-backed HashCalculator, adds crash-recoverable per-module hashes + key/byte stats persisted in each DB's LocalMeta, and renames the Legacy* catch-all bucket to Misc*. The concurrency model is sound and thoroughly tested (incremental≡full-scan invariants, persistence-after-reopen, import-survives-restart, load-time validation guard); no blocking issues found, only minor cosmetic staleness.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Leftover cosmetic staleness in
sei-db/common/keys/evm_test.go: the test-casename:strings ("EVMAddressToSeiAddress goes to Legacy", "NonceTooShort goes to Legacy", etc.) and the line-67 comment still say "Legacy" even thoughwantKindwas updated toEVMKeyMiscin this PR. Rename to "Misc" for consistency with the rest of the rename. - Second-opinion reviews produced no actionable findings: the Codex pass reported "No material issues found in the PR diff" and the Cursor review file was empty.
REVIEW_GUIDELINES.mdwas also empty, so no repo-specific standards could be applied. - Verified deadlock-safety of the shared fixed LtHash pool: with
queueSize == workers,Submitcan block, butfoldChunktasks are self-terminating and eachcomputeDeltasParallelcall buffers its result channel to the full task count, so workers never block on send and blocked submitters always drain — the importer's per-DB goroutines (which are not pool workers) can safely share one pool. No change needed; noting since it is the subtlest part of the change. - Optional:
LtHashThreadsPerCoreis only defaulted inDefaultConfig/test config; if a future caller constructsflatkv.Configdirectly and forgets it, the zero value clamps to a single worker (a silent performance floor, not a correctness bug). Consider documenting that on the field or logging the resolved worker count at startup.
| return nil, counts, fmt.Errorf("failed to gather account updates: %w", err) | ||
| } | ||
| newAccountValues := deriveNewAccountValues(accountWrites, accountOld, blockHeight) | ||
| accountPairs := gatherLTHashPairs(newAccountValues, accountOld) | ||
| maps.Copy(s.accountWrites, newAccountValues) | ||
| newAccounts := deriveNewAccountValues(accountUpdates, accountOld, blockHeight) | ||
| accountPairs := gatherPairs(newAccounts, oldByDB[accountDBDir]) | ||
| maps.Copy(s.accountWrites, newAccounts) | ||
| counts.accountWrites = len(newAccounts) | ||
|
|
||
| storageWrites, err := processStorageChanges(changesByType[keys.EVMKeyStorage], blockHeight) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse storage changes: %w", err) | ||
| return nil, counts, fmt.Errorf("failed to parse storage changes: %w", err) | ||
| } | ||
| storagePairs := gatherLTHashPairs(storageWrites, storageOld) | ||
| storagePairs := gatherPairs(storageWrites, oldByDB[storageDBDir]) | ||
| maps.Copy(s.storageWrites, storageWrites) | ||
| counts.storageWrites = len(storageWrites) | ||
|
|
||
| codeWrites, err := processCodeChanges(changesByType[keys.EVMKeyCode], blockHeight) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse code changes: %w", err) | ||
| return nil, counts, fmt.Errorf("failed to parse code changes: %w", err) | ||
| } | ||
| codePairs := gatherLTHashPairs(codeWrites, codeOld) | ||
| codePairs := gatherPairs(codeWrites, oldByDB[codeDBDir]) | ||
| maps.Copy(s.codeWrites, codeWrites) | ||
| counts.codeWrites = len(codeWrites) | ||
|
|
||
| legacyWrites, err := processLegacyChanges(changesByType[keys.EVMKeyLegacy], blockHeight) | ||
| miscWrites, err := processMiscChanges(changesByType[keys.EVMKeyMisc], blockHeight) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse legacy changes: %w", err) | ||
| return nil, counts, fmt.Errorf("failed to parse misc changes: %w", err) | ||
| } | ||
| legacyPairs := gatherLTHashPairs(legacyWrites, legacyOld) | ||
| maps.Copy(s.legacyWrites, legacyWrites) | ||
|
|
||
| addKVPairs(s.ctx, accountDBDir, len(newAccountValues)) | ||
| addKVPairs(s.ctx, storageDBDir, len(storageWrites)) | ||
| addKVPairs(s.ctx, codeDBDir, len(codeWrites)) | ||
| addKVPairs(s.ctx, legacyDBDir, len(legacyWrites)) | ||
| miscPairs := gatherPairs(miscWrites, oldByDB[miscDBDir]) | ||
| maps.Copy(s.miscWrites, miscWrites) | ||
| counts.miscWrites = len(miscWrites) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return m | ||
| } | ||
|
|
||
| for _, cs := range changeSets { |
There was a problem hiding this comment.
The empty module name isn't newly accepted here, main's classifyAndPrefix has no guard either. But on main it was harmless (just a /key row folded into the single per-DB hash).
With per-module metadata it becomes fatal: it writes _meta/x:/hash, which ParseModuleLtHashKey rejects on reload, so the new sum validation then fails and the store won't reopen. So the refactor turns a latent no-op into a store-won't-open failure. Cheap guard: reject nil changeset / empty cs.Name at the top of the loop. (Only reachable via malformed input — module names are never empty in normal operation.)
worth to add:
if cs == nil {
return nil, fmt.Errorf("flatkv: nil changeset")
}
if cs.Name == "" {
return nil, fmt.Errorf("flatkv: changeset with empty module name")
}
There was a problem hiding this comment.
So through the normal rootmulti → SC path, this is indeed unreachable in practice — confirming the reviewer's "module names are never empty in normal operation" note. But FlatKV could also receive changesets from other feeders that don't necessarily go through that constructor (WAL replay of a possibly-corrupted changelog entry, import/migration tooling, any future direct ApplyChangeSets caller).
Given the failure mode is "store can never be opened again" rather than an immediate loud error at the mistake's source, a cheap guard at the boundary is worth it as defense-in-depth.
There was a problem hiding this comment.
A large mechanical Legacy→Misc rename plus a well-designed, thoroughly-tested per-module LtHash/stats feature. Consensus-relevant global hash is provably unchanged, the shared-pool concurrency is deadlock-free, and migration is rejected loudly. No blockers; one defense-in-depth suggestion and notes on the empty second-opinion inputs.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Defense-in-depth (raised by Codex as P1; downgraded here): the new guard rejects only an empty
cs.Name, but a module name containing/(e.g./foo) also mis-parses.ModulePhysicalKey+StripModulePrefixsplit on the first/, so such a name folds into module "", persists_meta/x:/hash(rejected byParseModuleLtHashKeyon reload), and makes the store permanently unopenable via the sum-to-root check. This is not reachable for real Cosmos store keys (hardcoded, never contain/), so it is not a blocker — but since you already guard empty names, consider also rejecting names containing/(or validating aStripModulePrefixround-trip) to make the guard complete for the whole malformed-input class. REVIEW_GUIDELINES.md(base-branch review standards) is empty, so no repo-specific conventions could be applied.cursor-review.mdis empty — the Cursor second-opinion pass produced no output. Only the Codex pass produced a finding.- Minor/no action needed: deleting all of a module's keys leaves a persisted zeroed
_meta/x:<module>/hash(+/stats) entry that is never garbage-collected. Harmless given the module set is tiny and bounded, but worth being aware of for long-lived stores. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // fails forever). Reject it up front instead; module names are | ||
| // never empty in normal operation (Cosmos SDK's NewKVStoreKey | ||
| // panics on an empty name), so this only guards malformed input. | ||
| if cs.Name == "" { |
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
This PR touches consensus-critical FlatKV persistence and metadata-loading logic, and the inline finding shows the state-sync import path can still write an unopenable per-module meta key (permanently bricking the store) even after this PR's live-commit guard — that alone warrants a human look before merge.
Extended reasoning...
Overview
This PR refactors FlatKV's lattice-hash subsystem to add per-DB, per-module LtHash and storage-stats bookkeeping, and renames the LegacyDB/LegacyData catch-all to Misc. It touches the live-commit path (store_apply.go, store_write.go), the state-sync import path (importer.go), metadata load/validate (store_meta.go), and an offline verifier (verify.go), plus a large volume of mechanical rename/test churn.
Security risks
No injection/auth-bypass concerns, but this is consensus-adjacent persistence code: a wrong per-module hash/stats invariant can either corrupt the AppHash-adjacent bookkeeping or (per the inline finding) make a FlatKV store permanently unopenable after ingesting an untrusted, peer-supplied state-sync snapshot with an empty module name. That is a real availability/liveness risk for a node doing state sync, since the guard this PR added in classifyAndPrefix is not mirrored at the importer boundary.
Level of scrutiny
This warrants full human review rather than shadow-approval: it's a consensus-adjacent storage engine with a non-trivial concurrency model (shared worker pool feeding both live commit and the importer), and the PR is still under active back-and-forth with the author (yzang2019) about exactly this defense-in-depth gap as of the latest commits. FlatKV is described as pre-production, which lowers the blast radius somewhat, but the failure mode described (unrecoverable store) is severe enough to want a maintainer's explicit sign-off on the fix, not just an automated pass.
Other factors
The PR already has extensive review activity (multiple cursor[bot] and claude[bot] passes, plus human review from yzang2019 and blindchaser) and good test coverage for the new per-module hash/stats bookkeeping. The one outstanding, confirmed-live bug (importer bypassing the empty-module-name guard) is exactly the kind of finding that should block an automated approval, even though the analogous live-commit-path guard was already merged.
| require.NotNil(t, d) | ||
| require.Equal(t, wantKeys, d.KeyCount) | ||
| require.Equal(t, wantBytes, d.Bytes) | ||
| } |
There was a problem hiding this comment.
Parallel stats test stays serial
Low Severity
TestComputeModuleHashInfosStatsParallel claims to cover the pooled chunk-merge path, but builds only computeChunkSize*3+7 (307) pairs. That is below parallelThreshold (1000), so ComputeModuleHashInfos always takes the serial path and never exercises computeDeltasParallel.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit dbf1701. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e7c7e33. Configure here.
| dst[module] = st | ||
| } | ||
| return dst | ||
| } |
There was a problem hiding this comment.
Duplicate module map cloners
Low Severity
cloneModuleHashes / cloneModuleStats in store_meta.go duplicate cloneModuleMap / cloneModuleStatsMap in lthash/hash_calculator.go. Same nil-safe deep-copy behavior lives in two packages, so a future fix to one copy can easily miss the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e7c7e33. Configure here.
There was a problem hiding this comment.
A large, mostly-mechanical LegacyDB→MiscDB rename plus a well-contained, well-tested per-module LtHash/stats feature (new HashCalculator over a dedicated fixed pool, load-time validation, and expanded VerifyLtHash). The refactor preserves the global/consensus LtHash for fresh stores and the concurrency design is sound; no blocking issues found, only a cosmetic nit and notes about missing second-opinion output.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review file (cursor-review.md) was empty — that pass produced no output.
- Codex reported no material issues but explicitly could not run tests (Go 1.25.6 toolchain unavailable offline); rely on CI's
go test -racefor the correctness-critical per-module hash/stats invariants rather than treating Codex's pass as test verification. - REVIEW_GUIDELINES.md is empty, so no repo-specific review standards could be applied.
- Positive/confirmation: the consensus-relevant global LtHash is unchanged for fresh stores (per-module hashes sum homomorphically to the per-DB root and global root), and validatePerModuleMetadata correctly fails loudly on pre-per-module or drifted bookkeeping — appropriate since migration is intentionally unsupported. No action needed.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| fmt.Printf("code count=%d bucket_digest=%X\n", d.code.count, d.code.acc) | ||
| fmt.Printf("storage count=%d bucket_digest=%X\n", d.storage.count, d.storage.acc) | ||
| fmt.Printf("legacy count=%d bucket_digest=%X\n", legacyCountForCompare, legacyForCompare) | ||
| fmt.Printf("misc count=%d bucket_digest=%X\n", miscCountForCompare, miscForCompare) |
There was a problem hiding this comment.
[nit] Column misalignment: "misc count=" uses 3 trailing spaces (width 7), whereas the adjacent account/code/storage lines pad the label to width 9 (e.g. "account count=", "storage count="). The old "legacy count=" was width 9; misc should use 5 trailing spaces to keep the diagnostic output aligned. Purely cosmetic.
3 participants
Summary
Refactors FlatKV's lattice-hash (LtHash) subsystem and extends per-DB metadata to track per-module hashes and storage stats. Also renames the
Legacy*catch-all DB toMisc*for clarity. FlatKV is not yet in production, so no on-disk migration/backward-compat is provided (fresh stores only).Changes
1.
LegacyDB→MiscDBrenameThe catch-all DB (everything that isn't account/code/storage — i.e. all non-EVM cosmos modules, plus some EVM misc keys) is renamed throughout:
legacy/→misc/vtype.LegacyData→vtype.MiscData,keys.EVMKeyLegacy→keys.EVMKeyMisc2. Per-module LtHash metadata
Each data DB now decomposes its per-DB root LtHash into additive per-module hashes, persisted under
_meta/x:<module>/hashin that DB's LocalMeta:account/code/storageDBs only ever carry theevmmodule.miscDB may carry several (evmplus cosmos modules such asgov,bank).FinalizeImport.3. LtHash computation refactor + threading model
Extracted the whole per-block LtHash pipeline into a new
lthash.HashCalculator:threading.FixedPool) shared by both the live-commit path and the state-sync importer.OldValueReaderinterface and extracts module names via an injectedModuleFunc, keepinglthashdecoupled frompebbledb/ktype.ApplyChangeSetsis now a thin orchestrator (classify → read-old → apply EVM value semantics → compute); EVM/cosmos domain semantics stay inflatkv.4. Per-module storage stats
Alongside each module's hash, we now accumulate key count and total bytes (physical key + serialized value = logical footprint), persisted under
_meta/x:<module>/stats:5. Config
LtHashThreadsPerCore(float) to size the dedicated LtHash pool:workers = int(LtHashThreadsPerCore * NumCPU), clamped to ≥ 1. Default1.0(LtHash is CPU-bound). Mirrors the existing reader/misc pool config convention.6. Tests
lthash:ModuleStatsmarshal round-trip / bad-length,foldChunkadd/update/delete accounting, pooled aggregation path.ktype:_meta/x:<module>/hashand/statskey round-trips, mutual non-aliasing, malformed-key rejection.flatkv: per-module hash & stats "incremental equals full scan" invariants, add→update→delete transitions, persistence-after-reopen, and state-sync-import-survives-restart.