fix(eth/downloader): cap the ancestor search at the local head, close #2533 - #2547
fix(eth/downloader): cap the ancestor search at the local head, close #2533#2547gzliudan wants to merge 1 commit into
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 configurationConfiguration used: defaults Review profile: CHILL Plan: Team 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
There was a problem hiding this comment.
🔵 Needs a closer look
Ancestor selection affects every synchronization mode and warrants final human validation despite strong regression coverage.
Pull request overview
Caps downloader ancestor discovery at the local head, preventing stored side-chain blocks above it from causing skipped imports and stalled syncs.
Changes:
- Adds head-aware ancestor validation and bounded binary search.
- Reworks header sampling and gap refinement.
- Adds extensive full/fast-sync regression and boundary tests.
File summaries
| File | Description |
|---|---|
eth/downloader/downloader.go |
Bounds ancestor search and validates candidates. |
eth/downloader/downloader_test.go |
Covers above-head blocks, search bounds, and edge cases. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…n head, close XinFinOrg#2533 Side chain blocks are written by hash without state, so a node whose head sits below them still answers HasBlock for the whole segment; findAncestor then resolved the ancestor above its own head and the sync resumed there, stranding the range in between. On XDPoS this stalls the head for good: epoch-switch header verification needs the gap block's masternode snapshot, which is only written when the gap block imports with state. Cap both searches at the local head. calculateRequestSpan now always asks for two samples spaced one skipped header apart, taking its top sample from the highest block below the remote head and clamping it down to the local head, so the window never spends a round trip on a block the head guard would reject; findAncestor bounds the binary search with the same limit. The window keeps its count when 'from' is clamped up to zero, which can top it out above the head, so usableAsAncestor still rejects those candidates and keeps a span hit from being returned above the head. A hit on the lower sample no longer ends the search: the true fork can sit on the skipped block, so the hit seeds the binary search with the gap under the next sample, clamped by the local head and by the remote height. Covered by blocks-above-head tests in full and fast sync (stubs stored by hash, carrying receipts in fast sync but never entering the canonical chain, so the snap head stays pinned), span candidate rejection and the head-0 and head-2 window clamps, the gap refinement, a remote sitting at genesis, a binary-search fork variant, per-mode head-guard boundaries, the write-without-state semantics the bug rests on, and the per-case request span and window max.
224775f to
e8a6613
Compare
Summary
findAncestorcould resolve the common ancestor above the local chain head and resume the sync there, stranding the range in between. This caps both the span search and the binary search at the local head, so no candidate above it is ever sampled, accepted, or probed.closes #2533
Motivation & Context
Side chain blocks are written by hash without state, so a node whose head sits below them still answers
HasBlockfor the whole segment.findAncestorpicked up one of those blocks as the common ancestor and the downloader resumed from there, leaving the blocks between the real head and that block unimported. On XDPoS the head then stalls for good: header verification at an epoch switch needs the masternode snapshot of the skipped block, and that snapshot is only written when the block is imported with state. The oldcalculateRequestSpanalso derived its window fromlocalHeight-1inuint64, which underflows at head 0.Changes
calculateRequestSpanis rewritten around a fixed two-sample window: the sampling top is the highest block below the remote head, clamped down to the local head, and the request iscount=2, skip=1over three consecutive blocks withmax = from+2. The variablespan/countarithmetic, including itsMaxHeaderFetch/16cap, is gone.usableAsAncestorhelper adds the local-head guard to the per-mode known-block check and replaces the two inlineswitch modeblocks infindAncestor, so both the span search and the binary search reject above-head candidates.findAncestorintroducesancestorLimitExclusive = localHeight+1and caps the binary search interval with it, so a head far below the remote anchors in one round trip instead of burning the search on candidates that cannot be accepted.min(gap, ancestorLimitExclusive, remoteHeight+1). A hit at the local head or at the window top still returns as is.Testing
go test -count=1 ./eth/downloader/passes (7.4s) on the pushed commit, on top of themake all,go test -race -count=1 ./eth/downloader/andgo test -count=1 -short ./eth/... ./core/...runs recorded by the review of the same tree. New and extended cases cover: blocks above the head in full and fast sync (TestFindAncestorIgnoresBlocksAboveHead,...Fast), span candidate rejection (TestFindAncestorSpanRejectsAboveHeadCandidate,...Fast), the head-0 and head-2 window clamps (TestFindAncestorSpanAnchorsAtHeadZero,...HeadTwo), the gap refinement (TestFindAncestorSpanRefinesBelowLowestSample), a remote sitting at genesis (TestFindAncestorSpanRemoteAtGenesis), a binary-search fork variant (TestFindAncestorBinarySearchIgnoresBlocksAboveHead), per-mode head-guard boundaries (TestUsableAsAncestorBoundaries), the write-without-state semantics the bug rests on (TestWriteBlockWithoutState*), and the per-case request span and window max (TestRemoteHeaderRequestSpan). The above-head cases assert on the request sequence, so a probe above the local head or above the peer's head fails them.Related Issues
Risk & Impact
Touches
findAncestorandcalculateRequestSpan, which run in every sync mode (full, fast, light) at the start of every sync round, so a regression shows up immediately as a sync that cannot start or that re-downloads from too far back. The sampling geometry changed from a variablespan/countsized by the height difference to two samples spaced one skipped header apart: normal catch-up and small-lag scenarios still resolve in one round trip, deep forks keep the same order of probes, and the clamped window (fromclamped up to zero) can top out above the head, which is whyusableAsAncestorstays as a backstop. The binary search stays convergent:end = checkshrinks strictly and the interval is bounded bylocalHeight+1.Notes for Reviewers
The invariant to check is that neither search ever touches a block above the local head: the window top is clamped in
calculateRequestSpan, the binary interval is capped byancestorLimitExclusive, the gap refinement is capped by both the local head and the remote height, andusableAsAncestorrejects anything that still slips through. Suggested reading order:calculateRequestSpanand its doc comment for the window geometry, then the span-hit refinement infindAncestor, thenusableAsAncestor.TODO
None.