Implement new Giga GarbageCollector interface

[yzang2019]

Summary

Retention for Giga's storage components is currently decided by each store's own
pruner, independently and with no knowledge of the others. This PR implements
gc.PrunableStore for all four of them so a single StorageGarbageCollector
can manage the pruning logic for the whole fleet.

Each cycle the collector asks every store for its ingest height and for the
oldest block it must keep in order to serve the shared RollbackWindow, takes
the minimum of those answers, and prunes every store to it. The stores therefore
stay mutually consistent — a rollback target that one store can serve is one they
can all serve, which per-store pruners could not guarantee.

What's included

Store	Change
BlockDB	New litt_block_gc.go; adds a RetentionWindow config field
ReceiptDB	New litt_receipt_gc.go; adds an ExternalPruning config field that gates the existing background pruner; adds a litt GCFilter so reclamation follows the retention floor
StateWAL	New state_wal_gc.go; PruneBelow calls seiwal directly
FlatKV (SC)	New store_gc.go; snapshot-aware pruning boundary; adds an ExternalPruning config field

Notable design points

ExternalPruning() bool on PrunableStore

Two stores keep a pruner of their own — FlatKV (SnapshotKeepRecent) and
ReceiptDB (KeepRecent) — because both still run without a collector: FlatKV in
the seidb tools and bench paths, ReceiptDB on any node with
rs-backend = "littidx". Both pruners active at once is unsafe: the local one
would delete the very data the collector is holding to serve the rollback window.
Standing the local one down with nothing to replace it is equally unsafe, and
fails silently — the retention floor simply stops advancing.

ExternalPruning makes both combinations unrepresentable rather than merely
discouraged. Each store answers it from the same config.ExternalPruning field
its own pruner reads to stand down, so "the collector prunes this store" and "the
store does not prune itself" are one fact instead of two settings that can
disagree. Stores with no pruner of their own return true unconditionally.

A store that reports false is still asked for its boundary and still holds the
shared minimum down — it just never receives PruneBelow. Dropping it from the
vote instead would prune the WAL out from under the snapshots it replays from.

Neither ExternalPruning field is reachable from app.toml (both are
mapstructure:"-"). Enabling one is only correct when the store is registered
with a running collector, which is a property of how the process was wired and
not something an operator can assert. Where it is checkable it is checked:
newReceiptBackend rejects pebbledb + ExternalPruning, because that backend
is not a gc.PrunableStore and would end up with no pruner at all.

The retention floor gates reclamation, and the TTL is only an age failsafe

Both litt-backed stores now pair their TTL with a GCFilter, so a record is
reclaimed only when it is both below the store's retention floor and
older than the TTL. Previously ReceiptDB had no filter, which made the TTL the
sole reclamation mechanism, and it was sized as KeepRecent × 2s. Under
ExternalPruning the enforced retention is RollbackWindow + KeepRecent, so a
TTL sized for KeepRecent alone expired receipt bodies for blocks the collector
still considered live — ErrNotFound inside the rollback window, which is the
exact cross-store guarantee the collector exists to provide.

With the filter in place the TTL no longer has to know how many blocks anything
is, so littTTLPerBlock is gone and both stores take a flat duration
(RetentionTime / littRetentionTime), defaulting to 1 hour. Visible retention
follows the floor and reclamation can no longer lead it.

Retention semantics

With R = a store's GetRetentionWindow and F = LatestBlock - RollbackWindow - R,
collection guarantees, per managed store:

1. Nothing needed to roll back to any block in [LatestBlock - RollbackWindow, LatestBlock] is deleted.
2. No data at or above F is deleted — so even after rolling back to
   LatestBlock - RollbackWindow, the most recent R blocks are still readable.
3. Data below F is eventually deleted, each store reclaiming on its own schedule.

Guarantee 2 is why pruning is to the shared minimum rather than to each store's
own boundary: a retained snapshot is only restorable if the blocks that follow it
survive in the contiguous stores. This is recorded on
StorageGarbageCollectorConfig.RollbackWindow and, in block terms, on
BlockDBConfig.RetentionWindow.

GetRetentionWindow reports extra retention beyond the shared
RollbackWindow, with InfiniteRetentionWindow (-1) meaning never prune.
Note that ReceiptStoreConfig.KeepRecent == 0 already means "keep everything",
which is the opposite of what 0 means to the collector, so it is mapped to
InfiniteRetentionWindow rather than passed through.

StateWAL answers 0 unconditionally, and has no retention config of its own. Its
depth is not its to declare: it is a replay source, and SC/SS already express how
far back it must reach by answering their oldest live snapshot as a boundary. A
window here would be additive on top of the shared minimum, retaining every
managed store further back rather than the WAL alone — a fleet-wide decision
wearing a per-store name, which is what RollbackWindow already is.

Snapshot stores

FlatKV restores only at a snapshot boundary and replays the WAL forward from
there, so what it must retain is not a block range but the newest snapshot at or
below the target. GetPruningBoundary reports that, which is what holds the WAL
back for it.

Config changes

• littblock.BlockDBConfig.Retention → RetentionTime, default 24h → 1h.
  It is an age floor, not a retention policy; how much history BlockDB keeps is
  RetentionWindow. The AutobahnBlockDBConfig.Retention override keeps its
  name, since its retention JSON key is a persisted config format.
• littblock.DefaultConfig now leaves RetentionWindow at 0 (was 10000).
  It is an input to a minimum shared across every managed store, so a non-zero
  default here would have held ReceiptDB, the state WAL and the SC snapshots
  10k blocks further back on BlockDB's say-so. Every other store reports 0; a
  deployment wanting deeper block history sets it at the call site.
• New ExternalPruning on ReceiptStoreConfig and FlatKVConfig, both
  mapstructure:"-" and both defaulting to false.

ReceiptStoreConfig.KeepRecent is deliberately left at 0 (keep everything).
Nothing here couples it to RetentionWindow, because the two fields disagree
about 0: BlockDB folds only negatives to InfiniteRetentionWindow, so 0
there is the most aggressive setting, while ReceiptDB folds <= 0 to infinite,
so 0 there means never prune. KeepRecent cannot express "nothing beyond the
rollback window" at all. Reconciling the two is left to the wiring PR, along
with what the shared window should actually be.

Also in this PR

• Renames littblock.LittBlockConfig to BlockDBConfig (ripples into sei-tendermint).
• Renames the BlockDB table from ledger to blocks. No migration is needed —
  BlockDB is not deployed on any network and no such data exists. Because the
  table name is persisted layout rather than an identifier, NewBlockDB would
  otherwise open a fresh empty table beside the old data, and an empty store is
  indistinguishable from a correct one until something asks for history. A
  refuseLegacyTable check at open turns that into a startup error naming the
  directory, for dev/CI/devnet homes written before the rename; it can be deleted
  once no such directory remains.
• Documents seiwal.WAL.PruneBefore as safe to call off the WAL owner's
  goroutine, unlike most of the interface, which is what lets the collector prune
  the WAL from its own goroutine.

Not in this PR

Not moving all stores to use StorageGarbageCollector yet. We expect to
construct it in a future PR when we decide to unify the pruning for mainnet. Both
ExternalPruning fields deliberately default to false: no behavior change by
default.

That wiring PR is also where the remaining guard belongs. The receipt path can
reject its unsupported combination at startup, but nothing on the FlatKV path can
validate that a collector exists — "a collector exists" is not knowable from that
package. Keeping the field unreachable from config is what stands in for the
check until then.

Also worth knowing at wiring time: enabling ExternalPruning changes the shape of
snapshot retention rather than just its depth. Snapshot count becomes roughly
RollbackWindow / SnapshotInterval instead of SnapshotKeepRecent + 1.

Not managing SS yet in this PR since SS doesn't have snapshot capability yet.

Testing

• A *_gc_test.go suite per store, plus collector coverage for the self-pruning
  path (a store reporting false keeps its vote but receives no PruneBelow).
• litt_receipt_gcfilter_internal_test.go covers the filter as a predicate and
  end-to-end: blocks below the floor are reclaimed by a real litt GC pass, those
  at or above are retained. It fails if the filter is removed.
• litt_receipt_pruner_internal_test.go pins when the local pruner runs, as a
  pure predicate rather than a timing assertion.
• litt_block_legacy_table_test.go covers the pre-rename refusal, including that
  a refused open leaves the directory exactly as it found it.
