Skip to content

fix(dcutr): preserve QUIC server role during hole punching#2838

Merged
richard-ramos merged 2 commits into
masterfrom
dcutr-quic
Jul 16, 2026
Merged

fix(dcutr): preserve QUIC server role during hole punching#2838
richard-ramos merged 2 commits into
masterfrom
dcutr-quic

Conversation

@richard-ramos

@richard-ramos richard-ramos commented Jul 15, 2026

Copy link
Copy Markdown
Member

Summary

Fix QUIC DCUtR hole punching so the Sync sender retains the listener/server role required by the specification.

Listener-role QUIC dial attempts now send random UDP packets from the matching listener socket and wait for the inbound QUIC connection, rather than initiating a QUIC client connection. This mirrors go-libp2p’s transport-level handling and prevents both peers from acting as QUIC clients during hole punching.

Affected Areas

  • Transports — propagate connection direction to transports and add QUIC listener-role hole punching
  • Protocol Logic — preserve the asymmetric QUIC roles required by DCUtR

Risk Assessment

The behavioral change is limited to QUIC dial attempts marked Direction.In; normal outbound QUIC dialing remains unchanged. Selecting a listener requires exactly one endpoint matching the target address family.

References

@richard-ramos richard-ramos changed the title fix(dcutr): use listener role for QUIC hole punching fix(dcutr): preserve QUIC server role during hole punching Jul 15, 2026
@richard-ramos
richard-ramos marked this pull request as ready for review July 15, 2026 21:26
@richard-ramos
richard-ramos requested review from a team, gmelodie, rlve and vladopajic July 15, 2026 21:26
@codecov-commenter

codecov-commenter commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.17%. Comparing base (13473bc) to head (a470c8a).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
libp2p/crypto/rng.nim 69.23% 4 Missing ⚠️
libp2p/transports/quictransport.nim 94.44% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2838      +/-   ##
==========================================
- Coverage   81.23%   81.17%   -0.07%     
==========================================
  Files         170      170              
  Lines       30741    30787      +46     
  Branches       13       13              
