`FeeRate` calculation fixes

[evanlinjin]

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:

• 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)

Notes 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 Vbytes trait to WeightUnits.

Checklists

All Submissions:

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

Bugfixes:

• [x] This pull request breaks the existing API ~* [ ] I've added tests to reproduce the issue which are now passing~ ~* [ ] I'm linking the issue being fixed by this PR~

[danielabrozzoni]

Weight units should always be whole units.

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 :tm: (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 :)

[evanlinjin]

@danielabrozzoni

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)

Think about it this way, if we look at the smallest possible "unit" of a transaction, for example, an VarInt(1) that records witness stack length, the weight of this VarInt is 1. This is the smallest possible "unit", and so, weight units should always be whole integers.

I'm talking about weight units (wu), not feerate (sats/wu). Feerates (whether sats/wu or sats/vb), should always be in floats (unless we work in kilo-{wu|vb} units).

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 :)

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 .ceil.

https://github.com/bitcoindevkit/bdk/blob/7de8be46c04128328d8dbd1de7b29033a290fde8/src/types.rs#L146-L151

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.

[evanlinjin]

Although this PR will increase the "correctness" of our weight calculations, if there are bugs in miniscript max_satisfaction_weight we may end up undershooting, which is dangerous. I think this should NOT be merged as of now until we confirm that miniscript is correct.

For reference:

• https://github.com/rust-bitcoin/rust-miniscript/pull/472
• https://github.com/rust-bitcoin/rust-miniscript/pull/473
• https://github.com/rust-bitcoin/rust-miniscript/pull/474

[evanlinjin]

Also, if https://github.com/rust-bitcoin/rust-miniscript/pull/476 gets merged, we need to do +5 for TXIN_BASE_WEIGHT.

• +1 WU for witness field stack size varint
• +4 WU for scriptSig size varint

[LLFourn]

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.

I agree with this. The API should only take fee_for(tx: &Transaction) otherwise you are almost certainly using it incorrectly.

[ValuedMammal]

@nondiremanuel This should be mostly resolved by #1216