comments, clippy, small refactorings

Signed-off-by: Matteo Muraca <[email protected]>

src/permutation_chip/chip_setup_api.rs

@@ -15,27 +15,27 @@ impl<const N_OBJECTS: usize, F: ff::Field> PermutationChip<N_OBJECTS, F> {
     ) -> Result<[Number<F>; N_OBJECTS], Error> {
         layouter.assign_region(
             || "load input",
-            |region| apply_permutation_region_assignment(&self, &input_items, permutation, region),
+            |region| apply_permutation_region_assignment(self, &input_items, permutation, region),
         )
     }
 }
 
 /// A helper function to be used in
 /// `PermutationChip::<N_OBJECTS, F>::apply_permutation`.
 /// Its main purpose is to increase readability by reducing indentation.
-fn apply_permutation_region_assignment<'a, const N_OBJECTS: usize, F: ff::Field>(
+fn apply_permutation_region_assignment<const N_OBJECTS: usize, F: ff::Field>(
     chip: &PermutationChip<N_OBJECTS, F>,
     input_items: &[Number<F>; N_OBJECTS],
     permutation: [usize; N_OBJECTS],
-    mut region: Region<'a, F>,
+    mut region: Region<'_, F>,
 ) -> Result<[Number<F>; N_OBJECTS], Error> {
     // We enable the selector gate that activates all the constraints in
     // the permutation chip.
     chip.config.s_perm.enable(&mut region, 0)?;
 
     // We load the input cells in the first row of the region.
-    for idx in 0..N_OBJECTS {
-        input_items[idx].copy_advice(
+    for (idx, input_item) in input_items.iter().enumerate().take(N_OBJECTS) {
+        input_item.copy_advice(
             || "input items",
             &mut region,
             chip.config.item_columns[idx],

src/permutation_chip/gate_implementation.rs

@@ -14,7 +14,7 @@ impl<const N_OBJECTS: usize, F: ff::Field> PermutationChip<N_OBJECTS, F> {
         swap_selector_columns: Vec<Column<Advice>>,
     ) -> <Self as halo2_proofs::circuit::Chip<F>>::Config {
         assert!(
-            swap_selector_columns.len() > 0,
+            !swap_selector_columns.is_empty(),
             "At least one column to allocate swap selectors is needed."
         );
 
@@ -25,10 +25,8 @@ impl<const N_OBJECTS: usize, F: ff::Field> PermutationChip<N_OBJECTS, F> {
         let s_perm = meta.selector();
 
         // The initial position of input items.
-        let mut output_item_positions: [_; N_OBJECTS] = (0..N_OBJECTS)
-            .into_iter()
-            .map(|idx| (item_columns[idx], Rotation::cur()))
-            .f_collect("The number items is correct");
+        let mut output_item_positions: [_; N_OBJECTS] =
+            core::array::from_fn(|idx| (item_columns[idx], Rotation::cur()));
 
         meta.create_gate("object permutation", |meta| {
             let mut constraints = vec![];

src/permutation_circuit.rs

@@ -1,10 +1,10 @@
-use super::permutation_chip::{PConfig, PermutationChip};
-
-use super::Number;
+use crate::{
+    permutation_chip::{PConfig, PermutationChip},
+    Number,
+};
 
-use halo2_proofs::circuit::{Chip, Layouter};
 use halo2_proofs::{
-    circuit::Value,
+    circuit::{Chip, Layouter, Value},
     plonk::{Column, ConstraintSystem, Error, Instance},
 };
 use try_collect::{ForceCollect, TryCollect, TryFromIterator};
@@ -149,12 +149,8 @@ impl<F: ff::Field, const N_OBJECTS: usize> halo2_proofs::plonk::Circuit<F>
         )?;
 
         let mut output_layouter = layouter.namespace(|| "public output assignment");
-        for idx in 0..N_OBJECTS {
-            output_layouter.constrain_instance(
-                permutation_cells[idx].0.cell(),
-                config.instance,
-                idx,
-            )?;
+        for (idx, cell) in permutation_cells.iter().enumerate().take(N_OBJECTS) {
+            output_layouter.constrain_instance(cell.0.cell(), config.instance, idx)?;
         }
 
         Ok(())
@@ -168,6 +164,8 @@ mod tests {
     use crate::utilities::{inverse_permutation, PermutationsIter};
 
     #[test]
+    /// Test the permutation circuit with the mock prover, which prints out errors and warnings.
+    /// We prove that every possible permutation of 7 items is correctly proved.
     fn mock_permutation() {
         use halo2_proofs::{dev::MockProver, pasta::Fp};
 
@@ -177,8 +175,7 @@ mod tests {
         for permutation in PermutationsIter::<7> {
             let circuit = PermutationCircuit::<Fp, 7>::new_unchecked(objects.clone(), permutation);
 
-            // The permutation output is equal to the permutation that is
-            // inverse to `permutation`
+            // The permutation output is equal to the permutation that is inverse to `permutation`
             let permutation_output =
                 Vec::from(inverse_permutation(permutation).map(|x| Fp::from(x as u64)));
 
@@ -190,14 +187,16 @@ mod tests {
     }
 
     #[test]
+    /// Test the permutation circuit with actual prover and verifier through the wrappers we implemented.
+    /// This is very similar to a real use case.
+    /// We prove that every possible permutation of 5 items is correctly proved.
     fn permutation() {
         use halo2_proofs::pasta::Fp;
 
         use crate::utilities::{ProverWrapper, VerifierWrapper};
 
         const N_OBJECTS: usize = 5;
-        const N_OBJECTS_FACTORIAL: usize = 120;
-        type TestCircuit = PermutationCircuit<Fp, N_OBJECTS>;
+        const FACTORIAL: usize = 120;
 
         /// This constant controls the maximum number of rows available in each circuit.
         /// If K is too low, the proof generation fails.
@@ -207,37 +206,41 @@ mod tests {
         /// Currently, we do not know if choosing a bigger value has advantages.
         const K: u32 = 4;
 
-        let objects: [Value<Fp>; 5] = core::array::from_fn(|n| Value::known(Fp::from(n as u64)));
+        let objects: [Value<Fp>; N_OBJECTS] =
+            core::array::from_fn(|n| Value::known(Fp::from(n as u64)));
 
-        let circuit_wiring = TestCircuit::default();
+        let circuit_wiring = PermutationCircuit::<Fp, N_OBJECTS>::default();
         let mut prover = ProverWrapper::initialize_parameters_and_prover(K, circuit_wiring)
             .expect("prover setup should not fail");
 
-        let instances: [[Fp; 5]; N_OBJECTS_FACTORIAL] = PermutationsIter::<5>
+        // For every circuit instance, we need to provide the set of public inputs of that instance.
+        // We have `FACTORIAL` instances, with one column per instance.
+        let instances: [[Fp; N_OBJECTS]; FACTORIAL] = PermutationsIter::<N_OBJECTS>
             .into_iter()
             .map(|permutation| inverse_permutation(permutation).map(|x| Fp::from(x as u64)))
             .f_collect("the number of items is correct");
-        let instance_slices: [[&[Fp]; 1]; N_OBJECTS_FACTORIAL] =
+        let instance_slices: [[&[Fp]; 1]; FACTORIAL] =
             core::array::from_fn(|i| [instances[i].as_slice()]);
 
         for (instance, permutation) in instance_slices.iter().zip(PermutationsIter::<5>) {
-            let circuit = TestCircuit::new_unchecked(objects.clone(), permutation);
+            let circuit =
+                PermutationCircuit::<Fp, N_OBJECTS>::new_unchecked(objects.clone(), permutation);
 
             prover.add_item(circuit, instance.as_slice());
         }
 
         let transcript = crate::time_it! {
-                "The proving time of 120 5-items permutations with an actual prover is {:?}",
-                prover.prove().expect("proof generation should not fail")
+            "The proving time of 120 5-items permutations with an actual prover is {:?}",
+            prover.prove().expect("proof generation should not fail")
         };
 
-        let mut verifier = VerifierWrapper::from(prover);
-
         println!(
             "The aggregated proof's length is {} bytes",
             transcript.len()
         );
 
+        let mut verifier = VerifierWrapper::from(prover);
+
         crate::time_it! {
             "And the verification time with an actual verifier is {:?}",
             assert!(verifier.verify(instance_slices.iter().map(|a| a.as_slice()), transcript.as_slice()))

src/sudoku_circuit.rs

@@ -2,6 +2,7 @@ use crate::{
     permutation_chip::PermutationChip, sudoku_problem_chip::SudokuProblemChip,
     utilities::RegionSequenceAssignment,
 };
+
 use halo2_proofs::{
     circuit::Value,
     circuit::{Layouter, SimpleFloorPlanner},
@@ -60,15 +61,15 @@ impl<F: ff::PrimeField, const SIZE: usize, const SIZE_SQRT: usize>
 
         // We check that the problem contains only symbols or `F::ZERO` entries
         if !problem.iter().all(|col| {
-            col.into_iter()
+            col.iter()
                 .all(|n| *n == F::ZERO || duplicate_detector.contains(n.to_repr().as_ref()))
         }) {
             return Err(());
         }
 
         // We check that the solution only contains symbols
         if !solution.iter().all(|col| {
-            col.into_iter()
+            col.iter()
                 .all(|n| duplicate_detector.contains(n.to_repr().as_ref()))
         }) {
             return Err(());
@@ -315,7 +316,7 @@ impl<F: ff::PrimeField, const SIZE: usize, const SIZE_SQRT: usize> halo2_proofs:
                         |mut region| {
                             // For each permutation result, we constrain it to be equal to the loaded symbols.
                             for p_out in permutation_outputs.iter() {
-                                for (left, right) in p_out.into_iter().zip(symbol_cells.iter()) {
+                                for (left, right) in p_out.iter().zip(symbol_cells.iter()) {
                                     region.constrain_equal(left.cell(), right.cell())?;
                                 }
                             }
@@ -335,14 +336,14 @@ mod tests {
 
     use halo2_proofs::pasta::Fp;
 
-    type SurokuGrid = [[Fp; 9]; 9];
+    type SudokuGrid = [[Fp; 9]; 9];
 
     /// Helper function to generate symbols and a list of problems
     /// The return value is a tuple, laid out as
     /// `(symbols, impl Iterator<Item = (solution, problem)>)`
     fn setup_values(
         nr_random_masks_per_problem: usize,
-    ) -> ([Fp; 9], impl IntoIterator<Item = (SurokuGrid, SurokuGrid)>) {
+    ) -> ([Fp; 9], impl IntoIterator<Item = (SudokuGrid, SudokuGrid)>) {
         let (symbols, grids_iter) = numeric_setup_values(nr_random_masks_per_problem);
         let symbols = symbols.map(|n| Fp::from(n as u64));
 
@@ -450,6 +451,8 @@ mod tests {
     }
 
     #[test]
+    /// Test the sudoku circuit with the mock prover, which prints out errors and warnings.
+    /// We test all the sudoku problems in the test suite, with 10 random masks per problem.
     fn mock_sudoku() {
         use halo2_proofs::{dev::MockProver, pasta::Fp};
 
@@ -487,17 +490,19 @@ mod tests {
     }
 
     #[test]
+    /// Test the sudoku circuit with actual prover and verifier through the wrappers we implemented.
+    /// This is very similar to a real use case.
+    /// We test all the sudoku problems in the test suite, with 2 random masks per problem.
     fn sudoku() {
         use crate::utilities::{ProverWrapper, VerifierWrapper};
 
         const POW_OF_2_MAX_ROWS: u32 = 9;
 
         const NR_RANDOM_MASKS_PER_PROBLEM: usize = 2;
-        type TestCircuit = SudokuCircuit<Fp, 9, 3>;
 
         let (symbols, sudoku_problems) = setup_values(NR_RANDOM_MASKS_PER_PROBLEM);
 
-        let circuit_wiring = TestCircuit::circuit_wiring_from_symbols(symbols);
+        let circuit_wiring = SudokuCircuit::<Fp, 9, 3>::circuit_wiring_from_symbols(symbols);
 
         let mut prover =
             ProverWrapper::initialize_parameters_and_prover(POW_OF_2_MAX_ROWS, circuit_wiring)
@@ -514,8 +519,9 @@ mod tests {
         for ((solution, problem), instance_slices) in
             sudoku_problems.iter().zip(instance_slices.iter())
         {
-            let circuit = TestCircuit::try_new(problem.clone(), solution.clone(), symbols)
-                .expect("creation of circuit instance should not fail");
+            let circuit =
+                SudokuCircuit::<Fp, 9, 3>::try_new(problem.clone(), solution.clone(), symbols)
+                    .expect("creation of circuit instance should not fail");
 
             prover.add_item(circuit, instance_slices);
         }

src/truncated_factorial_circuit.rs

@@ -3,7 +3,7 @@ use halo2_proofs::{
     plonk::{Column, Instance},
 };
 
-use super::Number;
+use crate::Number;
 
 #[derive(Default)]
 pub struct TruncatedFactorialCircuit<
@@ -145,6 +145,11 @@ mod tests {
     }
 
     #[test]
+    /// Test the factorial circuit with the mock prover, which prints out errors and warnings.
+    /// Given a number m, we prove that we know the product of the n numbers starting from it,
+    /// with n in range (1..=20).
+    /// If m is 1, as it is hard-coded here, we prove that we know the factorial of n.
+    /// The circuit is also tested against invalid witness values.
     fn mock_factorial_1_to_20() {
         const POW_OF_2_MAX_ROWS: u32 = 6;
 
@@ -235,6 +240,10 @@ mod tests {
     }
 
     #[test]
+    /// Test the factorial circuit with the mock prover, which prints out errors and warnings.
+    /// We test the product of 1000 numbers, variating the starting number, the number of columns in the circuit,
+    /// and the degree of the polynomial constraints imposed by the circuit.
+    /// This was done for performance testing, to figure out halo2's internals with a trial-and-error approach.
     fn mock_factorial_1000() {
         /// This macro exists because [`iter_apply_macro`] requires
         /// a macro argument. It mostly behaves like a generic function,
@@ -285,22 +294,23 @@ mod tests {
     }
 
     #[test]
+    /// Test the sudoku circuit with actual prover and verifier through the wrappers we implemented.
+    /// This is very similar to a real use case.
+    /// We prove that we know 1000! % m, where m is the maximum number Fp can represent.
     fn factorial() {
         const MAX_NR_ROWS_POW_2_EXPONENT: u32 = 4;
         const N_FACTORS: usize = 1000;
 
         use crate::utilities::{ProverWrapper, VerifierWrapper};
 
-        type TestCircuit = TruncatedFactorialCircuit<Fp, N_FACTORS, 20, 10>;
-
-        let circuit_wiring = TestCircuit::default();
+        let circuit_wiring = TruncatedFactorialCircuit::<Fp, N_FACTORS, 20, 10>::default();
 
         let mut prover = ProverWrapper::initialize_parameters_and_prover(
             MAX_NR_ROWS_POW_2_EXPONENT,
             circuit_wiring,
         )
         .expect("prover setup should not fail");
-        let circuit = TestCircuit::new(Fp::from(1));
+        let circuit = TruncatedFactorialCircuit::<Fp, N_FACTORS, 20, 10>::new(Fp::from(1));
 
         let instance = [(1..=N_FACTORS).fold(Fp::from(1), |acc, f| acc * Fp::from(f as u64))];
         let instance = [instance.as_slice()];

src/utilities/iter_apply_macro.rs

@@ -33,7 +33,7 @@
 #[macro_export]
 macro_rules! iter_apply_macro {
     ($macro_name: path; $([$($seq: expr),+])+) => {
-        crate::_inner_iter_apply_macro!($macro_name; {} $([$($seq,)+])+);
+        $crate::_inner_iter_apply_macro!($macro_name; {} $([$($seq,)+])+);
     };
 }
 
@@ -46,7 +46,7 @@ macro_rules! _inner_iter_apply_macro {
         [$seq1_first: expr, $($seq1: expr,)*]
         $($other_seq: tt)*
     ) => {
-        crate::_inner_iter_apply_macro!(
+        $crate::_inner_iter_apply_macro!(
             $macro_name;
             {$($params_list,)* $seq1_first, }
             $($other_seq)*

src/utilities/permutations_iter.rs

@@ -29,9 +29,8 @@ impl<const N_OBJECTS: usize> Default for KnuthL<N_OBJECTS> {
 impl<const N_OBJECTS: usize> Iterator for KnuthL<N_OBJECTS> {
     type Item = [usize; N_OBJECTS];
     fn next(&mut self) -> Option<Self::Item> {
-        if self.0 == None {
-            return None;
-        }
+        self.0?; // return None if None
+
         // Copy the current state, as return value.
         let current = self.0;
 
@@ -47,7 +46,7 @@ impl<const N_OBJECTS: usize> Iterator for KnuthL<N_OBJECTS> {
             .find(|&j| array[j] <= array[j + 1]);
 
         // The last permutation we yield is [N_OBJECTS - 1, N_OBJECTS - 2, ..., 1, 0]
-        if j == None {
+        if j.is_none() {
             self.0 = None;
             return current;
         }

src/utilities/proving_utilities.rs

@@ -121,7 +121,7 @@ impl<C: Circuit<Fp>> VerifierWrapper<C> {
         instances: I,
         transcript: &[u8],
     ) -> bool {
-        let instances = Vec::from_iter(instances.into_iter());
+        let instances = Vec::from_iter(instances);
 
         let mut transcript = Blake2bRead::init(transcript);
         let strategy = SingleVerifier::new(&self.public_parameters);

src/utilities/region_sequence_assignment.rs

@@ -33,9 +33,9 @@ impl<E, T, const LEN: usize> TryFrom<Result<[T; LEN], E>> for ArrayWrap<T, LEN>
     }
 }
 
-impl<T, const LEN: usize> Into<[T; LEN]> for ArrayWrap<T, LEN> {
-    fn into(self) -> [T; LEN] {
-        self.0
+impl<T, const LEN: usize> From<ArrayWrap<T, LEN>> for [T; LEN] {
+    fn from(val: ArrayWrap<T, LEN>) -> Self {
+        val.0
     }
 }