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.

Sign up for GitHub

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

protocols/kad: Add Kademlia::get_closest_local_peers #2436

Merged
merged 15 commits into from
Jan 22, 2022
Merged

protocols/kad: Add Kademlia::get_closest_local_peers #2436

Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e0bf30c
Add local_closest_peers
folex Jan 14, 2022
dbe55b2
Merge branch 'master' into local_closest_peers
folex Jan 14, 2022
5ea5247
Merge branch 'master' into local_closest_peers
folex Jan 14, 2022
7f483c7
Merge branch 'master' into local_closest_peers
folex Jan 17, 2022
703d710
rustfmt, simplify type bound
folex Jan 18, 2022
bd6b4c3
Merge branch 'master' into local_closest_peers
folex Jan 18, 2022
2785d16
further simplify type bound
folex Jan 18, 2022
4d26886
rustfmt
folex Jan 18, 2022
ca50148
get_closest_local_peers
folex Jan 18, 2022
6479ac4
get_local_closest_peers: rename K to T
folex Jan 18, 2022
d085312
rustfmt
folex Jan 18, 2022
a992756
Merge branch 'master' into local_closest_peers
folex Jan 19, 2022
300df3d
Merge branch 'master' into local_closest_peers
folex Jan 21, 2022
2c2596e
Attempt to use same lifetime for self and key
folex Jan 21, 2022
12f645c
Update kad/CHANGELOG.md
folex Jan 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions protocols/kad/src/behaviour.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,17 @@ where
self.queries.add_iter_closest(target.clone(), peers, inner)
}

/// Returns closest peers to the given key; takes peers from local routing table
folex marked this conversation as resolved.
Show resolved Hide resolved
pub fn local_closest_peers<'s, 'k: 's, K: 's>(
folex marked this conversation as resolved.
Show resolved Hide resolved
&'s mut self,
key: &'k kbucket::Key<K>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@folex folex Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

@folex folex Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@folex folex Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

) -> impl Iterator<Item = kbucket::Key<PeerId>> + 's
where
K: Clone,
{
self.kbuckets.closest_keys(key)
}

/// Performs a lookup for a record in the DHT.
///
/// The result of this operation is delivered in a
Expand Down