From c0c8ae2f03dd42b80fee33a1856240df7bc64574 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 4 Aug 2026 03:34:24 +0000 Subject: [PATCH 1/5] Give `CoinSelector` its target instead of threading it through every call A selector was built for one target and evaluated against it throughout, but every method took the target as a parameter, so nothing stopped `cs.excess(target_a, drain)` being followed by `cs.is_funded(target_b)`. The correctness arguments in the metrics are all stated at a fixed target -- `LowestFee::bound`'s proof that a changeless superset always costs more, `Changeless::change_unavoidable`'s assumption that the drain decision is monotone in the excess -- and were held together by convention rather than by types. `CoinSelector::new` now takes the target and owns it. Twenty signatures *lose* a parameter rather than gaining one: fifteen public methods (`excess`, `implied_fee`, `is_funded`, `drain`, `select_until_target_met`, the four `*_excess`, ...), plus `bnb_solutions` and `run_bnb`, plus all three `BnbMetric` methods. The crate had already reached this conclusion one layer down: `BnbIter` stored the target as a field, took it once in `BnbIter::new`, and then re-passed it into `metric.score` and `metric.bound` at every node. That field and the re-threading are both gone. This is a breaking change, and it reaches `BnbMetric`, so metrics implemented outside this crate need their signatures updated: fn score(&mut self, cs: &CoinSelector<'_>) -> Option; fn bound(&mut self, cs: &CoinSelector<'_>) -> Option; fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain; `CoinSelector::target()` exposes the target for metrics that need to read it. Co-Authored-By: Claude Opus 5 --- README.md | 20 ++-- benches/coin_selector.rs | 7 +- src/bnb.rs | 17 ++- src/coin_selector.rs | 212 +++++++++++++++++++------------------- src/metrics/changeless.rs | 24 ++--- src/metrics/lowest_fee.rs | 103 +++++++++--------- tests/bnb.rs | 73 ++++++------- tests/changeless.rs | 6 +- tests/common.rs | 65 ++++++------ tests/lowest_fee.rs | 56 +++++----- tests/srd.rs | 35 +++---- tests/weight.rs | 33 ++++-- 12 files changed, 318 insertions(+), 333 deletions(-) diff --git a/README.md b/README.md index 4335d4b..922dd25 100644 --- a/README.md +++ b/README.md @@ -54,13 +54,13 @@ let candidates = vec![ ]; // You can now select coins! -let mut coin_selector = CoinSelector::new(&candidates); +let mut coin_selector = CoinSelector::new(&candidates, target); coin_selector.select(0); -assert!(!coin_selector.is_funded(target), "we didn't select enough"); -println!("we didn't select enough yet we're missing: {}", coin_selector.missing(target)); +assert!(!coin_selector.is_funded(), "we didn't select enough"); +println!("we didn't select enough yet we're missing: {}", coin_selector.missing()); coin_selector.select(1); -assert!(coin_selector.is_funded(target), "we should have enough now"); +assert!(coin_selector.is_funded(), "we should have enough now"); // Now we need to know if we need a change output to drain the excess if we overshot too much // @@ -69,7 +69,7 @@ assert!(coin_selector.is_funded(target), "we should have enough now"); let drain_weights = DrainWeights::TR_KEYSPEND; // Our policy is to only add a change output if the value is over 1_000 sats let change_policy = ChangePolicy::min_value(drain_weights, 1_000); -let change = coin_selector.drain(target, change_policy); +let change = coin_selector.drain(change_policy); if change.is_some() { println!("We need to add our change output to the transaction with {} value", change.value); } else { @@ -127,14 +127,14 @@ let drain_weights = bdk_coin_select::DrainWeights::default(); // You could determine this by looking at the user's transaction history and taking an average of the feerate. let long_term_feerate = FeeRate::from_sat_per_vb(10.0); -let mut coin_selector = CoinSelector::new(&candidates); - let target = Target { fee: TargetFee::from_feerate(FeeRate::from_sat_per_vb(15.0)), outputs: TargetOutputs::fund_outputs(outputs.iter().map(|output| (output.weight().to_wu(), output.value.to_sat()))), max_weight: None, }; +let mut coin_selector = CoinSelector::new(&candidates, target); + // The feerate used to work out whether a change output would be dust (and so shouldn't be added). // The standard dust relay feerate is 3 sat/vb. let dust_relay_feerate = FeeRate::from_sat_per_vb(3.0); @@ -150,13 +150,13 @@ let mut metric = LowestFee { // We run the branch and bound algorithm with a max round limit of 100,000. // On success it returns the score along with the change output the metric decided on. -let change = match coin_selector.run_bnb(target, metric, 100_000) { +let change = match coin_selector.run_bnb(metric, 100_000) { Err(err) => { println!("failed to find a solution: {}", err); // fall back to naive selection - coin_selector.select_until_target_met(target).expect("a selection was impossible!"); + coin_selector.select_until_target_met().expect("a selection was impossible!"); // the metric still decides the change output for whatever we end up selecting - metric.drain(&coin_selector, target) + metric.drain(&coin_selector) } Ok((score, change)) => { println!("we found a solution with score {}", score); diff --git a/benches/coin_selector.rs b/benches/coin_selector.rs index c05eabb..18e847e 100644 --- a/benches/coin_selector.rs +++ b/benches/coin_selector.rs @@ -56,7 +56,8 @@ fn bench_coin_selector_clone(c: &mut Criterion) { let mut group = c.benchmark_group("clone"); for &n in &[64usize, 256, 1024, 4096] { let candidates = make_candidates(n); - let mut selector = CoinSelector::new(&candidates); + let (target, _) = make_bnb_inputs(&candidates); + let mut selector = CoinSelector::new(&candidates, target); // Select ~10% of candidates so `selected` is non-trivial to copy. for i in (0..n).step_by(10) { selector.select(i); @@ -74,8 +75,8 @@ fn bench_run_bnb_lowest_fee(c: &mut Criterion) { group.sample_size(20); for &n in &[20usize, 50, 100, 200] { let candidates = make_candidates(n); - let selector = CoinSelector::new(&candidates); let (target, long_term_feerate) = make_bnb_inputs(&candidates); + let selector = CoinSelector::new(&candidates, target); group.bench_with_input(BenchmarkId::from_parameter(n), &n, |b, _| { b.iter_batched( || selector.clone(), @@ -85,7 +86,7 @@ fn bench_run_bnb_lowest_fee(c: &mut Criterion) { dust_relay_feerate: FeeRate::from_sat_per_vb(1.0), drain_weights: DrainWeights::TR_KEYSPEND, }; - let _ = sel.run_bnb(target, metric, black_box(100_000)); + let _ = sel.run_bnb(metric, black_box(100_000)); sel }, BatchSize::SmallInput, diff --git a/src/bnb.rs b/src/bnb.rs index 0498e5c..d20db4c 100644 --- a/src/bnb.rs +++ b/src/bnb.rs @@ -1,6 +1,6 @@ use core::cmp::Reverse; -use crate::{float::Ordf32, Drain, Target}; +use crate::{float::Ordf32, Drain}; use super::CoinSelector; use alloc::collections::BinaryHeap; @@ -11,8 +11,6 @@ use alloc::collections::BinaryHeap; pub(crate) struct BnbIter<'a, M: BnbMetric> { queue: BinaryHeap>, best: Option, - /// The target the metric scores selections against. - pub(crate) target: Target, /// The `BnBMetric` that will score each selection pub(crate) metric: M, } @@ -55,7 +53,7 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> { let mut return_val = None; if !branch.is_exclusion { - if let Some(score) = self.metric.score(&selector, self.target) { + if let Some(score) = self.metric.score(&selector) { let better = match self.best { Some(best_score) => score < best_score, None => true, @@ -73,11 +71,10 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> { } impl<'a, M: BnbMetric> BnbIter<'a, M> { - pub(crate) fn new(mut selector: CoinSelector<'a>, target: Target, metric: M) -> Self { + pub(crate) fn new(mut selector: CoinSelector<'a>, metric: M) -> Self { let mut iter = BnbIter { queue: BinaryHeap::default(), best: None, - target, metric, }; @@ -91,7 +88,7 @@ impl<'a, M: BnbMetric> BnbIter<'a, M> { } fn consider_adding_to_queue(&mut self, cs: &CoinSelector<'a>, is_exclusion: bool) { - let bound = self.metric.bound(cs, self.target); + let bound = self.metric.bound(cs); if let Some(bound) = bound { let is_good_enough = match self.best { Some(best) => best > bound, @@ -205,7 +202,7 @@ pub trait BnbMetric { /// Get the score of a given selection for `target`. /// /// If this returns `None`, the selection is invalid. - fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option; + fn score(&mut self, cs: &CoinSelector<'_>) -> Option; /// Get the lower bound score using a heuristic for `target`. /// @@ -214,13 +211,13 @@ pub trait BnbMetric { /// /// If this returns `None`, the current branch and all descendant branches will not have valid /// solutions. - fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option; + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option; /// The change output (a.k.a. drain) this metric decides on for the given selection and `target`, /// or [`Drain::NONE`] if it decides there should be no change. /// /// Call this on a branch-and-bound solution to get the change output the metric optimized against. - fn drain(&mut self, cs: &CoinSelector<'_>, target: Target) -> Drain; + fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain; /// Returns whether the metric requies we order candidates by descending value per weight unit. fn requires_ordering_by_descending_value_pwu(&self) -> bool { diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 604abd8..1a75724 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -18,6 +18,10 @@ pub const CHANGE_LOWER: u64 = 50_000; #[derive(Debug, Clone)] pub struct CoinSelector<'a> { candidates: &'a [Candidate], + /// What this selection is trying to fund. Owned rather than passed per call: a selector is + /// built for one target and evaluated against it throughout, and threading it through every + /// method made it possible to ask two different questions of the same selection. + target: Target, selected: Bitset, banned: Bitset, candidate_order: Arc>, @@ -35,15 +39,25 @@ impl<'a> CoinSelector<'a> { /// /// Note that methods in `CoinSelector` will refer to inputs by the index in the `candidates` /// slice you pass in. - pub fn new(candidates: &'a [Candidate]) -> Self { + /// + /// `target` is fixed for the life of the selector. Everything it reports -- excesses, implied + /// fees, whether it is funded -- is measured against that one target, so build a second + /// selector to ask about a second self.target. + pub fn new(candidates: &'a [Candidate], target: Target) -> Self { Self { candidates, + target, selected: Bitset::with_capacity(candidates.len()), banned: Bitset::with_capacity(candidates.len()), candidate_order: Arc::new((0..candidates.len()).collect::>()), } } + /// What this selector is funding. Fixed at construction. + pub fn target(&self) -> Target { + self.target + } + /// Iterate over all the candidates in their currently sorted order. Each item has the original /// index with the candidate. pub fn candidates( @@ -128,10 +142,10 @@ impl<'a> CoinSelector<'a> { /// [`ban`]: Self::ban /// [`is_funded`]: Self::is_funded /// [`select_until_target_met`]: Self::select_until_target_met - pub fn is_fundable(&self, target: Target) -> bool { + pub fn is_fundable(&self) -> bool { let mut test = self.clone(); - test.select_all_effective(target.fee.rate); - test.is_funded(target) + test.select_all_effective(self.target.fee.rate); + test.is_funded() } /// Returns true if no candidates have been selected. @@ -186,15 +200,15 @@ impl<'a> CoinSelector<'a> { /// /// In order for the resulting transaction to be valid this must be 0 or above. If it's above 0 /// this means the transaction will overpay for what it needs to reach `target`. - pub fn excess(&self, target: Target, drain: Drain) -> i64 { - self.rate_excess(target, drain) - .min(self.absolute_excess(target, drain)) - .min(self.replacement_excess(target, drain)) + pub fn excess(&self, drain: Drain) -> i64 { + self.rate_excess(drain) + .min(self.absolute_excess(drain)) + .min(self.replacement_excess(drain)) } - /// How much extra value needs to be selected to reach the target. - pub fn missing(&self, target: Target) -> u64 { - let excess = self.excess(target, Drain::NONE); + /// How much extra value needs to be selected to reach the self.target. + pub fn missing(&self) -> u64 { + let excess = self.excess(Drain::NONE); if excess < 0 { excess.unsigned_abs() } else { @@ -202,56 +216,56 @@ impl<'a> CoinSelector<'a> { } } - /// How much the current selection overshoots the value need to satisfy `target.fee.rate` and - /// `target.value` (while ignoring `target.fee.absolute`). - pub fn rate_excess(&self, target: Target, drain: Drain) -> i64 { + /// How much the current selection overshoots the value need to satisfy `self.target.fee.rate` and + /// `self.target.value` (while ignoring `self.target.fee.absolute`). + pub fn rate_excess(&self, drain: Drain) -> i64 { self.selected_value() as i64 - - target.value() as i64 + - self.target.value() as i64 - drain.value as i64 - - self.implied_fee_from_feerate(target, drain.weights) as i64 + - self.implied_fee_from_feerate(drain.weights) as i64 } - /// Same as [rate_excess](Self::rate_excess) except `target.fee.rate` is applied to the + /// Same as [rate_excess](Self::rate_excess) except `self.target.fee.rate` is applied to the /// implied transaction's weight units directly without any conversion to vbytes. - pub fn rate_excess_wu(&self, target: Target, drain: Drain) -> i64 { + pub fn rate_excess_wu(&self, drain: Drain) -> i64 { self.selected_value() as i64 - - target.value() as i64 + - self.target.value() as i64 - drain.value as i64 - - self.implied_fee_from_feerate_wu(target, drain.weights) as i64 + - self.implied_fee_from_feerate_wu(drain.weights) as i64 } - /// How much the current selection overshoots the value needed to satisfy `target.fee.absolute` - /// and `target.value` (while ignoring `target.fee.rate`). - pub fn absolute_excess(&self, target: Target, drain: Drain) -> i64 { + /// How much the current selection overshoots the value needed to satisfy `self.target.fee.absolute` + /// and `self.target.value` (while ignoring `self.target.fee.rate`). + pub fn absolute_excess(&self, drain: Drain) -> i64 { self.selected_value() as i64 - - target.value() as i64 + - self.target.value() as i64 - drain.value as i64 - - target.fee.absolute as i64 + - self.target.fee.absolute as i64 } /// How much the current selection overshoots the value needed to satisfy RBF's rule 4. - pub fn replacement_excess(&self, target: Target, drain: Drain) -> i64 { + pub fn replacement_excess(&self, drain: Drain) -> i64 { let mut replacement_excess_needed = 0; - if let Some(replace) = target.fee.replace { + if let Some(replace) = self.target.fee.replace { replacement_excess_needed = - replace.min_fee_to_do_replacement(self.weight(target.outputs, drain.weights)) + replace.min_fee_to_do_replacement(self.weight(self.target.outputs, drain.weights)) } self.selected_value() as i64 - - target.value() as i64 + - self.target.value() as i64 - drain.value as i64 - replacement_excess_needed as i64 } /// Same as [replacement_excess](Self::replacement_excess) except the replacement fee /// is calculated using weight units directly without any conversion to vbytes. - pub fn replacement_excess_wu(&self, target: Target, drain: Drain) -> i64 { + pub fn replacement_excess_wu(&self, drain: Drain) -> i64 { let mut replacement_excess_needed = 0; - if let Some(replace) = target.fee.replace { - replacement_excess_needed = - replace.min_fee_to_do_replacement_wu(self.weight(target.outputs, drain.weights)) + if let Some(replace) = self.target.fee.replace { + replacement_excess_needed = replace + .min_fee_to_do_replacement_wu(self.weight(self.target.outputs, drain.weights)) } self.selected_value() as i64 - - target.value() as i64 + - self.target.value() as i64 - drain.value as i64 - replacement_excess_needed as i64 } @@ -276,33 +290,33 @@ impl<'a> CoinSelector<'a> { /// [`Replace`] constraints and returns the larger of the two. /// /// `drain_weight` can be 0 to indicate no draining output. - pub fn implied_fee(&self, target: Target, drain_weights: DrainWeights) -> u64 { + pub fn implied_fee(&self, drain_weights: DrainWeights) -> u64 { let mut implied_fee = self - .implied_fee_from_feerate(target, drain_weights) - .max(target.fee.absolute); + .implied_fee_from_feerate(drain_weights) + .max(self.target.fee.absolute); - if let Some(replace) = target.fee.replace { + if let Some(replace) = self.target.fee.replace { implied_fee = Ord::max( implied_fee, - replace.min_fee_to_do_replacement(self.weight(target.outputs, drain_weights)), + replace.min_fee_to_do_replacement(self.weight(self.target.outputs, drain_weights)), ); } implied_fee } - fn implied_fee_from_feerate(&self, target: Target, drain_weights: DrainWeights) -> u64 { - target + fn implied_fee_from_feerate(&self, drain_weights: DrainWeights) -> u64 { + self.target .fee .rate - .implied_fee(self.weight(target.outputs, drain_weights)) + .implied_fee(self.weight(self.target.outputs, drain_weights)) } - fn implied_fee_from_feerate_wu(&self, target: Target, drain_weights: DrainWeights) -> u64 { - target + fn implied_fee_from_feerate_wu(&self, drain_weights: DrainWeights) -> u64 { + self.target .fee .rate - .implied_fee_wu(self.weight(target.outputs, drain_weights)) + .implied_fee_wu(self.weight(self.target.outputs, drain_weights)) } /// The actual fee the selection would pay if it was used in a transaction that had @@ -381,29 +395,24 @@ impl<'a> CoinSelector<'a> { /// You can pass in an `excess_discount` which must be between `0.0..1.0`. Passing in `1.0` gives you no discount /// /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection - pub fn waste( - &self, - target: Target, - long_term_feerate: FeeRate, - drain: Drain, - excess_discount: f32, - ) -> f32 { + pub fn waste(&self, long_term_feerate: FeeRate, drain: Drain, excess_discount: f32) -> f32 { debug_assert!((0.0..=1.0).contains(&excess_discount)); - let mut waste = self.input_waste(target.fee.rate, long_term_feerate); + let mut waste = self.input_waste(self.target.fee.rate, long_term_feerate); if drain.is_none() { // We don't allow negative excess waste since negative excess just means you haven't // satisified target yet in which case you probably shouldn't be calling this function. - let mut excess_waste = self.excess(target, drain).max(0) as f32; + let mut excess_waste = self.excess(drain).max(0) as f32; // we allow caller to discount this waste depending on how wasteful excess actually is // to them. excess_waste *= excess_discount.clamp(0.0, 1.0); waste += excess_waste; } else { - waste += - drain - .weights - .waste(target.fee.rate, long_term_feerate, target.outputs.n_outputs); + waste += drain.weights.waste( + self.target.fee.rate, + long_term_feerate, + self.target.outputs.n_outputs, + ); } waste @@ -467,9 +476,9 @@ impl<'a> CoinSelector<'a> { /// Always `true` when `max_weight` is `None`. Note this is the *anti-monotone* half of /// feasibility (adding inputs adds weight), so it is kept separate from the monotone /// value-only [`is_funded`](Self::is_funded). - pub fn is_within_max_weight(&self, target: Target, drain_weights: DrainWeights) -> bool { - match target.max_weight { - Some(max_weight) => self.weight(target.outputs, drain_weights) <= max_weight, + pub fn is_within_max_weight(&self, drain_weights: DrainWeights) -> bool { + match self.target.max_weight { + Some(max_weight) => self.weight(self.target.outputs, drain_weights) <= max_weight, None => true, } } @@ -479,8 +488,8 @@ impl<'a> CoinSelector<'a> { /// /// This is **monotone**: selecting more never un-meets it. It deliberately does *not* include /// the weight cap — see [`is_within_max_weight`](Self::is_within_max_weight). - pub fn is_funded_with_drain(&self, target: Target, drain: Drain) -> bool { - self.excess(target, drain) >= 0 + pub fn is_funded_with_drain(&self, drain: Drain) -> bool { + self.excess(drain) >= 0 } /// Whether the selection covers the target **value** (net of input fees), i.e. [`excess`] is @@ -492,8 +501,8 @@ impl<'a> CoinSelector<'a> { /// [`excess`]: Self::excess /// [`is_within_max_weight`]: Self::is_within_max_weight /// [`is_funded_with_drain`]: Self::is_funded_with_drain - pub fn is_funded(&self, target: Target) -> bool { - self.is_funded_with_drain(target, Drain::NONE) + pub fn is_funded(&self) -> bool { + self.is_funded_with_drain(Drain::NONE) } /// Select all unselected candidates @@ -509,24 +518,18 @@ impl<'a> CoinSelector<'a> { /// constraints of `target` and respecting `change_policy`. /// /// If not change output should be added according to policy then it will return `None`. - pub fn drain_value(&self, target: Target, change_policy: ChangePolicy) -> Option { - let excess = self.excess( - target, - Drain { - weights: change_policy.drain_weights, - value: 0, - }, - ); + pub fn drain_value(&self, change_policy: ChangePolicy) -> Option { + let excess = self.excess(Drain { + weights: change_policy.drain_weights, + value: 0, + }); if excess > change_policy.min_value as i64 { debug_assert_eq!( - self.is_funded(target), - self.is_funded_with_drain( - target, - Drain { - weights: change_policy.drain_weights, - value: excess as u64 - } - ), + self.is_funded(), + self.is_funded_with_drain(Drain { + weights: change_policy.drain_weights, + value: excess as u64 + }), "if the target is met without a drain it must be met after adding the drain" ); Some(excess as u64) @@ -546,8 +549,8 @@ impl<'a> CoinSelector<'a> { /// [`is_funded_with_drain`]: Self::is_funded_with_drain /// [`is_funded`]: Self::is_funded #[must_use] - pub fn drain(&self, target: Target, change_policy: ChangePolicy) -> Drain { - match self.drain_value(target, change_policy) { + pub fn drain(&self, change_policy: ChangePolicy) -> Drain { + match self.drain_value(change_policy) { Some(value) => Drain { weights: change_policy.drain_weights, value, @@ -580,14 +583,13 @@ impl<'a> CoinSelector<'a> { /// - [`SelectError::MaxWeightExceeded`] if the value is met but the resulting selection exceeds /// [`Target::max_weight`]. Note this only reflects *this* in-order greedy selection; a /// different selection might still fit the cap (use branch and bound to search for one). - pub fn select_until_target_met(&mut self, target: Target) -> Result<(), SelectError> { - self.select_until(|cs| cs.is_funded(target)) - .ok_or_else(|| { - SelectError::InsufficientFunds(InsufficientFunds { - missing: self.excess(target, Drain::NONE).unsigned_abs(), - }) - })?; - if !self.is_within_max_weight(target, DrainWeights::NONE) { + pub fn select_until_target_met(&mut self) -> Result<(), SelectError> { + self.select_until(|cs| cs.is_funded()).ok_or_else(|| { + SelectError::InsufficientFunds(InsufficientFunds { + missing: self.excess(Drain::NONE).unsigned_abs(), + }) + })?; + if !self.is_within_max_weight(DrainWeights::NONE) { return Err(SelectError::MaxWeightExceeded); } Ok(()) @@ -620,7 +622,7 @@ impl<'a> CoinSelector<'a> { /// The change *amount* comes out random on its own: because candidates are added in random order /// and we stop as soon as the change reaches `change_lower`, the final change is wherever the /// last (random) input pushed it — at or above `change_lower`. So, like Core, we use a fixed - /// lower bound rather than randomizing the target. + /// lower bound rather than randomizing the self.target. /// /// On success it returns the [`Drain`] to attach, whose value is the achieved change (at least /// `change_lower`). Returns [`SelectError::InsufficientFunds`] if the target plus `change_lower` @@ -628,7 +630,7 @@ impl<'a> CoinSelector<'a> { /// met but the resulting selection exceeds the weight cap. /// /// `rng` shuffles the candidates; it yields uniform `u64`s, e.g. `|| my_rng.next_u64()`. Any - /// already-selected candidates are kept and counted toward the target. + /// already-selected candidates are kept and counted toward the self.target. /// /// [`run_bnb`]: Self::run_bnb /// [`LowestFee`]: crate::metrics::LowestFee @@ -637,7 +639,6 @@ impl<'a> CoinSelector<'a> { // the max-weight PR lands. pub fn select_srd( &mut self, - target: Target, drain_weights: DrainWeights, change_lower: u64, rng: impl FnMut() -> u64, @@ -648,14 +649,11 @@ impl<'a> CoinSelector<'a> { let mut excess = 0_i64; self.select_until(|cs| { - is_within_max_weight = cs.is_within_max_weight(target, drain_weights); - excess = cs.excess( - target, - Drain { - weights: drain_weights, - value: 0, - }, - ); + is_within_max_weight = cs.is_within_max_weight(drain_weights); + excess = cs.excess(Drain { + weights: drain_weights, + value: 0, + }); excess >= change_lower as i64 || !is_within_max_weight }) .ok_or_else(|| { @@ -688,10 +686,9 @@ impl<'a> CoinSelector<'a> { /// Most of the time, you would want to use [`CoinSelector::run_bnb`] instead. pub fn bnb_solutions( &self, - target: Target, metric: M, ) -> impl Iterator, Ordf32)>> { - crate::bnb::BnbIter::new(self.clone(), target, metric) + crate::bnb::BnbIter::new(self.clone(), metric) } /// Run branch and bound to minimize the score of the provided [`BnbMetric`]. @@ -703,11 +700,10 @@ impl<'a> CoinSelector<'a> { /// Use [`CoinSelector::bnb_solutions`] to access the branch and bound iterator directly. pub fn run_bnb( &mut self, - target: Target, metric: M, max_rounds: usize, ) -> Result<(Ordf32, Drain), NoBnbSolution> { - let mut iter = crate::bnb::BnbIter::new(self.clone(), target, metric); + let mut iter = crate::bnb::BnbIter::new(self.clone(), metric); let mut rounds = 0_usize; let best = iter .by_ref() @@ -716,7 +712,7 @@ impl<'a> CoinSelector<'a> { .flatten() .last(); if let Some((selector, score)) = best { - let drain = iter.metric.drain(&selector, target); + let drain = iter.metric.drain(&selector); *self = selector; return Ok((score, drain)); } @@ -729,7 +725,7 @@ impl<'a> CoinSelector<'a> { assert_eq!(rounds, max_rounds); // still-yielding ⟹ we truncated at the cap return Err(NoBnbSolution::RoundLimit { max_rounds, rounds }); } - if !self.is_fundable(target) { + if !self.is_fundable() { return Err(NoBnbSolution::InsufficientFunds); } Err(NoBnbSolution::MaxWeightExceeded) diff --git a/src/metrics/changeless.rs b/src/metrics/changeless.rs index a9c9e32..c2c9036 100644 --- a/src/metrics/changeless.rs +++ b/src/metrics/changeless.rs @@ -1,4 +1,4 @@ -use crate::{bnb::BnbMetric, float::Ordf32, CoinSelector, Drain, Target}; +use crate::{bnb::BnbMetric, float::Ordf32, CoinSelector, Drain}; /// Constrains an `inner` metric to only changeless solutions. /// @@ -26,50 +26,50 @@ impl Changeless { /// are next to each other, which [`requires_ordering_by_descending_value_pwu`] guarantees. /// /// [`requires_ordering_by_descending_value_pwu`]: BnbMetric::requires_ordering_by_descending_value_pwu - fn change_unavoidable(&mut self, cs: &CoinSelector<'_>, target: Target) -> bool { - if self.0.drain(cs, target).is_none() { + fn change_unavoidable(&mut self, cs: &CoinSelector<'_>) -> bool { + if self.0.drain(cs).is_none() { return false; } let mut least_excess = cs.clone(); cs.unselected() .rev() - .take_while(|(_, wv)| wv.effective_value(target.fee.rate) < 0.0) + .take_while(|(_, wv)| wv.effective_value(cs.target().fee.rate) < 0.0) .for_each(|(index, _)| { least_excess.select(index); }); - self.0.drain(&least_excess, target).is_some() + self.0.drain(&least_excess).is_some() } } impl BnbMetric for Changeless { - fn drain(&mut self, _cs: &CoinSelector<'_>, _target: Target) -> Drain { + fn drain(&mut self, _cs: &CoinSelector<'_>) -> Drain { // by definition a changeless selection never has a change output Drain::NONE } - fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option { + fn score(&mut self, cs: &CoinSelector<'_>) -> Option { // Reject selections that have change. We don't need an explicit target-met check: `inner` // returns `None` for invalid (e.g. not-target-met) selections. // // NOTE: for metrics whose `score` recomputes the drain (e.g. `LowestFee`), this evaluates // the drain decision twice per node. Sharing it would mean threading the drain into // `score`, which we avoid to keep metrics composable. - if self.0.drain(cs, target).is_some() { + if self.0.drain(cs).is_some() { return None; } - self.0.score(cs, target) + self.0.score(cs) } - fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option { - if self.change_unavoidable(cs, target) { + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { + if self.change_unavoidable(cs) { // every descendant has change, so no changeless solution is reachable None } else { // the changeless-constrained optimum is no better than the inner metric's unconstrained // optimum, so the inner bound is a valid lower bound - self.0.bound(cs, target) + self.0.bound(cs) } } diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index 5499777..d990f71 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -1,11 +1,11 @@ -use crate::{float::Ordf32, BnbMetric, CoinSelector, Drain, DrainWeights, FeeRate, Target}; +use crate::{float::Ordf32, BnbMetric, CoinSelector, Drain, DrainWeights, FeeRate}; /// Metric that aims to minimize transaction fees. The future fee for spending the change output is /// included in this calculation. /// /// The fee is simply: /// -/// > `inputs - outputs` where `outputs = target.value + change_value` +/// > `inputs - outputs` where `outputs = cs.target().value + change_value` /// /// But the total value includes the cost of spending the change output if it exists: /// @@ -27,16 +27,13 @@ pub struct LowestFee { impl LowestFee { /// The value the change output should have, or `None` if this selection should be changeless. - fn drain_value(&self, cs: &CoinSelector<'_>, target: Target) -> Option { + fn drain_value(&self, cs: &CoinSelector<'_>) -> Option { // The change output pays for its own weight, so the value we'd actually recover is the // excess remaining after accounting for that weight. - let excess_with_drain_weight = cs.excess( - target, - Drain { - weights: self.drain_weights, - value: 0, - }, - ); + let excess_with_drain_weight = cs.excess(Drain { + weights: self.drain_weights, + value: 0, + }); // Adding change is only worth it if the value we'd recover exceeds the future cost of // spending it (i.e. it lowers the long-term fee). @@ -56,7 +53,7 @@ impl LowestFee { // ...and only if the change output would not push the tx over `max_weight`. If it would, // we refuse the drain and the excess goes to fee instead (a slightly conservative choice: // it can refuse change even when a no-change tx of this selection would fit). - if !cs.is_within_max_weight(target, self.drain_weights) { + if !cs.is_within_max_weight(self.drain_weights) { return None; } @@ -71,17 +68,15 @@ impl LowestFee { /// inside [`bound`](BnbMetric::bound): deferring the changeless rejection only loosens the lower /// bound and never makes it inadmissible, and `score` reuses the returned drain for its cap /// check so the drain is decided once. - fn fee_score(&self, cs: &CoinSelector<'_>, target: Target) -> Option<(Ordf32, Drain)> { - if !cs.is_funded(target) { + fn fee_score(&self, cs: &CoinSelector<'_>) -> Option<(Ordf32, Drain)> { + if !cs.is_funded() { return None; } - let drain = self - .drain_value(cs, target) - .map_or(Drain::NONE, |value| Drain { - weights: self.drain_weights, - value, - }); - let fee_for_the_tx = cs.fee(target.value(), drain.value); + let drain = self.drain_value(cs).map_or(Drain::NONE, |value| Drain { + weights: self.drain_weights, + value, + }); + let fee_for_the_tx = cs.fee(cs.target().value(), drain.value); assert!( fee_for_the_tx >= 0, "must not be called unless selection has met target: fee={}", @@ -96,36 +91,35 @@ impl LowestFee { } impl BnbMetric for LowestFee { - fn drain(&mut self, cs: &CoinSelector<'_>, target: Target) -> Drain { - self.drain_value(cs, target) - .map_or(Drain::NONE, |value| Drain { - weights: self.drain_weights, - value, - }) + fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain { + self.drain_value(cs).map_or(Drain::NONE, |value| Drain { + weights: self.drain_weights, + value, + }) } - fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option { - let (score, drain) = self.fee_score(cs, target)?; + fn score(&mut self, cs: &CoinSelector<'_>) -> Option { + let (score, drain) = self.fee_score(cs)?; // A final selection must fit the weight cap. `drain_value` already refuses an over-cap // change, but a changeless selection can still be too heavy on its own. Reuse the drain // `fee_score` already decided rather than recomputing it here. - if !cs.is_within_max_weight(target, drain.weights) { + if !cs.is_within_max_weight(drain.weights) { return None; } Some(score) } - fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option { + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { // Weight hard-prune: input weight only grows as this branch is extended, so the lightest // solution in the subtree is this selection with no drain. If even that busts `max_weight`, // the whole subtree is infeasible -> prune. (Also keeps `fee_score(cs).unwrap()` below // sound: a value-met but over-cap node would otherwise score `None`.) - if !cs.is_within_max_weight(target, DrainWeights::NONE) { + if !cs.is_within_max_weight(DrainWeights::NONE) { return None; } - if cs.is_funded(target) { - let current_score = self.fee_score(cs, target).unwrap().0; + if cs.is_funded() { + let current_score = self.fee_score(cs).unwrap().0; // `current_score` is already a valid lower bound for a selection that has change: a // descendant can never lower the fee by removing an existing (worthwhile) change @@ -146,17 +140,17 @@ impl BnbMetric for LowestFee { // `drain_value`, where `change_value` is `excess_with_drain_weight` and `spend_fee` is // `drain_spend_cost`). With `v >= 0` the difference is strictly positive: B always // costs more. - if self.drain_value(cs, target).is_none() { + if self.drain_value(cs).is_none() { // But a descendant might *add* a change output that improves the metric. This // happens when the current selection is changeless only because the change would be // dust: a descendant with more excess could clear the dust threshold and recover // value that is currently burned to fees. let cost_of_adding_change = self.drain_weights.waste( - target.fee.rate, + cs.target().fee.rate, self.long_term_feerate, - target.outputs.n_outputs, + cs.target().outputs.n_outputs, ); - let cost_of_no_change = cs.excess(target, Drain::NONE); + let cost_of_no_change = cs.excess(Drain::NONE); let best_score_with_change = Ordf32(current_score.0 - cost_of_no_change as f32 + cost_of_adding_change); @@ -165,10 +159,10 @@ impl BnbMetric for LowestFee { // of which only make the tx heavier. If there's no room for both under the cap the // improvement is unreachable down this branch, so don't credit it — keep // `current_score` (a tighter, still-admissible bound). - let change_is_reachable = match target.max_weight { + let change_is_reachable = match cs.target().max_weight { None => true, Some(max_weight) => cs.min_input_weight().map_or(false, |min_input_weight| { - cs.weight(target.outputs, self.drain_weights) + min_input_weight + cs.weight(cs.target().outputs, self.drain_weights) + min_input_weight <= max_weight }), }; @@ -179,20 +173,18 @@ impl BnbMetric for LowestFee { Some(current_score) } else { - // Step 1: select everything up until the input that hits the target. - let (mut cs, resize_index, to_resize) = cs - .clone() - .select_iter() - .find(|(cs, _, _)| cs.is_funded(target))?; + // Step 1: select everything up until the input that hits the cs.target(). + let (mut cs, resize_index, to_resize) = + cs.clone().select_iter().find(|(cs, _, _)| cs.is_funded())?; // If this selection is already perfect, return its score directly. - if cs.excess(target, Drain::NONE) == 0 { - return Some(self.fee_score(&cs, target).unwrap().0); + if cs.excess(Drain::NONE) == 0 { + return Some(self.fee_score(&cs).unwrap().0); }; cs.deselect(resize_index); // We need to find the minimum fee we'd pay if we satisfy the feerate constraint. We do - // this by imagining we had a perfect input that perfectly hit the target. The sats per + // this by imagining we had a perfect input that perfectly hit the cs.target(). The sats per // weight unit of this perfect input is that of `to_resize` but we'll do a scaled // resize of it to fit perfectly. // @@ -208,12 +200,13 @@ impl BnbMetric for LowestFee { // // In the perfect scenario, no additional fee would be required to pay for rounding up when converting from weight units to // vbytes and so all fee calculations below are performed on weight units directly. - let rate_excess = cs.rate_excess_wu(target, Drain::NONE) as f32; + let rate_excess = cs.rate_excess_wu(Drain::NONE) as f32; let mut scale = Ordf32(0.0); if rate_excess < 0.0 { let remaining_value_to_reach_feerate = rate_excess.abs(); - let effective_value_of_resized_input = to_resize.effective_value(target.fee.rate); + let effective_value_of_resized_input = + to_resize.effective_value(cs.target().fee.rate); if effective_value_of_resized_input > 0.0 { let feerate_scale = remaining_value_to_reach_feerate / effective_value_of_resized_input; @@ -225,8 +218,8 @@ impl BnbMetric for LowestFee { // We can use the same approach for replacement we just have to use the // incremental_relay_feerate. - if let Some(replace) = target.fee.replace { - let replace_excess = cs.replacement_excess_wu(target, Drain::NONE) as f32; + if let Some(replace) = cs.target().fee.replace { + let replace_excess = cs.replacement_excess_wu(Drain::NONE) as f32; if replace_excess < 0.0 { let remaining_value_to_reach_feerate = replace_excess.abs(); let effective_value_of_resized_input = @@ -243,7 +236,7 @@ impl BnbMetric for LowestFee { // Handle absolute fee constraint. Unlike feerate and replacement, the // absolute fee is a fixed amount (not weight-proportional), so we just // need enough raw value to cover the gap. - let absolute_excess = cs.absolute_excess(target, Drain::NONE) as f32; + let absolute_excess = cs.absolute_excess(Drain::NONE) as f32; if absolute_excess < 0.0 { let remaining = absolute_excess.abs(); if to_resize.value > 0 { @@ -260,8 +253,8 @@ impl BnbMetric for LowestFee { // no within-cap selection down this branch reaches the target -> prune. This is the // fractional relaxation, so it never prunes a branch with an (integer) within-cap // solution. - if let Some(max_weight) = target.max_weight { - if cs.weight(target.outputs, DrainWeights::NONE) as f32 + if let Some(max_weight) = cs.target().max_weight { + if cs.weight(cs.target().outputs, DrainWeights::NONE) as f32 + scale.0 * to_resize.weight as f32 > max_weight as f32 { @@ -272,7 +265,7 @@ impl BnbMetric for LowestFee { // `scale` could be 0 even if `is_funded` is `false` due to the latter being based on // rounded-up vbytes. let ideal_fee = scale.0 * to_resize.value as f32 + cs.selected_value() as f32 - - target.value() as f32; + - cs.target().value() as f32; assert!(ideal_fee >= 0.0); Some(Ordf32(ideal_fee)) diff --git a/tests/bnb.rs b/tests/bnb.rs index 45a22dc..55cf5e7 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -32,8 +32,8 @@ struct MinExcessThenWeight; const EXCESS_RATIO: f32 = 1_000_000_f32; impl BnbMetric for MinExcessThenWeight { - fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option { - let excess = cs.excess(target, Drain::NONE); + fn score(&mut self, cs: &CoinSelector<'_>) -> Option { + let excess = cs.excess(Drain::NONE); if excess < 0 { None } else { @@ -43,13 +43,13 @@ impl BnbMetric for MinExcessThenWeight { } } - fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option { + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { let mut cs = cs.clone(); - cs.select_until_target_met(target).ok()?; + cs.select_until_target_met().ok()?; Some(Ordf32(cs.input_weight() as f32)) } - fn drain(&mut self, _cs: &CoinSelector<'_>, _target: Target) -> Drain { + fn drain(&mut self, _cs: &CoinSelector<'_>) -> Drain { Drain::NONE } } @@ -68,20 +68,12 @@ fn bnb_finds_an_exact_solution_in_n_iter() { }); let solution: Vec = (0..solution_len).map(|_| wv.next().unwrap()).collect(); - let solution_weight = { - let mut cs = CoinSelector::new(&solution); - cs.select_all(); - cs.input_weight() - }; - let target_value = solution.iter().map(|c| c.value).sum(); - let mut candidates = solution; + let mut candidates = solution.clone(); candidates.extend(wv.take(num_additional_canidates)); candidates.sort_unstable_by_key(|wv| core::cmp::Reverse(wv.value)); - let cs = CoinSelector::new(&candidates); - let target = Target { outputs: TargetOutputs { value_sum: target_value, @@ -93,7 +85,14 @@ fn bnb_finds_an_exact_solution_in_n_iter() { max_weight: None, }; - let solutions = cs.bnb_solutions(target, MinExcessThenWeight); + let solution_weight = { + let mut cs = CoinSelector::new(&solution, target); + cs.select_all(); + cs.input_weight() + }; + + let cs = CoinSelector::new(&candidates, target); + let solutions = cs.bnb_solutions(MinExcessThenWeight); let mut rounds = 0; let (best, score) = solutions @@ -116,8 +115,6 @@ fn bnb_finds_solution_if_possible_in_n_iter() { let wv = test_wv(&mut rng); let candidates = wv.take(num_inputs).collect::>(); - let cs = CoinSelector::new(&candidates); - let target = Target { outputs: TargetOutputs { value_sum: target_value, @@ -128,7 +125,8 @@ fn bnb_finds_solution_if_possible_in_n_iter() { max_weight: None, }; - let solutions = cs.bnb_solutions(target, MinExcessThenWeight); + let cs = CoinSelector::new(&candidates, target); + let solutions = cs.bnb_solutions(MinExcessThenWeight); let mut rounds = 0; let (sol, _score) = solutions @@ -139,7 +137,7 @@ fn bnb_finds_solution_if_possible_in_n_iter() { .expect("found a solution"); assert_eq!(rounds, 164); - let excess = sol.excess(target, Drain::NONE); + let excess = sol.excess(Drain::NONE); assert_eq!(excess, 0); } @@ -150,19 +148,18 @@ proptest! { let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha); let wv = test_wv(&mut rng); let candidates = wv.take(num_inputs).collect::>(); - let cs = CoinSelector::new(&candidates); let target = Target { outputs: TargetOutputs { value_sum: target_value, weight_sum: 0, n_outputs: 1 }, fee: TargetFee::ZERO, max_weight: None, }; - - let solutions = cs.bnb_solutions(target, MinExcessThenWeight); + let cs = CoinSelector::new(&candidates, target); + let solutions = cs.bnb_solutions(MinExcessThenWeight); match solutions.enumerate().filter_map(|(i, sol)| Some((i, sol?))).last() { Some((_i, (sol, _score))) => assert!(sol.selected_value() >= target_value), - _ => prop_assert!(!cs.is_fundable(target)), + _ => prop_assert!(!cs.is_fundable()), } } @@ -177,20 +174,25 @@ proptest! { let mut wv = test_wv(&mut rng); let solution: Vec = (0..solution_len).map(|_| wv.next().unwrap()).collect(); - let solution_weight = { - let mut cs = CoinSelector::new(&solution); - cs.select_all(); - cs.input_weight() - }; - let target_value = solution.iter().map(|c| c.value).sum(); - let mut candidates = solution; + let mut candidates = solution.clone(); candidates.extend(wv.take(num_additional_canidates)); - let mut cs = CoinSelector::new(&candidates); + let target = Target { + outputs: TargetOutputs { value_sum: target_value, weight_sum: 0, n_outputs: 1 }, + // we're trying to find an exact selection value so set fees to 0 + fee: TargetFee::ZERO, + max_weight: None, + }; + let solution_weight = { + let mut cs = CoinSelector::new(&solution, target); + cs.select_all(); + cs.input_weight() + }; + let mut cs = CoinSelector::new(&candidates, target); for i in 0..num_preselected.min(solution_len) { cs.select(i); } @@ -198,14 +200,7 @@ proptest! { // sort in descending value cs.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(wv.value)); - let target = Target { - outputs: TargetOutputs { value_sum: target_value, weight_sum: 0, n_outputs: 1 }, - // we're trying to find an exact selection value so set fees to 0 - fee: TargetFee::ZERO, - max_weight: None, - }; - - let solutions = cs.bnb_solutions(target, MinExcessThenWeight); + let solutions = cs.bnb_solutions(MinExcessThenWeight); let (_i, (best, _score)) = solutions .enumerate() diff --git a/tests/changeless.rs b/tests/changeless.rs index aac10a3..4e9ca81 100644 --- a/tests/changeless.rs +++ b/tests/changeless.rs @@ -53,7 +53,6 @@ proptest! { let wv = test_wv(&mut rng); let candidates = wv.take(n_candidates).collect::>(); - let cs = CoinSelector::new(&candidates); let target = Target { outputs: TargetOutputs { @@ -68,6 +67,7 @@ proptest! { }, max_weight: None, }; + let cs = CoinSelector::new(&candidates, target); let make_metric = || { Changeless(LowestFee { @@ -77,7 +77,7 @@ proptest! { }) }; - let solutions = cs.bnb_solutions(target, make_metric()); + let solutions = cs.bnb_solutions(make_metric()); println!("candidates: {:#?}", cs.candidates().collect::>()); @@ -94,7 +94,7 @@ proptest! { None => { let mut cs = cs.clone(); let mut metric = make_metric(); - let has_solution = common::exhaustive_search(&mut cs, target, &mut metric).is_some(); + let has_solution = common::exhaustive_search(&mut cs, &mut metric).is_some(); dbg!(format!("{}", cs)); assert!(!has_solution); } diff --git a/tests/common.rs b/tests/common.rs index 273b830..880d45c 100644 --- a/tests/common.rs +++ b/tests/common.rs @@ -51,7 +51,7 @@ where let target = params.target(); - let mut selection = CoinSelector::new(&candidates); + let mut selection = CoinSelector::new(&candidates, target); let mut exp_selection = selection.clone(); if metric.requires_ordering_by_descending_value_pwu() { @@ -61,8 +61,8 @@ where println!("\texhaustive search:"); let now = std::time::Instant::now(); - let exp_result = exhaustive_search(&mut exp_selection, target, &mut metric); - let exp_change = metric.drain(&exp_selection, target); + let exp_result = exhaustive_search(&mut exp_selection, &mut metric); + let exp_change = metric.drain(&exp_selection); let exp_result_str = result_string(&exp_result.ok_or("no possible solution"), exp_change); println!( "\t\telapsed={:8}s result={}", @@ -72,7 +72,7 @@ where // bonus check: ensure replacement fee is respected if exp_result.is_some() { let selected_value = exp_selection.selected_value(); - let drain = metric.drain(&exp_selection, target); + let drain = metric.drain(&exp_selection); let target_value = target.value(); let replace_fee = params .replace @@ -87,8 +87,8 @@ where println!("\tbranch and bound:"); let now = std::time::Instant::now(); let mut bnb_metric = metric.clone(); - let result = bnb_search(&mut selection, target, metric, usize::MAX); - let change = bnb_metric.drain(&selection, target); + let result = bnb_search(&mut selection, metric, usize::MAX); + let change = bnb_metric.drain(&selection); let result_str = result_string(&result, change); println!( "\t\telapsed={:8}s result={}", @@ -112,7 +112,7 @@ where // bonus check: ensure replacement fee is respected let selected_value = selection.selected_value(); - let drain = bnb_metric.drain(&selection, target); + let drain = bnb_metric.drain(&selection); let target_value = target.value(); let replace_fee = params .replace @@ -148,7 +148,7 @@ where let target = params.target(); let init_cs = { - let mut cs = CoinSelector::new(&candidates); + let mut cs = CoinSelector::new(&candidates, target); if metric.requires_ordering_by_descending_value_pwu() { cs.sort_candidates_by_descending_value_pwu(); } @@ -157,12 +157,12 @@ where print_candidates(¶ms, &init_cs); for (cs, _) in ExhaustiveIter::new(&init_cs).into_iter().flatten() { - if let Some(lb_score) = metric.bound(&cs, target) { + if let Some(lb_score) = metric.bound(&cs) { // This is the branch's lower bound. In other words, this is the BEST selection // possible (can overshoot) traversing down this branch. Let's check that! - if let Some(score) = metric.score(&cs, target) { - let has_change = metric.drain(&cs, target).is_some(); + if let Some(score) = metric.score(&cs) { + let has_change = metric.drain(&cs).is_some(); prop_assert!( score >= lb_score, "checking branch: selection={} score={} change={} lb={}", @@ -178,9 +178,9 @@ where .flatten() .filter(|(_, inc)| *inc) { - if let Some(descendant_score) = metric.score(&descendant_cs, target) { - let parent_has_change = metric.drain(&cs, target).is_some(); - let descendant_has_change = metric.drain(&descendant_cs, target).is_some(); + if let Some(descendant_score) = metric.score(&descendant_cs) { + let parent_has_change = metric.drain(&cs).is_some(); + let descendant_has_change = metric.drain(&descendant_cs).is_some(); prop_assert!( descendant_score >= lb_score, " @@ -190,7 +190,7 @@ where cs, parent_has_change, lb_score, - cs.is_funded(target), + cs.is_funded(), descendant_cs, descendant_has_change, descendant_score, @@ -340,11 +340,7 @@ impl<'a> Iterator for ExhaustiveIter<'a> { } } -pub fn exhaustive_search( - cs: &mut CoinSelector, - target: Target, - metric: &mut M, -) -> Option<(Ordf32, usize)> +pub fn exhaustive_search(cs: &mut CoinSelector, metric: &mut M) -> Option<(Ordf32, usize)> where M: BnbMetric, { @@ -359,7 +355,7 @@ where .enumerate() .inspect(|(i, _)| rounds = *i) .filter(|(_, (_, inclusion))| *inclusion) - .filter_map(|(_, (cs, _))| metric.score(&cs, target).map(|score| (cs, score))); + .filter_map(|(_, (cs, _))| metric.score(&cs).map(|score| (cs, score))); for (child_cs, score) in iter { match &mut best { @@ -388,10 +384,8 @@ where /// [`CoinSelector::is_funded`] + [`CoinSelector::is_within_max_weight`], so it inherits the /// exact weight model and is independent of the BnB weight prune it audits. Exponential — small `n` /// only. -pub fn exact_selection_possible(cs: &CoinSelector, target: Target) -> bool { - let feasible = |s: &CoinSelector| { - s.is_funded(target) && s.is_within_max_weight(target, DrainWeights::NONE) - }; +pub fn exact_selection_possible(cs: &CoinSelector) -> bool { + let feasible = |s: &CoinSelector| s.is_funded() && s.is_within_max_weight(DrainWeights::NONE); // the current selection itself (no additions) is a valid subset and isn't yielded by the iter feasible(cs) || ExhaustiveIter::new(cs) @@ -401,7 +395,6 @@ pub fn exact_selection_possible(cs: &CoinSelector, target: Target) -> bool { pub fn bnb_search( cs: &mut CoinSelector, - target: Target, metric: M, max_rounds: usize, ) -> Result<(Ordf32, usize), NoBnbSolution> @@ -410,7 +403,7 @@ where { let mut rounds = 0_usize; let (selection, score) = cs - .bnb_solutions(target, metric) + .bnb_solutions(metric) .inspect(|_| rounds += 1) .take(max_rounds) .flatten() @@ -448,8 +441,8 @@ pub fn compare_against_benchmarks( let start = std::time::Instant::now(); let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha); let target = params.target(); - let cs = CoinSelector::new(&candidates); - let solutions = cs.bnb_solutions(target, metric.clone()); + let cs = CoinSelector::new(&candidates, target); + let solutions = cs.bnb_solutions(metric.clone()); let best = solutions .enumerate() @@ -465,7 +458,7 @@ pub fn compare_against_benchmarks( core::cmp::Reverse(Ordf32(wv.effective_value(target.fee.rate))) }); // we filter out failing onces below - let _ = naive_select.select_until_target_met(target); + let _ = naive_select.select_until_target_met(); naive_select }, { @@ -485,7 +478,7 @@ pub fn compare_against_benchmarks( // exists, so the comparison below isn't vacuous. let mut greedy = cs.clone(); greedy.sort_candidates_by_descending_value_pwu(); - let _ = greedy.select_until_target_met(target); + let _ = greedy.select_until_target_met(); greedy }, ]; @@ -501,11 +494,11 @@ pub fn compare_against_benchmarks( let cmp_benchmarks = cmp_benchmarks .into_iter() .filter_map(|cs| { - let score = metric.clone().score(&cs, target)?; + let score = metric.clone().score(&cs)?; Some((cs, score)) }) .collect::>(); - let sol_score = metric.score(&sol, target); + let sol_score = metric.score(&sol); for (_bench_id, (mut bench, bench_score)) in cmp_benchmarks.into_iter().enumerate() { prop_assert!( @@ -526,7 +519,7 @@ pub fn compare_against_benchmarks( None => { // Full feasibility (value *and* max_weight) is needed here; `is_fundable` // only covers value, so use the exact exhaustive oracle to assert impossibility. - prop_assert!(!exact_selection_possible(&cs, target)); + prop_assert!(!exact_selection_possible(&cs)); } } @@ -546,8 +539,8 @@ fn randomly_satisfy_target<'a, R: rand::Rng>( let mut last_score: Option = None; while let Some(next) = cs.unselected_indices().choose(rng) { cs.select(next); - if cs.is_funded(target) { - let curr_score = metric.score(&cs, target); + if cs.is_funded() { + let curr_score = metric.score(&cs); if let Some(last_score) = last_score { if curr_score.is_none() || curr_score.unwrap() > last_score { break; diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index d6b8cba..1d7538b 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -91,11 +91,11 @@ proptest! { params.n_candidates ]; - let mut cs = CoinSelector::new(&candidates); + let mut cs = CoinSelector::new(&candidates, params.target()); let metric = params.lowest_fee_metric(); - let is_impossible = !cs.is_fundable(params.target()); - match common::bnb_search(&mut cs, params.target(), metric, params.n_candidates * 10) { + let is_impossible = !cs.is_fundable(); + match common::bnb_search(&mut cs, metric, params.n_candidates * 10) { Ok((score, rounds)) => { // the +1 is because the iterator will always try selecting nothing as a solution so we have // to do one extra iteration to try that @@ -162,10 +162,10 @@ proptest! { let target = params.target(); let metric = params.lowest_fee_metric(); - let exact_possible = common::exact_selection_possible(&CoinSelector::new(&candidates), target); + let exact_possible = common::exact_selection_possible(&CoinSelector::new(&candidates, target)); - let mut cs = CoinSelector::new(&candidates); - let bnb_found = common::bnb_search(&mut cs, target, metric, usize::MAX).is_ok(); + let mut cs = CoinSelector::new(&candidates, target); + let bnb_found = common::bnb_search(&mut cs, metric, usize::MAX).is_ok(); prop_assert_eq!( bnb_found, exact_possible, "bnb_found={} but exact_possible={} (weight prune may have dropped a feasible subtree)", @@ -195,23 +195,21 @@ fn combined_changeless_metric() { }; let candidates = common::gen_candidates(params.n_candidates); - let mut cs_a = CoinSelector::new(&candidates); - let mut cs_b = CoinSelector::new(&candidates); - let target = params.target(); + let mut cs_a = CoinSelector::new(&candidates, target); + let mut cs_b = CoinSelector::new(&candidates, target); let metric_lowest_fee = params.lowest_fee_metric(); let metric_changeless = Changeless(params.lowest_fee_metric()); // cs_a uses the unconstrained metric - let (score, rounds) = common::bnb_search(&mut cs_a, target, metric_lowest_fee, usize::MAX) - .expect("must find solution"); + let (score, rounds) = + common::bnb_search(&mut cs_a, metric_lowest_fee, usize::MAX).expect("must find solution"); println!("score={:?} rounds={}", score, rounds); // cs_b uses the changeless-constrained metric let (combined_score, combined_rounds) = - common::bnb_search(&mut cs_b, target, metric_changeless, usize::MAX) - .expect("must find solution"); + common::bnb_search(&mut cs_b, metric_changeless, usize::MAX).expect("must find solution"); println!("score={:?} rounds={}", combined_score, combined_rounds); assert!(combined_rounds >= rounds); @@ -256,7 +254,7 @@ fn does_not_create_change_below_spend_cost() { }, ]; - let mut cs = CoinSelector::new(&candidates); + let mut cs = CoinSelector::new(&candidates, target); let drain_weights = DrainWeights { output_weight: 100, @@ -270,17 +268,17 @@ fn does_not_create_change_below_spend_cost() { drain_weights, }; - let (score, _) = common::bnb_search(&mut cs, target, metric, 10).expect("finds solution"); + let (score, _) = common::bnb_search(&mut cs, metric, 10).expect("finds solution"); // The optimal selection is candidate 0 alone, and it must be changeless. let expected = { - let mut expected = CoinSelector::new(&candidates); + let mut expected = CoinSelector::new(&candidates, target); expected.select(0); expected }; assert_eq!(cs.selected_indices(), expected.selected_indices()); assert!( - metric.drain(&cs, target).is_none(), + metric.drain(&cs).is_none(), "optimal selection must be changeless" ); @@ -290,12 +288,7 @@ fn does_not_create_change_below_spend_cost() { with_extra_input.select(2); with_extra_input }; - assert!( - score - <= metric - .score(&with_extra_input, target) - .expect("target is met") - ); + assert!(score <= metric.score(&with_extra_input).expect("target is met")); } #[test] @@ -338,14 +331,13 @@ fn zero_fee_tx() { n_outputs: 1, }; - let mut cs = CoinSelector::new(&candidates); + let mut cs = CoinSelector::new(&candidates, target); let metric = LowestFee { long_term_feerate, dust_relay_feerate: FeeRate::from_sat_per_vb(1.0), drain_weights, }; - let (_score, _rounds) = - common::bnb_search(&mut cs, target, metric, 1000).expect("must find solution"); + let (_score, _rounds) = common::bnb_search(&mut cs, metric, 1000).expect("must find solution"); } // --- `run_bnb` failure classification (`NoBnbSolution` variants) --- @@ -379,14 +371,14 @@ fn err_outputs(value_sum: u64) -> TargetOutputs { fn run_bnb_reports_insufficient_funds() { // Two 100k inputs can't cover a 10M target: the value is simply unreachable. let candidates = [err_candidate(100_000), err_candidate(100_000)]; - let mut cs = CoinSelector::new(&candidates); let target = Target { outputs: err_outputs(10_000_000), fee: TargetFee::ZERO, max_weight: None, }; + let mut cs = CoinSelector::new(&candidates, target); assert_eq!( - cs.run_bnb(target, err_metric(), 100_000).unwrap_err(), + cs.run_bnb(err_metric(), 100_000).unwrap_err(), NoBnbSolution::InsufficientFunds, ); } @@ -400,14 +392,14 @@ fn run_bnb_reports_max_weight_exceeded() { err_candidate(100_000), err_candidate(100_000), ]; - let mut cs = CoinSelector::new(&candidates); let target = Target { outputs: err_outputs(250_000), fee: TargetFee::ZERO, max_weight: Some(1), }; + let mut cs = CoinSelector::new(&candidates, target); assert_eq!( - cs.run_bnb(target, err_metric(), 100_000).unwrap_err(), + cs.run_bnb(err_metric(), 100_000).unwrap_err(), NoBnbSolution::MaxWeightExceeded, ); } @@ -420,14 +412,14 @@ fn run_bnb_reports_round_limit() { err_candidate(100_000), err_candidate(100_000), ]; - let mut cs = CoinSelector::new(&candidates); let target = Target { outputs: err_outputs(250_000), fee: TargetFee::ZERO, max_weight: None, }; + let mut cs = CoinSelector::new(&candidates, target); assert_eq!( - cs.run_bnb(target, err_metric(), 0).unwrap_err(), + cs.run_bnb(err_metric(), 0).unwrap_err(), NoBnbSolution::RoundLimit { max_rounds: 0, rounds: 0, diff --git a/tests/srd.rs b/tests/srd.rs index b1b3096..1f7c9f5 100644 --- a/tests/srd.rs +++ b/tests/srd.rs @@ -36,8 +36,8 @@ fn srd_success_yields_healthy_change_that_meets_target() { let mut successes = 0; for seed in 0..300u64 { - let mut cs = CoinSelector::new(&candidates); - let result = cs.select_srd(target, drain_weights, CHANGE_LOWER, splitmix64(seed)); + let mut cs = CoinSelector::new(&candidates, target); + let result = cs.select_srd(drain_weights, CHANGE_LOWER, splitmix64(seed)); if let Ok(drain) = result { successes += 1; @@ -49,18 +49,15 @@ fn srd_success_yields_healthy_change_that_meets_target() { ); assert_eq!(drain.weights, drain_weights); assert!( - cs.is_funded_with_drain(target, drain), + cs.is_funded_with_drain(drain), "seed {}: target not met with the returned drain", seed ); // The reported change equals the actual excess available to the drain. - let excess = cs.excess( - target, - Drain { - weights: drain_weights, - value: 0, - }, - ); + let excess = cs.excess(Drain { + weights: drain_weights, + value: 0, + }); assert_eq!(drain.value as i64, excess); } } @@ -95,8 +92,8 @@ fn srd_insufficient_funds() { let drain_weights = DrainWeights::TR_KEYSPEND; for seed in 0..50u64 { - let mut cs = CoinSelector::new(&candidates); - let result = cs.select_srd(target, drain_weights, CHANGE_LOWER, splitmix64(seed)); + let mut cs = CoinSelector::new(&candidates, target); + let result = cs.select_srd(drain_weights, CHANGE_LOWER, splitmix64(seed)); assert!( matches!(result, Err(SelectError::InsufficientFunds(_))), "seed {}: expected InsufficientFunds, got {:?}", @@ -129,9 +126,9 @@ fn srd_max_weight_exceeded() { }; // Weight of the smallest selection that reaches target + change_lower, with no cap. - let mut probe = CoinSelector::new(&candidates); + let mut probe = CoinSelector::new(&candidates, target(200_000, 5.0)); probe - .select_until(|cs| cs.excess(target(200_000, 5.0), drain) >= CHANGE_LOWER as i64) + .select_until(|cs| cs.excess(drain) >= CHANGE_LOWER as i64) .expect("candidates can cover target + change_lower"); let needed_weight = probe.weight(target(200_000, 5.0).outputs, drain_weights); @@ -142,8 +139,8 @@ fn srd_max_weight_exceeded() { }; for seed in 0..20u64 { - let mut cs = CoinSelector::new(&candidates); - let result = cs.select_srd(capped, drain_weights, CHANGE_LOWER, splitmix64(seed)); + let mut cs = CoinSelector::new(&candidates, capped); + let result = cs.select_srd(drain_weights, CHANGE_LOWER, splitmix64(seed)); assert!( matches!(result, Err(SelectError::MaxWeightExceeded)), "seed {}: expected MaxWeightExceeded, got {:?}", @@ -166,13 +163,13 @@ fn srd_adds_nothing_when_already_sufficient() { }; // Preselect enough that the change already exceeds `change_lower`. - let mut cs = CoinSelector::new(&candidates); - cs.select_until(|cs| cs.excess(target, drain) >= CHANGE_LOWER as i64) + let mut cs = CoinSelector::new(&candidates, target); + cs.select_until(|cs| cs.excess(drain) >= CHANGE_LOWER as i64) .expect("candidates can cover target + change_lower"); let before: Vec = cs.selected_indices().iter().collect(); let out = cs - .select_srd(target, drain_weights, CHANGE_LOWER, splitmix64(3)) + .select_srd(drain_weights, CHANGE_LOWER, splitmix64(3)) .expect("already sufficient"); let after: Vec = cs.selected_indices().iter().collect(); diff --git a/tests/weight.rs b/tests/weight.rs index 6a8dbb5..9e0883d 100644 --- a/tests/weight.rs +++ b/tests/weight.rs @@ -1,6 +1,8 @@ #![allow(clippy::zero_prefixed_literal)] -use bdk_coin_select::{Candidate, CoinSelector, Drain, DrainWeights, TargetOutputs}; +use bdk_coin_select::{ + Candidate, CoinSelector, Drain, DrainWeights, Target, TargetFee, TargetOutputs, +}; use bitcoin::{consensus::Decodable, ScriptBuf, Transaction}; fn hex_val(c: u8) -> u8 { @@ -46,7 +48,12 @@ fn segwit_one_input_one_output() { n_outputs: tx.output.len(), }; - let mut coin_selector = CoinSelector::new(&candidates); + let target = Target { + outputs: target_ouputs, + fee: TargetFee::ZERO, + max_weight: None, + }; + let mut coin_selector = CoinSelector::new(&candidates, target); coin_selector.select_all(); assert_eq!( @@ -83,13 +90,17 @@ fn segwit_two_inputs_one_output() { }) .collect::>(); - let mut coin_selector = CoinSelector::new(&candidates); - let target_ouputs = TargetOutputs { value_sum: tx.output.iter().map(|output| output.value.to_sat()).sum(), weight_sum: tx.output.iter().map(|output| output.weight().to_wu()).sum(), n_outputs: tx.output.len(), }; + let target = Target { + outputs: target_ouputs, + fee: TargetFee::ZERO, + max_weight: None, + }; + let mut coin_selector = CoinSelector::new(&candidates, target); coin_selector.select_all(); @@ -133,7 +144,12 @@ fn legacy_three_inputs() { n_outputs: tx.output.len(), }; - let mut coin_selector = CoinSelector::new(&candidates); + let target = Target { + outputs: target_ouputs, + fee: TargetFee::ZERO, + max_weight: None, + }; + let mut coin_selector = CoinSelector::new(&candidates, target); coin_selector.select_all(); assert_eq!( @@ -191,7 +207,12 @@ fn legacy_three_inputs_one_segwit() { n_outputs: tx.output.len(), }; - let mut coin_selector = CoinSelector::new(&candidates); + let target = Target { + outputs: target_ouputs, + fee: TargetFee::ZERO, + max_weight: None, + }; + let mut coin_selector = CoinSelector::new(&candidates, target); coin_selector.select_all(); assert_eq!( From 9ee9ab9d4f3cdd8f12354255d437054bf029ecef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 4 Aug 2026 03:51:10 +0000 Subject: [PATCH 2/5] Add exact CPFP package pricing with local per-candidate search figures When spending unconfirmed UTXOs, miners evaluate the transaction as a package with its unconfirmed ancestors; if the ancestors paid below the target feerate, the child must cover the difference (CPFP). This makes coin selection price that correctly, without banning anything from selection and without capping how many candidates may carry ancestors. `Cluster` holds the relevant piece of the mempool as a graph -- weights, fees, direct parent edges, and which candidate spends which -- and computes transitive ancestor closures itself, so a caller cannot under-price a package by listing only direct parents. `Cluster::mine` builds a mock block template at the target feerate using the same greedy ancestor-feerate loop Bitcoin Core's `MiniMiner` uses: whatever the template includes is already paying its own way, and a package's bump is what its remaining ancestors still owe. Package feerates are compared by cross-multiplication in `u128` so vbyte rounding cannot leak into the template's ordering. `BumpTable`, built from a cluster at one feerate, answers two different questions, and the difference is the whole design: - `individual(c)` -- what one candidate owes alone. Additive, so an over-estimate when ancestors are shared, but *local*: netted off by the new `CoinSelector::effective_value_of` and `value_pwu_of`, it makes the per-candidate figures the selection algorithms rank on tell the whole truth. - the combined package figure -- what a set of candidates owes together, exactly. What `excess`, `implied_fee`, `is_funded` and `drain` report, and what the transaction has to pay. A selector commits to one model at a time (`Pricing`), never mixing them: a bound computed against one and scored against the other is not a bound. `BnbIter` searches in the local model, and selections handed back are exactly priced, so `run_bnb`'s drain is sized by the true package cost -- the local over-estimate surfaces as a larger change output, never as a missing fee. This is the split Bitcoin Core draws between `calculateIndividualBumpFees` and `calculateCombinedBumpFee`. It relies on the local sum never falling below the combined figure, which the mock template guarantees (an overpaying ancestor is mined out, so whatever two candidates share is itself ancestor-closed and therefore deficient); `the_local_sum_never_undercuts_the_package_for_any_cluster` pins that over 2000 generated clusters. The accessors live on `CoinSelector` rather than on `Candidate`: a bump is `feerate * weight - fee_paid` over the unconfirmed ancestors, so it only means anything at the feerate it was derived for, and a `Candidate` has nowhere to record which. The selector holds the table and checks. `LowestFee::bound` needed three fixes, all the same mistake -- a bump is a fixed cost attached to an input, not a rate, so per-weight reasoning does not apply to it: - The greedy prefix charged bumps of candidates the node had not selected; a descendant may skip those candidates and never pay them, so only committed bumps may enter the deficit. - The hypothetical resized input must be priced bump-free, since scaling an input does not scale the ancestors it drags in. - The exclusion branch treated equal value and weight as interchangeable, which no longer holds when two inputs drag in different ancestors. Measured against an oracle over ~75k bound evaluations, these took over-estimates from 0.65%/2.03% of calls (k=1/k=3) to 0.01%/0.07%, and the answer gap versus the brute-forced optimum to at most 0.02% for k <= 8. `src/ancestor_search_experiment.rs` keeps the measurements runnable as ignored tests and records the two approaches this replaced: banning ancestor-carrying candidates (up to 3.07% worse answers, and insufficient-funds failures on up to 100% of instances once most candidates are unconfirmed), and repairing the bound with a `min_bump` floor over the exact model (prunes almost nothing, because the bound prices a greedy prefix and the least bump reachable from the root is always zero). Co-Authored-By: Noah Joeris Co-Authored-By: Claude Opus 5 --- README.md | 10 +- src/ancestor_search_experiment.rs | 560 ++++++++++++++++++++++++++++++ src/bnb.rs | 19 +- src/bump_table.rs | 159 +++++++++ src/coin_selector.rs | 290 ++++++++++++++-- src/lib.rs | 6 + src/mempool.rs | 300 ++++++++++++++++ src/metrics/changeless.rs | 6 +- src/metrics/lowest_fee.rs | 31 +- tests/ancestor_aware.rs | 359 +++++++++++++++++++ tests/cpfp_cluster.rs | 311 +++++++++++++++++ 11 files changed, 2000 insertions(+), 51 deletions(-) create mode 100644 src/ancestor_search_experiment.rs create mode 100644 src/bump_table.rs create mode 100644 src/mempool.rs create mode 100644 tests/ancestor_aware.rs create mode 100644 tests/cpfp_cluster.rs diff --git a/README.md b/README.md index 922dd25..be82fd8 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ let candidates = vec![ weight: TR_KEYSPEND_TXIN_WEIGHT, // wether it's a segwit input. Needed so we know whether to include the // segwit header in total weight calculations. - is_segwit: true + is_segwit: true, }, Candidate { // A candidate can represent multiple inputs in the case where you @@ -49,7 +49,7 @@ let candidates = vec![ input_count: 2, weight: 2*TR_KEYSPEND_TXIN_WEIGHT, value: 3_000_000, - is_segwit: true + is_segwit: true, } ]; @@ -108,19 +108,19 @@ let candidates = [ input_count: 1, value: 400_000, weight: TR_KEYSPEND_TXIN_WEIGHT, - is_segwit: true + is_segwit: true, }, Candidate { input_count: 1, value: 200_000, weight: TR_KEYSPEND_TXIN_WEIGHT, - is_segwit: true + is_segwit: true, }, Candidate { input_count: 1, value: 11_000, weight: TR_KEYSPEND_TXIN_WEIGHT, - is_segwit: true + is_segwit: true, } ]; let drain_weights = bdk_coin_select::DrainWeights::default(); diff --git a/src/ancestor_search_experiment.rs b/src/ancestor_search_experiment.rs new file mode 100644 index 0000000..fc3f665 --- /dev/null +++ b/src/ancestor_search_experiment.rs @@ -0,0 +1,560 @@ +//! Measurement, not production code: what does searching on the *local* ancestor bump model cost +//! in answer quality? +//! +//! Branch and bound ranks candidates on [`CoinSelector::effective_value_of`], which nets off each +//! candidate's *individual* bump. That is additive, and so over-states what a package owes whenever +//! ancestors are shared -- which is what makes it safe to search on, but means the search optimises +//! a slightly wrong objective. This measures the gap against the exactly-priced optimum. Run with: +//! +//! ```text +//! cargo test --release ancestor_search_experiment -- --ignored --nocapture +//! ``` +//! +//! # Two approaches this replaced +//! +//! **Banning** candidates with unconfirmed ancestors, so per-candidate figures could stay +//! ancestor-blind. Measured over 300 random instances per row (n=12, k=3, optimum by brute force), +//! sweeping the share of parents already paying the going rate, the ban cost up to 3.07% in answer +//! quality — and unbanning only the candidates that provably never change the bump recovered all +//! but 0.03% of it: +//! +//! ```text +//! p(fine) neutral ban-cost selective sel-rds no-anc +//! 0% 0% 0.03% 0.03% 51 107 +//! 25% 25% 0.95% 0.01% 70 116 +//! 50% 50% 1.50% 0.01% 75 103 +//! 75% 80% 2.57% 0.00% 92 104 +//! 100% 100% 3.07% 0.00% 110 110 +//! ``` +//! +//! **Keeping the exact model everywhere** and repairing `LowestFee::bound` with a `min_bump` +//! floor — the least bump any reachable superset could pay. It prunes almost nothing: 17-23x the +//! search of an ancestor-free problem, and only 1.0-1.34x better than assuming the bump vanishes +//! entirely. Two reasons compound. `LowestFee::bound` prices a greedy *prefix*, i.e. a superset of +//! the node, so the give-back must cover `max_bump - min_bump` rather than `bump - min_bump`. And +//! `min_bump` at the root is always zero, because the empty selection is reachable and pays +//! nothing — so where pruning matters most the whole bump is surrendered. An oracle bound needed +//! 5-6 rounds where the `min_bump` bound needed 500-650, so the objective was easy to search and +//! the bound was the problem; chasing that was not worth it against a prize this small. +//! +//! [`CoinSelector::effective_value_of`]: crate::CoinSelector::effective_value_of + +use crate::{ + float::Ordf32, metrics::LowestFee, BnbMetric, BumpTable, Candidate, Cluster, CoinSelector, + DrainWeights, FeeRate, MempoolTx, Target, TargetFee, TargetOutputs, +}; +use alloc::vec::Vec; + +/// A deterministic xorshift, so a surprising result can be reproduced from its seed. +struct Rng(u64); + +impl Rng { + fn next(&mut self) -> u64 { + self.0 ^= self.0 << 13; + self.0 ^= self.0 >> 7; + self.0 ^= self.0 << 17; + self.0 + } + + fn in_range(&mut self, lo: u64, hi: u64) -> u64 { + lo + self.next() % (hi - lo) + } +} + +struct Instance { + candidates: Vec, + table: BumpTable, + target: Target, +} + +fn metric() -> LowestFee { + LowestFee { + long_term_feerate: FeeRate::from_sat_per_vb(5.0), + dust_relay_feerate: FeeRate::from_sat_per_vb(1.0), + drain_weights: DrainWeights::TR_KEYSPEND, + } +} + +/// A cluster where `p_fine_pct`% of parents already pay at or above the cs.target() feerate (so a miner +/// takes them and they cost nothing), and the rest are stuck below it. +fn instance(rng: &mut Rng, n: usize, k: usize, p_fine_pct: u64) -> Option { + let feerate = FeeRate::from_sat_per_vb(10.0); + + let candidates = (0..n) + .map(|_| Candidate { + input_count: 1, + value: rng.in_range(20_000, 200_000), + weight: crate::TR_KEYSPEND_TXIN_WEIGHT, + is_segwit: true, + }) + .collect::>(); + + let mut txs: Vec = Vec::new(); + let mut spends: Vec<(usize, usize)> = Vec::new(); + + let push_tx = |txs: &mut Vec, rng: &mut Rng, parents: Vec| -> usize { + let weight = rng.in_range(400, 1_600) / 4 * 4; + // Fee as a multiple of what the cs.target() feerate would require: "fine" parents pay 1.0-3.0x + // and get mined, "stuck" ones pay 0.05-0.8x and need bumping. + let mult_pct = if rng.in_range(0, 100) < p_fine_pct { + rng.in_range(100, 300) + } else { + rng.in_range(5, 80) + }; + let fee = (weight / 4) * 10 * mult_pct / 100; + txs.push(MempoolTx { + weight, + fee, + parents, + }); + txs.len() - 1 + }; + + for c in 0..k { + let tx = match c { + // Candidate 1 shares candidate 0's parent, so shared-ancestor dedup is in play -- + // exactly where local and exact pricing diverge. + 1 if !spends.is_empty() => spends[0].1, + // Candidate 2 sits one level deeper, so transitive closure is in play. + 2 => { + let grandparent = push_tx(&mut txs, rng, alloc::vec![]); + push_tx(&mut txs, rng, alloc::vec![grandparent]) + } + _ => push_tx(&mut txs, rng, alloc::vec![]), + }; + spends.push((c, tx)); + } + + let cluster = Cluster::new(txs, spends).ok()?; + let table = BumpTable::from_cluster(&cluster, feerate); + + Some(Instance { + candidates, + table, + target: Target { + outputs: TargetOutputs { + value_sum: rng.in_range(100_000, 400_000), + weight_sum: 200, + n_outputs: 1, + }, + fee: TargetFee::from_feerate(feerate), + max_weight: None, + }, + }) +} + +/// An exactly-priced selector: what the caller sees, and what scores are compared on. +fn exact<'a>(inst: &'a Instance) -> CoinSelector<'a> { + CoinSelector::new(&inst.candidates, inst.target).with_bump_table(&inst.table) +} + +/// The best exactly-priced score over every subset. +fn brute_force(inst: &Instance) -> Option { + let mut best: Option = None; + let mut m = metric(); + for mask in 0..(1_u32 << inst.candidates.len()) { + let mut cs = exact(inst); + for i in 0..inst.candidates.len() { + if mask & (1 << i) != 0 { + cs.select(i); + } + } + if let Some(score) = m.score(&cs) { + if best.map_or(true, |b| score < b) { + best = Some(score); + } + } + } + best +} + +/// The best exactly-priced score reachable when only `allowed` candidates may be selected. +fn brute_force_allowing(inst: &Instance, allowed: &[usize]) -> Option { + let mut best: Option = None; + let mut m = metric(); + for mask in 0..(1_u32 << inst.candidates.len()) { + if (0..inst.candidates.len()).any(|i| mask & (1 << i) != 0 && !allowed.contains(&i)) { + continue; + } + let mut cs = exact(inst); + for i in 0..inst.candidates.len() { + if mask & (1 << i) != 0 { + cs.select(i); + } + } + if let Some(score) = m.score(&cs) { + if best.map_or(true, |b| score < b) { + best = Some(score); + } + } + } + best +} + +/// The alternative policy: keep exact pricing everywhere and ban the candidates that actually owe +/// something, leaving the rest selectable. A candidate owes nothing exactly when its unmined +/// ancestor closure is empty, so this needs no subset enumeration -- `individual(c) == 0` decides +/// it. +fn selectable_under_ban(inst: &Instance) -> Vec { + (0..inst.candidates.len()) + .filter(|&c| inst.table.individual(c) == 0) + .collect() +} + +/// The same candidates with every ancestor bump zeroed — an ancestor-free problem of the same +/// shape, used to separate what *local pricing* costs from what branch and bound costs anyway. +fn without_ancestors(inst: &Instance) -> CoinSelector<'_> { + CoinSelector::new(&inst.candidates, inst.target) +} + +/// Rounds, and how far branch and bound lands from the brute-force optimum, on the ancestor-free +/// problem. Any gap here is the search's own, not local pricing's. +fn baseline(inst: &Instance, max_rounds: usize) -> (usize, f64) { + let n = inst.candidates.len(); + let rounds = without_ancestors(inst) + .bnb_solutions(metric()) + .take(max_rounds) + .count(); + + let mut best: Option = None; + let mut m = metric(); + for mask in 0..(1_u32 << n) { + let mut cs = without_ancestors(inst); + for i in 0..n { + if mask & (1 << i) != 0 { + cs.select(i); + } + } + if let Some(score) = m.score(&cs) { + if best.map_or(true, |b| score < b) { + best = Some(score); + } + } + } + + let mut cs = without_ancestors(inst); + let gap = match (cs.run_bnb(metric(), max_rounds), best) { + (Ok(_), Some(optimum)) => metric() + .score(&cs) + .map_or(0.0, |s| (s.0 - optimum.0) as f64 / optimum.0 as f64), + _ => 0.0, + }; + (rounds, gap) +} + +#[test] +#[ignore = "measurement, not a correctness test; run with --nocapture"] +fn measure_local_search_quality() { + const N: usize = 12; + const K: usize = 3; + const TRIALS: usize = 300; + const MAX_ROUNDS: usize = 100_000; + + std::println!( + "n={}, k={}, {} trials per row, brute force = {} subsets\n", + N, + K, + TRIALS, + 1 << N + ); + std::println!( + "{:>8} {:>10} {:>10} {:>10} {:>12} {:>10}", + "p(fine)", + "local-gap", + "ban-gap", + "ban-fails", + "suboptimal", + "base-gap" + ); + + for p_fine in [0_u64, 25, 50, 75, 100] { + let mut rng = Rng(0xD1B54A32D192ED03 ^ (p_fine + 1) << 32); + let mut trials = 0; + let mut gap = 0f64; + let mut suboptimal = 0; + let (mut bnb_rounds, mut plain_rounds) = (0usize, 0usize); + let mut base_gap = 0f64; + let mut ban_gap = 0f64; + let mut ban_fails = 0; + + while trials < TRIALS { + let inst = match instance(&mut rng, N, K, p_fine) { + Some(inst) => inst, + None => continue, + }; + let optimum = match brute_force(&inst) { + Some(optimum) => optimum, + None => continue, + }; + trials += 1; + + bnb_rounds += exact(&inst) + .bnb_solutions(metric()) + .take(MAX_ROUNDS) + .count(); + let (rounds, gap_without) = baseline(&inst, MAX_ROUNDS); + plain_rounds += rounds; + base_gap += gap_without; + + // The alternative policy: what can it reach, and can it fund at all? + match brute_force_allowing(&inst, &selectable_under_ban(&inst)) { + Some(banned_opt) => ban_gap += (banned_opt.0 - optimum.0) as f64 / optimum.0 as f64, + // A funding selection exists, but not one this policy may reach: automatic + // selection would report insufficient funds. Not a quality loss -- a failure. + None => ban_fails += 1, + } + + let mut cs = exact(&inst); + if cs.run_bnb(metric(), MAX_ROUNDS).is_ok() { + // Re-score what branch and bound chose against the *exact* model. Its own score is + // the local one, which is a search artifact rather than what the caller pays. + if let Some(score) = metric().score(&cs) { + gap += (score.0 - optimum.0) as f64 / optimum.0 as f64; + if score > optimum { + suboptimal += 1; + } + } + } + } + + let t = trials as f64; + std::println!( + "{:>7}% {:>9.3}% {:>9.3}% {:>9.1}% {:>11.1}% {:>9.3}%", + p_fine, + 100.0 * gap / t, + 100.0 * ban_gap / t, + 100.0 * ban_fails as f64 / t, + 100.0 * suboptimal as f64 / t, + 100.0 * base_gap / t, + ); + let _ = (bnb_rounds, plain_rounds); + } + + std::println!( + "\np(fine): share of parents already paying the cs.target() rate. local-gap: how much worse branch\n\ + and bound's answer is than the exactly-priced optimum. base-gap: the same measured on\n\ + the ancestor-free problem, i.e. the search's own error rather than local pricing's.\n\ + suboptimal: share of instances where it is worse at all. bnb-rds/no-anc: mean rounds, and the same instance with the\n\ + ancestor bumps zeroed." + ); +} + +/// The case the sweep above cannot reach: a wallet where *most* candidates carry unconfirmed +/// ancestors, so banning them removes most of the pool. Run at `p(fine)=0`, where every parent is +/// stuck and every ancestor-carrying candidate is therefore banned — the worst case for that +/// policy, and the one that decides whether banning is viable at all. +#[test] +#[ignore = "measurement, not a correctness test; run with --nocapture"] +fn measure_when_most_candidates_are_unconfirmed() { + const N: usize = 12; + const TRIALS: usize = 300; + const MAX_ROUNDS: usize = 100_000; + + std::println!("n={}, p(fine)=0, {} trials per row\n", N, TRIALS); + std::println!( + "{:>3} {:>10} {:>10} {:>11} {:>12}", + "k", + "local-gap", + "ban-gap", + "ban-fails", + "suboptimal" + ); + + for k in [1_usize, 2, 4, 6, 8, 10, 12] { + let mut rng = Rng(0x853C49E6748FEA9B ^ (k as u64) << 32); + let mut trials = 0; + let (mut gap, mut ban_gap) = (0f64, 0f64); + let (mut ban_fails, mut suboptimal) = (0, 0); + + while trials < TRIALS { + let inst = match instance(&mut rng, N, k, 0) { + Some(inst) => inst, + None => continue, + }; + let optimum = match brute_force(&inst) { + Some(optimum) => optimum, + None => continue, + }; + trials += 1; + + match brute_force_allowing(&inst, &selectable_under_ban(&inst)) { + Some(banned_opt) => ban_gap += (banned_opt.0 - optimum.0) as f64 / optimum.0 as f64, + None => ban_fails += 1, + } + + let mut cs = exact(&inst); + if cs.run_bnb(metric(), MAX_ROUNDS).is_ok() { + if let Some(score) = metric().score(&cs) { + gap += (score.0 - optimum.0) as f64 / optimum.0 as f64; + if score > optimum { + suboptimal += 1; + } + } + } + } + + let t = trials as f64; + std::println!( + "{:>3} {:>9.3}% {:>9.3}% {:>10.1}% {:>11.1}%", + k, + 100.0 * gap / t, + 100.0 * ban_gap / t, + 100.0 * ban_fails as f64 / t, + 100.0 * suboptimal as f64 / t, + ); + } + + std::println!( + "\nk: how many of the {} candidates carry unconfirmed ancestors. ban-fails: share of\n\ + instances where a funding selection exists but the banning policy cannot reach one --\n\ + automatic selection would report insufficient funds.", + N + ); +} + +/// Diagnostic: is `LowestFee::bound` actually a lower bound once candidates carry ancestor bumps? +/// +/// At `k = 1` there is no ancestor sharing, so the local and exact models coincide and branch and +/// bound should find the optimum outright -- `base-gap` is zero. Any gap there has to come from the +/// bound over-estimating and pruning the optimal branch, so check it against an oracle: the exact +/// minimum score over every selection still reachable from each node. +#[test] +#[ignore = "diagnostic; run with --nocapture"] +fn diagnose_bound_admissibility() { + const N: usize = 12; + const TRIALS: usize = 400; + + struct Checked<'a> { + inner: LowestFee, + n: usize, + violations: &'a mut usize, + worst: &'a mut f32, + calls: &'a mut usize, + /// Violations split by which branch of `LowestFee::bound` produced them. + funded_branch: &'a mut usize, + prefix_branch: &'a mut usize, + /// Of the funded-branch violations, how many had a bumped candidate already selected. + with_bumped_selected: &'a mut usize, + /// Violations where the greedy prefix happened to land on the cs.target() exactly, which + /// `LowestFee::bound` short-circuits by returning the prefix's own score. + exact_prefix: &'a mut usize, + } + + impl BnbMetric for Checked<'_> { + fn drain(&mut self, cs: &CoinSelector<'_>) -> crate::Drain { + self.inner.drain(cs) + } + fn score(&mut self, cs: &CoinSelector<'_>) -> Option { + self.inner.score(cs) + } + fn requires_ordering_by_descending_value_pwu(&self) -> bool { + self.inner.requires_ordering_by_descending_value_pwu() + } + fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { + let bound = self.inner.bound(cs)?; + *self.calls += 1; + + // Exact minimum over everything reachable from here, in the same pricing model the + // metric is being run in. + let free = (0..self.n) + .filter(|&i| !cs.is_selected(i) && !cs.banned().contains(i)) + .collect::>(); + let mut best: Option = None; + for sub in 0..(1_u32 << free.len()) { + let mut t = cs.clone(); + for (j, &i) in free.iter().enumerate() { + if sub & (1 << j) != 0 { + t.select(i); + } + } + if let Some(s) = self.inner.score(&t) { + if best.map_or(true, |b| s < b) { + best = Some(s); + } + } + } + + if let Some(oracle) = best { + if bound > oracle { + *self.violations += 1; + if cs.is_funded() { + *self.funded_branch += 1; + } else { + *self.prefix_branch += 1; + } + if (0..self.n).any(|i| cs.is_selected(i) && cs.ancestor_bump_fee_of(i) > 0) { + *self.with_bumped_selected += 1; + } + // Re-walk the greedy prefix to see whether it hit the cs.target() dead on. + if let Some((prefix, _, _)) = + cs.clone().select_iter().find(|(c, _, _)| c.is_funded()) + { + if prefix.excess(crate::Drain::NONE) == 0 { + *self.exact_prefix += 1; + } + } + let over = (bound.0 - oracle.0) / oracle.0; + if over > *self.worst { + *self.worst = over; + } + } + } + Some(bound) + } + } + + // `bumps = false` zeroes every ancestor bump, leaving an ancestor-free problem of the same + // shape. If the bound over-estimates there too, this is not something ancestors introduced. + for (k, bumps) in [(1_usize, false), (1, true), (3, false), (3, true)] { + let mut rng = Rng(0x27D4EB2F165667C5 ^ (k as u64) << 32); + let (mut violations, mut calls, mut worst) = (0usize, 0usize, 0f32); + let (mut funded_branch, mut prefix_branch, mut with_bumped) = (0usize, 0usize, 0usize); + let mut exact_prefix = 0usize; + let mut trials = 0; + + while trials < TRIALS { + let inst = match instance(&mut rng, N, k, 0) { + Some(inst) => inst, + None => continue, + }; + if brute_force(&inst).is_none() { + continue; + } + trials += 1; + + let cs = if bumps { + exact(&inst) + } else { + without_ancestors(&inst) + }; + + let checked = Checked { + inner: metric(), + n: N, + violations: &mut violations, + worst: &mut worst, + calls: &mut calls, + funded_branch: &mut funded_branch, + prefix_branch: &mut prefix_branch, + with_bumped_selected: &mut with_bumped, + exact_prefix: &mut exact_prefix, + }; + let _ = cs.bnb_solutions(checked).take(100_000).count(); + } + + std::println!( + "k={} bumps={:<5}: {:>6} calls, {:>4} over-estimates ({:.2}%), worst {:>6.1}% \ + | funded {:>4}, prefix {:>4}, bumped-selected {:>4}, exact-prefix {:>4}", + k, + bumps, + calls, + violations, + 100.0 * violations as f64 / calls.max(1) as f64, + 100.0 * worst, + funded_branch, + prefix_branch, + with_bumped, + exact_prefix, + ); + } +} diff --git a/src/bnb.rs b/src/bnb.rs index d20db4c..8dc21db 100644 --- a/src/bnb.rs +++ b/src/bnb.rs @@ -66,12 +66,19 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> { } self.insert_new_branches(&selector); - Some(return_val.map(|score| (selector, score))) + // What escapes to the caller is priced exactly: the local model over-reserves, and the + // difference belongs in the change output rather than silently in the fee. + Some(return_val.map(|score| (selector.priced_exactly(), score))) } } impl<'a, M: BnbMetric> BnbIter<'a, M> { - pub(crate) fn new(mut selector: CoinSelector<'a>, metric: M) -> Self { + pub(crate) fn new(selector: CoinSelector<'a>, metric: M) -> Self { + // Search on the local (per-candidate) ancestor bump model. It is additive, so every figure + // a metric sees -- the per-candidate effective values, the excess, the bounds -- is + // consistent and each metric's correctness argument holds as written. Solutions are handed + // back exactly priced; see `Iterator::next`. + let mut selector = selector.priced_locally(); let mut iter = BnbIter { queue: BinaryHeap::default(), best: None, @@ -137,12 +144,14 @@ impl<'a, M: BnbMetric> BnbIter<'a, M> { inclusion_cs.select(next_index); self.consider_adding_to_queue(&inclusion_cs, false); - // for the exclusion branch, we keep banning if candidates have the same weight and value + // For the exclusion branch, keep banning while candidates are interchangeable. The + // ancestor bump is part of what a candidate costs, so two inputs of equal value and weight + // are *not* interchangeable when they drag in different unconfirmed ancestors. let mut is_first_ban = true; let mut exclusion_cs = cs.clone(); - let to_ban = (next.value, next.weight); + let to_ban = (next.value, next.weight, cs.ancestor_bump_fee_of(next_index)); for (next_index, next) in cs.unselected() { - if (next.value, next.weight) != to_ban { + if (next.value, next.weight, cs.ancestor_bump_fee_of(next_index)) != to_ban { break; } let (_index, _candidate) = exclusion_cs diff --git a/src/bump_table.rs b/src/bump_table.rs new file mode 100644 index 0000000..16813b8 --- /dev/null +++ b/src/bump_table.rs @@ -0,0 +1,159 @@ +use crate::{bitset::Bitset, mempool::Cluster, FeeRate}; +use alloc::{collections::BTreeMap, vec::Vec}; + +/// An unconfirmed transaction that may still need bumping, stripped to what pricing needs. +#[derive(Debug, Clone, Copy)] +struct Unit { + weight: u64, + fee: u64, +} + +/// What spending unconfirmed UTXOs costs, at one fixed feerate. +/// +/// Built from a [`Cluster`] — the relevant piece of the mempool as a graph — because pricing CPFP +/// correctly needs to know which transactions a miner would already include. A flat list of +/// ancestors cannot express that, and charges for packages that need no bump at all. +/// +/// This answers two different questions, and the difference between them is the whole design: +/// +/// - `combined` — what a *set* of candidates owes together, via +/// [`CoinSelector::selected_ancestor_bump_fee`]. Exact: an ancestor +/// two candidates share is paid for once. Every figure this crate reports is built on it, and it +/// is what the transaction actually has to pay. +/// - [`individual`](Self::individual) — what one candidate owes on its own. Additive across +/// candidates, so an over-estimate whenever ancestors are shared, but *local*: it is what +/// [`CoinSelector::effective_value_of`] subtracts, so the figures the selection algorithms rank +/// on tell the whole truth. Those algorithms need locality more than they need accuracy. +/// +/// Branch and bound searches on the local figures while the crate reports the combined one — the +/// same split Bitcoin Core draws between `calculateIndividualBumpFees` and +/// `calculateCombinedBumpFee`. Because the combined bump is never larger than the sum of the +/// individual ones, searching on the latter *over*-reserves, so the surplus surfaces as a larger +/// change output rather than going missing from the fee. +/// +/// # Feerate +/// +/// Both figures are computed for one feerate and are meaningless at any other, since the +/// arithmetic (`feerate * weight - fee_paid`) depends on it. Using one against a mismatched +/// [`Target::fee`] under-prices the package, so [`CoinSelector::selected_ancestor_bump_fee`] +/// checks that they agree. +/// +/// [`CoinSelector::effective_value_of`]: crate::CoinSelector::effective_value_of +/// [`CoinSelector::selected_ancestor_bump_fee`]: crate::CoinSelector::selected_ancestor_bump_fee +/// [`Target::fee`]: crate::Target::fee +#[derive(Debug, Clone)] +pub struct BumpTable { + feerate: FeeRate, + /// The unconfirmed transactions that may still need bumping: for a cluster, the ones a miner + /// would leave behind; for a flat ancestor list, all of them. + units: Vec, + /// Candidate index -> the units its selection drags in, and what it owes alone. + /// + /// Sparse — only candidates carrying ancestors — so this stays proportional to the number of + /// unconfirmed UTXOs rather than to the size of the candidate set. Ascending iteration comes + /// from the map rather than from construction discipline. + entries: BTreeMap, +} + +impl BumpTable { + /// Build by mining a mock block over `cluster` and charging only for what it leaves behind. + /// + /// Transactions a miner would already include at `feerate` are paying their own way and cost + /// nothing, so a parent already above the target — or one already carried by an overpaying + /// child in the cluster — correctly contributes zero. Neither is visible to a model that only + /// knows a flat list of ancestors. + /// + /// The template is built once: which transactions a miner includes depends on the cluster and + /// the feerate, not on which outputs you happen to be asking about. + pub fn from_cluster(cluster: &Cluster, feerate: FeeRate) -> Self { + let mined = cluster.mine(feerate); + + // Renumber the survivors so units are dense and the mined transactions simply do not exist. + let mut unit_of_tx = alloc::vec![usize::MAX; cluster.n_txs()]; + let mut units = Vec::new(); + for (tx, unit) in unit_of_tx.iter_mut().enumerate() { + if !mined.contains(tx) { + *unit = units.len(); + units.push(Unit { + weight: cluster.tx(tx).weight, + fee: cluster.tx(tx).fee, + }); + } + } + + let mut entries = BTreeMap::new(); + for candidate in cluster.candidates() { + let mut drags_in = Bitset::with_capacity(units.len()); + for closure in cluster.package_of(candidate) { + for tx in closure.iter() { + if !mined.contains(tx) { + drags_in.insert(unit_of_tx[tx]); + } + } + } + let individual = deficit(&drags_in, &units, feerate); + entries.insert(candidate, (drags_in, individual)); + } + + Self { + feerate, + units, + entries, + } + } + + /// The feerate these figures were computed for. They are meaningless at any other. + pub fn feerate(&self) -> FeeRate { + self.feerate + } + + /// The candidates that carry unconfirmed ancestors, ascending. + pub fn candidates(&self) -> impl Iterator + '_ { + self.entries.keys().copied() + } + + /// What `candidate` owes on its own, ignoring whatever else might be selected. + /// + /// Zero for a candidate with no unconfirmed ancestors, or one whose ancestors a miner would + /// already include. Summing these across a selection over-estimates the combined package + /// figure whenever ancestors are shared — and that over-estimate is the price of the figure + /// being additive, which is what the selection algorithms need. + pub fn individual(&self, candidate: usize) -> u64 { + self.entries.get(&candidate).map_or(0, |&(_, bump)| bump) + } + + /// Every candidate's individual bump, ascending by candidate index. + pub fn individual_bumps(&self) -> impl Iterator + '_ { + self.entries.iter().map(|(&c, &(_, bump))| (c, bump)) + } + + /// What the candidates in `selected` owe *together*, with shared ancestors counted once. + /// + /// This is what the transaction actually has to pay. + pub(crate) fn combined(&self, selected: &Bitset) -> u64 { + let mut units = Bitset::with_capacity(self.units.len()); + for (&candidate, (drags_in, _)) in &self.entries { + if selected.contains(candidate) { + for unit in drags_in.iter() { + units.insert(unit); + } + } + } + deficit(&units, &self.units, self.feerate) + } + + /// The largest candidate index this table refers to, or `None` if it refers to none. + pub(crate) fn max_candidate_index(&self) -> Option { + self.entries.keys().next_back().copied() + } +} + +/// What a set of unconfirmed transactions still owes at `feerate`, as a package. +fn deficit(set: &Bitset, units: &[Unit], feerate: FeeRate) -> u64 { + let (mut weight, mut fee) = (0_u64, 0_u64); + for i in set.iter() { + weight += units[i].weight; + fee += units[i].fee; + } + feerate.implied_fee(weight).saturating_sub(fee) +} diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 1a75724..98f07f2 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -1,13 +1,32 @@ use super::*; #[allow(unused)] // some bug in <= 1.48.0 sees this as unused when it isn't use crate::float::FloatExt; -use crate::{bitset::Bitset, bnb::BnbMetric, float::Ordf32, ChangePolicy, FeeRate, Target}; +use crate::{ + bitset::Bitset, bnb::BnbMetric, bump_table::BumpTable, float::Ordf32, ChangePolicy, FeeRate, + Target, +}; use alloc::{sync::Arc, vec::Vec}; /// The minimum change amount Bitcoin Core's `SelectCoinsSRD` targets; a sensible default for the /// `change_lower` argument of [`CoinSelector::select_srd`]. pub const CHANGE_LOWER: u64 = 50_000; +/// Which ancestor-bump model a [`CoinSelector`] answers with. +/// +/// The CPFP bump is a property of the *package*: an ancestor two candidates share is paid for +/// once. That makes it non-additive, and non-additive figures break the selection algorithms, +/// which rank and accumulate per candidate. So there are two models, and a selector commits to one +/// at a time rather than mixing them — a bound computed against one model and scored against the +/// other is not a bound. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Pricing { + /// The package figure: exact, non-additive. What every public method reports. + Exact, + /// The sum of per-candidate figures: additive, and never below the exact one. What branch and + /// bound searches on. + Local, +} + /// [`CoinSelector`] selects/deselects coins from a set of canididate coins. /// /// You can manually select coins using methods like [`select`], or automatically with methods such @@ -22,6 +41,10 @@ pub struct CoinSelector<'a> { /// built for one target and evaluated against it throughout, and threading it through every /// method made it possible to ask two different questions of the same selection. target: Target, + /// Exact CPFP pricing (via [`CoinSelector::with_bump_table`]). + bump_table: Option<&'a BumpTable>, + /// Which of the two ancestor-bump models this selector answers with. See [`Pricing`]. + pricing: Pricing, selected: Bitset, banned: Bitset, candidate_order: Arc>, @@ -47,6 +70,8 @@ impl<'a> CoinSelector<'a> { Self { candidates, target, + bump_table: None, + pricing: Pricing::Exact, selected: Bitset::with_capacity(candidates.len()), banned: Bitset::with_capacity(candidates.len()), candidate_order: Arc::new((0..candidates.len()).collect::>()), @@ -58,6 +83,71 @@ impl<'a> CoinSelector<'a> { self.target } + /// Report CPFP package costs exactly, using `bump_table`. + /// + /// Nothing is banned: candidates with unconfirmed ancestors are selected like any other. That + /// works because the *search* reasons about [`effective_value_of`], which nets off each + /// candidate's individual bump and is therefore additive, while everything this selector reports — + /// [`excess`], [`implied_fee`], [`is_funded`], [`drain`] — uses the table's exact combined + /// figure, in which an ancestor shared by two candidates is paid for once. + /// + /// The two differ, and deliberately: the sum of per-candidate bumps is never below the + /// combined one, so the search *over*-reserves and the surplus surfaces as a larger change + /// output. See [`BumpTable`]. + /// + /// # Panics + /// + /// If the table refers to a candidate index out of bounds for the slice passed to + /// [`CoinSelector::new`]. + /// + /// [`effective_value_of`]: Self::effective_value_of + /// [`excess`]: Self::excess + /// [`implied_fee`]: Self::implied_fee + /// [`is_funded`]: Self::is_funded + /// [`drain`]: Self::drain + pub fn with_bump_table(mut self, bump_table: &'a BumpTable) -> Self { + if let Some(max) = bump_table.max_candidate_index() { + assert!( + max < self.candidates.len(), + "bump table refers to candidate index {} but there are only {} candidates", + max, + self.candidates.len() + ); + } + self.bump_table = Some(bump_table); + self + } + + /// The same selector, answering with the local (per-candidate) bump model. + /// + /// Branch and bound searches in this mode so that every figure a metric sees is additive + /// across candidates, which is what its ranking and its bounds assume. Selections handed back + /// to the caller are returned to [`Pricing::Exact`]. + pub(crate) fn priced_locally(&self) -> Self { + let mut cs = self.clone(); + cs.pricing = Pricing::Local; + cs + } + + /// The same selector, answering with the exact (package) bump model. + pub(crate) fn priced_exactly(&self) -> Self { + let mut cs = self.clone(); + cs.pricing = Pricing::Exact; + cs + } + + /// The candidates that have unconfirmed ancestors, by index into the original `candidates` + /// slice passed to [`CoinSelector::new`]. + /// + /// These are selectable like any other candidate; the [ancestor bump fee] is priced into every + /// excess calculation when they are chosen. This is a query about *pricing*, not about + /// selectability. + /// + /// [ancestor bump fee]: Self::selected_ancestor_bump_fee + pub fn candidates_with_ancestors(&self) -> impl Iterator + '_ { + self.bump_table.into_iter().flat_map(|t| t.candidates()) + } + /// Iterate over all the candidates in their currently sorted order. Each item has the original /// index with the candidate. pub fn candidates( @@ -196,6 +286,84 @@ impl<'a> CoinSelector<'a> { + target_ouputs.output_weight_with_drain(drain_weight) } + /// The extra fee this selection owes on top of its own weight, so that its unconfirmed + /// ancestors reach `feerate` as a package (CPFP). + /// + /// Which of the two models answers depends on how this selector is priced. Everything a + /// caller can reach uses the **exact** figure — the [`BumpTable`]'s combined bump, in which an + /// ancestor shared by two selected candidates is paid for once, and which is what the + /// transaction actually owes. + /// + /// Branch and bound switches internally to the **local** figure — the sum of the selected + /// candidates' [`ancestor_bump_fee_of`] — additive, and therefore never below the exact one, + /// so its ranking and bounds can rely on it. + /// + /// Zero when neither a table nor per-candidate bumps were supplied. + /// + /// # Panics + /// + /// If `feerate` is not the one the table was built for. A table's figures are only valid at + /// its own feerate, and using them at another under-prices the package — in *both* directions, + /// so there is no safe side to land on. The check is one comparison and only runs when a table + /// is present, so it is not gated on debug builds. + /// + /// [`ancestor_bump_fee_of`]: Self::ancestor_bump_fee_of + pub fn selected_ancestor_bump_fee(&self, feerate: FeeRate) -> u64 { + let bump_table = match self.bump_table { + Some(bump_table) => bump_table, + None => return 0, + }; + assert_eq!( + feerate, + bump_table.feerate(), + "bump table was built for a different feerate; its figures do not apply here" + ); + match self.pricing { + Pricing::Local => self.selected.iter().map(|i| bump_table.individual(i)).sum(), + Pricing::Exact => bump_table.combined(&self.selected), + } + } + + /// What selecting the candidate at `index` alone would owe to bring its unconfirmed ancestors + /// up to the target feerate, in satoshis. Zero without a [`BumpTable`], or for a candidate + /// with no unconfirmed ancestors. + /// + /// Additive across candidates, and never in total below what the package actually owes -- see + /// [`BumpTable`] for why that direction matters. + pub fn ancestor_bump_fee_of(&self, index: usize) -> u64 { + self.bump_table.map_or(0, |t| t.individual(index)) + } + + /// [`Candidate::effective_value`] less what that candidate's unconfirmed ancestors cost. + /// + /// This is the figure to rank candidates on. `Candidate` cannot compute it: a bump is only + /// meaningful at the feerate it was derived for, and a `Candidate` has nowhere to record + /// which -- so it lives here, where the [`BumpTable`] is, and the feerate can be checked. + /// + /// # Panics + /// + /// If `feerate` is not the one the attached table was built for. + pub fn effective_value_of(&self, index: usize, feerate: FeeRate) -> f32 { + if let Some(bump_table) = self.bump_table { + assert_eq!( + feerate, + bump_table.feerate(), + "bump table was built for a different feerate; its figures do not apply here" + ); + } + self.candidates[index].effective_value(feerate) - self.ancestor_bump_fee_of(index) as f32 + } + + /// [`Candidate::value_pwu`] less what that candidate's unconfirmed ancestors cost, spread over + /// its weight. Needs no feerate of its own: the attached table fixes one. + pub fn value_pwu_of(&self, index: usize) -> f32 { + let candidate = self.candidates[index]; + candidate + .value + .saturating_sub(self.ancestor_bump_fee_of(index)) as f32 + / candidate.weight as f32 + } + /// How much the current selection overshoots the value needed to achieve `target`. /// /// In order for the resulting transaction to be valid this must be 0 or above. If it's above 0 @@ -222,7 +390,7 @@ impl<'a> CoinSelector<'a> { self.selected_value() as i64 - self.target.value() as i64 - drain.value as i64 - - self.implied_fee_from_feerate(drain.weights) as i64 + - self.implied_package_fee_from_feerate(drain.weights) as i64 } /// Same as [rate_excess](Self::rate_excess) except `self.target.fee.rate` is applied to the @@ -231,7 +399,7 @@ impl<'a> CoinSelector<'a> { self.selected_value() as i64 - self.target.value() as i64 - drain.value as i64 - - self.implied_fee_from_feerate_wu(drain.weights) as i64 + - self.implied_package_fee_from_feerate_wu(drain.weights) as i64 } /// How much the current selection overshoots the value needed to satisfy `self.target.fee.absolute` @@ -245,29 +413,19 @@ impl<'a> CoinSelector<'a> { /// How much the current selection overshoots the value needed to satisfy RBF's rule 4. pub fn replacement_excess(&self, drain: Drain) -> i64 { - let mut replacement_excess_needed = 0; - if let Some(replace) = self.target.fee.replace { - replacement_excess_needed = - replace.min_fee_to_do_replacement(self.weight(self.target.outputs, drain.weights)) - } self.selected_value() as i64 - self.target.value() as i64 - drain.value as i64 - - replacement_excess_needed as i64 + - self.implied_package_fee_from_replacement(drain.weights) as i64 } /// Same as [replacement_excess](Self::replacement_excess) except the replacement fee /// is calculated using weight units directly without any conversion to vbytes. pub fn replacement_excess_wu(&self, drain: Drain) -> i64 { - let mut replacement_excess_needed = 0; - if let Some(replace) = self.target.fee.replace { - replacement_excess_needed = replace - .min_fee_to_do_replacement_wu(self.weight(self.target.outputs, drain.weights)) - } self.selected_value() as i64 - self.target.value() as i64 - drain.value as i64 - - replacement_excess_needed as i64 + - self.implied_package_fee_from_replacement_wu(drain.weights) as i64 } /// The feerate the transaction would have if we were to use this selection of inputs to achieve @@ -286,37 +444,71 @@ impl<'a> CoinSelector<'a> { /// The fee the current selection and `drain_weight` should pay to satisfy `target_fee`. /// - /// This compares the fee calculated from the target feerate with the fee calculated from the - /// [`Replace`] constraints and returns the larger of the two. + /// This is the largest of the fees implied by `self.target.fee.rate`, `self.target.fee.absolute` and the + /// [`Replace`] constraints. The feerate and replacement fees include any [ancestor bump fee]; + /// `self.target.fee.absolute` is a minimum fee floor rather than an additive charge, so it does not. + /// + /// This is the exact counterpart of [`excess`](Self::excess): + /// `excess == selected_value - target.value() - drain.value - implied_fee`. /// /// `drain_weight` can be 0 to indicate no draining output. + /// + /// [ancestor bump fee]: Self::selected_ancestor_bump_fee pub fn implied_fee(&self, drain_weights: DrainWeights) -> u64 { - let mut implied_fee = self - .implied_fee_from_feerate(drain_weights) - .max(self.target.fee.absolute); - - if let Some(replace) = self.target.fee.replace { - implied_fee = Ord::max( - implied_fee, - replace.min_fee_to_do_replacement(self.weight(self.target.outputs, drain_weights)), - ); - } - - implied_fee + self.implied_package_fee_from_feerate(drain_weights) + .max(self.target.fee.absolute) + .max(self.implied_package_fee_from_replacement(drain_weights)) } - fn implied_fee_from_feerate(&self, drain_weights: DrainWeights) -> u64 { + /// The fee implied by `self.target.fee.rate` for the whole CPFP package — this transaction plus any + /// unconfirmed ancestors — i.e. the fee for the transaction's own weight plus the [ancestor + /// bump fee]. + /// + /// The bump is folded in here rather than at each call site because every caller needs it. + /// + /// [ancestor bump fee]: Self::selected_ancestor_bump_fee + fn implied_package_fee_from_feerate(&self, drain_weights: DrainWeights) -> u64 { self.target .fee .rate .implied_fee(self.weight(self.target.outputs, drain_weights)) + + self.selected_ancestor_bump_fee(self.target.fee.rate) } - fn implied_fee_from_feerate_wu(&self, drain_weights: DrainWeights) -> u64 { + /// Same as [`implied_package_fee_from_feerate`](Self::implied_package_fee_from_feerate) except `self.target.fee.rate` + /// is applied to weight units directly without any conversion to vbytes. + fn implied_package_fee_from_feerate_wu(&self, drain_weights: DrainWeights) -> u64 { self.target .fee .rate .implied_fee_wu(self.weight(self.target.outputs, drain_weights)) + + self.selected_ancestor_bump_fee(self.target.fee.rate) + } + + /// The fee needed for the whole CPFP package to satisfy RBF's rule 4, i.e. the replacement fee + /// plus the [ancestor bump fee]. No replacement (`self.target.fee.replace` is `None`) still leaves + /// the bump to pay. + /// + /// [ancestor bump fee]: Self::selected_ancestor_bump_fee + fn implied_package_fee_from_replacement(&self, drain_weights: DrainWeights) -> u64 { + let replacement_fee = match self.target.fee.replace { + Some(replace) => { + replace.min_fee_to_do_replacement(self.weight(self.target.outputs, drain_weights)) + } + None => 0, + }; + replacement_fee + self.selected_ancestor_bump_fee(self.target.fee.rate) + } + + /// Same as [`implied_package_fee_from_replacement`](Self::implied_package_fee_from_replacement) except the + /// replacement fee is calculated using weight units directly without any conversion to vbytes. + fn implied_package_fee_from_replacement_wu(&self, drain_weights: DrainWeights) -> u64 { + let replacement_fee = match self.target.fee.replace { + Some(replace) => replace + .min_fee_to_do_replacement_wu(self.weight(self.target.outputs, drain_weights)), + None => 0, + }; + replacement_fee + self.selected_ancestor_bump_fee(self.target.fee.rate) } /// The actual fee the selection would pay if it was used in a transaction that had @@ -328,8 +520,11 @@ impl<'a> CoinSelector<'a> { } /// The value of the current selected inputs minus the fee needed to pay for the selected inputs + /// and any ancestor bump fee. pub fn effective_value(&self, feerate: FeeRate) -> i64 { - self.selected_value() as i64 - (self.input_weight() as f32 * feerate.spwu()).ceil() as i64 + self.selected_value() as i64 + - (self.input_weight() as f32 * feerate.spwu()).ceil() as i64 + - self.selected_ancestor_bump_fee(feerate) as i64 } // /// Waste sum of all selected inputs. @@ -374,9 +569,10 @@ impl<'a> CoinSelector<'a> { /// Sorts the candidates by descending value per weight unit, tie-breaking with value. pub fn sort_candidates_by_descending_value_pwu(&mut self) { - self.sort_candidates_by_key(|(_, wv)| { - core::cmp::Reverse((Ordf32(wv.value_pwu()), wv.value)) - }); + let pwu = (0..self.candidates.len()) + .map(|i| Ordf32(self.value_pwu_of(i))) + .collect::>(); + self.sort_candidates_by_key(|(i, wv)| core::cmp::Reverse((pwu[i], wv.value))); } /// Shuffle the candidates with Fisher-Yates algorithm. @@ -498,6 +694,7 @@ impl<'a> CoinSelector<'a> { /// [`is_within_max_weight`]. See [`is_funded_with_drain`] for the version that /// accounts for a specific `drain`. /// + /// [`effective_value_of`]: Self::effective_value_of /// [`excess`]: Self::excess /// [`is_within_max_weight`]: Self::is_within_max_weight /// [`is_funded_with_drain`]: Self::is_funded_with_drain @@ -563,15 +760,24 @@ impl<'a> CoinSelector<'a> { /// /// A candidate if effective if it provides more value than it takes to pay for at `feerate`. pub fn select_all_effective(&mut self, feerate: FeeRate) { + // `Candidate::effective_value` subtracts a bump computed for the table's feerate, so + // ranking at any other one is meaningless. See `Candidate::ancestor_bump_fee`. + if let Some(bump_table) = self.bump_table { + assert_eq!( + feerate, + bump_table.feerate(), + "bump table was built for a different feerate; its figures do not apply here" + ); + } for i in 0..self.candidate_order.len() { let cand_index = self.candidate_order[i]; if self.selected.contains(cand_index) || self.banned.contains(cand_index) - || self.candidates[cand_index].effective_value(feerate) <= 0.0 + || self.effective_value_of(cand_index, feerate) <= 0.0 { continue; } - self.selected.insert(cand_index); + self.select(cand_index); } } @@ -712,6 +918,8 @@ impl<'a> CoinSelector<'a> { .flatten() .last(); if let Some((selector, score)) = best { + // `selector` is already exactly priced (see `BnbIter::next`), so the drain the caller + // gets is sized by the true package cost rather than by the search's over-estimate. let drain = iter.metric.drain(&selector); *self = selector; return Ok((score, drain)); @@ -922,11 +1130,17 @@ impl Candidate { } /// Effective value of this input candidate: `actual_value - input_weight * feerate (sats/wu)`. + /// + /// Note this knows nothing about unconfirmed ancestors. Where candidates may have them, rank + /// on [`CoinSelector::effective_value_of`] instead, which nets off what the CPFP package costs. pub fn effective_value(&self, feerate: FeeRate) -> f32 { self.value as f32 - (self.weight as f32 * feerate.spwu()) } - /// Value per weight unit + /// Value per weight unit. + /// + /// As with [`effective_value`](Self::effective_value), this ignores unconfirmed ancestors; see + /// [`CoinSelector::value_pwu_of`]. pub fn value_pwu(&self) -> f32 { self.value as f32 / self.weight as f32 } diff --git a/src/lib.rs b/src/lib.rs index 34c86ad..f63bf68 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,12 @@ extern crate std; mod bitset; pub use bitset::*; +mod bump_table; +pub use bump_table::*; +mod mempool; +pub use mempool::*; +#[cfg(test)] +mod ancestor_search_experiment; mod coin_selector; pub mod float; pub use coin_selector::*; diff --git a/src/mempool.rs b/src/mempool.rs new file mode 100644 index 0000000..a536d1b --- /dev/null +++ b/src/mempool.rs @@ -0,0 +1,300 @@ +use crate::{bitset::Bitset, FeeRate}; +use alloc::vec::Vec; + +/// An unconfirmed transaction in a [`Cluster`]. +#[derive(Debug, Clone)] +pub struct MempoolTx { + /// Weight in weight units. + pub weight: u64, + /// Fee paid, in satoshis. + pub fee: u64, + /// Indices into [`Cluster`]'s transaction list of this transaction's *direct* parents. + /// + /// Only *direct* parents: the transitive closure is computed for you, which is much of the + /// point of supplying a graph rather than a list. + pub parents: Vec, +} + +/// A connected piece of the mempool: the unconfirmed transactions relevant to a selection, and +/// which candidates spend from which. +/// +/// This is the input to [`BumpTable`]. Supplying the graph rather than a flat ancestor list buys +/// three things a list cannot express: +/// +/// - **Transitive closure is computed here**, so a candidate cannot be under-priced by a caller +/// listing only its direct parent. +/// - **Package feerates are visible**, so a transaction a miner would already include costs +/// nothing, and an overpaying child that carries a deficient parent means neither is charged +/// for. A flat list has to charge for everything it is given. +/// - **The bump stays safe to search on.** Branch and bound ranks candidates on their +/// [individual] bumps, whose sum must never fall below what the package owes together. Mining +/// guarantees that; pooling a flat list does not, because a shared ancestor that *overpays* has +/// its surplus counted once per dependent. +/// +/// If all you have is a flat list of ancestors, express it here: each ancestor becomes a +/// transaction with no parents, and each (ancestor, candidate) pair becomes an entry in +/// `candidate_spends`. You then get the mining step for free. +/// +/// [individual]: crate::BumpTable::individual +/// +/// # Completeness +/// +/// Include the descendants and siblings of your ancestors where you know them — a parent already +/// being paid for by *another* child needs no bump, and only a transaction present in the cluster +/// can demonstrate that. Where you don't know them (a child belonging to someone else), the +/// package is priced as if it needs the bump: you overpay, the transaction still confirms. That is +/// the safe direction, and it is the reason this is an optimality limit rather than a correctness +/// one. +/// +/// [`BumpTable`]: crate::BumpTable +#[derive(Debug, Clone)] +pub struct Cluster { + txs: Vec, + /// Candidate index -> index of the transaction whose output it spends. + candidate_spends: Vec<(usize, usize)>, + /// Per transaction, itself plus every transitive ancestor. + closures: Vec, +} + +/// Error returned when a [`Cluster`] cannot be built. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ClusterError { + /// A `parents` entry does not refer to a transaction in the cluster. + ParentOutOfBounds { + /// The transaction naming the bad parent. + tx: usize, + /// The out-of-bounds parent index. + parent: usize, + }, + /// A `candidate_spends` entry does not refer to a transaction in the cluster. + SpendOutOfBounds { + /// The candidate index. + candidate: usize, + /// The out-of-bounds transaction index. + tx: usize, + }, + /// The parent relation contains a cycle, so the transactions cannot all be ancestors of each + /// other. Real mempool clusters are acyclic. + Cycle { + /// A transaction on the cycle. + tx: usize, + }, +} + +impl core::fmt::Display for ClusterError { + fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { + match self { + ClusterError::ParentOutOfBounds { tx, parent } => write!( + f, + "transaction {} names parent {}, which is not in the cluster", + tx, parent + ), + ClusterError::SpendOutOfBounds { candidate, tx } => write!( + f, + "candidate {} spends transaction {}, which is not in the cluster", + candidate, tx + ), + ClusterError::Cycle { tx } => { + write!(f, "the parent relation cycles through transaction {}", tx) + } + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for ClusterError {} + +impl Cluster { + /// Build a cluster from unconfirmed transactions and the candidates that spend them. + /// + /// `candidate_spends` pairs a candidate index (into the slice given to + /// [`CoinSelector::new`]) with the index of the transaction in `txs` whose output it spends. + /// Both directions are many: a transaction may be spent by several candidates, and a candidate + /// may appear more than once when it spends outputs of several transactions — its package is + /// then the union of their ancestor closures. + /// + /// # Errors + /// + /// [`ClusterError`] if an index is out of bounds or the parent relation cycles. + /// + /// [`CoinSelector::new`]: crate::CoinSelector::new + pub fn new( + txs: Vec, + candidate_spends: Vec<(usize, usize)>, + ) -> Result { + for (tx_index, tx) in txs.iter().enumerate() { + for &parent in &tx.parents { + if parent >= txs.len() { + return Err(ClusterError::ParentOutOfBounds { + tx: tx_index, + parent, + }); + } + } + } + for &(candidate, tx) in &candidate_spends { + if tx >= txs.len() { + return Err(ClusterError::SpendOutOfBounds { candidate, tx }); + } + } + + let closures = ancestor_closures(&txs)?; + Ok(Self { + txs, + candidate_spends, + closures, + }) + } + + /// The candidates that spend from this cluster, ascending and deduplicated. + pub fn candidates(&self) -> Vec { + let mut out = self + .candidate_spends + .iter() + .map(|&(candidate, _)| candidate) + .collect::>(); + out.sort_unstable(); + out.dedup(); + out + } + + /// Every transaction that selecting `candidate` pulls into the package: the one it spends, + /// plus all of that transaction's ancestors. + pub(crate) fn package_of(&self, candidate: usize) -> impl Iterator + '_ { + self.candidate_spends + .iter() + .filter(move |&&(c, _)| c == candidate) + .map(move |&(_, tx)| &self.closures[tx]) + } + + pub(crate) fn n_txs(&self) -> usize { + self.txs.len() + } + + pub(crate) fn tx(&self, index: usize) -> &MempoolTx { + &self.txs[index] + } + + /// Build a mock block template at `feerate` and return the transactions it includes. + /// + /// This is the same greedy shape Bitcoin Core's `MiniMiner` uses: repeatedly take the + /// remaining ancestor package with the highest package feerate, and stop once even the best + /// package pays below `feerate`. Anything mined is already paying its own way and so needs no + /// bump; what remains is what a CPFP child has to cover. + /// + /// Note this is deliberately *package*-wise rather than transaction-wise. A transaction paying + /// below `feerate` on its own is still mined when a descendant in the cluster carries it, + /// which is exactly the case a per-ancestor rule gets wrong. + pub(crate) fn mine(&self, feerate: FeeRate) -> Bitset { + let n = self.txs.len(); + let mut mined = Bitset::with_capacity(n); + let mut n_mined = 0; + + while n_mined < n { + // The remaining ancestor package with the highest feerate, compared as a ratio so no + // rounding to vbytes creeps into the ordering. + let mut best: Option<(usize, u64, u64)> = None; + for tx_index in 0..n { + if mined.contains(tx_index) { + continue; + } + let (weight, fee) = self.remaining_package(tx_index, &mined); + let better = match best { + None => true, + // fee/weight > best_fee/best_weight, cross-multiplied. u128 because a + // fee x weight product overflows u64 at realistic extremes. + Some((_, best_weight, best_fee)) => { + (fee as u128) * (best_weight as u128) + > (best_fee as u128) * (weight as u128) + } + }; + if better { + best = Some((tx_index, weight, fee)); + } + } + + let (tx_index, weight, fee) = match best { + Some(best) => best, + None => break, + }; + // Once the best remaining package pays below the target, so does every other, and a + // miner would stop here. + if fee < feerate.implied_fee_wu(weight) { + break; + } + for ancestor in self.closures[tx_index].iter() { + if mined.insert(ancestor) { + n_mined += 1; + } + } + } + + mined + } + + /// Total weight and fee of `tx`'s ancestor package, skipping anything already mined. + fn remaining_package(&self, tx: usize, mined: &Bitset) -> (u64, u64) { + let mut weight = 0; + let mut fee = 0; + for ancestor in self.closures[tx].iter() { + if !mined.contains(ancestor) { + weight += self.txs[ancestor].weight; + fee += self.txs[ancestor].fee; + } + } + (weight, fee) + } +} + +/// For each transaction, the set containing it and all its transitive ancestors. +fn ancestor_closures(txs: &[MempoolTx]) -> Result, ClusterError> { + let n = txs.len(); + let mut closures = Vec::with_capacity(n); + for _ in 0..n { + closures.push(Bitset::with_capacity(n)); + } + + // Iterative post-order DFS: a transaction's closure is itself plus the union of its parents'. + // `done` marks a finished closure, `on_stack` catches a cycle. + let mut done = Bitset::with_capacity(n); + let mut on_stack = Bitset::with_capacity(n); + let mut stack: Vec<(usize, usize)> = Vec::new(); + + for root in 0..n { + if done.contains(root) { + continue; + } + stack.push((root, 0)); + on_stack.insert(root); + + while let Some(&mut (tx, ref mut next_parent)) = stack.last_mut() { + if *next_parent < txs[tx].parents.len() { + let parent = txs[tx].parents[*next_parent]; + *next_parent += 1; + if on_stack.contains(parent) { + return Err(ClusterError::Cycle { tx: parent }); + } + if !done.contains(parent) { + stack.push((parent, 0)); + on_stack.insert(parent); + } + continue; + } + + // Every parent is finished, so this closure can be completed. + let mut closure = Bitset::with_capacity(n); + closure.insert(tx); + for &parent in &txs[tx].parents { + for ancestor in closures[parent].iter() { + closure.insert(ancestor); + } + } + closures[tx] = closure; + done.insert(tx); + on_stack.remove(tx); + stack.pop(); + } + } + + Ok(closures) +} diff --git a/src/metrics/changeless.rs b/src/metrics/changeless.rs index c2c9036..489fc4f 100644 --- a/src/metrics/changeless.rs +++ b/src/metrics/changeless.rs @@ -25,6 +25,10 @@ impl Changeless { /// NOTE: this relies on candidates being sorted so that all negative effective value candidates /// are next to each other, which [`requires_ordering_by_descending_value_pwu`] guarantees. /// + /// NOTE: it also needs the per-candidate figure to tell the whole story about how much a + /// candidate lowers the excess, which is why this uses `CoinSelector::effective_value_of` + /// rather than `Candidate::effective_value` -- the latter cannot see unconfirmed ancestors. + /// /// [`requires_ordering_by_descending_value_pwu`]: BnbMetric::requires_ordering_by_descending_value_pwu fn change_unavoidable(&mut self, cs: &CoinSelector<'_>) -> bool { if self.0.drain(cs).is_none() { @@ -34,7 +38,7 @@ impl Changeless { let mut least_excess = cs.clone(); cs.unselected() .rev() - .take_while(|(_, wv)| wv.effective_value(cs.target().fee.rate) < 0.0) + .take_while(|&(index, _)| cs.effective_value_of(index, cs.target().fee.rate) < 0.0) .for_each(|(index, _)| { least_excess.select(index); }); diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index d990f71..6344e25 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -140,6 +140,10 @@ impl BnbMetric for LowestFee { // `drain_value`, where `change_value` is `excess_with_drain_weight` and `spend_fee` is // `drain_spend_cost`). With `v >= 0` the difference is strictly positive: B always // costs more. + // + // NOTE: the ancestor bump fee cancels between A and B because branch and bound + // searches on the per-candidate model, in which B's bump is A's plus the extra + // input's. See `Pricing` in `coin_selector.rs`. if self.drain_value(cs).is_none() { // But a descendant might *add* a change output that improves the metric. This // happens when the current selection is changeless only because the change would be @@ -173,7 +177,16 @@ impl BnbMetric for LowestFee { Some(current_score) } else { + // The bumps the node itself has already committed to. Every descendant pays these, so + // they belong in the deficit below; the ones the greedy prefix adds on top do not, + // because a descendant may simply not select those candidates. + let committed_bump = cs.selected_ancestor_bump_fee(cs.target().fee.rate); + // Step 1: select everything up until the input that hits the cs.target(). + // + // NOTE: this prices a greedy *prefix* that descendants need not select. That is a + // lower bound only because the ancestor bump fee is additive here -- the prefix can + // charge for candidates a descendant skips, never the reverse. See `Pricing`. let (mut cs, resize_index, to_resize) = cs.clone().select_iter().find(|(cs, _, _)| cs.is_funded())?; @@ -183,6 +196,14 @@ impl BnbMetric for LowestFee { }; cs.deselect(resize_index); + // A bump is a fixed cost, not a rate, so scaling a hypothetical input cannot stand in + // for it -- and a descendant that skips the candidate skips the cost outright. Charge + // only what this node has already committed to, or the deficit is overstated and the + // bound stops being a lower bound. + let uncommitted_bump = cs + .selected_ancestor_bump_fee(cs.target().fee.rate) + .saturating_sub(committed_bump) as f32; + // We need to find the minimum fee we'd pay if we satisfy the feerate constraint. We do // this by imagining we had a perfect input that perfectly hit the cs.target(). The sats per // weight unit of this perfect input is that of `to_resize` but we'll do a scaled @@ -200,11 +221,16 @@ impl BnbMetric for LowestFee { // // In the perfect scenario, no additional fee would be required to pay for rounding up when converting from weight units to // vbytes and so all fee calculations below are performed on weight units directly. - let rate_excess = cs.rate_excess_wu(Drain::NONE) as f32; + let rate_excess = cs.rate_excess_wu(Drain::NONE) as f32 + uncommitted_bump; let mut scale = Ordf32(0.0); if rate_excess < 0.0 { let remaining_value_to_reach_feerate = rate_excess.abs(); + // Deliberately `Candidate::effective_value` rather than the selector's: this + // prices a *hypothetical* input scaled to fit perfectly, and scaling an input does + // not scale the unconfirmed ancestors it drags in. A fixed cost in the denominator + // would inflate `scale`, which `ideal_fee` below multiplies by the raw value -- so + // the bound would exceed a real descendant's score and prune the optimum. let effective_value_of_resized_input = to_resize.effective_value(cs.target().fee.rate); if effective_value_of_resized_input > 0.0 { @@ -219,7 +245,8 @@ impl BnbMetric for LowestFee { // We can use the same approach for replacement we just have to use the // incremental_relay_feerate. if let Some(replace) = cs.target().fee.replace { - let replace_excess = cs.replacement_excess_wu(Drain::NONE) as f32; + let replace_excess = + cs.replacement_excess_wu(Drain::NONE) as f32 + uncommitted_bump; if replace_excess < 0.0 { let remaining_value_to_reach_feerate = replace_excess.abs(); let effective_value_of_resized_input = diff --git a/tests/ancestor_aware.rs b/tests/ancestor_aware.rs new file mode 100644 index 0000000..e5519a9 --- /dev/null +++ b/tests/ancestor_aware.rs @@ -0,0 +1,359 @@ +use bdk_coin_select::{ + BumpTable, Candidate, Cluster, CoinSelector, Drain, DrainWeights, FeeRate, MempoolTx, Replace, + Target, TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, +}; + +fn simple_target(feerate: f32) -> Target { + Target { + outputs: TargetOutputs { + value_sum: 100_000, + weight_sum: 200, + n_outputs: 1, + }, + fee: TargetFee::from_feerate(FeeRate::from_sat_per_vb(feerate)), + max_weight: None, + } +} + +fn tx(weight: u64, fee: u64, parents: Vec) -> MempoolTx { + MempoolTx { + weight, + fee, + parents, + } +} + +/// Build a bump table at `feerate` sat/vB. A table is bound to one feerate, so asking about +/// another needs a second table. +fn table_at(txs: Vec, spends: Vec<(usize, usize)>, feerate: f32) -> BumpTable { + let cluster = Cluster::new(txs, spends).expect("well-formed"); + BumpTable::from_cluster(&cluster, FeeRate::from_sat_per_vb(feerate)) +} + +/// One transaction paying far too little, spent by candidate 0: 400 wu = 100 vB, so at 10 sat/vB +/// it owes 1000 but paid 10 => a 990 bump. +fn one_stuck_parent(feerate: f32) -> BumpTable { + table_at(vec![tx(400, 10, vec![])], vec![(0, 0)], feerate) +} + +fn candidates(n: usize, value: u64) -> Vec { + (0..n) + .map(|_| Candidate { + input_count: 1, + value, + weight: TR_KEYSPEND_TXIN_WEIGHT, + is_segwit: true, + }) + .collect() +} + +/// Candidates for a selector that will carry a table. Nothing is written into them: the bump lives +/// on the table, and the selector nets it off. +fn priced(n: usize, value: u64) -> Vec { + candidates(n, value) +} + +#[test] +fn zero_ancestors_backward_compatible() { + let candidates = candidates(1, 200_000); + + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)); + cs.select(0); + + assert_eq!( + cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), + 0 + ); + assert!( + cs.excess(Drain::NONE) > 0, + "should meet target without ancestors" + ); +} + +#[test] +fn single_ancestor_reduces_excess() { + let table = one_stuck_parent(10.0); + let feerate = FeeRate::from_sat_per_vb(10.0); + let target = simple_target(10.0); + + let plain = candidates(1, 200_000); + let mut cs_no_anc = CoinSelector::new(&plain, target); + cs_no_anc.select(0); + let excess_no_anc = cs_no_anc.excess(Drain::NONE); + + let with_ancestors = priced(1, 200_000); + let mut cs = CoinSelector::new(&with_ancestors, target).with_bump_table(&table); + cs.select(0); + + assert_eq!(cs.selected_ancestor_bump_fee(feerate), 990); + assert_eq!( + excess_no_anc - cs.excess(Drain::NONE), + 990, + "the bump comes straight off the excess" + ); +} + +#[test] +fn shared_ancestors_are_deduplicated() { + let table = table_at(vec![tx(400, 10, vec![])], vec![(0, 0), (1, 0)], 10.0); + let candidates = priced(2, 100_000); + let feerate = FeeRate::from_sat_per_vb(10.0); + + let mut cs_one = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs_one.select(0); + + let mut cs_both = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs_both.select(0); + cs_both.select(1); + + assert_eq!( + cs_one.selected_ancestor_bump_fee(feerate), + cs_both.selected_ancestor_bump_fee(feerate), + "a shared ancestor is paid for once, however many dependents are selected" + ); +} + +/// The sum of per-candidate bumps is what branch and bound searches on, and it must never come out +/// below the package figure the transaction pays — otherwise the search would under-reserve. +#[test] +fn the_local_sum_never_undercuts_the_package() { + let table = table_at(vec![tx(400, 10, vec![])], vec![(0, 0), (1, 0)], 10.0); + let candidates = priced(2, 100_000); + + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs.select(0); + cs.select(1); + + let local: u64 = (0..candidates.len()) + .map(|i| cs.ancestor_bump_fee_of(i)) + .sum(); + let combined = cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)); + + assert_eq!(combined, 990, "the shared ancestor is charged once"); + assert_eq!(local, 1_980, "but each candidate is charged for it alone"); + assert!(local >= combined, "the search must over-reserve, not under"); +} + +#[test] +fn ancestor_package_above_target_contributes_zero_bump() { + let table = table_at(vec![tx(400, 10_000, vec![])], vec![(0, 0)], 10.0); + let candidates = priced(1, 200_000); + + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs.select(0); + + assert_eq!( + cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), + 0 + ); +} + +/// A table's figures are only valid at the feerate it was built for, so comparing feerates means +/// building a table per feerate. +#[test] +fn different_feerates_produce_different_bump_fees() { + let bump_at = |feerate: f32| { + let table = table_at(vec![tx(400, 100, vec![])], vec![(0, 0)], feerate); + let candidates = priced(1, 200_000); + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs.select(0); + cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(feerate)) + }; + + assert!( + bump_at(20.0) > bump_at(5.0), + "a higher feerate owes more of a bump" + ); +} + +/// A table built for one feerate silently under-prices the package at any other, so the mismatch +/// is caught rather than tolerated. +#[test] +#[should_panic(expected = "bump table was built for a different feerate")] +fn bump_table_rejects_a_mismatched_feerate() { + let table = one_stuck_parent(10.0); + let candidates = priced(1, 200_000); + + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs.select(0); + cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(20.0)); +} + +/// The figure the selection algorithms rank on has to tell the whole truth about what a candidate +/// costs. `Candidate` cannot supply it — a bump only means something at one feerate, and a +/// `Candidate` has nowhere to record which — so it comes from the selector. +#[test] +fn the_selectors_effective_value_includes_the_bump() { + let table = one_stuck_parent(10.0); + let feerate = FeeRate::from_sat_per_vb(10.0); + let candidates = candidates(1, 200_000); + + let plain = CoinSelector::new(&candidates, simple_target(10.0)); + let with_ancestors = + CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + + assert_eq!(with_ancestors.ancestor_bump_fee_of(0), 990); + assert_eq!(plain.ancestor_bump_fee_of(0), 0, "no table, nothing owed"); + assert_eq!( + plain.effective_value_of(0, feerate) - with_ancestors.effective_value_of(0, feerate), + 990.0, + "effective value drops by exactly the bump" + ); + assert!(with_ancestors.value_pwu_of(0) < plain.value_pwu_of(0)); + assert_eq!( + candidates[0].effective_value(feerate), + plain.effective_value_of(0, feerate), + "`Candidate`'s own figure is the ancestor-blind one" + ); +} + +/// `implied_fee` is the exact counterpart of `excess`, so it must carry the ancestor bump. A +/// wallet sizing its change output from it would otherwise underpay the package. +#[test] +fn implied_fee_includes_ancestor_bump() { + let table = one_stuck_parent(10.0); + let target = simple_target(10.0); + let plain = candidates(1, 200_000); + let with_ancestors = priced(1, 200_000); + + let mut without = CoinSelector::new(&plain, target); + without.select(0); + let mut with = CoinSelector::new(&with_ancestors, target).with_bump_table(&table); + with.select(0); + + assert_eq!( + with.implied_fee(DrainWeights::NONE) - without.implied_fee(DrainWeights::NONE), + 990, + "implied_fee must carry the ancestor bump" + ); +} + +/// Nothing is banned: a candidate with unconfirmed ancestors is reachable by the automatic +/// algorithms like any other, because its cost is visible in the figures they rank on. +#[test] +fn ancestor_candidates_are_selectable() { + let table = one_stuck_parent(10.0); + let candidates = priced(1, 200_000); + let cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + + assert!(cs.banned().is_empty(), "nothing is banned any more"); + assert_eq!(cs.unselected_indices().collect::>(), vec![0]); + assert_eq!(cs.candidates_with_ancestors().collect::>(), vec![0]); + + let mut greedy = cs.clone(); + greedy + .select_until_target_met() + .expect("the ancestor candidate covers the target even after its bump"); + assert!(greedy.is_selected(0)); + assert_eq!( + greedy.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), + 990 + ); +} + +/// `excess == selected_value - target.value() - drain.value - implied_fee` must hold for every +/// combination of fee constraints. This pins the whole `*_excess` family against `implied_fee`, so +/// a bump added to one but not the other cannot go unnoticed. +#[test] +fn excess_and_implied_fee_agree() { + // (transactions, which candidate spends which) -- no ancestors, one stuck parent, and a stuck + // parent for candidate 0 alongside an already-paying one for candidate 1. + type Fixture = (Vec, Vec<(usize, usize)>); + let clusters: [Fixture; 3] = [ + (vec![], vec![]), + (vec![tx(400, 10, vec![])], vec![(0, 0)]), + ( + vec![tx(400, 10, vec![]), tx(1_000, 100_000, vec![])], + vec![(0, 0), (1, 1)], + ), + ]; + + for (txs, spends) in &clusters { + for absolute in [0_u64, 5_000, 500_000] { + for replace in [None, Some(Replace::new(1_000))] { + for feerate in [1.0_f32, 10.0, 50.0] { + // A table is bound to one feerate, so it belongs inside this loop. + let table = table_at(txs.clone(), spends.clone(), feerate); + let mut candidates = candidates(2, 200_000); + candidates[1].value = 50_000; + + for drain in [ + Drain::NONE, + Drain { + weights: DrainWeights::TR_KEYSPEND, + value: 20_000, + }, + ] { + let target = Target { + outputs: TargetOutputs { + value_sum: 100_000, + weight_sum: 200, + n_outputs: 1, + }, + fee: TargetFee { + rate: FeeRate::from_sat_per_vb(feerate), + replace, + absolute, + }, + max_weight: None, + }; + + for selection in [vec![], vec![0], vec![1], vec![0, 1]] { + let mut cs = + CoinSelector::new(&candidates, target).with_bump_table(&table); + for i in &selection { + cs.select(*i); + } + + assert_eq!( + cs.excess(drain), + cs.selected_value() as i64 + - target.value() as i64 + - drain.value as i64 + - cs.implied_fee(drain.weights) as i64, + "identity broken: n_txs={} absolute={} replace={} \ + feerate={} drain={} selection={:?}", + txs.len(), + absolute, + replace.is_some(), + feerate, + drain.value, + selection, + ); + } + } + } + } + } + } +} + +/// `Candidate::ancestor_bump_fee` is `feerate * weight - fee_paid` over the unconfirmed ancestors, +/// so it only means anything at the feerate it was computed for — and `Candidate` has nowhere to +/// record which. Every path that reaches it through a selector carrying a table is checked; +/// `select_all_effective` is the one that takes a feerate of its own. +#[test] +#[should_panic(expected = "bump table was built for a different feerate")] +fn selecting_all_effective_rejects_a_mismatched_feerate() { + let table = one_stuck_parent(10.0); + let candidates = priced(1, 200_000); + + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs.select_all_effective(FeeRate::from_sat_per_vb(20.0)); +} + +/// The same figures at the table's own feerate are fine, and the bump is visible in the ranking. +#[test] +fn selecting_all_effective_works_at_the_table_feerate() { + let table = one_stuck_parent(10.0); + let candidates = priced(1, 200_000); + + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + cs.select_all_effective(FeeRate::from_sat_per_vb(10.0)); + + assert!(cs.is_selected(0), "still worth its bump at 200_000 sats"); + assert_eq!( + cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), + 990 + ); +} diff --git a/tests/cpfp_cluster.rs b/tests/cpfp_cluster.rs new file mode 100644 index 0000000..d24e01e --- /dev/null +++ b/tests/cpfp_cluster.rs @@ -0,0 +1,311 @@ +use bdk_coin_select::{ + BumpTable, Candidate, Cluster, ClusterError, CoinSelector, FeeRate, MempoolTx, Target, + TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, +}; + +const RATE: f32 = 10.0; + +fn rate() -> FeeRate { + FeeRate::from_sat_per_vb(RATE) +} + +fn candidates(n: usize) -> Vec { + (0..n) + .map(|_| Candidate { + input_count: 1, + value: 200_000, + weight: TR_KEYSPEND_TXIN_WEIGHT, + is_segwit: true, + }) + .collect() +} + +/// A modest target these fixtures can all fund; the pricing assertions do not depend on it. +fn target() -> Target { + Target { + outputs: TargetOutputs { + value_sum: 100_000, + weight_sum: 200, + n_outputs: 1, + }, + fee: TargetFee::from_feerate(rate()), + max_weight: None, + } +} + +fn tx(weight: u64, fee: u64, parents: Vec) -> MempoolTx { + MempoolTx { + weight, + fee, + parents, + } +} + +/// The bump a selection of `selection` owes under `table`. +fn bump(table: &BumpTable, n_candidates: usize, selection: &[usize]) -> u64 { + let candidates = candidates(n_candidates); + let mut cs = CoinSelector::new(&candidates, target()).with_bump_table(table); + for &i in selection { + cs.select(i); + } + cs.selected_ancestor_bump_fee(rate()) +} + +/// A deficient parent with nothing to carry it has to be bumped, in full. +#[test] +fn a_deficient_ancestor_alone_is_charged_in_full() { + // 1000 wu = 250 vB. At 10 sat/vB it owes 2500 but paid 500. + let cluster = Cluster::new(vec![tx(1_000, 500, vec![])], vec![(0, 0)]).unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + + assert_eq!(bump(&table, 1, &[0]), 2_000); +} + +/// A deficient parent already carried by an overpaying child that we are *not* spending. A miner +/// includes both, so the parent needs no bump — and only a cluster can show that, since the child +/// is nowhere in this candidate's ancestry. +#[test] +fn a_parent_carried_by_someone_elses_child_needs_no_bump() { + let cluster = Cluster::new( + vec![ + tx(1_000, 500, vec![]), // 0: parent, 250 vB, owes 2500, paid 500 + tx(400, 4_000, vec![0]), // 1: its child, 100 vB, pays 4000 + ], + // We spend the *parent*. The child is someone else's, or another of ours. + vec![(0, 0)], + ) + .unwrap(); + + // Package {0,1} is 350 vB paying 4500 against 3500 owed, so a miner takes both. + let from_cluster = BumpTable::from_cluster(&cluster, rate()); + assert_eq!( + bump(&from_cluster, 1, &[0]), + 0, + "the child already pays for the parent" + ); +} + +/// Transitive closure is computed from the graph, so spending a grandchild pulls in the +/// grandparent without the caller having to say so. +#[test] +fn transitive_ancestors_are_pulled_in_automatically() { + let cluster = Cluster::new( + vec![ + tx(400, 0, vec![]), // 0: grandparent, 100 vB, pays nothing + tx(400, 0, vec![0]), // 1: parent + tx(400, 0, vec![1]), // 2: the tx we spend + ], + vec![(0, 2)], + ) + .unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + + // All three are unmined: 1200 wu = 300 vB, owes 3000, paid 0. + assert_eq!(bump(&table, 1, &[0]), 3_000); +} + +/// A transaction shared by two selected candidates is paid for once. +#[test] +fn a_shared_ancestor_is_charged_once() { + let cluster = Cluster::new( + vec![ + tx(400, 0, vec![]), // 0: shared parent + tx(400, 0, vec![0]), // 1: spent by candidate 0 + tx(400, 0, vec![0]), // 2: spent by candidate 1 + ], + vec![(0, 1), (1, 2)], + ) + .unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + + // One candidate: parent + its own tx = 800 wu = 200 vB => 2000. + assert_eq!(bump(&table, 2, &[0]), 2_000); + assert_eq!(bump(&table, 2, &[1]), 2_000); + // Both: parent counted once => 1200 wu = 300 vB => 3000, not 4000. + assert_eq!(bump(&table, 2, &[0, 1]), 3_000); +} + +/// A cluster already paying above the target is mined entirely, so nothing is owed. +#[test] +fn a_cluster_above_the_target_owes_nothing() { + let cluster = Cluster::new( + vec![tx(400, 10_000, vec![]), tx(400, 10_000, vec![0])], + vec![(0, 1)], + ) + .unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + + assert_eq!(bump(&table, 1, &[0]), 0); + assert_eq!(bump(&table, 1, &[]), 0, "the empty subset always owes zero"); +} + +/// Mining is package-wise, so a deficient parent is carried by an overpaying descendant we *are* +/// spending — the whole chain comes in together or not at all. +#[test] +fn an_overpaying_descendant_carries_its_deficient_parent() { + let cluster = Cluster::new( + vec![ + tx(1_000, 500, vec![]), // 250 vB, owes 2500, paid 500 + tx(400, 4_000, vec![0]), // 100 vB, pays 4000 + ], + vec![(0, 1)], // we spend the child + ) + .unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + + assert_eq!(bump(&table, 1, &[0]), 0); +} + +#[test] +fn cluster_rejects_malformed_input() { + assert_eq!( + Cluster::new(vec![tx(400, 0, vec![7])], vec![]).unwrap_err(), + ClusterError::ParentOutOfBounds { tx: 0, parent: 7 } + ); + assert_eq!( + Cluster::new(vec![tx(400, 0, vec![])], vec![(0, 3)]).unwrap_err(), + ClusterError::SpendOutOfBounds { + candidate: 0, + tx: 3 + } + ); + assert!(matches!( + Cluster::new(vec![tx(400, 0, vec![1]), tx(400, 0, vec![0])], vec![(0, 0)]), + Err(ClusterError::Cycle { .. }) + )); +} + +/// Selecting a bump-neutral candidate really does leave the package price alone -- which is the +/// property that makes leaving it unbanned safe. +#[test] +fn selecting_a_bump_neutral_candidate_does_not_move_the_price() { + let cluster = Cluster::new( + vec![tx(400, 2_000, vec![]), tx(400, 10, vec![])], + vec![(0, 0), (1, 1)], + ) + .unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + + // Neutral candidate alone, and added on top of the stuck one: neither changes anything. + assert_eq!(bump(&table, 2, &[]), 0); + assert_eq!(bump(&table, 2, &[0]), 0); + assert_eq!(bump(&table, 2, &[1]), 990); + assert_eq!(bump(&table, 2, &[0, 1]), 990); +} + +/// **The inequality the whole design rests on.** Branch and bound searches on the sum of +/// per-candidate bumps while the crate reports the combined package figure. That is only safe if +/// the sum is never *below* the combined figure — otherwise the search would under-reserve and the +/// transaction would come up short. A mock template guarantees it: an ancestor that overpays gets +/// mined out, so whatever two candidates share is itself ancestor-closed and therefore deficient. +#[test] +fn the_local_sum_never_undercuts_the_package_for_any_cluster() { + // Deterministic xorshift, so a counterexample is reproducible from its seed. + let mut state = 0x2545F4914F6CDD1D_u64; + let mut next = move || { + state ^= state << 13; + state ^= state >> 7; + state ^= state << 17; + state + }; + + for trial in 0..2_000 { + let n_txs = 1 + (next() % 6) as usize; + let mut txs = Vec::new(); + for i in 0..n_txs { + // Parents are always earlier transactions, so the graph is acyclic by construction. + let mut parents = Vec::new(); + for p in 0..i { + if next() % 3 == 0 { + parents.push(p); + } + } + let weight = 400 + (next() % 1_200) / 4 * 4; + // Fees straddle the target rate so both mined and stuck transactions occur. + let fee = (weight / 4) * (next() % 30); + txs.push(tx(weight, fee, parents)); + } + + let n_candidates = 1 + (next() % 4) as usize; + let spends: Vec<(usize, usize)> = (0..n_candidates) + .map(|c| (c, (next() % n_txs as u64) as usize)) + .collect(); + + let cluster = Cluster::new(txs, spends).expect("acyclic by construction"); + let table = BumpTable::from_cluster(&cluster, rate()); + let cands = candidates(n_candidates); + let cs = CoinSelector::new(&cands, target()).with_bump_table(&table); + + for mask in 0..(1_u32 << n_candidates) { + let selection: Vec = + (0..n_candidates).filter(|b| mask & (1 << b) != 0).collect(); + let local: u64 = selection.iter().map(|&i| cs.ancestor_bump_fee_of(i)).sum(); + let combined = bump(&table, n_candidates, &selection); + assert!( + local >= combined, + "trial {}: local sum {} undercuts the package {} for selection {:?}", + trial, + local, + combined, + selection + ); + } + } +} + +/// Branch and bound may now choose candidates with unconfirmed ancestors, so the selection it +/// returns has to be genuinely funded once priced exactly — the search reasons about the local +/// over-estimate, and the difference must land in the change output rather than in a shortfall. +#[test] +fn branch_and_bound_selections_are_funded_when_priced_exactly() { + use bdk_coin_select::{metrics::LowestFee, DrainWeights, Target, TargetFee, TargetOutputs}; + + let cluster = Cluster::new( + vec![ + tx(1_000, 500, vec![]), // stuck parent, shared by candidates 0 and 1 + tx(400, 10, vec![0]), // stuck child + tx(400, 4_000, vec![]), // already paying + ], + vec![(0, 0), (1, 1), (2, 2)], + ) + .unwrap(); + let table = BumpTable::from_cluster(&cluster, rate()); + let cands = candidates(4); + + let target = Target { + outputs: TargetOutputs { + value_sum: 300_000, + weight_sum: 200, + n_outputs: 1, + }, + fee: TargetFee::from_feerate(rate()), + max_weight: None, + }; + let metric = LowestFee { + long_term_feerate: FeeRate::from_sat_per_vb(5.0), + dust_relay_feerate: FeeRate::from_sat_per_vb(1.0), + drain_weights: DrainWeights::TR_KEYSPEND, + }; + + let mut cs = CoinSelector::new(&cands, target).with_bump_table(&table); + let (_, drain) = cs.run_bnb(metric, 100_000).expect("a solution exists"); + + assert!( + cs.is_funded_with_drain(drain), + "the returned selection must fund the target under exact pricing" + ); + assert!( + cs.is_selected(0) || cs.is_selected(1) || cs.is_selected(2) || cs.is_selected(3), + "something was selected" + ); + // The drain handed back is sized by the exact package cost, not the search's over-estimate. + assert_eq!( + drain.value, + cs.drain_value(bdk_coin_select::ChangePolicy { + min_value: 0, + drain_weights: DrainWeights::TR_KEYSPEND, + }) + .unwrap_or(0), + "run_bnb's drain must match the exactly-priced drain value" + ); +} From 0596f4e3a7183e2fef5ed5bd30b1c0432482524e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 4 Aug 2026 03:59:22 +0000 Subject: [PATCH 3/5] Take the `Cluster` at the selector, and build the pricing table internally `with_bump_table` accepted a `BumpTable` built at any feerate, and two runtime asserts caught the case where it disagreed with the selector's target. But the selector owns its target now, so it can build the table itself: pub fn with_cluster(mut self, cluster: &Cluster) -> Self The table is always derived at `target.fee.rate`, the mismatch stops being representable, and both asserts disappear -- the same move that took the bump off `Candidate`, applied one level up. Since `from_ancestors` and `from_fn` were removed earlier, `from_cluster` was `BumpTable`'s only constructor, so `with_bump_table` was not an escape hatch for externally-computed figures; it was only a way to build the table at the wrong rate. With it gone, nothing public accepts or returns a `BumpTable`, so the type drops out of the public API entirely. Callers see `Cluster` in and per-candidate figures out (`ancestor_bump_fee_of`, `effective_value_of`, `value_pwu_of`, `selected_ancestor_bump_fee`). `selected_ancestor_bump_fee` loses its feerate parameter -- it existed only to catch the now-unrepresentable mismatch. `effective_value_of` and `CoinSelector::effective_value` keep theirs (the rate genuinely enters the weight term) and now check it against the target's feerate, since a bump computed at the target rate must not be netted off a differently-rated figure. The selector stores the table as `Arc`: it is cloned at every branch and bound node, and the table never changes after construction, so clone stays a refcount bump. Co-Authored-By: Claude Opus 5 --- src/ancestor_search_experiment.rs | 13 ++-- src/bump_table.rs | 18 +---- src/coin_selector.rs | 105 +++++++++++++------------ src/lib.rs | 1 - src/mempool.rs | 14 ++-- src/metrics/lowest_fee.rs | 6 +- tests/ancestor_aware.rs | 124 +++++++++++------------------- tests/cpfp_cluster.rs | 57 +++++++------- 8 files changed, 143 insertions(+), 195 deletions(-) diff --git a/src/ancestor_search_experiment.rs b/src/ancestor_search_experiment.rs index fc3f665..aef3ec1 100644 --- a/src/ancestor_search_experiment.rs +++ b/src/ancestor_search_experiment.rs @@ -40,8 +40,8 @@ //! [`CoinSelector::effective_value_of`]: crate::CoinSelector::effective_value_of use crate::{ - float::Ordf32, metrics::LowestFee, BnbMetric, BumpTable, Candidate, Cluster, CoinSelector, - DrainWeights, FeeRate, MempoolTx, Target, TargetFee, TargetOutputs, + float::Ordf32, metrics::LowestFee, BnbMetric, Candidate, Cluster, CoinSelector, DrainWeights, + FeeRate, MempoolTx, Target, TargetFee, TargetOutputs, }; use alloc::vec::Vec; @@ -63,7 +63,7 @@ impl Rng { struct Instance { candidates: Vec, - table: BumpTable, + cluster: Cluster, target: Target, } @@ -126,11 +126,10 @@ fn instance(rng: &mut Rng, n: usize, k: usize, p_fine_pct: u64) -> Option Option(inst: &'a Instance) -> CoinSelector<'a> { - CoinSelector::new(&inst.candidates, inst.target).with_bump_table(&inst.table) + CoinSelector::new(&inst.candidates, inst.target).with_cluster(&inst.cluster) } /// The best exactly-priced score over every subset. @@ -197,7 +196,7 @@ fn brute_force_allowing(inst: &Instance, allowed: &[usize]) -> Option { /// it. fn selectable_under_ban(inst: &Instance) -> Vec { (0..inst.candidates.len()) - .filter(|&c| inst.table.individual(c) == 0) + .filter(|&c| exact(inst).ancestor_bump_fee_of(c) == 0) .collect() } diff --git a/src/bump_table.rs b/src/bump_table.rs index 16813b8..08fd082 100644 --- a/src/bump_table.rs +++ b/src/bump_table.rs @@ -42,7 +42,7 @@ struct Unit { /// [`CoinSelector::selected_ancestor_bump_fee`]: crate::CoinSelector::selected_ancestor_bump_fee /// [`Target::fee`]: crate::Target::fee #[derive(Debug, Clone)] -pub struct BumpTable { +pub(crate) struct BumpTable { feerate: FeeRate, /// The unconfirmed transactions that may still need bumping: for a cluster, the ones a miner /// would leave behind; for a flat ancestor list, all of them. @@ -65,7 +65,7 @@ impl BumpTable { /// /// The template is built once: which transactions a miner includes depends on the cluster and /// the feerate, not on which outputs you happen to be asking about. - pub fn from_cluster(cluster: &Cluster, feerate: FeeRate) -> Self { + pub(crate) fn from_cluster(cluster: &Cluster, feerate: FeeRate) -> Self { let mined = cluster.mine(feerate); // Renumber the survivors so units are dense and the mined transactions simply do not exist. @@ -102,13 +102,8 @@ impl BumpTable { } } - /// The feerate these figures were computed for. They are meaningless at any other. - pub fn feerate(&self) -> FeeRate { - self.feerate - } - /// The candidates that carry unconfirmed ancestors, ascending. - pub fn candidates(&self) -> impl Iterator + '_ { + pub(crate) fn candidates(&self) -> impl Iterator + '_ { self.entries.keys().copied() } @@ -118,15 +113,10 @@ impl BumpTable { /// already include. Summing these across a selection over-estimates the combined package /// figure whenever ancestors are shared — and that over-estimate is the price of the figure /// being additive, which is what the selection algorithms need. - pub fn individual(&self, candidate: usize) -> u64 { + pub(crate) fn individual(&self, candidate: usize) -> u64 { self.entries.get(&candidate).map_or(0, |&(_, bump)| bump) } - /// Every candidate's individual bump, ascending by candidate index. - pub fn individual_bumps(&self) -> impl Iterator + '_ { - self.entries.iter().map(|(&c, &(_, bump))| (c, bump)) - } - /// What the candidates in `selected` owe *together*, with shared ancestors counted once. /// /// This is what the transaction actually has to pay. diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 98f07f2..0727939 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -41,8 +41,10 @@ pub struct CoinSelector<'a> { /// built for one target and evaluated against it throughout, and threading it through every /// method made it possible to ask two different questions of the same selection. target: Target, - /// Exact CPFP pricing (via [`CoinSelector::with_bump_table`]). - bump_table: Option<&'a BumpTable>, + /// Exact CPFP pricing, built from the caller's [`Cluster`] at the target's feerate (via + /// [`CoinSelector::with_cluster`]). `Arc` because the selector is cloned at every branch and + /// bound node, and the table never changes after construction. + bump_table: Option>, /// Which of the two ancestor-bump models this selector answers with. See [`Pricing`]. pricing: Pricing, selected: Bitset, @@ -83,21 +85,25 @@ impl<'a> CoinSelector<'a> { self.target } - /// Report CPFP package costs exactly, using `bump_table`. + /// Price CPFP packages exactly, for candidates that spend unconfirmed outputs of `cluster`. + /// + /// The pricing table is built here, at this selector's target feerate — the caller cannot + /// supply figures derived at some other rate, which would silently under-price the package. + /// This is why the method takes the raw [`Cluster`] rather than anything precomputed. /// /// Nothing is banned: candidates with unconfirmed ancestors are selected like any other. That /// works because the *search* reasons about [`effective_value_of`], which nets off each - /// candidate's individual bump and is therefore additive, while everything this selector reports — - /// [`excess`], [`implied_fee`], [`is_funded`], [`drain`] — uses the table's exact combined + /// candidate's individual bump and is therefore additive, while everything this selector + /// reports — [`excess`], [`implied_fee`], [`is_funded`], [`drain`] — uses the exact combined /// figure, in which an ancestor shared by two candidates is paid for once. /// /// The two differ, and deliberately: the sum of per-candidate bumps is never below the /// combined one, so the search *over*-reserves and the surplus surfaces as a larger change - /// output. See [`BumpTable`]. + /// output rather than a missing fee. /// /// # Panics /// - /// If the table refers to a candidate index out of bounds for the slice passed to + /// If the cluster refers to a candidate index out of bounds for the slice passed to /// [`CoinSelector::new`]. /// /// [`effective_value_of`]: Self::effective_value_of @@ -105,16 +111,17 @@ impl<'a> CoinSelector<'a> { /// [`implied_fee`]: Self::implied_fee /// [`is_funded`]: Self::is_funded /// [`drain`]: Self::drain - pub fn with_bump_table(mut self, bump_table: &'a BumpTable) -> Self { + pub fn with_cluster(mut self, cluster: &Cluster) -> Self { + let bump_table = BumpTable::from_cluster(cluster, self.target.fee.rate); if let Some(max) = bump_table.max_candidate_index() { assert!( max < self.candidates.len(), - "bump table refers to candidate index {} but there are only {} candidates", + "cluster refers to candidate index {} but there are only {} candidates", max, self.candidates.len() ); } - self.bump_table = Some(bump_table); + self.bump_table = Some(Arc::new(bump_table)); self } @@ -145,7 +152,7 @@ impl<'a> CoinSelector<'a> { /// /// [ancestor bump fee]: Self::selected_ancestor_bump_fee pub fn candidates_with_ancestors(&self) -> impl Iterator + '_ { - self.bump_table.into_iter().flat_map(|t| t.candidates()) + self.bump_table.iter().flat_map(|t| t.candidates()) } /// Iterate over all the candidates in their currently sorted order. Each item has the original @@ -287,10 +294,10 @@ impl<'a> CoinSelector<'a> { } /// The extra fee this selection owes on top of its own weight, so that its unconfirmed - /// ancestors reach `feerate` as a package (CPFP). + /// ancestors reach the target feerate as a package (CPFP). /// /// Which of the two models answers depends on how this selector is priced. Everything a - /// caller can reach uses the **exact** figure — the [`BumpTable`]'s combined bump, in which an + /// caller can reach uses the **exact** figure — the combined package bump, in which an /// ancestor shared by two selected candidates is paid for once, and which is what the /// transaction actually owes. /// @@ -298,26 +305,14 @@ impl<'a> CoinSelector<'a> { /// candidates' [`ancestor_bump_fee_of`] — additive, and therefore never below the exact one, /// so its ranking and bounds can rely on it. /// - /// Zero when neither a table nor per-candidate bumps were supplied. - /// - /// # Panics - /// - /// If `feerate` is not the one the table was built for. A table's figures are only valid at - /// its own feerate, and using them at another under-prices the package — in *both* directions, - /// so there is no safe side to land on. The check is one comparison and only runs when a table - /// is present, so it is not gated on debug builds. + /// Zero when no [`Cluster`] was supplied. /// /// [`ancestor_bump_fee_of`]: Self::ancestor_bump_fee_of - pub fn selected_ancestor_bump_fee(&self, feerate: FeeRate) -> u64 { - let bump_table = match self.bump_table { + pub fn selected_ancestor_bump_fee(&self) -> u64 { + let bump_table = match &self.bump_table { Some(bump_table) => bump_table, None => return 0, }; - assert_eq!( - feerate, - bump_table.feerate(), - "bump table was built for a different feerate; its figures do not apply here" - ); match self.pricing { Pricing::Local => self.selected.iter().map(|i| bump_table.individual(i)).sum(), Pricing::Exact => bump_table.combined(&self.selected), @@ -325,30 +320,31 @@ impl<'a> CoinSelector<'a> { } /// What selecting the candidate at `index` alone would owe to bring its unconfirmed ancestors - /// up to the target feerate, in satoshis. Zero without a [`BumpTable`], or for a candidate - /// with no unconfirmed ancestors. + /// up to the target feerate, in satoshis. Zero without a [`Cluster`], or for a candidate with + /// no unconfirmed ancestors. /// - /// Additive across candidates, and never in total below what the package actually owes -- see - /// [`BumpTable`] for why that direction matters. + /// Additive across candidates, and never in total below what the package actually owes -- + /// which is what makes it safe to search on; see [`with_cluster`](Self::with_cluster). pub fn ancestor_bump_fee_of(&self, index: usize) -> u64 { - self.bump_table.map_or(0, |t| t.individual(index)) + self.bump_table.as_ref().map_or(0, |t| t.individual(index)) } /// [`Candidate::effective_value`] less what that candidate's unconfirmed ancestors cost. /// /// This is the figure to rank candidates on. `Candidate` cannot compute it: a bump is only /// meaningful at the feerate it was derived for, and a `Candidate` has nowhere to record - /// which -- so it lives here, where the [`BumpTable`] is, and the feerate can be checked. + /// which -- so it lives here, where the target (and hence the feerate the bump was built at) + /// is known and can be checked. /// /// # Panics /// - /// If `feerate` is not the one the attached table was built for. + /// If a [`Cluster`] is attached and `feerate` is not the target's feerate — the bump is + /// computed at the target rate, and netting it off a differently-rated figure mixes rates. pub fn effective_value_of(&self, index: usize, feerate: FeeRate) -> f32 { - if let Some(bump_table) = self.bump_table { + if self.bump_table.is_some() { assert_eq!( - feerate, - bump_table.feerate(), - "bump table was built for a different feerate; its figures do not apply here" + feerate, self.target.fee.rate, + "the ancestor bump is computed at the target feerate; asking at another mixes rates" ); } self.candidates[index].effective_value(feerate) - self.ancestor_bump_fee_of(index) as f32 @@ -472,7 +468,7 @@ impl<'a> CoinSelector<'a> { .fee .rate .implied_fee(self.weight(self.target.outputs, drain_weights)) - + self.selected_ancestor_bump_fee(self.target.fee.rate) + + self.selected_ancestor_bump_fee() } /// Same as [`implied_package_fee_from_feerate`](Self::implied_package_fee_from_feerate) except `self.target.fee.rate` @@ -482,7 +478,7 @@ impl<'a> CoinSelector<'a> { .fee .rate .implied_fee_wu(self.weight(self.target.outputs, drain_weights)) - + self.selected_ancestor_bump_fee(self.target.fee.rate) + + self.selected_ancestor_bump_fee() } /// The fee needed for the whole CPFP package to satisfy RBF's rule 4, i.e. the replacement fee @@ -497,7 +493,7 @@ impl<'a> CoinSelector<'a> { } None => 0, }; - replacement_fee + self.selected_ancestor_bump_fee(self.target.fee.rate) + replacement_fee + self.selected_ancestor_bump_fee() } /// Same as [`implied_package_fee_from_replacement`](Self::implied_package_fee_from_replacement) except the @@ -508,7 +504,7 @@ impl<'a> CoinSelector<'a> { .min_fee_to_do_replacement_wu(self.weight(self.target.outputs, drain_weights)), None => 0, }; - replacement_fee + self.selected_ancestor_bump_fee(self.target.fee.rate) + replacement_fee + self.selected_ancestor_bump_fee() } /// The actual fee the selection would pay if it was used in a transaction that had @@ -521,10 +517,21 @@ impl<'a> CoinSelector<'a> { /// The value of the current selected inputs minus the fee needed to pay for the selected inputs /// and any ancestor bump fee. + /// + /// # Panics + /// + /// If a [`Cluster`] is attached and `feerate` is not the target's feerate; see + /// [`effective_value_of`](Self::effective_value_of). pub fn effective_value(&self, feerate: FeeRate) -> i64 { + if self.bump_table.is_some() { + assert_eq!( + feerate, self.target.fee.rate, + "the ancestor bump is computed at the target feerate; asking at another mixes rates" + ); + } self.selected_value() as i64 - (self.input_weight() as f32 * feerate.spwu()).ceil() as i64 - - self.selected_ancestor_bump_fee(feerate) as i64 + - self.selected_ancestor_bump_fee() as i64 } // /// Waste sum of all selected inputs. @@ -760,15 +767,7 @@ impl<'a> CoinSelector<'a> { /// /// A candidate if effective if it provides more value than it takes to pay for at `feerate`. pub fn select_all_effective(&mut self, feerate: FeeRate) { - // `Candidate::effective_value` subtracts a bump computed for the table's feerate, so - // ranking at any other one is meaningless. See `Candidate::ancestor_bump_fee`. - if let Some(bump_table) = self.bump_table { - assert_eq!( - feerate, - bump_table.feerate(), - "bump table was built for a different feerate; its figures do not apply here" - ); - } + // `effective_value_of` asserts `feerate` matches the target when a cluster is attached. for i in 0..self.candidate_order.len() { let cand_index = self.candidate_order[i]; if self.selected.contains(cand_index) diff --git a/src/lib.rs b/src/lib.rs index f63bf68..e1d1ef0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -14,7 +14,6 @@ extern crate std; mod bitset; pub use bitset::*; mod bump_table; -pub use bump_table::*; mod mempool; pub use mempool::*; #[cfg(test)] diff --git a/src/mempool.rs b/src/mempool.rs index a536d1b..5d9e355 100644 --- a/src/mempool.rs +++ b/src/mempool.rs @@ -18,8 +18,8 @@ pub struct MempoolTx { /// A connected piece of the mempool: the unconfirmed transactions relevant to a selection, and /// which candidates spend from which. /// -/// This is the input to [`BumpTable`]. Supplying the graph rather than a flat ancestor list buys -/// three things a list cannot express: +/// This is the input to [`CoinSelector::with_cluster`]. Supplying the graph rather than a flat +/// ancestor list buys three things a list cannot express: /// /// - **Transitive closure is computed here**, so a candidate cannot be under-priced by a caller /// listing only its direct parent. @@ -27,15 +27,14 @@ pub struct MempoolTx { /// nothing, and an overpaying child that carries a deficient parent means neither is charged /// for. A flat list has to charge for everything it is given. /// - **The bump stays safe to search on.** Branch and bound ranks candidates on their -/// [individual] bumps, whose sum must never fall below what the package owes together. Mining -/// guarantees that; pooling a flat list does not, because a shared ancestor that *overpays* has -/// its surplus counted once per dependent. +/// individual bumps ([`CoinSelector::ancestor_bump_fee_of`]), whose sum must never fall below +/// what the package owes together. Mining guarantees that; pooling a flat list does not, because +/// a shared ancestor that *overpays* has its surplus counted once per dependent. /// /// If all you have is a flat list of ancestors, express it here: each ancestor becomes a /// transaction with no parents, and each (ancestor, candidate) pair becomes an entry in /// `candidate_spends`. You then get the mining step for free. /// -/// [individual]: crate::BumpTable::individual /// /// # Completeness /// @@ -46,7 +45,8 @@ pub struct MempoolTx { /// the safe direction, and it is the reason this is an optimality limit rather than a correctness /// one. /// -/// [`BumpTable`]: crate::BumpTable +/// [`CoinSelector::with_cluster`]: crate::CoinSelector::with_cluster +/// [`CoinSelector::ancestor_bump_fee_of`]: crate::CoinSelector::ancestor_bump_fee_of #[derive(Debug, Clone)] pub struct Cluster { txs: Vec, diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index 6344e25..0b3c21f 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -180,7 +180,7 @@ impl BnbMetric for LowestFee { // The bumps the node itself has already committed to. Every descendant pays these, so // they belong in the deficit below; the ones the greedy prefix adds on top do not, // because a descendant may simply not select those candidates. - let committed_bump = cs.selected_ancestor_bump_fee(cs.target().fee.rate); + let committed_bump = cs.selected_ancestor_bump_fee(); // Step 1: select everything up until the input that hits the cs.target(). // @@ -201,11 +201,11 @@ impl BnbMetric for LowestFee { // only what this node has already committed to, or the deficit is overstated and the // bound stops being a lower bound. let uncommitted_bump = cs - .selected_ancestor_bump_fee(cs.target().fee.rate) + .selected_ancestor_bump_fee() .saturating_sub(committed_bump) as f32; // We need to find the minimum fee we'd pay if we satisfy the feerate constraint. We do - // this by imagining we had a perfect input that perfectly hit the cs.target(). The sats per + // this by imagining we had a perfect input that perfectly hit the target. The sats per // weight unit of this perfect input is that of `to_resize` but we'll do a scaled // resize of it to fit perfectly. // diff --git a/tests/ancestor_aware.rs b/tests/ancestor_aware.rs index e5519a9..d5c67cb 100644 --- a/tests/ancestor_aware.rs +++ b/tests/ancestor_aware.rs @@ -1,6 +1,6 @@ use bdk_coin_select::{ - BumpTable, Candidate, Cluster, CoinSelector, Drain, DrainWeights, FeeRate, MempoolTx, Replace, - Target, TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, + Candidate, Cluster, CoinSelector, Drain, DrainWeights, FeeRate, MempoolTx, Replace, Target, + TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, }; fn simple_target(feerate: f32) -> Target { @@ -23,17 +23,14 @@ fn tx(weight: u64, fee: u64, parents: Vec) -> MempoolTx { } } -/// Build a bump table at `feerate` sat/vB. A table is bound to one feerate, so asking about -/// another needs a second table. -fn table_at(txs: Vec, spends: Vec<(usize, usize)>, feerate: f32) -> BumpTable { - let cluster = Cluster::new(txs, spends).expect("well-formed"); - BumpTable::from_cluster(&cluster, FeeRate::from_sat_per_vb(feerate)) +fn cluster(txs: Vec, spends: Vec<(usize, usize)>) -> Cluster { + Cluster::new(txs, spends).expect("well-formed") } /// One transaction paying far too little, spent by candidate 0: 400 wu = 100 vB, so at 10 sat/vB /// it owes 1000 but paid 10 => a 990 bump. -fn one_stuck_parent(feerate: f32) -> BumpTable { - table_at(vec![tx(400, 10, vec![])], vec![(0, 0)], feerate) +fn one_stuck_parent() -> Cluster { + cluster(vec![tx(400, 10, vec![])], vec![(0, 0)]) } fn candidates(n: usize, value: u64) -> Vec { @@ -60,10 +57,7 @@ fn zero_ancestors_backward_compatible() { let mut cs = CoinSelector::new(&candidates, simple_target(10.0)); cs.select(0); - assert_eq!( - cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), - 0 - ); + assert_eq!(cs.selected_ancestor_bump_fee(), 0); assert!( cs.excess(Drain::NONE) > 0, "should meet target without ancestors" @@ -72,8 +66,7 @@ fn zero_ancestors_backward_compatible() { #[test] fn single_ancestor_reduces_excess() { - let table = one_stuck_parent(10.0); - let feerate = FeeRate::from_sat_per_vb(10.0); + let cluster = one_stuck_parent(); let target = simple_target(10.0); let plain = candidates(1, 200_000); @@ -82,10 +75,10 @@ fn single_ancestor_reduces_excess() { let excess_no_anc = cs_no_anc.excess(Drain::NONE); let with_ancestors = priced(1, 200_000); - let mut cs = CoinSelector::new(&with_ancestors, target).with_bump_table(&table); + let mut cs = CoinSelector::new(&with_ancestors, target).with_cluster(&cluster); cs.select(0); - assert_eq!(cs.selected_ancestor_bump_fee(feerate), 990); + assert_eq!(cs.selected_ancestor_bump_fee(), 990); assert_eq!( excess_no_anc - cs.excess(Drain::NONE), 990, @@ -95,20 +88,19 @@ fn single_ancestor_reduces_excess() { #[test] fn shared_ancestors_are_deduplicated() { - let table = table_at(vec![tx(400, 10, vec![])], vec![(0, 0), (1, 0)], 10.0); + let cluster = cluster(vec![tx(400, 10, vec![])], vec![(0, 0), (1, 0)]); let candidates = priced(2, 100_000); - let feerate = FeeRate::from_sat_per_vb(10.0); - let mut cs_one = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs_one = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); cs_one.select(0); - let mut cs_both = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs_both = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); cs_both.select(0); cs_both.select(1); assert_eq!( - cs_one.selected_ancestor_bump_fee(feerate), - cs_both.selected_ancestor_bump_fee(feerate), + cs_one.selected_ancestor_bump_fee(), + cs_both.selected_ancestor_bump_fee(), "a shared ancestor is paid for once, however many dependents are selected" ); } @@ -117,17 +109,17 @@ fn shared_ancestors_are_deduplicated() { /// below the package figure the transaction pays — otherwise the search would under-reserve. #[test] fn the_local_sum_never_undercuts_the_package() { - let table = table_at(vec![tx(400, 10, vec![])], vec![(0, 0), (1, 0)], 10.0); + let cluster = cluster(vec![tx(400, 10, vec![])], vec![(0, 0), (1, 0)]); let candidates = priced(2, 100_000); - let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); cs.select(0); cs.select(1); let local: u64 = (0..candidates.len()) .map(|i| cs.ancestor_bump_fee_of(i)) .sum(); - let combined = cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)); + let combined = cs.selected_ancestor_bump_fee(); assert_eq!(combined, 990, "the shared ancestor is charged once"); assert_eq!(local, 1_980, "but each candidate is charged for it alone"); @@ -136,28 +128,25 @@ fn the_local_sum_never_undercuts_the_package() { #[test] fn ancestor_package_above_target_contributes_zero_bump() { - let table = table_at(vec![tx(400, 10_000, vec![])], vec![(0, 0)], 10.0); + let cluster = cluster(vec![tx(400, 10_000, vec![])], vec![(0, 0)]); let candidates = priced(1, 200_000); - let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); cs.select(0); - assert_eq!( - cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), - 0 - ); + assert_eq!(cs.selected_ancestor_bump_fee(), 0); } -/// A table's figures are only valid at the feerate it was built for, so comparing feerates means -/// building a table per feerate. +/// The bump is priced at the selector's own target feerate — there is no separate rate to pass, +/// and so no way to price the package at a rate other than the one being aimed for. #[test] fn different_feerates_produce_different_bump_fees() { let bump_at = |feerate: f32| { - let table = table_at(vec![tx(400, 100, vec![])], vec![(0, 0)], feerate); + let cluster = cluster(vec![tx(400, 100, vec![])], vec![(0, 0)]); let candidates = priced(1, 200_000); - let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs = CoinSelector::new(&candidates, simple_target(feerate)).with_cluster(&cluster); cs.select(0); - cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(feerate)) + cs.selected_ancestor_bump_fee() }; assert!( @@ -166,31 +155,17 @@ fn different_feerates_produce_different_bump_fees() { ); } -/// A table built for one feerate silently under-prices the package at any other, so the mismatch -/// is caught rather than tolerated. -#[test] -#[should_panic(expected = "bump table was built for a different feerate")] -fn bump_table_rejects_a_mismatched_feerate() { - let table = one_stuck_parent(10.0); - let candidates = priced(1, 200_000); - - let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); - cs.select(0); - cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(20.0)); -} - /// The figure the selection algorithms rank on has to tell the whole truth about what a candidate /// costs. `Candidate` cannot supply it — a bump only means something at one feerate, and a /// `Candidate` has nowhere to record which — so it comes from the selector. #[test] fn the_selectors_effective_value_includes_the_bump() { - let table = one_stuck_parent(10.0); + let cluster = one_stuck_parent(); let feerate = FeeRate::from_sat_per_vb(10.0); let candidates = candidates(1, 200_000); let plain = CoinSelector::new(&candidates, simple_target(10.0)); - let with_ancestors = - CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let with_ancestors = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); assert_eq!(with_ancestors.ancestor_bump_fee_of(0), 990); assert_eq!(plain.ancestor_bump_fee_of(0), 0, "no table, nothing owed"); @@ -211,14 +186,14 @@ fn the_selectors_effective_value_includes_the_bump() { /// wallet sizing its change output from it would otherwise underpay the package. #[test] fn implied_fee_includes_ancestor_bump() { - let table = one_stuck_parent(10.0); + let cluster = one_stuck_parent(); let target = simple_target(10.0); let plain = candidates(1, 200_000); let with_ancestors = priced(1, 200_000); let mut without = CoinSelector::new(&plain, target); without.select(0); - let mut with = CoinSelector::new(&with_ancestors, target).with_bump_table(&table); + let mut with = CoinSelector::new(&with_ancestors, target).with_cluster(&cluster); with.select(0); assert_eq!( @@ -232,9 +207,9 @@ fn implied_fee_includes_ancestor_bump() { /// algorithms like any other, because its cost is visible in the figures they rank on. #[test] fn ancestor_candidates_are_selectable() { - let table = one_stuck_parent(10.0); + let cluster = one_stuck_parent(); let candidates = priced(1, 200_000); - let cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let cs = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); assert!(cs.banned().is_empty(), "nothing is banned any more"); assert_eq!(cs.unselected_indices().collect::>(), vec![0]); @@ -245,10 +220,7 @@ fn ancestor_candidates_are_selectable() { .select_until_target_met() .expect("the ancestor candidate covers the target even after its bump"); assert!(greedy.is_selected(0)); - assert_eq!( - greedy.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), - 990 - ); + assert_eq!(greedy.selected_ancestor_bump_fee(), 990); } /// `excess == selected_value - target.value() - drain.value - implied_fee` must hold for every @@ -272,8 +244,7 @@ fn excess_and_implied_fee_agree() { for absolute in [0_u64, 5_000, 500_000] { for replace in [None, Some(Replace::new(1_000))] { for feerate in [1.0_f32, 10.0, 50.0] { - // A table is bound to one feerate, so it belongs inside this loop. - let table = table_at(txs.clone(), spends.clone(), feerate); + let cluster = cluster(txs.clone(), spends.clone()); let mut candidates = candidates(2, 200_000); candidates[1].value = 50_000; @@ -300,7 +271,7 @@ fn excess_and_implied_fee_agree() { for selection in [vec![], vec![0], vec![1], vec![0, 1]] { let mut cs = - CoinSelector::new(&candidates, target).with_bump_table(&table); + CoinSelector::new(&candidates, target).with_cluster(&cluster); for i in &selection { cs.select(*i); } @@ -328,32 +299,27 @@ fn excess_and_implied_fee_agree() { } } -/// `Candidate::ancestor_bump_fee` is `feerate * weight - fee_paid` over the unconfirmed ancestors, -/// so it only means anything at the feerate it was computed for — and `Candidate` has nowhere to -/// record which. Every path that reaches it through a selector carrying a table is checked; -/// `select_all_effective` is the one that takes a feerate of its own. +/// The bump is computed at the target feerate, so per-candidate figures asked at any other rate +/// would mix rates. The methods that take a feerate of their own catch that. #[test] -#[should_panic(expected = "bump table was built for a different feerate")] +#[should_panic(expected = "the ancestor bump is computed at the target feerate")] fn selecting_all_effective_rejects_a_mismatched_feerate() { - let table = one_stuck_parent(10.0); + let cluster = one_stuck_parent(); let candidates = priced(1, 200_000); - let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); cs.select_all_effective(FeeRate::from_sat_per_vb(20.0)); } -/// The same figures at the table's own feerate are fine, and the bump is visible in the ranking. +/// The same figures at the target's own feerate are fine, and the bump is visible in the ranking. #[test] -fn selecting_all_effective_works_at_the_table_feerate() { - let table = one_stuck_parent(10.0); +fn selecting_all_effective_works_at_the_target_feerate() { + let cluster = one_stuck_parent(); let candidates = priced(1, 200_000); - let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_bump_table(&table); + let mut cs = CoinSelector::new(&candidates, simple_target(10.0)).with_cluster(&cluster); cs.select_all_effective(FeeRate::from_sat_per_vb(10.0)); assert!(cs.is_selected(0), "still worth its bump at 200_000 sats"); - assert_eq!( - cs.selected_ancestor_bump_fee(FeeRate::from_sat_per_vb(10.0)), - 990 - ); + assert_eq!(cs.selected_ancestor_bump_fee(), 990); } diff --git a/tests/cpfp_cluster.rs b/tests/cpfp_cluster.rs index d24e01e..064305d 100644 --- a/tests/cpfp_cluster.rs +++ b/tests/cpfp_cluster.rs @@ -1,6 +1,6 @@ use bdk_coin_select::{ - BumpTable, Candidate, Cluster, ClusterError, CoinSelector, FeeRate, MempoolTx, Target, - TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, + Candidate, Cluster, ClusterError, CoinSelector, FeeRate, MempoolTx, Target, TargetFee, + TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, }; const RATE: f32 = 10.0; @@ -41,14 +41,14 @@ fn tx(weight: u64, fee: u64, parents: Vec) -> MempoolTx { } } -/// The bump a selection of `selection` owes under `table`. -fn bump(table: &BumpTable, n_candidates: usize, selection: &[usize]) -> u64 { +/// The bump a selection of `selection` owes under `cluster`, at the fixture target's feerate. +fn bump(cluster: &Cluster, n_candidates: usize, selection: &[usize]) -> u64 { let candidates = candidates(n_candidates); - let mut cs = CoinSelector::new(&candidates, target()).with_bump_table(table); + let mut cs = CoinSelector::new(&candidates, target()).with_cluster(cluster); for &i in selection { cs.select(i); } - cs.selected_ancestor_bump_fee(rate()) + cs.selected_ancestor_bump_fee() } /// A deficient parent with nothing to carry it has to be bumped, in full. @@ -56,9 +56,8 @@ fn bump(table: &BumpTable, n_candidates: usize, selection: &[usize]) -> u64 { fn a_deficient_ancestor_alone_is_charged_in_full() { // 1000 wu = 250 vB. At 10 sat/vB it owes 2500 but paid 500. let cluster = Cluster::new(vec![tx(1_000, 500, vec![])], vec![(0, 0)]).unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); - assert_eq!(bump(&table, 1, &[0]), 2_000); + assert_eq!(bump(&cluster, 1, &[0]), 2_000); } /// A deficient parent already carried by an overpaying child that we are *not* spending. A miner @@ -77,9 +76,8 @@ fn a_parent_carried_by_someone_elses_child_needs_no_bump() { .unwrap(); // Package {0,1} is 350 vB paying 4500 against 3500 owed, so a miner takes both. - let from_cluster = BumpTable::from_cluster(&cluster, rate()); assert_eq!( - bump(&from_cluster, 1, &[0]), + bump(&cluster, 1, &[0]), 0, "the child already pays for the parent" ); @@ -98,10 +96,9 @@ fn transitive_ancestors_are_pulled_in_automatically() { vec![(0, 2)], ) .unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); // All three are unmined: 1200 wu = 300 vB, owes 3000, paid 0. - assert_eq!(bump(&table, 1, &[0]), 3_000); + assert_eq!(bump(&cluster, 1, &[0]), 3_000); } /// A transaction shared by two selected candidates is paid for once. @@ -116,13 +113,12 @@ fn a_shared_ancestor_is_charged_once() { vec![(0, 1), (1, 2)], ) .unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); // One candidate: parent + its own tx = 800 wu = 200 vB => 2000. - assert_eq!(bump(&table, 2, &[0]), 2_000); - assert_eq!(bump(&table, 2, &[1]), 2_000); + assert_eq!(bump(&cluster, 2, &[0]), 2_000); + assert_eq!(bump(&cluster, 2, &[1]), 2_000); // Both: parent counted once => 1200 wu = 300 vB => 3000, not 4000. - assert_eq!(bump(&table, 2, &[0, 1]), 3_000); + assert_eq!(bump(&cluster, 2, &[0, 1]), 3_000); } /// A cluster already paying above the target is mined entirely, so nothing is owed. @@ -133,10 +129,13 @@ fn a_cluster_above_the_target_owes_nothing() { vec![(0, 1)], ) .unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); - assert_eq!(bump(&table, 1, &[0]), 0); - assert_eq!(bump(&table, 1, &[]), 0, "the empty subset always owes zero"); + assert_eq!(bump(&cluster, 1, &[0]), 0); + assert_eq!( + bump(&cluster, 1, &[]), + 0, + "the empty subset always owes zero" + ); } /// Mining is package-wise, so a deficient parent is carried by an overpaying descendant we *are* @@ -151,9 +150,8 @@ fn an_overpaying_descendant_carries_its_deficient_parent() { vec![(0, 1)], // we spend the child ) .unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); - assert_eq!(bump(&table, 1, &[0]), 0); + assert_eq!(bump(&cluster, 1, &[0]), 0); } #[test] @@ -184,13 +182,12 @@ fn selecting_a_bump_neutral_candidate_does_not_move_the_price() { vec![(0, 0), (1, 1)], ) .unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); // Neutral candidate alone, and added on top of the stuck one: neither changes anything. - assert_eq!(bump(&table, 2, &[]), 0); - assert_eq!(bump(&table, 2, &[0]), 0); - assert_eq!(bump(&table, 2, &[1]), 990); - assert_eq!(bump(&table, 2, &[0, 1]), 990); + assert_eq!(bump(&cluster, 2, &[]), 0); + assert_eq!(bump(&cluster, 2, &[0]), 0); + assert_eq!(bump(&cluster, 2, &[1]), 990); + assert_eq!(bump(&cluster, 2, &[0, 1]), 990); } /// **The inequality the whole design rests on.** Branch and bound searches on the sum of @@ -232,15 +229,14 @@ fn the_local_sum_never_undercuts_the_package_for_any_cluster() { .collect(); let cluster = Cluster::new(txs, spends).expect("acyclic by construction"); - let table = BumpTable::from_cluster(&cluster, rate()); let cands = candidates(n_candidates); - let cs = CoinSelector::new(&cands, target()).with_bump_table(&table); + let cs = CoinSelector::new(&cands, target()).with_cluster(&cluster); for mask in 0..(1_u32 << n_candidates) { let selection: Vec = (0..n_candidates).filter(|b| mask & (1 << b) != 0).collect(); let local: u64 = selection.iter().map(|&i| cs.ancestor_bump_fee_of(i)).sum(); - let combined = bump(&table, n_candidates, &selection); + let combined = bump(&cluster, n_candidates, &selection); assert!( local >= combined, "trial {}: local sum {} undercuts the package {} for selection {:?}", @@ -269,7 +265,6 @@ fn branch_and_bound_selections_are_funded_when_priced_exactly() { vec![(0, 0), (1, 1), (2, 2)], ) .unwrap(); - let table = BumpTable::from_cluster(&cluster, rate()); let cands = candidates(4); let target = Target { @@ -287,7 +282,7 @@ fn branch_and_bound_selections_are_funded_when_priced_exactly() { drain_weights: DrainWeights::TR_KEYSPEND, }; - let mut cs = CoinSelector::new(&cands, target).with_bump_table(&table); + let mut cs = CoinSelector::new(&cands, target).with_cluster(&cluster); let (_, drain) = cs.run_bnb(metric, 100_000).expect("a solution exists"); assert!( From 3f1f8f5c466f4f287cc94992954acdf84e9839ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 4 Aug 2026 07:51:11 +0000 Subject: [PATCH 4/5] Build clusters through an id-keyed `ClusterBuilder` `Cluster::new` took transactions with parent edges expressed as indices into the very `Vec` being assembled, and candidate spends as index pairs. The caller had to invent a dense numbering for transactions they already key by txid, could not append a parent after its child without renumbering, and got errors back as positions. `ClusterBuilder` takes the graph in the caller's own vocabulary instead: let mut builder = ClusterBuilder::new(); builder.tx(txid_a, 1_000, 500, []); builder.tx(txid_b, 400, 0, [txid_a]); builder.spent_by(txid_b, 3); // candidate 3 spends an output of b let cluster = builder.build()?; `Id` is generic rather than a txid type, so the crate stays dependency-free -- the caller uses whatever they already have and the ids are resolved to internal indices once, in `build`. Insertion order stops mattering (parents are recorded as ids and resolved at the end), and `ClusterError` reports problems by the caller's ids, including a `DuplicateTx` case the index API could not even express as a mistake. The candidate side deliberately stays an index: candidates are indices everywhere else in this crate, and giving them a second identity here would be inconsistent rather than convenient. `MempoolTx` and the index-based constructor drop out of the public API; the builder is the only way in. Anyone who genuinely has indices instantiates `ClusterBuilder` and loses nothing. Co-Authored-By: Claude Fable 5 --- src/ancestor_search_experiment.rs | 47 +++--- src/mempool.rs | 252 ++++++++++++++++++++---------- tests/ancestor_aware.rs | 28 ++-- tests/cpfp_cluster.rs | 75 ++++++--- 4 files changed, 268 insertions(+), 134 deletions(-) diff --git a/src/ancestor_search_experiment.rs b/src/ancestor_search_experiment.rs index aef3ec1..2b04f86 100644 --- a/src/ancestor_search_experiment.rs +++ b/src/ancestor_search_experiment.rs @@ -40,8 +40,8 @@ //! [`CoinSelector::effective_value_of`]: crate::CoinSelector::effective_value_of use crate::{ - float::Ordf32, metrics::LowestFee, BnbMetric, Candidate, Cluster, CoinSelector, DrainWeights, - FeeRate, MempoolTx, Target, TargetFee, TargetOutputs, + float::Ordf32, metrics::LowestFee, BnbMetric, Candidate, Cluster, ClusterBuilder, CoinSelector, + DrainWeights, FeeRate, Target, TargetFee, TargetOutputs, }; use alloc::vec::Vec; @@ -89,26 +89,24 @@ fn instance(rng: &mut Rng, n: usize, k: usize, p_fine_pct: u64) -> Option>(); - let mut txs: Vec = Vec::new(); + // (weight, fee, parent positions); position doubles as the builder id. + let mut txs: Vec<(u64, u64, Vec)> = Vec::new(); let mut spends: Vec<(usize, usize)> = Vec::new(); - let push_tx = |txs: &mut Vec, rng: &mut Rng, parents: Vec| -> usize { - let weight = rng.in_range(400, 1_600) / 4 * 4; - // Fee as a multiple of what the cs.target() feerate would require: "fine" parents pay 1.0-3.0x - // and get mined, "stuck" ones pay 0.05-0.8x and need bumping. - let mult_pct = if rng.in_range(0, 100) < p_fine_pct { - rng.in_range(100, 300) - } else { - rng.in_range(5, 80) + let push_tx = + |txs: &mut Vec<(u64, u64, Vec)>, rng: &mut Rng, parents: Vec| -> usize { + let weight = rng.in_range(400, 1_600) / 4 * 4; + // Fee as a multiple of what the cs.target() feerate would require: "fine" parents pay 1.0-3.0x + // and get mined, "stuck" ones pay 0.05-0.8x and need bumping. + let mult_pct = if rng.in_range(0, 100) < p_fine_pct { + rng.in_range(100, 300) + } else { + rng.in_range(5, 80) + }; + let fee = (weight / 4) * 10 * mult_pct / 100; + txs.push((weight, fee, parents)); + txs.len() - 1 }; - let fee = (weight / 4) * 10 * mult_pct / 100; - txs.push(MempoolTx { - weight, - fee, - parents, - }); - txs.len() - 1 - }; for c in 0..k { let tx = match c { @@ -125,7 +123,16 @@ fn instance(rng: &mut Rng, n: usize, k: usize, p_fine_pct: u64) -> Option, +} + +/// Builds a [`Cluster`] from transactions keyed by the caller's own ids. +/// +/// `Id` is whatever the caller already keys transactions by — a txid, a `[u8; 32]`, anything +/// `Ord + Clone`. This crate deliberately has no `bitcoin` dependency, so it never names a txid +/// type; it just resolves the ids to internal indices once, in [`build`](Self::build). +/// Transactions may be added in any order: a child may name a parent that has not been added yet, +/// as long as it is there by `build` time. +/// +/// ``` +/// # use bdk_coin_select::ClusterBuilder; +/// let mut builder = ClusterBuilder::new(); +/// builder.tx("a", 1_000, 500, []); // id, weight (wu), fee paid (sats), parents +/// builder.tx("b", 400, 0, ["a"]); +/// builder.spent_by("b", 3); // candidate 3 spends an output of "b" +/// let cluster = builder.build().expect("well-formed"); +/// ``` +#[derive(Debug, Clone)] +pub struct ClusterBuilder { + /// (id, weight, fee, parent ids), in insertion order. + txs: Vec<(Id, u64, u64, Vec)>, + /// (candidate index, tx id). + spends: Vec<(usize, Id)>, +} + +impl Default for ClusterBuilder { + fn default() -> Self { + Self { + txs: Vec::new(), + spends: Vec::new(), + } + } +} + +impl ClusterBuilder { + /// A builder with no transactions. Building it yields an empty cluster, which prices every + /// candidate as ancestor-free. + pub fn new() -> Self { + Self::default() + } + + /// Record an unconfirmed transaction: its `weight` in weight units, the `fee` it already pays + /// in satoshis, and the ids of its *direct* in-cluster parents — transitive ancestors are + /// derived, which is much of the point of supplying a graph rather than a list. Parents + /// need not have been added yet. + pub fn tx(&mut self, id: Id, weight: u64, fee: u64, parents: impl IntoIterator) { + self.txs + .push((id, weight, fee, parents.into_iter().collect())); + } + + /// Record that the candidate at `candidate_index` (into the slice given to + /// [`CoinSelector::new`]) spends an output of the transaction `id`. /// - /// Only *direct* parents: the transitive closure is computed for you, which is much of the - /// point of supplying a graph rather than a list. - pub parents: Vec, + /// Both directions are many: a transaction may be spent by several candidates, and a candidate + /// may appear more than once when it spends outputs of several transactions — its package is + /// then the union of their ancestor closures. + /// + /// [`CoinSelector::new`]: crate::CoinSelector::new + pub fn spent_by(&mut self, id: Id, candidate_index: usize) { + self.spends.push((candidate_index, id)); + } + + /// Resolve ids and compute ancestor closures. + /// + /// # Errors + /// + /// [`ClusterError`], naming the offending ids: a duplicated transaction, a parent or spent + /// transaction that was never added, or a cycle in the parent relation (real mempool graphs + /// are acyclic; an id scheme that cycles is a caller bug). + pub fn build(self) -> Result> { + let mut index_of = BTreeMap::new(); + for (index, (id, _, _, _)) in self.txs.iter().enumerate() { + if index_of.insert(id.clone(), index).is_some() { + return Err(ClusterError::DuplicateTx { tx: id.clone() }); + } + } + + let txs = + self.txs + .iter() + .map(|(id, weight, fee, parents)| { + let parents = parents + .iter() + .map(|parent| { + index_of.get(parent).copied().ok_or_else(|| { + ClusterError::UnknownParent { + child: id.clone(), + parent: parent.clone(), + } + }) + }) + .collect::, _>>()?; + Ok(MempoolTx { + weight: *weight, + fee: *fee, + parents, + }) + }) + .collect::, ClusterError>>()?; + + let candidate_spends = self + .spends + .iter() + .map(|(candidate, id)| { + let tx = index_of + .get(id) + .copied() + .ok_or_else(|| ClusterError::UnknownSpend { + candidate: *candidate, + tx: id.clone(), + })?; + Ok((*candidate, tx)) + }) + .collect::, ClusterError>>()?; + + let closures = ancestor_closures(&txs).map_err(|index| ClusterError::Cycle { + tx: self.txs[index].0.clone(), + })?; + + Ok(Cluster { + txs, + candidate_spends, + closures, + }) + } } /// A connected piece of the mempool: the unconfirmed transactions relevant to a selection, and -/// which candidates spend from which. +/// which candidates spend from which. Built with [`ClusterBuilder`]. /// /// This is the input to [`CoinSelector::with_cluster`]. Supplying the graph rather than a flat /// ancestor list buys three things a list cannot express: @@ -31,11 +153,6 @@ pub struct MempoolTx { /// what the package owes together. Mining guarantees that; pooling a flat list does not, because /// a shared ancestor that *overpays* has its surplus counted once per dependent. /// -/// If all you have is a flat list of ancestors, express it here: each ancestor becomes a -/// transaction with no parents, and each (ancestor, candidate) pair becomes an entry in -/// `candidate_spends`. You then get the mining step for free. -/// -/// /// # Completeness /// /// Include the descendants and siblings of your ancestors where you know them — a parent already @@ -56,96 +173,64 @@ pub struct Cluster { closures: Vec, } -/// Error returned when a [`Cluster`] cannot be built. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum ClusterError { - /// A `parents` entry does not refer to a transaction in the cluster. - ParentOutOfBounds { - /// The transaction naming the bad parent. - tx: usize, - /// The out-of-bounds parent index. - parent: usize, +/// Error returned by [`ClusterBuilder::build`], naming the offending transactions by the caller's +/// own ids. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum ClusterError { + /// The same transaction id was added twice. + DuplicateTx { + /// The duplicated id. + tx: Id, }, - /// A `candidate_spends` entry does not refer to a transaction in the cluster. - SpendOutOfBounds { + /// A transaction names a parent that was never added. + UnknownParent { + /// The transaction naming the missing parent. + child: Id, + /// The missing parent. + parent: Id, + }, + /// A candidate spends a transaction that was never added. + UnknownSpend { /// The candidate index. candidate: usize, - /// The out-of-bounds transaction index. - tx: usize, + /// The missing transaction. + tx: Id, }, /// The parent relation contains a cycle, so the transactions cannot all be ancestors of each /// other. Real mempool clusters are acyclic. Cycle { /// A transaction on the cycle. - tx: usize, + tx: Id, }, } -impl core::fmt::Display for ClusterError { +impl core::fmt::Display for ClusterError { fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { match self { - ClusterError::ParentOutOfBounds { tx, parent } => write!( + ClusterError::DuplicateTx { tx } => { + write!(f, "transaction {:?} was added more than once", tx) + } + ClusterError::UnknownParent { child, parent } => write!( f, - "transaction {} names parent {}, which is not in the cluster", - tx, parent + "transaction {:?} names parent {:?}, which is not in the cluster", + child, parent ), - ClusterError::SpendOutOfBounds { candidate, tx } => write!( + ClusterError::UnknownSpend { candidate, tx } => write!( f, - "candidate {} spends transaction {}, which is not in the cluster", + "candidate {} spends transaction {:?}, which is not in the cluster", candidate, tx ), ClusterError::Cycle { tx } => { - write!(f, "the parent relation cycles through transaction {}", tx) + write!(f, "the parent relation cycles through transaction {:?}", tx) } } } } #[cfg(feature = "std")] -impl std::error::Error for ClusterError {} +impl std::error::Error for ClusterError {} impl Cluster { - /// Build a cluster from unconfirmed transactions and the candidates that spend them. - /// - /// `candidate_spends` pairs a candidate index (into the slice given to - /// [`CoinSelector::new`]) with the index of the transaction in `txs` whose output it spends. - /// Both directions are many: a transaction may be spent by several candidates, and a candidate - /// may appear more than once when it spends outputs of several transactions — its package is - /// then the union of their ancestor closures. - /// - /// # Errors - /// - /// [`ClusterError`] if an index is out of bounds or the parent relation cycles. - /// - /// [`CoinSelector::new`]: crate::CoinSelector::new - pub fn new( - txs: Vec, - candidate_spends: Vec<(usize, usize)>, - ) -> Result { - for (tx_index, tx) in txs.iter().enumerate() { - for &parent in &tx.parents { - if parent >= txs.len() { - return Err(ClusterError::ParentOutOfBounds { - tx: tx_index, - parent, - }); - } - } - } - for &(candidate, tx) in &candidate_spends { - if tx >= txs.len() { - return Err(ClusterError::SpendOutOfBounds { candidate, tx }); - } - } - - let closures = ancestor_closures(&txs)?; - Ok(Self { - txs, - candidate_spends, - closures, - }) - } - /// The candidates that spend from this cluster, ascending and deduplicated. pub fn candidates(&self) -> Vec { let mut out = self @@ -246,8 +331,9 @@ impl Cluster { } } -/// For each transaction, the set containing it and all its transitive ancestors. -fn ancestor_closures(txs: &[MempoolTx]) -> Result, ClusterError> { +/// For each transaction, the set containing it and all its transitive ancestors. `Err` carries the +/// index of a transaction on a cycle. +fn ancestor_closures(txs: &[MempoolTx]) -> Result, usize> { let n = txs.len(); let mut closures = Vec::with_capacity(n); for _ in 0..n { @@ -272,7 +358,7 @@ fn ancestor_closures(txs: &[MempoolTx]) -> Result, ClusterError> { let parent = txs[tx].parents[*next_parent]; *next_parent += 1; if on_stack.contains(parent) { - return Err(ClusterError::Cycle { tx: parent }); + return Err(parent); } if !done.contains(parent) { stack.push((parent, 0)); diff --git a/tests/ancestor_aware.rs b/tests/ancestor_aware.rs index d5c67cb..947daf0 100644 --- a/tests/ancestor_aware.rs +++ b/tests/ancestor_aware.rs @@ -1,6 +1,6 @@ use bdk_coin_select::{ - Candidate, Cluster, CoinSelector, Drain, DrainWeights, FeeRate, MempoolTx, Replace, Target, - TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, + Candidate, Cluster, ClusterBuilder, CoinSelector, Drain, DrainWeights, FeeRate, Replace, + Target, TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, }; fn simple_target(feerate: f32) -> Target { @@ -15,16 +15,22 @@ fn simple_target(feerate: f32) -> Target { } } -fn tx(weight: u64, fee: u64, parents: Vec) -> MempoolTx { - MempoolTx { - weight, - fee, - parents, - } +/// (weight, fee, parent positions) — fed to the builder with the position as the id. +type Tx = (u64, u64, Vec); + +fn tx(weight: u64, fee: u64, parents: Vec) -> Tx { + (weight, fee, parents) } -fn cluster(txs: Vec, spends: Vec<(usize, usize)>) -> Cluster { - Cluster::new(txs, spends).expect("well-formed") +fn cluster(txs: Vec, spends: Vec<(usize, usize)>) -> Cluster { + let mut builder = ClusterBuilder::new(); + for (id, (weight, fee, parents)) in txs.into_iter().enumerate() { + builder.tx(id, weight, fee, parents); + } + for (candidate, tx_id) in spends { + builder.spent_by(tx_id, candidate); + } + builder.build().expect("well-formed") } /// One transaction paying far too little, spent by candidate 0: 400 wu = 100 vB, so at 10 sat/vB @@ -230,7 +236,7 @@ fn ancestor_candidates_are_selectable() { fn excess_and_implied_fee_agree() { // (transactions, which candidate spends which) -- no ancestors, one stuck parent, and a stuck // parent for candidate 0 alongside an already-paying one for candidate 1. - type Fixture = (Vec, Vec<(usize, usize)>); + type Fixture = (Vec, Vec<(usize, usize)>); let clusters: [Fixture; 3] = [ (vec![], vec![]), (vec![tx(400, 10, vec![])], vec![(0, 0)]), diff --git a/tests/cpfp_cluster.rs b/tests/cpfp_cluster.rs index 064305d..7b5efc3 100644 --- a/tests/cpfp_cluster.rs +++ b/tests/cpfp_cluster.rs @@ -1,5 +1,5 @@ use bdk_coin_select::{ - Candidate, Cluster, ClusterError, CoinSelector, FeeRate, MempoolTx, Target, TargetFee, + Candidate, Cluster, ClusterBuilder, ClusterError, CoinSelector, FeeRate, Target, TargetFee, TargetOutputs, TR_KEYSPEND_TXIN_WEIGHT, }; @@ -33,12 +33,22 @@ fn target() -> Target { } } -fn tx(weight: u64, fee: u64, parents: Vec) -> MempoolTx { - MempoolTx { - weight, - fee, - parents, +/// (weight, fee, parent positions) — fed to the builder with the position as the id. +type Tx = (u64, u64, Vec); + +fn tx(weight: u64, fee: u64, parents: Vec) -> Tx { + (weight, fee, parents) +} + +fn try_cluster(txs: Vec, spends: Vec<(usize, usize)>) -> Result> { + let mut builder = ClusterBuilder::new(); + for (id, (weight, fee, parents)) in txs.into_iter().enumerate() { + builder.tx(id, weight, fee, parents); + } + for (candidate, tx_id) in spends { + builder.spent_by(tx_id, candidate); } + builder.build() } /// The bump a selection of `selection` owes under `cluster`, at the fixture target's feerate. @@ -55,7 +65,7 @@ fn bump(cluster: &Cluster, n_candidates: usize, selection: &[usize]) -> u64 { #[test] fn a_deficient_ancestor_alone_is_charged_in_full() { // 1000 wu = 250 vB. At 10 sat/vB it owes 2500 but paid 500. - let cluster = Cluster::new(vec![tx(1_000, 500, vec![])], vec![(0, 0)]).unwrap(); + let cluster = try_cluster(vec![tx(1_000, 500, vec![])], vec![(0, 0)]).unwrap(); assert_eq!(bump(&cluster, 1, &[0]), 2_000); } @@ -65,7 +75,7 @@ fn a_deficient_ancestor_alone_is_charged_in_full() { /// is nowhere in this candidate's ancestry. #[test] fn a_parent_carried_by_someone_elses_child_needs_no_bump() { - let cluster = Cluster::new( + let cluster = try_cluster( vec![ tx(1_000, 500, vec![]), // 0: parent, 250 vB, owes 2500, paid 500 tx(400, 4_000, vec![0]), // 1: its child, 100 vB, pays 4000 @@ -87,7 +97,7 @@ fn a_parent_carried_by_someone_elses_child_needs_no_bump() { /// grandparent without the caller having to say so. #[test] fn transitive_ancestors_are_pulled_in_automatically() { - let cluster = Cluster::new( + let cluster = try_cluster( vec![ tx(400, 0, vec![]), // 0: grandparent, 100 vB, pays nothing tx(400, 0, vec![0]), // 1: parent @@ -104,7 +114,7 @@ fn transitive_ancestors_are_pulled_in_automatically() { /// A transaction shared by two selected candidates is paid for once. #[test] fn a_shared_ancestor_is_charged_once() { - let cluster = Cluster::new( + let cluster = try_cluster( vec![ tx(400, 0, vec![]), // 0: shared parent tx(400, 0, vec![0]), // 1: spent by candidate 0 @@ -124,7 +134,7 @@ fn a_shared_ancestor_is_charged_once() { /// A cluster already paying above the target is mined entirely, so nothing is owed. #[test] fn a_cluster_above_the_target_owes_nothing() { - let cluster = Cluster::new( + let cluster = try_cluster( vec![tx(400, 10_000, vec![]), tx(400, 10_000, vec![0])], vec![(0, 1)], ) @@ -142,7 +152,7 @@ fn a_cluster_above_the_target_owes_nothing() { /// spending — the whole chain comes in together or not at all. #[test] fn an_overpaying_descendant_carries_its_deficient_parent() { - let cluster = Cluster::new( + let cluster = try_cluster( vec![ tx(1_000, 500, vec![]), // 250 vB, owes 2500, paid 500 tx(400, 4_000, vec![0]), // 100 vB, pays 4000 @@ -157,27 +167,52 @@ fn an_overpaying_descendant_carries_its_deficient_parent() { #[test] fn cluster_rejects_malformed_input() { assert_eq!( - Cluster::new(vec![tx(400, 0, vec![7])], vec![]).unwrap_err(), - ClusterError::ParentOutOfBounds { tx: 0, parent: 7 } + try_cluster(vec![tx(400, 0, vec![7])], vec![]).unwrap_err(), + ClusterError::UnknownParent { + child: 0, + parent: 7 + } ); assert_eq!( - Cluster::new(vec![tx(400, 0, vec![])], vec![(0, 3)]).unwrap_err(), - ClusterError::SpendOutOfBounds { + try_cluster(vec![tx(400, 0, vec![])], vec![(0, 3)]).unwrap_err(), + ClusterError::UnknownSpend { candidate: 0, tx: 3 } ); assert!(matches!( - Cluster::new(vec![tx(400, 0, vec![1]), tx(400, 0, vec![0])], vec![(0, 0)]), + try_cluster(vec![tx(400, 0, vec![1]), tx(400, 0, vec![0])], vec![(0, 0)]), Err(ClusterError::Cycle { .. }) )); + + let mut duplicated = ClusterBuilder::new(); + duplicated.tx("a", 400, 0, []); + duplicated.tx("a", 500, 0, []); + assert_eq!( + duplicated.build().unwrap_err(), + ClusterError::DuplicateTx { tx: "a" } + ); +} + +/// The builder is keyed by the caller's own ids — insertion order does not matter, a child may +/// name a parent that arrives later, and errors come back in the caller's vocabulary. +#[test] +fn builder_accepts_ids_in_any_order() { + let mut builder = ClusterBuilder::new(); + builder.tx("child", 400, 4_000, ["parent"]); // parent not added yet + builder.tx("parent", 1_000, 500, []); + builder.spent_by("child", 0); + let cluster = builder.build().expect("well-formed"); + + // Package {parent, child}: 350 vB paying 4500 against 3500 owed => mined, nothing to bump. + assert_eq!(bump(&cluster, 1, &[0]), 0); } /// Selecting a bump-neutral candidate really does leave the package price alone -- which is the /// property that makes leaving it unbanned safe. #[test] fn selecting_a_bump_neutral_candidate_does_not_move_the_price() { - let cluster = Cluster::new( + let cluster = try_cluster( vec![tx(400, 2_000, vec![]), tx(400, 10, vec![])], vec![(0, 0), (1, 1)], ) @@ -228,7 +263,7 @@ fn the_local_sum_never_undercuts_the_package_for_any_cluster() { .map(|c| (c, (next() % n_txs as u64) as usize)) .collect(); - let cluster = Cluster::new(txs, spends).expect("acyclic by construction"); + let cluster = try_cluster(txs, spends).expect("acyclic by construction"); let cands = candidates(n_candidates); let cs = CoinSelector::new(&cands, target()).with_cluster(&cluster); @@ -256,7 +291,7 @@ fn the_local_sum_never_undercuts_the_package_for_any_cluster() { fn branch_and_bound_selections_are_funded_when_priced_exactly() { use bdk_coin_select::{metrics::LowestFee, DrainWeights, Target, TargetFee, TargetOutputs}; - let cluster = Cluster::new( + let cluster = try_cluster( vec![ tx(1_000, 500, vec![]), // stuck parent, shared by candidates 0 and 1 tx(400, 10, vec![0]), // stuck child From 6bcbd26095e835668c81415387e141074ef078e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 4 Aug 2026 12:19:31 +0000 Subject: [PATCH 5/5] Treat ids outside the cluster as confirmed, and duplicate adds as no-ops `ClusterBuilder::build` errored on a parent edge or spend naming a transaction that was never added, and on adding the same id twice. All three cases have natural meanings under the rule the cluster already lives by -- membership *is* the definition of unconfirmed, exactly as mempool membership is for a node: - A parent never added is a confirmed output. Dropped, not an error, so the caller lists every input's prevout id unfiltered instead of checking each against the mempool first. - A spend of a transaction never added spends a confirmed output and drags in nothing. Same rule, so `spent_by` can also be called for every candidate prevout unconditionally. - Adding the same id again is a no-op (the first record wins), tracked by an id map maintained as transactions are added. Overlapping ancestry walks -- two candidates sharing a parent -- each add it without coordinating. `ClusterError` shrinks to the one case with no sensible meaning: a cycle in the parent relation. What this trades away is a tripwire: an underpaying parent whose id is mistyped is now silently priced as confirmed, under-pricing the package. That tripwire was already half-blind -- omitting the parent entirely under-priced identically, uncaught -- so the docs now state the real contract instead: unconfirmed ancestors must all be present (missing ones under-price; the unsafe direction), while descendants and siblings are best-effort (missing ones overpay; safe). The `Default` impl carries an `Id: Ord` bound because `BTreeMap::new` demanded it before Rust 1.66, and the MSRV is 1.54. Co-Authored-By: Claude Fable 5 --- src/mempool.rs | 147 +++++++++++++++++------------------------- tests/cpfp_cluster.rs | 70 +++++++++++++------- 2 files changed, 107 insertions(+), 110 deletions(-) diff --git a/src/mempool.rs b/src/mempool.rs index 569497f..9a344ab 100644 --- a/src/mempool.rs +++ b/src/mempool.rs @@ -29,16 +29,21 @@ pub(crate) struct MempoolTx { /// ``` #[derive(Debug, Clone)] pub struct ClusterBuilder { - /// (id, weight, fee, parent ids), in insertion order. + /// (id, weight, fee, parent ids), in first-insertion order. txs: Vec<(Id, u64, u64, Vec)>, + /// Id -> tx position in `txs`, maintained as transactions are added so a duplicate add can be + /// ignored on the spot. + index_of: BTreeMap, /// (candidate index, tx id). spends: Vec<(usize, Id)>, } -impl Default for ClusterBuilder { +// `Id: Ord` because `BTreeMap::new` demanded it before Rust 1.66, and our MSRV is 1.54. +impl Default for ClusterBuilder { fn default() -> Self { Self { txs: Vec::new(), + index_of: BTreeMap::new(), spends: Vec::new(), } } @@ -52,10 +57,21 @@ impl ClusterBuilder { } /// Record an unconfirmed transaction: its `weight` in weight units, the `fee` it already pays - /// in satoshis, and the ids of its *direct* in-cluster parents — transitive ancestors are - /// derived, which is much of the point of supplying a graph rather than a list. Parents - /// need not have been added yet. + /// in satoshis, and the ids of its *direct* parents — transitive ancestors are derived, which + /// is much of the point of supplying a graph rather than a list. + /// + /// List every input's prevout id, unfiltered: a parent never added to the builder is treated + /// as a confirmed output and dropped, so cluster membership alone decides what is unconfirmed + /// — the same way mempool membership does for a node. Insertion order is irrelevant. + /// + /// Adding the same id again is a no-op (the first record wins), so overlapping ancestry walks + /// — two candidates sharing a parent — can each add it without coordinating. pub fn tx(&mut self, id: Id, weight: u64, fee: u64, parents: impl IntoIterator) { + use alloc::collections::btree_map; + match self.index_of.entry(id.clone()) { + btree_map::Entry::Vacant(entry) => entry.insert(self.txs.len()), + btree_map::Entry::Occupied(_) => return, + }; self.txs .push((id, weight, fee, parents.into_iter().collect())); } @@ -63,6 +79,10 @@ impl ClusterBuilder { /// Record that the candidate at `candidate_index` (into the slice given to /// [`CoinSelector::new`]) spends an output of the transaction `id`. /// + /// As with parent edges, an `id` never added to the builder means a confirmed output is being + /// spent, and the call is a no-op — so this too can be called for every candidate's prevout, + /// unfiltered. + /// /// Both directions are many: a transaction may be spent by several candidates, and a candidate /// may appear more than once when it spends outputs of several transactions — its package is /// then the union of their ancestor closures. @@ -76,54 +96,31 @@ impl ClusterBuilder { /// /// # Errors /// - /// [`ClusterError`], naming the offending ids: a duplicated transaction, a parent or spent - /// transaction that was never added, or a cycle in the parent relation (real mempool graphs - /// are acyclic; an id scheme that cycles is a caller bug). + /// [`ClusterError::Cycle`] if the parent relation cycles. Real mempool graphs are acyclic, so + /// an id scheme that cycles is a caller bug. pub fn build(self) -> Result> { - let mut index_of = BTreeMap::new(); - for (index, (id, _, _, _)) in self.txs.iter().enumerate() { - if index_of.insert(id.clone(), index).is_some() { - return Err(ClusterError::DuplicateTx { tx: id.clone() }); - } - } - - let txs = - self.txs - .iter() - .map(|(id, weight, fee, parents)| { - let parents = parents - .iter() - .map(|parent| { - index_of.get(parent).copied().ok_or_else(|| { - ClusterError::UnknownParent { - child: id.clone(), - parent: parent.clone(), - } - }) - }) - .collect::, _>>()?; - Ok(MempoolTx { - weight: *weight, - fee: *fee, - parents, - }) - }) - .collect::, ClusterError>>()?; + // A parent never added to the builder is a confirmed output: cluster membership *is* the + // definition of unconfirmed, exactly as mempool membership is for a node. + let txs = self + .txs + .iter() + .map(|(_, weight, fee, parents)| MempoolTx { + weight: *weight, + fee: *fee, + parents: parents + .iter() + .filter_map(|parent| self.index_of.get(parent).copied()) + .collect(), + }) + .collect::>(); + // Same rule: spending a transaction that is not in the cluster is spending a confirmed + // output, which drags in nothing. let candidate_spends = self .spends .iter() - .map(|(candidate, id)| { - let tx = index_of - .get(id) - .copied() - .ok_or_else(|| ClusterError::UnknownSpend { - candidate: *candidate, - tx: id.clone(), - })?; - Ok((*candidate, tx)) - }) - .collect::, ClusterError>>()?; + .filter_map(|(candidate, id)| Some((*candidate, self.index_of.get(id).copied()?))) + .collect(); let closures = ancestor_closures(&txs).map_err(|index| ClusterError::Cycle { tx: self.txs[index].0.clone(), @@ -155,12 +152,18 @@ impl ClusterBuilder { /// /// # Completeness /// -/// Include the descendants and siblings of your ancestors where you know them — a parent already -/// being paid for by *another* child needs no bump, and only a transaction present in the cluster -/// can demonstrate that. Where you don't know them (a child belonging to someone else), the -/// package is priced as if it needs the bump: you overpay, the transaction still confirms. That is -/// the safe direction, and it is the reason this is an optimality limit rather than a correctness -/// one. +/// Membership in the cluster *is* the definition of unconfirmed: a transaction that was never +/// added is treated as confirmed wherever it is referenced, the same way a node treats anything +/// outside its mempool. That makes construction easy — list every prevout, unfiltered — but it +/// puts completeness on the caller, and the two directions are not symmetric: +/// +/// - **Unconfirmed ancestors must all be present.** An underpaying parent omitted from the +/// cluster is priced as confirmed, so the package is silently *under*-priced and the +/// transaction can fall short of the target feerate. This is the unsafe direction. +/// - **Descendants and siblings are best-effort.** A parent already being paid for by *another* +/// child needs no bump, and only a transaction present in the cluster can demonstrate that. +/// Where you don't know them (a child belonging to someone else), the package is priced as if +/// it needs the bump: you overpay, the transaction still confirms. Safe, merely suboptimal. /// /// [`CoinSelector::with_cluster`]: crate::CoinSelector::with_cluster /// [`CoinSelector::ancestor_bump_fee_of`]: crate::CoinSelector::ancestor_bump_fee_of @@ -173,29 +176,10 @@ pub struct Cluster { closures: Vec, } -/// Error returned by [`ClusterBuilder::build`], naming the offending transactions by the caller's -/// own ids. +/// Error returned by [`ClusterBuilder::build`], naming the offending transaction by the caller's +/// own id. #[derive(Debug, Clone, PartialEq, Eq)] pub enum ClusterError { - /// The same transaction id was added twice. - DuplicateTx { - /// The duplicated id. - tx: Id, - }, - /// A transaction names a parent that was never added. - UnknownParent { - /// The transaction naming the missing parent. - child: Id, - /// The missing parent. - parent: Id, - }, - /// A candidate spends a transaction that was never added. - UnknownSpend { - /// The candidate index. - candidate: usize, - /// The missing transaction. - tx: Id, - }, /// The parent relation contains a cycle, so the transactions cannot all be ancestors of each /// other. Real mempool clusters are acyclic. Cycle { @@ -207,19 +191,6 @@ pub enum ClusterError { impl core::fmt::Display for ClusterError { fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { match self { - ClusterError::DuplicateTx { tx } => { - write!(f, "transaction {:?} was added more than once", tx) - } - ClusterError::UnknownParent { child, parent } => write!( - f, - "transaction {:?} names parent {:?}, which is not in the cluster", - child, parent - ), - ClusterError::UnknownSpend { candidate, tx } => write!( - f, - "candidate {} spends transaction {:?}, which is not in the cluster", - candidate, tx - ), ClusterError::Cycle { tx } => { write!(f, "the parent relation cycles through transaction {:?}", tx) } diff --git a/tests/cpfp_cluster.rs b/tests/cpfp_cluster.rs index 7b5efc3..f6e6615 100644 --- a/tests/cpfp_cluster.rs +++ b/tests/cpfp_cluster.rs @@ -165,33 +165,59 @@ fn an_overpaying_descendant_carries_its_deficient_parent() { } #[test] -fn cluster_rejects_malformed_input() { - assert_eq!( - try_cluster(vec![tx(400, 0, vec![7])], vec![]).unwrap_err(), - ClusterError::UnknownParent { - child: 0, - parent: 7 - } - ); - assert_eq!( - try_cluster(vec![tx(400, 0, vec![])], vec![(0, 3)]).unwrap_err(), - ClusterError::UnknownSpend { - candidate: 0, - tx: 3 - } - ); +fn cluster_rejects_a_cycle() { assert!(matches!( try_cluster(vec![tx(400, 0, vec![1]), tx(400, 0, vec![0])], vec![(0, 0)]), Err(ClusterError::Cycle { .. }) )); +} - let mut duplicated = ClusterBuilder::new(); - duplicated.tx("a", 400, 0, []); - duplicated.tx("a", 500, 0, []); - assert_eq!( - duplicated.build().unwrap_err(), - ClusterError::DuplicateTx { tx: "a" } - ); +/// Bump owed by candidate 0 under a cluster assembled by `f`, at the fixture target's feerate. +fn cluster_bump(f: impl FnOnce(&mut ClusterBuilder<&'static str>)) -> u64 { + let mut builder = ClusterBuilder::new(); + f(&mut builder); + let cluster = builder.build().expect("acyclic"); + let candidates = candidates(1); + let mut cs = CoinSelector::new(&candidates, target()).with_cluster(&cluster); + cs.select(0); + cs.selected_ancestor_bump_fee() +} + +/// Cluster membership *is* the definition of unconfirmed. A parent edge or a spend pointing at a +/// transaction that was never added means a confirmed output — dropped, not an error — so the +/// caller can dump every prevout, unfiltered. +#[test] +fn ids_outside_the_cluster_are_confirmed() { + // Candidate 0's tx names a parent that is not in the cluster: priced as if parentless. + let with_unknown_parent = cluster_bump(|b| { + b.tx("stuck", 400, 10, ["confirmed-somewhere"]); + b.spent_by("stuck", 0); + }); + let without = cluster_bump(|b| { + b.tx("stuck", 400, 10, []); + b.spent_by("stuck", 0); + }); + assert_eq!(with_unknown_parent, without); + assert_eq!(with_unknown_parent, 990, "100 vB owing 1000, paid 10"); + + // A spend of a transaction not in the cluster drags in nothing at all. + let confirmed_spend = cluster_bump(|b| { + b.tx("unrelated", 400, 10, []); + b.spent_by("not-here", 0); + }); + assert_eq!(confirmed_spend, 0); +} + +/// Adding the same id twice is a no-op — the first record wins — so overlapping ancestry walks +/// need not coordinate. +#[test] +fn adding_a_tx_twice_is_a_no_op() { + let bump = cluster_bump(|b| { + b.tx("stuck", 400, 10, []); + b.tx("stuck", 40_000, 0, []); // ignored + b.spent_by("stuck", 0); + }); + assert_eq!(bump, 990, "the first record won"); } /// The builder is keyed by the caller's own ids — insertion order does not matter, a child may