Migrate to bitcoin::FeeRate

[ValuedMammal]

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:

• Constructing a proper FeeRate from a u64 in all unit tests, and thus obviating the need for the helper feerate_unchecked going forward.
• 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.
• Removed trait Vbytes.

Checklists

All Submissions:

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

[ValuedMammal]

After looking at some related work, I'm not totally opposed to displaying only whole numbers of fee rates. The difference between 99 and 100 sat/vb seems less significant than the difference between 1 and 2 sat/vb.

Some other conveniences I think would be handy:

• Saturating addition/subtraction of FeeRates
• A sanity check that a given rate when applied to a minimum valid tx size doesn't produce an amount greater than MAX_MONEY

[notmandatory]

* A sanity check that a given rate when applied to a minimum valid tx size doesn't produce an amount greater than `MAX_MONEY`

The fee and fee rate sanity check should be done in a new PR, I created bitcoindevkit/bdk_wallet#120 for this issue.

[ValuedMammal]

Rebased to 553bdf9

• Resolved conflict with doc(bdk): Clarify the absolute_fee docs #1158
• Fixed MSRV compile error

[evanlinjin]

I think I have a controversial opinion about these changes. Because we plan to redo the entire tx building logic (after 1.0), and the current tx builder works okay as it is... Would this be worth the effort?

[notmandatory]

This one needs a rebase after alpha.5 release.

[ValuedMammal]

I think I have a controversial opinion about these changes. Because we plan to redo the entire tx building logic (after 1.0), and the current tx builder works okay as it is... Would this be worth the effort?

I think it's fair to have a deeper discussion about this. I don't have strong feelings either way. I think I lean toward making the change because 1) ease of maintainability by aligning with rust bitcoin, 2) the units of a feerate can always be converted to any unit you like.

Granted I'm not as familiar with some of the long-standing design efforts of BDK, so I'm happy to get other opinions or hold off until everyone's in agreement.

[storopoli]

Some changes proposed.
Mostly important is to use from_vb instead of from_vb_unchecked in code and examples.

Tests are fine to use the unchecked version.

[ValuedMammal]

Thank you @storopoli , many good points brought up. I'm working through some of the comments now

[storopoli]

Just a minor thing.

[ValuedMammal]

Recent changes:

• Using checked vsize-to-weight conversions in documentation and during coin selection.
• Added docstring to the test helper feerate_unchecked to explain its purpose, and moved it to bdk/tests/common.rs

[evanlinjin]

Thanks for working on this, fee/weight calculations should be more accurate than before since the old FeeRate::fee_wu was rounding up virtual bytes to whole units, whereas the Mul<FeeRate> for Weight impl (which we are using now) is rounding up kilo-wus.

However, I think we should try our best to avoid these changes in the future. This took a long time to review for me as I had to revisit old code to understand the ramifications of these changes. If there is no bug, we should just leave it as is so we can focus our efforts on the new TxBuilder.

As a final nit, let's get rid of Vbytes and I will happily ACK this!

[ValuedMammal]

• Rebased onto d77a7f2
• Removed trait Vbytes in 0118419

[ValuedMammal]

Posting clippy checks here instead.
error: file opened with create, but truncate behavior not defined
--> crates/file_store/src/store.rs:64:14
|
64 | .create(true)
| ^^^^^^^^^^^^- help: add: .truncate(true)
|
= help: if you intend to overwrite an existing file entirely, call .truncate(true)

@evanlinjin ?

[evanlinjin]

@ValuedMammal rebase on master should fix it now!

[evanlinjin]

ACK 475a772