pick privacy-scaling-explorations#1719 from upstream: fix begin-tx tx…

…_id soundness (scroll-tech#1078)

* pick privacy-scaling-explorations#1719 from upstream: fix begin-tx tx_id soundness

* fix transition from end_tx to end_inner_block

---------

Co-authored-by: Rohit Narurkar <[email protected]>

bus-mapping/src/evm/opcodes/begin_end_tx.rs

@@ -4,7 +4,7 @@ use super::{
 };
 use crate::{
     circuit_input_builder::{
-        CircuitInputStateRef, CopyAccessList, CopyBytes, CopyDataType, CopyEvent, ExecStep,
+        Call, CircuitInputStateRef, CopyAccessList, CopyBytes, CopyDataType, CopyEvent, ExecStep,
         NumberOrHash,
     },
     l2_predeployed::l1_gas_price_oracle,
@@ -44,12 +44,15 @@ use ethers_core::utils::get_contract_address;
 
 pub fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<Vec<ExecStep>, Error> {
     let mut exec_step = state.new_begin_tx_step();
+    let call = state.call()?.clone();
+
+    // write tx_id
+    begin_tx(state, &mut exec_step, &call)?;
 
     // Add two copy-events for tx access-list addresses and storage keys for
     // EIP-1559 and EIP-2930.
     gen_tx_access_list_ops(state, &mut exec_step)?;
 
-    let call = state.call()?.clone();
     let caller_address = call.caller_address;
     if state.tx.tx_type.is_l1_msg() {
         // for l1 message, no need to add rw op, but we must check
@@ -114,7 +117,6 @@ pub fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<Vec<ExecSt
     // * write l1fee call context
 
     for (field, value) in [
-        (CallContextField::TxId, state.tx_ctx.id().into()),
         (
             CallContextField::RwCounterEndOfReversion,
             call.rw_counter_end_of_reversion.into(),
@@ -646,17 +648,53 @@ pub fn gen_end_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
         )?;
     }
 
+    end_tx(state, &mut exec_step, &call)?;
+
+    Ok(exec_step)
+}
+
+pub(crate) fn begin_tx(
+    state: &mut CircuitInputStateRef,
+    exec_step: &mut ExecStep,
+    call: &Call,
+) -> Result<(), Error> {
+    // Write the transaction id
+    state.call_context_write(
+        exec_step,
+        call.call_id,
+        CallContextField::TxId,
+        state.tx_ctx.id().into(),
+    )?;
+    Ok(())
+}
+
+pub(crate) fn end_tx(
+    state: &mut CircuitInputStateRef,
+    exec_step: &mut ExecStep,
+    call: &Call,
+) -> Result<(), Error> {
+    // Write the tx receipt
+    write_tx_receipt(state, exec_step, call.is_persistent)?;
+
+    Ok(())
+}
+
+fn write_tx_receipt(
+    state: &mut CircuitInputStateRef,
+    exec_step: &mut ExecStep,
+    is_persistent: bool,
+) -> Result<(), Error> {
     // handle tx receipt tag
     state.tx_receipt_write(
-        &mut exec_step,
+        exec_step,
         state.tx_ctx.id(),
         TxReceiptField::PostStateOrStatus,
-        call.is_persistent as u64,
+        is_persistent as u64,
     )?;
 
     let log_id = exec_step.log_id;
     state.tx_receipt_write(
-        &mut exec_step,
+        exec_step,
         state.tx_ctx.id(),
         TxReceiptField::LogLength,
         log_id as u64,
@@ -665,7 +703,7 @@ pub fn gen_end_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
     if state.tx_ctx.id() > 1 {
         // query pre tx cumulative gas
         state.tx_receipt_read(
-            &mut exec_step,
+            exec_step,
             state.tx_ctx.id() - 1,
             TxReceiptField::CumulativeGasUsed,
             state.block_ctx.cumulative_gas_used,
@@ -674,22 +712,13 @@ pub fn gen_end_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Er
 
     state.block_ctx.cumulative_gas_used += state.tx.gas - exec_step.gas_left.0;
     state.tx_receipt_write(
-        &mut exec_step,
+        exec_step,
         state.tx_ctx.id(),
         TxReceiptField::CumulativeGasUsed,
         state.block_ctx.cumulative_gas_used,
     )?;
 
-    if !state.tx_ctx.is_last_tx() {
-        state.call_context_write(
-            &mut exec_step,
-            state.block_ctx.rwc.0 + 1,
-            CallContextField::TxId,
-            (state.tx_ctx.id() + 1).into(),
-        )?;
-    }
-
-    Ok(exec_step)
+    Ok(())
 }
 
 // Add 3 RW read operations for transaction L1 fee.

zkevm-circuits/src/evm_circuit/execution.rs

@@ -870,6 +870,7 @@ impl<F: Field> ExecutionConfig<F> {
                             vec![ExecutionState::EndInnerBlock, ExecutionState::EndBlock],
                         ),
                         (
+                            // Empty block can result multiple EndInnerBlock states.
                             "Only EndTx or EndInnerBlock can transit to EndInnerBlock",
                             ExecutionState::EndInnerBlock,
                             vec![ExecutionState::EndTx, ExecutionState::EndInnerBlock],

zkevm-circuits/src/evm_circuit/execution/begin_tx.rs

@@ -116,6 +116,13 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
         let call_id = cb.curr.state.rw_counter.clone();
 
         let tx_id = cb.query_cell();
+        cb.call_context_lookup(
+            1.expr(),
+            Some(call_id.expr()),
+            CallContextFieldTag::TxId,
+            tx_id.expr(),
+        ); // rwc_delta += 1
+
         let sender_nonce = cb.query_cell();
 
         let [tx_type, tx_nonce, tx_gas, tx_caller_address, tx_callee_address, tx_is_create, tx_call_data_length, tx_call_data_gas_cost, tx_data_gas_cost] =
@@ -163,12 +170,6 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
             l1_fee_cost.expr(),
         ); // rwc_delta += 1
 
-        cb.call_context_lookup(
-            1.expr(),
-            Some(call_id.expr()),
-            CallContextFieldTag::TxId,
-            tx_id.expr(),
-        ); // rwc_delta += 1
         let mut reversion_info = cb.reversion_info_write(None); // rwc_delta += 2
         let is_persistent = reversion_info.is_persistent();
         cb.call_context_lookup(
@@ -841,6 +842,9 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
         */
 
         let mut rws = StepRws::new(block, step);
+        let rw = rws.next();
+        debug_assert_eq!(rw.tag(), RwTableTag::CallContext);
+        debug_assert_eq!(rw.field_tag(), Some(CallContextFieldTag::TxId as u64));
 
         let tx_type = tx.tx_type;
         let caller_code_hash = if tx_type.is_l1_msg() {
@@ -865,7 +869,6 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
         //              KeccakCodeHash
         // else:
         //      3 l1 fee rw
-        // TxId
         // RwCounterEndOfReversion
         // IsPersistent
         // IsSuccess
@@ -900,9 +903,6 @@ impl<F: Field> ExecutionGadget<F> for BeginTxGadget<F> {
         debug_assert_eq!(rw.tag(), RwTableTag::CallContext);
         debug_assert_eq!(rw.field_tag(), Some(CallContextFieldTag::L1Fee as u64));
 
-        let rw = rws.next();
-        debug_assert_eq!(rw.tag(), RwTableTag::CallContext);
-        debug_assert_eq!(rw.field_tag(), Some(CallContextFieldTag::TxId as u64));
         rws.offset_add(3);
 
         let rw = rws.next();

zkevm-circuits/src/evm_circuit/execution/end_inner_block.rs

@@ -3,7 +3,9 @@ use crate::{
         execution::ExecutionGadget,
         step::ExecutionState,
         util::{
-            constraint_builder::{ConstrainBuilderCommon, EVMConstraintBuilder},
+            constraint_builder::{
+                ConstrainBuilderCommon, EVMConstraintBuilder, StepStateTransition, Transition,
+            },
             math_gadget::IsZeroGadget,
             CachedRegion, Cell,
         },
@@ -39,6 +41,7 @@ impl<F: Field> ExecutionGadget<F> for EndInnerBlockGadget<F> {
     const EXECUTION_STATE: ExecutionState = ExecutionState::EndInnerBlock;
 
     fn configure(cb: &mut EVMConstraintBuilder<F>) -> Self {
+        // `cb.curr.state.block_number` is constrained inside execution.rs
         // The number of txs in the inner block is also the ID of the last tx in the
         // block.
         let last_tx_id = cb.query_cell();
@@ -91,6 +94,14 @@ impl<F: Field> ExecutionGadget<F> for EndInnerBlockGadget<F> {
             );
         });
 
+        cb.require_step_state_transition(StepStateTransition {
+            rw_counter: Transition::Same,
+            // We propagate call_id so that EndBlock can get the last tx_id
+            // in order to count processed txs.
+            call_id: Transition::Same,
+            ..StepStateTransition::any()
+        });
+
         Self {
             last_tx_id,
             num_txs,

zkevm-circuits/src/evm_circuit/execution/end_tx.rs

@@ -69,7 +69,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
         let tx_id = cb.call_context(None, CallContextFieldTag::TxId);
         let is_persistent = cb.call_context(None, CallContextFieldTag::IsPersistent);
         let tx_l1_fee = cb.call_context(None, CallContextFieldTag::L1Fee);
-
+        // rwc_delta = 3
         let [tx_gas, tx_caller_address, tx_type, tx_data_gas_cost] = [
             TxContextFieldTag::Gas,
             TxContextFieldTag::CallerAddress,
@@ -91,6 +91,8 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
         );
         let refund = cb.query_cell();
         cb.tx_refund_read(tx_id.expr(), refund.expr());
+        // rwc_delta = 4
+
         let effective_refund = MinMaxGadget::construct(cb, max_refund.quotient(), refund.expr());
 
         // Add effective_refund * tx_gas_price back to caller's balance
@@ -107,6 +109,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
                 None,
             )
         });
+        // rwc_delta = 4 + !tx_is_l1msg
 
         // Add gas_used * effective_tip to coinbase's balance
         let coinbase = cb.query_cell();
@@ -155,6 +158,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
             AccountFieldTag::CodeHash,
             coinbase_codehash.expr(),
         );
+        // rwc_delta = 5 + !tx_is_l1msg
         #[cfg(feature = "scroll")]
         let coinbase_keccak_codehash = cb.query_cell_phase2();
 
@@ -177,6 +181,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
                 None,
             )
         });
+        // rwc_delta = 5 + !tx_is_l1msg + !tx_is_l1msg * coinbase_transfer.rw_delta
 
         // constrain tx receipt fields
         cb.tx_receipt_lookup(
@@ -191,6 +196,7 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
             TxReceiptFieldTag::LogLength,
             cb.curr.state.log_id.expr(),
         );
+        // rwc_delta = 7 + !tx_is_l1msg * (coinbase_transfer.rw_delta + 1)
 
         let is_first_tx = IsEqualGadget::construct(cb, tx_id.expr(), 1.expr());
 
@@ -202,50 +208,54 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
             );
         });
 
-        cb.condition(1.expr() - is_first_tx.expr(), |cb| {
+        cb.condition(not::expr(is_first_tx.expr()), |cb| {
             cb.tx_receipt_lookup(
                 0.expr(),
                 tx_id.expr() - 1.expr(),
                 TxReceiptFieldTag::CumulativeGasUsed,
                 current_cumulative_gas_used.expr(),
             );
         });
+        // rwc_delta = 8 - is_first_tx + !tx_is_l1msg * (coinbase_transfer.rw_delta + 1)
 
         cb.tx_receipt_lookup(
             1.expr(),
             tx_id.expr(),
             TxReceiptFieldTag::CumulativeGasUsed,
             gas_used + current_cumulative_gas_used.expr(),
         );
+        // rwc_delta = 9 - is_first_tx + !tx_is_l1msg * (coinbase_transfer.rw_delta + 1)
+
+        // The next state of `end_tx` can only be 'begin_tx' or 'end_inner_block'
 
+        let rw_counter_offset = 9.expr() - is_first_tx.expr()
+            + not::expr(tx_is_l1msg.expr()) * (coinbase_transfer.rw_delta() + 1.expr());
         cb.condition(
             cb.next.execution_state_selector([ExecutionState::BeginTx]),
             |cb| {
-                cb.call_context_lookup(
-                    true.expr(),
-                    Some(cb.next.state.rw_counter.expr()),
+                let next_step_rwc = cb.next.state.rw_counter.expr();
+                // lookup use next step initial rwc, thus lead to same record on rw table
+                cb.call_context_lookup_write_with_counter(
+                    next_step_rwc.clone(),
+                    Some(next_step_rwc),
                     CallContextFieldTag::TxId,
+                    // tx_id has been lookup and range_check above
                     tx_id.expr() + 1.expr(),
                 );
 
                 cb.require_step_state_transition(StepStateTransition {
-                    rw_counter: Delta(
-                        10.expr() - is_first_tx.expr()
-                            + not::expr(tx_is_l1msg.expr())
-                                * (coinbase_transfer.rw_delta() + 1.expr()),
-                    ),
+                    rw_counter: Delta(rw_counter_offset.clone()),
                     ..StepStateTransition::any()
                 });
             },
         );
 
-        let rw_counter_delta = 9.expr() - is_first_tx.expr()
-            + not::expr(tx_is_l1msg.expr()) * (coinbase_transfer.rw_delta() + 1.expr());
         cb.condition(
-            cb.next.execution_state_selector([ExecutionState::EndBlock]),
+            cb.next
+                .execution_state_selector([ExecutionState::EndInnerBlock]),
             |cb| {
                 cb.require_step_state_transition(StepStateTransition {
-                    rw_counter: Delta(rw_counter_delta),
+                    rw_counter: Delta(rw_counter_offset),
                     // We propagate call_id so that EndBlock can get the last tx_id
                     // in order to count processed txs.
                     call_id: Same,

zkevm-circuits/src/evm_circuit/util/constraint_builder.rs

@@ -1134,6 +1134,33 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> {
         word
     }
 
+    // same as call_context_lookup_write with bypassing external rwc
+    // Note: will not bumping internal rwc
+    pub(crate) fn call_context_lookup_write_with_counter(
+        &mut self,
+        rw_counter: Expression<F>,
+        call_id: Option<Expression<F>>,
+        field_tag: CallContextFieldTag,
+        value: Expression<F>,
+    ) {
+        self.rw_lookup_with_counter(
+            "CallContext lookup",
+            rw_counter,
+            1.expr(),
+            RwTableTag::CallContext,
+            RwValues::new(
+                call_id.unwrap_or_else(|| self.curr.state.call_id.expr()),
+                0.expr(),
+                field_tag.expr(),
+                0.expr(),
+                value,
+                0.expr(),
+                0.expr(),
+                0.expr(),
+            ),
+        );
+    }
+
     pub(crate) fn call_context_lookup(
         &mut self,
         is_write: Expression<F>,

zkevm-circuits/src/witness/tx.rs

@@ -1288,13 +1288,18 @@ pub(super) fn tx_convert(
             .iter()
             .map(|step| step_convert(step, tx.block_num))
             .chain({
-                let rw_counter = tx.steps().last().unwrap().rwc.0 + 9 - (id == 1) as usize;
+                // TODO: it is a bit counter-intuitive to treat EndInnerBlock step, even multiple
+                // EndInnerBlock steps to belong to the last prev tx.
+                // We can change design later to make it easier to understand.
+                let last_step = tx.steps().last().unwrap();
+                let rw_counter = last_step.rwc.0 + last_step.bus_mapping_instance.len();
                 debug_assert!(next_block_num >= tx.block_num);
                 (tx.block_num..next_block_num)
                     .map(|block_num| ExecStep {
                         rw_counter,
                         execution_state: ExecutionState::EndInnerBlock,
                         block_num,
+                        call_index: last_step.call_index,
                         ..Default::default()
                     })
                     .collect::<Vec<ExecStep>>()