Validate snapshots before publication - #4036
Conversation
Reject malformed node hashes before writing snapshot records and validate completed snapshots before updating the current symlink. Add regression coverage for partial leaf records and corrupt snapshot publication.
PR SummaryHigh Risk Overview Rewrite / publish flow: Load path: Metadata and mmap layout checks ( Write path: Snapshot writers reject leaf/branch hashes that are not 32 bytes. Regression tests cover corrupt publication, skewed stores, dangling Reviewed by Cursor Bugbot for commit 1c45d68. 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 #4036 +/- ##
==========================================
- Coverage 61.26% 60.55% -0.72%
==========================================
Files 2153 2073 -80
Lines 188485 179854 -8631
==========================================
- Hits 115477 108907 -6570
+ Misses 62265 60930 -1335
+ Partials 10743 10017 -726
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
masih
added
backport release/v6.6
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a structural validation pass before a snapshot is published via the current symlink, plus hash-length guards in the snapshot writer — a solid defensive change with reasonable regression coverage. No blockers; the notes below concern how far the validation actually goes and how an already-corrupt snapshot directory is recovered from.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new tests cover only the temp-candidate path (
publishSnapshoton a corrupted tmp dir) and the two hash-length guards. Neither of the branches that adopt an already-existingsnapshot-<h>directory — the early skip inRewriteSnapshot(db.go:774) and the rename-conflict reuse inpublishSnapshot(db.go:823) — is exercised. Those are the paths where validation changes behaviour from "return nil" to "return error", so they are the ones most worth pinning. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
LoadMultiTree(sei-db/state_db/sc/memiavl/multitree.go:103) returns immediately whenOpenSnapshotfails for one store, without closing the snapshots already opened for earlier stores.OpenSnapshotcleans up its own partial handles, but the completed ones intreeMapare leaked (mmaps + fds) for the process lifetime. This existed before the PR, butvalidateSnapshotnow routes a recurring background operation through that error path, so a persistently corrupt multi-store snapshot leaks a mapping set per rewrite attempt rather than once at startup.
| func (db *DB) validateSnapshot(ctx context.Context, path string) error { | ||
| opts := db.opts | ||
| opts.SnapshotPrefetchThreshold = 0 | ||
| mtree, err := LoadMultiTree(ctx, path, opts) |
There was a problem hiding this comment.
LoadMultiTree leaks mmaps and fds on partial failure. multitree.go:103 returns nil, err without closing the snapshots it already opened for earlier stores. This is a pre-existing issue, but this PR makes it newly reachable: every validation failure now leaks fds
| if !ok { | ||
| // channel was closed without sending a result | ||
| // Still prune old snapshots to prevent accumulation | ||
| go db.pruneSnapshots() |
There was a problem hiding this comment.
I think we added that because there were edge causes causing snapshot not being pruned properly and getting accumulated, causing disk fililng up
There was a problem hiding this comment.
In the latest main, I think we already made the change so that we will make sure to keep at least 2 memIAVL snapshots
|
@seidroid review |
There was a problem hiding this comment.
The PR closes both notes from the previous review — validateSnapshot now compares version and per-store root hashes against lastCommitInfo, the adoption branches are covered by tests, and the LoadMultiTree handle leak is fixed. Two problems remain in the new recovery path: any validation failure (including a cancelled context at shutdown) now deletes an existing snapshot directory, and an unparseable-but-protobuf-valid __metadata panics instead of failing validation.
Findings: 2 blocking | 1 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
validateSnapshotcompares the multi-tree metadata version but not each loaded store's own snapshot version (entry.Version()). A directory whose__metadatasays version N while a store subdirectory carries an older version with an unchanged root hash would validate clean, and the resulting per-tree version skew would corrupt subsequent version progression. The snapshot writer always advances every tree together, so this is only reachable through external tampering or filesystem-level mixing — but the check is a one-line addition to the loop that already walksmtree.Trees().
|
@seidroid review |
There was a problem hiding this comment.
The PR closes both blockers from the previous round — the deletion path is now gated on errCorruptedSnapshot and readMetadata rejects nil CommitInfo — but the new self-heal in RewriteSnapshot deletes an existing (possibly current-referenced) snapshot before its replacement exists, and the nil-CommitInfo guard leaves an adjacent negative-InitialVersion panic reachable from the same commit path.
Findings: 3 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
- 1 critical pre-existing blocker(s) listed below under pre-existing issues.
Non-blocking
- [suggestion]
errCorruptedSnapshotis now load-bearing — it decides between "delete and regenerate" and "halt the node" — but it is applied ad hoc across three files, and some structural errors on the same load path are still untagged:NewNodes/NewLeavesreturn bareerrors.New("input buffer is not aligned")/"input buffer length is not correct"(layout_native.go:30, 34, 85, 89) and reachvalidateSnapshotas environmental failures. Worth one sweep overOpenSnapshot/readMetadata/NewNodes/NewLeavesso every "this data is bad" error carries the sentinel, and a note on the sentinel's declaration that new structural checks must join it. - [suggestion] The non-corruption branch in
publishSnapshot(db.go:845-851) — an existing target whose validation fails for an environmental reason must be kept, not deleted — has no test, while the equivalent branch inRewriteSnapshotdoes (TestRewriteSnapshotKeepsSnapshotOnCancelledValidation). Since the two removals are duplicated logic, pinning only one leaves the other free to drift. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
Pre-existing issues
- [blocker]
createDBIfNotExist(sei-db/state_db/sc/memiavl/db.go:1412) statscurrent/__metadataand, on anyIsNotExist, callsinitEmptyDB, which writes a fresh emptysnapshot-0and repointscurrentat it. A danglingcurrentsymlink is indistinguishable from a never-initialized directory, so a node whose snapshot target went missing silently comes up with an empty state-commitment store at version 0 instead of failing to start. Production usesCreateIfMissing: true(store.go:29). Distinguishing "nocurrentlink at all" from "link present but target missing" would turn this into a loud startup error.
![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.
Reviewed by Cursor Bugbot for commit 1c45d68. Configure here.
| logger.Debug("temporary snapshot directory cleaned up successfully", | ||
| "tmpDir", tmpDir, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Replace path can drop snapshot
Medium Severity
Leaving a corrupted snapshot in place until rewrite now routes replacement through publishSnapshot, which still RemoveAlls the target before renaming the validated temp. If that rename fails, cleanupFailedSnapshotRewrite deletes the temp too. When current pointed at the removed directory, the new dangling-link check then refuses to open the db, so the node cannot start.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 1c45d68. Configure here.
|
@seidroid review |
There was a problem hiding this comment.
All three unresolved findings from the previous round are genuinely closed: the premature deletion in RewriteSnapshot is gone and pinned by a test, readMetadata now rejects negative InitialVersion, and the structural errors on the load path all carry errCorruptedSnapshot. One narrowing remains in publishSnapshot's corrupt-target replacement ordering, plus two pre-existing gaps in paths that publish current without validation.
Findings: 0 blocking | 3 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.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
RewriteSnapshot's adopt-and-skip branch (sei-db/state_db/sc/memiavl/db.go:789-793) returns nil without callingupdateCurrentSymlink, while the equivalent adoption branch inpublishSnapshot(db.go:867-873) does update it. If a prior rewrite renamed the temp intosnapshot-<V>but died before the symlink update, a retry at the same version validates the directory, skips, and leavescurrenton the older snapshot — and the background goroutine then reloads that older snapshot fromcurrentPath. Impact is limited because the next rewrite triggers at a different version, but the two adoption branches now sit side by side with different endings. - [suggestion]
MultiTreeImporter.Close(sei-db/state_db/sc/memiavl/import.go:125-148) adopts a pre-existingsnapshot-<h>directory and then publishes it viaupdateCurrentSymlink, with no validation. That is the same "adopt a directory this call did not write" shape the PR addedvalidateSnapshotfor on the rewrite path, so the state-sync restore path still updatescurrentto point at an unverified directory. Routing it through an equivalent check would close the remaining unvalidated publication path.
| "path", targetPath, | ||
| "error", validationErr, | ||
| ) | ||
| if rmErr := os.RemoveAll(targetPath); rmErr != nil { |
There was a problem hiding this comment.
[suggestion] The corrupt target is removed before the validated temp takes its place, so targetPath does not exist for the duration of the RemoveAll — which, for a mainnet-sized store, is the time it takes to unlink every file in the snapshot. If the process dies in that window, or the os.Rename on line 862 fails, nothing remains at targetPath: the caller's cleanupFailedSnapshotRewrite then drops the validated temp as well, and removeTmpDirs would have cleared it at the next startup regardless. When current pointed at targetPath, it is left dangling, which createDBIfNotExist now turns into a hard startup failure requiring manual restore.
This is narrower than it looks — the branch only runs when the target already failed validation as corrupt, and on the base branch that same directory was silently adopted and published, so the PR is still a clear improvement here. But the atomic ordering is nearly free: rename the corrupt directory aside first, rename the temp into place, then remove the quarantined copy. That reduces the exposure to the gap between two instantaneous same-directory renames. Note that atomicRemoveDir cannot be reused as-is, since its path + "-tmp" quarantine name collides with path; a distinct -tmp-suffixed name still gets cleaned up by removeTmpDirs on the next open.
Superseded: latest AI review found no blocking issues.
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-4036-to-release/v6.6
git worktree add --checkout .worktree/backport-4036-to-release/v6.6 backport-4036-to-release/v6.6
cd .worktree/backport-4036-to-release/v6.6
git reset --hard HEAD^
git cherry-pick -x f28da4c23213808d751969e99a2ee3ccf41f8473
git push --force-with-lease |
|
Successfully created backport PR for |
2 participants
Reject malformed node hashes before writing snapshot records and validate
completed snapshots before updating the current symlink. Add regression
coverage for partial leaf records and corrupt snapshot publication.