Merge pull request #3262 from o1-labs/volhovm/fix-ci-benches

Fix failing mina benches in CI

Author: volhovm

.github/workflows/benches-mina-prover-set-baseline.yml

@@ -1,9 +1,15 @@
 name: Bench mina circuits (set baseline master)
 
+# Runs on either pushes to master, or on manual call
+#
+# Please don't regenerate the data unless you REALLY know what you're doing, e.g. you had to regenerate
+# benchmark data and now the baselines changed.
 on:
   push:
     branches:
       - master
+  pull_request:
+    types: [labeled]
 
 env:
   OCAML_VERSION: "4.14"
@@ -12,6 +18,7 @@ env:
 
 jobs:
   bench-set-baseline:
+    if: github.event_name == 'push' || github.event.label.name == 'unsafe-benches-mina-reset-baseline'
     runs-on: ubuntu-latest
     name: Run benches
     steps:

kimchi-stubs/src/pasta_fp_plonk_proof.rs

@@ -84,6 +84,10 @@ pub fn caml_pasta_fp_plonk_proof_create(
     // public input
     let public_input = witness[0][0..index.cs.public].to_vec();
 
+    if std::env::var("KIMCHI_PROVER_DUMP_ARGUMENTS").is_ok() {
+        kimchi::bench::bench_arguments_dump_into_file(&index.cs, &witness, &runtime_tables, &prev);
+    }
+
     // NB: This method is designed only to be used by tests. However, since creating a new reference will cause `drop` to be called on it once we are done with it. Since `drop` calls `caml_shutdown` internally, we *really, really* do not want to do this, but we have no other way to get at the active runtime.
     // TODO: There's actually a way to get a handle to the runtime as a function argument. Switch
     // to doing this instead.

kimchi-stubs/src/pasta_fq_plonk_proof.rs

@@ -79,6 +79,10 @@ pub fn caml_pasta_fq_plonk_proof_create(
     // public input
     let public_input = witness[0][0..index.cs.public].to_vec();
 
+    if std::env::var("KIMCHI_PROVER_DUMP_ARGUMENTS").is_ok() {
+        kimchi::bench::bench_arguments_dump_into_file(&index.cs, &witness, &runtime_tables, &prev);
+    }
+
     // NB: This method is designed only to be used by tests. However, since
     // creating a new reference will cause `drop` to be called on it once we are
     // done with it. Since `drop` calls `caml_shutdown` internally, we *really,

kimchi/src/circuits/constraints.rs

@@ -234,7 +234,6 @@ where
             gates: Arc<Vec<CircuitGate<F>>>,
             zk_rows: u64,
             feature_flags: FeatureFlags,
-            lazy_mode: bool,
             #[serde_as(as = "Vec<o1_utils::serialization::SerdeAs>")]
             sid: Vec<F>,
             #[serde_as(as = "[o1_utils::serialization::SerdeAs; PERMUTS]")]

kimchi/tests/test_constraints.rs

@@ -86,3 +86,43 @@ fn test_lookup_domain_size_computation() {
             assert_eq!(res.domain.d1.size, expected_domain_size);
         });
 }
+
+#[test]
+fn test_constraint_system_serialization_deserialization() {
+    let (next_start, range_check_gates_0) = CircuitGate::<Fp>::create_range_check(0); /* 1 range_check gate */
+    let (next_start, range_check_gates_1) = CircuitGate::<Fp>::create_range_check(next_start); /* 1 range_check gate */
+    let (next_start, xor_gates_0) = CircuitGate::<Fp>::create_xor_gadget(next_start, 3); /* 1 xor gate */
+    let (next_start, xor_gates_1) = CircuitGate::<Fp>::create_xor_gadget(next_start, 3); /* 1 xor gate */
+    let (_, ffm_gates) =
+        CircuitGate::<Fp>::create_foreign_field_mul(next_start, &Fq::modulus_biguint()); /* 1 foreign field multiplication gate */
+    let circuit_gates: Vec<CircuitGate<Fp>> = range_check_gates_0
+        .into_iter()
+        .chain(range_check_gates_1)
+        .chain(xor_gates_0)
+        .chain(xor_gates_1)
+        .chain(ffm_gates)
+        .collect(); /* 2 range check gates + 2 xor gates + 1 foreign field multiplication */
+
+    let (number_of_table_ids, size) = (10, 10);
+
+    let builder = ConstraintSystem::create(circuit_gates.clone());
+
+    let table_ids: Vec<i32> = (3..number_of_table_ids + 3).collect();
+    let lookup_tables: Vec<LookupTable<Fp>> = table_ids
+        .into_iter()
+        .map(|id| {
+            let indexes: Vec<u32> = (0..size).collect();
+            let data: Vec<Fp> = indexes.into_iter().map(Fp::from).collect();
+            LookupTable {
+                id,
+                data: vec![data],
+            }
+        })
+        .collect();
+    let cs = builder.lookup(lookup_tables).build().unwrap();
+
+    let bytes_cs: Vec<u8> = rmp_serde::to_vec(&cs).unwrap();
+
+    // Should not panic
+    let _: ConstraintSystem<Fp> = rmp_serde::from_read(bytes_cs.as_slice()).unwrap();
+}

scripts/bench-criterion-mina-circuits.sh

@@ -1,5 +1,7 @@
 #!/bin/bash
 
+echo "Starting bench-criterion-mina-circuits.sh"
+
 set -ex
 
 # Queries all test circuits from the cloud
@@ -25,6 +27,13 @@ for test_file in $(list_objects); do
         # The noise threshold is higher than default because our CI machines are not super precise
         REPORT_FILE=/tmp/criterion-result-$(date +%Y-%m-%d_%H-%M-%S).txt
         BENCH_PROOF_CREATION_MINA_INPUTS=$LOCAL_PATH cargo bench --bench proof_criterion_mina -- --noise-threshold 0.05 --baseline $BASELINE_NAME 2>&1 | tee $REPORT_FILE
+
+        BENCH_EXIT_STATUS=${PIPESTATUS[0]}
+        if [ $BENCH_EXIT_STATUS -ne 0 ]; then
+          echo "Cargo bench command failed with exit status $BENCH_EXIT_STATUS"
+          exit $BENCH_EXIT_STATUS
+        fi
+
         # Fail if there is 'regressed' in the logs
         grep 'regressed' $REPORT_FILE && exit 1 || echo "No regressions detected, continuing..."
     else