Merge pull request #490 from filecoin-project/feat/refactor-constructor

refactor: combine/refactor context/config/machine arguments

Author: Stebalien

fvm/src/call_manager/default.rs

@@ -100,7 +100,7 @@ where
         //
         // NOTE: Unlike the FVM, Lotus adds _then_ checks. It does this because the
         // `call_stack_depth` in lotus is 0 for the top-level call, unlike in the FVM where it's 1.
-        if self.call_stack_depth > self.machine.config().max_call_depth {
+        if self.call_stack_depth > self.machine.context().max_call_depth {
             return Err(
                 syscall_error!(LimitExceeded, "message execution exceeds call depth").into(),
             );

fvm/src/kernel/default.rs

@@ -915,7 +915,7 @@ where
     }
 
     fn debug_enabled(&self) -> bool {
-        self.call_manager.context().debug
+        self.call_manager.context().actor_debugging
     }
 }
 

fvm/src/lib.rs

@@ -42,37 +42,19 @@ lazy_static::lazy_static! {
     };
 }
 
-#[derive(Clone)]
-pub struct Config {
-    /// The maximum call depth.
-    pub max_call_depth: u32,
-    /// Whether debug mode is enabled or not.
-    pub debug: bool,
-}
-
-impl Default for Config {
-    fn default() -> Self {
-        Self {
-            max_call_depth: 4096,
-            debug: false,
-        }
-    }
-}
-
 #[cfg(test)]
 mod test {
     use fvm_ipld_blockstore::MemoryBlockstore;
     use fvm_ipld_encoding::CborStore;
     use fvm_shared::actor::builtin::Manifest;
     use fvm_shared::state::StateTreeVersion;
     use multihash::Code;
-    use num_traits::Zero;
 
     use crate::call_manager::DefaultCallManager;
     use crate::externs::{Consensus, Externs, Rand};
-    use crate::machine::{DefaultMachine, Engine};
+    use crate::machine::{DefaultMachine, Engine, NetworkConfig};
     use crate::state_tree::StateTree;
-    use crate::{executor, Config, DefaultKernel};
+    use crate::{executor, DefaultKernel};
 
     struct DummyExterns;
 
@@ -125,14 +107,10 @@ mod test {
         let actors_cid = bs.put_cbor(&(0, manifest_cid), Code::Blake2b256).unwrap();
 
         let machine = DefaultMachine::new(
-            Config::default(),
-            Engine::default(),
-            0,
-            Zero::zero(),
-            Zero::zero(),
-            fvm_shared::version::NetworkVersion::V14,
-            root,
-            Some(actors_cid),
+            &Engine::default(),
+            &NetworkConfig::new(fvm_shared::version::NetworkVersion::V14)
+                .override_actors(actors_cid)
+                .for_epoch(0, root),
             bs,
             DummyExterns,
         )

fvm/src/machine/boxed.rs

@@ -7,7 +7,8 @@ use fvm_shared::ActorID;
 use super::{Engine, Machine, MachineContext};
 use crate::kernel::Result;
 use crate::state_tree::{ActorState, StateTree};
-use crate::Config;
+
+type Type = MachineContext;
 
 impl<M: Machine> Machine for Box<M> {
     type Blockstore = M::Blockstore;
@@ -18,18 +19,13 @@ impl<M: Machine> Machine for Box<M> {
         (&**self).engine()
     }
 
-    #[inline(always)]
-    fn config(&self) -> &Config {
-        (&**self).config()
-    }
-
     #[inline(always)]
     fn blockstore(&self) -> &Self::Blockstore {
         (&**self).blockstore()
     }
 
     #[inline(always)]
-    fn context(&self) -> &MachineContext {
+    fn context(&self) -> &Type {
         (&**self).context()
     }
 

fvm/src/machine/default.rs

@@ -6,7 +6,6 @@ use fvm_ipld_blockstore::{Blockstore, Buffered};
 use fvm_ipld_encoding::CborStore;
 use fvm_shared::actor::builtin::{load_manifest, Manifest};
 use fvm_shared::address::Address;
-use fvm_shared::clock::ChainEpoch;
 use fvm_shared::econ::TokenAmount;
 use fvm_shared::error::ErrorNumber;
 use fvm_shared::version::NetworkVersion;
@@ -17,16 +16,13 @@ use num_traits::{Signed, Zero};
 use super::{Engine, Machine, MachineContext};
 use crate::blockstore::BufferedBlockstore;
 use crate::externs::Externs;
-use crate::gas::price_list_by_network_version;
 use crate::kernel::{ClassifyResult, Context as _, Result};
 use crate::state_tree::{ActorState, StateTree};
+use crate::syscall_error;
 use crate::system_actor::State as SystemActorState;
-use crate::{syscall_error, Config};
 
 pub struct DefaultMachine<B, E> {
-    /// The machine's configuration for this instantiation.
-    config: Config,
-    /// The context for the execution.
+    /// The initial execution context for this epoch.
     context: MachineContext,
     /// The WASM engine is created on construction of the DefaultMachine, and
     /// is dropped when the DefaultMachine is dropped.
@@ -48,17 +44,18 @@ where
     B: Blockstore + 'static,
     E: Externs + 'static,
 {
-    // ISSUE: #249
-    #[allow(clippy::too_many_arguments)]
+    /// Create a new [`DefaultMachine`].
+    ///
+    /// # Arguments
+    ///
+    /// * `engine`: The global wasm [`Engine`] (engine, pooled resources, caches).
+    /// * `context`: Machine execution [context][`MachineContext`] (system params, epoch, network
+    ///    version, etc.).
+    /// * `blockstore`: The underlying [blockstore][`Blockstore`] for reading/writing state.
+    /// * `externs`: Client-provided ["external"][`Externs`] methods for accessing chain state.
     pub fn new(
-        config: Config,
-        engine: Engine,
-        epoch: ChainEpoch,
-        base_fee: TokenAmount,
-        circ_supply: TokenAmount,
-        network_version: NetworkVersion,
-        state_root: Cid,
-        builtin_actors: Option<Cid>,
+        engine: &Engine,
+        context: &MachineContext,
         blockstore: B,
         externs: E,
     ) -> anyhow::Result<Self> {
@@ -67,23 +64,16 @@ where
 
         debug!(
             "initializing a new machine, epoch={}, base_fee={}, nv={:?}, root={}",
-            epoch, &base_fee, network_version, state_root
+            context.epoch, &context.base_fee, context.network_version, context.initial_state_root
         );
 
-        if !SUPPORTED_VERSIONS.contains(&network_version) {
-            return Err(anyhow!("unsupported network version: {}", network_version));
+        if !SUPPORTED_VERSIONS.contains(&context.network_version) {
+            return Err(anyhow!(
+                "unsupported network version: {}",
+                context.network_version
+            ));
         }
 
-        let context = MachineContext {
-            epoch,
-            base_fee,
-            circ_supply,
-            network_version,
-            initial_state_root: state_root,
-            price_list: price_list_by_network_version(network_version),
-            debug: config.debug,
-        };
-
         // Sanity check that the blockstore contains the supplied state root.
         if !blockstore
             .has(&context.initial_state_root)
@@ -103,7 +93,7 @@ where
 
         // Load the built-in actors manifest.
         // TODO: Check that the actor bundle is sane for the network version.
-        let (builtin_actors_cid, manifest_version) = match builtin_actors {
+        let (builtin_actors_cid, manifest_version) = match context.builtin_actors_override {
             Some(manifest_cid) => {
                 let (version, cid): (u32, Cid) = state_tree
                     .store()
@@ -126,9 +116,8 @@ where
         engine.preload(state_tree.store(), builtin_actors.left_values())?;
 
         Ok(DefaultMachine {
-            config,
-            context,
-            engine,
+            context: context.clone(),
+            engine: engine.clone(),
             externs,
             state_tree,
             builtin_actors,
@@ -148,10 +137,6 @@ where
         &self.engine
     }
 
-    fn config(&self) -> &Config {
-        &self.config
-    }
-
     fn blockstore(&self) -> &Self::Blockstore {
         self.state_tree.store()
     }

fvm/src/machine/mod.rs

@@ -1,17 +1,18 @@
 use cid::Cid;
+use derive_more::{Deref, DerefMut};
 use fvm_ipld_blockstore::Blockstore;
 use fvm_shared::actor::builtin::Manifest;
 use fvm_shared::address::Address;
 use fvm_shared::clock::ChainEpoch;
 use fvm_shared::econ::TokenAmount;
 use fvm_shared::version::NetworkVersion;
 use fvm_shared::ActorID;
+use num_traits::Zero;
 
 use crate::externs::Externs;
-use crate::gas::PriceList;
+use crate::gas::{price_list_by_network_version, PriceList};
 use crate::kernel::Result;
 use crate::state_tree::{ActorState, StateTree};
-use crate::Config;
 
 mod default;
 
@@ -47,9 +48,6 @@ pub trait Machine: 'static {
     /// static lifetime.
     fn engine(&self) -> &Engine;
 
-    /// Returns the FVM's configuration.
-    fn config(&self) -> &Config;
-
     /// Returns a reference to the machine's blockstore.
     fn blockstore(&self) -> &Self::Blockstore;
 
@@ -88,23 +86,110 @@ pub trait Machine: 'static {
     fn consume(self) -> Self::Blockstore;
 }
 
-/// Execution context supplied to the machine.
-#[derive(Clone, Debug)]
+/// Network-level settings. Except when testing locally, changing any of these likely requires a
+/// network upgrade.
+#[derive(Debug, Clone)]
+pub struct NetworkConfig {
+    /// The network version at epoch
+    pub network_version: NetworkVersion,
+
+    /// The maximum call depth.
+    ///
+    /// DEFAULT: 4096
+    pub max_call_depth: u32,
+
+    /// An override for builtin-actors. If specified, this should be the CID of a builtin-actors
+    /// "manifest".
+    ///
+    /// DEFAULT: `None`
+    pub builtin_actors_override: Option<Cid>,
+
+    /// Enable actor debugging.
+    ///
+    /// DEFAULT: `false`
+    pub actor_debugging: bool,
+
+    /// The price list.
+    ///
+    /// DEFAULT: The price-list for the current network version.
+    pub price_list: &'static PriceList,
+}
+
+impl NetworkConfig {
+    /// Create a new network config for the given network version.
+    pub fn new(network_version: NetworkVersion) -> Self {
+        NetworkConfig {
+            network_version,
+            max_call_depth: 4096,
+            actor_debugging: false,
+            builtin_actors_override: None,
+            price_list: price_list_by_network_version(network_version),
+        }
+    }
+
+    /// 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;
+        self
+    }
+
+    /// Override actors with the specific manifest. This is primarily useful for testing, or
+    /// networks prior to NV16 (where the actor's "manifest" isn't specified on-chain).
+    pub fn override_actors(&mut self, manifest: Cid) -> &mut Self {
+        self.builtin_actors_override = Some(manifest);
+        self
+    }
+
+    /// Create a [`MachineContext`] for a given `epoch` with the specified `initial_state`.
+    pub fn for_epoch(&self, epoch: ChainEpoch, initial_state: Cid) -> MachineContext {
+        MachineContext {
+            network: self.clone(),
+            epoch,
+            initial_state_root: initial_state,
+            base_fee: TokenAmount::zero(),
+            circ_supply: fvm_shared::TOTAL_FILECOIN.clone(),
+        }
+    }
+}
+
+/// Per-epoch machine context.
+#[derive(Clone, Debug, Deref, DerefMut)]
 pub struct MachineContext {
+    /// Network-level settings.
+    #[deref]
+    #[deref_mut]
+    pub network: NetworkConfig,
+
     /// The epoch at which the Machine runs.
     pub epoch: ChainEpoch,
+
+    /// The initial state root on which this block is based.
+    pub initial_state_root: Cid,
+
     /// The base fee that's in effect when the Machine runs.
+    ///
+    /// Default: 0.
     pub base_fee: TokenAmount,
+
     /// v15 and onwards: The amount of FIL that has vested from genesis actors.
     /// v14 and earlier: The amount of FIL that has vested from genesis msigs
     /// (the remainder of the circ supply must be calculated by the FVM)
+    ///
+    /// DEFAULT: Total FIL supply (likely not what you want).
     pub circ_supply: TokenAmount,
-    /// The initial state root on which this block is based.
-    pub initial_state_root: Cid,
-    /// The price list.
-    pub price_list: &'static PriceList,
-    /// The network version at epoch
-    pub network_version: NetworkVersion,
-    /// Whether debug mode is enabled or not.
-    pub debug: bool,
+}
+
+impl MachineContext {
+    /// Sets [`EpochContext::base_fee`].
+    pub fn set_base_fee(&mut self, amt: TokenAmount) -> &mut Self {
+        self.base_fee = amt;
+        self
+    }
+
+    /// Set [`EpochContext::circ_supply`].
+    pub fn set_circulating_supply(&mut self, amt: TokenAmount) -> &mut Self {
+        self.circ_supply = amt;
+        self
+    }
 }

testing/conformance/benches/bench_drivers.rs

@@ -45,7 +45,7 @@ pub fn bench_vector_variant(
                 let vector = &(*vector).clone();
                 let bs = bs.clone();
                 // TODO next few lines don't impact the benchmarks, but it might make them run waaaay more slowly... ought to make a base copy of the machine and exec and deepcopy them each time.
-                let machine = TestMachine::new_for_vector(vector, variant, bs, engine.clone());
+                let machine = TestMachine::new_for_vector(vector, variant, bs, engine);
                 // can assume this works because it passed a test before this ran
                 let exec: DefaultExecutor<TestKernel> = DefaultExecutor::new(machine);
                 (messages_with_lengths.clone(), exec)

testing/conformance/src/driver.rs

@@ -190,7 +190,7 @@ pub fn run_variant(
     let id = variant.id.clone();
 
     // Construct the Machine.
-    let machine = TestMachine::new_for_vector(v, variant, bs, engine.clone());
+    let machine = TestMachine::new_for_vector(v, variant, bs, engine);
     let mut exec: DefaultExecutor<TestKernel> = DefaultExecutor::new(machine);
 
     // Apply all messages in the vector.