From 695b0bd1d5aca52a05df1a697a6b23a20be529d4 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 19:07:11 +0000 Subject: [PATCH 01/11] Implement DL_SCHED_RANDOM_EXPONENTIAL support for download_status_t --- src/or/directory.c | 112 +++++++++++++++++++++++++++++++++++++---- src/or/directory.h | 4 ++ src/or/networkstatus.c | 8 +-- src/or/or.h | 16 ++++++ src/or/routerlist.c | 3 ++ 5 files changed, 130 insertions(+), 13 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 6caca11737..5b890cab1e 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3762,6 +3762,28 @@ find_dl_schedule(download_status_t *dls, const or_options_t *options) return NULL; } +/** Decide which minimum and maximum delay step we want to use based on + * descriptor type in dls and options. + * Helper function for download_status_schedule_get_delay(). */ +STATIC void +find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, + int *min, int *max) +{ + tor_assert(dls); + tor_assert(options); + tor_assert(min); + tor_assert(max); + + /* + * For now, just use the existing schedule config stuff and pick the + * first/last entries off to get min/max delay for backoff purposes + */ + const smartlist_t *schedule = find_dl_schedule(dls, options); + tor_assert(schedule != NULL && smartlist_len(schedule) >= 2); + *min = *((int *)(smartlist_get(schedule, 0))); + *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); +} + /* Find the current delay for dls based on schedule. * Set dls->next_attempt_at based on now, and return the delay. * Helper for download_status_increment_failure and @@ -3769,23 +3791,85 @@ find_dl_schedule(download_status_t *dls, const or_options_t *options) STATIC int download_status_schedule_get_delay(download_status_t *dls, const smartlist_t *schedule, + int min_delay, int max_delay, time_t now) { tor_assert(dls); - tor_assert(schedule); + /* We don't need a schedule if we're using random exponential backoff */ + tor_assert(dls->backoff == DL_SCHED_RANDOM_EXPONENTIAL || + schedule != NULL); + /* If we're using random exponential backoff, we do need min/max delay */ + tor_assert(dls->backoff != DL_SCHED_RANDOM_EXPONENTIAL || + (min_delay >= 0 && max_delay >= min_delay && + max_delay <= INT_MAX)); int delay = INT_MAX; + int delay_increment, i; uint8_t dls_schedule_position = (dls->increment_on == DL_SCHED_INCREMENT_ATTEMPT ? dls->n_download_attempts : dls->n_download_failures); + uint8_t entropy; - if (dls_schedule_position < smartlist_len(schedule)) - delay = *(int *)smartlist_get(schedule, dls_schedule_position); - else if (dls_schedule_position == IMPOSSIBLE_TO_DOWNLOAD) - delay = INT_MAX; - else - delay = *(int *)smartlist_get(schedule, smartlist_len(schedule) - 1); + if (dls->backoff == DL_SCHED_DETERMINISTIC) { + if (dls_schedule_position < smartlist_len(schedule)) + delay = *(int *)smartlist_get(schedule, dls_schedule_position); + else if (dls_schedule_position == IMPOSSIBLE_TO_DOWNLOAD) + delay = INT_MAX; + else + delay = *(int *)smartlist_get(schedule, smartlist_len(schedule) - 1); + } else if (dls->backoff == DL_SCHED_RANDOM_EXPONENTIAL) { + /* Check if we missed a reset somehow */ + if (dls->last_backoff_position > dls_schedule_position) { + dls->last_backoff_position = 0; + dls->last_delay_used = 0; + } + + if (dls_schedule_position > 0) { + delay = dls->last_delay_used; + + while (dls->last_backoff_position < dls_schedule_position) { + /* + * Backoff step: we want to multiply by something ~1.5, and then add + * 1 with non-zero probability so we can't get stuck at zero even if + * we start out with zero delay. To do this, pick a uint8_t of + * entropy in the range [0,255], and use it to construct an + * increment. + */ + delay_increment = 0; + /* Get a byte of entropy */ + crypto_rand((char *)(&entropy), sizeof(entropy)); + /* Clamp it just to be sure */ + entropy &= 0xff; + /* If we have non-zero delay; otherwise this is a no-op */ + if (delay > 0) { + /* Use the low 7 bits for the increment */ + for (i = 0; i < 7; ++i) { + if (entropy & (0x1 << i)) delay_increment += (delay >> (i + 1)); + } + } + /* + * Using the remaining bit of entropy, add 1 with probability 50% so + * we can't get stuck at 0 + */ + if (entropy & 0x80) delay_increment += 1; + /* Increment delay, make sure to saturate if we would wrap around */ + if (delay_increment < max_delay - delay) delay += delay_increment; + else delay = max_delay; + ++(dls->last_backoff_position); + } + } else { + delay = min_delay; + } + + /* Clamp it within min/max if we have them */ + if (min_delay >= 0 && delay < min_delay) delay = min_delay; + if (max_delay != INT_MAX && delay > max_delay) delay = max_delay; + + /* Store it for next time */ + dls->last_backoff_position = dls_schedule_position; + dls->last_delay_used = delay; + } /* A negative delay makes no sense. Knowing that delay is * non-negative allows us to safely do the wrapping check below. */ @@ -3846,6 +3930,8 @@ download_status_increment_failure(download_status_t *dls, int status_code, const char *item, int server, time_t now) { int increment = -1; + int min_delay = 0, max_delay = INT_MAX; + tor_assert(dls); /* only count the failure if it's permanent, or we're a server */ @@ -3866,7 +3952,9 @@ download_status_increment_failure(download_status_t *dls, int status_code, /* only return a failure retry time if this schedule increments on failures */ const smartlist_t *schedule = find_dl_schedule(dls, get_options()); - increment = download_status_schedule_get_delay(dls, schedule, now); + find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); + increment = download_status_schedule_get_delay(dls, schedule, + min_delay, max_delay, now); } download_status_log_helper(item, !dls->increment_on, "failed", @@ -3895,6 +3983,8 @@ download_status_increment_attempt(download_status_t *dls, const char *item, time_t now) { int delay = -1; + int min_delay = 0, max_delay = INT_MAX; + tor_assert(dls); if (dls->increment_on == DL_SCHED_INCREMENT_FAILURE) { @@ -3909,7 +3999,9 @@ download_status_increment_attempt(download_status_t *dls, const char *item, ++dls->n_download_attempts; const smartlist_t *schedule = find_dl_schedule(dls, get_options()); - delay = download_status_schedule_get_delay(dls, schedule, now); + find_dl_min_and_max_delay(dls, get_options(), &min_delay, &max_delay); + delay = download_status_schedule_get_delay(dls, schedule, + min_delay, max_delay, now); download_status_log_helper(item, dls->increment_on, "attempted", "on failure", dls->n_download_attempts, @@ -3941,6 +4033,8 @@ download_status_reset(download_status_t *dls) dls->n_download_failures = 0; dls->n_download_attempts = 0; dls->next_attempt_at = time(NULL) + *(int *)smartlist_get(schedule, 0); + dls->last_backoff_position = 0; + dls->last_delay_used = 0; /* Don't reset dls->want_authority or dls->increment_on */ } diff --git a/src/or/directory.h b/src/or/directory.h index 7646cac03f..d867aa38c4 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -146,6 +146,7 @@ STATIC int directory_handle_command_get(dir_connection_t *conn, size_t req_body_len); STATIC int download_status_schedule_get_delay(download_status_t *dls, const smartlist_t *schedule, + int min_delay, int max_delay, time_t now); STATIC char* authdir_type_to_string(dirinfo_type_t auth); @@ -154,6 +155,9 @@ STATIC int should_use_directory_guards(const or_options_t *options); STATIC zlib_compression_level_t choose_compression_level(ssize_t n_bytes); STATIC const smartlist_t *find_dl_schedule(download_status_t *dls, const or_options_t *options); +STATIC void find_dl_min_and_max_delay(download_status_t *dls, + const or_options_t *options, + int *min, int *max); #endif #endif diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index 5a91dda386..db4ed85573 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -86,9 +86,9 @@ static time_t time_to_download_next_consensus[N_CONSENSUS_FLAVORS]; static download_status_t consensus_dl_status[N_CONSENSUS_FLAVORS] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE }, + DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }, { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE }, + DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }, }; #define N_CONSENSUS_BOOTSTRAP_SCHEDULES 2 @@ -105,10 +105,10 @@ static download_status_t consensus_bootstrap_dl_status[N_CONSENSUS_BOOTSTRAP_SCHEDULES] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_ATTEMPT }, + DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_DETERMINISTIC, 0, 0 }, /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */ { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT }, + DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_DETERMINISTIC, 0, 0 }, }; /** True iff we have logged a warning about this OR's version being older than diff --git a/src/or/or.h b/src/or/or.h index 5a19f9a944..efe5680b40 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -1987,6 +1987,15 @@ typedef enum { #define download_schedule_increment_bitfield_t \ ENUM_BF(download_schedule_increment_t) +/** Enumeration: do we want to use the random exponential backoff + * mechanism? */ +typedef enum { + DL_SCHED_DETERMINISTIC = 0, + DL_SCHED_RANDOM_EXPONENTIAL = 1, +} download_schedule_backoff_t; +#define download_schedule_backoff_bitfield_t \ + ENUM_BF(download_schedule_backoff_t) + /** Information about our plans for retrying downloads for a downloadable * directory object. * Each type of downloadable directory object has a corresponding retry @@ -2033,6 +2042,13 @@ typedef struct download_status_t { download_schedule_increment_bitfield_t increment_on : 1; /**< does this * schedule increment on each attempt, * or after each failure? */ + download_schedule_backoff_bitfield_t backoff : 1; /**< do we use the + * deterministic schedule, or random + * exponential backoffs? */ + uint8_t last_backoff_position; /**< number of attempts/failures, depending + * on increment_on, when we last recalculated + * the delay. */ + int last_delay_used; /**< last delay used for random exponential backoff */ } download_status_t; /** If n_download_failures is this high, the download can never happen. */ diff --git a/src/or/routerlist.c b/src/or/routerlist.c index aaa8fad178..6721925b12 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -159,6 +159,9 @@ download_status_cert_init(download_status_t *dlstatus) dlstatus->schedule = DL_SCHED_CONSENSUS; dlstatus->want_authority = DL_WANT_ANY_DIRSERVER; dlstatus->increment_on = DL_SCHED_INCREMENT_FAILURE; + dlstatus->backoff = DL_SCHED_DETERMINISTIC; + dlstatus->last_backoff_position = 0; + dlstatus->last_delay_used = 0; /* Use the new schedule to set next_attempt_at */ download_status_reset(dlstatus); From 1553512af40fea10b060db4d236527a4f73e8a6c Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 19:07:38 +0000 Subject: [PATCH 02/11] Unit test for DL_SCHED_RANDOM_EXPONENTIAL --- src/test/test_dir.c | 69 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 5 deletions(-) diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 74b753a1ea..7acb4a3ba3 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -3333,13 +3333,16 @@ test_dir_download_status_schedule(void *arg) (void)arg; download_status_t dls_failure = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE }; + DL_SCHED_INCREMENT_FAILURE, + DL_SCHED_DETERMINISTIC, 0, 0 }; download_status_t dls_attempt = { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT}; + DL_SCHED_INCREMENT_ATTEMPT, + DL_SCHED_DETERMINISTIC, 0, 0 }; download_status_t dls_bridge = { 0, 0, 0, DL_SCHED_BRIDGE, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE}; + DL_SCHED_INCREMENT_FAILURE, + DL_SCHED_DETERMINISTIC, 0, 0 }; int increment = -1; int expected_increment = -1; time_t current_time = time(NULL); @@ -3355,6 +3358,7 @@ test_dir_download_status_schedule(void *arg) delay1 = 1000; increment = download_status_schedule_get_delay(&dls_failure, schedule, + 0, INT_MAX, TIME_MIN); expected_increment = delay1; tt_assert(increment == expected_increment); @@ -3363,6 +3367,7 @@ test_dir_download_status_schedule(void *arg) delay1 = INT_MAX; increment = download_status_schedule_get_delay(&dls_failure, schedule, + 0, INT_MAX, -1); expected_increment = delay1; tt_assert(increment == expected_increment); @@ -3371,6 +3376,7 @@ test_dir_download_status_schedule(void *arg) delay1 = 0; increment = download_status_schedule_get_delay(&dls_attempt, schedule, + 0, INT_MAX, 0); expected_increment = delay1; tt_assert(increment == expected_increment); @@ -3379,6 +3385,7 @@ test_dir_download_status_schedule(void *arg) delay1 = 1000; increment = download_status_schedule_get_delay(&dls_attempt, schedule, + 0, INT_MAX, 1); expected_increment = delay1; tt_assert(increment == expected_increment); @@ -3387,6 +3394,7 @@ test_dir_download_status_schedule(void *arg) delay1 = INT_MAX; increment = download_status_schedule_get_delay(&dls_bridge, schedule, + 0, INT_MAX, current_time); expected_increment = delay1; tt_assert(increment == expected_increment); @@ -3395,6 +3403,7 @@ test_dir_download_status_schedule(void *arg) delay1 = 1; increment = download_status_schedule_get_delay(&dls_bridge, schedule, + 0, INT_MAX, TIME_MAX); expected_increment = delay1; tt_assert(increment == expected_increment); @@ -3407,6 +3416,7 @@ test_dir_download_status_schedule(void *arg) delay2 = 100; increment = download_status_schedule_get_delay(&dls_attempt, schedule, + 0, INT_MAX, current_time); expected_increment = delay2; tt_assert(increment == expected_increment); @@ -3415,6 +3425,7 @@ test_dir_download_status_schedule(void *arg) delay2 = 1; increment = download_status_schedule_get_delay(&dls_bridge, schedule, + 0, INT_MAX, current_time); expected_increment = delay2; tt_assert(increment == expected_increment); @@ -3427,6 +3438,7 @@ test_dir_download_status_schedule(void *arg) delay2 = 5; increment = download_status_schedule_get_delay(&dls_attempt, schedule, + 0, INT_MAX, current_time); expected_increment = delay2; tt_assert(increment == expected_increment); @@ -3435,6 +3447,7 @@ test_dir_download_status_schedule(void *arg) delay2 = 17; increment = download_status_schedule_get_delay(&dls_bridge, schedule, + 0, INT_MAX, current_time); expected_increment = delay2; tt_assert(increment == expected_increment); @@ -3447,6 +3460,7 @@ test_dir_download_status_schedule(void *arg) delay2 = 35; increment = download_status_schedule_get_delay(&dls_attempt, schedule, + 0, INT_MAX, current_time); expected_increment = INT_MAX; tt_assert(increment == expected_increment); @@ -3455,6 +3469,7 @@ test_dir_download_status_schedule(void *arg) delay2 = 99; increment = download_status_schedule_get_delay(&dls_bridge, schedule, + 0, INT_MAX, current_time); expected_increment = INT_MAX; tt_assert(increment == expected_increment); @@ -3465,16 +3480,59 @@ test_dir_download_status_schedule(void *arg) smartlist_free(schedule); } +static void +test_dir_download_status_random_backoff(void *arg) +{ + download_status_t dls_random = + { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, + DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }; + int increment = -1; + int old_increment; + time_t current_time = time(NULL); + const int min_delay = 0; + const int max_delay = 1000000; + + (void)arg; + + /* Check the random backoff cases */ + old_increment = 0; + do { + increment = download_status_schedule_get_delay(&dls_random, + NULL, + min_delay, max_delay, + current_time); + /* Test */ + tt_int_op(increment, OP_GE, min_delay); + tt_int_op(increment, OP_LE, max_delay); + tt_int_op(increment, OP_GE, old_increment); + /* We at most double, and maybe add one */ + tt_int_op(increment, OP_LE, 2 * old_increment + 1); + + /* Advance */ + current_time += increment; + ++(dls_random.n_download_attempts); + ++(dls_random.n_download_failures); + + /* Try another maybe */ + old_increment = increment; + } while (increment < max_delay); + + done: + return; +} + static void test_dir_download_status_increment(void *arg) { (void)arg; download_status_t dls_failure = { 0, 0, 0, DL_SCHED_GENERIC, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_FAILURE }; + DL_SCHED_INCREMENT_FAILURE, + DL_SCHED_DETERMINISTIC, 0, 0 }; download_status_t dls_attempt = { 0, 0, 0, DL_SCHED_BRIDGE, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT}; + DL_SCHED_INCREMENT_ATTEMPT, + DL_SCHED_DETERMINISTIC, 0, 0 }; int delay0 = -1; int delay1 = -1; int delay2 = -1; @@ -4243,6 +4301,7 @@ struct testcase_t dir_tests[] = { DIR(fetch_type, 0), DIR(packages, 0), DIR(download_status_schedule, 0), + DIR(download_status_random_backoff, 0), DIR(download_status_increment, 0), DIR(authdir_type_to_string, 0), DIR(conn_purpose_to_string, 0), From 5104e5645f8bdb2197e57e45eed746470056f0d3 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 21:07:24 +0000 Subject: [PATCH 03/11] Use exponential backoffs for consensus downloads --- src/or/networkstatus.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/or/networkstatus.c b/src/or/networkstatus.c index db4ed85573..d5c8a262f0 100644 --- a/src/or/networkstatus.c +++ b/src/or/networkstatus.c @@ -86,9 +86,9 @@ static time_t time_to_download_next_consensus[N_CONSENSUS_FLAVORS]; static download_status_t consensus_dl_status[N_CONSENSUS_FLAVORS] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }, + DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_FAILURE, DL_SCHED_DETERMINISTIC, 0, 0 }, + DL_SCHED_INCREMENT_FAILURE, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, }; #define N_CONSENSUS_BOOTSTRAP_SCHEDULES 2 @@ -105,10 +105,10 @@ static download_status_t consensus_bootstrap_dl_status[N_CONSENSUS_BOOTSTRAP_SCHEDULES] = { { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_AUTHORITY, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_DETERMINISTIC, 0, 0 }, + DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, /* During bootstrap, DL_WANT_ANY_DIRSERVER means "use fallbacks". */ { 0, 0, 0, DL_SCHED_CONSENSUS, DL_WANT_ANY_DIRSERVER, - DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_DETERMINISTIC, 0, 0 }, + DL_SCHED_INCREMENT_ATTEMPT, DL_SCHED_RANDOM_EXPONENTIAL, 0, 0 }, }; /** True iff we have logged a warning about this OR's version being older than From 36d45a9f6448524d70e6fbe0fb16f61ef5b43c1f Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 21:13:09 +0000 Subject: [PATCH 04/11] Use exponential backoffs for certificate downloads --- src/or/routerlist.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 6721925b12..9cd5ef171c 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -159,7 +159,7 @@ download_status_cert_init(download_status_t *dlstatus) dlstatus->schedule = DL_SCHED_CONSENSUS; dlstatus->want_authority = DL_WANT_ANY_DIRSERVER; dlstatus->increment_on = DL_SCHED_INCREMENT_FAILURE; - dlstatus->backoff = DL_SCHED_DETERMINISTIC; + dlstatus->backoff = DL_SCHED_RANDOM_EXPONENTIAL; dlstatus->last_backoff_position = 0; dlstatus->last_delay_used = 0; From 5cb27d8991620af2b09c5cefaeed7b8b871c4aae Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 21:15:30 +0000 Subject: [PATCH 05/11] Use exponential backoffs for bridge descriptor downloads --- src/or/entrynodes.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/or/entrynodes.c b/src/or/entrynodes.c index 1a31aa4822..a210ca1125 100644 --- a/src/or/entrynodes.c +++ b/src/or/entrynodes.c @@ -2032,6 +2032,7 @@ bridge_add_from_config(bridge_line_t *bridge_line) if (bridge_line->transport_name) b->transport_name = bridge_line->transport_name; b->fetch_status.schedule = DL_SCHED_BRIDGE; + b->fetch_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; b->socks_args = bridge_line->socks_args; if (!bridge_list) bridge_list = smartlist_new(); From 6370c4ee87204bae264ff9c5a5cf5872958beda9 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 21:27:22 +0000 Subject: [PATCH 06/11] Use exponential backoff for router descriptor downloads from consensuses --- src/or/routerparse.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/or/routerparse.c b/src/or/routerparse.c index cefe607fc6..a612a94223 100644 --- a/src/or/routerparse.c +++ b/src/or/routerparse.c @@ -3204,6 +3204,8 @@ networkstatus_parse_vote_from_string(const char *s, const char **eos_out, NULL, NULL, ns->consensus_method, flav))) + /* Use exponential-backoff scheduling when downloading microdescs */ + rs->dl_status.backoff = DL_SCHED_RANDOM_EXPONENTIAL; smartlist_add(ns->routerstatus_list, rs); } } From 2905a3484eb02e5cf3bc3474240a42e24c07ab74 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sun, 12 Jun 2016 21:29:11 +0000 Subject: [PATCH 07/11] Changes file for random exponential backoffs --- changes/bug15942 | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changes/bug15942 diff --git a/changes/bug15942 b/changes/bug15942 new file mode 100644 index 0000000000..0edc2b7980 --- /dev/null +++ b/changes/bug15942 @@ -0,0 +1,3 @@ + o Bugfixes (downloading): + - Use random exponential backoffs when retrying downloads from the dir + servers. Fixes bug 15942; bugfix on ?????. From 1dfbfd319e417c06c6e6d97d8c617522873ad43f Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 18 Jun 2016 17:11:32 +0000 Subject: [PATCH 08/11] Better comment for download_status_schedule_get_delay() per code review --- src/or/directory.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 5b890cab1e..1a8fd2cb21 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3784,8 +3784,11 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); } -/* Find the current delay for dls based on schedule. - * Set dls->next_attempt_at based on now, and return the delay. +/** Find the current delay for dls based on schedule or min_delay/ + * max_delay if we're using exponential backoff. If dls->backoff is + * DL_SCHED_RANDOM_EXPONENTIAL, we must have 0 <= min_delay <= max_delay <= + * INT_MAX, but schedule may be set to NULL; otherwise schedule is required. + * This function sets dls->next_attempt_at based on now, and returns the delay. * Helper for download_status_increment_failure and * download_status_increment_attempt. */ STATIC int From 1f1df4ab740b2d2c2a833a81553bb723512bdd97 Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 18 Jun 2016 18:23:55 +0000 Subject: [PATCH 09/11] Move exponential-random backoff computation out of download_status_schedule_get_delay() into separate function, per code review --- src/or/directory.c | 79 +++++++++++++++++++++++++++++----------------- src/or/directory.h | 2 ++ 2 files changed, 52 insertions(+), 29 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 1a8fd2cb21..02016887ea 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3784,6 +3784,52 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, *max = *((int *)((smartlist_get(schedule, smartlist_len(schedule) - 1)))); } +/** Advance one delay step. The algorithm is to use the previous delay to + * compute an increment. Consuming one byte of entropy per step, we use 7 + * bits to construct an increment between 0 and (127/128)*delay by adding + * right-shifted copies of delay, controlled by each bit. Then, to prevent + * getting stuck at zero if we start from zero, we use one last bit to add + * 1 with probability 50%. Finally, we add the increment to the original + * delay, clamp the value <= max_delay, and return it. + */ +STATIC int +next_random_exponential_delay(int delay, int max_delay) +{ + int delay_increment, i; + uint8_t entropy; + + /* + * Backoff step: we want to multiply by something ~1.5, and then add + * 1 with non-zero probability so we can't get stuck at zero even if + * we start out with zero delay. To do this, pick a uint8_t of + * entropy in the range [0,255], and use it to construct an + * increment. + */ + delay_increment = 0; + /* Get a byte of entropy */ + crypto_rand((char *)(&entropy), sizeof(entropy)); + /* Clamp it just to be sure */ + entropy &= 0xff; + /* If we have non-zero delay; otherwise this is a no-op */ + if (delay > 0) { + /* Use the low 7 bits for the increment */ + for (i = 0; i < 7; ++i) { + if (entropy & (0x1 << i)) delay_increment += (delay >> (i + 1)); + } + } + /* + * Using the remaining bit of entropy, add 1 with probability 50% so + * we can't get stuck at 0 + */ + if (entropy & 0x80) delay_increment += 1; + /* Increment delay, make sure to saturate if we would wrap around */ + if (delay_increment < max_delay - delay) delay += delay_increment; + else delay = max_delay; + + /* Return the updated delay */ + return delay; +} + /** Find the current delay for dls based on schedule or min_delay/ * max_delay if we're using exponential backoff. If dls->backoff is * DL_SCHED_RANDOM_EXPONENTIAL, we must have 0 <= min_delay <= max_delay <= @@ -3807,12 +3853,10 @@ download_status_schedule_get_delay(download_status_t *dls, max_delay <= INT_MAX)); int delay = INT_MAX; - int delay_increment, i; uint8_t dls_schedule_position = (dls->increment_on == DL_SCHED_INCREMENT_ATTEMPT ? dls->n_download_attempts : dls->n_download_failures); - uint8_t entropy; if (dls->backoff == DL_SCHED_DETERMINISTIC) { if (dls_schedule_position < smartlist_len(schedule)) @@ -3832,36 +3876,13 @@ download_status_schedule_get_delay(download_status_t *dls, delay = dls->last_delay_used; while (dls->last_backoff_position < dls_schedule_position) { - /* - * Backoff step: we want to multiply by something ~1.5, and then add - * 1 with non-zero probability so we can't get stuck at zero even if - * we start out with zero delay. To do this, pick a uint8_t of - * entropy in the range [0,255], and use it to construct an - * increment. - */ - delay_increment = 0; - /* Get a byte of entropy */ - crypto_rand((char *)(&entropy), sizeof(entropy)); - /* Clamp it just to be sure */ - entropy &= 0xff; - /* If we have non-zero delay; otherwise this is a no-op */ - if (delay > 0) { - /* Use the low 7 bits for the increment */ - for (i = 0; i < 7; ++i) { - if (entropy & (0x1 << i)) delay_increment += (delay >> (i + 1)); - } - } - /* - * Using the remaining bit of entropy, add 1 with probability 50% so - * we can't get stuck at 0 - */ - if (entropy & 0x80) delay_increment += 1; - /* Increment delay, make sure to saturate if we would wrap around */ - if (delay_increment < max_delay - delay) delay += delay_increment; - else delay = max_delay; + /* Do one increment step */ + delay = next_random_exponential_delay(delay, max_delay); + /* Update our position */ ++(dls->last_backoff_position); } } else { + /* If we're just starting out, use the minimum delay */ delay = min_delay; } diff --git a/src/or/directory.h b/src/or/directory.h index d867aa38c4..afa3bcc611 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -158,6 +158,8 @@ STATIC const smartlist_t *find_dl_schedule(download_status_t *dls, STATIC void find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, int *min, int *max); +STATIC int next_random_exponential_delay(int delay, int max_delay); + #endif #endif From 5a4ed29f01479f0f5c0141ec09cf5ff2c1e15a9b Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Sat, 18 Jun 2016 19:05:46 +0000 Subject: [PATCH 10/11] Better comments on exponential-backoff related members of download_status_t --- src/or/or.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/or/or.h b/src/or/or.h index efe5680b40..ea38022f43 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2047,8 +2047,10 @@ typedef struct download_status_t { * exponential backoffs? */ uint8_t last_backoff_position; /**< number of attempts/failures, depending * on increment_on, when we last recalculated - * the delay. */ - int last_delay_used; /**< last delay used for random exponential backoff */ + * the delay. Only updated if backoff + * == 1. */ + int last_delay_used; /**< last delay used for random exponential backoff; + * only updated if backoff == 1 */ } download_status_t; /** If n_download_failures is this high, the download can never happen. */ From a09ec22a9b1d213716ac1792752c266c3a92a1f6 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 20 Jun 2016 10:10:02 -0400 Subject: [PATCH 11/11] Simpler implementation of random exponential backoff. Consumes more entropy, but is easier to read. --- src/or/directory.c | 63 +++++++++++++++++++--------------------------- 1 file changed, 26 insertions(+), 37 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 02016887ea..c1b5ae7519 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -3785,49 +3785,38 @@ find_dl_min_and_max_delay(download_status_t *dls, const or_options_t *options, } /** Advance one delay step. The algorithm is to use the previous delay to - * compute an increment. Consuming one byte of entropy per step, we use 7 - * bits to construct an increment between 0 and (127/128)*delay by adding - * right-shifted copies of delay, controlled by each bit. Then, to prevent - * getting stuck at zero if we start from zero, we use one last bit to add - * 1 with probability 50%. Finally, we add the increment to the original - * delay, clamp the value <= max_delay, and return it. + * compute an increment, we construct a value uniformly at random between + * delay and MAX(delay*2,delay+1). We then clamp that value to be no larger + * than max_delay, and return it. + * + * Requires that delay is less than INT_MAX, and delay is in [0,max_delay]. */ STATIC int next_random_exponential_delay(int delay, int max_delay) { - int delay_increment, i; - uint8_t entropy; + /* Check preconditions */ + if (BUG(delay > max_delay)) + delay = max_delay; + if (BUG(delay == INT_MAX)) + delay -= 1; /* prevent overflow */ + if (BUG(delay < 0)) + delay = 0; - /* - * Backoff step: we want to multiply by something ~1.5, and then add - * 1 with non-zero probability so we can't get stuck at zero even if - * we start out with zero delay. To do this, pick a uint8_t of - * entropy in the range [0,255], and use it to construct an - * increment. - */ - delay_increment = 0; - /* Get a byte of entropy */ - crypto_rand((char *)(&entropy), sizeof(entropy)); - /* Clamp it just to be sure */ - entropy &= 0xff; - /* If we have non-zero delay; otherwise this is a no-op */ - if (delay > 0) { - /* Use the low 7 bits for the increment */ - for (i = 0; i < 7; ++i) { - if (entropy & (0x1 << i)) delay_increment += (delay >> (i + 1)); - } - } - /* - * Using the remaining bit of entropy, add 1 with probability 50% so - * we can't get stuck at 0 - */ - if (entropy & 0x80) delay_increment += 1; - /* Increment delay, make sure to saturate if we would wrap around */ - if (delay_increment < max_delay - delay) delay += delay_increment; - else delay = max_delay; + /* How much are we willing to add to the delay? */ + int max_increment; - /* Return the updated delay */ - return delay; + if (delay) + max_increment = delay; /* no more than double. */ + else + max_increment = 1; /* we're always willing to slow down a little. */ + + /* the + 1 here is so that we include the end of the interval */ + int increment = crypto_rand_int(max_increment+1); + + if (increment < max_delay - delay) + return delay + increment; + else + return max_delay; } /** Find the current delay for dls based on schedule or min_delay/