• Config defaults that moved are re-recorded in testdata/*.golden, so each new
  value lands in a diff.
• Run under -race.

[cursor]

PR Summary

High Risk
Changes rollback and pruning invariants across block, receipt, WAL, and FlatKV storage; BlockDB on-disk table rename can brick or empty stores without the legacy guard; incorrect floors or dual pruners could cause data loss or failed rollbacks.

Overview
Unifies Giga storage retention behind a redesigned StorageGarbageCollector: each cycle takes the minimum GetRollbackFloor(RollbackWindow) across stores as the snapshot cut line, then prunes history at snapshotCutLine - LookbackWindow (or keeps all history when lookback is -1). Stores with ExternalPruning() receive PruneSnapshots and PruneHistory; self-pruning stores still vote on the floor but are not pruned by the collector.

PrunableStore replaces the older single PruneBelow / GetPruningBoundary / per-store retention window model with GetRollbackFloor, split snapshot vs history pruning, and ExternalPruning.

Store	What changed
litt BlockDB	Implements gc.PrunableStore; Retention → RetentionTime (policy vs age floor); persisted table ledger → blocks with refuseLegacyTable on open
litt receipt (littidx)	ExternalPruning config; local KeepRecent pruner stands down when managed; gcFilter so litt TTL cannot reclaim bodies above the retention floor; pebble backend rejects external pruning
FlatKV	store_gc.go snapshot pruning + rollback floor; ExternalPruning disables count-based snapshot prune and WAL truncation
State WAL	PruneHistory via PruneBefore (documented as collector-safe concurrent prune)

Config/docs: ExternalPruning on receipt/FlatKV (not in app.toml), golden files, and godoc style rules in AGENTS.md. Large *_gc_test.go coverage; default ExternalPruning: false so behavior is unchanged until wiring enables the collector.

^{Reviewed by Cursor Bugbot for commit a0de18c. Bugbot is set up for automated code reviews on this repo. Configure here.}

[claude]

Beyond the inline finding, I also checked two other candidate issues: the BlockDB litt table rename from "ledger" to "blocks" (no live production data exists under the old table name for this not-yet-deployed Giga store, so this isn't an on-disk compatibility break), and littidx ReceiptDB losing its background pruner with no ExternalPruning fallback (expected per this PR's description — the collector wiring lands in a follow-up PR, consistent with this repo's staged-rollout convention).

Extended reasoning...

This PR is large and touches critical, not-yet-wired pruning logic across four Giga storage stores. One nit-level bug was already found and posted inline (statewal.New skips config.Validate). Beyond that, I reviewed two additional candidate concerns raised by finder agents and ruled both out: the BlockDB table rename is safe because the Giga block store has no production data to migrate yet, and the ReceiptDB pruner removal is explicitly called out in the PR description as intentional pending the follow-up collector-wiring PR.

[codecov]

Codecov Report

❌ Patch coverage is 87.15278% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.74%. Comparing base (ab08efb) to head (a0de18c).

Files with missing lines	Patch %	Lines
sei-db/state_db/sc/flatkv/store_gc.go	84.69%	11 Missing and 4 partials ⚠️
sei-db/state_db/statewal/state_wal_gc.go	68.42%	5 Missing and 1 partial ⚠️
sei-db/ledger_db/receipt/litt_receipt_store.go	85.71%	3 Missing and 2 partials ⚠️
sei-db/ledger_db/block/littblock/litt_block_gc.go	78.94%	4 Missing ⚠️
sei-db/ledger_db/block/littblock/litt_block_db.go	84.21%	3 Missing ⚠️
sei-db/ledger_db/receipt/litt_receipt_gc.go	88.88%	2 Missing ⚠️
sei-tendermint/config/autobahn.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3868      +/-   ##
==========================================
- Coverage   59.45%   58.74%   -0.71%     
==========================================
  Files        2319     2250      -69     
  Lines      198379   190815    -7564     
==========================================
- Hits       117946   112100    -5846     
+ Misses      69235    68077    -1158     
+ Partials    11198    10638     -560     

Flag	Coverage Δ
sei-chain-pr	65.47% <88.57%> (?)
sei-db	70.41% <ø> (-0.22%)	⬇️
sei-db-state-db	?
sei-db-state-db-pr	76.42% <85.81%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
sei-db/config/giga_config.go	88.88% <ø> (ø)
sei-db/config/receipt_config.go	91.48% <ø> (ø)
sei-db/ledger_db/block/blocksim/blocksim.go	9.02% <100.00%> (ø)
...-db/ledger_db/block/littblock/litt_block_config.go	62.50% <100.00%> (+12.50%)	⬆️
sei-db/ledger_db/receipt/receipt_store.go	67.00% <100.00%> (+0.33%)	⬆️
sei-db/management/gc/storage_garbage_collector.go	100.00% <100.00%> (+1.78%)	⬆️
.../management/gc/storage_garbage_collector_config.go	100.00% <100.00%> (ø)
sei-db/seiwal/seiwal.go	71.42% <ø> (ø)
sei-db/state_db/sc/flatkv/config/config.go	72.30% <ø> (ø)
sei-db/state_db/sc/flatkv/snapshot.go	68.32% <100.00%> (+0.95%)	⬆️
... and 9 more

... and 74 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Beyond the inline finding, I also checked the littidx TTL depth under ExternalPruning: the litt TTL is set from KeepRecent independent of the collector's RollbackWindow, so it could in principle expire receipt bodies before the collector's retention floor — but with no StorageGarbageCollector constructed anywhere yet and ExternalPruning unreachable from app.toml, this can't produce a live gap today. Not a blocker.

Extended reasoning...

Checked whether littidx's TTL (derived from KeepRecent, littTTLPerBlock) could expire receipt bodies earlier than a StorageGarbageCollector's RollbackWindow would require once ExternalPruning is wired up. Confirmed this is currently unreachable: ExternalPruning is mapstructure:"-" (no app.toml key) and no code in this PR or elsewhere constructs a StorageGarbageCollector, so the mismatch has no live path today. Recording this so a future PR that wires up the collector re-checks the TTL depth against RollbackWindow before enabling ExternalPruning in production.

[seidroid]

Well-documented, well-tested implementation of gc.PrunableStore across the four Giga stores, with the collector-side ExternalPruning plumbing looking correct. One blocking issue: the LittDB block table is silently renamed from "ledger" to "blocks", which is a persisted on-disk identifier with no migration, and is not mentioned anywhere in the PR description.

Findings: 1 blocking | 8 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this review reflects only Claude's and Codex's findings. Codex reported a single finding (the block table rename), which I independently confirmed and raised inline.
• sei-db/state_db/sc/flatkv/store.go: InitializeDataDirectories and applyPebbleMetricsConfig are moved verbatim from the top of the file to the bottom (and var _ Store = (*CommitStore)(nil) moved up) — ~80 lines of pure code motion with no behavior change and no relation to GC. Worth dropping to keep the diff reviewable.
• No end-to-end test constructs a real StorageGarbageCollector over the four concrete stores: each store is tested against the interface in isolation, and the collector only against mocks. The load-bearing interaction the PR exists for — FlatKV's snapshot boundary holding the state WAL back — is therefore only covered by mockStore. Understood that wiring is deferred, but a single integration test over the real four would pin the invariant the mocks assert.
• The preconditions in store_gc.go and state_wal_gc.go ("FlatKV must be managed alongside the state WAL, and vice versa") are documented but unenforced. The converse combination is the sharper one: self-pruning FlatKV + collector-managed state WAL leaves tryTruncateWAL live, and it can truncate the WAL above the collector's shared minimum, dropping the replay range SS still needs. Consider a construction-time check wherever the collector is eventually wired.
• statewal and flatkv each got a race-detector test for the GC surface against a concurrent writer (TestGCConcurrentWithWriter, TestGCConcurrentWithCommitter); blockDB did not. It is fully mutex-guarded so it is likely fine, but note that GetLatestBlock and PruneBelow both take s.mu, the same mutex WriteBlock/WriteQC hold — so the collector's goroutine can now block behind block writes. A matching parallel test would pin both the safety and the intent.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This field is mapstructure-addressable while its exact counterpart on ReceiptStoreConfig is mapstructure:"-", and the receipt-side comment gives the reason: enabling it is "only correct when this store is registered with a running StorageGarbageCollector, which is a property of how the process was wired and not something an operator can assert from app.toml. Exposing a key for it would let a node stand its pruner down with nothing to replace it, and the resulting unbounded growth is silent."

That argument applies at least as strongly here — this flag stands down two mechanisms (pruneSnapshots and tryTruncateWAL), so a stray external-pruning = true under [state-commit.flatkvconfig] leaves both snapshots and the state WAL with nothing bounding them. The toml_test.go addition keeps it out of the generated template, but mapstructure still binds a hand-written key.

Two options: tag it mapstructure:"-" to match the receipt field, or, if it is meant to stay operator-settable, pin the new read in testutil/configtest per the repo's configuration-reads guidance.

Separately: the receipt path rejects the unsupported combination at startup (newReceiptBackend errors on pebble + ExternalPruning), but nothing on the FlatKV path validates that a collector exists. Worth an equivalent guard wherever the collector gets constructed.

[claude]

I didn't find any new issues in this pass, but this is a large, critical-path storage change — four different store implementations (BlockDB, ReceiptDB, StateWAL, FlatKV) are being given a shared PrunableStore/ExternalPruning contract that a follow-up StorageGarbageCollector will drive — so it's worth a human look rather than a bot approval.

What was reviewed: the ExternalPruning/GetRetentionWindow/GetPruningBoundary implementations across all four stores and the collector's cutLine/pruneHeight/self-pruning-store logic in storage_garbage_collector.go; confirmed the empty-store PruneBelow issue flagged earlier in this thread has been fixed (pruneBlocksBelow now no-ops when latestVersion <= 0); and checked FlatKV's ExternalPruning mapstructure:"-" omission — it's intentionally unexposed in app.toml like its receipt-store sibling, not a missing guard.

Extended reasoning...

This PR introduces gc.PrunableStore implementations for BlockDB, ReceiptDB, StateWAL, and FlatKV, plus the StorageGarbageCollector decision logic that will eventually drive pruning across all of them, and renames the public littblock.LittBlockConfig type (rippling into sei-tendermint). No collector is wired up yet in this PR — that is deliberately left for a follow-up — so the immediate behavioral risk is low, but the contracts being established here (ExternalPruning, GetRetentionWindow, GetPruningBoundary, the cutLine/pruneHeight minimum) are exactly what future pruning correctness will depend on.

No security-sensitive surface (auth/crypto/permissions) is touched; the risk here is data-retention correctness (accidentally pruning data a store still needs, or leaking unbounded growth), not exploitability.

Scrutiny should be high: this is core sei-db storage-lifecycle code, not app/consensus logic, but a mistake in the shared-minimum logic or in an ExternalPruning stand-down could silently either delete data needed for rollback or grow disk unbounded once the collector is wired up in the next PR. The PR is also large (29 files) and threads a subtle invariant (self-pruning stores still vote but never receive PruneBelow) through four independent implementations, which increases the chance of a one-store inconsistency.

The thread already contains substantive back-and-forth (cursor[bot] and my own prior comments) covering an empty-store PruneBelow bug (now fixed), a BlockDB table rename the author confirmed is safe (no existing data), and two still-open nits (statewal.New not calling config.Validate(), and the receipt store's litt TTL not accounting for RollbackWindow once ExternalPruning is wired up) that were assessed as non-blocking since no current code path can reach them. Given the size, the number of new invariants being introduced, and that not every open thread has been explicitly resolved, this is a good candidate for a human pass before merge.

[claude]

This PR implements a new fleet-wide StorageGarbageCollector and wires gc.PrunableStore into BlockDB, ReceiptDB, StateWAL, and FlatKV — no bugs were found this run, but given the scope and criticality of this change I think it's worth a human look before merging.

Beyond the bug hunting system's findings, this run also checked: FlatKV/state-WAL/BlockDB/ReceiptDB GetPruningBoundary/GetLatestBlock contracts for off-by-one and empty-store edge cases, the self-pruning-vs-collector ExternalPruning handoff in all four stores, and the two candidate issues the bug hunting system ruled out this run (receiptDB's boundary vs. its local pruner floor, and blockDB's ExternalPruning vs. its RetainHeight-driven pruner) — both were not real bugs. I also confirmed the concerns from earlier review rounds (the empty-store PruneBelow gap in littReceiptStore, the stale .LittBlockConfig/.BlockDBConfig comment, and the unreachable RetentionWindow validation in statewal) have since been addressed in later commits.

Extended reasoning...

Overview

This PR adds a shared StorageGarbageCollector (sei-db/management/gc) and implements gc.PrunableStore for four storage components — littblock.BlockDB, the littidx ReceiptDB, StateWAL, and FlatKV's CommitStore — so a single collector can coordinate pruning/retention across the whole Giga storage fleet instead of each store pruning independently. It also renames littblock.LittBlockConfig to BlockDBConfig (rippling into sei-tendermint) and renames the underlying litt table from ledger to blocks. None of this is wired into a running collector yet per the PR description — that is deferred to a follow-up PR.

Security risks

No injection/auth/crypto surface here. The real risk category is data-durability/consensus-adjacent correctness: a wrong pruning boundary or retention-window calculation could silently delete blocks, receipts, WAL entries, or state snapshots still needed to serve a rollback, or (in the WAL table-rename case) strand already-persisted data under an old table name. These failure modes are typically silent (unbounded disk growth or missing data discovered much later) rather than loud, which raises the value of a careful human pass even absent a concrete bug today.

Level of scrutiny

High. This is new infrastructure for coordinating retention across BlockDB, ReceiptDB, StateWAL and FlatKV — all storage paths that back consensus and RPC serving for Giga. Cursor's own summary flagged it "High Risk" for the same reason. The code is not live yet (no construction site calls NewStorageGarbageCollector), which lowers the blast radius today, but the interface contracts being established here (GetPruningBoundary, GetRetentionWindow, ExternalPruning) will govern real pruning behavior once wired up in the next PR, so getting them right now matters.

Other factors

Test coverage is extensive (a *_gc_test.go suite per store plus collector-level tests, per the PR description), and this run's independent look, plus the bug hunting system's, did not surface a live bug. Several issues raised in earlier review rounds on this PR (an empty-store PruneBelow gap in the receipt store, a stale comment, and a validation method that was never called) have since been fixed or the underlying field removed entirely, which is a good signal the author is engaging with feedback. Given the size (29 files) and the criticality of the code paths involved, I'd still like a human maintainer to sign off.

[claude]

No new bugs found this run beyond the existing inline findings on this PR. Beyond those, I checked whether CommitStore.reopenWAL/Rollback swapping in a new stateWALImpl would orphan a collector-held gc.PrunableStore reference to the old (now-closed) WAL — it would, but no code in this PR (or elsewhere in the tree) constructs a StorageGarbageCollector yet, so this is a wiring concern for the follow-up PR that actually registers these stores, not a bug in what's merged here.

Extended reasoning...

The candidate issue was: stateWALImpl's identity as a gc.PrunableStore is invalidated whenever CommitStore.reopenWAL or CommitStore.Rollback swaps s.wal for a freshly constructed statewal.New(...) instance. If a StorageGarbageCollector held a reference to the old stateWALImpl object, that reference would silently go stale (PruneBelow on it would just error 'state WAL is closed', and the real live WAL would never be pruned again). This is refuted as a bug in the current PR because no code anywhere in the tree constructs a StorageGarbageCollector with these stores yet — the PR description explicitly defers that wiring to a follow-up PR. Whoever writes that wiring will need to either re-register the store after a reopen, or expose a stable handle that survives it; that is a real design point to track at wiring time, but not something the current diff gets wrong.

[cody-littley]

Might be useful to call out the following invariants:

Storage garbage collection guarantees the following invariants:

1. Garbage collection will not delete any data that is necessary to roll back to any block 
   between LatestBlock and (LatestBlock - RollbackWindow), inclusive.
2. Garbage collection will not delete block DB data that is before 
   (LatestBlock - RollbackWindow - RetentionWindow). This ensures that even if the 
   system rolls back to block (LatestBlock - RollbackWindow), it is still possible to read any
   block from the last RetentionWindow blocks.
3. Garbage collection will eventually delete block data older than 
   (LatestBlock - RollbackWindow - RetentionWindow).

[cody-littley]

I suggest we paste this block of invariants at each RetentionWindow or RollbackWindow config (with wording adjusted a little, the above is specific to block storage).

[cody-littley]

LGTM. Technically Flush() is also legal to call from anywhere, but it's ok for the godocs to be more restrictive than the code when it comes to threading model.

[cody-littley]

The following would probably simplify this code a lot:

• Create a method getSnapshotBlocks() ([]uint64, error) that returns a slice of block numbers in sorted order for all the snapshots on disk.
• Create a method deleteSnapshot(block uint64) error that deletes a single snapshot with the given block number

This lets us avoid the lambda function, and splits apart the logic for traversing the directory structure and the logic for deciding which blocks to keep and which ones to drop.

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 11, 2026, 4:45 PM

[seidroid]

A carefully staged, unusually well-documented and well-tested implementation of gc.PrunableStore across the four Giga stores, with a genuine correctness gap: the littidx receipt TTL is still derived from KeepRecent alone, so under ExternalPruning receipt bodies inside the collector's shared RollbackWindow can expire while the retention floor still claims them servable. Also flagged: the two new ExternalPruning config fields carry live-looking mapstructure keys that contradict their own docs and a characterization-test comment.

Findings: 2 blocking | 10 non-blocking | 7 posted inline

Blockers

• Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this synthesis merges only Claude's and Codex's findings.
• 1 blocking issue(s) flagged inline on specific lines.

Non-blocking

• Nothing in the tree constructs a StorageGarbageCollector or sets either ExternalPruning field, so every path added here is unreachable in a running node today. That is stated in the PR description and is fine as staged work, but it means the interaction between the four stores is only exercised by mocks in storage_garbage_collector_test.go — there is no test where the real FlatKV, StateWAL, ReceiptDB and BlockDB vote in one cycle. Worth adding when the collector is wired, since the cross-store invariant (SC's snapshot boundary holding the WAL back) is the whole point of the design and is currently pinned only against mockStore.
• flatkv/config.Config.ExternalPruning has no equivalent of newReceiptBackend's "reject the combination we cannot honor" guard — the doc says so explicitly and defers it to wherever the collector is constructed. Please make sure that follow-up actually lands a "ExternalPruning set but this store is not registered with a running collector" check; the failure mode (snapshots and the state WAL both unbounded) is silent and expensive, and the flag alone stands down two mechanisms.
• sei-db/state_db/sc/flatkv/store.go: relocating InitializeDataDirectories/applyPebbleMetricsConfig to the bottom of the file and moving var _ Store = (*CommitStore)(nil) up is pure code motion unrelated to the GC work. It adds ~76 lines of diff noise and makes git blame on those functions point at this PR. Consider dropping it or splitting it out.
• TestReceiptLocalPrunerAdvancesFloorWithoutCollector pays a real 1–2s wall-clock wait against a jittered ticker with a 6s require.Eventually budget. The comment justifies it and TestRunsLocalPruner covers the decision without waiting, so this is only a note: it is the kind of test that becomes the flake on a loaded CI runner.
• 6 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[blocker] The litt TTL is the one thing that does not follow the collector's window. (Also raised by Codex as P1.)

newLittReceiptStore still sets receipts.SetTTL(KeepRecent * littTTLPerBlock) (line 162), i.e. the body TTL covers roughly KeepRecent blocks of wall clock. But under ExternalPruning the retention the collector enforces is RollbackWindow + GetRetentionWindow() = RollbackWindow + KeepRecent blocks.

Concretely, with ExternalPruning = true, KeepRecent = 100_000 and RollbackWindow = 100_000: the collector holds the floor at head - 200_000 and every other store retains to match, so a rollback to head - 150_000 is supposed to be servable — but litt has already expired the receipt bodies for everything older than ~100_000 × 2s. GetReceiptFromStore returns ErrNotFound for blocks the retention floor says are live, which is exactly the cross-store consistency guarantee the collector exists to provide.

The type doc here says "KeepRecent is what this store asks for either way: the pruner's window when it runs, the collector's retention window when it does not, and the litt TTL in both cases" — the third clause is the bug. The TTL needs to cover RollbackWindow + KeepRecent when external pruning is on, which means RollbackWindow has to reach this constructor (or the TTL has to be disabled under ExternalPruning and reclamation left entirely to the collector).

Latent today since nothing wires a collector, but the mapping between KeepRecent, GetRetentionWindow and the TTL is defined here, so this is where it should be resolved rather than in the wiring PR.

[seidroid]

[suggestion] The doc directly above says "Like KeepRecent this is not read from the receipt-store config", but unlike KeepRecent (line 52, mapstructure:"-") this field carries a live-looking key. ReadReceiptConfig only does explicit opts.Get lookups so nothing decodes it today — but the tag is the only structural expression of the invariant, and it currently says the opposite of the prose. Suggest mapstructure:"-" to match KeepRecent; testdata/receipt-store.golden records the field either way (as it does for KeepRecent), so the change is inert for the characterization suite.

[seidroid]

[suggestion] This comment states as fact that ExternalPruning is tagged mapstructure:"-", but receipt_config.go:74 tags it mapstructure:"external-pruning". Per testutil/configtest/AGENTS.md these manifest exclusions are the recorded contract a replacement implementation reads, so a comment that misdescribes the tag is the specific drift the suite is meant to prevent. Fix the tag (preferred) or the comment.

[seidroid]

[suggestion] Same as the receipt-side field: the doc says "Keeping the field unreachable from config is what replaces that guard", but mapstructure:"external-pruning" is a real key name. GetConfig reads FlatKV via explicit v.IsSet("state-commit.flatkv.*") calls and this one is not among them, so it is unreachable in practice — but toml_test.go:61 only asserts the key is absent from the template, which would keep passing if a future viper.Unmarshal path picked the struct up. Given the doc's own point that this one flag stands down both pruneSnapshots and tryTruncateWAL, mapstructure:"-" makes the claim structural rather than incidental.

[seidroid]

[nit] pruned is incremented at line 90 but never read — the per-snapshot logger.Info below already carries the information. Either drop the counter or use it (e.g. a single summary log with the count, which would also be quieter than one line per snapshot when RollbackWindow / SnapshotInterval is large).

[seidroid]

[suggestion] This default is a fleet-wide policy in a per-store field. As the RetentionWindow doc two dozen lines up says, the value is an input to a shared minimum — so 10000 here pushes pruneHeight 10k blocks deeper for ReceiptDB, the state WAL and the SC snapshots too, not just for BlockDB. Two things worth reconsidering: (a) whether the default should be 0 and the extra depth expressed once as RollbackWindow in StorageGarbageCollectorConfig, and (b) that AutobahnBlockDBConfig.LittBlockConfig exposes Retention but not RetentionWindow, so once wired there is no way to tune this from tendermint config.

[seidroid]

[suggestion] The comment you added here states the hazard precisely — a rename makes NewBlockDB open a fresh empty table while the old data sits under <root>/ledger/ "neither served nor reclaimed" — and then the rename is performed anyway, relying on "BlockDB is not deployed on any network yet" from the PR description. That holds for mainnet, but any dev/CI/devnet home directory carrying a ledger/ table silently comes up empty rather than failing, which is the worst shape for the one class of environment where it can happen.

Since the check is cheap and the comment already argues for it: os.Stat(<root>/ledger) at open and refuse to start (or log loudly) if it exists. That turns a silent empty store into a one-line operator action, and can be deleted once no such directories remain.

[seidroid]

Well-constructed implementation of gc.PrunableStore across BlockDB/ReceiptDB/StateWAL/FlatKV, with the ExternalPruning single-source-of-truth pattern correctly enforced at the collector (the choke point) and thorough per-store tests. I found no blocking correctness bug; the notes below are default/behavior changes that aren't called out in the PR description, plus some diff noise and deferred-wiring risks.

Findings: 0 blocking | 11 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Verification notes: I traced the concurrency claim added to seiwal.WAL.PruneBefore. Both real implementations (walImpl.PruneBefore → sendToWriter, serializingWAL.PruneBefore → submit) funnel through a channel with a shutdown-priority select, so the off-goroutine carve-out is genuinely honored. walsim.legacyWALShim.PruneBefore mutates a plain counter without a lock, but it satisfies walStore, not seiwal.WAL, so it is not bound by the new contract. Similarly confirmed CommitStore.GetLatestBlock's RLock matches Commit writing committedVersion under s.mu.Lock() (store_write.go:46,116), and that earliestVersion in the receipt store is monotonic in every writer, which is what the new gcFilter monotonicity requirement depends on.
• Stale comment: litt_block_db.go:397 still reads "gcFilter marks a key in the shared ledger table" after the table rename to blocks. Not on a changed line, so easy to miss.
• Nothing in the tree constructs a StorageGarbageCollector over these stores (GigaStorageConfig.PruningConfig is still unwired), so every new ExternalPruning stand-down path ships dark — pruneSnapshots, tryTruncateWAL, and startPruning all keep running in every real deployment. That is explicitly the stated scope, but it means the four-store end-to-end cycle has no coverage beyond mocks. Worth a follow-up integration test at the wiring PR.
• AutobahnBlockDBConfig (sei-tendermint/config/autobahn.go) exposes Retention and GCPeriod but no override for the new RetentionWindow, so sei-tendermint-configured block DBs are pinned to the 10000 default. Harmless today (no collector), but the knob will be needed at wiring time.
• TestReceiptLocalPrunerAdvancesFloorWithoutCollector spends up to 6 real seconds waiting on a 1–2s jittered ticker (~3 ticks of headroom). Under -race + coverage on a loaded CI shard that is a plausible flake source; the comment acknowledges the wait is deliberate, but consider making the prune interval injectable so the wait can shrink.
• refuseLegacyTable only distinguishes exists / not-exists; a file named ledger in a root would produce the "pre-rename table" error even though it is not a table directory. Cosmetic, and the operator action (move it aside) is the same.
• Second-opinion passes: Codex reported no material issues. cursor-review.md is empty — that pass produced no output, so it contributed nothing to this synthesis.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This silently drops the TTL failsafe from 24h to 1h (the setup_test.go assertion is updated to match, so it is deliberate), but the PR description doesn't mention it among the BlockDB changes.

RetentionTime is documented right above as the guard that "even an over-eager watermark cannot delete data younger than" — shrinking it 24× shrinks exactly that safety margin, and it applies today to autobahn/devnet block DBs that have no collector at all. Please call it out in the description, or keep 24h until the collector actually owns the watermark.

[seidroid]

[suggestion] I confirmed the safety claim for seid: readReceiptStoreConfig (app/receipt_store_config.go:27) overwrites KeepRecent unconditionally from min-retain-blocks, and KeepRecent is mapstructure:"-" so the app.toml template is unaffected. So no node changes behavior here.

The one caller worth flagging is DefaultGigaStorageConfig (sei-db/config/giga_config.go:58), which now hands a receipt store KeepRecent = 10000 instead of "keep everything". It has no production caller today, but it is the Giga wiring path this PR is building toward — a node constructed from it would serve eth_getTransactionReceipt for only ~10k blocks, and nothing in that path re-derives KeepRecent from min-retain-blocks. Consider setting it explicitly in DefaultGigaStorageConfig (or pinning it in giga_config_test.go) so the wiring PR can't inherit this default by accident.

[seidroid]

[suggestion] The doc is candid that there is no guard here and that the collector-exists check "belongs wherever the collector is eventually constructed" — agreed, but note this is the one place in the PR where the invariant is documentation only, and it is the highest-stakes one: this single flag stands down both pruneSnapshots and tryTruncateWAL, and the failure mode (nothing bounds snapshots or the state WAL) is silent and unbounded.

Per AGENTS.md's "guard at the choke point," the cleanest shape at wiring time is for NewStorageGarbageCollector to be the only thing that can turn this on — e.g. the collector constructor sets ExternalPruning on the configs it takes ownership of, rather than accepting configs that already claim it. Worth recording as the intended follow-up so the next author doesn't add a second setting that can disagree.

[seidroid]

[nit] Relocating InitializeDataDirectories / applyPebbleMetricsConfig from the top of the file to the bottom (and moving this interface assertion up in their place) is ~80 lines of pure churn unrelated to the GC work, and it makes this file's diff read as if it changed substantively when it didn't. Behavior is identical, so this is fine to keep — but a separate commit, or leaving it out, would make the PR easier to review.

Superseded: latest AI review found no blocking issues.

Superseded: latest AI review found no blocking issues.

[cursor]

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.}

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit 7ccdf29. Configure here.}

[seidroid]

Solid, unusually well-documented change: all four Giga stores implement gc.PrunableStore, the ExternalPruning flag genuinely makes "collector prunes me" and "my own pruner is off" a single fact, and the new GCFilter correctly makes the retention floor a precondition for litt reclamation. No blocking correctness or security issues found; the notes below are a coverage gap, a forward-looking guard gap, and some default/doc drift.

Findings: 0 blocking | 7 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion pass (cursor-review.md) is empty — that review produced no output. Codex (codex-review.md) reported no material issues, which matches my own read.
• sei-tendermint/config/autobahn.go:51 still documents Absent ⇒ littblock.DefaultConfig Retention; the field it resolves to is now RetentionTime. Outside the diff hunk, so noting it here.
• Wiring-PR hazard worth recording now: the collector holds a specific store instance, but CommitStore.reopenWAL() (sei-db/state_db/sc/flatkv/store.go:852) replaces s.wal with a freshly opened statewal.StateWAL on the state-sync/import path. A collector registered against the old stateWALImpl would then be pruning a closed WAL (returning errors every cycle) while the live one grows unbounded. The PrunableStore doc on stateWALImpl states the SC/SS co-management precondition but not this instance-identity one.
• Verified the claims I could check independently: readReceiptStoreConfig (app/receipt_store_config.go:27) does overwrite KeepRecent unconditionally, and DefaultGigaStorageConfig has no non-test callers — so the DefaultReceiptKeepRecent 0 → 10000 change really is confined to tools/tests as the description says. The tx-hash read path (GetReceiptFromStore) does enforce belowRetentionFloor, so shortening the litt TTL to a flat hour changes only physical reclamation, not visible retention.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This guard closes the pebbledb hole, but the littidx path can still reach the same silent-unbounded-growth state the guard exists to prevent: with ExternalPruning = true and KeepRecent == 0, runsLocalPruner() is false and GetRetentionWindow() folds to InfiniteRetentionWindow, so getCutLine returns 0, the store is never asked for a boundary, and never receives PruneBelow. Nothing prunes it.

That isn't an exotic combination — KeepRecent comes from min-retain-blocks, which defaults to 0, so it's the shape a wiring PR gets unless it remembers to set both. Not a live bug (nothing sets ExternalPruning yet), but it's exactly the combination the design section says should be unrepresentable rather than merely discouraged, and this switch is the choke point every receipt store passes through.

Suggest either rejecting ExternalPruning && KeepRecent <= 0 here alongside the pebble case, or — if "external pruning + keep everything" is meant to be legal — logging it once at open so an operator can tell it apart from a store whose floor has simply not moved yet.

[seidroid]

[suggestion] TestGCExternalPruningStandsDownSnapshotPruner pins the pruneSnapshots half of this flag, but nothing pins this half. Per the ExternalPruning doc in config/config.go, this is the higher-stakes of the two — dropping the || s.config.ExternalPruning here (or reordering the nil check) leaves the state WAL with both truncators off if the collector isn't registered, and the failure is silent and expensive.

A table-driven test in the shape of the snapshot-pruner one would do it: two CommitStores differing only in ExternalPruning, a WAL with a known stored range, tryTruncateWAL(), then assert the range moved in one case and not the other. snapshot_test.go:1510 already exercises tryTruncateWAL directly, so the harness exists.

[seidroid]

[nit] Worth calling out that this is a live default change, not just a rename: AutobahnBlockDBConfig.Retention is optional, so any autobahn node without an explicit retention in its config file goes from a 24h TTL failsafe to 1h. Benign as far as I can tell (the watermark is the visible-retention gate and reads below it are already refused, so this only makes reclamation of already-released data 24× more prompt), but the PR description presents the TTL rework under the ReceiptDB heading and this one reads as incidental to the rename.

Separately: RetentionWindow: 10000 on the next line has no path from AutobahnBlockDBConfig — the only two overrides are retention and gc_period — so whenever BlockDB is registered with a collector, autobahn nodes will be stuck on the hardcoded window until that config grows a key for it.

[seidroid]

A well-documented, test-heavy implementation of gc.PrunableStore across BlockDB, ReceiptDB, StateWAL and FlatKV; the collector integration, the ExternalPruning gate, and the new receipt GCFilter all hold up under inspection, and both new config fields default to false so there is no behavior change on any existing path. No blocking defects found — the notes below are a config trap the follow-up wiring PR will hit, a doc/implementation mismatch on the new PruneBefore concurrency contract, and one test-coverage gap.

Findings: 0 blocking | 11 non-blocking | 5 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, so this review merges only Claude's and Codex's findings.
• Test gap: nothing pins that the real receipt store attaches the GC filter. litt_receipt_gcfilter_internal_test.go builds its own litt table (and says so), and the store-level tests only assert the read-time floor (ErrNotFound), which is independent of GCFilter. Deleting tableConfig.GCFilter = s.gcFilter in newLittReceiptStore leaves the whole suite green while restoring exactly the TTL-leads-the-floor bug this PR exists to close. Consider a store-level test with tiny TargetSegmentFileSize/GCPeriod that drives a real reclamation through NewReceiptStore.
• Wiring-PR risk worth restating loudly (the PR body acknowledges it): FlatKVConfig.ExternalPruning stands down two mechanisms — pruneSnapshots and tryTruncateWAL — and nothing in flatkv can verify the state WAL was actually registered with a collector. Enabling that one flag without registering statewal leaves the state WAL with nothing bounding it at all. The receipt path got a startup refusal (newReceiptBackend rejecting pebbledb + ExternalPruning); the FlatKV path has none, so this is the single most expensive mistake available at wiring time.
• sei-db/state_db/sc/flatkv/store.go: moving InitializeDataDirectories/applyPebbleMetricsConfig to the bottom of the file and var _ Store = (*CommitStore)(nil) to the top is pure code motion unrelated to this PR's purpose. It is behavior-free (so it satisfies AGENTS.md's refactor rule), but it adds ~75 lines of unrelated diff noise to an already 2,257-line change.
• littReceiptStore.SetEarliestVersion (exported on the ReceiptStore interface) now has a materially stronger effect than before: advancing the floor releases receipt bodies to litt's GC for permanent reclamation, where previously it only masked reads. There is no production caller today, but a future one that advances the floor optimistically (e.g. a state-sync restore) would permanently delete bodies rather than temporarily hide them. Worth a line in that method's doc.
• Naming consistency nit: LittBlockConfig → BlockDBConfig and Retention → RetentionTime were renamed, but AutobahnBlockDBConfig.LittBlockConfig(dir) and its Retention field in sei-tendermint/config/autobahn.go keep the old names, so the tendermint-facing knob no longer matches the field it sets.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] ExternalPruning = true with the default KeepRecent = 0 yields no pruner at all — the exact silent, unbounded failure ExternalPruning is documented to make unrepresentable.

Trace: keepRecent <= 0 returns InfiniteRetentionWindow, and per gc/api.go that forces cutLine == 0, so the collector never asks this store for a boundary and never calls PruneBelow. Meanwhile runsLocalPruner() is already false (it requires keepRecent > 0). Both drivers are off and the tag index grows without bound.

The PR body defers reconciling KeepRecent's and RetentionWindow's disagreement about 0 to the wiring PR, which is reasonable — but the trap is cheap to close here, mirroring the guard that already exists one file over: reject ExternalPruning && KeepRecent <= 0 in newLittReceiptStore, the same way newReceiptBackend rejects pebbledb + ExternalPruning. That keeps the guard at the choke point every construction passes through instead of leaving it as a fact the wiring PR has to remember. No production impact today (the field is mapstructure:"-" and defaults false).

[seidroid]

[suggestion] The new contract says PruneBefore may be called "concurrently with any method including Close", but a prune racing a close is silently dropped while returning nil: submit can win its senderCtx check and enqueue serPrune behind serClose, and serializerLoop returns on serClose without draining the channel (seiwal_serializing.go:343-348). walImpl has the same shape.

Codex flagged this as High; I'd rate it lower — a dropped prune only defers reclamation (the WAL stays consistent, and the next cycle or the next process re-issues it), and the doc two lines below already says which records a prune reclaims is approximate and arbitrarily delayable. Reply channels for serFlush/serBounds/serIterator are unblocked by Close's s.cancel(nil), so nothing hangs.

Still worth one clause here: say that a prune racing Close may be dropped and that nil therefore does not promise the prune was scheduled. Otherwise the contract reads stronger than the two implementations deliver, and a future implementer will take it literally. TestGCPruneBelowBeforeClose only covers the ordered (non-racing) case.

[seidroid]

[nit] Codex flags this branch as violating the rollback guarantee (WAL pruned below a height this store cannot restore to). I checked and disagree that it's a correctness bug: this case is reachable only when the oldest snapshot is above cutLine, so there is nothing at or below cutLine for a lower answer to protect, and the WAL blocks below cutLine are unreachable anyway — replay is forward-only from a snapshot that doesn't exist. Returning CannotServeRollback here would stall every store's pruning indefinitely whenever snapshot retention is shallower than RollbackWindow, which is the worse failure. The doc comment above already reasons this through correctly.

What is missing is a signal. The store silently cannot serve the configured rollback window, and today the only place that surfaces is a failed rollback. A rate-limited logger.Warn naming cutLine and the oldest snapshot would turn a snapshot-retention shortfall into something operators can see before they need it — the CannotServeRollback path right below already logs, and this path is arguably the one more likely to persist unnoticed.

[seidroid]

[suggestion] This one line is the whole reason the retention floor gates reclamation, and no test covers it — the internal filter test builds its own litt table and explicitly notes it "cannot prove that the constructor attaches the filter". Removing this line leaves the entire suite green while making the TTL the sole reclaimer again, which is the bug the PR exists to fix. Worth a store-level test that drives a real reclamation through NewReceiptStore with tiny segment sizing.

[seidroid]

[nit] The BlockDB age failsafe drops 24h → 1h here (the receipt side goes the other way, from KeepRecent × 2s up to a flat 1h). The PR body states the new default, so this is disclosed — just confirming intent, since this value is the backstop against a watermark bug and it shrinks 24×. Autobahn devnets pick it up via DefaultConfig; AutobahnBlockDBConfig.Retention still overrides it, and the assertion in sei-tendermint/node/setup_test.go was updated to match.

[seidroid]

A well-tested, heavily-documented refactor of the storage GC contract; I found no blocking correctness bug in the collector's reduction, the per-store floors, or the new receipt GC filter. The non-blocking notes are a materially stale PR description, a few doc comments referring to methods this PR removed, and two default-behaviour changes that the "no behavior change by default" claim doesn't cover.

Findings: 0 blocking | 10 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The Cursor second-opinion pass produced no output (cursor-review.md is empty), so this review is Claude + Codex only. Codex reported "No material issues found in the reviewed diff."
• The PR description is substantially out of date relative to the diff and should be rewritten before merge. It describes GetRetentionWindow, GetPruningBoundary, PruneBelow, InfiniteRetentionWindow, a BlockDBConfig.RetentionWindow field, and a KeepRecent == 0 -> InfiniteRetentionWindow mapping — none of which exist in this diff. The actual API is GetRollbackFloor / PruneHistory / PruneSnapshots / ExternalPruning, plus a new fleet-wide LookbackWindow on StorageGarbageCollectorConfig that the description never mentions. Given how much this codebase leans on doc comments and PR text as the contract of record, the mismatch is worth fixing rather than leaving for the wiring PR.
• "Both ExternalPruning fields deliberately default to false: no behavior change by default" is narrower than the diff. Two defaults change for deployments with no collector: littblock.DefaultConfig TTL 24h -> 1h (reaches sei-tendermint via AutobahnBlockDBConfig whenever retention is unset — setup_test.go was updated accordingly), and the littidx receipt TTL goes from KeepRecent x 2s to a flat 1h with a new GCFilter gating reclamation. Both are defensible (reclamation now follows the retention floor rather than leading it), but they are live changes and belong in the description.
• Degenerate-config caveat worth a doc note: guarantee 1 on StorageGarbageCollectorConfig.LookbackWindow ("nothing needed to roll back to any block in [LatestBlock - RollbackWindow, LatestBlock] is deleted") is stated unconditionally, but the snapshot-shortfall path can answer above head - RollbackWindow — TestPruneDecisions/"a shortfall answer alone sets the cut lines" pins exactly that (95_000 with head 100_000 and window 10_000). It only binds when no contiguous store is registered, which the type-level preconditions already rule out, so this is a documentation gap rather than a defect — but the guarantee reads as absolute today.
• sei-db/state_db/sc/flatkv/store.go moves InitializeDataDirectories and applyPebbleMetricsConfig from the top of the file to the bottom, and relocates the var _ Store = (*CommitStore)(nil) assertion, with no behavioural change. That's ~75 lines of unrelated churn in a file this PR otherwise doesn't touch; splitting it out would keep the diff readable.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[nit] Stale reference: PruneBelow was replaced by PruneHistory on gc.PrunableStore in this same PR. Same on line 523 ("the collector's PruneBelow carries a minimum..."). Worth fixing here specifically because this comment is the pointer a reader follows to find the collector-side caller.

[seidroid]

[nit] PruneBelow no longer exists — the WAL's own method is Prune, the collector-facing one is PruneHistory, and the underlying seiwal.WAL method is PruneBefore. Suggest "by Prune or PruneHistory" so the sentence names methods that are actually on this type.

[seidroid]

[nit] "A sum that overflows uint64 is handled in getHistoryCutLine" no longer describes the code: the two windows are never added anywhere, and getHistoryCutLine only subtracts lookbackWindow from snapshotCutLine (guarding underflow, not overflow). This rationale looks carried over from the old getCutLine, which did add RollbackWindow + retention. Suggest: "getHistoryCutLine guards the underflow when the lookback window reaches below genesis."

[seidroid]

[suggestion] keepRecent > 0 && pruneInterval <= 0 is now a silently unbounded configuration. runsLocalPruner returns false, ExternalPruning is false so the collector never prunes either, so earliestVersion stays at 0 forever — and gcFilter refuses every key at floor 0, so litt reclaims nothing regardless of the TTL. Before this PR that combination still reclaimed bodies by TTL (SetTTL(KeepRecent * 2s) was set independently of the pruner cadence), so this swaps "bodies expire under a floor that never moved" for "nothing is ever reclaimed".

Arguably the new behaviour is the more honest of the two, and it needs prune-interval-seconds = 0 to be set explicitly (default is 600). But it is exactly the failure shape newReceiptBackend rejects for pebbledb + ExternalPruning: retention configured, no driver, silent unbounded growth. Consider rejecting it at construction the same way, or at least logging a warning — TestRunsLocalPruner already enumerates the case ("no cadence configured") without recording that it now means "never reclaim".

[seidroid]

[suggestion] The error return here is now vestigial. Every implementation in this PR (blockDB, littReceiptStore, stateWALImpl, CommitStore) returns a nil error unconditionally, and the doc itself says the collector never calls this method. The consequence is dead branches in each store's GetRollbackFloor — e.g. litt_block_gc.go's if err != nil || head <= rollbackWindow folds an unreachable condition into a reachable one, so the two cases can't be told apart in a log and neither can be tested. Either drop the error (making it GetLatestBlock() uint64) or keep it and note that it exists for implementations that don't yet exist; as written it reads like an error path someone should handle.

[seidroid]

A well-documented, heavily tested implementation of gc.PrunableStore across the four Giga stores, with no collector wired up yet so production impact is limited to a few default changes. I found no correctness blockers; the notable items are a dead config field on blockDB, an interface method the collector never calls, one untested stand-down path (tryTruncateWAL), a trivially-passing test, an unexplained 24h→1h TTL default drop, and a PR description that describes an earlier iteration of the design.

Findings: 0 blocking | 13 non-blocking | 7 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Codex's only finding (P1: PruneBefore racing Close on the plain closed field) does not hold, and I'd drop it. Neither walImpl.PruneBefore nor serializingWAL.PruneBefore reads closed — both go through sendToWriter/submit, which use a context.WithCancelCause and a channel send, both safe for concurrent use. closed is read only by owner-goroutine methods (Append, Flush, Iterator), and Close is itself an owner call, so the widened contract adds no unsynchronized access. The worst case is a prune enqueued during shutdown being dropped with a nil return, which the new doc already licenses ("may defer the work arbitrarily").
• cursor-review.md is empty — that pass produced no output, so this review merges only Claude's and Codex's findings.
• The PR description is substantially out of sync with the code it describes. It documents GetRetentionWindow, InfiniteRetentionWindow, PruneBelow, a BlockDBConfig.RetentionWindow config field, and a receipt TTL sized against RollbackWindow + KeepRecent. The merged interface is ExternalPruning/GetRollbackFloor/PruneHistory/PruneSnapshots with LookbackWindow on the collector config, and no RetentionWindow field is added to BlockDBConfig. For a 3k-line change whose whole value right now is the design record (nothing is wired), the description should be rewritten to match before merge.
• LookbackWindow = -1 combined with FlatKV ExternalPruning means the state WAL is never pruned at all: tryTruncateWAL has stood down and getHistoryCutLine pins historyCutLine to 0 so PruneHistory is never issued. The LookbackWindow doc frames -1 as "every block ever ingested stays readable"; worth saying plainly that on the SC path it also means unbounded state-WAL growth, which is the expensive half.
• Two load-bearing preconditions are documented but unenforced: the state WAL must be registered alongside SC/SS, and FlatKV's ExternalPruning must only be set where a collector actually exists. newReceiptBackend's pebbledb rejection shows the enforceable shape; the FlatKV/WAL side has nothing equivalent. Deferring to the wiring PR is reasonable, but that PR is the right place for a registration constructor that validates the set (e.g. reject a StateWAL registered without any snapshot store) rather than leaving it to reviewer discipline. Note also that memblock's blockDB does not implement gc.PrunableStore, so a memblock-backed BlockDB would drop out of the collector silently at wiring time.
• Since no StorageGarbageCollector is constructed, the only behavior shipping by default is: the receipt table's litt TTL is now set unconditionally (previously only when KeepRecent > 0) at a flat 1h with a new GCFilter, the BlockDB TTL default drop, refuseLegacyTable, and the flatkv code motion. I checked the always-on TTL is not a scan regression — litt's GC manager stops at the first key the filter blocks and resumes from a saved cursor, so a store whose floor is 0 aborts each pass immediately. Still worth calling the receipt TTL change out in release notes for rs-backend = "littidx" operators, as it is the one default-path retention change in the PR.
• 7 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] This field is assigned once (s.config = config, line 126) and never read anywhere in the package — grep '\.config' across littblock/*.go (non-test) returns only the assignment. The doc comments in litt_block_gc.go refer to config.RetentionTime, but the value that actually gates reclamation is the TTL already handed to littdb.DefaultTableConfig. unused isn't enabled in .golangci.yml, so nothing catches it. Drop the field and the assignment, or read it where the docs imply it is read.

[seidroid]

[suggestion] GetLatestBlock is now a required interface method that nothing in this package calls: prune uses only ExternalPruning() and GetRollbackFloor(), and describeDecisions renders only the floor. The doc justifies it as "here as the store's ingest position for operators and tests", but no operator can see it — it isn't in the prune log either, and it is exactly the number that explains a floor of 0 (empty vs. head-inside-window vs. unreadable). Either log it alongside the floor in describeDecisions, which makes the justification real, or drop it from the interface and let the four *_gc_test.go suites reach it through the concrete types (they all already hold one).

[seidroid]

[suggestion] A single store answering GetRollbackFloor == 0 halts pruning for the entire fleet, and this Info line is the only signal. That is reachable from ordinary transient conditions, not just misconfiguration: CommitStore.GetRollbackFloor returns 0 on any snapshot-scan error, an unresolvable current symlink, or a store still backfilling. It's also a behavioral change — the removed getGlobalLatestBlock deliberately excluded zero heads so a store still filling could not drag the head down, whereas now it pins both cut lines at 0.

The conservative direction is right, but a fleet-wide pruning stall that grows disk without bound should be alertable. Consider logging at Warn when snapshotCutLine == 0 (or after N consecutive such cycles), naming the store(s) that answered 0, so this is a metric/alert rather than log archaeology after a full disk.

[seidroid]

[suggestion] This stand-down has no test. config.go calls tryTruncateWAL the higher-stakes of the two mechanisms ExternalPruning disables ("a stray key would leave both the snapshots and the state WAL with nothing bounding them"), but only pruneSnapshotsByCount is pinned — by TestGCExternalPruningStandsDownSnapshotPruner. Deleting || s.config.ExternalPruning here fails nothing: the two existing tryTruncateWAL tests (snapshot_test.go:1510, :1552) never set the flag.

Since the whole argument for one field over two is that the stand-down cannot be forgotten, the half that is not pinned is the one a later change will quietly undo. A test in the shape of the existing one — commit past a snapshot with ExternalPruning on and off, assert GetStoredRange's first index moves only in the off case — would close it.

[seidroid]

[suggestion] The default drops 24h → 1h, shrinking by 24× the failsafe the field doc describes as protecting against an over-eager watermark ("even an over-eager watermark cannot delete data younger than RetentionTime"). The PR description explains at length why the receipt TTL became a flat duration and why littTTLPerBlock had to go, but says only that both "default to 1 hour" — it never addresses why BlockDB's existing margin was cut.

Note this also flows into sei-tendermint: AutobahnBlockDBConfig.LittBlockConfig starts from DefaultConfig, so any autobahn node that does not set Retention explicitly picks up the new value (node/setup_test.go re-pins it). Given BlockDB is not deployed on a network the blast radius is dev/CI/devnet, but the reasoning belongs in the field doc alongside the invariant it weakens.

[seidroid]

[nit] This assertion is trivially satisfied and would not catch the bug it is written against. setupLittIdxForGC writes no receipts, so latestVersion is 0 for both keepRecent values and GetRollbackFloor short-circuits on head <= rollbackWindow before keepRecent could matter — the test passes even if the floor were computed from keepRecent. The comment says it is "pinned across both readings of KeepRecent, since 0 used to mean 'keep everything' here and would once have suppressed pruning entirely", which is exactly the case the empty store hides.

Write a few blocks first (as the neighbouring tests do) and assert the same nonzero floor for both values.

[seidroid]

[nit] InitializeDataDirectories and applyPebbleMetricsConfig are moved verbatim from the top of the file to the bottom, and var _ Store = (*CommitStore)(nil) moved up into their place — ~80 lines of pure motion with no functional change, in a diff that is already 2990 lines. It resets git blame on both functions and is unrelated to the GC work. Worth dropping from this PR (or splitting out) so the reviewable surface is the retention change.

[seidroid]

A large but coherent refactor of the GC contract (PrunableStore now splits snapshot/history pruning and adds ExternalPruning + LookbackWindow), with unusually thorough per-store test suites and no behavior change by default. No blockers found; the notes below are a dead field, an unremarked default change, a missing test for one of the two mechanisms ExternalPruning stands down, and a stale PR description.

Findings: 0 blocking | 8 non-blocking | 4 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Cursor's second-opinion pass (cursor-review.md) is empty — no output was produced for this PR. Codex's pass produced exactly one finding, which I assessed and could not confirm (see the inline note on store_gc.go:196).
• The PR description is stale relative to the diff and should be rewritten before merge, since it is the main design record for this refactor. It describes GetRetentionWindow, InfiniteRetentionWindow, a BlockDBConfig.RetentionWindow config field, and an enforced retention of RollbackWindow + KeepRecent — none of which exist in the code. The shipped design removes all of those and uses a single StorageGarbageCollectorConfig.LookbackWindow instead. The "Retention semantics" and "What's included" sections both describe the previous iteration.
• Doc-comment duplication across the four *_gc.go files: the "0 when the window is deeper than the whole history" and "reports against its own head because the collector takes a minimum" rationale appears near-verbatim in litt_block_gc.go, litt_receipt_gc.go, and state_wal_gc.go, on top of already living on PrunableStore.GetRollbackFloor. Three copies of a shared contract will drift; the per-store comments could be cut down to what is actually store-specific.
• GetLatestBlock errors no longer reach the collector — getGlobalLatestBlock is gone and each store swallows the error inside GetRollbackFloor, returning 0. blockDB and stateWALImpl discard it without logging (FlatKV does log, three times). Today both of those implementations return a nil error unconditionally so nothing is lost, but the pattern means a future implementation that can fail would freeze pruning fleet-wide with a log line indistinguishable from a young chain.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] config is stored (s.config = config at line 126) but never read anywhere in the package — grep -rn '\.config' sei-db/ledger_db/block/littblock/ returns only the assignment. config.RetentionTime is consumed at construction time via tableConfig.TTL, so the retained pointer does no work.

Struct fields aren't caught by the unused linter, so this will sit here indefinitely. Either drop the field, or if it's groundwork for a follow-up, say so in a comment. Note the doc comments in litt_block_gc.go refer to "config.RetentionTime" as though this field were the source (PruneHistory, ExternalPruning), which reads as if it's live.

[seidroid]

[suggestion] The BlockDB default TTL failsafe drops from 24 * time.Hour to time.Hour — a 24× reduction — and sei-tendermint/node/setup_test.go was updated to match rather than the change being gated behind the collector.

BlockDB is not wired to a StorageGarbageCollector in this PR, so on the autobahn path today the only effect is that records the watermark has already released get reclaimed up to 23 hours sooner. That's defensible given the watermark is authoritative and this is only a failsafe, but it is a live behavior change on a shipping path, and it shrinks the window in which an operator could notice and recover from an over-eager watermark from a day to an hour.

The PR body mentions "defaulting to 1 hour" only in the context of the flat-duration rework; worth calling out explicitly as a default change (and confirming 1h is intended for the pre-collector world, not just the collector-managed one).

[seidroid]

[suggestion] Codex flagged this branch as High severity — "the collector then prunes every store to this higher height, deleting data within the configured rollback window." I don't think that holds: snapshotCutLine is a min across every store's floor (storage_garbage_collector.go:108), so an answer above head - rollbackWindow can never raise it. In exactly the case described (every snapshot above target), oldest > target >= every contiguous store's floor, so this answer is by construction never the minimum. TestPruneAsksEveryStoreOncePerCycle pins that reduction. No data inside the rollback window is deleted.

Two smaller things are real, though:

1. The doc at line 156 says "Reporting its oldest snapshot is what keeps that shortfall from compounding — the collector holds every store's history back to it." A min does the opposite: a higher answer holds nothing back, and some lower store binds instead. The sentence describes a max.

2. Reaching this branch means the store genuinely cannot restore as deep as RollbackWindow promises, and nothing says so. The old API had CannotServeRollback for precisely this class of signal; the new one has no equivalent, and floor=<n> in describeDecisions is indistinguishable from a healthy answer. A rate-limited logger.Warn here ("oldest snapshot %d is above the promised rollback floor %d") would make the shortfall visible while it self-resolves.

[seidroid]

[suggestion] This stand-down has no test. pruneSnapshotsByCount's equivalent guard is covered by TestGCExternalPruningStandsDownSnapshotPruner, but nothing asserts that tryTruncateWAL declines under ExternalPruning — store_gc_test.go never mentions the WAL path, and the two existing tryTruncateWAL call sites in snapshot_test.go (1510, 1552) predate this change.

By the PR's own reasoning this is the higher-stakes of the pair: config/config.go:54 says "this one flag stands down two mechanisms, pruneSnapshots and tryTruncateWAL," and if this one silently kept running it would truncate the state WAL to this store's earliest snapshot while the collector is holding the fleet's floor lower — i.e. pruning the WAL out from under SS. A table test in the shape of TestGCExternalPruningStandsDownSnapshotPruner (assert wal.Prune is not called with ExternalPruning set, and is with it unset) would close it without any timing dependency.

[seidroid]

The gc.PrunableStore rework (split PruneHistory/PruneSnapshots, GetRollbackFloor, ExternalPruning, new LookbackWindow) is coherent, unusually well documented in-code, and covered by a thorough per-store test suite including concurrency and real-litt-GC end-to-end cases; no store implements the interface without being updated, and the collector is still not constructed in production so both new flags are inert by default. I found no correctness blockers — remaining notes are a materially stale PR description, an observability gap under ExternalPruning, some stale identifier names left by the two renames, and Codex's (valid, design-level) point about self-pruning stores.

Findings: 0 blocking | 9 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The PR description no longer matches the diff, and it is the design record for this change. It describes GetRetentionWindow / GetPruningBoundary / PruneBelow / InfiniteRetentionWindow / CannotServeRollback and a new BlockDBConfig.RetentionWindow field — none of which exist in the final code — and never mentions LookbackWindow, GetRollbackFloor, or the PruneHistory/PruneSnapshots split, which are the PR's central design. The "Retention semantics" and "ExternalPruning() bool" sections both reason from the removed per-store retention model. Worth refreshing before merge.
• Cursor's second-opinion pass produced no output (cursor-review.md is empty), so this review merges only my findings with Codex's.
• The two renames leave stale wording behind: litt_block_db.go:121 still errors with "failed to build ledger table", and litt_block_db.go:395, litt_block_iterator.go:25, litt_block_simple_iterator.go:17, littblock_crash_test.go:24 all still say "the shared ledger table". Also AutobahnBlockDBConfig.LittBlockConfig() now returns a BlockDBConfig, so the method name no longer matches its return type.
• Verified as safe rather than flagged, for the record: the receipt gcFilter's length check can only error if the table's key layout changes — every primary key is written via littPartKey and tx hashes go in as litttypes.SecondaryKey, which litt reports as non-primary via sk.Kind.IsPrimary(); and the new seiwal.WAL.PruneBefore concurrency guarantee holds for both real implementations, since each only does a context-guarded channel send (sendToWriter/submit) and touches no plain field that Close mutates.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[nit] Stale method name in a comment added by this PR: the collector reaches pruneBlocksBelow via PruneHistory now, not PruneBelow. Same on line 523 ("the collector's PruneBelow carries a minimum..."). These are the only two places left pointing at the removed method name, so they read as a reference to something a grep won't find.

[seidroid]

[suggestion] Agreeing with Codex here, but at design-note rather than bug severity. The doc on ExternalPruning says a self-pruning store "is still protected by the minimum its answer produced", and that is true — but the converse is not: a self-pruning store can raise the minimum above the promised rollback target, so the fleet-wide RollbackWindow guarantee is not actually enforced for it. Concretely, a FlatKV with ExternalPruning=false and SnapshotKeepRecent small enough that every surviving snapshot sits above head - RollbackWindow hits snapshotFloor's shortfall branch and answers its oldest snapshot; that becomes snapshotCutLine, and the WAL is pruned to snapshotCutLine - LookbackWindow. Rolling back to head - RollbackWindow is then impossible even though the collector reported a successful cycle.

Nothing here is wrong — the shortfall branch is deliberate and TestPruneDecisions/"a shortfall answer alone sets the cut lines" pins it — and no production path constructs a collector yet, so this is inert. But the guarantee stated on StorageGarbageCollectorConfig.RollbackWindow ("Nothing needed to roll back to any block in [LatestBlock - RollbackWindow, LatestBlock] is deleted") holds only when every participating store can in fact serve that depth. Worth either narrowing that wording, or logging when a store's floor exceeds head - RollbackWindow so the shortfall is visible rather than inferred from the numbers.

[cody-littley]

Nit, not blocking. Perhaps we could instead name this file prunable_store.go? If we name it like this, then it becomes easy to search for this file by name. If I search "api" in my IDE, I get lots of hits.

[cody-littley]

This interface has overly verbose. Claude really likes generating long godocs that are not extremely helpful for understanding the code.

My general rules I give my LLM when generating godocs are as follows:

• Explain WHAT, not WHY or HOW.
• Never explain the history of design decisions in a godoc.
• Multi-paragraph godocs should be rare, in general. Most functions don't deserve multiple paragraphs of explanation.
• When editing a godoc to conform to these rules, it is often better to rewrite the godoc instead of making incremental edits. LLMs often struggle keeping a godoc cohesive when editing, which often leads to rambling paragraphs.
• Although sometimes appropriate, in general it's the wrong move to describe the system as a whole in a godoc. Most godocs should focus on the subject matter at hand, not encode an entire system's design doc.

[cody-littley]

I suggest explaining it like something like this:

There are two dimensions of garbage collection: snapshots, and history. Snapshots are a mechanism utilized to roll back the chain. History is what lets data stores answer queries about historical blocks. Not all stores have a concept of "history" (e.g. SC), and not all stores have "snapshots" (e.g. the state WAL).

[cody-littley]

I suggest avoiding details about how the garbage collection works in this godoc. Implementers of this godoc don't need to know how the garbage collector aggregates this data together with other stores, they just need to know the required contract for these methods.

[cody-littley]

Suggest avoiding details about the top level GC algorithm, and instead focusing on the specific contract required to be implanted by this method.

e.g.

If this store does not have the concepts of snapshots, it should return X. If it does manage snapshots,
it should return the block number of its highest snapshot that occurs on or before `rollbackWindow`.
If it has no snapshots (e.g. we are starting from genesis or a state sync), it should return Y.

[cody-littley]

Same comment, avoid talking about what the garbage collector does, and instead focus on what implementations of this interface are required to do.

[cody-littley]

It's unclear what "which it resolves against its own head" means

[seidroid]

Large but carefully-constructed rework of gc.PrunableStore (floor/snapshot/history split plus ExternalPruning) with all four stores implementing it and unusually thorough per-store tests. I found no correctness blockers — the collector's reduction is conservative in the safe direction (any store answering 0 stops the whole cycle), the receipt gcFilter is monotonic and correctly gated behind the retention floor, and the WAL's off-goroutine PruneBefore carve-out is genuinely satisfied by both real implementations. Remaining notes are about an unconditional SetTTL that now enables litt GC on keep-everything nodes, dead interface surface, a couple of doc/log gaps, and a stale PR description.

Findings: 0 blocking | 13 non-blocking | 5 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• PR description is stale relative to the merged code: it documents GetRetentionWindow, InfiniteRetentionWindow, PruneBelow, and a BlockDBConfig.RetentionWindow field, and the whole "Retention semantics" section is written in terms of R = GetRetentionWindow. The final diff replaced all of that with GetRollbackFloor + a single collector-level LookbackWindow, and BlockDBConfig has only Litt and RetentionTime. Worth rewriting before merge — this description is the main design record for the follow-up wiring PR.
• Stale-reference hazard the wiring PR will need to handle: CommitStore.reopenWAL() (store.go, state-sync import path) closes the current statewal.StateWAL and installs a fresh instance. A collector registered with the pre-restore stateWALImpl would then call PruneHistory on a closed WAL forever — erroring every cycle at Error level — while the live WAL grows unbounded. Since FlatKV's tryTruncateWAL also stands down under ExternalPruning, nothing else would truncate it. Worth adding to the "Not in this PR" list.
• The new off-goroutine guarantee added to seiwal.WAL.PruneBefore is not met by every in-tree implementation: walsim.legacyWALShim.PruneBefore mutates s.pruneRequestCount unsynchronized. walsim has no consumers outside itself so nothing breaks today, but the interface doc now makes this a contract violation.
• Cursor's second-opinion file (cursor-review.md) is empty — that pass produced no output. Codex reported "No material issues found in the reviewed diff."
• store.go moves InitializeDataDirectories/applyPebbleMetricsConfig to the bottom of the file and var _ Store = (*CommitStore)(nil) to the top, with no functional change. That's ~80 lines of pure motion in an already 2670-line diff; splitting it out would make the substantive changes easier to read.
• Stale comment: litt_receipt_store.go GetReceiptFromStore still says "Enforce the KeepRecent floor" (unchanged line ~273), but under ExternalPruning the floor is driven by the collector, not KeepRecent. The type-level doc above it was updated; this one was missed.
• Test-coverage asymmetry: flatkv and statewal each got a -race test driving the PrunableStore surface concurrently with a writer (TestGCConcurrentWithCommitter, TestGCConcurrentWithWriter), but blockDB and receiptDB did not. Both look safe (blockDB's surface is mu-protected, receiptDB's reads only atomics), so this is low risk — but the pair of existing tests suggests the intent was to cover all four.
• AGENTS.md gains a new "Godoc" section unrelated to this PR's subject. Harmless, but it makes the diff span guidance and implementation.
• 5 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

Well-structured rework of the GC contract (PruneHistory/PruneSnapshots/ExternalPruning/GetRollbackFloor) with genuinely strong test coverage per store; I found no correctness blockers, and the default-off ExternalPruning flags keep this a no-op change in production. The main issues are a contract-vs-implementation gap in the state WAL's PruneHistory, a now-unused interface method, and a PR description that documents an API this diff no longer contains.

Findings: 0 blocking | 12 non-blocking | 6 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• The PR description is substantially out of sync with the code it describes. It documents GetRetentionWindow, GetPruningBoundary, PruneBelow, InfiniteRetentionWindow, and a BlockDBConfig.RetentionWindow config field ("New litt_block_gc.go; adds a RetentionWindow config field", "littblock.DefaultConfig now leaves RetentionWindow at 0", the whole "Retention semantics" section). None of those exist in this diff — grep -rn RetentionWindow --include=*.go . returns nothing, and the interface is PruneHistory/PruneSnapshots/ExternalPruning/GetRollbackFloor. LookbackWindow, which the code actually adds and which the guarantees now hinge on, is never mentioned. Please rewrite the body against the final API before merging; it is the design record for a change this size.
• Cursor's second-opinion pass (cursor-review.md) produced no output — that file is empty, so this review reflects Claude + Codex only.
• Forward-looking gap for the wiring PR: CommitStore.Rollback closes s.wal and replaces it with a fresh statewal.New(cfg) instance (sei-db/state_db/sc/flatkv/snapshot.go:687-706). A collector registered against the original stateWALImpl would hold a closed WAL after any rollback, and PruneHistory would then error every cycle forever — TestGCPruneHistoryAfterClose pins exactly that behavior. Worth capturing as a constraint now, since nothing in this PR would catch it.
• Unrelated churn in an already-large diff: sei-db/state_db/sc/flatkv/store.go moves InitializeDataDirectories and applyPebbleMetricsConfig verbatim to the end of the file and relocates var _ Store = (*CommitStore)(nil), with no behavior change; the AGENTS.md godoc section is likewise independent of the GC work. Both would read better as separate commits/PRs.
• No test covers stateWALImpl.PruneHistory with a cutoff above the WAL's head, which is the one case the PrunableStore contract calls out and the one the other three stores each test (TestGCPruneHistoryAboveHeadIsCapped, TestReceiptGCPruneHistoryAboveHead). Adding it would make the inline finding above self-enforcing.
• Minor observability note: the collector no longer returns an error when a store cannot answer — GetRollbackFloor failures collapse into a floor of 0, which is indistinguishable in the Info-level prune log from a young chain. TestPruneUnreadableStoreStopsTheCycle confirms the safe behavior, but a persistently broken store now stalls all pruning with no error-level signal. A warn/metric after N consecutive cycles held at 0 would close that.
• 6 suggestion(s)/nit(s) flagged inline on specific lines.

[seidroid]

[suggestion] PruneHistory forwards blockNumber straight to PruneBefore with no head clamp, but the interface contract at sei-db/management/gc/prunable_store.go:17-18 states: "blockNumber may fall outside the range this store holds, including above its own head. Such a request must be clamped to a no-op rather than emptying the store." walImpl.PruneBefore (sei-db/seiwal/seiwal_impl.go:340) does no clamping either — it schedules removal of every sealed file below the index — so an above-head cutoff would drop the entire WAL, and SC/SS would lose the range they replay from.

Not reachable through the collector today: historyCutLine <= snapshotCutLine <= min(floors) <= this WAL's own floor <= its head, so the value never exceeds the head. But the other three implementations all clamp (blockDB caps to the newest cohort, pruneBlocksBelow caps to latestVersion, FlatKV no-ops), which makes this the odd one out and the one a future direct caller would trip on.

Either add the guard (if head, _ := w.GetLatestBlock(); blockNumber > head { blockNumber = head }) with a test, or tighten the interface doc to say the collector never issues an above-head history cutoff — but as written the contract and the implementation disagree. (Also raised by Codex.)

[seidroid]

[suggestion] GetLatestBlock is no longer called by the collector. prune now only calls ExternalPruning() and GetRollbackFloor(); the old getGlobalLatestBlock reduction was deleted with this rework. Every implementation calls its own GetLatestBlock internally from GetRollbackFloor, so nothing outside each store needs it on the interface.

Keeping it obliges every future PrunableStore to expose (and get right) a method the collector never reads, and it is the only method whose error return the collector would have had to handle. Consider dropping it from PrunableStore and leaving it as a concrete method on each store — the existing *_gc_test.go assertions on head values keep working either way.

[seidroid]

[suggestion] The default TTL failsafe drops 24× (24h → 1h). The stated justification in the PR body is that "how much history BlockDB keeps is RetentionWindow" — but no RetentionWindow field is added by this diff, and the new doc above points at RollbackWindow/LookbackWindow on a collector that this PR explicitly does not wire up ("Not in this PR").

So for BlockDB as actually deployed today (autobahn driving PruneBefore directly, no collector), this shortens the window in which an over-eager watermark is recoverable from a day to an hour, with nothing added in this PR to compensate. The refuseLegacyTable guard added alongside implies dev/CI/devnet homes do exist. Either keep 24h until the collector is wired, or say in the config doc why 1h is sufficient for the un-collected path.

(Receipts moving to a flat littRetentionTime = 1h reads fine by contrast, since gcFilter gates reclamation on the floor there.)

[seidroid]

[nit] F = LatestBlock - RollbackWindow - LookbackWindow references a global LatestBlock that the collector no longer computes — getGlobalLatestBlock is deleted in this PR, and each store now measures the window against its own head. The height actually used is min(GetRollbackFloor) - LookbackWindow, which for a snapshot store lands on a snapshot at or below head - RollbackWindow and is therefore often well beneath the F written here.

Guarantee 2 (the safety one) still holds — the real cut line is never above F. Guarantee 3 ("eventually delete data older than F") only holds asymptotically, as snapshots advance. Since this block is presented as the formal spec for the subsystem, it's worth restating F in terms of the minimum-of-floors reduction the code performs.

[seidroid]

[nit] "prunes a set of PrunableStores which prune old snapshots/checkpoints and history data" parses as the stores doing the pruning, which is the opposite of the arrangement. Also "setting" should be plural. Something like: "StorageGarbageCollector periodically prunes a set of PrunableStores, dropping snapshots and history according to RollbackWindow and LookbackWindow."

[seidroid]

[nit] err != nil is unreachable here: GetLatestBlock (line 49 below) returns nil on every path. Same in littReceiptStore.GetRollbackFloor — its GetLatestBlock also never errors. Only FlatKV's can actually fail, and that one logs before returning 0.

Harmless, but the accompanying doc ("or when the head cannot be read") describes a case that cannot occur for these two, which is misleading if someone later relies on it as a real failure path. Either drop the branch or drop the error return from these concrete methods (see the note on PrunableStore.GetLatestBlock).