Skip to content

net: IBD leaves a header chain whose block fails consensus - #1112

Merged
reardencode merged 17 commits into
reardencode:masterfrom
smolcars:net/ibd-invalid-heavier
Oct 21, 2026
Merged

reardencode merged 17 commits into
reardencode:masterfrom
smolcars:net/ibd-invalid-heavier

Conversation

@niteshbalusu11

@niteshbalusu11 niteshbalusu11 commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1082.

During IBD, when the most-work header chain held a consensus-invalid block, rbitcoin rejected the block but kept that chain as its candidate and download path. It never fetched a lighter valid chain from another peer, asked the honest peer for the bad chain's blocks (a Core peer stays silent and was dropped for a 10 minute cooldown), and after a restart forgot the block was invalid and rewound a valid tip to chase the bad chain again.

This follows the direction in the issue: retire the failed candidate, ask peers only for blocks on a chain they announced, and keep the invalid mark across restart.

Repro (regtest, same setup as the issue: 110 shared blocks, honest V to 160, heavier I to 190 with one coinbase-excess block)

Bad block in I Core 31.1 rbitcoin before rbitcoin after after restart, honest peer only
111 160 stuck at 111 160 (~2 s) 160 (before: rewound to 110)
150 160 149 160 (~4 s, rewinds 149 → 110 in IBD) 160 (before: 149)
181 180 180 180 –

