Skip to content

Conversation

@rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Nov 6, 2025

Description

on Connection that describes the connection state, and implement the fns alpn and remote_id differently depending on what the state is.

The upside is that it should be easier to write code that works with both a 0rtt connection and a fully initialized connection without having to define a trait to abstract over both variants.

The downside is that it is a bit fancy with the types. But then for the non 0rtt case you would never see the fanciness, so maybe it is OK?

Todo:

  • remove everything but connection and handshake_completed from Incoming/Outcoming...
  • make the ConnectionState trait sealed
  • document the ConnectionState instances

Note:

if you have a handler that needs to work with both handshake completed connections and non handshake completed connections, you could now just do:

async fn handler<T: ConnectionState>(conn: Connection<T>) { ... }

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
    • List all breaking changes in the above "Breaking Changes" section.
    • Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are:

on Connection that describes the connection state, and implement the fns
alpn and remote_id differently depending on what the state is.

The upside is that it should be easier to write code that works with both
a 0rtt connection and a fully initialized connection without having to
define a trait to abstract over both variants.

The downside is that it is a bit fancy with the types. But then for the
non 0rtt case you would never see the fanciness, so maybe it is OK?
@github-actions
Copy link

github-actions bot commented Nov 6, 2025

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

Last updated: 2025-11-11T17:55:24Z

@rklaehn rklaehn changed the title Instead of duplicating the entire connection API, have a type parameter feat: Instead of duplicating the entire connection API, have a type parameter Nov 6, 2025
…... structs

Note: this requires using .connection() when you want to do something with the
connection. You could avoid this with Deref, but it seems this way of using
Deref is frowned upon in rust.
@rklaehn
Copy link
Contributor Author

rklaehn commented Nov 6, 2025

There are 3 ways to do the two Incoming/Outgoing... structs:

  1. have an explicit .connection() accessor on Incoming/Outgoing... (what I implemented)
  2. Use Deref<Connection> (this seems to be non-idiomatic rust even though I use this quite a bit in blobs)
  3. Eliminate IncomingZeroRttConnection and OutgoingZeroRttConnection and instead have a state enum inside Connection that contains either a future or an (EndpointID, Alpn) tuple. That way you could implement handshake_completed directly on Connection and Connection

Maybe the last one is the way to go?

@github-actions
Copy link

github-actions bot commented Nov 6, 2025

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: b90d216

@n0bot n0bot bot added this to iroh Nov 6, 2025
@github-project-automation github-project-automation bot moved this to 🏗 In progress in iroh Nov 6, 2025
they are now just type aliases for Connection<Incoming...> and
Connection<Outgoing...>

With this the usage is idential to before. No more .connection() needed
for e.g. .open_bi().
@ramfox ramfox added this to the v0.96 milestone Nov 7, 2025
@matheus23
Copy link
Member

Playing with this PR a bit, I think trait ConnectionState, and the structs HandshakeCompleted, IncomingZeroRtt and OutgoingZeroRtt need to be exposed in iroh/endpoint.rs. Although they're pub in connection.rs, the connection module isn't exposed itself.

@matheus23
Copy link
Member

Also, it would be nice if we'd be able to downcast a Connection<T: ConnectionState> into some kind of enum that is one of either of the connection states.

E.g. that'd be useful to implement getting the remote id on a function that's generic over ConnectionState in case that connection state happens to be HandshakeCompleted (irpc-iroh uses that to optionally add the remote ID to logs).

@rklaehn
Copy link
Contributor Author

rklaehn commented Nov 10, 2025

Also, it would be nice if we'd be able to downcast a Connection<T: ConnectionState> into some kind of enum that is one of either of the connection states.

E.g. that'd be useful to implement getting the remote id on a function that's generic over ConnectionState in case that connection state happens to be HandshakeCompleted (irpc-iroh uses that to optionally add the remote ID to logs).

We could have a fourth state Generic or AnyState where the state is an enum, but it would be some work.

Maybe instead add methods to the trait that work in any case and return Option/Result?

@rklaehn rklaehn marked this pull request as ready for review November 11, 2025 17:54
@rklaehn
Copy link
Contributor Author

rklaehn commented Nov 11, 2025

Also, it would be nice if we'd be able to downcast a Connection<T: ConnectionState> into some kind of enum that is one of either of the connection states.
E.g. that'd be useful to implement getting the remote id on a function that's generic over ConnectionState in case that connection state happens to be HandshakeCompleted (irpc-iroh uses that to optionally add the remote ID to logs).

We could have a fourth state Generic or AnyState where the state is an enum, but it would be some work.

Maybe instead add methods to the trait that work in any case and return Option/Result?

@matheus23 so WDYT, should I just add endpoint_opt and alpn_opt that works for any state and be done with it, or do something more fancy to keep the API surface smaller?

@matheus23
Copy link
Member

I was thinking of something like this:

diff --git a/iroh/src/endpoint/connection.rs b/iroh/src/endpoint/connection.rs
index 6a4593adc..7f527708a 100644
--- a/iroh/src/endpoint/connection.rs
+++ b/iroh/src/endpoint/connection.rs
@@ -646,9 +646,20 @@ mod sealed {
 }
 
 /// Trait to track the state of a [`Connection`] at compile time.
-pub trait ConnectionState: sealed::Sealed {
+pub trait ConnectionState: sealed::Sealed + Sized {
     /// State-specific data stored in the [`Connection`].
     type Data: std::fmt::Debug + Clone;
+
+    /// Turns an arbitrary connection into an enum of possible typed connection states
+    fn into_state_enum(conn: Connection<Self>) -> StateEnum;
+}
+
+/// TODO docs
+#[derive(Debug)]
+pub enum StateEnum {
+    HandshakeCompleted(Connection<HandshakeCompleted>),
+    IncomingZeroRtt(Connection<IncomingZeroRtt>),
+    OutgoingZeroRtt(Connection<OutgoingZeroRtt>),
 }
 
 /// Marker type for a connection that has completed the handshake.
@@ -666,14 +677,26 @@ pub struct OutgoingZeroRtt;
 impl sealed::Sealed for HandshakeCompleted {}
 impl ConnectionState for HandshakeCompleted {
     type Data = HandshakeCompletedData;
+
+    fn into_state_enum(conn: Connection<Self>) -> StateEnum {
+        StateEnum::HandshakeCompleted(conn)
+    }
 }
 impl sealed::Sealed for IncomingZeroRtt {}
 impl ConnectionState for IncomingZeroRtt {
     type Data = IncomingZeroRttData;
+
+    fn into_state_enum(conn: Connection<Self>) -> StateEnum {
+        StateEnum::IncomingZeroRtt(conn)
+    }
 }
 impl sealed::Sealed for OutgoingZeroRtt {}
 impl ConnectionState for OutgoingZeroRtt {
     type Data = OutgoingZeroRttData;
+
+    fn into_state_enum(conn: Connection<Self>) -> StateEnum {
+        StateEnum::OutgoingZeroRtt(conn)
+    }
 }
 
 #[allow(missing_docs)]
@@ -686,6 +709,11 @@ impl<T: ConnectionState> Connection<T> {
         &self.inner
     }
 
+    /// TODO docs
+    pub fn into_state_enum(self) -> StateEnum {
+        T::into_state_enum(self)
+    }
+
     /// Initiates a new outgoing unidirectional stream.
     ///
     /// Streams are cheap and instantaneous to open unless blocked by flow control. As a

Where into_state_enum works like a sort of downcasting.

But that'll cost us another exposed enum that we need to name and another exposed function on the ConnectionState trait.

Perhaps adding a try_remote_id and try_alpn or sth like that on Connection<T>.

@rklaehn
Copy link
Contributor Author

rklaehn commented Nov 12, 2025

Where into_state_enum works like a sort of downcasting.

But that'll cost us another exposed enum that we need to name and another exposed function on the ConnectionState trait.

Perhaps adding a try_remote_id and try_alpn or sth like that on Connection<T>.

I think the state enum will only be used very rarely, so my current thinking is that we don't do anything for now. The state enum is something people can define for themselves without much friction, much less annoying than having to define a trait.

And even the try_remote_id and try_alpn are not strictly necessary. People can always do the match themselves before they go from concrete Connection to an abstract handler.

How about we merge this now, frando and me hack it into the places where it is needed, which are already quite esoteric, and then we decide if we need more simplification.

If we decide to do something about this, one option would also be to have a 4th state "generic". That would be the same number of additional public types, but would make usage nicer than an enum.

E.g. for open_bi with the enum you would have to do a match, open_bi with the 4th state you could just use as normal.

But again, I think there is not that much need to solve this right now. 1.0 means we can't modify, but we can still add syntax candy like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🏗 In progress

Development

Successfully merging this pull request may close these issues.

6 participants