fix: prevent maximum fee rate overflow

[reez]

Description

Fixes overflow when converting and displaying extremely large FeeRate values (including PSBT fee overflow errors), and adds regression coverage for both paths.

Notes to the reviewers

Documentation

• bdk_wallet
• bitcoin
• uniffi
• Other:

Changelog

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing
• I've added exactly one changelog:* label
• I've linked the relevant upstream docs or specs above

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• 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

[ItoroD]

ACK 00a1163

[Johnosezele]

cACK, one small nit on my end

[Johnosezele]

small nit here: I think this conversion could be routed through the existing FeeRate wrapper

Suggested change

	BdkExtractTxError::AbsurdFeeRate { fee_rate, .. } => {
	let sat_per_vbyte = fee_rate.to_sat_per_vb_ceil();
	let sat_per_vbyte = fee_rate.to_sat_per_kwu().div_ceil(250);
	ExtractTxError::AbsurdFeeRate {
	fee_rate: sat_per_vbyte,
	}
	BdkExtractTxError::AbsurdFeeRate { fee_rate, .. } => ExtractTxError::AbsurdFeeRate {
	fee_rate: crate::bitcoin::FeeRate::from(fee_rate).to_sat_per_vb_ceil(),
	},

[reez]

Agreed, updated. I tweaked the suggestion slightly. Thanks!

[Johnosezele]

ACK b120060

[thunderbiscuit]

ACK b120060. This is a good fix to a bug. Ready to merge.

More thoughts

The issue has been flagged in rust-bitcoin however, and the fix has been merged there, so I opened #1122 to remind us to remove this custom logic here when we bump to the 0.33.0 version of rust-bitcoin.

The change on the Display trait reminded me of something I had brushed against in #859. We display the fee rate with 2 decimal points, but they can never be anything else than 0s! This is done I think to mirror what we're used to seeing (fee rates are often expresses as floating numnber, i.e. 2.96, but in our case it's kind of implying a lot more precision than our type can give. Check my table below for examples. Anyway it's not lying because the method name implies this is a ceiling, and we also offer the floor version, but it's odd!

Note that this has been discussed in rust-bitcoin, and their decision for the latest Feerate in 0.33 was actually to not have any Display implemented on the type, and recommend applications write their own.

FeeRate doc comment:

FeeRate explicitly does not have any format/display trait implementations, as it doesn't have a standard unit for measure. Users are expected to format it on their own by extracting values in desired units with from_sat_per* functions.

All of this to say that when we do upgrade, we'll have to write our own implemetation of the trait, or leave it to users to do it for themselves.

┌─────────┬─────────────┬────────────────┐
│ sat/kwu │ Real sat/vB │   Displayed    │
├─────────┼─────────────┼────────────────┤
│ 250     │ 1.0         │ 1.00 sat/vbyte │
├─────────┼─────────────┼────────────────┤
│ 253     │ 1.012       │ 2.00 sat/vbyte │
├─────────┼─────────────┼────────────────┤
│ 375     │ 1.5         │ 2.00 sat/vbyte │
├─────────┼─────────────┼────────────────┤
│ 25      │ 0.1         │ 1.00 sat/vbyte │
└─────────┴─────────────┴────────────────┘