protocols/kad: Add Kademlia::get_closest_local_peers

[folex]

Hi! I'm developing a project where a need came for a network discovery on top of the Kademlia. Here's an example fluencelabs/quickstart/oracle.

For this, I expose API to gather Kademlia's k-closest peers. However, exposing get_closest_peers to users isn't secure: it may (it sure does, I've tested 🥲) lead to amplification attack for obvious reasons. So, I ended up using Kademlia's local routing table for that, and it's working great for my purposes.

However, maintaining a GitHub fork of libp2p is tedious. So it would be great to have this change upstream so I can use libp2p from crates.io instead of a GitHub fork. And I hope that may be useful for other people out there.

And that's what this PR is about: it adds a simple single-line method that runs ClosestBucketsIter over local kbuckets and returns it as iterator.

Thanks in advance!

[mxinden]

Sorry for the late review.

Do I understand correctly that Kademlia::kbucket is not enough for your use-case?

[mxinden]

Suggested change

	key: &'k kbucket::Key<K>,
	key: K,
	// ...
	where
	K: Into<kbucket::Key<K>> + Into<Vec<u8>> + Clone,

Why not mirror the signature of get_closest_peers?

[folex]

Because we have to pass key as a reference, so function can return an impl Iterator that would use that reference.

We can't pass key by value, because then fn get_local_closest_peers will own it and drop, thus invalidating impl Iterator. Borrow checker would be unhappy about that :)

[folex]

It's possible to do it this way:

    pub fn get_local_closest_peers<'s, 'k: 's, K: 's>(
        &'s mut self,
        key: &'k K,
    ) -> impl ...
    where
        K: AsRef<kbucket::Key<K>> + Clone,
    {
        self.kbuckets.closest_keys(key.as_ref())
    }

Do you find that better than the current version?

P.S. Into<Vec<u8>> isn't required for get_local_closest_peers

[folex]

On a second thought, K: AsRef<kbucket::Key<K>> + Clone doesn't make sense. It does make sense to do K: Into<Key<K>>, cuz then if refers to the following From impls:

impl From<Multihash> for Key<Multihash>
impl From<PeerId> for Key<PeerId>
impl From<Vec<u8>> for Key<Vec<u8>>

But since there aren't any analogous AsRef<Key<_>> impls, the bound K: AsRef<kbucket::Key<K>> doesn't make sense.

[folex]

So I see best to stay with key: &'k kbucket::Key<K> and rename K to T for naming consistency. WDYT?

Did that. Now it looks like this:

pub fn get_local_closest_peers<'s, 't: 's, T: 's>(
    &'s mut self,
    key: &'t kbucket::Key<T>,
) -> impl ...
where
    T: Clone,
{
    self.kbuckets.closest_keys(key)
}

[folex]

Do I understand correctly that Kademlia::kbucket is not enough for your use-case?

That's right. It doesn't follow the XOR-metric sorting, so I can't have a consistent set of peers for a key.

[mxinden]

Thanks for the explanation above on key needs to be borrowed.

How about the following?

    /// Returns closest peers to the given key; takes peers from local routing table only.
    pub fn get_closest_local_peers<'a, K: Clone>(
        &'a mut self,
        key: &'a kbucket::Key<K>,
    ) -> impl Iterator<Item = kbucket::Key<PeerId>> + 'a {
        self.kbuckets.closest_keys(key)
    }

• I think it is fine to keep referring to the generic key as K.
• Let's not mix trait bound listing in the initial <T: XXX> and a where clause.
• I don't think the lifetime on T (now K) is needed. In other words, the lifetime only needs to be mentioned on the reference (&'a)
• Let's rename it to get_closest_local_peers
• What does the lifetime parameter s stand for? If it is just a random letter, I would prefer 'a as that seems to be the Rust convention and mirrors the lifetime parameters used in closest_keys.

Let me know what you think @folex.

[folex]

Thanks for the thorough comments!

What does the lifetime parameter s stand for

Lifetime name 's was a short for self. Though I think 'a suits great as well 👍

Updated to reflect all your comments.

[mxinden]

Looks good to me. Final ask (I promise) would be to add a changelog entry in protocols/kad/CHANGELOG.md. Sorry for the many back and forths.

[folex]

Sorry for the many back and forths.

It took me almost a year to submit this PR, so I'm really happy with review timings 😅