Split `Peerset` into reputation store & `ProtocolController`s #13611

dmitry-markin · 2023-03-15T16:13:44Z

The connection logic (connect/drop/accept/reject) is extracted from Peerset into ProtocolController and PeerStore. Each ProtocolController instance is responsible for a specific notifications protocol, and should allow customization of connection logic by protocols in the future. PeerStore is responsible for connection candidates and peer reputations.

Resolves #13531.

altonen

Did a first pass. There are some problems we need to address, especially regarding Peer. I hope we can get rid of it and just match on peer state and then perform the correct action.

client/peerset/src/protocol_controller.rs

Co-authored-by: Aaro Altonen <[email protected]>

…(_)` & `NotConnected`

altonen

I think this is going in the right direction. Some of code duplication should be removed if possible

client/peerset/src/protocol_controller.rs

Co-authored-by: Aaro Altonen <[email protected]>

dmitry-markin · 2023-03-17T20:23:23Z

@altonen I've reworked peer management using ideas from your prototype. Apart from a couple of places where I had to call self.peerset_handle.is_banned() instead of self.is_banned(), borrowing issues are gone. Implementation of alloc_slots() feels sub-optimal, but that's likely an inevitable consequence of separating peer reputation values and connection handling.

altonen

I think this is looking good already. alloc_slots() can be improved but could you write some tests to confirm this implementation works as expected?

client/peerset/src/protocol_controller.rs

dmitry-markin · 2023-03-20T09:02:22Z

... could you write some tests to confirm this implementation works as expected?

Sure, that was my plan.

Co-authored-by: Aaro Altonen <[email protected]>

dmitry-markin · 2023-05-16T09:32:33Z

The PR is ready for review. Relaxing requirements on possible messages in Notifications a little more allowed to get rid of ACKing, preserving the eventual consistency of peer views in ProtocolController and Notifications.

dmitry-markin · 2023-05-16T09:35:04Z

We definitely need a burn-in for this PR.

altonen · 2023-05-16T09:48:58Z

We definitely need a burn-in for this PR.

Versi burn-in would be nice

altonen

LGTM but let's merge it only after the burn-in is done

client/network/src/protocol/notifications/behaviour.rs

client/peerset/src/lib.rs

client/peerset/src/protocol_controller.rs

melekes

client/network/src/service/traits.rs

client/peerset/src/protocol_controller.rs

Co-authored-by: Anton <[email protected]>

)" This reverts commit b9c9522.

…13611)

WIP: Introduce ProtocolController

143bc98

dmitry-markin added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 15, 2023

altonen reviewed Mar 15, 2023

View reviewed changes

dmitry-markin and others added 4 commits March 16, 2023 10:56

Code review suggestions: docs and trace->info

5256f19

Apply suggestions from code review

c333aac

Co-authored-by: Aaro Altonen <[email protected]>

Rename ConnectionState->PeerState and reduce states to `Connected…

b50f134

