diff --git a/bus-mapping/src/evm/opcodes/callop.rs b/bus-mapping/src/evm/opcodes/callop.rs index 74318b419f..7c9f25b973 100644 --- a/bus-mapping/src/evm/opcodes/callop.rs +++ b/bus-mapping/src/evm/opcodes/callop.rs @@ -100,7 +100,8 @@ impl Opcode for CallOpcode { )?; let callee_code_hash = call.code_hash; - let callee_exists = !state.sdb.get_account(&callee_address).1.is_empty(); + let callee = state.sdb.get_account(&callee_address).1.clone(); + let callee_exists = !callee.is_empty(); let (callee_code_hash_word, is_empty_code_hash) = if callee_exists { ( @@ -145,11 +146,13 @@ impl Opcode for CallOpcode { debug_assert!(found); let caller_balance = sender_account.balance; + let is_call_or_callcode = call.kind == CallKind::Call || call.kind == CallKind::CallCode; + let is_sufficient = caller_balance >= call.value; + let is_valid_depth = geth_step.depth < 1025; // Precheck is OK when depth is in range and caller balance is sufficient - let is_precheck_ok = - geth_step.depth < 1025 && (!is_call_or_callcode || caller_balance >= call.value); + let is_precheck_ok = is_valid_depth && (is_sufficient || !is_call_or_callcode); log::debug!( "is_precheck_ok: {}, call type: {:?}, sender_account: {:?} ", @@ -173,9 +176,11 @@ impl Opcode for CallOpcode { let is_precompile = code_address .map(|ref addr| is_precompiled(addr)) .unwrap_or(false); - // TODO: What about transfer for CALLCODE? - // Transfer value only for CALL opcode, is_precheck_ok = true. - if call.kind == CallKind::Call && is_precheck_ok { + // Transfer value only when all these conditions met: + // - The opcode is CALL + // - The precheck passed + // - The value to send is not zero + if call.kind == CallKind::Call && is_precheck_ok && !call.value.is_zero() { state.transfer( &mut exec_step, call.caller_address, @@ -221,9 +226,9 @@ impl Opcode for CallOpcode { // There are 4 branches from here. // add failure case for insufficient balance or error depth in the future. - match (!is_precheck_ok, is_precompile, is_empty_code_hash) { + match (is_precheck_ok, is_precompile, is_empty_code_hash) { // 1. Call to precompiled. - (false, true, _) => { + (true, true, _) => { assert!(call.is_success, "call to precompile should not fail"); let caller_ctx = state.caller_ctx_mut()?; let code_address = code_address.unwrap(); @@ -275,7 +280,7 @@ impl Opcode for CallOpcode { Ok(vec![exec_step]) } // 2. Call to account with empty code. - (false, _, true) => { + (true, _, true) => { for (field, value) in [ (CallContextField::LastCalleeId, 0.into()), (CallContextField::LastCalleeReturnDataOffset, 0.into()), @@ -287,7 +292,7 @@ impl Opcode for CallOpcode { Ok(vec![exec_step]) } // 3. Call to account with non-empty code. - (false, _, false) => { + (true, _, false) => { for (field, value) in [ (CallContextField::ProgramCounter, (geth_step.pc + 1).into()), ( @@ -349,7 +354,7 @@ impl Opcode for CallOpcode { } // 4. insufficient balance or error depth cases. - (true, _, _) => { + (false, _, _) => { for (field, value) in [ (CallContextField::LastCalleeId, 0.into()), (CallContextField::LastCalleeReturnDataOffset, 0.into()), diff --git a/zkevm-circuits/src/evm_circuit/execution/balance.rs b/zkevm-circuits/src/evm_circuit/execution/balance.rs index 9db943ffbe..cefc1e8a78 100644 --- a/zkevm-circuits/src/evm_circuit/execution/balance.rs +++ b/zkevm-circuits/src/evm_circuit/execution/balance.rs @@ -135,7 +135,7 @@ impl ExecutionGadget for BalanceGadget { self.is_warm .assign(region, offset, Value::known(F::from(is_warm as u64)))?; - let code_hash = block.get_rws(step, 5).account_value_pair().0; + let code_hash = block.get_rws(step, 5).account_codehash_pair().0; self.code_hash .assign_u256(region, offset, code_hash.to_word())?; self.not_exists @@ -143,7 +143,7 @@ impl ExecutionGadget for BalanceGadget { let balance = if code_hash.is_zero() { eth_types::Word::zero() } else { - block.get_rws(step, 6).account_value_pair().0 + block.get_rws(step, 6).account_balance_pair().0 }; self.balance.assign_u256(region, offset, balance)?; diff --git a/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs b/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs index c22adb67a3..4be0115c63 100644 --- a/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs +++ b/zkevm-circuits/src/evm_circuit/execution/begin_tx.rs @@ -514,20 +514,20 @@ impl ExecutionGadget for BeginTxGadget { let is_coinbase_warm = rws.next().tx_access_list_value_pair().1; let mut callee_code_hash = zero; if !is_precompiled(&tx.to_or_contract_addr()) && !tx.is_create() { - callee_code_hash = rws.next().account_value_pair().1; + callee_code_hash = rws.next().account_codehash_pair().1; } let callee_exists = is_precompiled(&tx.to_or_contract_addr()) || (!tx.is_create() && !callee_code_hash.is_zero()); - let caller_balance_sub_fee_pair = rws.next().account_value_pair(); + let caller_balance_sub_fee_pair = rws.next().account_balance_pair(); let must_create = tx.is_create(); if (!callee_exists && !tx.value.is_zero()) || must_create { - callee_code_hash = rws.next().account_value_pair().1; + callee_code_hash = rws.next().account_codehash_pair().1; } let mut caller_balance_sub_value_pair = (zero, zero); let mut callee_balance_pair = (zero, zero); if !tx.value.is_zero() { - caller_balance_sub_value_pair = rws.next().account_value_pair(); - callee_balance_pair = rws.next().account_value_pair(); + caller_balance_sub_value_pair = rws.next().account_balance_pair(); + callee_balance_pair = rws.next().account_balance_pair(); }; self.tx_id diff --git a/zkevm-circuits/src/evm_circuit/execution/callop.rs b/zkevm-circuits/src/evm_circuit/execution/callop.rs index f06fab13b6..be4a54316b 100644 --- a/zkevm-circuits/src/evm_circuit/execution/callop.rs +++ b/zkevm-circuits/src/evm_circuit/execution/callop.rs @@ -253,8 +253,7 @@ impl ExecutionGadget for CallOpGadget { // caller address and value (+2). // // No extra lookups for STATICCALL opcode. - let transfer_rwc_delta = - is_call.expr() * not::expr(transfer.value_is_zero.expr()) * 2.expr(); + let transfer_rwc_delta = is_call.expr() * transfer.reversible_w_delta(); let rw_counter_delta = 21.expr() + is_call.expr() * 1.expr() + transfer_rwc_delta.clone() @@ -484,7 +483,7 @@ impl ExecutionGadget for CallOpGadget { let depth = rws.next().call_context_value(); let current_callee_address = rws.next().call_context_value(); - let is_error_depth = depth.low_u64() > 1024; + let is_valid_depth = depth.low_u64() < 1025; self.is_depth_ok .assign(region, offset, F::from(depth.low_u64()), F::from(1025))?; // This offset is used to change the index offset of `step.rw_indices`. @@ -513,7 +512,7 @@ impl ExecutionGadget for CallOpGadget { let rd_length = rws.next().stack_value(); let is_success = rws.next().stack_value(); - let callee_code_hash = rws.next().account_value_pair().0; + let callee_code_hash = rws.next().account_codehash_pair().0; let callee_exists = !callee_code_hash.is_zero(); let (is_warm, is_warm_prev) = rws.next().tx_access_list_value_pair(); @@ -523,24 +522,12 @@ impl ExecutionGadget for CallOpGadget { // check if it is insufficient balance case. // get caller balance - let caller_balance = rws.next().account_value_pair().0; + let caller_balance = rws.next().account_balance_pair().0; self.caller_balance .assign_u256(region, offset, caller_balance)?; self.is_insufficient_balance .assign(region, offset, caller_balance, value)?; - let is_insufficient = (value > caller_balance) && (is_call || is_callcode); - // only call opcode do transfer in sucessful case. - let [caller_balance_pair, callee_balance_pair] = - if is_call && !is_insufficient && !is_error_depth && !value.is_zero() { - [ - rws.next().account_value_pair(), - rws.next().account_value_pair(), - ] - } else { - [(U256::zero(), U256::zero()), (U256::zero(), U256::zero())] - }; - self.opcode .assign(region, offset, Value::known(F::from(opcode.as_u64())))?; self.is_call.assign( @@ -612,8 +599,19 @@ impl ExecutionGadget for CallOpGadget { callee_rw_counter_end_of_reversion.low_u64() as usize, callee_is_persistent.low_u64() != 0, )?; + + let is_call_or_callcode = is_call || is_callcode; + let is_sufficient = caller_balance >= value; + let is_precheck_ok = is_valid_depth && (is_sufficient || !is_call_or_callcode); + // conditionally assign - if !is_insufficient && !is_error_depth && !value.is_zero() { + if is_call && is_precheck_ok && !value.is_zero() { + if !callee_exists { + rws.next().account_codehash_pair(); // callee hash + } + + let caller_balance_pair = rws.next().account_balance_pair(); + let callee_balance_pair = rws.next().account_balance_pair(); self.transfer.assign( region, offset, @@ -695,7 +693,6 @@ mod test { } } - #[ignore] #[test] fn callop_recursive() { for opcode in TEST_CALL_OPCODES { @@ -703,7 +700,6 @@ mod test { } } - #[ignore] #[test] fn callop_simple() { let stacks = [ diff --git a/zkevm-circuits/src/evm_circuit/execution/create.rs b/zkevm-circuits/src/evm_circuit/execution/create.rs index 393537eb1c..47898f4ac7 100644 --- a/zkevm-circuits/src/evm_circuit/execution/create.rs +++ b/zkevm-circuits/src/evm_circuit/execution/create.rs @@ -497,8 +497,8 @@ impl ExecutionGadget< let rw_offset = if is_create2 { 8 } else { 7 }; // Pre-check: call depth, user's nonce and user's balance - let (caller_balance, _) = block.get_rws(step, rw_offset + 1).account_value_pair(); - let (caller_nonce, _) = block.get_rws(step, rw_offset + 2).account_value_pair(); + let (caller_balance, _) = block.get_rws(step, rw_offset + 1).account_balance_pair(); + let (caller_nonce, _) = block.get_rws(step, rw_offset + 2).account_nonce_pair(); let is_precheck_ok = if call.depth < 1025 && caller_balance >= value && caller_nonce.as_u64() < u64::MAX { 1 @@ -513,7 +513,7 @@ impl ExecutionGadget< .get_rws(step, rw_offset + 4) .tx_access_list_value_pair(); let (callee_prev_code_hash, _) = - block.get_rws(step, rw_offset + 5).account_value_pair(); + block.get_rws(step, rw_offset + 5).account_codehash_pair(); (callee_prev_code_hash, was_warm) } else { (U256::from(0), false) @@ -586,7 +586,7 @@ impl ExecutionGadget< rw_offset + copy_rw_increase + 14, rw_offset + copy_rw_increase + 15, ] - .map(|i| block.get_rws(step, i).account_value_pair()) + .map(|i| block.get_rws(step, i).account_balance_pair()) } else { [(0.into(), 0.into()), (0.into(), 0.into())] }; diff --git a/zkevm-circuits/src/evm_circuit/execution/end_tx.rs b/zkevm-circuits/src/evm_circuit/execution/end_tx.rs index 2575527b98..751c9d4cd3 100644 --- a/zkevm-circuits/src/evm_circuit/execution/end_tx.rs +++ b/zkevm-circuits/src/evm_circuit/execution/end_tx.rs @@ -228,8 +228,8 @@ impl ExecutionGadget for EndTxGadget { ) -> Result<(), Error> { let gas_used = tx.gas() - step.gas_left; let (refund, _) = block.get_rws(step, 2).tx_refund_value_pair(); - let (caller_balance, caller_balance_prev) = block.get_rws(step, 3).account_value_pair(); - let (coinbase_code_hash_prev, _) = block.get_rws(step, 4).account_value_pair(); + let (caller_balance, caller_balance_prev) = block.get_rws(step, 3).account_balance_pair(); + let (coinbase_code_hash_prev, _) = block.get_rws(step, 4).account_codehash_pair(); let (coinbase_balance, coinbase_balance_prev) = block .get_rws( step, @@ -239,7 +239,7 @@ impl ExecutionGadget for EndTxGadget { 5 }, ) - .account_value_pair(); + .account_balance_pair(); self.tx_id .assign(region, offset, Value::known(F::from(tx.id)))?; diff --git a/zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs b/zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs index f31f08c310..ccb38f0a29 100644 --- a/zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs +++ b/zkevm-circuits/src/evm_circuit/execution/error_oog_call.rs @@ -129,7 +129,7 @@ impl ExecutionGadget for ErrorOOGCallGadget { let callee_code_hash = block .get_rws(step, 9 + is_call_or_callcode) - .account_value_pair() + .account_codehash_pair() .0; let callee_exists = !callee_code_hash.is_zero(); diff --git a/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs b/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs index b3e69ccd49..bb22ee2723 100644 --- a/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs +++ b/zkevm-circuits/src/evm_circuit/execution/extcodecopy.rs @@ -203,7 +203,7 @@ impl ExecutionGadget for ExtcodecopyGadget { self.is_warm .assign(region, offset, Value::known(F::from(is_warm as u64)))?; - let code_hash = block.get_rws(step, 8).account_value_pair().0; + let code_hash = block.get_rws(step, 8).account_codehash_pair().0; self.code_hash.assign_u256(region, offset, code_hash)?; self.not_exists.assign_u256(region, offset, code_hash)?; diff --git a/zkevm-circuits/src/evm_circuit/execution/extcodehash.rs b/zkevm-circuits/src/evm_circuit/execution/extcodehash.rs index 825a84e492..ae044ec9d7 100644 --- a/zkevm-circuits/src/evm_circuit/execution/extcodehash.rs +++ b/zkevm-circuits/src/evm_circuit/execution/extcodehash.rs @@ -122,7 +122,7 @@ impl ExecutionGadget for ExtcodehashGadget { self.is_warm .assign(region, offset, Value::known(F::from(is_warm as u64)))?; - let code_hash = block.get_rws(step, 5).account_value_pair().0; + let code_hash = block.get_rws(step, 5).account_codehash_pair().0; self.code_hash.assign_u256(region, offset, code_hash)?; Ok(()) diff --git a/zkevm-circuits/src/evm_circuit/execution/extcodesize.rs b/zkevm-circuits/src/evm_circuit/execution/extcodesize.rs index 7c453c6a61..ee6d540722 100644 --- a/zkevm-circuits/src/evm_circuit/execution/extcodesize.rs +++ b/zkevm-circuits/src/evm_circuit/execution/extcodesize.rs @@ -140,7 +140,7 @@ impl ExecutionGadget for ExtcodesizeGadget { self.is_warm .assign(region, offset, Value::known(F::from(is_warm as u64)))?; - let code_hash = block.get_rws(step, 5).account_value_pair().0; + let code_hash = block.get_rws(step, 5).account_codehash_pair().0; self.code_hash.assign_u256(region, offset, code_hash)?; self.not_exists .assign(region, offset, Word::from(code_hash))?; diff --git a/zkevm-circuits/src/evm_circuit/execution/shl_shr.rs b/zkevm-circuits/src/evm_circuit/execution/shl_shr.rs index a95a633101..60bf8622c2 100644 --- a/zkevm-circuits/src/evm_circuit/execution/shl_shr.rs +++ b/zkevm-circuits/src/evm_circuit/execution/shl_shr.rs @@ -211,7 +211,8 @@ impl ExecutionGadget for ShlShrGadget { self.remainder_is_zero .assign(region, offset, Word::from(remainder))?; self.remainder_lt_divisor - .assign(region, offset, remainder, divisor) + .assign(region, offset, remainder, divisor)?; + Ok(()) } } diff --git a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs index 71c79bf505..3c6850e38c 100644 --- a/zkevm-circuits/src/evm_circuit/util/common_gadget.rs +++ b/zkevm-circuits/src/evm_circuit/util/common_gadget.rs @@ -514,7 +514,7 @@ impl TransferWithGasFeeGadget { or::expr([ not::expr(self.value_is_zero.expr()) * not::expr(self.receiver.receiver_exists.expr()), self.receiver.must_create.clone()] - ) * 1.expr() + + ) + // +1 Write Account (sender) Balance // +1 Write Account (receiver) Balance not::expr(self.value_is_zero.expr()) * 2.expr() diff --git a/zkevm-circuits/src/evm_circuit/util/math_gadget/lt_word.rs b/zkevm-circuits/src/evm_circuit/util/math_gadget/lt_word.rs index 95c4010353..37899253b8 100644 --- a/zkevm-circuits/src/evm_circuit/util/math_gadget/lt_word.rs +++ b/zkevm-circuits/src/evm_circuit/util/math_gadget/lt_word.rs @@ -42,22 +42,22 @@ impl LtWordGadget { offset: usize, lhs: Word, rhs: Word, - ) -> Result<(), Error> { + ) -> Result { let (lhs_lo, lhs_hi) = split_u256(&lhs); let (rhs_lo, rhs_hi) = split_u256(&rhs); - self.comparison_hi.assign( + let (hi_lt, hi_eq) = self.comparison_hi.assign( region, offset, F::from_u128(lhs_hi.as_u128()), F::from_u128(rhs_hi.as_u128()), )?; - self.lt_lo.assign( + let (lt_lo, _) = self.lt_lo.assign( region, offset, F::from_u128(lhs_lo.as_u128()), F::from_u128(rhs_lo.as_u128()), )?; - Ok(()) + Ok(hi_lt + hi_eq * lt_lo) } } diff --git a/zkevm-circuits/src/witness/block.rs b/zkevm-circuits/src/witness/block.rs index 540f801900..ff790740f2 100644 --- a/zkevm-circuits/src/witness/block.rs +++ b/zkevm-circuits/src/witness/block.rs @@ -60,9 +60,11 @@ impl Block { for (tx_idx, tx) in self.txs.iter().enumerate() { println!("tx {}", tx_idx); for step in tx.steps() { - println!(" step {:?} rwc: {}", step.exec_state, step.rwc.0); + println!("> Step {:?}", step.exec_state); for rw_idx in 0..step.bus_mapping_instance.len() { - println!(" - {:?}", self.get_rws(step, rw_idx)); + let rw = self.get_rws(step, rw_idx); + let rw_str = if rw.is_write() { "READ" } else { "WRIT" }; + println!(" {} {} {:?}", rw.rw_counter(), rw_str, rw); } } } diff --git a/zkevm-circuits/src/witness/rw.rs b/zkevm-circuits/src/witness/rw.rs index 859c25e924..6b850648e5 100644 --- a/zkevm-circuits/src/witness/rw.rs +++ b/zkevm-circuits/src/witness/rw.rs @@ -357,11 +357,47 @@ impl Rw { } } - pub(crate) fn account_value_pair(&self) -> (Word, Word) { + pub(crate) fn account_balance_pair(&self) -> (Word, Word) { match self { Self::Account { - value, value_prev, .. - } => (*value, *value_prev), + value, + value_prev, + field_tag, + .. + } => { + debug_assert_eq!(field_tag, &AccountFieldTag::Balance); + (*value, *value_prev) + } + _ => unreachable!(), + } + } + + pub(crate) fn account_nonce_pair(&self) -> (Word, Word) { + match self { + Self::Account { + value, + value_prev, + field_tag, + .. + } => { + debug_assert_eq!(field_tag, &AccountFieldTag::Nonce); + (*value, *value_prev) + } + _ => unreachable!(), + } + } + + pub(crate) fn account_codehash_pair(&self) -> (Word, Word) { + match self { + Self::Account { + value, + value_prev, + field_tag, + .. + } => { + debug_assert_eq!(field_tag, &AccountFieldTag::CodeHash); + (*value, *value_prev) + } _ => unreachable!(), } }