From c3f204889483446e0e90552b8f3c64c18bc5c468 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sun, 30 Jan 2022 17:43:12 +0100 Subject: [PATCH 1/3] miniscript: don't re-introduce the usage of CScript's operator+ As noted by MarcoFalke on the Bitcoin Core repo, this may be potentially dangerous. Use a generalistic named function for constructing CScripts instead. Co-Authored-By: Pieter Wuille --- bitcoin/script/miniscript.h | 30 ++++++++++++++--------------- bitcoin/script/script.h | 38 +++++++++++++++++++++++-------------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/bitcoin/script/miniscript.h b/bitcoin/script/miniscript.h index 726178a..e522f8a 100644 --- a/bitcoin/script/miniscript.h +++ b/bitcoin/script/miniscript.h @@ -520,22 +520,22 @@ struct Node { case NodeType::RIPEMD160: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_RIPEMD160 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); case NodeType::HASH256: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_HASH256 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); case NodeType::HASH160: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_HASH160 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); - case NodeType::WRAP_A: return (CScript() << OP_TOALTSTACK) + std::move(subs[0]) + (CScript() << OP_FROMALTSTACK); - case NodeType::WRAP_S: return (CScript() << OP_SWAP) + std::move(subs[0]); - case NodeType::WRAP_C: return std::move(subs[0]) + CScript() << (verify ? OP_CHECKSIGVERIFY : OP_CHECKSIG); - case NodeType::WRAP_D: return (CScript() << OP_DUP << OP_IF) + std::move(subs[0]) + (CScript() << OP_ENDIF); - case NodeType::WRAP_V: return std::move(subs[0]) + (node.subs[0]->GetType() << "x"_mst ? (CScript() << OP_VERIFY) : CScript()); - case NodeType::WRAP_J: return (CScript() << OP_SIZE << OP_0NOTEQUAL << OP_IF) + std::move(subs[0]) + (CScript() << OP_ENDIF); - case NodeType::WRAP_N: return std::move(subs[0]) + CScript() << OP_0NOTEQUAL; + case NodeType::WRAP_A: return BuildScript(OP_TOALTSTACK, subs[0], OP_FROMALTSTACK); + case NodeType::WRAP_S: return BuildScript(OP_SWAP, subs[0]); + case NodeType::WRAP_C: return BuildScript(std::move(subs[0]), verify ? OP_CHECKSIGVERIFY : OP_CHECKSIG); + case NodeType::WRAP_D: return BuildScript(OP_DUP, OP_IF, subs[0], OP_ENDIF); + case NodeType::WRAP_V: return BuildScript(std::move(subs[0]), node.subs[0]->GetType() << "x"_mst ? CScript(OP_VERIFY) : CScript()); + case NodeType::WRAP_J: return BuildScript(OP_SIZE, OP_0NOTEQUAL, OP_IF, subs[0], OP_ENDIF); + case NodeType::WRAP_N: return BuildScript(std::move(subs[0]), OP_0NOTEQUAL); case NodeType::JUST_1: return CScript() << OP_1; case NodeType::JUST_0: return CScript() << OP_0; - case NodeType::AND_V: return std::move(subs[0]) + std::move(subs[1]); - case NodeType::AND_B: return std::move(subs[0]) + std::move(subs[1]) + (CScript() << OP_BOOLAND); - case NodeType::OR_B: return std::move(subs[0]) + std::move(subs[1]) + (CScript() << OP_BOOLOR); - case NodeType::OR_D: return std::move(subs[0]) + (CScript() << OP_IFDUP << OP_NOTIF) + std::move(subs[1]) + (CScript() << OP_ENDIF); - case NodeType::OR_C: return std::move(subs[0]) + (CScript() << OP_NOTIF) + std::move(subs[1]) + (CScript() << OP_ENDIF); - case NodeType::OR_I: return (CScript() << OP_IF) + std::move(subs[0]) + (CScript() << OP_ELSE) + std::move(subs[1]) + (CScript() << OP_ENDIF); - case NodeType::ANDOR: return std::move(subs[0]) + (CScript() << OP_NOTIF) + std::move(subs[2]) + (CScript() << OP_ELSE) + std::move(subs[1]) + (CScript() << OP_ENDIF); + case NodeType::AND_V: return BuildScript(std::move(subs[0]), subs[1]); + case NodeType::AND_B: return BuildScript(std::move(subs[0]), subs[1], OP_BOOLAND); + case NodeType::OR_B: return BuildScript(std::move(subs[0]), subs[1], OP_BOOLOR); + case NodeType::OR_D: return BuildScript(std::move(subs[0]), OP_IFDUP, OP_NOTIF, subs[1], OP_ENDIF); + case NodeType::OR_C: return BuildScript(std::move(subs[0]), OP_NOTIF, subs[1], OP_ENDIF); + case NodeType::OR_I: return BuildScript(OP_IF, subs[0], OP_ELSE, subs[1], OP_ENDIF); + case NodeType::ANDOR: return BuildScript(std::move(subs[0]), OP_NOTIF, subs[2], OP_ELSE, subs[1], OP_ENDIF); case NodeType::MULTI: { CScript script = CScript() << node.k; for (const auto& key : node.keys) { @@ -546,7 +546,7 @@ struct Node { case NodeType::THRESH: { CScript script = std::move(subs[0]); for (size_t i = 1; i < subs.size(); ++i) { - script = (std::move(script) + std::move(subs[i])) << OP_ADD; + script = BuildScript(std::move(script), subs[i], OP_ADD); } return std::move(script) << node.k << (verify ? OP_EQUALVERIFY : OP_EQUAL); } diff --git a/bitcoin/script/script.h b/bitcoin/script/script.h index 642161c..10dab74 100644 --- a/bitcoin/script/script.h +++ b/bitcoin/script/script.h @@ -421,20 +421,6 @@ class CScript : public CScriptBase READWRITEAS(CScriptBase, *this); } - CScript& operator+=(const CScript& b) - { - reserve(size() + b.size()); - insert(end(), b.begin(), b.end()); - return *this; - } - - friend CScript operator+(const CScript& a, const CScript& b) - { - CScript ret = a; - ret += b; - return ret; - } - CScript(int64_t b) { operator<<(b); } explicit CScript(opcodetype b) { operator<<(b); } @@ -587,4 +573,28 @@ struct CScriptWitness bool CheckMinimalPush(const std::vector& data, opcodetype opcode); +/** Build a script by concatenating other scripts, or any argument accepted by CScript::operator<<. */ +template +CScript BuildScript(Ts&&... inputs) +{ + CScript ret; + size_t cnt{0}; // Try to optimize over repeatedly calling ret.empty() + + ([&] (Ts&& input) { + if constexpr (std::is_same_v>, CScript>) { + // If it is a CScript, extend ret with it. Move or copy the first element instead. + if (cnt++ == 0) { + ret = std::forward(input); + } else { + ret.insert(ret.end(), input.begin(), input.end()); + } + } else { + // Otherwise invoke CScript::operator<<. + ret << input; + } + } (std::forward(inputs)), ...); + + return ret; +} + #endif // BITCOIN_SCRIPT_SCRIPT_H From 3992f6c18a72c7465a6586c2838b20e8d475c849 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 31 Jan 2022 16:42:20 +0100 Subject: [PATCH 2/3] Replace all usage of CScript() by BuildScript() --- bitcoin/script/miniscript.cpp | 6 +++--- bitcoin/script/miniscript.h | 36 ++++++++++++++++++++--------------- bitcoin/script/script.h | 9 +++++---- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/bitcoin/script/miniscript.cpp b/bitcoin/script/miniscript.cpp index 23acb75..de01f86 100644 --- a/bitcoin/script/miniscript.cpp +++ b/bitcoin/script/miniscript.cpp @@ -253,8 +253,8 @@ size_t ComputeScriptLen(NodeType nodetype, Type sub0typ, size_t subsize, uint32_ switch (nodetype) { case NodeType::PK_K: return subsize + 34; case NodeType::PK_H: return subsize + 3 + 21; - case NodeType::OLDER: return subsize + 1 + (CScript() << k).size(); - case NodeType::AFTER: return subsize + 1 + (CScript() << k).size(); + case NodeType::OLDER: return subsize + 1 + BuildScript(k).size(); + case NodeType::AFTER: return subsize + 1 + BuildScript(k).size(); case NodeType::HASH256: return subsize + 4 + 2 + 33; case NodeType::HASH160: return subsize + 4 + 2 + 21; case NodeType::SHA256: return subsize + 4 + 2 + 33; @@ -275,7 +275,7 @@ size_t ComputeScriptLen(NodeType nodetype, Type sub0typ, size_t subsize, uint32_ case NodeType::OR_C: return subsize + 2; case NodeType::OR_I: return subsize + 3; case NodeType::ANDOR: return subsize + 3; - case NodeType::THRESH: return subsize + n_subs + CScript(k).size(); + case NodeType::THRESH: return subsize + n_subs + BuildScript(k).size(); case NodeType::MULTI: return subsize + 3 + (n_keys > 16) + (k > 16) + 34 * n_keys; } assert(false); diff --git a/bitcoin/script/miniscript.h b/bitcoin/script/miniscript.h index e522f8a..7d4aa00 100644 --- a/bitcoin/script/miniscript.h +++ b/bitcoin/script/miniscript.h @@ -512,23 +512,29 @@ struct Node { // and the CScripts of its child nodes, the CScript of the node. auto upfn = [&ctx](bool verify, const Node& node, Span subs) -> CScript { switch (node.nodetype) { - case NodeType::PK_K: return CScript() << ctx.ToPKBytes(node.keys[0]); - case NodeType::PK_H: return CScript() << OP_DUP << OP_HASH160 << ctx.ToPKHBytes(node.keys[0]) << OP_EQUALVERIFY; - case NodeType::OLDER: return CScript() << node.k << OP_CHECKSEQUENCEVERIFY; - case NodeType::AFTER: return CScript() << node.k << OP_CHECKLOCKTIMEVERIFY; - case NodeType::SHA256: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_SHA256 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); - case NodeType::RIPEMD160: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_RIPEMD160 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); - case NodeType::HASH256: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_HASH256 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); - case NodeType::HASH160: return CScript() << OP_SIZE << 32 << OP_EQUALVERIFY << OP_HASH160 << node.data << (verify ? OP_EQUALVERIFY : OP_EQUAL); + case NodeType::PK_K: return BuildScript(ctx.ToPKBytes(node.keys[0])); + case NodeType::PK_H: return BuildScript(OP_DUP, OP_HASH160, ctx.ToPKHBytes(node.keys[0]), OP_EQUALVERIFY); + case NodeType::OLDER: return BuildScript(node.k, OP_CHECKSEQUENCEVERIFY); + case NodeType::AFTER: return BuildScript(node.k, OP_CHECKLOCKTIMEVERIFY); + case NodeType::SHA256: return BuildScript(OP_SIZE, 32, OP_EQUALVERIFY, OP_SHA256, node.data, verify ? OP_EQUALVERIFY : OP_EQUAL); + case NodeType::RIPEMD160: return BuildScript(OP_SIZE, 32, OP_EQUALVERIFY, OP_RIPEMD160, node.data, verify ? OP_EQUALVERIFY : OP_EQUAL); + case NodeType::HASH256: return BuildScript(OP_SIZE, 32, OP_EQUALVERIFY, OP_HASH256, node.data, verify ? OP_EQUALVERIFY : OP_EQUAL); + case NodeType::HASH160: return BuildScript(OP_SIZE, 32, OP_EQUALVERIFY, OP_HASH160, node.data, verify ? OP_EQUALVERIFY : OP_EQUAL); case NodeType::WRAP_A: return BuildScript(OP_TOALTSTACK, subs[0], OP_FROMALTSTACK); case NodeType::WRAP_S: return BuildScript(OP_SWAP, subs[0]); case NodeType::WRAP_C: return BuildScript(std::move(subs[0]), verify ? OP_CHECKSIGVERIFY : OP_CHECKSIG); case NodeType::WRAP_D: return BuildScript(OP_DUP, OP_IF, subs[0], OP_ENDIF); - case NodeType::WRAP_V: return BuildScript(std::move(subs[0]), node.subs[0]->GetType() << "x"_mst ? CScript(OP_VERIFY) : CScript()); + case NodeType::WRAP_V: { + if (node.subs[0]->GetType() << "x"_mst) { + return BuildScript(std::move(subs[0]), OP_VERIFY); + } else { + return std::move(subs[0]); + } + } case NodeType::WRAP_J: return BuildScript(OP_SIZE, OP_0NOTEQUAL, OP_IF, subs[0], OP_ENDIF); case NodeType::WRAP_N: return BuildScript(std::move(subs[0]), OP_0NOTEQUAL); - case NodeType::JUST_1: return CScript() << OP_1; - case NodeType::JUST_0: return CScript() << OP_0; + case NodeType::JUST_1: return BuildScript(OP_1); + case NodeType::JUST_0: return BuildScript(OP_0); case NodeType::AND_V: return BuildScript(std::move(subs[0]), subs[1]); case NodeType::AND_B: return BuildScript(std::move(subs[0]), subs[1], OP_BOOLAND); case NodeType::OR_B: return BuildScript(std::move(subs[0]), subs[1], OP_BOOLOR); @@ -537,18 +543,18 @@ struct Node { case NodeType::OR_I: return BuildScript(OP_IF, subs[0], OP_ELSE, subs[1], OP_ENDIF); case NodeType::ANDOR: return BuildScript(std::move(subs[0]), OP_NOTIF, subs[2], OP_ELSE, subs[1], OP_ENDIF); case NodeType::MULTI: { - CScript script = CScript() << node.k; + CScript script = BuildScript(node.k); for (const auto& key : node.keys) { - script << ctx.ToPKBytes(key); + script = BuildScript(std::move(script), ctx.ToPKBytes(key)); } - return std::move(script) << node.keys.size() << (verify ? OP_CHECKMULTISIGVERIFY : OP_CHECKMULTISIG); + return BuildScript(std::move(script), node.keys.size(), verify ? OP_CHECKMULTISIGVERIFY : OP_CHECKMULTISIG); } case NodeType::THRESH: { CScript script = std::move(subs[0]); for (size_t i = 1; i < subs.size(); ++i) { script = BuildScript(std::move(script), subs[i], OP_ADD); } - return std::move(script) << node.k << (verify ? OP_EQUALVERIFY : OP_EQUAL); + return BuildScript(std::move(script), node.k, verify ? OP_EQUALVERIFY : OP_EQUAL); } } assert(false); diff --git a/bitcoin/script/script.h b/bitcoin/script/script.h index 10dab74..d6c4d87 100644 --- a/bitcoin/script/script.h +++ b/bitcoin/script/script.h @@ -578,12 +578,13 @@ template CScript BuildScript(Ts&&... inputs) { CScript ret; - size_t cnt{0}; // Try to optimize over repeatedly calling ret.empty() + size_t cnt{0}; - ([&] (Ts&& input) { + ([&ret, &cnt] (Ts&& input) { + cnt++; if constexpr (std::is_same_v>, CScript>) { - // If it is a CScript, extend ret with it. Move or copy the first element instead. - if (cnt++ == 0) { + // If it is a CScript, extend ret with it. If ret is empty, move or copy it instead. + if (cnt == 0) { ret = std::forward(input); } else { ret.insert(ret.end(), input.begin(), input.end()); From c4f5df07f284e458ed5dd967199144acc19b2e28 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Sun, 30 Jan 2022 15:37:21 +0100 Subject: [PATCH 3/3] qa: replace the random unit tests with a miniscript_random fuzz target --- bitcoin/test/fuzz/miniscript_random.cpp | 422 ++++++++++++++++++++++++ bitcoin/test/miniscript_tests.cpp | 161 --------- 2 files changed, 422 insertions(+), 161 deletions(-) create mode 100644 bitcoin/test/fuzz/miniscript_random.cpp diff --git a/bitcoin/test/fuzz/miniscript_random.cpp b/bitcoin/test/fuzz/miniscript_random.cpp new file mode 100644 index 0000000..21a7a15 --- /dev/null +++ b/bitcoin/test/fuzz/miniscript_random.cpp @@ -0,0 +1,422 @@ +// Copyright (c) 2021 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include