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

Better identifier and logging for runtime upgrades #8123

Merged
42 commits merged into from
Feb 26, 2021
Merged

Better identifier and logging for runtime upgrades #8123

42 commits merged into from
Feb 26, 2021

Conversation

@kianenigma
Copy link
Contributor

@kianenigma kianenigma commented Feb 15, 2021 •
edited
Loading

This builds on top of #8038 and assigns a unique ID to each OnRuntimeUpgrade, which is used for two purposes:

  1. Generate a storage key that can be used in pre-upgrade/post-upgrade. I don't have an example of this yet in substrate, see an example here: paritytech/polkadot@146f1e2#diff-db5840c3c03c740de25aed56b9257559c224a04619c079c94cddbb23f9a855a4R1007

The use case is that we want to write some data in pre-migration and read it back in post migration. This storage key facilitates that.

  1. Log some useful information about the migration. Running a migration with the bot will now output:
2021-02-18 17:25:08.848   INFO main remote-ext: scraping keypairs from cache "./Kusama,0xdd772d86d5cfd2be9a11dd504866c7878ee071aefe02ff7db3d749f98bf890b8,.bin"
2021-02-18 17:25:09.356   INFO main remote-ext: extending externalities with 1 manually injected keys
2021-02-18 17:25:09.357   INFO main remote-ext: injecting a total of 327269 keys
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Lottery' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Mmr' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Tips' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Bounties' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Multisig' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Proxy' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Scheduler' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Vesting' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Recovery' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Society' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Identity' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'RandomnessCollectiveFlip' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Historical' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Offences' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'AuthorityDiscovery' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'ImOnline' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Sudo' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Contracts' and setting new storage version to PalletVersion { major: 2, minor: 0, patch: 1 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Treasury' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Grandpa' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'TechnicalMembership' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Elections' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'TechnicalCommittee' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Council' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Democracy' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:17.850   INFO main runtime::frame-support: ✅ no migration for 'Session' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main runtime::frame-support: ⚠️ running migration for Staking and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main runtime::frame-support: ✅ no migration for 'TransactionPayment' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main runtime::frame-support: ✅ no migration for 'Indices' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main runtime::frame-support: ✅ no migration for 'Authorship' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main runtime::frame-support: ✅ no migration for 'Babe' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main runtime::frame-support: ✅ no migration for 'Utility' and setting new storage version to PalletVersion { major: 3, minor: 0, patch: 0 }
2021-02-18 17:25:18.497   INFO main try_runtime_cli: try-runtime executed without errors. Consumed weight = 1024921600000000, total weight = 512000000000000 (2.0018)

I've made the assumption again that each crate has one pallet only.

@kianenigma kianenigma added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Feb 15, 2021
@kianenigma kianenigma requested review from bkchr and gui1117 February 15, 2021 11:28
kianenigma
kianenigma commented Feb 15, 2021
View reviewed changes
kianenigma
kianenigma commented Feb 15, 2021
View reviewed changes
@gui1117
Copy link
Contributor

gui1117 commented Feb 18, 2021

I don't think you need an additional identifier here.
The name given by PalletInfo is already enough in this context I think, it is unique for the pallet and identify the pallet.
Then if you want to have multiple unique keys inside the pallet you can concatenate unique keys like:

<T as frame_system::PalletInfo as frame_support::traits::PalletInfo>::name::<Pallet<T>>() ++ some unique key for migration purpose

@kianenigma kianenigma mentioned this pull request Feb 18, 2021
Merged
@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 18, 2021

Will remove the Identifier, I also noticed that it is optional at least as far as this PR goes.

@kianenigma kianenigma removed A0-please_review Pull request needs code review. A7-needspolkadotpr labels Feb 18, 2021
@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 23, 2021

Now the per-pallet OnRutnimeUpgrades can log their name and what they are doing, namely by the virtue of macros and that we add the log for them. For the custom ones, we can't do the same as far as I can see. Ideally, we should be able to achieve the same for the custom OnRuntimeUpgrades passed to executive as well. Not sure how. Even the const ID that I added before to OnRuntimeUpgrade won't help.

@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 23, 2021

I've made the assumption again that each crate has one pallet only.

What you mean by this?

This is no longer relevant. In the past I made a new proc macro the get the create name from the carg file and use that for logging, now I use PalletInfo.

@kianenigma kianenigma requested a review from bkchr February 23, 2021 07:25
bkchr
bkchr reviewed Feb 23, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

Some last questions :D

@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 23, 2021

@bkchr I hope you are happy now :D d6e7b8b

gui1117
gui1117 reviewed Feb 24, 2021
View reviewed changes
gui1117
gui1117 reviewed Feb 24, 2021
View reviewed changes
bin/node/runtime/Cargo.toml
"pallet-society/std",
"pallet-recovery/std",
"pallet-vesting/std",
"frame-try-runtime/std",
Copy link
Contributor

@gui1117 gui1117 Feb 24, 2021

Choose a reason for hiding this comment

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

yes this is needed because otherwise we can't compile node-runtime with try-runtime features and std :-/
the unused dependency on std should harm to much, and this is what we do for frame-benchmarking.

bkchr
bkchr approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I'm still not happy with bincode, but fine ;D

@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 25, 2021

@bkchr I hope you are happy now :D d6e7b8b

likewise: 4e154f0

@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 25, 2021

bot merge

@ghost
Copy link

ghost commented Feb 25, 2021

Waiting for commit status.

@ghost
Copy link

ghost commented Feb 25, 2021

Checks failed; merge aborted.

@kianenigma
Copy link
Contributor Author

kianenigma commented Feb 26, 2021

bot merge

@ghost
Copy link

ghost commented Feb 26, 2021

Trying merge.

@ghost ghost merged commit 28107d4 into master Feb 26, 2021
@ghost ghost deleted the kiz-migration-testin-2 branch February 26, 2021 15:41
jam10o-new pushed a commit to jam10o-new/substrate that referenced this pull request Feb 28, 2021
@kianenigma @apopiak
@bkchr @shawntabrizi @jam10o-new
Better identifier and logging for runtime upgrades (paritytech#8123)
4fedbf3 
* A clean new attempt

* Checkpoint to move remote.

* A lot of dependency wiring to make it feature gated.

* bad macro, bad macro.

* Undo the DB mess.

* Update frame/support/src/traits.rs

Co-authored-by: Alexander Popiak <[email protected]>

* Apply suggestions from code review

Co-authored-by: Alexander Popiak <[email protected]>

* unbreak the build

* Better logging and ids for migrations

* Fix doc.

* Test

* Update frame/try-runtime/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Update utils/frame/try-runtime/cli/Cargo.toml

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

* Update frame/try-runtime/Cargo.toml

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

* Address most review grumbles.

* Fix build

* Add some comments

* Remove allowing one pallet at a time.

* Rework the PR

* nit

* Slightly better error handling.

* Remove files

* Update utils/frame/remote-externalities/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Update frame/support/src/dispatch.rs

* Update frame/support/src/dispatch.rs

* Fix test

* Make extension trait.

* Bring back try-runtime/std

* remove bincode

* Remove warning

* Change test features

Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
@hackfisher hackfisher mentioned this pull request Mar 15, 2021
Closed
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@bkchr bkchr bkchr approved these changes

@gui1117 gui1117 gui1117 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kianenigma @gui1117 @bkchr @paritytech-ci