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

[achow101]

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 wallets created on master get corrupted when processed with v25 #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]

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.

[Sjors]

Thanks for improving these tests. Added to review list.

[ismaelsadeeq]

Concept ACK

[ismaelsadeeq]

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

[ismaelsadeeq]

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]

I don't think the specific keys are relevant.

[ismaelsadeeq]

Why are we no longer using the wallet?

[achow101]

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.

[S3RK]

Approach ACK

[achow101]

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.

[Sjors]

0ffa0f1 (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 (e408968).

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.

[Sjors]

6a5d245: 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]

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

[achow101]

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).

[Sjors]

ACK 91f47e6

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

[furszy]

ACK afd9a67

[Sjors]

re-ACK afd9a67