fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count - #4158
blindchaser wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…ount A composite read needs a version both backends still retain. At mainnet state size a memIAVL rewrite takes about six hours against the 10000-block snapshot interval, so memIAVL skips generations and publishes roughly every 50,000 blocks, while a single retained FlatKV checkpoint reaches back only 10,000 to 20,000. FlatKV therefore prunes each version before memIAVL publishes it, and no common version ever exists: measured on the mainnet shadow pair, memIAVL published 229810000 at a point where FlatKV already held only 229840000 and 229850000, so the version was unreachable before it existed. That is what blocks a cross-backend digest and leaves a composite rollback with no shared base snapshot. Ten old checkpoints give a guaranteed reach of 100,000 blocks, about 12 hours, which covers that publication gap with margin. Depth is affordable because a checkpoint hardlinks its SSTs and so pins only what compaction has since obsoleted: 261 MiB of pinned SSTs plus about 25 MiB of retained state WAL each at mainnet state size, roughly 2.8 GiB for ten. The cost is linear in depth, since each older checkpoint pins exactly the files obsoleted during its own interval and those sets are disjoint. No app.toml change is needed. The `[state-commit.flatkv]` section renders no keys, so nothing pins the old value and every node inherits the new default on restart. The configuration characterization records are updated. Co-authored-by: Cursor <cursoragent@cursor.com>
9d5afec to
1c02b9a
Compare
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 1c02b9a. Configure here.
| // | ||
| // It is not mirrored from memIAVL's sc-keep-recent, and no app.toml key is rendered for | ||
| // it, so a production node runs the DefaultConfig value. | ||
| // Default: 10 |
There was a problem hiding this comment.
Alignment overwrites new keep-recent default
High Severity
Raising DefaultSnapshotKeepRecent to 10 does not change what a production node keeps. NewCompositeCommitStore still runs alignFlatKVSnapshotWithMemIAVL, which overwrites FlatKV's keep-recent with memIAVL's sc-keep-recent (default 1). Nodes therefore still retain one old checkpoint, so the cross-backend digest and composite rollback gap this change aims to close remains.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1c02b9a. Configure here.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d5afec8b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // pinned SSTs plus about 25 MiB of retained state WAL, or roughly 2.8 GiB for ten. The | ||
| // cost is linear in depth, because each older checkpoint pins exactly the files obsoleted | ||
| // during its own interval and those sets are disjoint. | ||
| DefaultSnapshotKeepRecent uint32 = 10 |
There was a problem hiding this comment.
Whenever FlatKV is opened through the production composite constructor, NewCompositeCommitStore calls alignFlatKVSnapshotWithMemIAVL (sei-db/state_db/sc/composite/store.go:152), which unconditionally replaces this value with memIAVL's effective keep-recent count (store.go:225-228), still 1 by default. Consequently, default-configured nodes continue retaining one old FlatKV checkpoint, so the intended shared-version window and rollback fix never take effect; update the alignment logic rather than only this overwritten default.
Useful? React with 👍 / 👎.
| // It is sized against memIAVL's publication rate rather than against FlatKV's own disk | ||
| // use. A composite read needs a version both backends still hold, and at mainnet state | ||
| // size a memIAVL rewrite takes about six hours against this 10000-block interval, so | ||
| // memIAVL skips generations and publishes roughly every 50,000 blocks. Keeping a single | ||
| // old checkpoint reaches back 10,000 to 20,000 blocks, so FlatKV prunes each version |
There was a problem hiding this comment.
This exported constant's Godoc devotes three paragraphs to publication mechanics, operational rationale, and measured disk costs rather than briefly describing what the constant represents. Move any load-bearing rationale to the relevant implementation and keep this Godoc to one or two subject-focused sentences, as required by the repository's documentation rules.
AGENTS.md reference: AGENTS.md:L61-L72
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4158 +/- ##
================================================
- Coverage 61.34% 60.32% -1.03%
================================================
Files 2163 2066 -97
Lines 188785 177186 -11599
================================================
- Hits 115813 106881 -8932
+ Misses 62254 60522 -1732
+ Partials 10718 9783 -935
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 new FlatKV DefaultSnapshotKeepRecent = 10 is unconditionally overwritten back to memIAVL's effective keep-recent (default 1) by alignFlatKVSnapshotWithMemIAVL at composite store construction, so the PR is a no-op on every production node and does not deliver the 100,000-block reach it claims. The golden-record updates are correct for the config-parse layer but don't reflect the store's effective retention.
Findings: 1 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] No test pins FlatKV's effective retention after store construction, which is why this regression is invisible in CI:
TestAlignFlatKVSnapshotWithMemIAVL(sei-db/state_db/sc/composite/store_test.go:2574) asserts exactly the mirroring behavior the PR intends to remove and still passes unchanged. Alongside the fix, add a test that constructs a composite store fromDefaultStateCommitConfig()and assertscfg.FlatKVConfig.SnapshotKeepRecent == flatkvconfig.DefaultSnapshotKeepRecentwhileSnapshotIntervalstill follows memIAVL — that is the invariant this PR is really about, and it is the one currently unguarded. - [suggestion] The CHANGELOG entry states the change "raises the guaranteed reach of
migrate-evm-status,dump-flatkv, a cross-backend digest and a FlatKV rollback from 10,000 blocks ... to 100,000 blocks". As written that is not true of any node built throughrootmulti/NewCompositeCommitStore. Land the mirroring fix before this entry, or the release notes promise operators an availability window they will not have. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
alignFlatKVSnapshotWithMemIAVL(sei-db/state_db/sc/composite/store.go:225) silently discards an explicitly configuredstate-commit.flatkv.snapshot-keep-recent/snapshot-interval, even thoughGetConfig(sei-cosmos/server/config/config.go:486-491) reads those keys andconfig_fuzz_test.go:317exercises them. An operator who hand-adds the key sees it accepted at parse time and ignored at runtime. The doc comment argues this is deliberate for the interval, but for keep-recent it means there is no way at all to tune FlatKV retention independently.
| // pinned SSTs plus about 25 MiB of retained state WAL, or roughly 2.8 GiB for ten. The | ||
| // cost is linear in depth, because each older checkpoint pins exactly the files obsoleted | ||
| // during its own interval and those sets are disjoint. | ||
| DefaultSnapshotKeepRecent uint32 = 10 |
There was a problem hiding this comment.
[blocker] This new default never takes effect on a production node, so the PR does not change FlatKV retention at all.
NewCompositeCommitStore calls alignFlatKVSnapshotWithMemIAVL(&cfg) (sei-db/state_db/sc/composite/store.go:152), which unconditionally does:
interval, keepRecent := config.EffectiveMemIAVLSnapshotCadence(cfg.MemIAVLConfig)
cfg.FlatKVConfig.SnapshotInterval = interval
cfg.FlatKVConfig.SnapshotKeepRecent = keepRecent // store.go:228EffectiveMemIAVLSnapshotCadence resolves to memiavl.DefaultSnapshotKeepRecent, which is still 1 (sei-db/state_db/sc/memiavl/config.go:8). The production path sei-cosmos/storev2/rootmulti/store.go:130 goes through that constructor, and app/seidb.go never sets FlatKVConfig.SnapshotKeepRecent either — so the store that actually prunes (pruneSnapshotsByCount, sei-db/state_db/sc/flatkv/snapshot.go:462) still reads 1. The reach stays at 10,000–20,000 blocks and the memIAVL publication gap the PR describes is not covered.
The two updated golden files only pin the GetConfig/DefaultConfig parse result, which is upstream of the override, so they go green without the behavior changing.
The fix belongs at the choke point that creates the divergence: drop SnapshotKeepRecent from alignFlatKVSnapshotWithMemIAVL (keeping the interval mirror, which the surrounding doc comment justifies separately), rewrite that function's doc comment so it no longer claims keep-recent is derived from sc-keep-recent, and update the TestAlignFlatKVSnapshotWithMemIAVL subtests at sei-db/state_db/sc/composite/store_test.go:2574 that currently assert the mirroring.
Relatedly, the new field doc below (line 61) — "It is not mirrored from memIAVL's sc-keep-recent ... so a production node runs the DefaultConfig value" — is the statement this PR needs to make true; today it is contradicted by store.go:228.
| DefaultSnapshotInterval uint32 = 10000 | ||
| DefaultSnapshotKeepRecent uint32 = 1 | ||
| DefaultSnapshotInterval uint32 = 10000 | ||
| // DefaultSnapshotKeepRecent is how many old checkpoints (besides the latest) to keep, |
There was a problem hiding this comment.
[suggestion] This godoc is three paragraphs of rationale, measurement, and mechanism, which is what AGENTS.md's "Godoc" section rules out: godocs say what a thing is, not why it came to be or how it works inside; rationale belongs in an inline comment at the line that needs it, and multi-paragraph godocs are rare.
Suggest collapsing to the what and moving the sizing argument to an inline comment on the constant:
// DefaultSnapshotKeepRecent is how many old checkpoints, besides the latest, FlatKV
// retains. At DefaultSnapshotInterval that is a guaranteed reach of 100,000 blocks.The memIAVL-publication-rate derivation and the 2.8 GiB disk measurement are genuinely load-bearing and worth keeping — just not in the godoc. The measured numbers in particular will age, and an inline comment is where a future reader expects to find a value that was tuned against a specific state size.
1 participant
A composite read needs a version both backends still retain. At mainnet state
size a memIAVL rewrite takes about six hours against the 10000-block snapshot
interval, so memIAVL skips generations and publishes roughly every 50,000
blocks, while a single retained FlatKV checkpoint reaches back only 10,000 to
20,000. FlatKV therefore prunes each version before memIAVL publishes it, and
no common version ever exists: measured on the mainnet shadow pair, memIAVL
published 229810000 at a point where FlatKV already held only 229840000 and
229850000, so the version was unreachable before it existed. That is what
blocks a cross-backend digest and leaves a composite rollback with no shared
base snapshot.
Ten old checkpoints give a guaranteed reach of 100,000 blocks, about 12 hours,
which covers that publication gap with margin.
Depth is affordable because a checkpoint hardlinks its SSTs and so pins only
what compaction has since obsoleted: 261 MiB of pinned SSTs plus about 25 MiB
of retained state WAL each at mainnet state size, roughly 2.8 GiB for ten. The
cost is linear in depth, since each older checkpoint pins exactly the files
obsoleted during its own interval and those sets are disjoint.
No app.toml change is needed. The
[state-commit.flatkv]section renders nokeys, so nothing pins the old value and every node inherits the new default on
restart. The configuration characterization records are updated.
Co-authored-by: Cursor cursoragent@cursor.com
Made with Cursor