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 3 commits intomw/avm-ecc-fuzzer-fixfrom
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!
|
|
||
| // TODO(#AVM-266): Remove p_is_inf, q_is_inf entirely. | ||
| // Needs to be committed columns as they are used in the lookups | ||
| pol commit p_is_inf, q_is_inf; // constrained to be @boolean in the ecc subtrace (see #[INPUT_OUTPUT_ECC_ADD]) |
There was a problem hiding this comment.
can't we re-use the ones above?
There was a problem hiding this comment.
See below comment - I just separated the read from memory value from the derived in-circuit value to keep it sanitisied (we probably could re-use them but when I wrote this I thought of a possibility they may differ, which I can't recreate now...)
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
2 participants
WIP
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.