From af1333b3a13afccbfb96197c76d7dff72bf7818f Mon Sep 17 00:00:00 2001 From: Michael Mallan Date: Mon, 4 Nov 2024 15:21:21 +0000 Subject: [PATCH] Loosen LowestFee metric bound In order to get the lowest possible score, assume that no rounding up will be required when converting from weight units to vbytes. --- src/coin_selector.rs | 30 +++++++++++++++++++++++++++ src/feerate.rs | 9 ++++++-- src/metrics/lowest_fee.rs | 15 ++++++++++---- src/target.rs | 9 ++++++++ tests/lowest_fee.proptest-regressions | 1 + 5 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/coin_selector.rs b/src/coin_selector.rs index 469ef56..5dd6e60 100644 --- a/src/coin_selector.rs +++ b/src/coin_selector.rs @@ -200,6 +200,15 @@ impl<'a> CoinSelector<'a> { - self.implied_fee_from_feerate(target, drain.weights) as i64 } + /// Same as [rate_excess](Self::rate_excess) except `target.fee.rate` is applied to the + /// implied transaction's weight units directly without any conversion to vbytes. + pub fn rate_excess_wu(&self, target: Target, drain: Drain) -> i64 { + self.selected_value() as i64 + - target.value() as i64 + - drain.value as i64 + - self.implied_fee_from_feerate_wu(target, drain.weights) as 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; @@ -213,6 +222,20 @@ impl<'a> CoinSelector<'a> { - replacement_excess_needed as i64 } + /// Same as [replacement_excess](Self::replacement_excess) except the replacement fee + /// is calculated using weight units directly without any conversion to vbytes. + pub fn replacement_excess_wu(&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_wu(self.weight(target.outputs, drain.weights)) + } + self.selected_value() as i64 + - target.value() as i64 + - drain.value 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`'s value and weight. It is essentially telling you what target feerate you currently have. /// @@ -253,6 +276,13 @@ impl<'a> CoinSelector<'a> { .implied_fee(self.weight(target.outputs, drain_weights)) } + fn implied_fee_from_feerate_wu(&self, target: Target, drain_weights: DrainWeights) -> u64 { + target + .fee + .rate + .implied_fee_wu(self.weight(target.outputs, drain_weights)) + } + /// 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`. /// diff --git a/src/feerate.rs b/src/feerate.rs index 9168c7f..d4f89cb 100644 --- a/src/feerate.rs +++ b/src/feerate.rs @@ -79,11 +79,16 @@ impl FeeRate { self.0 .0 } - /// The fee that the transaction with weight `tx_weight` should pay in order to satisfy the fee rate given by `self`. + /// The fee that the transaction with weight `tx_weight` should pay in order to satisfy the fee rate given by `self`, + /// where the fee rate is applied to the rounded-up vbytes obtained from `tx_weight`. pub fn implied_fee(&self, tx_weight: u64) -> u64 { - // The fee rate is applied to the rounded-up vbytes. ((tx_weight as f32 / 4.0).ceil() * self.as_sat_vb()).ceil() as u64 } + + /// Same as [implied_fee](Self::implied_fee) except the fee rate given by `self` is applied to `tx_weight` directly. + pub fn implied_fee_wu(&self, tx_weight: u64) -> u64 { + (tx_weight as f32 * self.spwu()).ceil() as u64 + } } impl Add for FeeRate { diff --git a/src/metrics/lowest_fee.rs b/src/metrics/lowest_fee.rs index 9e3e569..830305a 100644 --- a/src/metrics/lowest_fee.rs +++ b/src/metrics/lowest_fee.rs @@ -115,6 +115,10 @@ impl BnbMetric for LowestFee { .select_iter() .find(|(cs, _, _)| cs.is_target_met(self.target))?; + // If this selection is already perfect, return its score directly. + if cs.excess(self.target, Drain::NONE) == 0 { + return Some(self.score(&cs).unwrap()); + }; cs.deselect(resize_index); // We need to find the minimum fee we'd pay if we satisfy the feerate constraint. We do @@ -131,7 +135,10 @@ impl BnbMetric for LowestFee { // 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; + // + // In the perfect scenario, no additional fee would be required to pay for rounding up when converting from weight units to + // vbytes and so all fee calculations below are performed on weight units directly. + let rate_excess = cs.rate_excess_wu(self.target, Drain::NONE) as f32; let mut scale = Ordf32(0.0); if rate_excess < 0.0 { @@ -150,7 +157,7 @@ impl BnbMetric for LowestFee { // We can use the same approach for replacement we just have to use the // incremental_relay_feerate. if let Some(replace) = self.target.fee.replace { - let replace_excess = cs.replacement_excess(self.target, Drain::NONE) as f32; + let replace_excess = cs.replacement_excess_wu(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 = @@ -164,8 +171,8 @@ impl BnbMetric for LowestFee { } } } - - assert!(scale.0 > 0.0); + // `scale` could be 0 even if `is_target_met` is `false` due to the latter being based on + // rounded-up vbytes. let ideal_fee = scale.0 * to_resize.value as f32 + cs.selected_value() as f32 - self.target.value() as f32; assert!(ideal_fee >= 0.0); diff --git a/src/target.rs b/src/target.rs index 6de3cb0..36a8803 100644 --- a/src/target.rs +++ b/src/target.rs @@ -141,4 +141,13 @@ impl Replace { .incremental_relay_feerate .implied_fee(replacing_tx_weight) } + + /// Same as (min_fee_to_do_replacement)[Self::min_fee_to_do_replacement] except the additional fee + /// is calculated using `replacing_tx_weight` directly without any conversion to vbytes. + pub fn min_fee_to_do_replacement_wu(&self, replacing_tx_weight: u64) -> u64 { + self.fee + + self + .incremental_relay_feerate + .implied_fee_wu(replacing_tx_weight) + } } diff --git a/tests/lowest_fee.proptest-regressions b/tests/lowest_fee.proptest-regressions index ed2f8c4..942563a 100644 --- a/tests/lowest_fee.proptest-regressions +++ b/tests/lowest_fee.proptest-regressions @@ -10,3 +10,4 @@ cc c580ee452624915fc710d5fe724c7a9347472ccd178f66c9db9479cfc6168f48 # shrinks to 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 cc 85d9dc968a690553484d0f166f3d778c3e0ec7d9059809c2818b62d6609853c1 +cc b32bb279f62e2300a14c9053cff09f6a953bf1020b878958cb510258ffe65028 # added by jp1ac4 from ci