Fix overflows in Weight and add fuzz target#4847
Fix overflows in Weight and add fuzz target#4847apoelstra merged 4 commits intorust-bitcoin:masterfrom
Weight and add fuzz target#4847Conversation
Pull Request Test Coverage Report for Build 17002522573Details
💛 - Coveralls |
|
Personally I think we should just delete this method. It has an undiscoverable name, uses the word "size" which is ambiguous, and it's redundant (before this PR it's identical to Weirdly all three of these methods date back to #1627, where they are right beside each other and don't have any of the constfn noise, and yet none of us noticed or commented on this. |
37190e7 to
8286aa9
Compare
|
I think I agree, it doesn't make sense to have two methods that do the same thing, with one of them being undiscoverable |
|
I've looked at these before, but never dug into the original PR. I came to the conclusion that the intent (just guessing) was for API symmetry.
IMO we should either remove a pair of them or leave them all. If we remove a pair of them we should probably add some docs for those devs not immediately familiar with wu vs vb and witness data vs non-witness data. This is not really to do with overflows so should be in a separate PR, especially because we will want to link to it in release notes. |
|
okay, sounds good. I've just added the overflow stuff here and the fuzz target since i used that as a test. I didn't fix |
|
|
||
| /// Constructs a new [`Weight`] from virtual bytes without an overflow check. | ||
| pub const fn from_vb_unchecked(vb: u64) -> Self { Weight::from_wu(vb * 4) } | ||
| pub const fn from_vb_unchecked(vb: u64) -> Self { Weight::from_wu(vb * Self::WITNESS_SCALE_FACTOR) } |
There was a problem hiding this comment.
nit: this line is unrelated to your commit and commit message.
There was a problem hiding this comment.
Same comment as above, this change is fine to do but should be done as a separate patch (on the front of this PR is fine).
There was a problem hiding this comment.
Done. Sorry about that, for some reason i have it in my head that i should not clutter the PR with unnecessary commits but your explanation makes a lot of sense. I'll try not to do this in the future
This version was bumped by the bot in commit 489118f so this keeps it consistent when the script is run
This fixes an overflow bug which occured when these constructors were used to construct a `Weight` of max value. Since `Weight` is a `u64` under the hood, the constructors panicked during the attempt to add to the max weight value
|
Nice, thanks for your effort man. |
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.)
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.)
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
…witness_data_size` 0cd384d Update `segwit_weight` and `legacy_weight` to use non-deprecated functions (Shing Him Ng) a11c037 Remove deprecated constructors from fuzz target (Shing Him Ng) fcc3473 Update `Weight` and `TxInExt` functions with panic details (Shing Him Ng) Pull request description: Weight::from_non_witness_data_size has an overflow bug. These methods duplicate the functionality of `Weight::from_vb` and `Weight::from_wu`, respectively. Furthermore, they are less discoverable than the methods they duplicate, so they will be removed in this PR. Related discussion in #4847 ACKs for top commit: tcharding: ACK 0cd384d apoelstra: ACK 0cd384d; successfully ran local tests Tree-SHA512: 56a370f02edde7108fa90120e359063788f2d2ce4b5b665216dfe67fe76485f79ba9a5d0c2fd454a8fe04795315bd8e81777c48ae39e663d4559bbf57d0e74cd
Pulling on the same thread from #4838, I started looking into
unitswhere a similar bugs may have happened and found a few forWeight. Found a few instances of the overflow bug in a few constructors so fixing them here