Disable reqwest's default features

[thomaseizinger]

Description

By default, reqwest uses openssl for TLS. Any consumer wanting to use
rustls will thus pull in unnecessary dependencies.

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
• I've updated CHANGELOG.md

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

[LLFourn]

utACK 0b90ac0. I noticed this too but forgot to fix it. Thanks @thomaseizinger.

[afilini]

Concept ACK, but I would also add another feature to enable explicit https support (like reqwest-with-https) so that if you want the default https implementation you don't have to include reqwest as a dependency of your project just to enable the feature

[LLFourn]

@afilini good point. I don't think @thomaseizinger was trying to disable https completely. But if you disable default features I guess that's what happens. I think we should just add rustls in the reqwest feature flags. I think you always want https when using reqwest for now.

[thomaseizinger]

I would also add another feature to enable explicit https support (like reqwest-with-https) so that if you want the default https implementation you don't have to include reqwest as a dependency of your project just to enable the feature

I generally agree with this but reqwest has a lot of TLS related features: https://github.com/seanmonstar/reqwest/blob/baffb9c004fbd4fb235046683edb3f86ab74596b/Cargo.toml#L31-L43

I don't think it is worth it passing all those through and we would have to do that if we don't want to be opinionated on either openssl or rustls which is the entire point of why I opened this PR :)

[notmandatory]

To make it easier for existing users who want to keep the default reqwest behavior I think @afilini 's suggestion makes sense. I'd call it something like reqwest-default-tls, and the whole change would be something like:

reqwest = { version = "0.11", optional = true, default-features = false, features = ["json"] }

# MUST ALSO USE `--no-default-features`
use-esplora-reqwest = ["esplora", "reqwest", "reqwest/socks", "futures"]

# Use below feature with `use-esplora-reqwest` to enable reqwest default TLS support
reqwest-default-tls = ["reqwest/default-tls"]

[thomaseizinger]

Done!

[rajarshimaitra]

utACK 5652704

@thomaseizinger @notmandatory would it be better to include this feature operations somewhere in readme doc?

There is also this CI failure happening which I noticed in #497 too.

[rajarshimaitra]

Restarted the CI jobs. The failure is still there. Weirdly its not happening in my local.

$ cargo +1.46.0 clippy --all-targets --features async-interface --no-default-features -- -D warnings
    Checking bdk v0.14.1-dev (/home/raj/github-repo/bdk)
    Finished dev [unoptimized + debuginfo] target(s) in 2.36s

[notmandatory]

Now that #501 is in you can rebase on master to fix the CI issue.

[thomaseizinger]

Now that #501 is in you can rebase on master to fix the CI issue.

Done.

[thomaseizinger]

Rebased to fix conflicts. Is there something else blocking this from merging?

[rajarshimaitra]

LGTM 380a4f2

[notmandatory]

ACK 380a4f2

[RCasatta]

LGTM 380a4f2

@rajarshimaitra, the github-merge.py tool use ACK keyword to show devs who acked the PR in the merge commit (see there isn't your LGTM in merge commit description 5107ff80c1ae39dae217b7bc82ce22beb00790ed)description, so I suggest not using LGTM in this case (utACK or other variation are ok too)

[rajarshimaitra]

Ah I see.. Thanks @RCasatta will keep that in mind..