chore: extract `TestEnv` into separate crate by LagginTimes · Pull Request #1171 · bitcoindevkit/bdk

LagginTimes · 2023-10-12T08:16:20Z

Description

TestEnv is extracted into its own crate with electrsd support added so that TestEnv can also serve esplora and electrum.
The tests in the esplora crate have also been updated to use TestEnv.

The tx_can_become_unconfirmed_after_reorg() test in test_electrum suggests that the electrum issue where a reorged tx would be stuck at confirmed does not exist anymore.

Notes to the reviewers

The code for tx_can_become_unconfirmed_after_reorg() was adapted from the same test in bitcoind_rpc/test_emitter. This electrum version of the test requires extra review, as I am uncertain if I used the API efficiently.

All Submissions:

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

evanlinjin

We need to change the internals to use electrsd so the test env can serve esplora and electrum as well.

Can use #979 as a reference to see what electrum-server methods we need.

LagginTimes · 2023-10-12T08:44:50Z

We need to change the internals to use electrsd so the test env can serve esplora and electrum as well.

I will look into this().

vladimirfomene · 2023-10-12T22:37:58Z

I think having reorg functions for all backends will help. I had to use a hack from Steve to implement reorg for electrum. You should see the code in #979. Read more here: https://bitcoin.stackexchange.com/questions/114044/how-can-i-simulate-a-reorg-for-testing.

evanlinjin · 2023-10-13T06:02:07Z

@vladimirfomene that's a great point.

https://github.com/bitcoindevkit/bdk/blob/release/0.29/src/testutils/blockchain_tests.rs

Maybe the old testutils is something to base this work on.

evanlinjin

Thanks for taking this forward. The code looks pretty good.

The documentation was a little lacking. It's important to communicate why things are implemented the way they are.

crates/bitcoind_rpc/tests/test_emitter.rs

crates/bitcoind_rpc/Cargo.toml

crates/testenv/src/lib.rs

LagginTimes · 2023-11-03T20:22:17Z

A simple electrsd reorg test was written and runs without having to pass in bitcoinD references to mine_blocks() and reorg(). I have therefore removed the extraneous bitcoind input parameters from TestEnv. Will have this commit sorted out tomorrow.

LagginTimes · 2023-11-11T20:31:21Z

A test_chain_sync() test has been added that reproduces the issue described in #1125 and also confirms that the fix in #1145 at least partially fixes the problem (in that there may be additional bugs as seen by @danielabrozzoni).

1010efd fix(electrum): fixed chain sync issue (Wei Chen) Pull request description: ### Description This may or may not fix #1125. Fixed what appeared to be a logic error in `construct_update_tip` in `electrum_ext.rs` that caused the local chain tip to always be a block behind the newest confirmed block. ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### Bugfixes: * [ ] This pull request breaks the existing API * [x] I've added tests to reproduce the issue which are now passing * [x] I'm linking the issue being fixed by this PR ACKs for top commit: danielabrozzoni: ACK 1010efd - although I've been able to reproduce the issue in #1125, I'm convinced that this PR fixes at least a bug, as demonstrated in #1171 (yet to be reviewed and merged). Tree-SHA512: 92790e9072d17be74d2cd24bec3503e1ad5d97f728ee81490eeb09ac3f8d4a3a7e8d9628e943bc801246d5bfd345152c11d5dbe25246f5a57b3118727d3ae315

crates/testenv/src/lib.rs

storopoli · 2024-02-15T07:33:15Z

I know that we're testing it in other crates.
But do we plan to expose this in the future?
If yes, I think we should add tests for the test_env crate (I know that this sounds like an oxymoron, but bear with me ...)

ValuedMammal · 2024-02-19T02:56:00Z

Now that #1351 is open, I made LagginTimes#1 to test stop_gap in ElectrumExt::full_scan

evanlinjin

ACK 2903697

Cargo.toml

notmandatory

Overall looks like a great improvement, related tests are much easier to read too. I just have one comment/question about a CI change.

.github/workflows/nightly_docs.yml

crates/testenv/Cargo.toml

storopoli

ACK e8dc8d3

oleonardolima

Overall looks pretty good!

I just left one minor comment about the documentation that I was uncertain about, and a general question for further discussion.

crates/electrum/tests/test_electrum.rs

oleonardolima · 2024-03-21T15:23:35Z

crates/bitcoind_rpc/tests/test_emitter.rs

@@ -744,7 +624,7 @@ fn mempool_during_reorg() -> anyhow::Result<()> {
        // emission.
        // TODO: How can have have reorg logic in `TestEnv` NOT blacklast old blocks first?


@LagginTimes general question: after this PR is merged, do you think we'll be able to solve this TODO ?

Unfortunately no. This can be done in a follow-up PR!

Cool, thanks!

`TestEnv` is extracted into its own crate to serve as a framework for testing other block explorer APIs.

Added scan and reorg tests to check electrum functionality using `TestEnv`.

evanlinjin · 2024-03-22T10:08:24Z

crates/esplora/Cargo.toml

 [target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies]
 electrsd = { version= "0.25.0", features = ["bitcoind_25_0", "esplora_a33e97e1", "legacy"] }


Do we need to make the tests run in WASM? Because electrsd is a mandatory dependency of bdk_testenv.

Right now for CI we only do a cargo check for WASM and don't rerun the tests for that target. Since I haven't heard of any WASM issues that slipped through by not running the tests I don't think we need to change TestEnv to work with WASM.

oleonardolima

ACK 04d0ab5

notmandatory

ACK 04d0ab5

Great job, this will be a big help with testing.

None of the `bdk_esplora` tests can run under WASM anyway since we depend on `electrsd`.

evanlinjin

ACK 7c9ba3c

This comment was marked as resolved.

Sign in to view

LagginTimes force-pushed the extract_testenv branch from dbbe366 to 0be5338 Compare October 12, 2023 08:19

LagginTimes changed the title ~~chore(): extract TestEnv into separate crate~~ chore: extract TestEnv into separate crate Oct 12, 2023

evanlinjin requested changes Oct 12, 2023

View reviewed changes

LagginTimes mentioned this pull request Oct 13, 2023

fix(electrum): fixed chain sync issue #1145

Merged

6 tasks

LagginTimes marked this pull request as draft October 15, 2023 15:55

LagginTimes force-pushed the extract_testenv branch 2 times, most recently from 50c5ccd to 12403a1 Compare October 17, 2023 09:59

LagginTimes marked this pull request as ready for review October 17, 2023 11:43

LagginTimes force-pushed the extract_testenv branch from 19cafee to 2cadfd2 Compare October 19, 2023 07:31

nondiremanuel mentioned this pull request Oct 22, 2023

W43 Lib Team Call bitcoindevkit/.github#83

Closed

33 tasks

evanlinjin requested changes Oct 23, 2023

View reviewed changes

nondiremanuel added this to the 1.0.0-beta.0 milestone Oct 24, 2023

LagginTimes force-pushed the extract_testenv branch from 2cadfd2 to c45a8f5 Compare November 4, 2023 15:40

nondiremanuel mentioned this pull request Nov 5, 2023

W45 Lib Team Call bitcoindevkit/.github#84

Closed

38 tasks

LagginTimes force-pushed the extract_testenv branch 6 times, most recently from fb8ab29 to f2af089 Compare November 11, 2023 20:15

LagginTimes requested a review from evanlinjin November 11, 2023 23:45

LagginTimes force-pushed the extract_testenv branch 2 times, most recently from 6bb1a40 to cc50303 Compare November 12, 2023 00:14

LagginTimes requested a review from evanlinjin February 5, 2024 16:57

ValuedMammal reviewed Feb 6, 2024

View reviewed changes

crates/testenv/src/lib.rs Show resolved Hide resolved

ValuedMammal reviewed Feb 6, 2024

View reviewed changes

crates/testenv/src/lib.rs Show resolved Hide resolved

LagginTimes force-pushed the extract_testenv branch from 53186e2 to 2903697 Compare February 8, 2024 17:35

evanlinjin mentioned this pull request Feb 20, 2024

The TxGraph::missing_heights implementation is problematic. #1354

Closed

evanlinjin approved these changes Mar 5, 2024

View reviewed changes

danielabrozzoni reviewed Mar 5, 2024

View reviewed changes

Cargo.toml Show resolved Hide resolved

LagginTimes force-pushed the extract_testenv branch from 2903697 to 29ac879 Compare March 6, 2024 14:37

notmandatory reviewed Mar 16, 2024

View reviewed changes

.github/workflows/nightly_docs.yml Show resolved Hide resolved

crates/testenv/Cargo.toml Outdated Show resolved Hide resolved

notmandatory added tests module-blockchain labels Mar 16, 2024

LagginTimes force-pushed the extract_testenv branch 2 times, most recently from a09b56b to e8dc8d3 Compare March 20, 2024 08:04

storopoli approved these changes Mar 20, 2024

View reviewed changes

oleonardolima reviewed Mar 21, 2024

View reviewed changes

LagginTimes added 3 commits March 22, 2024 17:59

chore: update MSRV dependency for nightly docs

6e648fd

chore: extract TestEnv into separate crate

4edf533

`TestEnv` is extracted into its own crate to serve as a framework for testing other block explorer APIs.

test(electrum): added scan and reorg tests

04d0ab5

Added scan and reorg tests to check electrum functionality using `TestEnv`.

LagginTimes force-pushed the extract_testenv branch from e8dc8d3 to 04d0ab5 Compare March 22, 2024 10:00

evanlinjin reviewed Mar 22, 2024

View reviewed changes

oleonardolima approved these changes Mar 22, 2024

View reviewed changes

notmandatory approved these changes Mar 22, 2024

View reviewed changes

evanlinjin added 2 commits March 23, 2024 17:57

chore(esplora): rm custom WASM target spec test dependency

2462e90

None of the `bdk_esplora` tests can run under WASM anyway since we depend on `electrsd`.

chore(electrum,testenv): move test_reorg_is_detected_in_electrsd

7c9ba3c

evanlinjin approved these changes Mar 23, 2024

View reviewed changes

evanlinjin merged commit 80e190b into bitcoindevkit:master Mar 23, 2024

evanlinjin mentioned this pull request Mar 23, 2024

Fix last seen unconfirmed #1385

Merged

3 tasks

notmandatory mentioned this pull request Mar 26, 2024

Release 1.0.0-alpha.8 #1388

Closed

25 tasks

		@@ -744,7 +624,7 @@ fn mempool_during_reorg() -> anyhow::Result<()> {
		// emission.
		// TODO: How can have have reorg logic in `TestEnv` NOT blacklast old blocks first?

		[target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies]
		electrsd = { version= "0.25.0", features = ["bitcoind_25_0", "esplora_a33e97e1", "legacy"] }

Conversation

LagginTimes commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

All Submissions:

Uh oh!

This comment was marked as resolved.

evanlinjin left a comment • 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!

LagginTimes commented Oct 12, 2023

Uh oh!

vladimirfomene commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evanlinjin commented Oct 13, 2023

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LagginTimes commented Nov 3, 2023

Uh oh!

LagginTimes commented Nov 11, 2023

Uh oh!

Uh oh!

Uh oh!

storopoli commented Feb 15, 2024

Uh oh!

ValuedMammal commented Feb 19, 2024

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

storopoli left a comment

Choose a reason for hiding this comment

Uh oh!

oleonardolima left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

oleonardolima Mar 21, 2024

Choose a reason for hiding this comment

Uh oh!

evanlinjin Mar 22, 2024 • 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 22, 2024

Choose a reason for hiding this comment

Uh oh!

evanlinjin Mar 22, 2024

Choose a reason for hiding this comment

Uh oh!

notmandatory Mar 22, 2024 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

notmandatory left a comment

Choose a reason for hiding this comment

Uh oh!

evanlinjin left a comment

Choose a reason for hiding this comment

Uh oh!

LagginTimes commented Oct 12, 2023 •

edited

Loading

evanlinjin left a comment •

edited

Loading

vladimirfomene commented Oct 12, 2023 •

edited

Loading

evanlinjin Mar 22, 2024 •

edited

Loading

notmandatory Mar 22, 2024 •

edited

Loading