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

Sjors commented at 10:08 am on May 19, 2020: member

This also simplifies the tests a bit.

fanquake added the label Tests on May 19, 2020

DrahtBot commented at 8:20 pm on May 19, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24236 (Remove utxo db upgrade code by MarcoFalke)
#23127 (tests: Use test framework utils where possible by vincenzopalazzo)
#19332 (test: Fix intermittent test failure in feature_backwards_compatibility by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Sjors force-pushed on May 22, 2020

Sjors force-pushed on Jun 3, 2020

Sjors marked this as ready for review on Jun 3, 2020

Sjors force-pushed on Jun 16, 2020

in test/functional/feature_backwards_compatibility.py:9 in 5a9152fdcc outdated

 5@@ -6,7 +6,7 @@
 6 
 7 Test various backwards compatibility scenarios. Download the previous node binaries:
 8 
 9-contrib/devtools/previous_release.sh -b v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2
10+contrib/devtools/previous_release.sh -b v0.20.0 v0.19.1 v0.18.1 v0.17.1 v0.16.3 v0.15.2

MarcoFalke commented at 2:38 pm on June 16, 2020:

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?

in test/functional/feature_backwards_compatibility.py:66 in 5a9152fdcc outdated

62             170100,
63             160300,
64         ])
65         # adapt bitcoin.conf, because older bitcoind's don't recognize config sections
66-        adjust_bitcoin_conf_for_pre_17(self.nodes[5].bitcoinconf)
67+        adjust_bitcoin_conf_for_pre_17(self.nodes[-1].bitcoinconf)

MarcoFalke commented at 2:39 pm on June 16, 2020:

this goes away with #19294

Sjors commented at 3:42 pm on June 17, 2020:

rebased

in test/functional/test_framework/test_framework.py:434 in 5a9152fdcc outdated

432@@ -429,7 +433,7 @@ def get_bin_from_version(version, bin_name, bin_default):
433                         (version % 10000) // 100,
434                         (version % 100) // 1,
435                     ),
436-                ),
437+                ) + suffix,

MarcoFalke commented at 2:41 pm on June 16, 2020:

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

Sjors commented at 4:05 pm on June 16, 2020:

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

MarcoFalke commented at 4:10 pm on June 16, 2020:

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.

in test/functional/test_framework/test_node.py:312 in 5a9152fdcc outdated

304@@ -305,7 +305,11 @@ def get_wallet_rpc(self, wallet_name):
305             return RPCOverloadWrapper(self.rpc / wallet_path, descriptors=self.descriptors)
306 
307     def version_is_at_least(self, ver):
308-        return self.version is None or self.version >= ver
309+        if self.version is None:
310+            return True
311+        if type(self.version) == tuple:
312+            return self.version[0] >= ver

MarcoFalke commented at 2:41 pm on June 16, 2020:

same

MarcoFalke commented at 2:41 pm on June 16, 2020: member

Concept ACK

DrahtBot added the label Needs rebase on Jun 16, 2020

Sjors force-pushed on Jun 17, 2020

DrahtBot removed the label Needs rebase on Jun 17, 2020

DrahtBot added the label Needs rebase on Jun 22, 2020

Sjors force-pushed on Jun 29, 2020

DrahtBot removed the label Needs rebase on Jun 29, 2020

DrahtBot added the label Needs rebase on Jul 21, 2020

Sjors commented at 12:41 pm on July 21, 2020: member

Rebased and dropped the release candidate commit.

Sjors force-pushed on Jul 21, 2020

DrahtBot removed the label Needs rebase on Jul 21, 2020

MarcoFalke commented at 1:51 pm on July 21, 2020: member

This fails on travis. Presumably because gpg is missing, etc

fjahr commented at 10:01 am on July 22, 2020: member

Concept ACK, code looks good, just need to fix Travis.

Also ACK on DRYing up the compatibility test.

Sjors commented at 2:00 pm on July 23, 2020: member

This fails on travis. Presumably because gpg is missing, etc

