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

tests/fuzzers: crypto/bn256 and crypto/bls12381 tests against gnark-crypto #22755

Merged
merged 6 commits into from
Apr 28, 2021

Conversation

gbotrel
Copy link
Contributor

@gbotrel gbotrel commented Apr 27, 2021

This PR is a follow up to #21812 .

  1. gurvy/bn256 package name changed to gnark-crypto/ecc/bn254

  2. in FuzzPair test:
    replaces PairingCheck comparaisons across libraries with the actual output of the pairing function (Pair). PairingCheck of random points will almost always output false.

  3. adds similar tests for crypto/bls12381, against gnark-crypto/ecc/bls12381

Notes about crypto/bls12381 / MultiExp function:
defined here
MultiExp(r *PointG1, points []*PointG1, powers []*big.Int)

  • this mutates the powers slice and sets all its element to zero after a call
  • the powers slice elements are big.Int, but providing larger than 32bytes inputs make this function return an incorrect result. It is only referenced here in geth, which can't provide inputs larger than 32bytes. However, as crypto/bls12381 is a public package, I think this issue deserves some attention.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

The renaming of bls_fuzzer.go should be fine but the new fuzzer(s) need a corresponding update on https://github.com/ethereum/go-ethereum/blob/master/oss-fuzz.sh

Although, that doesn't really block this PR, could be done in a follow-up

github.com/Azure/azure-storage-blob-go v0.7.0
github.com/Azure/go-autorest/autorest/adal v0.8.0 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how these wound up here (and a few more similar).

Copy link
Contributor

Choose a reason for hiding this comment

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

I get the same, though, when I do a go mod tidy, so it seems fine to me

kps := make([]*bls12381.PointG1, n)
cps := make([]gnark.G1Affine, n)
for i := 0; i < int(n); i++ {
kps[i] = new(bls12381.PointG1).Set(kp1)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer setting the G1 points to different points, not only to a single point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've fixed that now -- it reads as many as it can (max 17), and uses those, and does not use the n

@holiman
Copy link
Contributor

holiman commented Apr 28, 2021

I pushed the changes to the oss-fuzz script

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.10.3 milestone Apr 28, 2021
@holiman holiman merged commit 9e5bb84 into ethereum:master Apr 28, 2021
@holiman
Copy link
Contributor

holiman commented Apr 28, 2021

Thanks @gbotrel !

atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…rypto (ethereum#22755)

Add more cross-fuzzers to fuzz bls with gnark versus geth's own bls12-381 library
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