swarm/src/behaviour: Consider removing NetworkBehaviourAction::TInEvent

[mxinden]

Today NetworkBehaviourAction has a trait parameter TInEvent that defaults to THandlerInEvent<THandler>.

rust-libp2p/swarm/src/behaviour.rs

Lines 356 to 359 in b316c6d

	pub enum NetworkBehaviourAction<
	TOutEvent,
	THandler: IntoProtocolsHandler,
	TInEvent = THandlerInEvent<THandler>,

TInEvent is needed to construct a NetworkBehaviourAction that contains an InEvent not matching the corresponding handler. If I am not mistaken this is only needed in libp2p-gossipsub for an optimization of wrapping messages to be gossiped in an Arc.

rust-libp2p/protocols/gossipsub/src/behaviour.rs

Lines 200 to 201 in b316c6d

	type GossipsubNetworkBehaviourAction =
	NetworkBehaviourAction<GossipsubEvent, GossipsubHandler, Arc<GossipsubHandlerIn>>;

I think it is worth exploring an alternative for libp2p-gossipsub and thus being able to remove TInEvent on NetworkBehaviourAction. In my eyes, removing the trait parameter simplifies things a lot.

Proof of concept

diff --git a/swarm/src/behaviour.rs b/swarm/src/behaviour.rs
index f50b0250..face9a1f 100644
--- a/swarm/src/behaviour.rs
+++ b/swarm/src/behaviour.rs
@@ -258,11 +258,7 @@ pub trait NetworkBehaviourEventProcess<TEvent> {
 // [`NetworkBehaviourAction::map_in`], mapping the handler `InEvent` leaving the
 // handler itself untouched.
 #[derive(Debug)]
-pub enum NetworkBehaviourAction<
-    TOutEvent,
-    THandler: IntoProtocolsHandler,
-    TInEvent = THandlerInEvent<THandler>,
-> {
+pub enum NetworkBehaviourAction<TOutEvent, THandler: IntoProtocolsHandler> {
     /// Instructs the `Swarm` to return an event when it is being polled.
     GenerateEvent(TOutEvent),
 
@@ -474,7 +470,7 @@ pub enum NetworkBehaviourAction<
         /// The options w.r.t. which connection handler to notify of the event.
         handler: NotifyHandler,
         /// The event to send.
-        event: TInEvent,
+        event: THandlerInEvent<THandler>,
     },
 
     /// Informs the `Swarm` about an address observed by a remote for
@@ -511,42 +507,6 @@ pub enum NetworkBehaviourAction<
     },
 }
 
-impl<TOutEvent, THandler: IntoProtocolsHandler, TInEventOld>
-    NetworkBehaviourAction<TOutEvent, THandler, TInEventOld>
-{
-    /// Map the handler event.
-    pub fn map_in<TInEventNew>(
-        self,
-        f: impl FnOnce(TInEventOld) -> TInEventNew,
-    ) -> NetworkBehaviourAction<TOutEvent, THandler, TInEventNew> {
-        match self {
-            NetworkBehaviourAction::GenerateEvent(e) => NetworkBehaviourAction::GenerateEvent(e),
-            NetworkBehaviourAction::Dial { opts, handler } => {
-                NetworkBehaviourAction::Dial { opts, handler }
-            }
-            NetworkBehaviourAction::NotifyHandler {
-                peer_id,
-                handler,
-                event,
-            } => NetworkBehaviourAction::NotifyHandler {
-                peer_id,
-                handler,
-                event: f(event),
-            },
-            NetworkBehaviourAction::ReportObservedAddr { address, score } => {
-                NetworkBehaviourAction::ReportObservedAddr { address, score }
-            }
-            NetworkBehaviourAction::CloseConnection {
-                peer_id,
-                connection,
-            } => NetworkBehaviourAction::CloseConnection {
-                peer_id,
-                connection,
-            },
-        }
-    }
-}
-
 impl<TOutEvent, THandler: IntoProtocolsHandler> NetworkBehaviourAction<TOutEvent, THandler> {
     /// Map the event the swarm will return.
     pub fn map_out<E>(self, f: impl FnOnce(TOutEvent) -> E) -> NetworkBehaviourAction<E, THandler> {
@@ -578,25 +538,24 @@ impl<TOutEvent, THandler: IntoProtocolsHandler> NetworkBehaviourAction<TOutEvent
     }
 }
 
-impl<TInEvent, TOutEvent, THandlerOld> NetworkBehaviourAction<TOutEvent, THandlerOld>
+impl<TOutEvent, THandlerOld> NetworkBehaviourAction<TOutEvent, THandlerOld>
 where
     THandlerOld: IntoProtocolsHandler,
