Replace broadcast channels and refactor websocket streaming

[austinabell]

Summary of changes
Changes introduced in this pull request:

• Swaps flo-stream for tokio broadcast channels
  • Investigate flo-stream usage #700 the broadcast channels have the best API for our need which is production ready and async
• Refactors the websocket streaming support
  • Logic was very convoluted and janky, cleans up usage and sets up to be expanded on without bottlenecks
  • Previously was sending every event to every subscriber then matching and checking ids to filter out the events. This had large flaws and was not a great pattern.

Reference issue to close (if applicable)

Closes

Other information and links

[cryptoquick]

Not to hold this up, but have you considered using tokio-tungstenite? You're mixing concurrency execution engines.

Also, maybe consider using a feature to support parallel implementations for async-std, tokio, and maybe even smol. It's not a huge deal, but someday, someone might want to use an embedded Filecoin implementation... 😉

[cryptoquick]

Obviously it's a nitpick, so it shouldn't hold up the merge. But, some more thoughts on the crate feature. Usually, I'd say, "it's better to pick one", because it'd simplify testing, but also, in my experience, it shouldn't be that big a deal since Rust has so many correctness guarantees. "Fearless concurrency", after all. Further, when there are non-obvious issues with concurrency, it'd be very nice to have a parallel implementation, just to for debugging and benchmarking purposes.

[austinabell]

Not to hold this up, but have you considered using tokio-tungstenite? You're mixing concurrency execution engines.

Also, maybe consider using a feature to support parallel implementations for async-std, tokio, and maybe even smol. It's not a huge deal, but someday, someone might want to use an embedded Filecoin implementation... 😉

It's fine because of the tokio 1.0 support in async-std. I don't like mixing these either, but Tokio has the broadcast channel that works by far the best for our need. This is not a PR we are pulling in quickly and going to explore options more before pulling in. Also, we use async-std as our runtime, so I'd rather not use more Tokio things than needed.

Obviously it's a nitpick, so it shouldn't hold up the merge. But, some more thoughts on the crate feature. Usually, I'd say, "it's better to pick one", because it'd simplify testing, but also, in my experience, it shouldn't be that big a deal since Rust has so many correctness guarantees. "Fearless concurrency", after all. Further, when there are non-obvious issues with concurrency, it'd be very nice to have a parallel implementation, just to for debugging and benchmarking purposes.

The feature gets applied to everything in the workspace, I just don't want the case where tokio support is removed and it's forgotten to remove the flag from crates that never used/ needed. Do I understand your point correctly?

[cryptoquick]

Not to hold this up, but have you considered using tokio-tungstenite? You're mixing concurrency execution engines.
Also, maybe consider using a feature to support parallel implementations for async-std, tokio, and maybe even smol. It's not a huge deal, but someday, someone might want to use an embedded Filecoin implementation... 😉

It's fine because of the tokio 1.0 support in async-std. I don't like mixing these either, but Tokio has the broadcast channel that works by far the best for our need. This is not a PR we are pulling in quickly and going to explore options more before pulling in. Also, we use async-std as our runtime, so I'd rather not use more Tokio things than needed.

Honestly, there are other reasons to want a parallel Tokio implementation. Tokio has gotten better and has more momentum and adoption in the broader Rust ecosystem. If you look into async-std, there are some concerns about ongoing maintenance moving forward. If it's easy to support both behind a feature flag and crate attribute macros, then I'd recommend at least exploring them, to better compare.

Obviously it's a nitpick, so it shouldn't hold up the merge. But, some more thoughts on the crate feature. Usually, I'd say, "it's better to pick one", because it'd simplify testing, but also, in my experience, it shouldn't be that big a deal since Rust has so many correctness guarantees. "Fearless concurrency", after all. Further, when there are non-obvious issues with concurrency, it'd be very nice to have a parallel implementation, just to for debugging and benchmarking purposes.

The feature gets applied to everything in the workspace, I just don't want the case where tokio support is removed and it's forgotten to remove the flag from crates that never used/ needed. Do I understand your point correctly?

Not quite, I mean, for supporting both at the same time, just behind separate feature flags, like, for example, in these projects (it's somewhat common in the Rust community for libraries). For an example that comes to mind, see:

https://github.com/hecrj/iced/blob/8d882d787e6b7fd7c2435f42f82933e2ed904edf/Cargo.toml#L38-L45
https://github.com/hecrj/iced/blob/8d882d787e6b7fd7c2435f42f82933e2ed904edf/futures/src/lib.rs#L19-L35

That said, I'd scope the feature to just the two, async-std and tokio, for now.

[austinabell]

Honestly, there are other reasons to want a parallel Tokio implementation. Tokio has gotten better and has more momentum and adoption in the broader Rust ecosystem. If you look into async-std, there are some concerns about ongoing maintenance moving forward. If it's easy to support both behind a feature flag and crate attribute macros, then I'd recommend at least exploring them, to better compare.

We've obviously considered Tokio, but we have had a better experience with async-std as a default, and isn't a priority to switch.

Not quite, I mean, for supporting both at the same time, just behind separate feature flags, like, for example, in these projects (it's somewhat common in the Rust community for libraries). For an example that comes to mind, see:

https://github.com/hecrj/iced/blob/8d882d787e6b7fd7c2435f42f82933e2ed904edf/Cargo.toml#L38-L45
https://github.com/hecrj/iced/blob/8d882d787e6b7fd7c2435f42f82933e2ed904edf/futures/src/lib.rs#L19-L35

That said, I'd scope the feature to just the two, async-std and tokio, for now.

Ah yeah I see what you're saying, and there isn't a solid reason for a binary like ours to support both. It makes sense for that and other libs because they want to allow users to use whatever runtime they want with it, but our priority is on the actual forest binary, and the crates of ours that are used do not use or assume a runtime. Maybe in the future, but I doubt there will be a concrete need for this.

[cryptoquick]

I'll take it. That said, I wouldn't shut the door on this question just yet. There are certainly use cases, such as for a filecoin_embed. It's further off, but I also like to think of how end users of this project would likely want from it. Sure, feature parity with lotus is very important, there's also no reason why this project couldn't also do more than strictly necessary, all things being equal.

[ec2]

Everything else looks great. Just two small changes i noted:
FIrst thing to send back to a ChainNotify is {"jsonrpc":"2.0","result":<CHANNEL NUMBER>,"id":0}.
Second thing to send back is a HeadChange::Current event.
Then all other HeadChange events that come through the subscription.