Merge 7e8ef959d0 into 3a29ba33dc

refactor: Fix Sonar rule cpp:S4998 - avoid unique_ptr const& as parameter
Changed `FindChallenges()` parameter from `const std::unique_ptr<const Node<Key>>&` 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<T> const &" cpp:S4998 > Software qualities impacted: Maintainability
2025-04-29 06:49:38 -04:00 · 2025-04-28 17:42:13 +02:00 · 2025-04-28 16:10:35 +02:00 · 2025-04-28 16:09:34 +02:00 · 2025-04-28 15:47:01 +02:00 · 2025-04-28 15:46:59 +02:00
1 changed files with 23 additions and 22 deletions
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@ -297,28 +297,29 @@ using miniscript::operator""_mst;
 using Node = miniscript::Node<CPubKey>;

 /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
-// NOLINTNEXTLINE(misc-no-recursion)
-std::set<Challenge> FindChallenges(const NodeRef& ref) {
+std::set<Challenge> FindChallenges(const Node* root)
+{
    std::set<Challenge> 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(sub);
-        chal.insert(sub_chal.begin(), sub_chal.end());
+
+    for (std::vector stack{root}; !stack.empty();) {
+        const auto* ref{stack.back()};
+        stack.pop_back();
+
+        for (const auto& key : ref->keys) {
+            chal.emplace(ChallengeType::PK, ChallengeNumber(key));
+        }
+        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());
+        }
    }
    return chal;
 }
@ -347,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);
-    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<Challenge> challist(challenges.begin(), challenges.end());
    for (int iter = 0; iter < 3; ++iter) {
        std::shuffle(challist.begin(), challist.end(), m_rng);
Author	SHA1	Message	Date
l0rinc	2e1184e328	Merge `7e8ef959d0` into `3a29ba33dc`	2025-04-28 17:42:13 +02:00
Lőrinc	7e8ef959d0	refactor: Fix Sonar rule `cpp:S4998` - avoid unique_ptr const& as parameter Changed `FindChallenges()` parameter from `const std::unique_ptr<const Node<Key>>&` 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<T> const &" cpp:S4998 > Software qualities impacted: Maintainability	2025-04-28 16:10:35 +02:00
Lőrinc	e400ac5352	refactor: simplify repeated comparisons in `FindChallenges` This obviates that the LHS of the comparison is always the same	2025-04-28 16:09:34 +02:00
Lőrinc	f670836112	test: remove old recursive `FindChallenges_recursive` implementation The performance of the test is the same as before, with the recursive method.	2025-04-28 15:47:01 +02:00
Lőrinc	b80d0bdee4	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>	2025-04-28 15:46:59 +02:00