diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 9febf0514d..de8bdcd0eb 100644 --- a/fvm/src/kernel/default.rs +++ b/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)) } } diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index 2df91946b7..dcf5c547f8 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -176,10 +176,8 @@ pub trait SelfOps: IpldBlockOps { /// The balance of the receiver. fn current_balance(&self) -> Result; - /// 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 diff --git a/fvm/src/syscalls/sself.rs b/fvm/src/syscalls/sself.rs index eab0675085..f724083aae 100644 --- a/fvm/src/syscalls/sself.rs +++ b/fvm/src/syscalls/sself.rs @@ -33,12 +33,7 @@ pub fn current_balance(context: Context<'_, impl Kernel>) -> Result, - 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(()) } diff --git a/sdk/src/error.rs b/sdk/src/error.rs index 47085c8351..efe8a4d6da 100644 --- a/sdk/src/error.rs +++ b/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)] diff --git a/sdk/src/sself.rs b/sdk/src/sself.rs index e8f74c1fe4..0c42945371 100644 --- a/sdk/src/sself.rs +++ b/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), }) } diff --git a/sdk/src/sys/sself.rs b/sdk/src/sys/sself.rs index 34a5fb0379..1a32e0a44b 100644 --- a/sdk/src/sys/sself.rs +++ b/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; - /// 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<()>; } diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index 25c2dbe0ba..4ee681b49d 100644 --- a/testing/conformance/src/vm.rs +++ b/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) } } diff --git a/testing/test_actors/actors/fil-readonly-actor/src/actor.rs b/testing/test_actors/actors/fil-readonly-actor/src/actor.rs index abdc217437..4529688fa1 100644 --- a/testing/test_actors/actors/fil-readonly-actor/src/actor.rs +++ b/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()); diff --git a/testing/test_actors/actors/fil-sself-actor/src/actor.rs b/testing/test_actors/actors/fil-sself-actor/src/actor.rs index a0d0121bd5..83cfc9039b 100644 --- a/testing/test_actors/actors/fil-sself-actor/src/actor.rs +++ b/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());