Skip to content

Commit

Permalink
Implement proper RBF logic satisfying rule 4
Browse files Browse the repository at this point in the history
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
  • Loading branch information
LLFourn committed Jan 24, 2024
1 parent 7a28288 commit 0f7cc31
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 5 deletions.
24 changes: 19 additions & 5 deletions src/metrics/lowest_fee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions tests/lowest_fee.proptest-regressions
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit 0f7cc31

Please sign in to comment.