lightningdevkit / ldk-node

A ready-to-go node implementation built using LDK.
Other
148 stars 78 forks source link

Make `openssl` dependency truly optional #254

Closed elsirion closed 8 months ago

elsirion commented 8 months ago

Currently, vss is hidden behind a cfg() switch and not a feature, which means that it has to be considered for dependency resolution and its transitive dependencies, like openssl, will end up in Cargo.lock. This is necessary since, even though we know we'll never want to compile it, cargo can't tell this.

Since openssl is a pain to deal with we ban it in Fedimint CI by grepping Cargo.lock, which currently blocks upgrading to ldk-node 0.2 (https://github.com/fedimint/fedimint/pull/4354) and would be quite a bit of work to special-case to allow openssl onyl as a transitive dependency of ldk-node (since we know we'll never compile it there).

The resolutions that seem reasonable to me are:

I'm happy to open a PR, just want to get your feedback first.

tnull commented 8 months ago

So VSS support is currently just cfg-gated because it's not fully ready yet. This will however be dropped soon. We also won't hide it behind a feature as we want to make it configurable via the Builder during runtime, and especially since our bindings don't have access to the Rust features.

Or if you are ok with it maybe even defaulting to rustls in vls_client (that's what we do with Fedimint)

We actually do default to rustls, but the issue might be that VssClient doesn't set default-features = false for reqwest, which should be fixed with this: https://github.com/lightningdevkit/vss-rust-client/pull/27

Note that openssl is not an actual dependency of VssClient and would only be build when a specifc cfg flag is enabled.

G8XSU commented 8 months ago

Patched vss-client 0.2.2 should be available now with above fix.

tnull commented 8 months ago

Just confirmed LDK Node's Cargo.lock makes no mention of openssl anymore. Closing as completed.