fix(iroh-net): Make a single direct address in NodeAddr instant #2580

flub · 2024-08-02T16:05:35Z

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

Self-review.
Documentation updates following the style guide, if relevant.
Tests if relevant.
All breaking changes documented.

github-actions · 2024-08-02T16:08:00Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2580/docs/iroh/

Last updated: 2024-08-05T14:50:20Z

dignifiedquire · 2024-08-02T19:51:03Z

doc comments are sad

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

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

dignifiedquire · 2024-08-02T20:00:43Z

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

+
+        // The highest acceptable latency for an endpoint path.  If the latency is higher
+        // then this the path will be ignored.
+        const MAX_LATENCY: Duration = Duration::from_secs(60 * 60);


this seems rather pointless.. latency of 1 hour?

Yes, I don't really know how this is useful either. Though this code is copy-pasted and I'd really like to avoid doing unrelated changes here. I do want to carry on fixing up things here though, but in small PRs that each target one thing because my attempt to do a bunch of this together became messy and un-reviewable.

flub · 2024-08-05T09:05:23Z

fwiw test that rely on this functionality now run in 200ms again instead of being 1.2s

# Description There are no functional changes, this only moves code and fixes up imports. ## Breaking Changes None ## Notes & open questions This is on top of #2580, which needs to be merged first. ## 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.

fix(iroh-net): Make a single direct address in NodeAddr instant

04376c2

Remove, this was moved to NodeUdpPaths

abba21d