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

Kademlia Client mode #2521

Closed
wants to merge 45 commits into from

Conversation

MarcoPolo
Copy link
Contributor

Context

Continuation of #2184 and fixes #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.

@mxinden
Copy link
Member

mxinden commented Mar 5, 2022

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

@Fomalhauthmj
Copy link

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

@MarcoPolo
Copy link
Contributor Author

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.

@mxinden
Copy link
Member

mxinden commented May 30, 2022

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: #2680

@MarcoPolo and @ all additional input is welcome.

@MarcoPolo
Copy link
Contributor Author

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

@dariusc93
Copy link
Member

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

@mxinden
Copy link
Member

mxinden commented Jul 11, 2022

We would want to do #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.

@thomaseizinger
Copy link
Contributor

Setting this to draft until we can proceed.

@mergify
Copy link
Contributor

mergify bot commented Mar 22, 2023

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

@thomaseizinger
Copy link
Contributor

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

See #3651 and related PRs for further progress!

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 this pull request may close these issues.

protocols/kad: Support client mode
5 participants