From db2c43b16c181c018a6c8813d08e30bca76d7968 Mon Sep 17 00:00:00 2001 From: Nitesh Balusu Date: Mon, 19 Oct 2026 12:56:28 -0400 Subject: [PATCH 1/3] net: reconsiderblock activates the most-work chain Core's reconsiderblock clears the marks, then ActivateBestChain picks from every candidate. rbitcoin re-applied each restored invalidate path on its own. Paths that outweighed the tip only together stayed inactive, and a restored block that lost was forgotten as a fallback. Reconsider now ranks one branch per candidate tip: restored path ends, the reconsidered hash, fork tips, held bodies, and precious. It connects the most-work branch and tries the next untried tip when a block fails. Then each restored leaf that lost becomes a fork tip if every block to it was connected before: it came off the active chain (each recorded path now says so), or it is at or under a fork tip. A leaf under a fork tip or another kept leaf is skipped. A held child that never connected does not hide one. Restored blocks are walked past the 10,000-block cap. A store fault while ranking is returned. invalidateblock uses the same branch builder and ranking. Fixes #1080. Co-Authored-By: Claude Opus 5.5 --- changelog.d/reconsider-most-work.md | 8 + crates/rbitcoin-net/src/chain.rs | 352 ++++++++++++++---- .../src/regtest_chain_ops_journey.rs | 116 ++++++ 3 files changed, 400 insertions(+), 76 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 0000000000..85c12758a1 --- /dev/null +++ b/changelog.d/reconsider-most-work.md @@ -0,0 +1,8 @@ +Fixed + +- **`reconsiderblock` activates the most-work chain, as Core does.** Each + restored path used to be re-applied on its own, so a chain that only + outweighed the tip as a whole stayed inactive. A branch that fails to + connect no longer stops the next one. A restored leaf that loses, and + was connected before, now stays a valid fork, so a later + `invalidateblock` can return to it. diff --git a/crates/rbitcoin-net/src/chain.rs b/crates/rbitcoin-net/src/chain.rs index abb7e5f833..7242002b55 100644 --- a/crates/rbitcoin-net/src/chain.rs +++ b/crates/rbitcoin-net/src/chain.rs @@ -208,7 +208,15 @@ impl ForkTips { /// Uncapped. Forgetting an invalid hash would make that block acceptable again. struct Invalidated { set: RwLock>, - paths: RwLock>>, + paths: RwLock>, +} + +/// The hashes one `invalidateblock` marked, in height order. `connected` when +/// they came off the active chain, so every one passed full validation. +#[derive(Clone)] +struct InvalidPath { + hashes: Vec, + connected: bool, } impl Invalidated { @@ -1088,7 +1096,7 @@ impl ChainHub { } } for path in self.invalidated.paths.read().unwrap().iter() { - if let Some(h) = path.last().copied() { + if let Some(h) = path.hashes.last().copied() { record(&mut out, h, "invalid"); } } @@ -2094,7 +2102,10 @@ impl ChainHub { } } if !path.is_empty() { - self.invalidated.paths.write().unwrap().push(path); + self.invalidated.paths.write().unwrap().push(InvalidPath { + hashes: path, + connected: true, + }); } let keep = h.0.saturating_sub(1); self.disconnect_to(keep)?; @@ -2104,7 +2115,10 @@ impl ChainHub { // invalid without a tip disconnect. self.invalidated.set.write().unwrap().insert(hash); self.drop_held(hash); - self.invalidated.paths.write().unwrap().push(vec![hash]); + self.invalidated.paths.write().unwrap().push(InvalidPath { + hashes: vec![hash], + connected: false, + }); } else { return Err(NetError::Consensus("Block not found".into())); } @@ -2119,41 +2133,8 @@ impl ChainHub { /// the most total chain work. Equal work prefers precious, then the /// first-seen held tip. fn try_apply_after_invalidate(&self) -> Result, NetError> { - let inv = self.invalidated.set.read().unwrap().clone(); - let precious = *self.precious.read().unwrap(); - let mut starts: Vec = self.fork_tips.read().unwrap().iter().collect(); - starts.extend(self.held_bodies.read().unwrap().keys()); - if let Some(p) = precious { - if !starts.contains(&p) { - starts.push(p); - } - } - let mut best: Option<(bitcoin::Work, bool, u64, Vec)> = None; - for start in starts { - if inv.contains(&start) { - continue; - } - let Some(branch) = self.assemble_side_branch(start) else { - continue; - }; - if branch.iter().any(|b| inv.contains(&b.block_hash())) { - continue; - } - let tip = branch.last().map(|b| b.block_hash()).unwrap_or(start); - let Ok(w) = self.branch_chain_work(&branch) else { - continue; - }; - let is_p = Some(tip) == precious; - let seq = self.held_bodies.read().unwrap().seq(tip); - let take = match &best { - None => true, - Some((bw, was_p, bseq, _)) => held_branch_beats(w, is_p, seq, *bw, *was_p, *bseq), - }; - if take { - best = Some((w, is_p, seq, branch)); - } - } - let Some((_, _, _, branch)) = best else { + let none = HashSet::new(); + let Some(branch) = self.most_work_branch(self.side_branches(&[], &none, &none))? else { return Ok(None); }; match self.accept_branch_inner(&branch) { @@ -2165,8 +2146,9 @@ impl ChainHub { } } - /// Clear the invalid mark on `hash`, its invalidated path, and ancestors; - /// re-apply bodies from archive. Header-only descendants stay header tips. + /// Clear the invalid mark on `hash`, its invalidated paths, and ancestors, + /// then activate the most-work chain. Header-only descendants stay header + /// tips. pub fn reconsider_block(&self, hash: BlockHash) -> Result<(), NetError> { crate::tip_accept::run_on_tip_accept(|| self.reconsider_block_inner(hash)) } @@ -2205,7 +2187,7 @@ impl ChainHub { { let mut inv = self.invalidated.set.write().unwrap(); for path in &paths { - for h in path { + for h in &path.hashes { inv.remove(h); } } @@ -2214,42 +2196,152 @@ impl ChainHub { } } - for path in paths { - let mut branch = Vec::new(); - for h in &path { - if self.is_connected(h) { - continue; - } - let Some(b) = self.load_side_body(h) else { - break; - }; - branch.push(b); + let restored: HashSet = paths + .iter() + .flat_map(|p| p.hashes.iter().copied()) + .collect(); + let validated: HashSet = paths + .iter() + .filter(|p| p.connected) + .flat_map(|p| p.hashes.iter().copied()) + .collect(); + let mut leaves: Vec = paths + .iter() + .filter_map(|p| p.hashes.last().copied()) + .collect(); + leaves.push(hash); + self.apply_after_reconsider(&leaves, &restored, &validated) + } + + /// Core `ActivateBestChain` after `reconsiderblock`: connect the most-work + /// side branch, and try the next one when a block fails. Then keep the + /// restored leaves that lost as fork tips. + fn apply_after_reconsider( + &self, + leaves: &[BlockHash], + restored: &HashSet, + validated: &HashSet, + ) -> Result<(), NetError> { + // A failed tip may stay a candidate (a time reject is not a mark), so + // each tip is tried once. + let mut tried = HashSet::new(); + loop { + let untried = self.side_branches(leaves, restored, &tried); + let Some(branch) = self.most_work_branch(untried)? else { + break; + }; + let branch_tip = branch[branch.len() - 1].block_hash(); + tried.insert(branch_tip); + let attempt = { + let _guard = self.connect_lock.lock().unwrap_or_else(|e| e.into_inner()); + self.accept_branch_prefix(&branch) + }; + match attempt { + Ok((AcceptOutcome::Accepted { .. }, Some(_))) => {} + Ok(_) => break, + Err(NetError::Protocol(s)) if s.contains("branch parent not on chain") => break, + Err(e) if e.is_local_fault() => return Err(e), + Err(e) => self.remember_failed_accept(branch_tip, &e), } - 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(_) => {} - } + } + self.park_restored_leaves(leaves, restored, validated); + Ok(()) + } + + /// Keep each restored leaf that is still a side tip as a fork tip, so a + /// later `invalidateblock` can return to it. Every block to it must have + /// been connected before: it came off the active chain, or it is at or + /// under a fork tip. A leaf under a fork tip or another kept leaf is not a + /// tip. A held child that never connected does not hide it. + fn park_restored_leaves( + &self, + leaves: &[BlockHash], + restored: &HashSet, + validated: &HashSet, + ) { + let forks: HashSet = self.fork_tips.read().unwrap().iter().collect(); + let mut under_valid = HashSet::new(); + let mut kept = Vec::new(); + for branch in self.side_branches(leaves, restored, &HashSet::new()) { + let hashes: Vec = branch.iter().map(Block::block_hash).collect(); + let (tip, below) = hashes.split_last().expect("a side branch is not empty"); + let was_connected = hashes + .iter() + .rev() + .take_while(|h| !forks.contains(*h)) + .all(|h| validated.contains(h)); + let keep = leaves.contains(tip) && was_connected; + if keep { + kept.push(*tip); + } + if keep || forks.contains(tip) { + under_valid.extend(below.iter().copied()); } } - if !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(_) => {} - } + let mut forks = self.fork_tips.write().unwrap(); + for tip in kept { + if !under_valid.contains(&tip) { + forks.insert(tip); } } - Ok(()) + } + + /// Side branches to fork tips, held bodies, precious, and `leaves`, built + /// one at a time. A branch through an invalid block is left out, and so + /// is a tip in `skip`. + fn side_branches<'a>( + &'a self, + leaves: &[BlockHash], + restored: &'a HashSet, + skip: &HashSet, + ) -> impl Iterator> + 'a { + let inv = self.invalidated.set.read().unwrap().clone(); + let mut starts: Vec = self.fork_tips.read().unwrap().iter().collect(); + starts.extend(self.held_bodies.read().unwrap().keys()); + starts.extend(*self.precious.read().unwrap()); + starts.extend(leaves.iter().copied()); + let mut seen = HashSet::new(); + starts.retain(|s| seen.insert(*s) && !inv.contains(s) && !skip.contains(s)); + starts + .into_iter() + .filter_map(move |s| self.assemble_side_branch(s, restored)) + .filter(move |br| !br.iter().any(|b| inv.contains(&b.block_hash()))) + } + + /// The branch with the most total chain work, as in Core's + /// `CBlockIndexWorkComparator`. Equal work prefers the precious tip, then + /// the first-seen held tip. A store fault while summing work is returned. + fn most_work_branch( + &self, + branches: impl Iterator>, + ) -> Result>, NetError> { + let precious = *self.precious.read().unwrap(); + let mut best: Option<(Work, bool, u64, Vec)> = None; + for branch in branches { + let w = match self.branch_chain_work(&branch) { + Ok(w) => w, + Err(e) if e.is_local_fault() => return Err(e), + Err(_) => continue, + }; + let tip = branch[branch.len() - 1].block_hash(); + let is_p = Some(tip) == precious; + let seq = self.held_bodies.read().unwrap().seq(tip); + let take = match &best { + None => true, + Some((bw, was_p, bseq, _)) => held_branch_beats(w, is_p, seq, *bw, *was_p, *bseq), + }; + if take { + best = Some((w, is_p, seq, branch)); + } + } + Ok(best.map(|(_, _, _, branch)| branch)) } fn take_related_invalidated_paths( &self, hash: BlockHash, related: &HashSet, - ) -> Vec> { + ) -> Vec { let mut g = self.invalidated.paths.write().unwrap(); let mut taken = Vec::new(); let mut seeds = related.clone(); @@ -2257,11 +2349,12 @@ impl ChainHub { loop { let before = taken.len(); g.retain(|p| { - let hit = p.iter().any(|h| seeds.contains(h)) - || p.first() + let hit = p.hashes.iter().any(|h| seeds.contains(h)) + || p.hashes + .first() .is_some_and(|h| self.prev_of(h).is_some_and(|prev| seeds.contains(&prev))); if hit { - for h in p { + for h in &p.hashes { seeds.insert(*h); } taken.push(p.clone()); @@ -2286,7 +2379,7 @@ impl ChainHub { if self.is_block_invalid(&hash) { return Ok(()); } - let branch = self.assemble_side_branch(hash); + let branch = self.assemble_side_branch(hash, &HashSet::new()); if branch.is_none() && !self.is_connected(&hash) { return Err(NetError::Consensus("Block not found".into())); } @@ -2945,14 +3038,27 @@ impl ChainHub { .flatten() } - /// Walk hold + archive from `tip` back to a best-chain parent. - fn assemble_side_branch(&self, tip: BlockHash) -> Option> { + /// Walk hold + archive from `tip` back to a best-chain parent. The walk + /// gives up after 10,000 blocks outside `restored`. `invalidateblock` + /// recorded the restored ones, so any number of those is walked. + fn assemble_side_branch( + &self, + tip: BlockHash, + restored: &HashSet, + ) -> Option> { if self.is_connected(&tip) { return None; } let mut rev = Vec::new(); let mut h = tip; - for _ in 0..10_000 { + let mut unrecorded = 0u32; + loop { + if !restored.contains(&h) { + unrecorded += 1; + if unrecorded > 10_000 { + return None; + } + } let b = self.load_side_body(&h)?; let prev = b.header.prev_blockhash; rev.push(b); @@ -2966,7 +3072,6 @@ impl ChainHub { } h = prev; } - None } /// The held (or archived) branch with the most total chain work, as in @@ -2985,7 +3090,7 @@ impl ChainHub { if self.is_block_invalid(&start) { continue; } - let Some(branch) = self.assemble_side_branch(start) else { + let Some(branch) = self.assemble_side_branch(start, &HashSet::new()) else { continue; }; if branch @@ -4330,6 +4435,95 @@ mod tests { let _ = std::fs::remove_dir_all(dir); } + /// A peer's held side branch with an invalid block ties the restored + /// a2..a4 and ranks first. Reconsider marks it and connects a4, as + /// Core's `ActivateBestChain` moves on to the next candidate. Blocks + /// that fail are never kept as valid forks. + #[test] + fn reconsider_skips_a_branch_that_fails() { + 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 bad_hash = bad.block_hash(); + let b3 = mine(bad_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])); + assert!(hub.is_block_invalid(&bad_hash)); + + // A held child of a4 whose time is below the median fails its header + // check. That is not a mark, and a4 under it still connects. + hub.invalidate_block(a[3]).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[2])); + let late = mine(a[3], t0, 5); + let _ = hub.accept_received_block(late); + hub.reconsider_block(a[3]).unwrap(); + assert_eq!(hub.tip_hash(), Some(a[3])); + + // A tip child that failed connect keeps its archived body. It was + // never connected, so reconsider does not make it a valid fork. + let mut bad5 = mine(a[3], t0 + 30, 5); + let cb = &mut bad5.txdata[0].output[0].value; + *cb = Amount::from_sat(cb.to_sat() + 1); + bad5.header.merkle_root = bad5.compute_merkle_root().unwrap(); + rbitcoin_consensus::grind_regtest_pow(&mut bad5.header); + let bad5_hash = bad5.block_hash(); + assert!(hub.accept_received_block(bad5).is_err()); + let a5 = mine_distinct(a[3], t0 + 31, 5, &[bad5_hash]); + let a6 = mine(a5.block_hash(), t0 + 32, 6); + hub.accept_block(a5).unwrap(); + hub.accept_block(a6).unwrap(); + hub.invalidate_block(bad5_hash).unwrap(); + hub.reconsider_block(bad5_hash).unwrap(); + let bad5_status = hub + .chaintips() + .into_iter() + .find(|t| t.hash == bad5_hash) + .map(|t| t.status); + assert_ne!(bad5_status, Some("valid-fork")); + + // Last, because it truncates the store: a fault reading chain work + // while ranking is an error, not a skipped branch. + let tip = hub.tip_hash().unwrap(); + let tip_body = hub + .block_at_height(hub.tip_height().unwrap()) + .unwrap() + .unwrap(); + hub.invalidate_block(tip).unwrap(); + hub.hold_unconnected_body(tip_body); + hub.chain_work_prefix.write().unwrap().clear(); + let header_body = dir.path().join("header.body"); + std::fs::OpenOptions::new() + .write(true) + .open(&header_body) + .unwrap() + .set_len(16) + .unwrap(); + let err = hub.reconsider_block(tip).unwrap_err(); + assert!(err.is_local_fault(), "{err}"); + let _ = std::fs::remove_dir_all(dir); + } + #[test] fn invalid_child_missing_parent_and_linked_reconsider() { let (dir, hub) = tmp_hub(); @@ -5297,6 +5491,12 @@ mod tests { Some("valid-headers"), "a held side block outranks a header-only label" ); + hub.reconsider_block(side.block_hash()).unwrap(); + assert_eq!( + side_status(&hub).as_deref(), + Some("valid-headers"), + "a block that never connected is not a valid fork after reconsider" + ); hub.invalidate_block(side.block_hash()).unwrap(); assert_eq!( side_status(&hub).as_deref(), diff --git a/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs b/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs index d428aba923..dc30310a40 100644 --- a/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs +++ b/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs @@ -581,6 +581,7 @@ fn rpc_regtest_mature_chain_ops() { chain_ops_submit_overweight(&ctx, &hub, &mut cbs); chain_ops_maxfeerate(&ctx, &mut cbs); chain_ops_invalidate_and_precious(&ctx, &hub, &mut cbs, &p2wpkh); + chain_ops_reconsider_most_work(&ctx, &p2wpkh); // Two networks cannot be one chain: the same hub behind a mainnet RPC view. ctx.network = Network::Mainnet; @@ -2581,6 +2582,121 @@ fn chain_ops_invalidate_and_precious( assert_eq!(best_hash(ctx), tip, "precious of less work must not activate"); } +/// Core `reconsiderblock`: clear the marks, then activate the most-work +/// chain. Restored paths join into one branch, and a restored block that +/// loses stays a fork the next `invalidateblock` can return to. +fn chain_ops_reconsider_most_work(ctx: &RpcContext, p2wpkh: &ScriptBuf) { + let best = || best_hash(ctx).as_str().unwrap().parse::().unwrap(); + let call = |method: &str, hash: BlockHash| { + dispatch(ctx, method, vec![json!(hash.to_string())]).unwrap(); + }; + let generate = |n: u32| -> Vec { + let mined = dispatch(ctx, "generate", vec![json!(n)]).unwrap(); + let mined = mined.as_array().unwrap(); + mined.iter().map(|h| h.as_str().unwrap().parse().unwrap()).collect() + }; + let side = |mut prev: BlockHash, n: u32| -> Vec { + let parent = dispatch(ctx, "getblockheader", vec![json!(prev.to_string())]).unwrap(); + let mut time = parent["time"].as_u64().unwrap() as u32; + let mut height = parent["height"].as_u64().unwrap() as u32; + let mut out = Vec::new(); + for _ in 0..n { + time += 900; + height += 1; + let block = + rbitcoin_consensus::mine_regtest_paying(prev, time, height, p2wpkh.clone(), vec![]); + dispatch(ctx, "submitblock", vec![json!(block_hex(&block))]).unwrap(); + prev = block.block_hash(); + out.push(prev); + } + out + }; + let status = |hash: BlockHash| { + let tips = dispatch(ctx, "getchaintips", vec![]).unwrap(); + let tips = tips.as_array().unwrap(); + tips.iter() + .find(|t| t["hash"] == json!(hash.to_string())) + .map(|t| t["status"].as_str().unwrap().to_owned()) + }; + + // a2 alone only ties b2. The two restored paths outweigh it together. + let a = generate(3); + call("invalidateblock", a[2]); + call("invalidateblock", a[1]); + let b = side(a[0], 1); + assert_eq!(best(), b[0]); + call("reconsiderblock", a[1]); + assert_eq!(best(), a[2]); + assert_eq!(status(b[0]).as_deref(), Some("valid-fork")); + + // a3..a4 loses to c3..c5 but stays a fork, so invalidating c3 returns + // to a4. Only the leaf a4 is a tip. + let a4 = generate(1)[0]; + call("invalidateblock", a4); + call("invalidateblock", a[2]); + assert_eq!(best(), a[1]); + let c = side(a[1], 3); + call("reconsiderblock", a[2]); + assert_eq!(best(), c[2]); + assert_eq!(status(a4).as_deref(), Some("valid-fork")); + assert_eq!(status(a[2]), None, "a block under a4 is not a fork tip"); + assert_eq!(status(a[1]), None, "a connected block is not a fork tip"); + call("invalidateblock", c[0]); + assert_eq!(best(), a4); + + // d7 and the sibling e7..e8 share the restored d5..d6. + let d = generate(3); + call("invalidateblock", d[2]); + let e = side(d[1], 2); + call("invalidateblock", e[0]); + call("invalidateblock", d[0]); + assert_eq!(best(), a4); + call("reconsiderblock", d[0]); + assert_eq!(best(), e[1]); + assert_eq!(status(d[2]).as_deref(), Some("valid-fork")); + + // f2 is a fork tip after g2..g5 wins. The restored f3..f4 on top of it + // was connected before, so f4 is a fork tip too. f2 stays one: with f3 + // invalid again, invalidating g2 returns to f2. + let f = generate(4); + call("invalidateblock", f[2]); + let g = side(f[0], 4); + assert_eq!(best(), g[3]); + call("reconsiderblock", f[2]); + assert_eq!(best(), g[3]); + assert_eq!(status(f[3]).as_deref(), Some("valid-fork")); + call("invalidateblock", f[2]); + call("invalidateblock", g[0]); + assert_eq!(best(), f[1]); + call("reconsiderblock", f[2]); + assert_eq!(best(), f[3]); + + // A held child that never connected does not hide its restored parent: + // h4 under the invalid h5 is still a fork to return to. + let h = generate(4); + let top = dispatch(ctx, "getblockheader", vec![json!(h[3].to_string())]).unwrap(); + let mut bad = rbitcoin_consensus::mine_regtest_paying( + h[3], + top["time"].as_u64().unwrap() as u32 + 1, + top["height"].as_u64().unwrap() as u32 + 1, + p2wpkh.clone(), + vec![], + ); + let value = &mut bad.txdata[0].output[0].value; + *value = bitcoin::Amount::from_sat(value.to_sat() + 1); + bad.header.merkle_root = bad.compute_merkle_root().unwrap(); + rbitcoin_consensus::grind_regtest_pow(&mut bad.header); + call("invalidateblock", h[2]); + dispatch(ctx, "submitblock", vec![json!(block_hex(&bad))]).unwrap(); + let i = side(h[0], 5); + call("reconsiderblock", h[2]); + assert_eq!(best(), i[4]); + assert_eq!(status(h[3]).as_deref(), Some("valid-fork")); + call("invalidateblock", bad.block_hash()); + call("invalidateblock", i[0]); + assert_eq!(best(), h[3]); +} + /// Last beat, because it corrupts the store: a connected spend whose parent /// edge in `input.body` is gone is a broken promise, not a txid lookup. fn chain_ops_corrupt_input_edge(ctx: &RpcContext, store: &std::path::Path, height: u32) { From 59b8b0404d4c90fcbcca09709f862e28e929b90d Mon Sep 17 00:00:00 2001 From: Nitesh Balusu Date: Mon, 19 Oct 2026 16:00:17 -0400 Subject: [PATCH 2/3] net: reconsider keeps restored leaves when ranking hits a store fault A store fault while ranking returned before the losing restored leaves were parked. Their invalidate paths were already gone, so nothing named them any more. A branch whose work cannot be summed is now skipped, the first store fault is kept, the leaves are parked, and then the fault is returned. invalidateblock skips such a branch again, as before this PR. A branch whose parent is not on the chain no longer stops the retry loop; the next untried tip is tried. Co-Authored-By: Claude Opus 5.5 --- changelog.d/reconsider-most-work.md | 4 ++- crates/rbitcoin-net/src/chain.rs | 47 ++++++++++++++++++----------- 2 files changed, 33 insertions(+), 18 deletions(-) diff --git a/changelog.d/reconsider-most-work.md b/changelog.d/reconsider-most-work.md index 85c12758a1..f035e28e9e 100644 --- a/changelog.d/reconsider-most-work.md +++ b/changelog.d/reconsider-most-work.md @@ -5,4 +5,6 @@ Fixed outweighed the tip as a whole stayed inactive. A branch that fails to connect no longer stops the next one. A restored leaf that loses, and was connected before, now stays a valid fork, so a later - `invalidateblock` can return to it. + `invalidateblock` can return to it. That later walk keeps the usual + 10,000-block side-branch limit; only the reconsider itself walks + restored blocks past it. diff --git a/crates/rbitcoin-net/src/chain.rs b/crates/rbitcoin-net/src/chain.rs index 7242002b55..dcb17e29da 100644 --- a/crates/rbitcoin-net/src/chain.rs +++ b/crates/rbitcoin-net/src/chain.rs @@ -2134,7 +2134,8 @@ impl ChainHub { /// first-seen held tip. fn try_apply_after_invalidate(&self) -> Result, NetError> { let none = HashSet::new(); - let Some(branch) = self.most_work_branch(self.side_branches(&[], &none, &none))? else { + let Some(branch) = self.most_work_branch(self.side_branches(&[], &none, &none), &mut None) + else { return Ok(None); }; match self.accept_branch_inner(&branch) { @@ -2225,9 +2226,10 @@ impl ChainHub { // A failed tip may stay a candidate (a time reject is not a mark), so // each tip is tried once. let mut tried = HashSet::new(); + let mut fault = None; loop { let untried = self.side_branches(leaves, restored, &tried); - let Some(branch) = self.most_work_branch(untried)? else { + let Some(branch) = self.most_work_branch(untried, &mut fault) else { break; }; let branch_tip = branch[branch.len() - 1].block_hash(); @@ -2239,13 +2241,18 @@ impl ChainHub { match attempt { Ok((AcceptOutcome::Accepted { .. }, Some(_))) => {} Ok(_) => break, - Err(NetError::Protocol(s)) if s.contains("branch parent not on chain") => break, - Err(e) if e.is_local_fault() => return Err(e), + Err(NetError::Protocol(s)) if s.contains("branch parent not on chain") => {} + Err(e) if e.is_local_fault() => { + fault.get_or_insert(e); + break; + } Err(e) => self.remember_failed_accept(branch_tip, &e), } } + // The paths that named these leaves are gone, so park before a fault + // is returned. self.park_restored_leaves(leaves, restored, validated); - Ok(()) + fault.map_or(Ok(()), Err) } /// Keep each restored leaf that is still a side tip as a fork tip, so a @@ -2310,18 +2317,24 @@ impl ChainHub { /// The branch with the most total chain work, as in Core's /// `CBlockIndexWorkComparator`. Equal work prefers the precious tip, then - /// the first-seen held tip. A store fault while summing work is returned. + /// the first-seen held tip. A branch whose work cannot be summed is + /// skipped. The first store fault among those is kept in `fault`. fn most_work_branch( &self, branches: impl Iterator>, - ) -> Result>, NetError> { + fault: &mut Option, + ) -> Option> { let precious = *self.precious.read().unwrap(); let mut best: Option<(Work, bool, u64, Vec)> = None; for branch in branches { let w = match self.branch_chain_work(&branch) { Ok(w) => w, - Err(e) if e.is_local_fault() => return Err(e), - Err(_) => continue, + Err(e) => { + if e.is_local_fault() && fault.is_none() { + *fault = Some(e); + } + continue; + } }; let tip = branch[branch.len() - 1].block_hash(); let is_p = Some(tip) == precious; @@ -2334,7 +2347,7 @@ impl ChainHub { best = Some((w, is_p, seq, branch)); } } - Ok(best.map(|(_, _, _, branch)| branch)) + best.map(|(_, _, _, branch)| branch) } fn take_related_invalidated_paths( @@ -4495,15 +4508,14 @@ mod tests { hub.accept_block(a6).unwrap(); hub.invalidate_block(bad5_hash).unwrap(); hub.reconsider_block(bad5_hash).unwrap(); - let bad5_status = hub - .chaintips() - .into_iter() - .find(|t| t.hash == bad5_hash) - .map(|t| t.status); - assert_ne!(bad5_status, Some("valid-fork")); + assert!( + !hub.fork_tips.read().unwrap().iter().any(|h| h == bad5_hash), + "a block that never connected is not kept as a fork tip" + ); // Last, because it truncates the store: a fault reading chain work - // while ranking is an error, not a skipped branch. + // while ranking is an error. Its invalidate path is gone, so the + // restored tip is still kept as a fork tip to return to. let tip = hub.tip_hash().unwrap(); let tip_body = hub .block_at_height(hub.tip_height().unwrap()) @@ -4521,6 +4533,7 @@ mod tests { .unwrap(); let err = hub.reconsider_block(tip).unwrap_err(); assert!(err.is_local_fault(), "{err}"); + assert!(hub.fork_tips.read().unwrap().iter().any(|h| h == tip)); let _ = std::fs::remove_dir_all(dir); } From 8c1d3c54464e845a6ab79c5747e0f2e7ab2ed37a Mon Sep 17 00:00:00 2001 From: Nitesh Balusu Date: Mon, 19 Oct 2026 16:38:04 -0400 Subject: [PATCH 3/3] test: hold the invalid h5 like a peer's block in the reconsider journey The beat submitted h5 with submitblock while its parent h4 was invalidated, and relied on submitblock holding it. Core refuses that block as `bad-prevblk`, so the beat should not depend on rbitcoin holding it there. Accept it through the hub instead, as a peer's block arrives, so the held child still sits over the restored h4. Co-Authored-By: Claude Opus 5.5 --- .../rbitcoin-rpc/src/regtest_chain_ops_journey.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs b/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs index dc30310a40..a378977984 100644 --- a/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs +++ b/crates/rbitcoin-rpc/src/regtest_chain_ops_journey.rs @@ -581,7 +581,7 @@ fn rpc_regtest_mature_chain_ops() { chain_ops_submit_overweight(&ctx, &hub, &mut cbs); chain_ops_maxfeerate(&ctx, &mut cbs); chain_ops_invalidate_and_precious(&ctx, &hub, &mut cbs, &p2wpkh); - chain_ops_reconsider_most_work(&ctx, &p2wpkh); + chain_ops_reconsider_most_work(&ctx, &hub, &p2wpkh); // Two networks cannot be one chain: the same hub behind a mainnet RPC view. ctx.network = Network::Mainnet; @@ -2585,7 +2585,11 @@ fn chain_ops_invalidate_and_precious( /// Core `reconsiderblock`: clear the marks, then activate the most-work /// chain. Restored paths join into one branch, and a restored block that /// loses stays a fork the next `invalidateblock` can return to. -fn chain_ops_reconsider_most_work(ctx: &RpcContext, p2wpkh: &ScriptBuf) { +fn chain_ops_reconsider_most_work( + ctx: &RpcContext, + hub: &rbitcoin_net::ChainHub, + p2wpkh: &ScriptBuf, +) { let best = || best_hash(ctx).as_str().unwrap().parse::().unwrap(); let call = |method: &str, hash: BlockHash| { dispatch(ctx, method, vec![json!(hash.to_string())]).unwrap(); @@ -2672,7 +2676,9 @@ fn chain_ops_reconsider_most_work(ctx: &RpcContext, p2wpkh: &ScriptBuf) { assert_eq!(best(), f[3]); // A held child that never connected does not hide its restored parent: - // h4 under the invalid h5 is still a fork to return to. + // h4 under the invalid h5 is still a fork to return to. h5 comes from a + // peer and is held under the invalidated h4 (Core's submitblock refuses + // it as `bad-prevblk`). let h = generate(4); let top = dispatch(ctx, "getblockheader", vec![json!(h[3].to_string())]).unwrap(); let mut bad = rbitcoin_consensus::mine_regtest_paying( @@ -2687,7 +2693,7 @@ fn chain_ops_reconsider_most_work(ctx: &RpcContext, p2wpkh: &ScriptBuf) { bad.header.merkle_root = bad.compute_merkle_root().unwrap(); rbitcoin_consensus::grind_regtest_pow(&mut bad.header); call("invalidateblock", h[2]); - dispatch(ctx, "submitblock", vec![json!(block_hex(&bad))]).unwrap(); + let _ = hub.accept_received_block(bad.clone()); let i = side(h[0], 5); call("reconsiderblock", h[2]); assert_eq!(best(), i[4]);