cleanup: Count re-adding an existing bootstrap node as success.

auto_tests/save_load_test.c

@@ -27,7 +27,7 @@
 #ifdef TCP_RELAY_PORT
 #undef TCP_RELAY_PORT
 #endif
-#define TCP_RELAY_PORT 33430
+#define TCP_RELAY_PORT 33431
 
 static void accept_friend_request(Tox *m, const uint8_t *public_key, const uint8_t *data, size_t length, void *userdata)
 {

toxcore/onion_client.c

@@ -150,8 +150,16 @@ Net_Crypto *onion_get_net_crypto(const Onion_Client *onion_c)
 
 /** @brief Add a node to the path_nodes bootstrap array.
  *
- * return false on failure
- * return true on success
+ * If a node with the given public key was already in the bootstrap array, this function has no
+ * effect and returns successfully. There is currently no way to update the IP/port for a bootstrap
+ * node, so if it changes, the Onion_Client must be recreated.
+ *
+ * @param onion_c The onion client object.
+ * @param ip_port IP/port for the bootstrap node.
+ * @param public_key DHT public key for the bootstrap node.
+ *
+ * @retval false on failure
+ * @retval true on success
  */
 bool onion_add_bs_path_node(Onion_Client *onion_c, const IP_Port *ip_port, const uint8_t *public_key)
 {
@@ -161,7 +169,7 @@ bool onion_add_bs_path_node(Onion_Client *onion_c, const IP_Port *ip_port, const
 
     for (unsigned int i = 0; i < MAX_PATH_NODES; ++i) {
         if (pk_equal(public_key, onion_c->path_nodes_bs[i].public_key)) {
-            return false;
+            return true;
         }
     }
 

toxcore/onion_client.h

@@ -64,8 +64,16 @@ Net_Crypto *onion_get_net_crypto(const Onion_Client *onion_c);
 
 /** @brief Add a node to the path_nodes bootstrap array.
  *
- * return false on failure
- * return true on success
+ * If a node with the given public key was already in the bootstrap array, this function has no
+ * effect and returns successfully. There is currently no way to update the IP/port for a bootstrap
+ * node, so if it changes, the Onion_Client must be recreated.
+ *
+ * @param onion_c The onion client object.
+ * @param ip_port IP/port for the bootstrap node.
+ * @param public_key DHT public key for the bootstrap node.
+ *
+ * @retval false on failure
+ * @retval true on success
  */
 non_null()
 bool onion_add_bs_path_node(Onion_Client *onion_c, const IP_Port *ip_port, const uint8_t *public_key);

toxcore/tox.c

@@ -764,34 +764,49 @@ void tox_get_savedata(const Tox *tox, uint8_t *savedata)
     unlock(tox);
 }
 
-bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, Tox_Err_Bootstrap *error)
+non_null(5) nullable(1, 2, 4, 6)
+static int32_t resolve_bootstrap_node(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, IP_Port **root, Tox_Err_Bootstrap *error)
 {
     assert(tox != nullptr);
+    assert(root != nullptr);
 
     if (host == nullptr || public_key == nullptr) {
         SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_NULL);
-        return false;
+        return -1;
     }
 
     if (port == 0) {
         SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_PORT);
-        return false;
+        return -1;
     }
 
-    IP_Port *root;
-
-    const int32_t count = net_getipport(host, &root, TOX_SOCK_DGRAM);
+    const int32_t count = net_getipport(host, root, TOX_SOCK_DGRAM);
 
     if (count == -1) {
         LOGGER_DEBUG(tox->m->log, "could not resolve bootstrap node '%s'", host);
-        net_freeipport(root);
+        net_freeipport(*root);
         SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST);
+        return -1;
+    }
+
+    assert(*root != nullptr);
+    return count;
+}
+
+bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key, Tox_Err_Bootstrap *error)
+{
+    IP_Port *root;
+    const int32_t count = resolve_bootstrap_node(tox, host, port, public_key, &root, error);
+
+    if (count == -1) {
         return false;
     }
 
     lock(tox);
     assert(count >= 0);
-    bool success = false;
+    bool onion_success = false;
+    // UDP bootstrap is default success if it's disabled (because we don't even try).
+    bool udp_success = tox->m->options.udp_disabled;
 
     for (int32_t i = 0; i < count; ++i) {
         root[i].port = net_htons(port);
@@ -800,13 +815,13 @@ bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *pub
             // If UDP is enabled, the caller cares about whether any of the
             // bootstrap calls below will succeed. In TCP-only mode, adding
             // onion path nodes successfully is sufficient.
-            success = success || tox->m->options.udp_disabled;
+            onion_success = true;
         }
 
         if (!tox->m->options.udp_disabled) {
             if (dht_bootstrap(tox->m->dht, &root[i], public_key)) {
                 // If any of the bootstrap calls worked, we call it success.
-                success = true;
+                udp_success = true;
             }
         }
     }
@@ -815,45 +830,34 @@ bool tox_bootstrap(Tox *tox, const char *host, uint16_t port, const uint8_t *pub
 
     net_freeipport(root);
 
-    if (count > 0 && success) {
-        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK);
-        return true;
+    if (count == 0 || !onion_success || !udp_success) {
+        LOGGER_DEBUG(tox->m->log, "bootstrap node '%s' resolved to %d IP_Ports%s (onion: %s, UDP: %s)",
+                     host, count,
+                     count > 0 ? ", but failed to bootstrap with any of them" : "",
+                     onion_success ? "success" : "FAILURE",
+                     tox->m->options.udp_disabled ? "disabled" : (udp_success ? "success" : "FAILURE"));
+        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST);
+        return false;
     }
 
-    LOGGER_DEBUG(tox->m->log, "bootstrap node '%s' resolved to %d IP_Ports%s", host, count,
-            count > 0 ? ", but failed to bootstrap with any of them" : "");
-    SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST);
-    return false;
+    SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK);
+    return true;
 }
 
 bool tox_add_tcp_relay(Tox *tox, const char *host, uint16_t port, const uint8_t *public_key,
                        Tox_Err_Bootstrap *error)
 {
-    assert(tox != nullptr);
-
-    if (host == nullptr || public_key == nullptr) {
-        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_NULL);
-        return false;
-    }
-
-    if (port == 0) {
-        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_PORT);
-        return false;
-    }
-
     IP_Port *root;
-
-    const int32_t count = net_getipport(host, &root, TOX_SOCK_STREAM);
+    const int32_t count = resolve_bootstrap_node(tox, host, port, public_key, &root, error);
 
     if (count == -1) {
-        net_freeipport(root);
-        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST);
         return false;
     }
 
     lock(tox);
     assert(count >= 0);
 
+    LOGGER_DEBUG(tox->m->log, "count: %d", count);
     for (int32_t i = 0; i < count; ++i) {
         root[i].port = net_htons(port);
 
@@ -864,13 +868,13 @@ bool tox_add_tcp_relay(Tox *tox, const char *host, uint16_t port, const uint8_t
 
     net_freeipport(root);
 
-    if (count > 0) {
-        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK);
-        return true;
+    if (count == 0) {
+        SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST);
+        return false;
     }
 
-    SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_BAD_HOST);
-    return false;
+    SET_ERROR_PARAMETER(error, TOX_ERR_BOOTSTRAP_OK);
+    return true;
 }
 
 Tox_Connection tox_self_get_connection_status(const Tox *tox)

toxcore/tox_test.cc

@@ -17,6 +17,22 @@ static void set_random_name_and_status_message(Tox *tox, uint8_t *name, uint8_t
     }
 }
 
+TEST(Tox, BootstrapErrorCodes)
+{
+    Tox *tox = tox_new(nullptr, nullptr);
+    ASSERT_NE(tox, nullptr);
+
+    Tox_Err_Bootstrap err;
+    std::array<uint8_t, TOX_PUBLIC_KEY_SIZE> pk;
+    tox_bootstrap(tox, "127.0.0.1", 0, pk.data(), &err);
+    EXPECT_EQ(err, TOX_ERR_BOOTSTRAP_BAD_PORT);
+
+    tox_bootstrap(tox, nullptr, 33445, pk.data(), &err);
+    EXPECT_EQ(err, TOX_ERR_BOOTSTRAP_NULL);
+
+    tox_kill(tox);
+}
+
 TEST(Tox, OneTest)
 {
     struct Tox_Options *options = tox_options_new(nullptr);