Skip to content
New issue

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

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

Already on GitHub? # to your account

CIP-30: BLS12-377 Precompiles #1157

Closed
wants to merge 29 commits into from
Closed

CIP-30: BLS12-377 Precompiles #1157

wants to merge 29 commits into from

Conversation

kilic
Copy link
Contributor

@kilic kilic commented Sep 4, 2020

Description

This pr adds BLS12-377 elliptic curve operations as precompiles. Precompiles are added in same fashion with EIP-2537 except the mapping functions.

Background

Elliptic curve library is adapted from BLS12-381 and EIP-1962 works. x86 optimised field implementation is generated with fp and native go implementation is generated with goff

Other changes

Gas table for these new precompiles set is not decided yet. Rather than actual numbers placeholder constants are placed.

Tests

Underlying elliptic curve library is heavily tested in terms of mathematical properties.

Original library BLS12-381 is subjected to the security audit under drand project and also made its way to be backend of EIP-2537 go-ethereum precompiles* for the next HF.

Cross and fuzz testing are not applied for the adapted library yet, also static call test vectors are not generated.

Backwards compatibility

Precompiles are included next to Celo specific precompile bundle. It is assumed that precompile addresses incrementally follow the others.

@kilic kilic marked this pull request as draft September 4, 2020 21:25
@kilic kilic marked this pull request as ready for review September 21, 2020 13:14
@prestwich
Copy link
Contributor

There are #s specified in EIP-2539 based on benching the 1962 library. They are precisely equal to the EIP-2537 prices, so we should be able to re-use that # code for this PR as well.

I would prefer if this were explicitly an implementation of EIP-2539 (besides any address changes)

@prestwich
Copy link
Contributor

Update: I talked to @shamatar and he identified a metering issue that will result in gas price changes and a slight divergence between the EIPs. So hold off on making gas changes until those come back

@kilic
Copy link
Contributor Author

kilic commented Sep 24, 2020

Zexe test vectors are generated with https://github.com/kilic/zexe/blob/eip2539/algebra/src/bls12_377/curves/eip2539_test.rs

@prestwich we may want to reduce scalar multiplication gas price in short term since GLV multiplication which is more efficient is planned to be implemented.

@kilic
Copy link
Contributor Author

kilic commented Sep 24, 2020

Matter test vectors are generated with https://github.com/kilic/eip1962-1/tree/celo1157/src/public_interface/celo1157

However there are some issues of matter eip1962 implementation.

  1. b coefficient of G1 and G2 were wrong (I have fixed for test gens)
  2. Pairing equation outputs 1 where it should output 0. Here, apparently for odd numbers we expect 0. So I have changed manually to 0 which is I think not appropriate bad to do so. I haven't got a chance to what is wrong with it yet.

@prestwich
Copy link
Contributor

See gas metering update PR here:
ethereum/EIPs#2999

@prestwich
Copy link
Contributor

  • b coefficient of G1 and G2 were wrong (I have fixed for test gens)
  • Pairing equation outputs 1 where it should output 0. Here, apparently for odd numbers we expect 0. So I have changed manually to 0 which is I think not appropriate bad to do so. I haven't got a chance to what is wrong with it yet.

My open PR in 1962 addresses the b coefficient issue matter-labs/eip1962#14

I'll notify the Matter team and see if they'll take a look at the pairing output issue

@shamatar
Copy link

  1. Pairing equation outputs 1 where it should output 0. Here, apparently for odd numbers we expect 0. So I have changed manually to 0 which is I think not appropriate bad to do so. I haven't got a chance to what is wrong with it yet.

I've also updated coefficients in my optimizations branch (that uses special forms of non-residues), but what is a problem with a line there exactly? If number of pairs is even we should get combinations like e(a, b) * e(-a, b) = 1

errBLS12377G2PointSubgroup = errors.New("g2 point is not on correct subgroup")
)

// bls12377G1Add implements EIP-2537 G1Add precompile.
Copy link
Contributor

Choose a reason for hiding this comment

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

should be EIP-2539 throughout :)

@prestwich
Copy link
Contributor

prestwich commented Oct 13, 2020

I've been running some compatibility tests between this work and the rust eip1962 lib. I'm getting a mismatch on the pairing function with a specific input (below). The Golang implementation (accessed via geth rpc) returns false while the rustlang returns true. Given that these points were selected randomly, I would expect the output to be false.

I am working on this branch of eip1962:
https://github.com/prestwich/eip1962/tree/prestwich/eip2539

I would appreciate someone else checking this out and telling me if it's likely to be an error in my testing setup while I try to debug locally

