Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: don't abuse serialization for type erasure #5813

Merged
merged 3 commits into from
Dec 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions core/primitives/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ use std::fmt::{Debug, Display};

use crate::hash::CryptoHash;
use near_rpc_error_macro::RpcError;
use near_vm_errors::{CompilationError, FunctionCallErrorSer, MethodResolveError, VMLogicError};
use near_vm_errors::{
AnyError, CompilationError, FunctionCallErrorSer, MethodResolveError, VMLogicError,
};

/// Error returned in the ExecutionOutcome in case of failure
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))]
Expand Down Expand Up @@ -61,9 +63,8 @@ pub enum RuntimeError {
ValidatorError(EpochError),
}

/// Error used by `RuntimeExt`. This error has to be serializable, because it's transferred through
/// the `VMLogicError`, which isn't aware of internal Runtime errors.
#[derive(BorshSerialize, BorshDeserialize, Debug, Clone, PartialEq, Eq)]
/// Error used by `RuntimeExt`.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum ExternalError {
/// Unexpected error which is typically related to the node storage corruption.
/// It's possible the input state is invalid or malicious.
Expand All @@ -74,12 +75,12 @@ pub enum ExternalError {

impl From<ExternalError> for VMLogicError {
fn from(err: ExternalError) -> Self {
VMLogicError::ExternalError(err.try_to_vec().expect("Borsh serialize cannot fail"))
VMLogicError::ExternalError(AnyError::new(err))
}
}

/// Internal
#[derive(BorshSerialize, BorshDeserialize, Debug, Clone, PartialEq, Eq)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum StorageError {
/// Key-value db internal failure
StorageInternalError,
Expand Down Expand Up @@ -742,7 +743,7 @@ impl Display for ActionErrorKind {
}
}

