partiall address review comments of #670

Author: zhenfeizhang

aggregator/Cargo.toml

@@ -25,7 +25,7 @@ snark-verifier-sdk = { git = "https://github.com/scroll-tech/snark-verifier", br
 
 
 [features]
-default = [  ]
+default = [ "disable_proof_aggregation" ]
 print-trace = [ "ark-std/print-trace" ]
 # This feature is useful for unit tests where we check the SAT of pi aggregation circuit
 disable_proof_aggregation = []

aggregator/src/aggregation/circuit.rs

@@ -337,6 +337,8 @@ impl Circuit<Fr> for AggregationCircuit {
                             );
 
                             region.constrain_equal(
+                                // in the keccak table, the input and output date have different
+                                // endianess
                                 chunk_pi_hash_digests[i][j * 8 + k].cell(),
                                 snark_inputs[i * DIGEST_LEN + (3 - j) * 8 + k].cell(),
                             )?;

aggregator/src/aggregation/rlc/config.rs

@@ -64,7 +64,7 @@ impl RlcConfig {
             let q2 = meta.query_selector(enable_challenge);
             let cs2 = q2 * (a - challenge_expr.keccak_input());
 
-            vec![cs1 + cs2]
+            vec![cs1, cs2]
         });
         Self {
             #[cfg(test)]

aggregator/src/aggregation/rlc/gates.rs

@@ -16,12 +16,7 @@ impl RlcConfig {
         region.assign_fixed(|| "const one", self.fixed, 1, || Value::known(Fr::one()))?;
         region.assign_fixed(|| "const two", self.fixed, 2, || Value::known(Fr::from(2)))?;
         region.assign_fixed(|| "const four", self.fixed, 3, || Value::known(Fr::from(4)))?;
-        region.assign_fixed(
-            || "const eight",
-            self.fixed,
-            4,
-            || Value::known(Fr::from(8)),
-        )?;
+        region.assign_fixed(|| "const nine", self.fixed, 4, || Value::known(Fr::from(9)))?;
         region.assign_fixed(
             || "const thirty two",
             self.fixed,
@@ -68,7 +63,7 @@ impl RlcConfig {
     }
 
     #[inline]
-    pub(crate) fn eight_cell(&self, region_index: RegionIndex) -> Cell {
+    pub(crate) fn nine_cell(&self, region_index: RegionIndex) -> Cell {
         Cell {
             region_index,
             row_offset: 4,
@@ -288,7 +283,7 @@ impl RlcConfig {
         self.mul_add(region, b, &cond_not, &tmp, offset)
     }
 
-    // Returns inputs[0] + challenge * inputs[1] + ... + challenge^k * inputs[k]
+    // Returns challenge^k * inputs[0] + ... + challenge * inputs[k-1] + inputs[k]
     #[allow(dead_code)]
     pub(crate) fn rlc(
         &self,
@@ -304,7 +299,8 @@ impl RlcConfig {
         Ok(acc)
     }
 
-    // Returns inputs[0] + challenge * inputs[1] + ... + challenge^k * inputs[k]
+    // Returns challenge^k * inputs[0] * flag[0] + ... + challenge * inputs[k-1] * flag[k-1]] +
+    // inputs[k]* flag[k]
     pub(crate) fn rlc_with_flag(
         &self,
         region: &mut Region<Fr>,

aggregator/src/core.rs

@@ -100,7 +100,7 @@ pub(crate) fn extract_accumulators_and_proof(
 // 4. chunks are continuous: they are linked via the state roots
 // 5. batch and all its chunks use a same chain id
 // 6. chunk[i]'s prev_state_root == post_state_root when chunk[i] is padded
-// 7. chunk[i]'s data_hash == "" when chunk[i] is padded
+// 7. chunk[i]'s data_hash == keccak("") when chunk[i] is padded
 #[allow(clippy::type_complexity)]
 pub(crate) fn assign_batch_hashes(
     config: &AggregationConfig,
@@ -127,7 +127,7 @@ pub(crate) fn assign_batch_hashes(
     // 3. batch_data_hash and chunk[i].pi_hash use a same chunk[i].data_hash when chunk[i] is not
     // padded
     // 6. chunk[i]'s prev_state_root == post_state_root when chunk[i] is padded
-    // 7. chunk[i]'s data_hash == "" when chunk[i] is padded
+    // 7. chunk[i]'s data_hash == keccak("") when chunk[i] is padded
     let num_valid_snarks = conditional_constraints(
         &config.rlc_config,
         // config.flex_gate(),
@@ -405,7 +405,7 @@ fn copy_constraints(
 // 1. batch_data_hash digest is reused for public input hash
 // 3. batch_data_hash and chunk[i].pi_hash use a same chunk[i].data_hash when chunk[i] is not padded
 // 6. chunk[i]'s prev_state_root == post_state_root when chunk[i] is padded
-// 7. chunk[i]'s data_hash == "" when chunk[i] is padded
+// 7. chunk[i]'s data_hash == keccak("") when chunk[i] is padded
 #[allow(clippy::too_many_arguments)]
 pub(crate) fn conditional_constraints(
     rlc_config: &RlcConfig,
@@ -462,11 +462,11 @@ pub(crate) fn conditional_constraints(
                     region.constrain_equal(four_cell, four.cell())?;
                     four
                 };
-                let eight = {
-                    let eight = rlc_config.load_private(&mut region, &Fr::from(8), &mut offset)?;
-                    let eight_cell = rlc_config.eight_cell(eight.cell().region_index);
-                    region.constrain_equal(eight_cell, eight.cell())?;
-                    eight
+                let nine = {
+                    let nine = rlc_config.load_private(&mut region, &Fr::from(9), &mut offset)?;
+                    let nine_cell = rlc_config.nine_cell(nine.cell().region_index);
+                    region.constrain_equal(nine_cell, nine.cell())?;
+                    nine
                 };
                 let flag1 = rlc_config.is_smaller_than(
                     &mut region,
@@ -478,7 +478,7 @@ pub(crate) fn conditional_constraints(
                 let not_flag3 = rlc_config.is_smaller_than(
                     &mut region,
                     &num_of_valid_snarks_cell[0],
-                    &eight,
+                    &nine,
                     &mut offset,
                 )?;
                 let flag3 = rlc_config.not(&mut region, &not_flag3, &mut offset)?;
@@ -511,6 +511,11 @@ pub(crate) fn conditional_constraints(
                 //
                 // 1 batch_data_hash digest is reused for public input hash
                 //
+                // the following part of the code is hard coded for the case where
+                //   MAX_AGG_SNARKS <= 10
+                // in theory it may support up to 12 SNARKS (not tested)
+                // more SNARKs beyond 12 will require a revamp of the circuit
+                //
                 // public input hash is build as
                 //  keccak(
                 //      chain_id ||
@@ -519,6 +524,8 @@ pub(crate) fn conditional_constraints(
                 //      chunk[k-1].withdraw_root ||
                 //      batch_data_hash )
                 //
+                // batchDataHash = keccak(chunk[0].dataHash || ... || chunk[k-1].dataHash)
+                //
                 // #valid snarks | offset of data hash | flags
                 // 1,2,3,4       | 0                   | 1, 0, 0
                 // 5,6,7,8       | 32                  | 0, 1, 0
@@ -658,7 +665,7 @@ pub(crate) fn conditional_constraints(
                     }
                 }
 
-                // 7. chunk[i]'s data_hash == "" when chunk[i] is padded
+                // 7. chunk[i]'s data_hash == keccak("") when chunk[i] is padded
                 // that means the data_hash length is 32 * number_of_valid_snarks
                 let const32 = rlc_config.load_private(&mut region, &Fr::from(32), &mut offset)?;
                 let const32_cell = rlc_config.thirty_two_cell(const32.cell().region_index);

aggregator/src/util.rs

@@ -43,12 +43,12 @@ pub(crate) fn get_indices(preimages: &[Vec<u8>]) -> (Vec<usize>, Vec<usize>) {
     let mut round_ctr = 0;
 
     for preimage in preimages.iter().take(MAX_AGG_SNARKS + 1) {
-        //  136 = 17 * 8 is the size in bits of each
+        //  136 = 17 * 8 is the size in bytes of each
         //  input chunk that can be processed by Keccak circuit using absorb
         //  each chunk of size 136 needs 300 Keccak circuit rows to prove
         //  which consists of 12 Keccak rows for each of 24 + 1 Keccak circuit rounds
         //  digest only happens at the end of the last input chunk with
-        //  4 Keccak circuit rounds, so 48 Keccak rows, and 300 - 48 = 256
+        //  4 Keccak circuit rounds, so 48 Keccak rows, and 300 - 48 = 252
         let num_rounds = 1 + preimage.len() / INPUT_LEN_PER_ROUND;
         let mut preimage_padded = preimage.clone();
         preimage_padded.resize(INPUT_LEN_PER_ROUND * num_rounds, 0);
@@ -192,6 +192,9 @@ pub(crate) fn parse_hash_preimage_cells(
     Vec<&[AssignedCell<Fr, Fr>]>,
     &[AssignedCell<Fr, Fr>],
 ) {
+    // each pi hash has INPUT_LEN_PER_ROUND bytes as input
+    // keccak will pad the input with another INPUT_LEN_PER_ROUND bytes
+    // we extract all those bytes
     let batch_pi_hash_preimage = &hash_input_cells[0..INPUT_LEN_PER_ROUND * 2];
     let mut chunk_pi_hash_preimages = vec![];
     for i in 0..MAX_AGG_SNARKS {