More Nova forward ports (lurk-lang#113)

* feat: Refactor error handling and variable naming - Implement marking of `NovaError` enum as non-exhaustive in `errors.rs`. - Introduce a new error case `InvalidCommitmentKeyLength` in `NovaError` enum that checks the length of the provided commitment key. - Improve code readability by renaming `generators_hint` variable to `ck_hint` in the `commitment_key` function. * test: msm test and refactoring (lurk-lang#254) * test: msm test * refactor: batch invert and error flag * Apply suggestions from @huitseeker's code review --------- Co-authored-by: ashWhiteHat <[email protected]>
porcuquine · Nov 28, 2023 · 5f6f24d · 5f6f24d
1 parent 1445168
commit 5f6f24d
Show file tree

Hide file tree

Showing 6 changed files with 114 additions and 78 deletions.
diff --git a/src/errors.rs b/src/errors.rs
@@ -4,6 +4,7 @@ use thiserror::Error;
 
 /// Errors returned by Nova
 #[derive(Clone, Debug, Eq, PartialEq, Error)]
+#[non_exhaustive]
 pub enum NovaError {
   /// returned if the supplied row or col in (row,col,val) tuple is out of range
   #[error("InvalidIndex")]
@@ -29,6 +30,9 @@ pub enum NovaError {
   /// returned if proof verification fails
   #[error("ProofVerifyError")]
   ProofVerifyError,
+  /// returned if the provided commitment key is not of sufficient length
+  #[error("InvalidCommitmentKeyLength")]
+  InvalidCommitmentKeyLength,
   /// returned if the provided number of steps is zero
   #[error("InvalidNumSteps")]
   InvalidNumSteps,

diff --git a/src/lib.rs b/src/lib.rs
@@ -172,11 +172,12 @@ where
   ///
   /// let circuit1 = TrivialCircuit::<<G1 as Group>::Scalar>::default();
   /// let circuit2 = TrivialCircuit::<<G2 as Group>::Scalar>::default();
-  /// // Only relevant for a SNARK using computational commitments, pass &(|_| 0) otherwise.
-  /// let pp_hint1 = &*SPrime::<G1>::commitment_key_floor();
-  /// let pp_hint2 = &*SPrime::<G2>::commitment_key_floor();
+  /// // Only relevant for a SNARK using computational commitments, pass &(|_| 0)
+  /// // or &*nova_snark::traits::snark::default_commitment_key_hint() otherwise.
+  /// let ck_hint1 = &*SPrime::<G1>::commitment_key_floor();
+  /// let ck_hint2 = &*SPrime::<G2>::commitment_key_floor();
   ///
-  /// let pp = PublicParams::new(&circuit1, &circuit2, pp_hint1, pp_hint2);
+  /// let pp = PublicParams::new(&circuit1, &circuit2, ck_hint1, ck_hint2);
   /// ```
   pub fn new(
     c_primary: &C1,
@@ -542,24 +543,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);
     }
@@ -1033,9 +1033,9 @@ mod tests {
     <G2::Scalar as PrimeField>::Repr: Abomonation,
   {
     // this tests public parameters with a size specifically intended for a spark-compressed SNARK
-    let pp_hint1 = &*SPrime::<G1, E1>::commitment_key_floor();
-    let pp_hint2 = &*SPrime::<G2, E2>::commitment_key_floor();
-    let pp = PublicParams::<G1, G2, T1, T2>::new(circuit1, circuit2, pp_hint1, pp_hint2);
+    let ck_hint1 = &*SPrime::<G1, E1>::commitment_key_floor();
+    let ck_hint2 = &*SPrime::<G2, E2>::commitment_key_floor();
+    let pp = PublicParams::<G1, G2, T1, T2>::new(circuit1, circuit2, ck_hint1, ck_hint2);
 
     let digest_str = pp
       .digest()

diff --git a/src/provider/ipa_pc.rs b/src/provider/ipa_pc.rs
@@ -320,18 +320,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;
       }
 

diff --git a/src/provider/mod.rs b/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>();
+  }
+}
diff --git a/src/r1cs/mod.rs b/src/r1cs/mod.rs
@@ -104,8 +104,8 @@ pub fn commitment_key_size<G: Group>(
 ) -> usize {
   let num_cons = S.num_cons;
   let num_vars = S.num_vars;
-  let generators_hint = commitment_key_floor(S);
-  max(max(num_cons, num_vars), generators_hint)
+  let ck_hint = commitment_key_floor(S);
+  max(max(num_cons, num_vars), ck_hint)
 }
 
 impl<G: Group> R1CSShape<G> {

diff --git a/src/spartan/ppsnark.rs b/src/spartan/ppsnark.rs
@@ -1045,8 +1045,9 @@ where
     S: &R1CSShape<G>,
   ) -> Result<(Self::ProverKey, Self::VerifierKey), NovaError> {
     // check the provided commitment key meets minimal requirements
-    assert!(ck.length() >= Self::commitment_key_floor()(S));
-
+    if ck.length() < Self::commitment_key_floor()(S) {
+      return Err(NovaError::InvalidCommitmentKeyLength);
+    }
     let (pk_ee, vk_ee) = EE::setup(ck);
 
     // pad the R1CS matrices