From 5022d5dba5092fb13b32c2288d0a755a4f12c2cf Mon Sep 17 00:00:00 2001 From: Chih Cheng Liang Date: Wed, 4 Oct 2023 18:27:59 +0800 Subject: [PATCH] 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). --- Cargo.lock | 1 + testool/src/statetest/executor.rs | 21 +- zkevm-circuits/Cargo.toml | 1 + .../src/evm_circuit/execution/addmod.rs | 13 +- .../src/evm_circuit/execution/gas.rs | 9 +- .../src/evm_circuit/execution/mulmod.rs | 15 +- zkevm-circuits/src/test_util.rs | 228 +++++++++++------- 7 files changed, 179 insertions(+), 109 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d905111ac3..0f5d3a53af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4682,6 +4682,7 @@ dependencies = [ "snark-verifier", "strum", "strum_macros", + "thiserror", ] [[package]] diff --git a/testool/src/statetest/executor.rs b/testool/src/statetest/executor.rs index 92a143cf01..21521e43ef 100644 --- a/testool/src/statetest/executor.rs +++ b/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 = 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); diff --git a/zkevm-circuits/Cargo.toml b/zkevm-circuits/Cargo.toml index 437f752cc0..55e406991b 100644 --- a/zkevm-circuits/Cargo.toml +++ b/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"] } diff --git a/zkevm-circuits/src/evm_circuit/execution/addmod.rs b/zkevm-circuits/src/evm_circuit/execution/addmod.rs index cdac3a2a7a..52ed53dec8 100644 --- a/zkevm-circuits/src/evm_circuit/execution/addmod.rs +++ b/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) { diff --git a/zkevm-circuits/src/evm_circuit/execution/gas.rs b/zkevm-circuits/src/evm_circuit/execution/gas.rs index b86441db34..1e626ccc95 100644 --- a/zkevm-circuits/src/evm_circuit/execution/gas.rs +++ b/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() } } diff --git a/zkevm-circuits/src/evm_circuit/execution/mulmod.rs b/zkevm-circuits/src/evm_circuit/execution/mulmod.rs index c4b140b18e..332f86180c 100644 --- a/zkevm-circuits/src/evm_circuit/execution/mulmod.rs +++ b/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) { diff --git a/zkevm-circuits/src/test_util.rs b/zkevm-circuits/src/test_util.rs index 0cffa92f9a..2180c846f6 100644 --- a/zkevm-circuits/src/test_util.rs +++ b/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 { test_ctx: Option>, circuits_params: Option, block: Option>, - evm_checks: Box, &Vec, &Vec)>, - state_checks: Box, &Vec, &Vec)>, block_modifiers: Vec)>>, } @@ -90,18 +92,6 @@ impl CircuitTestBuilder { 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 CircuitTestBuilder { 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, &Vec, &Vec)>, - ) -> 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, &Vec, &Vec)>, - ) -> 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 CircuitTestBuilder { } impl CircuitTestBuilder { + fn build_block(&self) -> Result, 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) -> Result<(), CircuitTestError> { + let k = block.get_test_degree(); + + let (active_gate_rows, active_lookup_rows) = EvmCircuit::::get_active_rows(&block); + + let circuit = EvmCircuitCached::get_test_circuit_from_block(block); + let prover = MockProver::::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) -> Result<(), CircuitTestError> { + let rows_needed = StateCircuit::::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::::new(block.rws, max_rws); + let instance = state_circuit.instance(); + let prover = MockProver::::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 = 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::::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::::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, + }, +} - 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::::min_num_rows_block(&block).1; - let k = cmp::max(log2_ceil(rows_needed + NUM_BLINDING_ROWS), 18); - let state_circuit = StateCircuit::::new(block.rws, params.max_rws); - let instance = state_circuit.instance(); - let prover = MockProver::::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:?}"), } } }