From b80d0bdee4603aa8ab69587d0c311aad1a9b3c7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 25 Apr 2025 15:56:07 +0200 Subject: [PATCH 1/4] test: avoid stack overflow in `FindChallenges` via manual iteration The original recursive `FindChallenges` explores the Miniscript node tree using depth-first search. Specifically, it performs a pre-order traversal (processing the node's data, then recursively visiting children from left-to-right). This recursion uses the call stack, which can lead to stack overflows on platforms with limited stack space, particularly noticeable in Windows debug builds. This change replaces the recursive implementation with an iterative version using an explicit stack. The iterative version also performs a depth-first search and processes the node's data before exploring children (preserving pre-order characteristics), although the children are explored in right-to-left order due to the LIFO nature of the explicit stack. Critically, both versions collect challenges into a `std::set`, which automatically deduplicates and sorts elements. This ensures that not only the final result, but the actual state of the set at any equivalent point in traversal remains identical, despite the difference in insertion order. This iterative approach is an alternative to increasing the default stack size (as proposed in #32349) and directly addresses the stack overflow issue reported in #32341 by avoiding deep recursion. The change is done in two commits: * add a new iterative `FindChallenges` method and rename the old method to `*_recursive` (to simplify removal in the next commit), asserting that its result matches the original; * Remove the original recursive implementation. This approach avoids needing to suppress `misc-no-recursion` warnings and provides a portable, low-risk fix. Using a `std::set` is necessary for deduplication, matching the original function's behavior. An experiment using an `std::vector` showed duplicate challenges being added, confirming the need for the set: Example failure with vector: Recursive (set): (6, 9070746) (6, 19532513) (6, 3343376967) Iterative (vector attempt): (6, 19532513) (6, 9070746) (6, 3343376967) (6, 9070746) // Duplicate Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> --- src/test/miniscript_tests.cpp | 40 ++++++++++++++++++++++++++++++++--- 1 file changed, 37 insertions(+), 3 deletions(-) diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index f253562a2f6..c2b0d43dccf 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -298,7 +298,7 @@ using Node = miniscript::Node; /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */ // NOLINTNEXTLINE(misc-no-recursion) -std::set FindChallenges(const NodeRef& ref) { +std::set FindChallenges_recursive(const NodeRef& ref) { std::set chal; for (const auto& key : ref->keys) { chal.emplace(ChallengeType::PK, ChallengeNumber(key)); @@ -317,12 +317,44 @@ std::set FindChallenges(const NodeRef& ref) { chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); } for (const auto& sub : ref->subs) { - auto sub_chal = FindChallenges(sub); + auto sub_chal = FindChallenges_recursive(sub); chal.insert(sub_chal.begin(), sub_chal.end()); } return chal; } +/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */ +std::set FindChallenges(const NodeRef& root) +{ + std::set chal; + + for (std::vector stack{root.get()}; !stack.empty();) { + const Node* ref{stack.back()}; + stack.pop_back(); + + for (const auto& key : ref->keys) { + chal.emplace(ChallengeType::PK, ChallengeNumber(key)); + } + if (ref->fragment == miniscript::Fragment::OLDER) { + chal.emplace(ChallengeType::OLDER, ref->k); + } else if (ref->fragment == miniscript::Fragment::AFTER) { + chal.emplace(ChallengeType::AFTER, ref->k); + } else if (ref->fragment == miniscript::Fragment::SHA256) { + chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data)); + } else if (ref->fragment == miniscript::Fragment::RIPEMD160) { + chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data)); + } else if (ref->fragment == miniscript::Fragment::HASH256) { + chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data)); + } else if (ref->fragment == miniscript::Fragment::HASH160) { + chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); + } + for (const auto& sub : ref->subs) { + stack.push_back(sub.get()); + } + } + return chal; +} + //! The spk for this script under the given context. If it's a Taproot output also record the spend data. CScript ScriptPubKey(miniscript::MiniscriptContext ctx, const CScript& script, TaprootBuilder& builder) { @@ -347,7 +379,9 @@ struct MiniScriptTest : BasicTestingSetup { /** Run random satisfaction tests. */ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) { auto script = node->ToScript(converter); - auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript. + const auto challenges_recursive{FindChallenges_recursive(node)}; + const auto challenges{FindChallenges(node)}; // Find all challenges in the generated miniscript. + BOOST_CHECK(challenges_recursive == challenges); std::vector challist(challenges.begin(), challenges.end()); for (int iter = 0; iter < 3; ++iter) { std::shuffle(challist.begin(), challist.end(), m_rng); From f670836112c01feb3cb71618192e9c0c2e55767f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Fri, 25 Apr 2025 15:57:49 +0200 Subject: [PATCH 2/4] test: remove old recursive `FindChallenges_recursive` implementation The performance of the test is the same as before, with the recursive method. --- src/test/miniscript_tests.cpp | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index c2b0d43dccf..4d08fdfca6f 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -296,33 +296,6 @@ using NodeRef = miniscript::NodeRef; using miniscript::operator""_mst; using Node = miniscript::Node; -/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */ -// NOLINTNEXTLINE(misc-no-recursion) -std::set FindChallenges_recursive(const NodeRef& ref) { - std::set chal; - for (const auto& key : ref->keys) { - chal.emplace(ChallengeType::PK, ChallengeNumber(key)); - } - if (ref->fragment == miniscript::Fragment::OLDER) { - chal.emplace(ChallengeType::OLDER, ref->k); - } else if (ref->fragment == miniscript::Fragment::AFTER) { - chal.emplace(ChallengeType::AFTER, ref->k); - } else if (ref->fragment == miniscript::Fragment::SHA256) { - chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data)); - } else if (ref->fragment == miniscript::Fragment::RIPEMD160) { - chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data)); - } else if (ref->fragment == miniscript::Fragment::HASH256) { - chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data)); - } else if (ref->fragment == miniscript::Fragment::HASH160) { - chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); - } - for (const auto& sub : ref->subs) { - auto sub_chal = FindChallenges_recursive(sub); - chal.insert(sub_chal.begin(), sub_chal.end()); - } - return chal; -} - /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */ std::set FindChallenges(const NodeRef& root) { @@ -379,9 +352,7 @@ struct MiniScriptTest : BasicTestingSetup { /** Run random satisfaction tests. */ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) { auto script = node->ToScript(converter); - const auto challenges_recursive{FindChallenges_recursive(node)}; const auto challenges{FindChallenges(node)}; // Find all challenges in the generated miniscript. - BOOST_CHECK(challenges_recursive == challenges); std::vector challist(challenges.begin(), challenges.end()); for (int iter = 0; iter < 3; ++iter) { std::shuffle(challist.begin(), challist.end(), m_rng); From e400ac53524d143467740e2f59698a7c94644c21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 28 Apr 2025 15:48:53 +0200 Subject: [PATCH 3/4] refactor: simplify repeated comparisons in `FindChallenges` This obviates that the LHS of the comparison is always the same --- src/test/miniscript_tests.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 4d08fdfca6f..a3d8d4ed5d7 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -308,18 +308,14 @@ std::set FindChallenges(const NodeRef& root) for (const auto& key : ref->keys) { chal.emplace(ChallengeType::PK, ChallengeNumber(key)); } - if (ref->fragment == miniscript::Fragment::OLDER) { - chal.emplace(ChallengeType::OLDER, ref->k); - } else if (ref->fragment == miniscript::Fragment::AFTER) { - chal.emplace(ChallengeType::AFTER, ref->k); - } else if (ref->fragment == miniscript::Fragment::SHA256) { - chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data)); - } else if (ref->fragment == miniscript::Fragment::RIPEMD160) { - chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data)); - } else if (ref->fragment == miniscript::Fragment::HASH256) { - chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data)); - } else if (ref->fragment == miniscript::Fragment::HASH160) { - chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); + switch (ref->fragment) { + case Fragment::OLDER: chal.emplace(ChallengeType::OLDER, ref->k); break; + case Fragment::AFTER: chal.emplace(ChallengeType::AFTER, ref->k); break; + case Fragment::SHA256: chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data)); break; + case Fragment::RIPEMD160: chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data)); break; + case Fragment::HASH256: chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data)); break; + case Fragment::HASH160: chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); break; + default: break; } for (const auto& sub : ref->subs) { stack.push_back(sub.get()); From 7e8ef959d0637ca5543ed33d3919937e0d053e70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C5=91rinc?= Date: Mon, 28 Apr 2025 16:08:51 +0200 Subject: [PATCH 4/4] refactor: Fix Sonar rule `cpp:S4998` - avoid unique_ptr const& as parameter Changed `FindChallenges()` parameter from `const std::unique_ptr>&` to const `Node*`. Sonar rule `cpp:S4998` - https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=32351-8c0e673c4ac31c1c04750756de749fb813b2c33f&id=aureleoules_bitcoin&open=AZZ2q88IvFhp-eMuMy96: > Replace this use of "unique_ptr" by a raw pointer or a reference (possibly const). > Function parameters should not be of type "std::unique_ptr const &" cpp:S4998 > Software qualities impacted: Maintainability --- src/test/miniscript_tests.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index a3d8d4ed5d7..0a32727f37e 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -297,12 +297,12 @@ using miniscript::operator""_mst; using Node = miniscript::Node; /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */ -std::set FindChallenges(const NodeRef& root) +std::set FindChallenges(const Node* root) { std::set chal; - for (std::vector stack{root.get()}; !stack.empty();) { - const Node* ref{stack.back()}; + for (std::vector stack{root}; !stack.empty();) { + const auto* ref{stack.back()}; stack.pop_back(); for (const auto& key : ref->keys) { @@ -348,7 +348,7 @@ struct MiniScriptTest : BasicTestingSetup { /** Run random satisfaction tests. */ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) { auto script = node->ToScript(converter); - const auto challenges{FindChallenges(node)}; // Find all challenges in the generated miniscript. + const auto challenges{FindChallenges(node.get())}; // Find all challenges in the generated miniscript. std::vector challist(challenges.begin(), challenges.end()); for (int iter = 0; iter < 3; ++iter) { std::shuffle(challist.begin(), challist.end(), m_rng);