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

Run collator election during slot prediction #744

Merged
merged 14 commits into from
Sep 8, 2021

Conversation

JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Aug 26, 2021

Background

When a Moonbeam collator imports a new relay chain block, it may have an opportunity to author the next parachain block. To determine whether it is eligible according the the Runtime's nimbus configuration (based on parachain staking in our case), the collator calls into the runtime to check whether its nimbus id will be selected in the next block.

In Substrate, runtime API calls are made against the ending state of the previous block. When preparing to author the first block in the new round this does not work because the active collator set may change in the next block.

What does this PR do?

In order to accurately predict eligibility, we need to run the election results prior to predicting. This PR adds the minimal amount of logic from parachain staking's on_initialize function needed to do the prediction accurately.

What important points reviewers should know?

Original conversation in (private) slack channel https://purestake.slack.com/archives/C01ACCURYRZ/p1629120510363600

One concern I initially had about running the election during prediction was that the election would be too heavy. Howver, unlike phragmen elections, our elections are just a matter of sorting a list and then taking its prefix. And in the future we may not even need to sort the list. So this concern may not be valid.

Prior to updating our dependencies to the polkadot-v0.9.8 series, we had the same behaviour that this PR introduces. At that time every runtime api call began with full initialization of every pallet. That changed in paritytech/substrate#8953 The election prediction logic being re-introduced here is considerably lighter than prior to that PR, and we never noticed performance problems at that time.

What alternative implementations were considered?

#725 also solves this problem, but in a much different way. That PR runs the staking election onchain ahead of time.

This PR is less invasive in terms of changing parachain staking, but that may be a useful feature beyond the scope of this problem.

Is anything left for followups

To make this work I had to make a function in parachain staking public. That is not the safest interface. If we like this approach long term, it may be helpful to modify parachain staking's interface to expose a safer public function for this purpose. But it isn't really that bad as it is.

runtime/common/src/apis.rs Outdated Show resolved Hide resolved
@4meta5 4meta5 added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Sep 1, 2021
@JoshOrndorff JoshOrndorff force-pushed the joshy-elections-in-slot-prediction branch from 4765a30 to 8b96b06 Compare September 7, 2021 18:01
@crystalin crystalin added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Sep 8, 2021
@JoshOrndorff
Copy link
Contributor Author

Based on burn-in testing with @purestakeoskar on Moonbase Alpha, I believe this PR is tested and ready to be merged.

@JoshOrndorff JoshOrndorff merged commit 24ae608 into master Sep 8, 2021
@JoshOrndorff JoshOrndorff deleted the joshy-elections-in-slot-prediction branch September 8, 2021 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants