Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional well-formedness check for G2 elements #1938

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

ebfull
Copy link
Contributor

@ebfull ebfull commented Dec 13, 2016

libsnark currently checks that G1 and G2 elements are well-formed by ensuring that they satisfy their respective curve equations, and although this is enough for G1 (which is instantiated as an order r curve E/Fp: y^2 = x^3 + b), G2 is the order r subgroup of the composite order r(2q-r) curve E'/Fp2: y^2 = x^3 + b/e constructed via a sextic twisting isomorphism. This means we need to ensure these points are order r as well.

None of the proofs on the Zcash blockchain violate this check, and it may not even be possible for them to violate this check (bilinearity is not preserved). Let's be cautious and do it anyway.

@ebfull ebfull requested a review from str4d December 13, 2016 02:24
@str4d
Copy link
Contributor

str4d commented Dec 13, 2016

ut(ACK+cov). I have not checked the libsnark parameters satisfy the intended definitions.

@arielgabizon
Copy link
Contributor

Looks good!
We should just check that r does not divide (2q-r),
as this means its indeed enough to check the point has order r (and is on the curve E'),
according to page 51 here http://www.craigcostello.com.au/pairings/PairingsForBeginners.pdf

@ebfull
Copy link
Contributor Author

ebfull commented Dec 13, 2016

>>> q = 21888242871839275222246405745257275088696311157297823662689037894645226208583
>>> r = 21888242871839275222246405745257275088548364400416034343698204186575808495617
>>> (2 * q - r) % r
295893513763578637981667416138835425932

@ebfull ebfull added this to the 1.0.4 milestone Dec 13, 2016
Copy link
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ut(ACK+cov) (for benefit of the new review system)

@str4d
Copy link
Contributor

str4d commented Dec 13, 2016

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Dec 13, 2016

📌 Commit c4fce3f has been approved by str4d

@zkbot
Copy link
Contributor

zkbot commented Dec 13, 2016

⌛ Testing commit c4fce3f with merge 152c0eb...

zkbot pushed a commit that referenced this pull request Dec 13, 2016
Additional well-formedness check for G2 elements

libsnark currently checks that G<sub>1</sub> and G<sub>2</sub> elements are well-formed by ensuring that they satisfy their respective curve equations, and although this is enough for G<sub>1</sub> (which is instantiated as an order r curve E/F<sub>p</sub>: y^2 = x^3 + b), G<sub>2</sub> is the order r *subgroup* of the composite order r(2q-r) curve E'/Fp<sup>2</sup>: y^2 = x^3 + b/e constructed via a sextic twisting isomorphism. This means we need to ensure these points are order r as well.

None of the proofs on the Zcash blockchain violate this check, and it may not even be possible for them to violate this check (bilinearity is not preserved). Let's be cautious and do it anyway.
@zkbot
Copy link
Contributor

zkbot commented Dec 13, 2016

☀️ Test successful - zcash

@zkbot zkbot merged commit c4fce3f into zcash:master Dec 13, 2016
if (alt_bn128_modulus_r * r != curve_G2::zero()) {
throw std::runtime_error("point is not in G2");
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should is_well_formed include this check, rather than making it separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, though when/if libsnark changes those semantics we'll need to make sure we go through our codebase and understand the implications and assumptions broken by it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is now included in the protocol spec (2016.0-beta-1.11): zcash/zips@c791075

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except that I described it incorrectly in the spec. The check itself is correct. Will fix the spec.

@daira
Copy link
Contributor

daira commented Dec 14, 2016

ut(ACK+cov)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants