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

Perf: KZG in circuit #506

Merged
merged 13 commits into from
Mar 2, 2023
Merged

Perf: KZG in circuit #506

merged 13 commits into from
Mar 2, 2023

Conversation

yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Mar 1, 2023

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 in std/commitments/kzg*.

  • ScalarMulBase() for sw_bls12377 (on G1)
  • ScalarMulBase() for sw_bls12377 (on G2)
  • use ScalarMulBase() in kzg_bls12377
  • ScalarMulBase() for sw_bls24315 (on G1)
  • ScalarMulBase() for sw_bls24315 (on G2)
  • use ScalarMulBase() in kzg_bls24315

@yelhousni yelhousni added the perf label Mar 1, 2023
@yelhousni yelhousni requested a review from ivokub March 1, 2023 12:35
@ivokub
Copy link
Collaborator

ivokub commented Mar 1, 2023

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)
 

@ivokub
Copy link
Collaborator

ivokub commented Mar 1, 2023

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)

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.

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.

std/algebra/sw_bls12377/inner.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.

👍

@yelhousni
Copy link
Contributor Author

  • in-circuit KZG (bls12-377/bw6-761):
    Groth16: 20691 --> 20184
    Plonk: 84645 --> 84147

  • in-circuit KZG (bls24-315/bw6-633):
    Groth16: 57355 --> 43466
    Plonk: 262954 --> 211253

@yelhousni yelhousni requested a review from ivokub March 2, 2023 10:55
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.

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.

@yelhousni
Copy link
Contributor Author

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.

@yelhousni yelhousni merged commit 3149bf5 into develop Mar 2, 2023
@yelhousni yelhousni deleted the perf/kzg-in-circuit branch March 2, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants