diff --git a/changes/bug5049 b/changes/bug5049
new file mode 100644
index 0000000000000000000000000000000000000000..677fd428695e0755983ffbf850b858af0de7acd2
--- /dev/null
+++ b/changes/bug5049
@@ -0,0 +1,4 @@
+ o Minor bugfixes:
+ - Make sure circuitbuild.c checks LearnCircuitBuildTimeout in all the
+ right places and never depends on the consensus parameters or computes
+ adaptive timeouts when it is disabled.
diff --git a/src/or/circuitbuild.c b/src/or/circuitbuild.c
index 3ab72e4b82755ab83968ea66c8ba4370e0dfc707..fa99a4948903708de39f41190ecf7e5539e74485 100644
--- a/src/or/circuitbuild.c
+++ b/src/or/circuitbuild.c
@@ -156,6 +156,11 @@ circuit_build_times_disabled(void)
state_disabled);
return 1;
} else {
+ log_debug(LD_BUG,
+ "CircuitBuildTime learning is not disabled. "
+ "Consensus=%d, Config=%d, AuthDir=%d, StateFile=%d",
+ consensus_disabled, config_disabled, dirauth_disabled,
+ state_disabled);
return 0;
}
}
@@ -171,10 +176,21 @@ circuit_build_times_disabled(void)
static int32_t
circuit_build_times_max_timeouts(void)
{
- return networkstatus_get_param(NULL, "cbtmaxtimeouts",
+ int32_t cbt_maxtimeouts;
+
+ cbt_maxtimeouts = networkstatus_get_param(NULL, "cbtmaxtimeouts",
CBT_DEFAULT_MAX_RECENT_TIMEOUT_COUNT,
CBT_MIN_MAX_RECENT_TIMEOUT_COUNT,
CBT_MAX_MAX_RECENT_TIMEOUT_COUNT);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_max_timeouts() called, cbtmaxtimeouts is"
+ " %d",
+ cbt_maxtimeouts);
+ }
+
+ return cbt_maxtimeouts;
}
/**
@@ -193,6 +209,14 @@ circuit_build_times_default_num_xm_modes(void)
CBT_DEFAULT_NUM_XM_MODES,
CBT_MIN_NUM_XM_MODES,
CBT_MAX_NUM_XM_MODES);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_default_num_xm_modes() called, cbtnummodes"
+ " is %d",
+ num);
+ }
+
return num;
}
@@ -209,6 +233,14 @@ circuit_build_times_min_circs_to_observe(void)
CBT_DEFAULT_MIN_CIRCUITS_TO_OBSERVE,
CBT_MIN_MIN_CIRCUITS_TO_OBSERVE,
CBT_MAX_MIN_CIRCUITS_TO_OBSERVE);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_min_circs_to_observe() called, cbtmincircs"
+ " is %d",
+ num);
+ }
+
return num;
}
@@ -233,6 +265,14 @@ circuit_build_times_quantile_cutoff(void)
CBT_DEFAULT_QUANTILE_CUTOFF,
CBT_MIN_QUANTILE_CUTOFF,
CBT_MAX_QUANTILE_CUTOFF);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_quantile_cutoff() called, cbtquantile"
+ " is %d",
+ num);
+ }
+
return num/100.0;
}
@@ -263,6 +303,13 @@ circuit_build_times_close_quantile(void)
CBT_DEFAULT_CLOSE_QUANTILE,
CBT_MIN_CLOSE_QUANTILE,
CBT_MAX_CLOSE_QUANTILE);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_close_quantile() called, cbtclosequantile"
+ " is %d", param);
+ }
+
if (param < min) {
log_warn(LD_DIR, "Consensus parameter cbtclosequantile is "
"too small, raising to %d", min);
@@ -285,6 +332,13 @@ circuit_build_times_test_frequency(void)
CBT_DEFAULT_TEST_FREQUENCY,
CBT_MIN_TEST_FREQUENCY,
CBT_MAX_TEST_FREQUENCY);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_test_frequency() called, cbttestfreq is %d",
+ num);
+ }
+
return num;
}
@@ -302,6 +356,13 @@ circuit_build_times_min_timeout(void)
CBT_DEFAULT_TIMEOUT_MIN_VALUE,
CBT_MIN_TIMEOUT_MIN_VALUE,
CBT_MAX_TIMEOUT_MIN_VALUE);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_min_timeout() called, cbtmintimeout is %d",
+ num);
+ }
+
return num;
}
@@ -319,6 +380,14 @@ circuit_build_times_initial_timeout(void)
CBT_DEFAULT_TIMEOUT_INITIAL_VALUE,
CBT_MIN_TIMEOUT_INITIAL_VALUE,
CBT_MAX_TIMEOUT_INITIAL_VALUE);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_initial_timeout() called, "
+ "cbtinitialtimeout is %d",
+ param);
+ }
+
if (param < min) {
log_warn(LD_DIR, "Consensus parameter cbtinitialtimeout is too small, "
"raising to %d", min);
@@ -337,10 +406,20 @@ circuit_build_times_initial_timeout(void)
static int32_t
circuit_build_times_recent_circuit_count(networkstatus_t *ns)
{
- return networkstatus_get_param(ns, "cbtrecentcount",
- CBT_DEFAULT_RECENT_CIRCUITS,
- CBT_MIN_RECENT_CIRCUITS,
- CBT_MAX_RECENT_CIRCUITS);
+ int32_t num;
+ num = networkstatus_get_param(ns, "cbtrecentcount",
+ CBT_DEFAULT_RECENT_CIRCUITS,
+ CBT_MIN_RECENT_CIRCUITS,
+ CBT_MAX_RECENT_CIRCUITS);
+
+ if (!(get_options()->LearnCircuitBuildTimeout)) {
+ log_debug(LD_BUG,
+ "circuit_build_times_recent_circuit_count() called, "
+ "cbtrecentcount is %d",
+ num);
+ }
+
+ return num;
}
/**
@@ -353,41 +432,79 @@ void
circuit_build_times_new_consensus_params(circuit_build_times_t *cbt,
networkstatus_t *ns)
{
- int32_t num = circuit_build_times_recent_circuit_count(ns);
+ int32_t num;
+
+ /*
+ * First check if we're doing adaptive timeouts at all; nothing to
+ * update if we aren't.
+ */
- if (num > 0 && num != cbt->liveness.num_recent_circs) {
- int8_t *recent_circs;
- log_notice(LD_CIRC, "The Tor Directory Consensus has changed how many "
- "circuits we must track to detect network failures from %d "
- "to %d.", cbt->liveness.num_recent_circs, num);
+ if (!circuit_build_times_disabled()) {
+ num = circuit_build_times_recent_circuit_count(ns);
- tor_assert(cbt->liveness.timeouts_after_firsthop);
+ if (num > 0) {
+ if (num != cbt->liveness.num_recent_circs) {
+ int8_t *recent_circs;
+ log_notice(LD_CIRC, "The Tor Directory Consensus has changed how many "
+ "circuits we must track to detect network failures from %d "
+ "to %d.", cbt->liveness.num_recent_circs, num);
+
+ tor_assert(cbt->liveness.timeouts_after_firsthop ||
+ cbt->liveness.num_recent_circs == 0);
+
+ /*
+ * Technically this is a circular array that we are reallocating
+ * and memcopying. However, since it only consists of either 1s
+ * or 0s, and is only used in a statistical test to determine when
+ * we should discard our history after a sufficient number of 1's
+ * have been reached, it is fine if order is not preserved or
+ * elements are lost.
+ *
+ * cbtrecentcount should only be changing in cases of severe network
+ * distress anyway, so memory correctness here is paramount over
+ * doing acrobatics to preserve the array.
+ */
+ recent_circs = tor_malloc_zero(sizeof(int8_t)*num);
+ if (cbt->liveness.timeouts_after_firsthop &&
+ cbt->liveness.num_recent_circs > 0) {
+ memcpy(recent_circs, cbt->liveness.timeouts_after_firsthop,
+ sizeof(int8_t)*MIN(num, cbt->liveness.num_recent_circs));
+ }
+
+ // Adjust the index if it needs it.
+ if (num < cbt->liveness.num_recent_circs) {
+ cbt->liveness.after_firsthop_idx = MIN(num-1,
+ cbt->liveness.after_firsthop_idx);
+ }
+
+ tor_free(cbt->liveness.timeouts_after_firsthop);
+ cbt->liveness.timeouts_after_firsthop = recent_circs;
+ cbt->liveness.num_recent_circs = num;
+ }
+ /* else no change, nothing to do */
+ } else { /* num == 0 */
+ /*
+ * Weird. This probably shouldn't happen, so log a warning, but try
+ * to do something sensible anyway.
+ */
+ log_warn(LD_CIRC,
+ "The cbtrecentcircs consensus parameter came back zero! "
+ "This disables adaptive timeouts since we can't keep track of "
+ "any recent circuits.");
+
+ circuit_build_times_free_timeouts(cbt);
+ }
+ } else {
/*
- * Technically this is a circular array that we are reallocating
- * and memcopying. However, since it only consists of either 1s
- * or 0s, and is only used in a statistical test to determine when
- * we should discard our history after a sufficient number of 1's
- * have been reached, it is fine if order is not preserved or
- * elements are lost.
- *
- * cbtrecentcount should only be changing in cases of severe network
- * distress anyway, so memory correctness here is paramount over
- * doing acrobatics to preserve the array.
+ * Adaptive timeouts are disabled; this might be because of the
+ * LearnCircuitBuildTimes config parameter, and hence permanent, or
+ * the cbtdisabled consensus parameter, so it may be a new condition.
+ * Treat it like getting num == 0 above and free the circuit history
+ * if we have any.
*/
- recent_circs = tor_malloc_zero(sizeof(int8_t)*num);
- memcpy(recent_circs, cbt->liveness.timeouts_after_firsthop,
- sizeof(int8_t)*MIN(num, cbt->liveness.num_recent_circs));
-
- // Adjust the index if it needs it.
- if (num < cbt->liveness.num_recent_circs) {
- cbt->liveness.after_firsthop_idx = MIN(num-1,
- cbt->liveness.after_firsthop_idx);
- }
- tor_free(cbt->liveness.timeouts_after_firsthop);
- cbt->liveness.timeouts_after_firsthop = recent_circs;
- cbt->liveness.num_recent_circs = num;
+ circuit_build_times_free_timeouts(cbt);
}
}
@@ -406,16 +523,26 @@ static double
circuit_build_times_get_initial_timeout(void)
{
double timeout;
- if (!unit_tests && get_options()->CircuitBuildTimeout) {
- timeout = get_options()->CircuitBuildTimeout*1000;
- if (timeout < circuit_build_times_min_timeout()) {
- log_warn(LD_CIRC, "Config CircuitBuildTimeout too low. Setting to %ds",
- circuit_build_times_min_timeout()/1000);
- timeout = circuit_build_times_min_timeout();
+
+ /*
+ * Check if we have LearnCircuitBuildTimeout, and if we don't,
+ * always use CircuitBuildTimeout, no questions asked.
+ */
+ if (get_options()->LearnCircuitBuildTimeout) {
+ if (!unit_tests && get_options()->CircuitBuildTimeout) {
+ timeout = get_options()->CircuitBuildTimeout*1000;
+ if (timeout < circuit_build_times_min_timeout()) {
+ log_warn(LD_CIRC, "Config CircuitBuildTimeout too low. Setting to %ds",
+ circuit_build_times_min_timeout()/1000);
+ timeout = circuit_build_times_min_timeout();
+ }
+ } else {
+ timeout = circuit_build_times_initial_timeout();
}
} else {
- timeout = circuit_build_times_initial_timeout();
+ timeout = get_options()->CircuitBuildTimeout*1000;
}
+
return timeout;
}
@@ -444,14 +571,40 @@ void
circuit_build_times_init(circuit_build_times_t *cbt)
{
memset(cbt, 0, sizeof(*cbt));
- cbt->liveness.num_recent_circs =
+ /*
+ * Check if we really are using adaptive timeouts, and don't keep
+ * track of this stuff if not.
+ */
+ if (!circuit_build_times_disabled()) {
+ cbt->liveness.num_recent_circs =
circuit_build_times_recent_circuit_count(NULL);
- cbt->liveness.timeouts_after_firsthop = tor_malloc_zero(sizeof(int8_t)*
- cbt->liveness.num_recent_circs);
+ cbt->liveness.timeouts_after_firsthop =
+ tor_malloc_zero(sizeof(int8_t)*cbt->liveness.num_recent_circs);
+ } else {
+ cbt->liveness.num_recent_circs = 0;
+ cbt->liveness.timeouts_after_firsthop = NULL;
+ }
cbt->close_ms = cbt->timeout_ms = circuit_build_times_get_initial_timeout();
control_event_buildtimeout_set(cbt, BUILDTIMEOUT_SET_EVENT_RESET);
}
+/**
+ * Free the saved timeouts, if the cbtdisabled consensus parameter got turned
+ * on or something.
+ */
+
+void
+circuit_build_times_free_timeouts(circuit_build_times_t *cbt)
+{
+ if (!cbt) return;
+
+ if (cbt->liveness.timeouts_after_firsthop) {
+ tor_free(cbt->liveness.timeouts_after_firsthop);
+ }
+
+ cbt->liveness.num_recent_circs = 0;
+}
+
#if 0
/**
* Rewind our build time history by n positions.
@@ -1134,9 +1287,14 @@ circuit_build_times_network_is_live(circuit_build_times_t *cbt)
void
circuit_build_times_network_circ_success(circuit_build_times_t *cbt)
{
- cbt->liveness.timeouts_after_firsthop[cbt->liveness.after_firsthop_idx] = 0;
- cbt->liveness.after_firsthop_idx++;
- cbt->liveness.after_firsthop_idx %= cbt->liveness.num_recent_circs;
+ /* Check for NULLness because we might not be using adaptive timeouts */
+ if (cbt->liveness.timeouts_after_firsthop &&
+ cbt->liveness.num_recent_circs > 0) {
+ cbt->liveness.timeouts_after_firsthop[cbt->liveness.after_firsthop_idx]
+ = 0;
+ cbt->liveness.after_firsthop_idx++;
+ cbt->liveness.after_firsthop_idx %= cbt->liveness.num_recent_circs;
+ }
}
/**
@@ -1151,10 +1309,15 @@ static void
circuit_build_times_network_timeout(circuit_build_times_t *cbt,
int did_onehop)
{
- if (did_onehop) {
- cbt->liveness.timeouts_after_firsthop[cbt->liveness.after_firsthop_idx]=1;
- cbt->liveness.after_firsthop_idx++;
- cbt->liveness.after_firsthop_idx %= cbt->liveness.num_recent_circs;
+ /* Check for NULLness because we might not be using adaptive timeouts */
+ if (cbt->liveness.timeouts_after_firsthop &&
+ cbt->liveness.num_recent_circs > 0) {
+ if (did_onehop) {
+ cbt->liveness.timeouts_after_firsthop[cbt->liveness.after_firsthop_idx]
+ = 1;
+ cbt->liveness.after_firsthop_idx++;
+ cbt->liveness.after_firsthop_idx %= cbt->liveness.num_recent_circs;
+ }
}
}
@@ -1240,10 +1403,13 @@ circuit_build_times_network_check_changed(circuit_build_times_t *cbt)
int timeout_count=0;
int i;
- /* how many of our recent circuits made it to the first hop but then
- * timed out? */
- for (i = 0; i < cbt->liveness.num_recent_circs; i++) {
- timeout_count += cbt->liveness.timeouts_after_firsthop[i];
+ if (cbt->liveness.timeouts_after_firsthop &&
+ cbt->liveness.num_recent_circs > 0) {
+ /* how many of our recent circuits made it to the first hop but then
+ * timed out? */
+ for (i = 0; i < cbt->liveness.num_recent_circs; i++) {
+ timeout_count += cbt->liveness.timeouts_after_firsthop[i];
+ }
}
/* If 80% of our recent circuits are timing out after the first hop,
@@ -1253,9 +1419,12 @@ circuit_build_times_network_check_changed(circuit_build_times_t *cbt)
}
circuit_build_times_reset(cbt);
- memset(cbt->liveness.timeouts_after_firsthop, 0,
- sizeof(*cbt->liveness.timeouts_after_firsthop)*
- cbt->liveness.num_recent_circs);
+ if (cbt->liveness.timeouts_after_firsthop &&
+ cbt->liveness.num_recent_circs > 0) {
+ memset(cbt->liveness.timeouts_after_firsthop, 0,
+ sizeof(*cbt->liveness.timeouts_after_firsthop)*
+ cbt->liveness.num_recent_circs);
+ }
cbt->liveness.after_firsthop_idx = 0;
/* Check to see if this has happened before. If so, double the timeout
@@ -1436,6 +1605,12 @@ circuit_build_times_set_timeout(circuit_build_times_t *cbt)
long prev_timeout = tor_lround(cbt->timeout_ms/1000);
double timeout_rate;
+ /*
+ * Just return if we aren't using adaptive timeouts
+ */
+ if (circuit_build_times_disabled())
+ return;
+
if (!circuit_build_times_set_timeout_worker(cbt))
return;
diff --git a/src/or/circuitbuild.h b/src/or/circuitbuild.h
index 5b773990308934079712981c1b863abcd1bc3a2f..92811783d31cdf267cae5257a4dfb43b8ae4cd3f 100644
--- a/src/or/circuitbuild.h
+++ b/src/or/circuitbuild.h
@@ -121,6 +121,7 @@ int circuit_build_times_needs_circuits(circuit_build_times_t *cbt);
int circuit_build_times_needs_circuits_now(circuit_build_times_t *cbt);
void circuit_build_times_init(circuit_build_times_t *cbt);
+void circuit_build_times_free_timeouts(circuit_build_times_t *cbt);
void circuit_build_times_new_consensus_params(circuit_build_times_t *cbt,
networkstatus_t *ns);
double circuit_build_times_timeout_rate(const circuit_build_times_t *cbt);
diff --git a/src/or/circuituse.c b/src/or/circuituse.c
index f43ce19c836aaa64c79039091af693307b026dca..7532c4e923c11419d3f67e60fe2493bf61ae2063 100644
--- a/src/or/circuituse.c
+++ b/src/or/circuituse.c
@@ -746,6 +746,7 @@ circuit_predict_and_launch_new(void)
* want, don't do another -- we want to leave a few slots open so
* we can still build circuits preemptively as needed. */
if (num < MAX_UNUSED_OPEN_CIRCUITS-2 &&
+ get_options()->LearnCircuitBuildTimeout &&
circuit_build_times_needs_circuits_now(&circ_times)) {
flags = CIRCLAUNCH_NEED_CAPACITY;
log_info(LD_CIRC,
@@ -881,7 +882,8 @@ circuit_expire_old_circuits_clientside(void)
tor_gettimeofday(&now);
cutoff = now;
- if (circuit_build_times_needs_circuits(&circ_times)) {
+ if (get_options()->LearnCircuitBuildTimeout &&
+ circuit_build_times_needs_circuits(&circ_times)) {
/* Circuits should be shorter lived if we need more of them
* for learning a good build timeout */
cutoff.tv_sec -= IDLE_TIMEOUT_WHILE_LEARNING;
diff --git a/src/or/control.c b/src/or/control.c
index dfa7d364c7f3e35433c133496859f1d73573ece7..9465f895a447d9dda09cded93c051bfe930890c1 100644
--- a/src/or/control.c
+++ b/src/or/control.c
@@ -4109,11 +4109,13 @@ control_event_buildtimeout_set(const circuit_build_times_t *cbt,
buildtimeout_set_event_t type)
{
const char *type_string = NULL;
- double qnt = circuit_build_times_quantile_cutoff();
+ double qnt;
if (!control_event_is_interesting(EVENT_BUILDTIMEOUT_SET))
return 0;
+ qnt = circuit_build_times_quantile_cutoff();
+
switch (type) {
case BUILDTIMEOUT_SET_EVENT_COMPUTED:
type_string = "COMPUTED";