Implements pallet versioning by bkchr · Pull Request #7208 · paritytech/substrate

bkchr · 2020-09-24T19:27:35Z

This pull requests adds a new struct PalletVersion. This struct holds the members major, minor and patch to reflect the version of a crate.

The pallet version will be automatically bound to each pallet, this means on each time the runtime is upgraded and on_runtime_upgrade is called, the latest pallet version is stored in the state. This can be used on a later runtime upgrade to check which kinds of upgrades need to be applied. There is only one point in time where the actual version of a pallet and the version of a pallet in the storage is different and that is after a runtime upgrade and before on_runtime_upgrade of the pallet was called to update the version.

There is also a new trait GetPalletVersion provided. This trait will be automatically implemented by each pallet. It can be used from the runtime to get the versions of each pallet.

Besides that there is also a new macro crate_to_pallet_version!. This converts the version of the current crate into a PalletVersion object.

After this pr we should make sure that we update the crate versions accordingly to changes, to make sure we can write upgrades accordingly.

@jacogr maybe this is also something that can be supported from the UI to read the versions per pallet.

addresses #6888

Besides the renaming it also adds support getting the name of a pallet as configured in the runtime.

…e-setup

… bkchr-pallet-version

gui1117 · 2020-09-25T09:31:31Z

frame/support/src/traits.rs

+/// The storage key postfix that is used to store the [`PalletVersion`] per pallet.
+///
+/// The full storage key is build by using:
+/// Twox128([`PalletInfo::name`] ++ [`PALLET_VERSION_STORAGE_POSTFIX`])


why not Twox128([PalletInfo::name]) ++ Twox128([PALLET_VERSION_STORAGE_POSTFIX]) in order to keep the fact that all usage of a pallet is after some prefix.

Thus once we make pallet storages using PalletInfo::name then we have all pallet storage usage after a unique prefix.
This would ease removal and introspection IMHO

Yeah that sounds good as well;)

frame/support/test/tests/pallet_version.rs

gui1117 · 2020-09-25T12:36:37Z

frame/support/src/dispatch.rs

 			fn on_runtime_upgrade() -> $return {
 				$crate::sp_tracing::enter_span!($crate::sp_tracing::trace_span!("on_runtime_upgrade"));
-				{ $( $impl )* }
+				let result = { $( $impl )* };


if user write return Ok(..); inside the impl block, I think they would expect to have storage version written.
Maybe it is better to wrap it into a closure or something, no ?

frame/support/src/traits.rs

apopiak · 2020-09-26T10:10:33Z

I just want to note the edgecase that I recently saw in the UTXO runtime where the crate of the pallet was the same as the crate of the runtime.
Should still work, right?
And: do we want to support pallets that are not part of a (cargo) crate?

apopiak

apply typo suggestions

bkchr · 2020-10-20T09:29:44Z

Sorry for the long quite period.

I'm still in favor of this pallet versioning scheme:

It solves the problem of having a storage versioning scheme. We can just as discussed widely in this pr use the pallet version to check for required migrations etc. While this could be solved with a simple storage version as proposed by @thiolliere, I don't see any reason why not use the pallet version as well.
I see pallet versioning more useful in the future, when we really have multiple versions of a crate being deployed on different chains. Currenty the UI needs to do some magic to find out which version a crate is and what it can do or if there are certain bugs that it needs to circumvent. By using pallet versioning, we can make this much easier. External software can read the version of a pallet and act accordingly.
While pallets are a DSL that is written in Rust and it is sort of special, I don't see a reason why we should invent different mechanism for all the stuff. Why not use the crate version as input for the pallet version? Why should we come up with something different, that in the end wants to solve the same problem. Yeah, we will need some thinking around semver and how to integrate it in pallets, but why not do this and teach our users on how it should be done, like Rust is doing it.
We can still bring in a model where the user manually specifies the version, instead of automatically using the crate version. This could be helpful for crates which host different pallets or just for people who don't like using the crate version as pallet version. However this doesn't mean that we will need to change the structure of the pallet version struct and thus, we will stay compatible.

gui1117

Looks good to me

I agree we can use crate version, later if user request to be able to set it inside the pallet then we can introduce it without breaking.

frame/support/src/dispatch.rs

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

apopiak

LGTM

frame/support/test/tests/pallet_version.rs

kianenigma · 2020-10-22T13:26:24Z

Would be interesting to see this in action in a PR that does some migration :) Do we already happen to have one?

apopiak · 2020-10-22T13:46:01Z

Well how about #7040? ;-)

apopiak · 2020-10-22T14:46:41Z

@bkchr Just realized that we only set the version on_runtime_upgrade and thus don't set the version on initial launch of a chain, correct?

apopiak · 2020-10-22T14:47:14Z

Would be interesting to see this in action in a PR that does some migration :) Do we already happen to have one?

I think I'm also going to create a dummy migration PR to test this stuff :-)

bkchr · 2020-10-22T14:58:02Z

@bkchr Just realized that we only set the version on_runtime_upgrade and thus don't set the version on initial launch of a chain, correct?

Ohh yeah 🙈 I did not thought about this. Give me some time to find a good solution.

apopiak · 2020-10-26T15:25:50Z

frame/support/src/traits.rs

