transports/tcp: simplify IfWatcher integration

[elenaf9]

Description

Update to if-watch version v2.0.0, which allows us to simplify our integration in the tcp transport quite a bit:

• Since IfWatcher::new is not async anymore we do not need the IfWatch wrapper and related logic anymore
• Use if_watch::IfWatcher for both runtimes. According to this discussion, this should be ok now: transports/quic: Add implementation based on quinn-proto #2289 (comment).
  However, I just saw now that for cfg(not(any(target_os = "ios", target_os = "linux", target_os = "macos", target_os = "windows"))) if-watch still uses async-io. Given that this won't apply for the vast majority of users I'd say this is acceptable. Alternative is to do feature request: expose tokio vs async-global-executor feature flags if-watch#21 before continuing here.
• Remove InAddr and instead manage the IfWatcher directly in the TcpListenStream. In case of InAddr::One directly push an TransportEvent::NewAddress into our event queue when creating the listener.

Related discussions: #2289 (comment).

Open Questions

• We could remove InAddr completely and in case of InAddr::One just directly push an TransportEvent::NewAddress into our event queue when creating the listener. See elenaf9@c4f2124. But not sure if that really simplifies things. I guess for other transports that don't have whole port-reuse logic this might make more sense.
  Edit: Done now in 64a3165.

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• A changelog entry has been made in the appropriate crates

[thomaseizinger]

This looks like a really cool simplification!

[mxinden]

• However, I just saw now that for cfg(not(any(target_os = "ios", target_os = "linux", target_os = "macos", target_os = "windows"))) if-watch still uses async-io. Given that this won't apply for the vast majority of users I'd say this is acceptable. Alternative is to do fix

This seems fine to me.

[thomaseizinger]

This is a really great improvement!

I've added some stylistic comments.

[thomaseizinger]

• We could remove InAddr completely and in case of InAddr::One just directly push an TransportEvent::NewAddress into our event queue when creating the listener. See elenaf9@c4f2124. But not sure if that really simplifies things. I guess for other transports that don't have whole port-reuse logic this might make more sense.

I've only just noticed now. I think that is a good idea!

[thomaseizinger]

Note: I've edited your description so we don't actually close libp2p/if-watch#21 with merging this.

[mxinden]

@elenaf9 let me know if you want me to take another look here.

[thomaseizinger]

Great improvement!

[mxinden]

Looks good to me.

Have you tested this pull request with both tokio and async-std as a runtime using a wildcard IP address?

If yes, this is ready to merge from my end.

[melekes]

shouldn't we check if port is unspecified too?

[mxinden]

That was my first thought as well. But while an unspecified IP results in one IP per (potentially future) interface, an unspecified port results in a single randomly chosen port.

@elenaf9 @melekes please correct me in case I am missing something here.

[mxinden]

an unspecified port results in a single randomly chosen port.

Well, I am not sure what the OS does when it can not find a unique port number that is free on all interfaces. I am guessing that it will choose a different port number per interface. Though still only one per interface.

[elenaf9]

That was my first thought as well. But while an unspecified IP results in one IP per (potentially future) interface, an unspecified port results in a single randomly chosen port.

Yes exactly.

[elenaf9]

Have you tested this pull request with both tokio and async-std as a runtime using a wildcard IP address?

Yes, though on my local machine (linux). But our CI also tests both runtimes in the tcp test, which includes tests with wildcard addresses. But those tests only test the tcp transport directly and not within a swarm.
Lmk if you want me to add tests that integrate the TCP transport in the swarm.

[mxinden]

Have you tested this pull request with both tokio and async-std as a runtime using a wildcard IP address?

Yes, though on my local machine (linux). But our CI also tests both runtimes in the tcp test, which includes tests with wildcard addresses. But those tests only test the tcp transport directly and not within a swarm. Lmk if you want me to add tests that integrate the TCP transport in the swarm.

I think this is fine as is. Thanks.

[mxinden]

Thanks for thinking this all the way through!

[mxinden]

CI failure is unrelated. #2869 released v0.48.0. Thus this pull request updates the version to v0.49.0. Problem is, that https://github.com/libp2p/test-plans still operates with v0.48.0.

As far as I can tell this is a chicken-and-egg problem. We can't merge with a green CI here as the Testground tests are failing. We can not merge a patch to libp2p/test-plans as the version of libp2p/rust-libp2p is not yet bumped to v0.49.0.

I don't think this is a big issue. Will need to think about how to solve it long term. //CC @laurentsenta in case you have an idea.

I will merge here. I will create a pull request against libp2p/test-plans next.