feat: remove beneficiary from self destruct (#1838)

fixes #1837

Author: Stebalien

fvm/src/kernel/default.rs

@@ -11,7 +11,6 @@ use filecoin_proofs_api::{self as proofs, ProverId, PublicReplicaInfo, SectorId}
 use fvm_ipld_blockstore::Blockstore;
 use fvm_ipld_encoding::{bytes_32, IPLD_RAW};
 use fvm_shared::address::Payload;
-use fvm_shared::bigint::Zero;
 use fvm_shared::chainid::ChainID;
 use fvm_shared::consensus::ConsensusFault;
 use fvm_shared::crypto::signature;
@@ -36,7 +35,7 @@ use crate::call_manager::{CallManager, InvocationResult, NO_DATA_BLOCK_ID};
 use crate::externs::{Chain, Consensus, Rand};
 use crate::gas::GasTimer;
 use crate::init_actor::INIT_ACTOR_ID;
-use crate::machine::{MachineContext, NetworkConfig};
+use crate::machine::{MachineContext, NetworkConfig, BURNT_FUNDS_ACTOR_ID};
 use crate::state_tree::ActorState;
 use crate::{ipld, syscall_error};
 
@@ -244,35 +243,37 @@ where
         t.record(Ok(self.get_self()?.map(|a| a.balance).unwrap_or_default()))
     }
 
-    fn self_destruct(&mut self, beneficiary: &Address) -> Result<()> {
+    fn self_destruct(&mut self, burn_unspent: bool) -> Result<()> {
         if self.read_only {
             return Err(syscall_error!(ReadOnly; "cannot self-destruct when read-only").into());
         }
 
-        // Idempotentcy: If the actor doesn't exist, this won't actually do anything. The current
+        // Idempotent: If the actor doesn't exist, this won't actually do anything. The current
         // balance will be zero, and `delete_actor_id` will be a no-op.
         let t = self
             .call_manager
             .charge_gas(self.call_manager.price_list().on_delete_actor())?;
 
+        // If there are remaining funds, burn them. We do this instead of letting the user to
+        // specify the beneficiary as:
+        //
+        // 1. This lets the user handle transfer failure cases themselves. The only way _this_ can
+        //    fail is for the caller to run out of gas.
+        // 2. If we ever decide to allow code on method 0, allowing transfers here would be
+        //    unfortunate.
         let balance = self.current_balance()?;
-        if balance != TokenAmount::zero() {
-            // Starting from network version v7, the runtime checks if the beneficiary
-            // exists; if missing, it fails the self destruct.
-            //
-            // In FVM we check unconditionally, since we only support nv13+.
-            let beneficiary_id = self.resolve_address(beneficiary)?;
-
-            if beneficiary_id == self.actor_id {
-                return Err(syscall_error!(Forbidden, "benefactor cannot be beneficiary").into());
+        if !balance.is_zero() {
+            if !burn_unspent {
+                return Err(
+                    syscall_error!(IllegalOperation; "self-destruct with unspent funds").into(),
+                );
             }
-
-            // Transfer the entirety of funds to beneficiary.
             self.call_manager
-                .transfer(self.actor_id, beneficiary_id, &balance)?;
+                .transfer(self.actor_id, BURNT_FUNDS_ACTOR_ID, &balance)
+                .or_fatal()?;
         }
 
-        // Delete the executing actor
+        // Delete the executing actor.
         t.record(self.call_manager.delete_actor(self.actor_id))
     }
 }

fvm/src/kernel/mod.rs

@@ -176,10 +176,8 @@ pub trait SelfOps: IpldBlockOps {
     /// The balance of the receiver.
     fn current_balance(&self) -> Result<TokenAmount>;
 
-    /// Deletes the executing actor from the state tree, transferring any balance to beneficiary.
-    /// Aborts if the beneficiary does not exist.
-    /// May only be called by the actor itself.
-    fn self_destruct(&mut self, beneficiary: &Address) -> Result<()>;
+    /// Deletes the executing actor from the state tree, burning any remaining balance if requested.
+    fn self_destruct(&mut self, burn_unspent: bool) -> Result<()>;
 }
 
 /// Actors operations whose scope of action is actors other than the calling

fvm/src/syscalls/sself.rs

@@ -33,12 +33,7 @@ pub fn current_balance(context: Context<'_, impl Kernel>) -> Result<sys::TokenAm
         .or_fatal()
 }
 
-pub fn self_destruct(
-    context: Context<'_, impl Kernel>,
-    addr_off: u32,
-    addr_len: u32,
-) -> Result<()> {
-    let addr = context.memory.read_address(addr_off, addr_len)?;
-    context.kernel.self_destruct(&addr)?;
+pub fn self_destruct(context: Context<'_, impl Kernel>, burn_unspent: u32) -> Result<()> {
+    context.kernel.self_destruct(burn_unspent > 0)?;
     Ok(())
 }

sdk/src/error.rs

@@ -16,12 +16,10 @@ pub enum StateUpdateError {
 
 #[derive(Copy, Clone, Debug, Error, Eq, PartialEq)]
 pub enum ActorDeleteError {
-    #[error("deletion beneficiary is the current actor")]
-    BeneficiaryIsSelf,
-    #[error("deletion beneficiary does not exist")]
-    BeneficiaryDoesNotExist,
-    #[error("current execution context is read-only")]
+    #[error("cannot self-destruct when read-only")]
     ReadOnly,
+    #[error("actor did not request unspent funds to be burnt")]
+    UnspentFunds,
 }
 
 #[derive(Copy, Clone, Debug, Error, Eq, PartialEq)]

sdk/src/sself.rs

@@ -1,7 +1,6 @@
 // Copyright 2021-2023 Protocol Labs
 // SPDX-License-Identifier: Apache-2.0, MIT
 use cid::Cid;
-use fvm_shared::address::Address;
 use fvm_shared::econ::TokenAmount;
 use fvm_shared::error::ErrorNumber;
 use fvm_shared::MAX_CID_LEN;
@@ -53,18 +52,12 @@ pub fn current_balance() -> TokenAmount {
     }
 }
 
-/// Destroys the calling actor, sending its current balance
-/// to the supplied address, which cannot be itself.
-///
-/// Fails when calling actor has a non zero balance and the beneficiary doesn't
-/// exist or is the actor being deleted.
-pub fn self_destruct(beneficiary: &Address) -> Result<(), ActorDeleteError> {
-    let bytes = beneficiary.to_bytes();
+/// Destroys the calling actor, burning any remaining balance.
+pub fn self_destruct(burn_funds: bool) -> Result<(), ActorDeleteError> {
     unsafe {
-        sys::sself::self_destruct(bytes.as_ptr(), bytes.len() as u32).map_err(|e| match e {
-            ErrorNumber::Forbidden => ActorDeleteError::BeneficiaryIsSelf,
+        sys::sself::self_destruct(burn_funds).map_err(|e| match e {
+            ErrorNumber::IllegalOperation => ActorDeleteError::UnspentFunds,
             ErrorNumber::ReadOnly => ActorDeleteError::ReadOnly,
-            ErrorNumber::NotFound => ActorDeleteError::BeneficiaryDoesNotExist,
             _ => panic!("unexpected error from `self::self_destruct` syscall: {}", e),
         })
     }

sdk/src/sys/sself.rs

@@ -38,6 +38,7 @@ super::fvm_syscalls! {
     /// | Error                | Reason                                         |
     /// |----------------------|------------------------------------------------|
     /// | [`IllegalOperation`] | actor has been deleted                         |
+    /// | [`ReadOnly`]         | the actor is executing in read-only mode       |
     /// | [`NotFound`]         | specified root CID is not in the reachable set |
     pub fn set_root(cid: *const u8) -> Result<()>;
 
@@ -48,23 +49,19 @@ super::fvm_syscalls! {
     /// None.
     pub fn current_balance() -> Result<super::TokenAmount>;
 
-    /// Destroys the calling actor, sending its current balance
-    /// to the supplied address, which cannot be itself.
-    ///
-    /// Fails when calling actor has a non zero balance and the beneficiary doesn't
-    /// exist or is the actor being deleted.
+    /// Destroys the calling actor. If `burn_funds` is true, any unspent balance will be burnt
+    /// (destroyed). Otherwise, if `burnt_funds` is false and there are unspent funds, this syscall
+    /// will fail.
     ///
     /// # Arguments
     ///
-    /// - `addr_off` and `addr_len` specify the location and length of beneficiary's address in wasm
-    ///   memory.
+    /// - `burn_funds` must be true to delete an actor with unspent funds.
     ///
     /// # Errors
     ///
-    /// | Error               | Reason                                                         |
-    /// |---------------------|----------------------------------------------------------------|
-    /// | [`NotFound`]        | beneficiary isn't found                                        |
-    /// | [`Forbidden`]       | beneficiary is not allowed (usually means beneficiary is self) |
-    /// | [`IllegalArgument`] | if the passed address buffer isn't valid, in memory, etc.      |
-    pub fn self_destruct(addr_off: *const u8, addr_len: u32) -> Result<()>;
+    /// | Error                 | Reason                                    |
+    /// |-----------------------|-------------------------------------------|
+    /// | [`IllegalOperation`]  | the actor has unspent funds               |
+    /// | [`ReadOnly`]          | the actor is executing in read-only mode  |
+    pub fn self_destruct(burn_funds: bool) -> Result<()>;
 }

testing/conformance/src/vm.rs

@@ -529,8 +529,8 @@ where
         self.0.current_balance()
     }
 
-    fn self_destruct(&mut self, beneficiary: &Address) -> Result<()> {
-        self.0.self_destruct(beneficiary)
+    fn self_destruct(&mut self, burn_unspent: bool) -> Result<()> {
+        self.0.self_destruct(burn_unspent)
     }
 }
 

testing/test_actors/actors/fil-readonly-actor/src/actor.rs

@@ -159,9 +159,10 @@ fn invoke_method(blk: u32, method: u64) -> u32 {
             assert_eq!(err, ErrorNumber::ReadOnly);
 
             // Should not be able to delete self.
-            let err =
-                sdk::sself::self_destruct(&Address::new_id(sdk::message::origin())).unwrap_err();
-            assert_eq!(err, ActorDeleteError::ReadOnly);
+            assert_eq!(
+                sdk::sself::self_destruct(true).unwrap_err(),
+                ActorDeleteError::ReadOnly
+            );
         }
         4 => {
             assert!(sdk::vm::read_only());

testing/test_actors/actors/fil-sself-actor/src/actor.rs

@@ -4,7 +4,6 @@ use cid::multihash::{Code, MultihashDigest};
 use cid::Cid;
 use fvm_ipld_encoding::{to_vec, DAG_CBOR};
 use fvm_sdk as sdk;
-use fvm_shared::address::Address;
 use fvm_shared::econ::TokenAmount;
 use sdk::error::{ActorDeleteError, StateReadError, StateUpdateError};
 
@@ -31,21 +30,14 @@ pub fn invoke(_: u32) -> u32 {
     let balance = sdk::sself::current_balance();
     assert_eq!(TokenAmount::from_nano(1_000_000), balance);
 
-    // test that we can't destroy the calling actor when supplied beneficiary
-    // address does not exist or when its itself
-    //
-    assert_eq!(
-        sdk::sself::self_destruct(&Address::new_id(191919)),
-        Err(ActorDeleteError::BeneficiaryDoesNotExist),
-    );
+    // Now destroy the actor without burning funds. This should fail because we have unspent funds.
     assert_eq!(
-        sdk::sself::self_destruct(&Address::new_id(10000)),
-        Err(ActorDeleteError::BeneficiaryIsSelf),
+        sdk::sself::self_destruct(false).unwrap_err(),
+        ActorDeleteError::UnspentFunds
     );
 
-    // now lets destroy the calling actor
-    //
-    sdk::sself::self_destruct(&Address::new_id(sdk::message::origin())).unwrap();
+    // Now lets destroy the actor, burning the funds.
+    sdk::sself::self_destruct(true).unwrap();
 
     // test that root/set_root/self_destruct fail when the actor has been deleted
     // and balance is 0
@@ -56,14 +48,8 @@ pub fn invoke(_: u32) -> u32 {
     );
     assert_eq!(TokenAmount::from_nano(0), sdk::sself::current_balance());
 
-    // calling destroy on an already destroyed actor should succeed (since its
-    // balance is 0)
-    //
-    // TODO (fridrik): we should consider changing this behaviour in the future
-    // and disallow destroying actor with non-zero balance)
-    //
-    sdk::sself::self_destruct(&Address::new_id(sdk::message::origin()))
-        .expect("deleting an already deleted actor should succeed since it has zero balance");
+    // calling destroy on an already destroyed actor should succeed (no-op)
+    sdk::sself::self_destruct(false).expect("deleting an already deleted actor should succeed");
 
     #[cfg(coverage)]
     sdk::debug::store_artifact("sself_actor.profraw", minicov::capture_coverage());