Refactor: Migrate bdk::FeeRate to bitcoin::FeeRate #1141

junderw · 2023-10-01T06:41:47Z

Closes #1136

Description

This PR will eventually migrate FeeRate to rust-bitcoin's FeeRate.

Notes to the reviewers

This is currently a WIP that is meant for discussion. I have made a lot of arbitrary decisions just so that we can see the full diff and go over each decision again as a team. Don't assume any decision was made with explicit intent, I just tried to make everything compile and tests pass. Everything is on the table for discussion.
If anyone wants to, feel free to take over this PR at any time. The legwork of figuring out all the places that need changing and which tests are affected took a few hours of trial and error. If you look at this PR and think you want to take over and finish off the more interesting parts (investigating the changes and why they were necessary / whether there are better more appropriate ways to change it) feel free. Just let me know. This issue is low priority for me personally, so don't expect this to get done in a quick manner unless someone takes it from me. I will be happy to answer questions about the new FeeRate API though.

Changelog notice

Changed

bdk::FeeRate was replaced with bitcoin::FeeRate

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

notmandatory · 2023-10-01T21:52:59Z

@nondiremanuel I think this PR and the associated issue #1136 deserves to be added to the alpha. We can discuss more on the next team call, but I think it needs to be done so we don't duplicate functionality with our rust-bitcoin dependency and if we don't do it now we won't get another chance until a BDK 2.0 release.

notmandatory · 2023-10-01T22:03:31Z

Thanks for taking this on @junderw ! looks good so far.

realeinherjar · 2023-10-05T11:56:01Z

crates/bdk/src/wallet/coin_selection.rs

@@ -276,7 +276,7 @@ impl CoinSelectionAlgorithm for OldestFirstCoinSelection {
 pub fn decide_change(remaining_amount: u64, fee_rate: FeeRate, drain_script: &Script) -> Excess {
    // drain_output_len = size(len(script_pubkey)) + len(script_pubkey) + size(output_value)
    let drain_output_len = serialize(drain_script).len() + 8usize;
-    let change_fee = fee_rate.fee_vb(drain_output_len);
+    let change_fee = (fee_rate * Weight::from_wu((drain_output_len * 4) as u64)).to_sat();


Why are we hardcoding 4 here?
Might be worth a comment...

let change_fee = (fee_rate * Weight::from_vb_unchecked(drain_output_len as u64)).to_sat();

realeinherjar · 2023-10-05T11:56:48Z

crates/bdk/tests/wallet.rs

-    builder.fee_rate(FeeRate::from_sat_per_vb(2.5)).enable_rbf();
+    builder
+        .fee_rate(FeeRate::from_sat_per_vb_unchecked(3))
+        .enable_rbf();


This changes the logic of the test. We are now testing an integer instead of a float.
Why?

True, using kwu here to test 2.5 (which would be 625) would probably be better.

realeinherjar · 2023-10-05T11:58:06Z

crates/bdk/tests/wallet.rs

-    assert_fee_rate!(psbt, fee.unwrap_or(0), FeeRate::from_sat_per_vb(2.5), @add_signature);
+    assert_fee_rate!(psbt, fee.unwrap_or(0), FeeRate::from_sat_per_kwu(751), @add_signature);


Why change from vBytes to satoshis per 1000 weight units?

realeinherjar · 2023-10-05T11:58:51Z

crates/bdk/tests/wallet.rs

-    let fee_rate = FeeRate::from_sat_per_vb(2.01);
+    let fee_rate = FeeRate::from_sat_per_kwu(522);


Why change from vBytes to satoshis per 1000 weight units?

realeinherjar · 2023-10-05T12:00:26Z

crates/bdk/tests/wallet.rs

-    assert_eq!(psbt.inputs.len(), 1);
+    assert_eq!(psbt.inputs.len(), 2);


This seems odd to me.
From my understanding you are not changing the PSBT inputs in this test.
Hence, why this change?
Is this test broken? Was this passing before your change?

Since coin selection is affected by fees, likely the switch to a non-float FeeRate had some effect on the outcome of selected coins.

This will need investigation.

realeinherjar · 2023-10-05T12:00:33Z

crates/bdk/tests/wallet.rs

-    let fee_rate = FeeRate::from_sat_per_vb(1.0);
+    let fee_rate = FeeRate::from_sat_per_kwu(251);


Why change from vBytes to satoshis per 1000 weight units?

realeinherjar · 2023-10-05T12:00:41Z

crates/bdk/tests/wallet.rs

-    let fee_rate = FeeRate::from_sat_per_vb(1.0);
+    let fee_rate = FeeRate::from_sat_per_kwu(251);


Why change from vBytes to satoshis per 1000 weight units?

junderw · 2023-10-05T15:04:25Z

To answer all of these questions:

Because FeeRate now only deals with integers, so in order to get more granularity, using sat/kwu allows us to perform <1 sat/vbyte.

As for all the other places: Because that change is what was necessary to make the tests pass.

250 sat/kwu = 1 sat/vbyte, so technically, this is also changing the value to 1.004 sat/vbyte

I have not investigated into why this change was needed to make the tests pass.

The next steps in this PR will be uncovering why each of these test changes was necessary and considering what that means for the library.

junderw · 2023-10-05T15:16:40Z

I edited the body of the PR:

If anyone wants to, feel free to take over this PR at any time. The legwork of figuring out all the places that need changing and which tests are affected took a few hours of trial and error. If you look at this PR and think you want to take over and finish off the more interesting parts (investigating the changes and why they were necessary / whether there are better more appropriate ways to change it) feel free. Just let me know. This issue is low priority for me personally, so don't expect this to get done in a quick manner unless someone takes it from me. I will be happy to answer questions about the new FeeRate API though.

ValuedMammal · 2023-10-24T22:44:49Z

crates/bdk/src/psbt/mod.rs

            let weight = self.clone().extract_tx().weight();
-            FeeRate::from_wu(fee, weight)
+            Some(FeeRate::from_sat_per_kwu(


fee_amount.map(|fee| { Amount::from_sat(fee) / weight })

ValuedMammal · 2023-10-24T22:52:22Z

crates/bdk/src/wallet/coin_selection.rs

@@ -489,7 +491,7 @@ impl CoinSelectionAlgorithm for BranchAndBoundCoinSelection {
                curr_value,
                curr_available_value,
                target_amount,
-                cost_of_change,
+                cost_of_change as f32,


can the signature of BranchAndBoundCoinSelection::bnb change to accept cost_of_change: u64 as parameter ?

ValuedMammal · 2023-10-24T23:03:14Z

crates/bdk/src/wallet/mod.rs

@@ -574,7 +574,7 @@ impl<D> Wallet<D> {
    pub fn calculate_fee_rate(&self, tx: &Transaction) -> Result<FeeRate, CalculateFeeError> {
        self.calculate_fee(tx).map(|fee| {
            let weight = tx.weight();
-            FeeRate::from_wu(fee, weight)
+            FeeRate::from_sat_per_kwu(fee.saturating_mul(1000) / weight.to_wu())


self.calculate_fee(tx) .map(|fee| Amount::from_sat(fee) / tx.weight())

ValuedMammal · 2023-10-24T23:09:00Z

crates/bdk/src/wallet/mod.rs

@@ -1350,7 +1352,7 @@ impl<D> Wallet<D> {
            utxos: original_utxos,
            bumping_fee: Some(tx_builder::PreviousFee {
                absolute: fee,
-                rate: fee_rate.as_sat_per_vb(),
+                rate: fee_rate.to_sat_per_vb_ceil() as f32,
            }),


this can be rate: fee_rate if the type of rate in PreviousFee is changed to bitcoin::FeeRate

ValuedMammal · 2023-10-24T23:27:54Z

crates/bdk/tests/psbt.rs

-    assert!(finalized_fee_rate.as_sat_per_vb() >= expected_fee_rate);
-    assert!(finalized_fee_rate.as_sat_per_vb() < unfinalized_fee_rate.as_sat_per_vb());
+    assert!(finalized_fee_rate.to_sat_per_vb_ceil() as f32 >= expected_fee_rate);
+    assert!(finalized_fee_rate.to_sat_per_vb_ceil() <= unfinalized_fee_rate.to_sat_per_vb_ceil());
 }


If we provided a method for creating a FeeRate from a float with something like

fn from_sat_per_vbf(sat_vb: f64) -> FeeRate {...}

then the above can be simplified

let expected_fee_rate = FeeRate::from_sat_per_vbf(1.2345); builder.fee_rate(expected_fee_rate); ... assert!(finalized_fee_rate >= expected_fee_rate); assert!(finalized_fee_rate < unfinalized_fee_rate);

ValuedMammal · 2023-10-24T23:39:14Z

crates/bdk/tests/wallet.rs

-    // fee rate (sats per vbyte) = fee / vbytes = 1000 / 113 = 8.8495575221 rounded to 8.849558
-    assert_eq!(tx_fee_rate.as_sat_per_vb(), 8.849558);
+    // fee rate (sats per vbyte) = fee / vbytes = 1000 / 113 = 8.8495575221 rounded up to 9
+    assert_eq!(tx_fee_rate.to_sat_per_vb_ceil(), 9);


since we lose precision with to_sat_per_vb_ceil, we should assert the value in sat/kwu also.

realeinherjar · 2023-11-01T15:05:18Z

We don't need to implement the floor anymore.
rust-bitcoin v0.31.1 has added to_kwu_floor in rust-bitcoin/rust-bitcoin#1735

notmandatory · 2023-11-16T14:06:17Z

@junderw can we close this PR in favor of #1216 ?

junderw · 2023-11-16T17:08:23Z

Of course!

475a772 refactor(bdk)!: Remove trait Vbytes (vmammal) 0d64beb chore: organize some imports (vmammal) 89608dd refactor(bdk): display CreateTxError::FeeRateTooLow in sat/vb (vmammal) 09bd86e test(bdk): initialize all feerates from `u64` (vmammal) 004957d refactor(bdk)!: drop FeeRate from bdk::types (vmammal) Pull request description: ### Description This follows a similar approach to #1141 namely to remove `FeeRate` from `bdk::types` and instead defer to the upstream implementation for all fee rates. The idea is that making the switch allows BDK to benefit from a higher level abstraction, leaving the implementation details largely hidden. As noted in #774, we should avoid extraneous conversions that can result in deviations in estimated transaction size and calculated fee amounts, etc. This would happen for example whenever calling a method like `FeeRate::to_sat_per_vb_ceil`. The only exception I would make is if we must return a fee rate error to the user, we might prefer to display it in the more familiar sats/vb, but this would only be useful if the rate can be expressed as a float. ### Notes to the reviewers `bitcoin::FeeRate` is an integer whose native unit is sats per kilo-weight unit. In order to facilitate the change, a helper method `feerate_unchecked` is added and used only in wallet tests and psbt tests as necessary to convert existing fee rates to the new type. It's "unchecked" in the sense that we're not checking for integer overflow, because it's assumed we're passing a valid fee rate in a unit test. Potential follow-ups can include: - [x] Constructing a proper `FeeRate` from a `u64` in all unit tests, and thus obviating the need for the helper `feerate_unchecked` going forward. - [x] Remove trait `Vbytes`. - Consider adding an extra check that the argument to `TxBuilder::drain_to` is within "standard" size limits. - Consider refactoring `coin_selection::select_sorted_utxos` to be efficient and readable. closes #1136 ### Changelog notice - Removed `FeeRate` type. All fee rates are now rust-bitcoin [`FeeRate`](https://docs.rs/bitcoin/latest/bitcoin/blockdata/fee_rate/struct.FeeRate.html). - Removed trait `Vbytes`. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing ACKs for top commit: evanlinjin: ACK 475a772 Tree-SHA512: 511dab8aa7a65d2b15b160cb4feb96964e8401bb04cda4ef0f0244524bf23a575b3739783a14b90d2dccc984b3f30f5dabfb0a890ffe7c897c2dc23ba301bcaf

Refactor: Migrate bdk::FeeRate to bitcoin::FeeRate

2fa0b60

junderw marked this pull request as draft October 1, 2023 06:42

notmandatory assigned junderw Oct 1, 2023

notmandatory added this to the 1.0.0-alpha.3 milestone Oct 1, 2023

notmandatory mentioned this pull request Oct 1, 2023

test: legacy descriptors #1130

Open

8 tasks

realeinherjar reviewed Oct 5, 2023

View reviewed changes

nondiremanuel mentioned this pull request Oct 6, 2023

W41 Lib Team Call bitcoindevkit/.github#80

Closed

39 tasks

nondiremanuel mentioned this pull request Oct 22, 2023

W43 Lib Team Call bitcoindevkit/.github#83

Closed

33 tasks

ValuedMammal reviewed Oct 24, 2023

View reviewed changes

nondiremanuel mentioned this pull request Nov 5, 2023

W45 Lib Team Call bitcoindevkit/.github#84

Closed

38 tasks

notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023

ValuedMammal mentioned this pull request Nov 15, 2023

Migrate to bitcoin::FeeRate #1216

Merged

5 tasks

notmandatory mentioned this pull request Nov 16, 2023

Replace bdk FeeRate with rust-bitcoin FeeRate #1136

Closed

junderw closed this Nov 16, 2023

notmandatory removed this from the 1.0.0-alpha.4 milestone Nov 16, 2023

nondiremanuel mentioned this pull request Nov 19, 2023

W47 Lib Team Call bitcoindevkit/.github#86

Closed

54 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Migrate bdk::FeeRate to bitcoin::FeeRate #1141

Refactor: Migrate bdk::FeeRate to bitcoin::FeeRate #1141

junderw commented Oct 1, 2023 •

edited

Loading

notmandatory commented Oct 1, 2023

notmandatory commented Oct 1, 2023

realeinherjar Oct 5, 2023

ValuedMammal Oct 24, 2023

realeinherjar Oct 5, 2023

junderw Oct 5, 2023

realeinherjar Oct 5, 2023

realeinherjar Oct 5, 2023

realeinherjar Oct 5, 2023

junderw Oct 5, 2023

realeinherjar Oct 5, 2023

realeinherjar Oct 5, 2023

junderw commented Oct 5, 2023

junderw commented Oct 5, 2023

ValuedMammal Oct 24, 2023

ValuedMammal Oct 24, 2023

ValuedMammal Oct 24, 2023

ValuedMammal Oct 24, 2023

ValuedMammal Oct 24, 2023

ValuedMammal Oct 24, 2023

realeinherjar commented Nov 1, 2023

notmandatory commented Nov 16, 2023

junderw commented Nov 16, 2023

		assert_fee_rate!(psbt, fee.unwrap_or(0), FeeRate::from_sat_per_vb(2.5), @add_signature);
		assert_fee_rate!(psbt, fee.unwrap_or(0), FeeRate::from_sat_per_kwu(751), @add_signature);

		let fee_rate = FeeRate::from_sat_per_vb(2.01);
		let fee_rate = FeeRate::from_sat_per_kwu(522);

		assert_eq!(psbt.inputs.len(), 1);
		assert_eq!(psbt.inputs.len(), 2);

Refactor: Migrate bdk::FeeRate to bitcoin::FeeRate #1141

Refactor: Migrate bdk::FeeRate to bitcoin::FeeRate #1141

Conversation

junderw commented Oct 1, 2023 • edited Loading

Description

Notes to the reviewers

Changelog notice

Changed

Checklists

All Submissions:

notmandatory commented Oct 1, 2023

notmandatory commented Oct 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junderw commented Oct 5, 2023

junderw commented Oct 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

realeinherjar commented Nov 1, 2023

notmandatory commented Nov 16, 2023

junderw commented Nov 16, 2023

junderw commented Oct 1, 2023 •

edited

Loading