From 57114a8e04b5b0b3ae65c720635f2be5b08bd0d6 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/7] 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 --- CHANGELOG.md | 2 +- README.md | 20 +-- benches/coin_selector.rs | 7 +- src/bnb.rs | 35 +++-- src/coin_selector.rs | 285 +++++++++++++++++++++----------------- src/metrics/changeless.rs | 24 ++-- src/metrics/lowest_fee.rs | 99 ++++++------- tests/bnb.rs | 73 +++++----- tests/changeless.rs | 6 +- tests/common.rs | 77 +++++----- tests/lowest_fee.rs | 56 ++++---- tests/srd.rs | 37 +++-- tests/weight.rs | 47 +++++-- 13 files changed, 393 insertions(+), 375 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e5daf0..3891a90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Unreleased -- **Breaking:** `BnbMetric`'s `score`, `bound`, and `drain` take the `target: Target` as a parameter, and `CoinSelector::run_bnb`/`bnb_solutions` gain a leading `target` argument. Consequently `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless` previously had to keep in sync with its inner metric, and aligns the metric API with the rest of `CoinSelector`, where `target` is always passed in. +- **Breaking:** `CoinSelector` now owns its `Target`. `CoinSelector::new(candidates, target)` takes it and `CoinSelector::target()` returns it, and it is fixed for the life of the selector. Every method that took a `target: Target` argument no longer does, including `excess`, `missing`, `rate_excess`, `implied_fee`, `is_funded`, `is_within_max_weight`, `drain`, `select_until_target_met`, `select_srd`, `run_bnb`, and `bnb_solutions`. The same goes for arguments that merely restated part of the target: `weight` and `implied_feerate` no longer take `TargetOutputs`, `fee` no longer takes `target_value`, and `effective_value` and `select_all_effective` no longer take a `FeeRate`. `BnbMetric`'s methods read the target from the `CoinSelector` they are given — they are now `fn score(&mut self, cs: &CoinSelector<'_>) -> Option`, `fn bound(&mut self, cs: &CoinSelector<'_>) -> Option` and `fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain` — so `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless` previously had to keep in sync with its inner metric. To measure a selection against a second target, use `CoinSelector::with_target(target)`, which copies the selection, the bans and the candidate order over to the new target; `CoinSelector::new` starts from an empty selection. - **Breaking:** `BnbMetric` metrics now decide the change output themselves. The trait gains a `drain(&mut self, cs) -> Drain` method; call it on a branch-and-bound solution (or the `LowestFee` metric directly) to get the change output the metric optimized against, instead of computing a separate `ChangePolicy`. - **Breaking:** `CoinSelector::run_bnb` now returns `(Ordf32, Drain)` instead of just `Ordf32`, handing back the change output the metric decided on for the winning selection. - **Breaking:** `LowestFee` no longer takes a `change_policy`. It now takes `dust_relay_feerate: FeeRate` and `drain_weights: DrainWeights`, and adds change only when doing so lowers the long-term fee and the change would not be dust. 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..48c7b2c 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, @@ -201,26 +198,36 @@ impl Eq for Branch<'_> {} /// A branch and bound metric where we minimize the [`Ordf32`] score. /// /// This is to be used as input for [`CoinSelector::run_bnb`] or [`CoinSelector::bnb_solutions`]. +/// +/// Every selection passed to these methods carries its own [`Target`](crate::Target), reachable via +/// [`CoinSelector::target`]. [`bound`] is only a valid lower bound on the [`score`] of the +/// descendants of `cs` because the search clones `cs` to build them, so every node of a single +/// search is guaranteed to have the same target. +/// +/// [`bound`]: BnbMetric::bound +/// [`score`]: BnbMetric::score pub trait BnbMetric { - /// Get the score of a given selection for `target`. + /// Get the score of the selection `cs` against [`cs.target()`](CoinSelector::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`. + /// Get the lower bound score, using a heuristic, against + /// [`cs.target()`](CoinSelector::target). /// /// This represents the best possible score of all descendant branches (according to the /// heuristic). /// /// 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. + /// The change output (a.k.a. drain) this metric decides on for the selection `cs` and + /// [`cs.target()`](CoinSelector::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..2866c3b 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -18,6 +18,7 @@ pub const CHANGE_LOWER: u64 = 50_000; #[derive(Debug, Clone)] pub struct CoinSelector<'a> { candidates: &'a [Candidate], + target: Target, selected: Bitset, banned: Bitset, candidate_order: Arc>, @@ -35,15 +36,57 @@ 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 is measured against + /// that one 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. + pub fn target(&self) -> Target { + self.target + } + + /// A copy of this selector — same selection, bans and candidate order — that funds `target` + /// instead. + /// + /// Use this to measure a selection against a second target, for example to check whether a fee + /// bump needs more inputs. + /// + /// ``` + /// # use bdk_coin_select::{Candidate, CoinSelector, FeeRate, Target, TargetFee, TargetOutputs}; + /// # let candidates = [Candidate::new_tr_keyspend(100_000), Candidate::new_tr_keyspend(100_000)]; + /// let target = Target { + /// outputs: TargetOutputs::fund_outputs([(46 * 4, 90_000)]), + /// fee: TargetFee::from_feerate(FeeRate::from_sat_per_vb(1.0)), + /// max_weight: None, + /// }; + /// let mut selector = CoinSelector::new(&candidates, target); + /// selector.select(0); + /// assert!(selector.is_funded()); + /// + /// // Would that same selection still fund the transaction at a much higher feerate? + /// let bumped = selector.with_target(Target { + /// fee: TargetFee::from_feerate(FeeRate::from_sat_per_vb(500.0)), + /// ..target + /// }); + /// assert_eq!(bumped.selected_indices(), selector.selected_indices()); + /// assert!(!bumped.is_funded(), "the bump needs another input"); + /// ``` + pub fn with_target(&self, target: Target) -> Self { + Self { + target, + ..self.clone() + } + } + /// Iterate over all the candidates in their currently sorted order. Each item has the original /// index with the candidate. pub fn candidates( @@ -115,8 +158,9 @@ impl<'a> CoinSelector<'a> { self.selected.contains(index) } - /// Whether the candidates can cover this `target`'s **value** (net of input fees) — i.e. whether - /// enough value is reachable for [`is_funded`] to hold. Respects [`ban`]ned candidates. + /// Whether the candidates can cover the [`target`](Self::target)'s **value** (net of input + /// fees) — i.e. whether enough value is reachable for [`is_funded`] to hold. Respects + /// [`ban`]ned candidates. /// /// Selecting *all* effective inputs maximizes the value available, so if that can't meet the /// target value, nothing can. Monotone, hence exact. @@ -128,10 +172,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(); + test.is_funded() } /// Returns true if no candidates have been selected. @@ -176,25 +220,26 @@ impl<'a> CoinSelector<'a> { /// /// If you don't have any drain outputs (only target outputs) just set drain_weights to /// [`DrainWeights::NONE`]. - pub fn weight(&self, target_ouputs: TargetOutputs, drain_weight: DrainWeights) -> u64 { + pub fn weight(&self, drain_weight: DrainWeights) -> u64 { TX_FIXED_FIELD_WEIGHT + self.input_weight() - + target_ouputs.output_weight_with_drain(drain_weight) + + self.target.outputs.output_weight_with_drain(drain_weight) } - /// How much the current selection overshoots the value needed to achieve `target`. + /// How much the current selection overshoots the value needed to achieve the + /// [`target`](Self::target). /// /// 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)) + /// this means the transaction will overpay for what it needs to reach the target. + 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); + pub fn missing(&self) -> u64 { + let excess = self.excess(Drain::NONE); if excess < 0 { excess.unsigned_abs() } else { @@ -202,56 +247,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(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 { + if let Some(replace) = self.target.fee.replace { replacement_excess_needed = - replace.min_fee_to_do_replacement_wu(self.weight(target.outputs, drain.weights)) + replace.min_fee_to_do_replacement_wu(self.weight(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 } @@ -260,67 +305,67 @@ impl<'a> CoinSelector<'a> { /// the `target`'s value and weight. It is essentially telling you what target feerate you currently have. /// /// Returns `None` if the feerate would be negative or infinity. - pub fn implied_feerate(&self, target_outputs: TargetOutputs, drain: Drain) -> Option { - let numerator = - self.selected_value() as i64 - target_outputs.value_sum as i64 - drain.value as i64; - let denom = self.weight(target_outputs, drain.weights); + pub fn implied_feerate(&self, drain: Drain) -> Option { + let numerator = self.selected_value() as i64 + - self.target.outputs.value_sum as i64 + - drain.value as i64; + let denom = self.weight(drain.weights); if numerator < 0 || denom == 0 { return None; } Some(FeeRate::from_sat_per_wu(numerator as f32 / denom as f32)) } - /// The fee the current selection and `drain_weight` should pay to satisfy `target_fee`. + /// The fee the current selection and `drain_weight` should pay to satisfy the + /// [`target`](Self::target)'s [`TargetFee`]. /// /// This compares the fee calculated from the target feerate with the fee calculated from the /// [`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(drain_weights)), ); } implied_fee } - fn implied_fee_from_feerate(&self, target: Target, drain_weights: DrainWeights) -> u64 { - target - .fee - .rate - .implied_fee(self.weight(target.outputs, drain_weights)) + fn implied_fee_from_feerate(&self, drain_weights: DrainWeights) -> u64 { + self.target.fee.rate.implied_fee(self.weight(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(drain_weights)) } /// The actual fee the selection would pay if it was used in a transaction that had /// `target_value` value for outputs and change output of `drain_value`. /// /// This can be negative when the selection is invalid (outputs are greater than inputs). - pub fn fee(&self, target_value: u64, drain_value: u64) -> i64 { - self.selected_value() as i64 - target_value as i64 - drain_value as i64 + pub fn fee(&self, drain_value: u64) -> i64 { + self.selected_value() as i64 - self.target.value() as i64 - drain_value as i64 } /// The value of the current selected inputs minus the fee needed to pay for the selected inputs - pub fn effective_value(&self, feerate: FeeRate) -> i64 { - self.selected_value() as i64 - (self.input_weight() as f32 * feerate.spwu()).ceil() as i64 + pub fn effective_value(&self) -> i64 { + self.selected_value() as i64 + - (self.input_weight() as f32 * self.target.fee.rate.spwu()).ceil() as i64 } // /// Waste sum of all selected inputs. - fn input_waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 { - self.input_weight() as f32 * (feerate.spwu() - long_term_feerate.spwu()) + fn input_waste(&self, long_term_feerate: FeeRate) -> f32 { + self.input_weight() as f32 * (self.target.fee.rate.spwu() - long_term_feerate.spwu()) } /// Sorts the candidates by the comparision function. @@ -381,29 +426,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(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 +507,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(drain_weights) <= max_weight, None => true, } } @@ -479,8 +519,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 +532,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 @@ -506,27 +546,21 @@ impl<'a> CoinSelector<'a> { } /// The value of the change output should have to drain the excess value while maintaining the - /// constraints of `target` and respecting `change_policy`. + /// constraints of the [`target`](Self::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 +580,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, @@ -556,15 +590,15 @@ impl<'a> CoinSelector<'a> { } } - /// Select all candidates with an *effective value* greater than 0 at the provided `feerate`. + /// Select all candidates with an *effective value* greater than 0 at the target's feerate. /// - /// 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) { + /// A candidate is effective if it provides more value than it takes to pay for at that feerate. + pub fn select_all_effective(&mut self) { 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.candidates[cand_index].effective_value(self.target.fee.rate) <= 0.0 { continue; } @@ -572,7 +606,7 @@ impl<'a> CoinSelector<'a> { } } - /// Select candidates until `target` has been met. + /// Select candidates until the [`target`](Self::target) has been met. /// /// # Errors /// @@ -580,14 +614,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(()) @@ -637,7 +670,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 +680,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 +717,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 +731,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 +743,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 +756,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..cade774 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -1,4 +1,4 @@ -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. @@ -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(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,11 +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 - <= max_weight + cs.weight(self.drain_weights) + min_input_weight <= max_weight }), }; if change_is_reachable && best_score_with_change < current_score { @@ -180,14 +173,12 @@ 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))?; + 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); @@ -208,12 +199,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 +217,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 +235,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,9 +252,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 - + scale.0 * to_resize.weight as f32 + if let Some(max_weight) = cs.target().max_weight { + if cs.weight(DrainWeights::NONE) as f32 + scale.0 * to_resize.weight as f32 > max_weight as f32 { return None; @@ -272,7 +263,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..ffbaadc 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,14 +72,11 @@ 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 - .map(|replace| { - replace - .min_fee_to_do_replacement(exp_selection.weight(target.outputs, drain.weights)) - }) + .map(|replace| replace.min_fee_to_do_replacement(exp_selection.weight(drain.weights))) .unwrap_or(0); assert!(selected_value - target_value - drain.value >= replace_fee); } @@ -87,8 +84,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,14 +109,11 @@ 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 - .map(|replace| { - replace - .min_fee_to_do_replacement(selection.weight(target.outputs, drain.weights)) - }) + .map(|replace| replace.min_fee_to_do_replacement(selection.weight(drain.weights))) .unwrap_or(0); assert!(selected_value - target_value - drain.value >= replace_fee); } @@ -148,7 +142,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 +151,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 +172,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 +184,7 @@ where cs, parent_has_change, lb_score, - cs.is_funded(target), + cs.is_funded(), descendant_cs, descendant_has_change, descendant_score, @@ -340,11 +334,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 +349,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 +378,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 +389,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 +397,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 +435,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 +452,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 }, { @@ -475,7 +462,7 @@ pub fn compare_against_benchmarks( }, { let mut all_effective_selected = cs.clone(); - all_effective_selected.select_all_effective(target.fee.rate); + all_effective_selected.select_all_effective(); all_effective_selected }, { @@ -485,7 +472,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 +488,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 +513,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 +533,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..92dc251 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,11 +126,11 @@ 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); + let needed_weight = probe.weight(drain_weights); // Cap just below that, so SRD trips the weight limit as it reaches `change_lower`. let capped = Target { @@ -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..3163fc8 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,16 +48,21 @@ 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!( - coin_selector.weight(target_ouputs, DrainWeights::NONE), + coin_selector.weight(DrainWeights::NONE), tx.weight().to_wu() ); assert_eq!( (coin_selector - .implied_feerate(target_ouputs, Drain::NONE) + .implied_feerate(Drain::NONE) .unwrap() .as_sat_vb() * 10.0) @@ -83,23 +90,27 @@ 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(); assert_eq!( - coin_selector.weight(target_ouputs, DrainWeights::NONE), + coin_selector.weight(DrainWeights::NONE), tx.weight().to_wu() ); assert_eq!( (coin_selector - .implied_feerate(target_ouputs, Drain::NONE) + .implied_feerate(Drain::NONE) .unwrap() .as_sat_vb() * 10.0) @@ -133,16 +144,21 @@ 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!( - coin_selector.weight(target_ouputs, DrainWeights::NONE), + coin_selector.weight(DrainWeights::NONE), orig_weight.to_wu() ); assert_eq!( (coin_selector - .implied_feerate(target_ouputs, Drain::NONE) + .implied_feerate(Drain::NONE) .unwrap() .as_sat_vb() * 10.0) @@ -191,11 +207,16 @@ 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!( - coin_selector.weight(target_ouputs, DrainWeights::NONE), + coin_selector.weight(DrainWeights::NONE), tx.weight().to_wu() ); } From 7d69b325cdf35a40bb25dbaff002e2d87b795528 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 12 Aug 2026 06:54:37 +0000 Subject: [PATCH 2/7] fix!: replace Candidate input_count/is_segwit with segwit_count/legacy_count Fixes CoinSelector::input_weight undercounting candidates that group multiple legacy inputs in a segwit transaction (where each legacy input serializes a 1 WU empty witness). Tracking segwit and legacy input counts separately also allows a single Candidate to mix legacy and segwit inputs. --- CHANGELOG.md | 1 + README.md | 23 +++--- benches/coin_selector.rs | 4 +- src/bnb.rs | 19 ++++- src/coin_selector.rs | 46 +++++++---- tests/bnb.rs | 19 ++--- tests/changeless.rs | 4 +- tests/common.rs | 15 +++- tests/lowest_fee.rs | 77 +++++++++++++++---- tests/srd.rs | 16 ++-- tests/weight.rs | 161 +++++++++++++++++++++++++++++++++++++-- 11 files changed, 307 insertions(+), 78 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3891a90..1606dc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # Unreleased +- **Breaking:** Replace `Candidate`'s `input_count` and `is_segwit` fields with `segwit_count` and `legacy_count`, fixing `CoinSelector::input_weight` undercounting candidates that group multiple inputs: in a segwit transaction every legacy input still serializes an empty witness (1 WU), which was previously paid once per candidate instead of once per legacy input, so a group of N legacy inputs came out N-1 WU short. Splitting the count by script type also means a single candidate may now mix legacy and segwit inputs and still be priced exactly. Branch and bound also stops treating candidates of equal value and weight as interchangeable when their segwit/legacy counts differ, which could make it skip a cheaper selection. - **Breaking:** `CoinSelector` now owns its `Target`. `CoinSelector::new(candidates, target)` takes it and `CoinSelector::target()` returns it, and it is fixed for the life of the selector. Every method that took a `target: Target` argument no longer does, including `excess`, `missing`, `rate_excess`, `implied_fee`, `is_funded`, `is_within_max_weight`, `drain`, `select_until_target_met`, `select_srd`, `run_bnb`, and `bnb_solutions`. The same goes for arguments that merely restated part of the target: `weight` and `implied_feerate` no longer take `TargetOutputs`, `fee` no longer takes `target_value`, and `effective_value` and `select_all_effective` no longer take a `FeeRate`. `BnbMetric`'s methods read the target from the `CoinSelector` they are given — they are now `fn score(&mut self, cs: &CoinSelector<'_>) -> Option`, `fn bound(&mut self, cs: &CoinSelector<'_>) -> Option` and `fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain` — so `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless` previously had to keep in sync with its inner metric. To measure a selection against a second target, use `CoinSelector::with_target(target)`, which copies the selection, the bans and the candidate order over to the new target; `CoinSelector::new` starts from an empty selection. - **Breaking:** `BnbMetric` metrics now decide the change output themselves. The trait gains a `drain(&mut self, cs) -> Drain` method; call it on a branch-and-bound solution (or the `LowestFee` metric directly) to get the change output the metric optimized against, instead of computing a separate `ChangePolicy`. - **Breaking:** `CoinSelector::run_bnb` now returns `(Ordf32, Drain)` instead of just `Ordf32`, handing back the change output the metric decided on for the winning selection. diff --git a/README.md b/README.md index 922dd25..8e723a1 100644 --- a/README.md +++ b/README.md @@ -33,23 +33,22 @@ let candidates = vec![ Candidate { // How many inputs does this candidate represents. Needed so we can // figure out the weight of the varint that encodes the number of inputs - input_count: 1, + // and whether segwit transaction fields need to be counted in. + segwit_count: 1, + legacy_count: 0, // the value of the input value: 1_000_000, // the total weight of the input(s) including their witness/scriptSig // you may need to use miniscript to figure out the correct value here. 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 }, Candidate { // A candidate can represent multiple inputs in the case where you // always want some inputs to be spent together. - input_count: 2, + segwit_count: 2, + legacy_count: 0, weight: 2*TR_KEYSPEND_TXIN_WEIGHT, value: 3_000_000, - is_segwit: true } ]; @@ -105,22 +104,22 @@ let outputs = vec![TxOut { let candidates = [ Candidate { - input_count: 1, + segwit_count: 1, + legacy_count: 0, value: 400_000, weight: TR_KEYSPEND_TXIN_WEIGHT, - is_segwit: true }, Candidate { - input_count: 1, + segwit_count: 1, + legacy_count: 0, value: 200_000, weight: TR_KEYSPEND_TXIN_WEIGHT, - is_segwit: true }, Candidate { - input_count: 1, + segwit_count: 1, + legacy_count: 0, value: 11_000, weight: TR_KEYSPEND_TXIN_WEIGHT, - is_segwit: true } ]; let drain_weights = bdk_coin_select::DrainWeights::default(); diff --git a/benches/coin_selector.rs b/benches/coin_selector.rs index 18e847e..c48420e 100644 --- a/benches/coin_selector.rs +++ b/benches/coin_selector.rs @@ -33,8 +33,8 @@ fn make_candidates(n: usize) -> Vec { Candidate { value, weight: TXIN_BASE_WEIGHT + P2WPKH_SAT_W, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, } }) .collect() diff --git a/src/bnb.rs b/src/bnb.rs index 48c7b2c..c82965a 100644 --- a/src/bnb.rs +++ b/src/bnb.rs @@ -137,12 +137,25 @@ 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, we keep banning if candidates have the same weight, value and + // input counts. The counts matter because a segwit and a legacy input of equal weight + // change the tx weight differently. 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, + next.segwit_count, + next.legacy_count, + ); for (next_index, next) in cs.unselected() { - if (next.value, next.weight) != to_ban { + if ( + next.value, + next.weight, + next.segwit_count, + next.legacy_count, + ) != to_ban + { break; } let (_index, _candidate) = exclusion_cs diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 2866c3b..f4090aa 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -186,20 +186,23 @@ impl<'a> CoinSelector<'a> { /// The weight of the inputs including the witness header and the varint for the number of /// inputs. pub fn input_weight(&self) -> u64 { - let is_segwit_tx = self.selected().any(|(_, wv)| wv.is_segwit); + let is_segwit_tx = self.selected().any(|(_, wv)| wv.segwit_count > 0); let witness_header_extra_weight = is_segwit_tx as u64 * 2; - let input_count = self.selected().map(|(_, wv)| wv.input_count).sum::(); + let input_count = self + .selected() + .map(|(_, wv)| wv.segwit_count + wv.legacy_count) + .sum::(); let input_varint_weight = varint_size(input_count) * 4; let selected_weight: u64 = self .selected() .map(|(_, candidate)| { let mut weight = candidate.weight; - if is_segwit_tx && !candidate.is_segwit { - // non-segwit candidates do not have the witness length field included in their - // weight field so we need to add 1 here if it's in a segwit tx. - weight += 1; + if is_segwit_tx { + // Legacy inputs do not have the witness length included in their weight field + // so we need to add 1 to each if it's a segwit tx. + weight += candidate.legacy_count as u64; } weight }) @@ -916,7 +919,11 @@ impl std::error::Error for NoBnbSolution {} /// A `Candidate` represents an input candidate for [`CoinSelector`]. /// -/// This can either be a single UTXO, or a group of UTXOs that should be spent together. +/// This can either be a single UTXO, or a group of UTXOs that should be spent together. A group +/// may mix legacy and segwit inputs; set [`legacy_count`] and [`segwit_count`] accordingly. +/// +/// [`legacy_count`]: Candidate::legacy_count +/// [`segwit_count`]: Candidate::segwit_count #[derive(Debug, Clone, Copy)] pub struct Candidate { /// Total value of the UTXO(s) that this [`Candidate`] represents. @@ -924,11 +931,24 @@ pub struct Candidate { /// Total weight of including this/these UTXO(s). /// `txin` fields: `prevout`, `nSequence`, `scriptSigLen`, `scriptSig`, `scriptWitnessLen`, /// `scriptWitness` should all be included. + /// + /// For legacy inputs, do *not* include the `scriptWitnessLen` byte: a legacy input only + /// serializes an (empty) witness when the transaction has a witness section, and + /// [`CoinSelector::input_weight`] adds that 1 WU per legacy input once any segwit input is + /// selected. pub weight: u64, - /// Total number of inputs; so we can calculate extra `varint` weight due to `vin` len changes. - pub input_count: usize, - /// Whether this [`Candidate`] contains at least one segwit spend. - pub is_segwit: bool, + /// Total number of segwit inputs. + /// + /// If any selected candidate has a non-zero `segwit_count`, the transaction serializes a + /// witness section (marker + flag, 2 WU) and every input — including legacy ones — pays for + /// a witness. + pub segwit_count: usize, + /// Total number of legacy (non-segwit) inputs. + /// + /// Each legacy input serializes an empty witness (1 WU) when the transaction has a witness + /// section; [`CoinSelector::input_weight`] prices this per legacy input, so grouped legacy + /// inputs are counted exactly. + pub legacy_count: usize, } impl Candidate { @@ -947,8 +967,8 @@ impl Candidate { Candidate { value, weight, - input_count: 1, - is_segwit, + segwit_count: is_segwit as usize, + legacy_count: !is_segwit as usize, } } diff --git a/tests/bnb.rs b/tests/bnb.rs index 55cf5e7..7a0b668 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -11,16 +11,16 @@ use proptest::{prelude::*, proptest, test_runner::*}; fn test_wv(mut rng: impl RngCore) -> impl Iterator { core::iter::repeat_with(move || { let value = rng.random_range(0..1_000); - let mut candidate = Candidate { + let candidate = Candidate { value, weight: 100, - input_count: rng.random_range(1..2), - is_segwit: rng.random_bool(0.5), + segwit_count: rng.random_range(1..2), + legacy_count: 0, }; - // HACK: set is_segwit = true for all these tests because you can't actually lower bound - // things easily with how segwit inputs interfere with their weights. We can't modify the - // above since that would change what we pull from rng. - candidate.is_segwit = true; + // Keep drawing the bool these tests always drew so the rng stream (and therefore the + // generated cases) is unchanged. All candidates are segwit: mixing in legacy inputs makes + // their weights context-dependent, which these tests can't lower-bound easily. + let _ = rng.random_bool(0.5); candidate }) } @@ -62,10 +62,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() { let num_additional_canidates = 12; let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha); - let mut wv = test_wv(&mut rng).map(|mut candidate| { - candidate.is_segwit = true; - candidate - }); + let mut wv = test_wv(&mut rng); let solution: Vec = (0..solution_len).map(|_| wv.next().unwrap()).collect(); let target_value = solution.iter().map(|c| c.value).sum(); diff --git a/tests/changeless.rs b/tests/changeless.rs index 4e9ca81..c92ee08 100644 --- a/tests/changeless.rs +++ b/tests/changeless.rs @@ -14,8 +14,8 @@ fn test_wv(mut rng: impl RngCore) -> impl Iterator { Candidate { value, weight: rng.random_range(0..100), - input_count: rng.random_range(1..2), - is_segwit: false, + segwit_count: rng.random_range(1..2), + legacy_count: 0, } }) } diff --git a/tests/common.rs b/tests/common.rs index ffbaadc..9864ca2 100644 --- a/tests/common.rs +++ b/tests/common.rs @@ -263,14 +263,21 @@ pub fn gen_candidates(n: usize) -> Vec { core::iter::repeat_with(move || { let value = rng.random_range(1..500_001); let weight = rng.random_range(1..2001); - let input_count = rng.random_range(1..3); - let is_segwit = rng.random_bool(0.01); + + let (mut legacy_count, mut segwit_count); + loop { + legacy_count = rng.random_range(0..3); + segwit_count = if rng.random_bool(0.01) { 1 } else { 0 }; + if legacy_count > 0 || segwit_count > 0 { + break; + } + } Candidate { value, weight, - input_count, - is_segwit, + segwit_count, + legacy_count, } }) .take(n) diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 1d7538b..2166692 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -1,5 +1,4 @@ #![allow(unused_imports)] - mod common; use bdk_coin_select::metrics::{Changeless, LowestFee}; use bdk_coin_select::{ @@ -85,8 +84,8 @@ proptest! { Candidate { value: 20_000, weight: (32 + 4 + 4 + 1) * 4 + 64 + 32, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }; params.n_candidates ]; @@ -236,21 +235,21 @@ fn does_not_create_change_below_spend_cost() { Candidate { value: 100_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, Candidate { value: 50_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, // NOTE: this input has negative effective value Candidate { value: 10, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, ]; @@ -314,14 +313,14 @@ fn zero_fee_tx() { Candidate { value: 100_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, Candidate { value: 50_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, ]; @@ -346,8 +345,8 @@ fn err_candidate(value: u64) -> Candidate { Candidate { value, weight: 272, // ~1 P2WPKH input - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, } } @@ -426,3 +425,51 @@ fn run_bnb_reports_round_limit() { }, ); } + +/// A segwit and a legacy candidate with the same value and weight are *not* interchangeable: the +/// segwit one adds the witness header to the tx, and the legacy one doesn't. So excluding one must +/// not also ban the other, or branch and bound misses the cheaper (legacy-only) selection. +#[test] +fn does_not_ban_candidates_that_differ_only_in_script_type() { + let target = Target { + fee: TargetFee::from_feerate(FeeRate::from_sat_per_vb(10.0)), + outputs: TargetOutputs { + value_sum: 50_000, + weight_sum: 200 - TX_FIXED_FIELD_WEIGHT - 1, + n_outputs: 1, + }, + max_weight: None, + }; + let candidates = vec![ + Candidate { + value: 100_000, + weight: 472, + segwit_count: 1, + legacy_count: 0, + }, + Candidate { + value: 100_000, + weight: 472, + segwit_count: 0, + legacy_count: 1, + }, + ]; + let metric = LowestFee { + long_term_feerate: FeeRate::from_sat_per_vb(10.0), + dust_relay_feerate: FeeRate::from_sat_per_vb(1.0), + drain_weights: DrainWeights::TR_KEYSPEND, + }; + + let mut exhaustive = CoinSelector::new(&candidates, target); + let (best_score, _) = + common::exhaustive_search(&mut exhaustive, &mut metric.clone()).expect("solvable"); + assert!( + exhaustive.is_selected(1) && !exhaustive.is_selected(0), + "legacy-only is the optimum: {}", + exhaustive + ); + + let mut cs = CoinSelector::new(&candidates, target); + let (score, _) = cs.run_bnb(metric, 100).expect("solvable"); + assert_eq!(score, best_score, "bnb selected {}", cs); +} diff --git a/tests/srd.rs b/tests/srd.rs index 92dc251..62514c4 100644 --- a/tests/srd.rs +++ b/tests/srd.rs @@ -72,20 +72,20 @@ fn srd_insufficient_funds() { Candidate { value: 50_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, Candidate { value: 50_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, Candidate { value: 50_000, weight: 100, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }, ]; let target = target(200_000, 5.0); @@ -114,8 +114,8 @@ fn srd_max_weight_exceeded() { Candidate { value: 100_000, weight: 1000, - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }; 10 ]; diff --git a/tests/weight.rs b/tests/weight.rs index 3163fc8..df4372c 100644 --- a/tests/weight.rs +++ b/tests/weight.rs @@ -23,6 +23,26 @@ pub fn hex_decode(hex: &str) -> Vec { bytes } +// https://mempool.space/tx/5f231df4f73694b3cca9211e336451c20dab136e0a843c2e3166cdcb093e91f4 +const THREE_INPUT_LEGACY_TX_HEX: &str = "0100000003fe785783e14669f638ba902c26e8e3d7036fb183237bc00f8a10542191c7171300000000fdfd00004730440220418996f20477d143d02ad47e74e5949641b6c2904159ab7c592d2cfc659f9bd802205b18f18ac86b714971f84a8b74a4cb14ad5c1a5b9d0d939bb32c6ae4032f4ea10148304502210091296ff8dd87b5ebfc3d47cb82cfe4750d52c544a2b88a85970354a4d0d4b1db022069632067ee6f30f06145f649bc76d5e5d5e6404dbe985e006fcde938f778c297014c695221030502b8ade694d57a6e86998180a64f4ce993372830dc796c3d561ad8b2a504de210272b68e1c037c4630eff7ea5858640cc0748e36f5de82fb38529ef1fd0a89670d2103ba0544a3a2aa9f2314022760b78b5c833aebf6f88468a089550f93834a2886ed53aeffffffff7e048a7c53a8af656e24442c65fe4c4299b1494f6c7579fe0fd9fa741ce83e3279000000fc004730440220018fa343acccd048ed8f8f179e1b6ae27435a41b5fb2c1d96a5a772777acc6dc022074783814f2100c6fc4d4c976f941212be50825814502ca0cbe3f929db789979e0147304402206373f01b73fb09876d0f5ee3087e0614cab3be249934bc2b7eb64ee67f53dc8302200b50f8a327020172b82aaba7480c77ecf07bb32322a05f4afbc543aa97d2fde8014c69522103039d906b2494e310f6c7774c98618be552720d04781e073dd3ff25d5906f22662103d82026baa529619b103ec6341d548a7eb6d924061a8469a7416155513a3071c12102e452bc4aa726d44646ba80db70465683b30efde282a19aa35c6029ae8925df5e53aeffffffffef80f0b1cc543de4f73d59c02a3c575ae5d0af17c1e11e6be7abe3325c777507ad000000fdfd00004730440220220fee11bf836621a11a8ea9100a4600c109c13895f11468d3e2062210c5481902201c5c8a462175538e87b8248e1ed3927c3a461c66d1b46215641c875e86eb22c4014830450221008d2de8c2f20a720129c372791e595b9602b1a9bce99618497aec5266148ffc1302203a493359d700ed96323f8805ed03e909959ff0f22eff359028db6861486b1555014c6952210374a4add33567f09967592c5bcdc3db421fdbba67bac4636328f96d941da31bd221039636c2ffac90afb7499b16e265078113dfb2d77b54270e37353217c9eaeaf3052103d0bcea6d10cdd2f16018ea71572631708e26f457f67cda36a7f816a87f7791d253aeffffffff04977261000000000016001470385d054721987f41521648d7b2f5c77f735d6bee92030000000000225120d0cda1b675a0b369964cbfa381721aae3549dd2c9c6f2cf71ff67d5bc277afd3f2aaf30000000000160014ed2d41ba08313dbb2630a7106b2fedafc14aa121d4f0c70000000000220020e5c7c00d174631d2d1e365d6347b016fb87b6a0c08902d8e443989cb771fa7ec00000000"; + +/// The 3-legacy-input mainnet tx above. +fn legacy_three_input_tx() -> Transaction { + Transaction::consensus_decode(&mut hex_decode(THREE_INPUT_LEGACY_TX_HEX).as_slice()).unwrap() +} + +/// The same tx with the middle input turned into a (semi-realistic) P2WPKH segwit spend. +fn legacy_three_input_tx_mixed() -> Transaction { + let mut tx = legacy_three_input_tx(); + tx.input[1].script_sig = ScriptBuf::default(); + tx.input[1].witness = vec![ + // semi-realistic p2wpkh spend + hex_decode("3045022100bdc115b86e9c863279132b4808459cf9b266c8f6a9c14a3dfd956986b807e3320220265833b85197679687c5d5eed1b2637489b34249d44cf5d2d40bc7b514181a5101"), + hex_decode("02077741a668889ce15d59365886375aea47a7691941d7a0d301697edbc773b45b"), + ].into(); + tx +} + #[test] fn segwit_one_input_one_output() { // FROM https://mempool.space/tx/e627fbb7f775a57fd398bf9b150655d4ac3e1f8afed4255e74ee10d7a345a9cc @@ -37,8 +57,8 @@ fn segwit_one_input_one_output() { .map(|(txin, value)| Candidate { value, weight: txin.segwit_weight().to_wu(), - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }) .collect::>(); @@ -85,8 +105,8 @@ fn segwit_two_inputs_one_output() { .map(|(txin, value)| Candidate { value, weight: txin.segwit_weight().to_wu(), - input_count: 1, - is_segwit: true, + segwit_count: 1, + legacy_count: 0, }) .collect::>(); @@ -133,8 +153,8 @@ fn legacy_three_inputs() { .map(|(txin, value)| Candidate { value, weight: txin.legacy_weight().to_wu(), - input_count: 1, - is_segwit: false, + segwit_count: 0, + legacy_count: 1, }) .collect::>(); @@ -195,8 +215,8 @@ fn legacy_three_inputs_one_segwit() { txin.legacy_weight() } .to_wu(), - input_count: 1, - is_segwit, + segwit_count: is_segwit as usize, + legacy_count: !is_segwit as usize, } }) .collect::>(); @@ -221,6 +241,131 @@ fn legacy_three_inputs_one_segwit() { ); } +#[test] +fn legacy_three_inputs_grouped() { + // Same tx as `legacy_three_inputs`, but all three legacy inputs carried by a single candidate. + // No witness section is serialized, so nothing is added per legacy input — this guards the + // all-legacy path (it also passed under the old per-candidate accounting). + let tx = legacy_three_input_tx(); + let input_values = [022_680_000, 006_558_175, 006_558_200]; + + let candidates = [Candidate { + value: input_values.iter().sum(), + weight: tx + .input + .iter() + .map(|txin| txin.legacy_weight().to_wu()) + .sum(), + segwit_count: 0, + legacy_count: tx.input.len(), + }]; + + 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 mut coin_selector = CoinSelector::new( + &candidates, + Target { + fee: TargetFee::ZERO, + outputs: target_ouputs, + max_weight: None, + }, + ); + coin_selector.select_all(); + + assert_eq!( + coin_selector.weight(DrainWeights::NONE), + tx.weight().to_wu() + ); +} + +#[test] +fn legacy_pair_grouped_with_segwit_input() { + // Same tx as `legacy_three_inputs_one_segwit`, but the two legacy inputs are grouped into a + // single candidate. In a segwit tx each legacy input still serializes an (empty) witness + // costing 1 WU, so the grouped candidate must pay 2 WU — not 1 — for its two empty witnesses. + let tx = legacy_three_input_tx_mixed(); + let input_values = [022_680_000, 006_558_175, 006_558_200]; + + let candidates = [ + Candidate { + value: input_values[0] + input_values[2], + weight: tx.input[0].legacy_weight().to_wu() + tx.input[2].legacy_weight().to_wu(), + segwit_count: 0, + legacy_count: 2, + }, + Candidate { + value: input_values[1], + weight: tx.input[1].segwit_weight().to_wu(), + segwit_count: 1, + legacy_count: 0, + }, + ]; + + 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 mut coin_selector = CoinSelector::new( + &candidates, + Target { + fee: TargetFee::ZERO, + outputs: target_ouputs, + max_weight: None, + }, + ); + coin_selector.select_all(); + + assert_eq!( + coin_selector.weight(DrainWeights::NONE), + tx.weight().to_wu() + ); +} + +#[test] +fn mixed_group_all_inputs_one_candidate() { + // Same tx as `legacy_three_inputs_one_segwit`, with all three inputs — legacy *and* segwit — + // in a single mixed candidate. `legacy_count`/`segwit_count` price this exactly: 2 WU for the + // two empty legacy witnesses, the segwit header once, and a 3-input varint. + let tx = legacy_three_input_tx_mixed(); + let input_values = [022_680_000, 006_558_175, 006_558_200]; + + let candidates = [Candidate { + value: input_values.iter().sum(), + weight: tx.input[0].legacy_weight().to_wu() + + tx.input[1].segwit_weight().to_wu() + + tx.input[2].legacy_weight().to_wu(), + segwit_count: 1, + legacy_count: 2, + }]; + + let target_outputs = 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 mut coin_selector = CoinSelector::new( + &candidates, + Target { + fee: TargetFee::ZERO, + outputs: target_outputs, + max_weight: None, + }, + ); + coin_selector.select_all(); + + assert_eq!( + coin_selector.weight(DrainWeights::NONE), + tx.weight().to_wu() + ); +} + #[test] fn new_tr_keyspend_correct_weight() { // FROM https://mempool.space/tx/4936a1a4ea1a0085b9dc2a1d5b59d361f5b1b41241772f3e465153712b6d8dc0 From e51ab233c51ae02ae3d33471521e67e8bf2c885f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Wed, 12 Aug 2026 07:24:24 +0000 Subject: [PATCH 3/7] refactor!: replace Candidate::new with Candidate::new_segwit and new_legacy Replaces the boolean is_segwit parameter in Candidate::new with explicit new_segwit and new_legacy constructors. Clarifies in doc comments that satisfaction_weight is the additional weight required beyond TXIN_BASE_WEIGHT (which already accounts for a 1-byte scriptSigLen). --- CHANGELOG.md | 2 +- src/coin_selector.rs | 36 +++++++++++++++++++++++++++++------- 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1606dc6..4f0d3fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,7 @@ # Unreleased +- **Breaking:** Replace `Candidate`'s `input_count` and `is_segwit` fields with `segwit_count` and `legacy_count`, fixing `CoinSelector::input_weight` undercounting candidates that group multiple inputs: in a segwit transaction every legacy input still serializes an empty witness (1 WU), which was previously paid once per candidate instead of once per legacy input, so a group of N legacy inputs came out N-1 WU short. Splitting the count by script type also means a single candidate may now mix legacy and segwit inputs and still be priced exactly. Branch and bound also stops treating candidates of equal value and weight as interchangeable when their segwit/legacy counts differ, which could make it skip a cheaper selection. Replaces `Candidate::new` with `Candidate::new_segwit` and `Candidate::new_legacy`. - **Breaking:** Replace `Candidate`'s `input_count` and `is_segwit` fields with `segwit_count` and `legacy_count`, fixing `CoinSelector::input_weight` undercounting candidates that group multiple inputs: in a segwit transaction every legacy input still serializes an empty witness (1 WU), which was previously paid once per candidate instead of once per legacy input, so a group of N legacy inputs came out N-1 WU short. Splitting the count by script type also means a single candidate may now mix legacy and segwit inputs and still be priced exactly. Branch and bound also stops treating candidates of equal value and weight as interchangeable when their segwit/legacy counts differ, which could make it skip a cheaper selection. -- **Breaking:** `CoinSelector` now owns its `Target`. `CoinSelector::new(candidates, target)` takes it and `CoinSelector::target()` returns it, and it is fixed for the life of the selector. Every method that took a `target: Target` argument no longer does, including `excess`, `missing`, `rate_excess`, `implied_fee`, `is_funded`, `is_within_max_weight`, `drain`, `select_until_target_met`, `select_srd`, `run_bnb`, and `bnb_solutions`. The same goes for arguments that merely restated part of the target: `weight` and `implied_feerate` no longer take `TargetOutputs`, `fee` no longer takes `target_value`, and `effective_value` and `select_all_effective` no longer take a `FeeRate`. `BnbMetric`'s methods read the target from the `CoinSelector` they are given — they are now `fn score(&mut self, cs: &CoinSelector<'_>) -> Option`, `fn bound(&mut self, cs: &CoinSelector<'_>) -> Option` and `fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain` — so `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless` previously had to keep in sync with its inner metric. To measure a selection against a second target, use `CoinSelector::with_target(target)`, which copies the selection, the bans and the candidate order over to the new target; `CoinSelector::new` starts from an empty selection. - **Breaking:** `BnbMetric` metrics now decide the change output themselves. The trait gains a `drain(&mut self, cs) -> Drain` method; call it on a branch-and-bound solution (or the `LowestFee` metric directly) to get the change output the metric optimized against, instead of computing a separate `ChangePolicy`. - **Breaking:** `CoinSelector::run_bnb` now returns `(Ordf32, Drain)` instead of just `Ordf32`, handing back the change output the metric decided on for the winning selection. - **Breaking:** `LowestFee` no longer takes a `change_policy`. It now takes `dust_relay_feerate: FeeRate` and `drain_weights: DrainWeights`, and adds change only when doing so lowers the long-term fee and the change would not be dust. diff --git a/src/coin_selector.rs b/src/coin_selector.rs index f4090aa..9f6eaf9 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -955,20 +955,42 @@ impl Candidate { /// Create a [`Candidate`] input that spends a single taproot keyspend output. pub fn new_tr_keyspend(value: u64) -> Self { let weight = TR_KEYSPEND_SATISFACTION_WEIGHT; - Self::new(value, weight, true) + Self::new_segwit(value, weight) } - /// Create a new [`Candidate`] that represents a single input. + /// Create a new [`Candidate`] that represents a single segwit input. /// - /// `satisfaction_weight` is the weight of `scriptSigLen + scriptSig + scriptWitnessLen + - /// scriptWitness`. - pub fn new(value: u64, satisfaction_weight: u64, is_segwit: bool) -> Candidate { + /// `satisfaction_weight` is the additional weight (in weight units) required to satisfy the input + /// beyond [`TXIN_BASE_WEIGHT`] (e.g. `scriptWitnessLen + scriptWitness` in WU at 1 WU/byte, plus + /// any `scriptSig` data and extra `scriptSigLen` varint bytes if nested/wrapped segwit). + /// + /// Note that [`TXIN_BASE_WEIGHT`] already accounts for the outpoint, `nSequence`, and 1 byte for + /// `scriptSigLen`. + pub fn new_segwit(value: u64, satisfaction_weight: u64) -> Candidate { + let weight = TXIN_BASE_WEIGHT + satisfaction_weight; + Candidate { + value, + weight, + segwit_count: 1, + legacy_count: 0, + } + } + + /// Create a new [`Candidate`] that represents a single legacy (non-segwit) input. + /// + /// `satisfaction_weight` is the additional weight (in weight units) required to satisfy the input + /// beyond [`TXIN_BASE_WEIGHT`] (e.g. `scriptSig` at 4 WU/byte, plus 4 WU per extra `scriptSigLen` + /// varint byte if `scriptSig` exceeds 252 bytes). + /// + /// Note that [`TXIN_BASE_WEIGHT`] already accounts for the outpoint, `nSequence`, and 1 byte for + /// `scriptSigLen`. + pub fn new_legacy(value: u64, satisfaction_weight: u64) -> Candidate { let weight = TXIN_BASE_WEIGHT + satisfaction_weight; Candidate { value, weight, - segwit_count: is_segwit as usize, - legacy_count: !is_segwit as usize, + segwit_count: 0, + legacy_count: 1, } } From 7e8a749793eafb14a582efafd9f0cb8a683c9def Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 22 Sep 2026 08:48:11 +0000 Subject: [PATCH 4/7] feat!: remove the Changeless metric `LowestFee` already decides for itself whether a selection should carry change, adding one only when it lowers the long-term fee, clears the dust threshold, and fits `Target::max_weight`. A separate changeless objective duplicates that decision and then constrains it. Callers that required a changeless transaction should use `LowestFee` and inspect the returned `Drain`. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 +- src/metrics.rs | 2 - src/metrics/changeless.rs | 79 ------------------- tests/changeless.proptest-regressions | 9 --- tests/changeless.rs | 104 -------------------------- tests/lowest_fee.rs | 43 +---------- 6 files changed, 3 insertions(+), 239 deletions(-) delete mode 100644 src/metrics/changeless.rs delete mode 100644 tests/changeless.proptest-regressions delete mode 100644 tests/changeless.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 4f0d3fa..b0e9e69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,14 +1,13 @@ # Unreleased - **Breaking:** Replace `Candidate`'s `input_count` and `is_segwit` fields with `segwit_count` and `legacy_count`, fixing `CoinSelector::input_weight` undercounting candidates that group multiple inputs: in a segwit transaction every legacy input still serializes an empty witness (1 WU), which was previously paid once per candidate instead of once per legacy input, so a group of N legacy inputs came out N-1 WU short. Splitting the count by script type also means a single candidate may now mix legacy and segwit inputs and still be priced exactly. Branch and bound also stops treating candidates of equal value and weight as interchangeable when their segwit/legacy counts differ, which could make it skip a cheaper selection. Replaces `Candidate::new` with `Candidate::new_segwit` and `Candidate::new_legacy`. -- **Breaking:** Replace `Candidate`'s `input_count` and `is_segwit` fields with `segwit_count` and `legacy_count`, fixing `CoinSelector::input_weight` undercounting candidates that group multiple inputs: in a segwit transaction every legacy input still serializes an empty witness (1 WU), which was previously paid once per candidate instead of once per legacy input, so a group of N legacy inputs came out N-1 WU short. Splitting the count by script type also means a single candidate may now mix legacy and segwit inputs and still be priced exactly. Branch and bound also stops treating candidates of equal value and weight as interchangeable when their segwit/legacy counts differ, which could make it skip a cheaper selection. +- **Breaking:** `CoinSelector` now owns its `Target`. `CoinSelector::new(candidates, target)` takes it and `CoinSelector::target()` returns it, and it is fixed for the life of the selector. Every method that took a `target: Target` argument no longer does, including `excess`, `missing`, `rate_excess`, `implied_fee`, `is_funded`, `is_within_max_weight`, `drain`, `select_until_target_met`, `select_srd`, `run_bnb`, and `bnb_solutions`. The same goes for arguments that merely restated part of the target: `weight` and `implied_feerate` no longer take `TargetOutputs`, `fee` no longer takes `target_value`, and `effective_value` and `select_all_effective` no longer take a `FeeRate`. `BnbMetric`'s methods read the target from the `CoinSelector` they are given — they are now `fn score(&mut self, cs: &CoinSelector<'_>) -> Option`, `fn bound(&mut self, cs: &CoinSelector<'_>) -> Option` and `fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain` — so `LowestFee` no longer stores a `target` field. To measure a selection against a second target, use `CoinSelector::with_target(target)`, which copies the selection, the bans and the candidate order over to the new target; `CoinSelector::new` starts from an empty selection. - **Breaking:** `BnbMetric` metrics now decide the change output themselves. The trait gains a `drain(&mut self, cs) -> Drain` method; call it on a branch-and-bound solution (or the `LowestFee` metric directly) to get the change output the metric optimized against, instead of computing a separate `ChangePolicy`. - **Breaking:** `CoinSelector::run_bnb` now returns `(Ordf32, Drain)` instead of just `Ordf32`, handing back the change output the metric decided on for the winning selection. - **Breaking:** `LowestFee` no longer takes a `change_policy`. It now takes `dust_relay_feerate: FeeRate` and `drain_weights: DrainWeights`, and adds change only when doing so lowers the long-term fee and the change would not be dust. - Add `DrainWeights::dust_threshold(dust_relay_feerate)`, the minimum value a change output with these weights must have to not be dust. - Add `CoinSelector::select_srd`, a Single Random Draw selector (port of Bitcoin Core's `SelectCoinsSRD`) that adds candidates in random order until the change reaches `change_lower`, producing a healthy-sized (privacy-friendly) change output instead of minimizing fees. Adds the `CHANGE_LOWER` constant for Core's value. -- **Breaking:** `Changeless` is now `Changeless`, wrapping an inner metric it constrains to changeless solutions (e.g. `Changeless`), replacing the previous tuple-composition approach. -- **Breaking:** Removed the `BnbMetric` tuple implementations (`impl BnbMetric for ((A, f32), ...)`). Weighted composition of independent metrics is no longer supported; the only composition still provided is the changeless constraint, now expressed as `Changeless`. If you relied on tuples to blend multiple objectives, there is no drop-in replacement. +- **Breaking:** Remove the `Changeless` metric and the `BnbMetric` tuple implementations (`impl BnbMetric for ((A, f32), ...)`). Generic metric composition is no longer supported. `LowestFee` decides for itself whether a selection should carry change (adding one only when it lowers the long-term fee, clears the dust threshold, and fits `Target::max_weight`), so a separate changeless objective duplicates that decision and then constrains it. Callers that required a changeless transaction should use `LowestFee` and inspect the returned `Drain`. - **Breaking:** `CoinSelector::selected_indices` and `CoinSelector::banned` now return `&Bitset` instead of `&BTreeSet`. `Bitset` exposes `contains`/`len`/`is_empty`/`iter` (#46) - Replace the internal `Cow`/`Cow<[usize]>` selection state with a `Bitset` and an `Arc`-shared candidate order, making the per-branch clones in branch-and-bound substantially cheaper (#46) - Fix compilation error when building with `--no-default-features` (#36) diff --git a/src/metrics.rs b/src/metrics.rs index 1da1163..2d841d0 100644 --- a/src/metrics.rs +++ b/src/metrics.rs @@ -5,5 +5,3 @@ //! [`CoinSelector::run_bnb`]: crate::CoinSelector::run_bnb mod lowest_fee; pub use lowest_fee::*; -mod changeless; -pub use changeless::*; diff --git a/src/metrics/changeless.rs b/src/metrics/changeless.rs deleted file mode 100644 index c2c9036..0000000 --- a/src/metrics/changeless.rs +++ /dev/null @@ -1,79 +0,0 @@ -use crate::{bnb::BnbMetric, float::Ordf32, CoinSelector, Drain}; - -/// Constrains an `inner` metric to only changeless solutions. -/// -/// A selection is scored by `inner` only if the inner metric decides it should *not* have a change -/// output (see [`BnbMetric::drain`]); otherwise it is treated as invalid. This lets you find, for -/// example, the lowest-fee changeless solution via `Changeless`. -#[derive(Clone, Copy, Debug)] -pub struct Changeless( - /// The inner metric that scores changeless solutions and owns the change decision. - pub M, -); - -impl Changeless { - /// Whether every selection reachable down this branch (the current one and any superset of it) - /// would have a change output according to the inner metric — so no changeless solution exists - /// here and the branch can be pruned. - /// - /// The inner metric only adds change once the excess is large enough (we assume its change - /// decision is monotone in the excess). So the reachable selection least likely to have change - /// is the one with the smallest excess — the current selection plus every remaining - /// negative-effective-value candidate, since each of those lowers the excess. If even that - /// selection still has change, then so does every reachable selection. - /// - /// 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. - /// - /// [`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() { - return false; - } - - let mut least_excess = cs.clone(); - cs.unselected() - .rev() - .take_while(|(_, wv)| wv.effective_value(cs.target().fee.rate) < 0.0) - .for_each(|(index, _)| { - least_excess.select(index); - }); - - self.0.drain(&least_excess).is_some() - } -} - -impl BnbMetric for Changeless { - 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<'_>) -> 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).is_some() { - return None; - } - self.0.score(cs) - } - - 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) - } - } - - fn requires_ordering_by_descending_value_pwu(&self) -> bool { - true - } -} diff --git a/tests/changeless.proptest-regressions b/tests/changeless.proptest-regressions deleted file mode 100644 index 97089e2..0000000 --- a/tests/changeless.proptest-regressions +++ /dev/null @@ -1,9 +0,0 @@ -# Seeds for failure cases proptest has generated in the past. It is -# automatically read and these particular cases re-run before any -# novel cases are generated. -# -# It is recommended to check this file in to source control so that -# everyone who runs the test benefits from these saved cases. -cc b03fc0267d15cf4455c7f00feed18d1ba82a783a38bf689dacdd572356013877 # shrinks to num_inputs = 7, target = 1277, feerate = 1.0, min_fee = 177, base_weight = 0, long_term_feerate_diff = 0.0, change_weight = 1, change_spend_weight = 1 -cc 2ba2cfa2412c0f3c9de4eb35caeefa1a086797f5c3f5fc0528396cc90a85d993 # shrinks to num_inputs = 5, target = 908, feerate = 7.823237, replace = None, base_weight = 222, long_term_feerate_diff = 2.4063222, change_weight = 14, change_spend_weight = 14 -cc fc2f2211d811690b78ca4206be874e5c3e99727626f79306bbdd25d59df9c27b # shrinks to num_inputs = 10, target = 3821, feerate = 4.104783, replace = None, base_weight = 321, long_term_feerate_diff = 2.7914581, change_weight = 1, change_spend_weight = 1 diff --git a/tests/changeless.rs b/tests/changeless.rs deleted file mode 100644 index c92ee08..0000000 --- a/tests/changeless.rs +++ /dev/null @@ -1,104 +0,0 @@ -#![allow(unused)] -mod common; -use bdk_coin_select::{ - float::Ordf32, - metrics::{Changeless, LowestFee}, - Candidate, CoinSelector, DrainWeights, FeeRate, Target, TargetFee, TargetOutputs, -}; -use proptest::{prelude::*, proptest, test_runner::*}; -use rand::{prelude::IteratorRandom, Rng, RngCore}; - -fn test_wv(mut rng: impl RngCore) -> impl Iterator { - core::iter::repeat_with(move || { - let value = rng.random_range(0..1_000); - Candidate { - value, - weight: rng.random_range(0..100), - segwit_count: rng.random_range(1..2), - legacy_count: 0, - } - }) -} - -proptest! { - #![proptest_config(ProptestConfig { - ..Default::default() - })] - - #[test] - #[cfg(not(debug_assertions))] // too slow if compiling for debug - fn compare_against_benchmarks( - n_candidates in 0..15_usize, // candidates (n) - target_value in 500..1_000_000_u64, // target value (sats) - n_target_outputs in 1..150_usize, // the number of outputs we're funding - target_weight in 0..10_000_u32, // the sum of the weight of the outputs (wu) - replace in common::maybe_replace(0..10_000u64), // The weight of the transaction we're replacing - feerate in 1.0..100.0_f32, // feerate (sats/vb) - feerate_lt_diff in -5.0..50.0_f32, // longterm feerate diff (sats/vb) - drain_weight in 100..=500_u32, // drain weight (wu) - drain_spend_weight in 1..=2000_u32, // drain spend weight (wu) - drain_dust in 100..=1000_u64, // drain dust (sats) - n_drain_outputs in 1..150usize, // the number of drain outputs - ) { - println!("======================================="); - let start = std::time::Instant::now(); - let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha); - let feerate = FeeRate::from_sat_per_vb(feerate); - let drain_weights = DrainWeights { - output_weight: drain_weight as u64, - spend_weight: drain_spend_weight as u64, - n_outputs: n_drain_outputs, - }; - - let wv = test_wv(&mut rng); - let candidates = wv.take(n_candidates).collect::>(); - - - let target = Target { - outputs: TargetOutputs { - n_outputs: n_target_outputs, - value_sum: target_value, - weight_sum: target_weight as u64, - }, - fee: TargetFee { - rate: feerate, - replace, - ..TargetFee::ZERO - }, - max_weight: None, - }; - let cs = CoinSelector::new(&candidates, target); - - let make_metric = || { - Changeless(LowestFee { - long_term_feerate: feerate, - dust_relay_feerate: FeeRate::from_sat_per_vb(1.0), - drain_weights, - }) - }; - - let solutions = cs.bnb_solutions(make_metric()); - - println!("candidates: {:#?}", cs.candidates().collect::>()); - - let best = solutions - .enumerate() - .filter_map(|(i, sol)| Some((i, sol?))) - .last(); - - - match best { - Some((_i, (_sol, _score))) => { - /* there is nothing to check about a changeless solution */ - } - None => { - let mut cs = cs.clone(); - let mut metric = make_metric(); - let has_solution = common::exhaustive_search(&mut cs, &mut metric).is_some(); - dbg!(format!("{}", cs)); - assert!(!has_solution); - } - } - dbg!(start.elapsed()); - } -} diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 2166692..656c04b 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -1,6 +1,6 @@ #![allow(unused_imports)] mod common; -use bdk_coin_select::metrics::{Changeless, LowestFee}; +use bdk_coin_select::metrics::LowestFee; use bdk_coin_select::{ BnbMetric, Candidate, ChangePolicy, CoinSelector, Drain, DrainWeights, FeeRate, NoBnbSolution, Replace, Target, TargetFee, TargetOutputs, TX_FIXED_FIELD_WEIGHT, @@ -173,47 +173,6 @@ proptest! { } } -/// We wrap `LowestFee` in `Changeless` to derive a metric that finds the lowest-fee changeless -/// solution. Constraining to changeless should never take fewer rounds than the unconstrained -/// `LowestFee`. -#[test] -fn combined_changeless_metric() { - let params = common::StrategyParams { - n_candidates: 100, - target_value: 100_000, - target_weight: 1000 - TX_FIXED_FIELD_WEIGHT as u32 - 1, - replace: None, - feerate: 5.0, - feerate_lt_diff: -4.0, - drain_weight: 200, - drain_spend_weight: 600, - drain_dust: 200, - n_target_outputs: 1, - n_drain_outputs: 1, - max_weight: None, - }; - - let candidates = common::gen_candidates(params.n_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, 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, metric_changeless, usize::MAX).expect("must find solution"); - println!("score={:?} rounds={}", combined_score, combined_rounds); - - assert!(combined_rounds >= rounds); -} - /// Because this metric decides change optimally, it never creates a change output whose value /// wouldn't cover the future cost of spending it. Here a single input overshoots the target by only /// ~130 sats — far less than the drain's spend cost — so the fee-optimal choice is to burn the From 8b384957228ed5aa2ef10d710de37b8ba11e4553 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 22 Sep 2026 08:59:44 +0000 Subject: [PATCH 5/7] perf: keep running sums of the selection in CoinSelector `input_weight` scanned the selected set three times and `selected_value` once, and metrics call them several times per branch-and-bound node through `excess`, `rate_excess`, `implied_fee`, and friends. Track the selected value, weight, segwit count, and legacy count as running sums updated in `select`/`deselect`, so every aggregate is O(1). Four sums are enough since the segwit/legacy count split: a segwit transaction adds the 2 WU witness header plus 1 WU per legacy input, which is `2 + legacy_count`. `select_all_effective` now goes through `select` so the sums cannot drift. Co-Authored-By: Claude Opus 5 --- src/coin_selector.rs | 66 ++++++++++++++++++++++++++------------------ tests/weight.rs | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 27 deletions(-) diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 9f6eaf9..4dbb25e 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -22,6 +22,12 @@ pub struct CoinSelector<'a> { selected: Bitset, banned: Bitset, candidate_order: Arc>, + /// Running sums over the selected candidates, kept up to date by [`select`](Self::select) and + /// [`deselect`](Self::deselect) so the aggregate queries don't rescan the selection. + selected_value: u64, + selected_weight: u64, + selected_segwit_count: usize, + selected_legacy_count: usize, } impl<'a> CoinSelector<'a> { @@ -46,6 +52,10 @@ impl<'a> CoinSelector<'a> { selected: Bitset::with_capacity(candidates.len()), banned: Bitset::with_capacity(candidates.len()), candidate_order: Arc::new((0..candidates.len()).collect::>()), + selected_value: 0, + selected_weight: 0, + selected_segwit_count: 0, + selected_legacy_count: 0, } } @@ -106,7 +116,15 @@ impl<'a> CoinSelector<'a> { /// Deselect a candidate at `index`. `index` refers to its position in the original `candidates` /// slice passed into [`CoinSelector::new`]. pub fn deselect(&mut self, index: usize) -> bool { - self.selected.remove(index) + let removed = self.selected.remove(index); + if removed { + let candidate = self.candidates[index]; + self.selected_value -= candidate.value; + self.selected_weight -= candidate.weight; + self.selected_segwit_count -= candidate.segwit_count; + self.selected_legacy_count -= candidate.legacy_count; + } + removed } /// Convienince method to pick elements of a slice by the indexes that are currently selected. @@ -120,7 +138,15 @@ impl<'a> CoinSelector<'a> { /// slice passed into [`CoinSelector::new`]. pub fn select(&mut self, index: usize) -> bool { assert!(index < self.candidates.len()); - self.selected.insert(index) + let inserted = self.selected.insert(index); + if inserted { + let candidate = self.candidates[index]; + self.selected_value += candidate.value; + self.selected_weight += candidate.weight; + self.selected_segwit_count += candidate.segwit_count; + self.selected_legacy_count += candidate.legacy_count; + } + inserted } /// Select the next unselected candidate in the sorted order fo the candidates. @@ -186,37 +212,23 @@ impl<'a> CoinSelector<'a> { /// The weight of the inputs including the witness header and the varint for the number of /// inputs. pub fn input_weight(&self) -> u64 { - let is_segwit_tx = self.selected().any(|(_, wv)| wv.segwit_count > 0); - let witness_header_extra_weight = is_segwit_tx as u64 * 2; - - let input_count = self - .selected() - .map(|(_, wv)| wv.segwit_count + wv.legacy_count) - .sum::(); + let input_count = self.selected_segwit_count + self.selected_legacy_count; let input_varint_weight = varint_size(input_count) * 4; - let selected_weight: u64 = self - .selected() - .map(|(_, candidate)| { - let mut weight = candidate.weight; - if is_segwit_tx { - // Legacy inputs do not have the witness length included in their weight field - // so we need to add 1 to each if it's a segwit tx. - weight += candidate.legacy_count as u64; - } - weight - }) - .sum(); + let segwit_extra_weight = if self.selected_segwit_count > 0 { + // The witness header, plus: legacy inputs do not have the witness length included in + // their weight field so we need to add 1 to each if it's a segwit tx. + 2 + self.selected_legacy_count as u64 + } else { + 0 + }; - input_varint_weight + selected_weight + witness_header_extra_weight + input_varint_weight + self.selected_weight + segwit_extra_weight } /// Absolute value sum of all selected inputs. pub fn selected_value(&self) -> u64 { - self.selected - .iter() - .map(|index| self.candidates[index].value) - .sum() + self.selected_value } /// Current weight of transaction implied by the selection. @@ -605,7 +617,7 @@ impl<'a> CoinSelector<'a> { { continue; } - self.selected.insert(cand_index); + self.select(cand_index); } } diff --git a/tests/weight.rs b/tests/weight.rs index df4372c..c419d2d 100644 --- a/tests/weight.rs +++ b/tests/weight.rs @@ -4,6 +4,7 @@ use bdk_coin_select::{ Candidate, CoinSelector, Drain, DrainWeights, Target, TargetFee, TargetOutputs, }; use bitcoin::{consensus::Decodable, ScriptBuf, Transaction}; +use proptest::prelude::*; fn hex_val(c: u8) -> u8 { match c { @@ -377,3 +378,58 @@ fn new_tr_keyspend_correct_weight() { Candidate::new_tr_keyspend(420).weight ); } + +proptest! { + /// `CoinSelector` keeps running sums of the selected candidates. After any sequence of + /// selects and deselects they must match a recompute from the selected set. + #[test] + fn running_sums_match_recompute( + candidates in proptest::collection::vec( + (0u64..1_000_000, 0u64..2_000, 0usize..4, 0usize..4).prop_map( + |(value, weight, segwit_count, legacy_count)| Candidate { + value, + weight, + segwit_count, + legacy_count, + }, + ), + 1..300, + ), + ops in proptest::collection::vec((any::(), any::()), 0..600), + ) { + let mut cs = CoinSelector::new( + &candidates, + Target { + fee: TargetFee::ZERO, + outputs: TargetOutputs::fund_outputs([]), + max_weight: None, + }, + ); + for (index, select) in ops { + let index = index.index(candidates.len()); + if select { + cs.select(index); + } else { + cs.deselect(index); + } + + let selected = cs.selected().map(|(_, c)| c).collect::>(); + let is_segwit_tx = selected.iter().any(|c| c.segwit_count > 0); + let input_count = selected.iter().map(|c| c.segwit_count + c.legacy_count).sum::(); + let varint_size = match input_count { + 0..=0xfc => 1, + 0xfd..=0xffff => 3, + _ => 5, + }; + let expected_weight = varint_size * 4 + + selected + .iter() + .map(|c| c.weight + if is_segwit_tx { c.legacy_count as u64 } else { 0 }) + .sum::() + + if is_segwit_tx { 2 } else { 0 }; + + prop_assert_eq!(cs.input_weight(), expected_weight); + prop_assert_eq!(cs.selected_value(), selected.iter().map(|c| c.value).sum::()); + } + } +} From 833a7e3c175c6a75c9e2bb558781980187c17d1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 22 Sep 2026 08:48:45 +0000 Subject: [PATCH 6/7] perf: search branch and bound depth-first Replace the best-first `BinaryHeap` frontier with a depth-first search that visits the child with the better bound first and backtracks in place. Only the current path is held, instead of a cloned selector per frontier node. Both `LowestFee`'s bound and the exact-match test metric grow with depth, so a min-heap always pops the shallowest node: it expands every 1-input prefix, then every 2-input prefix, and on a large pool the round budget runs out before it reaches a funded leaf. Depth-first reaches a funded leaf in as many expansions as the solution has inputs. The round-count assertions in `tests/bnb.rs` move: 164 -> 94 for the feasibility search, and 3194 -> 62452 for the exhaustive exact-match search, where depth-first expands more nodes before proving optimality. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + src/bnb.rs | 402 ++++++++++++++++++++++++++++--------------- src/coin_selector.rs | 4 + tests/bnb.rs | 4 +- 4 files changed, 269 insertions(+), 142 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0e9e69..735a472 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - **Breaking:** `LowestFee` no longer takes a `change_policy`. It now takes `dust_relay_feerate: FeeRate` and `drain_weights: DrainWeights`, and adds change only when doing so lowers the long-term fee and the change would not be dust. - Add `DrainWeights::dust_threshold(dust_relay_feerate)`, the minimum value a change output with these weights must have to not be dust. - Add `CoinSelector::select_srd`, a Single Random Draw selector (port of Bitcoin Core's `SelectCoinsSRD`) that adds candidates in random order until the change reaches `change_lower`, producing a healthy-sized (privacy-friendly) change output instead of minimizing fees. Adds the `CHANGE_LOWER` constant for Core's value. +- Search branch and bound depth-first (better-bound child first, backtracking in place) instead of best-first over a heap of cloned branches. Only the current path is held in memory, and under a round cap it reaches complete selections on large pools where the old frontier often ran out of rounds first. - **Breaking:** Remove the `Changeless` metric and the `BnbMetric` tuple implementations (`impl BnbMetric for ((A, f32), ...)`). Generic metric composition is no longer supported. `LowestFee` decides for itself whether a selection should carry change (adding one only when it lowers the long-term fee, clears the dust threshold, and fits `Target::max_weight`), so a separate changeless objective duplicates that decision and then constrains it. Callers that required a changeless transaction should use `LowestFee` and inspect the returned `Drain`. - **Breaking:** `CoinSelector::selected_indices` and `CoinSelector::banned` now return `&Bitset` instead of `&BTreeSet`. `Bitset` exposes `contains`/`len`/`is_empty`/`iter` (#46) - Replace the internal `Cow`/`Cow<[usize]>` selection state with a `Bitset` and an `Arc`-shared candidate order, making the per-branch clones in branch-and-bound substantially cheaper (#46) diff --git a/src/bnb.rs b/src/bnb.rs index c82965a..8b449dc 100644 --- a/src/bnb.rs +++ b/src/bnb.rs @@ -1,154 +1,166 @@ -use core::cmp::Reverse; - use crate::{float::Ordf32, Drain}; use super::CoinSelector; -use alloc::collections::BinaryHeap; +use alloc::vec::Vec; /// An [`Iterator`] that iterates over rounds of branch and bound to minimize the score of the /// provided [`BnbMetric`]. +/// +/// The tree is searched depth-first, visiting the child with the better bound first and +/// backtracking in place, so only the current path is held in memory. #[derive(Debug)] pub(crate) struct BnbIter<'a, M: BnbMetric> { - queue: BinaryHeap>, + selector: CoinSelector<'a>, + stack: Vec, best: Option, + exhausted: bool, /// The `BnBMetric` that will score each selection pub(crate) metric: M, } +/// A decision on the current path: either `index` was selected, or `banned` (`index` and the +/// candidates interchangeable with it) were banned. +#[derive(Debug)] +struct Frame { + is_inclusion: bool, + index: usize, + /// Position of `index` in the candidate order. + cursor: usize, + /// Position to resume scanning for the next undecided candidate below this frame. + next_cursor: usize, + banned: Vec, + /// Whether the other child of this frame's parent still needs visiting. + sibling_pending: bool, +} + impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> { type Item = Option<(CoinSelector<'a>, Ordf32)>; fn next(&mut self) -> Option { + if self.exhausted { + return None; + } + // { // println!("=========================== {:?}", self.best); - // for thing in self.queue.iter() { - // println!("{} {:?}", &thing.selector, thing.lower_bound); + // println!("{} {:?}", &self.selector, self.metric.bound(&self.selector)); + // for frame in self.stack.iter() { + // println!( + // "\t{} [{}] cursor={} sibling_pending={}", + // if frame.is_inclusion { "IN " } else { "EX " }, + // frame.index, + // frame.cursor, + // frame.sibling_pending, + // ); // } // let _ = std::io::stdin().read_line(&mut alloc::string::String::new()); // } - let branch = self.queue.pop()?; - if let Some(best) = &self.best { - // If the next thing in queue is not better than our best we're done. - if *best < branch.lower_bound { - // println!( - // "\t\t(SKIP) branch={} inclusion={} lb={:?}, score={:?}", - // branch.selector, - // !branch.is_exclusion, - // branch.lower_bound, - // self.metric.score(&branch.selector), - // ); - return None; - } - } - // println!( - // "\t\t( POP) branch={} inclusion={} lb={:?}, score={:?}", - // branch.selector, - // !branch.is_exclusion, - // branch.lower_bound, - // self.metric.score(&branch.selector), - // ); - - let selector = branch.selector; + // An exclusion node has the same selection as its parent, which was already scored. + let return_val = if !self.is_exclusion_node() { + self.try_record_best() + .map(|score| (self.selector.clone(), score)) + } else { + None + }; - let mut return_val = None; - if !branch.is_exclusion { - if let Some(score) = self.metric.score(&selector) { - let better = match self.best { - Some(best_score) => score < best_score, - None => true, - }; - if better { - self.best = Some(score); - return_val = Some(score); - } - }; + if !self.descend() && !self.backtrack_to_next_branch() { + self.exhausted = true; } - self.insert_new_branches(&selector); - Some(return_val.map(|score| (selector, score))) + Some(return_val) } } impl<'a, M: BnbMetric> BnbIter<'a, M> { pub(crate) fn new(mut selector: CoinSelector<'a>, metric: M) -> Self { + if metric.requires_ordering_by_descending_value_pwu() { + selector.sort_candidates_by_descending_value_pwu(); + } + let mut iter = BnbIter { - queue: BinaryHeap::default(), + selector, + stack: Vec::new(), best: None, + exhausted: false, metric, }; - if iter.metric.requires_ordering_by_descending_value_pwu() { - selector.sort_candidates_by_descending_value_pwu(); + if !iter.bound_is_promising() { + iter.exhausted = true; } - iter.consider_adding_to_queue(&selector, false); - iter } - fn consider_adding_to_queue(&mut self, cs: &CoinSelector<'a>, is_exclusion: bool) { - let bound = self.metric.bound(cs); - if let Some(bound) = bound { - let is_good_enough = match self.best { - Some(best) => best > bound, - None => true, - }; - if is_good_enough { - let branch = Branch { - lower_bound: bound, - selector: cs.clone(), - is_exclusion, - }; - /*println!( - "\t\t(PUSH) branch={} inclusion={} lb={:?} score={:?}", - branch.selector, - !branch.is_exclusion, - branch.lower_bound, - self.metric.score(&branch.selector), - );*/ - self.queue.push(branch); - } /* else { - println!( - "\t\t( REJ) branch={} inclusion={} lb={:?} score={:?}", - cs, - !is_exclusion, - bound, - self.metric.score(cs), - ); - }*/ - } /*else { - println!( - "\t\t(NO B) branch={} inclusion={} score={:?}", - cs, - !is_exclusion, - self.metric.score(cs), - ); - }*/ - } - - fn insert_new_branches(&mut self, cs: &CoinSelector<'a>) { - let (next_index, next) = match cs.unselected().next() { - Some(c) => c, - None => return, // exhausted + fn is_exclusion_node(&self) -> bool { + self.stack.last().map_or(false, |frame| !frame.is_inclusion) + } + + fn try_record_best(&mut self) -> Option { + let score = self.metric.score(&self.selector)?; + let better = match self.best { + Some(best_score) => score < best_score, + None => true, }; + if better { + self.best = Some(score); + Some(score) + } else { + None + } + } + + fn is_promising(&self, bound: Option) -> bool { + match (bound, self.best) { + (Some(bound), Some(best)) => best > bound, + (Some(_), None) => true, + (None, _) => false, + } + } - let mut inclusion_cs = cs.clone(); - inclusion_cs.select(next_index); - self.consider_adding_to_queue(&inclusion_cs, false); + fn bound_is_promising(&mut self) -> bool { + let bound = self.metric.bound(&self.selector); + self.is_promising(bound) + } + + fn cursor(&self) -> usize { + self.stack.last().map_or(0, |frame| frame.next_cursor) + } + + /// The first undecided candidate at or after `start` in the candidate order, as + /// `(index, cursor)`. + fn next_candidate(&self, start: usize) -> Option<(usize, usize)> { + for (cursor, (index, _)) in (start..).zip(self.selector.candidates().skip(start)) { + if !self.selector.is_selected(index) && !self.selector.banned().contains(index) { + return Some((index, cursor)); + } + } + None + } - // for the exclusion branch, we keep banning if candidates have the same weight, value and - // input counts. The counts matter because a segwit and a legacy input of equal weight - // change the tx weight differently. - let mut is_first_ban = true; - let mut exclusion_cs = cs.clone(); + /// The candidates to ban when excluding `index`, and the cursor to resume from. + /// + /// For the exclusion branch, we keep banning candidates that have the same value, weight and + /// input counts as the one we exclude. The counts matter because a segwit and a legacy input of + /// equal weight change the tx weight differently. Candidates are only compared until the first + /// mismatch, since this exploits them being adjacent in the sorted order. + fn exclusion_plan(&self, index: usize, cursor: usize) -> (Vec, usize) { + let next = self.selector.candidate(index); let to_ban = ( next.value, next.weight, next.segwit_count, next.legacy_count, ); - for (next_index, next) in cs.unselected() { + let mut banned = alloc::vec![index]; + let mut next_cursor = cursor + 1; + for (next_index, next) in self.selector.candidates().skip(cursor + 1) { + if self.selector.is_selected(next_index) || self.selector.banned().contains(next_index) + { + next_cursor += 1; + continue; + } if ( next.value, next.weight, @@ -158,55 +170,165 @@ impl<'a, M: BnbMetric> BnbIter<'a, M> { { break; } - let (_index, _candidate) = exclusion_cs - .candidates() - .find(|(i, _)| *i == next_index) - .expect("must have index since we are planning to ban it"); - if is_first_ban { - is_first_ban = false; - } /*else { - println!("banning: [{}] {:?}", _index, _candidate); - }*/ - exclusion_cs.ban(next_index); + // println!("banning: [{}] {:?}", next_index, next); + banned.push(next_index); + next_cursor += 1; } - self.consider_adding_to_queue(&exclusion_cs, true); + (banned, next_cursor) } -} -#[derive(Debug, Clone)] -struct Branch<'a> { - lower_bound: Ordf32, - selector: CoinSelector<'a>, - is_exclusion: bool, -} + fn apply_exclude(&mut self, banned: &[usize]) { + for &index in banned { + self.selector.ban(index); + } + } -impl Ord for Branch<'_> { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - // NOTE: Reverse comparision `lower_bound` because we want a min-heap (by default BinaryHeap - // is a max-heap). - // NOTE: We tiebreak equal scores based on whether it's exlusion or not (preferring - // inclusion). We do this because we want to try and get to evaluating complete selection - // returning actual scores as soon as possible. - core::cmp::Ord::cmp( - &(Reverse(&self.lower_bound), !self.is_exclusion), - &(Reverse(&other.lower_bound), !other.is_exclusion), - ) + fn undo_exclude(&mut self, banned: &[usize]) { + for &index in banned { + self.selector.unban(index); + } } -} -impl PartialOrd for Branch<'_> { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) + fn push_include(&mut self, index: usize, cursor: usize, sibling_pending: bool) { + self.selector.select(index); + self.stack.push(Frame { + is_inclusion: true, + index, + cursor, + next_cursor: cursor + 1, + banned: Vec::new(), + sibling_pending, + }); } -} -impl PartialEq for Branch<'_> { - fn eq(&self, other: &Self) -> bool { - self.lower_bound == other.lower_bound + fn push_exclude( + &mut self, + index: usize, + cursor: usize, + banned: Vec, + next_cursor: usize, + sibling_pending: bool, + ) { + self.apply_exclude(&banned); + self.stack.push(Frame { + is_inclusion: false, + index, + cursor, + next_cursor, + banned, + sibling_pending, + }); } -} -impl Eq for Branch<'_> {} + /// Step into the more promising child of the current node. Returns `false` if neither child + /// can beat the incumbent (or there are no undecided candidates left). + fn descend(&mut self) -> bool { + let (index, cursor) = match self.next_candidate(self.cursor()) { + Some(next) => next, + None => return false, + }; + + self.selector.select(index); + let inc_bound = self.metric.bound(&self.selector); + let inc_ok = self.is_promising(inc_bound); + self.selector.deselect(index); + + let (banned, exc_next_cursor) = self.exclusion_plan(index, cursor); + self.apply_exclude(&banned); + let exc_bound = self.metric.bound(&self.selector); + let exc_ok = self.is_promising(exc_bound); + self.undo_exclude(&banned); + + // println!( + // "\t\t(DESC) branch={} next=[{}] inc_lb={:?}{} exc_lb={:?}{}", + // self.selector, + // index, + // inc_bound, + // if inc_ok { "" } else { " (REJ)" }, + // exc_bound, + // if exc_ok { "" } else { " (REJ)" }, + // ); + + match (inc_ok, exc_ok) { + (false, false) => false, + (true, false) => { + self.push_include(index, cursor, false); + true + } + (false, true) => { + self.push_exclude(index, cursor, banned, exc_next_cursor, false); + true + } + (true, true) => { + // NOTE: We tiebreak equal bounds by preferring inclusion. We do this because we + // want to try and get to evaluating complete selections as soon as possible. + let include_first = match (inc_bound, exc_bound) { + (Some(inc), Some(exc)) => inc <= exc, + _ => true, + }; + if include_first { + self.push_include(index, cursor, true); + } else { + self.push_exclude(index, cursor, banned, exc_next_cursor, true); + } + true + } + } + } + + /// Unwind the path until a frame whose pending sibling can still beat the incumbent, and step + /// into that sibling. Returns `false` once the whole tree is exhausted. + /// + /// The sibling's bound is recomputed here rather than reused from [`descend`](Self::descend): + /// the incumbent may have improved since. + fn backtrack_to_next_branch(&mut self) -> bool { + while let Some(frame) = self.stack.pop() { + // println!( + // "\t\t(BACK) undo {} [{}] sibling_pending={}", + // if frame.is_inclusion { "IN " } else { "EX " }, + // frame.index, + // frame.sibling_pending, + // ); + if frame.is_inclusion { + self.selector.deselect(frame.index); + if frame.sibling_pending { + let (banned, next_cursor) = self.exclusion_plan(frame.index, frame.cursor); + self.apply_exclude(&banned); + if self.bound_is_promising() { + self.stack.push(Frame { + is_inclusion: false, + index: frame.index, + cursor: frame.cursor, + next_cursor, + banned, + sibling_pending: false, + }); + return true; + } + self.undo_exclude(&banned); + } + } else { + self.undo_exclude(&frame.banned); + if frame.sibling_pending { + self.selector.select(frame.index); + if self.bound_is_promising() { + self.stack.push(Frame { + is_inclusion: true, + index: frame.index, + cursor: frame.cursor, + next_cursor: frame.cursor + 1, + banned: Vec::new(), + sibling_pending: false, + }); + return true; + } + self.selector.deselect(frame.index); + } + } + } + false + } +} /// A branch and bound metric where we minimize the [`Ordf32`] score. /// diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 4dbb25e..caef257 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -171,6 +171,10 @@ impl<'a> CoinSelector<'a> { self.banned.insert(index); } + pub(crate) fn unban(&mut self, index: usize) { + self.banned.remove(index); + } + /// Gets the list of inputs that have been banned by [`ban`]. /// /// [`ban`]: Self::ban diff --git a/tests/bnb.rs b/tests/bnb.rs index 7a0b668..a6a2687 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -99,7 +99,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() { .last() .expect("it found a solution"); - assert_eq!(rounds, 3194); + assert_eq!(rounds, 62452); assert_eq!(best.input_weight(), solution_weight); assert_eq!(best.selected_value(), target_value, "score={:?}", score); } @@ -133,7 +133,7 @@ fn bnb_finds_solution_if_possible_in_n_iter() { .last() .expect("found a solution"); - assert_eq!(rounds, 164); + assert_eq!(rounds, 94); let excess = sol.excess(Drain::NONE); assert_eq!(excess, 0); } From 6cf6fc461ca155f64fe7b46b56fbcdc4121062a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 22 Sep 2026 09:55:18 +0000 Subject: [PATCH 7/7] feat: seed branch and bound with a greedy incumbent Yield the greedy selection before expanding the first node, and adopt its score as the incumbent. Otherwise a caller whose round budget runs out before the first complete selection gets `NoBnbSolution::RoundLimit` and falls through to whatever fallback it has, which on a large pool is far worse than the selection a single greedy pass would have handed it. Only the incumbent changes, not the bound, so the optimum stays reachable and the improving-solutions contract is unaffected. The two round-count assertions in `tests/bnb.rs` each move by one: the seed is a round. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + src/bnb.rs | 32 ++++++++++++++++++++++++++++++++ tests/bnb.rs | 4 ++-- tests/lowest_fee.rs | 18 ++++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 735a472..ead41e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Add `DrainWeights::dust_threshold(dust_relay_feerate)`, the minimum value a change output with these weights must have to not be dust. - Add `CoinSelector::select_srd`, a Single Random Draw selector (port of Bitcoin Core's `SelectCoinsSRD`) that adds candidates in random order until the change reaches `change_lower`, producing a healthy-sized (privacy-friendly) change output instead of minimizing fees. Adds the `CHANGE_LOWER` constant for Core's value. - Search branch and bound depth-first (better-bound child first, backtracking in place) instead of best-first over a heap of cloned branches. Only the current path is held in memory, and under a round cap it reaches complete selections on large pools where the old frontier often ran out of rounds first. +- Seed branch and bound with the greedy selection, so a search that runs out of rounds returns the best selection it has instead of `NoBnbSolution::RoundLimit`. `RoundLimit` now means the round budget ran out before even the greedy selection was scored (e.g. `max_rounds` is 0), or the metric rejected it. - **Breaking:** Remove the `Changeless` metric and the `BnbMetric` tuple implementations (`impl BnbMetric for ((A, f32), ...)`). Generic metric composition is no longer supported. `LowestFee` decides for itself whether a selection should carry change (adding one only when it lowers the long-term fee, clears the dust threshold, and fits `Target::max_weight`), so a separate changeless objective duplicates that decision and then constrains it. Callers that required a changeless transaction should use `LowestFee` and inspect the returned `Drain`. - **Breaking:** `CoinSelector::selected_indices` and `CoinSelector::banned` now return `&Bitset` instead of `&BTreeSet`. `Bitset` exposes `contains`/`len`/`is_empty`/`iter` (#46) - Replace the internal `Cow`/`Cow<[usize]>` selection state with a `Bitset` and an `Arc`-shared candidate order, making the per-branch clones in branch-and-bound substantially cheaper (#46) diff --git a/src/bnb.rs b/src/bnb.rs index 8b449dc..98ad362 100644 --- a/src/bnb.rs +++ b/src/bnb.rs @@ -13,6 +13,10 @@ pub(crate) struct BnbIter<'a, M: BnbMetric> { selector: CoinSelector<'a>, stack: Vec, best: Option, + /// The greedy selection, yielded before the first node is expanded. Its score is `best`: + /// nothing else can have run yet, so the two are set together. See + /// [`seed_greedy_incumbent`](BnbIter::seed_greedy_incumbent). + seed: Option>, exhausted: bool, /// The `BnBMetric` that will score each selection pub(crate) metric: M, @@ -37,6 +41,11 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> { type Item = Option<(CoinSelector<'a>, Ordf32)>; fn next(&mut self) -> Option { + if let Some(seed) = self.seed.take() { + let score = self.best.expect("the seed and `best` are set together"); + return Some(Some((seed, score))); + } + if self.exhausted { return None; } @@ -82,10 +91,13 @@ impl<'a, M: BnbMetric> BnbIter<'a, M> { selector, stack: Vec::new(), best: None, + seed: None, exhausted: false, metric, }; + iter.seed_greedy_incumbent(); + if !iter.bound_is_promising() { iter.exhausted = true; } @@ -93,6 +105,26 @@ impl<'a, M: BnbMetric> BnbIter<'a, M> { iter } + /// Score the greedy prefix and adopt it as the incumbent. + /// + /// Without this the search is not anytime: a caller that runs out of rounds before the first + /// complete selection gets nothing back and falls through to whatever fallback it has, which on + /// a large pool is far worse than the selection a single greedy pass would have handed it. The + /// seed costs one round and one scored selection, and since it is only an incumbent — the bound + /// is unchanged and still admissible — the optimum stays reachable. + /// + /// It yields nothing for a metric that rejects the greedy prefix outright. + fn seed_greedy_incumbent(&mut self) { + let mut seed = self.selector.clone(); + if seed.select_until_target_met().is_err() { + return; + } + if let Some(score) = self.metric.score(&seed) { + self.best = Some(score); + self.seed = Some(seed); + } + } + fn is_exclusion_node(&self) -> bool { self.stack.last().map_or(false, |frame| !frame.is_inclusion) } diff --git a/tests/bnb.rs b/tests/bnb.rs index a6a2687..aef47f7 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -99,7 +99,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() { .last() .expect("it found a solution"); - assert_eq!(rounds, 62452); + assert_eq!(rounds, 62453); assert_eq!(best.input_weight(), solution_weight); assert_eq!(best.selected_value(), target_value, "score={:?}", score); } @@ -133,7 +133,7 @@ fn bnb_finds_solution_if_possible_in_n_iter() { .last() .expect("found a solution"); - assert_eq!(rounds, 94); + assert_eq!(rounds, 95); let excess = sol.excess(Drain::NONE); assert_eq!(excess, 0); } diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 656c04b..f00939d 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -362,6 +362,24 @@ fn run_bnb_reports_max_weight_exceeded() { ); } +/// The search is seeded with the greedy selection, so a budget too small to search anything still +/// comes back with a usable answer instead of `RoundLimit`. Without that, a caller on a large pool +/// falls through to whatever fallback it has for something branch and bound could have covered. +#[test] +fn run_bnb_returns_the_greedy_selection_on_a_tight_budget() { + let candidates = core::iter::repeat(err_candidate(100_000)) + .take(500) + .collect::>(); + let target = Target { + outputs: err_outputs(1_000_000), + fee: TargetFee::ZERO, + max_weight: None, + }; + let mut cs = CoinSelector::new(&candidates, target); + cs.run_bnb(err_metric(), 1).expect("the seed is a solution"); + assert!(cs.is_funded()); +} + #[test] fn run_bnb_reports_round_limit() { // A solvable target, but zero rounds: we can't conclude infeasibility, only that we gave up.