Make `FeeRate` use MvB internally by tcharding · Pull Request #4534 · rust-bitcoin/rust-bitcoin

tcharding · 2025-05-21T01:29:49Z

The FeeRate is a bit entangled with amount and weight. Also we have an off-by-one bug caused by rounding errors and the fact that we use kwu internally.

We can get more precision in the fee rate by internally using per million virtual bytes.

tcharding · 2025-05-21T01:30:55Z

Question, do the checked functions in fee.rs need to be const?

github-actions · 2025-05-21T01:31:31Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-05-21T01:39:38Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-05-21T01:53:57Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-05-21T01:59:01Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-05-21T02:04:24Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-05-21T02:34:14Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

github-actions · 2025-05-21T03:11:16Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

apoelstra · 2025-05-21T19:42:45Z

Question, do the checked functions in fee.rs need to be const?

I'd like to get there eventually. But I think we'd be fine to release without it.

tcharding · 2025-05-21T21:19:13Z

No worries, we can either do it straight after this or as part of the RC cycle. We'll need to go over fee and fee_rate again with a fine tooth comb anyway. This PR gets us a fair way though.

apoelstra · 2025-05-21T22:24:29Z

The first two commits fa69cde and e38b5ef are unobjectionable and can be PR'd independently to shrink this one. The next on removes an encapsulate module so I guess needs to be in the big PR.

Same with a6062b2 (reorders functions).

Same with fc52783 (changes signatures of checked_add and checked_sub)

Same with 9575521 (drop dumb From impl) and 252ec87 (reorders tests)

