Fix actor creation and deletion logic

vm/actor/src/builtin/codes.rs

@@ -26,6 +26,32 @@ fn make_builtin(bz: &[u8]) -> Cid {
     Cid::new_v1(Codec::Raw, Identity::digest(bz))
 }
 
+/// Returns true if the code `Cid` belongs to a builtin actor.
+pub fn is_builtin_actor(code: &Cid) -> bool {
+    code == &*SYSTEM_ACTOR_CODE_ID
+        || code == &*INIT_ACTOR_CODE_ID
+        || code == &*CRON_ACTOR_CODE_ID
+        || code == &*ACCOUNT_ACTOR_CODE_ID
+        || code == &*POWER_ACTOR_CODE_ID
+        || code == &*MINER_ACTOR_CODE_ID
+        || code == &*MARKET_ACTOR_CODE_ID
+        || code == &*PAYCH_ACTOR_CODE_ID
+        || code == &*MULTISIG_ACTOR_CODE_ID
+        || code == &*REWARD_ACTOR_CODE_ID
+        || code == &*VERIFREG_ACTOR_CODE_ID
+}
+
+/// Returns true if the code belongs to a singleton actor.
+pub fn is_singleton_actor(code: &Cid) -> bool {
+    code == &*SYSTEM_ACTOR_CODE_ID
+        || code == &*INIT_ACTOR_CODE_ID
+        || code == &*REWARD_ACTOR_CODE_ID
+        || code == &*CRON_ACTOR_CODE_ID
+        || code == &*POWER_ACTOR_CODE_ID
+        || code == &*MARKET_ACTOR_CODE_ID
+        || code == &*VERIFREG_ACTOR_CODE_ID
+}
+
 // Tests whether a code CID represents an actor that can be an external principal: i.e. an account or multisig.
 // We could do something more sophisticated here: https://github.com/filecoin-project/specs-actors/issues/178
 pub fn is_principal(code: &Cid) -> bool {

vm/actor/src/builtin/init/mod.rs

@@ -80,7 +80,7 @@ impl Actor {
         })??;
 
         // Create an empty actor
-        rt.create_actor(&params.code_cid, &id_address)?;
+        rt.create_actor(params.code_cid, &id_address)?;
 
         // Invoke constructor
         rt.send(

vm/actor/src/builtin/miner/mod.rs

@@ -2562,10 +2562,7 @@ impl ActorCode for Actor {
                 Self::change_multi_address(rt, params.deserialize()?)?;
                 Ok(Serialized::default())
             }
-            None => {
-                // Method number does not match available, abort in runtime
-                Err(rt.abort(ExitCode::SysErrInvalidMethod, "Invalid method".to_owned()))
-            }
+            None => Err(actor_error!(SysErrInvalidMethod; "Invalid method")),
         }
     }
 }

vm/actor/tests/common/mod.rs

