feat(iroh-net)!: Create our own connection struct #2768

flub · 2024-10-01T11:36:25Z

Description

We used to re-export quinn::Connection, this makes this an owned
connection so we can add our own methods to this. This adds the
.remote_peer_id() method and removes the helper function for this.

Breaking Changes

iroh_net::endpoint::get_remote_node_id has been removed. Use
iroh_net::endpoint::Connection::remote_node_id instead.

Notes & open questions

We have talked about wanting to do this for so long, decided to just
give this a go.
Strictly speaking we do not need to remove get_remote_node_id
right now.
This completely removes the ability of iroh-blobs to work with
upstream Quinn connections. It now only works with iroh. We could
probably use the h3::quic traits in iroh-blobs to make it generic
over QUIC implementation.
The code just lives in iroh_net::endpoint now. That's probably
fine, but it's a large file by now. Easy enough to move it later
though.
The public API is at iroh_net::endpoint::Connection. Perhaps it
should also be exported at iroh_net::Connection just like
Endpoint is. Though what belongs there? What about all the other
types? Hence I'm leaving this as the status quo for this PR and
leave it where it is.
There are a bunch of new Quinn structs exported. This probably
should have happened earlier, we knew we were not exporting enough
of those but never did a proper check on what is needed. Now maybe
everything is there, bar possibly some things from quinn-udp still
missing.

Change checklist

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

We used to re-export quinn::Connection, this makes this an owned connection so we can add our own methods to this. This adds the `.remote_peer_id()` method and removes the helper function for this.

github-actions · 2024-10-01T11:44:07Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 0d3af4d

divagant-martian · 2024-10-01T15:27:13Z

Seen from iroh_net it makes sense. Is there anything preventing us from implementing Into<quinn::Connection> or AsRef or similar so that it doesn't affect iroh_blobs as much?

flub · 2024-10-02T08:46:58Z

Seen from iroh_net it makes sense. Is there anything preventing us from implementing Into<quinn::Connection> or AsRef or similar so that it doesn't affect iroh_blobs as much?

This is a good idea!

flub · 2024-10-02T08:56:14Z

Seen from iroh_net it makes sense. Is there anything preventing us from implementing Into<quinn::Connection> or AsRef or similar so that it doesn't affect iroh_blobs as much?

This is a good idea!

Note that there are no code changes since iroh-blobs already uses iroh_net::nedpoint::Connection as import. I'd love to get @rklaehn's opinion on this before changing that back to iroh_quinn::Connection. Because the real breakage was done at the point when we transitioned from quinn to iroh-quinn.

divagant-martian · 2024-10-03T15:28:19Z

Back to this, is there any reason to not implement Deref and DerefMut so that we don't have to keep updating docs and migrating new functions? Sometimes docs for Dereferencing types is not great, but otherwise it seems appropriate

flub · 2024-10-03T15:58:35Z

Back to this, is there any reason to not implement Deref and DerefMut so that we don't have to keep updating docs and migrating new functions? Sometimes docs for Dereferencing types is not great, but otherwise it seems appropriate

I'm not really a fan of Deref for users, it always confuses me as a user. Plus it gives us less flexibility. Yes we need to keep this in sync when we update the dependency, but we also get to benefit from better integrated documentation - I did make changes to the docs here. So I think I'd prefer this approach.

divagant-martian · 2024-10-03T16:08:18Z

I'm not really a fan of Deref for users

Yeah, I wish rust had a better way to do or at least show this when all we need is delegation.
Agree with the conclussion

rklaehn · 2024-10-04T08:21:05Z

Can you elaborate your future plans with this? If the only thing this allows us to do is to change the get_remote_node_id fn into a member fn, I would say it is not worth it.

It would help with making examples more approachable, but you could do the same (except for an extra import) with an extension method.

flub · 2024-10-04T09:23:57Z

Can you elaborate your future plans with this?

This can enable a number of things which we have shied away from in the past simply because taking the step of adding this struct was not yet done:

Connection type and changes to it are currently on on the Endpoint, they also belong on the Connection.
Connection information, like which paths are used, the latencies of those paths, etc, also belongs on the Connection.
Some APIs don't work well for us, e.g. Connection::local_ip or Connection::remote_address are not that meaningful. This allows us to remove those and replace them with more appropriate APIs.
It is generally not great to expose types which you have no control over in your public API. This doesn't solve that fully, but it does tackle an important struct.

Probably some more things I'm not remembering right now.

Admittedly this PR doesn't tackle many of these things. But it makes it so much easier to do in the future.

b5 · 2024-10-09T10:03:30Z

docs: https://n0-computer.github.io/iroh/pr/2768/docs/iroh_net/index.html

rklaehn · 2024-10-16T11:10:43Z

Given that we have to hide all of quinn for 1.0 anyway, I think this is reasonable to do.

