Merge branch 'maint-0.2.8'

changes/bug18809

@@ -0,0 +1,14 @@
+  o Major bugfixes (bootstrap):
+    - Check if bootstrap consensus downloads are still needed
+      when the linked connection attaches. This prevents tor
+      making unnecessary begindir-style connections, which are
+      the only directory connections tor clients make since
+      #18483 was merged.
+    - Fix some edge cases where consensus download connections
+      may not have been closed, even though they were not needed.
+    - Make relays retry consensus downloads the correct number of
+      times, rather than the more aggresive client retry count.
+    - Stop downloading consensuses when we have a consensus,
+      even if we don't have all the certificates for it yet.
+      Closes ticket 18943, bugfix on #4483 in 0.2.8.1-alpha,
+      patches by arma and teor.

src/or/circuituse.c

@@ -2361,6 +2361,25 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn)
     /* we're a general conn */
     origin_circuit_t *circ=NULL;
 
+    /* Are we linked to a dir conn that aims to fetch a consensus?
+     * We check here because this conn might no longer be needed. */
+    if (base_conn->linked_conn &&
+        base_conn->linked_conn->type == CONN_TYPE_DIR &&
+        base_conn->linked_conn->purpose == DIR_PURPOSE_FETCH_CONSENSUS) {
+
+      /* Yes we are. Is there a consensus fetch farther along than us? */
+      if (networkstatus_consensus_is_already_downloading(
+            TO_DIR_CONN(base_conn->linked_conn)->requested_resource)) {
+        /* We're doing the "multiple consensus fetch attempts" game from
+         * proposal 210, and we're late to the party. Just close this conn.
+         * The circuit and TLS conn that we made will time out after a while
+         * if nothing else wants to use them. */
+        log_info(LD_DIR, "Closing extra consensus fetch (to %s) since one "
+                 "is already downloading.", base_conn->linked_conn->address);
+        return -1;
+      }
+    }
+
     if (conn->chosen_exit_name) {
       const node_t *node = node_get_by_nickname(conn->chosen_exit_name, 1);
       int opt = conn->chosen_exit_optional;

src/or/connection.c

@@ -4436,32 +4436,6 @@ connection_get_by_type_state_rendquery(int type, int state,
          ));
 }
 
-#define CONN_FIRST_AND_FREE_TEMPLATE(sl)       \
-  STMT_BEGIN                                   \
-    if (smartlist_len(sl) > 0) {               \
-      void *first_item = smartlist_get(sl, 0); \
-      smartlist_free(sl);                      \
-      return first_item;                       \
-    } else {                                   \
-      smartlist_free(sl);                      \
-      return NULL;                             \
-    }                                          \
-  STMT_END
-
-/** Return a directory connection (if any one exists) that is fetching
- * the item described by <b>purpose</b>/<b>resource</b>, otherwise return NULL.
- */
-dir_connection_t *
-connection_dir_get_by_purpose_and_resource(
-                                           int purpose,
-                                           const char *resource)
-{
-  smartlist_t *conns = connection_dir_list_by_purpose_and_resource(
-                                                          purpose,
-                                                          resource);
-  CONN_FIRST_AND_FREE_TEMPLATE(conns);
-}
-
 /** Return a new smartlist of dir_connection_t * from get_connection_array()
  * that satisfy conn_test on connection_t *conn_var, and dirconn_test on
  * dir_connection_t *dirconn_var. conn_var must be of CONN_TYPE_DIR and not
@@ -4502,25 +4476,6 @@ connection_dir_list_by_purpose_and_resource(
                                          dirconn->requested_resource));
 }
 
-/** Return a directory connection (if any one exists) that is fetching
- * the item described by <b>purpose</b>/<b>resource</b>/<b>state</b>,
- * otherwise return NULL. */
-dir_connection_t *
-connection_dir_get_by_purpose_resource_and_state(
-                                                 int purpose,
-                                                 const char *resource,
-                                                 int state)
-{
-  smartlist_t *conns =
-    connection_dir_list_by_purpose_resource_and_state(
-                                                      purpose,
-                                                      resource,
-                                                      state);
-  CONN_FIRST_AND_FREE_TEMPLATE(conns);
-}
-
-#undef CONN_FIRST_AND_FREE_TEMPLATE
-
 /** Return a list of directory connections that are fetching the item
  * described by <b>purpose</b>/<b>resource</b>/<b>state</b>. If there are
  * none, return an empty list. This list must be freed using smartlist_free,

src/or/connection.h

@@ -192,13 +192,6 @@ MOCK_DECL(connection_t *,connection_get_by_type_addr_port_purpose,(int type,
 connection_t *connection_get_by_type_state(int type, int state);
 connection_t *connection_get_by_type_state_rendquery(int type, int state,
                                                      const char *rendquery);
-dir_connection_t *connection_dir_get_by_purpose_and_resource(
-                                                  int purpose,
-                                                  const char *resource);
-dir_connection_t *connection_dir_get_by_purpose_resource_and_state(
-                                                  int purpose,
-                                                  const char *resource,
-                                                  int state);
 smartlist_t *connection_dir_list_by_purpose_and_resource(
                                                   int purpose,
                                                   const char *resource);

src/or/directory.c

@@ -96,6 +96,9 @@ static void directory_initiate_command_rend(
                                           time_t if_modified_since,
                                           const rend_data_t *rend_query);
 
+static void connection_dir_close_consensus_fetches(
+                   dir_connection_t *except_this_one, const char *resource);
+
 /********* START VARIABLES **********/
 
 /** How far in the future do we allow a directory server to tell us it is
@@ -1170,12 +1173,6 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
     return;
   }
 
-  /* ensure we don't make excess connections when we're already downloading
-   * a consensus during bootstrap */
-  if (connection_dir_avoid_extra_connection_for_purpose(dir_purpose)) {
-    return;
-  }
-
   conn = dir_connection_new(tor_addr_family(&addr));
 
   /* set up conn so it's got all the data we need to remember */
@@ -1216,11 +1213,6 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
         conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
         /* fall through */
       case 0:
-        /* Close this connection if there's another consensus connection
-         * downloading (during bootstrap), or connecting (after bootstrap). */
-        if (connection_dir_close_consensus_conn_if_extra(conn)) {
-          return;
-        }
         /* queue the command on the outbuf */
         directory_send_command(conn, dir_purpose, 1, resource,
                                payload, payload_len,
@@ -1268,11 +1260,6 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port,
       connection_mark_for_close(TO_CONN(conn));
       return;
     }
-    /* Close this connection if there's another consensus connection
-     * downloading (during bootstrap), or connecting (after bootstrap). */
-    if (connection_dir_close_consensus_conn_if_extra(conn)) {
-      return;
-    }
     conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
     /* queue the command on the outbuf */
     directory_send_command(conn, dir_purpose, 0, resource,
@@ -2029,6 +2016,10 @@ connection_dir_client_reached_eof(dir_connection_t *conn)
       networkstatus_consensus_download_failed(0, flavname);
       return -1;
     }
+
+    /* If we launched other fetches for this consensus, cancel them. */
+    connection_dir_close_consensus_fetches(conn, flavname);
+
     /* launches router downloads as needed */
     routers_update_all_from_networkstatus(now, 3);
     update_microdescs_from_networkstatus(now);
@@ -3826,226 +3817,37 @@ connection_dir_finished_flushing(dir_connection_t *conn)
   return 0;
 }
 
-/* A helper function for connection_dir_close_consensus_conn_if_extra()
- * and connection_dir_close_extra_consensus_conns() that returns 0 if
- * we can't have, or don't want to close, excess consensus connections. */
-STATIC int
-connection_dir_would_close_consensus_conn_helper(void)
-{
-  const or_options_t *options = get_options();
-
-  /* we're only interested in closing excess connections if we could
-   * have created any in the first place */
-  if (!networkstatus_consensus_can_use_multiple_directories(options)) {
-    return 0;
-  }
-
-  /* We want to close excess connections downloading a consensus.
-   * If there aren't any excess, we don't have anything to close. */
-  if (!networkstatus_consensus_has_excess_connections()) {
-    return 0;
-  }
-
-  /* If we have excess connections, but none of them are downloading a
-   * consensus, and we are still bootstrapping (that is, we have no usable
-   * consensus), we don't want to close any until one starts downloading. */
-  if (!networkstatus_consensus_is_downloading_usable_flavor()
-      && networkstatus_consensus_is_bootstrapping(time(NULL))) {
-    return 0;
-  }
-
-  /* If we have just stopped bootstrapping (that is, just parsed a consensus),
-   * we might still have some excess connections hanging around. So we still
-   * have to check if we want to close any, even if we've stopped
-   * bootstrapping. */
-  return 1;
-}
-
-/* Check if we would close excess consensus connections. If we would, any
- * new consensus connection would become excess immediately, so return 1.
- * Otherwise, return 0. */
-int
-connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose)
-{
-  const or_options_t *options = get_options();
-
-  /* We're not interested in connections that aren't fetching a consensus. */
-  if (purpose != DIR_PURPOSE_FETCH_CONSENSUS) {
-    return 0;
-  }
-
-  /* we're only interested in avoiding excess connections if we could
-   * have created any in the first place */
-  if (!networkstatus_consensus_can_use_multiple_directories(options)) {
-    return 0;
-  }
-
-  /* If there are connections downloading a consensus, and we are still
-   * bootstrapping (that is, we have no usable consensus), we can be sure that
-   * any further connections would be excess. */
-  if (networkstatus_consensus_is_downloading_usable_flavor()
-      && networkstatus_consensus_is_bootstrapping(time(NULL))) {
-    return 1;
-  }
-
-  return 0;
-}
-
-/* Check if we have more than one consensus download connection attempt, and
- * close conn:
- * - if we don't have a consensus, and we're downloading a consensus, and conn
- *   is not downloading a consensus yet;
- * - if we do have a consensus, and there's more than one consensus connection.
+/* We just got a new consensus! If there are other in-progress requests
+ * for this consensus flavor (for example because we launched several in
+ * parallel), cancel them.
  *
- * Post-bootstrap consensus connection attempts are initiated one at a time.
- * So this function won't close any consensus connection attempts that
- * are initiated after bootstrap.
- */
-int
-connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn)
-{
-  tor_assert(conn);
-  tor_assert(conn->base_.type == CONN_TYPE_DIR);
-
-  /* We're not interested in connections that aren't fetching a consensus. */
-  if (conn->base_.purpose != DIR_PURPOSE_FETCH_CONSENSUS) {
-    return 0;
-  }
-
-  /* The connection has already been closed */
-  if (conn->base_.marked_for_close) {
-    return 0;
-  }
-
-  /* Only close this connection if there's another consensus connection
-   * downloading (during bootstrap), or connecting (after bootstrap).
-   * Post-bootstrap consensus connection attempts won't be closed, because
-   * they only occur one at a time. */
-  if (!connection_dir_would_close_consensus_conn_helper()) {
-    return 0;
-  }
-
-  const int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping(
-                                                                  time(NULL));
-
-  /* We don't want to check other connections to see if they are downloading,
-   * as this is prone to race-conditions. So leave it for
-   * connection_dir_close_extra_consensus_conns(() to clean up.
-   *
-   * But if conn has just started connecting, or we have a consensus already,
-   * we can be sure it's not needed any more. */
-  if (!we_are_bootstrapping
-      || conn->base_.state == DIR_CONN_STATE_CONNECTING) {
-    connection_close_immediate(&conn->base_);
-    connection_mark_for_close(&conn->base_);
-    return -1;
-  }
-
-  return 0;
-}
-
-/* Clean up excess consensus download connection attempts.
- * During bootstrap, or when the bootstrap consensus has just been downloaded,
- * if we have more than one active consensus connection:
- * - if we don't have a consensus, and we're downloading a consensus, keep an
- *   earlier connection, or a connection to a fallback directory, and close
- *   all other connections;
- * - if we have just downloaded the bootstrap consensus, and have other
- *   consensus connections left over, close all of them.
+ * We do this check here (not just in
+ * connection_ap_handshake_attach_circuit()) to handle the edge case where
+ * a consensus fetch begins and ends before some other one tries to attach to
+ * a circuit, in which case the other one won't know that we're all happy now.
  *
- * Post-bootstrap consensus connection attempts are initiated one at a time.
- * So this function won't close any consensus connection attempts that
- * are initiated after bootstrap.
+ * Don't mark the conn that just gave us the consensus -- otherwise we
+ * would end up double-marking it when it cleans itself up.
  */
-void
-connection_dir_close_extra_consensus_conns(void)
+static void
+connection_dir_close_consensus_fetches(dir_connection_t *except_this_one,
+                                       const char *resource)
 {
-  /* Only cleanup connections if there is more than one consensus connection,
-   * and at least one of those connections is already downloading
-   * (during bootstrap), or connecting (just after the bootstrap consensus is
-   * downloaded).
-   * Post-bootstrap consensus connection attempts won't be cleaned up, because
-   * they only occur one at a time. */
-  if (!connection_dir_would_close_consensus_conn_helper()) {
-    return;
-  }
-
-  int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping(
-                                                                  time(NULL));
-
-  const char *usable_resource = networkstatus_get_flavor_name(
-                                                  usable_consensus_flavor());
-  smartlist_t *consens_usable_conns =
-                 connection_dir_list_by_purpose_and_resource(
-                                                  DIR_PURPOSE_FETCH_CONSENSUS,
-                                                  usable_resource);
-
-  /* If we want to keep a connection that's downloading, find a connection to
-   * keep, favouring:
-   * - connections opened earlier (they are likely to have progressed further)
-   * - connections to fallbacks (to reduce the load on authorities) */
-  dir_connection_t *kept_download_conn = NULL;
-  int kept_is_authority = 0;
-  if (we_are_bootstrapping) {
-    SMARTLIST_FOREACH_BEGIN(consens_usable_conns,
-                            dir_connection_t *, d) {
-      tor_assert(d);
-      int d_is_authority = router_digest_is_trusted_dir(d->identity_digest);
-      /* keep the first connection that is past the connecting state, but
-       * prefer fallbacks. */
-      if (d->base_.state != DIR_CONN_STATE_CONNECTING) {
-        if (!kept_download_conn || (kept_is_authority && !d_is_authority)) {
-          kept_download_conn = d;
-          kept_is_authority = d_is_authority;
-          /* we've found the earliest fallback, and want to keep it regardless
-           * of any other connections */
-          if (!kept_is_authority)
-            break;
-        }
-      }
-    } SMARTLIST_FOREACH_END(d);
-  }
-
-  SMARTLIST_FOREACH_BEGIN(consens_usable_conns,
-                          dir_connection_t *, d) {
-    tor_assert(d);
-    /* don't close this connection if it's the one we want to keep */
-    if (kept_download_conn && d == kept_download_conn)
+  smartlist_t *conns_to_close =
+    connection_dir_list_by_purpose_and_resource(DIR_PURPOSE_FETCH_CONSENSUS,
+                                                resource);
+  SMARTLIST_FOREACH_BEGIN(conns_to_close, dir_connection_t *, d) {
+    if (d == except_this_one)
       continue;
-    /* mark all other connections for close */
-    if (!d->base_.marked_for_close) {
-      connection_close_immediate(&d->base_);
-      connection_mark_for_close(&d->base_);
-    }
+    log_info(LD_DIR, "Closing consensus fetch (to %s) since one "
+             "has just arrived.", TO_CONN(d)->address);
+    connection_mark_for_close(TO_CONN(d));
   } SMARTLIST_FOREACH_END(d);
-
-  smartlist_free(consens_usable_conns);
-  consens_usable_conns = NULL;
-
-  /* make sure we've closed all excess connections */
-  const int final_connecting_conn_count =
-              connection_dir_count_by_purpose_resource_and_state(
-                                                DIR_PURPOSE_FETCH_CONSENSUS,
-                                                usable_resource,
-                                                DIR_CONN_STATE_CONNECTING);
-  if (final_connecting_conn_count > 0) {
-    log_warn(LD_BUG, "Expected 0 consensus connections connecting after "
-             "cleanup, got %d.", final_connecting_conn_count);
-  }
-  const int expected_final_conn_count = (we_are_bootstrapping ? 1 : 0);
-  const int final_conn_count =
-              connection_dir_count_by_purpose_and_resource(
-                                                DIR_PURPOSE_FETCH_CONSENSUS,
-                                                usable_resource);
-  if (final_conn_count > expected_final_conn_count) {
-    log_warn(LD_BUG, "Expected %d consensus connections after cleanup, got "
-             "%d.", expected_final_conn_count, final_connecting_conn_count);
-  }
+  smartlist_free(conns_to_close);
 }
 
 /** Connected handler for directory connections: begin sending data to the
- * server, and return 0, or, if the connection is an excess bootstrap
- * connection, close all excess bootstrap connections.
+ * server, and return 0.
  * Only used when connections don't immediately connect. */
 int
 connection_dir_finished_connecting(dir_connection_t *conn)
@@ -4057,12 +3859,6 @@ connection_dir_finished_connecting(dir_connection_t *conn)
   log_debug(LD_HTTP,"Dir connection to router %s:%u established.",
             conn->base_.address,conn->base_.port);
 
-  /* Close this connection if there's another consensus connection
-   * downloading (during bootstrap), or connecting (after bootstrap). */
-  if (connection_dir_close_consensus_conn_if_extra(conn)) {
-    return -1;
-  }
-
   /* start flushing conn */
   conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
   return 0;

src/or/directory.h

@@ -78,9 +78,6 @@ void directory_initiate_command(const tor_addr_t *or_addr, uint16_t or_port,
                                 const char *resource,
                                 const char *payload, size_t payload_len,
                                 time_t if_modified_since);
-int connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose);
-int connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn);
-void connection_dir_close_extra_consensus_conns(void);
 
 #define DSR_HEX       (1<<0)
 #define DSR_BASE64    (1<<1)
