Improvements to error handling and naming (lurk-lang#309)

* rename and introduce checks about length

* introduce a test about public IO

benches/compressed-snark.rs

@@ -68,7 +68,7 @@ fn bench_compressed_snark_internal<S1: RelaxedR1CSSNARKTrait<E1>, S2: RelaxedR1C
   let c_secondary = TrivialCircuit::default();
 
   // Produce public parameters
-  let pp = PublicParams::<E1>::setup(&c_primary, &c_secondary, &*S1::ck_floor(), &*S2::ck_floor());
+  let pp = PublicParams::<E1>::setup(&c_primary, &c_secondary, &*S1::ck_floor(), &*S2::ck_floor()).unwrap();
 
   // Produce prover and verifier keys for CompressedSNARK
   let (pk, vk) = CompressedSNARK::<_, S1, S2>::setup(&pp).unwrap();

benches/recursive-snark.rs

@@ -76,7 +76,7 @@ fn bench_recursive_snark(c: &mut Criterion) {
       &c_secondary,
       &*default_ck_hint(),
       &*default_ck_hint(),
-    );
+    ).unwrap();
 
     // Bench time to produce a recursive SNARK;
     // we execute a certain number of warm-up steps since executing

benches/sha256.rs

@@ -160,7 +160,7 @@ fn bench_recursive_snark(c: &mut Criterion) {
       &ttc,
       &*default_ck_hint(),
       &*default_ck_hint(),
-    );
+    ).unwrap();
 
     let circuit_secondary = TrivialCircuit::default();
     let z0_primary = vec![<E1 as Engine>::Scalar::from(2u64)];

examples/and.rs

