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.

Sign up for GitHub

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

Jump to bottom

Refactor peer managment #2142

Merged
merged 6 commits into from
Mar 22, 2021
Merged

Refactor peer managment #2142

merged 6 commits into from
Mar 22, 2021

Conversation

dapplion
Copy link
Contributor

@dapplion dapplion commented Mar 7, 2021 •
edited

Group all peer management functionality into a single class PeerManager. Current master segregates the functionality between tasks, network, and sync. PeerManager does:

  • Perform handshake on peer connect, notify Sync only when peer is relevant. Ping and status peers every interval
  • Mantain a set of peer by discovering + disconnecting according to conditions: targetPeers, minPeers, requested subnets. Groups logic to do so in testable pure functions

All ReqResp logic moved to network/reqResp so it can easily notify the PeerManager on incoming requests.

This PR is a spin-off from #2111 which is still unclear if it causes more memory and lag issues than master. I will A/B test this PR too to ensure it performs at least as well as master

PeerManager

PeerManager runs an internal heartbeat function which is a convenient start to follow its logic

lodestar/packages/lodestar/src/network/peers/peerManager.ts

Line 119 in 03f356b

this.heartbeat();

Key sensitive logic:

  • prioritizePeers: How to decide which peers to connect to and which to disconnect given an existing peer set and target peer count + target subnets

Fixes #823 fixes #1989

@github-actions github-actions bot added Api scope-networking All issues related to networking, gossip, and libp2p. labels Mar 7, 2021
@codeclimate
Copy link

codeclimate bot commented Mar 7, 2021 •
edited

Code Climate has analyzed commit 1aa305c and detected 8 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 8

View more on Code Climate.

@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 7, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 8, 2021
@dapplion dapplion mentioned this pull request Mar 17, 2021
Closed
@dapplion dapplion mentioned this pull request Mar 20, 2021
Merged
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 20, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 20, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 20, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 21, 2021
@github-actions github-actions bot added the CLI label Mar 21, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 21, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 21, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 21, 2021
@ChainSafe ChainSafe deleted a comment from lgtm-com bot Mar 21, 2021
@lgtm-com
Copy link

lgtm-com bot commented Mar 21, 2021

This pull request fixes 1 alert when merging 674b7a3 into 867db03 - view on LGTM.com

fixed alerts:

  • 1 for Useless conditional

@dapplion
Copy link
Contributor Author

Rebased into a merge-able status. Ready for review.

mpetrunic
mpetrunic reviewed Mar 22, 2021
View reviewed changes
packages/lodestar-cli/src/options/beaconNodeOptions/network.ts
@@ -6,7 +6,7 @@ export interface INetworkArgs {
"network.discv5.bindAddr": string;
"network.discv5.bootEnrs": string[];
"network.maxPeers": number;
"network.minPeers": number;
"network.targetPeers": number;
Copy link
Member

Choose a reason for hiding this comment

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

whats the difference between that and maxPeers?
Idea behind minPeers was to not allow sync if peer count is below that

Copy link
Contributor Author

@dapplion dapplion Mar 22, 2021 •
edited

Choose a reason for hiding this comment

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

You refer to sync.minPeers, which is different option with the purpose you describe. In current master network.minPeers is used by libp2p's autoDial logic. In the Range Sync PR #2111 sync.minPeers is eliminated, so we will then only have network.minPeers and network.targetPeers

Copy link
Member

Choose a reason for hiding this comment

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

ok, and whats the difference between network.targetPeers and network.maxPeers? 😄

Copy link
Contributor Author

@dapplion dapplion Mar 22, 2021 •
edited

Choose a reason for hiding this comment

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

  • targetPeers: Below that number actively discover and connect to peers. Above that number actively disconnect peers
  • maxPeers: Hard cap to not allow any peer connections above that number.

You want maxPeers to be a little larger than targetPeers to allow new inbound peers that just joined the network connect to you (for the sake of the network's health).

packages/lodestar/src/constants/network.ts
@@ -159,3 +162,9 @@ export const goodbyeReasonCodeDescriptions: Record<string, string> = {
250: "Peer score too low",
251: "Peer banned this node",
};

/** Until js-libp2p types its events */
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we maintaining types for that still in libp2p-ts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

libp2p-ts types these events but as a string uninon, not an enum.

export type ConnectionEvents = 'peer:connect' | 'peer:disconnect';

https://github.com/ChainSafe/libp2p-ts/blob/86c1d9eca212194c8a5ce1ea6572a3bd694103e5/types/libp2p/index.d.ts#L95

Copy link
Member

Choose a reason for hiding this comment

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

Why not change that there instead of here? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just opened an issue ChainSafe/libp2p-ts#21

packages/lodestar/src/network/events.ts Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Mar 22, 2021

This pull request fixes 1 alert when merging 1aa305c into 867db03 - view on LGTM.com

fixed alerts:

  • 1 for Useless conditional

@wemeetagain wemeetagain merged commit 5a35fce into master Mar 22, 2021
@wemeetagain wemeetagain deleted the dapplion/peer-manager branch March 22, 2021 15:26
@dapplion dapplion mentioned this pull request Mar 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpetrunic mpetrunic mpetrunic left review comments

@wemeetagain wemeetagain wemeetagain approved these changes

@twoeths twoeths Awaiting requested review from twoeths

Assignees
No one assigned
Labels
scope-networking All issues related to networking, gossip, and libp2p.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider doing sync peer management ourselves Disconnect after goodbye is sent
3 participants
@dapplion @wemeetagain @mpetrunic