Finer-grained multistream-select upgrade protocol version selection

[alindima]

Finer-grained multistream-select upgrade protocol version selection

[thomaseizinger]

Yes, @oblique did fantastic work to report these errors correctly in #4701 (which got merged as part of #3913 if I remember correctly.

[alindima]

As an alternative, have a failing OutboundUpgrade not kill the entire connection. Considering that a connection is multiplexed for various protocols, it's not obvious to me why we do this.

Another angle to view this problem is that https://github.com/libp2p/rust-libp2p/blob/3e06e0bca3c2d29bc7fb0e67f880d14a74049483/core/src/upgrade/apply.rs#L230C77-L230C77 shouldn't always return UpgradeError::Apply. It should take into account that the upgrade_outbound could also return a UpgradeError::Select error if the protocol version is V1Lazy. As far as I've seen, the UpgradeError::Select(Failed) will be gracefully handled and wouldn't kill the connection.

Maybe this should be the right way of solving this, although I think it warrants some breaking changes

[mxinden]

This is needed because V1Lazy will end up killing the entire connection if the protocol is not supported.

That comes as a surprise to me. Can you share details why that is the case?

Can you still reproduce this behavior on the latest release? I would consider the above a bug and suspect it has been fixed since, e.g. via #4701.

[alindima]

We're not running on the latest release yet, there's work under way for upgrading libp2p in substrate/polkadot.

I digged a bit and discovered that this PR solved the issue of killing connections for a failed outbound stream: #3913. Thanks!

However, there's still an issue. We are now no longer notified of a failed request. We want to be able to handle that (and precisely know why it failed).

Suppose we make a request with V1Lazy to a peer that doesn't support our req-response protocol. Multistream-select will not do protocol negotiation rightaway, it'll send the request along with the multistream-select packet.
Then, the request OutboundUpgrade will fail with a generic std::io::Error::custom("Failed"). This will be ignored by the req-response behaviour (thanks to #3913).

The higher-level code will not be notified of this failure.

I think the root of the problem is that protocol negotiation errors are swallowed when using V1Lazy and converted to a custom IO error that is ignored.

[alindima]

We are now no longer notified of a failed request

I may be wrong, as I only cherry-picked #3913 over v0.51.3 (which we are using in substrate). Reading the code on latest master, I see we should now report an OutboundFailure.

By reading the code on master, all of this should be fixed in the latest release, but I cannot test with it, as it's a ton of in-progress work to upgrade libp2p: paritytech/polkadot-sdk#1631.

As a workaround until we manage to upgrade libp2p and test it, I'll switch to using multistream-select V1 for now.

I'll close the issue and reopen it if it's still a problem after upgrading.

It may still be an issue, since it looks like the same symptoms are reported in: paritytech/substrate#14683.