000000000000000000000000000000000096ad951b95fb9160ed9087c35f5eab95e1e8aefe47440d7f1f1ec5cfde6f7a6caa774caca46ba2d988b16fac2831ff0000000000000000000000000000000000467e7134a6743759be14b955e7647983308dc12a389ca2fa3084ab54750afc82d7c4a58e5158aa307e0a6810c293a100000000000000000000000000000000015afe67f1ebc55d6bc08101957e673d56a5e675191f191ee537dbd3adaf342d3392147ab369b2cd930157dae07e178c0000000000000000000000000000000001511a4c76001b4acb211d77c5768da96c4b991075508065d8485ddda6fd4cf254f27e417f5d5da7234e6f48de449c39000000000000000000000000000000000075e634a2cfd17d1eb9b42ee6bee979e6cfbbb1a73608431fb8f783382b5a2c876580821a944c32b6637022a3d910f30000000000000000000000000000000000ab07f36540bb3ccad93061a48c5cc8848b5418e6b2eca075c439c2b336df56635552f9019980c58713fcfd74acab39

@kilic
Copy link
Contributor Author

kilic commented Oct 14, 2020

@prestwich

Did mismatch happen in the very first call or in a kind of fuzzing attempt?

I would appreciate someone else checking this out and telling me if it's likely to be an error in my testing setup while I try to debug locally

@shamatar

I've also updated coefficients in my optimizations branch (that uses special forms of non-residues), but what is a problem with a line there exactly?

I have added a small test below that demonstrates the issue.

https://github.com/kilic/eip1962-1/blob/1a8292cf676797025960b8d9b3328168394d3be5/src/public_interface/celo1157/mod.rs#L1060

@shamatar
Copy link

@prestwich

Did mismatch happen in the very first call or in a kind of fuzzing attempt?

I would appreciate someone else checking this out and telling me if it's likely to be an error in my testing setup while I try to debug locally

@shamatar

I've also updated coefficients in my optimizations branch (that uses special forms of non-residues), but what is a problem with a line there exactly?

I have added a small test below that demonstrates the issue.

https://github.com/kilic/eip1962-1/blob/1a8292cf676797025960b8d9b3328168394d3be5/src/public_interface/celo1157/mod.rs#L1060

@kilic Should I run this test for BLS12-377 curve? (to double check)

@shamatar
Copy link

Fixed the hardcoded parameters. Please use the "tuning" branch for now, it contains an API implementation for 2539, so can be tested from outside (signatures should match)

@prestwich
Copy link
Contributor

Did mismatch happen in the very first call or in a kind of fuzzing attempt?

On the very first call. I've also been running the others with random inputs generated by the eip1962 library without issues

@prestwich
Copy link
Contributor

Fixed the hardcoded parameters. Please use the "tuning" branch for now, it contains an API implementation for 2539, so can be tested from outside (signatures should match)

I will update my work to use the tuning branch and get back to you.

@shamatar
Copy link

There is a demo how to build generic fuzzer for precompiles like 2537/2539 in this repo https://github.com/shamatar/algebraic_fuzzer

It does not perform any actual comparison, just fuzzes G1 addition for now, but it looks to be ok to template it over some form of generators and runners

@shamatar
Copy link

There is now also branch with square root functions for 1 mod 16 (case of base field of BLS12-377), so we can actually fix a fuzzer for points in invalid subgroup and not on curve

@prestwich
Copy link
Contributor

My eip2539 branch of the fuzzer has been updated here: https://github.com/prestwich/algebraic_fuzzer

We are provisioning a node to run it for a bit.

@prestwich
Copy link
Contributor

My eip2539 branch of the fuzzer has been updated here: https://github.com/prestwich/algebraic_fuzzer

We are provisioning a node to run it for a bit.

Ran about 1billion iterations of EIP-2539 and 1.5 billion of EIP-2537. No issues encountered.

Given we have tests and a large number of fuzzing runs against an integration, we should update the gas costs to match the latest EIP2539. Any other things that need to be done before this is ready for review for inclusion in Donut?

@kilic kilic mentioned this pull request Jan 5, 2021
@prestwich
Copy link
Contributor

prestwich commented Jan 6, 2021

hey @kilic, I'm starting to shephard client integration. :) Can you please do the following:

  1. once tests: change precompile tests to use Donut block #1286 is merged, rebase onto latest master
  2. Remove your current address declarations
  3. Remove any activation logic (including any rows in the istanbul precompile block)
  4. Fill in the rows in the Donut Precompile block

Bls12377G2AddGas uint64 = 0 // Price for BLS12-377 elliptic curve G2 point addition
Bls12377G2MulGas uint64 = 0 // Price for BLS12-377 elliptic curve G2 point scalar multiplication
Bls12377PairingBaseGas uint64 = 0 // Base gas price for BLS12-377 elliptic curve pairing check
Bls12377PairingPerPairGas uint64 = 0 // Per-point pair gas price for BLS12-377 elliptic curve pairing check
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need to fill these in

@trianglesphere trianglesphere self-assigned this Jan 29, 2021
@mcortesi mcortesi changed the title BLS12-377 Precompiles CIP-30: BLS12-377 Precompiles Jan 29, 2021
@mcortesi mcortesi added the donut label Jan 29, 2021
@mcortesi
Copy link
Contributor

mcortesi commented Feb 8, 2021

Closed in favor of #1341

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

Successfully merging this pull request may close these issues.

5 participants