Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Always allocate slots for reserved nodes #11909

Merged

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Jul 25, 2022

This PR addresses paritytech/polkadot-sdk#533

Probably, similar logic should be implemented for user protocols (non-default peer sets) in polkadot.

@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Jul 25, 2022
@dmitry-markin dmitry-markin changed the title Dm account for reserved nodes in peer slots Account for reserved nodes in peer slots Jul 25, 2022
@dmitry-markin dmitry-markin changed the title Account for reserved nodes in peer slots Always allocate slots for reserved nodes Jul 25, 2022
@tomaka tomaka removed their request for review July 25, 2022 18:42
@dmitry-markin dmitry-markin requested a review from bkchr July 26, 2022 09:19
@arkpar
Copy link
Member

arkpar commented Jul 26, 2022

The problem with copying a set of reserved peers on startup is that the reserved peers set can be modified at runtime with RPC calls. Ideally there should be only one instance of that list contained in the peerset. But from what I see this is already a problem in the code with important_peers as well.

@tomaka Is there a good reason for the peerset to be an async interface and not just a simple struct? Perhaps we can simplify things here a lot by making it a simple struct shared with an Arc if required?

@tomaka
Copy link
Contributor

tomaka commented Jul 26, 2022

Is there a good reason for the peerset to be an async interface and not just a simple struct?

No, there's no good reason. It was not my decision, and I've always been in favor of making it synchronous. I've tried to make it synchronous in the past, but encountered difficulties which I can't remember.

@dmitry-markin
Copy link
Contributor Author

So I guess we should refactor peerset interface first to be able to properly handle RPC-added reserved peers?

Does this PR make any sense as it is, or I better close it?

@arkpar
Copy link
Member

arkpar commented Jul 26, 2022

I think we can merge this for now and if you want to take a stab at refactoring that would be great.

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Jul 27, 2022

I think we can merge this for now and if you want to take a stab at refactoring that would be great.

I've created an issue for such refactoring for now. Hopefully I'll be able to do it after other tasks.

@dmitry-markin
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 969a344 into master Jul 29, 2022
@paritytech-processbot paritytech-processbot bot deleted the dm-account-for-reserved-nodes-in-peer-slots branch July 29, 2022 12:36
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
* Always allocate slots for reserved nodes

* minor: replace no-slot peer counter with a set
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Always allocate slots for reserved nodes

* minor: replace no-slot peer counter with a set
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants