Enabling GLV for Bandersnatch #66

zhenfeizhang · 2021-07-03T15:59:08Z

Description

closes: #XXXX

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

This PR enables the GLV algorithm for bandersnatch curve. It improves group operations from 75 us to around 40 us on an AMD 5900x.

Additional todos:

update the dependencies once Add GLVParameters trait definition algebra#301 is approved and merged
~~currently GLV is not the default multiplication algorithm.~~ Make it into the default algorithm ~~(need help: I need a pointer to the code where I can overwrite the multiplicand)~~
impl R1CS for GLV this is a future work, and will not be included in this PR
- here is an implementation of GLV circuit https://github.com/zhenfeizhang/bandersnatch/blob/main/bandersnatch/src/constraints/glv.rs
- will create another PR to for those code once this PR is merged

zhenfeizhang · 2021-09-15T18:36:04Z

thanks for merging arkworks-rs/algebra#301
turns out that the repo still cannot compile due to dependencies hell.
Now that bandersnatch with GLV is depending on a newer version of ark-ec (GitHub version) than the rest of repos in this workspace (version 0.3.0).

One solution is to fix dependency to Github for every repo in this workspace. This is ugly.
The other solution is to bump up ark-ec to 0.3.1 on crates.io, and update the rest of the dependencies accordingly.

weikengchen · 2021-09-15T18:45:00Z

Let me try to find a way.

The common solution I use is to point this new repo to, not the crates.io version, but the GitHub version, so we can examine the PR and likely merge it.

Then, during the next version release (we will improve this part), it would be adjusted accordingly.

weikengchen · 2021-09-15T18:46:38Z

(And although I would be very happy for Translucence to use the arkworks-rs main branch, during this time we are still making adjustment and having the engineers onboard, I have to say that maybe you would be interested in a fork of arkworks...)

zhenfeizhang · 2021-09-15T19:32:52Z

Let me try to find a way.

The common solution I use is to point this new repo to, not the crates.io version, but the GitHub version, so we can examine the PR and likely merge it.

Then, during the next version release (we will improve this part), it would be adjusted accordingly.

That sounds good to me. I can push a change for this

weikengchen · 2021-09-15T19:39:07Z

Let me add a quick clarification: by "pointing to", I mean "patches".

zhenfeizhang · 2021-09-15T19:45:46Z

hmmm... turns out this is not easy. A curve repo depends on ark-r1cs-std which is external and depends on a different version of ark-ec.

an alternative temp solution is to copy the GLVParameter trait from https://github.com/arkworks-rs/algebra/pull/301/files to ed_on_bls12_381_bandersnatch for review purpose, so that ed_on_bls12_381_bandersnatch can depend on current ark-ec 0.3.0. And have the real fix during next release.

WDYT?

(This is none-work related, so it is definitely not a high priority anyway)

Pratyush · 2021-10-18T22:09:53Z

I believe if you use the patch directive (like what is in master), then you can avoid the dependency mismatch

zhenfeizhang · 2021-10-19T17:00:01Z

I believe if you use the patch directive (like what is in master), then you can avoid the dependency mismatch

Thanks! Yes this solved the issue, and CI is passing :-)

One more question:

currently GLV is not the default multiplication algorithm. Make it into the default algorithm (need help: I need a pointer to the code where I can overwrite the multiplicand)

Where should I change the code for this? Or should we leave as it is (i.e., the default multiplication use non-GLV, the caller need to specify GLV to get the speed improvement)

zhenfeizhang · 2021-10-19T17:02:37Z

Also FYI: this branch has rebased based on #76 for consistency

Pratyush · 2021-10-21T20:07:52Z

currently GLV is not the default multiplication algorithm. Make it into the default algorithm (need help: I need a pointer to the code where I can overwrite the multiplicand)

Where should I change the code for this? Or should we leave as it is (i.e., the default multiplication use non-GLV, the caller need to specify GLV to get the speed improvement)

I think the right approach for this is to enable overriding scalar_mul via ModelParameters, like in https://github.com/arkworks-rs/algebra/blob/6e568cdaf154806365823d48d547b3864412a516/ec/src/models/mod.rs#L41

zhenfeizhang · 2021-10-21T22:07:38Z

currently GLV is not the default multiplication algorithm. Make it into the default algorithm (need help: I need a pointer to the code where I can overwrite the multiplicand)

Where should I change the code for this? Or should we leave as it is (i.e., the default multiplication use non-GLV, the caller need to specify GLV to get the speed improvement)

I think the right approach for this is to enable overriding scalar_mul via ModelParameters, like in https://github.com/arkworks-rs/algebra/blob/6e568cdaf154806365823d48d547b3864412a516/ec/src/models/mod.rs#L41

Not sure about it.
mul is defined in ark_ec::short_weierstrass_jacobian::AffineCurve for ark_ec::twisted_edwards_extended::GroupAffine. Both are foreign to this crate. I don't think we can overload it.

I can think of two solutions:

either caller will have to manually call glv_mul
or write glv_mul in generic for ark_ec, similar to AffineCurve crate

WDYT?

Pratyush · 2021-10-21T22:43:21Z

Ah I meant do as follows. In ark-ec, change ModelParameters to have a (defaulted) impl of mul. Then, change AffineCurve::mul to invoke this method. Finally, in bandersnatch::ModelParameters, use the glv_mul method to implement ModelParameters::mul.

Pratyush · 2022-03-08T05:03:04Z

@zhenfeizhang any updates on this?

weikengchen · 2022-03-08T05:11:08Z

We might be able to push the updated code from Espresso's Jellyfish. It is under the MIT license.

Pratyush · 2022-03-08T05:12:03Z

Sounds good, whatever's convenient.

zhenfeizhang · 2022-03-08T15:13:12Z

Ah I meant do as follows. In ark-ec, change ModelParameters to have a (defaulted) impl of mul. Then, change AffineCurve::mul to invoke this method. Finally, in bandersnatch::ModelParameters, use the glv_mul method to implement ModelParameters::mul.

Ok. I implemented the suggested solution.

The current PR is blocked by arkworks-rs/algebra#396
We will need that to be merged for this PR to pass CI.
Will review this PR again for correctness once arkworks-rs/algebra#396 is merged.

zhenfeizhang · 2022-03-08T15:46:56Z

We might be able to push the updated code from Espresso's Jellyfish. It is under the MIT license.

Jellyfish does not really have those code as it does not use R1CS. The code are here
https://github.com/zhenfeizhang/bandersnatch/blob/main/bandersnatch/src/constraints/glv.rs
which is also under MIT.

I will create a separate PR to for glv constraints

ivokub · 2022-03-16T15:56:59Z

Jellyfish does not really have those code as it does not use R1CS. The code are here https://github.com/zhenfeizhang/bandersnatch/blob/main/bandersnatch/src/constraints/glv.rs which is also under MIT.

I will create a separate PR to for glv constraints

I'll reply here before PR for the Bandersnatch gadget is created.
The value of t in the circuit is not range checked - this would allow t * r1 and t * r2 to overflow Fq (comments on lines 219-220). Shouldn't t also be range constrained?

zhenfeizhang · 2022-03-16T18:08:21Z

Jellyfish does not really have those code as it does not use R1CS. The code are here https://github.com/zhenfeizhang/bandersnatch/blob/main/bandersnatch/src/constraints/glv.rs which is also under MIT.
I will create a separate PR to for glv constraints

I'll reply here before PR for the Bandersnatch gadget is created. The value of t in the circuit is not range checked - this would allow t * r1 and t * r2 to overflow Fq (comments on lines 219-220). Shouldn't t also be range constrained?

Not sure that is necessary.

We are proving over ZZ the following eq:

(0) lambda * k2_sign * k2 + s = t * Fr::modulus + k1

regardless the size of t.
Here lambda is public, and ~256 bits; k1 and k2 are both range checked.
Then, if there is a t that satisfies this equation, it must be of ~128 bits

Suppose that both t*r1 and t*r2 wrapped around with some parameters alpha and beta. That is, we obtain, over ZZ the following

