Improve error handling (#286)

* When a function already returns a `Result`, propagate errors instead of panicking with `expect` * For `NovaError::SynthesisError`, retain information about the original bellpepper error. Since `NovaError` implements `Clone` but bellpepper's `SynthesisError` does not, we keep the error information as a `String`. This commit only fixes low-hanging fruit in lib.rs, for functions that already return a `Result` and can easily propagate errors just by replacing `expect(...)` with `?`. There are still many `unwrap()` calls in functions returning `Result` in other modules, particularly gadgets. But I don't understand the code well in those parts, and I suspect some of those `unwrap()`s actually can't fail based on invariants of the code, so it makes perfect sense to leave them as is. Co-authored-by: Francois Garillot <[email protected]>
argumentcomputer · Jan 3, 2024 · 2988796 · 2988796
1 parent 9009b7b
commit 2988796
Show file tree

Hide file tree

Showing 6 changed files with 50 additions and 58 deletions.
diff --git a/src/errors.rs b/src/errors.rs
@@ -61,8 +61,8 @@ pub enum NovaError {
   #[error("IncorrectWitness")]
   IncorrectWitness,
   /// return when error during synthesis
-  #[error("SynthesisError")]
-  SynthesisError,
+  #[error("SynthesisError: {0}")]
+  SynthesisError(String),
   /// returned when there is an error creating a digest
   #[error("DigestError")]
   DigestError,
@@ -84,3 +84,9 @@ pub enum PCSError {
   #[error("LengthError")]
   LengthError,
 }
+
+impl From<bellpepper_core::SynthesisError> for NovaError {
+  fn from(err: bellpepper_core::SynthesisError) -> Self {
+    Self::SynthesisError(err.to_string())
+  }
+}
diff --git a/src/lib.rs b/src/lib.rs
@@ -40,7 +40,7 @@ use crate::{
 };
 use abomonation::Abomonation;
 use abomonation_derive::Abomonation;
-use bellpepper_core::ConstraintSystem;
+use bellpepper_core::{ConstraintSystem, SynthesisError};
 use circuit::{NovaAugmentedCircuit, NovaAugmentedCircuitInputs, NovaAugmentedCircuitParams};
 use constants::{BN_LIMB_WIDTH, BN_N_LIMBS, NUM_FE_WITHOUT_IO_FOR_CRHF, NUM_HASH_BITS};
 use core::marker::PhantomData;
@@ -356,14 +356,9 @@ where
       c_primary,
       pp.ro_consts_circuit_primary.clone(),
     );
-    let zi_primary = circuit_primary
-      .synthesize(&mut cs_primary)
-      .map_err(|_| NovaError::SynthesisError)
-      .expect("Nova error synthesis");
-    let (u_primary, w_primary) = cs_primary
-      .r1cs_instance_and_witness(r1cs_primary, &pp.ck_primary)
-      .map_err(|_e| NovaError::UnSat)
-      .expect("Nova error unsat");
+    let zi_primary = circuit_primary.synthesize(&mut cs_primary)?;
+    let (u_primary, w_primary) =
+      cs_primary.r1cs_instance_and_witness(r1cs_primary, &pp.ck_primary)?;
 
     // base case for the secondary
     let mut cs_secondary = SatisfyingAssignment::<E2>::new();
@@ -382,14 +377,9 @@ where
       c_secondary,
       pp.ro_consts_circuit_secondary.clone(),
     );
-    let zi_secondary = circuit_secondary
-      .synthesize(&mut cs_secondary)
-      .map_err(|_| NovaError::SynthesisError)
-      .expect("Nova error synthesis");
+    let zi_secondary = circuit_secondary.synthesize(&mut cs_secondary)?;
     let (u_secondary, w_secondary) = cs_secondary
-      .r1cs_instance_and_witness(&pp.circuit_shape_secondary.r1cs_shape, &pp.ck_secondary)
-      .map_err(|_e| NovaError::UnSat)
-      .expect("Nova error unsat");
+      .r1cs_instance_and_witness(&pp.circuit_shape_secondary.r1cs_shape, &pp.ck_secondary)?;
 
     // IVC proof for the primary circuit
     let l_w_primary = w_primary;
@@ -414,15 +404,13 @@ where
 
     let zi_primary = zi_primary
       .iter()
-      .map(|v| v.get_value().ok_or(NovaError::SynthesisError))
-      .collect::<Result<Vec<<E1 as Engine>::Scalar>, NovaError>>()
-      .expect("Nova error synthesis");
+      .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing))
+      .collect::<Result<Vec<<E1 as Engine>::Scalar>, _>>()?;
 
     let zi_secondary = zi_secondary
       .iter()
