Pre-connect to closestNodes in parallel to FIND_PEER query#245
Merged
lejeunerenard merged 8 commits intonodes-in-keyfrom Apr 8, 2026
Merged
Pre-connect to closestNodes in parallel to FIND_PEER query#245lejeunerenard merged 8 commits intonodes-in-keyfrom
closestNodes in parallel to FIND_PEER query#245lejeunerenard merged 8 commits intonodes-in-keyfrom
Conversation
Given `relayAddresses` (referred to as `closestNodes` in `dht-rpc`) are passed when connecting, these nodes could be used to connect through immediately while the normal `FIND_PEER` query is run. The closestNodes are likely candidates for connecting to a specific peer so attempting connection can speed up connection by skipping querying the DHT. To accommodate the extra connection, the total connections allowed is increased by 1 when closestNodes are passed. If a closestNode is returned as part of the findPeer query, then connection is skipped assuming that the preConnect parallel approach will establish the connection.
In the same way that connectThroughNode may work for a node returned via a `FIND_PEER` query, closestNodes attempted as part of preconnect should also be tracked for caching.
In the same way that connectThroughNode may work for a node returned via a `FIND_PEER` query, closestNodes attempted as part of preconnect should also be tracked for caching.
`preConnect` can be `true` while `closestNodes` is `null` when the `closestNodes` were set initially but then cleared when the query makes a second attempt.
This test previously wasn't likely to emit two `open` events, but since the pre-connect feature was added, if the client has the servers `id` in it's cache and populates the `closestNodes` with it, it will immediately connect to the server via the relay node simultaneously. This allows the encrypted stream to `open` twice instead of the normal 1 time because the connection to the relay has opened but the connection from the relay to the server will error when a duplicate connection is detected.
mafintosh
reviewed
Apr 7, 2026
lib/connect.js
Outdated
| if ( | ||
| preConnect && | ||
| closestNodes && | ||
| closestNodes.find((n) => n.host === data.from.host && n.port === data.from.port) |
Contributor
There was a problem hiding this comment.
make a static function isClosestNode(closestNodes, data) or similar and do a simple loop in that
mafintosh
reviewed
Apr 7, 2026
lib/connect.js
Outdated
| // Skip node already run via preConnect | ||
| if ( | ||
| preConnect && | ||
| closestNodes && |
Contributor
There was a problem hiding this comment.
if preconnect is set closestNodes is set afaik
Contributor
There was a problem hiding this comment.
Also does the length check when setting preconnect
Contributor
Author
There was a problem hiding this comment.
They are set together only initially. closestNodes will be nulled after the first try here:
https://github.com/holepunchto/hyperdht/pull/245/changes#diff-459561ac30417a9e40a7904e9a3c297a6c840189bf21583d951746a1d43a506aL372
I actually hit this when testing across the stack. The second try is incase the closestNodes are not valid and a fully new query needs to be done (much like the current implementation).
Drache93
reviewed
Apr 7, 2026
lib/connect.js
Outdated
|
|
||
| // 2 is how many parallel connect attempts we want to do, we can make this configurable | ||
| const sem = new Semaphore(2) | ||
| const sem = new Semaphore(closestNodes !== null ? 3 : 2) |
Contributor
There was a problem hiding this comment.
Do you want the length check here also? Otherwise we'll be doing 3 even if cloestNodes empty
Contributor
Author
There was a problem hiding this comment.
great point. i'll adjust
| closestNodes, | ||
| onlyClosestNodes: closestNodes !== null, | ||
| retries: closestNodes ? 1 : 3 | ||
| retries: 3 |
Contributor
There was a problem hiding this comment.
Is this less about the preConnect changes. And more the general insight we found that retries === 1 is just too little?
Contributor
Author
There was a problem hiding this comment.
This is because the 1 retry count only applied when it only queried closest nodes. Now that closest nodes are only used to help get new nodes here (aka we query vs trying to immediately connect) and onlyClosestNodes is always false, it can treat it as a general query that we don't need to bail early from if it fails.
lib/connect.js
Outdated
| // Skip node already run via preConnect | ||
| if ( | ||
| preConnect && | ||
| closestNodes && |
Contributor
There was a problem hiding this comment.
Also does the length check when setting preconnect
…Node` Moved to a function with a simple for loop instead of using `.find()`.
Without this check, 3 query nodes would attempt connecting at once if `closestNodes` is an empty array. Pre-connnect wouldn't run so they should be the same.
Often `closestNodes` & `relayAddresses` are the same, but they are distinct. `closestNodes` are DHT nodes that are closest (via XOR metric) to the target. `relayAddresses` are nodes that will serve as the relay node for doing the handshake with a given peer.
lejeunerenard
added a commit
that referenced
this pull request
Apr 9, 2026
* Add support for encoding closest nodes in key * flip order of providedNodes * simplify test * tidy * Swap to using hyperdht-address * update test * review fixes * Pre-connect to `closestNodes` in parallel to `FIND_PEER` query (#245) * Use closestNodes to connect in parallel to findPeer query Given `relayAddresses` (referred to as `closestNodes` in `dht-rpc`) are passed when connecting, these nodes could be used to connect through immediately while the normal `FIND_PEER` query is run. The closestNodes are likely candidates for connecting to a specific peer so attempting connection can speed up connection by skipping querying the DHT. To accommodate the extra connection, the total connections allowed is increased by 1 when closestNodes are passed. If a closestNode is returned as part of the findPeer query, then connection is skipped assuming that the preConnect parallel approach will establish the connection. * Add closestNode address to remoteRelayAddresses when attempting In the same way that connectThroughNode may work for a node returned via a `FIND_PEER` query, closestNodes attempted as part of preconnect should also be tracked for caching. * Add closestNode address to remoteRelayAddresses when attempting In the same way that connectThroughNode may work for a node returned via a `FIND_PEER` query, closestNodes attempted as part of preconnect should also be tracked for caching. * Check that `closestNodes` is set before skipping node in `FIND_PEER` `preConnect` can be `true` while `closestNodes` is `null` when the `closestNodes` were set initially but then cleared when the query makes a second attempt. * Filter out double `open` events in server side pool test This test previously wasn't likely to emit two `open` events, but since the pre-connect feature was added, if the client has the servers `id` in it's cache and populates the `closestNodes` with it, it will immediately connect to the server via the relay node simultaneously. This allows the encrypted stream to `open` twice instead of the normal 1 time because the connection to the relay has opened but the connection from the relay to the server will error when a duplicate connection is detected. * Split out logic to detect if `FIND_PEER` node is an existing `closestNode` Moved to a function with a simple for loop instead of using `.find()`. * Use `preConnect` bool to set number of semaphore waits Without this check, 3 query nodes would attempt connecting at once if `closestNodes` is an empty array. Pre-connnect wouldn't run so they should be the same. * Rename `closestNodes` to `relayAddresses` in `findAndConnect()` Often `closestNodes` & `relayAddresses` are the same, but they are distinct. `closestNodes` are DHT nodes that are closest (via XOR metric) to the target. `relayAddresses` are nodes that will serve as the relay node for doing the handshake with a given peer. --------- Co-authored-by: Sean Zellmer <sean@lejeunerenard.com>
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
3 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.
Builds on the
nodes-in-keyPR #237 to useclosestNodesfrom either the key, cache or passed option and attempts connecting through theclosestNodesin parallel to querying the DHT for more fresh nodes. This should skip the query when the cached node is still good and will speed up connections especially on slower devices where querying the DHT is more expensive because of latency or dropped packets.The
closestNodesare still passed to theFIND_PEERto speed up the query in the case that the closestNodes have updated their record of thetarget. The normal 2 tries is kept.