Implement lowest fee metric correctly

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 outupt to the transaction
+    let change_value = drain.value;
+}
 ```
 
 # Minimum Supported Rust Version (MSRV)

src/coin_selector.rs

@@ -319,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].
@@ -487,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;
             }
@@ -632,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
     }
 }
 
@@ -669,11 +671,12 @@ impl DrainWeights {
         (self.spend_weight as f32 * long_term_feerate.spwu()).ceil() as u64
     }
 
-    /// Create [`DrainWeights`] that represents a drain output with a taproot keyspend.
+    /// 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/lowest_fee.rs

@@ -1,6 +1,6 @@
 use crate::{
-    change_policy::ChangePolicy, float::Ordf32, metrics::change_lower_bound, BnbMetric, Candidate,
-    CoinSelector, Drain, DrainWeights, FeeRate, Target,
+    change_policy::ChangePolicy, float::Ordf32, BnbMetric, Candidate, CoinSelector, Drain, FeeRate,
+    Target,
 };
 
 /// Metric that aims to minimize transaction fees. The future fee for spending the change output is
@@ -25,41 +25,14 @@ pub struct LowestFee {
     pub change_policy: ChangePolicy,
 }
 
-impl LowestFee {
-    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) => {
-                (cs.input_weight() + drain_weights.output_weight) as f32
-                    * self.target.feerate.spwu()
-                    + drain_weights.spend_weight as f32 * self.long_term_feerate.spwu()
-            }
-            // changeless
-            None => cs.input_weight() as f32 * self.target.feerate.spwu(),
-        }
-    }
-}
-
 impl BnbMetric for LowestFee {
     fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
-        let drain = cs.drain(self.target, self.change_policy);
-        if !cs.is_target_met_with_drain(self.target, drain) {
+        if !cs.is_target_met(self.target) {
             return None;
         }
 
         let long_term_fee = {
+            let drain = cs.drain(self.target, self.change_policy);
             let fee_for_the_tx = cs.fee(self.target.value, drain.value);
             assert!(
                 fee_for_the_tx > 0,
@@ -75,98 +48,96 @@ impl BnbMetric for LowestFee {
     }
 
     fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
-        // this either returns:
-        // * None: change output may or may not exist
-        // * Some: change output must exist from this branch onwards
-        let change_lb = change_lower_bound(cs, self.target, self.change_policy);
-        let change_lb_weights = if change_lb.is_some() {
-            Some(change_lb.weights)
-        } else {
-            None
-        };
-        // println!("\tchange lb: {:?}", change_lb_weights);
-
-        if cs.is_target_met_with_drain(self.target, change_lb) {
-            // 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 (include excess).
-            let mut lower_bound = self.calc_metric(cs, change_lb_weights);
-
-            if change_lb_weights.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()
-                    .rev()
-                    .take_while(|(cs, _, candidate)| {
-                        candidate.effective_value(self.target.feerate).0 < 0.0
-                            && cs.is_target_met_with_drain(self.target, Drain::none())
-                    })
-                    .last()
-                    .map(|(cs, _, _)| cs);
-
-                if let Some(cs) = selection_with_as_much_negative_ev_as_possible {
-                    // we have selected as much "real" inputs as possible, is it possible to select
-                    // one more with the perfect weight?
-                    let can_do_better_by_slurping =
-                        cs.unselected().next_back().and_then(|(_, candidate)| {
-                            if candidate.effective_value(self.target.feerate).0 < 0.0 {
-                                Some(candidate)
-                            } else {
-                                None
-                            }
-                        });
-                    let lower_bound_changeless = match can_do_better_by_slurping {
-                        Some(finishing_input) => {
-                            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(self.target, excess, finishing_input);
-
-                            (cs.input_weight() as f32 + perfect_input_weight)
-                                * self.target.feerate.spwu()
+        if cs.is_target_met(self.target) {
+            let current_score = self.score(cs).unwrap();
+
+            let drain_value = cs.drain_value(self.target, self.change_policy);
+
+            if let Some(drain_value) = drain_value {
+                // it's possible that adding another input might reduce your long term if it gets
+                // rid of an expensive change output. Our strategy is to take the lowest sat per
+                // value candidate we have and use it as a benchmark. We imagine it has the perfect
+                // value (but the same sats per weight unit) to get rid of the change output by
+                // adding negative effective value (i.e. perfectly reducing excess to the point
+                // where change wouldn't be added according to the policy).
+                //
+                // TODO: This metric could be tighter by being more complicated but this seems to be
+                // good enough for now.
+                let amount_above_change_threshold = drain_value - self.change_policy.min_value;
+
+                if let Some((_, low_sats_per_wu_candidate)) = cs.unselected().next_back() {
+                    let ev = low_sats_per_wu_candidate.effective_value(self.target.feerate);
+                    if ev < 0.0 {
+                        // we can only reduce excess if ev is negative
+                        let value_per_negative_effective_value =
+                            low_sats_per_wu_candidate.value as f32 / ev.abs();
+                        // this is how much abosolute value we have to add to cancel out the excess
+                        let extra_value_needed_to_get_rid_of_change = amount_above_change_threshold
+                            as f32
+                            * value_per_negative_effective_value;
+
+                        // NOTE: the drain_value goes to fees if we get rid of it so it's part of
+                        // the cost of removing the change output
+                        let cost_of_getting_rid_of_change =
+                            extra_value_needed_to_get_rid_of_change + drain_value as f32;
+                        let cost_of_change = self
+                            .change_policy
+                            .drain_weights
+                            .waste(self.target.feerate, self.long_term_feerate);
+                        let best_score_without_change = Ordf32(
+                            current_score.0 - cost_of_change + cost_of_getting_rid_of_change,
+                        );
+                        if best_score_without_change < current_score {
+                            return Some(best_score_without_change);
                         }
-                        None => self.calc_metric(&cs, None),
-                    };
-
-                    lower_bound = lower_bound.min(lower_bound_changeless)
+                    }
                 }
             }
 
-            return Some(Ordf32(lower_bound));
+            Some(current_score)
+        } else {
+            // Step 1: select everything up until the input that hits the target.
+            let (mut cs, slurp_index, to_slurp) = cs
+                .clone()
+                .select_iter()
+                .find(|(cs, _, _)| cs.is_target_met(self.target))?;
+
+            cs.deselect(slurp_index);
+
+            // Step 2: We pretend that the final input exactly cancels out the remaining excess
+            // by taking whatever value we want from it but at the value per weight of the real
+            // input.
+            let ideal_next_weight = {
+                let remaining_rate = cs.rate_excess(self.target, Drain::none());
+
+                slurp_wv(to_slurp, remaining_rate.min(0), self.target.feerate)
+            };
+            let input_weight_lower_bound = cs.input_weight() as f32 + ideal_next_weight;
+            let ideal_fee_by_feerate =
+                (cs.base_weight() as f32 + input_weight_lower_bound) * self.target.feerate.spwu();
+            let ideal_fee = ideal_fee_by_feerate.max(self.target.min_fee as f32);
+
+            Some(Ordf32(ideal_fee))
         }
-
-        // target is not met yet
-        // select until we just exceed target, then we slurp the last selection
-        let (mut cs, slurp_index, candidate_to_slurp) = cs
-            .clone()
-            .select_iter()
-            .find(|(cs, _, _)| cs.is_target_met_with_drain(self.target, change_lb))?;
-        cs.deselect(slurp_index);
-
-        let mut lower_bound = self.calc_metric_lb(&cs, change_lb_weights);
-
-        // 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()),
-        );
-        // use the highest excess to find "perfect candidate weight"
-        let perfect_input_weight = slurp(self.target, perfect_excess, candidate_to_slurp);
-        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(target: Target, excess: i64, candidate: Candidate) -> f32 {
-    let vpw = candidate.value_pwu().0;
-    let perfect_weight = -excess as f32 / (vpw - target.feerate.spwu());
-    perfect_weight.max(0.0)
+/// Returns the "perfect weight" for this candidate to slurp up a given value with `feerate` while
+/// not changing the candidate's value/weight ratio.
+///
+/// Used to pretend that a candidate had precisely `value_to_slurp` + fee needed to include it. It
+/// tells you how much weight such a perfect candidate would have if it had the same value per
+/// weight unit as `candidate`. This is useful for estimating a lower weight bound for a perfect
+/// match.
+fn slurp_wv(candidate: Candidate, value_to_slurp: i64, feerate: FeeRate) -> f32 {
+    // the value per weight unit this candidate offers at feerate
+    let value_per_wu = (candidate.value as f32 / candidate.weight as f32) - feerate.spwu();
+    // return how much weight we need
+    let weight_needed = value_to_slurp as f32 / value_per_wu;
+    debug_assert!(weight_needed <= candidate.weight as f32);
+    weight_needed.min(0.0)
 }

src/metrics/waste.rs

@@ -80,15 +80,15 @@ impl BnbMetric for Waste {
                         .select_iter()
                         .rev()
                         .take_while(|(cs, _, wv)| {
-                            wv.effective_value(self.target.feerate) < Ordf32(0.0)
+                            wv.effective_value(self.target.feerate) < 0.0
                                 && cs.is_target_met(self.target)
                         })
                         .last();
 
                     if let Some((cs, _, _)) = selection_with_as_much_negative_ev_as_possible {
                         let can_do_better_by_slurping =
                             cs.unselected().next_back().and_then(|(_, wv)| {
-                                if wv.effective_value(self.target.feerate).0 < 0.0 {
+                                if wv.effective_value(self.target.feerate) < 0.0 {
                                     Some(wv)
                                 } else {
                                     None
@@ -149,8 +149,7 @@ impl BnbMetric for Waste {
                     let remaining_rate = cs.rate_excess(self.target, change_lower_bound);
                     let remaining_abs = cs.absolute_excess(self.target, change_lower_bound);
 
-                    let weight_to_satisfy_abs =
-                        remaining_abs.min(0) as f32 / to_slurp.value_pwu().0;
+                    let weight_to_satisfy_abs = remaining_abs.min(0) as f32 / to_slurp.value_pwu();
 
                     let weight_to_satisfy_rate =
                         slurp_wv(to_slurp, remaining_rate.min(0), self.target.feerate);
@@ -201,7 +200,7 @@ impl BnbMetric for Waste {
                     .select_iter()
                     .rev()
                     .take_while(|(cs, _, wv)| {
-                        wv.effective_value(self.target.feerate).0 < 0.0
+                        wv.effective_value(self.target.feerate) < 0.0
                             || cs.drain_value(self.target, self.change_policy).is_none()
                     })
                     .last();

tests/changeless.rs

@@ -77,7 +77,7 @@ proptest! {
                 let mut cmp_benchmarks = vec![
                     {
                         let mut naive_select = cs.clone();
-                        naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(wv.effective_value(target.feerate)));
+                        naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(Ordf32(wv.effective_value(target.feerate))));
                         // we filter out failing onces below
                         let _ = naive_select.select_until_target_met(target, Drain { weights: drain, value: 0 });
                         naive_select

tests/lowest_fee.rs

@@ -158,6 +158,5 @@ fn combined_changeless_metric() {
         common::bnb_search(&mut cs_b, metric_combined, usize::MAX).expect("must find solution");
     println!("score={:?} rounds={}", combined_score, combined_rounds);
 
-    // [todo] shouldn't rounds be less since we are only considering changeless branches?
-    assert!(combined_rounds <= rounds);
+    assert!(combined_rounds >= rounds);
 }

tests/waste.rs

@@ -116,7 +116,7 @@ fn waste_naive_effective_value_shouldnt_be_better() {
         .expect("should find solution");
 
     let mut naive_select = cs.clone();
-    naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(wv.value_pwu()));
+    naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(Ordf32(wv.value_pwu())));
     // we filter out failing onces below
     let _ = naive_select.select_until_target_met(target, drain);