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

roconnor-blockstream · 2022-08-19T16:42:12Z

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.

maflcko · 2022-08-19T16:56:23Z

Does this stylistic change have any performance impact?
I wonder if Span<std::byte> should be used instead.
See also (weakly related) Make script interpreter independent from storage type CScript #13062

src/script/standard.cpp

roconnor-blockstream · 2022-08-19T17:16:53Z

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

DrahtBot · 2022-08-20T06:30:50Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	sipa, aureleoules, ajtowns, instagibbs, achow101

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.

ajtowns · 2022-10-03T19:21:45Z

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

roconnor-blockstream · 2022-10-04T16:46:56Z

@ajtowns I've now addressed the issue.

ajtowns

Concept ACK.

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

ajtowns · 2022-10-05T01:02:22Z

src/psbt.h

@@ -206,7 +206,7 @@ struct PSBTInput
    // Taproot fields
    std::vector<unsigned char> m_tap_key_sig;
    std::map<std::pair<XOnlyPubKey, uint256>, std::vector<unsigned char>> m_tap_script_sigs;
-    std::map<std::pair<CScript, int>, std::set<std::vector<unsigned char>, ShortestVectorFirstComparator>> m_tap_scripts;


Perhaps promote valtype into a header file and use that instead of std::vector<unsigned char> everywhere?

My understanding is that valtype is the type of values that occur on the stacks within Bitcoin Script's execution environment, that its programs manipulate. However, the use of std::vector<unsigned char> in this PR is the type of unparsed binary blob that is either a CScript or some yet unknown other language in some non-tapscript tagged tapleaf. (I admit the line between these two data types can get a bit blurry especially with P2SH).

I've left this as is for now, however, a couple of type annotations have been removed after following @aureleoules's suggestion, which is nice. If we do want to give it a typedef, we will need a new name.

ajtowns · 2022-10-05T01:07:44Z

src/script/interpreter.cpp

