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

Filter Authors at each block height #229

Merged
merged 17 commits into from
Feb 8, 2021
Merged

Conversation

JoshOrndorff
Copy link
Contributor

What does it do?

This PR adds a pallet (pallet-author-filter) to the Moonbeam runtime that filters the complete set of staked active validators down to a subset at each block height. This means that, similar to aura and babe, simply being an active staked validator does not guarantee the right to author at every height. Rather the right to author is granted to a pseudorandomly selected subset of active validators at each height. This allows all nodes to get a (more) fair chance to author blocks.

What important points reviewers should know?

This is somewhat reactionary. The current cumulus authoring logic is that each node authors at each height and the first valid block to arrive at a validator is accepted. This means that authors with the lowest latency can dominate the authoring which leads to collecting all the block rewards and also creates an opportunity for censorship. Restricting the set or authors at each block works around this.

This change is entirely in the runtime. That means that the runtime logic will only admit block authored by valid authors. But the on the client side, the node is not yet smart enough to see that it is not a valid author. Thus it will still author a block. That block will be invalid and not gossiped, and will have no affect on the chain. However it does represent the authoring node wasting effort.

Is there something left for follow-up PRs?

  • Teach the client not to author when its blocks won't be valid
  • Authors need to actually sign their blocks to prove who authored the block

Checklist

  • ❌ Does it require a purge of the network?
  • [TODO] You bumped the runtime version if there are breaking changes in the runtime ?
  • ❌ Does it require changes in documentation/tutorials ?

@notlesh
Copy link
Contributor

notlesh commented Feb 4, 2021

This seems like a good start / POC, and I think it will help for those in "the middle of the pack." But for those outliers who have far greater latency than the average, I think this will do almost nothing to improve their chances of getting a block in / getting a reward.

Let's say Alice is the worst of the collators with an average of 300ms rtt to the validators and that the next-worst (Bob) has an average of 150ms, and further that our candidate filter selects 2 candidates at a time. In this case, Alice will have her best chance at getting a block in when the filter selects only her and Bob. But she will almost certainly lose this race every time. Alice will be unlikely to ever have a block included.

That said, I think this solution would be effective when there is less variance. We should repeat our tests from earlier to see how it performs.

@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 Feb 4, 2021
pallets/author-filter/src/lib.rs Outdated Show resolved Hide resolved
So it is guaranteed to happen after the relay chain randomness has been 
injected via inherent.
Will happen in authorshi_inherent's on_finalize.
@JoshOrndorff JoshOrndorff marked this pull request as ready for review February 5, 2021 18:03
@JoshOrndorff JoshOrndorff added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Feb 8, 2021
@JoshOrndorff JoshOrndorff merged commit 69476e0 into master Feb 8, 2021
@JoshOrndorff JoshOrndorff deleted the joshy-filter-author branch February 8, 2021 18:27
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