Backport release/v6.7: retain 72 FlatKV checkpoints instead of mirroring memIAVL's count - #4146
blindchaser wants to merge 1 commit into
Conversation
…oring memIAVL's count 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. Backport of #4145.
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 Tests and golden configs are updated to lock in interval-only mirroring and the new default depth (~20 GiB extra disk at mainnet-sized state, from hardlinked checkpoints). CHANGELOG documents the behavior for v6.7. Reviewed by Cursor Bugbot for commit 989e1b0. 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: 989e1b0c3b
ℹ️ 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".
| // DefaultSnapshotKeepRecent keeps 72 old checkpoints besides the latest one, | ||
| // which at DefaultSnapshotInterval is a guaranteed reach of 720,000 blocks — | ||
| // about 89 hours at mainnet's block rate, so it spans the EVM migration | ||
| // window at the rate that window is planned for. | ||
| // |
There was a problem hiding this comment.
Rewrite this exported godoc to state only that the constant is the default number of retained checkpoints. The migration timing, disk-cost estimates, hardlink mechanism, and operational rationale turn it into a three-paragraph design note that will become stale with deployment conditions and violates the repository requirement that godocs describe what a subject is in one or two sentences rather than why or how it works.
AGENTS.md reference: AGENTS.md:L59-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 #4146 +/- ##
================================================
- Coverage 61.34% 60.36% -0.99%
================================================
Files 2163 2070 -93
Lines 188785 177915 -10870
================================================
- Hits 115813 107396 -8417
+ Misses 62254 60665 -1589
+ Partials 10718 9854 -864
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.
Backport is mechanically correct: the retention mirror is dropped, the interval mirror kept, and goldens, tests, and comments are updated consistently. Two non-blocking points on the documented disk cost (the WAL term is measured post-migration but retention is sized for the migration window) and on retention now having no operator-visible knob.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // Reach matters because it bounds what can be answered about a past height at | ||
| // all. Below it, migrate-evm-status, dump-flatkv and a cross-backend digest | ||
| // cannot open a version, and a rollback has no base snapshot to rewind to. | ||
| DefaultSnapshotKeepRecent uint32 = 72 |
There was a problem hiding this comment.
[suggestion] The ~20 GiB cost model is extrapolated from a steady-state measurement ("post-H_done, at the tip"), but the depth is sized for the migration window, where both components are larger.
The WAL term is the bigger gap. tryTruncateWAL (sei-db/state_db/sc/flatkv/snapshot.go:682) prunes the state WAL below the earliest retained snapshot, so 1 -> 72 widens retained WAL from ~20,000 blocks to ~730,000. During the drain every block's FlatKV changeset also carries the NumKeysToMigratePerBlock batch (migration_manager.go:301-315 routes migrated pairs into the new DB's changeset, which Commit writes to the WAL), so the WAL retained across that window approaches the whole migrated EVM state — on the order of the 24,241 MiB of live SSTs this PR cites — rather than the ~1.8 GiB implied by 72 x 25 MiB. K is governance-tunable up to MaxNumKeysToMigratePerBlock = 1_000_000, so the rate this is measured against is not a floor. The pinned-SST term is also likely above 261 MiB/interval while the whole state is being bulk-inserted and compaction churn is at its peak.
Probably still affordable on the 2 TiB volume, but the number operators will plan against is off by roughly an order of magnitude in exactly the window the default exists for. Worth re-measuring mid-drain and correcting this comment and the CHANGELOG, or bounding WAL retention independently of snapshot count.
| // Options.FillDefaults would apply at OpenDB. Mirroring a raw 0 is never correct: | ||
| // 0 disables auto-snapshots for FlatKV, which lets the WAL grow without bound and | ||
| // makes every restart replay from snapshot-0. | ||
| func alignFlatKVSnapshotIntervalWithMemIAVL(cfg *config.StateCommitConfig) { |
There was a problem hiding this comment.
[suggestion] After this change FlatKV's retention has no operator-visible lever at all: parseSCConfigs (app/seidb.go:101) reads no FlatKV snapshot key, the template hides flatkv.*, and the sc-keep-recent mirror is gone. A disk-constrained node cannot reduce the 73 retained checkpoints without a new binary. The doc's own argument for keeping the interval mirror unconditional — "otherwise FlatKVOnly would have no template-visible way to tune it" — now applies to the count with no answer.
Relatedly, because retention is a fixed count while the interval is mirrored from sc-snapshot-interval, both the reach and the disk cost scale with whatever the operator sets there. sc-snapshot-interval = 1000 collapses the "guaranteed" 720,000-block reach to 72,000 blocks (~9 h, well under the 85-h drain) with no warning, and a larger interval multiplies the bytes each checkpoint pins. TestFlatKVDefaultRetentionSpansTheMigrationWindow only pins this at the default interval, so that case is not covered.
Expressing the target in blocks (or clamping the count against the effective interval so the reach invariant holds for any interval) would make the guarantee this default is named for actually hold, and give the count a defined behavior when the interval is tuned.
1 participant
Backport of #4145 to
release/v6.7. This is the branch the change is actually neededon: the FlatKV EVM migration ships in v6.7.
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, and
ExternalPruningis false on this path, so nothing else extendedthat.
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.
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: 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
pacific-1validator with noapp.tomledit. Nodes carrying a staleexplicit
state-commit.flatkv.snapshot-keep-recentfrom the pre-#3745 template arealso unaffected, because that key never reaches a store.
This backport deliberately adds no new config read. Making the key readable is a
follow-up on
mainonly.Differences from #4145
Not a clean cherry-pick.
mainhas since removed the golden config suite (#4057) andthe FlatKV default constants (#4039), so this branch additionally records the new
default in two golden files, and puts the rationale on the existing
DefaultSnapshotKeepRecentconstant rather than reintroducing one.Golden updates, regenerated per package with
-update:app/testdata/state-commit.golden:FlatKVConfig.SnapshotKeepRecentuint32(1)→uint32(72)sei-cosmos/server/config/testdata/server_config.golden:StateCommit.FlatKVConfig.SnapshotKeepRecentuint32(1)→uint32(72)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../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.Release note
v6.7.0-rc1is already cut, so merging this implies an rc2.