feat(coin_select): add DrainWeights and min_value_and_waste policy

This is a better default change policy as it minimizes waste without
introducing a change output with a dust value. We update `examples_cli`
to use this change policy.

We introduce `DrainWeights` and refactor `change_policy` to use it.

example-crates/example_cli/src/lib.rs

@@ -1,6 +1,6 @@
 pub use anyhow;
 use anyhow::Context;
-use bdk_coin_select::{Candidate, CoinSelector};
+use bdk_coin_select::{Candidate, CoinSelector, Drain};
 use bdk_file_store::Store;
 use serde::{de::DeserializeOwned, Serialize};
 use std::{cmp::Reverse, collections::HashMap, path::PathBuf, sync::Mutex};
@@ -466,13 +466,13 @@ where
     };
 
     let target = bdk_coin_select::Target {
-        feerate: bdk_coin_select::FeeRate::from_sat_per_vb(1.0),
+        feerate: bdk_coin_select::FeeRate::from_sat_per_vb(5.0),
         min_fee: 0,
         value: transaction.output.iter().map(|txo| txo.value).sum(),
     };
 
-    let drain = bdk_coin_select::Drain {
-        weight: {
+    let drain_weights = bdk_coin_select::DrainWeights {
+        output_weight: {
             // we calculate the weight difference of including the drain output in the base tx
             // this method will detect varint size changes of txout count
             let tx_weight = transaction.weight();
@@ -486,11 +486,14 @@ where
             };
             (tx_weight_with_drain - tx_weight).to_wu() as u32 - 1
         },
-        value: 0,
         spend_weight: change_plan.expected_weight() as u32,
     };
-    let long_term_feerate = bdk_coin_select::FeeRate::from_sat_per_wu(0.25);
-    let drain_policy = bdk_coin_select::change_policy::min_waste(drain, long_term_feerate);
+    let long_term_feerate = bdk_coin_select::FeeRate::from_sat_per_vb(1.0);
+    let drain_policy = bdk_coin_select::change_policy::min_value_and_waste(
+        drain_weights,
+        change_script.dust_value().to_sat(),
+        long_term_feerate,
+    );
 
     let mut selector = CoinSelector::new(&candidates, transaction.weight().to_wu() as u32);
     match cs_algorithm {
@@ -503,16 +506,10 @@ where
             let (final_selection, _score) = selector
                 .branch_and_bound(metric)
                 .take(50_000)
-                // we only process viable solutions
+                // skip exclusion branches (as they are not scored)
                 .flatten()
-                .reduce(|(best_sol, best_score), (curr_sol, curr_score)| {
-                    // we are reducing waste
-                    if curr_score < best_score {
-                        (curr_sol, curr_score)
-                    } else {
-                        (best_sol, best_score)
-                    }
-                })
+                // the last result is always the best score
+                .last()
                 .ok_or(anyhow::format_err!("no bnb solution found"))?;
             selector = final_selection;
         }
@@ -531,7 +528,13 @@ where
                 }),
                 CoinSelectionAlgo::BranchAndBound => unreachable!("bnb variant is matched already"),
             }
-            selector.select_until_target_met(target, drain)?
+            selector.select_until_target_met(
+                target,
+                Drain {
+                    weights: drain_weights,
+                    value: 0,
+                },
+            )?
         }
     };
 

nursery/coin_select/src/change_policy.rs

@@ -1,28 +1,34 @@
 #[allow(unused)] // some bug in <= 1.48.0 sees this as unused when it isn't
 use crate::float::FloatExt;
-use crate::{CoinSelector, Drain, FeeRate, Target};
+use crate::{CoinSelector, Drain, DrainWeights, FeeRate, Target};
 use core::convert::TryInto;
 
 /// Add a change output if the change value would be greater than or equal to `min_value`.
 ///
 /// Note that the value field of the `drain` is ignored.
-pub fn min_value(mut drain: Drain, min_value: u64) -> impl Fn(&CoinSelector, Target) -> Drain {
-    debug_assert!(drain.is_some());
+pub fn min_value(
+    drain_weights: DrainWeights,
+    min_value: u64,
+) -> impl Fn(&CoinSelector, Target) -> Drain {
     let min_value: i64 = min_value
         .try_into()
         .expect("min_value is ridiculously large");
-    drain.value = 0;
+
     move |cs, target| {
+        let mut drain = Drain {
+            weights: drain_weights,
+            ..Default::default()
+        };
+
         let excess = cs.excess(target, drain);
-        if excess >= min_value {
-            let mut drain = drain;
-            drain.value = excess.try_into().expect(
-                "cannot be negative since we checked it against min_value which is positive",
-            );
-            drain
-        } else {
-            Drain::none()
+        if excess < min_value {
+            return Drain::none();
         }
+
+        drain.value = excess
+            .try_into()
+            .expect("must be positive since it is greater than min_value (which is positive)");
+        drain
     }
 }
 
@@ -31,23 +37,49 @@ pub fn min_value(mut drain: Drain, min_value: u64) -> impl Fn(&CoinSelector, Tar
 /// Note that the value field of the `drain` is ignored.
 /// The `value` will be set to whatever needs to be to reach the given target.
 pub fn min_waste(
-    mut drain: Drain,
+    drain_weights: DrainWeights,
+    long_term_feerate: FeeRate,
+) -> impl Fn(&CoinSelector, Target) -> Drain {
+    move |cs, target| {
+        // The output waste of a changeless solution is the excess.
+        let waste_changeless = cs.excess(target, Drain::none());
+        let waste_with_change = drain_weights
+            .waste(target.feerate, long_term_feerate)
+            .ceil() as i64;
+
+        if waste_changeless <= waste_with_change {
+            return Drain::none();
+        }
+
+        let mut drain = Drain {
+            weights: drain_weights,
+            value: 0,
+        };
+        drain.value = cs
+            .excess(target, drain)
+            .try_into()
+            .expect("the excess must be positive because drain free excess was > waste");
+        drain
+    }
+}
+
+/// Add a change output if the change value is greater than or equal to `min_value` and if it would
+/// reduce the overall waste of the transaction.
+///
+/// Note that the value field of the `drain` is ignored. [`Drain`] is just used for the drain weight
+/// and drain spend weight.
+pub fn min_value_and_waste(
+    drain_weights: DrainWeights,
+    min_value: u64,
     long_term_feerate: FeeRate,
 ) -> impl Fn(&CoinSelector, Target) -> Drain {
-    debug_assert!(drain.is_some());
-    drain.value = 0;
+    let min_waste_policy = crate::change_policy::min_waste(drain_weights, long_term_feerate);
 
     move |cs, target| {
-        let excess = cs.excess(target, Drain::none());
-        if excess > drain.waste(target.feerate, long_term_feerate).ceil() as i64 {
-            let mut drain = drain;
-            drain.value = cs
-                .excess(target, drain)
-                .try_into()
-                .expect("the excess must be positive because drain free excess was > waste");
-            drain
-        } else {
-            Drain::none()
+        let drain = min_waste_policy(cs, target);
+        if drain.value < min_value {
+            return Drain::none();
         }
+        drain
     }
 }

nursery/coin_select/src/coin_selector.rs

@@ -208,14 +208,14 @@ impl<'a> CoinSelector<'a> {
         self.base_weight + self.input_weight() + drain_weight
     }
 
-    /// How much the current selection overshoots the value needed to acheive `target`.
+    /// How much the current selection overshoots the value needed to achieve `target`.
     ///
     /// In order for the resulting transaction to be valid this must be 0.
     pub fn excess(&self, target: Target, drain: Drain) -> i64 {
         self.selected_value() as i64
             - target.value as i64
             - drain.value as i64
-            - self.implied_fee(target.feerate, target.min_fee, drain.weight) as i64
+            - self.implied_fee(target.feerate, target.min_fee, drain.weights.output_weight) as i64
     }
 
     /// How much the current selection overshoots the value need to satisfy `target.feerate` and
@@ -224,7 +224,7 @@ impl<'a> CoinSelector<'a> {
         self.selected_value() as i64
             - target.value as i64
             - drain.value as i64
-            - self.implied_fee_from_feerate(target.feerate, drain.weight) as i64
+            - self.implied_fee_from_feerate(target.feerate, drain.weights.output_weight) as i64
     }
 
     /// How much the current selection overshoots the value needed to satisfy `target.min_fee` and
@@ -236,11 +236,11 @@ impl<'a> CoinSelector<'a> {
             - target.min_fee as i64
     }
 
-    /// The feerate the transaction would have if we were to use this selection of inputs to acheive
-    /// the `target_value`
+    /// The feerate the transaction would have if we were to use this selection of inputs to achieve
+    /// the `target_value`.
     pub fn implied_feerate(&self, target_value: u64, drain: Drain) -> FeeRate {
         let numerator = self.selected_value() as i64 - target_value as i64 - drain.value as i64;
-        let denom = self.weight(drain.weight);
+        let denom = self.weight(drain.weights.output_weight);
         FeeRate::from_sat_per_wu(numerator as f32 / denom as f32)
     }
 
@@ -327,8 +327,8 @@ impl<'a> CoinSelector<'a> {
             excess_waste *= excess_discount.max(0.0).min(1.0);
             waste += excess_waste;
         } else {
-            waste += drain.weight as f32 * target.feerate.spwu()
-                + drain.spend_weight as f32 * long_term_feerate.spwu();
+            waste += drain.weights.output_weight as f32 * target.feerate.spwu()
+                + drain.weights.spend_weight as f32 * long_term_feerate.spwu();
         }
 
         waste
@@ -514,6 +514,34 @@ impl Candidate {
     }
 }
 
+/// A structure that represents the weight costs of a drain (a.k.a. change) output.
+///
+/// This structure can also represent multiple outputs.
+#[derive(Default, Debug, Clone, Copy, Hash, PartialEq, Eq)]
+pub struct DrainWeights {
+    /// The weight of adding this drain output.
+    pub output_weight: u32,
+    /// The weight of spending this drain output (in the future).
+    pub spend_weight: u32,
+}
+
+impl DrainWeights {
+    /// The waste of adding this drain to a transaction according to the [waste metric].
+    ///
+    /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection
+    pub fn waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 {
+        self.output_weight as f32 * feerate.spwu()
+            + self.spend_weight as f32 * long_term_feerate.spwu()
+    }
+
+    pub fn new_tr_keyspend() -> Self {
+        Self {
+            output_weight: TXOUT_BASE_WEIGHT + TR_SPK_WEIGHT,
+            spend_weight: TXIN_BASE_WEIGHT + TR_KEYSPEND_SATISFACTION_WEIGHT,
+        }
+    }
+}
+
 /// A drain (A.K.A. change) output.
 /// Technically it could represent multiple outputs.
 ///
@@ -522,12 +550,10 @@ impl Candidate {
 /// [`change_policy`]: crate::change_policy
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Default)]
 pub struct Drain {
-    /// The weight of adding this drain
-    pub weight: u32,
-    /// The value that should be assigned to the drain
+    /// Weight of adding drain output and spending the drain output.
+    pub weights: DrainWeights,
+    /// The value that should be assigned to the drain.
     pub value: u64,
-    /// The weight of spending this drain
-    pub spend_weight: u32,
 }
 
 impl Drain {
@@ -546,19 +572,11 @@ impl Drain {
         !self.is_none()
     }
 
-    pub fn new_tr_keyspend() -> Self {
-        Self {
-            weight: TXOUT_BASE_WEIGHT + TR_SPK_WEIGHT,
-            value: 0,
-            spend_weight: TXIN_BASE_WEIGHT + TR_KEYSPEND_SATISFACTION_WEIGHT,
-        }
-    }
-
     /// The waste of adding this drain to a transaction according to the [waste metric].
     ///
     /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection
     pub fn waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 {
-        self.weight as f32 * feerate.spwu() + self.spend_weight as f32 * long_term_feerate.spwu()
+        self.weights.waste(feerate, long_term_feerate)
     }
 }