…(_)` & `NotConnected`

Get rid of Peer abstraction

2670615

altonen reviewed Mar 16, 2023

View reviewed changes

Apply suggestions from code review

f6b9bca

Co-authored-by: Aaro Altonen <[email protected]>

dmitry-markin added 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 Mar 17, 2023

dmitry-markin requested a review from a team March 17, 2023 12:24

Rework peer management + implement alloc_slots

6c6fb75

minor: naming

7d7d1d8

altonen reviewed Mar 18, 2023

View reviewed changes

dmitry-markin and others added 13 commits March 20, 2023 12:13

Apply suggestions from code review

de8e6d2

Co-authored-by: Aaro Altonen <[email protected]>

Apply review suggestions

4ab043d

Refactor on_peer_dropped()

770a38a

minor: fix docs

f4401cf

Add more docs

91a66ec

Don't put connected peers into the list of regular nodes

fd43963

minor: docs

63385ff

WIP: add tests

368eaed

minor: take no more connection candidates than slots available

4c95216

Test both connect/accept for reserved nodes

d09dc00

Test banned reserved nodes, fix alloc_slots

7a25e8a

Add more tests

9747eaf

minor: remove unneded tokio dev dependecy

0b4a4c2

altonen approved these changes May 16, 2023

View reviewed changes

client/network/src/protocol/notifications/behaviour.rs Show resolved Hide resolved

client/peerset/src/lib.rs Show resolved Hide resolved

client/peerset/src/lib.rs Outdated Show resolved Hide resolved

client/peerset/src/protocol_controller.rs Show resolved Hide resolved

altonen requested a review from a team May 16, 2023 10:48

Delete Peerset test duplicating ProtocolController tests

263db27

dmitry-markin mentioned this pull request May 16, 2023

Return peer info instead of Peerset::debug_info paritytech/polkadot-sdk#507

Open

melekes reviewed May 18, 2023

View reviewed changes

dmitry-markin mentioned this pull request May 18, 2023

Get rid of Peerset stub and move ProtocolController to protocols #14170

Closed

dmitry-markin and others added 3 commits May 18, 2023 12:41

minor: add issue references in TODOs

47edf31

Apply suggestions from code review

b50d998

Co-authored-by: Anton <[email protected]>

rustfmt

2cd0188

dmitry-markin mentioned this pull request Aug 24, 2023

Add backpressure to Notifications substreams allocation paritytech/polkadot-sdk#506

Open

monor: format after rustfmt

a0dcac4

dmitry-markin mentioned this pull request May 18, 2023

[do not merge] [companion] Split Peerset into reputation store & ProtocolControllers paritytech/polkadot#7252

Closed

dmitry-markin added 3 commits May 22, 2023 13:39

Add in/out slot counters logging

6960b8f

Merge remote-tracking branch 'origin/master' into dm-peerset-splitting

b7a5a48

Make clippy happy

a03b9a0

dmitry-markin requested a review from a team May 22, 2023 11:12

melekes approved these changes May 23, 2023

View reviewed changes

dmitry-markin merged commit b9c9522 into master May 23, 2023

dmitry-markin deleted the dm-peerset-splitting branch May 23, 2023 11:49

gpestana pushed a commit that referenced this pull request May 27, 2023

Split Peerset into PeerStore & ProtocolControllers (#13611)

d0ee758

github-actions bot mentioned this pull request Jun 15, 2023

Update substrate/polkadot/cumulus from v0.9.40 to v0.9.43 moonbeam-foundation/moonbeam#2354

Closed

kiltbot mentioned this pull request Jun 21, 2023

1️⃣ [AUTOMATIC] Update Polkadot dependencies from 0.9.42 to 0.9.43 KILTprotocol/kilt-node#531

Closed

kacperzuk-neti mentioned this pull request Jun 23, 2023

Polkadot v0.9.43 liberland/liberland_substrate#295

Merged

15 tasks

dmitry-markin added a commit that referenced this pull request Jul 3, 2023

Revert "Split Peerset into PeerStore & ProtocolControllers (#13611

8f8866e

)" This reverts commit b9c9522.

Ank4n pushed a commit that referenced this pull request Jul 8, 2023

Split Peerset into PeerStore & ProtocolControllers (#13611)

b2eea2b

nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023

Split Peerset into PeerStore & ProtocolControllers (paritytech#…

2153a9a

…13611)

dmitrylavrenov mentioned this pull request Oct 29, 2024

Tracking issue to bump substrate related deps to polkadot-v0.9.43 humanode-network/humanode#923

Open

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split `Peerset` into reputation store & `ProtocolController`s #13611

Split `Peerset` into reputation store & `ProtocolController`s #13611

dmitry-markin commented Mar 15, 2023 •

edited

Loading

altonen left a comment

altonen left a comment

dmitry-markin commented Mar 17, 2023

altonen left a comment

dmitry-markin commented Mar 20, 2023 •

edited

Loading

dmitry-markin commented May 16, 2023 •

edited

Loading

dmitry-markin commented May 16, 2023

altonen commented May 16, 2023

altonen left a comment

melekes left a comment

Split Peerset into reputation store & ProtocolControllers #13611

Split Peerset into reputation store & ProtocolControllers #13611

Conversation

dmitry-markin commented Mar 15, 2023 • edited Loading

altonen left a comment

Choose a reason for hiding this comment

altonen left a comment

Choose a reason for hiding this comment

dmitry-markin commented Mar 17, 2023

altonen left a comment

Choose a reason for hiding this comment

dmitry-markin commented Mar 20, 2023 • edited Loading

dmitry-markin commented May 16, 2023 • edited Loading

dmitry-markin commented May 16, 2023

altonen commented May 16, 2023

altonen left a comment

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

Split `Peerset` into reputation store & `ProtocolController`s #13611

Split `Peerset` into reputation store & `ProtocolController`s #13611

dmitry-markin commented Mar 15, 2023 •

edited

Loading

dmitry-markin commented Mar 20, 2023 •

edited

Loading

dmitry-markin commented May 16, 2023 •

edited

Loading