chore(networking): decouple discovery, add peers and connect to them

[alrevuelta]

Background

We currently have the following functionalities coupled into the same functions, making it difficult to extend or reuse components, and more in general, to handle connections:

• Discovery of peers
• Adding peers to peerstore
• Connecting to peers

Some examples:

• dialPeer dials a given peer but also attempts to add it to the peerstore.
• runDiscv5Loop both discovers peers but also tries to connect to them.
• wakuPeerExchange tries to connect to all discovered peers every time it does a request.
• wakuPeerExchange has its own cache, apart from the peerstore.

Acceptance criteria

• Every discovery method just takes care of adding peers to the peerstore.
• The peermanager takes care of selecting peers from the peerstore and connecting to them.
• PeerExchange no longer connects to peers, it just returns a set of new peers, that are handled in the waku_node

[alrevuelta]

@kaiserd since you created waku_peer_exchange #1152 perhaps you have some thought on this. TLDR: I would like to remove the enrCache from waku_peer_exchange and store all the peers in the node PeerStore intead, which is a common agenda for the node.

Since its also part of the spec (recommended) wanted to double check.

[kaiserd]

@alrevuelta Thanks for this issue :). This is part of the peer exchange overhaul.

As an additional task, you can add merging/removing the separate peer exchange discv5 loop.

However, the peer exchange peer cache cannot simply be merged into the general peer store.
Peers that are returned by peer exchange should be randomly sampled from the set of all peers,
meaning they should be retrieved independently.

We could (abstract):

• manage all discovered peers in a single peer store
  • (remove the px discv5 loop)
• sample px-peers from this general peer store
  • px-peers should have been discovered via discv5, because px-peers should be from a random sample over the whole discovery network
    • other future discovery methods that fulfill this are fine, too; DNS, or static bootstrap nodes do not fulfill this.
  • these px-peers have to be marked, e.g put in a specific peer book
  • marked peers should not be used for other purposes, but they can be considered for future px sampling
    • ideally, they should be removed after having been selected a few times, but we can leave that out for now
  • when discovering the same peers again via discv5, they should be unmarked
    • this important in the roll-out phase where the network is small
• return the sampled px-peers in a peer exchage response

[kaiserd]

@danisharora099 @D4nte This is part of the planned peer-exchange overhaul. This will change nwaku's logs with respect to peer exchange. Iirc you use the log messages for determining the wait time in js-waku tests. However, this change will significantly reduce necessary wait times, because the discv5 loops will be merged :).

[alrevuelta]

As agreed with @kaiserd, I will limit the scope of this issue to just add the peers from peer exchange to the PeerStore. The rest of the changes are substantial and will be dealt with after the networking mvp is complete.

[alrevuelta]

Some work torwards closing this has beeen done here: #1527

TLDR: When we get peers via px we add them to the peer store instead of trying to connect to them. This decouples discovery (px in this case) from connection (which its the peer manager job)