From 3d23a0e421acae8c488c8db6a7be65b186d998e8 Mon Sep 17 00:00:00 2001 From: KimiWu Date: Mon, 14 Aug 2023 15:36:02 +0800 Subject: [PATCH] feat: remove CREATE opcode from this oog_dynamic_memory --- .../circuit_input_builder/input_state_ref.rs | 49 ++-- .../evm/opcodes/error_oog_dynamic_memory.rs | 58 ++-- zkevm-circuits/src/evm_circuit/execution.rs | 2 +- .../execution/error_oog_dynamic_memory.rs | 267 ++++-------------- 4 files changed, 102 insertions(+), 274 deletions(-) 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 67e1ae60189..c3b7519fbaa 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -978,7 +978,7 @@ impl<'a> CircuitInputStateRef<'a> { let (offset, length) = match step.op { OpcodeId::RETURN | OpcodeId::REVERT => { let (offset, length) = if step.error.is_some() - || (self.call()?.is_create() && self.call()?.is_success()) + || (self.call()?.is_create() && self.call()?.is_success) { (0, 0) } else { @@ -1072,8 +1072,9 @@ impl<'a> CircuitInputStateRef<'a> { let geth_step = steps .get(0) .ok_or(Error::InternalError("invalid index 0"))?; - let is_revert_or_return_call_success = geth_step.op == OpcodeId::REVERT - || geth_step.op == OpcodeId::RETURN && exec_step.error.is_none(); + let is_revert_or_return_call_success = (geth_step.op == OpcodeId::REVERT + || geth_step.op == OpcodeId::RETURN) + && exec_step.error.is_none(); if !is_revert_or_return_call_success && !call.is_success { // add call failure ops for exception cases @@ -1127,25 +1128,29 @@ impl<'a> CircuitInputStateRef<'a> { _ => [Word::zero(), Word::zero()], }; - let curr_memory_word_size = (exec_step.memory_size as u64) / 32; - let next_memory_word_size = if !last_callee_return_data_length.is_zero() { - std::cmp::max( - (last_callee_return_data_offset + last_callee_return_data_length + 31).as_u64() - / 32, - curr_memory_word_size, - ) + let gas_refund = if exec_step.error.is_some() { + 0 } else { - curr_memory_word_size - }; + let curr_memory_word_size = (exec_step.memory_size as u64) / 32; + let next_memory_word_size = if !last_callee_return_data_length.is_zero() { + std::cmp::max( + (last_callee_return_data_offset + last_callee_return_data_length + 31).as_u64() + / 32, + curr_memory_word_size, + ) + } else { + curr_memory_word_size + }; - let memory_expansion_gas_cost = - memory_expansion_gas_cost(curr_memory_word_size, next_memory_word_size); - let code_deposit_cost = if call.is_create() && call.is_success { - GasCost::CODE_DEPOSIT_BYTE_COST * last_callee_return_data_length.as_u64() - } else { - 0 + let memory_expansion_gas_cost = + memory_expansion_gas_cost(curr_memory_word_size, next_memory_word_size); + let code_deposit_cost = if call.is_create() && call.is_success { + GasCost::CODE_DEPOSIT_BYTE_COST * last_callee_return_data_length.as_u64() + } else { + 0 + }; + geth_step.gas - memory_expansion_gas_cost - code_deposit_cost }; - let gas_refund = geth_step.gas - memory_expansion_gas_cost - code_deposit_cost; let caller_gas_left = if is_revert_or_return_call_success || call.is_success { geth_step_next.gas - gas_refund @@ -1179,11 +1184,13 @@ impl<'a> CircuitInputStateRef<'a> { } // EIP-211: CREATE/CREATE2 call successful case should set RETURNDATASIZE = 0 + let discard_return_data = + call.is_create() && geth_step.op == OpcodeId::RETURN || exec_step.error.is_some(); for (field, value) in [ (CallContextField::LastCalleeId, call.call_id.into()), ( CallContextField::LastCalleeReturnDataOffset, - if call.is_create() && call.is_success { + if discard_return_data { U256::zero() } else { last_callee_return_data_offset @@ -1191,7 +1198,7 @@ impl<'a> CircuitInputStateRef<'a> { ), ( CallContextField::LastCalleeReturnDataLength, - if call.is_create() && call.is_success { + if discard_return_data { U256::zero() } else { last_callee_return_data_length diff --git a/bus-mapping/src/evm/opcodes/error_oog_dynamic_memory.rs b/bus-mapping/src/evm/opcodes/error_oog_dynamic_memory.rs index 39245d6bca0..d4bc90e9b73 100644 --- a/bus-mapping/src/evm/opcodes/error_oog_dynamic_memory.rs +++ b/bus-mapping/src/evm/opcodes/error_oog_dynamic_memory.rs @@ -1,9 +1,10 @@ -use crate::circuit_input_builder::{CircuitInputStateRef, ExecStep}; -use crate::error::{ExecError, OogError}; -use crate::evm::Opcode; -use crate::Error; -use eth_types::evm_types::OpcodeId; -use eth_types::GethExecStep; +use crate::{ + circuit_input_builder::{CircuitInputStateRef, ExecStep}, + error::{ExecError, OogError}, + evm::Opcode, + Error, +}; +use eth_types::{evm_types::OpcodeId, GethExecStep}; #[derive(Clone, Copy, Debug)] pub(crate) struct OOGDynamicMemory {} @@ -14,44 +15,23 @@ impl Opcode for OOGDynamicMemory { geth_steps: &[GethExecStep], ) -> Result, Error> { let geth_step = &geth_steps[0]; - debug_assert!( - [OpcodeId::CREATE, OpcodeId::RETURN, OpcodeId::REVERT].contains(&geth_step.op) - ); + debug_assert!([OpcodeId::RETURN, OpcodeId::REVERT].contains(&geth_step.op)); let mut exec_step = state.new_step(geth_step)?; exec_step.error = Some(ExecError::OutOfGas(OogError::DynamicMemoryExpansion)); - if geth_step.op == OpcodeId::CREATE { - state.stack_read( - &mut exec_step, - geth_step.stack.last_filled(), - geth_step.stack.last()?, - )?; - state.stack_read( - &mut exec_step, - geth_step.stack.nth_last_filled(1), - geth_step.stack.nth_last(1)?, - )?; - state.stack_read( - &mut exec_step, - geth_step.stack.nth_last_filled(2), - geth_step.stack.nth_last(2)?, - )?; - } else { - state.stack_read( - &mut exec_step, - geth_step.stack.last_filled(), - geth_step.stack.last()?, - )?; - state.stack_read( - &mut exec_step, - geth_step.stack.nth_last_filled(1), - geth_step.stack.nth_last(1)?, - )?; - } + state.stack_read( + &mut exec_step, + geth_step.stack.last_filled(), + geth_step.stack.last()?, + )?; + state.stack_read( + &mut exec_step, + geth_step.stack.nth_last_filled(1), + geth_step.stack.nth_last(1)?, + )?; - state.gen_restore_context_ops(&mut exec_step, geth_steps)?; - state.handle_return(geth_step)?; + state.handle_return(&mut exec_step, geth_steps, true)?; Ok(vec![exec_step]) } diff --git a/zkevm-circuits/src/evm_circuit/execution.rs b/zkevm-circuits/src/evm_circuit/execution.rs index b9caa4292f8..70403e2a1b7 100644 --- a/zkevm-circuits/src/evm_circuit/execution.rs +++ b/zkevm-circuits/src/evm_circuit/execution.rs @@ -306,7 +306,7 @@ pub struct ExecutionConfig { Box>, error_stack: Box>, error_write_protection: Box>, - error_oog_dynamic_memory_gadget:Box>, + error_oog_dynamic_memory_gadget: Box>, error_oog_log: Box>, error_oog_sha3: Box>, error_oog_account_access: Box>, diff --git a/zkevm-circuits/src/evm_circuit/execution/error_oog_dynamic_memory.rs b/zkevm-circuits/src/evm_circuit/execution/error_oog_dynamic_memory.rs index 682f2d372e3..7722e6ecbf3 100644 --- a/zkevm-circuits/src/evm_circuit/execution/error_oog_dynamic_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/error_oog_dynamic_memory.rs @@ -1,167 +1,76 @@ -use crate::evm_circuit::execution::ExecutionGadget; -use crate::evm_circuit::param::{N_BYTES_GAS, N_BYTES_MEMORY_ADDRESS}; -use crate::evm_circuit::step::ExecutionState; -use crate::evm_circuit::util::common_gadget::RestoreContextGadget; -use crate::evm_circuit::util::constraint_builder::Transition::{Delta, Same}; -use crate::evm_circuit::util::constraint_builder::{ConstraintBuilder, StepStateTransition}; -use crate::evm_circuit::util::math_gadget::{IsEqualGadget, IsZeroGadget, LtGadget}; -use crate::evm_circuit::util::memory_gadget::{address_high, address_low, MemoryExpansionGadget}; -use crate::evm_circuit::util::{CachedRegion, Cell, Word}; -use crate::table::CallContextFieldTag; -use crate::witness::{Block, Call, ExecStep, Transaction}; -use eth_types::evm_types::OpcodeId; -use eth_types::{Field, ToLittleEndian}; -use gadgets::util::{and, not, select, Expr}; -use halo2_proofs::circuit::Value; -use halo2_proofs::plonk::Error; +use crate::{ + evm_circuit::{ + execution::ExecutionGadget, + param::{N_BYTES_GAS, N_BYTES_MEMORY_ADDRESS}, + step::ExecutionState, + util::{ + common_gadget::CommonErrorGadget, + constraint_builder::{ConstrainBuilderCommon, EVMConstraintBuilder}, + math_gadget::LtGadget, + memory_gadget::{MemoryAddressGadget, MemoryExpansionGadget, CommonMemoryAddressGadget}, + CachedRegion, Cell, + }, + }, + util::word::WordExpr, + witness::{Block, Call, ExecStep, Transaction}, +}; +use eth_types::{evm_types::OpcodeId, Field}; +use gadgets::util::Expr; +use halo2_proofs::{circuit::Value, plonk::Error}; #[derive(Clone, Debug)] pub(crate) struct ErrorOOGDynamicMemoryGadget { opcode: Cell, - is_create: IsEqualGadget, - is_return: IsEqualGadget, - value: Word, - address: Word, - size: Word, - address_in_range_high: IsZeroGadget, - size_in_range_high: IsZeroGadget, - expanded_address_in_range: LtGadget, + + memory_addr: MemoryAddressGadget, memory_expansion: MemoryExpansionGadget, insufficient_gas: LtGadget, - rw_counter_end_of_reversion: Cell, - restore_context: RestoreContextGadget, + common_error_gadget: CommonErrorGadget, } impl ExecutionGadget for ErrorOOGDynamicMemoryGadget { const NAME: &'static str = "ErrorOutOfGasDynamicMemoryExpansion"; const EXECUTION_STATE: ExecutionState = ExecutionState::ErrorOutOfGasDynamicMemoryExpansion; - // Support other OOG due to pure memory including CREATE, RETURN and REVERT - fn configure(cb: &mut ConstraintBuilder) -> Self { + // Support other OOG due to pure memory including RETURN and REVERT + fn configure(cb: &mut EVMConstraintBuilder) -> Self { let opcode = cb.query_cell(); - cb.opcode_lookup(opcode.expr(), 1.expr()); - - let is_create = IsEqualGadget::construct(cb, opcode.expr(), OpcodeId::CREATE.expr()); - let is_return = IsEqualGadget::construct(cb, opcode.expr(), OpcodeId::RETURN.expr()); - cb.require_equal( - "ErrorOutOfGasDynamicMemoryExpansion opcode must be CREATE or RETURN or REVERT", + cb.require_in_set( + "ErrorOutOfGasDynamicMemoryExpansion opcode must be RETURN or REVERT", opcode.expr(), - select::expr( - is_create.expr(), - OpcodeId::CREATE.expr(), - select::expr( - is_return.expr(), - OpcodeId::RETURN.expr(), - OpcodeId::REVERT.expr(), - ), - ), + vec![OpcodeId::RETURN.expr(), OpcodeId::REVERT.expr()], ); - let value = cb.query_word_rlc(); - cb.condition(is_create.expr(), |cb| { - cb.stack_pop(value.expr()); - }); - - let address = cb.query_word_rlc(); - cb.stack_pop(address.expr()); - let size = cb.query_word_rlc(); - cb.stack_pop(size.expr()); - - let address_high = address_high::expr(&address); - let address_in_range_high = IsZeroGadget::construct(cb, address_high); - let size_high = address_high::expr(&size); - let size_in_range_high = IsZeroGadget::construct(cb, size_high); - - let address_low = address_low::expr(&address); - let size_low = address_low::expr(&size); - let expanded_address = address_low.expr() + size_low.expr(); - let expanded_address_in_range = LtGadget::construct( - cb, - expanded_address.expr(), - (1u64 << (N_BYTES_MEMORY_ADDRESS * 8)).expr(), - ); - - cb.require_equal( - "address and size must less than 5 bytes", - and::expr([ - address_in_range_high.expr(), - size_in_range_high.expr(), - expanded_address_in_range.expr(), - ]), - true.expr(), - ); + let offset = cb.query_word_unchecked(); + let length = cb.query_memory_address(); + cb.stack_pop(offset.to_word()); + cb.stack_pop(length.to_word()); - let memory_expansion = - MemoryExpansionGadget::construct(cb, [address_low::expr(&address) + size.expr()]); + let memory_addr = MemoryAddressGadget::construct(cb, offset, length); + let memory_expansion = MemoryExpansionGadget::construct(cb, [memory_addr.address()]); + // constant gas of RETURN and REVERT is zero. let insufficient_gas = LtGadget::construct( cb, cb.curr.state.gas_left.expr(), - select::expr( - is_create.expr(), - OpcodeId::CREATE.constant_gas_cost().expr(), - select::expr( - is_return.expr(), - OpcodeId::RETURN.constant_gas_cost().expr(), - OpcodeId::REVERT.constant_gas_cost().expr(), - ), - ) + memory_expansion.gas_cost(), + memory_expansion.gas_cost(), ); - cb.require_equal( "gas_left must less than gas_cost", insufficient_gas.expr(), true.expr(), ); - // Current call must fail. - cb.call_context_lookup(false.expr(), None, CallContextFieldTag::IsSuccess, 0.expr()); - - let rw_counter_end_of_reversion = cb.query_cell(); - cb.call_context_lookup( - false.expr(), - None, - CallContextFieldTag::RwCounterEndOfReversion, - rw_counter_end_of_reversion.expr(), - ); - - cb.condition(cb.curr.state.is_root.expr(), |cb| { - cb.require_step_state_transition(StepStateTransition { - call_id: Same, - rw_counter: Delta( - 4.expr() + is_create.expr() + cb.curr.state.reversible_write_counter.expr(), - ), - ..StepStateTransition::any() - }); - }); - let restore_context = cb.condition(not::expr(cb.curr.state.is_root.expr()), |cb| { - RestoreContextGadget::construct( - cb, - 0.expr(), - 0.expr(), - 0.expr(), - 0.expr(), - 0.expr(), - 0.expr(), - 0.expr(), - ) - }); + let common_error_gadget = + CommonErrorGadget::construct(cb, opcode.expr(), cb.rw_counter_offset() + 2.expr()); Self { opcode, - is_create, - is_return, - value, - address, - size, - address_in_range_high, - size_in_range_high, - expanded_address_in_range, + memory_addr, memory_expansion, insufficient_gas, - rw_counter_end_of_reversion, - restore_context, + common_error_gadget, } } @@ -174,71 +83,15 @@ impl ExecutionGadget for ErrorOOGDynamicMemoryGadget { call: &Call, step: &ExecStep, ) -> Result<(), Error> { - let opcode = step.opcode.unwrap(); - + let opcode = step.opcode().unwrap(); self.opcode .assign(region, offset, Value::known(F::from(opcode.as_u64())))?; - self.is_create.assign( - region, - offset, - F::from(opcode.as_u64()), - F::from(OpcodeId::CREATE.as_u64()), - )?; - self.is_return.assign( - region, - offset, - F::from(opcode.as_u64()), - F::from(OpcodeId::RETURN.as_u64()), - )?; - - let mut rw_offset = 0; - if opcode == OpcodeId::CREATE { - let value = block.rws[step.rw_indices[rw_offset]].stack_value(); - self.value - .assign(region, offset, Some(value.to_le_bytes()))?; - rw_offset += 1; - } - let address = block.rws[step.rw_indices[rw_offset]].stack_value(); - let size = block.rws[step.rw_indices[rw_offset + 1]].stack_value(); - self.address - .assign(region, offset, Some(address.to_le_bytes()))?; - self.size.assign(region, offset, Some(size.to_le_bytes()))?; - - let address_high = address_high::value::(address.to_le_bytes()); - assert_eq!( - address_high, - F::zero(), - "address overflow {} bytes", - N_BYTES_MEMORY_ADDRESS - ); - self.address_in_range_high - .assign(region, offset, address_high)?; - let size_high = address_high::value::(size.to_le_bytes()); - assert_eq!( - size_high, - F::zero(), - "size overflow {} bytes", - N_BYTES_MEMORY_ADDRESS - ); - self.size_in_range_high.assign(region, offset, size_high)?; - - let address_value = address_low::value(address.to_le_bytes()); - let size_value = address_low::value(size.to_le_bytes()); - let expanded_address = address_value - .checked_add(size_value) - .expect("address overflow u64"); - assert!( - expanded_address < (1u64 << (N_BYTES_MEMORY_ADDRESS * 8)), - "expanded address overflow {} bytes", - N_BYTES_MEMORY_ADDRESS - ); - self.expanded_address_in_range.assign( - region, - offset, - F::from(expanded_address), - F::from(1u64 << (N_BYTES_MEMORY_ADDRESS * 8)), - )?; + let memory_offset = block.get_rws(step, 0).stack_value(); + let length = block.get_rws(step, 1).stack_value(); + let expanded_address = self + .memory_addr + .assign(region, offset, memory_offset, length)?; self.memory_expansion.assign( region, offset, @@ -253,13 +106,7 @@ impl ExecutionGadget for ErrorOOGDynamicMemoryGadget { F::from(step.gas_cost), )?; - self.rw_counter_end_of_reversion.assign( - region, - offset, - Value::known(F::from(call.rw_counter_end_of_reversion as u64)), - )?; - - self.restore_context.assign( + self.common_error_gadget.assign( region, offset, block, @@ -275,29 +122,23 @@ impl ExecutionGadget for ErrorOOGDynamicMemoryGadget { mod tests { use crate::test_util::CircuitTestBuilder; use eth_types::{bytecode, word, Bytecode, ToWord}; - use mock::test_ctx::helpers::account_0_code_account_1_no_code; - use mock::{eth, TestContext, MOCK_ACCOUNTS}; + use mock::{ + eth, test_ctx::helpers::account_0_code_account_1_no_code, TestContext, MOCK_ACCOUNTS, + }; #[test] fn test() { let codes = [ bytecode! { PUSH8(0xFF) - PUSH32(word!("0xffffffff")) // offset + PUSH32(word!("0xFFFFF")) // offset RETURN }, bytecode! { PUSH8(0xFF) - PUSH32(word!("0xffffffff")) // offset + PUSH32(word!("0xFFFFF")) // offset REVERT }, - bytecode! { - PUSH8(0xFF) - PUSH32(word!("0xffffffff")) // offset - PUSH8(0x0) // value - CREATE - STOP - }, ]; for code in codes.iter() { @@ -314,7 +155,7 @@ mod tests { txs[0] .from(accs[1].address) .to(accs[0].address) - .gas(word!("0xFFFF")); + .gas(word!("0xFFFFF")); }, |block, _tx| block, ) @@ -331,7 +172,7 @@ mod tests { PUSH32(0x00) // argsOffset PUSH1(0x00) // value PUSH32(MOCK_ACCOUNTS[1].to_word()) // addr - PUSH32(0xFFFF) // gas + PUSH32(word!("0xFFFFF")) // gas CALL STOP }; @@ -347,7 +188,7 @@ mod tests { txs[0] .from(accs[2].address) .to(accs[0].address) - .gas(word!("0xFFFFF")); + .gas(word!("0xFFFFFF")); }, |block, _tx| block, )