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

Conversation

ruseinov
Copy link
Contributor

No description provided.

@ruseinov
Copy link
Contributor Author

git rebase

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Branch is already up-to-date

}

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

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.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks broadly good, do you have any run outputs on kusama/polkadot? based on how many ongoing multisigs we have there, how big this migration is?

@kianenigma
Copy link
Contributor

@ggwpez recently mentioned that using try-runtime on-runtime-upgrade with --sync=warp is quite viable, perhaps that would help you.

@ruseinov
Copy link
Contributor Author

Looks broadly good, do you have any run outputs on kusama/polkadot? based on how many ongoing multisigs we have there, how big this migration is?

Not yet, will check with try-runtime precheck I guess.

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.


// TODO: Assuming this is one write and a read per record
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.

#[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.

frame/multisig/src/migrations.rs Outdated Show resolved Hide resolved

// 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?

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Branch is already up-to-date

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@ruseinov
Copy link
Contributor Author

This needs to be tested with try-runtime to see if we need any sort of batching, but it seems like the latest substrate master is not compatible with polkadot, so I'm going to retry this in the coming days.

@ruseinov
Copy link
Contributor Author

bot rebase

@paritytech-processbot
Copy link

Rebased

@ruseinov
Copy link
Contributor Author

@kianenigma @ggwpez
tried that on a couple weeks old snap on pdot, seems like this migration with MaxWeight definitely works and even is an overkill:

2022-09-29 12:23:54.050  INFO main runtime::multisig: [11802239] ✍️ Number of calls to refund and delete: 4
2022-09-29 12:23:54.051 DEBUG main try-runtime::cli: proof size: 17.94 KB (18368 bytes)
2022-09-29 12:23:54.051 DEBUG main try-runtime::cli: compact proof size: 16.91 KB (17314 bytes)
2022-09-29 12:23:54.051 DEBUG main try-runtime::cli: zstd-compressed compact proof 15.69 KB (16069 bytes)
2022-09-29 12:23:54.575  INFO main try-runtime::cli: TryRuntime_on_runtime_upgrade executed without errors. Consumed weight = 512021368576011, total weight = 512000000000011 (1.0000417355)

@@ -0,0 +1,86 @@
// This file is part of Substrate.

// Copyright (C) 2019-2022 Parity Technologies (UK) Ltd.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright (C) 2019-2022 Parity Technologies (UK) Ltd.
// Copyright (C) 2022 Parity Technologies (UK) Ltd.


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.

Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

LGTM.

@ruseinov
Copy link
Contributor Author

Merging this, please continue reviewing this in the parent PR: #12072

@ruseinov ruseinov merged commit e173fa7 into gav-rm-multisig-calls Sep 29, 2022
@ruseinov ruseinov deleted the ru/feature/migrate-storage-calls branch September 29, 2022 14:48
@shawntabrizi
Copy link
Member

shawntabrizi commented Sep 29, 2022

im not a fan that this PR doesnt have a clear description, reasoning, follow up actions, and stuff...

do you assume everyone affected by this will just read code? or that code represents why things are happening?

gavofyork added a commit that referenced this pull request Sep 30, 2022
* Remove multisig's Calls

