Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: set index and value to 0 for array_get with predicate #4971

Merged
merged 14 commits into from
May 7, 2024

Conversation

guipublic
Copy link
Contributor

@guipublic guipublic commented May 3, 2024

Description

Problem*

Resolves issue on noir-protocol-circuit and should also resolve issues described in #4716.
The issue it resolves is that array_get under a predicate are done at index 0 if the predicate is false, but since arrays are not homogenous, it may contain some value that overflow the type of the array_get.

Summary*

When getting value from an array_get, I multiply it with the predicate to avoid any possible overflow if element at index 0 has not the same type as the expected one.

Additional Context

If the array is simple (not nested), I get the offset to a compatible type and use it when computing the predicate_index.
If the first element of the array has compatible size, then the predicate_index will be correct
If not, we fallback to the multiplication of the value with the predicate.

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.

Copy link
Contributor

github-actions bot commented May 3, 2024

Changes to circuit sizes

Generated at commit: fe174de19d37d04f7dbc98ab69147770c5982c21, compared to commit: f3f11507983009771656811f9570bdbe6849c7ef

🧾 Summary (10% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap +926 ❌ +0.53% +926 ❌ +0.25%
slices +1 ❌ +0.29% +1 ❌ +0.03%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
hashmap 176,988 (+926) +0.53% 368,860 (+926) +0.25%
slices 351 (+1) +0.29% 3,421 (+1) +0.03%
slice_dynamic_index 1,355 (+2) +0.15% 6,949 (+2) +0.03%
nested_array_dynamic 4,585 (+1) +0.02% 12,952 (+1) +0.01%

@TomAFrench
Copy link
Member

It would be interesting to compare this against the solution of adding an index offset instead of reading 0 to ensure that we read the correct typed value from the array. This keeps the final value a single witness rather than a quadratic expression so should result in better codegen in later opcodes.

@TomAFrench
Copy link
Member

TomAFrench commented May 3, 2024

The above comment isn't a blocker for this PR and I've opened #4972 to follow up on this.

@TomAFrench TomAFrench enabled auto-merge May 3, 2024 17:50
@TomAFrench
Copy link
Member

It also seems like we can be smarter about when to multiply in the predicate/add an offset to the index. We only need to do this when the type which we would read is larger than the type we want to read. We can then avoid this in two cases.

  1. The array is homogeneous (I think this would cut out 99% of the circuit size increase we see)
  2. The array is heterogeneous but the type of the first field of the element type is smaller than the type we want to read. i.e. [Struct {foo: bool, bar: Field}; 3] can always read the first field in the flattened array safely and use its value for all other indices.

@guipublic
Copy link
Contributor Author

It also seems like we can be smarter about when to multiply in the predicate/add an offset to the index. We only need to do this when the type which we would read is larger than the type we want to read.

Good point, I have also added the use of the offset when the array is not a nested array. So the rule now is:

  • if array is not nested, find the offset and use it for the predicate_index, else
  • if the first element of the array has compatible bit_size, we can use directly the predicate_index, else
  • fallback to the multiply the value with the predicate.

I have updated the PR description.
I could not get the offset to work with nested arrays but it is definitively do-able. However, almost all cases are now properly optimised.

compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
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.

Some nits but good to go.

compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs Outdated Show resolved Hide resolved
@TomAFrench TomAFrench added this pull request to the merge queue May 7, 2024
Merged via the queue into master with commit c49d3a9 May 7, 2024
43 checks passed
@TomAFrench TomAFrench deleted the gd/pw_issue branch May 7, 2024 08:57
AztecBot added a commit to AztecProtocol/aztec-packages that referenced this pull request May 7, 2024
noir-lang/noir#4971)

fix: Compute the correct slice length when coercing from a literal array of complex types (noir-lang/noir#4986)
feat: add `Neg` trait to stdlib (noir-lang/noir#4983)
feat: implement `From` array trait for `BoundedVec` (noir-lang/noir#4927)
chore: Release Noir(0.29.0) (noir-lang/noir#4905)
fix: Move remove_if_else pass after second inlining  (noir-lang/noir#4976)
TomAFrench added a commit to AztecProtocol/aztec-packages that referenced this pull request May 7, 2024
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: set index and value to 0 for array_get with predicate
(noir-lang/noir#4971)
fix: Compute the correct slice length when coercing from a literal array
of complex types (noir-lang/noir#4986)
feat: add `Neg` trait to stdlib
(noir-lang/noir#4983)
feat: implement `From` array trait for `BoundedVec`
(noir-lang/noir#4927)
chore: Release Noir(0.29.0)
(noir-lang/noir#4905)
fix: Move remove_if_else pass after second inlining
(noir-lang/noir#4976)
END_COMMIT_OVERRIDE

---------

Co-authored-by: Tom French <[email protected]>
TomAFrench added a commit that referenced this pull request May 8, 2024
* master: (41 commits)
  fix: defer overflow checks for unsigned integers to acir-gen (#4832)
  feat: add support for u16/i16 (#4985)
  chore: split `ops` into `arith` and `bit` modules (#4989)
  chore(ci): run clippy on benchmarks (#4988)
  feat: remove query to backend to get expression width (#4975)
  fix: set index and value to 0 for array_get with predicate (#4971)
  fix: Compute the correct slice length when coercing from a literal array of complex types (#4986)
  feat: add `Neg` trait to stdlib (#4983)
  feat: implement `From` array trait for `BoundedVec` (#4927)
  chore: Release Noir(0.29.0) (#4905)
  fix: Move remove_if_else pass after second inlining  (#4976)
  feat: Optimize array sets in if conditions (alternate version) (#4716)
  chore: rename instruction checks for side effects (#4945)
  chore: Switch Noir JS to use execute program instead of circuit (#4965)
  fix: Use annotated type when checking declaration (#4966)
  feat: handle empty response foreign calls without an external resolver (#4959)
  feat: Complex outputs from acir call (#4952)
  fix: Require for all foldable functions to use distinct return  (#4949)
  feat!: use `distinct` return value witnesses by default (#4951)
  chore(docs): adding matomo tracking (#4898)
  ...
github-merge-queue bot pushed a commit that referenced this pull request May 21, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>0.30.0</summary>

## [0.30.0](v0.29.0...v0.30.0)
(2024-05-20)


### ⚠ BREAKING CHANGES

* remove `Opcode::Brillig` from ACIR
(AztecProtocol/aztec-packages#5995)
* AES blackbox
(AztecProtocol/aztec-packages#6016)

### Features

* `multi_scalar_mul` blackbox func
(AztecProtocol/aztec-packages#6097)
([73a635e](73a635e))
* `variable_base_scalar_mul` blackbox func
(AztecProtocol/aztec-packages#6039)
([73a635e](73a635e))
* Add `Not` trait to stdlib
([#4999](#4999))
([95d4d13](95d4d13))
* Add `std::ops::Neg` trait to stdlib
([07930d4](07930d4))
* Add native rust implementations of pedersen functions
([#4871](#4871))
([fb039f7](fb039f7))
* Add support for u16/i16
([#4985](#4985))
([e43661d](e43661d))
* AES blackbox
(AztecProtocol/aztec-packages#6016)
([73a635e](73a635e))
* Do not return databus returndata, keep it private.
([#5023](#5023))
([a5b7df1](a5b7df1))
* Dynamic assertion payloads v2
(AztecProtocol/aztec-packages#5949)
([73a635e](73a635e))
* Implement `From` array trait for `BoundedVec`
([#4927](#4927))
([bf491dc](bf491dc))
* Implement `ops` traits on `u16`/`i16`
([#4996](#4996))
([8b65663](8b65663))
* Implement `std::ops::Sub` on `EmbeddedCurvePoint`
([07930d4](07930d4))
* Increase default expression width to 4
([#4995](#4995))
([f01d309](f01d309))
* Parsing non-string assertion payloads in noir js
(AztecProtocol/aztec-packages#6079)
([73a635e](73a635e))
* Remove query to backend to get expression width
([#4975](#4975))
([e5f356b](e5f356b))
* Set aztec private functions to be recursive
(AztecProtocol/aztec-packages#6192)
([73a635e](73a635e))


### Bug Fixes

* Compute the correct slice length when coercing from a literal array of
complex types ([#4986](#4986))
([f3f1150](f3f1150))
* Defer overflow checks for unsigned integers to acir-gen
([#4832](#4832))
([b577761](b577761))
* Fix no predicates for brillig with intermediate functions
([#5015](#5015))
([9c6de4b](9c6de4b))
* Fixed several vulnerabilities in U128, added some tests
([#5024](#5024))
([e5ab24d](e5ab24d))
* Ignore no_predicates in brillig functions
([#5012](#5012))
([b541e79](b541e79))
* Set index and value to 0 for array_get with predicate
([#4971](#4971))
([c49d3a9](c49d3a9))


### Miscellaneous Chores

* Remove `Opcode::Brillig` from ACIR
(AztecProtocol/aztec-packages#5995)
([73a635e](73a635e))
</details>

<details><summary>0.46.0</summary>

## [0.46.0](v0.45.0...v0.46.0)
(2024-05-20)


### ⚠ BREAKING CHANGES

* remove `Opcode::Brillig` from ACIR
(AztecProtocol/aztec-packages#5995)
* AES blackbox
(AztecProtocol/aztec-packages#6016)
* Bit shift is restricted to u8 right operand
([#4907](#4907))
* contract interfaces and better function calls
(AztecProtocol/aztec-packages#5687)
* change backend width to 4
(AztecProtocol/aztec-packages#5374)
* Use fixed size arrays in black box functions where sizes are known
(AztecProtocol/aztec-packages#5620)
* trap with revert data
(AztecProtocol/aztec-packages#5732)
* **acir:** BrilligCall opcode
(AztecProtocol/aztec-packages#5709)
* remove fixed-length keccak256
(AztecProtocol/aztec-packages#5617)
* storage_layout and `#[aztec(storage)]`
(AztecProtocol/aztec-packages#5387)
* **acir:** Add predicate to call opcode
(AztecProtocol/aztec-packages#5616)
* contract_abi-exports
(AztecProtocol/aztec-packages#5386)
* Brillig typed memory
(AztecProtocol/aztec-packages#5395)
* **acir:** Program and witness stack structure
(AztecProtocol/aztec-packages#5149)
* automatic NoteInterface and NoteGetterOptions auto select
(AztecProtocol/aztec-packages#4508)
* Acir call opcode
(AztecProtocol/aztec-packages#4773)
* Support contracts with no constructor
(AztecProtocol/aztec-packages#5175)
* Internal as a macro
(AztecProtocol/aztec-packages#4898)
* move noir out of yarn-project
(AztecProtocol/aztec-packages#4479)
* note type ids
(AztecProtocol/aztec-packages#4500)
* rename bigint_neg into bigint_sub
(AztecProtocol/aztec-packages#4420)
* Add expression width into acir
(AztecProtocol/aztec-packages#4014)
* init storage macro
(AztecProtocol/aztec-packages#4200)
* **acir:** Move `is_recursive` flag to be part of the circuit
definition (AztecProtocol/aztec-packages#4221)
* Sync commits from `aztec-packages`
([#4144](#4144))

### Features

* `multi_scalar_mul` blackbox func
(AztecProtocol/aztec-packages#6097)
([73a635e](73a635e))
* `variable_base_scalar_mul` blackbox func
(AztecProtocol/aztec-packages#6039)
([73a635e](73a635e))
* Acir call opcode
(AztecProtocol/aztec-packages#4773)
([c3c9e19](c3c9e19))
* **acir_gen:** Brillig stdlib
([#4848](#4848))
([0c8175c](0c8175c))
* **acir_gen:** Fold attribute at compile-time and initial non inlined
ACIR (AztecProtocol/aztec-packages#5341)
([a0f7474](a0f7474))
* **acir:** Add predicate to call opcode
(AztecProtocol/aztec-packages#5616)
([2bd006a](2bd006a))
* **acir:** BrilligCall opcode
(AztecProtocol/aztec-packages#5709)
([0f9ae0a](0f9ae0a))
* **acir:** Program and witness stack structure
(AztecProtocol/aztec-packages#5149)
([13eb71b](13eb71b))
* **acvm_js:** Execute program
([#4694](#4694))
([386f6d0](386f6d0))
* **acvm:** Execute multiple circuits
(AztecProtocol/aztec-packages#5380)
([a0f7474](a0f7474))
* Add bit size to const opcode
(AztecProtocol/aztec-packages#4385)
([158c8ce](158c8ce))
* Add CMOV instruction to brillig and brillig gen
(AztecProtocol/aztec-packages#5308)
([13eb71b](13eb71b))
* Add expression width into acir
(AztecProtocol/aztec-packages#4014)
([158c8ce](158c8ce))
* Add instrumentation for tracking variables in debugging
([#4122](#4122))
([c58d691](c58d691))
* Add native rust implementations of pedersen functions
([#4871](#4871))
([fb039f7](fb039f7))
* Add poseidon2 opcode implementation for acvm/brillig, and Noir
([#4398](#4398))
([10e8292](10e8292))
* Add return values to aztec fns
(AztecProtocol/aztec-packages#5389)
([2bd006a](2bd006a))
* Add support for overriding expression width
([#4117](#4117))
([c8026d5](c8026d5))
* Added cast opcode and cast calldata
(AztecProtocol/aztec-packages#4423)
([78ef013](78ef013))
* AES blackbox
(AztecProtocol/aztec-packages#6016)
([73a635e](73a635e))
* Allow brillig to read arrays directly from memory
(AztecProtocol/aztec-packages#4460)
([158c8ce](158c8ce))
* Allow nested arrays and vectors in Brillig foreign calls
(AztecProtocol/aztec-packages#4478)
([158c8ce](158c8ce))
* Allow variables and stack trace inspection in the debugger
([#4184](#4184))
([bf263fc](bf263fc))
* Automatic NoteInterface and NoteGetterOptions auto select
(AztecProtocol/aztec-packages#4508)
([13eb71b](13eb71b))
* **avm:** Back in avm context with macro - refactor context
(AztecProtocol/aztec-packages#4438)
([158c8ce](158c8ce))
* **avm:** Brillig CONST of size &gt; u128
(AztecProtocol/aztec-packages#5217)
([c3c9e19](c3c9e19))
* **avm:** Integrate AVM with initializers
(AztecProtocol/aztec-packages#5469)
([2bd006a](2bd006a))
* **aztec-nr:** Initial work for aztec public vm macro
(AztecProtocol/aztec-packages#4400)
([158c8ce](158c8ce))
* Backpropagate constants in ACIR during optimization
([#3926](#3926))
([aad0da0](aad0da0))
* Bit shift is restricted to u8 right operand
([#4907](#4907))
([c4b0369](c4b0369))
* Brillig heterogeneous memory cells
(AztecProtocol/aztec-packages#5608)
([305bcdc](305bcdc))
* Brillig IR refactor
(AztecProtocol/aztec-packages#5233)
([c3c9e19](c3c9e19))
* Brillig pointer codegen and execution
(AztecProtocol/aztec-packages#5737)
([0f9ae0a](0f9ae0a))
* Brillig typed memory
(AztecProtocol/aztec-packages#5395)
([0bc18c4](0bc18c4))
* Change backend width to 4
(AztecProtocol/aztec-packages#5374)
([0f9ae0a](0f9ae0a))
* Check initializer msg.sender matches deployer from address preimage
(AztecProtocol/aztec-packages#5222)
([c3c9e19](c3c9e19))
* Contract interfaces and better function calls
(AztecProtocol/aztec-packages#5687)
([0f9ae0a](0f9ae0a))
* Contract_abi-exports
(AztecProtocol/aztec-packages#5386)
([2bd006a](2bd006a))
* Dynamic assertion payloads v2
(AztecProtocol/aztec-packages#5949)
([73a635e](73a635e))
* Evaluation of dynamic assert messages
([#4101](#4101))
([c284e01](c284e01))
* Handle `BrilligCall` opcodes in the debugger
([#4897](#4897))
([b380dc4](b380dc4))
* Impl of missing functionality in new key store
(AztecProtocol/aztec-packages#5750)
([0f9ae0a](0f9ae0a))
* Increase default expression width to 4
([#4995](#4995))
([f01d309](f01d309))
* Init storage macro
(AztecProtocol/aztec-packages#4200)
([158c8ce](158c8ce))
* Initial Earthly CI
(AztecProtocol/aztec-packages#5069)
([c3c9e19](c3c9e19))
* Internal as a macro
(AztecProtocol/aztec-packages#4898)
([5f57ebb](5f57ebb))
* **nargo:** Handle call stacks for multiple Acir calls
([#4711](#4711))
([5b23171](5b23171))
* New brillig field operations and refactor of binary operations
(AztecProtocol/aztec-packages#5208)
([c3c9e19](c3c9e19))
* Note type ids
(AztecProtocol/aztec-packages#4500)
([78ef013](78ef013))
* Parsing non-string assertion payloads in noir js
(AztecProtocol/aztec-packages#6079)
([73a635e](73a635e))
* Remove replacement of boolean range opcodes with `AssertZero` opcodes
([#4107](#4107))
([dac0e87](dac0e87))
* Restore hashing args via slice for performance
(AztecProtocol/aztec-packages#5539)
([2bd006a](2bd006a))
* Set aztec private functions to be recursive
(AztecProtocol/aztec-packages#6192)
([73a635e](73a635e))
* Signed integer division and modulus in brillig gen
(AztecProtocol/aztec-packages#5279)
([c3c9e19](c3c9e19))
* **simulator:** Fetch return values at circuit execution
(AztecProtocol/aztec-packages#5642)
([305bcdc](305bcdc))
* Storage_layout and `#[aztec(storage)]`
(AztecProtocol/aztec-packages#5387)
([2bd006a](2bd006a))
* Support contracts with no constructor
(AztecProtocol/aztec-packages#5175)
([c3c9e19](c3c9e19))
* Sync `aztec-packages`
([#4011](#4011))
([fee2452](fee2452))
* Sync commits from `aztec-packages`
([#4068](#4068))
([7a8f3a3](7a8f3a3))
* Sync commits from `aztec-packages`
([#4144](#4144))
([0205d3b](0205d3b))
* Sync from aztec-packages
([#4483](#4483))
([fe8f277](fe8f277))
* Sync from noir
(AztecProtocol/aztec-packages#5234)
([c3c9e19](c3c9e19))
* Sync from noir
(AztecProtocol/aztec-packages#5286)
([c3c9e19](c3c9e19))
* Sync from noir
(AztecProtocol/aztec-packages#5572)
([2bd006a](2bd006a))
* Sync from noir
(AztecProtocol/aztec-packages#5619)
([2bd006a](2bd006a))
* Sync from noir
(AztecProtocol/aztec-packages#5697)
([305bcdc](305bcdc))
* Sync from noir
(AztecProtocol/aztec-packages#5794)
([0f9ae0a](0f9ae0a))
* Sync from noir
(AztecProtocol/aztec-packages#5814)
([0f9ae0a](0f9ae0a))
* Sync from noir
(AztecProtocol/aztec-packages#5935)
([1b867b1](1b867b1))
* Sync from noir
(AztecProtocol/aztec-packages#5955)
([1b867b1](1b867b1))
* Sync from noir
(AztecProtocol/aztec-packages#5999)
([1b867b1](1b867b1))
* Trap with revert data
(AztecProtocol/aztec-packages#5732)
([0f9ae0a](0f9ae0a))
* Use fixed size arrays in black box functions where sizes are known
(AztecProtocol/aztec-packages#5620)
([0f9ae0a](0f9ae0a))
* Variable length returns
(AztecProtocol/aztec-packages#5633)
([305bcdc](305bcdc))


### Bug Fixes

* **acvm:** Mark outputs of Opcode::Call solvable
([#4708](#4708))
([8fea405](8fea405))
* Avoid huge unrolling in hash_args
(AztecProtocol/aztec-packages#5703)
([305bcdc](305bcdc))
* Catch panics from EC point creation (e.g. the point is at infinity)
([#4790](#4790))
([645dba1](645dba1))
* Don't reuse brillig with slice arguments
(AztecProtocol/aztec-packages#5800)
([0f9ae0a](0f9ae0a))
* Issue 4682 and add solver for unconstrained bigintegers
([#4729](#4729))
([e4d33c1](e4d33c1))
* Noir test incorrect reporting
(AztecProtocol/aztec-packages#4925)
([5f57ebb](5f57ebb))
* Proper field inversion for bigints
([#4802](#4802))
([b46d0e3](b46d0e3))
* Remove panic from `init_log_level` in `acvm_js`
([#4195](#4195))
([2e26530](2e26530))


### Miscellaneous Chores

* **acir:** Move `is_recursive` flag to be part of the circuit
definition (AztecProtocol/aztec-packages#4221)
([158c8ce](158c8ce))
* Move noir out of yarn-project
(AztecProtocol/aztec-packages#4479)
([78ef013](78ef013))
* Remove `Opcode::Brillig` from ACIR
(AztecProtocol/aztec-packages#5995)
([73a635e](73a635e))
* Remove fixed-length keccak256
(AztecProtocol/aztec-packages#5617)
([305bcdc](305bcdc))
* Rename bigint_neg into bigint_sub
(AztecProtocol/aztec-packages#4420)
([158c8ce](158c8ce))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

4 participants