test: add test for signet miner script

theStack commented at 11:48 am on March 14, 2022: member

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).

DrahtBot added the label Scripts and tools on Mar 14, 2022

theStack force-pushed on Mar 14, 2022

DrahtBot commented at 9:50 pm on March 14, 2022: member

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.

theStack force-pushed on Mar 17, 2022

laanwj commented at 12:06 pm on April 5, 2022: member

Concept ACK, thanks for adding tests

in test/functional/tool_signet_miner.py:51 in d1d98c247d outdated

46+        node.importprivkey(bytes_to_wif(CHALLENGE_PRIVATE_KEY))
47+
48+        # determine paths to tools
49+        base_dir = self.config["environment"]["SRCDIR"]
50+        signet_miner_path = os.path.join(base_dir, "contrib", "signet", "miner")
51+        # TODO: this is a dirty hack; the path to bitcoin-util should ideally

laanwj commented at 12:17 pm on April 5, 2022:

I don’t think it’s good to leave it like this, if we use bitcoin-util in the functional tests we’d at least need to

Add a self.options.bitcoinutil in BitcoinTestFramework.setup, analogous to the other two, overridable with BITCOINUTIL
we also need an entry in config.ini.in for BUILD_BITCOIN_UTIL → an is_bitcoin_util_compiled() → and an skip_if_no_bitcoin_util

theStack commented at 6:13 pm on April 5, 2022:

Thanks for the guide, will add a commit with these preliminaries within the next days.

test: determine path to `bitcoin-util` in test framework

The path is stored in `self.options.bitcoinutil`, points to
`src/bitcoin-util` by default and can be overrided with the
`BITCOINUTIL` environment variable.

dde33eca63

test: add `is_bitcoin_util_compiled` helper 449b96ed97

theStack force-pushed on Apr 11, 2022

theStack commented at 8:11 pm on April 11, 2022: member

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](https://github.com/bitcoin/bitcoin/pull/24559#discussion_r842715566).

laanwj commented at 7:52 pm on April 13, 2022: member

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

in test/functional/tool_signet_miner.py:38 in f8abc66ca1 outdated

33+        self.extra_args = [[f'-signetchallenge={challenge.hex()}']]
34+
35+    def skip_test_if_missing_module(self):
36+        self.skip_if_no_cli()
37+        self.skip_if_no_wallet()
38+        self.skip_if_no_bdb()

laanwj commented at 8:04 pm on April 13, 2022:

Is there a specific reason to require bdb here? I don’t think it even specifically makes a bdb wallet?

Edit: oh, you answer this in the OP “Note that the test only works with legacy wallet right now, since it relies on the importprivkey to import the issuer’s private key; not sure yet how this could be solved with descriptor wallets.”.

Might want to add a comment in the code, so people stumbling on this “why does this unrelated test require bdb” have an answer.

theStack commented at 10:39 pm on April 13, 2022:

The problem was the signing in the descriptor wallet rather than the import of the private key. Now it should work with both wallet types: #24559 (comment)

0xB10C commented at 9:27 pm on April 13, 2022: member

Concert ACK!

test: add test for signet miner script 038d2a607f

theStack force-pushed on Apr 13, 2022

theStack commented at 10:36 pm on April 13, 2022: member

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 commented at 5:03 am on April 14, 2022: member

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

laanwj merged this on Apr 14, 2022

laanwj closed this on Apr 14, 2022

theStack deleted the branch on Apr 14, 2022

sidhujag referenced this in commit 830f886f14 on Apr 14, 2022

DrahtBot locked this on Apr 14, 2023

test: add test for signet miner script #24559

Conflicts