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

Split Peerset into reputation store & ProtocolControllers #13611

Merged
dmitry-markin merged 106 commits into from
May 23, 2023
Merged

Split Peerset into reputation store & ProtocolControllers #13611

dmitry-markin merged 106 commits into from
May 23, 2023

Conversation

@dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Mar 15, 2023 •
edited
Loading

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.

@dmitry-markin dmitry-markin added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 15, 2023
altonen
altonen reviewed Mar 15, 2023
View reviewed changes
Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

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.

altonen
altonen reviewed Mar 16, 2023
View reviewed changes
Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

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

@dmitry-markin 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 dmitry-markin requested a review from a team March 17, 2023 12:24
@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Mar 17, 2023

@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
altonen reviewed Mar 18, 2023
View reviewed changes
Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

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?

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented Mar 20, 2023 •
edited
Loading

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

Sure, that was my plan.

@dmitry-markin
Copy link
Contributor Author

dmitry-markin commented May 16, 2023 •
edited
Loading

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
Copy link
Contributor Author

dmitry-markin commented May 16, 2023

We definitely need a burn-in for this PR.

@altonen
Copy link
Contributor

altonen commented May 16, 2023

We definitely need a burn-in for this PR.

Versi burn-in would be nice

altonen
altonen approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

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

@altonen altonen requested a review from a team May 16, 2023 10:48
@dmitry-markin dmitry-markin mentioned this pull request May 16, 2023
Open
melekes
melekes reviewed May 18, 2023
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

@dmitry-markin dmitry-markin mentioned this pull request May 18, 2023
Closed
@dmitry-markin dmitry-markin mentioned this pull request Aug 24, 2023
Open
@dmitry-markin dmitry-markin mentioned this pull request May 18, 2023
Closed
@dmitry-markin dmitry-markin requested a review from a team May 22, 2023 11:12
@dmitry-markin dmitry-markin merged commit b9c9522 into master May 23, 2023
@dmitry-markin 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
@dmitry-markin @gpestana
Split Peerset into PeerStore & ProtocolControllers (#13611)
d0ee758
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
Closed
@kiltbot kiltbot mentioned this pull request Jun 21, 2023
Closed
@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
dmitry-markin added a commit that referenced this pull request Jul 3, 2023
@dmitry-markin
Revert "Split Peerset into PeerStore & ProtocolControllers (#13611
8f8866e 
)"

This reverts commit b9c9522.
Ank4n pushed a commit that referenced this pull request Jul 8, 2023
@dmitry-markin @Ank4n
Split Peerset into PeerStore & ProtocolControllers (#13611)
b2eea2b
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@dmitry-markin @nathanwhit
Split Peerset into PeerStore & ProtocolControllers (paritytech#…
2153a9a 
…13611)
@dmitrylavrenov dmitrylavrenov mentioned this pull request Oct 29, 2024
Closed
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@kpp kpp Awaiting requested review from kpp

@andresilva andresilva Awaiting requested review from andresilva

2 more reviewers

@melekes melekes melekes approved these changes

@altonen altonen altonen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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. D2-breaksapi

Projects

Networking
Status: Done ✅

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactoring: prepare Peerset for moving connection establishment logic to protocol handlers

4 participants

@dmitry-markin @altonen @melekes