ibd: fetch again a BadPrev that races its parent commit

changelog.d/bad-prev-parent-commit-race.md

@@ -0,0 +1,9 @@
+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. It does not count toward the tip-child
+  three-strike halt, since the tip has not moved.

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_is_hub_tip(st, hub, hash);
+    // 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,24 +1214,50 @@ pub fn apply_soft_wire_reject(
     }
 }
 
-/// True when `hash` chains directly off the hub tip.
+/// 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_is_hub_tip(
+pub fn bad_prev_parent(
     st: &IbdWorkState,
     hub: Option<&rbitcoin_net::chain::ChainHub>,
     hash: BlockHash,
-) -> bool {
+    height: u32,
+) -> BadPrevParent {
     let Some(h) = hub else {
-        return false;
-    };
-    let Some(tip) = h.tip_hash() else {
-        return false;
+        return BadPrevParent::OffPath;
     };
-    st.path_prev
+    let Some(prev) = st
+        .path_prev
         .get(&hash)
         .copied()
         .or_else(|| h.prev_of(&hash))
-        == Some(tip)
+    else {
+        return BadPrevParent::OffPath;
+    };
+    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

crates/rbitcoin-ibd/tests/ibd/events/confirm_reject_tests.rs

@@ -296,6 +296,47 @@ 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. 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");
+    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);
+    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);
+}
+
 /// 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 +3046,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 +3094,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();
 }