Use async with esplora-reqwest #115

danielabrozzoni · 2022-08-24T22:16:25Z

Description

We previously had the esplora-reqwest feature, but it would use
sync reqwest, as the "async-interface" feature in BDK wasn't set.
This commit sets this feature so that using esplora-reqwest always
uses async mode.

Notes to the reviewers

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

afilini

ACK 2d8b1fa

rajarshimaitra · 2022-08-29T10:16:20Z

I am not sure if this is us or some problem with generic https calls with async-reqwest, but I am getting this error while trying out a wallet sync

$ ./target/debug/bdk-cli wallet -d "wpkh(tprv8ZgxMBicQKsPeuazF16EdPZw84eHj55AU8ZKgZgdhu3sXcHnFgjzskfDvZdTaAFHYNCbKqrurFo9onSaT7zGT1i3u3j7LKhVZF5sJA39WPN/86'/1'/0'/0/*)#40hv8z77" sync
[2022-08-29T10:12:21Z ERROR bdk_cli] Esplora(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("blockstream.info")), port: None, path: "/testnet/api//scripthash/3149608bac07ddc9d628dd85fa0a275d88cc60b4efded5499230973557151e80/txs", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") }))

But ureq is working fine on the same esplora endpoint..

rajarshimaitra · 2022-08-29T10:22:55Z

Cargo.toml

 esplora-ureq = ["esplora", "bdk/use-esplora-ureq"]
-esplora-reqwest = ["esplora", "bdk/use-esplora-reqwest"]
+async-interface = ["bdk/async-interface"]
+esplora-reqwest = ["esplora", "bdk/use-esplora-reqwest", "async-interface"]


Ah found the problem.. We need to also enable default-tls for reqwest..

Suggested change

esplora-reqwest = ["esplora", "bdk/use-esplora-reqwest", "async-interface"]

esplora-reqwest = ["esplora", "bdk/use-esplora-reqwest", "bdk/reqwest-default-tls", "async-interface"]

notmandatory · 2022-08-29T14:38:59Z

if we're not going to revert #114 then this PR also needs to be rebased. :-)

maybe_async is used regardless of which feature is activated, so it shouldn't be behind the cfg()

danielabrozzoni · 2022-08-29T21:07:28Z

Rebased :)
@rajarshimaitra I'm ok with your change, but first, could you help me understand why the tests weren't failing? :( With which features did you compile?

notmandatory · 2022-08-29T21:22:14Z

@raj I'm ok with your change, but first, could you help me understand why the tests weren't failing? :( With which features did you compile?

This seems to be the issue if you use the esplora-reqwest features and try to connect to an https URL (which is default) then we get the below error:

% RUST_LOG=debug cargo run --features esplora-reqwest -- wallet --descriptor "wpkh(tpubEBr4i6yk5nf5DAaJpsi9N2pPYBeJ7fZ5Z9rmN4977iYLCGco1VyjB9tvvuvYtfZzjD5A8igzgw3HeWeeKFmanHYqksqZXYXGsw5zjnj7KM9/*)" sync
...
ERROR bdk_cli] Esplora(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("blockstream.info")), port: None, path: "/testnet/api//scripthash/ca0948ef1c5bf5a023f22ed75275d7491c194476c11fbcfc63c0d25ed8234124/txs", query: None, fragment: None }, source: hyper::Error(Connect, "invalid URL, scheme is not http") }))

I confirmed that with @raj's suggestion this problem is fixed. We don't have any CI tests for this sort of thing so only found with the manual testing, not sure it's worth setting up integration tests for this project which is sort of meant to be used for manual testing (and other stuff).

rajarshimaitra · 2022-08-30T07:59:25Z

We don't have any CI tests for this sort of thing so only found with the manual testing, not sure it's worth setting up integration tests for this project which is sort of meant to be used for manual testing (and other stuff).

Its definitely worth setting up one, and now that we have automated backend we can use that to cook up a very simple integration test framework using std library only, where we can test out basic operations. This is included in this commit 2302ff5 of #102 . Its still very basic but it works for Rpc and Electrum. Esplora is disabled for now because I was facing some issues with it..

rajarshimaitra · 2022-08-30T08:02:46Z

