bn256: added consensys/gurvy bn256 implementation

[MariusVanDerWijden]

This pr adds consensys' gurvy bn256 variant into the code for differential fuzzing.
Since it is specially built with the correct parameters we don't need to vendor it.

[holiman]

For this we need to wait for Consensys/gnark-crypto#17.

It's merged

[MariusVanDerWijden]

Well then I know what to work on next :D

[MariusVanDerWijden]

TODO: add the build flags back in

[holiman]

The fuzzers only do essentially random fuzzing without structure which means ~ 1 in 80 million tests produces a valid input to add or mulexp, the pairing is probably even worse

Also, it's super-picky about input -- essentially disqualifyiing everything that is not exactly the correct size. A more forgiving implementation could instead read input in chunks of required_size, and loop over the chunks. Don't know many cases are discarded for the reason over being 'too large', but might be a couple

[holiman]

Maybe let's do something like this too:

diff --git a/crypto/bn256/bn256_fuzz.go b/crypto/bn256/bn256_fuzz.go
index 6aa1421170..dda80ff149 100644
--- a/crypto/bn256/bn256_fuzz.go
+++ b/crypto/bn256/bn256_fuzz.go
@@ -8,42 +8,44 @@ package bn256
 
 import (
 	"bytes"
+	"fmt"
+	"io"
 	"math/big"
 
 	cloudflare "github.com/ethereum/go-ethereum/crypto/bn256/cloudflare"
 	google "github.com/ethereum/go-ethereum/crypto/bn256/google"
 )
 
-// FuzzAdd fuzzez bn256 addition between the Google and Cloudflare libraries.
-func FuzzAdd(data []byte) int {
-	// Ensure we have enough data in the first place
-	if len(data) != 128 {
-		return 0
+func getG1Points(input io.Reader) (*cloudflare.G1, *google.G1) {
+	xc, err := cloudflare.RandomG1()
+	if err != nil {
+		// insufficient input
+		return nil, nil
+	}
+	dat, err := xc.MarshalText()
+	if err != nil {
+		panic("marshalling failed")
 	}
-	// Ensure both libs can parse the first curve point
-	xc := new(cloudflare.G1)
-	_, errc := xc.Unmarshal(data[:64])
-
 	xg := new(google.G1)
-	_, errg := xg.Unmarshal(data[:64])
-
-	if (errc == nil) != (errg == nil) {
-		panic("parse mismatch")
-	} else if errc != nil {
-		return 0
+	if _, err := xg.Unmarshal(dat); err != nil {
+		panic(fmt.Sprintf("Could not marshal couldflare -> google:", err))
 	}
-	// Ensure both libs can parse the second curve point
-	yc := new(cloudflare.G1)
-	_, errc = yc.Unmarshal(data[64:])
+	return xc, xg
 
-	yg := new(google.G1)
-	_, errg = yg.Unmarshal(data[64:])
+}
 
-	if (errc == nil) != (errg == nil) {
-		panic("parse mismatch")
-	} else if errc != nil {
+// FuzzAdd fuzzez bn256 addition between the Google and Cloudflare libraries.
+func FuzzAdd(data []byte) int {
+	input := bytes.NewReader(data)
+	xc, xg := getG1Points(input)
+	if xc == nil {
 		return 0
 	}
+	yc, yg := getG1Points(input)
+	if yc == nil {
+		return 0
+	}
+	// Ensure both libs can parse the second curve point
 	// Add the two points and ensure they result in the same output
 	rc := new(cloudflare.G1)
 	rc.Add(xc, yc)

[holiman]

Or let me make an alternative PR about that first... ?

[MariusVanDerWijden]

I think that should be an alternative pr @holiman (that could get merged in first)
Gurvy is still pretty rough around the edges so I think we should give it another month or two before putting it in

[holiman]

#21815

[gbotrel]

@MariusVanDerWijden hi :) . gurvy on develop should be 100% compatible with google and cloudflare implementations, including subgroup checks during deserialization.

We're making a pass on the pairing APIs so that they are easily compatible with the EVM precompiles . Then we will merge in master and ping you (in few days).

[MariusVanDerWijden]

Perfect! thank you very much @gbotrel

[MariusVanDerWijden]

One problem we currently face is that gurvy uses go-ethereum internally creating a dependency cycle which is technically not problematic. However it results in gurvy pulling in 116 additional dependencies. gurvy uses the go-ethereum packages crypto/bn256/cloudflare and crypto/bn256/google to test against in bn256/interop_test.go

[DGKSK8LIFE]

Hmm. Sounds like this is inherent to Gurvy, right? So seems like there's nothing you can really do here? @MariusVanDerWijden

[gbotrel]

@MariusVanDerWijden this issue should be fixed, using gurvy@v0.3.7 should work, let me know if you have other issues, happy to help.

[holiman]

@MariusVanDerWijden is this still WIP or good to go?

[MariusVanDerWijden]

I think its good to go, will rebase once more

[gbotrel]

@MariusVanDerWijden can you make gurvy point to v0.3.8 in the go.mod? @yelhousni merged some algorithmic / performance improvement to the pairing (& multi-pairing) (+ benchmarks )

Cheers,

[holiman]

LGTM but please look into the go.mod changes

[holiman]

These are probably not actual changes related to this pr, are they? Might need a rebase ?

[MariusVanDerWijden]

It's super weird, whenever I run go test ./... it just updates the go-mod with some unrelated changes. I removed them now by hand

[holiman]

LGTM