Merge #13: Implement lowest fee metric correctly

6ae0fdf docs: fix typos and use better wording (志宇)
9e1cecd Use ChangePolicy::min_value in lowest fee tests (LLFourn)
7360052 Write lowest fee test that hits important branch (LLFourn)
17cc8f2 Score branches before adding children (LLFourn)
e30246d Fix lowest_fee metric (LLFourn)
0aef6ff Make lowest fee test fail by implementing score correctly (LLFourn)
0c66696 Rethink is_target_met (LLFourn)

Pull request description:

  This replaces #11. This first commit just fixes the metric to make the tests fail. Note the previous calculation was overthinking it. The fee metric is just `inputs - outputs +  long_term_feerate * change_weight`.

  Next steps:
  1. Make ci actually run the tests and get them to fail.
  2. Fix the metric lower bound

ACKs for top commit:
  evanlinjin:
    ACK 6ae0fdf

Tree-SHA512: c9c684ed95bc946e7e1ad8d65cd03f15180ba0bbc4e901d0e55145006629063fd110a3a08307f3e8c091ff875e41492bebc31895819455b58cc6a137b56103bc

Author: evanlinjin

README.md

@@ -235,20 +235,23 @@ let candidates = candidate_txouts
     .collect::<Vec<_>>();
 
 let mut selector = CoinSelector::new(&candidates, base_weight);
-let _result = selector
-    .select_until_target_met(target,  Drain::none());
-
-// Determine what the drain output will be, based on our selection.
-let drain = selector.drain(target, change_policy);
-
-// In theory the target must always still be met at this point
-assert!(selector.is_target_met(target, drain));
+selector
+    .select_until_target_met(target,  Drain::none())
+    .expect("we've got enough coins");
 
 // Get a list of coins that are selected.
 let selected_coins = selector
     .apply_selection(&candidate_txouts)
     .collect::<Vec<_>>();
 assert_eq!(selected_coins.len(), 1);
+
+// Determine whether we should add a change output.
+let drain = selector.drain(target, change_policy);
+
+if drain.is_some() {
+    // add our change output to the transaction
+    let change_value = drain.value;
+}
 ```
 
 # Minimum Supported Rust Version (MSRV)

src/bnb.rs

@@ -51,24 +51,22 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> {
 
         let selector = branch.selector;
 
-        self.insert_new_branches(&selector);
-
-        if branch.is_exclusion {
-            return Some(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);
+                }
+            };
         }
 
-        let score = match self.metric.score(&selector) {
-            Some(score) => score,
-            None => return Some(None),
-        };
-
-        if let Some(best_score) = &self.best {
-            if score >= *best_score {
-                return Some(None);
-            }
-        }
-        self.best = Some(score);
-        Some(Some((selector, score)))
+        self.insert_new_branches(&selector);
+        Some(return_val.map(|score| (selector, score)))
     }
 }
 
@@ -92,7 +90,11 @@ 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);
         if let Some(bound) = bound {
-            if self.best.is_none() || self.best.as_ref().unwrap() >= &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(),

src/coin_selector.rs

@@ -169,29 +169,10 @@ impl<'a> CoinSelector<'a> {
     /// enough value.
     ///
     /// [`ban`]: Self::ban
-    pub fn is_selection_possible(&self, target: Target, drain: Drain) -> bool {
+    pub fn is_selection_possible(&self, target: Target) -> bool {
         let mut test = self.clone();
         test.select_all_effective(target.feerate);
-        test.is_target_met(target, drain)
-    }
-
-    /// Is meeting the target *plausible* with this `change_policy`.
-    /// Note this will respect [`ban`]ned candidates.
-    ///
-    /// This is very similar to [`is_selection_possible`] except that you pass in a change policy.
-    /// This method will give the right answer as long as `change_policy` is monotone but otherwise
-    /// can it can give false negatives.
-    ///
-    /// [`ban`]: Self::ban
-    /// [`is_selection_possible`]: Self::is_selection_possible
-    pub fn is_selection_plausible_with_change_policy(
-        &self,
-        target: Target,
-        change_policy: &impl Fn(&CoinSelector<'a>, Target) -> Drain,
-    ) -> bool {
-        let mut test = self.clone();
-        test.select_all_effective(target.feerate);
-        test.is_target_met(target, change_policy(&test, target))
+        test.is_target_met(target)
     }
 
     /// Returns true if no candidates have been selected.
@@ -283,6 +264,14 @@ impl<'a> CoinSelector<'a> {
         (self.weight(drain_weight) as f32 * feerate.spwu()).ceil() as u64
     }
 
+    /// 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
+    }
+
     /// 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
@@ -330,7 +319,9 @@ impl<'a> CoinSelector<'a> {
 
     /// Sorts the candidates by descending value per weight unit, tie-breaking with value.
     pub fn sort_candidates_by_descending_value_pwu(&mut self) {
-        self.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse((wv.value_pwu(), wv.value)));
+        self.sort_candidates_by_key(|(_, wv)| {
+            core::cmp::Reverse((Ordf32(wv.value_pwu()), wv.value))
+        });
     }
 
     /// The waste created by the current selection as measured by the [waste metric].
@@ -405,19 +396,30 @@ impl<'a> CoinSelector<'a> {
         self.unselected_indices().next().is_none()
     }
 
-    /// Whether the constraints of `Target` have been met if we include the `drain` ouput.
-    pub fn is_target_met(&self, target: Target, drain: Drain) -> bool {
+    /// Whether the constraints of `Target` have been met if we include a specific `drain` ouput.
+    ///
+    /// Note if [`is_target_met`] is true and the `drain` is produced from the [`drain`] method then
+    /// this method will also always be true.
+    ///
+    /// [`is_target_met`]: Self::is_target_met
+    /// [`drain`]: Self::drain
+    pub fn is_target_met_with_drain(&self, target: Target, drain: Drain) -> bool {
         self.excess(target, drain) >= 0
     }
 
+    /// Whether the constraints of `Target` have been met.
+    pub fn is_target_met(&self, target: Target) -> bool {
+        self.is_target_met_with_drain(target, Drain::none())
+    }
+
     /// Whether the constrains of `Target` have been met if we include the drain (change) output
     /// when `change_policy` decides it should be present.
     pub fn is_target_met_with_change_policy(
         &self,
         target: Target,
         change_policy: ChangePolicy,
     ) -> bool {
-        self.is_target_met(target, self.drain(target, change_policy))
+        self.is_target_met_with_drain(target, self.drain(target, change_policy))
     }
 
     /// Select all unselected candidates
@@ -442,17 +444,34 @@ impl<'a> CoinSelector<'a> {
             },
         );
         if excess > change_policy.min_value as i64 {
+            debug_assert_eq!(
+                self.is_target_met(target),
+                self.is_target_met_with_drain(
+                    target,
+                    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)
         } else {
             None
         }
     }
 
-    /// Convienince method that calls [`drain_value`] and converts the result into `Drain` by using
-    /// the provided `DrainWeights`. Note carefully that the `change_policy` should have been
-    /// calculated with the same `DrainWeights`.
+    /// Figures out whether the current selection should have a change output given the
+    /// `change_policy`. If it shouldn't then it will return a `Drain` where [`Drain::is_none`] is
+    /// true. The value of the `Drain` will be the same as [`drain_value`].
+    ///
+    /// If [`is_target_met`] returns true for this selection then [`is_target_met_with_drain`] will
+    /// also be true if you pass in the drain returned from this method.
     ///
     /// [`drain_value`]: Self::drain_value
+    /// [`is_target_met_with_drain`]: Self::is_target_met_with_drain
+    /// [`is_target_met`]: Self::is_target_met
+    #[must_use]
     pub fn drain(&self, target: Target, change_policy: ChangePolicy) -> Drain {
         match self.drain_value(target, change_policy) {
             Some(value) => Drain {
@@ -470,7 +489,7 @@ impl<'a> CoinSelector<'a> {
         for cand_index in self.candidate_order.iter() {
             if self.selected.contains(cand_index)
                 || self.banned.contains(cand_index)
-                || self.candidates[*cand_index].effective_value(feerate) <= Ordf32(0.0)
+                || self.candidates[*cand_index].effective_value(feerate) <= 0.0
             {
                 continue;
             }
@@ -486,7 +505,7 @@ impl<'a> CoinSelector<'a> {
         target: Target,
         drain: Drain,
     ) -> Result<(), InsufficientFunds> {
-        self.select_until(|cs| cs.is_target_met(target, drain))
+        self.select_until(|cs| cs.is_target_met_with_drain(target, drain))
             .ok_or_else(|| InsufficientFunds {
                 missing: self.excess(target, drain).unsigned_abs(),
             })
@@ -615,13 +634,13 @@ impl Candidate {
     }
 
     /// Effective value of this input candidate: `actual_value - input_weight * feerate (sats/wu)`.
-    pub fn effective_value(&self, feerate: FeeRate) -> Ordf32 {
-        Ordf32(self.value as f32 - (self.weight as f32 * feerate.spwu()))
+    pub fn effective_value(&self, feerate: FeeRate) -> f32 {
+        self.value as f32 - (self.weight as f32 * feerate.spwu())
     }
 
     /// Value per weight unit
-    pub fn value_pwu(&self) -> Ordf32 {
-        Ordf32(self.value as f32 / self.weight as f32)
+    pub fn value_pwu(&self) -> f32 {
+        self.value as f32 / self.weight as f32
     }
 }
 
@@ -647,11 +666,17 @@ impl DrainWeights {
             + self.spend_weight as f32 * long_term_feerate.spwu()
     }
 
-    /// Create [`DrainWeights`] that represents a drain output with a taproot keyspend.
+    /// The fee you will pay to spend these change output(s) in the future.
+    pub fn spend_fee(&self, long_term_feerate: FeeRate) -> u64 {
+        (self.spend_weight as f32 * long_term_feerate.spwu()).ceil() as u64
+    }
+
+    /// Create [`DrainWeights`] that represents a drain output that will be spent with a taproot
+    /// keyspend
     pub fn new_tr_keyspend() -> Self {
         Self {
             output_weight: TXOUT_BASE_WEIGHT + TR_SPK_WEIGHT,
-            spend_weight: TXIN_BASE_WEIGHT + TR_KEYSPEND_SATISFACTION_WEIGHT,
+            spend_weight: TR_KEYSPEND_TXIN_WEIGHT,
         }
     }
 }

src/metrics.rs

@@ -25,7 +25,7 @@ fn change_lower_bound(cs: &CoinSelector, target: Target, change_policy: ChangePo
         let mut least_excess = cs.clone();
         cs.unselected()
             .rev()
-            .take_while(|(_, wv)| wv.effective_value(target.feerate) < Ordf32(0.0))
+            .take_while(|(_, wv)| wv.effective_value(target.feerate) < 0.0)
             .for_each(|(index, _)| {
                 least_excess.select(index);
             });

src/metrics/changeless.rs

@@ -1,7 +1,5 @@
 use super::change_lower_bound;
-use crate::{
-    bnb::BnbMetric, change_policy::ChangePolicy, float::Ordf32, CoinSelector, Drain, Target,
-};
+use crate::{bnb::BnbMetric, change_policy::ChangePolicy, float::Ordf32, CoinSelector, Target};
 
 /// Metric for finding changeless solutions only.
 pub struct Changeless {
@@ -13,7 +11,7 @@ pub struct Changeless {
 
 impl BnbMetric for Changeless {
     fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
-        if cs.is_target_met(self.target, Drain::none())
+        if cs.is_target_met(self.target)
             && cs.drain_value(self.target, self.change_policy).is_none()
         {
             Some(Ordf32(0.0))