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

Always author in first slot of new round #704

Merged
merged 3 commits into from
Aug 19, 2021

Conversation

JoshOrndorff
Copy link
Contributor

@JoshOrndorff JoshOrndorff commented Aug 18, 2021

This PR introduces a temporary work-around to ensure that block production remains reliable even across round transitions.

Quoting from the body of the PR:

// Because the staking solution calculates the next staking set at the beginning
// of the first block in the new round, the only way to accurately predict the
// authors would be to run the staking election while predicting. However this
// election is heavy and will take too long during prediction. So instead we
// work around it by always authoring the first slot in a new round. A longer-
// term solution will be to calculate the staking election result in the last
// block of the ending round.

Downside
Because all collator nodes will author the first block in the new round while this code in effect, many of them will find that they are not in fact eligible. This will result in most collators displaying the following error a few times at the beginning of each round (every 300 blocks on moonriver).

2021-07-22 06:18:18 [🌗] Bad mandatory: Module { index: 18, error: 2, message: Some("CannotBeAuthor") }
2021-07-22 06:18:18 [🌗] ❌️ Mandatory inherent extrinsic returned error. Block cannot be produced.
2021-07-22 06:18:18 [🌗] Proposing failed. 

To make it more clear to collator node operators that this error is expected, harmless, and temporary, we've added an additional log at the beginning of each round that will tell the collator that the CannotBeAuthor error will come soon and is not a problem.

testing
I really have no idea how to test this...

@JoshOrndorff JoshOrndorff added A0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes C1-low Does not elevate a release containing this beyond "low priority". labels Aug 18, 2021
runtime/common/src/apis.rs Outdated Show resolved Hide resolved
@notlesh
Copy link
Contributor

notlesh commented Aug 18, 2021

As for the misleading error message, you could add another log statement when we attempt to author at the beginning of a round, something like "you can ignore the following Bad mandatory message"...

Copy link
Contributor

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

lgtm

@crystalin crystalin added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Aug 19, 2021
@crystalin
Copy link
Collaborator

@JoshOrndorff shouldn't the label be clientnoteworthy instead of runtime ?
Otherwise feel free to merge

@JoshOrndorff
Copy link
Contributor Author

All the code that changed here is in the runtime. Specifically it is in the Nimbus runtime API.

I agree that it feels funny to call this a runtime change though since that code is only ever executed in an off-chain context (called by the collator before authoring). I'm fine with either tag.

@JoshOrndorff
Copy link
Contributor Author

Thinking about it more, I actually think runtime-noteworthy is better for two reasons.

For this code to take effect, we need a runtime upgrade, but we don't need a client upgrade, nor will performing a client upgrade help in any way.

I still agree it feels weird though since runtime is usually approximately equal to "onchain".

@JoshOrndorff JoshOrndorff merged commit a45aa77 into master Aug 19, 2021
@JoshOrndorff JoshOrndorff deleted the joshy-always-author-at-beginning-of-round branch August 19, 2021 12:33
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 C1-low Does not elevate a release containing this beyond "low priority".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants