feat(avm)!: Remove is_infinite flag from AVM ECC & update noir submodule with serialization changes#23342
Conversation
### Update Will also close [Foundation AVM Issue 13](https://linear.app/aztec-foundation/issue/AVM-13/enforce-canonical-point-at-infinity-representation) Note that this is currently being used as a base for removing the flag from AVM's point representation ([Foundation AVM Issue 14](https://linear.app/aztec-foundation/issue/AVM-14/remove-is-inf-flag-from-avm-ec-point-representation)): - ~#22745 (in merge train) - #23342 (ACIR changes from #23155 for removing `inf`) - #22564 <-- here - #22921 ✅ - #22795 ✅ - #22945 ✅ - #23031 ✅ **Everything in this branch has been reviewed**, see above PRs for individual work ⬆️ ### [OLD] Overview Will close [AVM-248](https://linear.app/aztec-labs/issue/AVM-248/enforce-canonical-point-at-infinity-representation) As a kind of stopgap before removing the `is_infinite` flag completely from the AVM ([AVM-266](https://linear.app/aztec-labs/issue/AVM-266/remove-is-inf-flag-from-ec-point-representation)), we now follow Noir behaviour more closely by deriving `is_inf` from coordinates inside the circuits ( `(x, y) == (0, 0) ? is_inf == true`). This replaces previous logic remapping points to (0, 0) from `is_inf`. This method relies on the on curve check (for `(0, 0) ==> is_inf `) and some new relations enforcing coordinates (for `is_inf ==> (0, 0)`) rather than (more expensive) error handling. However this does mean that the former will fail with an on curve error whereas the latter will simply fail a relation.
| * BB, however, uses only the two coordinates to represent points. Infinity in barretenberg is represented as | ||
| * (P+1)/2,0,true. This class is a wrapper of the BB representation, needed to operate with points, that allows us to |
There was a problem hiding this comment.
Infinity in barretenberg is represented as (P+1)/2,0,true
Looks to be outdated. Or does barretenberg enforce (0,0) on the boundary and uses ((P+1)/2,0) internally?
There was a problem hiding this comment.
Ah it used to be true at the time of writing - then later I think was updated to (0,0) on the boundary but setting x as the bit max when serialising ( /ecc/groups/affine_element.hpp i.e. as a buffer with all slots = 1) or the modulus (self_set_infinity). I see some changes from bb came to next just a couple of days ago so will check what it is now!
There was a problem hiding this comment.
i think this is correct, the montgomery representation in bb is that. (0,0) only applies to the circuit representation that bb uses not the native representation
ECAdd no longer takes is_infinite operands. Update the transpiler, inline ECADD handler, and MSM procedure to use the new (x, y) point layout with (0, 0) as the point at infinity.
Regenerate acir.hpp from noir's autogen and propagate the resulting shape changes through the dsl: - ECAdd / MSM no longer carry is_infinite; (0, 0) is now the point at infinity throughout EcAdd, MultiScalarMul, and to_grumpkin_point. - MemOp switches from three Expressions (operation, index, value) to (read: bool, index: Witness, value: Witness). - Circuit drops current_witness_index. Tests that exercised the externally-supplied infinity flag have been dropped since the flag no longer exists; (0,0)-roundtrip cases are kept.
Acir::MemOp no longer accepts Expression-shaped index/value after the serialization change; mem_op_to_acir_mem_op now asserts both are witnesses. The ROM/RAM/CallData test configs that exercised constant ops (perform_constant_ops = true) round-trip through that helper and trip the assertion, so drop those configs together with the unused add_constant_ops helper and template flag.
EmbeddedCurveAdd dropped is_infinite in 4992cb1, so each MSM point is now two field elements and the output is (x, y). The two size assertions were missed in that change; update them to match.
EmbeddedCurveAdd/MSM dropped is_infinite from points (4992cb1). The constraint now uses 6 fewer gates for MSM and 8 fewer for EcAdd, so update the opcode-gate-count expectations to match.
…#22921) This branch solely contains the changes needed to remove the `is_infinite` flag from our `StandardAffinePoint` C++ wrapper. Now, we check whether `x` and `y` are zero to assign an `inf` underlying point. Will close [Foundation AVM Issue 17](https://linear.app/aztec-foundation/issue/AVM-17/remove-is-inf-flag-from-avms-standardaffinepoint) --- Stack: - #22745 - #22564 - `mw/avm-rem-inf-point-wrapper` <-- here - #22795 - #22945 - #23031
…y) (#22795) This branch includes the changes to remove the `is_infinite` flag from our point representation and conceptually treating a point as infinite iff its coordinates are `(0, 0)`. It only contains logic changes within the AVM for the above and does not touch the opcode - this is in a lower PR - so the **CI will probably fail**. Will close [Foundation AVM Issue 18](https://linear.app/aztec-foundation/issue/AVM-18/remove-is-inf-flag-from-resulting-ec-points-in-avm-circuits) --- Stack: - #22745 - #22564 - #22921 - `mw/avm-explore-remove-is-inf` <-- here - #22945 - #23031
…#22945) This branch includes the changes to remove the `is_infinite` flags from the ECADD opcode fn signature. The actual EC logic changes come above this PR in the stack, and any changes outside the AVM will be below. For ease of review, I've separated into commits: - **feat: remove inf flags from ecadd opcode - ec flow only** Isolated to the EC flow only (does not change registers so non avm tests will fail) - f**eat: rem infs from fuzzer (only gadget fuzzer tested)** Isolated fixes to get the fuzzer(s) compiling Will partially close [Foundation AVM Issue 19](https://linear.app/aztec-foundation/issue/AVM-19/) (the following PR Note that the opcode mismatches that in ts so **CI will fail** until --- Stack: - #22745 - #22564 - #22921 - #22795 - `mw/avm-rem-inf-opcode-ecadd` <-- here - #23031
… AVM only) (#23031) This branch includes the changes to remove the `is_infinite` flags from the ECADD opcode fn signature which reside outside `vm2`. This includes the transpiler, ts simulator, and anything required in ACIR. Note that ACIR and noir's black box still use [the flags](https://github.com/AztecProtocol/aztec-packages/blob/b30fe8f401d7af45148071924b22b3f377750eaf/barretenberg/cpp/src/barretenberg/dsl/acir_format/ec_operations.hpp#L34) and represent points by a[ triple of elements.](https://github.com/noir-lang/noir/blob/bc4a37e2994ebc7d44ae98be81e18606b2231c61/acvm-repo/bn254_blackbox_solver/src/embedded_curve_ops.rs#L98) Since this touches both private and public execution, I think it's out of scope of this task to update these. Will partially close [Foundation AVM Issue 19](https://linear.app/aztec-foundation/issue/AVM-19/) (the previous PR with AVM changes will close the initial portion) --- Stack: - #22745 - #22564 - #22921 - #22795 - #22945 - `mw/avm-rem-inf-opcode-ecadd-ext` <-- here
e4dbb5a to
851f98a
Compare
Flakey Tests🤖 says: This CI run detected 1 tests that failed, but were tolerated due to a .test_patterns.yml entry. |
Summary
This branch brings together two PRs:
chore!: remove is_infinite from EmbeddedCurvePointis_infiniteflag from point coordinates #22564: Top of large stack which removesis_infiniteflag from AVM opcode and point structPR 1 requires AVM changes in 2 to run (since it removes the
infflag from ACIR). For more details on specific changes see the relevant PRs above.Note that 1 is applied first so this PR will initially be identical to it, apart from any changes needed from targeting the AVM merge train.
Will close:
(0, 0)is_infiniteflagThese issues form the bulk of the project: Align Circuit Point Infinity Representation with Noir + BB