Consider replacing with stardust? (I've rewritten ws-star, and this time it's good)

[mkg20001]

I've been working on a complete rewrite of ws-star

https://github.com/mkg20001/js-libp2p-stardust

This project, among other things:

• ...solves the memory leaks
• ...doesn't make the client sign arbitrary data
• ...is faster as it's no longer using socket.io
• ...completely and reliably works! 🎉

Note that while the code works, the APIs currently don't perfectly match those of the interface-transport spec because I literally "just made this"

I'd appreciate feedback and would like to see ws-star replaced with that. (After all I made this mess here so I might just clean it up as well)

[lidel]

The key pain point end users had with ws-star was "suiciding js-ipfs startup if any of ws-star servers is down". Does the rewrite plan to address that?

[jacobheun]

I will go through stardust either later today or tomorrow and put together some notes.

@lidel, we've got a ticket together for interface-transport to resolve the startup suicide for all transports. Some context over at #61 (comment). I'd like to get that done in the coming weeks. Once we have that, we'll be able to enforce it everywhere, including stardust once it's integrated with interface-transport.

[mkg20001]

The key pain point end users had with ws-star was "suiciding js-ipfs startup if any of ws-star servers is down". Does the rewrite plan to address that?

Kinda. I just hacked in a tiny change that allows passing a softFail: true option to make it silently ignore any failures during listening

But it also solves another bunch of problems:

• ws-star wasn't really doing back-pressure to stop the relay server from getting flodded with packets to deliver
• also using socket.io for muxing propably isn't a good idea (especially since it was a pull-stream rewrite of some part of a tool that I've written a long time ago before my code was actually anything close to "sane and stable")

[mkg20001]

Another thing: Can I move this thing to the libp2p org?

[mkg20001]

Note that currently stardust is using the p2p-websocket-star addresses. It would make sense to add p2p-stardust addresses. libp2p/js-libp2p-stardust#3
Would anyone from multiformats mind doing that?

[mkg20001]

Btw here is the roadmap to v0.1.0: libp2p/js-libp2p-stardust#1

Feel free to comment if something is missing

[jacobheun]

@mkg20001 I've gone through stardust and overall it's much easier to digest, it would be great to get that replacing websocket-star. I didn't do a thorough code review since that can probably wait till it's about ready to go.

In looking to replace websocket-star, it seems like right now you're looking to deprecate this in favor of stardust. Would it be better to just replace the code here? We'd want everyone to transition over, but the upgrade path looks like it will be pretty easy. What were your thoughts on making it a different module?

[mkg20001]

@jacobheun Mainly the fact that this is something completely new that in no way has any relation to the current ws-star protocol, aside from the functionality it provides.

ws-star is a socket.io based non-caching sort-of-but-not-really event-based muxer-disaster

stardust is a ws+multistream+protobuf based protocol that runs over an existing muxer and reuses some of libp2p's concepts in the microswitch sub-module it provides (it's kept simple for reasons of performance)

Effectively 0% of what currently is stardust was copied from ws-star, which also means that no smooth upgrade path is there (ok, not really 0%, this part - 6 lines was copied 😄 )

The reason for making them separate modules is to make the upgrades as smooth as possible: My idea would be to add stardust to the current js-ipfs, then warn everyone that they should switch to stardust ASAP and then completely get rid of ws-star in the next major version

[jacobheun]

@mkg20001 that sounds reasonable to me. We could also deploy an update to the rendezvous server with a warning message about it going away once we have a better idea of a target date for that, in addition to other warnings that people are more likely to see.

👍 for moving stardust under libp2p.

[mkg20001]

+1 for moving stardust under libp2p.

🎉 https://github.com/libp2p/js-libp2p-stardust

I've made myself the lead maintainer for this project for now. @libp2p/javascript-team Is this okay?

Also: I'll take some time this weekend to polish stardust. Should be ready soon

[mkg20001]

My potential plan: Getting this released in js-ipfs master ASAP, possibly even before v0.1.0

I know that the IPFS project can't just release half-working things into production. But it certainly can release them behind a flag and with a warning about their potential instability.

I'd rather give this thing a second pass after getting some feedback, instead of finalizing it instantly and making the same mistake as was done with websocket-star. And shortening the feedback-cycle is key to achieving that.

Opinions? Objections?

[dryajov]

👍 on short feedback loops - having it behind a flag makes lots of sense IMO.

[jacobheun]

An experimental flag rollout makes sense to me. @alanshaw is back next week, I'm sure he'd like to weigh in on the roll out to js-ipfs.

[mkg20001]

An experimental flag rollout makes sense to me.

@jacobheun Currently developing that in here: ipfs/js-ipfs#1812

Note that this depends on libp2p/js-libp2p-crypto#125 being finished first

[mkg20001]

Since #57 is an actual security problem and could likely only be addressed with a change that would break compatibility with previous clients, I'd suggest that we should look at deperacting ws-star and/or replacing it with stardust again, since I'm not seeing any progress in this issue.

[daviddias]

Parallel proposal: Sunset the *-star protocols 🌅

[mkg20001]

Ws-star is still used. Since stardust is now ready to use and feature-compatible, I wonder if it would be a great idea to use the refactor to make a switch, since that would allow us to get rid of the inefficient socket.io-based streaming-hack that ws-star uses.
Stardust would also include proper flow-control. Did I say proper? No, I meant any. That's why the servers had memory issues with ws-star.

[lidel]

@jacobheun @alanshaw @daviddias
I think we should really consider this.

Rationale

#57 is a real issue that could block adoption of js-libp2p if a thorough security audit is performed before we have p2p rendezvous protocol implemented (which I don't think will be production ready in next 6 months). We could hit this problem with Brave's security team (ipfs/ipfs-companion#716) at some point (I did not switch Brave to stardust yet only due to the lack of stable stardust server).

Migration path

Right now, we provide ws-star server at: /dns4/ws-star.discovery.libp2p.io/tcp/443/wss/p2p-websocket-star and should run this service as-is for at least a year. No change here.

If we go with this, there should be a stardust service at /dns4/ws-stardust.discovery.libp2p.io/tcp/443/wss/p2p-stardust. (Should we rename it to ws-star2 to make it easier for people to update configs and understand what stardust is about?).

It should be backward-compatible: /p2p-stardust is a separate namespace, so we could enable stardust in js-ipfs by default, but during the migration period also run legacy ws-star if a server with /p2p-websocket-star is defined in config at Addresses.Swarm.

Thoughts?

[alanshaw]

ws-star is not configured by default in js-ipfs. At this point I'd like to remove libp2p-websocket-star and libp2p-webrtc-star from the default bundle and update the documentation to empower users to configure libp2p with them as modules instead. They are being deprioritised in the async/await refactor because of libp2p/js-libp2p#385 and I'm not 100% sure they'll even be converted.

This would solve the security concern in Brave right? Can you also configure js-ipfs in Brave to use stardust? It doesn't have to be bundled by default in js-ipfs for you to use it right?

I've also said before that I'd be willing to document stardust on the js-ipfs README.

[mkg20001]

I've also said before that I'd be willing to document stardust on the js-ipfs README.

As long as adding transports is a possibility, it should work.

Also, de-bundling ws-star and replacing documentation about ws-star with stardust is IMO the best path forward.

If we go with this, there should be a stardust service at /dns4/ws-star2.discovery.libp2p.io/tcp/443/wss/p2p-stardust.

The server mentioned in the stardust readme is currently online & up-to-date. If protocol labs wants to run another, feel free to.

I suggest referring/renaming it to ws-star2 if possible to make it easier for people to update configs and understand what stardust is about

A DNS alias could help, but certainly don't want this service to be named ws-star2 primarly, since it's no longer socket.io based.

[jacobheun]

Overall I am in favor of replacing websocket-star with stardust. Refactoring websocket-star to async await is not worth the effort, and we don't yet have the appropriate peer discovery replacement that websocket-star gives us. Circuit Relay combined with better peer discovery mechanisms like Rendezvous, Peer Exchange, or possibly the DHT are more ideal approaches imo, but they're not ready yet.

Stardust does rely on RSA key encryption, so anyone using non RSA keys won't be able to use it. I believe there are some users not using RSA who are currently using websocket-star, so they would not be able to make this switch.

We currently have 3 servers for ws-star, but only really point to one. I think It would be reasonable to repurpose at least one of those for stardust, in addition to what @mkg20001 has already provided.

I have not done a full review of the latest stardust code. I think this is something we can do as part of the migration of that to async/await. @mkg20001 since that is part of the libp2p org we need to make sure to go through the normal PR process going forward.

[mkg20001]

Stardust does rely on RSA key encryption, so anyone using non RSA keys won't be able to use it

Isn't secp256k1 usable for enc/dec as well?
(https://crypto.stackexchange.com/questions/45040/can-elliptic-curve-cryptography-encrypt-with-public-key-and-decrypt-with-private)

I think this is something we can do as part of the migration of that to async/await

👍

since that is part of the libp2p org we need to make sure to go through the normal PR process going forward.

:nods:
I've mostly switched to it as well, except for a few quick fixes... 😅 (wasn't working before, so 🙌 )

[alanshaw]

Also, de-bundling ws-star and replacing documentation about ws-star with stardust is IMO the best path forward.

@mkg20001 would you be able to send me a PR?

[mkg20001]

would you be able to send me a PR?

Against current master, right?

[mkg20001]

@alanshaw Just realizing: Due to the way ws-star & stardust work, it's hard to add it without bundling it.

The user would have to first get the peer id, then create the stardust/ws-star instance and then add the transport and the discovery from the already instantiated instance.

This is a bit complex, especially getting the peer-id beforehand.

[alanshaw]

I believe this is why you can pass a function as libp2p config - it is passed the peer ID.

https://github.com/ipfs/js-ipfs#optionslibp2p

[mkg20001]

@alanshaw The problem with this is that it fully overrides the bundle. I just want to extend it. I've added a proposed solution to this in my pr.