#[derive(Eq, PartialEq, BorshSerialize, BorshDeserialize, Clone)]
#[derive(Eq, PartialEq, Clone)]
pub enum EpochError {
/// Error calculating threshold from given stakes for given number of seats.
/// Only should happened if calling code doesn't check for integer value of stake > number of seats.
Expand Down
92 changes: 62 additions & 30 deletions runtime/near-vm-errors/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
#![doc = include_str!("../README.md")]

use std::fmt::{self, Error, Formatter};

use borsh::{BorshDeserialize, BorshSerialize};
use near_account_id::AccountId;
use near_rpc_error_macro::RpcError;
use serde::{Deserialize, Serialize};
use std::any::Any;
use std::fmt::{self, Error, Formatter};

#[derive(Debug, Clone, PartialEq, Eq)]
#[derive(Debug, PartialEq, Eq)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PartialEq and Eq impls here don't appear to be used at all. Is there any reason why we need to keep these implementations in place?

Comparing errors for equality (as opposed to matching their structure) in general seems like a pretty weird thing to need.

Copy link
Collaborator

@nagisa nagisa Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are dropped, then the whole AnyEq thing could be replaced with just Box<dyn std::error::Error + Send + Sync> fields. Which is pretty common way to do this kind of type erasure in Rust.

EDIT: or Box<dyn Any + Send + Sync> I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are used in our tests 😭 And yeah, this is also problematic, as it, eg, prevents us from using io::Error. I do want to fix that (by rewriting our test), but not in this PR.

pub enum VMError {
FunctionCallError(FunctionCallError),
/// Serialized external error from External trait implementation.
ExternalError(Vec<u8>),
/// Type erased error from `External` trait implementation.
ExternalError(AnyError),
/// An error that is caused by an operation on an inconsistent state.
/// E.g. an integer overflow by using a value from the given context.
InconsistentStateError(InconsistentStateError),
Expand Down Expand Up @@ -227,12 +227,12 @@ pub enum HostError {
AltBn128SerializationError { msg: String },
}

#[derive(Debug, Clone, PartialEq)]
#[derive(Debug, PartialEq)]
pub enum VMLogicError {
/// Errors coming from native Wasm VM.
HostError(HostError),
/// Serialized external error from External trait implementation.
ExternalError(Vec<u8>),
/// Type erased error from `External` trait implementation.
ExternalError(AnyError),
/// An error that is caused by an operation on an inconsistent state.
InconsistentStateError(InconsistentStateError),
}
Expand Down Expand Up @@ -268,14 +268,14 @@ impl From<PrepareError> for VMError {
}
}

impl From<&VMLogicError> for VMError {
fn from(err: &VMLogicError) -> Self {
impl From<VMLogicError> for VMError {
fn from(err: VMLogicError) -> Self {
match err {
VMLogicError::HostError(h) => {
VMError::FunctionCallError(FunctionCallError::HostError(h.clone()))
VMError::FunctionCallError(FunctionCallError::HostError(h))
}
VMLogicError::ExternalError(s) => VMError::ExternalError(s.clone()),
VMLogicError::InconsistentStateError(e) => VMError::InconsistentStateError(e.clone()),
VMLogicError::ExternalError(s) => VMError::ExternalError(s),
VMLogicError::InconsistentStateError(e) => VMError::InconsistentStateError(e),
}
}
}
Expand Down Expand Up @@ -432,27 +432,59 @@ impl std::fmt::Display for HostError {
}
}

pub mod hex_format {
use hex::{decode, encode};
/// Type-erased error used to shuttle some concrete error coming from `External`
/// through vm-logic.
///
/// The caller is supposed to downcast this to a concrete error type they should
/// know. This would be just `Box<dyn Any + Eq>` if the latter actually worked.
pub struct AnyError {
any: Box<dyn AnyEq>,
}

use serde::de;
use serde::{Deserialize, Deserializer, Serializer};
impl AnyError {
pub fn new<E: Any + Eq + Send + Sync + 'static>(err: E) -> AnyError {
AnyError { any: Box::new(err) }
}
pub fn downcast<E: Any + Eq + Send + Sync + 'static>(self) -> Result<E, ()> {
match self.any.into_any().downcast::<E>() {
Ok(it) => Ok(*it),
Err(_) => Err(()),
}
}
}

pub fn serialize<S, T>(data: T, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
T: AsRef<[u8]>,
{
serializer.serialize_str(&encode(data))
impl PartialEq for AnyError {
fn eq(&self, other: &Self) -> bool {
self.any.any_eq(&*other.any)
}
}

pub fn deserialize<'de, D, T>(deserializer: D) -> Result<T, D::Error>
where
D: Deserializer<'de>,
T: From<Vec<u8>>,
{
let s = String::deserialize(deserializer)?;
decode(&s).map_err(|err| de::Error::custom(err.to_string())).map(Into::into)
impl Eq for AnyError {}

impl fmt::Debug for AnyError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt::Debug::fmt(self.any.as_any(), f)
}
}

trait AnyEq: Any + Send + Sync {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this truly need to be using Any? Could std::error::Error be utilized? It also supports downcasting and is generally probably more accurately conveys the intent of what's happening here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question!

On the one hand, yes, this could use std::error::Error, and that does seem like a better fit (it's an error!). On the other hand, the intention here is subtly different -- we are not going to use this as a dynamic error, the sole purpose here is to downcast this to a concrete repr down the line. In this sense, Any is a better, because it does strictly what we need, and nothing more.

Perhaps more important thing here is practicality -- if I use Error here, than I'd have to write Display for ExternalError, but that would effectively be dead code.

So, I'd say let's keep this as Any. But thanks for the sugestions -- using Error didn't occure to me!

fn any_eq(&self, rhs: &dyn AnyEq) -> bool;
fn as_any(&self) -> &dyn Any;
fn into_any(self: Box<Self>) -> Box<dyn Any>;
}

impl<T: Any + Eq + Sized + Send + Sync> AnyEq for T {
fn any_eq(&self, rhs: &dyn AnyEq) -> bool {
match rhs.as_any().downcast_ref::<Self>() {
Some(rhs) => self == rhs,
None => false,
}
}
fn as_any(&self) -> &dyn Any {
&*self
}
fn into_any(self: Box<Self>) -> Box<dyn Any> {
self
}
}

Expand Down
2 changes: 1 addition & 1 deletion runtime/near-vm-runner/src/wasmer2_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ impl IntoVMError for wasmer::RuntimeError {
let error_msg = self.message();
let trap_code = self.clone().to_trap();
if let Ok(e) = self.downcast::<VMLogicError>() {
return (&e).into();
return e.into();
}
// If we panic here - it means we encountered an issue in Wasmer.
let trap_code = trap_code.unwrap_or_else(|| panic!("Unknown error: {}", error_msg));
Expand Down
21 changes: 9 additions & 12 deletions runtime/near-vm-runner/src/wasmer_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ impl IntoVMError for wasmer_runtime::error::ResolveError {
impl IntoVMError for wasmer_runtime::error::RuntimeError {
fn into_vm_error(self) -> VMError {
use wasmer_runtime::error::{InvokeError, RuntimeError};
match &self {
match self {
RuntimeError::InvokeError(invoke_error) => match invoke_error {
// Indicates an exceptional circumstance such as a bug in Wasmer
// or a hardware failure.
Expand Down Expand Up @@ -189,17 +189,14 @@ impl IntoVMError for wasmer_runtime::error::RuntimeError {
RuntimeError::InstanceImage(_) => {
panic!("Support instance image errors properly");
}
RuntimeError::User(data) => {
if let Some(err) = data.downcast_ref::<VMLogicError>() {
err.into()
} else {
panic!(
"Bad error case! Output is non-deterministic {:?} {:?}",
data.type_id(),
self.to_string()
);
}
}
RuntimeError::User(data) => match data.downcast::<VMLogicError>() {
Ok(err) => (*err).into(),
Err(data) => panic!(
"Bad error case! Output is non-deterministic {:?} {:?}",
data.type_id(),
RuntimeError::User(data).to_string()
),
},
}
}
}
Expand Down
8 changes: 3 additions & 5 deletions runtime/near-vm-runner/src/wasmtime_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,12 +74,10 @@ fn trap_to_error(trap: &wasmtime::Trap) -> VMError {
if trap.i32_exit_status() == Some(239) {
match imports::wasmtime::last_error() {
Some(VMLogicError::HostError(h)) => {
VMError::FunctionCallError(FunctionCallError::HostError(h.clone()))
}
Some(VMLogicError::ExternalError(s)) => VMError::ExternalError(s.clone()),
Some(VMLogicError::InconsistentStateError(e)) => {
VMError::InconsistentStateError(e.clone())
VMError::FunctionCallError(FunctionCallError::HostError(h))
}
Some(VMLogicError::ExternalError(s)) => VMError::ExternalError(s),
Some(VMLogicError::InconsistentStateError(e)) => VMError::InconsistentStateError(e),
None => panic!("Error is not properly set"),
}
} else {
Expand Down
6 changes: 3 additions & 3 deletions runtime/runtime/src/actions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,9 @@ pub(crate) fn action_function_call(
}
FunctionCallError::_EVMError => unreachable!(),
},
Some(VMError::ExternalError(serialized_error)) => {
let err: ExternalError = borsh::BorshDeserialize::try_from_slice(&serialized_error)
.expect("External error deserialization shouldn't fail");
Some(VMError::ExternalError(any_err)) => {
let err: ExternalError =
any_err.downcast().expect("Downcasting AnyError should not fail");
return match err {
ExternalError::StorageError(err) => Err(err.into()),
ExternalError::ValidatorError(err) => Err(RuntimeError::ValidatorError(err)),
Expand Down
5 changes: 1 addition & 4 deletions runtime/runtime/src/ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,7 @@ impl<'a> RuntimeExt<'a> {
}

fn wrap_storage_error(error: StorageError) -> VMLogicError {
VMLogicError::ExternalError(
borsh::BorshSerialize::try_to_vec(&ExternalError::StorageError(error))
.expect("Borsh serialize cannot fail"),
)
VMLogicError::from(ExternalError::StorageError(error))
}

type ExtResult<T> = ::std::result::Result<T, VMLogicError>;
Expand Down