Populate the ConnState field for connections

[julian88110]

Populate the NextProto / ConnState field in the case of Insecure transport.
This simplifies testing

[marten-seemann]

We should also do this for TLS and Noise, when talking to a node that doesn't support early muxer negotiation.

It would be really nice if this didn't require any changes to the security implementation, but could be done only in the upgrader.

[julian88110]

There are some data race on upgrader. Need to think about it more. putting this on draft mde

[julian88110]

We should also do this for TLS and Noise, when talking to a node that doesn't support early muxer negotiation.

It would be really nice if this didn't require any changes to the security implementation, but could be done only in the upgrader.

Sure we could add the same for TLS and Noise, but please note that the integration test we are building will not cover the TLS or Noise case for this scenario. We can provide the feature for future use though.

[marten-seemann]

It's probably easiest if you implement the ConnState() ConnectionState method on the upgrader.transportConn. You don't need to make any modifications to TLS, Noise or the Plaintext implementation then.

[julian88110]

upgrader.transportConn

The leverage of upgrader.transportConn is not an issue. The awkward part is to export the ms-muxer negotiation result so that it can be plugged in the upgrader.transportConn. Because libp2p employs parallel negotiation with multiple threads, there is potential for data race. I don't want to resort to locking. So I went through a couple of approaches and got one that works without locking. @marten-seemann please take a look and glad to discuss any other / better options if possible. Thanks!

[marten-seemann]

I think we should just remove the muxer_multistream.Transport. At this point, there's no point in having it be its own struct. It's just getting in the way: we already have a type assertion for the concrete type in upgrader.setupMuxer, and an awkward GetTransportByKey method.

Instead, the upgrader should perform the multistream negotiation itself (if the security protocol didn't inline the muxer negotiation). That should make it super easy to record the result of that negotiation step.

The result of the negotiation then can be exposed on the upgrader.transportConn, by adding a ConnState() ConnectionState method there. This function would then shadow the ConnState method of the network.ConnSecurity (very convenient!). This also means that we don't need to add any SetConnState everywhere across our code base.

@julian88110, does that make sense to you?

[BigLep]

@julian88110 : followup work is happening in #1854 right?

[julian88110]

I think we should just remove the muxer_multistream.Transport. At this point, there's no point in having it be its own struct. It's just getting in the way: we already have a type assertion for the concrete type in upgrader.setupMuxer, and an awkward GetTransportByKey method.

Instead, the upgrader should perform the multistream negotiation itself (if the security protocol didn't inline the muxer negotiation). That should make it super easy to record the result of that negotiation step.

The result of the negotiation then can be exposed on the upgrader.transportConn, by adding a ConnState() ConnectionState method there. This function would then shadow the ConnState method of the network.ConnSecurity (very convenient!). This also means that we don't need to add any SetConnState everywhere across our code base.

@julian88110, does that make sense to you?

Discussed offline on the solution, and the effort is tracked by #1854