-
Notifications
You must be signed in to change notification settings - Fork 275
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
Comments
I am in favor of deprecating simopen.
Implemented, but never merged or released. See libp2p/rust-libp2p#2066. |
Afaik I don't think js-libp2p supports simopen (couldn't find |
It's gone! Bye! |
Sorry, missed this - it's not implemented in js-libp2p as @p-shahi found 👍 |
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:
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. |
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. |
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), |
Not my business anymore, but I'd be surprised if this was the issue.
The connection shouldn't hang. The cryptographic handshake should fail. |
Hm. You're right, both sides will be clients. |
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:
I don't think any other implementation ever bothered to actually implement it. Is that correct, @mxinden @achingbrain?
The text was updated successfully, but these errors were encountered: