kad: Implement client-server mode for Kademlia#611
Conversation
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
|
In general looking good. From looking into this I was wondering about the impact on the routing table. So basically after this change the routing tables will be filles more conservatively which peers which have answered to some of our queries. Peers we discovered on the way but did not query will not be added, even though they might be in server mode (we just don't know). It sounds good to me, I assume we expect no impact? Or even positive impact, because these peers have already answered us. |
gab8i
left a comment
gab8i
left a comment
There was a problem hiding this comment.
Nice! So few lines of code for quite a feature!
dmitry-markin
left a comment
dmitry-markin
left a comment
There was a problem hiding this comment.
The logic seems good with some minor nits. I am going to analyze some general client-server operation consequences before the final approval, but should be good to go.
|
|
||
| /// Kademlia operating mode. | ||
| #[derive(Debug, Copy, Clone, Default, PartialEq, Eq)] | ||
| pub enum KademliaMode { |
There was a problem hiding this comment.
nit: move to config, not handle?
| // if identify was enabled, give it the advertised protocols and listen addresses and | ||
| // start it | ||
| if let Some((service, identify_config)) = identify_info.take() { | ||
| *identify_config.protocols.write() = transport_manager |
There was a problem hiding this comment.
There is currently no need to share RwLock, because the list of identify protocols only gets updated on the litep2p startup.
In general, sharing access to a private struct should be avoided and replaced by higher-level API for either adding/removing identify protocols, or even better enabling/disabling the advertisement of individual installed protocols.
| pub fn with_mode(mut self, mode: KademliaMode) -> Self { | ||
| self.mode = mode; | ||
| self | ||
| } |
There was a problem hiding this comment.
We should also make it possible to switch from client to server mode at runtime — e.g., when we discover a reachable external address. Can be a follow-up to make the PR smaller.
| // start kademlia protocol event loops | ||
| // | ||
| // protocol names of client-mode instances are not advertised via identify | ||
| let mut unadvertised_protocols = Vec::new(); |
There was a problem hiding this comment.
Keeping a separate list of ignored protocols seems to be error-prone. Can we update a transport_manager.register_protocol() to accept a mandatory parameter of whether we advertise the protocol instead?
|
|
||
| if let KademliaMode::Client = self.mode { | ||
| tracing::trace!(target: LOG_TARGET, ?peer, "ignoring inbound substream in client mode"); | ||
| let _ = substream.close().await; |
There was a problem hiding this comment.
We need to make sure this won't cause the nodes without client-server understanding to busy-loop on trying to open a substream. Reading and silently discarding all the input might be better if it is the case.
| // mode is | ||
| let _ = self | ||
| .event_tx | ||
| .send(KademliaEvent::RoutingTableUpdate { |
There was a problem hiding this comment.
It is not anymore the RoutingTableUpdate event, because we don't update the routing table. What about moving to insert_proven_server to not break the API by renaming the event?
| ); | ||
|
|
||
| self.engine.register_response( | ||
| let _ = self.engine.register_response( |
There was a problem hiding this comment.
We also learn the node is reachable through PUT_VALUE/ADD_PROVIDER/etc. success, so makes sense updating the routing table here as well.
ADD_PROVIDER is trickier, because the spec doesn't define the response message, so we should track the substream opening / message sending success instead (already done for ADD_PROVIDER queries), but at least worth adding a TODO comment.
4 participants
This PR implements the client-server mode of operation for Kademlia.
Full nodes and substrate-nodes utilize the server mode by default:
Light clients or DHT crawlers are encouraged to use the client mode:
To prepare for supporting the dynamic addition and removal of multiple DHTs at runtime, this PR introduces dynamic protocol configuration for the identify protocol.
By default, Kademlia implementations running in client mode are not advertised via the identify protocol.
cc @dmitry-markin @skunert