test(coin_select): prop tests for metrics

`can_eventually_find_best_solution` ensures that the metric is always
able to find the best solution. We find the best solution via an
exhaustive search. Essentially, this ensures that our bounding method
never undershoots.

`ensure_bound_does_not_undershoot` is a more fine-grained test for the
above. The checks descendant solutions after a branch, and ensures that
the branch's lower-bound is lower than all descendant scores.

example-crates/example_cli/src/lib.rs

@@ -1,6 +1,6 @@
 pub use anyhow;
 use anyhow::Context;
-use bdk_coin_select::{Candidate, CoinSelector, Drain};
+use bdk_coin_select::{Candidate, CoinSelector};
 use bdk_file_store::Store;
 use serde::{de::DeserializeOwned, Serialize};
 use std::{cmp::Reverse, collections::HashMap, path::PathBuf, sync::Mutex};
@@ -498,12 +498,18 @@ where
     let mut selector = CoinSelector::new(&candidates, transaction.weight().to_wu() as u32);
     match cs_algorithm {
         CoinSelectionAlgo::BranchAndBound => {
-            let metric = bdk_coin_select::metrics::LowestFee {
+            let metric = bdk_coin_select::metrics::Waste {
                 target,
                 long_term_feerate,
                 change_policy: &drain_policy,
             };
-            selector.run_bnb(metric, 100_000)?;
+            if let Err(bnb_err) = selector.run_bnb(metric, 100_000) {
+                selector.sort_candidates_by_descending_value_pwu();
+                println!(
+                    "Error: {} Falling back to select until target met.",
+                    bnb_err
+                );
+            };
         }
         CoinSelectionAlgo::LargestFirst => {
             selector.sort_candidates_by_key(|(_, c)| Reverse(c.value))
@@ -517,13 +523,7 @@ where
     };
 
     // ensure target is met
-    selector.select_until_target_met(
-        target,
-        Drain {
-            weights: drain_weights,
-            value: 0,
-        },
-    )?;
+    selector.select_until_target_met(target, drain_policy(&selector, target))?;
 
     // get the selected utxos
     let selected_txos = selector

nursery/coin_select/src/bnb.rs

@@ -18,7 +18,7 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> {
         //     for thing in self.queue.iter() {
         //         println!("{} {:?}", &thing.selector, thing.lower_bound);
         //     }
-        //     let _ = std::io::stdin().read_line(&mut String::new());
+        //     let _ = std::io::stdin().read_line(&mut alloc::string::String::new());
         // }
 
         let branch = self.queue.pop()?;
@@ -88,14 +88,14 @@ impl<'a, M: BnbMetric> BnbIter<'a, M> {
         }
 
         let next_unselected = cs.unselected_indices().next().unwrap();
+
         let mut inclusion_cs = cs.clone();
         inclusion_cs.select(next_unselected);
+        self.consider_adding_to_queue(&inclusion_cs, false);
+
         let mut exclusion_cs = cs.clone();
         exclusion_cs.ban(next_unselected);
-
-        for (child_cs, is_exclusion) in &[(&inclusion_cs, false), (&exclusion_cs, true)] {
-            self.consider_adding_to_queue(child_cs, *is_exclusion)
-        }
+        self.consider_adding_to_queue(&exclusion_cs, true);
     }
 }
 

nursery/coin_select/src/coin_selector.rs

@@ -12,7 +12,7 @@ use alloc::{borrow::Cow, collections::BTreeSet, vec::Vec};
 /// [`bnb_solutions`]: CoinSelector::bnb_solutions
 #[derive(Debug, Clone)]
 pub struct CoinSelector<'a> {
-    base_weight: u32,
+    pub base_weight: u32,
     candidates: &'a [Candidate],
     selected: Cow<'a, BTreeSet<usize>>,
     banned: Cow<'a, BTreeSet<usize>>,
@@ -672,8 +672,8 @@ impl std::error::Error for InsufficientFunds {}
 
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub struct NoBnbSolution {
-    max_rounds: usize,
-    rounds: usize,
+    pub max_rounds: usize,
+    pub rounds: usize,
 }
 
 impl core::fmt::Display for NoBnbSolution {

nursery/coin_select/src/metrics/lowest_fee.rs

@@ -9,11 +9,35 @@ pub struct LowestFee<'c, C> {
     pub change_policy: &'c C,
 }
 
+impl<'c, C> Clone for LowestFee<'c, C> {
+    fn clone(&self) -> Self {
+        Self {
+            target: self.target,
+            long_term_feerate: self.long_term_feerate,
+            change_policy: self.change_policy,
+        }
+    }
+}
+
+impl<'c, C> Copy for LowestFee<'c, C> {}
+
 impl<'c, C> LowestFee<'c, C>
 where
     for<'a, 'b> C: Fn(&'b CoinSelector<'a>, Target) -> Drain,
 {
-    fn calculate_metric(&self, cs: &CoinSelector<'_>, drain_weights: Option<DrainWeights>) -> f32 {
+    fn calc_metric(&self, cs: &CoinSelector<'_>, drain_weights: Option<DrainWeights>) -> f32 {
+        self.calc_metric_lb(cs, drain_weights)
+            + match drain_weights {
+                Some(_) => {
+                    let selected_value = cs.selected_value();
+                    assert!(selected_value >= self.target.value);
+                    (cs.selected_value() - self.target.value) as f32
+                }
+                None => 0.0,
+            }
+    }
+
+    fn calc_metric_lb(&self, cs: &CoinSelector<'_>, drain_weights: Option<DrainWeights>) -> f32 {
         match drain_weights {
             // with change
             Some(drain_weights) => {
@@ -22,10 +46,7 @@ where
                     + drain_weights.spend_weight as f32 * self.long_term_feerate.spwu()
             }
             // changeless
-            None => {
-                cs.input_weight() as f32 * self.target.feerate.spwu()
-                    + (cs.selected_value() - self.target.value) as f32
-            }
+            None => cs.input_weight() as f32 * self.target.feerate.spwu(),
         }
     }
 }
@@ -48,7 +69,7 @@ where
             None
         };
 
-        Some(Ordf32(self.calculate_metric(cs, drain_weights)))
+        Some(Ordf32(self.calc_metric(cs, drain_weights)))
     }
 
     fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Self::Score> {
@@ -66,11 +87,12 @@ where
             // Target is met, is it possible to add further inputs to remove drain output?
             // If we do, can we get a better score?
 
-            // First lower bound candidate is just the selection itself.
-            let mut lower_bound = self.calculate_metric(cs, change_lb_weights);
+            // First lower bound candidate is just the selection itself (include excess).
+            let mut lower_bound = self.calc_metric(cs, change_lb_weights);
 
-            // Since a changeless solution may exist, we should try reduce the excess
             if change_lb.is_none() {
+                // Since a changeless solution may exist, we should try minimize the excess with by
+                // adding as much -ev candidates as possible
                 let selection_with_as_much_negative_ev_as_possible = cs
                     .clone()
                     .select_iter()
@@ -98,13 +120,12 @@ where
                             let excess = cs.rate_excess(self.target, Drain::none());
 
                             // change the input's weight to make it's effective value match the excess
-                            let perfect_input_weight =
-                                slurp_candidate(finishing_input, excess, self.target.feerate);
+                            let perfect_input_weight = slurp(self.target, excess, finishing_input);
 
                             (cs.input_weight() as f32 + perfect_input_weight)
                                 * self.target.feerate.spwu()
                         }
-                        None => self.calculate_metric(&cs, None),
+                        None => self.calc_metric(&cs, None),
                     };
 
                     lower_bound = lower_bound.min(lower_bound_changeless)