+///
+/// Each pallet version is stored in the state under a fixed key. See
+/// [`PALLET_VERSION_STORAGE_KEY_POSTFIX`] for how this key is built.
+#[derive(RuntimeDebug, Eq, PartialEq, Encode, Decode, Ord)]


Sorry for the late comments/nitpicks, but I wonder whether this should be Copy or at least Clone.

bkchr added 14 commits September 15, 2020 17:20

Start

4c1adf6

Merge remote-tracking branch 'origin/master' into bkchr-pallet-version

9967a48

Make macro work

f1beba9

Rename ModuleToIndex to PalletRuntimeSetup

bdc1251

Besides the renaming it also adds support getting the name of a pallet as configured in the runtime.

Merge remote-tracking branch 'origin/master' into bkchr-pallet-runtim…

83c871b

…e-setup

Rename it to PalletInfo

7dcbcf1

Merge remote-tracking branch 'origin/master' into bkchr-pallet-runtim…

9a816fa

…e-setup

Remove accidentally added files

c6e7b8a

Some work

fe7a5f9

Merge remote-tracking branch 'origin/bkchr-pallet-runtime-setup' into…

eb8e837

… bkchr-pallet-version

Make everything compile

5c0343d

Merge remote-tracking branch 'origin/master' into bkchr-pallet-version

2159825

Adds a test and fixes some bugs

3e18d34

Implement ordering for PalletVersion

8b5adb2

bkchr added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Sep 24, 2020

bkchr requested review from apopiak, gui1117, kianenigma and shawntabrizi September 24, 2020 19:27

jacogr mentioned this pull request Sep 24, 2020

Support palette versioning struct polkadot-js/api#2649

Closed

bkchr mentioned this pull request Sep 25, 2020

Better migration API paritytech/polkadot-sdk#343

Open

gui1117 reviewed Sep 25, 2020

View reviewed changes

apopiak reviewed Sep 26, 2020

View reviewed changes

frame/support/src/traits.rs Outdated Show resolved Hide resolved

frame/support/src/traits.rs Outdated Show resolved Hide resolved

frame/support/src/traits.rs Outdated Show resolved Hide resolved

frame/support/src/traits.rs Outdated Show resolved Hide resolved

apopiak reviewed Sep 28, 2020

View reviewed changes

Apply suggestions from code review

b8182b8

github-actions bot added the A7-needspolkadotpr label Sep 28, 2020

Review feedback

4db05e6

paritytech-ci added the D2-breaksapi label Oct 20, 2020

github-actions bot removed the A7-needspolkadotpr label Oct 20, 2020

gui1117 approved these changes Oct 20, 2020

View reviewed changes

frame/support/src/dispatch.rs Outdated Show resolved Hide resolved

frame/support/src/dispatch.rs Outdated Show resolved Hide resolved

bkchr and others added 2 commits October 20, 2020 15:49

Update frame/support/src/dispatch.rs

71ec68c

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

Update frame/support/src/dispatch.rs

2d80e80

Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

apopiak approved these changes Oct 20, 2020

View reviewed changes

bkchr added 3 commits October 20, 2020 21:57

Fix compilation

20155c2

Fix test

caf173e

Fix doc test

5e5920e

shawntabrizi approved these changes Oct 21, 2020

View reviewed changes

apopiak reviewed Oct 21, 2020

View reviewed changes

frame/support/test/tests/pallet_version.rs Show resolved Hide resolved

bkchr merged commit 529e5d9 into master Oct 21, 2020

bkchr deleted the bkchr-pallet-version branch October 21, 2020 17:05

apopiak added the D1-runtime-migration label Oct 22, 2020

apopiak mentioned this pull request Oct 22, 2020

Unified Pallet Storage Version #6888

Closed

apopiak mentioned this pull request Oct 26, 2020

Fix elections-phragmen and proxy issue #7040

Merged

apopiak reviewed Oct 26, 2020

View reviewed changes

apopiak mentioned this pull request Nov 3, 2020

[WIP] Runtime Migration Tutorial polkadot-developers/substrate-developer-hub.github.io#767

Closed

10 tasks

raoulmillais mentioned this pull request Dec 9, 2020

Multiple vesting schedules per account #7699

Closed

32 tasks

mrq1911 mentioned this pull request Mar 7, 2021

fix: claims migration based on pallet version galacticcouncil/hydration-node#132

Merged

kianenigma mentioned this pull request Mar 15, 2021

Display pallet versions. polkadot-js/api#3323

Closed

ETeissonniere mentioned this pull request May 21, 2021

Marko/add capsule migration capsule-corp-ternoa/ternoa-node#53

Merged

c410-f3r mentioned this pull request May 28, 2021

Add PoolStatus parameter zeitgeistpm/zeitgeist#127

Merged

Conversation

bkchr commented Sep 24, 2020 • edited by apopiak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gui1117 Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gui1117 Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

bkchr Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apopiak commented Sep 26, 2020

Uh oh!

apopiak left a comment

Choose a reason for hiding this comment

Uh oh!

bkchr commented Oct 20, 2020

Uh oh!

gui1117 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

apopiak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kianenigma commented Oct 22, 2020

Uh oh!

apopiak commented Oct 22, 2020

Uh oh!

apopiak commented Oct 22, 2020

Uh oh!

apopiak commented Oct 22, 2020

Uh oh!

bkchr commented Oct 22, 2020

Uh oh!

apopiak Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bkchr commented Sep 24, 2020 •

edited by apopiak

Loading