-
Notifications
You must be signed in to change notification settings - Fork 336
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4meta5
reviewed
Aug 26, 2021
4meta5
approved these changes
Aug 26, 2021
JoshOrndorff
commented
Aug 31, 2021
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
crystalin
reviewed
Sep 7, 2021
crystalin
approved these changes
Sep 7, 2021
…ections-in-slot-prediction
JoshOrndorff
force-pushed
the
joshy-elections-in-slot-prediction
branch
from
September 7, 2021 18:01
4765a30
to
8b96b06
Compare
crystalin
added
A8-mergeoncegreen
Pull request is reviewed well.
and removed
A0-pleasereview
Pull request needs code review.
labels
Sep 8, 2021
Based on burn-in testing with @purestakeoskar on Moonbase Alpha, I believe this PR is tested and ready to be merged. |
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.