test: Verify if wallet is compiled in rpc_invalid_address

lsilva01 commented at 10:52 pm on August 24, 2021: contributor

Most of test/functional/rpc_invalid_address_message.py does not requires wallet. But if the project is compiled in disable-wallet mode, the entire test will be skipped.

This PR changes the test to run the RPC tests first and then checks if the wallet is compiled.

lsilva01 force-pushed on Aug 24, 2021

fanquake added the label Tests on Aug 25, 2021

in test/functional/rpc_invalid_address_message.py:20 in 90e382b512 outdated

16 BECH32_INVALID_BECH32 = 'bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqdmchcc'
17 BECH32_INVALID_BECH32M = 'bcrt1qw508d6qejxtdg4y5r3zarvary0c5xw7k35mrzd'
18-BECH32_INVALID_VERSION = 'bcrt130xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqynjegk'
19 BECH32_INVALID_SIZE = 'bcrt1s0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v8n0nx0muaewav25430mtr'
20 BECH32_INVALID_V0_SIZE = 'bcrt1qw508d6qejxtdg4y5r3zarvary0c5xw7kqqq5k3my'
21 BECH32_INVALID_PREFIX = 'bc1pw508d6qejxtdg4y5r3zarvary0c5xw7kw508d6qejxtdg4y5r3zarvary0c5xw7k7grplx'

Shubhankar-Gambhir commented at 5:41 am on August 28, 2021:

Test can have more invalid address types, check for different error messages in https://github.com/bitcoin/bitcoin/blob/master/src/key_io.cpp#:~:text=CTxDestination%20DecodeDestination(const,%7D%20//%20namespace

lsilva01 commented at 10:18 pm on September 8, 2021:

Thanks. Test added.

Shubhankar-Gambhir commented at 5:46 am on August 28, 2021: contributor

concept ACK there are 2 subtests and only one requires wallet, this PR enables the other one to run even with disabled wallet. although instead of splitting can add an if condition (using self.is_wallet_compiled()) in runtests.

aitorjs commented at 0:31 am on August 29, 2021: contributor

Makes sense to split in two file. There are 2 subtests and only one requires wallet as @Shubhankar-Gambhir sayd.
self.skip_if_no_wallet() calls to self.is_wallet_compiled(). Inside bitcoin functional tests, self.skip_if_no_wallet() is mainly used (77 times) and self.is_wallet_compiled() (16 times). I think that self.is_wallet_compiled() is used for some special cases and this one is not one of them. So, self.skip_if_no_wallet() is better.
ACK (90e382b5123ae97dc8f536e847ca790e21467446)

Tests I have done:

Run tests on compiled bitcoin with PR changes and wallet:

 0$ test/functional/rpc_invalid_address_message.py
 12021-08-27T19:39:18.334000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_q38ex0pf
 22021-08-27T19:39:18.648000Z TestFramework (INFO): Stopping nodes
 32021-08-27T19:39:18.803000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_q38ex0pf on exit
 42021-08-27T19:39:18.803000Z TestFramework (INFO): Tests successful
 5
 6$ test/functional/wallet_invalid_address_message.py
 72021-08-27T19:40:12.106000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_f3rhiwtn
 82021-08-27T19:40:12.563000Z TestFramework (INFO): Stopping nodes
 92021-08-27T19:40:12.669000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_f3rhiwtn on exit
102021-08-27T19:40:12.669000Z TestFramework (INFO): Tests successful

Run tests on compiled bitcoin with PR changes but without wallet (–disable-wallet):

 0$ test/functional/rpc_invalid_address_message.py
 12021-08-27T19:34:27.769000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ahdk12up
 22021-08-27T19:34:28.115000Z TestFramework (INFO): Stopping nodes
 32021-08-27T19:34:28.271000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_ahdk12up on exit
 42021-08-27T19:34:28.271000Z TestFramework (INFO): Tests successful
 5
 6$ test/functional/wallet_invalid_address_message.py
 72021-08-27T19:34:38.719000Z TestFramework (WARNING): Test Skipped: wallet has not been compiled.
 82021-08-27T19:34:38.769000Z TestFramework (INFO): Stopping nodes
 92021-08-27T19:34:38.769000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_2hs3yym1 on exit
102021-08-27T19:34:38.769000Z TestFramework (INFO): Test skipped

Run tests on compiled bitcoin from master without wallet (–disable-wallet):

0$ test/functional/rpc_invalid_address_message.py
12021-08-28T23:38:12.242000Z TestFramework (WARNING): Test Skipped: wallet has not been compiled.
22021-08-28T23:38:12.294000Z TestFramework (INFO): Stopping nodes
32021-08-28T23:38:12.294000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_mif2y1xy on exit
42021-08-28T23:38:12.294000Z TestFramework (INFO): Test skipped
5
6$ test/functional/wallet_invalid_address_message.py
7bash: test/functional/wallet_invalid_address_message.py: File or folder does not exist

