Roll a custom `TestEnv` and drop `lazy_static` by luisschwab · Pull Request #176 · bitcoindevkit/rust-esplora-client

luisschwab · 2026-03-08T15:43:19Z

Closes #149

Implement a custom TestEnv, which is instantiated per-test and fixes the un-dropped bitcoind issue. This also allows running tests with more than 1 thread, since they no longer share bitcoind and electrsd instances.

Also adds helpers to fetch externally sourced addresses, fixing the issue where bitcoind's wallet would send change outputs to it's own wallet address, which would mess with test_get_scripthash_stats.

The test recipe and CI job are updated to now use 16 threads.

luisschwab · 2026-03-08T15:55:20Z

Need to pin some stuff...

notmandatory

I ran this through my AI friend 🤖 and it found two issue and one nit that I think are worth mentioning. Otherwise very good improvement to the testing!

src/lib.rs

ValuedMammal · 2026-03-11T22:32:52Z

Concept ACK

luisschwab · 2026-03-11T23:00:12Z

Addressed Steve's review and added a headers field to the AsyncClient that's pub(crate), for testing purposes (it will be removed once we move to bitreq).

ValuedMammal · 2026-03-12T03:02:33Z

Looks like your code is correctly setting the headers on the Builder for both the async and blocking clients, so no need for the unused AsyncClient::headers.

luisschwab · 2026-03-12T03:21:21Z

Looks like your code is correctly setting the headers on the Builder for both the async and blocking clients, so no need for the unused AsyncClient::headers.

The extra field is for the test only, will be handy to catch any regressioms when we switch to bitreq.

oleonardolima

Overall looks good! However, we shouldn't have the new field on AsyncClient, left an explanation how to properly test it in a comment below.

ci/pin-msrv.sh

oleonardolima · 2026-03-12T21:00:28Z

src/lib.rs

+        fn wait_until_electrum_sees_block(&self) {
+            let _header = self.electrsd.client.block_headers_subscribe().unwrap();
+            loop {
+                if self.electrsd.client.block_headers_pop().unwrap().is_some() {
+                    break;
+                }
+                self.electrsd.trigger().unwrap();
+                self.electrsd.client.ping().unwrap();
+                std::thread::sleep(Duration::from_millis(500));
+            }
+        }


You should keep the exponential backoff approach, instead of having a fixed delay.

What is the rationale in keeping it? We only need to wait for electrum to index the block, and 500ms is a rounding error compared to the improvement in test time that this PR accomplished. It's unnecessary complexity, as I'm not touching client code in this PR.

What is the rationale in keeping it? We only need to wait for electrum to index the block, and 500ms is a rounding error compared to the improvement in test time that this PR accomplished. It's unnecessary complexity, as I'm not touching client code in this PR.

Alright, the rationale is that it already handles any scenario: one that indexes it quickly with the minimum overhead of 64 ms, and any other ones that might take longer, without the fixed overhead of 500ms for every single one.

It doesn't really add much complexity and the code already exists, and it does not touches any client code, only the test module.

Also, with the new change you are checking for any new block, which might not be the expected height.

I'm -0 on this, but the exponential backoff can be kept.

oleonardolima · 2026-03-12T21:03:41Z

src/lib.rs

+        /// Get a `Legacy` regtest address.
+        fn get_legacy_address(&self) -> Address {
+            Address::from_str("mvUsRD2pNeQQ8nZq8CDEx6fjVQsyzqyhVC")
+                .unwrap()
+                .assume_checked()
+        }


What's the rationale for having fixed addresses, instead o fetching one from the client. If it's required only for test_get_scripthash_stats they should probably constants under that test only.

bitcoind would use an address as destination for change for another addresses transaction. This is why tests were flaky. I see no issue in hardcoding these, as we only want to check transaction count and balance for an arbitrary address; in turn we also save a bunch of calls to bitcoind by using this approach.

bitcoind would use an address as destination for change for another addresses transaction. This is why tests were flaky. I see no issue in hardcoding these, as we only want to check transaction count and balance for an arbitrary address; in turn we also save a bunch of calls to bitcoind by using this approach.

