refactor: Do not use CScript for tapleaf scripts until the tapleaf version is known #25877

While BIP-341 calls the contents of tapleaf a "script", only in the case that the tapleaf version is 0xc0 is this script known to be a tapscript. Otherwise the tapleaf "script" is simply an uninterpreted string of bytes.

This PR corrects the issue where the type CScript is used prior to the tapleaf version being known to be a tapscript. This prevents CScript methods from erroneously being called on non-tapscript data.

A second commit abstracts out the TapBranch hash computation in the same manner that the TapLeaf computation is already abstracted. These two abstractions ensure that the TapLeaf and TapBranch tagged hashes are always constructed properly.

• Does this stylistic change have any performance impact?
• I wonder if Span<std::byte> should be used instead.
• See also (weakly related) https://github.com/bitcoin/bitcoin/pull/13062

1. I don't have any benchmarks, but I would expect the increased use of Spans ought to increase performance if anything.
2. I'm happy to move to std::byte if the process is to use std::byte in PRs going forward. Just let me know.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

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.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

CI is failing:

script/sign.cpp:193:34: error: temporary whose address is used as value of local variable 'match' will be destroyed at the end of the full-expression [-Werror,-Wdangling]
    if (auto match = MatchMultiA(CScript(script.begin(), script.end()))) {

@ajtowns I've now addressed the issue.

Concept ACK.

Don't see much need to switch to std::byte here. Would be nice to do this together with #13062 though.

Code review ACK 729aa3ce0879cebc75f3a416ad7c6a0ed3634f0c Verified in BIP-341 that a tapleaf is a script if its version is 0xc0.

Added @aureleoules's compute_tapbranch unit test.

Rebased.

Rebased above the fix for CI.

re-crACK 95b0442410672c3f28b43d33e49f264d4c59df62 - rebased, minor changes and added a unit test LGTM CI failure is unrelated (#26330)

rebased

ACK dee89438b82e94474ebaa31367035f98b4636dac

As I've noticed the newly introduced code contains some redundant constructors, this may be useful to add (not a blocker):

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 5815a059ae..6bbe0967d2 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1643,7 +1643,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
                 for (const auto& [depth, script, leaf_ver] : *tree) {
                     std::unique_ptr<DescriptorImpl> subdesc;
                     if (leaf_ver == TAPROOT_LEAF_TAPSCRIPT) {
-                        subdesc = InferScript(CScript(script.begin(), script.end()), ParseScriptContext::P2TR, provider);
+                        subdesc = InferScript({script.begin(), script.end()}, ParseScriptContext::P2TR, provider);
                     }
                     if (!subdesc) {
                         ok = false;
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index c06d0370c9..d63c3cde88 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1933,7 +1933,7 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion,
             execdata.m_tapleaf_hash_init = true;
             if ((control[0] & TAPROOT_LEAF_MASK) == TAPROOT_LEAF_TAPSCRIPT) {
                 // Tapscript (leaf version 0xc0)
-                exec_script = CScript(script.begin(), script.end());
+                exec_script = {script.begin(), script.end()};
                 execdata.m_validation_weight_left = ::GetSerializeSize(witness.stack, PROTOCOL_VERSION) + VALIDATION_WEIGHT_OFFSET;
                 execdata.m_validation_weight_left_init = true;
                 return ExecuteWitnessScript(stack, exec_script, flags, SigVersion::TAPSCRIPT, checker, execdata, serror);
diff --git a/src/script/sign.cpp b/src/script/sign.cpp
index 53377560e8..18b6c09236 100644
--- a/src/script/sign.cpp
+++ b/src/script/sign.cpp
@@ -176,7 +176,7 @@ static bool SignTaprootScript(const SigningProvider& provider, const BaseSignatu
     SigVersion sigversion = SigVersion::TAPSCRIPT;
 
     uint256 leaf_hash = ComputeTapleafHash(leaf_version, script_bytes);
-    CScript script = CScript(script_bytes.begin(), script_bytes.end());
+    CScript script = {script_bytes.begin(), script_bytes.end()};
 
     // <xonly pubkey> OP_CHECKSIG
     if (script.size() == 34 && script[33] == OP_CHECKSIG && script[0] == 0x20) {
diff --git a/src/script/standard.cpp b/src/script/standard.cpp
index 2b7c120eaf..ad7ccca7df 100644
--- a/src/script/standard.cpp
+++ b/src/script/standard.cpp
@@ -445,7 +445,7 @@ TaprootBuilder& TaprootBuilder::Add(int depth, Span<const unsigned char> script,
     /* Construct NodeInfo object with leaf hash and (if track is true) also leaf information. */
     NodeInfo node;
     node.hash = ComputeTapleafHash(leaf_version, script);
-    if (track) node.leaves.emplace_back(LeafInfo{std::vector<unsigned char>(script.begin(), script.end()), leaf_version, {}});
+    if (track) node.leaves.emplace_back(LeafInfo{{script.begin(), script.end()}, leaf_version, {}});
     /* Insert into the branch. */
     Insert(std::move(node), depth);
     return *this;

reACK dee89438b82e94474ebaa31367035f98b4636dac - I verified that there is no behavior change.

cc @ajtowns @instagibbs

looking good

ACK dee89438b82e94474ebaa31367035f98b4636dac

ACK dee89438b82e94474ebaa31367035f98b4636dac

ACK dee89438b82e94474ebaa31367035f98b4636dac