apoelstra · 2025-05-21T22:26:41Z

        /// [`minimal_non_dust`]: Script::minimal_non_dust
        fn minimal_non_dust_custom(&self, dust_relay_fee: FeeRate) -> Option<Amount> {
-            self.minimal_non_dust_internal(dust_relay_fee.to_sat_per_kwu() * 4)
+            self.minimal_non_dust_internal(dust_relay_fee.to_sat_per_kwu_floor() * 4)


In 4831689:

This one should be _ceil since we're trying to put a floor on the set of feerates that'll get broadcast.

BTW the commit message says that you add FIXME comments but actually you don't.

Also IMO this commit could be moved outside of the "remove encapsulate" commit so we could PR it separately.

apoelstra · 2025-05-21T22:28:33Z

    pub const fn checked_div_by_fee_rate_floor(self, fee_rate: FeeRate) -> Option<Weight> {
        match self.to_sat().checked_mul(1000) {
-            Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu()) {
+            Some(amount_msats) => match amount_msats.checked_div(fee_rate.to_sat_per_kwu_floor()) {


In 4831689:

This one should be _ceil (somewhat weirdly, since the function name has _floor in it, but that's the nature of division).

Mad. Thanks for putting in the thought required to review this PR.

For the same reason should checked_div_by_fee_rate_ceil also use _ceil?

It should use _floor, for the same reason.

apoelstra · 2025-05-21T22:29:09Z

    /// Returns [`NumOpResult::Error`] if overflow occurred.
    pub const fn checked_mul_by_weight(self, weight: Weight) -> NumOpResult<Amount> {
-        if let Some(fee) = self.to_sat_per_kwu().checked_mul(weight.to_wu()) {
+        if let Some(fee) = self.to_sat_per_kwu_floor().checked_mul(weight.to_wu()) {


In 4831689:

This one should be ceil.

I think it should rather work on the internal representation whatever it is.

apoelstra · 2025-05-21T22:30:38Z

    /// Converts to sat/vB rounding up.
    pub const fn to_sat_per_vb_ceil(self) -> u64 {
-        (self.to_sat_per_kwu() + (1000 / 4 - 1)) / (1000 / 4)
+        (self.to_sat_per_kwu_floor() + (1000 / 4 - 1)) / (1000 / 4)


In 4831689:

This one should be ceil.

apoelstra · 2025-05-21T22:32:02Z

@@ -81,7 +84,7 @@ impl FeeRate {
    #[must_use]
    pub const fn checked_mul(self, rhs: u64) -> Option<Self> {


In 4831689:

I'm gonna OK these for now, but they need to be changed to do some sort of infallible conversion. Arguably they should be moved inside the encapsulate boundary.

Same with Sum.

And the NonZeroU64 ops.

apoelstra · 2025-05-21T22:32:55Z

    }
 }

 impl From<FeeRate> for u64 {


In 4831689:

Can we just delete this impl? Not to channel Kix too hard but it's really unclear what it should mean.

Ah, done in 9575521

apoelstra · 2025-05-21T22:45:14Z

Done reviewing everything except 3603b50 (the one that makes the actual change).

I identified 6 out of these 17 commits which are independent and unobjectionable. Can you PR those separately so we can reduce the size of this PR?

You can take or leave my comments about changing _floors to _ceils. I see that you replace many of them of with mvb conversions anyway.

apoelstra · 2025-05-21T22:47:39Z

Also would be fine to PR separately (or alongside the other unobjectionable commits) to remove consts. We should file an issue to put them back. It's not hard, just annoying.

tcharding · 2025-05-22T00:59:28Z

are unobjectionable and can be PR'd independently to shrink this one

PR storm incomiiiiiiiiiiiiiing!

tcharding · 2025-05-29T04:13:06Z

Changes in force push:

Added rustdocs as part of 1ee5754
Added additional patch with code comment

github-actions · 2025-05-29T04:39:34Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2025-05-29T04:39:36Z

Changes in force push are a typo found by linter, backticked code in rustdocs incorrectly.

This module is a PITA to work on, just remove it until the dust settles on fee rate. While we are at it make the rustdocs on the getter more terse.

In preparation for changing the inner representation of `FeeRate` add floor and ceil versions of the getter function `to_sat_per_kwu`. For now both functions return the same thing but still call the correct one so that when we change the representation we do not need to re-visit them.

Code that works with `const` is annoying to use and hard to reason about. Just remove all the consts for now so we can hack on `FeeRate`. Introduces two lint warnings about manual implementation of `map` but they will go away later.

We document the other to arguments already, add the missing one.

The `FeeRate::checked_mul_by_weight` function currently returns a `NumOpResult` but all other `checked_` functions return an `Option`. This is surprising and adds no additional information. Change `checked_mul_by_weight` to return `None` on overflow. But in `to_fee` saturate to `Amount::MAX` because doing so makes a few APIs better without any risk since a fee must be checked anyway so `Amount::MAX` as a fee is equally descriptive in the error case. This leads to removing the `NumOpResult` from `effective_value` also. Note that sadly we remove the very nice docs on `NumOpResult::map` because they no longer work. Fix: rust-bitcoin#4497

In preparation for changing the internal representation of `FeeRate` to use MvB reduce the max value by 4_000. Done separately to make the change explicit.

Currently we get the fee_rate per kwu then multiply it by 4. Instead lets add a per_kvb function to `FeeRate`. We are about to change the internal representation of `FeeRate` to use MvB so for now just panic on ovelflow. Also these are fee _rates_ - we already have suboptimal names from Core in the consts in `policy`, no need to let them infect other identifiers.

To get more precision use sats per million virtual bytes. To make review easier keep most calls in tests using `FeeRate::from_sats_per_kwu` and just unwrap. These can likely be cleaned up later on if we want to. For `serde` just change the module to `_floor` and leave it at that. The serde stuff likely needs re-visiting before release anyways.

Now we have the `fee_rate` module clened up re-introduce the `encapsulate` module using MvB.

Remove the unit from associated consts and then make all the rustdocs on the various consts use 'The'.

We temporarily removed `const` in the `fee` module to make patching and reviewing easier. Now add it back in.

During dev I introduced a pancic, remove it.

The `fee` functions are a bit convoluted because of the usage of `const`. Refactor a couple of them to be easier to read. Internal change only.

Its not immediately obvious that x - y cannot overflow. Add a code comment to explain it.

tcharding · 2025-05-31T06:52:52Z

Rebase only, no changes.

github-actions · 2025-05-31T06:53:51Z

🚨 API BREAKING CHANGE DETECTED

To see the changes click details on "Check semver breaks / PR Semver - stable toolchain" job then expand "Run semver checker script" and scroll to the end of the section.

tcharding · 2025-06-01T10:11:51Z

Any movement on this one @apoelstra?

apoelstra · 2025-06-03T14:08:04Z

Nice, thanks! I'm going to ACK and merge this.

@Kixunil I still feel that the checked_ methods should return an Option. If you feel strongly about it we can revert it, but as I said above, if we do, we should go through all the checked_ methods in the units crate and do the same thing.

Meanwhile I do not want to hold up this 15-commit PR any further about it.

apoelstra

ACK 5651675; successfully ran local tests

github-actions Bot added C-bitcoin PRs modifying the bitcoin crate C-units PRs modifying the units crate test labels May 21, 2025

github-actions Bot added the API break This PR requires a version bump for the next release label May 21, 2025

tcharding force-pushed the 05-21-fee-rate branch from 8517756 to ede17a6 Compare May 21, 2025 01:38

tcharding mentioned this pull request May 21, 2025

Saturate to Amount::MAX when calculating fee #4513

Closed

tcharding force-pushed the 05-21-fee-rate branch from 9643f84 to 2955a19 Compare May 21, 2025 01:57

tcharding force-pushed the 05-21-fee-rate branch from 2955a19 to ed80575 Compare May 21, 2025 02:03

tcharding force-pushed the 05-21-fee-rate branch from ed80575 to f2ed30f Compare May 21, 2025 02:32

tcharding force-pushed the 05-21-fee-rate branch from f2ed30f to e66cfae Compare May 21, 2025 03:09

apoelstra reviewed May 21, 2025

View reviewed changes

tcharding force-pushed the 05-21-fee-rate branch from a346a3c to 15dc7af Compare May 29, 2025 04:38

tcharding mentioned this pull request May 29, 2025

units: Bump version to 1.0.0-rc.0 #4204

Merged

tcharding added 15 commits May 31, 2025 07:51

Remove encapsulate module from fee rate

64098e4

This module is a PITA to work on, just remove it until the dust settles on fee rate. While we are at it make the rustdocs on the getter more terse.

Temporarily remove const from fee calc function

fe0a448

Code that works with `const` is annoying to use and hard to reason about. Just remove all the consts for now so we can hack on `FeeRate`. Introduces two lint warnings about manual implementation of `map` but they will go away later.

Add missing argument docs

64ac337

We document the other to arguments already, add the missing one.

Reduce the FeeRate::MAX value

399bca5

In preparation for changing the internal representation of `FeeRate` to use MvB reduce the max value by 4_000. Done separately to make the change explicit.

Re-introduce FeeRate encapsulate module

1bd1e89

Now we have the `fee_rate` module clened up re-introduce the `encapsulate` module using MvB.

Improve rustdocs on FeeRate

9b2fc02

Remove the unit from associated consts and then make all the rustdocs on the various consts use 'The'.

Make fee functions const

b658600

We temporarily removed `const` in the `fee` module to make patching and reviewing easier. Now add it back in.

Remove panic using checked arithmetic

bf0776e

During dev I introduced a pancic, remove it.

Refactor fee functions

7c186e6

The `fee` functions are a bit convoluted because of the usage of `const`. Refactor a couple of them to be easier to read. Internal change only.

Fix incorrect code comment

8cf1dc3

Add code comment to amount calculation

5651675

Its not immediately obvious that x - y cannot overflow. Add a code comment to explain it.

tcharding force-pushed the 05-21-fee-rate branch from 15dc7af to 5651675 Compare May 31, 2025 06:52

apoelstra approved these changes Jun 4, 2025

View reviewed changes

apoelstra merged commit a13ba99 into rust-bitcoin:master Jun 4, 2025

tcharding deleted the 05-21-fee-rate branch June 18, 2025 00:06

tcharding mentioned this pull request Jun 25, 2025

June 2025 Meeting Topics SydneyBitDevs/meetup#1

Closed

apoelstra mentioned this pull request Apr 14, 2026

Maybe FeeRate should be backed by something else? #5995

Closed

		@@ -81,7 +84,7 @@ impl FeeRate {
		#[must_use]
		pub const fn checked_mul(self, rhs: u64) -> Option<Self> {

Uh oh!

Conversation

tcharding commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

github-actions Bot commented May 21, 2025

Uh oh!

apoelstra commented May 21, 2025

Uh oh!

tcharding commented May 21, 2025

Uh oh!

apoelstra commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra commented May 21, 2025

Uh oh!

apoelstra commented May 21, 2025

Uh oh!

tcharding commented May 22, 2025

Uh oh!

tcharding commented May 29, 2025

Uh oh!

github-actions Bot commented May 29, 2025

Uh oh!

tcharding commented May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented May 31, 2025

Uh oh!

github-actions Bot commented May 31, 2025

Uh oh!

tcharding commented Jun 1, 2025

Uh oh!

apoelstra commented Jun 3, 2025

Uh oh!

apoelstra left a comment

tcharding commented May 21, 2025 •

edited

Loading

apoelstra commented May 21, 2025 •

edited

Loading

tcharding May 26, 2025 •

edited

Loading

tcharding commented May 29, 2025 •

edited

Loading