-
Notifications
You must be signed in to change notification settings - Fork 976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transport: remove address_translation
functionality from Transport
trait
#3953
Comments
@mxinden Do you have any input on this? |
Previously, a `NetworkBehaviour` could report an `AddressScore` for an external address. This score was a `u32` and addresses would be ranked amongst those. In reality, an address is either confirmed to be publicly reachable (via a protocol such as AutoNAT) or merely represents a candidate that might be an external address. In a way, addresses are guilty (private) until proven innocent (publicly reachable). When a `NetworkBehaviour` reports an address candidate, we perform address translation on it to potentially correct for ephemeral ports of TCP. These candidates are then injected back into the `NetworkBehaviour`. Protocols such as AutoNAT can use these addresses as a source for probing their NAT status. Once confirmed, they can emit a `ToSwarm::ExternalAddrConfirmed` event which again will be passed to all `NetworkBehaviour`s. This simplified approach will allow us implement Kademlia's client-mode (#2032) without additional configuration options: As soon as an address is reported as publicly reachable, we can activate server-mode for that connection. Related: #3877. Related: #3953. Related: #2032. Related: libp2p/go-libp2p#2229. Co-authored-by: Max Inden <[email protected]> Pull-Request: #3954.
Apart from removing the "false-sense of extensibility", what problem would it solve to move the address translation to One could argue that the "false-sense of extensibility" applies to the |
Yes but it still makes sense to split them into different crates to segregate dependencies from one another. Plus, as we can see in the example of QUIC, one can implement the same transport differently. I'd argue that there is only one correct way to implement address translation and |
I am not sure this is true. In the case of a NAT in front of the local node the observed port and the listening port will not be the same and thus the former should not be replaced in favor of the latter as is implemented today (see rust-libp2p/transports/tcp/src/lib.rs Lines 535 to 538 in 8b91c89
|
Hmm. We could detect port-reuse in In other words, we could correctly translate observed addresses if outgoing connections had a local address. |
So if I understand correctly, the only blocker for this is that |
Technically, the setting for port-reuse is per connection right? In theory, a transport could allow for changing this at runtime. Thus, a better model might be if Additionally, it would also be useful if that returned object would give you access to the local address of the connection. That is a piece of information that we are currently not capturing / exposing. What do you think of this idea? All connections are either memory, TCP or UDP and the concept of port-reuse applies to all of them. Yes, even to memory because our implementation of We could introduce the following: pub trait PhysicalConnection {
fn uses_ephemeral_port(&self) -> bool;
fn local_addr(&self) -> Multiaddr;
} And require |
@mxinden Currently, we rely on In the case of port-reuse, this allows for hole-punching to work. Regardless of which port the NAT device assigned to the out-going connection, it will map it back correctly to our local port. If we don't have port-reuse enabled, is Would it be fair to say that doing |
In the case of a server with a public interface Does that make sense @thomaseizinger? |
It doesn't to me. If the local interface is a public one, we can just query its IP address via the kernel, right? Learning our address by having a remote node tell us what they observed is only useful in the case of a NAT device inbetween us and the remote peer, isn't it? As soon as we are not in the same subnet as the remote peer, any form of If on the other hand we have port-reuse enabled and a port forwarding configured in the NAT device, there is at least the possibility that the device will perform a "reverse" port mapping and use the same outgoing port that it would route incoming connections from. For example, lets say we port-forward 9331 to 8080. With port-reuse enabled, an outgoing connection will originate from 8080 which can then be mapped to 9331 by the NAT device. In such a scenario, If port-reuse is disabled, a first outgoing connection might originate from 54321 and a second one from 46342. Translating these to 8080 with the observed address doesn't make any sense because the configured port forwarding is actually 9331. Address translation in this case only works if the user configured a port-forwarding of 8080 -> 8080. |
Previously, a `NetworkBehaviour` could report an `AddressScore` for an external address. This score was a `u32` and addresses would be ranked amongst those. In reality, an address is either confirmed to be publicly reachable (via a protocol such as AutoNAT) or merely represents a candidate that might be an external address. In a way, addresses are guilty (private) until proven innocent (publicly reachable). When a `NetworkBehaviour` reports an address candidate, we perform address translation on it to potentially correct for ephemeral ports of TCP. These candidates are then injected back into the `NetworkBehaviour`. Protocols such as AutoNAT can use these addresses as a source for probing their NAT status. Once confirmed, they can emit a `ToSwarm::ExternalAddrConfirmed` event which again will be passed to all `NetworkBehaviour`s. This simplified approach will allow us implement Kademlia's client-mode (libp2p#2032) without additional configuration options: As soon as an address is reported as publicly reachable, we can activate server-mode for that connection. Related: libp2p#3877. Related: libp2p#3953. Related: libp2p#2032. Related: libp2p/go-libp2p#2229. Co-authored-by: Max Inden <[email protected]> Pull-Request: libp2p#3954.
Resolves: libp2p#4226. Resolves: libp2p#3953. Resolves: libp2p#3889. Pull-Request: libp2p#4568.
Description
Currently, the
Transport
trait exposes anaddress_translation
function that can compute the local listen address from an observed one. This is useful for transports such as TCP where - without port reuse - the port of an outgoing connection is ephemeral. Hence, it would be wrong to hand the observed address to another peer as a possible external address; they will never be able to dial us on this port.I am suggesting to remove this functionality from the
Transport
trait and instead put this functionality directly intolibp2p::Swarm
. At first, this might sound limiting: translation of these addresses is transport specific and users should be able to supply their own transport which might require implementing address translation.In reality, the set of transports we can perform address translation on is limited by the transports that the
multiaddr::Protocol
enum supports. Thus, delegating this to the actualTransport
implementation creates a false-sense of extensibility.The mapping algorithm is only specific to the transport technology. In fact, the existing
address_translation
function only does something meaningful for TCP and QUIC. For TCP, we are conditional on theport_reuse
configuration when in fact, also this is redundant. For TCP, we can always replace the observed port with the listen port. If we haveport_reuse
enabled in the transport, these ports will already be the same and the operation is effectively a no-op. If they are not the same, we will adjust the port to the correct one.Bear in mind that this does not guarantee that we are actually reachable only this address. This merely generates a candidate (see #3877 (comment)) that we might be reachable on. Actual reachability is subject to be tested with a protocol like AutoNAT.
Motivation
Simplify interfaces and code.
Are you planning to do it yourself in a pull request?
Yes, if we agree that the reasoning is sound.
The text was updated successfully, but these errors were encountered: