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

pallet-migrations: fix index access for singluar migrations #5695

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Sep 12, 2024

Discovered a bug in the migrations pallet while debugging paritytech/try-runtime-cli#90.
🚨 This can lead to your chain getting stuck. It happens when a single MBM is configured. Having no or more than one MBM will circumvent the issue.

Changes:

  • Check len of the tuple before accessing its nth_id
  • Make nth_id return None on unary tuples and n>0

@ggwpez ggwpez requested a review from a team as a code owner September 12, 2024 16:10
@@ -777,8 +777,10 @@ impl<T: SteppedMigration> SteppedMigrations for T {
1
}

fn nth_id(_n: u32) -> Option<Vec<u8>> {
Some(T::id().encode())
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the actual issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and this was not overwritten for a single migration that's not part of a tuple, right?

@ggwpez
Copy link
Member Author

ggwpez commented Sep 12, 2024

/cmd prdoc --pr 5695 --bump patch --audience "Runtime Dev"

@ggwpez ggwpez added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Sep 12, 2024
Signed-off-by: Oliver Tale-Yazdi <[email protected]>
@paritytech paritytech deleted a comment from github-actions bot Sep 12, 2024
@paritytech paritytech deleted a comment from github-actions bot Sep 12, 2024
@@ -777,8 +777,10 @@ impl<T: SteppedMigration> SteppedMigrations for T {
1
}

fn nth_id(_n: u32) -> Option<Vec<u8>> {
Some(T::id().encode())
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and this was not overwritten for a single migration that's not part of a tuple, right?

@ggwpez ggwpez enabled auto-merge September 13, 2024 10:54
@ggwpez ggwpez added this pull request to the merge queue Sep 13, 2024
Merged via the queue into master with commit 0136463 Sep 13, 2024
202 of 205 checks passed
@ggwpez ggwpez deleted the oty-pallet-migrations-fix branch September 13, 2024 12:17
@ggwpez ggwpez added the A4-needs-backport Pull request must be backported to all maintained releases. label Sep 13, 2024
github-actions bot pushed a commit that referenced this pull request Sep 13, 2024
Discovered a bug in the migrations pallet while debugging
paritytech/try-runtime-cli#90.
It only occurs when a single MBM is configured - hence it did not happen
when Ajuna Network tried it...

Changes:
- Check len of the tuple before accessing its nth_id
- Make nth_id return `None` on unary tuples and n>0

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: ggwpez <[email protected]>
(cherry picked from commit 0136463)
@ggwpez ggwpez added A4-needs-backport Pull request must be backported to all maintained releases. and removed A4-needs-backport Pull request must be backported to all maintained releases. labels Sep 13, 2024
github-actions bot pushed a commit that referenced this pull request Sep 13, 2024
Discovered a bug in the migrations pallet while debugging
paritytech/try-runtime-cli#90.
It only occurs when a single MBM is configured - hence it did not happen
when Ajuna Network tried it...

Changes:
- Check len of the tuple before accessing its nth_id
- Make nth_id return `None` on unary tuples and n>0

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: ggwpez <[email protected]>
(cherry picked from commit 0136463)
@ggwpez ggwpez added A4-needs-backport Pull request must be backported to all maintained releases. and removed A4-needs-backport Pull request must be backported to all maintained releases. labels Sep 16, 2024
github-actions bot pushed a commit that referenced this pull request Sep 16, 2024
Discovered a bug in the migrations pallet while debugging
paritytech/try-runtime-cli#90.
It only occurs when a single MBM is configured - hence it did not happen
when Ajuna Network tried it...

Changes:
- Check len of the tuple before accessing its nth_id
- Make nth_id return `None` on unary tuples and n>0

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: ggwpez <[email protected]>
(cherry picked from commit 0136463)
@paritytech-cmd-bot-polkadot-sdk

Git push to origin failed for stable2407 with exitcode 1

github-actions bot pushed a commit that referenced this pull request Sep 16, 2024
Discovered a bug in the migrations pallet while debugging
paritytech/try-runtime-cli#90.
It only occurs when a single MBM is configured - hence it did not happen
when Ajuna Network tried it...

Changes:
- Check len of the tuple before accessing its nth_id
- Make nth_id return `None` on unary tuples and n>0

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: ggwpez <[email protected]>
(cherry picked from commit 0136463)
@paritytech-cmd-bot-polkadot-sdk

Successfully created backport PR for stable2409:

@ggwpez ggwpez added A4-needs-backport Pull request must be backported to all maintained releases. and removed A4-needs-backport Pull request must be backported to all maintained releases. labels Sep 16, 2024
github-actions bot pushed a commit that referenced this pull request Sep 16, 2024
Discovered a bug in the migrations pallet while debugging
paritytech/try-runtime-cli#90.
It only occurs when a single MBM is configured - hence it did not happen
when Ajuna Network tried it...

Changes:
- Check len of the tuple before accessing its nth_id
- Make nth_id return `None` on unary tuples and n>0

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: ggwpez <[email protected]>
(cherry picked from commit 0136463)
@paritytech-cmd-bot-polkadot-sdk

Successfully created backport PR for stable2407:

@paritytech-cmd-bot-polkadot-sdk

Git push to origin failed for stable2409 with exitcode 1

ggwpez added a commit that referenced this pull request Sep 16, 2024
Backport #5695 into `stable2409` from ggwpez.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
ggwpez added a commit that referenced this pull request Sep 17, 2024
Backport #5695 into `stable2407` from ggwpez.

See the
[documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md)
on how to use this bot.

<!--
  # To be used by other automation, do not modify:
  original-pr-number: #${pull_number}
-->

Co-authored-by: Oliver Tale-Yazdi <[email protected]>
@ggwpez ggwpez removed the A4-needs-backport Pull request must be backported to all maintained releases. label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants