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

Sjors · 2020-05-19T10:08:00Z

This also simplifies the tests a bit.

DrahtBot · 2020-05-19T20:20:41Z

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.

maflcko

Concept ACK

test/functional/feature_backwards_compatibility.py

test/functional/test_framework/test_framework.py

test/functional/test_framework/test_node.py

Sjors · 2020-07-21T12:41:34Z

Rebased and dropped the release candidate commit.

maflcko · 2020-07-21T13:51:53Z

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

katesalazar · 2021-11-14T19:25:22Z

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

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

Sjors · 2021-11-15T08:57:40Z

@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 · 2021-11-15T10:43:52Z

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 · 2021-11-15T16:24:59Z

Rebased after #23515 and merge conflict.

ryanofsky

Code review ACK 62b22de. 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.

ryanofsky · 2021-11-17T18:53:31Z

test/functional/feature_backwards_compatibility.py

@@ -72,7 +72,7 @@ def run_test(self):
        res = self.nodes[self.num_nodes - 1].getblockchaininfo()
        assert_equal(res['blocks'], COINBASE_MATURITY + 1)

-        node_master = self.nodes[self.num_nodes - 5]
+        node_master = self.nodes[1]


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

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, clearly expressed.

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.

ryanofsky · 2021-11-17T18:57:23Z

test/get_previous_releases.py

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


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

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.

ryanofsky · 2021-11-17T19:06:50Z

test/functional/feature_backwards_compatibility.py

-                os.path.join(node_master_wallets_dir, wallet),
-                os.path.join(node_v19_wallets_dir, wallet)
-            )
+        for node in self.nodes[2:]:


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

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.

ryanofsky · 2021-11-17T19:12:05Z

test/functional/feature_backwards_compatibility.py

+                # and loadwallet is introduced in v0.17.0
+                if node.version >= 170000 and node.version < 210000:
+                    for wallet_name in ["w1", "w2", "w3"]:
+                        assert_raises_rpc_error(-4, "Wallet file verification failed: wallet.dat corrupt, salvage failed", node.loadwallet, wallet_name)


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

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.

ryanofsky · 2021-11-17T19:15:03Z

test/functional/test_framework/test_framework.py

+            if version > 219999:
+                version *= 100


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

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.

Sjors · 2021-11-18T14:41:24Z

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.

ryanofsky

Code review ACK 7124899. 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

test/functional/feature_backwards_compatibility.py

ryanofsky · 2021-11-30T18:45:10Z

test/functional/feature_backwards_compatibility.py

-        assert info['keypoolsize'] == 0
-
-        # w3_v18: blank wallet, created with v0.18
-        node_v18.rpc.createwallet(wallet_name="w3_v18", blank=True)


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

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.

test/functional/test_framework/test_framework.py

Sjors · 2021-12-01T05:23:04Z

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

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.

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.

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

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.

Sjors · 2021-12-16T05:51:48Z

Rebased and included #19332.

ryanofsky

Code review ACK 24cec4b. 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.

Sjors · 2022-02-24T19:24:15Z

🎉

fanquake added the Tests label May 19, 2020

DrahtBot mentioned this pull request May 19, 2020

test: Replace TEST_PREVIOUS_RELEASES env var with test_framework option #19014

Merged

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 01e66bf to b998f69 Compare May 22, 2020 11:49

Sjors force-pushed the 2020/05/previous_release_0.20 branch from b998f69 to 3181f0a Compare June 3, 2020 10:20

Sjors marked this pull request as ready for review June 3, 2020 10:21

This was referenced Jun 4, 2020

Refactor: Improve setup_clean_chain semantics #19168

Closed

test: move sync_blocks and sync_mempool functions to test_framework.py #19208

Merged

Sjors mentioned this pull request Jun 15, 2020

wallet: Skip hdKeypath of 'm' when determining inactive hd seeds #19054

Merged

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 3181f0a to 5a9152f Compare June 16, 2020 11:35

maflcko reviewed Jun 16, 2020

View reviewed changes

DrahtBot added the Needs rebase label Jun 16, 2020

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 5a9152f to 568b6c8 Compare June 17, 2020 15:35

DrahtBot removed the Needs rebase label Jun 17, 2020

DrahtBot added the Needs rebase label Jun 22, 2020

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 568b6c8 to 3619923 Compare June 29, 2020 18:38

DrahtBot removed the Needs rebase label Jun 29, 2020

This was referenced Jul 2, 2020

script: previous_release.sh rewritten in python #19205

Merged

Replace boost::filesystem with std::filesystem #19245

Closed

DrahtBot added the Needs rebase label Jul 21, 2020

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 3619923 to 7628ed0 Compare July 21, 2020 12:41

DrahtBot removed the Needs rebase label Jul 21, 2020

DrahtBot mentioned this pull request Nov 8, 2021

test: Enable SC2046 and SC2086 shellcheck rules #23462

Merged

Sjors force-pushed the 2020/05/previous_release_0.20 branch from ce3ffe5 to 77e3c9e Compare November 14, 2021 18:16

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 77e3c9e to 98a65b8 Compare November 15, 2021 10:42

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 98a65b8 to 62b22de Compare November 15, 2021 16:24

ryanofsky approved these changes Nov 17, 2021

View reviewed changes

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 62b22de to 7124899 Compare November 18, 2021 14:39

ryanofsky approved these changes Nov 30, 2021

View reviewed changes

Sjors force-pushed the 2020/05/previous_release_0.20 branch from 7124899 to ef08ba1 Compare December 1, 2021 05:23

MarcoFalke and others added 7 commits December 16, 2021 12:41

test: Remove i686 from test/get_previous_releases.py

76557cb

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.

test: backwards compatibility: misc fixes

0e4b695

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.

test: v0.20.1 backwards compatibility

8cba75f

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

test: previous releases: add v0.21.0

8a57a06

test: bump sandbox argument minimum version

40849ee

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.

test: previous releases: add v22.0

d8b705f

test: Fix intermittent test failure in feature_backwards_compatibility

24cec4b

Sjors force-pushed the 2020/05/previous_release_0.20 branch from ef08ba1 to 24cec4b Compare December 16, 2021 05:51

DrahtBot mentioned this pull request Feb 4, 2022

Remove utxo db upgrade code #24236

Merged

DrahtBot mentioned this pull request Feb 20, 2022

build: Fix Boost.Process check for Boost 1.73 and older #24397

Merged

ryanofsky approved these changes Feb 24, 2022

View reviewed changes

maflcko merged commit f1ce67f into bitcoin:master Feb 24, 2022

Sjors deleted the 2020/05/previous_release_0.20 branch February 24, 2022 19:24

bitcoin locked and limited conversation to collaborators Feb 24, 2023

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

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

Uh oh!

Conversation

Sjors commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jul 21, 2020

Uh oh!

maflcko commented Jul 21, 2020

Uh oh!

katesalazar commented Nov 14, 2021

Uh oh!

Sjors commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 15, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 18, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Dec 1, 2021

Uh oh!

Sjors commented Dec 16, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Feb 24, 2022

Uh oh!

Uh oh!

Sjors commented May 19, 2020 •

edited

Loading

DrahtBot commented May 19, 2020 •

edited

Loading

Sjors commented Nov 15, 2021 •

edited

Loading

Sjors commented Nov 15, 2021 •

edited

Loading

Sjors Nov 18, 2021 •

edited

Loading