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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32350.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
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)
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.
assert self.major_version_less_than(node, 21)
thx, may do if I have to re-touch.
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"},
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.
Correct. Thanks for adding the reference.
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.
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.
ACK fa58f40b898ba6c57655bf38a241fb10107d4a3a