+uint256 ComputeTapbranchHash(Span<const unsigned char> a, Span<const unsigned char> b)
+{
+    assert(a.size() == 32);
+    assert(b.size() == 32);


These assertions don't seem necessary? Wrong sizes won't cause a crash, they just mean you're probably misusing the api, but there's a million ways to misuse the api.

Presumably we want to stop if we are misusing the API somewhere, rather than proceeding with erroneous computations? In particular these assertions were a cheap way of helping ensure that the invariant that ComputeTapbranchHash only ever uses its tagged hash on tapbranch data. Normally I'd use uint256 arguments here (and perhaps I should), but that would involve extra copying in some cases.

Anyhow, I don't feel strongly, so I've removed the assertions.

aureleoules

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

src/rpc/rawtransaction.cpp

roconnor-blockstream · 2022-10-12T20:47:51Z

Added @aureleoules's compute_tapbranch unit test.

roconnor-blockstream · 2022-10-13T16:03:42Z

Rebased.

roconnor-blockstream · 2022-10-17T16:25:26Z

Rebased above the fix for CI.

aureleoules

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

…nown Prevents use of CScript methods until the tapleaf is known to be a tapscript.

roconnor-blockstream · 2022-11-21T19:00:03Z

rebased

sipa

ACK dee8943

sipa · 2022-11-22T21:31:20Z

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;

aureleoules

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

fanquake · 2023-01-18T15:50:10Z

cc @ajtowns @instagibbs

instagibbs

looking good

src/test/script_tests.cpp

ajtowns · 2023-01-19T19:47:44Z

ACK dee8943

instagibbs · 2023-01-19T20:05:51Z

ACK dee8943

achow101 · 2023-01-19T22:42:06Z

ACK dee8943

f34ada8 Add unit test for ComputeTapleafHash (Greg Sanders) Pull request description: Quick follow-up to #25877 ACKs for top commit: sipa: ACK f34ada8 Tree-SHA512: ebec658c9b33859874a3e5d13ca0a00a2484233f00f2da09c7d3fb47ed7f56fc6d476ddd0473fe1396a514dffd6ea6a200f26c6dbca45bac2473e729ffef04c2

… until the tapleaf version is known dee8943 Abstract out ComputeTapbranchHash (Russell O'Connor) 8e3fc99 Do not use CScript for tapleaf scripts until the tapleaf version is known (Russell O'Connor) Pull request description: 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. ACKs for top commit: ajtowns: ACK dee8943 instagibbs: ACK dee8943 achow101: ACK dee8943 sipa: ACK dee8943 aureleoules: reACK dee8943 - I verified that there is no behavior change. Tree-SHA512: 4a1d37f3e9a1890e7f5eadcf65562688cc451389581fe6e2da0feb2368708edacdd95392578d8afff05270d88fc61dce732d83d1063d84d12cf47b5f4633ec7e

f34ada8 Add unit test for ComputeTapleafHash (Greg Sanders) Pull request description: Quick follow-up to bitcoin#25877 ACKs for top commit: sipa: ACK f34ada8 Tree-SHA512: ebec658c9b33859874a3e5d13ca0a00a2484233f00f2da09c7d3fb47ed7f56fc6d476ddd0473fe1396a514dffd6ea6a200f26c6dbca45bac2473e729ffef04c2

roconnor-blockstream commented Aug 19, 2022

View reviewed changes

src/script/standard.cpp Outdated Show resolved Hide resolved

This was referenced Aug 20, 2022

psbt: Only include PSBT_OUT_TAP_TREE when the output has a script path #25858

Merged

[BugFix] Make PSBT serializations 174 compliant by excluding empty taptree from serialization #25856

Closed

maflcko changed the title ~~Do not use CScript for tapleaf scripts until the tapleaf version is known~~ refactor: Do not use CScript for tapleaf scripts until the tapleaf version is known Aug 20, 2022

DrahtBot added the Refactoring label Aug 20, 2022

roconnor-blockstream force-pushed the 202208_tapleaf_script branch 2 times, most recently from f138c3b to 729aa3c Compare October 4, 2022 15:27

ajtowns reviewed Oct 5, 2022

View reviewed changes

aureleoules reviewed Oct 5, 2022

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

roconnor-blockstream force-pushed the 202208_tapleaf_script branch 2 times, most recently from 88d888b to ce3c85c Compare October 12, 2022 20:47

DrahtBot added the Needs rebase label Oct 13, 2022

roconnor-blockstream force-pushed the 202208_tapleaf_script branch from ce3c85c to f92f888 Compare October 13, 2022 16:03

DrahtBot removed the Needs rebase label Oct 13, 2022

roconnor-blockstream force-pushed the 202208_tapleaf_script branch from f92f888 to 95b0442 Compare October 17, 2022 16:24

aureleoules approved these changes Oct 18, 2022

View reviewed changes

roconnor-blockstream added 2 commits November 21, 2022 12:38

Do not use CScript for tapleaf scripts until the tapleaf version is k…

8e3fc99

…nown Prevents use of CScript methods until the tapleaf is known to be a tapscript.

Abstract out ComputeTapbranchHash

dee8943

roconnor-blockstream force-pushed the 202208_tapleaf_script branch from 95b0442 to dee8943 Compare November 21, 2022 18:59

fanquake requested review from ajtowns and instagibbs November 22, 2022 17:07

fanquake requested a review from sipa November 22, 2022 17:07

sipa approved these changes Nov 22, 2022

View reviewed changes

aureleoules approved these changes Jan 18, 2023

View reviewed changes

instagibbs reviewed Jan 18, 2023

View reviewed changes

src/test/script_tests.cpp Show resolved Hide resolved

ajtowns approved these changes Jan 19, 2023

View reviewed changes

achow101 merged commit 58da161 into bitcoin:master Jan 19, 2023

fanquake mentioned this pull request Jan 20, 2023

Follow ups to #25877 #26928

Closed

instagibbs mentioned this pull request Jan 20, 2023

test: Add unit test for ComputeTapleafHash #26934

Merged

bitcoin locked and limited conversation to collaborators Jan 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

roconnor-blockstream commented Aug 19, 2022

maflcko commented Aug 19, 2022

roconnor-blockstream commented Aug 19, 2022

DrahtBot commented Aug 20, 2022 •

edited

ajtowns commented Oct 3, 2022

roconnor-blockstream commented Oct 4, 2022

ajtowns left a comment

ajtowns Oct 5, 2022

roconnor-blockstream Oct 11, 2022

ajtowns Oct 5, 2022

roconnor-blockstream Oct 11, 2022

aureleoules left a comment

roconnor-blockstream commented Oct 12, 2022

roconnor-blockstream commented Oct 13, 2022

roconnor-blockstream commented Oct 17, 2022

aureleoules left a comment

roconnor-blockstream commented Nov 21, 2022

sipa left a comment

sipa commented Nov 22, 2022 •

edited

aureleoules left a comment

fanquake commented Jan 18, 2023

instagibbs left a comment

ajtowns commented Jan 19, 2023

instagibbs commented Jan 19, 2023

achow101 commented Jan 19, 2023

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

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

Conversation

roconnor-blockstream commented Aug 19, 2022

maflcko commented Aug 19, 2022

roconnor-blockstream commented Aug 19, 2022

DrahtBot commented Aug 20, 2022 • edited

Reviews

Conflicts

ajtowns commented Oct 3, 2022

roconnor-blockstream commented Oct 4, 2022

ajtowns left a comment

Choose a reason for hiding this comment

ajtowns Oct 5, 2022

Choose a reason for hiding this comment

roconnor-blockstream Oct 11, 2022

Choose a reason for hiding this comment

ajtowns Oct 5, 2022

Choose a reason for hiding this comment

roconnor-blockstream Oct 11, 2022

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

roconnor-blockstream commented Oct 12, 2022

roconnor-blockstream commented Oct 13, 2022

roconnor-blockstream commented Oct 17, 2022

aureleoules left a comment

Choose a reason for hiding this comment

roconnor-blockstream commented Nov 21, 2022

sipa left a comment

Choose a reason for hiding this comment

sipa commented Nov 22, 2022 • edited

aureleoules left a comment

Choose a reason for hiding this comment

fanquake commented Jan 18, 2023

instagibbs left a comment

Choose a reason for hiding this comment

ajtowns commented Jan 19, 2023

instagibbs commented Jan 19, 2023

achow101 commented Jan 19, 2023

DrahtBot commented Aug 20, 2022 •

edited

sipa commented Nov 22, 2022 •

edited