From 0fb0b495f224c555b5b221bd65ea9fbf43384c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Garillot?= <4142+huitseeker@users.noreply.github.com> Date: Fri, 15 Dec 2023 13:22:10 -0500 Subject: [PATCH] fix: Improve benchmarks quality and code duplication (#178) (#282) * 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`. --- benches/compressed-snark.rs | 253 ++++++++++++++++-------------------- benches/recursive-snark.rs | 22 +++- src/circuit.rs | 1 + 3 files changed, 127 insertions(+), 149 deletions(-) diff --git a/benches/compressed-snark.rs b/benches/compressed-snark.rs index ff780224..53d007f0 100644 --- 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::::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, S2: RelaxedR1CSSNARKTrait>( + 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::::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 = RecursiveSNARK::new( + &pp, + &c_primary, + &c_secondary, + &[::Scalar::from(2u64)], + &[::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 = RecursiveSNARK::new( + // verify the recursive snark at each step of recursion + let res = recursive_snark.verify( &pp, - &c_primary, - &c_secondary, + i + 1, &[::Scalar::from(2u64)], &[::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, - &[::Scalar::from(2u64)], - &[::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(&[::Scalar::from(2u64)]), + black_box(&[::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(&[::Scalar::from(2u64)]), - black_box(&[::Scalar::from(2u64)]), - ) - .is_ok()); - }) - }); + bench_compressed_snark_internal::(&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::::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 = RecursiveSNARK::new( - &pp, - &c_primary, - &c_secondary, - &[::Scalar::from(2u64)], - &[::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, - &[::Scalar::from(2u64)], - &[::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(&[::Scalar::from(2u64)]), - black_box(&[::Scalar::from(2u64)]), - ) - .is_ok()); - }) - }); + bench_compressed_snark_internal::(&mut group, num_cons); group.finish(); } diff --git a/benches/recursive-snark.rs b/benches/recursive-snark.rs index 018b3a28..c49a12fa 100644 --- 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 index d9ffad93..89ec9768 100644 --- 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 = PoseidonConstantsCircuit::default();