chore(networking): set and use target outbound connections + prune #1739
Conversation
|
|
||
| for p in inRelayPeers[0..<connsToPrune]: | ||
| await pm.switch.disconnect(p) | ||
|
|
||
| proc connectToRelayPeers*(pm: PeerManager) {.async.} = |
There was a problem hiding this comment.
I'd rename this proc to something like updateRelayPeersConnections
There was a problem hiding this comment.
wops i missed this. isn't connectToRelayPeers shorter and more descriptive?
There was a problem hiding this comment.
Hey @alrevuelta ! It is shorter but this proc not only connectToNodes but also pruneInRelayConns. This is why I see it more convenient to have the "update" word on its name. Maybe sth like updateRelayPeers can suit better, which is even shorter ;P
| if outRelayPeers.len >= pm.outPeersTarget: | ||
| return |
There was a problem hiding this comment.
Shall we also prune the exceeding out connection at this point?
There was a problem hiding this comment.
good point, but the thing is that since we control the out connections that we create, that shouldnt be necessary. i mean, we should never exceed the max out limit, since we do this check at connectToRelayPeers.
note that on top of this in prunning, we will have a prunning based on libp2p scoring, which will prune in/out connections from our mesh based on a given score. But this comes from gossipsub.
| proc logSummary*(pm: PeerManager) {.async.} = | ||
| heartbeat "Log peer manager summary", LogSummaryInterval: | ||
| let (inRelayPeers, outRelayPeers) = pm.connectedPeers(WakuRelayCodec) | ||
| let maxConnections = pm.switch.connManager.inSema.size |
There was a problem hiding this comment.
I wonder if it may be also interesting to print the in/out ratio, for debugging purposes.
There was a problem hiding this comment.
its there right? or wdym? we have inPeersTarget and outPeersTarget
inRelayConns = $inRelayPeers.len & "/" & $inPeersTarget,
outRelayConns = $outRelayPeers.len & "/" & $pm.outPeersTarget,
There was a problem hiding this comment.
Morning! I mean to print the ratio that we expect. This is the real ratio that is being applied. I would print the theoretical ratio just in case it is not accomplished by the real one, for any reason.
Not a big deal though. If you consider we have enough info, then go ahead :)
There was a problem hiding this comment.
I see. if you dont mind i leave it as it is, since whats being printed here its more complete imho. eg: "...5/10, 30/50" which indicates that 5 in slots out of 10 are taken and 30 out of 50 out are taken.
There was a problem hiding this comment.
Ok cool, is perfect then! thanks !
| proc pruneInRelayConns(pm: PeerManager, amount: int) {.async.} = | ||
| let (inRelayPeers, outRelayPeers) = pm.connectedPeers(WakuRelayCodec) | ||
| let connsToPrune = min(amount, inRelayPeers.len) |
There was a problem hiding this comment.
| proc pruneInRelayConns(pm: PeerManager, amount: int) {.async.} = | |
| let (inRelayPeers, outRelayPeers) = pm.connectedPeers(WakuRelayCodec) | |
| let connsToPrune = min(amount, inRelayPeers.len) | |
| proc pruneInRelayConns(pm: PeerManager, connsToPrune: int) {.async.} = |
As per how this proc is used, the min(amount, inRelayPeers.len) will always return amount.
There was a problem hiding this comment.
yup, as its used yep but you never know hoy a function will be used. good sanity check to avoid accessing out of bound elements.
There was a problem hiding this comment.
Cool, makes sense!
There was a problem hiding this comment.
Would it make sense to pass list of peerIds to prune here though? Instead of amount - that way we could avoid counting/getting the peers again (you already do it before calling this function) and it would allow for potentially "smarter" ways of choosing the peers to prune - this just goes from first peer, but by passing an array of peerIds to prune the caller could come up with a different way how to choose the peers to prune
| @@ -333,6 +337,7 @@ proc new*(T: type PeerManager, | |||
| storage: storage, | |||
| initialBackoffInSec: initialBackoffInSec, | |||
| backoffFactor: backoffFactor, | |||
| outPeersTarget: max(maxConnections div 10, 10), | |||
There was a problem hiding this comment.
Thinking out loud that 1:9 out:in ratio may be a bit permissive as this increases the eclipse attack surface. Arguably the most trustworthy relay connections from each node's perspective that will provide the most stable mesh are the outgoing connections based on random discv5 walks. Perhaps then a further consideration could be to ensure that e.g. 50% of the mesh peerings are outgoing (although that will probably break gossipsub in unexpected ways). I don't have any ratio or other suggestion here, but highlighting for future discussion and investigation. :)
There was a problem hiding this comment.
yup, the eclipse attack was also my concern. took this ratio from both nimbus (1:9) and prysm (1:8) but we can revisit later on.
unsure how they justify this number given the possibility of eclipse attacks. Perhaps from a practical point of view its more complex since the "eclipser" is compeeting with the whole network to get these slots, so it would be very difficult for the attacker to end up having all slots?
There was a problem hiding this comment.
Probably unrelated to the eclipse attack, but would it make sense to make to ratio configurable?
Description
closes #1495