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

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:

190/321 - [1mwallet_backwards_compatibility.py --descriptors[0m failed, Duration: 23 s
[17:21:40.700] 
[17:21:40.700] [1mstdout:
[17:21:40.700] [0m2025-04-02T21:21:16.575000Z TestFramework (INFO): PRNG seed is: 5772716217847090743
[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
[17:21:40.700] 2025-04-02T21:21:26.378000Z TestFramework (INFO): Test wallet backwards compatibility...
[17:21:40.700] 2025-04-02T21:21:33.191000Z TestFramework (INFO): Testing 0.19 addmultisigaddress case (#18075)
[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): Test that a wallet made on master can be opened on:
[17:21:40.700] 2025-04-02T21:21:33.637000Z TestFramework (INFO): - 250000
[17:21:40.700] 2025-04-02T21:21:34.055000Z TestFramework (INFO): - 240001
[17:21:40.700] 2025-04-02T21:21:34.435000Z TestFramework (INFO): - 230000
[17:21:40.700] 2025-04-02T21:21:34.858000Z TestFramework (INFO): - 220000
[17:21:40.700] 2025-04-02T21:21:35.614000Z TestFramework (INFO): - 210000
[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): Test descriptor wallet incompatibility on:
[17:21:40.700] 2025-04-02T21:21:35.707000Z TestFramework (INFO): - 200100
[17:21:40.700] 2025-04-02T21:21:35.878000Z TestFramework (INFO): - 190100
[17:21:40.700] 2025-04-02T21:21:36.021000Z TestFramework (INFO): - 180100
[17:21:40.700] 2025-04-02T21:21:36.319000Z TestFramework (INFO): Test descriptor wallet incompatibility with 0.17
[17:21:40.700] 2025-04-02T21:21:37.328000Z TestFramework (INFO): Test that 0.21 cannot open wallet containing tr() descriptors
[17:21:40.700] 2025-04-02T21:21:37.356000Z TestFramework (INFO): Test that a wallet can upgrade to and downgrade from master, from:
[17:21:40.700] 2025-04-02T21:21:37.361000Z TestFramework (INFO): - 250000
[17:21:40.700] 2025-04-02T21:21:37.665000Z TestFramework (INFO): - 240001
[17:21:40.700] 2025-04-02T21:21:37.970000Z TestFramework (INFO): - 230000
[17:21:40.700] 2025-04-02T21:21:38.439000Z TestFramework (INFO): - 220000
[17:21:40.700] 2025-04-02T21:21:38.793000Z TestFramework (INFO): - 210000
[17:21:40.700] 2025-04-02T21:21:39.470000Z TestFramework (INFO): Stopping nodes
[17:21:40.700] 
[17:21:40.700] 
[17:21:40.700] [1mstderr:
[17:21:40.700] [0mTraceback (most recent call last):
[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>
[17:21:40.700]     BackwardsCompatibilityTest(__file__).main()
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 206, in main
[17:21:40.700]     exit_code = self.shutdown()
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 379, in shutdown
[17:21:40.700]     self.stop_nodes()
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_framework.py", line 643, in stop_nodes
[17:21:40.700]     node.stop_node(wait=wait, wait_until_stopped=False)
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/test_node.py", line 397, in stop_node
[17:21:40.700]     self.stop()
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/coverage.py", line 50, in __call__
[17:21:40.700]     return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/authproxy.py", line 132, in __call__
[17:21:40.700]     response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/authproxy.py", line 106, in _request
[17:21:40.700]     return self._get_response()
[17:21:40.700]   File "/ci_container_base/test/functional/test_framework/authproxy.py", line 169, in _get_response
[17:21:40.700]     http_response = self.__conn.getresponse()
[17:21:40.700]   File "/usr/lib/python3.10/http/client.py", line 1375, in getresponse
[17:21:40.700]     response.begin()
[17:21:40.700]   File "/usr/lib/python3.10/http/client.py", line 318, in begin
[17:21:40.700]     version, status, reason = self._read_status()
[17:21:40.700]   File "/usr/lib/python3.10/http/client.py", line 287, in _read_status
[17:21:40.700]     raise RemoteDisconnected("Remote end closed connection without"
[17:21:40.700] http.client.RemoteDisconnected: Remote end closed connection without response
[17:21:40.700] [node 10] Cleaning up leftover process
[17:21:40.700] [node 9] Cleaning up leftover process
[17:21:40.700] [node 8] Cleaning up leftover process
[17:21:40.700] [node 7] Cleaning up leftover process
[17:21:40.700] [node 6] Cleaning up leftover process
[17:21:40.700] [node 5] Cleaning up leftover process
[17:21:40.700] [node 4] Cleaning up leftover process
[17:21:40.700] [node 3] Cleaning up leftover process
[17:21:40.700] [node 2] Cleaning up leftover process
[17:21:40.700] [node 1] Cleaning up leftover process
[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+

            if self.major_version_less_than(node, 17):
                # loadwallet is only available in 0.17+
                shutil.copyfile(
                    down_backup_path,
                    node.wallets_path / down_wallet_name
                )
                self.start_node(node.index, extra_args=[f"-wallet={down_wallet_name}"])
                wallet_res = node.get_wallet_rpc(down_wallet_name)
                info = wallet_res.validateaddress(address)
                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...

                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