From 7bb5e1360b16e87f3e07be3b2c7fbccd27c3a152 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Tue, 20 Sep 2022 16:14:08 +0200 Subject: [PATCH 1/8] [Feature] Add a migration that drains and refunds stored calls --- frame/multisig/src/lib.rs | 25 ++++++++++++-- frame/multisig/src/migrations.rs | 56 ++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 frame/multisig/src/migrations.rs diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index d38bbccab3684..e8daf6648a697 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -47,6 +47,7 @@ #![cfg_attr(not(feature = "std"), no_std)] mod benchmarking; +pub mod migrations; mod tests; pub mod weights; @@ -58,7 +59,7 @@ use frame_support::{ }, ensure, traits::{Currency, Get, ReservableCurrency}, - weights::{GetDispatchInfo, Weight}, + weights::Weight, RuntimeDebug, }; use frame_system::{self as system, RawOrigin}; @@ -73,6 +74,20 @@ pub use weights::WeightInfo; pub use pallet::*; +/// The log target of this pallet. +pub const LOG_TARGET: &'static str = "runtime::multisig"; + +// syntactic sugar for logging. +#[macro_export] +macro_rules! log { + ($level:tt, $patter:expr $(, $values:expr)* $(,)?) => { + log::$level!( + target: crate::LOG_TARGET, + concat!("[{:?}] 🏊‍♂️ ", $patter), >::block_number() $(, $values)* + ) + }; +} + type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; @@ -103,7 +118,7 @@ pub struct Multisig { type CallHash = [u8; 32]; enum CallOrHash { - Call(::Call), + Call(::RuntimeCall), Hash([u8; 32]), } @@ -150,9 +165,13 @@ pub mod pallet { type WeightInfo: WeightInfo; } + /// The current storage version. + const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + #[pallet::pallet] #[pallet::generate_store(pub(super) trait Store)] #[pallet::without_storage_info] + #[pallet::storage_version(STORAGE_VERSION)] pub struct Pallet(_); /// The set of open multisig operations. @@ -356,7 +375,7 @@ pub mod pallet { threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, - call: Box<::Call>, + call: Box<::RuntimeCall>, max_weight: Weight, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs new file mode 100644 index 0000000000000..995b91beea1e8 --- /dev/null +++ b/frame/multisig/src/migrations.rs @@ -0,0 +1,56 @@ +use super::*; +use frame_support::{ + traits::{OnRuntimeUpgrade, WrapperKeepOpaque}, + Blake2_256, +}; + +#[cfg(feature = "try-runtime")] +use frame_support::ensure; + +pub mod v1 { + use super::*; + + type OpaqueCall = WrapperKeepOpaque<::RuntimeCall>; + + #[frame_support::storage_alias] + type Calls = StorageMap< + crate::Pallet, + Blake2_256, + [u8; 32], + (OpaqueCall, T::AccountId, BalanceOf), + >; + + pub struct MigrateToV1(sp_std::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV1 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result<(), &'static str> { + let current = Pallet::::current_storage_version(); + let onchain = Pallet::::on_chain_storage_version(); + + ensure!(onchain < 1, "this migration can be deleted"); + + log!(info, "Number of calls to refund and delete: {}", Calls::count()); + + Ok(()) + } + + fn on_runtime_upgrade() -> Weight { + let onchain = Pallet::::on_chain_storage_version(); + + ensure!(onchain > 0, "this migration can be deleted"); + + let calls = Calls::::drain().for_each(|call_hash, (_data, caller, deposit)| { + T::Currency::unreserve(&caller, deposit)?; + }); + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade() -> Result<(), &'static str> { + let onchain = Pallet::::on_chain_storage_version(); + + ensure!(onchain > 0, "this migration needs to be run"); + ensure!(Calls::count() == 0, + "there are some dangling calls that need to be destroyed and refunded"); + } + } +} From 82345391e8f3ab1378ea817b2c3481499efeedff Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Tue, 20 Sep 2022 16:52:12 +0200 Subject: [PATCH 2/8] migration fixes --- Cargo.lock | 1 + frame/multisig/Cargo.toml | 3 +++ frame/multisig/src/lib.rs | 2 +- frame/multisig/src/migrations.rs | 28 +++++++++++++++++++++------- 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2ee1adc3e087e..15fa5970f60ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5893,6 +5893,7 @@ dependencies = [ "frame-benchmarking", "frame-support", "frame-system", + "log", "pallet-balances", "parity-scale-codec", "scale-info", diff --git a/frame/multisig/Cargo.toml b/frame/multisig/Cargo.toml index a370215032714..319765e703e8f 100644 --- a/frame/multisig/Cargo.toml +++ b/frame/multisig/Cargo.toml @@ -22,6 +22,9 @@ sp-io = { version = "6.0.0", default-features = false, path = "../../primitives/ sp-runtime = { version = "6.0.0", default-features = false, path = "../../primitives/runtime" } sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" } +# third party +log = { version = "0.4.17", default-features = false } + [dev-dependencies] pallet-balances = { version = "4.0.0-dev", path = "../balances" } sp-core = { version = "6.0.0", path = "../../primitives/core" } diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index e8daf6648a697..20addd20338ff 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -83,7 +83,7 @@ macro_rules! log { ($level:tt, $patter:expr $(, $values:expr)* $(,)?) => { log::$level!( target: crate::LOG_TARGET, - concat!("[{:?}] 🏊‍♂️ ", $patter), >::block_number() $(, $values)* + concat!("[{:?}] ✍️ ", $patter), >::block_number() $(, $values)* ) }; } diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 995b91beea1e8..685b67454b2c0 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -1,7 +1,8 @@ use super::*; use frame_support::{ + dispatch::GetStorageVersion, traits::{OnRuntimeUpgrade, WrapperKeepOpaque}, - Blake2_256, + Identity, }; #[cfg(feature = "try-runtime")] @@ -14,10 +15,10 @@ pub mod v1 { #[frame_support::storage_alias] type Calls = StorageMap< - crate::Pallet, - Blake2_256, + Pallet, + Identity, [u8; 32], - (OpaqueCall, T::AccountId, BalanceOf), + (OpaqueCall, ::AccountId, BalanceOf), >; pub struct MigrateToV1(sp_std::marker::PhantomData); @@ -35,13 +36,26 @@ pub mod v1 { } fn on_runtime_upgrade() -> Weight { + let current = Pallet::::current_storage_version(); let onchain = Pallet::::on_chain_storage_version(); - ensure!(onchain > 0, "this migration can be deleted"); + if onchain > 0 { + log!(info, "MigrateToV1 should be removed"); + return T::DbWeight::get().reads(1) + } + + // TODO: Assuming this is one read + let calls_read = Calls::::iter().count() as u64; - let calls = Calls::::drain().for_each(|call_hash, (_data, caller, deposit)| { - T::Currency::unreserve(&caller, deposit)?; + // TODO: Assuming this is one write and a read per record + Calls::::drain().for_each(|(_call_hash, (_data, caller, deposit))| { + //TODO: What's the weight of one unreserve? + T::Currency::unreserve(&caller, deposit); }); + + current.put::>(); + + T::DbWeight::get().reads_writes(calls_read + 1, 2) } #[cfg(feature = "try-runtime")] From 35a4c77ceafd69bb1641ec1f6d09e81c66fdc493 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Tue, 20 Sep 2022 16:55:38 +0200 Subject: [PATCH 3/8] fixes --- frame/multisig/src/migrations.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 685b67454b2c0..06f1e46e9d7e8 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -25,12 +25,11 @@ pub mod v1 { impl OnRuntimeUpgrade for MigrateToV1 { #[cfg(feature = "try-runtime")] fn pre_upgrade() -> Result<(), &'static str> { - let current = Pallet::::current_storage_version(); let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain < 1, "this migration can be deleted"); - log!(info, "Number of calls to refund and delete: {}", Calls::count()); + log!(info, "Number of calls to refund and delete: {}", Calls::::iter().count()); Ok(()) } @@ -63,8 +62,11 @@ pub mod v1 { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain > 0, "this migration needs to be run"); - ensure!(Calls::count() == 0, - "there are some dangling calls that need to be destroyed and refunded"); + ensure!( + Calls::::iter().count() == 0, + "there are some dangling calls that need to be destroyed and refunded" + ); + Ok(()) } } } From 9c0ad6d945b202eb73217bb7b188070740f67a45 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Wed, 21 Sep 2022 11:16:47 +0200 Subject: [PATCH 4/8] address review comments --- frame/multisig/src/migrations.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 06f1e46e9d7e8..21afe1e70ccd1 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -43,11 +43,11 @@ pub mod v1 { return T::DbWeight::get().reads(1) } - // TODO: Assuming this is one read - let calls_read = Calls::::iter().count() as u64; + let mut calls_read = 0u64; // TODO: Assuming this is one write and a read per record Calls::::drain().for_each(|(_call_hash, (_data, caller, deposit))| { + calls_read += 1; //TODO: What's the weight of one unreserve? T::Currency::unreserve(&caller, deposit); }); From 1e8a954141343577a58d7172efbbb115a4920c18 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Wed, 21 Sep 2022 13:04:26 +0200 Subject: [PATCH 5/8] consume the whole block weight --- frame/multisig/src/migrations.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 21afe1e70ccd1..bae722ed2158d 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -43,18 +43,15 @@ pub mod v1 { return T::DbWeight::get().reads(1) } - let mut calls_read = 0u64; - // TODO: Assuming this is one write and a read per record Calls::::drain().for_each(|(_call_hash, (_data, caller, deposit))| { - calls_read += 1; //TODO: What's the weight of one unreserve? T::Currency::unreserve(&caller, deposit); }); current.put::>(); - T::DbWeight::get().reads_writes(calls_read + 1, 2) + ::BlockWeights::get().max_block } #[cfg(feature = "try-runtime")] From a4c139d03a4579c16ef4747e684149192a6e508a Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Mon, 26 Sep 2022 12:00:09 +0200 Subject: [PATCH 6/8] fix assertions --- frame/multisig/src/migrations.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index bae722ed2158d..648ac1a09d61a 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -43,9 +43,7 @@ pub mod v1 { return T::DbWeight::get().reads(1) } - // TODO: Assuming this is one write and a read per record Calls::::drain().for_each(|(_call_hash, (_data, caller, deposit))| { - //TODO: What's the weight of one unreserve? T::Currency::unreserve(&caller, deposit); }); @@ -57,8 +55,8 @@ pub mod v1 { #[cfg(feature = "try-runtime")] fn post_upgrade() -> Result<(), &'static str> { let onchain = Pallet::::on_chain_storage_version(); - - ensure!(onchain > 0, "this migration needs to be run"); + ensure!(onchain < 2, "this migration needs to be removed"); + ensure!(onchain == 1, "this migration needs to be run"); ensure!( Calls::::iter().count() == 0, "there are some dangling calls that need to be destroyed and refunded" From dbb2cd2dfc42cc4950f3f2cb24c63f52b8a82ff4 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Wed, 28 Sep 2022 11:56:36 +0200 Subject: [PATCH 7/8] license header --- frame/multisig/src/migrations.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 648ac1a09d61a..0f45792fa0965 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -1,3 +1,22 @@ +// This file is part of Substrate. + +// Copyright (C) 2019-2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Migrations for Multisig Pallet + use super::*; use frame_support::{ dispatch::GetStorageVersion, From 53779781b6240c98b0c86f2bc68f42a5827b4716 Mon Sep 17 00:00:00 2001 From: Roman Useinov Date: Wed, 28 Sep 2022 12:00:26 +0200 Subject: [PATCH 8/8] fix interface --- frame/multisig/src/migrations.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/multisig/src/migrations.rs b/frame/multisig/src/migrations.rs index 0f45792fa0965..5085297cde433 100644 --- a/frame/multisig/src/migrations.rs +++ b/frame/multisig/src/migrations.rs @@ -43,14 +43,14 @@ pub mod v1 { pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result<(), &'static str> { + fn pre_upgrade() -> Result, &'static str> { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain < 1, "this migration can be deleted"); log!(info, "Number of calls to refund and delete: {}", Calls::::iter().count()); - Ok(()) + Ok(Vec::new()) } fn on_runtime_upgrade() -> Weight { @@ -72,7 +72,7 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade() -> Result<(), &'static str> { + fn post_upgrade(_state: Vec) -> Result<(), &'static str> { let onchain = Pallet::::on_chain_storage_version(); ensure!(onchain < 2, "this migration needs to be removed"); ensure!(onchain == 1, "this migration needs to be run");