From 7fc446078ba77e3d4d2a861c43b32b381ed40ccf Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 20 Oct 2020 10:50:27 -0500 Subject: [PATCH 01/15] Completely ignore abandoned circs from circ timeout calc This prevents the timeout curve from getting spread out as much, resulting in more accurate timeout values for quantiles from 60-80. --- src/core/or/circuitstats.c | 64 +++++++++----------------------------- src/test/test.c | 6 +--- 2 files changed, 15 insertions(+), 55 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index 2cde21fa1fa..1b4dbd424bc 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -1009,43 +1009,6 @@ circuit_build_times_shuffle_and_store_array(circuit_build_times_t *cbt, } } -/** - * Filter old synthetic timeouts that were created before the - * new right-censored Pareto calculation was deployed. - * - * Once all clients before 0.2.1.13-alpha are gone, this code - * will be unused. - */ -static int -circuit_build_times_filter_timeouts(circuit_build_times_t *cbt) -{ - int num_filtered=0, i=0; - double timeout_rate = 0; - build_time_t max_timeout = 0; - - timeout_rate = circuit_build_times_timeout_rate(cbt); - max_timeout = (build_time_t)cbt->close_ms; - - for (i = 0; i < CBT_NCIRCUITS_TO_OBSERVE; i++) { - if (cbt->circuit_build_times[i] > max_timeout) { - build_time_t replaced = cbt->circuit_build_times[i]; - num_filtered++; - cbt->circuit_build_times[i] = CBT_BUILD_ABANDONED; - - log_debug(LD_CIRC, "Replaced timeout %d with %d", replaced, - cbt->circuit_build_times[i]); - } - } - - log_info(LD_CIRC, - "We had %d timeouts out of %d build times, " - "and filtered %d above the max of %u", - (int)(cbt->total_build_times*timeout_rate), - cbt->total_build_times, num_filtered, max_timeout); - - return num_filtered; -} - /** * Load histogram from state, shuffling the resulting array * after we do so. Use this result to estimate parameters and @@ -1171,10 +1134,6 @@ circuit_build_times_parse_state(circuit_build_times_t *cbt, circuit_build_times_set_timeout(cbt); - if (!state->CircuitBuildAbandonedCount && cbt->total_build_times) { - circuit_build_times_filter_timeouts(cbt); - } - done: tor_free(loaded_times); return err ? -1 : 0; @@ -1215,14 +1174,15 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) if (x[i] < cbt->Xm) { a += tor_mathlog(cbt->Xm); + n++; } else if (x[i] == CBT_BUILD_ABANDONED) { abandoned_count++; } else { a += tor_mathlog(x[i]); if (x[i] > max_time) max_time = x[i]; + n++; } - n++; } /* @@ -1231,11 +1191,11 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) * performs this same check, and resets state if it hits it. If we * hit it at runtime, something serious has gone wrong. */ - if (n!=cbt->total_build_times) { + if (n!=cbt->total_build_times-abandoned_count) { log_err(LD_CIRC, "Discrepancy in build times count: %d vs %d", n, cbt->total_build_times); } - tor_assert(n==cbt->total_build_times); + tor_assert(n==cbt->total_build_times-abandoned_count); if (max_time <= 0) { /* This can happen if Xm is actually the *maximum* value in the set. @@ -1248,13 +1208,17 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) return 0; } - a += abandoned_count*tor_mathlog(max_time); - + /* This is the "Maximum Likelihood Estimator" for parameter alpha of a Pareto + * Distribution. See: + * https://en.wikipedia.org/wiki/Pareto_distribution#Estimation_of_parameters + * + * The division in the estimator is done with subtraction outside the ln(), + * with the sum occurring in the for loop above. + * + * This done is to avoid the precision issues of logs of small values. + */ a -= n*tor_mathlog(cbt->Xm); - // Estimator comes from Eq #4 in: - // "Bayesian estimation based on trimmed samples from Pareto populations" - // by Arturo J. Fernández. We are right-censored only. - a = (n-abandoned_count)/a; + a = n/a; cbt->alpha = a; diff --git a/src/test/test.c b/src/test/test.c index 58b468775cb..551f0bd3732 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -413,11 +413,7 @@ test_circuit_timeout(void *arg) for (i=0; i < CBT_DEFAULT_MIN_CIRCUITS_TO_OBSERVE; i++) { build_time_t sample = circuit_build_times_generate_sample(&initial,0,1); - if (sample > close_ms) { - circuit_build_times_add_time(&estimate, CBT_BUILD_ABANDONED); - } else { - circuit_build_times_add_time(&estimate, sample); - } + circuit_build_times_add_time(&estimate, sample); } circuit_build_times_update_alpha(&estimate); timeout1 = circuit_build_times_calculate_timeout(&estimate, From 1d5f83bb30ac17514f56fae4ffd4371157da7665 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 21 Oct 2020 18:17:30 -0500 Subject: [PATCH 02/15] Lower min circ timeout from 1.5s to bin width (10ms) --- src/core/or/circuitstats.c | 2 +- src/core/or/circuitstats.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index 1b4dbd424bc..e92bb9535da 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -1794,7 +1794,7 @@ circuit_build_times_set_timeout(circuit_build_times_t *cbt) return; if (cbt->timeout_ms < circuit_build_times_min_timeout()) { - log_info(LD_CIRC, "Set buildtimeout to low value %fms. Setting to %dms", + log_notice(LD_CIRC, "Set buildtimeout to low value %fms. Setting to %dms", cbt->timeout_ms, circuit_build_times_min_timeout()); cbt->timeout_ms = circuit_build_times_min_timeout(); if (cbt->close_ms < cbt->timeout_ms) { diff --git a/src/core/or/circuitstats.h b/src/core/or/circuitstats.h index 845d7b67229..1cd16b6f2b4 100644 --- a/src/core/or/circuitstats.h +++ b/src/core/or/circuitstats.h @@ -119,8 +119,8 @@ double circuit_build_times_quantile_cutoff(void); #define CBT_MAX_TEST_FREQUENCY INT32_MAX /** Lowest allowable value for CircuitBuildTimeout in milliseconds */ -#define CBT_DEFAULT_TIMEOUT_MIN_VALUE (1500) -#define CBT_MIN_TIMEOUT_MIN_VALUE 500 +#define CBT_DEFAULT_TIMEOUT_MIN_VALUE (CBT_BIN_WIDTH) +#define CBT_MIN_TIMEOUT_MIN_VALUE CBT_BIN_WIDTH #define CBT_MAX_TIMEOUT_MIN_VALUE INT32_MAX /** Initial circuit build timeout in milliseconds */ From 6c68f1f29c8664680c29c3faccc78c7d685c759e Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 3 Nov 2020 14:28:43 -0600 Subject: [PATCH 03/15] Raise the circuit close time quantile to 99. This should allow us to more accurately estimate pareto parameters without relying on "right-censorship" of circuit build timeout values. --- src/core/or/circuitstats.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/or/circuitstats.h b/src/core/or/circuitstats.h index 1cd16b6f2b4..e7262e9e82d 100644 --- a/src/core/or/circuitstats.h +++ b/src/core/or/circuitstats.h @@ -78,7 +78,7 @@ void circuit_build_times_mark_circ_as_measurement_only(origin_circuit_t *circ); * How long to wait before actually closing circuits that take too long to * build in terms of CDF quantile. */ -#define CBT_DEFAULT_CLOSE_QUANTILE 95 +#define CBT_DEFAULT_CLOSE_QUANTILE 99 #define CBT_MIN_CLOSE_QUANTILE CBT_MIN_QUANTILE_CUTOFF #define CBT_MAX_CLOSE_QUANTILE CBT_MAX_QUANTILE_CUTOFF From f410936f1fa51f2c3cacd78b6742aa664ef869a4 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 3 Nov 2020 15:37:43 -0600 Subject: [PATCH 04/15] Log circuit timeout in milliseconds --- src/core/or/circuitstats.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index e92bb9535da..04899b3708f 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -1626,9 +1626,8 @@ circuit_build_times_network_check_changed(circuit_build_times_t *cbt) log_notice(LD_CIRC, "Your network connection speed appears to have changed. Resetting " - "timeout to %lds after %d timeouts and %d buildtimes.", - tor_lround(cbt->timeout_ms/1000), timeout_count, - total_build_times); + "timeout to %ldms after %d timeouts and %d buildtimes.", + tor_lround(cbt->timeout_ms), timeout_count, total_build_times); return 1; } @@ -1812,9 +1811,9 @@ circuit_build_times_set_timeout(circuit_build_times_t *cbt) log_info(LD_CIRC, "Based on %d circuit times, it looks like we don't need to " "wait so long for circuits to finish. We will now assume a " - "circuit is too slow to use after waiting %ld seconds.", + "circuit is too slow to use after waiting %ld milliseconds.", cbt->total_build_times, - tor_lround(cbt->timeout_ms/1000)); + tor_lround(cbt->timeout_ms)); log_info(LD_CIRC, "Circuit timeout data: %fms, %fms, Xm: %d, a: %f, r: %f", cbt->timeout_ms, cbt->close_ms, cbt->Xm, cbt->alpha, @@ -1823,18 +1822,18 @@ circuit_build_times_set_timeout(circuit_build_times_t *cbt) log_info(LD_CIRC, "Based on %d circuit times, it looks like we need to wait " "longer for circuits to finish. We will now assume a " - "circuit is too slow to use after waiting %ld seconds.", + "circuit is too slow to use after waiting %ld milliseconds.", cbt->total_build_times, - tor_lround(cbt->timeout_ms/1000)); + tor_lround(cbt->timeout_ms)); log_info(LD_CIRC, "Circuit timeout data: %fms, %fms, Xm: %d, a: %f, r: %f", cbt->timeout_ms, cbt->close_ms, cbt->Xm, cbt->alpha, timeout_rate); } else { log_info(LD_CIRC, - "Set circuit build timeout to %lds (%fms, %fms, Xm: %d, a: %f," + "Set circuit build timeout to %ldms (%fms, %fms, Xm: %d, a: %f," " r: %f) based on %d circuit times", - tor_lround(cbt->timeout_ms/1000), + tor_lround(cbt->timeout_ms), cbt->timeout_ms, cbt->close_ms, cbt->Xm, cbt->alpha, timeout_rate, cbt->total_build_times); } From b9e3b98d1021ee4cdd9608b8ad8d1b59cf3231cb Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Wed, 4 Nov 2020 12:30:50 -0600 Subject: [PATCH 05/15] Lower circuit build time bin width to 10ms. 50ms is not enough resolution. CBT can be as low as 80ms in datacenter clients close to their relays. --- src/core/or/circuitstats.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/or/circuitstats.h b/src/core/or/circuitstats.h index e7262e9e82d..15fe23641eb 100644 --- a/src/core/or/circuitstats.h +++ b/src/core/or/circuitstats.h @@ -55,7 +55,7 @@ void circuit_build_times_mark_circ_as_measurement_only(origin_circuit_t *circ); #define CBT_NCIRCUITS_TO_OBSERVE 1000 /** Width of the histogram bins in milliseconds */ -#define CBT_BIN_WIDTH ((build_time_t)50) +#define CBT_BIN_WIDTH ((build_time_t)10) /** Number of modes to use in the weighted-avg computation of Xm */ #define CBT_DEFAULT_NUM_XM_MODES 3 From 88fa8538fdf7290801366d48f0091fa56433685b Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Fri, 6 Nov 2020 16:30:16 -0600 Subject: [PATCH 06/15] Fix Xm mode calculation to properly average N=10 modes. This is still fast enough. ~100usec on my laptop with 1000 build times. --- src/core/or/circuitstats.c | 54 +++++++++++++++----------------------- src/core/or/circuitstats.h | 2 +- 2 files changed, 22 insertions(+), 34 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index 04899b3708f..840a234b460 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -860,51 +860,41 @@ circuit_build_times_create_histogram(const circuit_build_times_t *cbt, static build_time_t circuit_build_times_get_xm(circuit_build_times_t *cbt) { - build_time_t i, nbins; + build_time_t nbins = 0; build_time_t *nth_max_bin; - int32_t bin_counts=0; - build_time_t ret = 0; - uint32_t *histogram = circuit_build_times_create_histogram(cbt, &nbins); - int n=0; + build_time_t xm_total = 0; + build_time_t Xm = 0; + int32_t xm_counts=0; int num_modes = circuit_build_times_default_num_xm_modes(); + uint32_t *histogram = circuit_build_times_create_histogram(cbt, &nbins); tor_assert(nbins > 0); tor_assert(num_modes > 0); - // Only use one mode if < 1000 buildtimes. Not enough data - // for multiple. - if (cbt->total_build_times < CBT_NCIRCUITS_TO_OBSERVE) - num_modes = 1; - nth_max_bin = tor_calloc(num_modes, sizeof(build_time_t)); - /* Determine the N most common build times */ - for (i = 0; i < nbins; i++) { - if (histogram[i] >= histogram[nth_max_bin[0]]) { - nth_max_bin[0] = i; - } - - for (n = 1; n < num_modes; n++) { - if (histogram[i] >= histogram[nth_max_bin[n]] && - (!histogram[nth_max_bin[n-1]] - || histogram[i] < histogram[nth_max_bin[n-1]])) { + /* Determine the N most common build times, by selecting the + * nth largest mode, counting it, and removing it from the histogram. */ + for (int n = 0; n < num_modes; n++) { + /* Get nth mode */ + for (build_time_t i = 0; i < nbins; i++) { + if (histogram[i] > histogram[nth_max_bin[n]]) { nth_max_bin[n] = i; } } - } - for (n = 0; n < num_modes; n++) { - bin_counts += histogram[nth_max_bin[n]]; - ret += CBT_BIN_TO_MS(nth_max_bin[n])*histogram[nth_max_bin[n]]; - log_info(LD_CIRC, "Xm mode #%d: %u %u", n, CBT_BIN_TO_MS(nth_max_bin[n]), - histogram[nth_max_bin[n]]); + /* Update average */ + xm_counts += histogram[nth_max_bin[n]]; + xm_total += CBT_BIN_TO_MS(nth_max_bin[n])*histogram[nth_max_bin[n]]; + + /* Prevent from re-counting this value */ + histogram[nth_max_bin[n]] = 0; } - /* bin_counts can become zero if all of our last CBT_NCIRCUITS_TO_OBSERVE + /* xm_counts can become zero if all of our last CBT_NCIRCUITS_TO_OBSERVE * circuits were abandoned before they completed. This shouldn't happen, * though. We should have reset/re-learned a lower timeout first. */ - if (bin_counts == 0) { - ret = 0; + if (xm_counts == 0) { log_warn(LD_CIRC, "No valid circuit build time data out of %d times, %u modes, " "have_timeout=%d, %lfms", cbt->total_build_times, num_modes, @@ -912,15 +902,13 @@ circuit_build_times_get_xm(circuit_build_times_t *cbt) goto done; } - tor_assert(bin_counts > 0); - - ret /= bin_counts; + Xm = xm_total / xm_counts; done: tor_free(histogram); tor_free(nth_max_bin); - return ret; + return Xm; } /** diff --git a/src/core/or/circuitstats.h b/src/core/or/circuitstats.h index 15fe23641eb..864b3b5f43d 100644 --- a/src/core/or/circuitstats.h +++ b/src/core/or/circuitstats.h @@ -58,7 +58,7 @@ void circuit_build_times_mark_circ_as_measurement_only(origin_circuit_t *circ); #define CBT_BIN_WIDTH ((build_time_t)10) /** Number of modes to use in the weighted-avg computation of Xm */ -#define CBT_DEFAULT_NUM_XM_MODES 3 +#define CBT_DEFAULT_NUM_XM_MODES 10 #define CBT_MIN_NUM_XM_MODES 1 #define CBT_MAX_NUM_XM_MODES 20 From 15daf871f1d537f38cf640b72e14f4c7a15ffa03 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 3 Nov 2020 14:39:10 -0600 Subject: [PATCH 07/15] Bug 40168 changes file --- changes/bug40168 | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 changes/bug40168 diff --git a/changes/bug40168 b/changes/bug40168 new file mode 100644 index 00000000000..31cd0992ebd --- /dev/null +++ b/changes/bug40168 @@ -0,0 +1,16 @@ + o Minor bugfixes (circuit build timeout): + - Improve the accuracy of our circuit build timeout calculation for 60%, + 70%, and 80% build rates for various guard choices. We now use a maximum + likelihood estimator for Pareto parameters of the circuit build time + distribution, instead of a "right-censored estimator". This causes + clients to ignore circuits that never finish building in their timeout + calculations. Previously, clients were counting such unfinished circuits + as having the highest possible build time value, when in reality these + circuits most likely just contain relays that are offline. We also now + wait a bit longer to let circuits complete for measurement purposes, + lower the minimum possible effective timeout from 1.5 seconds to 10ms, + and increase the resolution of the circuit build time histrogram from + 50ms bin widths to 10ms bin widths. Additionally, we alter our estimate + Xm by taking the maximum of the top 10 most common build time values + of the 10ms histogram, and compute Xm as the average of these. + Fixes bug 40168; bugfix on 0.2.2.14-alpha. From 960a8599d7dacd5644de34e4012dcba586652777 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 23 Nov 2020 21:37:51 -0600 Subject: [PATCH 08/15] Bug 34088: Remove max timeout calculation and warning. With the maximum likelihood estimator for alpha from #40168, we no longer need max_time to calculate alpha. --- src/core/or/circuitstats.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index 840a234b460..c711cfbfac1 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -1142,7 +1142,6 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) build_time_t *x=cbt->circuit_build_times; double a = 0; int n=0,i=0,abandoned_count=0; - build_time_t max_time=0; /* http://en.wikipedia.org/wiki/Pareto_distribution#Parameter_estimation */ /* We sort of cheat here and make our samples slightly more pareto-like @@ -1167,8 +1166,6 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) abandoned_count++; } else { a += tor_mathlog(x[i]); - if (x[i] > max_time) - max_time = x[i]; n++; } } @@ -1185,17 +1182,6 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) } tor_assert(n==cbt->total_build_times-abandoned_count); - if (max_time <= 0) { - /* This can happen if Xm is actually the *maximum* value in the set. - * It can also happen if we've abandoned every single circuit somehow. - * In either case, tell the caller not to compute a new build timeout. */ - log_warn(LD_BUG, - "Could not determine largest build time (%d). " - "Xm is %dms and we've abandoned %d out of %d circuits.", max_time, - cbt->Xm, abandoned_count, n); - return 0; - } - /* This is the "Maximum Likelihood Estimator" for parameter alpha of a Pareto * Distribution. See: * https://en.wikipedia.org/wiki/Pareto_distribution#Estimation_of_parameters From 2558933efea4f58c1207eeca3a2273fc301a91f1 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 23 Nov 2020 21:44:55 -0600 Subject: [PATCH 09/15] Bug 34088: Changes file --- changes/bug34088 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug34088 diff --git a/changes/bug34088 b/changes/bug34088 new file mode 100644 index 00000000000..172d890898f --- /dev/null +++ b/changes/bug34088 @@ -0,0 +1,4 @@ + o Minor bugfixes (circuit build timeout): + - Remove max_time calculation and associated warn from circuit build + timeout 'alpha' parameter estimation, as this is no longer needed + by our new estimator from 40168. Fixes bug 34088; bugfix on 0.2.2.9-alpha. From b658f31d390f488cc946d933060ea40424408e69 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 10 Dec 2020 13:38:40 -0600 Subject: [PATCH 10/15] fixup! Bug 40168 changes file histogram spelling fix --- changes/bug40168 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/bug40168 b/changes/bug40168 index 31cd0992ebd..c52a0352c28 100644 --- a/changes/bug40168 +++ b/changes/bug40168 @@ -9,7 +9,7 @@ circuits most likely just contain relays that are offline. We also now wait a bit longer to let circuits complete for measurement purposes, lower the minimum possible effective timeout from 1.5 seconds to 10ms, - and increase the resolution of the circuit build time histrogram from + and increase the resolution of the circuit build time histogram from 50ms bin widths to 10ms bin widths. Additionally, we alter our estimate Xm by taking the maximum of the top 10 most common build time values of the 10ms histogram, and compute Xm as the average of these. From a86bdfb8a40f3890f89b0d67ccf1dfbbf44d45e0 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 10 Dec 2020 17:17:39 -0600 Subject: [PATCH 11/15] fixup! Completely ignore abandoned circs from circ timeout calc Make assert nonfatal. --- src/core/or/circuitstats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index c711cfbfac1..166131fbe09 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -1180,7 +1180,7 @@ circuit_build_times_update_alpha(circuit_build_times_t *cbt) log_err(LD_CIRC, "Discrepancy in build times count: %d vs %d", n, cbt->total_build_times); } - tor_assert(n==cbt->total_build_times-abandoned_count); + tor_assert_nonfatal(n==cbt->total_build_times-abandoned_count); /* This is the "Maximum Likelihood Estimator" for parameter alpha of a Pareto * Distribution. See: From c5f859384e21c1da25247a78e80fdcb85c055a04 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 10 Dec 2020 17:27:55 -0600 Subject: [PATCH 12/15] Update documentation for the number of modes for Xm estimator. --- src/core/or/circuitstats.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index 166131fbe09..c982d309f31 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -205,10 +205,10 @@ circuit_build_times_max_timeouts(void) * Retrieve and bounds-check the cbtnummodes consensus parameter. * * Effect: This value governs how many modes to use in the weighted - * average calculation of Pareto parameter Xm. A value of 3 introduces - * some bias (2-5% of CDF) under ideal conditions, but allows for better - * performance in the event that a client chooses guard nodes of radically - * different performance characteristics. + * average calculation of Pareto parameter Xm. Analysis of pairs of + * geographically near, far, and mixed guaeds has shown that a value of + * 10 introduces some allows for the actual timeout rate to be within + * 2-7% of the cutoff quantile, for quantiles between 60-80%. */ static int32_t circuit_build_times_default_num_xm_modes(void) From fed3cfcd2bfc6c0928c193f0e8408c9c345f8d70 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Thu, 10 Dec 2020 17:32:40 -0600 Subject: [PATCH 13/15] fixup! Fix Xm mode calculation to properly average N=10 modes. Update function documentation for 10 modes. --- src/core/or/circuitstats.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index c982d309f31..066c275b2b5 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -853,9 +853,10 @@ circuit_build_times_create_histogram(const circuit_build_times_t *cbt, * Return the Pareto start-of-curve parameter Xm. * * Because we are not a true Pareto curve, we compute this as the - * weighted average of the N most frequent build time bins. N is either - * 1 if we don't have enough circuit build time data collected, or - * determined by the consensus parameter cbtnummodes (default 3). + * weighted average of the 10 most frequent build time bins. This + * heuristic allowed for the actual timeout rate to be closest + * to the chosen quantile cutoff, for quantiles 60-80%, out of + * many variant approaches (see #40157 for analysis). */ static build_time_t circuit_build_times_get_xm(circuit_build_times_t *cbt) From 38033b0c8d45217eabb26cf5d0d33fda64aa9762 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Mon, 14 Dec 2020 16:19:20 -0600 Subject: [PATCH 14/15] Add CBT unit test for Xm and alpha estimation. --- src/core/or/circuitstats.c | 2 +- src/core/or/circuitstats.h | 1 + src/test/test.c | 102 +++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+), 1 deletion(-) diff --git a/src/core/or/circuitstats.c b/src/core/or/circuitstats.c index 066c275b2b5..e23b11ccf99 100644 --- a/src/core/or/circuitstats.c +++ b/src/core/or/circuitstats.c @@ -858,7 +858,7 @@ circuit_build_times_create_histogram(const circuit_build_times_t *cbt, * to the chosen quantile cutoff, for quantiles 60-80%, out of * many variant approaches (see #40157 for analysis). */ -static build_time_t +STATIC build_time_t circuit_build_times_get_xm(circuit_build_times_t *cbt) { build_time_t nbins = 0; diff --git a/src/core/or/circuitstats.h b/src/core/or/circuitstats.h index 864b3b5f43d..cd5d1a0c603 100644 --- a/src/core/or/circuitstats.h +++ b/src/core/or/circuitstats.h @@ -142,6 +142,7 @@ STATIC void circuit_build_times_reset(circuit_build_times_t *cbt); /* Network liveness functions */ STATIC int circuit_build_times_network_check_changed( circuit_build_times_t *cbt); +STATIC build_time_t circuit_build_times_get_xm(circuit_build_times_t *cbt); #endif /* defined(CIRCUITSTATS_PRIVATE) */ #ifdef TOR_UNIT_TESTS diff --git a/src/test/test.c b/src/test/test.c index 551f0bd3732..2253db11d91 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -57,6 +57,7 @@ #include "feature/stats/rephist.h" #include "app/config/statefile.h" #include "lib/crypt_ops/crypto_curve25519.h" +#include "feature/nodelist/networkstatus.h" #include "core/or/extend_info_st.h" #include "core/or/or_circuit_st.h" @@ -366,6 +367,106 @@ crypto_rand_deterministic_aes(char *out, size_t n) crypto_cipher_crypt_inplace(crypto_rand_aes_cipher, out, n); } +static int32_t cbtnummodes = 10; + +static int32_t +mock_xm_networkstatus_get_param( + const networkstatus_t *ns, const char *param_name, int32_t default_val, + int32_t min_val, int32_t max_val) +{ + (void)ns; + (void)default_val; + (void)min_val; + (void)max_val; + // only support cbtnummodes right now + tor_assert(strcmp(param_name, "cbtnummodes")==0); + return cbtnummodes; +} + +static void +test_circuit_timeout_xm_alpha(void *arg) +{ + circuit_build_times_t cbt; + build_time_t Xm; + int alpha_ret; + circuit_build_times_init(&cbt); + (void)arg; + + /* Plan: + * 1. Create array of build times with 10 modes. + * 2. Make sure Xm calc is sane for 1,3,5,10,15,20 modes. + * 3. Make sure alpha calc is sane for 1,3,5,10,15,20 modes. + */ + + /* 110 build times, 9 modes, 8 mode ties, 10 abandoned */ + build_time_t circuit_build_times[] = { + 100, 20, 1000, 500, 200, 5000, 30, 600, 200, 300, CBT_BUILD_ABANDONED, + 101, 21, 1001, 501, 201, 5001, 31, 601, 201, 301, CBT_BUILD_ABANDONED, + 102, 22, 1002, 502, 202, 5002, 32, 602, 202, 302, CBT_BUILD_ABANDONED, + 103, 23, 1003, 503, 203, 5003, 33, 603, 203, 303, CBT_BUILD_ABANDONED, + 104, 24, 1004, 504, 204, 5004, 34, 604, 204, 304, CBT_BUILD_ABANDONED, + 105, 25, 1005, 505, 205, 5005, 35, 605, 205, 305, CBT_BUILD_ABANDONED, + 106, 26, 1006, 506, 206, 5006, 36, 606, 206, 306, CBT_BUILD_ABANDONED, + 107, 27, 1007, 507, 207, 5007, 37, 607, 207, 307, CBT_BUILD_ABANDONED, + 108, 28, 1008, 508, 208, 5008, 38, 608, 208, 308, CBT_BUILD_ABANDONED, + 109, 29, 1009, 509, 209, 5009, 39, 609, 209, 309, CBT_BUILD_ABANDONED + }; + + memcpy(cbt.circuit_build_times, circuit_build_times, + sizeof(circuit_build_times)); + cbt.total_build_times = 110; + + MOCK(networkstatus_get_param, mock_xm_networkstatus_get_param); + +#define CBT_ALPHA_PRECISION 0.00001 + cbtnummodes = 1; + Xm = circuit_build_times_get_xm(&cbt); + alpha_ret = circuit_build_times_update_alpha(&cbt); + tt_int_op(alpha_ret, OP_EQ, 1); + tt_int_op(Xm, OP_EQ, 205); + tt_assert(fabs(cbt.alpha - 1.394401) < CBT_ALPHA_PRECISION); + + cbtnummodes = 3; + Xm = circuit_build_times_get_xm(&cbt); + alpha_ret = circuit_build_times_update_alpha(&cbt); + tt_int_op(alpha_ret, OP_EQ, 1); + tt_int_op(Xm, OP_EQ, 117); + tt_assert(fabs(cbt.alpha - 0.902313) < CBT_ALPHA_PRECISION); + + cbtnummodes = 5; + Xm = circuit_build_times_get_xm(&cbt); + alpha_ret = circuit_build_times_update_alpha(&cbt); + tt_int_op(alpha_ret, OP_EQ, 1); + tt_int_op(Xm, OP_EQ, 146); + tt_assert(fabs(cbt.alpha - 1.049032) < CBT_ALPHA_PRECISION); + + cbtnummodes = 10; + Xm = circuit_build_times_get_xm(&cbt); + alpha_ret = circuit_build_times_update_alpha(&cbt); + tt_int_op(alpha_ret, OP_EQ, 1); + tt_int_op(Xm, OP_EQ, 800); + tt_assert(fabs(cbt.alpha - 4.851754) < CBT_ALPHA_PRECISION); + + cbtnummodes = 15; + Xm = circuit_build_times_get_xm(&cbt); + alpha_ret = circuit_build_times_update_alpha(&cbt); + tt_int_op(alpha_ret, OP_EQ, 1); + tt_int_op(Xm, OP_EQ, 800); + tt_assert(fabs(cbt.alpha - 4.851754) < CBT_ALPHA_PRECISION); + + cbtnummodes = 20; + Xm = circuit_build_times_get_xm(&cbt); + alpha_ret = circuit_build_times_update_alpha(&cbt); + tt_int_op(alpha_ret, OP_EQ, 1); + tt_int_op(Xm, OP_EQ, 800); + tt_assert(fabs(cbt.alpha - 4.851754) < CBT_ALPHA_PRECISION); + + done: +#undef CBT_ALPHA_PRECISION + UNMOCK(networkstatus_get_param); + circuit_build_times_free_timeouts(&cbt); +} + static void test_circuit_timeout(void *arg) { @@ -819,6 +920,7 @@ static struct testcase_t test_array[] = { { "ntor_handshake", test_ntor_handshake, 0, NULL, NULL }, { "fast_handshake", test_fast_handshake, 0, NULL, NULL }, FORK(circuit_timeout), + FORK(circuit_timeout_xm_alpha), FORK(rend_fns), FORK(stats), From 20f998c5b9e60a632b6d056d092f6279a3962791 Mon Sep 17 00:00:00 2001 From: Mike Perry Date: Tue, 15 Dec 2020 11:21:04 -0600 Subject: [PATCH 15/15] fixup! Completely ignore abandoned circs from circ timeout calc Remove dead close_ms var. --- src/test/test.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/test/test.c b/src/test/test.c index 2253db11d91..f4a37b0ac4d 100644 --- a/src/test/test.c +++ b/src/test/test.c @@ -485,7 +485,6 @@ test_circuit_timeout(void *arg) double timeout1, timeout2; or_state_t *state=NULL; int i, runs; - double close_ms; (void)arg; initialize_periodic_events(); @@ -507,9 +506,6 @@ test_circuit_timeout(void *arg) circuit_build_times_initial_alpha(&initial, CBT_DEFAULT_QUANTILE_CUTOFF/100.0, timeout0); - close_ms = MAX(circuit_build_times_calculate_timeout(&initial, - CBT_DEFAULT_CLOSE_QUANTILE/100.0), - CBT_DEFAULT_TIMEOUT_INITIAL_VALUE); do { for (i=0; i < CBT_DEFAULT_MIN_CIRCUITS_TO_OBSERVE; i++) { build_time_t sample = circuit_build_times_generate_sample(&initial,0,1);