From 628be6e9f3225c330127a04bd47116d7e6b4ec2b Mon Sep 17 00:00:00 2001 From: enitrat Date: Wed, 18 Sep 2024 15:37:34 +0200 Subject: [PATCH 1/3] fix: origin nonce used in deploy tx --- crates/evm/src/interpreter.cairo | 147 ++++++++++++++++++++++++++----- 1 file changed, 126 insertions(+), 21 deletions(-) diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index 5775b5fcd..c2775fdc4 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -1,3 +1,4 @@ +use contracts::account_contract::{IAccountDispatcher, IAccountDispatcherTrait}; use contracts::kakarot_core::KakarotCore; use contracts::kakarot_core::interface::IKakarotCore; use core::num::traits::Zero; @@ -16,7 +17,7 @@ use evm::instructions::{ MemoryOperationTrait, Sha3Trait }; -use evm::model::account::{AccountTrait}; +use evm::model::account::{Account, AccountTrait}; use evm::model::vm::{VM, VMTrait}; use evm::model::{ Message, Environment, Transfer, ExecutionSummary, ExecutionSummaryTrait, ExecutionResult, @@ -35,6 +36,31 @@ use utils::set::{Set, SetTrait}; #[generate_trait] pub impl EVMImpl of EVMTrait { + fn prepare_message( + self: @KakarotCore::ContractState, + tx: @Transaction, + sender_account: @Account, + ref env: Environment + ) -> (Address, bool, Span, Address, Span) { + match tx.kind() { + TxKind::Create => { + let origin_nonce: u64 = sender_account.nonce(); + let to_evm_address = compute_contract_address( + sender_account.address().evm, origin_nonce + ); + let to_starknet_address = self.compute_starknet_address(to_evm_address); + let to = Address { evm: to_evm_address, starknet: to_starknet_address }; + (to, true, tx.input(), Zero::zero(), [].span()) + }, + TxKind::Call(to) => { + let target_starknet_address = self.compute_starknet_address(to); + let to = Address { evm: to, starknet: target_starknet_address }; + let code = env.state.get_account(to.evm).code; + (to, false, code, to, tx.input()) + } + } + } + fn process_transaction( ref self: KakarotCore::ContractState, origin: Address, tx: Transaction, intrinsic_gas: u64 ) -> TransactionResult { @@ -45,29 +71,14 @@ pub impl EVMImpl of EVMTrait { let mut env = starknet_backend::get_env(origin.evm, gas_price); let mut sender_account = env.state.get_account(origin.evm); + // Handle deploy/non-deploy transaction cases + let (to, is_deploy_tx, code, code_address, calldata) = self + .prepare_message(@tx, @sender_account, ref env); + + // Increment nonce of sender AFTER computing eventual created address sender_account.set_nonce(sender_account.nonce() + 1); env.state.set_account(sender_account); - // Handle deploy/non-deploy transaction cases - let (to, is_deploy_tx, code, code_address, calldata) = match tx.kind() { - TxKind::Create => { - // Deploy tx case. - let mut origin_nonce: u64 = get_tx_info().unbox().nonce.try_into().unwrap(); - let to_evm_address = compute_contract_address(origin.evm, origin_nonce); - let to_starknet_address = self.compute_starknet_address(to_evm_address); - let to = Address { evm: to_evm_address, starknet: to_starknet_address }; - let code = tx.input(); - let calldata = [].span(); - (to, true, code, Zero::zero(), calldata) - }, - TxKind::Call(to) => { - let target_starknet_address = self.compute_starknet_address(to); - let to = Address { evm: to, starknet: target_starknet_address }; - let code = env.state.get_account(to.evm).code; - (to, false, code, to, tx.input()) - } - }; - let mut accessed_addresses: Set = Default::default(); accessed_addresses.add(env.coinbase); accessed_addresses.add(to.evm); @@ -978,3 +989,97 @@ pub impl EVMImpl of EVMTrait { return Result::Err(EVMError::InvalidOpcode(opcode)); } } + +#[cfg(test)] +mod tests { + use contracts::kakarot_core::KakarotCore; + use core::num::traits::Zero; + use evm::model::{Account, Environment}; + use evm::state::StateTrait; + use evm::test_utils::test_dual_address; + use super::EVMTrait; + use utils::constants::EMPTY_KECCAK; + use utils::eth_transaction::common::TxKind; + use utils::eth_transaction::legacy::TxLegacy; + use utils::eth_transaction::transaction::{Transaction, TransactionTrait}; + + fn setup() -> (KakarotCore::ContractState, Account, Environment) { + let state = KakarotCore::contract_state_for_testing(); + let sender_account = Account { + address: test_dual_address(), + nonce: 5, + balance: 1000000000000000000_u256, // 1 ETH + code: array![].span(), + code_hash: EMPTY_KECCAK, + selfdestruct: false, + is_created: true, + }; + let mut env = Environment { + origin: 0x1234567890123456789012345678901234567890.try_into().unwrap(), + gas_price: 20000000000_u128, // 20 Gwei + chain_id: 1_u64, + prevrandao: 0_u256, + block_number: 12345_u64, + block_gas_limit: 30000000_u64, + block_timestamp: 1634567890_u64, + coinbase: 0x0000000000000000000000000000000000000000.try_into().unwrap(), + base_fee: 0_u64, + state: Default::default(), + }; + env.state.set_account(sender_account); + (state, sender_account, env) + } + + #[test] + fn test_prepare_message_create() { + let (mut state, sender_account, mut env) = setup(); + let tx = Transaction::Legacy( + TxLegacy { + chain_id: Option::Some(1), + nonce: 5, + gas_price: 20000000000_u128, // 20 Gwei + gas_limit: 1000000_u64, + to: TxKind::Create, + value: 0_u256, + input: array![0x60, 0x80, 0x60, 0x40, 0x52].span(), // Simple contract bytecode + } + ); + + let (to, is_deploy_tx, code, code_address, calldata) = state + .prepare_message(@tx, @sender_account, ref env); + + assert_eq!(is_deploy_tx, true); + assert_eq!(code, tx.input()); + assert_eq!( + to.evm, 0xf50541960eec6df5caa295adee1a1a95c3c3241c.try_into().unwrap() + ); // compute_contract_address('evm_address', 5); + assert_eq!(code_address, Zero::zero()); + assert_eq!(calldata, [].span()); + } + + #[test] + fn test_prepare_message_call() { + let (mut state, sender_account, mut env) = setup(); + let target_address = sender_account.address; + let tx = Transaction::Legacy( + TxLegacy { + chain_id: Option::Some(1), + nonce: 5, + gas_price: 20000000000_u128, // 20 Gwei + gas_limit: 1000000_u64, + to: TxKind::Call(target_address.evm), + value: 1000000000000000000_u256, // 1 ETH + input: array![0x12, 0x34, 0x56, 0x78].span(), // Some calldata + } + ); + + let (to, is_deploy_tx, code, code_address, calldata) = state + .prepare_message(@tx, @sender_account, ref env); + + assert_eq!(is_deploy_tx, false); + assert_eq!(to.evm, target_address.evm); + assert_eq!(code_address.evm, target_address.evm); + assert_eq!(code, sender_account.code); + assert_eq!(calldata, tx.input()); + } +} From 22a4efda6845ff58e6a461ba61b1166931a96fca Mon Sep 17 00:00:00 2001 From: enitrat Date: Wed, 18 Sep 2024 16:52:16 +0200 Subject: [PATCH 2/3] fix: withdraw max_fee balance prior to tx --- crates/evm/src/backend/starknet_backend.cairo | 2 +- .../environmental_information.cairo | 4 +- .../src/instructions/memory_operations.cairo | 1 + crates/evm/src/interpreter.cairo | 107 +++++++++++------- crates/evm/src/model.cairo | 2 +- crates/evm/src/model/account.cairo | 13 +-- crates/evm/src/state.cairo | 2 +- crates/evm/src/test_utils.cairo | 11 +- 8 files changed, 88 insertions(+), 54 deletions(-) diff --git a/crates/evm/src/backend/starknet_backend.cairo b/crates/evm/src/backend/starknet_backend.cairo index 5298e5c36..105480c5c 100644 --- a/crates/evm/src/backend/starknet_backend.cairo +++ b/crates/evm/src/backend/starknet_backend.cairo @@ -71,7 +71,7 @@ pub fn get_bytecode(evm_address: EthAddress) -> Span { } /// Populate an Environment with Starknet syscalls. -pub fn get_env(origin: EthAddress, gas_price: u128) -> Environment { +pub fn get_env(origin: Address, gas_price: u128) -> Environment { let kakarot_state = KakarotCore::unsafe_new_contract_state(); let kakarot_storage = kakarot_state.snapshot_deref(); let block_info = get_block_info().unbox(); diff --git a/crates/evm/src/instructions/environmental_information.cairo b/crates/evm/src/instructions/environmental_information.cairo index 1f5bc4da6..bc209eec9 100644 --- a/crates/evm/src/instructions/environmental_information.cairo +++ b/crates/evm/src/instructions/environmental_information.cairo @@ -46,7 +46,7 @@ pub impl EnvironmentInformationImpl of EnvironmentInformationTrait { /// # Specification: https://www.evm.codes/#32?fork=shanghai fn exec_origin(ref self: VM) -> Result<(), EVMError> { self.charge_gas(gas::BASE)?; - self.stack.push(self.env.origin.into()) + self.stack.push(self.env.origin.evm.into()) } /// 0x33 - CALLER @@ -448,7 +448,7 @@ mod tests { // Then assert_eq!(vm.stack.len(), 1); - assert_eq!(vm.stack.peek().unwrap(), origin().into()); + assert_eq!(vm.stack.peek().unwrap(), vm.env.origin.evm.into()); } diff --git a/crates/evm/src/instructions/memory_operations.cairo b/crates/evm/src/instructions/memory_operations.cairo index 3597803d0..b340f96b9 100644 --- a/crates/evm/src/instructions/memory_operations.cairo +++ b/crates/evm/src/instructions/memory_operations.cairo @@ -104,6 +104,7 @@ pub impl MemoryOperation of MemoryOperationTrait { fn exec_sstore(ref self: VM) -> Result<(), EVMError> { let key = self.stack.pop()?; let new_value = self.stack.pop()?; + println!("key: {}, new_value: {}", key, new_value); ensure(self.gas_left() > gas::CALL_STIPEND, EVMError::OutOfGas)?; // EIP-1706 let evm_address = self.message().target.evm; diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index c2775fdc4..ba4a0356d 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -40,9 +40,10 @@ pub impl EVMImpl of EVMTrait { self: @KakarotCore::ContractState, tx: @Transaction, sender_account: @Account, - ref env: Environment - ) -> (Address, bool, Span, Address, Span) { - match tx.kind() { + ref env: Environment, + gas_left: u64 + ) -> (Message, bool) { + let (to, is_deploy_tx, code, code_address, calldata) = match tx.kind() { TxKind::Create => { let origin_nonce: u64 = sender_account.nonce(); let to_evm_address = compute_contract_address( @@ -58,31 +59,12 @@ pub impl EVMImpl of EVMTrait { let code = env.state.get_account(to.evm).code; (to, false, code, to, tx.input()) } - } - } - - fn process_transaction( - ref self: KakarotCore::ContractState, origin: Address, tx: Transaction, intrinsic_gas: u64 - ) -> TransactionResult { - // Charge the cost of intrinsic gas - which has been verified to be <= gas_limit. - let block_base_fee = self.snapshot_deref().Kakarot_base_fee.read(); - let gas_price = tx.effective_gas_price(Option::Some(block_base_fee.into())); - let gas_left = tx.gas_limit() - intrinsic_gas; - let mut env = starknet_backend::get_env(origin.evm, gas_price); - - let mut sender_account = env.state.get_account(origin.evm); - // Handle deploy/non-deploy transaction cases - let (to, is_deploy_tx, code, code_address, calldata) = self - .prepare_message(@tx, @sender_account, ref env); - - // Increment nonce of sender AFTER computing eventual created address - sender_account.set_nonce(sender_account.nonce() + 1); - env.state.set_account(sender_account); + }; let mut accessed_addresses: Set = Default::default(); accessed_addresses.add(env.coinbase); accessed_addresses.add(to.evm); - accessed_addresses.add(origin.evm); + accessed_addresses.add(env.origin.evm); accessed_addresses.extend(eth_precompile_addresses().spanset()); let mut accessed_storage_keys: Set<(EthAddress, u256)> = Default::default(); @@ -97,7 +79,7 @@ pub impl EVMImpl of EVMTrait { }; let message = Message { - caller: origin, + caller: env.origin, target: to, gas_limit: gas_left, data: calldata, @@ -110,8 +92,44 @@ pub impl EVMImpl of EVMTrait { accessed_addresses: accessed_addresses.spanset(), accessed_storage_keys: accessed_storage_keys.spanset(), }; + + (message, is_deploy_tx) + } + + fn process_transaction( + ref self: KakarotCore::ContractState, origin: Address, tx: Transaction, intrinsic_gas: u64 + ) -> TransactionResult { + // Charge the cost of intrinsic gas - which has been verified to be <= gas_limit. + let block_base_fee = self.snapshot_deref().Kakarot_base_fee.read(); + let gas_price = tx.effective_gas_price(Option::Some(block_base_fee.into())); + let gas_left = tx.gas_limit() - intrinsic_gas; + let max_fee = tx.gas_limit().into() * gas_price; + let mut env = starknet_backend::get_env(origin, gas_price); + + let (message, is_deploy_tx) = { + let mut sender_account = env.state.get_account(origin.evm); + // Charge the intrinsic gas to the sender so that it's not available for the execution of the transaction + // but don't trigger any actual transfer, as only the actual consumde gas is charged at the end of the transaction + sender_account.set_balance(sender_account.balance() - max_fee.into()); + + let (message, is_deploy_tx) = self + .prepare_message(@tx, @sender_account, ref env, gas_left); + + // Increment nonce of sender AFTER computing eventual created address + sender_account.set_nonce(sender_account.nonce() + 1); + + env.state.set_account(sender_account); + (message, is_deploy_tx) + }; + let mut summary = Self::process_message_call(message, env, is_deploy_tx); + + // Cancel the max_fee that was taken from the sender to prevent double charging + let mut sender_account = summary.state.get_account(origin.evm); + sender_account.set_balance(sender_account.balance() + max_fee.into()); + summary.state.set_account(sender_account); + // Gas refunds let gas_used = tx.gas_limit() - summary.gas_left; let gas_refund = core::cmp::min(gas_used / 5, summary.gas_refund); @@ -388,6 +406,7 @@ pub impl EVMImpl of EVMTrait { } fn execute_opcode(ref self: VM, opcode: u8) -> Result<(), EVMError> { + println!("Address {:?}, opcode {:?}, pc {:?}, gas left in call {:?}", self.message().code_address.evm, opcode, self.pc(), self.gas_left()); // Call the appropriate function based on the opcode. if opcode == 0x00 { // STOP @@ -994,9 +1013,9 @@ pub impl EVMImpl of EVMTrait { mod tests { use contracts::kakarot_core::KakarotCore; use core::num::traits::Zero; - use evm::model::{Account, Environment}; + use evm::model::{Account, Environment, Message}; use evm::state::StateTrait; - use evm::test_utils::test_dual_address; + use evm::test_utils::{dual_origin, test_dual_address}; use super::EVMTrait; use utils::constants::EMPTY_KECCAK; use utils::eth_transaction::common::TxKind; @@ -1015,7 +1034,7 @@ mod tests { is_created: true, }; let mut env = Environment { - origin: 0x1234567890123456789012345678901234567890.try_into().unwrap(), + origin: dual_origin(), gas_price: 20000000000_u128, // 20 Gwei chain_id: 1_u64, prevrandao: 0_u256, @@ -1045,16 +1064,20 @@ mod tests { } ); - let (to, is_deploy_tx, code, code_address, calldata) = state - .prepare_message(@tx, @sender_account, ref env); + let (message, is_deploy_tx) = state + .prepare_message(@tx, @sender_account, ref env, tx.gas_limit()); assert_eq!(is_deploy_tx, true); - assert_eq!(code, tx.input()); + assert_eq!(message.code, tx.input()); assert_eq!( - to.evm, 0xf50541960eec6df5caa295adee1a1a95c3c3241c.try_into().unwrap() + message.target.evm, 0xf50541960eec6df5caa295adee1a1a95c3c3241c.try_into().unwrap() ); // compute_contract_address('evm_address', 5); - assert_eq!(code_address, Zero::zero()); - assert_eq!(calldata, [].span()); + assert_eq!(message.code_address, Zero::zero()); + assert_eq!(message.data, [].span()); + assert_eq!(message.gas_limit, tx.gas_limit()); + assert_eq!(message.depth, 0); + assert_eq!(message.should_transfer_value, true); + assert_eq!(message.value, 0_u256); } #[test] @@ -1073,13 +1096,17 @@ mod tests { } ); - let (to, is_deploy_tx, code, code_address, calldata) = state - .prepare_message(@tx, @sender_account, ref env); + let (message, is_deploy_tx) = state + .prepare_message(@tx, @sender_account, ref env, tx.gas_limit()); assert_eq!(is_deploy_tx, false); - assert_eq!(to.evm, target_address.evm); - assert_eq!(code_address.evm, target_address.evm); - assert_eq!(code, sender_account.code); - assert_eq!(calldata, tx.input()); + assert_eq!(message.target.evm, target_address.evm); + assert_eq!(message.code_address.evm, target_address.evm); + assert_eq!(message.code, sender_account.code); + assert_eq!(message.data, tx.input()); + assert_eq!(message.gas_limit, tx.gas_limit()); + assert_eq!(message.depth, 0); + assert_eq!(message.should_transfer_value, true); + assert_eq!(message.value, 1000000000000000000_u256); } } diff --git a/crates/evm/src/model.cairo b/crates/evm/src/model.cairo index bf70863fe..97a64ec6f 100644 --- a/crates/evm/src/model.cairo +++ b/crates/evm/src/model.cairo @@ -17,7 +17,7 @@ use utils::traits::{EthAddressDefault, ContractAddressDefault, SpanDefault}; #[derive(Destruct, Default)] pub struct Environment { - pub origin: EthAddress, + pub origin: Address, pub gas_price: u128, pub chain_id: u64, pub prevrandao: u256, diff --git a/crates/evm/src/model/account.cairo b/crates/evm/src/model/account.cairo index 6494dc652..87a5b2fae 100644 --- a/crates/evm/src/model/account.cairo +++ b/crates/evm/src/model/account.cairo @@ -162,6 +162,11 @@ pub impl AccountImpl of AccountTrait { *self.balance } + #[inline(always)] + fn set_balance(ref self: Account, value: u256) { + self.balance = value; + } + #[inline(always)] fn address(self: @Account) -> Address { *self.address @@ -268,14 +273,6 @@ pub impl AccountImpl of AccountTrait { } } -#[generate_trait] -pub(crate) impl AccountInternals of AccountInternalTrait { - #[inline(always)] - fn set_balance(ref self: Account, value: u256) { - self.balance = value; - } -} - #[cfg(test)] mod tests { mod test_has_code_or_nonce { diff --git a/crates/evm/src/state.cairo b/crates/evm/src/state.cairo index 5414c2758..ed455d10a 100644 --- a/crates/evm/src/state.cairo +++ b/crates/evm/src/state.cairo @@ -8,7 +8,7 @@ use core::starknet::{EthAddress}; use evm::backend::starknet_backend::fetch_original_storage; use evm::errors::{ensure, EVMError, BALANCE_OVERFLOW}; -use evm::model::account::{AccountTrait, AccountInternalTrait}; +use evm::model::account::{AccountTrait}; use evm::model::{Event, Transfer, Account}; use utils::set::{Set, SetTrait}; diff --git a/crates/evm/src/test_utils.cairo b/crates/evm/src/test_utils.cairo index be3de3fad..2b5ac3aed 100644 --- a/crates/evm/src/test_utils.cairo +++ b/crates/evm/src/test_utils.cairo @@ -131,6 +131,14 @@ pub fn origin() -> EthAddress { 'origin'.try_into().unwrap() } +pub fn dual_origin() -> Address{ + let origin_evm = origin(); + let origin_starknet = utils::helpers::compute_starknet_address( + test_address(), origin_evm, uninitialized_account() + ); + Address { evm: origin_evm, starknet: origin_starknet } +} + pub fn caller() -> EthAddress { 'caller'.try_into().unwrap() } @@ -253,8 +261,9 @@ pub fn preset_message() -> Message { pub fn preset_environment() -> Environment { let block_info = starknet::get_block_info().unbox(); + Environment { - origin: origin(), + origin: dual_origin(), gas_price: gas_price(), chain_id: chain_id(), prevrandao: 0, From a9ed1b5e4f98e869134c35eed7ad65e16203818f Mon Sep 17 00:00:00 2001 From: enitrat Date: Wed, 18 Sep 2024 16:56:04 +0200 Subject: [PATCH 3/3] remove prints --- crates/evm/src/instructions/memory_operations.cairo | 1 - crates/evm/src/interpreter.cairo | 7 +++---- crates/evm/src/test_utils.cairo | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/evm/src/instructions/memory_operations.cairo b/crates/evm/src/instructions/memory_operations.cairo index b340f96b9..3597803d0 100644 --- a/crates/evm/src/instructions/memory_operations.cairo +++ b/crates/evm/src/instructions/memory_operations.cairo @@ -104,7 +104,6 @@ pub impl MemoryOperation of MemoryOperationTrait { fn exec_sstore(ref self: VM) -> Result<(), EVMError> { let key = self.stack.pop()?; let new_value = self.stack.pop()?; - println!("key: {}, new_value: {}", key, new_value); ensure(self.gas_left() > gas::CALL_STIPEND, EVMError::OutOfGas)?; // EIP-1706 let evm_address = self.message().target.evm; diff --git a/crates/evm/src/interpreter.cairo b/crates/evm/src/interpreter.cairo index ba4a0356d..cb7f0bfc6 100644 --- a/crates/evm/src/interpreter.cairo +++ b/crates/evm/src/interpreter.cairo @@ -108,8 +108,9 @@ pub impl EVMImpl of EVMTrait { let (message, is_deploy_tx) = { let mut sender_account = env.state.get_account(origin.evm); - // Charge the intrinsic gas to the sender so that it's not available for the execution of the transaction - // but don't trigger any actual transfer, as only the actual consumde gas is charged at the end of the transaction + // Charge the intrinsic gas to the sender so that it's not available for the execution + // of the transaction but don't trigger any actual transfer, as only the actual consumde + // gas is charged at the end of the transaction sender_account.set_balance(sender_account.balance() - max_fee.into()); let (message, is_deploy_tx) = self @@ -124,7 +125,6 @@ pub impl EVMImpl of EVMTrait { let mut summary = Self::process_message_call(message, env, is_deploy_tx); - // Cancel the max_fee that was taken from the sender to prevent double charging let mut sender_account = summary.state.get_account(origin.evm); sender_account.set_balance(sender_account.balance() + max_fee.into()); @@ -406,7 +406,6 @@ pub impl EVMImpl of EVMTrait { } fn execute_opcode(ref self: VM, opcode: u8) -> Result<(), EVMError> { - println!("Address {:?}, opcode {:?}, pc {:?}, gas left in call {:?}", self.message().code_address.evm, opcode, self.pc(), self.gas_left()); // Call the appropriate function based on the opcode. if opcode == 0x00 { // STOP diff --git a/crates/evm/src/test_utils.cairo b/crates/evm/src/test_utils.cairo index 2b5ac3aed..90f0fc510 100644 --- a/crates/evm/src/test_utils.cairo +++ b/crates/evm/src/test_utils.cairo @@ -131,7 +131,7 @@ pub fn origin() -> EthAddress { 'origin'.try_into().unwrap() } -pub fn dual_origin() -> Address{ +pub fn dual_origin() -> Address { let origin_evm = origin(); let origin_starknet = utils::helpers::compute_starknet_address( test_address(), origin_evm, uninitialized_account()