search

libp2p / go-libp2p-kad-dht

A Kademlia DHT implementation on go-libp2p
https://github.com/libp2p/specs/tree/master/kad-dht
MIT License
524 stars 224 forks source link

Invalid logic in routing.go #378

Closed Warchant closed 5 years ago

Warchant commented 5 years ago

Found something interesting.

https://sourcegraph.com/github.com/libp2p/go-libp2p-kad-dht/-/blob/routing.go#L607-620

FindLocal is essentially the same as dht.peerstore.PeerInfo(id).

Then dht.routingTable.NearestPeers(kb.ConvertPeerID(id), AlphaValue) is executed. It does something, but it definitely does not modify peerstore.

Then, if id is found among nearest peers, we go to peerstore again and execute dht.peerstore.PeerInfo(id) again. But previous call did not find addresses, and NearestPeers did not modify peerstore, so what do we expect to get here?:)

It will also return empty PeerInfo.

This execution path returns no error, and at the same time returns empty PeerInfo and in my opinion, wrong.

Stebalien commented 5 years ago

I believe this is trying to handle a race condition:

  1. We check (FindLocal) to see if we're connected to the target peer, we aren't.
  2. We query the routing table for the nearest peers to ID.
  3. We find the peer we just checked for in the routing table. This means we were connected to them when calling NearestPeers.
  4. We return the peer info for that peer which shouldn't be empty because that peer was just in our routing table.

But we should probably do an additional sanity check to make sure that the peer info isn't empty, just in case.