From 19fdf196d89e670f3487caa756a8194076f9226b Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Wed, 19 Oct 2022 17:40:17 -0400
Subject: [PATCH 1/9] guardmgr: Add bridges sample, encode sample ID in
 FirstHopId.

The most important part of this commit is to make sure that each
`FirstHopId` includes the `GuardSetSelector` from which the guard
was selected.  Doing this lets us be certain that when we report
that a guard has succeeded or failed, we're reporting it in the
right context.

Additionally, this commit uses strum to make an iterator over the
samples, so that we can make sure that our "for each sample" code is
robust against future changes, and we don't miss the bridge sample.
---
 Cargo.lock                        |  1 +
 crates/tor-guardmgr/Cargo.toml    |  1 +
 crates/tor-guardmgr/src/guard.rs  | 19 +++----
 crates/tor-guardmgr/src/ids.rs    | 16 +++---
 crates/tor-guardmgr/src/lib.rs    | 89 ++++++++++++++++++++-----------
 crates/tor-guardmgr/src/sample.rs | 15 ++++--
 6 files changed, 89 insertions(+), 52 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index fd94d1449..7572128b8 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -3700,6 +3700,7 @@ dependencies = [
  "rand 0.8.5",
  "retain_mut",
  "serde",
+ "strum",
  "thiserror",
  "tor-basic-utils",
  "tor-config",
diff --git a/crates/tor-guardmgr/Cargo.toml b/crates/tor-guardmgr/Cargo.toml
index bae11c1bc..d8e1b4d05 100644
--- a/crates/tor-guardmgr/Cargo.toml
+++ b/crates/tor-guardmgr/Cargo.toml
@@ -41,6 +41,7 @@ postage = { version = "0.5.0", default-features = false, features = ["futures-tr
 rand = "0.8"
 retain_mut = "0.1.3"
 serde = { version = "1.0.103", features = ["derive"] }
+strum = { version = "0.24", features = ["derive"] }
 thiserror = "1"
 tor-basic-utils = { path = "../tor-basic-utils", version = "0.4.0" }
 tor-config = { path = "../tor-config", version = "0.6.0" }
diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs
index 19cba5cf6..f6314c4a4 100644
--- a/crates/tor-guardmgr/src/guard.rs
+++ b/crates/tor-guardmgr/src/guard.rs
@@ -14,7 +14,7 @@ use crate::dirstatus::DirStatus;
 use crate::skew::SkewObservation;
 use crate::util::randomize_time;
 use crate::{ids::GuardId, GuardParams, GuardRestriction, GuardUsage};
-use crate::{ExternalActivity, FirstHopId, GuardUsageKind};
+use crate::{ExternalActivity, FirstHopId, GuardSetSelector, GuardUsageKind};
 use tor_linkspec::{HasAddrs, HasRelayIds};
 use tor_persist::{Futureproof, JsonValue};
 
@@ -442,9 +442,10 @@ impl Guard {
         // not.
         let listed_as_guard = match self.listed_in(netdir) {
             Some(true) => {
-                let id: FirstHopId = self.id.clone().into();
                 // Definitely listed.
-                let relay = id.get_relay(netdir).expect("Couldn't get a listed relay?!");
+                let relay = netdir
+                    .by_ids(&self.id.0)
+                    .expect("Couldn't get a listed relay?!");
                 // Update address information.
                 self.orports = relay.addrs().into();
                 // Check whether we can currently use it as a directory cache.
@@ -662,9 +663,9 @@ impl Guard {
     }
 
     /// Return a [`FirstHop`](crate::FirstHop) object to represent this guard.
-    pub(crate) fn get_external_rep(&self) -> crate::FirstHop {
+    pub(crate) fn get_external_rep(&self, selection: GuardSetSelector) -> crate::FirstHop {
         crate::FirstHop {
-            id: self.id.clone().into(),
+            id: FirstHopId::in_sample(selection, self.id.clone()),
             orports: self.orports.clone(),
         }
     }
@@ -820,7 +821,7 @@ mod test {
         let g = basic_guard();
 
         assert_eq!(g.guard_id(), &id);
-        assert!(g.same_relay_ids(&FirstHopId::from(id)));
+        assert!(g.same_relay_ids(&FirstHopId::in_sample(GuardSetSelector::Default, id)));
         assert_eq!(g.addrs(), &["127.0.0.7:7777".parse().unwrap()]);
         assert_eq!(g.reachable(), Reachable::Unknown);
         assert_eq!(g.reachable(), Reachable::default());
@@ -1024,7 +1025,7 @@ mod test {
         assert!(Some(guard22.added_at) <= Some(now));
 
         // Can we still get the relay back?
-        let id: FirstHopId = guard22.id.clone().into();
+        let id = FirstHopId::in_sample(GuardSetSelector::Default, guard22.id.clone());
         let r = id.get_relay(&netdir).unwrap();
         assert!(r.same_relay_ids(&relay22));
 
@@ -1038,7 +1039,7 @@ mod test {
             vec![],
             now,
         );
-        let id: FirstHopId = guard255.id.clone().into();
+        let id = FirstHopId::in_sample(GuardSetSelector::Default, guard255.id.clone());
         assert!(id.get_relay(&netdir).is_none());
         assert!(guard255.get_weight(&netdir).is_none());
     }
@@ -1088,7 +1089,7 @@ mod test {
 
         // Try a guard that is in netdir, but not netdir2.
         let mut guard22 = Guard::new(GuardId::new([22; 32].into(), [22; 20].into()), vec![], now);
-        let id22: FirstHopId = guard22.id.clone().into();
+        let id22: FirstHopId = FirstHopId::in_sample(GuardSetSelector::Default, guard22.id.clone());
         let relay22 = id22.get_relay(&netdir).unwrap();
         assert_eq!(guard22.listed_in(&netdir), Some(true));
         guard22.update_from_netdir(&netdir);
diff --git a/crates/tor-guardmgr/src/ids.rs b/crates/tor-guardmgr/src/ids.rs
index c58853757..9d337a53c 100644
--- a/crates/tor-guardmgr/src/ids.rs
+++ b/crates/tor-guardmgr/src/ids.rs
@@ -6,6 +6,8 @@ use tor_linkspec::RelayIds;
 #[cfg(test)]
 use tor_llcrypto::pk;
 
+use crate::GuardSetSelector;
+
 /// An identifier for a fallback directory cache.
 ///
 /// This is a separate type from GuardId and FirstHopId to avoid confusion
@@ -59,7 +61,7 @@ impl GuardId {
 #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
 pub(crate) enum FirstHopIdInner {
     /// Identifies a guard.
-    Guard(GuardId),
+    Guard(GuardSetSelector, GuardId),
     /// Identifies a fallback.
     Fallback(FallbackId),
 }
@@ -76,11 +78,6 @@ pub(crate) enum FirstHopIdInner {
 #[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd)]
 pub struct FirstHopId(pub(crate) FirstHopIdInner);
 
-impl From<GuardId> for FirstHopId {
-    fn from(id: GuardId) -> Self {
-        Self(FirstHopIdInner::Guard(id))
-    }
-}
 impl From<FallbackId> for FirstHopId {
     fn from(id: FallbackId) -> Self {
         Self(FirstHopIdInner::Fallback(id))
@@ -93,7 +90,7 @@ impl AsRef<RelayIds> for FirstHopId {
     /// whether this identifies a guard or a fallback.
     fn as_ref(&self) -> &RelayIds {
         match &self.0 {
-            FirstHopIdInner::Guard(id) => id.as_ref(),
+            FirstHopIdInner::Guard(_, id) => id.as_ref(),
             FirstHopIdInner::Fallback(id) => id.as_ref(),
         }
     }
@@ -115,4 +112,9 @@ impl FirstHopId {
     pub fn get_relay<'a>(&self, netdir: &'a tor_netdir::NetDir) -> Option<tor_netdir::Relay<'a>> {
         netdir.by_ids(self)
     }
+
+    /// Construct a FirstHopId for a guard in a given sample.
+    pub(crate) fn in_sample(sample: GuardSetSelector, id: GuardId) -> Self {
+        Self(FirstHopIdInner::Guard(sample, id))
+    }
 }
diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs
index ee94b585a..76a3e344b 100644
--- a/crates/tor-guardmgr/src/lib.rs
+++ b/crates/tor-guardmgr/src/lib.rs
@@ -191,7 +191,7 @@ struct GuardMgrInner {
 }
 
 /// A selector that tells us which [`GuardSet`] of several is currently in use.
-#[derive(Clone, Debug, Eq, PartialEq, Educe)]
+#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Educe, strum::EnumIter)]
 #[educe(Default)]
 enum GuardSetSelector {
     /// The default guard set is currently in use: that's the one that we use
@@ -211,8 +211,7 @@ enum GuardSetSelector {
 }
 
 /// Persistent state for a guard manager, as serialized to disk.
-
-#[derive(Debug, Default, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
 struct GuardSets {
     /// Which set of guards is currently in use?
     #[serde(skip)]
@@ -228,6 +227,11 @@ struct GuardSets {
     #[serde(default)]
     restricted: GuardSet,
 
+    /// A guard set sampled from our configured bridges.
+    #[serde(default)]
+    #[cfg(feature = "bridge-client")]
+    bridges: GuardSet,
+
     /// Unrecognized fields, including (possibly) other guard sets.
     #[serde(flatten)]
     remaining: HashMap<String, tor_persist::JsonValue>,
@@ -499,7 +503,10 @@ impl<R: Runtime> GuardMgr<R> {
         inner.pending.insert(request_id, pending_request);
 
         match &guard.id.0 {
-            FirstHopIdInner::Guard(id) => inner.guards.active_guards_mut().record_attempt(id, now),
+            FirstHopIdInner::Guard(sample, id) => {
+                inner.guards.guards_mut(sample).record_attempt(id, now);
+            }
+
             FirstHopIdInner::Fallback(_) => {
                 // We don't record attempts for fallbacks; we only care when
                 // they have failed.
@@ -520,12 +527,11 @@ impl<R: Runtime> GuardMgr<R> {
         let ids = inner.lookup_ids(identity);
         for id in ids {
             match &id.0 {
-                FirstHopIdInner::Guard(id) => {
-                    inner.guards.active_guards_mut().record_failure(
-                        id,
-                        Some(external_failure),
-                        now,
-                    );
+                FirstHopIdInner::Guard(sample, id) => {
+                    inner
+                        .guards
+                        .guards_mut(sample)
+                        .record_failure(id, Some(external_failure), now);
                 }
                 FirstHopIdInner::Fallback(id) => {
                     if external_failure == ExternalActivity::DirCache {
@@ -593,28 +599,43 @@ impl GuardSets {
     /// complex filter types, and for bridge relays. Those will use separate
     /// `GuardSet` instances, and this accessor will choose the right one.)
     fn active_guards(&self) -> &GuardSet {
-        match self.active_set {
+        self.guards(&self.active_set)
+    }
+
+    /// Return the set of guards corresponding to the provided selector.
+    fn guards(&self, selector: &GuardSetSelector) -> &GuardSet {
+        match selector {
             GuardSetSelector::Default => &self.default,
             GuardSetSelector::Restricted => &self.restricted,
             #[cfg(feature = "bridge-client")]
-            GuardSetSelector::Bridges => todo!(), // TODO pt-client
+            GuardSetSelector::Bridges => &self.bridges,
         }
     }
 
     /// Return a mutable reference to the currently active set of guards.
     fn active_guards_mut(&mut self) -> &mut GuardSet {
-        match self.active_set {
+        self.guards_mut(&self.active_set.clone())
+    }
+
+    /// Return a mutable reference to the set of guards corresponding to the
+    /// provided selector.
+    fn guards_mut(&mut self, selector: &GuardSetSelector) -> &mut GuardSet {
+        match selector {
             GuardSetSelector::Default => &mut self.default,
             GuardSetSelector::Restricted => &mut self.restricted,
             #[cfg(feature = "bridge-client")]
-            GuardSetSelector::Bridges => todo!(), // TODO pt-client
+            GuardSetSelector::Bridges => &mut self.bridges,
         }
     }
 
     /// Update all non-persistent state for the guards in this object with the
     /// state in `other`.
-    fn copy_status_from(&mut self, other: GuardSets) {
-        self.default.copy_status_from(other.default);
+    fn copy_status_from(&mut self, mut other: GuardSets) {
+        use strum::IntoEnumIterator;
+        for sample in GuardSetSelector::iter() {
+            self.guards_mut(&sample)
+                .copy_status_from(std::mem::take(other.guards_mut(&sample)));
+        }
     }
 }
 
@@ -818,7 +839,7 @@ impl GuardMgrInner {
                 let observation = skew::SkewObservation { skew, when: now };
 
                 match &guard_id.0 {
-                    FirstHopIdInner::Guard(id) => {
+                    FirstHopIdInner::Guard(_, id) => {
                         self.guards.active_guards_mut().record_skew(id, observation);
                     }
                     FirstHopIdInner::Fallback(id) => {
@@ -844,7 +865,7 @@ impl GuardMgrInner {
                     // We don't record any other kind of circuit activity if we
                     // took the entry from the fallback list.
                 }
-                (GuardStatus::Success, FirstHopIdInner::Guard(id)) => {
+                (GuardStatus::Success, FirstHopIdInner::Guard(sample, id)) => {
                     // If we had gone too long without any net activity when we
                     // gave out this guard, and now we're seeing a circuit
                     // succeed, tell the primary guards that they might be
@@ -854,7 +875,7 @@ impl GuardMgrInner {
                     }
 
                     // The guard succeeded.  Tell the GuardSet.
-                    self.guards.active_guards_mut().record_success(
+                    self.guards.guards_mut(sample).record_success(
                         id,
                         &self.params,
                         None,
@@ -873,19 +894,19 @@ impl GuardMgrInner {
                         self.waiting.push(pending);
                     }
                 }
-                (GuardStatus::Failure, FirstHopIdInner::Guard(id)) => {
+                (GuardStatus::Failure, FirstHopIdInner::Guard(sample, id)) => {
                     self.guards
-                        .active_guards_mut()
+                        .guards_mut(sample)
                         .record_failure(id, None, runtime.now());
                     pending.reply(false);
                 }
-                (GuardStatus::AttemptAbandoned, FirstHopIdInner::Guard(id)) => {
-                    self.guards.active_guards_mut().record_attempt_abandoned(id);
+                (GuardStatus::AttemptAbandoned, FirstHopIdInner::Guard(sample, id)) => {
+                    self.guards.guards_mut(sample).record_attempt_abandoned(id);
                     pending.reply(false);
                 }
-                (GuardStatus::Indeterminate, FirstHopIdInner::Guard(id)) => {
+                (GuardStatus::Indeterminate, FirstHopIdInner::Guard(sample, id)) => {
                     self.guards
-                        .active_guards_mut()
+                        .guards_mut(sample)
                         .record_indeterminate_result(id);
                     pending.reply(false);
                 }
@@ -923,8 +944,8 @@ impl GuardMgrInner {
     {
         for id in self.lookup_ids(identity) {
             match &id.0 {
-                FirstHopIdInner::Guard(id) => {
-                    self.guards.active_guards_mut().record_success(
+                FirstHopIdInner::Guard(sample, id) => {
+                    self.guards.guards_mut(sample).record_success(
                         id,
                         &self.params,
                         Some(external_activity),
@@ -965,7 +986,7 @@ impl GuardMgrInner {
     /// a circuit is usable.
     fn guard_usability_status(&self, pending: &PendingRequest, now: Instant) -> Option<bool> {
         match &pending.guard_id().0 {
-            FirstHopIdInner::Guard(id) => self.guards.active_guards().circ_usability_status(
+            FirstHopIdInner::Guard(sample, id) => self.guards.guards(sample).circ_usability_status(
                 id,
                 pending.usage(),
                 &self.params,
@@ -1040,11 +1061,14 @@ impl GuardMgrInner {
     where
         T: tor_linkspec::HasRelayIds + ?Sized,
     {
+        use strum::IntoEnumIterator;
         let mut vec = Vec::with_capacity(2);
 
         let id = ids::GuardId::from_relay_ids(identity);
-        if self.guards.active_guards().contains(&id) {
-            vec.push(id.into());
+        for sample in GuardSetSelector::iter() {
+            if self.guards.guards(&sample).contains(&id) {
+                vec.push(FirstHopId(FirstHopIdInner::Guard(sample, id.clone())));
+            }
         }
 
         let id = ids::FallbackId::from_relay_ids(identity);
@@ -1125,9 +1149,10 @@ impl GuardMgrInner {
         usage: &GuardUsage,
         now: Instant,
     ) -> Result<(sample::ListKind, FirstHop), PickGuardError> {
+        let active_set = &self.guards.active_set;
         self.guards
-            .active_guards()
-            .pick_guard(usage, &self.params, now)
+            .guards(active_set)
+            .pick_guard(active_set, usage, &self.params, now)
     }
 
     /// Helper: Select a fallback directory.
diff --git a/crates/tor-guardmgr/src/sample.rs b/crates/tor-guardmgr/src/sample.rs
index 4a75b5921..d84e5af32 100644
--- a/crates/tor-guardmgr/src/sample.rs
+++ b/crates/tor-guardmgr/src/sample.rs
@@ -8,10 +8,10 @@
 use crate::filter::GuardFilter;
 use crate::guard::{Guard, NewlyConfirmed, Reachable};
 use crate::skew::SkewObservation;
-use crate::FirstHop;
 use crate::{
     ids::GuardId, ExternalActivity, GuardParams, GuardUsage, GuardUsageKind, PickGuardError,
 };
+use crate::{FirstHop, GuardSetSelector};
 use tor_basic_utils::iter::{FilterCount, IteratorExt as _};
 use tor_netdir::{NetDir, Relay};
 
@@ -43,7 +43,7 @@ use tracing::{debug, info};
 /// guards come first in preference order.  Then come the non-primary
 /// confirmed guards, in their confirmed order.  Finally come the
 /// non-primary, non-confirmed guards, in their sampled order.
-#[derive(Default, Debug, Clone, Deserialize)]
+#[derive(Debug, Default, Clone, Deserialize)]
 #[serde(from = "GuardSample")]
 pub(crate) struct GuardSet {
     /// Map from identities to guards, for every guard in this sample.
@@ -757,8 +757,15 @@ impl GuardSet {
     ///
     /// On success, returns the kind of guard that we got, and its filtered
     /// representation in a form suitable for use as a first hop.
+    ///
+    /// Label the returned guard as having come from `sample_id`.
+    //
+    // NOTE (nickm): I wish that we didn't have to take sample_id as an input,
+    // but the alternative would be storing it as a member of `GuardSet`, which
+    // makes things very complicated.
     pub(crate) fn pick_guard(
         &self,
+        sample_id: &GuardSetSelector,
         usage: &GuardUsage,
         params: &GuardParams,
         now: Instant,
@@ -767,7 +774,7 @@ impl GuardSet {
         let first_hop = self
             .get(&id)
             .expect("Somehow selected a guard we don't know!")
-            .get_external_rep();
+            .get_external_rep(sample_id.clone());
         let first_hop = self.active_filter.modify_hop(first_hop)?;
 
         Ok((list_kind, first_hop))
@@ -950,7 +957,7 @@ mod test {
 
             // make sure all the guards are okay.
             for (g, guard) in &guards.guards {
-                let id: FirstHopId = g.clone().into();
+                let id = FirstHopId::in_sample(GuardSetSelector::Default, g.clone());
                 let relay = id.get_relay(&netdir).unwrap();
                 assert!(relay.is_flagged_guard());
                 assert!(relay.is_dir_cache());

From 00f887db703b04fbe911d29bb91767a101062ef4 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 20 Oct 2022 09:49:00 -0400
Subject: [PATCH 2/9] linkspec: Add compare-by-relay-ids function to
 HasRelayIds

---
 crates/tor-linkspec/semver.md     |  2 +
 crates/tor-linkspec/src/ids.rs    |  6 ++-
 crates/tor-linkspec/src/traits.rs | 69 ++++++++++++++++++++++++++++++-
 3 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/crates/tor-linkspec/semver.md b/crates/tor-linkspec/semver.md
index 99b96fddf..a3986440e 100644
--- a/crates/tor-linkspec/semver.md
+++ b/crates/tor-linkspec/semver.md
@@ -2,4 +2,6 @@ MODIFIED: New ByRelayIds type.
 BREAKING: Changed the semantics of HasAddrs
 BREAKING: ChanTarget now requires HasChanMethod
 MODIFIED: RelayIdRef now implements Hash.
+MODIFIED: RelayId and RelayIdRef now implement Ord.
+MODIFIED: Added cmp_by_relay_ids() to HasRelayIds.
 
diff --git a/crates/tor-linkspec/src/ids.rs b/crates/tor-linkspec/src/ids.rs
index 549d05450..58b7db675 100644
--- a/crates/tor-linkspec/src/ids.rs
+++ b/crates/tor-linkspec/src/ids.rs
@@ -48,7 +48,7 @@ pub enum RelayIdType {
 }
 
 /// A single relay identity.
-#[derive(Debug, Clone, Copy, Eq, PartialEq, Display, From, Hash)]
+#[derive(Debug, Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Display, From, Hash)]
 #[non_exhaustive]
 pub enum RelayId {
     /// An Ed25519 identity.
@@ -60,7 +60,9 @@ pub enum RelayId {
 }
 
 /// A reference to a single relay identity.
-#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash, Display, From, derive_more::TryInto)]
+#[derive(
+    Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Display, From, derive_more::TryInto,
+)]
 #[non_exhaustive]
 pub enum RelayIdRef<'a> {
     /// An Ed25519 identity.
diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs
index 74d8caefa..843051a4b 100644
--- a/crates/tor-linkspec/src/traits.rs
+++ b/crates/tor-linkspec/src/traits.rs
@@ -91,6 +91,19 @@ pub trait HasRelayIds {
             }
         })
     }
+
+    /// Compare this object to another HasRelayIds, in a (somewhat) arbitrary
+    /// sorting order.
+    fn cmp_by_relay_ids<T: HasRelayIds + ?Sized>(&self, other: &T) -> std::cmp::Ordering {
+        use strum::IntoEnumIterator;
+        for key_type in RelayIdType::iter() {
+            let ordering = Ord::cmp(&self.identity(key_type), &other.identity(key_type));
+            if ordering.is_ne() {
+                return ordering;
+            }
+        }
+        std::cmp::Ordering::Equal
+    }
 }
 
 impl<T: HasRelayIdsLegacy> HasRelayIds for T {
@@ -211,7 +224,7 @@ mod test {
     use super::*;
     use hex_literal::hex;
     use std::net::IpAddr;
-    use tor_llcrypto::pk;
+    use tor_llcrypto::pk::{self, ed25519::Ed25519Identity, rsa::RsaIdentity};
 
     struct Example {
         addrs: Vec<SocketAddr>,
@@ -303,4 +316,58 @@ mod test {
             LinkSpec::OrPort("::1".parse::<IpAddr>().unwrap(), 909)
         );
     }
+
+    #[test]
+    fn cmp_by_ids() {
+        use crate::RelayIds;
+        use std::cmp::Ordering;
+        fn b(ed: Option<Ed25519Identity>, rsa: Option<RsaIdentity>) -> RelayIds {
+            let mut b = RelayIds::builder();
+            if let Some(ed) = ed {
+                b.ed_identity(ed);
+            }
+            if let Some(rsa) = rsa {
+                b.rsa_identity(rsa);
+            }
+            b.build().unwrap()
+        }
+        // Assert that v is strictly ascending.
+        fn assert_sorted(v: &[RelayIds]) {
+            for slice in v.windows(2) {
+                assert_eq!(slice[0].cmp_by_relay_ids(&slice[1]), Ordering::Less);
+                assert_eq!(slice[1].cmp_by_relay_ids(&slice[0]), Ordering::Greater);
+                assert_eq!(slice[0].cmp_by_relay_ids(&slice[0]), Ordering::Equal);
+            }
+        }
+
+        let ed1 = hex!("0a54686973206973207468652043656e7472616c205363727574696e697a6572").into();
+        let ed2 = hex!("6962696c69747920746f20656e666f72636520616c6c20746865206c6177730a").into();
+        let ed3 = hex!("73736564207965740a497420697320616c736f206d7920726573706f6e736962").into();
+        let rsa1 = hex!("2e2e2e0a4974206973206d7920726573706f6e73").into();
+        let rsa2 = hex!("5468617420686176656e2774206265656e207061").into();
+        let rsa3 = hex!("696c69747920746f20616c65727420656163680a").into();
+
+        assert_sorted(&[
+            b(Some(ed1), None),
+            b(Some(ed2), None),
+            b(Some(ed3), None),
+            b(Some(ed3), Some(rsa1)),
+        ]);
+        assert_sorted(&[
+            b(Some(ed1), Some(rsa3)),
+            b(Some(ed2), Some(rsa2)),
+            b(Some(ed3), Some(rsa1)),
+            b(Some(ed3), Some(rsa2)),
+        ]);
+        assert_sorted(&[
+            b(Some(ed1), Some(rsa1)),
+            b(Some(ed1), Some(rsa2)),
+            b(Some(ed1), Some(rsa3)),
+        ]);
+        assert_sorted(&[
+            b(None, Some(rsa1)),
+            b(None, Some(rsa2)),
+            b(None, Some(rsa3)),
+        ]);
+    }
 }

From fa1d2f453ade3c927ab71e14fc485396763b81f2 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 20 Oct 2022 09:53:24 -0400
Subject: [PATCH 3/9] 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<FallbackDir> 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<FallbackDir> 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<tor_linkspec::RelayIdRef<'_>> {
+        self.fallback.identity(key_type)
     }
 }
 
 impl From<FallbackList> for FallbackState {
     fn from(list: FallbackList) -> Self {
         let mut fallbacks: Vec<Entry> = 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 {
     //! <!-- @@ end test lint list maintained by maint/add_warning @@ -->
 
     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<usize> {
-            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<usize> {
+            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<tor_linkspec::RelayIdRef<'_>> {
+        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))
     }
 }

From bb117a4bd4ed264011619101881950371145e050 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 20 Oct 2022 10:46:00 -0400
Subject: [PATCH 4/9] guardmgr: Refactor the interior of FirstHop.

Now it contains either an `OwnedChanTarget` or an `OwnedCircTarget`,
which will let `GuardMgr` return bridges that can be used to make
circuits.

As part of this change, it was necessary to revise some
address-modification functions that applied to filters and
`OwnedChanTarget`.  Now they do the smart thing, and remove only the
address that are in the `ChanMethod`.  This means that the addresses
from HasAddrs are still accurate about which addresses the relay
"has".
---
 crates/tor-chanmgr/src/transport/default.rs |   6 +-
 crates/tor-guardmgr/src/fallback.rs         |   7 +-
 crates/tor-guardmgr/src/filter.rs           |  24 ++--
 crates/tor-guardmgr/src/guard.rs            |  10 +-
 crates/tor-guardmgr/src/lib.rs              | 115 ++++++++++++++------
 crates/tor-linkspec/semver.md               |   2 +
 crates/tor-linkspec/src/owned.rs            |  23 ++--
 crates/tor-linkspec/src/transport.rs        |  90 +++++++++++++--
 8 files changed, 202 insertions(+), 75 deletions(-)

diff --git a/crates/tor-chanmgr/src/transport/default.rs b/crates/tor-chanmgr/src/transport/default.rs
index 8d5b6a59c..b013261cd 100644
--- a/crates/tor-chanmgr/src/transport/default.rs
+++ b/crates/tor-chanmgr/src/transport/default.rs
@@ -51,10 +51,8 @@ impl<R: Runtime> crate::transport::TransportHelper for DefaultTransport<R> {
         };
 
         let (stream, addr) = connect_to_one(&self.runtime, &direct_addrs).await?;
-        let using_target = match target.restrict_addr(&addr) {
-            Ok(v) => v,
-            Err(v) => v,
-        };
+        let mut using_target = target.clone();
+        let _ignore = using_target.chan_method_mut().retain_addrs(|a| a == &addr);
 
         Ok((using_target, stream))
     }
diff --git a/crates/tor-guardmgr/src/fallback.rs b/crates/tor-guardmgr/src/fallback.rs
index 877923ed5..d6dba0279 100644
--- a/crates/tor-guardmgr/src/fallback.rs
+++ b/crates/tor-guardmgr/src/fallback.rs
@@ -12,11 +12,10 @@
 
 mod set;
 
-use crate::ids::FallbackId;
 use derive_builder::Builder;
 use tor_config::ConfigBuildError;
 use tor_config::{define_list_builder_accessors, impl_standard_builder, list_builder::VecBuilder};
-use tor_linkspec::DirectChanMethodsHelper;
+use tor_linkspec::{DirectChanMethodsHelper, OwnedChanTarget};
 use tor_llcrypto::pk::ed25519::Ed25519Identity;
 use tor_llcrypto::pk::rsa::RsaIdentity;
 
@@ -59,8 +58,8 @@ impl FallbackDir {
     /// Return a copy of this FallbackDir as a [`FirstHop`](crate::FirstHop)
     pub fn as_guard(&self) -> crate::FirstHop {
         crate::FirstHop {
-            id: FallbackId::from_relay_ids(self).into(),
-            orports: self.orports.clone(),
+            sample: None,
+            inner: crate::FirstHopInner::Chan(OwnedChanTarget::from_chan_target(self)),
         }
     }
 }
diff --git a/crates/tor-guardmgr/src/filter.rs b/crates/tor-guardmgr/src/filter.rs
index be60dc80d..e97fa234b 100644
--- a/crates/tor-guardmgr/src/filter.rs
+++ b/crates/tor-guardmgr/src/filter.rs
@@ -98,9 +98,17 @@ impl SingleFilter {
     /// Return true if this filter permits the provided target.
     fn permits<C: ChanTarget>(&self, target: &C) -> bool {
         match self {
-            SingleFilter::ReachableAddrs(patterns) => patterns
-                .iter()
-                .any(|pat| target.addrs().iter().any(|addr| pat.matches_sockaddr(addr))),
+            SingleFilter::ReachableAddrs(patterns) => {
+                patterns.iter().any(|pat| {
+                    match target.chan_method().socket_addrs() {
+                        // Check whether _any_ address actually used by this
+                        // method is permitted by _any_ pattern.
+                        Some(addrs) => addrs.iter().any(|addr| pat.matches_sockaddr(addr)),
+                        // This target doesn't use addresses: only hostnames or "None"
+                        None => true,
+                    }
+                })
+            }
         }
     }
 
@@ -115,10 +123,12 @@ impl SingleFilter {
     ) -> Result<crate::FirstHop, crate::PickGuardError> {
         match self {
             SingleFilter::ReachableAddrs(patterns) => {
-                first_hop
-                    .orports
-                    .retain(|addr| patterns.iter().any(|pat| pat.matches_sockaddr(addr)));
-                if first_hop.orports.is_empty() {
+                let r = first_hop
+                    .chan_target_mut()
+                    .chan_method_mut()
+                    .retain_addrs(|addr| patterns.iter().any(|pat| pat.matches_sockaddr(addr)));
+
+                if r.is_err() {
                     // TODO(nickm): The fact that this check needs to be checked
                     // happen indicates a likely problem in our code design.
                     // Right now, we have `modify_hop` and `permits` as separate
diff --git a/crates/tor-guardmgr/src/guard.rs b/crates/tor-guardmgr/src/guard.rs
index f6314c4a4..1420eb531 100644
--- a/crates/tor-guardmgr/src/guard.rs
+++ b/crates/tor-guardmgr/src/guard.rs
@@ -14,7 +14,7 @@ use crate::dirstatus::DirStatus;
 use crate::skew::SkewObservation;
 use crate::util::randomize_time;
 use crate::{ids::GuardId, GuardParams, GuardRestriction, GuardUsage};
-use crate::{ExternalActivity, FirstHopId, GuardSetSelector, GuardUsageKind};
+use crate::{ExternalActivity, GuardSetSelector, GuardUsageKind};
 use tor_linkspec::{HasAddrs, HasRelayIds};
 use tor_persist::{Futureproof, JsonValue};
 
@@ -665,8 +665,11 @@ impl Guard {
     /// Return a [`FirstHop`](crate::FirstHop) object to represent this guard.
     pub(crate) fn get_external_rep(&self, selection: GuardSetSelector) -> crate::FirstHop {
         crate::FirstHop {
-            id: FirstHopId::in_sample(selection, self.id.clone()),
-            orports: self.orports.clone(),
+            sample: Some(selection),
+            // TODO pt-client: might have a bridge descriptor.
+            inner: crate::FirstHopInner::Chan(tor_linkspec::OwnedChanTarget::from_chan_target(
+                self,
+            )),
         }
     }
 
@@ -792,6 +795,7 @@ impl CircHistory {
 mod test {
     #![allow(clippy::unwrap_used)]
     use super::*;
+    use crate::ids::FirstHopId;
     use tor_linkspec::{HasRelayIds, RelayId};
     use tor_llcrypto::pk::ed25519::Ed25519Identity;
 
diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs
index 75b793506..89b8224f8 100644
--- a/crates/tor-guardmgr/src/lib.rs
+++ b/crates/tor-guardmgr/src/lib.rs
@@ -50,7 +50,7 @@ use std::collections::HashMap;
 use std::net::SocketAddr;
 use std::sync::{Arc, Mutex, Weak};
 use std::time::{Duration, Instant, SystemTime};
-use tor_linkspec::{RelayId, RelayIdSet};
+use tor_linkspec::{OwnedChanTarget, OwnedCircTarget, RelayId, RelayIdSet};
 use tor_netdir::NetDirProvider;
 use tor_proto::ClockSkew;
 use tracing::{debug, info, trace, warn};
@@ -91,7 +91,7 @@ pub use skew::SkewEstimate;
 use pending::{PendingRequest, RequestId};
 use sample::GuardSet;
 
-use crate::ids::FirstHopIdInner;
+use crate::ids::{FirstHopIdInner, GuardId};
 
 /// A "guard manager" that selects and remembers a persistent set of
 /// guard nodes.
@@ -498,16 +498,23 @@ impl<R: Runtime> GuardMgr<R> {
                 false
             };
 
-        let pending_request =
-            pending::PendingRequest::new(guard.id.clone(), usage, usable_sender, net_has_been_down);
+        let pending_request = pending::PendingRequest::new(
+            guard.first_hop_id(),
+            usage,
+            usable_sender,
+            net_has_been_down,
+        );
         inner.pending.insert(request_id, pending_request);
 
-        match &guard.id.0 {
-            FirstHopIdInner::Guard(sample, id) => {
-                inner.guards.guards_mut(sample).record_attempt(id, now);
+        match &guard.sample {
+            Some(sample) => {
+                let guard_id = GuardId::from_relay_ids(&guard);
+                inner
+                    .guards
+                    .guards_mut(sample)
+                    .record_attempt(&guard_id, now);
             }
-
-            FirstHopIdInner::Fallback(_) => {
+            None => {
                 // We don't record attempts for fallbacks; we only care when
                 // they have failed.
             }
@@ -1268,42 +1275,78 @@ impl TryFrom<&NetParameters> for GuardParams {
 }
 
 /// Representation of a guard or fallback, as returned by [`GuardMgr::select_guard()`].
-#[derive(Debug, Clone, Eq, PartialEq)]
+#[derive(Debug, Clone)]
 pub struct FirstHop {
-    /// The guard's identities
-    id: FirstHopId,
-    /// The addresses at which the guard can be contacted.
-    //
-    // TODO pt-client: This needs to be more complex if we are using
-    // pluggable transports!
-    orports: Vec<SocketAddr>,
+    /// The sample from which this guard was taken, or `None` if this is a fallback.
+    sample: Option<GuardSetSelector>,
+    /// Information about connecting to (or through) this guard.
+    inner: FirstHopInner,
+}
+/// The enumeration inside a FirstHop that holds information about how to
+/// connect to (and possibly through) a guard or fallback.
+#[derive(Debug, Clone)]
+enum FirstHopInner {
+    /// We have enough information to connect to a guard.
+    Chan(OwnedChanTarget),
+    /// We have enough information to connect to a guards _and_ to build
+    /// multihop circuits through it.
+    #[allow(dead_code)] // TODO pt-client
+    Circ(OwnedCircTarget),
 }
 
 impl FirstHop {
-    /// Return the identities of this guard.
-    pub fn id(&self) -> &FirstHopId {
-        &self.id
+    /// Return a new [`FirstHopId`] for this `FirstHop`.
+    fn first_hop_id(&self) -> FirstHopId {
+        match &self.sample {
+            Some(sample) => {
+                let guard_id = GuardId::from_relay_ids(self);
+                FirstHopId::in_sample(sample.clone(), guard_id)
+            }
+            None => {
+                let fallback_id = crate::ids::FallbackId::from_relay_ids(self);
+                FirstHopId::from(fallback_id)
+            }
+        }
     }
+
     /// Look up this guard in `netdir`.
     pub fn get_relay<'a>(&self, netdir: &'a NetDir) -> Option<Relay<'a>> {
-        // TODO pt-client: This should always return "None" for a bridge.
-        self.id().get_relay(netdir)
+        match self.sample {
+            #[cfg(feature = "bridge-client")]
+            // Always return "None" for a bridge, since it isn't in a netdir.
+            Some(GuardSetSelector::Bridges) => None,
+            // Otherwise ask the netdir.
+            _ => netdir.by_ids(self),
+        }
     }
 
     /// If possible, return a view of this object that can be used to build a circuit.
     ///
     /// TODO pt-client: This will need to return "Some" only for bridges that have
     /// a bridge descriptor.
-    #[allow(clippy::missing_panics_doc)]
-    pub fn as_circ_target(&self) -> Option<tor_linkspec::OwnedCircTarget> {
-        todo!() // TODO pt-client: Implement
+    pub fn as_circ_target(&self) -> Option<&OwnedCircTarget> {
+        match &self.inner {
+            FirstHopInner::Chan(_) => None,
+            FirstHopInner::Circ(ct) => Some(ct),
+        }
+    }
+
+    /// Return a view of this as an OwnedChanTarget.
+    fn chan_target_mut(&mut self) -> &mut OwnedChanTarget {
+        match &mut self.inner {
+            FirstHopInner::Chan(ct) => ct,
+            FirstHopInner::Circ(ct) => ct.chan_target_mut(),
+        }
     }
 }
 
 // This is somewhat redundant with the implementations in crate::guard::Guard.
 impl tor_linkspec::HasAddrs for FirstHop {
     fn addrs(&self) -> &[SocketAddr] {
-        &self.orports[..]
+        match &self.inner {
+            FirstHopInner::Chan(ct) => ct.addrs(),
+            FirstHopInner::Circ(ct) => ct.addrs(),
+        }
     }
 }
 impl tor_linkspec::HasRelayIds for FirstHop {
@@ -1311,10 +1354,20 @@ impl tor_linkspec::HasRelayIds for FirstHop {
         &self,
         key_type: tor_linkspec::RelayIdType,
     ) -> Option<tor_linkspec::RelayIdRef<'_>> {
-        self.id.identity(key_type)
+        match &self.inner {
+            FirstHopInner::Chan(ct) => ct.identity(key_type),
+            FirstHopInner::Circ(ct) => ct.identity(key_type),
+        }
+    }
+}
+impl tor_linkspec::HasChanMethod for FirstHop {
+    fn chan_method(&self) -> tor_linkspec::ChannelMethod {
+        match &self.inner {
+            FirstHopInner::Chan(ct) => ct.chan_method(),
+            FirstHopInner::Circ(ct) => ct.chan_method(),
+        }
     }
 }
-impl tor_linkspec::DirectChanMethodsHelper for FirstHop {}
 impl tor_linkspec::ChanTarget for FirstHop {}
 
 /// The purpose for which we plan to use a guard.
@@ -1470,7 +1523,7 @@ mod test {
             // Since the guard was confirmed, we should get the same one this time!
             let usage = GuardUsage::default();
             let (id2, _mon, _usable) = guardmgr2.select_guard(usage, Some(&netdir)).unwrap();
-            assert_eq!(id2, id);
+            assert!(id2.same_relay_ids(&id));
         });
     }
 
@@ -1497,12 +1550,12 @@ mod test {
             guardmgr.flush_msg_queue().await; // avoid race
             guardmgr.flush_msg_queue().await; // avoid race
 
-            assert!(id1 != id2);
+            assert!(!id1.same_relay_ids(&id2));
 
             // Now we should get two sampled guards. They should be different.
             let (id3, mon3, usable3) = guardmgr.select_guard(u.clone(), Some(&netdir)).unwrap();
             let (id4, mon4, usable4) = guardmgr.select_guard(u.clone(), Some(&netdir)).unwrap();
-            assert!(id3 != id4);
+            assert!(!id3.same_relay_ids(&id4));
 
             let (u3, u4) = futures::join!(
                 async {
diff --git a/crates/tor-linkspec/semver.md b/crates/tor-linkspec/semver.md
index a3986440e..7b432b193 100644
--- a/crates/tor-linkspec/semver.md
+++ b/crates/tor-linkspec/semver.md
@@ -4,4 +4,6 @@ BREAKING: ChanTarget now requires HasChanMethod
 MODIFIED: RelayIdRef now implements Hash.
 MODIFIED: RelayId and RelayIdRef now implement Ord.
 MODIFIED: Added cmp_by_relay_ids() to HasRelayIds.
+BREAKING: Replaced functions to access addresses from ChanMethod.
+BREAKING: Replaced functions to strip addresses from ChanMethod.
 
diff --git a/crates/tor-linkspec/src/owned.rs b/crates/tor-linkspec/src/owned.rs
index e0f552c4c..d65e42daa 100644
--- a/crates/tor-linkspec/src/owned.rs
+++ b/crates/tor-linkspec/src/owned.rs
@@ -149,22 +149,10 @@ impl OwnedChanTarget {
         }
     }
 
-    /// Construct a new OwnedChanTarget containing _only_ the provided `addr`.
+    /// Return a mutable reference to this [`OwnedChanTarget`]'s [`ChannelMethod`]
     ///
-    /// If `addr` is not an address of this `ChanTarget`, return the original OwnedChanTarget.
-    //
-    // TODO pt-client: this method no longer makes any sense, and needs to get replaced with one
-    // that works by ChanMethod.
-    pub fn restrict_addr(&self, addr: &SocketAddr) -> Result<Self, Self> {
-        if self.addrs.contains(addr) {
-            Ok(OwnedChanTarget {
-                addrs: vec![*addr],
-                method: self.method.clone(),
-                ids: self.ids.clone(),
-            })
-        } else {
-            Err(self.clone())
-        }
+    pub fn chan_method_mut(&mut self) -> &mut ChannelMethod {
+        &mut self.method
     }
 }
 
@@ -214,6 +202,11 @@ impl OwnedCircTarget {
             protocols: target.protovers().clone(),
         }
     }
+
+    /// Return a mutable view of this OwnedCircTarget as an [`OwnedChanTarget`].
+    pub fn chan_target_mut(&mut self) -> &mut OwnedChanTarget {
+        &mut self.chan_target
+    }
 }
 
 /// Primarily for error reporting and logging
diff --git a/crates/tor-linkspec/src/transport.rs b/crates/tor-linkspec/src/transport.rs
index 120509a19..b459f9abe 100644
--- a/crates/tor-linkspec/src/transport.rs
+++ b/crates/tor-linkspec/src/transport.rs
@@ -346,6 +346,27 @@ impl PtTarget {
     pub fn settings(&self) -> impl Iterator<Item = (&str, &str)> {
         self.settings.settings.iter().map(|(k, v)| (&**k, &**v))
     }
+
+    /// Return all the advertized socket addresses to which this target may
+    /// connect.
+    ///
+    /// Returns `Some(&[])` if there is no way to connect to this target, and
+    /// `None` if this target does not use `SocketAddr` to connect
+    ///
+    /// NOTE that these are not necessarily an address to which you can open a
+    /// TCP connection! The address will be interpreted by the implementation of
+    /// this pluggable transport.
+
+    pub fn socket_addrs(&self) -> Option<&[std::net::SocketAddr]> {
+        match self {
+            PtTarget {
+                addr: PtTargetAddr::IpPort(addr),
+                ..
+            } => Some(std::slice::from_ref(addr)),
+
+            _ => None,
+        }
+    }
 }
 
 /// The way to approach a single relay in order to open a channel.
@@ -369,27 +390,29 @@ pub enum ChannelMethod {
 }
 
 impl ChannelMethod {
-    /// Return an advertised socket address that this method connects to.
+    /// Return all the advertized socket addresses to which this method may connect.
     ///
-    /// NOTE that this is not necessarily an address to which you can open a
+    /// Returns `Some(&[])` if there is no way to connect to this target, and
+    /// `None` if this target does not use `SocketAddr` to connect
+    ///
+    /// NOTE that these are not necessarily an address to which you can open a
     /// TCP connection! If this `ChannelMethod` is using a non-`Direct`
     /// transport, then this address will be interpreted by that transport's
     /// implementation.
-    pub fn declared_peer_addr(&self) -> Option<&std::net::SocketAddr> {
+    pub fn socket_addrs(&self) -> Option<&[std::net::SocketAddr]> {
         match self {
-            ChannelMethod::Direct(addr) if !addr.is_empty() => Some(&addr[0]),
+            ChannelMethod::Direct(addr) => Some(addr.as_ref()),
 
             #[cfg(feature = "pt-client")]
-            ChannelMethod::Pluggable(PtTarget {
-                addr: PtTargetAddr::IpPort(addr),
-                ..
-            }) => Some(addr),
-
-            #[cfg_attr(not(feature = "pt-client"), allow(unreachable_patterns))]
-            _ => None,
+            ChannelMethod::Pluggable(t) => t.socket_addrs(),
         }
     }
 
+    /// TODO nickm: REMOVE this.
+    pub fn declared_peer_addr(&self) -> Option<&std::net::SocketAddr> {
+        self.socket_addrs().and_then(|s| s.get(0))
+    }
+
     /// Return a PtTargetAddr that this ChannelMethod uses.
     pub fn target_addr(&self) -> Option<PtTargetAddr> {
         match self {
@@ -415,6 +438,51 @@ impl ChannelMethod {
             ChannelMethod::Pluggable(target) => target.transport().clone().into(),
         }
     }
+
+    ///
+    /// Change this `ChannelMethod` by removing every socket address that
+    /// does not satisfy `pred`.
+    ///
+    /// `Hostname` and `None` addresses are never removed.
+    ///
+    /// Return an error if we have removed every address.
+    pub fn retain_addrs<P>(&mut self, pred: P) -> Result<(), RetainAddrsError>
+    where
+        P: Fn(&std::net::SocketAddr) -> bool,
+    {
+        #[cfg(feature = "pt-client")]
+        use PtTargetAddr as Pt;
+
+        match self {
+            ChannelMethod::Direct(d) if d.is_empty() => {}
+            ChannelMethod::Direct(d) => {
+                d.retain(pred);
+                if d.is_empty() {
+                    return Err(RetainAddrsError::NoAddrsLeft);
+                }
+            }
+            #[cfg(feature = "pt-client")]
+            ChannelMethod::Pluggable(PtTarget { addr, .. }) => match addr {
+                Pt::IpPort(a) => {
+                    if !pred(a) {
+                        *addr = Pt::None;
+                        return Err(RetainAddrsError::NoAddrsLeft);
+                    }
+                }
+                Pt::HostPort(_, _) => {}
+                Pt::None => {}
+            },
+        }
+        Ok(())
+    }
+}
+
+/// An error that occurred while filtering addresses from a ChanMethod.
+#[derive(Clone, Debug, thiserror::Error)]
+pub enum RetainAddrsError {
+    /// We removed all of the addresses from this method.
+    #[error("All addresses were removed.")]
+    NoAddrsLeft,
 }
 
 impl HasAddrs for PtTargetAddr {

From 1195c40a99502c2463ac622acbae00a4c6ae1eeb Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 20 Oct 2022 11:16:13 -0400
Subject: [PATCH 5/9] linkspec: Remove now-useless declared_peer_addr

The singleton variation here is almost never what we want.
---
 crates/tor-linkspec/src/transport.rs      | 5 -----
 crates/tor-proto/src/channel/handshake.rs | 7 ++++---
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/crates/tor-linkspec/src/transport.rs b/crates/tor-linkspec/src/transport.rs
index b459f9abe..bfaa10787 100644
--- a/crates/tor-linkspec/src/transport.rs
+++ b/crates/tor-linkspec/src/transport.rs
@@ -408,11 +408,6 @@ impl ChannelMethod {
         }
     }
 
-    /// TODO nickm: REMOVE this.
-    pub fn declared_peer_addr(&self) -> Option<&std::net::SocketAddr> {
-        self.socket_addrs().and_then(|s| s.get(0))
-    }
-
     /// Return a PtTargetAddr that this ChannelMethod uses.
     pub fn target_addr(&self) -> Option<PtTargetAddr> {
         match self {
diff --git a/crates/tor-proto/src/channel/handshake.rs b/crates/tor-proto/src/channel/handshake.rs
index 3a1a7e626..804abc285 100644
--- a/crates/tor-proto/src/channel/handshake.rs
+++ b/crates/tor-proto/src/channel/handshake.rs
@@ -575,7 +575,8 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static, S: SleepProvider> Verif
         let peer_ip = self
             .target_method
             .as_ref()
-            .and_then(ChannelMethod::declared_peer_addr)
+            .and_then(ChannelMethod::socket_addrs)
+            .and_then(|addrs| addrs.get(0))
             .map(SocketAddr::ip);
         let netinfo = msg::Netinfo::for_client(peer_ip);
         self.tls
@@ -592,8 +593,8 @@ impl<T: AsyncRead + AsyncWrite + Send + Unpin + 'static, S: SleepProvider> Verif
 
         let mut peer_builder = OwnedChanTargetBuilder::default();
         if let Some(target_method) = self.target_method {
-            if let Some(addr) = target_method.declared_peer_addr() {
-                peer_builder.addrs(vec![*addr]);
+            if let Some(addrs) = target_method.socket_addrs() {
+                peer_builder.addrs(addrs.to_owned());
             }
             peer_builder.method(target_method);
         }

From 47cd5c97da01b2a600c82b6ab2947bf5f2108d94 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 20 Oct 2022 14:38:11 -0400
Subject: [PATCH 6/9] netdir: Expose addrs-in-same-subnets calculation from
 SubnetConfig

Previously this was a private method only visible from Relay, but now
we can use it on any two HasAddrs objects.
---
 crates/tor-netdir/semver.md  |  2 ++
 crates/tor-netdir/src/lib.rs | 26 ++++++++++++++++++--------
 2 files changed, 20 insertions(+), 8 deletions(-)
 create mode 100644 crates/tor-netdir/semver.md

diff --git a/crates/tor-netdir/semver.md b/crates/tor-netdir/semver.md
new file mode 100644
index 000000000..c42ce1fe9
--- /dev/null
+++ b/crates/tor-netdir/semver.md
@@ -0,0 +1,2 @@
+MODIFIED: Exposed addrs_in_same_subnets() api from SubnetConfig
+
diff --git a/crates/tor-netdir/src/lib.rs b/crates/tor-netdir/src/lib.rs
index eb446c455..7645f9232 100644
--- a/crates/tor-netdir/src/lib.rs
+++ b/crates/tor-netdir/src/lib.rs
@@ -107,8 +107,9 @@ impl SubnetConfig {
         }
     }
 
-    /// Are two addresses in the same subnet according to this configuration
-    fn addrs_in_same_subnet(&self, a: &IpAddr, b: &IpAddr) -> bool {
+    /// Return true if the two addresses in the same subnet, according to this
+    /// configuration.
+    pub fn addrs_in_same_subnet(&self, a: &IpAddr, b: &IpAddr) -> bool {
         match (a, b) {
             (IpAddr::V4(a), IpAddr::V4(b)) => {
                 let bits = self.subnets_family_v4;
@@ -131,6 +132,20 @@ impl SubnetConfig {
             _ => false,
         }
     }
+
+    /// Return true if any of the addresses in `a` shares a subnet with any of
+    /// the addresses in `b`, according to this configuration.
+    pub fn any_addrs_in_same_subnet<T, U>(&self, a: &T, b: &U) -> bool
+    where
+        T: tor_linkspec::HasAddrs,
+        U: tor_linkspec::HasAddrs,
+    {
+        a.addrs().iter().any(|aa| {
+            b.addrs()
+                .iter()
+                .any(|bb| self.addrs_in_same_subnet(&aa.ip(), &bb.ip()))
+        })
+    }
 }
 
 /// An opaque type representing the weight with which a relay or set of
@@ -1061,12 +1076,7 @@ impl<'a> Relay<'a> {
     /// prefix, or if they have IPv6 addresses with the same
     /// `subnets_family_v6`-bit prefix.
     pub fn in_same_subnet<'b>(&self, other: &Relay<'b>, subnet_config: &SubnetConfig) -> bool {
-        self.rs.orport_addrs().any(|addr| {
-            other
-                .rs
-                .orport_addrs()
-                .any(|other| subnet_config.addrs_in_same_subnet(&addr.ip(), &other.ip()))
-        })
+        subnet_config.any_addrs_in_same_subnet(self, other)
     }
     /// Return true if both relays are in the same family.
     ///

From 103c88dd9569bcad4d4d8e79225eccc6e2ef91cb Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 20 Oct 2022 11:37:48 -0400
Subject: [PATCH 7/9] Circmgr: construct paths using either Relay or
 OwnedCircTarget

Previously we could only use Relay for this case, which won't work
any more: a Bridge is not a `tor_netdir::Relay`.  Instead we allow
the GuardMgr to give us something that knows how to convert itself
into an OwnedCircTarget.

This change required a far amount of follow-on revisions and
refactoring, but it should all be internal to the path-building
logic.
---
 crates/tor-circmgr/src/path.rs          |  87 +++++++++++++++++---
 crates/tor-circmgr/src/path/exitpath.rs | 104 ++++++++++++++++++------
 2 files changed, 157 insertions(+), 34 deletions(-)

diff --git a/crates/tor-circmgr/src/path.rs b/crates/tor-circmgr/src/path.rs
index 46df65f3f..bd60fee28 100644
--- a/crates/tor-circmgr/src/path.rs
+++ b/crates/tor-circmgr/src/path.rs
@@ -8,7 +8,7 @@ pub mod exitpath;
 
 use tor_error::bad_api_usage;
 use tor_guardmgr::fallback::FallbackDir;
-use tor_linkspec::{OwnedChanTarget, OwnedCircTarget};
+use tor_linkspec::{HasAddrs, HasRelayIds, OwnedChanTarget, OwnedCircTarget};
 use tor_netdir::Relay;
 
 use crate::usage::ExitPolicy;
@@ -33,7 +33,63 @@ enum TorPathInner<'a> {
     /// A single-hop path taken from an OwnedChanTarget.
     OwnedOneHop(OwnedChanTarget),
     /// A multi-hop path, containing one or more relays.
-    Path(Vec<Relay<'a>>),
+    Path(Vec<MaybeOwnedRelay<'a>>),
+}
+
+/// Identifier for a a relay that could be either known from a NetDir, or
+/// specified as an OwnedCircTarget.
+#[derive(Clone)]
+enum MaybeOwnedRelay<'a> {
+    /// A relay from the netdir.
+    Relay(Relay<'a>),
+    /// An owned description of a relay.
+    //
+    // TODO: I don't love boxing this, but it fixes a warning about variant
+    // sizes and is probably not the worst thing we could do.  OTOH, we could
+    // probably afford to use an Arc here and in guardmgr?
+    //
+    // TODO pt-client: Try using an Arc.
+    Owned(Box<OwnedCircTarget>),
+}
+
+impl<'a> MaybeOwnedRelay<'a> {
+    /// Extract an OwnedCircTarget from this relay.
+    fn to_owned(&self) -> OwnedCircTarget {
+        match self {
+            MaybeOwnedRelay::Relay(r) => OwnedCircTarget::from_circ_target(r),
+            MaybeOwnedRelay::Owned(o) => o.as_ref().clone(),
+        }
+    }
+}
+
+impl<'a> From<OwnedCircTarget> for MaybeOwnedRelay<'a> {
+    fn from(ct: OwnedCircTarget) -> Self {
+        MaybeOwnedRelay::Owned(Box::new(ct))
+    }
+}
+impl<'a> From<Relay<'a>> for MaybeOwnedRelay<'a> {
+    fn from(r: Relay<'a>) -> Self {
+        MaybeOwnedRelay::Relay(r)
+    }
+}
+impl<'a> HasAddrs for MaybeOwnedRelay<'a> {
+    fn addrs(&self) -> &[std::net::SocketAddr] {
+        match self {
+            MaybeOwnedRelay::Relay(r) => r.addrs(),
+            MaybeOwnedRelay::Owned(r) => r.addrs(),
+        }
+    }
+}
+impl<'a> HasRelayIds for MaybeOwnedRelay<'a> {
+    fn identity(
+        &self,
+        key_type: tor_linkspec::RelayIdType,
+    ) -> Option<tor_linkspec::RelayIdRef<'_>> {
+        match self {
+            MaybeOwnedRelay::Relay(r) => r.identity(key_type),
+            MaybeOwnedRelay::Owned(r) => r.identity(key_type),
+        }
+    }
 }
 
 impl<'a> TorPath<'a> {
@@ -62,25 +118,37 @@ impl<'a> TorPath<'a> {
     }
 
     /// Create a new multi-hop path with a given number of ordered relays.
-    pub fn new_multihop(relays: impl IntoIterator<Item = Relay<'a>>) -> Self {
+    pub fn new_multihop<H>(relays: impl IntoIterator<Item = Relay<'a>>) -> Self {
         Self {
-            inner: TorPathInner::Path(relays.into_iter().collect()),
+            inner: TorPathInner::Path(relays.into_iter().map(MaybeOwnedRelay::from).collect()),
+        }
+    }
+    /// Construct a new multi-hop path from a vector of `MaybeOwned`.
+    ///
+    /// Internal only; do not expose without fixing up this API a bit.
+    fn new_multihop_from_maybe_owned(relays: Vec<MaybeOwnedRelay<'a>>) -> Self {
+        Self {
+            inner: TorPathInner::Path(relays),
         }
     }
 
     /// Return the final relay in this path, if this is a path for use
     /// with exit circuits.
-    fn exit_relay(&self) -> Option<&Relay<'a>> {
+    fn exit_relay(&self) -> Option<&MaybeOwnedRelay<'a>> {
         match &self.inner {
             TorPathInner::Path(relays) if !relays.is_empty() => Some(&relays[relays.len() - 1]),
             _ => None,
         }
     }
 
-    /// Return the exit policy of the final relay in this path, if this
-    /// is a path for use with exit circuits.
+    /// Return the exit policy of the final relay in this path, if this is a
+    /// path for use with exit circuits with an exit taken from the network
+    /// directory.
     pub(crate) fn exit_policy(&self) -> Option<ExitPolicy> {
-        self.exit_relay().map(ExitPolicy::from_relay)
+        self.exit_relay().and_then(|r| match r {
+            MaybeOwnedRelay::Relay(r) => Some(ExitPolicy::from_relay(r)),
+            MaybeOwnedRelay::Owned(_) => None,
+        })
     }
 
     /// Return the number of relays in this path.
@@ -115,7 +183,7 @@ impl<'a> TryFrom<&TorPath<'a>> for OwnedPath {
             OneHop(h) => OwnedPath::Normal(vec![OwnedCircTarget::from_circ_target(h)]),
             OwnedOneHop(owned) => OwnedPath::ChannelOnly(owned.clone()),
             Path(p) if !p.is_empty() => {
-                OwnedPath::Normal(p.iter().map(OwnedCircTarget::from_circ_target).collect())
+                OwnedPath::Normal(p.iter().map(MaybeOwnedRelay::to_owned).collect())
             }
             Path(_) => {
                 return Err(bad_api_usage!("Path with no entries!").into());
@@ -140,7 +208,6 @@ impl OwnedPath {
 #[cfg(test)]
 fn assert_same_path_when_owned(path: &TorPath<'_>) {
     #![allow(clippy::unwrap_used)]
-    use tor_linkspec::HasRelayIds;
     let owned: OwnedPath = path.try_into().unwrap();
 
     match (&owned, &path.inner) {
diff --git a/crates/tor-circmgr/src/path/exitpath.rs b/crates/tor-circmgr/src/path/exitpath.rs
index a34f6961b..31210589d 100644
--- a/crates/tor-circmgr/src/path/exitpath.rs
+++ b/crates/tor-circmgr/src/path/exitpath.rs
@@ -1,6 +1,6 @@
 //! Code for building paths to an exit relay.
 
-use super::TorPath;
+use super::{MaybeOwnedRelay, TorPath};
 use crate::{DirInfo, Error, PathConfig, Result, TargetPort};
 use rand::Rng;
 use std::time::SystemTime;
@@ -77,7 +77,7 @@ impl<'a> ExitPathBuilder<'a> {
         &self,
         rng: &mut R,
         netdir: &'a NetDir,
-        guard: Option<&Relay<'a>>,
+        guard: Option<&MaybeOwnedRelay<'a>>,
         config: SubnetConfig,
     ) -> Result<Relay<'a>> {
         let mut can_share = FilterCount::default();
@@ -182,13 +182,21 @@ impl<'a> ExitPathBuilder<'a> {
                 }
                 let guard_usage = b.build().expect("Failed while building guard usage!");
                 let (guard, mut mon, usable) = guardmgr.select_guard(guard_usage, Some(netdir))?;
-                // TODO pt-client: First try as_circ_target; then try get_relay.
-                let guard = guard.get_relay(netdir).ok_or_else(|| {
-                    internal!(
-                        "Somehow the guardmgr gave us an unlisted guard {:?}!",
-                        guard
-                    )
-                })?;
+                let guard = if let Some(ct) = guard.as_circ_target() {
+                    // This is a bridge; we will not look for it in the network directory.
+                    MaybeOwnedRelay::from(ct.clone())
+                } else {
+                    // Look this up in the network directory: we expect to find a relay.
+                    guard
+                        .get_relay(netdir)
+                        .ok_or_else(|| {
+                            internal!(
+                                "Somehow the guardmgr gave us an unlisted guard {:?}!",
+                                guard
+                            )
+                        })?
+                        .into()
+                };
                 if !path_is_fully_random {
                     // We were given a specific exit relay to use, and
                     // the choice of exit relay might be forced by
@@ -203,21 +211,27 @@ impl<'a> ExitPathBuilder<'a> {
             None => {
                 let mut can_share = FilterCount::default();
                 let mut correct_usage = FilterCount::default();
+                let chosen_exit = chosen_exit.map(|relay| MaybeOwnedRelay::from(relay.clone()));
                 let entry = netdir
                     .pick_relay(rng, WeightRole::Guard, |r| {
-                        can_share.count(relays_can_share_circuit_opt(r, chosen_exit, subnet_config))
-                            && correct_usage.count(r.is_flagged_guard())
+                        can_share.count(relays_can_share_circuit_opt(
+                            r,
+                            chosen_exit.as_ref(),
+                            subnet_config,
+                        )) && correct_usage.count(r.is_flagged_guard())
                     })
                     .ok_or(Error::NoPath {
                         role: "entry relay",
                         can_share,
                         correct_usage,
                     })?;
-                (entry, None, None)
+                (MaybeOwnedRelay::from(entry), None, None)
             }
         };
 
-        let exit = self.pick_exit(rng, netdir, Some(&guard), subnet_config)?;
+        let exit: MaybeOwnedRelay = self
+            .pick_exit(rng, netdir, Some(&guard), subnet_config)?
+            .into();
 
         let mut can_share = FilterCount::default();
         let mut correct_usage = FilterCount::default();
@@ -235,7 +249,11 @@ impl<'a> ExitPathBuilder<'a> {
             })?;
 
         Ok((
-            TorPath::new_multihop(vec![guard, middle, exit]),
+            TorPath::new_multihop_from_maybe_owned(vec![
+                guard,
+                MaybeOwnedRelay::from(middle),
+                exit,
+            ]),
             mon,
             usable,
         ))
@@ -243,12 +261,29 @@ impl<'a> ExitPathBuilder<'a> {
 }
 
 /// Returns true if both relays can appear together in the same circuit.
-fn relays_can_share_circuit(a: &Relay<'_>, b: &Relay<'_>, subnet_config: SubnetConfig) -> bool {
-    !a.in_same_family(b) && !a.in_same_subnet(b, &subnet_config)
+fn relays_can_share_circuit(
+    a: &Relay<'_>,
+    b: &MaybeOwnedRelay<'_>,
+    subnet_config: SubnetConfig,
+) -> bool {
+    if let MaybeOwnedRelay::Relay(r) = b {
+        if a.in_same_family(r) {
+            return false;
+        };
+        // TODO: When bridge families are finally implemented (likely via
+        // proposal `321-happy-families.md`), we should move family
+        // functionality into CircTarget.
+    }
+
+    !subnet_config.any_addrs_in_same_subnet(a, b)
 }
 
 /// Helper: wraps relays_can_share_circuit but takes an option.
-fn relays_can_share_circuit_opt(r1: &Relay<'_>, r2: Option<&Relay<'_>>, c: SubnetConfig) -> bool {
+fn relays_can_share_circuit_opt(
+    r1: &Relay<'_>,
+    r2: Option<&MaybeOwnedRelay<'_>>,
+    c: SubnetConfig,
+) -> bool {
     match r2 {
         Some(r2) => relays_can_share_circuit(r1, r2, c),
         None => true,
@@ -260,7 +295,7 @@ mod test {
     #![allow(clippy::unwrap_used)]
     #![allow(clippy::clone_on_copy)]
     use super::*;
-    use crate::path::{assert_same_path_when_owned, OwnedPath, TorPathInner};
+    use crate::path::{assert_same_path_when_owned, MaybeOwnedRelay, OwnedPath, TorPathInner};
     use crate::test::OptDummyGuardMgr;
     use std::collections::HashSet;
     use tor_basic_utils::test_rng::testing_rng;
@@ -269,7 +304,22 @@ mod test {
     use tor_netdir::testnet;
     use tor_rtcompat::SleepProvider;
 
-    fn assert_exit_path_ok(relays: &[Relay<'_>]) {
+    impl<'a> MaybeOwnedRelay<'a> {
+        fn can_share_circuit(
+            &self,
+            other: &MaybeOwnedRelay<'_>,
+            subnet_config: SubnetConfig,
+        ) -> bool {
+            match self {
+                MaybeOwnedRelay::Relay(r) => relays_can_share_circuit(r, other, subnet_config),
+                MaybeOwnedRelay::Owned(r) => {
+                    !subnet_config.any_addrs_in_same_subnet(r.as_ref(), other)
+                }
+            }
+        }
+    }
+
+    fn assert_exit_path_ok(relays: &[MaybeOwnedRelay<'_>]) {
         assert_eq!(relays.len(), 3);
 
         // TODO: Eventually assert that r1 has Guard, once we enforce that.
@@ -283,9 +333,9 @@ mod test {
         assert!(!r2.same_relay_ids(r3));
 
         let subnet_config = SubnetConfig::default();
-        assert!(relays_can_share_circuit(r1, r2, subnet_config));
-        assert!(relays_can_share_circuit(r1, r3, subnet_config));
-        assert!(relays_can_share_circuit(r2, r3, subnet_config));
+        assert!(r1.can_share_circuit(r2, subnet_config));
+        assert!(r2.can_share_circuit(r3, subnet_config));
+        assert!(r1.can_share_circuit(r3, subnet_config));
     }
 
     #[test]
@@ -307,7 +357,10 @@ mod test {
 
             if let TorPathInner::Path(p) = path.inner {
                 assert_exit_path_ok(&p[..]);
-                let exit = &p[2];
+                let exit = match &p[2] {
+                    MaybeOwnedRelay::Relay(r) => r,
+                    MaybeOwnedRelay::Owned(_) => panic!("Didn't asked for an owned target!"),
+                };
                 assert!(exit.ipv4_policy().allows_port(1119));
             } else {
                 panic!("Generated the wrong kind of path");
@@ -348,7 +401,10 @@ mod test {
             assert_same_path_when_owned(&path);
             if let TorPathInner::Path(p) = path.inner {
                 assert_exit_path_ok(&p[..]);
-                let exit = &p[2];
+                let exit = match &p[2] {
+                    MaybeOwnedRelay::Relay(r) => r,
+                    MaybeOwnedRelay::Owned(_) => panic!("Didn't asked for an owned target!"),
+                };
                 assert!(exit.policies_allow_some_port());
             } else {
                 panic!("Generated the wrong kind of path");

From d7672a13d52cd15bcac92b5322531934567f6010 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Sun, 23 Oct 2022 16:09:54 -0400
Subject: [PATCH 8/9] linkspec: specify sort order for HasRelayIds.

---
 crates/tor-linkspec/src/traits.rs | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/crates/tor-linkspec/src/traits.rs b/crates/tor-linkspec/src/traits.rs
index 843051a4b..a3eff6511 100644
--- a/crates/tor-linkspec/src/traits.rs
+++ b/crates/tor-linkspec/src/traits.rs
@@ -92,8 +92,14 @@ pub trait HasRelayIds {
         })
     }
 
-    /// Compare this object to another HasRelayIds, in a (somewhat) arbitrary
-    /// sorting order.
+    /// Compare this object to another HasRelayIds.
+    ///
+    /// Objects are sorted by Ed25519 identities, with ties decided by RSA
+    /// identities. An absent identity of a given type is sorted before a
+    /// present identity of that type.
+    ///
+    /// If additional identities are added in the future, they may taken into
+    /// consideration before _or_ after the current identity types.
     fn cmp_by_relay_ids<T: HasRelayIds + ?Sized>(&self, other: &T) -> std::cmp::Ordering {
         use strum::IntoEnumIterator;
         for key_type in RelayIdType::iter() {
@@ -369,5 +375,10 @@ mod test {
             b(None, Some(rsa2)),
             b(None, Some(rsa3)),
         ]);
+        assert_sorted(&[
+            b(None, Some(rsa1)),
+            b(Some(ed1), None),
+            b(Some(ed1), Some(rsa1)),
+        ]);
     }
 }

From c4263543d40f1788180a12bc15b608cbd136ecff Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Sun, 23 Oct 2022 16:16:29 -0400
Subject: [PATCH 9/9] circmgr: More NOTEs and TODO pt-clients.

---
 crates/tor-circmgr/src/path.rs | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/crates/tor-circmgr/src/path.rs b/crates/tor-circmgr/src/path.rs
index bd60fee28..ed9fc0cbd 100644
--- a/crates/tor-circmgr/src/path.rs
+++ b/crates/tor-circmgr/src/path.rs
@@ -23,6 +23,9 @@ pub struct TorPath<'a> {
 /// Non-public helper type to represent the different kinds of Tor path.
 ///
 /// (This is a separate type to avoid exposing its details to the user.)
+///
+/// NOTE: This type should NEVER be visible outside of path.rs and its
+/// sub-modules.
 enum TorPathInner<'a> {
     /// A single-hop path for use with a directory cache, when a relay is
     /// known.
@@ -38,15 +41,18 @@ enum TorPathInner<'a> {
 
 /// Identifier for a a relay that could be either known from a NetDir, or
 /// specified as an OwnedCircTarget.
+///
+/// NOTE: This type should NEVER be visible outside of path.rs and its
+/// sub-modules.
 #[derive(Clone)]
 enum MaybeOwnedRelay<'a> {
     /// A relay from the netdir.
     Relay(Relay<'a>),
     /// An owned description of a relay.
     //
-    // TODO: I don't love boxing this, but it fixes a warning about variant
-    // sizes and is probably not the worst thing we could do.  OTOH, we could
-    // probably afford to use an Arc here and in guardmgr?
+    // TODO pt-client: I don't love boxing this, but it fixes a warning about
+    // variant sizes and is probably not the worst thing we could do.  OTOH, we
+    // could probably afford to use an Arc here and in guardmgr?
     //
     // TODO pt-client: Try using an Arc.
     Owned(Box<OwnedCircTarget>),