Fix memiavl snapshot race condition

[yzang2019]

Summary

Tree.Copy() shares nodes with the live tree but did not freeze them. A reader of the copy and a writer on the live tree therefore share MemNode memory with no lock in common, and a torn read of MemNode.hash is enough to produce a leaves file that is 32 bytes short of a whole number of records. This PR makes Copy() establish the freeze before handing the tree over.

The production crash that motivated the work is:

corrupted snapshot, leaves file size 63201088 is not a multiple of 48

The byte pattern matches that torn-hash write. This PR does not claim to have found the production trigger, and should not be treated as a confirmed fix for that incident. On the affected node's config, every concrete path we could enumerate is closed; see "Scope" below.

The problem

Snapshots are written in the background so they don't block the chain. To do that safely, Tree.Copy() hands the writer its own view of the tree. The two views share the same underlying nodes in memory, and the deal is: the copy is frozen, so the live chain will never touch a node the writer is still reading.

Copy() was not keeping that deal. Three separate reasons:

• The freeze line was off by one version. Writes stamp new nodes at version + 1, but Copy() set its protection floor at version, leaving any node written during the current block unprotected. DB.Copy / CommitStore.Copy reach this state: ApplyChangeSets releases db.mtx on return, so a copy taken before the following Commit is mid-version.
• SaveVersion() updated the version with no lock held, so a concurrent copy could read the new version and compute a floor that protected nothing.
• Nodes weren't guaranteed to be hashed before being handed over, so a reader would compute and cache hashes in place on nodes the live tree also held.

Each leaf is written as a 16-byte header followed by its 32-byte hash. writeLeaf sizes that copy with len(hash). A torn read of the slice header — pointer set, length still zero — yields a zero-length hash, so the writer emits the header and nothing else. That is a leaves file with remainder 16, which is the observed 63201088 % 48 == 16.

Why one mutex per copy is the underlying hole

RootHash and the proof builders take the write lock because MemNode.Hash fills the hash cache in place — that was the fix for Immunefi 83246. But Copy() gives each tree its own mutex, so that lock only serializes hash fills within a single tree instance. Two trees sharing nodes serialize against nothing, which puts the same unsynchronized mutation right back. Freezing the nodes before handing them over is what actually closes it.

Changes

• Copy() now takes the write lock, hashes everything reachable before handing the tree over, and sets the protection floor to the highest version actually present in the tree instead of the last saved version.
• SaveVersion() is now a single locked section, so the version bump and the hashing can't be observed apart.
• The snapshot writer reads the root and the version together, so a snapshot's metadata can't claim a version its contents don't match.
• Separate bug, also fixed: a copy inherited the live tree's background-write channel, so calling Close() on any copy shut down the live tree's background writer and made the next async write panic on a closed channel. Copies now get their own.

There is no behavior change outside the race — no existing test needed editing.

Notes for reviewers

• Performance: the added hashing in Copy() is free on the rewrite path, since the commit already hashed everything and the call short-circuits. Only a mid-block copy does real work, and that's work the next SaveVersion would have done moments later anyway. The tighter freeze line avoids extra node copies rather than adding them.
• Relationship to Validate snapshots before publication #4036: complementary, not redundant. That PR catches a corrupt snapshot before publishing it; this one stops the known API-reachable way of producing one. Worth having both: the validator is the backstop if another path to this kind of mutation turns up, and it is the only thing that would have turned this incident into a failed rewrite instead of a published-and-unbootable snapshot.
• Still open: the production trigger. The mechanism (torn node.hash at the write site → remainder 16) is exact. A production path that writes node.hash on a node the rewrite is traversing has not been found on this node's config.

[cursor]

PR Summary

High Risk
Changes locking, copy-on-write versioning, and snapshot serialization in the state-commitment path; mistakes could affect AppHash, proofs, or published snapshots at restart.

Overview
Fixes a concurrency bug where Tree.Copy() shared MemNodes with the live tree without freezing them, so background snapshot writes and readers on a copy could race live commits and corrupt on-disk leaves (e.g. file size not a multiple of 48) or drift root hashes and proofs.

Copy() now takes the write lock, pre-hashes all reachable nodes via rootHashNoLock, and sets cowVersion to maxNodeVersion() (using new unsavedWrites) instead of last saved version, so mid-block copies that stamp nodes at version+1 are protected. SaveVersion is one locked section (hash then bump version). Snapshot WriteSnapshotWithRateLimit reads root and version together through snapshotSource(). Copies no longer share the live tree’s pendingChanges / pendingWg.

Adds regression tests for the freeze invariant, stability under live commits, on-disk snapshot correctness, background-write channel isolation, and -race coverage for copy reads vs commits and concurrent snapshot writes.

^{Reviewed by Cursor Bugbot for commit 3f67c52. Bugbot is set up for automated code reviews on this repo. Configure here.}

[github-actions]

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
✅ passed	✅ passed	✅ passed	✅ passed	Aug 27, 2026, 9:44 PM

[seidroid]

The fix is sound: taking the write lock in Copy/SaveVersion, hashing the whole reachable set before handing nodes to a reader, raising the COW floor to version+1 when unsaved writes exist, and detaching the copy from the live tree's background-write channel together close the real races (including the pre-existing t.cowVersion write under a read lock and cloned.Close() closing the live tree's pendingChanges). Only two non-blocking notes, both on the new regression test.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• None at the file/PR level.
• 2 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] Tree.Version(), Tree.IsEmpty(), Tree.SetZeroCopy() and Tree.Export() (sei-db/state_db/sc/memiavl/tree.go) read t.version/t.root/t.snapshot without taking t.mtx, while SaveVersion/Set/Remove/ReplaceWith now write them under the write lock. MultiTree.buildCommitInfo calls entry.Version() on this unlocked path. It is currently serialized by db.mtx in practice, but it leaves the last unsynchronized reads of the fields this PR otherwise locks down.
• [suggestion] Tree.StartBackgroundWrite/WaitToCompleteAsyncWrite (sei-db/state_db/sc/memiavl/tree.go:187,199) assign and nil t.pendingChanges without holding t.mtx, while Close and now copyNoLock read that field under the lock — an unsynchronized field access that the race detector can flag if catchup ever overlaps a copy.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.51%. Comparing base (7cf5a21) to head (3f67c52).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4042      +/-   ##
==========================================
- Coverage   61.25%   60.51%   -0.74%     
==========================================
  Files        2153     2073      -80     
  Lines      188426   179766    -8660     
==========================================
- Hits       115414   108783    -6631     
+ Misses      62274    60971    -1303     
+ Partials    10738    10012     -726     

Flag	Coverage Δ
sei-db	69.80% <ø> (ø)
sei-db-state-db	?
sei-db-state-db-pr	75.73% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
sei-db/state_db/sc/memiavl/snapshot.go	65.61% <100.00%> (+0.10%)	⬆️
sei-db/state_db/sc/memiavl/tree.go	88.88% <100.00%> (+0.90%)	⬆️

... and 114 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[blindchaser]

could this increase EndBlock latency? Trace snapshots call Copy() before commit, so this now hashes all dirty nodes while holding the write lock

[yzang2019]

Not a concern. Trace capture runs in EVM EndBlock, and that is before the current block is flushed into memiavl. So SnapshotSCStore → Copy() is hashing last-committed trees, not this block’s dirty nodes. rootHashNoLock() therefore stops at the root: one nil-check per tree, not a walk. MultiTree.Copy does that for each of the ~20 stores, under that tree’s write lock. The added EndBlock cost is lock acquire + an O(1) hash hit + the same shallow struct copy as before.

[yzang2019]

@seidroid review

[seidroid]

The fix is sound: hashing the reachable set and raising the COW floor to the highest node version under the write lock, making SaveVersion one locked section, reading root+version as a pair in the snapshot writer, and detaching the copy from the live tree's background-write plumbing together close the cross-copy hole. Both previously unresolved review threads are addressed; the only remaining notes are documentation accuracy around the new hashing cost and the maxNodeVersion contract.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• None at the file/PR level.
• 2 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] Tree.Version(), IsEmpty(), SetZeroCopy() and Export() (sei-db/state_db/sc/memiavl/tree.go:97-103, 250, 380) read t.version/t.root/t.snapshot without taking t.mtx, while SaveVersion/Set/Remove/ReplaceWith/Copy now write them under the write lock. MultiTree.buildCommitInfo reaches entry.Version() on this unlocked path; it is serialized by db.mtx in practice, but these are the last unsynchronized reads of the fields this PR otherwise locks down.
• [suggestion] Tree.StartBackgroundWrite/WaitToCompleteAsyncWrite (sei-db/state_db/sc/memiavl/tree.go:187, 199) assign and nil t.pendingChanges and call t.pendingWg.Add without holding t.mtx, while Close and copyNoLock read those fields under the lock — an unsynchronized field access the race detector can flag if catchup ever overlaps a copy.

[seidroid]

[suggestion] Copy is no longer O(1), and two callers still advertise that it is. memiavl.CommitStore.Copy (store.go:117, "returns an O(1) memiavl snapshot") and rootmulti.Store.SnapshotSCStore (sei-cosmos/storev2/rootmulti/store.go:483, "returns an O(1) SC snapshot") both reach here.

The mid-version case is the live one: rootmulti.Store.Commit runs rs.flush() (→ ApplyChangeSets) before taking rs.mtx, and SnapshotSCStore only takes rs.mtx.RLock(), so the per-block traceSnapshotCapture in EVM EndBlock can land on a tree with the whole block's dirty set unhashed. Total CPU is unchanged — GetWorkingHash/Commit would hash the same nodes moments later — but it now happens under the tree's write lock, so concurrent Get/Iterator readers block on it. Worth correcting those two doc comments so the cost isn't misread as free at the call site.

[seidroid]

[suggestion] The godoc claims equality ("the highest version carried by any node") but the function returns an upper bound. With an initial version, nextVersionU32(0, initialVersion) jumps t.version from 0 to initialVersion while the nodes written in that first block carry version 1, so maxNodeVersion() returns initialVersion and no node is anywhere near it.

That's harmless here — over-freezing is the safe direction, and the property Copy actually needs is "≥ every node's version". But stating it as an exact maximum invites a later reader to reuse it where equality matters (e.g. deriving the version snapshotSource records) or to "tighten" it back down. Documenting the bound rather than the maximum keeps the safety argument the one the code relies on.

[masih]

I will land the corruption check PR after this one lands, thanks @yzang2019