Support client mode in kademlia #2184

whereistejas · 2021-08-07T07:18:43Z

Fix #2032

Feature description:

To do this, we use libp2p's AutoNAT, which acts as a distributed session traversal utility for NAT (STUN) layer, informing peers of their observed addresses and whether or not they appear to be publicly dialable. Only when peers detect that they are publicly dialable do they switch from client mode (where they can query the DHT but not respond to queries) to server mode (where they can both query and respond to queries). Similarly, if a server discovers that it is no longer publicly dialable, it will switch back into client mode.

From https://docs.ipfs.io/concepts/dht/#undialable-peers

mxinden

There is still a lot missing here. Let me know if you need more details. Also see my comment #2032 (comment).

protocols/kad/src/behaviour.rs

protocols/kad/src/dht.proto

whereistejas · 2021-08-10T05:28:08Z

Hi @mxinden,
I'm trying to write an integration test to test this enhancement. It's taking longer than anticipated.

whereistejas · 2021-08-15T18:51:04Z

Hi @mxinden,
I have added an example file (sort of like an integration test) for our implementation. The idea is that you run multiple instances of the example in different terminals, and we can run one instance with --client flag. Then, we use the list peer command in the other (non-client) instances to check if the client peer is not present in their routing tables.

Could you give me some pointers about how to proceed from this point?

mxinden

Great steps forward @whereistejas!

In case you are interested in working on specifications as well: It would be great to have the Client Mode feature described in the Kademlia specification.

mxinden · 2021-08-19T08:20:58Z

examples/kademlia-example.rs

@@ -0,0 +1,157 @@
+#![allow(dead_code, unused_variables)]


Instead of an example, could you add a test to kad/src/behaviour/tests.rs?

Writing this test in the kademlia module will be kind of difficult, as I'm using a lot of other things as well, for building the swarm or transport. Is it okay if I put this is in a test directory?

I can further enhance kademlia-example to create four peers and cross-check if the peer in client mode is actually, absent from the other's routing tables.

Writing this test in the kademlia module will be kind of difficult, as I'm using a lot of other things as well, for building the swarm or transport. Is it okay if I put this is in a test directory?

No need for most of it. You can take the manual_bucket_insert and bootstrap test as an example.

I can further enhance kademlia-example to create four peers and cross-check if the peer in client mode is actually, absent from the other's routing tables.

I don't think there is any need for an example. The "cross-check" sounds great, but should really be done in a test instead of an example.

protocols/kad/src/behaviour.rs

protocols/kad/src/handler.rs

mxinden · 2021-08-19T08:41:30Z

protocols/kad/src/handler.rs

+
+    // If Mode::Client, node will act in `client` mode in the Kademlia network.
+    pub client: Mode,


The Mode logic you are introducing in this pull request replaces the KademliaHandlerConfig::allow_listening logic. The latter can thus be replaced in favor of the former 🎉

mxinden · 2021-08-19T08:43:14Z

protocols/kad/src/handler.rs

+        // TODO:
+        // Just because another peer is sending us Kademlia requests, doesn't necessarily
+        // mean that it will answer the Kademlia requests that we send to it.
+        // Thus, Commenting this code out for now.
+        // if let ProtocolStatus::Unconfirmed = self.protocol_status {
+        //     // Upon the first successfully negotiated substream, we know that the
+        //     // remote is configured with the same protocol name and we want
+        //     // the behaviour to add this peer to the routing table, if possible.
+        //     self.protocol_status = ProtocolStatus::Confirmed;
+        // }


Correct. I am in favor of documenting this, though obviously both the TODO and the commented out code needs to be removed.

mxinden · 2021-08-19T08:43:55Z

protocols/kad/src/protocol.rs

@@ -46,6 +46,18 @@ pub const DEFAULT_PROTO_NAME: &[u8] = b"/ipfs/kad/1.0.0";
 /// The default maximum size for a varint length-delimited packet.
 pub const DEFAULT_MAX_PACKET_SIZE: usize = 16 * 1024;

+#[derive(Debug, Clone)]


Suggested change

#[derive(Debug, Clone)]

/// See [`crate::KademliaConfig::set_mode`].

#[derive(Debug, Clone)]

whereistejas · 2021-08-19T09:18:57Z

In case you are interested in working on specifications as well: It would be great to have the Client Mode feature described in the Kademlia specification.

Sure! I can do this.

