test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets #28027

achow101 commented at 3:03 AM on July 4, 2023: member

It was somewhat surprising to me that wallet_backwards_compatibility.py did not catch #27915 since the purpose of the test is to find downgrade issues such as that. It turns out the test was deficient in several places when it came to testing descriptor wallets, as well as deficient in addition to failing to correctly test some releases.

This PR fixes these test cases, adds more informative logging, slightly refactors the entire test in order to better test future versions, and adds a 25.0 node to the test.

Notable changes:

The compatibility test with 0.16 should not have been passing. The wallets were being copied incorrectly for 0.16 and resulting in 0.16 creating new wallets rather than testing the target wallets.
The downgrade test will actually be run on descriptor wallets and it will test that downgrades are successful, and a subsequent upgrade is also successful. This catches #27915.
The upgrade and downgrade test will be run on all versions up to master, rather than just 0.16, 0.17, and 0.19.

DrahtBot commented at 3:03 AM on July 4, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, Sjors
Concept ACK	ismaelsadeeq
Approach ACK	S3RK

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28528 (test: Use test framework utils in functional tests by osagie98)
#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#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 added the label Tests on Jul 4, 2023

achow101 force-pushed on Jul 4, 2023

DrahtBot added the label CI failed on Jul 4, 2023

achow101 force-pushed on Jul 4, 2023