No stall-kick of the honest Core peer in the after runs. (Core 31.1 syncs headers from one peer during IBD; in some runs it picked the split peer and never saw V, ending at the split chain's valid prefix. The Core column is from runs where it did see V.)

Commits

Written against the pre-#1097 layout, then ported onto the rbitcoin-ibd crate split and rebased on current master.

  1. net: give the header peer a block no other peer has. A slot remembers the hashes it answered notfound (until it announces more). One may_serve check in issue_batch replaces the blanket "reserved header peer gets no blocks" rule: the header peer is asked when no other live peer may have the block. Before this, a heavier chain only the header peer had stalled IBD against rbitcoin seeders, which answer notfound.
  2. net: retire an IBD header candidate whose block fails consensus. retire_invalid drops the download path above the bad block's parent (queue, body queue, confirm plan, densify cursor) and cuts the walk candidate back to that parent (cut_back_to, shared with adopt_heavier). A header batch that holds or builds on a rejected block is refused and its peer leaves the walk, so peers are asked again from the parent; a heavier valid chain is adopted and the existing most-work rewind runs when it forks below the confirmed tip.
  3. net: keep a failed connect invalid across restart. The hub's invalid set stays the one owner. A connect failure (IBD reject or tip-follow failed accept) is also kept in store/block.invalid; reconsiderblock clears it. IBD seeds its reject set from the hub, so the resume seed and the walk refuse that chain, and the startup heavier-chain scan excludes those hashes. IBD stops classifying "timestamp too far in future" as consensus-invalid (requeued, as tip-follow and Core do), so it is never persisted.
  4. net: ask each IBD block peer for headers once. Through the existing block-inv challenge, one non-reserved block peer at a time is asked for headers, so a peer's chain is known before blocks are asked of it. Each slot keeps its announced chain above the tip (at most one headers reply); a peer whose chain left the path, or whose short reply ends below a block, is not asked for it.
  5. net: close the gaps review found in the IBD retire. See below.
  6. net: keep a peer's newest announced chain and settle its split. Maintainer review: a split whose height left the path is settled against the confirmed chain before block assignment; the newest announced run is kept; invalid_tips and block.invalid keep the newest entry at their caps.
  7. net: a header peer that shows a block is kept from it only by one that does. A peer with no record for a block no longer keeps the header peer from a block the header peer's announced chain shows; invalid_tips evicts oldest-first.
  8. net: write a failure before the retired walk, and never show a left path. Maintainer review: shows is false wherever left_path_by is true; the failure is written before the retire saves header.adopt.
  9. net: save no walk while a failure is not on disk, and let a notfound expire. A pending mark is retried and blocks header.adopt until it lands; a notfound lasts one stall window.
  10. net: probe a peer from the confirmed tip, and rewind onto a fork below it. Maintainer review: a short reply's top is where that peer's chain ends, also at or below the tip. The probe asks from the confirmed chain only, so an empty reply means nothing above the tip; an empty reply to a walk ask (which also fits a caught-up peer) is probed again. The probe reply is read into the peer's record, not the walk, and a probe needs a second live peer. The Initial sync stays stuck when the most-work header chain has an invalid block #1082 repro against Core then found that after a retire a heavier chain forking below the confirmed tip left IBD hung (now rewound to from the main loop), and that a rewind left peers' records on the old chain (now split at the fork).
  11. net: keep an open probe's lane, and keep a rewound peer on the chain it showed. Second review of 10: an open probe keeps the lane and the header poll asks once it is past its window, so it expires; a peer with an unanswered expired walk ask is not probed; a rewind splits a peer onto the old chain only if it announced nothing on the new one, and keeps a split the peer showed at or below the fork.
  12. net: refuse only a linked header batch on a rejected block. Review (coreyphillips): an unlinked batch naming a rejected block put an honest header in invalid_tips. Only a batch that passes pow_linked is refused or remembered.
  13. net: clear a peer's split once it builds on this chain above it. A reply on this node's block clears a split at or below it; the kept run continues only from its first new header's parent.
  14. net: resume a saved walk past stored failures below its base. header.adopt is refused only for a failure above the saved base, or one whose height is not found.
  15. net: wait on a block whose time is too far ahead instead of halting IBD. A time-too-new reject holds the confirm lookup for 1 s and does not count toward the cascade halt, so a clock stepped back no longer halts IBD; the block connects once the clock catches up.
  16. net: ask an IBD block peer only for blocks its chain showed. Core's rule, as the issue asks: a peer is asked only for a block its record shows, with a fallback when no peer shows it. The probe asks after the path top first, so one reply covers the 64k queue for a caught-up peer, and a stale record is refreshed.
  17. net: re-probe a peer whose record a rewind left stale. A rewind no longer infers a block for a peer whose record took the rewound chain as its own: that record is stale from the fork up until the peer answers again, and the peer is probed at once. Its own split is kept.

Schema addition (no version bump)

store/block.invalid is a new sidecar next to header.adopt: magic rbtcinv1, u32 count (at most 4096), 32-byte hashes. Missing or unparseable is an empty set, so old datadirs open unchanged. Mutated bodies, a future timestamp, store and engine faults, and operator invalidateblock are not written (operator marks stay in-process, noted in COMPAT.md). Documented in SCHEMA.md.

Tests

  • New live journey end_of_ibd_invalid_heavier_fork (three real P2PNode miners; the split miner never halves the subsidy, so from height 20 its taller chain is coinbase excess to the syncer): keeps a heavier valid prefix, rewinds a confirmed prefix for a heavier honest chain, survives two restarts without chasing the stored split headers, and finishes on a fork at the invalid block.
  • startup_tip_follow_journey: a heavier stored header listed in block.invalid does not hold startup in catch-up.
  • two_peers_reserve_the_header_walk chapters for notfound, the probe, announced-chain eligibility, the liveness fallback, and the commit-time refusal; restart_restores_the_checkpoint_skip_and_garbage_does_not chapters for the retired tip and the stored-invalid restore refusal; reconsider_skips_a_branch_that_fails pins the sidecar write, the reconsider prune, and that an operator-invalidated block is not written; the classifier table pins time-too-new. Chapters six and seven: the probe asks from the confirmed tip, an empty probe or walk reply, a crossing announcement, an open probe's lane and expiry, and peers' records across a rewind onto a fork below the tip. Units: a_reply_at_or_below_the_tip_ends_the_chain_there and a_third_chain_split_survives_two_rewinds (peer chain), a_walk_candidate_forked_below_the_tip_is_rewound_to (reorg).

Review

An independent review (GPT-6-Astra, high) of commits 1–4 raised seven findings; all seven were real and are fixed in commit 5 with a pin each (the fsync change has no in-process observable): a batch committed after its block failed could undo a retire; a retired tip was not saved to header.adopt; the failure mark was written before the walk was saved, and a stale header.adopt could bring back an invalid candidate; a peer was asked above a short reply's top, and its split was forgotten once that height left the path; an operator invalidation could be persisted through a P2P retry; the sidecar write was not synced.

Commits 10 to 17 were reviewed in seven more GPT-6-Astra passes: an open probe that lost its timeout lane, a late walk reply that could be read as a probe reply, a rewind that split a peer off the chain it had announced, a peer's split lost across two rewinds and an inferred block lost or contradicted at later rewinds (now a stale mark and a re-probe), a probe-expiry pin that bypassed the header poll, and a future-time guard the confirm worker bypassed. Each is fixed (in 11, 15, and 17) with a pin that fails without it. Commits 12 to 16 answer coreyphillips' review of e2914ba.

Not in this PR

  • A fork deeper than IBD's rewind cap (REWIND_MAX_DEPTH, 1024) or tip-follow's 288-block window (finding 088 F3). The header-thread rewind still does not consider a walk candidate that forks below the confirmed tip; the main-loop rewind does.
  • header_ancestry_invalid's 10,000-step cap, Core's bad-prevblk marking at header accept, persisting operator invalidateblock, and getheaders to every peer near the tip.
  • Disconnecting the sender of a consensus-invalid IBD body (Core's MaybePunishNodeForBlock).

🤖 Generated with Claude Code

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The retire path, block.invalid durability, and the time-too-new requeue match the issue: a consensus failure cuts the download path back to the parent, reconsiderblock drops the sidecar, operator invalidation is not written, and a torn destination file is unlikely because the sidecar is a synced temp plus rename. The new peer-chain filter does not stay true for the peer IBD just switched to, and it does not keep a chain longer than one headers reply. Both mistakes send block getdata to a peer whose announced chain does not have the block, which is the stall this PR is fixing.

Issue counts by severity

  • bugs: 2
  • suggestions: 1
  • nits: 0

Comment thread crates/rbitcoin-ibd/src/ibd/peer_chain.rs Outdated
Comment thread crates/rbitcoin-ibd/src/ibd/peer_chain.rs Outdated
Comment thread crates/rbitcoin-ibd/src/ibd/header_walk.rs Outdated
niteshbalusu11 added a commit to smolcars/rbitcoin that referenced this pull request Oct 20, 2026
Review on reardencode#1112:

- A recorded split whose height had been confirmed and pruned from the
  path still counted, so the peer whose chain IBD had just taken was no
  longer asked for its blocks. A missing path slot no longer counts by
  itself: before block assignment the split is compared with this
  node's confirmed block there; the same block clears it, a different
  one keeps it.
- The announced run kept the oldest reply: a continuation was cut off
  at one reply, the next full reply landed in a gap and was dropped,
  and a later short reply still set `complete`, so the peer looked
  finished at a stale top. The newest run is kept now, and `complete`
  describes it.
- With no peer that holds a block by what it announced, the fallback
  skipped the header peer while another peer was up. It no longer does.
- `invalid_tips` starts again at its cap, so the newest refused tip of a
  long invalid chain is kept, and a full `block.invalid` drops its
  oldest hash so the newest failure survives restart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
niteshbalusu11 added a commit to smolcars/rbitcoin that referenced this pull request Oct 20, 2026
…t does

Second review pass on reardencode#1112 found three ways a peer with no record
either way for a block could still exclude the reserved header peer
that announced it: an announced window that no longer reaches that
height after the candidate changed, a split cleared before a later
rewind, and a reply that never recorded where the peer's chain ends.
Each let a silent peer take the block while the peer that has it waited.

Once the header peer's announced chain shows a block (that block, or a
path block above it), only another peer whose announced chain shows it
keeps the header peer from it. While the header peer has shown nothing,
it still stays on headers when another peer may hold the block. A peer
with no record may still be asked, as before this branch.

`invalid_tips` now drops its oldest tip at the cap instead of starting
over, so a branch refused after the oldest tips is still refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@niteshbalusu11
niteshbalusu11 force-pushed the net/ibd-invalid-heavier branch from c8297b2 to 4c6a843 Compare October 20, 2026 20:19
@niteshbalusu11

Copy link
Copy Markdown
Contributor Author

@reardencode thanks for the review. All three threads are fixed and resolved:

  • 7cf7969 net: keep a peer's newest announced chain and settle its split. A split whose height left the path is settled against the confirmed chain before block assignment; the newest announced run is kept and complete describes it; both caps keep the newest entry.
  • 4c6a843 net: a header peer that shows a block is kept from it only by one that does.

Rebased onto current master (a89303e). Locally green: cargo test --workspace, fmt, clippy -D warnings, deny, ast-grep, ci-os-smoke.sh.

A few notes beyond the three threads:

  • I ran a second independent review pass (GPT-6-Astra) over the fixes. It found the same false positive in three more shapes: an announced window that no longer reaches the height after the candidate changes, a split cleared before a later rewind, and a reply that never records where the peer's chain ends. In each, a peer with no record for a block could still keep the header peer from it. Rather than three special cases, 4c6a843 makes that exclusion depend on positive evidence: once the header peer's own announced chain shows a block (that block, or a path block above it), only another peer whose chain shows it keeps the header peer from it. While the header peer has shown nothing, it still stays on headers, so block_getdata_skips_the_reserved_header_peer is unchanged. A peer with no record can still be asked, as before this branch; the stall detector handles one that stays silent.
  • invalid_tips drops its oldest tip at the cap rather than clearing, so a branch refused after the oldest tips is not forgotten when the set fills.
  • Each fix has a pin that fails without it. The chapter-4 beat you flagged (dropping unconfirmed heights and expecting exclusion) is removed; the fifth chapter covers the adopt-and-prune case instead.
  • Still out of scope, as in the description: IBD itself rewinding onto a heavier fork that forks below already-pruned confirmed heights. Tip-follow finishes that case today within its 288-block window; doing it in IBD needs the header-thread and main-loop rewinds serialized, so I would rather take that as a follow-up.

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The three threads from the last review hold on 4c6a8435: a split whose height has left the path is settled against the confirmed header, the retained headers run is the newest one, and both caps drop the oldest entry. Time-too-new stays a requeue, and an operator invalidateblock is not what remember_failed_accept writes. Two holes are still open. Either one brings back the stall in #1082.

Issue counts by severity

  • bugs: 2
  • suggestions: 0
  • nits: 0

Comment thread crates/rbitcoin-ibd/src/ibd/peer_chain.rs
Comment thread crates/rbitcoin-ibd/src/ibd/events/mod.rs Outdated
niteshbalusu11 added a commit to smolcars/rbitcoin that referenced this pull request Oct 20, 2026
Review on reardencode#1112:

- A recorded split whose height had been confirmed and pruned from the
  path still counted, so the peer whose chain IBD had just taken was no
  longer asked for its blocks. A missing path slot no longer counts by
  itself: before block assignment the split is compared with this
  node's confirmed block there; the same block clears it, a different
  one keeps it.
- The announced run kept the oldest reply: a continuation was cut off
  at one reply, the next full reply landed in a gap and was dropped,
  and a later short reply still set `complete`, so the peer looked
  finished at a stale top. The newest run is kept now, and `complete`
  describes it.
- With no peer that holds a block by what it announced, the fallback
  skipped the header peer while another peer was up. It no longer does.
- `invalid_tips` starts again at its cap, so the newest refused tip of a
  long invalid chain is kept, and a full `block.invalid` drops its
  oldest hash so the newest failure survives restart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
niteshbalusu11 added a commit to smolcars/rbitcoin that referenced this pull request Oct 20, 2026
…t does

Second review pass on reardencode#1112 found three ways a peer with no record
either way for a block could still exclude the reserved header peer
that announced it: an announced window that no longer reaches that
height after the candidate changed, a split cleared before a later
rewind, and a reply that never recorded where the peer's chain ends.
Each let a silent peer take the block while the peer that has it waited.

Once the header peer's announced chain shows a block (that block, or a
path block above it), only another peer whose announced chain shows it
keeps the header peer from it. While the header peer has shown nothing,
it still stays on headers when another peer may hold the block. A peer
with no record may still be asked, as before this branch.