@@ -550,7 +550,7 @@ impl Runtime<MemoryDB> for MockRuntime {
         F: FnOnce(&mut C, &mut Self) -> R,
     {
         if self.in_transaction {
-            return Err(self.abort(ExitCode::SysErrorIllegalActor, "nested transaction"));
+            return Err(actor_error!(SysErrorIllegalActor; "nested transaction"));
         }
         let mut read_only = self.state()?;
         self.in_transaction = true;
@@ -605,10 +605,6 @@ impl Runtime<MemoryDB> for MockRuntime {
         }
     }
 
-    fn abort<S: AsRef<str>>(&self, exit_code: ExitCode, msg: S) -> ActorError {
-        ActorError::new(exit_code, msg.as_ref().to_owned())
-    }
-
     fn new_actor_address(&mut self) -> Result<Address, ActorError> {
         self.require_in_call();
         let ret = self
@@ -620,7 +616,7 @@ impl Runtime<MemoryDB> for MockRuntime {
         return Ok(ret);
     }
 
-    fn create_actor(&mut self, code_id: &Cid, address: &Address) -> Result<(), ActorError> {
+    fn create_actor(&mut self, code_id: Cid, address: &Address) -> Result<(), ActorError> {
         self.require_in_call();
         if self.in_transaction {
             return Err(actor_error!(SysErrorIllegalActor; "side-effect within transaction"));
@@ -630,7 +626,7 @@ impl Runtime<MemoryDB> for MockRuntime {
             .take()
             .expect("unexpected call to create actor");
 
-        assert!(&expect_create_actor.code_id == code_id && &expect_create_actor.address == address, "unexpected actor being created, expected code: {:?} address: {:?}, actual code: {:?} address: {:?}", expect_create_actor.code_id, expect_create_actor.address, code_id, address);
+        assert!(&expect_create_actor.code_id == &code_id && &expect_create_actor.address == address, "unexpected actor being created, expected code: {:?} address: {:?}, actual code: {:?} address: {:?}", expect_create_actor.code_id, expect_create_actor.address, code_id, address);
         Ok(())
     }
 

vm/interpreter/src/default_runtime.rs

@@ -482,53 +482,54 @@ where
 
         Ok(ret)
     }
-
-    fn abort<S: AsRef<str>>(&self, exit_code: ExitCode, msg: S) -> ActorError {
-        ActorError::new(exit_code, msg.as_ref().to_owned())
-    }
     fn new_actor_address(&mut self) -> Result<Address, ActorError> {
         let oa = resolve_to_key_addr(self.state, &self.store, &self.origin)?;
         let mut b = to_vec(&oa).map_err(|e| {
-            self.abort(
-                ExitCode::ErrSerialization,
-                format!("Could not serialize address in new_actor_address: {}", e),
-            )
-        })?;
-        b.write_u64::<BigEndian>(self.origin_nonce).map_err(|e| {
-            self.abort(
-                ExitCode::ErrSerialization,
-                format!("Writing nonce address into a buffer: {}", e.to_string()),
-            )
+            actor_error!(fatal(
+                "Could not serialize address in new_actor_address: {}",
+                e
+            ))
         })?;
+        b.write_u64::<BigEndian>(self.origin_nonce)
+            .map_err(|e| actor_error!(fatal("Writing nonce address into a buffer: {}", e)))?;
         b.write_u64::<BigEndian>(self.num_actors_created)
             .map_err(|e| {
-                self.abort(
-                    ExitCode::ErrSerialization,
-                    format!(
-                        "Writing number of actors created into a buffer: {}",
-                        e.to_string()
-                    ),
-                )
+                actor_error!(fatal(
+                    "Writing number of actors created into a buffer: {}",
+                    e
+                ))
             })?;
         let addr = Address::new_actor(&b);
         self.num_actors_created += 1;
         Ok(addr)
     }
-    fn create_actor(&mut self, code_id: &Cid, address: &Address) -> Result<(), ActorError> {
+    fn create_actor(&mut self, code_id: Cid, address: &Address) -> Result<(), ActorError> {
+        if !is_builtin_actor(&code_id) {
+            return Err(actor_error!(SysErrorIllegalArgument; "Can only create built-in actors."));
+        }
+        if is_singleton_actor(&code_id) {
+            return Err(actor_error!(SysErrorIllegalArgument;
+                    "Can only have one instance of singleton actors."));
+        }
+
+        if let Ok(Some(_)) = self.state.get_actor(address) {
+            return Err(actor_error!(SysErrorIllegalArgument; "Actor address already exists"));
+        }
+
         self.charge_gas(self.price_list.on_create_actor())?;
         self.state
             .set_actor(
                 &address,
-                ActorState::new(code_id.clone(), Cid::default(), 0u64.into(), 0),
+                ActorState::new(code_id, EMPTY_ARR_CID.clone(), 0.into(), 0),
             )
-            .map_err(|e| {
-                self.abort(
-                    ExitCode::SysErrInternal,
-                    format!("creating actor entry: {}", e),
-                )
-            })
+            .map_err(|e| actor_error!(fatal("creating actor entry: {}", e)))
     }
-    fn delete_actor(&mut self, _beneficiary: &Address) -> Result<(), ActorError> {
+
+    /// DeleteActor 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 delete_actor(&mut self, beneficiary: &Address) -> Result<(), ActorError> {
         self.charge_gas(self.price_list.on_delete_actor())?;
         let balance = self
             .state
@@ -538,18 +539,21 @@ where
                 || actor_error!(SysErrorIllegalActor; "failed to load actor in delete actor"),
             )
             .map(|act| act.balance)?;
-        if !balance.eq(&0u64.into()) {
-            return Err(self.abort(
-                ExitCode::SysErrInternal,
-                "cannot delete actor with non-zero balance",
-            ));
+        let receiver = *self.message().receiver();
+        if balance != 0.into() {
+            // Transfer the executing actor's balance to the beneficiary
+            transfer(self.state, &receiver, beneficiary, &balance).map_err(|e| {
+                actor_error!(fatal(
+                    "failed to transfer balance to beneficiary actor: {}",
+                    e.msg()
+                ))
+            })?;
         }
-        self.state.delete_actor(self.message.to()).map_err(|e| {
-            self.abort(
-                ExitCode::SysErrInternal,
-                format!("failed to delete actor: {}", e),
-            )
-        })
+
+        // Delete the executing actor
+        self.state
+            .delete_actor(&receiver)
+            .map_err(|e| actor_error!(fatal("failed to delete actor: {}", e)))
     }
     fn syscalls(&self) -> &dyn Syscalls {
         &self.syscalls

vm/runtime/src/lib.rs

@@ -117,12 +117,6 @@ pub trait Runtime<BS: BlockStore> {
         value: TokenAmount,
     ) -> Result<Serialized, ActorError>;
 
-    /// Halts execution upon an error from which the receiver cannot recover.
-    /// The caller will receive the exitcode and an empty return value.
-    /// State changes made within this call will be rolled back. This method does not return.
-    /// The message and args are for diagnostic purposes and do not persist on chain.
-    fn abort<S: AsRef<str>>(&self, exit_code: ExitCode, msg: S) -> ActorError;
-
     /// Computes an address for a new actor. The returned address is intended to uniquely refer to
     /// the actor even in the event of a chain re-org (whereas an ID-address might refer to a
     /// different actor after messages are re-ordered).
@@ -131,7 +125,7 @@ pub trait Runtime<BS: BlockStore> {
 
     /// Creates an actor with code `codeID` and address `address`, with empty state.
     /// May only be called by Init actor.
-    fn create_actor(&mut self, code_id: &Cid, address: &Address) -> Result<(), ActorError>;
+    fn create_actor(&mut self, code_id: Cid, address: &Address) -> Result<(), ActorError>;
 
     /// Deletes the executing actor from the state tree, transferring any balance to beneficiary.
     /// Aborts if the beneficiary does not exist.

vm/src/lib.rs

@@ -5,7 +5,6 @@
 extern crate serde;
 
 mod actor_state;
-mod code;
 mod deal_id;
 mod error;
 mod exit_code;
@@ -15,7 +14,6 @@ mod randomness;
 mod token;
 
 pub use self::actor_state::*;
-pub use self::code::*;
 pub use self::deal_id::*;
 pub use self::error::*;
 pub use self::exit_code::*;