Rethink is_target_met

if is_target_met true it should continue to be true if you add a drain
from a ChangePolicy. We should try and guarantee we never add a drain
when it would invalidate the selection.

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.
@@ -405,19 +386,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 +434,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 {
@@ -486,7 +495,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(),
             })

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))

src/metrics/lowest_fee.rs

@@ -51,7 +51,7 @@ impl LowestFee {
 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(self.target, drain) {
+        if !cs.is_target_met_with_drain(self.target, drain) {
             return None;
         }
 
@@ -76,7 +76,7 @@ impl BnbMetric for LowestFee {
         };
         // println!("\tchange lb: {:?}", change_lb_weights);
 
-        if cs.is_target_met(self.target, change_lb) {
+        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?
 
@@ -92,7 +92,7 @@ impl BnbMetric for LowestFee {
                     .rev()
                     .take_while(|(cs, _, candidate)| {
                         candidate.effective_value(self.target.feerate).0 < 0.0
-                            && cs.is_target_met(self.target, Drain::none())
+                            && cs.is_target_met_with_drain(self.target, Drain::none())
                     })
                     .last()
                     .map(|(cs, _, _)| cs);
@@ -133,7 +133,7 @@ impl BnbMetric for LowestFee {
         let (mut cs, slurp_index, candidate_to_slurp) = cs
             .clone()
             .select_iter()
-            .find(|(cs, _, _)| cs.is_target_met(self.target, change_lb))?;
+            .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);

src/metrics/waste.rs

@@ -33,7 +33,7 @@ pub struct Waste {
 impl BnbMetric for Waste {
     fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
         let drain = cs.drain(self.target, self.change_policy);
-        if !cs.is_target_met(self.target, drain) {
+        if !cs.is_target_met_with_drain(self.target, drain) {
             return None;
         }
         let score = cs.waste(self.target, self.long_term_feerate, drain, 1.0);
@@ -57,7 +57,7 @@ impl BnbMetric for Waste {
 
         if rate_diff >= 0.0 {
             // Our lower bound algorithms differ depending on whether we have already met the target or not.
-            if cs.is_target_met(self.target, change_lower_bound) {
+            if cs.is_target_met_with_drain(self.target, change_lower_bound) {
                 let current_change = cs.drain(self.target, self.change_policy);
 
                 // first lower bound candidate is just the selection itself
@@ -80,8 +80,8 @@ impl BnbMetric for Waste {
                         .select_iter()
                         .rev()
                         .take_while(|(cs, _, wv)| {
-                            wv.effective_value(self.target.feerate).0 < 0.0
-                                && cs.is_target_met(self.target, Drain::none())
+                            wv.effective_value(self.target.feerate) < Ordf32(0.0)
+                                && cs.is_target_met(self.target)
                         })
                         .last();
 
@@ -133,10 +133,10 @@ impl BnbMetric for Waste {
                 // weight.
                 //
                 // 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, change_lower_bound))?;
+                let (mut cs, slurp_index, to_slurp) =
+                    cs.clone().select_iter().find(|(cs, _, _)| {
+                        cs.is_target_met_with_drain(self.target, change_lower_bound)
+                    })?;
 
                 cs.deselect(slurp_index);
 
@@ -177,7 +177,7 @@ impl BnbMetric for Waste {
                 let mut cs = cs.clone();
                 // ... but first check that by selecting all effective we can actually reach target
                 cs.select_all_effective(self.target.feerate);
-                if !cs.is_target_met(self.target, Drain::none()) {
+                if !cs.is_target_met(self.target) {
                     return None;
                 }
                 let change_at_value_optimum = cs.drain(self.target, self.change_policy);

tests/bnb.rs

@@ -154,7 +154,7 @@ proptest! {
 
         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_selection_possible(target, Drain::none())),
+            _ => prop_assert!(!cs.is_selection_possible(target)),
         }
     }
 

tests/common.rs

@@ -154,7 +154,7 @@ where
                         cs,
                         cs.drain_value(target, change_policy).is_some(),
                         lb_score,
-                        cs.is_target_met(target, Drain::none()),
+                        cs.is_target_met(target),
                         descendant_cs,
                         descendant_cs.drain_value(target, change_policy).is_some(),
                         descendant_score,

tests/waste.rs

@@ -485,7 +485,7 @@ fn randomly_satisfy_target_with_low_waste<'a>(
     while let Some(next) = cs.unselected_indices().choose(rng) {
         cs.select(next);
         let change = change_policy(&cs, target);
-        if cs.is_target_met(target, change) {
+        if cs.is_target_met_with_drain(target, change) {
             let curr_waste = cs.waste(target, long_term_feerate, change, 1.0);
             if let Some(last_waste) = last_waste {
                 if curr_waste > last_waste {