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

Don't disconnect reserved peers if their reputation falls below threshold#9003

Closed
tomaka wants to merge 2 commits intoparitytech:masterfrom
tomaka:no-ban-reserved
Closed

Don't disconnect reserved peers if their reputation falls below threshold#9003
tomaka wants to merge 2 commits intoparitytech:masterfrom
tomaka:no-ban-reserved

Conversation

@tomaka
Copy link
Contributor

@tomaka tomaka commented Jun 3, 2021

When the reputation of peer falls below the banning threshold, we currently unconditionally disconnect it from all sets. In other words, we close all its substreams. This happens even if the peer is reserved.

However, the alloc_slots functions, which allocates peers to slots, doesn't take the reputation into account when it comes to reserved peers. It always opens a substream to all reserved nodes if there isn't one.

So what happens when the reputation of a reserved peer drops too low is that it gets disconnected, but alloc_slots later gets called at an indefinite point in time (when another peer disconnects from that set), and the substream will be re-opened.
If, then, we do a small insubstantial reputation decrease (we sometimes do small reputation decreases in order to account for heavy requests) while the reserved node is still under the ban threshold, it will be disconnected again.

This behaviour can be rather surprising, so this PR prevents reserved nodes from being disconnected if their reputation falls too low.

It also calls alloc_slots immediately if the peer gets disconnected, as this was an oversight.

@tomaka tomaka 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 Jun 3, 2021
@tomaka tomaka requested a review from ordian June 3, 2021 08:50
@bkchr
Copy link
Member

bkchr commented Jun 3, 2021

Do we keep all the relay chain validators as reserved? If yes, what happens if one of these validators really starts to spam all other validators?

@ordian
Copy link

ordian commented Jun 3, 2021

Do we keep all the relay chain validators as reserved? If yes, what happens if one of these validators really starts to spam all other validators?

Yes and I have the same concern. Either we need to change Polkadot not to use reserved slots for every relay chain validator or change substrate not to treat reserved peers differently when it comes to reputation. It seems to the latter change would be easier to implement.

@ordian
Copy link

ordian commented Jun 11, 2021

Closing as #9020 is merged.

@ordian ordian closed this Jun 11, 2021
@tomaka tomaka deleted the no-ban-reserved branch June 11, 2021 12:45
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

Status: Done ✅

Development

Successfully merging this pull request may close these issues.

3 participants