Repository navigation
Phase A8 to main (re-landing #72, which merged into its stack base) - #73
Merged
Merged
Conversation
* Phase A8: DhtNode assembly and layering Completes Milestone A: the full DhtNode composed over a given transport (PeerManager, Router, RecursiveOperationManager, StoreManager, PeerDiscovery on one RoutingRpcCommunicator), implementing the Transport interface so a layer-1 DhtNode runs over a layer-0 DhtNode, plus the external API (ExternalApiRpcLocal/Remote: storeData / externalFetchData / externalFindClosestNodes) and the join/store/fetch/find public API. The ConnectionManager/real-socket construction branch is deferred to milestone B; every test in this phase drives the node through a transport. Tests ported from v103.8.0-rc.3 (all green, 24 integration + 172 unit): - integration/MultipleEntryPointJoining.test.ts (3 tests; exercised the phase-AA concurrent-connect fixes) - integration/DhtNode.test.ts (FakeTransport + RPC-stub members) - integration/Mock-Layer1-Layer0.test.ts (layer-1 topology == layer-0) - integration/Layer1-scale.test.ts (48 layer-0 nodes; single and 4-service layer-1 networks, ~245 nodes total in the second test) - integration/DhtNodeExternalAPI.test.ts (4 tests) - benchmark/KademliaCorrectness.test.ts as a slow correctness test: 200 nodes, ground truth computed in-test from XOR distance instead of the TS pre-generated data files, and the statistics asserted (observed: avg 6.4/8 exact-prefix-correct neighbours). Test util: DhtNodeTestUtils.hpp (createMockConnectionDhtNode / createMockConnectionLayer1Node) — a plain header, not a module: a module importing DhtNode's aggregated interface exhausts clang's source locations. Also fixes a real bug the 48-node scale test exposed (heap-buffer-overflow caught by ASan): ConnectionManager::getConnections() materialized a views::filter pipeline with ranges::to<vector>, which walks a forward range twice (distance, then copy) — endpoint->isConnected() is live state, so an endpoint connecting between the passes made the copy yield more elements than were counted. Replaced with a single-pass loop. Lint infrastructure hardening for the grown module graph: - clangd accumulates source-location space across the files one process serves; the batch is now chunked (13 files per clangd process) and the heaviest full-node tests run one per process. - clangd-tidy spawned by xargs inherited the exhausted file-list pipe as stdin, killing clangd at startup; stdin is now /dev/null. - clangd's background index is disabled project-wide (.clangd): back-to-back short-lived clangd processes crashed reading each other's half-written index shards. - DhtNodeTest.cpp, MultipleEntryPointJoiningTest.cpp and DhtNodeTestUtils.hpp join the documented clangd-tidy exclusions (std-type-unification false positives / plain header without a compile command); clang-format still checks them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * KademliaCorrectness: add the order-insensitive known-of-closest-8 metric The TS prefix metric truncates at the first mis-ordered entry, conflating "does not know its closest peers" (a real discovery failure) with "knows them in slightly different order" (inherent to Kademlia's liveness-biased buckets). The new statistic counts how many of the true closest-8 appear anywhere in the node's top-8. Observed at 200 nodes: 6.98/8 known vs 6.40/8 prefix-exact — so on average one true-closest peer is genuinely absent (sequential joins learn later-joining closer peers only passively, and the assertion runs right after the last join, before any recovery cycle) and half an entry is mere ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
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
added a commit
that referenced
this pull request
Jul 10, 2026
…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>
ptesavol
added a commit
that referenced
this pull request
Jul 10, 2026
…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>
ptesavol
added a commit
that referenced
this pull request
Jul 10, 2026
* Phase B1 (part 1): connectivity checker, request handler, signed peer 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> * Phase B1 (part 2): real connectivity checking and websocket reverse connections 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> * CI: raise ctest per-test timeout to clear the heavy DHT convergence tests 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> * WebsocketServerConnector: move requestConnection notifications to the 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> * DEBUG: instrument the reverse-connection notify path (temporary) 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> * Revert "DEBUG: instrument the reverse-connection notify path (temporary)" This reverts commit 2f5bcbe. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.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
1 participant
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.
Why this PR exists: the stacked-PR trap struck again — #72's base was
claude/phase-aa-to-main, so merging it landed the A8 squash into that branch, not into main (same as #70 before it; #71 re-landed that one). This PR cherry-picks the #72 squash commit onto main; the resulting tree is bit-identical (git diffempty) to the mergedclaude/phase-aa-to-maintip, so all of #72's verification applies as-is (172 unit + 24 integration green, lint exit 0).See #72 for the full phase A8 description (DhtNode assembly, layer-1-over-layer-0, external API, all A8 tests including the 200-node KademliaCorrectness, the
getConnections()heap-overflow fix, and the lint hardening).Suggestion to avoid this a third time: for stacked PRs, either merge bottom-up and let GitHub auto-retarget the next PR to main after each merge (it retargets only when the base branch is deleted on merge), or manually change the PR's base to
mainbefore merging once its parent has landed.🤖 Generated with Claude Code