Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bootstrap process not compatible with rust libp2p-kad #966

Closed
xinaxu opened this issue Mar 22, 2024 · 6 comments
Closed

Bootstrap process not compatible with rust libp2p-kad #966

xinaxu opened this issue Mar 22, 2024 · 6 comments

Comments

@xinaxu
Copy link

xinaxu commented Mar 22, 2024

In Pull Request #820, additional validation is added to check if the bootstrap node should be put into the routing table - if it fails to return itself for finding itself, then it won't be put into the routing table

return fmt.Errorf("peer %s failed to return its closest peers, got %d", p, len(peerids))

The logic indicates, when querying FIND_NODE for the destination peer, it is expecting the destination node to return at least themselves.

However, the logic in rust libp2p-kad does not include themselves in the response. The result will be, a golang DHT client cannot use a rust DHT client for bootstrapping.
https://github.com/libp2p/rust-libp2p/blob/a579e691c46a8efd2e38ebafca1c59a9b58fc56c/protocols/kad/src/behaviour.rs#L1184

The error shown is

2024-03-22T12:10:47.251-0700    ERROR   dht     go-libp2p-kad-dht/dht.go:694    connected peer not answering DHT request as expected    {"peer": "12D3KooWPRvdxAyaoq6hqBxCZkFknNW7k4aogJtQqWrQENmBEZWx", "error": "peer 12D3KooWPRvdxAyaoq6hqBxCZkFknNW7k4aogJtQqWrQENmBEZWx failed to return its closest peers, got 0
"}
@xinaxu
Copy link
Author

xinaxu commented Mar 22, 2024

@guillaumemichel
@Jorropo

@Jorropo
Copy link
Contributor

Jorropo commented Mar 23, 2024

Right I overlooked the situation where you creating a new DHT from thin air.
Can this happen if rust-libp2p is bootstrapped ? (afait no because it would return other peers too)

I wanted to use the DHT Ping RPC query (for performance reasons), but it is deprecated and we didn't went that route, that could solve issues like this.

I also still think this kind of hardening and edge cases should be written down in a spec but there were no interest last time I had discussions about this.

@dennis-tra
Copy link
Contributor

As I understand it, this is a general problem, even if rust-libp2p is bootstrapped. The issue is just that rust-libp2p never returns itself and hence the validation never passes.

As I see it, the peer itself is the only entry in its highest bucket, so it makes sense to return itself if asked for peers in that highest bucket. Therefore, I think, the better solution would be that rust-libp2p just returned itself.

@guillaumemichel
Copy link
Contributor

guillaumemichel commented Mar 26, 2024

The specs specify that FIND_NODE should return the closest nodes to the requested key. Hence, both go-libp2p-kad-dht and rust-libp2p don't follow the specs.

When requested for self's key:

  • go-libp2p-kad-dht only returns itself
  • rust-libp2p doesn't return any peer

Implicitly, a node wouldn't have to return itself (because the requester is already aware), but it should return a list of its closest peers.


@xinaxu is right, it is an issue that go-libp2p-kad-dht nodes don't accept rust-libp2p nodes in their buckets. A quick fix could be to modify the lookupCheck to request one's own key, instead of requesting the remote node's key.

What really matters is that the remote node is able to provide at least 1 peer in response to FIND_NODE.

@guillaumemichel
Copy link
Contributor

Just suggested a change in rust-libp2p to address the issue

See libp2p/rust-libp2p#5269 and libp2p/rust-libp2p#5270

@guillaumemichel
Copy link
Contributor

libp2p/rust-libp2p#5270 has been merged. It makes sure that rust-libp2p returns multiple peers when being requested for its own peer id. It used to return 0 peers in the past, which is why the rust-libp2p nodes were never added to go-libp2p-kad-dht buckets.

Also see: #968

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants