On alternative designs of libp2p-core and libp2p-swarm

[thomaseizinger]

Having worked with rust-libp2p for quite some time now, I noticed that some things are harder to achieve than others and I've been wondering what we can do about that.

Specifically, the abstractions of NetworkBehaviour and ProtocolsHandler are essentially driven by the design of ConnectionHandler in that the ConnectionHandler semantically out-lives all substreams of a connection. I believe that this particular aspect makes it cumbersome to implement protocols that don't need to take advantage of this additional state.

For example, any protocol that uses short-lived substreams (identify, ping, rendezvous, request-response and several IPFS ones if I am not mistaken) are harder to implement because all abstraction layers need to deal with the state prior and post the substream's existence.

If there were an abstraction that is tied to the lifetime of a single substream, implementing these protocols could be as simple as: fn(substream) -> (fn(request) -> response)).

One downside if only such a model is provided is that it is next to impossible to implement protocols that depend on the ability to orchestrate and coordinate between multiple substreams. Examples for this are gossipsub and kademlia.

What seems to be the million-dollar question is: What should the core primitives within libp2p-core look like to allow for both kinds of protocols in an easy way?

To get closer to this answer, I think it would be helpful if we can collect what specific parts and designs of the codebase are already considered in need of an overhaul. From looking at the code, one can see some duplication in the reporting of events. NetworkEvent and SwarmEvent for example a very similar. A PoolEvent in turn is reasonably similar to a NetworkEvent. A ManagerEvent is very similar to a PoolEvent.

---

As a starting point for the discussion, I would like to propose the following:

• It is my understanding that a ProtocolsHandler currently could - in theory - deal with multiple substreams of the same peer simultaneously. Looking at existing implementations though, this is rarely considered. For example, gossipsub simply replaces the current substream with the new one.
• A NetworkBehaviour already has to deal with the concept of multiple connections by the nature of dealing with multiple peers.

Is there a way of "relieving" a ProtocolsHandler from having to deal with potentially multiple substreams? If we tie the lifetime of a ProtocolsHandler to that of a substream (and create a new instance per substream), I think its implementations would become a easier:

1. Changing the API of ProtocolsHandler to create a new instance using the given negotiated substream (i.e. ProtocolsHandler::new_inbound(substream)) would avoid annoying "empty" states.
2. Limiting a ProtocolsHandler to a single substream at all times avoids complexity in that protocol implementors don't need to consider this aspect because it is handled within libp2p-core.

Please do point out if I misunderstood anything in the existing design. I am also very happy to submit patches if we can find consensus on how things can/should be changed.

[mxinden]

First off, thanks for the detailed write-up @thomaseizinger! Happy to see these new ideas.

It is my understanding that a ProtocolsHandler currently could - in theory - deal with multiple substreams of the same peer simultaneously.

Correct.

Looking at existing implementations though, this is rarely considered.

• This is used by libp2p-kad to manage multiple outbound and inbound substreams
  

  rust-libp2p/protocols/kad/src/handler.rs

  Lines 88 to 89 in 28fe6ed

  	/// List of active substreams with the state they are in.
  	substreams: Vec<SubstreamState<TUserData>>,

• This is used in libp2p-request-response to manage multiple outbound and inbound requests / responses
  

  rust-libp2p/protocols/request-response/src/handler.rs

  Lines 78 to 85 in 28fe6ed

  	/// Outbound upgrades waiting to be emitted as an `OutboundSubstreamRequest`.
  	outbound: VecDeque<RequestProtocol<TCodec>>,
  	/// Inbound upgrades waiting for the incoming request.
  	inbound: FuturesUnordered<BoxFuture<'static,
  	Result<
  	((RequestId, TCodec::Request), oneshot::Sender<TCodec::Response>),
  	oneshot::Canceled
  	>>>,

• This is used in libp2p-relay to relay multiple connections over a single connection
  

  rust-libp2p/protocols/relay/src/handler.rs

  Lines 89 to 102 in 28fe6ed

  	/// Futures that send back negative responses.
  	deny_futures: FuturesUnordered<BoxFuture<'static, Result<(), std::io::Error>>>,
  	/// Futures that send back an accept response to a relay.
  	accept_dst_futures: FuturesUnordered<
  	BoxFuture<
  	'static,
  	Result<
  	(PeerId, protocol::Connection, oneshot::Receiver<()>),
  	protocol::IncomingDstReqError,
  	>,
  	>,
  	>,
  	/// Futures that copy from a source to a destination.
  	copy_futures: FuturesUnordered<BoxFuture<'static, Result<(), protocol::IncomingRelayReqError>>>,

What these implementations then handle the coordination between substreams in their NetworkBehaviour?

A NetworkBehaviour already has to deal with the concept of multiple connections by the nature of dealing with multiple peers.

As well as multiple connections to the same peer.

