Remove witness tx struct (#1457)

### Description We remove the witness tx struct. We can wait for the lo-hi refactor ready then for this PR. ### Issue Link Part of #1391 ### Type of change New feature (non-breaking change which adds functionality) ### Contents - Remove zkevm-circuits/src/witness/tx.rs entirely - Remove eth_block field from block - Acquire txID ASAP so we don't need a Optional<tx_id>
privacy-scaling-explorations · Jul 5, 2023 · dd1e1c4 · dd1e1c4
1 parent 8030de3
commit dd1e1c4
Show file tree

Hide file tree

Showing 33 changed files with 200 additions and 292 deletions.
diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs
@@ -34,7 +34,7 @@ pub use execution::{
 pub use input_state_ref::CircuitInputStateRef;
 use itertools::Itertools;
 use log::warn;
-use std::collections::HashMap;
+use std::{collections::HashMap, ops::Deref};
 pub use transaction::{Transaction, TransactionContext};
 
 /// Circuit Setup Parameters
@@ -164,6 +164,7 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
     /// Create a new Transaction from a [`eth_types::Transaction`].
     pub fn new_tx(
         &mut self,
+        id: u64,
         eth_tx: &eth_types::Transaction,
         is_success: bool,
     ) -> Result<Transaction, Error> {
@@ -180,7 +181,14 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
             ),
         );
 
-        Transaction::new(call_id, &self.sdb, &mut self.code_db, eth_tx, is_success)
+        Transaction::new(
+            id,
+            call_id,
+            &self.sdb,
+            &mut self.code_db,
+            eth_tx,
+            is_success,
+        )
     }
 
     /// Iterate over all generated CallContext RwCounterEndOfReversion
@@ -214,8 +222,9 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
         eth_tx: &eth_types::Transaction,
         geth_trace: &GethExecTrace,
         is_last_tx: bool,
