Skip to content
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

Feat: Eisenstein integers #543

Merged
merged 12 commits into from
Oct 8, 2024
Merged

Feat: Eisenstein integers #543

merged 12 commits into from
Oct 8, 2024

Conversation

yelhousni
Copy link
Collaborator

@yelhousni yelhousni commented Sep 21, 2024

Description

This PR implements Eisenstein integers arithmetic, including the half-GCD algorithm.
This follows:

Type of change

  • New feature (non-breaking change which adds functionality)

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:

goos: linux
goarch: amd64
pkg: github.com/consensys/gnark-crypto/field/eisenstein
cpu: Intel(R) Xeon(R) Platinum 8151 CPU @ 3.40GHz
BenchmarkHalfGCD-2   	
248566157222030422477351007620901975759+127180499814568938453986077818539212698*ω
332172497967381992638818630507579627203+168595014618012844916999503528379356776*ω
250917177940023814038197936340989040662+62542345892514041158477824749997954710*ω
318633110928069828602520176758759340623+128691427653754930750011514601704135735*ω
33830068183445111887751763414565085374+45267849578544034355196683977077448326*ω 
189270309561050775793081572721741878623+306301444086216412434750350679994937465*ω
    6859	    171191 ns/op

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I did not modify files generated from templates
  • golangci-lint does not output errors locally
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@yelhousni yelhousni added this to the v0.10.0 milestone Sep 21, 2024
@yelhousni yelhousni self-assigned this Sep 21, 2024
ecc/eisenstein_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

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.

field/eisenstein/eisenstein.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein.go Show resolved Hide resolved
field/eisenstein/eisenstein.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein_test.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein_test.go Show resolved Hide resolved
field/eisenstein/eisenstein_test.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein_test.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

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?

@gbotrel
Copy link
Collaborator

gbotrel commented Sep 25, 2024

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

@ivokub
Copy link
Collaborator

ivokub commented Sep 25, 2024

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 utils becomes something people throw stuff in and ... never use again :)

Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

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.

field/eisenstein/eisenstein.go Outdated Show resolved Hide resolved
field/eisenstein/eisenstein_test.go Outdated Show resolved Hide resolved
@ivokub
Copy link
Collaborator

ivokub commented Sep 25, 2024

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)

@yelhousni yelhousni requested review from ivokub and ThomasPiellard and removed request for ThomasPiellard October 3, 2024 21:36
Copy link
Collaborator

@ivokub ivokub left a comment

Choose a reason for hiding this comment

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

Looks perfect now! Thanks for the fixes.

@yelhousni yelhousni merged commit efa4b11 into master Oct 8, 2024
5 checks passed
@yelhousni yelhousni deleted the feat/eisenstein branch October 8, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants