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

identify: Exchange fails with go-libp2p over QUIC when initialy closing #3298

Open
mxinden opened this issue Jan 2, 2023 · 3 comments
Open

Comments

@mxinden
Copy link
Member

mxinden commented Jan 2, 2023

To receive a remote's identify information one opens a stream and waits for the remote to send the identify payload in that stream.

When connecting to a go-libp2p node over a QUIC connection, identify exchanges fail.

async fn recv<T>(mut socket: T) -> Result<Info, UpgradeError>
where
T: AsyncRead + AsyncWrite + Unpin,
{
socket.close().await?;

Changing the above to flush instead of close resolves the issue. Note that flushing instead of no action at all is needed in case multistream-select V1Lazy is used where the multistream-select message is only send on the stream once userdata is send or it is flushed.

Occurs with the rust-libp2p punchr client when connecting to go-libp2p IPFS nodes.

Opening with little details for now to have a central place to track. @elenaf9 in case you need more details, let me know.

@mxinden
Copy link
Member Author

mxinden commented Jan 2, 2023

Note that flushing instead of no action at all is needed in case multistream-select V1Lazy is used where the multistream-select message is only send on the stream once userdata is send or it is flushed.

This is likely incorrect. That is, a read should be enough according to the below:

/// Reading from a `Negotiated` I/O stream that still has pending negotiation
/// protocol data to send implicitly triggers flushing of all yet unsent data.

@dignifiedquire
Copy link
Member

dignifiedquire commented Jan 2, 2023

Might this be the reason I am not able to get a single hole punch to work with the punchr setup + rust-libp2p-client?

Edit: Almost no connections, there is like 1 per hour or so at most that succeeds (been running for a couple of days now)

@mxinden
Copy link
Member Author

mxinden commented Jan 3, 2023

@dignifiedquire small update on rust-libp2p punchr. With a couple of upcoming patches I am able to achieve ~50% success rate.

  • This patch, i.e. flush instead of close, or even none of either.
  • fix(quic): Identify /quic as QUIC address #3288
  • rust-libp2p punchr to use keep_alive::Behaviour to await identify.
  • rust-libp2p punchr to prioritize QUIC connections to bootstrap nodes to learn QUIC external addresses.

While 50% is still behind go-libp2p, I would consider this a good step forward.

mergify bot pushed a commit that referenced this issue Feb 19, 2023
Don't close the stream `protocol::recv`.

This is a short-term fix for #3298.

The issue behind this is a general one on the QUIC transport when closing streams, as described in #3343. This PR only circumvents the issue for identify. A proper solution for our QUIC transport still needs more thought.

Pull-Request: #3344.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants