fetch peers from local peerStore instead of caching them as a temporary fix

[Ivansete-status]

Background

@igor-sirotin did a great analysis (thanks for it 🥳) about what is happening:

I have a nwaku bootnode running.

Then I start a status-backend.
The container gets some IP 172.20.0.6.
The go-waku instance selects a random port 22333
We use an ephemeral private key for waku, so it has a random PeerID
Let's call the above ENR-1

Here status-backend:
✅ statically connects to the bootnode
✅ can be discovered through peer exchange

Now, keeping the bootnode running, I restart the status-backend
it gets the same IP 172.20.0.6
it selects a random port 33444
new ephemeral private key, so a new PeerID
Let's call the above ENR-2

The restarted status-backend:
✅ statically connects to the bootnode
❌ can not be discovered through peer exchange

This happens because the bootnode continues to share ENR-1, and does not share ENR-2. So other backends get (through peer exchange with bootnode) an outdated ENR, try to connect to it and obviously fail.

After waiting for ~10 min, the bootnode forgets ENR-1 and starts to advertise ENR-2. Then the status-backend can be discovered and connected to

Details

In order to enhance that, we need to apply @fryorcraken and @chaitanyaprem suggestions.

• Avoid this poll and instead , update the local peer-exchange cache with the recently new discovered/connected peers. whenever a request for peers is received fetch data from peerStore using getReachablePeers, filter it out and then respond.
  - Also, is interesting to keep a regular poll to check peers' liveness. If a counter-party peer doesn't reply properly in 10 minutes, then remove it from the peer-exchange cache.
  • note: maybe this last point is not needed if we are able to reliably detect when a peer disconnects from us.

[chaitanyaprem]

Thanks for collating this info into an issue!

Also, is interesting to keep a regular poll to check peers' liveness. If a counter-party peer doesn't reply properly in 10 minutes, then remove it from the peer-exchange cache.

* note: maybe this last point is not needed if we are able to reliably detect when a peer disconnects from us.

Well, we may not need to do an actual liveness check but just use the getReachablePeers API itself and remove peers from cache that are not in the returned filtered list.

[chaitanyaprem]

Updating as per discussion in discord, we don't need to have the cache at all for now and just always invoke getReachablePeers on peerStore to return peers requested.

But looks like the peer-exchange is not implemented and there is a deviation from the spec and that needs to be fixed and would be taken up as a issue.

[chaitanyaprem]

review and fix this logic as this doesn't seem right :

• we should use px-client to get peers irrespective of whether discv5 is enabled or not, so the peerExchangeLoop has to be started even if discv5 is enabled

nwaku/waku/factory/node_factory.nim

Lines 471 to 484 in 6958eac

	# retrieve px peers and add the to the peer store
	if conf.remotePeerExchangeNode.isSome():
	var desiredOutDegree = DefaultPXNumPeersReq
	if not node.wakuRelay.isNil() and node.wakuRelay.parameters.d.uint64() > 0:
	desiredOutDegree = node.wakuRelay.parameters.d.uint64()
	(await node.fetchPeerExchangePeers(desiredOutDegree)).isOkOr:
	error "error while fetching peers from peer exchange", error = error
	# TODO: behavior described by comment is undesired. PX as client should be used in tandem with discv5.
	#
	# Use px to periodically get peers if discv5 is disabled, as discv5 nodes have their own
	# periodic loop to find peers and px returned peers actually come from discv5
	if conf.peerExchangeDiscovery and not conf.discv5Conf.isSome():
	node.startPeerExchangeLoop()

[Ivansete-status]

Adding @jm-clius 's comments from Discord thread

(1) the cache can be completely removed for now as it serves no purpose. This should fix your problem as you'll receive peers that the node updates in the peer store, which is refreshed more often than once in 10 mins.
(2) longer term the protocol should be fixed, as per Prem's issue.
...
Indeed. In the short term this won't change the security model of the implementation, as the cache is just created from the peer store (directly) in any case, but creates indirection and grows stale quicker. However, in the long term we don't want Peer Exchange to interact with the Peer Store at all, but to do lookups directly via discv5 (Prem's issue)

---

Next steps:

• Fix this issue, i.e., not use cache at all in peer-exchange. Instead, retrieve the nodes right from local PeerStore.
• Tackle Prem's issue so that peer-exchange doesn't interact with local PeerStore