* Multisig: Fix tests and re-introduce reserve logic (#12241)

* Fix tests and re-introduce reserve logic

* fix benches

* add todo

* remove irrelevant bench

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

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

* migration fixes

* fixes

* address review comments

* consume the whole block weight

* fix assertions

* license header

* fix interface

Co-authored-by: parity-processbot <>

Co-authored-by: parity-processbot <>
Co-authored-by: Roman Useinov <[email protected]>
gavofyork added a commit that referenced this pull request Oct 5, 2022
* Introduce preimages module in traits

* Multisize Preimages

* Len not actually necessary

* Tweaks to the preimage API

* Fixes

* Get Scheduler building with new API

* Scheduler tests pass

* Bounded Scheduler 🎉

* Use Agenda holes and introduce IncompleteSince to avoid need to reschedule

* Tests pass with new weight system

* New benchmarks

* Add missing file

* Drop preimage when permenantly overeight

* Drop preimage when permenantly overeight

* Referenda uses latest preimage API

* Testing ok

* Adding tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add preimage migration

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Docs

* Remove dbg

* Refactor Democracy

* Refactor Democracy

* Add final MEL

* Remove silly maps

* Fixes

* Minor refactor

* Formatting

* Fixes

* Fixes

* Fixes

* Update frame/preimage/src/lib.rs

Co-authored-by: Shawn Tabrizi <[email protected]>

* Add migrations to Democracy

* WIP

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Resolve conflicts

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Resolve conflicts"

This reverts commit a89cd0a.

* Undo wrong resolves...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* WIP

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Make compile

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* massage clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* clippy annoyance

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* clippy annoyance

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix benchmarks

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* add missing file

* Test <Preimage as QueryPreimage>

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Clippy harassment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fixup tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Remove old stuff

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test <Scheduler as Anon> trait functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Update pallet-ui tests

Why is this needed? Should not be the case unless master is broken...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More scheduler trait test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Apply review suggestion

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add Scheduler test migration_v3_to_v4_works

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Merge fixup

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Keep referenda benchmarks instantiatable

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Update weights

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new scheduler weight functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new democracy weight functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use weight compare functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Update pallet-ui tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More renaming…

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More renaming…

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add comment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Implement OnRuntimeUpgrade for scheduler::v3_to_v4 migration

Put the migration into a proper `MigrateToV4` struct and implement
the OnRuntimeUpgrade hooks for it. Also move the test to use that
instead.

This should make it easier for adding it to Polkadot.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Handle undecodable Agendas

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Remove trash

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new OnRuntimeUpgrade functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix BoundedSlice::truncate_from

Co-authored-by: jakoblell

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix pre_upgrade hook return values

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add more error logging

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Find too large preimages in the pre_upgrade hook

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test that too large Calls in agendas are ignored

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new OnRuntimeUpgrade hooks

Why did the CI not catch this?!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* works fine - just more logs

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix staking migration

Causing issues on Kusama...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix UI tests

No idea why this is needed. This is actually undoing an earlier change.
Maybe the CI has different rustc versions!?

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Remove multisig's Calls (#12072)

* Remove multisig's Calls

* Multisig: Fix tests and re-introduce reserve logic (#12241)

* Fix tests and re-introduce reserve logic

* fix benches

* add todo

* remove irrelevant bench

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

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

* migration fixes

* fixes

* address review comments

* consume the whole block weight

* fix assertions

* license header

* fix interface

Co-authored-by: parity-processbot <>

Co-authored-by: parity-processbot <>
Co-authored-by: Roman Useinov <[email protected]>

* Fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix multisig benchmarks

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* ".git/.scripts/bench-bot.sh" pallet dev pallet_democracy

* ".git/.scripts/bench-bot.sh" pallet dev pallet_scheduler

* ".git/.scripts/bench-bot.sh" pallet dev pallet_preimage

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: parity-processbot <>
Co-authored-by: Roman Useinov <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Introduce preimages module in traits

* Multisize Preimages

* Len not actually necessary

* Tweaks to the preimage API

* Fixes

* Get Scheduler building with new API

* Scheduler tests pass

* Bounded Scheduler 🎉

* Use Agenda holes and introduce IncompleteSince to avoid need to reschedule

* Tests pass with new weight system

* New benchmarks

* Add missing file

* Drop preimage when permenantly overeight

* Drop preimage when permenantly overeight

* Referenda uses latest preimage API

* Testing ok

* Adding tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add preimage migration

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Docs

* Remove dbg

* Refactor Democracy

* Refactor Democracy

* Add final MEL

* Remove silly maps

* Fixes

* Minor refactor

* Formatting

* Fixes

* Fixes

* Fixes

* Update frame/preimage/src/lib.rs

Co-authored-by: Shawn Tabrizi <[email protected]>

* Add migrations to Democracy

* WIP

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Resolve conflicts

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Revert "Resolve conflicts"

This reverts commit a89cd0a.

* Undo wrong resolves...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* WIP

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Make compile

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* massage clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* clippy annoyance

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* clippy annoyance

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix benchmarks

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* add missing file

* Test <Preimage as QueryPreimage>

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Clippy harassment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fixup tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Remove old stuff

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fmt

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test <Scheduler as Anon> trait functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Update pallet-ui tests

Why is this needed? Should not be the case unless master is broken...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More scheduler trait test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Apply review suggestion

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Beauty fixes

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add Scheduler test migration_v3_to_v4_works

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Merge fixup

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Keep referenda benchmarks instantiatable

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Update weights

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new scheduler weight functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new democracy weight functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use weight compare functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Update pallet-ui tests

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More renaming…

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* More renaming…

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add comment

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Implement OnRuntimeUpgrade for scheduler::v3_to_v4 migration

Put the migration into a proper `MigrateToV4` struct and implement
the OnRuntimeUpgrade hooks for it. Also move the test to use that
instead.

This should make it easier for adding it to Polkadot.

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Clippy

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Handle undecodable Agendas

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Remove trash

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new OnRuntimeUpgrade functions

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix BoundedSlice::truncate_from

Co-authored-by: jakoblell

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix pre_upgrade hook return values

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Add more error logging

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Find too large preimages in the pre_upgrade hook

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Test that too large Calls in agendas are ignored

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Use new OnRuntimeUpgrade hooks

Why did the CI not catch this?!

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* works fine - just more logs

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix staking migration

Causing issues on Kusama...

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix UI tests

No idea why this is needed. This is actually undoing an earlier change.
Maybe the CI has different rustc versions!?

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Remove multisig's Calls (paritytech#12072)

* Remove multisig's Calls

* Multisig: Fix tests and re-introduce reserve logic (paritytech#12241)

* Fix tests and re-introduce reserve logic

* fix benches

* add todo

* remove irrelevant bench

* [Feature] Add a migration that drains and refunds stored calls (paritytech#12313)

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

* migration fixes

* fixes

* address review comments

* consume the whole block weight

* fix assertions

* license header

* fix interface

Co-authored-by: parity-processbot <>

Co-authored-by: parity-processbot <>
Co-authored-by: Roman Useinov <[email protected]>

* Fix test

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* Fix multisig benchmarks

Signed-off-by: Oliver Tale-Yazdi <[email protected]>

* ".git/.scripts/bench-bot.sh" pallet dev pallet_democracy

* ".git/.scripts/bench-bot.sh" pallet dev pallet_scheduler

* ".git/.scripts/bench-bot.sh" pallet dev pallet_preimage

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: parity-processbot <>
Co-authored-by: Roman Useinov <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants