bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Enable selecting use-rustls-ring feature on electrum client

Open thunderbiscuit opened this issue 1 year ago • 1 comments

This PR is a companion to bitcoindevkit/rust-electrum-client#135. It enables choosing the ring dependency on rustls instead of the new default (as of 0.23.0) aws-lc-rs. The AWS dependency breaks the Android and Swift builds. I wrote a more detailed explanation on #135.

Let me know if there is a better way to do this. The core issue is that even if I enable the ring feature on rustls, it still compiles with the default features, and the compilation step fails at the aws-lc-rs bindings generation.

Notes to the reviewers

Do not merge before #135 is merged and the dependency points to the new version of the client rather than my fork of it.

Changelog notice

Added
    - bdk_electrum now enables choosing either the `use-rustls` or `use-rustls-ring` feature

Checklists

All Submissions:

  • [x] I've signed all my commits
  • [x] I followed the contribution guidelines
  • [x] I ran cargo fmt and cargo clippy before committing

thunderbiscuit avatar Jun 26 '24 15:06 thunderbiscuit

Quick update @oleonardolima: I added a default feature that in turn uses the rustls default features on the electrum client. This way this PR doesn't break anyone's code while offering the option for ring.

thunderbiscuit avatar Jun 29 '24 12:06 thunderbiscuit

Is it possible to set up the features so that you don't have to set default-features = false, when you want use-rustls-ring?

Not a big deal but a little nicer for the user

praveenperera avatar Jul 21 '24 21:07 praveenperera

Is it possible to set up the features so that you don't have to set default-features = false, when you want use-rustls-ring?

Not a big deal but a little nicer for the user

IIRC its doing this way, in order to match default features being the usage of dependencies default features too. (Although I agree and think it's better ergonomics for the BDKs user to not have to default-features = false

oleonardolima avatar Jul 21 '24 21:07 oleonardolima

Hey @thunderbiscuit do you know if it be possible to get this merged in soon? I was using bdk_electrum set to this branch, but i'm getting version mismatch errors on bdk_chain package.

praveenperera avatar Jul 26 '24 01:07 praveenperera

Sorry folks I'm coming back to this. Would love to get it merged for the summit.

@praveenperera

Is it possible to set up the features so that you don't have to set default-features = false, when you want use-rustls-ring?

The way to do this would be to disable rustls as a default feature completely and force users to choose either the use-rustls or use-rustls-ring feature. The reason why I chose to not do it this way is that this would break all existing users' code (the default rustls feature gets removed under their feet).

Sorry to tag but here are some interested parties in this PR: @oleonardolima @ValuedMammal @LLFourn @notmandatory @praveenperera

See the sister discussion on https://github.com/bitcoindevkit/rust-electrum-client/pull/135. This will fix a number of users' builds (BDK bindings, Anchorwatch, Cove). I want to make sure we get it in for the next release.

thunderbiscuit avatar Jul 27 '24 13:07 thunderbiscuit

Rebased and ready for final review.

thunderbiscuit avatar Aug 07 '24 03:08 thunderbiscuit

Thanks for the review @oleonardolima @tnull! I just got to your comments today. Rebased and fixed now.

thunderbiscuit avatar Aug 12 '24 19:08 thunderbiscuit

ACK f965f95721c719ac755da07aa997d9b858583856

LLFourn avatar Aug 13 '24 10:08 LLFourn