refactor(iroh,iroh-net)!: prefer remote to connection in api (#2610)

## Description Moves some functions and types to use `remote` to describe the information we know about a node, instead of connections. This, based on the fact that iroh-net does not itself handle connections, making the wording misleading at best. ## Breaking Changes ### iroh - `client::node::Client::connection_info` -> `client::node::Client::remote_info` - `client::node::Client::connections` -> `client::node::Client::remote_infos_iter` ### iroh-net - `endpoint::ConnectionInfo` -> `endpoint::RemoteInfo` - `endpoint::ConnectionInfo::id` is removed since it's internal information. - `endpoint::ConnectionInfo::last_alive_relay` is deprecated. Use `.relay_url.last_alive` - `endpoint::Endpoint::connection_info` -> `endpoint::Endpoint::remote_info` - `endpoint::Endpoint::connection_infos` -> `endpoint::Endpoint::remote_infos_iter` ### iroh-cli (bin) - `node connection-info` -> `node remote-info` - `node connections` -> `node remote-list` ## Notes & open questions 1. We decided to also move many of the (currently) node commands that come directly from iroh-net to a new `net` top level api. Doing so we should aim as well to move the cli to the more gracious `remote <node-id>` and `remote list`. In summary: - we have `iroh node remote <node-id>` and we want `iroh net remote <remote-id>` - we have `iroh node remote-list` and we want `iroh net remote list` Issue #2639 is created for this 2. This adds a deprecation that needs to be removed later. Issue #2640 is created for this. ## Change checklist - [x] Self-review. - [x] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] ~~Tests if relevant.~~ - [x] All breaking changes documented. --------- Co-authored-by: Diva M <[email protected]> Co-authored-by: Kasey <[email protected]>
n0-computer · Aug 17, 2024 · 9d06888 · 9d06888
1 parent 2e01d47
commit 9d06888
Show file tree

Hide file tree

Showing 12 changed files with 256 additions and 193 deletions.
diff --git a/iroh-cli/src/commands/doctor.rs b/iroh-cli/src/commands/doctor.rs
@@ -30,7 +30,7 @@ use iroh::{
         defaults::DEFAULT_STUN_PORT,
         discovery::{dns::DnsDiscovery, pkarr::PkarrPublisher, ConcurrentDiscovery, Discovery},
         dns::default_resolver,
-        endpoint::{self, Connection, ConnectionTypeStream, RecvStream, SendStream},
+        endpoint::{self, Connection, ConnectionTypeStream, RecvStream, RemoteInfo, SendStream},
         key::{PublicKey, SecretKey},
         netcheck, portmapper,
         relay::{RelayMap, RelayMode, RelayUrl},
@@ -380,7 +380,7 @@ impl Gui {
         let mp = MultiProgress::new();
         mp.set_draw_target(indicatif::ProgressDrawTarget::stderr());
         let counters = mp.add(ProgressBar::hidden());
-        let conn_info = mp.add(ProgressBar::hidden());
+        let remote_info = mp.add(ProgressBar::hidden());
         let send_pb = mp.add(ProgressBar::hidden());
         let recv_pb = mp.add(ProgressBar::hidden());
         let echo_pb = mp.add(ProgressBar::hidden());
@@ -390,7 +390,7 @@ impl Gui {
         send_pb.set_style(style.clone());
         recv_pb.set_style(style.clone());
         echo_pb.set_style(style.clone());
-        conn_info.set_style(style.clone());
+        remote_info.set_style(style.clone());
         counters.set_style(style);
         let pb = mp.add(indicatif::ProgressBar::hidden());
         pb.enable_steady_tick(Duration::from_millis(100));
@@ -401,7 +401,7 @@ impl Gui {
         let counter_task = tokio::spawn(async move {
             loop {
                 Self::update_counters(&counters2);
-                Self::update_connection_info(&conn_info, &endpoint, &node_id);
+                Self::update_remote_info(&remote_info, &endpoint, &node_id);
                 tokio::time::sleep(Duration::from_millis(100)).await;
             }
         });
@@ -419,13 +419,13 @@ impl Gui {
         }
     }
 
-    fn update_connection_info(target: &ProgressBar, endpoint: &Endpoint, node_id: &NodeId) {
+    fn update_remote_info(target: &ProgressBar, endpoint: &Endpoint, node_id: &NodeId) {
         let format_latency = |x: Option<Duration>| {
             x.map(|x| format!("{:.6}s", x.as_secs_f64()))
                 .unwrap_or_else(|| "unknown".to_string())
         };
-        let msg = match endpoint.connection_info(*node_id) {
-            Some(endpoint::ConnectionInfo {
+        let msg = match endpoint.remote_info(*node_id) {
+            Some(RemoteInfo {
                 relay_url,
                 conn_type,
                 latency,

diff --git a/iroh-cli/src/commands/node.rs b/iroh-cli/src/commands/node.rs
@@ -9,17 +9,17 @@ use comfy_table::{presets::NOTHING, Cell};
 use futures_lite::{Stream, StreamExt};
 use human_time::ToHumanTimeString;
 use iroh::client::Iroh;
-use iroh::net::endpoint::{ConnectionInfo, DirectAddrInfo};
+use iroh::net::endpoint::{DirectAddrInfo, RemoteInfo};
 use iroh::net::relay::RelayUrl;
 use iroh::net::{NodeAddr, NodeId};
 
 #[derive(Subcommand, Debug, Clone)]
 #[allow(clippy::large_enum_variant)]
 pub enum NodeCommands {
-    /// Get information about the different connections we have made
-    Connections,
-    /// Get connection information about a particular node
-    ConnectionInfo { node_id: NodeId },
+    /// Get information about the different remote nodes.
+    RemoteList,
+    /// Get information about a particular remote node.
+    Remote { node_id: NodeId },
     /// Get status of the running node.
     Status,
     /// Get statistics and metrics from the running node.
@@ -48,8 +48,8 @@ pub enum NodeCommands {
 impl NodeCommands {
     pub async fn run(self, iroh: &Iroh) -> Result<()> {
         match self {
-            Self::Connections => {
-                let connections = iroh.connections().await?;
+            Self::RemoteList => {
+                let connections = iroh.remote_infos_iter().await?;
                 let timestamp = time::OffsetDateTime::now_utc()
                     .format(&time::format_description::well_known::Rfc2822)
                     .unwrap_or_else(|_| String::from("failed to get current time"));
@@ -58,13 +58,13 @@ impl NodeCommands {
                     " {}: {}\n\n{}",
                     "current time".bold(),
                     timestamp,
-                    fmt_connections(connections).await
+                    fmt_remote_infos(connections).await
                 );
             }
-            Self::ConnectionInfo { node_id } => {
-                let conn_info = iroh.connection_info(node_id).await?;
-                match conn_info {
-                    Some(info) => println!("{}", fmt_connection(info)),
+            Self::Remote { node_id } => {
+                let info = iroh.remote_info(node_id).await?;
+                match info {
+                    Some(info) => println!("{}", fmt_info(info)),
                     None => println!("Not Found"),
                 }
             }
@@ -126,28 +126,28 @@ impl NodeCommands {
     }
 }
 
-async fn fmt_connections(
-    mut infos: impl Stream<Item = Result<ConnectionInfo, anyhow::Error>> + Unpin,
+async fn fmt_remote_infos(
+    mut infos: impl Stream<Item = Result<RemoteInfo, anyhow::Error>> + Unpin,
 ) -> String {
     let mut table = Table::new();
     table.load_preset(NOTHING).set_header(
         ["node id", "relay", "conn type", "latency", "last used"]
             .into_iter()
             .map(bold_cell),
     );
-    while let Some(Ok(conn_info)) = infos.next().await {
-        let node_id: Cell = conn_info.node_id.to_string().into();
-        let relay_url = conn_info
+    while let Some(Ok(info)) = infos.next().await {
+        let node_id: Cell = info.node_id.to_string().into();
+        let relay_url = info
             .relay_url
             .map_or(String::new(), |url_info| url_info.relay_url.to_string())
             .into();
-        let conn_type = conn_info.conn_type.to_string().into();
-        let latency = match conn_info.latency {
+        let conn_type = info.conn_type.to_string().into();
+        let latency = match info.latency {
             Some(latency) => latency.to_human_time_string(),
             None => String::from("unknown"),
         }
         .into();
-        let last_used = conn_info
+        let last_used = info
             .last_used
             .map(fmt_how_long_ago)
             .map(Cell::new)
@@ -157,9 +157,8 @@ async fn fmt_connections(
     table.to_string()
 }
 
-fn fmt_connection(info: ConnectionInfo) -> String {
-    let ConnectionInfo {
-        id: _,
+fn fmt_info(info: RemoteInfo) -> String {
+    let RemoteInfo {
         node_id,
         relay_url,
         addrs,

diff --git a/iroh-net/src/discovery.rs b/iroh-net/src/discovery.rs
@@ -260,11 +260,14 @@ impl DiscoveryTask {
     /// We need discovery if we have no paths to the node, or if the paths we do have
     /// have timed out.
     fn needs_discovery(ep: &Endpoint, node_id: NodeId) -> bool {
-        match ep.connection_info(node_id) {
-            // No connection info means no path to node -> start discovery.
+        match ep.remote_info(node_id) {
+            // No info means no path to node -> start discovery.
             None => true,
             Some(info) => {
-                match (info.last_received(), info.last_alive_relay()) {
+                match (
+                    info.last_received(),
+                    info.relay_url.as_ref().and_then(|r| r.last_alive),
+                ) {
                     // No path to node -> start discovery.
                     (None, None) => true,
                     // If we haven't received on direct addresses or the relay for MAX_AGE,

diff --git a/iroh-net/src/endpoint.rs b/iroh-net/src/endpoint.rs
@@ -45,8 +45,8 @@ pub use quinn::{
 };
 
 pub use super::magicsock::{
-    ConnectionInfo, ConnectionType, ConnectionTypeStream, ControlMsg, DirectAddr, DirectAddrInfo,
-    DirectAddrType, DirectAddrsStream,
+    ConnectionType, ConnectionTypeStream, ControlMsg, DirectAddr, DirectAddrInfo, DirectAddrType,
+    DirectAddrsStream, RemoteInfo,
 };
 
 pub use iroh_base::node_addr::{AddrInfo, NodeAddr};
@@ -708,26 +708,33 @@ impl Endpoint {
 
     // # Getter methods for information about other nodes.
 
-    /// Returns connection information about a specific node.
+    /// Returns information about the remote node identified by a [`NodeId`].
     ///
-    /// Then [`Endpoint`] stores some information about all the other iroh-net nodes it has
-    /// information about.  This includes information about the relay server in use, any
-    /// known direct addresses, when there was last any contact with this node and what kind
-    /// of connection this was.
-    pub fn connection_info(&self, node_id: NodeId) -> Option<ConnectionInfo> {
-        self.msock.connection_info(node_id)
+    /// The [`Endpoint`] keeps some information about remote iroh-net nodes, which it uses to find
+    /// the best path to a node. Having information on a remote node, however, does not mean we have
+    /// ever connected to it to or even whether a connection is even possible. The information about a
+    /// remote node will change over time, as the [`Endpoint`] learns more about the node. Future
+    /// calls may return different information. Furthermore, node information may even be
+    /// completely evicted as it becomes stale.
+    ///
+    /// See also [`Endpoint::remote_infos_iter`] which returns information on all nodes known
+    /// by this [`Endpoint`].
+    pub fn remote_info(&self, node_id: NodeId) -> Option<RemoteInfo> {
+        self.msock.remote_info(node_id)
     }
 
-    /// Returns information on all the nodes we have connection information about.
+    /// Returns information about all the remote nodes this [`Endpoint`] knows about.
+    ///
+    /// This returns the same information as [`Endpoint::remote_info`] for each node known to this
+    /// [`Endpoint`].
     ///
-    /// This returns the same information as [`Endpoint::connection_info`] for each node
-    /// known to this [`Endpoint`].
+    /// The [`Endpoint`] keeps some information about remote iroh-net nodes, which it uses to find
+    /// the best path to a node. This returns all the nodes it knows about, regardless of whether a
+    /// connection was ever made or is even possible.
     ///
-    /// Connections are currently only pruned on user action when using
-    /// [`Endpoint::add_node_addr`] so these connections are not necessarily active
-    /// connections.
-    pub fn connection_infos(&self) -> Vec<ConnectionInfo> {
-        self.msock.connection_infos()
+    /// See also [`Endpoint::remote_info`] to only retrieve information about a single node.
+    pub fn remote_infos_iter(&self) -> impl Iterator<Item = RemoteInfo> {
+        self.msock.list_remote_infos().into_iter()
     }
 
     // # Methods for less common getters.
@@ -1279,15 +1286,11 @@ mod tests {
         // first time, create a magic endpoint without peers but a peers file and add addressing
         // information for a peer
         let endpoint = new_endpoint(secret_key.clone(), None).await;
-        assert!(endpoint.connection_infos().is_empty());
+        assert_eq!(endpoint.remote_infos_iter().count(), 0);
         endpoint.add_node_addr(node_addr.clone()).unwrap();
 
         // Grab the current addrs
-        let node_addrs: Vec<NodeAddr> = endpoint
-            .connection_infos()
-            .into_iter()
-            .map(Into::into)
-            .collect();
+        let node_addrs: Vec<NodeAddr> = endpoint.remote_infos_iter().map(Into::into).collect();
         assert_eq!(node_addrs.len(), 1);
         assert_eq!(node_addrs[0], node_addr);
 
@@ -1298,7 +1301,7 @@ mod tests {
         info!("restarting endpoint");
         // now restart it and check the addressing info of the peer
         let endpoint = new_endpoint(secret_key, Some(node_addrs)).await;
-        let ConnectionInfo { mut addrs, .. } = endpoint.connection_info(peer_id).unwrap();
+        let RemoteInfo { mut addrs, .. } = endpoint.remote_info(peer_id).unwrap();
         let conn_addr = addrs.pop().unwrap().addr;
         assert_eq!(conn_addr, direct_addr);
     }

diff --git a/iroh-net/src/magicsock.rs b/iroh-net/src/magicsock.rs
@@ -75,12 +75,14 @@ mod relay_actor;
 mod timer;
 mod udp_conn;
 
+pub(crate) use node_map::Source;
+
+pub(super) use self::timer::Timer;
+
 pub use self::metrics::Metrics;
 pub use self::node_map::{
-    ConnectionType, ConnectionTypeStream, ControlMsg, DirectAddrInfo, NodeInfo as ConnectionInfo,
+    ConnectionType, ConnectionTypeStream, ControlMsg, DirectAddrInfo, RemoteInfo,
 };
-pub(super) use self::timer::Timer;
-pub(crate) use node_map::Source;
 
 /// How long we consider a STUN-derived endpoint valid for. UDP NAT mappings typically
 /// expire at 30 seconds, so this is a few seconds shy of that.
@@ -286,19 +288,19 @@ impl MagicSock {
 
     /// Returns `true` if we have at least one candidate address where we can send packets to.
     pub(crate) fn has_send_address(&self, node_key: PublicKey) -> bool {
-        self.connection_info(node_key)
+        self.remote_info(node_key)
             .map(|info| info.has_send_address())
             .unwrap_or(false)
     }
 
-    /// Retrieve connection information about nodes in the network.
-    pub(crate) fn connection_infos(&self) -> Vec<ConnectionInfo> {
-        self.node_map.node_infos(Instant::now())
+    /// Return the [`RemoteInfo`]s of all nodes in the node map.
+    pub(crate) fn list_remote_infos(&self) -> Vec<RemoteInfo> {
+        self.node_map.list_remote_infos(Instant::now())
     }
 
-    /// Retrieve connection information about a node in the network.
-    pub(crate) fn connection_info(&self, node_id: NodeId) -> Option<ConnectionInfo> {
-        self.node_map.node_info(node_id)
+    /// Return the [`RemoteInfo`] for a single node in the node map.
+    pub(crate) fn remote_info(&self, node_id: NodeId) -> Option<RemoteInfo> {
+        self.node_map.remote_info(node_id)
     }
 
     /// Returns the direct addresses as a stream.
@@ -2782,7 +2784,7 @@ mod tests {
         fn tracked_endpoints(&self) -> Vec<PublicKey> {
             self.endpoint
                 .magic_sock()
-                .connection_infos()
+                .list_remote_infos()
                 .into_iter()
                 .map(|ep| ep.node_id)
                 .collect()

diff --git a/iroh-net/src/magicsock/node_map.rs b/iroh-net/src/magicsock/node_map.rs
@@ -33,9 +33,10 @@ mod node_state;
 mod path_state;
 mod udp_paths;
 
-pub use node_state::{ConnectionType, ControlMsg, DirectAddrInfo, NodeInfo};
 pub(super) use node_state::{DiscoPingPurpose, PingAction, PingRole, SendPing};
 
+pub use node_state::{ConnectionType, ControlMsg, DirectAddrInfo, RemoteInfo};
+
 /// Number of nodes that are inactive for which we keep info about. This limit is enforced
 /// periodically via [`NodeMap::prune_inactive`].
 const MAX_INACTIVE_NODES: usize = 30;
@@ -224,9 +225,13 @@ impl NodeMap {
             .collect()
     }
 
-    /// Gets the [`NodeInfo`]s for each endpoint
-    pub(super) fn node_infos(&self, now: Instant) -> Vec<NodeInfo> {
-        self.inner.lock().node_infos(now)
+    /// Returns the [`RemoteInfo`]s for each node in the node map.
+    pub(super) fn list_remote_infos(&self, now: Instant) -> Vec<RemoteInfo> {
+        // NOTE: calls to this method will often call `into_iter` (or similar methods). Note that
+        // we can't avoid `collect` here since it would hold a lock for an indefinite time. Even if
+        // we were to find this acceptable, dealing with the lifetimes of the mutex's guard and the
+        // internal iterator will be a hassle, if possible at all.
+        self.inner.lock().remote_infos_iter(now).collect()
     }
 
     /// Returns a stream of [`ConnectionType`].
@@ -242,9 +247,9 @@ impl NodeMap {
         self.inner.lock().conn_type_stream(node_id)
     }
 
-    /// Get the [`NodeInfo`]s for each endpoint
-    pub(super) fn node_info(&self, node_id: NodeId) -> Option<NodeInfo> {
-        self.inner.lock().node_info(node_id)
+    /// Get the [`RemoteInfo`]s for the node identified by [`NodeId`].
+    pub(super) fn remote_info(&self, node_id: NodeId) -> Option<RemoteInfo> {
+        self.inner.lock().remote_info(node_id)
     }
 
     /// Prunes nodes without recent activity so that at most [`MAX_INACTIVE_NODES`] are kept.
@@ -385,13 +390,13 @@ impl NodeMapInner {
         self.by_id.iter_mut()
     }
 
-    /// Get the [`NodeInfo`]s for each endpoint
-    fn node_infos(&self, now: Instant) -> Vec<NodeInfo> {
-        self.node_states().map(|(_, ep)| ep.info(now)).collect()
+    /// Get the [`RemoteInfo`]s for all nodes.
+    fn remote_infos_iter(&self, now: Instant) -> impl Iterator<Item = RemoteInfo> + '_ {
+        self.node_states().map(move |(_, ep)| ep.info(now))
     }
 
-    /// Get the [`NodeInfo`]s for each endpoint
-    fn node_info(&self, node_id: NodeId) -> Option<NodeInfo> {
+    /// Get the [`RemoteInfo`]s for each node.
+    fn remote_info(&self, node_id: NodeId) -> Option<RemoteInfo> {
         self.get(NodeStateKey::NodeId(node_id))
             .map(|ep| ep.info(Instant::now()))
     }
@@ -666,7 +671,7 @@ mod tests {
         node_map.add_test_addr(node_addr_d);
 
         let mut addrs: Vec<NodeAddr> = node_map
-            .node_infos(Instant::now())
+            .list_remote_infos(Instant::now())
             .into_iter()
             .filter_map(|info| {
                 let addr: NodeAddr = info.into();
@@ -679,7 +684,7 @@ mod tests {
         let loaded_node_map = NodeMap::load_from_vec(addrs.clone());
 
         let mut loaded: Vec<NodeAddr> = loaded_node_map
-            .node_infos(Instant::now())
+            .list_remote_infos(Instant::now())
             .into_iter()
             .filter_map(|info| {
                 let addr: NodeAddr = info.into();