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.

7 files changed, 39 insertions, 18 deletions

diff --git a/changes/bug24367 b/changes/bug24367
index 6081f7b5d4..09ef3bb877 100644
--- a/changes/bug24367
+++ b/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.
diff --git a/src/or/bridges.c b/src/or/bridges.c
index ac759493cc..0b1bbbd158 100644
--- a/src/or/bridges.c
+++ b/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);
       }
diff --git a/src/or/directory.c b/src/or/directory.c
index e49fd595c9..f14bdde90b 100644
--- a/src/or/directory.c
+++ b/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:
diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c
index 100286f103..90b728726d 100644
--- a/src/or/entrynodes.c
+++ b/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);
diff --git a/src/or/entrynodes.h b/src/or/entrynodes.h
index ad6b47936f..d909115b1f 100644
--- a/src/or/entrynodes.h
+++ b/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
 /**
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 2075fccc92..e0a3e4cdc6 100644
--- a/src/or/networkstatus.c
+++ b/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";
       }
diff --git a/src/test/test_dir.c b/src/test/test_dir.c
index 0b34e8b339..997b110d62 100644
--- a/src/test/test_dir.c
+++ b/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);