fix: fix broken tests in runtime_bignum_test.nr
#39
Conversation
|
fixed an issue with the |
|
there's an issue with brillig. in line 42 of |
TomAFrench
force-pushed
the
cl/unique_test_names
branch
from
30af00f to
59363d0
Compare
src/fns/constrained_ops.nr
Outdated
| // this is a hack, the assert_max_bit_size is not working when limbs[i] is not a constant | ||
| // limbs[i].assert_max_bit_size::<120>(); | ||
| assert(limbs[i].lt(2.pow_32(120)), "call to assert_max_bit_size 2"); | ||
| } | ||
| limbs[N - 1].assert_max_bit_size::<MOD_BITS - ((N - 1) * 120)>(); | ||
| // this is a hack, the assert_max_bit_size is not working when limbs[i] is not a constant | ||
| // limbs[N - 1].assert_max_bit_size::<MOD_BITS - ((N - 1) * 120)>(); | ||
| assert( | ||
| limbs[N - 1].lt(2.pow_32((MOD_BITS as Field) - ((N - 1) * 120) as Field)), | ||
| "call to assert_max_bit_size", | ||
| ); |
There was a problem hiding this comment.
The tests are all passing with the commented out range checks. Is this still an issue?
There was a problem hiding this comment.
I think the asserts that I've added are doing the range check if I'm not making a mistake.
The issue was that as limb[N-1] is not a constant, the range check was happening for the bit size of the native field rather than the actual input.
There was a problem hiding this comment.
Yep, I agree that the added asserts are going a less efficient range check.
The issue was that as limb[N-1] is not a constant, the range check was happening for the bit size of the native field rather than the actual input.
Why would a non-constant field result in us not performing a check whether limb[N-1] fits within MOD_BITS - (N-1)*120 bits?
There was a problem hiding this comment.
Did you verify the output in the ACIR? If we're producing incorrect output here we shouldn't just hide that by rewriting the Noir.
There was a problem hiding this comment.
To be 100% clear, I'd like to restore the original calls to assert_max_bit_size so I'm keen to know why they were replaced and if it's due to a correctness issue in the compiler then we need to fix that.
TomAFrench
changed the title
runtime_bignum_test.nr
* main: feat: remove generic parameter from the `BigNum` trait (#44) fix: fix broken tests in `runtime_bignum_test.nr` (#39) feat: remove a bunch of unnecessary bytecode from unconstrained ops (#50) fix: Fix barrett reduction bug (#51) feat: optimize brillig execution of `split_X_bits` functions (#47)
Description
After making the comptime generated test names unique, now some fail.
--force-brilligSomeone more knowledgable than I will need to fix the tests before we can merge this.
PR Checklist*
cargo fmton default settings.