From ce11e3bf6946d1659e5915abac30b1972fc799c8 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 17:12:02 +1000
Subject: [PATCH 01/14] nodelist: Move the single-hop exit check

Check for single-hop exits in router_add_running_nodes_to_smartlist(),
rather than router_choose_random_node().

Part of 34200.
---
 src/feature/nodelist/node_select.c | 7 +------
 src/feature/nodelist/routerlist.c  | 5 +++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index fe88b7230a..ce07c450e8 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -985,12 +985,7 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
     (need_guard ? WEIGHT_FOR_GUARD : WEIGHT_FOR_MID);
 
   SMARTLIST_FOREACH_BEGIN(node_list, const node_t *, node) {
-    if (node_allows_single_hop_exits(node)) {
-      /* Exclude relays that allow single hop exit circuits. This is an
-       * obsolete option since 0.2.9.2-alpha and done by default in
-       * 0.3.1.0-alpha. */
-      smartlist_add(excludednodes, (node_t*)node);
-    } else if (rendezvous_v3 &&
+    if (rendezvous_v3 &&
                !node_supports_v3_rendezvous_point(node)) {
       /* Exclude relays that can not become a rendezvous for a hidden service
        * version 3. */
diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c
index 0d3d1bea3e..c18051d416 100644
--- a/src/feature/nodelist/routerlist.c
+++ b/src/feature/nodelist/routerlist.c
@@ -541,6 +541,11 @@ router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
     /* Don't choose nodes if we are certain they can't do ntor. */
     if ((node->ri || node->md) && !node_has_curve25519_onion_key(node))
       continue;
+    /* Exclude relays that allow single hop exit circuits. This is an
+     * obsolete option since 0.2.9.2-alpha and done by default in
+     * 0.3.1.0-alpha. */
+    if (node_allows_single_hop_exits(node))
+      continue;
     /* Choose a node with an OR address that matches the firewall rules */
     if (direct_conn && check_reach &&
         !fascist_firewall_allows_node(node,

From 280195f41471862964f5c47446e5ccd01afdd96b Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 17:21:47 +1000
Subject: [PATCH 02/14] nodelist: Move the v3 onion service rendezvous check

And delete a loop that is now empty. This change should improve tor's
performance, because we no longer iterate through the nodelist twice for
every node in every circuit path.

Part of 34200.
---
 src/core/or/circuitbuild.c         |  1 +
 src/feature/nodelist/node_select.c | 13 ++-----------
 src/feature/nodelist/routerlist.c  |  6 ++++++
 src/feature/nodelist/routerlist.h  |  1 +
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index 4dc3067a76..d7b07c4903 100644
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -1802,6 +1802,7 @@ pick_restricted_middle_node(router_crn_flags_t flags,
                                     (flags & CRN_NEED_DESC) != 0,
                                     (flags & CRN_PREF_ADDR) != 0,
                                     (flags & CRN_DIRECT_CONN) != 0,
+                                    (flags & CRN_RENDEZVOUS_V3) != 0,
                                     (flags & CRN_INITIATE_IPV6_EXTEND) != 0);
 
   /* Filter all_live_nodes to only add live *and* whitelisted middles
diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index ce07c450e8..7015063d05 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -973,7 +973,6 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   const int rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
   const bool initiate_ipv6_extend = (flags & CRN_INITIATE_IPV6_EXTEND) != 0;
 
-  const smartlist_t *node_list = nodelist_get_list();
   smartlist_t *sl=smartlist_new(),
     *excludednodes=smartlist_new();
   const node_t *choice = NULL;
@@ -984,15 +983,6 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   rule = weight_for_exit ? WEIGHT_FOR_EXIT :
     (need_guard ? WEIGHT_FOR_GUARD : WEIGHT_FOR_MID);
 
-  SMARTLIST_FOREACH_BEGIN(node_list, const node_t *, node) {
-    if (rendezvous_v3 &&
-               !node_supports_v3_rendezvous_point(node)) {
-      /* Exclude relays that can not become a rendezvous for a hidden service
-       * version 3. */
-      smartlist_add(excludednodes, (node_t*)node);
-    }
-  } SMARTLIST_FOREACH_END(node);
-
   /* If the node_t is not found we won't be to exclude ourself but we
    * won't be able to pick ourself in router_choose_random_node() so
    * this is fine to at least try with our routerinfo_t object. */
@@ -1001,7 +991,8 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
 
   router_add_running_nodes_to_smartlist(sl, need_uptime, need_capacity,
                                         need_guard, need_desc, pref_addr,
-                                        direct_conn, initiate_ipv6_extend);
+                                        direct_conn, rendezvous_v3,
+                                        initiate_ipv6_extend);
   log_debug(LD_CIRC,
            "We found %d running nodes.",
             smartlist_len(sl));
diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c
index c18051d416..d4dfffa1ab 100644
--- a/src/feature/nodelist/routerlist.c
+++ b/src/feature/nodelist/routerlist.c
@@ -520,6 +520,7 @@ router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
                                       int need_capacity, int need_guard,
                                       int need_desc, int pref_addr,
                                       int direct_conn,
+                                      bool rendezvous_v3,
                                       bool initiate_ipv6_extend)
 {
   const int check_reach = !router_or_conn_should_skip_reachable_address_check(
@@ -546,6 +547,11 @@ router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
      * 0.3.1.0-alpha. */
     if (node_allows_single_hop_exits(node))
       continue;
+    /* Exclude relays that can not become a rendezvous for a hidden service
+     * version 3. */
+    if (rendezvous_v3 &&
+        !node_supports_v3_rendezvous_point(node))
+      continue;
     /* Choose a node with an OR address that matches the firewall rules */
     if (direct_conn && check_reach &&
         !fascist_firewall_allows_node(node,
diff --git a/src/feature/nodelist/routerlist.h b/src/feature/nodelist/routerlist.h
index 1297bb4b65..d6d6064281 100644
--- a/src/feature/nodelist/routerlist.h
+++ b/src/feature/nodelist/routerlist.h
@@ -62,6 +62,7 @@ void router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
                                            int need_capacity, int need_guard,
                                            int need_desc, int pref_addr,
                                            int direct_conn,
+                                           bool rendezvous_v3,
                                            bool initiate_ipv6_extend);
 
 const routerinfo_t *routerlist_find_my_routerinfo(void);

From 1ec604f0f9dc7f642e5614de4741671105d6945f Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 17:31:20 +1000
Subject: [PATCH 03/14] nodelist: Move node flag checks

Move node flag checks to router_add_running_nodes_to_smartlist(), where
they are actually used.

Part of 34200.
---
 src/core/or/circuitbuild.c         | 10 +------
 src/feature/nodelist/node_select.c | 38 ++++-------------------
 src/feature/nodelist/routerlist.c  | 48 +++++++++++++++++++++++-------
 src/feature/nodelist/routerlist.h  |  7 +----
 4 files changed, 46 insertions(+), 57 deletions(-)

diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index d7b07c4903..b840ca7a7d 100644
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -1795,15 +1795,7 @@ pick_restricted_middle_node(router_crn_flags_t flags,
   tor_assert(pick_from);
 
   /* Add all running nodes to all_live_nodes */
-  router_add_running_nodes_to_smartlist(all_live_nodes,
-                                    (flags & CRN_NEED_UPTIME) != 0,
-                                    (flags & CRN_NEED_CAPACITY) != 0,
-                                    (flags & CRN_NEED_GUARD) != 0,
-                                    (flags & CRN_NEED_DESC) != 0,
-                                    (flags & CRN_PREF_ADDR) != 0,
-                                    (flags & CRN_DIRECT_CONN) != 0,
-                                    (flags & CRN_RENDEZVOUS_V3) != 0,
-                                    (flags & CRN_INITIATE_IPV6_EXTEND) != 0);
+  router_add_running_nodes_to_smartlist(all_live_nodes, flags);
 
   /* Filter all_live_nodes to only add live *and* whitelisted middles
    * to the list whitelisted_live_middles. */
diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index 7015063d05..66665afbad 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -934,44 +934,21 @@ nodelist_subtract(smartlist_t *sl, const smartlist_t *excluded)
  * in <b>excludedsmartlist</b>, or which matches <b>excludedset</b>, even if
  * they are the only nodes available.
  *
- * If the following <b>flags</b> are set:
- *  - <b>CRN_NEED_UPTIME</b>: if any router has more than a minimum uptime,
- *                            return one of those;
- *  - <b>CRN_NEED_CAPACITY</b>: weight your choice by the advertised capacity
- *                              of each router;
- *  - <b>CRN_NEED_GUARD</b>: only consider Guard routers;
- *  - <b>CRN_WEIGHT_AS_EXIT</b>: we weight bandwidths as if picking an exit
- *                               node, otherwise we weight bandwidths for
- *                               picking a relay node (that is, possibly
- *                               discounting exit nodes);
- *  - <b>CRN_NEED_DESC</b>: only consider nodes that have a routerinfo or
- *                          microdescriptor -- that is, enough info to be
- *                          used to build a circuit;
- *  - <b>CRN_DIRECT_CONN</b>: only consider nodes that are suitable for direct
- *                            connections. Check ReachableAddresses,
- *                            ClientUseIPv4 0, and
- *                            fascist_firewall_use_ipv6() == 0);
- *  - <b>CRN_PREF_ADDR</b>: only consider nodes that have an address that is
- *                          preferred by the ClientPreferIPv6ORPort setting.
- *  - <b>CRN_RENDEZVOUS_V3</b>: only consider nodes that can become v3 onion
- *                              service rendezvous points.
- *  - <b>CRN_INITIATE_IPV6_EXTEND</b>: only consider routers than can initiate
- *                                     IPv6 extends.
+ * <b>flags</b> is a set of CRN_* flags, see
+ * router_add_running_nodes_to_smartlist() for details.
  */
 const node_t *
 router_choose_random_node(smartlist_t *excludedsmartlist,
                           routerset_t *excludedset,
                           router_crn_flags_t flags)
-{ /* XXXX MOVE */
+{
+  /* A limited set of flags, used for weighting and fallback node selection.
+   */
   const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
   const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
   const int need_guard = (flags & CRN_NEED_GUARD) != 0;
   const int weight_for_exit = (flags & CRN_WEIGHT_AS_EXIT) != 0;
-  const int need_desc = (flags & CRN_NEED_DESC) != 0;
   const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
-  const int direct_conn = (flags & CRN_DIRECT_CONN) != 0;
-  const int rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
-  const bool initiate_ipv6_extend = (flags & CRN_INITIATE_IPV6_EXTEND) != 0;
 
   smartlist_t *sl=smartlist_new(),
     *excludednodes=smartlist_new();
@@ -989,10 +966,7 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   if ((r = router_get_my_routerinfo()))
     routerlist_add_node_and_family(excludednodes, r);
 
-  router_add_running_nodes_to_smartlist(sl, need_uptime, need_capacity,
-                                        need_guard, need_desc, pref_addr,
-                                        direct_conn, rendezvous_v3,
-                                        initiate_ipv6_extend);
+  router_add_running_nodes_to_smartlist(sl, flags);
   log_debug(LD_CIRC,
            "We found %d running nodes.",
             smartlist_len(sl));
diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c
index d4dfffa1ab..1106222830 100644
--- a/src/feature/nodelist/routerlist.c
+++ b/src/feature/nodelist/routerlist.c
@@ -512,21 +512,49 @@ routers_have_same_or_addrs(const routerinfo_t *r1, const routerinfo_t *r2)
 }
 
 /** Add every suitable node from our nodelist to <b>sl</b>, so that
- * we can pick a node for a circuit. See router_choose_random_node()
- * for details.
+ * we can pick a node for a circuit.
+ *
+ * If the following <b>flags</b> are set:
+ *  - <b>CRN_NEED_UPTIME</b>: if any router has more than a minimum uptime,
+ *                            return one of those;
+ *  - <b>CRN_NEED_CAPACITY</b>: weight your choice by the advertised capacity
+ *                              of each router;
+ *  - <b>CRN_NEED_GUARD</b>: only consider Guard routers;
+ *  - <b>CRN_WEIGHT_AS_EXIT</b>: we weight bandwidths as if picking an exit
+ *                               node, otherwise we weight bandwidths for
+ *                               picking a relay node (that is, possibly
+ *                               discounting exit nodes);
+ *  - <b>CRN_NEED_DESC</b>: only consider nodes that have a routerinfo or
+ *                          microdescriptor -- that is, enough info to be
+ *                          used to build a circuit;
+ *  - <b>CRN_DIRECT_CONN</b>: only consider nodes that are suitable for direct
+ *                            connections. Check ReachableAddresses,
+ *                            ClientUseIPv4 0, and
+ *                            fascist_firewall_use_ipv6() == 0);
+ *  - <b>CRN_PREF_ADDR</b>: only consider nodes that have an address that is
+ *                          preferred by the ClientPreferIPv6ORPort setting.
+ *  - <b>CRN_RENDEZVOUS_V3</b>: only consider nodes that can become v3 onion
+ *                              service rendezvous points.
+ *  - <b>CRN_INITIATE_IPV6_EXTEND</b>: only consider routers than can initiate
+ *                                     IPv6 extends.
  */
 void
-router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
-                                      int need_capacity, int need_guard,
-                                      int need_desc, int pref_addr,
-                                      int direct_conn,
-                                      bool rendezvous_v3,
-                                      bool initiate_ipv6_extend)
-{
+router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags)
+{
+  /* The full set of flags used for node selection. */
+  const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
+  const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
+  const int need_guard = (flags & CRN_NEED_GUARD) != 0;
+  const int need_desc = (flags & CRN_NEED_DESC) != 0;
+  const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
+  const int direct_conn = (flags & CRN_DIRECT_CONN) != 0;
+  const int rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
+  const bool initiate_ipv6_extend = (flags & CRN_INITIATE_IPV6_EXTEND) != 0;
+
   const int check_reach = !router_or_conn_should_skip_reachable_address_check(
                                                        get_options(),
                                                        pref_addr);
-  /* XXXX MOVE */
+
   SMARTLIST_FOREACH_BEGIN(nodelist_get_list(), const node_t *, node) {
     if (!node->is_running || !node->is_valid)
       continue;
diff --git a/src/feature/nodelist/routerlist.h b/src/feature/nodelist/routerlist.h
index d6d6064281..7d0788bacb 100644
--- a/src/feature/nodelist/routerlist.h
+++ b/src/feature/nodelist/routerlist.h
@@ -58,12 +58,7 @@ int router_dir_conn_should_skip_reachable_address_check(
                                        int try_ip_pref);
 void router_reset_status_download_failures(void);
 int routers_have_same_or_addrs(const routerinfo_t *r1, const routerinfo_t *r2);
-void router_add_running_nodes_to_smartlist(smartlist_t *sl, int need_uptime,
-                                           int need_capacity, int need_guard,
-                                           int need_desc, int pref_addr,
-                                           int direct_conn,
-                                           bool rendezvous_v3,
-                                           bool initiate_ipv6_extend);
+void router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags);
 
 const routerinfo_t *routerlist_find_my_routerinfo(void);
 uint32_t router_get_advertised_bandwidth(const routerinfo_t *router);

From 48413dc65f0d84b9d7c9566d1ecda359a1210dda Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 17:35:41 +1000
Subject: [PATCH 04/14] nodelist: Remove the unused CRN_WEIGHT_FOR_EXIT flag

Part of 34200.
---
 src/feature/nodelist/node_select.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index 66665afbad..11abc84d4b 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -947,7 +947,6 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
   const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
   const int need_guard = (flags & CRN_NEED_GUARD) != 0;
-  const int weight_for_exit = (flags & CRN_WEIGHT_AS_EXIT) != 0;
   const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
 
   smartlist_t *sl=smartlist_new(),
@@ -956,9 +955,7 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   const routerinfo_t *r;
   bandwidth_weight_rule_t rule;
 
-  tor_assert(!(weight_for_exit && need_guard));
-  rule = weight_for_exit ? WEIGHT_FOR_EXIT :
-    (need_guard ? WEIGHT_FOR_GUARD : WEIGHT_FOR_MID);
+  rule = (need_guard ? WEIGHT_FOR_GUARD : WEIGHT_FOR_MID);
 
   /* If the node_t is not found we won't be to exclude ourself but we
    * won't be able to pick ourself in router_choose_random_node() so

From 2ea1692c20b4afa929e9f1c6da16ee2fb134abe0 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 17:42:05 +1000
Subject: [PATCH 05/14] nodelist: Rewrite router_crn_flags_t

Re-order the flags in a logical order, and re-number them.
Add missing comments, fix comment typos.

Part of 34200.
---
 src/feature/nodelist/node_select.h | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/src/feature/nodelist/node_select.h b/src/feature/nodelist/node_select.h
index 29b6e6f2c8..1776d8ea1a 100644
--- a/src/feature/nodelist/node_select.h
+++ b/src/feature/nodelist/node_select.h
@@ -14,22 +14,26 @@
 /** Flags to be passed to control router_choose_random_node() to indicate what
  * kind of nodes to pick according to what algorithm. */
 typedef enum router_crn_flags_t {
+  /* Try to choose stable nodes. */
   CRN_NEED_UPTIME = 1<<0,
+  /* Try to choose nodes with a reasonable amount of bandwidth. */
   CRN_NEED_CAPACITY = 1<<1,
-  CRN_NEED_GUARD = 1<<2,
-  /* XXXX not used, apparently. */
-  CRN_WEIGHT_AS_EXIT = 1<<5,
-  CRN_NEED_DESC = 1<<6,
-  /* On clients, only provide nodes that satisfy ClientPreferIPv6OR */
-  CRN_PREF_ADDR = 1<<7,
+  /* Only choose nodes if we have downloaded their descriptor or
+   * microdescriptor. */
+  CRN_NEED_DESC = 1<<2,
+  /* Choose nodes that can be used as Guard relays. */
+  CRN_NEED_GUARD = 1<<3,
   /* On clients, only provide nodes that we can connect to directly, based on
-   * our firewall rules */
-  CRN_DIRECT_CONN = 1<<8,
-  /* On clients, only provide nodes with HSRend >= 2 protocol version which
-   * is required for hidden service version >= 3. */
-  CRN_RENDEZVOUS_V3 = 1<<9,
+   * our firewall rules. */
+  CRN_DIRECT_CONN = 1<<4,
+  /* On clients, if choosing a node for a direct connection, only provide
+   * nodes that satisfy ClientPreferIPv6OR. */
+  CRN_PREF_ADDR = 1<<5,
+  /* On clients, only provide nodes with HSRend=2 protocol version which
+   * is required for hidden service version 3. */
+  CRN_RENDEZVOUS_V3 = 1<<6,
   /* On clients, only provide nodes that can initiate IPv6 extends. */
-  CRN_INITIATE_IPV6_EXTEND = 1<<10,
+  CRN_INITIATE_IPV6_EXTEND = 1<<7,
 } router_crn_flags_t;
 
 /** Possible ways to weight routers when choosing one randomly.  See

From 3f7f976d4812a92bdbf5f14e25db0d276f123cef Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 17:58:28 +1000
Subject: [PATCH 06/14] nodelist: Stop recursing in router_choose_random_node()

Instead, call out to a helper function, repeating the call if needed.

Avoids duplicating exclusions for:
  * the current relay's family, and
  * any exclusions specified by the caller.

Part of 34200.
---
 src/feature/nodelist/node_select.c | 69 ++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 22 deletions(-)

diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index 11abc84d4b..85ef5ad568 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -930,6 +930,42 @@ nodelist_subtract(smartlist_t *sl, const smartlist_t *excluded)
   bitarray_free(excluded_idx);
 }
 
+/* Node selection helper for router_choose_random_node().
+ *
+ * Populates a node list based on <b>flags</b>, ignoring nodes in
+ * <b>excludednodes</b> and <b>excludedset</b>. Chooses the node based on
+ * <b>rule</b>. */
+static const node_t *
+router_choose_random_node_helper(smartlist_t *excludednodes,
+                                 routerset_t *excludedset,
+                                 router_crn_flags_t flags,
+                                 bandwidth_weight_rule_t rule)
+{
+  smartlist_t *sl=smartlist_new();
+  const node_t *choice = NULL;
+
+  router_add_running_nodes_to_smartlist(sl, flags);
+  log_debug(LD_CIRC,
+           "We found %d running nodes.",
+            smartlist_len(sl));
+
+  nodelist_subtract(sl, excludednodes);
+
+  if (excludedset) {
+    routerset_subtract_nodes(sl,excludedset);
+    log_debug(LD_CIRC,
+              "We removed excludedset, leaving %d nodes.",
+              smartlist_len(sl));
+  }
+
+  // Always weight by bandwidth
+  choice = node_sl_choose_by_bandwidth(sl, rule);
+
+  smartlist_free(sl);
+
+  return choice;
+}
+
 /** Return a random running node from the nodelist. Never pick a node that is
  * in <b>excludedsmartlist</b>, or which matches <b>excludedset</b>, even if
  * they are the only nodes available.
@@ -942,15 +978,14 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
                           routerset_t *excludedset,
                           router_crn_flags_t flags)
 {
-  /* A limited set of flags, used for weighting and fallback node selection.
+  /* A limited set of flags, used for fallback node selection.
    */
   const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
   const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
   const int need_guard = (flags & CRN_NEED_GUARD) != 0;
   const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
 
-  smartlist_t *sl=smartlist_new(),
-    *excludednodes=smartlist_new();
+  smartlist_t *excludednodes=smartlist_new();
   const node_t *choice = NULL;
   const routerinfo_t *r;
   bandwidth_weight_rule_t rule;
@@ -963,29 +998,17 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
   if ((r = router_get_my_routerinfo()))
     routerlist_add_node_and_family(excludednodes, r);
 
-  router_add_running_nodes_to_smartlist(sl, flags);
-  log_debug(LD_CIRC,
-           "We found %d running nodes.",
-            smartlist_len(sl));
-
   if (excludedsmartlist) {
     smartlist_add_all(excludednodes, excludedsmartlist);
   }
-  nodelist_subtract(sl, excludednodes);
 
-  if (excludedset) {
-    routerset_subtract_nodes(sl,excludedset);
-    log_debug(LD_CIRC,
-              "We removed excludedset, leaving %d nodes.",
-              smartlist_len(sl));
-  }
+  choice = router_choose_random_node_helper(excludednodes,
+                                            excludedset,
+                                            flags,
+                                            rule);
 
-  // Always weight by bandwidth
-  choice = node_sl_choose_by_bandwidth(sl, rule);
-
-  smartlist_free(sl);
   if (!choice && (need_uptime || need_capacity || need_guard || pref_addr)) {
-    /* try once more -- recurse but with fewer restrictions. */
+    /* try once more, with fewer restrictions. */
     log_info(LD_CIRC,
              "We couldn't find any live%s%s%s%s routers; falling back "
              "to list of all routers.",
@@ -995,8 +1018,10 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
              pref_addr?", preferred address":"");
     flags &= ~ (CRN_NEED_UPTIME|CRN_NEED_CAPACITY|CRN_NEED_GUARD|
                 CRN_PREF_ADDR);
-    choice = router_choose_random_node(
-                     excludedsmartlist, excludedset, flags);
+    choice = router_choose_random_node_helper(excludednodes,
+                                              excludedset,
+                                              flags,
+                                              rule);
   }
   smartlist_free(excludednodes);
   if (!choice) {

From a3244c03fb586268da72fbe90bc8f4def85aaff7 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 18:10:07 +1000
Subject: [PATCH 07/14] nodelist: Replace int with bool

Make some interfaces and implementations consistent by replacing int
with bool.

Part of 34200.
---
 src/feature/nodelist/node_select.c |  8 ++++----
 src/feature/nodelist/nodelist.c    | 12 ++++++------
 src/feature/nodelist/nodelist.h    | 17 ++++++++---------
 src/feature/nodelist/routerlist.c  | 20 ++++++++++----------
 src/test/test_circuitbuild.c       |  6 +++---
 5 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c
index 85ef5ad568..25904d4c63 100644
--- a/src/feature/nodelist/node_select.c
+++ b/src/feature/nodelist/node_select.c
@@ -980,10 +980,10 @@ router_choose_random_node(smartlist_t *excludedsmartlist,
 {
   /* A limited set of flags, used for fallback node selection.
    */
-  const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
-  const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
-  const int need_guard = (flags & CRN_NEED_GUARD) != 0;
-  const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
+  const bool need_uptime = (flags & CRN_NEED_UPTIME) != 0;
+  const bool need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
+  const bool need_guard = (flags & CRN_NEED_GUARD) != 0;
+  const bool pref_addr = (flags & CRN_PREF_ADDR) != 0;
 
   smartlist_t *excludednodes=smartlist_new();
   const node_t *choice = NULL;
diff --git a/src/feature/nodelist/nodelist.c b/src/feature/nodelist/nodelist.c
index a553390628..6b2c0d2016 100644
--- a/src/feature/nodelist/nodelist.c
+++ b/src/feature/nodelist/nodelist.c
@@ -1158,9 +1158,9 @@ node_get_protover_summary_flags(const node_t *node)
  * by ed25519 ID during the link handshake.  If <b>compatible_with_us</b>,
  * it needs to be using a link authentication method that we understand.
  * If not, any plausible link authentication method will do. */
-MOCK_IMPL(int,
+MOCK_IMPL(bool,
 node_supports_ed25519_link_authentication,(const node_t *node,
-                                           int compatible_with_us))
+                                           bool compatible_with_us))
 {
   if (! node_get_ed25519_id(node))
     return 0;
@@ -1175,7 +1175,7 @@ node_supports_ed25519_link_authentication,(const node_t *node,
 
 /** Return true iff <b>node</b> supports the hidden service directory version
  * 3 protocol (proposal 224). */
-int
+bool
 node_supports_v3_hsdir(const node_t *node)
 {
   tor_assert(node);
@@ -1185,7 +1185,7 @@ node_supports_v3_hsdir(const node_t *node)
 
 /** Return true iff <b>node</b> supports ed25519 authentication as an hidden
  * service introduction point.*/
-int
+bool
 node_supports_ed25519_hs_intro(const node_t *node)
 {
   tor_assert(node);
@@ -1195,7 +1195,7 @@ node_supports_ed25519_hs_intro(const node_t *node)
 
 /** Return true iff <b>node</b> can be a rendezvous point for hidden
  * service version 3 (HSRend=2). */
-int
+bool
 node_supports_v3_rendezvous_point(const node_t *node)
 {
   tor_assert(node);
@@ -1210,7 +1210,7 @@ node_supports_v3_rendezvous_point(const node_t *node)
 
 /** Return true iff <b>node</b> supports the DoS ESTABLISH_INTRO cell
  * extenstion. */
-int
+bool
 node_supports_establish_intro_dos_extension(const node_t *node)
 {
   tor_assert(node);
diff --git a/src/feature/nodelist/nodelist.h b/src/feature/nodelist/nodelist.h
index 991cec319b..4ba699d69d 100644
--- a/src/feature/nodelist/nodelist.h
+++ b/src/feature/nodelist/nodelist.h
@@ -74,17 +74,16 @@ MOCK_DECL(const struct ed25519_public_key_t *,node_get_ed25519_id,
           (const node_t *node));
 int node_ed25519_id_matches(const node_t *node,
                             const struct ed25519_public_key_t *id);
-MOCK_DECL(int,node_supports_ed25519_link_authentication,
+MOCK_DECL(bool,node_supports_ed25519_link_authentication,
           (const node_t *node,
-           int compatible_with_us));
-int node_supports_v3_hsdir(const node_t *node);
-int node_supports_ed25519_hs_intro(const node_t *node);
-int node_supports_v3_rendezvous_point(const node_t *node);
-int node_supports_establish_intro_dos_extension(const node_t *node);
+           bool compatible_with_us));
+bool node_supports_v3_hsdir(const node_t *node);
+bool node_supports_ed25519_hs_intro(const node_t *node);
+bool node_supports_v3_rendezvous_point(const node_t *node);
+bool node_supports_establish_intro_dos_extension(const node_t *node);
 bool node_supports_initiating_ipv6_extends(const node_t *node);
-bool node_supports_accepting_ipv6_extends(
-                                             const node_t *node,
-                                             bool need_canonical_ipv6_conn);
+bool node_supports_accepting_ipv6_extends(const node_t *node,
+                                          bool need_canonical_ipv6_conn);
 
 const uint8_t *node_get_rsa_id_digest(const node_t *node);
 MOCK_DECL(smartlist_t *,node_get_link_specifier_smartlist,(const node_t *node,
diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c
index 1106222830..72ea48898b 100644
--- a/src/feature/nodelist/routerlist.c
+++ b/src/feature/nodelist/routerlist.c
@@ -542,18 +542,18 @@ void
 router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags)
 {
   /* The full set of flags used for node selection. */
-  const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
-  const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
-  const int need_guard = (flags & CRN_NEED_GUARD) != 0;
-  const int need_desc = (flags & CRN_NEED_DESC) != 0;
-  const int pref_addr = (flags & CRN_PREF_ADDR) != 0;
-  const int direct_conn = (flags & CRN_DIRECT_CONN) != 0;
-  const int rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
+  const bool need_uptime = (flags & CRN_NEED_UPTIME) != 0;
+  const bool need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
+  const bool need_guard = (flags & CRN_NEED_GUARD) != 0;
+  const bool need_desc = (flags & CRN_NEED_DESC) != 0;
+  const bool pref_addr = (flags & CRN_PREF_ADDR) != 0;
+  const bool direct_conn = (flags & CRN_DIRECT_CONN) != 0;
+  const bool rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
   const bool initiate_ipv6_extend = (flags & CRN_INITIATE_IPV6_EXTEND) != 0;
 
-  const int check_reach = !router_or_conn_should_skip_reachable_address_check(
-                                                       get_options(),
-                                                       pref_addr);
+  const bool check_reach =
+    !router_or_conn_should_skip_reachable_address_check(get_options(),
+                                                        pref_addr);
 
   SMARTLIST_FOREACH_BEGIN(nodelist_get_list(), const node_t *, node) {
     if (!node->is_running || !node->is_valid)
diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c
index 44d286e758..88e46af136 100644
--- a/src/test/test_circuitbuild.c
+++ b/src/test/test_circuitbuild.c
@@ -281,10 +281,10 @@ mock_node_get_by_id(const char *identity_digest)
   return mocked_node;
 }
 
-static int mocked_supports_ed25519_link_authentication = 0;
-static int
+static bool mocked_supports_ed25519_link_authentication = 0;
+static bool
 mock_node_supports_ed25519_link_authentication(const node_t *node,
-                                                int compatible_with_us)
+                                               bool compatible_with_us)
 {
   (void)node;
   (void)compatible_with_us;

From 38c72400b7887c3bf5e9ff4082bd8da1e07efd52 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Wed, 13 May 2020 13:15:31 +1000
Subject: [PATCH 08/14] routerlist: Split the node checks into their own
 function

Split the node choosing checks into their own function, so we can call
it independently of iterating through the nodelist.

Part of 34200.
---
 src/feature/nodelist/routerlist.c | 127 ++++++++++++++++--------------
 src/feature/nodelist/routerlist.h |   1 +
 2 files changed, 71 insertions(+), 57 deletions(-)

diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c
index 72ea48898b..f606e3c18f 100644
--- a/src/feature/nodelist/routerlist.c
+++ b/src/feature/nodelist/routerlist.c
@@ -511,35 +511,34 @@ routers_have_same_or_addrs(const routerinfo_t *r1, const routerinfo_t *r2)
     r1->ipv6_orport == r2->ipv6_orport;
 }
 
-/** Add every suitable node from our nodelist to <b>sl</b>, so that
- * we can pick a node for a circuit.
+/* Returns true if <b>node</b> can be chosen based on <b>flags</b>.
+ *
+ * The following conditions are applied to all nodes:
+ *  - is running;
+ *  - is valid;
+ *  - has a general-purpose routerinfo;
+ *  - supports EXTEND2 cells;
+ *  - has an ntor circuit crypto key; and
+ *  - does not allow single-hop exits.
  *
- * If the following <b>flags</b> are set:
- *  - <b>CRN_NEED_UPTIME</b>: if any router has more than a minimum uptime,
- *                            return one of those;
- *  - <b>CRN_NEED_CAPACITY</b>: weight your choice by the advertised capacity
- *                              of each router;
- *  - <b>CRN_NEED_GUARD</b>: only consider Guard routers;
- *  - <b>CRN_WEIGHT_AS_EXIT</b>: we weight bandwidths as if picking an exit
- *                               node, otherwise we weight bandwidths for
- *                               picking a relay node (that is, possibly
- *                               discounting exit nodes);
- *  - <b>CRN_NEED_DESC</b>: only consider nodes that have a routerinfo or
- *                          microdescriptor -- that is, enough info to be
- *                          used to build a circuit;
- *  - <b>CRN_DIRECT_CONN</b>: only consider nodes that are suitable for direct
- *                            connections. Check ReachableAddresses,
- *                            ClientUseIPv4 0, and
+ * The <b>flags</b> chech that <b>node</b>:
+ *  - <b>CRN_NEED_UPTIME</b>: has more than a minimum uptime;
+ *  - <b>CRN_NEED_CAPACITY</b>: has more than a minimum capacity;
+ *  - <b>CRN_NEED_GUARD</b>: is a Guard;
+ *  - <b>CRN_NEED_DESC</b>: has a routerinfo or microdescriptor -- that is,
+ *                          enough info to be used to build a circuit;
+ *  - <b>CRN_DIRECT_CONN</b>: is suitable for direct connections. Checks
+ *                            for the relevant descriptors. Checks the address
+ *                            against ReachableAddresses, ClientUseIPv4 0, and
  *                            fascist_firewall_use_ipv6() == 0);
- *  - <b>CRN_PREF_ADDR</b>: only consider nodes that have an address that is
- *                          preferred by the ClientPreferIPv6ORPort setting.
- *  - <b>CRN_RENDEZVOUS_V3</b>: only consider nodes that can become v3 onion
- *                              service rendezvous points.
- *  - <b>CRN_INITIATE_IPV6_EXTEND</b>: only consider routers than can initiate
- *                                     IPv6 extends.
+ *  - <b>CRN_PREF_ADDR</b>: if we are connecting directly to the node, it has
+ *                          an address that is preferred by the
+ *                          ClientPreferIPv6ORPort setting;
+ *  - <b>CRN_RENDEZVOUS_V3</b>: can become a v3 onion service rendezvous point;
+ *  - <b>CRN_INITIATE_IPV6_EXTEND</b>: can initiate IPv6 extends.
  */
-void
-router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags)
+bool
+router_can_choose_node(const node_t *node, int flags)
 {
   /* The full set of flags used for node selection. */
   const bool need_uptime = (flags & CRN_NEED_UPTIME) != 0;
@@ -555,38 +554,52 @@ router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags)
     !router_or_conn_should_skip_reachable_address_check(get_options(),
                                                         pref_addr);
 
+  if (!node->is_running || !node->is_valid)
+    return false;
+  if (need_desc && !node_has_preferred_descriptor(node, direct_conn))
+    return false;
+  if (node->ri && node->ri->purpose != ROUTER_PURPOSE_GENERAL)
+    return false;
+  if (node_is_unreliable(node, need_uptime, need_capacity, need_guard))
+    return false;
+  /* Don't choose nodes if we are certain they can't do EXTEND2 cells */
+  if (node->rs && !routerstatus_version_supports_extend2_cells(node->rs, 1))
+    return false;
+  /* Don't choose nodes if we are certain they can't do ntor. */
+  if ((node->ri || node->md) && !node_has_curve25519_onion_key(node))
+    return false;
+  /* Exclude relays that allow single hop exit circuits. This is an
+   * obsolete option since 0.2.9.2-alpha and done by default in
+   * 0.3.1.0-alpha. */
+  if (node_allows_single_hop_exits(node))
+    return false;
+  /* Exclude relays that can not become a rendezvous for a hidden service
+   * version 3. */
+  if (rendezvous_v3 &&
+      !node_supports_v3_rendezvous_point(node))
+    return false;
+  /* Choose a node with an OR address that matches the firewall rules */
+  if (direct_conn && check_reach &&
+      !fascist_firewall_allows_node(node,
+                                    FIREWALL_OR_CONNECTION,
+                                    pref_addr))
+    return false;
+  if (initiate_ipv6_extend && !node_supports_initiating_ipv6_extends(node))
+    return false;
+
+  return true;
+}
+
+/** Add every suitable node from our nodelist to <b>sl</b>, so that
+ * we can pick a node for a circuit based on <b>flags</b>.
+ *
+ * See router_can_choose_node() for details of <b>flags</b>.
+ */
+void
+router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags)
+{
   SMARTLIST_FOREACH_BEGIN(nodelist_get_list(), const node_t *, node) {
-    if (!node->is_running || !node->is_valid)
-      continue;
-    if (need_desc && !node_has_preferred_descriptor(node, direct_conn))
-      continue;
-    if (node->ri && node->ri->purpose != ROUTER_PURPOSE_GENERAL)
-      continue;
-    if (node_is_unreliable(node, need_uptime, need_capacity, need_guard))
-      continue;
-    /* Don't choose nodes if we are certain they can't do EXTEND2 cells */
-    if (node->rs && !routerstatus_version_supports_extend2_cells(node->rs, 1))
-      continue;
-    /* Don't choose nodes if we are certain they can't do ntor. */
-    if ((node->ri || node->md) && !node_has_curve25519_onion_key(node))
-      continue;
-    /* Exclude relays that allow single hop exit circuits. This is an
-     * obsolete option since 0.2.9.2-alpha and done by default in
-     * 0.3.1.0-alpha. */
-    if (node_allows_single_hop_exits(node))
-      continue;
-    /* Exclude relays that can not become a rendezvous for a hidden service
-     * version 3. */
-    if (rendezvous_v3 &&
-        !node_supports_v3_rendezvous_point(node))
-      continue;
-    /* Choose a node with an OR address that matches the firewall rules */
-    if (direct_conn && check_reach &&
-        !fascist_firewall_allows_node(node,
-                                      FIREWALL_OR_CONNECTION,
-                                      pref_addr))
-      continue;
-    if (initiate_ipv6_extend && !node_supports_initiating_ipv6_extends(node))
+    if (!router_can_choose_node(node, flags))
       continue;
     smartlist_add(sl, (void *)node);
   } SMARTLIST_FOREACH_END(node);
diff --git a/src/feature/nodelist/routerlist.h b/src/feature/nodelist/routerlist.h
index 7d0788bacb..98472b2771 100644
--- a/src/feature/nodelist/routerlist.h
+++ b/src/feature/nodelist/routerlist.h
@@ -58,6 +58,7 @@ int router_dir_conn_should_skip_reachable_address_check(
                                        int try_ip_pref);
 void router_reset_status_download_failures(void);
 int routers_have_same_or_addrs(const routerinfo_t *r1, const routerinfo_t *r2);
+bool router_can_choose_node(const node_t *node, int flags);
 void router_add_running_nodes_to_smartlist(smartlist_t *sl, int flags);
 
 const routerinfo_t *routerlist_find_my_routerinfo(void);

From 766fc86df46ff88364610b18e28904c3b45f98f1 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Wed, 13 May 2020 13:17:41 +1000
Subject: [PATCH 09/14] circuitbuild: Do node checks when choosing exits

And check that the correct flags are passed when choosing exits.

Adds the following checks for exits:
* must support EXTEND2 cells,
* must have an ntor circuit crypto key,
* can't require the guard flag,
* can't be a direct connection.

All these checks are already implied by other code.

Part of 34200.
---
 src/core/or/circuitbuild.c | 48 +++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index b840ca7a7d..bac82eb9f5 100644
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -1563,7 +1563,23 @@ choose_good_exit_server_general(router_crn_flags_t flags)
   const node_t *selected_node=NULL;
   const int need_uptime = (flags & CRN_NEED_UPTIME) != 0;
   const int need_capacity = (flags & CRN_NEED_CAPACITY) != 0;
-  const int direct_conn = (flags & CRN_DIRECT_CONN) != 0;
+
+  /* We should not require guard flags on exits. */
+  IF_BUG_ONCE(flags & CRN_NEED_GUARD)
+    return NULL;
+
+  /* We reject single-hop exits for all node positions. */
+  IF_BUG_ONCE(flags & CRN_DIRECT_CONN)
+    return NULL;
+
+  /* This isn't the function for picking rendezvous nodes. */
+  IF_BUG_ONCE(flags & CRN_RENDEZVOUS_V3)
+    return NULL;
+
+  /* We only want exits to extend if we cannibalize the circuit.
+   * But we don't require IPv6 extends yet. */
+  IF_BUG_ONCE(flags & CRN_INITIATE_IPV6_EXTEND)
+    return NULL;
 
   connections = get_connection_array();
 
@@ -1596,19 +1612,14 @@ choose_good_exit_server_general(router_crn_flags_t flags)
        */
       continue;
     }
-    if (!node_has_preferred_descriptor(node, direct_conn)) {
+    if (!router_can_choose_node(node, flags)) {
       n_supported[i] = -1;
       continue;
     }
-    if (!node->is_running || node->is_bad_exit) {
+    if (node->is_bad_exit) {
       n_supported[i] = -1;
       continue; /* skip routers that are known to be down or bad exits */
     }
-    if (node_get_purpose(node) != ROUTER_PURPOSE_GENERAL) {
-      /* never pick a non-general node as a random exit. */
-      n_supported[i] = -1;
-      continue;
-    }
     if (routerset_contains_node(options->ExcludeExitNodesUnion_, node)) {
       n_supported[i] = -1;
       continue; /* user asked us not to use it, no matter what */
@@ -1618,27 +1629,6 @@ choose_good_exit_server_general(router_crn_flags_t flags)
       n_supported[i] = -1;
       continue; /* not one of our chosen exit nodes */
     }
-
-    if (node_is_unreliable(node, need_uptime, need_capacity, 0)) {
-      n_supported[i] = -1;
-      continue; /* skip routers that are not suitable.  Don't worry if
-                 * this makes us reject all the possible routers: if so,
-                 * we'll retry later in this function with need_update and
-                 * need_capacity set to 0. */
-    }
-    if (!(node->is_valid)) {
-      /* if it's invalid and we don't want it */
-      n_supported[i] = -1;
-//      log_fn(LOG_DEBUG,"Skipping node %s (index %d) -- invalid router.",
-//             router->nickname, i);
-      continue; /* skip invalid routers */
-    }
-    /* We do not allow relays that allow single hop exits by default. Option
-     * was deprecated in 0.2.9.2-alpha and removed in 0.3.1.0-alpha. */
-    if (node_allows_single_hop_exits(node)) {
-      n_supported[i] = -1;
-      continue;
-    }
     if (node_exit_policy_rejects_all(node)) {
       n_supported[i] = -1;
 //      log_fn(LOG_DEBUG,"Skipping node %s (index %d) -- it rejects all.",

From e46c3d95f4257fd1292c721a5d737af0f74f2f83 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Wed, 13 May 2020 13:23:39 +1000
Subject: [PATCH 10/14] circuitbuild: Do node checks when counting nodes

Use the node check function to check that there are enough nodes to
select a circuit path.

Adds these checks, which are implied by other code:
* supports EXTEND2 cells,
* does not allow single-hop exits,

Adds these extra checks:
* has a general-purpose routerinfo,
* if it is a direct connection, check reachable addresses.
These checks reduce the node count, but they will never under-count
nodes.

Bridge nodes aren't handled correctly, we'll fix that in the next
commit.

Part of 34200.
---
 src/core/or/circuitbuild.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c
index bac82eb9f5..233ea9bd08 100644
--- a/src/core/or/circuitbuild.c
+++ b/src/core/or/circuitbuild.c
@@ -2103,32 +2103,27 @@ circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit_ei)
   return 0;
 }
 
-/** Return the number of routers in <b>routers</b> that are currently up
- * and available for building circuits through.
+/** Return the number of routers in <b>nodes</b> that are currently up and
+ * available for building circuits through.
  *
- * (Note that this function may overcount or undercount, if we have
- * descriptors that are not the type we would prefer to use for some
- * particular router. See bug #25885.)
+ * If <b>direct</b> is true, only count nodes that are suitable for direct
+ * connections. Counts nodes regardless of whether their addresses are
+ * preferred.
  */
 MOCK_IMPL(STATIC int,
 count_acceptable_nodes, (const smartlist_t *nodes, int direct))
 {
   int num=0;
+  int flags = CRN_NEED_DESC;
+
+  if (direct)
+    flags |= CRN_DIRECT_CONN;
 
   SMARTLIST_FOREACH_BEGIN(nodes, const node_t *, node) {
     //    log_debug(LD_CIRC,
-//              "Contemplating whether router %d (%s) is a new option.",
-//              i, r->nickname);
-    if (! node->is_running)
-//      log_debug(LD_CIRC,"Nope, the directory says %d is not running.",i);
-      continue;
-    if (! node->is_valid)
-//      log_debug(LD_CIRC,"Nope, the directory says %d is not valid.",i);
-      continue;
-    if (! node_has_preferred_descriptor(node, direct))
-      continue;
-    /* The node has a descriptor, so we can just check the ntor key directly */
-    if (!node_has_curve25519_onion_key(node))
+    //              "Contemplating whether router %d (%s) is a new option.",
+    //              i, r->nickname);
+    if (!router_can_choose_node(node, flags))
       continue;
     ++num;
   } SMARTLIST_FOREACH_END(node);

From 73ace125a972c421287503738e9599f411193e1a Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Wed, 13 May 2020 13:47:52 +1000
Subject: [PATCH 11/14] routerlist: Choose bridges for direct bridge
 connections

When counting and choosing nodes on a client that uses bridges, only
choose bridges for direct connections.

Part of 34200.
---
 src/feature/nodelist/routerlist.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/src/feature/nodelist/routerlist.c b/src/feature/nodelist/routerlist.c
index f606e3c18f..ece3379fa5 100644
--- a/src/feature/nodelist/routerlist.c
+++ b/src/feature/nodelist/routerlist.c
@@ -516,11 +516,19 @@ routers_have_same_or_addrs(const routerinfo_t *r1, const routerinfo_t *r2)
  * The following conditions are applied to all nodes:
  *  - is running;
  *  - is valid;
- *  - has a general-purpose routerinfo;
  *  - supports EXTEND2 cells;
  *  - has an ntor circuit crypto key; and
  *  - does not allow single-hop exits.
  *
+ * If the node has a routerinfo, we're checking for a direct connection, and
+ * we're using bridges, the following condition is applied:
+ *  - has a bridge-purpose routerinfo;
+ * and for all other nodes:
+ *  - has a general-purpose routerinfo (or no routerinfo).
+ *
+ * Nodes that don't have a routerinfo must be general-purpose nodes, because
+ * routerstatuses and microdescriptors only come via consensuses.
+ *
  * The <b>flags</b> chech that <b>node</b>:
  *  - <b>CRN_NEED_UPTIME</b>: has more than a minimum uptime;
  *  - <b>CRN_NEED_CAPACITY</b>: has more than a minimum capacity;
@@ -550,16 +558,21 @@ router_can_choose_node(const node_t *node, int flags)
   const bool rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0;
   const bool initiate_ipv6_extend = (flags & CRN_INITIATE_IPV6_EXTEND) != 0;
 
+  const or_options_t *options = get_options();
   const bool check_reach =
-    !router_or_conn_should_skip_reachable_address_check(get_options(),
-                                                        pref_addr);
+    !router_or_conn_should_skip_reachable_address_check(options, pref_addr);
+  const bool direct_bridge = direct_conn && options->UseBridges;
 
   if (!node->is_running || !node->is_valid)
     return false;
   if (need_desc && !node_has_preferred_descriptor(node, direct_conn))
     return false;
-  if (node->ri && node->ri->purpose != ROUTER_PURPOSE_GENERAL)
-    return false;
+  if (node->ri) {
+    if (direct_bridge && node->ri->purpose != ROUTER_PURPOSE_BRIDGE)
+      return false;
+    else if (node->ri->purpose != ROUTER_PURPOSE_GENERAL)
+      return false;
+  }
   if (node_is_unreliable(node, need_uptime, need_capacity, need_guard))
     return false;
   /* Don't choose nodes if we are certain they can't do EXTEND2 cells */

From 8fbcc055e5e7217404039e84e23cf35c5435dbe5 Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Wed, 13 May 2020 15:34:37 +1000
Subject: [PATCH 12/14] node: Clean up some outdated comments

Part of 34200.
---
 src/feature/nodelist/node_st.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/feature/nodelist/node_st.h b/src/feature/nodelist/node_st.h
index b1ec4db202..3769f9dc84 100644
--- a/src/feature/nodelist/node_st.h
+++ b/src/feature/nodelist/node_st.h
@@ -84,12 +84,11 @@ struct node_t {
 
   /* Local info: derived. */
 
-  /** True if the IPv6 OR port is preferred over the IPv4 OR port.
-   * XX/teor - can this become out of date if the torrc changes? */
+  /** True if the IPv6 OR port is preferred over the IPv4 OR port. */
   unsigned int ipv6_preferred:1;
 
   /** According to the geoip db what country is this router in? */
-  /* XXXprop186 what is this suppose to mean with multiple OR ports? */
+  /* IPv6: what is this supposed to mean with multiple OR ports? */
   country_t country;
 
   /* The below items are used only by authdirservers for

From 8ec4d9cc3c90a56bf66cacd128e1b54124868f9f Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 11 May 2020 18:12:31 +1000
Subject: [PATCH 13/14] practracker: Accept extra file lines, enforce a smaller
 function

Accept extra lines in nodelist and routerlist due to extra features, and
due to refactors that simplify some functions.

Most of the refactor eliminated duplicate code in smaller functions, so
there's only one large function that got smaller.

Part of 34200.
---
 scripts/maint/practracker/exceptions.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt
index ca06d94fbd..8a62c260b5 100644
--- a/scripts/maint/practracker/exceptions.txt
+++ b/scripts/maint/practracker/exceptions.txt
@@ -96,7 +96,7 @@ problem function-size /src/core/or/channeltls.c:channel_tls_process_authenticate
 problem dependency-violation /src/core/or/channeltls.c 11
 problem include-count /src/core/or/circuitbuild.c 53
 problem function-size /src/core/or/circuitbuild.c:get_unique_circ_id_by_chan() 128
-problem function-size /src/core/or/circuitbuild.c:choose_good_exit_server_general() 206
+problem function-size /src/core/or/circuitbuild.c:choose_good_exit_server_general() 196
 problem dependency-violation /src/core/or/circuitbuild.c 25
 problem include-count /src/core/or/circuitlist.c 55
 problem function-size /src/core/or/circuitlist.c:HT_PROTOTYPE() 109
@@ -259,7 +259,7 @@ problem function-size /src/feature/nodelist/node_select.c:router_pick_directory_
 problem function-size /src/feature/nodelist/node_select.c:compute_weighted_bandwidths() 204
 problem function-size /src/feature/nodelist/node_select.c:router_pick_trusteddirserver_impl() 116
 problem function-size /src/feature/nodelist/nodelist.c:compute_frac_paths_available() 190
-problem file-size /src/feature/nodelist/routerlist.c 3300
+problem file-size /src/feature/nodelist/routerlist.c 3350
 problem function-size /src/feature/nodelist/routerlist.c:router_rebuild_store() 148
 problem function-size /src/feature/nodelist/routerlist.c:router_add_to_routerlist() 168
 problem function-size /src/feature/nodelist/routerlist.c:routerlist_remove_old_routers() 121

From 1df451aba116f77a5a24e4e54cf343ec46d55f9a Mon Sep 17 00:00:00 2001
From: teor <teor@riseup.net>
Date: Mon, 18 May 2020 22:12:26 +1000
Subject: [PATCH 14/14] changes: file for 34200

---
 changes/ticket34200 | 3 +++
 1 file changed, 3 insertions(+)
 create mode 100644 changes/ticket34200

diff --git a/changes/ticket34200 b/changes/ticket34200
new file mode 100644
index 0000000000..b984bd83bb
--- /dev/null
+++ b/changes/ticket34200
@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Refactor some common node selection code into a single function.
+      Closes ticket 34200.