From fa1d2f453ade3c927ab71e14fc485396763b81f2 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 20 Oct 2022 09:53:24 -0400 Subject: [PATCH] guardmgr: Hold FallbackDir in fallback::set::Entry This resolves an old TODO, and will simplify our work a little. --- crates/tor-guardmgr/src/fallback/set.rs | 86 +++++++++++++------------ crates/tor-guardmgr/src/ids.rs | 11 +++- crates/tor-guardmgr/src/lib.rs | 7 +- 3 files changed, 61 insertions(+), 43 deletions(-) diff --git a/crates/tor-guardmgr/src/fallback/set.rs b/crates/tor-guardmgr/src/fallback/set.rs index e60ff9477..965946385 100644 --- a/crates/tor-guardmgr/src/fallback/set.rs +++ b/crates/tor-guardmgr/src/fallback/set.rs @@ -3,6 +3,7 @@ use crate::skew::SkewObservation; use rand::seq::IteratorRandom; use std::time::{Duration, Instant}; +use tor_linkspec::HasRelayIds; use super::{DirStatus, FallbackDir, FallbackDirBuilder}; use crate::fallback::default_fallbacks; @@ -73,10 +74,7 @@ pub(crate) struct FallbackState { #[derive(Debug, Clone)] pub(super) struct Entry { /// The inner fallback directory. - /// - /// (TODO: We represent this as a `FirstHop`, which could technically hold a - /// guard as well. Ought to fix that.) - fallback: crate::FirstHop, + fallback: FallbackDir, /// Whether the directory is currently usable, and if not, when we can retry /// it. @@ -93,7 +91,6 @@ const FALLBACK_RETRY_FLOOR: Duration = Duration::from_secs(150); impl From for Entry { fn from(fallback: FallbackDir) -> Self { - let fallback = fallback.as_guard(); let status = DirStatus::new(FALLBACK_RETRY_FLOOR); Entry { fallback, @@ -103,22 +100,20 @@ impl From for Entry { } } -impl Entry { - /// Return the identity for this fallback entry. - fn id(&self) -> &FallbackId { - use crate::ids::FirstHopIdInner::*; - match &self.fallback.id().0 { - Fallback(id) => id, - _ => panic!("Somehow we constructed a fallback object with a non-fallback id!"), - } +impl HasRelayIds for Entry { + fn identity( + &self, + key_type: tor_linkspec::RelayIdType, + ) -> Option> { + self.fallback.identity(key_type) } } impl From for FallbackState { fn from(list: FallbackList) -> Self { let mut fallbacks: Vec = list.fallbacks.into_iter().map(|fb| fb.into()).collect(); - fallbacks.sort_by(|x, y| x.id().cmp(y.id())); - fallbacks.dedup_by(|x, y| x.id() == y.id()); + fallbacks.sort_by(|x, y| x.cmp_by_relay_ids(y)); + fallbacks.dedup_by(|x, y| x.same_relay_ids(y)); FallbackState { fallbacks } } } @@ -130,7 +125,7 @@ impl FallbackState { rng: &mut R, now: Instant, filter: &crate::GuardFilter, - ) -> Result<&crate::FirstHop, PickGuardError> { + ) -> Result<&FallbackDir, PickGuardError> { if self.fallbacks.is_empty() { return Err(PickGuardError::NoCandidatesAvailable); } @@ -163,7 +158,7 @@ impl FallbackState { /// Return a reference to the entry whose identity is `id`, if there is one. fn get(&self, id: &FallbackId) -> Option<&Entry> { - match self.fallbacks.binary_search_by(|e| e.id().cmp(id)) { + match self.fallbacks.binary_search_by(|e| e.cmp_by_relay_ids(id)) { Ok(idx) => Some(&self.fallbacks[idx]), Err(_) => None, } @@ -171,7 +166,7 @@ impl FallbackState { /// Return a mutable reference to the entry whose identity is `id`, if there is one. fn get_mut(&mut self, id: &FallbackId) -> Option<&mut Entry> { - match self.fallbacks.binary_search_by(|e| e.id().cmp(id)) { + match self.fallbacks.binary_search_by(|e| e.cmp_by_relay_ids(id)) { Ok(idx) => Some(&mut self.fallbacks[idx]), Err(_) => None, } @@ -208,11 +203,11 @@ impl FallbackState { itertools::merge_join_by( self.fallbacks.iter_mut(), other.fallbacks.into_iter(), - |a, b| a.fallback.id().cmp(b.fallback.id()), + |a, b| a.fallback.cmp_by_relay_ids(&b.fallback), ) .for_each(|entry| { if let Both(entry, other) = entry { - debug_assert_eq!(entry.fallback.id(), other.fallback.id()); + debug_assert!(entry.fallback.same_relay_ids(&other.fallback)); entry.status = other.status; } }); @@ -246,7 +241,6 @@ mod test { //! use super::*; - use crate::FirstHopId; use rand::Rng; use tor_basic_utils::test_rng::testing_rng; @@ -269,6 +263,7 @@ mod test { #[test] fn construct_fallback_set() { use rand::seq::SliceRandom; + use std::cmp::Ordering as O; // fabricate some fallbacks. let mut rng = testing_rng(); @@ -289,14 +284,23 @@ mod test { // inspect the generated set assert_eq!(set.fallbacks.len(), 4); - assert!(set.fallbacks[0].id() < set.fallbacks[1].id()); - assert!(set.fallbacks[1].id() < set.fallbacks[2].id()); - assert!(set.fallbacks[2].id() < set.fallbacks[3].id()); + assert_eq!( + set.fallbacks[0].cmp_by_relay_ids(&set.fallbacks[1]), + O::Less + ); + assert_eq!( + set.fallbacks[1].cmp_by_relay_ids(&set.fallbacks[2]), + O::Less + ); + assert_eq!( + set.fallbacks[2].cmp_by_relay_ids(&set.fallbacks[3]), + O::Less + ); // use the constructed set a little. for fb in fbs.iter() { let id = FallbackId::from_relay_ids(fb); - assert_eq!(set.get_mut(&id).unwrap().id(), &id); + assert_eq!(set.get_mut(&id).unwrap().cmp_by_relay_ids(&id), O::Equal); } assert!(set.get_mut(&id_other).is_none()); @@ -315,7 +319,7 @@ mod test { .fallbacks .iter() .zip(set2.fallbacks.iter()) - .all(|(ent1, ent2)| ent1.id() == ent2.id())); + .all(|(ent1, ent2)| ent1.same_relay_ids(ent2))); } #[test] @@ -336,29 +340,31 @@ mod test { let mut counts = [0_usize; 4]; let now = Instant::now(); dbg!("A"); - fn lookup_idx(set: &FallbackState, id: &FirstHopId) -> Option { - if let FirstHopId(crate::ids::FirstHopIdInner::Fallback(id)) = id { - set.fallbacks.binary_search_by(|ent| ent.id().cmp(id)).ok() - } else { - None - } + fn lookup_idx(set: &FallbackState, id: &impl HasRelayIds) -> Option { + set.fallbacks + .binary_search_by(|ent| ent.fallback.cmp_by_relay_ids(id)) + .ok() } // Basic case: everybody is up. for _ in 0..100 { let fb = set.choose(&mut rng, now, &filter).unwrap(); - let idx = lookup_idx(&set, fb.id()).unwrap(); + let idx = lookup_idx(&set, fb).unwrap(); counts[idx] += 1; } dbg!("B"); assert!(counts.iter().all(|v| *v > 0)); // Mark somebody down and make sure they don't get chosen. - let ids: Vec<_> = set.fallbacks.iter().map(|fb| fb.id().clone()).collect(); + let ids: Vec<_> = set + .fallbacks + .iter() + .map(|ent| FallbackId::from_relay_ids(&ent.fallback)) + .collect(); set.note_failure(&ids[2], now); counts = [0; 4]; for _ in 0..100 { let fb = set.choose(&mut rng, now, &filter).unwrap(); - let idx = lookup_idx(&set, fb.id()).unwrap(); + let idx = lookup_idx(&set, fb).unwrap(); counts[idx] += 1; } assert_eq!(counts.iter().filter(|v| **v > 0).count(), 3); @@ -394,7 +400,11 @@ mod test { ]; let list: FallbackList = fbs.clone().into(); let mut set: FallbackState = list.into(); - let ids: Vec<_> = set.fallbacks.iter().map(|fb| fb.id().clone()).collect(); + let ids: Vec<_> = set + .fallbacks + .iter() + .map(|ent| FallbackId::from_relay_ids(&ent.fallback)) + .collect(); let now = Instant::now(); @@ -424,10 +434,6 @@ mod test { assert!(set.fallbacks[0].status.next_retriable().is_none()); assert!(set.fallbacks[0].status.usable_at(now)); - for id in ids.iter() { - dbg!(id, set.get_mut(id).map(|e| e.id())); - } - // Make a new set with slightly different members; make sure that we can copy stuff successfully. let mut fbs2: Vec<_> = fbs .into_iter() diff --git a/crates/tor-guardmgr/src/ids.rs b/crates/tor-guardmgr/src/ids.rs index 9d337a53c..a9f0241ef 100644 --- a/crates/tor-guardmgr/src/ids.rs +++ b/crates/tor-guardmgr/src/ids.rs @@ -2,7 +2,7 @@ use derive_more::AsRef; use serde::{Deserialize, Serialize}; -use tor_linkspec::RelayIds; +use tor_linkspec::{HasRelayIds, RelayIds}; #[cfg(test)] use tor_llcrypto::pk; @@ -26,6 +26,15 @@ impl FallbackId { } } +impl HasRelayIds for FallbackId { + fn identity( + &self, + key_type: tor_linkspec::RelayIdType, + ) -> Option> { + self.0.identity(key_type) + } +} + /// An identifier for a sampled guard. /// /// This is a separate type from GuardId and FirstHopId to avoid confusion diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index 76a3e344b..75b793506 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -1165,8 +1165,11 @@ impl GuardMgrInner { ) -> Result<(sample::ListKind, FirstHop), PickGuardError> { let filt = self.guards.active_guards().filter(); - let fallback = self.fallbacks.choose(&mut rand::thread_rng(), now, filt)?; - let fallback = filt.modify_hop(fallback.clone())?; + let fallback = self + .fallbacks + .choose(&mut rand::thread_rng(), now, filt)? + .as_guard(); + let fallback = filt.modify_hop(fallback)?; Ok((sample::ListKind::Fallback, fallback)) } }