NetworkBehaviour trait is not fully implemented

[tomaka]

See the review comment #10035 (comment) (and the other comment right below)

The NetworkBehaviour trait implementations presently don't always redirect the method calls to their underlying NetworkBehaviours because rust-libp2p doesn't give the possibility to deconstruct a MultiHandler.

This means that a cargo update that updates libp2p could potentially break our code.

This should be solved in a short time frame.

[kpp]

That's what I came up with:

diff --git a/client/network/src/discovery.rs b/client/network/src/discovery.rs
index dc08ab57ed..93ea14ef56 100644
--- a/client/network/src/discovery.rs
+++ b/client/network/src/discovery.rs
@@ -589,13 +589,16 @@ impl NetworkBehaviour for DiscoveryBehaviour {
 
 	fn inject_connection_closed(
 		&mut self,
-		_peer_id: &PeerId,
-		_conn: &ConnectionId,
-		_endpoint: &ConnectedPoint,
+		peer_id: &PeerId,
+		conn: &ConnectionId,
+		endpoint: &ConnectedPoint,
 		_handler: <Self::ProtocolsHandler as IntoProtocolsHandler>::Handler,
 	) {
 		self.num_connections -= 1;
-		// NetworkBehaviour::inject_connection_closed on Kademlia<MemoryStore> does nothing.
+		for k in self.kademlias.values_mut() {
+			let handler = k.new_handler().into_handler(peer_id, endpoint);
+			NetworkBehaviour::inject_connection_closed(k, peer_id, conn, endpoint, handler);
+		}
 	}
 
 	fn inject_disconnected(&mut self, peer_id: &PeerId) {
@@ -689,8 +692,11 @@ impl NetworkBehaviour for DiscoveryBehaviour {
 		}
 	}
 
-	fn inject_listen_failure(&mut self, _: &Multiaddr, _: &Multiaddr, _: Self::ProtocolsHandler) {
-		// NetworkBehaviour::inject_listen_failure on Kademlia<MemoryStore> does nothing.
+	fn inject_listen_failure(&mut self, local_addr: &Multiaddr, send_back_addr: &Multiaddr, _: Self::ProtocolsHandler) {
+		for k in self.kademlias.values_mut() {
+			let handler = k.new_handler();
+			NetworkBehaviour::inject_listen_failure(k, local_addr, send_back_addr, handler);
+		}
 	}
 
 	fn inject_listener_error(&mut self, id: ListenerId, err: &(dyn std::error::Error + 'static)) {

@mxinden Max, is it enough or do I need to deconstruct MultiHandler to get (key, KademliaHandler) and pass in into associated inject_connection_closed by key?

[mxinden]

@mxinden Max, is it enough or do I need to deconstruct MultiHandler to get (key, KademliaHandler) and pass in into associated inject_connection_closed by key?

Yes, that is the way to go @kpp. Can you prepare a patch for https://github.com/libp2p/rust-libp2p/?

[kpp]

Sorry, would you please elaborate?

[mxinden]

@kpp you would need to add a method to Multihandler that takes self and returns a set of (key, CUSTOM_HANDLER_TYPE) pairs. For each pair in the set you can then call NetworkBehaviour::inject_connection_closed on the corresponding Kademlia behaviours in Substrate's discovery.rs.

[kpp]

Why should it differ from inject_listen_failure?

[tomaka]

I don't understand your questions. If you try to compile the code of your diff you will get compilation errors because of a type mismatch. When the behavior in discovery.rs starts dialing, it turns the KademliaHandler into a MultiHandler. If the dialing fails, libp2p gives you back that MultiHandler and you need to turn it back into the KademliaHandler.

The content of KademliaHandler and of the Kademlia::inject_* methods are implementation details of Kademlia. The implementation of NetworkBehaviour in discovery.rs is a wrapper around the Kademlia, and it is extremely bad if the wrapper modifies these implementation details.

[kpp]

you will get compilation errors because of a type mismatch.

It compiles.

I don't understand your questions.

Do we have to split handler as Max said for each inject_* method that accepts handler as the last argument or in inject_connection_closed only?

[tomaka]

It compiles.

Ahh, I misread your diff. Yeah, you definitely shouldn't be calling new_handler in that situation.

Do we have to split handler as Max said for each inject_* method that accepts handler as the last argument or in inject_connection_closed only?

All of them. It's part of the API contract of the NetworkBehaviour trait that the handler you get back is the same as the one you passed. It's actually the entire reason why the handler is passed back.

[kpp]

Thanks for clarification! Will do