test: msm test and refactoring

src/lib.rs

@@ -445,24 +445,23 @@ where
     z0_secondary: &[G2::Scalar],
   ) -> Result<(Vec<G1::Scalar>, Vec<G2::Scalar>), NovaError> {
     // number of steps cannot be zero
-    if num_steps == 0 {
-      return Err(NovaError::ProofVerifyError);
-    }
+    let is_num_steps_zero = num_steps == 0;
 
     // check if the provided proof has executed num_steps
-    if self.i != num_steps {
-      return Err(NovaError::ProofVerifyError);
-    }
+    let is_num_steps_not_match = self.i != num_steps;
 
     // check if the initial inputs match
-    if self.z0_primary != z0_primary || self.z0_secondary != z0_secondary {
-      return Err(NovaError::ProofVerifyError);
-    }
+    let is_inputs_not_match = self.z0_primary != z0_primary || self.z0_secondary != z0_secondary;
 
     // check if the (relaxed) R1CS instances have two public outputs
-    if self.l_u_secondary.X.len() != 2
+    let is_instance_has_two_outpus = self.l_u_secondary.X.len() != 2
       || self.r_U_primary.X.len() != 2
-      || self.r_U_secondary.X.len() != 2
+      || self.r_U_secondary.X.len() != 2;
+
+    if is_num_steps_zero
+      || is_num_steps_not_match
+      || is_inputs_not_match
+      || is_instance_has_two_outpus
     {
       return Err(NovaError::ProofVerifyError);
     }

src/provider/ipa_pc.rs

@@ -317,18 +317,17 @@ where
         acc *= v[i];
       }
 
-      // we can compute an inversion only if acc is non-zero
-      if acc == G::Scalar::ZERO {
-        return Err(NovaError::InternalError);
-      }
+      // return error if acc is zero
+      acc = match Option::from(acc.invert()) {
+        Some(inv) => inv,
+        None => return Err(NovaError::InternalError),
+      };
 
       // compute the inverse once for all entries
-      acc = acc.invert().unwrap();
-
       let mut inv = vec![G::Scalar::ZERO; v.len()];
