Make clients wait to refresh bridges when they have a recent descriptor

But when clients are just starting, make them try each bridge a few times
before giving up on it.

These changes make the bridge download schedules more explicit: before
17750, they relied on undocumented behaviour and specific schedule
entries. (And between 17750 and this fix, they were broken.)

Fixes 23347, not in any released version of tor.

changes/bug23347

@@ -1,6 +1,9 @@
-  o Minor bug fixes (bridge client bootstrap):
-    - Make bridge clients download bridge descriptors immediately.
-      The bridge schedules were never correct, but we didn't realise until
-      the fix for 17750 made bridge clients hang for 15 minutes before
-      bootstrapping.
-      Fixes bug 23347. Not in any released version of Tor.
+  o Minor fixes (bridge client bootstrap):
+    - Make bridge clients with no running bridges try to download
+      bridge descriptors immediately. But when bridge clients have
+      running bridges, make them wait at least 3 hours before
+      refreshing recently received bridge descriptors.
+      Download schedules used to start with an implicit 0, but the
+      fix for 17750 changed this undocumented behaviour, and made
+      bridge clients hang for 15 minutes before bootstrapping.
+      Fixes bug 23347, not in any released version of Tor.

doc/tor.1.txt

@@ -2475,7 +2475,8 @@ The following options are used for running a testing Tor network.
        TestingClientDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60
        TestingServerConsensusDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60
        TestingClientConsensusDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60
-       TestingBridgeDownloadSchedule 0, 5, 10, 30, 60
+       TestingBridgeDownloadSchedule 10, 30, 60
+       TestingBridgeBootstrapDownloadSchedule 0, 0, 5, 10, 15, 20, 30, 60
        TestingClientMaxIntervalWithoutRequest 5 seconds
        TestingDirConnectionMaxStall 30 seconds
        TestingConsensusMaxDownloadTries 80
@@ -2540,9 +2541,16 @@ The following options are used for running a testing Tor network.
     1800, 3600, 3600, 3600, 10800, 21600, 43200)
 
 [[TestingBridgeDownloadSchedule]] **TestingBridgeDownloadSchedule** __N__,__N__,__...__::
-    Schedule for when clients should download bridge descriptors. Changing this
-    requires that **TestingTorNetwork** is set. (Default: 0, 8, 3600, 10800,
-    25200, 54000, 111600, 262800)
+    Schedule for when clients should download each bridge descriptor when they
+    know that one or more of their configured bridges are running. Changing
+    this requires that **TestingTorNetwork** is set. (Default: 10800, 25200,
+    54000, 111600, 262800)
+
+[[TestingBridgeBootstrapDownloadSchedule]] **TestingBridgeBootstrapDownloadSchedule** __N__,__N__,__...__::
+    Schedule for when clients should download each bridge descriptor when they
+    have just started, or when they can not contact any of their bridges.
+    Changing this requires that **TestingTorNetwork** is set. (Default: 0, 30,
+    90, 600, 3600, 10800, 25200, 54000, 111600, 262800)
 
 [[TestingClientMaxIntervalWithoutRequest]] **TestingClientMaxIntervalWithoutRequest** __N__ **seconds**|**minutes**::
     When directory clients have only a few descriptors to request, they batch

src/or/bridges.c

@@ -794,17 +794,10 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
       node_t *node;
       /* it's here; schedule its re-fetch for a long time from now. */
       if (!from_cache) {
+        /* This schedules the re-fetch at a constant interval, which produces
+         * a pattern of bridge traffic. But it's better than trying all
+         * configured briges several times in the first few minutes. */
         download_status_reset(&bridge->fetch_status);
-        /* We have two quick attempts in the bridge schedule, and then slow
-         * ones */
-        download_status_increment_attempt(
-                        &bridge->fetch_status,
-                        safe_str_client(fmt_and_decorate_addr(&bridge->addr)),
-                        now);
-        download_status_increment_attempt(
-                        &bridge->fetch_status,
-                        safe_str_client(fmt_and_decorate_addr(&bridge->addr)),
-                        now);
       }
 
       node = node_get_mutable_by_id(ri->cache_info.identity_digest);
