Skip to content

feat: introduce rtt_hints for opening paths #278

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 1 commit into
base: main
Choose a base branch
from
Open

feat: introduce rtt_hints for opening paths #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 22 additions & 7 deletions quinn-proto/src/connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,7 @@ impl Connection {
remote: SocketAddr,
initial_status: PathStatus,
now: Instant,
rtt_hint: Option<Duration>,
) -> Result<(PathId, bool), PathError> {
match self
.paths
Expand All @@ -564,7 +565,7 @@ impl Connection {
{
Some((path_id, _state)) => Ok((*path_id, true)),
None => self
.open_path(remote, initial_status, now)
.open_path(remote, initial_status, now, rtt_hint)
.map(|id| (id, false)),
}
}
Expand All @@ -578,6 +579,7 @@ impl Connection {
remote: SocketAddr,
initial_status: PathStatus,
now: Instant,
rtt_hint: Option<Duration>,
) -> Result<PathId, PathError> {
if !self.is_multipath_negotiated() {
return Err(PathError::MultipathNotNegotiated);
Expand Down Expand Up @@ -608,7 +610,7 @@ impl Connection {
return Err(PathError::RemoteCidsExhausted);
}

let path = self.ensure_path(path_id, remote, now, None);
let path = self.ensure_path(path_id, remote, now, None, rtt_hint);
path.status.local_update(initial_status);

Ok(path_id)
Expand Down Expand Up @@ -828,6 +830,7 @@ impl Connection {
remote: SocketAddr,
now: Instant,
pn: Option<u64>,
rtt_hint: Option<Duration>,
) -> &mut PathData {
let vacant_entry = match self.paths.entry(path_id) {
btree_map::Entry::Vacant(vacant_entry) => vacant_entry,
Expand All @@ -851,7 +854,14 @@ impl Connection {
&self.config,
);

let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base();
let pto = match rtt_hint {
Some(rtt_hint) => rtt_hint,
None => {
// Fallback to the pessimistic calculation
// TODO: we could consider using known PTOs in this case
self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base()
}
};
Comment on lines +857 to +864
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.

Suggested change
let pto = match rtt_hint {
Some(rtt_hint) => rtt_hint,
None => {
// Fallback to the pessimistic calculation
// TODO: we could consider using known PTOs in this case
self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base()
}
};
if let Some(rtt_hint) = rtt_hint {
data.rtt = RttEstimator::new(rtt_hint);
}
let pto = self.ack_frequency.max_ack_delay_for_pto() + data.rtt.pto_base();

That seems more correct to me...
We don't want to only change the PathOpen timer, we also want to change the time at which we resend path challenges, and change when we consider packets lost, etc.
Also, the PTO would actually be a different value in this case, because we make assumptions about variance, and add the max ACK delay. It surprises me that all of that would be skipped in your version.

self.timers.set(
Timer::PerPath(path_id, PathTimer::PathOpen),
now + 3 * pto,
Expand Down Expand Up @@ -3699,7 +3709,7 @@ impl Connection {

if self.side().is_server() && !self.abandoned_paths.contains(&path_id) {
// Only the client is allowed to open paths
self.ensure_path(path_id, remote, now, number);
self.ensure_path(path_id, remote, now, number, None);
}
if self.paths.contains_key(&path_id) {
self.on_packet_authenticated(
Expand Down Expand Up @@ -6262,10 +6272,14 @@ impl Connection {
/// initiated, the previous one is cancelled, and paths that have not been opened are closed.
///
/// Returns the server addresses that are now being probed.
pub fn initiate_nat_traversal_round(
pub fn initiate_nat_traversal_round<F>(
&mut self,
now: Instant,
) -> Result<Vec<SocketAddr>, iroh_hp::Error> {
rtt_hints: F,
) -> Result<Vec<SocketAddr>, iroh_hp::Error>
where
F: Fn(&SocketAddr) -> Option<Duration>,
{
Comment on lines +6275 to +6282
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.

This gives you a lot of control in theory. But I'd have preferred to keep it simple with only a single RTT hint for all probes. This RTT would then be set to the worst RTT you expect to get. E.g. when we haven't hole-punched an IP path yet, then this should be set to the relay path's RTT, and if we have some IP paths already, then this should be set to the worst IP path's RTT.

Copy link
Collaborator 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.

a single value seems bad imho, making it a lot less useful, as we might be using multiple transports between different probes

if self.state.is_closed() {
return Err(iroh_hp::Error::Closed);
}
Expand Down Expand Up @@ -6314,7 +6328,8 @@ impl Connection {
continue;
}
};
match self.open_path_ensure(remote, PathStatus::Backup, now) {
let rtt_hint = rtt_hints(&remote);
match self.open_path_ensure(remote, PathStatus::Backup, now, rtt_hint) {
Ok((path_id, path_was_known)) if !path_was_known => {
path_ids.push(path_id);
probed_addresses.push(remote);
Expand Down
18 changes: 10 additions & 8 deletions quinn-proto/src/tests/multipath.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ fn open_path() {
let server_addr = pair.server.addr;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(server_addr, PathStatus::Available, now)
.open_path(server_addr, PathStatus::Available, now, None)
.unwrap();
pair.drive();
let client_conn = pair.client_conn_mut(client_ch);
Expand All @@ -488,7 +488,7 @@ fn open_path_key_update() {
let server_addr = pair.server.addr;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(server_addr, PathStatus::Available, now)
.open_path(server_addr, PathStatus::Available, now, None)
.unwrap();

// Do a key-update at the same time as opening the new path.
Expand Down Expand Up @@ -523,7 +523,7 @@ fn open_path_validation_fails_server_side() {
let now = pair.time;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(different_addr, PathStatus::Available, now)
.open_path(different_addr, PathStatus::Available, now, None)
.unwrap();

// block the server from receiving anything
Expand Down Expand Up @@ -556,7 +556,7 @@ fn open_path_validation_fails_client_side() {
let addr = pair.server.addr;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(addr, PathStatus::Available, now)
.open_path(addr, PathStatus::Available, now, None)
.unwrap();

// block the client from receiving anything
Expand All @@ -577,7 +577,7 @@ fn close_path() {
let server_addr = pair.server.addr;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(server_addr, PathStatus::Available, now)
.open_path(server_addr, PathStatus::Available, now, None)
.unwrap();
pair.drive();
assert_ne!(path_id, PathId::ZERO);
Expand Down Expand Up @@ -613,7 +613,7 @@ fn close_last_path() {
let server_addr = pair.server.addr;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(server_addr, PathStatus::Available, now)
.open_path(server_addr, PathStatus::Available, now, None)
.unwrap();
pair.drive();
assert_ne!(path_id, PathId::ZERO);
Expand Down Expand Up @@ -686,7 +686,9 @@ fn per_path_observed_address() {
let now = pair.time;
let remote = pair.server.addr;
let conn = pair.client_conn_mut(client_ch);
let _new_path_id = conn.open_path(remote, PathStatus::Available, now).unwrap();
let _new_path_id = conn
.open_path(remote, PathStatus::Available, now, None)
.unwrap();

pair.drive();
let conn = pair.client_conn_mut(client_ch);
Expand Down Expand Up @@ -739,7 +741,7 @@ fn mtud_on_two_paths() {
let server_addr = pair.server.addr;
let path_id = pair
.client_conn_mut(client_ch)
.open_path(server_addr, PathStatus::Available, now)
.open_path(server_addr, PathStatus::Available, now, None)
.unwrap();
pair.drive();
let client_conn = pair.client_conn_mut(client_ch);
Expand Down
4 changes: 2 additions & 2 deletions quinn-proto/src/tests/random_interaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ impl TestOp {
Side::Server => server,
};
let conn = state.conn(pair)?;
conn.open_path(remote, initial_status, now).ok();
conn.open_path(remote, initial_status, now, None).ok();
}
Self::ClosePath(side, path_idx, error_code) => {
let state = match side {
Expand Down Expand Up @@ -177,7 +177,7 @@ impl TestOp {
Side::Server => server,
};
let conn = state.conn(pair)?;
let addrs = conn.initiate_nat_traversal_round(now).ok()?;
let addrs = conn.initiate_nat_traversal_round(now, |_| None).ok()?;
trace!(?addrs, "initiating NAT Traversal");
}
}
Expand Down
30 changes: 24 additions & 6 deletions quinn/src/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,12 @@ impl Connection {
/// Otherwise behaves exactly as [`open_path`].
///
/// [`open_path`]: Self::open_path
pub fn open_path_ensure(&self, addr: SocketAddr, initial_status: PathStatus) -> OpenPath {
pub fn open_path_ensure(
&self,
addr: SocketAddr,
initial_status: PathStatus,
rtt_hint: Option<Duration>,
) -> OpenPath {
let mut state = self.0.state.lock("open_path");

// If endpoint::State::ipv6 is true we want to keep all our IP addresses as IPv6.
Expand Down Expand Up @@ -406,7 +411,9 @@ impl Connection {
};

let now = state.runtime.now();
let open_res = state.inner.open_path_ensure(addr, initial_status, now);
let open_res = state
.inner
.open_path_ensure(addr, initial_status, now, rtt_hint);
state.wake();
match open_res {
Ok((path_id, existed)) if existed => {
Expand Down Expand Up @@ -439,7 +446,12 @@ impl Connection {
/// future, or at a later time. If the failure is immediate [`OpenPath::path_id`] will
/// return `None` and the future will be ready immediately. If the failure happens
/// later, a [`PathEvent`] will be emitted.
pub fn open_path(&self, addr: SocketAddr, initial_status: PathStatus) -> OpenPath {
pub fn open_path(
&self,
addr: SocketAddr,
initial_status: PathStatus,
rtt_hint: Option<Duration>,
) -> OpenPath {
let mut state = self.0.state.lock("open_path");

// If endpoint::State::ipv6 is true we want to keep all our IP addresses as IPv6.
Expand Down Expand Up @@ -470,7 +482,7 @@ impl Connection {

let (on_open_path_send, on_open_path_recv) = watch::channel(Ok(()));
let now = state.runtime.now();
let open_res = state.inner.open_path(addr, initial_status, now);
let open_res = state.inner.open_path(addr, initial_status, now, rtt_hint);
state.wake();
match open_res {
Ok(path_id) => {
Expand Down Expand Up @@ -932,10 +944,16 @@ impl Connection {
/// initiated, the previous one is cancelled, and paths that have not been opened are closed.
///
/// Returns the server addresses that are now being probed.
pub fn initiate_nat_traversal_round(&self) -> Result<Vec<SocketAddr>, iroh_hp::Error> {
pub fn initiate_nat_traversal_round<F>(
&self,
rtt_hints: F,
) -> Result<Vec<SocketAddr>, iroh_hp::Error>
where
F: Fn(&SocketAddr) -> Option<Duration>,
{
let mut conn = self.0.state.lock("initiate_nat_traversal_round");
let now = conn.runtime.now();
conn.inner.initiate_nat_traversal_round(now)
conn.inner.initiate_nat_traversal_round(now, rtt_hints)
}
}

Expand Down
6 changes: 5 additions & 1 deletion quinn/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,11 @@ async fn test_multipath_observed_address() {
// this sleep after the poll_transmit unwraps have been addressed
tokio::time::sleep(tokio::time::Duration::from_millis(200)).await;
let path = conn
.open_path(server_addr, proto::PathStatus::Available)
.open_path(
server_addr,
proto::PathStatus::Available,
Some(Duration::from_millis(50)),
)
.await
.unwrap();
let mut reports = path.observed_external_addr().unwrap();
Expand Down
Loading