test: Add `leaf_version` parameter to `taproot_tree_helper()` #29371

pull Christewart wants to merge 1 commits into bitcoin:master from Christewart:2024-02-01-scriptpy-leafver changing 5 files +36 −34
  1. Christewart commented at 4:46 PM on February 2, 2024: contributor

    This PR adds a leaf_version parameter to taproot_tree_helper(). Previously the leaf version was hard coded, because we only currently support 1 leaf version.

    Proposed soft forks such as #29221 require new leaf versions to be allocated. This PR allows the test framework to accomodate new leaf versions. This commit is also included in #29221, but in the policy of trying to avoid mega-PRs, I carved this out into a separate PR that can be merged.

    This PR does not alter any test functionality, just adds a parameter and uses it across the codebase rather than hard coding the value inside of taproot_tree_helper().

  2. DrahtBot commented at 4:46 PM on February 2, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29371.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK theStack, achow101, scgbckbone
    Stale ACK edilmedeiros, itornaza

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  3. DrahtBot added the label Tests on Feb 2, 2024
  4. Christewart force-pushed on Feb 2, 2024
  5. Christewart marked this as ready for review on Feb 2, 2024
  6. edilmedeiros commented at 6:19 PM on February 5, 2024: contributor

    Tested ACK fe53febee0288809184ef6f49131fa342ae86f7b

    I've run test/functional/feature_taproot.py and test/functional/wallet_taproot.py.

    The changes make sense in preparation for future expansions, as said. They don't alter current test logic.

  7. in test/functional/test_framework/script.py:868 in fe53febee0 outdated 
     853 |          assert not callable(script)
 854 |          if isinstance(script, list):
 855 | -            return taproot_tree_helper(script)
 856 | +            return taproot_tree_helper(script, leaf_version)
 857 |          assert isinstance(script, tuple)
 858 | -        version = LEAF_VERSION_TAPSCRIPT
    Christewart commented at 7:44 PM on February 5, 2024:

    here is where the previous hard coded value was that the parameter is replacing.

    edilmedeiros commented at 8:28 PM on February 5, 2024:

    I see that you don't change the hardcoded value 0xc0, just pass it with function calls. It does make sense to generalize taproot_tree_helper and taproot_construct.

    edilmedeiros commented at 8:31 PM on February 5, 2024:

    I'm not familiar with taproot, but do you think it does make sense to add a test to check if someone is passing the correct version constants to the functions (maybe in a new PR, since this one is more like refactoring code)?

    Christewart commented at 9:38 PM on February 5, 2024:

    I think you probably need to understand Taproot at least a bit to understand why we aren't changing the hard coded constant 0xc0 for now. I would recommend taking a look at #29221 to understand what a new using a new leaf version looks like.

    https://github.com/bitcoin/bitcoin/pull/29221/files#diff-0d2e316fe5f2581b35d1703cfebba67e58b06d12edcea7262eb98f41e50410efR64

  8. edilmedeiros approved
  9. in test/functional/test_framework/script.py:856 in fe53febee0 outdated 
     854 |          if isinstance(script, list):
 855 | -            return taproot_tree_helper(script)
 856 | +            return taproot_tree_helper(script, leaf_version)
 857 |          assert isinstance(script, tuple)
 858 | -        version = LEAF_VERSION_TAPSCRIPT
 859 | +        version = leaf_version
    scgbckbone commented at 10:00 AM on February 6, 2024:

    this line should be removed and only leaf_version function argument should be used in the function. Both version and leaf_version will point to the same object anyways. Same with all possible future leaf versions even 0xc0..0xfe

    Christewart commented at 3:08 PM on February 6, 2024:

    Done in 25dc427e6c755b4c0d68599966caf95ada3565f7 , thank you for the review.

  10. scgbckbone approved
  11. scgbckbone commented at 10:01 AM on February 6, 2024: none

    utACK

  12. Christewart force-pushed on Feb 6, 2024
  13. Christewart commented at 3:53 PM on February 6, 2024: contributor

    I believe this CI failure is spurious: https://github.com/bitcoin/bitcoin/pull/29371/checks?check_run_id=21277712706

    Could someone restart the job ( :pray: ) or I can push an empty commit to restart everything.

  14. DrahtBot added the label CI failed on Feb 6, 2024
  15. DrahtBot removed the label CI failed on Feb 6, 2024
  16. Christewart force-pushed on Feb 22, 2024
  17. DrahtBot added the label CI failed on Feb 22, 2024
  18. Christewart force-pushed on Mar 6, 2024
  19. DrahtBot removed the label CI failed on Mar 6, 2024
  20. Christewart force-pushed on Mar 9, 2024
  21. theStack commented at 4:34 PM on March 12, 2024: contributor

    Concept ACK

    In order to avoid changes in functional tests and keep the patch small and simple, it probably would makes sense to set LEAF_VERSION_TAPSCRIPT as default value for parameter leaf_version for taproot_construct()?

  22. Christewart commented at 7:26 PM on March 12, 2024: contributor

    In order to avoid changes in functional tests and keep the patch small and simple, it probably would makes sense to set LEAF_VERSION_TAPSCRIPT as default value for parameter leaf_version for taproot_construct()?

    This is more philsophical, but I really prefer not to provide default parameters to critical test code like this. It is SO easy to just forget to pass a parameter and have your test cases not test all possible branches of code! In this case, when dealing with consensus critical test cases, I think it should be required that the developer of the unit test pass in the tapscript version they are intending to test rather than us just assuming they want to test LEAF_VERSION_TAPSCRIPT.

    Please see #29221 for examples of this. For instance if I forgot to pass LEAF_VERSION_TAPSCRIPT_64BIT in a specific spot I believe my tests would trivially pass because it is a soft fork!

    Reasonable minds can differ of course, for future readers of the PR leave a comment or give me a :-1: if you disagree and would like to see the default parameter used in this case. If it is clear that I'm in the minority, I will change it.

  23. itornaza commented at 5:29 PM on April 10, 2024: contributor

    tested ACK 1ccd751f5d5a5d6b970e7a6a3179e0403c6b9c90

    • Did a code review and everything lgtm
    • Built the PR with --with-incompatible-bdb and --enable-surpress-external-warnings
    • Run unit tests with make check and alll tests pass
    • Run functional tests with no extra flags and all tests pass
    • Run extended functional tests with --extended all tests pass

    Reasonable minds can differ of course, for future readers of the PR leave a comment or give me a 👎 if you disagree and would like to see the default parameter used in this case. If it is clear that I'm in the minority, I will change it.

    Who am I to say, but I philosophically agree with your reasoning on not passing default parameters in critical test code.

  24. DrahtBot requested review from edilmedeiros on Apr 10, 2024
  25. DrahtBot requested review from scgbckbone on Apr 10, 2024
  26. DrahtBot requested review from theStack on Apr 10, 2024
  27. scgbckbone commented at 1:01 PM on April 27, 2024: none

    re-ACK (I'm pro default function parameter)

  28. DrahtBot added the label Needs rebase on May 13, 2024
  29. Christewart force-pushed on May 13, 2024
  30. DrahtBot removed the label Needs rebase on May 13, 2024
  31. DrahtBot added the label CI failed on May 14, 2024
  32. Christewart force-pushed on May 21, 2024
  33. Christewart force-pushed on May 21, 2024
  34. DrahtBot removed the label CI failed on May 21, 2024
  35. itornaza commented at 4:49 PM on May 27, 2024: contributor

    trACK 3d4dda0b26d3e003d282be1e30e64055750a792e

  36. DrahtBot requested review from scgbckbone on May 27, 2024
  37. Christewart force-pushed on Jun 3, 2024
  38. DrahtBot added the label Needs rebase on Jul 26, 2024
  39. Christewart force-pushed on Aug 23, 2024
  40. DrahtBot removed the label Needs rebase on Aug 23, 2024
  41. achow101 removed review request from scgbckbone on Oct 15, 2024
  42. achow101 removed review request from edilmedeiros on Oct 15, 2024
  43. achow101 requested review from achow101 on Oct 15, 2024
  44. DrahtBot added the label CI failed on Oct 20, 2024
  45. DrahtBot removed the label CI failed on Oct 25, 2024
  46. Christewart force-pushed on Oct 29, 2024
  47. in test/functional/test_framework/script.py:929 in 6a0406bed8 outdated 
     925 | @@ -927,7 +926,7 @@ def taproot_construct(pubkey, scripts=None, treat_internal_as_infinity=False):
 926 |      if scripts is None:
 927 |          scripts = []
 928 |  
 929 | -    ret, h = taproot_tree_helper(scripts)
 930 | +    ret, h = taproot_tree_helper(scripts,leaf_version)
    achow101 commented at 10:40 PM on November 4, 2024:

    nit:

    add a space after the comma.

  48. achow101 commented at 10:40 PM on November 4, 2024: member

    Concept ACK

    Please cleanup your commit message, it contains message fragments from squashed commits.

  49. Christewart force-pushed on Nov 5, 2024
  50. Christewart commented at 2:29 PM on November 5, 2024: contributor

    Concept ACK

    Please cleanup your commit message, it contains message fragments from squashed commits.

    Done in 6ffdabd

  51. itornaza approved
  52. itornaza commented at 5:40 PM on November 18, 2024: contributor

    re ACK 6ffdabd4a00bf5578f975d0ed60c27adabf36c0b

  53. DrahtBot requested review from achow101 on Nov 18, 2024
  54. DrahtBot requested review from edilmedeiros on Nov 18, 2024
  55. DrahtBot requested review from scgbckbone on Nov 18, 2024
  56. scgbckbone approved
  57. scgbckbone commented at 11:54 AM on November 27, 2024: none

    ACK

  58. Christewart force-pushed on Feb 8, 2025
  59. Christewart commented at 2:52 PM on February 8, 2025: contributor

    Rebased

  60. itornaza commented at 9:45 AM on February 25, 2025: contributor

    rtACK fca0367d9a6f8a4296d41cc9b344c94b93ef136b

  61. DrahtBot requested review from scgbckbone on Feb 25, 2025
  62. Christewart force-pushed on Mar 17, 2025
  63. Christewart force-pushed on Apr 15, 2025
  64. DrahtBot added the label Needs rebase on Apr 15, 2025
  65. Christewart force-pushed on Apr 15, 2025
  66. Christewart commented at 10:10 PM on April 15, 2025: contributor

    Rebased

  67. DrahtBot added the label CI failed on Apr 15, 2025
  68. Christewart force-pushed on Apr 15, 2025
  69. DrahtBot removed the label Needs rebase on Apr 16, 2025
  70. DrahtBot removed the label CI failed on Apr 16, 2025
  71. DrahtBot added the label Needs rebase on May 6, 2025
  72. test: Add leaf_version parameter to taproot_tree_helper() 3b8b612962
  73. Christewart force-pushed on May 9, 2025
  74. DrahtBot removed the label Needs rebase on May 9, 2025
  75. scgbckbone commented at 1:41 PM on June 27, 2025: none

    ACK

  76. achow101 removed review request from edilmedeiros on Oct 22, 2025
  77. achow101 requested review from ajtowns on Oct 22, 2025
  78. achow101 requested review from instagibbs on Oct 22, 2025
  79. instagibbs commented at 3:28 PM on October 22, 2025: member

    Previously the leaf version was hard coded, because we only currently support 1 leaf version.

    Unless I'm missing something, this is untrue? https://github.com/bitcoin/bitcoin/blob/7d27af98c7cf858b5ab5a02e64f89a857cc53172/test/functional/test_framework/script.py#L870

    https://github.com/bitcoin/bitcoin/blob/7d27af98c7cf858b5ab5a02e64f89a857cc53172/test/functional/feature_taproot.py#L1683

    Can you explain why this is insufficient for usage?

  80. Christewart commented at 8:15 PM on October 26, 2025: contributor

    Previously the leaf version was hard coded, because we only currently support 1 leaf version.

    Unless I'm missing something, this is untrue?

    https://github.com/bitcoin/bitcoin/blob/7d27af98c7cf858b5ab5a02e64f89a857cc53172/test/functional/test_framework/script.py#L870

    https://github.com/bitcoin/bitcoin/blob/7d27af98c7cf858b5ab5a02e64f89a857cc53172/test/functional/feature_taproot.py#L1683

    Can you explain why this is insufficient for usage?

    Hi @instagibbs

    I missed this and you are correct. I modified the code base to remove the default version in taproot_tree_helper(), here is what the change looks like through the codebase. This shouldn't be necessary until there is another leaf version being used on the network.

    https://github.com/Christewart/bitcoin/commit/c85db522c6c5bab2c76ac7e4867be20ec19e7f3c

    Closing this for now.

  81. Christewart closed this on Oct 26, 2025
Contributors
ChristewartDrahtBotedilmedeirosscgbckbonetheStackitornazaachow101instagibbs


theStackajtownsachow101instagibbs

Labels
Tests
Linked (view graph)
#29221 Implement 64 bit arithmetic op codes in the Script interpreter
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-04-14 21:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me