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

add parallel g1/g2 msm gnark-crypto impl #217

Merged

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Oct 3, 2024

Implement gnark MultiExp for g1/g2 MSM, using default degree of parallelism.

image image

Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

LGTM.

w.r.t the benchmark results, can you confirm

  1. 24.9.1 is calling eip2537blsG1MultiExp
  2. besu-gnark-single-parallel is calling eip2537blsG1MultiExpParallel but limiting to a single core (by setting nbTasks in the MultExpConfig?) This code isn't in this PR.
  3. besu-gnark-parallel is calling eip2537blsG1MultiExpParallel using the implementation in this PR.

^ IIUC, then should we make (2) a configurable option, i.e. an option to limit to single core (not sure we need to expose a parameter for arbitrary cores?)

@garyschulte
Copy link
Contributor Author

garyschulte commented Oct 4, 2024

^ IIUC, then should we make (2) a configurable option, i.e. an option to limit to single core (not sure we need to expose a parameter for arbitrary cores?)

this seems like a reasonable idea. I think I can add the capability to configure NbTasks in the initialization of the gnark bls lib, and we can plumb an option in besu. I will 👀 at adding that bit of config and if it is as lightweight as I am thinking, will add it in this PR

Confirmed yes on 1, 2 and 3. re: # 2:

diff --git a/gnark/gnark-jni/gnark-eip-2537.go b/gnark/gnark-jni/gnark-eip-2537.go
index fdedf33..5217ee1 100644
--- a/gnark/gnark-jni/gnark-eip-2537.go
+++ b/gnark/gnark-jni/gnark-eip-2537.go
@@ -207,7 +207,7 @@ func eip2537blsG1MultiExpParallel(javaInputBuf, javaOutputBuf, javaErrorBuf *C.c
 
     var affineResult bls12381.G1Affine
     // leave nbTasks unset, allow golang to use available cpu cores as the parallelism limit
-    _, err := affineResult.MultiExp(g1Points, scalars, ecc.MultiExpConfig{})
+    _, err := affineResult.MultiExp(g1Points, scalars, ecc.MultiExpConfig{NbTasks: 1})
     if err != nil {
         copy(errorBuf, err.Error())
         return 1
@@ -377,7 +377,7 @@ func eip2537blsG2MultiExpParallel(javaInputBuf, javaOutputBuf, javaErrorBuf *C.c
 
     var affineResult bls12381.G2Affine
     // leave nbTasks unset, allow golang to use available cpu cores as the parallelism limit
-    _, err := affineResult.MultiExp(g2Points, scalars, ecc.MultiExpConfig{})
+    _, err := affineResult.MultiExp(g2Points, scalars, ecc.MultiExpConfig{NbTasks: 1})
     if err != nil {
         copy(errorBuf, err.Error())
         return 1
@@ -387,10 +387,6 @@ func eip2537blsG2MultiExpParallel(javaInputBuf, javaOutputBuf, javaErrorBuf *C.c
     return nonMontgomeryMarshalG2(&affineResult, javaOutputBuf, errorBuf)
 }

@garyschulte garyschulte merged commit 7581d1e into hyperledger:main Oct 7, 2024
12 checks passed
@garyschulte garyschulte mentioned this pull request Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants