Skip to content

feat: implement rtt hints for nat traversal #3784

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.

Sign up for GitHub

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

Open
dignifiedquire wants to merge 2 commits into
base: feat-multipath
Choose a base branch
from
Open

feat: implement rtt hints for nat traversal #3784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2b9ee35
feat: implement rtt hints for nat traversal
dignifiedquire Dec 17, 2025
b926b91
impl todo
dignifiedquire Dec 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

18 changes: 9 additions & 9 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,17 @@ unused-async = "warn"


[patch.crates-io]
iroh-quinn = { git = "https://github.com/n0-computer/quinn", branch = "main" }
iroh-quinn-proto = { git = "https://github.com/n0-computer/quinn", branch = "main" }
iroh-quinn-udp = { git = "https://github.com/n0-computer/quinn", branch = "main" }
iroh-quinn = { git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }
iroh-quinn-proto = { git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }
iroh-quinn-udp = { git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }

netwatch = { git = "https://github.com/n0-computer/net-tools", branch = "main" }

# iroh-quinn = { path = "../quinn/quinn" }
# iroh-quinn-proto = { path = "../quinn/quinn-proto" }
# iroh-quinn-udp = { path = "../quinn/quinn-udp" }
# iroh-quinn = { path = "../iroh-quinn/quinn" }
# iroh-quinn-proto = { path = "../iroh-quinn/quinn-proto" }
# iroh-quinn-udp = { path = "../iroh-quinn/quinn-udp" }

# [patch."https://github.com/n0-computer/quinn"]
# iroh-quinn = { path = "../quinn/quinn" }
# iroh-quinn-proto = { path = "../quinn/quinn-proto" }
# iroh-quinn-udp = { path = "../quinn/quinn-udp" }
# iroh-quinn = { path = "../iroh-quinn/quinn" }
# iroh-quinn-proto = { path = "../iroh-quinn/quinn-proto" }
# iroh-quinn-udp = { path = "../iroh-quinn/quinn-udp" }
4 changes: 2 additions & 2 deletions iroh-relay/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ postcard = { version = "1", default-features = false, features = [
"use-std",
"experimental-derive",
] }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "main" }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }
rand = "0.9.2"
reqwest = { version = "0.12", default-features = false, features = [
"rustls-tls",
Expand Down
8 changes: 4 additions & 4 deletions iroh/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ n0-watcher = "0.6"
netwatch = { version = "0.12" }
pin-project = "1"
pkarr = { version = "5", default-features = false, features = ["relays"] }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "main" }
quinn-udp = { package = "iroh-quinn-udp", git = "https://github.com/n0-computer/quinn", branch = "main" }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints", default-features = false, features = ["rustls-ring"] }
quinn-proto = { package = "iroh-quinn-proto", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }
quinn-udp = { package = "iroh-quinn-udp", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }
rand = "0.9.2"
reqwest = { version = "0.12", default-features = false, features = [
"rustls-tls",
Expand Down Expand Up @@ -82,7 +82,7 @@ hickory-resolver = "0.25.1"
igd-next = { version = "0.16", features = ["aio_tokio"] }
netdev = { version = "0.39.0" }
portmapper = { version = "0.12", default-features = false }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main", default-features = false, features = ["runtime-tokio", "rustls-ring"] }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints", default-features = false, features = ["runtime-tokio", "rustls-ring"] }
tokio = { version = "1", features = [
"io-util",
"macros",
Expand Down
2 changes: 1 addition & 1 deletion iroh/bench/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ iroh = { path = "..", default-features = false }
iroh-metrics = { version = "0.37", optional = true }
n0-future = "0.3.0"
n0-error = "0.1.0"
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "main" }
quinn = { package = "iroh-quinn", git = "https://github.com/n0-computer/quinn", branch = "feat-rtt-hints" }
rand = "0.9.2"
rcgen = "0.14"
rustls = { version = "0.23.33", default-features = false, features = ["ring"] }
Expand Down
2 changes: 2 additions & 0 deletions iroh/src/magicsock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1415,6 +1415,8 @@ impl Actor {

// Notify all transports
self.network_change_sender.on_network_change(r);
// Notify connections
self.msock.remote_map.on_network_change(r);
}

#[cfg(not(wasm_browser))]
Expand Down
12 changes: 12 additions & 0 deletions iroh/src/magicsock/remote_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use self::remote_state::{RemoteStateActor, RemoteStateHandle};
use super::{
DirectAddr, MagicsockMetrics,
mapped_addrs::{AddrMap, EndpointIdMappedAddr, RelayMappedAddr},
net_report::Report,
};
use crate::discovery::ConcurrentDiscovery;

Expand Down Expand Up @@ -98,6 +99,17 @@ impl RemoteMap {
handles.retain(|_eid, handle| !handle.sender.is_closed())
}

pub(super) fn on_network_change(&self, report: &Report) {
let handles = self.actor_handles.lock().expect("poisoned");
for handle in handles.values() {
if let Some(sender) = handle.sender.get() {
sender
.try_send(RemoteStateMessage::NetworkChange(report.clone()))
.ok();
}
}
}

/// Returns the sender for the [`RemoteStateActor`].
///
/// If needed a new actor is started on demand.
Expand Down
77 changes: 61 additions & 16 deletions iroh/src/magicsock/remote_map/remote_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ use crate::{
remote_map::Private,
transports::{self, OwnedTransmit, TransportsSender},
},
net_report::Report,
util::MaybeFuture,
};

Expand Down Expand Up @@ -190,6 +191,9 @@ pub(super) struct RemoteStateActor {
//
/// Stream of discovery results, or always pending if discovery is not running.
discovery_stream: DiscoveryStream,

/// Last Net Report
last_report: Option<Report>,
}

impl RemoteStateActor {
Expand Down Expand Up @@ -220,6 +224,7 @@ impl RemoteStateActor {
scheduled_open_path: None,
pending_open_paths: VecDeque::new(),
discovery_stream: Either::Left(n0_future::stream::pending()),
last_report: None,
}
}

Expand Down Expand Up @@ -368,6 +373,9 @@ impl RemoteStateActor {
};
tx.send(info).ok();
}
RemoteStateMessage::NetworkChange(report) => {
self.last_report.replace(report);
}
}
}

