feat(transport): Implement dial_with_new_port to prevent port reuse #3910
Conversation
This is useful in case of AutoNAT, whereby we do not want to reuse the same port when probing.
217ae9e to
2b2f5f5
Compare
| @@ -396,6 +396,33 @@ where | |||
| request_id | |||
| } | |||
|
|
|||
| /// TODO: Document | |||
| pub fn send_request_with_new_port( | |||
There was a problem hiding this comment.
This doesn't really work unfortunately. libp2p-request-response hides the idea of connections and streams from the user. At the moment, there is no guarantee that a certain messages is actually going to be sent over a specific connection.
I think to effectively utilize this API in AutoNAT, we will have to rewrite it to not use libp2p-request-response.
The flow would something like:
- Issue a new
ToSwarm::Dialthat uses thenew_portoption - Grab the (future)
ConnectionIdfromDialOpts - Remember it in the behaviour, together with the request that we want to send on that connection
- Wait for
handle_established_outbound_connectionwith this newConnectionId - Initialize the handler with that connection
This is quite specific to AutoNAT and I am not sure we should extend libp2p-request-response to be able to do this. We could make libp2p-request-response do such kind of things as well. But, I think it is a bad fit for the abstraction because it gets too leaky. To me, libp2p-request-response is all about providing convenience when you just want to declare a message type and send it back and forth between two peers. If we don't hide certain details about the internals of libp2p with it, then the complexity for the user doesn't actually go away.
Another thing that bothers me is that we expose functions such as add_address. Those should go away in my opinion.
There was a problem hiding this comment.
This is quite specific to AutoNAT and I am not sure we should extend libp2p-request-response to be able to do this.
Agreed.
There was a problem hiding this comment.
Note that even with rewriting it, the issue of "don't run other protocols over that connection" is not solved although I am not entirely convinced it is actually a problem.
| fn dial_with_new_port( | ||
| &mut self, | ||
| _addr: Multiaddr, | ||
| ) -> Result<Self::Dial, TransportError<Self::Error>> { | ||
| unimplemented!() | ||
| } |
There was a problem hiding this comment.
Not a fan of this API at all but I also don't have a better idea in the short-term. Long-term, I'd like to get rid of the Dial associated type (see #3436).
|
Closing this PR in anticipation of a more thought out perspective in #4226 |
This is useful in case of AutoNAT, whereby we do not want to reuse the same port when probing.
A new
dial_with_new_portis added, that TCP and QUIC can use to create a new source socket (port). This is called whenDialOptshas the option enabled to use a new port, which can be used by AutoNAT.Description
This is a draft to see what options there are to implement a fix for both #3889 and #3900. Please bear with me as I try to figure out the right way to maneuver the APIs and concepts here.
The AutoNAT client uses
request_responseto send a request, so that has to have an option added for using a new port too. For the AutoNAT server, I observed that secondary probes will still cause a dial to error withAddrNotAvailableif not using the newwith_new_port. For this reason the AutoNAT server also has to create a new socket (TCP). However, this is probably because I am not getting rid of the sockets after AutoNAT is done. I am not sure how to do that.For TCP the 'fix' is to ignore port reuse if enabled. And for QUIC we should create a new dialer. What I am trying to understand is how these sockets/dialers are handled and can be managed properly.
Notes & open questions
Change checklist