Make sure bridges are definitely running before delaying directory fetches

Retry directory downloads when we get our first bridge descriptor
during bootstrap or while reconnecting to the network. Keep retrying
every time we get a bridge descriptor, until we have a reachable bridge.

Stop delaying bridge descriptor fetches when we have cached bridge
descriptors. Instead, only delay bridge descriptor fetches when we
have at least one reachable bridge.

Fixes bug 24367; bugfix on 0.2.0.3-alpha.

changes/bug24367

@@ -1,7 +1,13 @@
   o Minor bugfixes (bridge clients, bootstrap):
-    - Stop checking for bridge descriptors when we actually want to know if
-      any bridges are usable. This avoids potential bootstrapping issues.
+    - Retry directory downloads when we get our first bridge descriptor
+      during bootstrap or while reconnecting to the network. Keep retrying
+      every time we get a bridge descriptor, until we have a reachable bridge.
+      Fixes bug 24367; bugfix on 0.2.0.3-alpha.
+    - Stop delaying bridge descriptor fetches when we have cached bridge
+      descriptors. Instead, only delay bridge descriptor fetches when we
+      have at least one reachable bridge.
+      Fixes bug 24367; bugfix on 0.2.0.3-alpha.
+    - Stop delaying directory fetches when we have cached bridge descriptors.
+      Instead, only delay bridge descriptor fetches when all our bridges are
+      definitely unreachable.
       Fixes bug 24367; bugfix on 0.2.0.3-alpha.
-    - Stop stalling when bridges are changed at runtime. Stop stalling when
-      old bridge descriptors are cached, but they are not in use.
-      Fixes bug 24367; bugfix on 23347 in 0.3.2.1-alpha.

src/or/bridges.c

@@ -796,7 +796,11 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
   tor_assert(ri);
   tor_assert(ri->purpose == ROUTER_PURPOSE_BRIDGE);
   if (get_options()->UseBridges) {
-    int first = num_bridges_usable() <= 1;
+    /* Retry directory downloads whenever we get a bridge descriptor:
+     * - when bootstrapping, and
+     * - when we aren't sure if any of our bridges are reachable.
+     * Keep on retrying until we have at least one reachable bridge. */
+    int first = num_bridges_usable(0) < 1;
     bridge_info_t *bridge = get_configured_bridge_by_routerinfo(ri);
     time_t now = time(NULL);
     router_set_status(ri->cache_info.identity_digest, 1);
@@ -826,8 +830,8 @@ learned_bridge_descriptor(routerinfo_t *ri, int from_cache)
 
       log_notice(LD_DIR, "new bridge descriptor '%s' (%s): %s", ri->nickname,
                  from_cache ? "cached" : "fresh", router_describe(ri));
-      /* set entry->made_contact so if it goes down we don't drop it from
-       * our entry node list */
+      /* If we didn't have a reachable bridge before this one, try directory
+       * documents again. */
       if (first) {
         routerlist_retry_directory_downloads(now);
       }

src/or/directory.c

@@ -5337,12 +5337,13 @@ find_dl_schedule(const download_status_t *dls, const or_options_t *options)
         }
       }
     case DL_SCHED_BRIDGE:
-      /* A bridge client downloading bridge descriptors */
-      if (options->UseBridges && num_bridges_usable() > 0) {
-        /* A bridge client with one or more running bridges */
+      if (options->UseBridges && num_bridges_usable(0) > 0) {
+        /* A bridge client that is sure that one or more of its bridges are
+         * running can afford to wait longer to update bridge descriptors. */
         return options->TestingBridgeDownloadSchedule;
       } else {
-        /* A bridge client with no running bridges */
+        /* A bridge client which might have no running bridges, must try to
+         * get bridge descriptors straight away. */
         return options->TestingBridgeBootstrapDownloadSchedule;
       }
     default:

src/or/entrynodes.c

@@ -3135,13 +3135,16 @@ entry_list_is_constrained(const or_options_t *options)
 }
 
 /** Return the number of bridges that have descriptors that are marked with
- * purpose 'bridge' and are running.
+ * purpose 'bridge' and are running. If use_maybe_reachable is
+ * true, include bridges that might be reachable in the count.
+ * Otherwise, if it is false, only include bridges that have recently been
+ * found running in the count.
  *
  * 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. */
 MOCK_IMPL(int,
-num_bridges_usable,(void))
+num_bridges_usable,(int use_maybe_reachable))
 {
   int n_options = 0;
 
@@ -3154,8 +3157,12 @@ num_bridges_usable,(void))
   }
 
   SMARTLIST_FOREACH_BEGIN(gs->sampled_entry_guards, entry_guard_t *, guard) {
+    /* Definitely not usable */
     if (guard->is_reachable == GUARD_REACHABLE_NO)
       continue;
+    /* If we want to be really sure the bridges will work, skip maybes */
+    if (!use_maybe_reachable && guard->is_reachable == GUARD_REACHABLE_MAYBE)
+      continue;
     if (tor_digest_is_zero(guard->identity))
       continue;
     const node_t *node = node_get_by_id(guard->identity);

src/or/entrynodes.h

@@ -383,7 +383,7 @@ void entry_guards_note_internet_connectivity(guard_selection_t *gs);
 
 int update_guard_selection_choice(const or_options_t *options);
 
-MOCK_DECL(int,num_bridges_usable,(void));
+MOCK_DECL(int,num_bridges_usable,(int use_maybe_reachable));
 
 #ifdef ENTRYNODES_PRIVATE
 /**

src/or/networkstatus.c

@@ -1209,7 +1209,9 @@ should_delay_dir_fetches(const or_options_t *options, const char **msg_out)
   }
 
   if (options->UseBridges) {
-    if (num_bridges_usable() == 0) {
+    /* If we know that none of our bridges can possibly work, avoid fetching
+     * directory documents. But if some of them might work, try again. */
+    if (num_bridges_usable(1) == 0) {
       if (msg_out) {
         *msg_out = "No running bridges";
       }

src/test/test_dir.c

@@ -5877,8 +5877,9 @@ mock_networkstatus_consensus_can_use_extra_fallbacks(
 
 static int mock_num_bridges_usable_value = 0;
 static int
-mock_num_bridges_usable(void)
+mock_num_bridges_usable(int use_maybe_reachable)
 {
+  (void)use_maybe_reachable;
   return mock_num_bridges_usable_value;
 }
 
@@ -6031,7 +6032,7 @@ test_dir_find_dl_schedule(void* data)
   /* client */
   mock_options->ClientOnly = 1;
   mock_options->UseBridges = 1;
-  if (num_bridges_usable() > 0) {
+  if (num_bridges_usable(0) > 0) {
     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);