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 PVF precompilation for Kusama #5606

Merged
merged 5 commits into from
Sep 6, 2024

Conversation

AndreiEres
Copy link
Contributor

image

Because on Kusama validators.len() < discovery_keys.len() we can tweak the PVF precompilation to allow prepare PVFs when the node is an authority but not a validator.

@AndreiEres AndreiEres added T0-node This PR/Issue is related to the topic “node”. I5-enhancement An additional feature request. T8-polkadot This PR/Issue is related to/affects the Polkadot network. labels Sep 5, 2024
@paritytech-cicd-pr
Copy link

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

Copy link
Member

@eskimor eskimor left a comment

Choose a reason for hiding this comment

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

Function name could be more descriptive, other than that: Looks good.


// There is still a chance to be a previous session authority, but this extra work does not
// affect the finalization.
is_past_present_or_future_authority && !is_present_authority
is_past_present_or_future_authority && !is_present_validator
Copy link
Contributor

Choose a reason for hiding this comment

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

This distinction exists until we remove the max_validators configuration so even without this fix it still works as expected on Polkadot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

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 at what check_next_session_authority, I understand this change, but it seems to conflate multiple cases in a not-ideal way.

My understanding is that after restart we want to check if we are a (para)validator in the current or the next session, and if so, precompile PVFs.
Also, if we are already running and are not a (para)validator in the current session, but are in the next, we also want to precompile PVFs.

What this function does is checks for (not-only-para)validator set of current and next session along with a paravalidator set of past dispute_period sessions. It doesn't distinguish fresh start from already running and checks if we are not current session (with this change para)validator.
Ideally, we'd have a separate runtime API to tell us if we are in the next (para)validator set, but we don't have that currently, so I understand this is a workaround solution. We could also probably simplify the check to just look at the next set assuming that if we already have the artifacts cached, pre-compilation will be a no-op.

@ordian
Copy link
Member

ordian commented Sep 6, 2024

Missing prdoc

@AndreiEres AndreiEres added this pull request to the merge queue Sep 6, 2024
Merged via the queue into master with commit 5040b3c Sep 6, 2024
206 of 208 checks passed
@AndreiEres AndreiEres deleted the AndreiEres/pvf-precompilation-kusama branch September 6, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I5-enhancement An additional feature request. T0-node This PR/Issue is related to the topic “node”. 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.

6 participants