From 640b2753121046ecc937210b5c4c01b141d260c0 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Thu, 2 Mar 2023 21:35:53 +0800 Subject: [PATCH 01/12] Fix to handle successful run with Uint64 overflow in multiple opcodes. --- .../circuit_input_builder/input_state_ref.rs | 61 +++- bus-mapping/src/evm/opcodes/calldatacopy.rs | 70 ++--- bus-mapping/src/evm/opcodes/calldataload.rs | 132 ++++----- bus-mapping/src/evm/opcodes/codecopy.rs | 55 ++-- bus-mapping/src/evm/opcodes/extcodecopy.rs | 65 ++--- bus-mapping/src/evm/opcodes/returndatacopy.rs | 9 +- eth-types/src/evm_types/memory.rs | 18 +- .../src/evm_circuit/execution/blockhash.rs | 102 ++++--- .../src/evm_circuit/execution/calldatacopy.rs | 90 +++--- .../src/evm_circuit/execution/calldataload.rs | 260 ++++++++++-------- .../src/evm_circuit/execution/codecopy.rs | 77 +++--- .../execution/error_invalid_jump.rs | 81 ++---- .../src/evm_circuit/execution/extcodecopy.rs | 97 ++++--- .../src/evm_circuit/execution/jumpi.rs | 54 ++-- .../src/evm_circuit/util/common_gadget.rs | 124 ++++++++- 15 files changed, 743 insertions(+), 552 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder/input_state_ref.rs b/bus-mapping/src/circuit_input_builder/input_state_ref.rs index 9f73db38c7..04eeac81e5 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -20,7 +20,7 @@ use eth_types::{ evm_types::{ gas_utils::memory_expansion_gas_cost, Gas, GasCost, MemoryAddress, OpcodeId, StackAddress, }, - Address, GethExecStep, ToAddress, ToBigEndian, ToWord, Word, H256, U256, + Address, Bytecode, GethExecStep, H256, ToAddress, ToBigEndian, ToWord, U256, Word, }; use ethers_core::utils::{get_contract_address, get_create2_address}; use keccak256::EMPTY_HASH_LE; @@ -1395,4 +1395,63 @@ impl<'a> CircuitInputStateRef<'a> { Ok(()) } + + /// Generate copy steps for bytecode. + pub(crate) fn gen_copy_steps_for_bytecode( + &mut self, + exec_step: &mut ExecStep, + bytecode: &Bytecode, + src_addr: u64, + dst_addr: u64, + src_addr_end: u64, + bytes_left: u64, + ) -> Result, Error> { + let mut copy_steps = Vec::with_capacity(bytes_left as usize); + for idx in 0..bytes_left { + let addr = src_addr.checked_add(idx).unwrap_or(src_addr_end); + let step = if addr < src_addr_end { + let code = bytecode.code.get(addr as usize).unwrap(); + (code.value, code.is_code) + } else { + (0, false) + }; + copy_steps.push(step); + self.memory_write(exec_step, (dst_addr + idx).into(), step.0)?; + } + + Ok(copy_steps) + } + + /// Generate copy steps for call data. + pub(crate) fn gen_copy_steps_for_call_data( + &mut self, + exec_step: &mut ExecStep, + src_addr: u64, + dst_addr: u64, + src_addr_end: u64, + bytes_left: u64, + ) -> Result, Error> { + let mut copy_steps = Vec::with_capacity(bytes_left as usize); + for idx in 0..bytes_left { + let addr = src_addr.checked_add(idx).unwrap_or(src_addr_end); + let value = if addr < src_addr_end { + let byte = + self.call_ctx()?.call_data[(addr - self.call()?.call_data_offset) as usize]; + if !self.call()?.is_root { + self.push_op( + exec_step, + RW::READ, + MemoryOp::new(self.call()?.caller_id, addr.into(), byte), + ); + } + byte + } else { + 0 + }; + copy_steps.push((value, false)); + self.memory_write(exec_step, (dst_addr + idx).into(), value)?; + } + + Ok(copy_steps) + } } diff --git a/bus-mapping/src/evm/opcodes/calldatacopy.rs b/bus-mapping/src/evm/opcodes/calldatacopy.rs index 22e319afe7..6e923a7a90 100644 --- a/bus-mapping/src/evm/opcodes/calldatacopy.rs +++ b/bus-mapping/src/evm/opcodes/calldatacopy.rs @@ -3,7 +3,7 @@ use crate::{ circuit_input_builder::{ CircuitInputStateRef, CopyDataType, CopyEvent, ExecStep, NumberOrHash, }, - operation::{CallContextField, MemoryOp, RW}, + operation::CallContextField, Error, }; use eth_types::GethExecStep; @@ -20,13 +20,13 @@ impl Opcode for Calldatacopy { let mut exec_steps = vec![gen_calldatacopy_step(state, geth_step)?]; // reconstruction - let memory_offset = geth_step.stack.nth_last(0)?.as_u64(); - let data_offset = geth_step.stack.nth_last(1)?.as_u64(); - let length = geth_step.stack.nth_last(2)?.as_usize(); + let memory_offset = geth_step.stack.nth_last(0)?; + let data_offset = geth_step.stack.nth_last(1)?; + let length = geth_step.stack.nth_last(2)?; let call_ctx = state.call_ctx_mut()?; let memory = &mut call_ctx.memory; - memory.copy_from(memory_offset, &call_ctx.call_data, data_offset, length); + memory.copy_from(memory_offset, data_offset, length, &call_ctx.call_data); let copy_event = gen_copy_event(state, geth_step)?; state.push_copy(&mut exec_steps[0], copy_event); @@ -92,64 +92,36 @@ fn gen_calldatacopy_step( Ok(exec_step) } -fn gen_copy_steps( - state: &mut CircuitInputStateRef, - exec_step: &mut ExecStep, - src_addr: u64, - dst_addr: u64, - src_addr_end: u64, - bytes_left: u64, - is_root: bool, -) -> Result, Error> { - let mut copy_steps = Vec::with_capacity(bytes_left as usize); - for idx in 0..bytes_left { - let addr = src_addr + idx; - let value = if addr < src_addr_end { - let byte = - state.call_ctx()?.call_data[(addr - state.call()?.call_data_offset) as usize]; - if !is_root { - state.push_op( - exec_step, - RW::READ, - MemoryOp::new(state.call()?.caller_id, addr.into(), byte), - ); - } - byte - } else { - 0 - }; - copy_steps.push((value, false)); - state.memory_write(exec_step, (dst_addr + idx).into(), value)?; - } - - Ok(copy_steps) -} - fn gen_copy_event( state: &mut CircuitInputStateRef, geth_step: &GethExecStep, ) -> Result { let rw_counter_start = state.block_ctx.rwc; - let memory_offset = geth_step.stack.nth_last(0)?.as_u64(); - let data_offset = geth_step.stack.nth_last(1)?.as_u64(); + + let memory_offset = geth_step.stack.nth_last(0)?; + let data_offset = geth_step.stack.nth_last(1)?; let length = geth_step.stack.nth_last(2)?.as_u64(); let call_data_offset = state.call()?.call_data_offset; let call_data_length = state.call()?.call_data_length; - let (src_addr, src_addr_end) = ( - call_data_offset + data_offset, - call_data_offset + call_data_length, - ); + + let dst_addr = memory_offset.as_u64(); + let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); + + // Reset start offset to end offset if overflow. + let src_addr = u64::try_from(data_offset) + .ok() + .and_then(|offset| offset.checked_add(call_data_offset)) + .unwrap_or(src_addr_end) + .min(src_addr_end); let mut exec_step = state.new_step(geth_step)?; - let copy_steps = gen_copy_steps( - state, + let copy_steps = state.gen_copy_steps_for_call_data( &mut exec_step, src_addr, - memory_offset, + dst_addr, src_addr_end, length, - state.call()?.is_root, )?; let (src_type, src_id) = if state.call()?.is_root { @@ -165,7 +137,7 @@ fn gen_copy_event( src_addr_end, dst_type: CopyDataType::Memory, dst_id: NumberOrHash::Number(state.call()?.call_id), - dst_addr: memory_offset, + dst_addr, log_id: None, rw_counter_start, bytes: copy_steps, diff --git a/bus-mapping/src/evm/opcodes/calldataload.rs b/bus-mapping/src/evm/opcodes/calldataload.rs index dacee4e721..97efa3200f 100644 --- a/bus-mapping/src/evm/opcodes/calldataload.rs +++ b/bus-mapping/src/evm/opcodes/calldataload.rs @@ -23,73 +23,79 @@ impl Opcode for Calldataload { let offset = geth_step.stack.nth_last(0)?; state.stack_read(&mut exec_step, geth_step.stack.nth_last_filled(0), offset)?; - let is_root = state.call()?.is_root; - if is_root { - state.call_context_read( - &mut exec_step, - state.call()?.call_id, - CallContextField::TxId, - state.tx_ctx.id().into(), + // Check if offset is Uint64 overflow. + let calldata_word = if let Ok(offset) = u64::try_from(offset) { + let is_root = state.call()?.is_root; + let call_id = state.call()?.call_id; + if is_root { + state.call_context_read( + &mut exec_step, + call_id, + CallContextField::TxId, + state.tx_ctx.id().into(), + ); + state.call_context_read( + &mut exec_step, + call_id, + CallContextField::CallDataLength, + state.call()?.call_data_length.into(), + ); + } else { + state.call_context_read( + &mut exec_step, + call_id, + CallContextField::CallerId, + state.call()?.caller_id.into(), + ); + state.call_context_read( + &mut exec_step, + call_id, + CallContextField::CallDataLength, + state.call()?.call_data_length.into(), + ); + state.call_context_read( + &mut exec_step, + call_id, + CallContextField::CallDataOffset, + state.call()?.call_data_offset.into(), + ); + } + + let call_data_offset = state.call()?.call_data_offset; + let call_data_length = state.call()?.call_data_length; + let (src_addr, src_addr_end, caller_id, call_data) = ( + call_data_offset + offset.min(call_data_length), + call_data_offset + call_data_length, + state.call()?.caller_id, + state.call_ctx()?.call_data.to_vec(), ); - state.call_context_read( - &mut exec_step, - state.call()?.call_id, - CallContextField::CallDataLength, - state.call()?.call_data_length.into(), - ); - } else { - state.call_context_read( - &mut exec_step, - state.call()?.call_id, - CallContextField::CallerId, - state.call()?.caller_id.into(), - ); - state.call_context_read( - &mut exec_step, - state.call()?.call_id, - CallContextField::CallDataLength, - state.call()?.call_data_length.into(), - ); - state.call_context_read( - &mut exec_step, - state.call()?.call_id, - CallContextField::CallDataOffset, - state.call()?.call_data_offset.into(), - ); - } - let call = state.call()?.clone(); - let (src_addr, src_addr_end, caller_id, call_data) = ( - call.call_data_offset as usize + offset.as_usize(), - call.call_data_offset as usize + call.call_data_length as usize, - call.caller_id, - state.call_ctx()?.call_data.to_vec(), - ); - let calldata_word = (0..32) - .map(|idx| { - let addr = src_addr + idx; - if addr < src_addr_end { - let byte = call_data[addr - call.call_data_offset as usize]; - if !is_root { - // caller id as call_id - state.push_op( - &mut exec_step, - RW::READ, - MemoryOp::new(caller_id, (src_addr + idx).into(), byte), - ); + let calldata: Vec<_> = (0..32) + .map(|idx| { + let addr = src_addr.checked_add(idx).unwrap_or(src_addr_end); + if addr < src_addr_end { + let byte = call_data[(addr - call_data_offset) as usize]; + if !is_root { + state.push_op( + &mut exec_step, + RW::READ, + MemoryOp::new(caller_id, (src_addr + idx).into(), byte), + ); + } + byte + } else { + 0 } - byte - } else { - 0 - } - }) - .collect::>(); + }) + .collect(); + + U256::from_big_endian(&calldata) + } else { + // Stack push `0` as result if overflow. + U256::zero() + }; - state.stack_write( - &mut exec_step, - geth_step.stack.last_filled(), - U256::from_big_endian(&calldata_word), - )?; + state.stack_write(&mut exec_step, geth_step.stack.last_filled(), calldata_word)?; Ok(vec![exec_step]) } diff --git a/bus-mapping/src/evm/opcodes/codecopy.rs b/bus-mapping/src/evm/opcodes/codecopy.rs index 3bd1134458..146b06258c 100644 --- a/bus-mapping/src/evm/opcodes/codecopy.rs +++ b/bus-mapping/src/evm/opcodes/codecopy.rs @@ -20,10 +20,9 @@ impl Opcode for Codecopy { let mut exec_steps = vec![gen_codecopy_step(state, geth_step)?]; // reconstruction - - let dest_offset = geth_step.stack.nth_last(0)?.as_u64(); - let code_offset = geth_step.stack.nth_last(1)?.as_u64(); - let length = geth_step.stack.nth_last(2)?.as_u64(); + let dst_offset = geth_step.stack.nth_last(0)?; + let code_offset = geth_step.stack.nth_last(1)?; + let length = geth_step.stack.nth_last(2)?; let code_hash = state.call()?.code_hash; let code = state.code(code_hash)?; @@ -31,7 +30,7 @@ impl Opcode for Codecopy { let call_ctx = state.call_ctx_mut()?; let memory = &mut call_ctx.memory; - memory.copy_from(dest_offset, &code, code_offset, length as usize); + memory.copy_from(dst_offset, code_offset, length, &code); let copy_event = gen_copy_event(state, geth_step)?; state.push_copy(&mut exec_steps[0], copy_event); @@ -65,56 +64,46 @@ fn gen_codecopy_step( Ok(exec_step) } -fn gen_copy_steps( - state: &mut CircuitInputStateRef, - exec_step: &mut ExecStep, - src_addr: u64, - dst_addr: u64, - bytes_left: u64, - bytecode: &Bytecode, -) -> Result, Error> { - let mut steps = Vec::with_capacity(bytes_left as usize); - for idx in 0..bytes_left { - let addr = src_addr + idx; - let bytecode_element = bytecode.get(addr as usize).unwrap_or_default(); - steps.push((bytecode_element.value, bytecode_element.is_code)); - state.memory_write(exec_step, (dst_addr + idx).into(), bytecode_element.value)?; - } - Ok(steps) -} - fn gen_copy_event( state: &mut CircuitInputStateRef, geth_step: &GethExecStep, ) -> Result { let rw_counter_start = state.block_ctx.rwc; - let dst_offset = geth_step.stack.nth_last(0)?.as_u64(); - let code_offset = geth_step.stack.nth_last(1)?.as_u64(); + let dst_offset = geth_step.stack.nth_last(0)?; + let code_offset = geth_step.stack.nth_last(1)?; let length = geth_step.stack.nth_last(2)?.as_u64(); let code_hash = state.call()?.code_hash; let bytecode: Bytecode = state.code(code_hash)?.into(); - let src_addr_end = bytecode.to_vec().len() as u64; + let code_size = bytecode.code.len() as u64; + + let dst_addr = dst_offset.as_u64(); + let src_addr_end = code_size; + + // Reset start offset to end offset if overflow. + let src_addr = u64::try_from(code_offset) + .unwrap_or(src_addr_end) + .min(src_addr_end); let mut exec_step = state.new_step(geth_step)?; - let copy_steps = gen_copy_steps( - state, + let copy_steps = state.gen_copy_steps_for_bytecode( &mut exec_step, - code_offset, - dst_offset, - length, &bytecode, + src_addr, + dst_addr, + src_addr_end, + length, )?; Ok(CopyEvent { src_type: CopyDataType::Bytecode, src_id: NumberOrHash::Hash(code_hash), - src_addr: code_offset, + src_addr, src_addr_end, dst_type: CopyDataType::Memory, dst_id: NumberOrHash::Number(state.call()?.call_id), - dst_addr: dst_offset, + dst_addr, log_id: None, rw_counter_start, bytes: copy_steps, diff --git a/bus-mapping/src/evm/opcodes/extcodecopy.rs b/bus-mapping/src/evm/opcodes/extcodecopy.rs index dd94afac28..9ccfbed146 100644 --- a/bus-mapping/src/evm/opcodes/extcodecopy.rs +++ b/bus-mapping/src/evm/opcodes/extcodecopy.rs @@ -24,9 +24,9 @@ impl Opcode for Extcodecopy { // reconstruction let address = geth_steps[0].stack.nth_last(0)?.to_address(); - let dest_offset = geth_steps[0].stack.nth_last(1)?.as_u64(); - let code_offset = geth_steps[0].stack.nth_last(2)?.as_u64(); - let length = geth_steps[0].stack.nth_last(3)?.as_u64(); + let dst_offset = geth_steps[0].stack.nth_last(1)?; + let code_offset = geth_step.stack.nth_last(2)?; + let length = geth_steps[0].stack.nth_last(3)?; let (_, account) = state.sdb.get_account(&address); let code_hash = account.code_hash; @@ -35,7 +35,7 @@ impl Opcode for Extcodecopy { let call_ctx = state.call_ctx_mut()?; let memory = &mut call_ctx.memory; - memory.copy_from(dest_offset, &code, code_offset, length as usize); + memory.copy_from(dst_offset, code_offset, length, &code); let copy_event = gen_copy_event(state, geth_step)?; state.push_copy(&mut exec_steps[0], copy_event); @@ -111,39 +111,15 @@ fn gen_extcodecopy_step( Ok(exec_step) } -fn gen_copy_steps( - state: &mut CircuitInputStateRef, - exec_step: &mut ExecStep, - src_addr: u64, - dst_addr: u64, - src_addr_end: u64, - bytes_left: u64, - code: &Bytecode, -) -> Result, Error> { - let mut copy_steps = Vec::with_capacity(bytes_left as usize); - for idx in 0..bytes_left { - let addr = src_addr + idx; - let step = if addr < src_addr_end { - let code = code.code.get(addr as usize).unwrap(); - (code.value, code.is_code) - } else { - (0, false) - }; - copy_steps.push(step); - state.memory_write(exec_step, (dst_addr + idx).into(), step.0)?; - } - - Ok(copy_steps) -} - fn gen_copy_event( state: &mut CircuitInputStateRef, geth_step: &GethExecStep, ) -> Result { let rw_counter_start = state.block_ctx.rwc; + let external_address = geth_step.stack.nth_last(0)?.to_address(); - let memory_offset = geth_step.stack.nth_last(1)?.as_u64(); - let data_offset = geth_step.stack.nth_last(2)?.as_u64(); + let dst_offset = geth_step.stack.nth_last(1)?; + let code_offset = geth_step.stack.nth_last(2)?; let length = geth_step.stack.nth_last(3)?.as_u64(); let account = state.sdb.get_account(&external_address).1; @@ -154,28 +130,37 @@ fn gen_copy_event( H256::zero() }; - let code: Bytecode = if exists { + let bytecode: Bytecode = if exists { state.code(code_hash)?.into() } else { Bytecode::default() }; - let src_addr_end = code.code.len() as u64; + let code_size = bytecode.code.len() as u64; + + let dst_addr = dst_offset.as_u64(); + let src_addr_end = code_size; + + // Reset start offset to end offset if overflow. + let src_addr = u64::try_from(code_offset) + .unwrap_or(src_addr_end) + .min(src_addr_end); + let mut exec_step = state.new_step(geth_step)?; - let copy_steps = gen_copy_steps( - state, + let copy_steps = state.gen_copy_steps_for_bytecode( &mut exec_step, - data_offset, - memory_offset, + &bytecode, + src_addr, + dst_addr, src_addr_end, length, - &code, )?; + Ok(CopyEvent { - src_addr: data_offset, + src_addr, src_addr_end, src_type: CopyDataType::Bytecode, src_id: NumberOrHash::Hash(code_hash), - dst_addr: memory_offset, + dst_addr, dst_type: CopyDataType::Memory, dst_id: NumberOrHash::Number(state.call()?.call_id), log_id: None, diff --git a/bus-mapping/src/evm/opcodes/returndatacopy.rs b/bus-mapping/src/evm/opcodes/returndatacopy.rs index 7c63965bef..e94db0113b 100644 --- a/bus-mapping/src/evm/opcodes/returndatacopy.rs +++ b/bus-mapping/src/evm/opcodes/returndatacopy.rs @@ -21,17 +21,16 @@ impl Opcode for Returndatacopy { // reconstruction let geth_step = &geth_steps[0]; - let dest_offset = geth_step.stack.nth_last(0)?; - let offset = geth_step.stack.nth_last(1)?; - let size = geth_step.stack.nth_last(2)?; + let dst_offset = geth_step.stack.nth_last(0)?; + let src_offset = geth_step.stack.nth_last(1)?; + let length = geth_step.stack.nth_last(2)?; // can we reduce this clone? let return_data = state.call_ctx()?.return_data.clone(); let call_ctx = state.call_ctx_mut()?; let memory = &mut call_ctx.memory; - let length = size.as_usize(); - memory.copy_from(dest_offset.as_u64(), &return_data, offset.as_u64(), length); + memory.copy_from(dst_offset, src_offset, length, &return_data); let copy_event = gen_copy_event(state, geth_step)?; state.push_copy(&mut exec_steps[0], copy_event); diff --git a/eth-types/src/evm_types/memory.rs b/eth-types/src/evm_types/memory.rs index 7b0c599a8e..8208bd377e 100644 --- a/eth-types/src/evm_types/memory.rs +++ b/eth-types/src/evm_types/memory.rs @@ -329,9 +329,21 @@ impl Memory { } /// Copy source data to memory. Used in (ext)codecopy/calldatacopy. - pub fn copy_from(&mut self, dst_offset: u64, data: &[u8], data_offset: u64, length: usize) { - // https://github.com/ethereum/go-ethereum/blob/df52967ff6080a27243569020ff64cd956fb8362/core/vm/instructions.go#L312 + pub fn copy_from(&mut self, dst_offset: Word, src_offset: Word, length: Word, data: &[u8]) { + // Reference go-ethereum `opCallDataCopy` function for defails. + // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/instructions.go#L299 + + // `length` should be checked for overflow during gas cost calculation. + // Otherwise should return an out of gas error previously. + let length = length.as_usize(); if length != 0 { + // `dst_offset` should be within range if length is non-zero. + // https://github.com/ethereum/go-ethereum/blob/bb4ac2d396de254898a5f44b1ea2086bfe5bd193/core/vm/common.go#L37 + let dst_offset = dst_offset.as_u64(); + + // Reset data offset to the maximum value of Uint64 if overflow. + let src_offset = u64::try_from(src_offset).unwrap_or(u64::MAX); + let minimal_length = dst_offset as usize + length; self.extend_at_least(minimal_length); @@ -339,7 +351,7 @@ impl Memory { let mem_ends = mem_starts + length as usize; let dst_slice = &mut self.0[mem_starts..mem_ends]; dst_slice.fill(0); - let data_starts = data_offset as usize; + let data_starts = src_offset as usize; let actual_length = std::cmp::min( length, data.len().checked_sub(data_starts).unwrap_or_default(), diff --git a/zkevm-circuits/src/evm_circuit/execution/blockhash.rs b/zkevm-circuits/src/evm_circuit/execution/blockhash.rs index 4ddcf450a6..12eff1dcd2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/blockhash.rs +++ b/zkevm-circuits/src/evm_circuit/execution/blockhash.rs @@ -4,11 +4,11 @@ use crate::{ param::N_BYTES_U64, step::ExecutionState, util::{ - common_gadget::SameContextGadget, + and, + common_gadget::{SameContextGadget, WordByteCapGadget}, constraint_builder::{ConstraintBuilder, StepStateTransition, Transition::Delta}, - from_bytes, math_gadget::LtGadget, - CachedRegion, Cell, RandomLinearCombination, Word, + CachedRegion, Cell, Word, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -23,10 +23,9 @@ use halo2_proofs::{circuit::Value, plonk::Error}; #[derive(Clone, Debug)] pub(crate) struct BlockHashGadget { same_context: SameContextGadget, - block_number: RandomLinearCombination, + block_number: WordByteCapGadget, current_block_number: Cell, block_hash: Word, - block_lt: LtGadget, diff_lt: LtGadget, } @@ -36,9 +35,6 @@ impl ExecutionGadget for BlockHashGadget { const EXECUTION_STATE: ExecutionState = ExecutionState::BLOCKHASH; fn configure(cb: &mut ConstraintBuilder) -> Self { - let block_number = cb.query_word_rlc(); - cb.stack_pop(block_number.expr()); - let current_block_number = cb.query_cell(); cb.block_lookup( BlockContextFieldTag::Number.expr(), @@ -46,28 +42,32 @@ impl ExecutionGadget for BlockHashGadget { current_block_number.expr(), ); - let block_lt = LtGadget::construct( - cb, - from_bytes::expr(&block_number.cells), - current_block_number.expr(), - ); + let block_number = WordByteCapGadget::construct(cb, current_block_number.expr()); + cb.stack_pop(block_number.original_word()); + + let block_hash = cb.query_word_rlc(); let diff_lt = LtGadget::construct( cb, current_block_number.expr(), - 257.expr() + from_bytes::expr(&block_number.cells), + 257.expr() + block_number.valid_value(), ); - let block_hash = cb.query_word_rlc(); - cb.condition(block_lt.expr() * diff_lt.expr(), |cb| { + let is_valid = and::expr([block_number.lt_cap(), diff_lt.expr()]); + + cb.condition(is_valid.expr(), |cb| { cb.block_lookup( BlockContextFieldTag::BlockHash.expr(), - Some(from_bytes::expr(&block_number.cells)), + Some(block_number.valid_value()), block_hash.expr(), ); }); - cb.condition(not::expr(block_lt.expr() * diff_lt.expr()), |cb| { - cb.require_zero("invalid range", block_hash.expr()); + + cb.condition(not::expr(is_valid), |cb| { + cb.require_zero( + "Invalid block number for block hash lookup", + block_hash.expr(), + ); }); cb.stack_push(block_hash.expr()); @@ -86,7 +86,6 @@ impl ExecutionGadget for BlockHashGadget { block_number, current_block_number, block_hash, - block_lt, diff_lt, } } @@ -102,29 +101,18 @@ impl ExecutionGadget for BlockHashGadget { ) -> Result<(), Error> { self.same_context.assign_exec_step(region, offset, step)?; + let current_block_number = block.context.number; + let current_block_number = current_block_number + .to_scalar() + .expect("unexpected U256 -> Scalar conversion failure"); + let block_number = block.rws[step.rw_indices[0]].stack_value(); - self.block_number.assign( - region, - offset, - Some( - block_number.to_le_bytes()[..N_BYTES_U64] - .try_into() - .unwrap(), - ), - )?; + self.block_number + .assign(region, offset, block_number, current_block_number)?; let block_number: F = block_number.to_scalar().unwrap(); - let current_block_number = block.context.number; - self.current_block_number.assign( - region, - offset, - Value::known( - current_block_number - .to_scalar() - .expect("unexpected U256 -> Scalar conversion failure"), - ), - )?; - let current_block_number: F = current_block_number.to_scalar().unwrap(); + self.current_block_number + .assign(region, offset, Value::known(current_block_number))?; self.block_hash.assign( region, @@ -132,9 +120,6 @@ impl ExecutionGadget for BlockHashGadget { Some(block.rws[step.rw_indices[1]].stack_value().to_le_bytes()), )?; - self.block_lt - .assign(region, offset, block_number, current_block_number)?; - self.diff_lt.assign( region, offset, @@ -152,7 +137,7 @@ mod test { use eth_types::{bytecode, U256}; use mock::test_ctx::{helpers::*, TestContext}; - fn test_ok(block_number: usize, current_block_number: u64) { + fn test_ok(block_number: U256, current_block_number: u64) { let code = bytecode! { PUSH32(block_number) BLOCKHASH @@ -182,21 +167,26 @@ mod test { #[test] fn blockhash_gadget_simple() { - test_ok(0, 5); - test_ok(1, 5); - test_ok(2, 5); - test_ok(3, 5); - test_ok(4, 5); - test_ok(5, 5); - test_ok(6, 5); + test_ok(0.into(), 5); + test_ok(1.into(), 5); + test_ok(2.into(), 5); + test_ok(3.into(), 5); + test_ok(4.into(), 5); + test_ok(5.into(), 5); + test_ok(6.into(), 5); } #[test] fn blockhash_gadget_large() { - test_ok(0xcafe - 257, 0xcafeu64); - test_ok(0xcafe - 256, 0xcafeu64); - test_ok(0xcafe - 1, 0xcafeu64); - test_ok(0xcafe, 0xcafeu64); - test_ok(0xcafe + 1, 0xcafeu64); + test_ok((0xcafe - 257).into(), 0xcafeu64); + test_ok((0xcafe - 256).into(), 0xcafeu64); + test_ok((0xcafe - 1).into(), 0xcafeu64); + test_ok(0xcafe.into(), 0xcafeu64); + test_ok((0xcafe + 1).into(), 0xcafeu64); + } + + #[test] + fn blockhash_gadget_block_number_overflow() { + test_ok(U256::MAX, 0xcafeu64); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 3a2892a686..fc47e44977 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -1,17 +1,16 @@ use crate::{ evm_circuit::{ execution::ExecutionGadget, - param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, + param::{N_BYTES_MEMORY_WORD_SIZE, N_BYTES_U64}, step::ExecutionState, util::{ - common_gadget::SameContextGadget, + common_gadget::{SameContextGadget, WordByteCapGadget}, constraint_builder::{ ConstraintBuilder, StepStateTransition, Transition::{Delta, To}, }, - from_bytes, memory_gadget::{MemoryAddressGadget, MemoryCopierGasGadget, MemoryExpansionGadget}, - not, select, CachedRegion, Cell, MemoryAddress, + not, select, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -19,7 +18,7 @@ use crate::{ util::Expr, }; use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; -use eth_types::{evm_types::GasCost, Field, ToLittleEndian, ToScalar}; +use eth_types::{evm_types::GasCost, Field, ToScalar}; use halo2_proofs::{circuit::Value, plonk::Error}; use std::cmp::min; @@ -28,7 +27,7 @@ use std::cmp::min; pub(crate) struct CallDataCopyGadget { same_context: SameContextGadget, memory_address: MemoryAddressGadget, - data_offset: MemoryAddress, + data_offset: WordByteCapGadget, src_id: Cell, call_data_length: Cell, call_data_offset: Cell, // Only used in the internal call @@ -45,20 +44,19 @@ impl ExecutionGadget for CallDataCopyGadget { fn configure(cb: &mut ConstraintBuilder) -> Self { let opcode = cb.query_cell(); - let memory_offset = cb.query_cell_phase2(); - let data_offset = cb.query_word_rlc(); + let src_id = cb.query_cell(); + let call_data_length = cb.query_cell(); + let call_data_offset = cb.query_cell(); + let length = cb.query_word_rlc(); + let memory_offset = cb.query_cell_phase2(); + let data_offset = WordByteCapGadget::construct(cb, call_data_length.expr()); // Pop memory_offset, data_offset, length from stack cb.stack_pop(memory_offset.expr()); - cb.stack_pop(data_offset.expr()); + cb.stack_pop(data_offset.original_word()); cb.stack_pop(length.expr()); - let memory_address = MemoryAddressGadget::construct(cb, memory_offset, length); - 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| { @@ -97,6 +95,7 @@ impl ExecutionGadget for CallDataCopyGadget { // Calculate the next memory size and the gas cost for this memory // access + let memory_address = MemoryAddressGadget::construct(cb, memory_offset, length); let memory_expansion = MemoryExpansionGadget::construct(cb, [memory_address.address()]); let memory_copier_gas = MemoryCopierGasGadget::construct( cb, @@ -111,13 +110,23 @@ impl ExecutionGadget for CallDataCopyGadget { CopyDataType::Memory.expr(), ); cb.condition(memory_address.has_length(), |cb| { + // Set source start to the minimun value of data offset and call data length. + let src_addr = call_data_offset.expr() + + select::expr( + data_offset.lt_cap(), + data_offset.valid_value(), + call_data_length.expr(), + ); + + let src_addr_end = call_data_offset.expr() + call_data_length.expr(); + cb.copy_table_lookup( src_id.expr(), src_tag, cb.curr.state.call_id.expr(), CopyDataType::Memory.expr(), - call_data_offset.expr() + from_bytes::expr(&data_offset.cells), - call_data_offset.expr() + call_data_length.expr(), + src_addr, + src_addr_end, memory_address.offset(), memory_address.length(), 0.expr(), // for CALLDATACOPY rlc_acc is 0 @@ -175,15 +184,6 @@ impl ExecutionGadget for CallDataCopyGadget { let memory_address = self .memory_address .assign(region, offset, memory_offset, length)?; - self.data_offset.assign( - region, - offset, - Some( - data_offset.to_le_bytes()[..N_BYTES_MEMORY_ADDRESS] - .try_into() - .unwrap(), - ), - )?; let src_id = if call.is_root { tx.id } else { call.caller_id }; self.src_id.assign( region, @@ -202,6 +202,9 @@ impl ExecutionGadget for CallDataCopyGadget { self.call_data_offset .assign(region, offset, Value::known(F::from(call_data_offset)))?; + self.data_offset + .assign(region, offset, data_offset, F::from(call_data_length))?; + // rw_counter increase from copy lookup is `length` memory writes + a variable // number of memory reads. let copy_rwc_inc = length @@ -212,9 +215,10 @@ impl ExecutionGadget for CallDataCopyGadget { // memory reads when reading from memory of caller is capped by call_data_length // - data_offset. min( - length.low_u64(), - call_data_length - .checked_sub(data_offset.low_u64()) + length.as_u64(), + u64::try_from(data_offset) + .ok() + .and_then(|offset| call_data_length.checked_sub(offset)) .unwrap_or_default(), ) }; @@ -257,8 +261,8 @@ mod test { fn test_ok_root( call_data_length: usize, memory_offset: usize, - data_offset: usize, length: usize, + data_offset: Word, ) { let bytecode = bytecode! { PUSH32(length) @@ -296,15 +300,15 @@ mod test { call_data_offset: usize, call_data_length: usize, dst_offset: usize, - offset: usize, length: usize, + data_offset: Word, ) { let (addr_a, addr_b) = (mock::MOCK_ACCOUNTS[0], mock::MOCK_ACCOUNTS[1]); // code B gets called by code A, so the call is an internal call. let code_b = bytecode! { - PUSH32(length) // size - PUSH32(offset) // offset + PUSH32(length) // size + PUSH32(data_offset) // data_offset PUSH32(dst_offset) // dst_offset CALLDATACOPY STOP @@ -350,25 +354,31 @@ mod test { #[test] fn calldatacopy_gadget_simple() { - test_ok_root(0x40, 0x40, 0x00, 10); - test_ok_internal(0x40, 0x40, 0xA0, 0x10, 10); + test_ok_root(0x40, 0x40, 10, 0x00.into()); + test_ok_internal(0x40, 0x40, 0xA0, 10, 0x10.into()); } #[test] fn calldatacopy_gadget_large() { - test_ok_root(0x204, 0x103, 0x102, 0x101); - test_ok_internal(0x30, 0x204, 0x103, 0x102, 0x101); + test_ok_root(0x204, 0x103, 0x101, 0x102.into()); + test_ok_internal(0x30, 0x204, 0x103, 0x101, 0x102.into()); } #[test] fn calldatacopy_gadget_out_of_bound() { - test_ok_root(0x40, 0x40, 0x20, 40); - test_ok_internal(0x40, 0x20, 0xA0, 0x28, 10); + test_ok_root(0x40, 0x40, 40, 0x20.into()); + test_ok_internal(0x40, 0x20, 0xA0, 10, 0x28.into()); } #[test] fn calldatacopy_gadget_zero_length() { - test_ok_root(0x40, 0x40, 0x00, 0); - test_ok_internal(0x40, 0x40, 0xA0, 0x10, 0); + test_ok_root(0x40, 0x40, 0, 0x00.into()); + test_ok_internal(0x40, 0x40, 0xA0, 0, 0x10.into()); + } + + #[test] + fn calldatacopy_gadget_data_offset_overflow() { + test_ok_root(0x40, 0x40, 0, Word::MAX); + test_ok_internal(0x40, 0x40, 0xA0, 0, Word::MAX); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs index a18f69cc6b..769fd78262 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs @@ -1,5 +1,5 @@ use bus_mapping::evm::OpcodeId; -use eth_types::{Field, ToLittleEndian}; +use eth_types::Field; use halo2_proofs::{ circuit::Value, plonk::{Error, Expression}, @@ -7,14 +7,14 @@ use halo2_proofs::{ use crate::{ evm_circuit::{ - param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_WORD}, + param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_U64, N_BYTES_WORD}, step::ExecutionState, util::{ - common_gadget::SameContextGadget, + and, + common_gadget::{SameContextGadget, WordByteCapGadget}, constraint_builder::{ConstraintBuilder, StepStateTransition, Transition::Delta}, - from_bytes, memory_gadget::BufferReaderGadget, - not, CachedRegion, Cell, MemoryAddress, + not, select, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -34,8 +34,6 @@ pub(crate) struct CallDataLoadGadget { /// Source of data, this is transaction ID for a root call and caller ID for /// an internal call. src_id: Cell, - /// The bytes offset in calldata, from which we load a 32-bytes word. - offset: MemoryAddress, /// The size of the call's data (tx input for a root call or calldata length /// of an internal call). call_data_length: Cell, @@ -43,6 +41,9 @@ pub(crate) struct CallDataLoadGadget { /// tx data starts at the first byte, but can be non-zero offset for an /// internal call. call_data_offset: Cell, + /// The bytes offset in calldata, from which we load a 32-bytes word. It + /// is valid if within range of Uint64 and less than call_data_length. + data_offset: WordByteCapGadget, /// Gadget to read from tx calldata, which we validate against the word /// pushed to stack. buffer_reader: BufferReaderGadget, @@ -56,61 +57,84 @@ impl ExecutionGadget for CallDataLoadGadget { fn configure(cb: &mut ConstraintBuilder) -> Self { let opcode = cb.query_cell(); - let offset = cb.query_word_rlc(); - - // Pop the offset value from stack. - cb.stack_pop(offset.expr()); - - // Add a lookup constrain for TxId in the RW table. let src_id = cb.query_cell(); let call_data_length = cb.query_cell(); let call_data_offset = cb.query_cell(); - let src_addr = from_bytes::expr(&offset.cells) + call_data_offset.expr(); - let src_addr_end = call_data_length.expr() + call_data_offset.expr(); + let data_offset = WordByteCapGadget::construct(cb, call_data_length.expr()); + cb.stack_pop(data_offset.original_word()); + + cb.condition( + and::expr([data_offset.within_range(), cb.curr.state.is_root.expr()]), + |cb| { + cb.call_context_lookup( + false.expr(), + None, + CallContextFieldTag::TxId, + src_id.expr(), + ); + cb.call_context_lookup( + false.expr(), + None, + CallContextFieldTag::CallDataLength, + call_data_length.expr(), + ); + cb.require_equal( + "if is_root then call_data_offset == 0", + call_data_offset.expr(), + 0.expr(), + ); + }, + ); - cb.condition(cb.curr.state.is_root.expr(), |cb| { - cb.call_context_lookup(false.expr(), None, CallContextFieldTag::TxId, src_id.expr()); - cb.call_context_lookup( - false.expr(), - None, - CallContextFieldTag::CallDataLength, - call_data_length.expr(), - ); - cb.require_equal( - "if is_root then call_data_offset == 0", - call_data_offset.expr(), - 0.expr(), - ); - }); - cb.condition(not::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, - CallContextFieldTag::CallDataLength, + cb.condition( + and::expr([ + data_offset.within_range(), + not::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, + CallContextFieldTag::CallDataLength, + call_data_length.expr(), + ); + cb.call_context_lookup( + false.expr(), + None, + CallContextFieldTag::CallDataOffset, + call_data_offset.expr(), + ); + }, + ); + + // Set source start to the minimun value of data offset and call data length. + let src_addr = call_data_offset.expr() + + select::expr( + data_offset.lt_cap(), + data_offset.valid_value(), call_data_length.expr(), ); - cb.call_context_lookup( - false.expr(), - None, - CallContextFieldTag::CallDataOffset, - call_data_offset.expr(), - ); - }); - let buffer_reader = BufferReaderGadget::construct(cb, src_addr.clone(), src_addr_end); + let src_addr_end = call_data_offset.expr() + call_data_length.expr(); - let mut calldata_word = (0..N_BYTES_WORD) + let buffer_reader = BufferReaderGadget::construct(cb, src_addr.expr(), src_addr_end); + + let mut calldata_word: Vec<_> = (0..N_BYTES_WORD) .map(|idx| { - // for a root call, the call data comes from tx's data field. + // For a root call, the call data comes from tx's data field. cb.condition( - cb.curr.state.is_root.expr() * buffer_reader.read_flag(idx), + and::expr([ + data_offset.within_range(), + buffer_reader.read_flag(idx), + cb.curr.state.is_root.expr(), + ]), |cb| { cb.tx_context_lookup( src_id.expr(), @@ -120,9 +144,13 @@ impl ExecutionGadget for CallDataLoadGadget { ); }, ); - // for an internal call, the call data comes from memory. + // For an internal call, the call data comes from memory. cb.condition( - (1.expr() - cb.curr.state.is_root.expr()) * buffer_reader.read_flag(idx), + and::expr([ + data_offset.within_range(), + buffer_reader.read_flag(idx), + not::expr(cb.curr.state.is_root.expr()), + ]), |cb| { cb.memory_lookup( 0.expr(), @@ -134,7 +162,7 @@ impl ExecutionGadget for CallDataLoadGadget { ); buffer_reader.byte(idx) }) - .collect::>>(); + .collect(); // Since the stack items are in little endian form, we reverse the bytes // here. @@ -143,7 +171,13 @@ impl ExecutionGadget for CallDataLoadGadget { // Add a lookup constraint for the 32-bytes that should have been pushed // to the stack. let calldata_word: [Expression; N_BYTES_WORD] = calldata_word.try_into().unwrap(); - cb.stack_push(cb.word_rlc(calldata_word)); + let calldata_word = cb.word_rlc(calldata_word); + cb.require_zero( + "Stack push result must be 0 if stack pop offset is Uint64 overflow", + data_offset.overflow() * calldata_word.expr(), + ); + + cb.stack_push(calldata_word); let step_state_transition = StepStateTransition { rw_counter: Delta(cb.rw_counter_offset()), @@ -157,10 +191,10 @@ impl ExecutionGadget for CallDataLoadGadget { Self { same_context, - offset, src_id, call_data_length, call_data_offset, + data_offset, buffer_reader, } } @@ -176,62 +210,60 @@ impl ExecutionGadget for CallDataLoadGadget { ) -> Result<(), Error> { self.same_context.assign_exec_step(region, offset, step)?; - // set the value for bytes offset in calldata. This is where we start - // reading bytes from. - let data_offset = block.rws[step.rw_indices[0]].stack_value(); - - // assign the calldata start and end cells. - self.offset.assign( - region, - offset, - Some( - data_offset.to_le_bytes()[..N_BYTES_MEMORY_ADDRESS] - .try_into() - .unwrap(), - ), - )?; - - // assign to the buffer reader gadget. - let (calldata_length, calldata_offset, src_id) = if call.is_root { - (tx.call_data_length as u64, 0u64, tx.id as u64) + // Assign to the buffer reader gadget. + let (src_id, call_data_offset, call_data_length) = if call.is_root { + (tx.id, 0, tx.call_data_length as u64) } else { - ( - call.call_data_length, - call.call_data_offset, - call.caller_id as u64, - ) + (call.caller_id, call.call_data_offset, call.call_data_length) }; self.src_id - .assign(region, offset, Value::known(F::from(src_id)))?; + .assign(region, offset, Value::known(F::from(src_id as u64)))?; self.call_data_length - .assign(region, offset, Value::known(F::from(calldata_length)))?; + .assign(region, offset, Value::known(F::from(call_data_length)))?; self.call_data_offset - .assign(region, offset, Value::known(F::from(calldata_offset)))?; + .assign(region, offset, Value::known(F::from(call_data_offset)))?; - let mut calldata_bytes = vec![0u8; N_BYTES_WORD]; - let (src_addr, src_addr_end) = ( - data_offset.as_usize() + calldata_offset as usize, - calldata_length as usize + calldata_offset as usize, - ); + let data_offset = block.rws[step.rw_indices[0]].stack_value(); + let offset_within_range = + self.data_offset + .assign(region, offset, data_offset, F::from(call_data_length))?; - for (i, byte) in calldata_bytes.iter_mut().enumerate() { - if call.is_root { - // fetch from tx call data - if src_addr + i < tx.call_data_length { - *byte = tx.call_data[src_addr + i]; - } - } else { - // fetch from memory - if src_addr + i < (call.call_data_offset + call.call_data_length) as usize { - *byte = block.rws[step.rw_indices[OFFSET_RW_MEMORY_INDICES + i]].memory_value(); + let data_offset = if offset_within_range { + data_offset.as_u64() + } else { + call_data_length + }; + let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); + let src_addr = call_data_offset + .checked_add(data_offset) + .unwrap_or(src_addr_end) + .min(src_addr_end); + + let mut calldata_bytes = vec![0u8; N_BYTES_WORD]; + if offset_within_range { + for (i, byte) in calldata_bytes.iter_mut().enumerate() { + if call.is_root { + // Fetch from tx call data. + if src_addr.checked_add(i as u64).unwrap() < tx.call_data_length as u64 { + *byte = tx.call_data[src_addr as usize + i]; + } + } else { + // Fetch from memory. + if src_addr.checked_add(i as u64).unwrap() + < call.call_data_offset + call.call_data_length + { + *byte = + block.rws[step.rw_indices[OFFSET_RW_MEMORY_INDICES + i]].memory_value(); + } } } } + self.buffer_reader.assign( region, offset, - src_addr as u64, - src_addr_end as u64, + src_addr, + src_addr_end, &calldata_bytes, &[true; N_BYTES_WORD], )?; @@ -246,9 +278,9 @@ mod test { use eth_types::{bytecode, ToWord, Word}; use mock::TestContext; - fn test_root_ok(offset: usize) { + fn test_root_ok(offset: Word) { let bytecode = bytecode! { - PUSH32(Word::from(offset)) + PUSH32(offset) CALLDATALOAD STOP }; @@ -259,12 +291,12 @@ mod test { .run(); } - fn test_internal_ok(call_data_length: usize, call_data_offset: usize, offset: usize) { + fn test_internal_ok(call_data_length: usize, call_data_offset: usize, offset: Word) { let (addr_a, addr_b) = (mock::MOCK_ACCOUNTS[0], mock::MOCK_ACCOUNTS[1]); // code B gets called by code A, so the call is an internal call. let code_b = bytecode! { - PUSH32(Word::from(offset)) + PUSH32(offset) CALLDATALOAD STOP }; @@ -308,17 +340,23 @@ mod test { #[test] fn calldataload_gadget_root() { - test_root_ok(0x00); - test_root_ok(0x08); - test_root_ok(0x10); - test_root_ok(0x2010); + test_root_ok(0x00.into()); + test_root_ok(0x08.into()); + test_root_ok(0x10.into()); + test_root_ok(0x2010.into()); } #[test] fn calldataload_gadget_internal() { - test_internal_ok(0x20, 0x00, 0x00); - test_internal_ok(0x20, 0x10, 0x10); - test_internal_ok(0x40, 0x20, 0x08); - test_internal_ok(0x1010, 0xff, 0x10); + test_internal_ok(0x20, 0x00, 0x00.into()); + test_internal_ok(0x20, 0x10, 0x10.into()); + test_internal_ok(0x40, 0x20, 0x08.into()); + test_internal_ok(0x1010, 0xff, 0x10.into()); + } + + #[test] + fn calldataload_gadget_offset_overflow() { + test_root_ok(Word::MAX); + test_internal_ok(0x1010, 0xff, Word::MAX); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs index 878d9f05b8..da3843eea7 100644 --- a/zkevm-circuits/src/evm_circuit/execution/codecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/codecopy.rs @@ -1,17 +1,16 @@ use bus_mapping::{circuit_input_builder::CopyDataType, evm::OpcodeId}; -use eth_types::{evm_types::GasCost, Field, ToLittleEndian, ToScalar}; +use eth_types::{evm_types::GasCost, Field, ToScalar}; use halo2_proofs::{circuit::Value, plonk::Error}; use crate::{ evm_circuit::{ - param::{N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, + param::{N_BYTES_MEMORY_WORD_SIZE, N_BYTES_U64}, step::ExecutionState, util::{ - common_gadget::SameContextGadget, + common_gadget::{SameContextGadget, WordByteCapGadget}, constraint_builder::{ConstraintBuilder, StepStateTransition, Transition}, - from_bytes, memory_gadget::{MemoryAddressGadget, MemoryCopierGasGadget, MemoryExpansionGadget}, - not, CachedRegion, Cell, MemoryAddress, + not, select, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -23,8 +22,9 @@ 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 memory address for the offset in code from where we + /// read. It is valid if within range of Uint64 and less than code_size. + code_offset: WordByteCapGadget, /// Holds the size of the current environment's bytecode. code_size: Cell, /// The code from current environment is copied to memory. To verify this @@ -49,14 +49,15 @@ impl ExecutionGadget for CodeCopyGadget { fn configure(cb: &mut ConstraintBuilder) -> Self { let opcode = cb.query_cell(); - // Query elements to be popped from the stack. - let dst_memory_offset = cb.query_cell_phase2(); - let code_offset = cb.query_word_rlc(); + let code_size = cb.query_cell(); + let size = cb.query_word_rlc(); + let dst_memory_offset = cb.query_cell_phase2(); + let code_offset = WordByteCapGadget::construct(cb, code_size.expr()); // Pop items from stack. cb.stack_pop(dst_memory_offset.expr()); - cb.stack_pop(code_offset.expr()); + cb.stack_pop(code_offset.original_word()); cb.stack_pop(size.expr()); // Construct memory address in the destionation (memory) to which we copy code. @@ -66,7 +67,6 @@ impl ExecutionGadget for CodeCopyGadget { let code_hash = cb.curr.state.code_hash.clone(); // Fetch the bytecode length from the bytecode table. - let code_size = cb.query_cell(); cb.bytecode_length(code_hash.expr(), code_size.expr()); // Calculate the next memory size and the gas cost for this memory @@ -81,12 +81,19 @@ impl ExecutionGadget for CodeCopyGadget { let copy_rwc_inc = cb.query_cell(); cb.condition(dst_memory_addr.has_length(), |cb| { + // Set source start to the minimun value of code offset and code size. + let src_addr = select::expr( + code_offset.lt_cap(), + code_offset.valid_value(), + code_size.expr(), + ); + cb.copy_table_lookup( code_hash.expr(), CopyDataType::Bytecode.expr(), cb.curr.state.call_id.expr(), CopyDataType::Memory.expr(), - from_bytes::expr(&code_offset.cells), + src_addr, code_size.expr(), dst_memory_addr.offset(), dst_memory_addr.length(), @@ -145,26 +152,17 @@ impl ExecutionGadget for CodeCopyGadget { 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 code = block + let bytecode = block .bytecodes .get(&call.code_hash) .expect("could not find current environment's bytecode"); - self.code_size.assign( - region, - offset, - Value::known(F::from(code.bytes.len() as u64)), - )?; + + let code_size = bytecode.bytes.len() as u64; + self.code_size + .assign(region, offset, Value::known(F::from(code_size)))?; + + self.code_offset + .assign(region, offset, code_offset, F::from(code_size))?; // assign the destination memory offset. let memory_address = self @@ -200,16 +198,16 @@ mod tests { use eth_types::{bytecode, Word}; use mock::TestContext; - fn test_ok(memory_offset: usize, code_offset: usize, size: usize, large: bool) { + fn test_ok(code_offset: Word, memory_offset: usize, size: usize, large: bool) { let mut code = bytecode! {}; if large { - for _ in 0..0x101 { + for _ in 0..size { code.push(1, Word::from(123)); } } let tail = bytecode! { PUSH32(Word::from(size)) - PUSH32(Word::from(code_offset)) + PUSH32(code_offset) PUSH32(Word::from(memory_offset)) CODECOPY STOP @@ -224,13 +222,18 @@ mod tests { #[test] fn codecopy_gadget_simple() { - test_ok(0x00, 0x00, 0x20, false); - test_ok(0x20, 0x30, 0x30, false); - test_ok(0x10, 0x20, 0x42, false); + test_ok(0x00.into(), 0x00, 0x20, false); + test_ok(0x30.into(), 0x20, 0x30, false); + test_ok(0x20.into(), 0x10, 0x42, false); } #[test] fn codecopy_gadget_large() { - test_ok(0x103, 0x102, 0x101, true); + test_ok(0x102.into(), 0x103, 0x101, true); + } + + #[test] + fn codecopy_gadget_code_offset_overflow() { + test_ok(Word::MAX, 0x103, 0x101, true); } } diff --git a/zkevm-circuits/src/evm_circuit/execution/error_invalid_jump.rs b/zkevm-circuits/src/evm_circuit/execution/error_invalid_jump.rs index 7d16b76eef..d26eebdc8d 100644 --- a/zkevm-circuits/src/evm_circuit/execution/error_invalid_jump.rs +++ b/zkevm-circuits/src/evm_circuit/execution/error_invalid_jump.rs @@ -4,30 +4,26 @@ use crate::{ param::N_BYTES_PROGRAM_COUNTER, step::ExecutionState, util::{ - and, - common_gadget::CommonErrorGadget, + common_gadget::{CommonErrorGadget, WordByteCapGadget}, constraint_builder::ConstraintBuilder, - from_bytes, - math_gadget::{IsEqualGadget, IsZeroGadget, LtGadget}, - select, sum, CachedRegion, Cell, Word, + math_gadget::{IsEqualGadget, IsZeroGadget}, + CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, util::Expr, }; -use eth_types::{evm_types::OpcodeId, Field, ToLittleEndian, U256}; +use eth_types::{evm_types::OpcodeId, Field, U256}; use halo2_proofs::{circuit::Value, plonk::Error}; #[derive(Clone, Debug)] pub(crate) struct ErrorInvalidJumpGadget { opcode: Cell, - dest_word: Word, + dest: WordByteCapGadget, code_len: Cell, value: Cell, is_code: Cell, - dest_not_overflow: IsZeroGadget, - dest_lt_code_len: LtGadget, is_jump_dest: IsEqualGadget, is_jumpi: IsEqualGadget, phase2_condition: Cell, @@ -41,14 +37,8 @@ impl ExecutionGadget for ErrorInvalidJumpGadget { const EXECUTION_STATE: ExecutionState = ExecutionState::ErrorInvalidJump; fn configure(cb: &mut ConstraintBuilder) -> Self { - let dest_word = cb.query_word_rlc(); - let dest_not_overflow = - IsZeroGadget::construct(cb, sum::expr(&dest_word.cells[N_BYTES_PROGRAM_COUNTER..])); - let dest = select::expr( - dest_not_overflow.expr(), - from_bytes::expr(&dest_word.cells[..N_BYTES_PROGRAM_COUNTER]), - u64::MAX.expr(), - ); + let code_len = cb.query_cell(); + let dest = WordByteCapGadget::construct(cb, code_len.expr()); let opcode = cb.query_cell(); let value = cb.query_cell(); @@ -71,7 +61,7 @@ impl ExecutionGadget for ErrorInvalidJumpGadget { let is_condition_zero = IsZeroGadget::construct(cb, phase2_condition.expr()); // Pop the value from the stack - cb.stack_pop(dest_word.expr()); + cb.stack_pop(dest.original_word()); cb.condition(is_jumpi.expr(), |cb| { cb.stack_pop(phase2_condition.expr()); @@ -80,39 +70,31 @@ impl ExecutionGadget for ErrorInvalidJumpGadget { }); // Look up bytecode length - let code_len = cb.query_cell(); cb.bytecode_length(cb.curr.state.code_hash.expr(), code_len.expr()); - let dest_lt_code_len = LtGadget::construct(cb, dest.expr(), code_len.expr()); - // If destination is in valid range, lookup for the value. - cb.condition( - and::expr([dest_not_overflow.expr(), dest_lt_code_len.expr()]), - |cb| { - cb.bytecode_lookup( - cb.curr.state.code_hash.expr(), - dest.expr(), - is_code.expr(), - value.expr(), - ); - cb.require_zero( - "is_code is false or not JUMPDEST", - is_code.expr() * is_jump_dest.expr(), - ); - }, - ); + cb.condition(dest.lt_cap(), |cb| { + cb.bytecode_lookup( + cb.curr.state.code_hash.expr(), + dest.valid_value(), + is_code.expr(), + value.expr(), + ); + cb.require_zero( + "is_code is false or not JUMPDEST", + is_code.expr() * is_jump_dest.expr(), + ); + }); let common_error_gadget = CommonErrorGadget::construct(cb, opcode.expr(), 3.expr() + is_jumpi.expr()); Self { opcode, - dest_word, + dest, code_len, value, is_code, - dest_not_overflow, - dest_lt_code_len, is_jump_dest, is_jumpi, phase2_condition, @@ -135,10 +117,6 @@ impl ExecutionGadget for ErrorInvalidJumpGadget { self.opcode .assign(region, offset, Value::known(F::from(opcode.as_u64())))?; - let dest = block.rws[step.rw_indices[0]].stack_value(); - self.dest_word - .assign(region, offset, Some(dest.to_le_bytes()))?; - let condition = if is_jumpi { block.rws[step.rw_indices[1]].stack_value() } else { @@ -154,19 +132,11 @@ impl ExecutionGadget for ErrorInvalidJumpGadget { self.code_len .assign(region, offset, Value::known(F::from(code_len)))?; - let dest_overflow_hi = dest.to_le_bytes()[N_BYTES_PROGRAM_COUNTER..] - .iter() - .fold(0, |acc, val| acc + u64::from(*val)); - self.dest_not_overflow - .assign(region, offset, F::from(dest_overflow_hi))?; - - let dest = if dest_overflow_hi == 0 { - dest.low_u64() - } else { - u64::MAX - }; + let dest = block.rws[step.rw_indices[0]].stack_value(); + self.dest.assign(region, offset, dest, F::from(code_len))?; // set default value in case can not find value, is_code from bytecode table + let dest = u64::try_from(dest).unwrap_or(code_len); let mut code_pair = [0u8, 0u8]; if dest < code_len { // get real value from bytecode table @@ -184,9 +154,6 @@ impl ExecutionGadget for ErrorInvalidJumpGadget { F::from(OpcodeId::JUMPDEST.as_u64()), )?; - self.dest_lt_code_len - .assign(region, offset, F::from(dest), F::from(code_len))?; - self.is_jumpi.assign( region, offset, diff --git a/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs b/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs index 7949522b2b..cd493b90dd 100644 --- a/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs @@ -1,15 +1,15 @@ use crate::{ evm_circuit::{ - param::{N_BYTES_ACCOUNT_ADDRESS, N_BYTES_MEMORY_ADDRESS, N_BYTES_MEMORY_WORD_SIZE}, + param::{N_BYTES_ACCOUNT_ADDRESS, N_BYTES_MEMORY_WORD_SIZE, N_BYTES_U64}, step::ExecutionState, util::{ - common_gadget::SameContextGadget, + common_gadget::{SameContextGadget, WordByteCapGadget}, constraint_builder::{ ConstraintBuilder, ReversionInfo, StepStateTransition, Transition, }, from_bytes, memory_gadget::{MemoryAddressGadget, MemoryCopierGasGadget, MemoryExpansionGadget}, - not, select, CachedRegion, Cell, MemoryAddress, Word, + not, select, CachedRegion, Cell, Word, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -27,7 +27,7 @@ pub(crate) struct ExtcodecopyGadget { same_context: SameContextGadget, external_address_word: Word, memory_address: MemoryAddressGadget, - data_offset: MemoryAddress, + code_offset: WordByteCapGadget, tx_id: Cell, reversion_info: ReversionInfo, is_warm: Cell, @@ -50,17 +50,17 @@ impl ExecutionGadget for ExtcodecopyGadget { let external_address = from_bytes::expr(&external_address_word.cells[..N_BYTES_ACCOUNT_ADDRESS]); - let memory_offset = cb.query_cell_phase2(); - let data_offset = cb.query_word_rlc(); + let code_size = cb.query_cell(); + let memory_length = cb.query_word_rlc(); + let memory_offset = cb.query_cell_phase2(); + let code_offset = WordByteCapGadget::construct(cb, code_size.expr()); cb.stack_pop(external_address_word.expr()); cb.stack_pop(memory_offset.expr()); - cb.stack_pop(data_offset.expr()); + cb.stack_pop(code_offset.original_word()); cb.stack_pop(memory_length.expr()); - let memory_address = MemoryAddressGadget::construct(cb, memory_offset, memory_length); - let tx_id = cb.call_context(None, CallContextFieldTag::TxId); let mut reversion_info = cb.reversion_info_read(None); let is_warm = cb.query_bool(); @@ -78,13 +78,13 @@ impl ExecutionGadget for ExtcodecopyGadget { AccountFieldTag::CodeHash, code_hash.expr(), ); - let code_size = cb.query_cell(); // TODO: If external_address doesn't exist, we will get code_hash = 0. With // this value, the bytecode_length lookup will not work, and the copy // from code_hash = 0 will not work. We should use EMPTY_HASH when // code_hash = 0. cb.bytecode_length(code_hash.expr(), code_size.expr()); + let memory_address = MemoryAddressGadget::construct(cb, memory_offset, memory_length); let memory_expansion = MemoryExpansionGadget::construct(cb, [memory_address.address()]); let memory_copier_gas = MemoryCopierGasGadget::construct( cb, @@ -100,12 +100,19 @@ impl ExecutionGadget for ExtcodecopyGadget { let copy_rwc_inc = cb.query_cell(); cb.condition(memory_address.has_length(), |cb| { + // Set source start to the minimun value of code offset and code size. + let src_addr = select::expr( + code_offset.lt_cap(), + code_offset.valid_value(), + code_size.expr(), + ); + cb.copy_table_lookup( code_hash.expr(), CopyDataType::Bytecode.expr(), cb.curr.state.call_id.expr(), CopyDataType::Memory.expr(), - from_bytes::expr(&data_offset.cells), + src_addr, code_size.expr(), memory_address.offset(), memory_address.length(), @@ -135,7 +142,7 @@ impl ExecutionGadget for ExtcodecopyGadget { same_context, external_address_word, memory_address, - data_offset, + code_offset, tx_id, is_warm, reversion_info, @@ -158,22 +165,13 @@ impl ExecutionGadget for ExtcodecopyGadget { ) -> Result<(), Error> { self.same_context.assign_exec_step(region, offset, step)?; - let [external_address, memory_offset, data_offset, memory_length] = + let [external_address, memory_offset, code_offset, memory_length] = [0, 1, 2, 3].map(|idx| block.rws[step.rw_indices[idx]].stack_value()); self.external_address_word .assign(region, offset, Some(external_address.to_le_bytes()))?; let memory_address = self.memory_address .assign(region, offset, memory_offset, memory_length)?; - self.data_offset.assign( - region, - offset, - Some( - data_offset.to_le_bytes()[..N_BYTES_MEMORY_ADDRESS] - .try_into() - .unwrap(), - ), - )?; self.tx_id .assign(region, offset, Value::known(F::from(transaction.id as u64)))?; @@ -192,7 +190,7 @@ impl ExecutionGadget for ExtcodecopyGadget { self.code_hash .assign(region, offset, region.word_rlc(code_hash))?; - let bytecode_len = if code_hash.is_zero() { + let code_size = if code_hash.is_zero() { 0 } else { block @@ -200,10 +198,13 @@ impl ExecutionGadget for ExtcodecopyGadget { .get(&code_hash) .expect("could not find external bytecode") .bytes - .len() + .len() as u64 }; self.code_size - .assign(region, offset, Value::known(F::from(bytecode_len as u64)))?; + .assign(region, offset, Value::known(F::from(code_size)))?; + + self.code_offset + .assign(region, offset, code_offset, F::from(code_size))?; self.copy_rwc_inc.assign( region, @@ -249,8 +250,8 @@ mod test { fn test_ok( external_account: Option, + code_offset: Word, memory_offset: usize, - data_offset: usize, length: usize, is_warm: bool, ) { @@ -270,7 +271,7 @@ mod test { } code.append(&bytecode! { PUSH32(length) - PUSH32(data_offset) + PUSH32(code_offset) PUSH32(memory_offset) PUSH20(external_address.to_word()) #[start] @@ -310,8 +311,8 @@ mod test { #[test] fn extcodecopy_empty_account() { - test_ok(None, 0x00, 0x00, 0x36, true); // warm account - test_ok(None, 0x00, 0x00, 0x36, false); // cold account + test_ok(None, 0x00.into(), 0x00, 0x36, true); // warm account + test_ok(None, 0x00.into(), 0x00, 0x36, false); // cold account } #[test] @@ -322,7 +323,7 @@ mod test { code: Bytes::from([10, 40]), ..Default::default() }), - 0x00, + 0x00.into(), 0x00, 0x36, true, @@ -334,7 +335,7 @@ mod test { code: Bytes::from([10, 40]), ..Default::default() }), - 0x00, + 0x00.into(), 0x00, 0x36, false, @@ -349,7 +350,7 @@ mod test { code: Bytes::from(rand_bytes_array::<256>()), ..Default::default() }), - 0x00, + 0x00.into(), 0x00, 0x36, true, @@ -360,7 +361,7 @@ mod test { code: Bytes::from(rand_bytes_array::<256>()), ..Default::default() }), - 0x00, + 0x00.into(), 0x00, 0x36, false, @@ -375,8 +376,8 @@ mod test { code: Bytes::from(rand_bytes_array::<64>()), ..Default::default() }), + 0x20.into(), 0x00, - 0x20, 0x104, true, ); @@ -386,10 +387,36 @@ mod test { code: Bytes::from(rand_bytes_array::<64>()), ..Default::default() }), + 0x20.into(), 0x00, - 0x20, 0x104, false, ); } + + #[test] + fn extcodecopy_code_offset_overflow() { + test_ok( + Some(Account { + address: *EXTERNAL_ADDRESS, + code: Bytes::from(rand_bytes_array::<256>()), + ..Default::default() + }), + Word::MAX, + 0x00, + 0x36, + true, + ); + test_ok( + Some(Account { + address: *EXTERNAL_ADDRESS, + code: Bytes::from(rand_bytes_array::<256>()), + ..Default::default() + }), + Word::MAX, + 0x00, + 0x36, + false, + ); + } } diff --git a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs index e8df2b822f..f604f97894 100644 --- a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs +++ b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs @@ -4,26 +4,25 @@ use crate::{ param::N_BYTES_PROGRAM_COUNTER, step::ExecutionState, util::{ - common_gadget::SameContextGadget, + common_gadget::{SameContextGadget, WordByteRangeGadget}, constraint_builder::{ ConstraintBuilder, StepStateTransition, Transition::{Delta, To}, }, - from_bytes, math_gadget::IsZeroGadget, - select, CachedRegion, Cell, RandomLinearCombination, + select, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, util::Expr, }; -use eth_types::{evm_types::OpcodeId, Field, ToLittleEndian}; +use eth_types::{evm_types::OpcodeId, Field}; use halo2_proofs::plonk::Error; #[derive(Clone, Debug)] pub(crate) struct JumpiGadget { same_context: SameContextGadget, - destination: RandomLinearCombination, + dest: WordByteRangeGadget, phase2_condition: Cell, is_condition_zero: IsZeroGadget, } @@ -34,11 +33,11 @@ impl ExecutionGadget for JumpiGadget { const EXECUTION_STATE: ExecutionState = ExecutionState::JUMPI; fn configure(cb: &mut ConstraintBuilder) -> Self { - let destination = cb.query_word_rlc(); + let dest = WordByteRangeGadget::construct(cb); let phase2_condition = cb.query_cell_phase2(); // Pop the value from the stack - cb.stack_pop(destination.expr()); + cb.stack_pop(dest.original_word()); cb.stack_pop(phase2_condition.expr()); // Determine if the jump condition is met @@ -47,18 +46,25 @@ impl ExecutionGadget for JumpiGadget { // Lookup opcode at destination when should_jump cb.condition(should_jump.clone(), |cb| { - cb.opcode_lookup_at( - from_bytes::expr(&destination.cells), - OpcodeId::JUMPDEST.expr(), + cb.require_equal( + "JUMPI destination must be within range if condition is non-zero", + dest.within_range(), 1.expr(), ); + + cb.opcode_lookup_at(dest.valid_value(), OpcodeId::JUMPDEST.expr(), 1.expr()); }); + cb.require_zero( + "JUMPI condition must be 0 if destination is Uint64 overflow", + dest.overflow() * not::expr(is_condition_zero.expr()), + ); + // Transit program_counter to destination when should_jump, otherwise by // delta 1. let next_program_counter = select::expr( should_jump, - from_bytes::expr(&destination.cells), + dest.valid_value(), cb.curr.state.program_counter.expr() + 1.expr(), ); @@ -75,7 +81,7 @@ impl ExecutionGadget for JumpiGadget { Self { same_context, - destination, + dest, phase2_condition, is_condition_zero, } @@ -96,15 +102,7 @@ impl ExecutionGadget for JumpiGadget { [step.rw_indices[0], step.rw_indices[1]].map(|idx| block.rws[idx].stack_value()); let condition = region.word_rlc(condition); - self.destination.assign( - region, - offset, - Some( - destination.to_le_bytes()[..N_BYTES_PROGRAM_COUNTER] - .try_into() - .unwrap(), - ), - )?; + self.dest.assign(region, offset, destination)?; self.phase2_condition.assign(region, offset, condition)?; self.is_condition_zero .assign_value(region, offset, condition)?; @@ -174,4 +172,18 @@ mod test { test_ok(rand_range(1 << 11..0x5fff), Word::zero()); test_ok(rand_range(1 << 11..0x5fff), rand_word()); } + + #[test] + fn jumpi_gadget_with_zero_cond_and_overflow_dest() { + let bytecode = bytecode! { + PUSH32(Word::MAX) + PUSH32(Word::zero()) + JUMPI + }; + + CircuitTestBuilder::new_from_test_ctx( + TestContext::<2, 1>::simple_ctx_with_bytecode(bytecode).unwrap(), + ) + .run(); + } } diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 907ca68d00..6bb575217b 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -1,6 +1,6 @@ use super::{ from_bytes, - math_gadget::{IsEqualGadget, IsZeroGadget}, + math_gadget::{IsEqualGadget, IsZeroGadget, LtGadget}, memory_gadget::{MemoryAddressGadget, MemoryExpansionGadget}, CachedRegion, }; @@ -1039,3 +1039,125 @@ impl CommonErrorGadget { Ok(1u64) } } + +/// Check if the passed in word is within the specified byte range and less than +/// a maximum cap. +#[derive(Clone, Debug)] +pub(crate) struct WordByteCapGadget { + word: WordByteRangeGadget, + lt_cap: LtGadget, +} + +impl WordByteCapGadget { + pub(crate) fn construct(cb: &mut ConstraintBuilder, cap: Expression) -> Self { + let word = WordByteRangeGadget::construct(cb); + let value = select::expr(word.within_range(), word.valid_value(), cap.expr()); + let lt_cap = LtGadget::construct(cb, value, cap); + + Self { word, lt_cap } + } + + /// Return true if within the specified byte range, false if overflow. No + /// matter whether it is less than the cap. + pub(crate) fn assign( + &self, + region: &mut CachedRegion<'_, '_, F>, + offset: usize, + original: U256, + cap: F, + ) -> Result { + let within_range = self.word.assign(region, offset, original)?; + + let value = if within_range { + let mut bytes = [0; 32]; + bytes[0..VALID_BYTES].copy_from_slice(&original.to_le_bytes()[0..VALID_BYTES]); + F::from_repr(bytes).unwrap() + } else { + cap + }; + + self.lt_cap.assign(region, offset, value, cap)?; + + Ok(within_range) + } + + pub(crate) fn lt_cap(&self) -> Expression { + self.lt_cap.expr() + } + + pub(crate) fn original_word(&self) -> Expression { + self.word.original_word() + } + + pub(crate) fn overflow(&self) -> Expression { + self.word.overflow() + } + + pub(crate) fn valid_value(&self) -> Expression { + self.word.valid_value() + } + + pub(crate) fn within_range(&self) -> Expression { + self.word.within_range() + } +} + +/// Check if the passed in word is within the specified byte range. +#[derive(Clone, Debug)] +pub(crate) struct WordByteRangeGadget { + original: Word, + within_range: IsZeroGadget, +} + +impl WordByteRangeGadget { + pub(crate) fn construct(cb: &mut ConstraintBuilder) -> Self { + debug_assert!(VALID_BYTES < 32); + + let original = cb.query_word_rlc(); + let within_range = IsZeroGadget::construct(cb, sum::expr(&original.cells[VALID_BYTES..])); + + Self { + original, + within_range, + } + } + + /// Return true if within the range, false if overflow. + pub(crate) fn assign( + &self, + region: &mut CachedRegion<'_, '_, F>, + offset: usize, + original: U256, + ) -> Result { + debug_assert!(VALID_BYTES < 32); + + self.original + .assign(region, offset, Some(original.to_le_bytes()))?; + + let overflow_hi = original.to_le_bytes()[VALID_BYTES..] + .iter() + .fold(0, |acc, val| acc + u64::from(*val)); + self.within_range + .assign(region, offset, F::from(overflow_hi))?; + + Ok(overflow_hi == 0) + } + + pub(crate) fn original_word(&self) -> Expression { + self.original.expr() + } + + pub(crate) fn overflow(&self) -> Expression { + not::expr(self.within_range()) + } + + pub(crate) fn valid_value(&self) -> Expression { + debug_assert!(VALID_BYTES < 32); + + from_bytes::expr(&self.original.cells[..VALID_BYTES]) + } + + pub(crate) fn within_range(&self) -> Expression { + self.within_range.expr() + } +} From 12669db765e2dba1c4f7339aca78c9965c7e1b83 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Mon, 13 Mar 2023 18:23:38 +0800 Subject: [PATCH 02/12] Fix lint. --- bus-mapping/src/circuit_input_builder/input_state_ref.rs | 2 +- zkevm-circuits/src/evm_circuit/execution/jumpi.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bus-mapping/src/circuit_input_builder/input_state_ref.rs b/bus-mapping/src/circuit_input_builder/input_state_ref.rs index 04eeac81e5..9a67b54338 100644 --- a/bus-mapping/src/circuit_input_builder/input_state_ref.rs +++ b/bus-mapping/src/circuit_input_builder/input_state_ref.rs @@ -20,7 +20,7 @@ use eth_types::{ evm_types::{ gas_utils::memory_expansion_gas_cost, Gas, GasCost, MemoryAddress, OpcodeId, StackAddress, }, - Address, Bytecode, GethExecStep, H256, ToAddress, ToBigEndian, ToWord, U256, Word, + Address, Bytecode, GethExecStep, ToAddress, ToBigEndian, ToWord, Word, H256, U256, }; use ethers_core::utils::{get_contract_address, get_create2_address}; use keccak256::EMPTY_HASH_LE; diff --git a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs index f604f97894..8acb602b7e 100644 --- a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs +++ b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs @@ -10,7 +10,7 @@ use crate::{ Transition::{Delta, To}, }, math_gadget::IsZeroGadget, - select, CachedRegion, Cell, + not, select, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, From 78931f3627a0044bbab8b70ba105fc15281f5a39 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Mon, 13 Mar 2023 18:49:14 +0800 Subject: [PATCH 03/12] Fix failed cases. --- zkevm-circuits/src/evm_circuit/execution/blockhash.rs | 3 +-- zkevm-circuits/src/witness/step.rs | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/blockhash.rs b/zkevm-circuits/src/evm_circuit/execution/blockhash.rs index 12eff1dcd2..4537b4eca6 100644 --- a/zkevm-circuits/src/evm_circuit/execution/blockhash.rs +++ b/zkevm-circuits/src/evm_circuit/execution/blockhash.rs @@ -109,7 +109,6 @@ impl ExecutionGadget for BlockHashGadget { let block_number = block.rws[step.rw_indices[0]].stack_value(); self.block_number .assign(region, offset, block_number, current_block_number)?; - let block_number: F = block_number.to_scalar().unwrap(); self.current_block_number .assign(region, offset, Value::known(current_block_number))?; @@ -124,7 +123,7 @@ impl ExecutionGadget for BlockHashGadget { region, offset, current_block_number, - block_number + F::from(257), + F::from(u64::try_from(block_number).unwrap_or(u64::MAX)) + F::from(257), )?; Ok(()) diff --git a/zkevm-circuits/src/witness/step.rs b/zkevm-circuits/src/witness/step.rs index bcc8e3bd37..e330705258 100644 --- a/zkevm-circuits/src/witness/step.rs +++ b/zkevm-circuits/src/witness/step.rs @@ -181,11 +181,11 @@ impl From<&circuit_input_builder::ExecStep> for ExecutionState { OpcodeId::CODECOPY => ExecutionState::CODECOPY, OpcodeId::CALLDATALOAD => ExecutionState::CALLDATALOAD, OpcodeId::CODESIZE => ExecutionState::CODESIZE, + OpcodeId::EXTCODECOPY => ExecutionState::EXTCODECOPY, OpcodeId::RETURN | OpcodeId::REVERT => ExecutionState::RETURN_REVERT, OpcodeId::RETURNDATASIZE => ExecutionState::RETURNDATASIZE, OpcodeId::RETURNDATACOPY => ExecutionState::RETURNDATACOPY, // dummy ops - OpcodeId::EXTCODECOPY => dummy!(ExecutionState::EXTCODECOPY), OpcodeId::CREATE => dummy!(ExecutionState::CREATE), OpcodeId::CREATE2 => dummy!(ExecutionState::CREATE2), OpcodeId::SELFDESTRUCT => dummy!(ExecutionState::SELFDESTRUCT), From 82505e23ff4ac03ef61b20b51e441dfb1d892ccb Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Tue, 18 Apr 2023 17:11:28 +0800 Subject: [PATCH 04/12] Fix failed tests. --- zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs | 2 +- zkevm-circuits/src/evm_circuit/execution/calldataload.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 81cf716018..e6da35f370 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -311,7 +311,7 @@ mod test { // code B gets called by code A, so the call is an internal call. let code_b = bytecode! { - .calldatacopy(dst_offset, offset, length) + .calldatacopy(dst_offset, data_offset, length) STOP }; diff --git a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs index 6d0959b24f..f054c9a207 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs @@ -286,7 +286,7 @@ mod test { } } - fn test_root_ok(offset: usize) { + fn test_root_ok(offset: Word) { let bytecode = test_bytecode(offset); CircuitTestBuilder::new_from_test_ctx( From c17ba9dfd72422fdeeb28bcf2405ace7773053f7 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Tue, 18 Apr 2023 17:31:21 +0800 Subject: [PATCH 05/12] Try to trigger CI for network failure. --- Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.toml b/Cargo.toml index 100836d5ce..cffc7d490b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,6 +13,7 @@ members = [ "testool" ] + [patch.crates-io] halo2_proofs = { git = "https://github.com/privacy-scaling-explorations/halo2.git", tag = "v2023_02_02" } From ee29e935e83cee991623d1f58715523fc9afb59c Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Tue, 18 Apr 2023 17:31:45 +0800 Subject: [PATCH 06/12] Revert "Try to trigger CI for network failure." This reverts commit c17ba9dfd72422fdeeb28bcf2405ace7773053f7. --- Cargo.toml | 1 - 1 file changed, 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index cffc7d490b..100836d5ce 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,6 @@ members = [ "testool" ] - [patch.crates-io] halo2_proofs = { git = "https://github.com/privacy-scaling-explorations/halo2.git", tag = "v2023_02_02" } From 138378f5636cf54faf78161d9eba4530ee05f6c9 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Mon, 24 Apr 2023 09:54:06 +0800 Subject: [PATCH 07/12] Fix lint. --- zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs index 62e897afff..ba4301f288 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldatacopy.rs @@ -311,7 +311,7 @@ mod test { // code B gets called by code A, so the call is an internal call. let code_b = bytecode! { - .op_calldatacopy(dst_offset, offset, length) + .op_calldatacopy(dst_offset, data_offset, length) STOP }; From 29cf64cc21e305a9248a649f1f8e7fb89917f985 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Thu, 27 Apr 2023 14:43:39 +0800 Subject: [PATCH 08/12] Delete `debug_assert!(VALID_BYTES < 32)` in `assign` and `valid_value` function (only leave it in `construct`). --- zkevm-circuits/src/evm_circuit/util/common_gadget.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 19e417b91a..b4e9445b23 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -1127,8 +1127,6 @@ impl WordByteRangeGadget { offset: usize, original: U256, ) -> Result { - debug_assert!(VALID_BYTES < 32); - self.original .assign(region, offset, Some(original.to_le_bytes()))?; @@ -1150,8 +1148,6 @@ impl WordByteRangeGadget { } pub(crate) fn valid_value(&self) -> Expression { - debug_assert!(VALID_BYTES < 32); - from_bytes::expr(&self.original.cells[..VALID_BYTES]) } From 787093ffc611c084e83d786ff64245768163aff3 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Thu, 27 Apr 2023 15:56:01 +0800 Subject: [PATCH 09/12] Delete redundant constraint `JUMPI condition must be 0 if destination is Uint64 overflow` in `jumpi`. --- zkevm-circuits/src/evm_circuit/execution/jumpi.rs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs index e06feb7acc..d954d774c7 100644 --- a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs +++ b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs @@ -10,7 +10,7 @@ use crate::{ Transition::{Delta, To}, }, math_gadget::IsZeroGadget, - not, select, CachedRegion, Cell, + select, CachedRegion, Cell, }, witness::{Block, Call, ExecStep, Transaction}, }, @@ -55,11 +55,6 @@ impl ExecutionGadget for JumpiGadget { cb.opcode_lookup_at(dest.valid_value(), OpcodeId::JUMPDEST.expr(), 1.expr()); }); - cb.require_zero( - "JUMPI condition must be 0 if destination is Uint64 overflow", - dest.overflow() * not::expr(is_condition_zero.expr()), - ); - // Transit program_counter to destination when should_jump, otherwise by // delta 1. let next_program_counter = select::expr( From 11f8eb326a88c65be6c234856fca8102a29fbbb0 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Thu, 27 Apr 2023 16:24:24 +0800 Subject: [PATCH 10/12] Rename `within_range` to `not_overflow`. --- .../src/evm_circuit/execution/calldataload.rs | 14 ++++---- .../src/evm_circuit/execution/jumpi.rs | 2 +- .../src/evm_circuit/util/common_gadget.rs | 36 +++++++++---------- 3 files changed, 26 insertions(+), 26 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs index cf2ba6d006..80dd2c4273 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs @@ -68,7 +68,7 @@ impl ExecutionGadget for CallDataLoadGadget { cb.stack_pop(data_offset.original_word()); cb.condition( - and::expr([data_offset.within_range(), cb.curr.state.is_root.expr()]), + and::expr([data_offset.not_overflow(), cb.curr.state.is_root.expr()]), |cb| { cb.call_context_lookup( false.expr(), @@ -92,7 +92,7 @@ impl ExecutionGadget for CallDataLoadGadget { cb.condition( and::expr([ - data_offset.within_range(), + data_offset.not_overflow(), not::expr(cb.curr.state.is_root.expr()), ]), |cb| { @@ -134,7 +134,7 @@ impl ExecutionGadget for CallDataLoadGadget { // For a root call, the call data comes from tx's data field. cb.condition( and::expr([ - data_offset.within_range(), + data_offset.not_overflow(), buffer_reader.read_flag(idx), cb.curr.state.is_root.expr(), ]), @@ -150,7 +150,7 @@ impl ExecutionGadget for CallDataLoadGadget { // For an internal call, the call data comes from memory. cb.condition( and::expr([ - data_offset.within_range(), + data_offset.not_overflow(), buffer_reader.read_flag(idx), not::expr(cb.curr.state.is_root.expr()), ]), @@ -227,11 +227,11 @@ impl ExecutionGadget for CallDataLoadGadget { .assign(region, offset, Value::known(F::from(call_data_offset)))?; let data_offset = block.rws[step.rw_indices[0]].stack_value(); - let offset_within_range = + let offset_not_overflow = self.data_offset .assign(region, offset, data_offset, F::from(call_data_length))?; - let data_offset = if offset_within_range { + let data_offset = if offset_not_overflow { data_offset.as_u64() } else { call_data_length @@ -243,7 +243,7 @@ impl ExecutionGadget for CallDataLoadGadget { .min(src_addr_end); let mut calldata_bytes = vec![0u8; N_BYTES_WORD]; - if offset_within_range { + if offset_not_overflow { for (i, byte) in calldata_bytes.iter_mut().enumerate() { if call.is_root { // Fetch from tx call data. diff --git a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs index d954d774c7..4a463bfd5a 100644 --- a/zkevm-circuits/src/evm_circuit/execution/jumpi.rs +++ b/zkevm-circuits/src/evm_circuit/execution/jumpi.rs @@ -48,7 +48,7 @@ impl ExecutionGadget for JumpiGadget { cb.condition(should_jump.clone(), |cb| { cb.require_equal( "JUMPI destination must be within range if condition is non-zero", - dest.within_range(), + dest.not_overflow(), 1.expr(), ); diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index b4e9445b23..b2761a7f1b 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -1038,8 +1038,8 @@ impl CommonErrorGadget { } } -/// Check if the passed in word is within the specified byte range and less than -/// a maximum cap. +/// Check if the passed in word is within the specified byte range (not overflow) +/// and less than a maximum cap. #[derive(Clone, Debug)] pub(crate) struct WordByteCapGadget { word: WordByteRangeGadget, @@ -1049,14 +1049,14 @@ pub(crate) struct WordByteCapGadget { impl WordByteCapGadget { pub(crate) fn construct(cb: &mut EVMConstraintBuilder, cap: Expression) -> Self { let word = WordByteRangeGadget::construct(cb); - let value = select::expr(word.within_range(), word.valid_value(), cap.expr()); + let value = select::expr(word.overflow(), cap.expr(), word.valid_value()); let lt_cap = LtGadget::construct(cb, value, cap); Self { word, lt_cap } } - /// Return true if within the specified byte range, false if overflow. No - /// matter whether it is less than the cap. + /// Return true if within the specified byte range (not overflow), false if overflow. + /// No matter whether it is less than the cap. pub(crate) fn assign( &self, region: &mut CachedRegion<'_, '_, F>, @@ -1064,9 +1064,9 @@ impl WordByteCapGadget { original: U256, cap: F, ) -> Result { - let within_range = self.word.assign(region, offset, original)?; + let not_overflow = self.word.assign(region, offset, original)?; - let value = if within_range { + let value = if not_overflow { let mut bytes = [0; 32]; bytes[0..VALID_BYTES].copy_from_slice(&original.to_le_bytes()[0..VALID_BYTES]); F::from_repr(bytes).unwrap() @@ -1076,7 +1076,7 @@ impl WordByteCapGadget { self.lt_cap.assign(region, offset, value, cap)?; - Ok(within_range) + Ok(not_overflow) } pub(crate) fn lt_cap(&self) -> Expression { @@ -1095,16 +1095,16 @@ impl WordByteCapGadget { self.word.valid_value() } - pub(crate) fn within_range(&self) -> Expression { - self.word.within_range() + pub(crate) fn not_overflow(&self) -> Expression { + self.word.not_overflow() } } -/// Check if the passed in word is within the specified byte range. +/// Check if the passed in word is within the specified byte range (not overflow). #[derive(Clone, Debug)] pub(crate) struct WordByteRangeGadget { original: Word, - within_range: IsZeroGadget, + not_overflow: IsZeroGadget, } impl WordByteRangeGadget { @@ -1112,11 +1112,11 @@ impl WordByteRangeGadget { debug_assert!(VALID_BYTES < 32); let original = cb.query_word_rlc(); - let within_range = IsZeroGadget::construct(cb, sum::expr(&original.cells[VALID_BYTES..])); + let not_overflow = IsZeroGadget::construct(cb, sum::expr(&original.cells[VALID_BYTES..])); Self { original, - within_range, + not_overflow, } } @@ -1133,7 +1133,7 @@ impl WordByteRangeGadget { let overflow_hi = original.to_le_bytes()[VALID_BYTES..] .iter() .fold(0, |acc, val| acc + u64::from(*val)); - self.within_range + self.not_overflow .assign(region, offset, F::from(overflow_hi))?; Ok(overflow_hi == 0) @@ -1144,14 +1144,14 @@ impl WordByteRangeGadget { } pub(crate) fn overflow(&self) -> Expression { - not::expr(self.within_range()) + not::expr(self.not_overflow()) } pub(crate) fn valid_value(&self) -> Expression { from_bytes::expr(&self.original.cells[..VALID_BYTES]) } - pub(crate) fn within_range(&self) -> Expression { - self.within_range.expr() + pub(crate) fn not_overflow(&self) -> Expression { + self.not_overflow.expr() } } From 594553d6f19d4f8d6ed77198b7b519bb4297d833 Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Thu, 27 Apr 2023 16:30:03 +0800 Subject: [PATCH 11/12] Replace `checked_add` and `unwrap` with just adding. --- zkevm-circuits/src/evm_circuit/execution/calldataload.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs index 80dd2c4273..f443980188 100644 --- a/zkevm-circuits/src/evm_circuit/execution/calldataload.rs +++ b/zkevm-circuits/src/evm_circuit/execution/calldataload.rs @@ -236,7 +236,7 @@ impl ExecutionGadget for CallDataLoadGadget { } else { call_data_length }; - let src_addr_end = call_data_offset.checked_add(call_data_length).unwrap(); + let src_addr_end = call_data_offset + call_data_length; let src_addr = call_data_offset .checked_add(data_offset) .unwrap_or(src_addr_end) @@ -247,14 +247,12 @@ impl ExecutionGadget for CallDataLoadGadget { for (i, byte) in calldata_bytes.iter_mut().enumerate() { if call.is_root { // Fetch from tx call data. - if src_addr.checked_add(i as u64).unwrap() < tx.call_data_length as u64 { + if src_addr + (i as u64) < tx.call_data_length as u64 { *byte = tx.call_data[src_addr as usize + i]; } } else { // Fetch from memory. - if src_addr.checked_add(i as u64).unwrap() - < call.call_data_offset + call.call_data_length - { + if src_addr + (i as u64) < call.call_data_offset + call.call_data_length { *byte = block.rws[step.rw_indices[OFFSET_RW_MEMORY_INDICES + i]].memory_value(); } From 74a938b7979c556281d91f36858218903b10470e Mon Sep 17 00:00:00 2001 From: Steven Gu Date: Fri, 28 Apr 2023 19:19:04 +0800 Subject: [PATCH 12/12] Update some comments to retry CI. --- zkevm-circuits/src/evm_circuit/util/common_gadget.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index b2761a7f1b..6858c37d56 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -1038,8 +1038,8 @@ impl CommonErrorGadget { } } -/// Check if the passed in word is within the specified byte range (not overflow) -/// and less than a maximum cap. +/// Check if the passed in word is within the specified byte range +/// (not overflow) and less than a maximum cap. #[derive(Clone, Debug)] pub(crate) struct WordByteCapGadget { word: WordByteRangeGadget, @@ -1055,8 +1055,8 @@ impl WordByteCapGadget { Self { word, lt_cap } } - /// Return true if within the specified byte range (not overflow), false if overflow. - /// No matter whether it is less than the cap. + /// Return true if within the specified byte range (not overflow), false if + /// overflow. No matter whether it is less than the cap. pub(crate) fn assign( &self, region: &mut CachedRegion<'_, '_, F>,