test: Remove fragile and ancient release 0.17 wallet test

maflcko commented at 10:53 am on April 3, 2025: member

The test checks that the 0.17 wallet rejects wallet files created in “the future”.

This is nice, and good to know. However,

The 0.17 release is ancient and should be unused outside of tests, especially to load future wallets.
The test intermittently fails, due to ancient RPC server bugs, that were fixed in the meantime. [1]
Albeit they are not identical, the 0.18 release is still checked in this test, so any theoretical bug that would be caught by 0.17 is hopefully still caught by 0.18 as well.

So fix all issues by removing the test case.

[1] For example from https://api.cirrus-ci.com/v1/task/6161588714995712/logs/ci.log:

 0190/321 - [1mwallet_backwards_compatibility.py --descriptors[0m failed, Duration: 23 s
 1[17:21:40.700] 
 2[17:21:40.700] [1mstdout:
 3[17:21:40.700] [0m2025-04-02T21:21:16.575000Z TestFramework (INFO): PRNG seed is: 5772716217847090743
 4[17:21:40.700] 2025-04-02T21:21:16.580000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20250402_210134/wallet_backwards_compatibility_134
 5[17:21:40.700] 2025-04-02T21:21:26.378000Z TestFramework (INFO): Test wallet backwards compatibility...
 6[17:21:40.700] 2025-04-02T21:21:33.191000Z TestFramework (INFO): Testing 0.19 addmultisigaddress case (#18075)
 7[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): Test that a wallet made on master can be opened on:
 8[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): - 250000
 9[17:21:40.700] 2025-04-02T21:21:34.055000Z TestFramework (INFO): - 240001
10[17:21:40.700] 2025-04-02T21:21:34.435000Z TestFramework (INFO): - 230000
11[17:21:40.700] 2025-04-02T21:21:34.858000Z TestFramework (INFO): - 220000
12[17:21:40.700] 2025-04-02T21:21:35.614000Z TestFramework (INFO): - 210000
13[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): Test descriptor wallet incompatibility on:
14[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): - 200100
15[17:21:40.700] 2025-04-02T21:21:35.878000Z TestFramework (INFO): - 190100
16[17:21:40.700] 2025-04-02T21:21:36.021000Z TestFramework (INFO): - 180100
17[17:21:40.700] 2025-04-02T21:21:36.319000Z TestFramework (INFO): Test descriptor wallet incompatibility with 0.17
18[17:21:40.700] 2025-04-02T21:21:37.328000Z TestFramework (INFO): Test that 0.21 cannot open wallet containing tr() descriptors
19[17:21:40.700] 2025-04-02T21:21:37.356000Z TestFramework (INFO): Test that a wallet can upgrade to and downgrade from master, from:
20[17:21:40.700] 2025-04-02T21:21:37.361000Z TestFramework (INFO): - 250000
21[17:21:40.700] 2025-04-02T21:21:37.665000Z TestFramework (INFO): - 240001
22[17:21:40.700] 2025-04-02T21:21:37.970000Z TestFramework (INFO): - 230000
23[17:21:40.700] 2025-04-02T21:21:38.439000Z TestFramework (INFO): - 220000
24[17:21:40.700] 2025-04-02T21:21:38.793000Z TestFramework (INFO): - 210000
25[17:21:40.700] 2025-04-02T21:21:39.470000Z TestFramework (INFO): Stopping nodes
26[17:21:40.700] 
27[17:21:40.700] 
28[17:21:40.700] [1mstderr:
29[17:21:40.700] [0mTraceback (most recent call last):
30[17:21:40.700]   File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_backwards_compatibility.py", line 389, in <module>
31[17:21:40.700]     BackwardsCompatibilityTest(__file__).main()
32[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 206, in main
33[17:21:40.700]     exit_code = self.shutdown()
34[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 379, in shutdown
35[17:21:40.700]     self.stop_nodes()
36[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 643, in stop_nodes
37[17:21:40.700]     node.stop_node(wait=wait, wait_until_stopped=False)
38[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_node.py", line 397, in stop_node
39[17:21:40.700]     self.stop()
40[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
41[17:21:40.700]     return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
42[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/authproxy.py", line 132, in __call__
43[17:21:40.700]     response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
44[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/authproxy.py", line 106, in _request
45[17:21:40.700]     return self._get_response()
46[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/authproxy.py", line 169, in _get_response
47[17:21:40.700]     http_response = self.__conn.getresponse()
48[17:21:40.700]   File "/usr/lib/python3.10/http/client.py", line 1375, in getresponse
49[17:21:40.700]     response.begin()
50[17:21:40.700]   File "/usr/lib/python3.10/http/client.py", line 318, in begin
51[17:21:40.700]     version, status, reason = self._read_status()
52[17:21:40.700]   File "/usr/lib/python3.10/http/client.py", line 287, in _read_status
53[17:21:40.700]     raise RemoteDisconnected("Remote end closed connection without"
54[17:21:40.700] http.client.RemoteDisconnected: Remote end closed connection without response
55[17:21:40.700] [node 10] Cleaning up leftover process
56[17:21:40.700] [node 9] Cleaning up leftover process
57[17:21:40.700] [node 8] Cleaning up leftover process
58[17:21:40.700] [node 7] Cleaning up leftover process
59[17:21:40.700] [node 6] Cleaning up leftover process
60[17:21:40.700] [node 5] Cleaning up leftover process
61[17:21:40.700] [node 4] Cleaning up leftover process
62[17:21:40.700] [node 3] Cleaning up leftover process
63[17:21:40.700] [node 2] Cleaning up leftover process
64[17:21:40.700] [node 1] Cleaning up leftover process
65[17:21:40.700] [node 0] Cleaning up leftover process

DrahtBot commented at 10:53 am on April 3, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32214.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	janb84, pablomartin4btc, laanwj, BrandonOdiwuor
Stale ACK	musaHaruna

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

DrahtBot added the label Tests on Apr 3, 2025

janb84 commented at 11:24 am on April 3, 2025: contributor

Are the lines 346-356 then still needed ? since we are only testing only from 18+

0            if self.major_version_less_than(node, 17):
1                # loadwallet is only available in 0.17+
2                shutil.copyfile(
3                    down_backup_path,
4                    node.wallets_path / down_wallet_name
5                )
6                self.start_node(node.index, extra_args=[f"-wallet={down_wallet_name}"])
7                wallet_res = node.get_wallet_rpc(down_wallet_name)
8                info = wallet_res.validateaddress(address)
9                assert_equal(info, addr_info)

maflcko force-pushed on Apr 3, 2025

DrahtBot added the label CI failed on Apr 3, 2025

maflcko commented at 12:04 pm on April 3, 2025: member

Good catch. I went through the test and removed all dead code now.

janb84 commented at 12:16 pm on April 3, 2025: contributor

ACK fa77238

Spring cleanup, removal of node V17 tests (V17 is EOL since 2020-08-01).

code reviewed ✅
compiled ✅
run tests (functional / unit) ✅

in test/functional/wallet_backwards_compatibility.py:261 in fa77238230 outdated

261-                # assert_raises_rpc_error(-4, "Wallet loading failed.", node_v17.loadwallet, 'w3')
262-                if self.major_version_less_than(node, 18):
263-                    continue
264                 self.log.info(f"- {node.version}")
265                 # Descriptor wallets appear to be corrupted wallets to old software
266                 assert self.major_version_at_least(node, 18) and self.major_version_less_than(node, 21)

pablomartin4btc commented at 12:30 pm on April 3, 2025:

since we removed v0.17…

0                assert self.major_version_less_than(node, 21)

pablomartin4btc commented at 12:30 pm on April 3, 2025: member

cr ACK fa772382301ed32d57b7af817678b5cc1022f17f

test: Remove fragile and ancient release 0.17 wallet test fac978fb21

maflcko force-pushed on Apr 3, 2025

janb84 commented at 12:37 pm on April 3, 2025: contributor

Re ACK fac978f

Changes sinds last ack:

removed obsolete test to check if node >=18

DrahtBot requested review from pablomartin4btc on Apr 3, 2025

pablomartin4btc commented at 12:57 pm on April 3, 2025: member

re ACK fac978fb213fbd6668194d8b9810ddd10f8a7323

laanwj added the label Wallet on Apr 3, 2025

DrahtBot removed the label CI failed on Apr 3, 2025

laanwj commented at 2:19 pm on April 3, 2025: member

Code review ACK fac978fb213fbd6668194d8b9810ddd10f8a7323

Because i wondered i also checked that this test (all the way back to 0.17) still exists after #31250, it does.

BrandonOdiwuor commented at 1:42 pm on April 4, 2025: contributor

Code Review ACK fac978fb213fbd6668194d8b9810ddd10f8a7323

musaHaruna commented at 2:07 pm on April 7, 2025: none

ACK fa77238

Code reviewed, ran functional test (passed)

DrahtBot requested review from musaHaruna on Apr 7, 2025

ryanofsky assigned ryanofsky on Apr 8, 2025

ryanofsky merged this on Apr 8, 2025

ryanofsky closed this on Apr 8, 2025

maflcko deleted the branch on Apr 9, 2025

test: Remove fragile and ancient release 0.17 wallet test #32214

Code Coverage & Benchmarks

Reviews