From 0c6669625b7d5c4f1f6c05f8c9d5e496eddcae8d Mon Sep 17 00:00:00 2001 From: LLFourn Date: Thu, 21 Dec 2023 14:46:58 +1100 Subject: [PATCH 1/7] 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 | 65 ++++++++++++++++++++++----------------- src/metrics/changeless.rs | 6 ++-- src/metrics/lowest_fee.rs | 8 ++--- src/metrics/waste.rs | 18 +++++------ tests/bnb.rs | 2 +- tests/common.rs | 2 +- tests/waste.rs | 2 +- 7 files changed, 55 insertions(+), 48 deletions(-) diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 0d42a56..8fd13ad 100644 --- a/src/coin_selector.rs +++ b/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,11 +386,22 @@ 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( @@ -417,7 +409,7 @@ impl<'a> CoinSelector<'a> { 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(), }) diff --git a/src/metrics/changeless.rs b/src/metrics/changeless.rs index 133859e..fec7e6f 100644 --- a/src/metrics/changeless.rs +++ b/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 { - 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)) diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index c9027a1..b8b3e39 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -51,7 +51,7 @@ impl LowestFee { impl BnbMetric for LowestFee { fn score(&mut self, cs: &CoinSelector<'_>) -> Option { 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); diff --git a/src/metrics/waste.rs b/src/metrics/waste.rs index a8f5312..430a504 100644 --- a/src/metrics/waste.rs +++ b/src/metrics/waste.rs @@ -33,7 +33,7 @@ pub struct Waste { impl BnbMetric for Waste { fn score(&mut self, cs: &CoinSelector<'_>) -> Option { 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); diff --git a/tests/bnb.rs b/tests/bnb.rs index 686e107..d39a77a 100644 --- a/tests/bnb.rs +++ b/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)), } } diff --git a/tests/common.rs b/tests/common.rs index 2b72c07..fad3baa 100644 --- a/tests/common.rs +++ b/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, diff --git a/tests/waste.rs b/tests/waste.rs index f67145a..4fcb390 100644 --- a/tests/waste.rs +++ b/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 { From 0aef6ff523784003c6d656f110cf007bdda89507 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Wed, 20 Dec 2023 09:39:45 +1100 Subject: [PATCH 2/7] Make lowest fee test fail by implementing score correctly --- src/coin_selector.rs | 13 +++++++++++++ src/metrics/lowest_fee.rs | 28 +++++++++++++++++++--------- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 8fd13ad..891f6dd 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -264,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 @@ -656,6 +664,11 @@ impl DrainWeights { + self.spend_weight as f32 * long_term_feerate.spwu() } + /// The the fee you will pay to spend this otuput 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 with a taproot keyspend. pub fn new_tr_keyspend() -> Self { Self { diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index b8b3e39..5c82df1 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -6,11 +6,15 @@ use crate::{ /// Metric that aims to minimize transaction fees. The future fee for spending the change output is /// included in this calculation. /// -/// The scoring function for changeless solutions is: -/// > input_weight * feerate + excess +/// The fee is simply: /// -/// The scoring function for solutions with change: -/// > (input_weight + change_output_weight) * feerate + change_spend_weight * long_term_feerate +/// > `inputs - outputs` where `outputs = target.value + change_value` +/// +/// But the total value includes the cost of spending the change output if it exists: +/// +/// > `change_spend_weight * long_term_feerate` +/// +/// The `change_spend_weight` and `change_value` are determined by the `change_policy` #[derive(Clone, Copy)] pub struct LowestFee { /// The target parameters for the resultant selection. @@ -55,13 +59,19 @@ impl BnbMetric for LowestFee { return None; } - let drain_weights = if drain.is_some() { - Some(drain.weights) - } else { - None + let long_term_fee = { + let fee_for_the_tx = cs.fee(self.target.value, drain.value); + assert!( + fee_for_the_tx > 0, + "must not be called unless selection has met target" + ); + // Why `spend_fee` rounds up here. We could use floats but I felt it was just better to + // accept the extra 1 sat penality to having a change output + let fee_for_spending_drain = drain.weights.spend_fee(self.long_term_feerate); + fee_for_the_tx as u64 + fee_for_spending_drain }; - Some(Ordf32(self.calc_metric(cs, drain_weights))) + Some(Ordf32(long_term_fee as f32)) } fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { From e30246dd9156150c3dcca2b499145957ec8055b0 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Fri, 22 Dec 2023 14:25:05 +1100 Subject: [PATCH 3/7] Fix lowest_fee metric --- README.md | 19 ++-- src/coin_selector.rs | 19 ++-- src/metrics.rs | 2 +- src/metrics/lowest_fee.rs | 197 ++++++++++++++++---------------------- src/metrics/waste.rs | 9 +- tests/changeless.rs | 2 +- tests/lowest_fee.rs | 3 +- tests/waste.rs | 2 +- 8 files changed, 114 insertions(+), 139 deletions(-) diff --git a/README.md b/README.md index 048a0cd..8969430 100644 --- a/README.md +++ b/README.md @@ -235,20 +235,23 @@ let candidates = candidate_txouts .collect::>(); 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::>(); 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) diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 891f6dd..b62fd8f 100644 --- a/src/coin_selector.rs +++ b/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, } } } diff --git a/src/metrics.rs b/src/metrics.rs index 35cd757..f78ba9e 100644 --- a/src/metrics.rs +++ b/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); }); diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index 5c82df1..4f1cf2f 100644 --- a/src/metrics/lowest_fee.rs +++ b/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) -> 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) -> 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 { - 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,89 +48,77 @@ impl BnbMetric for LowestFee { } fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { - // 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 { @@ -165,8 +126,18 @@ impl BnbMetric for LowestFee { } } -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) } diff --git a/src/metrics/waste.rs b/src/metrics/waste.rs index 430a504..57ce72a 100644 --- a/src/metrics/waste.rs +++ b/src/metrics/waste.rs @@ -80,7 +80,7 @@ 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(); @@ -88,7 +88,7 @@ impl BnbMetric for Waste { 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(); diff --git a/tests/changeless.rs b/tests/changeless.rs index 06e085e..d06e063 100644 --- a/tests/changeless.rs +++ b/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 diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 58d000c..0c4102b 100644 --- a/tests/lowest_fee.rs +++ b/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); } diff --git a/tests/waste.rs b/tests/waste.rs index 4fcb390..6b7ee81 100644 --- a/tests/waste.rs +++ b/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); From 17cc8f234430720a734e42cef28f8f22fc82a766 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Fri, 22 Dec 2023 12:08:43 +1100 Subject: [PATCH 4/7] Score branches before adding children It may improve best score allowing us to avoid ever adding the children to the heap if their bound function doesn't think they can improve on it. --- src/bnb.rs | 36 +++++++++++++++++++----------------- tests/bnb.rs | 2 +- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/bnb.rs b/src/bnb.rs index 601f5cf..5df7754 100644 --- a/src/bnb.rs +++ b/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(), diff --git a/tests/bnb.rs b/tests/bnb.rs index d39a77a..40776de 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -100,7 +100,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() { .last() .expect("it found a solution"); - assert_eq!(rounds, 50180); + assert_eq!(rounds, 50168); assert_eq!(best.input_weight(), solution_weight); assert_eq!(best.selected_value(), target.value, "score={:?}", score); } From 73600522abd9497eb4950261c2151b4cf91c413c Mon Sep 17 00:00:00 2001 From: LLFourn Date: Fri, 22 Dec 2023 14:12:02 +1100 Subject: [PATCH 5/7] Write lowest fee test that hits important branch That proptest is not hitting for some reason. --- src/metrics/lowest_fee.rs | 4 +- tests/lowest_fee.rs | 84 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index 4f1cf2f..18fc7ad 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -67,7 +67,7 @@ impl BnbMetric for LowestFee { 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 { + 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(); @@ -85,7 +85,7 @@ impl BnbMetric for LowestFee { .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, + current_score.0 + cost_of_getting_rid_of_change - cost_of_change, ); if best_score_without_change < current_score { return Some(best_score_without_change); diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 0c4102b..1ac0c7d 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -2,8 +2,8 @@ mod common; use bdk_coin_select::metrics::{Changeless, LowestFee}; -use bdk_coin_select::ChangePolicy; use bdk_coin_select::{BnbMetric, Candidate, CoinSelector}; +use bdk_coin_select::{ChangePolicy, Drain, DrainWeights, FeeRate, Target}; use proptest::prelude::*; proptest! { @@ -160,3 +160,85 @@ fn combined_changeless_metric() { assert!(combined_rounds >= rounds); } + +/// This test considers the case where you could actually lower your long term fee by adding another input. +#[test] +fn adding_another_input_to_remove_change() { + let target = Target { + feerate: FeeRate::from_sat_per_vb(1.0), + min_fee: 0, + value: 99_870, + }; + + let candidates = vec![ + Candidate { + value: 100_000, + weight: 100, + input_count: 1, + is_segwit: true, + }, + Candidate { + value: 50_000, + weight: 100, + input_count: 1, + is_segwit: true, + }, + // NOTE: this input has negative effective value + Candidate { + value: 10, + weight: 100, + input_count: 1, + is_segwit: true, + }, + ]; + + let mut cs = CoinSelector::new(&candidates, 200); + + let best_solution = { + let mut cs = cs.clone(); + cs.select(0); + cs.select(2); + cs.excess(target, Drain::none()); + assert!(cs.is_target_met(target)); + cs + }; + + let drain_weights = DrainWeights { + output_weight: 100, + spend_weight: 1_000, + }; + + let excess_to_make_first_candidate_satisfy_but_have_change = { + let mut cs = cs.clone(); + cs.select(0); + assert!(cs.is_target_met(target)); + let with_change_excess = cs.excess( + target, + Drain { + value: 0, + weights: drain_weights, + }, + ); + assert!(with_change_excess > 0); + with_change_excess as u64 + }; + + let change_policy = ChangePolicy { + min_value: excess_to_make_first_candidate_satisfy_but_have_change - 10, + drain_weights, + }; + + let mut metric = LowestFee { + target, + long_term_feerate: FeeRate::from_sat_per_vb(1.0), + change_policy, + }; + + let (score, _) = common::bnb_search(&mut cs, metric, 10).expect("finds solution"); + let best_solution_score = metric.score(&best_solution).expect("must be a solution"); + + assert_eq!(best_solution.drain(target, change_policy), Drain::none()); + + assert!(score <= best_solution_score); + assert_eq!(cs.selected_indices(), best_solution.selected_indices()); +} From 9e1cecd47dc369a863999696266f303940d9b45e Mon Sep 17 00:00:00 2001 From: LLFourn Date: Fri, 22 Dec 2023 17:04:21 +1100 Subject: [PATCH 6/7] Use ChangePolicy::min_value in lowest fee tests If we only add a drain when it wouldn't increase waste then it is impossible to decrease waste by adding a new input -- but the logic of the bound function has to account for this situation. So by using min_value_and_waste we never test it! --- tests/lowest_fee.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 1ac0c7d..4d1effd 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -26,7 +26,7 @@ proptest! { ) { let params = common::StrategyParams { n_candidates, target_value, base_weight, min_fee, feerate, feerate_lt_diff, drain_weight, drain_spend_weight, drain_dust }; let candidates = common::gen_candidates(params.n_candidates); - let change_policy = ChangePolicy::min_value_and_waste(params.drain_weights(), params.drain_dust, params.feerate(), params.long_term_feerate()); + let change_policy = ChangePolicy::min_value(params.drain_weights(), params.drain_dust); let metric = LowestFee { target: params.target(), long_term_feerate: params.long_term_feerate(), change_policy }; common::can_eventually_find_best_solution(params, candidates, change_policy, metric)?; } @@ -46,7 +46,7 @@ proptest! { ) { let params = common::StrategyParams { n_candidates, target_value, base_weight, min_fee, feerate, feerate_lt_diff, drain_weight, drain_spend_weight, drain_dust }; let candidates = common::gen_candidates(params.n_candidates); - let change_policy = ChangePolicy::min_value_and_waste(params.drain_weights(), params.drain_dust, params.feerate(), params.long_term_feerate()); + let change_policy = ChangePolicy::min_value(params.drain_weights(), params.drain_dust); let metric = LowestFee { target: params.target(), long_term_feerate: params.long_term_feerate(), change_policy }; common::ensure_bound_is_not_too_tight(params, candidates, change_policy, metric)?; } @@ -90,11 +90,9 @@ proptest! { let mut cs = CoinSelector::new(&candidates, params.base_weight); - let change_policy = ChangePolicy::min_value_and_waste( + let change_policy = ChangePolicy::min_value( params.drain_weights(), params.drain_dust, - params.feerate(), - params.long_term_feerate(), ); let metric = LowestFee { @@ -128,12 +126,7 @@ fn combined_changeless_metric() { let mut cs_a = CoinSelector::new(&candidates, params.base_weight); let mut cs_b = CoinSelector::new(&candidates, params.base_weight); - let change_policy = ChangePolicy::min_value_and_waste( - params.drain_weights(), - params.drain_dust, - params.feerate(), - params.long_term_feerate(), - ); + let change_policy = ChangePolicy::min_value(params.drain_weights(), params.drain_dust); let metric_lowest_fee = LowestFee { target: params.target(), From 6ae0fdf0aa03cdd31e317ef5a05d4fc2552139ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=BF=97=E5=AE=87?= Date: Tue, 16 Jan 2024 00:47:14 +0800 Subject: [PATCH 7/7] docs: fix typos and use better wording --- README.md | 2 +- src/coin_selector.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8969430..e37ed6d 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,7 @@ assert_eq!(selected_coins.len(), 1); let drain = selector.drain(target, change_policy); if drain.is_some() { - // add our change outupt to the transaction + // add our change output to the transaction let change_value = drain.value; } ``` diff --git a/src/coin_selector.rs b/src/coin_selector.rs index b62fd8f..b084e38 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -666,7 +666,7 @@ impl DrainWeights { + self.spend_weight as f32 * long_term_feerate.spwu() } - /// The the fee you will pay to spend this otuput in the future. + /// 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 }