swarm: re-introduce ConnectionHandler::{In,Out}boundOpenInfo

[jxs]

Description

With #3268 was decided to deprecate ConnectionHandler::{In,Out}boundOpenInfo.
#5242 made it official, but there's a bug with these deprecations:
storing the upgrade info in a VecDeque loses the order significance match they prevously had with StreamUpgrade.

Example with request-response after #3914 was merged:

• Behaviour notifies handler with a request
• Handler puts the request on requested_outbound and asks the Swarm for a OutboundSubstreamRequest
• Handler receives a new FullyNegotiatedOutbound from the Swarm and pops the request from requested_outbound.

Problem:

the popped request may not match inserted request in requested_outbound and therefore the negotiated Protocol with the outbound peer as Swarm's negotiating_out is a FuturesUnordered.

This is specially problematic in the Ethereum consensus spec as the negotiated protocol matches the request type and the outbound peer ends up discarding the request as it doesn't match the protocol negotiated.

We should rollback the deprecation attributes of ConnectionHandler::{In,Out}boundOpenInfo at least
cc @elenaf9 @dariusc93

[drHuangMHT]

I don't think bringing back {In, Out}boundOpenInfo is a good practice though. Instead we should map each request to their corresponding substream, that's what we should do after deprecation.

EDIT: This is more of a problem from the consensus spec because the protocol is a part of the substream itself, which should be job-agnostic, or at least the job should only be assgined AFTER the substream is negotiated. This is what we expect after the deprecation(and thus future removal) but the spec relates them. However, considering the significance of Ethereum itself, the spec will very likey remain unchanged.

[jxs]

I don't think bringing back {In, Out}boundOpenInfo is a good practice though. Instead we should map each request to their corresponding substream, that's what we should do after deprecation.

how would that be possible? And why is it better than maintaining the {In,Out}boundOpenInfo?

EDIT: This is more of a problem from the consensus spec because the protocol is a part of the substream itself, which should be job-agnostic, or at least the job should only be assgined AFTER the substream is negotiated.

AFAIK there aren't any recommendations (nor data backing them) on how should protocols handle multistream-select and substreams, so I don't think it's correct to say how or how not should users design their protocols.
I find it is actually an interesting way to support different request versions without bumping the global protocol version as for example Gossipsub does, which has the disadvantage that a small change in the protocol behavior needs a different version support, and one may end having to increase the version every time a new change is introduced, as it actually is happening with Gossipsub

[drHuangMHT]

how would that be possible? And why is it better than maintaining the {In,Out}boundOpenInfo?

Sorry, let me be clear. I believe that the whole idea behind deprecating OpenInfo is that all substreams should be treated equally and seen as interchangable entities. This, in practice, means that their jobs should be assigned after creation, and other streams can also take on the same job without any issue.
request-response is a special case here, because it opens substream when there is a request, essentially making the substream a part of the actual request, which is not desirable when substreams should be treated interchangably.
The easiest way to make it work is just replacing the queue with a map. When the substream is negotiated, find the corresponding request, essentially making Protocol the old OpenInfo. The desired way is to mimic HTTP, making requests channel-agnostic.

[jxs]

Sorry, let me be clear. I believe that the whole idea behind deprecating OpenInfo is that all substreams should be treated equally and seen as interchangable entities.

I challenge this, why should they be treated interchangeably? #3268 refers that:

Streams however are completely interchangeable in libp2p once the protocol to be spoken on top of it has been negotiated.

this assumed that they were interchangeable, not that they should be interchangeable.
But the fundamental point is: This feature has existed in rust-libp2p, it's stable and allows end users to express themselves in a way that without it they can't, why should it be removed?

The easiest way to make it work is just replacing the queue with a map. When the substream is negotiated, find the corresponding request, essentially making Protocol the old OpenInfo. The desired way is to mimic HTTP, making requests channel-agnostic.

this seems way more complicated than just maintaining what exists, the StreamUpgrade structure

[drHuangMHT]

I challenge this, why should they be treated interchangeably? #3268 refers that:

Streams however are completely interchangeable in libp2p once the protocol to be spoken on top of it has been negotiated.