Expand Down Expand Up @@ -707,7 +715,26 @@ impl RemoteStateActor {
.map(|daddr| daddr.addr)
.collect::<BTreeSet<_>>();

match conn.initiate_nat_traversal_round() {
let all_rtts = self.calculate_path_rtts();
let relay_mapped_addrs = self.relay_mapped_addrs.clone();
let relay_latencies = self.last_report.as_ref().map(|r| r.relay_latency.clone());
let rtt_hints = |addr: &SocketAddr| {
let addr = relay_mapped_addrs.to_transport_addr(*addr)?;
match addr {
transports::Addr::Relay(url, _id) => {
// Relay case, grab the latency from net_report
relay_latencies.as_ref().and_then(|l| l.get(&url))
}
Comment on lines +724 to +727
Copy link
Member

@matheus23 matheus23 Dec 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relay latency is not the same thing as the RTT to the peer via the relay, because that depends on the (potentially two!) relays involved in going back and forth.

Besides, I don't think we need an RTT hint for any relay addresses, because they will/should never be used in holepunching.

addr @ transports::Addr::Ip(_) => {
// IP case, grab the worst latency we currently have
all_rtts
.get(&addr)
.and_then(|rtts| rtts.iter().max().copied())
Comment on lines +729 to +732
Copy link
Member

@matheus23 matheus23 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be .min(Duration::from_millis(333)) additionally, just so we don't accidentally balloon the PTO in case there was some kind of weird bad RTT in our IP paths.

Copy link
Member

@matheus23 matheus23 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, IIUC, this won't help broken paths, right? You only have a single connection in the RemoteState, and the remote address that doesn't work for holepunching won't have an RTT estimate. So you're back to having a 333ms initial RTT estimate for remote addresses that don't work. Am I missing something?

Copy link
Contributor Author

@dignifiedquire dignifiedquire Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the probes will timeout faster for broken paths, that’s the whole point of this PR

Copy link
Member

@matheus23 matheus23 Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But... the way the code is written, I don't see how that's going to be the case?

}
}
};

match conn.initiate_nat_traversal_round(rtt_hints) {
Ok(remote_candidates) => {
let remote_candidates = remote_candidates
.iter()
Expand Down Expand Up @@ -766,6 +793,21 @@ impl RemoteStateActor {
.private_socket_addr(),
};

let rtt_hint = match &open_addr {
transports::Addr::Relay(url, _) => {
// Relay case, grab the latency from net_report
self.last_report
.as_ref()
.and_then(|r| r.relay_latency.get(url))
}
addr @ transports::Addr::Ip(_) => {
let all_rtts = self.calculate_path_rtts();
all_rtts
.get(addr)
.and_then(|rtts| rtts.iter().max().copied())
}
};

for (conn_id, conn_state) in self.connections.iter_mut() {
if conn_state.path_ids.contains_key(open_addr) {
continue;
Expand All @@ -776,7 +818,8 @@ impl RemoteStateActor {
if conn.side().is_server() {
continue;
}
let fut = conn.open_path_ensure(quic_addr, path_status);

let fut = conn.open_path_ensure(quic_addr, path_status, rtt_hint);
match fut.path_id() {
Some(path_id) => {
trace!(?conn_id, ?path_id, "opening new path");
Expand Down Expand Up @@ -905,6 +948,20 @@ impl RemoteStateActor {
}
}

fn calculate_path_rtts(&self) -> FxHashMap<transports::Addr, Vec<Duration>> {
let mut rtts: FxHashMap<transports::Addr, Vec<Duration>> = FxHashMap::default();
for conn_state in self.connections.values() {
let Some(conn) = conn_state.handle.upgrade() else {
continue;
};
for (path_id, addr) in conn_state.open_paths.iter() {
if let Some(stats) = conn.path_stats(*path_id) {
rtts.entry(addr.clone()).or_default().push(stats.rtt);
}
}
}
rtts
}
/// Selects the path with the lowest RTT, prefers direct paths.
///
/// If there are direct paths, this selects the direct path with the lowest RTT. If
Expand All @@ -914,22 +971,9 @@ impl RemoteStateActor {
/// direct paths are closed for all connections.
#[instrument(skip_all)]
fn select_path(&mut self) {
let all_path_rtts = self.calculate_path_rtts();
// Find the lowest RTT across all connections for each open path. The long way, so
// we get to log *all* RTTs.
let mut all_path_rtts: FxHashMap<transports::Addr, Vec<Duration>> = FxHashMap::default();
for conn_state in self.connections.values() {
let Some(conn) = conn_state.handle.upgrade() else {
continue;
};
for (path_id, addr) in conn_state.open_paths.iter() {
if let Some(stats) = conn.path_stats(*path_id) {
all_path_rtts
.entry(addr.clone())
.or_default()
.push(stats.rtt);
}
}
}
trace!(?all_path_rtts, "dumping all path RTTs");
let path_rtts: FxHashMap<transports::Addr, Duration> = all_path_rtts
.into_iter()
Expand Down Expand Up @@ -1175,6 +1219,7 @@ pub(crate) enum RemoteStateMessage {
///
/// This currently only includes a list of all known transport addresses for the remote.
RemoteInfo(oneshot::Sender<RemoteInfo>),
NetworkChange(Report),
}

/// A handle to a [`RemoteStateActor`].
Expand Down
2 changes: 1 addition & 1 deletion iroh/src/net_report/report.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ impl RelayLatencies {
}

/// Returns the lowest latency across records.
pub(super) fn get(&self, url: &RelayUrl) -> Option<Duration> {
pub(crate) fn get(&self, url: &RelayUrl) -> Option<Duration> {
let mut list = Vec::with_capacity(3);
if let Some(val) = self.https.get(url) {
list.push(*val);
Expand Down
Loading