harden some error cases around actor deletion

These cases should be unreachable in our current actors, but would not
have been unreachable in user programmable actors.

Fixes #185 by _not_ returning fatal errors if the current actor doesn't
exist in state.

After deletion:

1. Actors have "zero" balance.
2. Getting & setting the root fails with "illegal actor".
3. Sending still works, but sending with a value will fail because the
   balance is zero.
4. Sending _to_ this actor will fail.

Effectively, I'm treating "deletion" as "unlinking" the actor in the
state-tree. It still _exists_ until it returns, it just can't be
looked-up or linked back into the state.

fvm/src/executor/default.rs

@@ -249,12 +249,11 @@ where
         }
 
         // Deduct message inclusion gas cost and increment sequence.
-        self.state_tree_mut()
-            .mutate_actor(&Address::new_id(sender_id), |act| {
-                act.deduct_funds(&gas_cost)?;
-                act.sequence += 1;
-                Ok(())
-            })?;
+        self.state_tree_mut().mutate_actor_id(sender_id, |act| {
+            act.deduct_funds(&gas_cost)?;
+            act.sequence += 1;
+            Ok(())
+        })?;
 
         Ok(Ok((sender_id, gas_cost, inclusion_cost)))
     }

fvm/src/kernel/default.rs

@@ -1,7 +1,7 @@
 use std::collections::BTreeMap;
 use std::convert::{TryFrom, TryInto};
 
-use anyhow::anyhow;
+use anyhow::{anyhow, Context as _};
 use byteorder::{BigEndian, WriteBytesExt};
 use cid::Cid;
 use filecoin_proofs_api::seal::{
@@ -26,6 +26,7 @@ use rayon::iter::{IndexedParallelIterator, IntoParallelRefIterator, ParallelIter
 use super::blocks::{Block, BlockRegistry};
 use super::error::Result;
 use super::*;
+use crate::account_actor::is_account_actor;
 use crate::builtin::{is_builtin_actor, is_singleton_actor, EMPTY_ARR_CID};
 use crate::call_manager::{CallManager, InvocationResult};
 use crate::externs::{Consensus, Rand};
@@ -110,14 +111,21 @@ where
         let act = state_tree
             .get_actor(addr)?
             .ok_or(anyhow!("state tree doesn't contain actor"))
-            .or_error(ExitCode::ErrIllegalArgument)?;
+            .or_illegal_argument()?;
+
+        if !is_account_actor(&act.code) {
+            return Err(
+                syscall_error!(SysErrIllegalArgument; "target actor is not an account").into(),
+            );
+        }
 
         let state: crate::account_actor::State = state_tree
             .store()
             .get_cbor(&act.state)
-            .or_fatal()?
+            .context("failed to decode actor state as an account")
+            .or_fatal()? // because we've checked and this should be an account.
             .ok_or(anyhow!("account actor state not found"))
-            .or_fatal()?;
+            .or_fatal()?; // because the state should exist.
 
         Ok(state.address)
     }
@@ -157,51 +165,66 @@ where
         let (market_state, _) = MarketActorState::load(self.call_manager.state_tree())?;
         Ok(market_state.total_locked())
     }
+
+    /// Returns `Some(actor_state)` or `None` if this actor has been deleted.
+    fn get_self(&self) -> Result<Option<ActorState>> {
+        self.call_manager
+            .state_tree()
+            .get_actor_id(self.actor_id)
+            .or_fatal()
+            .context("error when finding current actor")
+    }
+
+    /// Mutates this actor's state, returning a syscall error if this actor has been deleted.
+    fn mutate_self<F>(&mut self, mutate: F) -> Result<()>
+    where
+        F: FnOnce(&mut ActorState) -> Result<()>,
+    {
+        self.call_manager
+            .state_tree_mut()
+            .maybe_mutate_actor_id(self.actor_id, mutate)
+            .context("failed to mutate self")
+            .and_then(|found| {
+                if found {
+                    Ok(())
+                } else {
+                    Err(syscall_error!(SysErrIllegalActor; "actor deleted").into())
+                }
+            })
+    }
 }
 
 impl<C> SelfOps for DefaultKernel<C>
 where
     C: CallManager,
 {
-    fn root(&self) -> Cid {
-        let addr = Address::new_id(self.actor_id);
-        let state_tree = self.call_manager.state_tree();
-
-        state_tree
-            .get_actor(&addr)
-            .unwrap()
-            .expect("expected actor to exist")
-            .state
+    fn root(&self) -> Result<Cid> {
+        // This can fail during normal operations if the actor has been deleted.
+        Ok(self
+            .get_self()?
+            .context("state root requested after actor deletion")
+            .or_error(ExitCode::SysErrIllegalActor)?
+            .state)
     }
 
     fn set_root(&mut self, new: Cid) -> Result<()> {
-        let addr = Address::new_id(self.actor_id);
-        let state_tree = self.call_manager.state_tree_mut();
-
-        state_tree.mutate_actor(&addr, |actor_state| {
+        self.mutate_self(|actor_state| {
             actor_state.state = new;
             Ok(())
-        })?;
-
-        Ok(())
+        })
     }
 
     fn current_balance(&self) -> Result<TokenAmount> {
-        let addr = Address::new_id(self.actor_id);
-        let balance = self
-            .call_manager
-            .state_tree()
-            .get_actor(&addr)
-            .or_fatal()
-            .context("error when finding current actor")?
-            .ok_or(anyhow!("state tree doesn't contain current actor"))
-            .or_fatal()?
-            .balance;
-        Ok(balance)
+        // If the actor doesn't exist, it has zero balance.
+        Ok(self
+            .get_self()?
+            .map(|a| a.balance)
+            .unwrap_or_else(|| TokenAmount::zero()))
     }
 
     fn self_destruct(&mut self, beneficiary: &Address) -> Result<()> {
-        // TODO abort with internal error instead of returning.
+        // Idempotentcy: 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.
         self.call_manager
             .charge_gas(self.call_manager.price_list().on_delete_actor())?;
 
@@ -211,9 +234,10 @@ where
             // 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)?.ok_or_else(||
-                // TODO this should not be an actor error, but a system error with an exit code.
-                syscall_error!(SysErrIllegalArgument, "beneficiary doesn't exist"))?;
+            let beneficiary_id = self
+                .resolve_address(beneficiary)?
+                .context("beneficiary doesn't exist")
+                .or_error(ExitCode::SysErrIllegalArgument)?;
 
             if beneficiary_id == self.actor_id {
                 return Err(syscall_error!(
@@ -230,7 +254,6 @@ where
         }
 
         // Delete the executing actor
-        // TODO errors here are FATAL errors
         self.call_manager
             .state_tree_mut()
             .delete_actor_id(self.actor_id)
@@ -352,6 +375,7 @@ where
         value: &TokenAmount,
     ) -> Result<InvocationResult> {
         let from = self.actor_id;
+        // TODO: make sure we still exist here?
         self.call_manager
             .with_transaction(|cm| cm.send::<Self>(from, *recipient, method, params, value))
     }
@@ -876,6 +900,9 @@ fn verify_seal(vi: &SealVerifyInfo) -> Result<bool> {
         bytes_32(&vi.interactive_randomness.0),
         &vi.proof,
     )
-    .or_fatal()
-    .context("failed to verify seal proof") // TODO: Verify that this is actually a fatal error.
+    .or_illegal_argument()
+    // TODO: There are probably errors here that should be fatal, but it's hard to tell so I'm
+    // sticking with illegal argument for now.
+    // Worst case, _some_ node falls out of sync. Better than the network halting.
+    .context("failed to verify seal proof")
 }

fvm/src/kernel/mod.rs

@@ -121,7 +121,7 @@ pub trait BlockOps {
 /// Depends on BlockOps to read and write blocks in the state tree.
 pub trait SelfOps: BlockOps {
     /// Get the state root.
-    fn root(&self) -> Cid;
+    fn root(&self) -> Result<Cid>;
 
     /// Update the state-root.
     ///

fvm/src/machine/default.rs

@@ -4,6 +4,7 @@ use fvm_shared::address::Address;
 use fvm_shared::blockstore::{Blockstore, Buffered};
 use fvm_shared::clock::ChainEpoch;
 use fvm_shared::econ::TokenAmount;
+use fvm_shared::error::ExitCode;
 use fvm_shared::version::NetworkVersion;
 use fvm_shared::ActorID;
 use log::Level::Trace;
@@ -244,18 +245,17 @@ where
             .into());
         }
 
-        // TODO: make sure these are actually fatal.
         let mut from_actor = self
             .state_tree
             .get_actor_id(from)?
-            .ok_or_else(|| anyhow!("sender actor does not exist in state during transfer"))
-            .or_fatal()?;
+            .context("cannot transfer from non-existent sender")
+            .ok_error(ExitCode::SysErrSenderInvalid)?;
 
         let mut to_actor = self
             .state_tree
             .get_actor_id(to)?
-            .ok_or_else(|| anyhow!("receiver actor does not exist in state during transfer"))
-            .or_fatal()?;
+            .context("cannot transfer to non-existent receiver")
+            .ok_error(ExitCode::SysErrInvalidReceiver)?;
 
         from_actor.deduct_funds(value).map_err(|e| {
             syscall_error!(SysErrInsufficientFunds;
@@ -264,12 +264,8 @@ where
         })?;
         to_actor.deposit_funds(value);
 
-        // TODO turn failures into fatal errors
         self.state_tree.set_actor_id(from, from_actor)?;
-        // .map_err(|e| e.downcast_fatal("failed to set from actor"))?;
-        // TODO turn failures into fatal errors
         self.state_tree.set_actor_id(to, to_actor)?;
-        //.map_err(|e| e.downcast_fatal("failed to set to actor"))?;
 
         log::trace!("transfered {} from {} to {}", value, from, to);
 

fvm/src/state_tree.rs

@@ -362,29 +362,60 @@ where
         self.delete_actor_id(id)
     }
 
-    /// Delete actor identified by the supplied ID.
+    /// Delete actor identified by the supplied ID. Returns no error if the actor doesn't exist.
     pub fn delete_actor_id(&mut self, id: ActorID) -> Result<()> {
         // Remove value from cache
         self.snaps.delete_actor(id)?;
 
         Ok(())
     }
 
-    /// Mutate and set actor state for an Address.
+    /// Mutate and set actor state for an Address. Returns false if the actor did not exist. Returns
+    /// a fatal error if the actor doesn't exist.
     pub fn mutate_actor<F>(&mut self, addr: &Address, mutate: F) -> Result<()>
+    where
+        F: FnOnce(&mut ActorState) -> Result<()>,
+    {
+        let id = match self.lookup_id(addr)? {
+            Some(id) => id,
+            None => return Err(anyhow!("failed to lookup actor {}", addr)).or_fatal(),
+        };
+
+        self.mutate_actor_id(id, mutate)
+    }
+
+    /// Mutate and set actor state identified by the supplied ID. Returns a fatal error if the actor
+    /// doesn't exist.
+    pub fn mutate_actor_id<F>(&mut self, id: ActorID, mutate: F) -> Result<()>
+    where
+        F: FnOnce(&mut ActorState) -> Result<()>,
+    {
+        self.maybe_mutate_actor_id(id, mutate).and_then(|found| {
+            if found {
+                Ok(())
+            } else {
+                Err(anyhow!("failed to lookup actor {}", id)).or_fatal()
+            }
+        })
+    }
+
+    /// Try to mutate the actor state identified by the supplied ID, returning false if the actor
+    /// doesn't exist.
+    pub fn maybe_mutate_actor_id<F>(&mut self, id: ActorID, mutate: F) -> Result<bool>
     where
         F: FnOnce(&mut ActorState) -> Result<()>,
     {
         // Retrieve actor state from address
-        let mut act: ActorState = self
-            .get_actor(addr)?
-            .with_context(|| format!("Actor for address: {} does not exist", addr))
-            .or_fatal()?;
+        let mut act = match self.get_actor_id(id)? {
+            Some(act) => act,
+            None => return Ok(false),
+        };
 
         // Apply function of actor state
         mutate(&mut act)?;
         // Set the actor
-        self.set_actor(addr, act)
+        self.set_actor_id(id, act)?;
+        Ok(true)
     }
 
     /// Register a new address through the init actor.
@@ -681,11 +712,12 @@ mod tests {
         tree.set_actor(&INIT_ACTOR_ADDR, act_s).unwrap();
 
         // Test mutate function
-        tree.mutate_actor(&INIT_ACTOR_ADDR, |mut actor| {
-            actor.sequence = 2;
-            Ok(())
-        })
-        .unwrap();
+        assert!(tree
+            .mutate_actor(&INIT_ACTOR_ADDR, |mut actor| {
+                actor.sequence = 2;
+                Ok(())
+            })
+            .unwrap());
         let new_init_s = tree.get_actor(&INIT_ACTOR_ADDR).unwrap();
         assert_eq!(
             new_init_s,

fvm/src/syscalls/sself.rs

@@ -10,7 +10,7 @@ use crate::kernel::{ClassifyResult, Kernel, Result};
 /// buffer is smaller, no value will have been written. The caller must retry
 /// with a larger buffer.
 pub fn root(context: Context<'_, impl Kernel>, obuf_off: u32, obuf_len: u32) -> Result<u32> {
-    let root = context.kernel.root();
+    let root = context.kernel.root()?;
     let size = super::encoded_cid_size(&root);
 
     if size <= obuf_len {

shared/src/sector/registered_proof.rs

@@ -24,6 +24,9 @@ pub enum RegisteredSealProof {
     StackedDRG8MiBV1P1,
     StackedDRG32GiBV1P1,
     StackedDRG64GiBV1P1,
+    // TODO: get rid of this option once we no longer need go compat.
+    // We use it to ensure that we can deserialize bad values here because go checks this value
+    // later.
     Invalid(i64),
 }
 

testing/conformance/src/vm.rs

@@ -535,7 +535,7 @@ where
     C: CallManager<Machine = TestMachine<M>>,
     K: Kernel<CallManager = TestCallManager<C>>,
 {
-    fn root(&self) -> Cid {
+    fn root(&self) -> Result<Cid> {
         self.0.root()
     }