Conversation
ivokub
left a comment
There was a problem hiding this comment.
In general looks quite good already, I only added a few comments for general improvement, but nothing significant imo. See which ones make sense, you don't have to incorporate all.
ivokub
left a comment
There was a problem hiding this comment.
One thing though - I'm not sure if gnark-crypto/field/eisenstein is good place for it. Previously the GLV scalar splitting functions are defined in gnark-crypto/ecc, but I wouldn't want to overload the ecc package.
Maybe gnark-crypto/utils instead?
Not a big fan of "utils" package, I prefer option 1 on my side it's more self describing when a caller uses it |
I completely agree, usually |
ivokub
left a comment
There was a problem hiding this comment.
Now looks a lot better, thanks! I'm approving for now even though there are two more remarks. I'm not really sure the benchmark thing is an issue (but I guess it really depends on the optimization levels etc.) and the other input length check only provides better panic message.
|
Suggested edit: diff --git a/field/eisenstein/eisenstein.go b/field/eisenstein/eisenstein.go
index 54bfde8af..2bf0dc46b 100644
--- a/field/eisenstein/eisenstein.go
+++ b/field/eisenstein/eisenstein.go
@@ -6,7 +6,16 @@ import (
// A ComplexNumber represents an arbitrary-precision Eisenstein integer.
type ComplexNumber struct {
- A0, A1 big.Int
+ A0, A1 *big.Int
+}
+
+func (z *ComplexNumber) init() {
+ if z.A0 == nil {
+ z.A0 = new(big.Int)
+ }
+ if z.A1 == nil {
+ z.A1 = new(big.Int)
+ }
}
// String implements Stringer interface for fancy printing
@@ -16,55 +25,60 @@ func (z *ComplexNumber) String() string {
// Equal returns true if z equals x, false otherwise
func (z *ComplexNumber) Equal(x *ComplexNumber) bool {
- return z.A0.Cmp(&x.A0) == 0 && z.A1.Cmp(&x.A1) == 0
+ return z.A0.Cmp(x.A0) == 0 && z.A1.Cmp(x.A1) == 0
}
// Set sets z to x, and returns z.
func (z *ComplexNumber) Set(x *ComplexNumber) *ComplexNumber {
- z.A0.Set(&x.A0)
- z.A1.Set(&x.A1)
+ z.init()
+ z.A0.Set(x.A0)
+ z.A1.Set(x.A1)
return z
}
// SetZero sets z to 0, and returns z.
func (z *ComplexNumber) SetZero() *ComplexNumber {
- z.A0 = *big.NewInt(0)
- z.A1 = *big.NewInt(0)
+ z.A0 = big.NewInt(0)
+ z.A1 = big.NewInt(0)
return z
}
// SetOne sets z to 1, and returns z.
func (z *ComplexNumber) SetOne() *ComplexNumber {
- z.A0 = *big.NewInt(1)
- z.A1 = *big.NewInt(0)
+ z.A0 = big.NewInt(1)
+ z.A1 = big.NewInt(0)
return z
}
// Neg sets z to the negative of x, and returns z.
func (z *ComplexNumber) Neg(x *ComplexNumber) *ComplexNumber {
- z.A0.Neg(&x.A0)
- z.A1.Neg(&x.A1)
+ z.init()
+ z.A0.Neg(x.A0)
+ z.A1.Neg(x.A1)
return z
}
// Conjugate sets z to the conjugate of x, and returns z.
func (z *ComplexNumber) Conjugate(x *ComplexNumber) *ComplexNumber {
- z.A0.Sub(&x.A0, &x.A1)
- z.A1.Neg(&x.A1)
+ z.init()
+ z.A0.Sub(x.A0, x.A1)
+ z.A1.Neg(x.A1)
return z
}
// Add sets z to the sum of x and y, and returns z.
func (z *ComplexNumber) Add(x, y *ComplexNumber) *ComplexNumber {
- z.A0.Add(&x.A0, &y.A0)
- z.A1.Add(&x.A1, &y.A1)
+ z.init()
+ z.A0.Add(x.A0, y.A0)
+ z.A1.Add(x.A1, y.A1)
return z
}
// Sub sets z to the difference of x and y, and returns z.
func (z *ComplexNumber) Sub(x, y *ComplexNumber) *ComplexNumber {
- z.A0.Sub(&x.A0, &y.A0)
- z.A1.Sub(&x.A1, &y.A1)
+ z.init()
+ z.A0.Sub(x.A0, y.A0)
+ z.A1.Sub(x.A1, y.A1)
return z
}
@@ -74,13 +88,14 @@ func (z *ComplexNumber) Sub(x, y *ComplexNumber) *ComplexNumber {
//
// (x0+x1ω)(y0+y1ω) = (x0y0-x1y1) + (x0y1+x1y0-x1y1)ω
func (z *ComplexNumber) Mul(x, y *ComplexNumber) *ComplexNumber {
+ z.init()
var t [3]big.Int
var z0, z1 big.Int
- t[0].Mul(&x.A0, &y.A0)
- t[1].Mul(&x.A1, &y.A1)
+ t[0].Mul(x.A0, y.A0)
+ t[1].Mul(x.A1, y.A1)
z0.Sub(&t[0], &t[1])
- t[0].Mul(&x.A0, &y.A1)
- t[2].Mul(&x.A1, &y.A0)
+ t[0].Mul(x.A0, y.A1)
+ t[2].Mul(x.A1, y.A0)
t[0].Add(&t[0], &t[2])
z1.Sub(&t[0], &t[1])
z.A0.Set(&z0)
@@ -97,12 +112,12 @@ func (z *ComplexNumber) Norm() *big.Int {
norm := new(big.Int)
temp := new(big.Int)
norm.Add(
- norm.Mul(&z.A0, &z.A0),
- temp.Mul(&z.A1, &z.A1),
+ norm.Mul(z.A0, z.A0),
+ temp.Mul(z.A1, z.A1),
)
norm.Sub(
norm,
- temp.Mul(&z.A0, &z.A1),
+ temp.Mul(z.A0, z.A1),
)
return norm
}
@@ -115,8 +130,8 @@ func (z *ComplexNumber) Quo(x, y *ComplexNumber) *ComplexNumber {
}
z.Conjugate(y)
z.Mul(x, z)
- z.A0.Div(&z.A0, norm)
- z.A1.Div(&z.A1, norm)
+ z.A0.Div(z.A0, norm)
+ z.A1.Div(z.A1, norm)
return z
}
@@ -128,8 +143,8 @@ func (z *ComplexNumber) QuoRem(x, y, r *ComplexNumber) (*ComplexNumber, *Complex
}
z.Conjugate(y)
z.Mul(x, z)
- z.A0.Div(&z.A0, norm)
- z.A1.Div(&z.A1, norm)
+ z.A0.Div(z.A0, norm)
+ z.A1.Div(z.A1, norm)
var t ComplexNumber
r.Sub(x, t.Mul(y, z))
return z, r
diff --git a/field/eisenstein/eisenstein_test.go b/field/eisenstein/eisenstein_test.go
index 224497201..67624d3fe 100644
--- a/field/eisenstein/eisenstein_test.go
+++ b/field/eisenstein/eisenstein_test.go
@@ -246,7 +246,7 @@ func GenNumber(boundSize int64) gopter.Gen {
var bound big.Int
bound.Exp(big.NewInt(2), big.NewInt(boundSize), nil)
elmt, _ := rand.Int(genParams.Rng, &bound)
- genResult := gopter.NewGenResult(*elmt, gopter.NoShrinker)
+ genResult := gopter.NewGenResult(elmt, gopter.NoShrinker)
return genResult
}
}
@@ -257,7 +257,7 @@ func GenComplexNumber(boundSize int64) gopter.Gen {
GenNumber(boundSize),
GenNumber(boundSize),
).Map(func(values []interface{}) *ComplexNumber {
- return &ComplexNumber{A0: values[0].(big.Int), A1: values[1].(big.Int)}
+ return &ComplexNumber{A0: values[0].(*big.Int), A1: values[1].(*big.Int)}
})
}
@@ -268,8 +268,8 @@ func BenchmarkHalfGCD(b *testing.B) {
a1, _ := rand.Int(rand.Reader, prime)
c0, _ := rand.Int(rand.Reader, prime)
c1, _ := rand.Int(rand.Reader, prime)
- a := ComplexNumber{A0: *a0, A1: *a1}
- c := ComplexNumber{A0: *c0, A1: *c1}
+ a := ComplexNumber{A0: a0, A1: a1}
+ c := ComplexNumber{A0: c0, A1: c1}
b.ResetTimer()
for i := 0; i < b.N; i++ {
_ = HalfGCD(&a, &c)
|
ivokub
left a comment
There was a problem hiding this comment.
Looks perfect now! Thanks for the fixes.
Description
This PR implements Eisenstein integers arithmetic, including the half-GCD algorithm.
This follows:
Type of change
How has this been tested?
Tests are implemented in
eisenstein_test.go.How has this been benchmarked?
Half-GCD of some random 128-bit Eisenstein integers on a z1d.large AWS machine:
Checklist:
golangci-lintdoes not output errors locally