Repository navigation
fix(seidb): keep writes in the old DB until the migration boundary first moves - #4369
Conversation
…rst moves Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4369 +/- ##
==========================================
- Coverage 67.70% 66.81% -0.90%
==========================================
Files 2168 2064 -104
Lines 168283 157912 -10371
==========================================
- Hits 113938 105509 -8429
+ Misses 54336 52394 -1942
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview
Upgrade note: AppHash changes only for networks pinned to a migrate write mode with a not-yet-started paused migration; affected nodes must upgrade together and may need resync if they ran the old binary in that state. Reviewed by Cursor Bugbot for commit fc94de0. Bugbot is set up for automated code reviews on this repo. Configure here. |
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. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is placed at the one function every caller write passes through, shouldForwardWriteToNewDB. While the boundary is NotStarted, all writes now go to the old DB, which matches the rule that the lattice hash joins the AppHash only once the boundary first moves. ApplyChangeSets advances the boundary before it routes caller writes, so the first batch correctly switches routing to the started behaviour. Read still falls back to the new DB. The tests cover a paused migration that never started, the block that starts the migration, a pause mid-migration (including across a restart), and AppHash parity with MemiavlOnly. I found no issues.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4369-to-release/v6.7
git worktree add --checkout .worktree/backport-4369-to-release/v6.7 backport-4369-to-release/v6.7
cd .worktree/backport-4369-to-release/v6.7
git reset --hard HEAD^
git cherry-pick -x b32f0425b242152d16d3eea9cb7df73989e4be72
git push --force-with-lease |
…rst moves (#4369) In `MigrateEVM`, flatkv's lattice hash joins the AppHash only after the migration boundary first moves (`shouldAppendLatticeHash`). But `MigrationManager.shouldForwardWriteToNewDB` sent new keys to flatkv before that. With batch size 0 (the `NumKeysToMigratePerBlock` default), the boundary never moves, so these writes were committed but not hashed. Only a node pinned to `sc-write-mode = migrate_evm` can reach this state. Auto switches modes only with a positive batch size, and that block's first batch moves the boundary. Now, while the boundary is `NotStarted`, every caller write goes to the old DB. Thus flatkv stays empty until the boundary moves, which is what the gate assumes. The iterator migrates keys written during the pause, and after the migration starts, new keys go to flatkv again. Upgrade note: the AppHash changes only for nodes pinned to `migrate_evm`, `migrate_all_but_bank`, or `migrate_bank` with a paused migration that has not started. Auto nodes and mainnet do not change. Pinned networks in that state must upgrade together, and a node that ran the old binary in that state must resync. New tests in `migration_manager_test.go` and `store_migration_test.go` cover the paused routing, the first-batch ordering, a pause mid-migration (also across a restart), and AppHash parity with `MemiavlOnly`. --------- Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit b32f042)
## Summary - Bump `version.json` from `v6.7.0-rc3` to `v6.7.0-rc4` to cut the fourth `v6.7` release candidate. Contents since rc3: #4411, #4410, #4409, #4397, #4378, #4377, #4371, plus the rc4 changelog update (#4415, with the conflict-marker fix #4416). The changelog has already landed, so the `v6.7.0-rc4` tag will include it. - All seven are labeled `non-app-hash-breaking`. #4371 changes the AppHash only for nodes pinned to a `migrate_*` write mode while the migration hasn't started (see #4369), and the hard-fork handlers added by #4409 and #4410 are not registered for any chain in this release. Unlike rc3, moving from rc3 to rc4 should not need a coordinated validator switch. - Push `v6.7.0-rc4` by hand on this PR's merge commit once it has merged; the tagging ruleset stops `uci-release-publish` from creating it. The rc3 tag was pushed before #4339 merged and sits on `0d56aaeef`, where `version.json` still reads `v6.7.0-rc2`. - #4378 raises GoReleaser's timeout to 2h. The rc3 tag push hit the 1h limit during the emulated arm64 build and attached nothing, so rc4 is the first `v6.7` release candidate that should get binaries. ## Test plan - [x] `git diff --check` Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
In
MigrateEVM, flatkv's lattice hash joins the AppHash only after the migration boundary first moves (shouldAppendLatticeHash). ButMigrationManager.shouldForwardWriteToNewDBsent new keys to flatkv before that. With batch size 0 (theNumKeysToMigratePerBlockdefault), the boundary never moves, so these writes were committed but not hashed. Only a node pinned tosc-write-mode = migrate_evmcan reach this state. Auto switches modes only with a positive batch size, and that block's first batch moves the boundary.Now, while the boundary is
NotStarted, every caller write goes to the old DB. Thus flatkv stays empty until the boundary moves, which is what the gate assumes. The iterator migrates keys written during the pause, and after the migration starts, new keys go to flatkv again.Upgrade note: the AppHash changes only for nodes pinned to
migrate_evm,migrate_all_but_bank, ormigrate_bankwith a paused migration that has not started. Auto nodes and mainnet do not change. Pinned networks in that state must upgrade together, and a node that ran the old binary in that state must resync. New tests inmigration_manager_test.goandstore_migration_test.gocover the paused routing, the first-batch ordering, a pause mid-migration (also across a restart), and AppHash parity withMemiavlOnly.