add more tests related intermediate chunk and fixed bugs

Author: hero78119

bus-mapping/src/circuit_input_builder.rs

@@ -357,7 +357,7 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         // Optain the first op of the next GethExecStep, for fixed case also lookahead
         let (mut cib, mut tx, mut tx_ctx) = (self.clone(), tx, tx_ctx);
         let mut cib_ref = cib.state_ref(&mut tx, &mut tx_ctx);
-        let next_ops = if let Some((i, step)) = next_geth_step {
+        let mut next_ops = if let Some((i, step)) = next_geth_step {
             log::trace!("chunk at {}th opcode {:?} ", i, step.op);
             gen_associated_ops(&step.op, &mut cib_ref, &geth_trace.struct_logs[i..])?.remove(0)
         } else {
@@ -368,10 +368,14 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         let last_copy = self.block.copy_events.len();
         // Generate EndChunk and proceed to the next if it's not the last chunk
         // Set next step pre-state as end_chunk state
-        self.set_end_chunk(&next_ops);
+        self.set_end_chunk(&next_ops, Some(&tx));
+
+        // need to update next_ops.rwc to catch block_ctx.rwc in `set_end_chunk`
+        next_ops.rwc = self.block_ctx.rwc;
+
         // tx.id start from 1, so it's equivalent to `next_tx_index`
         self.commit_chunk_ctx(true, tx.id as usize, last_copy, last_call);
-        self.set_begin_chunk(&next_ops);
+        self.set_begin_chunk(&next_ops, Some(&tx));
 
         Ok(())
     }
@@ -394,13 +398,12 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         let mut tx_ctx = TransactionContext::new(eth_tx, geth_trace, is_last_tx)?;
 
         let res = if !geth_trace.invalid {
-            let mut last_call = None;
-
             // Generate BeginTx step
             let begin_tx_step = gen_associated_steps(
                 &mut self.state_ref(&mut tx, &mut tx_ctx),
                 ExecState::BeginTx,
             )?;
+            let mut last_call = Some(tx.calls().get(begin_tx_step.call_index).unwrap().clone());
             tx.steps_mut().push(begin_tx_step);
 
             let mut trace = geth_trace.struct_logs.iter().enumerate().peekable();
@@ -416,9 +419,10 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
                 // Proceed to the next step
                 let (i, step) = trace.next().expect("Peeked step should exist");
                 log::trace!(
-                    "handle {}th opcode {:?} rws = {:?}",
+                    "handle {}th opcode {:?} {:?} rws = {:?}",
                     i,
                     step.op,
+                    step,
                     self.chunk_rws()
                 );
                 let exec_steps = gen_associated_ops(
@@ -466,9 +470,13 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         Ok(res)
     }
 
-    // TODO Fix this, for current logic on processing `call` is incorrect
-    // TODO re-design `gen_chunk_associated_steps` to separate RW
-    fn gen_chunk_associated_steps(&mut self, step: &mut ExecStep, rw: RW) {
+    // generate chunk related steps
+    fn gen_chunk_associated_steps(
+        &mut self,
+        step: &mut ExecStep,
+        rw: RW,
+        tx: Option<&Transaction>,
+    ) {
         let STEP_STATE_LEN = 10;
         let mut dummy_tx = Transaction::default();
         let mut dummy_tx_ctx = TransactionContext::default();
@@ -483,12 +491,10 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
 
         let tags = {
             let state = self.state_ref(&mut dummy_tx, &mut dummy_tx_ctx);
-            let last_call = state
-                .block
-                .txs
-                .last()
-                .map(|tx| tx.calls[0].clone())
-                .unwrap_or_else(Call::default);
+            let last_call = tx
+                .map(|tx| tx.calls()[step.call_index].clone())
+                .or_else(|| state.block.txs.last().map(|tx| tx.calls[0].clone()))
+                .unwrap();
             [
                 (StepStateField::CodeHash, last_call.code_hash.to_word()),
                 (StepStateField::CallID, Word::from(last_call.call_id)),
@@ -551,24 +557,44 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         self.chunk_ctx.end_copy_index = next_copy_index;
         self.cur_chunk_mut().ctx = self.chunk_ctx.clone();
         if to_next {
+            // here use `-1` to include previous set
             self.chunk_ctx
-                .bump(self.block_ctx.rwc.0, next_tx_index, next_copy_index);
+                .bump(self.block_ctx.rwc.0, next_tx_index - 1, next_copy_index);
+            // println!("bump last_call {:?}", last_call);
+            if last_call.is_none() {
+                panic!("??")
+            }
             self.cur_chunk_mut().prev_last_call = last_call;
         }
     }
 
-    fn set_begin_chunk(&mut self, first_step: &ExecStep) {
-        let mut begin_chunk = first_step.clone();
-        begin_chunk.exec_state = ExecState::BeginChunk;
-        self.gen_chunk_associated_steps(&mut begin_chunk, RW::READ);
+    fn set_begin_chunk(&mut self, first_step: &ExecStep, tx: Option<&Transaction>) {
+        let mut begin_chunk = ExecStep {
+            exec_state: ExecState::BeginChunk,
+            rwc: first_step.rwc,
+            gas_left: first_step.gas_left,
+            call_index: first_step.call_index,
+            ..ExecStep::default()
+        };
+        self.gen_chunk_associated_steps(&mut begin_chunk, RW::READ, tx);
+        println!("in set begin chunk {:?}", begin_chunk);
         self.chunks[self.chunk_ctx.idx].begin_chunk = Some(begin_chunk);
     }
 
-    fn set_end_chunk(&mut self, next_step: &ExecStep) {
-        let mut end_chunk = next_step.clone();
-        end_chunk.exec_state = ExecState::EndChunk;
-        self.gen_chunk_associated_steps(&mut end_chunk, RW::WRITE);
+    fn set_end_chunk(&mut self, next_step: &ExecStep, tx: Option<&Transaction>) {
+        println!("before self.block_ctx.rwc.0 {}", self.block_ctx.rwc.0);
+        // println!("next step {:?}", next_step);
+        let mut end_chunk = ExecStep {
+            exec_state: ExecState::EndChunk,
+            rwc: next_step.rwc,
+            rwc_inner_chunk: next_step.rwc_inner_chunk,
+            gas_left: next_step.gas_left,
+            call_index: next_step.call_index,
+            ..ExecStep::default()
+        };
+        self.gen_chunk_associated_steps(&mut end_chunk, RW::WRITE, tx);
         self.gen_chunk_padding(&mut end_chunk);
+        println!("after self.block_ctx.rwc.0 {}", self.block_ctx.rwc.0);
         self.chunks[self.chunk_ctx.idx].end_chunk = Some(end_chunk);
     }
 
@@ -578,8 +604,6 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         let total_rws = end_rwc - 1;
         let max_rws = self.cur_chunk().fixed_param.max_rws;
 
-        // We need at least 1 extra row at offset 0 for chunk continuous
-        // FIXME(Cecilia): adding + 1 fail some tests
         assert!(
             total_rws < max_rws,
             "total_rws <= max_rws, total_rws={}, max_rws={}",
@@ -707,17 +731,26 @@ impl CircuitInputBuilder<FixedCParams> {
         eth_block: &EthBlock,
         geth_traces: &[eth_types::GethExecTrace],
     ) -> Result<CircuitInputBuilder<FixedCParams>, Error> {
-        println!("--------------{:?}", self.circuits_params);
         // accumulates gas across all txs in the block
         let (last_step, last_call) = self.begin_handle_block(eth_block, geth_traces)?;
-        let last_step = last_step.unwrap_or_default();
+        // since there is no next step, we cook dummy next step from last step to reuse
+        // existing field while update its `rwc`.
+        let mut dummy_next_step = {
+            let mut dummy_next_step = last_step.unwrap_or_default();
+            // raise last step rwc to match with next step
+            (0..dummy_next_step.rw_indices_len()).for_each(|_| {
+                dummy_next_step.rwc.inc_pre();
+                dummy_next_step.rwc_inner_chunk.inc_pre();
+            });
+            dummy_next_step
+        };
 
         assert!(self.circuits_params.max_rws().is_some());
 
         let last_copy = self.block.copy_events.len();
 
         // TODO figure out and resolve generic param type and move fixed_param set inside
-        // commit_chunk_ctx After fixed, then we can set fixed_param on all chunks
+        // commit_chunk_ctx. After fixed, then we can set fixed_param on all chunks
         (0..self.circuits_params.total_chunks()).for_each(|idx| {
             self.get_chunk_mut(idx).fixed_param = self.circuits_params;
         });
@@ -734,16 +767,29 @@ impl CircuitInputBuilder<FixedCParams> {
                     last_call.clone(),
                 );
             } else {
-                // it doent matter what step was put to set_end_chunk/set_begin_chunk on no-used
-                // chunks before end_block. Just need to make sure it's step lookup is consistency.
-                self.set_end_chunk(&last_step);
+                self.set_end_chunk(&dummy_next_step, None);
+
                 self.commit_chunk_ctx(
                     true,
                     eth_block.transactions.len(),
                     last_copy,
                     last_call.clone(),
                 );
-                self.set_begin_chunk(&last_step);
+                // update dummy_next_step rwc to be used for next
+                dummy_next_step.rwc = self.block_ctx.rwc;
+                dummy_next_step.rwc_inner_chunk = self.chunk_ctx.rwc;
+                self.set_begin_chunk(&dummy_next_step, None);
+                dummy_next_step.rwc = self.block_ctx.rwc;
+                dummy_next_step.rwc_inner_chunk = self.chunk_ctx.rwc;
+                // update virtual step: end_block/padding so it can carry state context correctly
+                // TODO: enhance virtual step updating mechanism by having `running_next_step`
+                // defined in circuit_input_builder, so we dont need to
+                self.block.end_block = dummy_next_step.clone();
+                self.cur_chunk_mut().padding = {
+                    let mut padding = dummy_next_step.clone();
+                    padding.exec_state = ExecState::Padding;
+                    Some(padding)
+                };
             }
             Ok::<(), Error>(())
         })?;
@@ -773,6 +819,7 @@ impl CircuitInputBuilder<FixedCParams> {
     fn set_end_block(&mut self) -> Result<(), Error> {
         let mut end_block = self.block.end_block.clone();
         end_block.rwc = self.block_ctx.rwc;
+        end_block.exec_state = ExecState::EndBlock;
         end_block.rwc_inner_chunk = self.chunk_ctx.rwc;
 
         let mut dummy_tx = Transaction::default();

testool/src/statetest/executor.rs

@@ -348,12 +348,9 @@ pub fn run_test(
 
         let block: Block<Fr> =
             zkevm_circuits::evm_circuit::witness::block_convert(&builder).unwrap();
-        let chunk: Chunk<Fr> =
-            zkevm_circuits::evm_circuit::witness::chunk_convert(&block, &builder)
-                .unwrap()
-                .remove(0);
-
-        CircuitTestBuilder::<1, 1>::new_from_block(block, chunk)
+        let chunks: Vec<Chunk<Fr>> =
+            zkevm_circuits::evm_circuit::witness::chunk_convert(&block, &builder).unwrap();
+        CircuitTestBuilder::<1, 1>::new_from_block(block, chunks)
             .run_with_result()
             .map_err(|err| match err {
                 CircuitTestError::VerificationFailed { reasons, .. } => {

zkevm-circuits/src/evm_circuit.rs

@@ -638,7 +638,9 @@ mod evm_circuit_stats {
             TestContext::<0, 0>::new(None, |_| {}, |_, _| {}, |b, _| b).unwrap(),
         )
         .block_modifier(Box::new(|_block, chunk| {
-            chunk.fixed_param.max_evm_rows = (1 << 18) - 100
+            chunk
+                .iter_mut()
+                .for_each(|chunk| chunk.fixed_param.max_evm_rows = (1 << 18) - 100);
         }))
         .run();
     }

zkevm-circuits/src/evm_circuit/execution.rs

@@ -815,6 +815,10 @@ impl<F: Field> ExecutionConfig<F> {
 
         let step_curr_rw_counter = cb.curr.state.rw_counter.clone();
         let step_curr_rw_counter_offset = cb.rw_counter_offset();
+        if execution_state == ExecutionState::BeginChunk {
+            cb.debug_expression("step_curr_rw_counter.expr()", step_curr_rw_counter.expr());
+        }
+
         let debug_expressions = cb.debug_expressions.clone();
 
         // Extract feature config here before cb is built.
@@ -979,9 +983,10 @@ impl<F: Field> ExecutionConfig<F> {
                                 .collect(),
                         ),
                         (
-                            "Only EndTx or InvalidTx or EndBlock or Padding can transit to EndBlock",
+                            "Only BeginChunk or EndTx or InvalidTx or EndBlock or Padding can transit to EndBlock",
                             ExecutionState::EndBlock,
                             vec![
+                                ExecutionState::BeginChunk,
                                 ExecutionState::EndTx,
                                 ExecutionState::EndBlock,
                                 ExecutionState::Padding,
@@ -1125,24 +1130,26 @@ impl<F: Field> ExecutionConfig<F> {
                 self.q_step_first.enable(&mut region, offset)?;
 
                 let dummy_tx = Transaction::default();
-                let chunk_txs = block
+                // chunk_txs is just a super set of execstep including both belong to this chunk and
+                // outside of this chunk
+                let chunk_txs: &[Transaction] = block
                     .txs
                     .get(chunk.chunk_context.initial_tx_index..chunk.chunk_context.end_tx_index)
                     .unwrap_or_default();
 
-                let last_call = chunk_txs
+                // If it's the very first chunk in a block set last call & begin_chunk to default
+                let prev_chunk_last_call = chunk.prev_last_call.clone().unwrap_or_default();
+                let cur_chunk_last_call = chunk_txs
                     .last()
                     .map(|tx| tx.calls()[0].clone())
-                    .unwrap_or_else(Call::default);
-                // If it's the very first chunk in a block set last call & begin_chunk to default
-                let prev_last_call = chunk.prev_last_call.clone().unwrap_or_default();
+                    .unwrap_or_else(|| prev_chunk_last_call.clone());
 
                 let padding = chunk.padding.as_ref().expect("padding can't be None");
 
                 // conditionally adding first step as begin chunk
                 let maybe_begin_chunk = {
                     if let Some(begin_chunk) = &chunk.begin_chunk {
-                        vec![(&dummy_tx, &prev_last_call, begin_chunk)]
+                        vec![(&dummy_tx, &prev_chunk_last_call, begin_chunk)]
                     } else {
                         vec![]
                     }
@@ -1153,10 +1160,16 @@ impl<F: Field> ExecutionConfig<F> {
                     .chain(chunk_txs.iter().flat_map(|tx| {
                         tx.steps()
                             .iter()
+                            // chunk_txs is just a super set of execstep. To filter targetting
+                            // execstep we need to further filter by [initial_rwc, end_rwc)
+                            .filter(|step| {
+                                step.rwc.0 >= chunk.chunk_context.initial_rwc
+                                    && step.rwc.0 < chunk.chunk_context.end_rwc
+                            })
                             .map(move |step| (tx, &tx.calls()[step.call_index], step))
                     }))
                     // this dummy step is just for real step assignment proceed to `second last`
-                    .chain(std::iter::once((&dummy_tx, &last_call, padding)))
+                    .chain(std::iter::once((&dummy_tx, &cur_chunk_last_call, padding)))
                     .peekable();
 
                 let evm_rows = chunk.fixed_param.max_evm_rows;
@@ -1228,6 +1241,7 @@ impl<F: Field> ExecutionConfig<F> {
                     if next.is_none() {
                         break;
                     }
+
                     second_last_real_step = Some(cur);
                     // record offset of current step before assignment
                     second_last_real_step_offset = offset;
@@ -1243,7 +1257,7 @@ impl<F: Field> ExecutionConfig<F> {
                         next_step_after_real_step = Some(padding.clone());
                     }
                     offset = assign_padding_or_step(
-                        (&dummy_tx, &last_call, padding),
+                        (&dummy_tx, &cur_chunk_last_call, padding),
                         offset,
                         None,
                         Some(evm_rows - 1),
@@ -1254,7 +1268,7 @@ impl<F: Field> ExecutionConfig<F> {
                 if let Some(end_chunk) = &chunk.end_chunk {
                     debug_assert_eq!(ExecutionState::EndChunk.get_step_height(), 1);
                     offset = assign_padding_or_step(
-                        (&dummy_tx, &last_call, end_chunk),
+                        (&dummy_tx, &cur_chunk_last_call, end_chunk),
                         offset,
                         None,
                         None,
@@ -1269,7 +1283,7 @@ impl<F: Field> ExecutionConfig<F> {
                     );
                     debug_assert_eq!(ExecutionState::EndBlock.get_step_height(), 1);
                     offset = assign_padding_or_step(
-                        (&dummy_tx, &last_call, &block.end_block),
+                        (&dummy_tx, &cur_chunk_last_call, &block.end_block),
                         offset,
                         None,
                         None,
@@ -1286,7 +1300,11 @@ impl<F: Field> ExecutionConfig<F> {
                     _ = assign_padding_or_step(
                         last_real_step,
                         second_last_real_step_offset,
-                        Some((&dummy_tx, &last_call, &next_step_after_real_step.unwrap())),
+                        Some((
+                            &dummy_tx,
+                            &cur_chunk_last_call,
+                            &next_step_after_real_step.unwrap(),
+                        )),
                         None,
                     )?;
                 }
@@ -1421,6 +1439,13 @@ impl<F: Field> ExecutionConfig<F> {
                 step,
                 call
             );
+            // println!(
+            //     "assign_exec_step offset: {} state {:?} step: {:?} call: {:?}",
+            //     offset,
+            //     step.execution_state(),
+            //     step,
+            //     call
+            // );
         }
         // Make the region large enough for the current step and the next step.
         // The next step's next step may also be accessed, so make the region large
@@ -1438,6 +1463,13 @@ impl<F: Field> ExecutionConfig<F> {
         // so their witness values need to be known to be able
         // to correctly calculate the intermediate value.
         if let Some(next_step) = next_step {
+            // println!(
+            //     "assign_exec_step nextstep offset: {} state {:?} step: {:?} call: {:?}",
+            //     offset,
+            //     next_step.2.execution_state(),
+            //     next_step.2,
+            //     next_step.1
+            // );
             self.assign_exec_step_int(
                 region,
                 offset + height,