From c0fd609b0d3e183456a6d85b29a92da219cbe9fb Mon Sep 17 00:00:00 2001
From: soham <soham@zemse.in>
Date: Mon, 14 Oct 2024 12:16:07 +0530
Subject: [PATCH] mock prover hide duplicate errors (#354)

Currently, since we are adding a duplicate instance at the end, this
causes duplicate errors and sometimes it can be a lot of errors. This PR
attempts to cover the current case where if two consecutive errors are
identical then hide the duplicate.

```
======================================================

AssertZeroError ....

Error: 2 constraints not satisfied (1 duplicates hidden)
======================================================
```
---
 ceno_zkvm/src/scheme/mock_prover.rs | 108 ++++++++++++++++++++++++++--
 1 file changed, 101 insertions(+), 7 deletions(-)

diff --git a/ceno_zkvm/src/scheme/mock_prover.rs b/ceno_zkvm/src/scheme/mock_prover.rs
index 17e113659..9d4411063 100644
--- a/ceno_zkvm/src/scheme/mock_prover.rs
+++ b/ceno_zkvm/src/scheme/mock_prover.rs
@@ -116,7 +116,7 @@ pub const MOCK_PC_LUI: ByteAddr = ByteAddr(CENO_PLATFORM.pc_start() + 88);
 pub const MOCK_PC_AUIPC: ByteAddr = ByteAddr(CENO_PLATFORM.pc_start() + 92);
 
 #[allow(clippy::enum_variant_names)]
-#[derive(Debug, PartialEq, Clone)]
+#[derive(Debug, Clone)]
 pub(crate) enum MockProverError<E: ExtensionField> {
     AssertZeroError {
         expression: Expression<E>,
@@ -143,6 +143,75 @@ pub(crate) enum MockProverError<E: ExtensionField> {
     // w_expressions
 }
 
+impl<E: ExtensionField> PartialEq for MockProverError<E> {
+    // Compare errors based on the content, ignoring the inst_id
+    fn eq(&self, other: &Self) -> bool {
+        match (self, other) {
+            (
+                MockProverError::AssertZeroError {
+                    expression: left_expression,
+                    evaluated: left_evaluated,
+                    name: left_name,
+                    ..
+                },
+                MockProverError::AssertZeroError {
+                    expression: right_expression,
+                    evaluated: right_evaluated,
+                    name: right_name,
+                    ..
+                },
+            ) => {
+                left_expression == right_expression
+                    && left_evaluated == right_evaluated
+                    && left_name == right_name
+            }
+            (
+                MockProverError::AssertEqualError {
+                    left_expression: left_left_expression,
+                    right_expression: left_right_expression,
+                    left: left_left,
+                    right: left_right,
+                    name: left_name,
+                    ..
+                },
+                MockProverError::AssertEqualError {
+                    left_expression: right_left_expression,
+                    right_expression: right_right_expression,
+                    left: right_left,
+                    right: right_right,
+                    name: right_name,
+                    ..
+                },
+            ) => {
+                left_left_expression == right_left_expression
+                    && left_right_expression == right_right_expression
+                    && left_left == right_left
+                    && left_right == right_right
+                    && left_name == right_name
+            }
+            (
+                MockProverError::LookupError {
+                    expression: left_expression,
+                    evaluated: left_evaluated,
+                    name: left_name,
+                    ..
+                },
+                MockProverError::LookupError {
+                    expression: right_expression,
+                    evaluated: right_evaluated,
+                    name: right_name,
+                    ..
+                },
+            ) => {
+                left_expression == right_expression
+                    && left_evaluated == right_evaluated
+                    && left_name == right_name
+            }
+            _ => false,
+        }
+    }
+}
+
 impl<E: ExtensionField> MockProverError<E> {
     pub fn print(&self, wits_in: &[ArcMultilinearExtension<E>], wits_in_name: &[String]) {
         let mut wtns = vec![];
@@ -203,6 +272,14 @@ impl<E: ExtensionField> MockProverError<E> {
             }
         }
     }
+
+    fn inst_id(&self) -> usize {
+        match self {
+            Self::AssertZeroError { inst_id, .. }
+            | Self::AssertEqualError { inst_id, .. }
+            | Self::LookupError { inst_id, .. } => *inst_id,
+        }
+    }
 }
 
 pub(crate) struct MockProver<E: ExtensionField> {
@@ -419,7 +496,7 @@ impl<'a, E: ExtensionField + Hash> MockProver<E> {
                 }
             } else {
                 // contains require_zero
-                let expr_evaluated = wit_infer_by_expr(&[], wits_in, pi, &challenge, &expr);
+                let expr_evaluated = wit_infer_by_expr(&[], wits_in, pi, &challenge, expr);
                 let expr_evaluated = expr_evaluated
                     .get_ext_field_vec_optn()
                     .map(|v| v.to_vec())
@@ -488,7 +565,6 @@ impl<'a, E: ExtensionField + Hash> MockProver<E> {
             Ok(_) => {}
             Err(errors) => {
                 println!("======================================================");
-                println!("Error: {} constraints not satisfied", errors.len());
 
                 println!(
                     r"Hints:
@@ -499,8 +575,26 @@ impl<'a, E: ExtensionField + Hash> MockProver<E> {
                     "
                 );
 
-                for error in errors {
-                    error.print(wits_in, &cb.cs.witin_namespace_map);
+                // Print errors and skip consecutive duplicates errors if they are equal.
+                let mut duplicates = 0;
+                let mut prev_err = None;
+                for error in &errors {
+                    if prev_err.is_some() && prev_err.unwrap() == error {
+                        duplicates += 1;
+                    } else {
+                        error.print(wits_in, &cb.cs.witin_namespace_map);
+                    }
+                    prev_err = Some(error);
+                }
+
+                if duplicates > 0 {
+                    println!(
+                        "Error: {} constraints not satisfied ({} duplicates hidden)",
+                        errors.len(),
+                        duplicates
+                    );
+                } else {
+                    println!("Error: {} constraints not satisfied", errors.len());
                 }
                 println!("======================================================");
                 panic!("Constraints not satisfied");
@@ -638,8 +732,6 @@ mod tests {
                         Box::new(Expression::Challenge(
                             1,
                             1,
-                            // TODO this still uses default challenge in ConstraintSystem, but challengeId
-                            // helps to evaluate the expression correctly. Shoudl challenge be just challengeId?
                             GoldilocksExt2::ONE,
                             GoldilocksExt2::ZERO,
                         )),
@@ -659,6 +751,8 @@ mod tests {
                 inst_id: 0,
             }]
         );
+        // because inst_id is not checked in our PartialEq impl
+        assert_eq!(err[0].inst_id(), 0);
     }
 
     #[allow(dead_code)]