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

pull achow101 wants to merge 10 commits into bitcoin:master from achow101:2023-07-test-wallet-back-compat-updates changing 2 files +203 −147
  1. 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.
  2. 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.

  3. DrahtBot added the label Tests on Jul 4, 2023
  4. achow101 force-pushed on Jul 4, 2023
  5. DrahtBot added the label CI failed on Jul 4, 2023
  6. achow101 force-pushed on Jul 4, 2023
  7. achow101 force-pushed on Jul 4, 2023
  8. 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.
  9. fanquake requested review from Sjors on Jul 4, 2023
  10. Sjors commented at 2:29 pm on July 4, 2023: member
    Thanks for improving these tests. Added to review list.
  11. ismaelsadeeq commented at 9:32 pm on July 4, 2023: member
    Concept ACK
  12. DrahtBot removed the label CI failed on Jul 5, 2023
  13. DrahtBot added the label Needs rebase on Jul 6, 2023
  14. 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.
  15. 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
  16. achow101 force-pushed on Jul 10, 2023
  17. DrahtBot removed the label Needs rebase on Jul 10, 2023
  18. 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.
  19. 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.
  20. 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.
  21. ismaelsadeeq commented at 0: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

     02023-07-11T00:04:00.468000Z TestFramework (INFO): PRNG seed is: 1124432296273551722
 12023-07-11T00:04:00.468000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_banbrcm1
 22023-07-11T00:04:00.472000Z TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 550, in start_nodes
 5    node.start(extra_args[i], *args, **kwargs)
 6  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 221, in start
 7    self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
 8  File "/usr/lib/python3.10/subprocess.py", line 969, in __init__
 9    self._execute_child(args, executable, preexec_fn, close_fds,
10  File "/usr/lib/python3.10/subprocess.py", line 1845, in _execute_child
11    raise child_exception_type(errno_num, err_msg, err_filename)
12FileNotFoundError: [Errno 2] No such file or directory: '/home/abubakarsadiq/Desktop/bitcoin/releases/v25.0/bin/bitcoind'
13
14During handling of the above exception, another exception occurred:
15
16Traceback (most recent call last):
17  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 130, in main
18    self.setup()
19  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 295, in setup
20    self.setup_network()
21  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 389, in setup_network
22    self.setup_nodes()
23  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/wallet_backwards_compatibility.py", line 74, in setup_nodes
24    self.start_nodes()
25  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 555, in start_nodes
26    self.stop_nodes()
27  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 570, in stop_nodes
28    node.stop_node(wait=wait, wait_until_stopped=False)
29  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 346, in stop_node
30    self.stop(wait=wait)
31  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 192, in __getattr__
32    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
33AssertionError: [node 0] Error: no RPC connection
342023-07-11T00:04:00.523000Z TestFramework (INFO): Stopping nodes
35Traceback (most recent call last):
36  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/wallet_backwards_compatibility.py", line 368, in <module>
37    BackwardsCompatibilityTest().main()
38  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 154, in main
39    exit_code = self.shutdown()
40  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 311, in shutdown
41    self.stop_nodes()
42  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 570, in stop_nodes
43    node.stop_node(wait=wait, wait_until_stopped=False)
44  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 346, in stop_node
45    self.stop(wait=wait)
46  File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_node.py", line 192, in __getattr__
47    assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
48AssertionError: [node 0] Error: no RPC connection
49[node 1] Cleaning up leftover process
50[node 0] Cleaning up leftover process

    Left some comments and question

  22. S3RK commented at 6:51 am on July 11, 2023: contributor
    Approach ACK
  23. 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.

  24. 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.
  25. 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
  26. 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:

    0JSONRPCException: 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.

  27. achow101 force-pushed on Jul 14, 2023
  28. 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).

  29. test: Add helper functions for checking node versions 5d8469362a
  30. achow101 force-pushed on Jul 14, 2023
  31. DrahtBot added the label CI failed on Jul 14, 2023
  32. achow101 force-pushed on Jul 14, 2023
  33. DrahtBot removed the label CI failed on Jul 15, 2023
  34. 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
  35. 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:

    0            # Copy wallets to previous version
1            for wallet in os.listdir(node_master_wallets_dir):
2                source = node_master_wallets_dir / wallet
3                if self.major_version_equals(node, 16):
4                    # 0.16 node expect the wallet to be in the wallet dir but as a plain file rather than in directories
5                    source / "wallet.dat"
6                shutil.copytree(source, node.wallets_path / wallet)
    achow101 commented at 8:42 pm on August 23, 2023:
    Done
  36. 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
  37. 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).

  38. achow101 force-pushed on Aug 23, 2023
  39. 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
  40. test: Remove w1_v18 from wallet backwards compatibility 
    This wallet is no longer used in the test
    53f35d02cb
  41. 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
  42. test: add logging 0.17 incompatibilities in wallet back compat f41215c3f0
  43. test: Add 0.21 tr() incompatibility test f158573be1
  44. test: Run downgrade test on descriptor wallets 6d4699028b
  45. test: Run upgrade test on all nodes 538939ec39
  46. test: Add 25.0 to wallet backwards compatibiilty test bbf43c63b9
  47. 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
  48. achow101 force-pushed on Aug 23, 2023
  49. furszy commented at 10:55 pm on August 23, 2023: member
    ACK afd9a67
  50. DrahtBot requested review from Sjors on Aug 23, 2023
  51. achow101 requested review from S3RK on Oct 4, 2023
  52. Sjors commented at 7:46 am on October 5, 2023: member
    re-ACK afd9a673c458e97305da49a70a1ddbf60e651876
  53. DrahtBot removed review request from Sjors on Oct 5, 2023
  54. maflcko added this to the milestone 26.0 on Oct 5, 2023
  55. fanquake merged this on Oct 5, 2023
  56. fanquake closed this on Oct 5, 2023
  57. bitcoin locked this on Oct 4, 2024


achow101 DrahtBot maflcko Sjors ismaelsadeeq S3RK furszy


S3RK

Labels
Tests

Milestone
26.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me