chores: address review feedback

Author: hero78119

bus-mapping/src/circuit_input_builder.rs

@@ -48,6 +48,9 @@ use std::{
 pub use transaction::{Transaction, TransactionContext};
 pub use withdrawal::{Withdrawal, WithdrawalContext};
 
+/// number of execution state fields
+pub const N_EXEC_STATE: usize = 10;
+
 /// Runtime Config
 ///
 /// Default to mainnet block
@@ -341,6 +344,8 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         }
     }
 
+    // chunking and mutable bumping chunk_ctx once condition match
+    // return true on bumping to next chunk
     fn check_and_chunk(
         &mut self,
         geth_trace: &GethExecTrace,
@@ -493,15 +498,14 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         rw: RW,
         tx: Option<&Transaction>,
     ) {
-        let STEP_STATE_LEN = 10;
         let mut dummy_tx = Transaction::default();
         let mut dummy_tx_ctx = TransactionContext::default();
 
-        let rw_counters = (0..STEP_STATE_LEN)
+        let rw_counters = (0..N_EXEC_STATE)
             .map(|_| self.block_ctx.rwc.inc_pre())
             .collect::<Vec<RWCounter>>();
         // just bump rwc in chunk_ctx as block_ctx rwc to assure same delta apply
-        let rw_counters_inner_chunk = (0..STEP_STATE_LEN)
+        let rw_counters_inner_chunk = (0..N_EXEC_STATE)
             .map(|_| self.chunk_ctx.rwc.inc_pre())
             .collect::<Vec<RWCounter>>();
 
@@ -537,7 +541,7 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
             ]
         };
 
-        debug_assert_eq!(STEP_STATE_LEN, tags.len());
+        debug_assert_eq!(N_EXEC_STATE, tags.len());
         let state = self.state_ref(&mut dummy_tx, &mut dummy_tx_ctx);
 
         tags.iter()
