draft: progressing on removing `{In,Out}boundUpgrade`

[thomaseizinger]

Description

Attempting to make progress on simplifying the ConnectionHandler interface (#2863), specifically, how we should deal with {In,Out}boundUpgrade.

The idea of this work is to eventually replace {In,Out}boundUpgrade with a trait no longer does any (async) upgrading. Instead, we only want it to express, which protocols are available, i.e. UpgradeInfo.

To get there, I think the easiest migration for users will be if we move them away from using {In,Out}BoundUpgrade altogether.

1. A reasonably easy, first migration can be to introduce new types within libp2p-swarm that implement the old traits but hard hardcode to enforce StreamProtocol and Stream. We can then deprecate the existing types within libp2p-core and remove them eventually.
2. The second step would be to entirely deprecate the traits and instead point users towards using some of the predefined "upgrades" from (1).
3. With (1) and (2) in place we should be able to remove and/or change the trait bounds on ConnectionHandler::{In,Out}boundProtocol without actually breaking users.

One thing that this will allow us to do is require a new trait bound for the Info of an upgrade that exposes the underlying StreamProtocol. By adding this bound, we can remove a lot of allocations from the libp2p_swarm::Connection::poll function. At the moment, the contract here is AsRef<str> meaning, we are re-allocating all protocols strings here very, very often.

Notes & open questions

Change checklist

• [ ] I have performed a self-review of my own code
• [ ] I have made corresponding changes to the documentation
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] A changelog entry has been made in the appropriate crates

[mxinden]

The idea of this work is to eventually replace {In,Out}boundUpgrade with a trait no longer does any (async) upgrading. Instead, we only want it to express, which protocols are available, i.e. UpgradeInfo.

To make sure we are on the same page, the end-goal is to even get rid of UpgradeInfo, right? I would imagine a ConnectionHandler trait definition along the lines of:

pub trait ConnectionHandler: Send + 'static {
    /// A type representing the message(s) a [`NetworkBehaviour`](crate::behaviour::NetworkBehaviour) can send to a [`ConnectionHandler`] via [`ToSwarm::NotifyHandler`](crate::behaviour::ToSwarm::NotifyHandler)
    type FromBehaviour: fmt::Debug + Send + 'static;
    /// A type representing message(s) a [`ConnectionHandler`] can send to a [`NetworkBehaviour`](crate::behaviour::NetworkBehaviour) via [`ConnectionHandlerEvent::NotifyBehaviour`].
    type ToBehaviour: fmt::Debug + Send + 'static;

    fn listen_protocol(&self) -> &[StreamProtocol];

Note the removal of InboundProtocol, OutboundProtocol, InboundOpenInfo and OutboundOpenInfo and the changed signature of listen_protocol.

I don't feel strongly about how we get here. Though I wonder whether a single step process (i.e. the removal of the associated traits right away) is simpler for a user.

[thomaseizinger]

The idea of this work is to eventually replace {In,Out}boundUpgrade with a trait no longer does any (async) upgrading. Instead, we only want it to express, which protocols are available, i.e. UpgradeInfo.

To make sure we are on the same page, the end-goal is to even get rid of UpgradeInfo, right?

That would be great! That design unfortunately creates several error cases when we try to dispatch the stream back to the ConnectionHandler. For outbound streams, I wouldn't actually know how to do it. How should ConnectionHandlerSelect know, which of its two child handlers wants the stream?