-
Notifications
You must be signed in to change notification settings - Fork 1k
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
protocols/gossipsub: Revert back to wasm_timer for interval #2506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for reverting. Could you include a changelog entry as well?
Also I think this needs to use wasm_timer::Instant
instead of instant::Instant
.
//CC @wngr
cc0ea7c
to
7b4dceb
Compare
#2245 made the |
I've removed support for gossipsub in the wasm targets. It's sad since before #2245 there was support for the other two targets, but the |
Following @mxinden suggestion, Gossipsub should now be removed only in the > cargo tree -e features --target=wasm32-unknown-unknown | rg gossipsub
# empty
> cargo tree -e features --target=wasm32-wasi | rg gossipsub
#├── libp2p-gossipsub feature "default"
#│ └── libp2p-gossipsub v0.36.0 (/home/me/rust-libp2p/protocols/gossipsub)
#│ ├── libp2p-gossipsub feature "default" (*) # libp2p-metrics dep
> cargo tree -e features --target=wasm32-unknown-emscripten | rg gossipsub
#├── libp2p-gossipsub feature "default"
#│ └── libp2p-gossipsub v0.36.0 (/home/me/rust-libp2p/protocols/gossipsub)
#│ ├── libp2p-gossipsub feature "default" (*) # libp2p-metrics dep |
Thanks @divagant-martian and @AgeManning! |
…ibp2p#2506)" This reverts commit 60666f5.
…ibp2p#2506)" This reverts commit 60666f5.
…ibp2p#2506)" This reverts commit 60666f5.
Move the `libp2p` dependency to a fork to enable compilation for `wasm` and non-`wasm` targets. This is due to the fact that the `gossipsub` protocol got the `wasm` support dropped in this [PR](libp2p/rust-libp2p#2506).
Move the `libp2p` dependency to a fork to enable compilation for `wasm` and non-`wasm` targets. This is due to the fact that the `gossipsub` protocol got the `wasm` support dropped in this [PR](libp2p/rust-libp2p#2506).
As described in #2497 the current implementation for Interval in gossipsub can cause some serious issues with how gossipsub functions.
The heartbeat interval can grow unbounded, which unbounds the memory cache, exploding RAM and causes messages to be gossiped much later than they should be which can lead to very old messages being re-propagated on networks.
This PR reverts the Interval implementation back to
wasm_timer
. This should be sufficient to get gossipsub to work as expected however it was noted that this is not as performant as a straighttokio::time::Interval
. A future PR should be made that ports the interval to betokio
agnostic, yet equally performant and compatible with WASM on NodeJS.