Fix increment of Uint256 with carry#578
Fix increment of Uint256 with carry#578carolcapps wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
| arr[2] = arr[2].wrapping_add(1); | ||
| if arr[2] == 0 { | ||
| arr[3] += 1; | ||
| arr[3] = arr[3].wrapping_add(1); |
There was a problem hiding this comment.
Shouldn't we let the last one panic (or rename the function to wrapping_)?
There was a problem hiding this comment.
In Uint256, add and multiply operation also don't panic when overflow. I think increment should follow their behavior, so if we prefer panic, we should change them too.
| @@ -516,13 +516,13 @@ impl Uint256 { | |||
| #[inline] | |||
| pub fn increment(&mut self) { | |||
There was a problem hiding this comment.
It would be nice if we could move this into the macro generation, that way we can add a fuzz case for it for u128 and get fuzz coverage.
There was a problem hiding this comment.
Err, can you go ahead and move it there in this PR (can be a separate commit). That would require swapping the if branches for a loop, but that should be easy.
dr-orlovsky
left a comment
There was a problem hiding this comment.
Shouldn't we follow Rust API and have different addition etc operations for UInt256 type which will at least match add (panicking) , saturating_add, wrapping_add and checked_add functions? Otherwise it's really strange that u32::add panics and UInt256 does wrapping
|
Is this PR still active? |
|
Superseded by #612. |
5d71a9d Correct input length check for uin128 fuzzer (Matt Corallo) 9c256cc Add a fuzz check for `Uint128::increment` (Matt Corallo) a15f263 Move the `increment` fn into the uint macro to add it to Uint128 (Matt Corallo) d52b88b Fix increment of Uint256 with carry (carolcapps) Pull request description: This is #578 with review feedback addressed. ACKs for top commit: apoelstra: ACK 5d71a9d sanket1729: ACK 5d71a9d Tree-SHA512: 32e5ea6387943ecad8f190a0de336a545fda72b6ff7388d3479037a5f880434276a7d0607f5cf61710d45e984c01954f4e3199a60c542be48b397717afb3d406
The issue is that panic occurs when increment
Uint256values with carry on Rust debug mode.Changed to use
wrapping_addas same asadd()function and add tests.