Skip to content

fix: Do not panic if RHS constant in division has more bits than the operand#8197

Merged
aakoshh merged 5 commits intomasterfrom
af/8195-fix-div-larger-than-op
Apr 24, 2025
Merged

fix: Do not panic if RHS constant in division has more bits than the operand#8197
aakoshh merged 5 commits intomasterfrom
af/8195-fix-div-larger-than-op

Conversation

@aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Apr 23, 2025

Description

Problem*

Resolves #8195

Summary*

Changes acir_context::euclidean_division_var to not assert that the RHS, when it's a constant, must not have more bits than the operand type, and instead it lets the runtime handle the problem.

Additional Context

In the failing program we have the following final SSA:

g0 = u16 41618

acir(inline) predicate_pure fn main f0 {
  b0(v1: u16):
    v3 = sub u16 13834, v1
    v4 = mul u16 41618, u16 41618
    v5 = mod v3, v4
    return v5
}

During ACIR generation convert_ssa_binary calls AcirContext::mul_var to handle mul u16 41618, u16 41618, which runs the following:

            (AcirVarData::Const(lhs_constant), AcirVarData::Const(rhs_constant)) => {
                self.add_constant(lhs_constant * rhs_constant)
            }

That adds an AcirVar with the value 1732057924. Then, back in convert_ssa_binary, we insert a range constraint in check_unsigned_overflow, and move on. When we handle mod v3, v4, we retrieve 1732057924 for v4, and panic because it wouldn't fit into 16 bits. But at runtime we would have failed the multiplication overflow constraint already.

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 23, 2025 22:18
@aakoshh aakoshh requested a review from TomAFrench April 24, 2025 13:49
@TomAFrench
Copy link
Member

I'm a little concerned as we'll be telling the brillig call that the inputs are a smaller size than they are in reality but we should always error beforehand.

@TomAFrench
Copy link
Member

Perhaps we should inject a failing constraint at this point just to be sure?

@aakoshh
Copy link
Contributor Author

aakoshh commented Apr 24, 2025

I'm a little concerned as we'll be telling the brillig call that the inputs are a smaller size than they are in reality

Yes, that bothers me as well. I was wondering if it would be worth adjusting it, but then again this shouldn't actually be invoked, and I'm not sure it would even make sense or whether it could handle it.

@aakoshh aakoshh enabled auto-merge April 24, 2025 13:56
@aakoshh aakoshh added this pull request to the merge queue Apr 24, 2025
Merged via the queue into master with commit bddf22d Apr 24, 2025
114 checks passed
@aakoshh aakoshh deleted the af/8195-fix-div-larger-than-op branch April 24, 2025 15:03
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Apr 28, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: let `nargo test` use the new greybox fuzzer
(noir-lang/noir#8234)
chore(docs): add fixed bugs to list
(noir-lang/noir#8201)
fix(parser): do not use `Ident::default()`
(noir-lang/noir#8224)
feat(fuzz): Add experimental comptime code generator
(noir-lang/noir#8207)
fix: let SSA parser parse calls to print
(noir-lang/noir#8232)
feat: added 1, -1 case for inversions in brillig
(noir-lang/noir#8225)
chore: clippy (noir-lang/noir#8220)
fix: don't take implicitly added named generics when checking where
clauses (noir-lang/noir#8184)
chore(docs): Defunctionalization and some minor cleanup
(noir-lang/noir#8217)
feat: add `--no-fuzz` and `--only-fuzz` to `nargo test`
(noir-lang/noir#8213)
chore: add regression test for panic encountered in bigcurve library.
(noir-lang/noir#8211)
fix: validate numeric types when passing them to a FunctionBuilder
(noir-lang/noir#8215)
chore: refactor constrainedness checks on trait impls
(noir-lang/noir#8170)
chore(test): Add smoke test for AST generation
(noir-lang/noir#8048)
chore: bump more dependencies
(noir-lang/noir#8208)
chore: Don't use `i1` in the AST fuzzer
(noir-lang/noir#8204)
fix: Do not panic if RHS constant in division has more bits than the
operand (noir-lang/noir#8197)
chore: bump cargo deps (noir-lang/noir#8205)
fix: Handle truncating constants to 128 bits
(noir-lang/noir#8180)
fix!: disallow the `i1` type
(noir-lang/noir#8172)
chore(fuzz): Make sure `main` makes at least one call
(noir-lang/noir#8202)
fix: Use `get_local_or_global_instruction` in
`try_optimize_array_set_from_previous_get`
(noir-lang/noir#8200)
fix: returns 0 for right shift overflow
(noir-lang/noir#8189)
feat: avoid overflow check when subtracting from a value that is the
maximum for its bitsize (noir-lang/noir#8190)
feat: optimize `checked_to_unchecked` to take into account
multiplications (noir-lang/noir#8188)
chore: document ssa `inline_simple_functions`
(noir-lang/noir#8169)
chore(docs): remove_unreachable SSA pass
(noir-lang/noir#8196)
chore: Change AST fuzzer recursion limit
(noir-lang/noir#8173)
chore(acir): Unify how we display witness indices
(noir-lang/noir#8192)
fix: Detect ABI change of fuzzing harnesses
(noir-lang/noir#8193)
chore(acir): Test that `BLACKBOX::RANGE` display the number of bits
(noir-lang/noir#8191)
fix(debugger): send idle at loop end + fix test
(noir-lang/noir#8187)
fix: Use `get_local_or_global_instruction` when simplifying `IfElse`
(noir-lang/noir#8185)
fix(parser): avoid using `Location::dummy()`
(noir-lang/noir#8178)
chore: Push a bug list (noir-lang/noir#8186)
feat(greybox_fuzzer): Should_fail and should_fail_with
(noir-lang/noir#8118)
chore: move unsigned overflow check from acir/brillig to ssa
(noir-lang/noir#8163)
feat: location tree for debug_info
(noir-lang/noir#7034)
fix: Use `IntegerConstant` for loop boundaries in `unrolling`
(noir-lang/noir#8094)
fix(ssa): Recursive shared Brillig entry points
(noir-lang/noir#8099)
chore: enable '--pedantic-solving' on more tests
(noir-lang/noir#7701)
chore(readme): Update `acvm-repo` READMEs
(noir-lang/noir#8150)
chore(test): add `test_programs` dir for expected-panic tests
(noir-lang/noir#8147)
chore(docs): minor updates in solidity doc
(noir-lang/noir#8160)
feat(debugger): debug test functions
(noir-lang/noir#7958)
chore: migrate recursive proof test to ultrahonk
(noir-lang/noir#8038)
chore: bump glob package (noir-lang/noir#8159)
chore: bump external pinned commits
(noir-lang/noir#8139)
chore(docs): patch web app tutorial in 1.0.0-beta3 versioned docs
(noir-lang/noir#8158)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Ary Borenszweig <asterite@gmail.com>
Co-authored-by: TomAFrench <tom@tomfren.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compiler crash in acir_context: attempted to divide by constant larger than operand type

2 participants