Begin revising HasAddr and its relationship to ChanTarget

HasAddr used to mean "Here are addresses that I have, at which I can
be contacted."  But "Where (and how) can I be contacted?" is now a
question for HasChannelMethod to answer.

(We still need to have "HasAddr", though, so we can answer things
like "what country is this relay in" and "are these relays in the
same /8?")

So this commit introduces:
  * A new trait for adding an implementation of HasChannelMethod in
    terms of HasAddr.
  * A requirement on ChanTarget that it needs to implement
    HasChannelMethod.

There is some temporary breakage here, marked with "TODO pt-client",
that I'll fix later in this branch.

crates/tor-guardmgr/src/bridge/relay.rs

@@ -2,7 +2,9 @@
 
 use std::sync::Arc;
 
-use tor_linkspec::{ChanTarget, CircTarget, HasAddrs, HasRelayIds, RelayIdRef, RelayIdType};
+use tor_linkspec::{
+    ChanTarget, CircTarget, HasAddrs, HasChanMethods, HasRelayIds, RelayIdRef, RelayIdType,
+};
 
 use super::{Bridge, BridgeDesc};
 
@@ -63,6 +65,12 @@ impl HasAddrs for BridgeRelay {
     }
 }
 
+impl HasChanMethods for BridgeRelay {
+    fn chan_methods(&self) -> Vec<tor_linkspec::ChannelMethod> {
+        todo!()
+    }
+}
+
 impl ChanTarget for BridgeRelay {}
 
 impl<'a> HasRelayIds for BridgeRelayWithDesc<'a> {
@@ -84,6 +92,12 @@ impl<'a> HasAddrs for BridgeRelayWithDesc<'a> {
         &[]
     }
 }
+impl<'a> HasChanMethods for BridgeRelayWithDesc<'a> {
+    fn chan_methods(&self) -> Vec<tor_linkspec::ChannelMethod> {
+        todo!()
+    }
+}
+
 impl<'a> ChanTarget for BridgeRelayWithDesc<'a> {}
 
 impl<'a> BridgeRelayWithDesc<'a> {

crates/tor-guardmgr/src/fallback.rs

@@ -16,6 +16,7 @@ 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_llcrypto::pk::ed25519::Ed25519Identity;
 use tor_llcrypto::pk::rsa::RsaIdentity;
 
@@ -128,4 +129,6 @@ impl tor_linkspec::HasRelayIdsLegacy for FallbackDir {
     }
 }
 
+impl DirectChanMethodsHelper for FallbackDir {}
+
 impl tor_linkspec::ChanTarget for FallbackDir {}

crates/tor-guardmgr/src/guard.rs

@@ -702,6 +702,8 @@ impl tor_linkspec::HasRelayIds for Guard {
     }
 }
 
+impl tor_linkspec::DirectChanMethodsHelper for Guard {}
+
 impl tor_linkspec::ChanTarget for Guard {}
 
 /// A reason for permanently disabling a guard.

crates/tor-guardmgr/src/lib.rs

@@ -1378,6 +1378,7 @@ impl tor_linkspec::HasRelayIds for FirstHop {
         self.id.identity(key_type)
     }
 }
+impl tor_linkspec::DirectChanMethodsHelper for FirstHop {}
 impl tor_linkspec::ChanTarget for FirstHop {}
 
 /// The purpose for which we plan to use a guard.

crates/tor-linkspec/semver.md

@@ -1 +1,3 @@
 MODIFIED: New ByRelayIds type.
+BREAKING: Changed the semantics of HasAddrs
+BREAKING: ChanTarget now requires HasChanMethod

crates/tor-linkspec/src/lib.rs

@@ -105,7 +105,8 @@ pub use ids::{
 pub use ls::LinkSpec;
 pub use owned::{OwnedChanTarget, OwnedCircTarget, RelayIds};
 pub use traits::{
-    ChanTarget, CircTarget, HasAddrs, HasChanMethods, HasRelayIds, HasRelayIdsLegacy,
+    ChanTarget, CircTarget, DirectChanMethodsHelper, HasAddrs, HasChanMethods, HasRelayIds,
+    HasRelayIdsLegacy,
 };
 pub use transport::{ChannelMethod, PtAddrError, PtTargetAddr, TransportId, TransportIdError};
 

crates/tor-linkspec/src/owned.rs

@@ -5,7 +5,10 @@ use std::fmt::{self, Display};
 use std::net::SocketAddr;
 use tor_llcrypto::pk;
 
-use crate::{ChanTarget, CircTarget, HasAddrs, HasRelayIds, RelayIdRef, RelayIdType};
+use crate::{
+    ChanTarget, ChannelMethod, CircTarget, HasAddrs, HasChanMethods, HasRelayIds, RelayIdRef,
+    RelayIdType,
+};
 
 /// RelayIds is an owned copy of the set of known identities of a relay.
 ///
@@ -60,11 +63,15 @@ impl RelayIds {
 
 /// OwnedChanTarget is a summary of a [`ChanTarget`] that owns all of its
 /// members.
-// TODO pt-client: I believe that this should also implement HasChanMethods.
 #[derive(Debug, Clone)]
 pub struct OwnedChanTarget {
     /// Copy of the addresses from the underlying ChanTarget.
     addrs: Vec<SocketAddr>,
+    /// Copy of the channel methods from the underlying ChanTarget.
+    //
+    // TODO: in many cases this will be redundant with addrs; if we allocate a
+    // lot of these objects, we might want to handle that.
+    methods: Vec<ChannelMethod>,
     /// Identities that this relay provides.
     ids: RelayIds,
 }
@@ -75,6 +82,12 @@ impl HasAddrs for OwnedChanTarget {
     }
 }
 
+impl HasChanMethods for OwnedChanTarget {
+    fn chan_methods(&self) -> Vec<ChannelMethod> {
+        self.methods.clone()
+    }
+}
+
 impl HasRelayIds for OwnedChanTarget {
     fn identity(&self, key_type: RelayIdType) -> Option<RelayIdRef<'_>> {
         self.ids.identity(key_type)
@@ -85,7 +98,9 @@ impl ChanTarget for OwnedChanTarget {}
 
 impl OwnedChanTarget {
     /// Construct a new OwnedChanTarget from its parts.
-    // TODO: Put this function behind a feature.
+    //
+    // TODO pt-client: Eliminate all non-testing uses of this function, and make
+    // it experimental-api.
     pub fn new(
         addrs: Vec<SocketAddr>,
         ed_identity: pk::ed25519::Ed25519Identity,
@@ -93,6 +108,7 @@ impl OwnedChanTarget {
     ) -> Self {
         Self {
             addrs,
+            methods: Vec::new(), // this is incorrect for actual use! TODO pt-client.
             ids: RelayIds::new(ed_identity, rsa_identity),
         }
     }
@@ -104,6 +120,7 @@ impl OwnedChanTarget {
     {
         OwnedChanTarget {
             addrs: target.addrs().to_vec(),
+            methods: target.chan_methods(),
             ids: RelayIds::from_relay_ids(target),
         }
     }
@@ -111,10 +128,14 @@ impl OwnedChanTarget {
     /// Construct a new OwnedChanTarget containing _only_ the provided `addr`.
     ///
     /// 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],
+                methods: self.methods.clone(),
                 ids: self.ids.clone(),
             })
         } else {
@@ -154,7 +175,9 @@ pub struct OwnedCircTarget {
 
 impl OwnedCircTarget {
     /// Construct a new OwnedCircTarget from its parts.
-    // TODO: Put this function behind a feature.
+    //
+    // TODO pt-client: Eliminate all non-testing uses of this function, and make
+    // it experimental-api.
     pub fn new(
         chan_target: OwnedChanTarget,
         ntor_onion_key: pk::curve25519::PublicKey,
@@ -200,6 +223,11 @@ impl HasRelayIds for OwnedCircTarget {
         self.chan_target.identity(key_type)
     }
 }
+impl HasChanMethods for OwnedCircTarget {
+    fn chan_methods(&self) -> Vec<ChannelMethod> {
+        self.chan_target.chan_methods()
+    }
+}
 
 impl ChanTarget for OwnedCircTarget {}
 

crates/tor-linkspec/src/traits.rs

@@ -128,7 +128,15 @@ impl<'a, T: HasRelayIds + ?Sized> FusedIterator for RelayIdIter<'a, T> {}
 
 /// An object that represents a host on the network with known IP addresses.
 pub trait HasAddrs {
-    /// Return the addresses at which you can connect to this server.
+    /// Return the addresses listed for this server.
+    ///
+    /// NOTE that these addresses are not necessarily ones that we should
+    /// connect to directly!  They can be useful for telling where a server is
+    /// located, or whether it is "close" to another server, but without knowing
+    /// the associated protocols you cannot use these to launch a connection.
+    ///
+    /// To see how to _connect_ to a relay, use [`HasChanMethods::chan_methods`]
+    //
     // TODO: This is a questionable API. I'd rather return an iterator
     // of addresses or references to addresses, but both of those options
     // make defining the right associated types rather tricky.
@@ -144,22 +152,29 @@ pub trait HasChanMethods {
     fn chan_methods(&self) -> Vec<ChannelMethod>;
 }
 
-// TODO pt-client: _possibly_ we should have a blanket implementation of
-// HasChanMethods for T: HasAddrs, or vice versa.
+/// Implement `HasChanMethods` for an object with `HasAddr` whose addresses
+/// _all_ represent a host we can connect to by a direct Tor connection at its
+/// IP addresses.
 //
-// Alternatively, we could merge HasAddrs and HasChanMethods into one trait, and
-// have default implementatoins for addrs() and chan_methods(), each
-// implemented in terms of the other.
+// TODO pt-client: We should decide whether this is a good name. Maybe the name
+// should instead suggest that we _only_ have direct methods?
+pub trait DirectChanMethodsHelper: HasAddrs {}
+
+impl<D: DirectChanMethodsHelper> HasChanMethods for D {
+    fn chan_methods(&self) -> Vec<ChannelMethod> {
+        self.addrs()
+            .iter()
+            .map(|a| ChannelMethod::Direct(*a))
+            .collect()
+    }
+}
 
 /// Information about a Tor relay used to connect to it.
 ///
 /// Anything that implements 'ChanTarget' can be used as the
 /// identity of a relay for the purposes of launching a new
 /// channel.
-//
-// TODO pt-client: I believe that this should also implement HasChanMethods, or
-// possibly implement HasChanMethods _in place of_ HasAddrs. -nickm
-pub trait ChanTarget: HasRelayIds + HasAddrs {}
+pub trait ChanTarget: HasRelayIds + HasAddrs + HasChanMethods {}
 
 /// Information about a Tor relay used to extend a circuit to it.
 ///
@@ -203,6 +218,7 @@ mod test {
             &self.addrs[..]
         }
     }
+    impl DirectChanMethodsHelper for Example {}
     impl HasRelayIdsLegacy for Example {
         fn ed_identity(&self) -> &pk::ed25519::Ed25519Identity {
             &self.ed_id

crates/tor-netdir/src/lib.rs

@@ -70,7 +70,9 @@ mod weight;
 pub mod testnet;
 
 use static_assertions::const_assert;
-use tor_linkspec::{ChanTarget, HasAddrs, HasRelayIds, RelayIdRef, RelayIdType};
+use tor_linkspec::{
+    ChanTarget, DirectChanMethodsHelper, HasAddrs, HasRelayIds, RelayIdRef, RelayIdType,
+};
 use tor_llcrypto as ll;
 use tor_llcrypto::pk::{ed25519::Ed25519Identity, rsa::RsaIdentity};
 use tor_netdoc::doc::microdesc::{MdDigest, Microdesc};
@@ -1198,6 +1200,7 @@ impl<'a> HasRelayIds for UncheckedRelay<'a> {
     }
 }
 
+impl<'a> DirectChanMethodsHelper for Relay<'a> {}
 impl<'a> ChanTarget for Relay<'a> {}
 
 impl<'a> tor_linkspec::CircTarget for Relay<'a> {