From 7e9b71adb68f8bb33749643b04b88097522f345b Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Feb 2022 16:30:15 -0500 Subject: [PATCH 1/6] Merge the malleable and non-malleable satisfier --- bitcoin/script/miniscript.cpp | 29 +++++++------- bitcoin/script/miniscript.h | 52 ++++++++++++++----------- bitcoin/test/fuzz/miniscript_random.cpp | 40 +++++++++++-------- 3 files changed, 67 insertions(+), 54 deletions(-) diff --git a/bitcoin/script/miniscript.cpp b/bitcoin/script/miniscript.cpp index 31c5f05..5e3b127 100644 --- a/bitcoin/script/miniscript.cpp +++ b/bitcoin/script/miniscript.cpp @@ -323,25 +323,24 @@ InputStack operator+(InputStack a, InputStack b) { return a; } -InputStack Choose(InputStack a, InputStack b, bool nonmalleable) { +InputStack Choose(InputStack a, InputStack b) { // If only one (or neither) is valid, pick the other one. if (a.available == Availability::NO) return b; if (b.available == Availability::NO) return a; - // If both are valid, they must be distinct. - if (nonmalleable) { - // If both options are weak, any result is fine; it just needs the malleable marker. - if (!a.has_sig && !b.has_sig) return a.Malleable(); - // If one option is weak, we must pick that one. - if (!a.has_sig) return a; - if (!b.has_sig) return b; - // If both options are strong, prefer the canonical one. - if (b.non_canon) return a; - if (a.non_canon) return b; - // If both options are strong and canonical, prefer the nonmalleable one. - if (b.malleable) return a; - if (a.malleable) return b; + // If only one of the solutions has a signature, we must pick the other one. + if (!a.has_sig && b.has_sig) return a; + if (!b.has_sig && a.has_sig) return b; + if (!a.has_sig && !b.has_sig) { + // If neither solution requires a signature, the result is inevitably malleable. + a.malleable = true; + b.malleable = true; + } else { + // If both options require a signature, prefer the non-malleable one. + if (b.malleable && !a.malleable) return a; + if (a.malleable && !b.malleable) return b; } - // Pick the smaller between YESes and the bigger between MAYBEs. Prefer YES over MAYBE. + // Between two malleable or two non-malleable solutions, pick the smaller one between + // YESes, and the bigger ones between MAYBEs. Prefer YES over MAYBE. if (a.available == Availability::YES && b.available == Availability::YES) { return std::move(a.size <= b.size ? a : b); } else if (a.available == Availability::MAYBE && b.available == Availability::MAYBE) { diff --git a/bitcoin/script/miniscript.h b/bitcoin/script/miniscript.h index b2c9661..abad6ab 100644 --- a/bitcoin/script/miniscript.h +++ b/bitcoin/script/miniscript.h @@ -250,6 +250,7 @@ struct InputStack { //! Whether this stack is malleable (can be turned into an equally valid other stack by a third party). bool malleable = false; //! Whether this stack is non-canonical (using a construction known to be unnecessary for satisfaction). + //! Note that this flag does not affect the satisfaction algorithm; it is only used for sanity checking. bool non_canon = false; //! Serialized witness size. size_t size = 0; @@ -270,7 +271,7 @@ struct InputStack { //! Concatenate two input stacks. friend InputStack operator+(InputStack a, InputStack b); //! Choose between two potential input stacks. - friend InputStack Choose(InputStack a, InputStack b, bool nonmalleable); + friend InputStack Choose(InputStack a, InputStack b); }; static const auto ZERO = InputStack(std::vector()); @@ -796,10 +797,10 @@ struct Node { template - internal::InputResult ProduceInput(const Ctx& ctx, bool nonmal) const { + internal::InputResult ProduceInput(const Ctx& ctx) const { using namespace internal; - auto helper = [&ctx, nonmal](const Node& node, Span subres) -> InputResult { + auto helper = [&ctx](const Node& node, Span subres) -> InputResult { switch (node.nodetype) { case Fragment::PK_K: { std::vector sig; @@ -819,7 +820,7 @@ struct Node { auto sat = InputStack(std::move(sig)).WithSig().Available(avail); std::vector next_sats; next_sats.push_back(sats[0]); - for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j], std::move(sats[j - 1]) + sat, nonmal)); + for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j], std::move(sats[j - 1]) + sat)); next_sats.push_back(std::move(sats[sats.size() - 1]) + std::move(sat)); sats = std::move(next_sats); } @@ -834,13 +835,13 @@ struct Node { auto& res = subres[subres.size() - i - 1]; std::vector next_sats; next_sats.push_back(sats[0] + res.nsat); - for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j] + res.nsat, std::move(sats[j - 1]) + res.sat, nonmal)); + for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j] + res.nsat, std::move(sats[j - 1]) + res.sat)); next_sats.push_back(std::move(sats[sats.size() - 1]) + std::move(res.sat)); sats = std::move(next_sats); } InputStack nsat = INVALID; for (size_t i = 0; i < sats.size(); ++i) { - if (i != node.k) nsat = Choose(std::move(nsat), std::move(sats[i]), nonmal); + if (i != node.k) nsat = Choose(std::move(nsat), std::move(sats[i])); } assert(node.k <= sats.size()); return InputResult(std::move(nsat), std::move(sats[node.k])); @@ -877,28 +878,29 @@ struct Node { } case Fragment::AND_B: { auto& x = subres[0], &y = subres[1]; - return InputResult(Choose(Choose(y.nsat + x.nsat, (y.sat + x.nsat).NonCanon(), nonmal), (y.nsat + x.sat).NonCanon(), nonmal), y.sat + x.sat); + return InputResult(Choose(Choose(y.nsat + x.nsat, (y.sat + x.nsat).NonCanon()), (y.nsat + x.sat).NonCanon()), y.sat + x.sat); } case Fragment::OR_B: { auto& x = subres[0], &z = subres[1]; - return InputResult(z.nsat + x.nsat, Choose(Choose(z.nsat + x.sat, z.sat + x.nsat, nonmal), (z.sat + x.sat).NonCanon(), nonmal)); + // The (sat(Z) sat(X)) solution is overcomplete (attacker can change either into dsat). + return InputResult(z.nsat + x.nsat, Choose(Choose(z.nsat + x.sat, z.sat + x.nsat), (z.sat + x.sat).Malleable())); } case Fragment::OR_C: { auto& x = subres[0], &z = subres[1]; - return InputResult(INVALID, Choose(std::move(x.sat), z.sat + x.nsat, nonmal)); + return InputResult(INVALID, Choose(std::move(x.sat), z.sat + x.nsat)); } case Fragment::OR_D: { auto& x = subres[0], &z = subres[1]; auto nsat = z.nsat + x.nsat, sat_l = x.sat, sat_r = z.sat + x.nsat; - return InputResult(z.nsat + x.nsat, Choose(std::move(x.sat), z.sat + x.nsat, nonmal)); + return InputResult(z.nsat + x.nsat, Choose(std::move(x.sat), z.sat + x.nsat)); } case Fragment::OR_I: { auto& x = subres[0], &z = subres[1]; - return InputResult(Choose(x.nsat + ONE, z.nsat + ZERO, nonmal), Choose(x.sat + ONE, z.sat + ZERO, nonmal)); + return InputResult(Choose(x.nsat + ONE, z.nsat + ZERO), Choose(x.sat + ONE, z.sat + ZERO)); } case Fragment::ANDOR: { auto& x = subres[0], &y = subres[1], &z = subres[2]; - return InputResult(Choose((y.nsat + x.sat).NonCanon(), z.nsat + x.nsat, nonmal), Choose(y.sat + x.sat, z.sat + x.nsat, nonmal)); + return InputResult(Choose((y.nsat + x.sat).NonCanon(), z.nsat + x.nsat), Choose(y.sat + x.sat, z.sat + x.nsat)); } case Fragment::WRAP_A: case Fragment::WRAP_S: @@ -929,7 +931,7 @@ struct Node { return InputResult(INVALID, INVALID); }; - auto tester = [&helper, nonmal](const Node& node, Span subres) -> InputResult { + auto tester = [&helper](const Node& node, Span subres) -> InputResult { auto ret = helper(node, subres); // Do a consistency check between the satisfaction code and the type checker // (the actual satisfaction code in ProduceInputHelper does not use GetType) @@ -941,13 +943,11 @@ struct Node { if (node.GetType() << "d"_mst) assert(ret.nsat.available != Availability::NO); if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.has_sig); if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) assert(ret.sat.has_sig); - if (nonmal) { - if (node.GetType() << "d"_mst) assert(!ret.nsat.has_sig); - if (node.GetType() << "d"_mst && !ret.nsat.malleable) assert(!ret.nsat.non_canon); - if (node.GetType() << "e"_mst) assert(!ret.nsat.malleable); - if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.malleable); - if (ret.sat.available != Availability::NO && !ret.sat.malleable) assert(!ret.sat.non_canon); - } + if (node.GetType() << "d"_mst) assert(!ret.nsat.has_sig); + if (node.GetType() << "d"_mst && !ret.nsat.malleable) assert(!ret.nsat.non_canon); + if (node.GetType() << "e"_mst) assert(!ret.nsat.malleable); + if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.malleable); + if (ret.sat.available != Availability::NO && !ret.sat.malleable) assert(!ret.sat.non_canon); return ret; }; @@ -998,16 +998,22 @@ struct Node { //! Check whether there is no satisfaction path that contains both timelocks and heightlocks bool CheckTimeLocksMix() const { return GetType() << "k"_mst; } - //! Do all sanity checks. - bool IsSane() const { return IsValid() && IsNonMalleable() && CheckTimeLocksMix() && CheckOpsLimit() && CheckStackSize(); } + //! Whether successful non-malleable satisfactions are guaranteed to be valid. + bool ValidSatisfactions() const { return IsValid() && CheckOpsLimit() && CheckStackSize(); } + + //! Whether the apparent policy of this node matches its script semantics. + bool IsSane() const { return ValidSatisfactions() && IsNonMalleable() && CheckTimeLocksMix(); } //! Check whether this node is safe as a script on its own. bool IsSaneTopLevel() const { return IsValidTopLevel() && IsSane() && NeedsSignature(); } //! Produce a witness for this script, if possible and given the information available in the context. + //! The non-malleable satisfaction is guaranteed to be valid if it exists, and ValidSatisfaction() + //! is true. If IsSane() holds, this satisfaction is guaranteed to succeed in case the node's + //! conditions are satisfied (private keys and hash preimages available, locktimes satsified). template Availability Satisfy(const Ctx& ctx, std::vector>& stack, bool nonmalleable = true) const { - auto ret = ProduceInput(ctx, nonmalleable); + auto ret = ProduceInput(ctx); if (nonmalleable && (ret.sat.malleable || !ret.sat.has_sig)) return Availability::NO; stack = std::move(ret.sat.stack); return ret.sat.available; diff --git a/bitcoin/test/fuzz/miniscript_random.cpp b/bitcoin/test/fuzz/miniscript_random.cpp index 36db49d..251005e 100644 --- a/bitcoin/test/fuzz/miniscript_random.cpp +++ b/bitcoin/test/fuzz/miniscript_random.cpp @@ -390,23 +390,31 @@ FUZZ_TARGET_INIT(miniscript_random, initialize_miniscript_random) assert(parsed); assert(*parsed == *node); - // Check both malleable and non-malleable satisfaction. Note that we only assert the produced witness - // is valid if the Miniscript was sane, as otherwise it could overflow the limits. - CScriptWitness witness; + // Run malleable satisfaction algorithm. const CScript script_pubkey = CScript() << OP_0 << WitnessV0ScriptHash(script); - const bool mal_success = node->Satisfy(SATISFIER_CTX, witness.stack, false) == miniscript::Availability::YES; - if (mal_success && node->IsSaneTopLevel()) { - witness.stack.push_back(std::vector(script.begin(), script.end())); - assert(VerifyScript(DUMMY_SCRIPTSIG, script_pubkey, &witness, STANDARD_SCRIPT_VERIFY_FLAGS, CHECKER_CTX)); + CScriptWitness witness_mal; + const bool mal_success = node->Satisfy(SATISFIER_CTX, witness_mal.stack, false) == miniscript::Availability::YES; + witness_mal.stack.push_back(std::vector(script.begin(), script.end())); + + // Run non-malleable satisfaction algorithm. + CScriptWitness witness_nonmal; + const bool nonmal_success = node->Satisfy(SATISFIER_CTX, witness_nonmal.stack, true) == miniscript::Availability::YES; + witness_nonmal.stack.push_back(std::vector(script.begin(), script.end())); + + // If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it. + if (nonmal_success) { + assert(mal_success); + assert(witness_nonmal.stack == witness_mal.stack); } - witness.stack.clear(); - const bool nonmal_success = node->Satisfy(SATISFIER_CTX, witness.stack, true) == miniscript::Availability::YES; - if (nonmal_success && node->IsSaneTopLevel()) { - witness.stack.push_back(std::vector(script.begin(), script.end())); - assert(VerifyScript(DUMMY_SCRIPTSIG, script_pubkey, &witness, STANDARD_SCRIPT_VERIFY_FLAGS, CHECKER_CTX)); + + // Test satisfactions. + if (node->ValidSatisfactions() && nonmal_success) { + // Non-malleable satisfactions are guaranteed to be valid if ValidSatisfactions(). + assert(VerifyScript(DUMMY_SCRIPTSIG, script_pubkey, &witness_nonmal, STANDARD_SCRIPT_VERIFY_FLAGS, CHECKER_CTX)); + } + + if (node->IsSaneTopLevel()) { + // For sane nodes, the two algorithms behave identically. + assert(mal_success == nonmal_success); } - // If a nonmalleable solution exists, a solution whatsoever must also exist. - assert(mal_success >= nonmal_success); - // If a miniscript is nonmalleable and needs a signature, and a solution exists, a non-malleable solution must also exist. - if (node->IsNonMalleable() && node->NeedsSignature()) assert(nonmal_success == mal_success); } From 74edd566a0fe7e4bf2c86b060d2da752702ef793 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Feb 2022 16:32:19 -0500 Subject: [PATCH 2/6] Mark extra thresh() dissatisfactions as non-canonical --- bitcoin/script/miniscript.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bitcoin/script/miniscript.h b/bitcoin/script/miniscript.h index abad6ab..17c8161 100644 --- a/bitcoin/script/miniscript.h +++ b/bitcoin/script/miniscript.h @@ -841,6 +841,8 @@ struct Node { } InputStack nsat = INVALID; for (size_t i = 0; i < sats.size(); ++i) { + // i==k is the satisfaction; i==0 is the canonical dissatisfaction; the rest are non-canonical. + if (i != 0 && i != node.k) sats[i].NonCanon(); if (i != node.k) nsat = Choose(std::move(nsat), std::move(sats[i])); } assert(node.k <= sats.size()); From f7a276d10117408c3aded6897cc574ba34f3e625 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Feb 2022 16:35:25 -0500 Subject: [PATCH 3/6] Improve satisfaction assertions, and add comments --- bitcoin/script/miniscript.h | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/bitcoin/script/miniscript.h b/bitcoin/script/miniscript.h index 17c8161..a3461e2 100644 --- a/bitcoin/script/miniscript.h +++ b/bitcoin/script/miniscript.h @@ -935,21 +935,44 @@ struct Node { auto tester = [&helper](const Node& node, Span subres) -> InputResult { auto ret = helper(node, subres); + // Do a consistency check between the satisfaction code and the type checker // (the actual satisfaction code in ProduceInputHelper does not use GetType) + + // For 'z' nodes, available satisfactions/dissatisfactions must have stack size 0. if (node.GetType() << "z"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 0); if (node.GetType() << "z"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 0); + + // For 'o' nodes, available satisfactions/dissatisfactions must have stack size 1. if (node.GetType() << "o"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() == 1); if (node.GetType() << "o"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() == 1); - if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.back().size() != 0); + + // For 'n' nodes, available satisfactions/dissatisfactions must have stack size 1 or larger. For satisfactions, + // the top element cannot be 0. + if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(ret.sat.stack.size() >= 1); + if (node.GetType() << "n"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.stack.size() >= 1); + if (node.GetType() << "n"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.stack.back().empty()); + + // For 'd' nodes, a dissatisfaction must exist, and they must not need a signature. If it is non-malleable, + // it must be canonical. if (node.GetType() << "d"_mst) assert(ret.nsat.available != Availability::NO); - if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.has_sig); - if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) assert(ret.sat.has_sig); if (node.GetType() << "d"_mst) assert(!ret.nsat.has_sig); if (node.GetType() << "d"_mst && !ret.nsat.malleable) assert(!ret.nsat.non_canon); + + // For 'f'/'s' nodes, dissatisfactions/satisfactions must have a signature. + if (node.GetType() << "f"_mst && ret.nsat.available != Availability::NO) assert(ret.nsat.has_sig); + if (node.GetType() << "s"_mst && ret.sat.available != Availability::NO) assert(ret.sat.has_sig); + + // For 'e' nodes, a non-malleable dissatisfaction must exist. + if (node.GetType() << "e"_mst) assert(ret.nsat.available != Availability::NO); if (node.GetType() << "e"_mst) assert(!ret.nsat.malleable); + + // For 'm' nodes, if a satisfaction exists, it must be non-malleable. if (node.GetType() << "m"_mst && ret.sat.available != Availability::NO) assert(!ret.sat.malleable); + + // If a non-malleable satisfaction exists, it must be canonical. if (ret.sat.available != Availability::NO && !ret.sat.malleable) assert(!ret.sat.non_canon); + return ret; }; From c166591df7ae50de0f72a4fc7db1108fad4e9f93 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 18 Feb 2022 16:46:48 -0500 Subject: [PATCH 4/6] Rename Choose() -> operator| --- bitcoin/script/miniscript.cpp | 2 +- bitcoin/script/miniscript.h | 48 +++++++++++++++++------------------ 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/bitcoin/script/miniscript.cpp b/bitcoin/script/miniscript.cpp index 5e3b127..5dd09da 100644 --- a/bitcoin/script/miniscript.cpp +++ b/bitcoin/script/miniscript.cpp @@ -323,7 +323,7 @@ InputStack operator+(InputStack a, InputStack b) { return a; } -InputStack Choose(InputStack a, InputStack b) { +InputStack operator|(InputStack a, InputStack b) { // If only one (or neither) is valid, pick the other one. if (a.available == Availability::NO) return b; if (b.available == Availability::NO) return a; diff --git a/bitcoin/script/miniscript.h b/bitcoin/script/miniscript.h index a3461e2..41ddc2d 100644 --- a/bitcoin/script/miniscript.h +++ b/bitcoin/script/miniscript.h @@ -271,7 +271,7 @@ struct InputStack { //! Concatenate two input stacks. friend InputStack operator+(InputStack a, InputStack b); //! Choose between two potential input stacks. - friend InputStack Choose(InputStack a, InputStack b); + friend InputStack operator|(InputStack a, InputStack b); }; static const auto ZERO = InputStack(std::vector()); @@ -300,7 +300,7 @@ struct MaxInt { return a.value + b.value; } - friend MaxInt Choose(const MaxInt& a, const MaxInt& b) { + friend MaxInt operator|(const MaxInt& a, const MaxInt& b) { if (!a.valid) return b; if (!b.valid) return a; return std::max(a.value, b.value); @@ -691,30 +691,30 @@ struct Node { } case Fragment::OR_B: { const auto count{1 + subs[0]->ops.count + subs[1]->ops.count}; - const auto sat{Choose(subs[0]->ops.sat + subs[1]->ops.dsat, subs[1]->ops.sat + subs[0]->ops.dsat)}; + const auto sat{(subs[0]->ops.sat + subs[1]->ops.dsat) | (subs[1]->ops.sat + subs[0]->ops.dsat)}; const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat}; return {count, sat, dsat}; } case Fragment::OR_D: { const auto count{3 + subs[0]->ops.count + subs[1]->ops.count}; - const auto sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat + subs[0]->ops.dsat)}; + const auto sat{subs[0]->ops.sat | (subs[1]->ops.sat + subs[0]->ops.dsat)}; const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat}; return {count, sat, dsat}; } case Fragment::OR_C: { const auto count{2 + subs[0]->ops.count + subs[1]->ops.count}; - const auto sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat + subs[0]->ops.dsat)}; + const auto sat{subs[0]->ops.sat | (subs[1]->ops.sat + subs[0]->ops.dsat)}; return {count, sat, {}}; } case Fragment::OR_I: { const auto count{3 + subs[0]->ops.count + subs[1]->ops.count}; - const auto sat{Choose(subs[0]->ops.sat, subs[1]->ops.sat)}; - const auto dsat{Choose(subs[0]->ops.dsat, subs[1]->ops.dsat)}; + const auto sat{subs[0]->ops.sat | subs[1]->ops.sat}; + const auto dsat{subs[0]->ops.dsat | subs[1]->ops.dsat}; return {count, sat, dsat}; } case Fragment::ANDOR: { const auto count{3 + subs[0]->ops.count + subs[1]->ops.count + subs[2]->ops.count}; - const auto sat{Choose(subs[1]->ops.sat + subs[0]->ops.sat, subs[0]->ops.dsat + subs[2]->ops.sat)}; + const auto sat{(subs[1]->ops.sat + subs[0]->ops.sat) | (subs[0]->ops.dsat + subs[2]->ops.sat)}; const auto dsat{subs[0]->ops.dsat + subs[2]->ops.dsat}; return {count, sat, dsat}; } @@ -732,7 +732,7 @@ struct Node { for (const auto& sub : subs) { count += sub->ops.count + 1; auto next_sats = Vector(sats[0] + sub->ops.dsat); - for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j] + sub->ops.dsat, sats[j - 1] + sub->ops.sat)); + for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + sub->ops.dsat) | (sats[j - 1] + sub->ops.sat)); next_sats.push_back(sats[sats.size() - 1] + sub->ops.sat); sats = std::move(next_sats); } @@ -757,20 +757,20 @@ struct Node { case Fragment::HASH256: case Fragment::HASH160: return {1, {}}; case Fragment::ANDOR: { - const auto sat{Choose(subs[0]->ss.sat + subs[1]->ss.sat, subs[0]->ss.dsat + subs[2]->ss.sat)}; + const auto sat{(subs[0]->ss.sat + subs[1]->ss.sat) | (subs[0]->ss.dsat + subs[2]->ss.sat)}; const auto dsat{subs[0]->ss.dsat + subs[2]->ss.dsat}; return {sat, dsat}; } case Fragment::AND_V: return {subs[0]->ss.sat + subs[1]->ss.sat, {}}; case Fragment::AND_B: return {subs[0]->ss.sat + subs[1]->ss.sat, subs[0]->ss.dsat + subs[1]->ss.dsat}; case Fragment::OR_B: { - const auto sat{Choose(subs[0]->ss.dsat + subs[1]->ss.sat, subs[0]->ss.sat + subs[1]->ss.dsat)}; + const auto sat{(subs[0]->ss.dsat + subs[1]->ss.sat) | (subs[0]->ss.sat + subs[1]->ss.dsat)}; const auto dsat{subs[0]->ss.dsat + subs[1]->ss.dsat}; return {sat, dsat}; } - case Fragment::OR_C: return {Choose(subs[0]->ss.sat, subs[0]->ss.dsat + subs[1]->ss.sat), {}}; - case Fragment::OR_D: return {Choose(subs[0]->ss.sat, subs[0]->ss.dsat + subs[1]->ss.sat), subs[0]->ss.dsat + subs[1]->ss.dsat}; - case Fragment::OR_I: return {Choose(subs[0]->ss.sat + 1, subs[1]->ss.sat + 1), Choose(subs[0]->ss.dsat + 1, subs[1]->ss.dsat + 1)}; + case Fragment::OR_C: return {subs[0]->ss.sat | (subs[0]->ss.dsat + subs[1]->ss.sat), {}}; + case Fragment::OR_D: return {subs[0]->ss.sat | (subs[0]->ss.dsat + subs[1]->ss.sat), subs[0]->ss.dsat + subs[1]->ss.dsat}; + case Fragment::OR_I: return {(subs[0]->ss.sat + 1) | (subs[1]->ss.sat + 1), (subs[0]->ss.dsat + 1) | (subs[1]->ss.dsat + 1)}; case Fragment::MULTI: return {(uint32_t)keys.size() + 1, (uint32_t)keys.size() + 1}; case Fragment::WRAP_A: case Fragment::WRAP_N: @@ -783,7 +783,7 @@ struct Node { auto sats = Vector(internal::MaxInt(0)); for (const auto& sub : subs) { auto next_sats = Vector(sats[0] + sub->ss.dsat); - for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j] + sub->ss.dsat, sats[j - 1] + sub->ss.sat)); + for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + sub->ss.dsat) | (sats[j - 1] + sub->ss.sat)); next_sats.push_back(sats[sats.size() - 1] + sub->ss.sat); sats = std::move(next_sats); } @@ -820,7 +820,7 @@ struct Node { auto sat = InputStack(std::move(sig)).WithSig().Available(avail); std::vector next_sats; next_sats.push_back(sats[0]); - for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j], std::move(sats[j - 1]) + sat)); + for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(sats[j] | (std::move(sats[j - 1]) + sat)); next_sats.push_back(std::move(sats[sats.size() - 1]) + std::move(sat)); sats = std::move(next_sats); } @@ -835,7 +835,7 @@ struct Node { auto& res = subres[subres.size() - i - 1]; std::vector next_sats; next_sats.push_back(sats[0] + res.nsat); - for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back(Choose(sats[j] + res.nsat, std::move(sats[j - 1]) + res.sat)); + for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + res.nsat) | (std::move(sats[j - 1]) + res.sat)); next_sats.push_back(std::move(sats[sats.size() - 1]) + std::move(res.sat)); sats = std::move(next_sats); } @@ -843,7 +843,7 @@ struct Node { for (size_t i = 0; i < sats.size(); ++i) { // i==k is the satisfaction; i==0 is the canonical dissatisfaction; the rest are non-canonical. if (i != 0 && i != node.k) sats[i].NonCanon(); - if (i != node.k) nsat = Choose(std::move(nsat), std::move(sats[i])); + if (i != node.k) nsat = std::move(nsat) | std::move(sats[i]); } assert(node.k <= sats.size()); return InputResult(std::move(nsat), std::move(sats[node.k])); @@ -880,29 +880,29 @@ struct Node { } case Fragment::AND_B: { auto& x = subres[0], &y = subres[1]; - return InputResult(Choose(Choose(y.nsat + x.nsat, (y.sat + x.nsat).NonCanon()), (y.nsat + x.sat).NonCanon()), y.sat + x.sat); + return InputResult((y.nsat + x.nsat) | (y.sat + x.nsat).NonCanon() | (y.nsat + x.sat).NonCanon(), y.sat + x.sat); } case Fragment::OR_B: { auto& x = subres[0], &z = subres[1]; // The (sat(Z) sat(X)) solution is overcomplete (attacker can change either into dsat). - return InputResult(z.nsat + x.nsat, Choose(Choose(z.nsat + x.sat, z.sat + x.nsat), (z.sat + x.sat).Malleable())); + return InputResult(z.nsat + x.nsat, (z.nsat + x.sat) | (z.sat + x.nsat) | (z.sat + x.sat).Malleable()); } case Fragment::OR_C: { auto& x = subres[0], &z = subres[1]; - return InputResult(INVALID, Choose(std::move(x.sat), z.sat + x.nsat)); + return InputResult(INVALID, std::move(x.sat) | (z.sat + x.nsat)); } case Fragment::OR_D: { auto& x = subres[0], &z = subres[1]; auto nsat = z.nsat + x.nsat, sat_l = x.sat, sat_r = z.sat + x.nsat; - return InputResult(z.nsat + x.nsat, Choose(std::move(x.sat), z.sat + x.nsat)); + return InputResult(z.nsat + x.nsat, std::move(x.sat) | (z.sat + x.nsat)); } case Fragment::OR_I: { auto& x = subres[0], &z = subres[1]; - return InputResult(Choose(x.nsat + ONE, z.nsat + ZERO), Choose(x.sat + ONE, z.sat + ZERO)); + return InputResult((x.nsat + ONE) | (z.nsat + ZERO), (x.sat + ONE) | (z.sat + ZERO)); } case Fragment::ANDOR: { auto& x = subres[0], &y = subres[1], &z = subres[2]; - return InputResult(Choose((y.nsat + x.sat).NonCanon(), z.nsat + x.nsat), Choose(y.sat + x.sat, z.sat + x.nsat)); + return InputResult((y.nsat + x.sat).NonCanon() | (z.nsat + x.nsat), (y.sat + x.sat) | (z.sat + x.nsat)); } case Fragment::WRAP_A: case Fragment::WRAP_S: From e1643a95fe3eba7837e58bdfdd9f7b4bc107997c Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 21 Feb 2022 16:06:29 -0500 Subject: [PATCH 5/6] Stronger fuzz test --- bitcoin/test/fuzz/miniscript_random.cpp | 26 ++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/bitcoin/test/fuzz/miniscript_random.cpp b/bitcoin/test/fuzz/miniscript_random.cpp index 251005e..0e6dfbe 100644 --- a/bitcoin/test/fuzz/miniscript_random.cpp +++ b/bitcoin/test/fuzz/miniscript_random.cpp @@ -401,16 +401,32 @@ FUZZ_TARGET_INIT(miniscript_random, initialize_miniscript_random) const bool nonmal_success = node->Satisfy(SATISFIER_CTX, witness_nonmal.stack, true) == miniscript::Availability::YES; witness_nonmal.stack.push_back(std::vector(script.begin(), script.end())); - // If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it. if (nonmal_success) { + // Non-malleable satisfactions are bounded by GetStackSize(). + assert(witness_nonmal.stack.size() <= node->GetStackSize()); + // If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it. assert(mal_success); assert(witness_nonmal.stack == witness_mal.stack); - } - // Test satisfactions. - if (node->ValidSatisfactions() && nonmal_success) { + // Test non-malleable satisfaction. + ScriptError serror; + bool res = VerifyScript(DUMMY_SCRIPTSIG, script_pubkey, &witness_nonmal, STANDARD_SCRIPT_VERIFY_FLAGS, CHECKER_CTX, &serror); // Non-malleable satisfactions are guaranteed to be valid if ValidSatisfactions(). - assert(VerifyScript(DUMMY_SCRIPTSIG, script_pubkey, &witness_nonmal, STANDARD_SCRIPT_VERIFY_FLAGS, CHECKER_CTX)); + if (node->ValidSatisfactions()) assert(res); + // More detailed: non-malleable satisfactions must be valid, or could fail with ops count error (if CheckOpsLimit failed), + // or with a stack size error (if CheckStackSize check failed). + assert(res || + (!node->CheckOpsLimit() && serror == ScriptError::SCRIPT_ERR_OP_COUNT) || + (!node->CheckStackSize() && serror == ScriptError::SCRIPT_ERR_STACK_SIZE)); + } + + if (mal_success && (!nonmal_success || witness_mal.stack != witness_nonmal.stack)) { + // Test malleable satisfaction only if it's different from the non-malleable one. + ScriptError serror; + bool res = VerifyScript(DUMMY_SCRIPTSIG, script_pubkey, &witness_mal, STANDARD_SCRIPT_VERIFY_FLAGS, CHECKER_CTX, &serror); + // Malleable satisfactions are not guaranteed to be valid under any conditions, but they can only + // fail due to stack or ops limits. + assert(res || serror == ScriptError::SCRIPT_ERR_OP_COUNT || serror == ScriptError::SCRIPT_ERR_STACK_SIZE); } if (node->IsSaneTopLevel()) { From 6fbd6e4f76d9f51f26a47abb17dacc51424122d3 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Mon, 21 Feb 2022 16:54:16 -0500 Subject: [PATCH 6/6] Stronger unit test --- bitcoin/test/miniscript_tests.cpp | 87 +++++++++++++++++++------------ 1 file changed, 54 insertions(+), 33 deletions(-) diff --git a/bitcoin/test/miniscript_tests.cpp b/bitcoin/test/miniscript_tests.cpp index 896c48d..c29a236 100644 --- a/bitcoin/test/miniscript_tests.cpp +++ b/bitcoin/test/miniscript_tests.cpp @@ -287,26 +287,6 @@ std::set FindChallenges(const NodeRef& ref) { return chal; } -/** Verify a satisfaction. */ -void Verify(const std::string& testcase, const NodeRef& node, const Satisfier& ctx, std::vector> stack, const CScript& script, bool nonmal) { - // Construct P2WSH scriptPubKey. - CScript spk = GetScriptForDestination(WitnessV0ScriptHash(script)); - // Construct the P2WSH witness (script stack + script). - CScriptWitness witness; - witness.stack = std::move(stack); - witness.stack.push_back(std::vector(script.begin(), script.end())); - // Use a test signature checker aware of which afters/olders we made valid. - TestSignatureChecker checker(&ctx); - ScriptError serror; - if (nonmal) BOOST_CHECK(stack.size() <= node->GetStackSize()); - if (!VerifyScript(CScript(), spk, &witness, STANDARD_SCRIPT_VERIFY_FLAGS, checker, &serror)) { - // The only possible violation should be the ops limit, as that is hard to statically guarantee. - BOOST_CHECK(serror == SCRIPT_ERR_OP_COUNT); - // When using the non-malleable satisfier, and our OpsLimit analysis succeeds, no execution errors should occur at all. - BOOST_CHECK(!(nonmal && node->CheckOpsLimit())); - } -} - /** Run random satisfaction tests. */ void TestSatisfy(const std::string& testcase, const NodeRef& node) { auto script = node->ToScript(CONVERTER); @@ -315,31 +295,72 @@ void TestSatisfy(const std::string& testcase, const NodeRef& node) { for (int iter = 0; iter < 3; ++iter) { Shuffle(challist.begin(), challist.end(), g_insecure_rand_ctx); Satisfier satisfier; + TestSignatureChecker checker(&satisfier); bool prev_mal_success = false, prev_nonmal_success = false; // Go over all challenges involved in this miniscript in random order. for (int add = -1; add < (int)challist.size(); ++add) { if (add >= 0) satisfier.supported.insert(challist[add]); // The first iteration does not add anything - // First try to produce a potentially malleable satisfaction. - std::vector> stack; - bool mal_success = node->Satisfy(satisfier, stack, false) == miniscript::Availability::YES; - if (mal_success) Verify(testcase, node, satisfier, stack, script, false); // And verify it against consensus/standardness. - // Then produce a non-malleable satisfaction. - bool nonmal_success = node->Satisfy(satisfier, stack, true) == miniscript::Availability::YES; - if (nonmal_success) Verify(testcase, node, satisfier, std::move(stack), std::move(script), true); - // If a nonmalleable solution exists, a solution whatsoever must also exist. - BOOST_CHECK(mal_success >= nonmal_success); - // If a miniscript is nonmalleable and needs a signature, and a solution exists, a non-malleable solution must also exist. - if (node->IsNonMalleable() && node->NeedsSignature()) BOOST_CHECK_EQUAL(nonmal_success, mal_success); + + // Run malleable satisfaction algorithm. + const CScript script_pubkey = CScript() << OP_0 << WitnessV0ScriptHash(script); + CScriptWitness witness_mal; + const bool mal_success = node->Satisfy(satisfier, witness_mal.stack, false) == miniscript::Availability::YES; + witness_mal.stack.push_back(std::vector(script.begin(), script.end())); + + // Run non-malleable satisfaction algorithm. + CScriptWitness witness_nonmal; + const bool nonmal_success = node->Satisfy(satisfier, witness_nonmal.stack, true) == miniscript::Availability::YES; + witness_nonmal.stack.push_back(std::vector(script.begin(), script.end())); + + if (nonmal_success) { + // Non-malleable satisfactions are bounded by GetStackSize(). + BOOST_CHECK(witness_nonmal.stack.size() <= node->GetStackSize()); + // If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it. + BOOST_CHECK(mal_success); + BOOST_CHECK(witness_nonmal.stack == witness_mal.stack); + + // Test non-malleable satisfaction. + ScriptError serror; + bool res = VerifyScript(CScript(), script_pubkey, &witness_nonmal, STANDARD_SCRIPT_VERIFY_FLAGS, checker, &serror); + // Non-malleable satisfactions are guaranteed to be valid if ValidSatisfactions(). + if (node->ValidSatisfactions()) BOOST_CHECK(res); + // More detailed: non-malleable satisfactions must be valid, or could fail with ops count error (if CheckOpsLimit failed), + // or with a stack size error (if CheckStackSize check fails). + BOOST_CHECK(res || + (!node->CheckOpsLimit() && serror == ScriptError::SCRIPT_ERR_OP_COUNT) || + (!node->CheckStackSize() && serror == ScriptError::SCRIPT_ERR_STACK_SIZE)); + } + + if (mal_success && (!nonmal_success || witness_mal.stack != witness_nonmal.stack)) { + // Test malleable satisfaction only if it's different from the non-malleable one. + ScriptError serror; + bool res = VerifyScript(CScript(), script_pubkey, &witness_mal, STANDARD_SCRIPT_VERIFY_FLAGS, checker, &serror); + // Malleable satisfactions are not guaranteed to be valid under any conditions, but they can only + // fail due to stack or ops limits. + BOOST_CHECK(res || serror == ScriptError::SCRIPT_ERR_OP_COUNT || serror == ScriptError::SCRIPT_ERR_STACK_SIZE); + } + + if (node->IsSaneTopLevel()) { + // For sane nodes, the two algorithms behave identically. + BOOST_CHECK_EQUAL(mal_success, nonmal_success); + } + // Adding more satisfied conditions can never remove our ability to produce a satisfaction. BOOST_CHECK(mal_success >= prev_mal_success); - // For nonmalleable solutions this is only true if the added condition is PK; for other conditions, adding one may make an valid satisfaction become malleable. - if (add >= 0 && challist[add].first == ChallengeType::PK) BOOST_CHECK(nonmal_success >= prev_nonmal_success); + // For nonmalleable solutions this is only true if the added condition is PK; + // for other conditions, adding one may make an valid satisfaction become malleable. If the script + // is sane, this cannot happen however. + if (node->IsSaneTopLevel() || add < 0 || challist[add].first == ChallengeType::PK) { + BOOST_CHECK(nonmal_success >= prev_nonmal_success); + } // Remember results for the next added challenge. prev_mal_success = mal_success; prev_nonmal_success = nonmal_success; } // If the miniscript was satisfiable at all, a satisfaction must be found after all conditions are added. BOOST_CHECK_EQUAL(prev_mal_success, Satisfiable(node)); + // If the miniscript is sane and satisfiable, a nonmalleable satisfaction must eventually be found. + if (node->IsSaneTopLevel()) BOOST_CHECK_EQUAL(prev_nonmal_success, Satisfiable(node)); } }