feat(seidb): dump-flatkv computes per-bucket/total LtHash#3665
Conversation
…tle, metadata verify
PR SummaryLow Risk Overview LtHash (default on): The scan always accumulates per-bucket and global lattice hashes over all four buckets (even when ** — Read throttle: Tests cover batched vs single-shot LtHash equivalence, metadata probe behavior, skip-verify paths, and lthash-only mode. Reviewed by Cursor Bugbot for commit d843f92. 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).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3665 +/- ##
==========================================
- Coverage 59.26% 58.18% -1.09%
==========================================
Files 2272 2178 -94
Lines 188030 177356 -10674
==========================================
- Hits 111442 103191 -8251
+ Misses 66568 65023 -1545
+ Partials 10020 9142 -878
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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.
The PR cleanly extends seidb dump-flatkv with per-bucket/total LtHash computation, committed-root verification, and a read-throughput limiter; the logic (slice cloning, batch associativity, burst clamping, nil-map reads) is correct and well-tested. No blockers — only a couple of minor robustness/UX notes.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (./cursor-review.md) is empty — that pass produced no output.
- Behavior change:
--lthashnow defaults totrueand--read-limit-mbdefaults to64, so existingdump-flatkvinvocations now do extra LtHash work and are throttled to 64 MiB/s by default. This is documented in the command help, but consider calling it out in release notes/changelog so offline users aren't surprised by the slower default. - Minor redundancy:
printFlatKVLtHashalready computes the total LtHash but discards it, andverifyFlatKVLtHashre-sums the per-bucket accumulators from scratch. Cheap (just MixIn of four vectors), but the total could be computed once and reused. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // zero LtHash. Treat that as "nothing to verify against" rather than a | ||
| // spurious failure (e.g. a snapshot that predates LtHash metadata). | ||
| zero := lthash.New().Checksum() | ||
| if bytes.Equal(committedTotal, zero[:]) { |
There was a problem hiding this comment.
[suggestion] This conflates "no committed LtHash metadata" with "committed metadata present but zero/corrupt." The zero-LtHash checksum is also what an empty store and a store whose committed metadata was incorrectly zeroed both produce, so a non-empty store with zeroed/corrupt committed metadata is reported as skipped rather than failing verification — exactly the case verification exists to catch. Consider detecting metadata presence explicitly (e.g. an IsZero()/has-metadata signal from the store) instead of inferring absence from the checksum value. (Also raised by Codex.)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ba6f87ef0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — additive changes to the offline seidb dump-flatkv tool with no impact on commit/consensus paths.
Extended reasoning...
Overview
The PR extends an offline diagnostic CLI (seidb dump-flatkv) with two additions: (1) per-bucket and total LtHash computation, verified against snapshot metadata, and (2) byte-throughput read throttling so the scan can safely run against a live node. Changes are isolated to sei-db/tools/cmd/seidb/operations/dump_flatkv.go and its test file — no production commit/consensus code is touched.
Security risks
None substantive. The tool is read-only against a hard-linked snapshot clone. No new authentication, crypto, or RPC surfaces are introduced. The LtHash usage feeds raw iterator bytes into the same lthash package the FlatKV store itself uses for committed root hashing — no new cryptographic logic. A failed verification fails the command by design (intended exit-non-zero contract).
Level of scrutiny
Low. This is offline tooling whose worst-case failure is a wrong dump or a misleading PASS/FAIL on a CLI command — there is no on-chain or persisted-state effect. The default --read-limit-mb 64 is conservative and --lthash=true adds work that is purely diagnostic.
Other factors
The author handled the tricky edges explicitly: keys/values are cloned before retention (iterator slice-reuse), the limiter burst is floored at 4 MiB so an oversized row can't deadlock WaitN, and a zero committed-LtHash is treated as 'nothing to verify' rather than spuriously failing. A new test (TestBucketLtHasherMatchesSingleShot) exercises the batched MixIn path against a single-shot ComputeLtHash over the union, which is the right invariant to assert. Bug hunter found nothing and the cursor[bot] also flagged it low risk.
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 8ddbcfc. Configure here.
| fmt.Printf("Opened FlatKV at version %d\n", version) | ||
|
|
||
| return dumpFlatKVFromStore(store.CommitStore, outputDir, version, bucket) | ||
| return dumpFlatKVFromStore(store.CommitStore, outputDir, version, bucket, withLtHash, committedIsFullState, readLimitMiBps) |
There was a problem hiding this comment.
LtHash metadata probe races open
Medium Severity
DumpFlatKVData decides whether to verify LtHash via snapshotCommittedLtHashIsFullState before openFlatKVReadOnly, but each path calls selectFlatKVSnapshot on its own. On a live node the current snapshot or clone retries can change between the probe and the store actually opened, so committedIsFullState may not match the clone’s baseline—spurious verification failure or verification skipped when it should run.
Reviewed by Cursor Bugbot for commit 8ddbcfc. Configure here.
There was a problem hiding this comment.
Well-tested, well-documented extension of dump-flatkv to compute/verify per-bucket and total LtHash and throttle disk reads for live-node safety. No correctness blockers; two robustness/consistency points worth addressing, both originally surfaced by Codex.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no findings to merge.
- The new probe path (
snapshotCommittedLtHashIsFullState) is mostly informational: the actual PASS/FAIL inverifyFlatKVLtHashcompares the opened store's ownCommittedRootHash()against the freshly re-scanned rows, so a probe/open snapshot mismatch can never produce a wrong verification result — only a spurious skip or a spurious probe abort (see inline notes). Worth a one-line code comment clarifying the probe cannot affect verification correctness, to prevent future readers from over-trusting it. - Consider an integration-style test that exercises the real
snapshotCommittedLtHashIsFullState->openFlatKVReadOnlypath end-to-end (current tests cover the decision helper andselectedSnapshotHasLtHashMetadatain isolation but not the two-call sequence against a shared dbDir). - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| committedIsFullState := true | ||
| if withLtHash { | ||
| var probeErr error | ||
| committedIsFullState, probeErr = snapshotCommittedLtHashIsFullState(dbDir, height) |
There was a problem hiding this comment.
[suggestion] The probe runs before openFlatKVReadOnly and, unlike the open path, has no retry/clone-race hardening (prepareFlatKVToolingCloneWith retries on os.ErrNotExist/errSourceChurning). On a live, block-producing node the current symlink can be mid-swap or a selected snapshot can be pruned between this probe and the actual open, so selectFlatKVSnapshot/os.Readlink here can transiently fail and abort the whole dump (returning probe snapshot lthash watermark: ...) even though openFlatKVReadOnly would have succeeded via its retry loop. Since this is the new default (--lthash=true) live-node path, consider wrapping the probe in the same retry mechanism, or treating a transient probe error as committedIsFullState=false (skip verification) rather than a hard failure. Note: because verification itself reads the opened store's own committed hash + own re-scan, a probe/open snapshot mismatch can only cause a spurious skip, never a wrong PASS/FAIL — so this is robustness, not correctness.
| // zero LtHash. Treat that as "nothing to verify against" rather than a | ||
| // spurious failure (e.g. a snapshot that predates LtHash metadata). | ||
| zero := lthash.New().Checksum() | ||
| if bytes.Equal(committedTotal, zero[:]) { |
There was a problem hiding this comment.
[nit] This treats an all-zero committed checksum as "no committed LtHash," which is the exact heuristic the new probe was added to avoid (the probe distinguishes metadata presence precisely because a valid watermark can be all-zero). For a genuine full-state snapshot whose committed total is legitimately zero (effectively only an empty state), verification is silently skipped and reported as "no committed LtHash in metadata" instead of producing a PASS. In practice a non-empty state's lattice-sum checksum being all-zero is cryptographically negligible, so this is benign — but since committedIsFullState already gates whether we reach this code, you could drop the zero-hash skip here (or comment why it's kept) to keep the two presence checks consistent.
When the selected FlatKV snapshot predates the global LtHash watermark but the cloned changelog replays later blocks, the store opens with a zero baseline LtHash and catchup mixes in only the replayed WAL deltas. The resulting CommittedRootHash() is a partial hash (it omits the snapshot's pre-existing rows), so dump-flatkv --lthash's full re-scan compared against it and reported a false mismatch, failing the default --lthash path on any store upgraded from a pre-LtHash snapshot until a new snapshot exists. CommittedRootHash() on the opened store cannot distinguish a full-state hash from a partial one. The distinction is whether the selected snapshot carried the global LtHash metadata key before replay. Keep the fix tool-local by hardlink-cloning only the selected snapshot's metadata DB into a temporary probe directory and checking ktype.MetaLtHashKey directly. A missing key means skip verification; a present key, even if the stored watermark is all-zero, means the committed hash is full-state and remains verifiable. No change to the FlatKV store itself. Also add --lthash-only so operators can compute and verify the FlatKV lattice hash without writing enormous bucket dump files. This mode still scans all buckets and prints the same per-bucket/TOTAL hash and verification result, but it does not require --output-dir and creates no output files. Co-authored-by: Cursor <cursoragent@cursor.com>
8ddbcfc to
29c102a
Compare
There was a problem hiding this comment.
Solid, well-documented, well-tested extension of dump-flatkv to compute/verify per-bucket and total LtHash with read throttling for live-node safety. No blockers; a few low-severity robustness notes around the live-node verification decision.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- REVIEW_GUIDELINES.md (base branch) is empty, so no repo-specific standards were applied beyond AGENTS.md.
- The Cursor second-opinion review (cursor-review.md) was empty — that pass produced no output.
bucketLtHasher.flushdiscards the error fromlthash.ComputeLtHash(nil, h.batch)(delta, _ :=); the same is done inverifyFlatKVLtHash's total computation. If ComputeLtHash can ever return a non-nil error, the accumulator would be silently corrupted and verification could PASS/FAIL on wrong data. Consider propagating it. (Low severity — verify whether ComputeLtHash can error in practice.)- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| committedIsFullState := true | ||
| if withLtHash { | ||
| var probeErr error | ||
| committedIsFullState, probeErr = snapshotCommittedLtHashIsFullState(dbDir, height) |
There was a problem hiding this comment.
[suggestion] TOCTOU (raised by Codex, agreed): snapshotCommittedLtHashIsFullState selects the snapshot here, independently of the selection done later inside openFlatKVReadOnly. Since this command is explicitly designed to run against a live, block-producing node, current/snapshots can advance or be pruned between the two calls, so the full-state decision may describe a different snapshot than the one actually opened — yielding a false skip or false failure. The blast radius is low (a re-runnable diagnostic, not state corruption), but consider selecting the snapshot once and threading the result into both the probe and the open, or re-checking after open.
| // zero LtHash. Treat that as "nothing to verify against" rather than a | ||
| // spurious failure (e.g. a snapshot that predates LtHash metadata). | ||
| zero := lthash.New().Checksum() | ||
| if bytes.Equal(committedTotal, zero[:]) { |
There was a problem hiding this comment.
[nit] Inconsistent zero-handling (raised by Codex, agreed): the probe path deliberately distinguishes "metadata present" from "all-zero watermark" because a legitimate LtHash can be all-zero, yet here an all-zero committedTotal is treated as "nothing to verify" and skipped. For a genuinely empty/full-state snapshot the re-scan also yields zero, so a comparison would simply PASS — meaning this skips a verification it could perform. Low impact (a real non-empty state hashing to all-zero is astronomically unlikely, and skipping errs safe rather than false-failing), but consider gating this skip on the same presence signal (committedIsFullState) instead of the hash value.
There was a problem hiding this comment.
Solid, well-tested addition of LtHash computation/verification and read throttling to seidb dump-flatkv. No blocking issues; two minor robustness/consistency notes (a TOCTOU between the new metadata probe and the actual store open, and a now-redundant zero-hash skip path).
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (
cursor-review.md) was empty — no Cursor findings were available to merge. - The metadata probe (
snapshotCommittedLtHashIsFullState) lacks the retry-on-transient-error logic thatprepareFlatKVToolingCloneWithhas. On a live node, a snapshot pruned betweenselectFlatKVSnapshotandcloneDirRecursivesurfaces as a hard error that aborts the command, whereas the real open path retries. Since this is a diagnostic tool a re-run recovers, but consider reusing the retry wrapper or treating ENOENT during the probe clone as non-fatal. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| committedIsFullState := true | ||
| if withLtHash { | ||
| var probeErr error | ||
| committedIsFullState, probeErr = snapshotCommittedLtHashIsFullState(dbDir, height) |
There was a problem hiding this comment.
[suggestion] TOCTOU: this probe calls selectFlatKVSnapshot(dbDir, height) independently, and then openFlatKVReadOnly selects/clones the snapshot again. On a live node with --height 0, the current symlink can advance (or the probed snapshot can be pruned) between the two selections, so committedIsFullState may be computed against a different snapshot than the one actually opened. The practical impact is bounded (a false "skipped" or, around the LtHash-metadata deployment boundary, a missed verification), and verification itself stays self-consistent with the opened store — but worth a note/comment, or threading the resolved snapshot name through both paths so they agree. (Matches Codex P2.)
| // zero LtHash. Treat that as "nothing to verify against" rather than a | ||
| // spurious failure (e.g. a snapshot that predates LtHash metadata). | ||
| zero := lthash.New().Checksum() | ||
| if bytes.Equal(committedTotal, zero[:]) { |
There was a problem hiding this comment.
[nit] This zero-hash check is now largely redundant with the new committedIsFullState probe and partially defeats its purpose: the probe was added precisely to distinguish a legitimate all-zero watermark from "metadata absent." When committedIsFullState=true for an empty/zero-watermark full state, the re-scanned total is also zero, so verification reports "skipped (no committed LtHash...)" instead of actually PASSing. Since the predates-metadata case is already handled upstream, consider dropping or narrowing this branch so empty states are verified rather than skipped. (Matches Codex P3.)
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
There was a problem hiding this comment.
A well-documented, well-tested extension to the seidb dump-flatkv diagnostic tool adding per-bucket/total LtHash computation, verification against committed snapshot metadata, and a read-throughput throttle for live-node safety. One correctness gap (a TOCTOU between the metadata probe and the cloned store's snapshot selection) is worth addressing but is non-blocking for offline tooling.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (
cursor-review.md) produced no output (empty file); only Codex's pass contributed external findings, which is incorporated below. - The LtHash metadata probe (
snapshotCommittedLtHashIsFullState) re-selects the snapshot from the livedbDirindependently ofopenFlatKVReadOnly. Consider derivingcommittedIsFullStatefrom the already-cloned snapshot'smetadata/directory insidetempDir(the clone hardlinks it and is immutable), eliminating the second live-directory selection entirely and tying the watermark decision to exactly the snapshot that gets scanned. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| committedIsFullState := true | ||
| if withLtHash { | ||
| var probeErr error | ||
| committedIsFullState, probeErr = snapshotCommittedLtHashIsFullState(dbDir, height) |
There was a problem hiding this comment.
[suggestion] This probe selects/opens a snapshot independently of the clone opened on line 210, so on a live node the two can describe different snapshots (a TOCTOU). For height=0 both resolve through the current symlink, which a block-producing node advances; tryPrepareFlatKVToolingClone can also re-select an older snapshot after a mid-clone prune. The divergence can cause verification to be silently skipped when the opened store actually has a full-state hash, or (in the prune case) a spurious FAIL when the probe saw metadata but the clone fell back to an older, pre-LtHash snapshot — undermining the verification this tool exists to perform. Since openFlatKVReadOnly already hardlinks the selected snapshot's immutable metadata/ dir into tempDir, prefer probing that cloned metadata so the watermark decision is bound to the exact snapshot being scanned. (Matches Codex's finding.) Non-blocking given this is offline tooling and the window is narrow.
There was a problem hiding this comment.
A well-tested extension to seidb dump-flatkv adding per-bucket/total LtHash computation, verification against committed snapshot metadata, and read throttling. No blockers; one legitimate robustness/correctness-edge issue around the snapshot probe racing the actual clone on a live node.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output.
- verifyFlatKVLtHash's zero-hash skip overlaps with the committedIsFullState skip path. Both are defensive and harmless, but the redundancy is worth a one-line comment noting they cover different cases (no metadata at all vs. snapshot predating metadata) so a future reader doesn't remove one assuming it's dead.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| committedIsFullState := true | ||
| if withLtHash { | ||
| var probeErr error | ||
| committedIsFullState, probeErr = snapshotCommittedLtHashIsFullState(dbDir, height) |
There was a problem hiding this comment.
[suggestion] The full-state probe selects and clones the snapshot independently of the subsequent openFlatKVReadOnly call, so the two can disagree on a live node. Two concrete problems:
-
Spurious abort under pruning.
snapshotCommittedLtHashIsFullState→selectedSnapshotHasLtHashMetadatadoes its owncloneDirRecursiveof the metadata dir with no retry. If the live node prunes the selected snapshot mid-clone,os.Linkreturns ENOENT, which propagates up asprobe snapshot lthash watermark: ...and panics the whole command — whereas the main open path (prepareFlatKVToolingCloneWith) retries exactly this race up tomaxCloneRetries. -
Possible false verification FAIL (height-pinned). With
--heightset,selectFlatKVSnapshotreturns the largest snapshot ≤ height. If that snapshot is pruned between this probe andopenFlatKVReadOnly, the clone falls back to an older snapshot ≤ height that may predate LtHash metadata. The probe then computedcommittedIsFullState=true(verify), but the store actually opened on a metadata-less baseline with a partial committed hash →verifyFlatKVLtHashcompares a full re-scan against a partial hash and reports FAIL. For a verification tool, a false FAIL is the most damaging outcome (operator reads it as state corruption). (For height=0 thecurrentsymlink only advances and metadata is monotonic, so that direction is safe — this is the height-pinned case.)
Suggest tying the watermark check to the same cloned snapshot the scan actually uses — e.g. have openFlatKVReadOnly return the resolved snapshot name and probe its metadata from the temp clone — rather than re-selecting independently.
2 participants
Summary
Extends the
seidb dump-flatkvcommand to compute and verify the lattice hash (LtHash) of scanned FlatKV state and to throttle its disk reads so it can safely run against a live, block-producing node. The core scan path (dumpFlatKVFromStore) gains two parameters —withLtHashandreadLimitMiBps— and two new abstractions: a per-bucket streamingbucketLtHasherand a byte-throughput read limiter.sei-db/tools/cmd/seidb/operations/dump_flatkv.go:--lthash(defaulttrue) and--read-limit-mb(default64) flags, plus extended command documentation covering live-node and Kubernetes usage.--read-limit-mbis validated to be>= 0(0means unlimited).newReadLimiterbuilds agolang.org/x/time/ratelimiter from a MiB/s ceiling. The burst is one second of budget but never less thanminReadBurstBytes(4 MiB), guaranteeing a single large row (e.g. contract bytecode) can never exceed the burst and deadlockWaitN. Returnsnil(unlimited) when the ceiling is<= 0. The scan callsWaitN(len(key)+len(val))per row so throughput is measured on actual key+value bytes read.bucketLtHasherincrementally accumulates a bucket's LtHash from a stream of raw physical key / serialized value pairs. Keys and values are cloned onaddbecause the iterator may reuse the backing slices onNext(). Pairs are buffered up tolthashBatchCap(8192) then folded into the running accumulator viaMixIn; batching is safe because the LtHash group is associative.--bucketwrite filter, so the printed TOTAL equals the node's committed global LtHash even when only one bucket is written to disk. Each bucket feeds exactly the bytes the FlatKV store hashes into its per-DB working LtHash, so per-bucket checksums match per-DB committed LtHashes and their MixIn matches the global root.verifyFlatKVLtHashcross-checks the re-scanned total againstCommittedRootHash()from snapshot metadata, printing a PASS/FAIL block and returning an error (non-zero exit) on mismatch. A store with no committed LtHash (zero-hash, e.g. a snapshot predating LtHash metadata) is treated as "nothing to verify" and skipped rather than failing spuriously.Test plan
sei-db/tools/cmd/seidb/operations/dump_flatkv_test.go:TestDumpFlatKVFromStoreAllBucketsandTestDumpFlatKVFromStoreSingleBucketcalls for the newwithLtHash/readLimitMiBpssignature (LtHash on, throttling off).TestBucketLtHasherMatchesSingleShot, which uses more than two full batches (lthashBatchCap*2 + 17pairs) to exercise the incrementalMixInpath. It asserts each batched per-bucket checksum equals a single-shotComputeLtHashover the same pairs (associativity), and that theMixInof the per-bucket accumulators equals oneComputeLtHashover the union — the exact per-bucket + total relationship the command reports.