@@ -865,15 +869,15 @@ fn push_op<T: Op>(
 impl<C: CircuitsParams> CircuitInputBuilder<C> {
     ///
     pub fn rws_reserve(&self) -> usize {
-        // This is the last chunk of a block, reserve for EndBlock, not EndChunk
+        // rw ops reserved for EndBlock
         let end_block_rws = if self.chunk_ctx.is_last_chunk() && self.chunk_rws() > 0 {
             1
         } else {
             0
         };
-        // This is not the last chunk, reserve for EndChunk
+        // rw ops reserved for EndChunk
         let end_chunk_rws = if !self.chunk_ctx.is_last_chunk() {
-            10
+            N_EXEC_STATE
         } else {
             0
         };

bus-mapping/src/operation.rs

@@ -116,10 +116,10 @@ pub enum Target {
     TxReceipt,
     /// Means the target of the operation is the TxLog.
     TxLog,
-    /// StepState
-    StepState,
 
-    /// padding operation.
+    /// Chunking: StepState
+    StepState,
+    /// Chunking: padding operation.
     Padding,
 }
 
@@ -916,7 +916,7 @@ pub enum StepStateField {
     LogID,
 }
 
-/// Represents an CallContext read/write operation.
+/// StepStateOp represents exec state store and load
 #[derive(Clone, PartialEq, Eq)]
 pub struct StepStateOp {
     /// field of CallContext

zkevm-circuits/src/root_circuit.rs

@@ -286,8 +286,8 @@ where
                     config.aggregate::<M, As>(ctx, &key.clone(), &self.snark_witnesses)?;
 
                 // aggregate user challenge for rwtable permutation challenge
-                let (_alpha, _gamma) = {
-                    let mut challenges = config.aggregate_user_challenges::<M, As>(
+                let (alpha, gamma) = {
+                    let (mut challenges, _) = config.aggregate_user_challenges::<M, As>(
                         loader.clone(),
                         self.user_challenges,
                         proofs,
@@ -330,20 +330,6 @@ where
                             (zero_const, one_const, total_chunk_const)
                         };
 
-                        // TODO remove me
-                        let (_hardcode_alpha, _hardcode_gamma) = {
-                            (
-                                loader
-                                    .scalar_chip()
-                                    .assign_constant(&mut loader.ctx_mut(), M::Fr::from(101))
-                                    .unwrap(),
-                                loader
-                                    .scalar_chip()
-                                    .assign_constant(&mut loader.ctx_mut(), M::Fr::from(103))
-                                    .unwrap(),
-                            )
-                        };
-
                         // `first.sc_rwtable_row_prev_fingerprint ==
                         // first.ec_rwtable_row_prev_fingerprint` will be checked inside circuit
                         vec![
@@ -354,17 +340,11 @@ where
                             (first_chunk.initial_rwc.assigned(), &one_const),
                             // constraint permutation fingerprint
                             // challenge: alpha
-                            // TODO remove hardcode
-                            (first_chunk.sc_permu_alpha.assigned(), &_hardcode_alpha),
-                            (first_chunk.ec_permu_alpha.assigned(), &_hardcode_alpha),
-                            // (first_chunk.sc_permu_alpha.assigned(), &alpha.assigned()),
-                            // (first_chunk.ec_permu_alpha.assigned(), &alpha.assigned()),
+                            (first_chunk.sc_permu_alpha.assigned(), &alpha.assigned()),
+                            (first_chunk.ec_permu_alpha.assigned(), &alpha.assigned()),
                             // challenge: gamma
-                            // TODO remove hardcode
-                            (first_chunk.sc_permu_gamma.assigned(), &_hardcode_gamma),
-                            (first_chunk.ec_permu_gamma.assigned(), &_hardcode_gamma),
-                            // (first_chunk.sc_permu_gamma.assigned(), &gamma.assigned()),
-                            // (first_chunk.ec_permu_gamma.assigned(), &gamma.assigned()),
+                            (first_chunk.sc_permu_gamma.assigned(), &gamma.assigned()),
+                            (first_chunk.ec_permu_gamma.assigned(), &gamma.assigned()),
                             // fingerprint
                             (
                                 first_chunk.ec_rwtable_prev_fingerprint.assigned(),

zkevm-circuits/src/root_circuit/aggregation.rs

@@ -15,7 +15,7 @@ use snark_verifier::{
         self,
         halo2::{
             halo2_wrong_ecc::{self, integer::rns::Rns, maingate::*, EccConfig},
-            Scalar,
+            EcPoint, Scalar,
         },
         native::NativeLoader,
     },
@@ -233,7 +233,13 @@ impl AggregationConfig {
         loader: Rc<Halo2Loader<'c, M::G1Affine>>,
         user_challenges: Option<&UserChallenge>,
         proofs: Vec<PlonkProof<M::G1Affine, Rc<Halo2Loader<'c, M::G1Affine>>, As>>,
-    ) -> Result<Vec<LoadedScalar<'c, M::G1Affine>>, Error>
+    ) -> Result<
+        (
+            Vec<LoadedScalar<'c, M::G1Affine>>,
+            Vec<EcPoint<'c, M::G1Affine, EccChip<M::G1Affine>>>,
+        ),
+        Error,
+    >
     where
         M: MultiMillerLoop,
         M::Fr: Field,
@@ -253,8 +259,6 @@ impl AggregationConfig {
         type PoseidonTranscript<'a, C, S> =
             transcript::halo2::PoseidonTranscript<C, Rc<Halo2Loader<'a, C>>, S, T, RATE, R_F, R_P>;
 
-        // Verify the cheap part and get accumulator (left-hand and right-hand side of
-        // pairing) of individual proof.
         let witnesses = proofs
             .iter()
             .flat_map(|proof| {
@@ -279,9 +283,11 @@ impl AggregationConfig {
             .map(|user_challenges| user_challenges.num_challenges)
             .unwrap_or_default();
 
-        Ok((0..num_challenges)
-            .map(|_| transcript.squeeze_challenge())
-            .collect_vec())
+        let witnesses = witnesses
+            .into_iter()
+            .cloned()
+            .collect::<Vec<EcPoint<'c, M::G1Affine, EccChip<M::G1Affine>>>>();
+        Ok((transcript.squeeze_n_challenges(num_challenges), witnesses))
     }
 
     /// Aggregate snarks into a single accumulator and decompose it into
@@ -333,6 +339,7 @@ impl AggregationConfig {
             .iter()
             .map(|snark| {
                 let protocol = snark.protocol.loaded(&loader);
+
                 let instances = snark.loaded_instances(&loader);
                 let mut transcript = PoseidonTranscript::new(&loader, snark.proof());
                 let proof = PlonkSuccinctVerifier::<As>::read_proof(

zkevm-circuits/src/root_circuit/dev.rs

@@ -22,13 +22,14 @@ use std::{iter, marker::PhantomData, rc::Rc};
 
 /// Aggregation circuit for testing purpose.
 #[derive(Clone)]
+#[allow(clippy::type_complexity)]
 pub struct TestAggregationCircuit<'a, M: MultiMillerLoop, As>
 where
     M::G1Affine: CurveAffine,
 {
     svk: KzgSvk<M>,
     snarks: Vec<SnarkWitness<'a, M::G1Affine>>,
-    user_challenge: Option<(UserChallenge, Vec<M::Fr>)>,
+    user_challenge: Option<(UserChallenge, Vec<M::G1Affine>, Vec<M::Fr>)>,
     instances: Vec<M::Fr>,
     _marker: PhantomData<As>,
 }
@@ -53,10 +54,11 @@ where
 {
     /// Create an Aggregation circuit with aggregated accumulator computed.
     /// Returns `None` if any given snark is invalid.
+    #[allow(clippy::type_complexity)]
     pub fn new(
         params: &ParamsKZG<M>,
         snarks: impl IntoIterator<Item = Snark<'a, M::G1Affine>>,
-        user_challenge: Option<(UserChallenge, Vec<M::Fr>)>,
+        user_challenge: Option<(UserChallenge, Vec<M::G1Affine>, Vec<M::Fr>)>,
     ) -> Result<Self, snark_verifier::Error> {
         let snarks = snarks.into_iter().collect_vec();
 
@@ -156,16 +158,43 @@ where
                     config.aggregate::<M, As>(ctx, &self.svk, &self.snarks)?;
 
                 // aggregate user challenge for rwtable permutation challenge
-                let user_challenge = self.user_challenge.as_ref().map(|(challenge, _)| challenge);
-                let challenges = config.aggregate_user_challenges::<M, As>(
+                let user_challenge = self
+                    .user_challenge
+                    .as_ref()
+                    .map(|(challenge, _, _)| challenge);
+                let (challenges, commitments) = config.aggregate_user_challenges::<M, As>(
                     loader.clone(),
                     user_challenge,
                     proofs,
                 )?;
                 if !challenges.is_empty() {
-                    let Some((_, expected_challenges)) = self.user_challenge.as_ref() else {
+                    let Some((_, expected_commitments, expected_challenges)) =
+                        self.user_challenge.as_ref()
+                    else {
                         return Err(InvalidInstances);
                     };
+                    // check commitment equality
+                    let expected_commitments_loaded = expected_commitments
+                        .iter()
+                        .map(|expected_commitment| {
+                            loader.ecc_chip().assign_point(
+                                &mut loader.ctx_mut(),
+                                Value::known(*expected_commitment),
+                            )
+                        })
+                        .collect::<Result<Vec<_>, Error>>()?;
+                    expected_commitments_loaded
+                        .iter()
+                        .zip(commitments.iter())
+                        .try_for_each(|(expected_commitment, commitment)| {
+                            loader.ecc_chip().assert_equal(
+                                &mut loader.ctx_mut(),
+                                expected_commitment,
+                                &commitment.assigned(),
+                            )
+                        })?;
+
+                    // check challenge equality
                     let expected_challenges_loaded = expected_challenges
                         .iter()
                         .map(|value| loader.assign_scalar(Value::known(*value)))