@rajarshimaitra I'm ok with your change, but first, could you help me understand why the tests weren't failing? :( With which features did you compile?

Yes as @notmandatory noted, we don't have functional tests yet for bdk-cli. We only check for clap structures and some few other functions..

danielabrozzoni · 2022-08-30T08:50:38Z

Thanks for the explanation, I just updated the PR 🙏🏻

notmandatory

ACK 81e7226

rajarshimaitra

tACK 81e7226

notmandatory · 2022-08-31T16:47:53Z

@rajarshimaitra is it ok to merge this before #102? I don't think it will conflict much.

rajarshimaitra · 2022-09-02T06:07:30Z

@rajarshimaitra is it ok to merge this before #102? I don't think it will conflict much.

Yes it is.. I will rebase #102 once this is in..

afilini · 2022-09-02T14:12:24Z

Cargo.toml

 regex = { version = "1", optional = true }
 bdk-reserves = { version = "0.19", optional = true}
 electrsd = { version= "0.12", features = ["trigger", "bitcoind_22_0"], optional = true}
+tokio = { version = "1", features = ["rt", "macros", "rt-multi-thread"] }


This should be optional and only included with async-interface

afilini

ACK 3ad5b02

We previously had the esplora-reqwest feature, but it would use sync reqwest, as the "async-interface" feature in BDK wasn't set. This commit sets this feature so that using `esplora-reqwest` always uses async mode.

afilini

reACK 8d876ef

danielabrozzoni force-pushed the fix/async_reqwest branch 2 times, most recently from 29ffb54 to 6ca43c7 Compare August 24, 2022 22:23

notmandatory assigned danielabrozzoni Aug 24, 2022

danielabrozzoni force-pushed the fix/async_reqwest branch from 6ca43c7 to 2d8b1fa Compare August 25, 2022 10:16

afilini approved these changes Aug 27, 2022

View reviewed changes

rajarshimaitra mentioned this pull request Aug 28, 2022

Satoshi's Calculator. #102

Merged

6 tasks

rajarshimaitra suggested changes Aug 29, 2022

View reviewed changes

Remove wrong 'cfg()'

a9dffc3

maybe_async is used regardless of which feature is activated, so it shouldn't be behind the cfg()

danielabrozzoni force-pushed the fix/async_reqwest branch from 2d8b1fa to 3b36277 Compare August 29, 2022 21:06

danielabrozzoni force-pushed the fix/async_reqwest branch from 3b36277 to 81e7226 Compare August 30, 2022 08:50

notmandatory approved these changes Aug 30, 2022

View reviewed changes

notmandatory modified the milestones: Release 0.6.0, Release 0.7.0 Aug 30, 2022

rajarshimaitra approved these changes Aug 31, 2022

View reviewed changes

afilini reviewed Sep 2, 2022

View reviewed changes

danielabrozzoni force-pushed the fix/async_reqwest branch from 81e7226 to 3ad5b02 Compare September 2, 2022 14:12

afilini reviewed Sep 2, 2022

View reviewed changes

Use async with esplora-reqwest

8d876ef

We previously had the esplora-reqwest feature, but it would use sync reqwest, as the "async-interface" feature in BDK wasn't set. This commit sets this feature so that using `esplora-reqwest` always uses async mode.

danielabrozzoni force-pushed the fix/async_reqwest branch from 3ad5b02 to 8d876ef Compare September 2, 2022 14:32

afilini reviewed Sep 2, 2022

View reviewed changes

afilini merged commit a9d7726 into bitcoindevkit:master Sep 2, 2022

notmandatory modified the milestones: Release 0.7.0, Release 0.6.0 Sep 9, 2022

	esplora-reqwest = ["esplora", "bdk/use-esplora-reqwest", "async-interface"]
	esplora-reqwest = ["esplora", "bdk/use-esplora-reqwest", "bdk/reqwest-default-tls", "async-interface"]

Use async with esplora-reqwest #115

Use async with esplora-reqwest #115

Uh oh!

Conversation

danielabrozzoni commented Aug 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra commented Aug 29, 2022

Uh oh!

rajarshimaitra Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Aug 29, 2022

Uh oh!

danielabrozzoni commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajarshimaitra commented Aug 30, 2022

Uh oh!

rajarshimaitra commented Aug 30, 2022

Uh oh!

danielabrozzoni commented Aug 30, 2022

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Aug 31, 2022

Uh oh!

rajarshimaitra commented Sep 2, 2022

Uh oh!

afilini Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danielabrozzoni commented Aug 24, 2022 •

edited

Loading

rajarshimaitra Aug 29, 2022 •

edited

Loading

danielabrozzoni commented Aug 29, 2022 •

edited

Loading

notmandatory commented Aug 29, 2022 •

edited

Loading