Repository navigation
ibd: refill an empty header queue from the stored tip - #1015
Conversation
An empty ordered queue during IBD was asked from the walk checkpoint, so the reply never landed on the queue. A connecting peer also got no getheaders while the node was in IBD, and a headers_done latch stayed shut when a peer was more than two blocks ahead.
An empty queue asked from the unrestored checkpoint, so the reply never landed on the download path. A headers_done latch with no proved walk also stayed shut when a peer was more than two blocks ahead, and a connecting peer sent no initial getheaders during IBD. Ask from the stored tip while the queue has room, write a short refill that continues that tip, reopen an unproved latch, and keep Core's one initial headers sync.
There was a problem hiding this comment.
Summary
The locator change is the right shape for the restart hole: while the ordered queue is under the soft cap, header_ask_locator names the stored tip before the candidate, and a full queue still starts at the walk tip (a_full_queue_next_ask_starts_at_the_walk_tip matches that). walk_successor_indexed does not skip that hole. It looks up the header after the candidate, and a contiguous height_to_hash cannot hold that successor while the middle is missing, so send_getheaders still asks. Connect-time headers sync is still one peer while the tip header is older than 24h (try_start_headers_sync), and AddrFetch still returns before that. The two defects are downstream of the new accept and unlatch predicates. A short or checkpoint-divergent reply that merely continues the stored tip is written onto the work path as if it were the candidate, and a proved walk already at the confirmed tip unlatches whenever any non-quiet peer advertises a higher start height, which drops the latch ibd_caught_up uses to ignore a far version.start_height.
Issue counts by severity
- bugs: 2
- suggestions: 0
- nits: 0
| // further than this reply. | ||
| PathAgree::Short => { | ||
| if stored_top_hash(st, hub) == Some(prev) { | ||
| return headers.len(); |
There was a problem hiding this comment.
[bug] A proved, above-floor refill that continues the stored tip is now stored and enqueued even when it is not the candidate. plan_header_reply classifies prev == stored_top_hash as Refill and does not call suppress_competing_chain. reject_refill_miss returns false for PathAgree::Short (line 1662), and when ordered is empty it returns false before the path_agree match at all (no tail, so PathAgree::Diverges never reaches offer_alternate). proven_header_prefix then returns headers.len() for that Short arm, and the existing Diverges arm does the same. try_set_path_slot is first-wins, so those hashes occupy the gap. The walk checkpoints are not popped, but the next locator starts at ordered.back() (header_ask_locator). An honest peer that does not have the fork skips to the candidate and sends headers after the walk tip, not the real gap. settle_walk_peer treats the longer queue as an extension, so the peer is not retired. This is the restart ask the patch just enabled: the first batch after an empty queue is exactly the one reject_refill_miss does not judge. A same-chain prefix that later hits the checkpoint is fine; a lighter prefix that stops short of the next checkpoint, or a full window that mismatches it while the queue is still empty, is stored as the download path.
Suggestion: Keep the honest short prefix, but do not commit it with first-wins until it meets the next checkpoint. On Short, and on Diverges when ordered is empty, either leave the headers off height_to_hash or record the anchor they extended and rewind_above that anchor when a later batch from it disagrees. Until that checkpoint is met, put the anchor in the locator ahead of any unproved queue tail so a peer that lacks the fork still answers with the real gap.
| if lag == 0 || has_tip_plus_one(st, tip_h) { | ||
| return false; | ||
| } | ||
| !super::header_walk::headers_complete(st, tip_h) |
There was a problem hiding this comment.
[bug] !headers_complete is broader than "a latch that never proved a walk." headers_complete is false when the walk tip is above the confirmed tip (the hole this patch should reopen) and also when any non-quiet live peer has peer_height above a proved walk that is already at the confirmed tip (wants_lookahead). header_lag_behind_peers still uses max_peer_height, so a far version.start_height unlatches. The new should_unlatch_headers_done test never inserts a slot, and prove_through does not set a tip hash, so wants_lookahead stays false and the signet assertion does not cover a connected peer. After the unlatch, header_poll will not LatchDone while empty_path_header_fan sees lag > 2. A full 2000-header reply returns before settle_walk_peer can insert walk_quiet, so headers_complete stays false. headers_done is only set again by an empty reply or LatchDone. ibd_caught_up then stays false (headers_complete false and lag > 2 requires headers_done), and the main loop keeps enqueueing HeaderCmd::Ask on the empty-path cadence. A proved walk above the confirmed tip with a real hole should still unlatch; that part is not this bug. A proved walk at the confirmed tip must not.
Suggestion: Unlatch a proved walk only when its tip is above the confirmed tip or above max_ordered_height (the missing download path). When the walk is proved and tip_height() <= tip_h, keep the old rule: do not clear headers_done for lag > 2. Add a slot whose peer_height is far above that tip to should_unlatch_headers_done and expect no unlatch. Re-latch, or leave the latch set, when a full window fails to extend the candidate so catch-up can still finish.
…t fill the gap A connected start height unlatches a walk that is already proved at the confirmed tip. A headers window that disagrees with the next checkpoint is written into an empty queue.
A connected peer's start height unlatched a walk already proved at the confirmed tip, so catch-up never stuck. An empty queue also stored a batch that missed the next checkpoint, and the next ask then started there. A proved walk at the confirmed tip opens only for a one- or two-block hole. A short refill is still queued. A checkpoint miss is not, and a later batch that meets the checkpoint replaces that guess. The locator names the proved anchor under an unproved queue.
Summary
Overnight core-functional and a restarted node both left the block queue at 0 during IBD.
headers_donelatch that never proved a walk stayed shut when a peer was more than two blocks ahead. Regtest peers with a real chain therefore never got another headers ask from the walk.getheaderswhile the tip was older than 24 hours meant a fresh node never requested the chain those peers already had. An unknown block inv during IBD noted a challenge and then did not ask.While the queue has room, the locator now starts at the stored tip, then the candidate and a lighter fork. A short refill that continues the stored tip is written onto the queue. An unproved latch opens again when a peer is ahead. A proved walk at the confirmed tip still does not chase a far
start_height. One connecting peer still starts headers sync during IBD. After the tip is inside 24 hours, further connecting peers start as before. An unknown block inv during IBD still challenges the walk and can sendgetheaders.Tests
getheadersfor the stored tip. A second peer does not. A new peer gets it once the tip is recent.getheadersand notes the walk challenge.Local QC passed: fmt, clippy
-D warnings,cargo deny, ast-grep, and the QC script self-tests. The workspace suite is left to the GitHubtestjob.