==========================================
+ Hits        24973    24991      +18     
- Misses       5768     5796      +28     
Files with missing lines Coverage Δ
libp2p/builders.nim 76.62% <100.00%> (ø)
libp2p/dialer.nim 92.56% <100.00%> (ø)
libp2p/protocols/connectivity/relay/rtransport.nim 86.79% <ø> (ø)
libp2p/transports/memorytransport.nim 87.05% <ø> (ø)
libp2p/transports/tcptransport.nim 81.69% <ø> (ø)
libp2p/transports/tortransport.nim 83.17% <100.00%> (-0.40%) ⬇️
libp2p/transports/transport.nim 94.54% <100.00%> (-1.79%) ⬇️
libp2p/transports/wstransport.nim 80.13% <ø> (ø)
libp2p/transports/quictransport.nim 87.80% <94.44%> (-2.68%) ⬇️
libp2p/crypto/rng.nim 88.46% <69.23%> (-2.75%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tests/libp2p/transports/test_quic.nim Outdated
Comment thread tests/libp2p/transports/test_quic.nim Outdated
Comment thread libp2p/crypto/rng.nim
Comment thread tests/libp2p/transports/test_quic.nim Outdated
Comment on lines +610 to +614
while true:
let payload = self.rng.generateBytes(64)
await endpoint.get().datagramTransport().sendTo(taAddress, payload)
let delay = self.rng.rand(10, 200)
await sleepAsync(delay.milliseconds)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

understood, but it feels strange. for transport to implement this logic.

it also needs better explanation: while true: needs to be explained - this dial future is expected to be cancelled (must be), it will never return on it's own. this is also divination from behavior of other transports, when dialing using quic server it never returns, while dialing from other transport returns?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

fyi @rlve, ^ this is blind spot in unit tests, there should be test that assert what happens when server is dialing. code needs to be the same for every transport.

Comment thread libp2p/transports/quictransport.nim

@rlve rlve left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have run hole punch unified-testing suite locally with your branch and all the cases are green now ✅

## Test Results

| Test | Dialer | Listener | Transport | Status | Duration |
|------|--------|----------|-----------|--------|----------|
| nim-v2.2 x rust-v0.56 (tcp, noise, yamux) [dr - linux, rly - rust-v0.56, lr - linux] | nim-v2.2 | rust-v0.56 | tcp | ✅ | 3s |
| nim-v2.2 x rust-v0.56 (tcp, noise, mplex) [dr - linux, rly - rust-v0.56, lr - linux] | nim-v2.2 | rust-v0.56 | tcp | ✅ | 5s |
| nim-v2.2 x rust-v0.56 (quic-v1) [dr - linux, rly - rust-v0.56, lr - linux] | nim-v2.2 | rust-v0.56 | quic-v1 | ✅ | 5s |
| rust-v0.56 x nim-v2.2 (tcp, noise, yamux) [dr - linux, rly - rust-v0.56, lr - linux] | rust-v0.56 | nim-v2.2 | tcp | ✅ | 7s |
| rust-v0.56 x nim-v2.2 (tcp, noise, mplex) [dr - linux, rly - rust-v0.56, lr - linux] | rust-v0.56 | nim-v2.2 | tcp | ✅ | 7s |
| nim-v2.2 x nim-v2.2 (tcp, noise, yamux) [dr - linux, rly - rust-v0.56, lr - linux] | nim-v2.2 | nim-v2.2 | tcp | ✅ | 7s |
| nim-v2.2 x nim-v2.2 (tcp, noise, mplex) [dr - linux, rly - rust-v0.56, lr - linux] | nim-v2.2 | nim-v2.2 | tcp | ✅ | 7s |
| nim-v2.2 x nim-v2.2 (quic-v1) [dr - linux, rly - rust-v0.56, lr - linux] | nim-v2.2 | nim-v2.2 | quic-v1 | ✅ | 7s |
| rust-v0.56 x nim-v2.2 (quic-v1) [dr - linux, rly - rust-v0.56, lr - linux] | rust-v0.56 | nim-v2.2 | quic-v1 | ✅ | 7s |

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes QUIC DCUtR hole punching role behavior by propagating connection Direction into transport dialing and implementing a listener/server-role QUIC “dial” path that sends random UDP packets from the listener socket instead of initiating a QUIC client handshake (aligning with the DCUtR simultaneous connect spec).

Changes:

  • Extend Transport.dial (and all transport implementations) with a dir: Direction parameter and propagate it via Dialer.dialAndUpgrade.
  • Implement QUIC listener-role hole punching in QuicTransport.dial(..., dir = Direction.In) by sending random UDP packets from the matching listener endpoint.
  • Update/add tests to validate UDP hole-punch packet emission and adjust DCUtR QUIC test expectations; wire RNG explicitly into QuicTransport.new and add Rng.rand(low, high) helper.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/libp2p/transports/utils.nim Pass explicit RNG into QuicTransport.new in test helper.
tests/libp2p/transports/test_quic.nim Add test asserting listener-role QUIC “dial” sends UDP hole-punch packets; update transport construction.
tests/libp2p/protocols/test_dcutr.nim Adjust QUIC DCUtR test to validate listener-role behavior via expected timeout in the direct-connection-as-relay setup.
libp2p/transports/transport.nim Add dir: Direction parameter to the base Transport.dial method.
libp2p/transports/tcptransport.nim Update dial signature to accept dir.
libp2p/transports/wstransport.nim Update dial signature to accept dir.
libp2p/transports/tortransport.nim Update dial signature to accept dir.
libp2p/transports/memorytransport.nim Update dial signature to accept dir.
libp2p/protocols/connectivity/relay/rtransport.nim Update relay transport dial signature to accept dir.
libp2p/dialer.nim Pass dir through to transport.dial(...) and preserve the transport direction post-dial.
libp2p/transports/quictransport.nim Require explicit RNG in constructor; implement listener-role hole punching loop in dial for Direction.In; improve error mapping.
libp2p/crypto/rng.nim Add Rng.rand(low, high) helper for inclusive random interval selection.
libp2p/builders.nim Wire builder RNG into QuicTransport.new.
Comments suppressed due to low confidence (1)

libp2p/transports/quictransport.nim:625

  • QuicTransport.dial relies on the infinite while true inside the dir == Direction.In branch to prevent falling through into the normal client dial path. This is easy to misread/accidentally refactor into dialing as a QUIC client again. Making the branches explicit (if/else) will make the listener-role behavior unambiguous and self-contained.
    let endpoint = self.dialEndpointFor(taAddress)
    let quicConnection = await endpoint.dial(taAddress)
    peerId.withValue(expectedPeerId):
      if not verifyCertificatesForPeer(quicConnection.certificates(), expectedPeerId):
        quicConnection.abort()
        raise newException(
          QuicTransportDialError,
          "error in quic dial: certificate does not match expected peer id",
        )
    return self.wrapConnection(quicConnection, Direction.Out)

Comment thread libp2p/transports/quictransport.nim
@rlve rlve linked an issue Jul 16, 2026 that may be closed by this pull request
Comment thread libp2p/transports/quictransport.nim
@github-project-automation github-project-automation Bot moved this from new to In Progress in nim-libp2p Jul 16, 2026
@richard-ramos
richard-ramos enabled auto-merge July 16, 2026 15:05
@richard-ramos
richard-ramos added this pull request to the merge queue Jul 16, 2026
Merged via the queue into master with commit 460a759 Jul 16, 2026
39 checks passed
@richard-ramos
richard-ramos deleted the dcutr-quic branch July 16, 2026 16:13
@github-project-automation github-project-automation Bot moved this from In Progress to done in nim-libp2p Jul 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: done

Development

Successfully merging this pull request may close these issues.

dcutr: quic hole punching dials from both peers

6 participants