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

Elastic scaling: runtime v2 descriptor support #5423

Open
wants to merge 109 commits into
base: master
Choose a base branch
from

Conversation

sandreim
Copy link
Contributor

@sandreim sandreim commented Aug 20, 2024

Closes #5045 and #5046

On top of #5362

TODO:

  • storage migration for allowed relay parents tracker
  • check session index
  • PRdoc
  • tests
  • ensure UMP queue cannot be abused with this change
  • Zombienet runtime upgrade test

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7276883

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim sandreim added the T8-polkadot This PR/Issue is related to/affects the Polkadot network. label Sep 6, 2024
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Looking good! Have you tested the runtime migration with an upgrade in zombienet?
I've always discovered bugs by doing that in the past 😆

polkadot/primitives/src/vstaging/mod.rs Show resolved Hide resolved
polkadot/primitives/src/vstaging/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/inclusion/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/paras_inherent/mod.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/shared.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/shared/migration.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/shared/migration.rs Outdated Show resolved Hide resolved
polkadot/runtime/parachains/src/shared/migration.rs Outdated Show resolved Hide resolved
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Copy link
Contributor Author

Looking good! Have you tested the runtime migration with an upgrade in zombienet? I've always discovered bugs by doing that in the past 😆

Not yet, but I don't really expect any issues, it is a very simple migration.

Comment on lines +756 to +757
claim_queue: BTreeMap<CoreIndex, VecDeque<Id>>,
) -> BTreeMap<ParaId, BTreeMap<u8, BTreeSet<CoreIndex>>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some type alias should help here.

Copy link
Member

Choose a reason for hiding this comment

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

Agree :)
Also, I find it a bit strange to have a BTreeMap<u8, BTreeSet<_>>
instead of a Vec<BTreeSet<CoreIndex>> where the index in the Vec represents the depth. Given that the depth is supposed to be a number 0..x and without gaps IIUC

Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
@sandreim
Copy link
Contributor Author

Runtime upgrade successful in Zombienet using a Westend local chain.

@alindima alindima mentioned this pull request Sep 18, 2024
7 tasks
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Signed-off-by: Andrei Sandu <andrei-mihail@parity.io>
Copy link
Contributor

@alindima alindima left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Looking good overall!
Left some questions for my understanding and potential corner-cases


self.selected_core().and_then(|(core_selector, _cq_offset)| {
let core_index =
**assigned_cores.get(core_selector.0 as usize % assigned_cores.len())?;
Copy link
Member

Choose a reason for hiding this comment

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

what i didn't get from the RFC is why do we need to do % assigned_cores and not assume that the core_selector < assigned_cores.len()?

{
let (upward_messages, ump_signals) = upward_messages.split_at(separator_index);

if ump_signals.len() > 2 {
Copy link
Member

Choose a reason for hiding this comment

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

is 0 ump_signals allowed in case there's a separator?
is multiple separators allowed? we only look here for the last one
my thinking it should be no in both cases - might be good to validate those

//
// (relay_parent, state_root)
buffer: VecDeque<(Hash, Hash)>,
buffer: VecDeque<RelayParentInfo<Hash>>,
Copy link
Member

Choose a reason for hiding this comment

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

since we're making breaking changes to a storage that is used by a runtime API, we should make sure to workaround the recently experienced issue: #5738 by adjusting it

let min_relay_parent_number = shared::AllowedRelayParents::<T>::get()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦🏼‍♂️ good catch, I missed this, was thinking it is only used internally and not exposed via API.

Comment on lines +756 to +757
claim_queue: BTreeMap<CoreIndex, VecDeque<Id>>,
) -> BTreeMap<ParaId, BTreeMap<u8, BTreeSet<CoreIndex>>> {
Copy link
Member

Choose a reason for hiding this comment

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

Agree :)
Also, I find it a bit strange to have a BTreeMap<u8, BTreeSet<_>>
instead of a Vec<BTreeSet<CoreIndex>> where the index in the Vec represents the depth. Given that the depth is supposed to be a number 0..x and without gaps IIUC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T8-polkadot This PR/Issue is related to/affects the Polkadot network.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elastic scaling: relay chain support for new candidate receipts
4 participants