in test/functional/wallet_backwards_compatibility.py:169 in 95b140a4b7 outdated

 169 | -                    os.path.join(self.nodes_wallet_dir(node), wallet)
 170 | -                )
 171 | +                if node.version == 160300:
 172 | +                    # 0.16 node expect the wallet to be in the wallet dir but as a plain file rather than in directories
 173 | +                    shutil.copyfile(
 174 | +                        os.path.join(node_master.wallets_path, wallet, "wallet.dat"),

maflcko commented at 6:05 AM on July 4, 2023:

only if you re-touch, style nit in 95b140a4b75c61e4ae3f4d1ea6e2d4a6b9fa15e8 (and following commits): The *_path node properties can use the shorter a / b / c syntax, over os.path.join(a, b, c).

achow101 commented at 5:11 PM on July 10, 2023:

Done where the code was more than just moved.

fanquake requested review from Sjors on Jul 4, 2023

Sjors commented at 2:29 PM on July 4, 2023: member

Thanks for improving these tests. Added to review list.

ismaelsadeeq commented at 9:32 PM on July 4, 2023: member

Concept ACK

DrahtBot removed the label CI failed on Jul 5, 2023

DrahtBot added the label Needs rebase on Jul 6, 2023

in test/functional/wallet_backwards_compatibility.py:183 in 604440c0b8 outdated

 199 | -                    if node.version < 180000 and wallet_name == "w3":
 200 | -                        # Blank wallets were introduced in v0.18.0. We test the loading error below.
 201 | -                        continue
 202 | -                    node.loadwallet(wallet_name)
 203 | -                    wallet = node.get_wallet_rpc(wallet_name)
 204 | +                if node.version == 160300:

S3RK commented at 7:43 AM on July 10, 2023:

v0.16 is different in many regards, does it make sense to have a separate test to check incompatibility?

achow101 commented at 5:12 PM on July 10, 2023:

Perhaps, but I think we should test it in this test as it's important to have the upgrade and downgrade tests for the older wallet version.

S3RK commented at 6:50 AM on July 11, 2023:

agree, you can resolve this

Sjors commented at 7:27 PM on July 14, 2023:

c31e0db7f4263096d503f07707af9d0a733e50da The original < 170000 made it more clear that this applies to all old versions. Not a big deal though, since it's unlikely we'll add even older nodes.

achow101 commented at 8:24 PM on July 14, 2023:

No, this is more correct. This behavior doesn't apply to all older versions, only to 0.16.x. Versions prior to this kept the wallet files in the main datadir. 0.16 added the wallet directory but the wallets are still per file. 0.17 moved wallets into the current one directory per wallet structure.

in test/functional/wallet_backwards_compatibility.py:255 in 604440c0b8 outdated

 249 | +
 250 | +        # Check that descriptor wallets don't work on legacy only nodes
 251 | +        if self.options.descriptors:
 252 | +            self.log.info("Test descriptor wallet incompatibility on:")
 253 | +            for node in legacy_only_nodes:
 254 | +                if node.version < 180000:

S3RK commented at 7:46 AM on July 10, 2023:

pull up the comment explaining why?

achow101 commented at 5:12 PM on July 10, 2023:

Done

achow101 force-pushed on Jul 10, 2023

DrahtBot removed the label Needs rebase on Jul 10, 2023

in test/functional/wallet_backwards_compatibility.py:109 in 0ffa0f1c37 outdated

  82 | +        wallet = node_v19.get_wallet_rpc("w1_v19")
  83 | +        info = wallet.getwalletinfo()
  84 | +        assert info['private_keys_enabled']
  85 | +        assert info['keypoolsize'] > 0
  86 | +        # Use addmultisigaddress (see #18075)
  87 | +        address_18075 = wallet.rpc.addmultisigaddress(1, ["0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52", "037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073"], "", "legacy")["address"]

ismaelsadeeq commented at 11:40 PM on July 10, 2023:

The v19 test pubkeys 0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52 and 037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073 are they relevant to the test? I see they are the same as the one reported in #18075

achow101 commented at 7:20 PM on July 13, 2023:

I don't think the specific keys are relevant.

in test/functional/wallet_backwards_compatibility.py:206 in 3e7bbf3a86 outdated

 238 | +            if node.version < 170000:
 239 | +                # loadwallet was introduced in v0.17.0
 240 | +                continue
 241 | +            self.log.info(f"- {node.version}")
 242 | +            for wallet_name in ["w1", "w2", "w3"]:
 243 | +                if node.version < 180000 and wallet_name == "w3":

ismaelsadeeq commented at 11:46 PM on July 10, 2023:

180000, 210000, e.t.c To reduce the magic numbers we can create a variable for the versions, e.g v18 = 180000and reuse it throughout the tests?

achow101 commented at 7:24 PM on July 13, 2023:

Will do if I need to retouch.

achow101 commented at 8:57 PM on July 14, 2023:

Instead of using magic numbers, I've changed the test to use helper functions that check the major version.

in test/functional/wallet_backwards_compatibility.py:135 in 6a5d245b89 outdated

 125 | @@ -127,13 +126,6 @@ def run_test(self):
 126 |          assert wallet.getaddressinfo(address_18075)["solvable"]
 127 |          node_v19.unloadwallet("w1_v19")
 128 |  
 129 | -        # w1_v18: regular wallet, created with v0.18

ismaelsadeeq commented at 11:51 PM on July 10, 2023:

Why are we no longer using the wallet?

achow101 commented at 7:24 PM on July 13, 2023:

I would guess that it was used in the up-downgrade test but was accidentally removed at some point. It isn't necessary anymore either as that test is now automated to go over all of the previous versions so the hardcoded specific wallet versions are no longer necessary.

ismaelsadeeq commented at 12:11 AM on July 11, 2023: member

Running this test failed on my device not sure if it's from this PR though.

Steps to recreate git checkout pr/28027 make clean ./autogen.sh ./configure make test/functional/test_runner.py wallet_backwards_compatibility.py

2023-07-11T00:04:00.468000Z TestFramework (INFO): PRNG seed is: 1124432296273551722
2023-07-11T00:04:00.468000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_banbrcm1
2023-07-11T00:04:00.472000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 550, in start_nodes
    node.start(extra_args[i], *args, **kwargs)
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 221, in start
    self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
  File "/usr/lib/python3.10/subprocess.py", line 969, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib/python3.10/subprocess.py", line 1845, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/home/abubakarsadiq/Desktop/bitcoin/releases/v25.0/bin/bitcoind'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 130, in main
    self.setup()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 295, in setup
    self.setup_network()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 389, in setup_network
    self.setup_nodes()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/wallet_backwards_compatibility.py", line 74, in setup_nodes
    self.start_nodes()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 555, in start_nodes
    self.stop_nodes()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 570, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 346, in stop_node
    self.stop(wait=wait)
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 192, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
2023-07-11T00:04:00.523000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/wallet_backwards_compatibility.py", line 368, in <module>
    BackwardsCompatibilityTest().main()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 154, in main
    exit_code = self.shutdown()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 311, in shutdown
    self.stop_nodes()
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 570, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 346, in stop_node
    self.stop(wait=wait)
  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 192, in __getattr__
    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
[node 1] Cleaning up leftover process
[node 0] Cleaning up leftover process

Left some comments and question

S3RK commented at 6:51 AM on July 11, 2023: contributor

Approach ACK

achow101 commented at 7:19 PM on July 13, 2023: member

Running this test failed on my device not sure if it's from this PR though.

It looks like you're missing the 25.0 binaries. The test is expecting to find 25.0's bitcoind at /home/abubakarsadiq/Desktop/bitcoin/releases/v25.0/bin/bitcoind but doesn't.

in test/functional/wallet_backwards_compatibility.py:78 in 6a5d245b89 outdated

  75 | @@ -76,7 +76,6 @@ def run_test(self):
  76 |          node_miner = self.nodes[0]
  77 |          node_master = self.nodes[1]
  78 |          node_v19 = self.nodes[self.num_nodes - 4]

Sjors commented at 7:35 PM on July 14, 2023:

6a5d245b89c51a53989b984d31d7bb4d807837c3: if a node is node is not used, we should reduce the number of nodes and node_v19 should be self.num_nodes - 3.

achow101 commented at 8:13 PM on July 14, 2023:

0.18 is still tested, just not explicitly with its own special casing and variables.

in test/functional/wallet_backwards_compatibility.py:267 in 72362ed1fb outdated

 260 | @@ -261,6 +261,12 @@ def run_test(self):
 261 |              else:
 262 |                  node_v16.assert_start_raises_init_error([f"-wallet={wallet_name}"], f"Error: Error loading {wallet_name}: Wallet requires newer version of Bitcoin Core")
 263 |  
 264 | +        # When descriptors are enabled, w1 cannot be opened by 0.21 sinced it contains a taproot descriptor
 265 | +        if self.options.descriptors:
 266 | +            self.log.info("Test that 0.21 cannot open wallet containing tr() descriptors")
 267 | +            node_v21 = self.nodes[-6]

Sjors commented at 7:40 PM on July 14, 2023:

72362ed1fbbab9dfbaf224f930ed9b328f29c55f: maybe keep all the node_v.. definitions in one place at the top of the test?

achow101 commented at 8:57 PM on July 14, 2023:

Done

Sjors commented at 8:06 PM on July 14, 2023: member

0ffa0f1c370494b6b99a9cdf911b20ee655ac829 (and later) breaks at line line 96 when you run wallet_backwards_compatibility.py --descriptors without BDB:

JSONRPCException: Wallet file verification failed. Failed to open database path '/tmp/bitcoin_func_test_x3n9_0vn/node1/regtest/wallets/w1_v19'. Build does not support Berkeley DB database format. (-18)

Otherwise it works, just some stylistic notes inline (e40896845a57bb5bf657f51dc007c6563e4ca22c).

0.16 creating new wallets rather than testing the target wallets

Is there an assert you can add in c31e0db7f4263096d503f07707af9d0a733e50da that would catch this directly? Right now a regression would be "caught" simply because it doesn't fail to load. Ideally we'd check this for every version before we dropped the "just create a new one if it doesn't exist" behavior was dropped.

achow101 force-pushed on Jul 14, 2023

achow101 commented at 9:01 PM on July 14, 2023: member

0ffa0f1 (and later) breaks at line line 96 when you run wallet_backwards_compatibility.py --descriptors without BDB:

Fixed

0.16 creating new wallets rather than testing the target wallets

Is there an assert you can add in c31e0db that would catch this directly? Right now a regression would be "caught" simply because it doesn't fail to load. Ideally we'd check this for every version before we dropped the "just create a new one if it doesn't exist" behavior was dropped.

With the versions that have loadwallet, we catch that case by checking the contents of the wallet. The test previously wasn't doing that for 0.16 (it was actually one of the first things I changed before I realized there was an incompatibility).

test: Add helper functions for checking node versions 5d8469362a

achow101 force-pushed on Jul 14, 2023

DrahtBot added the label CI failed on Jul 14, 2023

achow101 force-pushed on Jul 14, 2023

DrahtBot removed the label CI failed on Jul 15, 2023

in test/functional/wallet_backwards_compatibility.py:191 in df850c257c outdated

 187 |          for node in legacy_nodes:
 188 |              # Copy wallets to previous version
 189 |              for wallet in os.listdir(node_master_wallets_dir):
 190 | -                shutil.copytree(
 191 | -                    os.path.join(node_master_wallets_dir, wallet),
 192 | -                    os.path.join(self.nodes_wallet_dir(node), wallet)

furszy commented at 3:05 PM on July 15, 2023:

In df850c25: Removed the only usage of the nodes_wallet_dir function but not the, now unused, function.

achow101 commented at 8:42 PM on August 23, 2023:

Done

in test/functional/wallet_backwards_compatibility.py:199 in df850c257c outdated

 199 | +                    )
 200 | +                else:
 201 | +                    shutil.copytree(
 202 | +                        node_master_wallets_dir / wallet,
 203 | +                        node.wallets_path / wallet,
 204 | +                    )

furszy commented at 3:17 PM on July 15, 2023:

In df850c25:

Could simplify this:

            # Copy wallets to previous version
            for wallet in os.listdir(node_master_wallets_dir):
                source = node_master_wallets_dir / wallet
                if self.major_version_equals(node, 16):
                    # 0.16 node expect the wallet to be in the wallet dir but as a plain file rather than in directories
                    source / "wallet.dat"
                shutil.copytree(source, node.wallets_path / wallet)

achow101 commented at 8:42 PM on August 23, 2023:

Done

in test/functional/wallet_backwards_compatibility.py:311 in 91f47e676a outdated

 330 | +            if self.options.descriptors:
 331 | +                node_v16.assert_start_raises_init_error([f"-wallet={wallet_name}"], f"Error: {wallet_name} corrupt, salvage failed")
 332 | +            else:
 333 | +                node_v16.assert_start_raises_init_error([f"-wallet={wallet_name}"], f"Error: Error loading {wallet_name}: Wallet requires newer version of Bitcoin Core")
 334 | +
 335 | +        # When descriptors are enabled, w1 cannot be opened by 0.21 sinced it contains a taproot descriptor

furszy commented at 3:40 PM on July 15, 2023:

s/sinced/since

achow101 commented at 8:43 PM on August 23, 2023:

Done

Sjors commented at 1:12 PM on July 28, 2023: member

ACK 91f47e676ae3f131e7cecae9b5b5e50b358e9b32

Also tested that reverting 6a9510d2dabde1c9ee6c4226e3d16ca32eb48ac5 causes the test to fail when it tries to load the wallet in master after having loaded it in v25 (Wallet corrupted).

achow101 force-pushed on Aug 23, 2023

test: Fix 0.16 wallet paths and downgrade test

The test for 0.16 wallet downgrading was using the wrong wallet path and
thus incorrectly finding that 0.16 could open wallets created in master.

313d665437

test: Remove w1_v18 from wallet backwards compatibility

This wallet is no longer used in the test

53f35d02cb

test: Refactor v19 addmultisigaddress test to be distinct

This specific test is distinct from the rest of the backwards
compatibility tests as it is checking a specific failure.

71c03aeff7

test: add logging 0.17 incompatibilities in wallet back compat f41215c3f0

test: Add 0.21 tr() incompatibility test f158573be1

test: Run downgrade test on descriptor wallets 6d4699028b

test: Run upgrade test on all nodes 538939ec39

test: Add 25.0 to wallet backwards compatibiilty test bbf43c63b9

test: roundtrip wallet backwards compat downgrade

Test that old nodes don't mess up new wallets by loading a downgraded
wallet in master again.

afd9a673c4

achow101 force-pushed on Aug 23, 2023

furszy commented at 10:55 PM on August 23, 2023: member

ACK afd9a67

DrahtBot requested review from Sjors on Aug 23, 2023

achow101 requested review from S3RK on Oct 4, 2023

Sjors commented at 7:46 AM on October 5, 2023: member

re-ACK afd9a673c458e97305da49a70a1ddbf60e651876

DrahtBot removed review request from Sjors on Oct 5, 2023

maflcko added this to the milestone 26.0 on Oct 5, 2023

fanquake merged this on Oct 5, 2023

fanquake closed this on Oct 5, 2023

bitcoin locked this on Oct 4, 2024