Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes Eisenstein integer arithmetic by switching to inline big.Int storage, introducing Karatsuba-based multiplication, and adding new division routines (Quo, MulByConjugate) and round-nearest logic.
Key changes:
- Redesigned
ComplexNumberto store temporaries and prevent unintended copies. - Added
MulByConjugate,roundNearest,Quo, and reworkedQuoRemandHalfGCDfor performance. - Updated tests and benchmarks to cover the new methods and signatures.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| field/eisenstein/eisenstein.go | Refactored ComplexNumber, implemented Karatsuba mul, new division and rounding methods, and adjusted HalfGCD. |
| field/eisenstein/eisenstein_test.go | Added property tests for MulByConjugate, updated Norm and QuoRem tests, and expanded benchmarks. |
Comments suppressed due to low confidence (1)
field/eisenstein/eisenstein_test.go:122
- The new property in
TestEisensteinArithmeticis never executed because there's noproperties.TestingRuncall at the end of that test function. Addproperties.TestingRun(t, gopter.ConsoleReporter(false))to ensure the property runs.
properties.Property("Mul(Conjugate) & MulByConjugate should output the same result", prop.ForAll(
There was a problem hiding this comment.
Bug: Nil Pointer Dereference in Complex Number Benchmarks
The new benchmark functions BenchmarkMul, BenchmarkNorm, and BenchmarkQuoRem attempt to use uninitialized *ComplexNumber pointers from the benchRes array (e.g., benchRes[0], benchRes[1]). Since benchRes elements are nil by default, these operations result in nil pointer dereference panics when methods are called or fields are accessed.
field/eisenstein/eisenstein_test.go#L394-L405
gnark-crypto/field/eisenstein/eisenstein_test.go
Lines 394 to 405 in 0dcd1ad
field/eisenstein/eisenstein_test.go#L417-L418
gnark-crypto/field/eisenstein/eisenstein_test.go
Lines 417 to 418 in 0dcd1ad
Bug: Method Modifies Input Parameter Violating Expectations
The QuoRem method incorrectly modifies its input parameter x by using x.t1 as a temporary variable for norm calculations. This violates the expectation that input parameters remain unchanged. Specifically, when the receiver z and parameter x are the same object, bestQ0 (aliased to z.t1) is overwritten by candR.Norm(&x.t1) before the comparison, leading to incorrect quotient selection in the Euclidean division's lattice search.
field/eisenstein/eisenstein.go#L265-L270
gnark-crypto/field/eisenstein/eisenstein.go
Lines 265 to 270 in 0dcd1ad
field/eisenstein/eisenstein.go#L240-L241
gnark-crypto/field/eisenstein/eisenstein.go
Lines 240 to 241 in 0dcd1ad
Was this report helpful? Give feedback by reacting with 👍 or 👎
Description
Some optimisations for Eisenstein arithmetic. This saves -50% for Half-GCD which we use in hints for emulated scalar multiplications in gnark.
Type of change
How has this been tested?
Existing tests pass + test for new method
MulByConjugate.How has this been benchmarked?
Macbook Air M1:
Checklist:
golangci-lintdoes not output errors locally