diff --git a/substrate/frame/multisig/src/benchmarking.rs b/substrate/frame/multisig/src/benchmarking.rs index ebe19df5dc43..6a4763741ec0 100644 --- a/substrate/frame/multisig/src/benchmarking.rs +++ b/substrate/frame/multisig/src/benchmarking.rs @@ -210,5 +210,26 @@ benchmarks! { assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); } + cancel_as_multi_without_timepoint { + // Signatories, need at least 2 people + let s in 2 .. T::MaxSignatories::get(); + // Transaction Length, not a component + let z = 10_000; + let (mut signatories, call) = setup_multi::(s, z)?; + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); + let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + let call_hash = call.using_encoded(blake2_256); + // Create the multi + let o = RawOrigin::Signed(caller.clone()).into(); + Multisig::::as_multi(o, s as u16, signatories.clone(), None, call, Weight::zero())?; + assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); + // Whitelist caller account from further DB operations. + let caller_key = frame_system::Account::::hashed_key_for(&caller); + frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into()); + }: _(RawOrigin::Signed(caller), s as u16, signatories, call_hash) + verify { + assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); + } + impl_benchmark_test_suite!(Multisig, crate::tests::new_test_ext(), crate::tests::Test); } diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index a83b78e316f5..dd33228ce370 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -36,9 +36,24 @@ //! ### Dispatchable Functions //! //! * `as_multi` - Approve and if possible dispatch a call from a composite origin formed from a -//! number of signed origins. +//! number of signed origins. Can be also used to create the multisig operation. //! * `approve_as_multi` - Approve a call from a composite origin. +//! Can be also used to create the multisig operation. //! * `cancel_as_multi` - Cancel a call from a composite origin. +//! * `cancel_as_multi_without_timepoint` - Cancel a call from a composite origin without providing a timepoint. +//! +//! ### Security +//! +//! Multisig operations are stored in a DoubleKeyMap, where the keys are `call_hash` +//! and `id` (derived from threshold and signatories). +//! `timepoint` acts as an identifier for otherwise would be indistinguishable multisig operations +//! (e.g. a recurring payment with, say, threshold 2, signatories A, B, C, and the same `call_hash`), +//! hence granting an additional layer of security. +//! However, retrieving and providing the `timepoint` may become an arduous task from the UX perspective, +//! for those who will not benefit from this extra security measure. +//! `timepoint` is made optional to balance the tradeoff between UX and security. +//! If `threshold`, `signatories` and `call_hash` together will be enough for your use-case to distinguish +//! multisig operations, then you can opt-out of `timepoint`. // Ensure we're `no_std` when compiling for Wasm. #![cfg_attr(not(feature = "std"), no_std)] @@ -109,7 +124,9 @@ where MaxApprovals: Get, { /// The extrinsic when the multisig operation was opened. - when: Timepoint, + /// `timepoint` is an optional extra security measure, which can be enabled by the creator of + /// the multisig during the creation by supplying a `timepoint` for the `maybe_when` field. + maybe_when: Option>, /// The amount held in reserve of the `depositor`, to be returned once the operation ends. deposit: Balance, /// The account who opened it (i.e. the first to approve it). @@ -169,7 +186,7 @@ pub mod pallet { } /// The in-code storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(2); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] @@ -206,12 +223,10 @@ pub mod pallet { NotFound, /// Only the account that originally created the multisig is able to cancel it. NotOwner, - /// No timepoint was given, yet the multisig operation is already underway. - NoTimepoint, /// A different timepoint was given to the multisig operation that is underway. WrongTimepoint, - /// A timepoint was given, yet no multisig operation is underway. - UnexpectedTimepoint, + /// Timepoint security measure is enabled, yet no timepoint is provided for the dispatchable. + MissingTimepoint, /// The maximum weight information provided was too low. MaxWeightTooLow, /// The data to be stored is already stored. @@ -226,14 +241,14 @@ pub mod pallet { /// A multisig operation has been approved by someone. MultisigApproval { approving: T::AccountId, - timepoint: Timepoint>, + timepoint: Option>>, multisig: T::AccountId, call_hash: CallHash, }, /// A multisig operation has been executed. MultisigExecuted { approving: T::AccountId, - timepoint: Timepoint>, + timepoint: Option>>, multisig: T::AccountId, call_hash: CallHash, result: DispatchResult, @@ -241,7 +256,7 @@ pub mod pallet { /// A multisig operation has been cancelled. MultisigCancelled { cancelling: T::AccountId, - timepoint: Timepoint>, + timepoint: Option>>, multisig: T::AccountId, call_hash: CallHash, }, @@ -327,9 +342,17 @@ pub mod pallet { /// - `threshold`: The total number of approvals for this dispatch before it is executed. /// - `other_signatories`: The accounts (other than the sender) who can approve this /// dispatch. May not be empty. - /// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is - /// not the first approval, then it must be `Some`, with the timepoint (block number and - /// transaction index) of the first approval transaction. + /// - `maybe_timepoint`: Optional extra security measure for differentiating otherwise would be + /// indistinguishable multisig operations (i.e. recurring payments). To enable this feature, the + /// creator (the first approval) must supply `Some(timepoint)`. The provided timepoint value + /// will be ignored, because `timepoint` will be the block number and the index this multisig + /// is submitted at. Providing `Some(timepoint)` for the first approval just acts as a + /// flag that enables `timepoint` security measure. + /// For the multisigs that are created with `timepoint`, the following approvals must supply + /// the timepoint for that multisig. + /// For the multisigs that are NOT created with `timepoint`, the following approvals can provide `None`. + /// This function does not error if a `timepoint` is provided for a multisig that is created WITHOUT + /// `timepoint`, the provided value will be simply ignored. /// - `call`: The call to be executed. /// /// NOTE: Unless this is the final approval, you will generally want to use @@ -393,9 +416,17 @@ pub mod pallet { /// - `threshold`: The total number of approvals for this dispatch before it is executed. /// - `other_signatories`: The accounts (other than the sender) who can approve this /// dispatch. May not be empty. - /// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is - /// not the first approval, then it must be `Some`, with the timepoint (block number and - /// transaction index) of the first approval transaction. + /// - `maybe_timepoint`: Optional extra security measure for differentiating otherwise would be + /// indistinguishable multisig operations (i.e. recurring payments). To enable this feature, the + /// creator (the first approval) must supply `Some(timepoint)`. The provided timepoint value + /// will be ignored, because `timepoint` will be the block number and the index this multisig + /// is submitted at. Providing `Some(timepoint)` for the first approval just acts as a + /// flag that enables `timepoint` security measure. + /// For the multisigs that are created with `timepoint`, the following approvals must supply + /// the timepoint for that multisig. + /// For the multisigs that are NOT created with `timepoint`, the following approvals can provide `None`. + /// This function does not error if a `timepoint` is provided for a multisig that is created WITHOUT + /// `timepoint`, the provided value will be simply ignored. /// - `call_hash`: The hash of the call to be executed. /// /// NOTE: If this is the final approval, you will want to use `as_multi` instead. @@ -446,8 +477,11 @@ pub mod pallet { /// - `threshold`: The total number of approvals for this dispatch before it is executed. /// - `other_signatories`: The accounts (other than the sender) who can approve this /// dispatch. May not be empty. - /// - `timepoint`: The timepoint (block number and transaction index) of the first approval - /// transaction for this dispatch. + /// - `timepoint`: The timepoint (block number and transaction index) of the first approval. + /// Timepoint serves as an additional layer of security. If the multisig is created without `timepoint`, + /// use `[`Pallet::cancel_as_multi_without_timepoint`]` + /// (`timepoint` parameter is not an `Option`, but instead there is another function + /// `cancel_as_multi_without_timepoint` due to preserve backwards compatibility). /// - `call_hash`: The hash of the call to be executed. /// /// ## Complexity @@ -469,29 +503,23 @@ pub mod pallet { call_hash: [u8; 32], ) -> DispatchResult { let who = ensure_signed(origin)?; - ensure!(threshold >= 2, Error::::MinimumThreshold); - let max_sigs = T::MaxSignatories::get() as usize; - ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); - ensure!(other_signatories.len() < max_sigs, Error::::TooManySignatories); - let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; - - let id = Self::multi_account_id(&signatories, threshold); - - let m = >::get(&id, call_hash).ok_or(Error::::NotFound)?; - ensure!(m.when == timepoint, Error::::WrongTimepoint); - ensure!(m.depositor == who, Error::::NotOwner); - - let err_amount = T::Currency::unreserve(&m.depositor, m.deposit); - debug_assert!(err_amount.is_zero()); - >::remove(&id, &call_hash); + Self::cancel(who, threshold, other_signatories, Some(timepoint), call_hash) + } - Self::deposit_event(Event::MultisigCancelled { - cancelling: who, - timepoint, - multisig: id, - call_hash, - }); - Ok(()) + /// Same as [`Pallet::cancel_as_multi`], but without a timepoint. + /// + /// If the multisig is created with a `timepoint`, use [`Pallet::cancel_as_multi`], + /// if the multisig is created without a `timepoint`, use this function. + #[pallet::call_index(4)] + #[pallet::weight(T::WeightInfo::cancel_as_multi(other_signatories.len() as u32))] + pub fn cancel_as_multi_without_timepoint( + origin: OriginFor, + threshold: u16, + other_signatories: Vec, + call_hash: [u8; 32], + ) -> DispatchResult { + let who = ensure_signed(origin)?; + Self::cancel(who, threshold, other_signatories, None, call_hash) } } } @@ -507,6 +535,45 @@ impl Pallet { .expect("infinite length input; no invalid inputs for type; qed") } + fn cancel( + who: T::AccountId, + threshold: u16, + other_signatories: Vec, + maybe_timepoint: Option>>, + call_hash: [u8; 32], + ) -> DispatchResult { + ensure!(threshold >= 2, Error::::MinimumThreshold); + let max_sigs = T::MaxSignatories::get() as usize; + ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); + ensure!(other_signatories.len() < max_sigs, Error::::TooManySignatories); + let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; + + let id = Self::multi_account_id(&signatories, threshold); + + let m = >::get(&id, call_hash).ok_or(Error::::NotFound)?; + if let Some(when) = m.maybe_when { + // `timepoint` security measure is enabled for the multisig, + // the caller of the dispatchable should have provided a value for `timepoint`. + match maybe_timepoint { + Some(timepoint) => ensure!(when == timepoint, Error::::WrongTimepoint), + None => return Err(Error::::MissingTimepoint.into()), + } + } + ensure!(m.depositor == who, Error::::NotOwner); + + let err_amount = T::Currency::unreserve(&m.depositor, m.deposit); + debug_assert!(err_amount.is_zero()); + >::remove(&id, &call_hash); + + Self::deposit_event(Event::MultisigCancelled { + cancelling: who, + timepoint: maybe_timepoint, + multisig: id, + call_hash, + }); + Ok(()) + } + fn operate( who: T::AccountId, threshold: u16, @@ -535,9 +602,14 @@ impl Pallet { // Branch on whether the operation has already started or not. if let Some(mut m) = >::get(&id, call_hash) { - // Yes; ensure that the timepoint exists and agrees. - let timepoint = maybe_timepoint.ok_or(Error::::NoTimepoint)?; - ensure!(m.when == timepoint, Error::::WrongTimepoint); + if let Some(when) = m.maybe_when { + // `timepoint` security measure is enabled for the multisig, + // the caller of the dispatchable should have provided a value for `timepoint`. + match maybe_timepoint { + Some(timepoint) => ensure!(when == timepoint, Error::::WrongTimepoint), + None => return Err(Error::::MissingTimepoint.into()), + } + } // Ensure that either we have not yet signed or that it is at threshold. let mut approvals = m.approvals.len() as u16; @@ -564,7 +636,7 @@ impl Pallet { let result = call.dispatch(RawOrigin::Signed(id.clone()).into()); Self::deposit_event(Event::MultisigExecuted { approving: who, - timepoint, + timepoint: maybe_timepoint, multisig: id, call_hash, result: result.map(|_| ()).map_err(|e| e.error), @@ -590,7 +662,7 @@ impl Pallet { >::insert(&id, call_hash, m); Self::deposit_event(Event::MultisigApproval { approving: who, - timepoint, + timepoint: maybe_timepoint, multisig: id, call_hash, }); @@ -606,9 +678,6 @@ impl Pallet { Ok(Some(final_weight).into()) } } else { - // Not yet started; there should be no timepoint given. - ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); - // Just start the operation by recording it in storage. let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into(); @@ -617,11 +686,16 @@ impl Pallet { let initial_approvals = vec![who.clone()].try_into().map_err(|_| Error::::TooManySignatories)?; + // to enable timepoint security measure, user may have supplied a + // timepoint during creation (the value of it does not matter), + // override the value with the current block number and index as the timepoint. + let maybe_when = maybe_timepoint.map(|_| Self::timepoint()); + >::insert( &id, call_hash, Multisig { - when: Self::timepoint(), + maybe_when, deposit, depositor: who.clone(), approvals: initial_approvals, diff --git a/substrate/frame/multisig/src/migrations.rs b/substrate/frame/multisig/src/migrations.rs deleted file mode 100644 index e6402600d0d3..000000000000 --- a/substrate/frame/multisig/src/migrations.rs +++ /dev/null @@ -1,87 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 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::{ - traits::{GetStorageVersion, OnRuntimeUpgrade, WrapperKeepOpaque}, - Identity, -}; - -#[cfg(feature = "try-runtime")] -use frame_support::ensure; - -pub mod v1 { - use super::*; - - type OpaqueCall = WrapperKeepOpaque<::RuntimeCall>; - - #[frame_support::storage_alias] - type Calls = StorageMap< - Pallet, - Identity, - [u8; 32], - (OpaqueCall, ::AccountId, BalanceOf), - >; - - pub struct MigrateToV1(core::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV1 { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { - log!(info, "Number of calls to refund and delete: {}", Calls::::iter().count()); - - Ok(Vec::new()) - } - - fn on_runtime_upgrade() -> Weight { - use sp_runtime::Saturating; - - let current = Pallet::::in_code_storage_version(); - let onchain = Pallet::::on_chain_storage_version(); - - if onchain > 0 { - log!(info, "MigrateToV1 should be removed"); - return T::DbWeight::get().reads(1) - } - - let mut call_count = 0u64; - Calls::::drain().for_each(|(_call_hash, (_data, caller, deposit))| { - T::Currency::unreserve(&caller, deposit); - call_count.saturating_inc(); - }); - - current.put::>(); - - T::DbWeight::get().reads_writes( - // Reads: Get Calls + Get Version - call_count.saturating_add(1), - // Writes: Drain Calls + Unreserves + Set version - call_count.saturating_mul(2).saturating_add(1), - ) - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { - ensure!( - Calls::::iter().count() == 0, - "there are some dangling calls that need to be destroyed and refunded" - ); - Ok(()) - } - } -} diff --git a/substrate/frame/multisig/src/migrations/mod.rs b/substrate/frame/multisig/src/migrations/mod.rs new file mode 100644 index 000000000000..2e82cc967d9d --- /dev/null +++ b/substrate/frame/multisig/src/migrations/mod.rs @@ -0,0 +1,29 @@ +// This file is part of Substrate. + +// Copyright (C) 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. + +/// # Multi-Block Migrations Module +/// +/// This module showcases a simple use of the multi-block migrations framework. +pub mod v2; + +/// A unique identifier across all pallets. +/// +/// This constant represents a unique identifier for the migrations of this pallet. +/// It helps differentiate migrations for this pallet from those of others. Note that we don't +/// directly pull the crate name from the environment, since that would change if the crate were +/// ever to be renamed and could cause historic migrations to run again. +pub const PALLET_MIGRATIONS_ID: &[u8; 15] = b"pallet-multisig"; diff --git a/substrate/frame/multisig/src/migrations/v2/benchmarks.rs b/substrate/frame/multisig/src/migrations/v2/benchmarks.rs new file mode 100644 index 000000000000..46e3beb31849 --- /dev/null +++ b/substrate/frame/multisig/src/migrations/v2/benchmarks.rs @@ -0,0 +1,59 @@ +// This file is part of Substrate. + +// Copyright (C) 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. + +//! Benchmark the multi-block-migration. + +#![cfg(feature = "runtime-benchmarks")] + +use crate::{ + migrations::{ + v2, + v2::{weights, weights::WeightInfo}, + }, + Config, Pallet, +}; +use frame_benchmarking::v2::*; +use frame_support::{migrations::SteppedMigration, weights::WeightMeter}; + +#[benchmarks] +mod benches { + use super::*; + + /// Benchmark a single step of the `v1::LazyMigrationV1` migration. + #[benchmark] + fn step() { + let multi_account = multi_account_id(&[1, 2, 3][..], 2); + let call = call_transfer(6, 15).encode(); + let hash = blake2_256(&call); + let val = OldMultisig { when: now(), deposit: 100, depositor: 1, approvals: &[1] }; + + v2::v1::Multisigs::::insert(multi_account, hash, val); + let mut meter = WeightMeter::new(); + + #[block] + { + v2::LazyMigrationV2::>::step(None, &mut meter).unwrap(); + } + + // Check that the new storage is decodable: + assert_eq!(crate::Multisigs::::get(multi_account), Some(val)); + // uses twice the weight once for migration and then for checking if there is another key. + assert_eq!(meter.consumed(), weights::SubstrateWeight::::step() * 2); + } + + impl_benchmark_test_suite!(Pallet, crate::mock::new_test_ext(), crate::mock::Runtime); +} diff --git a/substrate/frame/multisig/src/migrations/v2/mod.rs b/substrate/frame/multisig/src/migrations/v2/mod.rs new file mode 100644 index 000000000000..b6459a9ad09a --- /dev/null +++ b/substrate/frame/multisig/src/migrations/v2/mod.rs @@ -0,0 +1,150 @@ +// This file is part of Substrate. + +// Copyright (C) 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. + +use super::PALLET_MIGRATIONS_ID; +use crate::{pallet::Config, Multisig, Multisigs}; +use frame_support::{ + migrations::{MigrationId, SteppedMigration, SteppedMigrationError}, + pallet_prelude::PhantomData, + weights::WeightMeter, +}; + +mod benchmarks; +mod tests; +pub mod weights; + +/// Module containing the OLD (v1) storage items. +/// +/// Before running this migration, the storage alias defined here represents the +/// `on_chain` storage. +mod v1 { + use frame_support::{pallet_prelude::*, storage_alias}; + use frame_system::Config as SystemConfig; + + use crate::{pallet::Config, BalanceOf, BlockNumberFor, Pallet, Timepoint}; + + /// An open multisig operation. + #[derive(Decode, Encode)] + pub struct OldMultisig + where + MaxApprovals: Get, + { + /// The extrinsic when the multisig operation was opened. + pub when: Timepoint, + /// The amount held in reserve of the `depositor`, to be returned once the operation ends. + pub deposit: Balance, + /// The account who opened it (i.e. the first to approve it). + pub depositor: AccountId, + /// The approvals achieved so far, including the depositor. Always sorted. + pub approvals: BoundedVec, + } + + #[storage_alias] + pub type Multisigs = StorageDoubleMap< + Pallet, + Twox64Concat, + ::AccountId, + Blake2_128Concat, + [u8; 32], + OldMultisig< + BlockNumberFor, + BalanceOf, + ::AccountId, + ::MaxSignatories, + >, + >; +} + +/// Migrates the items of the [`crate::Multisigs`] map, by wrapping the timepoint value with `Some`, +/// to support new optional timepoint feature. +/// +/// The `step` function will be called once per block. It is very important that this function +/// *never* panics and never uses more weight than it got in its meter. The migrations should also +/// try to make maximal progress per step, so that the total time it takes to migrate stays low. +struct LazyMigrationV2(PhantomData<(T, W)>); +impl SteppedMigration for LazyMigrationV2 { + type Cursor = (T::AccountId, [u8; 32]); + // Without the explicit length here the construction of the ID would not be infallible. + type Identifier = MigrationId<15>; + + /// The identifier of this migration. Which should be globally unique. + fn id() -> Self::Identifier { + MigrationId { pallet_id: *PALLET_MIGRATIONS_ID, version_from: 1, version_to: 2 } + } + + /// The actual logic of the migration. + /// + /// This function is called repeatedly until it returns `Ok(None)`, indicating that the + /// migration is complete. Ideally, the migration should be designed in such a way that each + /// step consumes as much weight as possible. However, this is simplified to perform one stored + /// value mutation per block. + fn step( + mut cursor: Option, + meter: &mut WeightMeter, + ) -> Result, SteppedMigrationError> { + let required = W::step(); + // If there is not enough weight for a single step, return an error. This case can be + // problematic if it is the first migration that ran in this block. But there is nothing + // that we can do about it here. + if meter.remaining().any_lt(required) { + return Err(SteppedMigrationError::InsufficientWeight { required }); + } + + // We loop here to do as much progress as possible per step. + loop { + if meter.try_consume(required).is_err() { + break; + } + + let mut iter = if let Some(last_key) = cursor { + // If a cursor is provided, start iterating from the stored value + // corresponding to the last key processed in the previous step. + // Note that this only works if the old and the new map use the same way to hash + // storage keys. + + v1::Multisigs::::iter_from(v1::Multisigs::::hashed_key_for( + last_key.0, last_key.1, + )) + } else { + // If no cursor is provided, start iterating from the beginning. + v1::Multisigs::::iter() + }; + + // If there's a next item in the iterator, perform the migration. + if let Some((last_key1, last_key2, value)) = iter.next() { + // Migrate the `when` field (`Timepoint`) -> `maybe_when` + // (`Option>`) + let new_multisig = Multisig { + maybe_when: Some(value.when), + deposit: value.deposit, + depositor: value.depositor, + approvals: value.approvals, + }; + + // We can just insert here since the old and the new map share the same key-space. + // Otherwise it would have to invert the concat hash function and re-hash it. + Multisigs::::insert(last_key1.clone(), last_key2, new_multisig); + cursor = Some((last_key1, last_key2)) // Return the processed key as the new cursor. + } else { + StorageVersion::new(2).put::(); + cursor = None; // Signal that the migration is complete (no more items to process). + break; + } + } + Ok(cursor) + } +} diff --git a/substrate/frame/multisig/src/migrations/v2/tests.rs b/substrate/frame/multisig/src/migrations/v2/tests.rs new file mode 100644 index 000000000000..9d52bfbb8f60 --- /dev/null +++ b/substrate/frame/multisig/src/migrations/v2/tests.rs @@ -0,0 +1,74 @@ +// This file is part of Substrate. + +// Copyright (C) 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. + +#![cfg(all(test, not(feature = "runtime-benchmarks")))] + +use crate::{ + migrations::{ + v2, + v2::{weights, weights::WeightInfo as _}, + }, + mock::{ + new_test_ext, run_to_block, AllPalletsWithSystem, MigratorServiceWeight, Runtime as T, + System, + }, +}; +use frame_support::traits::OnRuntimeUpgrade; +use pallet_migrations::WeightInfo as _; + +#[test] +fn lazy_migration_works() { + new_test_ext().execute_with(|| { + frame_support::__private::sp_tracing::try_init_simple(); + // Insert some values into the old storage map. + for i in 0..1024 { + let multi_account = multi_account_id(&[i, 2, 3][..], 2); + let call = call_transfer(6, i).encode(); + let hash = blake2_256(&call); + let val = OldMultisig { when: now(), deposit: 100, depositor: i, approvals: &[i] }; + v2::v1::Multisigs::::insert(multi_account, hash, val); + } + + // Give it enough weight to do exactly 16 iterations: + let limit = ::WeightInfo::progress_mbms_none() + + pallet_migrations::Pallet::::exec_migration_max_weight() + + weights::SubstrateWeight::::step() * 16; + MigratorServiceWeight::set(&limit); + + System::set_block_number(1); + AllPalletsWithSystem::on_runtime_upgrade(); // onboard MBMs + + let mut last_decodable = 0; + for block in 2..=65 { + run_to_block(block); + let mut decodable = 0; + for i in 0..1024 { + if crate::MyMap::::get(i).is_some() { + decodable += 1; + } + } + + assert_eq!(decodable, last_decodable + 16); + last_decodable = decodable; + } + + // Check that everything is decodable now: + for i in 0..1024 { + assert_eq!(crate::MyMap::::get(i).unwrap().maybe_when, Some(now())); + } + }); +} diff --git a/substrate/frame/multisig/src/migrations/v2/weights.rs b/substrate/frame/multisig/src/migrations/v2/weights.rs new file mode 100644 index 000000000000..1b4d3a69262a --- /dev/null +++ b/substrate/frame/multisig/src/migrations/v2/weights.rs @@ -0,0 +1,84 @@ +// This file is part of Substrate. + +// Copyright (C) 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. + +//! Autogenerated weights for `pallet_example_mbm` +//! +//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 32.0.0 +//! DATE: 2024-03-26, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! WORST CASE MAP SIZE: `1000000` +//! HOSTNAME: `Olivers-MBP`, CPU: `` +//! WASM-EXECUTION: `Compiled`, CHAIN: `None`, DB CACHE: `1024` + +// Executed Command: +// polkadot-omni-bencher +// v1 +// benchmark +// pallet +// --runtime +// target/release/wbuild/kitchensink-runtime/kitchensink_runtime.compact.compressed.wasm +// --pallet +// pallet_example_mbm +// --extrinsic +// +// --template +// substrate/.maintain/frame-weight-template.hbs +// --output +// substrate/frame/examples/multi-block-migrations/src/migrations/weights.rs + +#![cfg_attr(rustfmt, rustfmt_skip)] +#![allow(unused_parens)] +#![allow(unused_imports)] +#![allow(missing_docs)] + +use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}}; +use core::marker::PhantomData; + +/// Weight functions needed for `pallet_example_mbm`. +pub trait WeightInfo { + fn step() -> Weight; +} + +/// Weights for `pallet_example_mbm` using the Substrate node and recommended hardware. +pub struct SubstrateWeight(PhantomData); +impl WeightInfo for SubstrateWeight { + /// Storage: `PalletMultisig::Multisigs` (r:2 w:1) + /// Proof: `PalletMultisig::Multisigs` (`max_values`: None, `max_size`: Some(28), added: 2503, mode: `MaxEncodedLen`) + fn step() -> Weight { + // Proof Size summary in bytes: + // Measured: `28` + // Estimated: `5996` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(8_000_000, 5996) + .saturating_add(T::DbWeight::get().reads(2_u64)) + .saturating_add(T::DbWeight::get().writes(1_u64)) + } +} + +// For backwards compatibility and tests. +impl WeightInfo for () { + /// Storage: `PalletMultisig::Multisigs` (r:2 w:1) + /// Proof: `PalletMultisig::Multisigs` (`max_values`: None, `max_size`: Some(28), added: 2503, mode: `MaxEncodedLen`) + fn step() -> Weight { + // Proof Size summary in bytes: + // Measured: `28` + // Estimated: `5996` + // Minimum execution time: 6_000_000 picoseconds. + Weight::from_parts(8_000_000, 5996) + .saturating_add(RocksDbWeight::get().reads(2_u64)) + .saturating_add(RocksDbWeight::get().writes(1_u64)) + } +} diff --git a/substrate/frame/multisig/src/tests.rs b/substrate/frame/multisig/src/tests.rs index 0d73e3db6615..980c1459f658 100644 --- a/substrate/frame/multisig/src/tests.rs +++ b/substrate/frame/multisig/src/tests.rs @@ -168,52 +168,108 @@ fn timepoint_checking_works() { assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5)); let call = call_transfer(6, 15); + let call_weight = call.get_dispatch_info().weight; let hash = blake2_256(&call.encode()); - assert_noop!( - Multisig::approve_as_multi( - RuntimeOrigin::signed(2), - 2, - vec![1, 3], - Some(now()), - hash, - Weight::zero() - ), - Error::::UnexpectedTimepoint, - ); - assert_ok!(Multisig::approve_as_multi( RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); + let later = Timepoint { index: 1, ..now() }; assert_noop!( Multisig::as_multi( RuntimeOrigin::signed(2), 2, vec![1, 3], - None, + Some(later), call.clone(), Weight::zero() ), - Error::::NoTimepoint, + Error::::WrongTimepoint, ); - let later = Timepoint { index: 1, ..now() }; + assert_noop!( Multisig::as_multi( RuntimeOrigin::signed(2), 2, vec![1, 3], - Some(later), - call, + None, + call.clone(), Weight::zero() ), - Error::::WrongTimepoint, + Error::::MissingTimepoint, ); + + assert_ok!(Multisig::as_multi( + RuntimeOrigin::signed(2), + 2, + vec![1, 3], + Some(now()), + call, + call_weight + )); + + assert_eq!(Balances::free_balance(6), 15); + }); +} + +#[test] +fn no_restrictions_for_no_timepoint() { + new_test_ext().execute_with(|| { + let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5)); + + let call = call_transfer(6, 15); + let call_weight = call.get_dispatch_info().weight; + let hash = blake2_256(&call.encode()); + + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(1), + 3, + vec![2, 3], + None, + hash, + Weight::zero() + )); + + let later = Timepoint { index: 1, ..now() }; + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(2), + 3, + vec![1, 3], + Some(later), + hash, + Weight::zero() + )); + + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(3), + 3, + vec![1, 2], + None, + hash, + Weight::zero() + )); + + assert_eq!(Balances::free_balance(6), 0); + + assert_ok!(Multisig::as_multi( + RuntimeOrigin::signed(2), + 3, + vec![1, 3], + Some(now()), + call, + call_weight + )); + + assert_eq!(Balances::free_balance(6), 15); }); } @@ -232,7 +288,7 @@ fn multisig_2_of_3_works() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -251,7 +307,7 @@ fn multisig_2_of_3_works() { } #[test] -fn multisig_3_of_3_works() { +fn multisig_3_of_3_without_timepoint_works() { new_test_ext().execute_with(|| { let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3); assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); @@ -269,6 +325,47 @@ fn multisig_3_of_3_works() { hash, Weight::zero() )); + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(3), + 3, + vec![1, 2], + None, + hash, + Weight::zero() + )); + assert_eq!(Balances::free_balance(6), 0); + + assert_ok!(Multisig::as_multi( + RuntimeOrigin::signed(2), + 3, + vec![1, 3], + None, + call, + call_weight + )); + assert_eq!(Balances::free_balance(6), 15); + }); +} + +#[test] +fn multisig_3_of_3_works() { + new_test_ext().execute_with(|| { + let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5)); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5)); + assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5)); + + let call = call_transfer(6, 15); + let call_weight = call.get_dispatch_info().weight; + let hash = blake2_256(&call.encode()); + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(1), + 3, + vec![2, 3], + Some(now()), + hash, + Weight::zero() + )); assert_ok!(Multisig::approve_as_multi( RuntimeOrigin::signed(2), 3, @@ -300,7 +397,7 @@ fn cancel_multisig_works() { RuntimeOrigin::signed(1), 3, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -316,7 +413,46 @@ fn cancel_multisig_works() { Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], now(), hash), Error::::NotOwner, ); - assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash),); + assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash)); + }); +} + +#[test] +fn cancel_multisig_without_timepoint_works() { + new_test_ext().execute_with(|| { + let call = call_transfer(6, 15).encode(); + let hash = blake2_256(&call); + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(1), + 3, + vec![2, 3], + None, + hash, + Weight::zero() + )); + assert_ok!(Multisig::approve_as_multi( + RuntimeOrigin::signed(2), + 3, + vec![1, 3], + None, + hash, + Weight::zero() + )); + assert_noop!( + Multisig::cancel_as_multi_without_timepoint( + RuntimeOrigin::signed(2), + 3, + vec![1, 3], + hash + ), + Error::::NotOwner, + ); + assert_ok!(Multisig::cancel_as_multi_without_timepoint( + RuntimeOrigin::signed(1), + 3, + vec![2, 3], + hash + )); }); } @@ -334,7 +470,7 @@ fn multisig_2_of_3_as_multi_works() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), call.clone(), Weight::zero() )); @@ -369,7 +505,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), call1.clone(), Weight::zero() )); @@ -377,7 +513,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() { RuntimeOrigin::signed(2), 2, vec![1, 3], - None, + Some(now()), call2.clone(), Weight::zero() )); @@ -418,7 +554,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), call.clone(), Weight::zero() )); @@ -436,7 +572,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), call.clone(), Weight::zero() )); @@ -452,7 +588,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() { System::assert_last_event( pallet_multisig::Event::MultisigExecuted { approving: 3, - timepoint: now(), + timepoint: Some(now()), multisig: multi, call_hash: hash, result: Err(TokenError::FundsUnavailable.into()), @@ -518,7 +654,7 @@ fn duplicate_approvals_are_ignored() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -628,14 +764,7 @@ fn weight_check_works() { assert_eq!(Balances::free_balance(6), 0); assert_noop!( - Multisig::as_multi( - RuntimeOrigin::signed(2), - 2, - vec![1, 3], - Some(now()), - call, - Weight::zero() - ), + Multisig::as_multi(RuntimeOrigin::signed(2), 2, vec![1, 3], None, call, Weight::zero()), Error::::MaxWeightTooLow, ); }); @@ -659,7 +788,7 @@ fn multisig_handles_no_preimage_after_all_approve() { RuntimeOrigin::signed(1), 3, vec![2, 3], - None, + Some(now()), hash, Weight::zero() ));