@@ -230,7 +230,7 @@ fn main() {
       &circuit_secondary,
       &*S1::ck_floor(),
       &*S2::ck_floor(),
-    );
+    ).unwrap();
     println!("PublicParams::setup, took {:?} ", start.elapsed());
 
     println!(

examples/minroot.rs

@@ -227,7 +227,7 @@ fn main() {
       &circuit_secondary,
       &*S1::ck_floor(),
       &*S2::ck_floor(),
-    );
+    ).unwrap();
     println!("PublicParams::setup, took {:?} ", start.elapsed());
     #[cfg(feature = "abomonate")]
     let pp = {

src/errors.rs

@@ -9,9 +9,10 @@ pub enum NovaError {
   /// returned if the supplied row or col in (row,col,val) tuple is out of range
   #[error("InvalidIndex")]
   InvalidIndex,
-  /// returned if the supplied input is not even-sized
-  #[error("OddInputLength")]
-  OddInputLength,
+  /// returned if the step circuit calls inputize or alloc_io in its synthesize method
+  /// instead of passing output with the return value
+  #[error("InvalidStepCircuitIO")]
+  InvalidStepCircuitIO,
   /// returned if the supplied input is not of the right length
   #[error("InvalidInputLength")]
   InvalidInputLength,
@@ -74,9 +75,9 @@ pub enum NovaError {
 /// Errors specific to the Polynomial commitment scheme
 #[derive(Debug, Eq, PartialEq, Error)]
 pub enum PCSError {
-  /// returned when an invalid inner product argument is provided
-  #[error("InvalidIPA")]
-  InvalidIPA,
+  /// returned when an invalid PCS evaluation argument is provided
+  #[error("InvalidPCS")]
+  InvalidPCS,
   /// returned when there is a Zeromorph error
   #[error("ZMError")]
   ZMError,

src/lib.rs

@@ -241,14 +241,14 @@ where
   /// let ck_hint1 = &*SPrime::<E1>::ck_floor();
   /// let ck_hint2 = &*SPrime::<E2>::ck_floor();
   ///
-  /// let pp = PublicParams::setup(&circuit1, &circuit2, ck_hint1, ck_hint2);
+  /// let pp = PublicParams::setup(&circuit1, &circuit2, ck_hint1, ck_hint2).unwrap();
   /// ```
   pub fn setup<C1: StepCircuit<E1::Scalar>, C2: StepCircuit<<Dual<E1> as Engine>::Scalar>>(
     c_primary: &C1,
     c_secondary: &C2,
     ck_hint1: &CommitmentKeyHint<E1>,
     ck_hint2: &CommitmentKeyHint<Dual<E1>>,
-  ) -> Self {
+  ) -> Result<Self, NovaError> {
     let augmented_circuit_params_primary =
       NovaAugmentedCircuitParams::new(BN_LIMB_WIDTH, BN_N_LIMBS, true);
     let augmented_circuit_params_secondary =
@@ -276,7 +276,6 @@ where
     let _ = circuit_primary.synthesize(&mut cs);
     let (r1cs_shape_primary, ck_primary) = cs.r1cs_shape_and_key(ck_hint1);
     let ck_primary = Arc::new(ck_primary);
-    let circuit_shape_primary = R1CSWithArity::new(r1cs_shape_primary, F_arity_primary);
 
     // Initialize ck for the secondary
     let circuit_secondary: NovaAugmentedCircuit<'_, E1, C2> = NovaAugmentedCircuit::new(
@@ -289,9 +288,15 @@ where
     let _ = circuit_secondary.synthesize(&mut cs);
     let (r1cs_shape_secondary, ck_secondary) = cs.r1cs_shape_and_key(ck_hint2);
     let ck_secondary = Arc::new(ck_secondary);
+
+    if r1cs_shape_primary.num_io != 2 || r1cs_shape_secondary.num_io != 2 {
+      return Err(NovaError::InvalidStepCircuitIO);
+    }
+
+    let circuit_shape_primary = R1CSWithArity::new(r1cs_shape_primary, F_arity_primary);
     let circuit_shape_secondary = R1CSWithArity::new(r1cs_shape_secondary, F_arity_secondary);
 
-    Self {
+    Ok(Self {
       F_arity_primary,
       F_arity_secondary,
       ro_consts_primary,
@@ -305,7 +310,7 @@ where
       augmented_circuit_params_primary,
       augmented_circuit_params_secondary,
       digest: OnceCell::new(),
-    }
+    })
   }
 
   /// Retrieve the digest of the public parameters.
@@ -1095,7 +1100,7 @@ mod tests {
     // this tests public parameters with a size specifically intended for a spark-compressed SNARK
     let ck_hint1 = &*SPrime::<E1, EE1>::ck_floor();
     let ck_hint2 = &*SPrime::<Dual<E1>, EE2>::ck_floor();
-    let pp = PublicParams::<E1>::setup(circuit1, circuit2, ck_hint1, ck_hint2);
+    let pp = PublicParams::<E1>::setup(circuit1, circuit2, ck_hint1, ck_hint2).unwrap();
 
     let digest_str = pp
       .digest()
@@ -1161,7 +1166,7 @@ mod tests {
       &test_circuit2,
       &*default_ck_hint(),
       &*default_ck_hint(),
-    );
+    ).unwrap();
     let num_steps = 1;
 
     // produce a recursive SNARK
@@ -1208,7 +1213,7 @@ mod tests {
       &circuit_secondary,
       &*default_ck_hint(),
       &*default_ck_hint(),
-    );
+    ).unwrap();
 
     let num_steps = 3;
 
@@ -1285,7 +1290,7 @@ mod tests {
       &circuit_secondary,
       &*S1::ck_floor(),
       &*S2::ck_floor(),
-    );
+    ).unwrap();
 
     let num_steps = 3;
 
@@ -1532,7 +1537,7 @@ mod tests {
       &circuit_secondary,
       &*default_ck_hint(),
       &*default_ck_hint(),
-    );
+    ).unwrap();
 
     let num_steps = 3;
 
@@ -1593,7 +1598,7 @@ mod tests {
       &test_circuit2,
       &*default_ck_hint(),
       &*default_ck_hint(),
-    );
+    ).unwrap();
 
     let num_steps = 1;
 
@@ -1633,4 +1638,68 @@ mod tests {
     test_ivc_base_with::<Bn256EngineKZG>();
     test_ivc_base_with::<Secp256k1Engine>();
   }
