test: Split rpc_signmessage test for disabled wallet #22641
Conversation
maflcko
changed the title
There was a problem hiding this comment.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
There was a problem hiding this comment.
tAck 1701949
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
|
|
||
| self.log.info('test parameter validity and error codes') | ||
| # signmessage(withprivkey) have two required parameters | ||
| for num_params in [0, 1, 3, 4, 5]: |
There was a problem hiding this comment.
Is there a reason for running the loop here on L32 and L36 from 0 to 5?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright! Makes sense then
| def set_test_params(self): | ||
| self.setup_clean_chain = True | ||
| self.num_nodes = 1 | ||
| self.extra_args = [["-addresstype=legacy"]] |
There was a problem hiding this comment.
I don't think this argument makes any difference without wallet?
Edit: yes, it works fine without this line
There was a problem hiding this comment.
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:
rpc_signmessagewithprivkey.py | ✓ Passed | 1 s
wallet_signmessagewithaddress.py | ✓ Passed | 1 s
When compiled with --disable-wallet:
rpc_signmessagewithprivkey.py | ✓ Passed | 1 s
wallet_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
::
$ git log --oneline --grep='^test:' origin/master |wc -l
829
$ git log --oneline --grep='^Test:' origin/master |wc -l
3
$ git log --oneline --grep='^Test :' origin/master |wc -l
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.
| assert self.nodes[0].verifymessage(address, signature, message) | ||
|
|
||
| self.log.info('test parameter validity and error codes') | ||
| # signmessage(withprivkey) have two required parameters |
There was a problem hiding this comment.
s/signmessage(withprivkey) have/signmessagewithprivkey has/ because this test is now executing just one RPC - signmessagewithprivkey.
| @@ -45,18 +36,16 @@ def run_test(self): | |||
| # signmessage(withprivkey) have two required parameters | |||
There was a problem hiding this comment.
s/signmessage(withprivkey) have/signmessage has/ because this test is now executing just one RPC - signmessage.
| # verifymessage has three required parameters | ||
| for num_params in [0, 1, 2, 4, 5]: | ||
| param_list = ["dummy"]*num_params | ||
| assert_raises_rpc_error(-1, "verifymessage", self.nodes[0].verifymessage, *param_list) |
There was a problem hiding this comment.
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.
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.
Shubhankar-Gambhir
changed the title
maflcko
changed the title
There was a problem hiding this comment.
Code-review ACK a3b559c
(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.
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.