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

New get_closest_peers signature prevents me to use arbitrary key (as was available in 0.31 version) #1898

Closed
izderadicka opened this issue Dec 18, 2020 · 5 comments

Comments

@izderadicka
Copy link
Contributor

Hi,
in 0.33 version kad method get_closest_peers changed signature:

pub fn get_closest_peers<K>(&mut self, key: K) -> QueryId
    where
        K: Into<kbucket::Key<K>> + Into<Vec<u8>> + Clone

With this signature I'm struggling how to submit arbitrary key - like record::Key or Vec<8>. From for kbucket::Key is implemented only for PeerId and Multihash. Is this intentional? How to use now with arbitrary key?

@mxinden
Copy link
Member

mxinden commented Jan 4, 2021

First off @izderadicka do I understand correctly that you are using get_closest_peers to find a node closest to a given key, but retrieve the actual value corresponding to the key out-of-band i.e. with something other than libp2p-kad?

For the record this change was introduced in #1874, you can find more details in #1874 (comment).

Off the top of my head I would suggest adding a From<Vec<u8>> for Key<Vec<u8>>:

diff --git a/protocols/kad/src/kbucket/key.rs b/protocols/kad/src/kbucket/key.rs
index e8d1af3e..7768d99c 100644
--- a/protocols/kad/src/kbucket/key.rs
+++ b/protocols/kad/src/kbucket/key.rs
@@ -111,6 +111,16 @@ impl From<PeerId> for Key<PeerId> {
     }
 }
 
+impl From<Vec<u8>> for Key<Vec<u8>> {
+    fn from(v: Vec<u8>) -> Self {
+        let bytes = KeyBytes(Sha256::digest(v.as_ref()));
+        Key {
+            preimage: v,
+            bytes
+        }
+    }
+}

That would allow you to call get_closest_peers with a Vec<u8>:

diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs
index 5cfd12ca..97105e78 100644
--- a/protocols/kad/src/behaviour.rs
+++ b/protocols/kad/src/behaviour.rs
@@ -560,6 +560,15 @@ where
     ///
     /// The result of the query is delivered in a
     /// [`KademliaEvent::QueryResult{QueryResult::GetClosestPeers}`].
+    ///
+    /// ```
+    /// use libp2p_kad::{store::MemoryStore, Kademlia};
+    /// use libp2p_core::PeerId;
+    /// let peer_id = PeerId::random();
+    /// let store = MemoryStore::new(peer_id);
+    /// let mut behaviour = Kademlia::new(peer_id, store);
+    /// behaviour.get_closest_peers(vec![1, 2, 3, 4, 5]);
+    /// ```
     pub fn get_closest_peers<K>(&mut self, key: K) -> QueryId

@romanb given that you wrote most of this logic, does the above sound reasonable to you?

@izderadicka
Copy link
Contributor Author

@mxinden purpose was somehow similar - for testing and diagnostics - get closes peers for given key and then explore them.

@romanb
Copy link
Contributor

romanb commented Jan 4, 2021

This looks like an oversight. There should be From<record::Key> and From<Vec<u8>> instances for kbucket::Key that just delegate to kbucket::Key::new.

@mxinden
Copy link
Member

mxinden commented Jan 5, 2021

@izderadicka do you want to prepare a pull request?

@mxinden
Copy link
Member

mxinden commented Jan 11, 2021

Fixed with #1909.

@mxinden mxinden closed this as completed Jan 11, 2021
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

No branches or pull requests

3 participants