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

Enable selecting use-rustls-ring feature on electrum client #1491

Merged

Conversation

thunderbiscuit
Copy link
Member

@thunderbiscuit thunderbiscuit commented Jun 26, 2024

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.

Notes to the reviewers

Do not merge before:

  • #135 is merged
  • A new version of rust-electrum-client is released (will be 0.21.0)
  • 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:

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

@notmandatory notmandatory added this to the 1.0.0-beta milestone Jun 27, 2024
@thunderbiscuit thunderbiscuit force-pushed the feature/electrum-client-ring branch 4 times, most recently from b52cf0a to 4e85341 Compare June 27, 2024 19:21
@thunderbiscuit
Copy link
Member Author

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.

@praveenperera
Copy link
Contributor

praveenperera commented Jul 21, 2024

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

@oleonardolima
Copy link
Contributor

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

@praveenperera
Copy link
Contributor

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.

@thunderbiscuit
Copy link
Member Author

thunderbiscuit commented Jul 27, 2024

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 bitcoindevkit/rust-electrum-client#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.

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 9a8fc8f

@thunderbiscuit
Copy link
Member Author

Rebased and ready for final review.

crates/electrum/Cargo.toml Outdated Show resolved Hide resolved
@thunderbiscuit
Copy link
Member Author

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

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK f965f95

@LLFourn
Copy link
Contributor

LLFourn commented Aug 13, 2024

ACK f965f95

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK f965f95

@notmandatory notmandatory merged commit 74b24d3 into bitcoindevkit:master Aug 13, 2024
12 checks passed
@thunderbiscuit thunderbiscuit deleted the feature/electrum-client-ring branch August 13, 2024 23:40
@notmandatory notmandatory mentioned this pull request Aug 25, 2024
31 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants