From 08473872abccf389db261b4631f83a7cf42c7976 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 28 Oct 2022 12:28:14 -0400 Subject: [PATCH] guardmgr: Refactor candidate info; add `full_dir_info` Previously we always set `dir_info_missing` to `false` for new guards, since new guards could only be taken from ones that were present in the NetDir. But for bridges, we don't download their info until _after_ we have chosen them as guards. --- crates/tor-guardmgr/src/bridge/descs.rs | 17 ++++++--- crates/tor-guardmgr/src/guard.rs | 31 +++++++++++++--- crates/tor-guardmgr/src/sample.rs | 14 ++++---- crates/tor-guardmgr/src/sample/candidate.rs | 40 +++++++++++++-------- 4 files changed, 72 insertions(+), 30 deletions(-) diff --git a/crates/tor-guardmgr/src/bridge/descs.rs b/crates/tor-guardmgr/src/bridge/descs.rs index e32b50e14..b0c678a90 100644 --- a/crates/tor-guardmgr/src/bridge/descs.rs +++ b/crates/tor-guardmgr/src/bridge/descs.rs @@ -9,7 +9,7 @@ use std::time::SystemTime; use crate::{ bridge::BridgeConfig, - sample::{CandidateStatus, Universe, WeightThreshold}, + sample::{Candidate, CandidateStatus, Universe, WeightThreshold}, }; use dyn_clone::DynClone; use futures::stream::BoxStream; @@ -214,11 +214,12 @@ impl Universe for BridgeSet { let bridge_relay = self.relay_by_bridge(bridge); // Logic here is similar to that of "contains" if bridge_relay.has_all_relay_ids_from(guard) { - CandidateStatus::Present { + CandidateStatus::Present(Candidate { listed_as_guard: true, is_dir_cache: true, // all bridges are directory caches. + full_dir_info: bridge_relay.has_descriptor(), owned_target: OwnedChanTarget::from_chan_target(&bridge_relay), - } + }) } else if bridge_relay.has_descriptor() { CandidateStatus::Absent } else { @@ -252,7 +253,7 @@ impl Universe for BridgeSet { pre_existing: &tor_linkspec::ByRelayIds, filter: &crate::GuardFilter, n: usize, - ) -> Vec<(tor_linkspec::OwnedChanTarget, tor_netdir::RelayWeight)> + ) -> Vec<(Candidate, tor_netdir::RelayWeight)> where T: HasRelayIds, { @@ -268,8 +269,14 @@ impl Universe for BridgeSet { .choose_multiple(&mut rand::thread_rng(), n) .into_iter() .map(|bridge_config| { + let relay = self.relay_by_bridge(bridge_config); ( - OwnedChanTarget::from_chan_target(&self.relay_by_bridge(bridge_config)), + Candidate { + listed_as_guard: true, + is_dir_cache: true, + full_dir_info: relay.has_descriptor(), + owned_target: OwnedChanTarget::from_chan_target(&relay), + }, RelayWeight::from(0), ) }) diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs index 46c255499..31b6c87bd 100644 --- a/crates/tor-guardmgr/src/guard.rs +++ b/crates/tor-guardmgr/src/guard.rs @@ -10,6 +10,7 @@ use std::time::{Duration, Instant, SystemTime}; use tracing::{trace, warn}; use crate::dirstatus::DirStatus; +use crate::sample::Candidate; use crate::skew::SkewObservation; use crate::util::randomize_time; use crate::{ids::GuardId, GuardParams, GuardRestriction, GuardUsage}; @@ -224,11 +225,31 @@ pub(crate) enum NewlyConfirmed { } impl Guard { + /// Create a new unused [`Guard`] from a [`Candidate`]. + pub(crate) fn from_candidate( + candidate: Candidate, + now: SystemTime, + params: &GuardParams, + ) -> Self { + let Candidate { + is_dir_cache, + full_dir_info, + owned_target, + .. + } = candidate; + + Guard { + is_dir_cache, + dir_info_missing: !full_dir_info, + ..Self::from_chan_target(&owned_target, now, params) + } + } + /// Create a new unused [`Guard`] from a [`ChanTarget`]. /// /// This function doesn't check whether the provided relay is a /// suitable guard node or not: that's up to the caller to decide. - pub(crate) fn from_chan_target(relay: &T, now: SystemTime, params: &GuardParams) -> Self + fn from_chan_target(relay: &T, now: SystemTime, params: &GuardParams) -> Self where T: ChanTarget, { @@ -268,7 +289,7 @@ impl Guard { disabled: None, confirmed_at: None, unlisted_since: None, - dir_info_missing: false, // TODO pt-client this can be wrong for bridges. + dir_info_missing: false, last_tried_to_connect_at: None, reachable: Reachable::Unknown, retry_at: None, @@ -494,11 +515,12 @@ impl Guard { // for the guard, we won't know its full set of identities. use sample::CandidateStatus::*; let listed_as_guard = match universe.status(self) { - Present { + Present(Candidate { listed_as_guard, is_dir_cache, + full_dir_info, owned_target, - } => { + }) => { // Update address information. self.orports = owned_target.addrs().into(); // Update Pt information. @@ -512,6 +534,7 @@ impl Guard { // Update our IDs: the Relay will have strictly more. assert!(owned_target.has_all_relay_ids_from(self)); self.id = GuardId(RelayIds::from_relay_ids(&owned_target)); + self.dir_info_missing = !full_dir_info; listed_as_guard } diff --git a/crates/tor-guardmgr/src/sample.rs b/crates/tor-guardmgr/src/sample.rs index 731dad199..985175241 100644 --- a/crates/tor-guardmgr/src/sample.rs +++ b/crates/tor-guardmgr/src/sample.rs @@ -15,7 +15,7 @@ use crate::{ }; use crate::{FirstHop, GuardSetSelector}; use tor_basic_utils::iter::{FilterCount, IteratorExt as _}; -use tor_linkspec::{ByRelayIds, ChanTarget, HasRelayIds}; +use tor_linkspec::{ByRelayIds, HasRelayIds}; use itertools::Itertools; use rand::seq::SliceRandom; @@ -25,7 +25,7 @@ use std::collections::{HashMap, HashSet}; use std::time::{Instant, SystemTime}; use tracing::{debug, info}; -pub(crate) use candidate::{CandidateStatus, Universe, WeightThreshold}; +pub(crate) use candidate::{Candidate, CandidateStatus, Universe, WeightThreshold}; /// A set of sampled guards, along with various orderings on subsets /// of the sample. @@ -455,11 +455,11 @@ impl GuardSet { // We've reached our target; no need to add more. break; } - if self.active_filter.permits(&candidate) { + if self.active_filter.permits(&candidate.owned_target) { n_filtered_usable += 1; } current_weight += weight; - self.add_guard(&candidate, now, params); + self.add_guard(candidate, now, params); any_added = true; } self.assert_consistency(); @@ -469,13 +469,13 @@ impl GuardSet { /// Add `relay` as a new guard. /// /// Does nothing if it is already a guard. - fn add_guard(&mut self, relay: &T, now: SystemTime, params: &GuardParams) { - let id = GuardId::from_relay_ids(relay); + fn add_guard(&mut self, relay: Candidate, now: SystemTime, params: &GuardParams) { + let id = GuardId::from_relay_ids(&relay.owned_target); if self.guards.by_all_ids(&id).is_some() { return; } debug!(guard_id=?id, "Adding guard to sample."); - let guard = Guard::from_chan_target(relay, now, params); + let guard = Guard::from_candidate(relay, now, params); self.guards.insert(guard); self.sample.push(id); self.primary_guards_invalidated = true; diff --git a/crates/tor-guardmgr/src/sample/candidate.rs b/crates/tor-guardmgr/src/sample/candidate.rs index 0b4e24fd5..87d98d4c0 100644 --- a/crates/tor-guardmgr/src/sample/candidate.rs +++ b/crates/tor-guardmgr/src/sample/candidate.rs @@ -40,7 +40,7 @@ pub(crate) trait Universe { pre_existing: &ByRelayIds, filter: &GuardFilter, n: usize, - ) -> Vec<(OwnedChanTarget, RelayWeight)> + ) -> Vec<(Candidate, RelayWeight)> where T: HasRelayIds; } @@ -50,15 +50,7 @@ pub(crate) trait Universe { #[derive(Clone, Debug)] pub(crate) enum CandidateStatus { /// The candidate is definitely present in some form. - Present { - /// True if the candidate is not currently disabled for use as a guard. - listed_as_guard: bool, - /// True if the candidate can be used as a directory cache. - is_dir_cache: bool, - /// Information about connecting to the candidate and using it to build - /// a channel. - owned_target: OwnedChanTarget, - }, + Present(Candidate), /// The candidate is definitely not in the [`Universe`]. Absent, /// We would need to download more directory information to be sure whether @@ -66,6 +58,20 @@ pub(crate) enum CandidateStatus { Uncertain, } +/// Information about a candidate that we have selected as a guard. +#[derive(Clone, Debug)] +pub(crate) struct Candidate { + /// True if the candidate is not currently disabled for use as a guard. + pub(crate) listed_as_guard: bool, + /// True if the candidate can be used as a directory cache. + pub(crate) is_dir_cache: bool, + /// True if we have complete directory information about this candidate. + pub(crate) full_dir_info: bool, + /// Information about connecting to the candidate and using it to build + /// a channel. + pub(crate) owned_target: OwnedChanTarget, +} + /// Information about how much of the universe we are using in a guard sample, /// and how much we are allowed to use. /// @@ -93,11 +99,12 @@ impl Universe for NetDir { fn status(&self, guard: &T) -> CandidateStatus { match NetDir::by_ids(self, guard) { - Some(relay) => CandidateStatus::Present { + Some(relay) => CandidateStatus::Present(Candidate { listed_as_guard: relay.is_flagged_guard(), is_dir_cache: relay.is_dir_cache(), owned_target: OwnedChanTarget::from_chan_target(&relay), - }, + full_dir_info: true, + }), None => match NetDir::ids_listed(self, guard) { Some(true) => panic!("ids_listed said true, but by_ids said none!"), Some(false) => CandidateStatus::Absent, @@ -139,7 +146,7 @@ impl Universe for NetDir { pre_existing: &ByRelayIds, filter: &GuardFilter, n: usize, - ) -> Vec<(OwnedChanTarget, RelayWeight)> + ) -> Vec<(Candidate, RelayWeight)> where T: HasRelayIds, { @@ -165,7 +172,12 @@ impl Universe for NetDir { .iter() .map(|relay| { ( - OwnedChanTarget::from_chan_target(relay), + Candidate { + listed_as_guard: true, + is_dir_cache: true, + full_dir_info: true, + owned_target: OwnedChanTarget::from_chan_target(relay), + }, weight(self, relay).unwrap_or_else(|| RelayWeight::from(0)), ) })