From 0152b2f4d7dac25c21bc3d64638bdbf3d334390d Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 17 Aug 2022 20:35:27 -0400 Subject: [PATCH 01/38] wip --- .../circuit_input_builder/input_state_ref.rs | 81 +++- bus-mapping/src/evm/opcodes/return.rs | 398 +++++++++++++----- bus-mapping/src/evm/opcodes/stop.rs | 48 ++- eth-types/src/lib.rs | 10 + .../src/evm_circuit/execution/return.rs | 294 ++++++++++++- .../src/evm_circuit/execution/stop.rs | 15 +- .../src/evm_circuit/util/common_gadget.rs | 48 ++- .../evm_circuit/util/constraint_builder.rs | 5 + 8 files changed, 726 insertions(+), 173 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 fb5217300a..715a0bd352 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -17,7 +17,7 @@ use crate::{ }; use eth_types::{ evm_types::{Gas, MemoryAddress, OpcodeId, StackAddress}, - Address, GethExecStep, ToAddress, ToBigEndian, Word, H256, + Address, GethExecStep, ToAddress, ToBigEndian, ToWord, Word, H256, }; use ethers_core::utils::{get_contract_address, get_create2_address}; @@ -813,8 +813,8 @@ impl<'a> CircuitInputStateRef<'a> { callee_account.code_hash = code_hash; } - // Handle reversion if this call doens't end successfully - if !self.call()?.is_success { + // Handle reversion if this call doesn't end successfully + if !call.is_success { self.handle_reversion(); } @@ -823,6 +823,81 @@ impl<'a> CircuitInputStateRef<'a> { Ok(()) } + ///asdfasdfasdfa + pub fn handle_restore_context( + &mut self, + steps: &[GethExecStep], + exec_step: &mut ExecStep, + ) -> Result<(), Error> { + let call = self.call()?.clone(); + let caller = self.caller()?.clone(); + self.call_context_read( + exec_step, + call.call_id, + CallContextField::CallerId, + caller.call_id.into(), + ); + + let geth_step = &steps[0]; + let geth_step_next = &steps[1]; + let caller_gas_left = geth_step_next.gas.0 - geth_step.gas.0; + for (field, value) in [ + (CallContextField::IsRoot, (caller.is_root as u64).into()), + ( + CallContextField::IsCreate, + (caller.is_create() as u64).into(), + ), + (CallContextField::CodeHash, caller.code_hash.to_word()), + (CallContextField::ProgramCounter, geth_step_next.pc.0.into()), + ( + CallContextField::StackPointer, + geth_step_next.stack.stack_pointer().0.into(), + ), + (CallContextField::GasLeft, caller_gas_left.into()), + ( + CallContextField::MemorySize, + geth_step_next.memory.word_size().into(), + ), + ( + CallContextField::ReversibleWriteCounter, + self.caller_ctx()?.reversible_write_counter.into(), + ), + ] { + self.call_context_read(exec_step, caller.call_id, field, value); + } + + let [last_callee_return_data_offset, last_callee_return_data_length] = match geth_step.op { + OpcodeId::STOP => [Word::zero(); 2], + OpcodeId::REVERT | OpcodeId::RETURN => { + // TODO: move this back into the return bus mapping. + let offset = geth_step.stack.nth_last(0)?; + let length = geth_step.stack.nth_last(1)?; + [offset, length] + } + _ => unreachable!(), + }; + + dbg!( + last_callee_return_data_offset, + last_callee_return_data_length + ); + for (field, value) in [ + (CallContextField::LastCalleeId, call.call_id.into()), + ( + CallContextField::LastCalleeReturnDataOffset, + last_callee_return_data_offset, + ), + ( + CallContextField::LastCalleeReturnDataLength, + last_callee_return_data_length, + ), + ] { + self.call_context_write(exec_step, caller.call_id, field, value); + } + + Ok(()) + } + /// Push a copy event to the state. pub fn push_copy(&mut self, copy: CopyEvent) { self.block.add_copy_event(copy); diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 3e6d232aaf..3d581f1a9f 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -1,144 +1,306 @@ -use crate::circuit_input_builder::{CircuitInputStateRef, ExecStep}; -use crate::evm::Opcode; -use crate::Error; -use eth_types::GethExecStep; +use super::Opcode; +use crate::circuit_input_builder::CopyEvent; +use crate::circuit_input_builder::CopyStep; +use crate::circuit_input_builder::{CopyDataType, NumberOrHash}; +use crate::operation::MemoryOp; +use crate::{ + circuit_input_builder::CircuitInputStateRef, + evm::opcodes::ExecStep, + operation::{CallContextField, RW}, + Error, +}; +use eth_types::{Bytecode, GethExecStep, ToWord, H256}; +use ethers_core::utils::keccak256; #[derive(Debug, Copy, Clone)] pub(crate) struct Return; +// rename to ReturnRevertStop? impl Opcode for Return { fn gen_associated_ops( state: &mut CircuitInputStateRef, - geth_steps: &[GethExecStep], + steps: &[GethExecStep], ) -> Result, Error> { - let geth_step = &geth_steps[0]; - let exec_step = state.new_step(geth_step)?; + let step = &steps[0]; + let mut exec_step = state.new_step(step)?; - let current_call = state.call()?.clone(); - let offset = geth_step.stack.nth_last(0)?.as_usize(); - let length = geth_step.stack.nth_last(1)?.as_usize(); + let offset = step.stack.nth_last(0)?; + let length = step.stack.nth_last(1)?; + state.stack_read(&mut exec_step, step.stack.nth_last_filled(0), offset)?; + state.stack_read(&mut exec_step, step.stack.nth_last_filled(1), length)?; + + if !length.is_zero() { + state + .call_ctx_mut()? + .memory + .extend_at_least((offset.low_u64() + length.low_u64()).try_into().unwrap()); + // TODO: handle memory expansion gas cost!! + } + + let call = state.call()?.clone(); + let is_root = call.is_root; + for (field, value) in [ + (CallContextField::IsRoot, is_root.to_word()), + (CallContextField::IsCreate, call.is_create().to_word()), + (CallContextField::IsSuccess, call.is_success.to_word()), // done in handle stop + (CallContextField::CallerId, call.caller_id.into()), + ( + CallContextField::ReturnDataOffset, + call.return_data_offset.into(), + ), + ( + CallContextField::ReturnDataLength, + call.return_data_length.into(), + ), + ] { + state.call_context_read(&mut exec_step, call.call_id, field, value); + } + + // move this into handle_restore_context? + state.call_context_read( + &mut exec_step, + call.call_id, + CallContextField::IsSuccess, + call.is_success.to_word(), + ); + + if !is_root { + state.handle_restore_context(steps, &mut exec_step)?; + } - // can we use ref here? let memory = state.call_ctx()?.memory.clone(); - // skip reconstruction for root-level return/revert - if !current_call.is_root { - if !current_call.is_create() { - // handle normal return/revert - // copy return data - // update to the caller memory - let caller_ctx = state.caller_ctx_mut()?; - let return_offset = current_call.return_data_offset as usize; - // already resized in Call::reconstruct_memory - // caller_ctx.memory.extend_at_least(return_offset + length); - let copy_len = std::cmp::min(current_call.return_data_length as usize, length); - caller_ctx.memory.0[return_offset..return_offset + copy_len] - .copy_from_slice(&memory.0[offset..offset + copy_len]); - caller_ctx.return_data.resize(length as usize, 0); - caller_ctx.return_data[0..copy_len] - .copy_from_slice(&memory.0[offset..offset + copy_len]); - } else { - // dealing with contract creation - assert!(offset + length <= memory.0.len()); - let code = memory.0[offset..offset + length].to_vec(); - state.code_db.insert(code); + let offset = offset.as_usize(); + let length = length.as_usize(); + if call.is_create() && call.is_success { + // is this always the case? + assert!(offset + length <= memory.0.len()); + let code = memory.0[offset..offset + length].to_vec(); + state.code_db.insert(code); + if length > 0 { + handle_create( + state, + &mut exec_step, + Source { + id: call.call_id, + offset: offset.try_into().unwrap(), + length: length.try_into().unwrap(), + }, + )?; + } + } else if !is_root { + let caller_ctx = state.caller_ctx_mut()?; + let return_offset = call.return_data_offset.try_into().unwrap(); + + let copy_len = std::cmp::min(call.return_data_length.try_into().unwrap(), length); + caller_ctx.memory.0[return_offset..return_offset + copy_len] + .copy_from_slice(&memory.0[offset..offset + copy_len]); + caller_ctx.return_data.resize(length, 0); + caller_ctx.return_data[0..copy_len] + .copy_from_slice(&memory.0[offset..offset + copy_len]); + + if length > 0 { + handle_copy( + state, + &mut exec_step, + Source { + id: call.call_id, + offset: offset.try_into().unwrap(), + length: length.try_into().unwrap(), + }, + Destination { + id: call.caller_id, + offset: call.return_data_offset.try_into().unwrap(), + length: call.return_data_length.try_into().unwrap(), + }, + )?; } } - state.handle_return(&geth_steps[0])?; + state.handle_return(step)?; Ok(vec![exec_step]) } } +struct Source { + id: usize, + offset: usize, + length: usize, +} + +struct Destination { + id: usize, + offset: usize, + length: usize, +} + +fn handle_copy( + state: &mut CircuitInputStateRef, + step: &mut ExecStep, + source: Source, + destination: Destination, +) -> Result<(), Error> { + let copy_length = std::cmp::min(source.length, destination.length); + let initial_rw_counter = state.block_ctx.rwc.0; + let rw_counter_increase = copy_length * 2; + + let bytes = state.call_ctx()?.memory.0[source.offset..source.offset + copy_length].to_vec(); + + let copy_steps = bytes + .iter() + .enumerate() + .flat_map(|(i, &byte)| { + [ + CopyStep { + addr: (source.offset + i).try_into().unwrap(), + tag: CopyDataType::Memory, + rw: RW::READ, + value: byte, + is_code: None, + is_pad: false, + rwc: (initial_rw_counter + 2 * i).into(), + rwc_inc_left: (rw_counter_increase - 2 * i).try_into().unwrap(), + }, + CopyStep { + addr: (destination.offset + i).try_into().unwrap(), + tag: CopyDataType::Memory, + rw: RW::WRITE, + value: byte, + is_code: None, + is_pad: false, + rwc: (initial_rw_counter + 2 * i + 1).into(), + rwc_inc_left: (rw_counter_increase - 2 * i - 1).try_into().unwrap(), + }, + ] + .into_iter() + }) + .collect(); + + for (i, &byte) in bytes.iter().enumerate() { + state.push_op( + step, + RW::READ, + MemoryOp::new(source.id, (source.offset + i).into(), byte), + ); + state.push_op( + step, + RW::WRITE, + MemoryOp::new(destination.id, (destination.offset + i).into(), byte), + ); + } + + state.push_copy(CopyEvent { + src_type: CopyDataType::Memory, + src_id: NumberOrHash::Number(source.id.try_into().unwrap()), + src_addr: 0, // not used + src_addr_end: (source.offset + copy_length).try_into().unwrap(), + dst_type: CopyDataType::Memory, + dst_id: NumberOrHash::Number(destination.id.try_into().unwrap()), + dst_addr: 0, // not used + length: copy_length.try_into().unwrap(), + log_id: None, + steps: copy_steps, + }); + + Ok(()) +} + +fn handle_create( + state: &mut CircuitInputStateRef, + step: &mut ExecStep, + source: Source, +) -> Result<(), Error> { + let bytes = state.call_ctx()?.memory.0[source.offset..source.offset + source.length].to_vec(); + let bytecode = Bytecode::from(bytes.clone()); + let initial_rw_counter = state.block_ctx.rwc.0; + let copy_steps = bytes + .iter() + .enumerate() + .flat_map(|(i, &byte)| { + [ + CopyStep { + addr: (source.offset + i).try_into().unwrap(), + tag: CopyDataType::Memory, + rw: RW::READ, + value: byte, + is_code: None, // does this matter? + is_pad: false, + rwc: (initial_rw_counter + i).into(), + rwc_inc_left: (source.length - i).try_into().unwrap(), + }, + CopyStep { + addr: i.try_into().unwrap(), + tag: CopyDataType::Bytecode, + rw: RW::WRITE, + value: byte, + is_code: Some(bytecode.get(i).unwrap().is_code), + is_pad: false, + rwc: (initial_rw_counter + i).into(), + rwc_inc_left: (source.length - i).try_into().unwrap(), + }, + ] + .into_iter() + }) + .collect(); + + state.push_copy(CopyEvent { + src_type: CopyDataType::Memory, + src_id: NumberOrHash::Number(source.id.try_into().unwrap()), + src_addr: 0, // not used + src_addr_end: (source.offset + source.length).try_into().unwrap(), + dst_type: CopyDataType::Bytecode, + dst_id: NumberOrHash::Hash(H256(keccak256(&bytes))), + dst_addr: 0, // not used + length: source.length.try_into().unwrap(), + log_id: None, + steps: copy_steps, + }); + + for (i, &byte) in bytes.iter().enumerate() { + state.push_op( + step, + RW::READ, + MemoryOp::new(source.id, (source.offset + i).into(), byte), + ); + } + + Ok(()) +} + #[cfg(test)] mod return_tests { use crate::mock::BlockData; use eth_types::geth_types::GethData; - use eth_types::{bytecode, word}; + use eth_types::{bytecode, word, Word}; use mock::test_ctx::helpers::{account_0_code_account_1_no_code, tx_from_1_to_0}; use mock::TestContext; - #[test] - fn test_ok() { - // // deployed contract - // PUSH1 0x20 - // PUSH1 0 - // PUSH1 0 - // CALLDATACOPY - // PUSH1 0x20 - // PUSH1 0 - // RETURN - // - // bytecode: 0x6020600060003760206000F3 - // - // // constructor - // PUSH12 0x6020600060003760206000F3 - // PUSH1 0 - // MSTORE - // PUSH1 0xC - // PUSH1 0x14 - // RETURN - // - // bytecode: 0x6B6020600060003760206000F3600052600C6014F3 - let code = bytecode! { - PUSH21(word!("6B6020600060003760206000F3600052600C6014F3")) + fn test_return( + callee_return_data_offset: usize, + callee_return_data_length: usize, + caller_return_data_offset: usize, + caller_return_data_length: usize, + ) { + let contract = bytecode! { + PUSH1(0x20) PUSH1(0) - MSTORE - - PUSH1 (0x15) - PUSH1 (0xB) - PUSH1 (0) - CREATE - - PUSH1 (0x20) - PUSH1 (0x20) - PUSH1 (0x20) - PUSH1 (0) - PUSH1 (0) - DUP6 - PUSH2 (0xFFFF) - CALL - STOP + PUSH1(0) + CALLDATACOPY + PUSH1(callee_return_data_length) + PUSH1(callee_return_data_offset) + RETURN }; - // Get the execution steps from the external tracer - let block: GethData = TestContext::<2, 1>::new( - None, - account_0_code_account_1_no_code(code), - tx_from_1_to_0, - |block, _tx| block.number(0xcafeu64), - ) - .unwrap() - .into(); - let mut builder = BlockData::new_from_geth_data(block.clone()).new_circuit_input_builder(); - builder - .handle_block(&block.eth_block, &block.geth_traces) - .unwrap(); - } + let constructor = bytecode! { + PUSH12(Word::from(contract.to_vec().as_slice())) + PUSH1(0) + MSTORE + PUSH1(0xC) + PUSH1(0x14) + RETURN + }; - #[test] - fn test_revert() { - // // deployed contract - // PUSH1 0x20 - // PUSH1 0 - // PUSH1 0 - // CALLDATACOPY - // PUSH1 0x20 - // PUSH1 0 - // REVERT - // - // bytecode: 0x6020600060003760206000FD - // - // // constructor - // PUSH12 0x6020600060003760206000FD - // PUSH1 0 - // MSTORE - // PUSH1 0xC - // PUSH1 0x14 - // RETURN - // - // bytecode: 0x6B6020600060003760206000FD600052600C6014F3 let code = bytecode! { - PUSH21(word!("6B6020600060003760206000FD600052600C6014F3")) + PUSH21(Word::from(constructor.to_vec().as_slice())) PUSH1(0) MSTORE @@ -147,14 +309,15 @@ mod return_tests { PUSH1 (0) CREATE - PUSH1 (0x20) - PUSH1 (0x20) + PUSH1 (caller_return_data_length) + PUSH1 (caller_return_data_offset) PUSH1 (0x20) PUSH1 (0) PUSH1 (0) DUP6 PUSH2 (0xFFFF) CALL + STOP }; // Get the execution steps from the external tracer @@ -172,4 +335,13 @@ mod return_tests { .handle_block(&block.eth_block, &block.geth_traces) .unwrap(); } + + #[test] + fn test_cases() { + test_return(0, 0, 0, 0); + test_return(10, 10, 10, 10); + test_return(0, 0, 0, 0); + test_return(0, 0, 0, 100); + test_return(0, 0, 0, 0); + } } diff --git a/bus-mapping/src/evm/opcodes/stop.rs b/bus-mapping/src/evm/opcodes/stop.rs index ee706d2203..d1cd938b82 100644 --- a/bus-mapping/src/evm/opcodes/stop.rs +++ b/bus-mapping/src/evm/opcodes/stop.rs @@ -4,7 +4,7 @@ use crate::{ operation::CallContextField, Error, }; -use eth_types::{GethExecStep, ToWord}; +use eth_types::{GethExecStep, ToWord, Word}; /// Placeholder structure used to implement [`Opcode`] trait over it /// corresponding to the [`OpcodeId::STOP`](crate::evm::OpcodeId::STOP) @@ -21,6 +21,7 @@ impl Opcode for Stop { geth_steps: &[GethExecStep], ) -> Result, Error> { let geth_step = &geth_steps[0]; + let mut exec_step = state.new_step(geth_step)?; let call = state.call()?.clone(); @@ -31,19 +32,15 @@ impl Opcode for Stop { CallContextField::IsSuccess, 1.into(), ); + // need to move this into handle_restore_context. + state.call_context_read( + &mut exec_step, + call.call_id, + CallContextField::IsSuccess, + call.is_success.to_word(), + ); - if call.is_root { - state.call_context_read( - &mut exec_step, - call.call_id, - CallContextField::IsPersistent, - 1.into(), - ); - } else { - // The following part corresponds to - // Instruction.step_state_transition_to_restored_context - // in python spec, and should be reusable among all expected halting opcodes or - // exceptions. TODO: Refactor it as a helper function. + if !call.is_root { let caller = state.caller()?.clone(); state.call_context_read( &mut exec_step, @@ -53,7 +50,6 @@ impl Opcode for Stop { ); let geth_step_next = &geth_steps[1]; - let caller_ctx = state.caller_ctx()?; let caller_gas_left = geth_step_next.gas.0 - geth_step.gas.0; for (field, value) in [ (CallContextField::IsRoot, (caller.is_root as u64).into()), @@ -70,7 +66,7 @@ impl Opcode for Stop { (CallContextField::GasLeft, caller_gas_left.into()), ( CallContextField::MemorySize, - caller_ctx.memory.word_size().into(), + geth_step_next.memory.word_size().into(), ), ( CallContextField::ReversibleWriteCounter, @@ -80,16 +76,32 @@ impl Opcode for Stop { state.call_context_read(&mut exec_step, caller.call_id, field, value); } + // let [last_callee_return_data_offset, last_callee_return_data_length] = + // match geth_step.op { + // OpcodeId::STOP => [Word::zero(); 2], + // OpcodeId::REVERT | OpcodeId::RETURN => { + // // TODO: move this back into the return bus mapping. + // let offset = geth_step.stack.nth_last(0)?; + // let length = geth_step.stack.nth_last(1)?; + // [offset, length] + // } + // _ => unreachable!(), + // }; + // + // dbg!( + // last_callee_return_data_offset, + // last_callee_return_data_length + // ); for (field, value) in [ (CallContextField::LastCalleeId, call.call_id.into()), - (CallContextField::LastCalleeReturnDataOffset, 0.into()), - (CallContextField::LastCalleeReturnDataLength, 0.into()), + (CallContextField::LastCalleeReturnDataOffset, Word::zero()), + (CallContextField::LastCalleeReturnDataLength, Word::zero()), ] { state.call_context_write(&mut exec_step, caller.call_id, field, value); } } - state.handle_return(geth_step)?; + state.handle_return(&geth_steps[0])?; Ok(vec![exec_step]) } diff --git a/eth-types/src/lib.rs b/eth-types/src/lib.rs index ee41adb7df..c738fb23db 100644 --- a/eth-types/src/lib.rs +++ b/eth-types/src/lib.rs @@ -184,6 +184,16 @@ impl ToWord for Address { } } +impl ToWord for bool { + fn to_word(&self) -> Word { + if *self { + Word::one() + } else { + Word::zero() + } + } +} + impl ToScalar for Address { fn to_scalar(&self) -> Option { let mut bytes = [0u8; 32]; diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index b70f0320ea..612058407c 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -1,20 +1,43 @@ +use crate::evm_circuit::util::memory_gadget::MemoryAddressGadget; use crate::{ evm_circuit::{ execution::ExecutionGadget, step::ExecutionState, - util::{constraint_builder::ConstraintBuilder, CachedRegion, Cell}, + util::{ + common_gadget::RestoreContextGadget, constraint_builder::ConstraintBuilder, not, + CachedRegion, Cell, + }, witness::{Block, Call, ExecStep, Transaction}, }, + table::CallContextFieldTag, util::Expr, }; +use bus_mapping::circuit_input_builder::CopyDataType; +use bus_mapping::evm::OpcodeId; use eth_types::Field; use halo2_proofs::plonk::Error; +use std::cmp::min; + #[derive(Clone, Debug)] pub(crate) struct ReturnGadget { opcode: Cell, + + range: MemoryAddressGadget, + + is_root: Cell, + is_create: Cell, + is_success: Cell, + restore_context: RestoreContextGadget, + + copy_length: Cell, // TODO: this needs more constraints. + + caller_id: Cell, // can you get this out of restore_context? + return_data_offset: Cell, + return_data_length: Cell, } +// This will handle reverts too? impl ExecutionGadget for ReturnGadget { const NAME: &'static str = "RETURN"; @@ -24,25 +47,278 @@ impl ExecutionGadget for ReturnGadget { let opcode = cb.query_cell(); cb.opcode_lookup(opcode.expr(), 1.expr()); - // TODO: Other constraints are ignored now for RETURN to serve as a - // mocking terminator + let offset = cb.query_cell(); + let length = cb.query_rlc(); + cb.stack_pop(offset.expr()); // +1 + cb.stack_pop(length.expr()); // +2 + let range = MemoryAddressGadget::construct(cb, offset, length); + + let [is_root, is_create, is_success, caller_id, return_data_offset, return_data_length] = [ + CallContextFieldTag::IsRoot, + CallContextFieldTag::IsCreate, + CallContextFieldTag::IsSuccess, + CallContextFieldTag::CallerId, + CallContextFieldTag::ReturnDataOffset, + CallContextFieldTag::ReturnDataLength, + ] + .map(|field_tag| cb.call_context(None, field_tag)); + + cb.condition(is_success.expr(), |cb| { + cb.require_equal( + "Opcode should be RETURN", + opcode.expr(), + OpcodeId::RETURN.expr(), + ) + }); + cb.condition(not::expr(is_success.expr()), |cb| { + cb.require_equal( + "Opcode should be REVERT", + opcode.expr(), + OpcodeId::REVERT.expr(), + ) + }); + + cb.condition(is_root.expr(), |cb| { + cb.require_next_state(ExecutionState::EndTx); + cb.call_context_lookup( + 0.expr(), + None, + CallContextFieldTag::IsSuccess, + is_success.expr(), + ); + }); - Self { opcode } + let copy_length = cb.query_cell(); + + let restore_context = cb.condition(not::expr(is_root.expr()), |cb| { + cb.require_next_state_not(ExecutionState::EndTx); + RestoreContextGadget::construct( + cb, + is_success.expr(), + copy_length.expr() + copy_length.expr(), + range.offset(), + range.length(), + ) + }); + + cb.condition( + not::expr(is_create.expr()) * not::expr(is_root.expr()) * range.has_length(), + |cb| { + cb.copy_table_lookup( + cb.curr.state.call_id.expr(), // source id + CopyDataType::Memory.expr(), // source tag + caller_id.expr(), // destination id + CopyDataType::Memory.expr(), // destination tag + range.offset(), // source address + range.offset() + copy_length.expr(), // + return_data_offset.expr(), // destination address + copy_length.expr(), // length + 0.expr(), + cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(), + copy_length.expr() + copy_length.expr(), + ); + }, + ); + + cb.condition( + is_create.expr() * is_success.expr() * range.has_length(), + |cb| { + cb.copy_table_lookup( + cb.curr.state.call_id.expr(), // source id + CopyDataType::Memory.expr(), // source tag + caller_id.expr(), // destination id + CopyDataType::Bytecode.expr(), // destination tag + range.offset(), // source address + range.address(), // + 0.expr(), // destination address + range.length(), // length + 0.expr(), + cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(), + range.length(), + ); + }, + ); + + Self { + opcode, + range, + is_root, + is_create, + is_success, + caller_id, + copy_length, + return_data_offset, + return_data_length, + restore_context, + } } fn assign_exec_step( &self, region: &mut CachedRegion<'_, '_, F>, offset: usize, - _: &Block, + block: &Block, _: &Transaction, - _: &Call, + call: &Call, step: &ExecStep, ) -> Result<(), Error> { - let opcode = step.opcode.unwrap(); - self.opcode - .assign(region, offset, Some(F::from(opcode.as_u64())))?; + self.opcode.assign( + region, + offset, + step.opcode.map(|opcode| F::from(opcode.as_u64())), + )?; + + let [memory_offset, length] = [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); + self.range + .assign(region, offset, memory_offset, length, block.randomness)?; + + self.is_root.assign( + region, + offset, + Some(if call.is_root { F::one() } else { F::zero() }), + )?; + self.is_create.assign( + region, + offset, + Some(if call.is_create { F::one() } else { F::zero() }), + )?; + self.is_success.assign( + region, + offset, + Some(if call.is_success { F::one() } else { F::zero() }), + )?; + + for (cell, value) in [ + (&self.caller_id, F::from(call.caller_id as u64)), + (&self.return_data_length, call.return_data_length.into()), + (&self.return_data_offset, call.return_data_offset.into()), + ] { + cell.assign(region, offset, Some(value))?; + } + + if !call.is_root { + self.restore_context + .assign(region, offset, block, call, step, 8)?; + } + + let copy_length = if call.is_root { + 0 + } else { + min(call.return_data_length, length.as_u64()) + }; + self.copy_length + .assign(region, offset, Some(F::from(copy_length)))?; Ok(()) } } + +#[cfg(test)] +mod test { + use crate::evm_circuit::test::run_test_circuit_incomplete_fixed_table; + use crate::evm_circuit::witness::block_convert; + use crate::test_util::run_test_circuits; + use eth_types::geth_types::Account; + use eth_types::{address, bytecode}; + use eth_types::{Address, ToWord, Word}; + use mock::TestContext; + + #[test] + fn test_return() { + let bytecode = bytecode! { + PUSH32(40) + PUSH32(30) // i think there's a memory expansion issue when there this value is too large? + RETURN + }; + + assert_eq!( + run_test_circuits( + TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode).unwrap(), + None + ), + Ok(()) + ); + } + // TODO: be sure to add tests that test offset = 0 + // root return with insufficient gas for memory expansion. + + #[test] + fn test_return_nonroot() { + let callee_bytecode = bytecode! { + PUSH32(Word::MAX) + PUSH1(Word::from(102u64)) + MSTORE + PUSH1(Word::from(10)) // length!?!? + PUSH2(Word::from(2)) // offset!?!?! + RETURN + }; + + let callee = Account { + address: Address::repeat_byte(0xff), + code: callee_bytecode.to_vec().into(), + nonce: Word::one(), + balance: 0xdeadbeefu64.into(), + ..Default::default() + }; + + let caller_bytecode = bytecode! { + PUSH32(Word::from(10)) // call_return_data_length + PUSH32(Word::from(10)) // call_return_data_offset + PUSH32(Word::from(14u64)) + PUSH32(Word::from(10u64)) + PUSH32(Word::from(4u64)) // value + PUSH32(Address::repeat_byte(0xff).to_word()) + PUSH32(Word::from(40000u64)) // gas + CALL + STOP + }; + + let caller = Account { + address: Address::repeat_byte(0x34), + code: caller_bytecode.to_vec().into(), + nonce: Word::one(), + balance: 0xdeadbeefu64.into(), + ..Default::default() + }; + + let block = TestContext::<3, 1>::new( + None, + |accs| { + accs[0] + .address(address!("0x000000000000000000000000000000000000cafe")) + .balance(Word::from(10u64.pow(19))); + accs[1] + .address(caller.address) + .code(caller.code) + .nonce(caller.nonce) + .balance(caller.balance); + accs[2] + .address(callee.address) + .code(callee.code) + .nonce(callee.nonce) + .balance(callee.balance); + }, + |mut txs, accs| { + txs[0] + .from(accs[0].address) + .to(accs[1].address) + .gas(100000u64.into()); + }, + |block, _tx| block.number(0xcafeu64), + ) + .unwrap(); + let block_data = bus_mapping::mock::BlockData::new_from_geth_data(block.into()); + let mut builder = block_data.new_circuit_input_builder(); + builder + .handle_block(&block_data.eth_block, &block_data.geth_traces) + .unwrap(); + + assert_eq!( + run_test_circuit_incomplete_fixed_table(block_convert( + &builder.block, + &builder.code_db + )), + Ok(()) + ); + } +} diff --git a/zkevm-circuits/src/evm_circuit/execution/stop.rs b/zkevm-circuits/src/evm_circuit/execution/stop.rs index 798f9f1c35..0f5a42367d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/stop.rs +++ b/zkevm-circuits/src/evm_circuit/execution/stop.rs @@ -65,12 +65,7 @@ impl ExecutionGadget for StopGadget { // When it's a root call cb.condition(cb.curr.state.is_root.expr(), |cb| { // When a transaction ends with STOP, this call must be persistent - cb.call_context_lookup( - false.expr(), - None, - CallContextFieldTag::IsPersistent, - 1.expr(), - ); + cb.call_context_lookup(false.expr(), None, CallContextFieldTag::IsSuccess, 1.expr()); // Do step state transition cb.require_step_state_transition(StepStateTransition { @@ -82,7 +77,7 @@ impl ExecutionGadget for StopGadget { // When it's an internal call let restore_context = cb.condition(1.expr() - cb.curr.state.is_root.expr(), |cb| { - RestoreContextGadget::construct(cb, 1.expr(), 0.expr(), 0.expr()) + RestoreContextGadget::construct(cb, true.expr(), 0.expr(), 0.expr(), 0.expr()) }); Self { @@ -119,8 +114,10 @@ impl ExecutionGadget for StopGadget { self.opcode .assign(region, offset, Some(F::from(opcode.as_u64())))?; - self.restore_context - .assign(region, offset, block, call, step)?; + if !call.is_root { + self.restore_context + .assign(region, offset, block, call, step, 1)?; + } Ok(()) } diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index ad8c411ca3..d821812a39 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -9,7 +9,7 @@ use crate::{ Transition::{Delta, Same, To}, }, math_gadget::{AddWordsGadget, RangeCheckGadget}, - Cell, Word, + not, Cell, Word, }, witness::{Block, Call, ExecStep}, }, @@ -31,7 +31,7 @@ pub(crate) struct SameContextGadget { impl SameContextGadget { pub(crate) fn construct( cb: &mut ConstraintBuilder, - opcode: Cell, + opcode: Cell, // this doesn't need to be a cell? step_state_transition: StepStateTransition, ) -> Self { cb.opcode_lookup(opcode.expr(), 1.expr()); @@ -82,6 +82,7 @@ impl SameContextGadget { /// Construction of step state transition that restores caller's state. #[derive(Clone, Debug)] pub(crate) struct RestoreContextGadget { + is_success: Cell, caller_id: Cell, caller_is_root: Cell, caller_is_create: Cell, @@ -96,10 +97,13 @@ pub(crate) struct RestoreContextGadget { impl RestoreContextGadget { pub(crate) fn construct( cb: &mut ConstraintBuilder, - rw_counter_delta: Expression, + is_success: Expression, + copy_lookup_rw_counter_increase: Expression, return_data_offset: Expression, return_data_length: Expression, ) -> Self { + let x = cb.call_context(None, CallContextFieldTag::IsSuccess); + // Read caller's context for restore let caller_id = cb.call_context(None, CallContextFieldTag::CallerId); let [caller_is_root, caller_is_create, caller_code_hash, caller_program_counter, caller_stack_pointer, caller_gas_left, caller_memory_word_size, caller_reversible_write_counter] = @@ -134,25 +138,28 @@ impl RestoreContextGadget { } // Consume all gas_left if call halts in exception + // this probably also needs to be run_test_circuit_incomplete_fixed_table let gas_left = if cb.execution_state().halts_in_exception() { caller_gas_left.expr() } else { caller_gas_left.expr() + cb.curr.state.gas_left.expr() }; + // also need to handle gas cost of mememory access in returns? // Accumulate reversible_write_counter in case this call stack reverts in the // future even it itself succeeds. Note that when sub-call halts in // failure, we don't need to accumulate reversible_write_counter because // what happened in the sub-call has been reverted. - let reversible_write_counter = if cb.execution_state().halts_in_success() { - caller_reversible_write_counter.expr() + cb.curr.state.reversible_write_counter.expr() - } else { - caller_reversible_write_counter.expr() - }; + let reversible_write_counter = caller_reversible_write_counter.expr() + + is_success.clone() * cb.curr.state.reversible_write_counter.expr(); + + let rw_counter_offset = cb.rw_counter_offset() + + copy_lookup_rw_counter_increase + + not::expr(is_success.expr()) * cb.curr.state.reversible_write_counter.expr(); // Do step state transition cb.require_step_state_transition(StepStateTransition { - rw_counter: Delta(rw_counter_delta + 12.expr()), + rw_counter: Delta(rw_counter_offset), call_id: To(caller_id.expr()), is_root: To(caller_is_root.expr()), is_create: To(caller_is_create.expr()), @@ -166,6 +173,7 @@ impl RestoreContextGadget { }); Self { + is_success: x, caller_id, caller_is_root, caller_is_create, @@ -185,23 +193,21 @@ impl RestoreContextGadget { block: &Block, call: &Call, step: &ExecStep, + rw_offset: usize, ) -> Result<(), Error> { + self.is_success.assign( + region, + offset, + Some(if call.is_success { F::one() } else { F::zero() }), + )?; + let [caller_id, caller_is_root, caller_is_create, caller_code_hash, caller_program_counter, caller_stack_pointer, caller_gas_left, caller_memory_word_size, caller_reversible_write_counter] = if call.is_root { [U256::zero(); 9] } else { - [ - step.rw_indices[1], - step.rw_indices[2], - step.rw_indices[3], - step.rw_indices[4], - step.rw_indices[5], - step.rw_indices[6], - step.rw_indices[7], - step.rw_indices[8], - step.rw_indices[9], - ] - .map(|idx| block.rws[idx].call_context_value()) + [1, 2, 3, 4, 5, 6, 7, 8, 9] + .map(|i| step.rw_indices[i + rw_offset]) + .map(|idx| block.rws[idx].call_context_value()) }; for (cell, value) in [ diff --git a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs index b242ca47cc..7fcc878de2 100644 --- a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs +++ b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs @@ -413,6 +413,11 @@ impl<'a, F: Field> ConstraintBuilder<'a, F> { ); } + pub(crate) fn require_next_state_not(&mut self, execution_state: ExecutionState) { + let next_state = self.next.execution_state_selector([execution_state]); + self.add_constraint("Constrain next execution state not", next_state.expr()); + } + pub(crate) fn require_step_state_transition( &mut self, step_state_transition: StepStateTransition, From fa58c806431a495907afd8fd770312c9cb92dc61 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 17 Aug 2022 21:11:01 -0400 Subject: [PATCH 02/38] Cleanup and implement ToScalar for bool --- bus-mapping/src/evm/opcodes/return.rs | 2 +- bus-mapping/src/evm/opcodes/stop.rs | 41 ++++++------ eth-types/src/lib.rs | 6 ++ .../src/evm_circuit/execution/return.rs | 63 +++++++------------ 4 files changed, 48 insertions(+), 64 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 3d581f1a9f..9423e3410d 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -270,7 +270,7 @@ fn handle_create( mod return_tests { use crate::mock::BlockData; use eth_types::geth_types::GethData; - use eth_types::{bytecode, word, Word}; + use eth_types::{bytecode, Word}; use mock::test_ctx::helpers::{account_0_code_account_1_no_code, tx_from_1_to_0}; use mock::TestContext; diff --git a/bus-mapping/src/evm/opcodes/stop.rs b/bus-mapping/src/evm/opcodes/stop.rs index d1cd938b82..4de59b55c8 100644 --- a/bus-mapping/src/evm/opcodes/stop.rs +++ b/bus-mapping/src/evm/opcodes/stop.rs @@ -4,7 +4,7 @@ use crate::{ operation::CallContextField, Error, }; -use eth_types::{GethExecStep, ToWord, Word}; +use eth_types::{GethExecStep, ToWord}; /// Placeholder structure used to implement [`Opcode`] trait over it /// corresponding to the [`OpcodeId::STOP`](crate::evm::OpcodeId::STOP) @@ -21,7 +21,6 @@ impl Opcode for Stop { geth_steps: &[GethExecStep], ) -> Result, Error> { let geth_step = &geth_steps[0]; - let mut exec_step = state.new_step(geth_step)?; let call = state.call()?.clone(); @@ -40,7 +39,18 @@ impl Opcode for Stop { call.is_success.to_word(), ); - if !call.is_root { + if call.is_root { + // state.call_context_read( + // &mut exec_step, + // call.call_id, + // CallContextField::IsPersistent, + // 1.into(), + // ); + } else { + // The following part corresponds to + // Instruction.step_state_transition_to_restored_context + // in python spec, and should be reusable among all expected halting opcodes or + // exceptions. TODO: Refactor it as a helper function. let caller = state.caller()?.clone(); state.call_context_read( &mut exec_step, @@ -50,6 +60,7 @@ impl Opcode for Stop { ); let geth_step_next = &geth_steps[1]; + let caller_ctx = state.caller_ctx()?; let caller_gas_left = geth_step_next.gas.0 - geth_step.gas.0; for (field, value) in [ (CallContextField::IsRoot, (caller.is_root as u64).into()), @@ -66,7 +77,7 @@ impl Opcode for Stop { (CallContextField::GasLeft, caller_gas_left.into()), ( CallContextField::MemorySize, - geth_step_next.memory.word_size().into(), + caller_ctx.memory.word_size().into(), ), ( CallContextField::ReversibleWriteCounter, @@ -76,32 +87,16 @@ impl Opcode for Stop { state.call_context_read(&mut exec_step, caller.call_id, field, value); } - // let [last_callee_return_data_offset, last_callee_return_data_length] = - // match geth_step.op { - // OpcodeId::STOP => [Word::zero(); 2], - // OpcodeId::REVERT | OpcodeId::RETURN => { - // // TODO: move this back into the return bus mapping. - // let offset = geth_step.stack.nth_last(0)?; - // let length = geth_step.stack.nth_last(1)?; - // [offset, length] - // } - // _ => unreachable!(), - // }; - // - // dbg!( - // last_callee_return_data_offset, - // last_callee_return_data_length - // ); for (field, value) in [ (CallContextField::LastCalleeId, call.call_id.into()), - (CallContextField::LastCalleeReturnDataOffset, Word::zero()), - (CallContextField::LastCalleeReturnDataLength, Word::zero()), + (CallContextField::LastCalleeReturnDataOffset, 0.into()), + (CallContextField::LastCalleeReturnDataLength, 0.into()), ] { state.call_context_write(&mut exec_step, caller.call_id, field, value); } } - state.handle_return(&geth_steps[0])?; + state.handle_return(geth_step)?; Ok(vec![exec_step]) } diff --git a/eth-types/src/lib.rs b/eth-types/src/lib.rs index c738fb23db..5ba5619374 100644 --- a/eth-types/src/lib.rs +++ b/eth-types/src/lib.rs @@ -203,6 +203,12 @@ impl ToScalar for Address { } } +impl ToScalar for bool { + fn to_scalar(&self) -> Option { + self.to_word().to_scalar() + } +} + /// Struct used to define the storage proof #[derive(Debug, Default, Clone, PartialEq, Eq, Deserialize)] pub struct StorageProof { diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 612058407c..d23801bbb2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -12,9 +12,8 @@ use crate::{ table::CallContextFieldTag, util::Expr, }; -use bus_mapping::circuit_input_builder::CopyDataType; -use bus_mapping::evm::OpcodeId; -use eth_types::Field; +use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; +use eth_types::{Field, ToScalar}; use halo2_proofs::plonk::Error; use std::cmp::min; @@ -49,8 +48,8 @@ impl ExecutionGadget for ReturnGadget { let offset = cb.query_cell(); let length = cb.query_rlc(); - cb.stack_pop(offset.expr()); // +1 - cb.stack_pop(length.expr()); // +2 + cb.stack_pop(offset.expr()); + cb.stack_pop(length.expr()); let range = MemoryAddressGadget::construct(cb, offset, length); let [is_root, is_create, is_success, caller_id, return_data_offset, return_data_length] = [ @@ -63,20 +62,12 @@ impl ExecutionGadget for ReturnGadget { ] .map(|field_tag| cb.call_context(None, field_tag)); - cb.condition(is_success.expr(), |cb| { - cb.require_equal( - "Opcode should be RETURN", - opcode.expr(), - OpcodeId::RETURN.expr(), - ) - }); - cb.condition(not::expr(is_success.expr()), |cb| { - cb.require_equal( - "Opcode should be REVERT", - opcode.expr(), - OpcodeId::REVERT.expr(), - ) - }); + cb.require_equal( + "if is_success, opcode is RETURN. if not, opcode is REVERT", + opcode.expr(), + is_success.expr() * OpcodeId::RETURN.expr() + + not::expr(is_success.expr()) * OpcodeId::REVERT.expr(), + ); cb.condition(is_root.expr(), |cb| { cb.require_next_state(ExecutionState::EndTx); @@ -172,24 +163,19 @@ impl ExecutionGadget for ReturnGadget { self.range .assign(region, offset, memory_offset, length, block.randomness)?; - self.is_root.assign( - region, - offset, - Some(if call.is_root { F::one() } else { F::zero() }), - )?; - self.is_create.assign( - region, - offset, - Some(if call.is_create { F::one() } else { F::zero() }), - )?; - self.is_success.assign( - region, - offset, - Some(if call.is_success { F::one() } else { F::zero() }), - )?; + for (cell, value) in [ + (&self.is_root, call.is_root), + (&self.is_create, call.is_create), + (&self.is_success, call.is_success), + ] { + cell.assign(region, offset, value.to_scalar())?; + } for (cell, value) in [ - (&self.caller_id, F::from(call.caller_id as u64)), + ( + &self.caller_id, + F::from(u64::try_from(call.caller_id).unwrap()), + ), (&self.return_data_length, call.return_data_length.into()), (&self.return_data_offset, call.return_data_offset.into()), ] { @@ -215,7 +201,7 @@ impl ExecutionGadget for ReturnGadget { #[cfg(test)] mod test { - use crate::evm_circuit::test::run_test_circuit_incomplete_fixed_table; + use crate::evm_circuit::test::run_test_circuit; use crate::evm_circuit::witness::block_convert; use crate::test_util::run_test_circuits; use eth_types::geth_types::Account; @@ -314,10 +300,7 @@ mod test { .unwrap(); assert_eq!( - run_test_circuit_incomplete_fixed_table(block_convert( - &builder.block, - &builder.code_db - )), + run_test_circuit(block_convert(&builder.block, &builder.code_db)), Ok(()) ); } From 214da7e86882f4a880aede4d2e43bc754e91cc96 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 17 Aug 2022 22:16:32 -0400 Subject: [PATCH 03/38] Remove extra is_success rw lookup --- bus-mapping/src/evm/opcodes/return.rs | 10 +--------- bus-mapping/src/evm/opcodes/stop.rs | 19 ++++++------------- .../src/evm_circuit/execution/call.rs | 1 + .../src/evm_circuit/execution/return.rs | 6 ------ .../src/evm_circuit/execution/stop.rs | 7 ++++++- .../src/evm_circuit/util/common_gadget.rs | 14 ++------------ 6 files changed, 16 insertions(+), 41 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 9423e3410d..79761a456d 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -42,7 +42,7 @@ impl Opcode for Return { for (field, value) in [ (CallContextField::IsRoot, is_root.to_word()), (CallContextField::IsCreate, call.is_create().to_word()), - (CallContextField::IsSuccess, call.is_success.to_word()), // done in handle stop + (CallContextField::IsSuccess, call.is_success.to_word()), (CallContextField::CallerId, call.caller_id.into()), ( CallContextField::ReturnDataOffset, @@ -56,14 +56,6 @@ impl Opcode for Return { state.call_context_read(&mut exec_step, call.call_id, field, value); } - // move this into handle_restore_context? - state.call_context_read( - &mut exec_step, - call.call_id, - CallContextField::IsSuccess, - call.is_success.to_word(), - ); - if !is_root { state.handle_restore_context(steps, &mut exec_step)?; } diff --git a/bus-mapping/src/evm/opcodes/stop.rs b/bus-mapping/src/evm/opcodes/stop.rs index 4de59b55c8..ee706d2203 100644 --- a/bus-mapping/src/evm/opcodes/stop.rs +++ b/bus-mapping/src/evm/opcodes/stop.rs @@ -31,21 +31,14 @@ impl Opcode for Stop { CallContextField::IsSuccess, 1.into(), ); - // need to move this into handle_restore_context. - state.call_context_read( - &mut exec_step, - call.call_id, - CallContextField::IsSuccess, - call.is_success.to_word(), - ); if call.is_root { - // state.call_context_read( - // &mut exec_step, - // call.call_id, - // CallContextField::IsPersistent, - // 1.into(), - // ); + state.call_context_read( + &mut exec_step, + call.call_id, + CallContextField::IsPersistent, + 1.into(), + ); } else { // The following part corresponds to // Instruction.step_state_transition_to_restored_context diff --git a/zkevm-circuits/src/evm_circuit/execution/call.rs b/zkevm-circuits/src/evm_circuit/execution/call.rs index 9c19c2edb2..8a7fcf47f7 100644 --- a/zkevm-circuits/src/evm_circuit/execution/call.rs +++ b/zkevm-circuits/src/evm_circuit/execution/call.rs @@ -763,6 +763,7 @@ mod test { ]; for (caller, callee) in callers.into_iter().cartesian_product(callees.into_iter()) { + dbg!("asdfasdf"); test_ok(caller, callee); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index d23801bbb2..42f29a52e8 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -71,12 +71,6 @@ impl ExecutionGadget for ReturnGadget { cb.condition(is_root.expr(), |cb| { cb.require_next_state(ExecutionState::EndTx); - cb.call_context_lookup( - 0.expr(), - None, - CallContextFieldTag::IsSuccess, - is_success.expr(), - ); }); let copy_length = cb.query_cell(); diff --git a/zkevm-circuits/src/evm_circuit/execution/stop.rs b/zkevm-circuits/src/evm_circuit/execution/stop.rs index 0f5a42367d..32dccfb315 100644 --- a/zkevm-circuits/src/evm_circuit/execution/stop.rs +++ b/zkevm-circuits/src/evm_circuit/execution/stop.rs @@ -65,7 +65,12 @@ impl ExecutionGadget for StopGadget { // When it's a root call cb.condition(cb.curr.state.is_root.expr(), |cb| { // When a transaction ends with STOP, this call must be persistent - cb.call_context_lookup(false.expr(), None, CallContextFieldTag::IsSuccess, 1.expr()); + cb.call_context_lookup( + false.expr(), + None, + CallContextFieldTag::IsPersistent, + 1.expr(), + ); // Do step state transition cb.require_step_state_transition(StepStateTransition { diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index d821812a39..2525d930fe 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -31,7 +31,7 @@ pub(crate) struct SameContextGadget { impl SameContextGadget { pub(crate) fn construct( cb: &mut ConstraintBuilder, - opcode: Cell, // this doesn't need to be a cell? + opcode: Cell, step_state_transition: StepStateTransition, ) -> Self { cb.opcode_lookup(opcode.expr(), 1.expr()); @@ -82,7 +82,6 @@ impl SameContextGadget { /// Construction of step state transition that restores caller's state. #[derive(Clone, Debug)] pub(crate) struct RestoreContextGadget { - is_success: Cell, caller_id: Cell, caller_is_root: Cell, caller_is_create: Cell, @@ -102,8 +101,6 @@ impl RestoreContextGadget { return_data_offset: Expression, return_data_length: Expression, ) -> Self { - let x = cb.call_context(None, CallContextFieldTag::IsSuccess); - // Read caller's context for restore let caller_id = cb.call_context(None, CallContextFieldTag::CallerId); let [caller_is_root, caller_is_create, caller_code_hash, caller_program_counter, caller_stack_pointer, caller_gas_left, caller_memory_word_size, caller_reversible_write_counter] = @@ -173,7 +170,6 @@ impl RestoreContextGadget { }); Self { - is_success: x, caller_id, caller_is_root, caller_is_create, @@ -195,17 +191,11 @@ impl RestoreContextGadget { step: &ExecStep, rw_offset: usize, ) -> Result<(), Error> { - self.is_success.assign( - region, - offset, - Some(if call.is_success { F::one() } else { F::zero() }), - )?; - let [caller_id, caller_is_root, caller_is_create, caller_code_hash, caller_program_counter, caller_stack_pointer, caller_gas_left, caller_memory_word_size, caller_reversible_write_counter] = if call.is_root { [U256::zero(); 9] } else { - [1, 2, 3, 4, 5, 6, 7, 8, 9] + [0, 1, 2, 3, 4, 5, 6, 7, 8] .map(|i| step.rw_indices[i + rw_offset]) .map(|idx| block.rws[idx].call_context_value()) }; From c01fec4dedf5e31b871e969c153dfad5552e3043 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 17 Aug 2022 22:39:18 -0400 Subject: [PATCH 04/38] Constraint copy length --- .../src/evm_circuit/execution/call.rs | 1 - .../src/evm_circuit/execution/return.rs | 50 +++++++++---------- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/call.rs b/zkevm-circuits/src/evm_circuit/execution/call.rs index 8a7fcf47f7..9c19c2edb2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/call.rs +++ b/zkevm-circuits/src/evm_circuit/execution/call.rs @@ -763,7 +763,6 @@ mod test { ]; for (caller, callee) in callers.into_iter().cartesian_product(callees.into_iter()) { - dbg!("asdfasdf"); test_ok(caller, callee); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 42f29a52e8..db476a7e93 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -2,10 +2,11 @@ use crate::evm_circuit::util::memory_gadget::MemoryAddressGadget; use crate::{ evm_circuit::{ execution::ExecutionGadget, + param::N_BYTES_MEMORY_ADDRESS, step::ExecutionState, util::{ - common_gadget::RestoreContextGadget, constraint_builder::ConstraintBuilder, not, - CachedRegion, Cell, + common_gadget::RestoreContextGadget, constraint_builder::ConstraintBuilder, + math_gadget::MinMaxGadget, not, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -16,8 +17,6 @@ use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; use eth_types::{Field, ToScalar}; use halo2_proofs::plonk::Error; -use std::cmp::min; - #[derive(Clone, Debug)] pub(crate) struct ReturnGadget { opcode: Cell, @@ -29,14 +28,14 @@ pub(crate) struct ReturnGadget { is_success: Cell, restore_context: RestoreContextGadget, - copy_length: Cell, // TODO: this needs more constraints. + nonroot_copy_length: MinMaxGadget, caller_id: Cell, // can you get this out of restore_context? return_data_offset: Cell, return_data_length: Cell, } -// This will handle reverts too? +// TODO: rename this is reflect the fact that is handles REVERT as well. impl ExecutionGadget for ReturnGadget { const NAME: &'static str = "RETURN"; @@ -73,14 +72,16 @@ impl ExecutionGadget for ReturnGadget { cb.require_next_state(ExecutionState::EndTx); }); - let copy_length = cb.query_cell(); + let nonroot_copy_length = + MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); + let copy_length = not::expr(is_root.expr()) * nonroot_copy_length.min(); let restore_context = cb.condition(not::expr(is_root.expr()), |cb| { cb.require_next_state_not(ExecutionState::EndTx); RestoreContextGadget::construct( cb, is_success.expr(), - copy_length.expr() + copy_length.expr(), + copy_length.clone() + copy_length.clone(), range.offset(), range.length(), ) @@ -90,17 +91,17 @@ impl ExecutionGadget for ReturnGadget { not::expr(is_create.expr()) * not::expr(is_root.expr()) * range.has_length(), |cb| { cb.copy_table_lookup( - cb.curr.state.call_id.expr(), // source id - CopyDataType::Memory.expr(), // source tag - caller_id.expr(), // destination id - CopyDataType::Memory.expr(), // destination tag - range.offset(), // source address - range.offset() + copy_length.expr(), // - return_data_offset.expr(), // destination address - copy_length.expr(), // length + cb.curr.state.call_id.expr(), + CopyDataType::Memory.expr(), + caller_id.expr(), + CopyDataType::Memory.expr(), + range.offset(), + range.offset() + copy_length.clone(), + return_data_offset.expr(), + copy_length.clone(), 0.expr(), cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(), - copy_length.expr() + copy_length.expr(), + copy_length.clone() + copy_length, ); }, ); @@ -131,7 +132,7 @@ impl ExecutionGadget for ReturnGadget { is_create, is_success, caller_id, - copy_length, + nonroot_copy_length, return_data_offset, return_data_length, restore_context, @@ -181,13 +182,12 @@ impl ExecutionGadget for ReturnGadget { .assign(region, offset, block, call, step, 8)?; } - let copy_length = if call.is_root { - 0 - } else { - min(call.return_data_length, length.as_u64()) - }; - self.copy_length - .assign(region, offset, Some(F::from(copy_length)))?; + self.nonroot_copy_length.assign( + region, + offset, + F::from(call.return_data_length), + F::from(length.as_u64()), + )?; Ok(()) } From d6f4a277749b5f5243b2298a52e69f0d6d27a8b1 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 17 Aug 2022 22:42:45 -0400 Subject: [PATCH 05/38] clean up comments --- zkevm-circuits/src/evm_circuit/util/common_gadget.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 2525d930fe..c5baf4c699 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -135,13 +135,12 @@ impl RestoreContextGadget { } // Consume all gas_left if call halts in exception - // this probably also needs to be run_test_circuit_incomplete_fixed_table let gas_left = if cb.execution_state().halts_in_exception() { caller_gas_left.expr() } else { caller_gas_left.expr() + cb.curr.state.gas_left.expr() }; - // also need to handle gas cost of mememory access in returns? + // TODO: check that memory expansion cost for return/revert is accounted for. // Accumulate reversible_write_counter in case this call stack reverts in the // future even it itself succeeds. Note that when sub-call halts in From 16dde29a0b38ccad27323d11d6ffe2e50bff1506 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 17 Aug 2022 23:34:58 -0400 Subject: [PATCH 06/38] clippy and use correct values for copy event --- bus-mapping/src/evm/opcodes/return.rs | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 79761a456d..77be1494d1 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -64,18 +64,19 @@ impl Opcode for Return { let offset = offset.as_usize(); let length = length.as_usize(); if call.is_create() && call.is_success { - // is this always the case? assert!(offset + length <= memory.0.len()); let code = memory.0[offset..offset + length].to_vec(); + // TODO: is this already handled by state.handle_return(step) below? state.code_db.insert(code); + // Warning: this isn't covered in any test! if length > 0 { handle_create( state, &mut exec_step, Source { id: call.call_id, - offset: offset.try_into().unwrap(), - length: length.try_into().unwrap(), + offset, + length, }, )?; } @@ -96,8 +97,8 @@ impl Opcode for Return { &mut exec_step, Source { id: call.call_id, - offset: offset.try_into().unwrap(), - length: length.try_into().unwrap(), + offset, + length, }, Destination { id: call.caller_id, @@ -182,12 +183,12 @@ fn handle_copy( state.push_copy(CopyEvent { src_type: CopyDataType::Memory, - src_id: NumberOrHash::Number(source.id.try_into().unwrap()), - src_addr: 0, // not used + src_id: NumberOrHash::Number(source.id), + src_addr: source.offset.try_into().unwrap(), src_addr_end: (source.offset + copy_length).try_into().unwrap(), dst_type: CopyDataType::Memory, - dst_id: NumberOrHash::Number(destination.id.try_into().unwrap()), - dst_addr: 0, // not used + dst_id: NumberOrHash::Number(destination.id), + dst_addr: destination.offset.try_into().unwrap(), length: copy_length.try_into().unwrap(), log_id: None, steps: copy_steps, @@ -214,7 +215,7 @@ fn handle_create( tag: CopyDataType::Memory, rw: RW::READ, value: byte, - is_code: None, // does this matter? + is_code: None, is_pad: false, rwc: (initial_rw_counter + i).into(), rwc_inc_left: (source.length - i).try_into().unwrap(), @@ -236,7 +237,7 @@ fn handle_create( state.push_copy(CopyEvent { src_type: CopyDataType::Memory, - src_id: NumberOrHash::Number(source.id.try_into().unwrap()), + src_id: NumberOrHash::Number(source.id), src_addr: 0, // not used src_addr_end: (source.offset + source.length).try_into().unwrap(), dst_type: CopyDataType::Bytecode, From 39557be24f6cdba008ae899d42ff7408cd8f305b Mon Sep 17 00:00:00 2001 From: z2trillion Date: Mon, 19 Sep 2022 16:39:01 -0400 Subject: [PATCH 07/38] wip --- bus-mapping/src/evm/opcodes/return.rs | 140 ++++++------------ eth-types/src/bytecode.rs | 3 +- .../src/evm_circuit/execution/return.rs | 6 +- 3 files changed, 49 insertions(+), 100 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 77be1494d1..81db92e02a 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -1,7 +1,5 @@ use super::Opcode; -use crate::circuit_input_builder::CopyEvent; -use crate::circuit_input_builder::CopyStep; -use crate::circuit_input_builder::{CopyDataType, NumberOrHash}; +use crate::circuit_input_builder::{CopyDataType, CopyEvent, NumberOrHash}; use crate::operation::MemoryOp; use crate::{ circuit_input_builder::CircuitInputStateRef, @@ -64,22 +62,22 @@ impl Opcode for Return { let offset = offset.as_usize(); let length = length.as_usize(); if call.is_create() && call.is_success { - assert!(offset + length <= memory.0.len()); - let code = memory.0[offset..offset + length].to_vec(); - // TODO: is this already handled by state.handle_return(step) below? - state.code_db.insert(code); - // Warning: this isn't covered in any test! - if length > 0 { - handle_create( - state, - &mut exec_step, - Source { - id: call.call_id, - offset, - length, - }, - )?; - } + // assert!(offset + length <= memory.0.len()); + // let code = memory.0[offset..offset + length].to_vec(); + // // TODO: is this already handled by state.handle_return(step) + // below? state.code_db.insert(code); + // // Warning: this isn't covered in any test! + // if length > 0 { + // handle_create( + // state, + // &mut exec_step, + // Source { + // id: call.call_id, + // offset, + // length, + // }, + // )?; + // } } else if !is_root { let caller_ctx = state.caller_ctx_mut()?; let return_offset = call.return_data_offset.try_into().unwrap(); @@ -133,55 +131,28 @@ fn handle_copy( destination: Destination, ) -> Result<(), Error> { let copy_length = std::cmp::min(source.length, destination.length); - let initial_rw_counter = state.block_ctx.rwc.0; - let rw_counter_increase = copy_length * 2; + let rw_counter_start = state.block_ctx.rwc; - let bytes = state.call_ctx()?.memory.0[source.offset..source.offset + copy_length].to_vec(); - - let copy_steps = bytes + let bytes: Vec<_> = state.call_ctx()?.memory.0[source.offset..source.offset + copy_length] .iter() - .enumerate() - .flat_map(|(i, &byte)| { - [ - CopyStep { - addr: (source.offset + i).try_into().unwrap(), - tag: CopyDataType::Memory, - rw: RW::READ, - value: byte, - is_code: None, - is_pad: false, - rwc: (initial_rw_counter + 2 * i).into(), - rwc_inc_left: (rw_counter_increase - 2 * i).try_into().unwrap(), - }, - CopyStep { - addr: (destination.offset + i).try_into().unwrap(), - tag: CopyDataType::Memory, - rw: RW::WRITE, - value: byte, - is_code: None, - is_pad: false, - rwc: (initial_rw_counter + 2 * i + 1).into(), - rwc_inc_left: (rw_counter_increase - 2 * i - 1).try_into().unwrap(), - }, - ] - .into_iter() - }) + .map(|byte| (*byte, false)) .collect(); - for (i, &byte) in bytes.iter().enumerate() { + for (i, (byte, _is_code)) in bytes.iter().enumerate() { state.push_op( step, RW::READ, - MemoryOp::new(source.id, (source.offset + i).into(), byte), + MemoryOp::new(source.id, (source.offset + i).into(), *byte), ); state.push_op( step, RW::WRITE, - MemoryOp::new(destination.id, (destination.offset + i).into(), byte), + MemoryOp::new(destination.id, (destination.offset + i).into(), *byte), ); } state.push_copy(CopyEvent { + rw_counter_start, src_type: CopyDataType::Memory, src_id: NumberOrHash::Number(source.id), src_addr: source.offset.try_into().unwrap(), @@ -189,9 +160,8 @@ fn handle_copy( dst_type: CopyDataType::Memory, dst_id: NumberOrHash::Number(destination.id), dst_addr: destination.offset.try_into().unwrap(), - length: copy_length.try_into().unwrap(), log_id: None, - steps: copy_steps, + bytes, }); Ok(()) @@ -204,58 +174,36 @@ fn handle_create( ) -> Result<(), Error> { let bytes = state.call_ctx()?.memory.0[source.offset..source.offset + source.length].to_vec(); let bytecode = Bytecode::from(bytes.clone()); - let initial_rw_counter = state.block_ctx.rwc.0; - let copy_steps = bytes + let rw_counter_start = state.block_ctx.rwc; + let bytes: Vec<_> = bytecode + .code .iter() - .enumerate() - .flat_map(|(i, &byte)| { - [ - CopyStep { - addr: (source.offset + i).try_into().unwrap(), - tag: CopyDataType::Memory, - rw: RW::READ, - value: byte, - is_code: None, - is_pad: false, - rwc: (initial_rw_counter + i).into(), - rwc_inc_left: (source.length - i).try_into().unwrap(), - }, - CopyStep { - addr: i.try_into().unwrap(), - tag: CopyDataType::Bytecode, - rw: RW::WRITE, - value: byte, - is_code: Some(bytecode.get(i).unwrap().is_code), - is_pad: false, - rwc: (initial_rw_counter + i).into(), - rwc_inc_left: (source.length - i).try_into().unwrap(), - }, - ] - .into_iter() - }) + .map(|element| (element.value, element.is_code)) .collect(); + let dst_id = NumberOrHash::Hash(H256(keccak256(&bytecode.to_vec()))); + + for (i, (byte, _)) in bytes.iter().enumerate() { + state.push_op( + step, + RW::READ, + MemoryOp::new(source.id, (source.offset + i).into(), *byte), + ); + } + state.push_copy(CopyEvent { + rw_counter_start, src_type: CopyDataType::Memory, src_id: NumberOrHash::Number(source.id), - src_addr: 0, // not used + src_addr: source.offset.try_into().unwrap(), src_addr_end: (source.offset + source.length).try_into().unwrap(), dst_type: CopyDataType::Bytecode, - dst_id: NumberOrHash::Hash(H256(keccak256(&bytes))), - dst_addr: 0, // not used - length: source.length.try_into().unwrap(), + dst_id, + dst_addr: 0, log_id: None, - steps: copy_steps, + bytes, }); - for (i, &byte) in bytes.iter().enumerate() { - state.push_op( - step, - RW::READ, - MemoryOp::new(source.id, (source.offset + i).into(), byte), - ); - } - Ok(()) } diff --git a/eth-types/src/bytecode.rs b/eth-types/src/bytecode.rs index 9df6250d93..fe0300dbb2 100644 --- a/eth-types/src/bytecode.rs +++ b/eth-types/src/bytecode.rs @@ -22,7 +22,8 @@ pub struct BytecodeElement { /// EVM Bytecode #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Bytecode { - code: Vec, + /// asdfasdf + pub code: Vec, num_opcodes: usize, markers: HashMap, } diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 26f2cecede..4ddebcd014 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -151,7 +151,7 @@ impl ExecutionGadget for ReturnGadget { self.opcode.assign( region, offset, - step.opcode.map(|opcode| F::from(opcode.as_u64())), + Value::known(F::from(step.opcode.unwrap().as_u64())), )?; let [memory_offset, length] = [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); @@ -163,7 +163,7 @@ impl ExecutionGadget for ReturnGadget { (&self.is_create, call.is_create), (&self.is_success, call.is_success), ] { - cell.assign(region, offset, value.to_scalar())?; + cell.assign(region, offset, Value::known(value.to_scalar().unwrap()))?; } for (cell, value) in [ @@ -174,7 +174,7 @@ impl ExecutionGadget for ReturnGadget { (&self.return_data_length, call.return_data_length.into()), (&self.return_data_offset, call.return_data_offset.into()), ] { - cell.assign(region, offset, Some(value))?; + cell.assign(region, offset, Value::known(value))?; } if !call.is_root { From b96ff25f533ba06b2dce48778b62fb11dbb19f93 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Mon, 19 Sep 2022 19:41:16 -0400 Subject: [PATCH 08/38] cleanup --- bus-mapping/src/evm/opcodes/return.rs | 13 +++--- .../src/evm_circuit/execution/return.rs | 44 +++++++++---------- 2 files changed, 26 insertions(+), 31 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 81db92e02a..2fc6fedd56 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -131,13 +131,12 @@ fn handle_copy( destination: Destination, ) -> Result<(), Error> { let copy_length = std::cmp::min(source.length, destination.length); - let rw_counter_start = state.block_ctx.rwc; - let bytes: Vec<_> = state.call_ctx()?.memory.0[source.offset..source.offset + copy_length] .iter() .map(|byte| (*byte, false)) .collect(); + let rw_counter_start = state.block_ctx.rwc; for (i, (byte, _is_code)) in bytes.iter().enumerate() { state.push_op( step, @@ -172,17 +171,15 @@ fn handle_create( step: &mut ExecStep, source: Source, ) -> Result<(), Error> { - let bytes = state.call_ctx()?.memory.0[source.offset..source.offset + source.length].to_vec(); - let bytecode = Bytecode::from(bytes.clone()); - let rw_counter_start = state.block_ctx.rwc; - let bytes: Vec<_> = bytecode + let values = state.call_ctx()?.memory.0[source.offset..source.offset + source.length].to_vec(); + let dst_id = NumberOrHash::Hash(H256(keccak256(&values))); + let bytes: Vec<_> = Bytecode::from(values) .code .iter() .map(|element| (element.value, element.is_code)) .collect(); - let dst_id = NumberOrHash::Hash(H256(keccak256(&bytecode.to_vec()))); - + let rw_counter_start = state.block_ctx.rwc; for (i, (byte, _)) in bytes.iter().enumerate() { state.push_op( step, diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 4ddebcd014..a2faa9f81b 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -90,17 +90,27 @@ impl ExecutionGadget for ReturnGadget { cb.condition( not::expr(is_create.expr()) * not::expr(is_root.expr()) * range.has_length(), |cb| { + let source_id = cb.curr.state.call_id.expr(); + let source_tag = CopyDataType::Memory.expr(); + let destination_id = caller_id.expr(); + let destination_tag = CopyDataType::Memory.expr(); + let source_address_start = range.offset(); + let source_address_end = range.offset() + copy_length.clone(); + let destination_address_start = return_data_offset.expr(); + let rlc_acc = 0.expr(); + let rw_counter_start = + cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(); cb.copy_table_lookup( - cb.curr.state.call_id.expr(), - CopyDataType::Memory.expr(), - caller_id.expr(), - CopyDataType::Memory.expr(), - range.offset(), - range.offset() + copy_length.clone(), - return_data_offset.expr(), + source_id, + source_tag, + destination_id, + destination_tag, + source_address_start, + source_address_end, + destination_address_start, copy_length.clone(), - 0.expr(), - cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(), + rlc_acc, + rw_counter_start, copy_length.clone() + copy_length, ); }, @@ -108,20 +118,8 @@ impl ExecutionGadget for ReturnGadget { cb.condition( is_create.expr() * is_success.expr() * range.has_length(), - |cb| { - cb.copy_table_lookup( - cb.curr.state.call_id.expr(), // source id - CopyDataType::Memory.expr(), // source tag - caller_id.expr(), // destination id - CopyDataType::Bytecode.expr(), // destination tag - range.offset(), // source address - range.address(), // - 0.expr(), // destination address - range.length(), // length - 0.expr(), - cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(), - range.length(), - ); + |_cb| { + // TODO: copy_table_lookup for contract creation }, ); From 7503d6e807b4a7c4ab04cfba9ef34b9dc7efad0a Mon Sep 17 00:00:00 2001 From: z2trillion Date: Mon, 19 Sep 2022 19:41:54 -0400 Subject: [PATCH 09/38] cleanup --- bus-mapping/src/evm/opcodes/return.rs | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 2fc6fedd56..8f69f9c00c 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -62,22 +62,7 @@ impl Opcode for Return { let offset = offset.as_usize(); let length = length.as_usize(); if call.is_create() && call.is_success { - // assert!(offset + length <= memory.0.len()); - // let code = memory.0[offset..offset + length].to_vec(); - // // TODO: is this already handled by state.handle_return(step) - // below? state.code_db.insert(code); - // // Warning: this isn't covered in any test! - // if length > 0 { - // handle_create( - // state, - // &mut exec_step, - // Source { - // id: call.call_id, - // offset, - // length, - // }, - // )?; - // } + unimplemented!(); } else if !is_root { let caller_ctx = state.caller_ctx_mut()?; let return_offset = call.return_data_offset.try_into().unwrap(); From 7ca57edbfdd3f12649278c4f49c19f351dc1a2bb Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 20 Sep 2022 11:19:51 -0400 Subject: [PATCH 10/38] Add create copy event --- bus-mapping/src/evm/opcodes/return.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 8f69f9c00c..364fdd8780 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -32,7 +32,7 @@ impl Opcode for Return { .call_ctx_mut()? .memory .extend_at_least((offset.low_u64() + length.low_u64()).try_into().unwrap()); - // TODO: handle memory expansion gas cost!! + // TODO: handle memory expansion gas cost? } let call = state.call()?.clone(); @@ -62,7 +62,19 @@ impl Opcode for Return { let offset = offset.as_usize(); let length = length.as_usize(); if call.is_create() && call.is_success { - unimplemented!(); + // Note: handle_return updates state.code_db. All we need to do here is push the + // copy event. + if length > 0 { + handle_create( + state, + &mut exec_step, + Source { + id: call.call_id, + offset, + length, + }, + )?; + } } else if !is_root { let caller_ctx = state.caller_ctx_mut()?; let return_offset = call.return_data_offset.try_into().unwrap(); From 7581924a18f93931e7967664225a2cfdf68db161 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 20 Sep 2022 17:14:56 -0400 Subject: [PATCH 11/38] wip --- bus-mapping/src/evm/opcodes/return.rs | 26 ++- mock/src/account.rs | 12 +- .../src/evm_circuit/execution/return.rs | 196 ++++++++++-------- 3 files changed, 140 insertions(+), 94 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 364fdd8780..de529a181c 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -204,26 +204,32 @@ fn handle_create( #[cfg(test)] mod return_tests { use crate::mock::BlockData; + use eth_types::evm_types::OpcodeId; use eth_types::geth_types::GethData; use eth_types::{bytecode, Word}; use mock::test_ctx::helpers::{account_0_code_account_1_no_code, tx_from_1_to_0}; use mock::TestContext; fn test_return( + is_return: bool, callee_return_data_offset: usize, callee_return_data_length: usize, caller_return_data_offset: usize, caller_return_data_length: usize, ) { - let contract = bytecode! { + let mut contract = bytecode! { PUSH1(0x20) PUSH1(0) PUSH1(0) CALLDATACOPY PUSH1(callee_return_data_length) PUSH1(callee_return_data_offset) - RETURN }; + contract.write_op(if is_return { + OpcodeId::RETURN + } else { + OpcodeId::REVERT + }); let constructor = bytecode! { PUSH12(Word::from(contract.to_vec().as_slice())) @@ -273,10 +279,16 @@ mod return_tests { #[test] fn test_cases() { - test_return(0, 0, 0, 0); - test_return(10, 10, 10, 10); - test_return(0, 0, 0, 0); - test_return(0, 0, 0, 100); - test_return(0, 0, 0, 0); + test_return(true, 0, 0, 0, 0); + test_return(true, 10, 10, 10, 10); + test_return(true, 0, 0, 0, 0); + test_return(true, 0, 0, 0, 100); + test_return(true, 0, 0, 0, 0); + + test_return(false, 0, 0, 0, 0); + test_return(false, 10, 10, 10, 10); + test_return(false, 0, 0, 0, 0); + test_return(false, 0, 0, 0, 100); + test_return(false, 0, 0, 0, 0); } } diff --git a/mock/src/account.rs b/mock/src/account.rs index cdaced292d..0cdda75549 100644 --- a/mock/src/account.rs +++ b/mock/src/account.rs @@ -6,7 +6,7 @@ use std::collections::HashMap; #[derive(Debug, Clone, Default)] /// Mock structure which represents an Account and can be used for tests. /// It contains all the builder-pattern methods required to be able to specify -/// any of it's details. +/// any of its details. pub struct MockAccount { /// Address pub address: Address, @@ -66,6 +66,16 @@ impl MockAccount { self } + /// Set all fields for the MockAccount based on their values in `account`. + pub fn account(&mut self, account: &Account) -> &mut Self { + self.address(account.address); + self.nonce(account.nonce); + self.balance(account.balance); + self.code(account.code.clone()); + self.storage(account.storage.iter().map(|(k, v)| (*k, *v))); + self + } + /// Finalizes the current MockAccount under construction returning a new /// instance to it. pub fn build(&mut self) -> Self { diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index a2faa9f81b..0f0b4d5173 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -199,104 +199,128 @@ mod test { use crate::evm_circuit::test::run_test_circuit; use crate::evm_circuit::witness::block_convert; use crate::test_util::run_test_circuits; + use eth_types::evm_types::OpcodeId; use eth_types::geth_types::Account; - use eth_types::{address, bytecode}; + use eth_types::{address, bytecode, Bytecode}; use eth_types::{Address, ToWord, Word}; use mock::TestContext; - #[test] - fn test_return() { - let bytecode = bytecode! { - PUSH32(40) - PUSH32(30) // i think there's a memory expansion issue when there this value is too large? - RETURN - }; - - assert_eq!( - run_test_circuits( - TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode).unwrap(), - None - ), - Ok(()) - ); - } - // TODO: be sure to add tests that test offset = 0 - // root return with insufficient gas for memory expansion. + const CALLEE_ADDRESS: Address = Address::repeat_byte(0xff); + const CALLER_ADDRESS: Address = Address::repeat_byte(0x34); - #[test] - fn test_return_nonroot() { - let callee_bytecode = bytecode! { - PUSH32(Word::MAX) - PUSH1(Word::from(102u64)) + fn callee_bytecode(is_return: bool, offset: u64, length: u64) -> Bytecode { + let memory_address = 2; + let memory_value = Word::MAX; + let mut code = bytecode! { + PUSH32(memory_value) + PUSH1(memory_address) MSTORE - PUSH1(Word::from(10)) // length!?!? - PUSH2(Word::from(2)) // offset!?!?! - RETURN - }; - - let callee = Account { - address: Address::repeat_byte(0xff), - code: callee_bytecode.to_vec().into(), - nonce: Word::one(), - balance: 0xdeadbeefu64.into(), - ..Default::default() + PUSH32(offset) + PUSH32(length) }; + code.write_op(if is_return { + OpcodeId::RETURN + } else { + OpcodeId::REVERT + }); + code + } - let caller_bytecode = bytecode! { - PUSH32(Word::from(10)) // call_return_data_length - PUSH32(Word::from(10)) // call_return_data_offset - PUSH32(Word::from(14u64)) - PUSH32(Word::from(10u64)) - PUSH32(Word::from(4u64)) // value - PUSH32(Address::repeat_byte(0xff).to_word()) - PUSH32(Word::from(40000u64)) // gas + fn caller_bytecode(call_return_data_length: u64, call_return_data_offset: u64) -> Bytecode { + let call_value = 1000; + let call_gas = 4000; + bytecode! { + PUSH32(call_return_data_length) + PUSH32(call_return_data_offset) + PUSH32(0) + PUSH32(0) + PUSH32(call_value) + PUSH32(CALLEE_ADDRESS.to_word()) + PUSH32(call_gas) CALL STOP - }; + } + } - let caller = Account { - address: Address::repeat_byte(0x34), - code: caller_bytecode.to_vec().into(), - nonce: Word::one(), - balance: 0xdeadbeefu64.into(), - ..Default::default() - }; + #[test] + fn test_root() { + let offset = 0; + let length = 10; + for is_return in [true, false] { + let code = callee_bytecode(is_return, offset, length); + assert_eq!( + run_test_circuits( + TestContext::<2, 1>::simple_ctx_with_bytecode(code).unwrap(), + None + ), + Ok(()), + "{}", + dbg!(is_return) + ); + } + } - let block = TestContext::<3, 1>::new( - None, - |accs| { - accs[0] - .address(address!("0x000000000000000000000000000000000000cafe")) - .balance(Word::from(10u64.pow(19))); - accs[1] - .address(caller.address) - .code(caller.code) - .nonce(caller.nonce) - .balance(caller.balance); - accs[2] - .address(callee.address) - .code(callee.code) - .nonce(callee.nonce) - .balance(callee.balance); - }, - |mut txs, accs| { - txs[0] - .from(accs[0].address) - .to(accs[1].address) - .gas(100000u64.into()); - }, - |block, _tx| block.number(0xcafeu64), - ) - .unwrap(); - let block_data = bus_mapping::mock::BlockData::new_from_geth_data(block.into()); - let mut builder = block_data.new_circuit_input_builder(); - builder - .handle_block(&block_data.eth_block, &block_data.geth_traces) + #[test] + fn test_return_nonroot() { + let callee_offset = 10; + let callee_length = 10; + + let caller_offset = 20; + let caller_length = 20; + + for is_return in [true, false] { + let callee = Account { + address: CALLEE_ADDRESS, + code: callee_bytecode(is_return, callee_offset, callee_length).into(), + nonce: Word::one(), + balance: 0xdeadbeefu64.into(), + ..Default::default() + }; + let call_argument_offset = 5; // these are flipped or something... + let call_argument_length = 5; // + let caller = Account { + address: CALLER_ADDRESS, + code: caller_bytecode(call_argument_offset, call_argument_length).into(), + nonce: Word::one(), + balance: 0xdeadbeefu64.into(), + ..Default::default() + }; + + let block = TestContext::<3, 1>::new( + None, + |accs| { + accs[0] + .address(address!("0x000000000000000000000000000000000000cafe")) + .balance(Word::from(10u64.pow(19))); + accs[1].account(&caller); + accs[2].account(&callee); + }, + |mut txs, accs| { + txs[0] + .from(accs[0].address) + .to(accs[1].address) + .gas(100000u64.into()); + }, + |block, _tx| block.number(0xcafeu64), + ) .unwrap(); - - assert_eq!( - run_test_circuit(block_convert(&builder.block, &builder.code_db)), - Ok(()) - ); + let block_data = bus_mapping::mock::BlockData::new_from_geth_data(block.into()); + let mut builder = block_data.new_circuit_input_builder(); + builder + .handle_block(&block_data.eth_block, &block_data.geth_traces) + .unwrap(); + + assert_eq!( + run_test_circuit(block_convert(&builder.block, &builder.code_db)), + Ok(()) + ); + } } + + // test_root_return + // test_nonroot_return + // test_memory_expansion.... + // + + // test_ } From 790025e98e75038cbb5471ead56e69785524c02a Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 13:48:29 -0400 Subject: [PATCH 12/38] wip --- .../src/evm_circuit/execution/return.rs | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 0f0b4d5173..c9303289d9 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -196,13 +196,13 @@ impl ExecutionGadget for ReturnGadget { #[cfg(test)] mod test { - use crate::evm_circuit::test::run_test_circuit; - use crate::evm_circuit::witness::block_convert; - use crate::test_util::run_test_circuits; - use eth_types::evm_types::OpcodeId; - use eth_types::geth_types::Account; - use eth_types::{address, bytecode, Bytecode}; - use eth_types::{Address, ToWord, Word}; + use crate::evm_circuit::{ + test::run_test_circuit, test_util::run_test_circuits, witness::block_convert, + }; + use eth_types::{ + address, bytecode, evm_types::OpcodeId, geth_types::Account, Address, Bytecode, ToWord, + Word, + }; use mock::TestContext; const CALLEE_ADDRESS: Address = Address::repeat_byte(0xff); @@ -227,7 +227,7 @@ mod test { } fn caller_bytecode(call_return_data_length: u64, call_return_data_offset: u64) -> Bytecode { - let call_value = 1000; + let call_value = 0; let call_gas = 4000; bytecode! { PUSH32(call_return_data_length) @@ -240,6 +240,8 @@ mod test { CALL STOP } + // what happens is that it reverts immediately, without being able to + // enter the inner call at all.. } #[test] @@ -273,7 +275,6 @@ mod test { address: CALLEE_ADDRESS, code: callee_bytecode(is_return, callee_offset, callee_length).into(), nonce: Word::one(), - balance: 0xdeadbeefu64.into(), ..Default::default() }; let call_argument_offset = 5; // these are flipped or something... @@ -282,7 +283,6 @@ mod test { address: CALLER_ADDRESS, code: caller_bytecode(call_argument_offset, call_argument_length).into(), nonce: Word::one(), - balance: 0xdeadbeefu64.into(), ..Default::default() }; @@ -312,7 +312,9 @@ mod test { assert_eq!( run_test_circuit(block_convert(&builder.block, &builder.code_db)), - Ok(()) + Ok(()), + "{}", + dbg!(is_return) ); } } From 12c3aa6fd497360ca61c3f58ca1fac104fcbd17a Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 22:50:33 -0400 Subject: [PATCH 13/38] wip --- .../circuit_input_builder/input_state_ref.rs | 36 +++-- bus-mapping/src/evm/opcodes/return.rs | 11 +- .../src/evm_circuit/execution/return.rs | 148 +++++++++--------- .../src/evm_circuit/util/memory_gadget.rs | 6 +- 4 files changed, 115 insertions(+), 86 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 7a443748ff..6d184e8ed4 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -96,7 +96,11 @@ impl<'a> CircuitInputStateRef<'a> { /// reference to the stored operation ([`OperationRef`]) inside the /// bus-mapping instance of the current [`ExecStep`]. Then increase the /// block_ctx [`RWCounter`](crate::operation::RWCounter) by one. - pub fn push_op(&mut self, step: &mut ExecStep, rw: RW, op: T) { + pub fn push_op(&mut self, step: &mut ExecStep, rw: RW, op: T) { + if self.block_ctx.rwc == 131.into() { + dbg!(op.clone()); + // panic!(); + } let op_ref = self.block .container @@ -628,6 +632,8 @@ impl<'a> CircuitInputStateRef<'a> { CallKind::Call | CallKind::CallCode => { let call_data = get_call_memory_offset_length(step, 3)?; let return_data = get_call_memory_offset_length(step, 5)?; + dbg!(call_data); + dbg!(return_data); (call_data.0, call_data.1, return_data.0, return_data.1) } CallKind::DelegateCall | CallKind::StaticCall => { @@ -867,16 +873,24 @@ impl<'a> CircuitInputStateRef<'a> { self.call_context_read(exec_step, caller.call_id, field, value); } - let [last_callee_return_data_offset, last_callee_return_data_length] = match geth_step.op { - OpcodeId::STOP => [Word::zero(); 2], - OpcodeId::REVERT | OpcodeId::RETURN => { - // TODO: move this back into the return bus mapping. - let offset = geth_step.stack.nth_last(0)?; - let length = geth_step.stack.nth_last(1)?; - [offset, length] - } - _ => unreachable!(), - }; + let [mut last_callee_return_data_offset, last_callee_return_data_length] = + match geth_step.op { + OpcodeId::STOP => [Word::zero(); 2], + OpcodeId::REVERT | OpcodeId::RETURN => { + // TODO: move this back into the return bus mapping. + // todo, somehow these are reversed.... + let offset = geth_step.stack.nth_last(0)?; + let length = geth_step.stack.nth_last(1)?; + [offset, length] + } + _ => unreachable!(), + }; + + // This is the convention we are using for memory addresses so that we do not + // charge of memory expansion costs when the length is 0. + if last_callee_return_data_length.is_zero() { + last_callee_return_data_offset = Word::zero(); + } dbg!( last_callee_return_data_offset, diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index de529a181c..31ac69bfee 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -36,6 +36,8 @@ impl Opcode for Return { } let call = state.call()?.clone(); + // assert_eq!(offset, call.return_data_offset.into()); + // assert_eq!(length, call.return_data_length.into()); let is_root = call.is_root; for (field, value) in [ (CallContextField::IsRoot, is_root.to_word()), @@ -43,6 +45,7 @@ impl Opcode for Return { (CallContextField::IsSuccess, call.is_success.to_word()), (CallContextField::CallerId, call.caller_id.into()), ( + // these two are wrong and should be for the caller? CallContextField::ReturnDataOffset, call.return_data_offset.into(), ), @@ -51,9 +54,12 @@ impl Opcode for Return { call.return_data_length.into(), ), ] { + // should these be caller? state.call_context_read(&mut exec_step, call.call_id, field, value); } + dbg!(call.return_data_offset, call.return_data_length); + if !is_root { state.handle_restore_context(steps, &mut exec_step)?; } @@ -75,7 +81,7 @@ impl Opcode for Return { }, )?; } - } else if !is_root { + } else if !is_root && length > 0 { let caller_ctx = state.caller_ctx_mut()?; let return_offset = call.return_data_offset.try_into().unwrap(); @@ -86,6 +92,9 @@ impl Opcode for Return { caller_ctx.return_data[0..copy_len] .copy_from_slice(&memory.0[offset..offset + copy_len]); + // how are these being set????? + dbg!(copy_len, call.return_data_offset, call.return_data_length); + if length > 0 { handle_copy( state, diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index c9303289d9..f96e062fb8 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -14,7 +14,7 @@ use crate::{ util::Expr, }; use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; -use eth_types::{Field, ToScalar}; +use eth_types::{Field, ToScalar, Word}; use halo2_proofs::{circuit::Value, plonk::Error}; #[derive(Clone, Debug)] @@ -47,7 +47,7 @@ impl ExecutionGadget for ReturnGadget { let offset = cb.query_cell(); let length = cb.query_rlc(); - cb.stack_pop(offset.expr()); + cb.stack_pop(offset.expr()); // how is this passing????? cb.stack_pop(length.expr()); let range = MemoryAddressGadget::construct(cb, offset, length); @@ -87,34 +87,34 @@ impl ExecutionGadget for ReturnGadget { ) }); - cb.condition( - not::expr(is_create.expr()) * not::expr(is_root.expr()) * range.has_length(), - |cb| { - let source_id = cb.curr.state.call_id.expr(); - let source_tag = CopyDataType::Memory.expr(); - let destination_id = caller_id.expr(); - let destination_tag = CopyDataType::Memory.expr(); - let source_address_start = range.offset(); - let source_address_end = range.offset() + copy_length.clone(); - let destination_address_start = return_data_offset.expr(); - let rlc_acc = 0.expr(); - let rw_counter_start = - cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(); - cb.copy_table_lookup( - source_id, - source_tag, - destination_id, - destination_tag, - source_address_start, - source_address_end, - destination_address_start, - copy_length.clone(), - rlc_acc, - rw_counter_start, - copy_length.clone() + copy_length, - ); - }, - ); + // cb.condition( + // not::expr(is_create.expr()) * not::expr(is_root.expr()) * + // range.has_length(), |cb| { + // let source_id = cb.curr.state.call_id.expr(); + // let source_tag = CopyDataType::Memory.expr(); + // let destination_id = caller_id.expr(); + // let destination_tag = CopyDataType::Memory.expr(); + // let source_address_start = range.offset(); + // let source_address_end = range.offset() + copy_length.clone(); + // let destination_address_start = return_data_offset.expr(); + // let rlc_acc = 0.expr(); + // let rw_counter_start = + // cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(); + // cb.copy_table_lookup( + // source_id, + // source_tag, + // destination_id, + // destination_tag, + // source_address_start, + // source_address_end, + // destination_address_start, + // copy_length.clone(), + // rlc_acc, + // rw_counter_start, + // copy_length.clone() + copy_length, + // ); + // }, + // ); cb.condition( is_create.expr() * is_success.expr() * range.has_length(), @@ -152,7 +152,15 @@ impl ExecutionGadget for ReturnGadget { Value::known(F::from(step.opcode.unwrap().as_u64())), )?; - let [memory_offset, length] = [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); + let [mut memory_offset, length] = + [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); + dbg!(memory_offset, length); + // if length.is_zero() { + // memory_offset = Word::zero(); + // } + + // there might be an issue if this is constructed with length = 0, but it does + // have a have_length, method, so that seems unlikely. self.range .assign(region, offset, memory_offset, length, block.randomness)?; @@ -196,13 +204,12 @@ impl ExecutionGadget for ReturnGadget { #[cfg(test)] mod test { - use crate::evm_circuit::{ - test::run_test_circuit, test_util::run_test_circuits, witness::block_convert, - }; + use crate::test_util::run_test_circuits; use eth_types::{ address, bytecode, evm_types::OpcodeId, geth_types::Account, Address, Bytecode, ToWord, Word, }; + use itertools::Itertools; use mock::TestContext; const CALLEE_ADDRESS: Address = Address::repeat_byte(0xff); @@ -215,8 +222,8 @@ mod test { PUSH32(memory_value) PUSH1(memory_address) MSTORE - PUSH32(offset) PUSH32(length) + PUSH32(offset) }; code.write_op(if is_return { OpcodeId::RETURN @@ -226,22 +233,19 @@ mod test { code } - fn caller_bytecode(call_return_data_length: u64, call_return_data_offset: u64) -> Bytecode { - let call_value = 0; - let call_gas = 4000; + fn caller_bytecode(return_data_offset: u64, return_data_length: u64) -> Bytecode { + dbg!(return_data_offset, return_data_length); bytecode! { - PUSH32(call_return_data_length) - PUSH32(call_return_data_offset) - PUSH32(0) - PUSH32(0) - PUSH32(call_value) + PUSH32(return_data_length) + PUSH32(return_data_offset) + PUSH32(0) // call data length + PUSH32(0) // call data offset + PUSH32(0) // value PUSH32(CALLEE_ADDRESS.to_word()) - PUSH32(call_gas) + PUSH32(4000) // gas CALL STOP } - // what happens is that it reverts immediately, without being able to - // enter the inner call at all.. } #[test] @@ -264,29 +268,33 @@ mod test { #[test] fn test_return_nonroot() { - let callee_offset = 10; - let callee_length = 10; - - let caller_offset = 20; - let caller_length = 20; - - for is_return in [true, false] { + let test_parameters = [ + ((0, 0), (0, 0)), + ((0, 10), (0, 10)), + ((0, 10), (0, 20)), + ((0, 20), (0, 10)), + ((0, 10), (20, 10)), + ((0, 10), (1000, 0)), + ((1000, 0), (0, 10)), + ((1000, 0), (1000, 0)), + ]; + for (((callee_offset, callee_length), (caller_offset, caller_length)), is_return) in + test_parameters.iter().cartesian_product(&[true, false]) + { let callee = Account { address: CALLEE_ADDRESS, - code: callee_bytecode(is_return, callee_offset, callee_length).into(), + code: callee_bytecode(*is_return, *callee_offset, *callee_length).into(), nonce: Word::one(), ..Default::default() }; - let call_argument_offset = 5; // these are flipped or something... - let call_argument_length = 5; // let caller = Account { address: CALLER_ADDRESS, - code: caller_bytecode(call_argument_offset, call_argument_length).into(), + code: caller_bytecode(*caller_offset, *caller_length).into(), nonce: Word::one(), ..Default::default() }; - let block = TestContext::<3, 1>::new( + let test_context = TestContext::<3, 1>::new( None, |accs| { accs[0] @@ -304,25 +312,19 @@ mod test { |block, _tx| block.number(0xcafeu64), ) .unwrap(); - let block_data = bus_mapping::mock::BlockData::new_from_geth_data(block.into()); - let mut builder = block_data.new_circuit_input_builder(); - builder - .handle_block(&block_data.eth_block, &block_data.geth_traces) - .unwrap(); assert_eq!( - run_test_circuit(block_convert(&builder.block, &builder.code_db)), + run_test_circuits(test_context, None), Ok(()), - "{}", - dbg!(is_return) + "(callee_offset, callee_length, caller_offset, caller_length, is_return) = {:?}", + ( + *callee_offset, + *callee_length, + *caller_offset, + *caller_length, + *is_return + ) ); } } - - // test_root_return - // test_nonroot_return - // test_memory_expansion.... - // - - // test_ } diff --git a/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs b/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs index d71f5d4b57..16f279060e 100644 --- a/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs @@ -101,6 +101,8 @@ impl MemoryAddressGadget { memory_length: U256, randomness: F, ) -> Result { + dbg!(memory_offset, memory_length); + let memory_offset_bytes = memory_offset.to_le_bytes(); let memory_length_bytes = memory_length.to_le_bytes(); let memory_length_is_zero = memory_length.is_zero(); @@ -109,6 +111,8 @@ impl MemoryAddressGadget { offset, Value::known(Word::random_linear_combine(memory_offset_bytes, randomness)), )?; + // this is so crazy... the offset rlc is the non-zero value, but the bytes are 0 + // if the length is 0.... self.memory_offset_bytes.assign( region, offset, @@ -118,7 +122,7 @@ impl MemoryAddressGadget { memory_offset_bytes[..N_BYTES_MEMORY_ADDRESS] .try_into() .unwrap() - }), + }), // this assigns 0 if the length is 0..... )?; self.memory_length.assign( region, From 3af27edd00c5f32c2c6dbbd0996ecd4dab5b9642 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 22:59:11 -0400 Subject: [PATCH 14/38] cleanup --- .../src/evm_circuit/execution/return.rs | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index f96e062fb8..b644724c49 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -14,7 +14,7 @@ use crate::{ util::Expr, }; use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; -use eth_types::{Field, ToScalar, Word}; +use eth_types::{Field, ToScalar}; use halo2_proofs::{circuit::Value, plonk::Error}; #[derive(Clone, Debug)] @@ -152,13 +152,7 @@ impl ExecutionGadget for ReturnGadget { Value::known(F::from(step.opcode.unwrap().as_u64())), )?; - let [mut memory_offset, length] = - [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); - dbg!(memory_offset, length); - // if length.is_zero() { - // memory_offset = Word::zero(); - // } - + let [memory_offset, length] = [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); // there might be an issue if this is constructed with length = 0, but it does // have a have_length, method, so that seems unlikely. self.range @@ -250,18 +244,19 @@ mod test { #[test] fn test_root() { - let offset = 0; - let length = 10; - for is_return in [true, false] { - let code = callee_bytecode(is_return, offset, length); + let test_parameters = [(0, 0), (0, 10), (300, 20), (1000, 0)]; + for ((offset, length), is_return) in + test_parameters.iter().cartesian_product(&[true, false]) + { + let code = callee_bytecode(*is_return, *offset, *length); assert_eq!( run_test_circuits( TestContext::<2, 1>::simple_ctx_with_bytecode(code).unwrap(), None ), Ok(()), - "{}", - dbg!(is_return) + "(offset, length, is_return) = {:?}", + (*offset, *length, *is_return) ); } } From f7a3ca8294e42782b2ae5ab2588e81dba67de28b Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 23:28:12 -0400 Subject: [PATCH 15/38] check for 0 copy length --- .../src/evm_circuit/execution/return.rs | 78 +++++++++++-------- 1 file changed, 47 insertions(+), 31 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index b644724c49..9103abc14b 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -5,8 +5,10 @@ use crate::{ param::N_BYTES_MEMORY_ADDRESS, step::ExecutionState, util::{ - common_gadget::RestoreContextGadget, constraint_builder::ConstraintBuilder, - math_gadget::MinMaxGadget, not, CachedRegion, Cell, + common_gadget::RestoreContextGadget, + constraint_builder::ConstraintBuilder, + math_gadget::{IsZeroGadget, MinMaxGadget}, + not, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -29,6 +31,7 @@ pub(crate) struct ReturnGadget { restore_context: RestoreContextGadget, nonroot_copy_length: MinMaxGadget, + nonroot_copy_length_is_zero: IsZeroGadget, caller_id: Cell, // can you get this out of restore_context? return_data_offset: Cell, @@ -74,6 +77,9 @@ impl ExecutionGadget for ReturnGadget { let nonroot_copy_length = MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); + let nonroot_copy_length_is_zero = IsZeroGadget::construct(cb, nonroot_copy_length.min()); + // TODO: check if this is needed. I think return_data_length is 0 for the root + // call, so this would not be needed. let copy_length = not::expr(is_root.expr()) * nonroot_copy_length.min(); let restore_context = cb.condition(not::expr(is_root.expr()), |cb| { @@ -87,34 +93,36 @@ impl ExecutionGadget for ReturnGadget { ) }); - // cb.condition( - // not::expr(is_create.expr()) * not::expr(is_root.expr()) * - // range.has_length(), |cb| { - // let source_id = cb.curr.state.call_id.expr(); - // let source_tag = CopyDataType::Memory.expr(); - // let destination_id = caller_id.expr(); - // let destination_tag = CopyDataType::Memory.expr(); - // let source_address_start = range.offset(); - // let source_address_end = range.offset() + copy_length.clone(); - // let destination_address_start = return_data_offset.expr(); - // let rlc_acc = 0.expr(); - // let rw_counter_start = - // cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(); - // cb.copy_table_lookup( - // source_id, - // source_tag, - // destination_id, - // destination_tag, - // source_address_start, - // source_address_end, - // destination_address_start, - // copy_length.clone(), - // rlc_acc, - // rw_counter_start, - // copy_length.clone() + copy_length, - // ); - // }, - // ); + cb.condition( + not::expr(is_create.expr()) + * not::expr(is_root.expr()) + * not::expr(nonroot_copy_length_is_zero.expr()), + |cb| { + let source_id = cb.curr.state.call_id.expr(); + let source_tag = CopyDataType::Memory.expr(); + let destination_id = caller_id.expr(); + let destination_tag = CopyDataType::Memory.expr(); + let source_address_start = range.offset(); // this is wrong.... + let source_address_end = range.offset() + copy_length.clone(); // this is also wrong.... + let destination_address_start = return_data_offset.expr(); + let rlc_acc = 0.expr(); + let rw_counter_start = + cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(); + cb.copy_table_lookup( + source_id, + source_tag, + destination_id, + destination_tag, + source_address_start, + source_address_end, + destination_address_start, + copy_length.clone(), + rlc_acc, + rw_counter_start, + copy_length.clone() + copy_length, + ); + }, + ); cb.condition( is_create.expr() * is_success.expr() * range.has_length(), @@ -131,6 +139,7 @@ impl ExecutionGadget for ReturnGadget { is_success, caller_id, nonroot_copy_length, + nonroot_copy_length_is_zero, return_data_offset, return_data_length, restore_context, @@ -188,6 +197,12 @@ impl ExecutionGadget for ReturnGadget { F::from(call.return_data_length), F::from(length.as_u64()), )?; + self.nonroot_copy_length_is_zero.assign( + region, + offset, + F::from(std::cmp::min(call.return_data_length, length.as_u64())), + )?; + let opcode = step.opcode.unwrap(); self.opcode .assign(region, offset, Value::known(F::from(opcode.as_u64())))?; @@ -243,7 +258,7 @@ mod test { } #[test] - fn test_root() { + fn test_return_root() { let test_parameters = [(0, 0), (0, 10), (300, 20), (1000, 0)]; for ((offset, length), is_return) in test_parameters.iter().cartesian_product(&[true, false]) @@ -261,6 +276,7 @@ mod test { } } + // (0, 10, 1000, 0, true) #[test] fn test_return_nonroot() { let test_parameters = [ From 267047a65c4d87b2a646b079823246d7bdfbe324 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 23:43:04 -0400 Subject: [PATCH 16/38] Remove nonroot from name --- .../src/evm_circuit/execution/return.rs | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 9103abc14b..91d4bd7b74 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -30,8 +30,8 @@ pub(crate) struct ReturnGadget { is_success: Cell, restore_context: RestoreContextGadget, - nonroot_copy_length: MinMaxGadget, - nonroot_copy_length_is_zero: IsZeroGadget, + copy_length: MinMaxGadget, + copy_length_is_zero: IsZeroGadget, caller_id: Cell, // can you get this out of restore_context? return_data_offset: Cell, @@ -75,19 +75,17 @@ impl ExecutionGadget for ReturnGadget { cb.require_next_state(ExecutionState::EndTx); }); - let nonroot_copy_length = + let copy_length = MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); - let nonroot_copy_length_is_zero = IsZeroGadget::construct(cb, nonroot_copy_length.min()); - // TODO: check if this is needed. I think return_data_length is 0 for the root - // call, so this would not be needed. - let copy_length = not::expr(is_root.expr()) * nonroot_copy_length.min(); + let copy_length_is_zero = IsZeroGadget::construct(cb, copy_length.min()); + let copy_rw_increase = copy_length.min() + copy_length.min(); let restore_context = cb.condition(not::expr(is_root.expr()), |cb| { cb.require_next_state_not(ExecutionState::EndTx); RestoreContextGadget::construct( cb, is_success.expr(), - copy_length.clone() + copy_length.clone(), + copy_rw_increase.clone(), range.offset(), range.length(), ) @@ -96,14 +94,14 @@ impl ExecutionGadget for ReturnGadget { cb.condition( not::expr(is_create.expr()) * not::expr(is_root.expr()) - * not::expr(nonroot_copy_length_is_zero.expr()), + * not::expr(copy_length_is_zero.expr()), |cb| { let source_id = cb.curr.state.call_id.expr(); let source_tag = CopyDataType::Memory.expr(); let destination_id = caller_id.expr(); let destination_tag = CopyDataType::Memory.expr(); - let source_address_start = range.offset(); // this is wrong.... - let source_address_end = range.offset() + copy_length.clone(); // this is also wrong.... + let source_address_start = range.offset(); + let source_address_end = range.offset() + copy_length.min(); let destination_address_start = return_data_offset.expr(); let rlc_acc = 0.expr(); let rw_counter_start = @@ -116,10 +114,10 @@ impl ExecutionGadget for ReturnGadget { source_address_start, source_address_end, destination_address_start, - copy_length.clone(), + copy_length.min(), rlc_acc, rw_counter_start, - copy_length.clone() + copy_length, + copy_rw_increase, ); }, ); @@ -138,8 +136,8 @@ impl ExecutionGadget for ReturnGadget { is_create, is_success, caller_id, - nonroot_copy_length, - nonroot_copy_length_is_zero, + copy_length, + copy_length_is_zero, return_data_offset, return_data_length, restore_context, @@ -191,13 +189,13 @@ impl ExecutionGadget for ReturnGadget { .assign(region, offset, block, call, step, 8)?; } - self.nonroot_copy_length.assign( + self.copy_length.assign( region, offset, F::from(call.return_data_length), F::from(length.as_u64()), )?; - self.nonroot_copy_length_is_zero.assign( + self.copy_length_is_zero.assign( region, offset, F::from(std::cmp::min(call.return_data_length, length.as_u64())), From 4b31b4b7ef2e0817f590bd5aac462f70493bf961 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 23:48:43 -0400 Subject: [PATCH 17/38] Update MemoryAddressGadget comments --- zkevm-circuits/src/evm_circuit/execution/return.rs | 3 +-- zkevm-circuits/src/evm_circuit/util/memory_gadget.rs | 12 +++++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 91d4bd7b74..10ab3ead62 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -75,8 +75,7 @@ impl ExecutionGadget for ReturnGadget { cb.require_next_state(ExecutionState::EndTx); }); - let copy_length = - MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); + let copy_length = MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); let copy_length_is_zero = IsZeroGadget::construct(cb, copy_length.min()); let copy_rw_increase = copy_length.min() + copy_length.min(); diff --git a/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs b/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs index 16f279060e..3b777367ab 100644 --- a/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/memory_gadget.rs @@ -57,8 +57,10 @@ pub(crate) mod address_high { } } -/// Convert the dynamic memory offset and length from random linear combiation -/// to integer. It handles the "no expansion" feature when length is zero. +/// Convert the dynamic memory offset and length from random linear combination +/// to integer. It handles the "no expansion" feature by setting the +/// `memory_offset_bytes` to zero when `memory_length` is zero. In this case, +/// the RLC value for `memory_offset` need not match the bytes. #[derive(Clone, Debug)] pub(crate) struct MemoryAddressGadget { memory_offset: Cell, @@ -101,8 +103,6 @@ impl MemoryAddressGadget { memory_length: U256, randomness: F, ) -> Result { - dbg!(memory_offset, memory_length); - let memory_offset_bytes = memory_offset.to_le_bytes(); let memory_length_bytes = memory_length.to_le_bytes(); let memory_length_is_zero = memory_length.is_zero(); @@ -111,8 +111,6 @@ impl MemoryAddressGadget { offset, Value::known(Word::random_linear_combine(memory_offset_bytes, randomness)), )?; - // this is so crazy... the offset rlc is the non-zero value, but the bytes are 0 - // if the length is 0.... self.memory_offset_bytes.assign( region, offset, @@ -122,7 +120,7 @@ impl MemoryAddressGadget { memory_offset_bytes[..N_BYTES_MEMORY_ADDRESS] .try_into() .unwrap() - }), // this assigns 0 if the length is 0..... + }), )?; self.memory_length.assign( region, From 6d3caa31b98550501dabd6d9a1f4a55d556f0d94 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 23:50:38 -0400 Subject: [PATCH 18/38] cleanup --- bus-mapping/src/circuit_input_builder/input_state_ref.rs | 8 +------- 1 file changed, 1 insertion(+), 7 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 6d184e8ed4..33874e3a42 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -96,11 +96,7 @@ impl<'a> CircuitInputStateRef<'a> { /// reference to the stored operation ([`OperationRef`]) inside the /// bus-mapping instance of the current [`ExecStep`]. Then increase the /// block_ctx [`RWCounter`](crate::operation::RWCounter) by one. - pub fn push_op(&mut self, step: &mut ExecStep, rw: RW, op: T) { - if self.block_ctx.rwc == 131.into() { - dbg!(op.clone()); - // panic!(); - } + pub fn push_op(&mut self, step: &mut ExecStep, rw: RW, op: T) { let op_ref = self.block .container @@ -632,8 +628,6 @@ impl<'a> CircuitInputStateRef<'a> { CallKind::Call | CallKind::CallCode => { let call_data = get_call_memory_offset_length(step, 3)?; let return_data = get_call_memory_offset_length(step, 5)?; - dbg!(call_data); - dbg!(return_data); (call_data.0, call_data.1, return_data.0, return_data.1) } CallKind::DelegateCall | CallKind::StaticCall => { From 5314a175fd056e269b558c74238ae09f54224b59 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 21 Sep 2022 23:56:51 -0400 Subject: [PATCH 19/38] cleanup --- .../circuit_input_builder/input_state_ref.rs | 39 ++++++++----------- 1 file changed, 17 insertions(+), 22 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 33874e3a42..b36f2f9488 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -824,7 +824,9 @@ impl<'a> CircuitInputStateRef<'a> { Ok(()) } - ///asdfasdfasdfa + /// Bus mapping for the RestoreContextGadget as used in RETURN. + // TODO: unify this with restore context bus mapping for STOP. + // TODO: unify this with the `handle return function above.` pub fn handle_restore_context( &mut self, steps: &[GethExecStep], @@ -867,29 +869,22 @@ impl<'a> CircuitInputStateRef<'a> { self.call_context_read(exec_step, caller.call_id, field, value); } - let [mut last_callee_return_data_offset, last_callee_return_data_length] = - match geth_step.op { - OpcodeId::STOP => [Word::zero(); 2], - OpcodeId::REVERT | OpcodeId::RETURN => { - // TODO: move this back into the return bus mapping. - // todo, somehow these are reversed.... - let offset = geth_step.stack.nth_last(0)?; - let length = geth_step.stack.nth_last(1)?; + let [last_callee_return_data_offset, last_callee_return_data_length] = match geth_step.op { + OpcodeId::STOP => [Word::zero(); 2], + OpcodeId::REVERT | OpcodeId::RETURN => { + // TODO: move this back into the return bus mapping. + let offset = geth_step.stack.nth_last(0)?; + let length = geth_step.stack.nth_last(1)?; + // This is the convention we are using for memory addresses so that there is no + // memory expansion cost when the length is 0. + if length.is_zero() { + [Word::zero(); 2] + } else { [offset, length] } - _ => unreachable!(), - }; - - // This is the convention we are using for memory addresses so that we do not - // charge of memory expansion costs when the length is 0. - if last_callee_return_data_length.is_zero() { - last_callee_return_data_offset = Word::zero(); - } - - dbg!( - last_callee_return_data_offset, - last_callee_return_data_length - ); + } + _ => unreachable!(), + }; for (field, value) in [ (CallContextField::LastCalleeId, call.call_id.into()), ( From 4837ea7d784e66f84f2915d5eec9209cf0392968 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Thu, 22 Sep 2022 00:07:11 -0400 Subject: [PATCH 20/38] Update todo --- zkevm-circuits/src/evm_circuit/util/common_gadget.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index d4df256c81..94535f12a2 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -143,7 +143,7 @@ impl RestoreContextGadget { } else { caller_gas_left.expr() + cb.curr.state.gas_left.expr() }; - // TODO: check that memory expansion cost for return/revert is accounted for. + // TODO: Do we need to constrain the gas cost of the return here? // Accumulate reversible_write_counter in case this call stack reverts in the // future even it itself succeeds. Note that when sub-call halts in From a96e3eb5203b02540710a7fe75494315d98d9d16 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Thu, 22 Sep 2022 00:10:00 -0400 Subject: [PATCH 21/38] cleanup --- zkevm-circuits/src/evm_circuit/execution/return.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 10ab3ead62..36bc6c04a3 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -50,7 +50,7 @@ impl ExecutionGadget for ReturnGadget { let offset = cb.query_cell(); let length = cb.query_rlc(); - cb.stack_pop(offset.expr()); // how is this passing????? + cb.stack_pop(offset.expr()); cb.stack_pop(length.expr()); let range = MemoryAddressGadget::construct(cb, offset, length); @@ -70,7 +70,6 @@ impl ExecutionGadget for ReturnGadget { is_success.expr() * OpcodeId::RETURN.expr() + not::expr(is_success.expr()) * OpcodeId::REVERT.expr(), ); - cb.condition(is_root.expr(), |cb| { cb.require_next_state(ExecutionState::EndTx); }); @@ -159,8 +158,6 @@ impl ExecutionGadget for ReturnGadget { )?; let [memory_offset, length] = [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); - // there might be an issue if this is constructed with length = 0, but it does - // have a have_length, method, so that seems unlikely. self.range .assign(region, offset, memory_offset, length, block.randomness)?; @@ -273,7 +270,6 @@ mod test { } } - // (0, 10, 1000, 0, true) #[test] fn test_return_nonroot() { let test_parameters = [ @@ -281,7 +277,6 @@ mod test { ((0, 10), (0, 10)), ((0, 10), (0, 20)), ((0, 20), (0, 10)), - ((0, 10), (20, 10)), ((0, 10), (1000, 0)), ((1000, 0), (0, 10)), ((1000, 0), (1000, 0)), From 2a4e4ec4f3f09e15a78ab9e94d8e4cfc8c1c382c Mon Sep 17 00:00:00 2001 From: z2trillion Date: Thu, 22 Sep 2022 00:15:03 -0400 Subject: [PATCH 22/38] Cleanup bus mapping --- bus-mapping/src/evm/opcodes/return.rs | 134 +++----------------------- eth-types/src/bytecode.rs | 2 +- 2 files changed, 16 insertions(+), 120 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 31ac69bfee..a000e8d3a3 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -13,7 +13,7 @@ use ethers_core::utils::keccak256; #[derive(Debug, Copy, Clone)] pub(crate) struct Return; -// rename to ReturnRevertStop? +// TODO: rename to indicate this handles REVERT (and maybe STOP)? impl Opcode for Return { fn gen_associated_ops( state: &mut CircuitInputStateRef, @@ -32,12 +32,9 @@ impl Opcode for Return { .call_ctx_mut()? .memory .extend_at_least((offset.low_u64() + length.low_u64()).try_into().unwrap()); - // TODO: handle memory expansion gas cost? } let call = state.call()?.clone(); - // assert_eq!(offset, call.return_data_offset.into()); - // assert_eq!(length, call.return_data_length.into()); let is_root = call.is_root; for (field, value) in [ (CallContextField::IsRoot, is_root.to_word()), @@ -45,7 +42,6 @@ impl Opcode for Return { (CallContextField::IsSuccess, call.is_success.to_word()), (CallContextField::CallerId, call.caller_id.into()), ( - // these two are wrong and should be for the caller? CallContextField::ReturnDataOffset, call.return_data_offset.into(), ), @@ -54,12 +50,9 @@ impl Opcode for Return { call.return_data_length.into(), ), ] { - // should these be caller? state.call_context_read(&mut exec_step, call.call_id, field, value); } - dbg!(call.return_data_offset, call.return_data_length); - if !is_root { state.handle_restore_context(steps, &mut exec_step)?; } @@ -92,25 +85,20 @@ impl Opcode for Return { caller_ctx.return_data[0..copy_len] .copy_from_slice(&memory.0[offset..offset + copy_len]); - // how are these being set????? - dbg!(copy_len, call.return_data_offset, call.return_data_length); - - if length > 0 { - handle_copy( - state, - &mut exec_step, - Source { - id: call.call_id, - offset, - length, - }, - Destination { - id: call.caller_id, - offset: call.return_data_offset.try_into().unwrap(), - length: call.return_data_length.try_into().unwrap(), - }, - )?; - } + handle_copy( + state, + &mut exec_step, + Source { + id: call.call_id, + offset, + length, + }, + Destination { + id: call.caller_id, + offset: call.return_data_offset.try_into().unwrap(), + length: call.return_data_length.try_into().unwrap(), + }, + )?; } state.handle_return(step)?; @@ -209,95 +197,3 @@ fn handle_create( Ok(()) } - -#[cfg(test)] -mod return_tests { - use crate::mock::BlockData; - use eth_types::evm_types::OpcodeId; - use eth_types::geth_types::GethData; - use eth_types::{bytecode, Word}; - use mock::test_ctx::helpers::{account_0_code_account_1_no_code, tx_from_1_to_0}; - use mock::TestContext; - - fn test_return( - is_return: bool, - callee_return_data_offset: usize, - callee_return_data_length: usize, - caller_return_data_offset: usize, - caller_return_data_length: usize, - ) { - let mut contract = bytecode! { - PUSH1(0x20) - PUSH1(0) - PUSH1(0) - CALLDATACOPY - PUSH1(callee_return_data_length) - PUSH1(callee_return_data_offset) - }; - contract.write_op(if is_return { - OpcodeId::RETURN - } else { - OpcodeId::REVERT - }); - - let constructor = bytecode! { - PUSH12(Word::from(contract.to_vec().as_slice())) - PUSH1(0) - MSTORE - PUSH1(0xC) - PUSH1(0x14) - RETURN - }; - - let code = bytecode! { - PUSH21(Word::from(constructor.to_vec().as_slice())) - PUSH1(0) - MSTORE - - PUSH1 (0x15) - PUSH1 (0xB) - PUSH1 (0) - CREATE - - PUSH1 (caller_return_data_length) - PUSH1 (caller_return_data_offset) - PUSH1 (0x20) - PUSH1 (0) - PUSH1 (0) - DUP6 - PUSH2 (0xFFFF) - CALL - - STOP - }; - // Get the execution steps from the external tracer - let block: GethData = TestContext::<2, 1>::new( - None, - account_0_code_account_1_no_code(code), - tx_from_1_to_0, - |block, _tx| block.number(0xcafeu64), - ) - .unwrap() - .into(); - - let mut builder = BlockData::new_from_geth_data(block.clone()).new_circuit_input_builder(); - builder - .handle_block(&block.eth_block, &block.geth_traces) - .unwrap(); - } - - #[test] - fn test_cases() { - test_return(true, 0, 0, 0, 0); - test_return(true, 10, 10, 10, 10); - test_return(true, 0, 0, 0, 0); - test_return(true, 0, 0, 0, 100); - test_return(true, 0, 0, 0, 0); - - test_return(false, 0, 0, 0, 0); - test_return(false, 10, 10, 10, 10); - test_return(false, 0, 0, 0, 0); - test_return(false, 0, 0, 0, 100); - test_return(false, 0, 0, 0, 0); - } -} diff --git a/eth-types/src/bytecode.rs b/eth-types/src/bytecode.rs index fe0300dbb2..6a8ca1eafe 100644 --- a/eth-types/src/bytecode.rs +++ b/eth-types/src/bytecode.rs @@ -22,7 +22,7 @@ pub struct BytecodeElement { /// EVM Bytecode #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct Bytecode { - /// asdfasdf + /// Vector for bytecode elements. pub code: Vec, num_opcodes: usize, markers: HashMap, From a90e5e5a5e489d79a5d6ccef50d6233d5df57610 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Thu, 22 Sep 2022 00:33:44 -0400 Subject: [PATCH 23/38] Don't make a copy event if nothing is copied --- bus-mapping/src/evm/opcodes/return.rs | 46 ++++++++++++++------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index a000e8d3a3..21e5494a82 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -57,6 +57,7 @@ impl Opcode for Return { state.handle_restore_context(steps, &mut exec_step)?; } + // can we use ref here? let memory = state.call_ctx()?.memory.clone(); let offset = offset.as_usize(); let length = length.as_usize(); @@ -77,28 +78,29 @@ impl Opcode for Return { } else if !is_root && length > 0 { let caller_ctx = state.caller_ctx_mut()?; let return_offset = call.return_data_offset.try_into().unwrap(); - - let copy_len = std::cmp::min(call.return_data_length.try_into().unwrap(), length); - caller_ctx.memory.0[return_offset..return_offset + copy_len] - .copy_from_slice(&memory.0[offset..offset + copy_len]); - caller_ctx.return_data.resize(length, 0); - caller_ctx.return_data[0..copy_len] - .copy_from_slice(&memory.0[offset..offset + copy_len]); - - handle_copy( - state, - &mut exec_step, - Source { - id: call.call_id, - offset, - length, - }, - Destination { - id: call.caller_id, - offset: call.return_data_offset.try_into().unwrap(), - length: call.return_data_length.try_into().unwrap(), - }, - )?; + let copy_length = std::cmp::min(call.return_data_length.try_into().unwrap(), length); + if copy_length > 0 { + caller_ctx.memory.0[return_offset..return_offset + copy_length] + .copy_from_slice(&memory.0[offset..offset + copy_length]); + caller_ctx.return_data.resize(length, 0); + caller_ctx.return_data[0..copy_length] + .copy_from_slice(&memory.0[offset..offset + copy_length]); + + handle_copy( + state, + &mut exec_step, + Source { + id: call.call_id, + offset, + length, + }, + Destination { + id: call.caller_id, + offset: call.return_data_offset.try_into().unwrap(), + length: call.return_data_length.try_into().unwrap(), + }, + )?; + } } state.handle_return(step)?; From 0a78b0b95fca6916ee4fb47483106e7a943b6be7 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 27 Sep 2022 13:02:53 -0400 Subject: [PATCH 24/38] Update to match specs --- bus-mapping/src/evm/opcodes/return.rs | 89 ++++---- .../src/evm_circuit/execution/return.rs | 191 ++++++++++++------ .../src/evm_circuit/util/common_gadget.rs | 5 +- .../evm_circuit/util/constraint_builder.rs | 2 + 4 files changed, 178 insertions(+), 109 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 21e5494a82..9ef21fb0ad 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -35,56 +35,65 @@ impl Opcode for Return { } let call = state.call()?.clone(); - let is_root = call.is_root; for (field, value) in [ - (CallContextField::IsRoot, is_root.to_word()), - (CallContextField::IsCreate, call.is_create().to_word()), - (CallContextField::IsSuccess, call.is_success.to_word()), - (CallContextField::CallerId, call.caller_id.into()), - ( - CallContextField::ReturnDataOffset, - call.return_data_offset.into(), - ), - ( - CallContextField::ReturnDataLength, - call.return_data_length.into(), - ), + (CallContextField::IsRoot, call.is_root), + (CallContextField::IsCreate, call.is_create()), + (CallContextField::IsSuccess, call.is_success), ] { - state.call_context_read(&mut exec_step, call.call_id, field, value); + state.call_context_read(&mut exec_step, call.call_id, field, value.to_word()); } - if !is_root { - state.handle_restore_context(steps, &mut exec_step)?; - } - - // can we use ref here? - let memory = state.call_ctx()?.memory.clone(); let offset = offset.as_usize(); let length = length.as_usize(); - if call.is_create() && call.is_success { + + // Case A in the spec. + if call.is_create() && call.is_success && length > 0 { // Note: handle_return updates state.code_db. All we need to do here is push the // copy event. - if length > 0 { - handle_create( - state, - &mut exec_step, - Source { - id: call.call_id, - offset, - length, - }, - )?; + handle_create( + state, + &mut exec_step, + Source { + id: call.call_id, + offset, + length, + }, + )?; + } + + // No additional bus mapping is needed for case B. + + // Case C in the specs. + if !call.is_root { + state.handle_restore_context(steps, &mut exec_step)?; + } + + // Case D in the specs. + if !call.is_root && !call.is_create() { + for (field, value) in [ + ( + CallContextField::CallerId, + u64::try_from(call.caller_id).unwrap(), + ), + (CallContextField::ReturnDataOffset, call.return_data_offset), + (CallContextField::ReturnDataLength, call.return_data_length), + ] { + state.call_context_read(&mut exec_step, call.call_id, field, value.into()); } - } else if !is_root && length > 0 { - let caller_ctx = state.caller_ctx_mut()?; - let return_offset = call.return_data_offset.try_into().unwrap(); - let copy_length = std::cmp::min(call.return_data_length.try_into().unwrap(), length); + + let return_data_length = usize::try_from(call.return_data_length).unwrap(); + let copy_length = std::cmp::min(return_data_length, length); if copy_length > 0 { + // reconstruction + let callee_memory = state.call_ctx()?.memory.clone(); + let caller_ctx = state.caller_ctx_mut()?; + let return_offset = call.return_data_offset.try_into().unwrap(); + caller_ctx.memory.0[return_offset..return_offset + copy_length] - .copy_from_slice(&memory.0[offset..offset + copy_length]); + .copy_from_slice(&callee_memory.0[offset..offset + copy_length]); caller_ctx.return_data.resize(length, 0); caller_ctx.return_data[0..copy_length] - .copy_from_slice(&memory.0[offset..offset + copy_length]); + .copy_from_slice(&callee_memory.0[offset..offset + copy_length]); handle_copy( state, @@ -96,8 +105,8 @@ impl Opcode for Return { }, Destination { id: call.caller_id, - offset: call.return_data_offset.try_into().unwrap(), - length: call.return_data_length.try_into().unwrap(), + offset: return_offset, + length: return_data_length, }, )?; } @@ -151,7 +160,7 @@ fn handle_copy( src_type: CopyDataType::Memory, src_id: NumberOrHash::Number(source.id), src_addr: source.offset.try_into().unwrap(), - src_addr_end: (source.offset + copy_length).try_into().unwrap(), + src_addr_end: (source.offset + source.length).try_into().unwrap(), dst_type: CopyDataType::Memory, dst_id: NumberOrHash::Number(destination.id), dst_addr: destination.offset.try_into().unwrap(), diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 36bc6c04a3..45e986f4a8 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -31,7 +31,8 @@ pub(crate) struct ReturnGadget { restore_context: RestoreContextGadget, copy_length: MinMaxGadget, - copy_length_is_zero: IsZeroGadget, + copy_rw_increase: Cell, + copy_rw_increase_is_zero: IsZeroGadget, caller_id: Cell, // can you get this out of restore_context? return_data_offset: Cell, @@ -54,13 +55,10 @@ impl ExecutionGadget for ReturnGadget { cb.stack_pop(length.expr()); let range = MemoryAddressGadget::construct(cb, offset, length); - let [is_root, is_create, is_success, caller_id, return_data_offset, return_data_length] = [ + let [is_root, is_create, is_success] = [ CallContextFieldTag::IsRoot, CallContextFieldTag::IsCreate, CallContextFieldTag::IsSuccess, - CallContextFieldTag::CallerId, - CallContextFieldTag::ReturnDataOffset, - CallContextFieldTag::ReturnDataLength, ] .map(|field_tag| cb.call_context(None, field_tag)); @@ -70,62 +68,103 @@ impl ExecutionGadget for ReturnGadget { is_success.expr() * OpcodeId::RETURN.expr() + not::expr(is_success.expr()) * OpcodeId::REVERT.expr(), ); + + // There are 4 cases non-mutually exclusive, A to D, to handle, depending on if + // the call is, or is not, a create, root, or successful. See the specs at + // https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/specs/opcode/F3RETURN.md + // for more details. + + // These are globally defined because they are used across multiple cases. + let copy_rw_increase = cb.query_cell(); + let copy_rw_increase_is_zero = IsZeroGadget::construct(cb, copy_rw_increase.expr()); + + // Case A in the specs. + cb.condition(is_create.expr() * is_success.expr(), |cb| { + cb.require_equal( + "increase rw counter once for each memory to bytecode byte copied", + copy_rw_increase.expr(), + range.length(), + ); + }); + cb.condition( + is_create.expr() * is_success.expr() * not::expr(copy_rw_increase_is_zero.expr()), + |_cb| { + // TODO: copy_table_lookup for contract creation. + }, + ); + + // Case B in the specs. cb.condition(is_root.expr(), |cb| { cb.require_next_state(ExecutionState::EndTx); }); - let copy_length = MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); - let copy_length_is_zero = IsZeroGadget::construct(cb, copy_length.min()); - let copy_rw_increase = copy_length.min() + copy_length.min(); - + // Case C in the specs. + // TODO: have copy_table_lookup update rw_counter expression so that this can go + // at the end of the constraints. let restore_context = cb.condition(not::expr(is_root.expr()), |cb| { - cb.require_next_state_not(ExecutionState::EndTx); RestoreContextGadget::construct( cb, is_success.expr(), - copy_rw_increase.clone(), + 3.expr() + copy_rw_increase.expr(), range.offset(), range.length(), ) }); + // Case D in the specs. + let (caller_id, return_data_offset, return_data_length, copy_length) = cb.condition( + not::expr(is_create.expr()) * not::expr(is_root.expr()), + |cb| { + let [caller_id, return_data_offset, return_data_length] = [ + CallContextFieldTag::CallerId, + CallContextFieldTag::ReturnDataOffset, + CallContextFieldTag::ReturnDataLength, + ] + .map(|field_tag| cb.call_context(None, field_tag)); + let copy_length = + MinMaxGadget::construct(cb, return_data_length.expr(), range.length()); + cb.require_equal( + "increase rw counter twice for each memory to memory byte copied", + copy_length.min() + copy_length.min(), + copy_rw_increase.expr(), + ); + ( + caller_id, + return_data_offset, + return_data_length, + copy_length, + ) + }, + ); cb.condition( not::expr(is_create.expr()) * not::expr(is_root.expr()) - * not::expr(copy_length_is_zero.expr()), + * not::expr(copy_rw_increase_is_zero.expr()), |cb| { - let source_id = cb.curr.state.call_id.expr(); - let source_tag = CopyDataType::Memory.expr(); - let destination_id = caller_id.expr(); - let destination_tag = CopyDataType::Memory.expr(); - let source_address_start = range.offset(); - let source_address_end = range.offset() + copy_length.min(); - let destination_address_start = return_data_offset.expr(); - let rlc_acc = 0.expr(); - let rw_counter_start = - cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(); cb.copy_table_lookup( - source_id, - source_tag, - destination_id, - destination_tag, - source_address_start, - source_address_end, - destination_address_start, + cb.curr.state.call_id.expr(), + CopyDataType::Memory.expr(), + caller_id.expr(), + CopyDataType::Memory.expr(), + range.offset(), + range.address(), + return_data_offset.expr(), copy_length.min(), - rlc_acc, - rw_counter_start, - copy_rw_increase, + 0.expr(), + cb.curr.state.rw_counter.expr() + cb.rw_counter_offset().expr(), + copy_rw_increase.expr(), ); }, ); - cb.condition( - is_create.expr() * is_success.expr() * range.has_length(), - |_cb| { - // TODO: copy_table_lookup for contract creation - }, - ); + // Without this, copy_rw_increase would be unconstrained for non-create root + // calls. + cb.condition(not::expr(is_create.expr()) * is_root.expr(), |cb| { + cb.require_zero( + "rw counter is 0 if there is no copy event", + copy_rw_increase.expr(), + ); + }); Self { opcode, @@ -135,7 +174,8 @@ impl ExecutionGadget for ReturnGadget { is_success, caller_id, copy_length, - copy_length_is_zero, + copy_rw_increase, + copy_rw_increase_is_zero, return_data_offset, return_data_length, restore_context, @@ -169,38 +209,46 @@ impl ExecutionGadget for ReturnGadget { cell.assign(region, offset, Value::known(value.to_scalar().unwrap()))?; } - for (cell, value) in [ - ( - &self.caller_id, - F::from(u64::try_from(call.caller_id).unwrap()), - ), - (&self.return_data_length, call.return_data_length.into()), - (&self.return_data_offset, call.return_data_offset.into()), - ] { - cell.assign(region, offset, Value::known(value))?; + if !call.is_root && !call.is_create { + for (cell, value) in [ + ( + &self.caller_id, + F::from(u64::try_from(call.caller_id).unwrap()), + ), + (&self.return_data_length, call.return_data_length.into()), + (&self.return_data_offset, call.return_data_offset.into()), + ] { + cell.assign(region, offset, Value::known(value))?; + } + + self.copy_length.assign( + region, + offset, + F::from(call.return_data_length), + F::from(length.as_u64()), + )?; } + let copy_rw_increase = if call.is_create { + length.as_u64() + } else if !call.is_root { + dbg!(2 * std::cmp::min(call.return_data_length, length.as_u64())); + 2 * std::cmp::min(call.return_data_length, length.as_u64()) + } else { + 0 + }; + self.copy_rw_increase + .assign(region, offset, Value::known(F::from(copy_rw_increase)))?; + self.copy_rw_increase_is_zero + .assign(region, offset, F::from(copy_rw_increase))?; + if !call.is_root { - self.restore_context - .assign(region, offset, block, call, step, 8)?; + self.restore_context.assign( + region, offset, block, call, step, + 5, // 5 + usize::try_from(copy_rw_increase).unwrap(), + )?; } - self.copy_length.assign( - region, - offset, - F::from(call.return_data_length), - F::from(length.as_u64()), - )?; - self.copy_length_is_zero.assign( - region, - offset, - F::from(std::cmp::min(call.return_data_length, length.as_u64())), - )?; - - let opcode = step.opcode.unwrap(); - self.opcode - .assign(region, offset, Value::known(F::from(opcode.as_u64())))?; - Ok(()) } } @@ -237,7 +285,6 @@ mod test { } fn caller_bytecode(return_data_offset: u64, return_data_length: u64) -> Bytecode { - dbg!(return_data_offset, return_data_length); bytecode! { PUSH32(return_data_length) PUSH32(return_data_offset) @@ -284,6 +331,16 @@ mod test { for (((callee_offset, callee_length), (caller_offset, caller_length)), is_return) in test_parameters.iter().cartesian_product(&[true, false]) { + dbg!( + "(callee_offset, callee_length, caller_offset, caller_length, is_return) = {:?}", + ( + *callee_offset, + *callee_length, + *caller_offset, + *caller_length, + *is_return + ) + ); let callee = Account { address: CALLEE_ADDRESS, code: callee_bytecode(*is_return, *callee_offset, *callee_length).into(), diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 94535f12a2..6cca737809 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -99,13 +99,14 @@ pub(crate) struct RestoreContextGadget { impl RestoreContextGadget { pub(crate) fn construct( cb: &mut ConstraintBuilder, - is_success: Expression, - copy_lookup_rw_counter_increase: Expression, + is_success: Expression, // just remove this? + copy_lookup_rw_counter_increase: Expression, // try to set it up so this is always 0? return_data_offset: Expression, return_data_length: Expression, ) -> Self { // Read caller's context for restore let caller_id = cb.call_context(None, CallContextFieldTag::CallerId); + // TODO: lookup is_success here instead of taking in as argument. let [caller_is_root, caller_is_create, caller_code_hash, caller_program_counter, caller_stack_pointer, caller_gas_left, caller_memory_word_size, caller_reversible_write_counter] = [ CallContextFieldTag::IsRoot, diff --git a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs index 9ed5aa8c38..bba78b81d8 100644 --- a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs +++ b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs @@ -1114,6 +1114,8 @@ impl<'a, F: Field> ConstraintBuilder<'a, F> { rw_counter: Expression, rwc_inc: Expression, ) { + // TODO: eliminate rw_counter argument since we have self.rw_counter_offset + // already. TODO: increment rw_counter_offset by rwc_inc. self.add_lookup( "copy lookup", Lookup::CopyTable { From 7a3e7059ed60487b2de5c9de68639bcb19f4edde Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 28 Sep 2022 11:37:45 -0400 Subject: [PATCH 25/38] Use built in call id and cleanup --- bus-mapping/src/evm/opcodes/return.rs | 4 -- .../src/evm_circuit/execution/return.rs | 39 ++++--------------- 2 files changed, 7 insertions(+), 36 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 9ef21fb0ad..adfe7c4a8f 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -71,10 +71,6 @@ impl Opcode for Return { // Case D in the specs. if !call.is_root && !call.is_create() { for (field, value) in [ - ( - CallContextField::CallerId, - u64::try_from(call.caller_id).unwrap(), - ), (CallContextField::ReturnDataOffset, call.return_data_offset), (CallContextField::ReturnDataLength, call.return_data_length), ] { diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 45e986f4a8..3768231df9 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -34,7 +34,6 @@ pub(crate) struct ReturnGadget { copy_rw_increase: Cell, copy_rw_increase_is_zero: IsZeroGadget, - caller_id: Cell, // can you get this out of restore_context? return_data_offset: Cell, return_data_length: Cell, } @@ -105,18 +104,17 @@ impl ExecutionGadget for ReturnGadget { RestoreContextGadget::construct( cb, is_success.expr(), - 3.expr() + copy_rw_increase.expr(), + 2.expr() + copy_rw_increase.expr(), range.offset(), range.length(), ) }); // Case D in the specs. - let (caller_id, return_data_offset, return_data_length, copy_length) = cb.condition( + let (return_data_offset, return_data_length, copy_length) = cb.condition( not::expr(is_create.expr()) * not::expr(is_root.expr()), |cb| { - let [caller_id, return_data_offset, return_data_length] = [ - CallContextFieldTag::CallerId, + let [return_data_offset, return_data_length] = [ CallContextFieldTag::ReturnDataOffset, CallContextFieldTag::ReturnDataLength, ] @@ -128,12 +126,7 @@ impl ExecutionGadget for ReturnGadget { copy_length.min() + copy_length.min(), copy_rw_increase.expr(), ); - ( - caller_id, - return_data_offset, - return_data_length, - copy_length, - ) + (return_data_offset, return_data_length, copy_length) }, ); cb.condition( @@ -144,7 +137,7 @@ impl ExecutionGadget for ReturnGadget { cb.copy_table_lookup( cb.curr.state.call_id.expr(), CopyDataType::Memory.expr(), - caller_id.expr(), + cb.next.state.call_id.expr(), CopyDataType::Memory.expr(), range.offset(), range.address(), @@ -172,7 +165,6 @@ impl ExecutionGadget for ReturnGadget { is_root, is_create, is_success, - caller_id, copy_length, copy_rw_increase, copy_rw_increase_is_zero, @@ -211,10 +203,6 @@ impl ExecutionGadget for ReturnGadget { if !call.is_root && !call.is_create { for (cell, value) in [ - ( - &self.caller_id, - F::from(u64::try_from(call.caller_id).unwrap()), - ), (&self.return_data_length, call.return_data_length.into()), (&self.return_data_offset, call.return_data_offset.into()), ] { @@ -232,7 +220,6 @@ impl ExecutionGadget for ReturnGadget { let copy_rw_increase = if call.is_create { length.as_u64() } else if !call.is_root { - dbg!(2 * std::cmp::min(call.return_data_length, length.as_u64())); 2 * std::cmp::min(call.return_data_length, length.as_u64()) } else { 0 @@ -243,10 +230,8 @@ impl ExecutionGadget for ReturnGadget { .assign(region, offset, F::from(copy_rw_increase))?; if !call.is_root { - self.restore_context.assign( - region, offset, block, call, step, - 5, // 5 + usize::try_from(copy_rw_increase).unwrap(), - )?; + self.restore_context + .assign(region, offset, block, call, step, 5)?; } Ok(()) @@ -331,16 +316,6 @@ mod test { for (((callee_offset, callee_length), (caller_offset, caller_length)), is_return) in test_parameters.iter().cartesian_product(&[true, false]) { - dbg!( - "(callee_offset, callee_length, caller_offset, caller_length, is_return) = {:?}", - ( - *callee_offset, - *callee_length, - *caller_offset, - *caller_length, - *is_return - ) - ); let callee = Account { address: CALLEE_ADDRESS, code: callee_bytecode(*is_return, *callee_offset, *callee_length).into(), From 832c6c0dfaa75cbe5b65accc46023bdd2f05280f Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 28 Sep 2022 12:02:56 -0400 Subject: [PATCH 26/38] Add is_persistent constraint --- bus-mapping/src/evm/opcodes/return.rs | 10 +++++++++- zkevm-circuits/src/evm_circuit/execution/return.rs | 6 ++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index adfe7c4a8f..9427a2248c 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -61,7 +61,15 @@ impl Opcode for Return { )?; } - // No additional bus mapping is needed for case B. + // Case B in the specs. + if call.is_root { + state.call_context_read( + &mut exec_step, + call.call_id, + CallContextField::IsPersistent, + call.is_persistent.to_word(), + ); + } // Case C in the specs. if !call.is_root { diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 3768231df9..6a699e1419 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -95,6 +95,12 @@ impl ExecutionGadget for ReturnGadget { // Case B in the specs. cb.condition(is_root.expr(), |cb| { cb.require_next_state(ExecutionState::EndTx); + cb.call_context_lookup( + false.expr(), + None, + CallContextFieldTag::IsPersistent, + is_success.expr(), + ); }); // Case C in the specs. From 8eafc1bdbaa666626dc80fa1ffb2956a259527ff Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 28 Sep 2022 13:26:51 -0400 Subject: [PATCH 27/38] Use is_root and is_create from cb --- bus-mapping/src/evm/opcodes/return.rs | 13 +++---- .../src/evm_circuit/execution/return.rs | 39 +++++++------------ 2 files changed, 19 insertions(+), 33 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/return.rs b/bus-mapping/src/evm/opcodes/return.rs index 9427a2248c..8113ba8940 100644 --- a/bus-mapping/src/evm/opcodes/return.rs +++ b/bus-mapping/src/evm/opcodes/return.rs @@ -35,13 +35,12 @@ impl Opcode for Return { } let call = state.call()?.clone(); - for (field, value) in [ - (CallContextField::IsRoot, call.is_root), - (CallContextField::IsCreate, call.is_create()), - (CallContextField::IsSuccess, call.is_success), - ] { - state.call_context_read(&mut exec_step, call.call_id, field, value.to_word()); - } + state.call_context_read( + &mut exec_step, + call.call_id, + CallContextField::IsSuccess, + call.is_success.to_word(), + ); let offset = offset.as_usize(); let length = length.as_usize(); diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 6a699e1419..5312c0fd38 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -16,7 +16,7 @@ use crate::{ util::Expr, }; use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; -use eth_types::{Field, ToScalar}; +use eth_types::Field; use halo2_proofs::{circuit::Value, plonk::Error}; #[derive(Clone, Debug)] @@ -25,8 +25,6 @@ pub(crate) struct ReturnGadget { range: MemoryAddressGadget, - is_root: Cell, - is_create: Cell, is_success: Cell, restore_context: RestoreContextGadget, @@ -54,13 +52,7 @@ impl ExecutionGadget for ReturnGadget { cb.stack_pop(length.expr()); let range = MemoryAddressGadget::construct(cb, offset, length); - let [is_root, is_create, is_success] = [ - CallContextFieldTag::IsRoot, - CallContextFieldTag::IsCreate, - CallContextFieldTag::IsSuccess, - ] - .map(|field_tag| cb.call_context(None, field_tag)); - + let is_success = cb.call_context(None, CallContextFieldTag::IsSuccess); cb.require_equal( "if is_success, opcode is RETURN. if not, opcode is REVERT", opcode.expr(), @@ -72,13 +64,15 @@ impl ExecutionGadget for ReturnGadget { // the call is, or is not, a create, root, or successful. See the specs at // https://github.com/privacy-scaling-explorations/zkevm-specs/blob/master/specs/opcode/F3RETURN.md // for more details. + let is_create = cb.curr.state.is_create.expr(); + let is_root = cb.curr.state.is_root.expr(); // These are globally defined because they are used across multiple cases. let copy_rw_increase = cb.query_cell(); let copy_rw_increase_is_zero = IsZeroGadget::construct(cb, copy_rw_increase.expr()); // Case A in the specs. - cb.condition(is_create.expr() * is_success.expr(), |cb| { + cb.condition(is_create.clone() * is_success.expr(), |cb| { cb.require_equal( "increase rw counter once for each memory to bytecode byte copied", copy_rw_increase.expr(), @@ -86,7 +80,7 @@ impl ExecutionGadget for ReturnGadget { ); }); cb.condition( - is_create.expr() * is_success.expr() * not::expr(copy_rw_increase_is_zero.expr()), + is_create.clone() * is_success.expr() * not::expr(copy_rw_increase_is_zero.expr()), |_cb| { // TODO: copy_table_lookup for contract creation. }, @@ -118,7 +112,7 @@ impl ExecutionGadget for ReturnGadget { // Case D in the specs. let (return_data_offset, return_data_length, copy_length) = cb.condition( - not::expr(is_create.expr()) * not::expr(is_root.expr()), + not::expr(is_create.clone()) * not::expr(is_root.clone()), |cb| { let [return_data_offset, return_data_length] = [ CallContextFieldTag::ReturnDataOffset, @@ -136,8 +130,8 @@ impl ExecutionGadget for ReturnGadget { }, ); cb.condition( - not::expr(is_create.expr()) - * not::expr(is_root.expr()) + not::expr(is_create.clone()) + * not::expr(is_root.clone()) * not::expr(copy_rw_increase_is_zero.expr()), |cb| { cb.copy_table_lookup( @@ -158,7 +152,7 @@ impl ExecutionGadget for ReturnGadget { // Without this, copy_rw_increase would be unconstrained for non-create root // calls. - cb.condition(not::expr(is_create.expr()) * is_root.expr(), |cb| { + cb.condition(not::expr(is_create) * is_root, |cb| { cb.require_zero( "rw counter is 0 if there is no copy event", copy_rw_increase.expr(), @@ -168,8 +162,6 @@ impl ExecutionGadget for ReturnGadget { Self { opcode, range, - is_root, - is_create, is_success, copy_length, copy_rw_increase, @@ -199,13 +191,8 @@ impl ExecutionGadget for ReturnGadget { self.range .assign(region, offset, memory_offset, length, block.randomness)?; - for (cell, value) in [ - (&self.is_root, call.is_root), - (&self.is_create, call.is_create), - (&self.is_success, call.is_success), - ] { - cell.assign(region, offset, Value::known(value.to_scalar().unwrap()))?; - } + self.is_success + .assign(region, offset, Value::known(call.is_success.into()))?; if !call.is_root && !call.is_create { for (cell, value) in [ @@ -237,7 +224,7 @@ impl ExecutionGadget for ReturnGadget { if !call.is_root { self.restore_context - .assign(region, offset, block, call, step, 5)?; + .assign(region, offset, block, call, step, 3)?; } Ok(()) From 3b1dcfb5da745da9ac8b94ab5b37a18679c0b9ae Mon Sep 17 00:00:00 2001 From: z2trillion Date: Thu, 29 Sep 2022 18:01:04 -0400 Subject: [PATCH 28/38] Constrain step state transition when root --- .../src/evm_circuit/execution/return.rs | 21 +++++++++++++++++-- .../src/evm_circuit/util/common_gadget.rs | 5 ++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 5312c0fd38..457219c3a9 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -2,11 +2,14 @@ use crate::evm_circuit::util::memory_gadget::MemoryAddressGadget; use crate::{ evm_circuit::{ execution::ExecutionGadget, - param::N_BYTES_MEMORY_ADDRESS, + param::{N_BYTES_MEMORY_ADDRESS, STACK_CAPACITY}, step::ExecutionState, util::{ common_gadget::RestoreContextGadget, - constraint_builder::ConstraintBuilder, + constraint_builder::{ + ConstraintBuilder, StepStateTransition, + Transition::{Any, Delta, To}, + }, math_gadget::{IsZeroGadget, MinMaxGadget}, not, CachedRegion, Cell, }, @@ -95,6 +98,19 @@ impl ExecutionGadget for ReturnGadget { CallContextFieldTag::IsPersistent, is_success.expr(), ); + cb.require_step_state_transition(StepStateTransition { + program_counter: To(0.expr()), + stack_pointer: To(STACK_CAPACITY.expr()), + rw_counter: Delta( + cb.rw_counter_offset() + + not::expr(is_success.expr()) + * cb.curr.state.reversible_write_counter.expr(), + ), + reversible_write_counter: To(0.expr()), + memory_word_size: Any, + gas_left: Any, + ..StepStateTransition::default() + }); }); // Case C in the specs. @@ -181,6 +197,7 @@ impl ExecutionGadget for ReturnGadget { call: &Call, step: &ExecStep, ) -> Result<(), Error> { + dbg!(step.reversible_write_counter); self.opcode.assign( region, offset, diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 6cca737809..94535f12a2 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -99,14 +99,13 @@ pub(crate) struct RestoreContextGadget { impl RestoreContextGadget { pub(crate) fn construct( cb: &mut ConstraintBuilder, - is_success: Expression, // just remove this? - copy_lookup_rw_counter_increase: Expression, // try to set it up so this is always 0? + is_success: Expression, + copy_lookup_rw_counter_increase: Expression, return_data_offset: Expression, return_data_length: Expression, ) -> Self { // Read caller's context for restore let caller_id = cb.call_context(None, CallContextFieldTag::CallerId); - // TODO: lookup is_success here instead of taking in as argument. let [caller_is_root, caller_is_create, caller_code_hash, caller_program_counter, caller_stack_pointer, caller_gas_left, caller_memory_word_size, caller_reversible_write_counter] = [ CallContextFieldTag::IsRoot, From 1c3f6d18e3a442a1012bb2ad98d006b68c76bb75 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Thu, 29 Sep 2022 18:45:06 -0400 Subject: [PATCH 29/38] Add memory expansion gadget --- .../src/evm_circuit/execution/return.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 457219c3a9..8e177f7def 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -1,8 +1,7 @@ -use crate::evm_circuit::util::memory_gadget::MemoryAddressGadget; use crate::{ evm_circuit::{ execution::ExecutionGadget, - param::{N_BYTES_MEMORY_ADDRESS, STACK_CAPACITY}, + param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE, STACK_CAPACITY}, step::ExecutionState, util::{ common_gadget::RestoreContextGadget, @@ -11,6 +10,7 @@ use crate::{ Transition::{Any, Delta, To}, }, math_gadget::{IsZeroGadget, MinMaxGadget}, + memory_gadget::{MemoryAddressGadget, MemoryExpansionGadget}, not, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, @@ -37,6 +37,8 @@ pub(crate) struct ReturnGadget { return_data_offset: Cell, return_data_length: Cell, + + memory_expansion: MemoryExpansionGadget, } // TODO: rename this is reflect the fact that is handles REVERT as well. @@ -74,6 +76,12 @@ impl ExecutionGadget for ReturnGadget { let copy_rw_increase = cb.query_cell(); let copy_rw_increase_is_zero = IsZeroGadget::construct(cb, copy_rw_increase.expr()); + let memory_expansion = MemoryExpansionGadget::construct( + cb, + cb.curr.state.memory_word_size.expr(), + [range.address()], + ); + // Case A in the specs. cb.condition(is_create.clone() * is_success.expr(), |cb| { cb.require_equal( @@ -185,6 +193,7 @@ impl ExecutionGadget for ReturnGadget { return_data_offset, return_data_length, restore_context, + memory_expansion, } } @@ -205,8 +214,11 @@ impl ExecutionGadget for ReturnGadget { )?; let [memory_offset, length] = [0, 1].map(|i| block.rws[step.rw_indices[i]].stack_value()); - self.range + let range = self + .range .assign(region, offset, memory_offset, length, block.randomness)?; + self.memory_expansion + .assign(region, offset, step.memory_word_size(), [range])?; self.is_success .assign(region, offset, Value::known(call.is_success.into()))?; From d1e2a8ab7761dff54099e3cd0b389eb7270a1ab9 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Fri, 30 Sep 2022 09:31:44 -0400 Subject: [PATCH 30/38] Constrain gas left and memory word size for root --- zkevm-circuits/src/evm_circuit/execution/return.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 8e177f7def..0bc4d55d51 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -114,9 +114,9 @@ impl ExecutionGadget for ReturnGadget { + not::expr(is_success.expr()) * cb.curr.state.reversible_write_counter.expr(), ), + gas_left: Delta(-memory_expansion.gas_cost()), reversible_write_counter: To(0.expr()), - memory_word_size: Any, - gas_left: Any, + memory_word_size: To(0.expr()), ..StepStateTransition::default() }); }); From cb0893193eda2d0c1ea0fbb713f4b5a87aa4c73d Mon Sep 17 00:00:00 2001 From: z2trillion Date: Fri, 30 Sep 2022 14:43:54 -0400 Subject: [PATCH 31/38] Calculate gas left correctly --- .../circuit_input_builder/input_state_ref.rs | 49 ++++++++++++------- .../src/evm_circuit/execution/return.rs | 2 + .../src/evm_circuit/execution/stop.rs | 2 +- .../src/evm_circuit/util/common_gadget.rs | 13 ++--- 4 files changed, 41 insertions(+), 25 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 b36f2f9488..fc435f9d34 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -16,7 +16,7 @@ use crate::{ Error, }; use eth_types::{ - evm_types::{Gas, MemoryAddress, OpcodeId, StackAddress}, + evm_types::{gas_utils::memory_expansion_gas_cost, Gas, MemoryAddress, OpcodeId, StackAddress}, Address, GethExecStep, ToAddress, ToBigEndian, ToWord, Word, H256, }; use ethers_core::utils::{get_contract_address, get_create2_address}; @@ -843,7 +843,36 @@ impl<'a> CircuitInputStateRef<'a> { let geth_step = &steps[0]; let geth_step_next = &steps[1]; - let caller_gas_left = geth_step_next.gas.0 - geth_step.gas.0; + + let [last_callee_return_data_offset, last_callee_return_data_length] = match geth_step.op { + OpcodeId::STOP => [Word::zero(); 2], + OpcodeId::REVERT | OpcodeId::RETURN => { + // TODO: move this back into the return bus mapping. + let offset = geth_step.stack.nth_last(0)?; + let length = geth_step.stack.nth_last(1)?; + // This is the convention we are using for memory addresses so that there is no + // memory expansion cost when the length is 0. + if length.is_zero() { + [Word::zero(); 2] + } else { + [offset, length] + } + } + _ => unreachable!(), + }; + + 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() { + (last_callee_return_data_offset + last_callee_return_data_length + 31).as_u64() / 32 + } else { + curr_memory_word_size + }; + + let memory_expansion_gas_cost = + memory_expansion_gas_cost(curr_memory_word_size, next_memory_word_size); + let gas_refund = geth_step.gas.0 - memory_expansion_gas_cost; + let caller_gas_left = geth_step_next.gas.0 - gas_refund; + for (field, value) in [ (CallContextField::IsRoot, (caller.is_root as u64).into()), ( @@ -869,22 +898,6 @@ impl<'a> CircuitInputStateRef<'a> { self.call_context_read(exec_step, caller.call_id, field, value); } - let [last_callee_return_data_offset, last_callee_return_data_length] = match geth_step.op { - OpcodeId::STOP => [Word::zero(); 2], - OpcodeId::REVERT | OpcodeId::RETURN => { - // TODO: move this back into the return bus mapping. - let offset = geth_step.stack.nth_last(0)?; - let length = geth_step.stack.nth_last(1)?; - // This is the convention we are using for memory addresses so that there is no - // memory expansion cost when the length is 0. - if length.is_zero() { - [Word::zero(); 2] - } else { - [offset, length] - } - } - _ => unreachable!(), - }; for (field, value) in [ (CallContextField::LastCalleeId, call.call_id.into()), ( diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 0bc4d55d51..d901834922 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -131,6 +131,7 @@ impl ExecutionGadget for ReturnGadget { 2.expr() + copy_rw_increase.expr(), range.offset(), range.length(), + memory_expansion.gas_cost(), ) }); @@ -331,6 +332,7 @@ mod test { ((0, 10), (0, 10)), ((0, 10), (0, 20)), ((0, 20), (0, 10)), + ((64, 1), (0, 10)), // Expands memory in RETURN/REVERT opcode ((0, 10), (1000, 0)), ((1000, 0), (0, 10)), ((1000, 0), (1000, 0)), diff --git a/zkevm-circuits/src/evm_circuit/execution/stop.rs b/zkevm-circuits/src/evm_circuit/execution/stop.rs index 736534c986..be2e6d1a1d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/stop.rs +++ b/zkevm-circuits/src/evm_circuit/execution/stop.rs @@ -82,7 +82,7 @@ impl ExecutionGadget for StopGadget { // When it's an internal call let restore_context = cb.condition(1.expr() - cb.curr.state.is_root.expr(), |cb| { - RestoreContextGadget::construct(cb, true.expr(), 0.expr(), 0.expr(), 0.expr()) + RestoreContextGadget::construct(cb, true.expr(), 0.expr(), 0.expr(), 0.expr(), 0.expr()) }); Self { diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 94535f12a2..d9ac75cd21 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -103,6 +103,7 @@ impl RestoreContextGadget { copy_lookup_rw_counter_increase: Expression, return_data_offset: Expression, return_data_length: Expression, + memory_expansion_cost: Expression, ) -> Self { // Read caller's context for restore let caller_id = cb.call_context(None, CallContextFieldTag::CallerId); @@ -137,13 +138,13 @@ impl RestoreContextGadget { cb.call_context_lookup(true.expr(), Some(caller_id.expr()), field_tag, value); } - // Consume all gas_left if call halts in exception - let gas_left = if cb.execution_state().halts_in_exception() { - caller_gas_left.expr() + let gas_refund = if cb.execution_state().halts_in_exception() { + 0.expr() // no gas refund if call halts in exception } else { - caller_gas_left.expr() + cb.curr.state.gas_left.expr() + cb.curr.state.gas_left.expr() - memory_expansion_cost }; - // TODO: Do we need to constrain the gas cost of the return here? + + let gas_left = caller_gas_left.expr() + gas_refund; // Accumulate reversible_write_counter in case this call stack reverts in the // future even it itself succeeds. Note that when sub-call halts in @@ -165,7 +166,7 @@ impl RestoreContextGadget { code_hash: To(caller_code_hash.expr()), program_counter: To(caller_program_counter.expr()), stack_pointer: To(caller_stack_pointer.expr()), - gas_left: To(gas_left.expr()), + gas_left: To(gas_left), memory_word_size: To(caller_memory_word_size.expr()), reversible_write_counter: To(reversible_write_counter), log_id: Same, From 3949303c743f275529e6b9815b82c101ef218762 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Fri, 30 Sep 2022 15:00:31 -0400 Subject: [PATCH 32/38] cleanup --- bus-mapping/src/circuit_input_builder/input_state_ref.rs | 1 - zkevm-circuits/src/evm_circuit/execution/return.rs | 2 +- 2 files changed, 1 insertion(+), 2 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 fc435f9d34..1106fa01ca 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -847,7 +847,6 @@ impl<'a> CircuitInputStateRef<'a> { let [last_callee_return_data_offset, last_callee_return_data_length] = match geth_step.op { OpcodeId::STOP => [Word::zero(); 2], OpcodeId::REVERT | OpcodeId::RETURN => { - // TODO: move this back into the return bus mapping. let offset = geth_step.stack.nth_last(0)?; let length = geth_step.stack.nth_last(1)?; // This is the convention we are using for memory addresses so that there is no diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index d901834922..1339de82ab 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -7,7 +7,7 @@ use crate::{ common_gadget::RestoreContextGadget, constraint_builder::{ ConstraintBuilder, StepStateTransition, - Transition::{Any, Delta, To}, + Transition::{Delta, To}, }, math_gadget::{IsZeroGadget, MinMaxGadget}, memory_gadget::{MemoryAddressGadget, MemoryExpansionGadget}, From 6c3568e8f8556781bf7287f7d43bfcc617076b27 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Fri, 30 Sep 2022 15:26:37 -0400 Subject: [PATCH 33/38] Use correct new memory word size --- bus-mapping/src/circuit_input_builder/input_state_ref.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 1106fa01ca..f243f0cbd5 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -862,7 +862,11 @@ impl<'a> CircuitInputStateRef<'a> { 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() { - (last_callee_return_data_offset + last_callee_return_data_length + 31).as_u64() / 32 + 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 }; From 3312b9cdb72e11e9849dcc2c4c676926a4f8df32 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Fri, 30 Sep 2022 15:26:52 -0400 Subject: [PATCH 34/38] Rename input variable --- zkevm-circuits/src/evm_circuit/util/common_gadget.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index d9ac75cd21..57cf4eeef6 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -100,7 +100,8 @@ impl RestoreContextGadget { pub(crate) fn construct( cb: &mut ConstraintBuilder, is_success: Expression, - copy_lookup_rw_counter_increase: Expression, + // Expression for the number of rw lookups that occur after this gadget is constructed. + subsequent_rw_lookups: Expression, return_data_offset: Expression, return_data_length: Expression, memory_expansion_cost: Expression, @@ -154,7 +155,7 @@ impl RestoreContextGadget { + is_success.clone() * cb.curr.state.reversible_write_counter.expr(); let rw_counter_offset = cb.rw_counter_offset() - + copy_lookup_rw_counter_increase + + subsequent_rw_lookups + not::expr(is_success.expr()) * cb.curr.state.reversible_write_counter.expr(); // Do step state transition From 6019bed2d35023af1f5e6712160f804f43dc2d4d Mon Sep 17 00:00:00 2001 From: z2trillion Date: Fri, 30 Sep 2022 15:44:11 -0400 Subject: [PATCH 35/38] Add condition for is_create in rw_counter increase --- zkevm-circuits/src/evm_circuit/execution/return.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index 1339de82ab..ebb05aecff 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -128,7 +128,7 @@ impl ExecutionGadget for ReturnGadget { RestoreContextGadget::construct( cb, is_success.expr(), - 2.expr() + copy_rw_increase.expr(), + 2.expr() * not::expr(is_create.clone()) + copy_rw_increase.expr(), range.offset(), range.length(), memory_expansion.gas_cost(), From 98c8cc2e69428d2a034dc478f98306454b176c91 Mon Sep 17 00:00:00 2001 From: z2trillion Date: Tue, 4 Oct 2022 23:30:32 -0400 Subject: [PATCH 36/38] Fix test and cleanup --- external-tracer/src/lib.rs | 2 +- zkevm-circuits/src/evm_circuit/execution/return.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/external-tracer/src/lib.rs b/external-tracer/src/lib.rs index 678bc21e33..c613643279 100644 --- a/external-tracer/src/lib.rs +++ b/external-tracer/src/lib.rs @@ -45,7 +45,7 @@ pub struct LoggerConfig { impl Default for LoggerConfig { fn default() -> Self { Self { - enable_memory: false, + enable_memory: true, disable_stack: false, disable_storage: false, enable_return_data: true, diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index ebb05aecff..e6754177f4 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -207,7 +207,6 @@ impl ExecutionGadget for ReturnGadget { call: &Call, step: &ExecStep, ) -> Result<(), Error> { - dbg!(step.reversible_write_counter); self.opcode.assign( region, offset, From 2fb9fd8a925eda97839e51dd0fa31edaf1fad10d Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 5 Oct 2022 15:52:58 -0400 Subject: [PATCH 37/38] Set enable_memory default to false --- bus-mapping/src/circuit_input_builder/input_state_ref.rs | 2 +- external-tracer/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 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 f243f0cbd5..9e8c56fa82 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -891,7 +891,7 @@ impl<'a> CircuitInputStateRef<'a> { (CallContextField::GasLeft, caller_gas_left.into()), ( CallContextField::MemorySize, - geth_step_next.memory.word_size().into(), + self.caller_ctx()?.memory.word_size().into(), ), ( CallContextField::ReversibleWriteCounter, diff --git a/external-tracer/src/lib.rs b/external-tracer/src/lib.rs index c613643279..678bc21e33 100644 --- a/external-tracer/src/lib.rs +++ b/external-tracer/src/lib.rs @@ -45,7 +45,7 @@ pub struct LoggerConfig { impl Default for LoggerConfig { fn default() -> Self { Self { - enable_memory: true, + enable_memory: false, disable_stack: false, disable_storage: false, enable_return_data: true, From 834f65b9ad758b500c06eda9ab48ca62779e1fca Mon Sep 17 00:00:00 2001 From: z2trillion Date: Wed, 5 Oct 2022 22:35:22 -0400 Subject: [PATCH 38/38] Constrain is_success to be boolean --- zkevm-circuits/src/evm_circuit/execution/return.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/zkevm-circuits/src/evm_circuit/execution/return.rs b/zkevm-circuits/src/evm_circuit/execution/return.rs index e6754177f4..9da3ab4761 100644 --- a/zkevm-circuits/src/evm_circuit/execution/return.rs +++ b/zkevm-circuits/src/evm_circuit/execution/return.rs @@ -58,6 +58,7 @@ impl ExecutionGadget for ReturnGadget { let range = MemoryAddressGadget::construct(cb, offset, length); let is_success = cb.call_context(None, CallContextFieldTag::IsSuccess); + cb.require_boolean("is_success is boolean", is_success.expr()); cb.require_equal( "if is_success, opcode is RETURN. if not, opcode is REVERT", opcode.expr(),