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

Expose AuthorFilter Call enum #706

Merged
merged 3 commits into from
Jul 11, 2022
Merged

Expose AuthorFilter Call enum #706

merged 3 commits into from
Jul 11, 2022

Conversation

sea212
Copy link
Member

@sea212 sea212 commented Jul 8, 2022

Closes #697

We have to set the eligibleCount of the author_slot_filter pallet to 1 to give every collator even chances of collation and to significantly reduce the number of forks and reorgs. To do so, root has to call a dispatchable of the author_slot_filter pallet. Unfortunately the Call enum of the author_slot_filter pallet is not exposed in the runtime, rendering setting this value through governance impossible.

@sea212 sea212 added the s:review-needed The pull request requires reviews label Jul 8, 2022
@sea212 sea212 added this to the v0.3.4 milestone Jul 8, 2022
@sea212 sea212 self-assigned this Jul 8, 2022
@sea212 sea212 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Jul 9, 2022
@sea212 sea212 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Jul 9, 2022
Copy link
Member

@Chralt98 Chralt98 left a comment

Choose a reason for hiding this comment

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

You found a temporary fix of disabling the cache, preventing the storage root mismatch because of the known bug. So my first question was, if this PR is still required then. I could imagine, that we would like to use the extrinsic of the pallet_author_slot_filter in the future. So I would lean towards approving.

It might be, that I don't get the full context now, but wouldn't setting the eligibleCount to 1 to only allow one collator to produce blocks (per active slot)? I could imagine, that this would lead to allowing one malicious lucky collator to rule the network with manipulated blocks.

@sea212
Copy link
Member Author

sea212 commented Jul 11, 2022

You found a temporary fix of disabling the cache, preventing the storage root mismatch because of the known bug. So my first question was, if this PR is still required then. I could imagine, that we would like to use the extrinsic of the pallet_author_slot_filter in the future. So I would lean towards approving.

It might be, that I don't get the full context now, but wouldn't setting the eligibleCount to 1 to only allow one collator to produce blocks (per active slot)? I could imagine, that this would lead to allowing one malicious lucky collator to rule the network with manipulated blocks.

@Chralt98 Just realized I referenced the wrong issue in this PR. Does #697 (comment) answer your question?

@sea212
Copy link
Member Author

sea212 commented Jul 11, 2022

In addition to that, collators cannot craft blocks as they like, they have to adhere to the consensus rules, other nodes will reject invalid state transitions that don't adhere to the consensus rules. A collator can control which transactions are executed though. If the complete active collator set is occupied by a malicious entity, it can censor transactions (just never include them) or even halt the network.

@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Jul 11, 2022
@sea212 sea212 merged commit 899ed47 into main Jul 11, 2022
@sea212 sea212 deleted the sea212-add-author-filter-calls branch July 11, 2022 13:11
@Chralt98
Copy link
Member

Thanks for your explanation. But why does the pallet_author_slot_filter set the default value of eligibleCount to 50? I mean, the number of forks and reorgs in other parachains should be also very high then. As long as the other collators validate the eligible collator which produces a new block, setting eligibleCount to 1 sounds reasonable.

Copy link
Contributor

@maltekliemann maltekliemann left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Network] Number of forks and reorgs is too high
3 participants