Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relay: refactor stream handling away from {In,Out}boundUpgrade #4075

Closed
Tracked by #2863 ...
thomaseizinger opened this issue Jun 15, 2023 · 13 comments · Fixed by #4275
Closed
Tracked by #2863 ...

relay: refactor stream handling away from {In,Out}boundUpgrade #4075

thomaseizinger opened this issue Jun 15, 2023 · 13 comments · Fixed by #4275

Comments

@thomaseizinger
Copy link
Contributor

The libp2p-relay protocol currently makes heavy use of the InboundUpgrade and OutboundUpgrade traits. This creates a lot of complexity because we actually cannot "complete" those requests within the upgrade. Instead, we have to read a message and pass the message alongside with the stream an as an event to the ConnectionHandler.

Long-term, we want completely remove these upgrade traits for ConnectionHandlers, see #2863.

Before we can do this, we need to migrate all of our own protocols away from using these traits. This issue tracks doing that for the relay protocol.

The actual steps are:

  • Instead of implementing our InboundUpgrade and OutboundUpgrade on our own types, we use ReadyUpgrade.
  • ReadyUpgrade directly hands the stream to the ConnectionHandler
  • In the ConnectionHandler, we can now either define a state machine ourselves or box up an async fn and store it inside a FuturesUnordered that gets polled as part of ConnectionHandler::poll
  • What was previously happening inside InboundUpgrade::inbound_upgrade would now happen in the async fn that gets stored inside a FuturesUnordered

One thing that we need to take into account is that the futures must be accompanied with a timeout, i.e. we cannot just read from the stream forever.

@dgarus
Copy link
Contributor

dgarus commented Jun 20, 2023

@thomaseizinger
Hello, Thomas!
I'd like to do this refactoring.

@thomaseizinger
Copy link
Contributor Author

@thomaseizinger Hello, Thomas! I'd like to do this refactoring.

Thank you! Feel free to ping me with any questions and/or open a draft PR at any time.

@dgarus
Copy link
Contributor

dgarus commented Jun 22, 2023

https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md#stop-protocol

In order to terminate a relayed connection, the relay opens a stream using an existing connection to the target peer.

Is this a typo?

@thomaseizinger
Copy link
Contributor Author

https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md#stop-protocol

In order to terminate a relayed connection, the relay opens a stream using an existing connection to the target peer.

Is this a typo?

I think terminate in this context means the same as "terminal" i.e. the end-station of something.

First we have a reservation from A to R which can be considered an "open" circuit. Once R opens a stream to B, the circuit is closed (A and B can talk to each other) and thus, the end is reached aka terminated.

@dgarus
Copy link
Contributor

dgarus commented Jun 23, 2023

https://github.com/libp2p/specs/blob/master/relay/circuit-v2.md#stop-protocol

In order to terminate a relayed connection, the relay opens a stream using an existing connection to the target peer.

Is this a typo?

I think terminate in this context means the same as "terminal" i.e. the end-station of something.

First we have a reservation from A to R which can be considered an "open" circuit. Once R opens a stream to B, the circuit is closed (A and B can talk to each other) and thus, the end is reached aka terminated.

Ok, I got it. Thanks!

@mxinden
Copy link
Member

mxinden commented Jun 26, 2023

Threw me of in the past as well @dgarus. In case you can think of a better wording, please propose a pull request against libp2p/specs.

@dgarus
Copy link
Contributor

dgarus commented Jun 30, 2023

@thomaseizinger
Hello, Thomas!
How to use ReadyUpgrade in the case when Upgrade has a state?
For example,

pub struct Upgrade {
    pub src_peer_id: PeerId,
    ...
}

src_peer_id comes from In::NegotiateOutboundConnect and we have to remember it.

@thomaseizinger
Copy link
Contributor Author

What you do is remember the state locally in a VecDeque and request a new stream. Once you receive a stream, you pop_front the state and you can combine your state with the new stream. See https://github.com/libp2p/rust-libp2p/pull/3763/files for example.

@thomaseizinger
Copy link
Contributor Author

What you do is remember the state locally in a VecDeque and request a new stream. Once you receive a stream, you pop_front the state and you can combine your state with the new stream. See #3763 (files) for example.

The reason this works is because streams are "interchangable", i.e. there is no identity tied to a requested stream and there also are no guarantees as to how data arrives at the remote when it is sent over multiple streams.

@dgarus
Copy link
Contributor

dgarus commented Jul 7, 2023

@thomaseizinger
Hi, Thomas!
I'm on vacation, a lot of sunshine and wine don't allow me to make progress as fast as I would want.
So, I'm going to continue working on this issue in 2 weeks.

@thomaseizinger
Copy link
Contributor Author

I'm on vacation, a lot of sunshine and wine don't allow me to make progress as fast as I would want.
So, I'm going to continue working on this issue in 2 weeks.

No pressure at all! Your dedication is much appreciated :)

I'll be on vacation too in the following week. Cheers 🍷

@thomaseizinger
Copy link
Contributor Author

@dgarus I hope you had a nice vacation! I am curious whether you've already got some work locally that you can put up as a draft PR?

@dgarus
Copy link
Contributor

dgarus commented Jul 31, 2023

@dgarus I hope you had a nice vacation! I am curious whether you've already got some work locally that you can put up as a draft PR?

@thomaseizinger
Hello! Thanks!
I finished with stop_protocol and currently I'm working on hop_protocol.

@thomaseizinger thomaseizinger added this to the Simplify ConnectionHandler trait milestone Sep 18, 2023
@thomaseizinger thomaseizinger removed this from the Simplify ConnectionHandler trait milestone Sep 19, 2023
@mergify mergify bot closed this as completed in #4275 Sep 20, 2023
mergify bot pushed a commit that referenced this issue Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants