fix(seidb): stop evm-logical-digest replay mode from mutating a live memiavl - #3840
fix(seidb): stop evm-logical-digest replay mode from mutating a live memiavl#3840blindchaser wants to merge 2 commits into
Conversation
…memiavl --memiavl-open-mode=replay relied on memiavl's ReadOnly option, which only skips the LOCK file. The changelog is still opened read-write, and sei-db's wal.open() answers wal.ErrCorrupt with os.Truncate on the node's segment. On a live node a torn tail is the writer mid-append, not corruption. Truncating it drops committed versions, and since the writer's fd keeps its old offset, its next append leaves a zero-filled hole that binary.Uvarint reads as valid empty records rather than an error. The node keeps producing blocks and finds out at its next restart. Clone instead, as openFlatKVReadOnly already does for the other backend: hardlink the snapshot, byte-copy the changelog, verify it still covers snapshotVersion+1, retry if the writer prunes mid-clone, then open the clone normally. Every source access is now a read. Also reject a clone that falls short of --height, since a tail repair costs the trailing version and a silently early digest looks like real divergence; and export memiavl's SeekSnapshotDir rather than reimplementing its snapshot layout in the tool. Replay mode now needs a writable dbDir and room for the changelog copy, as the FlatKV path already did. memiavl.OpenDB(ReadOnly) is still reachable from wasm snapshot export and storev2/rootmulti proof queries; fixing it at the WAL layer is tracked separately. Co-authored-by: Cursor <[email protected]>
66d9e50 to
c6308d5
Compare
PR SummaryMedium Risk Overview Shared memiavl exports Reviewed by Cursor Bugbot for commit 87b886c. 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 #3840 +/- ##
==========================================
- Coverage 61.29% 60.64% -0.65%
==========================================
Files 2351 2280 -71
Lines 197369 189948 -7421
==========================================
- Hits 120982 115202 -5780
+ Misses 65535 64496 -1039
+ Partials 10852 10250 -602
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66d9e50150
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // our changelog copy, then prune the changelog up to that newer | ||
| // version — leaving a copy that no longer covers snapshotVersion+1 and | ||
| // a catchup that would silently skip versions. Retryable. | ||
| if err := verifyClonedWALCovers(dstChangelogDir, snapshotVersion); err != nil { |
There was a problem hiding this comment.
When memiavl has been initialized with a height greater than 1, its initial snapshot is still snapshot-0, while the first changelog entry uses the configured initial height (for example, SetInitialVersion(100) produces WAL version 100). Passing that clone to verifyClonedWALCovers treats the valid first entry as a gap because it requires firstVer <= snapshotVersion+1, so replay mode retries three times and fails until a later snapshot is written. Derive the expected successor from the snapshot's stored initial-version metadata rather than assuming version 1 follows snapshot 0.
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any additional bugs, but this PR reworks memiavl's tooling replay path with several subtle concurrency/correctness invariants (hardlink-vs-copy cloning, WAL-coverage validation, retry-on-churn, shortfall rejection), so a human pass is worthwhile before merging.
What was reviewed:
- The new clone-and-replay path (
openMemiAVLReplay/tryPrepareMemIAVLToolingClone) and its reuse of the FlatKV tooling'sretryToolingClone/verifyClonedWALCovershelpers. - Confirmed the
--height 0case intentionally skips the shortfall check (no fixed target height to fall short of) — this matches a candidate issue the bug-hunting system already ruled out. - Traced clone cleanup (temp dir + lock release) on both the success and version-mismatch error paths in
openedMemIAVL.Close. - Checked that
SeekSnapshotDir's snapshot selection agrees withOpenDB's own internalseekSnapshotwhen it re-opens the clone, so the two don't disagree about which snapshot backs the replay.
Extended reasoning...
Overview
This PR reworks how the seidb tooling's evm-logical-digest --memiavl-open-mode replay" opens memiavl: instead of opening the live source directory with memiavl.Options{ReadOnly:true}(which still opens the changelog read-write and can truncate a torn tail out from under a live writer), it now clones the snapshot (hardlink) and changelog (byte copy) into a private temp directory underdbDirand replays there. It also exportsmemiavl.SeekSnapshotDir so the tool no longer reimplements memiavl's snapshot-selection logic, and factors the retry-on-churn logic (prepareFlatKVToolingCloneWith->retryToolingClone) so it's shared between the FlatKV and memiavl tooling clones. Two new files (memiavl_open.go/memiavl_open_test.go) and one exported function in sei-db/state_db/sc/memiavl/db.goare added;evm_logical_digest.gois updated to use the new*openedMemIAVL` wrapper.
Security risks
None of significance — this is offline/operator diagnostic tooling (seidb CLI), not a code path exercised during consensus, block execution, or RPC serving. The main risk class is data-safety (accidentally corrupting a live node's changelog while running a read-only diagnostic), which is precisely the bug this PR fixes, and it is covered by a dedicated regression test (TestOpenMemiAVLReplayLeavesSourceUntouched) that snapshots the source directory's file contents before/after and asserts byte-for-byte equality, plus a torn tail is deliberately left in place.
Level of scrutiny
Medium. The blast radius is limited to a tool operators run out-of-band against node data directories (not consensus-critical or app-hash-affecting, per the PR's own label), but the change involves nontrivial reasoning about concurrent writer races (hardlink vs. copy, WAL truncation mid-clone, retry semantics, snapshot-vs-changelog consistency) where a subtle mistake could reintroduce the exact class of bug being fixed, or introduce a new one only visible under a live-writer race. That combination — tool-scoped blast radius but concurrency-sensitive logic — puts this above "approve outright" but the existing tests target the specific hazard well.
Other factors
The PR body itself is unusually thorough (problem/fix/limits/tests sections) and explicitly scopes the fix to tooling only, noting the same ReadOnly weakness still exists in memiavl.OpenDB callers elsewhere (wasm snapshot export, storev2/rootmulti) and is tracked separately — that scoping honesty is a good sign. Three new tests exercise the specific hazard (source left untouched over a torn tail, opening while a live writer holds the LOCK, rejecting a clone short of the requested height and not leaving it behind). No CODEOWNERS or lint/build issues were apparent from the diff. The bug-hunting system found zero issues and one candidate (shortfall check skipped for --height 0) was independently confirmed to be correct-by-design rather than a bug.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-motivated fix: the replay path now clones the snapshot (hardlink) + changelog (byte-copy) instead of handing a live node's changelog to memiavl's WAL open, which truncates a torn tail. I verified the invariants it relies on (read-only snapshot mmap, rename-published snapshots, Version-bearing changelog entries, and Catchup silently falling short) all hold, so no blocking issues — only non-blocking notes around the unguarded --height 0 default, stale clone cleanup, and test coverage.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so it contributed nothing to this review. Codex explicitly reported no material issues. - Test coverage gap: both success-path tests use
height == 0, and the onlyheight > 0test asserts the rejection. There is no positive test that a non-zero--heightresolves an older snapshot viaSeekSnapshotDir'sseekSnapshotbranch and replays forward to exactly that height — i.e. the new strict equality check's success path and the non-currentsnapshot-selection branch are both untested. - No free-space pre-check before byte-copying the changelog. Because the clone must live inside
dbDirfor hardlinks to work, running the tool against a live node can fill that node's own data volume and halt it. The PR body notes callers "need room for the changelog copy," but nothing enforces or warns at runtime (same gap as the pre-existing FlatKV clone). - Behavior change worth surfacing to operators:
--memiavl-open-mode replaynow requires a writable--memiavl-dir(it creates a temp dir inside it). The flag help was updated to mention a "temp dir" but not that it is allocated inside the source directory, so a read-only-mounted data dir that used to work will now fail. Consider stating this in the flag help / package doc comment, not just the PR description. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| // when that falls short of the requested height. Every tail repair inside | ||
| // the clone costs the trailing version, and a digest computed one version | ||
| // early is indistinguishable from real divergence when comparing nodes. | ||
| if reached := db.Version(); height > 0 && reached != height { |
There was a problem hiding this comment.
[suggestion] The height > 0 guard leaves the flag's default (--height 0) with no coverage guarantee, even though the failure mode is identical. With --height 0, verifyClonedWALCovers opens the cloned WAL, tidwall repairs the torn tail by truncating it, and Catchup then replays to tip-1 and reports success — producing exactly the "digest one version early, indistinguishable from real divergence" outcome this check exists to prevent, on the most common invocation.
I acknowledge there's no cheap exact fix: learning the source's intended tip would mean opening the source WAL, which is the mutation this PR removes. But something is better than nothing — e.g. detect that the clone's WAL was repaired (compare the cloned changelog's byte size / last offset before and after verifyClonedWALCovers opens it) and either warn loudly or refuse for height == 0. At minimum, document in the doc comment above that height == 0 is best-effort so a reader doesn't infer the guarantee extends to it.
| } | ||
| // Not a "-tmp" suffix: memiavl's removeTmpDirs deletes every "*-tmp" | ||
| // directory under its root when a node opens the DB read-write. | ||
| tempDir, err := os.MkdirTemp(dbDir, ".seidb-memiavl-tool-*") |
There was a problem hiding this comment.
[suggestion] Nothing ever sweeps abandoned clones. If the tool is SIGKILLed (or the host reboots) between here and Close(), the .seidb-memiavl-tool-* directory survives inside the node's data dir holding hardlinks to the snapshot plus a full byte-copy of the changelog. The hardlinks mean the live node's snapshot pruning frees no space at all, and by design the name avoids the *-tmp suffix so memiavl's removeTmpDirs won't reclaim it on the next node restart either.
This is the same hazard the FlatKV clone already has, so it's not a regression — but this PR doubles the surface. Worth either a seidb subcommand / startup sweep for stale .seidb-*-tool-* dirs, or an explicit note in the flag help that operators must remove them manually after a crash.
| // The clone must sit inside dbDir to share a filesystem with the source | ||
| // snapshot: dbDir is often its own mount point, so a sibling directory is | ||
| // not enough and hardlinks would fail across the boundary. | ||
| if err := os.MkdirAll(dbDir, 0o750); err != nil { |
There was a problem hiding this comment.
[nit] This MkdirAll can never do useful work: memiavl.SeekSnapshotDir on line 85 already reads current / lists snapshots under dbDir, so it fails first if dbDir is missing. Carried over from tryPrepareFlatKVToolingClone, where it's equally moot. Dropping it (or moving it above the seek, if the intent was to create the dir) would remove a misleading suggestion that this path tolerates an absent source.
| // | ||
| // Exported for readers that need to resolve a snapshot without opening the DB, | ||
| // so they inherit this package's layout rules instead of restating them. | ||
| func SeekSnapshotDir(root string, targetVersion int64) (string, int64, error) { |
There was a problem hiding this comment.
[nit] Two small things on this newly exported API:
- The name says
Dirbut the first return value is a directory name relative toroot(callers mustfilepath.Join(root, name), astryPrepareMemIAVLToolingClonedoes). The doc comment says "directory name" correctly, butSeekSnapshotNamewould make the contract obvious at the call site. - No unit test accompanies the export — worth pinning at least the two branches (
targetVersion == 0resolving throughcurrent, andtargetVersionbelow the earliest snapshot returning thetarget version is prunederror), since this is now part of the package's public surface rather than an internal helper.
The clone-based replay open regressed nodes bootstrapped with a non-default initial height: memiavl keeps its bootstrap snapshot named snapshot-0 even after SetInitialVersion(N), while the first changelog entry is version N, so verifyClonedWALCovers treated the valid successor as a gap and replay mode failed deterministically with "source kept churning" until the first snapshot rewrite. A freshly recovered chain (genesis initial_height > 1) is exactly that shape. Derive the snapshot's successor from the cloned snapshot's metadata via utils.NextVersion instead of assuming snapshotVersion+1; FlatKV keeps the +1 rule because SetInitialVersion(N) seeds snapshot-<N-1> there. Also from review: - Reap abandoned clones. A SIGKILLed tool leaves its clone inside the data dir, where hardlinks pin snapshot inodes so pruning frees no disk and the deliberate non-"-tmp" name escapes memiavl's cleanup. Every clone now carries a flock-held owner marker (OFD/flock locks die with the process); clone creation sweeps siblings whose lock is acquirable, and marker-less directories once they are older than the mkdir-to-lock window can explain. - Document that --height 0 against a live node is best-effort: the clone is a consistent committed prefix as of the copy instant, and a detected torn-tail repair now prints a warning telling the operator to trust the report's version line. Cross-node comparisons should use an explicit common height. - Drop the MkdirAll that could never do useful work (snapshot selection already reads dbDir and fails first when it is missing). - Rename SeekSnapshotDir to SeekSnapshotName — it returns a name relative to root, not a path — and pin the exported contract with a unit test, along with ReadMetadata which the successor derivation needs. Co-authored-by: Cursor <[email protected]>
There was a problem hiding this comment.
Solid, well-motivated fix: replay mode now operates on a hardlinked/byte-copied private clone instead of handing a live changelog to memiavl's read-write WAL open, and the shared toolClone + flock ownership marker is a clean generalization of the existing FlatKV path. I verified the key assumptions hold (memiavl publishes snapshots via -tmp+rename and drops them via atomicRemoveDir, traverseSnapshots/removeTmpDirs ignore the .seidb-*-tool-* prefix, tryTruncateWAL truncates to the earliest snapshot so the coverage check really is a mid-clone-race detector, NextVersion + initEmptyDB make the SetInitialVersion case real, and FlatKV's LoadVersion does reject a version shortfall). No blockers; only polish and test-coverage notes.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Test coverage gap on the memiavl side: the FlatKV suite pins the two properties that make the clone affordable (
TestPrepareFlatKVToolingCloneHardlinksSnapshotAndCopiesChangelog,TestPrepareFlatKVToolingClonePlacesTempDirInsideDBDir), butmemiavl_open_test.gohas no equivalent. Nothing currently fails iftryPrepareMemIAVLToolingCloneregressed to byte-copying a multi-GB snapshot or to creating the clone outsidedbDir— both would still pass every new test. A cheap assertion onos.SameFile(src, dst)for one snapshot file plus afilepath.Rel(dbDir, clone.dir)check would cover it. - Leaked-clone recovery is only triggered by a later invocation of the same mode on the same dir (
newToolClonesweeps just its own prefix, whichTestSweepStaleToolClonesdeliberately pins). Since a leaked clone's hardlinks pin snapshot inodes and silently stop the live node's pruning from reclaiming disk, consider printing the clone path to stderr at startup so an operator who SIGKILLs the tool can find and remove it without waiting for the next run. - Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so that perspective is missing from this synthesis. Codex reported no material issues, which matches my own read. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| firstNeeded := utils.NextVersion(snapshotVersion, uint32(metadata.InitialVersion)) | ||
|
|
||
| srcChangelogDir := filepath.Join(dbDir, "changelog") |
There was a problem hiding this comment.
[suggestion] Nit: this hardcodes the memiavl changelog layout, while memiavl itself resolves it through utils.GetChangelogPath(opts.Dir) (sei-db/state_db/sc/memiavl/db.go:206). The file already imports sei-db/common/utils for NextVersion, so utils.GetChangelogPath(dbDir) here (and for dstChangelogDir below) keeps the tool inheriting the layout rather than restating it — the same rationale the PR gives for exporting SeekSnapshotName instead of reimplementing the snapshot layout.
| // publish snapshots by rename and drop them wholesale, so the same | ||
| // hardlink-the-snapshot / byte-copy-the-changelog split applies. | ||
| func tryPrepareMemIAVLToolingClone(dbDir string, height int64) (*toolClone, error) { | ||
| snapshotName, snapshotVersion, err := memiavl.SeekSnapshotName(dbDir, height) |
There was a problem hiding this comment.
[nit] For height == 0, SeekSnapshotName bottoms out in os.Readlink(current), so a missing or uninitialized dbDir returns an os.ErrNotExist-wrapped error. isCloneRetryableError treats that as retryable, so the operator gets three attempts and then clone aborted after 3 retries, source kept churning instead of the actual "no such file or directory". Same shape as the pre-existing FlatKV path, so not new, but it's a misleading diagnosis for the most common operator typo. Worth stat-ing dbDir/current once up front and failing fast.
There was a problem hiding this comment.
No new issues found in this pass. I revisited my earlier inline comment flagging that the WAL-coverage check might mis-handle a memiavl DB bootstrapped via SetInitialVersion(N) — that concern is already resolved in the current code: tryPrepareMemIAVLToolingClone derives the WAL successor from the cloned snapshot's InitialVersion metadata (via the newly exported memiavl.ReadMetadata) instead of assuming snapshotVersion+1, and TestOpenMemiAVLReplayAfterSetInitialVersion exercises and passes that exact scenario. Given this still touches memiavl's core snapshot/WAL read path and introduces new clone-lifecycle and locking logic, I think it is worth a human pass before merging.
Extended reasoning...
Overview
This PR replaces the tooling's use of memiavl.Options{ReadOnly: true} (which still opens the changelog WAL read-write and can truncate a live node's torn tail, destroying committed versions) with a clone-based approach mirroring the existing FlatKV tooling pattern: hardlink the snapshot, byte-copy the changelog, verify WAL coverage, then operate on the private clone. It adds a shared toolClone abstraction (owner flock + stale-clone sweep) used by both the FlatKV and new memiavl clone paths, and exports memiavl.SeekSnapshotName/memiavl.ReadMetadata so the tool inherits the package's snapshot-layout rules instead of re-implementing them. Production code paths (GetLatestVersion, LoadMultiTree) are only touched by the readMetadata->`ReadMetadata" export/rename, not behaviorally changed.
Security risks
None of consequence. This is operator-invoked CLI tooling (seidb evm-logical-digest), not exposed to untrusted input. The main risk class is data-integrity (a tool corrupting a live node's changelog), which is exactly what this PR fixes; it does not introduce a new one that I could find. The new clone directories inherit the source directory's permissions and live alongside the node's own data.
Level of scrutiny
Medium-high. While scoped to tooling, the change touches memiavl's snapshot/WAL layout assumptions directly (exports two previously-private helpers) and introduces new concurrency-sensitive logic (owner-lock based stale-clone sweeping, retry-on-churn cloning) that a live node's writer can race with. The regression tests are strong — they reproduce the exact torn-tail-truncation bug this PR fixes and assert the source directory is left untouched — and I ran the full operations + memiavl test suites locally; both new memiavl_open_test.go and tool_clone_test.go pass, along with the SetInitialVersion regression test.
Other factors
Two automated reviewers (chatgpt-codex-connector and seidroid) raised concerns in earlier PR comments — non-default initial-height WAL coverage, abandoned-clone cleanup, and a moot MkdirAll — that map exactly to the ReadMetadata-derived firstNeeded, sweepStaleToolClones, and warnIfCloneRepaired mechanisms already present in the current diff, so those appear to have been addressed already (possibly in the "address review findings" commit that was part of the PR from the start). My own prior inline comment repeated the same now-resolved concern; I want to flag here that it no longer applies against the current code so it doesn't mislead a future reviewer. Two local test failures (TestWriterLoopErrors, TestSnapshotWriterErrorHandling) are pre-existing and unrelated — they fail because the sandbox runs as root, which lets writes to '/invalid/path' succeed where the test expects a permission error.
1 participant
Problem
--memiavl-open-mode replayrelied onmemiavl.Options{ReadOnly: true},which only skips the
LOCKfile. The changelog is still opened read-write —db.gosays so outright ("Even in read-only mode we may need WAL replay") —and sei-db's
wal.open()answerswal.ErrCorruptwithos.Truncateon thenode's segment. Nothing on that path consults
ReadOnly.On a live node a torn tail is the writer mid-append, not corruption.
Truncating it drops committed versions, and since the writer's fd keeps its
old offset, its next append leaves a zero-filled hole — which
binary.Uvarintreads as valid empty records rather than an error. Nothing fails at the time;
the node finds out at its next restart.
Fix
Clone, as
openFlatKVReadOnlyalready does for the other backend: hardlinkthe snapshot, byte-copy the changelog, verify it still covers
snapshotVersion+1, retry if the writer prunes mid-clone, then open the clonenormally, lock and all. Every source access is now a read.
Two changes fall out of it:
--height(a tail repair costs thetrailing version, and a silently early digest looks like real divergence)
SeekSnapshotDirinstead of reimplementing its snapshotlayout in the tool
Limits
Tool-scoped only.
memiavl.OpenDB(ReadOnly)is still reachable from wasmsnapshot export and
storev2/rootmultiproof queries; fixing it at the WALlayer is tracked separately. The source
LOCKis still not taken — a racingwriter costs us a retry now instead of costing the node its changelog.
--memiavl-open-mode snapshot(the default) was never affected.Callers need a writable
dbDirand room for the changelog copy, as theFlatKV path already did.
Tests
the torn bytes left intact for the live writer
LOCK