Conversation
…he new head The fork-choice decision and the XDPoS head side effects are needed in more than one place, so pull them out before the known-block adoption lands on top: - blockBeatsHead holds the rule writeBlockWithState applied inline (a higher total difficulty, or an equal one with a higher number), and writeBlockWithState now calls it, so any later known-block path can never adopt a chain under a rule an executed block would not be allowed to. - isGapBlock, notifyEpochSwitchBlock and cacheSigningTxs collect the side effects that were duplicated in insertChain, insertBlock, writeBlockWithState and reorg. - reorg no longer writes the new head itself: its callers do it after it returns. Doing it in both places wrote the same canonical markers twice and ran UpdateM1 twice for a gap block. The doc comment states the contract, and the stale marker cleanup starts one block lower to match. - insertSidechain is renamed to insertSideChain, as upstream did. No behaviour change: the extracted rule is the one that was applied inline, the helpers are the same code, and the new reorg contract is pinned by a test.
|
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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
changed the title
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
and
a balanced review from Copilot
September 13, 2026 17:38
There was a problem hiding this comment.
🟡 Changes recommended
Known-block and sidechain paths can still report incomplete imports as successful, while another local failure can still cause peer eviction.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes partial block-batch imports being reported as successful and prevents local interruptions from penalizing peers.
Changes:
- Propagates insertion and sidechain errors.
- Exports and classifies insertion interruptions.
- Adds regression coverage and refactors fork-choice/reorg handling.
File summaries
| File | Description |
|---|---|
core/blockchain.go |
Updates insertion errors, known-block handling, and reorg logic. |
core/blockchain_test.go |
Adds insertion, interruption, fork-choice, and reorg tests. |
eth/downloader/downloader.go |
Maps insertion interruptions to cancellation. |
eth/downloader/downloader_test.go |
Tests interruption classification. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…inOrg#2534 insertChain returned a nil error unconditionally after its import loop. The loop stops as soon as it.next() yields a verification or body-validation error, so any failure past the first block was silently discarded and a partial import was reported to the caller as a complete one. The downloader then believed every block in the batch had been imported and moved on, which left the chain head below blocks that were already on disk. The ancestor search no longer anchors above the local head (XinFinOrg#2547 capped it in usableAsAncestor), so the skipped range is requested again - but every such batch was reported as a success, which hid the missing import instead of surfacing it. Return the real error instead, which is what the upstream commit this code was ported from does. ErrKnownBlock is still normalised to nil when the whole rest of the batch is already on disk: those blocks are imported, so that is not an import failure and must not reach the downloader as an invalid chain. The normalisation says nothing about the head, which may still sit below the batch, so a warning is logged when that happens. An interruption observed at the import loop, at the entry guard or while aborting a sidechain segment is now reported as errInsertionInterrupted, in line with what writeBlockWithState and getResultBlock already return. The sentinel is exported so the downloader can recognise it: an interruption is local and says nothing about the blocks, so it is mapped to errCancelContentProcessing before the errInvalidChain wrapping and no longer drops the peer that served them. Add regression tests for a batch that fails in the middle through header verification and through body validation, for a fully verified batch, for the already-on-disk and known-block-ahead-of-head cases, for an interruption seen mid-batch and at the entry guard, and for the downloader keeping the peer when the insertion was interrupted.
gzliudan
force-pushed
the
fix-issue-2534
branch
from
September 13, 2026 18:03
d9b4a43 to
65ef980
Compare
The ErrKnownBlock normalisation only claims that there is nothing left of a batch to import, not that the head reached the end of it. When every remaining block is on disk with its state while the head sits below them - a batch this node imported and then rolled back, or a branch that lost fork choice at the time - nothing moved the head any more. The downloader cannot anchor its ancestor search above the local head, so every following sync fetched the same range again and each of those batches was reported as a success. Adopt the end of the batch when it wins blockBeatsHead, the rule writeBlockWithState applies to executed blocks: reorg to it and write the head here, as the reorg contract requires, together with the gap block side effect. This is the first caller of the fork-choice rule and of the reorg contract extracted in the previous commit. A batch that loses fork choice, or that reorg refuses (a missing parent chain, or the XDPoS committed-block guard), is not a failure of the blocks: they were already on disk. Those are logged and still normalised, so the downloader does not drop the peer that served them.
insertSideChain scans a pruned sidechain segment and then re-imports the prefix below it to rebuild its state. When the scan stopped on a block that failed verification or body validation, that error was dropped and the result of the re-import was returned instead: re-importing the prefix succeeds, so a partial import was reported as a success although nothing after the failing block was even looked at. Report the error before the prefix is rebuilt. ErrUnknownAncestor and ErrKnownBlock are how a pruned segment ends normally - the next block cannot be linked yet, or it is already on disk with its state - so they still fall through to the re-import, as before.
commitFastSyncData wrapped any InsertReceiptChain failure into errInvalidChain, which is the error class Synchronise drops the peer for. InsertReceiptChain reports the same local conditions as InsertChain - a stopping chain, when it cannot take the chain lock - and those say nothing about the peer that served the batch. Exempt them with core.IsLocalInsertError before the wrapping, as importBlockResults already does for the full sync path, and cover it with a test that injects ErrChainStopped through the receipt insertion hook.
There was a problem hiding this comment.
🟡 Changes recommended
Fast-sync interruptions and known-block recovery still contain partial-success and event-propagation defects.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
Comment on lines
+2098
to
+2101
| adopted, aerr := bc.adoptKnownBlock(last, current) | ||
| switch { | ||
| case aerr != nil: | ||
| log.Error("Cannot adopt an already imported batch", "head", current.Number, "block", last.NumberU64(), "err", aerr) |
Comment on lines
+2270
to
+2274
| // looked at. ErrUnknownAncestor and ErrKnownBlock are how a pruned segment ends | ||
| // normally (the next block cannot be linked yet, or it is already on disk with its | ||
| // state), so they are not failures and fall through to the reimport below. | ||
| if err != nil && !errors.Is(err, consensus.ErrUnknownAncestor) && !errors.Is(err, ErrKnownBlock) { | ||
| return it.index, nil, nil, err |
Comment on lines
+1933
to
+1938
| // A local failure (shutdown, or another insertion holding the chain lock) says | ||
| // nothing about the peer that served the batch, so it must not be turned into | ||
| // errInvalidChain: that is the branch that drops the peer. | ||
| if core.IsLocalInsertError(err) { | ||
| log.Debug("Downloaded item processing interrupted", "number", results[0].Header.Number, "index", index, "err", err) | ||
| return errCancelContentProcessing |
Comment on lines
+2104
to
+2106
| default: | ||
| log.Debug("Adopted an already imported batch", "number", last.NumberU64(), "hash", last.Hash()) | ||
| events = append(events, ChainHeadEvent{last}) |
InsertReceiptChain returned (0, nil) when the insertion was interrupted, even though the blocks before the one that stopped it may already have been flushed to disk. commitFastSyncData took that as a complete import, so a half written receipt batch was reported to the downloader as a successful one - the same partial-import behaviour the block path of this series reports as a failure. Return the number of blocks that made it to disk together with ErrInsertionInterrupted. The sentinel is local, so the downloader already maps it to errCancelContentProcessing and keeps the peer that served the batch.
… a known block Scanning a pruned sidechain segment stops as soon as a block is already on disk with its state. ErrKnownBlock was treated as a normal terminator, so the scan fell through to the reimport below, which only rebuilds it.previous() and its ancestors: nothing after the known block was ever looked at, yet the result of the prefix was returned as a success - a partial import reported as a complete one, the same hole the block path of this series closes for batches that stop in the middle. Adopt the rest of the batch when it is on disk too, through the same rule the ErrKnownBlock normalisation of insertChain applies, and report ErrInsertionInterrupted when it is not: missing blocks are this node's problem, not the peer's, and the sentinel keeps the downloader from turning it into an errInvalidChain that drops the peer. The adoption of insertChain moves into adoptKnownBatch so that both paths share one implementation instead of growing a second one.
A batch whose first blocks are already on disk with their state stops on ErrKnownBlock, which insertChain reports whenever the rest of the batch is not on disk as well. importBlockResults turned that into errInvalidChain, so the peer that served exactly the range it was asked for was dropped - and since the head cannot move past the known block, the next attempt asks for the same range again and drops another peer, without the chain ever advancing. Cancel the content processing instead, as for the local conditions this series already exempts. This only stops the bleeding: the head still has to learn to move past a known block, which is a change of its own.
Collaborator
Author
|
replaced by #2566 |
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.
Problem
BlockChain.insertChainreturns a nil error unconditionally after its import loop:The loop exits as soon as
it.next()yields a verification or body-validation error. Unless that error isErrFutureBlock/ErrUnknownAncestor, which the future-block branch consumes, it is silently discarded and a partial import is reported to the caller as a complete one.Every other return site in this function propagates
err; only the final one does not, which looks like a porting slip from the commit this code was ported from (core: rework blockchain import #17973).On a mainnet archive node the downloader believed a whole batch had been imported while only part of it had:
handle proposed blockis only reached afterInsertChainreturns nil, so the batch counted as fully imported while the persisted head was still106,086,982. The head therefore sits below blocks the node already stores by hash. Since #2547 the ancestor search cannot anchor above the local head —usableAsAncestorrejects every candidate above it — so the skipped range is requested again on the next sync, but every such batch was reported as a success: the node kept re-fetching the same range while the head stayed behind.Note the asymmetry that made this hard to spot: when the failing block is the first of a batch,
case err != nil:reports aBAD BLOCKand returns the error, so the failure is visible; when it is in the middle, the failure is silent and the head simply stops.Changes
The PR is five commits: a no-behaviour-change refactor, the error propagation fix, and three follow-ups for the holes that first fix left (a head that still would not move, a sidechain path that still dropped its error, and the fast sync path that still blamed the peer).
core/blockchain.goinsertChainreturns the real error after its import loop instead ofnil, so a batch that stops in the middle is reported as a failure.ErrKnownBlockis still normalised tonilwhen the whole rest of the batch is already on disk: those blocks are imported, so this is not an import failure and must not reach the downloader as an invalid chain. The normalisation says nothing about the head though, so when the head sits below the batch it is now adopted:adoptKnownBlockappliesblockBeatsHead, the fork-choice rulewriteBlockWithStateuses for executed blocks, then reorgs and writes the head. Without this the head never moves and, since the ancestor search cannot anchor above it, every following sync fetches the same range again.errInsertionInterrupted, in line with whatwriteBlockWithStateandgetResultBlockalready return.ErrInsertionInterrupted, andblockchain is stoppedasErrChainStopped(the unexported names stay as aliases of the same values).IsLocalInsertErrorclassifies both: they are local conditions that say nothing about the blocks.insertSideChainreports the block that stopped the scan of a pruned segment. It used to fall through to re-importing the prefix and return that result, which reported a partial import as a success because the re-import of the prefix succeeds.ErrUnknownAncestorandErrKnownBlockstill fall through: they are how a pruned segment ends normally (the next block cannot be linked yet, or it is already on disk with its state), so they are not failures.eth/downloader/downloader.go: a local failure is mapped toerrCancelContentProcessingbefore theerrInvalidChainwrapping, and no longer drops the peer that served the blocks. Both insertion paths do this —importBlockResults(full sync) andcommitFastSyncData(fast sync) — and both local conditions are covered: an interruption, and a stopping chain, whichInsertChainandInsertReceiptChainreport when they cannot take the chain lock, notably while the node is shutting down.refactor(core): share the fork-choice rule and let the caller write the new head: no behaviour change on its own. It extractsblockBeatsHeadand makesreorgleave the new head to its caller, both of which the known-block adoption above needs; it also folds the duplicated XDPoS head side effects intoisGapBlock/notifyEpochSwitchBlock/cacheSigningTxsand renamesinsertSidechainas upstream did.Tests
core/blockchain_test.go: a batch that fails in the middle through header verification and through body validation; a fully verified batch; the already-on-disk case; the known-block-ahead-of-head case; an interruption seen mid-batch and at the entry guard;TestInsertChainAdoptsKnownBatchAheadOfHead(a rolled-back batch is adopted back instead of leaving the head behind);TestInsertSideChainReportsInvalidBlockInSegment(a pruned segment holding a block whose body does not match its header is a failure, not a success);TestIsLocalInsertError.eth/downloader/downloader_test.go:TestImportBlockResultsKeepsPeerOnInterruption,TestImportBlockResultsKeepsPeerOnStoppedChainandTestCommitFastSyncDataKeepsPeerOnStoppedChain— neither an interruption nor a stopping chain may becomeerrInvalidChain, the error classSynchronisedrops the peer for, on either sync mode.Trade-off
ErrInsertionInterruptedandErrChainStoppedwidens thecoreAPI. Keeping the unexported aliases means no internal call site has to change.reorgrefuses, e.g. the XDPoS committed-block guard — is only logged and still normalised to success, because those blocks were already on disk and blaming the peer for them would be wrong.UpdateM1failure inside the adoption returns an error instead of thelog.Critthe import paths use: this is a best-effort recovery step, not a consensus-critical write.Verification
go build ./...,go vet ./core/ ./eth/downloader/go test ./core/ ./eth/downloader/ -count=1make all,make tidy,make generateCloses #2534