From ce8266d52d296333eb1a735225e620760aab8802 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 13 Apr 2016 00:06:30 -0400 Subject: [PATCH 01/17] fix typos/etc before i go nuts on #18809 --- src/or/directory.c | 12 ++++++------ src/or/main.c | 2 +- src/or/networkstatus.c | 17 +++++++++-------- src/or/networkstatus.h | 2 +- src/test/test_connection.c | 2 +- 5 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 8dc018a662..8486c8c9c0 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3703,7 +3703,7 @@ connection_dir_would_close_consensus_conn_helper(void) * 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))) { + && networkstatus_consensus_is_bootstrapping(time(NULL))) { return 0; } @@ -3737,7 +3737,7 @@ connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose) * 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))) { + && networkstatus_consensus_is_bootstrapping(time(NULL))) { return 1; } @@ -3778,12 +3778,12 @@ connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn) return 0; } - const int we_are_bootstrapping = networkstatus_consensus_is_boostrapping( + const int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( time(NULL)); /* We don't want to check other connections to see if they are downloading, * as this is prone to race-conditions. So leave it for - * connection_dir_consider_close_extra_consensus_conns() to clean up. + * connection_dir_close_extra_consensus_conns(() to clean up. * * But if conn has just started connecting, or we have a consensus already, * we can be sure it's not needed any more. */ @@ -3823,7 +3823,7 @@ connection_dir_close_extra_consensus_conns(void) return; } - int we_are_bootstrapping = networkstatus_consensus_is_boostrapping( + int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( time(NULL)); const char *usable_resource = networkstatus_get_flavor_name( @@ -3932,7 +3932,7 @@ find_dl_schedule(download_status_t *dls, const or_options_t *options) const int dir_server = dir_server_mode(options); const int multi_d = networkstatus_consensus_can_use_multiple_directories( options); - const int we_are_bootstrapping = networkstatus_consensus_is_boostrapping( + const int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( time(NULL)); const int use_fallbacks = networkstatus_consensus_can_use_extra_fallbacks( options); diff --git a/src/or/main.c b/src/or/main.c index a2cf5b1101..bf5b2b823a 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1917,7 +1917,7 @@ fetch_networkstatus_callback(time_t now, const or_options_t *options) /* How often do we check whether we should download network status * documents? */ - const int we_are_bootstrapping = networkstatus_consensus_is_boostrapping( + const int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( now); const int prefer_mirrors = !directory_fetches_from_authorities( get_options()); diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 3967f56edd..51c985e85e 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -819,7 +819,7 @@ update_consensus_networkstatus_downloads(time_t now) { int i; const or_options_t *options = get_options(); - const int we_are_bootstrapping = networkstatus_consensus_is_boostrapping( + const int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( now); const int use_multi_conn = networkstatus_consensus_can_use_multiple_directories(options); @@ -875,12 +875,13 @@ update_consensus_networkstatus_downloads(time_t now) resource, DIR_CONN_STATE_CONNECTING); - if (i == usable_consensus_flavor() - && connect_consens_conn_count < consens_conn_count) { + /* If not all connections are "connecting", then some are + * downloading. We want to have at most one downloading at a time. */ + if (connect_consens_conn_count < consens_conn_count) { continue; } - /* Make multiple connections for a bootstrap consensus download */ + /* Make multiple connections for a bootstrap consensus download. */ update_consensus_bootstrap_multiple_downloads(now, options, we_are_bootstrapping); } else { @@ -954,7 +955,7 @@ update_consensus_bootstrap_attempt_downloads( * connections. * Only call when bootstrapping, and when we want to make additional * connections. Only nodes that satisfy - * networkstatus_consensus_can_use_multiple_directories make additonal + * networkstatus_consensus_can_use_multiple_directories make additional * connections. */ static void @@ -969,7 +970,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now, return; } - /* If we've managed to validate a usable consensus, don't make additonal + /* If we've managed to validate a usable consensus, don't make additional * connections. */ if (!we_are_bootstrapping) { return; @@ -1277,7 +1278,7 @@ networkstatus_get_reasonably_live_consensus(time_t now, int flavor) * only using the authorities and fallback directory mirrors to download the * consensus flavour we'll use. */ int -networkstatus_consensus_is_boostrapping(time_t now) +networkstatus_consensus_is_bootstrapping(time_t now) { /* If we don't have a consensus, we must still be bootstrapping */ return !networkstatus_get_reasonably_live_consensus( @@ -1327,7 +1328,7 @@ networkstatus_consensus_can_use_extra_fallbacks(const or_options_t *options) * 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(). */ + * and networkstatus_consensus_is_bootstrapping(). */ int networkstatus_consensus_has_excess_connections(void) { diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 9bbb9a389e..f2f8af5c6b 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -70,7 +70,7 @@ MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus_by_flavor, networkstatus_t *networkstatus_get_live_consensus(time_t now); networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now, int flavor); -int networkstatus_consensus_is_boostrapping(time_t now); +int networkstatus_consensus_is_bootstrapping(time_t now); int networkstatus_consensus_can_use_multiple_directories( const or_options_t *options); int networkstatus_consensus_can_use_extra_fallbacks( diff --git a/src/test/test_connection.c b/src/test/test_connection.c index 15ae973f00..6f7aef879c 100644 --- a/src/test/test_connection.c +++ b/src/test/test_connection.c @@ -705,7 +705,7 @@ test_conn_download_status(void *arg) /* now try closing the one that isn't downloading: * these tests won't work unless tor thinks it is bootstrapping */ - tt_assert(networkstatus_consensus_is_boostrapping(time(NULL))); + tt_assert(networkstatus_consensus_is_bootstrapping(time(NULL))); tt_assert(connection_dir_count_by_purpose_and_resource( TEST_CONN_RSRC_PURPOSE, From 91c58013be735584acc674a60bb18d1efb38f25a Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 13 Apr 2016 01:08:17 -0400 Subject: [PATCH 02/17] avoid following through on a consensus fetch if we have one already arriving --- src/or/circuituse.c | 19 +++++++++++++++ src/or/networkstatus.c | 53 ++++++++++++++++++++++++++++-------------- src/or/networkstatus.h | 1 + 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/src/or/circuituse.c b/src/or/circuituse.c index 31003ea095..f32763ad9e 100644 --- a/src/or/circuituse.c +++ b/src/or/circuituse.c @@ -2355,6 +2355,25 @@ connection_ap_handshake_attach_circuit(entry_connection_t *conn) /* we're a general conn */ origin_circuit_t *circ=NULL; + /* Are we linked to a dir conn that aims to fetch a consensus? + * We check here because this conn might no longer be needed. */ + if (base_conn->linked_conn && + base_conn->linked_conn->type == CONN_TYPE_DIR && + base_conn->linked_conn->purpose == DIR_PURPOSE_FETCH_CONSENSUS) { + + /* Yes we are. Is there a consensus fetch farther along than us? */ + if (networkstatus_consensus_is_already_downloading( + TO_DIR_CONN(base_conn->linked_conn)->requested_resource)) { + /* We're doing the "multiple consensus fetch attempts" game from + * proposal 210, and we're late to the party. Just close this conn. + * The circuit and TLS conn that we made will time out after a while + * if nothing else wants to use them. */ + log_info(LD_DIR, "Closing extra consensus fetch (to %s) since one " + "is already downloading.", base_conn->linked_conn->address); + return -1; + } + } + if (conn->chosen_exit_name) { const node_t *node = node_get_by_nickname(conn->chosen_exit_name, 1); int opt = conn->chosen_exit_optional; diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 51c985e85e..e90603e16b 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1349,27 +1349,46 @@ networkstatus_consensus_has_excess_connections(void) return 0; } +/* Is there a consensus fetch for flavor resource that's far + * enough along to be attached to a circuit? */ +int +networkstatus_consensus_is_already_downloading(const char *resource) +{ + int answer = 0; + + /* First, get a list of all the dir conns that are fetching a consensus, + * fetching *this* consensus, and are in state "reading" (meaning they + * have already flushed their request onto the socks connection). */ + smartlist_t *fetching_conns = + connection_dir_list_by_purpose_resource_and_state( + DIR_PURPOSE_FETCH_CONSENSUS, resource, DIR_CONN_STATE_CLIENT_READING); + + /* Then, walk through each conn, to see if its linked socks connection + * is in an attached state. We have to check this separately, since with + * the optimistic data feature, fetches can send their request to the + * socks connection and go into state 'reading', even before they're + * attached to any circuit. */ + SMARTLIST_FOREACH_BEGIN(fetching_conns, dir_connection_t *, dirconn) { + /* Do any of these other dir conns have a linked socks conn that is + * attached to a circuit already? */ + connection_t *base = TO_CONN(dirconn); + if (base->linked_conn && + base->linked_conn->type == CONN_TYPE_AP && + !AP_CONN_STATE_IS_UNATTACHED(base->linked_conn->state)) + answer = 1; + } SMARTLIST_FOREACH_END(dirconn); + smartlist_free(fetching_conns); + + return answer; +} + /* Is tor currently downloading a consensus of the usable flavor? */ int networkstatus_consensus_is_downloading_usable_flavor(void) { - const char *usable_resource = networkstatus_get_flavor_name( - usable_consensus_flavor()); - const int consens_conn_usable_count = - connection_dir_count_by_purpose_and_resource( - DIR_PURPOSE_FETCH_CONSENSUS, - usable_resource); - - const int connect_consens_conn_usable_count = - connection_dir_count_by_purpose_resource_and_state( - DIR_PURPOSE_FETCH_CONSENSUS, - usable_resource, - DIR_CONN_STATE_CONNECTING); - if (connect_consens_conn_usable_count < consens_conn_usable_count) { - return 1; - } - - return 0; + const char *resource = + networkstatus_get_flavor_name(usable_consensus_flavor()); + return networkstatus_consensus_is_already_downloading(resource); } /** Given two router status entries for the same router identity, return 1 if diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index f2f8af5c6b..e1cd10bf5b 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -76,6 +76,7 @@ int networkstatus_consensus_can_use_multiple_directories( int networkstatus_consensus_can_use_extra_fallbacks( const or_options_t *options); int networkstatus_consensus_has_excess_connections(void); +int networkstatus_consensus_is_already_downloading(const char *resource); int networkstatus_consensus_is_downloading_usable_flavor(void); #define NSSET_FROM_CACHE 1 From 59da060f10e6abd1681ad00dddd4410d52ce1781 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 13 Apr 2016 01:30:29 -0400 Subject: [PATCH 03/17] use the new function here too --- src/or/networkstatus.c | 15 +-------------- 1 file changed, 1 insertion(+), 14 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index e90603e16b..4a9b36771b 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -865,21 +865,8 @@ update_consensus_networkstatus_downloads(time_t now) && i == usable_consensus_flavor()) { /* Check if we're already downloading a usable consensus */ - int consens_conn_count = - connection_dir_count_by_purpose_and_resource( - DIR_PURPOSE_FETCH_CONSENSUS, - resource); - int connect_consens_conn_count = - connection_dir_count_by_purpose_resource_and_state( - DIR_PURPOSE_FETCH_CONSENSUS, - resource, - DIR_CONN_STATE_CONNECTING); - - /* If not all connections are "connecting", then some are - * downloading. We want to have at most one downloading at a time. */ - if (connect_consens_conn_count < consens_conn_count) { + if (networkstatus_consensus_is_already_downloading(resource)) continue; - } /* Make multiple connections for a bootstrap consensus download. */ update_consensus_bootstrap_multiple_downloads(now, options, From a7665df2f8607403e5e14bf1af7426cedfea01dc Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 13 Apr 2016 02:54:31 -0400 Subject: [PATCH 04/17] close other consensus fetches when we get a consensus not once per second, and only do it when a consensus arrives --- src/or/directory.c | 123 +++++++++++---------------------------------- src/or/directory.h | 1 - src/or/main.c | 11 ---- 3 files changed, 30 insertions(+), 105 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 8486c8c9c0..02db03505f 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -96,6 +96,9 @@ static void directory_initiate_command_rend( time_t if_modified_since, const rend_data_t *rend_query); +static void connection_dir_close_consensus_fetches( + dir_connection_t *except_this_one, const char *resource); + /********* START VARIABLES **********/ /** How far in the future do we allow a directory server to tell us it is @@ -2028,6 +2031,10 @@ connection_dir_client_reached_eof(dir_connection_t *conn) networkstatus_consensus_download_failed(0, flavname); return -1; } + + /* If we launched other fetches for this consensus, cancel them. */ + connection_dir_close_consensus_fetches(conn, flavname); + /* launches router downloads as needed */ routers_update_all_from_networkstatus(now, 3); update_microdescs_from_networkstatus(now); @@ -3680,7 +3687,7 @@ connection_dir_finished_flushing(dir_connection_t *conn) } /* A helper function for connection_dir_close_consensus_conn_if_extra() - * and connection_dir_close_extra_consensus_conns() that returns 0 if + * that returns 0 if * we can't have, or don't want to close, excess consensus connections. */ STATIC int connection_dir_would_close_consensus_conn_helper(void) @@ -3797,103 +3804,33 @@ connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn) return 0; } -/* Clean up excess consensus download connection attempts. - * During bootstrap, or when the bootstrap consensus has just been downloaded, - * if we have more than one active consensus connection: - * - if we don't have a consensus, and we're downloading a consensus, keep an - * earlier connection, or a connection to a fallback directory, and close - * all other connections; - * - if we have just downloaded the bootstrap consensus, and have other - * consensus connections left over, close all of them. +/* We just got a new consensus! If there are other in-progress requests + * for this consensus flavor (for example because we launched several in + * parallel), cancel them. * - * Post-bootstrap consensus connection attempts are initiated one at a time. - * So this function won't close any consensus connection attempts that - * are initiated after bootstrap. + * We do this check here (not just in + * connection_ap_handshake_attach_circuit()) to handle the edge case where + * a consensus fetch begins and ends before some other one tries to attach to + * a circuit, in which case the other one won't know that we're all happy now. + * + * Don't mark the conn that just gave us the consensus -- otherwise we + * would end up double-marking it when it cleans itself up. */ -void -connection_dir_close_extra_consensus_conns(void) +static void +connection_dir_close_consensus_fetches(dir_connection_t *except_this_one, + const char *resource) { - /* Only cleanup connections if there is more than one consensus connection, - * and at least one of those connections is already downloading - * (during bootstrap), or connecting (just after the bootstrap consensus is - * downloaded). - * Post-bootstrap consensus connection attempts won't be cleaned up, because - * they only occur one at a time. */ - if (!connection_dir_would_close_consensus_conn_helper()) { - return; - } - - int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( - time(NULL)); - - const char *usable_resource = networkstatus_get_flavor_name( - usable_consensus_flavor()); - smartlist_t *consens_usable_conns = - connection_dir_list_by_purpose_and_resource( - DIR_PURPOSE_FETCH_CONSENSUS, - usable_resource); - - /* If we want to keep a connection that's downloading, find a connection to - * keep, favouring: - * - connections opened earlier (they are likely to have progressed further) - * - connections to fallbacks (to reduce the load on authorities) */ - dir_connection_t *kept_download_conn = NULL; - int kept_is_authority = 0; - if (we_are_bootstrapping) { - SMARTLIST_FOREACH_BEGIN(consens_usable_conns, - dir_connection_t *, d) { - tor_assert(d); - int d_is_authority = router_digest_is_trusted_dir(d->identity_digest); - /* keep the first connection that is past the connecting state, but - * prefer fallbacks. */ - if (d->base_.state != DIR_CONN_STATE_CONNECTING) { - if (!kept_download_conn || (kept_is_authority && !d_is_authority)) { - kept_download_conn = d; - kept_is_authority = d_is_authority; - /* we've found the earliest fallback, and want to keep it regardless - * of any other connections */ - if (!kept_is_authority) - break; - } - } - } SMARTLIST_FOREACH_END(d); - } - - SMARTLIST_FOREACH_BEGIN(consens_usable_conns, - dir_connection_t *, d) { - tor_assert(d); - /* don't close this connection if it's the one we want to keep */ - if (kept_download_conn && d == kept_download_conn) + smartlist_t *conns_to_close = + connection_dir_list_by_purpose_and_resource(DIR_PURPOSE_FETCH_CONSENSUS, + resource); + SMARTLIST_FOREACH_BEGIN(conns_to_close, dir_connection_t *, d) { + if (d == except_this_one) continue; - /* mark all other connections for close */ - if (!d->base_.marked_for_close) { - connection_close_immediate(&d->base_); - connection_mark_for_close(&d->base_); - } + log_info(LD_DIR, "Closing consensus fetch (to %s) since one " + "has just arrived.", TO_CONN(d)->address); + connection_mark_for_close(TO_CONN(d)); } SMARTLIST_FOREACH_END(d); - - smartlist_free(consens_usable_conns); - consens_usable_conns = NULL; - - /* make sure we've closed all excess connections */ - const int final_connecting_conn_count = - connection_dir_count_by_purpose_resource_and_state( - DIR_PURPOSE_FETCH_CONSENSUS, - usable_resource, - DIR_CONN_STATE_CONNECTING); - if (final_connecting_conn_count > 0) { - log_warn(LD_BUG, "Expected 0 consensus connections connecting after " - "cleanup, got %d.", final_connecting_conn_count); - } - const int expected_final_conn_count = (we_are_bootstrapping ? 1 : 0); - const int final_conn_count = - connection_dir_count_by_purpose_and_resource( - DIR_PURPOSE_FETCH_CONSENSUS, - usable_resource); - if (final_conn_count > expected_final_conn_count) { - log_warn(LD_BUG, "Expected %d consensus connections after cleanup, got " - "%d.", expected_final_conn_count, final_connecting_conn_count); - } + smartlist_free(conns_to_close); } /** Connected handler for directory connections: begin sending data to the diff --git a/src/or/directory.h b/src/or/directory.h index c4edbb5c0f..9a17474a6b 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -80,7 +80,6 @@ void directory_initiate_command(const tor_addr_t *or_addr, uint16_t or_port, 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) diff --git a/src/or/main.c b/src/or/main.c index bf5b2b823a..3d84de6633 100644 --- a/src/or/main.c +++ b/src/or/main.c @@ -1484,17 +1484,6 @@ run_scheduled_events(time_t now) dirvote_act(options, now); } - /* 2d. Cleanup excess consensus bootstrap connections every second. - * connection_dir_close_consensus_conn_if_extra() closes some connections - * that are clearly excess, but this check is more thorough. - * This only closes connections if there is more than one consensus - * connection, and at least one of those connections is already downloading - * (during bootstrap), or connecting (just after the bootstrap consensus is - * downloaded). - * It won't close any consensus connections initiated after bootstrap, - * because those attempts are made one at a time. */ - connection_dir_close_extra_consensus_conns(); - /* 3a. Every second, we examine pending circuits and prune the * ones which have been pending for more than a few seconds. * We do this before step 4, so it can try building more if From e230e80ab3021caf8153d2ba09bab5cb185366dc Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 13 Apr 2016 03:05:54 -0400 Subject: [PATCH 05/17] get rid of the scattered checks to cancel a consensus fetch We'll back off from the request in connection_ap_handshake_attach_circuit, or cancel it in connection_dir_close_consensus_fetches, and those are the only places we need to check. --- src/or/directory.c | 104 --------------------------------------------- src/or/directory.h | 2 - 2 files changed, 106 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 02db03505f..2008d07f3e 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -1219,11 +1219,6 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port, conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING; /* fall through */ case 0: - /* Close this connection if there's another consensus connection - * downloading (during bootstrap), or connecting (after bootstrap). */ - if (connection_dir_close_consensus_conn_if_extra(conn)) { - return; - } /* queue the command on the outbuf */ directory_send_command(conn, dir_purpose, 1, resource, payload, payload_len, @@ -1271,11 +1266,6 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port, connection_mark_for_close(TO_CONN(conn)); return; } - /* Close this connection if there's another consensus connection - * downloading (during bootstrap), or connecting (after bootstrap). */ - if (connection_dir_close_consensus_conn_if_extra(conn)) { - return; - } conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING; /* queue the command on the outbuf */ directory_send_command(conn, dir_purpose, 0, resource, @@ -3686,41 +3676,6 @@ connection_dir_finished_flushing(dir_connection_t *conn) return 0; } -/* A helper function for connection_dir_close_consensus_conn_if_extra() - * that returns 0 if - * we can't have, or don't want to close, excess consensus connections. */ -STATIC int -connection_dir_would_close_consensus_conn_helper(void) -{ - const or_options_t *options = get_options(); - - /* we're only interested in closing excess connections if we could - * have created any in the first place */ - if (!networkstatus_consensus_can_use_multiple_directories(options)) { - return 0; - } - - /* We want to close excess connections downloading a consensus. - * If there aren't any excess, we don't have anything to close. */ - if (!networkstatus_consensus_has_excess_connections()) { - return 0; - } - - /* If we have excess connections, but none of them are downloading a - * consensus, and we are still bootstrapping (that is, we have no usable - * consensus), we don't want to close any until one starts downloading. */ - if (!networkstatus_consensus_is_downloading_usable_flavor() - && networkstatus_consensus_is_bootstrapping(time(NULL))) { - return 0; - } - - /* If we have just stopped bootstrapping (that is, just parsed a consensus), - * we might still have some excess connections hanging around. So we still - * have to check if we want to close any, even if we've stopped - * bootstrapping. */ - return 1; -} - /* Check if we would close excess consensus connections. If we would, any * new consensus connection would become excess immediately, so return 1. * Otherwise, return 0. */ @@ -3751,59 +3706,6 @@ connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose) return 0; } -/* Check if we have more than one consensus download connection attempt, and - * close conn: - * - if we don't have a consensus, and we're downloading a consensus, and conn - * is not downloading a consensus yet; - * - if we do have a consensus, and there's more than one consensus connection. - * - * Post-bootstrap consensus connection attempts are initiated one at a time. - * So this function won't close any consensus connection attempts that - * are initiated after bootstrap. - */ -int -connection_dir_close_consensus_conn_if_extra(dir_connection_t *conn) -{ - tor_assert(conn); - tor_assert(conn->base_.type == CONN_TYPE_DIR); - - /* We're not interested in connections that aren't fetching a consensus. */ - if (conn->base_.purpose != DIR_PURPOSE_FETCH_CONSENSUS) { - return 0; - } - - /* The connection has already been closed */ - if (conn->base_.marked_for_close) { - return 0; - } - - /* Only close this connection if there's another consensus connection - * downloading (during bootstrap), or connecting (after bootstrap). - * Post-bootstrap consensus connection attempts won't be closed, because - * they only occur one at a time. */ - if (!connection_dir_would_close_consensus_conn_helper()) { - return 0; - } - - const int we_are_bootstrapping = networkstatus_consensus_is_bootstrapping( - time(NULL)); - - /* We don't want to check other connections to see if they are downloading, - * as this is prone to race-conditions. So leave it for - * connection_dir_close_extra_consensus_conns(() to clean up. - * - * But if conn has just started connecting, or we have a consensus already, - * we can be sure it's not needed any more. */ - if (!we_are_bootstrapping - || conn->base_.state == DIR_CONN_STATE_CONNECTING) { - connection_close_immediate(&conn->base_); - connection_mark_for_close(&conn->base_); - return -1; - } - - return 0; -} - /* We just got a new consensus! If there are other in-progress requests * for this consensus flavor (for example because we launched several in * parallel), cancel them. @@ -3847,12 +3749,6 @@ connection_dir_finished_connecting(dir_connection_t *conn) log_debug(LD_HTTP,"Dir connection to router %s:%u established.", conn->base_.address,conn->base_.port); - /* Close this connection if there's another consensus connection - * downloading (during bootstrap), or connecting (after bootstrap). */ - if (connection_dir_close_consensus_conn_if_extra(conn)) { - return -1; - } - /* start flushing conn */ conn->base_.state = DIR_CONN_STATE_CLIENT_SENDING; return 0; diff --git a/src/or/directory.h b/src/or/directory.h index 9a17474a6b..3254e073d2 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -79,7 +79,6 @@ void directory_initiate_command(const tor_addr_t *or_addr, uint16_t or_port, 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); #define DSR_HEX (1<<0) #define DSR_BASE64 (1<<1) @@ -146,7 +145,6 @@ STATIC int directory_handle_command_get(dir_connection_t *conn, const char *headers, const char *req_body, size_t req_body_len); -STATIC int connection_dir_would_close_consensus_conn_helper(void); STATIC int download_status_schedule_get_delay(download_status_t *dls, const smartlist_t *schedule, time_t now); From bcae392e0edc682990da71524a0802cf79e3e698 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Wed, 13 Apr 2016 03:13:12 -0400 Subject: [PATCH 06/17] avoid another redundant check we should avoid launching a consensus fetch if we don't want one, but if we do end up with an extra one, we should let the other checks take care of it. --- src/or/directory.c | 39 +-------------------------------------- src/or/directory.h | 1 - src/or/networkstatus.c | 9 --------- src/or/networkstatus.h | 1 - 4 files changed, 1 insertion(+), 49 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 2008d07f3e..89b08223d2 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -1173,12 +1173,6 @@ directory_initiate_command_rend(const tor_addr_port_t *or_addr_port, return; } - /* ensure we don't make excess connections when we're already downloading - * a consensus during bootstrap */ - if (connection_dir_avoid_extra_connection_for_purpose(dir_purpose)) { - return; - } - conn = dir_connection_new(tor_addr_family(&addr)); /* set up conn so it's got all the data we need to remember */ @@ -3676,36 +3670,6 @@ connection_dir_finished_flushing(dir_connection_t *conn) return 0; } -/* Check if we would close excess consensus connections. If we would, any - * new consensus connection would become excess immediately, so return 1. - * Otherwise, return 0. */ -int -connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose) -{ - const or_options_t *options = get_options(); - - /* We're not interested in connections that aren't fetching a consensus. */ - if (purpose != DIR_PURPOSE_FETCH_CONSENSUS) { - return 0; - } - - /* we're only interested in avoiding excess connections if we could - * have created any in the first place */ - if (!networkstatus_consensus_can_use_multiple_directories(options)) { - return 0; - } - - /* If there are connections downloading a consensus, and we are still - * bootstrapping (that is, we have no usable consensus), we can be sure that - * any further connections would be excess. */ - if (networkstatus_consensus_is_downloading_usable_flavor() - && networkstatus_consensus_is_bootstrapping(time(NULL))) { - return 1; - } - - return 0; -} - /* We just got a new consensus! If there are other in-progress requests * for this consensus flavor (for example because we launched several in * parallel), cancel them. @@ -3736,8 +3700,7 @@ connection_dir_close_consensus_fetches(dir_connection_t *except_this_one, } /** Connected handler for directory connections: begin sending data to the - * server, and return 0, or, if the connection is an excess bootstrap - * connection, close all excess bootstrap connections. + * server, and return 0. * Only used when connections don't immediately connect. */ int connection_dir_finished_connecting(dir_connection_t *conn) diff --git a/src/or/directory.h b/src/or/directory.h index 3254e073d2..7646cac03f 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -78,7 +78,6 @@ void directory_initiate_command(const tor_addr_t *or_addr, uint16_t or_port, const char *resource, const char *payload, size_t payload_len, time_t if_modified_since); -int connection_dir_avoid_extra_connection_for_purpose(unsigned int purpose); #define DSR_HEX (1<<0) #define DSR_BASE64 (1<<1) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 4a9b36771b..3b38c07ea0 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1369,15 +1369,6 @@ networkstatus_consensus_is_already_downloading(const char *resource) return answer; } -/* Is tor currently downloading a consensus of the usable flavor? */ -int -networkstatus_consensus_is_downloading_usable_flavor(void) -{ - const char *resource = - networkstatus_get_flavor_name(usable_consensus_flavor()); - return networkstatus_consensus_is_already_downloading(resource); -} - /** Given two router status entries for the same router identity, return 1 if * if the contents have changed between them. Otherwise, return 0. */ static int diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index e1cd10bf5b..667bc1f062 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -77,7 +77,6 @@ int networkstatus_consensus_can_use_extra_fallbacks( const or_options_t *options); int networkstatus_consensus_has_excess_connections(void); int networkstatus_consensus_is_already_downloading(const char *resource); -int networkstatus_consensus_is_downloading_usable_flavor(void); #define NSSET_FROM_CACHE 1 #define NSSET_WAS_WAITING_FOR_CERTS 2 From c98fbd4169d70033c5058994cab5cfbd24cf4e78 Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 14 Apr 2016 02:18:25 -0400 Subject: [PATCH 07/17] remove some more unused code --- src/or/connection.c | 45 --------------------------------------------- src/or/connection.h | 7 ------- 2 files changed, 52 deletions(-) diff --git a/src/or/connection.c b/src/or/connection.c index 118e239176..4fbbaf1abd 100644 --- a/src/or/connection.c +++ b/src/or/connection.c @@ -4438,32 +4438,6 @@ connection_get_by_type_state_rendquery(int type, int state, )); } -#define CONN_FIRST_AND_FREE_TEMPLATE(sl) \ - STMT_BEGIN \ - if (smartlist_len(sl) > 0) { \ - void *first_item = smartlist_get(sl, 0); \ - smartlist_free(sl); \ - return first_item; \ - } else { \ - smartlist_free(sl); \ - return NULL; \ - } \ - STMT_END - -/** Return a directory connection (if any one exists) that is fetching - * the item described by purpose/resource, otherwise return NULL. - */ -dir_connection_t * -connection_dir_get_by_purpose_and_resource( - int purpose, - const char *resource) -{ - smartlist_t *conns = connection_dir_list_by_purpose_and_resource( - purpose, - resource); - CONN_FIRST_AND_FREE_TEMPLATE(conns); -} - /** Return a new smartlist of dir_connection_t * from get_connection_array() * that satisfy conn_test on connection_t *conn_var, and dirconn_test on * dir_connection_t *dirconn_var. conn_var must be of CONN_TYPE_DIR and not @@ -4504,25 +4478,6 @@ connection_dir_list_by_purpose_and_resource( dirconn->requested_resource)); } -/** Return a directory connection (if any one exists) that is fetching - * the item described by purpose/resource/state, - * otherwise return NULL. */ -dir_connection_t * -connection_dir_get_by_purpose_resource_and_state( - int purpose, - const char *resource, - int state) -{ - smartlist_t *conns = - connection_dir_list_by_purpose_resource_and_state( - purpose, - resource, - state); - CONN_FIRST_AND_FREE_TEMPLATE(conns); -} - -#undef CONN_FIRST_AND_FREE_TEMPLATE - /** Return a list of directory connections that are fetching the item * described by purpose/resource/state. If there are * none, return an empty list. This list must be freed using smartlist_free, diff --git a/src/or/connection.h b/src/or/connection.h index 45175cd5a2..4835235fba 100644 --- a/src/or/connection.h +++ b/src/or/connection.h @@ -192,13 +192,6 @@ MOCK_DECL(connection_t *,connection_get_by_type_addr_port_purpose,(int type, connection_t *connection_get_by_type_state(int type, int state); connection_t *connection_get_by_type_state_rendquery(int type, int state, const char *rendquery); -dir_connection_t *connection_dir_get_by_purpose_and_resource( - int purpose, - const char *resource); -dir_connection_t *connection_dir_get_by_purpose_resource_and_state( - int purpose, - const char *resource, - int state); smartlist_t *connection_dir_list_by_purpose_and_resource( int purpose, const char *resource); From d5a96286c2877ab56455ea5183d1df2e39453cde Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 14 Apr 2016 02:29:12 -0400 Subject: [PATCH 08/17] simplify more -- we only call these funcs when bootstrapping --- src/or/networkstatus.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 3b38c07ea0..6b4085299c 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -121,8 +121,7 @@ static int have_warned_about_new_version = 0; static void routerstatus_list_update_named_server_map(void); static void update_consensus_bootstrap_multiple_downloads( time_t now, - const or_options_t *options, - int we_are_bootstrapping); + const or_options_t *options); /** Forget that we've warned about anything networkstatus-related, so we will * give fresh warnings if the same behavior happens again. */ @@ -869,8 +868,7 @@ update_consensus_networkstatus_downloads(time_t now) continue; /* Make multiple connections for a bootstrap consensus download. */ - update_consensus_bootstrap_multiple_downloads(now, options, - we_are_bootstrapping); + update_consensus_bootstrap_multiple_downloads(now, options); } else { /* Check if we failed downloading a consensus too recently */ int max_dl_tries = consensus_max_download_tries(options, @@ -909,12 +907,10 @@ static void update_consensus_bootstrap_attempt_downloads( time_t now, const or_options_t *options, - int we_are_bootstrapping, download_status_t *dls, download_want_authority_t want_authority) { - int max_dl_tries = consensus_max_download_tries(options, - we_are_bootstrapping); + int max_dl_tries = consensus_max_download_tries(options, 1); const char *resource = networkstatus_get_flavor_name( usable_consensus_flavor()); @@ -947,8 +943,7 @@ update_consensus_bootstrap_attempt_downloads( */ static void update_consensus_bootstrap_multiple_downloads(time_t now, - const or_options_t *options, - int we_are_bootstrapping) + const or_options_t *options) { const int usable_flavor = usable_consensus_flavor(); @@ -957,12 +952,6 @@ update_consensus_bootstrap_multiple_downloads(time_t now, return; } - /* If we've managed to validate a usable consensus, don't make additional - * connections. */ - if (!we_are_bootstrapping) { - return; - } - /* Launch concurrent consensus download attempt(s) based on the mirror and * authority schedules. Try the mirror first - this makes it slightly more * likely that we'll connect to the fallback first, and then end the @@ -981,8 +970,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now, if (!check_consensus_waiting_for_certs(usable_flavor, now, dls_f)) { /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */ - update_consensus_bootstrap_attempt_downloads(now, options, - we_are_bootstrapping, dls_f, + update_consensus_bootstrap_attempt_downloads(now, options, dls_f, DL_WANT_ANY_DIRSERVER); } } @@ -992,8 +980,7 @@ update_consensus_bootstrap_multiple_downloads(time_t now, &consensus_bootstrap_dl_status[CONSENSUS_BOOTSTRAP_SOURCE_AUTHORITY]; if (!check_consensus_waiting_for_certs(usable_flavor, now, dls_a)) { - update_consensus_bootstrap_attempt_downloads(now, options, - we_are_bootstrapping, dls_a, + update_consensus_bootstrap_attempt_downloads(now, options, dls_a, DL_WANT_AUTHORITY); } } From 1f72653544272e24d685bd81abe2da5d32b67caf Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Thu, 14 Apr 2016 03:03:47 -0400 Subject: [PATCH 09/17] fix a bug where relays would use the aggressive client bootstrapping retry number --- src/or/networkstatus.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 6b4085299c..3d8bb55fc3 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -871,8 +871,7 @@ update_consensus_networkstatus_downloads(time_t now) update_consensus_bootstrap_multiple_downloads(now, options); } else { /* Check if we failed downloading a consensus too recently */ - int max_dl_tries = consensus_max_download_tries(options, - we_are_bootstrapping); + int max_dl_tries = consensus_max_download_tries(options, 0); /* Let's make sure we remembered to update consensus_dl_status */ tor_assert(consensus_dl_status[i].schedule == DL_SCHED_CONSENSUS); From aa6341d4b9159e02825a03f1490ce6b731c2a90e Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Fri, 6 May 2016 09:55:06 -0400 Subject: [PATCH 10/17] stop looping once we know what the answer will be suggested during code review by dgoulet --- src/or/networkstatus.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 3d8bb55fc3..074922bcf0 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1347,8 +1347,10 @@ networkstatus_consensus_is_already_downloading(const char *resource) connection_t *base = TO_CONN(dirconn); if (base->linked_conn && base->linked_conn->type == CONN_TYPE_AP && - !AP_CONN_STATE_IS_UNATTACHED(base->linked_conn->state)) + !AP_CONN_STATE_IS_UNATTACHED(base->linked_conn->state)) { answer = 1; + break; /* stop looping, because we know the answer will be yes */ + } } SMARTLIST_FOREACH_END(dirconn); smartlist_free(fetching_conns); From 53aaed81dd34f227366b0a0c9b143c4c43a5529e Mon Sep 17 00:00:00 2001 From: Roger Dingledine Date: Tue, 10 May 2016 11:16:30 -0400 Subject: [PATCH 11/17] get rid of another no-longer-used function --- src/or/networkstatus.c | 34 ---------------------------------- src/or/networkstatus.h | 1 - 2 files changed, 35 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 074922bcf0..a9c8037e41 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1288,40 +1288,6 @@ networkstatus_consensus_can_use_extra_fallbacks(const or_options_t *options) > smartlist_len(router_get_trusted_dir_servers()))); } -/* Check if there is more than 1 consensus connection retrieving the usable - * consensus flavor. If so, return 1, if not, return 0. - * - * During normal operation, Tor only makes one consensus download - * connection. But clients can make multiple simultaneous consensus - * connections to improve bootstrap speed and reliability. - * - * If there is more than one connection, we must have connections left - * over from bootstrapping. However, some of the connections may have - * completed and been cleaned up, so it is not sufficient to check the - * return value of this function to see if a client could make multiple - * bootstrap connections. Use - * networkstatus_consensus_can_use_multiple_directories() - * and networkstatus_consensus_is_bootstrapping(). */ -int -networkstatus_consensus_has_excess_connections(void) -{ - const char *usable_resource = networkstatus_get_flavor_name( - usable_consensus_flavor()); - const int consens_conn_usable_count = - connection_dir_count_by_purpose_and_resource( - DIR_PURPOSE_FETCH_CONSENSUS, - usable_resource); - /* The maximum number of connections we want downloading a usable consensus - * Always 1, whether bootstrapping or not. */ - const int max_expected_consens_conn_usable_count = 1; - - if (consens_conn_usable_count > max_expected_consens_conn_usable_count) { - return 1; - } - - return 0; -} - /* Is there a consensus fetch for flavor resource that's far * enough along to be attached to a circuit? */ int diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 667bc1f062..609fb85bcb 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -75,7 +75,6 @@ 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_already_downloading(const char *resource); #define NSSET_FROM_CACHE 1 From 84ab26c320151bde1fc52906266126aa8d7780a3 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 18 May 2016 17:50:46 -0400 Subject: [PATCH 12/17] Stop downloading consensuses when a consensus has been downloaded Previosuly, during bootstrap, we would continue to download consensuses if we had a consensus, but didn't have the certificates to validate it. --- src/or/networkstatus.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index a9c8037e41..5e32003afb 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1247,16 +1247,30 @@ networkstatus_get_reasonably_live_consensus(time_t now, int flavor) return NULL; } -/** Check if we're bootstrapping a consensus download. This means that we are - * only using the authorities and fallback directory mirrors to download the - * consensus flavour we'll use. */ +/** Check if we need to download a consensus during tor's bootstrap phase. + * If we have no consensus, or our consensus is unusably old, return 1. + * As soon as we have received a consensus, return 0, even if we don't have + * enough certificates to validate it. */ int networkstatus_consensus_is_bootstrapping(time_t now) { - /* If we don't have a consensus, we must still be bootstrapping */ - return !networkstatus_get_reasonably_live_consensus( - now, - usable_consensus_flavor()); + /* If we have a validated, reasonably live consensus, we're not + * bootstrapping a consensus at all. */ + if (networkstatus_get_reasonably_live_consensus( + now, + usable_consensus_flavor())) { + return 0; + } + + /* If we have a consensus, but we're waiting for certificates, + * we're not waiting for a consensus download while bootstrapping. */ + if (consensus_is_waiting_for_certs()) { + return 0; + } + + /* If we have no consensus, or our consensus is very old, we are + * bootstrapping, and we need to download a consensus. */ + return 1; } /** Check if we can use multiple directories for a consensus download. From 8dc8d712260a16eb9811b57abd4928081316f495 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 18 May 2016 18:04:02 -0400 Subject: [PATCH 13/17] Changes file for bug 18809 --- changes/bug18809 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 changes/bug18809 diff --git a/changes/bug18809 b/changes/bug18809 new file mode 100644 index 0000000000..6ee50887a0 --- /dev/null +++ b/changes/bug18809 @@ -0,0 +1,14 @@ + o Major bugfixes (bootstrap): + - Check if bootstrap consensus downloads are still needed + when the linked connection attaches. This prevents tor + making unnecessary begindir-style connections, which are + the only directory connections tor clients make since + #18483 was merged. + - Fix some edge cases where consensus download connections + may not have been closed, even though they were not needed. + - Make relays retry consensus downloads the correct number of + times, rather than the more aggresive client retry count. + - Stop downloading consensuses when we have a consensus, + even if we don't have all the certificates for it yet. + Closes ticket 18943, bugfix on #4483 in 0.2.8.1-alpha, + patches by arma and teor. From ab0a7e2961378ff0fb7f5061d6b1cdeebe7afa98 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 18 May 2016 18:07:40 -0400 Subject: [PATCH 14/17] Remove consensus_max_download_tries by refactoring No behaviour change This function is used twice. The code is simpler if we split it up and inline it where it is used. --- src/or/networkstatus.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 5e32003afb..5c9283004e 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -791,26 +791,6 @@ check_consensus_waiting_for_certs(int flavor, time_t now, return 0; } -/* Return the maximum download tries for a consensus, based on options and - * whether we_are_bootstrapping. */ -static int -consensus_max_download_tries(const or_options_t *options, - int we_are_bootstrapping) -{ - int use_fallbacks = networkstatus_consensus_can_use_extra_fallbacks(options); - - if (we_are_bootstrapping) { - if (use_fallbacks) { - return options->ClientBootstrapConsensusMaxDownloadTries; - } else { - return - options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries; - } - } - - return options->TestingConsensusMaxDownloadTries; -} - /** If we want to download a fresh consensus, launch a new download as * appropriate. */ static void @@ -871,7 +851,7 @@ update_consensus_networkstatus_downloads(time_t now) update_consensus_bootstrap_multiple_downloads(now, options); } else { /* Check if we failed downloading a consensus too recently */ - int max_dl_tries = consensus_max_download_tries(options, 0); + int max_dl_tries = options->TestingConsensusMaxDownloadTries; /* Let's make sure we remembered to update consensus_dl_status */ tor_assert(consensus_dl_status[i].schedule == DL_SCHED_CONSENSUS); @@ -909,7 +889,13 @@ update_consensus_bootstrap_attempt_downloads( download_status_t *dls, download_want_authority_t want_authority) { - int max_dl_tries = consensus_max_download_tries(options, 1); + int use_fallbacks = networkstatus_consensus_can_use_extra_fallbacks(options); + int max_dl_tries = options->ClientBootstrapConsensusMaxDownloadTries; + if (!use_fallbacks) { + max_dl_tries = + options->ClientBootstrapConsensusAuthorityOnlyMaxDownloadTries; + } + const char *resource = networkstatus_get_flavor_name( usable_consensus_flavor()); From 4254d0297c53155d582a51976b8e5bb41abfced6 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 18 May 2016 20:21:31 -0400 Subject: [PATCH 15/17] Update unit tests for multiple bootstrap connections --- src/test/test_connection.c | 403 +++++++++++++++++++++++-------------- 1 file changed, 251 insertions(+), 152 deletions(-) diff --git a/src/test/test_connection.c b/src/test/test_connection.c index 6f7aef879c..5aa7964ab6 100644 --- a/src/test/test_connection.c +++ b/src/test/test_connection.c @@ -11,6 +11,7 @@ #include "connection.h" #include "main.h" +#include "microdesc.h" #include "networkstatus.h" #include "rendcache.h" #include "directory.h" @@ -54,7 +55,11 @@ static int test_conn_get_rsrc_teardown(const struct testcase_t *tc, #define TEST_CONN_RSRC_STATE_SUCCESSFUL (DIR_CONN_STATE_CLIENT_FINISHED) #define TEST_CONN_RSRC_2 (networkstatus_get_flavor_name(FLAV_NS)) -#define TEST_CONN_DL_STATE (DIR_CONN_STATE_CLIENT_SENDING) +#define TEST_CONN_DL_STATE (DIR_CONN_STATE_CLIENT_READING) + +/* see AP_CONN_STATE_IS_UNATTACHED() */ +#define TEST_CONN_UNATTACHED_STATE (AP_CONN_STATE_CIRCUIT_WAIT) +#define TEST_CONN_ATTACHED_STATE (AP_CONN_STATE_CONNECT_WAIT) #define TEST_CONN_FD_INIT 50 static int mock_connection_connect_sockaddr_called = 0; @@ -109,27 +114,25 @@ test_conn_lookup_addr_helper(const char *address, int family, tor_addr_t *addr) tor_addr_make_null(addr, TEST_CONN_FAMILY); } -static void * -test_conn_get_basic_setup(const struct testcase_t *tc) +static connection_t * +test_conn_get_connection(uint8_t state, uint8_t type, uint8_t purpose) { connection_t *conn = NULL; tor_addr_t addr; int socket_err = 0; int in_progress = 0; - (void)tc; MOCK(connection_connect_sockaddr, mock_connection_connect_sockaddr); init_connection_lists(); - conn = connection_new(TEST_CONN_TYPE, TEST_CONN_FAMILY); + conn = connection_new(type, TEST_CONN_FAMILY); tt_assert(conn); test_conn_lookup_addr_helper(TEST_CONN_ADDRESS, TEST_CONN_FAMILY, &addr); tt_assert(!tor_addr_is_null(&addr)); - /* XXXX - connection_connect doesn't set these, should it? */ tor_addr_copy_tight(&conn->addr, &addr); conn->port = TEST_CONN_PORT; mock_connection_connect_sockaddr_called = 0; @@ -140,8 +143,8 @@ test_conn_get_basic_setup(const struct testcase_t *tc) tt_assert(in_progress == 0 || in_progress == 1); /* fake some of the attributes so the connection looks OK */ - conn->state = TEST_CONN_STATE; - conn->purpose = TEST_CONN_BASIC_PURPOSE; + conn->state = state; + conn->purpose = purpose; assert_connection_ok(conn, time(NULL)); UNMOCK(connection_connect_sockaddr); @@ -151,12 +154,17 @@ test_conn_get_basic_setup(const struct testcase_t *tc) /* On failure */ done: UNMOCK(connection_connect_sockaddr); - test_conn_get_basic_teardown(tc, conn); - - /* Returning NULL causes the unit test to fail */ return NULL; } +static void * +test_conn_get_basic_setup(const struct testcase_t *tc) +{ + (void)tc; + return test_conn_get_connection(TEST_CONN_STATE, TEST_CONN_TYPE, + TEST_CONN_BASIC_PURPOSE); +} + static int test_conn_get_basic_teardown(const struct testcase_t *tc, void *arg) { @@ -186,9 +194,8 @@ test_conn_get_basic_teardown(const struct testcase_t *tc, void *arg) connection_close_immediate(conn->linked_conn); connection_mark_for_close(conn->linked_conn); } - conn->linked_conn->linked_conn = NULL; - connection_free(conn->linked_conn); - conn->linked_conn = NULL; + + close_closeable_connections(); } /* We didn't set the events up properly, so we can't use event_del() in @@ -222,7 +229,10 @@ static void * test_conn_get_rend_setup(const struct testcase_t *tc) { dir_connection_t *conn = DOWNCAST(dir_connection_t, - test_conn_get_basic_setup(tc)); + test_conn_get_connection( + TEST_CONN_STATE, + TEST_CONN_TYPE, + TEST_CONN_REND_PURPOSE)); tt_assert(conn); assert_connection_ok(&conn->base_, time(NULL)); @@ -235,7 +245,6 @@ test_conn_get_rend_setup(const struct testcase_t *tc) TEST_CONN_REND_ADDR, REND_SERVICE_ID_LEN_BASE32+1); conn->rend_data->hsdirs_fp = smartlist_new(); - conn->base_.purpose = TEST_CONN_REND_PURPOSE; assert_connection_ok(&conn->base_, time(NULL)); return conn; @@ -266,42 +275,64 @@ test_conn_get_rend_teardown(const struct testcase_t *tc, void *arg) return rv; } -static void * -test_conn_get_rsrc_setup(const struct testcase_t *tc) +static dir_connection_t * +test_conn_download_status_add_a_connection(const char *resource) { dir_connection_t *conn = DOWNCAST(dir_connection_t, - test_conn_get_basic_setup(tc)); + test_conn_get_connection( + TEST_CONN_STATE, + TEST_CONN_TYPE, + TEST_CONN_RSRC_PURPOSE)); + tt_assert(conn); assert_connection_ok(&conn->base_, time(NULL)); - /* TODO: use the canonical function to do this - maybe? */ - conn->requested_resource = tor_strdup(TEST_CONN_RSRC); - conn->base_.purpose = TEST_CONN_RSRC_PURPOSE; + /* Replace the existing resource with the one we want */ + if (resource) { + if (conn->requested_resource) { + tor_free(conn->requested_resource); + } + conn->requested_resource = tor_strdup(resource); + assert_connection_ok(&conn->base_, time(NULL)); + } - assert_connection_ok(&conn->base_, time(NULL)); return conn; - /* On failure */ done: - test_conn_get_rend_teardown(tc, conn); - /* Returning NULL causes the unit test to fail */ + test_conn_get_rsrc_teardown(NULL, conn); return NULL; } +static void * +test_conn_get_rsrc_setup(const struct testcase_t *tc) +{ + (void)tc; + return test_conn_download_status_add_a_connection(TEST_CONN_RSRC); +} + static int test_conn_get_rsrc_teardown(const struct testcase_t *tc, void *arg) { - dir_connection_t *conn = DOWNCAST(dir_connection_t, arg); int rv = 0; + connection_t *conn = (connection_t *)arg; tt_assert(conn); - assert_connection_ok(&conn->base_, time(NULL)); + assert_connection_ok(conn, time(NULL)); - /* avoid a last-ditch attempt to refetch the consensus */ - conn->base_.state = TEST_CONN_RSRC_STATE_SUCCESSFUL; + if (conn->type == CONN_TYPE_DIR) { + dir_connection_t *dir_conn = DOWNCAST(dir_connection_t, arg); + + tt_assert(dir_conn); + assert_connection_ok(&dir_conn->base_, time(NULL)); + + /* avoid a last-ditch attempt to refetch the consensus */ + dir_conn->base_.state = TEST_CONN_RSRC_STATE_SUCCESSFUL; + assert_connection_ok(&dir_conn->base_, time(NULL)); + } /* connection_free_() cleans up requested_resource */ - rv = test_conn_get_basic_teardown(tc, arg); + rv = test_conn_get_basic_teardown(tc, conn); + done: return rv; } @@ -336,14 +367,30 @@ test_conn_download_status_teardown(const struct testcase_t *tc, void *arg) return rv; } -static dir_connection_t * -test_conn_download_status_add_a_connection(void) +/* Like connection_ap_make_link(), but does much less */ +static connection_t * +test_conn_get_linked_connection(connection_t *l_conn, uint8_t state) { - dir_connection_t *conn = DOWNCAST(dir_connection_t, - test_conn_get_rsrc_setup(NULL)); + tt_assert(l_conn); + assert_connection_ok(l_conn, time(NULL)); + + /* AP connections don't seem to have purposes */ + connection_t *conn = test_conn_get_connection(state, CONN_TYPE_AP, + 0); tt_assert(conn); - assert_connection_ok(&conn->base_, time(NULL)); + assert_connection_ok(conn, time(NULL)); + + conn->linked = 1; + l_conn->linked = 1; + conn->linked_conn = l_conn; + l_conn->linked_conn = conn; + /* we never opened a real socket, so we can just overwrite it */ + conn->s = TOR_INVALID_SOCKET; + l_conn->s = TOR_INVALID_SOCKET; + + assert_connection_ok(conn, time(NULL)); + assert_connection_ok(l_conn, time(NULL)); return conn; @@ -524,44 +571,6 @@ test_conn_get_rsrc(void *arg) tt_assert(conn); assert_connection_ok(&conn->base_, time(NULL)); - tt_assert(connection_dir_get_by_purpose_and_resource( - conn->base_.purpose, - conn->requested_resource) - == conn); - tt_assert(connection_dir_get_by_purpose_and_resource( - TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC) - == conn); - tt_assert(connection_dir_get_by_purpose_and_resource( - !conn->base_.purpose, - "") - == NULL); - tt_assert(connection_dir_get_by_purpose_and_resource( - !TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC_2) - == NULL); - - tt_assert(connection_dir_get_by_purpose_resource_and_state( - conn->base_.purpose, - conn->requested_resource, - conn->base_.state) - == conn); - tt_assert(connection_dir_get_by_purpose_resource_and_state( - TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC, - TEST_CONN_STATE) - == conn); - tt_assert(connection_dir_get_by_purpose_resource_and_state( - !conn->base_.purpose, - "", - !conn->base_.state) - == NULL); - tt_assert(connection_dir_get_by_purpose_resource_and_state( - !TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC_2, - !TEST_CONN_STATE) - == NULL); - sl_is_conn_assert(connection_dir_list_by_purpose_and_resource( conn->base_.purpose, conn->requested_resource), @@ -641,107 +650,190 @@ test_conn_get_rsrc(void *arg) static void test_conn_download_status(void *arg) { - (void)arg; dir_connection_t *conn = NULL; dir_connection_t *conn2 = NULL; dir_connection_t *conn3 = NULL; + dir_connection_t *conn4 = NULL; + connection_t *ap_conn = NULL; + connection_t *ap_conn2 = NULL; + /* we never create an ap_conn for conn3 */ + connection_t *ap_conn4 = NULL; - /* no connections, no excess, not downloading */ - tt_assert(networkstatus_consensus_has_excess_connections() == 0); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 0); + consensus_flavor_t usable_flavor = (consensus_flavor_t)arg; - /* one connection, no excess, not downloading */ - conn = test_conn_download_status_add_a_connection(); - tt_assert(networkstatus_consensus_has_excess_connections() == 0); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 0); + /* The "other flavor" trick only works if there are two flavors */ + tor_assert(N_CONSENSUS_FLAVORS == 2); + consensus_flavor_t other_flavor = ((usable_flavor == FLAV_NS) + ? FLAV_MICRODESC + : FLAV_NS); + const char *res = networkstatus_get_flavor_name(usable_flavor); + const char *other_res = networkstatus_get_flavor_name(other_flavor); - /* one connection, no excess, but downloading */ + /* no connections */ + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* one connection, not downloading */ + conn = test_conn_download_status_add_a_connection(res); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 1); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* one connection, downloading but not linked (not possible on a client, + * but possible on a relay) */ conn->base_.state = TEST_CONN_DL_STATE; - tt_assert(networkstatus_consensus_has_excess_connections() == 0); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 1); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* one connection, downloading and linked, but not yet attached */ + ap_conn = test_conn_get_linked_connection(TO_CONN(conn), + TEST_CONN_UNATTACHED_STATE); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 1); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* one connection, downloading and linked and attached */ + ap_conn->state = TEST_CONN_ATTACHED_STATE; + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 1); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* one connection, linked and attached but not downloading */ + conn->base_.state = TEST_CONN_STATE; + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 1); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* two connections, both not downloading */ + conn2 = test_conn_download_status_add_a_connection(res); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 2); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* two connections, one downloading */ + conn->base_.state = TEST_CONN_DL_STATE; + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 2); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); conn->base_.state = TEST_CONN_STATE; - /* two connections, excess, but not downloading */ - conn2 = test_conn_download_status_add_a_connection(); - tt_assert(networkstatus_consensus_has_excess_connections() == 1); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 0); + /* more connections, all not downloading */ + conn3 = test_conn_download_status_add_a_connection(res); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 0); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 3); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); - /* two connections, excess, downloading */ + /* more connections, one downloading */ + conn->base_.state = TEST_CONN_DL_STATE; + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + res) == 3); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); + + /* more connections, two downloading (should never happen, but needs + * to be tested for completeness) */ conn2->base_.state = TEST_CONN_DL_STATE; - tt_assert(networkstatus_consensus_has_excess_connections() == 1); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); - conn2->base_.state = TEST_CONN_STATE; - - /* more connections, excess, but not downloading */ - conn3 = test_conn_download_status_add_a_connection(); - tt_assert(networkstatus_consensus_has_excess_connections() == 1); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 0); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 0); - - /* more connections, excess, downloading */ - conn3->base_.state = TEST_CONN_DL_STATE; - tt_assert(networkstatus_consensus_has_excess_connections() == 1); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); - - /* more connections, more downloading */ - conn2->base_.state = TEST_CONN_DL_STATE; - tt_assert(networkstatus_consensus_has_excess_connections() == 1); - tt_assert(networkstatus_consensus_is_downloading_usable_flavor() == 1); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); - - /* now try closing the one that isn't downloading: - * these tests won't work unless tor thinks it is bootstrapping */ - tt_assert(networkstatus_consensus_is_bootstrapping(time(NULL))); - + ap_conn2 = test_conn_get_linked_connection(TO_CONN(conn2), + TEST_CONN_ATTACHED_STATE); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); tt_assert(connection_dir_count_by_purpose_and_resource( TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC) == 3); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); - tt_assert(connection_dir_close_consensus_conn_if_extra(conn) == -1); + res) == 3); tt_assert(connection_dir_count_by_purpose_and_resource( TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC) == 2); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); + other_res) == 0); + conn->base_.state = TEST_CONN_STATE; - /* now try closing one that is already closed - nothing happens */ - tt_assert(connection_dir_close_consensus_conn_if_extra(conn) == 0); + /* more connections, a different one downloading */ + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); tt_assert(connection_dir_count_by_purpose_and_resource( TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC) == 2); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); + res) == 3); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 0); - /* now try closing one that is downloading - it stays open */ - tt_assert(connection_dir_close_consensus_conn_if_extra(conn2) == 0); + /* a connection for the other flavor (could happen if a client is set to + * cache directory documents), one preferred flavor downloading + */ + conn4 = test_conn_download_status_add_a_connection(other_res); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 0); tt_assert(connection_dir_count_by_purpose_and_resource( TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC) == 2); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); + res) == 3); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 1); - /* now try closing all excess connections */ - connection_dir_close_extra_consensus_conns(); + /* a connection for the other flavor (could happen if a client is set to + * cache directory documents), both flavors downloading + */ + conn4->base_.state = TEST_CONN_DL_STATE; + ap_conn4 = test_conn_get_linked_connection(TO_CONN(conn4), + TEST_CONN_ATTACHED_STATE); + tt_assert(networkstatus_consensus_is_already_downloading(res) == 1); + tt_assert(networkstatus_consensus_is_already_downloading(other_res) == 1); tt_assert(connection_dir_count_by_purpose_and_resource( TEST_CONN_RSRC_PURPOSE, - TEST_CONN_RSRC) == 1); - tt_assert(connection_dir_avoid_extra_connection_for_purpose( - TEST_CONN_RSRC_PURPOSE) == 1); + res) == 3); + tt_assert(connection_dir_count_by_purpose_and_resource( + TEST_CONN_RSRC_PURPOSE, + other_res) == 1); + conn4->base_.state = TEST_CONN_STATE; done: /* the teardown function removes all the connections */; @@ -750,11 +842,18 @@ test_conn_download_status(void *arg) #define CONNECTION_TESTCASE(name, fork, setup) \ { #name, test_conn_##name, fork, &setup, NULL } +/* where arg is an expression (constant, varaible, compound expression) */ +#define CONNECTION_TESTCASE_ARG(name, fork, setup, arg) \ + { #name "_" #arg, test_conn_##name, fork, &setup, (void *)arg } + struct testcase_t connection_tests[] = { CONNECTION_TESTCASE(get_basic, TT_FORK, test_conn_get_basic_st), CONNECTION_TESTCASE(get_rend, TT_FORK, test_conn_get_rend_st), CONNECTION_TESTCASE(get_rsrc, TT_FORK, test_conn_get_rsrc_st), - CONNECTION_TESTCASE(download_status, TT_FORK, test_conn_download_status_st), + CONNECTION_TESTCASE_ARG(download_status, TT_FORK, + test_conn_download_status_st, FLAV_MICRODESC), + CONNECTION_TESTCASE_ARG(download_status, TT_FORK, + test_conn_download_status_st, FLAV_NS), //CONNECTION_TESTCASE(func_suffix, TT_FORK, setup_func_pair), END_OF_TESTCASES }; From d5c70d71027644efb31d16d17b46d43484b46b18 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 18 May 2016 21:31:03 -0400 Subject: [PATCH 16/17] Restore and improve download schedule unit tests --- src/or/networkstatus.c | 8 +-- src/or/networkstatus.h | 6 +- src/test/test_dir.c | 155 +++++++++++++++++++++++++++++++++++++---- 3 files changed, 147 insertions(+), 22 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 5c9283004e..5fa7a857a8 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -1237,8 +1237,8 @@ networkstatus_get_reasonably_live_consensus(time_t now, int flavor) * If we have no consensus, or our consensus is unusably old, return 1. * As soon as we have received a consensus, return 0, even if we don't have * enough certificates to validate it. */ -int -networkstatus_consensus_is_bootstrapping(time_t now) +MOCK_IMPL(int, +networkstatus_consensus_is_bootstrapping,(time_t now)) { /* If we have a validated, reasonably live consensus, we're not * bootstrapping a consensus at all. */ @@ -1273,8 +1273,8 @@ networkstatus_consensus_can_use_multiple_directories( /** Check if we can use fallback directory mirrors for a consensus download. * If we have fallbacks and don't want to fetch from the authorities, * we can use them. */ -int -networkstatus_consensus_can_use_extra_fallbacks(const or_options_t *options) +MOCK_IMPL(int, +networkstatus_consensus_can_use_extra_fallbacks,(const or_options_t *options)) { /* The list length comparisons are a quick way to check if we have any * non-authority fallback directories. If we ever have any authorities that diff --git a/src/or/networkstatus.h b/src/or/networkstatus.h index 609fb85bcb..ac93e5de91 100644 --- a/src/or/networkstatus.h +++ b/src/or/networkstatus.h @@ -70,11 +70,11 @@ MOCK_DECL(networkstatus_t *,networkstatus_get_latest_consensus_by_flavor, networkstatus_t *networkstatus_get_live_consensus(time_t now); networkstatus_t *networkstatus_get_reasonably_live_consensus(time_t now, int flavor); -int networkstatus_consensus_is_bootstrapping(time_t now); +MOCK_DECL(int, networkstatus_consensus_is_bootstrapping,(time_t now)); int networkstatus_consensus_can_use_multiple_directories( const or_options_t *options); -int networkstatus_consensus_can_use_extra_fallbacks( - const or_options_t *options); +MOCK_DECL(int, networkstatus_consensus_can_use_extra_fallbacks,( + const or_options_t *options)); int networkstatus_consensus_is_already_downloading(const char *resource); #define NSSET_FROM_CACHE 1 diff --git a/src/test/test_dir.c b/src/test/test_dir.c index ea179fb02c..d9aff52cef 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -3995,12 +3995,56 @@ test_dir_choose_compression_level(void* data) done: ; } +static int mock_networkstatus_consensus_is_bootstrapping_value = 0; +static int +mock_networkstatus_consensus_is_bootstrapping(time_t now) +{ + (void)now; + return mock_networkstatus_consensus_is_bootstrapping_value; +} + +static int mock_networkstatus_consensus_can_use_extra_fallbacks_value = 0; +static int +mock_networkstatus_consensus_can_use_extra_fallbacks( + const or_options_t *options) +{ + (void)options; + return mock_networkstatus_consensus_can_use_extra_fallbacks_value; +} + +/* data is a 2 character nul-terminated string. + * If data[0] is 'b', set bootstrapping, anything else means not bootstrapping + * If data[1] is 'f', set extra fallbacks, anything else means no extra + * fallbacks. + */ static void test_dir_find_dl_schedule(void* data) { + const char *str = (const char *)data; + + tt_assert(strlen(data) == 2); + + if (str[0] == 'b') { + mock_networkstatus_consensus_is_bootstrapping_value = 1; + } else { + mock_networkstatus_consensus_is_bootstrapping_value = 0; + } + + if (str[1] == 'f') { + mock_networkstatus_consensus_can_use_extra_fallbacks_value = 1; + } else { + mock_networkstatus_consensus_can_use_extra_fallbacks_value = 0; + } + + MOCK(networkstatus_consensus_is_bootstrapping, + mock_networkstatus_consensus_is_bootstrapping); + MOCK(networkstatus_consensus_can_use_extra_fallbacks, + mock_networkstatus_consensus_can_use_extra_fallbacks); + download_status_t dls; - smartlist_t server, client, server_cons, client_cons, bridge; - (void)data; + smartlist_t server, client, server_cons, client_cons; + smartlist_t client_boot_auth_only_cons, client_boot_auth_cons; + smartlist_t client_boot_fallback_cons, bridge; mock_options = malloc(sizeof(or_options_t)); reset_options(mock_options, &mock_get_options_calls); @@ -4010,43 +4054,121 @@ test_dir_find_dl_schedule(void* data) mock_options->TestingClientDownloadSchedule = &client; mock_options->TestingServerConsensusDownloadSchedule = &server_cons; mock_options->TestingClientConsensusDownloadSchedule = &client_cons; + mock_options->ClientBootstrapConsensusAuthorityOnlyDownloadSchedule = + &client_boot_auth_only_cons; + mock_options->ClientBootstrapConsensusAuthorityDownloadSchedule = + &client_boot_auth_cons; + mock_options->ClientBootstrapConsensusFallbackDownloadSchedule = + &client_boot_fallback_cons; mock_options->TestingBridgeDownloadSchedule = &bridge; dls.schedule = DL_SCHED_GENERIC; + /* client */ mock_options->ClientOnly = 1; tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &client); mock_options->ClientOnly = 0; + + /* dir mode */ mock_options->DirPort_set = 1; - mock_options->ORPort_set = 1; mock_options->DirCache = 1; tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &server); - -#if 0 - dls.schedule = DL_SCHED_CONSENSUS; - mock_options->ClientOnly = 1; + mock_options->DirPort_set = 0; mock_options->DirCache = 0; - tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &client_cons); - mock_options->ClientOnly = 0; - mock_options->DirCache = 1; + + dls.schedule = DL_SCHED_CONSENSUS; + /* public server mode */ + mock_options->ORPort_set = 1; tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &server_cons); -#endif + mock_options->ORPort_set = 0; + + /* client and bridge modes */ + if (networkstatus_consensus_is_bootstrapping(time(NULL))) { + if (networkstatus_consensus_can_use_extra_fallbacks(mock_options)) { + dls.want_authority = 1; + /* client */ + mock_options->ClientOnly = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_boot_auth_cons); + mock_options->ClientOnly = 0; + + /* bridge relay */ + mock_options->ORPort_set = 1; + mock_options->BridgeRelay = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_boot_auth_cons); + mock_options->ORPort_set = 0; + mock_options->BridgeRelay = 0; + + dls.want_authority = 0; + /* client */ + mock_options->ClientOnly = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_boot_fallback_cons); + mock_options->ClientOnly = 0; + + /* bridge relay */ + mock_options->ORPort_set = 1; + mock_options->BridgeRelay = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_boot_fallback_cons); + mock_options->ORPort_set = 0; + mock_options->BridgeRelay = 0; + + } else { + /* dls.want_authority is ignored */ + /* client */ + mock_options->ClientOnly = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_boot_auth_only_cons); + mock_options->ClientOnly = 0; + + /* bridge relay */ + mock_options->ORPort_set = 1; + mock_options->BridgeRelay = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_boot_auth_only_cons); + mock_options->ORPort_set = 0; + mock_options->BridgeRelay = 0; + } + } else { + /* client */ + mock_options->ClientOnly = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_cons); + mock_options->ClientOnly = 0; + + /* bridge relay */ + mock_options->ORPort_set = 1; + mock_options->BridgeRelay = 1; + tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, + &client_cons); + mock_options->ORPort_set = 0; + mock_options->BridgeRelay = 0; + } dls.schedule = DL_SCHED_BRIDGE; + /* client */ mock_options->ClientOnly = 1; tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge); - mock_options->ClientOnly = 0; - tt_ptr_op(find_dl_schedule(&dls, mock_options), OP_EQ, &bridge); done: + UNMOCK(networkstatus_consensus_is_bootstrapping); + UNMOCK(networkstatus_consensus_can_use_extra_fallbacks); UNMOCK(get_options); + free(mock_options); + mock_options = NULL; } -#define DIR_LEGACY(name) \ +#define DIR_LEGACY(name) \ { #name, test_dir_ ## name , TT_FORK, NULL, NULL } #define DIR(name,flags) \ { #name, test_dir_##name, (flags), NULL, NULL } +/* where arg is a string constant */ +#define DIR_ARG(name,flags,arg) \ + { #name "_" arg, test_dir_##name, (flags), &passthrough_setup, arg } + struct testcase_t dir_tests[] = { DIR_LEGACY(nicknames), DIR_LEGACY(formats), @@ -4081,7 +4203,10 @@ struct testcase_t dir_tests[] = { DIR(should_not_init_request_to_dir_auths_without_v3_info, 0), DIR(should_init_request_to_dir_auths, 0), DIR(choose_compression_level, 0), - DIR(find_dl_schedule, 0), + DIR_ARG(find_dl_schedule, TT_FORK, "bf"), + DIR_ARG(find_dl_schedule, TT_FORK, "ba"), + DIR_ARG(find_dl_schedule, TT_FORK, "cf"), + DIR_ARG(find_dl_schedule, TT_FORK, "ca"), END_OF_TESTCASES }; From f698b509d8efdc055cfc05c6feaf0aecee41b9d0 Mon Sep 17 00:00:00 2001 From: "teor (Tim Wilson-Brown)" Date: Wed, 18 May 2016 22:05:35 -0400 Subject: [PATCH 17/17] Add unit tests for networkstatus_consensus_is_bootstrapping --- src/or/microdesc.c | 4 ++-- src/or/microdesc.h | 2 +- src/test/test_routerlist.c | 48 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/or/microdesc.c b/src/or/microdesc.c index 299042995b..5b5c29a6d2 100644 --- a/src/or/microdesc.c +++ b/src/or/microdesc.c @@ -955,8 +955,8 @@ we_fetch_router_descriptors(const or_options_t *options) } /** Return the consensus flavor we actually want to use to build circuits. */ -int -usable_consensus_flavor(void) +MOCK_IMPL(int, +usable_consensus_flavor,(void)) { if (we_use_microdescriptors_for_circuits(get_options())) { return FLAV_MICRODESC; diff --git a/src/or/microdesc.h b/src/or/microdesc.h index 0675e233d6..40c83139e9 100644 --- a/src/or/microdesc.h +++ b/src/or/microdesc.h @@ -47,7 +47,7 @@ void microdesc_free_all(void); void update_microdesc_downloads(time_t now); void update_microdescs_from_networkstatus(time_t now); -int usable_consensus_flavor(void); +MOCK_DECL(int, usable_consensus_flavor,(void)); int we_fetch_microdescriptors(const or_options_t *options); int we_fetch_router_descriptors(const or_options_t *options); int we_use_microdescriptors_for_circuits(const or_options_t *options); diff --git a/src/test/test_routerlist.c b/src/test/test_routerlist.c index 497606920d..2cffa6e801 100644 --- a/src/test/test_routerlist.c +++ b/src/test/test_routerlist.c @@ -15,6 +15,7 @@ #include "container.h" #include "directory.h" #include "dirvote.h" +#include "microdesc.h" #include "networkstatus.h" #include "nodelist.h" #include "policies.h" @@ -190,6 +191,14 @@ construct_consensus(char **consensus_text_md) crypto_pk_free(sign_skey_leg); } +static int mock_usable_consensus_flavor_value = FLAV_NS; + +static int +mock_usable_consensus_flavor(void) +{ + return mock_usable_consensus_flavor_value; +} + static void test_router_pick_directory_server_impl(void *arg) { @@ -209,6 +218,22 @@ test_router_pick_directory_server_impl(void *arg) (void)arg; + MOCK(usable_consensus_flavor, mock_usable_consensus_flavor); + + /* With no consensus, we must be bootstrapping, regardless of time or flavor + */ + mock_usable_consensus_flavor_value = FLAV_NS; + tt_assert(networkstatus_consensus_is_bootstrapping(now)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2000)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60)); + tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60)); + + mock_usable_consensus_flavor_value = FLAV_MICRODESC; + tt_assert(networkstatus_consensus_is_bootstrapping(now)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2000)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60)); + tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60)); + /* No consensus available, fail early */ rs = router_pick_directory_server_impl(V3_DIRINFO, (const int) 0, NULL); tt_assert(rs == NULL); @@ -223,6 +248,28 @@ test_router_pick_directory_server_impl(void *arg) tt_int_op(smartlist_len(con_md->routerstatus_list), ==, 3); tt_assert(!networkstatus_set_current_consensus_from_ns(con_md, "microdesc")); + + /* If the consensus time or flavor doesn't match, we are still + * bootstrapping */ + mock_usable_consensus_flavor_value = FLAV_NS; + tt_assert(networkstatus_consensus_is_bootstrapping(now)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2000)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60)); + tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60)); + + /* With a valid consensus for the current time and flavor, we stop + * bootstrapping, even if we have no certificates */ + mock_usable_consensus_flavor_value = FLAV_MICRODESC; + tt_assert(!networkstatus_consensus_is_bootstrapping(now + 2000)); + tt_assert(!networkstatus_consensus_is_bootstrapping(con_md->valid_after)); + tt_assert(!networkstatus_consensus_is_bootstrapping(con_md->valid_until)); + tt_assert(!networkstatus_consensus_is_bootstrapping(con_md->valid_until + + 24*60*60)); + /* These times are outside the test validity period */ + tt_assert(networkstatus_consensus_is_bootstrapping(now)); + tt_assert(networkstatus_consensus_is_bootstrapping(now + 2*24*60*60)); + tt_assert(networkstatus_consensus_is_bootstrapping(now - 2*24*60*60)); + nodelist_set_consensus(con_md); nodelist_assert_ok(); @@ -362,6 +409,7 @@ test_router_pick_directory_server_impl(void *arg) node_router1->rs->last_dir_503_at = 0; done: + UNMOCK(usable_consensus_flavor); if (router1_id) tor_free(router1_id); if (router2_id)