test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #19013

instead of updating this in the ci config, as well as all the backward compat tests, why not simply make it the default value in the script?

this goes away with #19294

rebased

not sure if we want to add dead and untested code the the test framework. This can be added when it is actually needed.

I need it every time there's a release candidate, but then it never it gets merged...

Until we will be testing backwards compatibility for rcs, you can keep it as a commit in your local git and then cherry-pick as needed for your own needs.

same

  DOCKER_EXEC gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys $MAINTAINER_KEY

Would be surprising if a vanilla docker image had the key already

Maybe we want to cache it though if there's a DDOS on PGP key servers again.

I think this can be moved to test/README.md, which explains how to run the functional tests

Done, and rebased

see test/README.md.

The previous release v0.15.2 is required by this test, see test/README.md.

Requires previous releases binaries, see test/README.md.

nit: Add trailing comma to avoid having to touch this line again for the next release.

"376194f06596ecfa40331167c39bc70c355f960280bd2a645fdbf18f66527397":  "bitcoin-0.20.1-x86_64-linux-gnu.tar.gz",

is the source code tar.gz needed for anything? None of the other versions include it.

I'll drop it for consistency.

It is not possible running the compat tests on powerpc64, because the other versions are missing on that arch, but I guess it is fine adding them here nonetheless.

Maybe add a commit to remove i686 for that reason?

commit 6fd3fd1ba76831d83a43e9322e78afca70aabbeb (HEAD)
Author: MarcoFalke <falke.marco@gmail.com>
Date:   Wed Sep 22 13:35:01 2021 +0200

    test: Remove i686 from test/get_previous_releases.py
    
    It is not possible to run the compatibility tests on i686 because the
    releases v20+ are missing for that arch. It would be possible to
    self-compile those releases, but then one can also self-compile
    0.15-0.19.

diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py
index 62fcad04b3..27f7044094 100755
--- a/test/get_previous_releases.py
+++ b/test/get_previous_releases.py
@@ -23,32 +23,27 @@ import hashlib
 SHA256_SUMS = {
     "d40f18b4e43c6e6370ef7db9131f584fbb137276ec2e3dba67a4b267f81cb644": "bitcoin-0.15.2-aarch64-linux-gnu.tar.gz",
     "54fb877a148a6ad189a1e1ab1ff8b11181e58ff2aaf430da55b3fd46ae549a6b": "bitcoin-0.15.2-arm-linux-gnueabihf.tar.gz",
-    "2b843506c3f1af0eeca5854a920264f9a829f02d0d50328005950ddcbe88874d": "bitcoin-0.15.2-i686-pc-linux-gnu.tar.gz",
     "87e9340ff3d382d543b2b69112376077f0c8b4f7450d372e83b68f5a1e22b2df": "bitcoin-0.15.2-osx64.tar.gz",
     "566be44190fd76daa01f13d428939dadfb8e3daacefc8fa17f433cad28f73bd5": "bitcoin-0.15.2-x86_64-linux-gnu.tar.gz",
     #
     "0768c6c15caffbaca6524824c9563b42c24f70633c681c2744649158aa3fd484": "bitcoin-0.16.3-aarch64-linux-gnu.tar.gz",
     "fb2818069854a6ad20ea03b28b55dbd35d8b1f7d453e90b83eace5d0098a2a87": "bitcoin-0.16.3-arm-linux-gnueabihf.tar.gz",
-    "75a537844313b0a84bdb61ffcdc5c4ce19a738f7ddf71007cd2edf664efd7c37": "bitcoin-0.16.3-i686-pc-linux-gnu.tar.gz",
     "78c3bff3b619a19aed575961ea43cc9e142959218835cf51aede7f0b764fc25d": "bitcoin-0.16.3-osx64.tar.gz",
     "5d422a9d544742bc0df12427383f9c2517433ce7b58cf672b9a9b17c2ef51e4f": "bitcoin-0.16.3-x86_64-linux-gnu.tar.gz",
     #
     "5a6b35d1a348a402f2d2d6ab5aed653a1a1f13bc63aaaf51605e3501b0733b7a": "bitcoin-0.17.2-aarch64-linux-gnu.tar.gz",
     "d1913a5d19c8e8da4a67d1bd5205d03c8614dfd2e02bba2fe3087476643a729e": "bitcoin-0.17.2-arm-linux-gnueabihf.tar.gz",
-    "d295fc93f39bbf0fd937b730a93184899a2eb6c3a6d53f3d857cbe77ef89b98c": "bitcoin-0.17.2-i686-pc-linux-gnu.tar.gz",
     "a783ba20706dbfd5b47fbedf42165fce70fbbc7d78003305d964f6b3da14887f": "bitcoin-0.17.2-osx64.tar.gz",
     "943f9362b9f11130177839116f48f809d83478b4c28591d486ee9a7e35179da6": "bitcoin-0.17.2-x86_64-linux-gnu.tar.gz",
     #
     "88f343af72803b851c7da13874cc5525026b0b55e63e1b5e1298390c4688adc6": "bitcoin-0.18.1-aarch64-linux-gnu.tar.gz",
     "cc7d483e4b20c5dabd4dcaf304965214cf4934bcc029ca99cbc9af00d3771a1f": "bitcoin-0.18.1-arm-linux-gnueabihf.tar.gz",
-    "989e847b3e95fc9fedc0b109cae1b4fa43348f2f712e187a118461876af9bd16": "bitcoin-0.18.1-i686-pc-linux-gnu.tar.gz",
     "b7bbcee7a7540f711b171d6981f939ca8482005fde22689bc016596d80548bb1": "bitcoin-0.18.1-osx64.tar.gz",
     "425ee5ec631ae8da71ebc1c3f5c0269c627cf459379b9b030f047107a28e3ef8": "bitcoin-0.18.1-riscv64-linux-gnu.tar.gz",
     "600d1db5e751fa85903e935a01a74f5cc57e1e7473c15fd3e17ed21e202cfe5a": "bitcoin-0.18.1-x86_64-linux-gnu.tar.gz",
     #
     "3a80431717842672df682bdb619e66523b59541483297772a7969413be3502ff": "bitcoin-0.19.1-aarch64-linux-gnu.tar.gz",
     "657f28213823d240dd3324d14829702f9ad6f0710f8bdd1c379cb3c447197f48": "bitcoin-0.19.1-arm-linux-gnueabihf.tar.gz",
-    "10d1e53208aa7603022f4acc084a046299ab4ccf25fe01e81b3fb6f856772589": "bitcoin-0.19.1-i686-pc-linux-gnu.tar.gz",
     "1ae1b87de26487075cd2fd22e0d4ead87d969bd55c44f2f1d873ecdc6147ebb3": "bitcoin-0.19.1-osx64.tar.gz",
     "aa7a9563b48aa79252c8e7b6a41c07a5441bd9f14c5e4562cc72720ea6cb0ee5": "bitcoin-0.19.1-riscv64-linux-gnu.tar.gz",
     "5fcac9416e486d4960e1a946145566350ca670f9aaba99de6542080851122e4c": "bitcoin-0.19.1-x86_64-linux-gnu.tar.gz",

When I try to git am this patch file it complains "Patch format detection failed.", odd.

Ah, wrong format. See 0001-test-Remove-i686-from-test-get_previous_releases.py.patch.txt

Got it. Pushed.

In a617b9d3b7ffdd7f36e8574602b182edf9d70100 "test: previous releases: add v22.0"

Previously these specifiers matched the client version number, but this does not. 22.0's client version is 220000, but this causes the test framework to search for a directory named 0.22.0 rather than 22.0.

However the client version is used in a few places in the test framework to enable features, so it would be nice if this specifier would match the actual client version. Perhaps this could use a string or tuple that represents the dotted version with a conversion within the test framework to the client version?

I think I discovered half-way this commit that 22.0 is not expressed as 22000000, but as 220000. I'll look into that.

In commit "test: backwards compatibility: misc fixes" (f35faefac053ff2eb1aa6c99c24e99796a7b53d9)

All of the changes in this commit seem innocuous, but also arbitrary, and I don't know what they are doing. It would be great if the commit description had some summary to help reviewers like me, or to help debug in case any of these changes cause problems in the future.

What I [was trying to tell][0], clearly expressed.

[0]: #19013 (comment)

I added a bit more clarification. In particular I point out that coverage may have changed, which I think is worth it for the increased readability.

In commit "test: v0.20.1 backwards compatibility" (94d3989352aba63c86c901269438bfd566dd8632)

Does this change belong in this commit? Could it be split off or have a rationale mentioned in a commit description?

Yes, the DMG was added in an earlier PR, but shouldn't have. I'll add a note.

In commit "test: v0.20.1 backwards compatibility" (94d3989352aba63c86c901269438bfd566dd8632)

The deduplication here is great but the nodes[2:] references are more obscure and maybe fragile. Could this use for node in legacy_nodes? And

legacy_nodes = [node_v19, node_v18, node_v17] or legacy_nodes = nodes[2:] or legacy_nodes = [node for node in nodes if node.version <= 190000]

?

I've now restricted self.nodes[...] to the top of the test, using more human readable names in the rest of the test.

In commit "test: v0.20.1 backwards compatibility" (94d3989352aba63c86c901269438bfd566dd8632)

Looking at all the test changes here it seems like no test coverage is lost, but it would be nice if commit message could say whether this is the case.

IMO, it would also be nicer if this commit were split into a refactoring commit that did not change coverage and just deduped code, and a smaller followup commit adding the new v0.20 version. But just mentioning however this commit affects coverage would be good if you don't want to change the commit contents.

I moved the refactoring stuff into the misc changes commit. Adding a note that perhaps coverage changed in the process of this cleanup.

In commit "test: previous releases: add v22.0" (62b22de69dabfb86eaabb6a685e3989ce525d0e3)

This seems a little strange. Could you add a code comment here explaining this? It seems like this code could not have a special 219999 case and just multiply by 100 for all versions and strip .0.0 for all versions.

I'll add a note. When we changed the numbering in version v22.0, the client version wasn't bumped to 22000000, but to 220000. Without this change it would expect the binary to live in v0.22.0. Conversely if we multiplied all versions by 100 then it would look for v21.0 for the 0.21.0 release.

In commit "test: backwards compatibility: misc fixes" (408e9b48765dee164578c97b6006c525c70a14f4)

Minor: indentation is inconsistent here (2 spaces instead of usual 4)

In commit "test: backwards compatibility: misc fixes" (408e9b48765dee164578c97b6006c525c70a14f4)

Note: Dropping these node_v18.rpc.createwallet and node_v19.rpc.createwallet calls seems to lose test coverage creating these wallets. But the wallets are never used later, and this would only be testing create code in frozen releases, so losing these is probably good.

In commit "test: previous releases: add v22.0" (7124899057a351a3491b288d96426f91ef1fd040)

It would be helpful to say why this condition is needed in code comment like # The -sandbox argument is not present in the v22.0 release. (from the commit description)