Skip to content

fix: Return zero and insert an assertion if RHS bit size is over the limit in euclidian division#8294

Merged
aakoshh merged 9 commits intomasterfrom
af/8272-fix-mod-with-underflow
May 1, 2025
Merged

fix: Return zero and insert an assertion if RHS bit size is over the limit in euclidian division#8294
aakoshh merged 9 commits intomasterfrom
af/8272-fix-mod-with-underflow

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Apr 30, 2025

Description

Problem*

Resolves #8272
Also fixes #8274

In #8197 I changed the code to not panic if the RHS is larger than the expected bit size because of an overflow, but only for determining the quotient bit size, I still passed it to Brillig for the RHS and apparently also used it to set a range constraint on the result.

This time the value underflowed, and the negative result was actually represented by a 254 bit Field, which triggered another compile time failure (not panic) for ACIR (but not Brillig).

Summary*

This PR changes the code to completely ignore the RHS bit sizes over the operand size, trusting that there simply a range constraint inserted where the overflow happens which will prevent us from seeing any adverse affects from doing so.

This PR changes the code so that instead of ignoring the RHS bit size being over the operand size, it inserts an assert_eq(0, 1, "attempted to divide by constant larger than operand type") constraint, which will fail at runtime, unless of course we right assuming that there will have been an earlier overflow check which will catch the problem before we get here.

In practice this is what we see when we try to compile and execute the program:

cargo run -q -p nargo_cli -- compile --force 
bug: Assertion is always false
  ┌─ src/main.nr:5:13

5 │     b[0] = (b[0] % (869374236 - a));
  │             ---------------------- As a result, the compiled circuit is ensured to fail. Other assertions may also fail during execution

  = Call stack:
    1. src/main.nr:2:5
    2. src/main.nr:5:13cargo run -q -p nargo_cli -- execute 
error: Assertion failed: attempt to subtract with overflow
  ┌─ src/main.nr:5:21

5 │     b[0] = (b[0] % (869374236 - a));
  │                     -------------

  = Call stack:
    1. src/main.nr:2:5
    2. src/main.nr:5:21

Failed assertion

Additional Context

This fixes both bugs that cause flakiness in aztec-packages captured in #8271 when the smoke.rs test in noir_ast_fuzzer generates a random AST that doesn't compile.

I have not attempted to figure out how to replicate this behaviour for Brillig (the printing of a "bug"), since it does not fail to compile this program, and works identically at runtime.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@aakoshh aakoshh requested a review from a team April 30, 2025 15:21
@aakoshh aakoshh enabled auto-merge April 30, 2025 15:21
Copy link
Contributor

@guipublic guipublic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lhs and rhs are supposed to be of size 'bit_size', if it is not the case, then this is an error.

I think at that point you should simply return 0 and add an assert(false, "invalid bit size in euclidian division, expected {bit_size}, got {max_rhs_bits}").

@aakoshh
Copy link
Contributor Author

aakoshh commented Apr 30, 2025

@guipublic do you mean to insert an assertion instruction into ACIR?

do you know how the Brillig track handles this by any chance?

@guipublic
Copy link
Contributor

@guipublic do you mean to insert an assertion instruction into ACIR?
Yes

do you know how the Brillig track handles this by any chance?
I'd need to look, but the linked issue is about ACIR, saying it works for Brillig?

@aakoshh
Copy link
Contributor Author

aakoshh commented Apr 30, 2025

@guipublic yes, as the ticket shows if we use —force-brillig it compiles

@aakoshh aakoshh changed the title fix: Ignore the bit size of the RHS if it's over the limit in euclidian division fix: Return zero and insert an assertion if RHS bit size is over the limit in euclidian division Apr 30, 2025
@aakoshh aakoshh force-pushed the af/8272-fix-mod-with-underflow branch from c6cc7c0 to 10cf0c3 Compare April 30, 2025 20:23
Copy link
Contributor

@michaeljklein michaeljklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aakoshh aakoshh added this pull request to the merge queue May 1, 2025
Merged via the queue into master with commit f314498 May 1, 2025
115 checks passed
@aakoshh aakoshh deleted the af/8272-fix-mod-with-underflow branch May 1, 2025 15:44
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 2, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: disallow emitting multiple `MemoryInit` opcodes for the same block
(noir-lang/noir#8291)
fix(ssa): Remove unused calls to pure functions
(noir-lang/noir#8298)
fix(ssa): Do not remove unused checked binary ops
(noir-lang/noir#8303)
fix: Return zero and insert an assertion if RHS bit size is over the
limit in euclidian division
(noir-lang/noir#8294)
feat: remove unnecessary dynamic arrays when pushing onto slices
(noir-lang/noir#8287)
feat(testing): Add SSA interpreter for testing SSA
(noir-lang/noir#8115)
END_COMMIT_OVERRIDE

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants