Skip to content

Commit

Permalink
fix(iroh-net): Make a single direct address in NodeAddr instant (#2580)
Browse files Browse the repository at this point in the history
## Description

In #2509 (c1c3539) we removed chosing
a random unconfirmed direct address and through both it and the relay
if we did not yet have any confirmed direct address.  This required
any direct address passed in via a NodeAddr to do a DISCO Ping-Pong
round trip before it would be used.

It turns out this is used a lot in the sense that a common socket
address is used: when you know an IP is reachable.  Requiring a ping
would mean the first QUIC packets would be dropped, Quinn would wait
to detect this before retrying and connections are delayed by an
observable second by the end of it all.  Not great for this scenario,
even if it can be argued that outside of tests this is not a common
scenario.

So this brings back the random selection of unconfirmed addresses.

It tries to do this without adding more complexity to the NodeState.
Instead it consolidates the path information and best address into the
NodeUdpState struct.  But **without any logic changes.**  It then adds
just this extra caching of the chosen address to this struct.

This is a huge simplification of a much larger refactor I was trying
to do first.  I believe that was the right track but resulted in too
many changes to do in one large PR.  So the long-term goal here is to
continue removing state manipulation outside of NodeUdpState struct so
that it gains more control of how to manage the UDP paths.  This will
help the logic of how UDP paths are chosen to be based entirely on the
PathState - which is the right model for this.

## Breaking Changes

None

## Notes & open questions

Fixed #2546

## 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.
- [x] Tests if relevant.
- [x] All breaking changes documented.
  • Loading branch information
flub authored Aug 5, 2024
1 parent 605a85d commit f5b3918
Show file tree
Hide file tree
Showing 4 changed files with 301 additions and 151 deletions.
5 changes: 4 additions & 1 deletion iroh-net/src/magicsock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,10 @@ impl MagicSock {
let dest = QuicMappedAddr(dest);

let mut transmits_sent = 0;
match self.node_map.get_send_addrs(dest) {
match self
.node_map
.get_send_addrs(dest, self.ipv6_reported.load(Ordering::Relaxed))
{
Some((public_key, udp_addr, relay_url, mut msgs)) => {
let mut pings_sent = false;
// If we have pings to send, we *have* to send them out first.
Expand Down
4 changes: 3 additions & 1 deletion iroh-net/src/magicsock/node_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ use crate::{

mod best_addr;
mod node_state;
mod udp_paths;

pub use node_state::{ConnectionType, ControlMsg, DirectAddrInfo, NodeInfo};
pub(super) use node_state::{DiscoPingPurpose, PingAction, PingRole, SendPing};
Expand Down Expand Up @@ -186,6 +187,7 @@ impl NodeMap {
pub(super) fn get_send_addrs(
&self,
addr: QuicMappedAddr,
have_ipv6: bool,
) -> Option<(
PublicKey,
Option<SocketAddr>,
Expand All @@ -195,7 +197,7 @@ impl NodeMap {
let mut inner = self.inner.lock();
let ep = inner.get_mut(NodeStateKey::QuicMappedAddr(addr))?;
let public_key = *ep.public_key();
let (udp_addr, relay_url, msgs) = ep.get_send_addrs();
let (udp_addr, relay_url, msgs) = ep.get_send_addrs(have_ipv6);
Some((public_key, udp_addr, relay_url, msgs))
}

Expand Down
Loading

0 comments on commit f5b3918

Please sign in to comment.