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: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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. DrahtBot cross-referenced this on Nov 10, 2020 from issue External signer support - Wallet Box edition by Sjors
  5. luke-jr commented at 6:23 PM on November 13, 2020: member

    Concept ACK

  6. theStack commented at 8:46 PM on November 15, 2020: contributor

    Concept ACK

  7. DrahtBot cross-referenced this on Nov 26, 2020 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  8. DrahtBot cross-referenced this on Nov 26, 2020 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  9. DrahtBot cross-referenced this on Nov 26, 2020 from issue Multiprocess bitcoin by ryanofsky
  10. 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.

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

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

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

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

  16. DrahtBot cross-referenced this on Feb 3, 2021 from issue Replace unused BIP 9 logic with draft BIP 8 by luke-jr
  17. 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

  18. michaelfolkson cross-referenced this on Feb 4, 2021 from issue test: fix the unreachable code at feature_taproot by brunoerg
  19. DrahtBot cross-referenced this on Feb 6, 2021 from issue wallet: check when create wallets for the reserved name "wallets" by Saibato
  20. DrahtBot added the label Needs rebase on Feb 23, 2021
  21. MarcoFalke force-pushed on Feb 24, 2021
  22. MarcoFalke commented at 8:51 AM on February 24, 2021: member

    Rebased

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

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

    Concept ACK

  25. DrahtBot removed the label Needs rebase on Feb 24, 2021
  26. DrahtBot cross-referenced this on Feb 24, 2021 from issue tests: Run both descriptor and legacy tests within a single test invocation by achow101
  27. MarcoFalke force-pushed on Feb 24, 2021
  28. DrahtBot cross-referenced this on Feb 25, 2021 from issue test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test by Sjors
  29. MarcoFalke force-pushed on Feb 25, 2021
  30. test: previous releases: add v0.20.1
    Can be reviewed with --ignore-all-space
    29d6b1da2a
  31. test: move releases download incantation to README 85ccffa266
  32. test: Add feature_taproot.py --previous_release fa80e10d94
  33. MarcoFalke force-pushed on Feb 25, 2021
  34. MarcoFalke commented at 9:36 AM on February 25, 2021: member

    (force pushed to tidy up the first commit)

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

  36. MarcoFalke commented at 1:35 PM on February 25, 2021: member

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

  37. DrahtBot cross-referenced this on Mar 6, 2021 from issue Convert taproot to flag day activation by ajtowns
  38. DrahtBot cross-referenced this on Mar 9, 2021 from issue BIP 341: Add Speedy Trial activation parameters by achow101
  39. DrahtBot cross-referenced this on Mar 9, 2021 from issue Implement BIP 8 based Speedy Trial activation by achow101
  40. DrahtBot cross-referenced this on Mar 10, 2021 from issue Refactor versionbits deployments to avoid potential uninitialized variables by achow101
  41. DrahtBot cross-referenced this on Mar 22, 2021 from issue Implement BIP8 lockinontimeout by achow101
  42. DrahtBot cross-referenced this on Jul 8, 2021 from issue Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift
  43. 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

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

  45. MarcoFalke cross-referenced this on Jul 13, 2021 from issue Downloads proceed rapidly, then stall by Empact
  46. TheBlueMatt commented at 2:58 AM on July 14, 2021: contributor

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

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

  47. MarcoFalke referenced this in commit 531c2b7c04 on Jul 14, 2021
  48. DrahtBot added the label Needs rebase on Jul 14, 2021
  49. DrahtBot commented at 11:02 AM on July 14, 2021: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  50. fanquake commented at 11:12 AM on July 14, 2021: member

    Looks like this has been merged?

  51. fanquake closed this on Jul 14, 2021
  52. MarcoFalke deleted the branch on Jul 14, 2021
  53. MarcoFalke commented at 11:15 AM on July 14, 2021: member

    Jup, it's the usual brokenness of GitHub.

  54. Sjors commented at 12:34 PM on July 14, 2021: member

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

  55. sidhujag referenced this in commit 9f1d429b17 on Jul 14, 2021
  56. gwillen referenced this in commit ce53627c25 on Jun 1, 2022
  57. bitcoin locked this on Aug 18, 2022
Contributors
MarcoFalkeDrahtBotluke-jrtheStackmichaelfolksoninstagibbsbrunoergSjorsNelsonGaldemanTheBlueMattfanquake
Labels
TestsNeeds rebase
Linked (view graph)
#19013 test: add v0.20.1, v0.21.0 and v22.0 to backwards compatibility test #21081 test: fix the unreachable code at feature_taproot#22442 util: improves error messages on get_previous_releases script
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: 2026-05-19 06:53 UTC