Skip to content
This repository was 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
ruseinov merged 11 commits into from
Sep 29, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7bb5e13
[Feature] Add a migration that drains and refunds stored calls
ruseinov Sep 20, 2022
8234539
migration fixes
ruseinov Sep 20, 2022
35a4c77
fixes
ruseinov Sep 20, 2022
9c0ad6d
address review comments
ruseinov Sep 21, 2022
1e8a954
consume the whole block weight
ruseinov Sep 21, 2022
a4c139d
fix assertions
ruseinov Sep 26, 2022
fab3fc4
Merge remote-tracking branch 'origin/gav-rm-multisig-calls' into ru/f…
Sep 28, 2022
73d5643
Merge branch 'ru/feature/migrate-storage-calls' of github.com:parityt…
ruseinov Sep 28, 2022
dbb2cd2
license header
ruseinov Sep 28, 2022
5377978
fix interface
ruseinov Sep 28, 2022
e140d00
Merge remote-tracking branch 'origin/gav-rm-multisig-calls' into ru/f…
Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
migration fixes
  • Loading branch information
@ruseinov
ruseinov committed Sep 20, 2022
commit 82345391e8f3ab1378ea817b2c3481499efeedff
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

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
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion frame/multisig/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ macro_rules! log {
($level:tt, $patter:expr $(, $values:expr)* $(,)?) => {
log::$level!(
target: crate::LOG_TARGET,
concat!("[{:?}] 🏊‍♂️ ", $patter), <frame_system::Pallet<T>>::block_number() $(, $values)*
concat!("[{:?}] ✍️ ", $patter), <frame_system::Pallet<T>>::block_number() $(, $values)*
)
};
}
Expand Down
28 changes: 21 additions & 7 deletions frame/multisig/src/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use super::*;
use frame_support::{
dispatch::GetStorageVersion,
traits::{OnRuntimeUpgrade, WrapperKeepOpaque},
Blake2_256,
Identity,
};

#[cfg(feature = "try-runtime")]
Expand All @@ -14,10 +15,10 @@ pub mod v1 {

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

pub struct MigrateToV1<T>(sp_std::marker::PhantomData<T>);
Expand All @@ -35,13 +36,26 @@ pub mod v1 {
}

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

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

// TODO: Assuming this is one read
let calls_read = Calls::<T>::iter().count() as u64;
Copy link
Contributor

@kianenigma kianenigma Sep 20, 2022

Choose a reason for hiding this comment

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

This .iter().count() already reads all of the Calls from storage. They won't be re-read from the DB when you drain below, but is an avoidable overlay storage access.

I'd just use let mut call_reads = 0u32 and mutate it in the drain.


let calls = Calls::<T>::drain().for_each(|call_hash, (_data, caller, deposit)| {
T::Currency::unreserve(&caller, deposit)?;
// TODO: Assuming this is one write and a read per record
Copy link
Member

@ggwpez ggwpez Sep 25, 2022

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

@ggwpez ggwpez Sep 25, 2022

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::DbWeight::get().reads_writes(calls_read + 1, 2)
}

#[cfg(feature = "try-runtime")]
Expand Down