Migrate Weights properly to v2 (#1722)

* Migrate Weights properly to v2

* Add missing on_runtime_upgrade implementation

* Fix benchmarks

* Apply suggestions from code review

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* cargo fmt

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

cumulus/Cargo.lock

@@ -1752,6 +1752,7 @@ dependencies = [
  "cumulus-primitives-core",
  "frame-support",
  "frame-system",
+ "log",
  "parity-scale-codec",
  "scale-info",
  "sp-core",

cumulus/pallets/dmp-queue/Cargo.toml

@@ -6,6 +6,7 @@ edition = "2021"
 
 [dependencies]
 codec = { package = "parity-scale-codec", version = "3.0.0", features = [ "derive" ], default-features = false }
+log = { version = "0.4.17", default-features = false }
 scale-info = { version = "2.2.0", default-features = false, features = ["derive"] }
 
 # Substrate
@@ -32,6 +33,7 @@ std = [
 	"scale-info/std",
 	"frame-support/std",
 	"frame-system/std",
+	"log/std",
 	"sp-io/std",
 	"sp-runtime/std",
 	"sp-std/std",

cumulus/pallets/dmp-queue/src/lib.rs

@@ -21,6 +21,8 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
+pub mod migration;
+
 use codec::{Decode, DecodeLimit, Encode};
 use cumulus_primitives_core::{relay_chain::BlockNumber as RelayBlockNumber, DmpMessageHandler};
 use frame_support::{
@@ -31,7 +33,10 @@ pub use pallet::*;
 use scale_info::TypeInfo;
 use sp_runtime::RuntimeDebug;
 use sp_std::{convert::TryFrom, prelude::*};
-use xcm::{latest::prelude::*, VersionedXcm, MAX_XCM_DECODE_DEPTH};
+use xcm::{
+	latest::{prelude::*, Weight as XcmWeight},
+	VersionedXcm, MAX_XCM_DECODE_DEPTH,
+};
 
 #[derive(Copy, Clone, Eq, PartialEq, Encode, Decode, RuntimeDebug, TypeInfo)]
 pub struct ConfigData {
@@ -78,6 +83,7 @@ pub mod pallet {
 
 	#[pallet::pallet]
 	#[pallet::generate_store(pub(super) trait Store)]
+	#[pallet::storage_version(migration::STORAGE_VERSION)]
 	#[pallet::without_storage_info]
 	pub struct Pallet<T>(_);
 
@@ -121,6 +127,10 @@ pub mod pallet {
 
 	#[pallet::hooks]
 	impl<T: Config> Hooks<BlockNumberFor<T>> for Pallet<T> {
+		fn on_runtime_upgrade() -> Weight {
+			migration::migrate_to_latest::<T>()
+		}
+
 		fn on_idle(_now: T::BlockNumber, max_weight: Weight) -> Weight {
 			// on_idle processes additional messages with any remaining block weight.
 			Self::service_queue(max_weight)
@@ -141,16 +151,17 @@ pub mod pallet {
 		///
 		/// Events:
 		/// - `OverweightServiced`: On success.
-		#[pallet::weight(weight_limit.saturating_add(Weight::from_ref_time(1_000_000)))]
+		#[pallet::weight(Weight::from_ref_time(weight_limit.saturating_add(1_000_000)))]
 		pub fn service_overweight(
 			origin: OriginFor<T>,
 			index: OverweightIndex,
-			weight_limit: Weight,
+			weight_limit: XcmWeight,
 		) -> DispatchResultWithPostInfo {
 			T::ExecuteOverweightOrigin::ensure_origin(origin)?;
 
 			let (sent_at, data) = Overweight::<T>::get(index).ok_or(Error::<T>::Unknown)?;
-			let weight_used = Self::try_service_message(weight_limit, sent_at, &data[..])
+			let weight_used =
+				Self::try_service_message(Weight::from_ref_time(weight_limit), sent_at, &data[..])
 					.map_err(|_| Error::<T>::OverLimit)?;
 			Overweight::<T>::remove(index);
 			Self::deposit_event(Event::OverweightServiced { overweight_index: index, weight_used });
@@ -744,49 +755,31 @@ mod tests {
 			assert_eq!(overweights(), vec![0]);
 
 			assert_noop!(
-				DmpQueue::service_overweight(
-					RuntimeOrigin::signed(1),
-					0,
-					Weight::from_ref_time(20000)
-				),
+				DmpQueue::service_overweight(RuntimeOrigin::signed(1), 0, 20000),
 				BadOrigin
 			);
 			assert_noop!(
-				DmpQueue::service_overweight(
-					RuntimeOrigin::root(),
-					1,
-					Weight::from_ref_time(20000)
-				),
+				DmpQueue::service_overweight(RuntimeOrigin::root(), 1, 20000),
 				Error::<Test>::Unknown
 			);
 			assert_noop!(
-				DmpQueue::service_overweight(RuntimeOrigin::root(), 0, Weight::from_ref_time(9999)),
+				DmpQueue::service_overweight(RuntimeOrigin::root(), 0, 9999),
 				Error::<Test>::OverLimit
 			);
 			assert_eq!(take_trace(), vec![msg_limit_reached(10000)]);
 
-			let base_weight =
-				super::Call::<Test>::service_overweight { index: 0, weight_limit: Weight::zero() }
+			let base_weight = super::Call::<Test>::service_overweight { index: 0, weight_limit: 0 }
 				.get_dispatch_info()
 				.weight;
 			use frame_support::dispatch::GetDispatchInfo;
-			let info = DmpQueue::service_overweight(
-				RuntimeOrigin::root(),
-				0,
-				Weight::from_ref_time(20000),
-			)
-			.unwrap();
+			let info = DmpQueue::service_overweight(RuntimeOrigin::root(), 0, 20000).unwrap();
 			let actual_weight = info.actual_weight.unwrap();
 			assert_eq!(actual_weight, base_weight + Weight::from_ref_time(10000));
 			assert_eq!(take_trace(), vec![msg_complete(10000)]);
 			assert!(overweights().is_empty());
 
 			assert_noop!(
-				DmpQueue::service_overweight(
-					RuntimeOrigin::root(),
-					0,
-					Weight::from_ref_time(20000)
-				),
+				DmpQueue::service_overweight(RuntimeOrigin::root(), 0, 20000),
 				Error::<Test>::Unknown
 			);
 		});

cumulus/pallets/dmp-queue/src/migration.rs

@@ -0,0 +1,101 @@
+// Copyright 2022 Parity Technologies (UK) Ltd.
+// This file is part of Polkadot.
+
+// Polkadot is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// Polkadot is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with Polkadot.  If not, see <http://www.gnu.org/licenses/>.
+
+//! A module that is responsible for migration of storage.
+
+use crate::{Config, Pallet, Store};
+use frame_support::{
+	pallet_prelude::*,
+	traits::StorageVersion,
+	weights::{constants::WEIGHT_PER_MILLIS, Weight},
+};
+use xcm::latest::Weight as XcmWeight;
+
+/// The current storage version.
+pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
+/// Migrates the pallet storage to the most recent version, checking and setting the
+/// `StorageVersion`.
+pub fn migrate_to_latest<T: Config>() -> Weight {
+	let mut weight = T::DbWeight::get().reads(1);
+
+	if StorageVersion::get::<Pallet<T>>() == 0 {
+		weight += migrate_to_v1::<T>();
+		StorageVersion::new(1).put::<Pallet<T>>();
+	}
+
+	weight
+}
+
+mod v0 {
+	use super::*;
+	use codec::{Decode, Encode};
+
+	#[derive(Decode, Encode, Debug)]
+	pub struct ConfigData {
+		pub max_individual: XcmWeight,
+	}
+
+	impl Default for ConfigData {
+		fn default() -> Self {
+			ConfigData { max_individual: 10u64 * WEIGHT_PER_MILLIS.ref_time() }
+		}
+	}
+}
+
+/// Migrates `QueueConfigData` from v1 (using only reference time weights) to v2 (with
+/// 2D weights).
+///
+/// NOTE: Only use this function if you know what you're doing. Default to using
+/// `migrate_to_latest`.
+pub fn migrate_to_v1<T: Config>() -> Weight {
+	let translate = |pre: v0::ConfigData| -> super::ConfigData {
+		super::ConfigData { max_individual: Weight::from_ref_time(pre.max_individual) }
+	};
+
+	if let Err(_) = <Pallet<T> as Store>::Configuration::translate(|pre| pre.map(translate)) {
+		log::error!(
+			target: "dmp_queue",
+			"unexpected error when performing translation of the QueueConfig type during storage upgrade to v2"
+		);
+	}
+
+	T::DbWeight::get().reads_writes(1, 1)
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use crate::tests::{new_test_ext, Test};
+
+	#[test]
+	fn test_migration_to_v1() {
+		let v0 = v0::ConfigData { max_individual: 30_000_000_000 };
+
+		new_test_ext().execute_with(|| {
+			frame_support::storage::unhashed::put_raw(
+				&crate::Configuration::<Test>::hashed_key(),
+				&v0.encode(),
+			);
+
+			migrate_to_v1::<Test>();
+
+			let v1 = crate::Configuration::<Test>::get();
+
+			assert_eq!(v0.max_individual, v1.max_individual.ref_time());
+		});
+	}
+}

cumulus/pallets/xcmp-queue/src/benchmarking.rs

@@ -22,7 +22,7 @@ use frame_system::RawOrigin;
 
 benchmarks! {
 	set_config_with_u32 {}: update_resume_threshold(RawOrigin::Root, 100)
-	set_config_with_weight {}: update_weight_restrict_decay(RawOrigin::Root, Weight::from_ref_time(3_000_000))
+	set_config_with_weight {}: update_weight_restrict_decay(RawOrigin::Root, 3_000_000)
 }
 
 impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Test);

cumulus/pallets/xcmp-queue/src/lib.rs

@@ -54,7 +54,10 @@ use rand_chacha::{
 use scale_info::TypeInfo;
 use sp_runtime::{traits::Hash, RuntimeDebug};
 use sp_std::{convert::TryFrom, prelude::*};
-use xcm::{latest::prelude::*, VersionedXcm, WrapVersion, MAX_XCM_DECODE_DEPTH};
+use xcm::{
+	latest::{prelude::*, Weight as XcmWeight},
+	VersionedXcm, WrapVersion, MAX_XCM_DECODE_DEPTH,
+};
 use xcm_executor::traits::ConvertOrigin;
 
 pub use pallet::*;
@@ -130,11 +133,11 @@ pub mod pallet {
 		///
 		/// Events:
 		/// - `OverweightServiced`: On success.
-		#[pallet::weight((weight_limit.saturating_add(Weight::from_ref_time(1_000_000)), DispatchClass::Operational,))]
+		#[pallet::weight((Weight::from_ref_time(weight_limit.saturating_add(1_000_000)), DispatchClass::Operational,))]
 		pub fn service_overweight(
 			origin: OriginFor<T>,
 			index: OverweightIndex,
-			weight_limit: Weight,
+			weight_limit: XcmWeight,
 		) -> DispatchResultWithPostInfo {
 			T::ExecuteOverweightOrigin::ensure_origin(origin)?;
 
@@ -145,7 +148,8 @@ pub mod pallet {
 				&mut data.as_slice(),
 			)
 			.map_err(|_| Error::<T>::BadXcm)?;
-			let used = Self::handle_xcm_message(sender, sent_at, xcm, weight_limit)
+			let used =
+				Self::handle_xcm_message(sender, sent_at, xcm, Weight::from_ref_time(weight_limit))
 					.map_err(|_| Error::<T>::WeightOverLimit)?;
 			Overweight::<T>::remove(index);
 			Self::deposit_event(Event::OverweightServiced { index, used });
@@ -222,9 +226,9 @@ pub mod pallet {
 		/// - `origin`: Must pass `Root`.
 		/// - `new`: Desired value for `QueueConfigData.threshold_weight`
 		#[pallet::weight((T::WeightInfo::set_config_with_weight(), DispatchClass::Operational,))]
-		pub fn update_threshold_weight(origin: OriginFor<T>, new: Weight) -> DispatchResult {
+		pub fn update_threshold_weight(origin: OriginFor<T>, new: XcmWeight) -> DispatchResult {
 			ensure_root(origin)?;
-			QueueConfig::<T>::mutate(|data| data.threshold_weight = new);
+			QueueConfig::<T>::mutate(|data| data.threshold_weight = Weight::from_ref_time(new));
 
 			Ok(())
 		}
@@ -235,9 +239,14 @@ pub mod pallet {
 		/// - `origin`: Must pass `Root`.
 		/// - `new`: Desired value for `QueueConfigData.weight_restrict_decay`.
 		#[pallet::weight((T::WeightInfo::set_config_with_weight(), DispatchClass::Operational,))]
-		pub fn update_weight_restrict_decay(origin: OriginFor<T>, new: Weight) -> DispatchResult {
+		pub fn update_weight_restrict_decay(
+			origin: OriginFor<T>,
+			new: XcmWeight,
+		) -> DispatchResult {
 			ensure_root(origin)?;
-			QueueConfig::<T>::mutate(|data| data.weight_restrict_decay = new);
+			QueueConfig::<T>::mutate(|data| {
+				data.weight_restrict_decay = Weight::from_ref_time(new)
+			});
 
 			Ok(())
 		}
@@ -250,10 +259,12 @@ pub mod pallet {
 		#[pallet::weight((T::WeightInfo::set_config_with_weight(), DispatchClass::Operational,))]
 		pub fn update_xcmp_max_individual_weight(
 			origin: OriginFor<T>,
-			new: Weight,
+			new: XcmWeight,
 		) -> DispatchResult {
 			ensure_root(origin)?;
-			QueueConfig::<T>::mutate(|data| data.xcmp_max_individual_weight = new);
+			QueueConfig::<T>::mutate(|data| {
+				data.xcmp_max_individual_weight = Weight::from_ref_time(new)
+			});
 
 			Ok(())
 		}

cumulus/pallets/xcmp-queue/src/migration.rs

@@ -17,25 +17,30 @@
 //! A module that is responsible for migration of storage.
 
 use crate::{Config, Pallet, Store};
-use frame_support::{pallet_prelude::*, traits::StorageVersion, weights::Weight};
+use frame_support::{
+	pallet_prelude::*,
+	traits::StorageVersion,
+	weights::{constants::WEIGHT_PER_MILLIS, Weight},
+};
+use xcm::latest::Weight as XcmWeight;
 
 /// The current storage version.
-pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(2);
 
 /// Migrates the pallet storage to the most recent version, checking and setting the
 /// `StorageVersion`.
 pub fn migrate_to_latest<T: Config>() -> Weight {
-	let mut weight = Weight::zero();
+	let mut weight = T::DbWeight::get().reads(1);
 
-	if StorageVersion::get::<Pallet<T>>() == 0 {
-		weight += migrate_to_v1::<T>();
-		StorageVersion::new(1).put::<Pallet<T>>();
+	if StorageVersion::get::<Pallet<T>>() == 1 {
+		weight += migrate_to_v2::<T>();
+		StorageVersion::new(2).put::<Pallet<T>>();
 	}
 
 	weight
 }
 
-mod v0 {
+mod v1 {
 	use super::*;
 	use codec::{Decode, Encode};
 
@@ -44,8 +49,9 @@ mod v0 {
 		pub suspend_threshold: u32,
 		pub drop_threshold: u32,
 		pub resume_threshold: u32,
-		pub threshold_weight: Weight,
-		pub weight_restrict_decay: Weight,
+		pub threshold_weight: XcmWeight,
+		pub weight_restrict_decay: XcmWeight,
+		pub xcmp_max_individual_weight: XcmWeight,
 	}
 
 	impl Default for QueueConfigData {
@@ -54,37 +60,35 @@ mod v0 {
 				suspend_threshold: 2,
 				drop_threshold: 5,
 				resume_threshold: 1,
-				threshold_weight: Weight::from_ref_time(100_000),
-				weight_restrict_decay: Weight::from_ref_time(2),
+				threshold_weight: 100_000,
+				weight_restrict_decay: 2,
+				xcmp_max_individual_weight: 20u64 * WEIGHT_PER_MILLIS.ref_time(),
 			}
 		}
 	}
 }
 
-/// Migrates `QueueConfigData` from v0 (without the `xcmp_max_individual_weight` field) to v1 (with
-/// max individual weight).
-/// Uses the `Default` implementation of `QueueConfigData` to choose a value for
-/// `xcmp_max_individual_weight`.
+/// Migrates `QueueConfigData` from v1 (using only reference time weights) to v2 (with
+/// 2D weights).
 ///
 /// NOTE: Only use this function if you know what you're doing. Default to using
 /// `migrate_to_latest`.
-pub fn migrate_to_v1<T: Config>() -> Weight {
-	let translate = |pre: v0::QueueConfigData| -> super::QueueConfigData {
+pub fn migrate_to_v2<T: Config>() -> Weight {
+	let translate = |pre: v1::QueueConfigData| -> super::QueueConfigData {
 		super::QueueConfigData {
 			suspend_threshold: pre.suspend_threshold,
 			drop_threshold: pre.drop_threshold,
 			resume_threshold: pre.resume_threshold,
-			threshold_weight: pre.threshold_weight,
-			weight_restrict_decay: pre.weight_restrict_decay,
-			xcmp_max_individual_weight: super::QueueConfigData::default()
-				.xcmp_max_individual_weight,
+			threshold_weight: Weight::from_ref_time(pre.threshold_weight),
+			weight_restrict_decay: Weight::from_ref_time(pre.weight_restrict_decay),
+			xcmp_max_individual_weight: Weight::from_ref_time(pre.xcmp_max_individual_weight),
 		}
 	};
 
 	if let Err(_) = <Pallet<T> as Store>::QueueConfig::translate(|pre| pre.map(translate)) {
 		log::error!(
 			target: super::LOG_TARGET,
-			"unexpected error when performing translation of the QueueConfig type during storage upgrade to v1"
+			"unexpected error when performing translation of the QueueConfig type during storage upgrade to v2"
 		);
 	}
 
@@ -97,32 +101,32 @@ mod tests {
 	use crate::mock::{new_test_ext, Test};
 
 	#[test]
-	fn test_migration_to_v1() {
-		let v0 = v0::QueueConfigData {
+	fn test_migration_to_v2() {
+		let v1 = v1::QueueConfigData {
 			suspend_threshold: 5,
 			drop_threshold: 12,
 			resume_threshold: 3,
-			threshold_weight: Weight::from_ref_time(333_333),
-			weight_restrict_decay: Weight::from_ref_time(1),
+			threshold_weight: 333_333,
+			weight_restrict_decay: 1,
+			xcmp_max_individual_weight: 10_000_000_000,
 		};
 
 		new_test_ext().execute_with(|| {
-			// Put the v0 version in the state
 			frame_support::storage::unhashed::put_raw(
 				&crate::QueueConfig::<Test>::hashed_key(),
-				&v0.encode(),
+				&v1.encode(),
 			);
 
-			migrate_to_v1::<Test>();
+			migrate_to_v2::<Test>();
 
-			let v1 = crate::QueueConfig::<Test>::get();
+			let v2 = crate::QueueConfig::<Test>::get();
 
-			assert_eq!(v0.suspend_threshold, v1.suspend_threshold);
-			assert_eq!(v0.drop_threshold, v1.drop_threshold);
-			assert_eq!(v0.resume_threshold, v1.resume_threshold);
-			assert_eq!(v0.threshold_weight, v1.threshold_weight);
-			assert_eq!(v0.weight_restrict_decay, v1.weight_restrict_decay);
-			assert_eq!(v1.xcmp_max_individual_weight, Weight::from_ref_time(20_000_000_000));
+			assert_eq!(v1.suspend_threshold, v2.suspend_threshold);
+			assert_eq!(v1.drop_threshold, v2.drop_threshold);
+			assert_eq!(v1.resume_threshold, v2.resume_threshold);
+			assert_eq!(v1.threshold_weight, v2.threshold_weight.ref_time());
+			assert_eq!(v1.weight_restrict_decay, v2.weight_restrict_decay.ref_time());
+			assert_eq!(v1.xcmp_max_individual_weight, v2.xcmp_max_individual_weight.ref_time());
 		});
 	}
 }

cumulus/pallets/xcmp-queue/src/tests.rs

@@ -96,7 +96,7 @@ fn handle_invalid_data() {
 fn service_overweight_unknown() {
 	new_test_ext().execute_with(|| {
 		assert_noop!(
-			XcmpQueue::service_overweight(RuntimeOrigin::root(), 0, Weight::from_ref_time(1000)),
+			XcmpQueue::service_overweight(RuntimeOrigin::root(), 0, 1000),
 			Error::<Test>::BadOverweightIndex,
 		);
 	});
@@ -109,7 +109,7 @@ fn service_overweight_bad_xcm_format() {
 		Overweight::<Test>::insert(0, (ParaId::from(1000), 0, bad_xcm));
 
 		assert_noop!(
-			XcmpQueue::service_overweight(RuntimeOrigin::root(), 0, Weight::from_ref_time(1000)),
+			XcmpQueue::service_overweight(RuntimeOrigin::root(), 0, 1000),
 			Error::<Test>::BadXcm
 		);
 	});
@@ -187,15 +187,9 @@ fn update_threshold_weight_works() {
 	new_test_ext().execute_with(|| {
 		let data: QueueConfigData = <QueueConfig<Test>>::get();
 		assert_eq!(data.threshold_weight, Weight::from_ref_time(100_000));
-		assert_ok!(XcmpQueue::update_threshold_weight(
-			RuntimeOrigin::root(),
-			Weight::from_ref_time(10_000)
-		));
+		assert_ok!(XcmpQueue::update_threshold_weight(RuntimeOrigin::root(), 10_000));
 		assert_noop!(
-			XcmpQueue::update_threshold_weight(
-				RuntimeOrigin::signed(5),
-				Weight::from_ref_time(10_000_000)
-			),
+			XcmpQueue::update_threshold_weight(RuntimeOrigin::signed(5), 10_000_000),
 			BadOrigin
 		);
 		let data: QueueConfigData = <QueueConfig<Test>>::get();
@@ -209,15 +203,9 @@ fn update_weight_restrict_decay_works() {
 	new_test_ext().execute_with(|| {
 		let data: QueueConfigData = <QueueConfig<Test>>::get();
 		assert_eq!(data.weight_restrict_decay, Weight::from_ref_time(2));
-		assert_ok!(XcmpQueue::update_weight_restrict_decay(
-			RuntimeOrigin::root(),
-			Weight::from_ref_time(5)
-		));
+		assert_ok!(XcmpQueue::update_weight_restrict_decay(RuntimeOrigin::root(), 5));
 		assert_noop!(
-			XcmpQueue::update_weight_restrict_decay(
-				RuntimeOrigin::signed(6),
-				Weight::from_ref_time(4)
-			),
+			XcmpQueue::update_weight_restrict_decay(RuntimeOrigin::signed(6), 4),
 			BadOrigin
 		);
 		let data: QueueConfigData = <QueueConfig<Test>>::get();
@@ -233,12 +221,12 @@ fn update_xcmp_max_individual_weight() {
 		assert_eq!(data.xcmp_max_individual_weight, 20u64 * WEIGHT_PER_MILLIS);
 		assert_ok!(XcmpQueue::update_xcmp_max_individual_weight(
 			RuntimeOrigin::root(),
-			30u64 * WEIGHT_PER_MILLIS
+			30u64 * WEIGHT_PER_MILLIS.ref_time()
 		));
 		assert_noop!(
 			XcmpQueue::update_xcmp_max_individual_weight(
 				RuntimeOrigin::signed(3),
-				10u64 * WEIGHT_PER_MILLIS
+				10u64 * WEIGHT_PER_MILLIS.ref_time()
 			),
 			BadOrigin
 		);