FindPeer Design Review

[aschmahmann]

There are a few upcoming changes to the DHT and we'd like to nail down what the contract and expectations are for the FindPeer query to make sure they are met from a performance and security standpoint without adding too much complexity.

Questions for the upcoming design review include:

• Do we want FindPeer or ConnectPeer? Do we really want the address or just a connection?
• Should we run a full query looking for the target peer's addresses, or be async/allow early aborting if we find a single address?
  • Full query could be running GetClosestPeers(target) and then asking them each for the addresses they know about
  • Aborting early just stops once we get a single address
  • If doing async do we continue the query while we're trying to verify the address, or pause?
• Do we want peers behind NATs advertising their addresses in the DHT?
  • Relates to Ensure DHT clients remain discoverable #779

Existing implementation approaches

Current go-libp2p-kad-dht approach:

• Return multiaddrs
• Return as soon as we find a single address for the target peer
  • Should be a good address since peers should only return addresses of peers they are connected to
• Records are not signed and we could be told any address at all
  • Could be resolved once Signed Peer Records #776 lands

libp2p/go-libp2p-kad-dht#436:

• Connect to the peer we're looking for
• Return once we've connected to the peer
  • Query continues up until we've connected to the peer
  • If a particular dial to the target peer fails, we'll try again if we learn about them again through the query
    • Having address (instead of peerID) based backoff in the dialer would help here (although it could be handled within Kad if needed)

[aschmahmann]

Some results from this design review:

1. Once signed peer records land FindPeer should effectively run GetClosestPeers and return the best of all the records found
2. Add a FindPeerAsync function that returns a channel of peer records as we find them, but otherwise continues the query
3. Change FindPeer to not rely on the FindPeer DHT message since that message should only return DHT server nodes (i.e. nodes in the routing table), but currently returns DHT client nodes also in order to make their addresses findable

Hopefully landing 1 should be doable without any massive changes, and if so then should push 2 and 3 further down the road.

[aschmahmann]

@aarshkshah1992 asked in #779 about whether FindProviders should also not shortcut as FindPeer has been decided not to.

Shortcutting can lead to two issues:

1. We only learn about old addresses/provider records due to caching inside of Kad (or an attacker giving you a set of bogus provider records) and therefore can't find some content that's really popular
   • This could potentially be an issue, and so probably FindProviders should default to finishing the query instead of shortcutting after finding k records
   • On the other hand provider records are currently forgeable anyway which can lead to all sorts of problems, so until we tackle that maybe it's worth the performance gain to keep shortcutting
2. We find and dial old addresses first as the outputs of FindProvidersAsync, then when we try and dial the new addresses we get a dial backoff
   • It's my understanding that 2 should be covered by Improve Dialing #781

It's possible that the DHT improvements will yield a full search not being much more expensive than shortcutting. If, after benchmarking, the results are close I'd definitely be pro removing the shortcutting by default since we have FindProvidesAsync and the query is cancellable via context.

@Stebalien @jacobheun does this analysis make sense to you and seem like a reasonable plan forward? It's a pretty minimal implementation change (famous last words 😛) which we can revisit once we've got better data about how valuable the shortcuts could be.