py_ecc icon indicating copy to clipboard operation
py_ecc copied to clipboard

Generalize fields and curves

Open vbuterin opened this issue 7 years ago • 12 comments

Currently, there are separate folders and files for bn128 and bls-12-381, with ~90% duplicated code.

It should be possible to generalize this more, creating a generic prime field class much like there is a semi-generic FQP class, as well as generic elliptic curves, and then only have separate files for the two existing curve types to define curve parameters and twist formulas and the slightly different pairing functions.

vbuterin avatar Nov 28 '18 15:11 vbuterin

@vbuterin I am interested in generalizing and making the code flexible, could you please tell me the source of this whole codebase, such as a research paper or something. It would help me better in the understanding.

Bhargavasomu avatar Nov 28 '18 18:11 Bhargavasomu

Elliptic curve pairings in general:

https://medium.com/@VitalikButerin/exploring-elliptic-curve-pairings-c73c1864e627

BN128:

https://github.com/ethereum/EIPs/issues/197

BLS-12-381:

https://z.cash/blog/new-snark-curve/
https://github.com/zkcrypto/pairing/tree/master/src/bls12_381

vbuterin avatar Nov 28 '18 20:11 vbuterin

The following things come to my mind.

  • The classes FQ, FQP, FQ2 and FQ12 need not be reinitialized every time as they are not dependent on the type of curve or extension. So probably we could have these created in field_elements.py and we could use them everywhere (bn128, optimized_bn128, bls, optimized_bls).
  • We could also have a general class BaseCurve, and then maybe every curve (bn128_curve, optimized_bn128_curve, ...) could inherit this and make the changes specific to the inherited class.
  • We should probably move the constants into a seperate file (constants.py)
  • We should also remove the assert statements which are not part of any function, but are part of the script in general, as shown https://github.com/ethereum/py_ecc/blob/067a40261ad39526f6aa8bd69def7d6982993d13/py_ecc/bn128/bn128_pairing.py#L74-L83
  • Also the type hinting should be further generalized wherever possible (in terms of removing redundant types; like Optimized_FQPoint2D could be replaced by FQPoint2D). Similary the type hinting should be carried out for the bs12_381 and optimized_bs12_381 submodules.

Also the only difference I see in all the curves is

  • Difference in the constants such as b, b2, b12, G2, G12 ...
  • Difference in the twist function

@vbuterin are my facts right or am I missing anything. @pipermerriam is the above design ok?

Bhargavasomu avatar Nov 29 '18 04:11 Bhargavasomu

Design seems :+1:

Removing all of the runtime assertions would be nice, but lets make sure that they are all still executed as part of the test suite.

pipermerriam avatar Dec 04 '18 22:12 pipermerriam

@vbuterin in the twist function of bn128 and optimized_bn128, we are multiplying x by w^2 and y by w^3, whereas we should actually be dividing right? I think this is a bug (please correct me if I am wrong). Could you please follow up on this? (We are doing correctly by dividing in the case of bls_12_381 curve). Attaching the snippet for reference. https://github.com/ethereum/py_ecc/blob/212b19316c27b82774a4d414f2e7769d90fb67a3/py_ecc/bn128/bn128_curve.py#L139-L140

Bhargavasomu avatar Dec 09 '18 03:12 Bhargavasomu

@vbuterin in the twist function of bn128 and optimized_bn128, we are multiplying x by w^2 and y by w^3, whereas we should actually be dividing right? I think this is a bug (please correct me if I am wrong). Could you please follow up on this

Yeah, that seems like a mistake....

Here's the math: if in G2, y**2 - x**3 == b2 == b * w**6. In G12, b12 = b, and to achieve that we divide y by w**3 and x by x**2, so we get (y/w**3)**2 - (x/w**2)**3 = y**2 / w**6 - x**3 / w**6 = (y**2 - x**3) / w**6 = b * w**6 / w**6 = b. So division is correct.

vbuterin avatar Dec 10 '18 14:12 vbuterin

bls, optimized_bls

I'd say keep the names as bls12_381. "BLS" by itself seems like it would refer to Boneh-Lynn-Shacham signatures, which can be done over any pairing-equipped elliptic curve (yeah, I know that's confusing... did you know that BLS as in BLS-12-381 stands for something completely different, "Barreto-Lynn-Scott"?)

vbuterin avatar Dec 10 '18 14:12 vbuterin

@Bhargavasomu if you've got any reference materials for the math of these functions please feel free to toss links to them in the README.

pipermerriam avatar Dec 10 '18 19:12 pipermerriam

@vbuterin we have another issue with the constants(b, b2, b12, G1, G2) we are using in bn128 and optimized_bn128. This is because I tried to replace multiplication by division in the twist function of both the curves, and I ended up with the following error.

screenshot from 2018-12-11 11-54-52

This error occurs because after changing the twist function, the point G12 (which is generated by using the twist function) is no more on the bn128 curve characterized by b12 and hence the assertion fails. Attaching the snippet for reference. https://github.com/ethereum/py_ecc/blob/b0148825721835f2a41e412465ebb8ce145bcca9/py_ecc/bn128/bn128_curve.py#L143-L145

To fix this, I believe we need to adjust the values of our constants. I don't know how to fix them and hence need your help on this. Thanks.

EDIT : By constants, we might also need to review the correctness of ate_loop_count, log_ate_loop_count, pseudo_binary_encoding

Bhargavasomu avatar Dec 11 '18 06:12 Bhargavasomu

Also piggybacking in the same thread, the following tests have been failing after I have changed the multiplication to division, on both bn128 and optimized_bn128.

  • test_G12_object
  • test_pairing_bilinearity_on_G1
  • test_pairing_bilinearity_on_G2
  • test_pairing_composit_check I don't know how to fix these either and would greatly appreciate help.

Bhargavasomu avatar Dec 11 '18 13:12 Bhargavasomu

@vbuterin, my apologies for pinging you multiple times. But I would greatly appreciate your help regarding how to adjust the constants, in regards to the above comments.

Bhargavasomu avatar Dec 13 '18 15:12 Bhargavasomu

OK, I think I figured it out. In the bn128 curve, b2 = b / w**6. In the bls_12_381 curve, b2 = b * w**6. That's why it's different in the two documents and works in both.

vbuterin avatar Dec 16 '18 10:12 vbuterin