Kad-GetProviders doesn't work on two nodes

[mikevoronov]

It seems that getproviders in libp2p-kad doesn't work for two nodes. From code here it could be obtained that provider_peers tries to find providers only in the buckets, but buckets aren't contain any info about a current node... So it works perfectly on >2 nodes, but doesn't on 2.

[mxinden]

@michaelvoronov would you mind providing a unit test that reproduces the failure above?

[BlackYoup]

@mxinden I would be happy to provide such tests as I just had the issue when trying to adapt the distributed-key-value-store example to put providers instead of putting values :)

I'm not very familiar with the code base, where would it make more sense? Would protocols/kad/src/behaviour/test.rs be ok?

[mikevoronov]

Sorry, I missed notification, we've switched to put/get functionality, but I think this issue could be obtained from the source code.

[mxinden]

@BlackYoup either in protocols/kad/src/behaviour/test.rs or an example along the lines of https://github.com/libp2p/rust-libp2p/blob/master/examples/distributed-key-value-store.rs. Thanks!

@michaelvoronov I am still having difficulties following your above reasoning.

[BlackYoup]

@mxinden I opened #1704 which updates the distributed-key-value-store example. With this, if you run it in two terminals:

• Terminal 1: PUT_PROVIDER foo
• Terminal 2: GET_PROVIDERS foo

Nothing will be printed in terminal 2. If you add a log when the QueryResult::GetProviders(Ok(ok)) is called, you'll see that the providers hashset is empty. It works fine if you use three terminals.

[mikevoronov]

@mxinden Let's consider two nodes (1 and 2) in a network that know about each other. 1 starts providing some key and then 2 calls get providers for this key that will trigger provider_peers on the 1. This functions looks like this:

    fn provider_peers(&mut self, key: &record::Key, source: &PeerId) -> Vec<KadPeer> {
        let kbuckets = &mut self.kbuckets;
        self.store.providers(key)
            .into_iter()
            .filter_map(move |p|
                if &p.provider != source {
                    let key = kbucket::Key::new(p.provider.clone());
                    kbuckets.entry(&key).view().map(|e| KadPeer::from(e.to_owned()))
                } else {
                    None
                })
            .take(self.queries.config().replication_factor.get())
            .collect()
    }

From this code it could be obtained that info about providers is collecting only from kbuckets: kbuckets.entry(&key).view().map(|e| KadPeer::from(e.to_owned())).

But kbuckets don't contain a section for the self node (1). So provider_peers returns empty Vec for such setup.

[mxinden]

Thanks for the easy way to reproduce this @BlackYoup. Thanks @michaelvoronov for catching the bug down all the way to provider_peers!

Even though this is a bit of an edge case (2 node cluster) I am still of the opinion that it is worth fixing.

@BlackYoup @michaelvoronov would one of you like to tackle this in a pull request? I took a couple of notes to describe how I would approach the issue.

diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs
index dc06a444..5b9bccfd 100644
--- a/protocols/kad/src/behaviour.rs
+++ b/protocols/kad/src/behaviour.rs
@@ -790,6 +790,13 @@ where
                 if &p.provider != source {
                     let key = kbucket::Key::new(p.provider.clone());
                     kbuckets.entry(&key).view().map(|e| KadPeer::from(e.to_owned()))
+                } else if &p.provider == self.kbuckets.local_key().preimage() {
+                    // Create and return a KadPeer of ourself. Within `provider_peers` we are not
+                    // aware of our `Multiaddr`s. It might be good enough to only return our
+                    // `PeerId` (a bit hacky) as the remote obviously already knows how to contact
+                    // us. Otherwise we could enrich the event in `<Kademlia as
+                    // NetworkBehaviour>::poll` as the argument `parameters` contains our external
+                    // addresses via `parameters.external_addresses`.
                 } else {
                     None
                 })