test: Add feature_taproot.py --previous

MarcoFalke commented at 2:58 pm on November 9, 2020: member

Disabling the new consensus code at runtime is fine, but potentially fragile and incomplete. Fix that by giving the option to run with a version that has been compiled without any taproot code.

DrahtBot added the label Tests on Nov 9, 2020

DrahtBot commented at 5:13 pm on November 9, 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:

#19013 by Sjors
#10102 by ryanofsky

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.

luke-jr commented at 6:23 pm on November 13, 2020: member

Concept ACK

theStack commented at 8:46 pm on November 15, 2020: member

Concept ACK

michaelfolkson commented at 1:11 pm on December 29, 2020: contributor

The motivation for this is that someone might run the functional tests of a post Taproot release with the code of a pre Taproot release (without any Taproot code)? I’m not entirely sure on the motivation.

Also I’d like to review and test this but unsure how the feature_taproot.py --previous_release test should get triggered. Obviously I can run this test individually but I don’t think there is much point to that.

I asked on IRC and @jonatack pointed me to -vbparams config option which requires a start and end time “for specified version bits deployment” Let me know if this is the best way to test this.

MarcoFalke commented at 2:31 pm on December 29, 2020: member

Taproot is a softfork that comes with some consensus and policy (transaction relay) changes. While there is a way to “remove” the taproot code at runtime with the vbparams option, there is currently no formal proof that none of the taproot code is executed at runtime. It was unknown if the new taproot code influences the test result of this test in any way.

The subtest checks that taproot is a softfork, i.e. nodes without the taproot code can still sync with the network at a small cost of missing some consensus checks. As said, vbparams can be used to disable the taproot code. A cleaner approach is to run the test against a previously released version of a full node that isn’t aware of taproot.

You can run the test manually with the --previous_release or via the test_runner’s built-in list.

If you want to test -vbparams outside of what the test does, you can pass it as an option to Bitcoin Core.

MarcoFalke commented at 2:34 pm on December 29, 2020: member

You’ll also have to download the previous release first. See https://github.com/bitcoin/bitcoin/pull/19013/files#diff-5de36acd90308dc62abf7855a686ee7052ffb6e762c756fd735fb0c9fbd9595d for instructions.

michaelfolkson commented at 5:45 pm on December 30, 2020: contributor

ACK fa3c6150defd35d112d41597d8a9ae6d908e8add

Ran ./test/get_previous_releases.py -b v0.19.1 v0.18.1 v0.17.2 v0.16.3 v0.15.2 though only needed one previous release. Didn’t include v0.20.1 as that isn’t added yet. (Added in #19013)

Then feature_taproot.py --previous_release test passes. Reviewed code as well. Test successfully skips when you haven’t downloaded a previous release.

instagibbs commented at 8:08 am on January 8, 2021: member

If I wanted to intentionally break this, how do I mess with “older releases”? There documentation for this testing feature?

MarcoFalke commented at 8:23 am on January 8, 2021: member

See https://github.com/bitcoin/bitcoin/pull/19013/files#diff-5de36acd90308dc62abf7855a686ee7052ffb6e762c756fd735fb0c9fbd9595d for instructions.

in test/functional/feature_taproot.py:518 in fa3c6150de outdated

516@@ -517,9 +517,9 @@ def random_checksig_style(pubkey):
517     """Creates a random CHECKSIG* tapscript that would succeed with only the valid signature on witness stack."""
518     return bytes(CScript([pubkey, OP_CHECKSIG]))

brunoerg commented at 12:05 pm on February 4, 2021:

I think this function should be refactored since the code after that line (518) won’t be reached since there is a return right at the beginning of the function.

MarcoFalke commented at 12:32 pm on February 4, 2021:

Mind creating a pull for that, since it is completely unrelated to the changes here? The correct fix might be to remove this line

brunoerg commented at 12:46 pm on February 4, 2021:

I did it. See #21081

DrahtBot added the label Needs rebase on Feb 23, 2021

MarcoFalke force-pushed on Feb 24, 2021

MarcoFalke commented at 8:51 am on February 24, 2021: member

Rebased

in test/functional/feature_taproot.py:1217 in fa2811bcd8 outdated

1213+        else:
1214+            self.extra_args[0].append("-vbparams=taproot:1:1")
1215+
1216+    def setup_nodes(self):
1217+        self.add_nodes(self.num_nodes, self.extra_args, versions=[
1218+            170200 if self.options.previous_release else None,

Sjors commented at 9:27 am on February 24, 2021:

Maybe use the most recent release without taproot code, e.g. 0.20? That way if we have to ditch older versions due to breaking test API changes, this code doesn’t need an update.

MarcoFalke commented at 2:28 pm on February 24, 2021:

thx, done

Sjors commented at 9:27 am on February 24, 2021: member

Concept ACK

DrahtBot removed the label Needs rebase on Feb 24, 2021

MarcoFalke force-pushed on Feb 24, 2021

MarcoFalke force-pushed on Feb 25, 2021

test: previous releases: add v0.20.1

Can be reviewed with --ignore-all-space

29d6b1da2a

test: move releases download incantation to README 85ccffa266

test: Add feature_taproot.py --previous_release fa80e10d94

MarcoFalke force-pushed on Feb 25, 2021

MarcoFalke commented at 9:36 am on February 25, 2021: member

(force pushed to tidy up the first commit)

Sjors commented at 9:50 am on February 25, 2021: member

I was a bit worried that this PR would break #19013, but the subset of changes you’re using is simple enough to rebase on (this PR just downloads the binary and doesn’t touch the other backwards compatibility tests).

tACK fa80e10 @instagibbs replace releases/v0.20.1/bin/bitcoind with ransomware :-) @marcofalke your excellent explanation would make a nice comment in the test.

MarcoFalke commented at 1:35 pm on February 25, 2021: member

Will do in a follow-up or if I have to force push.

NelsonGaldeman commented at 3:34 pm on July 13, 2021: contributor

I ran it and succesfully skipped the test when previous releases are missing. After downloading previous versions it ran and tests pass! Code looks fine.

tACK fa80e10d94dbf86da84fc761b09fb631155a5b25

Am I right saying it will only be ran by the test runner for people who already had older versions compiled or purposely downloaded binaries?

Unrelated to the PR itself but my first try to download previous versions ended up in a Checksum did not match on the 0.21.0. Tried again and worked fine. Checked both files size showed on the console and they were slightly different. Update on above: https://github.com/bitcoin/bitcoin/pull/22442

MarcoFalke commented at 3:41 pm on July 13, 2021: member

Am I right saying it will only be ran by the test runner for people who already had older versions compiled or purposely downloaded binaries?

Yes. This is the case for the ci config (one task is using downloaded binaries) and locally if you have compiled/downloaded the binaries.

Unrelated to the PR itself but my first try to download previous versions ended up in a Checksum did not match on the 0.21.0. Tried again and worked fine. Checked both files size showed on the console and they were slightly different.

This is a “known” issue. See https://github.com/bitcoin-core/bitcoincore.org/issues/753

TheBlueMatt commented at 2:58 am on July 14, 2021: member

This is a “known” issue. See bitcoin-core/bitcoincore.org#753

This is absolutely not a known issue. Not at all. Not even remotely.

MarcoFalke referenced this in commit 531c2b7c04 on Jul 14, 2021

DrahtBot added the label Needs rebase on Jul 14, 2021

DrahtBot commented at 11:02 am on July 14, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

fanquake commented at 11:12 am on July 14, 2021: member

Looks like this has been merged?

fanquake closed this on Jul 14, 2021

MarcoFalke deleted the branch on Jul 14, 2021

MarcoFalke commented at 11:15 am on July 14, 2021: member

Jup, it’s the usual brokenness of GitHub.

Sjors commented at 12:34 pm on July 14, 2021: member

Ah, this took a few commits from #19013. I’m rebasing that now…

sidhujag referenced this in commit 9f1d429b17 on Jul 14, 2021

gwillen referenced this in commit ce53627c25 on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

test: Add feature_taproot.py –previous_release #20354

Conflicts