chore: makes ec addition opcode unsafe

[guipublic]

This PR is the same as #8374, but with some added runtime checks as suggested by @zac-williamson.
#8374 was merged and then rollback because it caused issues with e2e tests.
We should not merge this one until this issue is fixed, but I need the PR as ready for review in order to have the CI to run.

The PR makes the ACIR opcode 'EC ADD' unsafe because the safety checks are now done on the Noir side in the stdlib's embedded curve operations.
It will allow Aztec protocol circuit to use directly the opcode when the 'safety' is already known (and implied by previous operations).

EDIT: this unsafe EC ADD now handles points at infinity.
If they are constant, it works as expected
If not, it simply returns the infinity point.
This ensures that there is no performance impact (the checks are constant-time), but still support in a way the infinite points, i.e it does not crash and returns a valid point, although it is an incorrect result, when the infinite status cannot be resolved at constant time.

[github-actions]

Changes to circuit sizes

Generated at commit: 393fe779227f5114dd6044e3b1c5dda768b6f719, compared to commit: 1801f5b49fb3b153817a1596c6fd568f1c762fe5

🧾 Summary (100% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
private_kernel_reset_4_4_4_4_4_4_4_4_1	0 ➖	0.00%	-4 ✅	-0.01%
private_kernel_reset	0 ➖	0.00%	-64 ✅	-0.01%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
private_kernel_reset_4_4_4_4_4_4_4_4_1	34,897 (0)	0.00%	74,475 (-4)	-0.01%
private_kernel_reset	91,933 (0)	0.00%	467,666 (-64)	-0.01%

[TomAFrench]

Happy to merge once CI is passing.

[Rumata888]

What if the points are not on the curve? Where do we check that?

[guipublic]

It is the responsibility of the user, that's the point of this ec-add-unsafe; to avoid as many check as possible.
The checks done by the function have no cost because they are constant-time.
One use case is when you are adding points coming from a previous addition, so you know that the points are on the curve (if you checked the first ones) and there is no need to check this.

Note that the ec-add in Noir performs most of the checks

[Rumata888]

Right, makes sense

[Rumata888]

I just noticed that you don't detect the case where x_match by not y_match

[guipublic]

I added a check for this case:

• if x match and y does not match, at compile time, then return 0
• else, if x match, then return error
• else: we assume x-coordinates are distincts and will do the unconditional add

[Rumata888]

These two ifs are equivalent, but you create the point with 'use_g1' being set to different values

[guipublic]

Any valid point would do here, it is just for consistency, as I use G1 for point1 and G2=2G1 for point2 elsewhere in the file.
But I don't mind changing it.

[Rumata888]

Same here. Why is 1 true and 2 false?

[guipublic]

Here it matters, because the two points must be distinct (else it would be a doubling).

[Rumata888]

Why are you using unconditional add? Are you checking that the inputs are different? If the values are the same, this could allow you to create any point as a result

[guipublic]

This is the whole point of the PR, the checks have been moved on the Noir side so that we can avoid them in some cases. See this comment in the PR description:
"It will allow Aztec protocol circuit to use directly the opcode when the 'safety' is already known (and implied by previous operations)."

[Rumata888]

You shouldn't remove the difference!=zero check. It is always necessary. There is no case that it is not necessary, when you are performing non-doubling additions.

[guipublic]

I disagree, else the function unconditional_add() would not exist. See the comments of the function:

•    Only use this method if you know the x-coordinates of the operands cannot collide and none of the operands is a point at infinity
  

For instance it is used for batch mul:

•     If `unconditional_add = true`, we use `::unconditional_add` instead of `::checked_unconditional_add`. Use with caution! Only should be `true` if we're doing an ULTRA fixed-base MSM so we know the points cannot collide with the offset generators.
  

[Rumata888]

The function unconditional add is only used in 2 cases:

1. We know that the values provided are from a CRS and getting a combination of those that would be equal is tantamount to solving the discrete log
2. We use it in our own batch mul, when we specifically want to save 1 gate by batching non-zero checks.

However, these are extremely rare situations. It is extremely dangerous to leave the operation like this by default, because then it does not ensure the correctness of additions at all in 99% of cases. If you really want to allow the developer to disable this particular check, then there should be a setting that allows this (with loud disclaimers). It shouldn't be by default

[guipublic]

I will add a setting then.
In fact no, I will add the check because I don't want to have to change ACIR format.
I'll add the setting later, if this PR manage to go through!

[guipublic]

Done

[Rumata888]

Could you please add tests for all the edgecases? Right now we are only testing 1 case out of all the possibilities

[guipublic]

I am not sure because the edge cases are not expected to work.

[Rumata888]

I mean the various combinations of constant and witness elements, cases where it turns into a double, etc.

[guipublic]

Done

[guipublic]

I am closing this because this is now deprecated. Ec-Add is still safe in BB, and the code has been validated and audited, so we cannot change this for now.
This approach is still valid in theory and could be added in the future: having a 'fast' Ec-Add in BB and a validation in Noir, so that one can use the fast version and avoid Noir validation when it is safe to do.