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

Support client mode in kademlia #2184

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 35 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
f8cf486
Adding config parameter to support client mode
Aug 7, 2021
1a042f8
Merge branch 'master' into kademlia-client-mode
Aug 7, 2021
3e759a0
example for showing peer interaction in kademlia
Aug 8, 2021
b8cbe62
finally able to discover other peers
Aug 8, 2021
fc218a0
Add `Mode` enum for kademlia client/server mode
Aug 10, 2021
e58e544
Remove confirmation code from `inject_fully_negotiated_inbound`
Aug 10, 2021
af1fcbd
Correct `client` value for network behaviour handler
Aug 10, 2021
c944ecb
Merge branch 'kademlia-client-mode' into kademlia-example
Aug 10, 2021
4be8fca
refactoring
Aug 11, 2021
e213350
Refactoring complete
Aug 12, 2021
e47148d
Add example for running kademlia in `client` mode
Aug 15, 2021
cdaa0a1
Merge commit with master
Aug 15, 2021
1a09907
removing .vscode
Aug 15, 2021
46167a1
Add better documentation and comments
Aug 21, 2021
8bf3ef4
replaced example with test in kad-behaviour
Aug 28, 2021
9ef4c4c
Changes to client mode test
Aug 29, 2021
c387f41
remove kad-example from cargo.toml
Aug 29, 2021
5b21774
Better checks for client mode test
Aug 29, 2021
5ea645e
Fix client mode test
Aug 29, 2021
aa6500f
Refactor kademlia client mode test
Aug 30, 2021
146405f
Rename to check variables
Aug 30, 2021
2cea417
Final fix for `client_mode` test.
Aug 30, 2021
54331e5
remove commented code
Aug 30, 2021
9d23741
Merge branch 'master' into kademlia-client-mode
Sep 3, 2021
b202c5b
Add some basic comments.
Sep 7, 2021
8234a17
Changes to test. Currently failing.
Sep 7, 2021
d5e6509
Changes to `client_mode` test. Currently failing.
Sep 7, 2021
06c1a30
Correct variable name.
Sep 10, 2021
d420fa6
Add checks to see if PUT and GET are working.
Sep 10, 2021
190c3dd
Add correct checks for PUT and GET operations.
Sep 10, 2021
ab97b10
Merge branch `master` into `kademlia-client-mode`
Sep 10, 2021
f3259b1
update prost dependencies
Sep 10, 2021
f308b0c
Replace `any` with `all` for server peer check.
Sep 11, 2021
8f33ba1
Merge branch `master` into `kademlia-client-mode`
Sep 14, 2021
520fb8e
Extra comments
Sep 14, 2021
d52c6af
Fix merge conflict with master
Sep 30, 2021
81eae5f
New test
Oct 3, 2021
f0c2fc3
Remove `allow_listening` logic.
Oct 3, 2021
f0bf7df
Fix gitignore
Oct 3, 2021
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
target
Cargo.lock
.vscode/launch.json
whereistejas marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 21 additions & 1 deletion protocols/kad/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::handler::{
};
use crate::jobs::*;
use crate::kbucket::{self, Distance, KBucketsTable, NodeStatus};
use crate::protocol::{KadConnectionType, KadPeer, KademliaProtocolConfig};
use crate::protocol::{KadConnectionType, KadPeer, KademliaProtocolConfig, Mode};
use crate::query::{Query, QueryConfig, QueryId, QueryPool, QueryPoolState};
use crate::record::{
self,
Expand Down Expand Up @@ -175,6 +175,8 @@ pub struct KademliaConfig {
connection_idle_timeout: Duration,
kbucket_inserts: KademliaBucketInserts,
caching: KademliaCaching,
// Set the peer mode.
mode: Mode,
}

/// The configuration for Kademlia "write-back" caching after successful
Expand Down Expand Up @@ -209,6 +211,8 @@ impl Default for KademliaConfig {
connection_idle_timeout: Duration::from_secs(10),
kbucket_inserts: KademliaBucketInserts::OnConnected,
caching: KademliaCaching::Enabled { max_peers: 1 },
// By default, a peer will not be in client mode.
mode: Mode::default(),
}
}
}
Expand Down Expand Up @@ -390,6 +394,21 @@ impl KademliaConfig {
self.caching = c;
self
}

/// Sets the [`Mode`] the node is operating in.
///
/// The default is [`Mode::Server`].
///
/// In [`Mode::Client`] the node does not advertise support for the Kademlia
/// protocol, nor does it accept incoming Kademlia streams. Peers will thus not
/// include the local node in their routing table. The node can still make
/// outbound requests.
///
/// Use [`Mode::Client`] when not publicly reachable.
pub fn set_mode(&mut self, mode: Mode) -> &mut Self {
self.mode = mode;
self
}
}

impl<TStore> Kademlia<TStore>
Expand Down Expand Up @@ -1710,6 +1729,7 @@ where
protocol_config: self.protocol_config.clone(),
allow_listening: true,
idle_timeout: self.connection_idle_timeout,
mode: Mode::default(),
})
}

Expand Down
93 changes: 93 additions & 0 deletions protocols/kad/src/behaviour/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1317,3 +1317,96 @@ fn network_behaviour_inject_address_change() {
kademlia.addresses_of_peer(&remote_peer_id),
);
}

#[test]
fn client_mode() {
// Create server peers.
let mut server_nodes = build_fully_connected_nodes_with_config(2, KademliaConfig::default());

// Create a client peer.
let mut cfg = KademliaConfig::default();
cfg.set_mode(Mode::Client);
let mut client = build_node_with_config(cfg);

// Fitler out peer Ids.
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
// Fitler out peer Ids.
// Filter out peer Ids.

let mut peers: Vec<_> = server_nodes
.iter()
.map(|(_, swarm)| swarm.local_peer_id().clone())
.collect();
peers.push(client.1.local_peer_id().clone());

// Filter out MultiAddrs and swarms.
let mut addrs: Vec<_> = server_nodes.iter().map(|(addr, _)| addr.clone()).collect();
addrs.push(client.0.clone());

let mut swarms: Vec<_> = server_nodes.iter_mut().map(|(_, swarm)| swarm).collect();
swarms.push(&mut client.1);

let key = "123".to_string();
let key = Key::new(&key);
let record = Record::new(key.clone(), vec![4, 5, 6]);

// Put a record from a server peer.
swarms[0]
.behaviour_mut()
.put_record(record.clone(), Quorum::One)
.unwrap();

// Dial a server peer from the client peer.
swarms[2]
.behaviour_mut()
.add_address(&peers[1], addrs[1].clone());

// Try to get the same record from the client node.
swarms[2].behaviour_mut().get_record(&key, Quorum::One);

block_on(poll_fn(|ctx| {
for swarm in &mut swarms {
loop {
match swarm.poll_next_unpin(ctx) {
Poll::Ready(Some(SwarmEvent::Behaviour(
KademliaEvent::OutboundQueryCompleted { result, .. },
))) => match result {
QueryResult::PutRecord(result) => match result {
Ok(PutRecordOk { .. }) => {
// Check if the server peer is not connected to the client peer.
assert!(
swarm
.behaviour_mut()
.connected_peers
.iter()
.all(|p| *p != peers[2]),
"The server peer is connected to the client peer."
);
}
Copy link
Member

Choose a reason for hiding this comment

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

How about calling swarm[2].behaviour_mut().get_record(..) once the first server is done with the PutRecord query. That would prevent any races between the PUT and the GET.

Copy link
Author

@whereistejas whereistejas Oct 3, 2021

Choose a reason for hiding this comment

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

Hi, I have a doubt with the present test we have:

It is kind of hard to reliably check for absence of something, especially in an async setting.

  1. The server stores a value Kademlia::put_value, waiting for a KademliaEvent::OutboundQueryCompleted.
  2. The client connects to the server.
  3. The client does a Kademlia::get_record, waiting for a KademliaEvent::OutboundQueryCompleted.
  4. Check that the client has the server in its routing table and check that the server does not have the client in its routing table.

Are we sure that using the GetRecord operation will update the routing tables of the server peer? I'm trying to find the code, which does the "update"-ing. But, I'm unable to find it. Could you please point me towards the appropriate code?

Update: (Please correct me if I'm wrong) I don't think Kademlia is bi-directional. For example, if peer A adds peer B to its routing table, then peer B will not add peer A to its routing table.

Copy link
Member

Choose a reason for hiding this comment

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

Update: (Please correct me if I'm wrong) I don't think Kademlia is bi-directional. For example, if peer A adds peer B to its routing table, then peer B will not add peer A to its routing table.

Before this pull request both peers would likely add each other to their respective routing table. After this pull request, whether the two peers add each other to their routing table depends on the Mode the other peer is running in.

Err(e) => panic!("PUT operation failed: {:?}", e),
},
QueryResult::GetRecord(result) => match result {
Ok(GetRecordOk { .. }) => {
// Check if the client peer is connected to the server peer.
assert!(
swarm
.behaviour_mut()
.connected_peers
.iter()
.any(|p| (*p == peers[0] || *p == peers[1])),
"The client peer is not connected to the server peers."
);

// GetRecord will never be successful unless and until PutRecord is
// successful.
return Poll::Ready(());
}
Err(e) => panic!("GET operation failed: {:?}", e),
},
_ => {}
},
Poll::Ready(Some(_)) => {}
e @ Poll::Ready(_) => panic!("Unexpected return value: {:?}", e),
Poll::Pending => break,
}
}
}
Poll::Pending
}));
}
46 changes: 30 additions & 16 deletions protocols/kad/src/handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

use crate::protocol::{
KadInStreamSink, KadOutStreamSink, KadPeer, KadRequestMsg, KadResponseMsg,
KademliaProtocolConfig,
KademliaProtocolConfig, Mode,
};
use crate::record::{self, Record};
use futures::prelude::*;
Expand Down Expand Up @@ -62,10 +62,15 @@ impl<T: Clone + fmt::Debug + Send + 'static> IntoProtocolsHandler for KademliaHa
}

fn inbound_protocol(&self) -> <Self::Handler as ProtocolsHandler>::InboundProtocol {
if self.config.allow_listening {
upgrade::EitherUpgrade::A(self.config.protocol_config.clone())
} else {
upgrade::EitherUpgrade::B(upgrade::DeniedUpgrade)
match self.config.mode {
Mode::Client => upgrade::EitherUpgrade::B(upgrade::DeniedUpgrade),
Mode::Server => {
if self.config.allow_listening {
whereistejas marked this conversation as resolved.
Show resolved Hide resolved
upgrade::EitherUpgrade::A(self.config.protocol_config.clone())
} else {
upgrade::EitherUpgrade::B(upgrade::DeniedUpgrade)
}
}
}
}
}
Expand Down Expand Up @@ -123,6 +128,9 @@ pub struct KademliaHandlerConfig {

/// Time after which we close an idle connection.
pub idle_timeout: Duration,

/// [`Mode`] the handler is operating in.
pub mode: Mode,
}

/// State of an active substream, opened either by us or by the remote.
Expand Down Expand Up @@ -480,11 +488,20 @@ where
type InboundOpenInfo = ();

fn listen_protocol(&self) -> SubstreamProtocol<Self::InboundProtocol, Self::InboundOpenInfo> {
if self.config.allow_listening {
SubstreamProtocol::new(self.config.protocol_config.clone(), ())
.map_upgrade(upgrade::EitherUpgrade::A)
} else {
SubstreamProtocol::new(upgrade::EitherUpgrade::B(upgrade::DeniedUpgrade), ())
match self.config.mode {
Mode::Server => {
if self.config.allow_listening {
SubstreamProtocol::new(self.config.protocol_config.clone(), ())
.map_upgrade(upgrade::EitherUpgrade::A)
} else {
SubstreamProtocol::new(upgrade::EitherUpgrade::B(upgrade::DeniedUpgrade), ())
}
}
// If we are in client mode, I don't want to advertise Kademlia so that other peers will
// not send me Kademlia requests.
Mode::Client => {
SubstreamProtocol::new(upgrade::EitherUpgrade::B(upgrade::DeniedUpgrade), ())
}
}
}

Expand Down Expand Up @@ -520,12 +537,8 @@ where
self.next_connec_unique_id.0 += 1;
self.substreams
.push(SubstreamState::InWaitingMessage(connec_unique_id, protocol));
if let ProtocolStatus::Unconfirmed = self.protocol_status {
// Upon the first successfully negotiated substream, we know that the
// remote is configured with the same protocol name and we want
// the behaviour to add this peer to the routing table, if possible.
self.protocol_status = ProtocolStatus::Confirmed;
}
// Just because another peer is sending us Kademlia requests, doesn't necessarily
// mean that it will answer the Kademlia requests that we send to it.
}

fn inject_event(&mut self, message: KademliaHandlerIn<TUserData>) {
Expand Down Expand Up @@ -763,6 +776,7 @@ impl Default for KademliaHandlerConfig {
protocol_config: Default::default(),
allow_listening: true,
idle_timeout: Duration::from_secs(10),
mode: Mode::default(),
}
}
}
Expand Down
13 changes: 13 additions & 0 deletions protocols/kad/src/protocol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,19 @@ pub const DEFAULT_PROTO_NAME: &[u8] = b"/ipfs/kad/1.0.0";
/// The default maximum size for a varint length-delimited packet.
pub const DEFAULT_MAX_PACKET_SIZE: usize = 16 * 1024;

#[derive(Debug, Clone)]
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
#[derive(Debug, Clone)]
/// See [`crate::KademliaConfig::set_mode`].
#[derive(Debug, Clone)]

/// See [`crate::KademliaConfig::set_mode`].
pub enum Mode {
Client,
Server,
}

impl Default for Mode {
fn default() -> Self {
Mode::Server
}
}

/// Status of our connection to a node reported by the Kademlia protocol.
#[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)]
pub enum KadConnectionType {
Expand Down