From 44bf57796c330f19222edadfd664301c19ae38ed Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Wed, 23 Feb 2022 23:28:41 +0800 Subject: [PATCH 01/12] feat: codecopy (squashed and rebased with main) --- bus-mapping/src/circuit_input_builder.rs | 13 + zkevm-circuits/src/evm_circuit/execution.rs | 10 + .../src/evm_circuit/execution/codecopy.rs | 481 +++++++++++++++++ .../execution/copy_code_to_memory.rs | 489 ++++++++++++++++++ .../src/evm_circuit/execution/memory_copy.rs | 29 +- zkevm-circuits/src/evm_circuit/step.rs | 2 + zkevm-circuits/src/evm_circuit/table.rs | 1 + .../evm_circuit/util/constraint_builder.rs | 20 + zkevm-circuits/src/evm_circuit/witness.rs | 6 +- 9 files changed, 1042 insertions(+), 9 deletions(-) create mode 100644 zkevm-circuits/src/evm_circuit/execution/codecopy.rs create mode 100644 zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs index 43c297cbba..84044a01cd 100644 --- a/bus-mapping/src/circuit_input_builder.rs +++ b/bus-mapping/src/circuit_input_builder.rs @@ -155,6 +155,19 @@ pub enum StepAuxiliaryData { /// Indicate if copy from transaction call data from_tx: bool, }, + /// Auxiliary data of Copy Code To Memory + CopyCodeToMemory { + /// Source start address + src_addr: u64, + /// Destination address + dst_addr: u64, + /// Bytes left + bytes_left: u64, + /// Source end address + src_addr_end: u64, + /// Bytecode to be copied + code: eth_types::Bytecode, + }, } /// An execution step of the EVM. diff --git a/zkevm-circuits/src/evm_circuit/execution.rs b/zkevm-circuits/src/evm_circuit/execution.rs index 10651a6c7e..a1f65ace3f 100644 --- a/zkevm-circuits/src/evm_circuit/execution.rs +++ b/zkevm-circuits/src/evm_circuit/execution.rs @@ -28,8 +28,10 @@ mod calldatasize; mod caller; mod callvalue; mod chainid; +mod codecopy; mod coinbase; mod comparator; +mod copy_code_to_memory; mod dup; mod end_block; mod end_tx; @@ -70,8 +72,10 @@ use calldatasize::CallDataSizeGadget; use caller::CallerGadget; use callvalue::CallValueGadget; use chainid::ChainIdGadget; +use codecopy::CodeCopyGadget; use coinbase::CoinbaseGadget; use comparator::ComparatorGadget; +use copy_code_to_memory::CopyCodeToMemoryGadget; use dup::DupGadget; use end_block::EndBlockGadget; use end_tx::EndTxGadget; @@ -142,8 +146,10 @@ pub(crate) struct ExecutionConfig { calldatasize_gadget: CallDataSizeGadget, caller_gadget: CallerGadget, chainid_gadget: ChainIdGadget, + codecopy_gadget: CodeCopyGadget, coinbase_gadget: CoinbaseGadget, comparator_gadget: ComparatorGadget, + copy_code_to_memory_gadget: CopyCodeToMemoryGadget, dup_gadget: DupGadget, extcodehash_gadget: ExtcodehashGadget, gas_gadget: GasGadget, @@ -367,8 +373,10 @@ impl ExecutionConfig { calldatasize_gadget: configure_gadget!(), caller_gadget: configure_gadget!(), chainid_gadget: configure_gadget!(), + codecopy_gadget: configure_gadget!(), coinbase_gadget: configure_gadget!(), comparator_gadget: configure_gadget!(), + copy_code_to_memory_gadget: configure_gadget!(), dup_gadget: configure_gadget!(), extcodehash_gadget: configure_gadget!(), gas_gadget: configure_gadget!(), @@ -627,6 +635,7 @@ impl ExecutionConfig { match step.execution_state { // internal states ExecutionState::BeginTx => assign_exec_step!(self.begin_tx_gadget), + ExecutionState::CopyCodeToMemory => assign_exec_step!(self.copy_code_to_memory_gadget), ExecutionState::CopyToMemory => assign_exec_step!(self.copy_to_memory_gadget), ExecutionState::EndTx => assign_exec_step!(self.end_tx_gadget), ExecutionState::EndBlock => assign_exec_step!(self.end_block_gadget), @@ -641,6 +650,7 @@ impl ExecutionConfig { ExecutionState::CALLER => assign_exec_step!(self.caller_gadget), ExecutionState::CALLVALUE => assign_exec_step!(self.call_value_gadget), ExecutionState::CHAINID => assign_exec_step!(self.chainid_gadget), + ExecutionState::CODECOPY => assign_exec_step!(self.codecopy_gadget), ExecutionState::COINBASE => assign_exec_step!(self.coinbase_gadget), ExecutionState::CMP => assign_exec_step!(self.comparator_gadget), ExecutionState::DUP => assign_exec_step!(self.dup_gadget), diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs new file mode 100644 index 0000000000..110cf801bd --- /dev/null +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -0,0 +1,481 @@ +use std::convert::TryInto; + +use bus_mapping::evm::OpcodeId; +use eth_types::{Field, ToLittleEndian, ToScalar}; +use halo2_proofs::{circuit::Region, plonk::Error}; + +use crate::{ + evm_circuit::{ + param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, + step::ExecutionState, + table::{AccountFieldTag, CallContextFieldTag}, + util::{ + common_gadget::SameContextGadget, + constraint_builder::{ConstraintBuilder, StepStateTransition, Transition}, + from_bytes, + memory_gadget::{MemoryAddressGadget, MemoryCopierGasGadget, MemoryExpansionGadget}, + Cell, MemoryAddress, + }, + witness::{Block, Call, CodeSource, ExecStep, Transaction}, + }, + util::Expr, +}; + +use super::ExecutionGadget; + +#[derive(Clone, Debug)] +pub(crate) struct CodeCopyGadget { + same_context: SameContextGadget, + /// Holds the memory address for the offset in code from where we read. + code_offset: MemoryAddress, + /// Holds the size of the current environment's bytecode. + code_size: Cell, + /// Holds the current environment's address from where we wish to read code. + account: Cell, + /// The code from current environment is copied to memory. To verify this + /// copy operation we need the MemoryAddressGadget. + dst_memory_addr: MemoryAddressGadget, + /// Opcode CODECOPY has a dynamic gas cost: + /// gas_code = static_gas * minimum_word_size + memory_expansion_cost + memory_expansion: MemoryExpansionGadget, + /// Opcode CODECOPY needs to copy code bytes into memory. We account for + /// the copying costs using the memory copier gas gadget. + memory_copier_gas: MemoryCopierGasGadget, +} + +impl ExecutionGadget for CodeCopyGadget { + const NAME: &'static str = "CODECOPY"; + + const EXECUTION_STATE: ExecutionState = ExecutionState::CODECOPY; + + fn configure(cb: &mut ConstraintBuilder) -> Self { + let opcode = cb.query_cell(); + + // Query elements to be popped from the stack. + let dest_memory_offset = cb.query_cell(); + let code_offset = cb.query_rlc(); + let size = cb.query_rlc(); + + // Pop items from stack. + cb.stack_pop(dest_memory_offset.expr()); + cb.stack_pop(code_offset.expr()); + cb.stack_pop(size.expr()); + + // Construct memory address in the destionation (memory) to which we copy code. + let dst_memory_addr = MemoryAddressGadget::construct(cb, dest_memory_offset, size); + + // Query additional fields for the account's code. + let account = cb.call_context(None, CallContextFieldTag::CalleeAddress); + let code_size = cb.query_cell(); + let code_hash = cb.curr.state.code_source.clone(); + + // Lookup the code hash and code size of the current environment's account. + cb.account_read(account.expr(), AccountFieldTag::CodeSize, code_size.expr()); + cb.account_read(account.expr(), AccountFieldTag::CodeHash, code_hash.expr()); + + // Calculate the next memory size and the gas cost for this memory + // access. This also accounts for the dynamic gas required to copy bytes to + // memory. + let memory_expansion = MemoryExpansionGadget::construct( + cb, + cb.curr.state.memory_word_size.expr(), + [dst_memory_addr.address()], + ); + let memory_copier_gas = MemoryCopierGasGadget::construct( + cb, + dst_memory_addr.length(), + memory_expansion.gas_cost(), + ); + + // Constrain the next step to be the internal `CopyCodeToMemory` step and add + // some preliminary checks on its auxiliary data. + cb.constrain_next_step( + ExecutionState::CopyCodeToMemory, + Some(dst_memory_addr.has_length()), + |cb| { + let next_src_addr = cb.query_cell(); + let next_dst_addr = cb.query_cell(); + let next_bytes_left = cb.query_cell(); + let next_src_addr_end = cb.query_cell(); + let next_code_hash = cb.query_word(); + + cb.require_equal( + "next_src_addr == code_offset", + next_src_addr.expr(), + from_bytes::expr(&code_offset.cells), + ); + cb.require_equal( + "next_dst_addr = memory_offset", + next_dst_addr.expr(), + dst_memory_addr.offset(), + ); + cb.require_equal( + "next_bytes_left = length", + next_bytes_left.expr(), + dst_memory_addr.length(), + ); + cb.require_equal( + "next_src_addr_end == code_size", + next_src_addr_end.expr(), + code_size.expr(), + ); + cb.require_equal( + "next_code_hash == code_hash", + next_code_hash.expr(), + code_hash.expr(), + ); + }, + ); + + // Expected state transition. + let step_state_transition = StepStateTransition { + rw_counter: Transition::Delta(cb.rw_counter_offset()), + program_counter: Transition::Delta(1.expr()), + stack_pointer: Transition::Delta(3.expr()), + memory_word_size: Transition::To(memory_expansion.next_memory_word_size()), + gas_left: Transition::Delta( + -OpcodeId::CODECOPY.constant_gas_cost().expr() - memory_copier_gas.gas_cost(), + ), + ..Default::default() + }; + let same_context = SameContextGadget::construct(cb, opcode, step_state_transition); + + Self { + same_context, + code_offset, + code_size, + account, + dst_memory_addr, + memory_expansion, + memory_copier_gas, + } + } + + fn assign_exec_step( + &self, + region: &mut Region<'_, F>, + offset: usize, + block: &Block, + _tx: &Transaction, + call: &Call, + step: &ExecStep, + ) -> Result<(), Error> { + self.same_context.assign_exec_step(region, offset, step)?; + + // 1. `dest_offset` is the bytes offset in the memory where we start to + // write. + // 2. `code_offset` is the bytes offset in the current + // context's code where we start to read. + // 3. `size` is the number of + // bytes to be read and written (0s to be copied for out of bounds). + let [dest_offset, code_offset, size] = + [0, 1, 2].map(|i| block.rws[step.rw_indices[i]].stack_value()); + + // assign the code offset memory address. + self.code_offset.assign( + region, + offset, + Some( + code_offset.to_le_bytes()[..N_BYTES_MEMORY_ADDRESS] + .try_into() + .unwrap(), + ), + )?; + + let account = if call.is_root { + call.callee_address + } else { + unimplemented!("CODECOPY does not support internal calls yet"); + }; + println!("account = {:?}", account); + self.account.assign(region, offset, account.to_scalar())?; + + let code = block + .bytecodes + .iter() + .find(|b| { + let CodeSource::Account(code_hash) = &call.code_source; + b.hash == *code_hash + }) + .expect("could not find current environment's bytecode"); + self.code_size + .assign(region, offset, Some(F::from(code.bytes.len() as u64)))?; + + // assign the destination memory offset. + let memory_address = + self.dst_memory_addr + .assign(region, offset, dest_offset, size, block.randomness)?; + + // assign to gadgets handling memory expansion cost and copying cost. + let (_, memory_expansion_cost) = self.memory_expansion.assign( + region, + offset, + step.memory_word_size(), + [memory_address], + )?; + self.memory_copier_gas + .assign(region, offset, size.as_u64(), memory_expansion_cost)?; + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use std::ops::Sub; + + use bus_mapping::evm::OpcodeId; + use eth_types::{bytecode, Address, ToWord, Word}; + use halo2_proofs::arithmetic::BaseExt; + use num::Zero; + use pairing::bn256::Fr; + + use crate::evm_circuit::{ + execution::copy_code_to_memory::test::make_copy_code_steps, + step::ExecutionState, + table::{AccountFieldTag, CallContextFieldTag, RwTableTag}, + test::{calc_memory_copier_gas_cost, run_test_circuit_incomplete_fixed_table}, + witness::{Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, Transaction}, + }; + + fn test_ok(src_addr: u64, dst_addr: u64, size: usize) { + let randomness = Fr::rand(); + let call_id = 1; + let callee_addr = Address::random(); + + let code = bytecode! { + #[start] + PUSH32(Word::from(size)) + PUSH32(Word::from(src_addr)) + PUSH32(Word::from(dst_addr)) + CODECOPY + STOP + }; + let code = Bytecode::new(code.to_vec()); + + let mut rws_map = RwMap( + [ + ( + RwTableTag::Stack, + vec![ + // Stack item written by PUSH32. + Rw::Stack { + rw_counter: 1, + is_write: true, + call_id, + stack_pointer: 1023, + value: Word::from(size), + }, + // Stack item written by PUSH32. + Rw::Stack { + rw_counter: 2, + is_write: true, + call_id, + stack_pointer: 1022, + value: Word::from(src_addr), + }, + // Stack item written by PUSH32. + Rw::Stack { + rw_counter: 3, + is_write: true, + call_id, + stack_pointer: 1021, + value: Word::from(dst_addr), + }, + // First stack item read by CODECOPY. + Rw::Stack { + rw_counter: 4, + is_write: false, + call_id, + stack_pointer: 1021, + value: Word::from(dst_addr), + }, + // Second stack item read by CODECOPY. + Rw::Stack { + rw_counter: 5, + is_write: false, + call_id, + stack_pointer: 1022, + value: Word::from(src_addr), + }, + // Third stack item read by CODECOPY. + Rw::Stack { + rw_counter: 6, + is_write: false, + call_id, + stack_pointer: 1023, + value: Word::from(size), + }, + ], + ), + ( + RwTableTag::CallContext, + vec![ + // Callee address lookup in CODECOPY. + Rw::CallContext { + rw_counter: 7, + call_id, + is_write: false, + field_tag: CallContextFieldTag::CalleeAddress, + value: callee_addr.to_word(), + }, + ], + ), + ( + RwTableTag::Account, + vec![ + // Code size lookup in CODECOPY. + Rw::Account { + rw_counter: 8, + is_write: false, + account_address: callee_addr, + field_tag: AccountFieldTag::CodeSize, + value: Word::from(code.bytes.len()), + value_prev: Word::from(code.bytes.len()), + }, + // Code hash lookup in CODECOPY. + Rw::Account { + rw_counter: 9, + is_write: false, + account_address: callee_addr, + field_tag: AccountFieldTag::CodeHash, + value: code.hash, + value_prev: code.hash, + }, + ], + ), + ] + .into(), + ); + + // After copying bytes from code to memory, we would end up having used this + // much memory. + let next_memory_word_size = if size.is_zero() { + 0 + } else { + (dst_addr + size as u64 + 31) / 32 + }; + let gas_cost_push32 = OpcodeId::PUSH32.constant_gas_cost().as_u64(); + let gas_cost_codecopy = OpcodeId::CODECOPY.constant_gas_cost().as_u64() + + calc_memory_copier_gas_cost(0, next_memory_word_size, size as u64); + let total_gas_cost = (3 * gas_cost_push32) + gas_cost_codecopy; + + let mut steps = vec![ + ExecStep { + rw_indices: vec![(RwTableTag::Stack, 0)], + execution_state: ExecutionState::PUSH, + rw_counter: 1, + program_counter: 0, + stack_pointer: 1024, + gas_left: total_gas_cost, + gas_cost: gas_cost_push32, + opcode: Some(OpcodeId::PUSH32), + ..Default::default() + }, + ExecStep { + rw_indices: vec![(RwTableTag::Stack, 1)], + execution_state: ExecutionState::PUSH, + rw_counter: 2, + program_counter: 33, + stack_pointer: 1023, + gas_left: total_gas_cost.sub(gas_cost_push32), + gas_cost: gas_cost_push32, + opcode: Some(OpcodeId::PUSH32), + ..Default::default() + }, + ExecStep { + rw_indices: vec![(RwTableTag::Stack, 2)], + execution_state: ExecutionState::PUSH, + rw_counter: 3, + program_counter: 66, + stack_pointer: 1022, + gas_left: total_gas_cost.sub(2 * gas_cost_push32), + gas_cost: gas_cost_push32, + opcode: Some(OpcodeId::PUSH32), + ..Default::default() + }, + ExecStep { + rw_indices: vec![ + (RwTableTag::Stack, 3), + (RwTableTag::Stack, 4), + (RwTableTag::Stack, 5), + (RwTableTag::CallContext, 0), + (RwTableTag::Account, 0), + (RwTableTag::Account, 1), + ], + execution_state: ExecutionState::CODECOPY, + rw_counter: 4, + program_counter: 99, + stack_pointer: 1021, + gas_left: gas_cost_codecopy, + gas_cost: gas_cost_codecopy, + opcode: Some(OpcodeId::CODECOPY), + ..Default::default() + }, + ]; + + let program_counter = 100; + let stack_pointer = 1024; + let mut rw_counter = 10; + if !size.is_zero() { + make_copy_code_steps( + call_id, + &code, + src_addr, + dst_addr, + size, + program_counter, + stack_pointer, + next_memory_word_size * 32, + &mut rw_counter, + &mut rws_map, + &mut steps, + ); + } + steps.push(ExecStep { + execution_state: ExecutionState::STOP, + rw_counter, + program_counter: 100, + stack_pointer: 1024, + opcode: Some(OpcodeId::STOP), + memory_size: next_memory_word_size * 32, + ..Default::default() + }); + + let block = Block { + randomness, + txs: vec![Transaction { + id: 1, + calls: vec![Call { + id: call_id, + is_root: true, + is_create: false, + code_source: CodeSource::Account(code.hash), + callee_address: callee_addr, + ..Default::default() + }], + steps, + ..Default::default() + }], + rws: rws_map, + bytecodes: vec![code], + ..Default::default() + }; + assert_eq!(run_test_circuit_incomplete_fixed_table(block), Ok(())); + } + + #[test] + fn codecopy_gadget_single_step() { + test_ok(0x00, 0x00, 54); + } + + #[test] + fn codecopy_gadget_multi_step() { + test_ok(0x00, 0x40, 123); + } + + #[test] + fn codecopy_gadget_oob() { + test_ok(0x10, 0x20, 200); + } +} diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs new file mode 100644 index 0000000000..0d28312fc3 --- /dev/null +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -0,0 +1,489 @@ +use array_init::array_init; +use eth_types::{Field, ToLittleEndian}; +use halo2_proofs::{circuit::Region, plonk::Error}; + +use crate::{ + evm_circuit::{ + param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, + step::ExecutionState, + util::{ + constraint_builder::{ConstraintBuilder, StepStateTransition, Transition}, + math_gadget::ComparisonGadget, + memory_gadget::BufferReaderGadget, + Cell, Word, + }, + witness::{Block, Call, ExecStep, StepAuxiliaryData, Transaction}, + }, + util::Expr, +}; + +use super::ExecutionGadget; + +/// Maximum number of bytes that can be copied in one iteration of this gadget. +/// We are bounded because of the limitation on the number of cells we can +/// assign to in one iteration. +const MAX_COPY_BYTES: usize = 54; + +#[derive(Clone, Debug)] +/// This gadget is responsible for copying bytes from an account's code to +/// memory. This is an internal gadget used by the `CodeCopyGadget`. +pub(crate) struct CopyCodeToMemoryGadget { + /// Offset in the source (bytecode) to read from. + src_addr: Cell, + /// Offset in the destination (memory) to write to. + dst_addr: Cell, + /// Number of bytes left to be copied in this iteration. + bytes_left: Cell, + /// Source (bytecode) bytes end here. + src_addr_end: Cell, + /// Keccak-256 hash of the bytecode. + code_hash: Word, + /// Array of booleans to mark whether or not the byte in question is an + /// opcode byte or an argument that follows the opcode. For example, + /// `is_code = true` for `POP`, `is_code = true` for `PUSH32`, but + /// `is_code = false` for the 32 bytes that follow the `PUSH32` opcode. + is_codes: [Cell; MAX_COPY_BYTES], + /// Gadget to assign bytecode to buffer and read byte-by-byte. + buffer_reader: BufferReaderGadget, + /// Comparison gadget to conditionally stop this iterative internal step + /// once all the bytes have been copied. + finish_gadget: ComparisonGadget, +} + +impl ExecutionGadget for CopyCodeToMemoryGadget { + const NAME: &'static str = "COPYCODETOMEMORY"; + + const EXECUTION_STATE: ExecutionState = ExecutionState::CopyCodeToMemory; + + fn configure(cb: &mut ConstraintBuilder) -> Self { + // Query cells for the internal step's auxiliary data and construct the buffer + // reader. + let src_addr = cb.query_cell(); + let dst_addr = cb.query_cell(); + let bytes_left = cb.query_cell(); + let src_addr_end = cb.query_cell(); + let code_hash = cb.query_word(); + let is_codes = array_init(|_| cb.query_bool()); + let buffer_reader = BufferReaderGadget::construct(cb, src_addr.expr(), src_addr_end.expr()); + + // For every byte in the bytecode's span covered in this iteration. + for (idx, is_code) in is_codes.iter().enumerate() { + // Lookup the bytecode table for the byte value read at the appropriate source + // memory address from the buffer. + cb.condition(buffer_reader.read_flag(idx), |cb| { + cb.bytecode_lookup( + code_hash.expr(), + src_addr.expr() + idx.expr(), + buffer_reader.byte(idx), + is_code.expr(), + ); + }); + // Lookup the RW table for a memory write operation at the appropriate + // destination memory address. + cb.condition(buffer_reader.has_data(idx), |cb| { + cb.memory_lookup( + 1.expr(), + dst_addr.expr() + idx.expr(), + buffer_reader.byte(idx), + None, + ); + }); + } + + // Construct the comparison gadget using the number of bytes copied in this + // iteration and the number bytes that were left to be copied before the + // start of this iteration. + let copied_size = buffer_reader.num_bytes(); + let finish_gadget = ComparisonGadget::construct(cb, copied_size.clone(), bytes_left.expr()); + let (lt, finished) = finish_gadget.expr(); + + // We should have continued only until there were no more bytes left to be + // copied. In case the copied size was less than the number of bytes + // left, the iterative process should not be finished. + cb.add_constraint( + "Constrain num_bytes <= bytes_left", + (1.expr() - lt) * (1.expr() - finished.clone()), + ); + + // If the iterative process has not yet finished, we constrain the next step to + // be another `CopyCodeToMemory` while adding some additional + // constraints to the auxiliary data. + cb.constrain_next_step( + ExecutionState::CopyCodeToMemory, + Some(1.expr() - finished), + |cb| { + let next_src_addr = cb.query_cell(); + let next_dst_addr = cb.query_cell(); + let next_bytes_left = cb.query_cell(); + let next_src_addr_end = cb.query_cell(); + let next_code_hash = cb.query_word(); + + cb.require_equal( + "next_src_addr == src_addr + copied_size", + next_src_addr.expr(), + src_addr.expr() + copied_size.clone(), + ); + cb.require_equal( + "dst_addr + copied_size == next_dst_addr", + next_dst_addr.expr(), + dst_addr.expr() + copied_size.clone(), + ); + cb.require_equal( + "next_bytes_left == bytes_left - copied_size", + next_bytes_left.expr(), + bytes_left.expr() - copied_size.clone(), + ); + cb.require_equal( + "next_src_addr_end == src_addr_end", + next_src_addr_end.expr(), + src_addr_end.expr(), + ); + cb.require_equal( + "next_code_hash == code_hash", + next_code_hash.expr(), + code_hash.expr(), + ); + }, + ); + + // Since this is an internal step for `CODECOPY` opcode, we only increment the + // RW counter. The program counter, stack pointer, and other fields do + // not change. + let step_state_transition = StepStateTransition { + rw_counter: Transition::Delta(cb.rw_counter_offset()), + ..Default::default() + }; + cb.require_step_state_transition(step_state_transition); + + Self { + src_addr, + dst_addr, + bytes_left, + src_addr_end, + code_hash, + is_codes, + buffer_reader, + finish_gadget, + } + } + + fn assign_exec_step( + &self, + region: &mut Region<'_, F>, + offset: usize, + block: &Block, + _tx: &Transaction, + _call: &Call, + step: &ExecStep, + ) -> Result<(), Error> { + // Read the auxiliary data. + let (src_addr, dst_addr, bytes_left, src_addr_end, code, selectors) = + if let StepAuxiliaryData::CopyCodeToMemory { + src_addr, + dst_addr, + bytes_left, + src_addr_end, + code, + selectors, + } = step + .aux_data + .as_ref() + .expect("could not find aux_data for COPYCODETOMEMORY") + { + ( + src_addr, + dst_addr, + bytes_left, + src_addr_end, + code, + selectors, + ) + } else { + panic!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"); + }; + + // Assign to the appropriate cells. + self.src_addr + .assign(region, offset, Some(F::from(*src_addr)))?; + self.dst_addr + .assign(region, offset, Some(F::from(*dst_addr)))?; + self.bytes_left + .assign(region, offset, Some(F::from(*bytes_left)))?; + self.src_addr_end + .assign(region, offset, Some(F::from(*src_addr_end)))?; + self.code_hash + .assign(region, offset, Some(code.hash.to_le_bytes()))?; + + // Initialise selectors and bytes for the buffer reader. + assert_eq!(selectors.len(), MAX_COPY_BYTES); + let mut bytes = vec![0u8; MAX_COPY_BYTES]; + for (idx, (selector, code_it)) in selectors + .iter() + .zip( + code.table_assignments(block.randomness) + .skip(*src_addr as usize) + .take(MAX_COPY_BYTES), + ) + .enumerate() + { + let addr = *src_addr as usize + idx; + bytes[idx] = if *selector == 1 && addr < *src_addr_end as usize { + assert!(addr < code.bytes.len()); + code.bytes[addr] + } else { + 0 + }; + + // The last entry (index = 3) from the table assignments for bytecode represents + // whether or not it is an opcode byte or an opcode argument. + self.is_codes[idx].assign(region, offset, Some(code_it[3]))?; + } + + // Assign the buffer reader. + self.buffer_reader + .assign(region, offset, *src_addr, *src_addr_end, &bytes, selectors)?; + + // The number of bytes copied here will be the sum of 1s over the selector + // vector. + let num_bytes_copied = selectors.iter().sum::() as u64; + + // Assign the comparison gadget. + self.finish_gadget.assign( + region, + offset, + F::from(num_bytes_copied), + F::from(*bytes_left), + )?; + + Ok(()) + } +} + +#[cfg(test)] +pub(crate) mod test { + use super::MAX_COPY_BYTES; + use std::collections::HashMap; + + use bus_mapping::evm::OpcodeId; + use eth_types::{bytecode, Word}; + use halo2_proofs::arithmetic::BaseExt; + use pairing::bn256::Fr; + + use crate::evm_circuit::{ + step::ExecutionState, + table::RwTableTag, + test::run_test_circuit_incomplete_fixed_table, + witness::{ + Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, StepAuxiliaryData, Transaction, + }, + }; + + #[allow(clippy::too_many_arguments)] + pub(crate) fn make_copy_code_step( + call_id: usize, + src_addr: u64, + dst_addr: u64, + src_addr_end: u64, + bytes_left: usize, + program_counter: u64, + stack_pointer: usize, + memory_size: u64, + rw_counter: usize, + rws: &mut RwMap, + bytes_map: &HashMap, + code: &Bytecode, + ) -> (ExecStep, usize) { + let mut selectors = vec![0u8; MAX_COPY_BYTES]; + let mut rw_offset = 0usize; + let memory_rws: &mut Vec<_> = rws.0.entry(RwTableTag::Memory).or_insert_with(Vec::new); + let rw_idx_start = memory_rws.len(); + + for (idx, selector) in selectors.iter_mut().enumerate() { + if idx < bytes_left { + *selector = 1; + let addr = src_addr + idx as u64; + let byte = if addr < src_addr_end { + assert!(bytes_map.contains_key(&addr)); + bytes_map[&addr] + } else { + 0 + }; + memory_rws.push(Rw::Memory { + rw_counter: rw_counter + rw_offset, + is_write: true, + call_id, + memory_address: dst_addr + idx as u64, + byte, + }); + rw_offset += 1; + } + } + + let rw_idx_end = rws.0[&RwTableTag::Memory].len(); + let aux_data = StepAuxiliaryData::CopyCodeToMemory { + src_addr, + dst_addr, + bytes_left: bytes_left as u64, + src_addr_end, + code: code.clone(), + selectors, + }; + let step = ExecStep { + execution_state: ExecutionState::CopyCodeToMemory, + rw_indices: (rw_idx_start..rw_idx_end) + .map(|idx| (RwTableTag::Memory, idx)) + .collect(), + rw_counter, + program_counter, + stack_pointer, + memory_size, + gas_cost: 0, + aux_data: Some(aux_data), + ..Default::default() + }; + + (step, rw_offset) + } + + #[allow(clippy::too_many_arguments)] + pub(crate) fn make_copy_code_steps( + call_id: usize, + code: &Bytecode, + src_addr: u64, + dst_addr: u64, + length: usize, + program_counter: u64, + stack_pointer: usize, + memory_size: u64, + rw_counter: &mut usize, + rws: &mut RwMap, + steps: &mut Vec, + ) { + let bytes_map = (0..(code.bytes.len() as u64)) + .zip(code.bytes.iter().copied()) + .collect(); + + let mut copied = 0; + while copied < length { + let (step, rw_offset) = make_copy_code_step( + call_id, + src_addr + copied as u64, + dst_addr + copied as u64, + code.bytes.len() as u64, + length - copied, + program_counter, + stack_pointer, + memory_size, + *rw_counter, + rws, + &bytes_map, + code, + ); + steps.push(step); + *rw_counter += rw_offset; + copied += MAX_COPY_BYTES; + } + } + + fn test_ok(src_addr: u64, dst_addr: u64, length: usize) { + let randomness = Fr::rand(); + let call_id = 1; + let mut rws = RwMap::default(); + let mut rw_counter = 1; + let mut steps = Vec::new(); + let memory_size = (dst_addr + length as u64 + 31) / 32 * 32; + + // generate random bytecode longer than `src_addr_end` + let code = bytecode! { + #[start] + PUSH32(Word::from(0x123)) + POP + PUSH32(Word::from(0x213)) + POP + PUSH32(Word::from(0x321)) + POP + PUSH32(Word::from(0x12349AB)) + POP + PUSH32(Word::from(0x1928835)) + POP + }; + let code = Bytecode::new(code.to_vec()); + + let dummy_code = Bytecode::new(vec![OpcodeId::STOP.as_u8()]); + + let program_counter = 0; + let stack_pointer = 1024; + make_copy_code_steps( + call_id, + &code, + src_addr, + dst_addr, + length, + program_counter, + stack_pointer, + memory_size, + &mut rw_counter, + &mut rws, + &mut steps, + ); + + steps.push(ExecStep { + execution_state: ExecutionState::STOP, + rw_counter, + program_counter, + stack_pointer, + memory_size, + opcode: Some(OpcodeId::STOP), + ..Default::default() + }); + + let block = Block { + randomness, + txs: vec![Transaction { + id: 1, + calls: vec![Call { + id: call_id, + is_root: true, + is_create: false, + code_source: CodeSource::Account(dummy_code.hash), + ..Default::default() + }], + steps, + ..Default::default() + }], + rws, + bytecodes: vec![dummy_code, code], + ..Default::default() + }; + assert_eq!(run_test_circuit_incomplete_fixed_table(block), Ok(())); + } + + #[test] + fn copy_code_to_memory_single_step() { + test_ok( + 0x00, // src_addr + 0x00, // dst_addr + 54, // length + ); + } + + #[test] + fn copy_code_to_memory_multi_step() { + test_ok( + 0x00, // src_addr + 0x40, // dst_addr + 123, // length + ); + } + + #[test] + fn copy_code_to_memory_oob() { + // since the bytecode we construct above is (34 * 5) = 170 bytes long, copying + // 200 bytes means we go out-of-bounds. + test_ok( + 0x10, // src_addr + 0x20, // dst_addr + 200, // length + ); + } +} diff --git a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs index aba31b6906..c4d8f53ec5 100644 --- a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs @@ -167,13 +167,28 @@ impl ExecutionGadget for CopyToMemoryGadget { _: &Call, step: &ExecStep, ) -> Result<(), Error> { - let StepAuxiliaryData::CopyToMemory { - src_addr, - dst_addr, - bytes_left, - src_addr_end, - from_tx, - } = step.aux_data.as_ref().unwrap(); + let (src_addr, dst_addr, bytes_left, src_addr_end, from_tx) = + if let StepAuxiliaryData::CopyToMemory { + src_addr, + dst_addr, + bytes_left, + src_addr_end, + from_tx, + } = step + .aux_data + .as_ref() + .expect("could not find aux_data for COPYTOMEMORY") + { + ( + src_addr, + dst_addr, + bytes_left, + src_addr_end, + from_tx, + ) + } else { + panic!("could not find CopyToMemory aux_data for COPYTOMEMORY"); + }; self.src_addr .assign(region, offset, Some(F::from(*src_addr)))?; diff --git a/zkevm-circuits/src/evm_circuit/step.rs b/zkevm-circuits/src/evm_circuit/step.rs index b28b3d6b14..0d00e930ac 100644 --- a/zkevm-circuits/src/evm_circuit/step.rs +++ b/zkevm-circuits/src/evm_circuit/step.rs @@ -22,6 +22,7 @@ pub enum ExecutionState { BeginTx, EndTx, EndBlock, + CopyCodeToMemory, CopyToMemory, // Opcode successful cases STOP, @@ -139,6 +140,7 @@ impl ExecutionState { Self::BeginTx, Self::EndTx, Self::EndBlock, + Self::CopyCodeToMemory, Self::CopyToMemory, Self::STOP, Self::ADD_SUB, diff --git a/zkevm-circuits/src/evm_circuit/table.rs b/zkevm-circuits/src/evm_circuit/table.rs index 3d7aed8366..0315d0ec21 100644 --- a/zkevm-circuits/src/evm_circuit/table.rs +++ b/zkevm-circuits/src/evm_circuit/table.rs @@ -175,6 +175,7 @@ pub enum AccountFieldTag { Nonce = 1, Balance, CodeHash, + CodeSize, } #[derive(Clone, Copy, Debug, PartialEq)] diff --git a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs index 0153506161..ba0b9f8b40 100644 --- a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs +++ b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs @@ -554,6 +554,26 @@ impl<'a, F: FieldExt> ConstraintBuilder<'a, F> { ); } + // Bytecode table + + pub(crate) fn bytecode_lookup( + &mut self, + code_hash: Expression, + index: Expression, + value: Expression, + is_code: Expression, + ) { + self.add_lookup( + "Bytecode lookup", + Lookup::Bytecode { + hash: code_hash, + index, + value, + is_code, + }, + ) + } + // Tx context pub(crate) fn tx_context( diff --git a/zkevm-circuits/src/evm_circuit/witness.rs b/zkevm-circuits/src/evm_circuit/witness.rs index dab8c35111..d3847cb57d 100644 --- a/zkevm-circuits/src/evm_circuit/witness.rs +++ b/zkevm-circuits/src/evm_circuit/witness.rs @@ -319,7 +319,7 @@ impl ExecStep { } } -#[derive(Debug, Clone)] +#[derive(Clone, Debug)] pub struct Bytecode { pub hash: Word, pub bytes: Vec, @@ -697,7 +697,9 @@ impl Rw { value_prev, } => { let to_scalar = |value: &Word| match field_tag { - AccountFieldTag::Nonce => value.to_scalar().unwrap(), + AccountFieldTag::Nonce | AccountFieldTag::CodeSize => { + value.to_scalar().unwrap() + } _ => RandomLinearCombination::random_linear_combine( value.to_le_bytes(), randomness, From 7ae5d95f4c652f6c5bd10c271be5f6f61e7be6af Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Wed, 16 Mar 2022 13:13:22 +0800 Subject: [PATCH 02/12] fix: constraints for aux data --- zkevm-circuits/src/evm_circuit/execution/codecopy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index 110cf801bd..2f3acc4f9d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -62,7 +62,7 @@ impl ExecutionGadget for CodeCopyGadget { cb.stack_pop(size.expr()); // Construct memory address in the destionation (memory) to which we copy code. - let dst_memory_addr = MemoryAddressGadget::construct(cb, dest_memory_offset, size); + let dst_memory_addr = MemoryAddressGadget::construct(cb, dest_memory_offset, size.clone()); // Query additional fields for the account's code. let account = cb.call_context(None, CallContextFieldTag::CalleeAddress); @@ -112,7 +112,7 @@ impl ExecutionGadget for CodeCopyGadget { cb.require_equal( "next_bytes_left = length", next_bytes_left.expr(), - dst_memory_addr.length(), + size.expr(), ); cb.require_equal( "next_src_addr_end == code_size", From 6d4c28339e54973970dc6324f9eda369ff7e756f Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Mon, 21 Mar 2022 12:18:18 +0800 Subject: [PATCH 03/12] feat: update bytecode circuit as per updated spec (failing) --- .../src/bytecode_circuit/bytecode_unroller.rs | 226 +++++++++++++----- zkevm-circuits/src/evm_circuit/table.rs | 7 + zkevm-circuits/src/evm_circuit/witness.rs | 23 +- 3 files changed, 192 insertions(+), 64 deletions(-) diff --git a/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs b/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs index 94483c6728..92901fb428 100644 --- a/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs +++ b/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs @@ -1,6 +1,10 @@ use crate::{ - evm_circuit::util::{ - and, constraint_builder::BaseConstraintBuilder, not, or, select, RandomLinearCombination, + evm_circuit::{ + table::BytecodeFieldTag, + util::{ + and, constraint_builder::BaseConstraintBuilder, not, or, select, + RandomLinearCombination, + }, }, gadget::{ evm_word::encode, @@ -12,7 +16,7 @@ use bus_mapping::evm::OpcodeId; use eth_types::Field; use halo2_proofs::{ circuit::{Layouter, Region}, - plonk::{Advice, Column, ConstraintSystem, Error, Fixed, Selector, VirtualCells}, + plonk::{Advice, Column, ConstraintSystem, Error, Expression, Fixed, Selector, VirtualCells}, poly::Rotation, }; use keccak256::plain::Keccak; @@ -24,9 +28,10 @@ use super::param::{KECCAK_WIDTH, PUSH_TABLE_WIDTH}; #[derive(Clone, Debug, PartialEq)] pub(crate) struct BytecodeRow { hash: F, + tag: F, index: F, is_code: F, - byte: F, + value: F, } /// Unrolled bytecode @@ -44,9 +49,10 @@ pub struct Config { q_first: Column, q_last: Selector, hash: Column, + tag: Column, index: Column, is_code: Column, - byte: Column, + value: Column, push_rindex: Column, hash_rlc: Column, hash_length: Column, @@ -55,6 +61,8 @@ pub struct Config { padding: Column, push_rindex_inv: Column, push_rindex_is_zero: IsZeroConfig, + length_inv: Column, + length_is_zero: IsZeroConfig, push_table: [Column; PUSH_TABLE_WIDTH], keccak_table: [Column; KECCAK_WIDTH], } @@ -65,9 +73,10 @@ impl Config { let q_first = meta.fixed_column(); let q_last = meta.selector(); let hash = meta.advice_column(); + let tag = meta.advice_column(); let index = meta.advice_column(); let is_code = meta.advice_column(); - let byte = meta.advice_column(); + let value = meta.advice_column(); let push_rindex = meta.advice_column(); let hash_rlc = meta.advice_column(); let hash_length = meta.advice_column(); @@ -75,6 +84,7 @@ impl Config { let is_final = meta.advice_column(); let padding = meta.advice_column(); let push_rindex_inv = meta.advice_column(); + let length_inv = meta.advice_column(); let push_table = array_init::array_init(|_| meta.fixed_column()); let keccak_table = array_init::array_init(|_| meta.advice_column()); @@ -92,6 +102,39 @@ impl Config { push_rindex_inv, ); + // Did the previous row have bytecode field tag == Length? + let is_prev_row_tag_length = |meta: &mut VirtualCells| { + and::expr(vec![ + not::expr(meta.query_fixed(q_first, Rotation::cur())), + { + let prev_row_tag = meta.query_advice(tag, Rotation::prev()); + prev_row_tag.expr() * (BytecodeFieldTag::Byte.expr() - prev_row_tag.expr()) + }, + ]) + }; + + // Does the current row have bytecode field tag == Length? + let is_row_tag_length = |meta: &mut VirtualCells| { + let row_tag = meta.query_advice(tag, Rotation::cur()); + row_tag.expr() * (BytecodeFieldTag::Byte.expr() - row_tag.expr()) + }; + + // Does the current row have bytecode field tag == Byte? + let is_row_tag_byte = |meta: &mut VirtualCells| { + let row_tag = meta.query_advice(tag, Rotation::cur()); + row_tag.expr() + * (row_tag.expr() - BytecodeFieldTag::Length.expr()) + * F::from(2).invert().unwrap() + }; + + // For a row tagged Length, is the length (value) zero? + let length_is_zero = IsZeroChip::configure( + meta, + |meta| meta.query_selector(q_enable), + |meta| meta.query_advice(value, Rotation::cur()), + length_inv, + ); + let q_continue = |meta: &mut VirtualCells| { // When // - Not on the first row @@ -105,22 +148,29 @@ impl Config { meta.create_gate("continue", |meta| { let mut cb = BaseConstraintBuilder::default(); cb.require_equal( - "index needs to increase by 1", + "if prev_row_tag == Length: index == 0 else index == index + 1", meta.query_advice(index, Rotation::cur()), - meta.query_advice(index, Rotation::prev()) + 1.expr(), + select::expr( + is_prev_row_tag_length(meta), + 0.expr(), + meta.query_advice(index, Rotation::prev()) + 1.expr(), + ), ); cb.require_equal( "is_code := push_rindex_prev == 0", meta.query_advice(is_code, Rotation::cur()), - push_rindex_is_zero.clone().is_zero_expression, + select::expr( + is_prev_row_tag_length(meta), + 1.expr(), + push_rindex_is_zero.clone().is_zero_expression, + ), ); cb.require_equal( "hash_rlc := hash_rlc_prev * r + byte", meta.query_advice(hash_rlc, Rotation::cur()), meta.query_advice(hash_rlc, Rotation::prev()) * r - + meta.query_advice(byte, Rotation::cur()), + + meta.query_advice(value, Rotation::cur()), ); - cb.require_equal( "hash needs to remain the same", meta.query_advice(hash, Rotation::cur()), @@ -145,26 +195,46 @@ impl Config { ])) }); - meta.create_gate("start", |meta| { + meta.create_gate("start of bytecode", |meta| { let mut cb = BaseConstraintBuilder::default(); - cb.require_zero( - "index needs to start at 0", - meta.query_advice(index, Rotation::cur()), - ); cb.require_equal( - "is_code needs to be 1 (first byte is always an opcode)", - 1.expr(), - meta.query_advice(is_code, Rotation::cur()), + "next row is tagged Length if length of current bytecode == 0", + meta.query_advice(tag, Rotation::next()), + select::expr( + length_is_zero.clone().is_zero_expression, + BytecodeFieldTag::Byte.expr(), + BytecodeFieldTag::Length.expr(), + ), ); - cb.require_equal( - "hash_rlc needs to start at byte", - meta.query_advice(byte, Rotation::cur()), - meta.query_advice(hash_rlc, Rotation::cur()), + cb.condition(length_is_zero.clone().is_zero_expression, |cb| { + cb.require_equal( + "if length == 0: hash == EMPTY_HASH", + meta.query_advice(hash, Rotation::cur()), + Expression::Constant(keccak(&[], r)), + ); + }); + // Conditions: + // - Not Continuing + // - This is the start of a new bytecode + cb.gate(and::expr(vec![ + meta.query_selector(q_enable), + is_row_tag_length(meta), + not::expr(q_continue(meta)), + ])) + }); + + meta.create_gate("start of padding", |meta| { + let mut cb = BaseConstraintBuilder::default(); + cb.require_zero( + "row needs to be marked as padding row", + meta.query_advice(padding, Rotation::cur()), ); // Conditions: - // - Not continuing + // - Not Continuing + // - This is not the start of a new bytecode cb.gate(and::expr(vec![ meta.query_selector(q_enable), + not::expr(is_row_tag_length(meta)), not::expr(q_continue(meta)), ])) }); @@ -196,15 +266,17 @@ impl Config { "padding needs to be boolean", meta.query_advice(padding, Rotation::cur()), ); - cb.require_equal( - "push_rindex := is_code ? byte_push_size : push_rindex_prev - 1", - meta.query_advice(push_rindex, Rotation::cur()), - select::expr( - meta.query_advice(is_code, Rotation::cur()), - meta.query_advice(byte_push_size, Rotation::cur()), - meta.query_advice(push_rindex, Rotation::prev()) - 1.expr(), - ), - ); + cb.condition(is_row_tag_byte(meta), |cb| { + cb.require_equal( + "push_rindex := is_code ? byte_push_size : push_rindex_prev - 1", + meta.query_advice(push_rindex, Rotation::cur()), + select::expr( + meta.query_advice(is_code, Rotation::cur()), + meta.query_advice(byte_push_size, Rotation::cur()), + meta.query_advice(push_rindex, Rotation::prev()) - 1.expr(), + ), + ); + }); // Conditions: Always cb.gate(meta.query_selector(q_enable)) }); @@ -246,9 +318,9 @@ impl Config { // Lookup how many bytes the current opcode pushes // (also indirectly range checks `byte` to be in [0, 255]) meta.lookup_any("Range bytes", |meta| { - // Conditions: Always - let q_enable = meta.query_selector(q_enable); - let lookup_columns = vec![byte, byte_push_size]; + // Conditions: If the current row is is tagged Byte + let q_enable = meta.query_selector(q_enable) * is_row_tag_byte(meta); + let lookup_columns = vec![value, byte_push_size]; let mut constraints = vec![]; for i in 0..PUSH_TABLE_WIDTH { constraints.push(( @@ -286,9 +358,10 @@ impl Config { q_first, q_last, hash, + tag, index, is_code, - byte, + value, push_rindex, hash_rlc, hash_length, @@ -297,6 +370,8 @@ impl Config { padding, push_rindex_inv, push_rindex_is_zero, + length_inv, + length_is_zero, push_table, keccak_table, } @@ -309,6 +384,7 @@ impl Config { witness: &[UnrolledBytecode], ) { let push_rindex_is_zero_chip = IsZeroChip::construct(self.push_rindex_is_zero.clone()); + let length_is_zero_chip = IsZeroChip::construct(self.length_is_zero.clone()); // Subtract the unusable rows from the size let last_row_offset = size - self.minimum_rows + 1; @@ -323,38 +399,41 @@ impl Config { for bytecode in witness.iter() { // Run over all the bytes let mut push_rindex = 0; + let mut byte_push_size = 0; let mut hash_rlc = F::zero(); let hash_length = F::from(bytecode.bytes.len() as u64); - for row in bytecode.rows.iter() { + for (idx, row) in bytecode.rows.iter().enumerate() { // Track which byte is an opcode and which is push // data let is_code = push_rindex == 0; - let byte_push_size = get_push_size(row.byte.get_lower_128() as u8); - push_rindex = if is_code { - byte_push_size - } else { - push_rindex - 1 - }; - - // Add the byte to the accumulator - hash_rlc = hash_rlc * self.r + row.byte; + if idx > 0 { + byte_push_size = get_push_size(row.value.get_lower_128() as u8); + push_rindex = if is_code { + byte_push_size + } else { + push_rindex - 1 + }; + hash_rlc = hash_rlc * self.r + row.value; + } // Set the data for this row self.set_row( &mut region, &push_rindex_is_zero_chip, + &length_is_zero_chip, offset, true, offset == last_row_offset, row.hash, + row.tag, row.index, row.is_code, - row.byte, + row.value, push_rindex, hash_rlc, hash_length, F::from(byte_push_size as u64), - row.index + F::one() == hash_length, + row.index == hash_length, false, F::from(push_rindex_prev), )?; @@ -368,11 +447,13 @@ impl Config { self.set_row( &mut region, &push_rindex_is_zero_chip, + &length_is_zero_chip, idx, idx < size, idx == last_row_offset, F::zero(), F::zero(), + F::zero(), F::one(), F::zero(), 0, @@ -397,13 +478,15 @@ impl Config { &self, region: &mut Region<'_, F>, push_rindex_is_zero_chip: &IsZeroChip, + length_is_zero_chip: &IsZeroChip, offset: usize, enable: bool, last: bool, hash: F, + tag: F, index: F, is_code: F, - byte: F, + value: F, push_rindex: u64, hash_rlc: F, hash_length: F, @@ -433,9 +516,10 @@ impl Config { // Advices for (name, column, value) in &[ ("hash", self.hash, hash), + ("tag", self.tag, tag), ("index", self.index, index), ("is_code", self.is_code, is_code), - ("byte", self.byte, byte), + ("value", self.value, value), ("push_rindex", self.push_rindex, F::from(push_rindex)), ("hash_rlc", self.hash_rlc, hash_rlc), ("hash_length", self.hash_length, hash_length), @@ -454,6 +538,9 @@ impl Config { // push_rindex_is_zero_chip push_rindex_is_zero_chip.assign(region, offset, Some(push_rindex_prev))?; + // length_is_zero chip + length_is_zero_chip.assign(region, offset, Some(hash_length))?; + Ok(()) } @@ -517,7 +604,13 @@ impl Config { fn unroll(bytes: Vec, r: F) -> UnrolledBytecode { let hash = keccak(&bytes[..], r); - let mut rows = vec![]; + let mut rows = vec![BytecodeRow:: { + hash, + tag: F::from(BytecodeFieldTag::Length as u64), + index: F::zero(), + is_code: F::zero(), + value: F::from(bytes.len() as u64), + }]; // Run over all the bytes let mut push_rindex = 0; for (index, byte) in bytes.iter().enumerate() { @@ -531,9 +624,10 @@ fn unroll(bytes: Vec, r: F) -> UnrolledBytecode { rows.push(BytecodeRow:: { hash, + tag: F::from(BytecodeFieldTag::Byte as u64), index: F::from(index as u64), is_code: F::from(is_code as u64), - byte: F::from(*byte as u64), + value: F::from(*byte as u64), }); } UnrolledBytecode { bytes, rows } @@ -653,9 +747,10 @@ mod tests { bytecode.write(byte); rows.push(BytecodeRow { hash: Fr::zero(), + tag: Fr::from(BytecodeFieldTag::Byte as u64), index: Fr::from(rows.len() as u64), is_code: Fr::from(true as u64), - byte: Fr::from(byte as u64), + value: Fr::from(byte as u64), }); } } @@ -665,16 +760,18 @@ mod tests { bytecode.push(n, Word::from_little_endian(&vec![data_byte; n][..])); rows.push(BytecodeRow { hash: Fr::zero(), + tag: Fr::from(BytecodeFieldTag::Byte as u64), index: Fr::from(rows.len() as u64), is_code: Fr::from(true as u64), - byte: Fr::from(OpcodeId::PUSH1.as_u64() + ((n - 1) as u64)), + value: Fr::from(OpcodeId::PUSH1.as_u64() + ((n - 1) as u64)), }); for _ in 0..n { rows.push(BytecodeRow { hash: Fr::zero(), + tag: Fr::from(BytecodeFieldTag::Byte as u64), index: Fr::from(rows.len() as u64), is_code: Fr::from(false as u64), - byte: Fr::from(data_byte as u64), + value: Fr::from(data_byte as u64), }); } } @@ -683,6 +780,16 @@ mod tests { for row in rows.iter_mut() { row.hash = hash; } + rows.insert( + 0, + BytecodeRow { + hash, + tag: Fr::from(BytecodeFieldTag::Length as u64), + index: Fr::zero(), + is_code: Fr::zero(), + value: Fr::from(bytecode.to_vec().len() as u64), + }, + ); // Unroll the bytecode let unrolled = unroll(bytecode.to_vec(), r); // Check if the bytecode was unrolled correctly @@ -693,6 +800,7 @@ mod tests { }, unrolled, ); + println!("reached here"); // Verify the unrolling in the circuit verify::(k, vec![unrolled], true); } @@ -812,19 +920,19 @@ mod tests { // Change the first byte { let mut invalid = unrolled.clone(); - invalid.rows[0].byte = Fr::from(9u64); + invalid.rows[0].value = Fr::from(9u64); verify::(k, vec![invalid], false); } // Change a byte on another position { let mut invalid = unrolled.clone(); - invalid.rows[5].byte = Fr::from(6u64); + invalid.rows[5].value = Fr::from(6u64); verify::(k, vec![invalid], false); } // Set a byte value out of range { let mut invalid = unrolled; - invalid.rows[3].byte = Fr::from(256u64); + invalid.rows[3].value = Fr::from(256u64); verify::(k, vec![invalid], false); } } diff --git a/zkevm-circuits/src/evm_circuit/table.rs b/zkevm-circuits/src/evm_circuit/table.rs index 0315d0ec21..28a1d357b0 100644 --- a/zkevm-circuits/src/evm_circuit/table.rs +++ b/zkevm-circuits/src/evm_circuit/table.rs @@ -178,6 +178,12 @@ pub enum AccountFieldTag { CodeSize, } +#[derive(Clone, Copy, Debug)] +pub enum BytecodeFieldTag { + Length = 1, + Byte, +} + #[derive(Clone, Copy, Debug, PartialEq)] pub enum CallContextFieldTag { RwCounterEndOfReversion = 1, @@ -213,6 +219,7 @@ impl_expr!(FixedTableTag); impl_expr!(TxContextFieldTag); impl_expr!(RwTableTag); impl_expr!(AccountFieldTag); +impl_expr!(BytecodeFieldTag); impl_expr!(CallContextFieldTag); impl_expr!(BlockContextFieldTag); diff --git a/zkevm-circuits/src/evm_circuit/witness.rs b/zkevm-circuits/src/evm_circuit/witness.rs index d3847cb57d..bb57bee678 100644 --- a/zkevm-circuits/src/evm_circuit/witness.rs +++ b/zkevm-circuits/src/evm_circuit/witness.rs @@ -3,7 +3,8 @@ use crate::evm_circuit::{ param::{N_BYTES_WORD, STACK_CAPACITY}, step::ExecutionState, table::{ - AccountFieldTag, BlockContextFieldTag, CallContextFieldTag, RwTableTag, TxContextFieldTag, + AccountFieldTag, BlockContextFieldTag, BytecodeFieldTag, CallContextFieldTag, RwTableTag, + TxContextFieldTag, }, util::RandomLinearCombination, }; @@ -334,7 +335,7 @@ impl Bytecode { pub fn table_assignments<'a, F: FieldExt>( &'a self, randomness: F, - ) -> impl Iterator + '_ { + ) -> impl Iterator + '_ { struct BytecodeIterator<'a, F> { idx: usize, push_data_left: usize, @@ -343,10 +344,21 @@ impl Bytecode { } impl<'a, F: FieldExt> Iterator for BytecodeIterator<'a, F> { - type Item = [F; 4]; + type Item = [F; 5]; fn next(&mut self) -> Option { - if self.idx == self.bytes.len() { + if self.idx == 0 { + self.idx += 1; + return Some([ + self.hash, + F::from(BytecodeFieldTag::Length as u64), + F::from(0), + F::from(0), + F::from(self.bytes.len() as u64), + ]); + } + + if self.idx > self.bytes.len() { return None; } @@ -365,9 +377,10 @@ impl Bytecode { Some([ self.hash, + F::from(BytecodeFieldTag::Byte as u64), F::from(idx as u64), - F::from(byte as u64), F::from(is_code as u64), + F::from(byte as u64), ]) } } From d46451a8d647cad704b9d875c20802549c32df1f Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Mon, 21 Mar 2022 14:36:02 +0800 Subject: [PATCH 04/12] fix: constraints for updated bytecode circuit --- .../src/bytecode_circuit/bytecode_unroller.rs | 87 ++++++++++++------- zkevm-circuits/src/evm_circuit/witness.rs | 5 +- 2 files changed, 57 insertions(+), 35 deletions(-) diff --git a/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs b/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs index 92901fb428..fcdba0755a 100644 --- a/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs +++ b/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs @@ -102,17 +102,6 @@ impl Config { push_rindex_inv, ); - // Did the previous row have bytecode field tag == Length? - let is_prev_row_tag_length = |meta: &mut VirtualCells| { - and::expr(vec![ - not::expr(meta.query_fixed(q_first, Rotation::cur())), - { - let prev_row_tag = meta.query_advice(tag, Rotation::prev()); - prev_row_tag.expr() * (BytecodeFieldTag::Byte.expr() - prev_row_tag.expr()) - }, - ]) - }; - // Does the current row have bytecode field tag == Length? let is_row_tag_length = |meta: &mut VirtualCells| { let row_tag = meta.query_advice(tag, Rotation::cur()); @@ -130,7 +119,7 @@ impl Config { // For a row tagged Length, is the length (value) zero? let length_is_zero = IsZeroChip::configure( meta, - |meta| meta.query_selector(q_enable), + |meta| meta.query_selector(q_enable) * is_row_tag_length(meta), |meta| meta.query_advice(value, Rotation::cur()), length_inv, ); @@ -147,8 +136,20 @@ impl Config { meta.create_gate("continue", |meta| { let mut cb = BaseConstraintBuilder::default(); + + // Did the previous row have bytecode field tag == Length? + let is_prev_row_tag_length = |meta: &mut VirtualCells| { + and::expr(vec![ + not::expr(meta.query_fixed(q_first, Rotation::cur())), + { + let prev_row_tag = meta.query_advice(tag, Rotation::prev()); + prev_row_tag.expr() * (BytecodeFieldTag::Byte.expr() - prev_row_tag.expr()) + }, + ]) + }; + cb.require_equal( - "if prev_row_tag == Length: index == 0 else index == index + 1", + "if prev_row.tag == Length: index == 0 else index == index + 1", meta.query_advice(index, Rotation::cur()), select::expr( is_prev_row_tag_length(meta), @@ -198,17 +199,26 @@ impl Config { meta.create_gate("start of bytecode", |meta| { let mut cb = BaseConstraintBuilder::default(); cb.require_equal( - "next row is tagged Length if length of current bytecode == 0", + "next_row.tag == (tag.Length or tag.Padding) if length == 0 else tag.Byte", meta.query_advice(tag, Rotation::next()), select::expr( length_is_zero.clone().is_zero_expression, + select::expr( + meta.query_advice(padding, Rotation::next()), + 0.expr(), // Padding is 0 + BytecodeFieldTag::Length.expr(), + ), BytecodeFieldTag::Byte.expr(), - BytecodeFieldTag::Length.expr(), ), ); + cb.require_equal( + "if row.tag == tag.Length: value == row.hash_length", + meta.query_advice(value, Rotation::cur()), + meta.query_advice(hash_length, Rotation::cur()), + ); cb.condition(length_is_zero.clone().is_zero_expression, |cb| { cb.require_equal( - "if length == 0: hash == EMPTY_HASH", + "if length == 0: hash == RLC(EMPTY_HASH, randomness)", meta.query_advice(hash, Rotation::cur()), Expression::Constant(keccak(&[], r)), ); @@ -225,9 +235,10 @@ impl Config { meta.create_gate("start of padding", |meta| { let mut cb = BaseConstraintBuilder::default(); - cb.require_zero( - "row needs to be marked as padding row", + cb.require_equal( + "row needs to be marked as padding", meta.query_advice(padding, Rotation::cur()), + 1.expr(), ); // Conditions: // - Not Continuing @@ -241,13 +252,16 @@ impl Config { meta.create_gate("length needs to be correct", |meta| { let mut cb = BaseConstraintBuilder::default(); - cb.require_equal( - "index + 1 needs to equal hash_length", - meta.query_advice(index, Rotation::cur()) + 1.expr(), - meta.query_advice(hash_length, Rotation::cur()), - ); + cb.condition(1.expr() - length_is_zero.clone().is_zero_expression, |cb| { + cb.require_equal( + "index + 1 needs to equal hash_length", + meta.query_advice(index, Rotation::cur()) + 1.expr(), + meta.query_advice(hash_length, Rotation::cur()), + ); + }); // Conditions: // - On the row with the last byte (`is_final == 1`) + // - Of bytecode with length > 0 // - Not padding cb.gate(and::expr(vec![ meta.query_selector(q_enable), @@ -433,7 +447,7 @@ impl Config { hash_rlc, hash_length, F::from(byte_push_size as u64), - row.index == hash_length, + idx == bytecode.bytes.len(), false, F::from(push_rindex_prev), )?; @@ -458,7 +472,7 @@ impl Config { F::zero(), 0, F::zero(), - F::one(), + F::zero(), F::zero(), true, true, @@ -800,7 +814,6 @@ mod tests { }, unrolled, ); - println!("reached here"); // Verify the unrolling in the circuit verify::(k, vec![unrolled], true); } @@ -813,12 +826,24 @@ mod tests { verify::(k, vec![unroll(vec![], r)], true); } + #[test] + fn bytecode_simple() { + let k = 9; + let r = MyCircuit::r(); + let bytecodes = vec![ + unroll(vec![7u8], r), + unroll(vec![6u8], r), + unroll(vec![5u8], r), + ]; + verify::(k, bytecodes, true); + } + /// Tests a fully full circuit #[test] fn bytecode_full() { let k = 9; let r = MyCircuit::r(); - verify::(k, vec![unroll(vec![7u8; 2usize.pow(k) - 6], r)], true); + verify::(k, vec![unroll(vec![7u8; 2usize.pow(k) - 7], r)], true); } /// Tests a circuit with incomplete bytecode @@ -920,7 +945,7 @@ mod tests { // Change the first byte { let mut invalid = unrolled.clone(); - invalid.rows[0].value = Fr::from(9u64); + invalid.rows[1].value = Fr::from(9u64); verify::(k, vec![invalid], false); } // Change a byte on another position @@ -956,19 +981,19 @@ mod tests { // Mark the 3rd byte as code (is push data from the first PUSH1) { let mut invalid = unrolled.clone(); - invalid.rows[2].is_code = Fr::one(); + invalid.rows[3].is_code = Fr::one(); verify::(k, vec![invalid], false); } // Mark the 4rd byte as data (is code) { let mut invalid = unrolled.clone(); - invalid.rows[3].is_code = Fr::zero(); + invalid.rows[4].is_code = Fr::zero(); verify::(k, vec![invalid], false); } // Mark the 7th byte as code (is data for the PUSH7) { let mut invalid = unrolled; - invalid.rows[6].is_code = Fr::one(); + invalid.rows[7].is_code = Fr::one(); verify::(k, vec![invalid], false); } } diff --git a/zkevm-circuits/src/evm_circuit/witness.rs b/zkevm-circuits/src/evm_circuit/witness.rs index bb57bee678..b192f8dd21 100644 --- a/zkevm-circuits/src/evm_circuit/witness.rs +++ b/zkevm-circuits/src/evm_circuit/witness.rs @@ -362,19 +362,16 @@ impl Bytecode { return None; } - let idx = self.idx; + let idx = self.idx - 1; let byte = self.bytes[self.idx]; let mut is_code = true; - if self.push_data_left > 0 { is_code = false; self.push_data_left -= 1; } else if (OpcodeId::PUSH1.as_u8()..=OpcodeId::PUSH32.as_u8()).contains(&byte) { self.push_data_left = byte as usize - (OpcodeId::PUSH1.as_u8() - 1) as usize; } - self.idx += 1; - Some([ self.hash, F::from(BytecodeFieldTag::Byte as u64), From ef2b62215db9f1a0839abe486fcedbbb8fe92149 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Mon, 21 Mar 2022 15:00:03 +0800 Subject: [PATCH 05/12] feat: update cb and codecopy as per bytecode circuit updates --- .../src/evm_circuit/execution/codecopy.rs | 22 ++++---------- .../execution/copy_code_to_memory.rs | 5 ++-- zkevm-circuits/src/evm_circuit/table.rs | 18 +++++++++--- .../evm_circuit/util/constraint_builder.rs | 29 +++++++++++++++---- zkevm-circuits/src/evm_circuit/witness.rs | 2 +- 5 files changed, 47 insertions(+), 29 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index 2f3acc4f9d..d8de423083 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -66,13 +66,14 @@ impl ExecutionGadget for CodeCopyGadget { // Query additional fields for the account's code. let account = cb.call_context(None, CallContextFieldTag::CalleeAddress); - let code_size = cb.query_cell(); let code_hash = cb.curr.state.code_source.clone(); - // Lookup the code hash and code size of the current environment's account. - cb.account_read(account.expr(), AccountFieldTag::CodeSize, code_size.expr()); + // Lookup the code hash of the current environment's account. cb.account_read(account.expr(), AccountFieldTag::CodeHash, code_hash.expr()); + // Fetch the bytecode length from the bytecode table. + let code_size = cb.bytecode_length(code_hash.expr()); + // Calculate the next memory size and the gas cost for this memory // access. This also accounts for the dynamic gas required to copy bytes to // memory. @@ -187,7 +188,6 @@ impl ExecutionGadget for CodeCopyGadget { } else { unimplemented!("CODECOPY does not support internal calls yet"); }; - println!("account = {:?}", account); self.account.assign(region, offset, account.to_scalar())?; let code = block @@ -324,18 +324,9 @@ mod tests { ( RwTableTag::Account, vec![ - // Code size lookup in CODECOPY. - Rw::Account { - rw_counter: 8, - is_write: false, - account_address: callee_addr, - field_tag: AccountFieldTag::CodeSize, - value: Word::from(code.bytes.len()), - value_prev: Word::from(code.bytes.len()), - }, // Code hash lookup in CODECOPY. Rw::Account { - rw_counter: 9, + rw_counter: 8, is_write: false, account_address: callee_addr, field_tag: AccountFieldTag::CodeHash, @@ -401,7 +392,6 @@ mod tests { (RwTableTag::Stack, 5), (RwTableTag::CallContext, 0), (RwTableTag::Account, 0), - (RwTableTag::Account, 1), ], execution_state: ExecutionState::CODECOPY, rw_counter: 4, @@ -416,7 +406,7 @@ mod tests { let program_counter = 100; let stack_pointer = 1024; - let mut rw_counter = 10; + let mut rw_counter = 9; if !size.is_zero() { make_copy_code_steps( call_id, diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index 0d28312fc3..3daf4d8eeb 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -74,8 +74,8 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { cb.bytecode_lookup( code_hash.expr(), src_addr.expr() + idx.expr(), - buffer_reader.byte(idx), is_code.expr(), + buffer_reader.byte(idx), ); }); // Lookup the RW table for a memory write operation at the appropriate @@ -221,7 +221,8 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .iter() .zip( code.table_assignments(block.randomness) - .skip(*src_addr as usize) + // add 1 since the first row is reserved for bytecode length. + .skip(*src_addr as usize + 1) .take(MAX_COPY_BYTES), ) .enumerate() diff --git a/zkevm-circuits/src/evm_circuit/table.rs b/zkevm-circuits/src/evm_circuit/table.rs index 28a1d357b0..a076e47c16 100644 --- a/zkevm-circuits/src/evm_circuit/table.rs +++ b/zkevm-circuits/src/evm_circuit/table.rs @@ -273,13 +273,16 @@ pub(crate) enum Lookup { Bytecode { /// Hash to specify which code to read. hash: Expression, + /// Tag to specify whether its the bytecode length or byte value in the + /// bytecode. + tag: Expression, /// Index to specify which byte of bytecode. index: Expression, - /// Value of the index. - value: Expression, /// A boolean value to specify if the value is executable opcode or the /// data portion of PUSH* operations. is_code: Expression, + /// Value of the index. + value: Expression, }, /// Lookup to block table, which contains constants of this block. Block { @@ -332,11 +335,18 @@ impl Lookup { .concat(), Self::Bytecode { hash, + tag, index, - value, is_code, + value, } => { - vec![hash.clone(), index.clone(), value.clone(), is_code.clone()] + vec![ + hash.clone(), + tag.clone(), + index.clone(), + is_code.clone(), + value.clone(), + ] } Self::Block { field_tag, diff --git a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs index ba0b9f8b40..905a0cee25 100644 --- a/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs +++ b/zkevm-circuits/src/evm_circuit/util/constraint_builder.rs @@ -3,8 +3,8 @@ use crate::{ param::STACK_CAPACITY, step::{ExecutionState, Preset, Step}, table::{ - AccountFieldTag, CallContextFieldTag, FixedTableTag, Lookup, RwTableTag, - TxContextFieldTag, + AccountFieldTag, BytecodeFieldTag, CallContextFieldTag, FixedTableTag, Lookup, + RwTableTag, TxContextFieldTag, }, util::{Cell, RandomLinearCombination, Word}, }, @@ -546,9 +546,10 @@ impl<'a, F: FieldExt> ConstraintBuilder<'a, F> { "Opcode lookup", Lookup::Bytecode { hash: self.curr.state.code_source.expr(), + tag: BytecodeFieldTag::Byte.expr(), index, - value: opcode, is_code, + value: opcode, } .conditional(1.expr() - is_root_create), ); @@ -560,20 +561,36 @@ impl<'a, F: FieldExt> ConstraintBuilder<'a, F> { &mut self, code_hash: Expression, index: Expression, - value: Expression, is_code: Expression, + value: Expression, ) { self.add_lookup( - "Bytecode lookup", + "Bytecode (byte) lookup", Lookup::Bytecode { hash: code_hash, + tag: BytecodeFieldTag::Byte.expr(), index, - value, is_code, + value, }, ) } + pub(crate) fn bytecode_length(&mut self, code_hash: Expression) -> Cell { + let cell = self.query_cell(); + self.add_lookup( + "Bytecode (length)", + Lookup::Bytecode { + hash: code_hash, + tag: BytecodeFieldTag::Length.expr(), + index: 0.expr(), + is_code: 0.expr(), + value: cell.expr(), + }, + ); + cell + } + // Tx context pub(crate) fn tx_context( diff --git a/zkevm-circuits/src/evm_circuit/witness.rs b/zkevm-circuits/src/evm_circuit/witness.rs index b192f8dd21..7acddb39fd 100644 --- a/zkevm-circuits/src/evm_circuit/witness.rs +++ b/zkevm-circuits/src/evm_circuit/witness.rs @@ -363,7 +363,7 @@ impl Bytecode { } let idx = self.idx - 1; - let byte = self.bytes[self.idx]; + let byte = self.bytes[idx]; let mut is_code = true; if self.push_data_left > 0 { is_code = false; From 9896a77e877c269bfde458034d038c24fe391856 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Tue, 22 Mar 2022 00:36:38 +0800 Subject: [PATCH 06/12] fix: use current instruction's code source --- .../src/evm_circuit/execution/codecopy.rs | 180 +++++++----------- .../execution/copy_code_to_memory.rs | 20 +- 2 files changed, 76 insertions(+), 124 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index d8de423083..f7a2ac49ce 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -1,14 +1,13 @@ use std::convert::TryInto; use bus_mapping::evm::OpcodeId; -use eth_types::{Field, ToLittleEndian, ToScalar}; +use eth_types::{Field, ToLittleEndian}; use halo2_proofs::{circuit::Region, plonk::Error}; use crate::{ evm_circuit::{ param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, step::ExecutionState, - table::{AccountFieldTag, CallContextFieldTag}, util::{ common_gadget::SameContextGadget, constraint_builder::{ConstraintBuilder, StepStateTransition, Transition}, @@ -30,8 +29,6 @@ pub(crate) struct CodeCopyGadget { code_offset: MemoryAddress, /// Holds the size of the current environment's bytecode. code_size: Cell, - /// Holds the current environment's address from where we wish to read code. - account: Cell, /// The code from current environment is copied to memory. To verify this /// copy operation we need the MemoryAddressGadget. dst_memory_addr: MemoryAddressGadget, @@ -64,15 +61,11 @@ impl ExecutionGadget for CodeCopyGadget { // Construct memory address in the destionation (memory) to which we copy code. let dst_memory_addr = MemoryAddressGadget::construct(cb, dest_memory_offset, size.clone()); - // Query additional fields for the account's code. - let account = cb.call_context(None, CallContextFieldTag::CalleeAddress); - let code_hash = cb.curr.state.code_source.clone(); - - // Lookup the code hash of the current environment's account. - cb.account_read(account.expr(), AccountFieldTag::CodeHash, code_hash.expr()); + // Fetch the source code running in current environment. + let code_source = cb.curr.state.code_source.clone(); // Fetch the bytecode length from the bytecode table. - let code_size = cb.bytecode_length(code_hash.expr()); + let code_size = cb.bytecode_length(code_source.expr()); // Calculate the next memory size and the gas cost for this memory // access. This also accounts for the dynamic gas required to copy bytes to @@ -98,7 +91,7 @@ impl ExecutionGadget for CodeCopyGadget { let next_dst_addr = cb.query_cell(); let next_bytes_left = cb.query_cell(); let next_src_addr_end = cb.query_cell(); - let next_code_hash = cb.query_word(); + let next_code_source = cb.query_word(); cb.require_equal( "next_src_addr == code_offset", @@ -121,9 +114,9 @@ impl ExecutionGadget for CodeCopyGadget { code_size.expr(), ); cb.require_equal( - "next_code_hash == code_hash", - next_code_hash.expr(), - code_hash.expr(), + "next_code_source == code_source", + next_code_source.expr(), + code_source.expr(), ); }, ); @@ -145,7 +138,6 @@ impl ExecutionGadget for CodeCopyGadget { same_context, code_offset, code_size, - account, dst_memory_addr, memory_expansion, memory_copier_gas, @@ -183,19 +175,12 @@ impl ExecutionGadget for CodeCopyGadget { ), )?; - let account = if call.is_root { - call.callee_address - } else { - unimplemented!("CODECOPY does not support internal calls yet"); - }; - self.account.assign(region, offset, account.to_scalar())?; - let code = block .bytecodes .iter() .find(|b| { - let CodeSource::Account(code_hash) = &call.code_source; - b.hash == *code_hash + let CodeSource::Account(code_source) = &call.code_source; + b.hash == *code_source }) .expect("could not find current environment's bytecode"); self.code_size @@ -225,7 +210,7 @@ mod tests { use std::ops::Sub; use bus_mapping::evm::OpcodeId; - use eth_types::{bytecode, Address, ToWord, Word}; + use eth_types::{bytecode, Word}; use halo2_proofs::arithmetic::BaseExt; use num::Zero; use pairing::bn256::Fr; @@ -233,7 +218,7 @@ mod tests { use crate::evm_circuit::{ execution::copy_code_to_memory::test::make_copy_code_steps, step::ExecutionState, - table::{AccountFieldTag, CallContextFieldTag, RwTableTag}, + table::RwTableTag, test::{calc_memory_copier_gas_cost, run_test_circuit_incomplete_fixed_table}, witness::{Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, Transaction}, }; @@ -241,7 +226,6 @@ mod tests { fn test_ok(src_addr: u64, dst_addr: u64, size: usize) { let randomness = Fr::rand(); let call_id = 1; - let callee_addr = Address::random(); let code = bytecode! { #[start] @@ -254,88 +238,59 @@ mod tests { let code = Bytecode::new(code.to_vec()); let mut rws_map = RwMap( - [ - ( - RwTableTag::Stack, - vec![ - // Stack item written by PUSH32. - Rw::Stack { - rw_counter: 1, - is_write: true, - call_id, - stack_pointer: 1023, - value: Word::from(size), - }, - // Stack item written by PUSH32. - Rw::Stack { - rw_counter: 2, - is_write: true, - call_id, - stack_pointer: 1022, - value: Word::from(src_addr), - }, - // Stack item written by PUSH32. - Rw::Stack { - rw_counter: 3, - is_write: true, - call_id, - stack_pointer: 1021, - value: Word::from(dst_addr), - }, - // First stack item read by CODECOPY. - Rw::Stack { - rw_counter: 4, - is_write: false, - call_id, - stack_pointer: 1021, - value: Word::from(dst_addr), - }, - // Second stack item read by CODECOPY. - Rw::Stack { - rw_counter: 5, - is_write: false, - call_id, - stack_pointer: 1022, - value: Word::from(src_addr), - }, - // Third stack item read by CODECOPY. - Rw::Stack { - rw_counter: 6, - is_write: false, - call_id, - stack_pointer: 1023, - value: Word::from(size), - }, - ], - ), - ( - RwTableTag::CallContext, - vec![ - // Callee address lookup in CODECOPY. - Rw::CallContext { - rw_counter: 7, - call_id, - is_write: false, - field_tag: CallContextFieldTag::CalleeAddress, - value: callee_addr.to_word(), - }, - ], - ), - ( - RwTableTag::Account, - vec![ - // Code hash lookup in CODECOPY. - Rw::Account { - rw_counter: 8, - is_write: false, - account_address: callee_addr, - field_tag: AccountFieldTag::CodeHash, - value: code.hash, - value_prev: code.hash, - }, - ], - ), - ] + [( + RwTableTag::Stack, + vec![ + // Stack item written by PUSH32. + Rw::Stack { + rw_counter: 1, + is_write: true, + call_id, + stack_pointer: 1023, + value: Word::from(size), + }, + // Stack item written by PUSH32. + Rw::Stack { + rw_counter: 2, + is_write: true, + call_id, + stack_pointer: 1022, + value: Word::from(src_addr), + }, + // Stack item written by PUSH32. + Rw::Stack { + rw_counter: 3, + is_write: true, + call_id, + stack_pointer: 1021, + value: Word::from(dst_addr), + }, + // First stack item read by CODECOPY. + Rw::Stack { + rw_counter: 4, + is_write: false, + call_id, + stack_pointer: 1021, + value: Word::from(dst_addr), + }, + // Second stack item read by CODECOPY. + Rw::Stack { + rw_counter: 5, + is_write: false, + call_id, + stack_pointer: 1022, + value: Word::from(src_addr), + }, + // Third stack item read by CODECOPY. + Rw::Stack { + rw_counter: 6, + is_write: false, + call_id, + stack_pointer: 1023, + value: Word::from(size), + }, + ], + )] .into(), ); @@ -390,8 +345,6 @@ mod tests { (RwTableTag::Stack, 3), (RwTableTag::Stack, 4), (RwTableTag::Stack, 5), - (RwTableTag::CallContext, 0), - (RwTableTag::Account, 0), ], execution_state: ExecutionState::CODECOPY, rw_counter: 4, @@ -406,7 +359,7 @@ mod tests { let program_counter = 100; let stack_pointer = 1024; - let mut rw_counter = 9; + let mut rw_counter = 7; if !size.is_zero() { make_copy_code_steps( call_id, @@ -441,7 +394,6 @@ mod tests { is_root: true, is_create: false, code_source: CodeSource::Account(code.hash), - callee_address: callee_addr, ..Default::default() }], steps, diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index 3daf4d8eeb..6f4cf56333 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -36,8 +36,8 @@ pub(crate) struct CopyCodeToMemoryGadget { bytes_left: Cell, /// Source (bytecode) bytes end here. src_addr_end: Cell, - /// Keccak-256 hash of the bytecode. - code_hash: Word, + /// Keccak-256 hash of the bytecode source. + code_source: Word, /// Array of booleans to mark whether or not the byte in question is an /// opcode byte or an argument that follows the opcode. For example, /// `is_code = true` for `POP`, `is_code = true` for `PUSH32`, but @@ -62,7 +62,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { let dst_addr = cb.query_cell(); let bytes_left = cb.query_cell(); let src_addr_end = cb.query_cell(); - let code_hash = cb.query_word(); + let code_source = cb.query_word(); let is_codes = array_init(|_| cb.query_bool()); let buffer_reader = BufferReaderGadget::construct(cb, src_addr.expr(), src_addr_end.expr()); @@ -72,7 +72,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { // memory address from the buffer. cb.condition(buffer_reader.read_flag(idx), |cb| { cb.bytecode_lookup( - code_hash.expr(), + code_source.expr(), src_addr.expr() + idx.expr(), is_code.expr(), buffer_reader.byte(idx), @@ -116,7 +116,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { let next_dst_addr = cb.query_cell(); let next_bytes_left = cb.query_cell(); let next_src_addr_end = cb.query_cell(); - let next_code_hash = cb.query_word(); + let next_code_source = cb.query_word(); cb.require_equal( "next_src_addr == src_addr + copied_size", @@ -139,9 +139,9 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { src_addr_end.expr(), ); cb.require_equal( - "next_code_hash == code_hash", - next_code_hash.expr(), - code_hash.expr(), + "next_code_sourcec == code_source", + next_code_source.expr(), + code_source.expr(), ); }, ); @@ -160,7 +160,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { dst_addr, bytes_left, src_addr_end, - code_hash, + code_source, is_codes, buffer_reader, finish_gadget, @@ -211,7 +211,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .assign(region, offset, Some(F::from(*bytes_left)))?; self.src_addr_end .assign(region, offset, Some(F::from(*src_addr_end)))?; - self.code_hash + self.code_source .assign(region, offset, Some(code.hash.to_le_bytes()))?; // Initialise selectors and bytes for the buffer reader. From 685c5c20d2480478e37dcd3d6eee8cb8d828c488 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Tue, 22 Mar 2022 11:24:51 +0800 Subject: [PATCH 07/12] fix: remove selectors field from copycode aux data --- .../execution/copy_code_to_memory.rs | 88 ++++++++----------- 1 file changed, 37 insertions(+), 51 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index 6f4cf56333..ebe4acad6e 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -177,27 +177,19 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { step: &ExecStep, ) -> Result<(), Error> { // Read the auxiliary data. - let (src_addr, dst_addr, bytes_left, src_addr_end, code, selectors) = + let (src_addr, dst_addr, bytes_left, src_addr_end, code) = if let StepAuxiliaryData::CopyCodeToMemory { src_addr, dst_addr, bytes_left, src_addr_end, code, - selectors, } = step .aux_data .as_ref() .expect("could not find aux_data for COPYCODETOMEMORY") { - ( - src_addr, - dst_addr, - bytes_left, - src_addr_end, - code, - selectors, - ) + (src_addr, dst_addr, bytes_left, src_addr_end, code) } else { panic!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"); }; @@ -215,38 +207,37 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .assign(region, offset, Some(code.hash.to_le_bytes()))?; // Initialise selectors and bytes for the buffer reader. - assert_eq!(selectors.len(), MAX_COPY_BYTES); + let mut new_selectors = vec![0u8; MAX_COPY_BYTES]; let mut bytes = vec![0u8; MAX_COPY_BYTES]; - for (idx, (selector, code_it)) in selectors - .iter() - .zip( - code.table_assignments(block.randomness) - // add 1 since the first row is reserved for bytecode length. - .skip(*src_addr as usize + 1) - .take(MAX_COPY_BYTES), - ) - .enumerate() - { + let is_codes = code + .table_assignments(block.randomness) + .skip(1) + .map(|c| c[3]) + .collect::>(); + for idx in 0..std::cmp::min(*bytes_left as usize, MAX_COPY_BYTES) { + new_selectors[idx] = 1u8; let addr = *src_addr as usize + idx; - bytes[idx] = if *selector == 1 && addr < *src_addr_end as usize { + bytes[idx] = if addr < *src_addr_end as usize { assert!(addr < code.bytes.len()); + self.is_codes[idx].assign(region, offset, Some(is_codes[addr]))?; code.bytes[addr] } else { 0 }; - - // The last entry (index = 3) from the table assignments for bytecode represents - // whether or not it is an opcode byte or an opcode argument. - self.is_codes[idx].assign(region, offset, Some(code_it[3]))?; } - // Assign the buffer reader. - self.buffer_reader - .assign(region, offset, *src_addr, *src_addr_end, &bytes, selectors)?; + self.buffer_reader.assign( + region, + offset, + *src_addr, + *src_addr_end, + &bytes, + &new_selectors, + )?; // The number of bytes copied here will be the sum of 1s over the selector // vector. - let num_bytes_copied = selectors.iter().sum::() as u64; + let num_bytes_copied = std::cmp::min(*bytes_left, MAX_COPY_BYTES as u64); // Assign the comparison gadget. self.finish_gadget.assign( @@ -294,30 +285,26 @@ pub(crate) mod test { bytes_map: &HashMap, code: &Bytecode, ) -> (ExecStep, usize) { - let mut selectors = vec![0u8; MAX_COPY_BYTES]; let mut rw_offset = 0usize; let memory_rws: &mut Vec<_> = rws.0.entry(RwTableTag::Memory).or_insert_with(Vec::new); let rw_idx_start = memory_rws.len(); - for (idx, selector) in selectors.iter_mut().enumerate() { - if idx < bytes_left { - *selector = 1; - let addr = src_addr + idx as u64; - let byte = if addr < src_addr_end { - assert!(bytes_map.contains_key(&addr)); - bytes_map[&addr] - } else { - 0 - }; - memory_rws.push(Rw::Memory { - rw_counter: rw_counter + rw_offset, - is_write: true, - call_id, - memory_address: dst_addr + idx as u64, - byte, - }); - rw_offset += 1; - } + for idx in 0..std::cmp::min(bytes_left, MAX_COPY_BYTES) { + let addr = src_addr + idx as u64; + let byte = if addr < src_addr_end { + assert!(bytes_map.contains_key(&addr)); + bytes_map[&addr] + } else { + 0 + }; + memory_rws.push(Rw::Memory { + rw_counter: rw_counter + rw_offset, + is_write: true, + call_id, + memory_address: dst_addr + idx as u64, + byte, + }); + rw_offset += 1; } let rw_idx_end = rws.0[&RwTableTag::Memory].len(); @@ -327,7 +314,6 @@ pub(crate) mod test { bytes_left: bytes_left as u64, src_addr_end, code: code.clone(), - selectors, }; let step = ExecStep { execution_state: ExecutionState::CopyCodeToMemory, From d89e547bae2bbb66b8084041606f6fd2fad8ad5e Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Tue, 22 Mar 2022 11:55:18 +0800 Subject: [PATCH 08/12] chore: rebased and updated --- .../src/evm_circuit/execution/codecopy.rs | 2 +- .../execution/copy_code_to_memory.rs | 43 +++++++++---------- .../src/evm_circuit/execution/memory_copy.rs | 8 +--- 3 files changed, 23 insertions(+), 30 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index f7a2ac49ce..ea4d1ce0e2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -235,7 +235,6 @@ mod tests { CODECOPY STOP }; - let code = Bytecode::new(code.to_vec()); let mut rws_map = RwMap( [( @@ -385,6 +384,7 @@ mod tests { ..Default::default() }); + let code = Bytecode::new(code.to_vec()); let block = Block { randomness, txs: vec![Transaction { diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index ebe4acad6e..b210d19126 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -1,4 +1,5 @@ use array_init::array_init; +use bus_mapping::circuit_input_builder::StepAuxiliaryData; use eth_types::{Field, ToLittleEndian}; use halo2_proofs::{circuit::Region, plonk::Error}; @@ -12,7 +13,7 @@ use crate::{ memory_gadget::BufferReaderGadget, Cell, Word, }, - witness::{Block, Call, ExecStep, StepAuxiliaryData, Transaction}, + witness::{Block, Bytecode, Call, ExecStep, Transaction}, }, util::Expr, }; @@ -189,7 +190,13 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .as_ref() .expect("could not find aux_data for COPYCODETOMEMORY") { - (src_addr, dst_addr, bytes_left, src_addr_end, code) + ( + src_addr, + dst_addr, + bytes_left, + src_addr_end, + Bytecode::new(code.to_vec()), + ) } else { panic!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"); }; @@ -207,7 +214,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .assign(region, offset, Some(code.hash.to_le_bytes()))?; // Initialise selectors and bytes for the buffer reader. - let mut new_selectors = vec![0u8; MAX_COPY_BYTES]; + let mut selectors = vec![false; MAX_COPY_BYTES]; let mut bytes = vec![0u8; MAX_COPY_BYTES]; let is_codes = code .table_assignments(block.randomness) @@ -215,7 +222,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .map(|c| c[3]) .collect::>(); for idx in 0..std::cmp::min(*bytes_left as usize, MAX_COPY_BYTES) { - new_selectors[idx] = 1u8; + selectors[idx] = true; let addr = *src_addr as usize + idx; bytes[idx] = if addr < *src_addr_end as usize { assert!(addr < code.bytes.len()); @@ -226,14 +233,8 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { }; } - self.buffer_reader.assign( - region, - offset, - *src_addr, - *src_addr_end, - &bytes, - &new_selectors, - )?; + self.buffer_reader + .assign(region, offset, *src_addr, *src_addr_end, &bytes, &selectors)?; // The number of bytes copied here will be the sum of 1s over the selector // vector. @@ -256,7 +257,7 @@ pub(crate) mod test { use super::MAX_COPY_BYTES; use std::collections::HashMap; - use bus_mapping::evm::OpcodeId; + use bus_mapping::{circuit_input_builder::StepAuxiliaryData, evm::OpcodeId}; use eth_types::{bytecode, Word}; use halo2_proofs::arithmetic::BaseExt; use pairing::bn256::Fr; @@ -265,9 +266,7 @@ pub(crate) mod test { step::ExecutionState, table::RwTableTag, test::run_test_circuit_incomplete_fixed_table, - witness::{ - Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, StepAuxiliaryData, Transaction, - }, + witness::{Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, Transaction}, }; #[allow(clippy::too_many_arguments)] @@ -283,7 +282,7 @@ pub(crate) mod test { rw_counter: usize, rws: &mut RwMap, bytes_map: &HashMap, - code: &Bytecode, + code: ð_types::Bytecode, ) -> (ExecStep, usize) { let mut rw_offset = 0usize; let memory_rws: &mut Vec<_> = rws.0.entry(RwTableTag::Memory).or_insert_with(Vec::new); @@ -335,7 +334,7 @@ pub(crate) mod test { #[allow(clippy::too_many_arguments)] pub(crate) fn make_copy_code_steps( call_id: usize, - code: &Bytecode, + code: ð_types::Bytecode, src_addr: u64, dst_addr: u64, length: usize, @@ -346,8 +345,8 @@ pub(crate) mod test { rws: &mut RwMap, steps: &mut Vec, ) { - let bytes_map = (0..(code.bytes.len() as u64)) - .zip(code.bytes.iter().copied()) + let bytes_map = (0..(code.code().len() as u64)) + .zip(code.code().iter().copied()) .collect(); let mut copied = 0; @@ -356,7 +355,7 @@ pub(crate) mod test { call_id, src_addr + copied as u64, dst_addr + copied as u64, - code.bytes.len() as u64, + code.code().len() as u64, length - copied, program_counter, stack_pointer, @@ -394,7 +393,6 @@ pub(crate) mod test { PUSH32(Word::from(0x1928835)) POP }; - let code = Bytecode::new(code.to_vec()); let dummy_code = Bytecode::new(vec![OpcodeId::STOP.as_u8()]); @@ -424,6 +422,7 @@ pub(crate) mod test { ..Default::default() }); + let code = Bytecode::new(code.to_vec()); let block = Block { randomness, txs: vec![Transaction { diff --git a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs index c4d8f53ec5..cc17f59101 100644 --- a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs @@ -179,13 +179,7 @@ impl ExecutionGadget for CopyToMemoryGadget { .as_ref() .expect("could not find aux_data for COPYTOMEMORY") { - ( - src_addr, - dst_addr, - bytes_left, - src_addr_end, - from_tx, - ) + (src_addr, dst_addr, bytes_left, src_addr_end, from_tx) } else { panic!("could not find CopyToMemory aux_data for COPYTOMEMORY"); }; From 8af919f1a4addae1738f4d26606e895ab23861f4 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Wed, 6 Apr 2022 02:14:02 +0800 Subject: [PATCH 09/12] fix: helper fn moved to eth_types crate --- zkevm-circuits/src/evm_circuit/execution/codecopy.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index ea4d1ce0e2..1193f888fb 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -210,7 +210,7 @@ mod tests { use std::ops::Sub; use bus_mapping::evm::OpcodeId; - use eth_types::{bytecode, Word}; + use eth_types::{bytecode, evm_types::gas_utils::memory_copier_gas_cost, Word}; use halo2_proofs::arithmetic::BaseExt; use num::Zero; use pairing::bn256::Fr; @@ -219,7 +219,7 @@ mod tests { execution::copy_code_to_memory::test::make_copy_code_steps, step::ExecutionState, table::RwTableTag, - test::{calc_memory_copier_gas_cost, run_test_circuit_incomplete_fixed_table}, + test::run_test_circuit_incomplete_fixed_table, witness::{Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, Transaction}, }; @@ -302,7 +302,7 @@ mod tests { }; let gas_cost_push32 = OpcodeId::PUSH32.constant_gas_cost().as_u64(); let gas_cost_codecopy = OpcodeId::CODECOPY.constant_gas_cost().as_u64() - + calc_memory_copier_gas_cost(0, next_memory_word_size, size as u64); + + memory_copier_gas_cost(0, next_memory_word_size, size as u64); let total_gas_cost = (3 * gas_cost_push32) + gas_cost_codecopy; let mut steps = vec![ From a326290e04d62f1783f54291da4381ec4966f946 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Thu, 7 Apr 2022 16:34:51 +0800 Subject: [PATCH 10/12] fixes from code review --- bus-mapping/src/circuit_input_builder.rs | 8 +- .../src/bytecode_circuit/bytecode_unroller.rs | 26 +++---- zkevm-circuits/src/evm_circuit/execution.rs | 2 +- .../src/evm_circuit/execution/codecopy.rs | 3 +- .../execution/copy_code_to_memory.rs | 37 +++++---- zkevm-circuits/src/evm_circuit/table.rs | 6 +- zkevm-circuits/src/evm_circuit/witness.rs | 78 +++++++------------ 7 files changed, 67 insertions(+), 93 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs index 84044a01cd..a429cc92e2 100644 --- a/bus-mapping/src/circuit_input_builder.rs +++ b/bus-mapping/src/circuit_input_builder.rs @@ -11,7 +11,9 @@ use crate::state_db::{self, CodeDB, StateDB}; use crate::Error; use core::fmt::Debug; use eth_types::evm_types::{Gas, GasCost, MemoryAddress, OpcodeId, ProgramCounter, StackAddress}; -use eth_types::{self, Address, GethExecStep, GethExecTrace, Hash, ToAddress, ToBigEndian, Word}; +use eth_types::{ + self, Address, GethExecStep, GethExecTrace, Hash, ToAddress, ToBigEndian, Word, U256, +}; use ethers_core::utils::{get_contract_address, get_create2_address}; use std::collections::{hash_map::Entry, BTreeMap, HashMap, HashSet}; @@ -165,8 +167,8 @@ pub enum StepAuxiliaryData { bytes_left: u64, /// Source end address src_addr_end: u64, - /// Bytecode to be copied - code: eth_types::Bytecode, + /// Hash of the bytecode to be copied + code_source: U256, }, } diff --git a/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs b/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs index fcdba0755a..7e85af5a7e 100644 --- a/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs +++ b/zkevm-circuits/src/bytecode_circuit/bytecode_unroller.rs @@ -104,16 +104,18 @@ impl Config { // Does the current row have bytecode field tag == Length? let is_row_tag_length = |meta: &mut VirtualCells| { - let row_tag = meta.query_advice(tag, Rotation::cur()); - row_tag.expr() * (BytecodeFieldTag::Byte.expr() - row_tag.expr()) + and::expr(vec![ + not::expr(meta.query_advice(padding, Rotation::cur())), + not::expr(meta.query_advice(tag, Rotation::cur())), + ]) }; // Does the current row have bytecode field tag == Byte? let is_row_tag_byte = |meta: &mut VirtualCells| { - let row_tag = meta.query_advice(tag, Rotation::cur()); - row_tag.expr() - * (row_tag.expr() - BytecodeFieldTag::Length.expr()) - * F::from(2).invert().unwrap() + and::expr(vec![ + not::expr(meta.query_advice(padding, Rotation::cur())), + meta.query_advice(tag, Rotation::cur()), + ]) }; // For a row tagged Length, is the length (value) zero? @@ -141,10 +143,8 @@ impl Config { let is_prev_row_tag_length = |meta: &mut VirtualCells| { and::expr(vec![ not::expr(meta.query_fixed(q_first, Rotation::cur())), - { - let prev_row_tag = meta.query_advice(tag, Rotation::prev()); - prev_row_tag.expr() * (BytecodeFieldTag::Byte.expr() - prev_row_tag.expr()) - }, + not::expr(meta.query_advice(padding, Rotation::prev())), + not::expr(meta.query_advice(tag, Rotation::prev())), ]) }; @@ -205,7 +205,7 @@ impl Config { length_is_zero.clone().is_zero_expression, select::expr( meta.query_advice(padding, Rotation::next()), - 0.expr(), // Padding is 0 + BytecodeFieldTag::Padding.expr(), BytecodeFieldTag::Length.expr(), ), BytecodeFieldTag::Byte.expr(), @@ -466,7 +466,7 @@ impl Config { idx < size, idx == last_row_offset, F::zero(), - F::zero(), + F::from(BytecodeFieldTag::Padding as u64), F::zero(), F::one(), F::zero(), @@ -736,7 +736,7 @@ mod tests { let prover = MockProver::::run(k, &circuit, vec![]).unwrap(); let err = prover.verify(); - let print_failures = false; + let print_failures = true; if err.is_err() && print_failures { for e in err.err().iter() { for s in e.iter() { diff --git a/zkevm-circuits/src/evm_circuit/execution.rs b/zkevm-circuits/src/evm_circuit/execution.rs index a1f65ace3f..2bc36680a3 100644 --- a/zkevm-circuits/src/evm_circuit/execution.rs +++ b/zkevm-circuits/src/evm_circuit/execution.rs @@ -359,6 +359,7 @@ impl ExecutionConfig { q_step_last, // internal states begin_tx_gadget: configure_gadget!(), + copy_code_to_memory_gadget: configure_gadget!(), copy_to_memory_gadget: configure_gadget!(), end_block_gadget: configure_gadget!(), end_tx_gadget: configure_gadget!(), @@ -376,7 +377,6 @@ impl ExecutionConfig { codecopy_gadget: configure_gadget!(), coinbase_gadget: configure_gadget!(), comparator_gadget: configure_gadget!(), - copy_code_to_memory_gadget: configure_gadget!(), dup_gadget: configure_gadget!(), extcodehash_gadget: configure_gadget!(), gas_gadget: configure_gadget!(), diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index 1193f888fb..eb3be3024e 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -228,13 +228,13 @@ mod tests { let call_id = 1; let code = bytecode! { - #[start] PUSH32(Word::from(size)) PUSH32(Word::from(src_addr)) PUSH32(Word::from(dst_addr)) CODECOPY STOP }; + let code = Bytecode::new(code.to_vec()); let mut rws_map = RwMap( [( @@ -384,7 +384,6 @@ mod tests { ..Default::default() }); - let code = Bytecode::new(code.to_vec()); let block = Block { randomness, txs: vec![Transaction { diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index b210d19126..579243cfa2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -13,7 +13,7 @@ use crate::{ memory_gadget::BufferReaderGadget, Cell, Word, }, - witness::{Block, Bytecode, Call, ExecStep, Transaction}, + witness::{Block, Call, ExecStep, Transaction}, }, util::Expr, }; @@ -23,7 +23,7 @@ use super::ExecutionGadget; /// Maximum number of bytes that can be copied in one iteration of this gadget. /// We are bounded because of the limitation on the number of cells we can /// assign to in one iteration. -const MAX_COPY_BYTES: usize = 54; +const MAX_COPY_BYTES: usize = 32; #[derive(Clone, Debug)] /// This gadget is responsible for copying bytes from an account's code to @@ -178,29 +178,28 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { step: &ExecStep, ) -> Result<(), Error> { // Read the auxiliary data. - let (src_addr, dst_addr, bytes_left, src_addr_end, code) = + let (src_addr, dst_addr, bytes_left, src_addr_end, code_source) = if let StepAuxiliaryData::CopyCodeToMemory { src_addr, dst_addr, bytes_left, src_addr_end, - code, + code_source, } = step .aux_data .as_ref() .expect("could not find aux_data for COPYCODETOMEMORY") { - ( - src_addr, - dst_addr, - bytes_left, - src_addr_end, - Bytecode::new(code.to_vec()), - ) + (src_addr, dst_addr, bytes_left, src_addr_end, code_source) } else { panic!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"); }; + let code = block + .bytecodes + .iter() + .find(|b| b.hash == *code_source) + .unwrap_or_else(|| panic!("could not find bytecode for source {:?}", code_source)); // Assign to the appropriate cells. self.src_addr .assign(region, offset, Some(F::from(*src_addr)))?; @@ -218,6 +217,7 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { let mut bytes = vec![0u8; MAX_COPY_BYTES]; let is_codes = code .table_assignments(block.randomness) + .iter() .skip(1) .map(|c| c[3]) .collect::>(); @@ -282,7 +282,7 @@ pub(crate) mod test { rw_counter: usize, rws: &mut RwMap, bytes_map: &HashMap, - code: ð_types::Bytecode, + code: &Bytecode, ) -> (ExecStep, usize) { let mut rw_offset = 0usize; let memory_rws: &mut Vec<_> = rws.0.entry(RwTableTag::Memory).or_insert_with(Vec::new); @@ -312,7 +312,7 @@ pub(crate) mod test { dst_addr, bytes_left: bytes_left as u64, src_addr_end, - code: code.clone(), + code_source: code.hash, }; let step = ExecStep { execution_state: ExecutionState::CopyCodeToMemory, @@ -334,7 +334,7 @@ pub(crate) mod test { #[allow(clippy::too_many_arguments)] pub(crate) fn make_copy_code_steps( call_id: usize, - code: ð_types::Bytecode, + code: &Bytecode, src_addr: u64, dst_addr: u64, length: usize, @@ -345,8 +345,8 @@ pub(crate) mod test { rws: &mut RwMap, steps: &mut Vec, ) { - let bytes_map = (0..(code.code().len() as u64)) - .zip(code.code().iter().copied()) + let bytes_map = (0..(code.bytes.len() as u64)) + .zip(code.bytes.iter().copied()) .collect(); let mut copied = 0; @@ -355,7 +355,7 @@ pub(crate) mod test { call_id, src_addr + copied as u64, dst_addr + copied as u64, - code.code().len() as u64, + code.bytes.len() as u64, length - copied, program_counter, stack_pointer, @@ -381,7 +381,6 @@ pub(crate) mod test { // generate random bytecode longer than `src_addr_end` let code = bytecode! { - #[start] PUSH32(Word::from(0x123)) POP PUSH32(Word::from(0x213)) @@ -394,6 +393,7 @@ pub(crate) mod test { POP }; + let code = Bytecode::new(code.to_vec()); let dummy_code = Bytecode::new(vec![OpcodeId::STOP.as_u8()]); let program_counter = 0; @@ -422,7 +422,6 @@ pub(crate) mod test { ..Default::default() }); - let code = Bytecode::new(code.to_vec()); let block = Block { randomness, txs: vec![Transaction { diff --git a/zkevm-circuits/src/evm_circuit/table.rs b/zkevm-circuits/src/evm_circuit/table.rs index a076e47c16..1c0eb62126 100644 --- a/zkevm-circuits/src/evm_circuit/table.rs +++ b/zkevm-circuits/src/evm_circuit/table.rs @@ -175,13 +175,13 @@ pub enum AccountFieldTag { Nonce = 1, Balance, CodeHash, - CodeSize, } #[derive(Clone, Copy, Debug)] pub enum BytecodeFieldTag { - Length = 1, + Length, Byte, + Padding, } #[derive(Clone, Copy, Debug, PartialEq)] @@ -281,7 +281,7 @@ pub(crate) enum Lookup { /// A boolean value to specify if the value is executable opcode or the /// data portion of PUSH* operations. is_code: Expression, - /// Value of the index. + /// Value corresponding to the tag. value: Expression, }, /// Lookup to block table, which contains constants of this block. diff --git a/zkevm-circuits/src/evm_circuit/witness.rs b/zkevm-circuits/src/evm_circuit/witness.rs index 7acddb39fd..8072e6b3a4 100644 --- a/zkevm-circuits/src/evm_circuit/witness.rs +++ b/zkevm-circuits/src/evm_circuit/witness.rs @@ -332,65 +332,41 @@ impl Bytecode { Self { hash, bytes } } - pub fn table_assignments<'a, F: FieldExt>( - &'a self, - randomness: F, - ) -> impl Iterator + '_ { - struct BytecodeIterator<'a, F> { - idx: usize, - push_data_left: usize, - hash: F, - bytes: &'a [u8], - } - - impl<'a, F: FieldExt> Iterator for BytecodeIterator<'a, F> { - type Item = [F; 5]; - - fn next(&mut self) -> Option { - if self.idx == 0 { - self.idx += 1; - return Some([ - self.hash, - F::from(BytecodeFieldTag::Length as u64), - F::from(0), - F::from(0), - F::from(self.bytes.len() as u64), - ]); - } - - if self.idx > self.bytes.len() { - return None; - } - - let idx = self.idx - 1; - let byte = self.bytes[idx]; + pub fn table_assignments(&self, randomness: F) -> Vec<[F; 5]> { + let n = 1 + self.bytes.len(); + let mut rows = Vec::with_capacity(n); + let hash = + RandomLinearCombination::random_linear_combine(self.hash.to_le_bytes(), randomness); + let mut push_data_left = 0; + for idx in 0..n { + if idx == 0 { + rows.push([ + hash, + F::from(BytecodeFieldTag::Length as u64), + F::zero(), + F::zero(), + F::from(self.bytes.len() as u64), + ]); + } else { + let i = idx - 1; + let byte = self.bytes[i]; let mut is_code = true; - if self.push_data_left > 0 { + if push_data_left > 0 { is_code = false; - self.push_data_left -= 1; + push_data_left -= 1; } else if (OpcodeId::PUSH1.as_u8()..=OpcodeId::PUSH32.as_u8()).contains(&byte) { - self.push_data_left = byte as usize - (OpcodeId::PUSH1.as_u8() - 1) as usize; + push_data_left = byte as usize - (OpcodeId::PUSH1.as_u8() - 1) as usize; } - self.idx += 1; - Some([ - self.hash, + rows.push([ + hash, F::from(BytecodeFieldTag::Byte as u64), - F::from(idx as u64), + F::from(i as u64), F::from(is_code as u64), F::from(byte as u64), ]) } } - - BytecodeIterator { - idx: 0, - push_data_left: 0, - hash: RandomLinearCombination::random_linear_combine( - self.hash.to_le_bytes(), - randomness, - ), - bytes: &self.bytes, - } + rows } } @@ -707,9 +683,7 @@ impl Rw { value_prev, } => { let to_scalar = |value: &Word| match field_tag { - AccountFieldTag::Nonce | AccountFieldTag::CodeSize => { - value.to_scalar().unwrap() - } + AccountFieldTag::Nonce => value.to_scalar().unwrap(), _ => RandomLinearCombination::random_linear_combine( value.to_le_bytes(), randomness, From 92adc1b47b9bd64bd4adc6d2342f76866d5a173e Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Fri, 8 Apr 2022 09:02:08 +0800 Subject: [PATCH 11/12] chore: minor refactoring as per review --- bus-mapping/src/circuit_input_builder.rs | 56 +++++++++++-------- bus-mapping/src/evm/opcodes/calldatacopy.rs | 8 ++- .../src/evm_circuit/execution/codecopy.rs | 1 + .../execution/copy_code_to_memory.rs | 41 +++++++------- .../src/evm_circuit/execution/memory_copy.rs | 38 ++++++------- 5 files changed, 77 insertions(+), 67 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs index a429cc92e2..d1a8b649be 100644 --- a/bus-mapping/src/circuit_input_builder.rs +++ b/bus-mapping/src/circuit_input_builder.rs @@ -141,35 +141,43 @@ impl ExecState { } } +/// Auxiliary data for CopyToMemory internal state. +#[derive(Clone, Debug)] +pub struct CopyToMemoryAuxData { + /// Source start address + pub src_addr: u64, + /// Destination address + pub dst_addr: u64, + /// Bytes left + pub bytes_left: u64, + /// Source end address + pub src_addr_end: u64, + /// Indicate if copy from transaction call data + pub from_tx: bool, +} + +/// Auxiliary data for CopyCodeToMemory internal state. +#[derive(Clone, Debug)] +pub struct CopyCodeToMemoryAuxData { + /// Source start address + pub src_addr: u64, + /// Destination address + pub dst_addr: u64, + /// Bytes left + pub bytes_left: u64, + /// Source end address + pub src_addr_end: u64, + /// Hash of the bytecode to be copied + pub code_source: U256, +} + /// Auxiliary data of Execution step #[derive(Clone, Debug)] pub enum StepAuxiliaryData { /// Auxiliary data of Copy To Memory - CopyToMemory { - /// Source start address - src_addr: u64, - /// Destination address - dst_addr: u64, - /// Bytes left - bytes_left: u64, - /// Source end address - src_addr_end: u64, - /// Indicate if copy from transaction call data - from_tx: bool, - }, + CopyToMemory(CopyToMemoryAuxData), /// Auxiliary data of Copy Code To Memory - CopyCodeToMemory { - /// Source start address - src_addr: u64, - /// Destination address - dst_addr: u64, - /// Bytes left - bytes_left: u64, - /// Source end address - src_addr_end: u64, - /// Hash of the bytecode to be copied - code_source: U256, - }, + CopyCodeToMemory(CopyCodeToMemoryAuxData), } /// An execution step of the EVM. diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 03700ac5fc..cca89fce97 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -1,5 +1,7 @@ use super::Opcode; -use crate::circuit_input_builder::{CircuitInputStateRef, ExecState, ExecStep, StepAuxiliaryData}; +use crate::circuit_input_builder::{ + CircuitInputStateRef, CopyToMemoryAuxData, ExecState, ExecStep, StepAuxiliaryData, +}; use crate::operation::{CallContextField, CallContextOp, RW}; use crate::Error; use eth_types::GethExecStep; @@ -108,13 +110,13 @@ fn gen_memory_copy_step( state.push_memory_op(exec_step, RW::WRITE, (idx + dst_addr as usize).into(), byte)?; } - exec_step.aux_data = Some(StepAuxiliaryData::CopyToMemory { + exec_step.aux_data = Some(StepAuxiliaryData::CopyToMemory(CopyToMemoryAuxData { src_addr, dst_addr, bytes_left: bytes_left as u64, src_addr_end, from_tx: is_root, - }); + })); Ok(()) } diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index eb3be3024e..e07e1a523d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -408,6 +408,7 @@ mod tests { #[test] fn codecopy_gadget_single_step() { test_ok(0x00, 0x00, 54); + test_ok(0x10, 0x05, 54); } #[test] diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index 579243cfa2..78b3d62dcf 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -1,5 +1,5 @@ use array_init::array_init; -use bus_mapping::circuit_input_builder::StepAuxiliaryData; +use bus_mapping::circuit_input_builder::{CopyCodeToMemoryAuxData, StepAuxiliaryData}; use eth_types::{Field, ToLittleEndian}; use halo2_proofs::{circuit::Region, plonk::Error}; @@ -178,22 +178,20 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { step: &ExecStep, ) -> Result<(), Error> { // Read the auxiliary data. - let (src_addr, dst_addr, bytes_left, src_addr_end, code_source) = - if let StepAuxiliaryData::CopyCodeToMemory { - src_addr, - dst_addr, - bytes_left, - src_addr_end, - code_source, - } = step - .aux_data - .as_ref() - .expect("could not find aux_data for COPYCODETOMEMORY") - { - (src_addr, dst_addr, bytes_left, src_addr_end, code_source) - } else { - panic!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"); - }; + let CopyCodeToMemoryAuxData { + src_addr, + dst_addr, + bytes_left, + src_addr_end, + code_source, + } = match step + .aux_data + .as_ref() + .expect("could not find aux_data for COPYCODETOMEMORY") + { + StepAuxiliaryData::CopyCodeToMemory(aux) => aux, + _ => unreachable!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"), + }; let code = block .bytecodes @@ -257,7 +255,10 @@ pub(crate) mod test { use super::MAX_COPY_BYTES; use std::collections::HashMap; - use bus_mapping::{circuit_input_builder::StepAuxiliaryData, evm::OpcodeId}; + use bus_mapping::{ + circuit_input_builder::{CopyCodeToMemoryAuxData, StepAuxiliaryData}, + evm::OpcodeId, + }; use eth_types::{bytecode, Word}; use halo2_proofs::arithmetic::BaseExt; use pairing::bn256::Fr; @@ -307,13 +308,13 @@ pub(crate) mod test { } let rw_idx_end = rws.0[&RwTableTag::Memory].len(); - let aux_data = StepAuxiliaryData::CopyCodeToMemory { + let aux_data = StepAuxiliaryData::CopyCodeToMemory(CopyCodeToMemoryAuxData { src_addr, dst_addr, bytes_left: bytes_left as u64, src_addr_end, code_source: code.hash, - }; + }); let step = ExecStep { execution_state: ExecutionState::CopyCodeToMemory, rw_indices: (rw_idx_start..rw_idx_end) diff --git a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs index cc17f59101..c5d7e9ef0d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs @@ -14,7 +14,7 @@ use crate::{ }, util::Expr, }; -use bus_mapping::circuit_input_builder::StepAuxiliaryData; +use bus_mapping::circuit_input_builder::{CopyToMemoryAuxData, StepAuxiliaryData}; use eth_types::Field; use halo2_proofs::{circuit::Region, plonk::Error}; @@ -167,22 +167,20 @@ impl ExecutionGadget for CopyToMemoryGadget { _: &Call, step: &ExecStep, ) -> Result<(), Error> { - let (src_addr, dst_addr, bytes_left, src_addr_end, from_tx) = - if let StepAuxiliaryData::CopyToMemory { - src_addr, - dst_addr, - bytes_left, - src_addr_end, - from_tx, - } = step - .aux_data - .as_ref() - .expect("could not find aux_data for COPYTOMEMORY") - { - (src_addr, dst_addr, bytes_left, src_addr_end, from_tx) - } else { - panic!("could not find CopyToMemory aux_data for COPYTOMEMORY"); - }; + let CopyToMemoryAuxData { + src_addr, + dst_addr, + bytes_left, + src_addr_end, + from_tx, + } = match step + .aux_data + .as_ref() + .expect("could not find aux_data for COPYTOMEMORY") + { + StepAuxiliaryData::CopyToMemory(aux) => aux, + _ => unreachable!("could not find CopyToMemory aux_data for COPYTOMEMORY"), + }; self.src_addr .assign(region, offset, Some(F::from(*src_addr)))?; @@ -242,7 +240,7 @@ pub mod test { test::{rand_bytes, run_test_circuit_incomplete_fixed_table}, witness::{Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, Transaction}, }; - use bus_mapping::circuit_input_builder::StepAuxiliaryData; + use bus_mapping::circuit_input_builder::{CopyToMemoryAuxData, StepAuxiliaryData}; use eth_types::evm_types::OpcodeId; use halo2_proofs::arithmetic::BaseExt; use pairing::bn256::Fr as Fp; @@ -295,13 +293,13 @@ pub mod test { rw_offset += 1; } let rw_idx_end = rws.0[&RwTableTag::Memory].len(); - let aux_data = StepAuxiliaryData::CopyToMemory { + let aux_data = StepAuxiliaryData::CopyToMemory(CopyToMemoryAuxData { src_addr, dst_addr, bytes_left: bytes_left as u64, src_addr_end, from_tx, - }; + }); let step = ExecStep { execution_state: ExecutionState::CopyToMemory, rw_indices: (rw_idx_start..rw_idx_end) From b8f469ba516524b0a570bce8a257469956bdba30 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Fri, 8 Apr 2022 14:34:44 +0800 Subject: [PATCH 12/12] chore: refactoring/cleanup --- bus-mapping/src/circuit_input_builder.rs | 6 +-- .../execution/copy_code_to_memory.rs | 30 +++++------ .../src/evm_circuit/execution/memory_copy.rs | 32 +++++------- zkevm-circuits/src/evm_circuit/witness.rs | 51 +++++++++---------- 4 files changed, 54 insertions(+), 65 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder.rs b/bus-mapping/src/circuit_input_builder.rs index d1a8b649be..85d5c198e1 100644 --- a/bus-mapping/src/circuit_input_builder.rs +++ b/bus-mapping/src/circuit_input_builder.rs @@ -142,7 +142,7 @@ impl ExecState { } /// Auxiliary data for CopyToMemory internal state. -#[derive(Clone, Debug)] +#[derive(Clone, Copy, Debug)] pub struct CopyToMemoryAuxData { /// Source start address pub src_addr: u64, @@ -157,7 +157,7 @@ pub struct CopyToMemoryAuxData { } /// Auxiliary data for CopyCodeToMemory internal state. -#[derive(Clone, Debug)] +#[derive(Clone, Copy, Debug)] pub struct CopyCodeToMemoryAuxData { /// Source start address pub src_addr: u64, @@ -172,7 +172,7 @@ pub struct CopyCodeToMemoryAuxData { } /// Auxiliary data of Execution step -#[derive(Clone, Debug)] +#[derive(Clone, Copy, Debug)] pub enum StepAuxiliaryData { /// Auxiliary data of Copy To Memory CopyToMemory(CopyToMemoryAuxData), diff --git a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs index 78b3d62dcf..6ab8692f01 100644 --- a/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs +++ b/zkevm-circuits/src/evm_circuit/execution/copy_code_to_memory.rs @@ -184,29 +184,25 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { bytes_left, src_addr_end, code_source, - } = match step - .aux_data - .as_ref() - .expect("could not find aux_data for COPYCODETOMEMORY") - { - StepAuxiliaryData::CopyCodeToMemory(aux) => aux, + } = match step.aux_data { + Some(StepAuxiliaryData::CopyCodeToMemory(aux)) => aux, _ => unreachable!("could not find CopyCodeToMemory aux_data for COPYCODETOMEMORY"), }; let code = block .bytecodes .iter() - .find(|b| b.hash == *code_source) + .find(|b| b.hash == code_source) .unwrap_or_else(|| panic!("could not find bytecode for source {:?}", code_source)); // Assign to the appropriate cells. self.src_addr - .assign(region, offset, Some(F::from(*src_addr)))?; + .assign(region, offset, Some(F::from(src_addr)))?; self.dst_addr - .assign(region, offset, Some(F::from(*dst_addr)))?; + .assign(region, offset, Some(F::from(dst_addr)))?; self.bytes_left - .assign(region, offset, Some(F::from(*bytes_left)))?; + .assign(region, offset, Some(F::from(bytes_left)))?; self.src_addr_end - .assign(region, offset, Some(F::from(*src_addr_end)))?; + .assign(region, offset, Some(F::from(src_addr_end)))?; self.code_source .assign(region, offset, Some(code.hash.to_le_bytes()))?; @@ -219,10 +215,10 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { .skip(1) .map(|c| c[3]) .collect::>(); - for idx in 0..std::cmp::min(*bytes_left as usize, MAX_COPY_BYTES) { + for idx in 0..std::cmp::min(bytes_left as usize, MAX_COPY_BYTES) { selectors[idx] = true; - let addr = *src_addr as usize + idx; - bytes[idx] = if addr < *src_addr_end as usize { + let addr = src_addr as usize + idx; + bytes[idx] = if addr < src_addr_end as usize { assert!(addr < code.bytes.len()); self.is_codes[idx].assign(region, offset, Some(is_codes[addr]))?; code.bytes[addr] @@ -232,18 +228,18 @@ impl ExecutionGadget for CopyCodeToMemoryGadget { } self.buffer_reader - .assign(region, offset, *src_addr, *src_addr_end, &bytes, &selectors)?; + .assign(region, offset, src_addr, src_addr_end, &bytes, &selectors)?; // The number of bytes copied here will be the sum of 1s over the selector // vector. - let num_bytes_copied = std::cmp::min(*bytes_left, MAX_COPY_BYTES as u64); + let num_bytes_copied = std::cmp::min(bytes_left, MAX_COPY_BYTES as u64); // Assign the comparison gadget. self.finish_gadget.assign( region, offset, F::from(num_bytes_copied), - F::from(*bytes_left), + F::from(bytes_left), )?; Ok(()) diff --git a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs index c5d7e9ef0d..1624b34aa8 100644 --- a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs @@ -173,25 +173,21 @@ impl ExecutionGadget for CopyToMemoryGadget { bytes_left, src_addr_end, from_tx, - } = match step - .aux_data - .as_ref() - .expect("could not find aux_data for COPYTOMEMORY") - { - StepAuxiliaryData::CopyToMemory(aux) => aux, + } = match step.aux_data { + Some(StepAuxiliaryData::CopyToMemory(aux)) => aux, _ => unreachable!("could not find CopyToMemory aux_data for COPYTOMEMORY"), }; self.src_addr - .assign(region, offset, Some(F::from(*src_addr)))?; + .assign(region, offset, Some(F::from(src_addr)))?; self.dst_addr - .assign(region, offset, Some(F::from(*dst_addr)))?; + .assign(region, offset, Some(F::from(dst_addr)))?; self.bytes_left - .assign(region, offset, Some(F::from(*bytes_left)))?; + .assign(region, offset, Some(F::from(bytes_left)))?; self.src_addr_end - .assign(region, offset, Some(F::from(*src_addr_end)))?; + .assign(region, offset, Some(F::from(src_addr_end)))?; self.from_tx - .assign(region, offset, Some(F::from(*from_tx as u64)))?; + .assign(region, offset, Some(F::from(from_tx as u64)))?; self.tx_id .assign(region, offset, Some(F::from(tx.id as u64)))?; @@ -199,11 +195,11 @@ impl ExecutionGadget for CopyToMemoryGadget { let mut rw_idx = 0; let mut bytes = vec![0u8; MAX_COPY_BYTES]; let mut selectors = vec![false; MAX_COPY_BYTES]; - for idx in 0..std::cmp::min(*bytes_left as usize, MAX_COPY_BYTES) { - let src_addr = *src_addr as usize + idx; + for idx in 0..std::cmp::min(bytes_left as usize, MAX_COPY_BYTES) { + let src_addr = src_addr as usize + idx; selectors[idx] = true; - bytes[idx] = if selectors[idx] && src_addr < *src_addr_end as usize { - if *from_tx { + bytes[idx] = if selectors[idx] && src_addr < src_addr_end as usize { + if from_tx { tx.call_data[src_addr] } else { rw_idx += 1; @@ -217,14 +213,14 @@ impl ExecutionGadget for CopyToMemoryGadget { } self.buffer_reader - .assign(region, offset, *src_addr, *src_addr_end, &bytes, &selectors)?; + .assign(region, offset, src_addr, src_addr_end, &bytes, &selectors)?; - let num_bytes_copied = std::cmp::min(*bytes_left, MAX_COPY_BYTES as u64); + let num_bytes_copied = std::cmp::min(bytes_left, MAX_COPY_BYTES as u64); self.finish_gadget.assign( region, offset, F::from(num_bytes_copied), - F::from(*bytes_left), + F::from(bytes_left), )?; Ok(()) diff --git a/zkevm-circuits/src/evm_circuit/witness.rs b/zkevm-circuits/src/evm_circuit/witness.rs index 8072e6b3a4..d43faac2c5 100644 --- a/zkevm-circuits/src/evm_circuit/witness.rs +++ b/zkevm-circuits/src/evm_circuit/witness.rs @@ -337,34 +337,31 @@ impl Bytecode { let mut rows = Vec::with_capacity(n); let hash = RandomLinearCombination::random_linear_combine(self.hash.to_le_bytes(), randomness); + + rows.push([ + hash, + F::from(BytecodeFieldTag::Length as u64), + F::zero(), + F::zero(), + F::from(self.bytes.len() as u64), + ]); + let mut push_data_left = 0; - for idx in 0..n { - if idx == 0 { - rows.push([ - hash, - F::from(BytecodeFieldTag::Length as u64), - F::zero(), - F::zero(), - F::from(self.bytes.len() as u64), - ]); - } else { - let i = idx - 1; - let byte = self.bytes[i]; - let mut is_code = true; - if push_data_left > 0 { - is_code = false; - push_data_left -= 1; - } else if (OpcodeId::PUSH1.as_u8()..=OpcodeId::PUSH32.as_u8()).contains(&byte) { - push_data_left = byte as usize - (OpcodeId::PUSH1.as_u8() - 1) as usize; - } - rows.push([ - hash, - F::from(BytecodeFieldTag::Byte as u64), - F::from(i as u64), - F::from(is_code as u64), - F::from(byte as u64), - ]) + for (idx, byte) in self.bytes.iter().enumerate() { + let mut is_code = true; + if push_data_left > 0 { + is_code = false; + push_data_left -= 1; + } else if (OpcodeId::PUSH1.as_u8()..=OpcodeId::PUSH32.as_u8()).contains(byte) { + push_data_left = *byte as usize - (OpcodeId::PUSH1.as_u8() - 1) as usize; } + rows.push([ + hash, + F::from(BytecodeFieldTag::Byte as u64), + F::from(idx as u64), + F::from(is_code as u64), + F::from(*byte as u64), + ]) } rows } @@ -1159,7 +1156,7 @@ fn step_convert(step: &circuit_input_builder::ExecStep) -> ExecStep { }, memory_size: step.memory_size as u64, reversible_write_counter: step.reversible_write_counter, - aux_data: step.aux_data.clone().map(Into::into), + aux_data: step.aux_data.map(Into::into), } }