Address review comments

- Rename send_* to call_actor_* in call manager
- Check if upgrade is allowed locally inside kernel upgrade_actor
- Other minor refactorings

fvm/src/call_manager/default.rs

@@ -140,7 +140,7 @@ where
         // different matter.
         //
         // NOTE: Technically, we should be _caching_ the existence of the receiver, so we can skip
-        // this step on `send` and create the target actor immediately. By not doing that, we're not
+        // this step on `call_actor` and create the target actor immediately. By not doing that, we're not
         // being perfectly efficient and are technically under-charging gas. HOWEVER, this behavior
         // cannot be triggered by an actor on-chain, so it's not a concern (for now).
         state_access_tracker.record_lookup_address(&receiver_address);
@@ -203,7 +203,7 @@ where
         }
 
         let mut result = self.with_stack_frame(|s| {
-            s.send_unchecked::<K>(from, to, entrypoint, params, value, read_only)
+            s.call_actor_unchecked::<K>(from, to, entrypoint, params, value, read_only)
         });
 
         // If we pushed a limit, pop it.
@@ -330,7 +330,7 @@ where
         self.nonce
     }
 
-    fn get_actor_call_stack(&self) -> &[(ActorID, &'static str)] {
+    fn get_call_stack(&self) -> &[(ActorID, &'static str)] {
         &self.actor_call_stack
     }
 
@@ -574,7 +574,7 @@ where
             syscall_error!(IllegalArgument; "failed to serialize params: {}", e)
         })?;
 
-        self.send_resolved::<K>(
+        self.call_actor_resolved::<K>(
             system_actor::SYSTEM_ACTOR_ID,
             id,
             Entrypoint::Invoke(fvm_shared::METHOD_CONSTRUCTOR),
@@ -594,9 +594,9 @@ where
         self.create_actor_from_send(addr, state)
     }
 
-    /// Send without checking the call depth and/or dealing with transactions. This must _only_ be
-    /// called from `send`.
-    fn send_unchecked<K>(
+    /// Call actor without checking the call depth and/or dealing with transactions. This must _only_ be
+    /// called from `call_actor`.
+    fn call_actor_unchecked<K>(
         &mut self,
         from: ActorID,
         to: Address,
@@ -635,14 +635,14 @@ where
         };
 
         self.actor_call_stack.push((to, entrypoint.func_name()));
-        let res = self.send_resolved::<K>(from, to, entrypoint, params, value, read_only);
+        let res = self.call_actor_resolved::<K>(from, to, entrypoint, params, value, read_only);
         self.actor_call_stack.pop();
 
         res
     }
 
-    /// Send with resolved addresses.
-    fn send_resolved<K>(
+    /// Call actor with resolved addresses.
+    fn call_actor_resolved<K>(
         &mut self,
         from: ActorID,
         to: ActorID,
@@ -787,19 +787,28 @@ where
             let last_error = invocation_data.last_error;
             let (mut cm, block_registry) = invocation_data.kernel.into_inner();
 
+            // Resolve the return block's ID into an actual block, converting to an abort if it
+            // doesn't exist.
+            let result = result.and_then(|ret_id| {
+                Ok(if ret_id == NO_DATA_BLOCK_ID {
+                    None
+                } else {
+                    Some(block_registry.get(ret_id).map_err(|_| {
+                        Abort::Exit(
+                            ExitCode::SYS_MISSING_RETURN,
+                            String::from("returned block does not exist"),
+                            NO_DATA_BLOCK_ID,
+                        )
+                    })?)
+                })
+            });
+
             // Process the result, updating the backtrace if necessary.
             let mut ret = match result {
-                Ok(NO_DATA_BLOCK_ID) => Ok(InvocationResult {
+                Ok(ret) => Ok(InvocationResult {
                     exit_code: ExitCode::OK,
-                    value: None,
+                    value: ret.cloned(),
                 }),
-                Ok(block_id) => match block_registry.get(block_id) {
-                    Ok(blk) => Ok(InvocationResult {
-                        exit_code: ExitCode::OK,
-                        value: Some(blk.clone()),
-                    }),
-                    Err(e) => Err(ExecutionError::Fatal(anyhow!(e))),
-                },
                 Err(abort) => {
                     let (code, message, res) = match abort {
                         Abort::Exit(code, message, NO_DATA_BLOCK_ID) => (
@@ -811,6 +820,11 @@ where
                             }),
                         ),
                         Abort::Exit(code, message, blk_id) => match block_registry.get(blk_id) {
+                            Err(e) => (
+                                ExitCode::SYS_MISSING_RETURN,
+                                "error getting exit data block".to_owned(),
+                                Err(ExecutionError::Fatal(anyhow!(e))),
+                            ),
                             Ok(blk) => (
                                 code,
                                 message,
@@ -819,11 +833,6 @@ where
                                     value: Some(blk.clone()),
                                 }),
                             ),
-                            Err(e) => (
-                                ExitCode::SYS_MISSING_RETURN,
-                                "error getting exit data block".to_owned(),
-                                Err(ExecutionError::Fatal(anyhow!(e))),
-                            ),
                         },
                         Abort::OutOfGas => (
                             ExitCode::SYS_OUT_OF_GAS,

fvm/src/call_manager/mod.rs

@@ -35,13 +35,13 @@ pub const NO_DATA_BLOCK_ID: u32 = 0;
 ///
 /// 1. The [`crate::executor::Executor`] creates a [`CallManager`] for that message, giving itself
 ///    to the [`CallManager`].
-/// 2. The [`crate::executor::Executor`] calls the specified actor/method using
-///    [`CallManager::send()`].
+/// 2. The [`crate::executor::Executor`] calls the specified actor/entrypoint using
+///    [`CallManager::call_actor()`].
 /// 3. The [`CallManager`] then constructs a [`Kernel`] and executes the actual actor code on that
 ///    kernel.
 /// 4. If an actor calls another actor, the [`Kernel`] will:
 ///    1. Detach the [`CallManager`] from itself.
-///    2. Call [`CallManager::send()`] to execute the new message.
+///    2. Call [`CallManager::call_actor()`] to execute the new message.
 ///    3. Re-attach the [`CallManager`].
 ///    4. Return.
 pub trait CallManager: 'static {
@@ -62,7 +62,7 @@ pub trait CallManager: 'static {
         gas_premium: TokenAmount,
     ) -> Self;
 
-    /// Send a message. The type parameter `K` specifies the the _kernel_ on top of which the target
+    /// Calls an actor at the given address and entrypoint. The type parameter `K` specifies the the _kernel_ on top of which the target
     /// actor should execute.
     #[allow(clippy::too_many_arguments)]
     fn call_actor<K: Kernel<CallManager = Self>>(
@@ -76,7 +76,7 @@ pub trait CallManager: 'static {
         read_only: bool,
     ) -> Result<InvocationResult>;
 
-    /// Execute some operation (usually a send) within a transaction.
+    /// Execute some operation (usually a call_actor) within a transaction.
     fn with_transaction(
         &mut self,
         f: impl FnOnce(&mut Self) -> Result<InvocationResult>,
@@ -120,7 +120,7 @@ pub trait CallManager: 'static {
     ) -> Result<()>;
 
     // returns the actor call stack
-    fn get_actor_call_stack(&self) -> &[(ActorID, &'static str)];
+    fn get_call_stack(&self) -> &[(ActorID, &'static str)];
 
     /// Resolve an address into an actor ID, charging gas as appropriate.
     fn resolve_address(&self, address: &Address) -> Result<Option<ActorID>>;
@@ -176,7 +176,7 @@ pub trait CallManager: 'static {
     fn append_event(&mut self, evt: StampedEvent);
 }
 
-/// The result of a method invocation.
+/// The result of calling actor's entrypoint
 #[derive(Clone, Debug)]
 pub struct InvocationResult {
     /// The exit code (0 for success).
@@ -212,6 +212,8 @@ pub enum Entrypoint {
 pub static INVOKE_FUNC_NAME: &str = "invoke";
 pub static UPGRADE_FUNC_NAME: &str = "upgrade";
 
+const METHOD_UPGRADE: MethodNum = 932083;
+
 impl std::fmt::Display for Entrypoint {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
@@ -225,7 +227,7 @@ impl Entrypoint {
     fn method_num(&self) -> MethodNum {
         match self {
             Entrypoint::Invoke(num) => *num,
-            Entrypoint::Upgrade(_) => fvm_shared::METHOD_UPGRADE,
+            Entrypoint::Upgrade(_) => METHOD_UPGRADE,
         }
     }
 

fvm/src/kernel/default.rs

@@ -876,26 +876,6 @@ where
             .create_actor(code_id, actor_id, delegated_address)
     }
 
-    fn can_actor_upgrade(&self) -> bool {
-        let mut iter = self.call_manager.get_actor_call_stack().iter();
-
-        // find the first position of this actor on the call stack
-        let position = iter.position(|&tuple| tuple == (self.actor_id, INVOKE_FUNC_NAME));
-        if position.is_none() {
-            return true;
-        }
-
-        // make sure that no other actor appears on the call stack after 'position' (unless its
-        // a recursive upgrade call which is allowed)
-        for tuple in iter {
-            if tuple.0 != self.actor_id || tuple.1 != UPGRADE_FUNC_NAME {
-                return false;
-            }
-        }
-
-        true
-    }
-
     fn upgrade_actor<K: Kernel>(
         &mut self,
         new_code_cid: Cid,
@@ -907,6 +887,25 @@ where
             );
         }
 
+        // check if this actor is already on the call stack
+        //
+        // We first find the first position of this actor on the call stack, and then make sure that
+        // no other actor appears on the call stack after 'position' (unless its a recursive upgrade
+        // call which is allowed)
+        let mut iter = self.call_manager.get_call_stack().iter();
+        let position = iter.position(|&tuple| tuple == (self.actor_id, INVOKE_FUNC_NAME));
+        if position.is_some() {
+            for tuple in iter {
+                if tuple.0 != self.actor_id || tuple.1 != UPGRADE_FUNC_NAME {
+                    return Err(syscall_error!(
+                        Forbidden,
+                        "calling upgrade on actor already on call stack is forbidden"
+                    )
+                    .into());
+                }
+            }
+        }
+
         // Load parameters.
         let params = if params_id == NO_DATA_BLOCK_ID {
             None

fvm/src/kernel/mod.rs

@@ -215,8 +215,6 @@ pub trait ActorOps {
         params_id: BlockId,
     ) -> Result<SendResult>;
 
-    fn can_actor_upgrade(&self) -> bool;
-
     /// Installs actor code pointed by cid
     #[cfg(feature = "m2-native")]
     fn install_actor(&mut self, code_cid: Cid) -> Result<()>;

fvm/src/syscalls/actor.rs

@@ -121,13 +121,6 @@ pub fn upgrade_actor<K: Kernel>(
         Err(err) => return err.into(),
     };
 
-    if !context.kernel.can_actor_upgrade() {
-        return ControlFlow::Error(syscall_error!(
-            Forbidden;
-            "calling upgrade on actor already on call stack is forbidden"
-        ));
-    };
-
     match context.kernel.upgrade_actor::<K>(cid, params_id) {
         Ok(SendResult {
             block_id,

fvm/tests/dummy.rs

@@ -358,7 +358,7 @@ impl CallManager for DummyCallManager {
         todo!()
     }
 
-    fn get_actor_call_stack(&self) -> &[(ActorID, &'static str)] {
+    fn get_call_stack(&self) -> &[(ActorID, &'static str)] {
         todo!()
     }
 

shared/src/lib.rs

@@ -109,8 +109,6 @@ pub type MethodNum = u64;
 pub const METHOD_SEND: MethodNum = 0;
 /// Base actor constructor method.
 pub const METHOD_CONSTRUCTOR: MethodNum = 1;
-/// Upgrade actor method.
-pub const METHOD_UPGRADE: MethodNum = 932083;
 
 /// The outcome of a `Send`, covering its ExitCode and optional return data
 #[derive(Debug, PartialEq, Eq, Clone)]

testing/conformance/src/vm.rs

@@ -302,10 +302,6 @@ where
     fn upgrade_actor<KK>(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result<SendResult> {
         self.0.upgrade_actor::<Self>(new_code_cid, params_id)
     }
-
-    fn can_actor_upgrade(&self) -> bool {
-        self.0.can_actor_upgrade()
-    }
 }
 
 impl<M, C, K> IpldBlockOps for TestKernel<K>

testing/integration/tests/main.rs

@@ -1152,6 +1152,7 @@ fn upgrade_actor_test() {
     }
 
     {
+        // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code
         let message = Message {
             from: sender[1].1,
             to: receiver,
@@ -1174,6 +1175,7 @@ fn upgrade_actor_test() {
     }
 
     {
+        // test recursive update
         let message = Message {
             from: sender[2].1,
             to: receiver,
@@ -1199,6 +1201,7 @@ fn upgrade_actor_test() {
     }
 
     {
+        // test sending a message to ourself (putting us on the call stack)
         let message = Message {
             from: sender[3].1,
             to: receiver,