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: Pairing on BN254 using direct Fp12 extension and non-native Eval() #1339

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

yelhousni
Copy link
Contributor

@yelhousni yelhousni commented Dec 9, 2024

Description

Similar to #1312 but for BN254.
This PR refactor methods in fields_bn254 and sw_bn254 using a direct Fp12 extension and non-native Eval() introduced in #1299.

TODO:

  • Granger-Scott's cyclotomic squarings.
  • Karabina's cyclotomic squarings.
  • Torus-based arithmetic over direct Fp6 extension.

Type of change

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

How has this been tested?

  • New tests corresponding to new methods.
  • PairingCheck and FinalExpCheck tests against gnark-crypto.

How has this been benchmarked?

Some circuits proved in a in a BN254-PLONK:

circuit old (scs) new (scs) diff (scs)
e(a,b) 3,534,755 2,030,447 1,504,308
e(a,b) * e(c,d) == 1 3,879,428 2,239,682 1,639,746
ECPAIR-2 precompile* 4,407,402 2,425,399 1,982,003
ECPAIR-4 precompile* 7,815,376 4,181,633 3,633,743

*ECPAIR precompiles include G2 subgroup membership.

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 requested a review from ivokub December 9, 2024 21:35
@yelhousni yelhousni self-assigned this Dec 9, 2024
@ivokub
Copy link
Collaborator

ivokub commented Dec 10, 2024

Suggested edit:

diff --git a/std/algebra/emulated/sw_bn254/pairing.go b/std/algebra/emulated/sw_bn254/pairing.go
index fdfedafe..d2e95540 100644
--- a/std/algebra/emulated/sw_bn254/pairing.go
+++ b/std/algebra/emulated/sw_bn254/pairing.go
@@ -24,7 +24,6 @@ type Pairing struct {
 	curve  *sw_emulated.Curve[BaseField, ScalarField]
 	g2     *G2
 	bTwist *fields_bn254.E2
-	g2gen  *G2Affine
 }
 
 func NewGTEl(a bn254.GT) GTEl {
@@ -82,15 +81,6 @@ func NewPairing(api frontend.API) (*Pairing, error) {
 	}, nil
 }
 
-func (pr Pairing) generators() *G2Affine {
-	if pr.g2gen == nil {
-		_, _, _, g2gen := bn254.Generators()
-		cg2gen := NewG2AffineFixed(g2gen)
-		pr.g2gen = &cg2gen
-	}
-	return pr.g2gen
-}
-
 // Pair calculates the reduced pairing for a set of points
 // ∏ᵢ e(Pᵢ, Qᵢ).
 //

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.

See the suggested edits for removing deadcode.

Looks good to me, I didn't go through the formulas, but as the tests are functioning, then I think everything is valid. See only comments about removed examples and tests.

One thing I would do is to create isomorphism maps between the tower and direct extension as explicit methods and use them, as imo it makes it a bit clearer to see what extension is used where.

And I think it would be better to wait for #1340 to be merged to ensure that there are no failing tests for edge cases (which I understood you bypassed?)

std/algebra/emulated/sw_bn254/doc_test.go Outdated Show resolved Hide resolved
std/algebra/emulated/sw_bn254/pairing_test.go Outdated Show resolved Hide resolved
std/algebra/emulated/sw_bn254/pairing_test.go Outdated Show resolved Hide resolved
std/algebra/emulated/fields_bn254/e12.go Outdated Show resolved Hide resolved
std/algebra/emulated/fields_bn254/hints.go Show resolved Hide resolved
@yelhousni
Copy link
Contributor Author

yelhousni commented Dec 11, 2024

See the suggested edits for removing deadcode.

Looks good to me, I didn't go through the formulas, but as the tests are functioning, then I think everything is valid. See only comments about removed examples and tests.

One thing I would do is to create isomorphism maps between the tower and direct extension as explicit methods and use them, as imo it makes it a bit clearer to see what extension is used where.

And I think it would be better to wait for #1340 to be merged to ensure that there are no failing tests for edge cases (which I understood you bypassed?)

That makes sense. I pushed a few comments that address your review points. We can wait for #1340.
I'm still thinking whether to implement Karabina and Torus-based direct-Fp6 as it would be only useful for computing the exact value of the pairing, but we only use PairingCheck in our protocols. However torus stuff might come in handy in the future for Ethereum SSLE (ethresearch post).

@ivokub
Copy link
Collaborator

ivokub commented Dec 11, 2024

That makes sense. I pushed a few comments that address your review points. We can wait for #1340. I'm still thinking whether to implement Karabina and Torus-based direct-Fp6 as it would be only useful for computing the exact value of the pairing, but we only use PairingCheck in our protocols. However torus stuff might come in handy in the future for Ethereum SSLE (ethresearch post).

Yup, I agree that with different extensions computing pairing makes less sense and we'd mostly be interested in pairing check (or multi-pairing check). I guess we can keep it currently as is, but have documentation update that it may not be efficient.

@yelhousni
Copy link
Contributor Author

@ivokub PR is ready now!

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.

2 participants