feat(networking): prune peers from same ip beyond colocation limit #1765
Conversation
| case event.kind | ||
| of ConnEventKind.Connected: | ||
| let direction = if event.incoming: Inbound else: Outbound | ||
| discard | ||
| of ConnEventKind.Disconnected: | ||
| discard | ||
|
|
||
| # called when a peer i) first connects to us ii) disconnects all connections from us | ||
| proc onPeerEvent(pm: PeerManager, peerId: PeerId, event: PeerEvent) {.async.} = |
There was a problem hiding this comment.
minor refactor to avoid some duplicated code. no funcionality change.
alrevuelta
requested review from
jm-clius,
vpavlin and
Ivansete-status
and removed request for
jm-clius
| ## | ||
|
|
||
| # update the table tracking ip and the connected peers | ||
| pm.updateIpTable() |
There was a problem hiding this comment.
Mmm. This rebuilds the IP table every time, so I don't see any reason to cache the ipTable on the pm object. In other words, why not have a getPeersPerIp that simply creates and populates such a table every time?
There was a problem hiding this comment.
yup, but in the future I plan to use this table for other things. still unclear but some ideas:
- intercept a connection. before even allowing a connection to go through, check if colocation limit is ok.
- discovery. if we discover a peer violating the colocation limit, do not add it to the peer store.
- out connections. dont start connections to peers violating the colocation limit.
so would like to leave it if its fine.
| let connsToPrune = peersInIp.len - pm.colocationLimit | ||
| for peerId in peersInIp[0..<connsToPrune]: | ||
| debug "Pruning connection due to ip colocation", peerId = peerId, ip = ip | ||
| await pm.switch.disconnect(peerId) |
There was a problem hiding this comment.
May be obvious, but presumably this removes the peer from all peer books (including our own?) in the peer store as well so that we don't attempt an outgoing connection to it?
There was a problem hiding this comment.
actually it wasnt obvious, good catch, the peer was not being removed. added f9c25b4 removing + test case.
right now prunning is only for in connections, which is the attack this PR aims to mitigate. but yep i might implement it later on for out connections.
| asyncSpawn pm.updateMetrics() | ||
| asyncSpawn pm.relayConnectivityLoop() | ||
| asyncSpawn pm.prunePeerStoreLoop() | ||
| asyncSpawn pm.pruneConnsByIpLoop() | ||
| asyncSpawn pm.logSummary() |
There was a problem hiding this comment.
We have quite a large number of spawned tasks here. Wouldn't a single maintenance heartbeat be a better approach?
There was a problem hiding this comment.
yup, merged two of them in this commit
49db93e
not sure i can do merge more since each runs at different intervals, but will revisit once the peering strategy is clearer
closes #1749
Description:
colocationLimitconnected peers from the same ip.Note that this PR does not prevent connection attempts from peers violating the
colocationLimit. These connections will be succesful but cleaned up later on.