test: Slim down previous releases bdb check

maflcko commented at 2:13 pm on April 25, 2025: member

The check iterates over several previous BDB-only releases to check that descriptor wallets are considered “corrupt” when loading. It is unclear why this needs to be done for more than one release.

Avoid the confusion by removing the unused releases from the test and from the download script.

test: Slim down previous releases bdb check fa58f40b89

DrahtBot commented at 2:13 pm on April 25, 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/32350.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, achow101

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

DrahtBot renamed this:
~~test: Slim down previous releases bdb check~~
test: Slim down previous releases bdb check
on Apr 25, 2025

DrahtBot added the label Tests on Apr 25, 2025

in test/functional/wallet_backwards_compatibility.py:205 in fa58f40b89

207-            assert self.major_version_less_than(node, 21)
208-            for wallet_name in ["w1", "w2", "w3"]:
209-                assert_raises_rpc_error(-4, "Wallet file verification failed: wallet.dat corrupt, salvage failed", node.loadwallet, wallet_name)
210+        self.log.info("Test descriptor wallet incompatibility on v0.20")
211+        # Descriptor wallets appear to be corrupted wallets to old software
212+        assert self.major_version_equals(node_v20, 20)

rkrux commented at 10:48 am on April 28, 2025:

A small point: I don’t find this assertion helpful and makes me wonder if it’s asserting the obvious. The earlier assertion was still helpful because it asserted that the major version is less than 21 in which descriptor wallets were introduced.

I feel the earlier assertion should be preserved here.

0assert self.major_version_less_than(node, 21)

maflcko commented at 11:45 am on April 28, 2025:

thx, may do if I have to re-touch.

in test/get_previous_releases.py:41 in fa58f40b89

36-    "5d422a9d544742bc0df12427383f9c2517433ce7b58cf672b9a9b17c2ef51e4f": {"tag": "v0.16.3", "tarball": "bitcoin-0.16.3-x86_64-linux-gnu.tar.gz"},
37-    #
38-    "5a6b35d1a348a402f2d2d6ab5aed653a1a1f13bc63aaaf51605e3501b0733b7a": {"tag": "v0.17.2", "tarball": "bitcoin-0.17.2-aarch64-linux-gnu.tar.gz"},
39-    "d1913a5d19c8e8da4a67d1bd5205d03c8614dfd2e02bba2fe3087476643a729e": {"tag": "v0.17.2", "tarball": "bitcoin-0.17.2-arm-linux-gnueabihf.tar.gz"},
40-    "a783ba20706dbfd5b47fbedf42165fce70fbbc7d78003305d964f6b3da14887f": {"tag": "v0.17.2", "tarball": "bitcoin-0.17.2-osx64.tar.gz"},
41-    "943f9362b9f11130177839116f48f809d83478b4c28591d486ee9a7e35179da6": {"tag": "v0.17.2", "tarball": "bitcoin-0.17.2-x86_64-linux-gnu.tar.gz"},

rkrux commented at 10:50 am on April 28, 2025:

Nit if retouched (feel free to ignore): These 3 versions can be removed in a separate commit as they were unused before this pull as well; it would be helpful for future reference.

maflcko commented at 11:45 am on April 28, 2025:

Correct. Thanks for adding the reference.

rkrux approved

rkrux commented at 11:04 am on April 28, 2025: contributor

ACK fa58f40b898ba6c57655bf38a241fb10107d4a3a

It appears that the 18, 19 releases are not used elsewhere in the test. It makes sense to me to test the “descriptor-wallets-file-corrupted” error only for the last release that doesn’t support descriptor wallets, which is 20. Also, helps in reducing the number of nodes needed in this test.

I have noticed that the 17 release was removed from this test recently in some PR and now 18, 19 are removed. I’d assume that the 20 release is kept in this test for a considerable time as it marks the last release that didn’t contain descriptors (& SQLite) wallets & had only legacy (& BDB) ones.

maflcko commented at 11:44 am on April 28, 2025: member

I have noticed that the 17 release was removed from this test recently in some PR and now 18, 19 are removed.

Correct. For reference, the reasons for removal were different. This removal in this pull request only became possible after the bdb removal removing the test for the v0.19 18075 behavior (https://github.com/bitcoin/bitcoin/issues/18075)

I’d assume that the 20 release is kept in this test for a considerable time as it marks the last release that didn’t contain descriptors (& SQLite) wallets & had only legacy (& BDB) ones.

Yeah, it should be fine to keep for a while. I don’t anticipate any issues popping up.

achow101 commented at 7:30 pm on April 28, 2025: member

ACK fa58f40b898ba6c57655bf38a241fb10107d4a3a

achow101 merged this on Apr 28, 2025

achow101 closed this on Apr 28, 2025

maflcko deleted the branch on Apr 29, 2025

test: Slim down previous releases bdb check #32350

Code Coverage & Benchmarks

Reviews