runtime storage migration in aleph pallet (Cardinal-Cryptography#197)

* changes to pallet storage + storage migration

bin/runtime/src/lib.rs

@@ -109,7 +109,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
     spec_name: create_runtime_str!("aleph-node"),
     impl_name: create_runtime_str!("aleph-node"),
     authoring_version: 1,
-    spec_version: 5,
+    spec_version: 6,
     impl_version: 1,
     apis: RUNTIME_API_VERSIONS,
     transaction_version: 3,

pallet/Cargo.toml

@@ -1,11 +1,9 @@
 [package]
 name = "pallet-aleph"
-version = "0.1.0"
+version = "0.2.0"
 authors = ["Cardinal Cryptography"]
 edition = "2018"
 
-# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
-
 [dependencies]
 codec = { package = "parity-scale-codec", version = "2.0.0", default-features = false, features = ["derive"] }
 frame-support = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.9" }
@@ -14,9 +12,9 @@ sp-std = { default-features = false, git = "https://github.com/paritytech/substr
 serde = "1.0"
 primitives = { path = "../primitives", default-features = false}
 pallet-session = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.9" }
+sp-io = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.9" }
 
 [dev-dependencies]
-
 pallet-balances = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.9" }
 pallet-timestamp = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.9" }
 sp-runtime = { default-features = false, git = "https://github.com/paritytech/substrate.git", branch = "polkadot-v0.9.9" }

pallet/src/lib.rs

@@ -13,12 +13,20 @@ mod mock;
 #[cfg(test)]
 mod tests;
 
+mod migrations;
+
 use frame_support::Parameter;
 use sp_std::prelude::*;
 
-use frame_support::{sp_runtime::BoundToRuntimeAppPublic, traits::OneSessionHandler};
+use frame_support::{
+    sp_runtime::BoundToRuntimeAppPublic,
+    traits::{OneSessionHandler, StorageVersion},
+};
 pub use pallet::*;
 
+/// The current storage version.
+const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
 #[frame_support::pallet]
 pub mod pallet {
     use super::*;
@@ -33,25 +41,13 @@ pub mod pallet {
         ApiError as AlephApiError, DEFAULT_MILLISECS_PER_BLOCK, DEFAULT_SESSION_PERIOD,
     };
 
-    #[pallet::type_value]
-    pub fn DefaultValidators<T: Config>() -> Option<Vec<T::AccountId>> {
-        None
-    }
-
     #[pallet::storage]
     #[pallet::getter(fn validators)]
-    pub type Validators<T: Config> =
-        StorageValue<_, Option<Vec<T::AccountId>>, ValueQuery, DefaultValidators<T>>;
-
-    #[pallet::type_value]
-    pub fn DefaultSessionForValidatorsChange<T: Config>() -> Option<u32> {
-        None
-    }
+    pub type Validators<T: Config> = StorageValue<_, Vec<T::AccountId>, OptionQuery>;
 
     #[pallet::storage]
     #[pallet::getter(fn session_for_validators_change)]
-    pub type SessionForValidatorsChange<T: Config> =
-        StorageValue<_, Option<u32>, ValueQuery, DefaultSessionForValidatorsChange<T>>;
+    pub type SessionForValidatorsChange<T: Config> = StorageValue<_, u32, OptionQuery>;
 
     #[pallet::config]
     pub trait Config: frame_system::Config + pallet_session::Config {
@@ -73,10 +69,15 @@ pub mod pallet {
     pub struct AlephSessionManager<T>(sp_std::marker::PhantomData<T>);
 
     #[pallet::pallet]
-    pub struct Pallet<T>(sp_std::marker::PhantomData<T>);
+    #[pallet::storage_version(STORAGE_VERSION)]
+    pub struct Pallet<T>(_);
 
     #[pallet::hooks]
-    impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {}
+    impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+        fn on_runtime_upgrade() -> frame_support::weights::Weight {
+            migrations::v0_to_v1::migrate::<T, Self>()
+        }
+    }
 
     #[pallet::call]
     impl<T: Config> Pallet<T> {
@@ -87,8 +88,8 @@ pub mod pallet {
             session_for_validators_change: u32,
         ) -> DispatchResult {
             ensure_root(origin)?;
-            Validators::<T>::put(Some(validators.clone()));
-            SessionForValidatorsChange::<T>::put(Some(session_for_validators_change));
+            Validators::<T>::put(validators.clone());
+            SessionForValidatorsChange::<T>::put(session_for_validators_change);
             Self::deposit_event(Event::ChangeValidators(
                 validators,
                 session_for_validators_change,
@@ -146,8 +147,6 @@ pub mod pallet {
         fn build(&self) {
             <SessionPeriod<T>>::put(&self.session_period);
             <MillisecsPerBlock<T>>::put(&self.millisecs_per_block);
-            <Validators<T>>::put(Some(&self.validators));
-            <SessionForValidatorsChange<T>>::put(Some(0));
         }
     }
 
@@ -180,11 +179,9 @@ pub mod pallet {
                 Pallet::<T>::session_for_validators_change()
             {
                 if session_for_validators_change <= session {
-                    let validators = Pallet::<T>::validators().expect(
-                        "Validators also should be Some(), when session_for_validators_change is",
-                    );
-                    Validators::<T>::put(None::<Vec<T::AccountId>>);
-                    SessionForValidatorsChange::<T>::put(None::<u32>);
+                    let validators = Validators::<T>::take()
+                        .expect("When SessionForValidatorsChange is Some so should be Validators");
+                    let _ = SessionForValidatorsChange::<T>::take().unwrap();
                     return Some(validators);
                 }
             }

pallet/src/migrations/mod.rs

@@ -0,0 +1 @@
+pub mod v0_to_v1;

pallet/src/migrations/v0_to_v1.rs

@@ -0,0 +1,60 @@
+use crate::Config;
+use frame_support::log;
+use frame_support::{
+    traits::{Get, GetStorageVersion, PalletInfoAccess, StorageVersion},
+    weights::Weight,
+};
+use sp_std::prelude::*;
+
+frame_support::generate_storage_alias!(
+    Aleph, SessionForValidatorsChange => Value<Option<u32>>
+);
+
+frame_support::generate_storage_alias!(
+    Aleph, Validators<T: Config> => Value<Option<Vec<T::AccountId>>>
+);
+
+pub fn migrate<T: Config, P: GetStorageVersion + PalletInfoAccess>() -> Weight {
+    let on_chain_storage_version = <P as GetStorageVersion>::on_chain_storage_version();
+    let current_storage_version = <P as GetStorageVersion>::current_storage_version();
+
+    if on_chain_storage_version == StorageVersion::default() && current_storage_version == 1 {
+        log::info!(target: "pallet_aleph", "Running migration from STORAGE_VERSION 0 to 1");
+
+        let mut writes = 0;
+        match SessionForValidatorsChange::translate(sp_std::convert::identity) {
+            Ok(_) => {
+                writes += 1;
+                log::info!(target: "pallet_aleph", "Succesfully migrated storage for SessionForValidatorsChange");
+            }
+            Err(why) => {
+                log::error!(target: "pallet_aleph", "Something went wrong during the migration of SessionForValidatorsChange {:?}", why);
+            }
+        };
+
+        match Validators::<T>::translate(sp_std::convert::identity) {
+            Ok(_) => {
+                writes += 1;
+                log::info!(target: "pallet_aleph", "Succesfully migrated storage for Validators");
+            }
+            Err(why) => {
+                log::error!(target: "pallet_aleph", "Something went wrong during the migration of Validators storage {:?}", why);
+            }
+        };
+
+        // store new version
+        StorageVersion::new(1).put::<P>();
+        writes += 1;
+
+        T::DbWeight::get().reads(3) + T::DbWeight::get().writes(writes)
+    } else {
+        log::warn!(
+            target: "pallet_aleph",
+            "Not applying any storage migration because on-chain storage version is {:?} and the version declared in the aleph pallet is {:?}",
+            on_chain_storage_version,
+            current_storage_version
+        );
+        // I have only read the version
+        T::DbWeight::get().reads(1)
+    }
+}

pallet/src/mock.rs

@@ -3,13 +3,13 @@
 use super::*;
 use crate as pallet_aleph;
 
-use sp_core::H256;
-
 use frame_support::{
     construct_runtime, parameter_types, sp_io,
     traits::{OnFinalize, OnInitialize},
+    weights::RuntimeDbWeight,
 };
 use primitives::AuthorityId;
+use sp_core::H256;
 use sp_runtime::{
     impl_opaque_keys,
     testing::{Header, TestXt, UintAuthorityId},
@@ -48,6 +48,10 @@ parameter_types! {
     pub const BlockHashCount: u64 = 250;
     pub BlockWeights: frame_system::limits::BlockWeights =
         frame_system::limits::BlockWeights::simple_max(1024);
+    pub const TestDbWeight: RuntimeDbWeight = RuntimeDbWeight {
+        read: 25,
+        write: 100
+    };
 }
 
 impl frame_system::Config for Test {
@@ -65,7 +69,7 @@ impl frame_system::Config for Test {
     type Header = Header;
     type Event = Event;
     type BlockHashCount = BlockHashCount;
-    type DbWeight = ();
+    type DbWeight = TestDbWeight;
     type Version = ();
     type PalletInfo = PalletInfo;
     type AccountData = pallet_balances::AccountData<u128>;

pallet/src/tests.rs

@@ -1,7 +1,64 @@
 #![cfg(test)]
 
-use crate::mock::*;
+use crate::{migrations, mock::*, pallet};
 use frame_support::assert_ok;
+use frame_support::traits::{GetStorageVersion, StorageVersion};
+
+#[test]
+fn migration_from_v0_to_v1_works() {
+    new_test_ext(&[(1u64, 1u64), (2u64, 2u64)]).execute_with(|| {
+        frame_support::migration::put_storage_value(
+            b"Aleph",
+            b"SessionForValidatorsChange",
+            &[],
+            1u32,
+        );
+
+        frame_support::migration::put_storage_value(
+            b"Aleph",
+            b"Validators",
+            &[],
+            vec![AccountId::default()],
+        );
+
+        let v0 = <pallet::Pallet<Test> as GetStorageVersion>::on_chain_storage_version();
+
+        assert_eq!(
+            v0,
+            StorageVersion::default(),
+            "Storage version before applying migration should be default"
+        );
+
+        let _weight = migrations::v0_to_v1::migrate::<Test, Aleph>();
+
+        let v1 = <pallet::Pallet<Test> as GetStorageVersion>::on_chain_storage_version();
+
+        assert_ne!(
+            v1,
+            StorageVersion::default(),
+            "Storage version after applying migration should be incremented"
+        );
+
+        assert_eq!(
+            Aleph::session_for_validators_change(),
+            Some(1u32),
+            "Migration should preserve ongoing session change with respect to the session number"
+        );
+
+        assert_eq!(
+            Aleph::validators(),
+            Some(vec![AccountId::default()]),
+            "Migration should preserve ongoing session change with respect to the validators set"
+        );
+
+        let noop_weight = migrations::v0_to_v1::migrate::<Test, Aleph>();
+        assert_eq!(
+            noop_weight,
+            TestDbWeight::get().reads(1),
+            "Migration cannot be run twice"
+        );
+    })
+}
 
 #[test]
 fn test_update_authorities() {
@@ -38,6 +95,7 @@ fn test_change_validators() {
             0
         ));
 
+        assert_eq!(Aleph::session_for_validators_change(), Some(0));
         assert_eq!(Aleph::validators(), Some(vec![AccountId::default()]));
     });
 }