Complete add for AffineGagdet + fix serialization to field elements#172
Complete add for AffineGagdet + fix serialization to field elements#172nicholas-mainardi wants to merge 5 commits intospongefrom
Conversation
95DDB
left a comment
95DDB
left a comment
There was a problem hiding this comment.
Fine for me. Let's just remember that changing the zero representation for the gadget might affect some dependent circuits (must check that)
| } | ||
| } | ||
|
|
||
| impl<F: PrimeField> TryInto<Boolean> for FpGadget<F> { |
There was a problem hiding this comment.
Let's write a comment saying explicitly that this is intended to be used when we are sure that the 'self' FpGadget cannot have other values than 0 or 1 (for instance, because enforced by other constraints); i.e. this function doesn't explicitly enforce 'self' being 0 or 1 with the usual Boolean constraint.
There was a problem hiding this comment.
I agree. It was not completely what this function should do.
There was a problem hiding this comment.
Ok, I will add a detailed comment
| } | ||
| } | ||
|
|
||
| impl<F: PrimeField> TryInto<Boolean> for FpGadget<F> { |
There was a problem hiding this comment.
I agree. It was not completely what this function should do.
|
|
||
| // compute lambda_diff = (y2-y1)/(x2-x1+is_same_x), the value for lambda to be used in case x1 != x2 | ||
| let y2_minus_y1 = other.y.sub(cs.ns(|| "y2 - y1"), &self.y)?; | ||
| let x2_minus_x1_plus_x_same = x2_minus_x1.add(cs.ns(|| "x2 - x1 + x_is_same"), &is_same_x)?; |
There was a problem hiding this comment.
It should be
let x2_minus_x1_plus_is_same_x = x2_minus_x1.add(cs.ns(|| "x2 - x1 + is_same_x").
| lambda_diff.mul_equals(cs.ns(|| "enforce lambda_diff = (y2-y1) div (x2-x1+is_same_x"), &x2_minus_x1_plus_x_same, &y2_minus_y1)?; | ||
|
|
||
| // compute lambda_same = (3x1^2+a)/(2y1), the value for lambda to be used in case x1 = x2. | ||
| // in case y1 = 0, which happens only when self is the identity, the constraint |
There was a problem hiding this comment.
It is not true for a general curve.
The points with y-coordinate equal to 0 are the zero point plus the points with order 2.
In the case of Tweedle (a=0, b not a cube), these points are not defined over the prime field. But, if you have a general curve (and the algorithm looks to be general since we do not assume a = 0) then the number of points can be a multiple of 2. In this last case you have points of order 2 and then non-trivial points with y = 0.
There was a problem hiding this comment.
Observation by @UlrichHaboeck75 This is not a problem, because the witness points are always chosen in a prime order subgroup of the elliptic curve.
Anyway it could be specified in the comment.
There was a problem hiding this comment.
Ok, I modify the comment to specify that in our case we consider only curves with prime order, and in such curves there cannot be other oints with y=0 besides the point at infinity
|
|
||
| // compute lambda_same = (3x1^2+a)/(2y1), the value for lambda to be used in case x1 = x2. | ||
| // in case y1 = 0, which happens only when self is the identity, the constraint | ||
| // 2y1*lambda_same = 3x1^2+a cannot be satisfied, unless a == 0. |
There was a problem hiding this comment.
What do you mean that it cannot be satisfied?
There was a problem hiding this comment.
I mean that when self is the identity, then x1=0 and y1=0, therefore there exists no value for lambda_same which would satisfy the constraint 2y1*lambda_same = 3x1^2+a, unless a=0. I will try to make the comment clearer.
| let x1_minus_x3 = self.x.sub(cs.ns(|| "x1-x3"), &x3)?; | ||
| let y3 = lambda.mul(cs.ns(|| "lambda*(x1-x3)"), &x1_minus_x3)?.sub(cs.ns(|| "y3=lambda*(x1-x3)-y1"), &self.y)?; | ||
|
|
||
| // compute a flag is_sum_zero which is true iff self+other == identity |
There was a problem hiding this comment.
The comment is not precise: two points are opposite if and only if the x-coordinate is the same and the y-coordinate is the opposite.
The piece of code below is correct.
There was a problem hiding this comment.
The issue of the comment does not seem clear to me, the comment just state that is_sum_zero = self + other == identity without delving into the relationship between the coordinates of the 2 points when this happens.
3 participants
This PR partially addresses issue #36 (partially #125 too), providing a first implementation for complete addition for the
AffineGadget, both for Jacobian and Projective curves. The algorithm is based on the zcash solution for complete arithmetic over weierstrass curves. Nonetheless, although the zcash solution works for weierstrass curves of prime order with a=0 (i.e., y^2 = x^3+b), the algorithm implemented in this PR should work for generic weierstrass curves of prime order (i.e., y^2 = x^3+ax+b), without increasing the number of constraints. Note that the algorithm requires to represent infinity point as (0,0) rather than (0,1), which was the conventional representation previously employed in theAffineGadget.Besides that, this PR addresses an inconsistency bug between the serialization to field elements of the infinity point between the
AffineGadgetand the underlying Jacobian/Projective curves. The serialization of the infinity point in such curves is changed to always serialize the affine point (0,0), which is never in the curve unless b=0 (which however is quite unlikely to be ever needed in our case as it means that the curve has non-prime order).