| … | … | @@ -790,9 +790,35 @@ and B's missing liveness) — are exactly that supply of dead peers. |
| 790 | 790 | It also asserts no SECOND failure fires when the clock later reaches the old |
| 791 | 791 | timeout, so a fix that merely fired twice would not pass. |
| 792 | 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. | |
| 793 | ### 11.3 Defect C, the other half — the explicit-error abort (DONE) | |
| 794 | ||
| 795 | §11.1's table said "explicit error: JS immediate, ours 30s". Three branches in | |
| 796 | `on_holepunch_response` logged and returned — envelope decode failure, payload | |
| 797 | decrypt failure, payload decode failure — leaving the attempt in HOLEPUNCHING | |
| 798 | until the wall clock, for a reply that had already arrived and could never | |
| 799 | become valid. They now fail the attempt at once, which is what | |
| 800 | `c.relayClient.pair(...).on('error', onabort)` (connect.js:814) does. | |
| 801 | ||
| 802 | ⛔ The EMPTY reply is deliberately NOT one of them. "The relay could not reach | |
| 803 | the peer" is the case JS covers with its 15s `relayTimeout`, not with `onabort`, | |
| 804 | and aborting there would make us MORE aggressive than the reference — a | |
| 805 | divergence in the other direction is still a divergence. | |
| 806 | ||
| 807 | **Gate:** `test/test_relay_error_abort.cpp`, 12 assertions, seven of them | |
| 808 | negative — because the id in a holepunch tag is reusable and a late reply must | |
| 809 | not abort whoever holds that id now. That guard predates this change and had no | |
| 810 | test, which is the shape §8.3 and §8.4 both describe: delete such a guard and | |
| 811 | the suite calls it a fix. | |
| 812 | ||
| 813 | fixed 12/12 | |
| 814 | mutated 9/12 (the three aborts reverted to a bare return) | |
| 815 | ||
| 816 | Reaching HOLEPUNCHING legitimately needs a relay, a live peer and a completed | |
| 817 | handshake, so `DhtConnector::set_state_for_gate` exists and is named so it | |
| 818 | cannot be mistaken for anything else. The alternative was an ungated abort path. | |
| 819 | ||
| 820 | ⏳ Still open from §11.1: our single 30s connect timeout against JS's staged | |
| 821 | 10s (open firewall) and 15s (relay). Separate change, separate gate. | |
| 796 | 822 | |
| 797 | 823 | ## 12. Slice 8 — defect B, first half: the self-announce happened ONCE (FIXED) |
| 798 | 824 |
| … | … | @@ -580,6 +580,30 @@ void DhtConnector::on_holepunch_message(const DhtRequest &req) { |
| 580 | 580 | _process_holepunch(conn, payload); |
| 581 | 581 | } |
| 582 | 582 | |
| 583 | // Fail one attempt NOW: report it and give the slot back. The timeout path does | |
| 584 | // the same thing, minus the random-punch bookkeeping, which only matters for a | |
| 585 | // connection that had reached a punching stage. | |
| 586 | void DhtConnector::_fail_connection(DhtConnection *conn, const char *reason) { | |
| 587 | if (!conn || !conn->active) return; | |
| 588 | conn->state = DHT_CONN_FAILED; | |
| 589 | conn->error = reason; | |
| 590 | const HolepunchStage pstage = conn->puncher.stage(); | |
| 591 | if ((pstage == HP_PUNCH_RANDOM || pstage == HP_PUNCH_BIRTHDAY) && | |
| 592 | _random_punch_count > 0) { | |
| 593 | _random_punch_count--; | |
| 594 | } | |
| 595 | DHT_LOG("[DhtConnector] conn %u failed: %s\n", conn->id, reason); | |
| 596 | if (on_failed) on_failed(conn->id, conn->remote_public_key, conn->error); | |
| 597 | _free_connection(conn); | |
| 598 | } | |
| 599 | ||
| 600 | bool DhtConnector::set_state_for_gate(uint32_t conn_id, DhtConnectionState state) { | |
| 601 | DhtConnection *conn = _find_connection(conn_id); | |
| 602 | if (!conn) return false; | |
| 603 | conn->state = state; | |
| 604 | return true; | |
| 605 | } | |
| 606 | ||
| 583 | 607 | bool DhtConnector::on_holepunch_response(uint32_t tag, const uint8_t *value, |
| 584 | 608 | size_t value_len, const IPv4Address &from) { |
| 585 | 609 | // Holepunch response tags are 0x10000 | conn_id. |
| … | … | @@ -598,9 +622,9 @@ bool DhtConnector::on_holepunch_response(uint32_t tag, const uint8_t *value, |
| 598 | 622 | // Decode the holepunch message envelope (mode=REPLY from relay). |
| 599 | 623 | EncodeState s = { 0, value_len, const_cast<uint8_t *>(value) }; |
| 600 | 624 | dht_msg::HolepunchMessage hmsg; |
| 601 | if (!dht_msg::holepunch_msg_decode(s, hmsg)) { | |
| 625 | const bool envelope_ok = dht_msg::holepunch_msg_decode(s, hmsg); | |
| 626 | if (!envelope_ok) { | |
| 602 | 627 | DHT_LOG("[DhtConnector] holepunch response decode failed\n"); |
| 603 | return true; | |
| 604 | 628 | } |
| 605 | 629 | |
| 606 | 630 | DHT_LOG("[DhtConnector] holepunch response: mode=%d id=%u payload_len=%zu\n", |
| … | … | @@ -616,15 +640,40 @@ bool DhtConnector::on_holepunch_response(uint32_t tag, const uint8_t *value, |
| 616 | 640 | } |
| 617 | 641 | } |
| 618 | 642 | if (!conn || conn->state < DHT_CONN_HOLEPUNCHING) { |
| 643 | // Not attributable to an attempt that is waiting on a relay. The id in | |
| 644 | // the tag can be reused after a connection is freed, so a late reply | |
| 645 | // must NOT be allowed to abort whatever holds that id now. | |
| 619 | 646 | DHT_LOG("[DhtConnector] holepunch response: no matching conn for id=%u\n", conn_id); |
| 620 | 647 | return true; |
| 621 | 648 | } |
| 622 | 649 | |
| 650 | // ⛔ AN UNREADABLE REPLY IS AN EXPLICIT ERROR, NOT A REASON TO WAIT. | |
| 651 | // | |
| 652 | // JS wires `c.relayClient.pair(...).on('error', onabort)` (connect.js:814): | |
| 653 | // a relay error destroys the session immediately. These three branches used | |
| 654 | // to log and return, leaving the attempt in HOLEPUNCHING until the 30-second | |
| 655 | // wall clock — for a reply that will never become valid, because it is the | |
| 656 | // answer to this request and it did not decode. | |
| 657 | // | |
| 658 | // docs/PARITY-hyperswarm.md §11.1: "explicit error | JS immediate | ours | |
| 659 | // 30s". With SWARM_MAX_PARALLEL = 3 that is an outbound slot parked on a | |
| 660 | // dead attempt for half a minute. | |
| 661 | // | |
| 662 | // The EMPTY reply above is deliberately NOT treated this way: "the relay | |
| 663 | // could not reach the peer" is the case JS covers with its 15s relayTimeout, | |
| 664 | // not with onabort, and failing it here would be more aggressive than the | |
| 665 | // reference rather than equal to it. | |
| 666 | if (!envelope_ok) { | |
| 667 | _fail_connection(conn, "holepunch reply did not decode"); | |
| 668 | return true; | |
| 669 | } | |
| 670 | ||
| 623 | 671 | // Decrypt the payload using the holepunch secret. |
| 624 | 672 | std::vector<uint8_t> pt; |
| 625 | 673 | if (!dht_msg::secure_payload_decrypt(hmsg.payload, hmsg.payload_len, |
| 626 | 674 | conn->holepunch_secret, pt)) { |
| 627 | 675 | DHT_LOG("[DhtConnector] holepunch response: decrypt failed\n"); |
| 676 | _fail_connection(conn, "holepunch reply failed to authenticate"); | |
| 628 | 677 | return true; |
| 629 | 678 | } |
| 630 | 679 | |
| … | … | @@ -632,7 +681,7 @@ bool DhtConnector::on_holepunch_response(uint32_t tag, const uint8_t *value, |
| 632 | 681 | EncodeState ps = { 0, pt.size(), pt.data() }; |
| 633 | 682 | dht_msg::HolepunchPayload payload; |
| 634 | 683 | if (!dht_msg::holepunch_payload_decode(ps, payload)) { |
| 635 | DHT_LOG("[DhtConnector] holepunch response: payload decode failed\n"); | |
| 684 | _fail_connection(conn, "holepunch payload did not decode"); | |
| 636 | 685 | return true; |
| 637 | 686 | } |
| 638 | 687 |
| … | … | @@ -202,6 +202,18 @@ public: |
| 202 | 202 | // until the next connect/free. |
| 203 | 203 | const DhtConnection *find_connection(uint32_t id) const; |
| 204 | 204 | |
| 205 | // A DOOR FOR GATES, and named so it cannot be mistaken for anything else. | |
| 206 | // | |
| 207 | // Reaching HOLEPUNCHING legitimately needs a relay, a live peer and a | |
| 208 | // completed Noise handshake — a whole swarm, to test one branch that runs | |
| 209 | // when a relay answers with something unreadable. The alternative to this | |
| 210 | // door is an ungated abort path, and this repo has already paid for guards | |
| 211 | // that had no test: deleting one then looks like a fix (§8.3). | |
| 212 | // | |
| 213 | // It moves ONE connection's state and nothing else. It is not used outside | |
| 214 | // test/. | |
| 215 | bool set_state_for_gate(uint32_t conn_id, DhtConnectionState state); | |
| 216 | ||
| 205 | 217 | // Get the local IPv4 addresses we advertise in handshakes. |
| 206 | 218 | void set_local_addresses(const IPv4Address *addrs, int count); |
| 207 | 219 | |
| … | … | @@ -248,6 +260,9 @@ private: |
| 248 | 260 | |
| 249 | 261 | // Connection management. |
| 250 | 262 | DhtConnection *_alloc_connection(bool initiator, const uint8_t *remote_pk); |
| 263 | // Fail one attempt immediately rather than at the wall clock. | |
| 264 | void _fail_connection(DhtConnection *conn, const char *reason); | |
| 265 | ||
| 251 | 266 | DhtConnection *_find_connection(uint32_t id); |
| 252 | 267 | DhtConnection *_find_connection_by_pk(const uint8_t *pk); |
| 253 | 268 | void _free_connection(DhtConnection *conn); |
| … | … | @@ -344,4 +344,16 @@ g++ -o test_live_lookup \ |
| 344 | 344 | $UDX_OBJS $LDFLAGS |
| 345 | 345 | echo " OK: test_live_lookup" |
| 346 | 346 | |
| 347 | # ---- test_relay_error_abort ---- | |
| 348 | # An unreadable holepunch reply must fail the attempt now, not at the 30s wall | |
| 349 | # clock. See docs/PARITY-hyperswarm.md §11.1. | |
| 350 | echo "Building test_relay_error_abort..." | |
| 351 | g++ $CXXFLAGS -o $OBJ/test_relay_error_abort.o test_relay_error_abort.cpp | |
| 352 | g++ -o test_relay_error_abort \ | |
| 353 | $OBJ/test_relay_error_abort.o $OBJ/noise_ik.o $OBJ/noise_handshake.o \ | |
| 354 | $OBJ/noise_stream.o $OBJ/dht_rpc.o $OBJ/holepuncher.o $OBJ/hyperdht.o \ | |
| 355 | $OBJ/dht_connector.o \ | |
| 356 | $UDX_OBJS $LDFLAGS | |
| 357 | echo " OK: test_relay_error_abort" | |
| 358 | ||
| 347 | 359 | echo "=== All test binaries built successfully ===" |
| … | … | @@ -0,0 +1,146 @@ |
| 1 | // Gate: an unreadable holepunch reply fails the attempt NOW, not in 30 seconds. | |
| 2 | // | |
| 3 | // docs/PARITY-hyperswarm.md §11.1. JS wires the relay client's error straight | |
| 4 | // to onabort (connect.js:814), which destroys the session immediately. Our three | |
| 5 | // unreadable-reply branches logged and returned, so the attempt sat in | |
| 6 | // HOLEPUNCHING until DHT_CONNECT_TIMEOUT_MS — half a minute of an outbound slot | |
| 7 | // spent on an answer we had already received and could not use. With | |
| 8 | // SWARM_MAX_PARALLEL = 3 that is a third of our outbound capacity per dead peer. | |
| 9 | // | |
| 10 | // ⛔ IT ALSO GATES THE GUARD. The id in a holepunch tag is reusable, so a late | |
| 11 | // reply must NOT abort whatever holds that id now. That guard existed before | |
| 12 | // this change and had no test — and this repo has been bitten twice by a stale | |
| 13 | // gate hiding an untested guard, where deleting the guard looks like a fix | |
| 14 | // (§8.3, §8.4). The negative cases below are half of this file on purpose. | |
| 15 | ||
| 16 | #include <cstdio> | |
| 17 | #include <cstring> | |
| 18 | #include <vector> | |
| 19 | ||
| 20 | #include <sodium.h> | |
| 21 | ||
| 22 | #include "dht_connector.h" | |
| 23 | #include "hyperdht.h" | |
| 24 | ||
| 25 | extern "C" { | |
| 26 | #include <udx.h> | |
| 27 | #include <uv.h> | |
| 28 | } | |
| 29 | ||
| 30 | static int checks = 0, failures = 0; | |
| 31 | static void check(bool cond, const char *what) { | |
| 32 | checks++; | |
| 33 | if (cond) { printf(" PASS: %s\n", what); return; } | |
| 34 | failures++; | |
| 35 | printf(" FAIL: %s\n", what); | |
| 36 | } | |
| 37 | ||
| 38 | int main() { | |
| 39 | if (sodium_init() < 0) { printf("sodium_init failed\n"); return 1; } | |
| 40 | ||
| 41 | uv_loop_t loop; | |
| 42 | uv_loop_init(&loop); | |
| 43 | udx_t udx; | |
| 44 | udx_init(&loop, &udx, nullptr); | |
| 45 | udx_socket_t sock; | |
| 46 | udx_socket_init(&udx, &sock, nullptr); | |
| 47 | struct sockaddr_in addr; | |
| 48 | uv_ip4_addr("127.0.0.1", 0, &addr); | |
| 49 | udx_socket_bind(&sock, (struct sockaddr *)&addr, 0); | |
| 50 | ||
| 51 | DhtRpc rpc; | |
| 52 | rpc.init(&sock, &loop); | |
| 53 | Ed25519KeyPair kp; | |
| 54 | crypto_sign_keypair(kp.public_key, kp.secret_key); | |
| 55 | HyperDht dht; | |
| 56 | dht.init(&rpc, kp); | |
| 57 | DhtConnector connector; | |
| 58 | connector.init(&dht, &sock, &loop); | |
| 59 | ||
| 60 | int failed_count = 0; | |
| 61 | std::string last_error; | |
| 62 | connector.on_failed = [&](uint32_t, const uint8_t *, const char *err) { | |
| 63 | failed_count++; | |
| 64 | last_error = err ? err : ""; | |
| 65 | printf(" (on_failed: %s)\n", last_error.c_str()); | |
| 66 | }; | |
| 67 | ||
| 68 | IPv4Address relay; | |
| 69 | relay.ip[0] = 127; relay.ip[1] = 0; relay.ip[2] = 0; relay.ip[3] = 1; | |
| 70 | relay.port = 49737; | |
| 71 | ||
| 72 | uint8_t remote_pk[32]; | |
| 73 | randombytes_buf(remote_pk, 32); | |
| 74 | ||
| 75 | // ---- 1. garbage, for an attempt that IS waiting on a relay ---- | |
| 76 | { | |
| 77 | const uint32_t id = connector.connect(remote_pk); | |
| 78 | check(id != 0, "connect() started an attempt"); | |
| 79 | check(connector.set_state_for_gate(id, DHT_CONN_HOLEPUNCHING), | |
| 80 | "the attempt is waiting on its relay"); | |
| 81 | ||
| 82 | const uint8_t garbage[8] = { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff }; | |
| 83 | connector.on_holepunch_response(0x10000 | id, garbage, sizeof(garbage), relay); | |
| 84 | ||
| 85 | check(failed_count == 1, "an unreadable reply fails the attempt immediately"); | |
| 86 | check(connector.find_connection(id) == nullptr, "and the slot is given back"); | |
| 87 | ||
| 88 | // It must not have needed the wall clock, and must not fire twice. | |
| 89 | connector.tick(DHT_CONNECT_TIMEOUT_MS + 1000); | |
| 90 | check(failed_count == 1, "no second failure when the old timeout passes"); | |
| 91 | } | |
| 92 | ||
| 93 | // ---- 2. an EMPTY reply is not an error ---- | |
| 94 | // | |
| 95 | // "The relay could not reach the peer" is what JS covers with its 15s | |
| 96 | // relayTimeout, not with onabort. Failing here would be MORE aggressive than | |
| 97 | // the reference, which is its own kind of divergence. | |
| 98 | { | |
| 99 | failed_count = 0; | |
| 100 | randombytes_buf(remote_pk, 32); | |
| 101 | const uint32_t id = connector.connect(remote_pk); | |
| 102 | connector.set_state_for_gate(id, DHT_CONN_HOLEPUNCHING); | |
| 103 | ||
| 104 | connector.on_holepunch_response(0x10000 | id, nullptr, 0, relay); | |
| 105 | check(failed_count == 0, "an EMPTY reply does not abort — that is the relay's timeout, not an error"); | |
| 106 | check(connector.find_connection(id) != nullptr, "and the attempt is still alive"); | |
| 107 | } | |
| 108 | ||
| 109 | // ---- 3. the guard: a reply for an attempt that is NOT waiting on a relay ---- | |
| 110 | { | |
| 111 | failed_count = 0; | |
| 112 | randombytes_buf(remote_pk, 32); | |
| 113 | const uint32_t id = connector.connect(remote_pk); // still FINDING | |
| 114 | const uint8_t garbage[8] = { 1, 2, 3, 4, 5, 6, 7, 8 }; | |
| 115 | ||
| 116 | connector.on_holepunch_response(0x10000 | id, garbage, sizeof(garbage), relay); | |
| 117 | check(failed_count == 0, | |
| 118 | "a holepunch reply for an attempt still in FINDING is IGNORED"); | |
| 119 | check(connector.find_connection(id) != nullptr, | |
| 120 | "and that attempt is untouched — ids are reusable, so a late reply " | |
| 121 | "must not kill whoever holds one now"); | |
| 122 | } | |
| 123 | ||
| 124 | // ---- 4. the guard again: an id nobody holds ---- | |
| 125 | { | |
| 126 | failed_count = 0; | |
| 127 | const uint8_t garbage[8] = { 9, 9, 9, 9, 9, 9, 9, 9 }; | |
| 128 | connector.on_holepunch_response(0x10000 | 0xBEEF, garbage, sizeof(garbage), relay); | |
| 129 | check(failed_count == 0, "a reply for an unknown id fails nothing"); | |
| 130 | } | |
| 131 | ||
| 132 | // ---- 5. a tag that is not a holepunch tag at all ---- | |
| 133 | { | |
| 134 | failed_count = 0; | |
| 135 | const uint8_t garbage[4] = { 0, 0, 0, 0 }; | |
| 136 | check(!connector.on_holepunch_response(0x20000, garbage, sizeof(garbage), relay), | |
| 137 | "a tag outside the holepunch range is not claimed"); | |
| 138 | check(failed_count == 0, "and fails nothing"); | |
| 139 | } | |
| 140 | ||
| 141 | printf("\n%d/%d passed\n", checks - failures, checks); | |
| 142 | udx_socket_close(&sock); | |
| 143 | uv_run(&loop, UV_RUN_NOWAIT); | |
| 144 | uv_loop_close(&loop); | |
| 145 | return failures ? 1 : 0; | |
| 146 | } |