feat: dial direct connections for DCUtR instead of reusing the relay - #274
Merged
Conversation
dial checked the connection pool before looking at the addresses it was given. DCUtR necessarily runs while a relayed connection to the same peer exists, so both sides got that connection back and reported success without emitting a single packet -- which cannot produce the TCP simultaneous connect the spec relies on. Neither side of the exchange dialled directly: registerDCUtRHandler used the plain dial, and nothing ever started the upgrade in the first place. specs/relay/DCUtR, step 5: "Upon receiving the Sync, A immediately dials the address to B. Upon expiry of the timer, B dials the address to A. This will result in a TCP Simultaneous Connect. For the purpose of all protocols run on top of this TCP connection, A is assumed to be the client and B the server." Introduce DialOpts and dialWith, mirroring the two context values go-libp2p's hole puncher sets together, network.WithForceDirectDial and network.WithSimultaneousConnect: - doForceDirect bypasses pool reuse and the backoff check, as go-libp2p does, while still recording backoff on failure. It also bypasses dial deduplication, which is required rather than incidental: DCUtR calls its dialer once per address so every address is attempted at the same moment, and swPendingDials shares one result among all waiters, so joining it would collapse those attempts to a single address. - doUpgradeAsClient selects the security and muxer roles. upgradeOutbound and upgradeInbound are now one upgradeAs that derives every role from the direction, the way go-libp2p's upgrader does (isServer := dir == DirInbound). Peer B therefore calls connect() while running the responder side, without which both ends pick Yamux RoleClient and their odd stream ids collide. The reversed connection is Inbound and its resource slot is reserved and released as Inbound. go-libp2p reserves Outbound and reports Inbound; here closeConnection releases by connDirection, so the two must agree. Bound both the hole punch dial and the coordination exchange. Without a bound, a simultaneous connect that fails to collide lands on the peer's ordinary listener, leaving both ends running the responder side and waiting on each other forever. go-libp2p bounds the same two things with defaultDirectDialTimeout and StreamTimeout; the defaults match. Add isPublicAddr, mirroring go-multiaddr's manet.IsPublicAddr: IPv4 by exclusion, IPv6 by inclusion plus the NAT64 prefixes, DNS by special-use domain. Nothing equivalent existed. isRelayedAddr moves next to it so the connection pool can use it without depending on the NAT stack. lookupConn now prefers a direct connection over a relayed one, keeping the migration in one place as go-libp2p does in bestConnToPeer, and still returns the relay when it is all there is, so a failed punch leaves it usable. On success the relay is closed after a grace period, but only if a direct connection is still up: it can die inside the window, and dropping the relay too would strand the peer. Closes #258.
seetadev
approved these changes
Aug 24, 2026
seetadev
left a comment
Collaborator
There was a problem hiding this comment.
@adust09 : Great work on this, Shouki. This is an important fix for making DCUtR perform the direct simultaneous connect rather than accidentally reusing the existing relay.
Appreciate the careful handling of force-direct dialing, deduplication, connection direction/upgrade roles, timeouts, and the direct-vs-relayed connection preference. The alignment with go-libp2p’s behavior and the additional testing around the failure/hang cases make this a solid improvement.
Great to see the relay fallback and grace-period handling preserved as well. Thanks for the thorough implementation and testing here. LGTM, happy to approve and merge.
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
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.
Closes #258. Depends on #266, #267 (merged).
Problem
dialchecked the connection pool before looking at the addresses given to it. DCUtR runs while a relayed connection to the same peer exists, so it got that connection back and reported success without emitting a packet. No TCP simultaneous connect is possible that way. Neither side dialled directly, and nothing started the upgrade at all.Change
DialOptsanddialWithmirror the two context values go-libp2p's hole puncher sets together,WithForceDirectDialandWithSimultaneousConnect.dialis the wrapper supplying defaults, so no caller changed.doForceDirectskips pool reuse and the backoff check (still records backoff on failure, as go does) and skips dial dedup. Skipping dedup is required: DCUtR dials once per address so all are attempted at the same instant, andswPendingDialsshares one result among waiters, which would collapse them to a single address.doUpgradeAsClientselects the security and muxer roles.upgradeOutbound/upgradeInboundcollapse intoupgradeAs, deriving every role from the direction as go's upgrader does (isServer := dir == DirInbound). Without it both ends pick YamuxRoleClientand their stream ids collide.The reversed connection is
Inboundand its resource slot is reserved and released asInbound.closeConnectionreleases byconnDirection, so the two must agree.Testing surfaced a second defect: neither the hole punch dial nor the coordination exchange was bounded. A simultaneous connect that fails to collide lands on the peer's ordinary listener, leaving both ends running the responder side and waiting on each other forever. Both are bounded now, with go's defaults of 10s for the dial and 1m for the stream.
isPublicAddris new inLibP2P.Multiaddr, mirroring go-multiaddr'smanet.IsPublicAddr. Nothing equivalent existed.isRelayedAddrmoves next to it so the connection pool can use it without depending on the NAT stack.lookupConnnow prefers direct over relayed, keeping the choice in one place as go'sbestConnToPeerdoes, and still returns the relay when it is all there is. On success the relay closes after a 15s grace period, but only if a direct connection is still up.registerNATHandlerssubscribes a notifier onInbound && isRelayedAddr, the same condition as go'sholepuncher.go:281.upgradeRelayedConnectionis exported for direct use; it callsidentifyPeeritself rather than racing the identify notifier.Out of scope
Role flip on the final retry, which is go's pre-v0.41 compatibility workaround and not in the spec; revisit under #131. QUIC hole punching (#235). Migrating existing long-lived streams, which the spec says "will have to be recreated".