Oh, I guess we should have cleared Travis cache before merging #19205. I added a commit to fetch the release PGP key (can be cherry-picked into its own PR if we need it urgently).

Without shell=True I get:

0File "/usr/lib/python3.6/subprocess.py", line 1364, in _execute_child
11968    raise child_exception_type(errno_num, err_msg, err_filename)
21969FileNotFoundError: [Errno 2] No such file or directory: 'gpg': 'gpg'

Sjors force-pushed on Jul 23, 2020

in ci/test/05_before_script.sh:51 in b3b9b8aeda outdated

47@@ -48,6 +48,7 @@ if [ -z "$NO_DEPENDS" ]; then
48 fi
49 if [ -n "$PREVIOUS_RELEASES_TO_DOWNLOAD" ]; then
50   BEGIN_FOLD previous-versions
51+  DOCKER_EXEC gpg --list-keys $MAINTAINER_KEY >/dev/null 2>&1 || gpg --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys $MAINTAINER_KEY

MarcoFalke commented at 4:52 pm on July 23, 2020:

0  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

Sjors commented at 5:43 pm on July 23, 2020:

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

Sjors force-pushed on Jul 23, 2020

Sjors force-pushed on Jul 31, 2020

Sjors commented at 12:07 pm on July 31, 2020: member

Travis keeps failing the GPG check with a useful log entry, so instead I just added a flag to skip this check.

DrahtBot added the label Needs rebase on Aug 5, 2020

Sjors commented at 11:32 am on August 31, 2020: member

I’ll rebase after #19813

MarcoFalke referenced this in commit c1e0c2ad3b on Aug 31, 2020

sidhujag referenced this in commit dd6081216a on Aug 31, 2020

Sjors renamed this:
~~test: add v0.20.0 to backwards compatibility test~~
test: add v0.20.1 to backwards compatibility test
on Oct 8, 2020

Sjors force-pushed on Oct 8, 2020

Sjors commented at 10:32 am on October 8, 2020: member

Rebased and switched to v0.20.1

Sjors force-pushed on Oct 8, 2020

DrahtBot removed the label Needs rebase on Oct 8, 2020

in test/functional/wallet_upgradewallet.py:9 in 8049c16d1f outdated

 5@@ -6,7 +6,7 @@
 6 
 7 Test upgradewallet RPC. Download node binaries:
 8 
 9-test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
10+test/get_previous_releases.py -b v0.20.1 v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2

MarcoFalke commented at 4:29 pm on October 26, 2020:

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

Sjors commented at 8:30 pm on November 3, 2020:

Done, and rebased

MarcoFalke commented at 4:30 pm on October 26, 2020: member

ACK

DrahtBot added the label Needs rebase on Nov 2, 2020

Sjors force-pushed on Nov 3, 2020

DrahtBot removed the label Needs rebase on Nov 3, 2020

