Skip to content

feat(iroh): ping paths and trigger holepunching on networkchange #3796

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

Merged
dignifiedquire merged 2 commits into from
Jan 8, 2026
+38 −1
Merged

feat(iroh): ping paths and trigger holepunching on networkchange #3796

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
93fd8be
feat: ping paths and trigger holepunching on networkchange
dignifiedquire Dec 19, 2025
3f8bd35
fixup
dignifiedquire Jan 7, 2026
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
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

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

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 @@ -1232,6 +1232,8 @@ impl Actor {
} else {
self.re_stun(UpdateReason::LinkChangeMinor);
}

self.msock.remote_map.on_network_change(is_major);
}

fn handle_relay_map_change(&mut self) {
Expand Down
11 changes: 11 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 @@ -98,6 +98,17 @@ impl RemoteMap {
handles.retain(|_eid, handle| !handle.sender.is_closed())
}

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

/// Returns the sender for the [`RemoteStateActor`].
///
/// If needed a new actor is started on demand.
Expand Down
24 changes: 24 additions & 0 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 @@ -368,6 +368,28 @@ impl RemoteStateActor {
};
tx.send(info).ok();
}
RemoteStateMessage::NetworkChange { is_major } => {
Copy link
Contributor

@flub flub Dec 19, 2025

Choose a reason for hiding this comment

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

Can we not do this with the local addr watcher? I don't like having two mechanisms for this, and a watcher seemed nice.

Copy link
Contributor Author

@dignifiedquire dignifiedquire Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

addr change update happens a lot later, this is why I added this mechanism, and it also happens for potentially other reasons. The addr change will happen eventually when net report finishes, but we don't need to wait for that, to already know we want to recheck our connections in here

Copy link
Contributor

@flub flub Dec 19, 2025

Choose a reason for hiding this comment

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

I see. could we more often update our local addrs? i don't think if the interfaces change that we need to wait until we've also run netcheck until we get updates. I'd like to receive them all as quickly as possible, but still only via one API.

Copy link
Contributor Author

@dignifiedquire dignifiedquire Dec 19, 2025

Choose a reason for hiding this comment

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

one day, but that would require the much bigger refactor of netreport to be more stream based, which will not happen any time soon

Copy link
Contributor

@flub flub Jan 7, 2026

Choose a reason for hiding this comment

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

I'm going to accept this technical debt :)
It's annoyed me for a while though so hopefully this will motivate me to get round to improving this soon.

self.handle_network_change(is_major);
}
}
}

fn handle_network_change(&mut self, is_major: bool) {
for conn in self.connections.values() {
Copy link
Contributor

@flub flub Jan 7, 2026

Choose a reason for hiding this comment

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

I feel like I've forgotten half the things, I'm not really sure why we are sending pings today, because I don't think we can do anything with the resulting behaviour. Each path has it's keep-alive and idle-timeout already set. Sending an explicit ping will not change much: the idle timers are already set and will fire. Sending something now won't change that much I think?

Copy link
Contributor Author

@dignifiedquire dignifiedquire Jan 7, 2026

Choose a reason for hiding this comment

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

the idea is to make sure we have new information as early as possible

if let Some(quinn_conn) = conn.handle.upgrade() {
for (path_id, addr) in &conn.open_paths {
if let Some(path) = quinn_conn.path(*path_id) {
// Ping the current path
if let Err(err) = path.ping() {
warn!(%err, ?path_id, ?addr, "failed to ping path");
}
}
}
}
}

if is_major {
self.trigger_holepunching();
Copy link
Contributor

@flub flub Dec 19, 2025

Choose a reason for hiding this comment

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

so this code-path trigges holepunching less often than the watcher for the local addrs? but otherwise does the same? it seems more like the is_major detection should be added to trigger_holepunching. And that should also send pings if that is helpful. but i don't think this is helpful yet, i think we need to be able to see the number of in-flight tail-loss probes in select path to benefit from this.

}
}

Expand Down Expand Up @@ -1175,6 +1197,8 @@ pub(crate) enum RemoteStateMessage {
///
/// This currently only includes a list of all known transport addresses for the remote.
RemoteInfo(oneshot::Sender<RemoteInfo>),
/// The network status has changed in some way
NetworkChange { is_major: bool },
}

/// A handle to a [`RemoteStateActor`].
Expand Down
Loading