Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[Feature] Add a migration that drains and refunds stored calls #12313

Merged
merged 11 commits into from
Sep 29, 2022
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions frame/multisig/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ sp-io = { version = "6.0.0", default-features = false, path = "../../primitives/
sp-runtime = { version = "6.0.0", default-features = false, path = "../../primitives/runtime" }
sp-std = { version = "4.0.0", default-features = false, path = "../../primitives/std" }

# third party
log = { version = "0.4.17", default-features = false }

[dev-dependencies]
pallet-balances = { version = "4.0.0-dev", path = "../balances" }
sp-core = { version = "6.0.0", path = "../../primitives/core" }
Expand Down
25 changes: 22 additions & 3 deletions frame/multisig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#![cfg_attr(not(feature = "std"), no_std)]

mod benchmarking;
pub mod migrations;
mod tests;
pub mod weights;

Expand All @@ -58,7 +59,7 @@ use frame_support::{
},
ensure,
traits::{Currency, Get, ReservableCurrency},
weights::{GetDispatchInfo, Weight},
weights::Weight,
RuntimeDebug,
};
use frame_system::{self as system, RawOrigin};
Expand All @@ -73,6 +74,20 @@ pub use weights::WeightInfo;

pub use pallet::*;

/// The log target of this pallet.
pub const LOG_TARGET: &'static str = "runtime::multisig";

// syntactic sugar for logging.
#[macro_export]
macro_rules! log {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in here if you only use it in the migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I'd rather define logging macro for the whole package and have the opportunity to use it when/if needed. It's also consistent with other pallets which define logging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ruseinov's point is fair to me, I had to do the same in the past.

@ggwpez what I challenge you about is a way so we won't need to define this log macro per-pallet? wdyt?

($level:tt, $patter:expr $(, $values:expr)* $(,)?) => {
log::$level!(
target: crate::LOG_TARGET,
concat!("[{:?}] ✍️ ", $patter), <frame_system::Pallet<T>>::block_number() $(, $values)*
)
};
}

type BalanceOf<T> =
<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;

Expand Down Expand Up @@ -103,7 +118,7 @@ pub struct Multisig<BlockNumber, Balance, AccountId> {
type CallHash = [u8; 32];

enum CallOrHash<T: Config> {
Call(<T as Config>::Call),
Call(<T as Config>::RuntimeCall),
Hash([u8; 32]),
}

Expand Down Expand Up @@ -150,9 +165,13 @@ pub mod pallet {
type WeightInfo: WeightInfo;
}

/// The current storage version.
const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);

#[pallet::pallet]
#[pallet::generate_store(pub(super) trait Store)]
#[pallet::without_storage_info]
#[pallet::storage_version(STORAGE_VERSION)]
pub struct Pallet<T>(_);

/// The set of open multisig operations.
Expand Down Expand Up @@ -356,7 +375,7 @@ pub mod pallet {
threshold: u16,
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<T::BlockNumber>>,
call: Box<<T as Config>::Call>,
call: Box<<T as Config>::RuntimeCall>,
max_weight: Weight,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;
Expand Down
69 changes: 69 additions & 0 deletions frame/multisig/src/migrations.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
use super::*;
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
use frame_support::{
dispatch::GetStorageVersion,
traits::{OnRuntimeUpgrade, WrapperKeepOpaque},
Identity,
};

#[cfg(feature = "try-runtime")]
use frame_support::ensure;

pub mod v1 {
use super::*;

type OpaqueCall<T> = WrapperKeepOpaque<<T as Config>::RuntimeCall>;

#[frame_support::storage_alias]
type Calls<T: Config> = StorageMap<
Pallet<T>,
Identity,
[u8; 32],
(OpaqueCall<T>, <T as frame_system::Config>::AccountId, BalanceOf<T>),
>;

pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
impl<T: Config> OnRuntimeUpgrade for MigrateToV1<T> {
#[cfg(feature = "try-runtime")]
fn pre_upgrade() -> Result<(), &'static str> {
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
let onchain = Pallet::<T>::on_chain_storage_version();

ensure!(onchain < 1, "this migration can be deleted");

log!(info, "Number of calls to refund and delete: {}", Calls::<T>::iter().count());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be pedantic, we could add some math here that: if the number of calls is so much that this exhausts block weight, abort or log a warning.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just have a limit configurable limit (eg 100) which can then be tested with try-runtime in advance to respect the limits.


Ok(())
}

fn on_runtime_upgrade() -> Weight {
let current = Pallet::<T>::current_storage_version();
let onchain = Pallet::<T>::on_chain_storage_version();

if onchain > 0 {
log!(info, "MigrateToV1 should be removed");
return T::DbWeight::get().reads(1)
}

// TODO: Assuming this is one write and a read per record
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do a drain().collect() to store all calls into a variable. Then you can use then length as storage reads.

Calls::<T>::drain().for_each(|(_call_hash, (_data, caller, deposit))| {
//TODO: What's the weight of one unreserve?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sadly nobody knows since a lot of traits are missing weight info.

T::Currency::unreserve(&caller, deposit);
});

current.put::<Pallet<T>>();

<T as frame_system::Config>::BlockWeights::get().max_block
}

#[cfg(feature = "try-runtime")]
fn post_upgrade() -> Result<(), &'static str> {
let onchain = Pallet::<T>::on_chain_storage_version();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add an assertion that the new version is exactly 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the assertion we have now is wrong, about to fix it.

ensure!(onchain > 0, "this migration needs to be run");
ensure!(
Calls::<T>::iter().count() == 0,
"there are some dangling calls that need to be destroyed and refunded"
);
Ok(())
}
}
}