From ccb9c620b454782f6a8ee019d88da7340c002ab0 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Wed, 30 Aug 2023 17:44:53 +0000 Subject: [PATCH 01/40] wip --- Cargo.lock | 12 ++ fvm/src/call_manager/default.rs | 170 +++++++++++++++++- fvm/src/call_manager/mod.rs | 18 ++ fvm/src/executor/default.rs | 15 +- fvm/src/kernel/default.rs | 18 ++ fvm/src/kernel/error.rs | 17 +- fvm/src/kernel/mod.rs | 2 + fvm/src/syscalls/actor.rs | 12 ++ fvm/src/syscalls/bind.rs | 1 + fvm/src/syscalls/context.rs | 3 + fvm/src/syscalls/error.rs | 5 + fvm/src/syscalls/mod.rs | 1 + fvm/tests/default_kernel/mod.rs | 6 + fvm/tests/dummy.rs | 24 +++ sdk/src/actor.rs | 17 +- sdk/src/sys/actor.rs | 33 ++++ shared/src/lib.rs | 2 + testing/conformance/src/vm.rs | 4 + testing/integration/tests/upgrade_test.rs | 67 +++++++ .../actors/fil-syscall-actor/src/actor.rs | 11 ++ .../actors/fil-upgrade-actor/Cargo.toml | 16 ++ .../actors/fil-upgrade-actor/src/actor.rs | 54 ++++++ .../actors/fil-upgrade-actor/src/lib.rs | 4 + testing/test_actors/build.rs | 1 + 24 files changed, 508 insertions(+), 5 deletions(-) create mode 100644 testing/integration/tests/upgrade_test.rs create mode 100644 testing/test_actors/actors/fil-upgrade-actor/Cargo.toml create mode 100644 testing/test_actors/actors/fil-upgrade-actor/src/actor.rs create mode 100644 testing/test_actors/actors/fil-upgrade-actor/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 70b811cb4..f830adf71 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1929,6 +1929,18 @@ dependencies = [ "multihash 0.18.1", ] +[[package]] +name = "fil_upgrade_actor" +version = "0.1.0" +dependencies = [ + "cid 0.10.1", + "fvm_ipld_encoding 0.4.0", + "fvm_sdk 3.3.0", + "fvm_shared 3.5.0", + "serde", + "serde_tuple", +] + [[package]] name = "filecoin-hashers" version = "11.0.0" diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index f92c573c6..3462f5fcd 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -6,13 +6,14 @@ use anyhow::{anyhow, Context}; use cid::Cid; use derive_more::{Deref, DerefMut}; use fvm_ipld_amt::Amt; +use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_shared::address::{Address, Payload}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; -use fvm_shared::{ActorID, MethodNum, METHOD_SEND}; +use fvm_shared::{ActorID, MethodNum, METHOD_SEND, METHOD_UPGRADE}; use num_traits::Zero; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; @@ -262,6 +263,18 @@ where ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "fatal")) } Err(ExecutionError::Syscall(s)) => ExecutionEvent::CallError(s.clone()), + Err(ExecutionError::Abort(Abort::Return(maybe_block))) => { + ExecutionEvent::CallReturn( + ExitCode::USR_FORBIDDEN, + match maybe_block { + Some(block_id) => IpldBlock::serialize_cbor(&block_id).unwrap(), + None => None, + }, + ) + } + Err(ExecutionError::Abort(_)) => { + ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "aborted")) + } }); } @@ -412,6 +425,144 @@ where Ok(()) } + fn upgrade_actor>( + &mut self, + actor_id: ActorID, + new_code_cid: Cid, + params: Option, + ) -> Result { + let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; + + let origin = self.origin; + let state = self + .state_tree_mut() + .get_actor(actor_id)? + .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; + + self.state_tree_mut().set_actor( + origin, + ActorState::new( + new_code_cid, + state.state, + state.balance, + state.sequence, + None, + ), + ); + + // when we successfully upgrade an actor we want to abort the calling actor which + // made the upgrade and return the block id of the new actor state. + Err(ExecutionError::from(Abort::Exit( + ExitCode::OK, + String::from("actor upgraded"), + result, + ))) + } + + fn upgrade_actor_inner>( + &mut self, + actor_id: ActorID, + new_code_cid: Cid, + params: Option, + ) -> Result { + let state = self + .state_tree() + .get_actor(actor_id)? + .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; + + let mut block_registry = BlockRegistry::new(); + let params_id = if let Some(blk) = params { + block_registry.put(blk)? + } else { + NO_DATA_BLOCK_ID + }; + + log::trace!( + "upgrading {} from {} to {}", + actor_id, + state.code, + new_code_cid + ); + self.map_mut(|cm| { + let engine = cm.engine.clone(); // reference the RC. + + // Make the kernel. + let kernel = K::new( + cm, + block_registry, + actor_id, + actor_id, + 1919, + TokenAmount::from_atto(0), + false, + ); + + // Make a store. + let mut store = engine.new_store(kernel); + + let result: std::result::Result = (|| { + // Instantiate the module. + let instance = engine.instantiate(&mut store, &new_code_cid); + if instance.is_err() { + return Err(syscall_error!(NotFound, "actor not found").into()); + } + + // Resolve and store a reference to the exported memory. + let memory = instance + .as_ref() + .unwrap() + .unwrap() + .get_memory(&mut store, "memory") + .context("actor has no memory export") + .map_err(Abort::Fatal)?; + + store.data_mut().memory = memory; + + // Lookup the upgrade method. + let res = instance + .unwrap() + .unwrap() + .get_typed_func(&mut store, "upgrade"); + if res.is_err() { + return Err(syscall_error!(Forbidden, "actor does not support upgrade").into()); + } + let invoke = res.unwrap(); + + // Set the available gas. + update_gas_available(&mut store)?; + + // Invoke it. + let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + invoke.call(&mut store, (params_id,)) + })) + .map_err(|panic| { + Abort::Fatal(anyhow!("panic within actor upgrade: {:?}", panic)) + })?; + + // Charge for any remaining uncharged execution gas, returning an error if we run + // out. + charge_for_exec(&mut store)?; + + // If the invocation failed due to running out of exec_units, we have already + // detected it and returned OutOfGas above. Any other invocation failure is returned + // here as an Abort + Ok(res.map_err(|_| { + Abort::Exit( + ExitCode::SYS_MISSING_RETURN, + String::from("returned block does not exist"), + NO_DATA_BLOCK_ID, + ) + })?) + })(); + + let invocation_data = store.into_data(); + let _last_error = invocation_data.last_error; + let (cm, _block_registry) = invocation_data.kernel.into_inner(); + + (result, cm) + }) + } + fn append_event(&mut self, evt: StampedEvent) { self.events.append_event(evt) } @@ -828,6 +979,23 @@ where "fatal error".to_owned(), Err(ExecutionError::Fatal(err)), ), + // if we successfully upgrade an actor, we abort with the block id as the value + Abort::Return(ret) if method == METHOD_UPGRADE => ( + ExitCode::OK, + String::from("aborted"), + Ok(InvocationResult { + exit_code: ExitCode::OK, + value: match ret { + Some(blk_id) => block_registry.get(blk_id).ok().cloned(), + None => None, + }, + }), + ), + Abort::Return(_) => ( + ExitCode::USR_FORBIDDEN, + String::from("aborted"), + Err(ExecutionError::Fatal(anyhow!("forbidden"))), + ), }; if !code.is_success() { diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 03f4ec638..3d83fdb0a 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -111,6 +111,24 @@ pub trait CallManager: 'static { delegated_address: Option
, ) -> Result<()>; + fn upgrade_actor( + &mut self, + actor_id: ActorID, + new_code_cid: Cid, + params: Option, + ) -> Result + where + K: Kernel; + + fn upgrade_actor_inner( + &mut self, + actor_id: ActorID, + new_code_cid: Cid, + params: Option, + ) -> Result + where + K: Kernel; + /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 0bc982140..cad1e9de7 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -5,7 +5,7 @@ use std::result::Result as StdResult; use anyhow::{anyhow, Result}; use cid::Cid; -use fvm_ipld_encoding::{RawBytes, CBOR}; +use fvm_ipld_encoding::{to_vec, RawBytes, CBOR}; use fvm_shared::address::Payload; use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; @@ -22,6 +22,7 @@ use crate::engine::EnginePool; use crate::gas::{Gas, GasCharge, GasOutputs}; use crate::kernel::{Block, ClassifyResult, Context as _, ExecutionError, Kernel}; use crate::machine::{Machine, BURNT_FUNDS_ACTOR_ID, REWARD_ACTOR_ID}; +use crate::syscalls::error::Abort; use crate::trace::ExecutionTrace; /// The default [`Executor`]. @@ -199,6 +200,18 @@ where events_root, } } + Err(ExecutionError::Abort(Abort::Return(maybe_block))) => Receipt { + exit_code: ExitCode::OK, + return_data: match maybe_block { + Some(block_id) => RawBytes::new(to_vec(&block_id).unwrap()), + None => RawBytes::default(), + }, + gas_used, + events_root, + }, + + Err(ExecutionError::Abort(e)) => return Err(anyhow!("actor aborted: {}", e)), + Err(ExecutionError::OutOfGas) => Receipt { exit_code: ExitCode::SYS_OUT_OF_GAS, return_data: Default::default(), diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 283e7c2d2..466ddf2f1 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -864,6 +864,24 @@ where .create_actor(code_id, actor_id, delegated_address) } + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + if self.read_only { + return Err( + syscall_error!(ReadOnly, "upgrade_actor cannot be called while read-only").into(), + ); + } + + // Load parameters. + let params = if params_id == NO_DATA_BLOCK_ID { + None + } else { + Some(self.blocks.get(params_id)?.clone()) + }; + + self.call_manager + .upgrade_actor::(self.actor_id, new_code_cid, params) + } + fn get_builtin_actor_type(&self, code_cid: &Cid) -> Result { let t = self .call_manager diff --git a/fvm/src/kernel/error.rs b/fvm/src/kernel/error.rs index c909a6a87..0b9ec651d 100644 --- a/fvm/src/kernel/error.rs +++ b/fvm/src/kernel/error.rs @@ -5,6 +5,8 @@ use std::fmt::Display; use derive_more::Display; use fvm_shared::error::ErrorNumber; +use crate::syscalls::error::Abort; + /// Execution result. pub type Result = std::result::Result; @@ -35,6 +37,7 @@ pub enum ExecutionError { OutOfGas, Syscall(SyscallError), Fatal(anyhow::Error), + Abort(Abort), } impl ExecutionError { @@ -52,6 +55,8 @@ impl ExecutionError { match self { Fatal(_) => true, OutOfGas | Syscall(_) => false, + Abort(crate::syscalls::error::Abort::Return(_)) => false, + Abort(_) => true, } } @@ -60,8 +65,8 @@ impl ExecutionError { pub fn is_recoverable(&self) -> bool { use ExecutionError::*; match self { - OutOfGas | Fatal(_) => false, - Syscall(_) => true, + Syscall(_) | Abort(crate::syscalls::error::Abort::Return(_)) => true, + OutOfGas | Fatal(_) | Abort(_) => false, } } } @@ -74,6 +79,12 @@ impl From for ExecutionError { } } +impl From for ExecutionError { + fn from(e: Abort) -> Self { + ExecutionError::Abort(e) + } +} + pub trait ClassifyResult: Sized { type Value; type Error; @@ -148,6 +159,7 @@ impl Context for ExecutionError { Syscall(e) => Syscall(SyscallError(format!("{}: {}", context, e.0), e.1)), Fatal(e) => Fatal(e.context(context.to_string())), OutOfGas => OutOfGas, // no reason necessary + Abort(_) => self, } } @@ -168,6 +180,7 @@ impl From for anyhow::Error { OutOfGas => anyhow::anyhow!("out of gas"), Syscall(err) => anyhow::anyhow!(err.0), Fatal(err) => err, + Abort(e) => anyhow::anyhow!("aborted: {}", e), } } } diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index dcf5c547f..2d8aaf01a 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -209,6 +209,8 @@ pub trait ActorOps { delegated_address: Option
, ) -> Result<()>; + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result; + /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] fn install_actor(&mut self, code_cid: Cid) -> Result<()>; diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 7376631e9..e18749f79 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -4,6 +4,7 @@ use anyhow::{anyhow, Context as _}; use fvm_shared::{sys, ActorID}; use super::Context; +use crate::kernel::BlockId; use crate::kernel::{ClassifyResult, Result}; use crate::{syscall_error, Kernel}; @@ -109,6 +110,17 @@ pub fn create_actor( context.kernel.create_actor(typ, actor_id, addr) } +#[allow(dead_code)] +pub fn upgrade_actor( + context: Context<'_, impl Kernel>, + new_code_cid_off: u32, + params_id: u32, +) -> Result { + let cid = context.memory.read_cid(new_code_cid_off)?; + + context.kernel.upgrade_actor(cid, params_id) +} + pub fn get_builtin_actor_type( context: Context<'_, impl Kernel>, code_cid_off: u32, // Cid diff --git a/fvm/src/syscalls/bind.rs b/fvm/src/syscalls/bind.rs index 5ebb2d765..f3a5967d9 100644 --- a/fvm/src/syscalls/bind.rs +++ b/fvm/src/syscalls/bind.rs @@ -81,6 +81,7 @@ where ExecutionError::Syscall(err) => Ok(Err(err)), ExecutionError::OutOfGas => Err(Abort::OutOfGas), ExecutionError::Fatal(err) => Err(Abort::Fatal(err)), + ExecutionError::Abort(e) => Err(e), }, } } diff --git a/fvm/src/syscalls/context.rs b/fvm/src/syscalls/context.rs index fac49f9b2..5a3d7a53a 100644 --- a/fvm/src/syscalls/context.rs +++ b/fvm/src/syscalls/context.rs @@ -152,6 +152,9 @@ mod test { $crate::kernel::ExecutionError::OutOfGas => { panic!("got unexpected out of gas") } + $crate::kernel::ExecutionError::Abort(abort) => { + panic!("got unexpected abort {}", abort) + } } }; } diff --git a/fvm/src/syscalls/error.rs b/fvm/src/syscalls/error.rs index b7193e543..aec164e4b 100644 --- a/fvm/src/syscalls/error.rs +++ b/fvm/src/syscalls/error.rs @@ -20,6 +20,9 @@ pub enum Abort { /// The system failed with a fatal error. #[error("fatal error: {0}")] Fatal(anyhow::Error), + /// The actor aborted with a block id + #[error("abortive non-local return {0:?}")] + Return(Option), } impl Abort { @@ -37,6 +40,7 @@ impl Abort { ), ExecutionError::OutOfGas => Abort::OutOfGas, ExecutionError::Fatal(err) => Abort::Fatal(err), + ExecutionError::Abort(e) => e, } } @@ -46,6 +50,7 @@ impl Abort { ExecutionError::OutOfGas => Abort::OutOfGas, ExecutionError::Fatal(e) => Abort::Fatal(e), ExecutionError::Syscall(e) => Abort::Fatal(anyhow!("unexpected syscall error: {}", e)), + ExecutionError::Abort(e) => e, } } } diff --git a/fvm/src/syscalls/mod.rs b/fvm/src/syscalls/mod.rs index 26cbf242a..fda51081e 100644 --- a/fvm/src/syscalls/mod.rs +++ b/fvm/src/syscalls/mod.rs @@ -269,6 +269,7 @@ pub fn bind_syscalls( linker.bind("actor", "get_actor_code_cid", actor::get_actor_code_cid)?; linker.bind("actor", "next_actor_address", actor::next_actor_address)?; linker.bind("actor", "create_actor", actor::create_actor)?; + linker.bind("actor", "upgrade_actor", actor::upgrade_actor)?; linker.bind( "actor", "get_builtin_actor_type", diff --git a/fvm/tests/default_kernel/mod.rs b/fvm/tests/default_kernel/mod.rs index 3827e9f45..8ef956176 100644 --- a/fvm/tests/default_kernel/mod.rs +++ b/fvm/tests/default_kernel/mod.rs @@ -76,6 +76,9 @@ macro_rules! expect_syscall_err { ::fvm::kernel::ExecutionError::OutOfGas => { panic!("got unexpected out of gas") } + ::fvm::kernel::ExecutionError::Abort(abort) => { + panic!("got unexpected abort {}", abort) + } } }; } @@ -91,6 +94,9 @@ macro_rules! expect_out_of_gas { ::fvm::kernel::ExecutionError::Fatal(err) => { panic!("got unexpected fatal error: {}", err) } + ::fvm::kernel::ExecutionError::Abort(abort) => { + panic!("got unexpected abort {}", abort) + } } }; } diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 8fd2ea9ad..652175ffc 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -392,4 +392,28 @@ impl CallManager for DummyCallManager { ) -> fvm::kernel::Result<()> { todo!() } + + fn upgrade_actor( + &mut self, + _actor_id: ActorID, + _new_code_cid: Cid, + _params: Option, + ) -> kernel::Result + where + K: Kernel, + { + todo!() + } + + fn upgrade_actor_inner( + &mut self, + _actor_id: ActorID, + _new_code_cid: Cid, + _params: Option, + ) -> kernel::Result + where + K: Kernel, + { + todo!() + } } diff --git a/sdk/src/actor.rs b/sdk/src/actor.rs index 3aa6c283f..b0f8bf511 100644 --- a/sdk/src/actor.rs +++ b/sdk/src/actor.rs @@ -4,13 +4,14 @@ use core::option::Option; use std::ptr; // no_std use cid::Cid; +use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::{Address, Payload, MAX_ADDRESS_LEN}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ErrorNumber; use fvm_shared::{ActorID, MAX_CID_LEN}; use log::error; -use crate::{sys, SyscallResult}; +use crate::{sys, SyscallResult, NO_DATA_BLOCK_ID}; /// Resolves the ID address of an actor. Returns `None` if the address cannot be resolved. /// Successfully resolving an address doesn't necessarily mean the actor exists (e.g., if the @@ -107,6 +108,20 @@ pub fn create_actor( } } +/// Upgrades an actor using the given block which includes the old code cid and the upgrade params +pub fn upgrade_actor(new_code_cid: Cid, params: Option) -> SyscallResult { + unsafe { + let cid = new_code_cid.to_bytes(); + + let params_id = match params { + Some(p) => sys::ipld::block_create(p.codec, p.data.as_ptr(), p.data.len() as u32)?, + None => NO_DATA_BLOCK_ID, + }; + + sys::actor::upgrade_actor(cid.as_ptr(), params_id) + } +} + /// Installs or ensures an actor code CID is valid and loaded. /// Note: this is a privileged syscall, restricted to the init actor. #[cfg(feature = "m2-native")] diff --git a/sdk/src/sys/actor.rs b/sdk/src/sys/actor.rs index af8024729..25f9a8ebc 100644 --- a/sdk/src/sys/actor.rs +++ b/sdk/src/sys/actor.rs @@ -128,6 +128,39 @@ super::fvm_syscalls! { delegated_addr_len: u32, ) -> Result<()>; + + /// Atomically transition to the new actor code. On success, this syscall does not return to the + /// current actor. Instead, the target actor "replaces" the invocation. + /// + /// # Parameters + /// + /// - `new_code_cid_off` is the offset (in wasm memory) of the code CID to upgrade _to_. + /// - `params` is the IPLD block handle passed to the new code's `upgrade` wasm endpoint. + /// + /// # Returns + /// + /// On success, this syscall will not return. Instead, the current invocation will "complete" and + /// the return value will be the block returned by the new code's `upgrade` endpoint. + /// + /// If the new code rejects the upgrade (aborts) or performs an illegal operation, this syscall will + /// return the exit code of the `upgrade` endpoint. + /// + /// Finally, the syscall will return an error if it fails to call the upgrade endpoint entirely. + /// + /// # Errors + /// + /// | Error | Reason | + /// |-----------------------|------------------------------------------------------| + /// | [`NotFound`] | there's no actor deployed with the target code cid. | + /// | [`InvalidHandle`] | parameters block not found. | + /// | [`LimitExceeded`] | recursion limit reached. | + /// | [`IllegalArgument`] | invalid code cid buffer. | + /// | [`Forbidden`] | target actor doesn't have an upgrade endpoint. | + pub fn upgrade_actor( + new_code_cid_off: *const u8, + params: u32, + ) -> Result; + /// Installs and ensures actor code is valid and loaded. /// **Privileged:** May only be called by the init actor. #[cfg(feature = "m2-native")] diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 765281597..a17378795 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -108,6 +108,8 @@ pub type MethodNum = u64; pub const METHOD_SEND: MethodNum = 0; /// Base actor constructor method. pub const METHOD_CONSTRUCTOR: MethodNum = 1; +/// Upgraded actor constructor method. +pub const METHOD_UPGRADE: MethodNum = 191919; /// The outcome of a `Send`, covering its ExitCode and optional return data #[derive(Debug, PartialEq, Eq, Clone)] diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index 4ee681b49..fb801e643 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -298,6 +298,10 @@ where fn lookup_delegated_address(&self, actor_id: ActorID) -> Result> { self.0.lookup_delegated_address(actor_id) } + + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + self.0.upgrade_actor(new_code_cid, params_id) + } } impl IpldBlockOps for TestKernel diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs new file mode 100644 index 000000000..d74e4bd24 --- /dev/null +++ b/testing/integration/tests/upgrade_test.rs @@ -0,0 +1,67 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT +mod bundles; +use bundles::*; +use fvm::executor::{ApplyKind, Executor}; +use fvm_integration_tests::dummy::DummyExterns; +use fvm_ipld_blockstore::MemoryBlockstore; +use fvm_shared::address::Address; +use fvm_shared::econ::TokenAmount; +use fvm_shared::message::Message; +use fvm_shared::state::StateTreeVersion; +use fvm_shared::version::NetworkVersion; +use fvm_test_actors::wasm_bin::UPGRADE_ACTOR_BINARY; +use num_traits::Zero; + +#[test] +fn upgrade_actor_test() { + // Instantiate tester + let mut tester = new_tester( + NetworkVersion::V18, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) + .unwrap(); + + let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); + + let wasm_bin = UPGRADE_ACTOR_BINARY; + + // Set actor state + let actor_state = [(); 0]; + let state_cid = tester.set_state(&actor_state).unwrap(); + + // Set actor + let actor_address = Address::new_id(10000); + + tester + .set_actor_from_bin(wasm_bin, state_cid, actor_address, TokenAmount::zero()) + .unwrap(); + + // Instantiate machine + tester.instantiate_machine(DummyExterns).unwrap(); + + let executor = tester.executor.as_mut().unwrap(); + + for (seq, method) in (1..=1).enumerate() { + let message = Message { + from: sender_address, + to: actor_address, + gas_limit: 1000000000, + method_num: method, + sequence: seq as u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } +} diff --git a/testing/test_actors/actors/fil-syscall-actor/src/actor.rs b/testing/test_actors/actors/fil-syscall-actor/src/actor.rs index 927aa4ac8..fe86d97cb 100644 --- a/testing/test_actors/actors/fil-syscall-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-syscall-actor/src/actor.rs @@ -40,6 +40,7 @@ pub fn invoke(_: u32) -> u32 { test_message_context(); test_balance(); test_unaligned(); + test_upgrade(); #[cfg(coverage)] sdk::debug::store_artifact("syscall_actor.profraw", minicov::capture_coverage()); @@ -377,3 +378,13 @@ fn test_unaligned() { assert_eq!(expected, actual); } } + +fn test_upgrade() { + // test that calling `upgrade_actor` on ourselves results in a Forbidden error + // since we don't have a upgrade endpoint + let res = sdk::actor::upgrade_actor( + sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(), + None, + ); + assert_eq!(res, Err(ErrorNumber::Forbidden)); +} diff --git a/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml b/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml new file mode 100644 index 000000000..91dc61cbc --- /dev/null +++ b/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "fil_upgrade_actor" +version = "0.1.0" +edition = "2021" +publish = false + +[target.'cfg(target_arch = "wasm32")'.dependencies] +fvm_sdk = { version = "3.3.0", path = "../../../../sdk" } +fvm_shared = { version = "3.5.0", path = "../../../../shared" } +fvm_ipld_encoding = { version = "0.4.0", path = "../../../../ipld/encoding" } +cid = { workspace = true } +serde = { version = "1.0.164", features = ["derive"] } +serde_tuple = "0.5.0" + +[lib] +crate-type = ["cdylib"] ## cdylib is necessary for Wasm build diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs new file mode 100644 index 000000000..49e56fc10 --- /dev/null +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -0,0 +1,54 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT +use fvm_ipld_encoding::ipld_block::IpldBlock; +//use fvm_ipld_encoding::CBOR; +use fvm_sdk as sdk; +use fvm_shared::address::Address; +use sdk::sys::ErrorNumber; +use serde_tuple::*; + +//use sdk::sys::ErrorNumber; +//use fvm_shared::sys::BlockId; + +#[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] +struct Params { + value: u64, +} + +#[no_mangle] +pub fn upgrade(params: u32) -> u32 { + // verify that the params we sent from invoke are the same as the params we got here + let msg_params = sdk::message::params_raw(params).unwrap().unwrap(); + assert_eq!(msg_params.codec, fvm_ipld_encoding::CBOR); + let p: Params = fvm_ipld_encoding::from_slice(msg_params.data.as_slice()).unwrap(); + sdk::debug::log(format!("upgrade:: Param value: {}", p.value).to_string()); + assert_eq!(p.value, 10101); + + 0 +} + +#[no_mangle] +pub fn invoke(_: u32) -> u32 { + sdk::initialize(); + + let method = sdk::message::method_number(); + sdk::debug::log(format!("called upgrade_actor with method: {}", method).to_string()); + + match method { + // test that calling `upgrade_actor` on ourselves results will not return + 1 => { + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + let params = IpldBlock::serialize_cbor(&Params { value: 10101 }).unwrap(); + let _ = sdk::actor::upgrade_actor(new_code_cid, params); + assert!(false, "we should never return from a successful upgrade"); + } + _ => { + sdk::vm::abort( + fvm_shared::error::ExitCode::FIRST_USER_EXIT_CODE, + Some(format!("bad method {}", method).as_str()), + ); + } + } + + 0 +} diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/lib.rs b/testing/test_actors/actors/fil-upgrade-actor/src/lib.rs new file mode 100644 index 000000000..1d4daedef --- /dev/null +++ b/testing/test_actors/actors/fil-upgrade-actor/src/lib.rs @@ -0,0 +1,4 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT +#[cfg(target_arch = "wasm32")] +mod actor; diff --git a/testing/test_actors/build.rs b/testing/test_actors/build.rs index a1865c6f5..b3bc2c5e4 100644 --- a/testing/test_actors/build.rs +++ b/testing/test_actors/build.rs @@ -31,6 +31,7 @@ const ACTORS: &[(&str, &str)] = &[ ("CREATE_ACTOR_BINARY", "fil_create_actor"), ("OOM_ACTOR_BINARY", "fil_oom_actor"), ("SSELF_ACTOR_BINARY", "fil_sself_actor"), + ("UPGRADE_ACTOR_BINARY", "fil_upgrade_actor"), ]; const WASM_TARGET: &str = "wasm32-unknown-unknown"; From 3d5a07ded8bcc3526423b6a0f679fedc66dff32a Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 31 Aug 2023 14:26:29 +0000 Subject: [PATCH 02/40] Add UpgradeInfo as ipld block to upgrade endpoint --- fvm/src/call_manager/default.rs | 15 +++++++++++- shared/src/lib.rs | 1 + shared/src/upgrade/mod.rs | 10 ++++++++ .../actors/fil-upgrade-actor/src/actor.rs | 23 +++++++++++++------ 4 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 shared/src/upgrade/mod.rs diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 3462f5fcd..51cc35fbc 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -13,6 +13,7 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; +use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum, METHOD_SEND, METHOD_UPGRADE}; use num_traits::Zero; @@ -471,12 +472,24 @@ where .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; let mut block_registry = BlockRegistry::new(); + + // add the params block to the block registry let params_id = if let Some(blk) = params { block_registry.put(blk)? } else { NO_DATA_BLOCK_ID }; + // also add a new block to the registry which includes the UpgradeInfo which + // we pass to the actor's upgrade method + let ui = UpgradeInfo { + old_code_cid: state.code, + }; + let ui_params = to_vec(&ui).map_err( + |e| syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), + )?; + let ui_params_id = block_registry.put(Block::new(CBOR, ui_params))?; + log::trace!( "upgrading {} from {} to {}", actor_id, @@ -533,7 +546,7 @@ where // Invoke it. let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - invoke.call(&mut store, (params_id,)) + invoke.call(&mut store, (params_id, ui_params_id)) })) .map_err(|panic| { Abort::Fatal(anyhow!("panic within actor upgrade: {:?}", panic)) diff --git a/shared/src/lib.rs b/shared/src/lib.rs index a17378795..060c795b2 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -29,6 +29,7 @@ pub mod sector; pub mod smooth; pub mod state; pub mod sys; +pub mod upgrade; pub mod version; use econ::TokenAmount; diff --git a/shared/src/upgrade/mod.rs b/shared/src/upgrade/mod.rs new file mode 100644 index 000000000..48da6a760 --- /dev/null +++ b/shared/src/upgrade/mod.rs @@ -0,0 +1,10 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT +use cid::Cid; +use fvm_ipld_encoding::tuple::*; + +#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] +pub struct UpgradeInfo { + // the old code cid we are upgrading from + pub old_code_cid: Cid, +} diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index 49e56fc10..bc84db3e3 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -4,26 +4,35 @@ use fvm_ipld_encoding::ipld_block::IpldBlock; //use fvm_ipld_encoding::CBOR; use fvm_sdk as sdk; use fvm_shared::address::Address; -use sdk::sys::ErrorNumber; +//use sdk::sys::ErrorNumber; +use fvm_shared::upgrade::UpgradeInfo; use serde_tuple::*; //use sdk::sys::ErrorNumber; //use fvm_shared::sys::BlockId; #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] -struct Params { +struct SomeStruct { value: u64, } #[no_mangle] -pub fn upgrade(params: u32) -> u32 { +pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { // verify that the params we sent from invoke are the same as the params we got here - let msg_params = sdk::message::params_raw(params).unwrap().unwrap(); - assert_eq!(msg_params.codec, fvm_ipld_encoding::CBOR); - let p: Params = fvm_ipld_encoding::from_slice(msg_params.data.as_slice()).unwrap(); + let params = sdk::message::params_raw(params_id).unwrap().unwrap(); + assert_eq!(params.codec, fvm_ipld_encoding::CBOR); + let p = params.deserialize::().unwrap(); + //let p: UpgradeParams = fvm_ipld_encoding::from_slice(params.data.as_slice()).unwrap(); sdk::debug::log(format!("upgrade:: Param value: {}", p.value).to_string()); assert_eq!(p.value, 10101); + // verify that the params we sent from invoke are the same as the params we got here + let ui_params = sdk::message::params_raw(upgrade_info_id).unwrap().unwrap(); + assert_eq!(ui_params.codec, fvm_ipld_encoding::CBOR); + let ui = ui_params.deserialize::().unwrap(); + //let p: Params = fvm_ipld_encoding::from_slice(msg_params.data.as_slice()).unwrap(); + sdk::debug::log(format!("upgrade: old_code_cid: {}", ui.old_code_cid).to_string()); + 0 } @@ -38,7 +47,7 @@ pub fn invoke(_: u32) -> u32 { // test that calling `upgrade_actor` on ourselves results will not return 1 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - let params = IpldBlock::serialize_cbor(&Params { value: 10101 }).unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 10101 }).unwrap(); let _ = sdk::actor::upgrade_actor(new_code_cid, params); assert!(false, "we should never return from a successful upgrade"); } From d26bafbe58b067a1d3f1b98818b67bd17db82bed Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 31 Aug 2023 14:27:02 +0000 Subject: [PATCH 03/40] Cleanup unused Abort enum --- fvm/src/call_manager/default.rs | 29 +---------------------------- fvm/src/executor/default.rs | 12 +----------- fvm/src/kernel/error.rs | 3 +-- fvm/src/syscalls/error.rs | 3 --- 4 files changed, 3 insertions(+), 44 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 51cc35fbc..6d863a756 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -6,7 +6,6 @@ use anyhow::{anyhow, Context}; use cid::Cid; use derive_more::{Deref, DerefMut}; use fvm_ipld_amt::Amt; -use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_shared::address::{Address, Payload}; use fvm_shared::econ::TokenAmount; @@ -14,7 +13,7 @@ use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; use fvm_shared::upgrade::UpgradeInfo; -use fvm_shared::{ActorID, MethodNum, METHOD_SEND, METHOD_UPGRADE}; +use fvm_shared::{ActorID, MethodNum, METHOD_SEND}; use num_traits::Zero; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; @@ -264,15 +263,6 @@ where ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "fatal")) } Err(ExecutionError::Syscall(s)) => ExecutionEvent::CallError(s.clone()), - Err(ExecutionError::Abort(Abort::Return(maybe_block))) => { - ExecutionEvent::CallReturn( - ExitCode::USR_FORBIDDEN, - match maybe_block { - Some(block_id) => IpldBlock::serialize_cbor(&block_id).unwrap(), - None => None, - }, - ) - } Err(ExecutionError::Abort(_)) => { ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "aborted")) } @@ -992,23 +982,6 @@ where "fatal error".to_owned(), Err(ExecutionError::Fatal(err)), ), - // if we successfully upgrade an actor, we abort with the block id as the value - Abort::Return(ret) if method == METHOD_UPGRADE => ( - ExitCode::OK, - String::from("aborted"), - Ok(InvocationResult { - exit_code: ExitCode::OK, - value: match ret { - Some(blk_id) => block_registry.get(blk_id).ok().cloned(), - None => None, - }, - }), - ), - Abort::Return(_) => ( - ExitCode::USR_FORBIDDEN, - String::from("aborted"), - Err(ExecutionError::Fatal(anyhow!("forbidden"))), - ), }; if !code.is_success() { diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index cad1e9de7..32b00d37f 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -5,7 +5,7 @@ use std::result::Result as StdResult; use anyhow::{anyhow, Result}; use cid::Cid; -use fvm_ipld_encoding::{to_vec, RawBytes, CBOR}; +use fvm_ipld_encoding::{RawBytes, CBOR}; use fvm_shared::address::Payload; use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; @@ -22,7 +22,6 @@ use crate::engine::EnginePool; use crate::gas::{Gas, GasCharge, GasOutputs}; use crate::kernel::{Block, ClassifyResult, Context as _, ExecutionError, Kernel}; use crate::machine::{Machine, BURNT_FUNDS_ACTOR_ID, REWARD_ACTOR_ID}; -use crate::syscalls::error::Abort; use crate::trace::ExecutionTrace; /// The default [`Executor`]. @@ -200,15 +199,6 @@ where events_root, } } - Err(ExecutionError::Abort(Abort::Return(maybe_block))) => Receipt { - exit_code: ExitCode::OK, - return_data: match maybe_block { - Some(block_id) => RawBytes::new(to_vec(&block_id).unwrap()), - None => RawBytes::default(), - }, - gas_used, - events_root, - }, Err(ExecutionError::Abort(e)) => return Err(anyhow!("actor aborted: {}", e)), diff --git a/fvm/src/kernel/error.rs b/fvm/src/kernel/error.rs index 0b9ec651d..bf9dc9dd7 100644 --- a/fvm/src/kernel/error.rs +++ b/fvm/src/kernel/error.rs @@ -55,7 +55,6 @@ impl ExecutionError { match self { Fatal(_) => true, OutOfGas | Syscall(_) => false, - Abort(crate::syscalls::error::Abort::Return(_)) => false, Abort(_) => true, } } @@ -65,7 +64,7 @@ impl ExecutionError { pub fn is_recoverable(&self) -> bool { use ExecutionError::*; match self { - Syscall(_) | Abort(crate::syscalls::error::Abort::Return(_)) => true, + Syscall(_) => true, OutOfGas | Fatal(_) | Abort(_) => false, } } diff --git a/fvm/src/syscalls/error.rs b/fvm/src/syscalls/error.rs index aec164e4b..b7ab04458 100644 --- a/fvm/src/syscalls/error.rs +++ b/fvm/src/syscalls/error.rs @@ -20,9 +20,6 @@ pub enum Abort { /// The system failed with a fatal error. #[error("fatal error: {0}")] Fatal(anyhow::Error), - /// The actor aborted with a block id - #[error("abortive non-local return {0:?}")] - Return(Option), } impl Abort { From 078f902ca432f3f2c3c7b87d9ed51cf42ffbba82 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 31 Aug 2023 19:01:57 +0000 Subject: [PATCH 04/40] exit now returns to upgrade caller, still issue with block registry --- fvm/src/call_manager/default.rs | 48 ++++++++----------- testing/integration/tests/upgrade_test.rs | 11 +++-- .../actors/fil-upgrade-actor/src/actor.rs | 36 ++++++++++++-- 3 files changed, 60 insertions(+), 35 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 6d863a756..cb2cb7916 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -422,7 +422,13 @@ where new_code_cid: Cid, params: Option, ) -> Result { - let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; + let result = match self.upgrade_actor_inner::(actor_id, new_code_cid, params) { + Ok(id) => id, + Err(ExecutionError::Abort(Abort::Exit(ExitCode::OK, _, result))) => { + return Ok(result); + } + Err(e) => return Err(e), + }; let origin = self.origin; let state = self @@ -503,18 +509,15 @@ where // Make a store. let mut store = engine.new_store(kernel); - let result: std::result::Result = (|| { + let result: std::result::Result = (|| { // Instantiate the module. - let instance = engine.instantiate(&mut store, &new_code_cid); - if instance.is_err() { - return Err(syscall_error!(NotFound, "actor not found").into()); - } + let instance = engine + .instantiate(&mut store, &new_code_cid)? + .context("actor not found") + .map_err(Abort::Fatal)?; // Resolve and store a reference to the exported memory. let memory = instance - .as_ref() - .unwrap() - .unwrap() .get_memory(&mut store, "memory") .context("actor has no memory export") .map_err(Abort::Fatal)?; @@ -522,21 +525,16 @@ where store.data_mut().memory = memory; // Lookup the upgrade method. - let res = instance - .unwrap() - .unwrap() - .get_typed_func(&mut store, "upgrade"); - if res.is_err() { - return Err(syscall_error!(Forbidden, "actor does not support upgrade").into()); - } - let invoke = res.unwrap(); + let upgrade: wasmtime::TypedFunc<(u32, u32), u32> = instance + .get_typed_func(&mut store, "upgrade") + .map_err(|e| Abort::Fatal(anyhow!("actor does not support upgrade: {}", e)))?; // Set the available gas. update_gas_available(&mut store)?; // Invoke it. let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - invoke.call(&mut store, (params_id, ui_params_id)) + upgrade.call(&mut store, (params_id, ui_params_id)) })) .map_err(|panic| { Abort::Fatal(anyhow!("panic within actor upgrade: {:?}", panic)) @@ -546,16 +544,9 @@ where // out. charge_for_exec(&mut store)?; - // If the invocation failed due to running out of exec_units, we have already - // detected it and returned OutOfGas above. Any other invocation failure is returned - // here as an Abort - Ok(res.map_err(|_| { - Abort::Exit( - ExitCode::SYS_MISSING_RETURN, - String::from("returned block does not exist"), - NO_DATA_BLOCK_ID, - ) - })?) + log::info!("returned with: {:?}", res); + + Ok(res?) })(); let invocation_data = store.into_data(); @@ -564,6 +555,7 @@ where (result, cm) }) + .map_err(ExecutionError::from) } fn append_event(&mut self, evt: StampedEvent) { diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index d74e4bd24..8ca1a5057 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -4,6 +4,7 @@ mod bundles; use bundles::*; use fvm::executor::{ApplyKind, Executor}; use fvm_integration_tests::dummy::DummyExterns; +use fvm_integration_tests::tester::Account; use fvm_ipld_blockstore::MemoryBlockstore; use fvm_shared::address::Address; use fvm_shared::econ::TokenAmount; @@ -23,7 +24,9 @@ fn upgrade_actor_test() { ) .unwrap(); - let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); + let sender: [Account; 2] = tester.create_accounts().unwrap(); + + //let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); let wasm_bin = UPGRADE_ACTOR_BINARY; @@ -43,13 +46,13 @@ fn upgrade_actor_test() { let executor = tester.executor.as_mut().unwrap(); - for (seq, method) in (1..=1).enumerate() { + for (seq, method) in (1..=2).enumerate() { let message = Message { - from: sender_address, + from: sender[seq].1, to: actor_address, gas_limit: 1000000000, method_num: method, - sequence: seq as u64, + sequence: 0 as u64, value: TokenAmount::from_atto(100), ..Message::default() }; diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index bc84db3e3..fb364b420 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -1,9 +1,10 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT use fvm_ipld_encoding::ipld_block::IpldBlock; -//use fvm_ipld_encoding::CBOR; +use fvm_ipld_encoding::CBOR; use fvm_sdk as sdk; use fvm_shared::address::Address; +use fvm_shared::error::ExitCode; //use sdk::sys::ErrorNumber; use fvm_shared::upgrade::UpgradeInfo; use serde_tuple::*; @@ -16,15 +17,21 @@ struct SomeStruct { value: u64, } +#[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] +struct SomeReturnStruct { + value: u64, +} + #[no_mangle] pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { + sdk::initialize(); + // verify that the params we sent from invoke are the same as the params we got here let params = sdk::message::params_raw(params_id).unwrap().unwrap(); assert_eq!(params.codec, fvm_ipld_encoding::CBOR); let p = params.deserialize::().unwrap(); //let p: UpgradeParams = fvm_ipld_encoding::from_slice(params.data.as_slice()).unwrap(); sdk::debug::log(format!("upgrade:: Param value: {}", p.value).to_string()); - assert_eq!(p.value, 10101); // verify that the params we sent from invoke are the same as the params we got here let ui_params = sdk::message::params_raw(upgrade_info_id).unwrap().unwrap(); @@ -33,7 +40,15 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { //let p: Params = fvm_ipld_encoding::from_slice(msg_params.data.as_slice()).unwrap(); sdk::debug::log(format!("upgrade: old_code_cid: {}", ui.old_code_cid).to_string()); - 0 + if p.value == 10101 { + return 0; + } + + sdk::vm::exit( + 0, + IpldBlock::serialize_cbor(&SomeReturnStruct { value: 2020202 }).unwrap(), + None, + ) } #[no_mangle] @@ -51,6 +66,21 @@ pub fn invoke(_: u32) -> u32 { let _ = sdk::actor::upgrade_actor(new_code_cid, params); assert!(false, "we should never return from a successful upgrade"); } + // test that when `upgrade_actor` fails we return with an error + 2 => { + sdk::debug::log("test 1".to_string()); + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + sdk::debug::log("test 2".to_string()); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 10102 }).unwrap(); + sdk::debug::log("test 3".to_string()); + let ret = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); + sdk::debug::log("test 4".to_string()); + let ret_params = sdk::message::params_raw(ret).unwrap().unwrap(); + sdk::debug::log("test 5".to_string()); + let ret_ui = ret_params.deserialize::().unwrap(); + + sdk::debug::log(format!("upgrade_actor returned with: {:?}", ret_ui).to_string()); + } _ => { sdk::vm::abort( fvm_shared::error::ExitCode::FIRST_USER_EXIT_CODE, From 4847d0424b2a0de1ead52c90a14a59a0e3025726 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 1 Sep 2023 15:54:25 +0000 Subject: [PATCH 05/40] fix issue with block registry + initial handle success/failure --- fvm/src/call_manager/default.rs | 78 +++++++++++++++++++++++++-------- fvm/src/call_manager/mod.rs | 4 +- fvm/src/kernel/default.rs | 36 +++++++++++++-- fvm/src/syscalls/error.rs | 6 ++- fvm/tests/dummy.rs | 4 +- 5 files changed, 102 insertions(+), 26 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index cb2cb7916..8b4c34f94 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -6,7 +6,7 @@ use anyhow::{anyhow, Context}; use cid::Cid; use derive_more::{Deref, DerefMut}; use fvm_ipld_amt::Amt; -use fvm_ipld_encoding::{to_vec, CBOR}; +use fvm_ipld_encoding::{to_vec, RawBytes, CBOR}; use fvm_shared::address::{Address, Payload}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; @@ -263,6 +263,15 @@ where ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "fatal")) } Err(ExecutionError::Syscall(s)) => ExecutionEvent::CallError(s.clone()), + Err(ExecutionError::Abort(Abort::Return(maybe_block))) => { + ExecutionEvent::CallReturn( + ExitCode::OK, + match maybe_block { + Some(block) => RawBytes::new(block.data().to_vec()).into(), + None => RawBytes::default().into(), + }, + ) + } Err(ExecutionError::Abort(_)) => { ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "aborted")) } @@ -421,14 +430,8 @@ where actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result { - let result = match self.upgrade_actor_inner::(actor_id, new_code_cid, params) { - Ok(id) => id, - Err(ExecutionError::Abort(Abort::Exit(ExitCode::OK, _, result))) => { - return Ok(result); - } - Err(e) => return Err(e), - }; + ) -> Result> { + let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; let origin = self.origin; let state = self @@ -447,13 +450,7 @@ where ), ); - // when we successfully upgrade an actor we want to abort the calling actor which - // made the upgrade and return the block id of the new actor state. - Err(ExecutionError::from(Abort::Exit( - ExitCode::OK, - String::from("actor upgraded"), - result, - ))) + Ok(result) } fn upgrade_actor_inner>( @@ -461,7 +458,7 @@ where actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result { + ) -> Result> { let state = self .state_tree() .get_actor(actor_id)? @@ -551,7 +548,44 @@ where let invocation_data = store.into_data(); let _last_error = invocation_data.last_error; - let (cm, _block_registry) = invocation_data.kernel.into_inner(); + let (cm, block_registry) = invocation_data.kernel.into_inner(); + + let result: std::result::Result, ExecutionError> = match result { + Ok(block_id) => { + if block_id == NO_DATA_BLOCK_ID { + Ok(None) + } else { + Ok(Some( + block_registry + .get(block_id) + .map_err(|_| { + Abort::Exit( + ExitCode::SYS_MISSING_RETURN, + String::from("returned block does not exist"), + NO_DATA_BLOCK_ID, + ) + }) + .cloned() + .unwrap(), + )) + } + } + Err(abort) => match abort { + Abort::Exit(_, _, block_id) => match block_registry.get(block_id) { + Err(e) => Err(ExecutionError::Fatal(anyhow!( + "failed to retrieve block {}: {}", + block_id, + e + ))), + Ok(block) => Err(ExecutionError::Abort(Abort::Return(Some(block.clone())))), + }, + Abort::OutOfGas => Err(ExecutionError::OutOfGas), + Abort::Fatal(err) => Err(ExecutionError::Fatal(err)), + Abort::Return(_) => todo!(), + }, + }; + + //let ret: std::result::Result, ExecutionError> = Ok(None); (result, cm) }) @@ -974,6 +1008,14 @@ where "fatal error".to_owned(), Err(ExecutionError::Fatal(err)), ), + Abort::Return(value) => ( + ExitCode::OK, + String::from("aborted"), + Ok(InvocationResult { + exit_code: ExitCode::OK, + value, + }), + ), }; if !code.is_success() { diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 3d83fdb0a..0dd36f77c 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -116,7 +116,7 @@ pub trait CallManager: 'static { actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result + ) -> Result> where K: Kernel; @@ -125,7 +125,7 @@ pub trait CallManager: 'static { actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result + ) -> Result> where K: Kernel; diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 466ddf2f1..788acf3cc 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -5,6 +5,7 @@ use std::convert::{TryFrom, TryInto}; use std::panic::{self, UnwindSafe}; use std::path::PathBuf; +use crate::syscalls::error::Abort; use anyhow::{anyhow, Context as _}; use cid::Cid; use filecoin_proofs_api::{self as proofs, ProverId, PublicReplicaInfo, SectorId}; @@ -14,7 +15,7 @@ use fvm_shared::address::Payload; use fvm_shared::consensus::ConsensusFault; use fvm_shared::crypto::signature; use fvm_shared::econ::TokenAmount; -use fvm_shared::error::ErrorNumber; +use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::{ActorEvent, Entry, Flags}; use fvm_shared::piece::{zero_piece_commitment, PaddedPieceSize}; use fvm_shared::sector::{RegisteredPoStProof, SectorInfo}; @@ -878,8 +879,37 @@ where Some(self.blocks.get(params_id)?.clone()) }; - self.call_manager - .upgrade_actor::(self.actor_id, new_code_cid, params) + let result = self + .call_manager + .upgrade_actor::(self.actor_id, new_code_cid, params); + + if let Ok(None) = result { + Err(ExecutionError::Abort(Abort::Exit( + ExitCode::OK, + "actor upgraded".to_string(), + NO_DATA_BLOCK_ID, + ))) + } else if let Ok(Some(block)) = result { + let block_id = self + .blocks + .put(block) + .or_fatal() + .context("failed to store a valid return value")?; + Err(ExecutionError::Abort(Abort::Exit( + ExitCode::OK, + "actor upgraded".to_string(), + block_id, + ))) + } else if let Err(ExecutionError::Abort(Abort::Return(Some(block)))) = result { + let block_id = self + .blocks + .put(block) + .or_fatal() + .context("failed to store a valid return value")?; + Ok(block_id) + } else { + Err(result.unwrap_err()) + } } fn get_builtin_actor_type(&self, code_cid: &Cid) -> Result { diff --git a/fvm/src/syscalls/error.rs b/fvm/src/syscalls/error.rs index b7ab04458..d210a6bd1 100644 --- a/fvm/src/syscalls/error.rs +++ b/fvm/src/syscalls/error.rs @@ -6,7 +6,7 @@ use fvm_shared::error::ExitCode; use wasmtime::Trap; use crate::call_manager::NO_DATA_BLOCK_ID; -use crate::kernel::{BlockId, ExecutionError}; +use crate::kernel::{Block, BlockId, ExecutionError}; /// Represents an actor "abort". #[derive(Debug, thiserror::Error)] @@ -20,6 +20,9 @@ pub enum Abort { /// The system failed with a fatal error. #[error("fatal error: {0}")] Fatal(anyhow::Error), + /// The actor aborted with a block. + #[error("abortive non-local return {0:?}")] + Return(Option), } impl Abort { @@ -80,6 +83,7 @@ impl From for Abort { _ => Abort::Fatal(anyhow!("unexpected wasmtime trap: {}", trap)), }; }; + match e.downcast::() { Ok(abort) => abort, Err(e) => Abort::Fatal(e), diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 652175ffc..677f2550c 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -398,7 +398,7 @@ impl CallManager for DummyCallManager { _actor_id: ActorID, _new_code_cid: Cid, _params: Option, - ) -> kernel::Result + ) -> kernel::Result> where K: Kernel, { @@ -410,7 +410,7 @@ impl CallManager for DummyCallManager { _actor_id: ActorID, _new_code_cid: Cid, _params: Option, - ) -> kernel::Result + ) -> kernel::Result> where K: Kernel, { From ab25ab390fdf47215982c68ab0c8b52018a39511 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Mon, 4 Sep 2023 15:35:02 +0000 Subject: [PATCH 06/40] several fixes --- fvm/src/call_manager/default.rs | 82 ++++++------------ fvm/src/call_manager/mod.rs | 4 +- fvm/src/kernel/default.rs | 59 ++++++------- fvm/src/kernel/mod.rs | 2 +- fvm/src/syscalls/actor.rs | 3 +- fvm/src/syscalls/error.rs | 5 +- fvm/tests/dummy.rs | 4 +- testing/integration/tests/upgrade_test.rs | 49 +++++++---- .../actors/fil-upgrade-actor/src/actor.rs | 85 ++++++++++--------- 9 files changed, 138 insertions(+), 155 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 8b4c34f94..012b4f198 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -6,7 +6,7 @@ use anyhow::{anyhow, Context}; use cid::Cid; use derive_more::{Deref, DerefMut}; use fvm_ipld_amt::Amt; -use fvm_ipld_encoding::{to_vec, RawBytes, CBOR}; +use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_shared::address::{Address, Payload}; use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; @@ -263,15 +263,6 @@ where ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "fatal")) } Err(ExecutionError::Syscall(s)) => ExecutionEvent::CallError(s.clone()), - Err(ExecutionError::Abort(Abort::Return(maybe_block))) => { - ExecutionEvent::CallReturn( - ExitCode::OK, - match maybe_block { - Some(block) => RawBytes::new(block.data().to_vec()).into(), - None => RawBytes::default().into(), - }, - ) - } Err(ExecutionError::Abort(_)) => { ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "aborted")) } @@ -430,7 +421,7 @@ where actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result> { + ) -> Result { let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; let origin = self.origin; @@ -439,16 +430,18 @@ where .get_actor(actor_id)? .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; - self.state_tree_mut().set_actor( - origin, - ActorState::new( - new_code_cid, - state.state, - state.balance, - state.sequence, - None, - ), - ); + if result.exit_code == ExitCode::OK { + self.state_tree_mut().set_actor( + origin, + ActorState::new( + new_code_cid, + state.state, + state.balance, + state.sequence, + None, + ), + ); + } Ok(result) } @@ -458,7 +451,7 @@ where actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result> { + ) -> Result { let state = self .state_tree() .get_actor(actor_id)? @@ -550,38 +543,27 @@ where let _last_error = invocation_data.last_error; let (cm, block_registry) = invocation_data.kernel.into_inner(); - let result: std::result::Result, ExecutionError> = match result { + let result: std::result::Result = match result { Ok(block_id) => { if block_id == NO_DATA_BLOCK_ID { - Ok(None) + Ok(InvocationResult { + exit_code: ExitCode::OK, + value: None, + }) } else { - Ok(Some( - block_registry - .get(block_id) - .map_err(|_| { - Abort::Exit( - ExitCode::SYS_MISSING_RETURN, - String::from("returned block does not exist"), - NO_DATA_BLOCK_ID, - ) - }) - .cloned() - .unwrap(), - )) + Ok(InvocationResult { + exit_code: ExitCode::OK, + value: Some(block_registry.get(block_id).unwrap().clone()), + }) } } Err(abort) => match abort { - Abort::Exit(_, _, block_id) => match block_registry.get(block_id) { - Err(e) => Err(ExecutionError::Fatal(anyhow!( - "failed to retrieve block {}: {}", - block_id, - e - ))), - Ok(block) => Err(ExecutionError::Abort(Abort::Return(Some(block.clone())))), - }, + Abort::Exit(exit_code, _message, _block_id) => Ok(InvocationResult { + exit_code, + value: None, + }), Abort::OutOfGas => Err(ExecutionError::OutOfGas), Abort::Fatal(err) => Err(ExecutionError::Fatal(err)), - Abort::Return(_) => todo!(), }, }; @@ -1008,14 +990,6 @@ where "fatal error".to_owned(), Err(ExecutionError::Fatal(err)), ), - Abort::Return(value) => ( - ExitCode::OK, - String::from("aborted"), - Ok(InvocationResult { - exit_code: ExitCode::OK, - value, - }), - ), }; if !code.is_success() { diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 0dd36f77c..a1755eb59 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -116,7 +116,7 @@ pub trait CallManager: 'static { actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result> + ) -> Result where K: Kernel; @@ -125,7 +125,7 @@ pub trait CallManager: 'static { actor_id: ActorID, new_code_cid: Cid, params: Option, - ) -> Result> + ) -> Result where K: Kernel; diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 788acf3cc..5f26f76cd 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -865,7 +865,7 @@ where .create_actor(code_id, actor_id, delegated_address) } - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { if self.read_only { return Err( syscall_error!(ReadOnly, "upgrade_actor cannot be called while read-only").into(), @@ -879,36 +879,33 @@ where Some(self.blocks.get(params_id)?.clone()) }; - let result = self - .call_manager - .upgrade_actor::(self.actor_id, new_code_cid, params); - - if let Ok(None) = result { - Err(ExecutionError::Abort(Abort::Exit( - ExitCode::OK, - "actor upgraded".to_string(), - NO_DATA_BLOCK_ID, - ))) - } else if let Ok(Some(block)) = result { - let block_id = self - .blocks - .put(block) - .or_fatal() - .context("failed to store a valid return value")?; - Err(ExecutionError::Abort(Abort::Exit( - ExitCode::OK, - "actor upgraded".to_string(), - block_id, - ))) - } else if let Err(ExecutionError::Abort(Abort::Return(Some(block)))) = result { - let block_id = self - .blocks - .put(block) - .or_fatal() - .context("failed to store a valid return value")?; - Ok(block_id) - } else { - Err(result.unwrap_err()) + let result: Result = + self.call_manager + .upgrade_actor::(self.actor_id, new_code_cid, params); + + match result { + Ok(InvocationResult { + exit_code: ExitCode::OK, + value, + }) => { + let block_id = match value { + None => NO_DATA_BLOCK_ID, + Some(block) => self.blocks.put(block).unwrap_or_else(|_| { + log::error!("failed to write to kernel block registry"); + NO_DATA_BLOCK_ID + }), + }; + Err(ExecutionError::Abort(Abort::Exit( + ExitCode::OK, + "actor upgraded".to_string(), + block_id, + ))) + } + Ok(InvocationResult { + exit_code, + value: _, + }) => Ok(exit_code.value()), + Err(err) => Err(err), } } diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index 2d8aaf01a..1147837a6 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -209,7 +209,7 @@ pub trait ActorOps { delegated_address: Option
, ) -> Result<()>; - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result; + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result; /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index e18749f79..1ba75838d 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -4,7 +4,6 @@ use anyhow::{anyhow, Context as _}; use fvm_shared::{sys, ActorID}; use super::Context; -use crate::kernel::BlockId; use crate::kernel::{ClassifyResult, Result}; use crate::{syscall_error, Kernel}; @@ -115,7 +114,7 @@ pub fn upgrade_actor( context: Context<'_, impl Kernel>, new_code_cid_off: u32, params_id: u32, -) -> Result { +) -> Result { let cid = context.memory.read_cid(new_code_cid_off)?; context.kernel.upgrade_actor(cid, params_id) diff --git a/fvm/src/syscalls/error.rs b/fvm/src/syscalls/error.rs index d210a6bd1..ee545272d 100644 --- a/fvm/src/syscalls/error.rs +++ b/fvm/src/syscalls/error.rs @@ -6,7 +6,7 @@ use fvm_shared::error::ExitCode; use wasmtime::Trap; use crate::call_manager::NO_DATA_BLOCK_ID; -use crate::kernel::{Block, BlockId, ExecutionError}; +use crate::kernel::{BlockId, ExecutionError}; /// Represents an actor "abort". #[derive(Debug, thiserror::Error)] @@ -20,9 +20,6 @@ pub enum Abort { /// The system failed with a fatal error. #[error("fatal error: {0}")] Fatal(anyhow::Error), - /// The actor aborted with a block. - #[error("abortive non-local return {0:?}")] - Return(Option), } impl Abort { diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 677f2550c..ccb35b46e 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -398,7 +398,7 @@ impl CallManager for DummyCallManager { _actor_id: ActorID, _new_code_cid: Cid, _params: Option, - ) -> kernel::Result> + ) -> kernel::Result where K: Kernel, { @@ -410,7 +410,7 @@ impl CallManager for DummyCallManager { _actor_id: ActorID, _new_code_cid: Cid, _params: Option, - ) -> kernel::Result> + ) -> kernel::Result where K: Kernel, { diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index 8ca1a5057..265659c1d 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -16,7 +16,6 @@ use num_traits::Zero; #[test] fn upgrade_actor_test() { - // Instantiate tester let mut tester = new_tester( NetworkVersion::V18, StateTreeVersion::V5, @@ -25,33 +24,47 @@ fn upgrade_actor_test() { .unwrap(); let sender: [Account; 2] = tester.create_accounts().unwrap(); - - //let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); + let receiver = Address::new_id(10000); + let state_cid = tester.set_state(&[(); 0]).unwrap(); let wasm_bin = UPGRADE_ACTOR_BINARY; - - // Set actor state - let actor_state = [(); 0]; - let state_cid = tester.set_state(&actor_state).unwrap(); - - // Set actor - let actor_address = Address::new_id(10000); - tester - .set_actor_from_bin(wasm_bin, state_cid, actor_address, TokenAmount::zero()) + .set_actor_from_bin(wasm_bin, state_cid, receiver, TokenAmount::zero()) .unwrap(); - - // Instantiate machine tester.instantiate_machine(DummyExterns).unwrap(); let executor = tester.executor.as_mut().unwrap(); - for (seq, method) in (1..=2).enumerate() { + { + // test a successful call to `upgrade` endpoint + let message = Message { + from: sender[0].1, + to: receiver, + gas_limit: 1000000000, + method_num: 1, + sequence: 0 as u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); + assert_eq!(val, 666); + } + + { let message = Message { - from: sender[seq].1, - to: actor_address, + from: sender[1].1, + to: receiver, gas_limit: 1000000000, - method_num: method, + method_num: 2, sequence: 0 as u64, value: TokenAmount::from_atto(100), ..Message::default() diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index fb364b420..dbe08a79b 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -1,54 +1,56 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT use fvm_ipld_encoding::ipld_block::IpldBlock; -use fvm_ipld_encoding::CBOR; +use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_sdk as sdk; use fvm_shared::address::Address; -use fvm_shared::error::ExitCode; -//use sdk::sys::ErrorNumber; use fvm_shared::upgrade::UpgradeInfo; use serde_tuple::*; -//use sdk::sys::ErrorNumber; -//use fvm_shared::sys::BlockId; - #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] struct SomeStruct { value: u64, } -#[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] -struct SomeReturnStruct { - value: u64, -} +const PARAM_1_VALUE: u64 = 111111; +const PARAM_2_VALUE: u64 = 222222; + +const UPGRADE_FAILED_EXIT_CODE: u32 = 19; #[no_mangle] pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { sdk::initialize(); - // verify that the params we sent from invoke are the same as the params we got here let params = sdk::message::params_raw(params_id).unwrap().unwrap(); - assert_eq!(params.codec, fvm_ipld_encoding::CBOR); - let p = params.deserialize::().unwrap(); - //let p: UpgradeParams = fvm_ipld_encoding::from_slice(params.data.as_slice()).unwrap(); - sdk::debug::log(format!("upgrade:: Param value: {}", p.value).to_string()); - - // verify that the params we sent from invoke are the same as the params we got here let ui_params = sdk::message::params_raw(upgrade_info_id).unwrap().unwrap(); + + assert_eq!(params.codec, fvm_ipld_encoding::CBOR); assert_eq!(ui_params.codec, fvm_ipld_encoding::CBOR); + + let p = params.deserialize::().unwrap(); let ui = ui_params.deserialize::().unwrap(); - //let p: Params = fvm_ipld_encoding::from_slice(msg_params.data.as_slice()).unwrap(); - sdk::debug::log(format!("upgrade: old_code_cid: {}", ui.old_code_cid).to_string()); - if p.value == 10101 { - return 0; - } + sdk::debug::log( + format!( + "[upgrade] value: {}, old_code_cid: {}", + p.value, ui.old_code_cid + ) + .to_string(), + ); - sdk::vm::exit( - 0, - IpldBlock::serialize_cbor(&SomeReturnStruct { value: 2020202 }).unwrap(), - None, - ) + match p.value { + PARAM_1_VALUE => { + sdk::debug::log("returning 0 to mark that the upgrade was successful".to_string()); + sdk::ipld::put_block(CBOR, &to_vec(&666).unwrap()).unwrap() + } + PARAM_2_VALUE => { + sdk::debug::log("calling exit to mark that the upgrade failed".to_string()); + sdk::vm::exit(UPGRADE_FAILED_EXIT_CODE, None, None) + } + _ => { + panic!("unexpected value: {}", p.value); + } + } } #[no_mangle] @@ -59,27 +61,28 @@ pub fn invoke(_: u32) -> u32 { sdk::debug::log(format!("called upgrade_actor with method: {}", method).to_string()); match method { - // test that calling `upgrade_actor` on ourselves results will not return + // test that successful calls to `upgrade_actor` does not return 1 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - let params = IpldBlock::serialize_cbor(&SomeStruct { value: 10101 }).unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { + value: PARAM_1_VALUE, + }) + .unwrap(); let _ = sdk::actor::upgrade_actor(new_code_cid, params); assert!(false, "we should never return from a successful upgrade"); } - // test that when `upgrade_actor` fails we return with an error + // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code 2 => { - sdk::debug::log("test 1".to_string()); let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - sdk::debug::log("test 2".to_string()); - let params = IpldBlock::serialize_cbor(&SomeStruct { value: 10102 }).unwrap(); - sdk::debug::log("test 3".to_string()); - let ret = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); - sdk::debug::log("test 4".to_string()); - let ret_params = sdk::message::params_raw(ret).unwrap().unwrap(); - sdk::debug::log("test 5".to_string()); - let ret_ui = ret_params.deserialize::().unwrap(); - - sdk::debug::log(format!("upgrade_actor returned with: {:?}", ret_ui).to_string()); + let params = IpldBlock::serialize_cbor(&SomeStruct { + value: PARAM_2_VALUE, + }) + .unwrap(); + let exit_code = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); + assert_eq!( + UPGRADE_FAILED_EXIT_CODE, exit_code, + "invalid exit code returned from upgrade_actor" + ); } _ => { sdk::vm::abort( From b26f58964abcf66a303f1480bb2919700a3ac1b8 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Mon, 4 Sep 2023 16:07:55 +0000 Subject: [PATCH 07/40] simplify match arm in CM --- fvm/src/call_manager/default.rs | 33 ++++++++++++----------- testing/integration/tests/upgrade_test.rs | 4 +-- 2 files changed, 20 insertions(+), 17 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 012b4f198..d9a3fff83 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -543,20 +543,25 @@ where let _last_error = invocation_data.last_error; let (cm, block_registry) = invocation_data.kernel.into_inner(); + let result: std::result::Result, Abort> = 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, + ) + })?) + }) + }); + let result: std::result::Result = match result { - Ok(block_id) => { - if block_id == NO_DATA_BLOCK_ID { - Ok(InvocationResult { - exit_code: ExitCode::OK, - value: None, - }) - } else { - Ok(InvocationResult { - exit_code: ExitCode::OK, - value: Some(block_registry.get(block_id).unwrap().clone()), - }) - } - } + Ok(blk) => Ok(InvocationResult { + exit_code: ExitCode::OK, + value: blk.cloned(), + }), Err(abort) => match abort { Abort::Exit(exit_code, _message, _block_id) => Ok(InvocationResult { exit_code, @@ -567,8 +572,6 @@ where }, }; - //let ret: std::result::Result, ExecutionError> = Ok(None); - (result, cm) }) .map_err(ExecutionError::from) diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index 265659c1d..e9b841f5e 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -42,7 +42,7 @@ fn upgrade_actor_test() { to: receiver, gas_limit: 1000000000, method_num: 1, - sequence: 0 as u64, + sequence: 0_u64, value: TokenAmount::from_atto(100), ..Message::default() }; @@ -65,7 +65,7 @@ fn upgrade_actor_test() { to: receiver, gas_limit: 1000000000, method_num: 2, - sequence: 0 as u64, + sequence: 0_u64, value: TokenAmount::from_atto(100), ..Message::default() }; From 85a91296df3a994991de407c23aa21554f5326ba Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 5 Sep 2023 10:32:38 +0000 Subject: [PATCH 08/40] Improve error checking + more tests --- fvm/src/call_manager/default.rs | 25 ++++++++++++---- testing/integration/tests/upgrade_test.rs | 24 ++++++++++++++- .../actors/fil-upgrade-actor/src/actor.rs | 29 ++++++++++--------- 3 files changed, 57 insertions(+), 21 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index d9a3fff83..890c9c152 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -499,12 +499,19 @@ where // Make a store. let mut store = engine.new_store(kernel); + // TODO: a hack until I find a better/simpler way to do this + let mut my_syscall_err: Option = None; + let result: std::result::Result = (|| { // Instantiate the module. let instance = engine .instantiate(&mut store, &new_code_cid)? .context("actor not found") - .map_err(Abort::Fatal)?; + .map_err(|e| { + my_syscall_err = + Some(SyscallError::new(ErrorNumber::NotFound, "actor not found")); + Abort::Fatal(e) + })?; // Resolve and store a reference to the exported memory. let memory = instance @@ -517,7 +524,13 @@ where // Lookup the upgrade method. let upgrade: wasmtime::TypedFunc<(u32, u32), u32> = instance .get_typed_func(&mut store, "upgrade") - .map_err(|e| Abort::Fatal(anyhow!("actor does not support upgrade: {}", e)))?; + .map_err(|e| { + my_syscall_err = Some(SyscallError::new( + ErrorNumber::Forbidden, + "actor does not support upgrade", + )); + Abort::Fatal(e) + })?; // Set the available gas. update_gas_available(&mut store)?; @@ -534,8 +547,6 @@ where // out. charge_for_exec(&mut store)?; - log::info!("returned with: {:?}", res); - Ok(res?) })(); @@ -568,13 +579,15 @@ where value: None, }), Abort::OutOfGas => Err(ExecutionError::OutOfGas), - Abort::Fatal(err) => Err(ExecutionError::Fatal(err)), + Abort::Fatal(err) => match my_syscall_err { + Some(e) => Err(ExecutionError::Syscall(e)), + None => Err(ExecutionError::Fatal(err)), + }, }, }; (result, cm) }) - .map_err(ExecutionError::from) } fn append_event(&mut self, evt: StampedEvent) { diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index e9b841f5e..8cd6df74d 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -23,7 +23,7 @@ fn upgrade_actor_test() { ) .unwrap(); - let sender: [Account; 2] = tester.create_accounts().unwrap(); + let sender: [Account; 3] = tester.create_accounts().unwrap(); let receiver = Address::new_id(10000); let state_cid = tester.set_state(&[(); 0]).unwrap(); @@ -80,4 +80,26 @@ fn upgrade_actor_test() { res.failure_info ); } + + { + let message = Message { + from: sender[2].1, + to: receiver, + gas_limit: 1000000000, + method_num: 3, + sequence: 0_u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } } diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index dbe08a79b..b54bb0d01 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -1,20 +1,20 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT +use cid::multihash::Multihash; +use cid::Cid; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_sdk as sdk; use fvm_shared::address::Address; +use fvm_shared::error::ErrorNumber; use fvm_shared::upgrade::UpgradeInfo; +use fvm_shared::IDENTITY_HASH; use serde_tuple::*; - #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] struct SomeStruct { value: u64, } -const PARAM_1_VALUE: u64 = 111111; -const PARAM_2_VALUE: u64 = 222222; - const UPGRADE_FAILED_EXIT_CODE: u32 = 19; #[no_mangle] @@ -39,11 +39,11 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { ); match p.value { - PARAM_1_VALUE => { + 1 => { sdk::debug::log("returning 0 to mark that the upgrade was successful".to_string()); sdk::ipld::put_block(CBOR, &to_vec(&666).unwrap()).unwrap() } - PARAM_2_VALUE => { + 2 => { sdk::debug::log("calling exit to mark that the upgrade failed".to_string()); sdk::vm::exit(UPGRADE_FAILED_EXIT_CODE, None, None) } @@ -64,26 +64,27 @@ pub fn invoke(_: u32) -> u32 { // test that successful calls to `upgrade_actor` does not return 1 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - let params = IpldBlock::serialize_cbor(&SomeStruct { - value: PARAM_1_VALUE, - }) - .unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 1 }).unwrap(); let _ = sdk::actor::upgrade_actor(new_code_cid, params); assert!(false, "we should never return from a successful upgrade"); } // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code 2 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - let params = IpldBlock::serialize_cbor(&SomeStruct { - value: PARAM_2_VALUE, - }) - .unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 2 }).unwrap(); let exit_code = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); assert_eq!( UPGRADE_FAILED_EXIT_CODE, exit_code, "invalid exit code returned from upgrade_actor" ); } + // test that providing invalid new_code_cid returns a NotFound error + 3 => { + let new_code_cid = + Cid::new_v1(0x55, Multihash::wrap(IDENTITY_HASH, b"test123").unwrap()); + let res = sdk::actor::upgrade_actor(new_code_cid, None); + assert_eq!(res, Err(ErrorNumber::NotFound)); + } _ => { sdk::vm::abort( fvm_shared::error::ExitCode::FIRST_USER_EXIT_CODE, From aead4198d89139b59defbf077acafcec195da740 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 5 Sep 2023 15:01:42 +0000 Subject: [PATCH 09/40] Detect re-entrant upgrade --- fvm/src/call_manager/default.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 890c9c152..70e65a558 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -422,15 +422,31 @@ where new_code_cid: Cid, params: Option, ) -> Result { - let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; - let origin = self.origin; + let state = self .state_tree_mut() .get_actor(actor_id)? .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; + // store the code cid of the calling actor before running the upgrade endpoint + // in case it was changed (which could happen if the target upgrade endpoint + // sent a message to this actor which in turn called upgrade) + let code_id_before = state.code; + + let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; + if result.exit_code == ExitCode::OK { + // after running the upgrade, our code cid must not have changed + let code_id_after = self + .state_tree_mut() + .get_actor(actor_id)? + .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))? + .code; + if code_id_before != code_id_after { + return Err(syscall_error!(Forbidden; "re-entrant upgrade detected").into()); + } + self.state_tree_mut().set_actor( origin, ActorState::new( From 60ef934416790612a2f396698244646453606c85 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 5 Sep 2023 17:37:44 +0000 Subject: [PATCH 10/40] Recursively calling upgrade returns correct block --- fvm/src/call_manager/default.rs | 38 ++++++++++--------- testing/integration/tests/upgrade_test.rs | 27 ++++++++++++- .../actors/fil-upgrade-actor/src/actor.rs | 21 ++++++++++ 3 files changed, 67 insertions(+), 19 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 70e65a558..f8d00fd91 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -570,30 +570,32 @@ where let _last_error = invocation_data.last_error; let (cm, block_registry) = invocation_data.kernel.into_inner(); - let result: std::result::Result, Abort> = 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, - ) - })?) - }) - }); - let result: std::result::Result = match result { - Ok(blk) => Ok(InvocationResult { + Ok(NO_DATA_BLOCK_ID) => Ok(InvocationResult { exit_code: ExitCode::OK, - value: blk.cloned(), + value: None, }), + 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) => match abort { - Abort::Exit(exit_code, _message, _block_id) => Ok(InvocationResult { - exit_code, + Abort::Exit(code, _message, NO_DATA_BLOCK_ID) => Ok(InvocationResult { + exit_code: code, value: None, }), + Abort::Exit(exit_code, _message, block_id) => { + match block_registry.get(block_id) { + Ok(blk) => Ok(InvocationResult { + exit_code: exit_code, + value: Some(blk.clone()), + }), + Err(e) => Err(ExecutionError::Fatal(anyhow!(e))), + } + } Abort::OutOfGas => Err(ExecutionError::OutOfGas), Abort::Fatal(err) => match my_syscall_err { Some(e) => Err(ExecutionError::Syscall(e)), diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index 8cd6df74d..ed5e6f5ca 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -23,7 +23,7 @@ fn upgrade_actor_test() { ) .unwrap(); - let sender: [Account; 3] = tester.create_accounts().unwrap(); + let sender: [Account; 4] = tester.create_accounts().unwrap(); let receiver = Address::new_id(10000); let state_cid = tester.set_state(&[(); 0]).unwrap(); @@ -102,4 +102,29 @@ fn upgrade_actor_test() { res.failure_info ); } + + { + let message = Message { + from: sender[3].1, + to: receiver, + gas_limit: 1000000000, + method_num: 4, + sequence: 0_u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + + let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); + assert_eq!(val, 444); + + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } } diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index b54bb0d01..ee8b380d2 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -47,6 +47,18 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { sdk::debug::log("calling exit to mark that the upgrade failed".to_string()); sdk::vm::exit(UPGRADE_FAILED_EXIT_CODE, None, None) } + 3 => { + sdk::debug::log("calling upgrade within an upgrade".to_string()); + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 4 }).unwrap(); + let _ = sdk::actor::upgrade_actor(new_code_cid, params); + assert!(false, "we should never return from a successful upgrade"); + 0 + } + 4 => { + sdk::debug::log("inside upgrade within an upgrade".to_string()); + sdk::ipld::put_block(CBOR, &to_vec(&444).unwrap()).unwrap() + } _ => { panic!("unexpected value: {}", p.value); } @@ -85,6 +97,15 @@ pub fn invoke(_: u32) -> u32 { let res = sdk::actor::upgrade_actor(new_code_cid, None); assert_eq!(res, Err(ErrorNumber::NotFound)); } + // test recursive updare + 4 => { + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 3 }).unwrap(); + sdk::debug::log("upgrade 1".to_string()); + let res = sdk::actor::upgrade_actor(new_code_cid, params); + sdk::debug::log("upgrade 2".to_string()); + assert_eq!(res, Err(ErrorNumber::Forbidden)); + } _ => { sdk::vm::abort( fvm_shared::error::ExitCode::FIRST_USER_EXIT_CODE, From b7a0cb04746e8f23aa68f88405b6dcd05cf2494f Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 5 Sep 2023 17:39:39 +0000 Subject: [PATCH 11/40] fix lints --- fvm/src/call_manager/default.rs | 2 +- .../actors/fil-upgrade-actor/src/actor.rs | 18 +++++++----------- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index f8d00fd91..d46755861 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -590,7 +590,7 @@ where Abort::Exit(exit_code, _message, block_id) => { match block_registry.get(block_id) { Ok(blk) => Ok(InvocationResult { - exit_code: exit_code, + exit_code, value: Some(blk.clone()), }), Err(e) => Err(ExecutionError::Fatal(anyhow!(e))), diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index ee8b380d2..2942ccabf 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -30,13 +30,10 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { let p = params.deserialize::().unwrap(); let ui = ui_params.deserialize::().unwrap(); - sdk::debug::log( - format!( - "[upgrade] value: {}, old_code_cid: {}", - p.value, ui.old_code_cid - ) - .to_string(), - ); + sdk::debug::log(format!( + "[upgrade] value: {}, old_code_cid: {}", + p.value, ui.old_code_cid + )); match p.value { 1 => { @@ -52,8 +49,7 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 4 }).unwrap(); let _ = sdk::actor::upgrade_actor(new_code_cid, params); - assert!(false, "we should never return from a successful upgrade"); - 0 + unreachable!("we should never return from a successful upgrade"); } 4 => { sdk::debug::log("inside upgrade within an upgrade".to_string()); @@ -70,7 +66,7 @@ pub fn invoke(_: u32) -> u32 { sdk::initialize(); let method = sdk::message::method_number(); - sdk::debug::log(format!("called upgrade_actor with method: {}", method).to_string()); + sdk::debug::log(format!("called upgrade_actor with method: {}", method)); match method { // test that successful calls to `upgrade_actor` does not return @@ -78,7 +74,7 @@ pub fn invoke(_: u32) -> u32 { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 1 }).unwrap(); let _ = sdk::actor::upgrade_actor(new_code_cid, params); - assert!(false, "we should never return from a successful upgrade"); + unreachable!("we should never return from a successful upgrade"); } // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code 2 => { From d2195977e151b1fbb53a2206d4220c76ea53311a Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Wed, 6 Sep 2023 15:32:55 +0000 Subject: [PATCH 12/40] Add Entrypoint to send and refactor upgrade_actor to call send --- fvm/src/call_manager/backtrace.rs | 10 +- fvm/src/call_manager/default.rs | 321 ++++++------------ fvm/src/call_manager/mod.rs | 32 +- fvm/src/executor/default.rs | 26 +- fvm/src/kernel/default.rs | 22 +- fvm/src/trace/mod.rs | 5 +- fvm/tests/dummy.rs | 24 +- shared/src/lib.rs | 2 - testing/integration/tests/upgrade_test.rs | 24 +- .../actors/fil-upgrade-actor/src/actor.rs | 68 ++-- 10 files changed, 202 insertions(+), 332 deletions(-) diff --git a/fvm/src/call_manager/backtrace.rs b/fvm/src/call_manager/backtrace.rs index 12bf3c425..2f59a2419 100644 --- a/fvm/src/call_manager/backtrace.rs +++ b/fvm/src/call_manager/backtrace.rs @@ -4,10 +4,12 @@ use std::fmt::Display; use fvm_shared::address::Address; use fvm_shared::error::{ErrorNumber, ExitCode}; -use fvm_shared::{ActorID, MethodNum}; +use fvm_shared::ActorID; use crate::kernel::SyscallError; +use super::Entrypoint; + /// A call backtrace records the actors an error was propagated through, from /// the moment it was emitted. The original error is the _cause_. Backtraces are /// useful for identifying the root cause of an error. @@ -76,8 +78,8 @@ impl Backtrace { pub struct Frame { /// The actor that exited with this code. pub source: ActorID, - /// The method that was invoked. - pub method: MethodNum, + /// The entrypoint that was invoked. + pub entrypoint: Entrypoint, /// The exit code. pub code: ExitCode, /// The abort message. @@ -90,7 +92,7 @@ impl Display for Frame { f, "{} (method {}) -- {} ({})", Address::new_id(self.source), - self.method, + self.entrypoint, &self.message, self.code, ) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index d46755861..efdcc50cf 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -12,12 +12,11 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; -use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum, METHOD_SEND}; use num_traits::Zero; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; -use super::{Backtrace, CallManager, InvocationResult, NO_DATA_BLOCK_ID}; +use super::{Backtrace, CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; use crate::blockstore::DiscardBlockstore; use crate::call_manager::backtrace::Frame; use crate::call_manager::FinishRet; @@ -171,7 +170,7 @@ where &mut self, from: ActorID, to: Address, - method: MethodNum, + entrypoint: Entrypoint, params: Option, value: &TokenAmount, gas_limit: Option, @@ -184,7 +183,7 @@ where self.trace(ExecutionEvent::Call { from, to, - method, + entrypoint, params: params.as_ref().map(Into::into), value: value.clone(), gas_limit: std::cmp::min( @@ -200,44 +199,13 @@ where self.gas_tracker.push_limit(limit); } - if self.call_stack_depth >= self.machine.context().max_call_depth { - let sys_err = syscall_error!(LimitExceeded, "message execution exceeds call depth"); - if self.machine.context().tracing { - self.trace(ExecutionEvent::CallError(sys_err.clone())); - } - return Err(sys_err.into()); - } - - self.state_tree_mut().begin_transaction(); - self.events.begin_transaction(); - self.state_access_tracker.begin_transaction(); - self.call_stack_depth += 1; - - let (revert, mut result) = match <::Limiter>::with_stack_frame( - self, - |s| s.limiter_mut(), - |s| s.send_unchecked::(from, to, method, params, value, read_only), - ) { - Ok(v) => (!v.exit_code.is_success(), Ok(v)), - Err(e) => (true, Err(e)), - }; + let mut result = self.with_stack_frame(|s| { + s.send_unchecked::(from, to, entrypoint, params, value, read_only) + }); - self.call_stack_depth -= 1; - // Return the _first_ error (if any). We don't expect any errors here anyways as all error - // cases are fatal. - if let Some(err) = [ - // End all transactions - self.state_access_tracker.end_transaction(revert).err(), - self.events.end_transaction(revert).err(), - self.state_tree_mut().end_transaction(revert).err(), - // If we pushed a gas limit, pop it. - gas_limit.and_then(|_| self.gas_tracker.pop_limit().err()), - ] - .into_iter() - .flatten() // Iterator> -> Iterator - .next() - { - return Err(err); + // If we pushed a limit, pop it. + if gas_limit.is_some() { + self.gas_tracker.pop_limit()?; } // If we're not out of gas but the error is "out of gas" (e.g., due to a gas limit), replace @@ -272,6 +240,26 @@ where result } + fn with_transaction( + &mut self, + f: impl FnOnce(&mut Self) -> Result, + ) -> Result { + self.state_tree_mut().begin_transaction(); + self.events.begin_transaction(); + self.state_access_tracker.begin_transaction(); + + let (revert, res) = match f(self) { + Ok(v) => (!v.exit_code.is_success(), Ok(v)), + Err(e) => (true, Err(e)), + }; + + self.state_tree_mut().end_transaction(revert)?; + self.events.end_transaction(revert)?; + self.state_access_tracker.end_transaction(revert)?; + + res + } + fn finish(mut self) -> (Result, Self::Machine) { let InnerDefaultCallManager { machine, @@ -432,182 +420,46 @@ where // store the code cid of the calling actor before running the upgrade endpoint // in case it was changed (which could happen if the target upgrade endpoint // sent a message to this actor which in turn called upgrade) - let code_id_before = state.code; + let code = state.code; - let result = self.upgrade_actor_inner::(actor_id, new_code_cid, params)?; + // update the code cid of the actor to new_code_cid + self.state_tree_mut().set_actor( + origin, + ActorState::new( + new_code_cid, + state.state, + state.balance, + state.sequence, + None, + ), + ); + + // run the upgrade endpoint + let result = self.send::( + actor_id, + Address::new_id(actor_id), + Entrypoint::Upgrade, + params, + &TokenAmount::zero(), + None, + false, + )?; if result.exit_code == ExitCode::OK { // after running the upgrade, our code cid must not have changed - let code_id_after = self + let code_after_upgrade = self .state_tree_mut() .get_actor(actor_id)? .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))? .code; - if code_id_before != code_id_after { + if code != code_after_upgrade { return Err(syscall_error!(Forbidden; "re-entrant upgrade detected").into()); } - - self.state_tree_mut().set_actor( - origin, - ActorState::new( - new_code_cid, - state.state, - state.balance, - state.sequence, - None, - ), - ); } Ok(result) } - fn upgrade_actor_inner>( - &mut self, - actor_id: ActorID, - new_code_cid: Cid, - params: Option, - ) -> Result { - let state = self - .state_tree() - .get_actor(actor_id)? - .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; - - let mut block_registry = BlockRegistry::new(); - - // add the params block to the block registry - let params_id = if let Some(blk) = params { - block_registry.put(blk)? - } else { - NO_DATA_BLOCK_ID - }; - - // also add a new block to the registry which includes the UpgradeInfo which - // we pass to the actor's upgrade method - let ui = UpgradeInfo { - old_code_cid: state.code, - }; - let ui_params = to_vec(&ui).map_err( - |e| syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), - )?; - let ui_params_id = block_registry.put(Block::new(CBOR, ui_params))?; - - log::trace!( - "upgrading {} from {} to {}", - actor_id, - state.code, - new_code_cid - ); - self.map_mut(|cm| { - let engine = cm.engine.clone(); // reference the RC. - - // Make the kernel. - let kernel = K::new( - cm, - block_registry, - actor_id, - actor_id, - 1919, - TokenAmount::from_atto(0), - false, - ); - - // Make a store. - let mut store = engine.new_store(kernel); - - // TODO: a hack until I find a better/simpler way to do this - let mut my_syscall_err: Option = None; - - let result: std::result::Result = (|| { - // Instantiate the module. - let instance = engine - .instantiate(&mut store, &new_code_cid)? - .context("actor not found") - .map_err(|e| { - my_syscall_err = - Some(SyscallError::new(ErrorNumber::NotFound, "actor not found")); - Abort::Fatal(e) - })?; - - // Resolve and store a reference to the exported memory. - let memory = instance - .get_memory(&mut store, "memory") - .context("actor has no memory export") - .map_err(Abort::Fatal)?; - - store.data_mut().memory = memory; - - // Lookup the upgrade method. - let upgrade: wasmtime::TypedFunc<(u32, u32), u32> = instance - .get_typed_func(&mut store, "upgrade") - .map_err(|e| { - my_syscall_err = Some(SyscallError::new( - ErrorNumber::Forbidden, - "actor does not support upgrade", - )); - Abort::Fatal(e) - })?; - - // Set the available gas. - update_gas_available(&mut store)?; - - // Invoke it. - let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - upgrade.call(&mut store, (params_id, ui_params_id)) - })) - .map_err(|panic| { - Abort::Fatal(anyhow!("panic within actor upgrade: {:?}", panic)) - })?; - - // Charge for any remaining uncharged execution gas, returning an error if we run - // out. - charge_for_exec(&mut store)?; - - Ok(res?) - })(); - - let invocation_data = store.into_data(); - let _last_error = invocation_data.last_error; - let (cm, block_registry) = invocation_data.kernel.into_inner(); - - let result: std::result::Result = match result { - Ok(NO_DATA_BLOCK_ID) => Ok(InvocationResult { - exit_code: ExitCode::OK, - value: None, - }), - 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) => match abort { - Abort::Exit(code, _message, NO_DATA_BLOCK_ID) => Ok(InvocationResult { - exit_code: code, - value: None, - }), - Abort::Exit(exit_code, _message, block_id) => { - match block_registry.get(block_id) { - Ok(blk) => Ok(InvocationResult { - exit_code, - value: Some(blk.clone()), - }), - Err(e) => Err(ExecutionError::Fatal(anyhow!(e))), - } - } - Abort::OutOfGas => Err(ExecutionError::OutOfGas), - Abort::Fatal(err) => match my_syscall_err { - Some(e) => Err(ExecutionError::Syscall(e)), - None => Err(ExecutionError::Fatal(err)), - }, - }, - }; - - (result, cm) - }) - } - fn append_event(&mut self, evt: StampedEvent) { self.events.append_event(evt) } @@ -777,8 +629,8 @@ where self.send_resolved::( system_actor::SYSTEM_ACTOR_ID, id, - fvm_shared::METHOD_CONSTRUCTOR, - Some(Block::new(CBOR, params, Vec::new())), + Entrypoint::Invoke(fvm_shared::METHOD_CONSTRUCTOR), + Some(Block::new(CBOR, params)), &TokenAmount::zero(), false, )?; @@ -800,7 +652,7 @@ where &mut self, from: ActorID, to: Address, - method: MethodNum, + entrypoint: Entrypoint, params: Option, value: &TokenAmount, read_only: bool, @@ -834,7 +686,7 @@ where }, }; - self.send_resolved::(from, to, method, params, value, read_only) + self.send_resolved::(from, to, entrypoint, params, value, read_only) } /// Send with resolved addresses. @@ -842,7 +694,7 @@ where &mut self, from: ActorID, to: ActorID, - method: MethodNum, + entrypoint: Entrypoint, params: Option, value: &TokenAmount, read_only: bool, @@ -867,7 +719,7 @@ where } // Abort early if we have a send. - if method == METHOD_SEND { + if let Entrypoint::Invoke(METHOD_SEND) = entrypoint { log::trace!("sent {} -> {}: {}", from, to, &value); return Ok(InvocationResult::default()); } @@ -903,7 +755,7 @@ where |_| syscall_error!(NotFound; "actor code cid does not exist {}", &state.code), )?; - log::trace!("calling {} -> {}::{}", from, to, method); + log::trace!("calling {} -> {}::{}", from, to, entrypoint); self.map_mut(|cm| { let engine = cm.engine.clone(); // reference the RC. @@ -913,7 +765,7 @@ where block_registry, from, to, - method, + entrypoint.method_num(), value.clone(), read_only, ); @@ -939,7 +791,7 @@ where // Lookup the invoke method. let invoke: wasmtime::TypedFunc<(u32,), u32> = instance - .get_typed_func(&mut store, "invoke") + .get_typed_func(&mut store, entrypoint.func_name()) // All actors will have an invoke method. .map_err(Abort::Fatal)?; @@ -1033,7 +885,7 @@ where cm.backtrace.push_frame(Frame { source: to, - method, + entrypoint, message, code, }); @@ -1068,11 +920,11 @@ where Ok(val) => log::trace!( "returning {}::{} -> {} ({})", to, - method, + entrypoint, from, val.exit_code ), - Err(e) => log::trace!("failing {}::{} -> {} (err:{})", to, method, from, e), + Err(e) => log::trace!("failing {}::{} -> {} (err:{})", to, entrypoint, from, e), } } @@ -1090,6 +942,31 @@ where { replace_with::replace_with_and_return(self, || DefaultCallManager(None), f) } + + /// Check that we're not violating the call stack depth, then envelope a call + /// with an increase/decrease of the depth to make sure none of them are missed. + fn with_stack_frame(&mut self, f: F) -> Result + where + F: FnOnce(&mut Self) -> Result, + { + if self.call_stack_depth >= self.machine.context().max_call_depth { + let sys_err = syscall_error!(LimitExceeded, "message execution exceeds call depth"); + if self.machine.context().tracing { + self.trace(ExecutionEvent::CallError(sys_err.clone())); + } + return Err(sys_err.into()); + } + + self.call_stack_depth += 1; + let res = + << as CallManager>::Machine as Machine>::Limiter>::with_stack_frame( + self, + |s| s.limiter_mut(), + f, + ); + self.call_stack_depth -= 1; + res + } } /// Stores events in layers as they are emitted by actors. As the call stack progresses, when an @@ -1166,3 +1043,19 @@ impl EventsAccumulator { }) } } + +impl Entrypoint { + fn method_num(&self) -> MethodNum { + match self { + Entrypoint::Invoke(num) => *num, + Entrypoint::Upgrade => 191919, + } + } + + fn func_name(&self) -> &'static str { + match self { + Entrypoint::Invoke(_) => "invoke", + Entrypoint::Upgrade => "upgrade", + } + } +} diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index a1755eb59..4706b8ee6 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -67,13 +67,19 @@ pub trait CallManager: 'static { &mut self, from: ActorID, to: Address, - method: MethodNum, + entrypoint: Entrypoint, params: Option, value: &TokenAmount, gas_limit: Option, read_only: bool, ) -> Result; + /// Execute some operation (usually a send) within a transaction. + fn with_transaction( + &mut self, + f: impl FnOnce(&mut Self) -> Result, + ) -> Result; + /// Finishes execution, returning the gas used, machine, and exec trace if requested. fn finish(self) -> (Result, Self::Machine); @@ -120,15 +126,6 @@ pub trait CallManager: 'static { where K: Kernel; - fn upgrade_actor_inner( - &mut self, - actor_id: ActorID, - new_code_cid: Cid, - params: Option, - ) -> Result - where - K: Kernel; - /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; @@ -209,3 +206,18 @@ pub struct FinishRet { pub events: Vec, pub events_root: Option, } + +#[derive(Clone, Debug, Copy)] +pub enum Entrypoint { + Invoke(MethodNum), + Upgrade, +} + +impl std::fmt::Display for Entrypoint { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Entrypoint::Invoke(method) => write!(f, "invoke({})", method), + Entrypoint::Upgrade => write!(f, "upgrade"), + } + } +} diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 32b00d37f..3b5b60352 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -16,7 +16,7 @@ use fvm_shared::{ActorID, IPLD_RAW, METHOD_SEND}; use num_traits::Zero; use super::{ApplyFailure, ApplyKind, ApplyRet, Executor}; -use crate::call_manager::{backtrace, Backtrace, CallManager, InvocationResult}; +use crate::call_manager::{backtrace, Backtrace, CallManager, Entrypoint, InvocationResult}; use crate::eam_actor::EAM_ACTOR_ID; use crate::engine::EnginePool; use crate::gas::{Gas, GasCharge, GasOutputs}; @@ -141,17 +141,19 @@ where ) }); - // Invoke the message. We charge for the return value internally if the call-stack depth - // is 1. - let result = cm.send::( - sender_id, - msg.to, - msg.method_num, - params, - &msg.value, - None, - false, - ); + let result = cm.with_transaction(|cm| { + // Invoke the message. We charge for the return value internally if the call-stack depth + // is 1. + cm.send::( + sender_id, + msg.to, + Entrypoint::Invoke(msg.method_num), + params, + &msg.value, + None, + false, + ) + }); let (res, machine) = match cm.finish() { (Ok(res), machine) => (res, machine), diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 5f26f76cd..2483acf8c 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -30,7 +30,7 @@ use super::blocks::{Block, BlockRegistry}; use super::error::Result; use super::hash::SupportedHashes; use super::*; -use crate::call_manager::{CallManager, InvocationResult, NO_DATA_BLOCK_ID}; +use crate::call_manager::{CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; use crate::externs::{Chain, Consensus, Rand}; use crate::gas::GasTimer; use crate::init_actor::INIT_ACTOR_ID; @@ -138,9 +138,17 @@ where } // Send. - let result = self.call_manager.send::( - from, *recipient, method, params, value, gas_limit, read_only, - )?; + let result = self.call_manager.with_transaction(|cm| { + cm.send::( + from, + *recipient, + Entrypoint::Invoke(method), + params, + value, + gas_limit, + read_only, + ) + })?; // Store result and return. Ok(match result { @@ -879,9 +887,9 @@ where Some(self.blocks.get(params_id)?.clone()) }; - let result: Result = - self.call_manager - .upgrade_actor::(self.actor_id, new_code_cid, params); + let result = self + .call_manager + .with_transaction(|cm| cm.upgrade_actor::(self.actor_id, new_code_cid, params)); match result { Ok(InvocationResult { diff --git a/fvm/src/trace/mod.rs b/fvm/src/trace/mod.rs index 745410773..796123f43 100644 --- a/fvm/src/trace/mod.rs +++ b/fvm/src/trace/mod.rs @@ -4,8 +4,9 @@ use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::Address; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; -use fvm_shared::{ActorID, MethodNum}; +use fvm_shared::ActorID; +use crate::call_manager::Entrypoint; use crate::gas::GasCharge; use crate::kernel::SyscallError; use crate::Cid; @@ -25,7 +26,7 @@ pub enum ExecutionEvent { Call { from: ActorID, to: Address, - method: MethodNum, + entrypoint: Entrypoint, params: Option, value: TokenAmount, gas_limit: u64, diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index ccb35b46e..71fab3141 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -6,7 +6,7 @@ use std::rc::Rc; use anyhow::Context; use cid::Cid; -use fvm::call_manager::{Backtrace, CallManager, FinishRet, InvocationResult}; +use fvm::call_manager::{Backtrace, CallManager, Entrypoint, FinishRet, InvocationResult}; use fvm::engine::Engine; use fvm::externs::{Chain, Consensus, Externs, Rand}; use fvm::gas::{Gas, GasCharge, GasTimer, GasTracker}; @@ -278,7 +278,7 @@ impl CallManager for DummyCallManager { &mut self, _from: fvm_shared::ActorID, _to: Address, - _method: fvm_shared::MethodNum, + _entrypoint: Entrypoint, _params: Option, _value: &fvm_shared::econ::TokenAmount, _gas_limit: Option, @@ -288,6 +288,14 @@ impl CallManager for DummyCallManager { todo!() } + fn with_transaction( + &mut self, + _f: impl FnOnce(&mut Self) -> kernel::Result, + ) -> kernel::Result { + // Ok(InvocationResult::Return(None)) + todo!() + } + fn finish(self) -> (kernel::Result, Self::Machine) { ( Ok(FinishRet { @@ -404,16 +412,4 @@ impl CallManager for DummyCallManager { { todo!() } - - fn upgrade_actor_inner( - &mut self, - _actor_id: ActorID, - _new_code_cid: Cid, - _params: Option, - ) -> kernel::Result - where - K: Kernel, - { - todo!() - } } diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 060c795b2..1b31f717a 100644 --- a/shared/src/lib.rs +++ b/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; -/// Upgraded actor constructor method. -pub const METHOD_UPGRADE: MethodNum = 191919; /// The outcome of a `Send`, covering its ExitCode and optional return data #[derive(Debug, PartialEq, Eq, Clone)] diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index ed5e6f5ca..b010d0f59 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -23,7 +23,7 @@ fn upgrade_actor_test() { ) .unwrap(); - let sender: [Account; 4] = tester.create_accounts().unwrap(); + let sender: [Account; 3] = tester.create_accounts().unwrap(); let receiver = Address::new_id(10000); let state_cid = tester.set_state(&[(); 0]).unwrap(); @@ -92,28 +92,6 @@ fn upgrade_actor_test() { ..Message::default() }; - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - } - - { - let message = Message { - from: sender[3].1, - to: receiver, - gas_limit: 1000000000, - method_num: 4, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - let res = executor .execute_message(message, ApplyKind::Explicit, 100) .unwrap(); diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index 2942ccabf..48fff8c89 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -1,14 +1,9 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT -use cid::multihash::Multihash; -use cid::Cid; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_sdk as sdk; use fvm_shared::address::Address; -use fvm_shared::error::ErrorNumber; -use fvm_shared::upgrade::UpgradeInfo; -use fvm_shared::IDENTITY_HASH; use serde_tuple::*; #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] struct SomeStruct { @@ -18,45 +13,42 @@ struct SomeStruct { const UPGRADE_FAILED_EXIT_CODE: u32 = 19; #[no_mangle] -pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { +pub fn upgrade(params_id: u32) -> u32 { sdk::initialize(); let params = sdk::message::params_raw(params_id).unwrap().unwrap(); - let ui_params = sdk::message::params_raw(upgrade_info_id).unwrap().unwrap(); - assert_eq!(params.codec, fvm_ipld_encoding::CBOR); - assert_eq!(ui_params.codec, fvm_ipld_encoding::CBOR); - - let p = params.deserialize::().unwrap(); - let ui = ui_params.deserialize::().unwrap(); - sdk::debug::log(format!( - "[upgrade] value: {}, old_code_cid: {}", - p.value, ui.old_code_cid - )); - - match p.value { + match params.deserialize::().unwrap().value { 1 => { - sdk::debug::log("returning 0 to mark that the upgrade was successful".to_string()); - sdk::ipld::put_block(CBOR, &to_vec(&666).unwrap()).unwrap() + let block_id = sdk::ipld::put_block(CBOR, &to_vec(&666).unwrap()).unwrap(); + sdk::debug::log(format!( + "[upgrade] params:1, returning block_id {}", + block_id + )); + block_id } 2 => { - sdk::debug::log("calling exit to mark that the upgrade failed".to_string()); + sdk::debug::log("[upgrade] params:2, calling sdk::vm::exit()".to_string()); sdk::vm::exit(UPGRADE_FAILED_EXIT_CODE, None, None) } 3 => { - sdk::debug::log("calling upgrade within an upgrade".to_string()); + sdk::debug::log("[upgrade] params:3, calling upgrade within an upgrade".to_string()); let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 4 }).unwrap(); let _ = sdk::actor::upgrade_actor(new_code_cid, params); unreachable!("we should never return from a successful upgrade"); } 4 => { - sdk::debug::log("inside upgrade within an upgrade".to_string()); - sdk::ipld::put_block(CBOR, &to_vec(&444).unwrap()).unwrap() + let block_id = sdk::ipld::put_block(CBOR, &to_vec(&444).unwrap()).unwrap(); + sdk::debug::log(format!( + "[upgrade] params:4, inside upgrade within an upgrade, returning block_id {}", + block_id + )); + block_id } - _ => { - panic!("unexpected value: {}", p.value); + other => { + panic!("unexpected value: {}", other); } } } @@ -65,10 +57,7 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { pub fn invoke(_: u32) -> u32 { sdk::initialize(); - let method = sdk::message::method_number(); - sdk::debug::log(format!("called upgrade_actor with method: {}", method)); - - match method { + match sdk::message::method_number() { // test that successful calls to `upgrade_actor` does not return 1 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); @@ -86,26 +75,17 @@ pub fn invoke(_: u32) -> u32 { "invalid exit code returned from upgrade_actor" ); } - // test that providing invalid new_code_cid returns a NotFound error + // test recursive update 3 => { - let new_code_cid = - Cid::new_v1(0x55, Multihash::wrap(IDENTITY_HASH, b"test123").unwrap()); - let res = sdk::actor::upgrade_actor(new_code_cid, None); - assert_eq!(res, Err(ErrorNumber::NotFound)); - } - // test recursive updare - 4 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 3 }).unwrap(); - sdk::debug::log("upgrade 1".to_string()); - let res = sdk::actor::upgrade_actor(new_code_cid, params); - sdk::debug::log("upgrade 2".to_string()); - assert_eq!(res, Err(ErrorNumber::Forbidden)); + let _ = sdk::actor::upgrade_actor(new_code_cid, params); + unreachable!("we should never return from a successful upgrade"); } - _ => { + other => { sdk::vm::abort( fvm_shared::error::ExitCode::FIRST_USER_EXIT_CODE, - Some(format!("bad method {}", method).as_str()), + Some(format!("unexpected method {}", other).as_str()), ); } } From 6d287376dd079da10a46c2315bd8ebaf080cbca8 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 8 Sep 2023 14:56:48 +0000 Subject: [PATCH 13/40] Support entrypoints with different number of params --- fvm/src/call_manager/default.rs | 120 ++++++++++++------ fvm/src/call_manager/mod.rs | 5 +- fvm/src/syscalls/error.rs | 3 + shared/src/lib.rs | 2 + shared/src/upgrade/mod.rs | 2 +- .../actors/fil-upgrade-actor/src/actor.rs | 16 ++- 6 files changed, 103 insertions(+), 45 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index efdcc50cf..fe77e9a31 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -12,6 +12,7 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; +use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum, METHOD_SEND}; use num_traits::Zero; @@ -417,8 +418,8 @@ where .get_actor(actor_id)? .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; - // store the code cid of the calling actor before running the upgrade endpoint - // in case it was changed (which could happen if the target upgrade endpoint + // store the code cid of the calling actor before running the upgrade entrypoint + // in case it was changed (which could happen if the target upgrade entrypoint // sent a message to this actor which in turn called upgrade) let code = state.code; @@ -434,11 +435,11 @@ where ), ); - // run the upgrade endpoint + // run the upgrade entrypoint let result = self.send::( actor_id, Address::new_id(actor_id), - Entrypoint::Upgrade, + Entrypoint::Upgrade(UpgradeInfo { old_code_cid: code }), params, &TokenAmount::zero(), None, @@ -742,6 +743,11 @@ where NO_DATA_BLOCK_ID }; + // additional_params takes care of adding entrypoint specific params to the block + // registry and passing them to wasmtime + let mut additional_params = EntrypointParams::new(entrypoint); + additional_params.maybe_put_registry(&mut block_registry)?; + // Increment invocation count self.invocation_count += 1; @@ -789,20 +795,21 @@ where store.data_mut().memory = memory; - // Lookup the invoke method. - let invoke: wasmtime::TypedFunc<(u32,), u32> = instance - .get_typed_func(&mut store, entrypoint.func_name()) - // All actors will have an invoke method. - .map_err(Abort::Fatal)?; + let func = match instance.get_func(&mut store, entrypoint.func_name()) { + Some(func) => func, + None => { + return Err(Abort::EntrypointNotFound); + } + }; + + let mut params = vec![wasmtime::Val::I32(params_id as i32)]; + params.extend_from_slice(additional_params.params().as_slice()); // Set the available gas. update_gas_available(&mut store)?; - // Invoke it. - let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - invoke.call(&mut store, (params_id,)) - })) - .map_err(|panic| Abort::Fatal(anyhow!("panic within actor: {:?}", panic)))?; + let mut out = [wasmtime::Val::I32(0)]; + func.call(&mut store, params.as_slice(), &mut out)?; // Charge for any remaining uncharged execution gas, returning an error if we run // out. @@ -812,35 +819,26 @@ where // detected it and returned OutOfGas above. Any other invocation failure is returned // here as an Abort - Ok(res?) + Ok(out[0].unwrap_i32() as u32) })(); let invocation_data = store.into_data(); 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(ret) => Ok(InvocationResult { + Ok(NO_DATA_BLOCK_ID) => Ok(InvocationResult { exit_code: ExitCode::OK, - value: ret.cloned(), + value: None, }), + 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) => ( @@ -852,11 +850,6 @@ 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, @@ -865,7 +858,20 @@ where value: Some(blk.clone()), }), ), + Err(e) => ( + ExitCode::SYS_MISSING_RETURN, + "error getting exit data block".to_owned(), + Err(ExecutionError::Fatal(anyhow!(e))), + ), }, + Abort::EntrypointNotFound => ( + ExitCode::USR_FORBIDDEN, + "entrypoint not found".to_owned(), + Err(ExecutionError::Syscall(SyscallError::new( + ErrorNumber::Forbidden, + "entrypoint not found", + ))), + ), Abort::OutOfGas => ( ExitCode::SYS_OUT_OF_GAS, "out of gas".to_owned(), @@ -1048,14 +1054,48 @@ impl Entrypoint { fn method_num(&self) -> MethodNum { match self { Entrypoint::Invoke(num) => *num, - Entrypoint::Upgrade => 191919, + Entrypoint::Upgrade(_) => fvm_shared::METHOD_UPGRADE, } } fn func_name(&self) -> &'static str { match self { Entrypoint::Invoke(_) => "invoke", - Entrypoint::Upgrade => "upgrade", + Entrypoint::Upgrade(_) => "upgrade", } } } + +// EntrypointParams is a helper struct to init the registry with the entrypoint specific +// parameters and then forward them to wasmtime +struct EntrypointParams { + entrypoint: Entrypoint, + params: Vec, +} + +impl EntrypointParams { + fn new(entrypoint: Entrypoint) -> Self { + Self { + entrypoint, + params: Vec::new(), + } + } + + fn maybe_put_registry(&mut self, br: &mut BlockRegistry) -> Result<()> { + match self.entrypoint { + Entrypoint::Invoke(_) => Ok(()), + Entrypoint::Upgrade(ui) => { + let ui_params = to_vec(&ui).map_err( + |e| syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), + )?; + let block_id = br.put(Block::new(CBOR, ui_params))?; + self.params.push(wasmtime::Val::I32(block_id as i32)); + Ok(()) + } + } + } + + fn params(&self) -> &Vec { + &self.params + } +} diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 4706b8ee6..74a367a23 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -4,6 +4,7 @@ use cid::Cid; use fvm_shared::address::Address; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; +use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum}; use crate::engine::Engine; @@ -210,14 +211,14 @@ pub struct FinishRet { #[derive(Clone, Debug, Copy)] pub enum Entrypoint { Invoke(MethodNum), - Upgrade, + Upgrade(UpgradeInfo), } impl std::fmt::Display for Entrypoint { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Entrypoint::Invoke(method) => write!(f, "invoke({})", method), - Entrypoint::Upgrade => write!(f, "upgrade"), + Entrypoint::Upgrade(_) => write!(f, "upgrade"), } } } diff --git a/fvm/src/syscalls/error.rs b/fvm/src/syscalls/error.rs index ee545272d..f786ccded 100644 --- a/fvm/src/syscalls/error.rs +++ b/fvm/src/syscalls/error.rs @@ -17,6 +17,9 @@ pub enum Abort { /// The actor ran out of gas. #[error("out of gas")] OutOfGas, + /// The actor did not export the endpoint that was called. + #[error("entrypoint not found")] + EntrypointNotFound, /// The system failed with a fatal error. #[error("fatal error: {0}")] Fatal(anyhow::Error), diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 1b31f717a..40ea2eb56 100644 --- a/shared/src/lib.rs +++ b/shared/src/lib.rs @@ -109,6 +109,8 @@ 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)] diff --git a/shared/src/upgrade/mod.rs b/shared/src/upgrade/mod.rs index 48da6a760..557f45abd 100644 --- a/shared/src/upgrade/mod.rs +++ b/shared/src/upgrade/mod.rs @@ -3,7 +3,7 @@ use cid::Cid; use fvm_ipld_encoding::tuple::*; -#[derive(Clone, Debug, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] +#[derive(Clone, Debug, Copy, PartialEq, Eq, Serialize_tuple, Deserialize_tuple)] pub struct UpgradeInfo { // the old code cid we are upgrading from pub old_code_cid: Cid, diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index 48fff8c89..e9dc464b1 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -4,6 +4,7 @@ use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_sdk as sdk; use fvm_shared::address::Address; +use fvm_shared::upgrade::UpgradeInfo; use serde_tuple::*; #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] struct SomeStruct { @@ -13,13 +14,24 @@ struct SomeStruct { const UPGRADE_FAILED_EXIT_CODE: u32 = 19; #[no_mangle] -pub fn upgrade(params_id: u32) -> u32 { +pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { sdk::initialize(); let params = sdk::message::params_raw(params_id).unwrap().unwrap(); + let ui_params = sdk::message::params_raw(upgrade_info_id).unwrap().unwrap(); + assert_eq!(params.codec, fvm_ipld_encoding::CBOR); + assert_eq!(ui_params.codec, fvm_ipld_encoding::CBOR); + + let p = params.deserialize::().unwrap(); + let ui = ui_params.deserialize::().unwrap(); + + sdk::debug::log(format!( + "[upgrade] value: {}, old_code_cid: {}", + p.value, ui.old_code_cid + )); - match params.deserialize::().unwrap().value { + match p.value { 1 => { let block_id = sdk::ipld::put_block(CBOR, &to_vec(&666).unwrap()).unwrap(); sdk::debug::log(format!( From 5bff8a4fbde9febd0f8468700a1ebfaf90c53047 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 8 Sep 2023 15:24:54 +0000 Subject: [PATCH 14/40] Update cargo.lock --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index f830adf71..b7a7f3439 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1936,7 +1936,7 @@ dependencies = [ "cid 0.10.1", "fvm_ipld_encoding 0.4.0", "fvm_sdk 3.3.0", - "fvm_shared 3.5.0", + "fvm_shared 3.6.0", "serde", "serde_tuple", ] From cbea1d0d4b1c6b514d32f23a33df168457c22aa0 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 8 Sep 2023 15:45:21 +0000 Subject: [PATCH 15/40] Wasm programs within infinite loop should result in SYS_ILLEGAL_INSTRUCTION --- testing/integration/tests/main.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index 8f4b65be6..69927f07d 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -589,14 +589,14 @@ fn test_exitcode(wat: &str, code: ExitCode) { } #[test] -fn out_of_gas() { +fn infinite_loop() { test_exitcode( r#"(module (memory (export "memory") 1) (func (export "invoke") (param $x i32) (result i32) (loop (br 0)) (i32.const 1)))"#, - ExitCode::SYS_OUT_OF_GAS, + ExitCode::SYS_ILLEGAL_INSTRUCTION, ) } From ed2187dd10f80008cf03a1691b48a92ec6f79e09 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 8 Sep 2023 16:59:10 +0000 Subject: [PATCH 16/40] Fix check for sys_out_of_gas --- fvm/src/call_manager/default.rs | 9 ++++++++- testing/integration/tests/main.rs | 4 ++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index fe77e9a31..824fe0e70 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -809,7 +809,10 @@ where update_gas_available(&mut store)?; let mut out = [wasmtime::Val::I32(0)]; - func.call(&mut store, params.as_slice(), &mut out)?; + let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + func.call(&mut store, params.as_slice(), &mut out) + })) + .map_err(|panic| Abort::Fatal(anyhow!("panic within actor: {:?}", panic)))?; // Charge for any remaining uncharged execution gas, returning an error if we run // out. @@ -819,6 +822,10 @@ where // detected it and returned OutOfGas above. Any other invocation failure is returned // here as an Abort + if let Err(err) = res { + return Err(err.into()); + } + Ok(out[0].unwrap_i32() as u32) })(); diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index 69927f07d..8f4b65be6 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -589,14 +589,14 @@ fn test_exitcode(wat: &str, code: ExitCode) { } #[test] -fn infinite_loop() { +fn out_of_gas() { test_exitcode( r#"(module (memory (export "memory") 1) (func (export "invoke") (param $x i32) (result i32) (loop (br 0)) (i32.const 1)))"#, - ExitCode::SYS_ILLEGAL_INSTRUCTION, + ExitCode::SYS_OUT_OF_GAS, ) } From 237b9baa2dcd4101a70df137c823cbefb843ed99 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Sat, 9 Sep 2023 16:02:42 +0000 Subject: [PATCH 17/40] Use match where more appropriate --- fvm/src/call_manager/default.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 824fe0e70..a3306bc10 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -822,11 +822,10 @@ where // detected it and returned OutOfGas above. Any other invocation failure is returned // here as an Abort - if let Err(err) = res { - return Err(err.into()); + match res { + Ok(_) => Ok(out[0].unwrap_i32() as u32), + Err(e) => Err(e.into()), } - - Ok(out[0].unwrap_i32() as u32) })(); let invocation_data = store.into_data(); From a7828446c6d595dfb1e534e0f6327c62ae1fb0f8 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 28 Sep 2023 15:22:42 +0000 Subject: [PATCH 18/40] Preserve caller --- fvm/src/call_manager/default.rs | 3 ++- fvm/src/call_manager/mod.rs | 1 + fvm/src/kernel/default.rs | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index a3306bc10..2a61353b0 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -407,6 +407,7 @@ where fn upgrade_actor>( &mut self, + caller: ActorID, actor_id: ActorID, new_code_cid: Cid, params: Option, @@ -437,7 +438,7 @@ where // run the upgrade entrypoint let result = self.send::( - actor_id, + caller, Address::new_id(actor_id), Entrypoint::Upgrade(UpgradeInfo { old_code_cid: code }), params, diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 74a367a23..0a4c7fb93 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -120,6 +120,7 @@ pub trait CallManager: 'static { fn upgrade_actor( &mut self, + caller: ActorID, actor_id: ActorID, new_code_cid: Cid, params: Option, diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 2483acf8c..f0988051f 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -887,9 +887,9 @@ where Some(self.blocks.get(params_id)?.clone()) }; - let result = self - .call_manager - .with_transaction(|cm| cm.upgrade_actor::(self.actor_id, new_code_cid, params)); + let result = self.call_manager.with_transaction(|cm| { + cm.upgrade_actor::(self.caller, self.actor_id, new_code_cid, params) + }); match result { Ok(InvocationResult { From f15f9b08152f73792d562f3e4a747bb11da8ffac Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 28 Sep 2023 16:04:56 +0000 Subject: [PATCH 19/40] fixes after rebasing in ipld reachability --- Cargo.lock | 4 ++-- fvm/src/call_manager/default.rs | 4 ++-- fvm/src/kernel/default.rs | 2 +- fvm/tests/dummy.rs | 1 + testing/integration/tests/upgrade_test.rs | 2 +- testing/test_actors/actors/fil-upgrade-actor/Cargo.toml | 4 ++-- 6 files changed, 9 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b7a7f3439..2d51623cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1935,8 +1935,8 @@ version = "0.1.0" dependencies = [ "cid 0.10.1", "fvm_ipld_encoding 0.4.0", - "fvm_sdk 3.3.0", - "fvm_shared 3.6.0", + "fvm_sdk 4.0.0-alpha.3", + "fvm_shared 4.0.0-alpha.3", "serde", "serde_tuple", ] diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 2a61353b0..7722462f5 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -632,7 +632,7 @@ where system_actor::SYSTEM_ACTOR_ID, id, Entrypoint::Invoke(fvm_shared::METHOD_CONSTRUCTOR), - Some(Block::new(CBOR, params)), + Some(Block::new(CBOR, params, Vec::new())), &TokenAmount::zero(), false, )?; @@ -1095,7 +1095,7 @@ impl EntrypointParams { let ui_params = to_vec(&ui).map_err( |e| syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), )?; - let block_id = br.put(Block::new(CBOR, ui_params))?; + let block_id = br.put_reachable(Block::new(CBOR, ui_params, Vec::new()))?; self.params.push(wasmtime::Val::I32(block_id as i32)); Ok(()) } diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index f0988051f..571ad6ac6 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -898,7 +898,7 @@ where }) => { let block_id = match value { None => NO_DATA_BLOCK_ID, - Some(block) => self.blocks.put(block).unwrap_or_else(|_| { + Some(block) => self.blocks.put_reachable(block).unwrap_or_else(|_| { log::error!("failed to write to kernel block registry"); NO_DATA_BLOCK_ID }), diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 71fab3141..272b61370 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -403,6 +403,7 @@ impl CallManager for DummyCallManager { fn upgrade_actor( &mut self, + _caller: ActorID, _actor_id: ActorID, _new_code_cid: Cid, _params: Option, diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs index b010d0f59..e58822ca0 100644 --- a/testing/integration/tests/upgrade_test.rs +++ b/testing/integration/tests/upgrade_test.rs @@ -17,7 +17,7 @@ use num_traits::Zero; #[test] fn upgrade_actor_test() { let mut tester = new_tester( - NetworkVersion::V18, + NetworkVersion::V21, StateTreeVersion::V5, MemoryBlockstore::default(), ) diff --git a/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml b/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml index 91dc61cbc..302a1b0e1 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml +++ b/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml @@ -5,8 +5,8 @@ edition = "2021" publish = false [target.'cfg(target_arch = "wasm32")'.dependencies] -fvm_sdk = { version = "3.3.0", path = "../../../../sdk" } -fvm_shared = { version = "3.5.0", path = "../../../../shared" } +fvm_sdk = { version = "4.0.0-alpha.3", path = "../../../../sdk" } +fvm_shared = { version = "4.0.0-alpha.3", path = "../../../../shared" } fvm_ipld_encoding = { version = "0.4.0", path = "../../../../ipld/encoding" } cid = { workspace = true } serde = { version = "1.0.164", features = ["derive"] } From cd67d6da6a59ccd55d5c084029b5e43ef8f426e2 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 3 Oct 2023 10:16:03 +0000 Subject: [PATCH 20/40] Move upgrade logic to kernel --- fvm/src/call_manager/default.rs | 60 +-------------------------------- fvm/src/call_manager/mod.rs | 12 +------ fvm/src/executor/default.rs | 2 +- fvm/src/kernel/default.rs | 52 ++++++++++++++++++++++++++-- fvm/src/kernel/mod.rs | 2 +- fvm/src/syscalls/actor.rs | 7 ++-- fvm/tests/dummy.rs | 15 +-------- 7 files changed, 57 insertions(+), 93 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 7722462f5..97eac440c 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -12,7 +12,6 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; -use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum, METHOD_SEND}; use num_traits::Zero; @@ -167,7 +166,7 @@ where &mut self.limits } - fn send( + fn call_actor( &mut self, from: ActorID, to: Address, @@ -405,63 +404,6 @@ where Ok(()) } - fn upgrade_actor>( - &mut self, - caller: ActorID, - actor_id: ActorID, - new_code_cid: Cid, - params: Option, - ) -> Result { - let origin = self.origin; - - let state = self - .state_tree_mut() - .get_actor(actor_id)? - .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))?; - - // store the code cid of the calling actor before running the upgrade entrypoint - // in case it was changed (which could happen if the target upgrade entrypoint - // sent a message to this actor which in turn called upgrade) - let code = state.code; - - // update the code cid of the actor to new_code_cid - self.state_tree_mut().set_actor( - origin, - ActorState::new( - new_code_cid, - state.state, - state.balance, - state.sequence, - None, - ), - ); - - // run the upgrade entrypoint - let result = self.send::( - caller, - Address::new_id(actor_id), - Entrypoint::Upgrade(UpgradeInfo { old_code_cid: code }), - params, - &TokenAmount::zero(), - None, - false, - )?; - - if result.exit_code == ExitCode::OK { - // after running the upgrade, our code cid must not have changed - let code_after_upgrade = self - .state_tree_mut() - .get_actor(actor_id)? - .ok_or_else(|| syscall_error!(NotFound; "actor not found: {}", actor_id))? - .code; - if code != code_after_upgrade { - return Err(syscall_error!(Forbidden; "re-entrant upgrade detected").into()); - } - } - - Ok(result) - } - fn append_event(&mut self, evt: StampedEvent) { self.events.append_event(evt) } diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 0a4c7fb93..eede49859 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -64,7 +64,7 @@ pub trait CallManager: 'static { /// Send a message. The type parameter `K` specifies the the _kernel_ on top of which the target /// actor should execute. #[allow(clippy::too_many_arguments)] - fn send>( + fn call_actor>( &mut self, from: ActorID, to: Address, @@ -118,16 +118,6 @@ pub trait CallManager: 'static { delegated_address: Option
, ) -> Result<()>; - fn upgrade_actor( - &mut self, - caller: ActorID, - actor_id: ActorID, - new_code_cid: Cid, - params: Option, - ) -> Result - where - K: Kernel; - /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 3b5b60352..12e9a631e 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -144,7 +144,7 @@ where let result = cm.with_transaction(|cm| { // Invoke the message. We charge for the return value internally if the call-stack depth // is 1. - cm.send::( + cm.call_actor::( sender_id, msg.to, Entrypoint::Invoke(msg.method_num), diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 571ad6ac6..73c282ace 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -20,6 +20,7 @@ use fvm_shared::event::{ActorEvent, Entry, Flags}; use fvm_shared::piece::{zero_piece_commitment, PaddedPieceSize}; use fvm_shared::sector::{RegisteredPoStProof, SectorInfo}; use fvm_shared::sys::out::vm::ContextFlags; +use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{commcid, ActorID}; use lazy_static::lazy_static; use multihash::MultihashDigest; @@ -139,7 +140,7 @@ where // Send. let result = self.call_manager.with_transaction(|cm| { - cm.send::( + cm.call_actor::( from, *recipient, Entrypoint::Invoke(method), @@ -873,7 +874,7 @@ where .create_actor(code_id, actor_id, delegated_address) } - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { if self.read_only { return Err( syscall_error!(ReadOnly, "upgrade_actor cannot be called while read-only").into(), @@ -888,7 +889,52 @@ where }; let result = self.call_manager.with_transaction(|cm| { - cm.upgrade_actor::(self.caller, self.actor_id, new_code_cid, params) + let origin = cm.origin(); + + let state = cm + .get_actor(self.actor_id)? + .ok_or_else(|| syscall_error!(NotFound; "actor not found"))?; + + // store the code cid of the calling actor before running the upgrade entrypoint + // in case it was changed (which could happen if the target upgrade entrypoint + // sent a message to this actor which in turn called upgrade) + let code = state.code; + + // update the code cid of the actor to new_code_cid + cm.set_actor( + origin, + ActorState::new( + new_code_cid, + state.state, + state.balance, + state.sequence, + None, + ), + )?; + + // run the upgrade entrypoint + let result = cm.call_actor::( + self.caller, + Address::new_id(self.actor_id), + Entrypoint::Upgrade(UpgradeInfo { old_code_cid: code }), + params, + &TokenAmount::from_whole(0), + None, + false, + )?; + + if result.exit_code == ExitCode::OK { + // after running the upgrade, our code cid must not have changed + let code_after_upgrade = cm + .get_actor(self.actor_id)? + .ok_or_else(|| syscall_error!(NotFound; "actor not found"))? + .code; + if code != code_after_upgrade { + return Err(syscall_error!(Forbidden; "re-entrant upgrade detected").into()); + } + } + + Ok(result) }); match result { diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index 1147837a6..ad6626fe6 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -209,7 +209,7 @@ pub trait ActorOps { delegated_address: Option
, ) -> Result<()>; - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result; + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result; /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 1ba75838d..5bbff82cf 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -109,15 +109,14 @@ pub fn create_actor( context.kernel.create_actor(typ, actor_id, addr) } -#[allow(dead_code)] -pub fn upgrade_actor( - context: Context<'_, impl Kernel>, +pub fn upgrade_actor( + context: Context<'_, K>, new_code_cid_off: u32, params_id: u32, ) -> Result { let cid = context.memory.read_cid(new_code_cid_off)?; - context.kernel.upgrade_actor(cid, params_id) + context.kernel.upgrade_actor::(cid, params_id) } pub fn get_builtin_actor_type( diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 272b61370..677d9a0b5 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -274,7 +274,7 @@ impl CallManager for DummyCallManager { } } - fn send>( + fn call_actor>( &mut self, _from: fvm_shared::ActorID, _to: Address, @@ -400,17 +400,4 @@ impl CallManager for DummyCallManager { ) -> fvm::kernel::Result<()> { todo!() } - - fn upgrade_actor( - &mut self, - _caller: ActorID, - _actor_id: ActorID, - _new_code_cid: Cid, - _params: Option, - ) -> kernel::Result - where - K: Kernel, - { - todo!() - } } From 31f11f27f5130e092d618ef090a92a528c06f790 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 3 Oct 2023 12:45:22 +0000 Subject: [PATCH 21/40] Refactor EntrypointParams into Entrypoint --- Cargo.lock | 4 +- fvm/src/call_manager/default.rs | 57 +------------------ fvm/src/call_manager/mod.rs | 32 ++++++++++- testing/conformance/src/vm.rs | 4 +- .../actors/fil-upgrade-actor/Cargo.toml | 4 +- 5 files changed, 40 insertions(+), 61 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2d51623cb..6642bb060 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1935,8 +1935,8 @@ version = "0.1.0" dependencies = [ "cid 0.10.1", "fvm_ipld_encoding 0.4.0", - "fvm_sdk 4.0.0-alpha.3", - "fvm_shared 4.0.0-alpha.3", + "fvm_sdk 4.0.0-alpha.4", + "fvm_shared 4.0.0-alpha.4", "serde", "serde_tuple", ] diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 97eac440c..bcdb15153 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -12,7 +12,7 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::{ErrorNumber, ExitCode}; use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; -use fvm_shared::{ActorID, MethodNum, METHOD_SEND}; +use fvm_shared::{ActorID, METHOD_SEND}; use num_traits::Zero; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; @@ -688,8 +688,7 @@ where // additional_params takes care of adding entrypoint specific params to the block // registry and passing them to wasmtime - let mut additional_params = EntrypointParams::new(entrypoint); - additional_params.maybe_put_registry(&mut block_registry)?; + let additional_params = entrypoint.into_params(&mut block_registry)?; // Increment invocation count self.invocation_count += 1; @@ -746,7 +745,7 @@ where }; let mut params = vec![wasmtime::Val::I32(params_id as i32)]; - params.extend_from_slice(additional_params.params().as_slice()); + params.extend_from_slice(additional_params.as_slice()); // Set the available gas. update_gas_available(&mut store)?; @@ -998,53 +997,3 @@ impl EventsAccumulator { }) } } - -impl Entrypoint { - fn method_num(&self) -> MethodNum { - match self { - Entrypoint::Invoke(num) => *num, - Entrypoint::Upgrade(_) => fvm_shared::METHOD_UPGRADE, - } - } - - fn func_name(&self) -> &'static str { - match self { - Entrypoint::Invoke(_) => "invoke", - Entrypoint::Upgrade(_) => "upgrade", - } - } -} - -// EntrypointParams is a helper struct to init the registry with the entrypoint specific -// parameters and then forward them to wasmtime -struct EntrypointParams { - entrypoint: Entrypoint, - params: Vec, -} - -impl EntrypointParams { - fn new(entrypoint: Entrypoint) -> Self { - Self { - entrypoint, - params: Vec::new(), - } - } - - fn maybe_put_registry(&mut self, br: &mut BlockRegistry) -> Result<()> { - match self.entrypoint { - Entrypoint::Invoke(_) => Ok(()), - Entrypoint::Upgrade(ui) => { - let ui_params = to_vec(&ui).map_err( - |e| syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), - )?; - let block_id = br.put_reachable(Block::new(CBOR, ui_params, Vec::new()))?; - self.params.push(wasmtime::Val::I32(block_id as i32)); - Ok(()) - } - } - } - - fn params(&self) -> &Vec { - &self.params - } -} diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index eede49859..b22592d20 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -1,6 +1,7 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT use cid::Cid; +use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_shared::address::Address; use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; @@ -9,7 +10,7 @@ use fvm_shared::{ActorID, MethodNum}; use crate::engine::Engine; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; -use crate::kernel::{self, Result}; +use crate::kernel::{self, BlockRegistry, Result}; use crate::machine::{Machine, MachineContext}; use crate::state_tree::ActorState; use crate::Kernel; @@ -213,3 +214,32 @@ impl std::fmt::Display for Entrypoint { } } } + +impl Entrypoint { + fn method_num(&self) -> MethodNum { + match self { + Entrypoint::Invoke(num) => *num, + Entrypoint::Upgrade(_) => fvm_shared::METHOD_UPGRADE, + } + } + + fn func_name(&self) -> &'static str { + match self { + Entrypoint::Invoke(_) => "invoke", + Entrypoint::Upgrade(_) => "upgrade", + } + } + + fn into_params(self, br: &mut BlockRegistry) -> Result> { + match self { + Entrypoint::Invoke(_) => Ok(Vec::new()), + Entrypoint::Upgrade(ui) => { + let ui_params = to_vec(&ui).map_err( + |e| crate::syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), + )?; + let block_id = br.put_reachable(kernel::Block::new(CBOR, ui_params, Vec::new()))?; + Ok(vec![wasmtime::Val::I32(block_id as i32)]) + } + } + } +} diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index fb801e643..db37ca191 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -299,8 +299,8 @@ where self.0.lookup_delegated_address(actor_id) } - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { - self.0.upgrade_actor(new_code_cid, params_id) + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + self.0.upgrade_actor::(new_code_cid, params_id) } } diff --git a/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml b/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml index 302a1b0e1..328f3d26f 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml +++ b/testing/test_actors/actors/fil-upgrade-actor/Cargo.toml @@ -5,8 +5,8 @@ edition = "2021" publish = false [target.'cfg(target_arch = "wasm32")'.dependencies] -fvm_sdk = { version = "4.0.0-alpha.3", path = "../../../../sdk" } -fvm_shared = { version = "4.0.0-alpha.3", path = "../../../../shared" } +fvm_sdk = { version = "4.0.0-alpha.4", path = "../../../../sdk" } +fvm_shared = { version = "4.0.0-alpha.4", path = "../../../../shared" } fvm_ipld_encoding = { version = "0.4.0", path = "../../../../ipld/encoding" } cid = { workspace = true } serde = { version = "1.0.164", features = ["derive"] } From 22d00c5b5ffd774ffb93b60b609671af1dbe7441 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 3 Oct 2023 18:09:32 +0000 Subject: [PATCH 22/40] Address review comments --- fvm/src/call_manager/mod.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index b22592d20..810645a8c 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -13,6 +13,7 @@ use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; use crate::kernel::{self, BlockRegistry, Result}; use crate::machine::{Machine, MachineContext}; use crate::state_tree::ActorState; +use crate::syscalls::error::Abort; use crate::Kernel; pub mod backtrace; @@ -234,9 +235,10 @@ impl Entrypoint { match self { Entrypoint::Invoke(_) => Ok(Vec::new()), Entrypoint::Upgrade(ui) => { - let ui_params = to_vec(&ui).map_err( - |e| crate::syscall_error!(IllegalArgument; "failed to serialize upgrade params: {}", e), - )?; + let ui_params = to_vec(&ui).map_err(|e| { + Abort::Fatal(anyhow::anyhow!("failed to serialize upgrade params: {}", e)) + })?; + // This is CBOR instead of DAG_CBOR because these params are not reachable let block_id = br.put_reachable(kernel::Block::new(CBOR, ui_params, Vec::new()))?; Ok(vec![wasmtime::Val::I32(block_id as i32)]) } From e8ddd7d83cefc0d7dc9632a9da105f617fc399e0 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Wed, 4 Oct 2023 16:28:06 -0400 Subject: [PATCH 23/40] error refactor Add a general-purpose ControlFlow syscall return value and use it. --- fvm/src/call_manager/default.rs | 20 ++++----- fvm/src/call_manager/mod.rs | 9 ++-- fvm/src/executor/default.rs | 3 -- fvm/src/kernel/default.rs | 34 +++++++-------- fvm/src/kernel/error.rs | 14 +------ fvm/src/kernel/mod.rs | 6 ++- fvm/src/syscalls/actor.rs | 33 ++++++++++++--- fvm/src/syscalls/bind.rs | 74 +++++++++++++++++++++++---------- fvm/src/syscalls/context.rs | 3 -- fvm/src/syscalls/error.rs | 5 --- fvm/src/syscalls/vm.rs | 16 ++----- fvm/tests/default_kernel/mod.rs | 6 --- sdk/src/actor.rs | 37 ++++++++++++++--- sdk/src/sys/actor.rs | 11 +++-- shared/src/error/mod.rs | 3 +- 15 files changed, 156 insertions(+), 118 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index bcdb15153..2edf9cde4 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -231,9 +231,6 @@ where ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "fatal")) } Err(ExecutionError::Syscall(s)) => ExecutionEvent::CallError(s.clone()), - Err(ExecutionError::Abort(_)) => { - ExecutionEvent::CallError(SyscallError::new(ErrorNumber::Forbidden, "aborted")) - } }); } @@ -723,9 +720,10 @@ where // From this point on, there are no more syscall errors, only aborts. let result: std::result::Result = (|| { + let code = &state.code; // Instantiate the module. let instance = engine - .instantiate(&mut store, &state.code)? + .instantiate(&mut store, code)? .context("actor not found") .map_err(Abort::Fatal)?; @@ -740,7 +738,11 @@ where let func = match instance.get_func(&mut store, entrypoint.func_name()) { Some(func) => func, None => { - return Err(Abort::EntrypointNotFound); + return Err(Abort::Exit( + ExitCode::SYS_INVALID_RECEIVER, + format!("cannot upgrade to {code}"), + 0, + )); } }; @@ -812,14 +814,6 @@ where Err(ExecutionError::Fatal(anyhow!(e))), ), }, - Abort::EntrypointNotFound => ( - ExitCode::USR_FORBIDDEN, - "entrypoint not found".to_owned(), - Err(ExecutionError::Syscall(SyscallError::new( - ErrorNumber::Forbidden, - "entrypoint not found", - ))), - ), Abort::OutOfGas => ( ExitCode::SYS_OUT_OF_GAS, "out of gas".to_owned(), diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 810645a8c..84d4387dd 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -10,10 +10,9 @@ use fvm_shared::{ActorID, MethodNum}; use crate::engine::Engine; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; -use crate::kernel::{self, BlockRegistry, Result}; +use crate::kernel::{self, BlockRegistry, ClassifyResult, Context, Result}; use crate::machine::{Machine, MachineContext}; use crate::state_tree::ActorState; -use crate::syscalls::error::Abort; use crate::Kernel; pub mod backtrace; @@ -235,9 +234,9 @@ impl Entrypoint { match self { Entrypoint::Invoke(_) => Ok(Vec::new()), Entrypoint::Upgrade(ui) => { - let ui_params = to_vec(&ui).map_err(|e| { - Abort::Fatal(anyhow::anyhow!("failed to serialize upgrade params: {}", e)) - })?; + let ui_params = to_vec(&ui) + .or_fatal() + .context("failed to serialize upgrade params")?; // This is CBOR instead of DAG_CBOR because these params are not reachable let block_id = br.put_reachable(kernel::Block::new(CBOR, ui_params, Vec::new()))?; Ok(vec![wasmtime::Val::I32(block_id as i32)]) diff --git a/fvm/src/executor/default.rs b/fvm/src/executor/default.rs index 12e9a631e..69628221c 100644 --- a/fvm/src/executor/default.rs +++ b/fvm/src/executor/default.rs @@ -201,9 +201,6 @@ where events_root, } } - - Err(ExecutionError::Abort(e)) => return Err(anyhow!("actor aborted: {}", e)), - Err(ExecutionError::OutOfGas) => Receipt { exit_code: ExitCode::SYS_OUT_OF_GAS, return_data: Default::default(), diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 73c282ace..f28b2fd5d 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -5,7 +5,6 @@ use std::convert::{TryFrom, TryInto}; use std::panic::{self, UnwindSafe}; use std::path::PathBuf; -use crate::syscalls::error::Abort; use anyhow::{anyhow, Context as _}; use cid::Cid; use filecoin_proofs_api::{self as proofs, ProverId, PublicReplicaInfo, SectorId}; @@ -874,7 +873,11 @@ where .create_actor(code_id, actor_id, delegated_address) } - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + fn upgrade_actor( + &mut self, + new_code_cid: Cid, + params_id: BlockId, + ) -> Result { if self.read_only { return Err( syscall_error!(ReadOnly, "upgrade_actor cannot be called while read-only").into(), @@ -938,27 +941,18 @@ where }); match result { - Ok(InvocationResult { - exit_code: ExitCode::OK, - value, - }) => { - let block_id = match value { - None => NO_DATA_BLOCK_ID, - Some(block) => self.blocks.put_reachable(block).unwrap_or_else(|_| { - log::error!("failed to write to kernel block registry"); - NO_DATA_BLOCK_ID - }), + Ok(InvocationResult { exit_code, value }) => { + let (block_stat, block_id) = match value { + None => (BlockStat { codec: 0, size: 0 }, NO_DATA_BLOCK_ID), + // TODO: Check error cases. At a minimum, we could run out of gas here! + Some(block) => (block.stat(), self.blocks.put_reachable(block)?), }; - Err(ExecutionError::Abort(Abort::Exit( - ExitCode::OK, - "actor upgraded".to_string(), + Ok(SendResult { block_id, - ))) + block_stat, + exit_code, + }) } - Ok(InvocationResult { - exit_code, - value: _, - }) => Ok(exit_code.value()), Err(err) => Err(err), } } diff --git a/fvm/src/kernel/error.rs b/fvm/src/kernel/error.rs index bf9dc9dd7..4b7010324 100644 --- a/fvm/src/kernel/error.rs +++ b/fvm/src/kernel/error.rs @@ -5,8 +5,6 @@ use std::fmt::Display; use derive_more::Display; use fvm_shared::error::ErrorNumber; -use crate::syscalls::error::Abort; - /// Execution result. pub type Result = std::result::Result; @@ -37,7 +35,6 @@ pub enum ExecutionError { OutOfGas, Syscall(SyscallError), Fatal(anyhow::Error), - Abort(Abort), } impl ExecutionError { @@ -55,7 +52,6 @@ impl ExecutionError { match self { Fatal(_) => true, OutOfGas | Syscall(_) => false, - Abort(_) => true, } } @@ -65,7 +61,7 @@ impl ExecutionError { use ExecutionError::*; match self { Syscall(_) => true, - OutOfGas | Fatal(_) | Abort(_) => false, + OutOfGas | Fatal(_) => false, } } } @@ -78,12 +74,6 @@ impl From for ExecutionError { } } -impl From for ExecutionError { - fn from(e: Abort) -> Self { - ExecutionError::Abort(e) - } -} - pub trait ClassifyResult: Sized { type Value; type Error; @@ -158,7 +148,6 @@ impl Context for ExecutionError { Syscall(e) => Syscall(SyscallError(format!("{}: {}", context, e.0), e.1)), Fatal(e) => Fatal(e.context(context.to_string())), OutOfGas => OutOfGas, // no reason necessary - Abort(_) => self, } } @@ -179,7 +168,6 @@ impl From for anyhow::Error { OutOfGas => anyhow::anyhow!("out of gas"), Syscall(err) => anyhow::anyhow!(err.0), Fatal(err) => err, - Abort(e) => anyhow::anyhow!("aborted: {}", e), } } } diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index ad6626fe6..a47338003 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -209,7 +209,11 @@ pub trait ActorOps { delegated_address: Option
, ) -> Result<()>; - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result; + fn upgrade_actor( + &mut self, + new_code_cid: Cid, + params_id: BlockId, + ) -> Result; /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 5bbff82cf..9aeba3038 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -3,8 +3,10 @@ use anyhow::{anyhow, Context as _}; use fvm_shared::{sys, ActorID}; +use super::bind::ControlFlow; +use super::error::Abort; use super::Context; -use crate::kernel::{ClassifyResult, Result}; +use crate::kernel::{ClassifyResult, Result, SendResult}; use crate::{syscall_error, Kernel}; pub fn resolve_address( @@ -113,10 +115,31 @@ pub fn upgrade_actor( context: Context<'_, K>, new_code_cid_off: u32, params_id: u32, -) -> Result { - let cid = context.memory.read_cid(new_code_cid_off)?; - - context.kernel.upgrade_actor::(cid, params_id) +) -> ControlFlow { + let cid = match context.memory.read_cid(new_code_cid_off) { + Ok(cid) => cid, + Err(err) => return err.into(), + }; + + match context.kernel.upgrade_actor::(cid, params_id) { + Ok(SendResult { + block_id, + block_stat, + exit_code, + }) => { + if exit_code.is_success() { + ControlFlow::Abort(Abort::Exit(exit_code, String::new(), block_id)) + } else { + ControlFlow::Return(sys::out::send::Send { + exit_code: exit_code.value(), + return_id: block_id, + return_codec: block_stat.codec, + return_size: block_stat.size, + }) + } + } + Err(err) => err.into(), + } } pub fn get_builtin_actor_type( diff --git a/fvm/src/syscalls/bind.rs b/fvm/src/syscalls/bind.rs index f3a5967d9..f40317ea2 100644 --- a/fvm/src/syscalls/bind.rs +++ b/fvm/src/syscalls/bind.rs @@ -49,39 +49,69 @@ pub(super) trait BindSyscall { ) -> anyhow::Result<&mut Self>; } +pub enum ControlFlow { + Return(T), + Error(SyscallError), + Abort(Abort), +} + +impl From for ControlFlow { + fn from(value: ExecutionError) -> Self { + match value { + ExecutionError::Syscall(err) => ControlFlow::Error(err), + ExecutionError::Fatal(err) => ControlFlow::Abort(Abort::Fatal(err)), + ExecutionError::OutOfGas => ControlFlow::Abort(Abort::OutOfGas), + } + } +} + /// The helper trait used by `BindSyscall` to convert kernel results with execution errors into /// results that can be handled by wasmtime. See the documentation on `BindSyscall` for details. #[doc(hidden)] -pub trait IntoSyscallResult: Sized { +pub trait IntoControlFlow: Sized { type Value: SyscallSafe; - fn into(self) -> Result, Abort>; + fn into_control_flow(self) -> ControlFlow; +} + +/// An uninhabited type. We use this in `abort` to make sure there's no way to return without +/// returning an error. +#[derive(Copy, Clone)] +#[doc(hidden)] +pub enum Never {} +unsafe impl SyscallSafe for Never {} + +// Implementations for syscalls that always abort. +impl IntoControlFlow for Abort { + type Value = Never; + fn into_control_flow(self) -> ControlFlow { + ControlFlow::Abort(self) + } } -// Implementations for syscalls that abort on error. -impl IntoSyscallResult for Result +// Implementations for syscalls that can abort. +impl IntoControlFlow for ControlFlow where T: SyscallSafe, { type Value = T; - fn into(self) -> Result, Abort> { - Ok(Ok(self?)) + fn into_control_flow(self) -> ControlFlow { + self } } // Implementations for normal syscalls. -impl IntoSyscallResult for kernel::Result +impl IntoControlFlow for kernel::Result where T: SyscallSafe, { type Value = T; - fn into(self) -> Result, Abort> { + fn into_control_flow(self) -> ControlFlow { match self { - Ok(value) => Ok(Ok(value)), + Ok(value) => ControlFlow::Return(value), Err(e) => match e { - ExecutionError::Syscall(err) => Ok(Err(err)), - ExecutionError::OutOfGas => Err(Abort::OutOfGas), - ExecutionError::Fatal(err) => Err(Abort::Fatal(err)), - ExecutionError::Abort(e) => Err(e), + ExecutionError::Syscall(err) => ControlFlow::Error(err), + ExecutionError::OutOfGas => ControlFlow::Abort(Abort::OutOfGas), + ExecutionError::Fatal(err) => ControlFlow::Abort(Abort::Fatal(err)), }, } } @@ -112,7 +142,7 @@ macro_rules! impl_bind_syscalls { where K: Kernel, Func: Fn(Context<'_, K> $(, $t)*) -> Ret + Send + Sync + 'static, - Ret: IntoSyscallResult, + Ret: IntoControlFlow, $($t: WasmTy+SyscallSafe,)* { fn bind( @@ -130,21 +160,21 @@ macro_rules! impl_bind_syscalls { charge_syscall_gas!(data.kernel); let ctx = Context{kernel: &mut data.kernel, memory: &mut memory}; - let out = syscall(ctx $(, $t)*).into(); + let out = syscall(ctx $(, $t)*).into_control_flow(); let result = match out { - Ok(Ok(_)) => { + ControlFlow::Return(_) => { log::trace!("syscall {}::{}: ok", module, name); data.last_error = None; Ok(0) }, - Ok(Err(err)) => { + ControlFlow::Error(err) => { let code = err.1; log::trace!("syscall {}::{}: fail ({})", module, name, code as u32); data.last_error = Some(backtrace::Cause::from_syscall(module, name, err)); Ok(code as u32) }, - Err(e) => Err(e.into()), + ControlFlow::Abort(abort) => Err(abort.into()), }; update_gas_available(&mut caller)?; @@ -168,8 +198,8 @@ macro_rules! impl_bind_syscalls { } let ctx = Context{kernel: &mut data.kernel, memory: &mut memory}; - let result = match syscall(ctx $(, $t)*).into() { - Ok(Ok(value)) => { + let result = match syscall(ctx $(, $t)*).into_control_flow() { + ControlFlow::Return(value) => { log::trace!("syscall {}::{}: ok", module, name); unsafe { // We're writing into a user-specified pointer, so avoid @@ -179,13 +209,13 @@ macro_rules! impl_bind_syscalls { data.last_error = None; Ok(0) }, - Ok(Err(err)) => { + ControlFlow::Error(err) => { let code = err.1; log::trace!("syscall {}::{}: fail ({})", module, name, code as u32); data.last_error = Some(backtrace::Cause::from_syscall(module, name, err)); Ok(code as u32) }, - Err(e) => Err(e.into()), + ControlFlow::Abort(abort) => Err(abort.into()), }; update_gas_available(&mut caller)?; diff --git a/fvm/src/syscalls/context.rs b/fvm/src/syscalls/context.rs index 5a3d7a53a..fac49f9b2 100644 --- a/fvm/src/syscalls/context.rs +++ b/fvm/src/syscalls/context.rs @@ -152,9 +152,6 @@ mod test { $crate::kernel::ExecutionError::OutOfGas => { panic!("got unexpected out of gas") } - $crate::kernel::ExecutionError::Abort(abort) => { - panic!("got unexpected abort {}", abort) - } } }; } diff --git a/fvm/src/syscalls/error.rs b/fvm/src/syscalls/error.rs index f786ccded..1dbd81aaa 100644 --- a/fvm/src/syscalls/error.rs +++ b/fvm/src/syscalls/error.rs @@ -17,9 +17,6 @@ pub enum Abort { /// The actor ran out of gas. #[error("out of gas")] OutOfGas, - /// The actor did not export the endpoint that was called. - #[error("entrypoint not found")] - EntrypointNotFound, /// The system failed with a fatal error. #[error("fatal error: {0}")] Fatal(anyhow::Error), @@ -40,7 +37,6 @@ impl Abort { ), ExecutionError::OutOfGas => Abort::OutOfGas, ExecutionError::Fatal(err) => Abort::Fatal(err), - ExecutionError::Abort(e) => e, } } @@ -50,7 +46,6 @@ impl Abort { ExecutionError::OutOfGas => Abort::OutOfGas, ExecutionError::Fatal(e) => Abort::Fatal(e), ExecutionError::Syscall(e) => Abort::Fatal(anyhow!("unexpected syscall error: {}", e)), - ExecutionError::Abort(e) => e, } } } diff --git a/fvm/src/syscalls/vm.rs b/fvm/src/syscalls/vm.rs index 8b8834997..1e5ad4218 100644 --- a/fvm/src/syscalls/vm.rs +++ b/fvm/src/syscalls/vm.rs @@ -2,19 +2,11 @@ // SPDX-License-Identifier: Apache-2.0, MIT use fvm_shared::error::ExitCode; use fvm_shared::sys::out::vm::MessageContext; -use fvm_shared::sys::SyscallSafe; use super::error::Abort; use super::Context; use crate::kernel::Kernel; -/// An uninhabited type. We use this in `abort` to make sure there's no way to return without -/// returning an error. -#[derive(Copy, Clone)] -pub enum Never {} - -unsafe impl SyscallSafe for Never {} - /// The maximum message length included in the backtrace. Given 1024 levels, this gives us a total /// maximum of around 1MiB for debugging. const MAX_MESSAGE_LEN: usize = 1024; @@ -26,14 +18,14 @@ pub fn exit( blk: u32, message_off: u32, message_len: u32, -) -> Result { +) -> Abort { let code = ExitCode::new(code); if !code.is_success() && code.is_system_error() { - return Err(Abort::Exit( + return Abort::Exit( ExitCode::SYS_ILLEGAL_EXIT_CODE, format!("actor aborted with code {}", code), blk, - )); + ); } let message = if message_len == 0 { @@ -57,7 +49,7 @@ pub fn exit( Err(e) => format!("failed to extract error message: {e}"), } }; - Err(Abort::Exit(code, message, blk)) + Abort::Exit(code, message, blk) } pub fn message_context(context: Context<'_, impl Kernel>) -> crate::kernel::Result { diff --git a/fvm/tests/default_kernel/mod.rs b/fvm/tests/default_kernel/mod.rs index 8ef956176..3827e9f45 100644 --- a/fvm/tests/default_kernel/mod.rs +++ b/fvm/tests/default_kernel/mod.rs @@ -76,9 +76,6 @@ macro_rules! expect_syscall_err { ::fvm::kernel::ExecutionError::OutOfGas => { panic!("got unexpected out of gas") } - ::fvm::kernel::ExecutionError::Abort(abort) => { - panic!("got unexpected abort {}", abort) - } } }; } @@ -94,9 +91,6 @@ macro_rules! expect_out_of_gas { ::fvm::kernel::ExecutionError::Fatal(err) => { panic!("got unexpected fatal error: {}", err) } - ::fvm::kernel::ExecutionError::Abort(abort) => { - panic!("got unexpected abort {}", abort) - } } }; } diff --git a/sdk/src/actor.rs b/sdk/src/actor.rs index b0f8bf511..9e798babb 100644 --- a/sdk/src/actor.rs +++ b/sdk/src/actor.rs @@ -1,14 +1,13 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT -use core::option::Option; use std::ptr; // no_std use cid::Cid; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::{Address, Payload, MAX_ADDRESS_LEN}; use fvm_shared::econ::TokenAmount; -use fvm_shared::error::ErrorNumber; -use fvm_shared::{ActorID, MAX_CID_LEN}; +use fvm_shared::error::{ErrorNumber, ExitCode}; +use fvm_shared::{ActorID, Response, MAX_CID_LEN}; use log::error; use crate::{sys, SyscallResult, NO_DATA_BLOCK_ID}; @@ -109,7 +108,7 @@ pub fn create_actor( } /// Upgrades an actor using the given block which includes the old code cid and the upgrade params -pub fn upgrade_actor(new_code_cid: Cid, params: Option) -> SyscallResult { +pub fn upgrade_actor(new_code_cid: Cid, params: Option) -> SyscallResult { unsafe { let cid = new_code_cid.to_bytes(); @@ -118,7 +117,35 @@ pub fn upgrade_actor(new_code_cid: Cid, params: Option) -> SyscallRes None => NO_DATA_BLOCK_ID, }; - sys::actor::upgrade_actor(cid.as_ptr(), params_id) + let fvm_shared::sys::out::send::Send { + exit_code, + return_id, + return_codec, + return_size, + } = sys::actor::upgrade_actor(cid.as_ptr(), params_id)?; + + // Process the result. + // TODO: dedup with the send syscall... + let exit_code = ExitCode::new(exit_code); + let return_data = if return_id == NO_DATA_BLOCK_ID { + None + } else { + // Allocate a buffer to read the return data. + let mut bytes = vec![0; return_size as usize]; + + // Now read the return data. + let unread = sys::ipld::block_read(return_id, 0, bytes.as_mut_ptr(), return_size)?; + assert_eq!(0, unread); + Some(IpldBlock { + codec: return_codec, + data: bytes.to_vec(), + }) + }; + + Ok(Response { + exit_code, + return_data, + }) } } diff --git a/sdk/src/sys/actor.rs b/sdk/src/sys/actor.rs index 25f9a8ebc..ad93ca1c8 100644 --- a/sdk/src/sys/actor.rs +++ b/sdk/src/sys/actor.rs @@ -2,6 +2,9 @@ // SPDX-License-Identifier: Apache-2.0, MIT //! Syscalls for creating and resolving actors. +#[doc(inline)] +pub use fvm_shared::sys::out::send::*; + // for documentation links #[cfg(doc)] use crate::sys::ErrorNumber::*; @@ -139,11 +142,11 @@ super::fvm_syscalls! { /// /// # Returns /// - /// On success, this syscall will not return. Instead, the current invocation will "complete" and - /// the return value will be the block returned by the new code's `upgrade` endpoint. + /// On successful upgrade, this syscall will not return. Instead, the current invocation will + /// "complete" and the return value will be the block returned by the new code's `upgrade` endpoint. /// /// If the new code rejects the upgrade (aborts) or performs an illegal operation, this syscall will - /// return the exit code of the `upgrade` endpoint. + /// return the exit code plus the error returned by the upgrade endpoint. /// /// Finally, the syscall will return an error if it fails to call the upgrade endpoint entirely. /// @@ -159,7 +162,7 @@ super::fvm_syscalls! { pub fn upgrade_actor( new_code_cid_off: *const u8, params: u32, - ) -> Result; + ) -> Result; /// Installs and ensures actor code is valid and loaded. /// **Privileged:** May only be called by the init actor. diff --git a/shared/src/error/mod.rs b/shared/src/error/mod.rs index fb4a06a9a..3f4712304 100644 --- a/shared/src/error/mod.rs +++ b/shared/src/error/mod.rs @@ -64,7 +64,8 @@ impl ExitCode { //pub const SYS_RESERVED_3 ExitCode = ExitCode::new(3); /// The message receiver trapped (panicked). pub const SYS_ILLEGAL_INSTRUCTION: ExitCode = ExitCode::new(4); - /// The message receiver doesn't exist and can't be automatically created + /// The message receiver either doesn't exist and can't be automatically created or it doesn't + /// implement the required entrypoint. pub const SYS_INVALID_RECEIVER: ExitCode = ExitCode::new(5); /// The message sender didn't have the requisite funds. pub const SYS_INSUFFICIENT_FUNDS: ExitCode = ExitCode::new(6); From 9a7583cae89c5f6f3b520952e02584e5170aa9a7 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 5 Oct 2023 11:31:52 +0000 Subject: [PATCH 24/40] fix tests after merging in stebs error refactor --- testing/conformance/src/vm.rs | 2 +- .../test_actors/actors/fil-syscall-actor/src/actor.rs | 9 +++++++-- .../test_actors/actors/fil-upgrade-actor/src/actor.rs | 5 +++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index db37ca191..549cbd251 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -299,7 +299,7 @@ where self.0.lookup_delegated_address(actor_id) } - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { self.0.upgrade_actor::(new_code_cid, params_id) } } diff --git a/testing/test_actors/actors/fil-syscall-actor/src/actor.rs b/testing/test_actors/actors/fil-syscall-actor/src/actor.rs index fe86d97cb..6098497b3 100644 --- a/testing/test_actors/actors/fil-syscall-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-syscall-actor/src/actor.rs @@ -380,11 +380,16 @@ fn test_unaligned() { } fn test_upgrade() { - // test that calling `upgrade_actor` on ourselves results in a Forbidden error + // test that calling `upgrade_actor` on ourselves results in a SYS_INVALID_RECEIVER error // since we don't have a upgrade endpoint let res = sdk::actor::upgrade_actor( sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(), None, + ) + .unwrap(); + + assert_eq!( + res.exit_code, + fvm_shared::error::ExitCode::SYS_INVALID_RECEIVER, ); - assert_eq!(res, Err(ErrorNumber::Forbidden)); } diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index e9dc464b1..525d25ec7 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -81,9 +81,10 @@ pub fn invoke(_: u32) -> u32 { 2 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 2 }).unwrap(); - let exit_code = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); + let res = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); assert_eq!( - UPGRADE_FAILED_EXIT_CODE, exit_code, + UPGRADE_FAILED_EXIT_CODE, + res.exit_code.value(), "invalid exit code returned from upgrade_actor" ); } From 8839e131d21af04a0533245a9efb228f1c2610c5 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 5 Oct 2023 12:03:08 +0000 Subject: [PATCH 25/40] dedup processing send response --- sdk/src/actor.rs | 35 ++++------------------------------- sdk/src/lib.rs | 27 +++++++++++++++++++++++++++ sdk/src/send.rs | 33 ++++----------------------------- 3 files changed, 35 insertions(+), 60 deletions(-) diff --git a/sdk/src/actor.rs b/sdk/src/actor.rs index 9e798babb..0269fbaa1 100644 --- a/sdk/src/actor.rs +++ b/sdk/src/actor.rs @@ -6,11 +6,11 @@ use cid::Cid; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::{Address, Payload, MAX_ADDRESS_LEN}; use fvm_shared::econ::TokenAmount; -use fvm_shared::error::{ErrorNumber, ExitCode}; +use fvm_shared::error::ErrorNumber; use fvm_shared::{ActorID, Response, MAX_CID_LEN}; use log::error; -use crate::{sys, SyscallResult, NO_DATA_BLOCK_ID}; +use crate::{build_response, sys, SyscallResult, NO_DATA_BLOCK_ID}; /// Resolves the ID address of an actor. Returns `None` if the address cannot be resolved. /// Successfully resolving an address doesn't necessarily mean the actor exists (e.g., if the @@ -117,35 +117,8 @@ pub fn upgrade_actor(new_code_cid: Cid, params: Option) -> SyscallRes None => NO_DATA_BLOCK_ID, }; - let fvm_shared::sys::out::send::Send { - exit_code, - return_id, - return_codec, - return_size, - } = sys::actor::upgrade_actor(cid.as_ptr(), params_id)?; - - // Process the result. - // TODO: dedup with the send syscall... - let exit_code = ExitCode::new(exit_code); - let return_data = if return_id == NO_DATA_BLOCK_ID { - None - } else { - // Allocate a buffer to read the return data. - let mut bytes = vec![0; return_size as usize]; - - // Now read the return data. - let unread = sys::ipld::block_read(return_id, 0, bytes.as_mut_ptr(), return_size)?; - assert_eq!(0, unread); - Some(IpldBlock { - codec: return_codec, - data: bytes.to_vec(), - }) - }; - - Ok(Response { - exit_code, - return_data, - }) + let send = sys::actor::upgrade_actor(cid.as_ptr(), params_id)?; + build_response(send) } } diff --git a/sdk/src/lib.rs b/sdk/src/lib.rs index 5ce05a36f..f1a362905 100644 --- a/sdk/src/lib.rs +++ b/sdk/src/lib.rs @@ -46,3 +46,30 @@ pub fn initialize() { debug::init_logging(); vm::set_panic_handler(); } + +fn build_response(send: fvm_shared::sys::out::send::Send) -> SyscallResult { + let exit_code = fvm_shared::error::ExitCode::new(send.exit_code); + let return_data = if send.return_id == NO_DATA_BLOCK_ID { + None + } else { + // Allocate a buffer to read the return data. + let mut bytes = vec![0; send.return_size as usize]; + + unsafe { + // Now read the return data. + let unread = + sys::ipld::block_read(send.return_id, 0, bytes.as_mut_ptr(), send.return_size)?; + assert_eq!(0, unread); + } + + Some(fvm_ipld_encoding::ipld_block::IpldBlock { + codec: send.return_codec, + data: bytes.to_vec(), + }) + }; + + Ok(fvm_shared::Response { + exit_code, + return_data, + }) +} diff --git a/sdk/src/send.rs b/sdk/src/send.rs index dc02f81df..5d54785ca 100644 --- a/sdk/src/send.rs +++ b/sdk/src/send.rs @@ -5,11 +5,11 @@ use std::convert::TryInto; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_shared::address::Address; use fvm_shared::econ::TokenAmount; -use fvm_shared::error::{ErrorNumber, ExitCode}; +use fvm_shared::error::ErrorNumber; use fvm_shared::sys::SendFlags; use fvm_shared::{MethodNum, Response}; -use crate::{sys, SyscallResult, NO_DATA_BLOCK_ID}; +use crate::{build_response, sys, SyscallResult, NO_DATA_BLOCK_ID}; /// Sends a message to another actor. pub fn send( @@ -33,12 +33,7 @@ pub fn send( }; // Perform the syscall to send the message. - let fvm_shared::sys::out::send::Send { - exit_code, - return_id, - return_codec, - return_size, - } = sys::send::send( + let send = sys::send::send( recipient.as_ptr(), recipient.len() as u32, method, @@ -49,26 +44,6 @@ pub fn send( flags, )?; - // Process the result. - let exit_code = ExitCode::new(exit_code); - let return_data = if return_id == NO_DATA_BLOCK_ID { - None - } else { - // Allocate a buffer to read the return data. - let mut bytes = vec![0; return_size as usize]; - - // Now read the return data. - let unread = sys::ipld::block_read(return_id, 0, bytes.as_mut_ptr(), return_size)?; - assert_eq!(0, unread); - Some(IpldBlock { - codec: return_codec, - data: bytes.to_vec(), - }) - }; - - Ok(Response { - exit_code, - return_data, - }) + build_response(send) } } From 9807fefb79c639b90a1494eeec8fa07aba669966 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 5 Oct 2023 14:52:17 +0000 Subject: [PATCH 26/40] put_reachable can never fail in kernel:upgrade_actor --- fvm/src/kernel/default.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index f28b2fd5d..23821d07d 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -891,6 +891,11 @@ where Some(self.blocks.get(params_id)?.clone()) }; + // Make sure we can actually store the return block. + if self.blocks.is_full() { + return Err(syscall_error!(LimitExceeded; "cannot store return block").into()); + } + let result = self.call_manager.with_transaction(|cm| { let origin = cm.origin(); @@ -944,7 +949,6 @@ where Ok(InvocationResult { exit_code, value }) => { let (block_stat, block_id) = match value { None => (BlockStat { codec: 0, size: 0 }, NO_DATA_BLOCK_ID), - // TODO: Check error cases. At a minimum, we could run out of gas here! Some(block) => (block.stat(), self.blocks.put_reachable(block)?), }; Ok(SendResult { From c1ef6e3dacda71694af7699aee50778cd27e6843 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 6 Oct 2023 11:27:34 +0000 Subject: [PATCH 27/40] reduce disk space by having fewer integration test files which broke CI --- testing/integration/tests/address_test.rs | 66 ------- testing/integration/tests/main.rs | 201 ++++++++++++++++++++- testing/integration/tests/readonly_test.rs | 65 ------- testing/integration/tests/upgrade_test.rs | 108 ----------- 4 files changed, 199 insertions(+), 241 deletions(-) delete mode 100644 testing/integration/tests/address_test.rs delete mode 100644 testing/integration/tests/readonly_test.rs delete mode 100644 testing/integration/tests/upgrade_test.rs diff --git a/testing/integration/tests/address_test.rs b/testing/integration/tests/address_test.rs deleted file mode 100644 index da9b061a9..000000000 --- a/testing/integration/tests/address_test.rs +++ /dev/null @@ -1,66 +0,0 @@ -// Copyright 2021-2023 Protocol Labs -// SPDX-License-Identifier: Apache-2.0, MIT -mod bundles; -use bundles::*; -use fvm::executor::{ApplyKind, Executor}; -use fvm_integration_tests::dummy::DummyExterns; -use fvm_ipld_blockstore::MemoryBlockstore; -use fvm_shared::address::Address; -use fvm_shared::econ::TokenAmount; -use fvm_shared::message::Message; -use fvm_shared::state::StateTreeVersion; -use fvm_shared::version::NetworkVersion; -use fvm_test_actors::wasm_bin::ADDRESS_ACTOR_BINARY; -use num_traits::Zero; - -#[test] -fn basic_address_tests() { - // Instantiate tester - let mut tester = new_tester( - NetworkVersion::V21, - StateTreeVersion::V5, - MemoryBlockstore::default(), - ) - .unwrap(); - - let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); - - let wasm_bin = ADDRESS_ACTOR_BINARY; - - // Set actor state - let actor_state = [(); 0]; - let state_cid = tester.set_state(&actor_state).unwrap(); - - // Set actor - let actor_address = Address::new_id(10000); - - tester - .set_actor_from_bin(wasm_bin, state_cid, actor_address, TokenAmount::zero()) - .unwrap(); - - // Instantiate machine - tester.instantiate_machine(DummyExterns).unwrap(); - - let executor = tester.executor.as_mut().unwrap(); - - // Test all methods. - for (seq, method) in (2..=5).enumerate() { - let message = Message { - from: sender_address, - to: actor_address, - gas_limit: 1000000000, - method_num: method, - sequence: seq as u64, - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - } -} diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index 8f4b65be6..b40f169db 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -19,8 +19,9 @@ use fvm_shared::message::Message; use fvm_shared::state::StateTreeVersion; use fvm_shared::version::NetworkVersion; use fvm_test_actors::wasm_bin::{ - CREATE_ACTOR_BINARY, EXIT_DATA_ACTOR_BINARY, HELLO_WORLD_ACTOR_BINARY, IPLD_ACTOR_BINARY, - OOM_ACTOR_BINARY, SSELF_ACTOR_BINARY, STACK_OVERFLOW_ACTOR_BINARY, SYSCALL_ACTOR_BINARY, + ADDRESS_ACTOR_BINARY, CREATE_ACTOR_BINARY, EXIT_DATA_ACTOR_BINARY, HELLO_WORLD_ACTOR_BINARY, + IPLD_ACTOR_BINARY, OOM_ACTOR_BINARY, READONLY_ACTOR_BINARY, SSELF_ACTOR_BINARY, + STACK_OVERFLOW_ACTOR_BINARY, SYSCALL_ACTOR_BINARY, UPGRADE_ACTOR_BINARY, }; use num_traits::Zero; @@ -1002,6 +1003,202 @@ fn test_oom4() { assert_eq!(res.msg_receipt.exit_code, ExitCode::SYS_ILLEGAL_INSTRUCTION); } +#[test] +fn basic_address_tests() { + // Instantiate tester + let mut tester = new_tester( + NetworkVersion::V21, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) + .unwrap(); + + let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); + + let wasm_bin = ADDRESS_ACTOR_BINARY; + + // Set actor state + let actor_state = [(); 0]; + let state_cid = tester.set_state(&actor_state).unwrap(); + + // Set actor + let actor_address = Address::new_id(10000); + + tester + .set_actor_from_bin(wasm_bin, state_cid, actor_address, TokenAmount::zero()) + .unwrap(); + + // Instantiate machine + tester.instantiate_machine(DummyExterns).unwrap(); + + let executor = tester.executor.as_mut().unwrap(); + + // Test all methods. + for (seq, method) in (2..=5).enumerate() { + let message = Message { + from: sender_address, + to: actor_address, + gas_limit: 1000000000, + method_num: method, + sequence: seq as u64, + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } +} + +#[test] +fn readonly_actor_tests() { + // Instantiate tester + let mut tester = new_tester( + NetworkVersion::V21, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) + .unwrap(); + + let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); + + let wasm_bin = READONLY_ACTOR_BINARY; + + // Set actor state + let actor_state = [(); 0]; + let state_cid = tester.set_state(&actor_state).unwrap(); + + // Set actor + let actor_address = Address::new_id(10000); + + tester + .set_actor_from_bin(wasm_bin, state_cid, actor_address, TokenAmount::zero()) + .unwrap(); + + // Instantiate machine + tester.instantiate_machine(DummyExterns).unwrap(); + + let executor = tester.executor.as_mut().unwrap(); + + let message = Message { + from: sender_address, + to: actor_address, + gas_limit: 1000000000, + method_num: 2, + sequence: 0, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + assert!(res.msg_receipt.events_root.is_none()); +} + +#[test] +fn upgrade_actor_test() { + let mut tester = new_tester( + NetworkVersion::V21, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) + .unwrap(); + + let sender: [Account; 3] = tester.create_accounts().unwrap(); + let receiver = Address::new_id(10000); + let state_cid = tester.set_state(&[(); 0]).unwrap(); + + let wasm_bin = UPGRADE_ACTOR_BINARY; + tester + .set_actor_from_bin(wasm_bin, state_cid, receiver, TokenAmount::zero()) + .unwrap(); + tester.instantiate_machine(DummyExterns).unwrap(); + + let executor = tester.executor.as_mut().unwrap(); + + { + // test a successful call to `upgrade` endpoint + let message = Message { + from: sender[0].1, + to: receiver, + gas_limit: 1000000000, + method_num: 1, + sequence: 0_u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); + assert_eq!(val, 666); + } + + { + let message = Message { + from: sender[1].1, + to: receiver, + gas_limit: 1000000000, + method_num: 2, + sequence: 0_u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } + + { + let message = Message { + from: sender[2].1, + to: receiver, + gas_limit: 1000000000, + method_num: 3, + sequence: 0_u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + + let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); + assert_eq!(val, 444); + + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } +} + #[derive(Default)] pub struct FailingBlockstore { fail_for: RefCell>, diff --git a/testing/integration/tests/readonly_test.rs b/testing/integration/tests/readonly_test.rs deleted file mode 100644 index eb489119e..000000000 --- a/testing/integration/tests/readonly_test.rs +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright 2021-2023 Protocol Labs -// SPDX-License-Identifier: Apache-2.0, MIT -mod bundles; -use bundles::*; -use fvm::executor::{ApplyKind, Executor}; -use fvm_integration_tests::dummy::DummyExterns; -use fvm_ipld_blockstore::MemoryBlockstore; -use fvm_shared::address::Address; -use fvm_shared::econ::TokenAmount; -use fvm_shared::message::Message; -use fvm_shared::state::StateTreeVersion; -use fvm_shared::version::NetworkVersion; -use fvm_test_actors::wasm_bin::READONLY_ACTOR_BINARY; -use num_traits::Zero; - -#[test] -fn readonly_actor_tests() { - // Instantiate tester - let mut tester = new_tester( - NetworkVersion::V21, - StateTreeVersion::V5, - MemoryBlockstore::default(), - ) - .unwrap(); - - let [(_sender_id, sender_address)] = tester.create_accounts().unwrap(); - - let wasm_bin = READONLY_ACTOR_BINARY; - - // Set actor state - let actor_state = [(); 0]; - let state_cid = tester.set_state(&actor_state).unwrap(); - - // Set actor - let actor_address = Address::new_id(10000); - - tester - .set_actor_from_bin(wasm_bin, state_cid, actor_address, TokenAmount::zero()) - .unwrap(); - - // Instantiate machine - tester.instantiate_machine(DummyExterns).unwrap(); - - let executor = tester.executor.as_mut().unwrap(); - - let message = Message { - from: sender_address, - to: actor_address, - gas_limit: 1000000000, - method_num: 2, - sequence: 0, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - assert!(res.msg_receipt.events_root.is_none()); -} diff --git a/testing/integration/tests/upgrade_test.rs b/testing/integration/tests/upgrade_test.rs deleted file mode 100644 index e58822ca0..000000000 --- a/testing/integration/tests/upgrade_test.rs +++ /dev/null @@ -1,108 +0,0 @@ -// Copyright 2021-2023 Protocol Labs -// SPDX-License-Identifier: Apache-2.0, MIT -mod bundles; -use bundles::*; -use fvm::executor::{ApplyKind, Executor}; -use fvm_integration_tests::dummy::DummyExterns; -use fvm_integration_tests::tester::Account; -use fvm_ipld_blockstore::MemoryBlockstore; -use fvm_shared::address::Address; -use fvm_shared::econ::TokenAmount; -use fvm_shared::message::Message; -use fvm_shared::state::StateTreeVersion; -use fvm_shared::version::NetworkVersion; -use fvm_test_actors::wasm_bin::UPGRADE_ACTOR_BINARY; -use num_traits::Zero; - -#[test] -fn upgrade_actor_test() { - let mut tester = new_tester( - NetworkVersion::V21, - StateTreeVersion::V5, - MemoryBlockstore::default(), - ) - .unwrap(); - - let sender: [Account; 3] = tester.create_accounts().unwrap(); - let receiver = Address::new_id(10000); - let state_cid = tester.set_state(&[(); 0]).unwrap(); - - let wasm_bin = UPGRADE_ACTOR_BINARY; - tester - .set_actor_from_bin(wasm_bin, state_cid, receiver, TokenAmount::zero()) - .unwrap(); - tester.instantiate_machine(DummyExterns).unwrap(); - - let executor = tester.executor.as_mut().unwrap(); - - { - // test a successful call to `upgrade` endpoint - let message = Message { - from: sender[0].1, - to: receiver, - gas_limit: 1000000000, - method_num: 1, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); - assert_eq!(val, 666); - } - - { - let message = Message { - from: sender[1].1, - to: receiver, - gas_limit: 1000000000, - method_num: 2, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - } - - { - let message = Message { - from: sender[2].1, - to: receiver, - gas_limit: 1000000000, - method_num: 3, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - - let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); - assert_eq!(val, 444); - - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - } -} From 53aa0d53cf4e0d84e2cbffc35823b048f32ddfd6 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 17 Oct 2023 18:52:30 +0000 Subject: [PATCH 28/40] minor fixes --- fvm/src/call_manager/default.rs | 2 +- fvm/src/call_manager/mod.rs | 7 +++++++ fvm/src/kernel/default.rs | 4 ++-- fvm/src/kernel/error.rs | 10 ---------- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 2edf9cde4..1c1d0f76d 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -660,7 +660,7 @@ where } // Abort early if we have a send. - if let Entrypoint::Invoke(METHOD_SEND) = entrypoint { + if entrypoint.invokes(METHOD_SEND) { log::trace!("sent {} -> {}: {}", from, to, &value); return Ok(InvocationResult::default()); } diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 84d4387dd..2e5b2d7fd 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -230,6 +230,13 @@ impl Entrypoint { } } + fn invokes(&self, method: MethodNum) -> bool { + match self { + Entrypoint::Invoke(num) => *num == method, + Entrypoint::Upgrade(_) => false, + } + } + fn into_params(self, br: &mut BlockRegistry) -> Result> { match self { Entrypoint::Invoke(_) => Ok(Vec::new()), diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 23821d07d..72c024d80 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -14,7 +14,7 @@ use fvm_shared::address::Payload; use fvm_shared::consensus::ConsensusFault; use fvm_shared::crypto::signature; use fvm_shared::econ::TokenAmount; -use fvm_shared::error::{ErrorNumber, ExitCode}; +use fvm_shared::error::ErrorNumber; use fvm_shared::event::{ActorEvent, Entry, Flags}; use fvm_shared::piece::{zero_piece_commitment, PaddedPieceSize}; use fvm_shared::sector::{RegisteredPoStProof, SectorInfo}; @@ -931,7 +931,7 @@ where false, )?; - if result.exit_code == ExitCode::OK { + if result.exit_code.is_success() { // after running the upgrade, our code cid must not have changed let code_after_upgrade = cm .get_actor(self.actor_id)? diff --git a/fvm/src/kernel/error.rs b/fvm/src/kernel/error.rs index 4b7010324..257b5dfbf 100644 --- a/fvm/src/kernel/error.rs +++ b/fvm/src/kernel/error.rs @@ -54,16 +54,6 @@ impl ExecutionError { OutOfGas | Syscall(_) => false, } } - - /// Returns true if an actor can catch the error. All errors except fatal and out of gas errors - /// are recoverable. - pub fn is_recoverable(&self) -> bool { - use ExecutionError::*; - match self { - Syscall(_) => true, - OutOfGas | Fatal(_) => false, - } - } } // NOTE: this is the _only_ from impl we provide. Otherwise, we expect the user to explicitly From 19138681706b57d7bb4eed2a7d25b43b2bdc8f56 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Wed, 18 Oct 2023 11:46:22 +0000 Subject: [PATCH 29/40] Reject upgrades if actor already on call stack --- fvm/src/call_manager/default.rs | 12 ++++++++++++ fvm/src/call_manager/mod.rs | 4 ++++ fvm/src/kernel/default.rs | 14 ++++++++++++++ fvm/src/kernel/mod.rs | 2 ++ fvm/src/syscalls/actor.rs | 8 ++++++++ fvm/tests/dummy.rs | 9 +++++++++ testing/conformance/src/vm.rs | 4 ++++ 7 files changed, 53 insertions(+) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 1c1d0f76d..d2f46e5f6 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -14,6 +14,7 @@ use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; use fvm_shared::{ActorID, METHOD_SEND}; use num_traits::Zero; +use std::collections::HashMap; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; use super::{Backtrace, CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; @@ -75,6 +76,8 @@ pub struct InnerDefaultCallManager { limits: M::Limiter, /// Accumulator for events emitted in this call stack. events: EventsAccumulator, + /// A map of ActorID and how often they appear on the call stack. + actor_call_stack: HashMap, } #[doc(hidden)] @@ -159,6 +162,7 @@ where limits, events: Default::default(), state_access_tracker, + actor_call_stack: HashMap::new(), }))) } @@ -327,6 +331,14 @@ where self.nonce } + fn get_actor_call_stack(&self) -> &HashMap { + &self.actor_call_stack + } + + fn get_actor_call_stack_mut(&mut self) -> &mut HashMap { + &mut self.actor_call_stack + } + fn next_actor_address(&self) -> Address { // Base the next address on the address specified as the message origin. This lets us use, // e.g., an f2 address even if we can't look it up anywhere. diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 2e5b2d7fd..4ef7fd3ee 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -7,6 +7,7 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum}; +use std::collections::HashMap; use crate::engine::Engine; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; @@ -119,6 +120,9 @@ pub trait CallManager: 'static { delegated_address: Option
, ) -> Result<()>; + fn get_actor_call_stack(&self) -> &HashMap; + fn get_actor_call_stack_mut(&mut self) -> &mut HashMap; + /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 72c024d80..9ed60f14e 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -137,6 +137,12 @@ where return Err(syscall_error!(LimitExceeded; "cannot store return block").into()); } + self.call_manager + .get_actor_call_stack_mut() + .entry(self.actor_id) + .and_modify(|count| *count += 1) + .or_insert(1); + // Send. let result = self.call_manager.with_transaction(|cm| { cm.call_actor::( @@ -873,6 +879,14 @@ where .create_actor(code_id, actor_id, delegated_address) } + fn is_actor_on_call_stack(&self) -> bool { + self.call_manager + .get_actor_call_stack() + .get(&self.actor_id) + .map(|count| *count > 0) + .unwrap_or(false) + } + fn upgrade_actor( &mut self, new_code_cid: Cid, diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index a47338003..9af6c7851 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -215,6 +215,8 @@ pub trait ActorOps { params_id: BlockId, ) -> Result; + fn is_actor_on_call_stack(&self) -> bool; + /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] fn install_actor(&mut self, code_cid: Cid) -> Result<()>; diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 9aeba3038..2f694b1b6 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -121,6 +121,14 @@ pub fn upgrade_actor( Err(err) => return err.into(), }; + // actor cannot be upgraded if its already on the call stack + if context.kernel.is_actor_on_call_stack() { + return ControlFlow::Error(syscall_error!( + Forbidden; + "cannot upgrade actor if it is already on the call stack" + )); + }; + match context.kernel.upgrade_actor::(cid, params_id) { Ok(SendResult { block_id, diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 677d9a0b5..1994a79e4 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0, MIT use std::borrow::Borrow; use std::cell::RefCell; +use std::collections::HashMap; use std::rc::Rc; use anyhow::Context; @@ -358,6 +359,14 @@ impl CallManager for DummyCallManager { todo!() } + fn get_actor_call_stack(&self) -> &HashMap { + todo!() + } + + fn get_actor_call_stack_mut(&mut self) -> &mut HashMap { + todo!() + } + fn invocation_count(&self) -> u64 { todo!() } diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index 549cbd251..b43421da7 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -302,6 +302,10 @@ where fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { self.0.upgrade_actor::(new_code_cid, params_id) } + + fn is_actor_on_call_stack(&self) -> bool { + self.0.is_actor_on_call_stack() + } } impl IpldBlockOps for TestKernel From c7e3498ac5d86a31448a507ce806a0e8d75b4cb2 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Wed, 18 Oct 2023 14:17:29 +0000 Subject: [PATCH 30/40] Add test for calling upgrade on an actor alread on call stack --- testing/integration/tests/main.rs | 24 ++++++++++++++++++- .../actors/fil-upgrade-actor/src/actor.rs | 21 ++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index b40f169db..fc3de3e9c 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -1115,7 +1115,7 @@ fn upgrade_actor_test() { ) .unwrap(); - let sender: [Account; 3] = tester.create_accounts().unwrap(); + let sender: [Account; 4] = tester.create_accounts().unwrap(); let receiver = Address::new_id(10000); let state_cid = tester.set_state(&[(); 0]).unwrap(); @@ -1197,6 +1197,28 @@ fn upgrade_actor_test() { res.failure_info ); } + + { + let message = Message { + from: sender[3].1, + to: receiver, + gas_limit: 1000000000, + method_num: 4, + sequence: 0_u64, + value: TokenAmount::from_atto(100), + ..Message::default() + }; + + let res = executor + .execute_message(message, ApplyKind::Explicit, 100) + .unwrap(); + + assert!( + res.msg_receipt.exit_code.is_success(), + "{:?}", + res.failure_info + ); + } } #[derive(Default)] diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index 525d25ec7..cb0c43386 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -4,6 +4,8 @@ use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{to_vec, CBOR}; use fvm_sdk as sdk; use fvm_shared::address::Address; +use fvm_shared::econ::TokenAmount; +use fvm_shared::error::ErrorNumber; use fvm_shared::upgrade::UpgradeInfo; use serde_tuple::*; #[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] @@ -95,6 +97,25 @@ pub fn invoke(_: u32) -> u32 { let _ = sdk::actor::upgrade_actor(new_code_cid, params); unreachable!("we should never return from a successful upgrade"); } + // test sending a message to ourself (putting us on the call stack) + 4 => { + sdk::send::send( + &Address::new_id(10000), + 5, + Default::default(), + TokenAmount::from_atto(100), + None, + Default::default(), + ) + .unwrap(); + } + // test that calling an upgrade with actor already on the call stack fails + 5 => { + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + let res = sdk::actor::upgrade_actor(new_code_cid, None); + assert_eq!(res, Err(ErrorNumber::Forbidden)); + } + other => { sdk::vm::abort( fvm_shared::error::ExitCode::FIRST_USER_EXIT_CODE, From 0f65803a27e02139390aef38776c76017c65afb0 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 19 Oct 2023 18:35:45 +0000 Subject: [PATCH 31/40] Add actor call stack to call manager + detect reentrant upgrades in syscall --- fvm/src/call_manager/default.rs | 23 +++++++++++++------- fvm/src/call_manager/mod.rs | 18 +++++++++++----- fvm/src/kernel/default.rs | 35 +++++++++++++++++++------------ fvm/src/kernel/mod.rs | 2 +- fvm/src/syscalls/actor.rs | 5 ++--- fvm/tests/dummy.rs | 9 +++++--- testing/conformance/src/vm.rs | 4 ++-- testing/integration/tests/main.rs | 2 +- 8 files changed, 62 insertions(+), 36 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index d2f46e5f6..4cd6abcc6 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -14,7 +14,6 @@ use fvm_shared::event::StampedEvent; use fvm_shared::sys::BlockId; use fvm_shared::{ActorID, METHOD_SEND}; use num_traits::Zero; -use std::collections::HashMap; use super::state_access_tracker::{ActorAccessState, StateAccessTracker}; use super::{Backtrace, CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; @@ -76,8 +75,8 @@ pub struct InnerDefaultCallManager { limits: M::Limiter, /// Accumulator for events emitted in this call stack. events: EventsAccumulator, - /// A map of ActorID and how often they appear on the call stack. - actor_call_stack: HashMap, + /// The actor call stack (ActorID and entrypoint name tuple). + actor_call_stack: Vec<(ActorID, &'static str)>, } #[doc(hidden)] @@ -162,7 +161,7 @@ where limits, events: Default::default(), state_access_tracker, - actor_call_stack: HashMap::new(), + actor_call_stack: vec![], }))) } @@ -331,12 +330,16 @@ where self.nonce } - fn get_actor_call_stack(&self) -> &HashMap { + fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> { &self.actor_call_stack } - fn get_actor_call_stack_mut(&mut self) -> &mut HashMap { - &mut self.actor_call_stack + fn actor_call_stack_push(&mut self, actor_id: ActorID, entrypoint: &Entrypoint) { + self.actor_call_stack + .push((actor_id, entrypoint.func_name())) + } + fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)> { + self.actor_call_stack.pop() } fn next_actor_address(&self) -> Address { @@ -639,7 +642,11 @@ where }, }; - self.send_resolved::(from, to, entrypoint, params, value, read_only) + self.actor_call_stack_push(to, &entrypoint); + let res = self.send_resolved::(from, to, entrypoint, params, value, read_only); + self.actor_call_stack_pop(); + + res } /// Send with resolved addresses. diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 4ef7fd3ee..718a351a1 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -7,7 +7,6 @@ use fvm_shared::econ::TokenAmount; use fvm_shared::error::ExitCode; use fvm_shared::upgrade::UpgradeInfo; use fvm_shared::{ActorID, MethodNum}; -use std::collections::HashMap; use crate::engine::Engine; use crate::gas::{Gas, GasCharge, GasTimer, GasTracker, PriceList}; @@ -120,8 +119,14 @@ pub trait CallManager: 'static { delegated_address: Option
, ) -> Result<()>; - fn get_actor_call_stack(&self) -> &HashMap; - fn get_actor_call_stack_mut(&mut self) -> &mut HashMap; + // returns the actor call stack + fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)>; + + /// add an actor to the calling stack. + fn actor_call_stack_push(&mut self, actor_id: ActorID, entrypoint: &Entrypoint); + + /// pop an actor from the calling stack. + fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)>; /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; @@ -210,6 +215,9 @@ pub enum Entrypoint { Upgrade(UpgradeInfo), } +pub static INVOKE_FUNC_NAME: &str = "invoke"; +pub static UPGRADE_FUNC_NAME: &str = "upgrade"; + impl std::fmt::Display for Entrypoint { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { @@ -229,8 +237,8 @@ impl Entrypoint { fn func_name(&self) -> &'static str { match self { - Entrypoint::Invoke(_) => "invoke", - Entrypoint::Upgrade(_) => "upgrade", + Entrypoint::Invoke(_) => INVOKE_FUNC_NAME, + Entrypoint::Upgrade(_) => UPGRADE_FUNC_NAME, } } diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 9ed60f14e..3a51a2d19 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -30,7 +30,10 @@ use super::blocks::{Block, BlockRegistry}; use super::error::Result; use super::hash::SupportedHashes; use super::*; -use crate::call_manager::{CallManager, Entrypoint, InvocationResult, NO_DATA_BLOCK_ID}; +use crate::call_manager::{ + CallManager, Entrypoint, InvocationResult, INVOKE_FUNC_NAME, NO_DATA_BLOCK_ID, + UPGRADE_FUNC_NAME, +}; use crate::externs::{Chain, Consensus, Rand}; use crate::gas::GasTimer; use crate::init_actor::INIT_ACTOR_ID; @@ -137,12 +140,6 @@ where return Err(syscall_error!(LimitExceeded; "cannot store return block").into()); } - self.call_manager - .get_actor_call_stack_mut() - .entry(self.actor_id) - .and_modify(|count| *count += 1) - .or_insert(1); - // Send. let result = self.call_manager.with_transaction(|cm| { cm.call_actor::( @@ -879,12 +876,24 @@ where .create_actor(code_id, actor_id, delegated_address) } - fn is_actor_on_call_stack(&self) -> bool { - self.call_manager - .get_actor_call_stack() - .get(&self.actor_id) - .map(|count| *count > 0) - .unwrap_or(false) + 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( diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index 9af6c7851..16ce621cd 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -215,7 +215,7 @@ pub trait ActorOps { params_id: BlockId, ) -> Result; - fn is_actor_on_call_stack(&self) -> bool; + fn can_actor_upgrade(&self) -> bool; /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 2f694b1b6..6b5c9eaa5 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -121,11 +121,10 @@ pub fn upgrade_actor( Err(err) => return err.into(), }; - // actor cannot be upgraded if its already on the call stack - if context.kernel.is_actor_on_call_stack() { + if !context.kernel.can_actor_upgrade() { return ControlFlow::Error(syscall_error!( Forbidden; - "cannot upgrade actor if it is already on the call stack" + "calling upgrade on actor already on call stack is forbidden" )); }; diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 1994a79e4..424cdedda 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0, MIT use std::borrow::Borrow; use std::cell::RefCell; -use std::collections::HashMap; use std::rc::Rc; use anyhow::Context; @@ -359,11 +358,15 @@ impl CallManager for DummyCallManager { todo!() } - fn get_actor_call_stack(&self) -> &HashMap { + fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> { todo!() } - fn get_actor_call_stack_mut(&mut self) -> &mut HashMap { + fn actor_call_stack_push(&mut self, _actor_id: ActorID, _entrypoint: &Entrypoint) { + todo!() + } + + fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)> { todo!() } diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index b43421da7..c8b4d49ef 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -303,8 +303,8 @@ where self.0.upgrade_actor::(new_code_cid, params_id) } - fn is_actor_on_call_stack(&self) -> bool { - self.0.is_actor_on_call_stack() + fn can_actor_upgrade(&self) -> bool { + self.0.can_actor_upgrade() } } diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index fc3de3e9c..d621c7352 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -1189,7 +1189,7 @@ fn upgrade_actor_test() { .unwrap(); let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); - assert_eq!(val, 444); + assert_eq!(val, 444, "{:?}", res.failure_info); assert!( res.msg_receipt.exit_code.is_success(), From 816ed065d57b94a5af3af38b1f2e6b28ff907a76 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 20 Oct 2023 10:21:26 +0000 Subject: [PATCH 32/40] simplify call_manager interface for actor call stack --- fvm/src/call_manager/default.rs | 14 +++----------- fvm/src/call_manager/mod.rs | 8 +------- fvm/tests/dummy.rs | 10 +--------- 3 files changed, 5 insertions(+), 27 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 4cd6abcc6..81c89adc0 100644 --- a/fvm/src/call_manager/default.rs +++ b/fvm/src/call_manager/default.rs @@ -330,18 +330,10 @@ where self.nonce } - fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> { + fn get_actor_call_stack(&self) -> &[(ActorID, &'static str)] { &self.actor_call_stack } - fn actor_call_stack_push(&mut self, actor_id: ActorID, entrypoint: &Entrypoint) { - self.actor_call_stack - .push((actor_id, entrypoint.func_name())) - } - fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)> { - self.actor_call_stack.pop() - } - fn next_actor_address(&self) -> Address { // Base the next address on the address specified as the message origin. This lets us use, // e.g., an f2 address even if we can't look it up anywhere. @@ -642,9 +634,9 @@ where }, }; - self.actor_call_stack_push(to, &entrypoint); + self.actor_call_stack.push((to, entrypoint.func_name())); let res = self.send_resolved::(from, to, entrypoint, params, value, read_only); - self.actor_call_stack_pop(); + self.actor_call_stack.pop(); res } diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 718a351a1..2ea808016 100644 --- a/fvm/src/call_manager/mod.rs +++ b/fvm/src/call_manager/mod.rs @@ -120,13 +120,7 @@ pub trait CallManager: 'static { ) -> Result<()>; // returns the actor call stack - fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)>; - - /// add an actor to the calling stack. - fn actor_call_stack_push(&mut self, actor_id: ActorID, entrypoint: &Entrypoint); - - /// pop an actor from the calling stack. - fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)>; + fn get_actor_call_stack(&self) -> &[(ActorID, &'static str)]; /// Resolve an address into an actor ID, charging gas as appropriate. fn resolve_address(&self, address: &Address) -> Result>; diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 424cdedda..9f7ff1d0b 100644 --- a/fvm/tests/dummy.rs +++ b/fvm/tests/dummy.rs @@ -358,15 +358,7 @@ impl CallManager for DummyCallManager { todo!() } - fn get_actor_call_stack(&self) -> &Vec<(ActorID, &'static str)> { - todo!() - } - - fn actor_call_stack_push(&mut self, _actor_id: ActorID, _entrypoint: &Entrypoint) { - todo!() - } - - fn actor_call_stack_pop(&mut self) -> Option<(ActorID, &'static str)> { + fn get_actor_call_stack(&self) -> &[(ActorID, &'static str)] { todo!() } From 79653b6b8ea8a328692cb4c10cf6dacf73015a41 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 24 Oct 2023 11:59:04 +0000 Subject: [PATCH 33/40] 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 | 57 ++++++++++++++++++------------- fvm/src/call_manager/mod.rs | 18 +++++----- fvm/src/kernel/default.rs | 39 +++++++++++---------- fvm/src/kernel/mod.rs | 2 -- fvm/src/syscalls/actor.rs | 7 ---- fvm/tests/dummy.rs | 2 +- shared/src/lib.rs | 2 -- testing/conformance/src/vm.rs | 4 --- testing/integration/tests/main.rs | 3 ++ 9 files changed, 66 insertions(+), 68 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 81c89adc0..c6ce6b98a 100644 --- a/fvm/src/call_manager/default.rs +++ b/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::(from, to, entrypoint, params, value, read_only) + s.call_actor_unchecked::(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::( + self.call_actor_resolved::( 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( + /// Call actor without checking the call depth and/or dealing with transactions. This must _only_ be + /// called from `call_actor`. + fn call_actor_unchecked( &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::(from, to, entrypoint, params, value, read_only); + let res = self.call_actor_resolved::(from, to, entrypoint, params, value, read_only); self.actor_call_stack.pop(); res } - /// Send with resolved addresses. - fn send_resolved( + /// Call actor with resolved addresses. + fn call_actor_resolved( &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, diff --git a/fvm/src/call_manager/mod.rs b/fvm/src/call_manager/mod.rs index 2ea808016..680c8ecda 100644 --- a/fvm/src/call_manager/mod.rs +++ b/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>( @@ -76,7 +76,7 @@ pub trait CallManager: 'static { read_only: bool, ) -> Result; - /// 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, @@ -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>; @@ -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, } } diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 3a51a2d19..4f221ff6a 100644 --- a/fvm/src/kernel/default.rs +++ b/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( &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 diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index 16ce621cd..a47338003 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -215,8 +215,6 @@ pub trait ActorOps { params_id: BlockId, ) -> Result; - 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<()>; diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 6b5c9eaa5..9aeba3038 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -121,13 +121,6 @@ pub fn upgrade_actor( 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::(cid, params_id) { Ok(SendResult { block_id, diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 9f7ff1d0b..1faf483c0 100644 --- a/fvm/tests/dummy.rs +++ b/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!() } diff --git a/shared/src/lib.rs b/shared/src/lib.rs index 40ea2eb56..1b31f717a 100644 --- a/shared/src/lib.rs +++ b/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)] diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index c8b4d49ef..549cbd251 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -302,10 +302,6 @@ where fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { self.0.upgrade_actor::(new_code_cid, params_id) } - - fn can_actor_upgrade(&self) -> bool { - self.0.can_actor_upgrade() - } } impl IpldBlockOps for TestKernel diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index d621c7352..d00fe7e3f 100644 --- a/testing/integration/tests/main.rs +++ b/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, From caf82001764f5d7d6b42ada756794ce75bd5307e Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 24 Oct 2023 16:21:41 +0000 Subject: [PATCH 34/40] Rename SendResult to CallResult --- fvm/src/kernel/default.rs | 10 +++++----- fvm/src/kernel/mod.rs | 6 +++--- fvm/src/syscalls/actor.rs | 4 ++-- fvm/src/syscalls/send.rs | 4 ++-- testing/conformance/src/vm.rs | 4 ++-- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 4f221ff6a..4d46c2ed1 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -120,7 +120,7 @@ where value: &TokenAmount, gas_limit: Option, flags: SendFlags, - ) -> Result { + ) -> Result { let from = self.actor_id; let read_only = self.read_only || flags.read_only(); @@ -169,7 +169,7 @@ where .put_reachable(blk) .or_fatal() .context("failed to store a valid return value")?; - SendResult { + CallResult { block_id, block_stat, exit_code, @@ -178,7 +178,7 @@ where InvocationResult { exit_code, value: None, - } => SendResult { + } => CallResult { block_id: NO_DATA_BLOCK_ID, block_stat: BlockStat { codec: 0, size: 0 }, exit_code, @@ -880,7 +880,7 @@ where &mut self, new_code_cid: Cid, params_id: BlockId, - ) -> Result { + ) -> Result { if self.read_only { return Err( syscall_error!(ReadOnly, "upgrade_actor cannot be called while read-only").into(), @@ -973,7 +973,7 @@ where None => (BlockStat { codec: 0, size: 0 }, NO_DATA_BLOCK_ID), Some(block) => (block.stat(), self.blocks.put_reachable(block)?), }; - Ok(SendResult { + Ok(CallResult { block_id, block_stat, exit_code, diff --git a/fvm/src/kernel/mod.rs b/fvm/src/kernel/mod.rs index a47338003..02a5f38ef 100644 --- a/fvm/src/kernel/mod.rs +++ b/fvm/src/kernel/mod.rs @@ -38,7 +38,7 @@ use crate::gas::{Gas, GasTimer, PriceList}; use crate::machine::limiter::MemoryLimiter; use crate::machine::Machine; -pub struct SendResult { +pub struct CallResult { pub block_id: BlockId, pub block_stat: BlockStat, pub exit_code: ExitCode, @@ -112,7 +112,7 @@ pub trait Kernel: value: &TokenAmount, gas_limit: Option, flags: SendFlags, - ) -> Result; + ) -> Result; } /// Network-related operations. @@ -213,7 +213,7 @@ pub trait ActorOps { &mut self, new_code_cid: Cid, params_id: BlockId, - ) -> Result; + ) -> Result; /// Installs actor code pointed by cid #[cfg(feature = "m2-native")] diff --git a/fvm/src/syscalls/actor.rs b/fvm/src/syscalls/actor.rs index 9aeba3038..4c790a0db 100644 --- a/fvm/src/syscalls/actor.rs +++ b/fvm/src/syscalls/actor.rs @@ -6,7 +6,7 @@ use fvm_shared::{sys, ActorID}; use super::bind::ControlFlow; use super::error::Abort; use super::Context; -use crate::kernel::{ClassifyResult, Result, SendResult}; +use crate::kernel::{CallResult, ClassifyResult, Result}; use crate::{syscall_error, Kernel}; pub fn resolve_address( @@ -122,7 +122,7 @@ pub fn upgrade_actor( }; match context.kernel.upgrade_actor::(cid, params_id) { - Ok(SendResult { + Ok(CallResult { block_id, block_stat, exit_code, diff --git a/fvm/src/syscalls/send.rs b/fvm/src/syscalls/send.rs index edf51cb1f..fbb450d42 100644 --- a/fvm/src/syscalls/send.rs +++ b/fvm/src/syscalls/send.rs @@ -7,7 +7,7 @@ use fvm_shared::sys::{self, SendFlags}; use super::Context; use crate::gas::Gas; -use crate::kernel::{ClassifyResult, Result, SendResult}; +use crate::kernel::{CallResult, ClassifyResult, Result}; use crate::Kernel; /// Send a message to another actor. The result is placed as a CBOR-encoded @@ -37,7 +37,7 @@ pub fn send( // An execution error here means that something went wrong in the FVM. // Actor errors are communicated in the receipt. - let SendResult { + let CallResult { block_id, block_stat, exit_code, diff --git a/testing/conformance/src/vm.rs b/testing/conformance/src/vm.rs index 549cbd251..26ea2beb7 100644 --- a/testing/conformance/src/vm.rs +++ b/testing/conformance/src/vm.rs @@ -241,7 +241,7 @@ where value: &TokenAmount, gas_limit: Option, flags: SendFlags, - ) -> Result { + ) -> Result { // Note that KK, the type of the kernel to crate for the receiving actor, is ignored, // and Self is passed as the type parameter for the nested call. // If we could find the correct bound to specify KK for the call, we would. @@ -299,7 +299,7 @@ where self.0.lookup_delegated_address(actor_id) } - fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { + fn upgrade_actor(&mut self, new_code_cid: Cid, params_id: BlockId) -> Result { self.0.upgrade_actor::(new_code_cid, params_id) } } From f55904d87c0df34d80718d4208b33d83daa5d242 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 24 Oct 2023 16:33:05 +0000 Subject: [PATCH 35/40] fix: take cid as ref in upgrade_actor --- sdk/src/actor.rs | 2 +- .../test_actors/actors/fil-syscall-actor/src/actor.rs | 7 ++----- .../test_actors/actors/fil-upgrade-actor/src/actor.rs | 10 +++++----- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/sdk/src/actor.rs b/sdk/src/actor.rs index 0269fbaa1..e2f96c272 100644 --- a/sdk/src/actor.rs +++ b/sdk/src/actor.rs @@ -108,7 +108,7 @@ pub fn create_actor( } /// Upgrades an actor using the given block which includes the old code cid and the upgrade params -pub fn upgrade_actor(new_code_cid: Cid, params: Option) -> SyscallResult { +pub fn upgrade_actor(new_code_cid: &Cid, params: Option) -> SyscallResult { unsafe { let cid = new_code_cid.to_bytes(); diff --git a/testing/test_actors/actors/fil-syscall-actor/src/actor.rs b/testing/test_actors/actors/fil-syscall-actor/src/actor.rs index 6098497b3..4b009e81a 100644 --- a/testing/test_actors/actors/fil-syscall-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-syscall-actor/src/actor.rs @@ -382,11 +382,8 @@ fn test_unaligned() { fn test_upgrade() { // test that calling `upgrade_actor` on ourselves results in a SYS_INVALID_RECEIVER error // since we don't have a upgrade endpoint - let res = sdk::actor::upgrade_actor( - sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(), - None, - ) - .unwrap(); + let code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + let res = sdk::actor::upgrade_actor(&code_cid, None).unwrap(); assert_eq!( res.exit_code, diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index cb0c43386..908604d37 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -50,7 +50,7 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { sdk::debug::log("[upgrade] params:3, calling upgrade within an upgrade".to_string()); let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 4 }).unwrap(); - let _ = sdk::actor::upgrade_actor(new_code_cid, params); + let _ = sdk::actor::upgrade_actor(&new_code_cid, params); unreachable!("we should never return from a successful upgrade"); } 4 => { @@ -76,14 +76,14 @@ pub fn invoke(_: u32) -> u32 { 1 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 1 }).unwrap(); - let _ = sdk::actor::upgrade_actor(new_code_cid, params); + let _ = sdk::actor::upgrade_actor(&new_code_cid, params); unreachable!("we should never return from a successful upgrade"); } // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code 2 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 2 }).unwrap(); - let res = sdk::actor::upgrade_actor(new_code_cid, params).unwrap(); + let res = sdk::actor::upgrade_actor(&new_code_cid, params).unwrap(); assert_eq!( UPGRADE_FAILED_EXIT_CODE, res.exit_code.value(), @@ -94,7 +94,7 @@ pub fn invoke(_: u32) -> u32 { 3 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 3 }).unwrap(); - let _ = sdk::actor::upgrade_actor(new_code_cid, params); + let _ = sdk::actor::upgrade_actor(&new_code_cid, params); unreachable!("we should never return from a successful upgrade"); } // test sending a message to ourself (putting us on the call stack) @@ -112,7 +112,7 @@ pub fn invoke(_: u32) -> u32 { // test that calling an upgrade with actor already on the call stack fails 5 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - let res = sdk::actor::upgrade_actor(new_code_cid, None); + let res = sdk::actor::upgrade_actor(&new_code_cid, None); assert_eq!(res, Err(ErrorNumber::Forbidden)); } From dc66e33fb0fecc6243eb1a5571d8914619cf8655 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Tue, 24 Oct 2023 17:02:51 +0000 Subject: [PATCH 36/40] Refactor tests + add testcase for upgrading after self_destruct --- fvm/src/kernel/default.rs | 2 +- fvm/src/syscalls/bind.rs | 2 + testing/integration/tests/main.rs | 120 +++++++----------- .../actors/fil-upgrade-actor/src/actor.rs | 11 +- 4 files changed, 58 insertions(+), 77 deletions(-) diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 4d46c2ed1..01d525bfa 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -923,7 +923,7 @@ where let state = cm .get_actor(self.actor_id)? - .ok_or_else(|| syscall_error!(NotFound; "actor not found"))?; + .ok_or_else(|| syscall_error!(IllegalOperation; "actor deleted"))?; // store the code cid of the calling actor before running the upgrade entrypoint // in case it was changed (which could happen if the target upgrade entrypoint diff --git a/fvm/src/syscalls/bind.rs b/fvm/src/syscalls/bind.rs index f40317ea2..5ab2598b6 100644 --- a/fvm/src/syscalls/bind.rs +++ b/fvm/src/syscalls/bind.rs @@ -49,6 +49,8 @@ pub(super) trait BindSyscall { ) -> anyhow::Result<&mut Self>; } +/// ControlFlow is a general-purpose enum allowing us to pass syscall error up the +/// stack to the actor and treat error handling there (decide when to abort, etc). pub enum ControlFlow { Return(T), Error(SyscallError), diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index d00fe7e3f..f70d8d355 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -1115,7 +1115,7 @@ fn upgrade_actor_test() { ) .unwrap(); - let sender: [Account; 4] = tester.create_accounts().unwrap(); + let sender: [Account; 5] = tester.create_accounts().unwrap(); let receiver = Address::new_id(10000); let state_cid = tester.set_state(&[(); 0]).unwrap(); @@ -1127,60 +1127,53 @@ fn upgrade_actor_test() { let executor = tester.executor.as_mut().unwrap(); - { - // test a successful call to `upgrade` endpoint - let message = Message { - from: sender[0].1, - to: receiver, - gas_limit: 1000000000, - method_num: 1, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); - assert_eq!(val, 666); + struct Case { + from: Address, + method_num: u64, + return_data: Option, } - { - // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code - let message = Message { - from: sender[1].1, - to: receiver, - gas_limit: 1000000000, - method_num: 2, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); - } + let cases = { + [ + // test that successful calls to `upgrade_actor` does not return + Case { + from: sender[0].1, + method_num: 1, + return_data: Some(666), + }, + // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code + Case { + from: sender[1].1, + method_num: 2, + return_data: None, + }, + // test recursive update + Case { + from: sender[2].1, + method_num: 3, + return_data: Some(444), + }, + // test sending a message to ourself (putting us on the call stack) + Case { + from: sender[3].1, + method_num: 4, + return_data: None, + }, + // test that calling an upgrade after self destruct fails with IllegalOperation + Case { + from: sender[4].1, + method_num: 5, + return_data: None, + }, + ] + }; - { - // test recursive update + for case in cases.into_iter() { let message = Message { - from: sender[2].1, + from: case.from, to: receiver, gas_limit: 1000000000, - method_num: 3, + method_num: case.method_num, sequence: 0_u64, value: TokenAmount::from_atto(100), ..Message::default() @@ -1190,37 +1183,16 @@ fn upgrade_actor_test() { .execute_message(message, ApplyKind::Explicit, 100) .unwrap(); - let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); - assert_eq!(val, 444, "{:?}", res.failure_info); - assert!( res.msg_receipt.exit_code.is_success(), "{:?}", res.failure_info ); - } - - { - // test sending a message to ourself (putting us on the call stack) - let message = Message { - from: sender[3].1, - to: receiver, - gas_limit: 1000000000, - method_num: 4, - sequence: 0_u64, - value: TokenAmount::from_atto(100), - ..Message::default() - }; - let res = executor - .execute_message(message, ApplyKind::Explicit, 100) - .unwrap(); - - assert!( - res.msg_receipt.exit_code.is_success(), - "{:?}", - res.failure_info - ); + if let Some(return_data) = case.return_data { + let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); + assert_eq!(val, return_data); + } } } diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index 908604d37..f34ee1dc3 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -101,7 +101,7 @@ pub fn invoke(_: u32) -> u32 { 4 => { sdk::send::send( &Address::new_id(10000), - 5, + 99, Default::default(), TokenAmount::from_atto(100), None, @@ -109,8 +109,15 @@ pub fn invoke(_: u32) -> u32 { ) .unwrap(); } - // test that calling an upgrade with actor already on the call stack fails + // test that calling an upgrade after self destruct fails with IllegalOperation 5 => { + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + sdk::sself::self_destruct(true).unwrap(); + let res = sdk::actor::upgrade_actor(&new_code_cid, None); + assert_eq!(res, Err(ErrorNumber::IllegalOperation)); + } + // test that calling an upgrade with actor already on the call stack fails + 99 => { let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); let res = sdk::actor::upgrade_actor(&new_code_cid, None); assert_eq!(res, Err(ErrorNumber::Forbidden)); From 700837c455b8574f0cfc7812c777cd21d2ff2eaa Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 26 Oct 2023 12:59:35 +0000 Subject: [PATCH 37/40] fix: used wrong id when updating the code_cid in upgrade_actor --- fvm/src/kernel/default.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 01d525bfa..416eb210a 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -919,8 +919,6 @@ where } let result = self.call_manager.with_transaction(|cm| { - let origin = cm.origin(); - let state = cm .get_actor(self.actor_id)? .ok_or_else(|| syscall_error!(IllegalOperation; "actor deleted"))?; @@ -932,7 +930,7 @@ where // update the code cid of the actor to new_code_cid cm.set_actor( - origin, + self.actor_id, ActorState::new( new_code_cid, state.state, From 7d0021817411b322a220c88efd0ef927062395a7 Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Thu, 26 Oct 2023 13:00:24 +0000 Subject: [PATCH 38/40] Refactor and improve test cases for testing upgrade_actor - Now checking correctly that the code cid changed - Refactored tests into test cases which makes them more readable - Added new upgrade receiver actor so we can test upgrades with different actors - Added test case for testing failure in a recursive upgrade --- Cargo.lock | 12 ++ fvm/src/kernel/default.rs | 11 -- testing/integration/tests/main.rs | 110 ++++++++++++++---- .../actors/fil-upgrade-actor/src/actor.rs | 6 +- .../fil-upgrade-receive-actor/Cargo.toml | 16 +++ .../fil-upgrade-receive-actor/src/actor.rs | 74 ++++++++++++ .../fil-upgrade-receive-actor/src/lib.rs | 4 + testing/test_actors/build.rs | 1 + 8 files changed, 197 insertions(+), 37 deletions(-) create mode 100644 testing/test_actors/actors/fil-upgrade-receive-actor/Cargo.toml create mode 100644 testing/test_actors/actors/fil-upgrade-receive-actor/src/actor.rs create mode 100644 testing/test_actors/actors/fil-upgrade-receive-actor/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 6642bb060..da7dd74f2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1941,6 +1941,18 @@ dependencies = [ "serde_tuple", ] +[[package]] +name = "fil_upgrade_receive_actor" +version = "0.1.0" +dependencies = [ + "cid 0.10.1", + "fvm_ipld_encoding 0.4.0", + "fvm_sdk 4.0.0-alpha.4", + "fvm_shared 4.0.0-alpha.4", + "serde", + "serde_tuple", +] + [[package]] name = "filecoin-hashers" version = "11.0.0" diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index 416eb210a..6e20873a0 100644 --- a/fvm/src/kernel/default.rs +++ b/fvm/src/kernel/default.rs @@ -951,17 +951,6 @@ where false, )?; - if result.exit_code.is_success() { - // after running the upgrade, our code cid must not have changed - let code_after_upgrade = cm - .get_actor(self.actor_id)? - .ok_or_else(|| syscall_error!(NotFound; "actor not found"))? - .code; - if code != code_after_upgrade { - return Err(syscall_error!(Forbidden; "re-entrant upgrade detected").into()); - } - } - Ok(result) }); diff --git a/testing/integration/tests/main.rs b/testing/integration/tests/main.rs index f70d8d355..d2345f5ec 100644 --- a/testing/integration/tests/main.rs +++ b/testing/integration/tests/main.rs @@ -7,8 +7,9 @@ use std::rc::Rc; use anyhow::anyhow; use cid::Cid; use fvm::executor::{ApplyKind, Executor, ThreadedExecutor}; +use fvm::machine::Machine; use fvm_integration_tests::dummy::DummyExterns; -use fvm_integration_tests::tester::{Account, IntegrationExecutor}; +use fvm_integration_tests::tester::{Account, IntegrationExecutor, Tester}; use fvm_ipld_blockstore::{Blockstore, MemoryBlockstore}; use fvm_ipld_encoding::tuple::*; use fvm_ipld_encoding::RawBytes; @@ -22,6 +23,7 @@ use fvm_test_actors::wasm_bin::{ ADDRESS_ACTOR_BINARY, CREATE_ACTOR_BINARY, EXIT_DATA_ACTOR_BINARY, HELLO_WORLD_ACTOR_BINARY, IPLD_ACTOR_BINARY, OOM_ACTOR_BINARY, READONLY_ACTOR_BINARY, SSELF_ACTOR_BINARY, STACK_OVERFLOW_ACTOR_BINARY, SYSCALL_ACTOR_BINARY, UPGRADE_ACTOR_BINARY, + UPGRADE_RECEIVE_ACTOR_BINARY, }; use num_traits::Zero; @@ -1108,69 +1110,119 @@ fn readonly_actor_tests() { #[test] fn upgrade_actor_test() { - let mut tester = new_tester( - NetworkVersion::V21, - StateTreeVersion::V5, - MemoryBlockstore::default(), - ) - .unwrap(); + // inline function to calculate cid from address + let calc_cid_func = |bytes: &[u8]| -> Cid { + fvm_ipld_blockstore::Block { + codec: fvm_shared::IPLD_RAW, + data: bytes, + } + .cid(multihash::Code::Blake2b256) + }; - let sender: [Account; 5] = tester.create_accounts().unwrap(); let receiver = Address::new_id(10000); - let state_cid = tester.set_state(&[(); 0]).unwrap(); - - let wasm_bin = UPGRADE_ACTOR_BINARY; - tester - .set_actor_from_bin(wasm_bin, state_cid, receiver, TokenAmount::zero()) + let receiver2 = Address::new_id(10001); + let receiver3 = Address::new_id(10002); + + // inline function to reset the tester framework so we can have clean slate between test cases + let init_tester = || -> (Tester, [Account; 1]) { + let mut tester = new_tester( + NetworkVersion::V21, + StateTreeVersion::V5, + MemoryBlockstore::default(), + ) .unwrap(); - tester.instantiate_machine(DummyExterns).unwrap(); - let executor = tester.executor.as_mut().unwrap(); + let sender: [Account; 1] = tester.create_accounts().unwrap(); + + let state_cid = tester.set_state(&[(); 0]).unwrap(); + + // UPGRADE_ACTOR_BINARY is our main actor where we will do the upgrade tests + tester + .set_actor_from_bin( + UPGRADE_ACTOR_BINARY, + state_cid, + receiver, + TokenAmount::zero(), + ) + .unwrap(); + + // UPGRADE_RECEIVE_ACTOR_BINARY is another actor to test recursive upgrade calls + tester + .set_actor_from_bin( + UPGRADE_RECEIVE_ACTOR_BINARY, + state_cid, + receiver2, + TokenAmount::zero(), + ) + .unwrap(); + + // lets have a third actor that will reject the upgrade (since it does not have + // an upgrade entrypoint) + tester + .set_actor_from_bin( + HELLO_WORLD_ACTOR_BINARY, + state_cid, + receiver3, + TokenAmount::zero(), + ) + .unwrap(); + + tester.instantiate_machine(DummyExterns).unwrap(); + + (tester, sender) + }; struct Case { - from: Address, + // the method inside invoke method_num: u64, + // if set, this is the expected receipt data return_data: Option, + // if set, this is the expected code cid after the upgrade + expected_cid: Option, } let cases = { [ - // test that successful calls to `upgrade_actor` does not return + // test that successful calls to `upgrade_actor` does not return and that the + // code cid is updated to the UPGRADE_RECEIVE_ACTOR_BINARY) Case { - from: sender[0].1, method_num: 1, return_data: Some(666), + expected_cid: Some(calc_cid_func(UPGRADE_RECEIVE_ACTOR_BINARY)), }, // test that when `upgrade` endpoint rejects upgrade that we get the returned exit code Case { - from: sender[1].1, method_num: 2, return_data: None, + expected_cid: None, }, // test recursive update Case { - from: sender[2].1, method_num: 3, return_data: Some(444), + expected_cid: Some(calc_cid_func(UPGRADE_RECEIVE_ACTOR_BINARY)), }, // test sending a message to ourself (putting us on the call stack) Case { - from: sender[3].1, method_num: 4, return_data: None, + expected_cid: None, }, // test that calling an upgrade after self destruct fails with IllegalOperation Case { - from: sender[4].1, method_num: 5, return_data: None, + expected_cid: None, }, ] }; for case in cases.into_iter() { + let (mut tester, sender) = init_tester(); + let executor = tester.executor.as_mut().unwrap(); + let message = Message { - from: case.from, + from: sender[0].1, to: receiver, gas_limit: 1000000000, method_num: case.method_num, @@ -1189,10 +1241,22 @@ fn upgrade_actor_test() { res.failure_info ); + // if this test case should return some data, check that it did if let Some(return_data) = case.return_data { let val: i64 = res.msg_receipt.return_data.deserialize().unwrap(); assert_eq!(val, return_data); } + + // if this test case should have changed the code cid, check that it did + if let Some(expected_cid) = case.expected_cid { + let code = executor + .state_tree() + .get_actor(receiver.id().unwrap()) + .unwrap() + .unwrap() + .code; + assert_eq!(code, expected_cid); + } } } diff --git a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs index f34ee1dc3..716b4c63d 100644 --- a/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs +++ b/testing/test_actors/actors/fil-upgrade-actor/src/actor.rs @@ -48,8 +48,8 @@ pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { } 3 => { sdk::debug::log("[upgrade] params:3, calling upgrade within an upgrade".to_string()); - let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); - let params = IpldBlock::serialize_cbor(&SomeStruct { value: 4 }).unwrap(); + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10001)).unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 2 }).unwrap(); let _ = sdk::actor::upgrade_actor(&new_code_cid, params); unreachable!("we should never return from a successful upgrade"); } @@ -74,7 +74,7 @@ pub fn invoke(_: u32) -> u32 { match sdk::message::method_number() { // test that successful calls to `upgrade_actor` does not return 1 => { - let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10000)).unwrap(); + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10001)).unwrap(); let params = IpldBlock::serialize_cbor(&SomeStruct { value: 1 }).unwrap(); let _ = sdk::actor::upgrade_actor(&new_code_cid, params); unreachable!("we should never return from a successful upgrade"); diff --git a/testing/test_actors/actors/fil-upgrade-receive-actor/Cargo.toml b/testing/test_actors/actors/fil-upgrade-receive-actor/Cargo.toml new file mode 100644 index 000000000..b8bfd01f4 --- /dev/null +++ b/testing/test_actors/actors/fil-upgrade-receive-actor/Cargo.toml @@ -0,0 +1,16 @@ +[package] +name = "fil_upgrade_receive_actor" +version = "0.1.0" +edition = "2021" +publish = false + +[target.'cfg(target_arch = "wasm32")'.dependencies] +fvm_sdk = { version = "4.0.0-alpha.4", path = "../../../../sdk" } +fvm_shared = { version = "4.0.0-alpha.4", path = "../../../../shared" } +fvm_ipld_encoding = { version = "0.4.0", path = "../../../../ipld/encoding" } +cid = { workspace = true } +serde = { version = "1.0.164", features = ["derive"] } +serde_tuple = "0.5.0" + +[lib] +crate-type = ["cdylib"] ## cdylib is necessary for Wasm build diff --git a/testing/test_actors/actors/fil-upgrade-receive-actor/src/actor.rs b/testing/test_actors/actors/fil-upgrade-receive-actor/src/actor.rs new file mode 100644 index 000000000..e15271a18 --- /dev/null +++ b/testing/test_actors/actors/fil-upgrade-receive-actor/src/actor.rs @@ -0,0 +1,74 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT +use fvm_ipld_encoding::ipld_block::IpldBlock; +use fvm_ipld_encoding::{to_vec, CBOR}; +use fvm_sdk as sdk; +use fvm_shared::address::Address; +use fvm_shared::error::ExitCode; +use fvm_shared::upgrade::UpgradeInfo; +use serde_tuple::*; + +#[derive(Serialize_tuple, Deserialize_tuple, PartialEq, Eq, Clone, Debug)] +struct SomeStruct { + value: u64, +} + +#[no_mangle] +pub fn upgrade(params_id: u32, upgrade_info_id: u32) -> u32 { + sdk::initialize(); + + let params = sdk::message::params_raw(params_id).unwrap().unwrap(); + let ui_params = sdk::message::params_raw(upgrade_info_id).unwrap().unwrap(); + + assert_eq!(params.codec, fvm_ipld_encoding::CBOR); + assert_eq!(ui_params.codec, fvm_ipld_encoding::CBOR); + + let p = params.deserialize::().unwrap(); + let ui = ui_params.deserialize::().unwrap(); + + sdk::debug::log(format!( + "[upgrade-receive-actor] value: {}, old_code_cid: {}", + p.value, ui.old_code_cid + )); + + match p.value { + 1 => { + // try upgrade to a cid which should fail since it does not implement the upgrade endpoint + let new_code_cid = sdk::actor::get_actor_code_cid(&Address::new_id(10002)).unwrap(); + let params = IpldBlock::serialize_cbor(&SomeStruct { value: 4 }).unwrap(); + let res = sdk::actor::upgrade_actor(&new_code_cid, params).unwrap(); + sdk::debug::log(format!("[upgrade-receive-actor] res: {:?}", res)); + assert_eq!( + res.exit_code, + ExitCode::SYS_INVALID_RECEIVER, + "expected invalid receiver error" + ); + + let block_id = sdk::ipld::put_block(CBOR, &to_vec(&666).unwrap()).unwrap(); + sdk::debug::log(format!( + "[upgrade] params:1, returning block_id {}", + block_id + )); + block_id + } + 2 => { + sdk::debug::log("[upgrade-receive-actor] params:2".to_string()); + let block_id = sdk::ipld::put_block(CBOR, &to_vec(&444).unwrap()).unwrap(); + sdk::debug::log(format!( + "[upgrade] params:2, returning block_id {}", + block_id + )); + block_id + } + other => { + panic!("unexpected value: {}", other); + } + } +} + +#[no_mangle] +pub fn invoke(_: u32) -> u32 { + sdk::initialize(); + sdk::debug::log("[upgrade-receive-actor] calling vm::exit()".to_string()); + sdk::vm::exit(1, None, None) +} diff --git a/testing/test_actors/actors/fil-upgrade-receive-actor/src/lib.rs b/testing/test_actors/actors/fil-upgrade-receive-actor/src/lib.rs new file mode 100644 index 000000000..1d4daedef --- /dev/null +++ b/testing/test_actors/actors/fil-upgrade-receive-actor/src/lib.rs @@ -0,0 +1,4 @@ +// Copyright 2021-2023 Protocol Labs +// SPDX-License-Identifier: Apache-2.0, MIT +#[cfg(target_arch = "wasm32")] +mod actor; diff --git a/testing/test_actors/build.rs b/testing/test_actors/build.rs index b3bc2c5e4..a3f118cb7 100644 --- a/testing/test_actors/build.rs +++ b/testing/test_actors/build.rs @@ -32,6 +32,7 @@ const ACTORS: &[(&str, &str)] = &[ ("OOM_ACTOR_BINARY", "fil_oom_actor"), ("SSELF_ACTOR_BINARY", "fil_sself_actor"), ("UPGRADE_ACTOR_BINARY", "fil_upgrade_actor"), + ("UPGRADE_RECEIVE_ACTOR_BINARY", "fil_upgrade_receive_actor"), ]; const WASM_TARGET: &str = "wasm32-unknown-unknown"; From 0a985554f63f1b6af1c0ad3ead7897f662f7d32f Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 26 Oct 2023 12:06:11 -0700 Subject: [PATCH 39/40] fixup the error conditions --- sdk/src/sys/actor.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/sdk/src/sys/actor.rs b/sdk/src/sys/actor.rs index ad93ca1c8..f93c096dc 100644 --- a/sdk/src/sys/actor.rs +++ b/sdk/src/sys/actor.rs @@ -152,13 +152,15 @@ super::fvm_syscalls! { /// /// # Errors /// - /// | Error | Reason | - /// |-----------------------|------------------------------------------------------| - /// | [`NotFound`] | there's no actor deployed with the target code cid. | - /// | [`InvalidHandle`] | parameters block not found. | - /// | [`LimitExceeded`] | recursion limit reached. | - /// | [`IllegalArgument`] | invalid code cid buffer. | - /// | [`Forbidden`] | target actor doesn't have an upgrade endpoint. | + /// | Error | Reason | + /// |-----------------------|-----------------------------------------------------------------| + /// | [`NotFound`] | no code with the specified CID has been deployed. | + /// | [`IllegalOperation`] | the actor has been deleted. | + /// | [`InvalidHandle`] | parameters block not found. | + /// | [`LimitExceeded`] | recursion limit reached. | + /// | [`IllegalArgument`] | invalid code cid buffer. | + /// | [`Forbidden`] | the actor is not allowed to upgrade (e.g., due to re-entrency). | + /// | [`ReadOnly`] | the actor is executing in read-only mode. | pub fn upgrade_actor( new_code_cid_off: *const u8, params: u32, From 84a47d13c15dda853ea56f7fc1e9d4d6810d63b1 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Thu, 26 Oct 2023 12:15:00 -0700 Subject: [PATCH 40/40] disable the actor-upgrade feature by default This is a minimal version of #1906 that still compiles the relevent code, just disables it at runtime. --- fvm/Cargo.toml | 1 + fvm/src/syscalls/mod.rs | 6 +++++- testing/integration/Cargo.toml | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/fvm/Cargo.toml b/fvm/Cargo.toml index b2acb5b97..c1ccba9e5 100644 --- a/fvm/Cargo.toml +++ b/fvm/Cargo.toml @@ -66,4 +66,5 @@ cuda-supraseal = ["filecoin-proofs-api/cuda-supraseal"] testing = [] arb = ["arbitrary", "quickcheck", "fvm_shared/arb", "cid/arb"] m2-native = [] +upgrade-actor = [] gas_calibration = [] diff --git a/fvm/src/syscalls/mod.rs b/fvm/src/syscalls/mod.rs index fda51081e..81490ccdf 100644 --- a/fvm/src/syscalls/mod.rs +++ b/fvm/src/syscalls/mod.rs @@ -269,7 +269,11 @@ pub fn bind_syscalls( linker.bind("actor", "get_actor_code_cid", actor::get_actor_code_cid)?; linker.bind("actor", "next_actor_address", actor::next_actor_address)?; linker.bind("actor", "create_actor", actor::create_actor)?; - linker.bind("actor", "upgrade_actor", actor::upgrade_actor)?; + if cfg!(feature = "upgrade-actor") { + // We disable/enable with the feature, but we always compile this code to ensure we don't + // accidentally break it. + linker.bind("actor", "upgrade_actor", actor::upgrade_actor)?; + } linker.bind( "actor", "get_builtin_actor_type", diff --git a/testing/integration/Cargo.toml b/testing/integration/Cargo.toml index b5660f33c..11de6a4a5 100644 --- a/testing/integration/Cargo.toml +++ b/testing/integration/Cargo.toml @@ -8,7 +8,7 @@ authors = ["Protocol Labs", "Filecoin Core Devs", "Polyphene"] repository = "https://github.com/filecoin-project/ref-fvm" [dependencies] -fvm = { version = "4.0.0-alpha.4", path = "../../fvm", default-features = false, features = ["testing"] } +fvm = { version = "4.0.0-alpha.4", path = "../../fvm", default-features = false, features = ["testing", "upgrade-actor"] } fvm_shared = { version = "4.0.0-alpha.4", path = "../../shared", features = ["testing"] } fvm_ipld_car = { version = "0.7.1", path = "../../ipld/car" } fvm_ipld_blockstore = { version = "0.2.0", path = "../../ipld/blockstore" }