[r3.4] db/state: fix CursorHeap tie-break to prefer RAM over DB over FILE#20318
[r3.4] db/state: fix CursorHeap tie-break to prefer RAM over DB over FILE#20318sudeepdino008 merged 4 commits intorelease/3.4from
Conversation
|
@sudeepdino008 but files must be over DB? Because pruning can leave garbage? |
i'll think about it. |
@AskAlexSharov we should be fine as long as we don't do out of order pruning (e.g removing more latest step first, then older step). Half pruning is okay. |
…FILE (#20318) - When `DomainDelPrefix` iterates storage keys during account deletion, it merges RAM, DB, and FILE cursors via a min-heap - RAM and DB cursors both used `endTxNum: math.MaxUint64`, so when the same key existed in both, heap order was undefined - If DB won the tie, the old value was used as `prevVal` in the history entry instead of the current uncommitted RAM value - This caused corrupted history values that propagated to commitment root mismatches Fix: add tie-breaker in `CursorHeap.Less` — when `endTxNum` is equal, prefer `RAM_CURSOR > DB_CURSOR > FILE_CURSOR` **Safety**: `CursorHeap` is used in 3 contexts: - `merge.go` / `deduplicate.go`: only `FILE_CURSOR` — tie-break is a no-op (same type, returns false) - `DomainLatestIterFile` (DB + FILE): DB `endTxNum = step * stepSize` always differs from FILE `endTxNum = item.endTxNum - 1`, so tie-break rarely fires. If it does, DB correctly wins over FILE - `debugIteratePrefixLatest` (RAM + DB + FILE): the bug fix — RAM now correctly wins over DB when both use `MaxUint64` Fixes #20246
There was a problem hiding this comment.
Pull request overview
This PR fixes a deterministic ordering bug in db/state’s cursor merge heap that could select stale DB values over uncommitted RAM values when keys and endTxNum tie, leading to incorrect history entries and downstream commitment root mismatches (per #20246).
Changes:
- Add a tie-break rule in
CursorHeap.Lessto preferRAM_CURSOR > DB_CURSOR > FILE_CURSORwhenendTxNumis equal. - Add unit tests covering the new tie-break behavior and the previously failing merge-loop scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
db/state/domain_stream.go |
Makes heap ordering deterministic on (key, endTxNum) ties by prioritizing RAM over DB over FILE. |
db/state/domain_stream_test.go |
Adds tests to ensure tie-break priority is enforced and that merge-loop selection prefers RAM values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestCursorHeapPriority_MaxUint64TieBreak(t *testing.T) { | ||
| // Reproduces the exact scenario from #20246: RAM and DB both use | ||
| // math.MaxUint64 as endTxNum in debugIteratePrefixLatest. | ||
| // RAM must win so DomainDelPrefix picks up the current uncommitted value. | ||
| var h CursorHeap | ||
| heap.Init(&h) | ||
| heap.Push(&h, &CursorItem{t: DB_CURSOR, key: []byte("key1"), val: []byte("0f"), endTxNum: ^uint64(0), reverse: true}) | ||
| heap.Push(&h, &CursorItem{t: RAM_CURSOR, key: []byte("key1"), val: []byte("15"), endTxNum: ^uint64(0), reverse: true}) | ||
|
|
There was a problem hiding this comment.
In TestCursorHeapPriority_MaxUint64TieBreak the comment says the scenario uses math.MaxUint64, but the test value is ^uint64(0). Consider either importing math and using math.MaxUint64 or updating the comment so the test intent matches the literal being used (this avoids confusion when scanning failures).
…x, xsync fix, warmuper fix, logs debug, unused code (#20337) Cherry-picks from release/3.4 to main: - integrity: blk-range chk to pre-build "changed keys" index before calc state root (#20302) - up gql and grpc (#20304) - stepSize: all tooling to use `tx.Debug().StepSize()` instead of `DefaultStepSize` constant (#20280) - db/state: fix CursorHeap tie-break to prefer RAM over DB over FILE (#20318) - xsync deprecated use fix (#20331) - warmuper.WaitAndClose: cancel work before wait (#20332) - logs: move some Info logs to Debug level (to simplify logs for Users) (#20329) - db: unused code remove (#20334) --------- Co-authored-by: moskud <sudeepdino008@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
DomainDelPrefixiterates storage keys during account deletion, it merges RAM, DB, and FILE cursors via a min-heapendTxNum: math.MaxUint64, so when the same key existed in both, heap order was undefinedprevValin the history entry instead of the current uncommitted RAM valueFix: add tie-breaker in
CursorHeap.Less— whenendTxNumis equal, preferRAM_CURSOR > DB_CURSOR > FILE_CURSORSafety:
CursorHeapis used in 3 contexts:merge.go/deduplicate.go: onlyFILE_CURSOR— tie-break is a no-op (same type, returns false)DomainLatestIterFile(DB + FILE): DBendTxNum = step * stepSizealways differs from FILEendTxNum = item.endTxNum - 1, so tie-break rarely fires. If it does, DB correctly wins over FILEdebugIteratePrefixLatest(RAM + DB + FILE): the bug fix — RAM now correctly wins over DB when both useMaxUint64Fixes #20246