feat: 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

[dariusc93]

Although libp2p-relay was updated first, I think it would be best to also add an entry for this change for tracking since 0.17.2 was already released. Thoughts @jxs ?

[jxs]

yeah makes sense Darius thanks!

[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?

[mergify]

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

[dariusc93]

Hmm Im not sure what is causing the transport interoperability test to continue to fail (been that way since upgrading to web-time). Could be that test-plans need updating too?

[jxs]

Hmm Im not sure what is causing the transport interoperability test to continue to fail (been that way since upgrading to web-time). Could be that test-plans need updating too?

yeah seems to be failing, did you see https://github.com/libp2p/rust-libp2p/actions/runs/9191874572/job/25279212100?pr=5347#step:5:1133 seems to not be able to connect

[oblique]

Will this be included in the release?

About the connection failure: Can you check if this is happening if you revert into specifying Chrome's version?