Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test electrum ext #979

Closed

Conversation

vladimirfomene
Copy link
Contributor

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

@evanlinjin
Copy link
Member

evanlinjin commented May 17, 2023

@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
Contributor Author

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

@LLFourn
Copy link
Contributor

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?

@notmandatory
Copy link
Member

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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants