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

Code Coverage & Benchmarks

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

Reviews

See the guideline for information on the review process.

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

Conflicts

No conflicts as of last run.

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.

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.

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

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.

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.

Rebased

Rebased

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

Unless I'm missing something, this is untrue?

Can you explain why this is insufficient for usage?

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

Unless I'm missing something, this is untrue?

bitcoin/test/functional/test_framework/script.py

Line 870 in 7d27af9

if len(script) == 3:

bitcoin/test/functional/feature_taproot.py

Line 1683 in 7d27af9

[("0", CScript([pubs[52], OP_CHECKSIG]), LEAF_VERSION_TAPSCRIPT), ("1", CScript([b"BIP341"]), VALID_LEAF_VERS[pubs[99][0] % 41])],

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.

Christewart@c85db52

Closing this for now.