spend: a temporary partial fix for LowestFee
#867
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
darosior
reviewed
Dec 12, 2023
This is a temporary partial fix for bitcoindevkit/coin-select#6 that should be reverted once the upstream fix has been made. When calculating the score, the excess should be added to changeless solutions instead of those with change. Given a solution has been found, this fix adds or removes the excess to its incorrectly calculated score as required so that two changeless solutions can be differentiated if one has higher excess (and therefore pays a higher fee). Note that the `bound` function is also affected by this bug, which could mean some branches are not considered when running BnB, but at least this fix will mean the score for those solutions that are found is correct.
ed75952 to
5f534eb
Compare
darosior
reviewed
Dec 12, 2023
|
ACK 5f534eb |
darosior
added a commit
that referenced
this pull request
Apr 15, 2024
43ecd94 spend: change parameter type for rbf (jp1ac4) 6376909 commands: remove rbf rule 4 logic (jp1ac4) 936d7e9 spend: bump bdk_coin_select to 0.3.0 (jp1ac4) Pull request description: This is to resolve #923. The `score` method of the `LowestFee` metric has been fixed upstream and so our temporary partial fix from #867 is no longer required. The `min_fee` parameter of `select_coins_for_spend`, if positive, now ensures that RBF rule 4 is satisfied. Accordingly, it has been renamed to `replaced_fee` and made an `Option`. We could potentially have used the `SpendTxFees` enum as a parameter directly instead of `feerate_vb` and `replaced_fee`, but `feerate_vb` is currently `f32` rather than `u64` and so I kept them as separate parameters. Thanks to how the `replaced_fee` parameter works, the fee iteration approach used in `rbf_psbt` to ensure the replacement satisfies [RBF rule 4](https://github.com/bitcoin/bitcoin/blob/master/doc/policy/mempool-replacements.md#current-replace-by-fee-policy) is no longer required. `base_weight` is no longer stored in `CoinSelector` and instead the output weights are stored in `Target`. This means that the `output_weight` of `DrainWeights` no longer needs to take into account a potential change in output count varint as this is now handled by bdk_coin_select. The min value for change no longer includes the threshold itself and so we have to subtract 1 from our change policy's min value (see bitcoindevkit/coin-select#14 (comment)). We already have a [test](https://github.com/wizardsardine/liana/blob/master/src/commands/mod.rs#L1653-#L1654) that fails without this subtraction as it expects a change output of 5000 sats. ACKs for top commit: darosior: ACK 43ecd94 Tree-SHA512: a064bafef13abefcb8c4b640cfc4017eb288c62891a8b486add33c1499e7061bf262d6aadabbdd4c191ed9b81e3931b382c8c8445e6bb9c1b052380caf14b3f9
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a temporary partial fix for bitcoindevkit/coin-select#6 that should be reverted once the upstream fix has been made.
When calculating the score, the excess should be added to changeless solutions instead of those with change.
Given a solution has been found, this fix adds or removes the excess to its incorrectly calculated score as required so that two changeless solutions can be differentiated if one has higher excess (and therefore pays a higher fee).
Note that the
boundfunction is also affected by this bug, which could mean some branches are not considered when running BnB, but at least this fix will mean the score for those solutions that are found is correct.The new functional test included here fails without this fix.