diff --git a/Cargo.lock b/Cargo.lock index c838cc4aac..d4746e56aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2058,11 +2058,8 @@ name = "geth-utils" version = "0.1.0" dependencies = [ "env_logger", - "eth-types", "gobuild", "log", - "serde", - "serde_json", ] [[package]] diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs index 839f10093c..0b4c0ae3a8 100644 --- a/bus-mapping/src/circuit_input_builder.rs +++ b/bus-mapping/src/circuit_input_builder.rs @@ -153,7 +153,6 @@ impl<'a> CircuitInputBuilder { &mut self, eth_tx: ð_types::Transaction, is_success: bool, - is_invalid: bool, ) -> Result { let call_id = self.block_ctx.rwc.0; @@ -168,14 +167,7 @@ impl<'a> CircuitInputBuilder { ), ); - Transaction::new( - call_id, - &self.sdb, - &mut self.code_db, - eth_tx, - is_success, - is_invalid, - ) + Transaction::new(call_id, &self.sdb, &mut self.code_db, eth_tx, is_success) } /// Iterate over all generated CallContext RwCounterEndOfReversion @@ -292,8 +284,9 @@ impl<'a> CircuitInputBuilder { is_anchor_tx: bool, is_last_tx: bool, ) -> Result<(), Error> { - let mut tx = self.new_tx(eth_tx, !geth_trace.failed, geth_trace.invalid)?; + let mut tx = self.new_tx(eth_tx, !geth_trace.failed)?; 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), diff --git a/bus-mapping/src/circuit_input_builder/input_state_ref.rs b/bus-mapping/src/circuit_input_builder/input_state_ref.rs index 8ab9ed212c..836a690ef7 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -472,7 +472,6 @@ impl<'a> CircuitInputStateRef<'a> { receiver: Address, receiver_exists: bool, must_create: bool, - must_read_caller_balance: bool, value: Word, fee: Option, is_anchor_tx: bool, @@ -535,25 +534,21 @@ 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; @@ -586,7 +581,6 @@ impl<'a> CircuitInputStateRef<'a> { receiver, receiver_exists, must_create, - false, value, None, false, diff --git a/bus-mapping/src/circuit_input_builder/tracer_tests.rs b/bus-mapping/src/circuit_input_builder/tracer_tests.rs index 1616de9e4f..4ec5dcc209 100644 --- a/bus-mapping/src/circuit_input_builder/tracer_tests.rs +++ b/bus-mapping/src/circuit_input_builder/tracer_tests.rs @@ -37,14 +37,13 @@ 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, false) + .new_tx(&block.eth_block.transactions[0], true) .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()], }, diff --git a/bus-mapping/src/circuit_input_builder/transaction.rs b/bus-mapping/src/circuit_input_builder/transaction.rs index b31792c785..199603cf81 100644 --- a/bus-mapping/src/circuit_input_builder/transaction.rs +++ b/bus-mapping/src/circuit_input_builder/transaction.rs @@ -189,10 +189,6 @@ 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, /// Execution steps @@ -207,7 +203,6 @@ impl Transaction { code_db: &mut CodeDB, eth_tx: ð_types::Transaction, is_success: bool, - is_invalid: bool, ) -> Result { let (found, _) = sdb.get_account(ð_tx.from); if !found { @@ -258,8 +253,6 @@ impl Transaction { Ok(Self { tx: eth_tx.into(), - invalid_tx: is_invalid, - access_list_gas_cost: 0, calls: vec![call], steps: Vec::new(), }) diff --git a/bus-mapping/src/evm/opcodes/begin_end_tx.rs b/bus-mapping/src/evm/opcodes/begin_end_tx.rs index f94431446a..ba899d6ca0 100644 --- a/bus-mapping/src/evm/opcodes/begin_end_tx.rs +++ b/bus-mapping/src/evm/opcodes/begin_end_tx.rs @@ -48,19 +48,14 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result Result Result Result { @@ -200,13 +181,13 @@ fn gen_begin_tx_steps(state: &mut CircuitInputStateRef) -> Result { + (_, _, is_empty_code_hash) => { // 3. Call to account with empty code. - if do_not_run_code { + if is_empty_code_hash { return Ok(exec_step); } - // 4. Call to account with non-empty code/invalid tx. + // 4. Call to account with non-empty code. for (field, value) in [ (CallContextField::Depth, call.depth.into()), ( diff --git a/bus-mapping/src/lib.rs b/bus-mapping/src/lib.rs index 214ad9041d..5a068e21f8 100644 --- a/bus-mapping/src/lib.rs +++ b/bus-mapping/src/lib.rs @@ -141,7 +141,6 @@ //! return_value: "".to_string(), //! gas: Gas(block.eth_block.transactions[0].gas.as_u64()), //! failed: false, -//! invalid: false, //! struct_logs: geth_steps, //! }; //! diff --git a/eth-types/src/evm_types.rs b/eth-types/src/evm_types.rs index 90e6cad70b..61d1441892 100644 --- a/eth-types/src/evm_types.rs +++ b/eth-types/src/evm_types.rs @@ -144,10 +144,6 @@ 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 diff --git a/eth-types/src/lib.rs b/eth-types/src/lib.rs index c9eb247bd5..e3417d6e50 100644 --- a/eth-types/src/lib.rs +++ b/eth-types/src/lib.rs @@ -446,9 +446,6 @@ 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, @@ -562,7 +559,6 @@ mod tests { assert_eq!( trace, GethExecTrace { - invalid: false, gas: Gas(26809), failed: false, return_value: "".to_owned(), diff --git a/external-tracer/src/lib.rs b/external-tracer/src/lib.rs index fa7a1b8a7d..678bc21e33 100644 --- a/external-tracer/src/lib.rs +++ b/external-tracer/src/lib.rs @@ -71,20 +71,6 @@ pub fn trace(config: &TraceConfig) -> Result, Error> { }, )?; - let trace: Vec = - 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())); - } - } + let trace = serde_json::from_str(&trace_string).map_err(Error::SerdeError)?; Ok(trace) } diff --git a/geth-utils/Cargo.toml b/geth-utils/Cargo.toml index f80f06877a..ae6750c671 100644 --- a/geth-utils/Cargo.toml +++ b/geth-utils/Cargo.toml @@ -7,9 +7,4 @@ license = "MIT OR Apache-2.0" [build-dependencies] gobuild = "0.1.0-alpha.1" log = "0.4.14" -env_logger = "0.9" - -[dev-dependencies] -eth-types = { path = "../eth-types" } -serde = {version = "1.0.130", features = ["derive"] } -serde_json = "1.0.66" +env_logger = "0.9" \ No newline at end of file diff --git a/geth-utils/gethutil/trace.go b/geth-utils/gethutil/trace.go index e8187bfc42..744ce01d40 100644 --- a/geth-utils/gethutil/trace.go +++ b/geth-utils/gethutil/trace.go @@ -20,7 +20,6 @@ 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"` @@ -222,23 +221,15 @@ func Trace(config TraceConfig) ([]*ExecutionResult, error) { result, err := core.ApplyMessage(evm, &message, new(core.GasPool).AddGas(message.GasLimit)) if err != nil { - 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()), - } + 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()), } } diff --git a/geth-utils/src/lib.rs b/geth-utils/src/lib.rs index fef6ffaf6b..60a237164f 100644 --- a/geth-utils/src/lib.rs +++ b/geth-utils/src/lib.rs @@ -53,7 +53,6 @@ impl Display for Error { #[cfg(test)] mod test { use crate::trace; - use eth_types::{Error, GethExecTrace}; #[test] fn valid_tx() { @@ -103,15 +102,7 @@ mod test { ] }"#, ] { - let trace_result = trace(config); - assert!(trace_result.is_ok()); - let trace_string = trace_result.unwrap(); - let trace: Vec = serde_json::from_str(&trace_string) - .map_err(Error::SerdeError) - .unwrap(); - for trace in trace.iter() { - assert!(!trace.invalid); - } + assert!(trace(config).is_ok()); } } @@ -159,15 +150,7 @@ mod test { ] }"#, ] { - let trace_result = trace(config); - assert!(trace_result.is_ok()); - let trace_string = trace_result.unwrap(); - let trace: Vec = serde_json::from_str(&trace_string) - .map_err(Error::SerdeError) - .unwrap(); - for trace in trace.iter() { - assert!(trace.invalid); - } + assert!(trace(config).is_err()) } } } diff --git a/mock/src/test_ctx.rs b/mock/src/test_ctx.rs index bc23495c8a..befff9f3e0 100644 --- a/mock/src/test_ctx.rs +++ b/mock/src/test_ctx.rs @@ -154,7 +154,7 @@ impl TestContext { // Build Block modifiers let mut block = MockBlock::default(); block.transactions.extend_from_slice(&transactions); - func_block(&mut block, transactions.clone()).build(); + func_block(&mut block, transactions).build(); let chain_id = block.chain_id; let block = Block::::from(block); @@ -174,17 +174,6 @@ impl TestContext { logger_config, )?; - // Don't allow invalid transactions unless explicitely allowed to avoid unrelated tests from - // passing simply because the test transaction was incorrectly set up. - for (transaction, geth_trace) in transactions.iter().zip(geth_traces.iter()) { - if !transaction.enable_skipping_invalid_tx && geth_trace.invalid { - panic!( - "{:?}", - Error::TracingError(geth_trace.return_value.clone()).to_string() - ) - } - } - Ok(Self { chain_id, accounts, diff --git a/mock/src/transaction.rs b/mock/src/transaction.rs index 95ec506ba8..1b6c6ccd1f 100644 --- a/mock/src/transaction.rs +++ b/mock/src/transaction.rs @@ -137,7 +137,6 @@ pub struct MockTransaction { pub max_priority_fee_per_gas: Word, pub max_fee_per_gas: Word, pub chain_id: Word, - pub enable_skipping_invalid_tx: bool, } impl Default for MockTransaction { @@ -162,7 +161,6 @@ impl Default for MockTransaction { max_priority_fee_per_gas: Word::zero(), max_fee_per_gas: Word::zero(), chain_id: *MOCK_CHAIN_ID, - enable_skipping_invalid_tx: false, } } } @@ -282,12 +280,6 @@ impl MockTransaction { self } - /// Set enable_skipping_invalid_tx field for the MockTransaction. - pub fn enable_skipping_invalid_tx(&mut self, enable_skipping_invalid_tx: bool) -> &mut Self { - self.enable_skipping_invalid_tx = enable_skipping_invalid_tx; - self - } - /// Set access_list field for the MockTransaction. pub fn access_list(&mut self, access_list: AccessList) -> &mut Self { self.access_list = access_list; diff --git a/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs b/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs index 7eab16dc9b..39c404ce50 100644 --- a/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs +++ b/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs @@ -12,8 +12,8 @@ use crate::{ }, is_precompiled, math_gadget::{ - AddWordsGadget, ContractCreateGadget, IsEqualGadget, IsZeroGadget, LtGadget, - LtWordGadget, MulWordByU64Gadget, + ContractCreateGadget, IsEqualGadget, IsZeroGadget, MulWordByU64Gadget, + RangeCheckGadget, }, not, or, select, CachedRegion, Cell, StepRws, Word, }, @@ -22,7 +22,7 @@ use crate::{ table::{AccountFieldTag, CallContextFieldTag, TxFieldTag as TxContextFieldTag}, util::Expr, }; -use eth_types::{evm_types::GasCost, Field, ToLittleEndian, ToScalar, U256}; +use eth_types::{evm_types::GasCost, Field, ToLittleEndian, ToScalar}; use ethers_core::utils::{get_contract_address, keccak256}; use gadgets::util::expr_from_bytes; use halo2_proofs::{circuit::Value, plonk::Error}; @@ -42,15 +42,8 @@ pub(crate) struct BeginTxGadget { tx_value: Word, tx_call_data_length: Cell, tx_call_data_gas_cost: Cell, - tx_is_invalid: Cell, - tx_access_list_gas_cost: Cell, - nonce: Cell, - nonce_prev: Cell, - is_nonce_valid: IsEqualGadget, - effective_gas_fee: Word, - effective_tx_value: Word, reversion_info: ReversionInfo, - is_gas_not_enough: LtGadget, + sufficient_gas_left: RangeCheckGadget, transfer_with_gas_fee: TransferWithGasFeeGadget, phase2_code_hash: Cell, is_empty_code_hash: IsEqualGadget, @@ -58,9 +51,6 @@ pub(crate) struct BeginTxGadget { create: ContractCreateGadget, callee_not_exists: IsZeroGadget, is_caller_callee_equal: Cell, - total_eth_cost: AddWordsGadget, - total_eth_cost_sum: Word, - balance_not_enough: LtWordGadget, } impl ExecutionGadget for BeginTxGadget { @@ -87,7 +77,7 @@ impl ExecutionGadget for BeginTxGadget { reversion_info.is_persistent(), ); // rwc_delta += 1 - let [tx_nonce, tx_gas, tx_caller_address, tx_callee_address, tx_is_create, tx_call_data_length, tx_call_data_gas_cost, tx_is_invalid, tx_access_list_gas_cost] = + let [tx_nonce, tx_gas, tx_caller_address, tx_callee_address, tx_is_create, tx_call_data_length, tx_call_data_gas_cost] = [ TxContextFieldTag::Nonce, TxContextFieldTag::Gas, @@ -96,8 +86,6 @@ impl ExecutionGadget for BeginTxGadget { TxContextFieldTag::IsCreate, TxContextFieldTag::CallDataLength, TxContextFieldTag::CallDataGasCost, - TxContextFieldTag::TxInvalid, - TxContextFieldTag::AccessListGasCost, ] .map(|field_tag| cb.tx_context(tx_id.expr(), field_tag, None)); let tx_caller_address_is_zero = IsZeroGadget::construct(cb, tx_caller_address.expr()); @@ -123,21 +111,13 @@ impl ExecutionGadget for BeginTxGadget { cb.require_equal("tx_id is initialized to be 1", tx_id.expr(), 1.expr()); }); - // Increase caller's nonce if the tx is valid. - // (a valid tx caller's nonce always increases even if the tx ends with error) - let nonce = cb.query_cell(); - let nonce_prev = cb.query_cell(); - let is_nonce_valid = IsEqualGadget::construct(cb, tx_nonce.expr(), nonce_prev.expr()); - cb.require_equal( - "update nonce", - nonce.expr(), - nonce_prev.expr() + 1.expr() - tx_is_invalid.expr(), - ); + // Increase caller's nonce. + // (tx caller's nonce always increases even tx ends with error) cb.account_write( tx_caller_address.expr(), AccountFieldTag::Nonce, - nonce.expr(), - nonce_prev.expr(), + tx_nonce.expr() + 1.expr(), + tx_nonce.expr(), None, ); // rwc_delta += 1 @@ -153,12 +133,11 @@ impl ExecutionGadget for BeginTxGadget { tx_is_create.expr(), GasCost::CREATION_TX.expr(), GasCost::TX.expr(), - ) + tx_call_data_gas_cost.expr() - + tx_access_list_gas_cost.expr(); + ) + tx_call_data_gas_cost.expr(); // Check gas_left is sufficient - let gas_left = tx_gas.expr() - intrinsic_gas_cost.clone(); - let is_gas_not_enough = LtGadget::construct(cb, tx_gas.expr(), intrinsic_gas_cost); + let gas_left = tx_gas.expr() - intrinsic_gas_cost; + let sufficient_gas_left = RangeCheckGadget::construct(cb, gas_left.clone()); // Prepare access list of caller and callee cb.account_access_list_write( @@ -199,76 +178,17 @@ impl ExecutionGadget for BeginTxGadget { }); // Transfer value from caller to callee, creating account if necessary. - // For invalid transactions we do not do any transfers - // A bit awkward for now because TransferWithGasFeeGadget requires words, - // will be cleaner after lo/hi split. - let effective_gas_fee = cb.query_word_rlc(); - let effective_tx_value = cb.query_word_rlc(); - cb.condition(tx_is_invalid.expr(), |cb| { - cb.require_equal( - "effective_tx_value == 0", - effective_tx_value.clone().expr(), - 0.expr(), - ); - cb.require_equal( - "effective_gas_fee == 0", - effective_gas_fee.clone().expr(), - 0.expr(), - ); - }); - cb.condition(not::expr(tx_is_invalid.expr()), |cb| { - cb.require_equal( - "effective_tx_value == tx_value", - effective_tx_value.expr(), - tx_value.expr(), - ); - cb.require_equal( - "effective_gas_fee == gas_fee", - effective_gas_fee.expr(), - mul_gas_fee_by_gas.product().expr(), - ); - }); let transfer_with_gas_fee = TransferWithGasFeeGadget::construct( cb, tx_caller_address.expr(), tx_callee_address.expr(), not::expr(callee_not_exists.expr()), - and::expr([ - not::expr(tx_is_invalid.expr()), - or::expr([tx_is_create.expr(), callee_not_exists.expr()]), - ]), - 1.expr(), - effective_tx_value.clone(), - effective_gas_fee.clone(), + or::expr([tx_is_create.expr(), callee_not_exists.expr()]), + tx_value.clone(), + mul_gas_fee_by_gas.product().clone(), &mut reversion_info, ); - // Check if the account ETH balance is sufficient - let sender_balance_prev = transfer_with_gas_fee.sender_sub_fee.balance_prev(); - let total_eth_cost_sum = cb.query_word_rlc(); - let total_eth_cost = AddWordsGadget::construct( - cb, - [tx_value.clone(), mul_gas_fee_by_gas.product().clone()], - total_eth_cost_sum.clone(), - ); - let balance_not_enough = - LtWordGadget::construct(cb, sender_balance_prev, total_eth_cost.sum()); - - // Check if the `is_invalid` value in the tx table is correct. - // A transaction is invalid when - // - The transaction requires more ETH than the transaction needs - // - The amount of gas specified in the transaction is lower than the intrinsic gas cost - // - The transaction nonce does not match the nonce stored in the account - cb.require_equal( - "is_tx_invalid is correct", - or::expr([ - balance_not_enough.expr(), - is_gas_not_enough.expr(), - not::expr(is_nonce_valid.expr()), - ]), - tx_is_invalid.expr(), - ); - let caller_nonce_hash_bytes = array_init::array_init(|_| cb.query_byte()); let create = ContractCreateGadget::construct(cb); cb.require_equal( @@ -378,11 +298,7 @@ impl ExecutionGadget for BeginTxGadget { // 3. Call to account with empty code. cb.condition( - and::expr([ - not::expr(tx_is_create.expr()), - no_callee_code.clone(), - tx_is_invalid.expr(), - ]), + and::expr([not::expr(tx_is_create.expr()), no_callee_code.clone()]), |cb| { cb.require_equal( "Tx to account with empty code should be persistent", @@ -415,11 +331,7 @@ impl ExecutionGadget for BeginTxGadget { // 4. Call to account with non-empty code. cb.condition( - and::expr([ - not::expr(tx_is_create.expr()), - not::expr(no_callee_code), - not::expr(tx_is_invalid.expr()), - ]), + and::expr([not::expr(tx_is_create.expr()), not::expr(no_callee_code)]), |cb| { // Setup first call's context. for (field_tag, value) in [ @@ -494,15 +406,8 @@ impl ExecutionGadget for BeginTxGadget { tx_value, tx_call_data_length, tx_call_data_gas_cost, - tx_is_invalid, - tx_access_list_gas_cost, - nonce, - nonce_prev, - is_nonce_valid, - effective_gas_fee, - effective_tx_value, reversion_info, - is_gas_not_enough, + sufficient_gas_left, transfer_with_gas_fee, phase2_code_hash, is_empty_code_hash, @@ -510,9 +415,6 @@ impl ExecutionGadget for BeginTxGadget { create, callee_not_exists, is_caller_callee_equal, - total_eth_cost, - total_eth_cost_sum, - balance_not_enough, } } @@ -529,8 +431,6 @@ impl ExecutionGadget for BeginTxGadget { let zero = eth_types::Word::zero(); let mut rws = StepRws::new(block, step); - rws.offset_add(4); - let caller_nonce_pair = rws.next().account_value_pair(); rws.offset_add(7); let mut callee_code_hash = zero; if !is_precompiled(&tx.callee_address) && !tx.is_create { @@ -543,9 +443,10 @@ impl ExecutionGadget for BeginTxGadget { if (!callee_exists && !tx.value.is_zero()) || must_create { callee_code_hash = rws.next().account_value_pair().1; } - let caller_balance_sub_value_pair = rws.next().account_value_pair(); + let mut caller_balance_sub_value_pair = (zero, zero); let mut callee_balance_pair = (zero, zero); if !tx.value.is_zero() { + caller_balance_sub_value_pair = rws.next().account_value_pair(); callee_balance_pair = rws.next().account_value_pair(); }; @@ -603,91 +504,23 @@ impl ExecutionGadget for BeginTxGadget { offset, Value::known(F::from(tx.call_data_gas_cost)), )?; - - self.tx_is_invalid - .assign(region, offset, Value::known(F::from(tx.invalid_tx as u64)))?; - self.tx_access_list_gas_cost.assign( - region, - offset, - Value::known(F::from(tx.access_list_gas_cost)), - )?; - - // Increase caller's nonce if the tx is valid. - let (nonce, nonce_prev) = caller_nonce_pair; - self.nonce - .assign(region, offset, Value::known(nonce.to_scalar().unwrap()))?; - self.nonce_prev.assign( - region, - offset, - Value::known(nonce_prev.to_scalar().unwrap()), - )?; - self.is_nonce_valid.assign( - region, - offset, - tx.nonce.to_scalar().unwrap(), - nonce_prev.to_scalar().unwrap(), - )?; - self.reversion_info.assign( region, offset, call.rw_counter_end_of_reversion, call.is_persistent, )?; - - let intrinsic_gas = select::value( - F::from(tx.is_create as u64), - F::from(GasCost::CREATION_TX.as_u64()), - F::from(GasCost::TX.as_u64()), - ) + F::from(tx.call_data_gas_cost) - + F::from(tx.access_list_gas_cost); - - // Check gas_left is sufficient - self.is_gas_not_enough - .assign(region, offset, F::from(tx.gas), intrinsic_gas)?; - - // Transfer value from caller to callee, creating account if necessary. - let (intrinsic_tx_value, intrinsic_gas_fee) = if !tx.invalid_tx { - (tx.value, gas_fee) - } else { - (U256::zero(), U256::zero()) - }; - self.effective_gas_fee.assign( - region, - offset, - Some(intrinsic_gas_fee.clone().to_le_bytes()), - )?; - self.effective_tx_value.assign( - region, - offset, - Some(intrinsic_tx_value.clone().to_le_bytes()), - )?; + self.sufficient_gas_left + .assign(region, offset, F::from(tx.gas - step.gas_cost.0))?; self.transfer_with_gas_fee.assign( region, offset, caller_balance_sub_fee_pair, caller_balance_sub_value_pair, callee_balance_pair, - intrinsic_tx_value, - intrinsic_gas_fee, - )?; - - // Check if the account ETH balance is sufficient - let total_eth_cost = tx.value + gas_fee; - self.total_eth_cost - .assign(region, offset, [tx.value, gas_fee], total_eth_cost)?; - self.total_eth_cost_sum.assign( - region, - offset, - Some(total_eth_cost.clone().to_le_bytes()), - )?; - self.balance_not_enough.assign( - region, - offset, - caller_balance_sub_fee_pair.1, - total_eth_cost, + tx.value, + gas_fee, )?; - self.phase2_code_hash .assign(region, offset, region.word_rlc(callee_code_hash))?; self.is_empty_code_hash.assign_value( @@ -733,7 +566,7 @@ mod test { use std::vec; use crate::{evm_circuit::test::rand_bytes, test_util::CircuitTestBuilder}; - use bus_mapping::{circuit_input_builder::CircuitsParams, evm::OpcodeId}; + use bus_mapping::evm::OpcodeId; use eth_types::{self, bytecode, evm_types::GasCost, word, Bytecode, Word}; use mock::{eth, gwei, MockTransaction, TestContext, MOCK_ACCOUNTS}; @@ -765,11 +598,7 @@ mod test { } } - fn test_ok( - tx: eth_types::Transaction, - code: Option, - enable_skipping_invalid_tx: bool, - ) { + fn test_ok(tx: eth_types::Transaction, code: Option) { // Get the execution steps from the external tracer let ctx = TestContext::<2, 1>::new( None, @@ -787,8 +616,7 @@ mod test { .gas_price(tx.gas_price.unwrap()) .gas(tx.gas) .input(tx.input) - .value(tx.value) - .enable_skipping_invalid_tx(enable_skipping_invalid_tx); + .value(tx.value); }, |block, _tx| block.number(0xcafeu64), ) @@ -813,46 +641,24 @@ mod test { eth_types::Transaction::from(mock_transaction) } - fn begin_tx_gadget_simple(enable_skipping_invalid_tx: bool) { + #[test] + fn begin_tx_gadget_simple() { // Transfer 1 ether to account with empty code, successfully - test_ok( - mock_tx(eth(1), gwei(2), vec![]), - None, - enable_skipping_invalid_tx, - ); + test_ok(mock_tx(eth(1), gwei(2), vec![]), None); // Transfer 1 ether, successfully - test_ok( - mock_tx(eth(1), gwei(2), vec![]), - Some(code_with_return()), - enable_skipping_invalid_tx, - ); + test_ok(mock_tx(eth(1), gwei(2), vec![]), Some(code_with_return())); // Transfer 1 ether, tx reverts - test_ok( - mock_tx(eth(1), gwei(2), vec![]), - Some(code_with_revert()), - enable_skipping_invalid_tx, - ); + test_ok(mock_tx(eth(1), gwei(2), vec![]), Some(code_with_revert())); // Transfer nothing with some calldata test_ok( mock_tx(eth(0), gwei(2), vec![1, 2, 3, 4, 0, 0, 0, 0]), Some(code_with_return()), - enable_skipping_invalid_tx, ); } - #[test] - fn begin_tx_gadget_simple_enable_skipping_invalid_tx() { - begin_tx_gadget_simple(true); - } - - #[test] - fn begin_tx_gadget_simple_disable_skipping_invalid_tx() { - begin_tx_gadget_simple(false); - } - #[test] fn begin_tx_large_nonce() { // This test checks that the rw table assignment and evm circuit are consistent @@ -883,7 +689,8 @@ mod test { CircuitTestBuilder::new_from_test_ctx(ctx).run(); } - fn begin_tx_gadget_rand(enable_skipping_invalid_tx: bool) { + #[test] + fn begin_tx_gadget_rand() { let random_amount = Word::from_little_endian(&rand_bytes(32)) % eth(1); let random_gas_price = Word::from_little_endian(&rand_bytes(32)) % gwei(2); // If this test fails, we want these values to appear in the CI logs. @@ -903,24 +710,10 @@ mod test { // Transfer nothing with random gas_price, tx reverts (eth(0), random_gas_price, vec![], Some(code_with_revert())), ] { - test_ok( - mock_tx(value, gas_price, calldata), - code, - enable_skipping_invalid_tx, - ); + test_ok(mock_tx(value, gas_price, calldata), code); } } - #[test] - fn begin_tx_gadget_rand_enable_skipping_invalid_tx() { - begin_tx_gadget_rand(true); - } - - #[test] - fn begin_tx_gadget_rand_disable_skipping_invalid_tx() { - begin_tx_gadget_rand(false); - } - #[test] fn begin_tx_no_code() { let ctx = TestContext::<2, 1>::new( @@ -1057,141 +850,4 @@ mod test { begin_tx_deploy(0x1020304050607080u64); begin_tx_deploy(0xfffffffffffffffeu64); } - - #[test] - #[should_panic] - fn begin_tx_disable_skipping_invalid_tx_invalid_nonce() { - begin_tx_invalid_nonce(false); - } - - #[test] - #[should_panic] - fn begin_tx_disable_skipping_invalid_tx_not_enough_eth() { - begin_tx_not_enough_eth(false); - } - - #[test] - #[should_panic] - fn begin_tx_disable_skipping_invalid_tx_insufficient_gas() { - begin_tx_insufficient_gas(false); - } - - #[test] - fn begin_tx_enable_skipping_invalid_tx() { - begin_tx_invalid_nonce(true); - begin_tx_not_enough_eth(true); - begin_tx_insufficient_gas(true); - } - - fn begin_tx_invalid_nonce(enable_skipping_invalid_tx: bool) { - // The nonce of the account doing the transaction is not correct - // Use the same nonce value for two transactions. - - let to = MOCK_ACCOUNTS[0]; - let from = MOCK_ACCOUNTS[1]; - - let code = bytecode! { - STOP - }; - - let ctx = TestContext::<2, 2>::new( - None, - |accs| { - accs[0].address(to).balance(eth(1)).code(code); - accs[1].address(from).balance(eth(1)).nonce(1); - }, - |mut txs, _| { - // Work around no payment to the coinbase address - txs[0].to(to).from(from).nonce(1); - txs[1] - .to(to) - .from(from) - .nonce(1) - .enable_skipping_invalid_tx(enable_skipping_invalid_tx); - }, - |block, _| block, - ) - .unwrap(); - - CircuitTestBuilder::new_from_test_ctx(ctx) - .params(CircuitsParams { - max_txs: 2, - ..Default::default() - }) - .run(); - } - - fn begin_tx_not_enough_eth(enable_skipping_invalid_tx: bool) { - // The account does not have enough ETH to pay for eth_value + tx_gas * - // tx_gas_price. - let to = MOCK_ACCOUNTS[0]; - let from = MOCK_ACCOUNTS[1]; - - let balance = gwei(1) + Word::from(10u64.pow(5)); - let ctx = TestContext::<2, 2>::new( - None, - |accs| { - accs[0].address(to).balance(balance); - accs[1].address(from).balance(balance).nonce(1); - }, - |mut txs, _| { - // Work around no payment to the coinbase address - txs[0] - .to(to) - .from(from) - .nonce(1) - .gas_price(Word::from(1u64)); - txs[1] - .to(to) - .from(from) - .nonce(2) - .gas_price(gwei(1)) - .gas(Word::from(10u64.pow(5))) - .enable_skipping_invalid_tx(enable_skipping_invalid_tx); - }, - |block, _| block, - ) - .unwrap(); - - CircuitTestBuilder::new_from_test_ctx(ctx) - .params(CircuitsParams { - max_txs: 2, - ..Default::default() - }) - .run(); - } - - fn begin_tx_insufficient_gas(enable_skipping_invalid_tx: bool) { - let to = MOCK_ACCOUNTS[0]; - let from = MOCK_ACCOUNTS[1]; - - let balance = eth(1); - let ctx = TestContext::<2, 2>::new( - None, - |accs| { - accs[0].address(to).balance(balance); - accs[1].address(from).balance(balance).nonce(1); - }, - |mut txs, _| { - // Work around no payment to the coinbase address - txs[0].to(to).from(from).nonce(1); - txs[1] - .to(to) - .from(from) - .nonce(2) - .gas_price(gwei(1)) - .gas(Word::from(1)) - .enable_skipping_invalid_tx(enable_skipping_invalid_tx); - }, - |block, _| block, - ) - .unwrap(); - - CircuitTestBuilder::new_from_test_ctx(ctx) - .params(CircuitsParams { - max_txs: 2, - ..Default::default() - }) - .run(); - } } diff --git a/zkevm-circuits/src/evm_circuit/execution/end_block.rs b/zkevm-circuits/src/evm_circuit/execution/end_block.rs index 768cf56867..0b3eaee2a4 100644 --- a/zkevm-circuits/src/evm_circuit/execution/end_block.rs +++ b/zkevm-circuits/src/evm_circuit/execution/end_block.rs @@ -21,7 +21,6 @@ use halo2_proofs::{circuit::Value, plonk::Error}; pub(crate) struct EndBlockGadget { total_txs: Cell, total_txs_is_max_txs: IsEqualGadget, - total_valid_txs: Cell, is_empty_block: IsZeroGadget, max_rws: Cell, max_txs: Cell, @@ -39,8 +38,6 @@ impl ExecutionGadget for EndBlockGadget { let max_rws = cb.query_copy_cell(); let total_txs = cb.query_cell(); let total_txs_is_max_txs = IsEqualGadget::construct(cb, total_txs.expr(), max_txs.expr()); - let total_valid_txs = cb.query_cell(); - // Note that rw_counter starts at 1 let is_empty_block = IsZeroGadget::construct(cb, cb.curr.state.rw_counter.clone().expr() - 1.expr()); @@ -49,19 +46,14 @@ impl ExecutionGadget for EndBlockGadget { let total_rws = not::expr(is_empty_block.expr()) * (cb.curr.state.rw_counter.clone().expr() - 1.expr() + 1.expr()); - // 1. Constraint total_valid_txs and total_txs witness values depending on the - // empty block case. + // 1. Constraint total_rws and total_txs witness values depending on the empty + // block case. cb.condition(is_empty_block.expr(), |cb| { // 1a. - // 1a. total_valid_txs is 0 in empty block - cb.require_equal( - "total_txs is 0 in empty block", - total_valid_txs.expr(), - 0.expr(), - ); + cb.require_equal("total_txs is 0 in empty block", total_txs.expr(), 0.expr()); }); cb.condition(not::expr(is_empty_block.expr()), |cb| { - // 1b. total_valid_txs matches the tx_id that corresponds to the final step. + // 1b. total_txs matches the tx_id that corresponds to the final step. cb.call_context_lookup(0.expr(), None, CallContextFieldTag::TxId, total_txs.expr()); }); @@ -114,7 +106,6 @@ impl ExecutionGadget for EndBlockGadget { max_rws, total_txs, total_txs_is_max_txs, - total_valid_txs, is_empty_block, } } @@ -139,12 +130,6 @@ impl ExecutionGadget for EndBlockGadget { .assign(region, offset, Value::known(total_txs))?; self.total_txs_is_max_txs .assign(region, offset, total_txs, max_txs)?; - let total_invalid_txs = block.txs.iter().filter(|&tx| tx.invalid_tx).count(); - self.total_valid_txs.assign( - region, - offset, - Value::known(F::from((block.txs.len() - total_invalid_txs) as u64)), - )?; let max_txs_assigned = self.max_txs.assign(region, offset, Value::known(max_txs))?; // When rw_indices is not empty, we're at the last row (at a fixed offset), // where we need to access the max_rws and max_txs constant. diff --git a/zkevm-circuits/src/evm_circuit/execution/end_tx.rs b/zkevm-circuits/src/evm_circuit/execution/end_tx.rs index b34baad6b3..5461f36ffc 100644 --- a/zkevm-circuits/src/evm_circuit/execution/end_tx.rs +++ b/zkevm-circuits/src/evm_circuit/execution/end_tx.rs @@ -34,7 +34,6 @@ pub(crate) struct EndTxGadget { effective_refund: MinMaxGadget, mul_gas_price_by_refund: MulWordByU64Gadget, tx_caller_address: Cell, - tx_is_invalid: Cell, gas_fee_refund: UpdateBalanceGadget, sub_gas_price_by_base_fee: AddWordsGadget, mul_effective_tip_by_gas_used: MulWordByU64Gadget, @@ -53,7 +52,6 @@ impl ExecutionGadget for EndTxGadget { fn configure(cb: &mut EVMConstraintBuilder) -> Self { let tx_id = cb.call_context(None, CallContextFieldTag::TxId); let is_persistent = cb.call_context(None, CallContextFieldTag::IsPersistent); - let tx_is_invalid = cb.tx_context(tx_id.expr(), TxContextFieldTag::TxInvalid, None); let [tx_gas, tx_caller_address] = [TxContextFieldTag::Gas, TxContextFieldTag::CallerAddress] @@ -71,14 +69,6 @@ impl ExecutionGadget for EndTxGadget { cb.tx_refund_read(tx_id.expr(), refund.expr()); let effective_refund = MinMaxGadget::construct(cb, max_refund.quotient(), refund.expr()); - // No refund if the tx is invalid - cb.condition(tx_is_invalid.expr(), |cb| { - cb.require_zero( - "refund == 0 if tx is invalid", - effective_refund.min().expr(), - ); - }); - // Add effective_refund * tx_gas_price back to caller's balance let mul_gas_price_by_refund = MulWordByU64Gadget::construct( cb, @@ -126,13 +116,6 @@ impl ExecutionGadget for EndTxGadget { TxReceiptFieldTag::LogLength, cb.curr.state.log_id.expr(), ); - // For invalid transactions the log id needs to be zero - cb.condition(tx_is_invalid.expr(), |cb| { - cb.require_zero( - "log_id is zero when tx is invalid", - cb.curr.state.log_id.expr(), - ); - }); let is_first_tx = IsEqualGadget::construct(cb, tx_id.expr(), 1.expr()); @@ -198,7 +181,6 @@ impl ExecutionGadget for EndTxGadget { effective_refund, mul_gas_price_by_refund, tx_caller_address, - tx_is_invalid, gas_fee_refund, sub_gas_price_by_base_fee, mul_effective_tip_by_gas_used, @@ -255,8 +237,6 @@ impl ExecutionGadget for EndTxGadget { .expect("unexpected Address -> Scalar conversion failure"), ), )?; - self.tx_is_invalid - .assign(region, offset, Value::known(F::from(tx.invalid_tx as u64)))?; self.gas_fee_refund.assign( region, offset, diff --git a/zkevm-circuits/src/evm_circuit/param.rs b/zkevm-circuits/src/evm_circuit/param.rs index e080f50cb4..e21530bb9c 100644 --- a/zkevm-circuits/src/evm_circuit/param.rs +++ b/zkevm-circuits/src/evm_circuit/param.rs @@ -9,7 +9,7 @@ use std::collections::HashMap; // Step dimension pub(crate) const STEP_WIDTH: usize = 128; /// Step height -pub const MAX_STEP_HEIGHT: usize = 24; +pub const MAX_STEP_HEIGHT: usize = 21; /// The height of the state of a step, used by gates that connect two /// consecutive steps. We target 1, which is also convenient for padding with /// EndBlock steps. diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 9ba79177cd..fba7e7770b 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -352,12 +352,11 @@ impl /// unconditionally if must_create is true. This gadget is used in BeginTx. #[derive(Clone, Debug)] pub(crate) struct TransferWithGasFeeGadget { - pub sender_sub_fee: UpdateBalanceGadget, - pub sender_sub_value: UpdateBalanceGadget, - pub receiver: UpdateBalanceGadget, + sender_sub_fee: UpdateBalanceGadget, + sender_sub_value: UpdateBalanceGadget, + receiver: UpdateBalanceGadget, receiver_exists: Expression, must_create: Expression, - must_read_caller_balance: Expression, pub(crate) value_is_zero: IsZeroGadget, } @@ -369,7 +368,6 @@ impl TransferWithGasFeeGadget { receiver_address: Expression, receiver_exists: Expression, must_create: Expression, - must_read_caller_balance: Expression, value: Word, gas_fee: Word, reversion_info: &mut ReversionInfo, @@ -394,23 +392,20 @@ impl TransferWithGasFeeGadget { }, ); // Skip transfer if value == 0 - // but still read the caller balance when required - let sender_sub_value = cb.condition( - or::expr([ - must_read_caller_balance.expr(), - not::expr(value_is_zero.expr()), - ]), - |cb| { - UpdateBalanceGadget::construct( - cb, - sender_address, - vec![value.clone()], - Some(reversion_info), - ) - }, - ); - let receiver = cb.condition(not::expr(value_is_zero.expr()), |cb| { - UpdateBalanceGadget::construct(cb, receiver_address, vec![value], Some(reversion_info)) + let (sender_sub_value, receiver) = cb.condition(not::expr(value_is_zero.expr()), |cb| { + let sender_sub_value = UpdateBalanceGadget::construct( + cb, + sender_address, + vec![value.clone()], + Some(reversion_info), + ); + let receiver = UpdateBalanceGadget::construct( + cb, + receiver_address, + vec![value], + Some(reversion_info), + ); + (sender_sub_value, receiver) }); Self { @@ -419,7 +414,6 @@ impl TransferWithGasFeeGadget { receiver, receiver_exists, must_create, - must_read_caller_balance, value_is_zero, } } @@ -434,8 +428,7 @@ impl TransferWithGasFeeGadget { ) * 1.expr() + // +1 Write Account (sender) Balance // +1 Write Account (receiver) Balance - or::expr([self.must_read_caller_balance.expr(), not::expr(self.value_is_zero.expr())]) + - not::expr(self.value_is_zero.expr()) + not::expr(self.value_is_zero.expr()) * 2.expr() } pub(crate) fn reversible_w_delta(&self) -> Expression { @@ -447,8 +440,7 @@ impl TransferWithGasFeeGadget { ) * 1.expr() + // +1 Write Account (sender) Balance // +1 Write Account (receiver) Balance - or::expr([self.must_read_caller_balance.expr(), not::expr(self.value_is_zero.expr())]) + - not::expr(self.value_is_zero.expr()) + not::expr(self.value_is_zero.expr()) * 2.expr() } #[allow(clippy::too_many_arguments)] diff --git a/zkevm-circuits/src/table/tx_table.rs b/zkevm-circuits/src/table/tx_table.rs index 3285d6a46a..9d7cb74c91 100644 --- a/zkevm-circuits/src/table/tx_table.rs +++ b/zkevm-circuits/src/table/tx_table.rs @@ -35,10 +35,6 @@ pub enum TxFieldTag { SigR, /// Signature field S. SigS, - /// Invalid tx - TxInvalid, - /// AccessListGasCost - AccessListGasCost, } impl_expr!(TxFieldTag); diff --git a/zkevm-circuits/src/witness/tx.rs b/zkevm-circuits/src/witness/tx.rs index 3ccd977a35..80780aa94b 100644 --- a/zkevm-circuits/src/witness/tx.rs +++ b/zkevm-circuits/src/witness/tx.rs @@ -37,10 +37,6 @@ pub struct Transaction { pub call_data_length: usize, /// The gas cost for transaction call data pub call_data_gas_cost: u64, - /// Invalid tx - pub invalid_tx: bool, - /// AccessListGasCost - pub access_list_gas_cost: u64, /// The calls made in the transaction pub calls: Vec, /// The steps executioned in the transaction @@ -150,18 +146,6 @@ impl Transaction { Value::known(F::ZERO), rlc_be_bytes(&self.s.to_be_bytes(), challenges.evm_word()), ], - [ - Value::known(F::from(self.id as u64)), - Value::known(F::from(TxContextFieldTag::TxInvalid as u64)), - Value::known(F::ZERO), - Value::known(F::from(self.invalid_tx as u64)), - ], - [ - Value::known(F::from(self.id as u64)), - Value::known(F::from(TxContextFieldTag::AccessListGasCost as u64)), - Value::known(F::ZERO), - Value::known(F::from(self.access_list_gas_cost)), - ], ]; let tx_calldata = self .call_data @@ -199,8 +183,6 @@ pub(super) fn tx_convert( call_data: tx.tx.call_data.to_vec(), call_data_length: tx.tx.call_data.len(), call_data_gas_cost: tx.tx.call_data_gas_cost(), - invalid_tx: tx.invalid_tx, - access_list_gas_cost: tx.access_list_gas_cost, calls: tx.calls().to_vec(), steps: tx.steps().to_vec(), v: tx.tx.v,