Introduce a RelayIdSet and use it in place of HashSet<RelayId>.

This makes lookups a little more efficient.  I do with that HashSet
just supported this use-case, but for now this seems our best option.

crates/tor-circmgr/src/path/exitpath.rs

@@ -7,7 +7,7 @@ use std::time::SystemTime;
 use tor_basic_utils::iter::FilterCount;
 use tor_error::{bad_api_usage, internal};
 use tor_guardmgr::{GuardMgr, GuardMonitor, GuardUsable};
-use tor_linkspec::RelayId;
+use tor_linkspec::RelayIdSet;
 use tor_netdir::{NetDir, Relay, SubnetConfig, WeightRole};
 use tor_rtcompat::Runtime;
 
@@ -166,14 +166,17 @@ impl<'a> ExitPathBuilder<'a> {
                 b.kind(tor_guardmgr::GuardUsageKind::Data);
                 guardmgr.update_network(netdir); // possibly unnecessary.
                 if let Some(exit_relay) = chosen_exit {
-                    let mut family = std::collections::HashSet::new();
-                    family.insert(RelayId::from(*exit_relay.id()));
+                    // TODO(nickm): Our way of building a family here is
+                    // somewhat questionable. We're only adding the ed25519
+                    // identities of the exit relay and its family to the
+                    // RelayId set.  That's fine for now, since we will only use
+                    // relays at this point if they have a known Ed25519
+                    // identity.  But if in the future the ed25519 identity
+                    // becomes optional, this will need to change.
+                    let mut family = RelayIdSet::new();
+                    family.insert(*exit_relay.id());
                     // TODO(nickm): See "limitations" note on `known_family_members`.
-                    family.extend(
-                        netdir
-                            .known_family_members(exit_relay)
-                            .map(|r| RelayId::from(*r.id())),
-                    );
+                    family.extend(netdir.known_family_members(exit_relay).map(|r| *r.id()));
                     b.restrictions()
                         .push(tor_guardmgr::GuardRestriction::AvoidAllIds(family));
                 }

crates/tor-guardmgr/semver.md

@@ -1,3 +1,6 @@
 BREAKING: Exposed objects implement the new versions of the linkspec traits
 BREAKING: note_external_{success,failure} functions now take HasRelayIds
+BREAKING: GuardRestriction now takes a RelayId or RelayIdSet.  This doesn't
+          affect serde users, but anybody calling it directly will need to
+          change.
 

crates/tor-guardmgr/src/guard.rs

@@ -395,12 +395,7 @@ impl Guard {
         match r {
             GuardRestriction::AvoidId(avoid_id) => !self.id.0.has_identity(avoid_id.as_ref()),
             GuardRestriction::AvoidAllIds(avoid_ids) => {
-                // TODO(nickm): This copies all of our IDs!
-                // We should use a contains method on a RelayIdSet or something.
-                self.id
-                    .0
-                    .identities()
-                    .all(|id| !avoid_ids.contains(&id.to_owned()))
+                self.id.0.identities().all(|id| !avoid_ids.contains(id))
             }
         }
     }

crates/tor-guardmgr/src/lib.rs

@@ -137,11 +137,11 @@ use educe::Educe;
 use futures::channel::mpsc;
 use futures::task::SpawnExt;
 use serde::{Deserialize, Serialize};
-use std::collections::{HashMap, HashSet};
+use std::collections::HashMap;
 use std::net::SocketAddr;
 use std::sync::{Arc, Mutex, Weak};
 use std::time::{Duration, Instant, SystemTime};
-use tor_linkspec::RelayId;
+use tor_linkspec::{RelayId, RelayIdSet};
 use tor_netdir::NetDirProvider;
 use tor_proto::ClockSkew;
 use tracing::{debug, info, trace, warn};
@@ -1414,9 +1414,7 @@ pub enum GuardRestriction {
     /// Don't pick a guard with the provided identity.
     AvoidId(RelayId),
     /// Don't pick a guard with any of the provided Ed25519 identities.
-    //
-    // TODO(nickm): Switch this to a type that can actually work well with RelayId.
-    AvoidAllIds(HashSet<RelayId>),
+    AvoidAllIds(RelayIdSet),
 }
 
 #[cfg(test)]

crates/tor-linkspec/src/ids.rs

@@ -11,6 +11,8 @@ use tor_llcrypto::pk::{
     rsa::{RsaIdentity, RSA_ID_LEN},
 };
 
+pub(crate) mod set;
+
 /// The type of a relay identity.
 ///
 #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash, Ord, PartialOrd, Display, strum::EnumIter)]
@@ -197,6 +199,17 @@ impl serde::Serialize for RelayId {
         self.as_ref().serialize(serializer)
     }
 }
+impl<'a> serde::Serialize for RelayIdRef<'a> {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        // TODO(nickm): maybe encode this as bytes when dealing with
+        // non-human-readable formats.
+        self.to_string().serialize(serializer)
+    }
+}
+
 impl<'de> serde::Deserialize<'de> for RelayId {
     fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
     where

crates/tor-linkspec/src/ids/set.rs

@@ -0,0 +1,135 @@
+//! Implement a set of RelayId.
+
+use std::collections::HashSet;
+
+use serde::de::Visitor;
+use tor_llcrypto::pk::{ed25519::Ed25519Identity, rsa::RsaIdentity};
+
+use crate::{RelayId, RelayIdRef};
+
+/// A set of relay identities, backed by `HashSet`.
+///
+/// # Note
+///
+/// I'd rather use `HashSet` entirely, but that doesn't let us index by
+/// RelayIdRef.
+#[derive(Clone, Debug, Default, Eq, PartialEq)]
+pub struct RelayIdSet {
+    /// The Ed25519 members of this set.
+    ed25519: HashSet<Ed25519Identity>,
+    /// The RSA members of this set.
+    rsa: HashSet<RsaIdentity>,
+}
+
+impl RelayIdSet {
+    /// Construct a new empty RelayIdSet.
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    /// Insert `key` into this set.  
+    ///
+    /// Return true if it was not already there.
+    pub fn insert<T: Into<RelayId>>(&mut self, key: T) -> bool {
+        let key: RelayId = key.into();
+        match key {
+            RelayId::Ed25519(key) => self.ed25519.insert(key),
+            RelayId::Rsa(key) => self.rsa.insert(key),
+        }
+    }
+
+    /// Remove `key` from the set.
+    ///
+    /// Return true if `key` was present.
+    pub fn remove<'a, T: Into<RelayIdRef<'a>>>(&mut self, key: T) -> bool {
+        let key: RelayIdRef<'a> = key.into();
+        match key {
+            RelayIdRef::Ed25519(key) => self.ed25519.remove(key),
+            RelayIdRef::Rsa(key) => self.rsa.remove(key),
+        }
+    }
+
+    /// Return true if `key` is a member of this set.
+    pub fn contains<'a, T: Into<RelayIdRef<'a>>>(&self, key: T) -> bool {
+        let key: RelayIdRef<'a> = key.into();
+        match key {
+            RelayIdRef::Ed25519(key) => self.ed25519.contains(key),
+            RelayIdRef::Rsa(key) => self.rsa.contains(key),
+        }
+    }
+
+    /// Return an iterator over the members of this set.
+    ///
+    /// The ordering of the iterator is undefined; do not rely on it.
+    pub fn iter(&self) -> impl Iterator<Item = RelayIdRef<'_>> {
+        self.ed25519
+            .iter()
+            .map(|id| id.into())
+            .chain(self.rsa.iter().map(|id| id.into()))
+    }
+
+    /// Return the number of keys in this set.
+    pub fn len(&self) -> usize {
+        self.ed25519.len() + self.rsa.len()
+    }
+
+    /// Return true if there are not keys in this set.
+    pub fn is_empty(&self) -> bool {
+        self.ed25519.is_empty() && self.rsa.is_empty()
+    }
+}
+
+impl<ID: Into<RelayId>> Extend<ID> for RelayIdSet {
+    fn extend<T: IntoIterator<Item = ID>>(&mut self, iter: T) {
+        for item in iter {
+            self.insert(item);
+        }
+    }
+}
+
+impl FromIterator<RelayId> for RelayIdSet {
+    fn from_iter<T: IntoIterator<Item = RelayId>>(iter: T) -> Self {
+        let mut set = RelayIdSet::new();
+        set.extend(iter);
+        set
+    }
+}
+
+impl serde::Serialize for RelayIdSet {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        serializer.collect_seq(self.iter())
+    }
+}
+
+impl<'de> serde::Deserialize<'de> for RelayIdSet {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        /// A serde visitor to deserialize a sequence of RelayIds into a
+        /// RelayIdSet.
+        struct IdSetVisitor;
+        impl<'de> Visitor<'de> for IdSetVisitor {
+            type Value = RelayIdSet;
+
+            fn expecting(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+                write!(f, "a list of relay identities")
+            }
+
+            fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error>
+            where
+                A: serde::de::SeqAccess<'de>,
+            {
+                let mut set = RelayIdSet::new();
+                while let Some(key) = seq.next_element::<RelayId>()? {
+                    set.insert(key);
+                }
+                Ok(set)
+            }
+        }
+        deserializer.deserialize_seq(IdSetVisitor)
+    }
+}

crates/tor-linkspec/src/lib.rs

@@ -77,7 +77,7 @@ mod ls;
 mod owned;
 mod traits;
 
-pub use ids::{RelayId, RelayIdError, RelayIdRef, RelayIdType, RelayIdTypeIter};
+pub use ids::{set::RelayIdSet, RelayId, RelayIdError, RelayIdRef, RelayIdType, RelayIdTypeIter};
 pub use ls::LinkSpec;
 pub use owned::{OwnedChanTarget, OwnedCircTarget, RelayIds};
 pub use traits::{ChanTarget, CircTarget, HasAddrs, HasRelayIds};