(f') tmp1 + 2^128 tmp2 =  lambda_1 * k2_sign * k2 + s - t' * r1 - k1 + alpha * r  // note that tmp1 = 0
(g') tmp2 + lambda_2 * k2_sign * k2   = t' * r2 + beta * r

which is

2^128( t' * r2 + beta * r -  lambda_2 * k2_sign * k2) 
    = lambda_1 * k2_sign * k2 + s - t' * r1 - k1 + alpha * r

That is

(2^128 * r2 + r1) * t' + k1 -  (2^128 * lambda_2 + lambda_1)  * k2_sign * k2  + (2^128 beta -alpha) r = 0

which is

t' * Fr::modulus + k1 + (2^128 beta -alpha) r  = lambda * k2_sign * k2 + s

That means t' + 2^128 beta -alpha = t and (0) remains correct.

Pratyush · 2022-03-18T02:40:04Z

ed_on_bls12_381/src/curves/mod.rs

@@ -47,7 +47,7 @@ pub type SWProjective = SWGroupProjective<JubjubParameters>;
 ///
 /// A = 52296097456646850916096512823759002727550416093741407922227928430486925478210
 /// B = 48351165704696163914533707656614864561753505123260775585269522553028192119009
-///
+


Suggested change

///

Pratyush · 2022-03-18T02:41:52Z

ed_on_bls12_381_bandersnatch/src/curves/glv.rs

+// Here we need to implement a customized MSM algorithm, since we know that
+// the high bits of Fr are restricted to be small, i.e. ~ 128 bits.
+// This MSM will save us some 128 doublings.
+pub(crate) fn multi_scalar_mul(


could you provide a better name for this? What does this method do?

Pratyush · 2022-03-18T02:43:51Z

@zhenfeizhang is there a way to derive the GLV parameters at compile-time in Rust itself?

zhenfeizhang · 2022-05-16T19:01:58Z

Had to close this one because the target branch (master at my forked branch) is changed.
Recreated PR here: #102

zhenfeizhang force-pushed the master branch 4 times, most recently from f57672f to d906717 Compare October 19, 2021 16:52

zhenfeizhang marked this pull request as ready for review October 19, 2021 17:04

zhenfeizhang mentioned this pull request Mar 8, 2022

add mul to ModelParam traits arkworks-rs/algebra#396

Merged

6 tasks

Pratyush reviewed Mar 18, 2022

View reviewed changes

zhenfeizhang closed this May 16, 2022

zhenfeizhang force-pushed the master branch from 0eb86b4 to 9a89ea8 Compare May 16, 2022 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling GLV for Bandersnatch #66

Enabling GLV for Bandersnatch #66

zhenfeizhang commented Jul 3, 2021 •

edited

Loading

zhenfeizhang commented Sep 15, 2021

weikengchen commented Sep 15, 2021

weikengchen commented Sep 15, 2021

zhenfeizhang commented Sep 15, 2021

weikengchen commented Sep 15, 2021

zhenfeizhang commented Sep 15, 2021

Pratyush commented Oct 18, 2021

zhenfeizhang commented Oct 19, 2021

zhenfeizhang commented Oct 19, 2021

Pratyush commented Oct 21, 2021

zhenfeizhang commented Oct 21, 2021

Pratyush commented Oct 21, 2021

Pratyush commented Mar 8, 2022

weikengchen commented Mar 8, 2022

Pratyush commented Mar 8, 2022

zhenfeizhang commented Mar 8, 2022

zhenfeizhang commented Mar 8, 2022

ivokub commented Mar 16, 2022

zhenfeizhang commented Mar 16, 2022 •

edited

Loading

Pratyush Mar 18, 2022

Pratyush Mar 18, 2022

Pratyush commented Mar 18, 2022

zhenfeizhang commented May 16, 2022 •

edited

Loading

Enabling GLV for Bandersnatch #66

Enabling GLV for Bandersnatch #66

Conversation

zhenfeizhang commented Jul 3, 2021 • edited Loading

Description

zhenfeizhang commented Sep 15, 2021

weikengchen commented Sep 15, 2021

weikengchen commented Sep 15, 2021

zhenfeizhang commented Sep 15, 2021

weikengchen commented Sep 15, 2021

zhenfeizhang commented Sep 15, 2021

Pratyush commented Oct 18, 2021

zhenfeizhang commented Oct 19, 2021

zhenfeizhang commented Oct 19, 2021

Pratyush commented Oct 21, 2021

zhenfeizhang commented Oct 21, 2021

Pratyush commented Oct 21, 2021

Pratyush commented Mar 8, 2022

weikengchen commented Mar 8, 2022

Pratyush commented Mar 8, 2022

zhenfeizhang commented Mar 8, 2022

zhenfeizhang commented Mar 8, 2022

ivokub commented Mar 16, 2022

zhenfeizhang commented Mar 16, 2022 • edited Loading

Pratyush Mar 18, 2022

Choose a reason for hiding this comment

Pratyush Mar 18, 2022

Choose a reason for hiding this comment

Pratyush commented Mar 18, 2022

zhenfeizhang commented May 16, 2022 • edited Loading

zhenfeizhang commented Jul 3, 2021 •

edited

Loading

zhenfeizhang commented Mar 16, 2022 •

edited

Loading

zhenfeizhang commented May 16, 2022 •

edited

Loading