Refactor MSM: Use VariableBaseMSM trait

CHANGELOG.md

@@ -32,7 +32,8 @@
 - [\#393](https://github.com/arkworks-rs/algebra/pull/393) (`ark-ec`, `ark-ff`) Rename `FpXParams` to `FpXConfig` and `FpXParamsWrapper` to `FpXConfigWrapper`.
 - [\#396](https://github.com/arkworks-rs/algebra/pull/396) (`ark-ec`) Remove `mul_bits` feature, and remove default implementations of `mul` and `mul_by_cofactor_to_projective`.
 - [\#408](https://github.com/arkworks-rs/algebra/pull/408) (`ark-ff`) Change the output of `Display` formatting for BigInt & Fp from hex to decimal.
-- [\#412](https://github.com/arkworks-rs/algebra/pull/412) (`ark-poly`) rename UV/MVPolynomial to DenseUV/MVPolynomial
+- [\#412](https://github.com/arkworks-rs/algebra/pull/412) (`ark-poly`) Rename UV/MVPolynomial to DenseUV/MVPolynomial.
+- [\#425](https://github.com/arkworks-rs/algebra/pull/425) (`ark-ec`) Refactor `{FixedBase,VariableBase}` structs to `{Fixed,Variable}BaseMSM` traits and implement these for `GroupProjective`.
 
 ### Features
 

ec/src/models/short_weierstrass_jacobian.rs

@@ -16,7 +16,9 @@ use ark_ff::{
     ToConstraintField, UniformRand,
 };
 
-use crate::{models::SWModelParameters as Parameters, AffineCurve, ProjectiveCurve};
+use crate::{
+    models::SWModelParameters as Parameters, msm::VariableBaseMSM, AffineCurve, ProjectiveCurve,
+};
 
 use num_traits::{One, Zero};
 use zeroize::Zeroize;
@@ -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);
         *self = s_proj.into();
     }
 }
@@ -561,7 +563,7 @@ impl<P: Parameters> ProjectiveCurve for GroupProjective<P> {
 
         if self.x == u2 && self.y == s2 {
             // The two points are equal, so we double.
-            self.double_in_place();
+            ProjectiveCurve::double_in_place(self);
         } else {
             // If we're adding -a and a together, self.z becomes zero as H becomes zero.
 
@@ -670,7 +672,7 @@ impl<'a, P: Parameters> AddAssign<&'a Self> for GroupProjective<P> {
 
         if u1 == u2 && s1 == s2 {
             // The two points are equal, so we double.
-            self.double_in_place();
+            ProjectiveCurve::double_in_place(self);
         } else {
             // If we're adding -a and a together, self.z becomes zero as H becomes zero.
 
@@ -911,3 +913,17 @@ where
         GroupAffine::from(*self).to_field_elements()
     }
 }
+
+impl<P: Parameters> VariableBaseMSM for GroupProjective<P> {
+    type MSMBase = GroupAffine<P>;
+
+    type Scalar = <Self as ProjectiveCurve>::ScalarField;
+
+    fn double_in_place(&mut self) -> &mut Self {
+        ProjectiveCurve::double_in_place(self)
+    }
+
+    fn add_assign_mixed(&mut self, other: &Self::MSMBase) {
+        ProjectiveCurve::add_assign_mixed(self, other)
+    }
+}

ec/src/msm/fixed_base.rs

@@ -1,6 +1,6 @@
 use crate::{AffineCurve, ProjectiveCurve};
 use ark_ff::{BigInteger, PrimeField};
-use ark_std::{cfg_iter, cfg_iter_mut, vec::Vec};
+use ark_std::{borrow::Borrow, cfg_iter, cfg_iter_mut, vec::Vec};
 
 #[cfg(feature = "parallel")]
 use rayon::prelude::*;
@@ -81,17 +81,17 @@ impl FixedBase {
 
     // TODO use const-generics for the scalar size and window
     // TODO use iterators of iterators of T::Affine instead of taking owned Vec
-    pub fn msm<T: ProjectiveCurve>(
-        scalar_size: usize,
-        window: usize,
-        table: &[Vec<T::Affine>],
-        v: &[T::ScalarField],
-    ) -> Vec<T> {
+    pub fn msm<C, I>(scalar_size: usize, window: usize, table: &[Vec<C::Affine>], v: I) -> Vec<C>
+    where
+        C: ProjectiveCurve,
+        I: IntoIterator,
+        I::Item: Borrow<C::ScalarField>,
+    {
         let outerc = (scalar_size + window - 1) / window;
         assert!(outerc <= table.len());
 
-        cfg_iter!(v)
-            .map(|e| Self::windowed_mul::<T>(outerc, window, table, e))
+        cfg_into_iter!(v)
+            .map(|e| Self::windowed_mul::<C>(outerc, window, table, e.borrow()))
             .collect::<Vec<_>>()
     }
 }

ec/src/msm/variable_base/mod.rs

@@ -1,43 +1,74 @@
 use ark_ff::{prelude::*, PrimeField};
-use ark_std::{borrow::Borrow, iterable::Iterable, ops::AddAssign, vec::Vec};
-
-use crate::{AffineCurve, ProjectiveCurve};
+use ark_std::{
+    borrow::Borrow,
+    iterable::Iterable,
+    ops::{Add, AddAssign},
+    vec::Vec,
+};
 
 #[cfg(feature = "parallel")]
 use rayon::prelude::*;
 
 pub mod stream_pippenger;
 pub use stream_pippenger::*;
 
-pub struct VariableBase;
+pub trait VariableBaseMSM:
+    Eq
+    + Sized
+    + Sync
+    + Zero
+    + Clone
+    + Copy
+    + Send
+    + AddAssign<Self>
+    + for<'a> AddAssign<&'a Self>
+    + for<'a> Add<&'a Self, Output = Self>
+{
+    type MSMBase: Sync + Copy;
+
+    type Scalar: PrimeField;
+
+    fn double_in_place(&mut self) -> &mut Self;
+
+    fn add_assign_mixed(&mut self, other: &Self::MSMBase);
 
-impl VariableBase {
     /// Optimized implementation of multi-scalar multiplication.
     ///
-    /// Will return `None` if `bases` and `scalar` have different lengths.
+    /// Multiply the [`ScalarField::BigInt`] elements in `scalars` with the
+    /// respective group elements in `bases` and sum the resulting set.
     ///
-    /// Reference: [`VariableBase::msm`]
-    pub fn msm_checked_len<G: AffineCurve>(
-        bases: &[G],
-        scalars: &[<G::ScalarField as PrimeField>::BigInt],
-    ) -> Option<G::Projective> {
-        (bases.len() == scalars.len()).then(|| Self::msm(bases, scalars))
+    /// <section class="warning">
+    ///
+    /// If the elements have different length, it will chop the slices to the
+    /// shortest length between `scalars.len()` and `bases.len()`.
+    ///
+    /// </section>
+    fn msm(bases: &[Self::MSMBase], scalars: &[Self::Scalar]) -> Self {
+        let bigints = cfg_into_iter!(scalars)
+            .map(|s| s.into_bigint())
+            .collect::<Vec<_>>();
+        Self::msm_bigint(bases, &bigints)
     }
 
-    /// Optimized implementation of multi-scalar multiplication.
+    /// Optimized implementation of multi-scalar multiplication, that checks bounds.
     ///
-    /// Will multiply the tuples of the diagonal product of `bases × scalars`
-    /// and sum the resulting set. Will iterate only for the elements of the
-    /// smallest of the two sets, ignoring the remaining elements of the biggest
-    /// set.
+    /// Performs `Self::msm`, checking that `bases` and `scalars` have the same length.
+    /// If the length are not equal, returns an error containing the shortest legth over which msm can be performed.
     ///
-    /// ∑i (Bi · Si)
-    pub fn msm<G: AffineCurve>(
-        bases: &[G],
-        scalars: &[<G::ScalarField as PrimeField>::BigInt],
-    ) -> G::Projective {
-        let size = ark_std::cmp::min(bases.len(), scalars.len());
-        let scalars = &scalars[..size];
+    /// Reference: [`VariableBase::msm`]
+    fn msm_checked(bases: &[Self::MSMBase], scalars: &[Self::Scalar]) -> Result<Self, usize> {
+        (bases.len() == scalars.len())
+            .then(|| Self::msm(bases, scalars))
+            .ok_or(usize::min(bases.len(), scalars.len()))
+    }
+
+    /// Optimized implementation of multi-scalar multiplication.
+    fn msm_bigint(
+        bases: &[Self::MSMBase],
+        bigints: &[<Self::Scalar as PrimeField>::BigInt],
+    ) -> Self {
+        let size = ark_std::cmp::min(bases.len(), bigints.len());
+        let scalars = &bigints[..size];
         let bases = &bases[..size];
         let scalars_and_bases_iter = scalars.iter().zip(bases).filter(|(s, _)| !s.is_zero());
 
@@ -47,10 +78,10 @@ impl VariableBase {
             super::ln_without_floats(size) + 2
         };
 
-        let num_bits = G::ScalarField::MODULUS_BIT_SIZE as usize;
-        let fr_one = G::ScalarField::one().into_bigint();
+        let num_bits = Self::Scalar::MODULUS_BIT_SIZE as usize;
+        let fr_one = Self::Scalar::one().into_bigint();
 
-        let zero = G::Projective::zero();
+        let zero = Self::zero();
         let window_starts: Vec<_> = (0..num_bits).step_by(c).collect();
 
         // Each window is of size `c`.
@@ -102,7 +133,7 @@ impl VariableBase {
 
                 // `running_sum` = sum_{j in i..num_buckets} bucket[j],
                 // where we iterate backward from i = num_buckets to 0.
-                let mut running_sum = G::Projective::zero();
+                let mut running_sum = Self::zero();
                 buckets.into_iter().rev().for_each(|b| {
                     running_sum += &b;
                     res += &running_sum;
@@ -129,14 +160,12 @@ impl VariableBase {
     }
     /// Streaming multi-scalar multiplication algorithm with hard-coded chunk
     /// size.
-    pub fn msm_chunks<G, F, I: ?Sized, J>(bases_stream: &J, scalars_stream: &I) -> G::Projective
+    fn msm_chunks<I: ?Sized, J>(bases_stream: &J, scalars_stream: &I) -> Self
     where
-        G: AffineCurve<ScalarField = F>,
         I: Iterable,
-        F: PrimeField,
-        I::Item: Borrow<F>,
+        I::Item: Borrow<Self::Scalar>,
         J: Iterable,
-        J::Item: Borrow<G>,
+        J::Item: Borrow<Self::MSMBase>,
     {
         assert!(scalars_stream.len() <= bases_stream.len());
 
@@ -149,7 +178,7 @@ impl VariableBase {
         // See <https://github.com/rust-lang/rust/issues/77404>
         let mut bases = bases_init.skip(bases_stream.len() - scalars_stream.len());
         let step: usize = 1 << 20;
-        let mut result = G::Projective::zero();
+        let mut result = Self::zero();
         for _ in 0..(scalars_stream.len() + step - 1) / step {
             let bases_step = (&mut bases)
                 .take(step)
@@ -159,7 +188,7 @@ impl VariableBase {
                 .take(step)
                 .map(|s| s.borrow().into_bigint())
                 .collect::<Vec<_>>();
-            result.add_assign(crate::msm::VariableBase::msm(
+            result.add_assign(Self::msm_bigint(
                 bases_step.as_slice(),
                 scalars_step.as_slice(),
             ));

ec/src/msm/variable_base/stream_pippenger.rs

@@ -5,6 +5,8 @@ use ark_ff::{PrimeField, Zero};
 use ark_std::{borrow::Borrow, ops::AddAssign, vec::Vec};
 use hashbrown::HashMap;
 
+use super::VariableBaseMSM;
+
 /// Struct for the chunked Pippenger algorithm.
 pub struct ChunkedPippenger<G: AffineCurve> {
     scalars_buffer: Vec<<G::ScalarField as PrimeField>::BigInt>,
@@ -13,7 +15,11 @@ pub struct ChunkedPippenger<G: AffineCurve> {
     buf_size: usize,
 }
 
-impl<G: AffineCurve> ChunkedPippenger<G> {
+impl<G> ChunkedPippenger<G>
+where
+    G: AffineCurve,
+    G::Projective: VariableBaseMSM<MSMBase = G, Scalar = G::ScalarField>,
+{
     /// Initialize a chunked Pippenger instance with default parameters.
     pub fn new(max_msm_buffer: usize) -> Self {
         Self {
@@ -44,10 +50,11 @@ impl<G: AffineCurve> ChunkedPippenger<G> {
         self.scalars_buffer.push(*scalar.borrow());
         self.bases_buffer.push(*base.borrow());
         if self.scalars_buffer.len() == self.buf_size {
-            self.result.add_assign(crate::msm::VariableBase::msm(
-                self.bases_buffer.as_slice(),
-                self.scalars_buffer.as_slice(),
-            ));
+            self.result
+                .add_assign(<G::Projective as VariableBaseMSM>::msm_bigint(
+                    self.bases_buffer.as_slice(),
+                    self.scalars_buffer.as_slice(),
+                ));
             self.scalars_buffer.clear();
             self.bases_buffer.clear();
         }
@@ -57,10 +64,11 @@ impl<G: AffineCurve> ChunkedPippenger<G> {
     #[inline(always)]
     pub fn finalize(mut self) -> G::Projective {
         if !self.scalars_buffer.is_empty() {
-            self.result.add_assign(crate::msm::VariableBase::msm(
-                self.bases_buffer.as_slice(),
-                self.scalars_buffer.as_slice(),
-            ));
+            self.result
+                .add_assign(<G::Projective as VariableBaseMSM>::msm_bigint(
+                    self.bases_buffer.as_slice(),
+                    self.scalars_buffer.as_slice(),
+                ));
         }
         self.result
     }
@@ -73,7 +81,11 @@ pub struct HashMapPippenger<G: AffineCurve> {
     buf_size: usize,
 }
 
-impl<G: AffineCurve> HashMapPippenger<G> {
+impl<G> HashMapPippenger<G>
+where
+    G: AffineCurve,
+    G::Projective: VariableBaseMSM<MSMBase = G, Scalar = G::ScalarField>,
+{
     /// Produce a new hash map with the maximum msm buffer size.
     pub fn new(max_msm_buffer: usize) -> Self {
         Self {
@@ -104,9 +116,7 @@ impl<G: AffineCurve> HashMapPippenger<G> {
                 .map(|s| s.into_bigint())
                 .collect::<Vec<_>>();
             self.result
-                .add_assign(crate::msm::variable_base::VariableBase::msm(
-                    &bases, &scalars,
-                ));
+                .add_assign(G::Projective::msm_bigint(&bases, &scalars));
             self.buffer.clear();
         }
     }
@@ -123,9 +133,7 @@ impl<G: AffineCurve> HashMapPippenger<G> {
                 .collect::<Vec<_>>();
 
             self.result
-                .add_assign(crate::msm::variable_base::VariableBase::msm(
-                    &bases, &scalars,
-                ));
+                .add_assign(G::Projective::msm_bigint(&bases, &scalars));
         }
         self.result
     }