use PATH_CHALLENGE for hole punching #231

divagant-martian · 2025-12-10T02:55:19Z

Description

TBD

Breaking Changes

TBD

Notes & open questions

TBD

This is not retransmittable data

…ueue

…ble' into hole-punch-with-challenge

…lenge

codecov-commenter · 2025-12-10T02:59:16Z

Codecov Report

❌ Patch coverage is 38.33333% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.36%. Comparing base (8dfdc04) to head (52f3d2d).

Files with missing lines	Patch %	Lines
quinn-proto/src/connection/mod.rs	27.16%	59 Missing ⚠️
quinn-proto/src/connection/paths.rs	61.53%	15 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##           main-iroh     #231      +/-   ##
=============================================
- Coverage      76.57%   76.36%   -0.21%     
=============================================
  Files             83       83              
  Lines          23152    23220      +68     
=============================================
+ Hits           17728    17733       +5     
- Misses          5424     5487      +63

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

flub · 2025-12-10T10:34:15Z

quinn-proto/src/connection/mod.rs

+                            let token: u64 = self.rng.random();
+                            trace!(%destination, cid=%new_cid, token=format!("{:08x}", token), "Nat traversal: PATH_CHALLENGE packet");
+                            // TODO(@divma): abstract writting path challenges, this logic should
+                            // no be here


How far do you want to go? I was thinking that at some point we have a generic function that writes frames (takes frame::Frame enum) and takes care of all the things we want for writing them:

log the FRAME_TYPE being written with appropriate fields (please add that to the code here 😄 )

increment the frame stats

records the frame with qlog

writes the frame to the packet/buffer

That's probably better as a separate refactor though.

completely agree on all accounts, this current state is gross

btw, the log line is already there friend :p

…lenge

github-actions · 2025-12-15T16:13:30Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/quinn/pr/231/docs/iroh_quinn/

Last updated: 2025-12-15T21:09:43Z

flub

Only managed to cover the sending side, will continue with the rest.

flub · 2025-12-15T17:43:17Z

quinn-proto/src/connection/mod.rs


+                if transmit.is_empty() {
+                    // Nothing to send on this path, and nothing yet built; try to send hole punching
+                    // path challenges.


Edit: see the suggestion i make above to address these things.

I know we've tried to cover this bit about where to put this before, but upon reading this again I wondered:

are we doing this on the right side of the congestion controller?

do we need to have a congestion controller for each 4-tuple we're sending off-path patch_challenges for?

this branch is only taken if path_should_send is false. but since holepunching is time-sensitive, should it should probably be prioritised over any other data to be sent on this path. otherwise application data can starve out the holepunching.

I think the congestion controller bit is fine to ignore at first. We can not count the off-path path challenges to a congestion controller for now (but should create an issue).

Another problem with this location that I hadn't realised before is that this is now prioritised over a CONNECTION_CLOSE frame, which it shouldn't.

flub · 2025-12-15T17:52:27Z

quinn-proto/src/connection/mod.rs

+                            let token: u64 = self.rng.random();
+                            let challenge = frame::PathChallenge(token);
+                            trace!(%destination, cid=%new_cid, %challenge, "Nat traversal: PATH_CHALLENGE packet");
+                            buf.write(challenge);


You don't have an active PacketBuilder here. So I don't think this is writing a proper encrypted packet. Just writing some random bytes on the wire.

Should this not basically do the same as send_pref_path_challenge()? Perhaps that function can be made more generic to be like send_off_path_path_challenge.

flub · 2025-12-15T18:21:46Z

quinn-proto/src/connection/mod.rs

            // Send an off-path PATH_RESPONSE. Prioritized over on-path data to ensure that
            // path validation can occur while the link is saturated.
-            if space_id == SpaceId::Data && builder.buf.num_datagrams() == 1 {
+            if space_id == SpaceId::Data && builder.buf.is_empty() {


I know we spoke about this earlier and assumed this was a better check. But reviewing now I can see that we've already created a PacketBuilder on line 1230. At that time we already wrote a packet header into the transmit buffer. So really the previous check was correct because we should be in the first packet being built.

(I should make another attempt at making the PacketBuilder own the transmit at this point so that you can't make this mistake. In the upstream PR of the TransmitBuf/TransmitBuilder this might already be better even.)

flub · 2025-12-15T18:25:28Z

quinn-proto/src/connection/mod.rs

+                            can_send.other,
+                            self,
+                            &mut qlog,
+                        )?


I don't think this works, because the PacketBuilder is already created on line 1230 and already wrote the packet header with a CID. Perhaps it is best to move off-path

flub · 2025-12-15T18:34:04Z

quinn-proto/src/connection/mod.rs

            max_datagrams,
            self.path_data(path_id).current_mtu().into(),
        );
        if let Some(challenge) = self.send_prev_path_challenge(now, &mut transmit, path_id) {


OMG, just realised this bug, because it has the same challenge that you have with off-path PATH_RESPONSE and I was wondering how it worked.

This needs to happen for each path we have! Not just PathId(0).

we do it for every path but I'm not sure the buffer has the right contents. Probably doesn't

flub · 2025-12-15T18:55:44Z

quinn-proto/src/connection/mod.rs

                    }
                }
            };



Suggestion:

This is the place where we can do all the off-path stuff:

send_prev_path_challenge()

send_off_path_path_challenge(), to be created, probably share a lot of code with the previous function.

send_off_path_path_response().

This allow each of them to start their own packet before the packet for the on-path stuff is stared. Additionally it makes sure they are prioritised over the on-path data, so application data can not hinder holepunching. If there are no CIDs to do their thing then these functions would return None (all would have the same signature as send_prev_path_challenge) and the loop would carry on with on-path data for the path.

Finally we'd have to leave a TODO to handle the congestion control properly for them. But for the first cut don't think it's problematic to skip this.

All of them need to be gated on !close, because they should not happen when we're closing probably.

…, fix PATH_ABANDON logic (#234) * fix(proto): Never use `self.close` from within quinn-proto, to ensure errors are returned from `poll` * Use `Connection::poll` instead of accessing `.state().take_error()` * Don't allow closing the last path, if no other *validated* path exists. Closes #244 * Add yet another regression test, also fix a typo in random_interaction * Set the `PathNotAbandoned` timer to at least the idle timeout Otherwise it's possible to mistake the connection timing out for a transport error of PATH_ABANDON not being sent. * `cargo make format` * Only return a transport error for an unanswered PATH_ABANDON if we still receive data on that path * Remove unused `PathNotAbandoned` timer * Rename `PathAbandon` to `ForgetPath` and `drop_path_state` to `forget_path` * Ignore "last path abandoned by peer" errors * Apply clippy suggestion * I guess a follow-up clippy fix? Lol * Rename "forget" to "discard" * Store `last_allowed_receive` in `PathState` * Chnage error message for failing PATH_ABANDON response * Fix timer name in qlog * Nit: Better warn message * Comment suggestion Co-authored-by: Floris Bruynooghe <[email protected]> * Fix comment grammar Co-authored-by: Floris Bruynooghe <[email protected]> * Add reference to `on_packet_authenticated` in comment --------- Co-authored-by: Floris Bruynooghe <[email protected]>

Co-authored-by: “ramfox” <“[email protected]”>

matheus23 and others added 8 commits November 23, 2025 16:19

Implement CidQueue::next_reserved

3467051

Explanatory comment

59187e1

keep info about sent challenges in the server state

8975e9d

move the pending challenges to the hp state

132f969

This is not retransmittable data

Merge remote-tracking branch 'n0/main-iroh' into path-challenge-cid-q…

8ba015b

…ueue

Merge remote-tracking branch 'n0/future-challenges-arent-retransmitta…

443346b

…ble' into hole-punch-with-challenge

Merge remote-tracking branch 'n0/main-iroh' into hole-punch-with-chal…

fa72173

…lenge

send path challenges, add debug on recv side

cc59abb

n0bot bot added this to iroh Dec 10, 2025

github-project-automation bot moved this to 🏗 In progress in iroh Dec 10, 2025

flub reviewed Dec 10, 2025

View reviewed changes

divagant-martian changed the title ~~use hole PATH_CHALLENGE for hole punching~~ use PATH_CHALLENGE for hole punching Dec 10, 2025

divagant-martian added 5 commits December 12, 2025 17:06

Merge remote-tracking branch 'n0/main-iroh'

4b1fbde

spelling

d7947f2

add on_received_path_response fn to PathData

4f57a1c

Merge remote-tracking branch 'n0/main-iroh' into hole-punch-with-chal…

24f2318

…lenge

cleanup

ff990de

divagant-martian and others added 2 commits December 15, 2025 11:27

use the right cid

1d5daee

Merge branch 'main-iroh' into hole-punch-with-challenge

52f3d2d

flub reviewed Dec 15, 2025

View reviewed changes

matheus23 and others added 3 commits December 15, 2025 16:05

feat: publically export UnorderedRecvStream (#260)

0ec61d4

Co-authored-by: “ramfox” <“[email protected]”>

reduce diff by a lot

9f22564

flub added the holepunching label Dec 22, 2025

flub added this to the iroh 1.0 RC milestone Dec 22, 2025

flub assigned divagant-martian Dec 22, 2025

use PATH_CHALLENGE for hole punching #231

Are you sure you want to change the base?

use PATH_CHALLENGE for hole punching #231

Uh oh!

Conversation

divagant-martian commented Dec 10, 2025

Description

Breaking Changes

Notes & open questions

Uh oh!

codecov-commenter commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flub left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Dec 10, 2025 •

edited

Loading

github-actions bot commented Dec 15, 2025 •

edited

Loading