aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorlaanwj <126646+laanwj@users.noreply.github.com>2022-05-26 14:14:03 +0200
committerlaanwj <126646+laanwj@users.noreply.github.com>2022-05-26 14:34:04 +0200
commitcacbdbaa95317b45cf2100702bca92410fb43b9a (patch)
treef9dcd1ff21b26a455980cd11021cdc9e7f97cc1f
parent192d639a6b1bd0feaa52e6ea4e63e33982704c32 (diff)
parentbd7c5e2f0a1b572decdf3a4bafe4ee990e1f4953 (diff)
downloadbitcoin-cacbdbaa95317b45cf2100702bca92410fb43b9a.tar.xz
Merge bitcoin/bitcoin#25132: consensus: Add BIP-341 specified constraints in `ComputeTaprootMerkleRoot`
bd7c5e2f0a1b572decdf3a4bafe4ee990e1f4953 Add BIP-341 specified constraints to `ComputeTaprootMerkleRoot` (David Bakin) Pull request description: [**N.B.:** This PR **_does not change the consensus_**. It only adds `assert` statements according to the current consensus in consensus-sensitive code (`interpreter.cpp`). So that's why the bot added the "consensus" tag and I prefixed the PR title with "consensus".] BIP 341 specifies [constraints on the size of the control block _c_ used to compute the taproot merkle root](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#script-validation-rules). > The last stack element is called the control block _c_, and must have length _33 + 32m_, for a value of _m_ that is an integer between 0 and 128, inclusive. Fail if it does not have such a length. The actual merkle root is computed in `ComputeTaprootMerkleRoot` ([interpreter.cpp@1833](https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1833)) - this code does _not_ check these constraints. All the callers do check the constraints before calling `ComputeTaprootMerkleRoot`. But in the future there may be more callers, and these checks may be inadvertently omitted at those future calls. Also, code at/near the current call sites may also change and skip these checks. Therefore _this PR adds those checks as `asserts` directly in `ComputeTaprootMerkleRoot`_ to help prevent that error. No unit tests provided: they'd have to be death tests as these are `assert` statements which raise `SIGABRT` and kill the program. Boost Test has a way to implement death tests (see the in-progress draft PR #25097 at [this code (you may have to click to expand the diff)](https://github.com/bitcoin/bitcoin/pull/25097/files#diff-21483d0e032747850208f21325b29cde89e9c1f55f83a7a166a388cc5c27115aR1089) and could be added here if desired by reviewers. Current callers of `ComputeTaprootMerkleRoot`: - `InferTaprootTree` ([standard.cpp@1552](https://github.com/bitcoin/bitcoin/blob/master/src/script/standard.cpp#L546)) - `VerifyTaprootCommittment` ([interpreter.cpp@1859](https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1859)) does a partial check, but it is called from `VerifyWitnessProgram` ([interpreter.cpp@1922](https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1918)) where a full check is done ACKs for top commit: sipa: ACK bd7c5e2f0a1b572decdf3a4bafe4ee990e1f4953 theStack: ACK bd7c5e2f0a1b572decdf3a4bafe4ee990e1f4953 Tree-SHA512: cec5aebfdf9fc9d13011abdf8427c934edf1df1ffc32b3e7cc5580f12809f24cf83e64ab0c843fabfce3591873bfcfa248277b30820fd786684319a196e4e550
-rw-r--r--src/script/interpreter.cpp4
1 files changed, 4 insertions, 0 deletions
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index c4d13d7283..9f56301377 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1832,6 +1832,10 @@ uint256 ComputeTapleafHash(uint8_t leaf_version, const CScript& script)
uint256 ComputeTaprootMerkleRoot(Span<const unsigned char> control, const uint256& tapleaf_hash)
{
+ assert(control.size() >= TAPROOT_CONTROL_BASE_SIZE);
+ assert(control.size() <= TAPROOT_CONTROL_MAX_SIZE);
+ assert((control.size() - TAPROOT_CONTROL_BASE_SIZE) % TAPROOT_CONTROL_NODE_SIZE == 0);
+
const int path_len = (control.size() - TAPROOT_CONTROL_BASE_SIZE) / TAPROOT_CONTROL_NODE_SIZE;
uint256 k = tapleaf_hash;
for (int i = 0; i < path_len; ++i) {