@@ -147,7 +144,6 @@ STATIC int directory_handle_command_get(dir_connection_t *conn,
                                         const char *headers,
                                         const char *req_body,
                                         size_t req_body_len);
-STATIC int connection_dir_would_close_consensus_conn_helper(void);
 STATIC int download_status_schedule_get_delay(download_status_t *dls,
                                               const smartlist_t *schedule,
                                               time_t now);

src/or/main.c

@@ -1484,17 +1484,6 @@ run_scheduled_events(time_t now)
     dirvote_act(options, now);
   }
 
-  /* 2d. Cleanup excess consensus bootstrap connections every second.
-   * connection_dir_close_consensus_conn_if_extra() closes some connections
-   * that are clearly excess, but this check is more thorough.
-   * This only closes connections if there is more than one consensus
-   * connection, and at least one of those connections is already downloading
-   * (during bootstrap), or connecting (just after the bootstrap consensus is
-   * downloaded).
-   * It won't close any consensus connections initiated after bootstrap,
-   * because those attempts are made one at a time. */
-  connection_dir_close_extra_consensus_conns();
-
   /* 3a. Every second, we examine pending circuits and prune the
    *    ones which have been pending for more than a few seconds.
    *    We do this before step 4, so it can try building more if

src/or/microdesc.c

@@ -955,8 +955,8 @@ we_fetch_router_descriptors(const or_options_t *options)
 }
 
 /** Return the consensus flavor we actually want to use to build circuits. */
-int
-usable_consensus_flavor(void)
+MOCK_IMPL(int,
+usable_consensus_flavor,(void))
 {
   if (we_use_microdescriptors_for_circuits(get_options())) {
     return FLAV_MICRODESC;

src/or/microdesc.h

@@ -47,7 +47,7 @@ void microdesc_free_all(void);
 void update_microdesc_downloads(time_t now);
 void update_microdescs_from_networkstatus(time_t now);
 
-int usable_consensus_flavor(void);
+MOCK_DECL(int, usable_consensus_flavor,(void));
 int we_fetch_microdescriptors(const or_options_t *options);
 int we_fetch_router_descriptors(const or_options_t *options);
 int we_use_microdescriptors_for_circuits(const or_options_t *options);

src/or/networkstatus.c

@@ -121,8 +121,7 @@ static int have_warned_about_new_version = 0;
 static void routerstatus_list_update_named_server_map(void);
 static void update_consensus_bootstrap_multiple_downloads(
                                                   time_t now,
-                                                  const or_options_t *options,
-                                                  int we_are_bootstrapping);
+                                                  const or_options_t *options);
 
 /** Forget that we've warned about anything networkstatus-related, so we will
  * give fresh warnings if the same behavior happens again. */
@@ -793,26 +792,6 @@ check_consensus_waiting_for_certs(int flavor, time_t now,
   return 0;
 }
 
-/* Return the maximum download tries for a consensus, based on options and
- * whether we_are_bootstrapping. */
-static int
-consensus_max_download_tries(const or_options_t *options,
-                                        int we_are_bootstrapping)
-{
-  int use_fallbacks = networkstatus_consensus_can_use_extra_fallbacks(options);
-
-  if (we_are_bootstrapping) {
-    if (use_fallbacks) {
-      return options->ClientBootstrapConsensusMaxDownloadTries;
-    } else {
-      return
-      options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries;
-    }
-  }
-
-  return options->TestingConsensusMaxDownloadTries;
-}
-
 /** If we want to download a fresh consensus, launch a new download as
  * appropriate. */
 static void
@@ -866,29 +845,14 @@ update_consensus_networkstatus_downloads(time_t now)
         && i == usable_consensus_flavor()) {
 
       /* Check if we're already downloading a usable consensus */
-      int consens_conn_count =
-        connection_dir_count_by_purpose_and_resource(
-                                                   DIR_PURPOSE_FETCH_CONSENSUS,
-                                                   resource);
-      int connect_consens_conn_count =
-        connection_dir_count_by_purpose_resource_and_state(
-                                                   DIR_PURPOSE_FETCH_CONSENSUS,
-                                                   resource,
-                                                   DIR_CONN_STATE_CONNECTING);
-
-      /* If not all connections are "connecting", then some are
-       * downloading. We want to have at most one downloading at a time. */
-      if (connect_consens_conn_count < consens_conn_count) {
+      if (networkstatus_consensus_is_already_downloading(resource))
         continue;
-      }
 
       /* Make multiple connections for a bootstrap consensus download. */
-      update_consensus_bootstrap_multiple_downloads(now, options,
-                                                    we_are_bootstrapping);
+      update_consensus_bootstrap_multiple_downloads(now, options);
     } else {
       /* Check if we failed downloading a consensus too recently */
-      int max_dl_tries = consensus_max_download_tries(options,
-                                                      we_are_bootstrapping);
+      int max_dl_tries = options->TestingConsensusMaxDownloadTries;
 
       /* Let's make sure we remembered to update consensus_dl_status */
       tor_assert(consensus_dl_status[i].schedule == DL_SCHED_CONSENSUS);
@@ -923,12 +887,16 @@ static void
 update_consensus_bootstrap_attempt_downloads(
                                       time_t now,
                                       const or_options_t *options,
-                                      int we_are_bootstrapping,
                                       download_status_t *dls,
                                       download_want_authority_t want_authority)
 {
-  int max_dl_tries = consensus_max_download_tries(options,
-                                                  we_are_bootstrapping);
+  int use_fallbacks = networkstatus_consensus_can_use_extra_fallbacks(options);
+  int max_dl_tries = options->ClientBootstrapConsensusMaxDownloadTries;
+  if (!use_fallbacks) {
+    max_dl_tries =
+              options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries;
+  }
+
   const char *resource = networkstatus_get_flavor_name(
                                                   usable_consensus_flavor());
 
@@ -961,8 +929,7 @@ update_consensus_bootstrap_attempt_downloads(
  */
 static void
 update_consensus_bootstrap_multiple_downloads(time_t now,
-                                              const or_options_t *options,
-                                              int we_are_bootstrapping)
+                                              const or_options_t *options)
 {
   const int usable_flavor = usable_consensus_flavor();
 
@@ -971,12 +938,6 @@ update_consensus_bootstrap_multiple_downloads(time_t now,
     return;
   }
 
-  /* If we've managed to validate a usable consensus, don't make additional
-   * connections. */
-  if (!we_are_bootstrapping) {
-    return;
-  }
-
   /* Launch concurrent consensus download attempt(s) based on the mirror and
    * authority schedules. Try the mirror first - this makes it slightly more
    * likely that we'll connect to the fallback first, and then end the
@@ -995,8 +956,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now,
 
     if (!check_consensus_waiting_for_certs(usable_flavor, now, dls_f)) {
       /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */
-      update_consensus_bootstrap_attempt_downloads(now, options,
-                                                   we_are_bootstrapping, dls_f,
+      update_consensus_bootstrap_attempt_downloads(now, options, dls_f,
                                                    DL_WANT_ANY_DIRSERVER);
     }
   }
@@ -1006,8 +966,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now,
     &consensus_bootstrap_dl_status[CONSENSUS_BOOTSTRAP_SOURCE_AUTHORITY];
 
   if (!check_consensus_waiting_for_certs(usable_flavor, now, dls_a)) {
-    update_consensus_bootstrap_attempt_downloads(now, options,
-                                                 we_are_bootstrapping, dls_a,
+    update_consensus_bootstrap_attempt_downloads(now, options, dls_a,
                                                  DL_WANT_AUTHORITY);
   }
 }
@@ -1275,16 +1234,30 @@ networkstatus_get_reasonably_live_consensus(time_t now, int flavor)
     return NULL;
 }
 
-/** Check if we're bootstrapping a consensus download. This means that we are
- *  only using the authorities and fallback directory mirrors to download the
- * consensus flavour we'll use. */
-int
-networkstatus_consensus_is_bootstrapping(time_t now)
+/** Check if we need to download a consensus during tor's bootstrap phase.
+ * If we have no consensus, or our consensus is unusably old, return 1.
+ * As soon as we have received a consensus, return 0, even if we don't have
+ * enough certificates to validate it. */
+MOCK_IMPL(int,
+networkstatus_consensus_is_bootstrapping,(time_t now))
 {
-  /* If we don't have a consensus, we must still be bootstrapping */
-  return !networkstatus_get_reasonably_live_consensus(
-                                                  now,
-                                                  usable_consensus_flavor());
+  /* If we have a validated, reasonably live consensus, we're not
+   * bootstrapping a consensus at all. */
+  if (networkstatus_get_reasonably_live_consensus(
+                                                now,
+                                                usable_consensus_flavor())) {
+    return 0;
+  }
+
+  /* If we have a consensus, but we're waiting for certificates,
+   * we're not waiting for a consensus download while bootstrapping. */
+  if (consensus_is_waiting_for_certs()) {
+    return 0;
+  }
+
+  /* If we have no consensus, or our consensus is very old, we are
+   * bootstrapping, and we need to download a consensus. */
+  return 1;
 }
 
 /** Check if we can use multiple directories for a consensus download.
@@ -1301,8 +1274,8 @@ networkstatus_consensus_can_use_multiple_directories(
 /** Check if we can use fallback directory mirrors for a consensus download.
  * If we have fallbacks and don't want to fetch from the authorities,
  * we can use them. */
-int
-networkstatus_consensus_can_use_extra_fallbacks(const or_options_t *options)
+MOCK_IMPL(int,
+networkstatus_consensus_can_use_extra_fallbacks,(const or_options_t *options))
 {
   /* The list length comparisons are a quick way to check if we have any
    * non-authority fallback directories. If we ever have any authorities that
@@ -1316,61 +1289,39 @@ networkstatus_consensus_can_use_extra_fallbacks(const or_options_t *options)
               > smartlist_len(router_get_trusted_dir_servers())));
 }
 
-/* Check if there is more than 1 consensus connection retrieving the usable
- * consensus flavor. If so, return 1, if not, return 0.
- *
- * During normal operation, Tor only makes one consensus download
- * connection. But clients can make multiple simultaneous consensus
- * connections to improve bootstrap speed and reliability.
- *
- * If there is more than one connection, we must have connections left
- * over from bootstrapping. However, some of the connections may have
- * completed and been cleaned up, so it is not sufficient to check the
- * return value of this function to see if a client could make multiple
- * bootstrap connections. Use
- * networkstatus_consensus_can_use_multiple_directories()
- * and networkstatus_consensus_is_bootstrapping(). */
-int
-networkstatus_consensus_has_excess_connections(void)
-{
-  const char *usable_resource = networkstatus_get_flavor_name(
-                                                  usable_consensus_flavor());
-  const int consens_conn_usable_count =
-              connection_dir_count_by_purpose_and_resource(
-                                               DIR_PURPOSE_FETCH_CONSENSUS,
-                                               usable_resource);
-  /* The maximum number of connections we want downloading a usable consensus
-   * Always 1, whether bootstrapping or not. */
-  const int max_expected_consens_conn_usable_count = 1;
-
-  if (consens_conn_usable_count > max_expected_consens_conn_usable_count) {
-    return 1;
-  }
-
-  return 0;
-}
-
-/* Is tor currently downloading a consensus of the usable flavor? */
+/* Is there a consensus fetch for flavor <b>resource</b> that's far
+ * enough along to be attached to a circuit? */
 int
-networkstatus_consensus_is_downloading_usable_flavor(void)
-{
-  const char *usable_resource = networkstatus_get_flavor_name(
-                                                  usable_consensus_flavor());
-  const int consens_conn_usable_count =
-              connection_dir_count_by_purpose_and_resource(
-                                               DIR_PURPOSE_FETCH_CONSENSUS,
-                                               usable_resource);
-
-  const int connect_consens_conn_usable_count =
-              connection_dir_count_by_purpose_resource_and_state(
-                                                DIR_PURPOSE_FETCH_CONSENSUS,
-                                                usable_resource,
-                                                DIR_CONN_STATE_CONNECTING);
-  if (connect_consens_conn_usable_count < consens_conn_usable_count) {
-    return 1;
-  }
+networkstatus_consensus_is_already_downloading(const char *resource)
+{
+  int answer = 0;
+
+  /* First, get a list of all the dir conns that are fetching a consensus,
+   * fetching *this* consensus, and are in state "reading" (meaning they
+   * have already flushed their request onto the socks connection). */
+  smartlist_t *fetching_conns =
+    connection_dir_list_by_purpose_resource_and_state(
+      DIR_PURPOSE_FETCH_CONSENSUS, resource, DIR_CONN_STATE_CLIENT_READING);
+
+  /* Then, walk through each conn, to see if its linked socks connection
+   * is in an attached state. We have to check this separately, since with
+   * the optimistic data feature, fetches can send their request to the
+   * socks connection and go into state 'reading', even before they're
+   * attached to any circuit. */
+  SMARTLIST_FOREACH_BEGIN(fetching_conns, dir_connection_t *, dirconn) {
+    /* Do any of these other dir conns have a linked socks conn that is
+     * attached to a circuit already? */
+    connection_t *base = TO_CONN(dirconn);
+    if (base->linked_conn &&
+        base->linked_conn->type == CONN_TYPE_AP &&
+        !AP_CONN_STATE_IS_UNATTACHED(base->linked_conn->state)) {
+      answer = 1;
+      break; /* stop looping, because we know the answer will be yes */
+    }
+  } SMARTLIST_FOREACH_END(dirconn);
+  smartlist_free(fetching_conns);
 
-  return 0;
+  return answer;
 }
 
 /** Given two router status entries for the same router identity, return 1 if

src/or/networkstatus.h

@@ -70,13 +70,12 @@ MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus_by_flavor,
 networkstatus_t *networkstatus_get_live_consensus(time_t now);
 networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now,
                                                              int flavor);
-int networkstatus_consensus_is_bootstrapping(time_t now);
+MOCK_DECL(int, networkstatus_consensus_is_bootstrapping,(time_t now));
 int networkstatus_consensus_can_use_multiple_directories(
                                                 const or_options_t *options);
-int networkstatus_consensus_can_use_extra_fallbacks(
-                                                const or_options_t *options);
-int networkstatus_consensus_has_excess_connections(void);
-int networkstatus_consensus_is_downloading_usable_flavor(void);
+MOCK_DECL(int, networkstatus_consensus_can_use_extra_fallbacks,(
+                                                const or_options_t *options));
+int networkstatus_consensus_is_already_downloading(const char *resource);
 
 #define NSSET_FROM_CACHE 1
 #define NSSET_WAS_WAITING_FOR_CERTS 2

src/test/test_connection.c

@@ -11,6 +11,7 @@
 
 #include "connection.h"
 #include "main.h"
+#include "microdesc.h"
 #include "networkstatus.h"
 #include "rendcache.h"
 #include "directory.h"
@@ -54,7 +55,11 @@ static int test_conn_get_rsrc_teardown(const struct testcase_t *tc,
 #define TEST_CONN_RSRC_STATE_SUCCESSFUL (DIR_CONN_STATE_CLIENT_FINISHED)
 #define TEST_CONN_RSRC_2        (networkstatus_get_flavor_name(FLAV_NS))
 
-#define TEST_CONN_DL_STATE      (DIR_CONN_STATE_CLIENT_SENDING)
+#define TEST_CONN_DL_STATE      (DIR_CONN_STATE_CLIENT_READING)
+
+/* see AP_CONN_STATE_IS_UNATTACHED() */
+#define TEST_CONN_UNATTACHED_STATE (AP_CONN_STATE_CIRCUIT_WAIT)
+#define TEST_CONN_ATTACHED_STATE   (AP_CONN_STATE_CONNECT_WAIT)
 
 #define TEST_CONN_FD_INIT 50
 static int mock_connection_connect_sockaddr_called = 0;
@@ -109,27 +114,25 @@ test_conn_lookup_addr_helper(const char *address, int family, tor_addr_t *addr)
   tor_addr_make_null(addr, TEST_CONN_FAMILY);
 }
 
-static void *
-test_conn_get_basic_setup(const struct testcase_t *tc)
+static connection_t *
+test_conn_get_connection(uint8_t state, uint8_t type, uint8_t purpose)
 {
   connection_t *conn = NULL;
   tor_addr_t addr;
   int socket_err = 0;
   int in_progress = 0;
-  (void)tc;
 
   MOCK(connection_connect_sockaddr,
        mock_connection_connect_sockaddr);
 
   init_connection_lists();
 
-  conn = connection_new(TEST_CONN_TYPE, TEST_CONN_FAMILY);
+  conn = connection_new(type, TEST_CONN_FAMILY);
   tt_assert(conn);
 
   test_conn_lookup_addr_helper(TEST_CONN_ADDRESS, TEST_CONN_FAMILY, &addr);
   tt_assert(!tor_addr_is_null(&addr));
 
-  /* XXXX - connection_connect doesn't set these, should it? */
   tor_addr_copy_tight(&conn->addr, &addr);
   conn->port = TEST_CONN_PORT;
   mock_connection_connect_sockaddr_called = 0;
@@ -140,8 +143,8 @@ test_conn_get_basic_setup(const struct testcase_t *tc)
   tt_assert(in_progress == 0 || in_progress == 1);
 
   /* fake some of the attributes so the connection looks OK */
-  conn->state = TEST_CONN_STATE;
-  conn->purpose = TEST_CONN_BASIC_PURPOSE;
+  conn->state = state;
+  conn->purpose = purpose;
   assert_connection_ok(conn, time(NULL));
 
   UNMOCK(connection_connect_sockaddr);
@@ -151,12 +154,17 @@ test_conn_get_basic_setup(const struct testcase_t *tc)
   /* On failure */
  done:
   UNMOCK(connection_connect_sockaddr);
-  test_conn_get_basic_teardown(tc, conn);
-
-  /* Returning NULL causes the unit test to fail */
   return NULL;
 }
 
+static void *
+test_conn_get_basic_setup(const struct testcase_t *tc)
+{
+  (void)tc;
+  return test_conn_get_connection(TEST_CONN_STATE, TEST_CONN_TYPE,
+                                  TEST_CONN_BASIC_PURPOSE);
+}
+
 static int
 test_conn_get_basic_teardown(const struct testcase_t *tc, void *arg)
 {
@@ -186,9 +194,8 @@ test_conn_get_basic_teardown(const struct testcase_t *tc, void *arg)
       connection_close_immediate(conn->linked_conn);
       connection_mark_for_close(conn->linked_conn);
     }
-    conn->linked_conn->linked_conn = NULL;
-    connection_free(conn->linked_conn);
-    conn->linked_conn = NULL;
+
+    close_closeable_connections();
   }
 
   /* We didn't set the events up properly, so we can't use event_del() in
@@ -222,7 +229,10 @@ static void *
 test_conn_get_rend_setup(const struct testcase_t *tc)
 {
   dir_connection_t *conn = DOWNCAST(dir_connection_t,
-                                    test_conn_get_basic_setup(tc));
+                                    test_conn_get_connection(
+                                                    TEST_CONN_STATE,
+                                                    TEST_CONN_TYPE,
+                                                    TEST_CONN_REND_PURPOSE));
   tt_assert(conn);
   assert_connection_ok(&conn->base_, time(NULL));
 
@@ -235,7 +245,6 @@ test_conn_get_rend_setup(const struct testcase_t *tc)
          TEST_CONN_REND_ADDR,
          REND_SERVICE_ID_LEN_BASE32+1);
   conn->rend_data->hsdirs_fp = smartlist_new();
-  conn->base_.purpose = TEST_CONN_REND_PURPOSE;
 
   assert_connection_ok(&conn->base_, time(NULL));
   return conn;
@@ -266,42 +275,64 @@ test_conn_get_rend_teardown(const struct testcase_t *tc, void *arg)
   return rv;
 }
 
-static void *
-test_conn_get_rsrc_setup(const struct testcase_t *tc)
+static dir_connection_t *
+test_conn_download_status_add_a_connection(const char *resource)
 {
   dir_connection_t *conn = DOWNCAST(dir_connection_t,
-                                    test_conn_get_basic_setup(tc));
+                                    test_conn_get_connection(
+                                                      TEST_CONN_STATE,
+                                                      TEST_CONN_TYPE,
+                                                      TEST_CONN_RSRC_PURPOSE));
+
   tt_assert(conn);
   assert_connection_ok(&conn->base_, time(NULL));
 
-  /* TODO: use the canonical function to do this - maybe? */
-  conn->requested_resource = tor_strdup(TEST_CONN_RSRC);
-  conn->base_.purpose = TEST_CONN_RSRC_PURPOSE;
+  /* Replace the existing resource with the one we want */
+  if (resource) {
+    if (conn->requested_resource) {
+      tor_free(conn->requested_resource);
+    }
+    conn->requested_resource = tor_strdup(resource);
+    assert_connection_ok(&conn->base_, time(NULL));
+  }
 
-  assert_connection_ok(&conn->base_, time(NULL));
   return conn;
 
-  /* On failure */
  done:
-  test_conn_get_rend_teardown(tc, conn);
-  /* Returning NULL causes the unit test to fail */
+  test_conn_get_rsrc_teardown(NULL, conn);
   return NULL;
 }
 
+static void *
+test_conn_get_rsrc_setup(const struct testcase_t *tc)
+{
+  (void)tc;
+  return test_conn_download_status_add_a_connection(TEST_CONN_RSRC);
+}
+
 static int
 test_conn_get_rsrc_teardown(const struct testcase_t *tc, void *arg)
 {
-  dir_connection_t *conn = DOWNCAST(dir_connection_t, arg);
   int rv = 0;
 
+  connection_t *conn = (connection_t *)arg;
   tt_assert(conn);
-  assert_connection_ok(&conn->base_, time(NULL));
+  assert_connection_ok(conn, time(NULL));
+
+  if (conn->type == CONN_TYPE_DIR) {
+    dir_connection_t *dir_conn = DOWNCAST(dir_connection_t, arg);
+
+    tt_assert(dir_conn);
+    assert_connection_ok(&dir_conn->base_, time(NULL));
 
-  /* avoid a last-ditch attempt to refetch the consensus */
-  conn->base_.state = TEST_CONN_RSRC_STATE_SUCCESSFUL;
+    /* avoid a last-ditch attempt to refetch the consensus */
+    dir_conn->base_.state = TEST_CONN_RSRC_STATE_SUCCESSFUL;
+    assert_connection_ok(&dir_conn->base_, time(NULL));
+  }
 
   /* connection_free_() cleans up requested_resource */
-  rv = test_conn_get_basic_teardown(tc, arg);
+  rv = test_conn_get_basic_teardown(tc, conn);
+
  done:
   return rv;
 }
