test: Increase stack size for "Debug" builds with MSVC #32349

This PR accommodates the deep recursion in the FindChallenges() function used in test/miniscript_tests.cpp.

Fixes #32341 (comment).

CI log: https://github.com/hebasto/bitcoin/actions/runs/14664806617/job/41156972137

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32349.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

I'm not against increasing the Windows stack depth, but the underlying problem may be that we're ignoring warnings such as misc-no-recursion, even when the fix is quite simple.

FindChallenges looks like a simple depth-first search, which should be straightforward to rewrite as an iterative walk:

diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index f253562a2f..14ac44e2c6 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -323,6 +323,35 @@ std::set<Challenge> FindChallenges(const NodeRef& ref) {
     return chal;
 }
 
+std::set<Challenge> FindChallenges_no_recursion(const NodeRef& root)
+{
+    std::set<Challenge> chal;
+    std::vector<const Node*> stack;
+    stack.push_back(root.get());
+
+    while (!stack.empty()) {
+        const Node* 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;
+}
+
 //! 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)
 {
@@ -348,6 +377,8 @@ struct MiniScriptTest : BasicTestingSetup {
 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.
+    auto challenges_no_recursion = FindChallenges_no_recursion(node); // Find all challenges in the generated miniscript.
+    BOOST_CHECK(std::vector{challenges} == std::vector{challenges_no_recursion});
     std::vector<Challenge> challist(challenges.begin(), challenges.end());
     for (int iter = 0; iter < 3; ++iter) {
         std::shuffle(challist.begin(), challist.end(), m_rng);

I'm not against increasing the Windows stack depth, but the underlying problem may be that we're ignoring warnings such as misc-no-recursion, even when the fix is quite simple.

FindChallenges looks like a simple depth-first search, which should be straightforward to rewrite as an iterative walk:

diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index f253562a2f..14ac44e2c6 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -323,6 +323,35 @@ std::set<Challenge> FindChallenges(const NodeRef& ref) {
     return chal;
 }
 
+std::set<Challenge> FindChallenges_no_recursion(const NodeRef& root)
+{
+    std::set<Challenge> chal;
+    std::vector<const Node*> stack;
+    stack.push_back(root.get());
+
+    while (!stack.empty()) {
+        const Node* 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;
+}
+
 //! 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)
 {
@@ -348,6 +377,8 @@ struct MiniScriptTest : BasicTestingSetup {
 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.
+    auto challenges_no_recursion = FindChallenges_no_recursion(node); // Find all challenges in the generated miniscript.
+    BOOST_CHECK(std::vector{challenges} == std::vector{challenges_no_recursion});
     std::vector<Challenge> challist(challenges.begin(), challenges.end());
     for (int iter = 0; iter < 3; ++iter) {
         std::shuffle(challist.begin(), challist.end(), m_rng);

cc @sipa @darosior

That sounds sensible, can you open a PR with this patch and tag me?

Opened #32351 as an alternative fix, cc: @darosior

Closing in favour of #32351.