Skip to content

feat: return unsolvable opcode from CircuitSimulator#8943

Merged
guipublic merged 22 commits intomasterfrom
gd/issue_8774
Jul 25, 2025
Merged

feat: return unsolvable opcode from CircuitSimulator#8943
guipublic merged 22 commits intomasterfrom
gd/issue_8774

Conversation

@guipublic
Copy link
Contributor

Description

Problem*

Resolves #8774

Summary*

The compiler crash is transformed into an error during execution by using non solvable witness to represent unresolved loads.

Additional Context

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.

@guipublic guipublic requested a review from a team June 18, 2025 08:15
@asterite
Copy link
Collaborator

I wonder, the SSA in the issue is this one:

acir(inline) predicate_pure fn main f0 {
  b0():
    v0 = make_array [] : [&mut u1; 0]
    constrain u1 0 == u1 1, "Index out of bounds"
    v4 = array_get v0, index u32 0 -> &mut u1
    v5 = load v4 -> u1
    return v5
}

I guess it crashes because there's an array get of an empty array, then loading that value...

Could we instead have an SSA pass that recursively removes values that can't happen? I'm thinking that v4 can't succeed regardless of the index because the array is empty, so we'd remove that instruction. Now that v4 was removed, v5 should be removed too because it uses that, etc. The tricky part is removing the return, or replacing it with a zeroed value I guess.

@asterite
Copy link
Collaborator

Actually, this might be simpler. In this SSA:

acir(inline) predicate_pure fn main f0 {
  b0():
    v0 = make_array [] : [&mut u1; 0]
    constrain u1 0 == u1 1, "Index out of bounds"
    v4 = array_get v0, index u32 0 -> &mut u1
    v5 = load v4 -> u1
    return v5
}

there's a constrain that will always fail. Then we can remove any subsequent instructions in that block and replace the terminator values with zeroed values (the "zeroed values" logic was introduced in a recent PR).

@asterite
Copy link
Collaborator

I'll try the above in a separate PR, just because now I'm curious about it 😊

@TomAFrench
Copy link
Member

This is superceded by #9008 right @asterite?

@asterite
Copy link
Collaborator

asterite commented Jun 25, 2025

@TomAFrench Yes. I didn't know if this PR was also wanted, though I guess now unresolved loads can't (shouldn't) happen anymore.

@guipublic
Copy link
Contributor Author

This PR is different than #9008 and removes a compiler crash. I don't think there is a 100% guarantee that loads will always be removed.

@aakoshh
Copy link
Contributor

aakoshh commented Jun 25, 2025

About load removal: I added a panic if they are still present after DIE in #9018

I guess this is something that could be turned into a warning if we find a case we cannot eliminate.

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Let's revert the unreachable on Instruction::Load but we can keep the callstack changes

@guipublic
Copy link
Contributor Author

I reverted the changes to unresolved loads.

@guipublic guipublic requested a review from jfecher July 11, 2025 14:57
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: e1af39d Previous: f073164 Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob 3 s 2 s 1.50
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib 2 s 1 s 2
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib 4 s 3 s 1.33
test_report_zkpassport_noir-ecdsa_ 2 s 1 s 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 1e0d05a Previous: 80b04ec Ratio
private-kernel-inner 0.018 s 0.014 s 1.29

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Member

@TomAFrench TomAFrench left a comment

Choose a reason for hiding this comment

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

LGTM

@guipublic guipublic added this pull request to the merge queue Jul 24, 2025
@TomAFrench TomAFrench changed the title fix: map unresolved loads to not solvable witness feat: return unsolvable opcode from CircuitSimulator Jul 24, 2025
@TomAFrench TomAFrench removed this pull request from the merge queue due to a manual request Jul 24, 2025
@TomAFrench TomAFrench added this pull request to the merge queue Jul 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2025
@guipublic guipublic added this pull request to the merge queue Jul 25, 2025
Merged via the queue into master with commit cb861f8 Jul 25, 2025
103 checks passed
@guipublic guipublic deleted the gd/issue_8774 branch July 25, 2025 08:38
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 4, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: forbid self-referencing type aliases
(noir-lang/noir#9103)
chore: add a mem2reg test for when all references need to be invalidated
(noir-lang/noir#9377)
fix(ssa): Do not check ArrayGet/Set as unreachable for Brillig
(noir-lang/noir#9376)
chore: use SSA parser in all mem2reg tests
(noir-lang/noir#9372)
fix: trait where clause check fixes
(noir-lang/noir#9369)
fix: Correct doc comments for SSA passes
(noir-lang/noir#9371)
fix: prevent `SignedField::from(i128::MIN)` from crashing
(noir-lang/noir#9366)
fix: allow constants in the type-system to be negative
(noir-lang/noir#9360)
feat: show circuit output as a value of the program's return type
(noir-lang/noir#9364)
feat: add `FunctionDefinition::visibility`
(noir-lang/noir#9363)
chore(docs): Add example for `$crate` in docs
(noir-lang/noir#9361)
fix: Prevent accidental tuple sharing in comptime code
(noir-lang/noir#9313)
fix: perserve purities after SSA normalization
(noir-lang/noir#9355)
fix: modulo overflow in comptime
(noir-lang/noir#9348)
fix: handle short-syntax for trait constraints on trait generics
(noir-lang/noir#9167)
chore: enhance trait constraint comment
(noir-lang/noir#9358)
fix: replace implicitly added named generics with fresh type vars in
check_trait_impl_where_clause_matches_trait_where_clause
(noir-lang/noir#9352)
fix: push definition trait constraints after trait item constraint
(noir-lang/noir#9354)
chore(ci): Update status of noir_json_parser
(noir-lang/noir#9351)
fix(ssa): Keep reference count increments for array set values
(noir-lang/noir#9344)
chore: remove unused `compile_workspace`
(noir-lang/noir#9353)
chore: try printing byte arrays as strings in the SSA interpreter
(noir-lang/noir#9346)
feat(lsp): allow opening noir stdlib files
(noir-lang/noir#9339)
fix: do u128 operations with u128, not i128
(noir-lang/noir#9345)
chore(acir): ACIR parser error handling for blackbox inputs/outputs
(noir-lang/noir#9342)
fix: prevent invalid types in test/fuzz functions
(noir-lang/noir#9343)
chore(lsp): avoid redundant type checking
(noir-lang/noir#9337)
feat(acir): Parse ACIR memory and call opcodes
(noir-lang/noir#9331)
fix(ssa_gen): Add constraint on slice length before popping
(noir-lang/noir#9323)
chore: impl for u16 conversions
(noir-lang/noir#9314)
fix: substitute bindings in type before canonicalization
(noir-lang/noir#9328)
fix(ssa_interpreter): `push_back` and `pop_back` to slices with padding
(noir-lang/noir#9320)
fix: wildcard type should be allowed in lambda parameter types
(noir-lang/noir#9325)
chore: graceful handling of SIGPIPE
(noir-lang/noir#9075)
feat: return unsolvable opcode from `CircuitSimulator`
(noir-lang/noir#8943)
fix: allow nested fmtstr (noir-lang/noir#9309)
feat: Initial ACIR parser (arithmetic exprs and black box functions)
(noir-lang/noir#9316)
fix(mem2reg): Register aliases when the `IfElse` result in a reference
(noir-lang/noir#9305)
fix: Make Ssa-gen use existing reference when compiling `&mut
foo.bar.baz` (noir-lang/noir#9307)
fix: top-level item in dependency isn't always visible
(noir-lang/noir#9295)
fix(ssa-interpreter): Return error if slice length is 0 during popping
(noir-lang/noir#9308)
chore: Release Noir(1.0.0-beta.9)
(noir-lang/noir#9184)
chore(LSP): simplify code lens request handling
(noir-lang/noir#9279)
chore: add regression tests for #6383
(noir-lang/noir#9302)
fix: disallow `_` in signatures and struct members
(noir-lang/noir#9301)
fix: check associated types after validating where clause when looking
up trait impls, plus some unification fixes
(noir-lang/noir#9265)
chore: Add fmtstr to coercions list
(noir-lang/noir#9300)
chore: Add a helper function `fmtstr::as_quoted_str`
(noir-lang/noir#9293)
chore(docs): Copy Type Coercions docs into v1.0.0-beta.8 versioned docs
(noir-lang/noir#9298)
feat: only inject "out of bounds" checks in brillig
(noir-lang/noir#9200)
END_COMMIT_OVERRIDE

---------

Co-authored-by: AztecBot <tech@aztecprotocol.com>
Co-authored-by: Maxim Vezenov <mvezenov@gmail.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

Development

Successfully merging this pull request may close these issues.

Compiler crash: Expected all load instructions to be removed before acir_gen

5 participants