test: Add leaf_version parameter to taproot_tree_helper()

[Christewart]

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

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack, 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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29939 (test: add MiniWallet tagging support to avoid UTXO mixing, use in fill_mempool by theStack)

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.

[edilmedeiros]

Tested ACK fe53feb

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.

[Christewart]

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

[edilmedeiros]

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]

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]

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

[scgbckbone]

utACK

[Christewart]

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

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

[theStack]

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()?

[Christewart]

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

[itornaza]

tested ACK 1ccd751

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

[scgbckbone]

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