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.

Sign up for GitHub

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

Jump to bottom

test: add test for signet miner script #24559

Merged
merged 3 commits into from
Apr 14, 2022
Merged

test: add test for signet miner script #24559

merged 3 commits into from
Apr 14, 2022

Conversation

theStack
Copy link
Contributor

@theStack theStack commented Mar 14, 2022 •
edited
Loading

This PR adds a very basic test for the signet miner script (contrib/signet/miner). It was based on #24553 (merged by now) which fixes a bug (and was also the motivation to write this test).

The test roughly follows the steps from https://en.bitcoin.it/wiki/Signet#Custom_Signet, except that the challenge key-pair is created solely with the test framework. Calibration is also skipped, the difficulty is simply set to the first mainnet target 0x1d00ffff (see also https://bitcoin.stackexchange.com/a/57186).

@theStack theStack force-pushed the 202203-test-add_test_for_signet_miner branch 6 times, most recently from 11f79b7 to aaa81d2 Compare March 14, 2022 16:46
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 14, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Mar 15, 2022
Closed
@gruve-p gruve-p mentioned this pull request Mar 16, 2022
Merged
@theStack theStack force-pushed the 202203-test-add_test_for_signet_miner branch from aaa81d2 to d1d98c2 Compare March 17, 2022 17:44
@laanwj
Copy link
Member

laanwj commented Apr 5, 2022

Concept ACK, thanks for adding tests

@theStack
test: determine path to bitcoin-util in test framework
dde33ec 
The path is stored in `self.options.bitcoinutil`, points to
`src/bitcoin-util` by default and can be overrided with the
`BITCOINUTIL` environment variable.
@theStack
test: add is_bitcoin_util_compiled helper
449b96e
@theStack theStack force-pushed the 202203-test-add_test_for_signet_miner branch from d1d98c2 to f8abc66 Compare April 11, 2022 20:03
@theStack
Copy link
Contributor Author

Force-pushed with preliminary commits integrating bitcoin-util into the test framework (storing its path to self.options.bitcoinutil, adding is_bitcoin_util_compiled/skip_if_no_bitcoin_util helpers depending on the test/config.ini entry), as suggested by @laanwj.

@laanwj
Copy link
Member

laanwj commented Apr 13, 2022 •
edited
Loading

Code review ACK f8abc66
I've verified that the test gets run (and not skipped) in the various CI runs.

@0xB10C
Copy link
Contributor

0xB10C commented Apr 13, 2022

Concert ACK!

@theStack theStack force-pushed the 202203-test-add_test_for_signet_miner branch from f8abc66 to 038d2a6 Compare April 13, 2022 22:29
@theStack
Copy link
Contributor Author

Force-pushed with a simpler signet challenge (p2wpkh instead of 1-of-1 multisig that the descriptor wallet couldn't sign), the test can now be run with both legacy and descriptor wallet. Also removed the "works only with legacy wallet" part from the PR description accordingly.

@laanwj
Copy link
Member

laanwj commented Apr 14, 2022

re-ACK 038d2a6
Thanks for making the test work for the non-legacy wallet too.

@laanwj laanwj merged commit cf0a8b9 into bitcoin:master Apr 14, 2022
@theStack theStack deleted the 202203-test-add_test_for_signet_miner branch April 14, 2022 17:40
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 14, 2022
@laanwj
Merge bitcoin#24559: test: add test for signet miner script
830f886
@bitcoin bitcoin locked and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@laanwj laanwj laanwj left review comments

Assignees
No one assigned
Labels
Scripts and tools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@theStack @DrahtBot @laanwj @0xB10C