feat(avm)!: WIP Derive is_infinite flag from point coordinates#22564
feat(avm)!: WIP Derive is_infinite flag from point coordinates#22564MirandaWood wants to merge 6 commits into
is_infinite flag from point coordinates#22564Conversation
MirandaWood
force-pushed
the
mw/avm-derive-is-inf
branch
from
8f3f2d6 to
50d4a19
Compare
| {} | ||
|
|
||
| // TODO(MW): Do we want to silently discard input x, y if is_infinity = true? | ||
| // Or, discard is_infinity and set point to AffinePoint::infinity() iff x = y = 0? |
There was a problem hiding this comment.
if you're concerned about this distinction you can drop the bool is_infinity from the parameter list check for x==0 && y==0 before setting the coordinates.
Otherwise i dont think there is such a thing as "discarding is_infinity" as iirc the only call site where this matters is in execution.cpp and there is_infinity is derived from the coordinates.
There was a problem hiding this comment.
I was implementing more in the other direction i.e. we can input a bb-represented inf point and ensure this class handles it as an inf by relying on the input flag (see new test below). Happy to implement it the 'other way' as you suggest (since constructing via AffinePoint should still correctly treat bb infs as infs)!
| ecc_add_mem.p_x, | ||
| ecc_add_mem.p_y, | ||
| ecc_add_mem.p_is_inf, | ||
| ecc_add_mem.p_is_inf_, |
There was a problem hiding this comment.
can we drop this from the permutation?
There was a problem hiding this comment.
Yes, if safe:
TODO(MW): Is ignoring i.e. leaving a memory operand unconstrained safe? Execution still reads them but will remove p/q_is_inf_ if so.
I wasn't sure of the implications of leaving a memory slot there (since we're not yet removing the flag from the opcode) but unread/unconstrained. This is also why there's a separate is_inf and is_inf_ - the former is completely internal to the circuits and derived from coordinates while the latter is only connected to memory. If that sounds good, I'll just remove the latter!
MirandaWood
force-pushed
the
mw/avm-derive-is-inf
branch
from
1ede827 to
09131db
Compare
MirandaWood
changed the base branch from
merge-train/avm
to
mw/avm-ecc-fuzzer-fix
MirandaWood
force-pushed
the
mw/avm-ecc-fuzzer-fix
branch
from
a76c688 to
ed831c6
Compare
MirandaWood
force-pushed
the
mw/avm-derive-is-inf
branch
from
09131db to
b01d712
Compare
…#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 #23031 with ts/rs changes will fully close it). Note that the opcode mismatches that in ts so **CI will fail** until #23031 is merged into this branch! --- Stack: - #22745 - #22564 - #22921 - #22795 - `mw/avm-rem-inf-opcode-ecadd` <-- here - #23031
2 participants
Update
Will also close Foundation AVM Issue 13
Note that this is currently being used as a base for removing the flag from AVM's point representation (Foundation AVM Issue 14):
is_infiniteflag from point coordinates #22564 <-- hereis_infiniteflag from point wrapper constructor #22921 ✅is_infiniteflag from AVM (PIL and EC points only) #22795 ✅is_infiniteflags from ECADD opcode (outside AVM only) #23031Everything in this branch has been reviewed, see above PRs for individual work ⬆️
[OLD] Overview
Will close AVM-248
As a kind of stopgap before removing the
is_infiniteflag completely from the AVM (AVM-266), we now follow Noir behaviour more closely by derivingis_inffrom coordinates inside the circuits ((x, y) == (0, 0) ? is_inf == true). This replaces previous logic remapping points to (0, 0) fromis_inf.This method relies on the on curve check (for
(0, 0) ==> is_inf) and some new relations enforcing coordinates (foris_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.