deps: use web-time instead of instant

[dariusc93]

Description

See sebcrozet/instant#52

Notes & open questions

Change checklist

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• A changelog entry has been made in the appropriate crates

[jxs]

Thanks for this Darius! Only one remark

[jxs]

LGTM

[zvolin]

There is also a usage of std::time in protocols/kad/src/kbucket.rs and misc/memory-connection-limits/src/lib.rs that should be replaced with wasm-compatible time implementation. Could you handle those in scope of this PR?

[dariusc93]

There is also a usage of std::time in protocols/kad/src/kbucket.rs and misc/memory-connection-limits/src/lib.rs that should be replaced with wasm-compatible time implementation. Could you handle those in scope of this PR?

Hi. In kbucket.rs, Instant is used in the test so while we could change it to use web-time, it would not affect anything unless we want to try running test in wasm environment so I could make the change there. For memory-connection-limits, that crate is not compatible with wasm due to memory-stats not supporting wasm (though might be possible in the future - see Arc-blroth/memory-stats#1) so it would not be required to make changes there.

[zvolin]

The issue with kbucket is that it's inner modules use use super::*; and leaks std::time into themselves like here. This isn't easy to spot because that wildcard import, ideally it should be removed too to prevent such surprises in future

[dariusc93]

The issue with kbucket is that it's inner modules use use super::*; and leaks std::time into themselves like here. This isn't easy to spot because that wildcard import, ideally it should be removed too to prevent such surprises in future

Nice catch on that!

[zvolin]

sorry for the offtop, but do you happen to know if there is any plan for the release which will include this? Or maybe some backport for 0.53? It crashes our node occasionally and since we're on crates already I'd like to avoid git dependencies

[dariusc93]

sorry for the offtop, but do you happen to know if there is any plan for the release which will include this? Or maybe some backport for 0.53? It crashes our node occasionally and since we're on crates already I'd like to avoid git dependencies

I can imagine a patch release can be done for majority of the crates here while others like kad may fall under 0.54. A PR could be done to backport the web-time change too

[zvolin]

For memory-connection-limits, that crate is not compatible with wasm

mdns would not be compatible with wasm

By the way, I included those without checking if they indeed support wasm, but my thinking is that it doesn't hurt to add it, eventually it may end up being useful and it's probably much easier to maintain if every Instant usage happens through wasm compatible crate. You don't need to wonder 'should it work for wasm?' and rather you see Instant in PR then just check if it is through web-time.

[mergify]

This pull request has merge conflicts. Could you please resolve them @dariusc93? 🙏

[jxs]

sorry @dariusc93 can you rebase again one last time?

[oblique]

So after some hours of investigation I found why CI was failing. The instant/wasm-bindgen is still needed to be defined in libp2p/Cargo.toml because two dependencies were still using it:

• yamux - Opened PR: feat: Use web-time instead of instant rust-yamux#191
• libp2p-tls - This was a mistake in Cargo.toml which made the libp2p-tls to use libp2p-core only from crates.io. Opened a PR with a fix: chore: Use libp2p-tls from workspace #5452

After the above PRs are merged and a new yamux is released, the CI will pass.

[dariusc93]

So after some hours of investigation I found why CI was failing. The instant/wasm-bindgen is still needed to be defined in libp2p/Cargo.toml because two dependencies were still using it:

* `yamux` - Opened PR: [feat: Use `web-time` instead of `instant` rust-yamux#191](https://github.com/libp2p/rust-yamux/pull/191)

* `libp2p-tls` - This was a mistake in `Cargo.toml` which made the `libp2p-tls` to use `libp2p-core` only from crates.io. Opened a PR with a fix: [fix: Use `libp2p-tls` from workspace #5452](https://github.com/libp2p/rust-libp2p/pull/5452)

After the above PRs are merged and a new yamux is released, the CI will pass.

Interesting but good catch. Im curious on why there wasnt an error with yamux at runtime when it attempts to use Instant under usual wasm testing.

[oblique]

Im curious on why there wasnt an error with yamux at runtime when it attempts to use Instant under usual wasm testing.

I think because of these two reasons:

• wasm_ping binary of interop-tests does not fetch any errors that might happen.
• run_test_wasm function of interop-tests does not set console_error_on_panic hook, so even if you check browser's console, you will not see an error.

[oblique]

@jxs Isn't it better to update libp2p-yamux/Cargo.toml instead of just Cargo.lock?

[jxs]

@jxs Isn't it better to update libp2p-yamux/Cargo.toml instead of just Cargo.lock?

sorry Yianis, I did not understand, van you elaborate?

[oblique]

Now yamux dependency is only updated in Cargo.lock to the 0.13.3 version and user can still get the non-web-time one because Cargo.toml is not updated.

I'm saying we should do this change:

diff --git a/muxers/yamux/Cargo.toml b/muxers/yamux/Cargo.toml
index ddb942489..4d6d655e3 100644
--- a/muxers/yamux/Cargo.toml
+++ b/muxers/yamux/Cargo.toml
@@ -16,7 +16,7 @@ futures = { workspace = true }
 libp2p-core = { workspace = true }
 thiserror = "1.0"
 yamux012 = { version = "0.12.1", package = "yamux" }
-yamux013 = { version = "0.13.1", package = "yamux" }
+yamux013 = { version = "0.13.3", package = "yamux" }
 tracing = { workspace = true }

 [dev-dependencies]

[mergify]

This pull request has merge conflicts. Could you please resolve them @dariusc93? 🙏

[jxs]

Now yamux dependency is only updated in Cargo.lock to the 0.13.3 version and user can still get the non-web-time one because Cargo.toml is not updated.

I'm saying we should do this change:

diff --git a/muxers/yamux/Cargo.toml b/muxers/yamux/Cargo.toml
index ddb942489..4d6d655e3 100644
--- a/muxers/yamux/Cargo.toml
+++ b/muxers/yamux/Cargo.toml
@@ -16,7 +16,7 @@ futures = { workspace = true }
 libp2p-core = { workspace = true }
 thiserror = "1.0"
 yamux012 = { version = "0.12.1", package = "yamux" }
-yamux013 = { version = "0.13.1", package = "yamux" }
+yamux013 = { version = "0.13.3", package = "yamux" }
 tracing = { workspace = true }

 [dev-dependencies]

ah right, thanks!

[jxs]

thanks for the initiative @dariusc93 and @oblique for investing time into debugging <3