FeeRate calculation fixes
#774
Conversation
Weight units should always be whole units. However, virtual bytes will need decimal places and the conversion from weight unit to vbytes should not be rounded. This commit has the following changes: * vbytes should always be represented in `f32` * conversion between vbytes and weight units should never be rounded * simple calculations (such as those for feerates) should be explicitly defined (instead of using multiple small functions)
evanlinjin
force-pushed
the
fix_feerate
branch
from
416fb94 to
d5c5fe6
Compare
Why? 1 WU = 0.25 sat/vbyte. If you can have 0.1 sat/vbyte, then you also need decimal weight units. Or am I misinterpreting what you're saying? (I haven't read the code yet) Anywoo, as we were saying on discord, it would be cool to handle feerates the core way ™️ (https://discord.com/channels/753336465005608961/753367451319926827/1026784080714534932) Should we just move to this method instead? If so, can you update this PR, or you have too much on your plate already? If you need help just ping :) |
Think about it this way, if we look at the smallest possible "unit" of a transaction, for example, an I'm talking about weight units (
I agree that avoiding floats and working in kilo-everything seems to be the way to go. However, this PR is more of a bug fix, not an attempt at changing behavior. If you look at how we did conversions from wu to vb, you see that vb is rounded up Lines 146 to 151 in 7de8be4 This is only correct if we are considering a transaction as a whole (as per BIP-0141) However, many times we are converting from wu to vb for only a portion of a tx (such as satisfaction weight for coin selection) and then adding the portions up. This will result in overshooting weight calculations of the entire tx by quite a bit. |
|
Although this PR will increase the "correctness" of our weight calculations, if there are bugs in miniscript For reference: |
|
Also, if rust-bitcoin/rust-miniscript#476 gets merged, we need to do +5 for
|
I agree with this. The API should only take |
|
@nondiremanuel This should be mostly resolved by #1216 |
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
Description
Weight units should always be whole units. However, virtual bytes will need decimal places and the conversion from weight unit to vbytes should not be rounded.
This commit has the following changes:
f32Notes to the reviewers
Some "feerate" values for tests needed to be increased. This is because we were calling
.ceil()on weight unit to vbytes conversion before (which is wrong, and results in more "vbytes" than what is real).Changelog notice
Improve accuracy of fee/feerate calculations and conversions. Also renamed the
Vbytestrait toWeightUnits.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes:
* [ ] I've added tests to reproduce the issue which are now passing* [ ] I'm linking the issue being fixed by this PR