fix(flatkv): retain 72 checkpoints instead of mirroring memIAVL's count - #4145
fix(flatkv): retain 72 checkpoints instead of mirroring memIAVL's count#4145blindchaser wants to merge 2 commits into
Conversation
FlatKV took both its snapshot interval and its retention count from memIAVL's sc-* keys, and sc-keep-recent defaults to 1. FlatKV therefore kept one old checkpoint, a guaranteed reach of 10,000 blocks, or 74 minutes at mainnet's block rate. Below that reach migrate-evm-status, dump-flatkv and a cross-backend digest cannot open a past version, and a rollback has no base snapshot to rewind to, so the EVM migration window was unobservable within an hour of passing. A single shared count cannot serve both backends, because it is a disk decision and not a cadence. Measured on a mainnet-sized shadow node, one further retained memIAVL snapshot costs 56,782 MiB, since memIAVL snapshots are independent full copies, while one further FlatKV checkpoint costs about 286 MiB, since Pebble checkpoints hardlink their SSTs and so only pin the bytes compaction has since obsoleted. The depth FlatKV wants would ask memIAVL for more than the volume holds. Mirror only the interval, which must match because a rollback rewinds both backends and a cross-backend digest opens both at one height, and give FlatKV its own retention default of 72. That is a guaranteed reach of 720,000 blocks, about 89 hours, against an 85-hour drain at K=1024, for roughly 20 GiB.
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. |
PR SummaryMedium Risk Overview That extends how far back FlatKV can serve historical versions—roughly 720,000 blocks (~89h at default interval vs ~74 minutes before)—for Tests and config comments are updated to lock in interval-only mirroring and guard default retention against the EVM migration window. Reviewed by Cursor Bugbot for commit aabd353. 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).
|
![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: 7f6998f085
ℹ️ 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".
| // It is this deep because a FlatKV checkpoint is nearly free. Checkpoints | ||
| // hardlink their SSTs, so one only costs the bytes compaction has since made | ||
| // obsolete: measured at mainnet state size, 261 MiB of pinned SSTs plus about | ||
| // 25 MiB of retained state WAL. 72 of them is roughly 20 GiB. The cost is linear | ||
| // in depth, because each older checkpoint pins exactly the files obsoleted during |
There was a problem hiding this comment.
This new Godoc devotes multiple paragraphs to migration rationale, mainnet measurements, hardlink mechanics, and affected commands rather than documenting what DefaultSnapshotKeepRecent represents. The repository explicitly requires Godocs to describe their subject in one or two sentences and keep rationale, mechanisms, and surrounding architecture out, so this should be reduced to the retention semantics and the rationale moved inline or omitted.
AGENTS.md reference: AGENTS.md:L79-L90
Useful? React with 👍 / 👎.
| // It is not mirrored from memIAVL's sc-keep-recent, and the production store | ||
| // reads no app.toml key for it, so a node runs the DefaultConfig value. See | ||
| // composite.alignFlatKVSnapshotIntervalWithMemIAVL for why the two backends | ||
| // share an interval but not a retention count. |
There was a problem hiding this comment.
The added paragraph documents which production reader and composite helper configure this field, rather than what SnapshotKeepRecent means to callers. That system-level wiring is expressly excluded from field Godocs by the repository guidance and will also become stale when configuration paths change; keep the existing retention and pruning semantics here and place wiring rationale at the relevant implementation site instead.
AGENTS.md reference: AGENTS.md:L79-L90
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4145 +/- ##
==========================================
- Coverage 61.58% 60.67% -0.92%
==========================================
Files 2196 2085 -111
Lines 193825 181222 -12603
==========================================
- Hits 119366 109949 -9417
+ Misses 62886 60761 -2125
+ Partials 11573 10512 -1061
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.
Splitting FlatKV's retention count from memIAVL's sc-keep-recent is the right correction and is applied at the single choke point (alignFlatKVSnapshotIntervalWithMemIAVL), with the config/doc trail updated consistently; production reads of the key were verified to be unaffected. Two non-blocking concerns: the surviving zero-interval test became tautological after the deleted subtest, and the ~20 GiB disk estimate is measured at the tip rather than during the migration drain the depth is sized for.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The PR's motivation table lists "a rollback" as gaining 720,000 blocks of reach, but
CompositeCommitStore.Rollback(sei-db/state_db/sc/composite/store.go:1374) rewinds memIAVL first, and memIAVL still runs atsc-keep-recent= 1. On a composite nodeseid rollbacktherefore still fails at memIAVL after ~10-20k blocks; only FlatKV-only rollback and the FlatKV-only read paths (migrate-evm-status,dump-flatkv, a FlatKV digest) actually gain the deeper reach. The CHANGELOG wording ("a FlatKV rollback") is technically precise, but worth stating the composite limit explicitly so operators don't size recovery expectations from it. - [suggestion] The new
DefaultSnapshotKeepRecentgodoc (sei-db/state_db/sc/flatkv/config/config.go:13-28) is three paragraphs of rationale — cost measurements, the linearity argument, why reach matters — which is what AGENTS.md's godoc rules ask to keep out ("Explain WHAT, not WHY or HOW", "Multi-paragraph godocs are rare"). The same applies to the expandedalignFlatKVSnapshotIntervalWithMemIAVLdoc. Suggest keeping the godoc to what the constant is and its effective reach, and moving the measurement/cost argument to an inline comment at the constant or leaving it in the CHANGELOG entry, which already carries it. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // A raw 0 would disable FlatKV auto-snapshots; instead FlatKV mirrors the | ||
| // value FillDefaults will heal memIAVL's interval to. | ||
| alignFlatKVSnapshotWithMemIAVL(&cfg) | ||
| alignFlatKVSnapshotIntervalWithMemIAVL(&cfg) |
There was a problem hiding this comment.
[suggestion] This subtest no longer proves anything after the deleted "an explicit FlatKV override loses to memIAVL's healed default" case. It starts from DefaultStateCommitConfig(), so cfg.FlatKVConfig.SnapshotInterval is already 10000, and memiavl.DefaultSnapshotInterval is also 10000 (sei-db/state_db/sc/memiavl/config.go:4) — the assertion passes even if alignFlatKVSnapshotIntervalWithMemIAVL did nothing at all on a zero memIAVL interval.
The deleted subtest was the only one covering resolve-then-assign versus skip-on-zero, i.e. the upgrade case where an old app.toml still pins an explicit state-commit.flatkv.snapshot-interval while sc-snapshot-interval is 0. That scenario is still live for the interval even though the retention half of it is gone. One line restores it:
cfg.MemIAVLConfig.SnapshotInterval = 0
cfg.FlatKVConfig.SnapshotInterval = 7777 // must lose to memIAVL's healed default| // It is this deep because a FlatKV checkpoint is nearly free. Checkpoints | ||
| // hardlink their SSTs, so one only costs the bytes compaction has since made | ||
| // obsolete: measured at mainnet state size, 261 MiB of pinned SSTs plus about | ||
| // 25 MiB of retained state WAL. 72 of them is roughly 20 GiB. The cost is linear |
There was a problem hiding this comment.
[suggestion] The 261 MiB / 25 MiB per-checkpoint figures are measured at the tip post-H_done, but the depth is chosen for the drain window — and both components scale with write volume, which is much higher during the drain than at the tip.
The WAL half is the clearer one: tryTruncateWAL (sei-db/state_db/sc/flatkv/snapshot.go:905) prunes below the earliest snapshot, so raising retention from 1 to 72 widens the retained WAL span from ~20k to 720k blocks. During the drain every block's changeset additionally carries NumKeysToMigratePerBlock (1024) migrated keys through s.wal.Write (store_write.go:86), so WAL bytes per interval are far above the steady-state 25 MiB. Near the end of an 85-hour drain the retained WAL approximates the whole migrated EVM dataset in changeset form. The pinned-SST half moves the same way: a bulk ingest obsoletes far more per interval than tip traffic does, so each checkpoint pins more than 261 MiB.
So peak disk during exactly the window this depth exists for is plausibly several times the ~20 GiB the doc and CHANGELOG quote. It likely still fits the 2 TiB volume, but the number operators will size from is the steady-state one. Worth either measuring one interval mid-drain and quoting the peak, or stating that 20 GiB is the post-migration steady state and the drain peak is higher.
This compounds with there being no escape hatch: parseSCConfigs (app/seidb.go:104) reads no FlatKV snapshot key, and this PR removes sc-keep-recent as the indirect lever, so an operator who runs short of disk mid-drain cannot lower the retention without a new binary.
|
Superseded by #4158, which lands the same fix on |
1 participant
What this changes
FlatKV stops taking its retention count from memIAVL and keeps its own default of
72 old checkpoints. The snapshot interval is still mirrored.
Why
alignFlatKVSnapshotWithMemIAVLoverwrote both of FlatKV's snapshot knobs frommemIAVL's
sc-*keys, andsc-keep-recentdefaults to 1. FlatKV therefore kept oneold checkpoint.
ExternalPruningis false on the composite path, so nothing elseextended that.
Below FlatKV's reach,
migrate-evm-status,dump-flatkvand a cross-backend digestcannot open a past version, and a rollback has no base snapshot to rewind to. The EVM
migration window became unobservable 74 minutes after passing.
The shared knob cannot express the setting we want, and this is not a matter of cost.
Measured on the mainnet-sized
pacific-1shadow node, post-H_done, at the tip:A Pebble checkpoint hardlinks its SSTs, so an old checkpoint only pins the bytes
compaction has since obsoleted. The factor is about 200. At the depth FlatKV wants,
the memIAVL side of a shared count would need 3.68 TiB on a 2 TiB volume, so the
knob has to be split before any number can be chosen.
Why 72
72 checkpoints at the 10,000-block interval is a guaranteed reach of 720,000 blocks,
about 89 hours at the shadow node's measured 2.247 blocks/s, against an 85-hour drain
at
NumKeysToMigratePerBlock = 1024. It covers the whole migration window with margin.The cost is linear in depth, not sublinear: each older checkpoint pins exactly the
files obsoleted during its own interval, and those sets are disjoint by construction.
It only saturates at ~93 checkpoints, where the pinned bytes reach the 24,241 MiB of
live SSTs.
How operators get it
The production store is built by
parseSCConfigs, which starts from the in-codedefaults and reads no FlatKV retention key, so raising the default reaches every
existing node with no
app.tomledit. Nodes carrying a stale explicitstate-commit.flatkv.snapshot-keep-recentfrom the pre-#3745 template are alsounaffected, because that key never reaches a store.
Making the key actually readable is a deliberate follow-up rather than part of this
change, so the v6.7 backport carries no new config read.
Testing
TestAlignFlatKVSnapshotIntervalWithMemIAVLreplaces the test that pinned theboth-values mirror; it now pins that the interval is mirrored, a zero interval is
still healed before assignment, and neither a set nor a zero memIAVL keep-recent
reaches FlatKV.
TestFlatKVDefaultRetentionSpansTheMigrationWindowguards the reach itself, so alater depth change that no longer spans the window fails rather than passing quietly.
./sei-db/state_db/sc/composite/,./sei-db/state_db/sc/flatkv/...,./sei-db/config/,./sei-cosmos/server/config/,./app/all pass.make fmtcheckandmake dblintare clean.Backport
release/v6.7is where this is needed, and it is filed separately becausemainhassince removed the golden config suite and the FlatKV default constants, so the two
diffs are not identical.
v6.7.0-rc1is already cut, so the backport implies an rc2.