From 5bac1f25d53a4f62186532493151d1723eb673a8 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Sat, 9 Apr 2022 11:34:13 +0800 Subject: [PATCH 1/5] fix: read from caller memory in case of internal call --- .../src/evm_circuit/execution/calldatacopy.rs | 53 +++++++++++-------- .../src/evm_circuit/execution/memory_copy.rs | 50 ++++++++--------- 2 files changed, 55 insertions(+), 48 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 9f78367f90..c90f076db9 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -29,7 +29,7 @@ pub(crate) struct CallDataCopyGadget { same_context: SameContextGadget, memory_address: MemoryAddressGadget, data_offset: MemoryAddress, - tx_id: Cell, + src_id: Cell, call_data_length: Cell, call_data_offset: Cell, // Only used in the internal call memory_expansion: MemoryExpansionGadget, @@ -54,15 +54,16 @@ impl ExecutionGadget for CallDataCopyGadget { cb.stack_pop(length.expr()); let memory_address = MemoryAddressGadget::construct(cb, memory_offset, length); - let tx_id = cb.call_context(None, CallContextFieldTag::TxId); + let src_id = cb.query_cell(); let call_data_length = cb.query_cell(); let call_data_offset = cb.query_cell(); // Lookup the calldata_length and caller_address in Tx context table or // Call context table cb.condition(cb.curr.state.is_root.expr(), |cb| { + cb.call_context_lookup(false.expr(), None, CallContextFieldTag::TxId, src_id.expr()); cb.tx_context_lookup( - tx_id.expr(), + src_id.expr(), TxContextFieldTag::CallDataLength, None, call_data_length.expr(), @@ -70,9 +71,15 @@ impl ExecutionGadget for CallDataCopyGadget { cb.require_zero( "call_data_offset == 0 in the root call", call_data_offset.expr(), - ) + ); }); cb.condition(1.expr() - cb.curr.state.is_root.expr(), |cb| { + cb.call_context_lookup( + false.expr(), + None, + CallContextFieldTag::CallerId, + src_id.expr(), + ); cb.call_context_lookup( false.expr(), None, @@ -84,7 +91,7 @@ impl ExecutionGadget for CallDataCopyGadget { None, CallContextFieldTag::CallDataOffset, call_data_offset.expr(), - ) + ); }); // Calculate the next memory size and the gas cost for this memory @@ -110,7 +117,7 @@ impl ExecutionGadget for CallDataCopyGadget { let next_bytes_left = cb.query_cell(); let next_src_addr_end = cb.query_cell(); let next_from_tx = cb.query_cell(); - let next_tx_id = cb.query_cell(); + let next_src_id = cb.query_cell(); cb.require_equal( "next_src_addr = data_offset + call_data_offset", next_src_addr.expr(), @@ -136,7 +143,7 @@ impl ExecutionGadget for CallDataCopyGadget { next_from_tx.expr(), cb.curr.state.is_root.expr(), ); - cb.require_equal("next_tx_id = tx_id", next_tx_id.expr(), tx_id.expr()); + cb.require_equal("next_src_id = src_id", next_src_id.expr(), src_id.expr()); }, ); @@ -158,7 +165,7 @@ impl ExecutionGadget for CallDataCopyGadget { same_context, memory_address, data_offset, - tx_id, + src_id, call_data_length, call_data_offset, memory_expansion, @@ -192,8 +199,9 @@ impl ExecutionGadget for CallDataCopyGadget { .unwrap(), ), )?; - self.tx_id - .assign(region, offset, Some(F::from(tx.id as u64)))?; + let src_id = if call.is_root { tx.id } else { call.caller_id }; + self.src_id + .assign(region, offset, Some(F::from(src_id as u64)))?; // Call data length and call data offset let (call_data_length, call_data_offset) = if call.is_root { @@ -228,7 +236,7 @@ impl ExecutionGadget for CallDataCopyGadget { #[cfg(test)] mod test { use crate::evm_circuit::{ - execution::memory_copy::test::make_memory_copy_steps, + execution::memory_copy::test::{make_memory_copy_steps, CALLER_ID, CALL_ID, TX_ID}, step::ExecutionState, table::{CallContextFieldTag, RwTableTag}, test::{rand_bytes, run_test_circuit_incomplete_fixed_table}, @@ -292,7 +300,6 @@ mod test { ] .concat(), ); - let call_id = 1; let call_data = rand_bytes(call_data_length.as_usize()); let mut rws = RwMap( @@ -303,21 +310,21 @@ mod test { Rw::Stack { rw_counter: 1, is_write: false, - call_id, + call_id: CALL_ID, stack_pointer: 1021, value: memory_offset, }, Rw::Stack { rw_counter: 2, is_write: false, - call_id, + call_id: CALL_ID, stack_pointer: 1022, value: data_offset, }, Rw::Stack { rw_counter: 3, is_write: false, - call_id, + call_id: CALL_ID, stack_pointer: 1023, value: length, }, @@ -329,21 +336,21 @@ mod test { Rw::CallContext { rw_counter: 4, is_write: false, - call_id, - field_tag: CallContextFieldTag::TxId, - value: Word::one(), + call_id: CALL_ID, + field_tag: CallContextFieldTag::CallerId, + value: Word::from(CALLER_ID), }, Rw::CallContext { rw_counter: 5, is_write: false, - call_id, + call_id: CALL_ID, field_tag: CallContextFieldTag::CallDataLength, value: call_data_length, }, Rw::CallContext { rw_counter: 6, is_write: false, - call_id, + call_id: CALL_ID, field_tag: CallContextFieldTag::CallDataOffset, value: call_data_offset, }, @@ -392,7 +399,6 @@ mod test { if !length.is_zero() { make_memory_copy_steps( - call_id, &call_data, call_data_offset.as_u64(), call_data_offset.as_u64() + data_offset.as_u64(), @@ -421,14 +427,15 @@ mod test { let block = Block { randomness, txs: vec![Transaction { - id: 1, + id: TX_ID, calls: vec![Call { - id: call_id, + id: CALL_ID, is_root: false, is_create: false, call_data_length: call_data_length.as_u64(), call_data_offset: call_data_offset.as_u64(), code_source: CodeSource::Account(bytecode.hash), + caller_id: CALLER_ID, ..Default::default() }], steps, diff --git a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs index 1624b34aa8..474a73c365 100644 --- a/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/memory_copy.rs @@ -35,8 +35,9 @@ pub(crate) struct CopyToMemoryGadget { src_addr_end: Cell, // Indicate whether src is from Tx Calldata from_tx: Cell, - // Transaction ID, optional, only used when src_is_tx == 1 - tx_id: Cell, + // Source from where we read the bytes. This equals the tx ID in case of a root call, or caller + // ID in case of an internal call + src_id: Cell, // Buffer reader gadget buffer_reader: BufferReaderGadget, // The comparison gadget between num bytes copied and bytes_left @@ -54,7 +55,7 @@ impl ExecutionGadget for CopyToMemoryGadget { let bytes_left = cb.query_cell(); let src_addr_end = cb.query_cell(); let from_tx = cb.query_bool(); - let tx_id = cb.query_cell(); + let src_id = cb.query_cell(); let buffer_reader = BufferReaderGadget::construct(cb, src_addr.expr(), src_addr_end.expr()); let from_memory = 1.expr() - from_tx.expr(); @@ -67,13 +68,13 @@ impl ExecutionGadget for CopyToMemoryGadget { 0.expr(), src_addr.expr() + i.expr(), buffer_reader.byte(i), - None, + Some(src_id.expr()), ) }); // Read bytes[i] from Tx cb.condition(from_tx.expr() * read_flag.clone(), |cb| { cb.tx_context_lookup( - tx_id.expr(), + src_id.expr(), TxContextFieldTag::CallData, Some(src_addr.expr() + i.expr()), buffer_reader.byte(i), @@ -109,7 +110,7 @@ impl ExecutionGadget for CopyToMemoryGadget { let next_bytes_left = cb.query_cell(); let next_src_addr_end = cb.query_cell(); let next_from_tx = cb.query_cell(); - let next_tx_id = cb.query_cell(); + let next_src_id = cb.query_cell(); cb.require_equal( "next_src_addr == src_addr + copied_size", next_src_addr.expr(), @@ -135,7 +136,7 @@ impl ExecutionGadget for CopyToMemoryGadget { next_from_tx.expr(), from_tx.expr(), ); - cb.require_equal("next_tx_id == tx_id", next_tx_id.expr(), tx_id.expr()); + cb.require_equal("next_src_id == src_id", next_src_id.expr(), src_id.expr()); }, ); @@ -152,7 +153,7 @@ impl ExecutionGadget for CopyToMemoryGadget { bytes_left, src_addr_end, from_tx, - tx_id, + src_id, buffer_reader, finish_gadget, } @@ -164,7 +165,7 @@ impl ExecutionGadget for CopyToMemoryGadget { offset: usize, block: &Block, tx: &Transaction, - _: &Call, + call: &Call, step: &ExecStep, ) -> Result<(), Error> { let CopyToMemoryAuxData { @@ -188,8 +189,9 @@ impl ExecutionGadget for CopyToMemoryGadget { .assign(region, offset, Some(F::from(src_addr_end)))?; self.from_tx .assign(region, offset, Some(F::from(from_tx as u64)))?; - self.tx_id - .assign(region, offset, Some(F::from(tx.id as u64)))?; + let src_id = if call.is_root { tx.id } else { call.caller_id }; + self.src_id + .assign(region, offset, Some(F::from(src_id as u64)))?; // Fill in selectors and bytes let mut rw_idx = 0; @@ -242,9 +244,12 @@ pub mod test { use pairing::bn256::Fr as Fp; use std::collections::HashMap; + pub(crate) const CALL_ID: usize = 1; + pub(crate) const CALLER_ID: usize = 0; + pub(crate) const TX_ID: usize = 1; + #[allow(clippy::too_many_arguments)] fn make_memory_copy_step( - call_id: usize, src_addr: u64, dst_addr: u64, src_addr_end: u64, @@ -268,7 +273,7 @@ pub mod test { memory_rws.push(Rw::Memory { rw_counter: rw_counter + rw_offset, is_write: false, - call_id, + call_id: CALLER_ID, memory_address: src_addr + idx as u64, byte: bytes_map[&addr], }); @@ -282,7 +287,7 @@ pub mod test { memory_rws.push(Rw::Memory { rw_counter: rw_counter + rw_offset, is_write: true, - call_id, + call_id: CALL_ID, memory_address: dst_addr + idx as u64, byte, }); @@ -314,7 +319,6 @@ pub mod test { #[allow(clippy::too_many_arguments)] pub(crate) fn make_memory_copy_steps( - call_id: usize, buffer: &[u8], buffer_addr: u64, // buffer base address, use 0 for tx calldata src_addr: u64, @@ -336,7 +340,6 @@ pub mod test { let mut copied = 0; while copied < length { let (step, rw_offset) = make_memory_copy_step( - call_id, src_addr + copied as u64, dst_addr + copied as u64, buffer_addr_end, @@ -358,7 +361,6 @@ pub mod test { fn test_ok_from_memory(src_addr: u64, dst_addr: u64, src_addr_end: u64, length: usize) { let randomness = Fp::rand(); let bytecode = Bytecode::new(vec![OpcodeId::STOP.as_u8()]); - let call_id = 1; let mut rws = RwMap(Default::default()); let mut rw_counter = 1; let mut steps = Vec::new(); @@ -366,7 +368,6 @@ pub mod test { let memory_size = (dst_addr + length as u64 + 31) / 32 * 32; make_memory_copy_steps( - call_id, &buffer, src_addr, src_addr, @@ -394,12 +395,13 @@ pub mod test { let block = Block { randomness, txs: vec![Transaction { - id: 1, + id: TX_ID, calls: vec![Call { - id: call_id, - is_root: true, + id: CALL_ID, + is_root: false, is_create: false, code_source: CodeSource::Account(bytecode.hash), + caller_id: CALLER_ID, ..Default::default() }], steps, @@ -415,7 +417,6 @@ pub mod test { fn test_ok_from_tx(calldata_length: usize, src_addr: u64, dst_addr: u64, length: usize) { let randomness = Fp::rand(); let bytecode = Bytecode::new(vec![OpcodeId::STOP.as_u8(), OpcodeId::STOP.as_u8()]); - let call_id = 1; let mut rws = RwMap(Default::default()); let mut rw_counter = 1; let calldata: Vec = rand_bytes(calldata_length); @@ -423,7 +424,6 @@ pub mod test { let memory_size = (dst_addr + length as u64 + 31) / 32 * 32; make_memory_copy_steps( - call_id, &calldata, 0, src_addr, @@ -451,11 +451,11 @@ pub mod test { let block = Block { randomness, txs: vec![Transaction { - id: 1, + id: TX_ID, call_data: calldata, call_data_length: calldata_length, calls: vec![Call { - id: call_id, + id: CALL_ID, is_root: true, is_create: false, code_source: CodeSource::Account(bytecode.hash), From cc02aa0e75f870c3920d90f85af4aa2316256c7b Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Mon, 11 Apr 2022 16:03:24 +0800 Subject: [PATCH 2/5] fix: call data length comes from call context --- Cargo.lock | 1 + bus-mapping/Cargo.toml | 1 + bus-mapping/src/evm/opcodes/calldatacopy.rs | 129 ++++++++++++++---- .../src/evm_circuit/execution/calldatacopy.rs | 46 ++++++- 4 files changed, 149 insertions(+), 28 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dbdea4a487..a10902b7cc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -338,6 +338,7 @@ dependencies = [ "eth-types", "ethers-core", "ethers-providers", + "hex", "itertools", "keccak256", "lazy_static", diff --git a/bus-mapping/Cargo.toml b/bus-mapping/Cargo.toml index 0acfa70ee2..d1fecd29ed 100644 --- a/bus-mapping/Cargo.toml +++ b/bus-mapping/Cargo.toml @@ -17,6 +17,7 @@ serde = {version = "1.0.130", features = ["derive"] } serde_json = "1.0.66" [dev-dependencies] +hex = "0.4.3" mock = { path = "../mock" } pretty_assertions = "1.0.0" tokio = { version = "1.13", features = ["macros"] } diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 4b9699b3e4..73f83b4b67 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -53,17 +53,36 @@ fn gen_calldatacopy_step( geth_step.stack.nth_last_filled(2), length, )?; - state.push_op( - &mut exec_step, - RW::READ, - CallContextOp { - call_id: state.call()?.call_id, - field: CallContextField::TxId, - value: state.tx_ctx.id().into(), - }, - ); - if !state.call()?.is_root { + if state.call()?.is_root { + state.push_op( + &mut exec_step, + RW::READ, + CallContextOp { + call_id: state.call()?.call_id, + field: CallContextField::TxId, + value: state.tx_ctx.id().into(), + }, + ); + state.push_op( + &mut exec_step, + RW::READ, + CallContextOp { + call_id: state.call()?.call_id, + field: CallContextField::CallDataLength, + value: state.call()?.call_data_length.into(), + }, + ); + } else { + state.push_op( + &mut exec_step, + RW::READ, + CallContextOp { + call_id: state.call()?.call_id, + field: CallContextField::CallerId, + value: state.call()?.caller_id.into(), + }, + ); state.push_op( &mut exec_step, RW::READ, @@ -83,6 +102,7 @@ fn gen_calldatacopy_step( }, ); }; + Ok(exec_step) } @@ -178,8 +198,58 @@ mod calldatacopy_tests { use mock::test_ctx::{helpers::*, TestContext}; use pretty_assertions::assert_eq; + // Test must be enabled and should pass once `CREATE` is handled. #[test] - fn calldatacopy_opcode_impl() { + #[ignore] + fn calldatacopy_opcode_internal() { + let creator_code: Vec = hex::decode("666020600060003760005260076019F3").unwrap(); + let code = bytecode! { + // 1. Store the following bytes to memory + PUSH16(Word::from_big_endian(&creator_code)) + PUSH1(0x00) // offset + MSTORE + // 2. Create a contract with code: 6020 6000 6000 37 + PUSH1(0x10) // size + PUSH1(0x10) // offset + PUSH1(0x00) // value + CREATE + // 3. Call created contract, i.e. CALLDATACOPY (37) is in internal call + PUSH1(0x00) // retSize + PUSH1(0x00) // retOffset + PUSH1(0x20) // argsSize + PUSH1(0x00) // argsOffset + PUSH1(0x00) // value + DUP6 // address + PUSH2(0xFFFF) // gas + CALL + }; + + // Get the execution steps from the external tracer + let block: GethData = TestContext::<2, 1>::new( + None, + account_0_code_account_1_no_code(code), + tx_from_1_to_0, + |block, _tx| block.number(0xcafeu64), + ) + .unwrap() + .into(); + + let mut builder = BlockData::new_from_geth_data(block.clone()).new_circuit_input_builder(); + builder + .handle_block(&block.eth_block, &block.geth_traces) + .unwrap(); + + let step = builder.block.txs()[0] + .steps() + .iter() + .find(|step| step.exec_state == ExecState::Op(OpcodeId::CALLDATACOPY)) + .unwrap(); + + println!("{:#?}", step); + } + + #[test] + fn calldatacopy_opcode_root() { let code = bytecode! { PUSH32(0) PUSH32(0) @@ -209,6 +279,8 @@ mod calldatacopy_tests { .find(|step| step.exec_state == ExecState::Op(OpcodeId::CALLDATACOPY)) .unwrap(); + assert_eq!(step.bus_mapping_instance.len(), 5); + assert_eq!( [0, 1, 2] .map(|idx| &builder.block.container.stack[step.bus_mapping_instance[idx].as_usize()]) @@ -230,19 +302,28 @@ mod calldatacopy_tests { ); assert_eq!( - { - let operation = - &builder.block.container.call_context[step.bus_mapping_instance[3].as_usize()]; - (operation.rw(), operation.op()) - }, - ( - RW::READ, - &CallContextOp { - call_id: builder.block.txs()[0].calls()[0].call_id, - field: CallContextField::TxId, - value: Word::from(1), - } - ) + [3, 4] + .map(|idx| &builder.block.container.call_context + [step.bus_mapping_instance[idx].as_usize()]) + .map(|operation| (operation.rw(), operation.op())), + [ + ( + RW::READ, + &CallContextOp { + call_id: builder.block.txs()[0].calls()[0].call_id, + field: CallContextField::TxId, + value: Word::from(1), + } + ), + ( + RW::READ, + &CallContextOp { + call_id: builder.block.txs()[0].calls()[0].call_id, + field: CallContextField::CallDataLength, + value: Word::zero(), + }, + ), + ] ); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index c90f076db9..090b06f08d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -3,7 +3,7 @@ use crate::{ execution::ExecutionGadget, param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, step::ExecutionState, - table::{CallContextFieldTag, TxContextFieldTag}, + table::CallContextFieldTag, util::{ common_gadget::SameContextGadget, constraint_builder::{ @@ -62,10 +62,10 @@ impl ExecutionGadget for CallDataCopyGadget { // Call context table cb.condition(cb.curr.state.is_root.expr(), |cb| { cb.call_context_lookup(false.expr(), None, CallContextFieldTag::TxId, src_id.expr()); - cb.tx_context_lookup( - src_id.expr(), - TxContextFieldTag::CallDataLength, + cb.call_context_lookup( + false.expr(), None, + CallContextFieldTag::CallDataLength, call_data_length.expr(), ); cb.require_zero( @@ -495,4 +495,42 @@ mod test { Word::from(0), ); } + + // This test must be enabled and should pass once `CREATE` is handled in + // bus-mapping. + #[test] + #[ignore] + fn calldatacopy_gadget_busmapping_internal() { + let creator_code: Vec = hex::decode("666020600060003760005260076019F3").unwrap(); + let nested_code = bytecode! { + // 1. Store the following bytes to memory + PUSH16(Word::from_big_endian(&creator_code)) + PUSH1(0x00) // offset + MSTORE + // 2. Create a contract with code: 6020 6000 6000 37 + PUSH1(0x10) // size + PUSH1(0x10) // offset + PUSH1(0x00) // value + CREATE + // 3. Call created contract, i.e. CALLDATACOPY (37) is in internal call + PUSH1(0x00) // retSize + PUSH1(0x00) // retOffset + PUSH1(0x20) // argsSize + PUSH1(0x00) // argsOffset + PUSH1(0x00) // value + DUP6 // address + PUSH2(0xFFFF) // gas + CALL + }; + + let ctx = TestContext::<2, 1>::new( + None, + account_0_code_account_1_no_code(nested_code), + tx_from_1_to_0, + |block, _tx| block.number(0xcafeu64), + ) + .unwrap(); + + assert_eq!(run_test_circuits(ctx, None), Ok(())); + } } From 092eb6507017ef9a6612e18ef04c9b5c79137bfb Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Tue, 12 Apr 2022 11:58:43 +0800 Subject: [PATCH 3/5] tests: internal call tests for bus-mapping --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 171 +++++++++++++++--- .../src/evm_circuit/execution/calldatacopy.rs | 66 ++++--- 2 files changed, 188 insertions(+), 49 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 44434905d5..7efaa12983 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -185,49 +185,72 @@ mod calldatacopy_tests { use crate::{ circuit_input_builder::ExecState, mock::BlockData, - operation::{CallContextField, CallContextOp, StackOp, RW}, + operation::{CallContextField, CallContextOp, MemoryOp, StackOp, RW}, }; use eth_types::{ - bytecode, + address, bytecode, evm_types::{OpcodeId, StackAddress}, geth_types::GethData, - Word, + Address, ToWord, Word, }; use mock::test_ctx::{helpers::*, TestContext}; use pretty_assertions::assert_eq; - // Test must be enabled and should pass once `CREATE` is handled. #[test] - #[ignore] fn calldatacopy_opcode_internal() { - let creator_code: Vec = hex::decode("666020600060003760005260076019F3").unwrap(); - let code = bytecode! { - // 1. Store the following bytes to memory - PUSH16(Word::from_big_endian(&creator_code)) + let ADDR_A = address!("0x000000000000000000000000000000000cafe00a"); + let ADDR_B = address!("0x000000000000000000000000000000000cafe00b"); + + // code B gets called by code A, so the call is an internal call. + let dst_offset = 0x00usize; + let offset = 0x00usize; + let copy_size = 0x10usize; + let code_b = bytecode! { + PUSH32(copy_size) // size + PUSH32(offset) // offset + PUSH32(dst_offset) // dst_offset + CALLDATACOPY + STOP + }; + + // code A calls code B. + let pushdata = hex::decode("1234567890abcdef").unwrap(); + let memory_a = std::iter::repeat(0) + .take(24) + .chain(pushdata.clone()) + .collect::>(); + let call_data_length = 0x20usize; + let call_data_offset = 0x10usize; + let code_a = bytecode! { + // populate memory in A's context. + PUSH8(Word::from_big_endian(&pushdata)) PUSH1(0x00) // offset MSTORE - // 2. Create a contract with code: 6020 6000 6000 37 - PUSH1(0x10) // size - PUSH1(0x10) // offset + // call ADDR_B. + PUSH1(0x00) // retLength + PUSH1(0x00) // retOffset + PUSH1(call_data_length) // argsLength + PUSH1(call_data_offset) // argsOffset PUSH1(0x00) // value - CREATE - // 3. Call created contract, i.e. CALLDATACOPY (37) is in internal call - PUSH1(0x00) // retSize - PUSH1(0x00) // retOffset - PUSH1(0x20) // argsSize - PUSH1(0x00) // argsOffset - PUSH1(0x00) // value - DUP6 // address - PUSH2(0xFFFF) // gas + PUSH32(ADDR_B.to_word()) // addr + PUSH32(0x1_0000) // gas CALL }; // Get the execution steps from the external tracer - let block: GethData = TestContext::<2, 1>::new( + let block: GethData = TestContext::<3, 1>::new( None, - account_0_code_account_1_no_code(code), - tx_from_1_to_0, + |accs| { + accs[0].address(ADDR_B).code(code_b); + accs[1].address(ADDR_A).code(code_a); + accs[2] + .address(Address::random()) + .balance(Word::from(1u64 << 30)); + }, + |mut txs, accs| { + txs[0].to(accs[1].address).from(accs[2].address); + }, |block, _tx| block.number(0xcafeu64), ) .unwrap() @@ -244,7 +267,105 @@ mod calldatacopy_tests { .find(|step| step.exec_state == ExecState::Op(OpcodeId::CALLDATACOPY)) .unwrap(); - println!("{:#?}", step); + let caller_id = builder.block.txs()[0].calls()[step.call_index].caller_id; + let expected_call_id = builder.block.txs()[0].calls()[step.call_index].call_id; + + // 3 stack reads + 3 call context reads. + assert_eq!(step.bus_mapping_instance.len(), 6); + + // 3 stack reads. + assert_eq!( + [0, 1, 2] + .map(|idx| &builder.block.container.stack[step.bus_mapping_instance[idx].as_usize()]) + .map(|operation| (operation.rw(), operation.op())), + [ + ( + RW::READ, + &StackOp::new(expected_call_id, StackAddress::from(1021), Word::from(dst_offset)) + ), + ( + RW::READ, + &StackOp::new(expected_call_id, StackAddress::from(1022), Word::from(offset)) + ), + ( + RW::READ, + &StackOp::new(expected_call_id, StackAddress::from(1023), Word::from(copy_size)) + ), + ] + ); + + // 3 call context reads. + assert_eq!( + [3, 4, 5] + .map(|idx| &builder.block.container.call_context + [step.bus_mapping_instance[idx].as_usize()]) + .map(|operation| (operation.rw(), operation.op())), + [ + ( + RW::READ, + &CallContextOp { + call_id: expected_call_id, + field: CallContextField::CallerId, + value: Word::from(1), + } + ), + ( + RW::READ, + &CallContextOp { + call_id: expected_call_id, + field: CallContextField::CallDataLength, + value: Word::from(call_data_length), + }, + ), + ( + RW::READ, + &CallContextOp { + call_id: expected_call_id, + field: CallContextField::CallDataOffset, + value: Word::from(call_data_offset), + }, + ), + ] + ); + + // memory writes. + // 1. First `call_data_length` memory ops are RW::WRITE and come from the `CALL` + // opcode. (we skip checking those) + // 2. Following that, we should have tuples of (RW::READ and RW::WRITE) where + // the caller memory is read and the current call's memory is written + // to. + assert_eq!( + builder.block.container.memory.len(), + call_data_length + 2 * copy_size + ); + assert_eq!( + (call_data_length..(call_data_length + (2 * copy_size))) + .map(|idx| &builder.block.container.memory[idx]) + .map(|op| (op.rw(), op.op().clone())) + .collect::>(), + { + let mut memory_ops = Vec::with_capacity(2 * copy_size); + (0..copy_size).for_each(|idx| { + memory_ops.push(( + RW::READ, + MemoryOp::new( + caller_id, + (call_data_offset + offset + idx).into(), + memory_a[call_data_offset + idx], + ), + )); + memory_ops.push(( + RW::WRITE, + MemoryOp::new( + expected_call_id, + (dst_offset + idx).into(), + memory_a[call_data_offset + idx], + ), + )); + }); + memory_ops + }, + ); } #[test] diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 090b06f08d..4f094e55bd 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -244,9 +244,9 @@ mod test { }; use crate::test_util::run_test_circuits; use eth_types::{ - bytecode, + address, bytecode, evm_types::{gas_utils::memory_copier_gas_cost, GasCost, OpcodeId}, - ToBigEndian, Word, + Address, ToBigEndian, ToWord, Word, }; use halo2_proofs::arithmetic::BaseExt; use mock::test_ctx::{helpers::*, TestContext}; @@ -496,37 +496,55 @@ mod test { ); } - // This test must be enabled and should pass once `CREATE` is handled in - // bus-mapping. #[test] - #[ignore] fn calldatacopy_gadget_busmapping_internal() { - let creator_code: Vec = hex::decode("666020600060003760005260076019F3").unwrap(); - let nested_code = bytecode! { - // 1. Store the following bytes to memory - PUSH16(Word::from_big_endian(&creator_code)) + let addr_a = address!("0x000000000000000000000000000000000cafe00a"); + let addr_b = address!("0x000000000000000000000000000000000cafe00b"); + + // code B gets called by code A, so the call is an internal call. + let dst_offset = 0x00usize; + let offset = 0x00usize; + let copy_size = 0x10usize; + let code_b = bytecode! { + PUSH32(copy_size) // size + PUSH32(offset) // offset + PUSH32(dst_offset) // dst_offset + CALLDATACOPY + STOP + }; + + // code A calls code B. + let pushdata = hex::decode("1234567890abcdef").unwrap(); + let call_data_length = 0x20usize; + let call_data_offset = 0x10usize; + let code_a = bytecode! { + // populate memory in A's context. + PUSH8(Word::from_big_endian(&pushdata)) PUSH1(0x00) // offset MSTORE - // 2. Create a contract with code: 6020 6000 6000 37 - PUSH1(0x10) // size - PUSH1(0x10) // offset + // call ADDR_B. + PUSH1(0x00) // retLength + PUSH1(0x00) // retOffset + PUSH1(call_data_length) // argsLength + PUSH1(call_data_offset) // argsOffset PUSH1(0x00) // value - CREATE - // 3. Call created contract, i.e. CALLDATACOPY (37) is in internal call - PUSH1(0x00) // retSize - PUSH1(0x00) // retOffset - PUSH1(0x20) // argsSize - PUSH1(0x00) // argsOffset - PUSH1(0x00) // value - DUP6 // address - PUSH2(0xFFFF) // gas + PUSH32(addr_b.to_word()) // addr + PUSH32(0x1_0000) // gas CALL }; - let ctx = TestContext::<2, 1>::new( + let ctx = TestContext::<3, 1>::new( None, - account_0_code_account_1_no_code(nested_code), - tx_from_1_to_0, + |accs| { + accs[0].address(addr_b).code(code_b); + accs[1].address(addr_a).code(code_a); + accs[2] + .address(Address::random()) + .balance(Word::from(1u64 << 30)); + }, + |mut txs, accs| { + txs[0].to(accs[1].address).from(accs[2].address); + }, |block, _tx| block.number(0xcafeu64), ) .unwrap(); From 0e9b97d00c786c4b7b4a5950b763ce722d146fff Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Tue, 12 Apr 2022 12:31:02 +0800 Subject: [PATCH 4/5] fix: include range64 and range1024 in fixed tables --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 1 + zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs | 1 + zkevm-circuits/src/test_util.rs | 2 ++ 3 files changed, 4 insertions(+) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 7efaa12983..50738f5a5d 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -236,6 +236,7 @@ mod calldatacopy_tests { PUSH32(ADDR_B.to_word()) // addr PUSH32(0x1_0000) // gas CALL + STOP }; // Get the execution steps from the external tracer diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 4f094e55bd..31b3f56eb6 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -531,6 +531,7 @@ mod test { PUSH32(addr_b.to_word()) // addr PUSH32(0x1_0000) // gas CALL + STOP }; let ctx = TestContext::<3, 1>::new( diff --git a/zkevm-circuits/src/test_util.rs b/zkevm-circuits/src/test_util.rs index 662d87907f..78f60a97de 100644 --- a/zkevm-circuits/src/test_util.rs +++ b/zkevm-circuits/src/test_util.rs @@ -26,8 +26,10 @@ pub fn get_fixed_table(conf: FixedTableConfig) -> Vec { FixedTableTag::Range5, FixedTableTag::Range16, FixedTableTag::Range32, + FixedTableTag::Range64, FixedTableTag::Range256, FixedTableTag::Range512, + FixedTableTag::Range1024, FixedTableTag::SignByte, FixedTableTag::ResponsibleOpcode, ] From d848d8c9bba2c0972880e4eb25eb414ec3d614f6 Mon Sep 17 00:00:00 2001 From: Rohit Narurkar Date: Tue, 12 Apr 2022 15:55:32 +0800 Subject: [PATCH 5/5] refactor: remove redundant tests --- bus-mapping/src/evm/opcodes/calldatacopy.rs | 12 +- .../src/evm_circuit/execution/calldatacopy.rs | 274 +++--------------- 2 files changed, 43 insertions(+), 243 deletions(-) diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 50738f5a5d..17aa358769 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -199,8 +199,8 @@ mod calldatacopy_tests { #[test] fn calldatacopy_opcode_internal() { - let ADDR_A = address!("0x000000000000000000000000000000000cafe00a"); - let ADDR_B = address!("0x000000000000000000000000000000000cafe00b"); + let addr_a = address!("0x000000000000000000000000000000000cafe00a"); + let addr_b = address!("0x000000000000000000000000000000000cafe00b"); // code B gets called by code A, so the call is an internal call. let dst_offset = 0x00usize; @@ -227,13 +227,13 @@ mod calldatacopy_tests { PUSH8(Word::from_big_endian(&pushdata)) PUSH1(0x00) // offset MSTORE - // call ADDR_B. + // call addr_b. PUSH1(0x00) // retLength PUSH1(0x00) // retOffset PUSH1(call_data_length) // argsLength PUSH1(call_data_offset) // argsOffset PUSH1(0x00) // value - PUSH32(ADDR_B.to_word()) // addr + PUSH32(addr_b.to_word()) // addr PUSH32(0x1_0000) // gas CALL STOP @@ -243,8 +243,8 @@ mod calldatacopy_tests { let block: GethData = TestContext::<3, 1>::new( None, |accs| { - accs[0].address(ADDR_B).code(code_b); - accs[1].address(ADDR_A).code(code_a); + accs[0].address(addr_b).code(code_b); + accs[1].address(addr_a).code(code_a); accs[2] .address(Address::random()) .balance(Word::from(1u64 << 30)); diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 31b3f56eb6..67a8d98088 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -235,24 +235,16 @@ impl ExecutionGadget for CallDataCopyGadget { #[cfg(test)] mod test { - use crate::evm_circuit::{ - execution::memory_copy::test::{make_memory_copy_steps, CALLER_ID, CALL_ID, TX_ID}, - step::ExecutionState, - table::{CallContextFieldTag, RwTableTag}, - test::{rand_bytes, run_test_circuit_incomplete_fixed_table}, - witness::{Block, Bytecode, Call, CodeSource, ExecStep, Rw, RwMap, Transaction}, - }; - use crate::test_util::run_test_circuits; - use eth_types::{ - address, bytecode, - evm_types::{gas_utils::memory_copier_gas_cost, GasCost, OpcodeId}, - Address, ToBigEndian, ToWord, Word, - }; - use halo2_proofs::arithmetic::BaseExt; + use crate::{evm_circuit::test::rand_bytes, test_util::run_test_circuits}; + use eth_types::{address, bytecode, Address, ToWord, Word}; use mock::test_ctx::{helpers::*, TestContext}; - use pairing::bn256::Fr as Fp; - fn test_ok_root(call_data_length: usize, memory_offset: Word, data_offset: Word, length: Word) { + fn test_ok_root( + call_data_length: usize, + memory_offset: usize, + data_offset: usize, + length: usize, + ) { let bytecode = bytecode! { PUSH32(length) PUSH32(data_offset) @@ -281,230 +273,16 @@ mod test { } fn test_ok_internal( - call_data_offset: Word, - call_data_length: Word, - memory_offset: Word, - data_offset: Word, - length: Word, + call_data_offset: usize, + call_data_length: usize, + dst_offset: usize, + offset: usize, + copy_size: usize, ) { - let randomness = Fp::rand(); - let bytecode = Bytecode::new( - [ - vec![OpcodeId::PUSH32.as_u8()], - length.to_be_bytes().to_vec(), - vec![OpcodeId::PUSH32.as_u8()], - data_offset.to_be_bytes().to_vec(), - vec![OpcodeId::PUSH32.as_u8()], - memory_offset.to_be_bytes().to_vec(), - vec![OpcodeId::CALLDATACOPY.as_u8(), OpcodeId::STOP.as_u8()], - ] - .concat(), - ); - let call_data = rand_bytes(call_data_length.as_usize()); - - let mut rws = RwMap( - [ - ( - RwTableTag::Stack, - vec![ - Rw::Stack { - rw_counter: 1, - is_write: false, - call_id: CALL_ID, - stack_pointer: 1021, - value: memory_offset, - }, - Rw::Stack { - rw_counter: 2, - is_write: false, - call_id: CALL_ID, - stack_pointer: 1022, - value: data_offset, - }, - Rw::Stack { - rw_counter: 3, - is_write: false, - call_id: CALL_ID, - stack_pointer: 1023, - value: length, - }, - ], - ), - ( - RwTableTag::CallContext, - vec![ - Rw::CallContext { - rw_counter: 4, - is_write: false, - call_id: CALL_ID, - field_tag: CallContextFieldTag::CallerId, - value: Word::from(CALLER_ID), - }, - Rw::CallContext { - rw_counter: 5, - is_write: false, - call_id: CALL_ID, - field_tag: CallContextFieldTag::CallDataLength, - value: call_data_length, - }, - Rw::CallContext { - rw_counter: 6, - is_write: false, - call_id: CALL_ID, - field_tag: CallContextFieldTag::CallDataOffset, - value: call_data_offset, - }, - ], - ), - ] - .into(), - ); - let mut rw_counter = 7; - - let curr_memory_word_size = - (call_data_length.as_u64() + call_data_length.as_u64() + 31) / 32; - let next_memory_word_size = if length.is_zero() { - curr_memory_word_size - } else { - std::cmp::max( - curr_memory_word_size, - (memory_offset.as_u64() + length.as_u64() + 31) / 32, - ) - }; - let gas_cost = GasCost::FASTEST.as_u64() - + memory_copier_gas_cost( - curr_memory_word_size, - next_memory_word_size, - length.as_u64(), - ); - let mut steps = vec![ExecStep { - rw_indices: vec![ - (RwTableTag::Stack, 0), - (RwTableTag::Stack, 1), - (RwTableTag::Stack, 2), - (RwTableTag::CallContext, 0), - (RwTableTag::CallContext, 1), - (RwTableTag::CallContext, 2), - ], - execution_state: ExecutionState::CALLDATACOPY, - rw_counter: 1, - program_counter: 99, - stack_pointer: 1021, - gas_left: gas_cost, - gas_cost, - memory_size: curr_memory_word_size * 32, - opcode: Some(OpcodeId::CALLDATACOPY), - ..Default::default() - }]; - - if !length.is_zero() { - make_memory_copy_steps( - &call_data, - call_data_offset.as_u64(), - call_data_offset.as_u64() + data_offset.as_u64(), - memory_offset.as_u64(), - length.as_usize(), - false, - 100, - 1024, - next_memory_word_size * 32, - &mut rw_counter, - &mut rws, - &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: TX_ID, - calls: vec![Call { - id: CALL_ID, - is_root: false, - is_create: false, - call_data_length: call_data_length.as_u64(), - call_data_offset: call_data_offset.as_u64(), - code_source: CodeSource::Account(bytecode.hash), - caller_id: CALLER_ID, - ..Default::default() - }], - steps, - ..Default::default() - }], - rws, - bytecodes: vec![bytecode], - ..Default::default() - }; - assert_eq!(run_test_circuit_incomplete_fixed_table(block), Ok(())); - } - - #[test] - fn calldatacopy_gadget_simple() { - test_ok_root(64, Word::from(0x40), Word::from(0), Word::from(10)); - test_ok_internal( - Word::from(0x40), - Word::from(64), - Word::from(0xA0), - Word::from(16), - Word::from(10), - ); - } - - #[test] - fn calldatacopy_gadget_multi_step() { - test_ok_root(128, Word::from(0x40), Word::from(16), Word::from(90)); - test_ok_internal( - Word::from(0x40), - Word::from(128), - Word::from(0x100), - Word::from(16), - Word::from(90), - ); - } - - #[test] - fn calldatacopy_gadget_out_of_bound() { - test_ok_root(64, Word::from(0x40), Word::from(32), Word::from(40)); - test_ok_internal( - Word::from(0x40), - Word::from(32), - Word::from(0xA0), - Word::from(40), - Word::from(10), - ); - } - - #[test] - fn calldatacopy_gadget_zero_length() { - test_ok_root(64, Word::from(0x40), Word::from(0), Word::from(0)); - test_ok_internal( - Word::from(0x40), - Word::from(64), - Word::from(0xA0), - Word::from(16), - Word::from(0), - ); - } - - #[test] - fn calldatacopy_gadget_busmapping_internal() { let addr_a = address!("0x000000000000000000000000000000000cafe00a"); let addr_b = address!("0x000000000000000000000000000000000cafe00b"); // code B gets called by code A, so the call is an internal call. - let dst_offset = 0x00usize; - let offset = 0x00usize; - let copy_size = 0x10usize; let code_b = bytecode! { PUSH32(copy_size) // size PUSH32(offset) // offset @@ -515,8 +293,6 @@ mod test { // code A calls code B. let pushdata = hex::decode("1234567890abcdef").unwrap(); - let call_data_length = 0x20usize; - let call_data_offset = 0x10usize; let code_a = bytecode! { // populate memory in A's context. PUSH8(Word::from_big_endian(&pushdata)) @@ -552,4 +328,28 @@ mod test { assert_eq!(run_test_circuits(ctx, None), Ok(())); } + + #[test] + fn calldatacopy_gadget_simple() { + test_ok_root(0x40, 0x40, 0x00, 0x0A); + test_ok_internal(0x40, 0x40, 0xA0, 0x10, 0x0A); + } + + #[test] + fn calldatacopy_gadget_multi_step() { + test_ok_root(0x80, 0x40, 0x10, 0x5A); + test_ok_internal(0x30, 0x70, 0x20, 0x10, 0x5A); + } + + #[test] + fn calldatacopy_gadget_out_of_bound() { + test_ok_root(0x40, 0x40, 0x20, 0x28); + test_ok_internal(0x40, 0x20, 0xA0, 0x28, 0x0A); + } + + #[test] + fn calldatacopy_gadget_zero_length() { + test_ok_root(0x40, 0x40, 0x00, 0x00); + test_ok_internal(0x40, 0x40, 0xA0, 0x10, 0x00); + } }