Repository navigation
Backport release/v6.7: Refuse pinned write modes that disagree with persisted migration - #4503
Conversation
A store pinned to a steady-state write mode (`evm_migrated`, `all_migrated_but_bank` or `flatkv_only`) copies the configured mode without looking at the migration state persisted in flatkv. When that state says a migration is still in flight, for example after `Rollback` to a height below a migration's completion, the pinned router and commit info no longer match the data. A `flatkv_only` pin never opens memiavl, so its commit info drops memiavl's stores. An `evm_migrated` pin stops running the in-flight migration, so the next block diverges. Either way it persists across restarts for as long as the pin stays. `resolveCurrentWriteMode`, which `LoadLatest` and, since #4477, `Rollback` both go through, now calls `requirePinnedModeMatchesMigration`. It derives the persisted mode and refuses the pin unless the two match or the persisted mode is `MemiavlOnly`, which is what a store whose history began in flatkv derives. The error tells the operator to re-enable auto mode, and reopening in Auto reconciles memiavl down to the rolled-back height. This builds on #4477 and is based on its branch, so the diff here shows only this change. It touches the same fields as #4475, so whichever lands second needs a small rebase. There is no AppHash change for supported configurations: the genesis-pinned `flatkv_only` localnode and CI clusters carry no migration metadata and are unaffected. New composite tests cover the rollback refusal and recovery in Auto, the in-flight refusal for each steady-state pin, and a genesis-pinned store, and the composite, migration and rootmulti packages pass under `-race`. (cherry picked from commit 8d4ac58)
|
@seidroid review |
PR SummaryMedium Risk Overview Integration tests cover refusing pinned opens/rollbacks while Reviewed by Cursor Bugbot for commit 1a3fb7c. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4503 +/- ##
================================================
- Coverage 61.85% 60.86% -0.99%
================================================
Files 2168 2072 -96
Lines 188679 177580 -11099
================================================
- Hits 116701 108087 -8614
+ Misses 61128 59536 -1592
+ Partials 10850 9957 -893
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@seidroid review |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing material has changed since the last review: the diff is byte-identical (155 lines), the merge commit is still 18a5782, and none of my earlier findings is addressed, so they stand as before and the verdict is still approve with suggestions. Scout codex repeated its single finding; I kept it at reduced severity in the inline comment at store.go:512, because the gap is real but was already there before this change.
Non-blocking
1 finding on the changed lines, as inline comments.
Rollbackrolls both backends back on disk beforeadoptPersistedWriteModeruns the new check. A refused pinned rollback therefore returns an error after the state has already moved to the target, and the new FlatKVOnly rollback test relies on that. An operator who seesseid rollbackfail will not know it took effect. Either check the pin against the target height's migration state before changing anything, or say in the error that the backends were already rolled back.LoadVersionReadOnlyand the export path that builds read-only views also go throughresolveCurrentWriteMode. On a pinned node, any historical view at a height where a migration was in flight now fails with this error instead of building the pinned router. That is probably safer than misrouting, but it changes behaviour for historical queries and snapshot export, and no test covers a read-only view under a pin.
seidroid review · decision approve · session 525f1d01035249bbbedc8f970d56ce85 · turn resp_claude_60754720a62954c578f29c6f1c84e1af · item 7469be07230d5f7e94283e90657c4d32
Findings: 0 blocking | 3 non-blocking | 1 posted inline
| if err != nil { | ||
| return fmt.Errorf("failed to derive persisted write mode: %w", err) | ||
| } | ||
| if persisted == pinned || persisted == types.MemiavlOnly { |
There was a problem hiding this comment.
suggestion — (Also raised by codex.) Accepting MemiavlOnly for every pin also lets through a store with real memiavl history and no migration, such as a pinned EVMMigrated or AllMigratedButBank node whose memiavl is at version > 0 while flatkv derives MemiavlOnly (freshly seeded, or rolled back below activation). Such a node then runs steady-state routing over a flatkv that never received the migrated data. Those two pins already open memiavl, so memIAVL.Version() > 0 combined with a MemiavlOnly derivation is evidence you can refuse on; FlatKVOnly never opens memiavl and would need some other provenance signal.
There was a problem hiding this comment.
Claude finished @alexander-sei's task in 12s —— View job
Approval request
I can't approve this PR. For security reasons this integration can't submit formal GitHub PR reviews, including approvals. All I can do is reply in this comment. The FAQ has more on this and possible workarounds.
Some context that may help:
- You already approved it. Your review from 17:00:58Z is still there, so this PR has one approval. If branch protection on
release/v6.7needs more, a second maintainer has to approve it. - My last review recommended approval. It found nothing blocking: 0 blocking and 3 non-blocking items. All three are about behaviour this change inherits:
- A refused pinned
Rollbackreturns an error after both backends have already been rolled back on disk. - Read-only and export views at heights where a migration was in flight now fail under a pin, and no test covers this.
- Accepting
MemiavlOnlyfor every pin lets through a pinned node that has memiavl history but no migration (store.go:512).
- A refused pinned
None of these stops a backport that matches #4484. They are better handled as follow-ups on main.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Systematic changelog re-generation for the v6.7.2 patch release. Adds the PRs merged to `release/v6.7` since the v6.7.1 changelog (#4481), as reported by `scripts/generate-changelog.sh release/v6.6 release/v6.7`. New in v6.7.2: - #4520 Backport `release/v6.7`: Export known peer address count as a Prometheus gauge - #4515 Backport `release/v6.7`: Count only connected peers in /net_info n_peers - #4508 Backport `release/v6.7`: Fix pruning issue in SS causing huge disk spike - #4507 Backport `release/v6.7`: Initialize app from last header when a frozen node restarts Shipped in v6.7.1, but merged after #4481 was generated, so the v6.7.1 changelog never listed them: - #4503 Backport `release/v6.7`: Refuse pinned write modes that disagree with persisted migration - #4502 Backport `release/v6.7`: Guard FlatKV rollback floor - #4499 Backport `release/v6.7`: Resolve composite iteration duplicates to memiavl, matching Read - #4489 Backport `release/v6.7`: Remove genesis export - #4486 Backport `release/v6.7`: Re-derive the composite write mode on rollback - #4485 Backport `release/v6.7`: Make the composite store flatKV and write mode atomic - #4474 Backport `release/v6.7`: Validate module names in the memiavl snapshot importer v6.7.1 release bookkeeping: - #4506 Backport `release/v6.7`: Update v6.7 change log in prep to cut v6.7.1 patch - #4482 Bump version to v6.7.1 in prep for patch release Only the `## v6.7` PR list changes, so the `backport release/v6.7` cherry-pick applies cleanly (simulated with `git merge-tree` against `origin/release/v6.7`). Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Backport of #4484 to
release/v6.7.