-
Notifications
You must be signed in to change notification settings - Fork 472
feat: GKR Add Instance #1504
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
base: master
Are you sure you want to change the base?
feat: GKR Add Instance #1504
Changes from all commits
609d47d
79d4cbf
82e33e5
ba06e5d
700c152
30f5633
44d6b4c
f5f726c
644fd6a
2001e83
d3ca3c1
6474482
0c0b6b0
478d53d
6c83740
d6e382f
a8f30a4
8ac5f9f
5bb879d
7f9a0f1
355277c
cfdb9d3
12788f3
9a0bf0e
618beff
22b77f2
250a35b
1586760
e593d90
9df619a
8c88c52
f4d7aa8
499ff52
4ae9324
4765d61
b3d1af8
31a0a59
db56157
b4544d6
bdd01fd
c170695
70802ea
4ca453c
f2b1122
b9c01be
893ba91
a13ac27
8947d9f
4a09fd1
929d732
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
"runtime" | ||
"sync" | ||
|
||
"github.com/consensys/gnark" | ||
"github.com/consensys/gnark-crypto/ecc" | ||
|
||
bls12377 "github.com/consensys/gnark/internal/gkr/bls12-377" | ||
|
@@ -99,31 +100,62 @@ func WithCurves(curves ...ecc.ID) registerOption { | |
// - name is a human-readable name for the gate. | ||
// - f is the polynomial function defining the gate. | ||
// - nbIn is the number of inputs to the gate. | ||
func Register(f gkr.GateFunction, nbIn int, options ...registerOption) error { | ||
s := registerSettings{degree: -1, solvableVar: -1, name: GetDefaultGateName(f), curves: []ecc.ID{ecc.BN254}} | ||
// | ||
// If the gate is already registered, it will return false and no error. | ||
func Register(f gkr.GateFunction, nbIn int, options ...registerOption) (registered bool, err error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And it usually is better to make |
||
s := registerSettings{degree: -1, solvableVar: -1, name: GetDefaultGateName(f)} | ||
for _, option := range options { | ||
option(&s) | ||
} | ||
|
||
for _, curve := range s.curves { | ||
curvesForTesting := s.curves | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For gate degree testing - do we need an option at all? Cannot we just take some "good" modulus and test over that field. For me it makes a bit confusing that the user can choose the curve, but then needs to know the relation between the gate degree and modulus. |
||
allowedCurves := s.curves | ||
if len(curvesForTesting) == 0 { | ||
// no restriction on curves, but only test on BN254 | ||
curvesForTesting = []ecc.ID{ecc.BN254} | ||
allowedCurves = gnark.Curves() | ||
} | ||
|
||
gatesLock.Lock() | ||
defer gatesLock.Unlock() | ||
|
||
if g, ok := gates[s.name]; ok { | ||
// gate already registered | ||
if g.NbIn() != nbIn { | ||
return false, fmt.Errorf("gate \"%s\" already registered with a different number of inputs (%d != %d)", s.name, g.NbIn(), nbIn) | ||
} | ||
|
||
for _, curve := range curvesForTesting { | ||
gateVer, err := NewGateVerifier(curve) | ||
if err != nil { | ||
return false, err | ||
} | ||
if !gateVer.equal(f, g.Evaluate, nbIn) { | ||
return false, fmt.Errorf("mismatch with already registered gate \"%s\" (degree %d) over curve %s", s.name, g.Degree(), curve) | ||
} | ||
} | ||
|
||
return false, nil // gate already registered | ||
} | ||
|
||
for _, curve := range curvesForTesting { | ||
gateVer, err := NewGateVerifier(curve) | ||
if err != nil { | ||
return err | ||
return false, err | ||
} | ||
|
||
if s.degree == -1 { // find a degree | ||
if s.noDegreeVerification { | ||
panic("invalid settings") | ||
} | ||
const maxAutoDegreeBound = 32 | ||
var err error | ||
if s.degree, err = gateVer.findDegree(f, maxAutoDegreeBound, nbIn); err != nil { | ||
return fmt.Errorf("for gate %s: %v", s.name, err) | ||
return false, fmt.Errorf("for gate \"%s\": %v", s.name, err) | ||
} | ||
} else { | ||
if !s.noDegreeVerification { // check that the given degree is correct | ||
if err = gateVer.verifyDegree(f, s.degree, nbIn); err != nil { | ||
return fmt.Errorf("for gate %s: %v", s.name, err) | ||
return false, fmt.Errorf("for gate \"%s\": %v", s.name, err) | ||
} | ||
} | ||
} | ||
|
@@ -135,16 +167,14 @@ func Register(f gkr.GateFunction, nbIn int, options ...registerOption) error { | |
} else { | ||
// solvable variable given | ||
if !s.noSolvableVarVerification && !gateVer.isVarSolvable(f, s.solvableVar, nbIn) { | ||
return fmt.Errorf("cannot verify the solvability of variable %d in gate %s", s.solvableVar, s.name) | ||
return false, fmt.Errorf("cannot verify the solvability of variable %d in gate \"%s\"", s.solvableVar, s.name) | ||
} | ||
} | ||
|
||
} | ||
|
||
gatesLock.Lock() | ||
defer gatesLock.Unlock() | ||
gates[s.name] = gkrtypes.NewGate(f, nbIn, s.degree, s.solvableVar) | ||
return nil | ||
gates[s.name] = gkrtypes.NewGate(f, nbIn, s.degree, s.solvableVar, allowedCurves) | ||
return true, nil | ||
} | ||
|
||
func Get(name gkr.GateName) *gkrtypes.Gate { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add method documentation. |
||
|
@@ -160,6 +190,7 @@ type gateVerifier struct { | |
isAdditive func(f gkr.GateFunction, i int, nbIn int) bool | ||
findDegree func(f gkr.GateFunction, max, nbIn int) (int, error) | ||
verifyDegree func(f gkr.GateFunction, claimedDegree, nbIn int) error | ||
equal func(f1, f2 gkr.GateFunction, nbIn int) bool | ||
} | ||
|
||
func NewGateVerifier(curve ecc.ID) (*gateVerifier, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add method documentation |
||
|
@@ -172,30 +203,37 @@ func NewGateVerifier(curve ecc.ID) (*gateVerifier, error) { | |
o.isAdditive = bls12377.IsGateFunctionAdditive | ||
o.findDegree = bls12377.FindGateFunctionDegree | ||
o.verifyDegree = bls12377.VerifyGateFunctionDegree | ||
o.equal = bls12377.EqualGateFunction | ||
case ecc.BLS12_381: | ||
o.isAdditive = bls12381.IsGateFunctionAdditive | ||
o.findDegree = bls12381.FindGateFunctionDegree | ||
o.verifyDegree = bls12381.VerifyGateFunctionDegree | ||
o.equal = bls12381.EqualGateFunction | ||
case ecc.BLS24_315: | ||
o.isAdditive = bls24315.IsGateFunctionAdditive | ||
o.findDegree = bls24315.FindGateFunctionDegree | ||
o.verifyDegree = bls24315.VerifyGateFunctionDegree | ||
o.equal = bls24315.EqualGateFunction | ||
case ecc.BLS24_317: | ||
o.isAdditive = bls24317.IsGateFunctionAdditive | ||
o.findDegree = bls24317.FindGateFunctionDegree | ||
o.verifyDegree = bls24317.VerifyGateFunctionDegree | ||
o.equal = bls24317.EqualGateFunction | ||
case ecc.BN254: | ||
o.isAdditive = bn254.IsGateFunctionAdditive | ||
o.findDegree = bn254.FindGateFunctionDegree | ||
o.verifyDegree = bn254.VerifyGateFunctionDegree | ||
o.equal = bn254.EqualGateFunction | ||
case ecc.BW6_633: | ||
o.isAdditive = bw6633.IsGateFunctionAdditive | ||
o.findDegree = bw6633.FindGateFunctionDegree | ||
o.verifyDegree = bw6633.VerifyGateFunctionDegree | ||
o.equal = bw6633.EqualGateFunction | ||
case ecc.BW6_761: | ||
o.isAdditive = bw6761.IsGateFunctionAdditive | ||
o.findDegree = bw6761.FindGateFunctionDegree | ||
o.verifyDegree = bw6761.VerifyGateFunctionDegree | ||
o.equal = bw6761.EqualGateFunction | ||
default: | ||
err = fmt.Errorf("unsupported curve %s", curve) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,20 +11,38 @@ import ( | |
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestRegisterDegreeDetection(t *testing.T) { | ||
func TestRegister(t *testing.T) { | ||
testGate := func(name gkr.GateName, f gkr.GateFunction, nbIn, degree int) { | ||
t.Run(string(name), func(t *testing.T) { | ||
name = name + "-register-gate-test" | ||
|
||
assert.NoError(t, Register(f, nbIn, WithDegree(degree), WithName(name)), "given degree must be accepted") | ||
added, err := Register(f, nbIn, WithDegree(degree), WithName(name+"_given")) | ||
assert.NoError(t, err, "given degree must be accepted") | ||
assert.True(t, added, "registration must succeed for given degree") | ||
|
||
assert.Error(t, Register(f, nbIn, WithDegree(degree-1), WithName(name)), "lower degree must be rejected") | ||
registered, err := Register(f, nbIn, WithDegree(degree-1), WithName(name+"_lower")) | ||
assert.Error(t, err, "error must be returned for lower degree") | ||
assert.False(t, registered, "registration must fail for lower degree") | ||
|
||
assert.Error(t, Register(f, nbIn, WithDegree(degree+1), WithName(name)), "higher degree must be rejected") | ||
registered, err = Register(f, nbIn, WithDegree(degree+1), WithName(name+"_higher")) | ||
assert.Error(t, err, "error must be returned for higher degree") | ||
assert.False(t, registered, "registration must fail for higher degree") | ||
|
||
assert.NoError(t, Register(f, nbIn), "no degree must be accepted") | ||
registered, err = Register(f, nbIn, WithName(name+"_no_degree")) | ||
assert.NoError(t, err, "no error must be returned when no degree is specified") | ||
assert.True(t, registered, "registration must succeed when no degree is specified") | ||
|
||
assert.Equal(t, degree, Get(name).Degree(), "degree must be detected correctly") | ||
assert.Equal(t, degree, Get(name+"_no_degree").Degree(), "degree must be detected correctly") | ||
|
||
added, err = Register(f, nbIn, WithDegree(degree), WithName(name+"_given")) | ||
assert.NoError(t, err, "given degree must be accepted") | ||
assert.False(t, added, "gate must not be re-registered") | ||
|
||
added, err = Register(func(api gkr.GateAPI, x ...frontend.Variable) frontend.Variable { | ||
return api.Add(f(api, x...), 1) | ||
}, nbIn, WithDegree(degree), WithName(name+"_given")) | ||
assert.Error(t, err, "registering another function under the same name must fail") | ||
assert.False(t, added, "gate must not be re-registered") | ||
}) | ||
} | ||
|
||
|
@@ -47,15 +65,23 @@ func TestRegisterDegreeDetection(t *testing.T) { | |
) | ||
}, 2, 1) | ||
|
||
// zero polynomial must not be accepted | ||
t.Run("zero", func(t *testing.T) { | ||
const gateName gkr.GateName = "zero-register-gate-test" | ||
expectedError := fmt.Errorf("for gate %s: %v", gateName, gkrtypes.ErrZeroFunction) | ||
expectedError := fmt.Errorf("for gate \"%s\": %v", gateName, gkrtypes.ErrZeroFunction).Error() | ||
zeroGate := func(api gkr.GateAPI, x ...frontend.Variable) frontend.Variable { | ||
return api.Sub(x[0], x[0]) | ||
} | ||
assert.Equal(t, expectedError, Register(zeroGate, 1, WithName(gateName))) | ||
|
||
assert.Equal(t, expectedError, Register(zeroGate, 1, WithName(gateName), WithDegree(2))) | ||
// Attempt to register the zero gate without specifying a degree | ||
registered, err := Register(zeroGate, 1, WithName(gateName)) | ||
assert.Error(t, err, "error must be returned for zero polynomial") | ||
assert.EqualError(t, err, expectedError, "error message must match expected error") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is not idiomatic Go to compare errors by messages. If you really want to ensure that some error is what you expect, then define a new error type var errSomeKnownError = errors.New("error msg")
///
err := functionThatReturnsErrors()
errors.Is(err, errSomeKnownError) or if you want to add context to error then implement type parametricError struct { name string }
func (pe parametricError) Error() string {
return fmt.Sprintf("error with parameter: %s", pe.name)
}
func functionThatReturnsErrors() error {
return ¶metricError{name: "aaaa"}
}
err := functionThatReturnsErrors()
var wantedError *parametricError
if errors.As(err, ¶metricError) {
// error is *parametricError
} |
||
assert.False(t, registered, "registration must fail for zero polynomial") | ||
|
||
// Attempt to register the zero gate with a specified degree | ||
registered, err = Register(zeroGate, 1, WithName(gateName), WithDegree(2)) | ||
assert.Error(t, err, "error must be returned for zero polynomial with degree") | ||
assert.EqualError(t, err, expectedError, "error message must match expected error") | ||
assert.False(t, registered, "registration must fail for zero polynomial with degree") | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to return boolean to indicate if the gate was registered or not? When you return an error, then it is always false. And when the gate is already registered with same properties, then it is a no-op.
It seems you only use it for testing, I think it doesn't make sense to break APIs and add extra return values you don't use outside of tests.