From 9e600d900592b44aacfb470e1d718cf17576f9de Mon Sep 17 00:00:00 2001 From: Jeb Bearer Date: Mon, 1 Jan 2024 09:04:54 -0800 Subject: [PATCH] Improve error handling * 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. --- src/errors.rs | 15 +++++++++++-- src/lib.rs | 62 +++++++++++++++++---------------------------------- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/src/errors.rs b/src/errors.rs index fd692208..e2ff3d02 100644 --- a/src/errors.rs +++ b/src/errors.rs @@ -58,8 +58,11 @@ pub enum NovaError { #[error("IncorrectWitness")] IncorrectWitness, /// return when error during synthesis - #[error("SynthesisError")] - SynthesisError, + #[error("SynthesisError: {reason}")] + SynthesisError { + /// The reason for circuit synthesis failure + reason: String, + }, /// returned when there is an error creating a digest #[error("DigestError")] DigestError, @@ -67,3 +70,11 @@ pub enum NovaError { #[error("InternalError")] InternalError, } + +impl From for NovaError { + fn from(err: bellpepper_core::SynthesisError) -> Self { + Self::SynthesisError { + reason: err.to_string(), + } + } +} diff --git a/src/lib.rs b/src/lib.rs index baa09b82..4e7eca24 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -33,7 +33,7 @@ use crate::bellpepper::{ solver::SatisfyingAssignment, }; use crate::digest::{DigestComputer, SimpleDigestible}; -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; @@ -286,14 +286,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(&pp.r1cs_shape_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(&pp.r1cs_shape_primary, &pp.ck_primary)?; // base case for the secondary let mut cs_secondary = SatisfyingAssignment::::new(); @@ -312,14 +307,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 (u_secondary, w_secondary) = cs_secondary - .r1cs_instance_and_witness(&pp.r1cs_shape_secondary, &pp.ck_secondary) - .map_err(|_e| NovaError::UnSat) - .expect("Nova error unsat"); + let zi_secondary = circuit_secondary.synthesize(&mut cs_secondary)?; + let (u_secondary, w_secondary) = + cs_secondary.r1cs_instance_and_witness(&pp.r1cs_shape_secondary, &pp.ck_secondary)?; // IVC proof for the primary circuit let l_w_primary = w_primary; @@ -342,15 +332,13 @@ where let zi_primary = zi_primary .iter() - .map(|v| v.get_value().ok_or(NovaError::SynthesisError)) - .collect::::Scalar>, NovaError>>() - .expect("Nova error synthesis"); + .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing)) + .collect::::Scalar>, _>>()?; let zi_secondary = zi_secondary .iter() - .map(|v| v.get_value().ok_or(NovaError::SynthesisError)) - .collect::::Scalar>, NovaError>>() - .expect("Nova error synthesis"); + .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing)) + .collect::::Scalar>, _>>()?; Ok(Self { z0_primary: z0_primary.to_vec(), @@ -392,8 +380,7 @@ where &self.r_W_secondary, &self.l_u_secondary, &self.l_w_secondary, - ) - .expect("Unable to fold secondary"); + )?; let mut cs_primary = SatisfyingAssignment::::new(); let inputs_primary: NovaAugmentedCircuitInputs = NovaAugmentedCircuitInputs::new( @@ -412,14 +399,10 @@ where c_primary, 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.r1cs_shape_primary, &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.r1cs_shape_primary, &pp.ck_primary)?; // fold the primary circuit's instance let (nifs_primary, (r_U_primary, r_W_primary)) = NIFS::prove( @@ -431,8 +414,7 @@ where &self.r_W_primary, &l_u_primary, &l_w_primary, - ) - .expect("Unable to fold primary"); + )?; let mut cs_secondary = SatisfyingAssignment::::new(); let inputs_secondary: NovaAugmentedCircuitInputs = NovaAugmentedCircuitInputs::new( @@ -451,9 +433,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.r1cs_shape_secondary, &pp.ck_secondary) @@ -462,12 +442,12 @@ where // update the running instances and witnesses self.zi_primary = zi_primary .iter() - .map(|v| v.get_value().ok_or(NovaError::SynthesisError)) - .collect::::Scalar>, NovaError>>()?; + .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing)) + .collect::::Scalar>, _>>()?; self.zi_secondary = zi_secondary .iter() - .map(|v| v.get_value().ok_or(NovaError::SynthesisError)) - .collect::::Scalar>, NovaError>>()?; + .map(|v| v.get_value().ok_or(SynthesisError::AssignmentMissing)) + .collect::::Scalar>, _>>()?; self.l_u_secondary = l_u_secondary; self.l_w_secondary = l_w_secondary;