Merge #52: refactor!: pass Target to BnbMetric methods instead of storing it

304ac2d refactor!: pass Target to BnbMetric methods instead of storing it (志宇)

Pull request description:

  ## Motivation

  `BnbMetric` implementations currently each **store** their own `Target` (`LowestFee { target, .. }`, `Changeless { target, inner }`). That has two problems:

  1. **`Changeless<M>` duplicates the target** and must keep its copy in sync with the inner metric's — a silent-mismatch footgun (both fields are public and set independently).
  2. It's **inconsistent with the rest of the library**, where `target` is always a *parameter*: `cs.excess(target, drain)`, `cs.is_target_met(target)`, `cs.drain(target, policy)`. The metric structs are the only place target is baked into state.

  ## What this does

  Make `target` a parameter of the metric methods (and the BnB entry points):

  ```rust
  pub trait BnbMetric {
      fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32>;
      fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32>;
      fn drain(&mut self, cs: &CoinSelector<'_>, target: Target) -> Drain;
      fn requires_ordering_by_descending_value_pwu(&self) -> bool { false }
  }

  // cs.run_bnb(target, metric, max_rounds)
  // cs.bnb_solutions(target, metric)
  ```

  Consequently:
  - `LowestFee` → `{ long_term_feerate, dust_relay_feerate, drain_weights }` (no `target`).
  - `Changeless<M>` → `Changeless<M>(pub M)` — with `target` gone it collapses to a single field, so it becomes a plain newtype: `Changeless(LowestFee { .. })`. The duplicate target is gone **by construction**, so the "must match inner's target" invariant is deleted rather than documented.
  - `BnbIter` holds the single `target` and threads it to the metric.

  Conceptually this reflects the right split: `target` is the *problem/constraints*; the metric is the *objective*. The feerates/drain-weights stay in the metric (objective config); target — universal and problem-level — becomes a shared input, so a mismatch across composed metrics is unrepresentable.

  Argument order is `cs`, then `target` (then the rest), keeping the same `cs`-before-`target` relative order the library already uses (where `cs` is usually the receiver).

  ## Notes

  - The `target` field and its plumbing simply disappear, so the net diff is small despite the wide surface.
  - Breaking change on the unreleased 0.5.0 API surface (post-#49).
  - `cargo fmt` / `clippy` / `doc` / full test suite + doctests / `no_std` build are all green.

  🤖 Generated with [Claude Code](https://claude.com/claude-code)

ACKs for top commit:
  evanlinjin:
    ACK 304ac2d

Tree-SHA512: 1087e9154ffcb57358209e2da97e1cd685c732da02385d278427da45be660fe0febff366985b1114095230b135d10a29f2d7aa7dd99591aba43b98fcbe345080

Author: evanlinjin

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Unreleased
 
+- **Breaking:** `BnbMetric`'s `score`, `bound`, and `drain` take the `target: Target` as a parameter, and `CoinSelector::run_bnb`/`bnb_solutions` gain a leading `target` argument. Consequently `LowestFee` and `Changeless` no longer store a `target` field. This removes the target that `Changeless<M>` previously had to keep in sync with its inner metric, and aligns the metric API with the rest of `CoinSelector`, where `target` is always passed in.
 - **Breaking:** `BnbMetric` metrics now decide the change output themselves. The trait gains a `drain(&mut self, cs) -> Drain` method; call it on a branch-and-bound solution (or the `LowestFee` metric directly) to get the change output the metric optimized against, instead of computing a separate `ChangePolicy`.
 - **Breaking:** `CoinSelector::run_bnb` now returns `(Ordf32, Drain)` instead of just `Ordf32`, handing back the change output the metric decided on for the winning selection.
 - **Breaking:** `LowestFee` no longer takes a `change_policy`. It now takes `dust_relay_feerate: FeeRate` and `drain_weights: DrainWeights`, and adds change only when doing so lowers the long-term fee and the change would not be dust.

README.md

@@ -140,21 +140,20 @@ let dust_relay_feerate = FeeRate::from_sat_per_vb(3.0);
 // decides for itself whether to add a change output: change is added whenever doing so reduces the
 // long-term fee (factoring in the cost to spend the output later on) and the change wouldn't be dust.
 let mut metric = LowestFee {
-    target,
     long_term_feerate, // used to calculate the cost of spending the change output in the future
     dust_relay_feerate,
     drain_weights,
 };
 
 // We run the branch and bound algorithm with a max round limit of 100,000.
 // On success it returns the score along with the change output the metric decided on.
-let change = match coin_selector.run_bnb(metric, 100_000) {
+let change = match coin_selector.run_bnb(target, metric, 100_000) {
     Err(err) => {
         println!("failed to find a solution: {}", err);
         // fall back to naive selection
         coin_selector.select_until_target_met(target).expect("a selection was impossible!");
         // the metric still decides the change output for whatever we end up selecting
-        metric.drain(&coin_selector)
+        metric.drain(&coin_selector, target)
     }
     Ok((score, change)) => {
         println!("we found a solution with score {}", score);

benches/coin_selector.rs

@@ -75,12 +75,11 @@ fn bench_run_bnb_lowest_fee(c: &mut Criterion) {
                 || selector.clone(),
                 |mut sel| {
                     let metric = LowestFee {
-                        target,
                         long_term_feerate,
                         dust_relay_feerate: FeeRate::from_sat_per_vb(1.0),
                         drain_weights: DrainWeights::TR_KEYSPEND,
                     };
-                    let _ = sel.run_bnb(metric, black_box(100_000));
+                    let _ = sel.run_bnb(target, metric, black_box(100_000));
                     sel
                 },
                 BatchSize::SmallInput,

src/bnb.rs

@@ -1,6 +1,6 @@
 use core::cmp::Reverse;
 
-use crate::{float::Ordf32, Drain};
+use crate::{float::Ordf32, Drain, Target};
 
 use super::CoinSelector;
 use alloc::collections::BinaryHeap;
@@ -11,6 +11,8 @@ use alloc::collections::BinaryHeap;
 pub(crate) struct BnbIter<'a, M: BnbMetric> {
     queue: BinaryHeap<Branch<'a>>,
     best: Option<Ordf32>,
+    /// The target the metric scores selections against.
+    pub(crate) target: Target,
     /// The `BnBMetric` that will score each selection
     pub(crate) metric: M,
 }
@@ -53,7 +55,7 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> {
 
         let mut return_val = None;
         if !branch.is_exclusion {
-            if let Some(score) = self.metric.score(&selector) {
+            if let Some(score) = self.metric.score(&selector, self.target) {
                 let better = match self.best {
                     Some(best_score) => score < best_score,
                     None => true,
@@ -71,10 +73,11 @@ impl<'a, M: BnbMetric> Iterator for BnbIter<'a, M> {
 }
 
 impl<'a, M: BnbMetric> BnbIter<'a, M> {
-    pub(crate) fn new(mut selector: CoinSelector<'a>, metric: M) -> Self {
+    pub(crate) fn new(mut selector: CoinSelector<'a>, target: Target, metric: M) -> Self {
         let mut iter = BnbIter {
             queue: BinaryHeap::default(),
             best: None,
+            target,
             metric,
         };
 
@@ -88,7 +91,7 @@ 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);
+        let bound = self.metric.bound(cs, self.target);
         if let Some(bound) = bound {
             let is_good_enough = match self.best {
                 Some(best) => best > bound,
@@ -199,25 +202,25 @@ impl Eq for Branch<'_> {}
 ///
 /// This is to be used as input for [`CoinSelector::run_bnb`] or [`CoinSelector::bnb_solutions`].
 pub trait BnbMetric {
-    /// Get the score of a given selection.
+    /// Get the score of a given selection for `target`.
     ///
     /// If this returns `None`, the selection is invalid.
-    fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>;
+    fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32>;
 
-    /// Get the lower bound score using a heuristic.
+    /// Get the lower bound score using a heuristic for `target`.
     ///
     /// This represents the best possible score of all descendant branches (according to the
     /// heuristic).
     ///
     /// If this returns `None`, the current branch and all descendant branches will not have valid
     /// solutions.
-    fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32>;
+    fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32>;
 
-    /// The change output (a.k.a. drain) this metric decides on for the given selection, or
-    /// [`Drain::NONE`] if it decides there should be no change.
+    /// The change output (a.k.a. drain) this metric decides on for the given selection and `target`,
+    /// or [`Drain::NONE`] if it decides there should be no change.
     ///
     /// Call this on a branch-and-bound solution to get the change output the metric optimized against.
-    fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain;
+    fn drain(&mut self, cs: &CoinSelector<'_>, target: Target) -> Drain;
 
     /// Returns whether the metric requies we order candidates by descending value per weight unit.
     fn requires_ordering_by_descending_value_pwu(&self) -> bool {

src/coin_selector.rs

@@ -562,9 +562,10 @@ impl<'a> CoinSelector<'a> {
     /// Most of the time, you would want to use [`CoinSelector::run_bnb`] instead.
     pub fn bnb_solutions<M: BnbMetric>(
         &self,
+        target: Target,
         metric: M,
     ) -> impl Iterator<Item = Option<(CoinSelector<'a>, Ordf32)>> {
-        crate::bnb::BnbIter::new(self.clone(), metric)
+        crate::bnb::BnbIter::new(self.clone(), target, metric)
     }
 
     /// Run branch and bound to minimize the score of the provided [`BnbMetric`].
@@ -576,10 +577,11 @@ impl<'a> CoinSelector<'a> {
     /// Use [`CoinSelector::bnb_solutions`] to access the branch and bound iterator directly.
     pub fn run_bnb<M: BnbMetric>(
         &mut self,
+        target: Target,
         metric: M,
         max_rounds: usize,
     ) -> Result<(Ordf32, Drain), NoBnbSolution> {
-        let mut iter = crate::bnb::BnbIter::new(self.clone(), metric);
+        let mut iter = crate::bnb::BnbIter::new(self.clone(), target, metric);
         let mut rounds = 0_usize;
         let best = iter
             .by_ref()
@@ -588,7 +590,7 @@ impl<'a> CoinSelector<'a> {
             .flatten()
             .last();
         let (selector, score) = best.ok_or(NoBnbSolution { max_rounds, rounds })?;
-        let drain = iter.metric.drain(&selector);
+        let drain = iter.metric.drain(&selector, target);
         *self = selector;
         Ok((score, drain))
     }

src/metrics/changeless.rs

@@ -5,17 +5,11 @@ use crate::{bnb::BnbMetric, float::Ordf32, CoinSelector, Drain, Target};
 /// A selection is scored by `inner` only if the inner metric decides it should *not* have a change
 /// output (see [`BnbMetric::drain`]); otherwise it is treated as invalid. This lets you find, for
 /// example, the lowest-fee changeless solution via `Changeless<LowestFee>`.
-///
-/// `target` must match the target `inner` is optimizing for. It's used only by the branch-pruning
-/// heuristic (to tell which candidates reduce the excess); the change decision and the scoring are
-/// delegated entirely to `inner`.
 #[derive(Clone, Copy, Debug)]
-pub struct Changeless<M> {
-    /// The target of the resultant selection. Must match the target of `inner`.
-    pub target: Target,
+pub struct Changeless<M>(
     /// The inner metric that scores changeless solutions and owns the change decision.
-    pub inner: M,
-}
+    pub M,
+);
 
 impl<M: BnbMetric> Changeless<M> {
     /// Whether every selection reachable down this branch (the current one and any superset of it)
@@ -32,50 +26,50 @@ impl<M: BnbMetric> Changeless<M> {
     /// are next to each other, which [`requires_ordering_by_descending_value_pwu`] guarantees.
     ///
     /// [`requires_ordering_by_descending_value_pwu`]: BnbMetric::requires_ordering_by_descending_value_pwu
-    fn change_unavoidable(&mut self, cs: &CoinSelector<'_>) -> bool {
-        if self.inner.drain(cs).is_none() {
+    fn change_unavoidable(&mut self, cs: &CoinSelector<'_>, target: Target) -> bool {
+        if self.0.drain(cs, target).is_none() {
             return false;
         }
 
         let mut least_excess = cs.clone();
         cs.unselected()
             .rev()
-            .take_while(|(_, wv)| wv.effective_value(self.target.fee.rate) < 0.0)
+            .take_while(|(_, wv)| wv.effective_value(target.fee.rate) < 0.0)
             .for_each(|(index, _)| {
                 least_excess.select(index);
             });
 
-        self.inner.drain(&least_excess).is_some()
+        self.0.drain(&least_excess, target).is_some()
     }
 }
 
 impl<M: BnbMetric> BnbMetric for Changeless<M> {
-    fn drain(&mut self, _cs: &CoinSelector<'_>) -> Drain {
+    fn drain(&mut self, _cs: &CoinSelector<'_>, _target: Target) -> Drain {
         // by definition a changeless selection never has a change output
         Drain::NONE
     }
 
-    fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
+    fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32> {
         // Reject selections that have change. We don't need an explicit target-met check: `inner`
         // returns `None` for invalid (e.g. not-target-met) selections.
         //
         // NOTE: for metrics whose `score` recomputes the drain (e.g. `LowestFee`), this evaluates
         // the drain decision twice per node. Sharing it would mean threading the drain into
         // `score`, which we avoid to keep metrics composable.
-        if self.inner.drain(cs).is_some() {
+        if self.0.drain(cs, target).is_some() {
             return None;
         }
-        self.inner.score(cs)
+        self.0.score(cs, target)
     }
 
-    fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
-        if self.change_unavoidable(cs) {
+    fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32> {
+        if self.change_unavoidable(cs, target) {
             // every descendant has change, so no changeless solution is reachable
             None
         } else {
             // the changeless-constrained optimum is no better than the inner metric's unconstrained
             // optimum, so the inner bound is a valid lower bound
-            self.inner.bound(cs)
+            self.0.bound(cs, target)
         }
     }
 

src/metrics/lowest_fee.rs

@@ -17,8 +17,6 @@ use crate::{float::Ordf32, BnbMetric, CoinSelector, Drain, DrainWeights, FeeRate
 /// dust threshold implied by `dust_relay_feerate`.
 #[derive(Clone, Copy)]
 pub struct LowestFee {
-    /// The target parameters for the resultant selection.
-    pub target: Target,
     /// The estimated feerate needed to spend our change output later.
     pub long_term_feerate: FeeRate,
     /// The feerate used to determine the dust threshold of the change output.
@@ -29,11 +27,11 @@ pub struct LowestFee {
 
 impl LowestFee {
     /// The value the change output should have, or `None` if this selection should be changeless.
-    fn drain_value(&self, cs: &CoinSelector<'_>) -> Option<u64> {
+    fn drain_value(&self, cs: &CoinSelector<'_>, target: Target) -> Option<u64> {
         // The change output pays for its own weight, so the value we'd actually recover is the
         // excess remaining after accounting for that weight.
         let excess_with_drain_weight = cs.excess(
-            self.target,
+            target,
             Drain {
                 weights: self.drain_weights,
                 value: 0,
@@ -60,21 +58,22 @@ impl LowestFee {
 }
 
 impl BnbMetric for LowestFee {
-    fn drain(&mut self, cs: &CoinSelector<'_>) -> Drain {
-        self.drain_value(cs).map_or(Drain::NONE, |value| Drain {
-            weights: self.drain_weights,
-            value,
-        })
+    fn drain(&mut self, cs: &CoinSelector<'_>, target: Target) -> Drain {
+        self.drain_value(cs, target)
+            .map_or(Drain::NONE, |value| Drain {
+                weights: self.drain_weights,
+                value,
+            })
     }
 
-    fn score(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
-        if !cs.is_target_met(self.target) {
+    fn score(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32> {
+        if !cs.is_target_met(target) {
             return None;
         }
 
         let long_term_fee = {
-            let drain = self.drain(cs);
-            let fee_for_the_tx = cs.fee(self.target.value(), drain.value);
+            let drain = self.drain(cs, target);
+            let fee_for_the_tx = cs.fee(target.value(), drain.value);
             assert!(
                 fee_for_the_tx >= 0,
                 "must not be called unless selection has met target: fee={}",
@@ -89,9 +88,9 @@ impl BnbMetric for LowestFee {
         Some(Ordf32(long_term_fee as f32))
     }
 
-    fn bound(&mut self, cs: &CoinSelector<'_>) -> Option<Ordf32> {
-        if cs.is_target_met(self.target) {
-            let current_score = self.score(cs).unwrap();
+    fn bound(&mut self, cs: &CoinSelector<'_>, target: Target) -> Option<Ordf32> {
+        if cs.is_target_met(target) {
+            let current_score = self.score(cs, target).unwrap();
 
             // `current_score` is already a valid lower bound for a selection that has change: a
             // descendant can never lower the fee by removing an existing (worthwhile) change
@@ -112,17 +111,17 @@ impl BnbMetric for LowestFee {
             // `drain_value`, where `change_value` is `excess_with_drain_weight` and `spend_fee` is
             // `drain_spend_cost`). With `v >= 0` the difference is strictly positive: B always
             // costs more.
-            if self.drain_value(cs).is_none() {
+            if self.drain_value(cs, target).is_none() {
                 // But a descendant might *add* a change output that improves the metric. This
                 // happens when the current selection is changeless only because the change would be
                 // dust: a descendant with more excess could clear the dust threshold and recover
                 // value that is currently burned to fees.
                 let cost_of_adding_change = self.drain_weights.waste(
-                    self.target.fee.rate,
+                    target.fee.rate,
                     self.long_term_feerate,
-                    self.target.outputs.n_outputs,
+                    target.outputs.n_outputs,
                 );
-                let cost_of_no_change = cs.excess(self.target, Drain::NONE);
+                let cost_of_no_change = cs.excess(target, Drain::NONE);
 
                 let best_score_with_change =
                     Ordf32(current_score.0 - cost_of_no_change as f32 + cost_of_adding_change);
@@ -137,11 +136,11 @@ impl BnbMetric for LowestFee {
             let (mut cs, resize_index, to_resize) = cs
                 .clone()
                 .select_iter()
-                .find(|(cs, _, _)| cs.is_target_met(self.target))?;
+                .find(|(cs, _, _)| cs.is_target_met(target))?;
 
             // If this selection is already perfect, return its score directly.
-            if cs.excess(self.target, Drain::NONE) == 0 {
-                return Some(self.score(&cs).unwrap());
+            if cs.excess(target, Drain::NONE) == 0 {
+                return Some(self.score(&cs, target).unwrap());
             };
             cs.deselect(resize_index);
 
@@ -162,13 +161,12 @@ impl BnbMetric for LowestFee {
             //
             // In the perfect scenario, no additional fee would be required to pay for rounding up when converting from weight units to
             // vbytes and so all fee calculations below are performed on weight units directly.
-            let rate_excess = cs.rate_excess_wu(self.target, Drain::NONE) as f32;
+            let rate_excess = cs.rate_excess_wu(target, Drain::NONE) as f32;
             let mut scale = Ordf32(0.0);
 
             if rate_excess < 0.0 {
                 let remaining_value_to_reach_feerate = rate_excess.abs();
-                let effective_value_of_resized_input =
-                    to_resize.effective_value(self.target.fee.rate);
+                let effective_value_of_resized_input = to_resize.effective_value(target.fee.rate);
                 if effective_value_of_resized_input > 0.0 {
                     let feerate_scale =
                         remaining_value_to_reach_feerate / effective_value_of_resized_input;
@@ -180,8 +178,8 @@ impl BnbMetric for LowestFee {
 
             // We can use the same approach for replacement we just have to use the
             // incremental_relay_feerate.
-            if let Some(replace) = self.target.fee.replace {
-                let replace_excess = cs.replacement_excess_wu(self.target, Drain::NONE) as f32;
+            if let Some(replace) = target.fee.replace {
+                let replace_excess = cs.replacement_excess_wu(target, Drain::NONE) as f32;
                 if replace_excess < 0.0 {
                     let remaining_value_to_reach_feerate = replace_excess.abs();
                     let effective_value_of_resized_input =
@@ -198,7 +196,7 @@ impl BnbMetric for LowestFee {
             // Handle absolute fee constraint. Unlike feerate and replacement, the
             // absolute fee is a fixed amount (not weight-proportional), so we just
             // need enough raw value to cover the gap.
-            let absolute_excess = cs.absolute_excess(self.target, Drain::NONE) as f32;
+            let absolute_excess = cs.absolute_excess(target, Drain::NONE) as f32;
             if absolute_excess < 0.0 {
                 let remaining = absolute_excess.abs();
                 if to_resize.value > 0 {
@@ -212,7 +210,7 @@ impl BnbMetric for LowestFee {
             // `scale` could be 0 even if `is_target_met` is `false` due to the latter being based on
             // rounded-up vbytes.
             let ideal_fee = scale.0 * to_resize.value as f32 + cs.selected_value() as f32
-                - self.target.value() as f32;
+                - target.value() as f32;
             assert!(ideal_fee >= 0.0);
 
             Some(Ordf32(ideal_fee))