Mvm static-init refactoring (#188)

* remove `no-vm-static` feature

* draft usage-marker for the VM

* impl usage-marker for the VM

* describe boxed closures types in less complex way

* fmt

* more consistent declarations

* add test for VM's cache cleaning

pallets/sp-mvm/Cargo.toml

@@ -132,11 +132,6 @@ runtime-benchmarks = [
     "bcs-alt",
 ]
 
-# Optional feature `no-vm-static` that disables VM static initialization,
-# so Move VM will be initialized for each block when it used if this feature is enabled.
-# Otherwise when the feature is disabled VM initialization happens only once, but only cache cleaning for each block.
-no-vm-static = []
-
 # Note: frame-support `try-runtime` feature is released after v3.
 # Uncomment the following line when `frame-support` version > `3.0.0`.
 # try-runtime = ['frame-support/try-runtime']

pallets/sp-mvm/README.md

@@ -12,14 +12,8 @@ All provided extrinsics functions require to configure a gas limit, similar to E
  - `execute(tx_bc: Vec<u8>, gas_limit: u64)` - execute Move script with bytecode `tx_bc`.
  - `publish_module(module_bc: Vec<u8>, gas_limit: u64)` - publish Move module with bytecode `module_bc`.
  - `publish_package(package: Vec<u8>, gas_limit: u64)` - publish package (a set of Move modules) from binary `package`. Allows to update Standard Library if calls from root, in the future root will be replaced with gov.
-
-Read more about the Move VM pallet in the [Pontem Documentation](https://docs.pontem.network/03.-move-vm/move_vm).
-
-### Note
 
-There's optional feature `no-vm-static` that disables VM static initialization,
-so VM will init for each block when it used if this feature is enabled.
-Otherwise when the feature is disabled VM initialization happens only once, but only cache cleaning for each block.
+Read more about the Move VM pallet in the [Pontem Documentation](https://docs.pontem.network/03.-move-vm/move_vm).
 
 ## LICENSE
 

pallets/sp-mvm/src/balance.rs

@@ -220,7 +220,6 @@ where
     }
 }
 
-#[cfg(not(feature = "no-vm-static"))]
 pub mod boxed {
     use move_vm::io::{
         traits::{Balance as VmBalance, BalanceAccess},
@@ -235,14 +234,16 @@ pub mod boxed {
     use orml_traits::MultiCurrency;
 
     pub type BalancesAdapter = BalancesBoxedAdapter;
+    type Withdraw = dyn Fn(&PalletId, &AccountAddress, &[u8], VmBalance);
+    type Deposit = dyn Fn(&PalletId, &AccountAddress, &[u8], VmBalance);
+    type Get = dyn Fn(&AccountAddress, &[u8]) -> Option<VmBalance>;
 
     /// Vm storage boxed adapter for native storage
-    #[allow(clippy::type_complexity)]
     pub struct BalancesBoxedAdapter {
         pallet_id: PalletId,
-        f_get: Box<dyn Fn(&AccountAddress, &[u8]) -> Option<VmBalance>>,
-        f_deposit: Box<dyn Fn(&PalletId, &AccountAddress, &[u8], VmBalance)>,
-        f_withdraw: Box<dyn Fn(&PalletId, &AccountAddress, &[u8], VmBalance)>,
+        f_get: Box<Get>,
+        f_deposit: Box<Deposit>,
+        f_withdraw: Box<Withdraw>,
     }
 
     impl<

pallets/sp-mvm/src/event.rs

@@ -61,11 +61,13 @@ impl<F> EventWriter<F> {
     }
 }
 
+type EventHandlerFn = dyn Fn(MoveEventArguments);
+
 /// Default EventWriter.
-pub type DefaultEventHandler = EventWriter<Box<dyn Fn(MoveEventArguments)>>;
+pub type DefaultEventHandler = EventWriter<Box<EventHandlerFn>>;
 
 /// Boxed fn ptr to something looks like `DepositMoveEvent::deposit_move_event`.
-pub type DepositMoveEventFnPtr = Box<dyn Fn(MoveEventArguments)>;
+pub type DepositMoveEventFnPtr = Box<EventHandlerFn>;
 
 pub trait GetDepositMoveEventFn<T: DepositMoveEvent + 'static> {
     fn get_deposit_move_event_fn() -> DepositMoveEventFnPtr {

pallets/sp-mvm/src/lib.rs

@@ -55,11 +55,8 @@ pub mod pallet {
     use mvm::*;
     use weights::WeightInfo;
 
-    #[cfg(not(feature = "no-vm-static"))]
-    mod boxed {
-        pub use crate::storage::boxed::VmStorageBoxAdapter as StorageAdapter;
-        pub use crate::balance::boxed::BalancesAdapter;
-    }
+    use crate::storage::boxed::VmStorageBoxAdapter as StorageAdapter;
+    use crate::balance::boxed::BalancesAdapter;
 
     use core::convert::TryInto;
     use core::convert::TryFrom;
@@ -344,32 +341,27 @@ pub mod pallet {
 
     /// Clearing Move VM cache once block processed.
     #[pallet::hooks]
-    impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
-        #[cfg(not(feature = "no-vm-static"))]
+    impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T>
+    // TODO: make it configurable:  where <T as Config>::ClearMvmCachePolicy = ...
+    {
         fn on_finalize(_: BlockNumberFor<T>) {
-            if let Some(vm) = Self::get_move_vm_cell().get() {
-                vm.clear();
-                trace!("VM cache cleared on finalize block");
+            if Self::is_move_vm_used() {
+                if let Some(vm) = Self::get_move_vm_cell().get() {
+                    vm.clear();
+                    Self::set_move_vm_clean();
+                    trace!("VM cache cleared on finalize block");
+                }
             }
+            // Otherwise we are not requesting VM.
         }
     }
 
     /// Get VM methods unification.
     impl<T: Config> Pallet<T> {
-        #[cfg(not(feature = "no-vm-static"))]
         fn get_vm() -> Result<&'static VmWrapperTy, Error<T>> {
             let vm = Self::try_get_or_create_move_vm()?;
             Ok(vm)
         }
-
-        #[cfg(feature = "no-vm-static")]
-        fn get_vm() -> Result<
-            DefaultVm<VMStorage<T>, event::DefaultEventHandler, oracle::DummyOracle, T>,
-            Error<T>,
-        > {
-            let vm = Self::try_create_move_vm()?;
-            Ok(vm)
-        }
     }
 
     /// Move VM allows us to configure Gas Price, but we use constant for gas price, as we follow general Substrate approach with weight and tips.
@@ -559,14 +551,7 @@ pub mod pallet {
     ///
     /// Supports both static (created at launch of chain), and dynamic one (usually we use regulated one);.
     impl<T: Config> mvm::TryCreateMoveVm<T> for Pallet<T> {
-        #[cfg(not(feature = "no-vm-static"))]
-        type Vm = Mvm<boxed::StorageAdapter, event::DefaultEventHandler, boxed::BalancesAdapter>;
-        #[cfg(feature = "no-vm-static")]
-        type Vm = Mvm<
-            StorageAdapter<VMStorage<T>>,
-            event::DefaultEventHandler,
-            balance::BalancesAdapter<T>,
-        >;
+        type Vm = Mvm<StorageAdapter, event::DefaultEventHandler, BalancesAdapter>;
         type Error = Error<T>;
 
         /// Try to create Move VM instance.
@@ -591,30 +576,30 @@ pub mod pallet {
         }
     }
 
-    #[cfg(not(feature = "no-vm-static"))]
     impl<T: Config> GetStaticMoveVmCell for Pallet<T> {
         type Vm = VmWrapper<<Self as mvm::TryCreateMoveVm<T>>::Vm>;
 
         #[inline(never)]
-        fn get_move_vm_cell() -> &'static OnceCell<VmWrapperTy> {
+        fn get_move_vm_cell() -> &'static OnceCell<Self::Vm> {
             static VM: OnceCell<VmWrapperTy> = OnceCell::new();
             &VM
         }
     }
 
-    #[cfg(not(feature = "no-vm-static"))]
     impl<T: Config> TryGetStaticMoveVm for Pallet<T> {
         type Vm = <Self as GetStaticMoveVmCell>::Vm;
         type Error = Error<T>;
 
         fn try_get_or_create_move_vm() -> Result<&'static Self::Vm, Self::Error> {
-            Self::get_move_vm_cell().get_or_try_init(|| {
-                trace!("Static VM initializing");
-                Self::try_create_move_vm_static().map(Into::into)
-            })
+            Self::set_move_vm_used();
+            Self::get_move_vm_cell()
+                .get_or_try_init(|| Self::try_create_move_vm_static().map(Into::into))
         }
     }
 
+    /// Usage marker for the VM.
+    impl<T: Config> MoveVmUsed for Pallet<T> {}
+
     /// Errors that occur during Move VM execution.
     /// Based on initial Move VM errors, but adopted for Substrate.
     #[pallet::error]

pallets/sp-mvm/src/mvm.rs

@@ -12,15 +12,7 @@ use crate::storage::*;
 pub type DefaultVm<S, E, AccountId, Currencies, CurrencyId> =
     Mvm<StorageAdapter<S>, E, BalancesAdapter<Currencies, AccountId, CurrencyId>>;
 
-// The trait to create Move VM (without possible errors).
-pub trait CreateMoveVm<T> {
-    type Vm: move_vm::Vm;
-
-    /// Create VM instance.
-    fn create_move_vm() -> Self::Vm;
-}
-
-// The trait to create Move VM (returns Result, possible with errors).
+/// The trait to create Move VM (returns Result, possible with errors).
 pub trait TryCreateMoveVm<T> {
     type Vm: move_vm::Vm;
     type Error;
@@ -29,10 +21,8 @@ pub trait TryCreateMoveVm<T> {
     fn try_create_move_vm() -> Result<Self::Vm, Self::Error>;
 }
 
-#[cfg(not(feature = "no-vm-static"))]
-pub use vm_static::*;
-#[cfg(not(feature = "no-vm-static"))]
-mod vm_static {
+pub use boxed::*;
+mod boxed {
     use anyhow::Error;
     use move_vm::StateAccess;
     use move_vm::io::context::ExecutionContext;
@@ -58,6 +48,7 @@ mod vm_static {
     /// so it should only be used between threads.
     /// For thread-local usage or inside the `OnceCell`.
     pub struct VmWrapper<T: move_vm::Vm>(T);
+    #[allow(clippy::non_send_fields_in_send_ty)]
     unsafe impl<T: move_vm::Vm> Send for VmWrapper<T> {}
     unsafe impl<T: move_vm::Vm> Sync for VmWrapper<T> {}
 
@@ -170,4 +161,46 @@ mod vm_static {
     }
 
     impl<T, C: TryCreateMoveVm<T>> TryCreateMoveVmWrapped<T> for C {}
+
+    use core::sync::atomic::{AtomicBool, Ordering};
+
+    /// Usage marker for the VM.
+    pub trait MoveVmUsed {
+        #[inline(never)]
+        fn get_move_vm_used() -> &'static AtomicBool {
+            static USED: AtomicBool = AtomicBool::new(false);
+            &USED
+        }
+
+        fn is_move_vm_used() -> bool {
+            Self::get_move_vm_used().load(Ordering::Relaxed)
+        }
+
+        fn set_move_vm_used() {
+            Self::get_move_vm_used().store(true, Ordering::Relaxed);
+        }
+        fn set_move_vm_clean() {
+            Self::get_move_vm_used().store(false, Ordering::Relaxed);
+        }
+    }
+
+    #[cfg(test)]
+    mod tests {
+        use super::MoveVmUsed;
+
+        struct Vm;
+
+        impl MoveVmUsed for Vm {}
+
+        #[test]
+        fn usage_marker() {
+            assert_eq!(false, Vm::is_move_vm_used());
+
+            Vm::set_move_vm_used();
+            assert_eq!(true, Vm::is_move_vm_used());
+
+            Vm::set_move_vm_clean();
+            assert_eq!(false, Vm::is_move_vm_used());
+        }
+    }
 }

pallets/sp-mvm/src/storage.rs

@@ -49,16 +49,19 @@ impl<T: StorageMap<Vec<u8>, Vec<u8>, Query = Option<Vec<u8>>>> Storage
     }
 }
 
-#[cfg(not(feature = "no-vm-static"))]
 pub mod boxed {
     use sp_std::prelude::*;
     pub type VmStorageAdapter = VmStorageBoxAdapter;
 
+    type Get = dyn Fn(&[u8]) -> Option<Vec<u8>>;
+    type Insert = dyn Fn(&[u8], &[u8]);
+    type Remove = dyn Fn(&[u8]);
+
     /// Vm storage boxed adapter for native storage
     pub struct VmStorageBoxAdapter {
-        f_get: Box<dyn Fn(&[u8]) -> Option<Vec<u8>>>,
-        f_insert: Box<dyn Fn(&[u8], &[u8])>,
-        f_remove: Box<dyn Fn(&[u8])>,
+        f_get: Box<Get>,
+        f_insert: Box<Insert>,
+        f_remove: Box<Remove>,
     }
 
     pub fn into_boxfn_adapter<T>() -> VmStorageBoxAdapter

pallets/sp-mvm/tests/vm.rs

@@ -0,0 +1,28 @@
+/// Tests related to VM usage and management.
+mod common;
+use common::assets::modules;
+use common::mock::*;
+use common::addr::*;
+use common::utils;
+
+#[test]
+/// Cache should be cleaned for new block if VM used.
+fn mvm_cleanup_cache() {
+    use sp_mvm::mvm::MoveVmUsed;
+
+    RuntimeBuilder::new().build().execute_with(|| {
+        roll_next_block();
+        // on finalize -> clean cache
+        assert!(!sp_mvm::Pallet::<Test>::is_move_vm_used());
+
+        // use the VM
+        utils::publish_module(bob_public_key(), &modules::user::STORE, None).unwrap();
+
+        // VM used just now
+        assert!(sp_mvm::Pallet::<Test>::is_move_vm_used());
+
+        roll_next_block();
+        // on finalize -> clean cache
+        assert!(!sp_mvm::Pallet::<Test>::is_move_vm_used());
+    });
+}