-    <THandlerOld as IntoProtocolsHandler>::Handler: ProtocolsHandler<InEvent = TInEvent>,
 {
-    /// Map the handler.
-    pub fn map_handler<THandlerNew>(
+    /// Map the handler and hanlder event
+    pub fn map_handler_and_in_event<THandlerNew>(
         self,
-        f: impl FnOnce(THandlerOld) -> THandlerNew,
+        f_handler: impl FnOnce(THandlerOld) -> THandlerNew,
+        f_in_event: impl FnOnce(THandlerInEvent<THandlerOld>) -> THandlerInEvent<THandlerNew>,
     ) -> NetworkBehaviourAction<TOutEvent, THandlerNew>
     where
         THandlerNew: IntoProtocolsHandler,
-        <THandlerNew as IntoProtocolsHandler>::Handler: ProtocolsHandler<InEvent = TInEvent>,
     {
         match self {
             NetworkBehaviourAction::GenerateEvent(e) => NetworkBehaviourAction::GenerateEvent(e),
             NetworkBehaviourAction::Dial { opts, handler } => NetworkBehaviourAction::Dial {
                 opts,
-                handler: f(handler),
+                handler: f_handler(handler),
             },
             NetworkBehaviourAction::NotifyHandler {
                 peer_id,
@@ -605,7 +564,7 @@ where
             } => NetworkBehaviourAction::NotifyHandler {
                 peer_id,
                 handler,
-                event,
+                event: f_in_event(event),
             },
             NetworkBehaviourAction::ReportObservedAddr { address, score } => {
                 NetworkBehaviourAction::ReportObservedAddr { address, score }
diff --git a/swarm/src/toggle.rs b/swarm/src/toggle.rs
index b6af0e38..5debbd5d 100644
--- a/swarm/src/toggle.rs
+++ b/swarm/src/toggle.rs
@@ -221,9 +221,10 @@ where
         params: &mut impl PollParameters,
     ) -> Poll<NetworkBehaviourAction<Self::OutEvent, Self::ProtocolsHandler>> {
         if let Some(inner) = self.inner.as_mut() {
-            inner
-                .poll(cx, params)
-                .map(|action| action.map_handler(|h| ToggleIntoProtoHandler { inner: Some(h) }))
+            inner.poll(cx, params).map(|action| {
+                action
+                    .map_handler_and_in_event(|h| ToggleIntoProtoHandler { inner: Some(h) }, |e| e)
+            })
         } else {
             Poll::Pending
         }

[mexskican]

Even if manageable to have a TInEvent in the NetworkBehaviourAction. I think it would simplify the implementation, which can be a little confusing at the moment.

Do you know what was the initial motivation to have NetworkBehaviourAction::TInEvent? Was it only to allow the optimization for gossipsub? I think removing the option to have an InEvent different from the Handler one would not impact other NetworkBehaviour implementations inside or outside of rust-libp2p.

I looked quickly into thegossipsub NetworkBehaviour implementation, I am not sure what was the objective with the wrapping the of GossipsubHandlerIn into an Arc.

[thomaseizinger]

Do you know what was the initial motivation to have NetworkBehaviourAction::TInEvent? Was it only to allow the optimization for gossipsub? I think removing the option to have an InEvent different from the Handler one would not impact other NetworkBehaviour implementations inside or outside of rust-libp2p.

Initially, we had multiple type parameters. It has only been a recent effort to reduce those and "derive" them from associated types of other parameters (like in this case, from the ProtocolsHandler trait).

[mxinden]

It has only been a recent effort to reduce those and "derive" them from associated types of other parameters (like in this case, from the ProtocolsHandler trait).

Corresponding pull requests: #2182 #2183

I am not sure what was the objective with the wrapping the of GossipsubHandlerIn into an Arc.

If I recall correctly, the goal was to not clone the gossip messages for each peer the message would be send to. @AgeManning @divagant-martian would know more.

[AgeManning]

Yes, this was an optimization that was suggested here: #898 (comment)

If its standing in the way of this, I think we can probably find another way in doing this. The messages can be quite large and send to quite a number of peers, so it would be nice to avoid cloning these.

[mxinden]

Something along the lines of 1123952 could be used, i.e. a custom Action type that can later on be transformed into a NetworkBehaviourAction in NetworkBehaviour::poll.

[divagant-martian]

Github refuses to open the link to the comment since it's resolved. This one worked better for me (takes a couple of secs to load)