@@ -336,14 +367,30 @@ test_conn_download_status_teardown(const struct testcase_t *tc, void *arg)
   return rv;
 }
 
-static dir_connection_t *
-test_conn_download_status_add_a_connection(void)
+/* Like connection_ap_make_link(), but does much less */
+static connection_t *
+test_conn_get_linked_connection(connection_t *l_conn, uint8_t state)
 {
-  dir_connection_t *conn = DOWNCAST(dir_connection_t,
-                                    test_conn_get_rsrc_setup(NULL));
+  tt_assert(l_conn);
+  assert_connection_ok(l_conn, time(NULL));
+
+  /* AP connections don't seem to have purposes */
+  connection_t *conn = test_conn_get_connection(state, CONN_TYPE_AP,
+                                                       0);
 
   tt_assert(conn);
-  assert_connection_ok(&conn->base_, time(NULL));
+  assert_connection_ok(conn, time(NULL));
+
+  conn->linked = 1;
+  l_conn->linked = 1;
+  conn->linked_conn = l_conn;
+  l_conn->linked_conn = conn;
+  /* we never opened a real socket, so we can just overwrite it */
+  conn->s = TOR_INVALID_SOCKET;
+  l_conn->s = TOR_INVALID_SOCKET;
+
+  assert_connection_ok(conn, time(NULL));
+  assert_connection_ok(l_conn, time(NULL));
 
   return conn;
 
@@ -524,44 +571,6 @@ test_conn_get_rsrc(void *arg)
   tt_assert(conn);
   assert_connection_ok(&conn->base_, time(NULL));
 
-  tt_assert(connection_dir_get_by_purpose_and_resource(
-                                                    conn->base_.purpose,
-                                                    conn->requested_resource)
-            == conn);
-  tt_assert(connection_dir_get_by_purpose_and_resource(
-                                                    TEST_CONN_RSRC_PURPOSE,
-                                                    TEST_CONN_RSRC)
-            == conn);
-  tt_assert(connection_dir_get_by_purpose_and_resource(
-                                                    !conn->base_.purpose,
-                                                    "")
-            == NULL);
-  tt_assert(connection_dir_get_by_purpose_and_resource(
-                                                    !TEST_CONN_RSRC_PURPOSE,
-                                                    TEST_CONN_RSRC_2)
-            == NULL);
-
-  tt_assert(connection_dir_get_by_purpose_resource_and_state(
-                                                    conn->base_.purpose,
-                                                    conn->requested_resource,
-                                                    conn->base_.state)
-            == conn);
-  tt_assert(connection_dir_get_by_purpose_resource_and_state(
-                                                    TEST_CONN_RSRC_PURPOSE,
-                                                    TEST_CONN_RSRC,
-                                                    TEST_CONN_STATE)
-            == conn);
-  tt_assert(connection_dir_get_by_purpose_resource_and_state(
-                                                    !conn->base_.purpose,
-                                                    "",
-                                                    !conn->base_.state)
-            == NULL);
-  tt_assert(connection_dir_get_by_purpose_resource_and_state(
-                                                    !TEST_CONN_RSRC_PURPOSE,
-                                                    TEST_CONN_RSRC_2,
-                                                    !TEST_CONN_STATE)
-            == NULL);
-
   sl_is_conn_assert(connection_dir_list_by_purpose_and_resource(
                                                     conn->base_.purpose,
                                                     conn->requested_resource),
@@ -641,107 +650,190 @@ test_conn_get_rsrc(void *arg)
 static void
 test_conn_download_status(void *arg)
 {
-  (void)arg;
   dir_connection_t *conn = NULL;
   dir_connection_t *conn2 = NULL;
   dir_connection_t *conn3 = NULL;
+  dir_connection_t *conn4 = NULL;
+  connection_t *ap_conn = NULL;
+  connection_t *ap_conn2 = NULL;
+  /* we never create an ap_conn for conn3 */
+  connection_t *ap_conn4 = NULL;
+
+  consensus_flavor_t usable_flavor = (consensus_flavor_t)arg;
+
+  /* The "other flavor" trick only works if there are two flavors */
+  tor_assert(N_CONSENSUS_FLAVORS == 2);
+  consensus_flavor_t other_flavor = ((usable_flavor == FLAV_NS)
+                                     ? FLAV_MICRODESC
+                                     : FLAV_NS);
+  const char *res = networkstatus_get_flavor_name(usable_flavor);
+  const char *other_res = networkstatus_get_flavor_name(other_flavor);
+
+  /* no connections */
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
 
-  /* no connections, no excess, not downloading */
-  tt_assert(networkstatus_consensus_has_excess_connections() == 0);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 0);
-
-  /* one connection, no excess, not downloading */
-  conn = test_conn_download_status_add_a_connection();
-  tt_assert(networkstatus_consensus_has_excess_connections() == 0);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 0);
+  /* one connection, not downloading */
+  conn = test_conn_download_status_add_a_connection(res);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
 
-  /* one connection, no excess, but downloading */
+  /* one connection, downloading but not linked (not possible on a client,
+   * but possible on a relay) */
   conn->base_.state = TEST_CONN_DL_STATE;
-  tt_assert(networkstatus_consensus_has_excess_connections() == 0);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
-  conn->base_.state = TEST_CONN_STATE;
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
 
-  /* two connections, excess, but not downloading */
-  conn2 = test_conn_download_status_add_a_connection();
-  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 0);
+  /* one connection, downloading and linked, but not yet attached */
+  ap_conn = test_conn_get_linked_connection(TO_CONN(conn),
+                                            TEST_CONN_UNATTACHED_STATE);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
 
-  /* two connections, excess, downloading */
-  conn2->base_.state = TEST_CONN_DL_STATE;
-  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
-  conn2->base_.state = TEST_CONN_STATE;
-
-  /* more connections, excess, but not downloading */
-  conn3 = test_conn_download_status_add_a_connection();
-  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 0);
-
-  /* more connections, excess, downloading */
-  conn3->base_.state = TEST_CONN_DL_STATE;
-  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
-
-  /* more connections, more downloading */
-  conn2->base_.state = TEST_CONN_DL_STATE;
-  tt_assert(networkstatus_consensus_has_excess_connections() == 1);
-  tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
+    /* one connection, downloading and linked and attached */
+  ap_conn->state = TEST_CONN_ATTACHED_STATE;
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
 
-  /* now try closing the one that isn't downloading:
-   * these tests won't work unless tor thinks it is bootstrapping */
-  tt_assert(networkstatus_consensus_is_bootstrapping(time(NULL)));
+  /* one connection, linked and attached but not downloading */
+  conn->base_.state = TEST_CONN_STATE;
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
 
+  /* two connections, both not downloading */
+  conn2 = test_conn_download_status_add_a_connection(res);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
   tt_assert(connection_dir_count_by_purpose_and_resource(
                                                         TEST_CONN_RSRC_PURPOSE,
-                                                        TEST_CONN_RSRC) == 3);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
-  tt_assert(connection_dir_close_consensus_conn_if_extra(conn) == -1);
+                                                        res) == 2);
   tt_assert(connection_dir_count_by_purpose_and_resource(
                                                         TEST_CONN_RSRC_PURPOSE,
-                                                        TEST_CONN_RSRC) == 2);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
+                                                         other_res) == 0);
 
-  /* now try closing one that is already closed - nothing happens */
-  tt_assert(connection_dir_close_consensus_conn_if_extra(conn) == 0);
+  /* two connections, one downloading */
+  conn->base_.state = TEST_CONN_DL_STATE;
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
   tt_assert(connection_dir_count_by_purpose_and_resource(
                                                         TEST_CONN_RSRC_PURPOSE,
-                                                        TEST_CONN_RSRC) == 2);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
+                                                        res) == 2);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
+  conn->base_.state = TEST_CONN_STATE;
 
-  /* now try closing one that is downloading - it stays open */
-  tt_assert(connection_dir_close_consensus_conn_if_extra(conn2) == 0);
+  /* more connections, all not downloading */
+  conn3 = test_conn_download_status_add_a_connection(res);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 0);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 3);
   tt_assert(connection_dir_count_by_purpose_and_resource(
                                                         TEST_CONN_RSRC_PURPOSE,
-                                                        TEST_CONN_RSRC) == 2);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
+                                                         other_res) == 0);
 
-  /* now try closing all excess connections */
-  connection_dir_close_extra_consensus_conns();
+  /* more connections, one downloading */
+  conn->base_.state = TEST_CONN_DL_STATE;
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 3);
   tt_assert(connection_dir_count_by_purpose_and_resource(
                                                         TEST_CONN_RSRC_PURPOSE,
-                                                        TEST_CONN_RSRC) == 1);
-  tt_assert(connection_dir_avoid_extra_connection_for_purpose(
-                                                 TEST_CONN_RSRC_PURPOSE) == 1);
+                                                         other_res) == 0);
+
+  /* more connections, two downloading (should never happen, but needs
+   * to be tested for completeness) */
+  conn2->base_.state = TEST_CONN_DL_STATE;
+  ap_conn2 = test_conn_get_linked_connection(TO_CONN(conn2),
+                                             TEST_CONN_ATTACHED_STATE);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 3);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
+  conn->base_.state = TEST_CONN_STATE;
+
+  /* more connections, a different one downloading */
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 3);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                         other_res) == 0);
+
+  /* a connection for the other flavor (could happen if a client is set to
+   * cache directory documents), one preferred flavor downloading
+   */
+  conn4 = test_conn_download_status_add_a_connection(other_res);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 3);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        other_res) == 1);
+
+  /* a connection for the other flavor (could happen if a client is set to
+   * cache directory documents), both flavors downloading
+   */
+  conn4->base_.state = TEST_CONN_DL_STATE;
+  ap_conn4 = test_conn_get_linked_connection(TO_CONN(conn4),
+                                             TEST_CONN_ATTACHED_STATE);
+  tt_assert(networkstatus_consensus_is_already_downloading(res) == 1);
+  tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 1);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        res) == 3);
+  tt_assert(connection_dir_count_by_purpose_and_resource(
+                                                        TEST_CONN_RSRC_PURPOSE,
+                                                        other_res) == 1);
+  conn4->base_.state = TEST_CONN_STATE;
 
  done:
   /* the teardown function removes all the connections */;
@@ -750,11 +842,18 @@ test_conn_download_status(void *arg)
 #define CONNECTION_TESTCASE(name, fork, setup)                           \
   { #name, test_conn_##name, fork, &setup, NULL }
 
+/* where arg is an expression (constant, varaible, compound expression) */
+#define CONNECTION_TESTCASE_ARG(name, fork, setup, arg)                  \
+  { #name "_" #arg, test_conn_##name, fork, &setup, (void *)arg }
+
 struct testcase_t connection_tests[] = {
   CONNECTION_TESTCASE(get_basic, TT_FORK, test_conn_get_basic_st),
   CONNECTION_TESTCASE(get_rend,  TT_FORK, test_conn_get_rend_st),
   CONNECTION_TESTCASE(get_rsrc,  TT_FORK, test_conn_get_rsrc_st),
-  CONNECTION_TESTCASE(download_status,  TT_FORK, test_conn_download_status_st),
+  CONNECTION_TESTCASE_ARG(download_status,  TT_FORK,
+                          test_conn_download_status_st, FLAV_MICRODESC),
+  CONNECTION_TESTCASE_ARG(download_status,  TT_FORK,
+                          test_conn_download_status_st, FLAV_NS),
 //CONNECTION_TESTCASE(func_suffix, TT_FORK, setup_func_pair),
   END_OF_TESTCASES
 };

src/test/test_dir.c

@@ -4042,12 +4042,56 @@ test_dir_choose_compression_level(void* data)
   done: ;
 }
 
+static int mock_networkstatus_consensus_is_bootstrapping_value = 0;
+static int
+mock_networkstatus_consensus_is_bootstrapping(time_t now)
+{
+  (void)now;
+  return mock_networkstatus_consensus_is_bootstrapping_value;
+}
+
+static int mock_networkstatus_consensus_can_use_extra_fallbacks_value = 0;
+static int
+mock_networkstatus_consensus_can_use_extra_fallbacks(
+                                                  const or_options_t *options)
+{
+  (void)options;
+  return mock_networkstatus_consensus_can_use_extra_fallbacks_value;
+}
+
+/* data is a 2 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
+ * fallbacks.
+ */
 static void
 test_dir_find_dl_schedule(void* data)
 {
+  const char *str = (const char *)data;
+
+  tt_assert(strlen(data) == 2);
+
+  if (str[0] == 'b') {
+    mock_networkstatus_consensus_is_bootstrapping_value = 1;
+  } else {
+    mock_networkstatus_consensus_is_bootstrapping_value = 0;
+  }
+
+  if (str[1] == 'f') {
+    mock_networkstatus_consensus_can_use_extra_fallbacks_value = 1;
+  } else {
+    mock_networkstatus_consensus_can_use_extra_fallbacks_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);
+
   download_status_t dls;
-  smartlist_t server, client, server_cons, client_cons, bridge;
-  (void)data;
+  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;
 
   mock_options = malloc(sizeof(or_options_t));
   reset_options(mock_options, &mock_get_options_calls);
@@ -4057,43 +4101,121 @@ test_dir_find_dl_schedule(void* data)
   mock_options->TestingClientDownloadSchedule = &client;
   mock_options->TestingServerConsensusDownloadSchedule = &server_cons;
   mock_options->TestingClientConsensusDownloadSchedule = &client_cons;
+  mock_options->ClientBootstrapConsensusAuthorityOnlyDownloadSchedule =
+    &client_boot_auth_only_cons;
+  mock_options->ClientBootstrapConsensusAuthorityDownloadSchedule =
+    &client_boot_auth_cons;
+  mock_options->ClientBootstrapConsensusFallbackDownloadSchedule =
+    &client_boot_fallback_cons;
   mock_options->TestingBridgeDownloadSchedule = &bridge;
 
   dls.schedule = DL_SCHED_GENERIC;
+  /* client */
   mock_options->ClientOnly = 1;
   tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &client);
   mock_options->ClientOnly = 0;
+
+  /* dir mode */
   mock_options->DirPort_set = 1;
-  mock_options->ORPort_set = 1;
   mock_options->DirCache = 1;
   tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &server);