-      for i in 0..v.len() {
-        let tmp = acc * v[v.len() - 1 - i];
-        inv[v.len() - 1 - i] = products[v.len() - 1 - i] * acc;
+      for i in (0..v.len()).rev() {
+        let tmp = acc * v[i];
+        inv[i] = products[i] * acc;
         acc = tmp;
       }
 

src/provider/mod.rs

@@ -19,8 +19,6 @@ use rayon::{current_num_threads, prelude::*};
 /// Native implementation of fast multiexp
 /// Adapted from zcash/halo2
 fn cpu_multiexp_serial<C: CurveAffine>(coeffs: &[C::Scalar], bases: &[C]) -> C::Curve {
-  let coeffs: Vec<_> = coeffs.iter().map(|a| a.to_repr()).collect();
-
   let c = if bases.len() < 4 {
     1
   } else if bases.len() < 32 {
@@ -50,64 +48,57 @@ fn cpu_multiexp_serial<C: CurveAffine>(coeffs: &[C::Scalar], bases: &[C]) -> C::
   }
 
   let segments = (256 / c) + 1;
-  let mut acc = C::Curve::identity();
 
-  for current_segment in (0..segments).rev() {
-    for _ in 0..c {
-      acc = acc.double();
-    }
+  (0..segments)
+    .rev()
+    .fold(C::Curve::identity(), |mut acc, segment| {
+      (0..c).for_each(|_| acc = acc.double());
 
-    #[derive(Clone, Copy)]
-    enum Bucket<C: CurveAffine> {
-      None,
-      Affine(C),
-      Projective(C::Curve),
-    }
+      #[derive(Clone, Copy)]
+      enum Bucket<C: CurveAffine> {
+        None,
+        Affine(C),
+        Projective(C::Curve),
+      }
 
-    impl<C: CurveAffine> Bucket<C> {
-      fn add_assign(&mut self, other: &C) {
-        *self = match *self {
-          Bucket::None => Bucket::Affine(*other),
-          Bucket::Affine(a) => Bucket::Projective(a + *other),
-          Bucket::Projective(mut a) => {
-            a += *other;
-            Bucket::Projective(a)
+      impl<C: CurveAffine> Bucket<C> {
+        fn add_assign(&mut self, other: &C) {
+          *self = match *self {
+            Bucket::None => Bucket::Affine(*other),
+            Bucket::Affine(a) => Bucket::Projective(a + *other),
+            Bucket::Projective(a) => Bucket::Projective(a + other),
           }
         }
-      }
 
-      fn add(self, mut other: C::Curve) -> C::Curve {
-        match self {
-          Bucket::None => other,
-          Bucket::Affine(a) => {
-            other += a;
-            other
+        fn add(self, other: C::Curve) -> C::Curve {
+          match self {
+            Bucket::None => other,
+            Bucket::Affine(a) => other + a,
+            Bucket::Projective(a) => other + a,
           }
-          Bucket::Projective(a) => other + a,
         }
       }
-    }
 
-    let mut buckets: Vec<Bucket<C>> = vec![Bucket::None; (1 << c) - 1];
+      let mut buckets = vec![Bucket::None; (1 << c) - 1];
 
-    for (coeff, base) in coeffs.iter().zip(bases.iter()) {
-      let coeff = get_at::<C::Scalar>(current_segment, c, coeff);
-      if coeff != 0 {
-        buckets[coeff - 1].add_assign(base);
+      for (coeff, base) in coeffs.iter().zip(bases.iter()) {
+        let coeff = get_at::<C::Scalar>(segment, c, &coeff.to_repr());
+        if coeff != 0 {
+          buckets[coeff - 1].add_assign(base);
+        }
       }
-    }
 
-    // Summation by parts
-    // e.g. 3a + 2b + 1c = a +
-    //                    (a) + b +
-    //                    ((a) + b) + c
-    let mut running_sum = C::Curve::identity();
-    for exp in buckets.into_iter().rev() {
-      running_sum = exp.add(running_sum);
-      acc += &running_sum;
-    }
-  }
-  acc
+      // Summation by parts
+      // e.g. 3a + 2b + 1c = a +
+      //                    (a) + b +
+      //                    ((a) + b) + c
+      let mut running_sum = C::Curve::identity();
+      for exp in buckets.into_iter().rev() {
+        running_sum = exp.add(running_sum);
+        acc += &running_sum;
+      }
+      acc
+    })
 }
 
 /// Performs a multi-exponentiation operation without GPU acceleration.
@@ -268,3 +259,44 @@ macro_rules! impl_traits {
     }
   };
 }
+
+#[cfg(test)]
+mod tests {
+  use super::cpu_best_multiexp;
+
+  use crate::provider::{
+    bn256_grumpkin::{bn256, grumpkin},
+    secp_secq::{secp256k1, secq256k1},
+  };
+  use group::{ff::Field, Group};
+  use halo2curves::CurveAffine;
+  use pasta_curves::{pallas, vesta};
+  use rand_core::OsRng;
+
+  fn test_msm_with<F: Field, A: CurveAffine<ScalarExt = F>>() {
+    let n = 8;
+    let coeffs = (0..n).map(|_| F::random(OsRng)).collect::<Vec<_>>();
+    let bases = (0..n)
+      .map(|_| A::from(A::generator() * F::random(OsRng)))
+      .collect::<Vec<_>>();
+    let naive = coeffs
+      .iter()
+      .zip(bases.iter())
+      .fold(A::CurveExt::identity(), |acc, (coeff, base)| {
+        acc + *base * coeff
+      });
+    let msm = cpu_best_multiexp(&coeffs, &bases);
+
+    assert_eq!(naive, msm)
+  }
+
+  #[test]
+  fn test_msm() {
+    test_msm_with::<pallas::Scalar, pallas::Affine>();
+    test_msm_with::<vesta::Scalar, vesta::Affine>();
+    test_msm_with::<bn256::Scalar, bn256::Affine>();
+    test_msm_with::<grumpkin::Scalar, grumpkin::Affine>();
+    test_msm_with::<secp256k1::Scalar, secp256k1::Affine>();
+    test_msm_with::<secq256k1::Scalar, secq256k1::Affine>();
+  }
+}