Prop210: Close excess connections once a consensus is downloading

Once tor is downloading a usable consensus, any other connection
attempts are not needed.

Choose a connection to keep, favouring:
* fallback directories over authorities,
* connections initiated earlier over later connections

Close all other connections downloading a consensus.

5 files changed, 260 insertions, 2 deletions

diff --git a/src/or/directory.c b/src/or/directory.c
index 0d2a8b2459..63bbdafd3f 100644
--- a/src/or/directory.c
+++ b/src/or/directory.c
@@ -961,6 +961,12 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
     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 */
@@ -1001,6 +1007,9 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
         conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
         /* fall through */
       case 0:
+        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,
@@ -1044,6 +1053,9 @@ directory_initiate_command_rend(const tor_addr_t *_addr,
       connection_mark_for_close(TO_CONN(conn));
       return;
     }
+    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,
@@ -3426,8 +3438,205 @@ 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. */
+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_boostrapping(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_boostrapping(time(NULL))) {
+    return 1;
+  }
+
+  return 0;
+}
+
+/* Check if we have excess consensus download connection attempts, and close
+ * conn:
+ * - if we don't have a consensus, and we're downloading a consensus, and conn
+ *   is not downloading a consensus yet, close it;
+ * - if we do have a consensus, conn is excess, close it. */
+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;
+  }
+
+  if (!connection_dir_would_close_consensus_conn_helper()) {
+    return 0;
+  }
+
+  const int we_are_bootstrapping = networkstatus_consensus_is_boostrapping(
+                                                                  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_consider_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;
+}
+
+/* Check if we have excess consensus download connection attempts, and close
+ * them:
+ * - 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 do have a consensus, close all connections: they are all excess. */
+void
+connection_dir_close_extra_consensus_conns(void)
+{
+  if (!connection_dir_would_close_consensus_conn_helper()) {
+    return;
+  }
+
+  int we_are_bootstrapping = networkstatus_consensus_is_boostrapping(
+                                                                  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)
+      continue;
+    /* mark all other connections for close */
+    if (!d->base_.marked_for_close) {
+      connection_close_immediate(&d->base_);
+      connection_mark_for_close(&d->base_);
+    }
+  } 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);
+  }
+}
+
 /** Connected handler for directory connections: begin sending data to the
- * server */
+ * server, and return 0, or, if the connection is an excess bootstrap
+ * connection, close all excess bootstrap connections.
+ * Only used when connections don't immediately connect. */
 int
 connection_dir_finished_connecting(dir_connection_t *conn)
 {
@@ -3438,7 +3647,12 @@ 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);
 
-  conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING; /* start flushing conn */
+  if (connection_dir_close_consensus_conn_if_extra(conn)) {
+    return -1;
+  }
+
+  /* start flushing conn */
+  conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING;
   return 0;
 }
 
diff --git a/src/or/directory.h b/src/or/directory.h
index 4255868d3a..37f9ab0815 100644
--- a/src/or/directory.h
+++ b/src/or/directory.h
@@ -74,6 +74,9 @@ void directory_initiate_command(const tor_addr_t *addr,
                                 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)
@@ -139,6 +142,7 @@ STATIC int directory_handle_command_get(dir_connection_t *conn,
                                         const char *headers,
                                         const char *req_body,
                                         size_t req_body_len);
+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);
diff --git a/src/or/main.c b/src/or/main.c
index 60957bd6ab..455cba4513 100644
--- a/src/or/main.c
+++ b/src/or/main.c
@@ -1460,6 +1460,11 @@ 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() will close connections
+   * that are clearly excess, but this check is more thorough. */
+  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
diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c
index 1d5b2f2723..173c109d60 100644
--- a/src/or/networkstatus.c
+++ b/src/or/networkstatus.c
@@ -1310,6 +1310,40 @@ 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_boostrapping(). */
+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? */
 int
 networkstatus_consensus_is_downloading_usable_flavor(void)
diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h
index d44022c80c..4cb33c3fc0 100644
--- a/src/or/networkstatus.h
+++ b/src/or/networkstatus.h
@@ -75,6 +75,7 @@ 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);
 
 #define NSSET_FROM_CACHE 1