+  mock_options->DirPort_set = 0;
+  mock_options->DirCache = 0;
 
-#if 0
   dls.schedule = DL_SCHED_CONSENSUS;
-  mock_options->ClientOnly = 1;
-  mock_options->DirCache = 0;
-  tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &client_cons);
-  mock_options->ClientOnly = 0;
-  mock_options->DirCache = 1;
+  /* public server mode */
+  mock_options->ORPort_set = 1;
   tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &server_cons);
-#endif
+  mock_options->ORPort_set = 0;
+
+  /* client and bridge modes */
+  if (networkstatus_consensus_is_bootstrapping(time(NULL))) {
+    if (networkstatus_consensus_can_use_extra_fallbacks(mock_options)) {
+      dls.want_authority = 1;
+      /* client */
+      mock_options->ClientOnly = 1;
+      tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+                &client_boot_auth_cons);
+      mock_options->ClientOnly = 0;
+
+      /* bridge relay */
+      mock_options->ORPort_set = 1;
+      mock_options->BridgeRelay = 1;
+      tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+                &client_boot_auth_cons);
+      mock_options->ORPort_set = 0;
+      mock_options->BridgeRelay = 0;
+
+      dls.want_authority = 0;
+      /* client */
+      mock_options->ClientOnly = 1;
+      tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+                &client_boot_fallback_cons);
+      mock_options->ClientOnly = 0;
+
+      /* bridge relay */
+      mock_options->ORPort_set = 1;
+      mock_options->BridgeRelay = 1;
+      tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+                &client_boot_fallback_cons);
+      mock_options->ORPort_set = 0;
+      mock_options->BridgeRelay = 0;
+
+    } else {
+      /* dls.want_authority is ignored */
+      /* client */
+      mock_options->ClientOnly = 1;
+      tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+                &client_boot_auth_only_cons);
+      mock_options->ClientOnly = 0;
+
+      /* bridge relay */
+      mock_options->ORPort_set = 1;
+      mock_options->BridgeRelay = 1;
+      tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+                &client_boot_auth_only_cons);
+      mock_options->ORPort_set = 0;
+      mock_options->BridgeRelay = 0;
+    }
+  } else {
+    /* client */
+    mock_options->ClientOnly = 1;
+    tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+              &client_cons);
+    mock_options->ClientOnly = 0;
+
+    /* bridge relay */
+    mock_options->ORPort_set = 1;
+    mock_options->BridgeRelay = 1;
+    tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ,
+              &client_cons);
+    mock_options->ORPort_set = 0;
+    mock_options->BridgeRelay = 0;
+  }
 
   dls.schedule = DL_SCHED_BRIDGE;
+  /* client */
   mock_options->ClientOnly = 1;
   tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge);
-  mock_options->ClientOnly = 0;
-  tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge);
 
  done:
+  UNMOCK(networkstatus_consensus_is_bootstrapping);
+  UNMOCK(networkstatus_consensus_can_use_extra_fallbacks);
   UNMOCK(get_options);
+  free(mock_options);
+  mock_options = NULL;
 }
 
-#define DIR_LEGACY(name)                                                   \
+#define DIR_LEGACY(name)                             \
   { #name, test_dir_ ## name , TT_FORK, NULL, NULL }
 
 #define DIR(name,flags)                              \
   { #name, test_dir_##name, (flags), NULL, NULL }
 
+/* where arg is a string constant */
+#define DIR_ARG(name,flags,arg)                      \
+  { #name "_" arg, test_dir_##name, (flags), &passthrough_setup, arg }
+
 struct testcase_t dir_tests[] = {
   DIR_LEGACY(nicknames),
   DIR_LEGACY(formats),
@@ -4128,7 +4250,10 @@ struct testcase_t dir_tests[] = {
   DIR(should_not_init_request_to_dir_auths_without_v3_info, 0),
   DIR(should_init_request_to_dir_auths, 0),
   DIR(choose_compression_level, 0),
-  DIR(find_dl_schedule, 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"),
   END_OF_TESTCASES
 };
 

src/test/test_routerlist.c

@@ -15,6 +15,7 @@
 #include "container.h"
 #include "directory.h"
 #include "dirvote.h"
+#include "microdesc.h"
 #include "networkstatus.h"
 #include "nodelist.h"
 #include "policies.h"
@@ -190,6 +191,14 @@ construct_consensus(char **consensus_text_md)
   crypto_pk_free(sign_skey_leg);
 }
 
+static int mock_usable_consensus_flavor_value = FLAV_NS;
+
+static int
+mock_usable_consensus_flavor(void)
+{
+  return mock_usable_consensus_flavor_value;
+}
+
 static void
 test_router_pick_directory_server_impl(void *arg)
 {
@@ -209,6 +218,22 @@ test_router_pick_directory_server_impl(void *arg)
 
   (void)arg;
 
+  MOCK(usable_consensus_flavor, mock_usable_consensus_flavor);
+
+  /* With no consensus, we must be bootstrapping, regardless of time or flavor
+   */
+  mock_usable_consensus_flavor_value = FLAV_NS;
+  tt_assert(networkstatus_consensus_is_bootstrapping(now));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2000));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60));
+
+  mock_usable_consensus_flavor_value = FLAV_MICRODESC;
+  tt_assert(networkstatus_consensus_is_bootstrapping(now));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2000));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60));
+
   /* No consensus available, fail early */
   rs = router_pick_directory_server_impl(V3_DIRINFO, (const int) 0, NULL);
   tt_assert(rs == NULL);
@@ -223,6 +248,28 @@ test_router_pick_directory_server_impl(void *arg)
   tt_int_op(smartlist_len(con_md->routerstatus_list), ==, 3);
   tt_assert(!networkstatus_set_current_consensus_from_ns(con_md,
                                                  "microdesc"));
+
+  /* If the consensus time or flavor doesn't match, we are still
+   * bootstrapping */
+  mock_usable_consensus_flavor_value = FLAV_NS;
+  tt_assert(networkstatus_consensus_is_bootstrapping(now));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2000));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60));
+
+  /* With a valid consensus for the current time and flavor, we stop
+   * bootstrapping, even if we have no certificates */
+  mock_usable_consensus_flavor_value = FLAV_MICRODESC;
+  tt_assert(!networkstatus_consensus_is_bootstrapping(now + 2000));
+  tt_assert(!networkstatus_consensus_is_bootstrapping(con_md->valid_after));
+  tt_assert(!networkstatus_consensus_is_bootstrapping(con_md->valid_until));
+  tt_assert(!networkstatus_consensus_is_bootstrapping(con_md->valid_until
+                                                      + 24*60*60));
+  /* These times are outside the test validity period */
+  tt_assert(networkstatus_consensus_is_bootstrapping(now));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60));
+  tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60));
+
   nodelist_set_consensus(con_md);
   nodelist_assert_ok();
 
@@ -362,6 +409,7 @@ test_router_pick_directory_server_impl(void *arg)
   node_router1->rs->last_dir_503_at = 0;
 
  done:
+  UNMOCK(usable_consensus_flavor);
   if (router1_id)
     tor_free(router1_id);
   if (router2_id)