From 6c352f56ca51e2254b1dfe52a5e2b50e8d48ea3b Mon Sep 17 00:00:00 2001 From: Fridrik Asmundsson Date: Fri, 1 Sep 2023 15:54:25 +0000 Subject: [PATCH] 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/blocks.rs | 10 ++--- fvm/src/kernel/default.rs | 36 +++++++++++++-- fvm/src/syscalls/error.rs | 11 ++++- fvm/tests/dummy.rs | 4 +- 6 files changed, 110 insertions(+), 33 deletions(-) diff --git a/fvm/src/call_manager/default.rs b/fvm/src/call_manager/default.rs index 247b9b1c0d..c684ed614c 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) }) @@ -967,6 +1001,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 3d83fdb0ad..0dd36f77ce 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/blocks.rs b/fvm/src/kernel/blocks.rs index 72d9005e8b..c134101d7f 100644 --- a/fvm/src/kernel/blocks.rs +++ b/fvm/src/kernel/blocks.rs @@ -1,7 +1,6 @@ // Copyright 2021-2023 Protocol Labs // SPDX-License-Identifier: Apache-2.0, MIT use std::convert::TryInto; -use std::rc::Rc; use fvm_ipld_encoding::ipld_block::IpldBlock; use fvm_ipld_encoding::{CBOR, DAG_CBOR}; @@ -36,10 +35,7 @@ pub struct BlockStat { #[derive(Debug, Clone)] pub struct Block { codec: u64, - // Unfortunately, we usually start with a vector/boxed buffer. If we used Rc<[u8]>, we'd have to - // copy the bytes. So we accept some indirection for reliable performance. - #[allow(clippy::redundant_allocation)] - data: Rc>, + data: Box<[u8]>, } impl Block { @@ -48,7 +44,7 @@ impl Block { // The extra allocation is basically nothing. Self { codec, - data: Rc::new(data.into()), + data: data.into(), } } @@ -92,7 +88,7 @@ impl From<&Block> for IpldBlock { fn from(b: &Block) -> Self { IpldBlock { codec: b.codec, - data: Vec::from(&**b.data), + data: Vec::from(&*b.data), } } } diff --git a/fvm/src/kernel/default.rs b/fvm/src/kernel/default.rs index de4ecff654..468b2fe1a0 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 blake2b_simd::Params; use byteorder::WriteBytesExt; @@ -18,7 +19,7 @@ use fvm_shared::chainid::ChainID; 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; use fvm_shared::piece::{zero_piece_commitment, PaddedPieceSize}; use fvm_shared::sector::RegisteredPoStProof::{StackedDRGWindow32GiBV1, StackedDRGWindow32GiBV1P1}; @@ -887,8 +888,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 b7ab044583..a05bcf501c 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,9 +83,15 @@ impl From for Abort { _ => Abort::Fatal(anyhow!("unexpected wasmtime trap: {}", trap)), }; }; + match e.downcast::() { Ok(abort) => abort, Err(e) => Abort::Fatal(e), } + + /*match e.source().and_then(|e| e.downcast_mut::()) { + Some(abort) => abort, + None => Abort::Fatal(e), + }*/ } } diff --git a/fvm/tests/dummy.rs b/fvm/tests/dummy.rs index 533bc5479d..45afbc3de7 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, {