The current code changes in the kademlia submodule are not producing the desired results. I will take a look at it over the weekend. Do I need to make any other changes? I remember us discussing about another edge case in the multistream module (I maybe completely wrong here).

mxinden · 2021-08-19T09:36:16Z

The current code changes in the kademlia submodule are not producing the desired results. I will take a look at it over the weekend. Do I need to make any other changes? I remember us discussing about another edge case in the multistream module (I maybe completely wrong here).

Nothing comes to my mind. What exactly is not working? I don't think this is related to multistream-select.

whereistejas · 2021-08-23T14:36:39Z

Hi @mxinden, this is what I meant by not working. As you can see in the screenshot, the "other" peers can still see the "client" node.

whereistejas · 2021-08-24T18:40:36Z

Okay, so something really weird is happening. I put some println statements in listen_protocol and inject_fully_negotiated_inbound to check if these methods are getting called when two peers connect to one another and I found that they are not getting called. I am using the code I wrote in examples/kademlia-example.rs to test this. Am I doing something wrong?

Update: I think this is happening because of the following code:

MdnsEvent::Discovered(nodes) => {
    for (peer_id, multiaddr) in nodes {
        swarm
            .behaviour_mut()
            .kademlia
            .add_address(&peer_id, multiaddr);
    }
}

SInce, I'm directly adding addresses, the peers are not performing a protocol negotiation.

protocols/kad/src/behaviour/test.rs

whereistejas · 2021-08-30T18:03:37Z

This screenshot is from my machine.

😢

whereistejas · 2021-09-03T10:28:13Z

So, I guess the CI failure for commit 54331e5 was just a random event.

mxinden · 2021-09-06T13:30:24Z

protocols/kad/src/behaviour/test.rs

+                        if swarm.local_peer_id().clone() == peers[0] {
+                            is_server_present = peer == peers[1];
+                        }
+                        return Poll::Ready(());


I suggest reading the async book: https://rust-lang.github.io/async-book/

With the early return we might not check whether the server stores the client in its routing table.

mxinden · 2021-09-06T13:32:35Z

protocols/kad/src/behaviour/test.rs

+        .collect();
+
+    swarms[0].1.dial_addr(addrs[1].clone()).unwrap();
+    swarms[1].1.dial_addr(addrs[0].clone()).unwrap();


Why does the server dial the client?

It's there to guarantee that server tried to reach out to the client, before we check if the client is absent from the server's routing table.

mxinden · 2021-09-06T13:35:45Z

protocols/kad/src/behaviour/test.rs

+                    Poll::Ready(_) => {
+                        // Check if the client peer is NOT present in the server peer's routing table.
+                        if swarm.local_peer_id().clone() == peers[1] {
+                            is_client_absent = swarm.behaviour_mut().kbucket(peers[0]).is_none();


It is kind of hard to reliably check for absence of something, especially in an async setting.

How about:

The server stores a value Kademlia::put_value, waiting for a KademliaEvent::OutboundQueryCompleted.

The client connects to the server.

The client does a Kademlia::get_record, waiting for a KademliaEvent::OutboundQueryCompleted.

Check that the client has the server in its routing table and check that the server does not have the client in its routing table.

Sure. I will implement this. I will try to finish before the same time, tomorrow. Please, keep an eye out for any notifications from this PR. I want to get this PR closed, as early as possible (ofcourse, without sacrificing code quality).

Are we finished with the other parts of this PR except the test?

whereistejas · 2021-09-10T11:26:19Z

Hi @mxinden, the CI tests are passing on my local machine. Could you take a look at the code, please?

whereistejas · 2021-09-20T09:54:41Z

Hi @mxinden, this PR ready for review. Just a gentle reminder. 😄

mxinden

Good progress here.

Just to make sure, does your test fail without the patch?

.gitignore

mxinden · 2021-09-29T12:22:41Z

protocols/kad/src/behaviour/test.rs

+    cfg.set_mode(Mode::Client);
+    let mut client = build_node_with_config(cfg);
+
+    // Fitler out peer Ids.


Suggested change

// Fitler out peer Ids.

// Filter out peer Ids.

mxinden · 2021-09-29T12:30:00Z

protocols/kad/src/behaviour/test.rs

+                            Ok(PutRecordOk { .. }) => {
+                                // Check if the server peer is not connected to the client peer.
+                                assert!(
+                                    swarm
+                                        .behaviour_mut()
+                                        .connected_peers
+                                        .iter()
+                                        .all(|p| *p != peers[2]),
+                                    "The server peer is connected to the client peer."
+                                );
+                            }


How about calling swarm[2].behaviour_mut().get_record(..) once the first server is done with the PutRecord query. That would prevent any races between the PUT and the GET.

Hi, I have a doubt with the present test we have:

It is kind of hard to reliably check for absence of something, especially in an async setting.

The server stores a value Kademlia::put_value, waiting for a KademliaEvent::OutboundQueryCompleted.

The client connects to the server.

The client does a Kademlia::get_record, waiting for a KademliaEvent::OutboundQueryCompleted.

Check that the client has the server in its routing table and check that the server does not have the client in its routing table.

Are we sure that using the GetRecord operation will update the routing tables of the server peer? I'm trying to find the code, which does the "update"-ing. But, I'm unable to find it. Could you please point me towards the appropriate code?

Update: (Please correct me if I'm wrong) I don't think Kademlia is bi-directional. For example, if peer A adds peer B to its routing table, then peer B will not add peer A to its routing table.

Update: (Please correct me if I'm wrong) I don't think Kademlia is bi-directional. For example, if peer A adds peer B to its routing table, then peer B will not add peer A to its routing table.

Before this pull request both peers would likely add each other to their respective routing table. After this pull request, whether the two peers add each other to their routing table depends on the Mode the other peer is running in.

protocols/kad/src/handler.rs

whereistejas · 2021-09-30T09:38:01Z

The test is not failing without the patch, which is worrying me. I will try to debug and find out why.

mxinden · 2021-10-07T14:35:54Z

protocols/kad/src/behaviour/test.rs

+
+    // Create a client peer.
+    let mut cfg = KademliaConfig::default();
+    cfg.set_mode(Mode::Server);


I am a bit confused. Why is the client initialized with Mode::Server?

Oops, that's just a typing mistake on my side. Anyways, this test should fail when the client is in Server mode, which it is not. I have some doubts regarding the way Kademlia works, which I have expressed here. Could you please help me out with those?

mxinden · 2022-06-27T07:03:55Z

Closing here in favor of #2521.

Tejas Sanap added 4 commits August 7, 2021 12:31

Adding config parameter to support client mode

f8cf486

Merge branch 'master' into kademlia-client-mode

1a042f8

example for showing peer interaction in kademlia

3e759a0

finally able to discover other peers

b8cbe62

mxinden reviewed Aug 9, 2021

View reviewed changes

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved

protocols/kad/src/behaviour.rs Outdated Show resolved Hide resolved

protocols/kad/src/dht.proto Outdated Show resolved Hide resolved

Add Mode enum for kademlia client/server mode

fc218a0

whereistejas force-pushed the kademlia-client-mode branch from 02a0a74 to fc218a0 Compare August 10, 2021 04:35

Tejas Sanap added 6 commits August 10, 2021 11:09

Remove confirmation code from inject_fully_negotiated_inbound

e58e544

Correct client value for network behaviour handler

af1fcbd

Merge branch 'kademlia-client-mode' into kademlia-example

c944ecb

refactoring

4be8fca

Refactoring complete

e213350

Add example for running kademlia in client mode

e47148d

Tejas Sanap added 2 commits August 16, 2021 00:26

Merge commit with master

cdaa0a1

removing .vscode

1a09907

mxinden reviewed Aug 19, 2021

View reviewed changes

Add better documentation and comments

46167a1

Tejas Sanap added 5 commits August 28, 2021 17:19

replaced example with test in kad-behaviour

8bf3ef4

Changes to client mode test

9ef4c4c

remove kad-example from cargo.toml

c387f41

Better checks for client mode test

5b21774

Fix client mode test

5ea645e

whereistejas requested a review from mxinden August 29, 2021 20:26

whereistejas commented Aug 29, 2021

View reviewed changes

protocols/kad/src/behaviour/test.rs Outdated Show resolved Hide resolved

Merge branch 'master' into kademlia-client-mode

9d23741

mxinden reviewed Sep 6, 2021

View reviewed changes

Tejas Sanap added 7 commits September 7, 2021 19:25

Add some basic comments.

b202c5b

Changes to test. Currently failing.

8234a17

Changes to client_mode test. Currently failing.

d5e6509

Correct variable name.

06c1a30

Add checks to see if PUT and GET are working.

d420fa6

Add correct checks for PUT and GET operations.

190c3dd

Merge branch master into kademlia-client-mode

ab97b10

whereistejas requested a review from mxinden September 10, 2021 11:25

Tejas Sanap added 4 commits September 10, 2021 17:07

update prost dependencies

f3259b1

Replace any with all for server peer check.

f308b0c

Merge branch master into kademlia-client-mode

8f33ba1

Extra comments

520fb8e

mxinden reviewed Sep 29, 2021

View reviewed changes

Fix merge conflict with master

d52c6af

Tejas Sanap added 3 commits October 3, 2021 17:39

New test

81eae5f

Remove allow_listening logic.

f0c2fc3

Fix gitignore

f0bf7df

whereistejas requested a review from mxinden October 3, 2021 14:19

elenaf9 mentioned this pull request Oct 5, 2021

protocols/: Add basic AutoNAT implementation #2262

Merged

mxinden reviewed Oct 7, 2021

View reviewed changes

MarcoPolo mentioned this pull request Feb 16, 2022

Kademlia Client mode #2521

Closed

mxinden closed this Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support client mode in kademlia #2184

Support client mode in kademlia #2184

whereistejas commented Aug 7, 2021 •

edited

Loading

mxinden left a comment

whereistejas commented Aug 10, 2021

whereistejas commented Aug 15, 2021 •

edited

Loading

mxinden left a comment

mxinden Aug 19, 2021

whereistejas Aug 19, 2021 •

edited

Loading

mxinden Aug 19, 2021

mxinden Aug 19, 2021

mxinden Aug 19, 2021

mxinden Aug 19, 2021

whereistejas commented Aug 19, 2021 •

edited

Loading

mxinden commented Aug 19, 2021

whereistejas commented Aug 23, 2021

whereistejas commented Aug 24, 2021 •

edited

Loading

whereistejas commented Aug 30, 2021 •

edited

Loading

whereistejas commented Sep 3, 2021

mxinden Sep 6, 2021

mxinden Sep 6, 2021

whereistejas Sep 6, 2021 •

edited

Loading

mxinden Sep 6, 2021

whereistejas Sep 6, 2021 •

edited

Loading

whereistejas commented Sep 10, 2021

whereistejas commented Sep 20, 2021 •

edited

Loading

mxinden left a comment

mxinden Sep 29, 2021

mxinden Sep 29, 2021

whereistejas Oct 3, 2021 •

edited

Loading

mxinden Oct 19, 2021

whereistejas commented Sep 30, 2021 •

edited

Loading

mxinden Oct 7, 2021

whereistejas Oct 7, 2021

mxinden commented Jun 27, 2022


		// If Mode::Client, node will act in `client` mode in the Kademlia network.
		pub client: Mode,

	#[derive(Debug, Clone)]
	/// See [`crate::KademliaConfig::set_mode`].
	#[derive(Debug, Clone)]

Support client mode in kademlia #2184

Support client mode in kademlia #2184

Conversation

whereistejas commented Aug 7, 2021 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

whereistejas commented Aug 10, 2021

whereistejas commented Aug 15, 2021 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereistejas Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereistejas commented Aug 19, 2021 • edited Loading

mxinden commented Aug 19, 2021

whereistejas commented Aug 23, 2021

whereistejas commented Aug 24, 2021 • edited Loading

whereistejas commented Aug 30, 2021 • edited Loading

whereistejas commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereistejas Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereistejas Sep 6, 2021 • edited Loading

Choose a reason for hiding this comment

whereistejas commented Sep 10, 2021

whereistejas commented Sep 20, 2021 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereistejas Oct 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whereistejas commented Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Jun 27, 2022

whereistejas commented Aug 7, 2021 •

edited

Loading

whereistejas commented Aug 15, 2021 •

edited

Loading

whereistejas Aug 19, 2021 •

edited

Loading

whereistejas commented Aug 19, 2021 •

edited

Loading

whereistejas commented Aug 24, 2021 •

edited

Loading

whereistejas commented Aug 30, 2021 •

edited

Loading

whereistejas Sep 6, 2021 •

edited

Loading

whereistejas Sep 6, 2021 •

edited

Loading

whereistejas commented Sep 20, 2021 •

edited

Loading

whereistejas Oct 3, 2021 •

edited

Loading

whereistejas commented Sep 30, 2021 •

edited

Loading