Fix Uint256::increment panics#612
Conversation
dc08760 to
9c256cc
Compare
dr-orlovsky
left a comment
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?
| ($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) |
sanket1729
left a comment
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.