fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count #4158
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,8 +9,25 @@ import ( | |
| ) | ||
|
|
||
| const ( | ||
| DefaultSnapshotInterval uint32 = 10000 | ||
| // DefaultSnapshotKeepRecent is how many old checkpoints (besides the latest) to keep, | ||
| // which at the default interval is a guaranteed reach of 100,000 blocks — about 12 hours | ||
| // at mainnet's block rate. | ||
| // | ||
| // 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 | ||
|
Comment on lines
+17
to
+21
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 👍 / 👎. |
||
| // before memIAVL publishes it and no common version ever exists — which is what blocks a | ||
| // cross-backend digest and leaves a composite rollback with no shared base. | ||
| // | ||
| // Depth is affordable here because a checkpoint hardlinks its SSTs, so one costs only 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, 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, Useful? React with 👍 / 👎. 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.
interval, keepRecent := config.EffectiveMemIAVLSnapshotCadence(cfg.MemIAVLConfig)
cfg.FlatKVConfig.SnapshotInterval = interval
cfg.FlatKVConfig.SnapshotKeepRecent = keepRecent // store.go:228
The two updated golden files only pin the The fix belongs at the choke point that creates the divergence: drop 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. |
||
| ) | ||
|
|
||
| // Config defines configuration for the FlatKV (EVM) commit store. | ||
|
|
@@ -40,7 +57,10 @@ type Config struct { | |
| // SnapshotKeepRecent defines how many old snapshots to keep besides the | ||
| // latest one. 0 means keep only the current snapshot (no old snapshots). | ||
| // Ignored entirely when ExternalPruning is set. | ||
| // | ||
| // 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 defaultHigh Severity Raising Additional Locations (1)  Reviewed by Cursor Bugbot for commit 1c02b9a. Configure here. |
||
| SnapshotKeepRecent uint32 `mapstructure:"snapshot-keep-recent"` | ||
|
|
||
| // ExternalPruning hands retention to the StorageGarbageCollector: the store stops pruning its | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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:
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.