fix(iroh-net): do not persist invalid node addresses (#2209)

## Description

Changes the logic of which addresses are persisted in the magic
endpoint:
* For nodes that were used in the current session, only the paths that
were successfully used will be saved. This means that invalid paths will
be omitted. If all paths are invalid, and no relay URL is known, the
node will not be persisted at all.
* For nodes that were not used at all, all known information will be
persisted. This is in place to not forget all info if a node starts,
loads the addressbook, and then shuts down again without doing anything.

## Notes & open questions

Still needs a test for the new behavior, will do tomorrow

## Change checklist

- [ ] Self-review.
- [ ] Documentation updates if relevant.
- [ ] Tests if relevant.

iroh-net/src/magicsock/node_map.rs

@@ -93,11 +93,9 @@ impl NodeMap {
         }
     }
 
-    /// Get the known node addresses stored in the map. Nodes with empty addressing information are
-    /// filtered out.
-    #[cfg(test)]
-    pub(super) fn known_node_addresses(&self) -> Vec<NodeAddr> {
-        self.inner.lock().known_node_addresses().collect()
+    /// Get the known node addresses which should be persisted.
+    pub fn node_addresses_for_storage(&self) -> Vec<NodeAddr> {
+        self.inner.lock().node_addresses_for_storage().collect()
     }
 
     /// Add the contact information for a node.
@@ -241,14 +239,13 @@ impl NodeMap {
     pub(super) async fn save_to_file(&self, path: &Path) -> anyhow::Result<usize> {
         ensure!(!path.is_dir(), "{} must be a file", path.display());
 
-        // So, not sure what to do here.
-        let mut known_nodes = self
-            .inner
-            .lock()
-            .known_node_addresses()
-            .collect::<Vec<_>>()
-            .into_iter()
-            .peekable();
+        // always prune inactive addresses first
+        self.prune_inactive();
+
+        // persist only the nodes which were
+        // * not used at all (so we don't forget everything we loaded)
+        // * were attempted to be used, and have at least one usable path
+        let mut known_nodes = self.node_addresses_for_storage().into_iter().peekable();
         if known_nodes.peek().is_none() {
             // prevent file handling if unnecessary
             return Ok(0);
@@ -295,13 +292,14 @@ impl NodeMap {
 }
 
 impl NodeMapInner {
-    /// Get the known node addresses stored in the map. Nodes with empty addressing information are
-    /// filtered out.
-    fn known_node_addresses(&self) -> impl Iterator<Item = NodeAddr> + '_ {
-        self.by_id.values().filter_map(|endpoint| {
-            let node_addr = endpoint.node_addr();
-            (!node_addr.info.is_empty()).then_some(node_addr)
-        })
+    /// Get those node addresses from the map which should be persistet.
+    ///
+    /// This filters out all addresses which were neither loaded from storage nor used.
+    /// For node addresses which were used, only the used paths will be included.
+    fn node_addresses_for_storage(&self) -> impl Iterator<Item = NodeAddr> + '_ {
+        self.by_id
+            .values()
+            .filter_map(|endpoint| endpoint.node_addr_for_storage())
     }
 
     /// Create a new [`NodeMap`] from data stored in `path`.
@@ -661,10 +659,6 @@ mod tests {
         let relay_x: RelayUrl = "https://my-relay-1.com".parse().unwrap();
         let relay_y: RelayUrl = "https://my-relay-2.com".parse().unwrap();
 
-        fn addr(port: u16) -> SocketAddr {
-            (std::net::IpAddr::V4(Ipv4Addr::LOCALHOST), port).into()
-        }
-
         let direct_addresses_a = [addr(4000), addr(4001)];
         let direct_addresses_c = [addr(5000)];
 
@@ -686,20 +680,51 @@ mod tests {
 
         let loaded_node_map = NodeMap::load_from_file(&path).unwrap();
         let loaded: HashMap<PublicKey, AddrInfo> = loaded_node_map
-            .known_node_addresses()
+            .node_addresses_for_storage()
             .into_iter()
             .map(|NodeAddr { node_id, info }| (node_id, info))
             .collect();
 
         let og: HashMap<PublicKey, AddrInfo> = node_map
-            .known_node_addresses()
+            .node_addresses_for_storage()
             .into_iter()
             .map(|NodeAddr { node_id, info }| (node_id, info))
             .collect();
         // compare the node maps via their known nodes
         assert_eq!(og, loaded);
     }
 
+    fn addr(port: u16) -> SocketAddr {
+        (std::net::IpAddr::V4(Ipv4Addr::LOCALHOST), port).into()
+    }
+
+    #[tokio::test]
+    async fn save_node_map_cases() {
+        let node_a = SecretKey::generate().public();
+        let direct_addrs_a = [addr(4000), addr(4001)];
+        let node_addr_a = NodeAddr::new(node_a).with_direct_addresses(direct_addrs_a);
+
+        let node_map = NodeMap::default();
+        node_map.add_node_addr(node_addr_a.clone());
+
+        // unused endpoints are included
+        let list = node_map.node_addresses_for_storage();
+        assert_eq!(list, vec![node_addr_a.clone()]);
+
+        // once the endpoint is used, only valid paths are included
+        node_map.receive_udp(direct_addrs_a[0]);
+        let list = node_map.node_addresses_for_storage();
+        assert_eq!(
+            list,
+            vec![NodeAddr::new(node_a).with_direct_addresses([direct_addrs_a[0]])]
+        );
+
+        // if all paths are used, all are included
+        node_map.receive_udp(direct_addrs_a[1]);
+        let list = node_map.node_addresses_for_storage();
+        assert_eq!(list, vec![node_addr_a.clone()]);
+    }
+
     #[test]
     fn test_prune_direct_addresses() {
         let _guard = iroh_test::logging::setup();

iroh-net/src/magicsock/node_map/node_state.rs

@@ -1082,15 +1082,37 @@ impl NodeState {
         self.direct_addr_state.keys().copied()
     }
 
-    /// Get the addressing information of this endpoint.
-    pub(super) fn node_addr(&self) -> NodeAddr {
-        let direct_addresses = self.direct_addresses().map(SocketAddr::from).collect();
-        NodeAddr {
-            node_id: self.node_id,
-            info: AddrInfo {
-                relay_url: self.relay_url(),
-                direct_addresses,
-            },
+    pub(super) fn used_paths(&self) -> impl Iterator<Item = IpPort> + '_ {
+        self.direct_addr_state
+            .iter()
+            .filter_map(|(addr, state)| state.last_alive().map(|_| *addr))
+    }
+
+    /// Get the addressing information of this endpoint that should be stored.
+    ///
+    /// If the endpoint was not used at all in this session, all known addresses will be returned.
+    /// If the endpoint was used, only the paths that were in use will be returned.
+    ///
+    /// Returns `None` if the resulting [`NodeAddr`] would be empty.
+    pub(super) fn node_addr_for_storage(&self) -> Option<NodeAddr> {
+        let direct_addresses = if self.last_used().is_none() {
+            self.direct_addresses()
+                .map(SocketAddr::from)
+                .collect::<BTreeSet<_>>()
+        } else {
+            self.used_paths().map(SocketAddr::from).collect()
+        };
+        let relay_url = self.relay_url();
+        if direct_addresses.is_empty() && relay_url.is_none() {
+            None
+        } else {
+            Some(NodeAddr {
+                node_id: self.node_id,
+                info: AddrInfo {
+                    relay_url: self.relay_url(),
+                    direct_addresses,
+                },
+            })
         }
     }