Skip to content
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

Remove lightning-invoice std feature from client #3838

Open
elsirion opened this issue Dec 5, 2023 · 2 comments
Open

Remove lightning-invoice std feature from client #3838

elsirion opened this issue Dec 5, 2023 · 2 comments
Labels

Comments

@elsirion
Copy link
Contributor

elsirion commented Dec 5, 2023

Remove lightning-invoice std feature from client and test that it compiles independently (without the workspace pulling the feature in again). This might be more involved, so I split it out into a separate issue.

@TonyGiorgio anything else we can do that would help you guys with WASM compatibility?

@TonyGiorgio
Copy link
Contributor

TonyGiorgio commented Dec 5, 2023

You guys have really good wasm32 practice already, so not much more. I think the weirdness mostly comes when using a library like LDK that pulls in rust-bitcoin std, or trying to go no-std and uses rust-bitcoin no-std and then other things break.

I hear this recent merge will help LDK work around it: rust-bitcoin/rust-bitcoin#2066

It might be required unless you also want to move to rust-bitcoin no-std as well.

We've tried to get a couple things into LDK like swapping the now() calls with wasm32 capable calls or removing them from wasm32 but they are all rejected: lightningdevkit/rust-lightning#2560 which is why we run with this patch: MutinyWallet/rust-lightning@a992512

though as I noted in #3839 (comment) we might just have to loosen up a couple of the not(wasm32) flags we added so we can attempt to use some of the lightning-invoice calls you guys are using.

In the end, LDK's std flag is not safe to run in wasm32 and I think their preferred solution is to only allow that use case in no-std with the ability to eventually allow rust-bitcoin std as an option.

Maybe @TheBlueMatt can chime in here. Another option is to just rip out lightning-invoice if you just need invoice parsing since it's heavily reliant upon the rust-lightning ecosystem and their practices.

@TheBlueMatt
Copy link

rust-bitcoin/rust-bitcoin#2066 teed rust-bitcoin up for rust-bitcoin/rust-bitcoin#2233 which will let the ecosystem clean up the no-std crap a ton. That will mean you can just enable std on the ecosystem crates that need it, and the std/no-std flags wont be "viral" across the BDK/LDK/rust-bitcoin ecosystem.

This will make default-features = false no longer annoying to deal with, as its otherwise best practice for pulling in any dependency, especially in a library crate, and wont imply std. Of course, if you're expecting downstream crates to pick std/no-std directly on ldk/rust-bitcoin you should still set default-features = false. You should also be able to set no-std on the lightning-invoice crate if you dont need any of the std features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants