Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump bdk_coin_select to 0.3.0 #1076

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

jp1ac4
Copy link
Collaborator

@jp1ac4 jp1ac4 commented Apr 5, 2024

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 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 that fails without this subtraction as it expects a change output of 5000 sats.

jp1ac4 added 3 commits April 5, 2024 21:46
The `score` method of the `LowestFee` metric has been fixed
and so our temporary fix is no longer required.

The `min_fee` parameter of `select_coins_for_spend`,
if positive, now ensures that RBF rule 4 is satisfied.

`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.
RBF rule 4 is now enforced by coin selection.
The parameter has been renamed and changed to `Option` as it is
only required when creating a replacement transaction using RBF.
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 43ecd94

Great work as usual.

if base_tx.input.is_empty() {
base_weight = base_weight.saturating_sub(2);
}
let out_weight_nochange: u32 = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: could have used iter().map().sum() to match the previous line.

@darosior darosior merged commit 8c55eec into wizardsardine:master Apr 15, 2024
21 checks passed
@jp1ac4 jp1ac4 deleted the bump-bdk-coin-select-0.3.0 branch April 18, 2024 18:47
@nondiremanuel nondiremanuel added this to the Liana v6 milestone Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Status: Done
Development

Successfully merging this pull request may close these issues.

spend: bump bdk_coin_select dependency to 0.3.0
3 participants