in test/functional/feature_backwards_compatibility.py:8 in 256a76b48d outdated

 3@@ -4,9 +4,8 @@
 4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 5 """Backwards compatibility functional test
 6 
 7-Test various backwards compatibility scenarios. Download the previous node binaries:
 8-
 9-test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
10+Test various backwards compatibility scenarios. Requires previous releases binaries,
11+see README.

MarcoFalke commented at 7:54 am on November 4, 2020:

0see test/README.md.

in test/functional/mempool_compatibility.py:10 in 256a76b48d outdated

 6@@ -7,10 +7,7 @@
 7 NOTE: The test is designed to prevent cases when compatibility is broken accidentally.
 8 In case we need to break mempool compatibility we can continue to use the test by just bumping the version number.
 9 
10-Download node binaries:
11-test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
12-
13-Only v0.15.2 is required by this test. The rest is used in other backwards compatibility tests.
14+The previous release v0.15.2 is required by this test, see README.

MarcoFalke commented at 7:55 am on November 4, 2020:

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

in test/functional/wallet_upgradewallet.py:9 in 256a76b48d outdated

 5@@ -6,9 +6,8 @@
 6 
 7 Test upgradewallet RPC. Download node binaries:
 8 
 9-test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2
10-
11-Only v0.15.2 and v0.16.3 are required by this test. The others are used in feature_backwards_compatibility.py
12+Requires previous releases binaries, see README.

MarcoFalke commented at 7:55 am on November 4, 2020:

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

MarcoFalke commented at 7:56 am on November 4, 2020: member

ACK, but there is also a test/functional/Readme

Sjors force-pushed on Nov 4, 2020

Sjors commented at 8:07 am on November 4, 2020: member

test/README seems focussed on how to run them, whereas test/functional/README only covers development. It might make sense to move some stuff over from test/README to test/functional/README though, but maybe in another PR?

Sjors force-pushed on Nov 4, 2020

DrahtBot added the label Needs rebase on Nov 19, 2020

Sjors force-pushed on Nov 19, 2020

DrahtBot removed the label Needs rebase on Nov 19, 2020

in test/get_previous_releases.py:64 in 0888343b6c outdated

60+"c378d4e21109f09e8829f3591e015c66632dff2925a60b64d259be05a334c30b":  "bitcoin-0.20.1-osx.dmg",
61+"fa71cb52ee5e0459cbf5248cdec72df27995840c796f58b304607a1ed4c165af":  "bitcoin-0.20.1-riscv64-linux-gnu.tar.gz",
62+"4bbd62fd6acfa5e9864ebf37a24a04bc2dcfe3e3222f056056288d854c53b978":  "bitcoin-0.20.1.tar.gz",
63+"930b96e774f5fe4795b9a3c0d4fd1da278d2b0777c9401dea3ba7453f8bbe14c":  "bitcoin-0.20.1-win64-setup.exe",
64+"e59fba67afce011d32b5d723a3a0be12da1b8a34f5d7966e504520c48d64716d":  "bitcoin-0.20.1-win64.zip",
65+"376194f06596ecfa40331167c39bc70c355f960280bd2a645fdbf18f66527397":  "bitcoin-0.20.1-x86_64-linux-gnu.tar.gz"

MarcoFalke commented at 3:57 pm on December 29, 2020:

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

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

michaelfolkson commented at 1:52 pm on December 30, 2020: contributor

ACK 0888343b6cc13d7449972345e4f576fd76a43919

Reviewed code, downloaded the previous release binaries, ran test, test passed.

Put this up on StackExchange, feel free to suggest improvements to my answer or add your own answer. https://bitcoin.stackexchange.com/questions/100924/what-backward-compatibility-testing-is-done-on-bitcoin-core

I’m also a fan of moving most the functional test docs to the functional test README and linking to it from the test README.

Will add to the possible docs improvements in https://github.com/bitcoin/bitcoin/issues/20132

DrahtBot added the label Needs rebase on Feb 8, 2021

Sjors commented at 4:29 pm on February 24, 2021: member

Rebased and simplified the code a bit. I dropped a few testing permutations in the process, but I don’t think those were important.

I also added v0.21.0

Sjors force-pushed on Feb 24, 2021

DrahtBot removed the label Needs rebase on Feb 24, 2021

Sjors renamed this:
~~test: add v0.20.1 to backwards compatibility test~~
test: add v0.20.1 and v0.21.0 to backwards compatibility test
on Feb 24, 2021

Sjors force-pushed on Feb 24, 2021

hebasto commented at 9:13 pm on February 24, 2021: member

Concept ACK.

hebasto commented at 10:49 am on February 25, 2021: member

Approach ACK cb646de919fd25505f9feaa2703b1acc16cffa1a

I don’t think it is required to add *-osx.dmg, *-win64*, and sources to the test/get_previous_releases.py.

Sjors commented at 2:31 pm on February 25, 2021: member

@hebasto it doesn’t hurt to commit these hashes in the repo though.

Sjors force-pushed on Mar 20, 2021

Sjors force-pushed on Apr 1, 2021

Sjors force-pushed on Apr 15, 2021

Sjors force-pushed on Apr 22, 2021

Sjors force-pushed on May 25, 2021

DrahtBot added the label Needs rebase on Jul 14, 2021

Sjors force-pushed on Jul 14, 2021

DrahtBot removed the label Needs rebase on Jul 14, 2021

in test/get_previous_releases.py:68 in 4c8a94e771 outdated

63+    "43416854330914992bbba2d0e9adf2a6fff4130be9af8ae2ef1186e743d9a3fe": "bitcoin-0.21.0-aarch64-linux-gnu.tar.gz",
64+    "f028af308eda45a3c4c90f9332f96b075bf21e3495c945ebce48597151808176": "bitcoin-0.21.0-arm-linux-gnueabihf.tar.gz",
65+    "695fb624fa6423f5da4f443b60763dd1d77488bfe5ef63760904a7b54e91298d": "bitcoin-0.21.0-osx64.tar.gz",
66+    "6223fd23d07133a6bfa2aa3d2554a09dc1d790d28ce67b0085d3fdcc1c126e05": "bitcoin-0.21.0-osx.dmg",
67+    "f8b2adfeae021a672effbc7bd40d5c48d6b94e53b2dd660f787340bf1a52e4e9": "bitcoin-0.21.0-riscv64-linux-gnu.tar.gz",
68+    "1a91202c62ee49fb64d57a52b8d6d01cd392fffcbef257b573800f9289655f37": "bitcoin-0.21.0.tar.gz",

MarcoFalke commented at 1:11 pm on July 14, 2021:

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

Sjors commented at 1:13 pm on July 14, 2021:

I’ll drop it for consistency.

Sjors force-pushed on Jul 14, 2021

Sjors commented at 1:32 pm on July 14, 2021: member

Slightly more complicated rebase after #20354.

Note how short the last commit is. It’s now much less involved to add a new version to this test.

DrahtBot added the label Needs rebase on Sep 16, 2021

Sjors force-pushed on Sep 22, 2021

Sjors renamed this:
~~test: add v0.20.1 and v0.21.0 to backwards compatibility test~~
test: add v0.20.1 , v0.21.0 and v22.0 to backwards compatibility test
on Sep 22, 2021

Sjors commented at 11:11 am on September 22, 2021: member

Rebased and added v22.0

in test/get_previous_releases.py:67 in a617b9d3b7 outdated

68+
69+    "ac718fed08570a81b3587587872ad85a25173afa5f9fbbd0c03ba4d1714cfa3e": "bitcoin-22.0-aarch64-linux-gnu.tar.gz",
70+    "b8713c6c5f03f5258b54e9f436e2ed6d85449aa24c2c9972f91963d413e86311": "bitcoin-22.0-arm-linux-gnueabihf.tar.gz",
71+    "2744d199c3343b2d94faffdfb2c94d75a630ba27301a70e47b0ad30a7e0155e9": "bitcoin-22.0-osx64.tar.gz",
72+    "2cca5f99007d060aca9d8c7cbd035dfe2f040dd8200b210ce32cdf858479f70d": "bitcoin-22.0-powerpc64-linux-gnu.tar.gz",
73+    "91b1e012975c5a363b5b5fcc81b5b7495e86ff703ec8262d4b9afcfec633c30d": "bitcoin-22.0-powerpc64le-linux-gnu.tar.gz",

MarcoFalke commented at 11:31 am on September 22, 2021:

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.

MarcoFalke commented at 11:35 am on September 22, 2021:

Maybe add a commit to remove i686 for that reason?

 0commit 6fd3fd1ba76831d83a43e9322e78afca70aabbeb (HEAD)
 1Author: MarcoFalke <falke.marco@gmail.com>
 2Date:   Wed Sep 22 13:35:01 2021 +0200
 3
 4    test: Remove i686 from test/get_previous_releases.py
 5    
 6    It is not possible to run the compatibility tests on i686 because the
 7    releases v20+ are missing for that arch. It would be possible to
 8    self-compile those releases, but then one can also self-compile
 9    0.15-0.19.
10
11diff --git a/test/get_previous_releases.py b/test/get_previous_releases.py
12index 62fcad04b3..27f7044094 100755
13--- a/test/get_previous_releases.py
14+++ b/test/get_previous_releases.py
15@@ -23,32 +23,27 @@ import hashlib
16 SHA256_SUMS = {
17     "d40f18b4e43c6e6370ef7db9131f584fbb137276ec2e3dba67a4b267f81cb644": "bitcoin-0.15.2-aarch64-linux-gnu.tar.gz",
18     "54fb877a148a6ad189a1e1ab1ff8b11181e58ff2aaf430da55b3fd46ae549a6b": "bitcoin-0.15.2-arm-linux-gnueabihf.tar.gz",
19-    "2b843506c3f1af0eeca5854a920264f9a829f02d0d50328005950ddcbe88874d": "bitcoin-0.15.2-i686-pc-linux-gnu.tar.gz",
20     "87e9340ff3d382d543b2b69112376077f0c8b4f7450d372e83b68f5a1e22b2df": "bitcoin-0.15.2-osx64.tar.gz",
21     "566be44190fd76daa01f13d428939dadfb8e3daacefc8fa17f433cad28f73bd5": "bitcoin-0.15.2-x86_64-linux-gnu.tar.gz",
22     #
23     "0768c6c15caffbaca6524824c9563b42c24f70633c681c2744649158aa3fd484": "bitcoin-0.16.3-aarch64-linux-gnu.tar.gz",
24     "fb2818069854a6ad20ea03b28b55dbd35d8b1f7d453e90b83eace5d0098a2a87": "bitcoin-0.16.3-arm-linux-gnueabihf.tar.gz",
25-    "75a537844313b0a84bdb61ffcdc5c4ce19a738f7ddf71007cd2edf664efd7c37": "bitcoin-0.16.3-i686-pc-linux-gnu.tar.gz",
26     "78c3bff3b619a19aed575961ea43cc9e142959218835cf51aede7f0b764fc25d": "bitcoin-0.16.3-osx64.tar.gz",
27     "5d422a9d544742bc0df12427383f9c2517433ce7b58cf672b9a9b17c2ef51e4f": "bitcoin-0.16.3-x86_64-linux-gnu.tar.gz",
28     #
29     "5a6b35d1a348a402f2d2d6ab5aed653a1a1f13bc63aaaf51605e3501b0733b7a": "bitcoin-0.17.2-aarch64-linux-gnu.tar.gz",
30     "d1913a5d19c8e8da4a67d1bd5205d03c8614dfd2e02bba2fe3087476643a729e": "bitcoin-0.17.2-arm-linux-gnueabihf.tar.gz",
31-    "d295fc93f39bbf0fd937b730a93184899a2eb6c3a6d53f3d857cbe77ef89b98c": "bitcoin-0.17.2-i686-pc-linux-gnu.tar.gz",
32     "a783ba20706dbfd5b47fbedf42165fce70fbbc7d78003305d964f6b3da14887f": "bitcoin-0.17.2-osx64.tar.gz",
33     "943f9362b9f11130177839116f48f809d83478b4c28591d486ee9a7e35179da6": "bitcoin-0.17.2-x86_64-linux-gnu.tar.gz",
34     #
35     "88f343af72803b851c7da13874cc5525026b0b55e63e1b5e1298390c4688adc6": "bitcoin-0.18.1-aarch64-linux-gnu.tar.gz",
36     "cc7d483e4b20c5dabd4dcaf304965214cf4934bcc029ca99cbc9af00d3771a1f": "bitcoin-0.18.1-arm-linux-gnueabihf.tar.gz",
37-    "989e847b3e95fc9fedc0b109cae1b4fa43348f2f712e187a118461876af9bd16": "bitcoin-0.18.1-i686-pc-linux-gnu.tar.gz",
38     "b7bbcee7a7540f711b171d6981f939ca8482005fde22689bc016596d80548bb1": "bitcoin-0.18.1-osx64.tar.gz",
39     "425ee5ec631ae8da71ebc1c3f5c0269c627cf459379b9b030f047107a28e3ef8": "bitcoin-0.18.1-riscv64-linux-gnu.tar.gz",
40     "600d1db5e751fa85903e935a01a74f5cc57e1e7473c15fd3e17ed21e202cfe5a": "bitcoin-0.18.1-x86_64-linux-gnu.tar.gz",
41     #
42     "3a80431717842672df682bdb619e66523b59541483297772a7969413be3502ff": "bitcoin-0.19.1-aarch64-linux-gnu.tar.gz",
43     "657f28213823d240dd3324d14829702f9ad6f0710f8bdd1c379cb3c447197f48": "bitcoin-0.19.1-arm-linux-gnueabihf.tar.gz",
44-    "10d1e53208aa7603022f4acc084a046299ab4ccf25fe01e81b3fb6f856772589": "bitcoin-0.19.1-i686-pc-linux-gnu.tar.gz",
45     "1ae1b87de26487075cd2fd22e0d4ead87d969bd55c44f2f1d873ecdc6147ebb3": "bitcoin-0.19.1-osx64.tar.gz",
46     "aa7a9563b48aa79252c8e7b6a41c07a5441bd9f14c5e4562cc72720ea6cb0ee5": "bitcoin-0.19.1-riscv64-linux-gnu.tar.gz",
47     "5fcac9416e486d4960e1a946145566350ca670f9aaba99de6542080851122e4c": "bitcoin-0.19.1-x86_64-linux-gnu.tar.gz",

Sjors commented at 11:45 am on September 22, 2021:

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

MarcoFalke commented at 11:52 am on September 22, 2021:

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

Sjors commented at 11:59 am on September 22, 2021:

Got it. Pushed.

DrahtBot removed the label Needs rebase on Sep 22, 2021

MarcoFalke renamed this:
~~test: add v0.20.1 , v0.21.0 and v22.0 to backwards compatibility test~~
test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test
on Sep 22, 2021

katesalazar commented at 9:27 am on September 26, 2021: contributor

Came by only to add a friendly reminder that once this is merged, it could be interesting to replace obsolete minor versions in a subsequent PR.

You could consider back porting as well.

I think this change could be of help for newcomers. Thank you very much!

Sjors commented at 9:40 am on September 27, 2021: member

@katesalazar v0.21.0 is very different from v0.21.1 because the latter has taproot enabled. Not upgrading that one is intentional. For the other releases I don’t think it matters too much which patch version we use here. The purpose of these tests is to find backwards compatibility issues. Ideally we would run every single patch version for that, but that might make the tests too slow.

in test/functional/feature_backwards_compatibility.py:60 in a617b9d3b7 outdated

56@@ -56,6 +57,7 @@ def setup_nodes(self):
57         self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
58             None,
59             None,
60+            22000000,

achow101 commented at 4:59 pm on November 2, 2021:

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?

Sjors commented at 7:39 pm on November 2, 2021:

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

Sjors commented at 2:05 pm on November 5, 2021: member

Rebased and changed the client version in the test from 22000000 to 220000.

Sjors force-pushed on Nov 5, 2021

Sjors force-pushed on Nov 14, 2021

katesalazar commented at 7:25 pm on November 14, 2021: contributor

I wonder if 9e03b891af is enough self explanatory about the bugs it fixes.

It wouldn’t hurt me adding a thorough detailed text to it.

Sjors commented at 8:57 am on November 15, 2021: member

@katesalazar that commit ensures that we we add a new version to the list, everything keeps working. It also adds some missing node_master.unloadwallet statements. I wrote it more than a year ago, so that’s all I remember :-)

I guess the test failure in feature_backwards_compatibility.py --legacy-wallet is not spurious, because it happened before the rebase too. But I can’t reproduce it on macOS. Update: reproduced on Ubuntu…

Sjors force-pushed on Nov 15, 2021

Sjors commented at 10:43 am on November 15, 2021: member

The test framework sets -sandbox=log-and-abort for version 22.0 and up, but that’s not part of the release binary. I added a commit to fix that.

I’ll rebase after #23514 is fixed.

Sjors force-pushed on Nov 15, 2021

Sjors commented at 4:24 pm on November 15, 2021: member

Rebased after #23515 and merge conflict.

in test/functional/feature_backwards_compatibility.py:79 in f35faefac0 outdated

71@@ -72,7 +72,7 @@ def run_test(self):
72         res = self.nodes[self.num_nodes - 1].getblockchaininfo()
73         assert_equal(res['blocks'], COINBASE_MATURITY + 1)
74 
75-        node_master = self.nodes[self.num_nodes - 5]
76+        node_master = self.nodes[1]

ryanofsky commented at 6:53 pm on November 17, 2021:

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.

katesalazar commented at 8:08 pm on November 17, 2021:

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

[0]: #19013 (comment)

Sjors commented at 2:39 pm on November 18, 2021:

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 test/get_previous_releases.py:59 in 94d3989352 outdated

50@@ -51,7 +51,6 @@
51     "60c93e3462c303eb080be7cf623f1a7684b37fd47a018ad3848bc23e13c84e1c": "bitcoin-0.20.1-aarch64-linux-gnu.tar.gz",
52     "55b577e0fb306fb429d4be6c9316607753e8543e5946b542d75d876a2f08654c": "bitcoin-0.20.1-arm-linux-gnueabihf.tar.gz",
53     "b9024dde373ea7dad707363e07ec7e265383204127539ae0c234bff3a61da0d1": "bitcoin-0.20.1-osx64.tar.gz",
54-    "c378d4e21109f09e8829f3591e015c66632dff2925a60b64d259be05a334c30b": "bitcoin-0.20.1-osx.dmg",

ryanofsky commented at 6:57 pm on November 17, 2021:

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?

Sjors commented at 2:13 pm on November 18, 2021:

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

in test/functional/feature_backwards_compatibility.py:162 in 94d3989352 outdated

210-        for wallet in os.listdir(node_master_wallets_dir):
211-            shutil.copytree(
212-                os.path.join(node_master_wallets_dir, wallet),
213-                os.path.join(node_v19_wallets_dir, wallet)
214-            )
215+        for node in self.nodes[2:]:

ryanofsky commented at 7:06 pm on November 17, 2021:

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]

?

Sjors commented at 2:13 pm on November 18, 2021:

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

in test/functional/feature_backwards_compatibility.py:214 in 94d3989352 outdated

352+            for node in self.nodes[2:-1]:
353+                # Descriptor wallets appear to be corrupted wallets to old software
354+                # and loadwallet is introduced in v0.17.0
355+                if node.version >= 170000 and node.version < 210000:
356+                    for wallet_name in ["w1", "w2", "w3"]:
357+                        assert_raises_rpc_error(-4, "Wallet file verification failed: wallet.dat corrupt, salvage failed", node.loadwallet, wallet_name)

ryanofsky commented at 7:12 pm on November 17, 2021:

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.

Sjors commented at 2:10 pm on November 18, 2021:

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

in test/functional/test_framework/test_framework.py:453 in 62b22de69d outdated

446@@ -447,11 +447,13 @@ def add_nodes(self, num_nodes: int, extra_args=None, *, rpchost=None, binary=Non
447         def get_bin_from_version(version, bin_name, bin_default):
448             if not version:
449                 return bin_default
450+            if version > 219999:
451+                version *= 100

ryanofsky commented at 7:15 pm on November 17, 2021:

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.

Sjors commented at 1:55 pm on November 18, 2021:

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.

ryanofsky approved

ryanofsky commented at 7:23 pm on November 17, 2021: member

Code review ACK 62b22de69dabfb86eaabb6a685e3989ce525d0e3. This all looks very good, and the deduping is nice. Left suggestions below but they aren’t important and are just questions and requests to explain some things better.

Sjors force-pushed on Nov 18, 2021

Sjors commented at 2:41 pm on November 18, 2021: member

Rebased, moved refactoring stuff out of the v0.20 commit into 408e9b4. I improved that commit further by using variables in more places instead of indexes.

in test/functional/feature_backwards_compatibility.py:171 in 408e9b4876 outdated

291-            info = wallet.getwalletinfo()
292-            assert info['private_keys_enabled'] == False
293-            assert info['keypoolsize'] == 0
294+            # Load modern wallet with older nodes
295+            for node in legacy_nodes:
296+              for wallet_name in ["w1", "w2", "w3"]:

ryanofsky commented at 5:50 pm on November 30, 2021:

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

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

in test/functional/feature_backwards_compatibility.py:163 in 408e9b4876 outdated

153-        info = wallet.getwalletinfo()
154-        assert info['private_keys_enabled']
155-        assert info['keypoolsize'] == 0
156-
157-        # w3_v18: blank wallet, created with v0.18
158-        node_v18.rpc.createwallet(wallet_name="w3_v18", blank=True)

ryanofsky commented at 6:45 pm on November 30, 2021:

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 test/functional/test_framework/test_framework.py:481 in b39643f88e outdated

472@@ -473,7 +473,7 @@ def get_bin_from_version(version, bin_name, bin_default):
473             versions = [None] * num_nodes
474         if self.is_syscall_sandbox_compiled() and not self.disable_syscall_sandbox:
475             for i in range(len(extra_args)):
476-                if versions[i] is None or versions[i] >= 219900:
477+                if versions[i] is None or versions[i] >= 229900:

ryanofsky commented at 6:53 pm on November 30, 2021:

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)

ryanofsky approved

ryanofsky commented at 7:05 pm on November 30, 2021: member

Code review ACK 7124899057a351a3491b288d96426f91ef1fd040. This PR looks great and now is simpler to review with code changes consolidated in one commit, and new releases added in other commits. Other than that, only other changes since last review are replacing hardcoded node indexes with variables, and adding a code and commit comments

Sjors commented at 5:23 am on December 1, 2021: member

Thanks @ryanofsky. I fixed the indentation and added the requested comment.

Sjors force-pushed on Dec 1, 2021

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.

76557cbe4c

test: backwards compatibility: misc fixes

This cleanup may slightly impact test coverage.

Reduce code repition by looping over the list of nodes.

Reduce brittleness by refering to nodes by name rather than index.

Add helper method nodes_wallet_dir.

0e4b695b6a

test: v0.20.1 backwards compatibility

The file checksums were added in an earlier commit. Since the DMG
file is never downloaded, we drop that checksum.

8cba75f5fd

test: previous releases: add v0.21.0 8a57a06a50

test: bump sandbox argument minimum version

The -sandbox argument is not present in the v22.0 release. Changing the minimum version to 229900 ensures it's used when testing the master branch.

If the argument is backported, the minimum version can be adjusted to e.g. 220100.

40849eebd9

test: previous releases: add v22.0 d8b705f1ca

test: Fix intermittent test failure in feature_backwards_compatibility 24cec4b5c0

Sjors force-pushed on Dec 16, 2021

Sjors commented at 5:51 am on December 16, 2021: member

Rebased and included #19332.

ryanofsky approved

ryanofsky commented at 4:19 pm on February 24, 2022: member

Code review ACK 24cec4b5c02e12cf0b6b56ba5055b8f5758627a5. Only change since last review is rebasing and adding comment and whitelist args.

I’m surprised this PR is still open. It seems useful and not a particularly risky change to merge.

MarcoFalke merged this on Feb 24, 2022

MarcoFalke closed this on Feb 24, 2022

Sjors commented at 7:24 pm on February 24, 2022: member

🎉

Sjors deleted the branch on Feb 24, 2022

DrahtBot locked this on Feb 24, 2023

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

Conflicts