@@ -837,8 +830,8 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
  * We use this function to decide if we're ready to start building
  * circuits through our bridges, or if we need to wait until the
  * directory "server/authority" requests finish. */
-int
-any_bridge_descriptors_known(void)
+MOCK_IMPL(int,
+any_bridge_descriptors_known, (void))
 {
   tor_assert(get_options()->UseBridges);
 

src/or/bridges.h

@@ -45,7 +45,7 @@ void bridge_add_from_config(struct bridge_line_t *bridge_line);
 void retry_bridge_descriptor_fetch_directly(const char *digest);
 void fetch_bridge_descriptors(const or_options_t *options, time_t now);
 void learned_bridge_descriptor(routerinfo_t *ri, int from_cache);
-int any_bridge_descriptors_known(void);
+MOCK_DECL(int, any_bridge_descriptors_known, (void));
 const smartlist_t *get_socks_args_by_bridge_addrport(const tor_addr_t *addr,
                                                      uint16_t port);
 

src/or/config.c

@@ -586,10 +586,16 @@ static config_var_t option_vars_[] = {
    * blackholed. Clients will try 3 directories simultaneously.
    * (Relays never use simultaneous connections.) */
   V(ClientBootstrapConsensusMaxInProgressTries, UINT, "3"),
-  /* The bridge code relies on the third item in this schedule being slow
-   * (~ 1 consensus interval) */
+  /* When a client has any running bridges, check each bridge occasionally,
+    * whether or not that bridge is actually up. */
   V(TestingBridgeDownloadSchedule, CSV_INTERVAL,
-    "0, 8, 3600, 10800, 25200, 54000, 111600, 262800"),
+    "10800, 25200, 54000, 111600, 262800"),
+  /* When a client is just starting, or has no running bridges, check each
+   * bridge a few times quickly, and then try again later. These schedules
+   * are much longer than the other schedules, because we try each and every
+   * configured bridge with this schedule. */
+  V(TestingBridgeBootstrapDownloadSchedule, CSV_INTERVAL,
+    "0, 30, 90, 600, 3600, 10800, 25200, 54000, 111600, 262800"),
   V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "10 minutes"),
   V(TestingDirConnectionMaxStall, INTERVAL, "5 minutes"),
   V(TestingConsensusMaxDownloadTries, UINT, "8"),
@@ -651,9 +657,9 @@ static const config_var_t testing_tor_network_defaults[] = {
                                  "15, 20, 30, 60"),
   V(TestingClientConsensusDownloadSchedule, CSV_INTERVAL, "0, 0, 5, 10, "
                                  "15, 20, 30, 60"),
-  /* The bridge code relies on the third item in this schedule being slow
-   * (~ 1 consensus interval) */
-  V(TestingBridgeDownloadSchedule, CSV_INTERVAL, "0, 5, 10, 30, 60"),
+  V(TestingBridgeDownloadSchedule, CSV_INTERVAL, "10, 30, 60"),
+  V(TestingBridgeBootstrapDownloadSchedule, CSV_INTERVAL, "0, 0, 5, 10, "
+                                 "15, 20, 30, 60"),
   V(TestingClientMaxIntervalWithoutRequest, INTERVAL, "5 seconds"),
   V(TestingDirConnectionMaxStall, INTERVAL, "30 seconds"),
   V(TestingConsensusMaxDownloadTries, UINT, "80"),
@@ -4066,6 +4072,7 @@ options_validate(or_options_t *old_options, or_options_t *options,
   CHECK_DEFAULT(TestingServerConsensusDownloadSchedule);
   CHECK_DEFAULT(TestingClientConsensusDownloadSchedule);
   CHECK_DEFAULT(TestingBridgeDownloadSchedule);
+  CHECK_DEFAULT(TestingBridgeBootstrapDownloadSchedule);
   CHECK_DEFAULT(TestingClientMaxIntervalWithoutRequest);
   CHECK_DEFAULT(TestingDirConnectionMaxStall);
   CHECK_DEFAULT(TestingConsensusMaxDownloadTries);

src/or/directory.c

@@ -5342,7 +5342,14 @@ find_dl_schedule(const download_status_t *dls, const or_options_t *options)
         }
       }
     case DL_SCHED_BRIDGE:
-      return options->TestingBridgeDownloadSchedule;
+      /* A bridge client downloading bridge descriptors */
+      if (any_bridge_descriptors_known()) {
+        /* A bridge client with one or more running bridges */
+        return options->TestingBridgeDownloadSchedule;
+      } else {
+        /* A bridge client with no running bridges */
+        return options->TestingBridgeBootstrapDownloadSchedule;
+      }
     default:
       tor_assert(0);
   }

src/or/or.h

@@ -4314,6 +4314,10 @@ typedef struct {
    * altered on testing networks. */
   smartlist_t *TestingBridgeDownloadSchedule;
 
+  /** Schedule for when clients should download bridge descriptors when they
+   * have no running bridges.  Only altered on testing networks. */
+  smartlist_t *TestingBridgeBootstrapDownloadSchedule;
+
   /** When directory clients have only a few descriptors to request, they
    * batch them until they have more, or until this amount of time has
    * passed.  Only altered on testing networks. */

src/test/test_dir.c

@@ -18,6 +18,7 @@
 #define RELAY_PRIVATE
 
 #include "or.h"
+#include "bridges.h"
 #include "confparse.h"
 #include "config.h"
 #include "control.h"
@@ -4245,7 +4246,7 @@ test_dir_download_status_increment(void *arg)
   /* Put it in the options */
   mock_options = &test_options;
   reset_options(mock_options, &mock_get_options_calls);
-  mock_options->TestingBridgeDownloadSchedule = schedule;
+  mock_options->TestingBridgeBootstrapDownloadSchedule = schedule;
   mock_options->TestingClientDownloadSchedule = schedule;
 
   MOCK(get_options, mock_get_options);
@@ -4425,6 +4426,7 @@ test_dir_download_status_increment(void *arg)
 
   /* Check that failure increments do happen on attempt-based schedules,
    * but that the retry is set at the end of time */
+  mock_options->UseBridges = 1;
   mock_get_options_calls = 0;
   next_at = download_status_increment_failure(&dls_attempt, 404, "test", 0,
                                               current_time);
@@ -4539,6 +4541,7 @@ test_dir_download_status_increment(void *arg)
   tt_int_op(download_status_get_n_failures(&dls_attempt), OP_EQ, 0);
   tt_int_op(download_status_get_n_attempts(&dls_attempt), OP_EQ, 0);
   tt_int_op(mock_get_options_calls, OP_GE, 1);
+  mock_options->UseBridges = 0;
 
   /* Check that attempt increments don't happen on failure-based schedules,
    * and that the attempt is set at the end of time */
@@ -5859,9 +5862,17 @@ mock_networkstatus_consensus_can_use_extra_fallbacks(
   return mock_networkstatus_consensus_can_use_extra_fallbacks_value;
 }
 
-/* data is a 2 character nul-terminated string.
+static int mock_any_bridge_descriptors_known_value = 0;
+static int
+mock_any_bridge_descriptors_known(void)
+{
+  return mock_any_bridge_descriptors_known_value;
+}
+
+/* data is a 3 character nul-terminated string.
  * If data[0] is 'b', set bootstrapping, anything else means not bootstrapping
  * If data[1] is 'f', set extra fallbacks, anything else means no extra
+ * If data[2] is 'f', set running bridges, anything else means no extra
  * fallbacks.
  */
 static void
@@ -5869,7 +5880,7 @@ test_dir_find_dl_schedule(void* data)
 {
   const char *str = (const char *)data;
 
-  tt_assert(strlen(data) == 2);
+  tt_assert(strlen(data) == 3);
 
   if (str[0] == 'b') {
     mock_networkstatus_consensus_is_bootstrapping_value = 1;
@@ -5883,15 +5894,23 @@ test_dir_find_dl_schedule(void* data)
     mock_networkstatus_consensus_can_use_extra_fallbacks_value = 0;
   }
 
+  if (str[2] == 'r') {
+    mock_any_bridge_descriptors_known_value = 1;
+  } else {
+    mock_any_bridge_descriptors_known_value = 0;
+  }
+
   MOCK(networkstatus_consensus_is_bootstrapping,
        mock_networkstatus_consensus_is_bootstrapping);
   MOCK(networkstatus_consensus_can_use_extra_fallbacks,
        mock_networkstatus_consensus_can_use_extra_fallbacks);
+  MOCK(any_bridge_descriptors_known,
+       mock_any_bridge_descriptors_known);
 
   download_status_t dls;
   smartlist_t server, client, server_cons, client_cons;
   smartlist_t client_boot_auth_only_cons, client_boot_auth_cons;
-  smartlist_t client_boot_fallback_cons, bridge;
+  smartlist_t client_boot_fallback_cons, bridge, bridge_bootstrap;
 
   mock_options = tor_malloc(sizeof(or_options_t));
   reset_options(mock_options, &mock_get_options_calls);
@@ -5908,6 +5927,7 @@ test_dir_find_dl_schedule(void* data)
   mock_options->ClientBootstrapConsensusFallbackDownloadSchedule =
     &client_boot_fallback_cons;
   mock_options->TestingBridgeDownloadSchedule = &bridge;
+  mock_options->TestingBridgeBootstrapDownloadSchedule = &bridge_bootstrap;
 
   dls.schedule = DL_SCHED_GENERIC;
   /* client */
@@ -5996,11 +6016,17 @@ test_dir_find_dl_schedule(void* data)
   dls.schedule = DL_SCHED_BRIDGE;
   /* client */
   mock_options->ClientOnly = 1;
-  tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge);
+  mock_options->UseBridges = 1;
+  if (any_bridge_descriptors_known()) {
+    tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge);
+  } else {
+    tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge_bootstrap);
+  }
 
  done:
   UNMOCK(networkstatus_consensus_is_bootstrapping);
   UNMOCK(networkstatus_consensus_can_use_extra_fallbacks);
+  UNMOCK(any_bridge_descriptors_known);
   UNMOCK(get_options);
   tor_free(mock_options);
   mock_options = NULL;
@@ -6165,10 +6191,14 @@ struct testcase_t dir_tests[] = {
   DIR(dump_unparseable_descriptors, 0),
   DIR(populate_dump_desc_fifo, 0),
   DIR(populate_dump_desc_fifo_2, 0),
-  DIR_ARG(find_dl_schedule, TT_FORK, "bf"),
-  DIR_ARG(find_dl_schedule, TT_FORK, "ba"),
-  DIR_ARG(find_dl_schedule, TT_FORK, "cf"),
-  DIR_ARG(find_dl_schedule, TT_FORK, "ca"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "bfd"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "bad"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "cfd"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "cad"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "bfr"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "bar"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "cfr"),
+  DIR_ARG(find_dl_schedule, TT_FORK, "car"),
   DIR(assumed_flags, 0),
   DIR(networkstatus_compute_bw_weights_v10, 0),
   END_OF_TESTCASES

src/test/test_options.c

@@ -2230,6 +2230,7 @@ test_options_validate__testing(void *ignored)
   ENSURE_DEFAULT(TestingServerConsensusDownloadSchedule, 3000);
   ENSURE_DEFAULT(TestingClientConsensusDownloadSchedule, 3000);
   ENSURE_DEFAULT(TestingBridgeDownloadSchedule, 3000);
+  ENSURE_DEFAULT(TestingBridgeBootstrapDownloadSchedule, 3000);
   ENSURE_DEFAULT(TestingClientMaxIntervalWithoutRequest, 3000);
   ENSURE_DEFAULT(TestingDirConnectionMaxStall, 3000);
   ENSURE_DEFAULT(TestingConsensusMaxDownloadTries, 3000);