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

Add a KademliaHandler #580

Merged
merged 16 commits into from
Nov 29, 2018
Merged

Add a KademliaHandler #580

merged 16 commits into from
Nov 29, 2018

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Oct 19, 2018

Adds a KademliaHandler that generates Kademlia requests and responses.

Needs some improvements before merging.

Based on #573

@tomaka tomaka added the on-ice label Oct 19, 2018
@ghost ghost assigned tomaka Nov 19, 2018
@tomaka tomaka force-pushed the kad-proto-handler branch 13 times, most recently from e1519f6 to 34dcaed Compare November 24, 2018 13:07
@miketang84
Copy link
Contributor

Good work.

@tomaka tomaka removed the on-ice label Nov 28, 2018
@tomaka tomaka changed the title [WIP] Add a KademliaHandler Add a KademliaHandler Nov 28, 2018
@tomaka
Copy link
Member Author

tomaka commented Nov 28, 2018

This is ready for review.

While a lot of details should be fleshed out, I'd really like to get this in soon-ish, as Kademlia is still totally non-working at the moment.

Notable changes:

  • Adds a Topology generic to NetworkBehaviour. It is passed by mutable reference in NetworkBehaviour::poll(). Each NetworkBehaviour implementation can add additional requirements on the topology they accept.
  • Touches a bit the Topology trait and the MemoryTopology struct.
  • Splits the Kadmelia messages in two: requests and responses.
  • Rewrote query.rs to be more poll-oriented and more generic.

use std::vec;

/// Trait allowing retreival of information necessary for the Kadmelia system to work.
pub trait KademliaTopology: Topology {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the Topology constraint? KademliaTopology does not depend on anything in Topology.

Copy link
Member Author

@tomaka tomaka Nov 28, 2018

Choose a reason for hiding this comment

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

The Kademlia behaviour needs to be able to call addresses_of_peer on the topology in order to answer Kademlia requests from remotes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, by requiring both bounds: where TTopology: Topology + KademliaTopology

Copy link
Member Author

Choose a reason for hiding this comment

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

KademliaTopology is a trait specifically for topologies that can be used by the Kademlia behaviour, hence why it depends on Topology.

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise, by following that logic, we should have four traits, AddKadDiscoveredAddress, ClosestPeer, AddProvider and GetProviders, and we would require AddKadDiscoveredAddress + ClosestPeer + AddProvider + GetProviders + Topology.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the suggestion to use A + B + C + D would have come up only if B: A, C: B, D: C already existed.

Copy link
Contributor

@twittner twittner left a comment

Choose a reason for hiding this comment

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

> git diff --check master 
examples/ipfs-kad.rs:53: trailing whitespace.
+    
protocols/kad/src/protocol.rs:475: trailing whitespace.
+    
protocols/kad/src/protocol.rs:511: trailing whitespace.
+    
protocols/kad/src/protocol.rs:515: trailing whitespace.
+    
protocols/kad/src/protocol.rs:518: trailing whitespace.
+    
protocols/kad/src/protocol.rs:523: trailing whitespace.
+    
protocols/kad/src/protocol.rs:536: trailing whitespace.
+    
protocols/kad/src/protocol.rs:538: trailing whitespace.
+    

protocols/kad/src/topology.rs Outdated Show resolved Hide resolved
if peer_id == result_source {
match state {
state @ QueryPeerState::InProgress(_) => *state = QueryPeerState::Succeeded,
_ => (),
Copy link
Contributor

Choose a reason for hiding this comment

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

if let?

@tomaka tomaka merged commit 3aa1fcb into libp2p:master Nov 29, 2018
@tomaka tomaka deleted the kad-proto-handler branch November 29, 2018 11:11
@ghost ghost removed the in progress label Nov 29, 2018
dvdplm added a commit to dvdplm/rust-libp2p that referenced this pull request Dec 4, 2018
* upstream/master:
  substream -> substreams (libp2p#720)
  Enhance the swarm a bit (libp2p#711)
  multiaddr: change UDP constant from 17 to 273. (libp2p#714)
  Add &message.source in println! as per … (libp2p#705)
  Use UpgradeError::into_io_error (libp2p#709)
  Revamp the documentation of the root of core (libp2p#684)
  Fix core-derive (libp2p#707)
  Chore/grammar (libp2p#701)
  Add a KademliaHandler (libp2p#580)
  Add 'of' (libp2p#700)
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

Successfully merging this pull request may close these issues.

3 participants