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

Add clear_cofactor. #18

Merged
merged 3 commits into from
Dec 10, 2019
Merged

Add clear_cofactor. #18

merged 3 commits into from
Dec 10, 2019

Conversation

mmaker
Copy link
Contributor

@mmaker mmaker commented Nov 17, 2019

In g1, simply multiply by the cofactor.
In g2, we use Budroni-Pintore.

@ebfull
Copy link
Contributor

ebfull commented Nov 22, 2019

Thanks!

As remarked in the recent BLS12-381 hashing paper, for G1 you can just multiply by 1 - x instead.

@ebfull
Copy link
Contributor

ebfull commented Nov 22, 2019

I think a "multiply by x" method should be made since I think you can build everything on top of that method (even the multiplication by x^2 - x - 1, using Horner's rule.) The "multiply by x" method can call the double method directly as needed, so you can avoid using Scalar entirely throughout this PR.

Copy link
Contributor

@ebfull ebfull left a comment

Choose a reason for hiding this comment

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

Here's a nice point in the Jacobian with nontrivial Z-coordinate. It has all torsion components.

x = 350898114783857325927094128797824245304332326695197644670817064152147218046382658891659391253935977554749163039309*u + 1942754808117291816143999833892557460183300072014507344900746720185664302954222967967024873214934378647278006268833

y = 3088704814004870030528824518395462854568730512980615222311538832067684268838353942640261277389143006135178816895355*u + 3899650597351899360349258896995920865499279578127773928753636067070462033619448741202620179159122308793599703155804

z = 2134816543916995720562686544050099617875819220457026417152635513766956164039125702682165220413578798936075993043249*u + 1106895829710730856658424812785614944981939716636035738306633804301486234264494172392187662811280865021721915588505

Let's use this in tests instead.

src/g1.rs Outdated Show resolved Hide resolved
src/g1.rs Outdated
#[test]
fn test_clear_cofactor() {
// the generator (and the identity) are always on the curve,
// even after clearning the cofactor
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: clearning -> clearing

Copy link
Member

Choose a reason for hiding this comment

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

The typo is still here.

src/g1.rs Outdated Show resolved Hide resolved
src/g1.rs Show resolved Hide resolved
src/g1.rs Outdated Show resolved Hide resolved
src/g1.rs Show resolved Hide resolved
src/g2.rs Show resolved Hide resolved
src/g2.rs Outdated Show resolved Hide resolved
src/g2.rs Show resolved Hide resolved
src/g2.rs Show resolved Hide resolved
src/g2.rs Show resolved Hide resolved
@mmaker mmaker force-pushed the master branch 2 times, most recently from 1e664d1 to fd382dc Compare November 24, 2019 12:29
@mmaker mmaker marked this pull request as ready for review November 24, 2019 12:47
@mmaker
Copy link
Contributor Author

mmaker commented Nov 24, 2019

Hey, thank you so much for the fast review!
I was actually waiting for the weekend to add more tests before marking it for "ready to review", but yes:

  • I added a couple tests with a nontrivial z;
  • you're right, certainly BP meant us to use multiplication by x the way you wrote it -- I ignored the fact that it has small hamming weight ^^";
  • I wanted to test for the relation psi2(P) - t *psi(P) - q * P = 0, but it's very difficult to multiply by q with the current API? In any case they seem sufficient for now.

@mmaker
Copy link
Contributor Author

mmaker commented Nov 24, 2019

Just for the record, on G2 I used the point you gave me. I converted it to the Montgomery representation via:

def to_fp_repr(x):               
  mont_hex = hex(x * Zp(2)^384)[2:].rjust(16*6, '0')                                                                                                                                                           
  print('0x' + ',\n0x'.join([mont_hex[i:i+16] for i in range(0, 16*6, 16)][::-1]) + ',\n')
  
def to_fp2_repr(x):
  real, img = x.polynomial()
  to_fp_repr(real)
  print()
  to_fp_repr(img)

while for G1 I took a random point and a random scalar via:

sage: Px, Py = E1.random_element().xy()
sage: Pz = Zp.random_element()
sage: to_fp_repr(Px*Pz^2)
sage: to_fp_repr(Py*Pz^3)
sage: to_fp_repr(Pz)

src/g2.rs Outdated Show resolved Hide resolved
src/g2.rs Show resolved Hide resolved
@mmaker mmaker force-pushed the master branch 2 times, most recently from 18cbf1c to 7dc6f31 Compare November 27, 2019 18:15
@ebfull
Copy link
Contributor

ebfull commented Nov 28, 2019

I'll open some issues about adding similar methods to G1Affine/G2Affine, especially since they'd be more efficient through mixed addition. Not necessary for this PR.

Copy link
Member

@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.

utACK. I did not verify the correctness of psi_coeff_x, psi_coeff_y, or psi2_coeff_x.

src/g1.rs Outdated
#[test]
fn test_clear_cofactor() {
// the generator (and the identity) are always on the curve,
// even after clearning the cofactor
Copy link
Member

Choose a reason for hiding this comment

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

The typo is still here.

@ebfull ebfull merged commit e32494e into zkcrypto:master Dec 10, 2019
@ebfull
Copy link
Contributor

ebfull commented Dec 10, 2019

Thank you @mmaker!

@mmaker
Copy link
Contributor Author

mmaker commented Dec 11, 2019

I'll open some issues about adding similar methods to G1Affine/G2Affine, especially since they'd be more efficient through mixed addition. Not necessary for this PR.

Hmm, could you please mention me once you do?

The typo is still here.

Ugh, sorry…

Thanks a lot for your kind help!

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.

3 participants