Which tests were flaky ?

It was flaky due to:

(i) The address issue you mentioned
(ii) The conflicting bitcoind usage ?

If was due to (i) it's probably an issue on the test setup itself. If it was due to (ii) it's basically solved by using the new TestEnv, isn't ?

It was because of (ii). IIRC bitcoind would use the legacy address to send change when sending to the other 3, so we would get 2 transactions vs the expected 1 transaction.

src/async.rs

oleonardolima · 2026-03-19T16:46:17Z

I pushed 5bdab27 to address the test for HTTP headers, it does in such a way that asserts it in the final HTTP request.

I wasn't able to do it following the approach I mentioned #176 (comment), because reqwest only builds it's final requests by calling .send(), which calls .execute_request() and builds the final requests with headers internally (there's no public API for it).

Implement a custom `TestEnv`, which is instantiated per-test and fixes the un-dropped `bitcoind` issue. This also allows running tests with more than 1 thread, since they no longer share `bitcoind` and `electrsd` instances. Also adds helpers to fetch externally sourced addresses, fixing the issue where `bitcoind`'s wallet would send change outputs to it's own wallet address, which would mess with `test_get_scripthash_stats`. The `test` recipe and CI job are updated to now use 16 threads.

- updates the `test_get_tx_with_http_header` to assert for expected HTTP headers in final request. - updates `setup_clients_with_headers` to expect custom `url`.

oleonardolima

cACK f6faee5

I applied a few changes regarding the MSRV and the HTTP headers test, it still needs addressing for the exponential backoff and addresses comments above.

outdated

luisschwab requested review from ValuedMammal and notmandatory March 8, 2026 15:43

luisschwab self-assigned this Mar 8, 2026

luisschwab requested a review from oleonardolima as a code owner March 8, 2026 15:43

luisschwab added the chore label Mar 8, 2026

luisschwab force-pushed the feat/custom-testenv branch 3 times, most recently from 28bba06 to 4f5ee42 Compare March 8, 2026 16:36

luisschwab added the bug Something isn't working label Mar 8, 2026

notmandatory reviewed Mar 11, 2026

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

luisschwab force-pushed the feat/custom-testenv branch from 4f5ee42 to 8653954 Compare March 11, 2026 22:58

luisschwab requested a review from notmandatory March 11, 2026 23:00

oleonardolima requested changes Mar 12, 2026

View reviewed changes

oleonardolima previously requested changes Mar 12, 2026

View reviewed changes

src/async.rs Outdated Show resolved Hide resolved

oleonardolima force-pushed the feat/custom-testenv branch from 8653954 to 5bdab27 Compare March 19, 2026 16:41

luisschwab added 3 commits March 19, 2026 14:01

chore(ci): fix pinning for 1.75.0 MSRV

6eeb80c

refactor(test): properly assert for HTTP headers

f6faee5

- updates the `test_get_tx_with_http_header` to assert for expected HTTP headers in final request. - updates `setup_clients_with_headers` to expect custom `url`.

oleonardolima force-pushed the feat/custom-testenv branch from 5bdab27 to f6faee5 Compare March 19, 2026 17:01

oleonardolima reviewed Mar 19, 2026

View reviewed changes

oleonardolima added this to BDK Chain Mar 19, 2026

oleonardolima moved this to Needs Review in BDK Chain Mar 19, 2026

luisschwab requested a review from oleonardolima March 22, 2026 21:51

Conversation

luisschwab commented Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luisschwab commented Mar 8, 2026

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ValuedMammal commented Mar 11, 2026

Uh oh!

luisschwab commented Mar 11, 2026

Uh oh!

ValuedMammal commented Mar 12, 2026

Uh oh!

luisschwab commented Mar 12, 2026

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

luisschwab Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

oleonardolima Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

luisschwab Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

oleonardolima Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

luisschwab Mar 22, 2026 • 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!

oleonardolima Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

luisschwab Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima commented Mar 19, 2026

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

luisschwab commented Mar 8, 2026 •

edited

Loading

luisschwab Mar 22, 2026 •

edited

Loading