Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

GLV implementation for BLS12_377, BLS12_381 and BN254 #158

Merged
merged 47 commits into from
Sep 11, 2023
Merged

Conversation

mmagician
Copy link
Member

@mmagician mmagician commented Apr 27, 2023

Description

Updates #147 by @simonmasson to use the slightly improved trait interface from arkworks-rs/algebra#644.

Update:
Benchmarks show that there is a significant improvement for BLS12-377, BLS12-381 (40%, 37% respectively), moderate improvement for BN254 (10%) and a small improvement for Bandersnatch at 2%. The rest perform worse with GLV enabled by default. For the last group I've left the implementation of the GLV trait in, but haven't enabled it by default.
PR title reflects the defaults only.

closes: #147


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer

@mmagician mmagician changed the title GLV implementation for BLS12_377, BLS12_381, BN254, BW6_761, Bandersnatch, Pallas and Vesta, v2 GLV implementation for BLS12_377, BLS12_381, BN254 and Bandersnatch Sep 3, 2023
@@ -119,21 +120,112 @@ const TE_GENERATOR_Y: Fq =

/// x coordinate for SW curve generator
const SW_GENERATOR_X: Fq =
MontFp!("30900340493481298850216505686589334086208278925799850409469406976849338430199");
MontFp!("4732093294267640299242820317528400560681136891967543338160850811774078125840");
Copy link
Member

Choose a reason for hiding this comment

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

Why did these generators change?

const COEFF_A: Self::BaseField =
MontFp!("10773120815616481058602537765553212789256758185246796157495669123169359657269");
MontFp!("52435875175126190479447740508185965837690552500527637822603658699934817984513");
Copy link
Member

Choose a reason for hiding this comment

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

How did the curve coefficients change? That seems fishy.

Regardless, we can use the original short values here, namely:

Suggested change
MontFp!("52435875175126190479447740508185965837690552500527637822603658699934817984513");
MontFp!("−3763200000");

Copy link
Contributor

@simonmasson simonmasson Sep 12, 2023

Choose a reason for hiding this comment

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

There were changes on the coefficients in order to optimize slightly one computation.
We should use these new coefficients (matching https://eprint.iacr.org/2021/1152.pdf).
Note that the two curves are isomorphic (same order, etc.), as this SageMath script shows:

p = 0x73eda753299d7d483339d80809a1d80553bda402fffe5bfeffffffff00000001

Fp = GF(p)

HD = hilbert_class_polynomial(-8)

# new coefficients from the commit, can be written using negative numbers
A1 = 52435875175126190479447740508185965837690552500527637822603658699934817984513
B1 = 52435875175126190479447740508185965837690552500527637822603658621262613184513

# previous coefficients from the commit
A2 = 10773120815616481058602537765553212789256758185246796157495669123169359657269
B2 = 29569587568322301171008055308580903175558631321415017492731745847794083609535

E1 = EllipticCurve(Fp, [A1,B1])
E2 = EllipticCurve(Fp, [A2,B2])

assert HD(E1.j_invariant()) == 0
assert HD(E2.j_invariant()) == 0

impl SWCurveConfig for BandersnatchConfig {
/// COEFF_A = 10773120815616481058602537765553212789256758185246796157495669123169359657269
/// COEFF_A = 52435875175126190479447740508185965837690552500527637822603658699934817984513
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// COEFF_A = 52435875175126190479447740508185965837690552500527637822603658699934817984513
/// COEFF_A = -3763200000

const COEFF_B: Self::BaseField =
MontFp!("29569587568322301171008055308580903175558631321415017492731745847794083609535");
MontFp!("52435875175126190479447740508185965837690552500527637822603658621262613184513");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
MontFp!("52435875175126190479447740508185965837690552500527637822603658621262613184513");
MontFp!("-78675968000000");


/// COEFF_B = 29569587568322301171008055308580903175558631321415017492731745847794083609535
/// COEFF_B = 52435875175126190479447740508185965837690552500527637822603658621262613184513
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// COEFF_B = 52435875175126190479447740508185965837690552500527637822603658621262613184513
/// COEFF_B = -78675968000000

let y = p.y;

// 1/(x + 44800)
let tmp1 = FpConfig::inverse(&(x + var44800)).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Hm, inverting in the endomorphism seems to defeat the purpose, no? The endomorphism is supposed to be fast.

Copy link
Member

Choose a reason for hiding this comment

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

I see, the endomorphism is fast in the Edwards form.

Copy link
Member

Choose a reason for hiding this comment

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

Since the endomorphism is slow here, we should just use the normal scalar multiplication.

Copy link
Member

Choose a reason for hiding this comment

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

In fact, could we adapt #102 for this task? That constructs GLV multiplication for the TE form directly, and should get the expected speed up

Copy link
Member

Choose a reason for hiding this comment

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

(We could do that in a follow up PR)

@mmagician
Copy link
Member Author

@Pratyush I'm thinking to just pull out the Bandersnatch changes into a separate PR, as I don't know why a new parameter set is used. Maybe later @simonmasson can add more details on this change, but I think we should move forward with the rest for now, WDYT?

@mmagician
Copy link
Member Author

@asanso maybe you can shed some light on the new parameters as we discussed today?

@Pratyush
Copy link
Member

Pratyush commented Sep 8, 2023

@mmagician Handling Bandersnatch in a different PR seems reasonable!

@mmagician mmagician changed the title GLV implementation for BLS12_377, BLS12_381, BN254 and Bandersnatch GLV implementation for BLS12_377, BLS12_381 and BN254 Sep 9, 2023
@mmagician
Copy link
Member Author

The failing tests for curves with GLV enabled should be fixed when #171 is merged.
As explained there, overriding mul_projective with the new GLV implementation forces the scalars passed to be at most MODULUS (in particular, at most N limbs), and otherwise panics upon conversion from more than N limbs. This is a breaking change, and maybe we should think about a way to either 1) impose the same behaviour for non-GLV implementations 2) change the trait signature to directly receive a Scalar 3) distinguish the two cases here and handle appropriately.

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants