diff --git a/iroh/src/discovery/pkarr.rs b/iroh/src/discovery/pkarr.rs index a319094b68..80fcf0425d 100644 --- a/iroh/src/discovery/pkarr.rs +++ b/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) { - 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(); } } diff --git a/iroh/src/magicsock.rs b/iroh/src/magicsock.rs index 49336e0d20..8ecac68cb3 100644 --- a/iroh/src/magicsock.rs +++ b/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(()) + } } diff --git a/iroh/src/magicsock/node_map.rs b/iroh/src/magicsock/node_map.rs index d25fbf84fe..e93d9d054b 100644 --- a/iroh/src/magicsock/node_map.rs +++ b/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) diff --git a/iroh/src/magicsock/node_map/node_state.rs b/iroh/src/magicsock/node_map/node_state.rs index d62d4c294e..936ea01161 100644 --- a/iroh/src/magicsock/node_map/node_state.rs +++ b/iroh/src/magicsock/node_map/node_state.rs @@ -634,17 +634,17 @@ impl NodeState { pub(super) fn update_from_node_addr( &mut self, - relay_url: Option<&RelayUrl>, - addrs: &BTreeSet, + new_relay_url: Option<&RelayUrl>, + new_addrs: &BTreeSet, 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) } @@ -652,12 +652,12 @@ impl NodeState { 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), @@ -665,7 +665,7 @@ impl NodeState { }); } - 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.