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.

Sign up for GitHub

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

Jump to bottom

parachain-system: send core selector ump signal #5888

Merged
merged 11 commits into from
Oct 7, 2024
Merged

parachain-system: send core selector ump signal #5888

merged 11 commits into from
Oct 7, 2024

Conversation

alindima
Copy link
Contributor

@alindima alindima commented Oct 1, 2024 •
edited
Loading

Runtime side of #5048

Send the core selector ump signal in cumulus. Guarded by a compile time feature until nodes are upgraded to a version that includes #5423 for gracefully handling ump signals.

@alindima
Copy link
Contributor Author

alindima commented Oct 1, 2024

/cmd prdoc --help

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024

Command help: 
usage: /cmd prdoc [-h] --pr PR [--audience AUDIENCE] [--bump BUMP]
                  [--force FORCE]

options:
  -h, --help           show this help message and exit
  --pr PR              The PR number to generate the PrDoc for.
  --audience AUDIENCE  The audience of whom the changes may concern.
  --bump BUMP          A default bump level for all crates.
  --force FORCE        Whether to overwrite any existing PrDoc.

@alindima
Copy link
Contributor Author

alindima commented Oct 1, 2024

/cmd prdoc --pr 5888

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024

Command "prdoc --pr 5888" has started 🚀 See logs here

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024

Command "prdoc --pr 5888" has finished ✅ See logs here

sandreim
sandreim approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@sandreim sandreim left a comment

Choose a reason for hiding this comment

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

LGTM! just some nits and questions.

cumulus/pallets/parachain-system/src/lib.rs Outdated Show resolved Hide resolved
cumulus/pallets/parachain-system/src/lib.rs
@@ -1418,6 +1429,21 @@ impl<T: Config> Pallet<T> {
CustomValidationHeadData::<T>::put(head_data);
}

/// Send the ump signals
#[cfg(feature = "experimental-ump-signals")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it would be a good idea to hide the `select_core_for_child runtime API behind this feature ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it helps us. The parachain can implement the API without any problem. I introduced the compile-time feature because otherwise the relay chain would have rejected these candidates

cumulus/client/consensus/aura/src/collators/lookahead.rs
@@ -260,8 +262,7 @@ where
relay_parent,
params.para_id,
&mut params.relay_client,
// Use depth 0, to preserve behaviour.
ClaimQueueOffset(0),
ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET),
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we want to use 0 to preserve old behaviour ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem with using 0 is this:

we have a DEFAULT_CLAIM_QUEUE_OFFSET of 1 (as you also suggested).

The default implementation of the SelectCore trait uses DEFAULT_CLAIM_QUEUE_OFFSET.

if we instead use here offset 0 and the runtime is updated to use offset 1, there will be a discrepancy and the collator will not advertise to the right core. we need these two places to use the same offset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explanation makes sense to me, offset of 1 should also not break anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an alternative would be to set the DEFAULT_CLAIM_QUEUE_OFFSET to 0 and have some kind of LookaheadCoreSelector impl of SelectCore that uses a claim queue offset of 1.

the current state of the PR (with lookahead using offset 1) breaks the coretime-shared-core zombienet test because core sharing doesn't work between paras if one of the paras is not producing blocks. will be fixed by #5461

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I think using a default claim queue offset of 0 is a must, because if the UMP signal is not present, the runtime will assume offset 0: #5423

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and another reason to use default offset 0 is because the claim queue size is at least 1 (so an offset of 1 will not work on a network that is configured with size 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

an alternative would be to set the DEFAULT_CLAIM_QUEUE_OFFSET to 0 and have some kind of LookaheadCoreSelector impl of SelectCore that uses a claim queue offset of 1.

I've implemented this now

cumulus/pallets/parachain-system/src/lib.rs Outdated Show resolved Hide resolved
sandreim
sandreim reviewed Oct 1, 2024
View reviewed changes
prdoc/pr_5888.prdoc Outdated Show resolved Hide resolved
@alindima alindima added T9-cumulus This PR/Issue is related to cumulus. I5-enhancement An additional feature request. labels Oct 1, 2024
@alindima alindima requested a review from skunert October 2, 2024 09:25
skunert
skunert approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@skunert skunert left a comment

Choose a reason for hiding this comment

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

LGTM!

cumulus/pallets/parachain-system/src/lib.rs Outdated Show resolved Hide resolved
cumulus/client/consensus/aura/src/collators/lookahead.rs
@@ -260,8 +262,7 @@ where
relay_parent,
params.para_id,
&mut params.relay_client,
// Use depth 0, to preserve behaviour.
ClaimQueueOffset(0),
ClaimQueueOffset(DEFAULT_CLAIM_QUEUE_OFFSET),
Copy link
Contributor

Choose a reason for hiding this comment

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

Explanation makes sense to me, offset of 1 should also not break anything here.

@alindima alindima added this pull request to the merge queue Oct 7, 2024
Merged via the queue into master with commit df8b870 Oct 7, 2024
204 of 206 checks passed
@alindima alindima deleted the alindima/cumulus-ump-signal branch October 7, 2024 09:11
@louismerlin louismerlin changed the title parachain-system: send core selector ump signal parachain-system: send core selector ump signalk Dec 9, 2024
@louismerlin louismerlin changed the title parachain-system: send core selector ump signalk parachain-system: send core selector ump signal Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sandreim sandreim sandreim approved these changes

@skunert skunert skunert approved these changes

Assignees

@sandreim sandreim

Labels
I5-enhancement An additional feature request. T9-cumulus This PR/Issue is related to cumulus.
Projects
Security Audit (PRs) - SRLabs
Status: Audited
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alindima @skunert @sandreim @actions-user