fix: bugs on border cases of udiv_mod #76
Conversation
Pablo-Dallegri
changed the title
| for _ in 0..(N * 120) { | ||
| if (remainder_u60.gte(b) == false) { | ||
| break; | ||
| if (divisor_u60.gte(remainder_u60 + U60Repr::one())) { |
There was a problem hiding this comment.
are these just formatter changes?
There was a problem hiding this comment.
They're from the if statement causing extra indentation.
| @@ -171,34 +171,38 @@ pub(crate) unconstrained fn __udiv_mod<let N: u32>( | |||
| let mut divisor_u60: U60Repr<N, 2> = U60Repr::from(divisor); | |||
| let b = divisor_u60; | |||
|
|
|||
| let mut bit_difference = remainder_u60.get_msb() - divisor_u60.get_msb(); | |||
There was a problem hiding this comment.
I see how this line can cause issues when the denominator has more bits in its u60 representation. Thanks for catching this!
src/fns/unconstrained_ops.nr
Outdated
| @@ -171,34 +171,38 @@ pub(crate) unconstrained fn __udiv_mod<let N: u32>( | |||
| let mut divisor_u60: U60Repr<N, 2> = U60Repr::from(divisor); | |||
| let b = divisor_u60; | |||
|
|
|||
| let mut bit_difference = remainder_u60.get_msb() - divisor_u60.get_msb(); | |||
| if !remainder_u60.gte(divisor_u60) { | |||
There was a problem hiding this comment.
I wonder if there would be a better way to catch this, without using the if statement. @TomAFrench would it be more efficient to look at the index where the msb of the denominator is rather than using the if statement?
regardless the change should fix this issue.
There was a problem hiding this comment.
Yeah it looks like it would be better to get the two msbs and compare these directly.
There was a problem hiding this comment.
@Pablo-Dallegri would you like to tackle this yourself or should I try to make the change before merging the PR?
Description
Problem*
Failures when attempting to prove certain
udiv_modoperations.Summary*
🪲 Unsigned integer division fails if the numerator can be represented with less bits than the divisor, due to an underflow on the subtraction at:
noir-bignum/src/fns/unconstrained_ops.nr
Line 174 in 1fcb2c9
🐞 Unsigned integer division also fails in cases were the least significant limb of the remainder is 2¹²⁰-1, due to a miscalculation of borrow flags, halting the proving at:
noir-bignum/src/fns/constrained_ops.nr
Line 463 in 1fcb2c9
🔧 This PR adds tests for both cases, and proposes fixes.
Additional Context
These bugs where found while developing in the context of NRG#2, and tests were reduced to minimal examples.
PR Checklist*
cargo fmton default settings.