@@ -122,54 +143,51 @@ where
             .find(|(cs, _, _)| cs.is_target_met(self.target, change_lb))?;
         cs.deselect(slurp_index);
 
-        let perfect_excess = i64::min(
-            cs.rate_excess(self.target, Drain::none()),
-            cs.absolute_excess(self.target, Drain::none()),
-        );
+        let mut lower_bound = self.calc_metric_lb(&cs, change_lb_weights);
 
-        match change_lb_weights {
-            // must have change!
-            Some(change_weights) => {
-                // [todo] This will not be perfect, just a placeholder for now
-                let lowest_fee = (cs.input_weight() + change_weights.output_weight) as f32
-                    * self.target.feerate.spwu()
-                    + change_weights.spend_weight as f32 * self.long_term_feerate.spwu();
-
-                Some(Ordf32(lowest_fee))
-            }
-            // can be changeless!
-            None => {
-                // use the lowest excess to find "perfect candidate weight"
-                let perfect_input_weight =
-                    slurp_candidate(candidate_to_slurp, perfect_excess, self.target.feerate);
+        if change_lb_weights.is_none() {
+            // changeless solution is possible, find the max excess we need to rid of
+            let perfect_excess = i64::max(
+                cs.rate_excess(self.target, Drain::none()),
+                cs.absolute_excess(self.target, Drain::none()),
+            );
 
-                // the perfect input weight canned the excess and we assume no change
-                let lowest_fee =
-                    (cs.input_weight() as f32 + perfect_input_weight) * self.target.feerate.spwu();
+            // use the highest excess to find "perfect candidate weight"
+            let perfect_input_weight = slurp(self.target, perfect_excess, candidate_to_slurp);
 
-                Some(Ordf32(lowest_fee))
-            }
+            lower_bound += perfect_input_weight * self.target.feerate.spwu();
         }
+
+        Some(Ordf32(lower_bound))
     }
 
     fn requires_ordering_by_descending_value_pwu(&self) -> bool {
         true
     }
 }
 
