Implement holepunching - Initial implementation #830

dignifiedquire · 2023-03-13T12:52:56Z

Just opening this as a draft for tracking purpose

RangerMauve · 2023-03-13T18:51:43Z

Are you going to be testing this on mobile by any chance?

dignifiedquire · 2023-03-13T19:30:30Z

Are you going to be testing this on mobile by any chance?

Yes, very much so

…f the client connection (#826) * added `Conn` trait, `ClientConn`, & added more to `Server`, * `ClientConnManager`, `ClientConnWriter`, `ClientConnReader` Implement structures that manage the connection from the server to the client: When the server "accepts" a connection (`accept` is not yet implemented), it separates the write and read portions of the connection. It should create a `ClientConnManager` to interact with the connection. The `ClientConnManager` runs the `ClientConnWriter` and `ClientConnReader` in a loop. The `ClientConnWriter` accepts notifications & relayed packets from the server & sends them to the client. The `ClientConnReader` to reads any notifications or packets it wants the server to relay from the client, & sends them to be processed in the server. The server should hold onto the `ClientConnManager`, and use that as a handle to communicate with the `ClientConnWriter` to forward any relayed packets, or to shutdown the connection with the client if something goes wrong.

[WIP] Fix endpoint updates and deadlocks

cleanup and improve endpoint

…nnected clients (#846) * add `Client` and `Clients` `Client` will eventually be able to handle multiple `ClientConnManager`s, but for now, the lastest connection will be the one we hold onto The server will use the `Clients` struct to manages all the clients connection to the server. It is how it will actually send the packets from one client to another. * comments * shutdown & creation of `Client` is sync * add `types.rs` file keep types used in multiple systems in the same file

The `ServerActor` does all the work of coordinating traffic on the server, including sending packets, forwarding packets, adding & removing clients, adding & removing packet forwarders, adding "watchers" (verified peers in the mesh that need to be kept informed about the topology of the network), and shutting down the server.

* `Server::new` & other `Server` methods * `ClientConnHandler` & `PacketForwarderHandler` used to add new client connections and add and remove packet forwarders, respectfully. * implement init_meta_cert * temporarily(?) change node keys to `crypto_box keys * test relaying packets from a client to the server to another client, as well as a client to the server to a packet forwarder Co-authored-by: dignifiedquire <me@dignifiedquire.com>

…riting to the server

refactoring `derp::Client`

* start implementation of bin/derper * update crypto-box * derper: expand structure * implement basic http server functionality

* ci: try latest cross * try stable rust

* feat: use BASE32_LOWER_NOPAD for encoding of peerid and ticket Unfortunately BASE32_LOWER_NOPAD is not a constant, so we have to use BASE32_NOPAD and to_upper/to_lower. Also use hex for encoding hashes. This is not a public format, since cids should be encoded using Blake3Cid. Get rid of the base64 dependency. * docs: update docs to reflect that we now use base32 also use base32 for Hash. * fix: make data-encoding non optional we need it to format peerids and hashes

…efactoring (#1070) * fix: add handling of --single parameter for stdout target * fix: add handling of --single parameter for dir target * test: add tests for --single paramter both stdout and file output, but not resume * fix: allow using Blake3Cid in tests even for the MSRV test

* get rid of DataSource::File it's always named now, with the name being inferred from the path if not given * refactor: move foo/mod.rs into foo.rs

Try and improve on the online stun test: - It uses our own STUN server so we can control both sides. - The stun server avoids a unneeded delay. - Adds logging of STUN transaction IDs on both sides so we can correlate them It's still a flaky test, but succeeds sometimes now. --------- Co-authored-by: dignifiedquire <me@dignifiedquire.com> Co-authored-by: Floris Bruynooghe <flub@devork.be>

We have flaky tests on platforms. It's best to let them all run on all platforms so we see all failures and have more debugging tools.

Sounds like we don't use this format and that makes the failures more readable for humans.

This branch should now be stable enough that we should deny compiler warnings again. Fix resulting clippy error: no .into_iter() call on a reference

flub · 2023-06-06T13:22:21Z

src/bin/derper.rs

+        s.client_conn_handler(headers)
+    });
+    let tls_config = if serve_tls {
+        let contact = "d@iroh.computer".to_string(); // TODO: configurable.


We should probably do this?

yes 😅 this should be done, I am not the contact person

flub · 2023-06-06T13:26:45Z

src/bin/derper.rs

+}
+
+const NO_CONTENT_CHALLENGE_HEADER: &str = "X-Tailscale-Challenge";
+const NO_CONTENT_RESPONSE_HEADER: &str = "X-Tailscale-Response";


doesn't look like this is terribly important, but we should probably change these headers.

rklaehn · 2023-06-06T13:20:28Z

src/hp/magicsock/conn.rs

+                    self.network_send_wakers
+                        .lock()
+                        .unwrap()
+                        .replace(cx.waker().clone());


Probably does not make a big difference, but I think you only have to store the waker if you return pending.

If this returns with n>0 we will return Ready.

rklaehn · 2023-06-06T13:21:11Z

src/hp/magicsock/conn.rs

+        for (buf_out, meta_out) in bufs.iter_mut().zip(metas.iter_mut()) {
+            match self.network_recv_ch.try_recv() {
+                Err(flume::TryRecvError::Empty) => {
+                    self.network_recv_wakers


Same here. If we return ready we don't have to store the waker.

src/hp/magicsock/conn.rs

- use rooted fqdn - use .context()? instead of .expect() - typo

* fix: update integration tests * fix: ignore NAT integration tests

The distinction is now just whether something is stored internally or externally.

This makes cargo-add respect the sorting as well, it's generally nicer to find things.

The cargo check runs do not help with any speedup, they cost >20s, do not test anything that's not already covered by other tests. So remove them. Updates the cargo test invocations to: - Use --workspace instead of deprecated --all. We don't have a workspace crate yet though so could just remove it for now, but this is a bit more future-proof I guess. - Use --all-features since we have no features that are incompatible with each other. Let's keep it that way! - Select the --lib --bins and --tests targets, --all-targets would include the benches which we probably don't want to run as part of the main test run, but we don't have any yet so that is also just future-proofing.

This test is now just a best effort, you need to run it with --nocapture to see if it works or not. Sad.

ramfox

let's merge this bad boy and make incremental improvements after

Arqu

Seems simple enough, LGTM 😄

@ramfox

As suggested by @ramfox in n0-computer/iroh#830 (comment)

Implement holepunching - Initial implementation

update deps

c80c5b9

ramfox and others added 27 commits March 13, 2023 18:50

fix(netcheck): reduce locking and improved task tracking

5a733ff

simplify by removing the ability to not have a private key

d02f65b

fix: improve binding and rebinding of sockets

156560a

Merge pull request #831 from n0-computer/x-hp-fixes

235c458

[WIP] Fix endpoint updates and deadlocks

Merge remote-tracking branch 'origin/main' into x-hp

fee2b4e

magicsock: some cleanup

c0118d0

cleanup and improve endpoint

3a44560

Merge pull request #865 from n0-computer/x-hp-endpoint-cleanup

1cc89c9

cleanup and improve endpoint

remove unneeded "Conn" trait

ebb5326

http derp server derp_connection_handler

f19485a

temp - create client writer and client reader to handle reading and w…

8867e17

…riting to the server

Merge pull request #883 from n0-computer/ramfox/client_redo

53de779

refactoring `derp::Client`

initial implementation of bin/derper

5f6868e

* start implementation of bin/derper * update crypto-box * derper: expand structure * implement basic http server functionality

place holder fixes to get things to compile

12d98ff

cleanup derp test

5ef5caa

use lower level service implementation

573e6f8

integrate derp server into derper

16b8955

implement http and TLS setup

72f3f9a

fix tls integration to actually run

c8b6dad

adjust derp::Client and start impl on derp::http::Client (#889)

2545e92

derp: implement first pass at client dialing

a37b3dc

add dial timeout

d789a61

finish ping/pong

f1f43a7

dignifiedquire and others added 9 commits June 3, 2023 13:25

ci: try latest cross (#1069)

71dcab1

* ci: try latest cross * try stable rust

Merge remote-tracking branch 'origin/main' into x-hp

dcd477c

Remove DataSource::File (#1074)

a98fae8

* get rid of DataSource::File it's always named now, with the name being inferred from the path if not given * refactor: move foo/mod.rs into foo.rs

ref(ci): Do not use fail-fast strategy (#1076)

e0b2a9b

We have flaky tests on platforms. It's best to let them all run on all platforms so we see all failures and have more debugging tools.

ref(ci): Do not output clippy in json format (#1080)

a89fccf

Sounds like we don't use this format and that makes the failures more readable for humans.

ref(ci): Deny warnings again (#1075)

6df66f4

This branch should now be stable enough that we should deny compiler warnings again. Fix resulting clippy error: no .into_iter() call on a reference

flub reviewed Jun 6, 2023

View reviewed changes

rklaehn reviewed Jun 6, 2023

View reviewed changes

flub and others added 3 commits June 6, 2023 17:56

fix(derper): small derper fixes (#1083)

4fb925a

- use rooted fqdn - use .context()? instead of .expect() - typo

fix: update integration tests (#1082)

36cd904

* fix: update integration tests * fix: ignore NAT integration tests

refactor: remove BlobOrCollection (#1078)

63a2529

The distinction is now just whether something is stored internally or externally.

ramfox marked this pull request as ready for review June 8, 2023 17:18

flub and others added 5 commits June 9, 2023 10:58

ref(build): Sort dependencies in Cargo.toml (#1081)

d9e3b7b

This makes cargo-add respect the sorting as well, it's generally nicer to find things.

feat: integration metrics and viz dump (#1089)

2f65bc1

tests(netcheck): assume udp packets get lost (#1094)

daa7c0c

This test is now just a best effort, you need to run it with --nocapture to see if it works or not. Sad.

derper: stun improvements (#1091)

e191500

dignifiedquire changed the title ~~[WIP] Implement holepunching~~ Implement holepunching - Initial implementation Jun 9, 2023

ramfox approved these changes Jun 9, 2023

View reviewed changes

Arqu approved these changes Jun 9, 2023

View reviewed changes

dignifiedquire merged commit 5c96bb3 into main Jun 9, 2023

dignifiedquire deleted the x-hp branch June 9, 2023 18:16

This was referenced Jun 14, 2023

Hole punching experiment clean up #916

Closed

Update to quinn 0.10.1 and quinn-udp 0.4.0 #1006

Closed

rklaehn added a commit to n0-computer/iroh-blobs that referenced this pull request Oct 22, 2024

ref: Move packet building into conn (#1016)

147a10b

As suggested by @ramfox in n0-computer/iroh#830 (comment)

matheus23 pushed a commit that referenced this pull request Nov 14, 2024

Merge pull request #830 from n0-computer/x-hp

d78a78c

Implement holepunching - Initial implementation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement holepunching - Initial implementation #830

Implement holepunching - Initial implementation #830

dignifiedquire commented Mar 13, 2023

RangerMauve commented Mar 13, 2023

dignifiedquire commented Mar 13, 2023

flub Jun 6, 2023

dignifiedquire Jun 6, 2023

flub Jun 6, 2023

rklaehn Jun 6, 2023

rklaehn Jun 6, 2023

ramfox left a comment

Arqu left a comment

Implement holepunching - Initial implementation #830

Implement holepunching - Initial implementation #830

Conversation

dignifiedquire commented Mar 13, 2023

RangerMauve commented Mar 13, 2023

dignifiedquire commented Mar 13, 2023

flub Jun 6, 2023

Choose a reason for hiding this comment

dignifiedquire Jun 6, 2023

Choose a reason for hiding this comment

flub Jun 6, 2023

Choose a reason for hiding this comment

rklaehn Jun 6, 2023

Choose a reason for hiding this comment

rklaehn Jun 6, 2023

Choose a reason for hiding this comment

ramfox left a comment

Choose a reason for hiding this comment

Arqu left a comment

Choose a reason for hiding this comment