Repository navigation
fix(core/rawdb): return iterator errors from the dangling-hash sweep, close #2604 - #2707
Merged
gzliudan merged 1 commit intoOct 13, 2026
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
September 29, 2026 10:50
anunay-xin
approved these changes
Sep 30, 2026
wanwiset25
approved these changes
Oct 1, 2026
gzliudan
force-pushed
the
fix-rawdb-sweep-iterator-error
branch
5 times, most recently
from
October 3, 2026 15:35
d61a4f7 to
530eb1e
Compare
4 of 25 tasks
gzliudan
added a commit
to gzliudan/XDPoSChain
that referenced
this pull request
Oct 6, 2026
ported from go-ethereum ethereum#32506 InspectDatabase walked the keyspace and never consulted it.Error(), so a walk the database aborted printed a table built from a partial scan and returned nil, which reads as "inspection passed" to whoever ran db inspect. Only the per-key accounting was affected; nothing on the consensus or sync paths reads this function, its single caller is the db inspect command. Upstream landed the check as part of parallelising the traversal, so its InspectDatabase is a worker closure over per-range stat structs driven by an errgroup. The version here is the single-threaded one and the two do not line up line for line, so only the semantics are ported: report the pending iterator error once the walk ends. The defect predates dev-upgrade, hence a separate commit rather than a fixup. The test also pins the user-visible half of the early return, by capturing os.Stdout around the call: an aborted walk must render nothing, and a clean walk over the same fixture must still render the report. Upstream orders it the same way - its workers return the iterator error and eg.Wait short-circuits before the table is rendered - but upstream has no test for either function, so the assertion is ours. One direction alone cannot tell an early return apart from a function that never renders anything. Confirmed with go test -overlay, both directions: deleting the check fails the first assertion with got <nil>, moving it below table.Render() fails the second one on the table built from the partial scan. The store that aborts the walk mid-iteration is a verbatim copy of the fixture TestDeleteDanglingHashes uses, which dev-upgrade does not carry yet. It is duplicated here so this branch builds on its own; merge XinFinOrg#2707 first, then rebase this branch onto it and delete the copy, at which point the test picks the shared fixture up from accessors_chain_test.go. The copy is marked as temporary in the file so the deletion is a pure removal.
gzliudan
force-pushed
the
fix-rawdb-sweep-iterator-error
branch
2 times, most recently
from
October 13, 2026 07:10
08fdf38 to
e76ecd8
Compare
benjamin202410
approved these changes
Oct 13, 2026
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix correctly distinguishes iterator failure from exhaustion and includes adequate regression coverage.
0 open findings
What changed in this PR
Ensures interrupted dangling-hash sweeps propagate iterator errors during chain rewinds.
Changes:
- Return iterator errors before committing the final deletion batch.
- Document intentional
ReadAllHashesbehavior. - Add failure and successful-sweep tests.
| File | Description |
|---|---|
core/rawdb/accessors_chain.go |
Propagates sweep iterator failures. |
core/rawdb/accessors_chain_test.go |
Tests failed and completed sweeps. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gzliudan
added a commit
to gzliudan/XDPoSChain
that referenced
this pull request
Oct 13, 2026
ported from go-ethereum ethereum#32506 InspectDatabase walked the keyspace and never consulted it.Error(), so a walk that the database aborted printed a table built from a partial scan and returned nil, which reads as "inspection passed" to whoever ran db inspect. Only the per-key accounting was affected; nothing on the consensus or sync paths reads this function, its single caller is the db inspect command. Upstream landed the check as part of parallelising the traversal, so its InspectDatabase is a worker closure over per-range stat structs driven by an errgroup. The version here is the single-threaded one and the two do not line up line for line, so only the semantics are ported: report the pending iterator error once the walk ends. The defect predates dev-upgrade, hence a separate commit rather than a fixup. The test also pins the user-visible half of the early return, by capturing os.Stdout around the call: an aborted walk must render nothing, and a clean walk over the same fixture must still render the report, down to its per-category rows and a non-zero total. Upstream orders it the same way - its workers return the iterator error and eg.Wait short-circuits before the table is rendered - but upstream has no test for either function, so the assertion is ours. One direction alone cannot tell an early return apart from a function that never renders anything. Confirmed with go test -overlay, both directions: deleting the check fails the first assertion with got <nil>, moving it below table.Render() fails the second one on the table built from the partial scan. The capture helper drains the pipe from its own goroutine and restores os.Stdout through t.Cleanup, so the walk cannot deadlock against the pipe buffer. The store that aborts the walk mid-iteration is a verbatim copy of the fixture TestDeleteDanglingHashes uses, which dev-upgrade does not carry yet. It is duplicated here so this branch builds on its own; merge XinFinOrg#2707 first, then rebase this branch onto it and delete the copy, at which point the test picks the shared fixture up from accessors_chain_test.go. The copy is marked as temporary in the file so the deletion is a pure removal.
…lose XinFinOrg#2604 DeleteDanglingHashes walked the header keyspace and only propagated the final batch write error, so a walk the database aborted on an I/O error looked exactly like one that ran to the end. Its only caller, HeaderChain.SetHead, acts on a non-nil error alone: the rewind therefore completed and purged its caches while orphaned headers, total-difficulty entries and canonical markers stayed above the new head. That is the state XinFinOrg#2442 set out to prevent, where the downloader's HasBlock ancestor search can latch onto a stale block across a gap and skip re-syncing the range in between. Return the pending iterator error before committing the last batch, the shape upstream uses in rawdb.SafeDeleteRange. The caller's existing log.Crit("Failed to rewind block") then ends the process, which is how this function already treats a failed batch write. The defect comes from XinFinOrg#2442 and is inherited by dev-upgrade, so it lands as its own commit. Upstream has no DeleteDanglingHashes - geth's setHead still cleans leftovers with a contiguous upward scan - so there is no upstream fix to port. ReadAllHashes keeps ignoring iterator errors: it is byte-identical to upstream and whatever a truncated walk misses is removed by the sweep that follows the rewind loop. Its doc comment now records that boundary. Tests use a wrapper around ethdb.Database whose NewIterator stops after one key and reports an error, making a truncated walk reproducible without a production seam; a second case pins the clean walk so the new check cannot reject a completed sweep. The error case likewise pins the heights the aborted walk never reached, so reporting the error only after the keyspace had been swept and the batch committed - a completed sweep that happens to fail - would fail it too. The sweep's behaviour across gaps is already covered by core/blockchain_sethead_test.go. The clean-walk case now also checks the two entries the sweep removes beside the header: writeOrphanHeaders writes the td and canonical-hash entries along with each header, as a real import would, and the case requires all three to be gone. It previously asserted header removal only, against a fixture that wrote neither of the other two, so a regression that kept a total difficulty or a canonical marker above the new head passed. Confirmed with go test -overlay: dropping DeleteTd fails the case on the td entry, dropping DeleteCanonicalHash fails it on the marker.
gzliudan
force-pushed
the
fix-rawdb-sweep-iterator-error
branch
from
October 13, 2026 12:14
e76ecd8 to
02a836f
Compare
gzliudan
added a commit
to gzliudan/XDPoSChain
that referenced
this pull request
Oct 14, 2026
ported from go-ethereum ethereum#32506 InspectDatabase walked the keyspace and never consulted it.Error(), so a walk that the database aborted printed a table built from a partial scan and returned nil, which reads as "inspection passed" to whoever ran db inspect. Only the per-key accounting was affected; nothing on the consensus or sync paths reads this function, its single caller is the db inspect command. Upstream landed the check as part of parallelising the traversal, so its InspectDatabase is a worker closure over per-range stat structs driven by an errgroup. The version here is the single-threaded one and the two do not line up line for line, so only the semantics are ported: report the pending iterator error once the walk ends. The defect predates dev-upgrade, hence a separate commit rather than a fixup. The test also pins the user-visible half of the early return, by capturing os.Stdout around the call: an aborted walk must render nothing, and a clean walk over the same fixture must still render the report, down to its per-category rows and a non-zero total. Upstream orders it the same way - its workers return the iterator error and eg.Wait short-circuits before the table is rendered - but upstream has no test for either function, so the assertion is ours. One direction alone cannot tell an early return apart from a function that never renders anything. Confirmed with go test -overlay, both directions: deleting the check fails the first assertion with got <nil>, moving it below table.Render() fails the second one on the table built from the partial scan. The capture helper drains the pipe from its own goroutine and restores os.Stdout through t.Cleanup, so the walk cannot deadlock against the pipe buffer. The store that aborts the walk mid-iteration is a verbatim copy of the fixture TestDeleteDanglingHashes uses, which dev-upgrade does not carry yet. It is duplicated here so this branch builds on its own; merge XinFinOrg#2707 first, then rebase this branch onto it and delete the copy, at which point the test picks the shared fixture up from accessors_chain_test.go. The copy is marked as temporary in the file so the deletion is a pure removal.
gzliudan
added a commit
to gzliudan/XDPoSChain
that referenced
this pull request
Oct 14, 2026
ported from go-ethereum ethereum#32506 InspectDatabase walked the keyspace and never consulted it.Error(), so a walk that the database aborted printed a table built from a partial scan and returned nil, which reads as "inspection passed" to whoever ran db inspect. Only the per-key accounting was affected; nothing on the consensus or sync paths reads this function, its single caller is the db inspect command. Upstream landed the check as part of parallelising the traversal, so its InspectDatabase is a worker closure over per-range stat structs driven by an errgroup. The version here is the single-threaded one and the two do not line up line for line, so only the semantics are ported: report the pending iterator error once the walk ends. The defect predates dev-upgrade, hence a separate commit rather than a fixup. The test also pins the user-visible half of the early return, by capturing os.Stdout around the call: an aborted walk must render nothing, and a clean walk over the same fixture must still render the report, down to its per-category rows and a non-zero total. Upstream orders it the same way - its workers return the iterator error and eg.Wait short-circuits before the table is rendered - but upstream has no test for either function, so the assertion is ours. One direction alone cannot tell an early return apart from a function that never renders anything. Confirmed with go test -overlay, both directions: deleting the check fails the first assertion with got <nil>, moving it below table.Render() fails the second one on the table built from the partial scan. The capture helper drains the pipe from its own goroutine and restores os.Stdout through t.Cleanup, so the walk cannot deadlock against the pipe buffer. The store that aborts the walk mid-iteration is a verbatim copy of the fixture TestDeleteDanglingHashes uses, which dev-upgrade does not carry yet. It is duplicated here so this branch builds on its own; merge XinFinOrg#2707 first, then rebase this branch onto it and delete the copy, at which point the test picks the shared fixture up from accessors_chain_test.go. The copy is marked as temporary in the file so the deletion is a pure removal.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes
DeleteDanglingHasheswalks the header keyspace with an iterator and propagated only the final batch write error, so a walk the database aborted on an I/O error was indistinguishable from one that ran to the end - both leave the loop onNext() == false.Its only caller,
HeaderChain.SetHead, acts on a non-nil error alone: the rewind reported success, purged its caches and moved on toDeleteBadBlocksAbovewhile orphaned headers, total-difficulty entries and canonical markers stayed above the new head. That is the state #2442 set out to prevent, where the downloader'sHasBlockancestor search can latch onto a stale block across a gap and skip re-syncing the range in between.The fix returns the pending iterator error before the last batch is committed, the shape upstream uses in
rawdb.SafeDeleteRange(core/rawdb/database.go:828). The caller's existinglog.Crit("Failed to rewind block")then ends the process, which is how this function already treats a failed batch write, so the caller's policy is unchanged.The defect comes from #2442 and is inherited by
dev-upgrade, so it lands as its own commit rather than a fixup. Upstream has noDeleteDanglingHashes- geth'ssetHeadstill cleans leftovers with a contiguous upward scan, which is the scan #2442 replaced because it stops at the first gap - so there is no upstream fix to port and no upstream PR to reference.ReadAllHasheskeeps ignoring iterator errors. It is byte-identical to upstream, and whatever a truncated walk misses is removed by the sweep that follows the rewind loop, so changing it would buy nothing and cost an upstream divergence. Its doc comment now records that boundary.Tests
TestDeleteDanglingHashesIteratorErrorwrapsethdb.Databasein a test-only store whoseNewIteratorhands out an iterator that stops after one key and reports an error, so a truncated walk is reproducible without a production seam. Reverting theit.Error()check makes it fail withgot <nil>, which is the pre-fix behaviour.TestDeleteDanglingHashesCleanWalkpins the other side of the boundary: a walk that runs to the end still returns no error and still removes every orphaned header, together with the total-difficulty entry and the canonical marker beside it, so the new check cannot reject a completed sweep. The fixture writes those two entries next to each header, as a real import would - without them an assertion on either would pass vacuously.The sweep's behaviour across gaps is already covered by
TestSetHeadCleansDanglingBlocksAcrossGapand its neighbours incore/blockchain_sethead_test.go.make allandmake quick-testpass on the branch.Types of changes
Impacted Components
Checklist
The three network-facing items are not applicable here: the change only alters the one error path that surfaces when the database itself fails mid-walk - the dangling-hash sweep during a rewind - so the wire format, consensus rules, storage layout and API signatures are untouched, and nodes running this build coexist with older ones unchanged. Reaching the changed line end to end needs an I/O failure while a keyspace is being walked, which cannot be produced by configuration; the unit test drives it through the injected iterator instead.
Closes #2604