feat(gossipsub): allow compiling for WASM

[onur-ozkan]

Description

This modification removes deprecated dependency wasm_timer and enables wasm compatibility on the gossibsup protocol by simply substituting the wasm_timer::Instant with instant::Instant(which supports fn checked_add) and wasm_timer::Interval with futures_ticker::Ticker.

Notes

In my opinion, wasm-timer should be removed completely from the dependency tree since it's quite bad that relying on the dependency which is deprecated and no longer maintained. instant is also not so up-to-date dependency, but at very least it works today. But it might be broken someday as well if we make a change on standard library on rustc. Writing simple wrapper in the libp2p tree would be the most stable solution for this project since you can have complete control over it and update it whenever standard library adds or changes something.

[thomaseizinger]

In my opinion, wasm-timer should be removed completely from the dependency tree since it's quite bad that relying on the dependency which is deprecated and no longer maintained.

Agreed! Are you happy to send a PR for the remaining usages? (I haven't checked whether there actually are any.)

[thomaseizinger]

Thank you! One question.

[onur-ozkan]

Are you happy to send a PR for the remaining usages?

Certainly, I will check/work on that matter during the upcoming weekend.

[thomaseizinger]

Two more comments, otherwise LGTM :)

[thomaseizinger]

Thanks! I've pushed a few follow-up commits. Most notable, the crate is now included in our WASM build on CI!

If we are already working on WASM compatibility, I'd be great if we could also get rid of wasm_timer in this PR. Let me know if you are up for that. I think replacing Interval with futures_timer::Delay shouldn't be too difficult.

[onur-ozkan]

Most notable, the crate is now included in our WASM build on CI!

Awesome :)

I believed that shipping this would greatly benefit many people, so I directly opened the pull request for it. But I am also completely okay with replacing wasm_timer entirely in this PR. Since this PR is a blocker for one of our project, I will probably do it in couple days.

[thomaseizinger]

Most notable, the crate is now included in our WASM build on CI!

Awesome :)

I believed that shipping this would greatly benefit many people, so I directly opened the pull request for it. But I am also completely okay with replacing wasm_timer entirely in this PR. Since this PR is a blocker for one of our project, I will probably do it in couple days.

We will make a new release soon but it is likely a week or so away. This PR should easily make it in even if it takes a few more days before we can land it :)

[onur-ozkan]

I decided to use futures_ticker::Ticker as a replacement of wasm_timer::Interval. The reason I did that is that crate already using the dependencies we have in libp2p dependency tree. It implements the ticker functionality on top of futures_timer.

futures_timer has an open issue about merging futures_ticker. See async-rs/futures-timer#60

Also, it's worth to see this comment about futures_timer and futures_ticker #2497 (comment)

If futures_ticker will be merged into futures_timer, we can use futures_timer easily in libp2p by simply replacing a few keywords in the codebase.

Let me know if this is acceptable or not :)

[thomaseizinger]

Awesome!

I guess this means we can close #2497 with this PR then?

[thomaseizinger]

cc @mxinden @AgeManning In #2497, there was talk that the only remaining work would be to (re?) enable WASM support for gossipsub. This PR achieves this by means of https://github.com/antifuchs/futures-ticker.

[onur-ozkan]

I guess this means we can close #2497 with this PR then?

I think so.

Another way of solving this would be porting futures_ticker implementation in gossipsub. The implementation is super simple(~60 lines of code), and you can update it's dependencies(futures_timer, futures and instant) whenever you want. When you have those 3 dependencies updated in libp2p dependency tree, you will have multiple versions of the same dependency just because of futures_ticker.

[thomaseizinger]

I guess this means we can close #2497 with this PR then?

I think so.

Another way of solving this would be porting futures_ticker implementation in gossipsub. The implementation is super simple(~60 lines of code), and you can update it's dependencies(futures_timer, futures and instant) whenever you want. When you have those 3 dependencies updated in libp2p dependency tree, you will have multiple versions of the same dependency just because of futures_ticker.

If the dependencies aren't pinned, cargo should resolve them to the same version if they are semver compatible.#2497 was created because we initially had our own interval implementation and it was buggy. I'd like to not go back to that (even without a buggy implemention 😀 )

[mxinden]

Thanks for the work here. No objections from my end.

Note that futures-tick uses a "skip" style strategy, i.e. when one misses e.g. 2 ticks, only one is delivered. See also good documentation in tokio's Interval implementation.

Looking at futures-tick, I think there is room for improvement. See antifuchs/futures-ticker#10.

[thomaseizinger]

Note that futures-tick uses a "skip" style strategy, i.e. when one misses e.g. 2 ticks, only one is delivered. See also good documentation in tokio's Interval implementation.

I think that makes sense for us. This is used for heartbeats so I don't see a need for a burst strategy. Rather, performing the next heartbeat as soon as possible is important.

Do we agree?

[onur-ozkan]

I think that makes sense for us. This is used for heartbeats so I don't see a need for a burst strategy.

++

[mxinden]

Note that futures-tick uses a "skip" style strategy, i.e. when one misses e.g. 2 ticks, only one is delivered. See also good documentation in tokio's Interval implementation.

I think that makes sense for us. This is used for heartbeats so I don't see a need for a burst strategy. Rather, performing the next heartbeat as soon as possible is important.

Do we agree?

yes

[mxinden]

Thank you for the work here!

[mxinden]

and enables wasm compatibility on the gossibsup protocol

If you don't mind sharing, I would like to know what you are building with rust-libp2p and WASM @ozkanonur.

[AgeManning]

Sorry for the late input here.

As you've all pointed out, we had significant issues because in the past because there was a buggy implementation of interval. It's very important that the heartbeat stays consistent. It appears the ticker approach behaves like what we would need, in that if the software is over-loaded heartbeats are skipped, but should still occur at regular intervals.

We (Lighthouse) have no affinity to wasm-timer. So changes look good, provided the heartbeat works as expected :)

Approvals have been dismissed because the PR was updated after the send-it label was applied.

[onur-ozkan]

and enables wasm compatibility on the gossibsup protocol

If you don't mind sharing, I would like to know what you are building with rust-libp2p and WASM @ozkanonur.

We have a dex/swap protocol called https://github.com/KomodoPlatform/atomicDEX-API that utilizes libp2p for the network stack. Initially, it was written in C, but around 2019, we made the decision to rewrite it in Rust. As part of this rewrite, the network stack was migrated from nanomsg to libp2p, which involved creating custom patches for the libp2p library.

Today, I am rewriting the network layer of the project(KomodoPlatform/komodo-defi-framework#1756). This involves removing the forks and instead utilizing the original version of libp2p (the upstream). Throughout this process, I will likely come up with several more ideas and proposals to improve the libp2p upstream if I believe they would be beneficial.

[thomaseizinger]

As you've all pointed out, we had significant issues because in the past because there was a buggy implementation of interval. It's very important that the heartbeat stays consistent. It appears the ticker approach behaves like what we would need, in that if the software is over-loaded heartbeats are skipped, but should still occur at regular intervals.

This sounds like we should have a test for that :)

I'll create an issue.