From b949829ba9eb12b8574c982c829c8863e2fdcedc Mon Sep 17 00:00:00 2001 From: Nitesh Balusu Date: Sun, 18 Oct 2026 17:25:34 -0400 Subject: [PATCH] net: reconsiderblock connects restored paths as one branch reconsiderblock re-applied each restored path on its own. Each path had to outweigh the tip by itself, and a path whose parent sat in a refused path failed with "branch parent not on chain". A chain that only outweighed the tip as a whole stayed inactive. A restored block that lost was also forgotten, so a later invalidateblock could not fall back to it. Restored paths now join parent-first into branches, and the heaviest connects first, still from their recorded block lists. A restored tip that loses and came from the archive goes into fork_tips, so it is a valid fork and a fallback for invalidateblock, as in Core. Header-only and connected blocks are not added. Co-Authored-By: Claude Opus 5.5 --- changelog.d/reconsider-most-work.md | 7 ++ crates/rbitcoin-net/src/chain.rs | 189 ++++++++++++++++++++++++++-- 2 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 changelog.d/reconsider-most-work.md diff --git a/changelog.d/reconsider-most-work.md b/changelog.d/reconsider-most-work.md new file mode 100644 index 000000000..561018522 --- /dev/null +++ b/changelog.d/reconsider-most-work.md @@ -0,0 +1,7 @@ +Fixed + +- **`reconsiderblock` connects restored paths that continue one another as + one branch**, as in Core. Each path used to be re-applied on its own, so a + chain that only outweighed the tip as a whole stayed inactive. A restored + block that loses stays a valid fork, so a later `invalidateblock` can fall + back to it. diff --git a/crates/rbitcoin-net/src/chain.rs b/crates/rbitcoin-net/src/chain.rs index 4e9fc8b72..4d6cae84a 100644 --- a/crates/rbitcoin-net/src/chain.rs +++ b/crates/rbitcoin-net/src/chain.rs @@ -2179,8 +2179,13 @@ impl ChainHub { } } + // Core `ResetBlockFailureFlags` + `ActivateBestChain`. Restored paths + // that continue one another connect as one branch, heaviest first. + let mut paths = paths; + paths.sort_by_key(|path| path.first().and_then(|h| self.stored_header_height(h))); + let mut branches: Vec> = Vec::new(); for path in paths { - let mut branch = Vec::new(); + let mut blocks = Vec::new(); for h in &path { if self.is_connected(h) { continue; @@ -2188,23 +2193,47 @@ impl ChainHub { let Some(b) = self.load_side_body(h) else { break; }; - branch.push(b); + blocks.push(b); } - if !branch.is_empty() { - match self.accept_branch_inner(&branch) { - Err(NetError::Protocol(s)) if s.contains("branch parent not on chain") => {} - Err(e) => return Err(e), - Ok(_) => {} - } + let Some(parent) = blocks.first().map(|b| b.header.prev_blockhash) else { + continue; + }; + match branches + .iter_mut() + .find(|branch| branch.last().map(Block::block_hash) == Some(parent)) + { + Some(branch) => branch.extend(blocks), + None => branches.push(blocks), } } - if !self.is_connected(&hash) { + let restored = branches.iter().flatten().any(|b| b.block_hash() == hash); + if !restored && !self.is_connected(&hash) { if let Some(branch) = self.assemble_side_branch(hash) { - match self.accept_branch_inner(&branch) { - Err(NetError::Protocol(s)) if s.contains("branch parent not on chain") => {} - Err(e) => return Err(e), - Ok(_) => {} + branches.push(branch); + } + } + let mut ranked = Vec::with_capacity(branches.len()); + for branch in branches { + let Ok(work) = self.branch_chain_work(&branch) else { + continue; + }; + ranked.push((work, branch)); + } + ranked.sort_by_key(|(work, _)| std::cmp::Reverse(*work)); + for (_, branch) in ranked { + match self.accept_branch_inner(&branch) { + // A restored archive tip that loses stays a valid fork, so a + // later invalidateblock can fall back to it. + Ok(AcceptOutcome::IgnoredWeaker) => { + if let Some(tip) = branch.last().map(Block::block_hash) { + if self.held_body(&tip).is_none() { + self.fork_tips.write().unwrap().insert(tip); + } + } } + Err(NetError::Protocol(s)) if s.contains("branch parent not on chain") => {} + Err(e) => return Err(e), + Ok(_) => {} } } Ok(()) @@ -4251,6 +4280,140 @@ mod tests { let _ = std::fs::remove_dir_all(dir); } + /// Core `reconsiderblock` clears the marks, then activates the most-work + /// chain. Here the restored a8..a10 outweighs b8..b9 only as a whole. + #[test] + fn reconsider_activates_the_most_work_chain() { + let (dir, hub) = tmp_hub(); + hub.ensure_genesis().unwrap(); + let gen = hub.tip_hash().unwrap(); + let t0 = hub.tip_header().unwrap().time; + let mut a = Vec::new(); + let mut prev = gen; + for h in 1..=10u32 { + let b = mine(prev, t0 + h, h); + prev = b.block_hash(); + a.push(prev); + hub.accept_block(b).unwrap(); + } + hub.invalidate_block(a[9]).unwrap(); + hub.invalidate_block(a[7]).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[6])); + let b8 = mine_distinct(a[6], t0 + 20, 8, &[a[7]]); + let b9 = mine(b8.block_hash(), t0 + 21, 9); + hub.accept_block(b8).unwrap(); + hub.accept_block(b9).unwrap(); + hub.reconsider_block(a[7]).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[9])); + let _ = std::fs::remove_dir_all(dir); + } + + /// A block a reconsider made valid again is still a fallback when a later + /// `invalidateblock` shrinks the chain (Core's `InvalidateBlock` re-adds + /// every valid block with enough work as a candidate). + #[test] + fn invalidate_falls_back_to_a_reconsidered_block() { + let (dir, hub) = tmp_hub(); + hub.ensure_genesis().unwrap(); + let gen = hub.tip_hash().unwrap(); + let t0 = hub.tip_header().unwrap().time; + let mut a = Vec::new(); + let mut prev = gen; + for h in 1..=5u32 { + let b = mine(prev, t0 + h, h); + prev = b.block_hash(); + a.push(prev); + hub.accept_block(b).unwrap(); + } + hub.invalidate_block(a[4]).unwrap(); + let b5 = mine_distinct(a[3], t0 + 20, 5, &[a[4]]); + let b5_hash = b5.block_hash(); + let b6 = mine(b5_hash, t0 + 21, 6); + hub.accept_block(b5).unwrap(); + hub.accept_block(b6).unwrap(); + hub.reconsider_block(a[4]).unwrap(); + assert_eq!(hub.tip_height(), Some(6)); + hub.invalidate_block(b5_hash).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[4])); + let _ = std::fs::remove_dir_all(dir); + } + + /// An invalid held side branch does not stop reconsider: the restored + /// path still connects. + #[test] + fn reconsider_restores_past_an_invalid_held_branch() { + let (dir, hub) = tmp_hub(); + hub.ensure_genesis().unwrap(); + let gen = hub.tip_hash().unwrap(); + let t0 = hub.tip_header().unwrap().time; + let mut a = Vec::new(); + let mut prev = gen; + for h in 1..=4u32 { + let b = mine(prev, t0 + h, h); + prev = b.block_hash(); + a.push(prev); + hub.accept_block(b).unwrap(); + } + let mut bad = mine(a[0], t0 + 20, 2); + let cb = &mut bad.txdata[0].output[0].value; + *cb = Amount::from_sat(cb.to_sat() + 1); + bad.header.merkle_root = bad.compute_merkle_root().unwrap(); + rbitcoin_consensus::grind_regtest_pow(&mut bad.header); + let b3 = mine(bad.block_hash(), t0 + 21, 3); + let b4 = mine(b3.block_hash(), t0 + 22, 4); + for b in [bad, b3, b4] { + let _ = hub.accept_received_block(b); + } + assert_eq!(hub.tip_hash(), Some(a[3])); + let _ = hub.invalidate_block(a[1]); + assert_eq!(hub.tip_hash(), Some(a[0])); + hub.reconsider_block(a[1]).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[3])); + let _ = std::fs::remove_dir_all(dir); + } + + /// Reconsider makes only a restored archive tip that loses a fork tip. A + /// header-only descendant stays `headers-only`, and a connected block + /// is never listed as a fork. + #[test] + fn reconsider_keeps_header_only_and_connected_blocks_out_of_forks() { + let (dir, hub) = tmp_hub(); + hub.ensure_genesis().unwrap(); + let gen = hub.tip_hash().unwrap(); + let t0 = hub.tip_header().unwrap().time; + let mut a = Vec::new(); + let mut prev = gen; + for h in 1..=3u32 { + let b = mine(prev, t0 + h, h); + prev = b.block_hash(); + a.push(prev); + hub.accept_block(b).unwrap(); + } + let a4 = mine(a[2], t0 + 4, 4); + hub.ensure_header(&a4.header).unwrap(); + hub.invalidate_block(a[1]).unwrap(); + hub.reconsider_block(a4.block_hash()).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[2])); + let status = |hash: BlockHash| { + hub.chaintips() + .into_iter() + .find(|t| t.hash == hash) + .map(|t| t.status) + }; + assert_eq!(status(a4.block_hash()), Some("headers-only")); + + hub.reconsider_block(a[1]).unwrap(); + let mut prev = a[0]; + for h in 2..=4u32 { + let b = mine_distinct(prev, t0 + 20 + h, h, &a); + prev = b.block_hash(); + let _ = hub.accept_received_block(b); + } + assert_eq!(hub.tip_hash(), Some(prev)); + assert_eq!(status(a[1]), None); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn invalid_child_missing_parent_and_linked_reconsider() { let (dir, hub) = tmp_hub();