fix(iroh): only switch paths if the new path is actually better

For now the restriction is that for direct to direct path switching latency must be at least 5ms better.

Author: dignifiedquire

Cargo.lock

@@ -1,6 +1,6 @@
 use std::{
     collections::{BTreeSet, VecDeque},
-    net::SocketAddr,
+    net::{SocketAddr, SocketAddrV4, SocketAddrV6},
     pin::Pin,
     sync::Arc,
     task::Poll,
@@ -74,6 +74,12 @@ mod path_state;
 /// in a high frequency, and to keep data about previous path around for subsequent connections.
 const ACTOR_MAX_IDLE_TIMEOUT: Duration = Duration::from_secs(60);
 
+/// The minimum RTT difference to make it worth switching IP paths
+const RTT_SWITCHING_MIN_IP: Duration = Duration::from_millis(5);
+
+/// How much do we prefer IPv6 over IPv4?
+const IPV6_RTT_ADVANTAGE: Duration = Duration::from_millis(3);
+
 /// A stream of events from all paths for all connections.
 ///
 /// The connection is identified using [`ConnId`].  The event `Err` variant happens when the
@@ -856,34 +862,58 @@ impl RemoteStateActor {
             .filter_map(|(addr, rtts)| rtts.into_iter().min().map(|rtt| (addr, rtt)))
             .collect();
 
-        // Find the fastest direct or relay path.
-        const IPV6_RTT_ADVANTAGE: Duration = Duration::from_millis(3);
-        let direct_path = path_rtts
+        // Find the fastest direct IPv4 path.
+        let direct_path_ipv4 = path_rtts
             .iter()
-            .filter(|(addr, _rtt)| addr.is_ip())
-            .map(|(addr, rtt)| {
-                if addr.is_ipv4() {
-                    (*rtt + IPV6_RTT_ADVANTAGE, addr)
+            .filter_map(|(addr, rtt)| {
+                if let transports::Addr::Ip(SocketAddr::V4(addr)) = *addr {
+                    Some((addr, *rtt))
                 } else {
-                    (*rtt, addr)
+                    None
                 }
             })
-            .min();
-        let selected_path = direct_path.or_else(|| {
-            // Find the fasted relay path.
-            path_rtts
-                .iter()
-                .filter(|(addr, _rtt)| addr.is_relay())
-                .map(|(addr, rtt)| (*rtt, addr))
-                .min()
-        });
-        if let Some((rtt, addr)) = selected_path {
+            .min_by_key(|(_addr, rtt)| *rtt);
+
+        // Find the fastest direct IPv6 path.
+        let direct_path_ipv6 = path_rtts
+            .iter()
+            .filter_map(|(addr, rtt)| {
+                if let transports::Addr::Ip(SocketAddr::V6(addr)) = *addr {
+                    Some((addr, *rtt))
+                } else {
+                    None
+                }
+            })
+            .min_by_key(|(_addr, rtt)| *rtt);
+
+        // Find the fastest relay path.
+        let relay_path = path_rtts
+            .iter()
+            .filter(|(addr, _rtt)| addr.is_relay())
+            .min_by_key(|(_addr, rtt)| *rtt)
+            .map(|(addr, rtt)| (addr.clone(), *rtt));
+
+        let current_path = self
+            .selected_path
+            .get()
+            .and_then(|addr| path_rtts.get(&addr).copied().map(|rtt| (addr, rtt)));
+        let selected_path = select_best_path(
+            current_path.clone(),
+            direct_path_ipv4,
+            direct_path_ipv6,
+            relay_path,
+        );
+
+        // Apply our new path
+        if let Some((addr, rtt)) = selected_path {
             let prev = self.selected_path.set(Some(addr.clone()));
             if prev.is_ok() {
                 debug!(?addr, ?rtt, ?prev, "selected new path");
             }
-            self.open_path(addr);
-            self.close_redundant_paths(addr);
+            self.open_path(&addr);
+            self.close_redundant_paths(&addr);
+        } else {
+            trace!(?current_path, "keeping current path");
         }
     }
 
@@ -932,6 +962,83 @@ impl RemoteStateActor {
     }
 }
 
+/// Returns `Some` if a new path should be selected, `None` if the `current_path` should
+/// continued to be used.
+fn select_best_path(
+    current_path: Option<(transports::Addr, Duration)>,
+    direct_path_ipv4: Option<(SocketAddrV4, Duration)>,
+    direct_path_ipv6: Option<(SocketAddrV6, Duration)>,
+    new_relay_path: Option<(transports::Addr, Duration)>,
+) -> Option<(transports::Addr, Duration)> {
+    // Determine the best new IP path.
+    let best_ip_path = match (direct_path_ipv4, direct_path_ipv6) {
+        (Some((addr_v4, rtt_v4)), Some((addr_v6, rtt_v6))) => {
+            Some(select_v4_v6(addr_v4, rtt_v4, addr_v6, rtt_v6))
+        }
+        (None, Some((addr_v6, rtt_v6))) => Some((addr_v6.into(), rtt_v6)),
+        (Some((addr_v4, rtt_v4)), None) => Some((addr_v4.into(), rtt_v4)),
+        (None, None) => None,
+    };
+    let best_ip_path = best_ip_path.map(|(addr, rtt)| (transports::Addr::Ip(addr), rtt));
+
+    match current_path {
+        None => {
+            // If we currently have no path
+            if best_ip_path.is_some() {
+                // Use the best IP path
+                best_ip_path
+            } else {
+                // Use the new relay path
+                new_relay_path
+            }
+        }
+        Some((transports::Addr::Relay(..), _)) => {
+            // If we have a current path, but it is relay
+            if best_ip_path.is_some() {
+                //  Use the best IP path
+                best_ip_path
+            } else {
+                // Use the new relay path
+                new_relay_path
+            }
+        }
+        Some((transports::Addr::Ip(_), current_rtt)) => {
+            match &best_ip_path {
+                Some((_, new_rtt)) => {
+                    // Comparing available IP paths
+                    if current_rtt >= *new_rtt + RTT_SWITCHING_MIN_IP {
+                        // New IP path is faster
+                        best_ip_path
+                    } else {
+                        // New IP is not faster
+                        None
+                    }
+                }
+                None => {
+                    // No new IP path, don't switch away
+                    None
+                }
+            }
+        }
+    }
+}
+
+/// Compare two IP addrs v4 & v6 and selects the "best" one.
+///
+/// This prefers IPv6 paths over IPv4 paths.
+fn select_v4_v6(
+    addr_v4: SocketAddrV4,
+    rtt_v4: Duration,
+    addr_v6: SocketAddrV6,
+    rtt_v6: Duration,
+) -> (SocketAddr, Duration) {
+    if rtt_v6 <= rtt_v4 + IPV6_RTT_ADVANTAGE {
+        (addr_v6.into(), rtt_v6)
+    } else {
+        (addr_v4.into(), rtt_v4)
+    }
+}
+
 fn send_datagram<'a>(
     sender: &'a mut TransportsSender,
     dst: transports::Addr,
@@ -1319,3 +1426,32 @@ fn to_transports_addr(
         }
     })
 }
+
+#[cfg(test)]
+mod tests {
+    use std::net::{Ipv4Addr, Ipv6Addr};
+
+    use super::*;
+
+    #[test]
+    fn test_select_v4_v6_addr() {
+        let v4 = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1);
+        let v6 = SocketAddrV6::new(Ipv6Addr::LOCALHOST, 1, 0, 0);
+
+        // Same RTT, prefer v6
+        let (addr, rtt) =
+            select_v4_v6(v4, Duration::from_millis(10), v6, Duration::from_millis(10));
+        assert_eq!(addr, v6.into());
+        assert_eq!(rtt, Duration::from_millis(10));
+
+        // v4 better
+        let (addr, rtt) = select_v4_v6(v4, Duration::from_millis(1), v6, Duration::from_millis(10));
+        assert_eq!(addr, v4.into());
+        assert_eq!(rtt, Duration::from_millis(1));
+
+        // v6 better
+        let (addr, rtt) = select_v4_v6(v4, Duration::from_millis(10), v6, Duration::from_millis(1));
+        assert_eq!(addr, v6.into());
+        assert_eq!(rtt, Duration::from_millis(1));
+    }
+}

iroh/src/magicsock/remote_map/remote_state.rs

@@ -467,13 +467,6 @@ impl Addr {
         matches!(self, Self::Ip(_))
     }
 
-    pub(crate) fn is_ipv4(&self) -> bool {
-        match self {
-            Addr::Ip(socket_addr) => socket_addr.is_ipv4(),
-            Addr::Relay(_, _) => false,
-        }
-    }
-
     /// Returns `None` if not an `Ip`.
     pub(crate) fn into_socket_addr(self) -> Option<SocketAddr> {
         match self {