Fix circuit failure output (#1638)

### Description

1. Capture the error detail of the unsatisfied circuit from
CircuitTestBuilder
2. Split the body of CircuitTestBuilder::run() into 3 methods for
readability.
3. Revamp the error handling in CircuitTestBuilder

### Issue Link

#1637  

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)



### Design choices

The error handling has been a pain point for us for a long time. 
The design of error handling in this PR is heavily inspired by [this
blog post](https://mmapped.blog/posts/12-rust-error-handling.html).

testool/src/statetest/executor.rs

@@ -14,7 +14,11 @@ use external_tracer::TraceConfig;
 use halo2_proofs::{dev::MockProver, halo2curves::bn256::Fr};
 use std::{collections::HashMap, str::FromStr};
 use thiserror::Error;
-use zkevm_circuits::{super_circuit::SuperCircuit, test_util::CircuitTestBuilder, witness::Block};
+use zkevm_circuits::{
+    super_circuit::SuperCircuit,
+    test_util::{CircuitTestBuilder, CircuitTestError},
+    witness::Block,
+};
 
 #[derive(PartialEq, Eq, Error, Debug)]
 pub enum StateTestError {
@@ -329,7 +333,20 @@ pub fn run_test(
         let block: Block<Fr> =
             zkevm_circuits::evm_circuit::witness::block_convert(&builder).unwrap();
 
-        CircuitTestBuilder::<1, 1>::new_from_block(block).run();
+        CircuitTestBuilder::<1, 1>::new_from_block(block)
+            .run_with_result()
+            .map_err(|err| match err {
+                CircuitTestError::VerificationFailed { reasons, .. } => {
+                    StateTestError::CircuitUnsatisfied {
+                        num_failure: reasons.len(),
+                        first_failure: reasons[0].to_string(),
+                    }
+                }
+                err => StateTestError::Exception {
+                    expected: false,
+                    found: err.to_string(),
+                },
+            })?;
     } else {
         geth_data.sign(&wallets);
 

zkevm-circuits/Cargo.toml

@@ -38,6 +38,7 @@ cli-table = { version = "0.4", optional = true }
 num_enum = "0.5.7"
 serde = { version = "1.0.130", features = ["derive"] }
 serde_json = "1.0.78"
+thiserror = "1.0"
 
 [dev-dependencies]
 bus-mapping = { path = "../bus-mapping", features = ["test"] }

zkevm-circuits/src/evm_circuit/execution/addmod.rs

@@ -252,15 +252,12 @@ mod test {
                 .unwrap();
             last.stack = Stack::from_vec(vec![r]);
         }
-        let mut ctb = CircuitTestBuilder::new_from_test_ctx(ctx);
-        if !ok {
-            ctb = ctb.evm_checks(Box::new(|prover, gate_rows, lookup_rows| {
-                assert!(prover
-                    .verify_at_rows_par(gate_rows.iter().cloned(), lookup_rows.iter().cloned())
-                    .is_err())
-            }));
+        let result = CircuitTestBuilder::new_from_test_ctx(ctx).run_with_result();
+        if ok {
+            assert!(result.is_ok());
+        } else {
+            result.unwrap_err().assert_evm_failure()
         };
-        ctb.run()
     }
 
     fn test_ok_u32(a: u32, b: u32, c: u32, r: Option<u32>) {

zkevm-circuits/src/evm_circuit/execution/gas.rs

@@ -147,11 +147,8 @@ mod test {
                 assert_eq!(block.txs[0].steps().len(), 4);
                 block.txs[0].steps_mut()[2].gas_left -= 1;
             }))
-            .evm_checks(Box::new(|prover, gate_rows, lookup_rows| {
-                assert!(prover
-                    .verify_at_rows_par(gate_rows.iter().cloned(), lookup_rows.iter().cloned())
-                    .is_err())
-            }))
-            .run();
+            .run_with_result()
+            .unwrap_err()
+            .assert_evm_failure()
     }
 }

zkevm-circuits/src/evm_circuit/execution/mulmod.rs

@@ -189,15 +189,12 @@ mod test {
             last.stack = Stack::from_vec(vec![r]);
         }
 
-        let mut ctb = CircuitTestBuilder::new_from_test_ctx(ctx);
-        if !ok {
-            ctb = ctb.evm_checks(Box::new(|prover, gate_rows, lookup_rows| {
-                assert!(prover
-                    .verify_at_rows_par(gate_rows.iter().cloned(), lookup_rows.iter().cloned())
-                    .is_err())
-            }));
-        };
-        ctb.run()
+        let result = CircuitTestBuilder::new_from_test_ctx(ctx).run_with_result();
+        if ok {
+            assert!(result.is_ok())
+        } else {
+            result.unwrap_err().assert_evm_failure()
+        }
     }
 
     fn test_ok_u32(a: u32, b: u32, n: u32, r: Option<u32>) {

zkevm-circuits/src/test_util.rs

@@ -8,10 +8,15 @@ use crate::{
 };
 use bus_mapping::{circuit_input_builder::FixedCParams, mock::BlockData};
 use eth_types::geth_types::GethData;
+use itertools::all;
 use std::cmp;
+use thiserror::Error;
 
 use crate::util::log2_ceil;
-use halo2_proofs::{dev::MockProver, halo2curves::bn256::Fr};
+use halo2_proofs::{
+    dev::{MockProver, VerifyFailure},
+    halo2curves::bn256::Fr,
+};
 use mock::TestContext;
 
 #[cfg(test)]
@@ -71,15 +76,12 @@ const NUM_BLINDING_ROWS: usize = 64;
 ///
 /// CircuitTestBuilder::new_from_test_ctx(ctx)
 ///     .block_modifier(Box::new(|block| block.circuits_params.max_evm_rows = (1 << 18) - 100))
-///     .state_checks(Box::new(|prover, evm_rows, lookup_rows| assert!(prover.verify_at_rows_par(evm_rows.iter().cloned(), lookup_rows.iter().cloned()).is_err())))
 ///     .run();
 /// ```
 pub struct CircuitTestBuilder<const NACC: usize, const NTX: usize> {
     test_ctx: Option<TestContext<NACC, NTX>>,
     circuits_params: Option<FixedCParams>,
     block: Option<Block<Fr>>,
-    evm_checks: Box<dyn Fn(MockProver<Fr>, &Vec<usize>, &Vec<usize>)>,
-    state_checks: Box<dyn Fn(MockProver<Fr>, &Vec<usize>, &Vec<usize>)>,
     block_modifiers: Vec<Box<dyn Fn(&mut Block<Fr>)>>,
 }
 
@@ -90,18 +92,6 @@ impl<const NACC: usize, const NTX: usize> CircuitTestBuilder<NACC, NTX> {
             test_ctx: None,
             circuits_params: None,
             block: None,
-            evm_checks: Box::new(|prover, gate_rows, lookup_rows| {
-                prover.assert_satisfied_at_rows_par(
-                    gate_rows.iter().cloned(),
-                    lookup_rows.iter().cloned(),
-                )
-            }),
-            state_checks: Box::new(|prover, gate_rows, lookup_rows| {
-                prover.assert_satisfied_at_rows_par(
-                    gate_rows.iter().cloned(),
-                    lookup_rows.iter().cloned(),
-                )
-            }),
             block_modifiers: vec![],
         }
     }
@@ -142,28 +132,6 @@ impl<const NACC: usize, const NTX: usize> CircuitTestBuilder<NACC, NTX> {
         self
     }
 
-    #[allow(clippy::type_complexity)]
-    /// Allows to provide checks different than the default ones for the State
-    /// Circuit verification.
-    pub fn state_checks(
-        mut self,
-        state_checks: Box<dyn Fn(MockProver<Fr>, &Vec<usize>, &Vec<usize>)>,
-    ) -> Self {
-        self.state_checks = state_checks;
-        self
-    }
-
-    #[allow(clippy::type_complexity)]
-    /// Allows to provide checks different than the default ones for the EVM
-    /// Circuit verification.
-    pub fn evm_checks(
-        mut self,
-        evm_checks: Box<dyn Fn(MockProver<Fr>, &Vec<usize>, &Vec<usize>)>,
-    ) -> Self {
-        self.evm_checks = evm_checks;
-        self
-    }
-
     #[allow(clippy::type_complexity)]
     /// Allows to provide modifier functions for the [`Block`] that will be
     /// generated within this builder.
