Fix Uint256::increment panics
#612
Conversation
TheBlueMatt
force-pushed
the
2021-06-fix-increment
branch
from
dc08760
to
9c256cc
Compare
There was a problem hiding this comment.
Frankly speaking, to be rust ideomatic, we should have inc function panic on overflows, and special wrapping_inc, checked_inc and overflowing_inc and saturating_inc functions... But we are not numeric crate, so the question is why we need this increment function at all? Shouldn't we implement those methods on UInt* types and just use appropriate *_add(1) for do the incrementing?
| @@ -162,6 +162,16 @@ macro_rules! construct_uint { | |||
|
|
|||
| ($name(ret), sub_copy) | |||
| } | |||
|
|
|||
| /// Increment by 1 | |||
There was a problem hiding this comment.
I think we need to explain in the doc that the function does wrapping increment
|
Personally I had to update/extend current UInt* implementations in https://docs.rs/amplify_num/0.1.1/amplify_num/ crate, and did all those arithmetic methods there, so they can be taken from it if needed here (or just use it since it is MSRV compatible) |
There was a problem hiding this comment.
Ack 9c256cc
Unrelated to this PR and also does not affect the correctness of the fuzzer, but while reviewing I found this line to be slightly confusing. I think it should be
if data.len() != 16*2 { return; }
But since the +1 is explicitly added, I maybe also be misunderstanding the code.
I think it should
https://github.com/TheBlueMatt/rust-bitcoin/blob/9c256cc88e4b71f7ee4e486a10b0be26c31a5f66/fuzz/fuzz_targets/uint128_fuzz.rs#L30
In an earlier version of the uint128 fuzz target, there was a byte which incidated the specific operation to test. However, that was dropped for the much-more-effective approach of simply testing all operations in each fuzz iteration.
Yep! Totally right. That's just stale against a (probably never upstreamed) version of that fuzz target. |
This is #578 with review feedback addressed.