rust-libp2p icon indicating copy to clipboard operation
rust-libp2p copied to clipboard

Kademlia Client mode

Open MarcoPolo opened this issue 3 years ago • 7 comments

Context

Continuation of https://github.com/libp2p/rust-libp2p/pull/2184 and fixes https://github.com/libp2p/rust-libp2p/issues/2032.

See https://github.com/libp2p/specs/blob/master/kad-dht/README.md#client-and-server-mode for the specs on client mode.

Most of this diff is tests :)

Warning

This changes the behavior of Kademlia's handler's inject_fully_negotiated_inbound. Namely before this change we assumed the dialer spoke the Kademlia protocol, but now we don't. This is because we don't know if the dialer is a client and therefore would not respond to our request or a server and would.

Before we merge this change I think we should make some other change that would let a node know if another node supports kademlia. Currently you'd have to wire up the identify behavior and manually call add_address.

MarcoPolo avatar Feb 16 '22 03:02 MarcoPolo

For those interested, this is now deployed on kademlia-exporter.max-inden.de/. See dashboard below to compare with vanilla libp2p v0.44.0.

https://kademlia-exporter.max-inden.de/d/Pfr0Fj6Mk/rust-libp2p?orgId=1&var-data_source=Prometheus&var-instance=kademlia-exporter-ipfs-client-mode:8080&var-instance=kademlia-exporter-ipfs:8080

mxinden avatar Mar 05 '22 20:03 mxinden

What else shall we do for this pr merged into master?

Fomalhauthmj avatar May 16 '22 03:05 Fomalhauthmj

I wrote in the PR description:

Before we merge this change I think we should make some other change that would let a node know if another node supports kademlia. Currently you'd have to wire up the identify behavior and manually call add_address.

I think this is still true. Right now if we merge this change we could negatively impact the routing table of servers. Since before they would add a peer when the peer reached out, now they would only do that on dial out or if the peer was manually added. Of course the peer could have been unreachable anyways (say behind a nat) so maybe it's a wash. Still it would be good to have a mechanism that lets the old behavior keep working, instead of changing it completely.

I have some ideas on the change that would be required to get this working. And I'll make an issue that outlines it so we can track the blocker properly.

MarcoPolo avatar May 16 '22 18:05 MarcoPolo

I have some ideas on the change that would be required to get this working. And I'll make an issue that outlines it so we can track the blocker properly.

Corresponding issue based on a past meeting with @MarcoPolo: https://github.com/libp2p/rust-libp2p/issues/2680

@MarcoPolo and @ all additional input is welcome.

mxinden avatar May 30 '22 13:05 mxinden

Thanks Max, that's exactly what I had in mind :)

MarcoPolo avatar Jun 01 '22 00:06 MarcoPolo

Any updates on this or what would need need to be done?

dariusc93 avatar Jul 10 '22 20:07 dariusc93

We would want to do https://github.com/libp2p/rust-libp2p/issues/2680 first in order to address the "warning" section in the initial pull request description. @dariusc93 let me know if that (a) makes sense and (b) whether you would be interested working on any of it.

mxinden avatar Jul 11 '22 03:07 mxinden

Setting this to draft until we can proceed.

thomaseizinger avatar Nov 02 '22 04:11 thomaseizinger

This pull request has merge conflicts. Could you please resolve them @MarcoPolo? 🙏

mergify[bot] avatar Mar 22 '23 15:03 mergify[bot]

Closing this as we will likely ship this in a different PR, thanks kicking it off @MarcoPolo !

See https://github.com/libp2p/rust-libp2p/pull/3651 and related PRs for further progress!

thomaseizinger avatar Apr 28 '23 10:04 thomaseizinger