From 5a11e1c41c7e137fa946301edf6608e877e421aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Mon, 18 Dec 2023 15:38:41 +0300 Subject: [PATCH 01/14] relaxing timepoint constraint for multisig --- substrate/frame/multisig/src/lib.rs | 25 +++--- substrate/frame/multisig/src/tests.rs | 115 ++++++++++++++++++++++---- 2 files changed, 113 insertions(+), 27 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index e4426c64b412..7ae903bd75a2 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -226,14 +226,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 +241,7 @@ pub mod pallet { /// A multisig operation has been cancelled. MultisigCancelled { cancelling: T::AccountId, - timepoint: Timepoint>, + timepoint: Option>>, multisig: T::AccountId, call_hash: CallHash, }, @@ -465,7 +465,7 @@ pub mod pallet { origin: OriginFor, threshold: u16, other_signatories: Vec, - timepoint: Timepoint>, + maybe_timepoint: Option>>, call_hash: [u8; 32], ) -> DispatchResult { let who = ensure_signed(origin)?; @@ -478,7 +478,9 @@ pub mod pallet { let id = Self::multi_account_id(&signatories, threshold); let m = >::get(&id, call_hash).ok_or(Error::::NotFound)?; - ensure!(m.when == timepoint, Error::::WrongTimepoint); + if let Some(timepoint) = maybe_timepoint.as_ref() { + ensure!(m.when == *timepoint, Error::::WrongTimepoint); + } ensure!(m.depositor == who, Error::::NotOwner); let err_amount = T::Currency::unreserve(&m.depositor, m.deposit); @@ -487,7 +489,7 @@ pub mod pallet { Self::deposit_event(Event::MultisigCancelled { cancelling: who, - timepoint, + timepoint: maybe_timepoint, multisig: id, call_hash, }); @@ -535,9 +537,10 @@ 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(timepoint) = maybe_timepoint.as_ref() { + // if the optional timepoint is supplied, it should be the correct one + ensure!(m.when == *timepoint, Error::::WrongTimepoint); + } // Ensure that either we have not yet signed or that it is at threshold. let mut approvals = m.approvals.len() as u16; @@ -564,7 +567,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 +593,7 @@ impl Pallet { >::insert(&id, call_hash, m); Self::deposit_event(Event::MultisigApproval { approving: who, - timepoint, + timepoint: maybe_timepoint, multisig: id, call_hash, }); diff --git a/substrate/frame/multisig/src/tests.rs b/substrate/frame/multisig/src/tests.rs index 179827255291..44208cbbd844 100644 --- a/substrate/frame/multisig/src/tests.rs +++ b/substrate/frame/multisig/src/tests.rs @@ -153,7 +153,13 @@ fn cancel_multisig_returns_deposit() { )); assert_eq!(Balances::free_balance(1), 6); assert_eq!(Balances::reserved_balance(1), 4); - 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], + Some(now()), + hash + )); assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::reserved_balance(1), 0); }); @@ -168,6 +174,7 @@ 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!( @@ -191,17 +198,6 @@ fn timepoint_checking_works() { Weight::zero() )); - assert_noop!( - Multisig::as_multi( - RuntimeOrigin::signed(2), - 2, - vec![1, 3], - None, - call.clone(), - Weight::zero() - ), - Error::::NoTimepoint, - ); let later = Timepoint { index: 1, ..now() }; assert_noop!( Multisig::as_multi( @@ -209,11 +205,22 @@ fn timepoint_checking_works() { 2, vec![1, 3], Some(later), - call, + call.clone(), Weight::zero() ), Error::::WrongTimepoint, ); + + assert_ok!(Multisig::as_multi( + RuntimeOrigin::signed(2), + 2, + vec![1, 3], + None, + call, + call_weight + )); + + assert_eq!(Balances::free_balance(6), 15); }); } @@ -250,6 +257,47 @@ fn multisig_2_of_3_works() { }); } +#[test] +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)); + 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() + )); + 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(|| { @@ -313,10 +361,45 @@ fn cancel_multisig_works() { Weight::zero() )); assert_noop!( - Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], now(), hash), + Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], Some(now()), hash), + Error::::NotOwner, + ); + assert_ok!(Multisig::cancel_as_multi( + RuntimeOrigin::signed(1), + 3, + vec![2, 3], + Some(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(RuntimeOrigin::signed(2), 3, vec![1, 3], None, 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], None, hash)); }); } @@ -452,7 +535,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()), From 72a21257aa94db3de637a23ce96a648ba1791c00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Mon, 18 Dec 2023 15:49:02 +0300 Subject: [PATCH 02/14] documentation fixes --- substrate/frame/multisig/src/lib.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 7ae903bd75a2..42b3b5d860bb 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -206,8 +206,6 @@ 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. @@ -328,8 +326,9 @@ pub mod pallet { /// - `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. + /// not the first approval, then it can be `Some`, with the timepoint (block number and + /// transaction index) of the first approval transaction. When provided, timepoint will serve + /// as another security layer. /// - `call`: The call to be executed. /// /// NOTE: Unless this is the final approval, you will generally want to use @@ -394,8 +393,9 @@ pub mod pallet { /// - `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. + /// not the first approval, then it can be `Some`, with the timepoint (block number and + /// transaction index) of the first approval transaction. When provided, timepoint will serve + /// as another security layer. /// - `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 +446,8 @@ 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. + /// - `maybe_timepoint`: The timepoint (block number and transaction index) of the first approval + /// transaction for this dispatch. When provided, timepoint will serve as another security layer. /// - `call_hash`: The hash of the call to be executed. /// /// ## Complexity From 27a54efe930995608cd123ee7b6c8b616826ace3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Mon, 18 Dec 2023 16:22:07 +0300 Subject: [PATCH 03/14] improve docs --- substrate/frame/multisig/src/lib.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 42b3b5d860bb..da1ce950f294 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -328,7 +328,7 @@ pub mod pallet { /// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is /// not the first approval, then it can be `Some`, with the timepoint (block number and /// transaction index) of the first approval transaction. When provided, timepoint will serve - /// as another security layer. + /// as an additional layer of security. /// - `call`: The call to be executed. /// /// NOTE: Unless this is the final approval, you will generally want to use @@ -395,7 +395,7 @@ pub mod pallet { /// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is /// not the first approval, then it can be `Some`, with the timepoint (block number and /// transaction index) of the first approval transaction. When provided, timepoint will serve - /// as another security layer. + /// as an additional layer of security. /// - `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. @@ -447,7 +447,8 @@ pub mod pallet { /// - `other_signatories`: The accounts (other than the sender) who can approve this /// dispatch. May not be empty. /// - `maybe_timepoint`: The timepoint (block number and transaction index) of the first approval - /// transaction for this dispatch. When provided, timepoint will serve as another security layer. + /// transaction for this dispatch. When provided, timepoint will serve as an additional + /// layer of security. /// - `call_hash`: The hash of the call to be executed. /// /// ## Complexity From b5c8889df787a54d8ab73267518bd6855a509834 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Tue, 19 Dec 2023 09:42:46 +0300 Subject: [PATCH 04/14] cancel without timepoint extrinsic --- substrate/frame/multisig/src/benchmarking.rs | 21 +++++ substrate/frame/multisig/src/lib.rs | 98 ++++++++++++++------ substrate/frame/multisig/src/tests.rs | 32 +++---- 3 files changed, 106 insertions(+), 45 deletions(-) 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 da1ce950f294..888524789f17 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -446,9 +446,9 @@ 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`: The timepoint (block number and transaction index) of the first approval - /// transaction for this dispatch. When provided, timepoint will serve as an additional - /// layer of security. + /// - `timepoint`: The timepoint (block number and transaction index) of the first approval. + /// Timepoint serves as an additional layer of security. If you do not wish to provide timepoint, + /// use `cancel_as_multi_without_timepoint`. /// - `call_hash`: The hash of the call to be executed. /// /// ## Complexity @@ -466,35 +466,42 @@ pub mod pallet { origin: OriginFor, threshold: u16, other_signatories: Vec, - maybe_timepoint: Option>>, + timepoint: Timepoint>, 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)?; - if let Some(timepoint) = maybe_timepoint.as_ref() { - 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: maybe_timepoint, - multisig: id, - call_hash, - }); - Ok(()) + /// Cancel a pre-existing, on-going multisig transaction. Any deposit reserved previously + /// for this operation will be unreserved on success. + /// + /// The dispatch origin for this call must be _Signed_. + /// + /// - `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. + /// - `call_hash`: The hash of the call to be executed. + /// + /// ## Complexity + /// - `O(S)`. + /// - Up to one balance-reserve or unreserve operation. + /// - One passthrough operation, one insert, both `O(S)` where `S` is the number of + /// signatories. `S` is capped by `MaxSignatories`, with weight being proportional. + /// - One encode & hash, both of complexity `O(S)`. + /// - One event. + /// - I/O: 1 read `O(S)`, one remove. + /// - Storage: removes one item. + #[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) } } } @@ -510,6 +517,41 @@ 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(timepoint) = maybe_timepoint.as_ref() { + // if the optional timepoint is supplied, it should be the correct one + 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::deposit_event(Event::MultisigCancelled { + cancelling: who, + timepoint: maybe_timepoint, + multisig: id, + call_hash, + }); + Ok(()) + } + fn operate( who: T::AccountId, threshold: u16, diff --git a/substrate/frame/multisig/src/tests.rs b/substrate/frame/multisig/src/tests.rs index 44208cbbd844..8de0b69bd4ce 100644 --- a/substrate/frame/multisig/src/tests.rs +++ b/substrate/frame/multisig/src/tests.rs @@ -153,13 +153,7 @@ fn cancel_multisig_returns_deposit() { )); assert_eq!(Balances::free_balance(1), 6); assert_eq!(Balances::reserved_balance(1), 4); - assert_ok!(Multisig::cancel_as_multi( - RuntimeOrigin::signed(1), - 3, - vec![2, 3], - Some(now()), - hash - )); + assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash)); assert_eq!(Balances::free_balance(1), 10); assert_eq!(Balances::reserved_balance(1), 0); }); @@ -361,16 +355,10 @@ fn cancel_multisig_works() { Weight::zero() )); assert_noop!( - Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], Some(now()), hash), + 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], - Some(now()), - hash - )); + assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash)); }); } @@ -396,10 +384,20 @@ fn cancel_multisig_without_timepoint_works() { Weight::zero() )); assert_noop!( - Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], None, hash), + Multisig::cancel_as_multi_without_timepoint( + RuntimeOrigin::signed(2), + 3, + vec![1, 3], + hash + ), Error::::NotOwner, ); - assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], None, hash)); + assert_ok!(Multisig::cancel_as_multi_without_timepoint( + RuntimeOrigin::signed(1), + 3, + vec![2, 3], + hash + )); }); } From 643f20d893c7b451193a392385161035e83185d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Wed, 27 Mar 2024 16:08:10 +0300 Subject: [PATCH 05/14] suggested documentation improvement --- substrate/frame/multisig/src/lib.rs | 38 ++++++++++++++--------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 888524789f17..5334c227dca9 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -39,6 +39,19 @@ //! number of signed origins. //! * `approve_as_multi` - Approve a call from a composite origin. //! * `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 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. +//! `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)] @@ -448,7 +461,9 @@ pub mod pallet { /// dispatch. May not be empty. /// - `timepoint`: The timepoint (block number and transaction index) of the first approval. /// Timepoint serves as an additional layer of security. If you do not wish to provide timepoint, - /// use `cancel_as_multi_without_timepoint`. + /// use `cancel_as_multi_without_timepoint` + /// (`timepoint` parameter is not an `Option`, but instead there is another function + /// `cancel_as_multi_without_timepoint` due to backwards compatibility). /// - `call_hash`: The hash of the call to be executed. /// /// ## Complexity @@ -473,25 +488,10 @@ pub mod pallet { Self::cancel(who, threshold, other_signatories, Some(timepoint), call_hash) } - /// Cancel a pre-existing, on-going multisig transaction. Any deposit reserved previously - /// for this operation will be unreserved on success. - /// - /// The dispatch origin for this call must be _Signed_. - /// - /// - `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. - /// - `call_hash`: The hash of the call to be executed. + /// Same as [`Pallet::cancel_as_multi`], but without a timepoint. /// - /// ## Complexity - /// - `O(S)`. - /// - Up to one balance-reserve or unreserve operation. - /// - One passthrough operation, one insert, both `O(S)` where `S` is the number of - /// signatories. `S` is capped by `MaxSignatories`, with weight being proportional. - /// - One encode & hash, both of complexity `O(S)`. - /// - One event. - /// - I/O: 1 read `O(S)`, one remove. - /// - Storage: removes one item. + /// If `threshold`, `signatories` and `call_hash` together will be enough for your use-case + /// to distinguish multisig operations, use this function instead of `cancel_as_multi` #[pallet::call_index(4)] #[pallet::weight(T::WeightInfo::cancel_as_multi(other_signatories.len() as u32))] pub fn cancel_as_multi_without_timepoint( From cd02b502d8a587d25f72214b598135eecf33a752 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Tue, 2 Apr 2024 12:39:47 +0300 Subject: [PATCH 06/14] enforcable optional timepoint --- substrate/frame/multisig/src/lib.rs | 86 +++++++++++++++++++---------- 1 file changed, 57 insertions(+), 29 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 5334c227dca9..60187099d9cc 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -36,8 +36,9 @@ //! ### 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. //! @@ -45,10 +46,11 @@ //! //! 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 indistinguishable multisig operations +//! `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. +//! 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`. @@ -122,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). @@ -221,8 +225,8 @@ pub mod pallet { NotOwner, /// 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. @@ -338,10 +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 can be `Some`, with the timepoint (block number and - /// transaction index) of the first approval transaction. When provided, timepoint will serve - /// as an additional layer of security. + /// - `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 not created with + /// `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 @@ -405,10 +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 can be `Some`, with the timepoint (block number and - /// transaction index) of the first approval transaction. When provided, timepoint will serve - /// as an additional layer of security. + /// - `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 not created with + /// `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. @@ -460,10 +478,10 @@ pub mod pallet { /// - `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. - /// Timepoint serves as an additional layer of security. If you do not wish to provide timepoint, - /// use `cancel_as_multi_without_timepoint` + /// 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 backwards compatibility). + /// `cancel_as_multi_without_timepoint` due to preserve backwards compatibility). /// - `call_hash`: The hash of the call to be executed. /// /// ## Complexity @@ -490,8 +508,8 @@ pub mod pallet { /// Same as [`Pallet::cancel_as_multi`], but without a timepoint. /// - /// If `threshold`, `signatories` and `call_hash` together will be enough for your use-case - /// to distinguish multisig operations, use this function instead of `cancel_as_multi` + /// 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( @@ -533,9 +551,13 @@ impl Pallet { let id = Self::multi_account_id(&signatories, threshold); let m = >::get(&id, call_hash).ok_or(Error::::NotFound)?; - if let Some(timepoint) = maybe_timepoint.as_ref() { - // if the optional timepoint is supplied, it should be the correct one - 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!(m.depositor == who, Error::::NotOwner); @@ -580,9 +602,13 @@ impl Pallet { // Branch on whether the operation has already started or not. if let Some(mut m) = >::get(&id, call_hash) { - if let Some(timepoint) = maybe_timepoint.as_ref() { - // if the optional timepoint is supplied, it should be the correct one - 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. @@ -652,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(); @@ -663,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, From 6077b6bcd7519599f2631364d69793802b90c2d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Tue, 2 Apr 2024 12:52:06 +0300 Subject: [PATCH 07/14] typo --- substrate/frame/multisig/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 60187099d9cc..737bad526b86 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -351,7 +351,7 @@ pub mod pallet { /// 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 not created with + /// 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. /// @@ -425,7 +425,7 @@ pub mod pallet { /// 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 not created with + /// 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. /// From 305e90cafc48ac535c220dedf6477e30767cb35a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Tue, 2 Apr 2024 18:14:33 +0300 Subject: [PATCH 08/14] migrations --- substrate/frame/multisig/src/lib.rs | 2 +- substrate/frame/multisig/src/migrations.rs | 133 +++++++++++++++++++-- 2 files changed, 127 insertions(+), 8 deletions(-) diff --git a/substrate/frame/multisig/src/lib.rs b/substrate/frame/multisig/src/lib.rs index 737bad526b86..83e7c060f8b5 100644 --- a/substrate/frame/multisig/src/lib.rs +++ b/substrate/frame/multisig/src/lib.rs @@ -186,7 +186,7 @@ pub mod pallet { } /// The current storage version. - const STORAGE_VERSION: StorageVersion = StorageVersion::new(1); + const STORAGE_VERSION: StorageVersion = StorageVersion::new(2); #[pallet::pallet] #[pallet::storage_version(STORAGE_VERSION)] diff --git a/substrate/frame/multisig/src/migrations.rs b/substrate/frame/multisig/src/migrations.rs index 330613bb3dfd..a98cff0db1a2 100644 --- a/substrate/frame/multisig/src/migrations.rs +++ b/substrate/frame/multisig/src/migrations.rs @@ -19,12 +19,17 @@ use super::*; use frame_support::{ - traits::{GetStorageVersion, OnRuntimeUpgrade, WrapperKeepOpaque}, - Identity, + pallet_prelude::*, + traits::{OnRuntimeUpgrade, WrapperKeepOpaque}, + weights::Weight, }; +use log; +use sp_runtime::traits::Saturating; #[cfg(feature = "try-runtime")] use frame_support::ensure; +#[cfg(feature = "try-runtime")] +use sp_runtime::TryRuntimeError; pub mod v1 { use super::*; @@ -42,21 +47,19 @@ pub mod v1 { pub struct MigrateToV1(sp_std::marker::PhantomData); impl OnRuntimeUpgrade for MigrateToV1 { #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, sp_runtime::TryRuntimeError> { + fn pre_upgrade() -> Result, 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::::current_storage_version(); let onchain = Pallet::::on_chain_storage_version(); if onchain > 0 { log!(info, "MigrateToV1 should be removed"); - return T::DbWeight::get().reads(1) + return T::DbWeight::get().reads(1); } let mut call_count = 0u64; @@ -76,7 +79,7 @@ pub mod v1 { } #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), sp_runtime::TryRuntimeError> { + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { ensure!( Calls::::iter().count() == 0, "there are some dangling calls that need to be destroyed and refunded" @@ -85,3 +88,119 @@ pub mod v1 { } } } + +pub mod v2 { + use super::*; + + /// An open multisig operation. + #[derive(Decode)] + pub struct OldMultisig + where + MaxApprovals: Get, + { + /// The extrinsic when the multisig operation was opened. + when: Timepoint, + /// 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). + depositor: AccountId, + /// The approvals achieved so far, including the depositor. Always sorted. + approvals: BoundedVec, + } + + impl + OldMultisig + where + MaxApprovals: Get, + { + fn migrate_to_v2(self) -> Multisig { + Multisig { + maybe_when: Some(self.when), + deposit: self.deposit, + depositor: self.depositor, + approvals: self.approvals, + } + } + } + + pub struct MigrateToV2(core::marker::PhantomData); + impl OnRuntimeUpgrade for MigrateToV2 { + #[cfg(feature = "try-runtime")] + fn pre_upgrade() -> Result, TryRuntimeError> { + frame_support::ensure!( + Pallet::::on_chain_storage_version() == 1, + "must upgrade linearly" + ); + let prev_count = Multisigs::::iter().count(); + Ok((prev_count as u32).encode()) + } + + fn on_runtime_upgrade() -> Weight { + let current_code_version = Pallet::::current_storage_version(); + let on_chain_version = Pallet::::on_chain_storage_version(); + + if on_chain_version == 1 && current_code_version == 2 { + let mut translated_count = 0u64; + Multisigs::::translate::< + OldMultisig, BalanceOf, T::AccountId, T::MaxSignatories>, + _, + >( + |_account, + _hash, + old_multisig: OldMultisig< + BlockNumberFor, + BalanceOf, + T::AccountId, + T::MaxSignatories, + >| { + translated_count.saturating_inc(); + Some(old_multisig.migrate_to_v2()) + }, + ); + + current_code_version.put::>(); + log::info!(target: LOG_TARGET, "Upgraded {} multisigs, storage to version {:?}", translated_count, current_code_version); + + T::DbWeight::get().reads_writes(translated_count + 1, translated_count + 1) + } else { + log::info!( + target: LOG_TARGET, + "Migration did not execute. This probably should be removed" + ); + T::DbWeight::get().reads(1) + } + } + + #[cfg(feature = "try-runtime")] + fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { + let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect( + "the state parameter should be something that was generated by pre_upgrade", + ); + let post_count = Multisigs::::iter().count() as u32; + ensure!( + prev_count == post_count, + "the multigis count before and after the migration should be the same" + ); + + let current_code_version = Pallet::::current_code_storage_version(); + let on_chain_version = Pallet::::on_chain_storage_version(); + + frame_support::ensure!(current_code_version == 2, "must_upgrade"); + ensure!( + current_code_version == on_chain_version, + "after migration, the current_code_version and on_chain_version should be the same" + ); + + Multisigs::::iter().try_for_each( + |(_account, _hash, multisig)| -> Result<(), TryRuntimeError> { + ensure!( + multisig.maybe_when.is_some(), + "all previous multisigs timepoint should be set to Some(timepoint)" + ); + Ok(()) + }, + )?; + Ok(()) + } + } +} From efb41c1ff1aae757eb0f8510b36bed18b2f8d49e Mon Sep 17 00:00:00 2001 From: Ozgun Ozerk Date: Tue, 2 Apr 2024 20:37:54 +0300 Subject: [PATCH 09/14] tests --- substrate/frame/multisig/src/migrations.rs | 20 ++-- substrate/frame/multisig/src/tests.rs | 110 +++++++++++++++------ 2 files changed, 89 insertions(+), 41 deletions(-) diff --git a/substrate/frame/multisig/src/migrations.rs b/substrate/frame/multisig/src/migrations.rs index d346ad491380..6e0d07bcc00a 100644 --- a/substrate/frame/multisig/src/migrations.rs +++ b/substrate/frame/multisig/src/migrations.rs @@ -54,7 +54,7 @@ pub mod v1 { } fn on_runtime_upgrade() -> Weight { - let current = Pallet::::current_storage_version(); + let in_code = Pallet::::in_code_storage_version(); let onchain = Pallet::::on_chain_storage_version(); if onchain > 0 { @@ -68,7 +68,7 @@ pub mod v1 { call_count.saturating_inc(); }); - current.put::>(); + in_code.put::>(); T::DbWeight::get().reads_writes( // Reads: Get Calls + Get Version @@ -136,10 +136,10 @@ pub mod v2 { } fn on_runtime_upgrade() -> Weight { - let current_code_version = Pallet::::current_storage_version(); + let in_code_version = Pallet::::in_code_storage_version(); let on_chain_version = Pallet::::on_chain_storage_version(); - if on_chain_version == 1 && current_code_version == 2 { + if on_chain_version == 1 && in_code_version == 2 { let mut translated_count = 0u64; Multisigs::::translate::< OldMultisig, BalanceOf, T::AccountId, T::MaxSignatories>, @@ -158,8 +158,8 @@ pub mod v2 { }, ); - current_code_version.put::>(); - log::info!(target: LOG_TARGET, "Upgraded {} multisigs, storage to version {:?}", translated_count, current_code_version); + in_code_version.put::>(); + log::info!(target: LOG_TARGET, "Upgraded {} multisigs, storage to version {:?}", translated_count, in_code_version); T::DbWeight::get().reads_writes(translated_count + 1, translated_count + 1) } else { @@ -182,13 +182,13 @@ pub mod v2 { "the multigis count before and after the migration should be the same" ); - let current_code_version = Pallet::::current_code_storage_version(); + let in_code_version = Pallet::::in_code_storage_version(); let on_chain_version = Pallet::::on_chain_storage_version(); - frame_support::ensure!(current_code_version == 2, "must_upgrade"); + frame_support::ensure!(in_code_version == 2, "must_upgrade"); ensure!( - current_code_version == on_chain_version, - "after migration, the current_code_version and on_chain_version should be the same" + in_code_version == on_chain_version, + "after migration, the in_code_version and on_chain_version should be the same" ); Multisigs::::iter().try_for_each( diff --git a/substrate/frame/multisig/src/tests.rs b/substrate/frame/multisig/src/tests.rs index bdc446b56834..980c1459f658 100644 --- a/substrate/frame/multisig/src/tests.rs +++ b/substrate/frame/multisig/src/tests.rs @@ -171,23 +171,11 @@ fn timepoint_checking_works() { 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() )); @@ -205,11 +193,78 @@ fn timepoint_checking_works() { Error::::WrongTimepoint, ); + assert_noop!( + Multisig::as_multi( + RuntimeOrigin::signed(2), + 2, + vec![1, 3], + None, + call.clone(), + Weight::zero() + ), + 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 )); @@ -233,7 +288,7 @@ fn multisig_2_of_3_works() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -307,7 +362,7 @@ fn multisig_3_of_3_works() { RuntimeOrigin::signed(1), 3, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -342,7 +397,7 @@ fn cancel_multisig_works() { RuntimeOrigin::signed(1), 3, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -415,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() )); @@ -450,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() )); @@ -458,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() )); @@ -499,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() )); @@ -517,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() )); @@ -599,7 +654,7 @@ fn duplicate_approvals_are_ignored() { RuntimeOrigin::signed(1), 2, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); @@ -709,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, ); }); @@ -740,7 +788,7 @@ fn multisig_handles_no_preimage_after_all_approve() { RuntimeOrigin::signed(1), 3, vec![2, 3], - None, + Some(now()), hash, Weight::zero() )); From 32e6d86bacf5f3ae3018d3d3a4a929c43e51015a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=96zg=C3=BCn=20=C3=96zerk?= Date: Thu, 4 Apr 2024 14:12:56 +0300 Subject: [PATCH 10/14] multi block migrations --- substrate/frame/multisig/src/migrations.rs | 206 ------------------ .../frame/multisig/src/migrations/mod.rs | 29 +++ .../multisig/src/migrations/v2/benchmarks.rs | 59 +++++ .../frame/multisig/src/migrations/v2/mod.rs | 155 +++++++++++++ .../frame/multisig/src/migrations/v2/tests.rs | 74 +++++++ .../multisig/src/migrations/v2/weights.rs | 84 +++++++ 6 files changed, 401 insertions(+), 206 deletions(-) delete mode 100644 substrate/frame/multisig/src/migrations.rs create mode 100644 substrate/frame/multisig/src/migrations/mod.rs create mode 100644 substrate/frame/multisig/src/migrations/v2/benchmarks.rs create mode 100644 substrate/frame/multisig/src/migrations/v2/mod.rs create mode 100644 substrate/frame/multisig/src/migrations/v2/tests.rs create mode 100644 substrate/frame/multisig/src/migrations/v2/weights.rs diff --git a/substrate/frame/multisig/src/migrations.rs b/substrate/frame/multisig/src/migrations.rs deleted file mode 100644 index 6e0d07bcc00a..000000000000 --- a/substrate/frame/multisig/src/migrations.rs +++ /dev/null @@ -1,206 +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::{ - pallet_prelude::*, - traits::{OnRuntimeUpgrade, WrapperKeepOpaque}, - weights::Weight, -}; -use log; -use sp_runtime::traits::Saturating; - -#[cfg(feature = "try-runtime")] -use frame_support::ensure; -#[cfg(feature = "try-runtime")] -use sp_runtime::TryRuntimeError; - -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, TryRuntimeError> { - log!(info, "Number of calls to refund and delete: {}", Calls::::iter().count()); - - Ok(Vec::new()) - } - - fn on_runtime_upgrade() -> Weight { - let in_code = 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(); - }); - - in_code.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<(), TryRuntimeError> { - ensure!( - Calls::::iter().count() == 0, - "there are some dangling calls that need to be destroyed and refunded" - ); - Ok(()) - } - } -} - -pub mod v2 { - use super::*; - - /// An open multisig operation. - #[derive(Decode)] - pub struct OldMultisig - where - MaxApprovals: Get, - { - /// The extrinsic when the multisig operation was opened. - when: Timepoint, - /// 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). - depositor: AccountId, - /// The approvals achieved so far, including the depositor. Always sorted. - approvals: BoundedVec, - } - - impl - OldMultisig - where - MaxApprovals: Get, - { - fn migrate_to_v2(self) -> Multisig { - Multisig { - maybe_when: Some(self.when), - deposit: self.deposit, - depositor: self.depositor, - approvals: self.approvals, - } - } - } - - pub struct MigrateToV2(core::marker::PhantomData); - impl OnRuntimeUpgrade for MigrateToV2 { - #[cfg(feature = "try-runtime")] - fn pre_upgrade() -> Result, TryRuntimeError> { - frame_support::ensure!( - Pallet::::on_chain_storage_version() == 1, - "must upgrade linearly" - ); - let prev_count = Multisigs::::iter().count(); - Ok((prev_count as u32).encode()) - } - - fn on_runtime_upgrade() -> Weight { - let in_code_version = Pallet::::in_code_storage_version(); - let on_chain_version = Pallet::::on_chain_storage_version(); - - if on_chain_version == 1 && in_code_version == 2 { - let mut translated_count = 0u64; - Multisigs::::translate::< - OldMultisig, BalanceOf, T::AccountId, T::MaxSignatories>, - _, - >( - |_account, - _hash, - old_multisig: OldMultisig< - BlockNumberFor, - BalanceOf, - T::AccountId, - T::MaxSignatories, - >| { - translated_count.saturating_inc(); - Some(old_multisig.migrate_to_v2()) - }, - ); - - in_code_version.put::>(); - log::info!(target: LOG_TARGET, "Upgraded {} multisigs, storage to version {:?}", translated_count, in_code_version); - - T::DbWeight::get().reads_writes(translated_count + 1, translated_count + 1) - } else { - log::info!( - target: LOG_TARGET, - "Migration did not execute. This probably should be removed" - ); - T::DbWeight::get().reads(1) - } - } - - #[cfg(feature = "try-runtime")] - fn post_upgrade(_state: Vec) -> Result<(), TryRuntimeError> { - let prev_count: u32 = Decode::decode(&mut prev_count.as_slice()).expect( - "the state parameter should be something that was generated by pre_upgrade", - ); - let post_count = Multisigs::::iter().count() as u32; - ensure!( - prev_count == post_count, - "the multigis count before and after the migration should be the same" - ); - - let in_code_version = Pallet::::in_code_storage_version(); - let on_chain_version = Pallet::::on_chain_storage_version(); - - frame_support::ensure!(in_code_version == 2, "must_upgrade"); - ensure!( - in_code_version == on_chain_version, - "after migration, the in_code_version and on_chain_version should be the same" - ); - - Multisigs::::iter().try_for_each( - |(_account, _hash, multisig)| -> Result<(), TryRuntimeError> { - ensure!( - multisig.maybe_when.is_some(), - "all previous multisigs timepoint should be set to Some(timepoint)" - ); - Ok(()) - }, - )?; - 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..07ff617384d9 --- /dev/null +++ b/substrate/frame/multisig/src/migrations/v2/mod.rs @@ -0,0 +1,155 @@ +// 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::{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 crate::pallet::Config; + use frame_support::pallet_prelude::*; + use frame_support::storage_alias; + + use crate::{BalanceOf, BlockNumberFor, Timepoint}; + + /// An open multisig operation. + #[derive(Decode)] + 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< + _, // TODO: empty prefix is not allowed, what value should be supplied here? + Twox64Concat, + T::AccountId, + Blake2_128Concat, + [u8; 32], + OldMultisig, BalanceOf, T::AccountId, T::MaxSignatories>, + >; +} + +use crate::MaxEncodedLen; +use crate::{Decode, Encode}; +// TODO: did not want to touch frame/support/src/migrations.rs +// so I moved it here, please double check +#[derive(MaxEncodedLen, Encode, Decode)] +pub struct MigrationId { + pub pallet_id: [u8; N], + pub version_from: u8, + pub version_to: u8, +} + +/// 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, last_key2, new_multisig); + cursor = Some((last_key1, last_key2)) // Return the processed key as the new cursor. + } else { + 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)) + } +} From 0dfa81b72de6386fda8b35a8c5a1825b8dc76916 Mon Sep 17 00:00:00 2001 From: Ozgun Ozerk Date: Fri, 5 Apr 2024 21:49:22 +0300 Subject: [PATCH 11/14] prefix --- substrate/frame/multisig/src/migrations/v2/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/multisig/src/migrations/v2/mod.rs b/substrate/frame/multisig/src/migrations/v2/mod.rs index 07ff617384d9..cf5512530024 100644 --- a/substrate/frame/multisig/src/migrations/v2/mod.rs +++ b/substrate/frame/multisig/src/migrations/v2/mod.rs @@ -56,7 +56,7 @@ mod v1 { #[storage_alias] pub type Multisigs = StorageDoubleMap< - _, // TODO: empty prefix is not allowed, what value should be supplied here? + Pallet, Twox64Concat, T::AccountId, Blake2_128Concat, From b1ec99372057a0d8f2d6737b9329b2952157020d Mon Sep 17 00:00:00 2001 From: Ozgun Ozerk Date: Mon, 15 Apr 2024 14:19:18 +0300 Subject: [PATCH 12/14] fix cargo check errors --- .../frame/multisig/src/migrations/v2/mod.rs | 26 +++++++++++-------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/substrate/frame/multisig/src/migrations/v2/mod.rs b/substrate/frame/multisig/src/migrations/v2/mod.rs index cf5512530024..3975e59e15be 100644 --- a/substrate/frame/multisig/src/migrations/v2/mod.rs +++ b/substrate/frame/multisig/src/migrations/v2/mod.rs @@ -32,14 +32,13 @@ pub mod weights; /// Before running this migration, the storage alias defined here represents the /// `on_chain` storage. mod v1 { - use crate::pallet::Config; - use frame_support::pallet_prelude::*; - use frame_support::storage_alias; + use frame_support::{pallet_prelude::*, storage_alias}; + use frame_system::Config as SystemConfig; - use crate::{BalanceOf, BlockNumberFor, Timepoint}; + use crate::{pallet::Config, BalanceOf, BlockNumberFor, Pallet, Timepoint}; /// An open multisig operation. - #[derive(Decode)] + #[derive(Decode, Encode)] pub struct OldMultisig where MaxApprovals: Get, @@ -58,15 +57,19 @@ mod v1 { pub type Multisigs = StorageDoubleMap< Pallet, Twox64Concat, - T::AccountId, + ::AccountId, Blake2_128Concat, [u8; 32], - OldMultisig, BalanceOf, T::AccountId, T::MaxSignatories>, + OldMultisig< + BlockNumberFor, + BalanceOf, + ::AccountId, + ::MaxSignatories, + >, >; } -use crate::MaxEncodedLen; -use crate::{Decode, Encode}; +use crate::{Decode, Encode, MaxEncodedLen}; // TODO: did not want to touch frame/support/src/migrations.rs // so I moved it here, please double check #[derive(MaxEncodedLen, Encode, Decode)] @@ -133,7 +136,8 @@ impl SteppedMigration for LazyMigrationV2`) -> `maybe_when` (`Option>`) + // Migrate the `when` field (`Timepoint`) -> `maybe_when` + // (`Option>`) let new_multisig = Multisig { maybe_when: Some(value.when), deposit: value.deposit, @@ -143,7 +147,7 @@ impl SteppedMigration for LazyMigrationV2::insert(last_key1, last_key2, new_multisig); + Multisigs::::insert(last_key1.clone(), last_key2, new_multisig); cursor = Some((last_key1, last_key2)) // Return the processed key as the new cursor. } else { cursor = None; // Signal that the migration is complete (no more items to process). From 675dc2663b644ccfeb85c4e1376ad80d4c5b561a Mon Sep 17 00:00:00 2001 From: Ozgun Ozerk Date: Tue, 23 Apr 2024 12:57:57 +0300 Subject: [PATCH 13/14] import MigrationId --- substrate/frame/multisig/src/migrations/v2/mod.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/substrate/frame/multisig/src/migrations/v2/mod.rs b/substrate/frame/multisig/src/migrations/v2/mod.rs index 3975e59e15be..862ac35f7203 100644 --- a/substrate/frame/multisig/src/migrations/v2/mod.rs +++ b/substrate/frame/multisig/src/migrations/v2/mod.rs @@ -18,7 +18,7 @@ use super::PALLET_MIGRATIONS_ID; use crate::{pallet::Config, Multisig, Multisigs}; use frame_support::{ - migrations::{SteppedMigration, SteppedMigrationError}, + migrations::{MigrationId, SteppedMigration, SteppedMigrationError}, pallet_prelude::PhantomData, weights::WeightMeter, }; @@ -69,16 +69,6 @@ mod v1 { >; } -use crate::{Decode, Encode, MaxEncodedLen}; -// TODO: did not want to touch frame/support/src/migrations.rs -// so I moved it here, please double check -#[derive(MaxEncodedLen, Encode, Decode)] -pub struct MigrationId { - pub pallet_id: [u8; N], - pub version_from: u8, - pub version_to: u8, -} - /// Migrates the items of the [`crate::Multisigs`] map, by wrapping the timepoint value with `Some`, /// to support new optional timepoint feature. /// From 2346f1335f7d01a6850ab417a897ad4ab5e92d4d Mon Sep 17 00:00:00 2001 From: Ozgun Ozerk Date: Tue, 23 Apr 2024 19:23:49 +0300 Subject: [PATCH 14/14] storage version added --- substrate/frame/multisig/src/migrations/v2/mod.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/substrate/frame/multisig/src/migrations/v2/mod.rs b/substrate/frame/multisig/src/migrations/v2/mod.rs index 862ac35f7203..b6459a9ad09a 100644 --- a/substrate/frame/multisig/src/migrations/v2/mod.rs +++ b/substrate/frame/multisig/src/migrations/v2/mod.rs @@ -140,6 +140,7 @@ impl SteppedMigration for LazyMigrationV2::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; }