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

[Coretime chain] Add high assignment count mitigation to testnets #6022

Merged
merged 10 commits into from
Oct 21, 2024

Conversation

seadanda
Copy link
Contributor

Fixed in Polkadot and Kusama in polkadot-fellows/runtimes#434 and this PR just adds to testnets.

We can handle a maximum of 28 assignments inside one XCM, while it's possible to have 80 (if a region is interlaced 79 times).
This can be chunked on the coretime chain side but currently the scheduler on the relay makes assumptions that means we can't send more than one chunk for a given core.

This just truncates the additional assignments until we can extend the relay to support this. This means that the first 27 assignments are taken, the final 28th is used to pad with idle to complete the mask (the relay also assumes that every schedule is complete). Any other assignments are dropped.

@seadanda seadanda added the T14-system_parachains This PR/Issue is related to system parachains. label Oct 11, 2024
@seadanda
Copy link
Contributor Author

seadanda commented Oct 11, 2024

/cmd prdoc --pr 6022 --bump patch --audience "Runtime User"
edit: this should have been --audience "runtime_user"

@seadanda seadanda marked this pull request as ready for review October 16, 2024 12:34
@seadanda seadanda requested review from bkchr and eskimor October 16, 2024 12:35
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.

Is the underlying problem tracked in an issue?

@seadanda
Copy link
Contributor Author

Just made it - that was too far down on my TODO list but sorted now

);

// Set the parts of the `Idle` assignment we injected at the start of the vec above.
assignment_truncated[0].1 = 57_600u16.saturating_sub(total_parts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assignment_truncated[0].1 = 57_600u16.saturating_sub(total_parts);
assignment_truncated[0].1 = PartsOf57600::FULL.saturating_sub(total_parts);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realised that this is just a u16 in the broker pallet, rather than the type that the relay chain uses. This means we don't have FULL etc here. It will introduce another dependency and require an into anyway. I think longer term we should change the type in the broker pallet, but this will be a breaking change

Copy link
Member

Choose a reason for hiding this comment

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

I think longer term we should change the type in the broker pallet, but this will be a breaking change

In the relay chain it is also a u16, but just a transparent wrapper around u16. While this is a "breaking change" on the code level, it is not changing the storage layout or anything. Thus, changing it can be done very easily.

seadanda and others added 4 commits October 18, 2024 16:07
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
This reverts commits 38e8342 and c05ed3e.
@bkchr bkchr added this pull request to the merge queue Oct 21, 2024
Merged via the queue into master with commit a3bca4b Oct 21, 2024
191 of 194 checks passed
@bkchr bkchr deleted the donal-coretime-assignments-mitigation branch October 21, 2024 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T14-system_parachains This PR/Issue is related to system parachains.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants