diff --git a/changes/ticket30307 b/changes/ticket30307 new file mode 100644 index 0000000000..abcacb6085 --- /dev/null +++ b/changes/ticket30307 @@ -0,0 +1,4 @@ + o Major features (performance): + - Update our node selection algorithm to exclude nodes in linear time. + Previously, the algorithm was quadratic, which could slow down heavily + used onion services. Closes ticket 30307. diff --git a/changes/ticket30308 b/changes/ticket30308 new file mode 100644 index 0000000000..b78e6b3e9f --- /dev/null +++ b/changes/ticket30308 @@ -0,0 +1,5 @@ + o Minor bugfixes (performance): + - When checking a node for bridge status, use a fast check to make sure + that its identity is set. Previously, we used a constant-time check, + which is not necessary when verifying a BUG() condition that causes + a stack trace. Fixes bug 30308; bugfix on 0.3.5.1-alpha. diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index 677e7f0d42..a4ffd8e531 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -78,7 +78,7 @@ problem function-size /src/core/or/channeltls.c:channel_tls_process_versions_cel problem function-size /src/core/or/channeltls.c:channel_tls_process_netinfo_cell() 214 problem function-size /src/core/or/channeltls.c:channel_tls_process_certs_cell() 246 problem function-size /src/core/or/channeltls.c:channel_tls_process_authenticate_cell() 202 -problem file-size /src/core/or/circuitbuild.c 3060 +problem file-size /src/core/or/circuitbuild.c 3061 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:circuit_extend() 147 diff --git a/src/core/or/circuitbuild.c b/src/core/or/circuitbuild.c index f8e87bf026..cfe0a97bcf 100644 --- a/src/core/or/circuitbuild.c +++ b/src/core/or/circuitbuild.c @@ -1683,7 +1683,8 @@ route_len_for_purpose(uint8_t purpose, extend_info_t *exit_ei) * to handle the desired path length, return -1. */ STATIC int -new_route_len(uint8_t purpose, extend_info_t *exit_ei, smartlist_t *nodes) +new_route_len(uint8_t purpose, extend_info_t *exit_ei, + const smartlist_t *nodes) { int routelen; @@ -2345,7 +2346,7 @@ circuit_extend_to_new_exit(origin_circuit_t *circ, extend_info_t *exit_ei) * particular router. See bug #25885.) */ MOCK_IMPL(STATIC int, -count_acceptable_nodes, (smartlist_t *nodes, int direct)) +count_acceptable_nodes, (const smartlist_t *nodes, int direct)) { int num=0; diff --git a/src/core/or/circuitbuild.h b/src/core/or/circuitbuild.h index b19bc41235..b45bc816a3 100644 --- a/src/core/or/circuitbuild.h +++ b/src/core/or/circuitbuild.h @@ -83,8 +83,8 @@ void circuit_upgrade_circuits_from_guard_wait(void); #ifdef CIRCUITBUILD_PRIVATE STATIC circid_t get_unique_circ_id_by_chan(channel_t *chan); STATIC int new_route_len(uint8_t purpose, extend_info_t *exit_ei, - smartlist_t *nodes); -MOCK_DECL(STATIC int, count_acceptable_nodes, (smartlist_t *nodes, + const smartlist_t *nodes); +MOCK_DECL(STATIC int, count_acceptable_nodes, (const smartlist_t *nodes, int direct)); STATIC int onion_extend_cpath(origin_circuit_t *circ); diff --git a/src/feature/client/bridges.c b/src/feature/client/bridges.c index 05f89ad36c..ea1aff9519 100644 --- a/src/feature/client/bridges.c +++ b/src/feature/client/bridges.c @@ -348,7 +348,7 @@ int node_is_a_configured_bridge(const node_t *node) { /* First, let's try searching for a bridge with matching identity. */ - if (BUG(tor_digest_is_zero(node->identity))) + if (BUG(tor_mem_is_zero(node->identity, DIGEST_LEN))) return 0; if (find_bridge_by_digest(node->identity) != NULL) diff --git a/src/feature/dirauth/voteflags.c b/src/feature/dirauth/voteflags.c index 4040f162fa..957ebe4a4f 100644 --- a/src/feature/dirauth/voteflags.c +++ b/src/feature/dirauth/voteflags.c @@ -239,7 +239,7 @@ dirserv_compute_performance_thresholds(digestmap_t *omit_as_sybil) uint32_t *uptimes, *bandwidths_kb, *bandwidths_excluding_exits_kb; long *tks; double *mtbfs, *wfus; - smartlist_t *nodelist; + const smartlist_t *nodelist; time_t now = time(NULL); const or_options_t *options = get_options(); diff --git a/src/feature/nodelist/node_select.c b/src/feature/nodelist/node_select.c index 93ddb066d4..719b4b1b27 100644 --- a/src/feature/nodelist/node_select.c +++ b/src/feature/nodelist/node_select.c @@ -30,6 +30,7 @@ #include "feature/nodelist/routerset.h" #include "feature/relay/router.h" #include "feature/relay/routermode.h" +#include "lib/container/bitarray.h" #include "lib/crypt_ops/crypto_rand.h" #include "lib/math/fp.h" @@ -826,6 +827,58 @@ routerlist_add_node_and_family(smartlist_t *sl, const routerinfo_t *router) nodelist_add_node_and_family(sl, node); } +/** + * Remove every node_t that appears in excluded from sl. + * + * Behaves like smartlist_subtract, but uses nodelist_idx values to deliver + * linear performance when smartlist_subtract would be quadratic. + **/ +static void +nodelist_subtract(smartlist_t *sl, const smartlist_t *excluded) +{ + const smartlist_t *nodelist = nodelist_get_list(); + const int nodelist_len = smartlist_len(nodelist); + bitarray_t *excluded_idx = bitarray_init_zero(nodelist_len); + + /* We haven't used nodelist_idx in this way previously, so I'm going to be + * paranoid in this code, and check that nodelist_idx is correct for every + * node before we use it. If we fail, we fall back to smartlist_subtract(). + */ + + /* Set the excluded_idx bit corresponding to every excluded node... + */ + SMARTLIST_FOREACH_BEGIN(excluded, const node_t *, node) { + const int idx = node->nodelist_idx; + if (BUG(idx < 0) || BUG(idx >= nodelist_len) || + BUG(node != smartlist_get(nodelist, idx))) { + goto internal_error; + } + bitarray_set(excluded_idx, idx); + } SMARTLIST_FOREACH_END(node); + + /* Then remove them from sl. + */ + SMARTLIST_FOREACH_BEGIN(sl, const node_t *, node) { + const int idx = node->nodelist_idx; + if (BUG(idx < 0) || BUG(idx >= nodelist_len) || + BUG(node != smartlist_get(nodelist, idx))) { + goto internal_error; + } + if (bitarray_is_set(excluded_idx, idx)) { + SMARTLIST_DEL_CURRENT(sl, node); + } + } SMARTLIST_FOREACH_END(node); + + bitarray_free(excluded_idx); + return; + + internal_error: + log_warn(LD_BUG, "Internal error prevented us from using the fast method " + "for subtracting nodelists. Falling back to the quadratic way."); + smartlist_subtract(sl, excluded); + bitarray_free(excluded_idx); +} + /** Return a random running node from the nodelist. Never * pick a node that is in * excludedsmartlist, or which matches excludedset, @@ -860,6 +913,7 @@ router_choose_random_node(smartlist_t *excludedsmartlist, const int direct_conn = (flags & CRN_DIRECT_CONN) != 0; const int rendezvous_v3 = (flags & CRN_RENDEZVOUS_V3) != 0; + const smartlist_t *node_list = nodelist_get_list(); smartlist_t *sl=smartlist_new(), *excludednodes=smartlist_new(); const node_t *choice = NULL; @@ -870,17 +924,17 @@ 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(nodelist_get_list(), node_t *, node) { + 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); + smartlist_add(excludednodes, (node_t*)node); } else if (rendezvous_v3 && !node_supports_v3_rendezvous_point(node)) { /* Exclude relays that do not support to rendezvous for a hidden service * version 3. */ - smartlist_add(excludednodes, node); + smartlist_add(excludednodes, (node_t*)node); } } SMARTLIST_FOREACH_END(node); @@ -897,19 +951,11 @@ router_choose_random_node(smartlist_t *excludedsmartlist, "We found %d running nodes.", smartlist_len(sl)); - smartlist_subtract(sl,excludednodes); - log_debug(LD_CIRC, - "We removed %d excludednodes, leaving %d nodes.", - smartlist_len(excludednodes), - smartlist_len(sl)); - if (excludedsmartlist) { - smartlist_subtract(sl,excludedsmartlist); - log_debug(LD_CIRC, - "We removed %d excludedsmartlist, leaving %d nodes.", - smartlist_len(excludedsmartlist), - smartlist_len(sl)); + smartlist_add_all(excludednodes, excludedsmartlist); } + nodelist_subtract(sl, excludednodes); + if (excludedset) { routerset_subtract_nodes(sl,excludedset); log_debug(LD_CIRC, diff --git a/src/feature/nodelist/nodelist.c b/src/feature/nodelist/nodelist.c index f878d47fd7..8aa4915107 100644 --- a/src/feature/nodelist/nodelist.c +++ b/src/feature/nodelist/nodelist.c @@ -944,7 +944,7 @@ nodelist_ensure_freshness(networkstatus_t *ns) /** Return a list of a node_t * for every node we know about. The caller * MUST NOT modify the list. (You can set and clear flags in the nodes if * you must, but you must not add or remove nodes.) */ -MOCK_IMPL(smartlist_t *, +MOCK_IMPL(const smartlist_t *, nodelist_get_list,(void)) { init_nodelist(); @@ -1939,7 +1939,7 @@ node_set_country(node_t *node) void nodelist_refresh_countries(void) { - smartlist_t *nodes = nodelist_get_list(); + const smartlist_t *nodes = nodelist_get_list(); SMARTLIST_FOREACH(nodes, node_t *, node, node_set_country(node)); } diff --git a/src/feature/nodelist/nodelist.h b/src/feature/nodelist/nodelist.h index a3d65347a8..84ab5f7a54 100644 --- a/src/feature/nodelist/nodelist.h +++ b/src/feature/nodelist/nodelist.h @@ -101,7 +101,7 @@ const struct curve25519_public_key_t *node_get_curve25519_onion_key( const node_t *node); crypto_pk_t *node_get_rsa_onion_key(const node_t *node); -MOCK_DECL(smartlist_t *, nodelist_get_list, (void)); +MOCK_DECL(const smartlist_t *, nodelist_get_list, (void)); /* Temporary during transition to multiple addresses. */ void node_get_addr(const node_t *node, tor_addr_t *addr_out); diff --git a/src/feature/nodelist/routerset.c b/src/feature/nodelist/routerset.c index 55e2756959..e801fd81b1 100644 --- a/src/feature/nodelist/routerset.c +++ b/src/feature/nodelist/routerset.c @@ -378,7 +378,7 @@ routerset_get_all_nodes(smartlist_t *out, const routerset_t *routerset, } else { /* We need to iterate over the routerlist to get all the ones of the * right kind. */ - smartlist_t *nodes = nodelist_get_list(); + const smartlist_t *nodes = nodelist_get_list(); SMARTLIST_FOREACH(nodes, const node_t *, node, { if (running_only && !node->is_running) continue; diff --git a/src/test/test_circuitbuild.c b/src/test/test_circuitbuild.c index 27f2cd1ca5..47218a559a 100644 --- a/src/test/test_circuitbuild.c +++ b/src/test/test_circuitbuild.c @@ -21,7 +21,7 @@ static smartlist_t dummy_nodes; static extend_info_t dummy_ei; static int -mock_count_acceptable_nodes(smartlist_t *nodes, int direct) +mock_count_acceptable_nodes(const smartlist_t *nodes, int direct) { (void)nodes; diff --git a/src/test/test_controller.c b/src/test/test_controller.c index c130248592..ee48d656bd 100644 --- a/src/test/test_controller.c +++ b/src/test/test_controller.c @@ -1723,7 +1723,7 @@ test_current_time(void *arg) static size_t n_nodelist_get_list = 0; static smartlist_t *nodes = NULL; -static smartlist_t * +static const smartlist_t * mock_nodelist_get_list(void) { n_nodelist_get_list++; diff --git a/src/test/test_entrynodes.c b/src/test/test_entrynodes.c index 729795b674..c43b21c673 100644 --- a/src/test/test_entrynodes.c +++ b/src/test/test_entrynodes.c @@ -67,7 +67,7 @@ static networkstatus_t *dummy_consensus = NULL; static smartlist_t *big_fake_net_nodes = NULL; -static smartlist_t * +static const smartlist_t * bfn_mock_nodelist_get_list(void) { return big_fake_net_nodes; @@ -197,6 +197,7 @@ big_fake_network_setup(const struct testcase_t *testcase) n->md->exit_policy = parse_short_policy("accept 443"); } + n->nodelist_idx = smartlist_len(big_fake_net_nodes); smartlist_add(big_fake_net_nodes, n); } diff --git a/src/test/test_helpers.c b/src/test/test_helpers.c index 13de1e154b..489c257761 100644 --- a/src/test/test_helpers.c +++ b/src/test/test_helpers.c @@ -78,7 +78,7 @@ helper_setup_fake_routerlist(void) { int retval; routerlist_t *our_routerlist = NULL; - smartlist_t *our_nodelist = NULL; + const smartlist_t *our_nodelist = NULL; /* Read the file that contains our test descriptors. */ diff --git a/src/test/test_hs_common.c b/src/test/test_hs_common.c index eb7f3bfbb0..bb41f1f870 100644 --- a/src/test/test_hs_common.c +++ b/src/test/test_hs_common.c @@ -275,7 +275,7 @@ test_start_time_of_next_time_period(void *arg) static void cleanup_nodelist(void) { - smartlist_t *nodelist = nodelist_get_list(); + const smartlist_t *nodelist = nodelist_get_list(); SMARTLIST_FOREACH_BEGIN(nodelist, node_t *, node) { tor_free(node->md); node->md = NULL; diff --git a/src/test/test_routerset.c b/src/test/test_routerset.c index c45f0e1595..cc73e6c20a 100644 --- a/src/test/test_routerset.c +++ b/src/test/test_routerset.c @@ -1765,7 +1765,7 @@ NS(node_get_by_nickname)(const char *nickname, unsigned flags) * Structural test for routerset_get_all_nodes, when the nodelist has no nodes. */ -NS_DECL(smartlist_t *, nodelist_get_list, (void)); +NS_DECL(const smartlist_t *, nodelist_get_list, (void)); static smartlist_t *NS(mock_smartlist); @@ -1795,7 +1795,7 @@ NS(test_main)(void *arg) ; } -smartlist_t * +const smartlist_t * NS(nodelist_get_list)(void) { CALLED(nodelist_get_list)++; @@ -1811,7 +1811,7 @@ NS(nodelist_get_list)(void) * the running_only flag is set, but the nodes are not running. */ -NS_DECL(smartlist_t *, nodelist_get_list, (void)); +NS_DECL(const smartlist_t *, nodelist_get_list, (void)); static smartlist_t *NS(mock_smartlist); static node_t NS(mock_node); @@ -1844,7 +1844,7 @@ NS(test_main)(void *arg) ; } -smartlist_t * +const smartlist_t * NS(nodelist_get_list)(void) { CALLED(nodelist_get_list)++;