fix: improve holepunching after network changes#3928
Merged
Conversation
I never know what netmon is. Let's name this thing after what it does.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3928/docs/iroh/ Last updated: 2026-03-24T10:32:57Z |
Frando
reviewed
Feb 12, 2026
| } | ||
| } | ||
| PathEvent::Closed { id, .. } | PathEvent::LocallyClosed { id, .. } => { | ||
| PathEvent::LocallyClosed { id, .. } => { |
Member
There was a problem hiding this comment.
If I'm reading things correctly, this event is only emitted when we opened a path, and the PathOpen timer fires without the path being established by then. So I think the below code should also run on PathEvent::Abandoned so that if one connection abandones a path, this is forwarded to all other connections?
Contributor
Author
There was a problem hiding this comment.
I'm postponing thinking about this for #3930
11 tasks
- relay paths are **always** recoverable - ip paths are recoverable when the local ip is not set
flub
added a commit
that referenced
this pull request
Feb 26, 2026
I can't work with obscure names that seem obvious to the persons writing them, so add doc comments explaining how things works. The RemoteStateActor did get a bit messy from all the changes. Make functions follow the naming conventions again, move them to their more logically grouped places and rename and document for clarity. Finally netmon, netwatch etc are all just meaningless names to me. Even after all this time. Name the variables and fields after the information they actually contain. This is all noise from #3928 that I'm splitting off so that that PR can contain just the relevant changes (and is easier to rebase/merge main).
2 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 26, 2026
## Description I can't work with obscure names that seem obvious to the persons writing them, so add doc comments explaining how things works. The RemoteStateActor did get a bit messy from all the changes. Make functions follow the naming conventions again, move them to their more logically grouped places and rename and document for clarity. Finally netmon, netwatch etc are all just meaningless names to me. Even after all this time. Name the variables and fields after the information they actually contain. ## Breaking Changes n/a ## Notes & open questions This is all noise from #3928 that I'm splitting off so that that PR can contain just the relevant changes (and is easier to rebase/merge main). ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant.
4 tasks
# Conflicts: # Cargo.lock # Cargo.toml # iroh/src/socket.rs # iroh/src/socket/remote_map/remote_state.rs
- Use FourTuple accessors instead of private field access - Use n0_future::time::sleep_until for wasm compat - Gate default_route_interface access behind cfg(not(wasm)) - Fix "addreses" typo
481cd63 to
b302842
Compare
aws-lc-rs 1.16.2 no longer uses the OpenSSL license, and cargo-deny treats unmatched license allowances as errors.
dignifiedquire
approved these changes
Mar 23, 2026
flub
commented
Mar 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When the network changes we should recover our connection and
holepunch again. Because quinn now tracks 4-tuples a lot of network
paths would turn into black holes because the source address would no
longer match.
This hooks up network changes to quinn so that it will close and open
new paths correctly when this happens. This should result in most
paths not being validated after a network change which in turn means
any new ADD_ADDRESS and REACH_OUT frames will be sent on the validated
path instead of a path that is no longer functional.
Breaking Changes
n/a
Notes & open questions
The first few commits do some renaming and documenting of stuff that's not
strictly required. It was nicer and easier to build on top of that. If
you, the reviewer, would prefer I can split those off into a refactoring
PR and base this one on top of that.
Change checklist
the correct ADD_ADDRESS and REMOVE_ADDRESS stuff.
happen in the correct order.
endpoint_relay_connect_looptest failure