-fn slurp_candidate(candidate: Candidate, excess: i64, feerate: FeeRate) -> f32 {
-    let candidate_weight = candidate.weight as f32;
-
-    // this equation is dervied from:
-    // * `input_effective_value = input_value - input_weight * feerate`
-    // * `input_value * new_input_weight = new_input_value * input_weight`
-    //      (ensure we have the same value:weight ratio)
-    // where we want `input_effective_value` to match `-excess`.
-    let perfect_weight = -(candidate_weight * excess as f32)
-        / (candidate.value as f32 - candidate_weight * feerate.spwu());
-
-    debug_assert!(perfect_weight <= candidate_weight);
+fn slurp(target: Target, excess: i64, candidate: Candidate) -> f32 {
+    let vpw = candidate.value_pwu().0;
+    let perfect_weight = -excess as f32 / (vpw - target.feerate.spwu());
+
+    #[cfg(debug_assertions)]
+    {
+        let perfect_value = (candidate.value as f32 * perfect_weight) / candidate.weight as f32;
+        let perfect_vpw = perfect_value / perfect_weight;
+        if perfect_vpw.is_nan() {
+            assert_eq!(perfect_value, 0.0);
+            assert_eq!(perfect_weight, 0.0);
+        } else {
+            assert!(
+                (vpw - perfect_vpw).abs() < 0.01,
+                "value:weight ratio must stay the same: vpw={} perfect_vpw={} perfect_value={} perfect_weight={}",
+                vpw,
+                perfect_vpw,
+                perfect_value,
+                perfect_weight,
+            );
+        }
+    }
 
-    // we can't allow the weight to go negative
-    perfect_weight.min(0.0)
+    perfect_weight.max(0.0)
 }

nursery/coin_select/src/metrics/waste.rs

@@ -24,6 +24,18 @@ pub struct Waste<'c, C> {
     pub change_policy: &'c C,
 }
 
+impl<'c, C> Clone for Waste<'c, C> {
+    fn clone(&self) -> Self {
+        Self {
+            target: self.target,
+            long_term_feerate: self.long_term_feerate,
+            change_policy: self.change_policy,
+        }
+    }
+}
+
+impl<'c, C> Copy for Waste<'c, C> {}
+
 impl<'c, C> BnbMetric for Waste<'c, C>
 where
     for<'a, 'b> C: Fn(&'b CoinSelector<'a>, Target) -> Drain,

nursery/coin_select/tests/changeless.rs

@@ -1,5 +1,7 @@
 #![allow(unused)]
-use bdk_coin_select::{float::Ordf32, metrics, Candidate, CoinSelector, Drain, FeeRate, Target};
+use bdk_coin_select::{
+    float::Ordf32, metrics, Candidate, CoinSelector, Drain, DrainWeights, FeeRate, Target,
+};
 use proptest::{
     prelude::*,
     test_runner::{RngAlgorithm, TestRng},
@@ -41,10 +43,9 @@ proptest! {
         let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha);
         let long_term_feerate = FeeRate::from_sat_per_vb(0.0f32.max(feerate - long_term_feerate_diff));
         let feerate = FeeRate::from_sat_per_vb(feerate);
-        let drain = Drain {
-            weight: change_weight,
+        let drain = DrainWeights {
+            output_weight: change_weight,
             spend_weight: change_spend_weight,
-            value: 0
         };
 
         let change_policy = bdk_coin_select::change_policy::min_waste(drain, long_term_feerate);
@@ -59,7 +60,7 @@ proptest! {
             min_fee
         };
 
-        let solutions = cs.branch_and_bound(metrics::Changeless {
+        let solutions = cs.bnb_solutions(metrics::Changeless {
             target,
             change_policy: &change_policy
         });
@@ -78,7 +79,7 @@ proptest! {
                         let mut naive_select = cs.clone();
                         naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(wv.effective_value(target.feerate)));
                         // we filter out failing onces below
-                        let _ = naive_select.select_until_target_met(target, drain);
+                        let _ = naive_select.select_until_target_met(target, Drain { weights: drain, value: 0 });
                         naive_select
                     },
                 ];

nursery/coin_select/tests/metrics.proptest-regressions

@@ -0,0 +1,12 @@
+# 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 78e8456749053271949d1821613de18d007ef6ddabc85eb0b9dc64b640f85736 # shrinks to n_candidates = 11, target_value = 500, base_weight = 0, min_fee = 0, feerate = 72.77445, feerate_lt_diff = 0.0, drain_weight = 100, drain_spend_weight = 1, drain_dust = 100
+cc 36b9844f4bd28caa412b4a7e384c370bf9406dd6d1cd3a37409181c096a3da95 # shrinks to n_candidates = 8, target_value = 378748, base_weight = 245, min_fee = 0, feerate = 90.57628, feerate_lt_diff = 41.46504, drain_weight = 408, drain_spend_weight = 1095, drain_dust = 100
+cc 9c5c20afb83a7b1b8dc66404c63379f12ac796f5f23e04ccb568778c84230e18 # shrinks to n_candidates = 11, target_value = 434651, base_weight = 361, min_fee = 0, feerate = 41.85748, feerate_lt_diff = 0.0, drain_weight = 100, drain_spend_weight = 1, drain_dust = 100
+cc 858be736b81a2b1ca5dafc2d6442c7facfd46af6d14659df3772daf0940b105e # shrinks to n_candidates = 3, target_value = 422791, base_weight = 272, min_fee = 0, feerate = 93.71708, feerate_lt_diff = 8.574516, drain_weight = 100, drain_spend_weight = 703, drain_dust = 100
+cc d643d1aaf1d708ca2a7ce3bf5357a14e82c9d60935b126c9b3f338a9bb0ebed3 # shrinks to n_candidates = 10, target_value = 381886, base_weight = 684, min_fee = 0, feerate = 72.56796, feerate_lt_diff = 0.0, drain_weight = 100, drain_spend_weight = 354, drain_dust = 100
+cc 931d5609471a5575882a8b2cb2c45884330cb18e95368c89a63cfe507a7c1a62 # shrinks to n_candidates = 10, target_value = 76204, base_weight = 71, min_fee = 0, feerate = 72.3613, feerate_lt_diff = 0.0, drain_weight = 100, drain_spend_weight = 357, drain_dust = 100