josibake approved

josibake commented at 11:38 am on August 31, 2021: member

ACK https://github.com/bitcoin/bitcoin/pull/22794/commits/90e382b5123ae97dc8f536e847ca790e21467446

verified the correct tests run as expected with and without wallet compiled. you may want to add a note indicating this is “move only” to the commit or pr description. it helps reviewers know what they should be looking for

in test/functional/rpc_invalid_address_message.py:17 in 90e382b512 outdated

13 )
14 
15 BECH32_VALID = 'bcrt1qtmp74ayg7p24uslctssvjm06q5phz4yrxucgnv'
16 BECH32_INVALID_BECH32 = 'bcrt1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqdmchcc'
17 BECH32_INVALID_BECH32M = 'bcrt1qw508d6qejxtdg4y5r3zarvary0c5xw7k35mrzd'
18-BECH32_INVALID_VERSION = 'bcrt130xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7vqynjegk'

MarcoFalke commented at 1:04 pm on August 31, 2021:

Instead of removing this, I think a test should added instead?

lsilva01 commented at 10:17 pm on September 8, 2021:

Sure. Test added.

MarcoFalke commented at 1:07 pm on August 31, 2021: member

I think it is acceptable to use is_wallet_compiled here to conditionally execute the tests. For test coverage it will be no difference.

Downside is that wallet-rpcs are mixed with non-wallet-rpcs in the same file, but the benefit would be that the same node can be used to run the tests without restart (should be faster).

Shubhankar-Gambhir commented at 12:36 pm on September 3, 2021: contributor

Just in case i was not clear earlier the below patch would achieve the same

 0diff --git a/test/functional/rpc_invalid_address_message.py b/test/functional/rpc_invalid_address_message.py
 1index e362642f0..6b92b2fe1 100755
 2--- a/test/functional/rpc_invalid_address_message.py
 3+++ b/test/functional/rpc_invalid_address_message.py
 4@@ -29,9 +29,6 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework):
 5         self.setup_clean_chain = True
 6         self.num_nodes = 1
 7 
 8-    def skip_test_if_missing_module(self):
 9-        self.skip_if_no_wallet()
10-
11     def test_validateaddress(self):
12         node = self.nodes[0]
13 
14@@ -87,7 +84,10 @@ class InvalidAddressErrorMessageTest(BitcoinTestFramework):
15 
16     def run_test(self):
17         self.test_validateaddress()
18-        self.test_getaddressinfo()
19+
20+        # skip if wallet is disabled
21+        if self.is_wallet_compiled():
22+            self.test_getaddressinfo()
23 
24 
25 if __name__ == '__main__':

lsilva01 force-pushed on Sep 8, 2021

lsilva01 commented at 10:21 pm on September 8, 2021: contributor

The test has been changed to use is_wallet_compiled and init_wallet.

Also added test for BECH32_INVALID_VERSION case.

Shubhankar-Gambhir commented at 7:36 pm on September 20, 2021: contributor

Approach ACK test was succesful when run on bitcoin-core compiled without wallet (–disable-wallet):

02021-09-20T19:30:10.881000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_bha1iotz
12021-09-20T19:30:26.999000Z TestFramework (INFO): Stopping nodes
22021-09-20T19:30:33.380000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_bha1iotz on exit
32021-09-20T19:30:33.380000Z TestFramework (INFO): Tests successful

Shubhankar-Gambhir approved

in test/functional/rpc_invalid_address_message.py:88 in 8fff59716c outdated

83@@ -87,7 +84,11 @@ def test_getaddressinfo(self):
84 
85     def run_test(self):
86         self.test_validateaddress()
87-        self.test_getaddressinfo()
88+
89+        # skip if wallet is disabled

MarcoFalke commented at 7:26 am on September 21, 2021:

nit: I don’t think the comment is needed, when the code is self-explanatory

lsilva01 commented at 10:28 pm on September 21, 2021:

Really, the comment is not necessary. Removed.

MarcoFalke commented at 7:27 am on September 21, 2021: member

Needs OP/Title adjusted before merge

MarcoFalke approved

pg156 commented at 5:16 pm on September 21, 2021: none

Concept and approach ACK

pg156 commented at 5:22 pm on September 21, 2021: none

Out of curiosity, how did you notice the non-wallet-rpc test is skipped when wallet is not enabled (before your PR)? Thanks @lsilva01.

in test/functional/rpc_invalid_address_message.py:93 in 8fff59716c outdated

