fix soundness: share txid write lookup across step (#1719)

### Description

This PR fix soundness by sharing txid write lookup between current step
and next step

### Issue Link

#1707 

### Type of change

- [x] Bug fix (non-breaking change which fixes an issue)

### Design rationale
Before fix, `cur_end_tx -> callcontext(txid, WRITE)` vs `next_begin_tx
-> callcontext(txid, WRITE)` are meant to constraint `next_tx_id =
cur_tx_id + 1`. However, because 2 callcontext write are different
records in rw_table, so actually second record are under-constraint and
tx_id can be manipulated.

Because so far we do not have negative test framework. So this PR just
simply adding a simple positive test to assure there is no consecutive
tx_id write with SAME tx_id.

If this fix make sense, will update zkevm-specs accordingly
https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/src/zkevm_specs/evm_circuit/execution/end_tx.py#L71-L79

Author: hero78119

Diff for: bus-mapping/src/evm/opcodes/begin_end_tx.rs

@@ -343,15 +343,6 @@ pub(crate) fn end_tx(
     // Write the tx receipt
     write_tx_receipt(state, exec_step, call.is_persistent)?;
 
-    // Write the next transaction id if we're not at the last tx
-    if !state.tx_ctx.is_last_tx() {
-        state.call_context_write(
-            exec_step,
-            state.block_ctx.rwc.0 + 1,
-            CallContextField::TxId,
-            (state.tx_ctx.id() + 1).into(),
-        )?;
-    }
     Ok(())
 }
 

Diff for: zkevm-circuits/src/evm_circuit/execution/end_tx.rs

@@ -239,9 +239,11 @@ impl<F: Field> ExecutionGadget<F> for EndTxGadget<F> {
 
 #[cfg(test)]
 mod test {
-    use crate::test_util::CircuitTestBuilder;
-    use bus_mapping::circuit_input_builder::FixedCParams;
+
+    use crate::{table::CallContextFieldTag, test_util::CircuitTestBuilder};
+    use bus_mapping::{circuit_input_builder::FixedCParams, operation::Target};
     use eth_types::{self, bytecode, Word};
+    use itertools::Itertools;
     use mock::{
         eth, gwei, test_ctx::helpers::account_0_code_account_1_no_code, TestContext, MOCK_ACCOUNTS,
     };
@@ -420,6 +422,57 @@ mod test {
         );
     }
 
+    #[test]
+    fn end_tx_consistent_tx_id_write() {
+        // check there is no consecutive txid write with same txid in rw_table
+
+        let block = CircuitTestBuilder::new_from_test_ctx(
+            TestContext::<2, 3>::new(
+                None,
+                account_0_code_account_1_no_code(bytecode! { STOP }),
+                |mut txs, accs| {
+                    txs[0]
+                        .to(accs[0].address)
+                        .from(accs[1].address)
+                        .value(eth(1));
+                    txs[1]
+                        .to(accs[0].address)
+                        .from(accs[1].address)
+                        .value(eth(1));
+                    txs[2]
+                        .to(accs[0].address)
+                        .from(accs[1].address)
+                        .value(eth(1));
+                },
+                |block, _tx| block.number(0xcafeu64),
+            )
+            .unwrap(),
+        )
+        .params(FixedCParams {
+            max_txs: 5,
+            ..Default::default()
+        })
+        .build_block()
+        .unwrap();
+
+        block.rws.0[&Target::CallContext]
+            .iter()
+            .filter(|rw| {
+                // filter all txid write operation
+                rw.is_write()
+                    && rw
+                        .field_tag()
+                        .is_some_and(|tag| tag == CallContextFieldTag::TxId as u64)
+            })
+            .sorted_by_key(|a| a.rw_counter())
+            .tuple_windows()
+            .for_each(|(a, b)| {
+                // chech there is no consecutive write with same txid value
+                assert!(a.rw_counter() != b.rw_counter());
+                assert!(a.value_assignment() != b.value_assignment());
+            })
+    }
+
     #[test]
     fn end_tx_gadget_nonexisting_coinbase() {
         // Check that the code hash of the coinbase address is correctly set to be the empty code

Diff for: zkevm-circuits/src/evm_circuit/util/constraint_builder.rs

@@ -1081,7 +1081,6 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> {
     }
 
     // Call context
-
     pub(crate) fn call_context(
         &mut self,
         call_id: Option<Expression<F>>,
@@ -1132,6 +1131,32 @@ impl<'a, F: Field> EVMConstraintBuilder<'a, F> {
         );
     }
 
+    // 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: Word<Expression<F>>,
+    ) {
+        self.rw_lookup_with_counter(
+            "CallContext lookup",
+            rw_counter,
+            1.expr(),
+            Target::CallContext,
+            RwValues::new(
+                call_id.unwrap_or_else(|| self.curr.state.call_id.expr()),
+                0.expr(),
+                field_tag.expr(),
+                Word::zero(),
+                value,
+                Word::zero(),
+                Word::zero(),
+            ),
+        );
+    }
+
     pub(crate) fn call_context_lookup_write(
         &mut self,
         call_id: Option<Expression<F>>,

Diff for: zkevm-circuits/src/evm_circuit/util/tx.rs

@@ -119,18 +119,24 @@ impl<F: Field> EndTxHelperGadget<F> {
         );
 
         // Transition
-        let rw_counter = num_rw.expr() - is_first_tx.expr();
+        let rw_counter_offset = num_rw.expr() - is_first_tx.expr();
         cb.condition(
             cb.next
                 .execution_state_selector([ExecutionState::BeginTx, ExecutionState::InvalidTx]),
             |cb| {
-                cb.call_context_lookup_write(
-                    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
                     Word::from_lo_unchecked(tx_id.expr() + 1.expr()),
                 );
+                // minus 1.expr() because `call_context_lookup_write_with_counter` do not bump
+                // rwc
                 cb.require_step_state_transition(StepStateTransition {
-                    rw_counter: Delta(rw_counter.clone()),
+                    rw_counter: Delta(rw_counter_offset.clone() - 1.expr()),
                     ..StepStateTransition::any()
                 });
             },
@@ -139,7 +145,7 @@ impl<F: Field> EndTxHelperGadget<F> {
             cb.next.execution_state_selector([ExecutionState::EndBlock]),
             |cb| {
                 cb.require_step_state_transition(StepStateTransition {
-                    rw_counter: Delta(rw_counter.expr() - 1.expr()),
+                    rw_counter: Delta(rw_counter_offset.expr() - 1.expr()),
                     // We propagate call_id so that EndBlock can get the last tx_id
                     // in order to count processed txs.
                     call_id: Same,

Diff for: zkevm-circuits/src/test_util.rs

@@ -145,7 +145,8 @@ impl<const NACC: usize, const NTX: usize> CircuitTestBuilder<NACC, NTX> {
 }
 
 impl<const NACC: usize, const NTX: usize> CircuitTestBuilder<NACC, NTX> {
-    fn build_block(&self) -> Result<Block<Fr>, CircuitTestError> {
+    /// build block
+    pub fn build_block(&self) -> Result<Block<Fr>, CircuitTestError> {
         if let Some(block) = &self.block {
             // If a block is specified, no need to modify the block
             return Ok(block.clone());