-
Notifications
You must be signed in to change notification settings - Fork 59
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.
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
fix(networkmanager): regularly disconnect from random peers #2553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just added a naïve question :)
if outConns.len >= toInt(MaxConnectedPeers/2): | ||
for p in outConns[0 ..< toInt(outConns.len/2)]: | ||
trace "Pruning Peer", Peer = $p | ||
asyncSpawn(node.switch.disconnect(p)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why disconnect only out
and not in
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason, it is mostly NM discovering and connecting to other nodes, so the out
is more important, but it is a good point to also make sure in
is not filling up too much
@@ -429,7 +441,8 @@ proc initAndStartApp( | |||
|
|||
nodeBuilder.withNodeKey(key) | |||
nodeBuilder.withRecord(record) | |||
nodeBuilder.withPeerManagerConfig(maxRelayPeers = none(int), shardAware = true) | |||
nodeBUilder.withSwitchConfiguration(maxConnections = some(MaxConnectedPeers)) | |||
nodeBuilder.withPeerManagerConfig(maxRelayPeers = some(20), shardAware = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shard aware peer manager will try to keep each shard populated with the same average # of peers.
It prevent too many in connections and also keep a % of max connection for service peers.
Are you sure that's what you want in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, I am not sure - what do you think?:) I probably want same average # of peers per shard. I don't really need any in connections since the main goal of NM is to discover nodes and keep track of the network, but not really to be discoverable by others, so limiting in connections might be useful?
I'll probably metge this PR and we can think about it for next one
Description
As reported by @chaitanyaprem, NetworkMonitor was stuck at
maxConnections = 50
and although it discovered new peers, it was not able to connect with internal errorToo many connections
This PR bumps macConnections to 150 and adds simple logic to regularly check and disconnect from half of currently connected peers selected randomly.
Changes
MaxConnectedPeers
constant (150)maxConnections
to the constant aboveHow to test
make networkmonitor
./build/networkmonitor --cluster-id=1 --metrics-server --rln-relay-eth-client-address=https://sepolia.infura.io/v3/...