Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump versions for sessions keys migration #263

Conversation

s0me0ne-unkn0wn
Copy link
Contributor

@s0me0ne-unkn0wn s0me0ne-unkn0wn commented Mar 29, 2024

Fixes #262

CC @bkchr

  • Does not require a CHANGELOG entry

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.

It is an interesting tie to safeguard a migration to a specific spec version. But the downside is that an emergency upgrade like 1.1.2 can disrupt the flow :)

Somewhat more ideally, a migration would be smart enough to know if it is executed or not. In some migrations, this is possible, in some it is harder but can be achieved if we attach a unique identifier to all impls of OnRuntimeUpgrade and then we will never have to worry about duplicate or stale migrations again.

Copy link
Contributor

@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.

Ty!

@bkchr
Copy link
Contributor

bkchr commented Mar 29, 2024

@kianenigma paritytech/polkadot-sdk#2421

@s0me0ne-unkn0wn
Copy link
Contributor Author

@kianenigma yes, exactly. The root cause of the problem is that some runtime structures, as SessionKeys, are not versioned. We should either make them versioned or implement something like what you're talking about (we have paritytech/polkadot-sdk#2421 for that), but until then we have no options but use this hacky and brittle way 🤷

@bkchr bkchr enabled auto-merge (squash) March 29, 2024 16:46
@bkchr bkchr disabled auto-merge March 29, 2024 16:46
@bkchr
Copy link
Contributor

bkchr commented Mar 29, 2024

/merge

@bkchr bkchr enabled auto-merge (squash) March 29, 2024 17:43
@bkchr bkchr merged commit 7a52317 into polkadot-fellows:main Mar 29, 2024
37 of 38 checks passed
@s0me0ne-unkn0wn s0me0ne-unkn0wn deleted the bump-versions-session-key-migration branch March 29, 2024 19:07
@@ -1860,7 +1860,7 @@ pub mod migrations {
/// Upgrade Session keys to exclude `ImOnline` key.
/// When this is removed, should also remove `OldSessionKeys`.
pub struct UpgradeSessionKeys;
const UPGRADE_SESSION_KEYS_FROM_SPEC: u32 = 1001002;
const UPGRADE_SESSION_KEYS_FROM_SPEC: u32 = 1001003;
Copy link
Member

@ggwpez ggwpez Mar 29, 2024

Choose a reason for hiding this comment

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

It would be safer to create a storage key like :upgrade_session_keys_migration_executed and then manually cleaning it up via governance afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

im-online session keys migration will not run on Kusama
5 participants