Feat: emulated subgroup check by yelhousni · Pull Request #629 · Consensys/gnark

yelhousni · 2023-04-07T16:57:46Z

Adds the methods:

AssertIsOnCurve to sw_emulated,
AssertIsOnG1 and AssertIsOnG2 to sw_bn254 and sw_bl12381 with hints.

This PR needs Consensys/gnark-crypto#376.

TODO:

use these methods in the EVM precompiles (std/evmprecompiles). This is to confirm with @OlivierBBB, as the arithmetization would probably do the AssertIsOnCurve part.

OlivierBBB · 2023-04-11T18:58:32Z

The current plan is for the zk-evm arithmetization to perform preliminary checks (is on curve for C1 and C2) but not subgroup checks (for G2) in order to reduce the number of junk calls to this circuit.

yelhousni · 2023-04-11T19:02:39Z

The current plan is for the zk-evm arithmetization to perform preliminary checks (is on curve for C1 and C2) but not subgroup checks (for G2) in order to reduce the number of junk calls to this circuit.

Yes! this is captured in the last commit 0e0958c.

yelhousni · 2023-04-13T12:27:55Z

Follows https://github.com/ConsenSys/zkevm-spec/issues/50 and fixes #634

ivokub · 2023-04-17T08:06:42Z

Suggested edit:

diff --git a/std/algebra/emulated/sw_bn254/hints.go b/std/algebra/emulated/sw_bn254/hints.go
index b1674afd..9f240981 100644
--- a/std/algebra/emulated/sw_bn254/hints.go
+++ b/std/algebra/emulated/sw_bn254/hints.go
@@ -9,7 +9,11 @@ import (
 )
 
 func init() {
-	solver.RegisterHint(subgroupG2Hint)
+	solver.RegisterHint(GetHints()...)
+}
+
+func GetHints() []solver.Hint {
+	return []solver.Hint{subgroupG2Hint}
 }
 
 func subgroupG2Hint(nativeMod *big.Int, nativeInputs, nativeOutputs []*big.Int) error {
diff --git a/std/hints.go b/std/hints.go
index 2d044830..5d54fb10 100644
--- a/std/hints.go
+++ b/std/hints.go
@@ -4,6 +4,8 @@ import (
 	"sync"
 
 	"github.com/consensys/gnark/constraint/solver"
+	"github.com/consensys/gnark/std/algebra/emulated/sw_bls12381"
+	"github.com/consensys/gnark/std/algebra/emulated/sw_bn254"
 	"github.com/consensys/gnark/std/algebra/native/sw_bls12377"
 	"github.com/consensys/gnark/std/algebra/native/sw_bls24315"
 	"github.com/consensys/gnark/std/evmprecompiles"
@@ -37,4 +39,6 @@ func registerHints() {
 	solver.RegisterHint(emulated.GetHints()...)
 	solver.RegisterHint(rangecheck.CountHint, rangecheck.DecomposeHint)
 	solver.RegisterHint(evmprecompiles.GetHints()...)
+	solver.RegisterHint(sw_bn254.GetHints()...)
+	solver.RegisterHint(sw_bls12381.GetHints()...)
 }

ivokub · 2023-04-17T08:16:34Z

Suggested edit:

diff --git a/std/algebra/emulated/sw_bn254/hints.go b/std/algebra/emulated/sw_bn254/hints.go
index b1674afd..9f240981 100644
--- a/std/algebra/emulated/sw_bn254/hints.go
+++ b/std/algebra/emulated/sw_bn254/hints.go
@@ -9,7 +9,11 @@ import (
 )
 
 func init() {
-	solver.RegisterHint(subgroupG2Hint)
+	solver.RegisterHint(GetHints()...)
+}
+
+func GetHints() []solver.Hint {
+	return []solver.Hint{subgroupG2Hint}
 }
 
 func subgroupG2Hint(nativeMod *big.Int, nativeInputs, nativeOutputs []*big.Int) error {
diff --git a/std/hints.go b/std/hints.go
index 2d044830..5d54fb10 100644
--- a/std/hints.go
+++ b/std/hints.go
@@ -4,6 +4,8 @@ import (
 	"sync"
 
 	"github.com/consensys/gnark/constraint/solver"
+	"github.com/consensys/gnark/std/algebra/emulated/sw_bls12381"
+	"github.com/consensys/gnark/std/algebra/emulated/sw_bn254"
 	"github.com/consensys/gnark/std/algebra/native/sw_bls12377"
 	"github.com/consensys/gnark/std/algebra/native/sw_bls24315"
 	"github.com/consensys/gnark/std/evmprecompiles"
@@ -37,4 +39,6 @@ func registerHints() {
 	solver.RegisterHint(emulated.GetHints()...)
 	solver.RegisterHint(rangecheck.CountHint, rangecheck.DecomposeHint)
 	solver.RegisterHint(evmprecompiles.GetHints()...)
+	solver.RegisterHint(sw_bn254.GetHints()...)
+	solver.RegisterHint(sw_bls12381.GetHints()...)
 }

This is to ensure that if the compiler and prover are in different processes then in the prover process we do std.RegisterHints() once to get all hints defined in std/ packages.

ivokub · 2023-04-17T08:36:13Z

Suggested edit:

diff --git a/std/algebra/emulated/sw_bls12381/doc_test.go b/std/algebra/emulated/sw_bls12381/doc_test.go
index ae87b729..a1ce0f5c 100644
--- a/std/algebra/emulated/sw_bls12381/doc_test.go
+++ b/std/algebra/emulated/sw_bls12381/doc_test.go
@@ -23,6 +23,10 @@ func (c *PairCircuit) Define(api frontend.API) error {
 	if err != nil {
 		return fmt.Errorf("new pairing: %w", err)
 	}
+	// Pair method does not check that the points are in the proper groups.
+	pairing.AssertIsOnG1(&c.InG1)
+	pairing.AssertIsOnG2(&c.InG2)
+	// Compute the pairing
 	res, err := pairing.Pair([]*sw_bls12381.G1Affine{&c.InG1}, []*sw_bls12381.G2Affine{&c.InG2})
 	if err != nil {
 		return fmt.Errorf("pair: %w", err)
diff --git a/std/algebra/emulated/sw_bn254/doc_test.go b/std/algebra/emulated/sw_bn254/doc_test.go
index db095a02..7d8ef6a6 100644
--- a/std/algebra/emulated/sw_bn254/doc_test.go
+++ b/std/algebra/emulated/sw_bn254/doc_test.go
@@ -23,6 +23,10 @@ func (c *PairCircuit) Define(api frontend.API) error {
 	if err != nil {
 		return fmt.Errorf("new pairing: %w", err)
 	}
+	// Pair method does not check that the points are in the proper groups.
+	pairing.AssertIsOnG1(&c.InG1)
+	pairing.AssertIsOnG2(&c.InG2)
+	// Compute the pairing
 	res, err := pairing.Pair([]*sw_bn254.G1Affine{&c.InG1}, []*sw_bn254.G2Affine{&c.InG2})
 	if err != nil {
 		return fmt.Errorf("pair: %w", err)

std/algebra/emulated/sw_bn254/pairing.go

std/algebra/emulated/sw_bls12381/pairing.go

ivokub

I'm not sure the G1 group checks are sufficient. Imo if the prover is malicious and replaces the hints with identity functions then the in-circuit assertions hold but the points may still be outside the group?

It is also possible I have misunderstood. Otherwise I think is perfect, but I would only wrap AssertIsOnCurve of sw_emulated instead of reimplementing in this package, but I can do it myself.

yelhousni · 2023-04-17T09:01:14Z

I'm not sure the G1 group checks are sufficient. Imo if the prover is malicious and replaces the hints with identity functions then the in-circuit assertions hold but the points may still be outside the group?

I assumed that the hint is a private function and a gnark user cannot change it, but I see your point.

It is also possible I have misunderstood. Otherwise I think is perfect, but I would only wrap AssertIsOnCurve of sw_emulated instead of reimplementing in this package, but I can do it myself.

Go for the suggested edit and I'll think about the subgroup membership hints.

ivokub · 2023-04-17T10:14:36Z

I'm not sure the G1 group checks are sufficient. Imo if the prover is malicious and replaces the hints with identity functions then the in-circuit assertions hold but the points may still be outside the group?

I assumed that the hint is a private function and a gnark user cannot change it, but I see your point.

We now even have made it easier with a solver option for overriding hints: https://github.com/ConsenSys/gnark/blob/develop/constraint/solver/options.go#L39-L44

yelhousni · 2023-04-17T18:14:16Z

@ivokub Ok I did the BN254 G2 membership directly in-circuit (without hints). I optimised quite a bit the (fixed) scalar mul. A full pairing with both G1 and G2 membership:

first attempt (unoptimized b6cc505): 1578534
optimized b5caf13: 1498532
with hint (but insecure): 1384729

Will do BLS12-381 later when you review the structure of the code?

std/algebra/emulated/sw_bn254/pairing.go

ivokub · 2023-04-18T00:24:51Z

I think it looks good! Imo considering that the subgroup check is optional the performance of the check is great.

yelhousni · 2023-04-18T13:05:42Z

I think it looks good! Imo considering that the subgroup check is optional the performance of the check is great.

Did the BLS12-381 too but did not optimize the fixed scalar Mul in G1. We don't use the AssertIsOnG1 anyway in EVM nor in Pair method anyway.

Edit: An addition chain using the public method AddUnified from sw_emulated for both add/double in the chain costs more constraints (3.6M vs 2.7M) than calling ScalarMul with the fixed scalar (which uses private double and add methods).

ivokub

Looks good! Only fixed one typo.

Edit: and merge develop. No should be good to merge!

yelhousni added 9 commits March 23, 2023 17:42

feat: add IsOnCurve to sw_emulated

263b4b3

feat: add IsOnCurve to sw_bn254/g2

fc31d31

Merge branch 'develop' into feat/emulated/subgroup-check

6d0884c

feat(pairing): check points are on curve and twist

45394fc

Merge branch 'develop' into feat/emulated/subgroup-check

d82be56

test: test bls12-381 in sw_emulated + comments

52f2465

test: add bn254 and bl12381 test of AssertIsOnCurve

b9484d1

feat(sw_bn254): add AssertIsOnG2

3bea95b

feat(sw_bls12381): add AssertIsOnG1 and AssertIsOnG2

d62695e

yelhousni added the dep: linea Issues affecting Linea downstream label Apr 7, 2023

yelhousni added this to the v0.9.0 milestone Apr 7, 2023

yelhousni requested review from gbotrel and ivokub April 7, 2023 16:57

yelhousni self-assigned this Apr 7, 2023

yelhousni added 3 commits April 11, 2023 15:31

Merge branch 'develop' into feat/emulated/subgroup-check

ec8253a

chore: point to gnark-crypto@develop

ba144cc

feat: use AssertIsOnG2 for ECPAIR precompile + comments

0e0958c

fix: add (0,0) case to curve membership

760d534