Skip to content

Test electrum ext#979

Closed
vladimirfomene wants to merge 1 commit intobitcoindevkit:masterfrom
vladimirfomene:test_electrum_ext
Closed

Test electrum ext#979
vladimirfomene wants to merge 1 commit intobitcoindevkit:masterfrom
vladimirfomene:test_electrum_ext

Conversation

@vladimirfomene
Copy link
Copy Markdown
Contributor

@vladimirfomene vladimirfomene commented May 16, 2023

Description

This PR tests our Electrum scanning logic

Notes to the reviewers

I will leave this in draft for now because the test still occasionally fail. I'm still trying to figure out why but the root cause of the bug is hard to find because the bug occurs occasionally.

Checklists

All Submissions:

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

@vladimirfomene vladimirfomene marked this pull request as draft May 16, 2023 09:30
@evanlinjin
Copy link
Copy Markdown
Member

evanlinjin commented May 17, 2023 •
edited
Loading

@vladimirfomene This doesn't seem to build for 1.57.0. Could you figure out a way, to have the CI skip these tests for 1.57.0?

Also, could you provide more details for the failing tests? Thank you.

@vladimirfomene vladimirfomene force-pushed the test_electrum_ext branch 2 times, most recently from 7898945 to 3734a5c Compare May 18, 2023 10:18
@vladimirfomene
Copy link
Copy Markdown
Contributor Author

vladimirfomene commented May 18, 2023

@evanlinjin, was able to get the test to compile with 1.57. Here are the two errors I occasionally get.
two-test-failed

@LLFourn LLFourn mentioned this pull request Oct 3, 2023
Merged
6 tasks
@LLFourn
Copy link
Copy Markdown
Collaborator

LLFourn commented Oct 3, 2023

We have a bug in #1145 that having tests like this PR introduced may have caught. I think we want to introduce a test in that PR to reproduce the issue. If this PR already reproduces the issue then we could use this one. In any case the groundwork here should be useful. I think this PR can be improved by not using concepts like miniscript or KeychainTxOutIndex since we are only trying to test the electrum logic (not bdk_chain). it's easy enough to call the APIs here by just putting some spks in a Vec or whatever. In the case of #1145's bug we just need to check the chain returned is correct.

@vladimirfomene wdyt?

@nondiremanuel nondiremanuel mentioned this pull request Oct 6, 2023
Closed
39 tasks
@nondiremanuel nondiremanuel added this to the 1.0.0-alpha.3 milestone Oct 10, 2023
@evanlinjin evanlinjin mentioned this pull request Oct 12, 2023
Merged
3 tasks
@nondiremanuel nondiremanuel mentioned this pull request Oct 22, 2023
Closed
33 tasks
@nondiremanuel nondiremanuel mentioned this pull request Nov 5, 2023
Closed
38 tasks
@nondiremanuel nondiremanuel mentioned this pull request Nov 19, 2023
Closed
54 tasks
@notmandatory
Copy link
Copy Markdown
Member

notmandatory commented Nov 21, 2024

Test coverage is good for electrum client see #906. We can add more testing for new scenarios as needed. Closing this PR as it's out of date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evanlinjin evanlinjin Awaiting requested review from evanlinjin evanlinjin will be requested when the pull request is marked ready for review evanlinjin is a code owner

@oleonardolima oleonardolima Awaiting requested review from oleonardolima oleonardolima will be requested when the pull request is marked ready for review oleonardolima is a code owner

Assignees

No one assigned

Labels

module-blockchain tests

Projects

BDK Wallet
Archived in project

Milestone

1.0.0-beta

Development

Successfully merging this pull request may close these issues.

5 participants

@vladimirfomene @evanlinjin @LLFourn @notmandatory @nondiremanuel