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

multistream: deprecate the simopen extension #573

Closed
2 tasks done
marten-seemann opened this issue Sep 5, 2023 · 9 comments
Closed
2 tasks done

multistream: deprecate the simopen extension #573

marten-seemann opened this issue Sep 5, 2023 · 9 comments

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented Sep 5, 2023

It looks like the simopen extension to multistream is barely every used in production. I've been running a public Kubo node for an extended amount of time, only running the TCP transport and enabling the Accelerated DHT client, and simopen is exercised not even once per hour.

This proves out intuition (see libp2p/go-libp2p#2330) that simopen is additional complexity with very little benefit. See that issue for further motivation.

I suggest:

  • deprecating the specs here
  • removing it from go-libp2p

I don't think any other implementation ever bothered to actually implement it. Is that correct, @mxinden @achingbrain?

@mxinden
Copy link
Member

mxinden commented Sep 5, 2023

I am in favor of deprecating simopen.

I don't think any other implementation ever bothered to actually implement it. Is that correct, @mxinden @achingbrain?

Implemented, but never merged or released. See libp2p/rust-libp2p#2066.

@p-shahi
Copy link
Member

p-shahi commented Sep 6, 2023

Afaik I don't think js-libp2p supports simopen (couldn't find /libp2p/simultaneous-connect protocol negotiation string in multistream-select package or elsewhere in the monorepo); @achingbrain can confirm though

@marten-seemann
Copy link
Contributor Author

It's gone! Bye!

@achingbrain
Copy link
Member

Sorry, missed this - it's not implemented in js-libp2p as @p-shahi found 👍

@Stebalien
Copy link
Member

This needs to be reverted or we need to stop re-using the inbound the inbound TCP port for outbound connections.

This feature didn't exist to support holepunching, it existed to fix the following:

  1. TCP connection drops.
  2. TCP connection re-connects simultaneously on both sides.
  3. Due to reuseport, we form a single TCP connection (TCP simultaneous connect) and both sides think they're the client.
  4. The connection hangs.

We're now experiencing sync issues in lotus where nodes fail to "reconnect" after transient network issues and I'm guessing this is the issue.

@Stebalien
Copy link
Member

Stebalien commented Apr 11, 2024

To be clear, the lotus issue may be unrelated. But we definitely didn't introduce this feature to fix NATs. I'm now asking some users to disable reuseport to see if this is the issue.

@Stebalien
Copy link
Member

Eh, sorry for spamming this issue. I'm moving the conversation to a new issue while we figure out what's going on (libp2p/go-libp2p#2764),

@marten-seemann
Copy link
Contributor Author

marten-seemann commented Apr 11, 2024

Not my business anymore, but I'd be surprised if this was the issue.

  1. The connection hangs.

The connection shouldn't hang. The cryptographic handshake should fail.

@Stebalien
Copy link
Member

The connection shouldn't hang. The cryptographic handshake should fail.

Hm. You're right, both sides will be clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants