Repository navigation
Backport release/v6.7: fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count - #4347
Conversation
|
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4322-to-release/v6.7
git worktree add --checkout .worktree/backport-4322-to-release/v6.7 backport-4322-to-release/v6.7
cd .worktree/backport-4322-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x 8c95ff34abd8634c1bfbabaddc6fbe85823a2c02
git push --force-with-lease |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
7745b05 to
c21346a
Compare
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 Docs, config comments, golden fixtures, and composite-store tests are updated to match, including coverage that default-config nodes keep FlatKV’s independent keep-recent after store construction. Reviewed by Cursor Bugbot for commit b7d710a. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The backport correctly separates FlatKV's keep-recent from memIAVL's, raises its default to 10, and adds tests for the new behavior. However, it does not regenerate app/testdata/state-commit.golden, so app.TestDefaultsMatchTheRecordedValues will fail on this branch.
Findings: 1 blocking | 0 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
Inline comments (could not post inline; listed here)
sei-db/state_db/sc/flatkv/config/config.go:13(RIGHT) -- [blocker] ChangingDefaultSnapshotKeepRecentto 10 meansapp/testdata/state-commit.goldenmust be regenerated too. Line 17 of that file still recordsFlatKVConfig.SnapshotKeepRecent = uint32(1).app/config_fuzz_test.goTestDefaultsMatchTheRecordedValuesrunsconfigtest.CheckDefaults(t, "state-commit", config.DefaultStateCommitConfig())against that file, so it will fail. The comment above that test warns about exactly this: moving a StateCommitConfig default breaks bothappandsei-cosmos/server/config, and this PR regenerates onlyserver_config.golden. Regenerate the app golden too and include its diff (seetestutil/configtest/AGENTS.md).
![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.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c21346a. 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: c21346a71c
ℹ️ 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".
| const ( | ||
| DefaultSnapshotInterval uint32 = 10000 | ||
| DefaultSnapshotKeepRecent uint32 = 1 | ||
| DefaultSnapshotKeepRecent uint32 = 10 |
There was a problem hiding this comment.
Changing this default makes config.DefaultStateCommitConfig() report SnapshotKeepRecent = 10, but app/testdata/state-commit.golden still records 1. Consequently, app.TestDefaultsMatchTheRecordedValues (app/config_fuzz_test.go:633-635) deterministically fails when configtest.CheckDefaults compares the new default with the stale record; update that second golden alongside the server-config golden.
AGENTS.md reference: AGENTS.md:L21-L27
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4347 +/- ##
================================================
- Coverage 61.37% 60.39% -0.98%
================================================
Files 2163 2070 -93
Lines 189086 178179 -10907
================================================
- Hits 116057 107617 -8440
+ Misses 62304 60704 -1600
+ Partials 10725 9858 -867
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
c21346a to
e6bc047
Compare
…ount (#4322) A composite read needs a version that both backends still hold. At mainnet state size, memIAVL publishes a snapshot only about every 50,000 blocks. One retained FlatKV checkpoint reaches back only 10,000 to 20,000 blocks, so FlatKV prunes each version before memIAVL publishes it, and no common version exists. On the mainnet shadow pair, memIAVL published 229810000 when FlatKV held only 229840000 and 229850000. This blocks a cross-backend digest and leaves a composite rollback with no shared base snapshot. - FlatKV's default `SnapshotKeepRecent` goes from 1 to 10. At the default 10,000-block interval, this is a guaranteed reach of 100,000 blocks (about 12 hours). - `alignFlatKVSnapshotWithMemIAVL` still copies memIAVL's effective snapshot **interval** to FlatKV, but it no longer overwrites FlatKV's **keep-recent** with `sc-keep-recent`. Without this, the new default has no effect on a node (the review finding on #4158). - New test `TestNewCompositeCommitStoreKeepsFlatKVKeepRecent` builds a composite store from the default config and checks the effective FlatKV retention. `TestAlignFlatKVSnapshotWithMemIAVL` now checks that keep-recent is left as configured. - Three test configs that set only memIAVL keep-recent (and relied on the mirror) now also set FlatKV keep-recent. A checkpoint hardlinks its SSTs, so each one costs only what compaction has obsoleted since: about 261 MiB of pinned SSTs plus about 25 MiB of retained state WAL at mainnet state size, roughly 2.8 GiB for ten. No `app.toml` change is needed. The node path (`parseSCConfigs` in `app/seidb.go`) does not read `state-commit.flatkv.snapshot-keep-recent`, so a key rendered by the v6.4–v6.6 templates does not pin the old value. Every node gets the new default on restart. Replaces #4158, which targeted `release/v6.7` directly. Labelled `backport release/v6.7`. The FlatKV `config.go` differs between `main` and `release/v6.7` (v6.7 still has the `DefaultSnapshotKeepRecent` constant and the config golden records), so the automatic backport can conflict there. - `scripts/ramtest.sh` on `sei-db/state_db/sc/composite`, `sei-db/state_db/sc/flatkv/...`, `sei-db/config`, `sei-cosmos/storev2/rootmulti`, `sei-cosmos/server/config`, and the `app` config tests. - `make fmtcheck`. --------- Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 8c95ff3) Co-authored-by: Cursor <cursoragent@cursor.com>
e6bc047 to
b7d710a
Compare
|
@seidroid review |
There was a problem hiding this comment.
This backport cleanly separates FlatKV's keep-recent from memIAVL's, raises the default to 10, and now updates both config golden files, so the earlier blocker is fixed. One non-blocking gap remains: nodes still carrying the old template's explicit flatkv.snapshot-keep-recent value won't get the new depth.
Findings: 0 blocking | 1 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
Inline comments (could not post inline; listed here)
sei-db/state_db/sc/composite/store.go:206(RIGHT) -- [suggestion] Once keep-recent stops being mirrored, a stale explicitstate-commit.flatkv.snapshot-keep-recentwins again. An older app.toml template rendered that key, and the old comment here says so.GetConfigstill reads it when it is set (sei-cosmos/server/config/config.go). Before this PR, the unconditional mirror overwrote that stale value; now an upgrading node whose app.toml still hassnapshot-keep-recent = 1keeps retention at 1. That node doesn't get the 100k-block reach this fix is meant to provide. The comment above explicitly handles the stale interval case but not this one. Either document it in the changelog/upgrade notes (operators should remove or raise the key) or decide on a policy for it, such as a startup warning when the value is below the default.
Regenerated with ./scripts/generate-changelog.sh release/v6.6 release/v6.7. Co-authored-by: Cursor <cursoragent@cursor.com>
Adds the `release/v6.7` entries merged since the rc2 changelog (#4293), in prep to cut **v6.7.0-rc4**. The rc3 update (#4335) was closed without merging, so its entries are included here: - [#4411](#4411) — Log a pinned node's skipped migration kick-off once per batch size - [#4410](#4410) — Apply compiled KV repair files at a fixed height and generate them from digest inspect lists - [#4409](#4409) — feat: add migration pause handler - [#4397](#4397) — fix(evmrpc): release eth_getLogs DB-read slots when a block read panics - [#4378](#4378) — Raise goreleaser timeout to 2h - [#4377](#4377) — Fix FlatKV state sync bad-hash scenario - [#4371](#4371) — fix(seidb): keep writes in the old DB until the migration boundary first moves - [#4348](#4348) — fix(seidb): report only the current migration boundary on the snapshot gauge - [#4347](#4347) — fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count - [#4339](#4339) — rc3 version bump - [#4334](#4334) — Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call - [#4315](#4315) — Pin the Go builder image per architecture in build-static.sh - [#4313](#4313) — fix(memiavl): hold a snapshot reference for an iterator's lifetime - [#4295](#4295) — rc2 version bump - [#4294](#4294) — rc2 changelog backport Also returns `## v6.7` to the format used through v6.6: the version heading, `sei-chain`, and the generated PR list. The hand-written `### Improvements` and `### Upgrade guide` sections are removed; every PR they described is already a line in the generated list. Regenerated with `./scripts/generate-changelog.sh release/v6.6 release/v6.7`; only the `## v6.7` section changes. Docs-only; no code change. **Backport note:** the #4347 backport added its own line to the top of the `## v6.7` list on `release/v6.7`, and `main` doesn't have it, so the `backport release/v6.7` cherry-pick of this PR conflicts at that one spot (simulated with `git merge-tree`). Resolve it by taking this PR's side: its fifteen lines already include #4347. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
#4415, seidroid's backport of #4406, was merged with its `BACKPORT-CONFLICT` placeholder commit, which committed the unresolved cherry-pick into `CHANGELOG.md` on `release/v6.7`: three conflict-marker lines, plus a duplicate #4347 line from the `release/v6.7` side. This removes those four lines and keeps #4406's side, which already lists #4347. Afterwards: - `## v6.7` matches `./scripts/generate-changelog.sh release/v6.6 release/v6.7` again. The #4415 line itself is left for the next changelog update, as with earlier changelog backports. - The `## v6.7` section is identical on `release/v6.7` and `main`, so the next changelog backport applies cleanly. Docs-only; no code change. Should merge before the rc4 version bump (#4407), so v6.7.0-rc4 ships a clean changelog. Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Backport of #4322 to
release/v6.7.