From 16ec1d21f2388afe91a66b8b2415c036c3af529e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 20 Oct 2021 12:06:58 -0400 Subject: [PATCH] Allow type of timeout estimator to change at runtime. This is a big change, but it does simplify the type of Builder a little, and isolates locking across different (potential) timeout estimator types. --- crates/tor-circmgr/src/build.rs | 107 ++++---- crates/tor-circmgr/src/timeouts.rs | 19 +- crates/tor-circmgr/src/timeouts/estimator.rs | 131 ++++++++++ crates/tor-circmgr/src/timeouts/pareto.rs | 253 ++++++++----------- crates/tor-netdir/src/params.rs | 9 + 5 files changed, 308 insertions(+), 211 deletions(-) create mode 100644 crates/tor-circmgr/src/timeouts/estimator.rs diff --git a/crates/tor-circmgr/src/build.rs b/crates/tor-circmgr/src/build.rs index da5bafea7..e111723dc 100644 --- a/crates/tor-circmgr/src/build.rs +++ b/crates/tor-circmgr/src/build.rs @@ -1,7 +1,7 @@ //! Facilities to build circuits directly, instead of via a circuit manager. use crate::path::{OwnedPath, TorPath}; -use crate::timeouts::{pareto::ParetoTimeoutEstimator, Action, TimeoutEstimator}; +use crate::timeouts::{self, pareto::ParetoTimeoutEstimator, Action}; use crate::{Error, Result}; use async_trait::async_trait; use futures::channel::oneshot; @@ -130,30 +130,21 @@ impl Buildable for Arc { /// /// In general, you should not need to construct or use this object yourself, /// unless you are choosing your own paths. -struct Builder< - R: Runtime, - C: Buildable + Sync + Send + 'static, - T: TimeoutEstimator + Send + Sync + 'static, -> { +struct Builder { /// The runtime used by this circuit builder. runtime: R, /// A channel manager that this circuit builder uses to make channels. chanmgr: Arc>, /// An estimator to determine the correct timeouts for circuit building. - timeouts: T, + timeouts: timeouts::Estimator, /// We don't actually hold any clientcircs, so we need to put this /// type here so the compiler won't freak out. _phantom: std::marker::PhantomData, } -impl< - R: Runtime, - C: Buildable + Sync + Send + 'static, - T: TimeoutEstimator + Send + Sync + 'static, - > Builder -{ +impl Builder { /// Construct a new [`Builder`]. - fn new(runtime: R, chanmgr: Arc>, timeouts: T) -> Self { + fn new(runtime: R, chanmgr: Arc>, timeouts: timeouts::Estimator) -> Self { Builder { runtime, chanmgr, @@ -271,7 +262,7 @@ impl< /// unless you are choosing your own paths. pub struct CircuitBuilder { /// The underlying [`Builder`] object - builder: Arc, ParetoTimeoutEstimator>>, + builder: Arc>>, /// Configuration for how to choose paths for circuits. path_config: crate::PathConfig, /// State-manager object to use in storing current state. @@ -300,6 +291,7 @@ impl CircuitBuilder { ParetoTimeoutEstimator::default() } }; + let timeouts = timeouts::Estimator::new(timeouts); CircuitBuilder { builder: Arc::new(Builder::new(runtime, chanmgr, timeouts)), @@ -313,16 +305,14 @@ impl CircuitBuilder { pub fn save_state(&self) -> Result<()> { // TODO: someday we'll want to only do this if there is something // changed. - let state = self.builder.timeouts.build_state(); - self.storage.store(&state)?; - Ok(()) + self.builder.timeouts.save_state(&self.storage) } /// Reconfigure this builder using the latest set of network parameters. /// /// (NOTE: for now, this only affects circuit timeout estimation.) pub fn update_network_parameters(&self, p: &tor_netdir::params::NetParameters) { - self.builder.timeouts.update_params(p.into()); + self.builder.timeouts.update_params(p); } /// DOCDOC @@ -421,6 +411,7 @@ where mod test { #![allow(clippy::unwrap_used)] use super::*; + use crate::timeouts::TimeoutEstimator; use futures::channel::oneshot; use std::sync::atomic::{AtomicU64, Ordering::SeqCst}; use std::sync::Mutex; @@ -602,24 +593,28 @@ mod test { } } impl TimeoutEstimator for Arc> { - fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool) { + fn note_hop_completed(&mut self, hop: u8, delay: Duration, is_last: bool) { if !is_last { return; } - - let mut h = self.lock().unwrap(); - h.hist.push((true, hop, delay)); + let mut this = self.lock().unwrap(); + this.hist.push((true, hop, delay)); } - fn note_circ_timeout(&self, hop: u8, delay: Duration) { - let mut h = self.lock().unwrap(); - h.hist.push((false, hop, delay)); + fn note_circ_timeout(&mut self, hop: u8, delay: Duration) { + let mut this = self.lock().unwrap(); + this.hist.push((false, hop, delay)); } - fn timeouts(&self, _action: &Action) -> (Duration, Duration) { + fn timeouts(&mut self, _action: &Action) -> (Duration, Duration) { (Duration::from_secs(3), Duration::from_secs(100)) } fn learning_timeouts(&self) -> bool { false } + fn update_params(&mut self, _params: &tor_netdir::params::NetParameters) {} + + fn build_state(&mut self) -> Option { + None + } } /// Testing only: create a bogus circuit target @@ -649,8 +644,11 @@ mod test { ]); let chanmgr = Arc::new(ChanMgr::new(rt.clone())); let timeouts = Arc::new(Mutex::new(TimeoutRecorder::new())); - let builder: Builder<_, Mutex, _> = - Builder::new(rt.clone(), chanmgr, Arc::clone(&timeouts)); + let builder: Builder<_, Mutex> = Builder::new( + rt.clone(), + chanmgr, + timeouts::Estimator::new(Arc::clone(&timeouts)), + ); let builder = Arc::new(builder); let rng = StdRng::from_rng(rand::thread_rng()).expect("couldn't construct temporary rng"); @@ -677,15 +675,14 @@ mod test { ); { - let mut h = timeouts.lock().unwrap(); - assert_eq!(h.hist.len(), 2); - assert!(h.hist[0].0); // completed - assert_eq!(h.hist[0].1, 0); // last hop completed - // TODO: test time elapsed, once wait_for is more reliable. - assert!(h.hist[1].0); // completed - assert_eq!(h.hist[1].1, 2); // last hop completed - // TODO: test time elapsed, once wait_for is more reliable. - h.hist.clear(); + let timeouts = timeouts.lock().unwrap(); + assert_eq!(timeouts.hist.len(), 2); + assert!(timeouts.hist[0].0); // completed + assert_eq!(timeouts.hist[0].1, 0); // last hop completed + // TODO: test time elapsed, once wait_for is more reliable. + assert!(timeouts.hist[1].0); // completed + assert_eq!(timeouts.hist[1].1, 2); // last hop completed + // TODO: test time elapsed, once wait_for is more reliable. } // Try a very long timeout. @@ -700,11 +697,10 @@ mod test { assert!(outcome.is_err()); { - let mut h = timeouts.lock().unwrap(); - assert_eq!(h.hist.len(), 1); - assert!(!h.hist[0].0); - assert_eq!(h.hist[0].1, 2); - h.hist.clear(); + let timeouts = timeouts.lock().unwrap(); + assert_eq!(timeouts.hist.len(), 3); + assert!(!timeouts.hist[2].0); + assert_eq!(timeouts.hist[2].1, 2); } // Now try a recordable timeout. @@ -721,26 +717,27 @@ mod test { rt.advance(Duration::from_millis(100)).await; } { - let h = timeouts.lock().unwrap(); - dbg!(&h.hist); + let timeouts = timeouts.lock().unwrap(); + dbg!(&timeouts.hist); + assert!(timeouts.hist.len() >= 4); // First we notice a circuit timeout after 2 hops - assert!(!h.hist[0].0); - assert_eq!(h.hist[0].1, 2); + assert!(!timeouts.hist[3].0); + assert_eq!(timeouts.hist[3].1, 2); // TODO: check timeout more closely. - assert!(h.hist[0].2 < Duration::from_secs(100)); - assert!(h.hist[0].2 >= Duration::from_secs(3)); + assert!(timeouts.hist[3].2 < Duration::from_secs(100)); + assert!(timeouts.hist[3].2 >= Duration::from_secs(3)); // This test is not reliable under test coverage; see arti#149. #[cfg(not(tarpaulin))] { - assert_eq!(h.hist.len(), 2); + assert_eq!(timeouts.hist.len(), 5); // Then we notice a circuit completing at its third hop. - assert!(h.hist[1].0); - assert_eq!(h.hist[1].1, 2); + assert!(timeouts.hist[4].0); + assert_eq!(timeouts.hist[4].1, 2); // TODO: check timeout more closely. - assert!(h.hist[1].2 < Duration::from_secs(100)); - assert!(h.hist[1].2 >= Duration::from_secs(5)); - assert!(h.hist[0].2 < h.hist[1].2); + assert!(timeouts.hist[4].2 < Duration::from_secs(100)); + assert!(timeouts.hist[4].2 >= Duration::from_secs(5)); + assert!(timeouts.hist[3].2 < timeouts.hist[4].2); } } HOP3_DELAY.store(300, SeqCst); // undo previous run. diff --git a/crates/tor-circmgr/src/timeouts.rs b/crates/tor-circmgr/src/timeouts.rs index 0bfdbf9b9..b8a499541 100644 --- a/crates/tor-circmgr/src/timeouts.rs +++ b/crates/tor-circmgr/src/timeouts.rs @@ -10,8 +10,11 @@ use std::time::Duration; +pub(crate) mod estimator; pub(crate) mod pareto; +pub(crate) use estimator::Estimator; + /// An object that calculates circuit timeout thresholds from the history /// of circuit build times. pub(crate) trait TimeoutEstimator { @@ -23,7 +26,7 @@ pub(crate) trait TimeoutEstimator { /// circuit. /// /// If this is the last hop of the circuit, then `is_last` is true. - fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool); + fn note_hop_completed(&mut self, hop: u8, delay: Duration, is_last: bool); /// Record that a circuit failed to complete because it took too long. /// @@ -32,7 +35,7 @@ pub(crate) trait TimeoutEstimator { /// /// The `delay` number is the amount of time after we first launched the /// circuit. - fn note_circ_timeout(&self, hop: u8, delay: Duration); + fn note_circ_timeout(&mut self, hop: u8, delay: Duration); /// Return the current estimation for how long we should wait for a given /// [`Action`] to complete. @@ -43,11 +46,21 @@ pub(crate) trait TimeoutEstimator { /// building it in order see how long it takes. After `abandon` /// has elapsed since circuit launch, the circuit should be /// abandoned completely. - fn timeouts(&self, action: &Action) -> (Duration, Duration); + fn timeouts(&mut self, action: &Action) -> (Duration, Duration); /// Return true if we're currently trying to learn more timeouts /// by launching testing circuits. fn learning_timeouts(&self) -> bool; + + /// Replace the network parameters used by this estimator (if any) + /// with ones derived from `params`. + fn update_params(&mut self, params: &tor_netdir::params::NetParameters); + + /// Construct a new ParetoTimeoutState to represent the current state + /// of this estimator, if it is possible to store the state to disk. + /// + /// TODO: change the type used for the state. + fn build_state(&mut self) -> Option; } /// A possible action for which we can try to estimate a timeout. diff --git a/crates/tor-circmgr/src/timeouts/estimator.rs b/crates/tor-circmgr/src/timeouts/estimator.rs new file mode 100644 index 000000000..871a6b32b --- /dev/null +++ b/crates/tor-circmgr/src/timeouts/estimator.rs @@ -0,0 +1,131 @@ +//! Declarations for a [`TimeoutEstimator`] type that can change implementation. + +use crate::timeouts::{Action, TimeoutEstimator}; +use std::sync::Mutex; +use std::time::Duration; +use tor_netdir::params::NetParameters; + +/// A timeout estimator that can change its inner implementation and share its +/// implementation among multiple threads. +pub(crate) struct Estimator { + /// The estimator we're currently using. + inner: Mutex>, +} + +impl Estimator { + /// Construct a new estimator from some variant. + pub(crate) fn new(est: impl TimeoutEstimator + Send + 'static) -> Self { + Self { + inner: Mutex::new(Box::new(est)), + } + } + + /// Record that a given circuit hop has completed. + /// + /// The `hop` number is a zero-indexed value for which hop just completed. + /// + /// The `delay` value is the amount of time after we first launched the + /// circuit. + /// + /// If this is the last hop of the circuit, then `is_last` is true. + pub(crate) fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool) { + let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned."); + + inner.note_hop_completed(hop, delay, is_last); + } + + /// Record that a circuit failed to complete because it took too long. + /// + /// The `hop` number is a the number of hops that were successfully + /// completed. + /// + /// The `delay` number is the amount of time after we first launched the + /// circuit. + pub(crate) fn note_circ_timeout(&self, hop: u8, delay: Duration) { + let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned."); + inner.note_circ_timeout(hop, delay); + } + + /// Return the current estimation for how long we should wait for a given + /// [`Action`] to complete. + /// + /// This function should return a 2-tuple of `(timeout, abandon)` + /// durations. After `timeout` has elapsed since circuit launch, + /// the circuit should no longer be used, but we should still keep + /// building it in order see how long it takes. After `abandon` + /// has elapsed since circuit launch, the circuit should be + /// abandoned completely. + pub(crate) fn timeouts(&self, action: &Action) -> (Duration, Duration) { + let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned."); + + inner.timeouts(action) + } + + /// Return true if we're currently trying to learn more timeouts + /// by launching testing circuits. + pub(crate) fn learning_timeouts(&self) -> bool { + let inner = self.inner.lock().expect("Timeout estimator lock poisoned."); + inner.learning_timeouts() + } + + /// Replace the network parameters used by this estimator (if any) + /// with ones derived from `params`. + pub(crate) fn update_params(&self, params: &NetParameters) { + let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned."); + inner.update_params(params); + } + + /// Store any state associated with this timeout esimator into `storage`. + pub(crate) fn save_state(&self, storage: &crate::TimeoutStateHandle) -> crate::Result<()> { + let state = { + let mut inner = self.inner.lock().expect("Timeout estimator lock poisoned."); + inner.build_state() + }; + if let Some(state) = state { + storage.store(&state)?; + } + Ok(()) + } +} + +/* +/// An enum that can hold an estimator state. +enum EstimatorInner { + Pareto(ParetoTimeoutEstimator), +} + +impl TimeoutEstimatorImpl for EstimatorInner { + fn note_hop_completed(&mut self, hop: u8, delay: Duration, is_last: bool) { + match self { + EstimatorInner::Pareto(mut p) => p.note_hop_completed(hop, delay, is_last) + } + } + + fn note_circ_timeout(&mut self, hop: u8, delay: Duration) { + match self { + EstimatorInner::Pareto(mut p) => p.note_circ_timeout(hop, delay) + } + } + + fn timeouts(&mut self, action: &Action) -> (Duration, Duration) { + match self { + EstimatorInner::Pareto(mut p) => p.timeouts(action) + } + } + + fn learning_timeouts(&self) -> bool { + match self { + EstimatorInner::Pareto(p) => p.learning_timeouts() + } + } + + fn update_params(&mut self, params: &tor_netdir::NetParameters) { + match self { + EstimatorInner::Pareto(mut p) => p.update_params(params), + } + } + + +} + +*/ diff --git a/crates/tor-circmgr/src/timeouts/pareto.rs b/crates/tor-circmgr/src/timeouts/pareto.rs index 411042d08..1ceade820 100644 --- a/crates/tor-circmgr/src/timeouts/pareto.rs +++ b/crates/tor-circmgr/src/timeouts/pareto.rs @@ -16,8 +16,8 @@ use bounded_vec_deque::BoundedVecDeque; use serde::{Deserialize, Serialize}; use std::collections::BTreeMap; use std::convert::TryInto; -use std::sync::Mutex; use std::time::Duration; +use tor_netdir::params::NetParameters; use super::Action; @@ -402,8 +402,8 @@ impl Default for Params { } } -impl From<&tor_netdir::params::NetParameters> for Params { - fn from(p: &tor_netdir::params::NetParameters) -> Params { +impl From<&NetParameters> for Params { + fn from(p: &NetParameters) -> Params { // Because of the underlying bounds, the "unwrap_or_else" // conversions here should be impossible, and the "as" // conversions should always be in-range. @@ -431,10 +431,15 @@ impl From<&tor_netdir::params::NetParameters> for Params { } } -/// Implementation type for [`ParetoTimeoutEstimator`] +/// Tor's default circuit build timeout estimator. /// -/// (This type hides behind a mutex to allow concurrent modification.) -struct ParetoEstimatorInner { +/// This object records a set of observed circuit build times, and +/// uses it to determine good values for how long we should allow +/// circuits to build. +/// +/// For full details of the algorithms used, see +/// [`path-spec.txt`](https://gitlab.torproject.org/tpo/core/torspec/-/blob/master/path-spec.txt). +pub(crate) struct ParetoTimeoutEstimator { /// Our observations for circuit build times and success/failure /// history. history: History, @@ -457,19 +462,6 @@ struct ParetoEstimatorInner { p: Params, } -/// Tor's default circuit build timeout estimator. -/// -/// This object records a set of observed circuit build times, and -/// uses it to determine good values for how long we should allow -/// circuits to build. -/// -/// For full details of the algorithms used, see -/// [`path-spec.txt`](https://gitlab.torproject.org/tpo/core/torspec/-/blob/master/path-spec.txt). -pub(crate) struct ParetoTimeoutEstimator { - /// The actual data inside this estimator. - est: Mutex, -} - impl Default for ParetoTimeoutEstimator { fn default() -> Self { Self::from_history(History::new_empty()) @@ -495,14 +487,11 @@ impl ParetoTimeoutEstimator { /// object. fn from_history(history: History) -> Self { let p = Params::default(); - let inner = ParetoEstimatorInner { + ParetoTimeoutEstimator { history, timeouts: None, fallback_timeouts: p.default_thresholds, p, - }; - ParetoTimeoutEstimator { - est: Mutex::new(inner), } } @@ -513,114 +502,6 @@ impl ParetoTimeoutEstimator { Self::from_history(history) } - /// Construct a new ParetoTimeoutState to represent the current state - /// of this estimator. - pub(crate) fn build_state(&self) -> ParetoTimeoutState { - let mut this = self - .est - .lock() - .expect("Poisoned lock for ParetoTimeoutEstimator"); - - let cur_timeout = MsecDuration::new_saturating(&this.base_timeouts().0); - ParetoTimeoutState { - version: 1, - histogram: this.history.sparse_histogram().collect(), - current_timeout: Some(cur_timeout), - } - } - - /// Change the parameters used for this estimator. - pub(crate) fn update_params(&self, parameters: Params) { - let mut this = self - .est - .lock() - .expect("Poisoned lock for ParetoTimeoutEstimator"); - - this.p = parameters; - let new_success_len = this.p.success_history_len; - this.history.set_success_history_len(new_success_len); - } -} - -impl super::TimeoutEstimator for ParetoTimeoutEstimator { - fn note_hop_completed(&self, hop: u8, delay: Duration, is_last: bool) { - let mut this = self - .est - .lock() - .expect("Poisoned lock for ParetoTimeoutEstimator"); - - if hop == this.p.significant_hop { - let time = MsecDuration::new_saturating(&delay); - this.history.add_time(time); - this.timeouts.take(); - } - if is_last { - this.history.add_success(true); - } - } - - fn note_circ_timeout(&self, hop: u8, _delay: Duration) { - // XXXXX This only counts if we have recent-enough - // activity. See circuit_build_times_network_check_live. - if hop > 0 { - let mut this = self - .est - .lock() - .expect("Poisoned lock for ParetoTimeoutEstimator"); - this.history.add_success(false); - if this.history.n_recent_timeouts() > this.p.reset_after_timeouts { - let base_timeouts = this.base_timeouts(); - this.history.clear(); - this.timeouts.take(); - // If we already had a timeout that was at least the - // length of our fallback timeouts, we should double - // those fallback timeouts. - if base_timeouts.0 >= this.fallback_timeouts.0 { - this.fallback_timeouts.0 *= 2; - this.fallback_timeouts.1 *= 2; - } - } - } - } - - fn timeouts(&self, action: &Action) -> (Duration, Duration) { - let mut this = self - .est - .lock() - .expect("Poisoned lock for ParetoTimeoutEstimator"); - - let (base_t, base_a) = if this.p.use_estimates { - this.base_timeouts() - } else { - // If we aren't using this estimator, then just return the - // default thresholds from our parameters. - return this.p.default_thresholds; - }; - - let reference_action = Action::BuildCircuit { - length: this.p.significant_hop as usize + 1, - }; - debug_assert!(reference_action.timeout_scale() > 0); - - let multiplier = - (action.timeout_scale() as f64) / (reference_action.timeout_scale() as f64); - - // TODO-SPEC The spec define any of this. Tor doesn't multiply the - // abandon timeout. - // XXXX `mul_f64()` can panic if we overflow Duration. - (base_t.mul_f64(multiplier), base_a.mul_f64(multiplier)) - } - - fn learning_timeouts(&self) -> bool { - let this = self - .est - .lock() - .expect("Poisoned lock for ParetoTimeoutEstimator"); - this.p.use_estimates && this.history.n_times() < this.p.min_observations.into() - } -} - -impl ParetoEstimatorInner { /// Compute an unscaled basic pair of timeouts for a circuit of /// the "normal" length. /// @@ -660,6 +541,82 @@ impl ParetoEstimatorInner { } } +impl super::TimeoutEstimator for ParetoTimeoutEstimator { + fn update_params(&mut self, p: &NetParameters) { + let parameters = p.into(); + self.p = parameters; + let new_success_len = self.p.success_history_len; + self.history.set_success_history_len(new_success_len); + } + + fn note_hop_completed(&mut self, hop: u8, delay: Duration, is_last: bool) { + if hop == self.p.significant_hop { + let time = MsecDuration::new_saturating(&delay); + self.history.add_time(time); + self.timeouts.take(); + } + if is_last { + self.history.add_success(true); + } + } + + fn note_circ_timeout(&mut self, hop: u8, _delay: Duration) { + // XXXXX This only counts if we have recent-enough + // activity. See circuit_build_times_network_check_live. + if hop > 0 { + self.history.add_success(false); + if self.history.n_recent_timeouts() > self.p.reset_after_timeouts { + let base_timeouts = self.base_timeouts(); + self.history.clear(); + self.timeouts.take(); + // If we already had a timeout that was at least the + // length of our fallback timeouts, we should double + // those fallback timeouts. + if base_timeouts.0 >= self.fallback_timeouts.0 { + self.fallback_timeouts.0 *= 2; + self.fallback_timeouts.1 *= 2; + } + } + } + } + + fn timeouts(&mut self, action: &Action) -> (Duration, Duration) { + let (base_t, base_a) = if self.p.use_estimates { + self.base_timeouts() + } else { + // If we aren't using this estimator, then just return the + // default thresholds from our parameters. + return self.p.default_thresholds; + }; + + let reference_action = Action::BuildCircuit { + length: self.p.significant_hop as usize + 1, + }; + debug_assert!(reference_action.timeout_scale() > 0); + + let multiplier = + (action.timeout_scale() as f64) / (reference_action.timeout_scale() as f64); + + // TODO-SPEC The spec define any of self. Tor doesn't multiply the + // abandon timeout. + // XXXX `mul_f64()` can panic if we overflow Duration. + (base_t.mul_f64(multiplier), base_a.mul_f64(multiplier)) + } + + fn learning_timeouts(&self) -> bool { + self.p.use_estimates && self.history.n_times() < self.p.min_observations.into() + } + + fn build_state(&mut self) -> Option { + let cur_timeout = MsecDuration::new_saturating(&self.base_timeouts().0); + Some(ParetoTimeoutState { + version: 1, + histogram: self.history.sparse_histogram().collect(), + current_timeout: Some(cur_timeout), + }) + } +} + #[cfg(test)] mod test { #![allow(clippy::unwrap_used)] @@ -847,19 +804,16 @@ mod test { #[test] fn pareto_estimate_timeout() { - let est = ParetoTimeoutEstimator::default(); + let mut est = ParetoTimeoutEstimator::default(); assert_eq!( est.timeouts(&b3()), (Duration::from_secs(60), Duration::from_secs(60)) ); - { - // Set the parameters up to mimic the situation in - // `pareto_estimate` above. - let mut inner = est.est.lock().unwrap(); - inner.p.min_observations = 0; - inner.p.n_modes_for_xm = 2; - } + // Set the parameters up to mimic the situation in + // `pareto_estimate` above. + est.p.min_observations = 0; + est.p.n_modes_for_xm = 2; assert_eq!( est.timeouts(&b3()), (Duration::from_secs(60), Duration::from_secs(60)) @@ -884,16 +838,12 @@ mod test { #[test] fn pareto_estimate_clear() { - let est = ParetoTimeoutEstimator::default(); + let mut est = ParetoTimeoutEstimator::default(); // Set the parameters up to mimic the situation in // `pareto_estimate` above. - let params = Params { - min_observations: 1, - n_modes_for_xm: 2, - ..Params::default() - }; - est.update_params(params); + let params = NetParameters::from_map(&"cbtmincircs=1 cbtnummodes=2".parse().unwrap()); + est.update_params(¶ms); assert_eq!(est.timeouts(&b3()).0.as_micros(), 60_000_000); assert!(est.learning_timeouts()); @@ -904,10 +854,7 @@ mod test { } assert_ne!(est.timeouts(&b3()).0.as_micros(), 60_000_000); assert!(!est.learning_timeouts()); - { - let inner = est.est.lock().unwrap(); - assert_eq!(inner.history.n_recent_timeouts(), 0); - } + assert_eq!(est.history.n_recent_timeouts(), 0); // 17 timeouts happen and we're still getting real numbers... for _ in 0..18 { @@ -941,18 +888,18 @@ mod test { // that the histogram conversion happens. use rand::Rng; - let est = ParetoTimeoutEstimator::default(); + let mut est = ParetoTimeoutEstimator::default(); let mut rng = rand::thread_rng(); for _ in 0..1000 { let d = Duration::from_millis(rng.gen_range(10..3_000)); est.note_hop_completed(2, d, true); } - let state = est.build_state(); + let state = est.build_state().unwrap(); assert_eq!(state.version, 1); assert!(state.current_timeout.is_some()); - let est2 = ParetoTimeoutEstimator::from_state(state); + let mut est2 = ParetoTimeoutEstimator::from_state(state); let act = Action::BuildCircuit { length: 3 }; // This isn't going to be exact, since we're recording histogram bins // instead of exact timeouts. diff --git a/crates/tor-netdir/src/params.rs b/crates/tor-netdir/src/params.rs index 09a12aa97..4378242bf 100644 --- a/crates/tor-netdir/src/params.rs +++ b/crates/tor-netdir/src/params.rs @@ -293,6 +293,15 @@ impl Default for NetParameters { } impl NetParameters { + /// Construct a new NetParameters from a given list of key=value parameters. + /// + /// Unrecognized parameters are ignored. + pub fn from_map(p: &tor_netdoc::doc::netstatus::NetParams) -> Self { + let mut params = NetParameters::default(); + let _ = params.saturating_update(p.iter()); + params + } + /// Replace a list of parameters, using the logic of /// `set_saturating`. ///