rklaehn · 2024-10-16T11:11:43Z

iroh-gossip/examples/chat.rs

@@ -221,7 +221,7 @@ async fn handle_connection(
 ) -> anyhow::Result<()> {
    let alpn = conn.alpn().await?;
    let conn = conn.await?;
-    let peer_id = iroh_net::endpoint::get_remote_node_id(&conn)?;
+    let peer_id = conn.remote_node_id()?;


Call this node_id while we're at it?

rklaehn · 2024-10-16T11:11:50Z

iroh-docs/src/net.rs

@@ -116,7 +116,7 @@ where
 {
    let t_start = Instant::now();
    let connection = connecting.await.map_err(AcceptError::connect)?;
-    let peer = get_remote_node_id(&connection).map_err(AcceptError::connect)?;
+    let peer = connection.remote_node_id().map_err(AcceptError::connect)?;


rklaehn · 2024-10-16T11:12:08Z

iroh-cli/src/commands/doctor.rs

@@ -792,7 +792,7 @@ async fn accept(
            match connecting.await {
                Ok(connection) => {
                    if n == 0 {
-                        let Ok(remote_peer_id) = endpoint::get_remote_node_id(&connection) else {
+                        let Ok(remote_peer_id) = connection.remote_node_id() else {


let Ok(remote_node_id)

rklaehn · 2024-10-16T11:15:56Z

iroh-net/src/endpoint.rs

+///
+/// May be cloned to obtain another handle to the same connection.
+#[derive(Debug, Clone)]
+pub struct Connection {


Can we have #[repr(transparent)] on this? Then we could use ref_cast to get a &quinn::Connection from a &Connection as a (possibly feature flagged) compat feature.

We might or might not do this, but the repr(transparent) certainly does not do any harm unless we plan to have additional stuff here...

divagant-martian · 2024-11-12T22:52:08Z

PR came first checking approved ones (candidates for merging) but this has a long list of conflicts. Since this has essentially passed the approval bar, here's a friendly ping to refresh it @flub

flub · 2024-11-13T14:38:19Z

Yeah, I was waiting for the dust to settle on all the renames and moving crates before getting back to this. Doing backwards-incompatible changes was very difficult at some point during this churn.

Converted it back to draft for now, will pick it up after the next release probably.

feat(iroh-net)!: Create our own connection struct

96c4cef

We used to re-export quinn::Connection, this makes this an owned connection so we can add our own methods to this. This adds the `.remote_peer_id()` method and removes the helper function for this.

flub requested review from dignifiedquire, rklaehn and divagant-martian and removed request for dignifiedquire October 1, 2024 11:36

rklaehn approved these changes Oct 16, 2024

View reviewed changes

flub mentioned this pull request Oct 29, 2024

refactor: Use new iroh-net Connection API n0-computer/iroh-docs#4

Draft

4 tasks

flub marked this pull request as draft November 13, 2024 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-net)!: Create our own connection struct #2768

feat(iroh-net)!: Create our own connection struct #2768

flub commented Oct 1, 2024

github-actions bot commented Oct 1, 2024

divagant-martian commented Oct 1, 2024

flub commented Oct 2, 2024

flub commented Oct 2, 2024

divagant-martian commented Oct 3, 2024

flub commented Oct 3, 2024

divagant-martian commented Oct 3, 2024

rklaehn commented Oct 4, 2024

flub commented Oct 4, 2024

b5 commented Oct 9, 2024

rklaehn commented Oct 16, 2024

rklaehn Oct 16, 2024

rklaehn Oct 16, 2024

rklaehn Oct 16, 2024

rklaehn Oct 16, 2024

divagant-martian commented Nov 12, 2024

flub commented Nov 13, 2024

feat(iroh-net)!: Create our own connection struct #2768

Are you sure you want to change the base?

feat(iroh-net)!: Create our own connection struct #2768

Conversation

flub commented Oct 1, 2024

Description

Breaking Changes

Notes & open questions

Change checklist

github-actions bot commented Oct 1, 2024

divagant-martian commented Oct 1, 2024

flub commented Oct 2, 2024

flub commented Oct 2, 2024

divagant-martian commented Oct 3, 2024

flub commented Oct 3, 2024

divagant-martian commented Oct 3, 2024

rklaehn commented Oct 4, 2024

flub commented Oct 4, 2024

b5 commented Oct 9, 2024

rklaehn commented Oct 16, 2024

rklaehn Oct 16, 2024

Choose a reason for hiding this comment

rklaehn Oct 16, 2024

Choose a reason for hiding this comment

rklaehn Oct 16, 2024

Choose a reason for hiding this comment

rklaehn Oct 16, 2024

Choose a reason for hiding this comment

divagant-martian commented Nov 12, 2024

flub commented Nov 13, 2024