Ticket34200

changes/ticket34200

@@ -0,0 +1,3 @@
+  o Code simplification and refactoring:
+    - Refactor some common node selection code into a single function.
+      Closes ticket 34200.

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

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.",
@@ -1795,14 +1785,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_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. */
@@ -2120,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);

src/feature/nodelist/node_select.c

@@ -930,105 +930,85 @@ 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.
  *
- * 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 */
-  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;
-
-  const smartlist_t *node_list = nodelist_get_list();
-  smartlist_t *sl=smartlist_new(),
-    *excludednodes=smartlist_new();
+{
+  /* A limited set of flags, used for fallback node selection.
+   */
+  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;
   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);
-
-  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 &&
-               !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);
+  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
    * this is fine to at least try with our routerinfo_t object. */
   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, initiate_ipv6_extend);
-  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.",
@@ -1038,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) {

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

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