| … | … | @@ -747,3 +747,49 @@ topic carrying stale records, which is why it compounds with defect B. |
| 747 | 747 | attempt that never opened has no stream to close. What we lack is (a) an |
| 748 | 748 | explicit-error path that aborts without waiting, and (b) a connect timeout in |
| 749 | 749 | JS's range rather than 30s. Those are two separate changes with separate gates. |
| 750 | ||
| 751 | ### 11.2 Defect C, third reading — and the fix (DONE) | |
| 752 | ||
| 753 | ⚠ I characterised this defect three times before getting it right. §11 said | |
| 754 | "leak" (wrong — the timeout always fires). §11.1 said "10-15s vs 30s" (closer, | |
| 755 | but the wrong fix). Reading `connect.js` all the way through gives the actual | |
| 756 | shape: | |
| 757 | ||
| 758 | **JS HAS NO OVERALL CONNECT TIMER.** Its 10s/15s timers are for the punch and | |
| 759 | relay stages. The find_peer stage ends when the ITERATIVE QUERY ITSELF finishes | |
| 760 | (`connect.js:381`, `for await (const data of c.query)`). A peer that is simply | |
| 761 | not there therefore fails as fast as the DHT can say so. | |
| 762 | ||
| 763 | Ours held the connection in `DHT_CONN_FINDING` until | |
| 764 | `DHT_CONNECT_TIMEOUT_MS` (30s) regardless — the DHT had already answered and we | |
| 765 | waited half a minute to act on it. The plumbing to know better existed and was | |
| 766 | never joined up: | |
| 767 | ||
| 768 | find_peer() registers a query_info with is_find_peer hyperdht.cpp:168 | |
| 769 | HyperDht::on_query_complete(qid) hyperdht.cpp:103 | |
| 770 | DhtConnection::find_peer_qid dht_connector.h:56 | |
| 771 | ||
| 772 | Three pieces, no wire between them. `DhtConnector` subscribed to | |
| 773 | `on_peer_found` — the SUCCESS event — and to nothing that says "the search is | |
| 774 | over". | |
| 775 | ||
| 776 | **Fix:** `DhtConnector::on_find_peer_complete(query_id)` fails every connection | |
| 777 | still in FINDING on that query, and `udx_transport` routes `on_query_complete` | |
| 778 | into it. The 30s timeout stays as the backstop it should always have been. | |
| 779 | ||
| 780 | With `SWARM_MAX_PARALLEL = 3`, this is the difference between three dead peers | |
| 781 | monopolising every outbound slot for 30 seconds each and releasing them the | |
| 782 | moment the DHT reports nothing. Stale announce records — which we produce (A, | |
| 783 | and B's missing liveness) — are exactly that supply of dead peers. | |
| 784 | ||
| 785 | **Gate:** `test/test_find_peer_exhausted.cpp`. Written against a STUB first: | |
| 786 | ||
| 787 | stub 3/5 "the attempt failed as soon as find_peer exhausted" FAILS | |
| 788 | implemented 5/5 | |
| 789 | ||
| 790 | It also asserts no SECOND failure fires when the clock later reaches the old | |
| 791 | timeout, so a fix that merely fired twice would not pass. | |
| 792 | ||
| 793 | ⚠ Not addressed: the explicit-error abort path (§11.1), where JS aborts | |
| 794 | immediately on a relay error and we still wait the timeout. Separate change, | |
| 795 | separate gate. |
| … | … | @@ -682,6 +682,36 @@ void DhtConnector::_process_holepunch(DhtConnection *conn, |
| 682 | 682 | } |
| 683 | 683 | } |
| 684 | 684 | |
| 685 | const DhtConnection *DhtConnector::find_connection(uint32_t id) const { | |
| 686 | for (int i = 0; i < DHT_MAX_CONNECTIONS; i++) { | |
| 687 | if (_connections[i].active && _connections[i].id == id) return &_connections[i]; | |
| 688 | } | |
| 689 | return nullptr; | |
| 690 | } | |
| 691 | ||
| 692 | void DhtConnector::on_find_peer_complete(uint32_t query_id) { | |
| 693 | // The DHT has finished looking and, for any connection still in FINDING, | |
| 694 | // found nobody — a peer that answered would have moved the connection on | |
| 695 | // via on_peer_found. Waiting out DHT_CONNECT_TIMEOUT_MS for an answer we | |
| 696 | // already have is what parked the slot for 30 seconds. | |
| 697 | for (int i = 0; i < DHT_MAX_CONNECTIONS; i++) { | |
| 698 | DhtConnection *conn = &_connections[i]; | |
| 699 | if (!conn->active) continue; | |
| 700 | if (conn->state != DHT_CONN_FINDING) continue; | |
| 701 | if (conn->find_peer_qid != query_id) continue; | |
| 702 | ||
| 703 | conn->state = DHT_CONN_FAILED; | |
| 704 | conn->error = "peer not found"; | |
| 705 | DHT_LOG("[DhtConnector] find_peer exhausted for conn %u — failing now " | |
| 706 | "rather than at the timeout\n", conn->id); | |
| 707 | if (on_failed) on_failed(conn->id, conn->remote_public_key, conn->error); | |
| 708 | _free_connection(conn); | |
| 709 | // _free_connection may compact, so do not advance i — the entry now at | |
| 710 | // this index has not been examined. Same rule as the handshake sweep. | |
| 711 | i--; | |
| 712 | } | |
| 713 | } | |
| 714 | ||
| 685 | 715 | void DhtConnector::on_probe_received(const IPv4Address &from) { |
| 686 | 716 | // Route the probe to the puncher that's actually punching THIS peer. |
| 687 | 717 | // |
| … | … | @@ -185,6 +185,23 @@ public: |
| 185 | 185 | // Handle a raw UDP probe received on the socket. |
| 186 | 186 | void on_probe_received(const IPv4Address &from); |
| 187 | 187 | |
| 188 | // ⛔ THE FIND_PEER QUERY EXHAUSTED. Fail every connection still waiting on | |
| 189 | // it, instead of holding the slot until DHT_CONNECT_TIMEOUT_MS. | |
| 190 | // | |
| 191 | // JS has NO overall connect timer: its find_peer stage ends when the | |
| 192 | // iterative query itself finishes (connect.js:381 `for await (const data of | |
| 193 | // c.query)`), so a peer that is simply not there fails as fast as the DHT | |
| 194 | // can say so. Ours waited the full 30s wall clock for the same answer, | |
| 195 | // which with SWARM_MAX_PARALLEL = 3 parks every outbound slot on peers the | |
| 196 | // DHT has already reported nothing for. Stale announce records — which we | |
| 197 | // produce, see defects A and B — are exactly that case. | |
| 198 | void on_find_peer_complete(uint32_t query_id); | |
| 199 | ||
| 200 | // Read-only lookup, for diagnostics and gates. Does not extend the | |
| 201 | // connection's life: the pointer is into the fixed array and is valid only | |
| 202 | // until the next connect/free. | |
| 203 | const DhtConnection *find_connection(uint32_t id) const; | |
| 204 | ||
| 188 | 205 | // Get the local IPv4 addresses we advertise in handshakes. |
| 189 | 206 | void set_local_addresses(const IPv4Address *addrs, int count); |
| 190 | 207 |
| … | … | @@ -1380,6 +1380,15 @@ void UdxTransport::_init_dht() { |
| 1380 | 1380 | } |
| 1381 | 1381 | }; |
| 1382 | 1382 | |
| 1383 | // ⛔ TELL THE CONNECTOR WHEN A QUERY EXHAUSTS. Without this it learns that a | |
| 1384 | // peer is unreachable only from DHT_CONNECT_TIMEOUT_MS, 30 seconds after the | |
| 1385 | // DHT already said so, and holds one of SWARM_MAX_PARALLEL slots the whole | |
| 1386 | // time. JS has no overall connect timer at all — its find_peer stage simply | |
| 1387 | // ends when the query does. See docs/PARITY-hyperswarm.md §11.2. | |
| 1388 | _hyperdht->on_query_complete = [this](uint32_t query_id) { | |
| 1389 | if (_dht_connector) _dht_connector->on_find_peer_complete(query_id); | |
| 1390 | }; | |
| 1391 | ||
| 1383 | 1392 | // Hook holepunch callbacks — route DHT events to the connector. |
| 1384 | 1393 | _hyperdht->on_holepunch_request = [this](const DhtRequest &req) { |
| 1385 | 1394 | if (_dht_connector) { |
| … | … | @@ -244,3 +244,15 @@ g++ -o test_handshake_pool \ |
| 244 | 244 | $UDX_OBJS $LDFLAGS |
| 245 | 245 | echo " OK: test_handshake_pool" |
| 246 | 246 | echo "=== All test binaries built successfully ===" |
| 247 | ||
| 248 | # ---- test_find_peer_exhausted ---- | |
| 249 | # A connect attempt must fail when find_peer EXHAUSTS, not 30s later. See | |
| 250 | # docs/PARITY-hyperswarm.md §11.2. | |
| 251 | echo "Building test_find_peer_exhausted..." | |
| 252 | g++ $CXXFLAGS -o $OBJ/test_find_peer_exhausted.o test_find_peer_exhausted.cpp | |
| 253 | g++ -o test_find_peer_exhausted \ | |
| 254 | $OBJ/test_find_peer_exhausted.o $OBJ/noise_ik.o $OBJ/noise_handshake.o \ | |
| 255 | $OBJ/noise_stream.o $OBJ/dht_rpc.o $OBJ/holepuncher.o $OBJ/hyperdht.o \ | |
| 256 | $OBJ/dht_connector.o \ | |
| 257 | $UDX_OBJS $LDFLAGS | |
| 258 | echo " OK: test_find_peer_exhausted" |
| … | … | @@ -0,0 +1,103 @@ |
| 1 | // Gate: a connect attempt must fail when the find_peer query EXHAUSTS, not | |
| 2 | // when a 30-second wall clock expires. | |
| 3 | // | |
| 4 | // docs/PARITY-hyperswarm.md §11.2. JS has no overall connect timer at all — | |
| 5 | // its find_peer stage ends when the iterative query finishes | |
| 6 | // (connect.js:381), so a peer that is not there fails as fast as the DHT can | |
| 7 | // say so, and the connect slot is released. Ours held the slot for | |
| 8 | // DHT_CONNECT_TIMEOUT_MS regardless. With SWARM_MAX_PARALLEL = 3 that is three | |
| 9 | // dead peers monopolising every outbound slot for 30s each, and a topic | |
| 10 | // carrying stale announce records is precisely a supply of dead peers. | |
| 11 | // | |
| 12 | // RED before the fix: on_find_peer_complete is a stub, so nothing fails until | |
| 13 | // the timeout, and this reports the slot still held. | |
| 14 | ||
| 15 | #include <cstdio> | |
| 16 | #include <cstring> | |
| 17 | ||
| 18 | #include <sodium.h> | |
| 19 | ||
| 20 | #include "dht_connector.h" | |
| 21 | #include "hyperdht.h" | |
| 22 | ||
| 23 | extern "C" { | |
| 24 | #include <udx.h> | |
| 25 | #include <uv.h> | |
| 26 | } | |
| 27 | ||
| 28 | static int checks = 0, failures = 0; | |
| 29 | ||
| 30 | static void check(bool cond, const char *what) { | |
| 31 | checks++; | |
| 32 | if (cond) { printf(" PASS: %s\n", what); return; } | |
| 33 | failures++; | |
| 34 | printf(" FAIL: %s\n", what); | |
| 35 | } | |
| 36 | ||
| 37 | int main() { | |
| 38 | if (sodium_init() < 0) { printf("sodium_init failed\n"); return 1; } | |
| 39 | ||
| 40 | uv_loop_t loop; | |
| 41 | uv_loop_init(&loop); | |
| 42 | udx_t udx; | |
| 43 | udx_init(&loop, &udx, nullptr); | |
| 44 | udx_socket_t sock; | |
| 45 | udx_socket_init(&udx, &sock, nullptr); | |
| 46 | struct sockaddr_in addr; | |
| 47 | uv_ip4_addr("127.0.0.1", 0, &addr); | |
| 48 | udx_socket_bind(&sock, (struct sockaddr *)&addr, 0); | |
| 49 | ||
| 50 | DhtRpc rpc; | |
| 51 | rpc.init(&sock, &loop); | |
| 52 | ||
| 53 | Ed25519KeyPair kp; | |
| 54 | crypto_sign_keypair(kp.public_key, kp.secret_key); | |
| 55 | ||
| 56 | HyperDht dht; | |
| 57 | dht.init(&rpc, kp); | |
| 58 | ||
| 59 | DhtConnector connector; | |
| 60 | connector.init(&dht, &sock, &loop); | |
| 61 | ||
| 62 | int failed_count = 0; | |
| 63 | connector.on_failed = [&failed_count](uint32_t, const uint8_t *, const char *err) { | |
| 64 | failed_count++; | |
| 65 | printf(" (on_failed: %s)\n", err ? err : "?"); | |
| 66 | }; | |
| 67 | ||
| 68 | // A peer that does not exist. find_peer will be issued and will find nobody. | |
| 69 | uint8_t remote_pk[32]; | |
| 70 | randombytes_buf(remote_pk, 32); | |
| 71 | ||
| 72 | uint32_t conn_id = connector.connect(remote_pk); | |
| 73 | check(conn_id != 0, "connect() started an attempt"); | |
| 74 | ||
| 75 | // The query the connector is waiting on. | |
| 76 | const DhtConnection *conn = connector.find_connection(conn_id); | |
| 77 | check(conn != nullptr, "the connection exists"); | |
| 78 | if (!conn) { printf("\n%d/%d passed\n", checks - failures, checks); return 1; } | |
| 79 | const uint32_t qid = conn->find_peer_qid; | |
| 80 | ||
| 81 | // Nothing has failed yet: the query is still running. | |
| 82 | check(failed_count == 0, "nothing failed while the query was still running"); | |
| 83 | ||
| 84 | // ⛔ THE QUERY EXHAUSTS. This is the event JS acts on. | |
| 85 | connector.on_find_peer_complete(qid); | |
| 86 | ||
| 87 | check(failed_count == 1, | |
| 88 | "the attempt failed as soon as find_peer exhausted"); | |
| 89 | ||
| 90 | // And it must not have needed the wall clock. Tick to just under the old | |
| 91 | // timeout: if the fix works nothing more happens; if it does not, this is | |
| 92 | // where the failure would finally have been reported. | |
| 93 | connector.tick(DHT_CONNECT_TIMEOUT_MS - 1000); | |
| 94 | check(failed_count == 1, | |
| 95 | "no second failure, and the slot was not held to the timeout"); | |
| 96 | ||
| 97 | printf("\n%d/%d passed\n", checks - failures, checks); | |
| 98 | ||
| 99 | udx_socket_close(&sock); | |
| 100 | uv_run(&loop, UV_RUN_NOWAIT); | |
| 101 | uv_loop_close(&loop); | |
| 102 | return failures ? 1 : 0; | |
| 103 | } |