feat(seidb): rollback the state store from a snapshot plus the changelog - #3953
feat(seidb): rollback the state store from a snapshot plus the changelog#3953blindchaser wants to merge 3 commits into
Conversation
…gelog seid rollback rewound Tendermint and state-commit but left the state store where it was, so an operator had to rebuild SS from state sync to recover a height. The state store now rolls back with them: the newest snapshot at or below the target is republished as the live database and the changelog is replayed forward onto it. Retention is what makes that reachable. Changelog pruning is anchored to the oldest retained snapshot instead of a fixed entry count, so every retained snapshot stays replayable forward, at a cost of roughly one snapshot interval of changelog per retained snapshot. The directory swap is ordered so that a crash between any two steps leaves the changelog inside exactly one directory, and recovery empties both staging directories into the live one before deleting either: the changelog is the only copy of the versions above the snapshots. A rollback that dies partway is finished on the next open through a marker file. evm-ss-split topologies are refused, since recovery replays only the Cosmos changelog. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PR SummaryHigh Risk Overview
Composite SS rollback ( Changelog retention is tied to snapshots: higher WAL Reviewed by Cursor Bugbot for commit d305f39. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3953 +/- ##
==========================================
- Coverage 59.69% 58.63% -1.07%
==========================================
Files 2333 2238 -95
Lines 200172 190033 -10139
==========================================
- Hits 119490 111419 -8071
+ Misses 69277 67929 -1348
+ Partials 11405 10685 -720
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A well-structured, carefully documented SS rollback implementation with strong crash-recovery test coverage, but three issues block: seid rollback now hard-fails on the default configuration (SS snapshots are off by default), the directory swap is not crash-safe on legacy-layout nodes because GetStateStorePath re-resolves to a different path while the live DB is moved aside, and the rollback marker is never fsynced so a crash can publish a restored DB without it.
Findings: 3 blocking | 5 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The commit store is rolled back before the state store (
rootmulti/store.go), so a clean SSRollbackerror afterscStore.Rollbacksucceeds leaves SC at the target with SS ahead. A crash converges via the marker, but a returned error does not — which contradicts the PR description's "the two layers can only end on the same height or on the height they started from". Worth either stating the weaker guarantee in the doc comment or attempting an SC re-forward/abort path. - No test exercises the legacy
data/{backend}state-store layout. Every rollback and crash-recovery test uses a fresht.TempDir(), which always resolves to the newdata/state_store/cosmos/{backend}path, so the path-resolution hazard in the directory swap is invisible to the suite. A crash-recovery case seeded with a legacydata/pebbledbdirectory would pin it. RollbackcallsclearRollbackTarget(s.dbHome)after the reopen, butNewCompositeStateStorealready cleared the marker on thehasPendingRollbackpath, so this call is always a no-opos.Removeon a missing file. Harmless, but it reads as a second, independent guarantee that isn't one.- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| return fmt.Errorf("state store %T does not support rollback", rs.ssStore) | ||
| } | ||
| if validator, ok := rs.ssStore.(seidbtypes.RollbackValidator); ok { | ||
| if err := validator.ValidateRollback(target); err != nil { |
There was a problem hiding this comment.
[blocker] This makes seid rollback fail outright on the default node configuration.
DefaultStateStoreConfig() sets SnapshotEnable: false, and AlignSSSnapshotWithSC then zeroes SnapshotInterval/SnapshotKeepRecent. With no SS snapshots on disk, planRollback → rollbackBaseVersion returns "cannot roll back state store to version %d: no snapshot at or below target", ValidateRollback propagates it here, and RollbackToVersion returns before scStore.Rollback ever runs. NewStateStore always returns a *CompositeStateStore, so the Rollbackable assertion always succeeds and there is no path that skips this check.
The same applies to a rocksdb backend build (planRollback requires config.PebbleDBBackend) and to evm-ss-split=true.
Before this PR, seid rollback on those configurations rolled back SC + Tendermint and left SS ahead. After it, the operator recovery command documented in server/rollback.go ("recover from an incorrect application state transition ... unable to make progress") returns an error and does nothing. The evm-ss-split refusal is called out as deliberate in the PR description; the snapshots-disabled default is not.
Please either degrade to the previous behavior with a loud warning when SS cannot follow, or add an explicit opt-out flag on the rollback command so an operator can accept an SS that stays ahead.
| if ssConfig.DBDirectory != "" { | ||
| dbHome = ssConfig.DBDirectory | ||
| } | ||
| if err := recoverRollbackDirSwap(dbHome); err != nil { |
There was a problem hiding this comment.
[blocker] The directory swap is not crash-safe on a legacy-layout node, because dbHome is resolved by a helper whose answer depends on whether the directory currently exists.
utils.GetStateStorePath returns data/{backend} when that directory exists and data/state_store/cosmos/{backend} otherwise. restoreSteps step 4 renames dbHome to dbHome + "-rollback-old", so between step 4 and step 5 the legacy directory does not exist.
On an existing node using the legacy layout, a crash in that window means the next open resolves dbHome to <home>/data/state_store/cosmos/pebbledb (the legacy path is gone). recoverRollbackDirSwap then stats that new path, finds neither -rollback-tmp nor -rollback-old beside it, and returns nil; completePendingRollback reads the marker from the same wrong directory and finds none. The backend then opens a brand-new empty PebbleDB at the new-layout path, and both the staged restore and the entire pre-rollback database — including the changelog, which is the only copy of the versions above the snapshots — are orphaned under data/pebbledb-rollback-{tmp,old}. The node comes up with an empty state store and no error.
The crash tests can't catch this: setupRollbackStore uses t.TempDir(), which always resolves to the new layout.
Suggested fix: resolve dbHome once and pass it in rather than re-deriving it (or have recoverRollbackDirSwap also probe the legacy path's -rollback-tmp/-rollback-old siblings before accepting the new-layout resolution).
| return filepath.Join(dbHome, rollbackTargetFile) | ||
| } | ||
|
|
||
| func writeRollbackTarget(dbHome string, target int64) error { |
There was a problem hiding this comment.
[blocker] The rollback marker is never made durable, which defeats the resumability the marker exists for.
ClonePebbleDir fsyncs tmpDir at the end of the clone, and writeRollbackTarget runs after that, so neither the marker's contents nor tmpDir's directory entry for it is fsynced. Step 8 (persist directory swap) syncs only filepath.Dir(dbHome), which makes the two renames durable but not the file inside the renamed directory.
Failure: power loss after step 5 publishes the restored database. On reboot dbHome is the restored snapshot but readRollbackTarget returns false, so completePendingRollback performs no cut and RecoverCompositeStateStore replays the whole uncut changelog forward — the store lands back at the pre-rollback height while SC is at the target, silently, with no error. That is exactly the divergence this PR sets out to remove, and the crash tests can't see it because they stop the process cleanly rather than losing the page cache.
Fix: write the marker via a temp file + Sync() + rename, then utils.SyncDir(tmpDir) before step 4.
The mirror case at clearRollbackTarget (line 352) has the same gap in the other direction: os.Remove without a SyncDir(dbHome) can leave the marker resurrected after a crash, and the next open will then truncate the changelog back to the stale target, discarding entries for blocks committed since the rollback finished. A utils.SyncDir(dbHome) after the remove closes it.
| if lastOffset == 0 || firstOffset > lastOffset { | ||
| return nil | ||
| } | ||
| firstNeeded, err := wal.FindFirstOffsetAfterVersion(db.streamHandler, firstOffset, lastOffset, version) |
There was a problem hiding this comment.
[suggestion] Pruning to the first entry above the snapshot destroys the evidence that rollbackBaseVersion relies on.
After this prune, firstOffset is the first entry with version > version. In rollbackBaseVersion the guard at rollback.go:162 accepts a gap only when firstNeeded > firstOffset — i.e. only when a retained entry sits at or below the snapshot proves the prefix wasn't pruned. Once the WAL has been anchored here, that proof is gone for the oldest retained snapshot.
So if the block immediately after the oldest retained snapshot wrote no changesets, the first retained entry is oldest+2, and every rollback whose base snapshot is oldest is refused with "the changelog starts at version N, above snapshot M, so the versions in between may have been pruned" — even though nothing was actually lost. The failure is conservative rather than corrupting, and ValidateRollback refuses before SC moves, but it silently removes the oldest snapshot from the usable rollback window.
Keeping one entry at or below the snapshot fixes it exactly: use wal.FindLastOffsetAtOrBeforeVersion(...) (already added in this PR) and truncate before that offset instead.
| return base, nil | ||
| } | ||
|
|
||
| stream, err := wal.NewChangelogWAL(changelogPath, wal.Config{}) |
There was a problem hiding this comment.
[suggestion] planRollback runs before s.Close(), so this opens a second wal.Log over a changelog directory the live store still holds open. Two concerns:
ValidateRollbackis documented onRollbackValidatoras checking "without changing store state", butwal.opentruncates a corrupted tail onwal.ErrCorrupt— a repair, not a read.- The snapshot and pruning managers are still running at this point (
Closeis what stops them), andManager.prune→pruneWALToOldestSnapshot→PruneWALBeforeVersioncallsTruncateBeforeon the store's own handle. A concurrent truncation under a second reader on the same segments is not something tidwall/wal guards against.
The offline seid rollback flow makes this unlikely to bite in practice, but closing the store (or reading the first replayable entry through the already-open handle) before opening a second one would remove the hazard entirely.
Planning ran before the store closed, so it opened a second WAL handle over a directory the live store still held. That open repairs a corrupt tail, which ValidateRollback promises not to do, and it reads segments snapshot retention can truncate underneath it. The engine answers the same question through the handle it already holds, where reads and truncations are serialized. Co-authored-by: Cursor <cursoragent@cursor.com>
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.
Reviewed by Cursor Bugbot for commit d305f39. Configure here.
|
|
||
| func writeRollbackTarget(dbHome string, target int64) error { | ||
| return os.WriteFile(rollbackTargetPath(dbHome), []byte(strconv.FormatInt(target, 10)+"\n"), 0o600) | ||
| } |
There was a problem hiding this comment.
Rollback marker is never fsynced
High Severity
The .ss-rollback-target file is written with os.WriteFile after ClonePebbleDir already synced the temp directory, and neither the file nor that directory is synced again. A crash after the restored database is published can leave a missing or truncated marker. Open then skips the changelog cut, replays the full WAL onto the snapshot, and can bring SS back to the pre-rollback height.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d305f39. Configure here.
1 participant
Summary
Make
seid rollbackrewind the State Store (SS) with Tendermint and state-commit, instead of leaving SS ahead. A rollback to heightTrestores the newest snapshot at or belowTinto the live database and replays the changelog forward onto it, so it is an offline, single-command operation and needs no resync. Two optional capabilities carry it:Rollbackableperforms the rewind andRollbackValidatoranswers whether it can be performed without touching state. Retention changes with it: the changelog is now anchored to the oldest retained snapshot, so every retained snapshot stays replayable forward.evm-ss-split=trueis refused up front rather than half-supported.sei-db/db_engine/types/types.go: define the capability contract —Rollbackable,RollbackValidator, andSnapshotWALPruner. All three are optional, so a store that does not implement them is a caller decision rather than a silent degradation.sei-cosmos/storev2/rootmulti/store.go:RollbackToVersionsettles both SS capabilities beforescStore.Rollback. A state store that is notRollbackableis refused there, andValidateRollbackruns there, so the commit store is never rolled back into a height the state store cannot reach. The two layers can only end on the same height or on the height they started from.sei-db/state_db/ss/composite/rollback.go: the rollback itself.planRollbackrejects a target above the store version, at or below the oldest snapshot, on a non-Pebble backend, or underevm-ss-split, and picks the base snapshot.rollbackBaseVersionrefuses a base the changelog cannot reach: an empty changelog, no entry above the snapshot, or a changelog that starts inside the gap after the snapshot. A gap the changelog spans is accepted, because retention prunes a prefix only, so a retained entry below the first entry to replay proves the gap is blocks that wrote nothing.sei-db/state_db/ss/composite/rollback.go: the directory swap is crash-safe.restoreStepsnames the eight steps in order, and the order is the guarantee: the changelog is the only copy of every version the snapshot does not hold, so it stays inside the live directory until that directory is moved aside whole, and it moves into the published directory before the moved-aside one is deleted. Between any two steps it is inside exactly one of the two directories.recoverRollbackDirSwapruns before the store opens, drains the changelog out of both directories, and only then deletes them; it publishes a staged restore or puts the live database back, depending on which one survived.sei-db/state_db/ss/composite/rollback.go: a.ss-rollback-targetmarker makes the rewind resumable. It is written inside the restored copy before that copy is published, so it appears with the restored database and never without it. On the next open the changelog cut runs again, the version watermark advances to the target, and the marker is cleared. A changelog left with no entry at or below the target is deleted rather than emptied in place: the WAL only empties a log opened withAllowEmpty, and a log emptied that way is refused by every opener that does not set the flag, which is every opener of this changelog.sei-db/state_db/ss/composite/store.go:CompositeStateStoremoves its members into an embeddedcompositeState, so the reopened store is adopted in one assignment and a field added later travels with it. The pending-rollback cut runs before the backend opens, so it cannot compete with a second handle on the same changelog directory.sei-db/state_db/ss/snapshot/manager.go,sei-db/state_db/ss/cosmos/store.go,sei-db/db_engine/pebbledb/mvcc/db.go: after every retention pass, snapshot retention prunes the changelog to the first entry needed to replay forward from the oldest retained snapshot. Pruning is best-effort — a failure is logged and the snapshots stay valid, only wider than rollback can use.sei-db/db_engine/pebbledb/mvcc/db.go,sei-db/config/ss_config.go: count-based WAL pruning keeps a ceiling instead of switching off.snapshotWALKeepRecentraises the floor to the widest span a rollback can ask for, which covers the cases the snapshot-anchored pass does not reach: external snapshot pruning, and the stretch before enough snapshots exist to prune. With the defaults that is 40,000 entries against the previous floor of 1,000. The disk cost is recorded where an operator sizing a node will find it.sei-db/wal/changelog.go:FindFirstOffsetAfterVersionandFindLastOffsetAtOrBeforeVersionsearch entry versions rather than assuming offset equals version, because an empty block advances the version without writing an entry. The composite replay path now uses the shared search instead of its own copy.sei-db/common/utils/clone.go:ClonePebbleDirhardlinks.sstfiles, copies the rest, skipsLOCK, and fsyncs the destination directory. A subdirectory is an error rather than a skip, so a configured WAL directory inside the database cannot make a clone silently incomplete.SyncDirpersists directory entries after the clone, the publish swap, and the changelog reset.Test plan
sei-db/state_db/ss/composite/rollback_test.go: rollback restores the snapshot and replays the changelog to the target, with the versions above it gone and the store writable afterwards; a target equal to the oldest retained snapshot, where every retained changelog entry is above the target, writes a new block afterwards and reopens.sei-db/state_db/ss/composite/rollback_test.go: a rollback across a block that wrote no changesets succeeds — the changelog has a gap the retained prefix proves is empty blocks.sei-db/state_db/ss/composite/rollback_test.go:ValidateRollbackandRollbackagree across six targets, which is the invariant the pre-flight exists for.sei-db/state_db/ss/composite/rollback_test.go: crash recovery after every one of the eight swap steps, driven from the same step list the production path runs, so a step added later cannot escape the test. Each case asserts the changelog survives and the store reopens on a consistent height. Two further cases cover a crash after the restore but before the changelog cut, and after the cut but before the reopen.sei-db/state_db/ss/composite/rollback_test.go: refused targets — below the first snapshot, above the store version, zero, a changelog cut below the target, a changelog pruned past the target, andevm-ss-split.sei-db/state_db/ss/composite/rollback_test.go: retention prunes the changelog to the oldest retained snapshot, and no further.sei-cosmos/storev2/rootmulti/flatkv_recovery_test.go:RollbackToVersionrolls the state store back with the commit store, and refuses a state store withoutRollbackbefore the commit store moves.go test -raceonsei-db/state_db/ss/...andsei-cosmos/storev2/rootmulti;golangci-lint,gofmt -s, andgoimportsare clean on all changed files.