+        tx_index: u64,
     ) -> Result<(), Error> {
-        let mut tx = self.new_tx(eth_tx, !geth_trace.failed)?;
+        let mut tx = self.new_tx(tx_index, eth_tx, !geth_trace.failed)?;
         let mut tx_ctx = TransactionContext::new(eth_tx, geth_trace, is_last_tx)?;
 
         // Generate BeginTx step
@@ -318,9 +327,16 @@ impl<C: CircuitsParams> CircuitInputBuilder<C> {
         geth_traces: &[eth_types::GethExecTrace],
     ) -> Result<(), Error> {
         // accumulates gas across all txs in the block
-        for (tx_index, tx) in eth_block.transactions.iter().enumerate() {
-            let geth_trace = &geth_traces[tx_index];
-            self.handle_tx(tx, geth_trace, tx_index + 1 == eth_block.transactions.len())?;
+        for (idx, tx) in eth_block.transactions.iter().enumerate() {
+            let geth_trace = &geth_traces[idx];
+            // Transaction index starts from 1
+            let tx_id = idx + 1;
+            self.handle_tx(
+                tx,
+                geth_trace,
+                tx_id == eth_block.transactions.len(),
+                tx_id as u64,
+            )?;
         }
         // set eth_block
         self.block.eth_block = eth_block.clone();
@@ -404,7 +420,7 @@ impl CircuitInputBuilder<DynamicCParams> {
 pub fn keccak_inputs(block: &Block, code_db: &CodeDB) -> Result<Vec<Vec<u8>>, Error> {
     let mut keccak_inputs = Vec::new();
     // Tx Circuit
-    let txs: Vec<geth_types::Transaction> = block.txs.iter().map(|tx| tx.tx.clone()).collect();
+    let txs: Vec<geth_types::Transaction> = block.txs.iter().map(|tx| tx.deref().clone()).collect();
     keccak_inputs.extend_from_slice(&keccak_inputs_tx_circuit(&txs, block.chain_id.as_u64())?);
     // Bytecode Circuit
     for bytecode in code_db.0.values() {

diff --git a/bus-mapping/src/circuit_input_builder/tracer_tests.rs b/bus-mapping/src/circuit_input_builder/tracer_tests.rs
@@ -37,7 +37,7 @@ impl CircuitInputBuilderTx {
         let block = crate::mock::BlockData::new_from_geth_data(geth_data.clone());
         let mut builder = block.new_circuit_input_builder();
         let tx = builder
-            .new_tx(&block.eth_block.transactions[0], true)
+            .new_tx(0, &block.eth_block.transactions[0], true)
             .unwrap();
         let tx_ctx = TransactionContext::new(
             &block.eth_block.transactions[0],

diff --git a/bus-mapping/src/circuit_input_builder/transaction.rs b/bus-mapping/src/circuit_input_builder/transaction.rs
@@ -179,8 +179,10 @@ impl TransactionContext {
 #[derive(Debug, Clone, Default)]
 /// Result of the parsing of an Ethereum Transaction.
 pub struct Transaction {
+    /// The transaction id
+    pub id: u64,
     /// The raw transaction fields
-    pub tx: geth_types::Transaction,
+    tx: geth_types::Transaction,
     /// Calls made in the transaction
     pub(crate) calls: Vec<Call>,
     /// Execution steps
@@ -190,6 +192,7 @@ pub struct Transaction {
 impl Transaction {
     /// Create a new Self.
     pub fn new(
+        id: u64,
         call_id: usize,
         sdb: &StateDB,
         code_db: &mut CodeDB,
@@ -244,17 +247,13 @@ impl Transaction {
         };
 
         Ok(Self {
+            id,
             tx: eth_tx.into(),
             calls: vec![call],
             steps: Vec::new(),
         })
     }
 
-    /// Whether this [`Transaction`] is a create one
-    pub fn is_create(&self) -> bool {
-        self.calls[0].is_create()
-    }
-
     /// Return the list of execution steps of this transaction.
     pub fn steps(&self) -> &[ExecStep] {
         &self.steps
@@ -295,8 +294,19 @@ impl Transaction {
         self.steps.is_empty()
     }
 
-    /// Convinient method for gas limit
-    pub fn gas(&self) -> u64 {
-        self.tx.gas_limit.as_u64()
+    /// Constructor for padding tx in tx circuit
+    pub fn padding_tx(id: usize) -> Self {
+        Self {
+            id: id as u64,
+            ..Default::default()
+        }
+    }
+}
+
+impl std::ops::Deref for Transaction {
+    type Target = geth_types::Transaction;
+
+    fn deref(&self) -> &Self::Target {
+        &self.tx
     }
 }
diff --git a/bus-mapping/src/evm/opcodes/begin_end_tx.rs b/bus-mapping/src/evm/opcodes/begin_end_tx.rs
@@ -73,7 +73,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
 
     let init_code_gas_cost = if state.tx.is_create() {
         // Calculate gas cost of init code for EIP-3860.
-        (state.tx.tx.call_data.len() as u64 + 31) / 32 * eth_types::evm_types::INIT_CODE_WORD_GAS
+        (state.tx.call_data.len() as u64 + 31) / 32 * eth_types::evm_types::INIT_CODE_WORD_GAS
     } else {
         0
     };
@@ -82,7 +82,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
         GasCost::CREATION_TX
     } else {
         GasCost::TX
-    } + state.tx.tx.call_data_gas_cost()
+    } + state.tx.call_data_gas_cost()
         + init_code_gas_cost;
     exec_step.gas_cost = intrinsic_gas_cost;
 
@@ -114,7 +114,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
         callee_exists,
         call.is_create(),
         call.value,
-        Some(state.tx.tx.gas_price * state.tx.gas()),
+        Some(state.tx.gas_price * state.tx.gas()),
     )?;
 
     // In case of contract creation we wish to verify the correctness of the
@@ -168,7 +168,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
                 ),
                 (
                     CallContextField::CallDataLength,
-                    state.tx.tx.call_data.len().into(),
+                    state.tx.call_data.len().into(),
                 ),
                 (CallContextField::Value, call.value),
                 (CallContextField::IsStatic, (call.is_static as usize).into()),
@@ -263,7 +263,7 @@ fn gen_end_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Error>
     }
     let caller_balance_prev = caller_account.balance;
     let caller_balance =
-        caller_balance_prev + state.tx.tx.gas_price * (exec_step.gas_left + effective_refund);
+        caller_balance_prev + state.tx.gas_price * (exec_step.gas_left + effective_refund);
     state.account_write(
         &mut exec_step,
         call.caller_address,
@@ -272,7 +272,7 @@ fn gen_end_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Error>
         caller_balance_prev,
     )?;
 
-    let effective_tip = state.tx.tx.gas_price - state.block.base_fee;
+    let effective_tip = state.tx.gas_price - state.block.base_fee;
     let (found, coinbase_account) = state.sdb.get_account(&state.block.coinbase);
     if !found {
         return Err(Error::AccountNotFound(state.block.coinbase));

diff --git a/bus-mapping/src/evm/opcodes/logs.rs b/bus-mapping/src/evm/opcodes/logs.rs
@@ -291,7 +291,7 @@ mod log_tests {
             .unwrap();
 
         let is_persistent = builder.block.txs()[0].calls()[0].is_persistent;
-        let callee_address = builder.block.txs()[0].tx.to_or_contract_addr();
+        let callee_address = builder.block.txs()[0].to_or_contract_addr();
 
         let step = builder.block.txs()[0]
             .steps()

diff --git a/bus-mapping/src/evm/opcodes/selfbalance.rs b/bus-mapping/src/evm/opcodes/selfbalance.rs
@@ -91,7 +91,7 @@ mod selfbalance_tests {
             .unwrap();
 
         let call_id = builder.block.txs()[0].calls()[0].call_id;
-        let callee_address = builder.block.txs()[0].tx.to_or_contract_addr();
+        let callee_address = builder.block.txs()[0].to_or_contract_addr();
         let self_balance = builder.sdb.get_account(&callee_address).1.balance;
 
         assert_eq!(

diff --git a/circuit-benchmarks/src/pi_circuit.rs b/circuit-benchmarks/src/pi_circuit.rs
@@ -18,6 +18,7 @@ mod tests {
             Blake2bRead, Blake2bWrite, Challenge255, TranscriptReadBuffer, TranscriptWriterBuffer,
         },
     };
+    use itertools::Itertools;
     use rand::SeedableRng;
     use rand_xorshift::XorShiftRng;
     use std::env::var;
@@ -112,15 +113,16 @@ mod tests {
     }
 
     fn generate_publicdata(max_txs: usize) -> PublicData {
-        let mut public_data = PublicData::default();
-        let chain_id = 1337u64;
-        public_data.chain_id = Word::from(chain_id);
+        let transactions = std::iter::repeat(eth_types::geth_types::Transaction::from(
+            mock::CORRECT_MOCK_TXS[0].clone(),
+        ))
+        .take(max_txs)
+        .collect_vec();
 
-        let n_tx = max_txs;
-        for _ in 0..n_tx {
-            let eth_tx = eth_types::Transaction::from(mock::CORRECT_MOCK_TXS[0].clone());
-            public_data.transactions.push(eth_tx);
+        PublicData {
+            chain_id: Word::from(1337),
+            transactions,
+            ..Default::default()
         }
-        public_data
     }
 }
diff --git a/eth-types/src/geth_types.rs b/eth-types/src/geth_types.rs
@@ -288,6 +288,10 @@ impl Transaction {
             ..response::Transaction::default()
         }
     }
+    /// Convinient method for gas limit
+    pub fn gas(&self) -> u64 {
+        self.gas_limit.as_u64()
+    }
 }
 
 /// GethData is a type that contains all the information of a Ethereum block

diff --git a/zkevm-circuits/src/evm_circuit.rs b/zkevm-circuits/src/evm_circuit.rs
@@ -214,7 +214,7 @@ impl<F: Field> EvmCircuit<F> {
     pub fn get_min_num_rows_required(block: &Block<F>) -> usize {
         let mut num_rows = 0;
         for transaction in &block.txs {
-            for step in &transaction.steps {
+            for step in transaction.steps() {
                 num_rows += step.execution_state().get_step_height();
             }
         }
@@ -270,7 +270,7 @@ impl<F: Field> SubCircuit<F> for EvmCircuit<F> {
 /// create fixed_table_tags needed given witness block
 pub(crate) fn detect_fixed_table_tags<F: Field>(block: &Block<F>) -> Vec<FixedTableTag> {
     let need_bitwise_lookup = block.txs.iter().any(|tx| {
-        tx.steps.iter().any(|step| {
+        tx.steps().iter().any(|step| {
             matches!(
                 step.opcode(),
                 Some(OpcodeId::AND)

diff --git a/zkevm-circuits/src/evm_circuit/execution.rs b/zkevm-circuits/src/evm_circuit/execution.rs
@@ -922,7 +922,7 @@ impl<F: Field> ExecutionConfig<F> {
                 let last_call = block
                     .txs
                     .last()
-                    .map(|tx| tx.calls[0].clone())
+                    .map(|tx| tx.calls()[0].clone())
                     .unwrap_or_else(Call::default);
                 let end_block_not_last = &block.end_block_not_last;
                 let end_block_last = &block.end_block_last;
@@ -931,9 +931,9 @@ impl<F: Field> ExecutionConfig<F> {
                     .txs
                     .iter()
                     .flat_map(|tx| {
-                        tx.steps
+                        tx.steps()
                             .iter()
-                            .map(move |step| (tx, &tx.calls[step.call_index], step))
+                            .map(move |step| (tx, &tx.calls()[step.call_index], step))
                     })
                     .chain(std::iter::once((&dummy_tx, &last_call, end_block_not_last)))
                     .peekable();

diff --git a/zkevm-circuits/src/evm_circuit/execution/balance.rs b/zkevm-circuits/src/evm_circuit/execution/balance.rs
@@ -122,7 +122,7 @@ impl<F: Field> ExecutionGadget<F> for BalanceGadget<F> {
             .assign_h160(region, offset, address.to_address())?;
 
         self.tx_id
-            .assign(region, offset, Value::known(F::from(tx.id as u64)))?;
+            .assign(region, offset, Value::known(F::from(tx.id)))?;
 
         self.reversion_info.assign(
             region,