feat(aztec-nr)!: Remove nr Point struct in favour of EmbeddedCurvePoint#23392
Conversation
IlyasRidhuan
left a comment
IlyasRidhuan
left a comment
There was a problem hiding this comment.
The conflicts after merge-train will be pain but the changes largely look good
MirandaWood
force-pushed
the
mw/rem-nr-point-struct
branch
from
6a09e47 to
b4c8b37
Compare
MirandaWood
changed the title
MirandaWood
requested review from
Thunkar and
benesjan
and removed request for
LeilaWang and
jeanmon
MirandaWood
added
ci-full
MirandaWood
force-pushed
the
mw/rem-nr-point-struct
branch
from
7819615 to
db7e934
Compare
| // 4 prefix fields (salt, class_id, init_hash, immutables_hash) + 6 public-key fields + 1 | ||
| // universal_deploy flag = 11. | ||
| let serialized_params: [Field; 11] = [salt, contract_class_id.to_field(), initialization_hash, immutables_hash] | ||
| // 4 prefix fields (salt, class_id, init_hash, immutables_hash) + 5 public-key fields + 1 |
There was a problem hiding this comment.
Are we going from 6 to 5 because we drop is_infinite?
| // Note: BB encodes infinity as x == y == Buffer of all ones. | ||
| // Infinity should never pass through here, but for correctness: | ||
| const ALL_ONES = (1n << 256n) - 1n; |
There was a problem hiding this comment.
Why do we not use the same serialization? And if we don't allow passing infinity to bb, why do we allow receiving it from it? Wouldn't it be easier to to disallow it everywhere?
There was a problem hiding this comment.
I don't know why we don't use the same - the many reps of infinity has plagued us for a long time!
So yes ideally we would disallow it everywhere, but now we have no isInfinite flag that means we have two possibilities for infinities here:
- BB's version (
ALL_ONES) -> for now, I converted toinf, but can disallow - 'Empty' coordinates -> no flag means that this is identical to an
inf, but disallowing here would mean we couldn't receive any empty points, which I imagine would cause a lot of problems
So for now I didn't disallow for all cases just for consistency. Happy to do either though!
| if (data.x.toBigInt() === ALL_ONES && data.y.toBigInt() === ALL_ONES) { | ||
| return Point.ZERO; |
There was a problem hiding this comment.
This is quite confusing and error-prone - Point.ZERO is not a zero point, but actually the infinity point, which is disallowed in most places (e.g. as keys, in the bb boundary, etc.). People might do Point.ZERO thinking it's a simple way to get a no-op point and it's actually a trap.
There was a problem hiding this comment.
Fair enough - I can rename to INF? The difficulty is that now an 'empty' point class is equivalent to the point at infinity.
There was a problem hiding this comment.
Renamed to INFINITY here: #23507
(any comments I've marked as resolved were either already fixed here or are fixed in the PR above)
5 participants
Summary
Removes the
Pointstruct fromaztec-nrin favour of Noir's native EmbeddedCurvePoint, now that we use the two element point representation. Largelynrchanges but requires corresponding updates to ts.Will close AVM Linear issue 15.
Partially based on this previous commit. for the portions just removing the
nrstruct. AVM changes are in the above linked PR or in #23342.TODO/Outstanding
Update: Only small TODOs left over - see comments/code
Once the above public key changes are in (Linear issue AVM 22), most hacks/temp hashing solutions will be removedUse a re-export forpoint.nrto expose the noir classNote that the tsPointclass still contains anis_infinitebool (from the previous work I based this on), which means ts serialisation often includes this extra element. This can be very awkward with infinity points. If we allow emptyPointclass instances in ts with this bool, then(0, 0)can mean[0, 0, true]OR[0, 0, false].We could remove theis_infinitebool from the tsPointclass so all representations are two element? The only reason I didn't do this is because the bool was kept in the previous work above.Otherwise, we need to reworkEmpty()for classes such asKeyValidationRequest.Closes https://linear.app/aztec-labs/issue/F-553/remove-custom-point-struct-and-use-embeddedcurvepoint-directly