Add giga config and unify garbage collector logic - #3829
Conversation
PR SummaryMedium Risk Overview Adds Giga-oriented wiring: Collector defaults change (rollback window 1_000, 10m prune interval via Reviewed by Cursor Bugbot for commit 6cfcc2c. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3829 +/- ##
==========================================
- Coverage 61.29% 60.41% -0.89%
==========================================
Files 2351 2262 -89
Lines 197491 186891 -10600
==========================================
- Hits 121060 112915 -8145
+ Misses 65579 63982 -1597
+ Partials 10852 9994 -858
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The garbage-collector generalization (single PrunableStore list + StoreType) is a clean refactor with good test coverage and no production implementers to break, but the unrelated new sei-db/config/giga_config.go ships a default config that cannot pass FlatKV's own Validate() because FlatKVConfig.DataDir is left empty. Secondary notes on the fat PrunableStore interface, an unconditional GetLastCommittedBlock call that contradicts its documented contract, and a silently-no-op empty store list.
Findings: 2 blocking | 12 non-blocking | 8 posted inline
Blockers
sei-db/config/giga_config.gois entirely new, entirely unused (no caller ofGigaStorageConfig,DefaultGigaStorageConfig,DefaultRollbackWindow, orDefaultRetentionanywhere in the tree), untested, and unrelated to the PR title "Unify and make garbage collector more generic". Either wire it up in this PR or split it out — as it stands it's dead config that the FlatKVDataDirbug below shows nobody has exercised.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only the Codex findings merged with my own. - Test gap:
mockSnapshotStore.GetLastCommittedBlockandGetStoredSnapshotsshare a singlegetErrfield, so the new"failed to read last committed block from %s"error path inobserveStoresis never exercised independently. Give the mock a separatelastCommittedErrand add a case. - Test gap: the
default:branch inobserveStores(unsupported store type at prune time) is unreachable throughNewStorageGarbageCollectorbut reachable by callingprunedirectly —TestNewStorageGarbageCollectorRejectsUnsupportedStoreTypeonly covers the constructor. A directprune([]PrunableStore{&mockUnknownStore{...}})case would pin it. - Design note for the follow-up work this PR enables:
pruningFloorsgives every stream store the minimum snapshot floor across all snapshot stores. That is correct and safe for the state WAL, but once a block DB or receipt stream is registered as aStreamStoreit will be forced to retain from the SC/SS snapshot floor even though nothing replays it from those snapshots — over-retention that is invisible at runtime. Worth considering whether streams need to declare which snapshot stores depend on them. - No prompt-injection or other suspicious content found in the diff, commit messages, or PR description.
- 7 suggestion(s)/nit(s) flagged inline on specific lines.
| RollbackWindow: DefaultRollbackWindow, | ||
| BlockRetention: DefaultRetention, | ||
| DataDirectory: dbDir, | ||
| FlatKVConfig: flatkvConfig.DefaultConfig(), |
There was a problem hiding this comment.
[blocker] flatkvConfig.DefaultConfig() does not set DataDir, and flatkv/config.Validate() hard-rejects an empty one (config.go:165 — "data dir is required"). So DefaultGigaStorageConfig(dbDir) returns a config that is guaranteed to fail validation for its first consumer, even though dbDir is threaded into DataDirectory and BlockDBConfig right above.
Note this is unlike SSConfig.DBDirectory / ReceiptDBConfig.DBDirectory, which are documented overrides with a fallback — FlatKV has no fallback.
Suggested fix, matching how the flatkv tests root it (filepath.Join(dir, flatkvRootDir)):
flatKVConfig := flatkvConfig.DefaultConfig()
flatKVConfig.DataDir = filepath.Join(dbDir, <flatkv root subdir>)(Also flagged by Codex.)
| // Both GetStoredSnapshots and GetBlockRange must be implemented, but only the accessor matching the store's | ||
| // StoreType is required to carry information: a StreamStore has no discrete snapshots to report, and a | ||
| // SnapshotStore reports the range spanned by the snapshots it holds. | ||
| type PrunableStore interface { |
There was a problem hiding this comment.
[suggestion] Collapsing the two interfaces into one forces every implementation to provide accessors that are never called for its type: observeStores only calls GetStoredSnapshots for a SnapshotStore and only GetBlockRange for a StreamStore. Every future stream store therefore has to write a GetStoredSnapshots() { return nil, nil } stub (as mockStreamStore now does) that no code path reads.
An alternative that keeps the single-slice API the PR is after without the dead methods: a narrow base interface (Name, GetStoreType, PruneBelow, GetLastCommittedBlock) plus small SnapshotSource/StreamSource interfaces asserted in observeStores alongside the existing type switch. A store then physically cannot claim a type whose accessor it doesn't implement, instead of the current convention that it must implement both and leave one empty.
| return nil, fmt.Errorf("store %s has unsupported store type %s", store.Name(), obs.storeType) | ||
| } | ||
|
|
||
| latestBlock, err := store.GetLastCommittedBlock() |
There was a problem hiding this comment.
[suggestion] GetLastCommittedBlock is called for every store, including ones that just reported hasData == false, and any error aborts the whole cycle. But the contract at store_types.go:58 says the value is "Meaningful only when the store has data" — so an implementation is entitled to error or return junk when empty.
That turns the benign "skipping pruning, not all stores have data" path (a fresh node, or a snapshot store mid-first-write) into a prune cycle failed error logged every interval. Since obs.latestBlock is only ever consumed after the allStoresHaveData gate, guarding the call is free:
if obs.hasData {
latestBlock, err := store.GetLastCommittedBlock()
...
}| stateStoreBlocks[i] = blocks | ||
| if len(blocks) == 0 { | ||
| anyStoreEmpty = true | ||
| if len(observations) == 0 { |
There was a problem hiding this comment.
[suggestion] An empty store list now returns silently every cycle, and NewStorageGarbageCollector accepts it (asserted by TestNewStorageGarbageCollectorAcceptsAnyMixOfStoreTypes's "no stores" case). Previously the WAL was a required parameter, so this configuration was unrepresentable.
That makes a wiring bug — forgetting to pass the stores — indistinguishable from healthy operation: no pruning ever happens and nothing is logged. Consider a one-time logger.Warn at construction when len(stores) == 0.
| type StreamStore interface { | ||
| // PrunableStore is a store whose old data may be dropped by the StorageGarbageCollector. | ||
| // | ||
| // Both GetStoredSnapshots and GetBlockRange must be implemented, but only the accessor matching the store's |
There was a problem hiding this comment.
[nit] These three lines contradict themselves: "only the accessor matching the store's StoreType is required to carry information" is immediately followed by "a SnapshotStore reports the range spanned by the snapshots it holds" — which asks a SnapshotStore to populate the non-matching accessor. The collector never calls GetBlockRange on a SnapshotStore, so the second clause is both unenforced and misleading. Drop it, or say explicitly that it is optional and unused.
| func DefaultGigaStorageConfig(dbDir string) GigaStorageConfig { | ||
| blockDBConfig, err := littblock.DefaultConfig(dbDir) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
[suggestion] panic on a config-construction error is out of step with the rest of sei-db/config (DefaultStateStoreConfig, DefaultReceiptStoreConfig) and with littblock.DefaultConfig itself, which deliberately returns an error. Prefer func DefaultGigaStorageConfig(dbDir string) (GigaStorageConfig, error) and let the caller decide.
| flatkvConfig "github.com/sei-protocol/sei-chain/sei-db/state_db/sc/flatkv/config" | ||
| ) | ||
|
|
||
| const DefaultRollbackWindow = 1000 |
There was a problem hiding this comment.
[nit] DefaultRollbackWindow = 1000 silently disagrees with management.DefaultStorageGarbageCollectorConfig(), which uses RollbackWindow: 10_000 for the same concept. Two defaults for one knob will drift; derive this from the management default (or vice versa) rather than restating it.
|
|
||
| type GigaStorageConfig struct { | ||
| RollbackWindow uint64 | ||
| BlockRetention uint64 |
There was a problem hiding this comment.
[nit] BlockRetention has no doc comment and no unit — DefaultRetention = 1000000 reads as blocks, while the block DB's own retention (littblock.LittBlockConfig.Retention, defaulted to 24h) is a time.Duration. Nothing reads this field yet, so document the unit and its relationship to LittBlockConfig.Retention before a consumer guesses wrong. The other fields in this struct are also undocumented, unlike the neighbouring configs in this package.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found by the bug hunting system, and the logic checked out in my own read of the diff — but this rewrites the core prune-floor / rollback invariant logic (new GetLastCommittedBlock-based head computation across heterogeneous stores, generalized snapshot/stream floor logic) in a data-retention-critical path, so I'd like a human to sign off before merge.
Extended reasoning...
Overview
The PR generalizes StorageGarbageCollector to operate over a single heterogeneous []PrunableStore list instead of separate snapshot-store/WAL arguments, introduces a StoreType enum (SnapshotStore/StreamStore) to drive per-store observation, and adds a new GetLastCommittedBlock() accessor used to compute the rollback target as the minimum committed height across all stores (previously it was just the WAL's end). It also adds a GigaStorageConfig defaults struct and removes two empty placeholder packages.
Security risks
None of the classic categories (auth, injection, crypto) apply here; the risk surface is data-integrity/availability — an incorrect prune floor could either over-prune (breaking rollback capability) or under-prune (disk growth). I traced pruningFloors, committedBlockRange, and the empty-store skip logic and they look correct, including the edge cases the tests exercise (lagging snapshot store, multiple stream stores, snapshot-only sets, store order independence).
Level of scrutiny
This warrants more than a mechanical-change level of review: it rewrites the invariant-preserving logic for a garbage collector whose entire job is 'never delete data needed for rollback.' The change is currently well isolated — grep confirms no production code calls NewStorageGarbageCollector or references GigaStorageConfig yet, so nothing is live-wired to this logic in this repo state — but the logic itself is exactly the kind of critical-path correctness change that should get a second set of human eyes before it's built upon.
Other factors
Test coverage is thorough (decision-matrix table tests, two property-based tests via random sorted input, explicit tests for multi-stream-store and lagging-store scenarios), and go test ./sei-db/management/... passes. The bug hunting system found nothing, and I found nothing beyond it, but coverage and my own read don't substitute for a human review of a rollback-safety invariant change.
yzang2019
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
The GC refactor into sei-db/management/gc is well-documented and well-tested, but it silently inverts the previous "an empty store means unknown, so skip pruning entirely" safety guard in a data-deletion path, and the new GigaStorageConfig duplicates the GC's retention knobs with divergent values, panics in its constructor, and lacks the mapstructure tags every sibling config in that package has.
Findings: 1 blocking | 13 non-blocking | 8 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The PR description is empty (both template sections blank). This PR changes GC defaults (RollbackWindow 10,000 -> 1,000, prune interval 60s -> 600s), adds a new retention knob, and reworks the store interface contract; please record the reasoning and any testing performed.
- Neither
gcnorGigaStorageConfigis referenced anywhere in the tree (verified by grep). Assuming they're wired up in a follow-up in the stack, that's fine, but worth stating in the description so reviewers know the new defaults aren't live yet. - Retention regression worth acknowledging explicitly: previously each snapshot store was pruned to its own floor and only the WAL was pruned to the minimum. Now every store is pruned to the global minimum, so SC/SS/flatKV retain strictly more snapshots than before. The config's TODO acknowledges this; a note in the PR body about the expected disk impact would help.
- Codex flagged the
management->management/gcmove as a breaking API change needing compatibility aliases. I checked and disagree that it blocks:sei-dbis part of the root module (no separate go.mod), nothing in the repo importssei-db/management, and the base package contained only the four moved files. No aliases needed. - The Cursor second-opinion file (
cursor-review.md) is empty, so that pass produced no output. - I could not run
go build ./sei-db/...orgo test ./sei-db/management/gc/in this environment (sandbox denied), so the new tests are reviewed but unverified by execution. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| var pruneHeight uint64 | ||
| for _, store := range s.stores { | ||
| oldestBlockToRetain := store.GetOldestBlockToRetain(cutLine) | ||
| if oldestBlockToRetain == 0 { |
There was a problem hiding this comment.
[blocker] This inverts a deliberate safety guard from the code being deleted. The old prune bailed out of the whole cycle if any snapshot store returned no blocks, with the rationale: "An empty store is treated as unknown rather than empty (a snapshot may be mid-write and take hours), and pruning against it would risk breaking the primary invariant."
Here a store answering 0 is skipped and the remaining stores are pruned anyway (codified by the a store holding nothing is ignored test case, which prunes to 80,000 while one store holds nothing). The api.go contract justifies this with "a store holding nothing cannot serve a rollback to any height" — true for a store that permanently holds nothing, but it doesn't cover the case the old comment was guarding: a store that holds nothing yet because its first/next snapshot is mid-write. If that snapshot lands for a height below pruneHeight, the contiguous stores have already dropped the blocks needed to make it usable, and the rollback invariant is broken by the act of deletion. At Sei block rates a multi-hour snapshot write is well within the 101,000-block default retain window.
Since this is an unrecoverable-deletion path, please either restore the conservative behavior (skip the cycle when a store answers 0 while others report data) or document why mid-write snapshots can't fall below the cut line.
| func DefaultGigaStorageConfig(dbDir string) GigaStorageConfig { | ||
| blockDBConfig, err := littblock.DefaultConfig(dbDir) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
[suggestion] Panicking in a default-config constructor is out of step with the rest of this package (DefaultStateStoreConfig, DefaultReceiptStoreConfig, flatkvConfig.DefaultConfig all return plain values). Prefer func DefaultGigaStorageConfig(dbDir string) (GigaStorageConfig, error).
Also note the panic is currently unreachable: littblock.DefaultConfig(dir) forwards to littdb.DefaultConfig(paths ...string), whose only error is len(paths) == 0, and a single dir argument is always length 1 — even dbDir == "" passes. So this doesn't actually catch a bad dbDir; it just adds a crash path if that function grows new error cases.
| ) | ||
|
|
||
| const DefaultRollbackWindow = 1000 | ||
| const DefaultRetention = 1000000 |
There was a problem hiding this comment.
[suggestion] This PR introduces two independent sources of truth for the same two knobs, and they disagree. gc.DefaultStorageGarbageCollectorConfig() uses RollbackWindow: 1000 / StoreRetention: 100_000, while here DefaultRetention = 1000000 — a 10x divergence on retention. Given the PR title is "unify garbage collector logic", GigaStorageConfig should either embed/reference gc.StorageGarbageCollectorConfig or derive these from the gc package constants rather than redeclaring them.
Minor: use 1_000_000 to match the digit-grouping convention used throughout sei-db.
| const DefaultRollbackWindow = 1000 | ||
| const DefaultRetention = 1000000 | ||
|
|
||
| type GigaStorageConfig struct { |
There was a problem hiding this comment.
[suggestion] Every other config struct in this package (StateStoreConfig, ReceiptStoreConfig) carries mapstructure tags so the values can be read from app.toml. Without them GigaStorageConfig can only ever hold compiled-in defaults. If these are meant to be operator-tunable, add the tags now (and see testutil/configtest/AGENTS.md for pinning the new keys); if they're intentionally internal-only, a short comment saying so would prevent the next person from assuming they're configurable.
| return nil | ||
| } | ||
|
|
||
| logger.Info("pruning storage", "pruneHeight", pruneHeight) |
There was a problem hiding this comment.
[nit] Observability regression relative to the deleted implementation, which logged per-store initial and final block sets alongside latestBlock/oldestBlockNeeded. For an operation that irreversibly deletes data, logging which store's answer produced pruneHeight (and each store's answer) makes post-hoc diagnosis much easier. Consider adding the per-store GetOldestBlockToRetain results to this line.
| "github.com/sei-protocol/seilog" | ||
| ) | ||
|
|
||
| var logger = seilog.NewLogger("db", "management") |
There was a problem hiding this comment.
[nit] The logger is still tagged "management" after the move into package gc. Worth updating to seilog.NewLogger("db", "gc") (or "management/gc") so log filtering matches the new package.
|
|
||
| // PrunableStore is a store whose old data may be dropped by the StorageGarbageCollector. | ||
| type PrunableStore interface { | ||
| // Name Return the name of the store. |
There was a problem hiding this comment.
[nit] Godoc convention: // Name returns the name of the store. — the current // Name Return the name of the store. reads as if Return were part of the identifier. Same pattern is worth checking on the other methods.
There was a problem hiding this comment.
I didn't find any additional bugs beyond what's already been flagged, but given this rewires the storage garbage collector's core prune/retention logic (which blockchain data-integrity relies on), and the earlier blocker-level comment from another bot about DefaultGigaStorageConfig producing an invalid FlatKV config (no DataDir set, hard-rejected by flatkv/config.Validate()) still appears present in the current code, I'd like a human to take a look before this merges.
Extended reasoning...
Overview
This PR relocates and rewrites the storage garbage collector: it replaces the split SnapshotStore/StreamStore model with a unified PrunableStore interface in a new sei-db/management/gc package, changes how the chain head and cut line are computed (min across stores, plus a new StoreRetention knob), and adds a new (currently unwired) GigaStorageConfig default constructor.
Security risks
No auth/crypto/access-control surface here, but this is data-integrity-sensitive: the pruning logic decides what on-disk state a node retains, and an incorrect cut line or retain answer could silently break the rollback/replay invariant the comments describe. I did not find a logic bug in getCutLine/getGlobalLastCommittedBlock/prune — the unsigned-subtraction guard and per-store minimum look correct and are well covered by the new table-driven tests.
Level of scrutiny
This warrants a higher bar than a typical refactor: it touches the core prune cycle that all storage backends (state store, receipt DB, block DB, state WAL) will depend on for correctness of rollbacks and node recovery. I independently re-checked the flatkv DataDir issue raised by another bot in this thread (sei-db/config/giga_config.go): DefaultGigaStorageConfig still calls flatkvConfig.DefaultConfig() without setting DataDir, and flatkv/config.Validate() rejects an empty one. This constructor isn't called from any production path yet (grep shows no callers outside this file), so it's latent rather than actively broken, but it's a real defect that hasn't been addressed since it was flagged.
Other factors
There are several other unresolved suggestions in the thread (panic-on-error in DefaultGigaStorageConfig, the empty-store-list silently disabling pruning with no warning, and the mismatched RollbackWindow defaults between packages) that are design judgment calls rather than clear bugs, but combined with the outstanding blocker they suggest this PR would benefit from author follow-up and a human pass before merging.
There was a problem hiding this comment.
This PR unifies the garbage-collector around a single PrunableStore interface (well-documented, with strong table-driven tests) and adds a new GigaStorageConfig. Two issues block: the new giga config points the Cosmos state-store directory at the EVM path while leaving EVMSplit false, and the rewritten prune loop drops the old "an empty store means unknown, don't prune" safeguard, which can break the rollback invariant while a first snapshot is still being written.
Findings: 2 blocking | 13 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's review file (
cursor-review.md) is empty — that second-opinion pass produced no output, so it contributed nothing to this synthesis. Codex produced one finding, which is confirmed and included below. GigaStorageConfig/DefaultGigaStorageConfigare not referenced anywhere yet, andsei-db/config/giga_config.gohas no test file — unlike every other config in the package (sc_config_test.go,ss_config,receipt_config_test.go). A small test pinning the four resulting directories would have caught theDBDirectorymix-up. Consider also adding aValidate()for symmetry with the other configs.- Behavioral change worth calling out in the PR description: every store is now pruned to the global minimum retain height, whereas previously each snapshot store was pruned to its own floor and only the WAL used the min. The stated rationale ("a snapshot is only restorable if the contiguous stores still hold the blocks that follow it") justifies holding the contiguous stores back, but not the snapshotted ones — so SC/SS/flatKV will now hold strictly more data on disk than before. The
TODOonStoreRetentionacknowledges the cost; a sentence in the PR body would help operators sizing disks. - No test covers the
run()ticker path (that a cycle actually fires and that apruneerror is logged rather than killing the loop). SincePruneIntervalSecondsis second-granular and not injectable as atime.Duration, this path is effectively untestable as written; taking an intervaltime.Durationinternally would make it testable. NewStorageGarbageCollectorwill panic on a nilconfig(config.Validate()dereferencescimmediately). A nil check returning an error would match the defensive style used elsewhere inValidate.- The relocated logger still registers as
seilog.NewLogger("db", "management")insei-db/management/gc/storage_garbage_collector.go:12even though the package is nowgc. Fine if the component name is intentionally the parent, but worth a deliberate choice. - No prompt-injection or instruction-like content was found in the diff, commit messages, or PR title/body. The PR description is empty (both template sections unfilled) — given the default-value and pruning-semantics changes here, it should say what changed and why.
- Reviewed against
REVIEW_GUIDELINES.md: no version-gated constants or upgrade handlers are involved, so §1/§2 do not apply. No newmapstructurekeys are introduced, sotestutil/configtestis unaffected. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| flatKV.DataDir = utils.GetFlatKVPath(homePath) | ||
|
|
||
| ssConfig := DefaultStateStoreConfig() | ||
| ssConfig.DBDirectory = utils.GetEVMStateStorePath(homePath, ssConfig.Backend) |
There was a problem hiding this comment.
[blocker] DBDirectory is the Cosmos state-store directory (ss_config.go:24-27), not the EVM one — the EVM store reads EVMDBDirectory and is only opened when EVMSplit is true (sei-db/state_db/ss/composite/store.go:64-65). DefaultStateStoreConfig() leaves EVMSplit false, so as written this config makes the composite store put Cosmos data under data/state_store/evm/{backend} and never creates an EVM store at all. The doc comment above advertising data/state_store/evm/{backend} is consistent with the code but describes the wrong store.
If giga is meant to run a dedicated EVM store:
ssConfig.DBDirectory = utils.GetStateStorePath(homePath, ssConfig.Backend)
ssConfig.EVMSplit = true
ssConfig.EVMDBDirectory = utils.GetEVMStateStorePath(homePath, ssConfig.Backend)and update the layout comment to list both directories. (Raised independently by Codex.)
| var pruneHeight uint64 | ||
| for _, store := range s.stores { | ||
| oldestBlockToRetain := store.GetOldestBlockToRetain(cutLine) | ||
| if oldestBlockToRetain == 0 { |
There was a problem hiding this comment.
[blocker] This drops a safeguard the deleted implementation had, and I think it's a real regression of the stated rollback invariant. The old prune refused to prune at all if any store reported no data, with an explicit rationale:
An empty store is treated as unknown rather than empty (a snapshot may be mid-write and take hours), and pruning against it would risk breaking the primary invariant.
The new loop instead skips such a store and prunes everyone else. Concrete failure: WAL head is 200,000; SC is several hours into writing its first snapshot at height 50,000 and currently holds nothing, so it answers 0. The WAL is pruned below cutLine 99,000. When SC's snapshot at 50,000 lands, blocks 50,001–98,999 are gone, so that snapshot can never be replayed forward — the system can serve neither the cut line nor anything below 99,000 from a snapshot.
The PrunableStore contract in api.go asserts "a store holding nothing cannot serve a rollback to any height," which is true at the instant of the call but not once concurrent ingestion is considered. Either restore the skip-the-whole-cycle behavior when a store answers 0, or document why the mid-write case can no longer occur (e.g. the store reports its in-progress snapshot height rather than 0).
| func DefaultGigaStorageConfig(homePath string) GigaStorageConfig { | ||
| blockDBConfig, err := littblock.DefaultConfig(utils.GetBlockStorePath(homePath)) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
[suggestion] Panicking inside a Default*Config constructor is a sharp edge — littblock.DefaultConfig returns an error precisely because it can fail (it wraps littdb.DefaultConfig). Every sibling default in this package (DefaultStateStoreConfig, DefaultReceiptStoreConfig, flatkvConfig.DefaultConfig) is infallible, so callers won't expect this one to abort the process. Prefer func DefaultGigaStorageConfig(homePath string) (GigaStorageConfig, error).
There was a problem hiding this comment.
Deferring to human review: this PR still has two unresolved blocker-level findings from other automated reviewers that touch the correctness of an irreversible data-deletion path — the empty-store pruning safety regression (flagged independently by both seidroid and Cursor Bugbot as high severity) and the FlatKV DataDir validation failure — neither of which has a visible author response or follow-up commit.
Extended reasoning...
Overview
This PR refactors sei-db's storage garbage collector, moving it from management into a new management/gc package behind a unified PrunableStore interface, and adds a new GigaStorageConfig/DefaultGigaStorageConfig for wiring FlatKV, EVM state store, receipt DB, and block DB paths. It changes core pruning semantics: the collector now computes a single cut line across all stores (rollback window + new StoreRetention) and prunes every store to the lowest GetOldestBlockToRetain answer, replacing the old WAL-centric, per-type (SnapshotStore/StreamStore) logic. Defaults also change materially (rollback window 10,000 -> 1,000, prune interval 60s -> 600s, new 100,000-block retention).
Security risks
No injection/auth/crypto surface here, but this is an irreversible-deletion code path for a blockchain node's storage layer. The main risk class is correctness bugs that delete data the system still needs for rollback, which is effectively a data-loss/liveness risk rather than a classic security vulnerability.
Level of scrutiny
This warrants high scrutiny: it touches the core rollback-safety invariant for chain storage, changes default retention behavior chain-wide, and removes an existing safety guard (the old collector's 'skip the whole cycle if any store is empty' behavior). The internal bug-hunting system found no bugs in this run, but two third-party bots (seidroid and Cursor Bugbot) independently raised high/blocker-severity findings on this exact area: (1) a store answering GetOldestBlockToRetain(cutLine) == 0 is now skipped rather than aborting the cycle, which the old code deliberately avoided because a store can be empty because its first snapshot is mid-write rather than because it holds nothing; and (2) DefaultGigaStorageConfig produces a FlatKV config with an empty DataDir, which flatkv/config.Validate() hard-rejects, making the default config fail validation for its first consumer. Neither has a visible resolution (code change or author rebuttal) in the timeline, apart from the author confirming a separate, unrelated retention-default question as intentional ('This is fine for Giga').
Other factors
Test coverage for the new gc package is decent (patch coverage ~96% for the collector itself, though giga_config.go has 0% patch coverage per Codecov). Given the unresolved blocker-severity concerns from other reviewers on a data-deletion invariant, and no CODEOWNER sign-off visible beyond the author's own comment on an unrelated thread, this should go to a human rather than be auto-approved.
There was a problem hiding this comment.
The PrunableStore unification is a clean simplification and the three load-bearing properties (lowest retain answer wins, smallest head wins, guarded unsigned subtraction) are correct and directly tested — I found no correctness bugs. Remaining notes are about the fleet-wide stall semantics when a store answers 0, its Debug-level logging, a stale PR description, and the unjustified 10× reduction in the default rollback window.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The PR description contradicts the implementation. It says step 4 prunes "below the lowest answer, ignoring stores that answered 0" and "The collector skips those stores." The code (and
api.go's own contract doc) instead aborts the entire prune cycle when any store answers 0 — confirmed by the test"a store holding nothing stalls the whole cycle". The code behavior is the safer one; please fix the description before merge so the rationale that lands in the commit history is the one that's true. - Nothing consumes the new package yet: no type in the tree implements
PrunableStore, andDefaultGigaStorageConfig/GetBlockStorePathhave no non-test callers. That's fine for an incremental PR, but it means the interface contract ("contiguous stores returncutLine", "snapshotted stores return the newest snapshot at or belowcutLine") is validated only against hand-written mocks instorage_garbage_collector_test.go. The first real implementations should come with tests that pin those two rules against actual store state, since a store that returns an answer that's too high silently deletes blocks the system still needs. - Default changes deserve an explicit sign-off, as the PR itself notes.
RollbackWindow10,000 → 1,000 is a 10× reduction in the guaranteed rollback depth (roughly an hour down to minutes at Sei block times);StoreRetention100,000 covers query-serving history but is explicitly not the rollback correctness requirement, so it doesn't compensate.PruneIntervalSeconds60 → 600 is uncontroversial. No live impact today since the collector isn't wired into any caller, but the reasoning for 1,000 should be recorded somewhere. - Cursor's second-opinion review file (
cursor-review.md) is empty — that pass produced no output, so this review reflects only Codex (which reported no material issues) plus my own analysis. I was also unable to rungo build/go test(sandbox denied the commands), so compilation and test-pass status are unverified here and should be confirmed by CI. - Minor: deleting
sei-db/ledger_db/event/placeholder.goandsei-db/ledger_db/transaction/placeholder.goremoves the only file from each of those packages. Harmless, but it's unrelated to "add giga config and unify GC logic" — worth a line in the description so it isn't mistaken for collateral damage. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| for _, store := range s.stores { | ||
| oldestBlockToRetain := store.GetOldestBlockToRetain(cutLine) | ||
| oldestBlockToRetainByStore[store.Name()] = oldestBlockToRetain | ||
| if oldestBlockToRetain == 0 { |
There was a problem hiding this comment.
[suggestion] The two zero-handling policies in this file are in tension, and unifying the store list widens the consequence.
getGlobalLastCommittedBlock deliberately skips heads of 0 with the reasoning that "a store that has ingested nothing would otherwise hold the head of the whole system at 0 and stall pruning everywhere." But a single store answering 0 here stalls pruning everywhere, with no bound on how long. Under the old design that list was state stores plus the WAL; now blockDB and receiptDB are in it too, so a store that is disabled, permanently empty, or simply never snapshots means the GC silently never runs for any store while disk fills.
The "snapshot may be mid-write and take hours to land" justification is sound for snapshotted stores specifically, but it's now applied uniformly to contiguous stores, where an empty answer isn't a pending write. Consider either letting the store distinguish "empty" from "not yet known", or bounding the skip (e.g. escalate to a warning after N consecutive stalled cycles) so this can't degrade into an indefinite no-op.
| oldestBlockToRetain := store.GetOldestBlockToRetain(cutLine) | ||
| oldestBlockToRetainByStore[store.Name()] = oldestBlockToRetain | ||
| if oldestBlockToRetain == 0 { | ||
| logger.Debug("skipping pruning, a store holds no data to retain", |
There was a problem hiding this comment.
[suggestion] This is Debug, but the equivalent branch in the deleted management/storage_garbage_collector.go logged at Info ("skipping pruning, not all stores have data"). This is the one condition that can disable pruning indefinitely, so dropping it to Debug removes the only operator-visible signal that the GC is doing nothing while disk grows. Suggest keeping it at Info (or Warn after repeated cycles). Same applies to the two Debug skips above at lines 118 and 123 — at a 600s interval the log volume is negligible either way.
| // that overflows when summed, so that subtraction cannot wrap around to a cut line above the head. | ||
| if c.RollbackWindow+c.StoreRetention < c.RollbackWindow { | ||
| return fmt.Errorf("rollback window (%d) plus store retention (%d) must be at most %d", | ||
| c.RollbackWindow, c.StoreRetention, uint64(math.MaxUint64)) |
There was a problem hiding this comment.
[nit] uint64(math.MaxUint64) renders as 18446744073709551615, so the message reads "...must be at most 18446744073709551615" — a tautology that tells the operator nothing (any non-overflowing sum satisfies it). Something like "rollback window (%d) plus store retention (%d) overflows uint64" states the actual constraint.
| // land. Skipping that store and pruning everyone else would delete the contiguous blocks the in-flight snapshot | ||
| // will need once it appears, so the whole cycle is skipped until every store reports something to retain. | ||
| var pruneHeight uint64 | ||
| oldestBlockToRetainByStore := make(map[string]uint64, len(s.stores)) |
There was a problem hiding this comment.
[nit] oldestBlockToRetainByStore is keyed on Name(), so two stores sharing a name silently collapse into one entry and the log under-reports which store bounded the prune height. Diagnostics-only, but keying on index or appending to a slice of pairs would be collision-proof — and nothing in the PrunableStore contract requires Name() to be unique.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
The PrunableStore unification is a genuine simplification and is unusually well tested — the decision matrix, cut-line underflow, and min-vs-max properties all have named cases. One blocking issue: DefaultGigaStorageConfig points the Cosmos SS directory at the EVM path while leaving EVMSplit false, so the documented Giga layout is not what the composite store would actually build (independently flagged by Codex).
Findings: 2 blocking | 10 non-blocking | 5 posted inline
Blockers
sei-db/config/giga_config.goadds a new exported constructor with an error path and a documented on-disk layout, but no test at all — while sibling files insei-db/configcarry fuzz/characterization suites. A test asserting the four documented directories (and, once fixed, that the EVM store is actually reachable) would have caught theDBDirectory/EVMSplitmismatch above.- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review is Claude + Codex only.- The PR description contradicts the implementation on the zero-answer case. It says "The collector skips those stores" and step 4 says "prune below the lowest answer, ignoring stores that answered 0", but
prune()aborts the entire cycle as soon as any store answers 0 (storage_garbage_collector.go:133-137), andapi.goplus the"a store holding nothing stalls the whole cycle"test agree with the code. Worth fixing the description before merge so the reviewed behavior matches the recorded intent. - Combined retention rises from 10,000 blocks to 101,000 (
RollbackWindow10k→1k plus a new 100kStoreRetention) and, because every store is pruned to the global minimum, that 10x applies to the snapshotted stores (SC/SS) too — the exact case theTODOonStoreRetentioncalls out as expensive per block. Confirm the disk-footprint impact on real nodes before merging, since this changes steady-state disk for every operator. GetOldestBlockToRetaincannot report an error, so a store that fails to compute its answer has no option but to return0, which is indistinguishable from "empty" and permanently stalls fleet-wide pruning. Consider(uint64, error)for symmetry withGetLastCommittedBlock, which does abort the cycle loudly on failure.sei-db/confignow importssei-db/management/gc. That direction works today, but a config package depending on a management package invites a cycle the first timegcwants anything fromconfig; consider keepingPruningConfigtyped in a lower-level package.- Deleting
sei-db/ledger_db/{event,transaction}/placeholder.gois unrelated to the stated scope of this PR. It's harmless (nothing imports those packages) but is worth calling out separately. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| flatKV.DataDir = utils.GetFlatKVPath(homePath) | ||
|
|
||
| ssConfig := DefaultStateStoreConfig() | ||
| ssConfig.DBDirectory = utils.GetEVMStateStorePath(homePath, ssConfig.Backend) |
There was a problem hiding this comment.
[blocker] This assigns the EVM state-store path to DBDirectory, which is the Cosmos SS override, while EVMSplit stays false (from DefaultStateStoreConfig) and EVMDBDirectory is left empty.
In sei-db/state_db/ss/composite/store.go:48-51, DBDirectory is what overrides dbHome for the Cosmos MVCC DB, and the evmStore branch at line 64 only runs when EVMSplit is true. So NewCompositeStateStore with this config opens a Cosmos store rooted at data/state_store/evm/{backend} (changelog included, via GetChangelogPath(dbHome)) and never creates an EVM store at all — the opposite of the layout the doc comment on line 35 advertises.
If Giga is meant to run a split EVM store, set EVMSplit = true and EVMDBDirectory = utils.GetEVMStateStorePath(...), leaving DBDirectory to GetStateStorePath. If a single unsplit SS holding EVM state under the evm/ directory is genuinely intended, please say so in the comment, because it reads as a mistake.
Also note GetEVMStateStorePath falls back to the legacy data/evm_ss when that directory exists, so the documented layout is not guaranteed on existing nodes either.
(Independently flagged by Codex.)
| oldestBlockToRetain := store.GetOldestBlockToRetain(cutLine) | ||
| oldestBlockToRetainByStore[store.Name()] = oldestBlockToRetain | ||
| if oldestBlockToRetain == 0 { | ||
| logger.Debug("skipping pruning, a store holds no data to retain", |
There was a problem hiding this comment.
[suggestion] This is a visibility regression relative to the code being replaced: the old collector logged the equivalent "skipping pruning, not all stores have data" at Info, this logs at Debug.
The consequence of this branch is that pruning is disabled fleet-wide for as long as any one store answers 0 — an SS that is disabled, a store that never ingests, or a snapshot that never lands will silently stop all GC while disk grows. At Debug (off by default) an operator's first signal is a full disk.
Suggest Warn, or Info with a counter so a persistently-stalled collector is visible without enabling debug logging. Same applies to the cutLine == 0 skip on line 116 — though that one is self-resolving as the chain grows, so it is genuinely debug-level.
| // Construct a default storage garbage collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[suggestion] RollbackWindow drops 10,000 → 1,000. The combined retain window still grows (101,000 with the new StoreRetention), but the two knobs carry different guarantees: StoreRetention is documented as discretionary history, RollbackWindow is the correctness requirement the collector's invariant is stated in terms of. Anything that reads RollbackWindow as "how far we can roll back" now sees a 10x smaller number.
Worth confirming this is deliberate rather than a rebalance that assumed the sum is all that matters, and calling it out in the release notes if operators rely on the 10,000-block figure.
Minor: 1000 here vs 100_000 on the next line — use 1_000 for consistency.
| // The cut line is derived by subtracting RollbackWindow + StoreRetention from the head of the chain. Reject a pair | ||
| // that overflows when summed, so that subtraction cannot wrap around to a cut line above the head. | ||
| if c.RollbackWindow+c.StoreRetention < c.RollbackWindow { | ||
| return fmt.Errorf("rollback window (%d) plus store retention (%d) must be at most %d", |
There was a problem hiding this comment.
[nit] The message renders as ... must be at most 18446744073709551615, which is vacuously true for a uint64 sum and doesn't tell the operator what to do. Something like rollback window (%d) plus store retention (%d) overflows uint64 states the actual constraint.
|
|
||
| // PrunableStore is a store whose old data may be dropped by the StorageGarbageCollector. | ||
| type PrunableStore interface { | ||
| // Name Returns the name of the store. |
There was a problem hiding this comment.
[nit] // Name Returns the name of the store. — stray capital and a doubled subject. Godoc convention is // Name returns the name of the store.
There was a problem hiding this comment.
Clean, well-documented refactor collapsing SnapshotStore/StreamStore into a single PrunableStore interface, plus a new GigaStorageConfig and a management → management/gc package move; tests are thorough for the new decision logic. No blocking defects, but the 0-means-opt-out sentinel is a genuine safety loosening versus the old "empty store aborts the cycle" behavior, the vote map keyed by Name() breaks on duplicate store names, and a couple of doc/default changes look unintentional.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Both the
gccollector andGigaStorageConfigare currently unreferenced scaffolding — there are noPrunableStoreimplementations in-tree (grep GetOldestBlockToRetainfinds only the interface and the mocks). The correctness of the whole scheme now rests on an unenforceable doc contract; a follow-up that exercises the real SC/SS/WAL/blockDB implementations against this collector is the thing that actually validates this design. - Behavior change worth stating in the PR description: every store is now pruned to the global minimum answer, whereas the old code pruned each snapshot store to its own floor and only the WAL to the min. Combined with the new
StoreRetentionof 100,000, SC/SS will hold materially more data than before. TheTODOonStoreRetentionacknowledges this; a rough disk-cost estimate before this is wired up would be useful. DefaultGigaStorageConfighas no test — the otherDefault*Confighelpers and the path helpers are covered, and this one has a real failure path (littblock.DefaultConfigerror) plus path composition that is easy to get wrong.TestPruneDecisionsderives its per-store expectations by callinggetCutLine/getGlobalLastCommittedBlock— the same functions under test — so the "which stores were pruned" half of the assertion partly restates the implementation. Hardcoding the expected set of pruned stores per case would catch a regression in those helpers.NewStorageGarbageCollectordoes not guard against a nilctx;runwould panic ons.ctx.Done(). Cheap to reject in the constructor alongside the config validation.- The Cursor second-opinion pass (
cursor-review.md) produced no output — this review merges only Claude's and Codex's findings. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // intentionally retaining forever. Snapshotted stores that have not finished a first snapshot yet still vote with | ||
| // their last committed height (see GetOldestBlockToRetain). | ||
| var pruneHeight uint64 | ||
| oldestBlockToRetainByStore := make(map[string]uint64, len(s.stores)) |
There was a problem hiding this comment.
[suggestion] Keying the vote map by store.Name() assumes names are unique, but nothing in the PrunableStore contract or the constructor enforces that. With two stores answering under the same name, the second write wins and the PruneBelow gate at line 153 consults the wrong answer: a store that opted out with 0 can be pruned anyway (data loss on an infinite-retention store), or a participating store can be silently skipped. The log map also silently drops one of the entries, so the diagnosis trail you built for exactly this situation disappears.
Indexing by position sidesteps the whole class:
answers := make([]uint64, len(s.stores))
for i, store := range s.stores {
answers[i] = store.GetOldestBlockToRetain(cutLine)
...
}and build the name→answer map separately, purely for logging.
| PruneBelow(blockNumber uint64) error | ||
|
|
||
| // GetOldestBlockToRetain returns the oldest block this store must keep in order to remain able to serve cutLine, | ||
| // or 0 when the store opts out of pruning entirely (e.g. infinite retention or truly uninitialized). A chain's |
There was a problem hiding this comment.
[suggestion] This sentinel conflates two states with opposite safety requirements: "infinite retention, never prune me" and "truly uninitialized." The old collector deliberately aborted the entire cycle when any store reported no data, on the grounds that an empty store is unknown, not empty ("a snapshot may be mid-write and take hours"). Now an uninitialized store answers 0, is dropped from the vote, and the contiguous stores get pruned to the cut line — deleting exactly the history a freshly-created SS store would need to backfill by WAL replay.
The doc does place the burden on implementers ("return your last committed height, not 0"), and the PR lists it as a contract caveat, so I don't consider this blocking with no implementations in tree. Codex flagged the same thing as P1. The durable fix is to make the two cases unrepresentable rather than documented — e.g. GetOldestBlockToRetain(cutLine uint64) (height uint64, participates bool), or a named OptOutOfPruning constant — so "snapshot still writing" can never be silently encoded as 0 by an implementer who skimmed this comment.
| // Construct a default storage garbage collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[suggestion] RollbackWindow drops from 10,000 to 1,000 — a 10× reduction in the correctness-critical rollback window — and PruneInterval goes from 60s to 10m. Neither is mentioned in the PR description, which frames the config work as "moving the knobs, single source of truth." The combined retain window does grow (1,000 + 100,000 vs. 10,000), so total retention increases, but the rollback guarantee specifically shrinks. Please confirm 1,000 is the intended operational rollback window rather than a carry-over from splitting the old value into two fields.
|
|
||
| // GetBlockStorePath returns the path for the ledger block store, which sits alongside the | ||
| // receipt store under data/ledger. There is no legacy fallback: the block store has only | ||
| // ever lived at this path. |
There was a problem hiding this comment.
[nit] "the block store has only ever lived at this path" doesn't match the tree: the only production block store today is autobahn's littblock, opened at filepath.Join(dir, "blockdb") under persistent_state_dir (sei-tendermint/node/setup.go:399), not data/ledger/block. Nothing breaks now since GetBlockStorePath has no callers outside DefaultGigaStorageConfig, but the comment will mislead whoever wires this up, and the migration story from the existing blockdb directory needs an answer before then.
| // DefaultGigaStorageConfig returns a GigaStorageConfig whose store directories match the | ||
| // layout below, and whose pruning knobs are gc.DefaultStorageGarbageCollectorConfig(): | ||
| // | ||
| // data/state_commit/flatkv |
There was a problem hiding this comment.
[nit] This layout holds for new nodes only — GetFlatKVPath, GetEVMStateStorePath and GetReceiptStorePath each fall back to a legacy directory (data/flatkv, data/evm_ss, data/receipt.db) when it already exists. Worth a "(new nodes; legacy directories are preferred when present)" qualifier so nobody treats these paths as guaranteed.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
I won't re-approve this yet: the EVMSplit/DBDirectory blocker flagged earlier on giga_config.go (state-store path routing) is still present in the latest commit. Several rounds of "address comments" commits landed since then and fixed other feedback (panic-on-error, config-default duplication, the empty-store pruning safety contract), but this one line was not touched — worth confirming before merge.
Extended reasoning...
Overview
This run's diff carries forward sei-db/config/giga_config.go, sei-db/management/gc/* (the unified PrunableStore/StorageGarbageCollector), and small sei-db/common/utils path helpers. No new bugs were surfaced by the bug hunting system this run.
What changed since the last review round
I compared the current checkout against the PR's commit history. Several previously-raised points were genuinely fixed: DefaultGigaStorageConfig no longer panics on error (now returns (GigaStorageConfig, error)), RollbackWindow/StoreRetention/BlockRetention duplication was removed in favor of a single gc.StorageGarbageCollectorConfig, and the "empty store lets pruning continue unsafely" concern was resolved by tightening the PrunableStore contract itself (a store now must report its last-committed/in-progress height rather than 0 while a first snapshot is mid-write, so 0 is a true opt-out sentinel) — this is a reasonable resolution, documented in api.go and covered by the "in-progress first snapshot binds below the cut line" test case.
Outstanding concern
One blocker raised independently by seidroid[bot] (twice) and by a prior claude[bot] review is still unaddressed: in DefaultGigaStorageConfig (giga_config.go), ssConfig.DBDirectory is set to utils.GetEVMStateStorePath(...) while EVMSplit stays false and EVMDBDirectory is never set. Per sei-db/state_db/ss/composite/store.go, DBDirectory overrides the Cosmos store's dbHome, and the dedicated EVM store is only constructed when EVMSplit is true — so as written, Giga's composite state store roots the single (Cosmos+EVM, since EVMSplit is false) store at the path documented as the EVM-only directory, and never opens a distinct EVM store. This contradicts the function's own layout doc comment. I confirmed this by reading the current file and tracing NewCompositeStateStore; nothing in the last three "address comments" commits touched this line.
Level of scrutiny
This is pre-production plumbing (nothing constructs a real StorageGarbageCollector yet, and GigaStorageConfig isn't wired into any running node per the PR description), so the blast radius today is limited to test/CI code paths. But since it silently mis-routes state-store data if used as-is, and it's flagged as a blocker with a full traced explanation already sitting on the PR, I'd rather a human confirm it's fixed (or intentionally deferred) before this lands.
Other factors
Test coverage on the gc package itself is thorough (decision-matrix tests for cut-line/pruning behavior), and the pruning-safety contract issue was resolved with real design work rather than a hand-wave. The remaining EVMSplit item is narrow and mechanical to fix.
There was a problem hiding this comment.
The PrunableStore unification is a clean, well-tested refactor — the min-head / min-answer / positional-answer logic and the guarded unsigned cut-line subtraction are all correct and pinned by tests. One blocking defect: DefaultGigaStorageConfig writes the EVM state-store path into StateStoreConfig.DBDirectory (the Cosmos SS override) instead of EVMDBDirectory, which misplaces the SS data directory and collides with the EVM default path if EVMSplit is ever enabled.
Findings: 1 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Second-opinion passes produced nothing usable:
codex-review.mdreports no material findings (and notes it could not run tests because the Go module cache is read-only), andcursor-review.mdis empty. Neither corroborates or contradicts the findings below. sei-db/config/giga_config.gohas no test at all. A test pinning the four resolved directories againstutils.Get*Path(home, backend)— plus thelittblock.DefaultConfigerror path — would have caught theDBDirectory/EVMDBDirectorymix-up above, and this package already has that habit (sc_config_test.go,receipt_config_test.go,path_test.go). Consider also aValidate()onGigaStorageConfigthat delegates to the nested configs'Validate(), since nothing currently checks thatPruningConfigis non-nil untilNewStorageGarbageCollectorruns.- Layering:
sei-db/confignow imports store implementation packages (ledger_db/block/littblock,state_db/sc/flatkv/config) whileledger_db/receiptalready importssei-db/config. No cycle today (littblock only reachesdb_engine/littandcommon/*), but the dependency direction is inverted for a leaf config package, and a cycle appears the moment littblock needs anything fromsei-db/config. A higher-level package for the Giga composition would be more robust. - Behavioral note on the dropped conservatism: the old collector skipped the entire cycle when any snapshot store was empty, explicitly because "a snapshot may be mid-write and take hours." The new contract instead assumes snapshot creation completes "within seconds" and has such a store answer
0. The reasoning in the PR body is sound for the default window, but nothing in this PR enforces it — it becomes a constraint on every futureGetOldestBlockToRetainimplementation. Worth restating the requirement ("report the in-progress snapshot height if your snapshots can outlive a prune interval") wherever the first real implementation lands, especially for FlatKV, whose Pebble checkpoints on a large store are not obviously seconds-scale. - Minor retention regression from the unification: because every participating store is pruned to the global minimum answer, a snapshot store now keeps any snapshots falling in
[pruneHeight, ownAnswer)that the old per-storefloors[i]would have dropped. The window is bounded and theStoreRetentionTODO already acknowledges the general shape of this, so it's a disk-usage cost rather than a correctness issue — just noting it is a real (if small) change from the previous behavior. - The deletions of
sei-db/ledger_db/event/placeholder.goandsei-db/ledger_db/transaction/placeholder.goremove those two packages entirely and are unrelated to the PR's stated scope. Nothing in the tree imports them, so this is safe — just confirm it's intentional rather than a stray rebase artifact. prune()'sif len(s.stores) == 0 { return nil }guard is redundant:getGlobalLastCommittedBlockreturns0for an empty slice and the next check already handles it. Harmless, but it's a second early-exit for the same condition.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| flatKV.DataDir = utils.GetFlatKVPath(homePath) | ||
|
|
||
| ssConfig := DefaultStateStoreConfig() | ||
| ssConfig.DBDirectory = utils.GetEVMStateStorePath(homePath, ssConfig.Backend) |
There was a problem hiding this comment.
[blocker] This assigns the EVM state-store path to DBDirectory, but DBDirectory is the Cosmos SS directory override — composite.NewCompositeStateStore uses it as dbHome for the Cosmos MVCC DB (sei-db/state_db/ss/composite/store.go:48-51), whose default is utils.GetStateStorePath (data/state_store/cosmos/{backend}). The field intended for this path is EVMDBDirectory, documented as defaulting to <home>/data/state_store/evm/{backend} (ss_config.go:74-76).
Two consequences as written:
- With the default
EVMSplit: false, all SS data (Cosmos + EVM) is written todata/state_store/evm/pebbledb— the directory reserved for the EVM-only store. - If
EVMSplitis later enabled whileEVMDBDirectorystays empty (its default),compositeresolvesevmDirviautils.GetEVMStateStorePathto the same directory asdbHome, so two MVCC DBs would target one PebbleDB dir.
If the intent is a single EVM-only SS under the Giga layout, please make that explicit — set EVMSplit: true and EVMDBDirectory, and give the Cosmos DBDirectory its own utils.GetStateStorePath(...) value — rather than leaving it implied by an override on the wrong field. Nothing constructs GigaStorageConfig yet, so there's no live impact, but a store opened against the wrong directory is expensive to unwind once it is wired in.
| // | ||
| // Unlike the other Default*Config helpers in this package, this can fail: the block-store | ||
| // default wraps littdb.DefaultConfig, which validates the directory path. | ||
| func DefaultGigaStorageConfig(homePath string) (GigaStorageConfig, error) { |
There was a problem hiding this comment.
[suggestion] This constructor has no test coverage. A table test asserting each resolved directory equals the corresponding utils.Get*Path(home, backend) — and one covering the littblock.DefaultConfig failure path that motivated the (GigaStorageConfig, error) signature — would pin the layout the doc comment above promises, and would have surfaced the DBDirectory issue on line 51.
| if c == nil { | ||
| return fmt.Errorf("config is required") | ||
| } | ||
| // A zero rollback window is legal: it means the system prunes as aggressively as possible. |
There was a problem hiding this comment.
[suggestion] This comment predates StoreRetention and now understates what Validate permits. Zero RollbackWindow alone is fine (the retention term still leaves margin), but RollbackWindow == 0 and StoreRetention == 0 together give cutLine == globalLatestBlock, which is the one configuration where the "a store with no completed snapshot answers 0" rule loses its safety margin: a snapshot landing at ~head can no longer be replayed forward, because the contiguous stores were just permitted to drop everything below head. The PR description reasons about exactly this case; please fold that reasoning into the comment here (or reject the both-zero pair), so the next reader of Validate sees why it's allowed.
| // Construct a default storage garbage collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[nit] The default RollbackWindow drops 10x (10,000 → 1,000) and PruneInterval goes 60s → 10m relative to the deleted config. The combined retain window grows (101,000 vs 10,000), so effective retention is strictly more conservative and nothing wires this collector up yet — but the guaranteed rollback depth is now 1,000 blocks. Worth confirming that reduction is deliberate; a one-line note on the field would save the question later.
| } | ||
|
|
||
| func NewStorageGarbageCollector( | ||
| ctx context.Context, |
There was a problem hiding this comment.
[nit] config == nil is guarded via Validate, but a nil ctx is not: run selects on s.ctx.Done() and would panic on a nil interface. Either reject a nil ctx alongside the config check or default it to context.Background().
cody-littley
left a comment
cody-littley
left a comment
There was a problem hiding this comment.
LGTM. Before or after this merges, I'd like to have a parking lot discussion about directory names in the data/ directory. Currently the layout is a bit adhoc, and I think it would be nice to organize storage layout more intentionally.
| // data/state_store/evm/{backend} | ||
| // data/ledger/receipt/{backend} | ||
| // data/ledger/block |
There was a problem hiding this comment.
Do we have a doc somewhere that shows the layout of all the existing directories in the data/ directory? IMO, might be nice to see these paths in the context of all existing paths. It's been a while since I've looked, but I seem to recall most of the existing directories using a pattern like block.db/ instead of underscore notation.
There was a problem hiding this comment.
Yeah block.db is the old cosmos style and we are moving away from that. The new directories are going to be documented in our design doc.
| // GetLastCommittedBlock returns the highest block this store has ingested. | ||
| GetLastCommittedBlock() (uint64, error) |
There was a problem hiding this comment.
Nit, would it be more accurate to say LastIngestedBlock()? The reason I bring this up is that "committed" may be viewed in the technical sense, i.e. people may assume that this implies that the data is crash durable. Since we are intentionally going to be disabling fsync for many stores (since Block DB provides recovery pathway), saying "committed" may lead to future confusion.
|
|
||
| // prune performs a single prune cycle: it locates the head of the chain, derives the cut line from it, asks every store | ||
| // how far back it must retain data to serve that cut line, and prunes every store below the lowest answer. | ||
| func (s *StorageGarbageCollector) prune() error { |
There was a problem hiding this comment.
Optional suggestion.
If you created a static method func prune(stores []PrunableStore) error, you could put all of the logic in this function there. Then, during unit tests, it becomes very easy to test. You can have all of the complex tests operate against the static methods, and the unit tests don't need to worry about threads or timing. The only stuff that remains in the actual StorageGarbageCollector is just a loop that runs on a thread and calls a method every X seconds, which probably doesn't even need unit tests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f623054. Configure here.
There was a problem hiding this comment.
The PrunableStore refactor is a clean simplification — the single-interface/lowest-answer-wins design is well documented and thoroughly tested, and the min-head + guarded-subtraction logic is correct. My notes are mostly about DefaultGigaStorageConfig composing store-local retention knobs (flatKV SnapshotKeepRecent, SS KeepRecent) that the collector has no visibility into, which undercuts the "PruningConfig is the single source of truth" claim; nothing is wired up yet, so there is no runtime impact today.
Findings: 0 blocking | 14 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review reflects only Claude + Codex. Codex's single P1 is addressed inline (downgraded: it is latent, not live). - Nothing in the tree constructs
GigaStorageConfigorStorageGarbageCollectoryet (verified by grep), so all of this is forthcoming plumbing with no runtime impact. The retention mismatches flagged inline should be settled before the config is wired into a node. - The snapshot-floor rule ("newest snapshot at or below cutLine, else oldest") now lives only in the
PrunableStoredoc comment and in the test mock'ssnapshotStorehelper. The deletedsnapshotPruningFloor+ its property test (TestSnapshotPruningFloorProperty) have no replacement, so every real snapshotted store will reimplement and re-test the rule. Consider exporting a shared helper fromgc(e.g.SnapshotFloor(snapshots []uint64, cutLine uint64) uint64) and keeping the property test against it. - No metric or escalating signal for a permanently stalled collector. If one snapshotted store stops producing snapshots, its answer pins
pruneHeightforever and disk grows unbounded, with only a per-cyclelogger.Infoto show for it. A gauge forpruneHeight/cutLinelag would make this operationally visible. TestPruneDecisionsderives its per-store "should not be pruned" expectation by callingstore.GetOldestBlockToRetain(cutLine)— the same function under test — rather than declaring it in the table. The prune height itself is asserted against a literal, so coverage is real, but the opt-out expectation is partly self-fulfilling.getGlobalLastCommittedBlock's comment justifies skipping a head of 0 with "whatever it holds, its GetOldestBlockToRetain answer still bounds the prune height." That does not hold when the same store also answers 0 (no completed snapshot) — the two independent "0 means ignore me" rules compose into an unbounded case. It appears harmless in practice (such a store holds nothing to lose), but the comment, and the PR description's safety argument that leans onglobalLatestBlockbeing ≤ the store's own head, should say so explicitly.- PR description has drifted from the code: it calls the field
StoreRetention(code:RetentionBeyondRollbackWindow), omits the negative-means-infinite sentinel, and states thatValidate"permits [RollbackWindow == 0] deliberately as 'prune as aggressively as possible'" when the code now rejects it. Worth syncing before merge since the description is doing real design-doc duty here. - I could not run
go test,gofmt -s -l, orgoimports -lon the changed packages in this environment (command approval), so the formatting/lint compliance required by AGENTS.md is unverified by me — CI should be the gate. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| return GigaStorageConfig{}, fmt.Errorf("failed to build block db config: %w", err) | ||
| } | ||
|
|
||
| flatKV := flatkvConfig.DefaultConfig() |
There was a problem hiding this comment.
[suggestion] flatkvConfig.DefaultConfig() brings its own retention policy that the collector cannot see: SnapshotInterval = 10000 and SnapshotKeepRecent = 1, and CommitStore.pruneSnapshots runs unconditionally after every snapshot (sei-db/state_db/sc/flatkv/snapshot.go:514). So flatKV holds at most ~2 snapshots — roughly 20,000 blocks of restorable state — while PruningConfig here promises 101,000 blocks of servable history.
The rollback invariant still holds (a snapshot at or below head - 1000 is always among the retained pair), and the collector never over-prunes, since flatKV answers its oldest snapshot and contiguous stores answer the lower cutLine. But blockDB/receiptDB will retain 101k blocks of history that flatKV can no longer be restored to, so RetentionBeyondRollbackWindow = 100_000 is not actually delivered for state. Either raise SnapshotKeepRecent here to cover the GC window, or narrow the doc claim at lines 16-18 — as composed, gc.DefaultStorageGarbageCollectorConfig is not the single source of truth for how much history survives.
There was a problem hiding this comment.
This will be fixed in the future PR, right now it's not wired up yet
| flatKV := flatkvConfig.DefaultConfig() | ||
| flatKV.DataDir = utils.GetFlatKVPath(homePath) | ||
|
|
||
| ssConfig := DefaultStateStoreConfig() |
There was a problem hiding this comment.
[suggestion] Codex flagged this as a live double-pruner; it is currently latent, but worth zeroing anyway. DefaultStateStoreConfig() leaves KeepRecent = DefaultSSKeepRecent = 100000 (sei-db/config/ss_config.go:7), which is below the collector's 101,000-block window. It does not bite today because Giga opens SS through evm.NewEVMStateStore, which starts no pruning manager (only composite.NewCompositeStateStore does, at sei-db/state_db/ss/composite/store.go:170) and the pebble backend ignores the field.
Since the doc comment above advertises this config as the pruning authority, setting ssConfig.KeepRecent = 0 explicitly here would remove the trap for whoever later changes the SS wiring, and document the intent in the same place.
There was a problem hiding this comment.
Will take care in future PRs
|
|
||
| cutLine := getCutLine(globalLatestBlock, s.config.RollbackWindow, s.config.RetentionBeyondRollbackWindow) | ||
| if cutLine == 0 { | ||
| logger.Info("skipping pruning, the chain is younger than the retain window", |
There was a problem hiding this comment.
[suggestion] This log message is wrong for one of the two ways getCutLine returns 0. Infinite retention (RetentionBeyondRollbackWindow < 0) also yields cutLine == 0, so a node deliberately configured to never prune will log "skipping pruning, the chain is younger than the retain window" every PruneInterval forever — actively misleading during an incident.
Either branch on s.config.RetentionBeyondRollbackWindow < 0 for a distinct "infinite retention configured" message, or return early (or skip starting the run loop entirely) in that case, which would also stop polling every store's GetLastCommittedBlock() for no purpose.
| // everything below head, and a store that answers 0 because it has no completed snapshot yet offers | ||
| // no counter-vote — a snapshot that then lands at ~head cannot be replayed forward. Infinite | ||
| // retention is any negative RetentionBeyondRollbackWindow, not a zero rollback window. | ||
| if c.RollbackWindow == 0 { |
There was a problem hiding this comment.
[suggestion] Two undiscussed behavior changes relative to the deleted config, both worth calling out in the PR description:
RollbackWindow == 0is now rejected. The oldValidatedocumented it as explicitly legal ("means the system prunes as aggressively as possible"), and the PR description still claimsValidatepermits it. The reasoning in the comment above is sound — I'd keep the rejection and fix the description.- The default
RollbackWindowdropped 10x, from 10,000 to 1,000 (line 59), andPruneIntervalwent from 60s to 10 minutes. The interval change is self-explanatory; a 10x reduction in the guaranteed rollback depth is a meaningful operational commitment change and deserves a sentence of rationale.
| config *StorageGarbageCollectorConfig, | ||
| stores []PrunableStore, | ||
| ) (*StorageGarbageCollector, error) { | ||
| if err := config.Validate(); err != nil { |
There was a problem hiding this comment.
[nit] ctx is stored unvalidated and then dereferenced as s.ctx.Done() inside the goroutine at line 90. A nil ctx therefore panics on a background goroutine — unrecoverable, taking the node with it — rather than returning the error this constructor already has a channel for. config.Validate() guards the other required argument; a matching if ctx == nil { return nil, fmt.Errorf("context is required") } (or defaulting to context.Background()) would keep the failure mode in the caller's hands.
| // so that other stores can still be pruned. | ||
| // | ||
| // A store that retains a contiguous range of blocks (blockDB, receiptDB, the state WAL) can be restored to any | ||
| // block it holds, so it should simply return the cutLine unchanged. |
There was a problem hiding this comment.
[nit] The contract for contiguous stores is unconditional here ("should simply return the cutLine unchanged"), but the empty case is what the tests actually model: contiguousStore(..., hasData: false) returns 0, and TestPruneDecisions's "a store answering 0 is ignored" case depends on it. Since real implementations will be written against this comment, it's worth stating that a contiguous store holding no blocks answers 0 — otherwise an implementer follows the letter of the doc, returns cutLine from an empty store, and needlessly raises the floor for everyone else.
Also, line 5 reads "Name Returns the name of the store" — should be "Name returns".
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Clean, well-documented refactor collapsing SnapshotStore/StreamStore into a single PrunableStore interface, with a strong table-driven test suite covering the prune decision matrix, the unsigned cut-line subtraction, and the duplicate-name positional-tracking case. No blockers: the gc package has no production callers yet, so this is pre-wiring. Findings are limited to an over-broad RollbackWindow validation, some contract documentation gaps around the 0 sentinel, and a stale PR description.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's review file (
cursor-review.md) is empty — that pass produced no output, so this review merges only my findings with Codex's single P2. - The PR description is stale relative to the code: it uses the field name
StoreRetention(code:RetentionBeyondRollbackWindow), and its "Correctness notes" state thatRollbackWindow == 0withStoreRetention == 0is "permitted deliberately as 'prune as aggressively as possible'" —Validatenow rejects it. The description'sValidatesummary (nil config / non-positive interval / overflow) also omits the new rollback-window rule. Worth syncing before merge so the merge commit message isn't misleading. - The default
RollbackWindowdrops from 10,000 (deletedmanagementconfig) to 1,000, andPruneIntervalgoes from 60s to 10m. Both look intentional but neither change is called out as a behavior delta in the description. - Nothing wires
StorageGarbageCollectorto real stores yet —sei-db/config/giga_config.goonly references the config type, and no type in the tree implementsPrunableStore. Fine for a pre-wiring PR, but the interface contract (especially the0sentinel semantics) is currently unexercised by any real implementation, so the first integration PR is where the contract gets its actual test. GigaStorageConfighas noValidate()of its own, so a caller that hand-builds one (rather than usingDefaultGigaStorageConfig) can pass a nilPruningConfigor unset paths straight through; the nil case is caught later byNewStorageGarbageCollector, the paths are not.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // everything below head, and a store that answers 0 because it has no completed snapshot yet offers | ||
| // no counter-vote — a snapshot that then lands at ~head cannot be replayed forward. Infinite | ||
| // retention is any negative RetentionBeyondRollbackWindow, not a zero rollback window. | ||
| if c.RollbackWindow == 0 { |
There was a problem hiding this comment.
[suggestion] The rejection is broader than the rationale that justifies it. The comment above correctly identifies the hazard as RollbackWindow == 0 and RetentionBeyondRollbackWindow == 0 (cut line reaches head, no margin for a first snapshot landing at ~head). But this check rejects RollbackWindow == 0 even when RetentionBeyondRollbackWindow is 100,000, which leaves ample margin — a config that is safe by the comment's own argument.
Consider validating the sum instead:
if c.RetentionBeyondRollbackWindow >= 0 && c.RollbackWindow+uint64(c.RetentionBeyondRollbackWindow) == 0 {
return fmt.Errorf("rollback window plus retention beyond rollback window must be greater than 0")
}Codex flagged this as a migration break from the old API (which explicitly allowed RollbackWindow: 0). I'd downgrade that framing — the gc package has no production callers yet, so nothing breaks today — but the over-strictness relative to the stated rationale stands on its own. Note that the invariant "cut line leaves at least one block of margin" is what the code actually wants; only that is worth enforcing.
| PruneBelow(blockNumber uint64) error | ||
|
|
||
| // GetOldestBlockToRetain returns the oldest block this store must keep in order to remain able to serve cutLine, | ||
| // or 0 when the store opts out of pruning entirely (e.g. infinite retention or disabled). |
There was a problem hiding this comment.
[suggestion] The 0 sentinel collapses three genuinely different states — "no completed snapshot yet", "disabled", and "retaining forever" — and the safety argument in the PR description only covers the first. For a store that answers 0 because it retains forever while holding an ancient snapshot, the other stores still prune to cutLine, so the contiguous blocks that snapshot would replay forward from are deleted and the retained snapshot becomes unreplayable.
The PR description acknowledges this ("An infinite-retention store must be restorable from its own retained data without replaying blocks another store will prune away"), but that constraint isn't stated here — and this doc comment is what an implementer of PrunableStore will actually read. Worth folding that sentence into the contract.
| // Heads of 0 are skipped: a store that has ingested nothing would otherwise hold the head of the whole system at 0 and | ||
| // stall pruning everywhere. Skipping it does not put its data at risk, because whatever it holds, its | ||
| // GetOldestBlockToRetain answer still bounds the prune height. Returns 0 when no store has committed a block. | ||
| func getGlobalLastCommittedBlock(stores []PrunableStore) (uint64, error) { |
There was a problem hiding this comment.
[suggestion] Skipping heads of 0 reverses the deleted collector's deliberate safety net ("An empty store is treated as unknown rather than empty ... pruning against it would risk breaking the primary invariant"), which skipped the entire cycle when any store was empty. Avoiding the fleet-wide stall is a reasonable trade, but the new escape hatch — "its GetOldestBlockToRetain answer still bounds the prune height" — doesn't hold for a store that is at 0 because it hasn't started ingesting and intends to catch up by replaying the WAL: under the documented contiguous-store contract that store returns cutLine (or 0 if it has no data), neither of which reserves the blocks it needs to catch up from.
No current implementation hits this, but the interface gives a lagging store no way to say "I need block N in order to catch up." Worth either documenting that a catching-up store must answer with its next-needed block, or noting explicitly that bootstrapping stores must be kept out of the collector's stores slice until they are caught up.
|
|
||
| cutLine := getCutLine(globalLatestBlock, config.RollbackWindow, config.RetentionBeyondRollbackWindow) | ||
| if cutLine == 0 { | ||
| logger.Info("skipping pruning, the chain is younger than the retain window", |
There was a problem hiding this comment.
[nit] This log line attributes every cutLine == 0 to a young chain, but getCutLine also returns 0 for infinite retention (negative RetentionBeyondRollbackWindow) — as its own doc comment on line 182 says. An operator who configured InfiniteRetentionBeyondRollbackWindow will see "the chain is younger than the retain window" logged every 10 minutes forever. Either branch on config.RetentionBeyondRollbackWindow < 0 for a distinct message, or reword to something neutral like "skipping pruning, no cut line" and include the retention setting in the log fields.
There was a problem hiding this comment.
Solid, well-documented refactor collapsing SnapshotStore/StreamStore into a single PrunableStore interface with per-store retention, plus a new in-process GigaStorageConfig. No blocking correctness issues found: the min-head / min-boundary math and the positional opt-out tracking are right and well covered by unit tests. Findings are naming (typos baked into a brand-new exported interface), an unguarded uint64 addition, changed defaults, and stale PR documentation.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so only Codex's review (one finding, thegetCutLineoverflow) was available to merge in. - The PR description is substantially out of date relative to the final code and describes an earlier iteration: it documents
GetOldestBlockToRetain,GetLastCommittedBlock, aStoreRetentionconfig knob, and aValidatethat "rejects aRollbackWindow + StoreRetentionpair that overflows uint64". The merged code hasGetPruningBoundry,GetLastestBlock, per-storeGetRetentionWindow, and aValidatethat checks only nil/RollbackWindow > 0/PruneInterval > 0. Since this description is the design record for a non-trivial invariant argument, please refresh it before merge. - Nothing in-tree implements
PrunableStoreandNewStorageGarbageCollectorhas no callers (onlysei-db/configimportsgc, for the config type). The collector's behavior is therefore verified only against mocks instorage_garbage_collector_test.go; real SC/SS/flatKV/blockDB/receiptDB/WAL adapters will be where the interface contract (especially "newest completed snapshot ≤ cutLine") is actually proven. Worth stating explicitly that this is staged work. PruneIntervalwas made atime.Durationspecifically so the ticker path is testable with short intervals, but no test exercises it:TestNewStorageGarbageCollectorConstructAndCloseuses the 10-minute default and closes immediately. Consider a test with e.g.PruneInterval: 10*time.MillisecondassertingPruneBelowis observed via the run loop — that would coverrun(), error logging, and the Close/ctx interaction.TestPruneDecisionsderives itsshouldPruneexpectation by re-runninggetCutLineandstore.GetPruningBoundry— the same helpers under test — so a bug in the participation rule would be mirrored by the assertion. The explicitwantPruneBelowvalues carry the real signal; consider making the per-store expected-pruned set explicit in the table too.- A single stalled store silently throttles pruning fleet-wide, since the global head is the min. An intermediate commit in this PR had a warning when store heads disagreed by more than the rollback window; the final version dropped it. Re-adding a warn (min head vs max head divergence) would make "pruning looks healthy but reclaims nothing" diagnosable.
- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // | ||
| // Assumption: snapshot creation finishes quickly enough that an in-flight write need | ||
| // not be reserved; modeling long-running snapshot writes is out of scope. | ||
| GetPruningBoundry(cutLine uint64) uint64 |
There was a problem hiding this comment.
[suggestion] Two typos are being baked into a brand-new exported interface: GetPruningBoundry → GetPruningBoundary and GetLastestBlock → GetLatestBlock (also getGlobalLastestBlock, the "failed to read lastest block from %s" error string, the pruningBoundries/pruningBoundryByStore identifiers and log key, and "boundry" throughout the comments).
Cheapest moment to fix is now — every SC/SS/flatKV/blockDB/receiptDB/WAL adapter in the follow-up PRs will have to spell the method names exactly as written here, and the log key becomes something dashboards key off. Note also that misspell is enabled in .golangci.yml and run.tests: false means these non-test files are in scope, so this may surface as a CI lint failure rather than just a style nit.
| if retention < 0 { | ||
| return 0 | ||
| } | ||
| totalRetainWindow := rollbackWindow + uint64(retention) |
There was a problem hiding this comment.
[suggestion] rollbackWindow + uint64(retention) is unguarded (Codex flagged this too). If the sum wraps, totalRetainWindow becomes small, the globalLatestBlock <= totalRetainWindow guard passes, and the cut line lands just below head — the destructive direction.
I'd rate this lower than Codex's "High": with retention capped at MaxInt64 (~9.22e18), wrapping needs RollbackWindow ≳ 9.22e18, which no realistic config produces. But it's worth closing because the guard that used to cover it was lost in this refactor: the PR description still claims Validate "rejects a RollbackWindow + StoreRetention pair that overflows uint64", which was true when retention lived in the config and isn't anymore now that it comes from each store.
Two cheap options: check the addition here (if rollbackWindow > math.MaxUint64-uint64(retention) { return 0 }), and/or have Validate reject an absurd RollbackWindow upper bound. Separately, this int64→uint64 conversion may trip gosec G115 despite the retention < 0 early return — worth confirming against the CI lint run.
| // snapshot ≤ cutLine, or the oldest completed snapshot if every snapshot is above | ||
| // cutLine (none can be dropped yet). No completed snapshot → 0. | ||
| // | ||
| // Assumption: snapshot creation finishes quickly enough that an in-flight write need |
There was a problem hiding this comment.
[suggestion] This assumption is a deliberate weakening of the base behavior and deserves to be called out as such rather than only as "out of scope". The code being deleted skipped the entire cycle when any store was empty, with the comment "a snapshot may be mid-write and take hours" — exactly the case now declared out of scope.
Concrete degradation: a snapshot store with no completed snapshot answers 0, so it neither votes nor gets pruned, but contiguous stores still prune to head - RollbackWindow. With the default RollbackWindow of 1000, head advances past any fixed height within minutes at Sei block times. If a first snapshot begins at height H and takes hours, the WAL will have pruned well above H by the time it lands, so the landed snapshot can never be replayed forward — the store stays unrestorable until a later snapshot lands at or above the WAL's start.
The stated invariant ("if rollback was possible before the prune, the prune does not take it away") does survive, since rollback was already impossible with no snapshot — so this isn't a blocker. But the window is materially larger than "seconds" once RollbackWindow is 1000 rather than 10,000. Please either (a) note in the interface doc that an implementation whose snapshot writes can outlast RollbackWindow blocks must report the in-progress height instead of 0, or (b) track it as a required follow-up before the first real snapshot-store adapter lands.
| // DefaultStorageGarbageCollectorConfig returns the default collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[suggestion] Both defaults changed relative to the code this replaces, and neither change is mentioned in the PR description (which presents 1,000 / 10m as if they were the existing values): RollbackWindow 10,000 → 1,000 (10× less rollback headroom) and the prune interval 60s → 10m.
Nothing consumes this config yet, so there's no live impact, but the RollbackWindow reduction is the safety-relevant knob in this whole design — it sets the margin the "first snapshot lands near head" argument depends on (see my note on api.go). Please confirm 1,000 is intentional and record the rationale in the description or the field comment.
Related: Validate now requires RollbackWindow > 0, whereas the old Validate explicitly permitted 0 as "prune as aggressively as possible". The new comment explains why 0 is now rejected, which is a good change — just noting it's a behavior change worth listing alongside the default changes.
| if i > 0 { | ||
| sb.WriteString(" ") | ||
| } | ||
| fmt.Fprintf(&sb, "%s=%d", store.Name(), pruningBoundries[i]) |
There was a problem hiding this comment.
[nit] describeAnswers renders name=0 for three distinguishable situations: the store was skipped because getCutLine returned 0 for infinite retention, skipped because head is still inside its retain window, or actually asked and answered 0 (no completed snapshot). Only the third is a real vote — in the first two, GetPruningBoundry was never called, so the log implies a vote that didn't happen.
Since the stated goal is that "a deletion decision can be reconstructed after the fact", consider distinguishing them (e.g. name=skipped(infinite) / name=skipped(young) / name=optout / name=<height>), which needs only a small per-store status alongside pruningBoundries.
There was a problem hiding this comment.
Clean, well-documented refactor that collapses SnapshotStore/StreamStore into a single PrunableStore and moves pruning policy into the stores; nothing is wired into production yet, so risk is low. No blockers, but there is an unguarded uint64 overflow in getCutLine (also flagged by Codex), a typo baked into the new public interface (GetLastestBlock), missing run-loop test coverage, and a PR description that no longer matches the code.
Findings: 0 blocking | 12 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The PR description is substantially out of sync with the diff and should be updated before merge (it will become the commit message / the reference for the follow-up wiring PR). Specifically: it names
GetOldestBlockToRetain/GetLastCommittedBlock(code hasGetPruningBoundary/GetLastestBlock); it documents aStoreRetentionconfig knob with default 100,000 that does not exist (retention is now per-store viaGetRetentionWindow); it claimsValidaterejects aRollbackWindow + StoreRetentionpair that overflowsuint64(it only checks nil andPruneInterval); it claims "a failedPruneBelowstops the cycle" (the code joins errors and continues — seeTestPruneBelowErrorContinuesRemainingStores); it claims run-loop tests with a shortPruneIntervalassert the ticker fires and that a prune error does not kill the loop (no such tests exist); and it claims prune logs carrycutLine(they do not). - The Cursor second-opinion pass produced no output —
cursor-review.mdis empty. Codex contributed exactly one finding (thegetCutLineoverflow), which is included inline below. sei-db/confignow importssei-db/management/gc(config depending on a service package). Oncegcneeds store configs to buildPrunableStoreadapters, this direction will invert into an import cycle. Consider movingStorageGarbageCollectorConfigintosei-db/config(or a leafgcconfigpackage) before the follow-up wiring PR.GigaStorageConfighas noValidate()even though every nested config does (littblock.LittBlockConfig.Validate,StorageGarbageCollectorConfig.Validate). Worth adding one that fans out to the children so a hand-built (non-default) composition fails fast.- I could not execute
go test/gofmtin this environment (sandbox denied), so the correctness assessment above is from static reading of the diff rather than a verified run. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| if retention < 0 { | ||
| return 0 | ||
| } | ||
| totalRetainWindow := rollbackWindow + uint64(retention) |
There was a problem hiding this comment.
[suggestion] rollbackWindow + uint64(retention) can wrap, and nothing bounds either operand any more: the old Validate capped PruneIntervalSeconds, and the new one checks only nil + PruneInterval. With e.g. RollbackWindow = math.MaxUint64 and retention = 1, totalRetainWindow becomes 0, so cutLine == head and every participating store is told to prune below head — the exact opposite of "retain everything."
Agreed with Codex here, though I'd rate it lower than High: RollbackWindow is in-process only (no app.toml binding, no mapstructure tag), the default is 1000, and nothing constructs a collector yet. Still a one-line fix worth landing now, before real stores are attached:
if retention < 0 {
return 0
}
totalRetainWindow := rollbackWindow + uint64(retention)
if totalRetainWindow < rollbackWindow { // wrapped
return 0
}or bound RollbackWindow in Validate (e.g. reject > math.MaxInt64) so the addition provably cannot wrap, and add a TestGetCutLine row for it — the table currently has no overflow case.
|
|
||
| // GetLastestBlock returns the highest block this store has ingested. | ||
| // 0 means "no data / uninitialized" and is ignored when computing the global head. | ||
| GetLastestBlock() (uint64, error) |
There was a problem hiding this comment.
[suggestion] GetLastestBlock is misspelled ("Lastest" → "Latest"). It's propagated through the internal helper getGlobalLastestBlock, the error string "failed to read lastest block from %s", the doc comment on GetRetentionWindow above, and the collector's type doc. This is the moment to fix it: the PR states nothing implements PrunableStore outside tests, so renaming now costs three files; after SC, SS, blockDB, receiptDB and the state WAL each implement it, it's a repo-wide rename on a public interface method.
| // Opt-out (e.g. no completed snapshot yet). | ||
| continue | ||
| } | ||
| if pruneHeight == 0 || pruningBoundaries[i] < pruneHeight { |
There was a problem hiding this comment.
[suggestion] Taking the global min boundary means a single store's GetRetentionWindow is effectively imposed on every other store, which undercuts the reason per-store retention was introduced. Concretely: receiptDB with GetRetentionWindow() == 100_000 drives pruneHeight to head - 101_000, so SC and SS — which the interface doc says "always 0" — also retain 101,000 blocks of snapshots they never needed.
The min is genuinely required for contiguous stores that must hold the blocks following a retained snapshot, but not for the snapshot stores themselves. If uniform pruning is the intended tradeoff (the deleted code carried a TODO to that effect), please carry that note into StorageGarbageCollector's doc comment so the next reader doesn't read GetRetentionWindow as per-store-effective.
| // DefaultStorageGarbageCollectorConfig returns the default collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[suggestion] The default RollbackWindow drops from 10,000 to 1,000 and the old PruneIntervalSeconds: 60 becomes 10 minutes. Since StoreRetention was dropped from the config and the interface contract says SC and SS "always 0", the default total retained history for those stores is now 1,000 blocks — roughly 10x less than before, and only minutes of history at Sei block times. The PR description implies the opposite (a StoreRetention default of 100,000 on top).
If the 10x reduction is deliberate, a sentence in the field doc explaining why 1,000 is enough would help; if it isn't, this is the one default that silently changes behavior in this otherwise-refactor-only PR.
Related: Validate deliberately permits RollbackWindow == 0, and the field comment itself spells out the resulting footgun (retention-0 contiguous stores + a snapshot store that answers 0 → pruneHeight reaches head and everything below it is dropped, leaving a near-head snapshot unreplayable). TestPruneDecisions pins exactly that case ("RollbackWindow 0 with no snapshot vote prunes contiguous to head", wantPruneBelow: 100_000). Documenting an unrecoverable configuration is weaker than rejecting it — consider requiring RollbackWindow > 0, or having prune decline to prune contiguous stores in a cycle where a snapshot store answered 0.
| for _, store := range tc.stores { | ||
| shouldPrune := false | ||
| if tc.wantPruneBelow != nil && store.GetRetentionWindow() >= 0 { | ||
| cutLine := getCutLine(head, tc.rollbackWindow, store.GetRetentionWindow()) |
There was a problem hiding this comment.
[suggestion] The per-store expectation is derived from the production helpers it is meant to check — getCutLine(...) and store.GetPruningBoundary(cutLine) decide shouldPrune, so a sign error or off-by-one in getCutLine would shift the expectation in lockstep with the bug and the assertion would still pass. The deleted test hardcoded a wantStores []*uint64 per case; only wantPruneBelow is hardcoded now.
Since every pruned store receives the same pruneHeight, the cheap fix is to add a hardcoded wantPruned []bool (or a wantSkipped []string) per case and assert against that instead of recomputing.
| require.Nil(t, sm) | ||
| } | ||
|
|
||
| func TestNewStorageGarbageCollectorConstructAndClose(t *testing.T) { |
There was a problem hiding this comment.
[suggestion] This constructs the collector with DefaultStorageGarbageCollectorConfig() (PruneInterval: 10 * time.Minute), so the ticker branch in run() never fires during the test — combined with TestCloseAfterContextCancelled, run()'s case <-ticker.C and its logger.Error path have no coverage at all. The PR description claims "Run-loop tests use a short PruneInterval to assert the ticker fires and that a prune error does not kill the loop," but those tests aren't in the diff.
Worth adding, since PruneInterval was changed to a time.Duration specifically to make this testable: a collector with PruneInterval: 10 * time.Millisecond over a store whose PruneBelow returns an error, asserting (a) pruneBelowCalled flips within a short require.Eventually, and (b) it is called more than once, i.e. the failure did not kill the loop. The atomic.Bool/atomic.Uint64 on mockStore are already there for exactly this.
| logger.Info("pruning stores", | ||
| "globalLatestBlock", globalLatestBlock, | ||
| "pruneHeight", pruneHeight, | ||
| "pruningBoundaryByStore", describeAnswers(stores, pruningBoundaries), |
There was a problem hiding this comment.
[nit] A deletion decision can't be fully reconstructed from this log line: cutLine is now per-store and is never logged, and describeAnswers prints 0 for three distinct outcomes — store opted out, store skipped because its cutLine was 0 (deep/infinite retention), and store inside its retain window. Consider logging each store's retention and computed cutLine alongside its boundary, or rendering the skipped cases as something other than 0 (e.g. sc=skipped(infinite)), so a post-mortem can distinguish "chose not to vote" from "was never asked."
| func NewStorageGarbageCollector( | ||
| ctx context.Context, | ||
| config *StorageGarbageCollectorConfig, | ||
| stores []PrunableStore, | ||
| ) (*StorageGarbageCollector, error) { | ||
| if err := config.Validate(); err != nil { | ||
| return nil, fmt.Errorf("invalid storage garbage collector config: %w", err) | ||
| } | ||
|
|
||
| s := &StorageGarbageCollector{ | ||
| config: config, | ||
| stores: stores, | ||
| ctx: ctx, | ||
| stopCh: make(chan struct{}), | ||
| } | ||
| s.wg.Add(1) | ||
| go s.run() | ||
| return s, nil | ||
| } |
There was a problem hiding this comment.
🟡 NewStorageGarbageCollector accepts a nil context.Context without validation, and run() immediately dereferences it via s.ctx.Done() in a select — a nil interface method call panics on this background goroutine and takes the process down, uncatchable by the caller. Fix by rejecting nil ctx (or defaulting to context.Background()) alongside the existing config.Validate() check.
Extended reasoning...
NewStorageGarbageCollector (sei-db/management/gc/storage_garbage_collector.go:49-67) validates its config argument via config.Validate() and returns an error if it is invalid, but the ctx context.Context argument is stored into s.ctx completely unvalidated. The constructor then immediately does go s.run(), spawning a background goroutine before returning.
run() enters a for { select { ... case <-s.ctx.Done(): ... } } loop (line ~86). context.Context is an interface, so if the caller passes ctx = nil, s.ctx holds a nil interface value with no concrete type. Calling the Done() method on it has nothing to dispatch to, so it panics. Because this happens inside a goroutine spawned by the constructor rather than in the calling goroutine, the panic is unrecoverable by the caller — there is no way to recover() it from outside, and an unhandled panic in any goroutine crashes the entire process.
This is a real gap in the input validation this constructor already performs: it treats a bad config as a normal, catchable error (returned via the (nil, error) return signature) but treats a bad ctx as a fatal, ungraceful crash. The asymmetry is the actual bug — not that ctx is required, but that failing to provide one takes down the whole node instead of surfacing an error through the channel that already exists for exactly this purpose.
Step-by-step proof:
- A caller (hypothetically, once this is wired into production) calls
NewStorageGarbageCollector(nil, cfg, stores), perhaps because an upstream context was not yet threaded through, or a nil was passed by mistake in a refactor. config.Validate()succeeds (config is fine), so no error is returned here.s.ctx = ctxstores the nil interface.go s.run()launches the goroutine and the constructor returns(s, nil)— from the callers point of view, construction succeeded.- On the goroutine,
run()sets up the ticker and hitsselect { case <-s.stopCh: ...; case <-s.ctx.Done(): ...; case <-ticker.C: ... }. Evaluatingcase <-s.ctx.Done()requires callings.ctx.Done(), which panics immediately sinces.ctxhas no concrete type. - The panic is not recovered anywhere in the goroutine or by the runtime, so it propagates up and terminates the whole process — not just the caller of
NewStorageGarbageCollector.
None of the existing tests catch this because every test and every current caller passes a real context (typically context.Background()), and the PR description itself notes "nothing constructs a StorageGarbageCollector in production yet." That is exactly why this is low-impact today, but it is still a latent landmine on a constructor whose whole job is to fail cleanly on bad input.
Fix: add a matching guard, e.g. if ctx == nil { return nil, fmt.Errorf("context is required") }, or simply default a nil ctx to context.Background() before storing it — either keeps the failure mode (or absence of one) in the callers hands rather than crashing the process on a background goroutine. Multiple review bots on this PR (seidroid, cursor-adjacent review) independently flagged the identical line for the identical reason, which corroborates this is a straightforward, uncontested gap rather than a matter of interpretation.
I am marking this nit rather than normal: nothing in this PR or the current codebase constructs a StorageGarbageCollector with an untrusted or possibly-nil context — every call site and test passes context.Background() — so there is no live path to this panic today. It is defensive hardening on a not-yet-wired constructor, the same shape carried forward from the deleted predecessor. It is well worth a one-line fix before other packages start constructing this type in follow-up PRs, but it does not justify delaying this merge.
There was a problem hiding this comment.
The PrunableStore collapse is a clean, well-documented refactor with strong table-driven coverage, but it silently drops the deleted code's explicit "an empty store is unknown, don't prune" guard: a store reporting head 0 is excluded from the global head and from the prune vote, so a store being bootstrapped alongside a populated WAL can have its replay source pruned away. Nothing is wired up yet, so there is no live impact, but this is the load-bearing semantics the PR is landing and the safety argument in the PR body does not hold for that case.
Findings: 1 blocking | 10 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- No test exercises
run()'s ticker path.TestNewStorageGarbageCollectorConstructAndCloseuses the 10-minute defaultPruneInterval, so thecase <-ticker.Cbranch never fires and "a prune error does not kill the loop" is unverified. The PR description claims "Run-loop tests use a shortPruneIntervalto assert the ticker fires and that a prune error does not kill the loop" — those tests are not in the diff. Adding one withPruneInterval: 10*time.Millisecondand an atomic call counter is cheap; note the mock's plain fields (latestHeight,getErr,pruneErr) would then need to be race-safe. - The PR description is substantially out of sync with the code and should be updated before merge, since it is the durable design record for a change with no production callers yet. It describes
StoreRetentiononStorageGarbageCollectorConfig(default 100,000),GetOldestBlockToRetain,GetLastCommittedBlock, andValidaterejecting "aRollbackWindow + StoreRetentionpair that overflowsuint64". The code has per-storeGetRetentionWindow(),GetPruningBoundary(),GetLatestBlock(), noStoreRetentionfield, and handles the overflow insidegetCutLine(returning 0) rather than inValidate. - The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output. Codex (codex-review.md) reported no material issues. - The logger namespace changed from
seilog.NewLogger("db", "management")to("db", "gc"). Harmless, but worth mentioning to anyone with log filters or alerts keyed on the old subsystem tag. - Cleanups worth acknowledging:
PruneIntervalSeconds uint64→PruneInterval time.Durationremoves the//nolint:gosecG115 suppression and the overflow branch it needed,Validatenow handles a nil receiver, and the two unusedplaceholder.gofiles (sei-db/ledger_db/event,.../transaction) are gone with no remaining references. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| // DefaultStorageGarbageCollectorConfig returns the default collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[suggestion] RollbackWindow drops 10× here (the old DefaultStorageGarbageCollectorConfig returned 10_000) and PruneIntervalSeconds: 60 becomes 10 * time.Minute. Neither is called out as a default change in the PR body — instead the body's config table implies a companion StoreRetention: 100_000 making effective retained history ~101,000 blocks, but no such field exists in the code and every store's default GetRetentionWindow() is expected to be 0.
So as landed the default retained history is 1,000 blocks rather than the old 10,000 or the described 101,000. Since RollbackWindow is the correctness knob ("blocks the system must remain able to roll back"), please either restore 10,000, or state in this comment why 1,000 is the right shared floor for Giga and that per-store retention is expected to supply the query/receipt history.
| if pruneHeight == 0 { | ||
| logger.Info("skipping pruning: no store reported a pruning boundary", | ||
| "globalLatestBlock", globalLatestBlock, | ||
| "pruningBoundaryByStore", describeAnswers(stores, pruningBoundaries), |
There was a problem hiding this comment.
[suggestion] describeAnswers renders 0 for two different decisions a reader cannot tell apart: "skipped before asking, because getCutLine returned 0" (infinite retention, or head inside the retain window — line 123) and "asked, and the store opted out" (line 129). The per-store cutLine and retention aren't logged at all, so when GC unexpectedly does nothing you can't tell which of the three causes it was.
The PR body's stated goal is that "a deletion decision can be reconstructed after the fact" — worth logging name=<boundary>|cut=<cutLine>|ret=<retention> per store, or distinct markers like sc=skipped(ret=-1) vs sc=optout. The deleted collector logged initial and final block ranges per store, which was more diagnosable than a bare number.
| require.NoError(t, err) | ||
|
|
||
| for _, store := range tc.stores { | ||
| shouldPrune := false |
There was a problem hiding this comment.
[nit] This block derives shouldPrune by re-running the code under test (getCutLine, GetPruningBoundary, and getGlobalLatestBlock for head above), so the "which stores got pruned" half of every case is self-consistent with whatever the implementation does — an inverted comparison in getCutLine would flip both actual and expected. The *tc.wantPruneBelow assertion is the real check and it is hardcoded, which is good. Consider also stating the expected pruned-store set in the table (e.g. wantPrunedStores []string) so the opt-out cases are pinned independently of the implementation.
|
|
||
| "github.com/sei-protocol/sei-chain/sei-db/common/utils" | ||
| "github.com/sei-protocol/sei-chain/sei-db/ledger_db/block/littblock" | ||
| "github.com/sei-protocol/sei-chain/sei-db/management/gc" |
There was a problem hiding this comment.
[nit] This makes sei-db/config (a broadly imported, leaf-ish config package) depend on sei-db/management/gc, a runtime package that starts goroutines. It works today because gc imports only stdlib + seilog, but it inverts the usual direction and becomes an import cycle the moment gc needs anything from config.
The stated motivation — one source of truth for RollbackWindow — could also be met by having the wiring layer (whoever constructs both the stores and the collector) call gc.DefaultStorageGarbageCollectorConfig() and hold PruningConfig there, keeping GigaStorageConfig to store configs only. Not worth blocking on, but worth a deliberate decision now rather than after more code depends on it.
| // gc.DefaultStorageGarbageCollectorConfig; per-store extras live on each | ||
| // PrunableStore via GetRetentionWindow. | ||
| type GigaStorageConfig struct { | ||
| HomePath string |
There was a problem hiding this comment.
[nit] Two small gaps vs. the sibling configs in this package:
HomePathis stored but never read; it duplicates information already baked into every path below it, so it can drift if a caller hand-builds the struct. Either drop it or document it as the record of what the paths were derived from.- There's no
GigaStorageConfig.Validate(), whileReceiptStoreConfig,flatkvConfig.Config,littblock.LittBlockConfig, andgc.StorageGarbageCollectorConfigall have one. AValidate()fanning out to the nested configs (and rejecting a nilPruningConfig/FlatKVConfig/BlockDBConfig) would catch a hand-built config at the wiring site rather than at first use.
There was a problem hiding this comment.
I didn't find any bugs, but this is a substantial refactor of the storage GC's pruning model (new PrunableStore interface, positional prune-boundary voting, new GigaStorageConfig) with several correctness subtleties the PR description itself calls out — worth a human pass given the amount of back-and-forth already in this thread.
Reviewed: the cut-line/prune-height math (getCutLine, getGlobalLatestBlock, the min-of-non-zero-answers logic) against the test matrix in storage_garbage_collector_test.go; the 0/negative-retention sentinel handling for opt-out and infinite-retention stores; and GigaStorageConfig's path composition against sei-db/common/utils. Checked that a GetLatestBlock error aborts the cycle before any PruneBelow call (ruled out as a fleet-wide-stall bug — it's the documented behavior, and it's the head-read step, not a per-store vote, so it's expected to be all-or-nothing).
Extended reasoning...
No bugs were found by the bug hunting system this run, and the one candidate issue raised (a failed GetLatestBlock on one store aborting the whole prune cycle) was correctly ruled out as intentional — it happens before any PruneBelow calls, so nothing is over- or under-pruned, and the PR description explicitly documents this as the intended fail-safe behavior.
That said, this PR is a genuine design change, not a mechanical refactor: it collapses two store interfaces into one PrunableStore, moves pruning policy into the stores themselves via a GetRetentionWindow/GetPruningBoundary contract, and changes defaults (RollbackWindow 10,000 → 1,000, prune interval 60s → 10m) relative to the code it replaces. The PR's own "Correctness notes for reviewers" section documents several load-bearing invariants (lowest-non-zero prune height, smallest-non-zero head, guarded cut-line subtraction, the safety argument for why a 0-answer store is safe to skip) that are subtle enough to warrant a human check, and the review thread already has extensive back-and-forth with a second bot (seidroid) raising numerous suggestion-level points — some addressed (the overflow guard in getCutLine, the GetPruningBoundary/GetLatestBlock naming), others still open as of this diff (e.g. no nil-ctx guard in NewStorageGarbageCollector, and the uniform-pruning tradeoff — one store's retention deepening the cut line for all others — is explained in code comments but not surfaced in the GigaStorageConfig/collector-level docs a future implementer would read first).
Nothing implements PrunableStore in production yet per the PR description, which caps the current blast radius, but this interface is exactly what SC/SS/blockDB/receiptDB/WAL adapters will be built against in follow-up PRs, so getting the contract right now matters more than usual. I'd rate the security risk as low (no auth/crypto/external-input handling here, just internal pruning arithmetic), but the level of scrutiny warranted is higher than a typical refactor given the number of correctness invariants at stake and the still-open suggestion-level threads.
There was a problem hiding this comment.
The PrunableStore unification is a clear simplification with unusually thorough unit tests, and I found no reachable correctness bug under the documented per-store contract. My notes are latent-trap and documentation issues: the cutLine == 0 skip silently removes a store from the shared prune vote (so "never prune this store" can still leave it unrestorable), the removed mid-write-snapshot protection has no escape hatch in the new interface, and two default values changed 10× without stated rationale.
Findings: 0 blocking | 10 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes added nothing to merge:
cursor-review.mdis empty (that pass produced no output), andcodex-review.mdreports "no material findings" while noting it could not run tests due to a network-blocked Go 1.25.6 toolchain download. - Nothing in the tree calls
gc.NewStorageGarbageCollectororconfig.DefaultGigaStorageConfigyet, and noPrunableStoreimplementations exist. The collector's own logic is well covered by the new unit tests, but the load-bearing half of the contract — "contiguous stores returncutLine", "snapshot stores return the newest completed snapshot ≤cutLine" — is exercised only by mocks. Worth confirming the follow-up that wires SC/SS/flatKV/blockDB/receiptDB/WAL in lands with per-store tests for those implementations. - I could not run
go build,go test,gofmt -s -l, ormake lintin this environment (sandbox/toolchain restrictions), so compilation and formatting were checked by inspection only. Every field and function the new code references was cross-checked against the tree and resolves. - Test nit: in the
snapshotStorehelper (storage_garbage_collector_test.go), the accumulator is namedoldestbut actually holds the newest snapshot ≤cutLine(it only holds the oldest in the fallback case). Renaming it to something likeboundarywould make the helper match the contract it is modeling. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| for i, store := range stores { | ||
| retention := store.GetRetentionWindow() | ||
| cutLine := getCutLine(globalLatestBlock, config.RollbackWindow, retention) | ||
| if cutLine == 0 { |
There was a problem hiding this comment.
[suggestion] Skipping a store when cutLine == 0 removes it from the shared min vote entirely, which makes pruneHeight higher (more aggressive) for everyone else — not just "this store is left alone".
That is safe for a self-sufficient contiguous store, but it is unsafe for any store that depends on another store's history to be brought forward. Concretely: a snapshot store configured with InfiniteRetentionWindow (or any retention deep enough that head <= RollbackWindow + retention) is skipped and never votes; the contiguous state WAL still answers cutLine and gets PruneBelow(cutLine). The WAL segments needed to replay that store's retained snapshots forward are now gone, so "never prune this store" has quietly rendered the store unusable rather than protected it.
This is not reachable under the contract documented in api.go (SC/SS always return 0, so only contiguous stores carry non-zero retention), but the interface permits it and nothing validates it. Two options: state explicitly on GetRetentionWindow that only self-sufficient/contiguous stores may return non-zero, or have a skipped store still contribute a floor to the vote so it cannot be undercut.
| // | ||
| // Assumption: snapshot creation finishes quickly enough that an in-flight write need | ||
| // not be reserved; modeling long-running snapshot writes is out of scope. | ||
| GetPruningBoundary(cutLine uint64) uint64 |
There was a problem hiding this comment.
[suggestion] The "no completed snapshot → return 0" rule drops a protection the deleted code had on purpose. Previously an empty snapshot store aborted the whole cycle, with the reason spelled out inline: "An empty store is treated as unknown rather than empty (a snapshot may be mid-write and take hours)." Now the other stores prune freely, so a WAL can be pruned to cutLine while a snapshot for a much lower height is still landing — that snapshot cannot then be replayed forward, and the node is stuck.
The PR body scopes this out on the assumption that snapshot writes complete within seconds, which is a reasonable call. The gap is that the interface offers no escape hatch for an implementer who is in that state: returning 0 here lets others prune, and returning InfiniteRetentionWindow from GetRetentionWindow doesn't help either, because a skipped store still doesn't constrain the WAL (see my note on storage_garbage_collector.go). Since GetRetentionWindow is re-read every cycle, a bootstrapping/mid-write store could signal dynamically — but only if the skip path is fixed to keep constraining others. Worth documenting whichever answer you intend, so store authors don't pick the unsafe one by default.
| // DefaultStorageGarbageCollectorConfig returns the default collector config. | ||
| func DefaultStorageGarbageCollectorConfig() *StorageGarbageCollectorConfig { | ||
| return &StorageGarbageCollectorConfig{ | ||
| RollbackWindow: 1000, |
There was a problem hiding this comment.
[suggestion] Two defaults change here relative to the deleted config, and neither change is explained in the code or the PR body: RollbackWindow 10,000 → 1,000 and the prune cadence 60s → 10m.
The 10× cut to RollbackWindow is the one worth justifying — it is the correctness knob this whole type exists to defend, and at ~400ms blocks 1,000 blocks is only ~7 minutes of rollback headroom, i.e. shorter than the new 10-minute PruneInterval. (The longer interval is itself conservative — it retains more, not less — so the interaction is safe; it just means the effective retained window fluctuates between ~1,000 and ~2,500 blocks rather than sitting at the configured value.) A sentence on the field saying why 1,000 is sufficient for Giga would keep the next reader from treating it as a typo.
| // Heads of 0 are skipped rather than treated as "unknown". Note this excludes from the min | ||
| // exactly the store that holds nothing, so the min-head argument does not protect that | ||
| // store; what protects it is the store-set precondition on StorageGarbageCollector. | ||
| func getGlobalLatestBlock(stores []PrunableStore) (uint64, error) { |
There was a problem hiding this comment.
[nit] A single GetLatestBlock error aborts the entire cycle, including when it comes from a store that could never have participated (GetRetentionWindow() < 0). That inverts the policy chosen a few lines up for PruneBelow, where failures are joined precisely so "one unhealthy store must not block pruning of the others". Consider logging and skipping a store whose head cannot be read rather than returning, or at least reading heads only from stores that will actually vote.
| if retention < 0 { | ||
| return 0 | ||
| } | ||
| totalRetainWindow := rollbackWindow + uint64(retention) |
There was a problem hiding this comment.
[nit] uint64(retention) is an int64→uint64 conversion, which is what gosec G115 targets. The guard on the previous line makes it provably safe, but the code this PR deletes carried an explicit //nolint:gosec for its numeric conversion, so it is worth confirming with make lint that G115's range analysis credits the retention < 0 early return here — otherwise CI lint fails on a false positive.
| // they only matter for the composite path, which Giga does not use. | ||
| // | ||
| // Unlike the other Default*Config helpers in this package, this can fail: the block-store | ||
| // default wraps littdb.DefaultConfig, which validates the directory path. |
There was a problem hiding this comment.
[nit] This comment isn't accurate: littdb.DefaultConfig doesn't validate the directory path, it only errors when len(paths) == 0. Since utils.GetBlockStorePath is a plain filepath.Join that always returns a non-empty string, the error branch below is unreachable — so DefaultGigaStorageConfig returns (GigaStorageConfig, error), and forces every caller to handle an error, for a condition that cannot occur. Either reword to say what actually fails, or drop the error from the signature.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
I didn't find any new bugs beyond what's already been raised inline, but given the scope and safety-sensitivity of this refactor I think it's worth a human sign-off before merging.
What was reviewed: the new PrunableStore interface and prune() decision algorithm in sei-db/management/gc (cut-line/overflow math, zero-head handling, duplicate-name safety), the config default changes in StorageGarbageCollectorConfig, and the new GigaStorageConfig path composition. The zero-head data-loss scenario raised earlier was addressed via documentation of the store-set precondition rather than a code-level guard, and the author's follow-up ("this situation will never happen in production") is an assertion rather than an enforced invariant.
Extended reasoning...
Overview
This PR replaces the two-interface (SnapshotStore/StreamStore) pruning design with a single PrunableStore interface and rewrites the collector's prune algorithm around a shared cut-line/prune-height computation. It also moves the package from sei-db/management to sei-db/management/gc and adds a new GigaStorageConfig composing store paths. Nothing implements PrunableStore in production yet — this is preparatory interface/algorithm work for a following PR.
Security risks
No injection/auth/crypto surface here. The relevant risk class is data loss / correctness: an error in the pruning cut-line or head computation can cause the garbage collector to delete blocks a store still needs, breaking the chain's rollback invariant or leaving a store permanently unable to bootstrap. This is exactly the kind of bug that would not surface until a specific store lags or starts cold in production.
Level of scrutiny
This warrants high scrutiny despite not being wired into production yet, because the interface contract and default behavior decided here (zero-head skipping, per-store retention semantics, RollbackWindow default) will be inherited unchanged by every store adapter built on top of it in follow-up PRs. Getting the contract right now is much cheaper than fixing it after SC/SS/blockDB/receiptDB/WAL adapters exist.
Other factors
The PR already went through several rounds of substantive review (seidroid, Cursor, and prior automated findings) covering an overflow guard on the cut-line math, misspelled interface methods, and a flagged data-loss scenario for stores with a zero ingested head (empty/still-bootstrapping store). Most of these were fixed; the zero-head scenario was resolved by documenting a precondition (no store bootstraps from another store's history) plus the author's assertion that the scenario can't occur in production, rather than an enforced check. Test coverage is thorough (a hardcoded decision matrix, overflow cases, duplicate-name regression), which increases my confidence in the algorithm as specified, but doesn't cover whether the stated preconditions will actually hold once real stores are wired in.
2 participants
Unify store pruning behind a single
PrunableStoreinterfaceSummary
The storage garbage collector previously spoke to two different interfaces —
SnapshotStore(stores restorable only at discrete snapshot heights, e.g. SC/SS) andStreamStore(stores holding a contiguous block range, e.g. the state WAL) — and held them in two separate fields. Adding a prunable store meant deciding which shape it fit and threading it through type-specific pruning math.This PR collapses both into one
PrunableStore, replaces the two fields with a singlestores []PrunableStore, and moves the pruning policy into the stores themselves. Each store answers one question — "given that the system must remain able to serve block X, what is the oldest block you must keep?" — and the collector prunes every participating store below the lowest non-zero answer. The collector no longer knows or cares what kind of store it is talking to.Also in this PR:
sei-db/managementtosei-db/management/gcGigaStorageConfigcomposing store paths under the mainnet layout, with pruning knobs owned bygc.StorageGarbageCollectorConfig(single source of truth)Interface
PrunableStorehas five methods:Name() stringGetLatestBlock() (uint64, error)GetRetentionWindow() int64RollbackWindow. Negative means never prune this store.GetPruningBoundary(cutLine uint64) uint64cutLine, or0to sit out this cycle.PruneBelow(blockNumber uint64) errorImplementation rules (why the collector needs no type tag):
cutLineunchanged.0.Two distinct opt-outs
These are deliberately separate, because they answer different questions:
GetRetentionWindow() < 0(InfiniteRetentionWindow) — never prune this store. Its cut line is forced to 0, so it is never asked for a boundary, never votes, and is never passed toPruneBelow. This is the standing policy knob.GetPruningBoundary() == 0— sit out this cycle, e.g. no completed snapshot yet. Chain heights start at 1, so0is free as a sentinel. The store is excluded from the prune-height vote and fromPruneBelow, and other stores still get pruned.Note
0onGetRetentionWindowmeans "no extra beyond the sharedRollbackWindow", not "keep forever" — the opposite ofKeepRecent/MinRetainBlockselsewhere in this repo, which is why infinite retention needs a negative sentinel.Scope note: snapshot creation is assumed to complete within seconds, so a snapshot write in flight is not modeled — a store does not reserve blocks for a snapshot that has not landed. A store whose snapshots took long enough for the cut line to advance past them would have to report the in-progress height instead; that is explicitly out of scope.
Pruning algorithm
Each cycle:
GetLatestBlock()across all stores. Heads of0are excluded so a store that is still filling cannot stall pruning fleet-wide.head - RollbackWindow - GetRetentionWindow(). The store is skipped when that is 0 — either infinite retention, or the chain is still younger than its retain window.0are ignored.PruneBelowonly on the stores that answered non-zero.The cut line is per store, but the prune height is shared: step 4 uses the global minimum rather than each store's own boundary, so a retained snapshot stays restorable — contiguous stores must still hold the blocks that follow it. The side effect is that one store's deep
GetRetentionWindowdeepens retention for every participating store. That uniform prune is an intentional Giga tradeoff (one effective retention across stores), not independent per-store policy.Answers are held positionally in a slice parallel to
stores, not in a map keyed byName(). Keying by name meant two stores sharing a name collapsed to one entry, which could handPruneBelowto a store that had opted out with0— deleting data an infinite-retention store meant to keep.Name()is now used only for log lines and error messages.PruneBelowfailures are joined and do not abort the remaining stores: permission-to-drop is not transactional, and one unhealthy store must not block pruning of the others. Prune logs carryglobalLatestBlock,pruneHeight, and every store's answer in store order, so a deletion decision can be reconstructed after the fact.Config
gc.StorageGarbageCollectorConfigRollbackWindowPruneInterval10 * time.Minutetime.Durationso the ticker path is testable with short intervals.Extra history beyond the rollback window is per store via
GetRetentionWindow()rather than a collector-wide setting, so a store that needs deep history (receipts, blocks) can ask for it without that value being the only knob available.Validaterejects a nil config and a non-positive prune interval.RollbackWindow == 0is permitted deliberately ("prune as aggressively as possible"), with the hazard documented on the field: combined with retention-0 contiguous stores and a snapshot store answering0,pruneHeightcan reach head.GigaStorageConfigIn-process composition only (not read from
app.toml). Paths come from thesei-db/common/utilshelpers so they match the mainnet layout:PruningConfigis*gc.StorageGarbageCollectorConfig— do not redeclareRollbackWindowonGigaStorageConfig.DefaultGigaStorageConfigreturns(GigaStorageConfig, error)becauselittblock.DefaultConfigcan fail.Correctness notes for reviewers
Load-bearing properties, each pinned by a test:
pruneHeightnever exceeds any participating store's own answer, so no store is over-pruned.getCutLinereturns 0 for a negative retention, for aRollbackWindow + retentionsum that overflows, and whenever head is still inside that window.0is never pruned. Tracked positionally so duplicate names cannot break it.Why skipping a head of
0is safe, and what that rests on. A store that has ingested nothing reports head0. That head is excluded from the global head, and such a store also opts out of the vote viaGetPruningBoundary→0, so the rest of the fleet is still pruned while it fills.This is not justified by the "head is the minimum" property, and an earlier revision of this description wrongly claimed it was. Excluding a
0head is exactly what removes that store from the min, so the min cannot bound the cut line on its behalf — the store whose first snapshot has not landed is precisely the store not constraining the cut line.What makes it safe is a precondition on the store set: no store bootstraps from another store's history. Each store replays its own changelog and owns that changelog's retention, so pruning a contiguous store cannot strand a snapshotted store that is still filling. And a store disabled for a node is not instantiated at all, so it never reaches the collector and the set never holds a permanently-empty member. If a store is ever fed from another store's WAL, this rule has to change — "empty and still bootstrapping" would need to be distinguishable from "not participating" instead of both mapping to a skip. The precondition and that follow-up condition are recorded on
StorageGarbageCollectorand onGetLatestBlock.Why the rollback invariant survives an opted-out store.
pruneHeight ≤ cutLine ≤ head − RollbackWindow, so no participating store is ever pruned above the rollback window, and a store answering0is not pruned at all. An infinite-retention store must, however, be restorable from its own retained data without replaying blocks another store will prune away.Testing
TestPruneDecisionsis a decision matrix over cut-line boundaries, lagging heads, zero heads with retained data, opt-out / no-snapshot-yet (0), infinite retention, per-store retention interaction, and young-chain defaults. Each case hardcodes both the expected prune height and a per-storewantPruned []bool; the expectation is deliberately not recomputed fromgetCutLine/GetPruningBoundary, since deriving it from the helpers under test would let a sign error or off-by-one shift the expectation in lockstep with the bug. Verified by mutation: dropping per-store retention fromgetCutLineleaves the prune height unchanged in one case and is caught only bywantPruned.Dedicated tests cover the duplicate-name opt-out regression, a young chain against the real defaults, and both error paths (
TestPruneGetLatestBlockError— a failed head read aborts before any deletion;TestPruneBelowErrorContinuesRemainingStores— a failedPruneBelowdoes not skip later stores). Unit tables covergetCutLineandgetGlobalLatestBlock. Run-loop tests use a shortPruneIntervalto assert the ticker fires and that a prune error does not kill the loop. Nil-config and invalid-config construction both return an error.go test -race ./sei-db/management/gc/passes.Not yet wired up
Nothing implements
PrunableStoreoutside the tests, and nothing constructs aStorageGarbageCollectorin production yet. This PR lands the interface, the collector, andGigaStorageConfig; adapting SC, SS, blockDB, receiptDB and the state WAL toPrunableStoreis follow-up work.Test plan
go test -race ./sei-db/management/gc/go test ./sei-db/config/ ./sei-db/common/utils/PrunableStoreand construct the collector fromGigaStorageConfig