Skip to content

test: handle gnark-crypto not returning malleable signatures anymore #1601

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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

test: handle gnark-crypto not returning malleable signatures anymore #1601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5983894
test: handle gnark-crypto returning non-malleable signatures
ivokub Sep 10, 2025
1f3a5d5
chore: remove unused benchmark
ivokub Sep 10, 2025
97b8ec1
chore: don't need loop anymore
ivokub Sep 10, 2025
2ec5218
fix: also sign in v
ivokub Sep 10, 2025
38a0bc0
test: ensure full coverage with small and large s
ivokub Sep 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ require (
github.com/blang/semver/v4 v4.0.0
github.com/consensys/bavard v0.2.1
github.com/consensys/compress v0.2.5
github.com/consensys/gnark-crypto v0.19.0
github.com/consensys/gnark-crypto v0.19.1-0.20250910105230-a45dd40113a7
github.com/fxamacker/cbor/v2 v2.9.0
github.com/google/go-cmp v0.7.0
github.com/google/pprof v0.0.0-20250820193118-f64d9cf942d6
Expand All @@ -16,7 +16,7 @@ require (
github.com/leanovate/gopter v0.2.11
github.com/ronanh/intcomp v1.1.1
github.com/rs/zerolog v1.34.0
github.com/stretchr/testify v1.10.0
github.com/stretchr/testify v1.11.1
golang.org/x/crypto v0.41.0
golang.org/x/exp v0.0.0-20250819193227-8b4c13bb791b
golang.org/x/sync v0.16.0
Expand Down
8 changes: 4 additions & 4 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ github.com/consensys/bavard v0.2.1 h1:i2/ZeLXpp7eblPWzUIWf+dtfBocKQIxuiqy9XZlNSf
github.com/consensys/bavard v0.2.1/go.mod h1:k/zVjHHC4B+PQy1Pg7fgvG3ALicQw540Crag8qx+dZs=
github.com/consensys/compress v0.2.5 h1:gJr1hKzbOD36JFsF1AN8lfXz1yevnJi1YolffY19Ntk=
github.com/consensys/compress v0.2.5/go.mod h1:pyM+ZXiNUh7/0+AUjUf9RKUM6vSH7T/fsn5LLS0j1Tk=
github.com/consensys/gnark-crypto v0.19.0 h1:zXCqeY2txSaMl6G5wFpZzMWJU9HPNh8qxPnYJ1BL9vA=
github.com/consensys/gnark-crypto v0.19.0/go.mod h1:rT23F0XSZqE0mUA0+pRtnL56IbPxs6gp4CeRsBk4XS0=
github.com/consensys/gnark-crypto v0.19.1-0.20250910105230-a45dd40113a7 h1:HRkegCH6HOk2algk1j6pvAydWTJPcKyxvokCla1/FYQ=
github.com/consensys/gnark-crypto v0.19.1-0.20250910105230-a45dd40113a7/go.mod h1:OgCH7cSoJ46c+nOzvQuwOrIE9fawpXMYOQFzj22Vy3E=
github.com/coreos/go-semver v0.3.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk=
github.com/coreos/go-systemd/v22 v22.3.2/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc=
github.com/coreos/go-systemd/v22 v22.5.0/go.mod h1:Y58oyj3AT4RCenI/lSvhwexgC+NSVTIJ3seZv2GcEnc=
Expand Down Expand Up @@ -272,8 +272,8 @@ github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81P
github.com/stretchr/testify v1.5.1/go.mod h1:5W2xD1RspED5o8YsWQXVCued0rvSQ+mT+I5cxcmMvtA=
github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
github.com/stretchr/testify v1.10.0 h1:Xv5erBjTwe/5IxqUQTdXv5kgmIvbHo3QQyRwhJsOfJA=
github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U=
github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U=
github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw=
github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg=
Expand Down
54 changes: 29 additions & 25 deletions std/evmprecompiles/01-ecrecover_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (c *ecrecoverCircuit) Define(api frontend.API) error {
return nil
}

func testRoutineECRecover(t *testing.T, wantStrict bool) (circ, wit *ecrecoverCircuit, largeS bool) {
func testRoutineECRecover(t *testing.T, forceLargeS bool) (circ, wit *ecrecoverCircuit) {
halfFr := new(big.Int).Sub(fr.Modulus(), big.NewInt(1))
halfFr.Div(halfFr, big.NewInt(2))

Expand All @@ -72,18 +72,22 @@ func testRoutineECRecover(t *testing.T, wantStrict bool) (circ, wit *ecrecoverCi
msg := []byte("test")
var r, s *big.Int
var v uint
for {
v, r, s, err = sk.SignForRecover(msg, nil)
if err != nil {
t.Fatal("sign", err)
}
if !wantStrict || halfFr.Cmp(s) > 0 {
break
}
v, r, s, err = sk.SignForRecover(msg, nil)
if err != nil {
t.Fatal("sign", err)
}
strict := 0
if wantStrict {
strict = 1
// SignForRecover always returns s < r_mod/2. But in the tests we want
// to check that the circuit fails when s > r_mod/2 in strict mode.
if forceLargeS {
// first we make s large
s.Sub(fr.Modulus(), s)
// but we also have to swap the sign of the recovered public key
v ^= 1
}

strict := 1
if forceLargeS {
strict = 0
}
circuit := ecrecoverCircuit{}
witness := ecrecoverCircuit{
Expand All @@ -98,19 +102,19 @@ func testRoutineECRecover(t *testing.T, wantStrict bool) (circ, wit *ecrecoverCi
Y: emulated.ValueOf[emulated.Secp256k1Fp](pk.A.Y),
},
}
return &circuit, &witness, halfFr.Cmp(s) <= 0
return &circuit, &witness
}

func TestECRecoverCircuitShortStrict(t *testing.T) {
assert := test.NewAssert(t)
circuit, witness, _ := testRoutineECRecover(t, true)
circuit, witness := testRoutineECRecover(t, false)
err := test.IsSolved(circuit, witness, ecc.BN254.ScalarField())
assert.NoError(err)
}

func TestECRecoverCircuitShortLax(t *testing.T) {
assert := test.NewAssert(t)
circuit, witness, _ := testRoutineECRecover(t, false)
circuit, witness := testRoutineECRecover(t, true)
Copy link

Choose a reason for hiding this comment

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

Bug: Refactoring Alters Test Behavior and Coverage

The refactoring of testRoutineECRecover changed how strict mode and s values are set. This results in TestECRecoverCircuitShortLax now testing large s values, which conflicts with its name. Additionally, TestECRecoverCircuitFull and TestInvalidFailureTag now run in strict mode, potentially reducing coverage for lax mode functionality.

Additional Locations (1)

Fix in Cursor Fix in Web

err := test.IsSolved(circuit, witness, ecc.BN254.ScalarField())
assert.NoError(err)
}
Expand All @@ -120,25 +124,21 @@ func TestECRecoverCircuitShortMismatch(t *testing.T) {
halfFr := new(big.Int).Sub(fr.Modulus(), big.NewInt(1))
halfFr.Div(halfFr, big.NewInt(2))
var circuit, witness *ecrecoverCircuit
var largeS bool
for {
circuit, witness, largeS = testRoutineECRecover(t, false)
if largeS {
witness.Strict = 1
break
}
}
circuit, witness = testRoutineECRecover(t, true)
witness.Strict = 1
err := test.IsSolved(circuit, witness, ecc.BN254.ScalarField())
assert.Error(err)
}

func TestECRecoverCircuitFull(t *testing.T) {
assert := test.NewAssert(t)
circuit, witness, _ := testRoutineECRecover(t, false)
circuit, witness := testRoutineECRecover(t, false)
_, witness2 := testRoutineECRecover(t, true)

assert.CheckCircuit(
circuit,
test.WithValidAssignment(witness),
test.WithValidAssignment(witness2),
test.WithCurves(ecc.BN254, ecc.BLS12_377),
test.NoProverChecks(),
)
Expand Down Expand Up @@ -256,10 +256,14 @@ func TestECRecoverInfinityWoFailure(t *testing.T) {

func TestInvalidFailureTag(t *testing.T) {
assert := test.NewAssert(t)
circuit, witness, _ := testRoutineECRecover(t, false)
circuit, witness := testRoutineECRecover(t, false)
witness.IsFailure = 1
err := test.IsSolved(circuit, witness, ecc.BN254.ScalarField())
assert.Error(err)
_, witness2 := testRoutineECRecover(t, true)
witness2.IsFailure = 1
err = test.IsSolved(circuit, witness2, ecc.BN254.ScalarField())
assert.Error(err)
}

func TestLargeV(t *testing.T) {
Expand Down
27 changes: 0 additions & 27 deletions std/signature/ecdsa/ecdsa_secpr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,6 @@ import (
"testing"

"github.com/consensys/gnark-crypto/ecc"
"github.com/consensys/gnark/constraint"
"github.com/consensys/gnark/frontend"
"github.com/consensys/gnark/frontend/cs/r1cs"
"github.com/consensys/gnark/frontend/cs/scs"
"github.com/consensys/gnark/std/math/emulated"
"github.com/consensys/gnark/test"
"golang.org/x/crypto/cryptobyte"
Expand Down Expand Up @@ -113,26 +109,3 @@ func TestEcdsaP384PreHashed(t *testing.T) {
assert.NoError(err)

}

var ccsBench constraint.ConstraintSystem

func BenchmarkCompile(b *testing.B) {
// create an empty cs
var circuit EcdsaCircuit[emulated.P384Fp, emulated.P384Fr]

var ccs constraint.ConstraintSystem
b.ResetTimer()
for i := 0; i < b.N; i++ {
ccs, _ = frontend.Compile(ecc.BN254.ScalarField(), scs.NewBuilder, &circuit)
}
b.Log("scs constraints", ccs.GetNbConstraints())

b.Run("groth16", func(b *testing.B) {
for i := 0; i < b.N; i++ {
ccsBench, _ = frontend.Compile(ecc.BW6_633.ScalarField(), r1cs.NewBuilder, &circuit)
}

})
b.Log("r1cs constraints", ccsBench.GetNbConstraints())

}
Loading