From 2bb7d039d53a446315abbd03e34a848c0c940a24 Mon Sep 17 00:00:00 2001 From: russeree Date: Tue, 20 Oct 2026 09:15:20 -0700 Subject: [PATCH 1/2] ibd: fetch again a BadPrev that races its parent commit Load can stamp tip+2 while tip+1 is still being written (drain_fk at tip+1, fence at tip). That BadPrev can reach IBD before tip+1 Accepted, so prev_is_hub_tip saw the old tip, took the block for a fork, evicted it from the path, and never asked for it again. Catch-up stalled with asked=[] at tip+2. A BadPrev whose parent is this path's own block at height - 1 is not a fork, so prev_continues_own_path now keeps it on the path and fetches it again. Here it still shares the tip-child strike counter; the next commit takes it off that counter. batched_pin_engine_fault_retries_one_block_at_a_time discarded confirm events after the mend, so a dropped tip+2 could never be fetched. It now pumps IBD, which serves that fetch. Run as 120 parallel copies it failed 2 of 720 runs before and 0 of 600 after. Co-Authored-By: Claude Opus 5.5 --- changelog.d/bad-prev-parent-commit-race.md | 8 +++ crates/rbitcoin-ibd/src/ibd/events/mod.rs | 24 +++++--- .../tests/ibd/events/confirm_reject_tests.rs | 55 +++++++++++++++---- 3 files changed, 67 insertions(+), 20 deletions(-) create mode 100644 changelog.d/bad-prev-parent-commit-race.md diff --git a/changelog.d/bad-prev-parent-commit-race.md b/changelog.d/bad-prev-parent-commit-race.md new file mode 100644 index 0000000000..0bf197c368 --- /dev/null +++ b/changelog.d/bad-prev-parent-commit-race.md @@ -0,0 +1,8 @@ +Fixed + +- **A BadPrev that races its parent's commit is fetched again.** Load can + stamp tip+2 while tip+1 is still being written, and that reject can + reach IBD before tip+1's `Accepted`. IBD then treated the block as a fork, + dropped it from the path, and never asked for it again, so catch-up + stalled. A BadPrev whose parent is the path's own block at the height + below is now fetched again like a tip child. diff --git a/crates/rbitcoin-ibd/src/ibd/events/mod.rs b/crates/rbitcoin-ibd/src/ibd/events/mod.rs index 4b4dee6eb2..6cdda3e9eb 100644 --- a/crates/rbitcoin-ibd/src/ibd/events/mod.rs +++ b/crates/rbitcoin-ibd/src/ibd/events/mod.rs @@ -1126,7 +1126,7 @@ pub fn apply_soft_wire_reject( let bad_prev = super::reorg::is_bad_prev_err(err); // A BadPrev whose parent is the current tip is a missing header slot, // not a competing fork. Keep the hash so densify asks for it again. - let retry_tip_child = bad_prev && prev_is_hub_tip(st, hub, hash); + let retry_tip_child = bad_prev && prev_continues_own_path(st, hub, hash, height); if bad_prev { st.headers_done = false; if let Some(h) = hub { @@ -1200,24 +1200,32 @@ pub fn apply_soft_wire_reject( } } -/// True when `hash` chains directly off the hub tip. +/// True when `hash` chains off the hub tip, or off this path's own block at +/// `height - 1`. Load may stamp a child while its parent is still being +/// written, and that reject can arrive before the parent's `Accepted`. #[doc(hidden)] -pub fn prev_is_hub_tip( +pub fn prev_continues_own_path( st: &IbdWorkState, hub: Option<&rbitcoin_net::chain::ChainHub>, hash: BlockHash, + height: u32, ) -> bool { let Some(h) = hub else { return false; }; - let Some(tip) = h.tip_hash() else { - return false; - }; - st.path_prev + let Some(prev) = st + .path_prev .get(&hash) .copied() .or_else(|| h.prev_of(&hash)) - == Some(tip) + else { + return false; + }; + h.tip_hash() == Some(prev) + || height + .checked_sub(1) + .and_then(|below| st.height_to_hash.get(&below)) + == Some(&prev) } /// Core `MaybePunishNodeForBlock` (`BLOCK_MUTATED`): drop the sender and diff --git a/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs b/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs index 36d572411f..1d2a74afb4 100644 --- a/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs +++ b/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs @@ -296,6 +296,42 @@ fn bad_prev_on_the_tip_child_is_fetched_again() { let _ = std::fs::remove_dir_all(dir); } +/// Load can stamp tip+2 while tip+1 is still committing, and its BadPrev +/// can reach IBD before tip+1's `Accepted`. The parent is this path's own +/// block at the height below, so it is not a fork: the hash is fetched +/// again the same as a tip child. Dropping it left `asked` empty. +#[test] +fn bad_prev_racing_its_parents_commit_is_fetched_again() { + let (dir, hub) = rbitcoin_net::chain::tiny_regtest_hub_labeled("bad-prev-parent-commit"); + hub.ensure_genesis().unwrap(); + let tip = hub.tip_hash().unwrap(); + let t = hub.tip_height().unwrap(); + let (parent, hash) = (h(0x45), h(0x46)); + let mut st = IbdWorkState::new(Vec::new(), Some(tip), Some(t)); + st.record_height(parent, t + 1); + st.note_path_prev(parent, tip); + st.record_height(hash, t + 2); + st.note_path_prev(hash, parent); + st.densify_scan_lo = t + 5; + st.densify_ready_through = t + 5; + apply_confirm_reject( + &mut st, + t + 2, + hash, + "consensus: unexpected previous header", + Some(hub.query.as_ref()), + Some(&hub), + ); + assert_eq!(st.height_to_hash.get(&(t + 2)), Some(&hash)); + assert!(st.body.is_missing(&hash)); + assert!( + st.densify_scan_lo <= t + 2, + "scan_lo={}", + st.densify_scan_lo + ); + let _ = std::fs::remove_dir_all(dir); +} + /// A tip-child BadPrev that a new body cannot fix stops after three tries. /// The hash is not fetched again and is not blacklisted. #[test] @@ -3005,13 +3041,14 @@ fn load_engine_fault_near_the_tip_retakes_the_wave() { /// tip+1 and tip+2 load in one wave, and pin cannot read the coinbase /// outputs they spend. A pin-stage engine fault can be a stale-plan race, /// so the retry runs one block at a time and names tip+1 alone. Once the -/// outputs read again, both blocks connect. +/// outputs read again, both blocks connect. A load reject may drop tip+2 +/// for a fetch from peers (`docs/concurrency.md`), so IBD runs after the +/// mend and serves that fetch. #[test] fn batched_pin_engine_fault_retries_one_block_at_a_time() { use bitcoin::ScriptBuf; use rbitcoin_consensus::mine_regtest_paying; use rbitcoin_ibd::ibd::state::InflightReq; - use std::time::{Duration, Instant}; let mut rig = WireRig::new("pin-engine-fault", 1); let (t, cbs) = (rig.t, rig.cbs.clone()); @@ -3052,16 +3089,10 @@ fn batched_pin_engine_fault_retries_one_block_at_a_time() { (h1, ConfirmRejectClass::EngineFault, 1) ] ); - let rx = &rig.engine.as_ref().unwrap().1; - let deadline = Instant::now() + Duration::from_secs(20); - while rig.hub.tip_hash() != Some(h2) { - assert!( - Instant::now() < deadline, - "stall at {:?}", - rig.hub.tip_height() - ); - let _ = rx.recv_timeout(Duration::from_millis(5)); - } + rig.pump( + |_, hash| if hash == h1 { b1.clone() } else { b2.clone() }, + |_, hub, _| hub.tip_hash() == Some(h2), + ); rig.finish(); } From bb9b9f2ee0e50f7063e5e911f00fd8b29e7e2935 Mon Sep 17 00:00:00 2001 From: russeree Date: Tue, 20 Oct 2026 11:14:54 -0700 Subject: [PATCH 2/2] ibd: keep a committing-parent BadPrev off the tip-child strikes Review follow-up. prev_continues_own_path folded "parent is the hub tip" and "parent is the path block at height - 1, still committing" into one retry_tip_child, so the second case also ran apply_tip_child_bad_prev. Those strikes are keyed by (hash, hub tip), and in this race the tip has not moved, so a third BadPrev would set st.halt and stop IBD. bad_prev_parent now returns BadPrevParent::{Tip, Committing, OffPath}. Only Tip takes the strikes. Committing is re-fetched without one and leaves cascade_at alone, and the log names which case it was. bad_prev_racing_its_parents_commit_is_fetched_again applies the reject four times with a frozen tip and asserts no halt and the hash still missing; it failed on the parent commit with "a committing parent is not a strike". Co-Authored-By: Claude Opus 5.5 --- changelog.d/bad-prev-parent-commit-race.md | 3 +- crates/rbitcoin-ibd/src/ibd/events/mod.rs | 86 +++++++++++++------ .../tests/ibd/events/confirm_reject_tests.rs | 41 +++++---- 3 files changed, 84 insertions(+), 46 deletions(-) diff --git a/changelog.d/bad-prev-parent-commit-race.md b/changelog.d/bad-prev-parent-commit-race.md index 0bf197c368..497fed4541 100644 --- a/changelog.d/bad-prev-parent-commit-race.md +++ b/changelog.d/bad-prev-parent-commit-race.md @@ -5,4 +5,5 @@ Fixed reach IBD before tip+1's `Accepted`. IBD then treated the block as a fork, dropped it from the path, and never asked for it again, so catch-up stalled. A BadPrev whose parent is the path's own block at the height - below is now fetched again like a tip child. + below is now fetched again. It does not count toward the tip-child + three-strike halt, since the tip has not moved. diff --git a/crates/rbitcoin-ibd/src/ibd/events/mod.rs b/crates/rbitcoin-ibd/src/ibd/events/mod.rs index 6cdda3e9eb..3ae8e324fd 100644 --- a/crates/rbitcoin-ibd/src/ibd/events/mod.rs +++ b/crates/rbitcoin-ibd/src/ibd/events/mod.rs @@ -1125,8 +1125,14 @@ pub fn apply_soft_wire_reject( ) { let bad_prev = super::reorg::is_bad_prev_err(err); // A BadPrev whose parent is the current tip is a missing header slot, - // not a competing fork. Keep the hash so densify asks for it again. - let retry_tip_child = bad_prev && prev_continues_own_path(st, hub, hash, height); + // and one whose parent is still committing is a stale stamp. Neither is + // a competing fork. Keep the hash so densify asks for it again. + let parent = if bad_prev { + bad_prev_parent(st, hub, hash, height) + } else { + BadPrevParent::OffPath + }; + let retry_tip_child = parent != BadPrevParent::OffPath; if bad_prev { st.headers_done = false; if let Some(h) = hub { @@ -1172,26 +1178,34 @@ pub fn apply_soft_wire_reject( // also clears a count for another hash: the tip+1 retry got a // verdict, so the pipeline is not stuck repeating that cascade. // A tip-child BadPrev is fetched again as the same hash. It has its - // own counter: this clear must not restart those three strikes. - if retry_tip_child { - apply_tip_child_bad_prev(st, height, hash, err, hub); - if st.halt.is_some() { - st.body.stop_reget(hash); - return; + // own counter: this clear must not restart those three strikes. A + // committing parent is not a strike: the tip has not moved yet. + match parent { + BadPrevParent::Tip => { + apply_tip_child_bad_prev(st, height, hash, err, hub); + if st.halt.is_some() { + st.body.stop_reget(hash); + return; + } } - } else { - st.cascade_at = None; + BadPrevParent::Committing => {} + BadPrevParent::OffPath => st.cascade_at = None, } st.body.mark_missing(hash); st.reopen_for_densify(&[hash]); st.body.demote_known(hash); st.reopen_for_densify(&[hash]); - if retry_tip_child { - warn!( + match parent { + BadPrevParent::Tip => warn!( "ibd: confirm reject BadPrev @{height} {hash}: {err} (parent is tip, re-getdata)" - ); - } else { - warn!("ibd: confirm reject soft @{height} {hash}: {err} (re-getdata, not blacklisted)"); + ), + BadPrevParent::Committing => warn!( + "ibd: confirm reject BadPrev @{height} {hash}: {err} \ + (parent is the path block still committing, re-getdata)" + ), + BadPrevParent::OffPath => warn!( + "ibd: confirm reject soft @{height} {hash}: {err} (re-getdata, not blacklisted)" + ), } } else { warn!( @@ -1200,18 +1214,30 @@ pub fn apply_soft_wire_reject( } } -/// True when `hash` chains off the hub tip, or off this path's own block at -/// `height - 1`. Load may stamp a child while its parent is still being -/// written, and that reject can arrive before the parent's `Accepted`. +/// Where a BadPrev block's parent sits relative to this IBD path. +#[doc(hidden)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum BadPrevParent { + /// The hub tip: a missing header slot, under the tip-child strikes. + Tip, + /// This path's own block at `height - 1`, not the tip yet. Load stamped + /// the child while that parent was still being written, and the reject + /// arrived before the parent's `Accepted`. The tip has not moved, so it + /// is not a strike. + Committing, + /// Anything else: a competing fork. + OffPath, +} + #[doc(hidden)] -pub fn prev_continues_own_path( +pub fn bad_prev_parent( st: &IbdWorkState, hub: Option<&rbitcoin_net::chain::ChainHub>, hash: BlockHash, height: u32, -) -> bool { +) -> BadPrevParent { let Some(h) = hub else { - return false; + return BadPrevParent::OffPath; }; let Some(prev) = st .path_prev @@ -1219,13 +1245,19 @@ pub fn prev_continues_own_path( .copied() .or_else(|| h.prev_of(&hash)) else { - return false; + return BadPrevParent::OffPath; }; - h.tip_hash() == Some(prev) - || height - .checked_sub(1) - .and_then(|below| st.height_to_hash.get(&below)) - == Some(&prev) + if h.tip_hash() == Some(prev) { + BadPrevParent::Tip + } else if height + .checked_sub(1) + .and_then(|below| st.height_to_hash.get(&below)) + == Some(&prev) + { + BadPrevParent::Committing + } else { + BadPrevParent::OffPath + } } /// Core `MaybePunishNodeForBlock` (`BLOCK_MUTATED`): drop the sender and diff --git a/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs b/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs index 1d2a74afb4..ecc4f883d8 100644 --- a/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs +++ b/crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs @@ -299,7 +299,8 @@ fn bad_prev_on_the_tip_child_is_fetched_again() { /// Load can stamp tip+2 while tip+1 is still committing, and its BadPrev /// can reach IBD before tip+1's `Accepted`. The parent is this path's own /// block at the height below, so it is not a fork: the hash is fetched -/// again the same as a tip child. Dropping it left `asked` empty. +/// again. Dropping it left `asked` empty. The tip does not move during the +/// race, so repeats do not take the tip-child strikes and cannot halt IBD. #[test] fn bad_prev_racing_its_parents_commit_is_fetched_again() { let (dir, hub) = rbitcoin_net::chain::tiny_regtest_hub_labeled("bad-prev-parent-commit"); @@ -312,23 +313,27 @@ fn bad_prev_racing_its_parents_commit_is_fetched_again() { st.note_path_prev(parent, tip); st.record_height(hash, t + 2); st.note_path_prev(hash, parent); - st.densify_scan_lo = t + 5; - st.densify_ready_through = t + 5; - apply_confirm_reject( - &mut st, - t + 2, - hash, - "consensus: unexpected previous header", - Some(hub.query.as_ref()), - Some(&hub), - ); - assert_eq!(st.height_to_hash.get(&(t + 2)), Some(&hash)); - assert!(st.body.is_missing(&hash)); - assert!( - st.densify_scan_lo <= t + 2, - "scan_lo={}", - st.densify_scan_lo - ); + for _ in 0..4 { + st.densify_scan_lo = t + 5; + st.densify_ready_through = t + 5; + apply_confirm_reject( + &mut st, + t + 2, + hash, + "consensus: unexpected previous header", + Some(hub.query.as_ref()), + Some(&hub), + ); + assert!(st.halt.is_none(), "a committing parent is not a strike"); + assert_eq!(st.height_to_hash.get(&(t + 2)), Some(&hash)); + assert!(st.body.is_missing(&hash)); + assert!( + st.densify_scan_lo <= t + 2, + "scan_lo={}", + st.densify_scan_lo + ); + } + assert_eq!(hub.tip_hash(), Some(tip)); let _ = std::fs::remove_dir_all(dir); }