83@@ -87,7 +84,11 @@ def test_getaddressinfo(self):
84 
85     def run_test(self):
86         self.test_validateaddress()
87-        self.test_getaddressinfo()
88+
89+        # skip if wallet is disabled
90+        if self.is_wallet_compiled():
91+            self.init_wallet(0)

jonatack commented at 5:58 pm on September 21, 2021:

unrelated to this change, but it might be nice for init_wallet() to be defined as

0-    def init_wallet(self, i):
1+    def init_wallet(self, *, node):

so the calls become self.init_wallet(node=0)

use of splat optional, to be decided.

For now there is only a handful of calls to update.

lsilva01 commented at 10:34 pm on September 21, 2021:

Interesting. I will open a new PR for this change.

lsilva01 commented at 3:42 am on October 20, 2021:

Implemented this change in #23316 .

jonatack commented at 6:06 pm on September 21, 2021: member

Untested light review ACK f9cf67428ec25f98559a72a2504452e4b994acd7

(modulo #22794#pullrequestreview-759390259)

skip test_getaddressinfo() if wallet is disabled b142f79ddb

Add BECH32_INVALID_VERSION test c2fbdca549

lsilva01 force-pushed on Sep 21, 2021

lsilva01 commented at 10:33 pm on September 21, 2021: contributor

@p16n The code below always skips the test if the wallet is not enabled.

0def skip_test_if_missing_module(self):
1        self.skip_if_no_wallet()

You can verify this in the test/functional/test_framework.py/BitcoinTestFramework/skip_if_no_wallet file.

0def skip_if_no_wallet(self):
1        """Skip the running test if wallet has not been compiled."""
2        self.requires_wallet = True
3        if not self.is_wallet_compiled():
4            raise SkipTest("wallet has not been compiled.")
5        if self.options.descriptors:
6            self.skip_if_no_sqlite()
7        else:
8            self.skip_if_no_bdb()

lsilva01 renamed this:
~~test: Split rpc_invalid_address_message test into two~~
test: Test if wallet is compiled in rpc_invalid_address_message.py test
on Sep 21, 2021

lsilva01 renamed this:
~~test: Test if wallet is compiled in rpc_invalid_address_message.py test~~
test: Verify if wallet is compiled in rpc_invalid_address_message.py test
on Sep 21, 2021

MarcoFalke approved

DrahtBot commented at 10:30 pm on October 1, 2021: 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:

#16807 (Let validateaddress locate error in Bech32 address by meshcollider)

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.

stratospher commented at 9:16 am on October 11, 2021: contributor

tested ACK c2fbdca

rpc_invalid_address_message.py checks whether correct error messages are generated for invalid addresses. Since getaddressinfo RPC requires a wallet and validateaddress RPC does not require a wallet, the tests which invoke validateaddress should work even when it’s compiled without a wallet. This PR fixes the problem by always running the tests which invoke validateaddress and running the tests which invoke getaddressinfo only when the wallet is compiled.

Test results when compiled in disable-wallet mode:

0$ test/functional/rpc_invalid_address_message.py
12021-10-11T08:20:44.641000Z TestFramework (WARNING): Test Skipped: wallet has not been compiled.
22021-10-11T08:20:44.695000Z TestFramework (INFO): Stopping nodes
32021-10-11T08:20:44.696000Z TestFramework (INFO): Cleaning up /var/folders/bh/7x61zw991x15d01dlnx_cgrh0000gn/T/bitcoin_func_test__2uih3yl on exit
42021-10-11T08:20:44.696000Z TestFramework (INFO): Test skipped

0$ test/functional/rpc_invalid_address_message.py
12021-10-11T08:55:10.132000Z TestFramework (INFO): Initializing test directory /var/folders/bh/7x61zw991x15d01dlnx_cgrh0000gn/T/bitcoin_func_test_v7gdmep5
22021-10-11T08:55:10.994000Z TestFramework (INFO): Stopping nodes
32021-10-11T08:55:11.149000Z TestFramework (INFO): Cleaning up /var/folders/bh/7x61zw991x15d01dlnx_cgrh0000gn/T/bitcoin_func_test_v7gdmep5 on exit
42021-10-11T08:55:11.149000Z TestFramework (INFO): Tests successful

MarcoFalke merged this on Oct 11, 2021

MarcoFalke closed this on Oct 11, 2021

sidhujag referenced this in commit 476983a089 on Oct 11, 2021

MarcoFalke referenced this in commit 1435161f64 on Oct 20, 2021

sidhujag referenced this in commit 190e2200b9 on Oct 20, 2021

lsilva01 deleted the branch on Oct 21, 2021

DrahtBot locked this on Oct 30, 2022

test: Verify if wallet is compiled in rpc_invalid_address_message.py test #22794

Conflicts