Skip to content

Commit

Permalink
fix(iroh-net)!: only call quinn_connect if a send addr is available (#…
Browse files Browse the repository at this point in the history
…2225)

## Description

A bug in the discovery flow assumed that if we had a mapped quic address
for a `node_id`, then we had at least a relay URL or one direct address
available for that node.

This meant there were instances in which discovery should have been
launched before attempting to dial the node, but was never launched,
leading to `no UDP or relay address available for node` errors.

Now we check if addresses are available for a `node_id` and launch
discovery if we do not have any before we attempt to dial.

We also now take into account the "alive" status of any relay URL
information we have on a `node_id` when determining if we need to run
discovery for that `node_id`

### tests
This also refactors the test DNS server and the test Pkarr relay server
to use `CleanupDropGuard` for resource cleanup. It also moves the
functions that launch those servers into the `iroh-net::test_utils`
crate. This ended up being an unnecessary refactor (I ended up writing
the test in the `discovery` crate anyway), but I left it in case we need
to do other tests that rely on discovery outside of the `discovery`
crate.

## Notes & open questions

The above issue uncovered a more serious bug: the endpoint currently
dies when it attempts to dial a node without any available address
information because we return the `no UDP or relay address available for
node` error. We should not do this. In that situation, we should let
that connection timeout or launch discovery services inside the
magicsocket. That bug (#2226) is not fixed in this PR.

## Breaking changes
- Created new public struct `RelayUrlInfo` that combines the relay_url
and additional information about the state of our connection to the
remote node at this relay URL:
```
struct RelayUrlInfo {
    relay_url: RelayUrl,
    /// How long since there has been activity on this relay url
    last_alive: Option<Duration>,
    /// Latest latency information for this relay url
    latency: Option<Duration>,
}
```
- `NodeInfo.relay_url` (called `ConnectionInfo` outside of `iroh-net`)
is now `Option<RelayUrlInfo>`, changed from `Option<RelayUrl>`

## Change checklist

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

---------

Co-authored-by: Kasey <klhuizinga@gmail.com>
  • Loading branch information
Frando and ramfox committed Apr 26, 2024
1 parent 0c336c4 commit e913051
Show file tree
Hide file tree
Showing 8 changed files with 462 additions and 315 deletions.
2 changes: 1 addition & 1 deletion iroh-cli/src/commands/doctor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ impl Gui {
..
}) => {
let relay_url = relay_url
.map(|x| x.to_string())
.map(|x| x.relay_url.to_string())
.unwrap_or_else(|| "unknown".to_string());
let latency = format_latency(latency);
let addrs = addrs
Expand Down
4 changes: 2 additions & 2 deletions iroh-cli/src/commands/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ async fn fmt_connections(
let node_id: Cell = conn_info.node_id.to_string().into();
let relay_url = conn_info
.relay_url
.map_or(String::new(), |url| url.to_string())
.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 {
Expand Down Expand Up @@ -132,7 +132,7 @@ fn fmt_connection(info: ConnectionInfo) -> String {
table.add_row([bold_cell("current time"), timestamp.into()]);
table.add_row([bold_cell("node id"), node_id.to_string().into()]);
let relay_url = relay_url
.map(|r| r.to_string())
.map(|r| r.relay_url.to_string())
.unwrap_or_else(|| String::from("unknown"));
table.add_row([bold_cell("relay url"), relay_url.into()]);
table.add_row([bold_cell("connection type"), conn_type.to_string().into()]);
Expand Down
3 changes: 2 additions & 1 deletion iroh-net/src/disco.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use std::{
};

use anyhow::{anyhow, bail, ensure, Context, Result};
use serde::{Deserialize, Serialize};
use url::Url;

use crate::{key, net::ip::to_canonical, relay::RelayUrl};
Expand Down Expand Up @@ -133,7 +134,7 @@ pub struct Pong {
}

/// Addresses to which we can send. This is either a UDP or a relay address.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
pub enum SendAddr {
/// UDP, the ip addr.
Udp(SocketAddr),
Expand Down

0 comments on commit e913051

Please sign in to comment.