Skip to content

Fix overflow during *_ceil FeeRate conversions#4838

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
shinghim:fee-ceil
Aug 13, 2025
Merged

Fix overflow during *_ceil FeeRate conversions#4838
apoelstra merged 1 commit intorust-bitcoin:masterfrom
shinghim:fee-ceil

Conversation

@shinghim
Copy link
Copy Markdown
Contributor

@shinghim shinghim commented Aug 11, 2025

During some work on updating the fuzz targets, I came across an overflow error during this call when FeeRate::MAX was passed in:

script.minimal_non_dust_custom(fee_rate);

This calls FeeRate::to_sat_per_kvb_ceil() under the covers, which was where the overflow was happening. I also updated similar functions that would have run into the same issue

@github-actions github-actions bot added the C-units PRs modifying the units crate label Aug 11, 2025
@coveralls
Copy link
Copy Markdown

coveralls commented Aug 11, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 16911897866

Details

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 83.389%
Totals Coverage Status
Change from base Build 16909646800: 0.03%
Covered Lines: 21547
Relevant Lines: 25839
💛 - Coveralls

@shinghim shinghim marked this pull request as ready for review August 11, 2025 22:12
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Aug 12, 2025 •
edited
Loading

Appreciate the effort. units is stablised (almost). Any patches to it imply either bugs and/or a reasonably serious oversight.

I'd like to see a bit more discussion in the commit log about what is wrong and why the fix is correct. Also can we have a patch on top of this that adds a test that fails if the patch is re-ordered before the fix please.

You might have also uncovered a whole class of bugs. So, for example, we may need to add tests to the whole module for any function that has a '+' in it (or some such thing). You don't have to do that but if its necessary could you either raise an issue or ask me and I'll do it. Thanks man, good fuzzing!

@shinghim
Fix overflow during *_ceil FeeRate conversions
dc2de6a 
This fixes an overflow bug which occured when these ceil functions were
called on `FeeRate::MAX`, which is u64::MAX under the hood. This
resulted in the function panicking due to an overflow since part of the
ceil operation involves adding to the FeeRate before dividing by the
appropriate unit. By calling `saturating_add` instead of using `+`, this
prevents the function from panicking while preserving the expected
behavior when converting `FeeRate`s that would have previously
overflowed
@shinghim
Copy link
Copy Markdown
Contributor Author

shinghim commented Aug 12, 2025

@tcharding Updated the commit log. Let me know if it's any good, I can fix it if there's not enough detail or doesn't explain well enough.

Also can we have a patch on top of this that adds a test that fails if the patch is re-ordered before the fix please.

do you mean moving the unit tests into a separate commit or adding another test somewhere else?

You don't have to do that but if its necessary could you either raise an issue or ask me and I'll do it.

I'd be happy to look through for more instances of these. Is there a date in mind for the units release? I'll get it done before then 😄

@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Aug 12, 2025

cc #3339 since in 1.73+ we have the div_ceil method which exists exactly to avoid this overflow

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Aug 13, 2025

@tcharding Updated the commit log. Let me know if it's any good, I can fix it if there's not enough detail or doesn't explain well enough.

Thanks for making the effort.

Also can we have a patch on top of this that adds a test that fails if the patch is re-ordered before the fix please.

do you mean moving the unit tests into a separate commit or adding another test somewhere else?

Don't worry its fine like this. But typically when we find a bug we do the fix as a patch (or patches) then add an additional patch that adds a test. Then reviewers can move the test patch up front and see it fails. In this case the code is trivial so reading it is enough. FTR yesterday when I was brain fried I didn't know if this was semantically correct, although I then wasted time convincing myself today before I saw the test, seeing the test convinced me instantly and easily with basically no brain required.

You don't have to do that but if its necessary could you either raise an issue or ask me and I'll do it.

I'd be happy to look through for more instances of these. Is there a date in mind for the units release? I'll get it done before then 😄

