Refactor MSM: Use `VariableBaseMSM` trait #425

mmagician · 2022-06-03T12:06:39Z

Description

I believe this PR should address the comments from #423 and offer more flexible interface.
closes: #411

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~~ Adapted 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

Pratyush · 2022-06-03T18:18:13Z

ec/src/models/short_weierstrass_jacobian.rs

@@ -170,7 +172,7 @@ impl<P: Parameters> Add<Self> for GroupAffine<P> {
 impl<'a, P: Parameters> AddAssign<&'a Self> for GroupAffine<P> {
    fn add_assign(&mut self, other: &'a Self) {
        let mut s_proj = GroupProjective::from(*self);
-        s_proj.add_assign_mixed(other);
+        ProjectiveCurve::add_assign_mixed(&mut s_proj, other);


Did this break? i.e. does s_proj.add_assign_mixed(other) no longer work?

That's needed for disambiguation, since we have two traits that define that method

Pratyush · 2022-06-03T18:29:34Z

Ah I see that we need to put the double_in_place there, as it's used by the default algorithm. Ideally we don't duplicate these method definitions, since whenever you import the VariableBaseMSM trait, you'll automatically need to do the transformation s.double_in_place() -> ProjectiveCurve::double_in_place(&mut s).

I have a couple of ideas to fix this, but the cleanest way would be to do the refactor from #294 =/

mmaker · 2022-06-09T21:21:53Z

EDITED: @mmagician thanks a lot for the pull request! I have a few edits that are available in https://github.com/mmaker/arkworks-algebra/tree/fix/425 and I think will overall improve the quality of this new API.

I'd also much rather keep fancy unicode out of the documentation and add latex support for properly explaining things. I'll add this in a separate pull reuqest.
What do you think? @Pratyush ?

The default, user-friendly way is to multiply group elements by elements in the scalar field. In an attempt to add some ergonomics, I'm using the same template but for scalars and adding a big fat warning that no checks is performed on the size. The function that acts immediately on the bigints can also be called explicitly if the caller knows that the same scalar vectors will be used multiple times. Note that copying that vector only buys a few ms per MSM of 2^20 elements or more. The extra function `msm_bigint` also allows to perform optimizations on `msm` before the call to `msm_bigint`: instead of normalizing into `BigInt`s with `u64` we can move to `i32` and use half the number of buckets in Pippenger, exploiting group addition, using the representation in buckets as [-w/2 + 1, w/2] instead of [0, w-1]. Use the frontend API `VariableBaseMSM::msm` in tests when possible. Change (again) defaults for VariableBaseMSM not to panic. After asking around, looks like most people assume that there is no bound check (e.g. when committing to a vector or a polynomial). I'm therefore reverting the change on the bound check

mmagician · 2022-06-14T14:18:43Z

@mmaker Thanks a lot, I agree that your edits make the API better! Do you want to make a PR to my branch with these changes?

@Pratyush Sure, duplicated method names aren't ideal, but I think that using fully qualified syntax isn't so bad? While the user has to be more explicit, the code stays easy to read. Ultimately, I agree though that having a Group trait like in #294 would automatically allow VariableBaseMSM to extend Group and inherit the necessary methods.

mmaker · 2022-06-15T16:07:48Z

@mmagician done!

Fix/425

mmaker · 2022-06-15T19:30:46Z

@mmagician thanks! maybe this pull request is not anymore a "Draft"?
@Pratyush maybe you concern can be deferred to #294 ?

mmagician · 2022-06-15T19:38:58Z

I was actually thinking of doing the same for FixedMSM, hence I left it as a draft to get feedback before tackling it.

mmaker · 2022-06-17T08:33:08Z

@mmagician understood -- any chance they could be separated into two different pull requests? I feel like that is another battle and I'm interested in getting this more ergonomic trait available in master sooner than later

mmagician · 2022-06-17T08:37:19Z

@mmaker For sure, as far as I'm concerned there's no issue in separating it. We just need to convince @Pratyush to accept the duplicated method name ;)

Pratyush · 2022-06-21T14:05:08Z

ec/src/models/short_weierstrass_jacobian.rs

+
+    type Scalar = <Self as ProjectiveCurve>::ScalarField;
+
+    fn double_in_place(&mut self) -> &mut Self {


May be let's rename this to _double_in_place and put it behind a #[doc(hidden)], so that we can avoid having to disambiguate everywhere.

Pratyush · 2022-06-21T14:05:19Z

ec/src/models/short_weierstrass_jacobian.rs

+        ProjectiveCurve::double_in_place(self)
+    }
+
+    fn add_assign_mixed(&mut self, other: &Self::MSMBase) {


Ditto for this.

Pratyush · 2022-06-21T14:09:16Z

ec/src/msm/fixed_base.rs

+        C: ProjectiveCurve,
+        I: IntoIterator,
+        I::Item: Borrow<C::ScalarField>,


Does this more generic bound but us much? the IntoIterator I can understand (but we will want to fix it to be conditionally IntoParallelIterator, or call par_bridge() on it.)

However, the Borrow one doesn't seem as useful, IMO

mmmmh, I was trying to emulate @oleganza in curve25519-dalek: https://github.com/dalek-cryptography/curve25519-dalek/blob/main/src/backend/serial/scalar_mul/pippenger.rs.
Looking at https://github.com/search?q=optional_multiscalar_mul&type=code there's a bunch of repos that use .iter().chain() or iter::once (which can avoid copying the scalars if we support Borrow<C::ScalarField>). I'm however happy to move this into a separate pull request, this can be a different discussion.

@mmagician, @Pratyush: with a % git checkout master -- ec/src/msm/fixed_base.rs we are all good with the new VariableBaseMSM API, only the Changelog might need minor edits.

The impl in curve25519-dalek is not parallelized, which is why they're fine with Iterator. In our case, since we have parallelization, and because iterators are lazy, if you do iter().chain(), the chain() call will be evalauted many times. For more complex iterators this can be an issue.

This response is not related to Borrow, is it?
To reply about iterators: we still have to copy them in order to make them bigints

@mmaker So is the suggestion to provide an Iterator-based API for MSMs with Scalars, while providing the standard slice-based API for MSMs with bigints?

Prefix `add_assign_mixed` and `double_in_place` with underscore and hide from documentation.

Co-Authored-By: Michele Orrù <[email protected]>

mmagician · 2022-06-21T14:57:30Z

@mmaker I've cherry-picked your previous commit and also checked out the fixed-base file from master. Thanks for that!

Pratyush · 2022-06-22T07:31:23Z

test-templates/src/msm.rs

        .collect::<Vec<_>>();
    let g = (0..SAMPLES)
        .map(|_| G::Projective::rand(&mut rng))
        .collect::<Vec<_>>();
    let g = <G::Projective as ProjectiveCurve>::batch_normalization_into_affine(&g);

    let naive = naive_var_base_msm(g.as_slice(), v.as_slice());
-    let fast = VariableBase::msm(g.as_slice(), v.as_slice());
+    let fast = <G::Projective as VariableBaseMSM>::msm(g.as_slice(), v.as_slice());


Suggested change

let fast = <G::Projective as VariableBaseMSM>::msm(g.as_slice(), v.as_slice());

let fast = VariableBaseMSM::msm(g.as_slice(), v.as_slice());

Should still work, right?

No, error[E0282]: type annotations needed. We can be explicit about the type of fast, though:

let fast: G::Projective = VariableBaseMSM::msm(g.as_slice(), v.as_slice());

Pratyush · 2022-06-22T07:32:48Z

test-templates/src/msm.rs

@@ -48,7 +54,7 @@ pub fn test_chunked_pippenger<G: AffineCurve>() {
        .collect::<Vec<_>>();
    let g = <G::Projective as ProjectiveCurve>::batch_normalization_into_affine(&g);

-    let arkworks = VariableBase::msm(g.as_slice(), v.as_slice());
+    let arkworks = <G::Projective as VariableBaseMSM>::msm_bigint(g.as_slice(), v.as_slice());


Suggested change

let arkworks = <G::Projective as VariableBaseMSM>::msm_bigint(g.as_slice(), v.as_slice());

let arkworks = VariableBaseMSM::msm_bigint(g.as_slice(), v.as_slice());

Pratyush · 2022-06-22T07:33:15Z

test-templates/src/msm.rs

@@ -76,7 +86,7 @@ pub fn test_hashmap_pippenger<G: AffineCurve>() {
        .collect::<Vec<_>>();
    let g = <G::Projective as ProjectiveCurve>::batch_normalization_into_affine(&g);

-    let arkworks = VariableBase::msm(g.as_slice(), v.as_slice());
+    let arkworks = <G::Projective as VariableBaseMSM>::msm_bigint(g.as_slice(), v.as_slice());


Suggested change

let arkworks = <G::Projective as VariableBaseMSM>::msm_bigint(g.as_slice(), v.as_slice());

let arkworks = VariableBaseMSM::msm_bigint(g.as_slice(), v.as_slice());

mmaker · 2022-06-23T17:12:29Z

On Thu 23 Jun 2022 at 10:11, Pratyush Mishra ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ec/src/msm/fixed_base.rs <#425 (comment)>: > + C: ProjectiveCurve, + I: IntoIterator, + I::Item: Borrow<C::ScalarField>, @mmaker <https://github.com/mmaker> So is the suggestion to provide an Iterator-based API for MSMs with Scalars, while providing the standard slice-based API for MSMs with bigints?

In a glorious future yes

…

— Reply to this email directly, view it on GitHub <#425 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACD7BIPU635MOIGQ4U2WATVQSLDBANCNFSM5XYNYCBQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

CHANGELOG.md

mmaker

The documentation has now wrong links:

  --> ec/src/msm/variable_base/mod.rs:39:24
   |
39 |     /// Multiply the [`ScalarField::BigInt`] elements in `scalars` with the
   |                        ^^^^^^^^^^^^^^^^^^^ no item named `ScalarField` in scope
   |
   = note: `#[warn(rustdoc::broken_intra_doc_links)]` on by default

warning: unresolved link to `VariableBase::msm`
  --> ec/src/msm/variable_base/mod.rs:60:22
   |
60 |     /// Reference: [`VariableBase::msm`]
   |                      ^^^^^^^^^^^^^^^^^ no item named `VariableBase` in scope

@mmagician I fixed those in mmaker@42e733a or mmagician@42e733a.

Co-authored-by: Michele Orrù <[email protected]>

mmaker · 2022-06-28T15:09:26Z

@mmagician can you remove FixedBase from the title of the pull request?

mmagician added 10 commits June 3, 2022 12:39

variable msm core fn refactored

f1d8c46

Adapt msm_chunks and checked_length over generics

21e694c

impl VariableBaseMSM for SW Projective

ad0f704

disambiguate calls

97f9214

Restrict base and self of VariableBaseMSM to Copy

1408ca6

Adapt Pippenger MSM code to use new trait

58fee09

Adapt msm tests to use new trait

f46cbfd

shorten the type assignment

5e3ad28

fix formatting

6c092a7

Update CHANGELOG.md

574ca9c

mmagician requested a review from Pratyush June 3, 2022 12:06

Update PR number in CHANGELOG

bc30532

Pratyush reviewed Jun 3, 2022

View reviewed changes

mmaker added 2 commits June 10, 2022 17:49

Use generic iterator for FixedBaseMSM.

288667f

mmaker mentioned this pull request Jun 14, 2022

Fix/425 mmagician/algebra#14

Merged

Merge pull request #14 from mmaker/fix/425

8cced93

Fix/425

mmagician marked this pull request as ready for review June 17, 2022 08:38

Pratyush reviewed Jun 21, 2022

View reviewed changes

mmaker added a commit to mmaker/arkworks-algebra that referenced this pull request Jun 21, 2022

Address Pratyush's comments for arkworks-rs#425 in VariableBaseMSM.

0ac06a2

Prefix `add_assign_mixed` and `double_in_place` with underscore and hide from documentation.

mmaker and others added 4 commits June 21, 2022 16:51

Address Pratyush's comments for arkworks-rs#425 in VariableBaseMSM.

27b9e15

Prefix `add_assign_mixed` and `double_in_place` with underscore and hide from documentation.

Fix formatting

73065ac

Revert fixed_base changes for now

c02bd9f

Co-Authored-By: Michele Orrù <[email protected]>

Merge branch 'master' into msm-refactor-2

3a5075a

mmagician requested a review from Pratyush June 21, 2022 15:05

Pratyush reviewed Jun 22, 2022

View reviewed changes

mmaker approved these changes Jun 24, 2022

View reviewed changes

mmaker reviewed Jun 24, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mmaker requested changes Jun 24, 2022

View reviewed changes

mmagician and others added 4 commits June 27, 2022 12:18

Update CHANGELOG.md

996a69a

Co-authored-by: Michele Orrù <[email protected]>

Merge branch 'master' into msm-refactor-2

82cf15f

Fix documentation for VariableBaseMSM.

f11be6f

Merge branch 'master' into msm-refactor-2

74c95c2

mmaker approved these changes Jun 28, 2022

View reviewed changes

mmagician changed the title ~~Refactor MSM: Use {Fixed,Variable}BaseMSM traits~~ Refactor MSM: Use VariableBaseMSM trait Jun 28, 2022

Pratyush approved these changes Jun 30, 2022

View reviewed changes

Pratyush merged commit 2c5c803 into arkworks-rs:master Jun 30, 2022

mmagician mentioned this pull request Jul 1, 2022

Use the VariableBaseMSM trait for benches arkworks-rs/curves#105

Closed

6 tasks

mmagician deleted the msm-refactor-2 branch December 9, 2022 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MSM: Use `VariableBaseMSM` trait #425

Refactor MSM: Use `VariableBaseMSM` trait #425

mmagician commented Jun 3, 2022

Pratyush Jun 3, 2022

mmagician Jun 3, 2022

Pratyush commented Jun 3, 2022

mmaker commented Jun 9, 2022 •

edited

Loading

mmagician commented Jun 14, 2022

mmaker commented Jun 15, 2022

mmaker commented Jun 15, 2022

mmagician commented Jun 15, 2022

mmaker commented Jun 17, 2022 •

edited

Loading

mmagician commented Jun 17, 2022

Pratyush Jun 21, 2022

Pratyush Jun 21, 2022

Pratyush Jun 21, 2022

mmaker Jun 21, 2022 •

edited

Loading

mmaker Jun 21, 2022

Pratyush Jun 22, 2022

mmaker Jun 22, 2022 •

edited

Loading

Pratyush Jun 23, 2022

mmagician commented Jun 21, 2022

Pratyush Jun 22, 2022

mmagician Jun 22, 2022

Pratyush Jun 22, 2022

Pratyush Jun 22, 2022

mmaker commented Jun 23, 2022 via email

mmaker left a comment •

edited

Loading

mmaker commented Jun 28, 2022


		type Scalar = <Self as ProjectiveCurve>::ScalarField;

		fn double_in_place(&mut self) -> &mut Self {

	let fast = <G::Projective as VariableBaseMSM>::msm(g.as_slice(), v.as_slice());
	let fast = VariableBaseMSM::msm(g.as_slice(), v.as_slice());

	let arkworks = <G::Projective as VariableBaseMSM>::msm_bigint(g.as_slice(), v.as_slice());
	let arkworks = VariableBaseMSM::msm_bigint(g.as_slice(), v.as_slice());

Refactor MSM: Use VariableBaseMSM trait #425

Refactor MSM: Use VariableBaseMSM trait #425

Conversation

mmagician commented Jun 3, 2022

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pratyush commented Jun 3, 2022

mmaker commented Jun 9, 2022 • edited Loading

mmagician commented Jun 14, 2022

mmaker commented Jun 15, 2022

mmaker commented Jun 15, 2022

mmagician commented Jun 15, 2022

mmaker commented Jun 17, 2022 • edited Loading

mmagician commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmaker Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmaker Jun 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmagician commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmaker commented Jun 23, 2022 via email

mmaker left a comment • edited Loading

Choose a reason for hiding this comment

mmaker commented Jun 28, 2022

Refactor MSM: Use `VariableBaseMSM` trait #425

Refactor MSM: Use `VariableBaseMSM` trait #425

mmaker commented Jun 9, 2022 •

edited

Loading

mmaker commented Jun 17, 2022 •

edited

Loading

mmaker Jun 21, 2022 •

edited

Loading

mmaker Jun 22, 2022 •

edited

Loading

mmaker left a comment •

edited

Loading