Repository navigation
Backport release/v6.7: fix(seidb): refuse a corrupted changelog in digest replay instead of repairing it - #4282
blindchaser wants to merge 1 commit into
Conversation
…repairing it (#3983) ## Summary `seidb evm-logical-digest --memiavl-open-mode replay` opens the changelog through the same opener a writer uses, and that opener repairs a tail ending mid-record by truncating it. `Options.ReadOnly` does not prevent this: it gates the DB API, not the changelog open. On a live node the repair fires on a healthy log. A tail ending mid-record is usually the writer mid-append — `write` is not atomic against a concurrent reader, so a reader sees a page-aligned prefix of a multi-page changeset. Truncating it discards a record `seid` has committed, and the writer's descriptor keeps its old offset, leaving a zero-filled hole the binary decoder reads as valid zero-length records. Nothing fails at the time; it surfaces when the node next reopens and replays. Nothing about the directory distinguishes that from real damage, so this stops trying to. Replay refuses instead, leaving the tail where it found it, and reports that a rerun is the next step. The window is a single write, so a rerun clears it; a tail that survives repeated runs is real damage, and the message says so. - `sei-db/wal/wal.go`, `utils.go`: add `Config.NoRepairOnOpen`, which returns `ErrCorrupt` from `open` instead of truncating, and export `ErrCorrupt` so a caller can name the outcome. Default is unset, so every existing caller — `seid` included — keeps repairing as before. - `sei-db/state_db/sc/memiavl/opts.go`, `db.go`: add `Options.NoChangelogRepair` and pass it through. `ReadOnly` alone still repairs, because a reader that cannot rerun is better served by proceeding. - `sei-db/tools/cmd/seidb/operations/evm_logical_digest.go`: set it for replay mode, and translate `ErrCorrupt` into what the operator should do. The translation also covers a torn record found during the replay rather than at the open: `computeWALIndexDelta` and `Catchup` read the changelog after the open returns, and they surface the same sentinel, where a rerun is the same right answer. ### What this does not cover `wal.Open` also finishes an interrupted truncation before it checks for a torn record, by removing and renaming segments — a `.START` file left by `TruncateFront`, or a `.END` file left by `TruncateBack`. It reports no error for either, so there is nothing to gate on and no rerun to prompt. Finishing one underneath the writer makes the writer's own remove fail, and tidwall sets `l.corrupt` past that point, so every later append fails until `seid` restarts. The operator message therefore promises only the changelog tail, which does survive, rather than the whole directory. Both windows are narrower than the mid-record one, which recurs every block. `TruncateFront` runs after a snapshot rewrite, roughly hourly at the default interval. `TruncateBack` runs only inside a `LoadForOverwriting` open, so reaching that window needs a crash during a rollback and a replay before the node restarts. Both are left open deliberately — closing them needs either a check before the open, which the writer can invalidate in the gap, or the directory's `LOCK`, which would require the node stopped and take away the case replay exists for. Replay stays usable on a live node, which is the point: it is the fallback for a height with no snapshot, and on a live migrating node the two backends rarely retain a common snapshot height. ## Test plan - `sei-db/tools/cmd/seidb/operations/memiavl_open_test.go`: tear the last changelog segment, then require the open to report `ErrCorrupt` with the rerun guidance *and* the segment's bytes to be unchanged, since the refusal is worth nothing if the tail does not survive it. Replaying an intact changelog to the requested height is the positive control. Reverting `NoChangelogRepair` makes the first test fail on a successful open, so it pins the behavior rather than restating it. - `sei-db/wal/wal_test.go`: existing `TestOpenAndCorruptedTail` still passes with repair requested, pinning that the default path is untouched. - `go test ./sei-db/wal/... ./sei-db/tools/cmd/seidb/operations/...`, `scripts/ramtest.sh ./sei-db/state_db/sc/memiavl/...` - `make dblint` --------- Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit 7273a43) Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview The WAL layer gains
New tests assert a torn changelog is refused without mutating the segment, and intact changelogs still replay. Reviewed by Cursor Bugbot for commit b829937. 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/v6.7 #4282 +/- ##
================================================
- Coverage 61.34% 60.60% -0.74%
================================================
Files 2163 2083 -80
Lines 188785 180185 -8600
================================================
- Hits 115813 109206 -6607
+ Misses 62254 60970 -1284
+ Partials 10718 10009 -709
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Closing: this was created by hand. The right path is the seidroid backport bot, so #3983 gets the Keeping the note about the one fix the cherry-pick needs: |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Faithful backport of #3983 — the diff matches upstream exactly apart from the documented store.Commit() signature adaptation, and the NoRepairOnOpen → wal.ErrCorrupt plumbing through memiavl to the digest tool is correct and covered by a positive/negative test pair. Only a doc-accuracy point on the newly exported ErrCorrupt and a test-coverage gap at the wal layer.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new
Config.NoRepairOnOpenfield has no direct test insei-db/wal;wal_test.gowas only updated mechanically to passfalse.TestOpenAndCorruptedTailis already a table test over corrupt-tail cases, so adding anoRepair: truerow that assertsopenreturnswal.ErrCorruptand leaves the segment bytes untouched would pin the new behaviour at the layer that implements it, rather than only end-to-end through the seidb tool. - 1 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]
truncateCorruptedTail(sei-db/wal/utils.go:58) ends withif pos != len(data), butdatais resliced inside the loop (data = data[n:]), so the check compares bytes consumed against bytes remaining instead of against the original file length. When those two happen to be equal the truncation is silently skipped and the retriedwal.Openfails withErrCorrupteven though repair was requested. Comparing against the original length (captured before the loop) is the intended condition.
| const defaultWriteBatchSize = 64 | ||
|
|
||
| // ErrCorrupt reports that the log ends mid-record. An open returns it only under | ||
| // Config.NoRepairOnOpen; otherwise the tail is truncated and the open succeeds. |
There was a problem hiding this comment.
[suggestion] The word "only" overstates the guarantee. open also returns wal.ErrCorrupt on the repair path: when no segment file is found (len(lastSeg) == 0 returns the original err), and when the retried wal.Open after truncateCorruptedTail still reports corruption. Since this is a newly exported sentinel that callers will branch on with errors.Is, the doc as written invites the inference "ErrCorrupt ⇒ NoRepairOnOpen was set ⇒ the log was not modified," which does not hold. Something like "NoRepairOnOpen makes an open return it instead of truncating the tail; it can also surface when the repair itself fails" keeps the contract honest.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
1 participant
Backport of #3983 to
release/v6.7.Why this goes to v6.7
release/v6.7already shipsevm-logical-digest --memiavl-open-mode replay, and on this branch that open path still repairs a changelog it cannot read. The repair is a truncation, and a record a running node is midway through writing is indistinguishable from a corrupt one, so pointing the tool at a live v6.7 node can discard a block that node has committed.#4255 (backport of #4166) widens who reaches that path: inspect mode and the composite backend both start accepting
--memiavl-open-mode replay. Land this PR before #4255.Conflicts
The cherry-pick applied without conflicts. One change was needed on top:
sei-db/tools/cmd/seidb/operations/memiavl_open_test.go—CommitStore.Committakes a version argument onmainand no argument onrelease/v6.7, sostore.Commit(store.Version() + 1)becomesstore.Commit(). This matches how the rest of the package already calls it on this branch.No CHANGELOG entry, following the convention on this branch since the v6.7 changelog cut (#4113).
Tests
All three packages pass. Both new tests run and pass on this branch:
TestOpenMemiAVLReplayReadOnlyRefusesATornChangelogWithoutTruncatingItTestOpenMemiAVLReplayReadOnlyReplaysAnIntactChangeloggo build ./sei-db/...,gofmt -s -l, andgoimports -lare clean.