-
Notifications
You must be signed in to change notification settings - Fork 360
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
Perf: KZG in circuit #506
Perf: KZG in circuit #506
Conversation
Suggested edit: diff --git a/std/algebra/sw_bls12377/g2.go b/std/algebra/sw_bls12377/g2.go
index e4da2028..c9576f7f 100644
--- a/std/algebra/sw_bls12377/g2.go
+++ b/std/algebra/sw_bls12377/g2.go
@@ -485,30 +485,30 @@ func (P *G2Affine) ScalarMulBase(api frontend.API, s frontend.Variable) *G2Affin
// gm[0] = 3g, gm[1] = 5g, gm[2] = 7g
res.X.Lookup2(api, sBits[1], sBits[2],
fields_bls12377.E2{
- points.G2x0,
- points.G2x1},
+ A0: points.G2x0,
+ A1: points.G2x1},
fields_bls12377.E2{
- points.G2mx0[0],
- points.G2mx1[0]},
+ A0: points.G2mx0[0],
+ A1: points.G2mx1[0]},
fields_bls12377.E2{
- points.G2mx0[1],
- points.G2mx1[1]},
+ A0: points.G2mx0[1],
+ A1: points.G2mx1[1]},
fields_bls12377.E2{
- points.G2mx0[2],
- points.G2mx1[2]})
+ A0: points.G2mx0[2],
+ A1: points.G2mx1[2]})
res.Y.Lookup2(api, sBits[1], sBits[2],
fields_bls12377.E2{
- points.G2y0,
- points.G2y1},
+ A0: points.G2y0,
+ A1: points.G2y1},
fields_bls12377.E2{
- points.G2my0[0],
- points.G2my1[0]},
+ A0: points.G2my0[0],
+ A1: points.G2my1[0]},
fields_bls12377.E2{
- points.G2my0[1],
- points.G2my1[1]},
+ A0: points.G2my0[1],
+ A1: points.G2my1[1]},
fields_bls12377.E2{
- points.G2my0[2],
- points.G2my1[2]})
+ A0: points.G2my0[2],
+ A1: points.G2my1[2]})
for i := 3; i < 253; i++ {
// gm[i] = [2^i]g
@@ -516,18 +516,18 @@ func (P *G2Affine) ScalarMulBase(api frontend.API, s frontend.Variable) *G2Affin
tmp.Y = res.Y
tmp.AddAssign(api, G2Affine{
fields_bls12377.E2{
- points.G2mx0[i],
- points.G2mx1[i]},
+ A0: points.G2mx0[i],
+ A1: points.G2mx1[i]},
fields_bls12377.E2{
- points.G2my0[i],
- points.G2my1[i]}})
+ A0: points.G2my0[i],
+ A1: points.G2my1[i]}})
res.Select(api, sBits[i], tmp, res)
}
// i = 0
tmp.Neg(api, G2Affine{
- fields_bls12377.E2{points.G2x0, points.G2x1},
- fields_bls12377.E2{points.G2y0, points.G2y1}})
+ fields_bls12377.E2{A0: points.G2x0, A1: points.G2x1},
+ fields_bls12377.E2{A0: points.G2y0, A1: points.G2y1}})
tmp.AddAssign(api, res)
res.Select(api, sBits[0], res, tmp)
|
Suggested edit: diff --git a/std/algebra/fields_bls12377/e2.go b/std/algebra/fields_bls12377/e2.go
index ab43cc1c..4e3df6a6 100644
--- a/std/algebra/fields_bls12377/e2.go
+++ b/std/algebra/fields_bls12377/e2.go
@@ -241,7 +241,11 @@ func (e *E2) Select(api frontend.API, b frontend.Variable, r1, r2 E2) *E2 {
return e
}
-// Lookup2 ...
+// Lookup2 implements two-bit lookup. It returns:
+// - r1 if b1=0 and b2=0,
+// - r2 if b1=0 and b2=1,
+// - r3 if b1=1 and b2=0,
+// - r3 if b1=1 and b2=1.
func (e *E2) Lookup2(api frontend.API, b1, b2 frontend.Variable, r1, r2, r3, r4 E2) *E2 {
e.A0 = api.Lookup2(b1, b2, r1.A0, r2.A0, r3.A0, r4.A0)
|
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.
DId some small changes to make linter happy. Otherwise looks good, but I'd would use here also lazy computation of the base multiples for better auditability.
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.
👍
|
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.
Looked a bit more thoroughly. I made some changes:
- compute the tables only once in init. Right now we were computing them every time to the call to
Get...Points()
methods. Now do it in init and return existing table. - refactored method names a bit. The table computation functions are local to the package and didn't see reason to duplicate the naming.
- made the internal methods and types private (CurvePoints, TwistedCurvePoints). The main point being that if the user uses the package then they don't have too many different choices which may confuse them. Actually I think we could even make other methods like
DoubleAndAddStep
etc. private. I do not think anyone would need to use them directly outside of this package. But I didn't want to change unrelated code too much. - instead of hardcoding generator coordinate values, I refer to the values in gnark-crypto. Main goal is to make auditing easier.
Unfortunately changed a lot of code thought with it :( But if looks good then it is good to go from my side.
No that makes sense. We'll merge when tests pass. |
KZG can also benefit from precomputed tables in
ScalarMulBase()
(for both G1 and G2). This PR adds these methods to native-field ecc packages and use them instd/commitments/kzg*
.ScalarMulBase()
forsw_bls12377
(on G1)ScalarMulBase()
forsw_bls12377
(on G2)ScalarMulBase()
inkzg_bls12377
ScalarMulBase()
forsw_bls24315
(on G1)ScalarMulBase()
forsw_bls24315
(on G2)ScalarMulBase()
inkzg_bls24315