Skip to content
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

fix(quic): improve listener socket selection when dialing #4259

Open
mxinden opened this issue Jul 27, 2023 · 4 comments
Open

fix(quic): improve listener socket selection when dialing #4259

mxinden opened this issue Jul 27, 2023 · 4 comments

Comments

@mxinden
Copy link
Member

mxinden commented Jul 27, 2023

Problem

Say the local node listens on /ip4/0.0.0.0/udp/0/quic-v1. One would expect a consecutive dial to /ip4/127.0.0.1/udp/42/quic-v1 to use one of the listen addresses as the source address of the dial.

That is not the case today. Instead libp2p-quic will create a new quinn endpoint and thus a new UDP socket for the outgoing dial.

Why?

In TCP we track the specific listen addresses, even if the user calls listen_on with an unspecified address (e.g. 0.0.0.0). If the user calls listen_on with a specific IP address i.e. not a wildcard like 0.0.0.0 but e.g. 127.0.0.1 we track (here register) the specific address right away.

self.port_reuse.register(local_addr.ip(), local_addr.port());

In case where the user calls listen_on with an unspecified address (e.g. 0.0.0.0) we depend on if-watch, more specifically poll_if_watch to provide us with the concrete addresses:

/// Poll for a next If Event.
fn poll_if_addr(&mut self, cx: &mut Context<'_>) -> Poll<<Self as Stream>::Item> {
let if_watcher = match self.if_watcher.as_mut() {
Some(if_watcher) => if_watcher,
None => return Poll::Pending,
};
let my_listen_addr_port = self.listen_addr.port();
while let Poll::Ready(Some(event)) = if_watcher.poll_next_unpin(cx) {
match event {
Ok(IfEvent::Up(inet)) => {
let ip = inet.addr();
if self.listen_addr.is_ipv4() == ip.is_ipv4() {
let ma = ip_to_multiaddr(ip, my_listen_addr_port);
log::debug!("New listen address: {}", ma);
self.port_reuse.register(ip, my_listen_addr_port);

Either way, we end up tracking specific listen addresses with port-reuse enabled and can thus choose the right listener in local_dial_addr:

/// Selects a listening socket address suitable for use
/// as the local socket address when dialing.
///
/// If multiple listening sockets are registered for port
/// reuse, one is chosen whose IP protocol version and
/// loopback status is the same as that of `remote_ip`.
///
/// Returns `None` if port reuse is disabled or no suitable
/// listening socket address is found.
fn local_dial_addr(&self, remote_ip: &IpAddr) -> Option<SocketAddr> {
if let PortReuse::Enabled { listen_addrs } = self {
for (ip, port) in listen_addrs
.read()
.expect("`local_dial_addr` never panic while holding the lock")
.iter()
{
if ip.is_ipv4() == remote_ip.is_ipv4()
&& ip.is_loopback() == remote_ip.is_loopback()
{
if remote_ip.is_ipv4() {
return Some(SocketAddr::new(IpAddr::V4(Ipv4Addr::UNSPECIFIED), *port));
} else {
return Some(SocketAddr::new(IpAddr::V6(Ipv6Addr::UNSPECIFIED), *port));
}
}
}
}
None
}

In libp2p-quic we only track the initial address that the user provided on the listen_on call.

/// Channel used to send commands to the [`Driver`].
#[derive(Debug, Clone)]
pub(crate) struct Channel {
/// Channel to the background of the endpoint.
to_endpoint: mpsc::Sender<ToEndpoint>,
/// Address that the socket is bound to.
/// Note: this may be a wildcard ip address.
socket_addr: SocketAddr,
}

This might be a wildcard address. Later on, when dialing a 127.0.0.1 address, the check listen_addr.ip().is_loopback() == socket_addr.ip().is_loopback() discards the listener with the unspecified listen address, thus a new dial-only socket is used, even though one of the existing listener sockets is available.

This is e.g. problematic when trying to dial a node on 127.0.0.1 with the expectation that the dial uses one of the listen addresses as a source address.

How does go-libp2p solve this:

go-libp2p asks the OS for the correct interface given a destination address. It uses Google's routing package. See https://github.com/libp2p/go-libp2p/blob/260b9695cafdd8e35ec65b30ef153f0c15549c72/p2p/transport/quicreuse/reuse.go#L209 for concrete usage and implementation.

How to move forward?

I am not sure simply removing the listen_addr.ip().is_loopback() == socket_addr.ip().is_loopback() check is the way to go. This check is still helpful. E.g. say we have two listeners, one on localhost, one not on localhost. Ideally we would use the former when dialing.

We could mirror what libp2p-tcp does. That is, track the specified listen addresses reported by if-watch. Later, when dialing, choose the listener with the right address.

Ideally I would like to have the operating system choose. It has the most information about all available routes to each interface. In other words ideally we would do what go-libp2p does.

For now, I suggest we go with the libp2p-tcp approach. A second iteration would mirror what go-libp2p does.

Originally posted by @mxinden in #3454 (comment)

Thank you @kpp for surfacing this bug.

Thank you @marten-seemann for the quick help, pointing me to how go-libp2p solves this issue.

@arsenron
Copy link
Contributor

arsenron commented Aug 7, 2023

I would like to work on this but I have a question. If using the same approach as in libp2p-tcp, should we also have a configuration related to port_reuse: bool (false by default) as in libp2p::tcp::Config, or make it true by default so reuse an existing listener?

@mxinden
Copy link
Member Author

mxinden commented Aug 7, 2023

Great to hear that you are interested.

#4226

port_reuse in libp2p-quic will be introduced with #4226. Let's not do it as part of this issue. In other words, let's always use a listener if available for now.

Let me know if you have any other questions @arsenron.

@arsenron
Copy link
Contributor

arsenron commented Aug 8, 2023

@mxinden Could you please help me with a question?

Could we make it as simple as adding a separate field to a Listener something like is_loopback bool. So when we add a new listener we check either ip is loopback and set it to true, or later if IfWatcher returns us a new loopback address, we also set it to true. Then in case a remote address is also loopback, we just iterate listeners to find a listener which supports loopback, if any. Or am I missing something?

mergify bot pushed a commit that referenced this issue Aug 11, 2023
Tracked in #4259. Now if a listener supports loopback interfaces, when a remote address is also a loopback address, we reuse an existing listener.

Pull-Request: #4304.
@mxinden
Copy link
Member Author

mxinden commented Aug 11, 2023

Thank you @arsenron for the fix.

I will keep this issue open given that there is still the more sophisticated solution that go-libp2p follows.

How does go-libp2p solve this:

go-libp2p asks the OS for the correct interface given a destination address. It uses Google's routing package. See https://github.com/libp2p/go-libp2p/blob/260b9695cafdd8e35ec65b30ef153f0c15549c72/p2p/transport/quicreuse/reuse.go#L209 for concrete usage and implementation.

@arsenron let me know in case you want to contribute more to the project. The above more sophisticated solution might be an option. Or any of the other help-wanted issues.

https://github.com/libp2p/rust-libp2p/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22

thomaseizinger pushed a commit that referenced this issue Aug 20, 2023
Tracked in #4259. Now if a listener supports loopback interfaces, when a remote address is also a loopback address, we reuse an existing listener.

Pull-Request: #4304.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants