feat(iroh)!: introduce transport abstraction #3279

dignifiedquire · 2025-04-28T12:53:39Z

A first step towards the work of #3276

Breaking changes

iroh::watcher is now its own crate n0-watcher
iroh::endpoint::Endpoint::node_addr now returns impl Watcher<Value = Option<NodeAddr>>
iroh::endpoint::Endpoint::home_relay now returns impl Watcher<Value = Vec<RelayUrl>>
iroh::endpoint::Endpoint::bound_sockets now returns Vec<SocketAddr>

Depends on

Frando · 2025-05-05T13:20:38Z

(Note: my "merged main into refactor-transports" commit was a pushed-the-button-by-accident, didn't actually want to do that at that point in time)

github-actions · 2025-05-08T17:49:10Z

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

Last updated: 2025-05-28T12:31:31Z

allows for poll_recv to now take &mut

flub · 2025-05-27T12:24:27Z

Cargo.toml

+iroh-quinn = { git = "https://github.com/n0-computer/quinn", branch = "matheus23/mut-self" }
+iroh-quinn-udp = { git = "https://github.com/n0-computer/quinn", branch = "matheus23/mut-self" }
+iroh-quinn-proto = { git = "https://github.com/n0-computer/quinn", branch = "matheus23/mut-self" }
+netwatch = { git = "https://github.com/n0-computer/net-tools", branch = "feat-new-udp-api"}


Is the plan to merge this with git refs?

oh, I guess the netwatch branch is also a dependency I should have reviewed before?

oh, I guess the netwatch branch is also a dependency I should have reviewed before?

yes..

flub · 2025-05-27T12:29:39Z

Cargo.lock

@@ -5489,7 +5488,7 @@ version = "0.1.9"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb"
 dependencies = [
- "windows-sys 0.59.0",
+ "windows-sys 0.48.0",


I'm a bit confused by this one.

no idea, this changes back in my other PR, cargo seems to resolve this differently in some cases 🤷

flub · 2025-05-27T13:39:35Z

iroh/src/magicsock.rs

+            .expect("disconnected")
+            .into_iter()
+            .filter_map(|addr| SocketAddr::try_from(addr).ok())
+            .collect();


Pretty sure this could be written without allocating to a Vec with a bit more gymnastics... Anyway, I don't mind.

flub · 2025-05-27T13:41:17Z

iroh/src/magicsock.rs

    #[instrument(skip_all)]
-    fn try_send(&self, transmit: &quinn_udp::Transmit) -> io::Result<()> {
+    fn prepare_send(


Can this have a doc comment? It seems it is supposed to find the transport addresses on which to send?

iroh/src/magicsock.rs

flub · 2025-05-27T15:12:37Z

iroh/src/magicsock/transports.rs

+                for sender in &self.ip {
+                    if sender.is_valid_send_addr(addr) {


This is like super-nitpicky. But I noticed every time when I review that that I have to double-check these lines to figure out what is going on. In a way something like for sender in self.ip.iter().filter(|ip| ip.is_valid_send_addr(addr)) would be clearer.

I actually find that much harder to read, because the for loop gets split over multiple lines

flub · 2025-05-27T15:46:46Z

iroh/src/magicsock/transports.rs

+
+        match destination {
+            #[cfg(wasm_browser)]
+            Addr::Ip(..) => return Err(io::Error::other("IP is unsupported in browser")),


I wonder if ErrorKind::AddrNotAvailable would be suitable for these errors. Though making it an Other probably makes it clearer where this is coming from, so perhaps this is better anyway.

flub · 2025-05-27T15:52:42Z

iroh/src/magicsock/transports/ip.rs

+        metrics: Arc<MagicsockMetrics>,
+    ) -> Self {
+        // Currently gets updated on manual rebind
+        // TODO: update when UdpSocket under the hood rebinds automatically


how important is this todo? probably fairly important for ios?

it's important todo eventually, but the world doesn't end if it doesn't make it in this PR

flub · 2025-05-27T16:02:19Z

iroh/src/magicsock/transports/relay.rs

+    pub(crate) fn new(config: RelayActorConfig) -> Self {
+        let (relay_datagram_send_tx, relay_datagram_send_rx) = mpsc::channel(256);
+
+        let (relay_datagram_recv_tx, relay_datagram_recv_rx) = mpsc::channel(512);


I know you're probably copying these values... but how did we end up with the recv queue being twice as long as the send queue? Anyway, something to clean up some other time.

flub · 2025-05-27T16:07:32Z

iroh/src/magicsock/transports/relay.rs

+                }
+            };
+
+            buf_out[..dm.buf.len()].copy_from_slice(&dm.buf);


Long, long in the future I can now see us polling the TCP socket from the relay server directly during this poll, allowing us to avoid this copy. Especially once we don't have any non-quic messages left since then only very few times we'll need to send a non-QUIC message over the relay protocol that should not be passed up.

one can dream

dignifiedquire added 3 commits April 28, 2025 13:06

refactor: introduce an IP transport

0c5d837

use more iptransport

5a81be0

fixups

c4510a0

n0bot bot added this to iroh Apr 28, 2025

github-project-automation bot moved this to 🏗 In progress in iroh Apr 28, 2025

dignifiedquire and others added 2 commits April 29, 2025 12:26

wip

7b2163c

Merge branch 'main' into refactor-transports

98ac5a0

dignifiedquire added 15 commits May 6, 2025 14:48

Merge remote-tracking branch 'origin/main' into refactor-transports

25d0997

Merge remote-tracking branch 'origin/main' into refactor-transports

0269959

it compiles again

bfa5c57

impl iopoller for relay

e7093f9

remove direct reference to magicsock from relay actor

8bd0c7c

refactor: move relay actor into relay transport

bc44cfc

basic tests passing again

d812589

simplify wasm and non wasm transport handling

5b9c67d

magicsock: unify send path more

282e798

push home relay updates into the relay transport

e1ab4f8

remove custom shutdown from relay transport

84f73c9

magicsock: unify stored transports

900bef4

watch for home relay changes

2537866

fix: only publish addr if home relay is set

874f72b

happy clippy

4083c73

dignifiedquire added 6 commits May 8, 2025 21:15

wip

945eac7

wip

a560af8

fixup

28db482

a new approach

1face6b

compiles

c7dd80f

merge addrs

9e3935c

dignifiedquire force-pushed the refactor-transports branch from 1586c82 to 2ee29c2 Compare May 20, 2025 14:08

dignifiedquire added 6 commits May 20, 2025 17:36

pass destination earlier

0bf0343

switch to use udpsenders

161a502

improve relay sending by using the context

ef90325

pass through context to ip sending

7dd3feb

implement try_send

60a245c

cleanup

581e845

dignifiedquire disabled auto-merge May 21, 2025 15:13

dignifiedquire added 14 commits May 21, 2025 18:07

refactor: move sending onto the sender

cbb2bc4

proper context and task tracking using direct senders

5f7d621

fix: ensure certificates are skipped for all probes in testing

2b402b0

fix async send fn

91e2a07

update to quinn with buffering

aec022d

send pings earlier again

7b48346

only send non empty pings

53506c9

refactor: use pollsender in relay

78dc81c

fixup: remove dead code

dbf3914

fix: only do split work after readiness

941f2eb

refactor: move Transports out of the MagicSock

3750b86

allows for poll_recv to now take &mut

cleanups

99bb16f

remove bad comment

803bead

update netwatch dep

2d3c288

dignifiedquire mentioned this pull request May 23, 2025

[WIP]: rework net_report #3314

Draft

2 tasks

dignifiedquire added 2 commits May 27, 2025 10:52

Merge remote-tracking branch 'origin/main' into refactor-transports

3b13410

update netwatch

416b358

flub approved these changes May 27, 2025

View reviewed changes

dignifiedquire added 4 commits May 27, 2025 18:31

apply CR

73609e5

Merge remote-tracking branch 'origin/main' into refactor-transports

4d3b3c1

fixups after merge

1d5e619

Merge remote-tracking branch 'origin/main' into refactor-transports

3d1ec63

		for sender in &self.ip {
		if sender.is_valid_send_addr(addr) {

feat(iroh)!: introduce transport abstraction #3279

Are you sure you want to change the base?

feat(iroh)!: introduce transport abstraction #3279

Uh oh!

Conversation

dignifiedquire commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Uh oh!

Frando commented May 5, 2025

Uh oh!

github-actions bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dignifiedquire commented Apr 28, 2025 •

edited

Loading

github-actions bot commented May 8, 2025 •

edited

Loading