From 112949d280f67ff28da256a19568ac7ce6670770 Mon Sep 17 00:00:00 2001 From: Mathieu <60658558+enitrat@users.noreply.github.com> Date: Fri, 23 Aug 2024 18:50:36 +0200 Subject: [PATCH] feat: cancun selfdestruct (#861) * wip * feat: cancun selfdestruct * fmt * fix tests --- crates/evm/src/backend/starknet_backend.cairo | 78 ++++++++----------- crates/evm/src/create_helpers.cairo | 2 +- .../src/instructions/system_operations.cairo | 40 +++++----- crates/evm/src/model.cairo | 58 +++++++------- 4 files changed, 80 insertions(+), 98 deletions(-) diff --git a/crates/evm/src/backend/starknet_backend.cairo b/crates/evm/src/backend/starknet_backend.cairo index c87c6926a..2f9ca11a8 100644 --- a/crates/evm/src/backend/starknet_backend.cairo +++ b/crates/evm/src/backend/starknet_backend.cairo @@ -174,64 +174,46 @@ mod internals { // Case new account if !self.evm_address().is_deployed() { deploy(self.evm_address()).expect('account deployment failed'); + } - let has_code_or_nonce = self.has_code_or_nonce(); - if !has_code_or_nonce { - // Nothing to commit - return; - } - - // If SELFDESTRUCT, leave the account empty after deploying it - including - // burning any leftover balance. - if (self.is_selfdestruct()) { - let kakarot_state = KakarotCore::unsafe_new_contract_state(); - let burn_starknet_address = kakarot_state - .compute_starknet_address(BURN_ADDRESS.try_into().unwrap()); - let burn_address = Address { - starknet: burn_starknet_address, evm: BURN_ADDRESS.try_into().unwrap() - }; - state - .add_transfer( - Transfer { - sender: self.address(), recipient: burn_address, amount: self.balance() - } - ) - .expect('Failed to burn on selfdestruct'); - return; - } - - // Write bytecode and set nonce - let starknet_account = IAccountDispatcher { contract_address: self.starknet_address() }; - starknet_account.write_bytecode(self.bytecode()); - starknet_account.set_nonce(*self.nonce); - - //TODO: storage commits are done in the State commitment - //Storage is handled outside of the account and must be committed after all accounts are - //committed. + // @dev: EIP-6780 - If selfdestruct on an account created, dont commit data + // and burn any leftover balance. + if (self.is_selfdestruct() && self.is_created()) { + let kakarot_state = KakarotCore::unsafe_new_contract_state(); + let burn_starknet_address = kakarot_state + .compute_starknet_address(BURN_ADDRESS.try_into().unwrap()); + let burn_address = Address { + starknet: burn_starknet_address, evm: BURN_ADDRESS.try_into().unwrap() + }; + state + .add_transfer( + Transfer { + sender: self.address(), recipient: burn_address, amount: self.balance() + } + ) + .expect('Failed to burn on selfdestruct'); return; - }; + } - // @dev: EIP-6780 - If selfdestruct on an account created, dont commit data - let is_created_selfdestructed = self.is_selfdestruct() && self.is_created(); - if is_created_selfdestructed { - // If the account was created and selfdestructed in the same transaction, we don't need - // to commit it. + if !self.has_code_or_nonce() { + // Nothing to commit return; } + // Write updated nonce and storage + //TODO: storage commits are done in the State commitment as they're not part of the account + //model in SSJ let starknet_account = IAccountDispatcher { contract_address: self.starknet_address() }; - starknet_account.set_nonce(*self.nonce); - //TODO: storage commits are done in the State commitment + //Storage is handled outside of the account and must be committed after all accounts are //committed. - - // Update bytecode if required (SELFDESTRUCTed contract committed and redeployed) - //TODO: add bytecode_len entrypoint for optimization - let bytecode_len = starknet_account.bytecode().len(); - if bytecode_len != self.bytecode().len() { + if self.is_created() { starknet_account.write_bytecode(self.bytecode()); + //TODO: save valid jumpdests https://github.com/kkrt-labs/kakarot-ssj/issues/839 + //TODO: set code hash https://github.com/kkrt-labs/kakarot-ssj/issues/840 } + return; } /// Iterates through the list of pending transfer and triggers them @@ -267,6 +249,10 @@ mod internals { while let Option::Some(state_key) = storage_keys.pop_front() { let (evm_address, key, value) = self.accounts_storage.changes.get(*state_key).deref(); let mut account = self.get_account(evm_address); + // @dev: EIP-6780 - If selfdestruct on an account created, dont commit data + if account.is_selfdestruct() { + continue; + } IAccountDispatcher { contract_address: account.starknet_address() } .write_storage(key, value); }; diff --git a/crates/evm/src/create_helpers.cairo b/crates/evm/src/create_helpers.cairo index d6c9611bb..7cf4ed6c2 100644 --- a/crates/evm/src/create_helpers.cairo +++ b/crates/evm/src/create_helpers.cairo @@ -110,7 +110,7 @@ impl CreateHelpersImpl of CreateHelpers { // - it's deployed on SN and is an active EVM contract // - it's not deployed on SN and is an active EVM contract in the Kakarot cache if target_account.has_code_or_nonce() { - sender.set_nonce(sender.nonce() + 1); + sender.set_nonce(sender_current_nonce + 1); self.env.state.set_account(sender); return self.stack.push(0); }; diff --git a/crates/evm/src/instructions/system_operations.cairo b/crates/evm/src/instructions/system_operations.cairo index 249e84fdd..352c6dd42 100644 --- a/crates/evm/src/instructions/system_operations.cairo +++ b/crates/evm/src/instructions/system_operations.cairo @@ -353,52 +353,50 @@ impl SystemOperations of SystemOperationsTrait { /// SELFDESTRUCT /// # Specification: https://www.evm.codes/#ff?fork=shanghai fn exec_selfdestruct(ref self: VM) -> Result<(), EVMError> { - let kakarot_state = KakarotCore::unsafe_new_contract_state(); - let address = self.stack.pop_eth_address()?; + let recipient = self.stack.pop_eth_address()?; // GAS let mut gas_cost = gas::SELFDESTRUCT; - if !self.accessed_addresses.contains(address) { + if !self.accessed_addresses.contains(recipient) { gas_cost += gas::COLD_ACCOUNT_ACCESS_COST; }; - if (!self.env.state.is_account_alive(address) - && self.env.state.get_account(address).balance() != 0) { + let mut self_account = self.env.state.get_account(self.message().target.evm); + let self_balance = self_account.balance(); + if (!self.env.state.is_account_alive(recipient) && self_balance != 0) { gas_cost += gas::NEWACCOUNT; } self.charge_gas(gas_cost)?; + // Operation ensure(!self.message().read_only, EVMError::WriteInStaticContext)?; - //TODO Remove this when https://eips.ethereum.org/EIPS/eip-6780 is validated - let recipient_evm_address = if (address == self.message().target.evm) { + // If the account was created in the same transaction and recipient is self, the native + // token is burnt + let recipient_evm_address = if (self_account.is_created + && recipient == self_account.evm_address()) { 0.try_into().unwrap() } else { - address + recipient }; - let recipient_starknet_address = kakarot_state - .compute_starknet_address(recipient_evm_address); - let mut account = self.env.state.get_account(self.message().target.evm); - - let recipient = Address { - evm: recipient_evm_address, starknet: recipient_starknet_address - }; - + let recipient_account = self.env.state.get_account(recipient_evm_address); // Transfer balance self .env .state .add_transfer( Transfer { - sender: account.address(), - recipient, - amount: self.env.state.get_account(account.address().evm).balance + sender: self_account.address(), + recipient: recipient_account.address(), + amount: self_balance } )?; + //@dev: get_account again because add_transfer modified its balance + self_account = self.env.state.get_account(self.message().target.evm); // Register for selfdestruct - account.selfdestruct(); - self.env.state.set_account(account); + self_account.selfdestruct(); + self.env.state.set_account(self_account); self.stop(); Result::Ok(()) } diff --git a/crates/evm/src/model.cairo b/crates/evm/src/model.cairo index 8b0ec36dd..6f61620a6 100644 --- a/crates/evm/src/model.cairo +++ b/crates/evm/src/model.cairo @@ -322,7 +322,7 @@ mod tests { } #[test] - fn test_account_commit_already_deployed() { + fn test_account_commit_already_deployed_should_not_change_code() { setup_contracts_for_testing(); let mut ca_address = deploy_contract_account(evm_address(), [].span()); @@ -339,32 +339,32 @@ mod tests { let account_dispatcher = IAccountDispatcher { contract_address: ca_address.starknet }; let nonce = account_dispatcher.get_nonce(); let code = account_dispatcher.bytecode(); - assert(nonce == 420, 'wrong nonce'); - assert(code == [0x1].span(), 'notdeploying = unmodified code'); + assert(nonce == 420, 'account should be committed'); + assert!(code != [0x1].span(), "code should not be mofidied unless account is created") } - //TODO unskip after selfdestruct rework - // #[test] - // fn test_account_commit_redeploy_selfdestructed_new_nonce() { - // setup_contracts_for_testing(); - // let mut ca_address = deploy_contract_account(evm_address(), [].span()); - - // // When - // // Selfdestructing the deployed CA to reset its code and nonce. - // // Setting the nonce and the code of a CA - // IAccountDispatcher { contract_address: ca_address.starknet }.selfdestruct(); - // let mut account = AccountTrait::fetch(evm_address()).unwrap(); - // account.nonce = 420; - // account.code = [0x1].span(); - // account.commit(); - - // // Then - // let account_dispatcher = IAccountDispatcher { contract_address: ca_address.starknet }; - // let nonce = account_dispatcher.nonce(); - // let code = account_dispatcher.bytecode(); - // assert(nonce == 420, 'nonce should be modified'); - // assert(code == [0x1].span(), 'code should be modified'); - // } + #[test] + fn test_account_commit_created_but_already_deployed() { + setup_contracts_for_testing(); + let mut ca_address = deploy_contract_account(evm_address(), [].span()); + + // When created in this same tx, the account should have a new code. + + let mut state: State = Default::default(); + let mut account = AccountTrait::fetch(evm_address()).unwrap(); + account.set_created(true); + account.nonce = 420; + account.code = [0x1].span(); + state.set_account(account); + starknet_backend::commit(ref state).expect('commitment failed'); + + // Then + let account_dispatcher = IAccountDispatcher { contract_address: ca_address.starknet }; + let nonce = account_dispatcher.get_nonce(); + let code = account_dispatcher.bytecode(); + assert(nonce == 420, 'nonce should be modified'); + assert(code == [0x1].span(), 'code should be modified'); + } #[test] fn test_account_commit_undeployed() { @@ -377,10 +377,8 @@ mod tests { let mut account = Account { address: Address { evm, starknet }, nonce: 420, code: [ 0x69 - ].span(), balance: 0, selfdestruct: false, is_created: false, + ].span(), balance: 0, selfdestruct: false, is_created: true, }; - account.nonce = 420; - account.code = [0x1].span(); state.set_account(account); starknet_backend::commit(ref state).expect('commitment failed'); @@ -388,8 +386,8 @@ mod tests { let account_dispatcher = IAccountDispatcher { contract_address: starknet }; let nonce = account_dispatcher.get_nonce(); let code = account_dispatcher.bytecode(); - assert(nonce == 420, 'nonce should be modified'); - assert(code == [0x1].span(), 'code should be modified'); + assert(nonce == 420, 'nonce should be committed'); + assert(code == [0x69].span(), 'code should be committed'); } #[test]