fix: correctly set publishing details for pkarr records (#3082)

Cleanup some changes from #3024

- Cleans up some `is_empty` logic and naming
- fixes DNS publishing to only publish the relay url if one is
available, and not the IP addresses

Manual testing confirms, this fixes a regression of connections flip
flopping between mixed and direct

Closes #3081

iroh/src/discovery/pkarr.rs

@@ -194,7 +194,14 @@ impl PkarrPublisher {
     ///
     /// This is a nonblocking function, the actual update is performed in the background.
     pub fn update_addr_info(&self, url: Option<&RelayUrl>, addrs: &BTreeSet<SocketAddr>) {
-        let info = NodeInfo::new(self.node_id, url.cloned().map(Into::into), addrs.clone());
+        let (relay_url, direct_addresses) = if let Some(relay_url) = url {
+            // Only publish relay url, and no direct addrs
+            let url = relay_url.clone();
+            (Some(url.into()), Default::default())
+        } else {
+            (None, addrs.clone())
+        };
+        let info = NodeInfo::new(self.node_id, relay_url, direct_addresses);
         self.watchable.set(Some(info)).ok();
     }
 }

iroh/src/magicsock.rs

@@ -364,7 +364,7 @@ impl MagicSock {
                 pruned += 1;
             }
         }
-        if !addr.direct_addresses.is_empty() || addr.relay_url.is_some() {
+        if !addr.is_empty() {
             self.node_map.add_node_addr(addr, source);
             Ok(())
         } else if pruned != 0 {
@@ -4065,4 +4065,63 @@ mod tests {
 
         tasks.join_all().await;
     }
+
+    #[tokio::test]
+    async fn test_add_node_addr() -> Result<()> {
+        let stack = MagicStack::new(RelayMode::Default).await?;
+        let mut rng = rand::thread_rng();
+
+        assert_eq!(stack.endpoint.magic_sock().node_map.node_count(), 0);
+
+        // Empty
+        let empty_addr = NodeAddr {
+            node_id: SecretKey::generate(&mut rng).public(),
+            relay_url: None,
+            direct_addresses: Default::default(),
+        };
+        let err = stack
+            .endpoint
+            .magic_sock()
+            .add_node_addr(empty_addr, node_map::Source::App)
+            .unwrap_err();
+        assert!(err.to_string().contains("empty addressing info"));
+
+        // relay url only
+        let addr = NodeAddr {
+            node_id: SecretKey::generate(&mut rng).public(),
+            relay_url: Some("http://my-relay.com".parse()?),
+            direct_addresses: Default::default(),
+        };
+        stack
+            .endpoint
+            .magic_sock()
+            .add_node_addr(addr, node_map::Source::App)?;
+        assert_eq!(stack.endpoint.magic_sock().node_map.node_count(), 1);
+
+        // addrs only
+        let addr = NodeAddr {
+            node_id: SecretKey::generate(&mut rng).public(),
+            relay_url: None,
+            direct_addresses: ["127.0.0.1:1234".parse()?].into_iter().collect(),
+        };
+        stack
+            .endpoint
+            .magic_sock()
+            .add_node_addr(addr, node_map::Source::App)?;
+        assert_eq!(stack.endpoint.magic_sock().node_map.node_count(), 2);
+
+        // both
+        let addr = NodeAddr {
+            node_id: SecretKey::generate(&mut rng).public(),
+            relay_url: Some("http://my-relay.com".parse()?),
+            direct_addresses: ["127.0.0.1:1234".parse()?].into_iter().collect(),
+        };
+        stack
+            .endpoint
+            .magic_sock()
+            .add_node_addr(addr, node_map::Source::App)?;
+        assert_eq!(stack.endpoint.magic_sock().node_map.node_count(), 3);
+
+        Ok(())
+    }
 }

iroh/src/magicsock/node_map.rs

@@ -709,7 +709,7 @@ mod tests {
             .into_iter()
             .filter_map(|info| {
                 let addr: NodeAddr = info.into();
-                if addr.direct_addresses.is_empty() && addr.relay_url.is_none() {
+                if addr.is_empty() {
                     return None;
                 }
                 Some(addr)
@@ -722,7 +722,7 @@ mod tests {
             .into_iter()
             .filter_map(|info| {
                 let addr: NodeAddr = info.into();
-                if addr.direct_addresses.is_empty() && addr.relay_url.is_none() {
+                if addr.is_empty() {
                     return None;
                 }
                 Some(addr)

iroh/src/magicsock/node_map/node_state.rs

@@ -634,38 +634,38 @@ impl NodeState {
 
     pub(super) fn update_from_node_addr(
         &mut self,
-        relay_url: Option<&RelayUrl>,
-        addrs: &BTreeSet<SocketAddr>,
+        new_relay_url: Option<&RelayUrl>,
+        new_addrs: &BTreeSet<SocketAddr>,
         source: super::Source,
     ) {
         if self.udp_paths.best_addr.is_empty() {
             // we do not have a direct connection, so changing the relay information may
             // have an effect on our connection status
-            if self.relay_url.is_none() && relay_url.is_some() {
+            if self.relay_url.is_none() && new_relay_url.is_some() {
                 // we did not have a relay connection before, but now we do
                 inc!(MagicsockMetrics, num_relay_conns_added)
-            } else if self.relay_url.is_some() && relay_url.is_none() {
+            } else if self.relay_url.is_some() && new_relay_url.is_none() {
                 // we had a relay connection before but do not have one now
                 inc!(MagicsockMetrics, num_relay_conns_removed)
             }
         }
 
         let now = Instant::now();
 
-        if relay_url.is_some() && relay_url != self.relay_url().as_ref() {
+        if new_relay_url.is_some() && new_relay_url != self.relay_url().as_ref() {
             debug!(
                 "Changing relay node from {:?} to {:?}",
-                self.relay_url, relay_url
+                self.relay_url, new_relay_url
             );
-            self.relay_url = relay_url.map(|url| {
+            self.relay_url = new_relay_url.map(|url| {
                 (
                     url.clone(),
                     PathState::new(self.node_id, url.clone().into(), source.clone(), now),
                 )
             });
         }
 
-        for &addr in addrs.iter() {
+        for &addr in new_addrs.iter() {
             self.udp_paths
                 .paths
                 .entry(addr.into())
@@ -677,7 +677,7 @@ impl NodeState {
                 });
         }
         let paths = summarize_node_paths(&self.udp_paths.paths);
-        debug!(new = ?addrs , %paths, "added new direct paths for endpoint");
+        debug!(new = ?new_addrs , %paths, "added new direct paths for endpoint");
     }
 
     /// Clears all the endpoint's p2p state, reverting it to a relay-only endpoint.