this assumed that they were interchangeable, not that they should be interchangeable.

Does that mean the consensus had been established? And what does "they were interchangeable" mean? Are they still interchangeable now?

But the fundamental point is: This feature has existed in rust-libp2p, it's stable and allows end users to express themselves in a way that without it they can't, why should it be removed?

So here's the conflict: there is no doubt that UpgradeInfo is a powerful feature, but is also against the spirit of interchangeable substreams. Are we moving away from the established consensus?
There's also some ambiguity: what does "the protocol to be spoken on top of it" mean? Does it refer to request-response itself or SubstreamProtocol?

this seems way more complicated than just maintaining what exists, the StreamUpgrade structure

Yes it is. But that's what we should do(though not that we should implement it in such an inefficient way) if we want both interchangeable substreams in general and dedicated substream in request-response.

I'm not strongly opposing the comeback of UpgradeInfo, it is the conflict that makes me uneasy.

[jxs]

Does that mean the consensus had been established? And what does "they were interchangeable" mean? Are they still interchangeable now?

substreams were never interchangeable, because the moment one negotiates a protocol for the upgrade with the outbound connection those protocol and upgrade are tied together, and this isn't particular to rust-lib2p is particular to libp2p, right?

So here's the conflict: there is no doubt that UpgradeInfo is a powerful feature, but is also against the spirit of interchangeable substreams. Are we moving away from the established consensus?

I haven't seen a consensus wrt this, what happened was:
Thomas created that issue two years ago without knowledge of the particularities of Ethereum consensus Rpc protocol when he was working actively as a maintainer of the project.

I have since spoke about this in the calls but I am happy to elaborate here for posterity, when Max and Thomas left and I inherited the maintenance of rust-libp2p the goals changed, I cannot be full time dedicated to the project, nor I think for the future it makes sense to develop features that lack stakeholders cause rust-libp2p is (thanks to the amazing work of Max and Thomas) fundamentally stable in my opinion (it powers 30-40% of the ethereum network for example).

So, unless something is required for a particular active user for rust-lib2p or something is affecting particular an active user of rust-libp2p, I don't think it should be changed.
This particular case and issue is paradigmatic, the deprecation breaks the Ethereum Consensus RPC Protocol without any existing alternative possible. So anything that is not reverting the deprecation of In and OutboundOpenInfo introduces instability and confusion, therefore doesn't make sense to me.

[drHuangMHT]

substreams were never interchangeable, because the moment one negotiates a protocol for the upgrade with the outbound connection those protocol and upgrade are tied together

The ideal case after deprecating OpenInfo would be that substreams are only tied to a specific ConnectionHandler, not a specific job. But the consensus protocol require the latter, making the deprecation undesirable.

I haven't seen a consensus wrt this,

I consider merging of the PR a sign of consensus, plus it only marked OpenInfo as deprecated, after all protocols had migrated away from OpenInfo, hence the confusion I have right now. I believe that confuses others too, before the problem with ethereum surfaced.

what happened was:
Thomas created that issue two years ago without knowledge of the particularities of Ethereum consensus Rpc protocol when he was working actively as a maintainer of the project.

I have since spoke about this in the calls but I am happy to elaborate here for posterity, when Max and Thomas left and I inherited the maintenance of rust-libp2p the goals changed, I cannot be full time dedicated to the project, nor I think for the future it makes sense to develop features that lack stakeholders cause rust-libp2p is (thanks to the amazing work of Max and Thomas) fundamentally stable in my opinion (it powers 30-40% of the ethereum network for example).

Thanks for clarifying this!

[jxs]

I consider merging of the PR a sign of consensus, plus it only marked OpenInfo as deprecated, after all protocols had migrated away from OpenInfo, hence the confusion I have right now. I believe that confuses others too, before the problem with ethereum surfaced.

thanks for elaborating Dr Huang. I wouldn't use consensus as wording for this context, it was a decision taken by previous maintainers, and the current maintainers reverted that decision partially, due to its side effects.
The referred protocols will still be kept as they are as the reverting of the decision doesn't affect them. it just allows for more possible use cases.

Submitted #5860 to address this