-      .map(|v| v.get_value().ok_or(NovaError::SynthesisError))
-      .collect::<Result<Vec<<E2 as Engine>::Scalar>, NovaError>>()
-      .expect("Nova error synthesis");
+      .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing))
+      .collect::<Result<Vec<<E2 as Engine>::Scalar>, _>>()?;
 
     let buffer_primary = ResourceBuffer {
       l_w: None,
@@ -492,8 +480,7 @@ where
       &mut self.buffer_secondary.T,
       &mut self.buffer_secondary.ABC_Z_1,
       &mut self.buffer_secondary.ABC_Z_2,
-    )
-    .expect("Unable to fold secondary");
+    )?;
 
     let mut cs_primary = SatisfyingAssignment::<E1>::with_capacity(
       pp.circuit_shape_primary.r1cs_shape.num_io + 1,
@@ -516,14 +503,10 @@ where
       pp.ro_consts_circuit_primary.clone(),
     );
 
-    let zi_primary = circuit_primary
-      .synthesize(&mut cs_primary)
-      .map_err(|_| NovaError::SynthesisError)?;
+    let zi_primary = circuit_primary.synthesize(&mut cs_primary)?;
 
-    let (l_u_primary, l_w_primary) = cs_primary
-      .r1cs_instance_and_witness(&pp.circuit_shape_primary.r1cs_shape, &pp.ck_primary)
-      .map_err(|_e| NovaError::UnSat)
-      .expect("Nova error unsat");
+    let (l_u_primary, l_w_primary) =
+      cs_primary.r1cs_instance_and_witness(&pp.circuit_shape_primary.r1cs_shape, &pp.ck_primary)?;
 
     // fold the primary circuit's instance
     let nifs_primary = NIFS::prove_mut(
@@ -538,8 +521,7 @@ where
       &mut self.buffer_primary.T,
       &mut self.buffer_primary.ABC_Z_1,
       &mut self.buffer_primary.ABC_Z_2,
-    )
-    .expect("Unable to fold primary");
+    )?;
 
     let mut cs_secondary = SatisfyingAssignment::<E2>::with_capacity(
       pp.circuit_shape_secondary.r1cs_shape.num_io + 1,
@@ -561,9 +543,7 @@ where
       c_secondary,
       pp.ro_consts_circuit_secondary.clone(),
     );
-    let zi_secondary = circuit_secondary
-      .synthesize(&mut cs_secondary)
-      .map_err(|_| NovaError::SynthesisError)?;
+    let zi_secondary = circuit_secondary.synthesize(&mut cs_secondary)?;
 
     let (l_u_secondary, l_w_secondary) = cs_secondary
       .r1cs_instance_and_witness(&pp.circuit_shape_secondary.r1cs_shape, &pp.ck_secondary)
@@ -572,12 +552,12 @@ where
     // update the running instances and witnesses
     self.zi_primary = zi_primary
       .iter()
-      .map(|v| v.get_value().ok_or(NovaError::SynthesisError))
-      .collect::<Result<Vec<<E1 as Engine>::Scalar>, NovaError>>()?;
+      .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing))
+      .collect::<Result<Vec<<E1 as Engine>::Scalar>, _>>()?;
     self.zi_secondary = zi_secondary
       .iter()
-      .map(|v| v.get_value().ok_or(NovaError::SynthesisError))
-      .collect::<Result<Vec<<E2 as Engine>::Scalar>, NovaError>>()?;
+      .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing))
+      .collect::<Result<Vec<<E2 as Engine>::Scalar>, _>>()?;
 
     self.l_u_secondary = l_u_secondary;
     self.l_w_secondary = l_w_secondary;

diff --git a/src/provider/non_hiding_zeromorph.rs b/src/provider/non_hiding_zeromorph.rs
@@ -238,7 +238,7 @@ where
 
     // Compute and send proof commitment pi
     let (uvproof, _uveval): (UVKZGProof<_>, UVKZGEvaluation<_>) =
-      UVKZGPCS::<E>::open(&pp.open_pp, &f, &x).map(|(proof, eval)| (proof, eval))?;
+      UVKZGPCS::<E>::open(&pp.open_pp, &f, &x)?;
 
     let proof = ZMProof {
       pi: uvproof.proof,

diff --git a/src/supernova/error.rs b/src/supernova/error.rs
@@ -10,7 +10,7 @@ pub enum SuperNovaError {
   /// Nova error
   #[error("NovaError")]
   NovaError(#[from] NovaError),
-  /// missig commitment key
+  /// missing commitment key
   #[error("MissingCK")]
   MissingCK,
   /// Extended error for supernova

diff --git a/src/supernova/mod.rs b/src/supernova/mod.rs
@@ -23,6 +23,7 @@ use crate::{
 
 use abomonation::Abomonation;
 use abomonation_derive::Abomonation;
+use bellpepper_core::SynthesisError;
 use ff::{Field, PrimeField};
 use itertools::Itertools as _;
 use once_cell::sync::OnceCell;
@@ -497,7 +498,7 @@ where
     let (zi_primary_pc_next, zi_primary) =
       circuit_primary.synthesize(&mut cs_primary).map_err(|err| {
         debug!("err {:?}", err);
-        NovaError::SynthesisError
+        NovaError::from(err)
       })?;
     if zi_primary.len() != pp[circuit_index].F_arity {
       return Err(SuperNovaError::NovaError(
@@ -508,7 +509,7 @@ where
       .r1cs_instance_and_witness(&pp[circuit_index].r1cs_shape, &pp.ck_primary)
       .map_err(|err| {
         debug!("err {:?}", err);
-        NovaError::SynthesisError
+        err
       })?;
 
     // base case for the secondary
@@ -535,7 +536,7 @@ where
     );
     let (_, zi_secondary) = circuit_secondary
       .synthesize(&mut cs_secondary)
-      .map_err(|_| NovaError::SynthesisError)?;
+      .map_err(NovaError::from)?;
     if zi_secondary.len() != pp.circuit_shape_secondary.F_arity {
       return Err(NovaError::InvalidStepOutputLength.into());
     }
@@ -567,15 +568,21 @@ where
     // Outputs of the two circuits and next program counter thus far.
     let zi_primary = zi_primary
       .iter()
-      .map(|v| v.get_value().ok_or(NovaError::SynthesisError.into()))
+      .map(|v| {
+        v.get_value()
+          .ok_or(NovaError::from(SynthesisError::AssignmentMissing).into())
+      })
       .collect::<Result<Vec<<E1 as Engine>::Scalar>, SuperNovaError>>()?;
     let zi_primary_pc_next = zi_primary_pc_next
       .expect("zi_primary_pc_next missing")
       .get_value()
-      .ok_or::<SuperNovaError>(NovaError::SynthesisError.into())?;
+      .ok_or::<SuperNovaError>(NovaError::from(SynthesisError::AssignmentMissing).into())?;
     let zi_secondary = zi_secondary
       .iter()
-      .map(|v| v.get_value().ok_or(NovaError::SynthesisError.into()))
+      .map(|v| {
+        v.get_value()
+          .ok_or(NovaError::from(SynthesisError::AssignmentMissing).into())
+      })
       .collect::<Result<Vec<<E2 as Engine>::Scalar>, SuperNovaError>>()?;
 
     // handle the base case by initialize U_next in next round
@@ -671,7 +678,7 @@ where
 
     let (zi_primary_pc_next, zi_primary) = circuit_primary
       .synthesize(&mut cs_primary)
-      .map_err(|_| SuperNovaError::NovaError(NovaError::SynthesisError))?;
+      .map_err(NovaError::from)?;
     if zi_primary.len() != pp[circuit_index].F_arity {
       return Err(SuperNovaError::NovaError(
         NovaError::InvalidInitialInputLength,
@@ -737,34 +744,33 @@ where
     );
     let (_, zi_secondary) = circuit_secondary
       .synthesize(&mut cs_secondary)
-      .map_err(|_| SuperNovaError::NovaError(NovaError::SynthesisError))?;
+      .map_err(NovaError::from)?;
     if zi_secondary.len() != pp.circuit_shape_secondary.F_arity {
       return Err(SuperNovaError::NovaError(
         NovaError::InvalidInitialInputLength,
       ));
     }
 
     let (l_u_secondary_next, l_w_secondary_next) = cs_secondary
-      .r1cs_instance_and_witness(&pp.circuit_shape_secondary.r1cs_shape, &pp.ck_secondary)
-      .map_err(|_| SuperNovaError::NovaError(NovaError::UnSat))?;
+      .r1cs_instance_and_witness(&pp.circuit_shape_secondary.r1cs_shape, &pp.ck_secondary)?;
 
     // update the running instances and witnesses
     let zi_primary = zi_primary
       .iter()
       .map(|v| {
         v.get_value()
-          .ok_or(SuperNovaError::NovaError(NovaError::SynthesisError))
+          .ok_or(NovaError::from(SynthesisError::AssignmentMissing).into())
       })
       .collect::<Result<Vec<<E1 as Engine>::Scalar>, SuperNovaError>>()?;
     let zi_primary_pc_next = zi_primary_pc_next
       .expect("zi_primary_pc_next missing")
       .get_value()
-      .ok_or(SuperNovaError::NovaError(NovaError::SynthesisError))?;
+      .ok_or::<SuperNovaError>(NovaError::from(SynthesisError::AssignmentMissing).into())?;
     let zi_secondary = zi_secondary
       .iter()
       .map(|v| {
         v.get_value()
-          .ok_or(SuperNovaError::NovaError(NovaError::SynthesisError))
+          .ok_or(NovaError::from(SynthesisError::AssignmentMissing).into())
       })
       .collect::<Result<Vec<<E2 as Engine>::Scalar>, SuperNovaError>>()?;
 

diff --git a/src/supernova/utils.rs b/src/supernova/utils.rs
@@ -29,7 +29,7 @@ pub fn get_from_vec_alloc_relaxed_r1cs<E: Engine, CS: ConstraintSystem<<E as Eng
 
   // Compare all instances in `a` to the first one
   let first: AllocatedRelaxedR1CSInstance<E> = a
-    .get(0)
+    .first()
     .cloned()
     .ok_or_else(|| SynthesisError::IncompatibleLengthVector("empty vec length".to_string()))?;