Align FlatKV snapshot config with MemIAVL#3745
Conversation
PR SummaryMedium Risk Overview Snapshot retention defaults move to keep-recent = 1 for memIAVL and FlatKV (FlatKV was 2); memIAVL Config parsing in Removes the boot-time check that required SS when state-sync snapshot interval was set with SC enabled. FlatKV read-only views get an independent context so closing the parent store does not cancel in-flight reads. Reviewed by Cursor Bugbot for commit 5dd8701. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3745 +/- ##
==========================================
- Coverage 59.91% 59.50% -0.42%
==========================================
Files 2288 2241 -47
Lines 189782 186006 -3776
==========================================
- Hits 113706 110681 -3025
+ Misses 65942 65443 -499
+ Partials 10134 9882 -252
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cleanly stops exposing FlatKV knobs in app.toml, mirrors FlatKV's snapshot cadence onto memIAVL, and floors memIAVL snapshot-keep-recent to 1; well-tested. No blocking issues, but one code comment misdescribes where the keep-recent floor is enforced, plus a couple of minor divergences worth noting.
Findings: 0 blocking | 6 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) was empty — that pass produced no output.
- Backward-compat behavior change:
GetConfigno longer reads anystate-commit.flatkv.*keys (fsync, async-write-buffer, snapshot-interval, snapshot-keep-recent, enable-read-write-metrics). Operators who previously set these in app.toml will have them silently ignored (FlatKV now mirrors the sc-* snapshot cadence instead). Consider calling this out in release/upgrade notes. - Minor divergence between the two parse paths:
app/seidb.go:parseSCConfigsstill reads the now-unadvertised hidden keystate-commit.flatkv.enable-read-write-metrics(line 115), whileGetConfigno longer reads it and the toml template no longer renders it. Harmless (GetConfig's StateCommit isn't used to build the store) but the asymmetry is a small maintenance snag. - Nit: when
sc-keep-recentis absent from app.toml,parseSCConfigsyields 1 (0 floored up), not the newly documented default of 2. Freshly generated configs render 2 via the template, so this only affects hand-edited/older configs, but it's a slight mismatch with the 'Defaults to 2' comment in toml.go and config.go. - No prompt-injection or suspicious instructions were found in the PR diff, title, or description.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // in-code defaults, except the snapshot cadence which mirrors the memIAVL | ||
| // (SC) snapshot settings so both backends checkpoint/prune in lockstep. The | ||
| // keep-recent floor is applied later at store construction | ||
| // (composite.NewCompositeCommitStore), so this stays a faithful parse. |
There was a problem hiding this comment.
[suggestion] This comment is inaccurate: the keep-recent floor is NOT applied at composite.NewCompositeCommitStore. That constructor only calls cfg.Validate(), and StateCommitConfig.Validate() does not enforce MinSnapshotKeepRecent. The floor is actually applied only in app/seidb.go:parseSCConfigs via AlignFlatKVWithMemIAVL(). Today there's no runtime bug because GetConfig().StateCommit is never fed to a store (only parseSCConfigs' output is), but the comment points a future maintainer to the wrong enforcement point — if someone ever builds a store from a GetConfig-derived config (or relies on Validate), a configured 0 would slip through unfloored. Please either fix the comment to say the floor is applied in the app config layer (parseSCConfigs / AlignFlatKVWithMemIAVL), or actually enforce the minimum here / in Validate. (This is the substance of Codex's P1, downgraded to a suggestion since no store is currently built from this path.)
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR raises memIAVL's default snapshot-keep-recent (0→2) with a floor of 1, and makes FlatKV's snapshot cadence mirror memIAVL when CompositeSC is used; the change is well-tested and backward-compatible (hidden flatkv.* keys still parse). No blocking issues found — only a maintainability note about duplicated mirror logic and a couple of non-blocking observations.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex flagged that
GetConfigreturnssc-keep-recent = 0despite the new minimum of 1. This is intentional-by-design:GetConfigis a faithful parse of app.toml/flags and itsStateCommitis not wired to SC store construction (grep shows no.StateCommit.consumer in the sei-cosmos server package). The real store path in sei-chain (SetupSeiDB→parseSCConfigs) callsAlignFlatKVWithMemIAVL(), which normalizes 0→1. So this is a low-risk consistency nuance rather than a correctness bug — I'd keep the Codex note only as a documentation/hygiene item, not a must-fix. - Maintainability: the FlatKV-follows-memIAVL mirror policy now lives in two places —
AlignFlatKVWithMemIAVL()(sei-db) and the manual assignments inGetConfig(sei-cosmos, lines 466-467). They deliberately diverge (GetConfig does not floor keep-recent), which is easy to get out of sync if the policy changes later. Consider a shared helper or a comment cross-referencing the two so future edits stay consistent. - Behavior change: bumping
DefaultSnapshotKeepRecentfrom 0 to 2 means every node keeps two extra old memIAVL snapshots after upgrade even with no config change, increasing disk usage. This appears intentional (matches the PR's goal of retaining more history) — just confirm operators/release notes call this out. - Cursor's second-opinion review (
cursor-review.md) was empty — that pass produced no output. Codex produced a single note (addressed above). - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| // precedence over the memIAVL mirror). | ||
| flatKVConfig := config.DefaultStateCommitConfig().FlatKVConfig | ||
| flatKVConfig.SnapshotInterval = v.GetUint32("state-commit.sc-snapshot-interval") | ||
| flatKVConfig.SnapshotKeepRecent = v.GetUint32("state-commit.sc-keep-recent") |
There was a problem hiding this comment.
[nit] This inlines the FlatKV-follows-memIAVL mirror (also implemented in AlignFlatKVWithMemIAVL), but unlike that helper it does not floor sc-keep-recent to the new minimum of 1, so GetConfig still returns 0 here. That's acceptable since GetConfig().StateCommit isn't used to build the SC store (the store path goes through parseSCConfigs→AlignFlatKVWithMemIAVL, which normalizes), but the duplicated-yet-divergent mirror logic is a maintenance hazard. Consider deduplicating or adding a cross-reference comment so the two mirror sites don't drift.
There was a problem hiding this comment.
The PR aligns FlatKV's snapshot cadence with memIAVL and floors snapshot-keep-recent to 1, and is well-tested overall. However, AlignFlatKVWithMemIAVL mirrors the snapshot interval without normalizing it: memIAVL clamps a configured interval of 0 up to 10000 while FlatKV treats 0 as "disable snapshots", so sc-snapshot-interval = 0 silently breaks the intended lockstep and disables FlatKV checkpointing.
Findings: 1 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- REVIEW_GUIDELINES.md is empty (no base-branch guidelines available) and cursor-review.md is empty (the Cursor pass produced no output); proceeded using Codex's review plus manual analysis.
- Normalization/alignment only runs in the app-layer parseSCConfigs path (via AlignFlatKVWithMemIAVL), not in sei-cosmos GetConfig, which is asserted to return a raw un-floored keep-recent of 0. This is intentional per the new tests, but it means the two config-construction paths can report different snapshot settings; worth confirming GetConfig's StateCommit is never used to build the actual SC store so the divergence stays cosmetic.
- The test comment in TestGetConfigParsesRawSnapshotKeepRecent claims the raw 0 is "floored later at store construction", but NormalizeSnapshotKeepRecent is only invoked from AlignFlatKVWithMemIAVL — memIAVL DB construction (db.go:292) does not floor it. Consider correcting the comment to avoid implying a store-level floor that does not exist.
- Changing DefaultSnapshotKeepRecent from 0 to 2 increases default on-disk snapshot retention for all nodes; intended per the PR description, but worth calling out in release notes as an operational/disk-usage change.
| // backends checkpoint and prune in lockstep. | ||
| func (c *StateCommitConfig) AlignFlatKVWithMemIAVL() { | ||
| c.MemIAVLConfig.SnapshotKeepRecent = memiavl.NormalizeSnapshotKeepRecent(c.MemIAVLConfig.SnapshotKeepRecent) | ||
| c.FlatKVConfig.SnapshotInterval = c.MemIAVLConfig.SnapshotInterval |
There was a problem hiding this comment.
[blocker] The interval is mirrored raw, but it is not normalized the way memIAVL normalizes it at runtime. memIAVL's Options.FillDefaults() clamps SnapshotInterval <= 0 up to DefaultSnapshotInterval (10000), so memIAVL never actually disables snapshots. FlatKV, by contrast, treats SnapshotInterval == 0 as "disable auto-snapshots" (store_write.go:100). So with a configured sc-snapshot-interval = 0, memIAVL keeps checkpointing every 10000 blocks while FlatKV stops checkpointing entirely — its WAL grows unbounded and every restart replays full history. That silently breaks the "both backends checkpoint/prune in lockstep" invariant this function is meant to enforce, and it is also a behavior change from the base branch, where FlatKV's interval defaulted to 10000 regardless of sc-snapshot-interval. Normalize the interval before mirroring (e.g. bump a 0/<=0 memIAVL interval to DefaultSnapshotInterval, mirroring FillDefaults) so both backends see the same effective cadence. (Matches Codex P1.)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 36d1ecf. Configure here.
There was a problem hiding this comment.
The FlatKV-follows-memIAVL alignment and the keep-recent floor (default 2, min 1) are implemented cleanly and well-tested, applied on the real store-construction path (parseSCConfigs). However, the PR also silently deletes the unrelated validateConfigs startup guard, which is undocumented and drops a safety check that fails fast on a snapshot-enabled/SS-disabled misconfiguration.
Findings: 2 blocking | 4 non-blocking | 2 posted inline
Blockers
- app/seidb.go removes the entire
validateConfigsfunction and its call, which is unrelated to the PR's stated purpose (FlatKV/keep-recent alignment) and undocumented in the description. It previously panicked when state-sync snapshot export was enabled (snapshot-interval > 0) with SC enabled but SS disabled, catching a misconfiguration at startup. Removing it lets such a node boot and potentially produce broken/incomplete state-sync snapshots instead of failing fast. Please justify the removal (or confirm the invariant is enforced elsewhere) or restore it. - 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Config-parsing consistency: flooring of sc-keep-recent (0→1) is only applied via AlignFlatKVWithMemIAVL in app/seidb.go parseSCConfigs (the actual store-construction path); sei-cosmos GetConfig deliberately leaves the raw 0 unfloored. This is fine because GetConfig's StateCommit is not what builds the store, but the divergence between the two parse entry points is a latent trap if a future caller ever builds a store from GetConfig's config.
- Comment/doc accuracy: the test comment in config_test.go (TestGetConfigParsesRawSnapshotKeepRecent) and the surrounding rationale say the value is "only floored later at store construction," but flooring actually happens during app config parsing (parseSCConfigs → AlignFlatKVWithMemIAVL), not at memIAVL store open (NormalizeSnapshotKeepRecent is not called on the store-open path). Consider rewording to avoid implying store-level enforcement that does not exist.
- Second-opinion passes: Codex reported no material issues but could not run the focused tests (Go 1.25.6 download blocked by network), so its pass is not test-backed; the Cursor review file was empty (no output).
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| "separateDBs", ssConfig.SeparateEVMSubDBs, | ||
| ) | ||
| } | ||
| validateConfigs(appOpts) |
There was a problem hiding this comment.
[blocker] This removes the validateConfigs guard (and its function definition below) with no mention in the PR description. It panicked when state-sync snapshot export is enabled with SC on but SS off — a fail-fast check for a misconfiguration that would otherwise produce broken snapshots. This is unrelated to the FlatKV/keep-recent alignment goal; please justify the removal or restore it (or confirm the invariant is now enforced elsewhere).
There was a problem hiding this comment.
That's intentional
| DefaultSnapshotInterval = 10000 | ||
| // DefaultSnapshotKeepRecent is how many old snapshots (besides the latest) to | ||
| // keep by default. | ||
| DefaultSnapshotKeepRecent = 2 |
There was a problem hiding this comment.
[nit] Changing DefaultSnapshotKeepRecent from 0 to 2 (and flooring configured 0 up to 1) increases retained snapshots and thus on-disk footprint for every node using defaults. This is the intended behavior per the PR, but worth calling out in release notes/upgrade docs so operators expecting the old single-snapshot behavior aren't surprised by extra disk usage.
There was a problem hiding this comment.
The PR sets a minimum snapshot-keep-recent of 1 and mirrors FlatKV's snapshot cadence onto memIAVL's when CompositeSC is used, backed by thorough tests. The main concern is a real inconsistency between the two config paths: the store-construction path (parseSCConfigs) unconditionally mirrors memIAVL and never reads explicit state-commit.flatkv.* snapshot overrides, while GetConfig documents and tests that those overrides "take precedence" — but GetConfig's StateCommit is never used to build the store, so that documented precedence has no runtime effect.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Config-path divergence (also raised by Codex as P1):
GetConfigin sei-cosmos/server/config/config.go now parses explicitstate-commit.flatkv.snapshot-interval/snapshot-keep-recentoverrides and its comment/tests (TestGetConfigHonorsExplicitFlatKVOverrides) assert they win over the memIAVL mirror. But the SC store is built only from app/seidb.goparseSCConfigs, which never reads those flatkv keys and always callsAlignFlatKVWithMemIAVL()(unconditional overwrite).GetConfig'sStateCommitis used only for telemetry/grpc/validation in start.go, never for store construction. Net effect: an operator addingstate-commit.flatkv.snapshot-intervalsees it honored by config tooling but silently ignored by the actual store. Recommend making the two paths consistent (either honor the overrides in parseSCConfigs, or drop the override support + misleading precedence claim/test from GetConfig). Note this override support was already unused pre-PR, but the new comments/tests make the false expectation explicit. - Removing
validateConfigsdrops the guard that panicked whenstate-sync.snapshot-interval > 0but SS was disabled. The PR description says state-sync snapshot creation doesn't require SS; worth a maintainer confirmation that snapshot export truly functions with SS disabled, since this relaxes a previously-enforced invariant. - cursor-review.md was empty (Cursor produced no output). codex-review.md contained one P1 finding, folded in above.
- No prompt-injection or suspicious content found in the PR.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
Comments that couldn't be anchored to the diff
app/seidb.go:58-- [nit]FlagSnapshotIntervalwas only referenced by the now-removedvalidateConfigsand appears unused elsewhere. Consider removing the dead constant.
|
|
||
| // Now that the raw flags are parsed, floor memIAVL snapshot-keep-recent and | ||
| // mirror the (unexposed) FlatKV snapshot cadence onto the memIAVL settings. | ||
| scConfig.AlignFlatKVWithMemIAVL() |
There was a problem hiding this comment.
[suggestion] This is the actual store-construction path, and AlignFlatKVWithMemIAVL() unconditionally overwrites FlatKVConfig.SnapshotInterval/SnapshotKeepRecent from memIAVL. Unlike GetConfig (sei-cosmos/server/config/config.go), parseSCConfigs never reads state-commit.flatkv.snapshot-interval / snapshot-keep-recent, so an operator's explicit FlatKV snapshot override is silently ignored here even though GetConfig's comment/tests assert those overrides "take precedence." Since GetConfig's StateCommit is never used to build the store, that documented precedence has no runtime effect — the two paths should be reconciled to avoid a silent operator footgun.
|
|
||
| // FlatKV knobs are not rendered in the default app.toml template. By default | ||
| // FlatKV's snapshot cadence mirrors the memIAVL (SC) settings so both backends | ||
| // checkpoint/prune in lockstep, but explicit state-commit.flatkv.* keys are |
There was a problem hiding this comment.
[nit] This comment claims explicit state-commit.flatkv.* keys "take precedence over the memIAVL mirror," but the object this produces (cfg.StateCommit) is not what builds the SC store — app/seidb.go parseSCConfigs is, and it ignores these flatkv keys and always mirrors memIAVL. Consider clarifying that this precedence applies only to config tooling/validation, not the running store (or reconcile the two paths).
seidroid
Bot
dismissed stale reviews from themself
Superseded: latest AI review found no blocking issues.
| // (a configured 0, "keep only the current snapshot", becomes 1); and | ||
| // - FlatKV's snapshot cadence mirrors the (floored) memIAVL settings so both | ||
| // backends checkpoint and prune in lockstep. | ||
| func (c *StateCommitConfig) AlignFlatKVWithMemIAVL() { |
There was a problem hiding this comment.
sc-snapshot-interval = 0 will the lockstep invariant and silently disables FlatKV snapshots
parseSCConfigs mirrors the raw memIAVL interval onto FlatKV before store construction, but the two backends normalize 0 differently: memIAVL treats a configured 0 as "use the default 10000" (applied later, in DB.Load), while FlatKV treats 0 as "auto-snapshots disabled"
There was a problem hiding this comment.
What do you think should the correct behavior be when we set 0 for FlatKV? Do we allow snapshot to be disabled or shall we just override it to default and not allow 0
| // FlatKV knobs are not rendered in the default app.toml template. By default | ||
| // FlatKV's snapshot cadence mirrors the memIAVL (SC) settings so both backends | ||
| // checkpoint/prune in lockstep, but explicit state-commit.flatkv.* keys are | ||
| // still parsed and honored if an operator adds them by hand (they take |
There was a problem hiding this comment.
it looks the running node's store is built from app/seidb.go parseSCConfigs (via SetupSeiDB → rootmulti.NewStore), which never reads those keys
There was a problem hiding this comment.
Only snapshot is derived from MemIAVL, other FlatKV configs can still parsed from config.toml -> Flags -> app/seidb.go
| // still parsed and honored if an operator adds them by hand (they take | ||
| // precedence over the memIAVL mirror). | ||
| flatKVConfig := config.DefaultStateCommitConfig().FlatKVConfig | ||
| flatKVConfig.SnapshotInterval = v.GetUint32("state-commit.sc-snapshot-interval") |
There was a problem hiding this comment.
if the state-commit.sc-snapshot-interval and state-commit.sc-keep-recent keys absent, line 465 default config will be overwrite by the line 466-467 to be 0/0. before this pr, it will uuse the default value even absent
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
There was a problem hiding this comment.
Solid, well-tested change that aligns FlatKV snapshot cadence with memIAVL, floors keep-recent to 1, and fixes a latent bug where absent app.toml keys clobbered in-code defaults. No blocking correctness issues, but several comments/docs are now stale (they say "defaults to 2" while the real default is 1) and one comment references a symbol that does not exist.
Findings: 0 blocking | 7 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The alignment
alignFlatKVSnapshotWithMemIAVL(store.go:195) unconditionally overwritesFlatKVConfig.SnapshotInterval/SnapshotKeepRecentfrom memIAVL whenever memIAVL's value is non-zero (which is effectively always, given the non-zero defaults). This means an operator who hand-sets the hiddenstate-commit.flatkv.snapshot-interval/snapshot-keep-recentkeys will have those values silently discarded at store construction — even though the newTestGetConfigHonorsExplicitFlatKVOverridesasserts GetConfig honors them, giving a false impression that they take effect. This appears intended (FlatKV is meant to mirror memIAVL and the keys are no longer exposed), but worth documenting/reconciling so the honored-at-parse-but-overwritten-at-construction split isn't surprising. (Codex P1.) - Cursor's second-opinion review (
cursor-review.md) was empty — that pass produced no output. - REVIEW_GUIDELINES.md is empty, so no repo-specific standards were applied beyond AGENTS.md.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| # KeepRecent defines how many state-commit snapshots (besides the latest one) to keep | ||
| # defaults to 0 to only keep one current snapshot | ||
| # KeepRecent defines how many state-commit snapshots (besides the latest one) to keep. | ||
| # Defaults to 2. The minimum is 1: a configured value of 0 is overridden to 1. |
There was a problem hiding this comment.
[suggestion] Stale default in this comment: the PR changes memIAVL DefaultSnapshotKeepRecent to 1, so this should read "Defaults to 1" (not 2). The following sentence ("minimum is 1") is correct. (Codex P3.)
| // SnapshotKeepRecent defines what many old snapshots (excluding the latest one) to keep | ||
| // defaults to 0 to only keep one current snapshot | ||
| // SnapshotKeepRecent defines how many old snapshots (excluding the latest one) to keep. | ||
| // Defaults to 2; a configured value of 0 is overridden to the default by FillDefaults. |
There was a problem hiding this comment.
[suggestion] This comment says "Defaults to 2" but DefaultSnapshotKeepRecent is now 1 (line 8). Update to "Defaults to 1" for consistency with the actual constant. (Codex P3.)
| // preserve the in-code default from DefaultStateCommitConfig above rather | ||
| // than reading back the zero value (cast.To*(nil) == 0/false) and clobbering | ||
| // it. This matters for keys whose default is non-zero (async-commit-buffer | ||
| // 100, snapshot-interval 10000, keep-recent 2, ...) so a config that omits a |
There was a problem hiding this comment.
[nit] The example list here says "keep-recent 2", but the memIAVL default is now 1. Minor, but it's another stale reference to the old default.
| // is a faithful parse of app.toml/flags: it only reads the explicit | ||
| // state-commit.flatkv.* keys (if an operator adds them by hand) on top of the | ||
| // in-code defaults. The FlatKV-follows-memIAVL mirror (and snapshot cadence | ||
| // normalization) is applied later by StateCommitConfig.AlignFlatKVWithMemIAVL |
There was a problem hiding this comment.
[nit] This comment references StateCommitConfig.AlignFlatKVWithMemIAVL, but no such method exists — the alignment is done by the free function composite.alignFlatKVSnapshotWithMemIAVL. Consider correcting the name so the pointer doesn't send readers looking for a method that isn't there.
There was a problem hiding this comment.
Sound, well-tested config change: memIAVL default keep-recent moves 0→1, FlatKV snapshot cadence is aligned to memIAVL and its knobs hidden from app.toml, and the SS-required-for-snapshot validation is dropped. No blockers; a few non-blocking edge cases and doc/comment inconsistencies remain.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Doc contradiction (Codex P3): sei-db/config/toml.go:107 still says "exporting state snapshot requires setting this [ss-enable] to true", which contradicts this PR removing the SC+snapshot-interval => SS-must-be-enabled validation. This line is not in the diff, but the guidance should be updated to reflect that snapshot export no longer requires ss-enable = true.
- cursor-review.md was empty (0 findings from the Cursor pass); REVIEW_GUIDELINES.md was also empty so no repo-specific standards were applied. Codex's two findings (P2/P3) are incorporated above.
- Operational note (not a bug, appears intended per the
non-app-hash-breakinglabel): raising memIAVL DefaultSnapshotKeepRecent 0→1 means every existing node now retains the latest snapshot plus one older snapshot instead of just the latest, increasing on-disk snapshot footprint after upgrade. Worth calling out in release notes. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // "disabled" for FlatKV) while memIAVL kept checkpointing, letting FlatKV's WAL | ||
| // grow unbounded. | ||
| func alignFlatKVSnapshotWithMemIAVL(cfg *config.StateCommitConfig) { | ||
| if cfg.MemIAVLConfig.SnapshotInterval != 0 { |
There was a problem hiding this comment.
[suggestion] Edge case (matches Codex P2): the align only copies when memIAVL is non-zero, but memIAVL zeros aren't resolved until Options.FillDefaults runs later inside memiavl.NewCommitStore. If an operator explicitly sets sc-snapshot-interval = 0 / sc-keep-recent = 0 and carries a legacy hand-set state-commit.flatkv.snapshot-interval / snapshot-keep-recent (still parsed via mapstructure even though hidden from the template), FlatKV keeps its custom value while memIAVL heals to the default — leaving the two backends divergent, the opposite of this function's goal. Consider resolving the memIAVL zeros to their defaults first (interval 0 -> DefaultSnapshotInterval, keep-recent 0 -> DefaultSnapshotKeepRecent) and then always copying, so the two are guaranteed to converge regardless of FlatKV's prior value. The common default/override paths are fine; this only bites explicitly-zeroed-plus-legacy configs.
| // is a faithful parse of app.toml/flags: it only reads the explicit | ||
| // state-commit.flatkv.* keys (if an operator adds them by hand) on top of the | ||
| // in-code defaults. The FlatKV-follows-memIAVL mirror (and snapshot cadence | ||
| // normalization) is applied later by StateCommitConfig.AlignFlatKVWithMemIAVL |
There was a problem hiding this comment.
[nit] This comment references StateCommitConfig.AlignFlatKVWithMemIAVL, but no such method exists — the alignment is done by the unexported composite.alignFlatKVSnapshotWithMemIAVL (store.go). Update the name to avoid pointing readers at a symbol that isn't there.
There was a problem hiding this comment.
Aligns FlatKV snapshot cadence with memIAVL, raises the minimum snapshot-keep-recent to 1, and drops the state-sync/SS validation. The change is internally consistent, backward-compatible with existing app.toml files, and well-covered by tests; no blocking issues found.
Findings: 0 blocking | 4 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Codex's only finding (P2, composite/store.go: legacy
sc-keep-recent = 0leaves FlatKV misaligned at 2) is addressed by this same PR: FlatKVDefaultSnapshotKeepRecentis lowered from 2 to 1, so legacy 0 heals to 1 on memIAVL (FillDefaults) and FlatKV keeps its new default 1 — both converge. No action needed. - Cursor produced no output (cursor-review.md is empty), and REVIEW_GUIDELINES.md is empty, so no repo-specific standards were applied beyond AGENTS.md.
- In FlatKVOnly write mode, FlatKV's snapshot interval/keep-recent are still derived from the memIAVL config (which isn't otherwise used to construct a store). This is documented and harmless since the sc-* defaults match FlatKV's, but it's slightly surprising that FlatKV cadence is only controllable via the sc-* (memIAVL) keys now that the flatkv.* keys are hidden from the template.
- Removing
validateConfigsdrops the panic that required SS to be enabled when snapshot interval > 0 and SC enabled. The PR description justifies this (state-sync snapshot creation doesn't require SS), but there is no test asserting the old panic no longer fires / that this combination now boots cleanly.
| // is a faithful parse of app.toml/flags: it only reads the explicit | ||
| // state-commit.flatkv.* keys (if an operator adds them by hand) on top of the | ||
| // in-code defaults. The FlatKV-follows-memIAVL mirror (and snapshot cadence | ||
| // normalization) is applied later by StateCommitConfig.AlignFlatKVWithMemIAVL |
There was a problem hiding this comment.
There is no StateCommitConfig.AlignFlatKVWithMemIAVL method. The real function is the unexported package-level composite.alignFlatKVSnapshotWithMemIAVL. Same wrong name appears in config_test.go (lines ~484, ~513). Minor, but it will mislead the next reader searching for it.
| } | ||
|
|
||
| // GetConfig returns a fully parsed Config object. | ||
| func GetConfig(v *viper.Viper) (Config, error) { |
There was a problem hiding this comment.
new GetConfig tests only cover the FlatKV branch. suggest to add testings asserting memIAVL keeps its in-code defaults when the sc-* keys are absent
There was a problem hiding this comment.
A well-tested config refactor that aligns FlatKV snapshot cadence with MemIAVL, defaults keep-recent to 1 (0 is now healed to the default), guards config reads with presence checks so absent keys keep in-code defaults, drops the state-sync/SS coupling check, and fixes a read-only FlatKV clone lifecycle bug. No blockers; one alignment edge case for upgrading nodes is worth considering.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion pass produced no output (cursor-review.md is empty). Codex's review was consumed and its one finding is folded into the inline comment on composite/store.go.
- Behavioral change: memiavl DefaultSnapshotKeepRecent moves 0->1 and FillDefaults now heals a configured 0 to 1, so operators can no longer pin keep-recent to 0 (single-snapshot). This is intended and documented in the template/comments, but it increases on-disk snapshot retention fleet-wide; confirm the added disk cost is acceptable on all node profiles.
- Minor consistency note: GetConfig now seeds MemIAVLConfig from DefaultStateCommitConfig(), so SnapshotWriteRateMBps picks up its non-zero default (100) whereas the previous struct literal left it 0/unlimited. This is almost certainly the intended effect of starting from defaults and matches app/seidb.go's parseSCConfigs, just flagging the subtle change in the GetConfig path.
- Nice test coverage overall: the new tests exercise absent-key defaults, raw-0 parsing, the FlatKV<-memIAVL alignment, template omission of flatkv keys, and the state-sync-without-SS boot path; the intent is clearly documented in comments.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if cfg.MemIAVLConfig.SnapshotInterval != 0 { | ||
| cfg.FlatKVConfig.SnapshotInterval = cfg.MemIAVLConfig.SnapshotInterval | ||
| } | ||
| if cfg.MemIAVLConfig.SnapshotKeepRecent != 0 { |
There was a problem hiding this comment.
[suggestion] The != 0 guard skips mirroring when memIAVL's value is 0, relying on FlatKV's in-code default matching memIAVL's healed default. That holds for a fresh config, but an upgrading node whose old app.toml still carries sc-keep-recent = 0 (the previous memIAVL default) together with an explicit state-commit.flatkv.snapshot-keep-recent = 2 (the previous FlatKV default, which the old template rendered) ends up misaligned: GetConfig reads FlatKV=2, this function skips (memIAVL==0), then memiavl FillDefaults heals 0->1, leaving memIAVL=1 vs FlatKV=2. Consider resolving memIAVL zeros to their effective defaults (DefaultSnapshotKeepRecent / DefaultSnapshotInterval) before mirroring, then assigning FlatKV unconditionally, so alignment holds even for old on-disk configs. Practical impact is limited because FlatKV retaining more than memIAVL is the safe direction for rollback (FlatKV then has a superset of snapshots), so this is a suggestion rather than a blocker — I agree with Codex's mechanism here but not its High severity.
| func TestParseSCConfigs_SnapshotKeepRecentParsedRaw(t *testing.T) { | ||
| // An explicitly configured value is preserved verbatim; the min-clamp (0 -> 1) | ||
| // happens later in composite.NewCompositeCommitStore, not here. | ||
| scConfig := parseSCConfigs(mapAppOpts{ | ||
| FlagSCEnable: true, | ||
| FlagSCSnapshotKeepRecent: uint32(0), | ||
| }) | ||
|
|
||
| assert.Equal(t, uint32(0), scConfig.MemIAVLConfig.SnapshotKeepRecent) |
There was a problem hiding this comment.
🟡 The comment in TestParseSCConfigs_SnapshotKeepRecentParsedRaw at app/seidb_test.go:139-140 says the min-clamp (0 -> 1) "happens later in composite.NewCompositeCommitStore, not here", but that constructor does not clamp — the 0->1 clamp lives in memiavl.Options.FillDefaults (sei-db/state_db/sc/memiavl/opts.go:57-59), invoked from memiavl.OpenDB during memiavl.CommitStore.LoadVersion. Documentation-only; consider rephrasing to "…happens later in memiavl.Options.FillDefaults during LoadVersion, not here."
Extended reasoning...
The added test comment is technically inaccurate. Tracing the code path from composite.NewCompositeCommitStore:
NewCompositeCommitStore(sei-db/state_db/sc/composite/store.go:132) callscfg.Validate(), thenalignFlatKVSnapshotWithMemIAVL(&cfg), thenmemiavl.NewCommitStore(homeDir, cfg.MemIAVLConfig).alignFlatKVSnapshotWithMemIAVL(store.go:203-210) only conditionally copies memIAVL values onto FlatKV when memIAVL is non-zero; it never rewritesMemIAVLConfig.SnapshotKeepRecent. The companion testTestAlignFlatKVSnapshotWithMemIAVL/a zero memIAVL keep-recent does not zero out FlatKVexplicitly assertscfg.MemIAVLConfig.SnapshotKeepRecentstays at 0 after align.memiavl.NewCommitStoreis a plain struct constructor — it stores the config into anOptionsvalue and does not callFillDefaults.
The actual 0->1 clamp lives at sei-db/state_db/sc/memiavl/opts.go:57-59 in Options.FillDefaults: if opts.SnapshotKeepRecent <= 0 { opts.SnapshotKeepRecent = DefaultSnapshotKeepRecent }. FillDefaults runs from OpenDB (db.go:167), which is invoked by memiavl.CommitStore.LoadVersion — a call site inside the composite store lifecycle, but not inside NewCompositeCommitStore itself. The added test in this PR (sei-db/state_db/sc/memiavl/config_test.go) verifies exactly this behavior on the memiavl side.
Step-by-step proof (matches the test's own scenario):
parseSCConfigsreadsFlagSCSnapshotKeepRecent = uint32(0)and writes it verbatim intoscConfig.MemIAVLConfig.SnapshotKeepRecent.- The test asserts
scConfig.MemIAVLConfig.SnapshotKeepRecent == 0— confirming parse-time preservation. - If the reader follows the comment's pointer and greps
NewCompositeCommitStorefor a clamp, they find only the align helper (which skips on memIAVL == 0) and the pass-through tomemiavl.NewCommitStore(a struct build). No clamp. - To find the actual clamp, the reader must instead grep
SnapshotKeepRecentin memiavl and land onFillDefaultsat opts.go:57-59, and then trace its callers toOpenDB->LoadVersion.
Impact: Pure documentation drift. Zero runtime effect — the shape of the sentence ("happens later, not here") is correct; only the specific function name is wrong. Because this comment sits on a test whose subject is precisely the "how does 0 get to 1?" question, though, it is the exact spot a future maintainer will read, and it currently sends them to a function that does not perform the clamp.
Refutation addressed: One verifier argued this is too minor to comment on given the PR already has multiple documentation nits. Agreed on the size, but the comment is on a test dedicated to explaining this specific mechanism — misdirecting readers there is more load-bearing than an incidental comment nit elsewhere. Filing as nit, not blocking. A one-line fix would be to rephrase as "…happens later in memiavl.Options.FillDefaults during LoadVersion, not here." The similarly-worded comments at seidb_test.go:83-84 and :124 are actually correct because they describe the FlatKV<-memIAVL alignment (which is in NewCompositeCommitStore via alignFlatKVSnapshotWithMemIAVL); only the min-clamp comment misplaces the mechanism.
| // state-commit.sc-* key preserves the in-code default from | ||
| // DefaultStateCommitConfig rather than reading back the zero value | ||
| // (v.Get*(absent) == 0) and clobbering it. This matters for the keys whose | ||
| // default is non-zero (async-commit-buffer 100, snapshot-interval 10000, | ||
| // keep-recent 1, ...): a config that omits a key must not silently downgrade | ||
| // the node (e.g. to synchronous commits or disabled snapshots). Mirrors the | ||
| // guarded parse in app/seidb.go's parseSCConfigs. An explicit key (including | ||
| // an explicit 0) is IsSet == true and is read verbatim. | ||
| memIAVLConfig := config.DefaultStateCommitConfig().MemIAVLConfig | ||
| if v.IsSet("state-commit.sc-async-commit-buffer") { | ||
| memIAVLConfig.AsyncCommitBuffer = v.GetInt("state-commit.sc-async-commit-buffer") | ||
| } | ||
| if v.IsSet("state-commit.sc-keep-recent") { | ||
| memIAVLConfig.SnapshotKeepRecent = v.GetUint32("state-commit.sc-keep-recent") | ||
| } | ||
| if v.IsSet("state-commit.sc-snapshot-interval") { | ||
| memIAVLConfig.SnapshotInterval = v.GetUint32("state-commit.sc-snapshot-interval") | ||
| } | ||
| if v.IsSet("state-commit.sc-snapshot-min-time-interval") { | ||
| memIAVLConfig.SnapshotMinTimeInterval = v.GetUint32("state-commit.sc-snapshot-min-time-interval") | ||
| } | ||
| if v.IsSet("state-commit.sc-snapshot-writer-limit") { | ||
| memIAVLConfig.SnapshotWriterLimit = v.GetInt("state-commit.sc-snapshot-writer-limit") | ||
| } | ||
| if v.IsSet("state-commit.sc-snapshot-prefetch-threshold") { | ||
| memIAVLConfig.SnapshotPrefetchThreshold = v.GetFloat64("state-commit.sc-snapshot-prefetch-threshold") |
There was a problem hiding this comment.
🟣 This is a pre-existing gap: GetConfig's memIAVL parse block (sei-cosmos/server/config/config.go:483-508) adds IsSet-guarded reads for six sc-* keys but omits state-commit.sc-snapshot-write-rate-mbps, so Config.StateCommit.MemIAVLConfig.SnapshotWriteRateMBps always reflects the in-code default (100) regardless of the operator's app.toml override. No runtime impact — the running SC store is built via parseSCConfigs (app/seidb.go:128), which honors the override — but since this PR refactored this exact block to add IsSet guards for the six sibling keys, it is the natural place to close the parity gap by adding one more guarded read.
Extended reasoning...
What the gap is
GetConfig in sei-cosmos/server/config/config.go:483-508 now resolves the memIAVL config via presence-checked reads:
memIAVLConfig := config.DefaultStateCommitConfig().MemIAVLConfig
if v.IsSet("state-commit.sc-async-commit-buffer") { memIAVLConfig.AsyncCommitBuffer = v.GetInt(...) }
if v.IsSet("state-commit.sc-keep-recent") { memIAVLConfig.SnapshotKeepRecent = v.GetUint32(...) }
if v.IsSet("state-commit.sc-snapshot-interval") { memIAVLConfig.SnapshotInterval = v.GetUint32(...) }
if v.IsSet("state-commit.sc-snapshot-min-time-interval") { memIAVLConfig.SnapshotMinTimeInterval = v.GetUint32(...) }
if v.IsSet("state-commit.sc-snapshot-writer-limit") { memIAVLConfig.SnapshotWriterLimit = v.GetInt(...) }
if v.IsSet("state-commit.sc-snapshot-prefetch-threshold") { memIAVLConfig.SnapshotPrefetchThreshold = v.GetFloat64(...) }The seventh memIAVL field — SnapshotWriteRateMBps — is not read here. But:
- The app.toml template renders it at
sei-db/config/toml.go:51:sc-snapshot-write-rate-mbps = {{ .StateCommit.MemIAVLConfig.SnapshotWriteRateMBps }}(default 100). - The store-construction path reads it:
app/seidb.go:128usesFlagSCSnapshotWriteRateMBps("state-commit.sc-snapshot-write-rate-mbps") with the same guarded pattern.
So GetConfig().StateCommit.MemIAVLConfig.SnapshotWriteRateMBps never reflects an operator override and always returns the in-code default.
Pre-existing, no runtime impact
Pre-PR the enclosing block used unguarded v.GetInt(...) reads inside a memiavl.Config{...} struct literal, and SnapshotWriteRateMBps was already absent from that literal. So the gap is not new — this PR converted the six existing reads to IsSet-guarded form without adding the seventh.
No runtime effect: the store is built from parseSCConfigs (via SetupSeiDB → rootmulti.NewStore), which does read the key correctly. A repo-wide grep for .SnapshotWriteRateMBps shows no consumer of GetConfig().StateCommit.MemIAVLConfig.SnapshotWriteRateMBps outside the template rendering and parseSCConfigs.
Step-by-step proof
- Operator sets
state-commit.sc-snapshot-write-rate-mbps = 50inapp.toml. parseSCConfigs(app/seidb.go:128):appOpts.Get(FlagSCSnapshotWriteRateMBps)returns 50, soscConfig.MemIAVLConfig.SnapshotWriteRateMBps = 50. Store built with 50 MB/s throttle — correct.- Any tool that calls
server/config.GetConfig(v)on the same viper instance: no branch handles thesc-snapshot-write-rate-mbpskey, somemIAVLConfig.SnapshotWriteRateMBpsstays at theDefaultStateCommitConfig()value (100). The returnedConfig.StateCommit.MemIAVLConfig.SnapshotWriteRateMBps == 100, contradicting the operator's override.
Fix
Add one more guarded read alongside the other six, matching the exact pattern:
if v.IsSet("state-commit.sc-snapshot-write-rate-mbps") {
memIAVLConfig.SnapshotWriteRateMBps = v.GetInt("state-commit.sc-snapshot-write-rate-mbps")
}Addressing the refutation
The refutation notes this is a duplicate of an earlier finding and that the submitter themselves acknowledges no runtime effect. That's correct — hence pre_existing rather than a merge blocker. Flagging it here because this PR touched the exact block; the seventh guard is a one-liner that would leave GetConfig consistent with parseSCConfigs. If the maintainers prefer to leave it, dropping the comment is fine.
3 participants
Describe your changes and provide context
Two main changes:
Also removed the check and validation for state sync, because state sync snapshot creation doesn't really require SS to be enabled at all.
Testing performed to validate your change