-
Notifications
You must be signed in to change notification settings - Fork 886
fix(flatkv): retain 72 checkpoints instead of mirroring memIAVL's count #4145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,23 @@ import ( | |
| "github.com/sei-protocol/sei-chain/sei-db/state_db/sc/flatkv/lthash" | ||
| ) | ||
|
|
||
| // DefaultSnapshotKeepRecent is how many old checkpoints (besides the latest) to | ||
| // keep, which at the default snapshot interval of 10000 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. | ||
| // | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] The 261 MiB / 25 MiB per-checkpoint figures are measured at the tip post- The WAL half is the clearer one: 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: |
||
| // in depth, because each older checkpoint pins exactly the files obsoleted during | ||
|
Comment on lines
+18
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new Godoc devotes multiple paragraphs to migration rationale, mainnet measurements, hardlink mechanics, and affected commands rather than documenting what AGENTS.md reference: AGENTS.md:L79-L90 Useful? React with 👍 / 👎. |
||
| // its own interval and those sets are disjoint. | ||
| // | ||
| // 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. | ||
| const DefaultSnapshotKeepRecent uint32 = 72 | ||
|
|
||
| // Config defines configuration for the FlatKV (EVM) commit store. | ||
| type Config struct { | ||
| // DataDir is the root directory for the FlatKV data files. | ||
|
|
@@ -34,6 +51,11 @@ 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 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. | ||
|
Comment on lines
+55
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The added paragraph documents which production reader and composite helper configure this field, rather than what AGENTS.md reference: AGENTS.md:L79-L90 Useful? React with 👍 / 👎. |
||
| SnapshotKeepRecent uint32 `mapstructure:"snapshot-keep-recent"` | ||
|
|
||
| // MaxSnapshotLagBlocks is how many committed blocks may queue up behind a snapshot that is still | ||
|
|
@@ -136,7 +158,7 @@ func DefaultConfig() *Config { | |
| Fsync: false, | ||
| AsyncWriteBuffer: 0, | ||
| SnapshotInterval: 10000, | ||
| SnapshotKeepRecent: 1, | ||
| SnapshotKeepRecent: DefaultSnapshotKeepRecent, | ||
| MaxSnapshotLagBlocks: 64, | ||
| EnablePebbleMetrics: true, | ||
| AccountDBConfig: pebbledb.DefaultConfig(), | ||
|
|
||
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 subtest no longer proves anything after the deleted "an explicit FlatKV override loses to memIAVL's healed default" case. It starts from
DefaultStateCommitConfig(), socfg.FlatKVConfig.SnapshotIntervalis already 10000, andmemiavl.DefaultSnapshotIntervalis also 10000 (sei-db/state_db/sc/memiavl/config.go:4) — the assertion passes even ifalignFlatKVSnapshotIntervalWithMemIAVLdid 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-intervalwhilesc-snapshot-intervalis 0. That scenario is still live for the interval even though the retention half of it is gone. One line restores it: