test: Add feature_taproot.py –previous_release #20354

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2010-testFeatureTaprootPreviousVersion changing 8 files +73 −46
  1. 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.
  2. DrahtBot added the label Tests on Nov 9, 2020
  3. 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:

    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.

  4. luke-jr commented at 6:23 pm on November 13, 2020: member
    Concept ACK
  5. theStack commented at 8:46 pm on November 15, 2020: member
    Concept ACK
  6. 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.

  7. 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.

  8. 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.
  9. 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.

  10. 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?
  12. 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
  13. DrahtBot added the label Needs rebase on Feb 23, 2021
  14. MarcoFalke force-pushed on Feb 24, 2021
  15. MarcoFalke commented at 8:51 am on February 24, 2021: member
    Rebased
  16. 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
  17. Sjors commented at 9:27 am on February 24, 2021: member
    Concept ACK
  18. DrahtBot removed the label Needs rebase on Feb 24, 2021
  19. MarcoFalke force-pushed on Feb 24, 2021
  20. MarcoFalke force-pushed on Feb 25, 2021
  21. test: previous releases: add v0.20.1 
    Can be reviewed with --ignore-all-space
    29d6b1da2a
  22. test: move releases download incantation to README 85ccffa266
  23. test: Add feature_taproot.py --previous_release fa80e10d94
  24. MarcoFalke force-pushed on Feb 25, 2021
  25. MarcoFalke commented at 9:36 am on February 25, 2021: member
    (force pushed to tidy up the first commit)
  26. 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.

  27. MarcoFalke commented at 1:35 pm on February 25, 2021: member
    Will do in a follow-up or if I have to force push.
  28. 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

  29. 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

  30. 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.

  31. MarcoFalke referenced this in commit 531c2b7c04 on Jul 14, 2021
  32. DrahtBot added the label Needs rebase on Jul 14, 2021
  33. 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”.

  34. fanquake commented at 11:12 am on July 14, 2021: member
    Looks like this has been merged?
  35. fanquake closed this on Jul 14, 2021
  36. MarcoFalke deleted the branch on Jul 14, 2021
  37. MarcoFalke commented at 11:15 am on July 14, 2021: member
    Jup, it’s the usual brokenness of GitHub.
  38. Sjors commented at 12:34 pm on July 14, 2021: member
    Ah, this took a few commits from #19013. I’m rebasing that now…
  39. sidhujag referenced this in commit 9f1d429b17 on Jul 14, 2021
  40. gwillen referenced this in commit ce53627c25 on Jun 1, 2022
  41. DrahtBot locked this on Aug 18, 2022


MarcoFalke DrahtBot luke-jr theStack michaelfolkson instagibbs brunoerg Sjors NelsonGaldeman TheBlueMatt fanquake

Labels
Tests Needs rebase

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me