I had a look at fee_rate but did not check other modules in units. I did create #4846 though.

Thanks man, appreciate the work.

tcharding
tcharding approved these changes Aug 13, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dc2de6a

@shinghim shinghim mentioned this pull request Aug 13, 2025
Merged
apoelstra
apoelstra approved these changes Aug 13, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK dc2de6a; successfully ran local tests

@apoelstra apoelstra merged commit 3b8f259 into rust-bitcoin:master Aug 13, 2025
25 checks passed
apoelstra added a commit that referenced this pull request Aug 22, 2025
@apoelstra
Merge #4847: Fix overflows in Weight and add fuzz target
911e143 
aa086a0 Fix overflow bug in `Weight` constructors (Shing Him Ng)
b762f74 Add fuzz target for `Weight` (Shing Him Ng)
e11d3ed Update `from_vb_unchecked` to use constant (Shing Him Ng)
e815d32 Bump version of workflow in fuzz file generation (Shing Him Ng)

Pull request description:

  Pulling on the same thread from #4838, I started looking into `units` where a similar bugs may have happened and found a few for `Weight`. Found a few instances of the overflow bug in a few constructors so fixing them here


ACKs for top commit:
  tcharding:
    ACK aa086a0
  apoelstra:
    ACK aa086a0; successfully ran local tests


Tree-SHA512: 8a586ed3fdc6bd27c86ff218c3ae4ac79d51a43712af4af85d5caea8252b730cdceed967c8eb13ffd05e010404478de195dd3a4759a1e76d3b60a3880e555b36
apoelstra added a commit to apoelstra/rust-bitcoin that referenced this pull request Sep 1, 2025
@apoelstra
revert rust-bitcoin#4838 and rust-bitcoin#4847 using div_ceil
3bdc48f 
In rust-bitcoin#4838 we used saturating addition to simulate `div_ceil`, which gives
incorrect results on extreme values but at least doesn't panic. Similar
in the followup PR rust-bitcoin#4847. (These aren't detected by clippy, but I remember
them.)
apoelstra added a commit to apoelstra/rust-bitcoin that referenced this pull request Sep 1, 2025
@apoelstra
revert rust-bitcoin#4838 and rust-bitcoin#4847 using div_ceil
2b38ffc 
In rust-bitcoin#4838 we used saturating addition to simulate `div_ceil`, which gives
incorrect results on extreme values but at least doesn't panic. Similar
in the followup PR rust-bitcoin#4847. (These aren't detected by clippy, but I remember
them.)
apoelstra added a commit that referenced this pull request Sep 2, 2025
@apoelstra
Merge #4926: Bump MSRV from 1.63.0 to 1.74.0 for all crates in the repo
820ba7b 
9b7e920 remove some pointer casts (Andrew Poelstra)
2b38ffc revert #4838 and #4847 using div_ceil (Andrew Poelstra)
5a04527 clippy: fix new clippy links (Andrew Poelstra)
b90c4e8 internals: update MSRV to 1.74 (Andrew Poelstra)
c2ab0f7 update MSRV to 1.74 everywhere except in internals (Andrew Poelstra)

Pull request description:

  Bump MSRV from 1.63 to 1.74 for all crates in this repo.
  
  Closes #3339.
  
  We should open a new tracking issue.


ACKs for top commit:
  tcharding:
    ACK 9b7e920


Tree-SHA512: 7e5ab81b359b72696e1fca728c9096abd6fb0ade0cebd1df9401c148be87f1fe716972ea4742a7f05e0fca3b100142aec329a911be2f712af112999b5d13e215
@shinghim shinghim deleted the fee-ceil branch September 30, 2025 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apoelstra apoelstra apoelstra approved these changes

@tcharding tcharding tcharding approved these changes

Assignees

No one assigned

Labels

C-units PRs modifying the units crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shinghim @coveralls @tcharding @apoelstra