RFC: Remove StreamMuxerEvent::AddressChange variant

[thomaseizinger]

Description

This is never constructed, neither by libp2p-yamux, nor by libp2p-mplex.
Also, the WIP QUIC (#2289) implementation which is mentioned in the comments
also does not support this feature.

All of theses lines are dead code and are never executed.

I propose to delete this "functionality" until we know that it will be implemented and used.

This is a draft for now because:

• It is stacked on top of core/muxing: Force StreamMuxer::Substream to implement Async{Read,Write} #2707.
• More simplifications can be landed if there is consensus that we can remove this code, i.e. there are several single-variant enums now which could be removed.

Links to any relevant issues

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]

I am torn. On the one hand I am in favor of deleting the dead code, on the other hand, would be a bummer to have two consecutive breaking changes, one removing it, one re-introducing it ones supported in libp2p-quic.

@elenaf9 what are your thoughts on this? Do you expect we can support this feature in libp2p-quic in the near future, say till early 2023?

[thomaseizinger]

I am torn. On the one hand I am in favor of deleting the dead code, on the other hand, would be a bummer to have two consecutive breaking changes, one removing it, one re-introducing it ones supported in libp2p-quic.

At least in the current version of this PR, the functions on ConnectionHandler and NetworkBehaviour are only deprecated, not removed. We could remove that attribute again and pretend like the code is called but it actually won't be? :D

@elenaf9 what are your thoughts on this? Do you expect we can support this feature in libp2p-quic in the near future, say till early 2023?

To add to this question: Will we support this feature via the StreamMuxer interface? AFAIK, QUIC shares the same object between Transport and StreamMuxer. Perhaps this feature can also be supported via a TransportEvent::ListenerAddressChange variant?

It feels a bit odd that the StreamMuxer has to report the address change. Yes it wraps the connection but in regards to its other concerns, roaming seems to be unrelated?

[elenaf9]

@elenaf9 what are your thoughts on this? Do you expect we can support this feature in libp2p-quic in the near future, say till early 2023?

Yes I think we can support it. Not necessarily in the first version, but in near future afterwards. I believe the reason that we currently don't do it is because quinn-proto does not emit an event if the remote's address changed. So right now we'd have to manually poll quinn_proto::Connection::remote_address, which is not ideal. But looking at quinn-rs/quinn#1095 it seems like the quinn maintainers would be open for adding such an event. I think that's doable given that they already handle if a connection migrates to a new address: https://github.com/quinn-rs/quinn/blob/cd00119d254f9442618a7f1b8f748dcb9f309740/quinn-proto/src/connection/mod.rs#L2617. CC @kpp.

To add to this question: Will we support this feature via the StreamMuxer interface? AFAIK, QUIC shares the same object between Transport and StreamMuxer. Perhaps this feature can also be supported via a TransportEvent::ListenerAddressChange variant?

Not sure if I understand what you mean. From how you named the variant I would assume that you are referring to address changes on the local node, e.g. because we switched network. But I believe the StreamMuxerEvent::AddressChange refers to the remote's address changing on an established connection. This we can not handle in the Transport, since a transport is not aware of established connections, it just produces futures that eventually result in a connection. So in our QuicTransport we only manage the Endpoint, and the QuicMuxer manages the connections.
Am I missing something?

[kpp]

Yes I think we can support it. Not necessarily in the first version, but in near future afterwards.

I agree

[thomaseizinger]

All good. I think the solution in #2724 is superior to a design without this variant anyway :)

Going to close as a result.