Skip to content

Phase B1: connectivity checking and signed peer descriptors - #74

Merged
ptesavol merged 6 commits into
mainfrom
claude/phase-b1
Jul 10, 2026
Merged

ptesavol merged 6 commits into
mainfrom
claude/phase-b1

Conversation

@ptesavol

Copy link
Copy Markdown
Collaborator

Implements Phase B1 of trackerless-network-completion-plan.md (Milestone B — direct connectivity): the NAT connectivity-check protocol, signed peer descriptors, and the websocket reverse-connection path, ported line-by-line from the pinned TypeScript (af966cf03, v103.8.0-rc.3).

Part 1 — new modules

  • connectivityChecker (client side): connectSync (TS connectAsync) opens a raw websocket and waits for it to come up; sendConnectivityRequest runs the check against one entry point and returns its ConnectivityResponse. Deliberately synchronous-blocking: both entry points run on dedicated worker threads, never on an event-dispatch or RPC thread; listeners are registered before connect()/send() so a fast completion cannot be lost.
  • connectivityRequestHandler (server side): attached to incoming connections carrying the connectivityRequest action; probes the requester's advertised websocket server (connectivityProbe action) unless probing is disabled (port 0), and replies with a ConnectivityResponse. Runs on a dedicated worker executor so the probe cannot stall the websocket dispatch thread. GeoIP is omitted (deferred to milestone E).
  • createPeerDescriptor + createPeerDescriptorSignaturePayload: the signed local peer descriptor. Node id = concat(last 13 bytes of keccak(ip), last 7 bytes of sign(ip)); the keccak and signature use the Ethereum message magic (TS EcdsaSecp256k1Evm defaults) via SigningUtils — wire-exact. Like TS (its TODO), the key pair is throwaway.
  • New error types ConnectionFailed, ConnectivityResponseTimeout.

Part 2 — wiring and reverse connections

  • WebsocketServerConnector.start(): the connectivityRequest action now attaches the handler; connectivityProbe sockets are pinned by a listener until the prober closes them (C++ has no GC to keep an unowned server socket alive; onClosed drops all listeners, releasing the pin).
  • WebsocketServerConnector.checkConnectivity(): real flow — state snapshot under the lock, network outside it; shuffled entry points; 2 s abortable wait between failed attempts; WebsocketServerStartError with the attempted hosts after the list is exhausted. The no-entry-points branch tolerates a serverless connector (empty host/port, like the TS undefined fields).
  • Reverse connections (connect / isPossibleToFormConnection / requestConnectionFromPeer): a node with a websocket server asks a serverless peer over the signaling transport to open a websocket back to it; the pending connection completes when the incoming handshake matches ongoingConnectRequests. The notification runs detached on a dedicated executor that destroy() joins after abort (the PeerManager::stop() pattern).
  • DefaultConnectorFacade: routes createConnection through the server connector when the client connector cannot form the connection (TS falls through to WebRTC next — milestone B2), and passes the new entryPoints option through. externalIp only feeds the WebRTC connector in TS, so it lands with B2; TLS/auto-certify options stay deferred.

Two real bugs found by the new tests

  1. WebsocketClientConnectorRpcRemote::requestConnection SEGV on first real use. It returned the generated client's task, but RpcCommunicatorClientApi::notify is a lazy coroutine taking const RequestType& — by the time the task first ran, the locals it referenced were destroyed (ASan: SEGV in Any::PackFrom). Now a proper coroutine: co_await keeps the locals alive in its own frame. Repo swept for other return this->getClient()... instances — this was the only one.
  2. Connectivity-request server sockets had no owner. attachConnectivityRequestHandler captured the connection weakly, so the socket was destroyed as soon as WebsocketServer released its half-ready reference — the request never got processed (the handshake path survives because the handshaker owns its socket; TS survives on GC). The Data listener now holds the connection strongly; the cycle is broken by onClosed's remove-all-listeners. The executor task still captures weakly, so a connection that closes while queued is simply dropped.

Tests

Ported (v103.8.0-rc.3): unit/createPeerDescriptor.test.ts, unit/connectivityRequestHandler.test.ts (real websocket server as probe target), integration/ConnectivityChecking.test.ts, integration/Websocket.test.ts (round-trip case added to the existing WebsocketClientServerTest), integration/WebsocketConnectionManagement.test.ts (all five cases, including both serverless reverse-connection directions and connect-to-self rejection). unit/ConnectivityHelpers.test.ts was already covered by the existing ConnectivityTest.

  • Unit suite: 177/177; integration suite: 31/31; the new tests also pass under AddressSanitizer.
  • lint.sh green; the three new test files that trip the known clangd std-type-unification false positive are added to the documented exclusion list (compiler builds and runs them; clang-format still checks them).

🤖 Generated with Claude Code

@cursor

cursor Bot commented Jul 10, 2026

Copy link
Copy Markdown

Bugbot is not enabled for this team, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@cursor

cursor Bot commented Jul 10, 2026 •

Copy link
Copy Markdown

Bugbot is not enabled for this team, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

ptesavol and others added 4 commits July 10, 2026 23:31
… descriptors

Ports the first half of milestone B1 from v103.8.0-rc.3:

- createPeerDescriptorSignaturePayload + createPeerDescriptor: the local
  node's signed peer descriptor built from a ConnectivityResponse; the node
  id derives from the reported ip (last 13 bytes of keccak(ip) + last 7
  bytes of sign(ip)) and the descriptor is signed over the payload — both
  through SigningUtils' Ethereum-magic keccak/secp256k1, matching the TS
  EcdsaSecp256k1Evm defaults bit-exactly (wire-visible). Like the TS TODO,
  the key pair is throwaway (random private key, 20 random publicKey bytes).
- connectivityChecker (client side): connectSync opens a raw websocket
  client connection with a connect timeout; sendConnectivityRequest runs
  the connectivity check against an entry point and returns its
  ConnectivityResponse (5 s response timeout, protocol-version check).
  Deliberately synchronous-blocking: both call sites run on dedicated
  worker threads, and the listeners are registered before connect()/send()
  so a fast completion cannot be lost.
- connectivityRequestHandler (server side): parses ConnectivityRequests off
  a connection's data events, probes the requester's advertised websocket
  server (action=connectivityProbe) unless the port is 0, and replies with
  a ConnectivityResponse. Runs on a dedicated magic-static executor so the
  probe never blocks the websocket dispatch thread. GeoIP lookup omitted
  (deferred to milestone E). Template over the connection type so the unit
  test drives it with a mock (the TS test uses a bare EventEmitter).
- New error types ConnectionFailed / ConnectivityResponseTimeout.

Tests ported (all green, unit suite 177/177):
- unit/createPeerDescriptor.test.ts -> CreatePeerDescriptorTest (3 tests)
- unit/connectivityRequestHandler.test.ts -> ConnectivityRequestHandlerTest
  (2 tests; the happy path exercises a real websocket probe round trip
  against a local WebsocketServer)

Also records in the plan the owner's manual follow-up: compare the
KademliaCorrectness statistics against the TypeScript implementation (the
TS benchmark is bit-rotted at the pin; C++ numbers and the diagnosis are in
the note).

Still to come in B1 (part 2): wiring the handler into
WebsocketServerConnector's connectivityRequest/connectivityProbe actions,
the real client-side checkConnectivity flow in the connector facade
(honoring externalIp/websocketHost/port-range options), and the
ConnectivityChecking/Websocket/WebsocketConnectionManagement integration
tests.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…onnections

WebsocketServerConnector now runs the ported TS flows instead of stubs:
- start() wires attachConnectivityRequestHandler into the
  connectivityRequest action and pins connectivityProbe sockets until the
  prober closes them (C++ has no GC to keep an unowned server socket
  alive; the pin is released by onClosed dropping all listeners).
- checkConnectivity() does real connectivity checking against shuffled
  entry points with 2 s abortable waits between attempts and
  WebsocketServerStartError after the list is exhausted; the
  options-info branch tolerates a serverless connector (empty host/port,
  like the TS undefined fields).
- connect()/isPossibleToFormConnection()/requestConnectionFromPeer():
  the reverse-connection path — ask a serverless peer over the signaling
  transport to open a websocket back to this node's server; the RPC
  notification runs detached on a dedicated executor that destroy()
  joins after abort (the PeerManager::stop() pattern).

DefaultConnectorFacade routes createConnection through the server
connector when the client connector cannot form the connection, and
passes the new entryPoints option through (externalIp and the TLS/
autocertify options stay deferred to B2/E).

Fixed in passing (found by the new tests):
- WebsocketClientConnectorRpcRemote::requestConnection returned the lazy
  notify() task whose const& parameters referenced already-destroyed
  locals (SEGV in Any::PackFrom on first use); it now co_awaits so the
  locals live in its own coroutine frame.
- attachConnectivityRequestHandler captured the connection weakly, so a
  connectivityRequest server socket had no owner and was destroyed
  before the request arrived; the Data listener now holds it strongly
  (cycle broken by onClosed's removeAllListeners).

Tests ported (v103.8.0-rc.3): integration/ConnectivityChecking.test.ts,
integration/Websocket.test.ts (round-trip case added to the existing
WebsocketClientServerTest), integration/
WebsocketConnectionManagement.test.ts (all five cases, including the
serverless reverse-connection ones). lint.sh: three new files added to
the documented clangd std-type-unification exclusion list.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ests

The macOS CI leg has been red since PR #73 landed: ctest's --timeout 300
(sized when the whole suite ran in ~35 s) kills
Layer1ScaleTest.MultipleLayer1Dht (~214 s on a fast 10-core dev machine,
longer on the 3-core macOS runners) and KademliaCorrectnessTest, then
fails the until-pass retry the same way. The faster Linux runners stay
under the limit, which is why only macOS failed. 1200 s clears the heavy
tests with headroom while still converting a genuine hang into a failure.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… shared pool

Applies the PR #75 executor architecture to the one per-instance pool B1
added: the detached requestConnection notifications now run on a serial
view of the shared worker pool, tracked by a GuardedAsyncScope that
destroy() drains after abort (outside mMutex, the PeerManager::stop()
pattern); the scope's gate drops a connect() racing the drain.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The ubuntu-latest leg SIGSEGVs (address 0x20) deterministically in the
three WebsocketConnectionManagement reverse-connection tests while macOS
and local arm64 runs (including under ASan) are green. These INFO logs
bracket every step of requestConnectionFromPeer -> GuardedAsyncScope ->
ClientApi::notify so the next CI run shows the exact crash point.
Will be reverted once the cause is fixed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ptesavol
ptesavol merged commit 4f2c1dd into main Jul 10, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant