fix: Improve benchmarks quality and code duplication (#178)

* fix: correct bench group typo * refactor: Refactor snark benchmark code to remove duplication - Constants have been introduced to replace hard coded values previously used in the verifier circuit and number of samples. - `bench_compressed_snark` function has been restructured, with a new `bench_compressed_snark_internal` function introduced to reduce duplication. - Similar refactoring has also been applied to `bench_compressed_snark_with_computational_commitments`, ensuring uniformity in the code and decreasing duplication. - The benchmark function has been updated to incorporate the use of the new constants as well as the refactored functions. * refactor: Refactor Supernova SNARK benchmarks for code reuse - Refactored benchmarking code in `compressed-snark-supernova.rs` for improved reuse and readability. - Introduced the use of constants for constant values. - Replaced hardcoded assignments with references to declared constants. - Eliminated duplicated benchmarking codes and introduced a shared function for increased efficiency. * refactor: Refactor recursive SNARK benchmarking code - Improved the benchmarking system by introducing a new function `bench_recursive_snark_internal_with_arity`, which offers code reuse. - Replaced a hardcoded number with a constant (`NUM_CONS_VERIFIER_CITCUIT_PRIMARY`) and introduced a new constant (`NUM_SAMPLES`) for setting the benchmark group sample size, - Refactored the functions `bench_one_augmented_circuit_recursive_snark` and `bench_two_augmented_circuit_recursive_snark` to reduce code redundancy and improve maintainability. - Removed unnecessary assertions and error handling process with more concise `.expect` methods. * refactor: Refactor constants in recursive-snark.rs - Introduced named constants (`NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` and `NUM_SAMPLES`) for an enhancement of readability and maintainability. - Improved the benchmarking function through the replacement of hard-coded values with the newly initialized constants. * fix: update num_cons_verifier_circuit_primary * test: Refine supernova recursive circuit tests and benchmarks - Updated the `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` constant value in both `recursive-snark-supernova` and `compressed-snark-supernova` benchmarks for accurate evaluation. - Enhanced the documentation for `NUM_CONS_VERIFIER_CIRCUIT_PRIMARY` with detailed comments explaining its correlation with `test_supernova_recursive_circuit_pasta` and `num_augmented_circuits`. - Extended the `src/supernova/circuit.rs` module with a new test suite and additional functions to test supernova recursive circuits, * fix: also update constraint values in the loop * refactor: Refine public parameter size hints in compressed supernova bench - Updated public params sizing in `compressed-snark-supernova.rs`.
microsoft · Dec 15, 2023 · 75c67a3 · 75c67a3
1 parent cda4a42
commit 75c67a3
Show file tree

Hide file tree

Showing 3 changed files with 127 additions and 149 deletions.
diff --git a/benches/compressed-snark.rs b/benches/compressed-snark.rs
@@ -2,7 +2,7 @@
 
 use bellpepper_core::{num::AllocatedNum, ConstraintSystem, SynthesisError};
 use core::marker::PhantomData;
-use criterion::*;
+use criterion::{measurement::WallTime, *};
 use ff::PrimeField;
 use nova_snark::{
   provider::{PallasEngine, VestaEngine},
@@ -50,172 +50,137 @@ cfg_if::cfg_if! {
 
 criterion_main!(compressed_snark);
 
-fn bench_compressed_snark(c: &mut Criterion) {
-  let num_samples = 10;
-  let num_cons_verifier_circuit_primary = 9819;
-  // we vary the number of constraints in the step circuit
-  for &num_cons_in_augmented_circuit in
-    [9819, 16384, 32768, 65536, 131072, 262144, 524288, 1048576].iter()
-  {
-    // number of constraints in the step circuit
-    let num_cons = num_cons_in_augmented_circuit - num_cons_verifier_circuit_primary;
-
-    let mut group = c.benchmark_group(format!("CompressedSNARK-StepCircuitSize-{num_cons}"));
-    group.sample_size(num_samples);
-
-    let c_primary = NonTrivialCircuit::new(num_cons);
-    let c_secondary = TrivialCircuit::default();
-
-    // Produce public parameters
-    let pp = PublicParams::<E1, E2, C1, C2>::setup(
-      &c_primary,
-      &c_secondary,
-      &*S1::ck_floor(),
-      &*S2::ck_floor(),
-    );
-
-    // Produce prover and verifier keys for CompressedSNARK
-    let (pk, vk) = CompressedSNARK::<_, _, _, _, S1, S2>::setup(&pp).unwrap();
+// This should match the value for the primary in test_recursive_circuit_pasta
+const NUM_CONS_VERIFIER_CIRCUIT_PRIMARY: usize = 9825;
+const NUM_SAMPLES: usize = 10;
+
+/// Benchmarks the compressed SNARK at a provided number of constraints
+///
+/// Parameters
+/// - `group``: the criterion benchmark group
+/// - `num_cons`: the number of constraints in the step circuit
+fn bench_compressed_snark_internal<S1: RelaxedR1CSSNARKTrait<E1>, S2: RelaxedR1CSSNARKTrait<E2>>(
+  group: &mut BenchmarkGroup<'_, WallTime>,
+  num_cons: usize,
+) {
+  let c_primary = NonTrivialCircuit::new(num_cons);
+  let c_secondary = TrivialCircuit::default();
+
+  // Produce public parameters
+  let pp = PublicParams::<E1, E2, C1, C2>::setup(
+    &c_primary,
+    &c_secondary,
+    &*S1::ck_floor(),
+    &*S2::ck_floor(),
+  );
+
+  // Produce prover and verifier keys for CompressedSNARK
+  let (pk, vk) = CompressedSNARK::<_, _, _, _, S1, S2>::setup(&pp).unwrap();
+
+  // produce a recursive SNARK
+  let num_steps = 3;
+  let mut recursive_snark: RecursiveSNARK<E1, E2, C1, C2> = RecursiveSNARK::new(
+    &pp,
+    &c_primary,
+    &c_secondary,
+    &[<E1 as Engine>::Scalar::from(2u64)],
+    &[<E2 as Engine>::Scalar::from(2u64)],
+  )
+  .unwrap();
+
+  for i in 0..num_steps {
+    let res = recursive_snark.prove_step(&pp, &c_primary, &c_secondary);
+    assert!(res.is_ok());
 
-    // produce a recursive SNARK
-    let num_steps = 3;
-    let mut recursive_snark: RecursiveSNARK<E1, E2, C1, C2> = RecursiveSNARK::new(
+    // verify the recursive snark at each step of recursion
+    let res = recursive_snark.verify(
       &pp,
-      &c_primary,
-      &c_secondary,
+      i + 1,
       &[<E1 as Engine>::Scalar::from(2u64)],
       &[<E2 as Engine>::Scalar::from(2u64)],
-    )
-    .unwrap();
-
-    for i in 0..num_steps {
-      let res = recursive_snark.prove_step(&pp, &c_primary, &c_secondary);
-      assert!(res.is_ok());
-
-      // verify the recursive snark at each step of recursion
-      let res = recursive_snark.verify(
-        &pp,
-        i + 1,
-        &[<E1 as Engine>::Scalar::from(2u64)],
-        &[<E2 as Engine>::Scalar::from(2u64)],
-      );
-      assert!(res.is_ok());
-    }
+    );
+    assert!(res.is_ok());
+  }
 
-    // Bench time to produce a compressed SNARK
-    group.bench_function("Prove", |b| {
-      b.iter(|| {
-        assert!(CompressedSNARK::<_, _, _, _, S1, S2>::prove(
-          black_box(&pp),
-          black_box(&pk),
-          black_box(&recursive_snark)
+  // Bench time to produce a compressed SNARK
+  group.bench_function("Prove", |b| {
+    b.iter(|| {
+      assert!(CompressedSNARK::<_, _, _, _, S1, S2>::prove(
+        black_box(&pp),
+        black_box(&pk),
+        black_box(&recursive_snark)
+      )
+      .is_ok());
+    })
+  });
+  let res = CompressedSNARK::<_, _, _, _, S1, S2>::prove(&pp, &pk, &recursive_snark);
+  assert!(res.is_ok());
+  let compressed_snark = res.unwrap();
+
+  // Benchmark the verification time
+  group.bench_function("Verify", |b| {
+    b.iter(|| {
+      assert!(black_box(&compressed_snark)
+        .verify(
+          black_box(&vk),
+          black_box(num_steps),
+          black_box(&[<E1 as Engine>::Scalar::from(2u64)]),
+          black_box(&[<E2 as Engine>::Scalar::from(2u64)]),
         )
         .is_ok());
-      })
-    });
-    let res = CompressedSNARK::<_, _, _, _, S1, S2>::prove(&pp, &pk, &recursive_snark);
-    assert!(res.is_ok());
-    let compressed_snark = res.unwrap();
+    })
+  });
+}
+
+fn bench_compressed_snark(c: &mut Criterion) {
+  // we vary the number of constraints in the step circuit
+  for &num_cons_in_augmented_circuit in [
+    NUM_CONS_VERIFIER_CIRCUIT_PRIMARY,
+    16384,
+    32768,
+    65536,
+    131072,
+    262144,
+    524288,
+    1048576,
+  ]
+  .iter()
+  {
+    // number of constraints in the step circuit
+    let num_cons = num_cons_in_augmented_circuit - NUM_CONS_VERIFIER_CIRCUIT_PRIMARY;
+
+    let mut group = c.benchmark_group(format!("CompressedSNARK-StepCircuitSize-{num_cons}"));
+    group.sample_size(NUM_SAMPLES);
 
-    // Benchmark the verification time
-    group.bench_function("Verify", |b| {
-      b.iter(|| {
-        assert!(black_box(&compressed_snark)
-          .verify(
-            black_box(&vk),
-            black_box(num_steps),
-            black_box(&[<E1 as Engine>::Scalar::from(2u64)]),
-            black_box(&[<E2 as Engine>::Scalar::from(2u64)]),
-          )
-          .is_ok());
-      })
-    });
+    bench_compressed_snark_internal::<S1, S2>(&mut group, num_cons);
 
     group.finish();
   }
 }
 
 fn bench_compressed_snark_with_computational_commitments(c: &mut Criterion) {
-  let num_samples = 10;
-  let num_cons_verifier_circuit_primary = 9819;
   // we vary the number of constraints in the step circuit
-  for &num_cons_in_augmented_circuit in [9819, 16384, 32768, 65536, 131072, 262144].iter() {
+  for &num_cons_in_augmented_circuit in [
+    NUM_CONS_VERIFIER_CIRCUIT_PRIMARY,
+    16384,
+    32768,
+    65536,
+    131072,
+    262144,
+  ]
+  .iter()
+  {
     // number of constraints in the step circuit
-    let num_cons = num_cons_in_augmented_circuit - num_cons_verifier_circuit_primary;
+    let num_cons = num_cons_in_augmented_circuit - NUM_CONS_VERIFIER_CIRCUIT_PRIMARY;
 
     let mut group = c.benchmark_group(format!(
       "CompressedSNARK-Commitments-StepCircuitSize-{num_cons}"
     ));
     group
       .sampling_mode(SamplingMode::Flat)
-      .sample_size(num_samples);
-
-    let c_primary = NonTrivialCircuit::new(num_cons);
-    let c_secondary = TrivialCircuit::default();
-
-    // Produce public parameters
-    let pp = PublicParams::<E1, E2, C1, C2>::setup(
-      &c_primary,
-      &c_secondary,
-      &*SS1::ck_floor(),
-      &*SS2::ck_floor(),
-    );
-    // Produce prover and verifier keys for CompressedSNARK
-    let (pk, vk) = CompressedSNARK::<_, _, _, _, SS1, SS2>::setup(&pp).unwrap();
-
-    // produce a recursive SNARK
-    let num_steps = 3;
-    let mut recursive_snark: RecursiveSNARK<E1, E2, C1, C2> = RecursiveSNARK::new(
-      &pp,
-      &c_primary,
-      &c_secondary,
-      &[<E1 as Engine>::Scalar::from(2u64)],
-      &[<E2 as Engine>::Scalar::from(2u64)],
-    )
-    .unwrap();
-
-    for i in 0..num_steps {
-      let res = recursive_snark.prove_step(&pp, &c_primary, &c_secondary);
-      assert!(res.is_ok());
-
-      // verify the recursive snark at each step of recursion
-      let res = recursive_snark.verify(
-        &pp,
-        i + 1,
-        &[<E1 as Engine>::Scalar::from(2u64)],
-        &[<E2 as Engine>::Scalar::from(2u64)],
-      );
-      assert!(res.is_ok());
-    }
-
-    // Bench time to produce a compressed SNARK
-    group.bench_function("Prove", |b| {
-      b.iter(|| {
-        assert!(CompressedSNARK::<_, _, _, _, SS1, SS2>::prove(
-          black_box(&pp),
-          black_box(&pk),
-          black_box(&recursive_snark)
-        )
-        .is_ok());
-      })
-    });
-    let res = CompressedSNARK::<_, _, _, _, SS1, SS2>::prove(&pp, &pk, &recursive_snark);
-    assert!(res.is_ok());
-    let compressed_snark = res.unwrap();
+      .sample_size(NUM_SAMPLES);
 
-    // Benchmark the verification time
-    group.bench_function("Verify", |b| {
-      b.iter(|| {
-        assert!(black_box(&compressed_snark)
-          .verify(
-            black_box(&vk),
-            black_box(num_steps),
-            black_box(&[<E1 as Engine>::Scalar::from(2u64)]),
-            black_box(&[<E2 as Engine>::Scalar::from(2u64)]),
-          )
-          .is_ok());
-      })
-    });
+    bench_compressed_snark_internal::<SS1, SS2>(&mut group, num_cons);
 
     group.finish();
   }

diff --git a/benches/recursive-snark.rs b/benches/recursive-snark.rs
@@ -42,17 +42,29 @@ cfg_if::cfg_if! {
 
 criterion_main!(recursive_snark);
 
+// This should match the value for the primary in test_recursive_circuit_pasta
+const NUM_CONS_VERIFIER_CIRCUIT_PRIMARY: usize = 9825;
+const NUM_SAMPLES: usize = 10;
+
 fn bench_recursive_snark(c: &mut Criterion) {
-  let num_cons_verifier_circuit_primary = 9819;
   // we vary the number of constraints in the step circuit
-  for &num_cons_in_augmented_circuit in
-    [9819, 16384, 32768, 65536, 131072, 262144, 524288, 1048576].iter()
+  for &num_cons_in_augmented_circuit in [
+    NUM_CONS_VERIFIER_CIRCUIT_PRIMARY,
+    16384,
+    32768,
+    65536,
+    131072,
+    262144,
+    524288,
+    1048576,
+  ]
+  .iter()
   {
     // number of constraints in the step circuit
-    let num_cons = num_cons_in_augmented_circuit - num_cons_verifier_circuit_primary;
+    let num_cons = num_cons_in_augmented_circuit - NUM_CONS_VERIFIER_CIRCUIT_PRIMARY;
 
     let mut group = c.benchmark_group(format!("RecursiveSNARK-StepCircuitSize-{num_cons}"));
-    group.sample_size(10);
+    group.sample_size(NUM_SAMPLES);
 
     let c_primary = NonTrivialCircuit::new(num_cons);
     let c_secondary = TrivialCircuit::default();

diff --git a/src/circuit.rs b/src/circuit.rs
@@ -449,6 +449,7 @@ mod tests {
 
   #[test]
   fn test_recursive_circuit_pasta() {
+    // this test checks against values that must be replicated in benchmarks if changed here
     let params1 = NovaAugmentedCircuitParams::new(BN_LIMB_WIDTH, BN_N_LIMBS, true);
     let params2 = NovaAugmentedCircuitParams::new(BN_LIMB_WIDTH, BN_N_LIMBS, false);
     let ro_consts1: ROConstantsCircuit<VestaEngine> = PoseidonConstantsCircuit::default();