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

fix: collator selection v2 storage migration #1862

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

wischli
Copy link
Contributor

@wischli wischli commented Jun 6, 2024

Description

Fixes unmentioned breaking storage change in collator selection pallet by manually backporting an improved version of their fix.

Unfortunately, we bump the storage version here higher than the max version of the [email protected] such that try-runtime throws an error:

[2024-06-06T09:06:24Z ERROR runtime::frame-support] CollatorSelection: On chain storage version StorageVersion(2) doesn't match current storage version StorageVersion(1).

This will lead to unsuccessful try-runtime CI in our codebase until we upgrade to SDK v1.11.0. I still prefer this over applying the migration and staying at v1. WDYT @lemunozm @mustermeiszer?

Checklist:

  • I have added Rust doc comments to structs, enums, traits and functions
  • I have made corresponding changes to the documentation
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works

@wischli wischli self-assigned this Jun 6, 2024
@wischli wischli added the D8-migration Pull request touches storage and needs migration code. label Jun 6, 2024
@wischli wischli changed the base branch from main to polkadot-v1.7.2 June 6, 2024 09:07
@lemunozm
Copy link
Contributor

lemunozm commented Jun 6, 2024

we bump the storage version here higher than the max version of the [email protected] such that try-runtime throws an error

As a workaround, can we later force v1 again with a new migration?

@wischli
Copy link
Contributor Author

wischli commented Jun 6, 2024

we bump the storage version here higher than the max version of the [email protected] such that try-runtime throws an error

As a workaround, can we later force v1 again with a new migration?

Then, we could also just not bump the storage version with this migration.

@lemunozm
Copy link
Contributor

lemunozm commented Jun 6, 2024

Does that make sense to you? Or could it lead to future issues? I guess when the correct 1.11 migration enters, then it will migrate 0 entries and everything will work

@wischli
Copy link
Contributor Author

wischli commented Jun 6, 2024

Does that make sense to you? Or could it lead to future issues? I guess when the correct 1.11 migration enters, then it will migrate 0 entries and everything will work

Since we drain the Candidates storage here, re-applying this migrations shouldn't do any harm. So I am also fine with going the path of applying the migration in an unchecked fashion in which we don't bump the storage version. Will push after lunch!

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.68%. Comparing base (e1bad5f) to head (952f8f6).
Report is 9 commits behind head on polkadot-v1.7.2.

Additional details and impacted files
@@                 Coverage Diff                 @@
##           polkadot-v1.7.2    #1862      +/-   ##
===================================================
- Coverage            46.93%   46.68%   -0.26%     
===================================================
  Files                  166      167       +1     
  Lines                13048    13121      +73     
===================================================
+ Hits                  6124     6125       +1     
- Misses                6924     6996      +72     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lemunozm
Copy link
Contributor

lemunozm commented Jun 6, 2024

Great! And we do no longer get errors from try-runtime

Copy link
Contributor

@lemunozm lemunozm left a comment

Choose a reason for hiding this comment

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

LGTM!

@lemunozm
Copy link
Contributor

lemunozm commented Jun 6, 2024

Thanks for your good research about this topic

@lemunozm
Copy link
Contributor

lemunozm commented Jun 6, 2024

@wischli does it mean this PR is no longer needed?

@wischli
Copy link
Contributor Author

wischli commented Jun 6, 2024

@wischli does it mean this PR is no longer needed?

We still need the migration but the diff is now only 3 lines :)

@wischli wischli merged commit dde6ca4 into polkadot-v1.7.2 Jun 6, 2024
12 checks passed
@wischli wischli deleted the wf/polkadot-v1.7.2-collator-selection branch June 6, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D8-migration Pull request touches storage and needs migration code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants