From 7a282883b8009d4599133450a9c31eaf5689f64d Mon Sep 17 00:00:00 2001 From: LLFourn Date: Wed, 24 Jan 2024 15:17:06 +1100 Subject: [PATCH 1/2] Implement proper RBF logic satisfying rule 4 Removes `min_fee` constraint in favour of proper rule 4 handling (min_fee was pretty useless). See for rule 4: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy LowestFee BnB metric updated to bound correctly for this. A few other coin_selector API changes --- CHANGELOG.md | 7 + README.md | 52 +++-- src/coin_selector.rs | 298 ++++++++++---------------- src/drain.rs | 74 +++++++ src/feerate.rs | 16 +- src/lib.rs | 4 + src/metrics.rs | 2 +- src/metrics/lowest_fee.rs | 93 +++++--- src/metrics/waste.rs | 18 +- src/target.rs | 86 ++++++++ tests/bnb.rs | 19 +- tests/changeless.proptest-regressions | 2 + tests/changeless.rs | 23 +- tests/common.rs | 149 +++++++++++-- tests/lowest_fee.rs | 42 +++- tests/rbf.rs | 69 ++++++ tests/waste.rs | 34 +-- tests/weight.rs | 3 + 18 files changed, 672 insertions(+), 319 deletions(-) create mode 100644 CHANGELOG.md create mode 100644 src/drain.rs create mode 100644 src/target.rs create mode 100644 tests/rbf.rs diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..2ee2cc2 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,7 @@ +# Unreleased + +- Remove `is_target_met_with_change_policy`: it was redundant. If the target is met without a change policy it will always be met with it. +- Remove `min_fee` in favour of `replace` which allows you to replace a transaction +- Remove `Drain` argument from `CoinSelector::select_until_target_met` because adding a drain won't + change when the target is met. + diff --git a/README.md b/README.md index e37ed6d..db85e72 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,6 @@ output(s). The other is the weight of spending the drain output later on (the in use std::str::FromStr; use bdk_coin_select::{CoinSelector, Candidate, DrainWeights, TXIN_BASE_WEIGHT, ChangePolicy, TR_KEYSPEND_TXIN_WEIGHT}; use bitcoin::{Address, Network, Transaction, TxIn, TxOut}; -const TR_SATISFACTION_WEIGHT: u32 = 66; let base_tx = Transaction { input: vec![], output: vec![/* include your recipient outputs here */], @@ -129,19 +128,37 @@ Built-in metrics are provided in the [`metrics`] submodule. Currently, only the [`LowestFee`](metrics::LowestFee) metric is considered stable. ```rust -use bdk_coin_select::{ Candidate, CoinSelector, FeeRate, Target, ChangePolicy }; +use bdk_coin_select::{ Candidate, CoinSelector, FeeRate, Target, TargetFee, ChangePolicy, TR_KEYSPEND_TXIN_WEIGHT }; use bdk_coin_select::metrics::LowestFee; -let candidates = []; +let candidates = [ + Candidate { + input_count: 1, + value: 400_000, + weight: TR_KEYSPEND_TXIN_WEIGHT, + is_segwit: true + }, + Candidate { + input_count: 1, + value: 200_000, + weight: TR_KEYSPEND_TXIN_WEIGHT, + is_segwit: true + }, + Candidate { + input_count: 1, + value: 11_000, + weight: TR_KEYSPEND_TXIN_WEIGHT, + is_segwit: true + } +]; let base_weight = 0; let drain_weights = bdk_coin_select::DrainWeights::default(); let dust_limit = 0; -let long_term_feerate = FeeRate::default_min_relay_fee(); +let long_term_feerate = FeeRate::from_sat_per_vb(10.0); let mut coin_selector = CoinSelector::new(&candidates, base_weight); let target = Target { - feerate: FeeRate::default_min_relay_fee(), - min_fee: 0, + fee: TargetFee::from_feerate(FeeRate::from_sat_per_vb(15.0)), value: 210_000, }; @@ -151,7 +168,7 @@ let target = Target { let change_policy = ChangePolicy::min_value_and_waste( drain_weights, dust_limit, - target.feerate, + target.fee.rate, long_term_feerate, ); @@ -166,7 +183,11 @@ let metric = LowestFee { // We run the branch and bound algorithm with a max round limit of 100,000. match coin_selector.run_bnb(metric, 100_000) { - Err(err) => println!("failed to find a solution: {}", err), + 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!"); + } Ok(score) => { println!("we found a solution with score {}", score); @@ -179,6 +200,7 @@ match coin_selector.run_bnb(metric, 100_000) { println!("We are including a change output of {} value (0 means not change)", change.value); } }; + ``` ## Finalizing a Selection @@ -195,7 +217,7 @@ match coin_selector.run_bnb(metric, 100_000) { use bdk_coin_select::{CoinSelector, Candidate, DrainWeights, Target, ChangePolicy, TR_KEYSPEND_TXIN_WEIGHT, Drain}; use bitcoin::{Amount, TxOut, Address}; let base_weight = 0_u32; -let drain_weights = DrainWeights::new_tr_keyspend(); +let drain_weights = DrainWeights::TR_KEYSPEND; use core::str::FromStr; // A random target, as an example. @@ -222,7 +244,7 @@ let candidate_txouts = vec![ script_pubkey: Address::from_str("bc1p0d0rhyynq0awa9m8cqrcr8f5nxqx3aw29w4ru5u9my3h0sfygnzs9khxz8").unwrap().payload.script_pubkey() } ]; -// We transform the candidate txouts into something `CoinSelector` can +// We transform the candidate txouts into something `CoinSelector` can // understand. let candidates = candidate_txouts .iter() @@ -236,7 +258,7 @@ let candidates = candidate_txouts let mut selector = CoinSelector::new(&candidates, base_weight); selector - .select_until_target_met(target, Drain::none()) + .select_until_target_met(target) .expect("we've got enough coins"); // Get a list of coins that are selected. @@ -256,11 +278,5 @@ if drain.is_some() { # Minimum Supported Rust Version (MSRV) -This library is tested to compile on 1.54 +This library is compiles on rust v1.54 and above -To build with the MSRV, you will need to pin the following dependencies: - -```shell -# tempfile 3.7.0 has MSRV 1.63.0+ -cargo update -p tempfile --precise "3.6.0" -``` diff --git a/src/coin_selector.rs b/src/coin_selector.rs index b084e38..d082410 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -1,7 +1,9 @@ use super::*; #[allow(unused)] // some bug in <= 1.48.0 sees this as unused when it isn't use crate::float::FloatExt; -use crate::{bnb::BnbMetric, change_policy::ChangePolicy, float::Ordf32, FeeRate}; +use crate::{ + bnb::BnbMetric, change_policy::ChangePolicy, float::Ordf32, FeeRate, Target, TargetFee, +}; use alloc::{borrow::Cow, collections::BTreeSet, vec::Vec}; /// [`CoinSelector`] selects/deselects coins from a set of canididate coins. @@ -20,27 +22,6 @@ pub struct CoinSelector<'a> { candidate_order: Cow<'a, Vec>, } -/// A target value to select for along with feerate constraints. -#[derive(Debug, Clone, Copy)] -pub struct Target { - /// The minimum feerate that the selection must have - pub feerate: FeeRate, - /// The minimum fee the selection must have - pub min_fee: u64, - /// The minmum value that should be left for the output - pub value: u64, -} - -impl Default for Target { - fn default() -> Self { - Self { - feerate: FeeRate::default_min_relay_fee(), - min_fee: 0, - value: 0, - } - } -} - impl<'a> CoinSelector<'a> { /// Creates a new coin selector from some candidate inputs and a `base_weight`. /// @@ -171,7 +152,7 @@ impl<'a> CoinSelector<'a> { /// [`ban`]: Self::ban pub fn is_selection_possible(&self, target: Target) -> bool { let mut test = self.clone(); - test.select_all_effective(target.feerate); + test.select_all_effective(target.fee.rate); test.is_target_met(target) } @@ -223,41 +204,60 @@ impl<'a> CoinSelector<'a> { /// /// In order for the resulting transaction to be valid this must be 0. pub fn excess(&self, target: Target, drain: Drain) -> i64 { - self.selected_value() as i64 - - target.value as i64 - - drain.value as i64 - - self.implied_fee(target.feerate, target.min_fee, drain.weights.output_weight) as i64 + self.rate_excess(target, drain) + .min(self.replacement_excess(target, drain)) } - /// How much the current selection overshoots the value need to satisfy `target.feerate` and + /// How much the current selection overshoots the value need to satisfy `target.fee.rate` and /// `target.value` (while ignoring `target.min_fee`). pub fn rate_excess(&self, target: Target, drain: Drain) -> i64 { self.selected_value() as i64 - target.value as i64 - drain.value as i64 - - self.implied_fee_from_feerate(target.feerate, drain.weights.output_weight) as i64 + - self.implied_fee_from_feerate(target.fee.rate, drain.weights.output_weight) as i64 } - /// How much the current selection overshoots the value needed to satisfy `target.min_fee` and - /// `target.value` (while ignoring `target.feerate`). - pub fn absolute_excess(&self, target: Target, drain: Drain) -> i64 { + /// How much the current selection overshoots the value needed to satisfy RBF's rule 4. + pub fn replacement_excess(&self, target: Target, drain: Drain) -> i64 { + let mut replacement_excess_needed = 0; + if let Some(replace) = target.fee.replace { + replacement_excess_needed = + replace.min_fee_to_do_replacement(self.weight(drain.weights.output_weight)) + } self.selected_value() as i64 - target.value as i64 - drain.value as i64 - - target.min_fee as i64 + - replacement_excess_needed as i64 } /// The feerate the transaction would have if we were to use this selection of inputs to achieve /// the `target_value`. - pub fn implied_feerate(&self, target_value: u64, drain: Drain) -> FeeRate { + /// + /// Returns `None` if the feerate would be negative or infinity. + pub fn implied_feerate(&self, target_value: u64, drain: Drain) -> Option { let numerator = self.selected_value() as i64 - target_value as i64 - drain.value as i64; let denom = self.weight(drain.weights.output_weight); - FeeRate::from_sat_per_wu(numerator as f32 / denom as f32) + if numerator < 0 || denom == 0 { + return None; + } + Some(FeeRate::from_sat_per_wu(numerator as f32 / denom as f32)) + } + + /// The fee the current selection and `drain_weight` should pay to satisfy `target_fee`. + /// + /// `drain_weight` can be 0 to indicate no draining output + pub fn implied_fee(&self, target_fee: TargetFee, drain_weight: u32) -> u64 { + let mut implied_fee = self.implied_fee_from_feerate(target_fee.rate, drain_weight); + + if let Some(replace) = target_fee.replace { + implied_fee = implied_fee.max(self.implied_fee_from_replace(replace, drain_weight)); + } + + implied_fee } - /// The fee the current selection should pay to reach `feerate` and provide `min_fee` - fn implied_fee(&self, feerate: FeeRate, min_fee: u64, drain_weight: u32) -> u64 { - (self.implied_fee_from_feerate(feerate, drain_weight)).max(min_fee) + fn implied_fee_from_replace(&self, replace: Replace, drain_weight: u32) -> u64 { + replace.min_fee_to_do_replacement(self.weight(drain_weight)) } fn implied_fee_from_feerate(&self, feerate: FeeRate, drain_weight: u32) -> u64 { @@ -337,7 +337,7 @@ impl<'a> CoinSelector<'a> { excess_discount: f32, ) -> f32 { debug_assert!((0.0..=1.0).contains(&excess_discount)); - let mut waste = self.input_waste(target.feerate, long_term_feerate); + let mut waste = self.input_waste(target.fee.rate, long_term_feerate); if drain.is_none() { // We don't allow negative excess waste since negative excess just means you haven't @@ -348,7 +348,7 @@ impl<'a> CoinSelector<'a> { excess_waste *= excess_discount.max(0.0).min(1.0); waste += excess_waste; } else { - waste += drain.weights.output_weight as f32 * target.feerate.spwu() + waste += drain.weights.output_weight as f32 * target.fee.rate.spwu() + drain.weights.spend_weight as f32 * long_term_feerate.spwu(); } @@ -412,16 +412,6 @@ impl<'a> CoinSelector<'a> { 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_with_drain(target, self.drain(target, change_policy)) - } - /// Select all unselected candidates pub fn select_all(&mut self) { loop { @@ -462,8 +452,8 @@ impl<'a> CoinSelector<'a> { } /// 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`]. + /// `change_policy`. If it shouldn't then it will return a [`Drain::none`]. 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. @@ -499,15 +489,11 @@ impl<'a> CoinSelector<'a> { /// Select candidates until `target` has been met assuming the `drain` output is attached. /// - /// Returns an `Some(_)` if it was able to meet the target. - pub fn select_until_target_met( - &mut self, - target: Target, - drain: Drain, - ) -> Result<(), InsufficientFunds> { - self.select_until(|cs| cs.is_target_met_with_drain(target, drain)) + /// Returns an error if the target was unable to be met. + pub fn select_until_target_met(&mut self, target: Target) -> Result<(), InsufficientFunds> { + self.select_until(|cs| cs.is_target_met(target)) .ok_or_else(|| InsufficientFunds { - missing: self.excess(target, drain).unsigned_abs(), + missing: self.excess(target, Drain::none()).unsigned_abs(), }) } @@ -595,129 +581,6 @@ impl<'a> core::fmt::Display for CoinSelector<'a> { } } -/// A `Candidate` represents an input candidate for [`CoinSelector`]. -/// -/// This can either be a single UTXO, or a group of UTXOs that should be spent together. -#[derive(Debug, Clone, Copy)] -pub struct Candidate { - /// Total value of the UTXO(s) that this [`Candidate`] represents. - pub value: u64, - /// Total weight of including this/these UTXO(s). - /// `txin` fields: `prevout`, `nSequence`, `scriptSigLen`, `scriptSig`, `scriptWitnessLen`, - /// `scriptWitness` should all be included. - pub weight: u32, - /// Total number of inputs; so we can calculate extra `varint` weight due to `vin` len changes. - pub input_count: usize, - /// Whether this [`Candidate`] contains at least one segwit spend. - pub is_segwit: bool, -} - -impl Candidate { - /// Create a [`Candidate`] input that spends a single taproot keyspend output. - pub fn new_tr_keyspend(value: u64) -> Self { - let weight = TXIN_BASE_WEIGHT + TR_KEYSPEND_SATISFACTION_WEIGHT; - Self::new(value, weight, true) - } - - /// Create a new [`Candidate`] that represents a single input. - /// - /// `satisfaction_weight` is the weight of `scriptSigLen + scriptSig + scriptWitnessLen + - /// scriptWitness`. - pub fn new(value: u64, satisfaction_weight: u32, is_segwit: bool) -> Candidate { - let weight = TXIN_BASE_WEIGHT + satisfaction_weight; - Candidate { - value, - weight, - input_count: 1, - is_segwit, - } - } - - /// Effective value of this input candidate: `actual_value - input_weight * feerate (sats/wu)`. - 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) -> f32 { - self.value as f32 / self.weight as f32 - } -} - -/// Represents the weight costs of a drain (a.k.a. change) output. -/// -/// May also represent multiple outputs. -#[derive(Default, Debug, Clone, Copy, Hash, PartialEq, Eq)] -pub struct DrainWeights { - /// The weight of including this drain output. - /// - /// This must take into account the weight change from varint output count. - pub output_weight: u32, - /// The weight of spending this drain output (in the future). - pub spend_weight: u32, -} - -impl DrainWeights { - /// The waste of adding this drain to a transaction according to the [waste metric]. - /// - /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection - pub fn waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 { - self.output_weight as f32 * feerate.spwu() - + self.spend_weight as f32 * long_term_feerate.spwu() - } - - /// The fee you will pay to spend these change output(s) in the future. - pub fn spend_fee(&self, long_term_feerate: FeeRate) -> u64 { - (self.spend_weight as f32 * long_term_feerate.spwu()).ceil() as u64 - } - - /// Create [`DrainWeights`] that represents a drain output that will be spent with a taproot - /// keyspend - pub fn new_tr_keyspend() -> Self { - Self { - output_weight: TXOUT_BASE_WEIGHT + TR_SPK_WEIGHT, - spend_weight: TR_KEYSPEND_TXIN_WEIGHT, - } - } -} - -/// A drain (A.K.A. change) output. -/// Technically it could represent multiple outputs. -/// -/// This is returned from [`CoinSelector::drain`]. Note if `drain` returns a drain where `is_none()` -/// returns true then **no change should be added** to the transaction. -#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Default)] -pub struct Drain { - /// Weight of adding drain output and spending the drain output. - pub weights: DrainWeights, - /// The value that should be assigned to the drain. - pub value: u64, -} - -impl Drain { - /// A drian representing no drain at all. - pub fn none() -> Self { - Self::default() - } - - /// is the "none" drain - pub fn is_none(&self) -> bool { - self == &Drain::none() - } - - /// Is not the "none" drain - pub fn is_some(&self) -> bool { - !self.is_none() - } - - /// The waste of adding this drain to a transaction according to the [waste metric]. - /// - /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection - pub fn waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 { - self.weights.waste(feerate, long_term_feerate) - } -} - /// The `SelectIter` allows you to select candidates by calling [`Iterator::next`]. /// /// The [`Iterator::Item`] is a tuple of `(selector, last_selected_index, last_selected_candidate)`. @@ -780,3 +643,72 @@ impl core::fmt::Display for NoBnbSolution { #[cfg(feature = "std")] impl std::error::Error for NoBnbSolution {} + +/// A `Candidate` represents an input candidate for [`CoinSelector`]. +/// +/// This can either be a single UTXO, or a group of UTXOs that should be spent together. +#[derive(Debug, Clone, Copy)] +pub struct Candidate { + /// Total value of the UTXO(s) that this [`Candidate`] represents. + pub value: u64, + /// Total weight of including this/these UTXO(s). + /// `txin` fields: `prevout`, `nSequence`, `scriptSigLen`, `scriptSig`, `scriptWitnessLen`, + /// `scriptWitness` should all be included. + pub weight: u32, + /// Total number of inputs; so we can calculate extra `varint` weight due to `vin` len changes. + pub input_count: usize, + /// Whether this [`Candidate`] contains at least one segwit spend. + pub is_segwit: bool, +} + +impl Candidate { + /// Create a [`Candidate`] input that spends a single taproot keyspend output. + pub fn new_tr_keyspend(value: u64) -> Self { + let weight = TXIN_BASE_WEIGHT + TR_KEYSPEND_SATISFACTION_WEIGHT; + Self::new(value, weight, true) + } + + /// Create a new [`Candidate`] that represents a single input. + /// + /// `satisfaction_weight` is the weight of `scriptSigLen + scriptSig + scriptWitnessLen + + /// scriptWitness`. + pub fn new(value: u64, satisfaction_weight: u32, is_segwit: bool) -> Candidate { + let weight = TXIN_BASE_WEIGHT + satisfaction_weight; + Candidate { + value, + weight, + input_count: 1, + is_segwit, + } + } + + /// Effective value of this input candidate: `actual_value - input_weight * feerate (sats/wu)`. + 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) -> f32 { + self.value as f32 / self.weight as f32 + } + + /// The amount of *effective value* you receive per weight unit from adding this candidate as an + /// input. + pub fn effective_value_pwu(&self, feerate: FeeRate) -> f32 { + self.value_pwu() - feerate.spwu() + } + + /// The (minimum) fee you'd have to pay to add this input to a transaction as implied by the + /// `feerate`. + pub fn implied_fee(&self, feerate: FeeRate) -> f32 { + self.weight as f32 * feerate.spwu() + } + + /// The amount of fee you have to pay per satoshi of value you add from this input. + /// + /// The value is always positive but values below 1.0 mean the input has negative [*effective + /// value*](Self::effective_value) at this `feerate`. + pub fn fee_per_value(&self, feerate: FeeRate) -> f32 { + self.implied_fee(feerate) / self.value as f32 + } +} diff --git a/src/drain.rs b/src/drain.rs new file mode 100644 index 0000000..e1871df --- /dev/null +++ b/src/drain.rs @@ -0,0 +1,74 @@ +use crate::{FeeRate, TR_KEYSPEND_TXIN_WEIGHT, TR_SPK_WEIGHT, TXOUT_BASE_WEIGHT}; + +/// Represents the weight costs of a drain (a.k.a. change) output. +/// +/// May also represent multiple outputs. +#[derive(Default, Debug, Clone, Copy, Hash, PartialEq, Eq)] +pub struct DrainWeights { + /// The weight of including this drain output. + /// + /// This must take into account the weight change from varint output count. + pub output_weight: u32, + /// The weight of spending this drain output (in the future). + pub spend_weight: u32, +} + +impl DrainWeights { + /// `DrainWeights` that represents a drain output that will be spent with a taproot keyspend + pub const TR_KEYSPEND: Self = Self { + output_weight: TXOUT_BASE_WEIGHT + TR_SPK_WEIGHT, + spend_weight: TR_KEYSPEND_TXIN_WEIGHT, + }; + + /// The waste of adding this drain to a transaction according to the [waste metric]. + /// + /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection + pub fn waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 { + self.output_weight as f32 * feerate.spwu() + + self.spend_weight as f32 * long_term_feerate.spwu() + } + + /// 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 + } +} + +/// A drain (A.K.A. change) output. +/// Technically it could represent multiple outputs. +/// +/// This is returned from [`CoinSelector::drain`]. Note if `drain` returns a drain where `is_none()` +/// returns true then **no change should be added** to the transaction. +/// +/// [`CoinSelector::drain`]: crate::CoinSelector::drain +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Default)] +pub struct Drain { + /// Weight of adding drain output and spending the drain output. + pub weights: DrainWeights, + /// The value that should be assigned to the drain. + pub value: u64, +} + +impl Drain { + /// A drian representing no drain at all. + pub fn none() -> Self { + Self::default() + } + + /// is the "none" drain + pub fn is_none(&self) -> bool { + self == &Drain::none() + } + + /// Is not the "none" drain + pub fn is_some(&self) -> bool { + !self.is_none() + } + + /// The waste of adding this drain to a transaction according to the [waste metric]. + /// + /// [waste metric]: https://bitcoin.stackexchange.com/questions/113622/what-does-waste-metric-mean-in-the-context-of-coin-selection + pub fn waste(&self, feerate: FeeRate, long_term_feerate: FeeRate) -> f32 { + self.weights.waste(feerate, long_term_feerate) + } +} diff --git a/src/feerate.rs b/src/feerate.rs index 474c8c6..e7570e5 100644 --- a/src/feerate.rs +++ b/src/feerate.rs @@ -7,6 +7,16 @@ use core::ops::{Add, Sub}; pub struct FeeRate(Ordf32); impl FeeRate { + /// A feerate of zero + pub const ZERO: Self = Self(Ordf32(0.0)); + /// The default minimum relay fee that bitcoin core uses (1 sat per vbyte). The feerate your transaction has must + /// be at least this to be forwarded by most nodes on the network. + pub const DEFAULT_MIN_RELAY: Self = Self(Ordf32(0.25)); + /// The defualt incremental relay fee that bitcoin core uses (1 sat per vbyte). You must pay + /// this fee over the fee of the transaction(s) you are replacing by through the replace-by-fee + /// mechanism. This feerate is applied to the transaction that is replacing the old + /// transactions. + pub const DEFUALT_RBF_INCREMENTAL_RELAY: Self = Self(Ordf32(0.25)); /// Create a new instance checking the value provided /// /// ## Panics @@ -28,11 +38,6 @@ impl FeeRate { Self::new_checked(btc_per_kvb * 1e5 / 4.0) } - /// A feerate of zero - pub fn zero() -> Self { - Self(Ordf32(0.0)) - } - /// Create a new instance of [`FeeRate`] given a float fee rate in satoshi/vbyte /// /// ## Panics @@ -43,6 +48,7 @@ impl FeeRate { } /// Create a new [`FeeRate`] with the default min relay fee value + #[deprecated(note = "use the DEFAULT_MIN_RELAY constant instead")] pub const fn default_min_relay_fee() -> Self { Self(Ordf32(0.25)) } diff --git a/src/lib.rs b/src/lib.rs index 68dca23..1106cbc 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -24,6 +24,10 @@ mod feerate; pub use feerate::*; mod change_policy; pub use change_policy::*; +mod target; +pub use target::*; +mod drain; +pub use drain::*; /// Txin "base" fields include `outpoint` (32+4) and `nSequence` (4) and 1 byte for the scriptSig /// length. diff --git a/src/metrics.rs b/src/metrics.rs index f78ba9e..8fb0d03 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) < 0.0) + .take_while(|(_, wv)| wv.effective_value(target.fee.rate) < 0.0) .for_each(|(index, _)| { least_excess.select(index); }); diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index 18fc7ad..d88e75a 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -1,6 +1,5 @@ use crate::{ - change_policy::ChangePolicy, float::Ordf32, BnbMetric, Candidate, CoinSelector, Drain, FeeRate, - Target, + change_policy::ChangePolicy, float::Ordf32, BnbMetric, CoinSelector, Drain, FeeRate, Target, }; /// Metric that aims to minimize transaction fees. The future fee for spending the change output is @@ -54,7 +53,7 @@ impl BnbMetric for LowestFee { 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 + // it's possible that adding another input might reduce your long term fee 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 @@ -66,9 +65,9 @@ impl BnbMetric for LowestFee { 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); + let ev = low_sats_per_wu_candidate.effective_value(self.target.fee.rate); + // we can only reduce excess if ev is negative 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 @@ -83,7 +82,7 @@ impl BnbMetric for LowestFee { let cost_of_change = self .change_policy .drain_weights - .waste(self.target.feerate, self.long_term_feerate); + .waste(self.target.fee.rate, self.long_term_feerate); let best_score_without_change = Ordf32( current_score.0 + cost_of_getting_rid_of_change - cost_of_change, ); @@ -97,25 +96,65 @@ impl BnbMetric for LowestFee { Some(current_score) } else { // Step 1: select everything up until the input that hits the target. - let (mut cs, slurp_index, to_slurp) = cs + let (mut cs, resize_index, to_resize) = cs .clone() .select_iter() .find(|(cs, _, _)| cs.is_target_met(self.target))?; - cs.deselect(slurp_index); + cs.deselect(resize_index); + + // We need to find the minimum fee we'd pay if we satisfy the feerate constraint. We do + // this by imagining we had a perfect input that perfectly hit the target. The sats per + // weight unit of this perfect input is the one at `slurp_index` but we'll do a scaled + // resize of it to fit perfectly. + // + // Here's the formaula: + // + // target_feerate = (current_input_value - current_output_value + scale * value_resized_input) / (current_weight + scale * weight_resized_input) + // + // Rearranging to find `scale` we find that: + // + // scale = remaining_value_to_reach_feerate / effective_value_of_resized_input + // + // This should be intutive since we're finding out how to scale the input we're resizing to get the effective value we need. + let rate_excess = cs.rate_excess(self.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); + if effective_value_of_resized_input > 0.0 { + let feerate_scale = + remaining_value_to_reach_feerate / effective_value_of_resized_input; + scale = scale.max(Ordf32(feerate_scale)); + } else { + return None; // we can never satisfy the constraint + } + } - // 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()); + // 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(self.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 = + to_resize.effective_value(replace.incremental_relay_feerate); + if effective_value_of_resized_input > 0.0 { + let replace_scale = + remaining_value_to_reach_feerate / effective_value_of_resized_input; + scale = scale.max(Ordf32(replace_scale)); + } else { + return None; // we can never satisfy the constraint + } + } + } - 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); + assert!(scale.0 > 0.0); + let ideal_fee = scale.0 * to_resize.value as f32 + cs.selected_value() as f32 + - self.target.value as f32; + assert!(ideal_fee >= 0.0); Some(Ordf32(ideal_fee)) } @@ -125,19 +164,3 @@ impl BnbMetric for LowestFee { true } } - -/// 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 57ce72a..3d6483d 100644 --- a/src/metrics/waste.rs +++ b/src/metrics/waste.rs @@ -49,7 +49,7 @@ impl BnbMetric for Waste { // // Don't be afraid. This function is a "heuristic" lower bound. It doesn't need to be super // duper correct. In testing it seems to come up with pretty good results pretty fast. - let rate_diff = self.target.feerate.spwu() - self.long_term_feerate.spwu(); + let rate_diff = self.target.fee.rate.spwu() - self.long_term_feerate.spwu(); // whether from this coin selection it's possible to avoid change let change_lower_bound = change_lower_bound(cs, self.target, self.change_policy); const IGNORE_EXCESS: f32 = 0.0; @@ -80,7 +80,7 @@ impl BnbMetric for Waste { .select_iter() .rev() .take_while(|(cs, _, wv)| { - wv.effective_value(self.target.feerate) < 0.0 + wv.effective_value(self.target.fee.rate) < 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 { + if wv.effective_value(self.target.fee.rate) < 0.0 { Some(wv) } else { None @@ -100,7 +100,7 @@ impl BnbMetric for Waste { // the hopes of getting rid of the change output let value_to_slurp = -cs.rate_excess(self.target, Drain::none()); let weight_to_extinguish_excess = - slurp_wv(finishing_input, value_to_slurp, self.target.feerate); + slurp_wv(finishing_input, value_to_slurp, self.target.fee.rate); let waste_to_extinguish_excess = weight_to_extinguish_excess * rate_diff; // return: waste after excess reduction @@ -147,12 +147,12 @@ impl BnbMetric for Waste { // satisfying absolute and feerate constraints requires different calculations so we do them // both independently and find which requires the most weight of the next input. let remaining_rate = cs.rate_excess(self.target, change_lower_bound); - let remaining_abs = cs.absolute_excess(self.target, change_lower_bound); + let remaining_abs = cs.replacement_excess(self.target, change_lower_bound); 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); + slurp_wv(to_slurp, remaining_rate.min(0), self.target.fee.rate); let weight_to_satisfy = weight_to_satisfy_abs.max(weight_to_satisfy_rate); debug_assert!(weight_to_satisfy <= to_slurp.weight as f32); @@ -160,7 +160,7 @@ impl BnbMetric for Waste { }; let weight_lower_bound = cs.input_weight() as f32 + ideal_next_weight; let mut waste = weight_lower_bound * rate_diff; - waste += change_lower_bound.waste(self.target.feerate, self.long_term_feerate); + waste += change_lower_bound.waste(self.target.fee.rate, self.long_term_feerate); Some(Ordf32(waste)) } @@ -175,7 +175,7 @@ impl BnbMetric for Waste { let mut lower_bound = { 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); + cs.select_all_effective(self.target.fee.rate); if !cs.is_target_met(self.target) { return None; } @@ -200,7 +200,7 @@ impl BnbMetric for Waste { .select_iter() .rev() .take_while(|(cs, _, wv)| { - wv.effective_value(self.target.feerate) < 0.0 + wv.effective_value(self.target.fee.rate) < 0.0 || cs.drain_value(self.target, self.change_policy).is_none() }) .last(); diff --git a/src/target.rs b/src/target.rs new file mode 100644 index 0000000..2bbccac --- /dev/null +++ b/src/target.rs @@ -0,0 +1,86 @@ +use crate::FeeRate; + +/// A target value to select for along with feerate constraints. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd, Default)] +pub struct Target { + /// The fee constraints that must be satisfied by the selection + pub fee: TargetFee, + /// The minmum value that should be left for the output + pub value: u64, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] +/// The fee constraints of a coin selection. +/// +/// There are two orthogonal constraints: +/// +/// - `rate`: The feerate of the transaction must at least be this high. You set this to control how +/// quickly your transaction is confirmed. Typically a coin selection will try and hit this target +/// exactly but it might go over if the `replace` constraint takes precedence or if the +/// [`ChangePolicy`] determines that the excess value should just be given to miners (rather than +/// create a change output). +/// - `replace`: The selection must have a high enough fee to satisfy [RBF rule 4] +/// +/// [RBF rule 4]: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy +/// [`ChangePolicy`]: crate::ChangePolicy +pub struct TargetFee { + /// The feerate the transaction must have + pub rate: FeeRate, + /// The fee must enough enough to replace this + pub replace: Option, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Ord, PartialOrd)] +/// The weight transaction(s) that this new transaction is replacing including the feerate. +pub struct Replace { + /// The fee of the transaction being replaced paid + pub fee: u64, + /// The incrememental relay feerate (by default 1 sat per vbyte). + pub incremental_relay_feerate: FeeRate, +} + +impl Replace { + /// Replace transaction(s) that paid `tx_fee` in fees assuming the default *incremental relay feerate*. + pub fn new(tx_fee: u64) -> Self { + Self { + fee: tx_fee, + incremental_relay_feerate: FeeRate::DEFUALT_RBF_INCREMENTAL_RELAY, + } + } + + /// The minimum fee for the transaction with weight `replacing_tx_weight` that wants to do the replacement. + /// This is defined by [RBF rule 4]. + /// + /// [RBF rule 4]: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy + pub fn min_fee_to_do_replacement(&self, replacing_tx_weight: u32) -> u64 { + let min_fee_increment = + (replacing_tx_weight as f32 * self.incremental_relay_feerate.spwu()).ceil() as u64; + self.fee + min_fee_increment + } +} + +impl Default for TargetFee { + /// The default is feerate set is [`FeeRate::DEFAULT_MIN_RELAY`] and doesn't replace anything. + fn default() -> Self { + Self { + rate: FeeRate::DEFAULT_MIN_RELAY, + replace: None, + } + } +} + +impl TargetFee { + /// A target fee of 0 sats per vbyte (and no replacement) + pub const ZERO: Self = TargetFee { + rate: FeeRate::ZERO, + replace: None, + }; + + /// Creates a target fee from a feerate. The target won't include a replacement. + pub fn from_feerate(feerate: FeeRate) -> Self { + Self { + rate: feerate, + replace: None, + } + } +} diff --git a/tests/bnb.rs b/tests/bnb.rs index 40776de..1a2c359 100644 --- a/tests/bnb.rs +++ b/tests/bnb.rs @@ -1,5 +1,7 @@ mod common; -use bdk_coin_select::{float::Ordf32, BnbMetric, Candidate, CoinSelector, Drain, FeeRate, Target}; +use bdk_coin_select::{ + float::Ordf32, BnbMetric, Candidate, CoinSelector, Drain, Target, TargetFee, +}; #[macro_use] extern crate alloc; @@ -44,8 +46,7 @@ impl BnbMetric for MinExcessThenWeight { fn bound(&mut self, cs: &CoinSelector<'_>) -> Option { let mut cs = cs.clone(); - cs.select_until_target_met(self.target, Drain::none()) - .ok()?; + cs.select_until_target_met(self.target).ok()?; if let Some(last_index) = cs.selected_indices().iter().last().copied() { cs.deselect(last_index); } @@ -86,8 +87,7 @@ fn bnb_finds_an_exact_solution_in_n_iter() { let target = Target { value: target, // we're trying to find an exact selection value so set fees to 0 - feerate: FeeRate::zero(), - min_fee: 0, + fee: TargetFee::ZERO, }; let solutions = cs.bnb_solutions(MinExcessThenWeight { target }); @@ -117,8 +117,7 @@ fn bnb_finds_solution_if_possible_in_n_iter() { let target = Target { value: target, - feerate: FeeRate::default_min_relay_fee(), - min_fee: 0, + fee: TargetFee::default(), }; let solutions = cs.bnb_solutions(MinExcessThenWeight { target }); @@ -146,8 +145,7 @@ proptest! { let target = Target { value: target, - feerate: FeeRate::zero(), - min_fee: 0, + fee: TargetFee::ZERO, }; let solutions = cs.bnb_solutions(MinExcessThenWeight { target }); @@ -193,8 +191,7 @@ proptest! { let target = Target { value: target, // we're trying to find an exact selection value so set fees to 0 - feerate: FeeRate::zero(), - min_fee: 0 + fee: TargetFee::ZERO, }; let solutions = cs.bnb_solutions(MinExcessThenWeight { target }); diff --git a/tests/changeless.proptest-regressions b/tests/changeless.proptest-regressions index b515b1b..97089e2 100644 --- a/tests/changeless.proptest-regressions +++ b/tests/changeless.proptest-regressions @@ -5,3 +5,5 @@ # It is recommended to check this file in to source control so that # everyone who runs the test benefits from these saved cases. cc b03fc0267d15cf4455c7f00feed18d1ba82a783a38bf689dacdd572356013877 # shrinks to num_inputs = 7, target = 1277, feerate = 1.0, min_fee = 177, base_weight = 0, long_term_feerate_diff = 0.0, change_weight = 1, change_spend_weight = 1 +cc 2ba2cfa2412c0f3c9de4eb35caeefa1a086797f5c3f5fc0528396cc90a85d993 # shrinks to num_inputs = 5, target = 908, feerate = 7.823237, replace = None, base_weight = 222, long_term_feerate_diff = 2.4063222, change_weight = 14, change_spend_weight = 14 +cc fc2f2211d811690b78ca4206be874e5c3e99727626f79306bbdd25d59df9c27b # shrinks to num_inputs = 10, target = 3821, feerate = 4.104783, replace = None, base_weight = 321, long_term_feerate_diff = 2.7914581, change_weight = 1, change_spend_weight = 1 diff --git a/tests/changeless.rs b/tests/changeless.rs index d06e063..d98a77c 100644 --- a/tests/changeless.rs +++ b/tests/changeless.rs @@ -2,7 +2,7 @@ mod common; use bdk_coin_select::{ float::Ordf32, metrics, Candidate, ChangePolicy, CoinSelector, Drain, DrainWeights, FeeRate, - Target, + Target, TargetFee, }; use proptest::{prelude::*, proptest, test_runner::*}; use rand::{prelude::IteratorRandom, Rng, RngCore}; @@ -14,7 +14,7 @@ fn test_wv(mut rng: impl RngCore) -> impl Iterator { value, weight: rng.gen_range(0..100), input_count: rng.gen_range(1..2), - is_segwit: rng.gen_bool(0.5), + is_segwit: false, } }) } @@ -31,7 +31,7 @@ proptest! { num_inputs in 0usize..15, target in 0u64..15_000, feerate in 1.0f32..10.0, - min_fee in 0u64..1_000, + replace in common::maybe_replace(0u64..1_000), base_weight in 0u32..500, long_term_feerate_diff in -5.0f32..5.0, change_weight in 1u32..100, @@ -50,14 +50,15 @@ proptest! { let change_policy = ChangePolicy::min_value_and_waste(drain, 0, feerate, long_term_feerate); let wv = test_wv(&mut rng); let candidates = wv.take(num_inputs).collect::>(); - println!("candidates: {:#?}", candidates); let cs = CoinSelector::new(&candidates, base_weight); let target = Target { value: target, - feerate, - min_fee + fee: TargetFee { + rate: feerate, + replace, + } }; let solutions = cs.bnb_solutions(metrics::Changeless { @@ -65,6 +66,7 @@ proptest! { change_policy }); + println!("candidates: {:#?}", cs.candidates().collect::>()); let best = solutions .enumerate() @@ -77,16 +79,16 @@ proptest! { let mut cmp_benchmarks = vec![ { let mut naive_select = cs.clone(); - naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(Ordf32(wv.effective_value(target.feerate)))); + naive_select.sort_candidates_by_key(|(_, wv)| core::cmp::Reverse(Ordf32(wv.effective_value(target.fee.rate)))); // we filter out failing onces below - let _ = naive_select.select_until_target_met(target, Drain { weights: drain, value: 0 }); + let _ = naive_select.select_until_target_met(target); naive_select }, ]; cmp_benchmarks.extend((0..10).map(|_|random_minimal_selection(&cs, target, long_term_feerate, change_policy, &mut rng))); - let cmp_benchmarks = cmp_benchmarks.into_iter().filter(|cs| cs.is_target_met_with_change_policy(target, change_policy)); + let cmp_benchmarks = cmp_benchmarks.into_iter().filter(|cs| cs.is_target_met(target)); for (_bench_id, bench) in cmp_benchmarks.enumerate() { prop_assert!(bench.drain_value(target, change_policy).is_some() >= sol.drain_value(target, change_policy).is_some()); } @@ -95,6 +97,7 @@ proptest! { let mut cs = cs.clone(); let mut metric = metrics::Changeless { target, change_policy }; let has_solution = common::exhaustive_search(&mut cs, &mut metric).is_some(); + dbg!(format!("{}", cs)); assert!(!has_solution); } } @@ -115,7 +118,7 @@ fn random_minimal_selection<'a>( let mut last_waste: Option = None; while let Some(next) = cs.unselected_indices().choose(rng) { cs.select(next); - if cs.is_target_met_with_change_policy(target, change_policy) { + if cs.is_target_met(target) { break; } } diff --git a/tests/common.rs b/tests/common.rs index fad3baa..fa3bfa9 100644 --- a/tests/common.rs +++ b/tests/common.rs @@ -1,16 +1,29 @@ #![allow(dead_code)] -use std::any::type_name; - use bdk_coin_select::{ float::Ordf32, BnbMetric, Candidate, ChangePolicy, CoinSelector, Drain, DrainWeights, FeeRate, - NoBnbSolution, Target, + NoBnbSolution, Replace, Target, TargetFee, }; use proptest::{ prelude::*, prop_assert, prop_assert_eq, test_runner::{RngAlgorithm, TestRng}, }; +use rand::seq::IteratorRandom; +use std::any::type_name; + +pub fn replace(fee_strategy: impl Strategy) -> impl Strategy { + fee_strategy.prop_map(|fee| Replace { + fee, + incremental_relay_feerate: FeeRate::DEFUALT_RBF_INCREMENTAL_RELAY, + }) +} + +pub fn maybe_replace( + fee_strategy: impl Strategy, +) -> impl Strategy> { + proptest::option::of(replace(fee_strategy)) +} /// Used for constructing a proptest that compares an exhaustive search result with a bnb result /// with the given metric. @@ -50,12 +63,18 @@ where now.elapsed().as_secs_f64(), exp_result_str ); - // bonus check: ensure min_fee is respected + // bonus check: ensure replacement fee is respected if exp_result.is_some() { let selected_value = exp_selection.selected_value(); - let drain_value = exp_selection.drain(target, change_policy).value; + let drain = exp_selection.drain(target, change_policy); let target_value = target.value; - assert!(selected_value - target_value - drain_value >= params.min_fee); + let replace_fee = params + .replace + .map(|replace| { + replace.min_fee_to_do_replacement(exp_selection.weight(drain.weights.output_weight)) + }) + .unwrap_or(0); + assert!(selected_value - target_value - drain.value >= replace_fee); } println!("\tbranch and bound:"); @@ -83,11 +102,17 @@ where exp_result_str ); - // bonus check: ensure min_fee is respected + // bonus check: ensure replacement fee is respected let selected_value = selection.selected_value(); - let drain_value = selection.drain(target, change_policy).value; + let drain = selection.drain(target, change_policy); let target_value = target.value; - assert!(selected_value - target_value - drain_value >= params.min_fee); + let replace_fee = params + .replace + .map(|replace| { + replace.min_fee_to_do_replacement(selection.weight(drain.weights.output_weight)) + }) + .unwrap_or(0); + assert!(selected_value - target_value - drain.value >= replace_fee); } _ => prop_assert!(result.is_err(), "should not find solution"), } @@ -171,7 +196,7 @@ pub struct StrategyParams { pub n_candidates: usize, pub target_value: u64, pub base_weight: u32, - pub min_fee: u64, + pub replace: Option, pub feerate: f32, pub feerate_lt_diff: f32, pub drain_weight: u32, @@ -182,8 +207,10 @@ pub struct StrategyParams { impl StrategyParams { pub fn target(&self) -> Target { Target { - feerate: self.feerate(), - min_fee: self.min_fee, + fee: TargetFee { + rate: FeeRate::from_sat_per_vb(self.feerate), + replace: self.replace, + }, value: self.target_value, } } @@ -355,3 +382,101 @@ where err => format!("{:?}", err), } } + +pub fn compare_against_benchmarks( + params: StrategyParams, + candidates: Vec, + change_policy: ChangePolicy, + mut metric: M, +) -> Result<(), TestCaseError> { + println!("======================================="); + let start = std::time::Instant::now(); + let mut rng = TestRng::deterministic_rng(RngAlgorithm::ChaCha); + let target = params.target(); + let cs = CoinSelector::new(&candidates, params.base_weight); + let solutions = cs.bnb_solutions(metric.clone()); + + let best = solutions + .enumerate() + .filter_map(|(i, sol)| Some((i, sol?))) + .last(); + + match best { + Some((_i, (sol, _score))) => { + let mut cmp_benchmarks = vec![ + { + let mut naive_select = cs.clone(); + naive_select.sort_candidates_by_key(|(_, wv)| { + core::cmp::Reverse(Ordf32(wv.effective_value(target.fee.rate))) + }); + // we filter out failing onces below + let _ = naive_select.select_until_target_met(target); + naive_select + }, + { + let mut all_selected = cs.clone(); + all_selected.select_all(); + all_selected + }, + { + let mut all_effective_selected = cs.clone(); + all_effective_selected.select_all_effective(target.fee.rate); + all_effective_selected + }, + ]; + + // add some random selections -- technically it's possible that one of these is better but it's very unlikely if our algorithm is working correctly. + cmp_benchmarks.extend((0..10).map(|_| { + randomly_satisfy_target(&cs, target, change_policy, &mut rng, metric.clone()) + })); + + let cmp_benchmarks = cmp_benchmarks + .into_iter() + .filter(|cs| cs.is_target_met(target)); + let sol_score = metric.score(&sol); + + for (_bench_id, mut bench) in cmp_benchmarks.enumerate() { + let bench_score = metric.score(&bench); + if sol_score > bench_score { + dbg!(_bench_id); + println!("bnb solution: {}", sol); + bench.sort_candidates_by_descending_value_pwu(); + println!("found better: {}", bench); + } + prop_assert!(sol_score <= bench_score); + } + } + None => { + prop_assert!(!cs.is_selection_possible(target)); + } + } + + dbg!(start.elapsed()); + Ok(()) +} + +#[allow(unused)] +fn randomly_satisfy_target<'a>( + cs: &CoinSelector<'a>, + target: Target, + change_policy: ChangePolicy, + rng: &mut impl RngCore, + mut metric: impl BnbMetric, +) -> CoinSelector<'a> { + let mut cs = cs.clone(); + + let mut last_score: Option = None; + while let Some(next) = cs.unselected_indices().choose(rng) { + cs.select(next); + if cs.is_target_met(target) { + let curr_score = metric.score(&cs); + if let Some(last_score) = last_score { + if curr_score.is_none() || curr_score.unwrap() > last_score { + break; + } + } + last_score = curr_score; + } + } + cs +} diff --git a/tests/lowest_fee.rs b/tests/lowest_fee.rs index 4d1effd..bee6809 100644 --- a/tests/lowest_fee.rs +++ b/tests/lowest_fee.rs @@ -2,8 +2,10 @@ mod common; use bdk_coin_select::metrics::{Changeless, LowestFee}; -use bdk_coin_select::{BnbMetric, Candidate, CoinSelector}; -use bdk_coin_select::{ChangePolicy, Drain, DrainWeights, FeeRate, Target}; +use bdk_coin_select::{ + BnbMetric, Candidate, ChangePolicy, CoinSelector, Drain, DrainWeights, FeeRate, Replace, + Target, TargetFee, +}; use proptest::prelude::*; proptest! { @@ -17,14 +19,14 @@ proptest! { n_candidates in 1..20_usize, // candidates (n) target_value in 500..500_000_u64, // target value (sats) base_weight in 0..1000_u32, // base weight (wu) - min_fee in 0..1000_u64, // min fee (sats) + replace in common::maybe_replace(0u64..1_000), feerate in 1.0..100.0_f32, // feerate (sats/vb) feerate_lt_diff in -5.0..50.0_f32, // longterm feerate diff (sats/vb) drain_weight in 100..=500_u32, // drain weight (wu) drain_spend_weight in 1..=2000_u32, // drain spend weight (wu) drain_dust in 100..=1000_u64, // drain dust (sats) ) { - let params = common::StrategyParams { n_candidates, target_value, base_weight, min_fee, feerate, feerate_lt_diff, drain_weight, drain_spend_weight, drain_dust }; + let params = common::StrategyParams { n_candidates, target_value, base_weight, replace, 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(params.drain_weights(), params.drain_dust); let metric = LowestFee { target: params.target(), long_term_feerate: params.long_term_feerate(), change_policy }; @@ -37,14 +39,14 @@ proptest! { n_candidates in 0..15_usize, // candidates (n) target_value in 500..500_000_u64, // target value (sats) base_weight in 0..1000_u32, // base weight (wu) - min_fee in 0..1000_u64, // min fee (sats) + replace in common::maybe_replace(0u64..1_000), feerate in 1.0..100.0_f32, // feerate (sats/vb) feerate_lt_diff in -5.0..50.0_f32, // longterm feerate diff (sats/vb) drain_weight in 100..=500_u32, // drain weight (wu) drain_spend_weight in 1..=1000_u32, // drain spend weight (wu) drain_dust in 100..=1000_u64, // drain dust (sats) ) { - let params = common::StrategyParams { n_candidates, target_value, base_weight, min_fee, feerate, feerate_lt_diff, drain_weight, drain_spend_weight, drain_dust }; + let params = common::StrategyParams { n_candidates, target_value, base_weight, replace, 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(params.drain_weights(), params.drain_dust); let metric = LowestFee { target: params.target(), long_term_feerate: params.long_term_feerate(), change_policy }; @@ -57,7 +59,7 @@ proptest! { n_candidates in 30..300_usize, target_value in 50_000..500_000_u64, // target value (sats) base_weight in 0..641_u32, // base weight (wu) - min_fee in 0..1_000_u64, // min fee (sats) + replace in common::maybe_replace(0u64..1_000), feerate in 1.0..10.0_f32, // feerate (sats/vb) feerate_lt_diff in -5.0..5.0_f32, // longterm feerate diff (sats/vb) drain_weight in 100..=500_u32, // drain weight (wu) @@ -70,7 +72,7 @@ proptest! { n_candidates, target_value, base_weight, - min_fee, + replace, feerate, feerate_lt_diff, drain_weight, @@ -105,6 +107,25 @@ proptest! { println!("\t\tscore={} rounds={}", score, rounds); prop_assert!(rounds <= params.n_candidates); } + + #[test] + #[cfg(not(debug_assertions))] // too slow if compiling for debug + fn compare_against_benchmarks(n_candidates in 0..50_usize, // candidates (n) + target_value in 500..1_000_000_u64, // target value (sats) + base_weight in 0..1000_u32, // base weight (wu) + replace in common::maybe_replace(0u64..1_000), + feerate in 1.0..100.0_f32, // feerate (sats/vb) + feerate_lt_diff in -5.0..50.0_f32, // longterm feerate diff (sats/vb) + drain_weight in 100..=500_u32, // drain weight (wu) + drain_spend_weight in 1..=1000_u32, // drain spend weight (wu) + drain_dust in 100..=1000_u64, // drain dust (sats) + ) { + let params = common::StrategyParams { n_candidates, target_value, base_weight, replace, 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(params.drain_weights(), params.drain_dust); + let metric = LowestFee { target: params.target(), long_term_feerate: params.long_term_feerate(), change_policy }; + common::compare_against_benchmarks(params, candidates, change_policy, metric)?; + } } /// We combine the `LowestFee` and `Changeless` metrics to derive a `ChangelessLowestFee` metric. @@ -114,7 +135,7 @@ fn combined_changeless_metric() { n_candidates: 100, target_value: 100_000, base_weight: 1000, - min_fee: 0, + replace: None, feerate: 5.0, feerate_lt_diff: -4.0, drain_weight: 200, @@ -158,8 +179,7 @@ fn combined_changeless_metric() { #[test] fn adding_another_input_to_remove_change() { let target = Target { - feerate: FeeRate::from_sat_per_vb(1.0), - min_fee: 0, + fee: TargetFee::default(), value: 99_870, }; diff --git a/tests/rbf.rs b/tests/rbf.rs new file mode 100644 index 0000000..70e943e --- /dev/null +++ b/tests/rbf.rs @@ -0,0 +1,69 @@ +use bdk_coin_select::{FeeRate, Replace}; + +#[test] +fn run_bitcoin_core_rbf_tests() { + // see rbf_tests.cpp + // + // https://github.com/bitcoin/bitcoin/blob/e69796c79c0aa202087a13ba62d9fbcc1c8754d4/src/test/rbf_tests.cpp#L151 + const CENT: u64 = 100_000; // no clue why this would be called CENT 😕 + let low_fee = CENT / 100; + let _normal_fee = CENT / 10; + let high_fee = CENT; + let incremental_relay_feerate = FeeRate::DEFUALT_RBF_INCREMENTAL_RELAY; + let higher_relay_feerate = FeeRate::from_sat_per_vb(2.0); + + assert!(pays_for_rbf(high_fee, high_fee, 1, FeeRate::ZERO)); + assert!(!pays_for_rbf(high_fee, high_fee - 1, 1, FeeRate::ZERO)); + assert!(!pays_for_rbf(high_fee + 1, high_fee, 1, FeeRate::ZERO)); + assert!(!pays_for_rbf( + high_fee, + high_fee + 1, + 2, + incremental_relay_feerate + )); + assert!(pays_for_rbf( + high_fee, + high_fee + 2, + 2, + incremental_relay_feerate + )); + assert!(!pays_for_rbf( + high_fee, + high_fee + 2, + 2, + higher_relay_feerate + )); + assert!(pays_for_rbf( + high_fee, + high_fee + 4, + 2, + higher_relay_feerate + )); + assert!(!pays_for_rbf( + low_fee, + high_fee, + 99999999, + incremental_relay_feerate + )); + assert!(pays_for_rbf( + low_fee, + high_fee + 99999999, + 99999999, + incremental_relay_feerate + )); +} + +fn pays_for_rbf( + original_fees: u64, + replacement_fees: u64, + replacement_vsize: u32, + relay_fee: FeeRate, +) -> bool { + let min_fee = Replace { + fee: original_fees, + incremental_relay_feerate: relay_fee, + } + .min_fee_to_do_replacement(replacement_vsize * 4); + + replacement_fees >= min_fee +} diff --git a/tests/waste.rs b/tests/waste.rs index 6b7ee81..e465671 100644 --- a/tests/waste.rs +++ b/tests/waste.rs @@ -4,7 +4,7 @@ mod common; use bdk_coin_select::{ float::Ordf32, metrics::Waste, Candidate, ChangePolicy, CoinSelector, Drain, DrainWeights, - FeeRate, Target, + FeeRate, Target, TargetFee, }; use proptest::{ prelude::*, @@ -17,7 +17,6 @@ fn waste_all_selected_except_one_is_optimal_and_awkward() { let num_inputs = 40; let target = 15578; let feerate = 8.190512; - let min_fee = 0; let base_weight = 453; let long_term_feerate_diff = -3.630499; let change_weight = 1; @@ -39,8 +38,7 @@ fn waste_all_selected_except_one_is_optimal_and_awkward() { let cs = CoinSelector::new(&candidates, base_weight); let target = Target { value: target, - feerate, - min_fee, + fee: TargetFee::from_feerate(feerate), }; let solutions = cs.bnb_solutions(Waste { @@ -72,7 +70,7 @@ fn waste_naive_effective_value_shouldnt_be_better() { let num_inputs = 23; let target = 1475; let feerate = 1.0; - let min_fee = 989; + // let min_fee = 989; let base_weight = 0; let long_term_feerate_diff = 3.8413858; let change_weight = 1; @@ -85,11 +83,6 @@ fn waste_naive_effective_value_shouldnt_be_better() { output_weight: change_weight, spend_weight: change_spend_weight, }; - let drain = Drain { - weights: drain_weights, - value: 0, - }; - let change_policy = ChangePolicy::min_value_and_waste(drain_weights, 0, feerate, long_term_feerate); let wv = test_wv(&mut rng); @@ -99,8 +92,7 @@ fn waste_naive_effective_value_shouldnt_be_better() { let target = Target { value: target, - feerate, - min_fee, + fee: TargetFee::from_feerate(feerate), }; let solutions = cs.bnb_solutions(Waste { @@ -118,7 +110,7 @@ fn waste_naive_effective_value_shouldnt_be_better() { let mut naive_select = cs.clone(); 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); + let _ = naive_select.select_until_target_met(target); let bench_waste = naive_select.waste( target, @@ -136,7 +128,6 @@ fn waste_doesnt_take_too_long_to_finish() { let num_inputs = 22; let target = 0; let feerate = 4.9522414; - let min_fee = 0; let base_weight = 2; let long_term_feerate_diff = -0.17994404; let change_weight = 1; @@ -160,8 +151,7 @@ fn waste_doesnt_take_too_long_to_finish() { let target = Target { value: target, - feerate, - min_fee, + fee: TargetFee::from_feerate(feerate), }; let solutions = cs.bnb_solutions(Waste { @@ -190,7 +180,6 @@ fn waste_lower_long_term_feerate_but_still_need_to_select_all() { let num_inputs = 16; let target = 5586; let feerate = 9.397041; - let min_fee = 0; let base_weight = 91; let long_term_feerate_diff = 0.22074795; let change_weight = 1; @@ -213,8 +202,7 @@ fn waste_lower_long_term_feerate_but_still_need_to_select_all() { let target = Target { value: target, - feerate, - min_fee, + fee: TargetFee::from_feerate(feerate), }; let solutions = cs.bnb_solutions(Waste { @@ -249,7 +237,6 @@ fn waste_low_but_non_negative_rate_diff_means_adding_more_inputs_might_reduce_ex let num_inputs = 22; let target = 7620; let feerate = 8.173157; - let min_fee = 0; let base_weight = 35; let long_term_feerate_diff = 0.0; let change_weight = 1; @@ -277,8 +264,7 @@ fn waste_low_but_non_negative_rate_diff_means_adding_more_inputs_might_reduce_ex let target = Target { value: target, - feerate, - min_fee, + fee: TargetFee::from_feerate(feerate), }; let solutions = cs.bnb_solutions(Waste { @@ -369,7 +355,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(wv.effective_value(target.fee.rate))); // // we filter out failing onces below // let _ = naive_select.select_until_target_met(target, Drain { weights: drain, value: 0 }); // naive_select @@ -381,7 +367,7 @@ proptest! { // }, // { // let mut all_effective_selected = cs.clone(); - // all_effective_selected.select_all_effective(target.feerate); + // all_effective_selected.select_all_effective(target.fee.rate); // all_effective_selected // } // ]; diff --git a/tests/weight.rs b/tests/weight.rs index a7f71ea..e1ba021 100644 --- a/tests/weight.rs +++ b/tests/weight.rs @@ -50,6 +50,7 @@ fn segwit_one_input_one_output() { assert_eq!( (coin_selector .implied_feerate(tx.output[0].value, Drain::none()) + .unwrap() .as_sat_vb() * 10.0) .round(), @@ -84,6 +85,7 @@ fn segwit_two_inputs_one_output() { assert_eq!( (coin_selector .implied_feerate(tx.output[0].value, Drain::none()) + .unwrap() .as_sat_vb() * 10.0) .round(), @@ -118,6 +120,7 @@ fn legacy_three_inputs() { assert_eq!( (coin_selector .implied_feerate(tx.output.iter().map(|o| o.value).sum(), Drain::none()) + .unwrap() .as_sat_vb() * 10.0) .round(), From 0f7cc313b792c4b4a9b58ece8396f02f71191d77 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Wed, 24 Jan 2024 16:02:40 +1100 Subject: [PATCH 2/2] Implement proper RBF logic satisfying rule 4 Removes `min_fee` constraint in favour of proper rule 4 handling (min_fee was pretty useless). See for rule 4: https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy LowestFee BnB metric updated to bound correctly for this. A few other coin_selector API changes --- src/metrics/lowest_fee.rs | 24 +++++++++++++++++++----- tests/lowest_fee.proptest-regressions | 1 + 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index d88e75a..d1c3e1e 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -52,12 +52,13 @@ impl BnbMetric for LowestFee { let drain_value = cs.drain_value(self.target, self.change_policy); + // I think this whole if statement could be removed if we made this metric decide the change policy if let Some(drain_value) = drain_value { - // it's possible that adding another input might reduce your long term fee 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 + // it's possible that adding another input might reduce your long term fee 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 @@ -91,6 +92,19 @@ impl BnbMetric for LowestFee { } } } + } else { + // Ok but maybe adding change could improve the metric? + let cost_of_adding_change = self + .change_policy + .drain_weights + .waste(self.target.fee.rate, self.long_term_feerate); + let cost_of_no_change = cs.excess(self.target, Drain::none()); + + let best_score_with_change = + Ordf32(current_score.0 - cost_of_no_change as f32 + cost_of_adding_change); + if best_score_with_change < current_score { + return Some(best_score_with_change); + } } Some(current_score) diff --git a/tests/lowest_fee.proptest-regressions b/tests/lowest_fee.proptest-regressions index 2e9a467..9a23f55 100644 --- a/tests/lowest_fee.proptest-regressions +++ b/tests/lowest_fee.proptest-regressions @@ -8,3 +8,4 @@ cc 9c841bb85574de2412972df187e7ebd01f7a06a178a67f4d99c0178dd578ac34 # shrinks to cc e30499b75a1846759fc9ffd7ee558b08a4795598cf7919f6be6d62cc7a79d4cb # shrinks to n_candidates = 25, target_value = 56697, base_weight = 621, min_fee = 0, feerate = 9.417939, feerate_lt_diff = 0.0, drain_weight = 100, drain_spend_weight = 1, drain_dust = 100 cc c580ee452624915fc710d5fe724c7a9347472ccd178f66c9db9479cfc6168f48 # shrinks to n_candidates = 25, target_value = 488278, base_weight = 242, min_fee = 0, feerate = 6.952743, feerate_lt_diff = 0.0, drain_weight = 100, drain_spend_weight = 1, drain_dust = 100 cc 850e0115aeeb7ed50235fdb4b5183eb5bf8309a45874dc261e3d3fd2d8c84660 # shrinks to n_candidates = 8, target_value = 444541, base_weight = 253, min_fee = 0, feerate = 55.98181, feerate_lt_diff = 36.874306, drain_weight = 490, drain_spend_weight = 1779, drain_dust = 100 +cc ca9831dfe4ad27fc705ae4af201b9b50739404bda0e1e130072858634f8d25d9 # added by llfourn from ci: has a case where the lowest fee metric would benefit by adding change