Fix esplora testing

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.

[tcharding]

Description

Currently testing of esplora/reqwest is broken because of incorrect usage of, or lack of, async.

Enable testing of esplora/reqwest by using maybe_await! macro in tests and declaring all unit tests in blockchain_tests.rs to be async. This adds a dev dependency on tokio.

With this patch applied one can successfully run:

cargo test --features=test-esplora,use-esplora-reqwest --no-default-features

Notes to the reviewers

This is, in my opinion, a better fix to #431 than #430 (i.e., supercedes #430)

The primary advantage of this PR over #430 is it does not introduce a hard dependency on tokio.

Checklists

All Submissions:

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

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

[notmandatory]

This also looks like a good approach. Either way I agree if we only need tokio for testing it should be a dev-dependency. This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest, I think can copy from @rajarshimaitra PR. I'll check it out more closely tomorrow for a proper review.

[notmandatory]

I think you meant to move this to esplora/mod.rs as in your original #415 (and remove from esplora/reqwest.rs).

[tcharding]

If we want to be able to test both eslpora/reqwest and esplora/ureq we need this to be in the ureq/reqwest submodules. I originally only put it in mod.rs thinking testing both was unnecessary, in hindsight this was wrong. With this change testing can be done by enabling feature test-esplora as well as the feature to enable the backend we want to test (e.g. use-esplora-reqwest).

[notmandatory]

I locally tried moving this chunk to esplora/mod.rs and removing from reqwest.rs and ureq.rs and tests run fine with these commands:

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
cargo test --no-default-features --features test-esplora,use-esplora-reqwest esplora::bdk_blockchain_tests

[tcharding]

This PR also needs to have the CI pipeline updated to run the blockchain tests for esplora with ureq and reqwest

Oh, yes, my bad. Will add, thanks.

[tcharding]

Patch 655560f Enable testing Esplora backends in CI needs careful review please. The patch is pure cargo cult programing.

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
Finished test [unoptimized + debuginfo] target(s) in 0.05s
Running unittests (/home/tobin/.cache/cargo/debug/deps/bdk-0eaddf4dc688116f)
running 0 tests
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 213 filtered out; finished in 0.00s

And without the test filter I get just the unit tests running

cargo test --no-default-features --features test-esplora,use-esplora-ureq esplora::bdk_blockchain_tests
...
test result: ok. 41 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 20.21s

[RCasatta]

When I attempt to run the esplora integration test as it appears it will be run on CI, nothing runs?

You have to give the right initial namespace, for example:

cargo test --no-default-features --features test-esplora,use-esplora-ureq blockchain::esplora

[notmandatory]

As @RCasatta suggested, this will work:

Suggested change

	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::bdk_blockchain_tests
	run: cargo test --no-default-features --features test-esplora,use-esplora-${{ matrix.backend.name }} esplora::${{ matrix.backend.name }}::bdk_blockchain_tests

Or I think the better solution is to move the below chunk to esplora/mod.rs and remove it from reqwest.rs and ureq.rs (I confirmed it works):

#[cfg(test)]
#[cfg(feature = "test-esplora")]
crate::bdk_blockchain_tests! {
fn test_instance(test_client: &TestClient) -> EsploraBlockchain {
EsploraBlockchain::new(&format!("http://{}",test_client.electrsd.esplora_url.as_ref().unwrap()), 20)
}
}

And then the above CI line does not need to change.

[rajarshimaitra]

tACK.

Agreed with the concept that reqwest user should not have to have tokio just for testing.

All the tests passes as expected.

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.
Cargo.lock after build:

...
[[package]]
name = "tokio"
version = "1.8.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "570c2eb13b3ab38208130eccd41be92520388791207fde783bda7c1e8ace28d4"
dependencies = [
"autocfg",
"bytes",
"libc",
"memchr",
"mio",
"pin-project-lite",
"tokio-macros",
"winapi 0.3.9",
]
[[package]]
name = "tokio-macros"
version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "54473be61f4ebe4efd09cec9bd5d16fa51d70ea0192213d754d2d500457db110"
dependencies = [
"proc-macro2",
"quote",
"syn",
]
[[package]]
name = "tokio-native-tls"
version = "0.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "f7d995660bd2b7f8c1568414c1126076c13fbb725c40112dc0120b78eb9b717b"
dependencies = [
"native-tls",
"tokio",
]
[[package]]
name = "tokio-util"
version = "0.6.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "1caa0b0c8d94a049db56b5acf8cba99dc0623aab1b26d5b5f5e2d945846b3592"
dependencies = [
"bytes",
"futures-core",
"futures-sink",
"log",
"pin-project-lite",
"tokio",
]

I am not entirely sure whether there is any difference between "hard dependency" and "dependency of a dependency". Just by looking at Cargo.locks from builds of both #433 and #430, it seems like it doesn't matter to our dependency tree. tokio is gonna be there in release mode whether we ask for it or not. reqwest depends on it (among few other crates).

Another minor concern I have is over code repetition. And there is really no other workaround if we choose not use await_or_block!() macro. This might create maintenance burden. But good thing is at least they are in testsutils only.

So far I haven't able to form any hard opinion on either approach. I think we should choose whichever makes our lives easier down the line. Maintaining a consistent async interfaces is a tricky thing for sure.

[LLFourn]

I just had a chat with @tcharding where we tried to come up with a way around the sprawling complexity of having multiple http backends. The result of the discussion was something like why not just have our own HttpClient and implement both async and non-async versions. This would just include get, post methods and return the body a Result<Vec<u8>, HttpError>

In blockchain tests we would only use the blocking one and therefore not test request there. However the only logic in the http backend would be making a http request and returning the result so there is no real reason to test this as there is no business logic.

At this point I think HttpClient should be a trait and esplora would use it like Esplora<C: HttpClient>. This also enables BYO http client (I know of one person who said they want this). This would make the http client a separate choice for the user to consider which might make the feature flags more coherent anyway.

I think I will take on this work myself starting next week if nobody has any objections.

[LLFourn]

I am observing though, even in a release build cargo build --release --features=use-esplora-reqwest --no-default-features, tokio dependencies are still in the dependency tree because other dependencies already use it.

note that reqwest has a hard dependency on tokio so this is what you should see.

[notmandatory]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

[LLFourn]

I think an HttpClient trait in the esplora module sounds like a good way decouple things, similar to having a Database trait with different backends. In this case will bdk still offer optional esplora-ureq or esplora-reqwest features? or will we just include the ureq as dev-dependencies for the esplora blockchain tests? and in this case an end user (like bdk-cli) would implement it's own ureq based (or which ever) HttpClient?

My idea would be to have a http-client-blocking and http-client-async features and ureq and reqwest would not be re-exported anywhere at all in the api. bdk-cli would not have to worry at all about this (much) but downstream crates would have to have their own http dependency if they need to make http requests which I think is fine.

One other thing to keep in mind for the HttpClient trait is to support socks5 proxies, see #429.

yes

[tcharding]

Converted this to draft while development of the proposed HttpClient takes place. Cheers.

[rajarshimaitra]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

[tcharding]

Since this is converted into draft can we have #430 merged for the mean time? I have some downstream bdk-cli PRs pending on the esplora fix.

Go for it

[tcharding]

I think we can close this now, no further progress or discussion on this PR seems necessary.