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

Implement execution traces for the FVM #412

Merged
merged 2 commits into from
Apr 18, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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: 12 additions & 3 deletions fvm/src/call_manager/default.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use fvm_ipld_encoding::{RawBytes, DAG_CBOR};
use fvm_shared::actor::builtin::Type;
use fvm_shared::address::{Address, Protocol};
use fvm_shared::econ::TokenAmount;
use fvm_shared::error::ExitCode;
use fvm_shared::error::{ErrorNumber, ExitCode};
use fvm_shared::version::NetworkVersion;
use fvm_shared::{ActorID, MethodNum, METHOD_SEND};
use num_traits::Zero;
Expand All @@ -13,7 +13,7 @@ use super::{Backtrace, CallManager, InvocationResult, NO_DATA_BLOCK_ID};
use crate::call_manager::backtrace::Frame;
use crate::call_manager::FinishRet;
use crate::gas::GasTracker;
use crate::kernel::{ClassifyResult, ExecutionError, Kernel, Result};
use crate::kernel::{ClassifyResult, ExecutionError, Kernel, Result, SyscallError};
use crate::machine::Machine;
use crate::syscalls::error::Abort;
use crate::trace::{ExecutionEvent, ExecutionTrace, SendParams};
Expand Down Expand Up @@ -125,7 +125,16 @@ where
self.call_stack_depth -= 1;

if self.machine.context().tracing {
self.exec_trace.push(ExecutionEvent::Return(result.clone()));
self.exec_trace.push(ExecutionEvent::Return(match result {
Err(ref e) => Err(match e {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This combination of refs and clones appeased the checker, but I feel like there's something better that can be done?

Copy link
Member

Choose a reason for hiding this comment

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

Not really...

Copy link
Member

Choose a reason for hiding this comment

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

Well, you can use &result and remove the ref e and friends, but you still need to clone. On the other hand, this will only happen when tracing is enabled.

ExecutionError::OutOfGas => {
SyscallError::new(ErrorNumber::Forbidden, "out of gas")
Copy link
Member

Choose a reason for hiding this comment

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

Are these syscall errors numbers the correct ones to use here?

}
ExecutionError::Fatal(_) => SyscallError::new(ErrorNumber::Forbidden, "fatal"),
ExecutionError::Syscall(s) => s.clone(),
}),
Ok(ref v) => Ok(v.clone()),
}));
}

result
Expand Down
12 changes: 0 additions & 12 deletions fvm/src/kernel/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ use std::fmt::Display;
use derive_more::Display;
use fvm_shared::error::ErrorNumber;

use crate::kernel::ExecutionError::{Fatal, OutOfGas, Syscall};

/// Execution result.
pub type Result<T> = std::result::Result<T, ExecutionError>;

Expand Down Expand Up @@ -37,16 +35,6 @@ pub enum ExecutionError {
Fatal(anyhow::Error),
}

impl Clone for ExecutionError {
fn clone(&self) -> Self {
match self {
OutOfGas => OutOfGas,
Syscall(v) => Syscall(v.clone()),
Fatal(v) => Fatal(anyhow::Error::msg(v.to_string())),
}
}
}

impl ExecutionError {
/// Returns true if the error is fatal. A fatal error means that something went wrong
/// when processing the message. This might be due to the message, the state of the filecoin
Expand Down
8 changes: 5 additions & 3 deletions fvm/src/machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ impl NetworkConfig {

/// Enable actor debugging. This is a consensus-critical option (affects gas usage) so it should
/// only be enabled for local testing or as a network-wide parameter.
pub fn enable_actor_debugging(&mut self, enabled: bool) -> &mut Self {
self.actor_debugging = enabled;
pub fn enable_actor_debugging(&mut self) -> &mut Self {
self.actor_debugging = true;
self
}

Expand Down Expand Up @@ -180,6 +180,8 @@ pub struct MachineContext {
/// DEFAULT: Total FIL supply (likely not what you want).
pub circ_supply: TokenAmount,

/// Whether or not to produce execution traces in the returned result.
/// Not consensus-critical, but has a performance impact.
pub tracing: bool,
}

Expand All @@ -196,7 +198,7 @@ impl MachineContext {
self
}

/// Set [`MachineContext::tracing`].
/// Enable execution traces. [`MachineContext::tracing`].
pub fn enable_tracing(&mut self) -> &mut Self {
self.tracing = true;
self
Expand Down
4 changes: 2 additions & 2 deletions fvm/src/trace/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@ use fvm_shared::econ::TokenAmount;
use fvm_shared::{ActorID, MethodNum};

use crate::call_manager::InvocationResult;
use crate::kernel::Result;
use crate::kernel::SyscallError;

/// Execution Trace, only for informational and debugging purposes.
pub type ExecutionTrace = Vec<ExecutionEvent>;

#[derive(Clone, Debug)]
pub enum ExecutionEvent {
Call(SendParams),
Return(Result<InvocationResult>),
Return(Result<InvocationResult, SyscallError>),
}

#[derive(Clone, Debug)]
Expand Down