Fix relayed keepalive and direct-upgrade handoff#266
Conversation
marcus-pousette-hp
changed the title
lejeunerenard
left a comment
lejeunerenard
left a comment
There was a problem hiding this comment.
Addressed/will address the upgrading of the relay connection in the other stacked PRs. Looking at the connectionKeepAlive change and found the new assertion isn't testing the missing configuration path.
|
|
||
| const bServer = b.createServer({ relayThrough: aServer.publicKey }, function (socket) { | ||
| lc.pass('server socket opened') | ||
| lc.is(socket.keepAlive, 12345, 'server relayed socket inherits connectionKeepAlive') |
There was a problem hiding this comment.
This assertion passes without the keepAlive change in server.js so isn't covering that case.
marcus-pousette-hp
force-pushed
the
fix/relay-server-connection-keepalive
branch
2 times, most recently
from
b3ef84b to
9dc7833
Compare
lejeunerenard
left a comment
lejeunerenard
left a comment
There was a problem hiding this comment.
Just one nit: Otherwise looks good to merge #268 in.
| relayThrough: aServer.publicKey | ||
| relayThrough: aServer.publicKey, | ||
| createSecretStream(isInitiator, rawStream, opts) { | ||
| if (!isInitiator && !assertedRelayKeepAlive) { |
There was a problem hiding this comment.
Is the assertedRelayKeepAlive necessary? I've tried removing it and the assert is only ever called once.
There was a problem hiding this comment.
good point, missed it
marcus-pousette-hp
force-pushed
the
fix/relay-server-connection-keepalive
branch
from
9dc7833 to
f19226f
Compare
#268) * Validate upgrading to direct when changing remote from relay to direct Changing the remote and immediately closing the relay connection leads to situations where one side is left trying to ack the packets sent via the relay connection but unable to since the peer closed their side since the remote changed successfully. Instead this approach uses the fact that changing the remote (whether immediately or deferred) changes the stream's socket which means that the `firewall` callback will be called for each packet received that comes from a different socket, in the case of changing remote from the relay -> direct means `firewall` only triggers from the relay. This means the direct connection can be validated by waiting for a packet to come through that doesn't trigger the firewall. This is done by setting a flag on the connection / hs that assumes the next `data` event will be direct. The flag is set to false when the firewall is called and the socket its called with is shown to be the relay's. Once direct the relay connection is cleaned up via the normal method. * Test/relay upgrade passive confirmation (#269) * test: cover passive relay upgrade confirmation * test: fold relay upgrade cases into one test * fix: nudge idle relay upgrades (#270) * fix: nudge idle relay upgrades * fix: simplify idle relay upgrade confirmation * refactor: centralize relay upgrade confirmation --------- Co-authored-by: marcus-pousette-hp <marcus-pousette-hp@users.noreply.github.com> --------- Co-authored-by: marcus-pousette-hp <marcus-pousette-hp@users.noreply.github.com> * Remove unnecessary setting `validUpgrade` to `true` --------- Co-authored-by: Marcus Pousette <marcus.pousette@tether.io> Co-authored-by: marcus-pousette-hp <marcus-pousette-hp@users.noreply.github.com>
|
I amend my previous comment (above) as invalid conclusions and I reminded myself of the implied order assumptions we do around the firewall callback. I would argue there is missing test coverage for the order of the callbacks: holepunchto/libudx#293 this would prevent a downstream refactor to potentially screw with assumption we are making with this PR. Otherwise this PR is good to go now! |
2 participants
This fixes server-side relayed connections so they inherit
connectionKeepAlivethe same way direct server sockets and client sockets already do.Enabling that exposed a relay-to-direct upgrade race: HyperDHT was closing the relay transport after local
remote-changed, but that only proves this side switched to direct. The peer may not have observed the direct path yet. To avoid stranding the peer, the upgrade path now sends one zero-length UDX message afterchangeRemote()so the other side can observe the new direct path before relay cleanup.One subtle behavior change here is that the old server-side relayed path had
keepAlive: 0, so empty secret-stream keepalive frames from the client could reach app code. In the upgrade test, the server echoes alldata, so those empty frames could accidentally create traffic during the relay-to-direct handoff. Once server-side keepalive is enabled, secret-stream correctly consumes those empty frames instead, so the upgrade path needs its own explicit UDX-level handoff packet.Co-authored with AI