Fix: constant column of PI circuit is not fixed

zkevm-circuits/src/pi_circuit.rs

@@ -45,6 +45,7 @@ use halo2_proofs::{
 use once_cell::sync::Lazy;
 
 use crate::{
+    evm_circuit::param::{N_BYTES_ACCOUNT_ADDRESS, N_BYTES_U64, N_BYTES_WORD},
     pi_circuit::param::{COINBASE_OFFSET, DIFFICULTY_OFFSET},
     table::BlockContextFieldTag::{
         BaseFee, ChainId, Coinbase, CumNumTxs, Difficulty, GasLimit, NumTxs, Number, Timestamp,
@@ -353,7 +354,7 @@ impl<F: Field> SubCircuitConfig<F> for PiCircuitConfig<F> {
                 let rpi_bytes_next = meta.query_advice(rpi_bytes, Rotation::next());
                 let keccak_rand = challenges.keccak_input();
                 let evm_rand = challenges.evm_word();
-                let is_rlc_keccak = meta.query_fixed(is_rlc_keccak, Rotation::cur());
+                let is_rlc_keccak = meta.query_fixed(is_rlc_keccak, Rotation::next());
                 let r = select::expr(is_rlc_keccak, keccak_rand, evm_rand);
 
                 cb.require_equal(
@@ -612,6 +613,10 @@ impl<F: Field> PiCircuitConfig<F> {
         ///////////////////////////////////
         ///////  assign data bytes ////////
         ///////////////////////////////////
+        let data_bytes_start_row = 0;
+        let data_bytes_end_row =
+            self.max_inner_blocks * BLOCK_HEADER_BYTES_NUM + self.max_txs * KECCAK_DIGEST_SIZE;
+        self.assign_rlc_start(region, &mut offset, &mut rpi_rlc_acc, &mut rpi_length_acc)?;
         // assign block contexts
         for (i, block) in block_values
             .ctxs
@@ -665,7 +670,10 @@ impl<F: Field> PiCircuitConfig<F> {
 
             block_table_offset += BLOCK_LEN;
         }
-        for i in 0..offset {
+
+        let q_block_context_start_row = 1;
+        let q_block_context_end_row = 1 + BLOCK_HEADER_BYTES_NUM * self.max_inner_blocks;
+        for i in q_block_context_start_row..q_block_context_end_row {
             // assign q_block_context
             region.assign_fixed(
                 || "q_block_context",
@@ -675,10 +683,11 @@ impl<F: Field> PiCircuitConfig<F> {
             )?;
         }
 
-        debug_assert_eq!(offset, BLOCK_HEADER_BYTES_NUM * self.max_inner_blocks);
+        debug_assert_eq!(offset, 1 + BLOCK_HEADER_BYTES_NUM * self.max_inner_blocks);
 
         // assign tx hashes
-        let block_context_row = offset;
+        let q_tx_hashes_start_row = offset;
+        let q_tx_hashes_end_row = q_tx_hashes_start_row + KECCAK_DIGEST_SIZE * self.max_txs;
         let num_txs = tx_hashes.len();
         let mut data_bytes_rlc = None;
         let mut data_bytes_length = None;
@@ -711,7 +720,7 @@ impl<F: Field> PiCircuitConfig<F> {
                 data_bytes_length = Some(cells[RPI_LENGTH_ACC_CELL_IDX].clone());
             }
         }
-        for i in block_context_row..offset {
+        for i in q_tx_hashes_start_row..q_tx_hashes_end_row {
             region.assign_fixed(
                 || "q_tx_hashes",
                 self.q_tx_hashes,
@@ -720,13 +729,10 @@ impl<F: Field> PiCircuitConfig<F> {
             )?;
         }
 
-        debug_assert_eq!(
-            offset,
-            BLOCK_HEADER_BYTES_NUM * self.max_inner_blocks + KECCAK_DIGEST_SIZE * self.max_txs
-        );
+        assert_eq!(offset, data_bytes_end_row + 1);
 
-        // the last row of data bytes part is offset - 1
-        for i in 0..(offset - 1) {
+        // the last row of data bytes part is disabled
+        for i in data_bytes_start_row..data_bytes_end_row {
             self.q_not_end.enable(region, i)?;
         }
 
@@ -773,15 +779,14 @@ impl<F: Field> PiCircuitConfig<F> {
             || data_hash_rlc,
         )?;
         self.q_keccak.enable(region, data_hash_row)?;
+        offset += 1;
 
         /////////////////////////////////
         ///////// assign pi bytes ///////
         /////////////////////////////////
-        // reset
-        rpi_rlc_acc = Value::known(F::zero());
-        rpi_length_acc = 0;
-
-        offset += 1;
+        let pi_bytes_start_row = offset;
+        let pi_bytes_end_row = pi_bytes_start_row + N_BYTES_U64 + N_BYTES_WORD * 4;
+        self.assign_rlc_start(region, &mut offset, &mut rpi_rlc_acc, &mut rpi_length_acc)?;
         // assign chain_id
         let cells = self.assign_field_in_pi(
             region,
@@ -873,10 +878,13 @@ impl<F: Field> PiCircuitConfig<F> {
         // copy data_hash down here
         region.constrain_equal(data_hash_rlc_cell.cell(), data_hash_cell.cell())?;
 
-        for i in (data_hash_row + 1)..(offset - 1) {
+        for i in pi_bytes_start_row..pi_bytes_end_row {
             self.q_not_end.enable(region, i)?;
         }
 
+        // +1 for the rlc_start row
+        assert_eq!(offset, pi_bytes_end_row + 1);
+
         // assign keccak row for computing pi_hash = keccak256(pi_bytes)
         let pi_hash_row = offset;
         pi_bytes_rlc.copy_advice(
@@ -900,19 +908,19 @@ impl<F: Field> PiCircuitConfig<F> {
             || pi_hash_rlc,
         )?;
         self.q_keccak.enable(region, pi_hash_row)?;
+        offset += 1;
 
         //////////////////////////////////////////////////
         //// assign pi_hash (high, low)-decomposition ////
         //////////////////////////////////////////////////
+        let pi_hash_bytes_start_row = offset;
+        let pi_hash_bytes_end_row = pi_hash_bytes_start_row + KECCAK_DIGEST_SIZE;
+        self.assign_rlc_start(region, &mut offset, &mut rpi_rlc_acc, &mut rpi_length_acc)?;
 
-        // reset
-        rpi_rlc_acc = Value::known(F::zero());
-        rpi_length_acc = 0;
-        offset += 1;
-
-        for i in offset..(offset + 31) {
+        for i in pi_hash_bytes_start_row..pi_hash_bytes_end_row {
             self.q_not_end.enable(region, i)?;
         }
+
         // the high 16 bytes of keccak output
         let cells = self.assign_field_in_pi(
             region,
@@ -943,17 +951,20 @@ impl<F: Field> PiCircuitConfig<F> {
         let pi_hash_lo_byte_cells = cells[3..].to_vec();
         // let pi_hash_lo_cell = cells[RPI_CELL_IDX].clone();
 
+        // +1 for the rlc_start row
+        assert_eq!(offset, pi_hash_bytes_end_row + 1);
+
         // copy pi hash down here
         region.constrain_equal(pi_hash_rlc_cell.cell(), cells[RPI_RLC_ACC_CELL_IDX].cell())?;
 
         //////////////////////////////////////////////////
         ////////// assign COINBASE, DIFFICULTY  //////////
         //////////////////////////////////////////////////
-        // reset
-        rpi_rlc_acc = Value::known(F::zero());
-        rpi_length_acc = 0;
+        let coinbase_diff_start_row = offset;
+        let coinbase_diff_end_row =
+            coinbase_diff_start_row + N_BYTES_ACCOUNT_ADDRESS + N_BYTES_WORD;
 
-        let off_start = offset;
+        self.assign_rlc_start(region, &mut offset, &mut rpi_rlc_acc, &mut rpi_length_acc)?;
 
         let cells = self.assign_field_in_pi_ext(
             region,
@@ -983,7 +994,7 @@ impl<F: Field> PiCircuitConfig<F> {
         )?;
         let difficulty_cell = cells[RPI_CELL_IDX].clone();
 
-        for i in off_start..(offset - 1) {
+        for i in coinbase_diff_start_row..coinbase_diff_end_row {
             self.q_not_end.enable(region, i)?;
         }
 
@@ -999,6 +1010,23 @@ impl<F: Field> PiCircuitConfig<F> {
             )?;
         }
 
+        assert_eq!(
+            offset,
+            // for data bytes start row
+            1 + self.max_inner_blocks * BLOCK_HEADER_BYTES_NUM
+                + self.max_txs * KECCAK_DIGEST_SIZE
+                + 1 // for data hash row
+                + 1 // for pi bytes start row
+                + N_BYTES_U64
+                + 4 * KECCAK_DIGEST_SIZE
+                + 1 // for pi hash row
+                + 1 // for pi hash bytes start row
+                + KECCAK_DIGEST_SIZE
+                + 1 // for coinbase & difficulty start row
+                + N_BYTES_ACCOUNT_ADDRESS
+                + N_BYTES_WORD,
+        );
+
         let instance_byte_cells = [
             chain_id_byte_cells,
             pi_hash_hi_byte_cells,
@@ -1009,6 +1037,33 @@ impl<F: Field> PiCircuitConfig<F> {
         Ok((instance_byte_cells, connections))
     }
 
+    fn assign_rlc_start(
+        &self,
+        region: &mut Region<'_, F>,
+        offset: &mut usize,
+        rpi_rlc_acc: &mut Value<F>,
+        rpi_length_acc: &mut u64,
+    ) -> Result<(), Error> {
+        *rpi_rlc_acc = Value::known(F::zero());
+        *rpi_length_acc = 0;
+
+        region.assign_advice_from_constant(
+            || "rpi_rlc_acc[0]",
+            self.rpi_rlc_acc,
+            *offset,
+            F::zero(),
+        )?;
+        region.assign_advice_from_constant(
+            || "rpi_length_acc[0]",
+            self.rpi_length_acc,
+            *offset,
+            F::zero(),
+        )?;
+        *offset += 1;
+
+        Ok(())
+    }
+
     #[allow(clippy::too_many_arguments)]
     fn assign_field_in_pi(
         &self,
@@ -1077,7 +1132,6 @@ impl<F: Field> PiCircuitConfig<F> {
         let mut rpi_bytes_acc_cells = vec![];
         let mut byte_cells = vec![];
         for (i, byte) in value_be_bytes.iter().enumerate() {
-            let is_rlc_start = *rpi_length_acc == 0;
             let row_offset = *offset + i;
 
             let real_value = if is_rpi_padding {
@@ -1150,25 +1204,14 @@ impl<F: Field> PiCircuitConfig<F> {
                 row_offset,
                 || *rpi_rlc_acc,
             )?;
-            let rpi_length_cell = if is_rlc_start {
-                region.assign_advice_from_constant(
-                    || "rpi_length_acc",
-                    self.rpi_length_acc,
-                    row_offset,
-                    F::one(),
-                )?
-            } else {
+            let rpi_length_cell = {
                 region.assign_advice(
                     || "rpi_length_acc",
                     self.rpi_length_acc,
                     row_offset,
                     || Value::known(F::from(*rpi_length_acc)),
                 )?
             };
-            if is_rlc_start {
-                // use copy constraint to make sure that rpi_rlc_acc == byte at start
-                region.constrain_equal(rpi_rlc_cell.cell(), byte_cell.cell())?;
-            }
 
             region.assign_advice(
                 || "is_rpi_padding",

zkevm-circuits/src/pi_circuit/test.rs

@@ -68,6 +68,14 @@ fn block_2txs() -> Block<Fr> {
     block_convert(&builder.block, &builder.code_db).unwrap()
 }
 
+fn empty_block() -> Block<Fr> {
+    Block::<Fr> {
+        txs: vec![],
+        sigs: vec![],
+        ..Default::default()
+    }
+}
+
 #[cfg(feature = "scroll")]
 #[test]
 fn serial_test_simple_pi() {
@@ -105,13 +113,13 @@ fn run_size_check<
     let public_inputs = circuit.0.instance();
     let prover1 = MockProver::run(20, &circuit, public_inputs).unwrap();
 
-    let circuit2 = PiTestCircuit::<F, MAX_TXS, MAX_CALLDATA, MAX_INNER_BLOCKS>(PiCircuit::new(
+    let circuit = PiTestCircuit::<F, MAX_TXS, MAX_CALLDATA, MAX_INNER_BLOCKS>(PiCircuit::new(
         MAX_TXS,
         MAX_CALLDATA,
         MAX_INNER_BLOCKS,
         &blocks[1],
     ));
-    let public_inputs = circuit2.0.instance();
+    let public_inputs = circuit.0.instance();
     let prover2 = MockProver::run(20, &circuit, public_inputs).unwrap();
 
     assert_eq!(prover1.fixed(), prover2.fixed());
@@ -124,8 +132,10 @@ fn variadic_size_check() {
     const MAX_CALLDATA: usize = 200;
     const MAX_INNER_BLOCKS: usize = 4;
 
+    let block_0 = empty_block();
     let block_1 = block_1tx();
     let block_2 = block_2txs();
 
-    run_size_check::<Fr, MAX_TXS, MAX_CALLDATA, MAX_INNER_BLOCKS>([block_1, block_2]);
+    run_size_check::<Fr, MAX_TXS, MAX_CALLDATA, MAX_INNER_BLOCKS>([block_1, block_2.clone()]);
+    run_size_check::<Fr, MAX_TXS, MAX_CALLDATA, MAX_INNER_BLOCKS>([block_0, block_2]);
 }