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

feat(bdk_electrum): add use-openssl as a feature #1620

Conversation

oleonardolima
Copy link
Contributor

@oleonardolima oleonardolima commented Sep 20, 2024

partially addresses #1598

Description

It's a simple PR to expose the use-openssl from electrum-client to bdk_electrum. It partially addresses #1598, allowing the user to use openssl as an alternative to rustls, as there are some problems with it when handling some types of TLS certificates.

Do we need to add some sort of bdk_electrum tests using the new exposed use-openssl feature ?

Notes to the reviewers

Changelog notice

  • Adds use-openssl as feature to bdk_electrum, exposing electrum-client use-openssl feature.

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

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.

tACK 19849bd

I tested with example_electrum and confirmed that if I use the default bdk_electrum features ("use-rustls") then I'm unable to sync to "ssl://electrum.blockstream.info:60002". But if I disable default features and enable the new "use-openssl" feature then I am able to sync to "ssl://electrum.blockstream.info:60002".

@notmandatory
Copy link
Member

I just have one question, should we make use-openssl the default instead of use-rustls since it seems to be more compatible with self-signed certs?

@oleonardolima oleonardolima marked this pull request as ready for review September 26, 2024 14:32
@oleonardolima
Copy link
Contributor Author

I just have one question, should we make use-openssl the default instead of use-rustls since it seems to be more compatible with self-signed certs?

I'm fine with it, but I'm not sure about other impacts. Do bindings and other users rely on the default feature being rustls? Does OpenSSL work fine on bindings/mobile stuff? cc @thunderbiscuit @reez

@tnull
Copy link
Contributor

tnull commented Oct 1, 2024

Does OpenSSL work fine on bindings/mobile stuff?

I can only speak to this part, and the answer is: no, unfortunately it doesn't work on mobile as nobody has figured out a way to have it link in the right libraries when cross-compiling for Android. It would be great if we could (which would allow dropping a bunch of dependencies and would be more secure), but so far we seem to be stuck on rustls.

@thunderbiscuit
Copy link
Member

Yes I second what tnull said: open-ssl is not currently a solution for the Android libraries as we built them. It's not impossible to get it to work and would be great to figure it out one day, but as is rustls is our best option.

@oleonardolima to answer your question "Do bindings and other users rely on the default feature being rustls?" I think the answer is no (at least for bdk-ffi). We currently disable the default features and simply enable use-rustls-ring, so changing the default for the bdk client would not impact us. If you think it's a better default to use open-ssl, I'm open to it.

@notmandatory notmandatory force-pushed the feat/expose-electrum-client-openssl-feature branch from 19849bd to fb6dd98 Compare October 1, 2024 15:22
@notmandatory
Copy link
Member

I had to rebase this one due to a small merge conflict.

@oleonardolima oleonardolima force-pushed the feat/expose-electrum-client-openssl-feature branch from fb6dd98 to f602d1b Compare October 2, 2024 01:43
@oleonardolima
Copy link
Contributor Author

I had to rebase it again to solve the CI issue (although I think a re-run would've solved it 😅 ).

@oleonardolima
Copy link
Contributor Author

Thanks for the comment @tnull and @thunderbiscuit, I wasn't aware of it.

@notmandatory notmandatory merged commit 5c8cfcc into bitcoindevkit:master Oct 2, 2024
21 checks passed
@oleonardolima oleonardolima deleted the feat/expose-electrum-client-openssl-feature branch October 2, 2024 02:17
@ValuedMammal ValuedMammal mentioned this pull request Oct 2, 2024
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module-blockchain new feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants