test: Split rpc_signmessage test for disabled wallet

Shubhankar-Gambhir commented at 9:53 pm on August 5, 2021: contributor

This PR enables a part of the non-wallet functional test (rpc_signmessage.py) to be run even with the Bitcoin Core wallet disabled, it is inspired by #20078.

Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled that provides extra test which was not performed earlier.

we need bitcoincore wallet to run rpc_signmessage.py, but it is olny required for signing messages with address and not for signing messages wih private key, so latter one can be in a seperate test which can run without wallet
verifying message doesn’t require wallet, so it can be used in both tests without any problem
2 tests are named as wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py to provide clarity of what they are testing.

DrahtBot added the label Tests on Aug 5, 2021

MarcoFalke renamed this:
~~Test : Added test for disabled wallet~~
Test: Added test for disabled wallet
on Aug 6, 2021

in test/functional/test_runner.py:227 in 2910b5ddfb outdated

223@@ -224,7 +224,8 @@
224     'wallet_importprunedfunds.py --descriptors',
225     'p2p_leak_tx.py',
226     'p2p_eviction.py',
227-    'rpc_signmessage.py',
228+    'rpc_signmessagewithaddress.py',

MarcoFalke commented at 7:05 am on August 6, 2021:

wallet tests should to be prefixed with wallet_ (see our docs on prefixes)

Shubhankar-Gambhir commented at 8:45 am on August 7, 2021:

Done, thanks for pointing out.

MarcoFalke commented at 7:05 am on August 6, 2021: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

in test/functional/rpc_signmessagewithprivkey.py:31 in 17019494d9 outdated

27+        assert_equal(expected_signature, signature)
28+        assert self.nodes[0].verifymessage(address, signature, message)
29+
30+        self.log.info('test parameter validity and error codes')
31+        # signmessage(withprivkey) have two required parameters
32+        for num_params in [0, 1, 3, 4, 5]:

ShubhamPalriwala commented at 4:15 am on August 10, 2021:

Is there a reason for running the loop here on L32 and L36 from 0 to 5?

vasild commented at 12:13 pm on August 11, 2021:

It skips 2 (signmessagewithprivkey has two required parameters). I think the intention here is to ensure the RPC returns an error if called with the wrong number of parameters.

ShubhamPalriwala commented at 8:04 am on August 12, 2021:

Alright! Makes sense then

ShubhamPalriwala commented at 4:21 am on August 10, 2021: contributor

tAck 17019494d9ae381fa75bf469ca10bdad95df7ae1

Decoupling the signing of messages with a private key and with an address opens up the doors for further individual testing in the future.

Tested on Ubuntu 21.04

in test/functional/rpc_signmessagewithprivkey.py:17 in 17019494d9 outdated

12+
13+class SignMessagesWithPrivTest(BitcoinTestFramework):
14+    def set_test_params(self):
15+        self.setup_clean_chain = True
16+        self.num_nodes = 1
17+        self.extra_args = [["-addresstype=legacy"]]

laanwj commented at 9:40 am on August 10, 2021:

I don’t think this argument makes any difference without wallet? Edit: yes, it works fine without this line

in test/functional/rpc_signmessagewithprivkey.py:31 in 17019494d9 outdated

26+        signature = self.nodes[0].signmessagewithprivkey(priv_key, message)
27+        assert_equal(expected_signature, signature)
28+        assert self.nodes[0].verifymessage(address, signature, message)
29+
30+        self.log.info('test parameter validity and error codes')
31+        # signmessage(withprivkey) have two required parameters

vasild commented at 11:48 am on August 11, 2021:

s/signmessage(withprivkey) have/signmessagewithprivkey has/ because this test is now executing just one RPC - signmessagewithprivkey.

in test/functional/wallet_signmessagewithaddress.py:36 in 17019494d9 outdated

35@@ -45,18 +36,16 @@ def run_test(self):
36         # signmessage(withprivkey) have two required parameters

vasild commented at 11:49 am on August 11, 2021:

s/signmessage(withprivkey) have/signmessage has/ because this test is now executing just one RPC - signmessage.

in test/functional/wallet_signmessagewithaddress.py:43 in 17019494d9 outdated

35@@ -45,18 +36,16 @@ def run_test(self):
36         # signmessage(withprivkey) have two required parameters
37         for num_params in [0, 1, 3, 4, 5]:
38             param_list = ["dummy"]*num_params
39-            assert_raises_rpc_error(-1, "signmessagewithprivkey", self.nodes[0].signmessagewithprivkey, *param_list)
40             assert_raises_rpc_error(-1, "signmessage", self.nodes[0].signmessage, *param_list)
41         # verifymessage has three required parameters
42         for num_params in [0, 1, 2, 4, 5]:
43             param_list = ["dummy"]*num_params
44             assert_raises_rpc_error(-1, "verifymessage", self.nodes[0].verifymessage, *param_list)

vasild commented at 11:55 am on August 11, 2021:

This and lines 46,47,48 below exercise verifymessage which does not require a wallet, I think they can be removed from this file (wallet_signmessagewithaddress.py) since they are also present in rpc_signmessagewithprivkey.py.

vasild commented at 12:11 pm on August 11, 2021: member

Approach ACK

I verified that this is just moving bits of a test to a separate new test (as intended).

Tested:

When compiled with --enable-wallet:

0rpc_signmessagewithprivkey.py    | ✓ Passed  | 1 s
1wallet_signmessagewithaddress.py | ✓ Passed  | 1 s

When compiled with --disable-wallet:

0rpc_signmessagewithprivkey.py    | ✓ Passed  | 1 s
1wallet_signmessagewithaddress.py | ○ Skipped | 0 s

wallet_signmessagewithaddress.py fails as expected if skip_if_no_wallet() is removed from it and the wallet is not enabled.

Just a few minor suggestions:

The first line in the commit message usually starts with a lowercase letter and has no space before the ::

0$ git log --oneline --grep='^test:' origin/master |wc -l
1     829
2
3$ git log --oneline --grep='^Test:' origin/master |wc -l
4       3
5
6$ git log --oneline --grep='^Test :' origin/master |wc -l
7       0

In the OP: s/rpc_signmessagewithaddress.py/wallet_signmessagewithaddress.py/
The commit message is a bit scarce. In general, it is good if it contains a summary of the changes and more importantly, why are they being done. It would be best if one can make sense of it by just reading the commit message, if github.com is not available.

test: added test for disabled wallet

Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and
rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled.

a3b559c970

Shubhankar-Gambhir renamed this:
~~Test: Added test for disabled wallet~~
test: added test for disabled wallet
on Aug 12, 2021

theStack commented at 8:23 pm on August 14, 2021: member

Concept ACK

MarcoFalke renamed this:
~~test: added test for disabled wallet~~
test: Split rpc_signmessage test for disabled wallet
on Aug 15, 2021

vasild approved

vasild commented at 11:28 am on August 19, 2021: member

ACK a3b559c970ada3c123ae24f21e45892734e7d494

In the OP: s/rpc_signmessagewithaddress.py/wallet_signmessagewithaddress.py/

theStack approved

theStack commented at 1:13 pm on August 19, 2021: member

Code-review ACK a3b559c970ada3c123ae24f21e45892734e7d494

(review was quite painless thanks to --color-moved=dimmed-zebra) No big deal, but if you for any reason need to retouch, I’d suggest to also adapt the commit subject to the PR title – “added test for disabled wallet” is very generic and doesn’t say much.

MarcoFalke merged this on Aug 23, 2021

MarcoFalke closed this on Aug 23, 2021

sidhujag referenced this in commit bde112ebf4 on Aug 23, 2021

DrahtBot locked this on Aug 23, 2022

test: Split rpc_signmessage test for disabled wallet #22641