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

[flub]

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

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

[github-actions[bot]]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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

[rklaehn]

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

[divagant-martian]

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]

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.

[flub]

Replaced with #3110