From 0f7cc313b792c4b4a9b58ece8396f02f71191d77 Mon Sep 17 00:00:00 2001 From: LLFourn Date: Wed, 24 Jan 2024 16:02:40 +1100 Subject: [PATCH] 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