Skip invalid txs due to nonce, intrinsic gas and insufficient funds (#…

…115)

* Add invalid signature support

* Update Ref. spec 0. in tx_circuit.rs

* Update

* Support invalid tx in begin_tx and end_tx

* Update

* Revert changes in begin_tx.rs

* Add nonce back

* Update

* Update

* Update

* Add comments

* Rename mul_gas_fee_by_gas to gas_fee

* Debug

* Revert "Rename mul_gas_fee_by_gas to gas_fee"

This reverts commit 7179baa.

* Update

* Update

* Clean code

* Upgrade snark-verifier

* Fix failed tests

* Update

* Clean test

* Update

* Update

* Remove default method in eth-types/src/lib.rs

* Remove type

* Revert formatting in GoLang in trace.go

* Update

* Addressed comments

* Revert changes in evm_circuit/param.rs

* Update

* Update

* Update

* Update

* Update

* Update

* Support invalid tx

* Update

* Work around zero gas payment to non-existing coinbase address

* Always read the caller balance in begin_tx

---------

Co-authored-by: Brechtpd <Brechtp.devos@gmail.com>

bus-mapping/src/circuit_input_builder.rs

@@ -153,6 +153,7 @@ impl<'a> CircuitInputBuilder {
         &mut self,
         eth_tx: &eth_types::Transaction,
         is_success: bool,
+        is_invalid: bool,
     ) -> Result<Transaction, Error> {
         let call_id = self.block_ctx.rwc.0;
 
@@ -167,7 +168,14 @@ impl<'a> CircuitInputBuilder {
             ),
         );
 
-        Transaction::new(call_id, &self.sdb, &mut self.code_db, eth_tx, is_success)
+        Transaction::new(
+            call_id,
+            &self.sdb,
+            &mut self.code_db,
+            eth_tx,
+            is_success,
+            is_invalid,
+        )
     }
 
     /// Iterate over all generated CallContext RwCounterEndOfReversion
@@ -284,9 +292,8 @@ impl<'a> CircuitInputBuilder {
         is_anchor_tx: bool,
         is_last_tx: bool,
     ) -> Result<(), Error> {
-        let mut tx = self.new_tx(eth_tx, !geth_trace.failed)?;
+        let mut tx = self.new_tx(eth_tx, !geth_trace.failed, geth_trace.invalid)?;
         let mut tx_ctx = TransactionContext::new(eth_tx, geth_trace, is_anchor_tx, is_last_tx)?;
-
         // Generate BeginTx step
         let begin_tx_step = gen_associated_steps(
             &mut self.state_ref(&mut tx, &mut tx_ctx),

bus-mapping/src/circuit_input_builder/input_state_ref.rs

@@ -472,6 +472,7 @@ impl<'a> CircuitInputStateRef<'a> {
         receiver: Address,
         receiver_exists: bool,
         must_create: bool,
+        must_read_caller_balance: bool,
         value: Word,
         fee: Option<Word>,
         is_anchor_tx: bool,
@@ -534,21 +535,25 @@ impl<'a> CircuitInputStateRef<'a> {
                 },
             )?;
         }
+
+        // Read the caller balance when required, skip if value == 0 otherwise
+        if must_read_caller_balance || !value.is_zero() {
+            self.push_op_reversible(
+                step,
+                AccountOp {
+                    address: sender,
+                    field: AccountField::Balance,
+                    value: sender_balance,
+                    value_prev: sender_balance_prev,
+                },
+            )?;
+        }
+
         if value.is_zero() {
             // Skip transfer if value == 0
             return Ok(());
         }
 
-        self.push_op_reversible(
-            step,
-            AccountOp {
-                address: sender,
-                field: AccountField::Balance,
-                value: sender_balance,
-                value_prev: sender_balance_prev,
-            },
-        )?;
-
         let (_found, receiver_account) = self.sdb.get_account(&receiver);
         let receiver_balance_prev = receiver_account.balance;
         let receiver_balance = receiver_account.balance + value;
@@ -581,6 +586,7 @@ impl<'a> CircuitInputStateRef<'a> {
             receiver,
             receiver_exists,
             must_create,
+            false,
             value,
             None,
             false,

bus-mapping/src/circuit_input_builder/tracer_tests.rs

@@ -37,13 +37,14 @@ 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(&block.eth_block.transactions[0], true, false)
             .unwrap();
         let tx_ctx = TransactionContext::new(
             &block.eth_block.transactions[0],
             &GethExecTrace {
                 gas: Gas(0),
                 failed: false,
+                invalid: false,
                 return_value: "".to_owned(),
                 struct_logs: vec![geth_step.clone()],
             },

bus-mapping/src/circuit_input_builder/transaction.rs

@@ -189,6 +189,10 @@ impl TransactionContext {
 pub struct Transaction {
     /// The raw transaction fields
     pub tx: geth_types::Transaction,
+    /// Invalid tx
+    pub invalid_tx: bool,
+    /// AccessListGasCost
+    pub access_list_gas_cost: u64,
     /// Calls made in the transaction
     pub(crate) calls: Vec<Call>,
     /// Execution steps
@@ -203,6 +207,7 @@ impl Transaction {
         code_db: &mut CodeDB,
         eth_tx: &eth_types::Transaction,
         is_success: bool,
+        is_invalid: bool,
     ) -> Result<Self, Error> {
         let (found, _) = sdb.get_account(&eth_tx.from);
         if !found {
@@ -253,6 +258,8 @@ impl Transaction {
 
         Ok(Self {
             tx: eth_tx.into(),
+            invalid_tx: is_invalid,
+            access_list_gas_cost: 0,
             calls: vec![call],
             steps: Vec::new(),
         })

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

@@ -48,14 +48,19 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
         state.call_context_write(&mut exec_step, call.call_id, field, value);
     }
 
-    // Increase caller's nonce
+    // Increase caller's nonce when the tx is not invalid
     let caller_address = call.caller_address;
     let nonce_prev = state.sdb.get_account(&caller_address).1.nonce;
+    let nonce = if !state.tx.invalid_tx {
+        nonce_prev + 1
+    } else {
+        nonce_prev
+    };
     state.account_write(
         &mut exec_step,
         caller_address,
         AccountField::Nonce,
-        (nonce_prev + 1).into(),
+        nonce.into(),
         nonce_prev.into(),
     )?;
 
@@ -76,7 +81,20 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
     } else {
         GasCost::TX.as_u64()
     } + state.tx.tx.call_data_gas_cost();
-    exec_step.gas_cost = GasCost(intrinsic_gas_cost);
+
+    // Don't pay any fee or transfer any ETH for invalid transactions
+    let (gas_cost, value, fee) = if state.tx.invalid_tx {
+        (0, Word::zero(), Some(Word::zero()))
+    } else {
+        (
+            intrinsic_gas_cost,
+            call.value,
+            Some(state.tx.tx.gas_price * state.tx.gas()),
+        )
+    };
+
+    // Set the gas cost
+    exec_step.gas_cost = GasCost(gas_cost);
 
     // Get code_hash of callee
     let (_, callee_account) = state.sdb.get_account(&call.address);
@@ -104,9 +122,10 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
         call.caller_address,
         call.address,
         callee_exists,
-        call.is_create(),
-        call.value,
-        Some(state.tx.tx.gas_price * state.tx.gas()),
+        !state.tx.invalid_tx && call.is_create(),
+        true,
+        value,
+        fee,
         state.tx_ctx.is_anchor_tx(),
     )?;
 
@@ -132,7 +151,7 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
     match (
         call.is_create(),
         state.is_precompiled(&call.address),
-        is_empty_code_hash,
+        is_empty_code_hash || state.tx.invalid_tx,
     ) {
         // 1. Creation transaction.
         (true, _, _) => {
@@ -181,13 +200,13 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro
             evm_unimplemented!("Call to precompiled is left unimplemented");
             Ok(exec_step)
         }
-        (_, _, is_empty_code_hash) => {
+        (_, _, do_not_run_code) => {
             // 3. Call to account with empty code.
-            if is_empty_code_hash {
+            if do_not_run_code {
                 return Ok(exec_step);
             }
 
-            // 4. Call to account with non-empty code.
+            // 4. Call to account with non-empty code/invalid tx.
             for (field, value) in [
                 (CallContextField::Depth, call.depth.into()),
                 (

bus-mapping/src/lib.rs

@@ -141,6 +141,7 @@
 //!     return_value: "".to_string(),
 //!     gas: Gas(block.eth_block.transactions[0].gas.as_u64()),
 //!     failed: false,
+//!     invalid: false,
 //!     struct_logs: geth_steps,
 //! };
 //!

eth-types/src/evm_types.rs

@@ -144,6 +144,10 @@ impl GasCost {
     pub const CALL_WITH_VALUE: Self = Self(9000);
     /// Constant cost for turning empty account into non-empty account
     pub const NEW_ACCOUNT: Self = Self(25000);
+    /// Gas cost of warming up an account with the access list
+    pub const ACCESS_LIST_ADDRESS: Self = Self(2400);
+    /// Gas cost of warming up a storage with the access list
+    pub const ACCESS_LIST_STORAGE: Self = Self(1900);
     /// Cost per byte of deploying a new contract
     pub const CODE_DEPOSIT_BYTE_COST: Self = Self(200);
     /// Denominator of quadratic part of memory expansion gas cost

eth-types/src/lib.rs

@@ -446,6 +446,9 @@ pub struct GethExecTrace {
     pub gas: Gas,
     /// True when the transaction has failed.
     pub failed: bool,
+    /// True when the tx could not execute
+    #[serde(default)]
+    pub invalid: bool,
     /// Return value of execution which is a hex encoded byte array
     #[serde(rename = "returnValue")]
     pub return_value: String,
@@ -559,6 +562,7 @@ mod tests {
         assert_eq!(
             trace,
             GethExecTrace {
+                invalid: false,
                 gas: Gas(26809),
                 failed: false,
                 return_value: "".to_owned(),

external-tracer/src/lib.rs

@@ -71,6 +71,20 @@ pub fn trace(config: &TraceConfig) -> Result<Vec<GethExecTrace>, Error> {
         },
     )?;
 
-    let trace = serde_json::from_str(&trace_string).map_err(Error::SerdeError)?;
+    let trace: Vec<GethExecTrace> =
+        serde_json::from_str(&trace_string).map_err(Error::SerdeError)?;
+    // Don't throw only for specific invalid transactions we support.
+    for trace in trace.iter() {
+        if trace.invalid
+            && !(trace.return_value.starts_with("nonce too low")
+                || trace.return_value.starts_with("nonce too high")
+                || trace.return_value.starts_with("intrinsic gas too low")
+                || trace
+                    .return_value
+                    .starts_with("insufficient funds for gas * price + value"))
+        {
+            return Err(Error::TracingError(trace.return_value.clone()));
+        }
+    }
     Ok(trace)
 }

geth-utils/Cargo.toml

@@ -7,4 +7,9 @@ license = "MIT OR Apache-2.0"
 [build-dependencies]
 gobuild = "0.1.0-alpha.1"
 log = "0.4.14"
-env_logger = "0.9"
+env_logger = "0.9"
+
+[dev-dependencies]
+eth-types = { path = "../eth-types" }
+serde = {version = "1.0.130", features = ["derive"] }
+serde_json = "1.0.66"

geth-utils/gethutil/trace.go

@@ -20,6 +20,7 @@ import (
 // while replaying a transaction in debug mode as well as transaction
 // execution status, the amount of gas used and the return value
 type ExecutionResult struct {
+	Invalid     bool           `json:"invalid"`
 	Gas         uint64         `json:"gas"`
 	Failed      bool           `json:"failed"`
 	ReturnValue string         `json:"returnValue"`
@@ -221,15 +222,23 @@ func Trace(config TraceConfig) ([]*ExecutionResult, error) {
 
 		result, err := core.ApplyMessage(evm, &message, new(core.GasPool).AddGas(message.GasLimit))
 		if err != nil {
-			return nil, fmt.Errorf("Failed to apply config.Transactions[%d]: %w", i, err)
-		}
-		stateDB.Finalise(true)
-
-		executionResults[i] = &ExecutionResult{
-			Gas:         result.UsedGas,
-			Failed:      result.Failed(),
-			ReturnValue: fmt.Sprintf("%x", result.ReturnData),
-			StructLogs:  FormatLogs(tracer.StructLogs()),
+			executionResults[i] = &ExecutionResult{
+				Invalid:     true,
+				Gas:         0,
+				Failed:      false,
+				ReturnValue: fmt.Sprintf("%v", err),
+				StructLogs:  []StructLogRes{},
+			}
+		} else {
+			stateDB.Finalise(true)
+
+			executionResults[i] = &ExecutionResult{
+				Invalid:     false,
+				Gas:         result.UsedGas,
+				Failed:      result.Failed(),
+				ReturnValue: fmt.Sprintf("%x", result.ReturnData),
+				StructLogs:  FormatLogs(tracer.StructLogs()),
+			}
 		}
 	}
 

geth-utils/src/lib.rs

@@ -53,6 +53,7 @@ impl Display for Error {
 #[cfg(test)]
 mod test {
     use crate::trace;
+    use eth_types::{Error, GethExecTrace};
 
     #[test]
     fn valid_tx() {
@@ -102,7 +103,15 @@ mod test {
                 ]
             }"#,
         ] {
-            assert!(trace(config).is_ok());
+            let trace_result = trace(config);
+            assert!(trace_result.is_ok());
+            let trace_string = trace_result.unwrap();
+            let trace: Vec<GethExecTrace> = serde_json::from_str(&trace_string)
+                .map_err(Error::SerdeError)
+                .unwrap();
+            for trace in trace.iter() {
+                assert!(!trace.invalid);
+            }
         }
     }
 
@@ -150,7 +159,15 @@ mod test {
                 ]
             }"#,
         ] {
-            assert!(trace(config).is_err())
+            let trace_result = trace(config);
+            assert!(trace_result.is_ok());
+            let trace_string = trace_result.unwrap();
+            let trace: Vec<GethExecTrace> = serde_json::from_str(&trace_string)
+                .map_err(Error::SerdeError)
+                .unwrap();
+            for trace in trace.iter() {
+                assert!(trace.invalid);
+            }
         }
     }
 }