@@ -177,60 +145,152 @@ impl<const NACC: usize, const NTX: usize> CircuitTestBuilder<NACC, NTX> {
 }
 
 impl<const NACC: usize, const NTX: usize> CircuitTestBuilder<NACC, NTX> {
+    fn build_block(&self) -> Result<Block<Fr>, CircuitTestError> {
+        if let Some(block) = &self.block {
+            // If a block is specified, no need to modify the block
+            return Ok(block.clone());
+        }
+        let block = self
+            .test_ctx
+            .as_ref()
+            .ok_or(CircuitTestError::NotEnoughAttributes)?;
+        let block: GethData = block.clone().into();
+        let builder = BlockData::new_from_geth_data(block.clone()).new_circuit_input_builder();
+        let builder = builder
+            .handle_block(&block.eth_block, &block.geth_traces)
+            .map_err(|err| CircuitTestError::CannotHandleBlock(err.to_string()))?;
+        // Build a witness block from trace result.
+        let mut block = crate::witness::block_convert(&builder)
+            .map_err(|err| CircuitTestError::CannotConvertBlock(err.to_string()))?;
+
+        for modifier_fn in &self.block_modifiers {
+            modifier_fn.as_ref()(&mut block);
+        }
+        Ok(block)
+    }
+
+    fn run_evm_circuit_test(&self, block: Block<Fr>) -> Result<(), CircuitTestError> {
+        let k = block.get_test_degree();
+
+        let (active_gate_rows, active_lookup_rows) = EvmCircuit::<Fr>::get_active_rows(&block);
+
+        let circuit = EvmCircuitCached::get_test_circuit_from_block(block);
+        let prover = MockProver::<Fr>::run(k, &circuit, vec![]).map_err(|err| {
+            CircuitTestError::SynthesisFailure {
+                circuit: Circuit::EVM,
+                reason: err,
+            }
+        })?;
+
+        prover
+            .verify_at_rows_par(
+                active_gate_rows.iter().cloned(),
+                active_lookup_rows.iter().cloned(),
+            )
+            .map_err(|err| CircuitTestError::VerificationFailed {
+                circuit: Circuit::EVM,
+                reasons: err,
+            })
+    }
+    // TODO: use randomness as one of the circuit public input, since randomness in
+    // state circuit and evm circuit must be same
+    fn run_state_circuit_test(&self, block: Block<Fr>) -> Result<(), CircuitTestError> {
+        let rows_needed = StateCircuit::<Fr>::min_num_rows_block(&block).1;
+        let k = cmp::max(log2_ceil(rows_needed + NUM_BLINDING_ROWS), 18);
+        let max_rws = block.circuits_params.max_rws;
+        let state_circuit = StateCircuit::<Fr>::new(block.rws, max_rws);
+        let instance = state_circuit.instance();
+        let prover = MockProver::<Fr>::run(k, &state_circuit, instance).map_err(|err| {
+            CircuitTestError::SynthesisFailure {
+                circuit: Circuit::State,
+                reason: err,
+            }
+        })?;
+        // Skip verification of Start rows to accelerate testing
+        let non_start_rows_len = state_circuit
+            .rows
+            .iter()
+            .filter(|rw| !matches!(rw, Rw::Start { .. }))
+            .count();
+        let rows = max_rws - non_start_rows_len..max_rws;
+        prover
+            .verify_at_rows_par(rows.clone(), rows)
+            .map_err(|err| CircuitTestError::VerificationFailed {
+                circuit: Circuit::EVM,
+                reasons: err,
+            })
+    }
     /// Triggers the `CircuitTestBuilder` to convert the [`TestContext`] if any,
     /// into a [`Block`] and apply the default or provided block_modifiers or
     /// circuit checks to the provers generated for the State and EVM circuits.
-    pub fn run(self) {
-        let block: Block<Fr> = if self.block.is_some() {
-            self.block.unwrap()
-        } else if self.test_ctx.is_some() {
-            let block: GethData = self.test_ctx.unwrap().into();
-            let builder = BlockData::new_from_geth_data(block.clone()).new_circuit_input_builder();
-            let builder = builder
-                .handle_block(&block.eth_block, &block.geth_traces)
-                .unwrap();
-            // Build a witness block from trace result.
-            let mut block = crate::witness::block_convert(&builder).unwrap();
-
-            for modifier_fn in self.block_modifiers {
-                modifier_fn.as_ref()(&mut block);
-            }
-            block
-        } else {
-            panic!("No attribute to build a block was passed to the CircuitTestBuilder")
-        };
-        let params = block.circuits_params;
+    pub fn run_with_result(self) -> Result<(), CircuitTestError> {
+        let block = self.build_block()?;
+
+        self.run_evm_circuit_test(block.clone())?;
+        self.run_state_circuit_test(block)
+    }
 
-        // Run evm circuit test
-        {
-            let k = block.get_test_degree();
+    /// Convenient method to run in test cases that error handling is not required.
+    pub fn run(self) {
+        self.run_with_result().unwrap()
+    }
+}
 
-            let (active_gate_rows, active_lookup_rows) = EvmCircuit::<Fr>::get_active_rows(&block);
+#[derive(Debug)]
+/// Circuits to test in [`CircuitTestBuilder`]
+pub enum Circuit {
+    /// EVM circuit
+    EVM,
+    /// State circuit
+    State,
+}
 
-            let circuit = EvmCircuitCached::get_test_circuit_from_block(block.clone());
-            let prover = MockProver::<Fr>::run(k, &circuit, vec![]).unwrap();
+#[derive(Debug, Error)]
+/// Errors for Circuit test
+pub enum CircuitTestError {
+    /// We didn't specify enough attibutes to define a block for the circuit test
+    #[error("NotEnoughAttributes")]
+    NotEnoughAttributes,
+    /// Something wrong in the handle_block
+    #[error("CannotHandleBlock({0})")]
+    CannotHandleBlock(String),
+    /// Something worng in the block_convert
+    #[error("CannotConvertBlock({0})")]
+    CannotConvertBlock(String),
+    /// Problem constructing MockProver
+    #[error("SynthesisFailure({circuit:?}, reason: {reason:?})")]
+    SynthesisFailure {
+        /// The circuit that causes the failure
+        circuit: Circuit,
+        /// The MockProver error that causes the failure
+        reason: halo2_proofs::plonk::Error,
+    },
+    /// Failed to verify a circuit in the MockProver
+    #[error("VerificationFailed({circuit:?}, reasons: {reasons:?})")]
+    VerificationFailed {
+        /// The circuit that causes the failure
+        circuit: Circuit,
+        /// The list of verification failure
+        reasons: Vec<VerifyFailure>,
+    },
+}
 
-            self.evm_checks.as_ref()(prover, &active_gate_rows, &active_lookup_rows)
-        }
+impl CircuitTestError {
+    /// Filter out EVM circuit failures
+    ///
+    /// Errors must come from EVM circuit and must be unsatisifed constraints or lookup failure
+    pub fn assert_evm_failure(&self) {
+        match self {
+            Self::VerificationFailed { circuit, reasons } => {
+                assert!(matches!(circuit, Circuit::EVM));
+                assert!(!reasons.is_empty());
 
-        // Run state circuit test
-        // TODO: use randomness as one of the circuit public input, since randomness in
-        // state circuit and evm circuit must be same
-        {
-            let rows_needed = StateCircuit::<Fr>::min_num_rows_block(&block).1;
-            let k = cmp::max(log2_ceil(rows_needed + NUM_BLINDING_ROWS), 18);
-            let state_circuit = StateCircuit::<Fr>::new(block.rws, params.max_rws);
-            let instance = state_circuit.instance();
-            let prover = MockProver::<Fr>::run(k, &state_circuit, instance).unwrap();
-            // Skip verification of Start rows to accelerate testing
-            let non_start_rows_len = state_circuit
-                .rows
-                .iter()
-                .filter(|rw| !matches!(rw, Rw::Start { .. }))
-                .count();
-            let rows = (params.max_rws - non_start_rows_len..params.max_rws).collect();
-
-            self.state_checks.as_ref()(prover, &rows, &rows);
+                assert!(all(reasons, |reason| matches!(
+                    reason,
+                    VerifyFailure::ConstraintNotSatisfied { .. } | VerifyFailure::Lookup { .. }
+                )));
+            }
+            _ => panic!("Not a EVM circuit failure {self:?}"),
         }
     }
 }