You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
why does subscriptions array in Peerconnector exist?
Reason: if Peerconnector isn't running and either of the peer discovery protocols (rendezvous/peer_exchange/discv5) adds a subscription. that subscription needs to be stored. So that once peer connector is running we can start getting peers from these saved subscriptions.
Why are there different peerCh and dialCh channels?
Reason: peerCh also have origin information that has to be added to peerManager. Where is dialCh is peerInfo field of peerCh which is only used for establishing connection.
why are there workerCtx, workerCancel and shouldDialPeers?
Reason: if no of outConnection exceeds outRelayTarget, then pause.
Let's consider host connected relay peers are more than outRelayTarget, then shouldDialPeers will pause PeerConnector. Since peerCh is unbuffered, this will block rendezvous(https://github.com/waku-org/go-waku/blob/master/waku/v2/rendezvous/rendezvous.go#L95). Not block peer_exchange(as response is handled asynchronisely)/discv5(async runDiscoveryV5Loop-> peerLoop-> peerConnector.Subscribe)
Note: if new peers are discovered and peerConnector is paused we don't drop those peers, waits peerConnector to unPaused.
Why is waitGroup needed?
it can happen that one of goroutine ends after the peerCh or dialCh is closed. So it might type to send to that routine which will panic.
Problem with workerCtx, scenario:
AddDiscoveredPeer called for PeerData.
peerConnector is in paused state. workerCancel is called.
This new peer will not make the connection even though it is saved in host due to AddDiscoveredPeer call.
Questions:
shouldn't c.pm.GroupPeersByDirection() be c.pm.relayPeers()? Since GroupPeersByDirection doesn't filter by relay.
should we drop the peers from subscription when the peerConnector is paused? There can be multiple handling routine bcz of this.
should we drop the peers from subscription when the peerConnector is paused? There can be multiple handling routine bcz of this.
Not sure. IIUC, peers from the subscription be consumed only while the peerConnector is running. If so, the worst that could happen is that you end up receiving peers that are not online when the peerConnector runs again, and you'd waste some seconds while attempting to connect to them.
shouldn't c.pm.GroupPeersByDirection() be c.pm.relayPeers()? Since GroupPeersByDirection doesn't filter by relay.
GroupPeersByDirection is just grouping all peers we are connected to. There can be peers which is not a relayPeer and can just be providing other Waku services only. So, would not recommend renaming it.
Also note that, the current peerMgmt implementation is tied with managing relay peer connections. In future this will get expanded to manage connections towards other service peers as well.
shouldn't c.pm.GroupPeersByDirection() be c.pm.relayPeers()? Since GroupPeersByDirection doesn't filter by relay.
GroupPeersByDirection is just grouping all peers we are connected to. There can be peers which is not a relayPeer and can just be providing other Waku services only. So, would not recommend renaming it.
Also note that, the current peerMgmt implementation is tied with managing relay peer connections. In future this will get expanded to manage connections towards other service peers as well.
Currently peerConector is paused when inDirectionPeer > inRelayPeersTarget. Whereas at another place in peerManager, we have similar check inRelayPeer>pm.InRelayPeersTarget. There after getting the direction peers we also filter by proto(/relay/2.0.0 in this case).
shouldn't c.pm.GroupPeersByDirection() be c.pm.relayPeers()? Since GroupPeersByDirection doesn't filter by relay.
GroupPeersByDirection is just grouping all peers we are connected to. There can be peers which is not a relayPeer and can just be providing other Waku services only. So, would not recommend renaming it.
Also note that, the current peerMgmt implementation is tied with managing relay peer connections. In future this will get expanded to manage connections towards other service peers as well.
Currently peerConector is paused when inDirectionPeer > inRelayPeersTarget. Whereas at another place in peerManager, we have similar check inRelayPeer>pm.InRelayPeersTarget. There after getting the direction peers we also filter by proto(/relay/2.0.0 in this case).
PeerConnector is paused when outPeersTarget is reached. Whereas in peerManager we have checks for both inPeers and outPeers target.
Did not get your point here. Maybe i am missing something. Can you elaborate?
Sorry, i confused with inRelay.
What i meant to say was:
OutDirectionPeer is more than or eq to outRelayPeers.
Let say OutDirectionPeer = 4 and outRelayPeer are 2. If if outTarget in peerManager is 3, currently peerConnector will be paused but peerManager will keep sending peer to connect as outRelayPeer<outRelayTarget. Where as peerConnector is using OutDirectionPeer, so it will think needed no of out relay peer is already achieved.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
harsh-98
force-pushed
the
peerConnector
branch
from
Understanding:
why does subscriptions array in Peerconnector exist?
Reason: if Peerconnector isn't running and either of the peer discovery protocols (rendezvous/peer_exchange/discv5) adds a subscription. that subscription needs to be stored. So that once peer connector is running we can start getting peers from these saved subscriptions.
Why are there different peerCh and dialCh channels?
Reason: peerCh also have origin information that has to be added to peerManager. Where is dialCh is peerInfo field of peerCh which is only used for establishing connection.
why are there workerCtx, workerCancel and shouldDialPeers?
Reason: if no of outConnection exceeds outRelayTarget, then pause.
Let's consider host connected relay peers are more than outRelayTarget, then shouldDialPeers will pause PeerConnector. Since peerCh is unbuffered, this will block rendezvous(https://github.com/waku-org/go-waku/blob/master/waku/v2/rendezvous/rendezvous.go#L95). Not block peer_exchange(as response is handled asynchronisely)/discv5(async runDiscoveryV5Loop-> peerLoop-> peerConnector.Subscribe)
it can happen that one of goroutine ends after the peerCh or dialCh is closed. So it might type to send to that routine which will panic.
Problem with workerCtx, scenario:
This new peer will not make the connection even though it is saved in host due to AddDiscoveredPeer call.
Questions: