Transport trait

[dvc94ch]

There are still a couple of todos, but would like to clear some questions in the meantime:

• Is the new Transport/Dialer trait ok
• Is using async-io ok
• tcp transport port reuse code needs review
• dns transport uses async-std-resolver based on trustdns

TODO

• fix tests
• use new dialer in swarm
• test quic based transport in ipfs-embed
• use released async-io

part one of #1722, closes #1563, closes #1787

[tomaka]

Can you indicate what is the purpose of this PR?

[dvc94ch]

The purpose of this PR to allow port reuse for outgoing tcp connections and support for quic. As you know quic doesn't fit naturally with the current transport traits, and the current quic implementation only allows for one quic listener and dials on the same port as that listener ignoring the multiaddr passed to listen_on. I think quic should support multiple listeners for ipv4 and ipv6 for example.

In addition to that the translate address logic is not correct for quic or tcp with port reuse. When traversing a nat the nat may assign a new port, since we are using the same port for incoming and outgoing connections the reason why the observed address is different is because of the nat.

The reason for using async-io is because the macros are a nuissance when debugging (can't insert a panic or unwrap because it will give me the line where the macro was declared) and obfuscate the code.

Some more background are in #1722, #1563 and #1787, and the closed pr #1667

[dvc94ch]

I guess the way to review this would be to start with commit e8dbdec which shows how it all fits together. The first two commits are also fairly small they make the changes needed for the feature. The rest of the commits are updating the transports to the new api and fixing the tests. I guess most of the added code are from tests and a little more boilerplatte in the transports required for extracting the dialer into a separate struct.

[dvc94ch]

@tomaka I assume you don't like the new transport trait much? It's about letting you dial on the same port as the listening port. The dialer returned by the transport will dial on an arbitrary port. If you try to adapt the quic branch to this new api I think you'll find it's a more natural fit. If you have an alternative suggestion, I'm open to suggestions...

[romanb]

The following are some high-level comments after reading through the related issues and skimming the PR.

I do think that eventually it would be nice to permit port reuse for both listening and dialing, e.g. via SO_REUSEPORT where available, but I'm not sure how strong the motivation is right now. I'm currently aware of the following two motivations / use-cases:

1. Proper (parallel) TCP hole punching for NAT traversal (e.g. as defined in NAT Hole Punching Revisited).

2. The "classical" use-case of listen()ing on the same port from any number of threads for servers with a very high inbound connection throughput where a single listener thread may otherwise become a bottleneck in the application.

Regarding 1., I think the main problem that needs to be solved first is the handling of TCP simultaneous open that can frequently occur with parallel TCP hole punching. Many if not most libp2p protocols, like multistream-select and noise, require a clear separation of roles between dialer and listener (or initiator and responder) provided by the transport for each connection. With a TCP simultaneous open both sides end up as the dialer (/ initiator). This is something that may be resolved on the multistream-select level as proposed in libp2p/specs#196 but that is still lacking implementations and adoption, probably due to more focus on a multistream-select "2.0" which is also still in the works. It would certainly be great to sort this out and eventually have built-in support in libp2p for parallel TCP hole punching including a minimal implementation of the necessary "mediator" node (not a full relay, as defined in e.g. NAT Hole Punching Revisited), but that requires sorting out TCP simultaneous open first.

Regarding 2., this seems to be mainly a potential performance improvement (see also quic-go/quic-go#2597). To be practically useful one would need to listen_on() from multiple threads which, apart from using multiple Networks or Swarms, I don't think the current libp2p-core design even facilitates, but I haven't spent much time thinking about it.

Is the new Transport/Dialer trait ok?

I think I don't fully understand the motivation behind changing the Transport interface yet. I would think that support for port-reuse for dialing and listening and SO_REUSEPORT should be transport-specific features and thus not be in some way a part of the Transport interface by dialing "through" listeners. So you would generally just configure a transport like TCP for port reuse before giving it to a Network or Swarm. What is preventing such an approach that is scoped to the TCP transport and QUIC? At least that seems to be how such functionality is integrated in e.g. go-libp2p. I also think that reusing the listening port as the source port when dialing should not be done by default, as it can prevent you from having more than one connection with a peer and, incidentally, the fact that it is enabled by default in go-tcp-transport seemingly exacerbates the problem with TCP simultaneous open. As an aside, with libp2p-core it is possible to perform the listening and dialing with the transport on your own followed by setting the connections of peers, though such APIs could be expanded further e.g. to allow adding such connections also to already connected peers.

It seems to me that a new implementation of the relay protocol as you mentioned in step 2 of #1722, and relay servers in general as already mentioned in #1722 (comment), may be the best starting point for better and interoperable NAT traversal with libp2p.

Is using async-io ok?

I'm personally also not fond of the macro-based abstraction for async-std vs tokio. However, I'm afraid that if someone wishes to use only tokio-based crates, any dependencies that are not already transitive dependencies of tokio and do not provide any functionality that tokio does not already have, are not expected to be in the dependency tree. That seems to include async-io. Similarly for the use of async-std-resolver that now always pulls in async-std for libp2p-dns, unless we declare that libp2p-dns inevitably needs features not available with tokio-based crates. Lastly there is also libp2p-mdns, which uses the same macro-based strategy for tokio support and shouldn't be left with the macros in case we decide to go with async-io instead of macros in all these cases. Maybe async-io is minimal enough that tokio users won't have objections. In any case, it would be good if this effort can be separated from the effort of NAT traversal and port reuse.

In addition to that the translate address logic is not correct for quic or tcp with port reuse. When traversing a nat the nat may assign a new port, since we are using the same port for incoming and outgoing connections the reason why the observed address is different is because of the nat.

I'm still trying to understand this point. My current understanding is the following: The current implementation of address_translation() assumes the listening ports are public (for anyone we wish to communicate with) and only performs an IP mapping. Under that assumption, it yields a publicly reachable address of the local node when given an observed address that was either received from a peer to whom the local node connected with a random connection-specific source port, without any NAT involved, or if additionally the local node is behind a basic NAT (i.e. no port mapping). It is thus used to try to obtain an "external" address of the local node that it can later advertise to others which may then attempt to connect to that address. Now if I read your changes correctly, when a listening port is reused as the source port when dialing a peer, and that peer reports back an "observed address" of the local node, we take that observed address as-is and later advertise it to others, even if it has a different port than our local listening port. So we assume that this port, different from the local listening port, is assigned by the NAT behind which the local node is sitting, but how can the local node be reachable on that port for other peers?

[dvc94ch]

So you would generally just configure a transport like TCP for port reuse before giving it to a Network or Swarm.

I'm not sure it's that easy. The main reason for the current approach is that it works, it was a little tricky to get it to work. One problem is that there can be multiple listeners for the same transport. So the transport would need to know it's listening ports. Some examples:

let config = TcpConfig::new().port_reuse(true);
let stream1 = config.listen_on(ip4);
let stream2 =  config.listen_on(ip6);
let fallback = TcpConfig::new();
let transport = config.or_transport(fallback);
transport.dial(ip4); // dial on ip4
transport.dial(same ip4); // dial on fallback

let config = TcpConfig::new().port_reuse(true);
let stream1 = config.listen_on(ip4);
let fallback = TcpConfig::new();
let transport = config.or_transport(fallback);
transport.dial(ip6); // dial on fallback

So we assume that this port, different from the local listening port, is assigned by the NAT behind which the local node is sitting, but how can the local node be reachable on that port for other peers?

We need to distinguish between two kinds of nat's. Endpoint independent and endpoint dependent. In an endpoint independent nat we have a map from (source_ip, source_port) -> nat port, so as long as we use the same source port for all connections we have the same external port. An endpoint dependent nat (or symmetric nat) maps from (source_ip, source_port, dest_ip, dest_port) -> nat port. These nat's can only be traversed by relay (or in some cases guessing the external port and trying them). So to dial a peer behind an endpoint independent nat we need the (nat_ip, nat_port) which will be the same for all connections. The requirement to start the connection simultaneously (hole punching) is for traversing a firewall (that is likely located behind the nat). In summary through the combination of hole punching and the observed port we can traverse any firewall/eip nat combination. For a classification and solutions to the problem [0] is a great resource.

In any case, it would be good if this effort can be separated from the effort of NAT traversal and port reuse.

Sounds reasonable, but I guess some more discussion is needed to determine if that is the way forward? With a little bit more code async-std-resolver can be removed and async-io + trust-dns-resolver can be used directly.

• [0] https://tailscale.com/blog/how-nat-traversal-works/

[romanb]

We need to distinguish between two kinds of nat's. Endpoint independent and endpoint dependent. [..]

Thanks for pointing that out, I did in fact not consider NATs that do endpoint-independent filtering of inbound traffic. However, based on all the sources I've seen, including the one you linked to, such NATs are very rare. In contrast, endpoint-independent mapping combined with endpoint-dependent filtering seems to be a very common combination that can be successfully traversed with hole-punching techniques, e.g. "parallel TCP hole punching" (if the TCP simultaneous open were resolved first for libp2p). Symmetric NATs, i.e. that also do endpoint-dependent mapping, are obviously the most annoying and usually need a full relay to overcome. That is not to say that it is not useful to make it easy to traverse NATs that conveniently do both endpoint-independent mapping & filtering by just advertising the observed mapped port to all peers, as I now understand you suggest to do here when port-reuse is "enabled", it just doesn't seem to get us very far in terms of NAT traversal overall.

I'm not sure it's that easy. The main reason for the current approach is that it works, it was a little tricky to get it to work. One problem is that there can be multiple listeners for the same transport. So the transport would need to know it's listening ports. [..]

I didn't mean to imply that it is necessarily easy, but am still hoping that there is a better solution that consolidates the required changes on a smaller scale. There is on one hand the configuration of port-reuse on a concrete transport like TCP (where I still think it shouldn't be the default), and with that the decision of the transport and / or user code which ports to use for listening and dialing actions. On the other hand there is the "address translation" of observed addresses under consideration of the listener addresses and ports as well as transport configuration and possibly other transport state. So rather than just calling the current simple IP address_translation() the Swarm may call something like Network::translate_address(observed_addr) which in turn calls Transport::translate_address(listen_addrs, observed_addr). The default implementation on the trait could just use the current simple IP address_translation() while the TCP transport could further do the desired port (un)mapping. A concrete transport may certainly need to hold shared state across its instances to have the information necessary for such a translation, or to decide which source port to set on an outgoing connection. Just a rough idea that may or may not lead somewhere. At least to me the introduction of Transport::Dialer seems off-putting due to the increase in API complexity and follow-up boilerplate, without wanting in any way to diminish the effort you've obviously put into it.

[dvc94ch]

it just doesn't seem to get us very far in terms of NAT traversal overall.

well it's a first step. endpoint dependent filtering can be overcome by simultaneously opening connections from both parties, however that requires an external coordination server. the address translation performed by the default address_translation is only useful for tcp without port reuse and bound to a public ip, so maybe that should be part of the tcp transport, keeping the default as no-translation. Assuming udp based transports like quic gain popularity it seems like a very special tcp case. But the approach seems good.

At least to me the introduction of Transport::Dialer seems off-putting due to the increase in API complexity and follow-up boilerplate

If I understand correctly you're suggesting something like the following for both tcp and quic:

#[derive(Clone)]
struct TcpTransport {
    config: Arc<TcpConfig>,
    listeners: Arc<Mutex<Vec<Multiaddr>>>,
}
let transport = TcpConfig::new().to_transport();

The reason this approach was dismissed by me was because this would be the only place a Mutex is used in libp2p and essentially it's a waste since polling the swarm is &mut. But it is certainly doable and I can make the changes.

So I suggest the following path forward:

1. migrate to async-io to avoid the macros (probably leave the dns transport as is since I shouldn't need to touch it much given the new approach)
2. move the address translation logic into tcp and make it the responsibility of the transport following your suggested api
3. add port reuse to the tcp transport using the above mentioned approach

Discovering a relay server, registering with the relay server, detecting if the nat is eip and hole punching or relaying connections using the relay is left as issues for follow-up work.

NOTE: the reason for tcp port reuse is because it is a requirement for tcp hole punching to work

[romanb]

the address translation performed by the default address_translation is only useful for tcp without port reuse and bound to a public ip, so maybe that should be part of the tcp transport, keeping the default as no-translation.

Sounds good to me.

If I understand correctly you're suggesting something like the following for both tcp and quic: [..]

I guess, but since the TcpConfig is the Transport in the current design, more something like

pub struct TcpConfig {
    listen_addrs: Arc<RwLock<Vec<Multiaddr>>>,
    ... existing config options
}

as I would hope RwLock would suffice and the code paths that access it only run when port-reuse is enabled. It should also suite the strong read-bias for this field very well. I don't expect any performance problems really, but if performance unexpectedly turns out to be a problem, even a lock-free data structure could be an option. It does seem conceptually right to me that, if a transport needs to consult all its listening addresses e.g. to choose an appropriate source port for outgoing connections or translation of observed addresses, that it needs to keep track of these.

The reason this approach was dismissed by me was because this would be the only place a Mutex is used in libp2p and essentially it's a waste since polling the swarm is &mut.

I understand that uncontended locks can be a code smell, but the APIs are such that you can use (cloned) transports given to a Swarm or Network outside of it as well. At least the libp2p-core API, though not currently libp2p-swarm, further allows feeding connections thus established (possibly concurrently) back into the Network, as I hinted at earlier.

So I suggest the following path forward: [..]

Sounds good to me, though I would suggest that each of 1. and 2. + 3. be separate PRs for ease of comparison and separating the discussions of these different approaches. I assume that you would like the PR for 2 + 3 to be based on the changes in 1. for convenience and to avoid resolving conflicts if both are accepted. Nevertheless it may be preferable to not make the PR for 2 + 3 be dependent on the acceptance of the PR for 1.. While I may have a preference for async-io over the macros, it does add new dependencies that may be controversial - at least I think we need more feedback from more people, which a PR dedicated to that change hopefully brings forward.

Discovering a relay server, registering with the relay server, detecting if the nat is eip and hole punching or relaying connections using the relay is left as issues for follow-up work.

👍