-
Notifications
You must be signed in to change notification settings - Fork 886
fix(seidb): refuse a corrupted changelog in digest replay instead of repairing it #3983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
33c6cde
fix(seidb): fail closed on live memiavl WAL reads
blindchaser e8d1778
fix(wal): classify vanished segments as retryable
blindchaser 5879f4e
fix(wal): stabilize live read-only views
blindchaser 9afce99
refactor(seidb): scope fail-loud WAL reads to digest
blindchaser dcdfa41
refactor(seidb): check the changelog instead of reading it read-only
blindchaser 37c745f
refactor(wal): move VerifyIntact beside the repair it avoids
blindchaser 9075ecb
fix(seidb): reject a digest replay that skipped pruned versions
blindchaser 022d9df
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser 38feadd
test(seidb): adopt the CommitStore.Commit version cross-check
blindchaser 10c4278
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser d47e81b
fix(seidb): stop the gap check from rejecting seeded chains
blindchaser cc54a07
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser f76dd3e
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser efde104
refactor(seidb): drop the digest replay-coverage guard
blindchaser 259276f
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser 5eb3d8e
fix(seidb): stop the digest replay open from repairing the changelog
blindchaser 431de98
fix(seidb): refuse a digest replay of a directory a writer holds
blindchaser 9c8aac8
refactor(seidb): drop the changelog no-repair option
blindchaser 7e5af21
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser d3aab48
fix(seidb): refuse a torn changelog in digest replay instead of repai…
blindchaser bad7368
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser d03ad01
Merge remote-tracking branch 'origin/main' into fix/seidb-digest-read…
blindchaser File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package operations | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/sei-protocol/sei-chain/sei-db/common/keys" | ||
| "github.com/sei-protocol/sei-chain/sei-db/common/utils" | ||
| "github.com/sei-protocol/sei-chain/sei-db/proto" | ||
| "github.com/sei-protocol/sei-chain/sei-db/wal" | ||
| ) | ||
|
|
||
| // TestOpenMemiAVLReplayReadOnlyRefusesATornChangelogWithoutTruncatingIt pins the | ||
| // reason replay does not repair: a record that ends mid-write looks the same | ||
| // whether the node crashed or is committing right now, and truncating it in the | ||
| // second case discards a committed block. The refusal is only worth anything if | ||
| // the tail survives it, so the segment's bytes are compared too. | ||
| func TestOpenMemiAVLReplayReadOnlyRefusesATornChangelogWithoutTruncatingIt(t *testing.T) { | ||
| homeDir := t.TempDir() | ||
| writeMemiavlNonces(t, homeDir, 3) | ||
|
|
||
| dbDir := utils.GetCosmosSCStorePath(homeDir) | ||
| segment := lastChangelogSegment(t, dbDir) | ||
| torn := tearFileTail(t, segment) | ||
|
|
||
| db, err := openMemiAVLReplayReadOnly(dbDir, 3) | ||
| require.Nil(t, db) | ||
| require.ErrorIs(t, err, wal.ErrCorrupt) | ||
| require.Contains(t, err.Error(), "rerun this command") | ||
|
|
||
| after, err := os.ReadFile(segment) //nolint:gosec // test-controlled path | ||
| require.NoError(t, err) | ||
| require.Equal(t, torn, after, "the open must leave the changelog segment as it found it") | ||
| } | ||
|
|
||
| // TestOpenMemiAVLReplayReadOnlyReplaysAnIntactChangelog is the positive control: | ||
| // refusing a torn tail is only the intended change if an intact one still | ||
| // replays. | ||
| func TestOpenMemiAVLReplayReadOnlyReplaysAnIntactChangelog(t *testing.T) { | ||
| homeDir := t.TempDir() | ||
| writeMemiavlNonces(t, homeDir, 3) | ||
|
|
||
| db, err := openMemiAVLReplayReadOnly(utils.GetCosmosSCStorePath(homeDir), 3) | ||
| require.NoError(t, err) | ||
| defer func() { _ = db.Close() }() | ||
| require.Equal(t, int64(3), db.Version()) | ||
| } | ||
|
|
||
| // writeMemiavlNonces commits count blocks to a fresh memiavl store under homeDir | ||
| // and closes it, leaving a changelog with one entry per block. | ||
| func writeMemiavlNonces(t *testing.T, homeDir string, count uint64) { | ||
| t.Helper() | ||
| store := newTestMemiavlStore(t, homeDir) | ||
| for nonce := uint64(1); nonce <= count; nonce++ { | ||
| require.NoError(t, store.ApplyChangeSets([]*proto.NamedChangeSet{{ | ||
| Name: keys.EVMStoreKey, | ||
| Changeset: proto.ChangeSet{Pairs: []*proto.KVPair{noncePair(addrN(0xA1), nonce)}}, | ||
| }})) | ||
| _, err := store.Commit(store.Version() + 1) | ||
| require.NoError(t, err) | ||
| } | ||
| require.NoError(t, store.Close()) | ||
| } | ||
|
|
||
| // lastChangelogSegment returns the path of the segment the changelog appends to. | ||
| // Segment names are zero-padded indices, so the highest name sorts last. | ||
| func lastChangelogSegment(t *testing.T, dbDir string) string { | ||
| t.Helper() | ||
| changelogDir := utils.GetChangelogPath(dbDir) | ||
| entries, err := os.ReadDir(changelogDir) | ||
| require.NoError(t, err) | ||
| var name string | ||
| for _, entry := range entries { | ||
| if !entry.IsDir() && len(entry.Name()) >= 20 { | ||
| name = entry.Name() | ||
| } | ||
| } | ||
| require.NotEmpty(t, name, "no changelog segment under %s", changelogDir) | ||
| return filepath.Join(changelogDir, name) | ||
| } | ||
|
|
||
| // tearFileTail drops the last byte of path, which is what a reader sees partway | ||
| // through the writer's append, and returns the resulting contents. | ||
| func tearFileTail(t *testing.T, path string) []byte { | ||
| t.Helper() | ||
| data, err := os.ReadFile(path) //nolint:gosec // test-controlled path | ||
| require.NoError(t, err) | ||
| require.NotEmpty(t, data) | ||
| torn := data[:len(data)-1] | ||
| require.NoError(t, os.WriteFile(path, torn, 0o600)) | ||
| return torn | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replay still repairs live TruncateFront
High Severity
Replay no longer refuses a directory a writer holds.
NoRepairOnOpenonly skips torn-tail truncation afterwal.Open; that open still finishes an in-flightTruncateFrontwhenever a.STARTsegment exists, which is a normal prune window on every successfultryTruncateWAL. Completing that rename underseidmakes the writer's next remove fail and setsl.corrupt, so later appends returnErrCorruptuntil restart.Additional Locations (2)
sei-db/tools/cmd/seidb/operations/evm_logical_digest.go#L1114-L1121sei-db/state_db/sc/memiavl/db.go#L181-L186Reviewed by Cursor Bugbot for commit d3aab48. Configure here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, and deliberate — the PR body has a "What this does not cover" section for exactly this. One correction to the severity, though.
tidwall's own comment at the top of that cleanup says what the consequence is:
Both parties perform the same sequence toward the same target — remove the segments preceding
.START, then rename.STARTto the final segment name — so the directory converges on the state the writer was already moving it to. What breaks is the writer's process: its ownos.Removehits ENOENT, the deferred handler setsl.corrupt, and appends fail until restart. That is an availability event a restart clears, not data loss, and it is loud on the node rather than silent.The torn-tail path this PR closes was the one that lost data: it truncated a record
seidhad committed and left a zero-filled hole the decoder accepts, surfacing only at the node's next replay.Both closures were considered and cost more than the residual:
.STARTin the gap, and the completion that follows reports no error, so there is nothing to retry on.LOCKcloses it completely, but requires the node stopped, and replay exists for heights with no snapshot on a live migrating node, where the two backends rarely retain a common snapshot height.Window sizes differ by about an order of magnitude too: a mid-record tail recurs every block, while
.STARTexists only during aTruncateFrontthat follows a snapshot rewrite, roughly hourly at the default interval.