`invalid_tips` now drops its oldest tip at the cap instead of starting
over, so a branch refused after the oldest tips is still refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
niteshbalusu11 added a commit to smolcars/rbitcoin that referenced this pull request Oct 20, 2026
Review on reardencode#1112:

- `shows` could be true where `left_path_by` was also true: a peer that
  announced the path above the tip and then sent a batch forking at or
  below the tip kept its old run. If the header peer also showed the
  block, that stale `shows` kept the header peer off it while the stale
  peer was not asked either, and no getdata went out. `shows` is now
  false wherever the peer left the path, and a reply that moves the
  split without landing above the tip drops the kept run.
- The retire saved `header.adopt` before `block.invalid` was written. A
  crash or a failed write in between left the retired walk on disk with
  no mark, so the next start chased the invalid chain again. The failure
  is written first now; `restore_adopt` already refuses a saved candidate
  above the tip while the file lists a block. If the write fails, the
  retire still holds in this process and `header.adopt` is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
niteshbalusu11 added a commit to smolcars/rbitcoin that referenced this pull request Oct 20, 2026
…expire

Third review pass on reardencode#1112:

- When `block.invalid` could not be written, the retire still saved
  `header.adopt` before removing it, and a later walk save could bring it
  back. A failure that was not on disk was also reported as kept the
  second time and never written again. The hub now tracks a pending
  mark: every later mark (and a reconsider prune) tries the write again,
  and `save_adopt` saves no walk, removing an existing one, until the
  mark is on disk. The removal syncs the directory.