+
+  fn test_setup_with<E1: CurveCycleEquipped>()
+  {
+    #[derive(Clone, Debug, Default)]
+    struct CircuitWithInputize<F: PrimeField> {
+      _p: PhantomData<F>,
+    }
+
+    impl<F: PrimeField> StepCircuit<F> for CircuitWithInputize<F> {
+      fn arity(&self) -> usize {
+        1
+      }
+
+      fn synthesize<CS: ConstraintSystem<F>>(
+        &self,
+        cs: &mut CS,
+        z: &[AllocatedNum<F>],
+      ) -> Result<Vec<AllocatedNum<F>>, SynthesisError> {
+        let x = &z[0];
+        // a simplified version of this test would only have one input
+        // but beside the Nova Public parameter requirement for a num_io = 2, being
+        // probed in this test, we *also* require num_io to be even, so 
+        // negative testing requires at least 4 inputs
+        let y = x.square(cs.namespace(|| "x_sq"))?;
+        y.inputize(cs.namespace(|| "y"))?; // inputize y
+        let y2 = x.square(cs.namespace(|| "x_sq2"))?;
+        y2.inputize(cs.namespace(|| "y2"))?; // inputize y2
+        let y3 = x.square(cs.namespace(|| "x_sq3"))?;
+        y3.inputize(cs.namespace(|| "y3"))?; // inputize y2
+        let y4 = x.square(cs.namespace(|| "x_sq4"))?;
+        y4.inputize(cs.namespace(|| "y4"))?; // inputize y2
+        Ok(vec![y, y2, y3, y4])
+      }
+    }
+
+    // produce public parameters with trivial secondary
+    let circuit = CircuitWithInputize::<<E1 as Engine>::Scalar>::default();
+    let pp =
+      PublicParams::<E1>::setup(
+        &circuit,
+        &TrivialCircuit::default(),
+        &*default_ck_hint(),
+        &*default_ck_hint(),
+      );
+    assert!(pp.is_err());
+    assert_eq!(pp.err(), Some(NovaError::InvalidStepCircuitIO));
+
+    // produce public parameters with the trivial primary
+    let circuit = CircuitWithInputize::<<Dual<E1> as Engine>::Scalar>::default();
+    let pp =
+      PublicParams::<E1>::setup(
+        &TrivialCircuit::default(),
+        &circuit,
+        &*default_ck_hint(),
+        &*default_ck_hint(),
+      );
+    assert!(pp.is_err());
+    assert_eq!(pp.err(), Some(NovaError::InvalidStepCircuitIO));
+  }
+
+  #[test]
+  fn test_setup() {
+    test_setup_with::<Bn256EngineKZG>();
+  }
 }

src/provider/hyperkzg.rs

@@ -422,9 +422,10 @@ mod tests {
   use super::*;
   use crate::provider::util::test_utils::prove_verify_from_num_vars;
   use crate::{
-    provider::keccak::Keccak256Transcript, traits::commitment::CommitmentTrait, CommitmentKey,
+    provider::keccak::Keccak256Transcript, CommitmentKey,
   };
   use bincode::Options;
+  use expect_test::expect;
 
   type E = halo2curves::bn256::Bn256;
   type NE = crate::provider::Bn256EngineKZG;
@@ -565,15 +566,12 @@ mod tests {
     // same state
     assert_eq!(post_c_p, post_c_v);
 
-    let my_options = bincode::DefaultOptions::new()
+    let proof_bytes = bincode::DefaultOptions::new()
       .with_big_endian()
-      .with_fixint_encoding();
-    let mut output_bytes = my_options.serialize(&vk).unwrap();
-    output_bytes.append(&mut my_options.serialize(&C.compress()).unwrap());
-    output_bytes.append(&mut my_options.serialize(&point).unwrap());
-    output_bytes.append(&mut my_options.serialize(&eval).unwrap());
-    output_bytes.append(&mut my_options.serialize(&proof).unwrap());
-    println!("total output = {} bytes", output_bytes.len());
+      .with_fixint_encoding()
+      .serialize(&proof)
+      .unwrap();
+    expect!["368"].assert_eq(&proof_bytes.len().to_string());
 
     // Change the proof and expect verification to fail
     let mut bad_proof = proof.clone();

src/provider/ipa_pc.rs

@@ -396,7 +396,7 @@ where
     if P_hat == CE::<E>::commit(&ck_hat.combine(&ck_c), &[self.a_hat, self.a_hat * b_hat]) {
       Ok(())
     } else {
-      Err(NovaError::PCSError(PCSError::InvalidIPA))
+      Err(NovaError::PCSError(PCSError::InvalidPCS))
     }
   }
 }

src/r1cs/mod.rs

@@ -145,7 +145,7 @@ impl<E: Engine> R1CSShape<E> {
 
     // We require the number of public inputs/outputs to be even
     if num_io % 2 != 0 {
-      return Err(NovaError::OddInputLength);
+      return Err(NovaError::InvalidStepCircuitIO);
     }
 
     Ok(Self {