EVM: implement SELFDESTRUCT opcode. (#593)

This required a small refactor to allow us to mutably borrow the Runtime.

actors/evm/src/interpreter/execution.rs

@@ -1,5 +1,6 @@
 #![allow(dead_code)]
 
+use fvm_shared::address::Address as FilecoinAddress;
 use {
     super::instructions::*,
     super::opcode::OpCode,
@@ -23,6 +24,8 @@ pub struct ExecutionState {
     pub input_data: Bytes,
     pub return_data: Bytes,
     pub output_data: Bytes,
+    /// Indicates whether the contract called SELFDESTRUCT, providing the beneficiary.
+    pub selfdestroyed: Option<FilecoinAddress>,
 }
 
 impl ExecutionState {
@@ -33,12 +36,13 @@ impl ExecutionState {
             input_data,
             return_data: Default::default(),
             output_data: Bytes::new(),
+            selfdestroyed: None,
         }
     }
 }
 
 struct Machine<'r, BS: Blockstore, RT: Runtime<BS>> {
-    system: &'r System<'r, BS, RT>,
+    system: &'r mut System<'r, BS, RT>,
     runtime: &'r mut ExecutionState,
     bytecode: &'r Bytecode<'r>,
     pc: usize,
@@ -90,7 +94,7 @@ macro_rules! def_ins {
 
 impl<'r, BS: Blockstore + 'r, RT: Runtime<BS> + 'r> Machine<'r, BS, RT> {
     pub fn new(
-        system: &'r System<'r, BS, RT>,
+        system: &'r mut System<'r, BS, RT>,
         runtime: &'r mut ExecutionState,
         bytecode: &'r Bytecode,
     ) -> Self {
@@ -843,7 +847,7 @@ impl<'r, BS: Blockstore + 'r, RT: Runtime<BS> + 'r> Machine<'r, BS, RT> {
 
         SELFDESTRUCT(m) {
             lifecycle::selfdestruct(m.runtime, m.system)?;
-            Ok(ControlFlow::Continue)
+            Ok(ControlFlow::Exit) // selfdestruct halts the current context
         }
     }
 
@@ -853,13 +857,14 @@ impl<'r, BS: Blockstore + 'r, RT: Runtime<BS> + 'r> Machine<'r, BS, RT> {
 pub fn execute<'r, BS: Blockstore, RT: Runtime<BS>>(
     bytecode: &'r Bytecode,
     runtime: &'r mut ExecutionState,
-    system: &'r System<'r, BS, RT>,
+    system: &'r mut System<'r, BS, RT>,
 ) -> Result<Output, StatusCode> {
     let mut m = Machine::new(system, runtime, bytecode);
     m.execute()?;
     Ok(Output {
         reverted: m.reverted,
         status_code: StatusCode::Success,
         output_data: m.runtime.output_data.clone(),
+        selfdestroyed: m.runtime.selfdestroyed,
     })
 }

actors/evm/src/interpreter/instructions/call.rs

@@ -113,7 +113,7 @@ pub fn call<'r, BS: Blockstore, RT: Runtime<BS>>(
     _kind: CallKind,
 ) -> Result<(), StatusCode> {
     let ExecutionState { stack, memory, .. } = state;
-    let rt = platform.rt;
+    let rt = &*platform.rt; // as immutable reference
 
     let _gas = stack.pop(); // EVM gas is not used in FVM
     let dst: H160 = crate::interpreter::uints::_u256_to_address(stack.pop());

actors/evm/src/interpreter/instructions/lifecycle.rs

@@ -1,3 +1,4 @@
+use crate::interpreter::address::Address;
 use {
     crate::interpreter::{ExecutionState, StatusCode, System},
     fil_actors_runtime::runtime::Runtime,
@@ -15,22 +16,11 @@ pub fn create<'r, BS: Blockstore, RT: Runtime<BS>>(
 
 #[inline]
 pub fn selfdestruct<'r, BS: Blockstore, RT: Runtime<BS>>(
-    _state: &mut ExecutionState,
-    _platform: &'r System<'r, BS, RT>,
+    state: &mut ExecutionState,
+    _system: &'r mut System<'r, BS, RT>,
 ) -> Result<(), StatusCode> {
-    // Commented due to horrible borrow checker issues that stem from owning a HAMT during
-    // the entire execution inside the System. The HAMT needs to be flushed and dropped when
-    // create_actor, delete_actor, and send are called. All other methods taking a &mut self
-    // on the Runtime should not require &mut.
-    //
-    // let beneficiary_addr = Address::from(state.stack.pop());
-    //
-    // if let Some(id_addr) = beneficiary_addr.as_id_address() {
-    //     platform.rt.delete_actor(&id_addr)?;
-    // } else {
-    //     todo!("no support for non-ID addresses")
-    // }
-    //
-    // Ok(())
-    todo!()
+    let beneficiary_addr = Address::from(state.stack.pop());
+    let id_addr = beneficiary_addr.as_id_address().expect("no support for non-ID addresses yet");
+    state.selfdestroyed = Some(id_addr);
+    Ok(())
 }

actors/evm/src/interpreter/instructions/storage.rs

@@ -7,7 +7,7 @@ use {
 #[inline]
 pub fn sload<'r, BS: Blockstore, RT: Runtime<BS>>(
     state: &mut ExecutionState,
-    platform: &'r System<'r, BS, RT>,
+    platform: &'r mut System<'r, BS, RT>,
 ) -> Result<(), StatusCode> {
     // where?
     let location = state.stack.pop();
@@ -24,7 +24,7 @@ pub fn sload<'r, BS: Blockstore, RT: Runtime<BS>>(
 #[inline]
 pub fn sstore<'r, BS: Blockstore, RT: Runtime<BS>>(
     state: &mut ExecutionState,
-    platform: &'r System<'r, BS, RT>,
+    platform: &'r mut System<'r, BS, RT>,
 ) -> Result<(), StatusCode> {
     let location = state.stack.pop();
     let value = state.stack.pop();

actors/evm/src/interpreter/output.rs

@@ -1,4 +1,5 @@
 use fil_actors_runtime::ActorError;
+use fvm_shared::address::Address as FilecoinAddress;
 use {
     bytes::Bytes,
     fvm_ipld_encoding::Cbor,
@@ -14,8 +15,10 @@ pub struct Output {
     pub status_code: StatusCode,
     /// Output data returned.
     pub output_data: Bytes,
-    // indicates if revert was requested
+    /// Indicates if revert was requested
     pub reverted: bool,
+    /// Indicates whether the contract called SELFDESTRUCT, providing the beneficiary.
+    pub selfdestroyed: Option<FilecoinAddress>,
 }
 
 impl Debug for Output {

actors/evm/src/interpreter/system.rs

@@ -6,7 +6,6 @@ use {
     fil_actors_runtime::{runtime::Runtime, ActorError},
     fvm_ipld_blockstore::Blockstore,
     fvm_ipld_hamt::Hamt,
-    std::cell::RefCell,
 };
 
 #[derive(Clone, Copy, Debug)]
@@ -26,56 +25,53 @@ pub enum StorageStatus {
 /// Platform Abstraction Layer
 /// that bridges the FVM world to EVM world
 pub struct System<'r, BS: Blockstore, RT: Runtime<BS>> {
-    pub rt: &'r RT,
-    state: RefCell<Hamt<&'r BS, U256, U256>>,
+    pub rt: &'r mut RT,
+    state: &'r mut Hamt<BS, U256, U256>,
 }
 
 impl<'r, BS: Blockstore, RT: Runtime<BS>> System<'r, BS, RT> {
-    pub fn new(rt: &'r RT, state_cid: Cid) -> anyhow::Result<Self> {
-        Ok(Self { rt, state: RefCell::new(Hamt::load(&state_cid, rt.store())?) })
+    pub fn new(rt: &'r mut RT, state: &'r mut Hamt<BS, U256, U256>) -> anyhow::Result<Self> {
+        Ok(Self { rt, state })
     }
 
-    pub fn flush_state(&self) -> Result<Cid, ActorError> {
-        self.state.borrow_mut().flush().map_err(|e| ActorError::illegal_state(e.to_string()))
+    /// Reborrow the system with a shorter lifetime.
+    #[allow(clippy::needless_lifetimes)]
+    pub fn reborrow<'a>(&'a mut self) -> System<'a, BS, RT> {
+        System { rt: &mut *self.rt, state: &mut *self.state }
+    }
+
+    pub fn flush_state(&mut self) -> Result<Cid, ActorError> {
+        self.state.flush().map_err(|e| ActorError::illegal_state(e.to_string()))
     }
 
     /// Get value of a storage key.
-    pub fn get_storage(&self, key: U256) -> Result<Option<U256>, StatusCode> {
+    pub fn get_storage(&mut self, key: U256) -> Result<Option<U256>, StatusCode> {
         let mut key_bytes = [0u8; 32];
         key.to_big_endian(&mut key_bytes);
 
-        Ok(self
-            .state
-            .borrow()
-            .get(&key)
-            .map_err(|e| StatusCode::InternalError(e.to_string()))?
-            .cloned())
+        Ok(self.state.get(&key).map_err(|e| StatusCode::InternalError(e.to_string()))?.cloned())
     }
 
     /// Set value of a storage key.
-    pub fn set_storage(&self, key: U256, value: Option<U256>) -> Result<StorageStatus, StatusCode> {
+    pub fn set_storage(
+        &mut self,
+        key: U256,
+        value: Option<U256>,
+    ) -> Result<StorageStatus, StatusCode> {
         let mut key_bytes = [0u8; 32];
         key.to_big_endian(&mut key_bytes);
 
-        let prev_value = self
-            .state
-            .borrow()
-            .get(&key)
-            .map_err(|e| StatusCode::InternalError(e.to_string()))?
-            .cloned();
+        let prev_value =
+            self.state.get(&key).map_err(|e| StatusCode::InternalError(e.to_string()))?.cloned();
 
         let mut storage_status =
             if prev_value == value { StorageStatus::Unchanged } else { StorageStatus::Modified };
 
         if value.is_none() {
-            self.state
-                .borrow_mut()
-                .delete(&key)
-                .map_err(|e| StatusCode::InternalError(e.to_string()))?;
+            self.state.delete(&key).map_err(|e| StatusCode::InternalError(e.to_string()))?;
             storage_status = StorageStatus::Deleted;
         } else {
             self.state
-                .borrow_mut()
                 .set(key, value.unwrap())
                 .map_err(|e| StatusCode::InternalError(e.to_string()))?;
         }

actors/evm/src/lib.rs

@@ -1,6 +1,7 @@
 pub mod interpreter;
 mod state;
 
+use fvm_shared::address::Address;
 use {
     crate::interpreter::{execute, Bytecode, ExecutionState, StatusCode, System, U256},
     crate::state::State,
@@ -38,7 +39,7 @@ pub struct EvmContractActor;
 impl EvmContractActor {
     pub fn constructor<BS, RT>(rt: &mut RT, params: ConstructorParams) -> Result<(), ActorError>
     where
-        BS: Blockstore,
+        BS: Blockstore + Clone,
         RT: Runtime<BS>,
     {
         rt.validate_immediate_caller_accept_any()?;
@@ -54,22 +55,23 @@ impl EvmContractActor {
             return Err(ActorError::illegal_argument("no bytecode provided".into()));
         }
 
-        // initialize contract state
-        let init_contract_state_cid =
-            Hamt::<_, U256, U256>::new(rt.store()).flush().map_err(|e| {
-                ActorError::unspecified(format!("failed to flush contract state: {e:?}"))
-            })?;
+        // create an empty storage HAMT to pass it down for execution.
+        let mut hamt = Hamt::<_, U256, U256>::new(rt.store().clone());
+
         // create an instance of the platform abstraction layer -- note: do we even need this?
-        let system = System::new(rt, init_contract_state_cid).map_err(|e| {
+        let mut system = System::new(rt, &mut hamt).map_err(|e| {
             ActorError::unspecified(format!("failed to create execution abstraction layer: {e:?}"))
         })?;
+
         // create a new execution context
         let mut exec_state = ExecutionState::new(Bytes::copy_from_slice(&params.input_data));
+
         // identify bytecode valid jump destinations
         let bytecode = Bytecode::new(&params.bytecode)
             .map_err(|e| ActorError::unspecified(format!("failed to parse bytecode: {e:?}")))?;
+
         // invoke the contract constructor
-        let exec_status = execute(&bytecode, &mut exec_state, &system)
+        let exec_status = execute(&bytecode, &mut exec_state, &mut system.reborrow())
             .map_err(|e| ActorError::unspecified(format!("EVM execution error: {e:?}")))?;
 
         if !exec_status.reverted
@@ -103,14 +105,16 @@ impl EvmContractActor {
         params: InvokeParams,
     ) -> Result<RawBytes, ActorError>
     where
-        BS: Blockstore,
+        BS: Blockstore + Clone,
         RT: Runtime<BS>,
     {
         rt.validate_immediate_caller_accept_any()?;
 
+        let mut selfdestroyed = Option::<Address>::default();
+
         // TODO this is fine in a transaction for now, as we don't have yet cross-contact calls
         //      some refactoring will be needed when we start making cross contract calls.
-        rt.transaction(|state: &mut State, rt| {
+        let output = rt.transaction(|state: &mut State, rt| {
             let bytecode: Vec<u8> =
                 match rt.store().get(&state.bytecode).map_err(|e| {
                     ActorError::unspecified(format!("failed to parse bytecode: {e:?}"))
@@ -122,17 +126,29 @@ impl EvmContractActor {
             let bytecode = Bytecode::new(&bytecode)
                 .map_err(|e| ActorError::unspecified(format!("failed to parse bytecode: {e:?}")))?;
 
-            let system = System::new(rt, state.contract_state).map_err(|e| {
+            // clone the blockstore here to pass to the System, this is bound to the HAMT.
+            let blockstore = rt.store().clone();
+
+            // load the storage HAMT
+            let mut hamt = Hamt::load(&state.contract_state, blockstore).map_err(|e| {
+                ActorError::illegal_state(format!(
+                    "failed to load storage HAMT on invoke: {e:?}, e"
+                ))
+            })?;
+
+            let mut system = System::new(rt, &mut hamt).map_err(|e| {
                 ActorError::unspecified(format!(
                     "failed to create execution abstraction layer: {e:?}"
                 ))
             })?;
 
             let mut exec_state = ExecutionState::new(Bytes::copy_from_slice(&params.input_data));
 
-            let exec_status = execute(&bytecode, &mut exec_state, &system)
+            let exec_status = execute(&bytecode, &mut exec_state, &mut system.reborrow())
                 .map_err(|e| ActorError::unspecified(format!("EVM execution error: {e:?}")))?;
 
+            selfdestroyed = exec_status.selfdestroyed;
+
             // XXX is this correct handling of reverts? or should we fail execution?
             if exec_status.status_code == StatusCode::Success {
                 let result = RawBytes::from(exec_status.output_data.to_vec());
@@ -148,7 +164,13 @@ impl EvmContractActor {
                     exec_status.status_code
                 )))
             }
-        })
+        })?;
+
+        if let Some(addr) = selfdestroyed {
+            rt.delete_actor(&addr)?
+        }
+
+        Ok(output)
     }
 }
 

actors/evm/tests/selfdestruct.hex

@@ -0,0 +1 @@
+6080604052348015600f57600080fd5b5060988061001e6000396000f3fe6080604052348015600f57600080fd5b506004361060285760003560e01c806335f4699414602d575b600080fd5b60336035565b005b73ff000000000000000000000000000000000003e973ffffffffffffffffffffffffffffffffffffffff16fffea26469706673582212205539162e3dd1c84b3cf9043b55e3890bbe09c5726899ad6003650e8f1ac698b564736f6c63430008070033

actors/evm/tests/selfdestruct.rs

@@ -0,0 +1,42 @@
+use fil_actor_evm as evm;
+use fil_actors_runtime::test_utils::*;
+use fvm_ipld_encoding::RawBytes;
+use fvm_shared::address::Address;
+
+#[test]
+fn test_selfdestruct() {
+    let mut rt = MockRuntime::default();
+
+    let contract = Address::new_id(100);
+    let beneficiary = Address::new_id(1001);
+
+    let params = evm::ConstructorParams {
+        bytecode: hex::decode(include_str!("selfdestruct.hex")).unwrap().into(),
+        input_data: RawBytes::default(),
+    };
+
+    // invoke constructor
+    rt.actor_code_cids.insert(contract, *EVM_ACTOR_CODE_ID);
+    rt.expect_validate_caller_any();
+    rt.set_origin(contract);
+
+    let result = rt
+        .call::<evm::EvmContractActor>(
+            evm::Method::Constructor as u64,
+            &RawBytes::serialize(params).unwrap(),
+        )
+        .unwrap();
+    expect_empty(result);
+    rt.verify();
+
+    let solidity_params = hex::decode("35f46994").unwrap();
+    let params = evm::InvokeParams { input_data: RawBytes::from(solidity_params) };
+    rt.expect_validate_caller_any();
+    rt.expect_delete_actor(beneficiary);
+
+    rt.call::<evm::EvmContractActor>(
+        evm::Method::InvokeContract as u64,
+        &RawBytes::serialize(params).unwrap(),
+    )
+    .unwrap();
+}

actors/evm/tests/selfdestruct.sol

@@ -0,0 +1,7 @@
+pragma solidity ^0.8.0;
+
+contract Selfdestruct {
+    function die() public {
+        selfdestruct(payable(address(0xFF000000000000000000000000000000000003E9)));
+    }
+}