Don't call system time in no-std

[benthecarman]

When calling system time in wasm it causes a panic. Need to gate this behind build flags

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4deb263) 88.46% compared to head (f836794) 88.42%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2799      +/-   ##
==========================================
- Coverage   88.46%   88.42%   -0.05%     
==========================================
  Files         114      114              
  Lines       91806    91806              
  Branches    91806    91806              
==========================================
- Hits        81214    81176      -38     
- Misses       8112     8140      +28     
- Partials     2480     2490      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I think this is not so much about supporting no_std but really about just not panicking in WASM. While I'm not against this change, it's a bit confusing to introduce selective std feature-gates in a crate not (currently) aiming to support no_std.

So if we want to do this it might be nice to:

a) leave a short comment explaining the motivation for the feature gate and why we only feature-gate Instant while the other std imports are fine.
b) Change the commit message to clarify this is about WASM, not no_std.
c) Do this also for ElectrumSyncClient, which will have the same issue.

[TonyGiorgio]

This is incredibly frustrating that this keeps coming up time and time again. There has been multiple attempts at fixing wasm issues in LDK and they're all shot down: #1865 #2497 #2560

At this point I'm just wondering if wasm32 is something you even care about supporting at all and if we should move off if it'll continue to be neglected. We've been dealing with this for over a year and still waiting for the @TheBlueMatt approved "proper" way for it to be done. IMHO, there's only a few of us wasm people and we've brought it up and ignored it. So we're either going to maintain and promote a wasm32 compatible fork going forward or we merge something into LDK so every wasm32 project pulling in any LDK project can actually safely pull in the library until you guys feel like the "proper" fix can get in place.

[tnull]

This is incredibly frustrating that this keeps coming up time and time again. There has been multiple attempts at fixing wasm issues in LDK and they're all shot down: #1865 #2497 #2560

At this point I'm just wondering if wasm32 is something you even care about supporting at all and if we should move off if it'll continue to be neglected. We've been dealing with this for over a year and still waiting for the @TheBlueMatt approved "proper" way for it to be done. IMHO, there's only a few of us wasm people and we've brought it up and ignored it. So we're either going to maintain and promote a wasm32 compatible fork going forward or we merge something into LDK so every wasm32 project pulling in any LDK project can actually safely pull in the library until you guys feel like the "proper" fix can get in place.

Hm, sorry to hear about your frustration, but I didn't shut this down? I merely remarked that the motivation given in the PR/commit message doesn't exactly match the change, which confused me at first. We also at least should leave a comment to ensure these feature-gates are not eventually cleaned up again, as they for themselves seem superfluous and misplaced in a crate that doesn't aim to/can't (?) support no_std. So we need to explain the context to avoid running into the same issue again in the future.

[tnull]

FWIW, for consistency with other crates we might also want to consider feature-gating this behind feature = "no-std" rather than not(feature = "std"). Not entirely sure if it would warrant introducing the feature just for this though.

[TonyGiorgio]

Hm, sorry to hear about your frustration, but I didn't shut this down?

Sorry, I was not meaning for my comment to be about your reply. I'm commenting my general frustrations about how this keeps happening and that these tiny little bandaid fixes are not enough to prevent and mitigate this from continuously happening.

[tnull]

Hm, sorry to hear about your frustration, but I didn't shut this down?

Sorry, I was not meaning for my comment to be about your reply. I'm commenting my general frustrations about how this keeps happening and that these tiny little bandaid fixes are not enough to prevent and mitigate this from continuously happening.

No worries!

Hmm, @TheBlueMatt, any reason why we shouldn't add a dedicated wasm32 target to CI to ensure we maintain compatibility going forward?

[benthecarman]

The problem is that it isn't a compile time issue, it is a runtime issue. So it is hard to catch. You can use the instant library for getting time and it'll handle it for you, but that has its tradeoffs of introducing a new dep. What we do in mutiny is only use the dep if we are on the build target which works nicely.

https://github.com/MutinyWallet/mutiny-node/blob/master/mutiny-core/src/utils.rs#L41

[tnull]

The problem is that it isn't a compile time issue, it is a runtime issue. So it is hard to catch.

Sure, it would depend on our coverage, but for example in this instance we would have hit this issue if we also ran our lightning-transaction-sync tests on a wasm32 target. And we could of course make sure to add regression tests if we would hit another issue again.

[moneyball]

It definitely seems like a good idea to improve our testing so that we catch this while preparing a release rather than waiting for our users to discover it.

[TheBlueMatt]

This is incredibly frustrating that this keeps coming up time and time again. There has been multiple attempts at fixing wasm issues in LDK and they're all shot down: #1865 #2497 #2560

Indeed, when we've gone through this in the past, I really wanted to support wasm by simply using no-std - that guarantees we don't have some time access slip in because it simply won't compile. That's obviously the "right" way to do it from the rust perspective, but admittedly I didn't anticipate the more LDK and rust-bitcoin-specific issues that ended up cropping up. Instead, setting no-std on LDK forces no-std on other Rust ecosystem crates and has led to downstream (and upstream) dependencies failing to compile instead.

Luckily, and finally, that is coming to an end with the next rust-bitcoin release, where no-std on LDK will be wholly unrelated to no-std on rust-bitcoin or any other ecosystem crate. However, that is still a number of months away, both in terms of rust-bitcoin cutting a new release and us upgrading to it (which may or may not be tied to BDK doing the same, so that there aren't other incompatibility reasons for users to hit issues).

In the mean time I'm not sure what to do - we can either continue to support this via no-std (with the other dependency breakages we've suffered through) or we could do it via std. We've ripped time fetching out of the scorer in the latest release, so the only remaining reference in the no-std-supporting crates is in outbound payments, where the only call is in case the Retry::Timeout enum variant is used. So, in theory y'all could use the std version as well at this point. Let me know what's easier for you and we're happy to support it, at least until we get to the new rust-bitcoin and can make LDK use no-std standalone, where it should be a great solution for y'all.

This new issue is somewhat separate - admittedly none of us really thought about supporting electrum/esplora via the transaction-sync crate on wasm, after all Rust can't open sockets. I'd forgotten that you can make HTTP calls via the request javascript wraparound crap, so it wasn't even on my radar. Apologies that it slipped through.

In the short term, we should probably, indeed, support it via this approach for now, and I don't read @tnull's comments as saying absolutely not, but rather we should use a separate feature rather than std/no-std, which mean something different than this in Rust, and we should document it. Ideally we should have some method of testing this, but I'm not really sure what to do about it (you can always have some call slip through that isn't hit in tests, hence why the no-std approach is much simpler).

Eventually, we want to rip out reqwest and use something where we can explicitly support WASM calls both for y'all and our JS bindings, though at this point its looking increasingly like that means building our own HTTP library, which may take a bit of time.

At this point I'm just wondering if wasm32 is something you even care about supporting at all and if we should move off if it'll continue to be neglected. We've been dealing with this for over a year and still waiting for the @TheBlueMatt approved "proper" way for it to be done. IMHO, there's only a few of us wasm people and we've brought it up and ignored it. So we're either going to maintain and promote a wasm32 compatible fork going forward or we merge something into LDK so every wasm32 project pulling in any LDK project can actually safely pull in the library until you guys feel like the "proper" fix can get in place.

Its been quite frustrating, and understandably so. I'll note that we absolutely do care about wasm, and spent the time to add an io crate to rust-bitcoin to unblock the "everything is either std or not" issue, as well as removed the scorer time-fetching in the latest release to make this less of an issue if y'all use std. Sadly, wasm wasn't something we were thinking about here because it uses HTTP/sockets so generally were assuming this crate simply wouldn't work. I'd be happy to see it support wasm, though long-term we should find a way to do that that isn't relyant on wasm_bindgen.

[TonyGiorgio]

Appreciate the thorough response.

coming to an end with the next rust-bitcoin release, where no-std on LDK will be wholly unrelated to no-std on rust-bitcoin or any other ecosystem crate

We're on std now with an LDK fork and at this point I don't want to go back to LDK no-std unless the only difference is no SystemTime::now() calls, but even then, we do have the ability to use time and we'd rather have the ability to use time in a way that other projects using browser wasm32 runtimes, which is either wasm_timer or instant, or at least allow us to pass in a function to LDK whenever now needs to be called and we'll do it ourselves without it dirtying up ldk dependencies if that's the main concern. Though environment/feature specific flags are trivial to support in rust projects since they often use identitcal names for std::SystemTime::* and can be solved with four lines in the import statement (or single utility file for calling time everywhere).

We've ripped time fetching out of the scorer in the latest release, so the only remaining reference in the no-std-supporting crates is in outbound payments .. So, in theory y'all could use the std version as well at this point.

Other wasm32 projects pull in lightning-invoice in std and are bound to run into the other instances of SystemTime::now() panics that exist there too, like what's happening in Fedimint as well. There either needs to be big red flags (or even go as far as panics) that wasm32 is not supported in std or this needs to be fixed there too. I'd rather the later, because if you want lightning-invoice to be a common crate across the rust ecosystem, it's going to get incredibly problematic for us to pull in another bitcoin library into our project ever again.

Ideally we should have some method of testing this, but I'm not really sure what to do about it

Semgrep seems to work rather well for the Fedimint team: https://github.com/fedimint/fedimint/blob/master/.config/semgrep.yaml

[justinmoon]

From our experience in Fedimint, it is extremely easy for system time calls to slip into a codebase and only blow up at runtime when first developer tries running in WASM.

We try to address this in 2 ways:

• Semgrep rules as @TonyGiorgio mentions. Our CI and git pre-commit hooks just completely ban usage of functions like std::time::SystemTime::now and instead demands usage of fedimint_core::time::now which work on WASM.
• A basic test suite that runs in firefox. This doesn't have nearly as much coverage as we'd like, but has caught a few regressions.

@dpc setup first one, @maan2003 setup the second one

[Kixunil]

FWIW, for consistency with other crates we might also want to consider feature-gating this behind feature = "no-std" rather than not(feature = "std"). Not entirely sure if it would warrant introducing the feature just for this though.

Happened to read this out of curiosity, this would be a very bad idea because features are additive and the whole Rust ecosystem assumes that. Even something as silly as having a no-std feature really meaning "alloc and core" caused immense pain that was recently solved in bitcoin. So that's a very strong do not recommend from me.

[benthecarman]

Changed this to be gated behind a time feature, would be nice if the rest of ldk was like this instead of the std vs no-std it currently is

[TheBlueMatt]

We're on std now with an LDK fork and at this point I don't want to go back to LDK no-std unless the only difference is no SystemTime::now() calls, but even then, we do have the ability to use time and we'd rather have the ability to use time in a way that other projects using browser wasm32 runtimes, which is either wasm_timer or instant, or at least allow us to pass in a function to LDK whenever now needs to be called and we'll do it ourselves without it dirtying up ldk dependencies if that's the main concern. Though environment/feature specific flags are trivial to support in rust projects since they often use identitcal names for std::SystemTime::* and can be solved with four lines in the import statement (or single utility file for calling time everywhere).

In general, the reason for wanting to support wasm via no-std, rather than some wasm-specific tooling is that there is more than just system time that is not supported on wasm (system time just happens to be the one that is commonly broken). Things like threading, sockets, etc all break if we start to rely on them anywhere.

I'm curious why you're on std - since y'all have been using wasm, we've cut back substantially on the set of things we use std for, in part to better support y'all, and at this point IIRC almost the only one left in the main lightning crate is the Retry::Timeout enum variant (which is somewhat easy to work around, you just have to abandon_payment when you want to time out, even if that's a bit of work). Is it just a historical artifact that you're still using std. Can we drop that for no-std now?

Other wasm32 projects pull in lightning-invoice in std and are bound to run into the other instances of SystemTime::now() panics that exist there too, like what's happening in Fedimint as well. There either needs to be big red flags (or even go as far as panics) that wasm32 is not supported in std or this needs to be fixed there too. I'd rather the later, because if you want lightning-invoice to be a common crate across the rust ecosystem, it's going to get incredibly problematic for us to pull in another bitcoin library into our project ever again.

I'm curious why fedimint is using the std flag on lightning-invoice? I know you opened fedimint/fedimint#3838 but there doesn't seem to be an answer there - do they actually use any of the std-specific features? Again, like with the main lightning crate, I'm pretty hesitant to suggest we will/want to support wasm builds with std, its really not how rust stdlib is set up. If there's a pressing need to support both std and wasm we can revisit that, but I'd like to have a clear motivating reason before we jump to supporting something that rustc isn't really set up to support.

[TonyGiorgio]

I'm curious why you're on std

We have to be, because we're pulling in other libraries that use lightning in std. We're not waiting years to get this resolved.

I'm curious why fedimint is using the std flag on lightning-invoice

They have to be because they're using rust-bitcoin in std. This goes all the way back to how how rust-lightning and rust-bitcoin are intertwined with std vs no-std.

[TheBlueMatt]

We have to be, because we're pulling in other libraries that use lightning in std. We're not waiting years to get this resolved.
They have to be because they're using rust-bitcoin in std. This goes all the way back to how how rust-lightning and rust-bitcoin are intertwined with std vs no-std.

Right, okay. So I think we're on the same page, then. For now, we'll continue to avoid any new time calls in std, with the removal in scoring in 119 we should be basically fine in the main crate. Once we get to rust-bitcoin 0.32 we'll no longer have the whole std/no-std lock-step nonsense and can relax that, though of course that may be a while.

[benthecarman]

How LDK handles wasm32 stuff aside, this was a regression. How does this changeset look for fixing?

[TheBlueMatt]

this was a regression

Its not a regression if its new code :)

This LGTM.

[tnull]

LGTM, introducing a dedicated time feature seems fine provided we'll want to drop no-std eventually.