kad: FIND_NODE not conform to specs

[guillaumemichel]

Summary

According to kad specs, FIND_NODE is expected to return the closest nodes to the requested key. Currently, receiving a FIND_NODE request for self (node's own key) results in an empty response. Because FIND_NODE is expected to return the closest nodes, the closest nodes to self should also be returned in the response.

IMO it isn't necessary to include self in the response though, because the requester is already aware of it.

Related issue: libp2p/go-libp2p-kad-dht#966

Expected behavior

When a FIND_NODE request for self (node's own key) is received, the response should contain the closest known peers.

Actual behavior

When a FIND_NODE request for self (node's own key) is received, an empty response is returned.

rust-libp2p/protocols/kad/src/behaviour.rs

Lines 1183 to 1184 in a579e69

	if target == self.kbuckets.local_key() {
	Vec::new()

Relevant log output

No response

Possible Solution

Don't return an empty response, but rather the closest peers to the requested key (excluding self).

Version

No response

Would you like to work on fixing this bug ?

Yes

[dariusc93]

This was discussed here and here. I think if the latter was discussed more that could provide more clarification not just to rust-libp2p but other implementations as well but in my opinion, I dont believe it really make much sense to be able to find one self when calling FIND_NODE. I havent tested it recently but i know in go-libp2p i was not able to do the same when I tried it so did something change in the go implementation for this?

[dennis-tra]

IMO it can make sense that the peer returns itself. The response could include additional multiaddresses that the requestor isn't aware of yet. For example, the peer record could include a multiaddress that allows a direct connection while the requestor and requestee are connected through a relay.

You could argue that this is already covered by the identify protocol though 🤷‍♂️ personally I don't find this implicit dependency on another protocol a super compelling argument to not do the suggested change.

[guillaumemichel]

Currently go-libp2p-kad-dht is returning only itself when its own key is requested through FIND_NODE, which isn't either conform to the libp2p kad specs.

The question isn't: should the node return itself to such queries. It would be silly, because the requester already know the peer it is sending the request to.

My main concern here is that nodes should return the closest nodes to the requested key (excluding themselves, and the requester). FIND_NODE should never return an empty response, unless the routing table of the node is empty.

So both rust-libp2p and go-libp2p-kad-dht should should return the list of their closest peers, when receiving a FIND_NODE request for their own id.

[dennis-tra]

It would be silly, because the requester already know the peer it is sending the request to.

As mentioned above, I think there could be a valid reason why you would want the latest peer record

[guillaumemichel]

@mxinden do you know why the empty response was introduced in ef9cb05?

[mxinden]

I don't unfortunately @guillaumemichel.

[guillaumemichel]

The spec says:

The libp2p Kademlia DHT offers the following types of operations:

• Peer routing
  • Finding the closest nodes to a given key via FIND_NODE.

3. Upon a response:
   1. If successful the response will contain the k closest nodes the peer knows to the key Key. Add them to the candidate list Pn, except for those that have already been queried.

So the response should contain k peers (including/excluding self). An empty response isn't conform to specs

[dhuseby]

@achingbrain did you check the js-libp2p behavior to see how it behaves?

[achingbrain]

Sorry - I missed this. It copies the go-libp2p-kad-dht behaviour as that was what was understood to be correct at the time.

This issue has an analogue here: libp2p/js-libp2p#2450 - the fix is here: libp2p/js-libp2p#2499

[hanabi1224]

Thanks for the fix! Is there a planned release of libp2p-kad anytime soon?

[dariusc93]

Thanks for the fix! Is there a planned release of libp2p-kad anytime soon?

👋 To my knowledge this will be released after #4568 is completed (which may prompt 0.54 release, cc @jxs). Of course, this could be backported too.

[hanabi1224]

@dariusc93 Thanks for the info! BTW, does rust-libp2p accept cross-language compatibility/regression tests? If so I can port ChainSafe/forest#4622 to rust-libp2p. Thanks!

[jxs]

Hi @hanabi1224 I'd suggest to instead turn that into an interop test wdyt? cc @dhuseby