Changing the API of ProtocolsHandler to create a new instance using the given negotiated substream (i.e. ProtocolsHandler::new_inbound(substream)) would avoid annoying "empty" states.

Agreed that the "empty" state is somewhat strange and also in combination with keep-alive handling error prone.

[thomaseizinger]

What these implementations then handle the coordination between substreams in their NetworkBehaviour?

Yes that would be the idea. NetworkBehaviours already have to deal with multiple connections (to multiple peers and multiple connections to the same peer). Having to deal with multiple substreams per connections doesn't increase complexity a whole lot. It might make things actually more consistent because the triple of PeerId, ConnectionId and SubstreamId would form an actually unique identifier for a message exchange with another peer.

The issue I see with the current design is that if a ProtocolsHandler wants to support simultaneous substreams per connection, it has to design its own identification system of mapping substreams to incoming events or use channels like request-response does. The NetworkBehaviour already does this for us by tagging events from the handlers with PeerId and ConnectionId.

Hence, even though it is "the right thing" to do, actually supporting simultaneous substreams is quite a bit of effort. It would be nice if we could just solve this problem in one place for all protocols.

[mxinden]

The NetworkBehaviour already does this for us by tagging events from the handlers with PeerId and ConnectionId.

Just an aside, a ProtocolsHandler can already learn the PeerId of the remote via the IntoProtocolsHandler constructor. I don't see any obstacles in also passing the ConnectionId to the ProtocolsHandler in a similar way.

[thomaseizinger]

It is not so much about the ProtocolsHandler learning the PeerId. What I am trying to achieve is simplifying the implementations of ProtocolsHandler by re-designing them such that they always map to a single substream and hence never need to deal any kind of 1-to-n mapping. A NetworkBehaviour on the other end already has to do that anyway because it deals with multiple peers.

I'll see to draft up a PR that showcases what I mean.

[thomaseizinger]

I've sketched out something in this branch here: https://github.com/comit-network/rust-libp2p/blob/single-substream-handler/swarm/src/protocols_handler/substream.rs

It did turn out slightly different than originally anticipated. As you've probably noticed, my original intention was to change ProtocolsHandler to be per substream. This might still be a viable option in the long run but I think we can do something less invasive. All problems in computer science can be solved by adding another layer of indirection right? :)

That is essentially what the above module does. It defines a SubstreamHandler trait and a corresponding ProtocolsHandlerAdaptor (all names subject to bike-shedding). I've sketched out how it could be used in the tests module. Everything compiles but it is not quite functional, yet I hope it gets the idea across.

The module is slightly opinionated in that it assumes:

• we don't need InboundOpenInfo
• we don't actually upgrade the connection but just return the substream

I think such a module could be very helpful for various kinds of protocols:

• Protocols with long-lived substreams (like gossipsub) would benefit from reduced boilerplate by not having to deal with empty states before and after the substream is present
• Protocols with short-lived substreams (like identify) would also benefit because it is quite easy to build an abstraction on top of SubstreamHandler that only reads/writes a single message to the substream: https://github.com/comit-network/rust-libp2p/blob/8fedd6725da0f94d917c16b618b78ef79bde2dde/swarm/src/protocols_handler/substream.rs#L27-L79
• Other usecases like "streaming responses" (protocols/streaming-response #1947) should also be easier to realize because it is just a matter of reading more than one message from the substream inside the SubstreamHandler

Overall, I think such an abstraction would serve as a better foundation for building new protocols than the current ProtocolsHandler does. Hence, as mentioned above, I think it might make sense to eventually have NetworkBehaviour directly depend on SubstreamHandler instead of ProtocolsHandler. As you can see from the design of the events, a NetworkBehaviour can now essentially directly open a substream:

https://github.com/comit-network/rust-libp2p/blob/8fedd6725da0f94d917c16b618b78ef79bde2dde/swarm/src/protocols_handler/substream.rs#L303-L309

Similarly, any event is essentially uniquely attributable to a single substream:

https://github.com/comit-network/rust-libp2p/blob/8fedd6725da0f94d917c16b618b78ef79bde2dde/swarm/src/protocols_handler/substream.rs#L297-L301

This opens up an issue where notifying the wrong handler will end up not delivering the message. In other words, there is a potential problem here:

https://github.com/comit-network/rust-libp2p/blob/8fedd6725da0f94d917c16b618b78ef79bde2dde/swarm/src/protocols_handler/substream.rs#L318-L322

If NetworkBehaviour would directly depend on a SubstreamHandler, finding the correct connection for a substream could be done by libp2p-core behind the scenes and it would always be correct. In addition, the interface for inject_event would get simpler because the substream-id could be lifted out of the event type, next to the connection-id and peer id if there were first-class support.

Curious to hear your thoughts @mxinden !

[thomaseizinger]

I am trialing this approach in the rendezvous implementation: da0e761