- A `notfound` kept a peer off a block until it announced again, so a
  block every peer had answered `notfound` was never asked for again. It
  now lasts one stall window (30 s).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@niteshbalusu11
niteshbalusu11 force-pushed the net/ibd-invalid-heavier branch from 4c6a843 to e2914ba Compare October 20, 2026 21:07
@niteshbalusu11

Copy link
Copy Markdown
Contributor Author

@reardencode both new threads are fixed and resolved:

  • 59ed0e5 net: write a failure before the retired walk, and never show a left path
  • e2914ba net: save no walk while a failure is not on disk, and let a notfound expire

Rebased onto current master (6740431, after #1115). Locally green: cargo test --workspace, fmt, clippy -D warnings, deny, ast-grep, ci-os-smoke.sh, and the new qc steps (cargo check --release in both shapes, cargo doc, the journeys manifest).

Notes:

  • A third GPT-6-Astra pass over 59ed0e5 agreed the stale-shows fix closes your scenario and found three failure-path gaps, all fixed in e2914ba: a header.adopt that a later walk save could bring back after a failed mark write; a failed mark reported as kept on the second call and never written again; and a notfound that kept a peer off a block until it announced again, so a block every peer had answered notfound was never asked for again. A notfound now lasts one stall window (30 s).
  • With shows implying holds, a peer that keeps the header peer off a block is itself eligible for it, and with no holder the fallback asks any peer outside its notfound window, so a needed block has a peer to ask while any peer is up.
  • Known limit, in SCHEMA.md: a restart while block.invalid still cannot be written has no record of the block and validates it again.

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The current head does retire an IBD candidate whose block fails consensus, writes that failure before header.adopt, and keeps peer shows / left_path_by, the announced window, and the invalid-hash caps consistent with the last review. Those five earlier defects are fixed in the code, not only in the replies. What remains is that a headers answer which does not extend above the confirmed tip is stored as "no chain," so that peer is still asked for later path blocks and can be stall-disconnected — the #1082 failure mode for the common empty reply.

Issue counts by severity

  • bugs: 2
  • suggestions: 0
  • nits: 0

Comment thread crates/rbitcoin-ibd/src/ibd/events/mod.rs
Comment thread crates/rbitcoin-ibd/src/ibd/peer_chain.rs
@coreyphillips

Copy link
Copy Markdown

Two independent reviews.

needs changing before merge

  • An unlinked headers batch can mark an honest header as an invalid tip (crates/rbitcoin-ibd/src/ibd/header_walk.rs:843). refuse_invalid_batch records the last header of any refused batch in invalid_tips without checking that the batch is linked. One crafted headers message therefore makes every later batch that contains that honest header get refused. The IBD reader forwards headers straight to plan_header_batch (peer_io.rs:471) with no continuity or PoW check. refuse_invalid_batch (header_walk.rs:843) runs before classify and pow_linked. A batch is refused when first.prev_blockhash is in reorg.invalid, and that is only a header field. Its last header then goes into invalid_tips (line 855). After that, the headers.iter().any(rejected) check refuses any batch that contains that hash. The honest sender is moved to walk_quiet and loses the reservation. The same check runs again at commit in on_headers_batch. The attacker only needs one hash in reorg.invalid. That can be the block it served itself (the issue's threat model), or any entry seeded from block.invalid. In the issue's height-111 case: after the retire, the split peer sends [header{prev: I111}, V111]. The honest peer's V111..V160 reply is then refused, and IBD stays at 110, as it did before this PR. Confirmed with a scratch test in a copy of HEAD (tests/ibd/header_walk.rs helpers): rust st.reorg.invalid.mark(bad.block_hash().to_byte_array()); let junk = Header { prev_blockhash: bad.block_hash(), ..v1 }; // no PoW apply(&mut st, &hub, 0, vec![junk, v2]); assert!(send_getheaders(&mut st, &hub).unwrap()); // honest peer reserved apply(&mut st, &hub, 1, vec![v1, v2, v3]); // control (no junk): tip=3, honest reserved // poisoned: tip=0, honest in walk_quiet, reservation gone Requiring prev-linkage across the batch (for example pow_linked) before refusing or recording closes this. With linkage enforced, an honest header can never be the last header of a batch that descends from a rejected hash.
  • Unlinked headers can mark an unrelated valid chain invalid (crates/rbitcoin-ibd/src/ibd/header_walk.rs:854). A peer can make an honest header unusable by appending it to an unlinked batch containing a rejected header. refuse_invalid_batch runs before linkage validation and caches the final hash. I confirmed through plan_header_batch that [bad_header, honest_header], with both headers sharing a parent, puts the honest hash in invalid_tips. A later valid reply containing it is refused and its peer is retired.
  • A full probe permits requests beyond the peer's announced chain (crates/rbitcoin-ibd/src/ibd/peer_chain.rs:192). A block peer that returns 2,000 shared headers remains eligible for later blocks whose ancestry it never announced. For a full reply, left_path_by compares requests above its top against only the last announced hash. The one-time probe does not continue. I confirmed that issue_one sends this peer a request for a fork's height-3,001 block after it announced only heights 1 through 2,000. An honest Core peer can therefore still receive requests for the invalid fork, stay silent, and incur the cooldown described in Initial sync stays stuck when the most-work header chain has an invalid block #1082.

worth doing, does not block

  • A peer that joins this node's chain keeps its old split for the session (crates/rbitcoin-ibd/src/ibd/peer_chain.rs:95). PeerChain::note_headers clears a recorded split only when a later reply includes a header at that exact height that matches (peer_chain.rs:95). A reply that builds on this node's block above the split proves the peer now has the path, but it does not clear the split. A locator-based reply after the peer reorgs starts above the split, so the split stays. Once the height is confirmed, settle_split sets split_confirmed and the peer counts as having left for good. Confirmed with a unit test: the peer announces a stale sibling at height 1, then announces ours[1..], whose first header's parent is ours[0]. Afterwards left_path_by(2) and left_path_by(3) are still true, and shows(3) is false. The effect is that the peer is not asked for blocks at or above the split while any other peer qualifies. A natural one-block race near the end of IBD is enough to trigger it.
  • A peer that has not announced a chain may still be asked for any block (crates/rbitcoin-ibd/src/ibd/header_walk.rs:547). In may_serve, a peer with no chain record counts as holding every block (holds at header_walk.rs:646). Until its probe reply lands, the honest peer can be asked for the split chain's blocks. In a scratch test, may_serve(honest, i1) was true before the honest reply arrived, with the split peer reserved. The faster split peer happened to win that request, so I did not reproduce the stall-kick itself. A later headers reply does not pull back requests already in flight, so a silent Core peer can still be stall-kicked. Separately, challenge_unprobed_peer calls note_probed() before the ask is sent. note_block_inv can overwrite the pending challenge, and take_challenge drops a peer in walk_quiet. In both cases the peer is never asked and is never probed again. This is narrower than the owner's direction to only ask a peer for a chain it has announced.
  • Any stored invalid block disables the header.adopt resume mid-IBD (crates/rbitcoin-ibd/src/ibd/header_walk.rs:3180). restore_adopt refuses a saved candidate above the tip whenever hub.invalid_blocks() is non-empty (header_walk.rs:3180). That includes a listed block far below the confirmed tip, which cannot be in a candidate whose base matches the confirmed chain. Once one failure has been persisted, every later restart during IBD skips the resume, and the milestone note at startup, until reconsiderblock. Refusing only when a listed hash's stored height is above the walk base would keep the guard. I did not measure the cost of the skipped resume.
  • Future timestamps exhaust the cascade retry limit (crates/rbitcoin-ibd/src/ibd/confirm/mod.rs:421). A stored header that becomes too new after a backward clock correction halts IBD instead of waiting. Cascade immediately requeues its body, while apply_cascade_reject halts after three rejects at an unchanged tip. I confirmed through spawn_confirm_engine that a stored future-dated header produces three automatic rejects and sets st.halt within milliseconds. Time-based retries should wait without consuming the stale-plan failure limit.

nits

  • Docs still describe the old notfound and header-peer rules (docs/ibd-memory.md:60). The changelog (changelog.d/ibd-invalid-heavier.md:15) and the TESTING.md P2P walls line say a notfound lasts until the peer announces more. Commit 9 also expires it after LACKS_TTL_MS (30 s). docs/ibd-memory.md:60 still says "Block download uses the other peers". It also does not list the new per-peer PeerChain state (up to 2,000 announced hashes and 1,024 notfound entries per peer) or the 1,024-entry invalid_tips set.

niteshbalusu11 and others added 14 commits October 21, 2026 14:17
IBD never sent block getdata to the reserved header peer while another
peer was connected, and a peer that answered `notfound` was asked again
for the same hash. When only the header peer had a heavier chain, every
getdata went to a peer that kept answering `notfound`, and IBD stalled.

Each IBD slot now remembers the hashes it answered `notfound` until it
announces more (a headers message or a block inv). A peer is not asked
for such a hash, and the reserved header peer is asked when no other
live peer may have the block. One `may_serve` check in `issue_batch`
replaces the blanket reserved-peer exclusion.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the most-work header chain held a consensus-invalid block, IBD
rejected the block but kept that chain as the walk candidate and the
download path. A lighter valid chain another peer served was never
fetched, and IBD stayed below the invalid block (issue reardencode#1082).

On a consensus-invalid body the walk now drops the path above the
block's parent (queue, body queue, confirm plan, densify cursor) and
moves the candidate back to that parent. A batch that holds or builds
on a rejected block is refused and its peer leaves the walk, so peers
are asked again from the parent and a heavier valid chain is adopted,
rewinding confirmed blocks when it forks below them. A challenger that
forks at or below the parent with more work is promoted.

Each IBD slot also remembers where its announced headers left the
download path, so a peer is not asked for path blocks above that point
and the header peer takes a block only it has.

`adopt_heavier` and the retire share `cut_back_to`. The live journey
`end_of_ibd_invalid_heavier_fork` stages the split with three miners.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Invalid marks were in memory only. After a restart the node forgot that
a stored header chain held a consensus-invalid block: the startup scan
saw that chain as more work, and the IBD resume seed planted it again,
rewinding a valid tip to chase it (issue reardencode#1082).

The hub's invalid set stays the one owner. A block that fails connect on
a consensus rule (IBD reject, or tip-follow's failed accept) is also
written to the `store/block.invalid` sidecar, at most 4096 hashes, and
`reconsiderblock` removes it. The hub loads the file at open, IBD seeds
its reject set from the hub, so the resume seed and the header walk
refuse that chain, and the startup heavier-chain scan excludes those
hashes. Mutated bodies, store faults, and operator `invalidateblock` are
not written. This is a schema addition without a version bump: a missing
or unparseable file is an empty set (SCHEMA.md).

IBD also stops treating a header timestamp too far in the future as
consensus-invalid. It is requeued, as in tip-follow and Core
(BLOCK_TIME_FUTURE), so it is never persisted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A peer that had announced nothing was asked for blocks of any chain. A
Core peer serving a lighter chain ignores getdata for blocks it does
not have, so it went quiet, was dropped, and cooled down for 10 minutes
before the node needed its chain (issue reardencode#1082).

Each block peer that has not announced a chain now gets one headers
ask, through the block-inv challenge (one peer at a time, never the
reserved header peer). Each slot keeps the hashes of its announced chain
above the confirmed tip, at most one headers reply. Two chains that
differ at one height differ above it, so one comparison at the lower of
the asked height and the peer's top tells whether the peer holds a path
block. A lighter fork in the reply also becomes the walk's challenger.

The retire no longer promotes a challenger. Its headers are not stored
and its fork can sit below the confirmed tip; peers are asked again from
the parent instead, and a heavier chain comes back as stored headers.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A review of this branch found these, each pinned before the fix:

- A header batch can be written outside the lock while one of its
  blocks fails. `on_headers_batch` now refuses a batch that holds or
  builds on a rejected block at commit too, so a retire is not undone.
- `retire_invalid` left no checkpoint at the parent, so `header.adopt`
  restored an older tip or none. It now records one.
- The failure mark was written before the retire saved the walk. The
  retire runs first, and `restore_adopt` does not bring back a saved
  candidate above the confirmed tip while invalid blocks are stored.
- A peer whose reply was shorter than a full batch has nothing past its
  top and is not asked above it. Where its chain split from this node's
  (path or confirmed) is kept when that height leaves the path. When no
  peer holds a block by what it announced, any peer that did not answer
  `notfound` may be asked, so a stale record cannot stall a download.
- A block refused because it was already marked invalid (an operator
  `invalidateblock`) is not written to `block.invalid`.
- `block.invalid` is written through the store's synced temp-file
  helper (sync, rename, directory sync). Durability is not observable
  in-process; the on-disk pins still cover the bytes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review on reardencode#1112:

- A recorded split whose height had been confirmed and pruned from the
  path still counted, so the peer whose chain IBD had just taken was no
  longer asked for its blocks. A missing path slot no longer counts by
  itself: before block assignment the split is compared with this
  node's confirmed block there; the same block clears it, a different
  one keeps it.
- The announced run kept the oldest reply: a continuation was cut off
  at one reply, the next full reply landed in a gap and was dropped,
  and a later short reply still set `complete`, so the peer looked
  finished at a stale top. The newest run is kept now, and `complete`
  describes it.
- With no peer that holds a block by what it announced, the fallback
  skipped the header peer while another peer was up. It no longer does.
- `invalid_tips` starts again at its cap, so the newest refused tip of a
  long invalid chain is kept, and a full `block.invalid` drops its
  oldest hash so the newest failure survives restart.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t does

Second review pass on reardencode#1112 found three ways a peer with no record
either way for a block could still exclude the reserved header peer
that announced it: an announced window that no longer reaches that
height after the candidate changed, a split cleared before a later
rewind, and a reply that never recorded where the peer's chain ends.
Each let a silent peer take the block while the peer that has it waited.

Once the header peer's announced chain shows a block (that block, or a
path block above it), only another peer whose announced chain shows it
keeps the header peer from it. While the header peer has shown nothing,
it still stays on headers when another peer may hold the block. A peer
with no record may still be asked, as before this branch.

`invalid_tips` now drops its oldest tip at the cap instead of starting
over, so a branch refused after the oldest tips is still refused.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review on reardencode#1112:

- `shows` could be true where `left_path_by` was also true: a peer that
  announced the path above the tip and then sent a batch forking at or
  below the tip kept its old run. If the header peer also showed the
  block, that stale `shows` kept the header peer off it while the stale
  peer was not asked either, and no getdata went out. `shows` is now
  false wherever the peer left the path, and a reply that moves the
  split without landing above the tip drops the kept run.
- The retire saved `header.adopt` before `block.invalid` was written. A
  crash or a failed write in between left the retired walk on disk with
  no mark, so the next start chased the invalid chain again. The failure
  is written first now; `restore_adopt` already refuses a saved candidate
  above the tip while the file lists a block. If the write fails, the
  retire still holds in this process and `header.adopt` is removed.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…expire

Third review pass on reardencode#1112:

- When `block.invalid` could not be written, the retire still saved
  `header.adopt` before removing it, and a later walk save could bring it
  back. A failure that was not on disk was also reported as kept the
  second time and never written again. The hub now tracks a pending
  mark: every later mark (and a reconsider prune) tries the write again,
  and `save_adopt` saves no walk, removing an existing one, until the
  mark is on disk. The removal syncs the directory.
- A `notfound` kept a peer off a block until it announced again, so a
  block every peer had answered `notfound` was never asked for again. It
  now lasts one stall window (30 s).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w it

Maintainer review: an empty headers reply, or one that ends at or below the
confirmed tip, left the peer looking as if it had announced nothing, so it
was still asked for path blocks and could be stall-disconnected.

- A short reply's top is kept as where the peer's chain ends, also when it
  is at or below the tip and the kept run is gone. `left_path_by` reads
  that end with an empty run.
- The probe asks from the confirmed chain only. An empty reply then means
  nothing above the tip, and that peer is not asked for path blocks. A
  walk ask carries the path top first, so an empty reply to it is a
  caught-up peer as well as one behind. That peer is probed again rather
  than marked.
- The probe reply goes into the peer's chain record, not the walk. One
  that shows a header the walk has not seen gives that peer the walk's own
  ask. A batch that crosses the probe leaves its ask open, and a peer with
  a walk ask out is not probed.

- A probe needs a second live peer. A record only chooses among peers;
  with one, the probe would take the header lane and its reply would not
  reach the walk, so the walk would not continue a full fork window.

Running the reardencode#1082 repro against Core found two more:

- After a retire, a heavier chain that forked below the confirmed tip could
  become the walk's candidate with nothing planted above the tip. IBD
  neither rewound nor exited. The main-loop rewind now walks back from
  that candidate.
- A rewind replaced the confirmed blocks a peer's record took as its
  chain, so a peer on the old chain was asked for the new one. A peer whose
  record starts past the fork splits there at the rewind.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it showed

A second review of the probe and rewind change found three gaps, each
pinned before the fix:

- With nothing left for the walk to ask, the lane was released under an
  open probe, so it never expired and no other peer was probed. An open
  probe now keeps the lane, and the header poll asks once its window
  passes, also with header sync otherwise done, so it can expire.
- A peer with an expired walk ask that the lane no longer tracked could be
  probed, and its late walk reply read as the probe's. A peer with an
  unanswered expired ask is not probed.
- At a rewind, a peer whose record started past the fork was split onto
  the old chain even when its announced headers were on the new one. The
  split is now inferred after the new chain is planted, and not for a peer
  that announced a header on it. A split the peer itself showed at or
  below the fork is kept, so a peer on a third chain stays off both chains
  across a rewind and back.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review: `refuse_invalid_batch` ran before any linkage or proof-of-work
check and remembered the last header of every batch it refused. One
`headers` message with a header naming a rejected block, followed by an
honest header, put that honest header in `invalid_tips`. After that, every
batch holding it was refused and its peer left the walk, so in the reardencode#1082
case IBD stayed at the fork as before this branch.

A batch is refused, and its last header remembered, only when it is one
linked chain that meets its own proof of work. An unlinked batch goes to
the usual checks, which drop it. A linked batch that builds on or holds a
rejected block cannot end in an honest header.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review: a recorded split was cleared only by a later reply holding a
header at that exact height. A reply that builds on this node's block
above the split proves the peer has this chain there, but the split
stayed, and once that height was confirmed the peer counted as having left
for good. A one-block race near the end of IBD was enough.

- A reply whose first header's parent is this node's block clears a split
  at or below that block.
- The kept run is continued only from the parent of the reply's first new
  header. A reply at the next height that built on another block was
  appended to the run, so the peer still looked as if it held the stale
  sibling.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review: `restore_adopt` refused a saved candidate above the tip whenever
`block.invalid` listed any block, also one far below the confirmed tip
that cannot be in a candidate whose base matches the confirmed chain. Once
one failure was stored, every later restart during IBD skipped the resume
until `reconsiderblock`.

The resume is refused only when a listed block is above the saved base,
or when its height is not found within 64 headers of the best chain.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@niteshbalusu11
niteshbalusu11 force-pushed the net/ibd-invalid-heavier branch from e2914ba to e241cd2 Compare October 21, 2026 18:18
@niteshbalusu11

niteshbalusu11 commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

@coreyphillips thanks, every point checked out. Each fix has a test that fails without it.

Needs changing

  • Unlinked batch marks an honest header invalid (both points). Fixed in a372775. refuse_invalid_batch refuses a batch, and records its last header, only if it passes pow_linked. An unlinked batch goes to the usual checks. Your [junk(prev: rejected), honest] and [rejected, honest sibling] shapes are both in a_long_chain_on_an_invalid_block_stays_refused, and the honest chain is still accepted afterwards.
  • A full probe reply leaves a peer eligible past what it announced. Fixed in 8bebe6d by moving to the owner's direction: a peer is asked only for a block its record shows.
    • With no peer that shows a block, a peer that hasn't left the path is asked, and as a last resort any peer outside its notfound window, so a stale record can't stop a download.
    • The header peer stays on headers while another peer can fetch the block.
    • To keep IBD parallel without a 2,000-header stride, the probe asks after the download path top alone. A reply on that top shows every path block below it, which covers the 64k queue.
    • An empty reply is ambiguous: a peer at the top, or an rbitcoin peer below minimum work. That, or a genesis fallback that doesn't pass the tip, is asked again from the confirmed tip. Core replies from genesis+1, rbitcoin from genesis itself, and both forms are pinned.
    • A peer whose record no longer covers the next 4,096 heights is probed again, at most every 10 s.
    • Your height-3,001 case is the eighth chapter of two_peers_reserve_the_header_walk.

Worth doing

  • A split that never clears. Fixed in 552a4b8. A reply whose first header builds on this node's block clears a split at or below that block. Your unit case, plus a reply that leaves again one height up, is a_reply_on_this_chain_above_a_split_clears_it. This also turned up a second bug: the kept run was extended by height alone, so a reply at the next height that built on another block was appended to it. It now continues only from the parent of the reply's first new header.
  • A peer with no record can be asked for any block. Fixed by the same change, 8bebe6d. The note_probed-before-send and walk_quiet drops you describe were fixed in 812f44c: the probe has its own slot, is marked when sent, and is not dropped for walk_quiet.
  • header.adopt resume disabled by any stored failure. Fixed in 07e3124 as you suggested. The resume is refused only when a stored failure is above the saved base. Its height comes from walking at most 64 headers back to the best chain, and an unknown height still refuses. SCHEMA.md is updated.
  • Future timestamps exhaust the cascade limit. Fixed in 39ec677. A time-too-new reject puts a 1 s hold on the confirm feed, and the lookup worker selects nothing while it holds. It doesn't count toward the cascade halt. a_block_too_far_ahead_waits_and_then_connects runs the real engine with a header stored just over 2 h ahead of the wall clock: no halt, a bounded retry rate, and the block connects once the clock passes it. a_block_too_far_ahead_does_not_count_toward_the_cascade_halt pins the exemption deterministically. Header acceptance already refuses such headers, so only a clock stepped back can leave one stored.

Nits

  • Fixed in 8bebe6d and 39ec677. TESTING.md's walls line and the changelog give the 30 s notfound window. docs/ibd-memory.md says how block peers are chosen and lists the per-peer record (2,000 announced hashes, 1,024 notfound entries, one split, one chain end, one path block, and the height a rewind left the record stale from) and the 1,024-entry invalid_tips.

Rerun of the #1082 repro against Core 31.1 on the new head: bad block at 111 → 160, at 150 → rewinds 149 → 110 inside IBD and reaches 160, at 181 → 180. Restarts stay at 160 and no peer is stall-kicked.

@niteshbalusu11

niteshbalusu11 commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

@reardencode both threads from your 14:27Z review are fixed and resolved. coreyphillips' review is answered in the comment below. The branch is rebased onto current master (b0c4c6a) and force-pushed.

New since e2914ba:

  • 812f44c: your two threads. A probe also now needs a second live peer: with one, it took the header lane and its reply didn't reach the walk. Upstream's new end_of_ibd_work_fork replay beat caught this after the rebase.
    • A short reply's top is where the peer's chain ends, also at or below the tip.
    • The probe has its own slot and isn't a walk answer.
    • Rerunning the Initial sync stays stuck when the most-work header chain has an invalid block #1082 repro against Core 31.1 found two more bugs, fixed here. After a retire, a heavier chain forking below the confirmed tip left IBD hung, and it is now rewound to from the main loop. A rewind also left peers' records on the old chain.
  • 490c446: GPT-6-Astra follow-ups. An open probe keeps the lane and expires, a peer with an unanswered expired walk ask isn't probed, and rewound peers' records are corrected.
  • a372775, 552a4b8, 07e3124, 39ec677, 8bebe6d: coreyphillips' points, one commit each.
  • 7be776a: a rewind no longer infers a peer's chain. A record that relied on the rewound blocks is stale from the fork up until the peer answers again, and the peer is re-probed.

Notes:

  • Empty reply: changed from your suggestion. An empty reply to a walk ask can't be read as "ends at the confirmed tip". The walk's locator starts at the queue tail and the candidate tip, so a caught-up peer answers empty too. Marking those peers would leave the header peer as the only block source on mainnet. Only an empty answer to a probe from the confirmed tip is read that way. The thread reply has the details.
  • Eligibility now follows Core's rule (8bebe6d). This is the issue's direction and coreyphillips' blocking point: a block is asked only of a peer whose record shows it.
    • Fallback: when no peer shows a block, a peer that hasn't left the path is asked first, then any peer.
    • Parallelism: the probe asks after the path top alone, so one reply covers the 64k queue for a caught-up peer. A stale record is re-probed at most every 10 s.
    • Upside: in the 150 case IBD now rewinds and finishes in about 4 s. Before, it exited at 149 and tip-follow finished in about 12 s.
  • Review: seven GPT-6-Astra passes over these commits found 14 more issues. Each is fixed with a pin that fails without it; three were removed with the rewind inference in 7be776a.
  • Local checks: cargo test --workspace passes, and so do fmt, clippy -D warnings, deny, ast-grep, ci-os-smoke.sh, both release cargo check shapes, cargo doc and the journeys manifest. Each commit passes fmt, clippy and the IBD suite on its own.
  • Repro against Core 31.1: bad block at 111 → 160, at 150 → 160, at 181 → 180. Restarts stay at 160 and no peer is stall-kicked.

niteshbalusu11 and others added 3 commits October 21, 2026 14:21
Review: this branch made a "timestamp too far in future" reject a requeue,
as in tip-follow and Core. But the confirm lookup selects the body again
at once, and `apply_cascade_reject` halts IBD after three rejects at the
same tip. A stored header that a backward clock correction left more than
2 hours ahead halted IBD within milliseconds.

- Such a reject puts a 1 s hold on the confirm feed, and the lookup
  selects nothing while it holds, so the block is tried about once a
  second instead of in a tight loop.
- It does not count toward the cascade halt. The block connects once the
  clock catches up.

Header acceptance already refuses such a header, so only a clock stepped
back can leave one stored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review: a block peer was eligible for any block it had not ruled out. A
peer with no record counted as holding every block, and one whose full
probe reply ended below a block stayed eligible above it, so an honest
Core peer could still be asked for a fork's blocks, stay silent, and take
the 10 minute cooldown in reardencode#1082. The issue's direction is Core's rule: ask
a peer only for a block on the chain it announced.

- `may_serve` asks a peer only for a block its record shows. With no peer
  that shows the block, a peer whose chain has not left the path is asked,
  else any peer outside its `notfound` window, so a stale record cannot
  stop a download. The header peer stays on headers while another peer can
  fetch the block, except in that last case.
- A probe asks for headers after the download path's top alone. A reply on
  that top shows every path block below it, up to the 64,000-header queue,
  so one probe covers a caught-up peer. An empty reply (also a peer below
  the minimum work), or one from genesis that stops at or below the tip,
  is asked again from the confirmed tip.
- A recorded split whose hash is now on the path shows that block too.
- A peer whose record no longer decides the next 4,096 heights is probed
  again, at most every 10 s.

docs/ibd-memory.md lists the per-peer record and `invalid_tips`, and says
how block peers are chosen.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of d00f2c50: at a rewind, a record that took the rewound blocks as
the peer's chain was given an inferred split at the fork, in place of a
split the peer had shown. A peer on a third chain that left this chain at
1999, then a rewind to 1997 and one back, looked eligible for blocks above
1999 it did not hold. Further review found that any inferred block can be
lost or contradicted across later rewinds and replies.

A rewind no longer infers a block. Such a record is stale from the fork up
until the peer answers again: the peer is not taken to hold a block there,
and it is probed at once. Only a reply that reaches the fork clears it; a
full reply that ends below the fork says nothing there, so the peer is
asked again. A later rewind keeps the lower fork. The peer's own split is
kept as it was. A record whose split is at or below the fork, or whose
announced headers are on the new chain, is not stale.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@niteshbalusu11
niteshbalusu11 force-pushed the net/ibd-invalid-heavier branch from e241cd2 to 7be776a Compare October 21, 2026 18:24

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

On a consensus-invalid body during IBD, this head retires that header and every header that builds on it, keeps the connected prefix only when it still has more work, and otherwise replants onto the honest challenger and downloads that chain. Block requests go to a peer whose record shows the hash; while the announcer of the heavier chain still shows those blocks, the honest peer is not asked for them and is not stall-cooled. A real connect failure is written to store/block.invalid before header.adopt is saved, so a restart with only the honest peer does not rewind a valid tip onto the stored invalid chain. Mutated bodies, time-too-new, store and engine faults, and operator invalidateblock stay out of that file, and reconsiderblock clears it. The author's fixes for an empty headers reply and for a short reply at or below the tip match the current may_serve, probe, and ends code and the tests that pin them. The last-resort ask of any peer, and an exact multiple of 2000 headers never recording ends, remain, and neither stalls the honest peer on the issue's two-peer timeline while the announcer still shows the blocks.

Issue counts by severity

  • bugs: 0
  • suggestions: 0
  • nits: 0

@reardencode
reardencode merged commit 9eabc08 into reardencode:master Oct 21, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Initial sync stays stuck when the most-work header chain has an invalid block

3 participants