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

hebasto commented at 12:42 pm on April 25, 2025: member

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

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

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

8684dc7f9c

hebasto added the label Windows on Apr 25, 2025

hebasto added the label Tests on Apr 25, 2025

DrahtBot commented at 12:42 pm on April 25, 2025: contributor

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

Code Coverage & Benchmarks

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

Reviews

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

Conflicts

No conflicts as of last run.

in CMakeLists.txt:295 in 8684dc7f9c

288@@ -289,6 +289,11 @@ if(WIN32)
289       /Zc:__cplusplus
290       /sdl
291     )
292+    target_link_options(core_interface INTERFACE
293+      # Increase stack size to accommodate the deep recursion
294+      # in the FindChallenges() function used in miniscript_tests.
295+      $<$<CONFIG:Debug>:/STACK:2097152>

fanquake commented at 12:54 pm on April 25, 2025:

Where does this reserve value come from?

hebasto commented at 1:07 pm on April 25, 2025:

Determined empirically—this value was found to be sufficient during testing.

l0rinc commented at 1:02 pm on April 25, 2025: contributor

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:

 0diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
 1index f253562a2f..14ac44e2c6 100644
 2--- a/src/test/miniscript_tests.cpp
 3+++ b/src/test/miniscript_tests.cpp
 4@@ -323,6 +323,35 @@ std::set<Challenge> FindChallenges(const NodeRef& ref) {
 5     return chal;
 6 }
 7 
 8+std::set<Challenge> FindChallenges_no_recursion(const NodeRef& root)
 9+{
10+    std::set<Challenge> chal;
11+    std::vector<const Node*> stack;
12+    stack.push_back(root.get());
13+
14+    while (!stack.empty()) {
15+        const Node* ref{stack.back()};
16+        stack.pop_back();
17+
18+        for (const auto& key : ref->keys) {
19+            chal.emplace(ChallengeType::PK, ChallengeNumber(key));
20+        }
21+        switch (ref->fragment) {
22+        case Fragment::OLDER: chal.emplace(ChallengeType::OLDER, ref->k); break;
23+        case Fragment::AFTER: chal.emplace(ChallengeType::AFTER, ref->k); break;
24+        case Fragment::SHA256: chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data)); break;
25+        case Fragment::RIPEMD160: chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data)); break;
26+        case Fragment::HASH256: chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data)); break;
27+        case Fragment::HASH160: chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); break;
28+        default: break;
29+        }
30+        for (const auto& sub : ref->subs) {
31+            stack.push_back(sub.get());
32+        }
33+    }
34+    return chal;
35+}
36+
37 //! The spk for this script under the given context. If it's a Taproot output also record the spend data.
38 CScript ScriptPubKey(miniscript::MiniscriptContext ctx, const CScript& script, TaprootBuilder& builder)
39 {
40@@ -348,6 +377,8 @@ struct MiniScriptTest : BasicTestingSetup {
41 void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
42     auto script = node->ToScript(converter);
43     auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
44+    auto challenges_no_recursion = FindChallenges_no_recursion(node); // Find all challenges in the generated miniscript.
45+    BOOST_CHECK(std::vector{challenges} == std::vector{challenges_no_recursion});
46     std::vector<Challenge> challist(challenges.begin(), challenges.end());
47     for (int iter = 0; iter < 3; ++iter) {
48         std::shuffle(challist.begin(), challist.end(), m_rng);

hebasto commented at 1:12 pm on April 25, 2025: member

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:

 0diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
 1index f253562a2f..14ac44e2c6 100644
 2--- a/src/test/miniscript_tests.cpp
 3+++ b/src/test/miniscript_tests.cpp
 4@@ -323,6 +323,35 @@ std::set<Challenge> FindChallenges(const NodeRef& ref) {
 5     return chal;
 6 }
 7 
 8+std::set<Challenge> FindChallenges_no_recursion(const NodeRef& root)
 9+{
10+    std::set<Challenge> chal;
11+    std::vector<const Node*> stack;
12+    stack.push_back(root.get());
13+
14+    while (!stack.empty()) {
15+        const Node* ref{stack.back()};
16+        stack.pop_back();
17+
18+        for (const auto& key : ref->keys) {
19+            chal.emplace(ChallengeType::PK, ChallengeNumber(key));
20+        }
21+        switch (ref->fragment) {
22+        case Fragment::OLDER: chal.emplace(ChallengeType::OLDER, ref->k); break;
23+        case Fragment::AFTER: chal.emplace(ChallengeType::AFTER, ref->k); break;
24+        case Fragment::SHA256: chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data)); break;
25+        case Fragment::RIPEMD160: chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data)); break;
26+        case Fragment::HASH256: chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data)); break;
27+        case Fragment::HASH160: chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data)); break;
28+        default: break;
29+        }
30+        for (const auto& sub : ref->subs) {
31+            stack.push_back(sub.get());
32+        }
33+    }
34+    return chal;
35+}
36+
37 //! The spk for this script under the given context. If it's a Taproot output also record the spend data.
38 CScript ScriptPubKey(miniscript::MiniscriptContext ctx, const CScript& script, TaprootBuilder& builder)
39 {
40@@ -348,6 +377,8 @@ struct MiniScriptTest : BasicTestingSetup {
41 void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
42     auto script = node->ToScript(converter);
43     auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
44+    auto challenges_no_recursion = FindChallenges_no_recursion(node); // Find all challenges in the generated miniscript.
45+    BOOST_CHECK(std::vector{challenges} == std::vector{challenges_no_recursion});
46     std::vector<Challenge> challist(challenges.begin(), challenges.end());
47     for (int iter = 0; iter < 3; ++iter) {
48         std::shuffle(challist.begin(), challist.end(), m_rng);

cc @sipa @darosior

darosior commented at 1:17 pm on April 25, 2025: member

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

l0rinc referenced this in commit bd89f7eff0 on Apr 25, 2025

l0rinc commented at 2:16 pm on April 25, 2025: contributor

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

hebasto marked this as a draft on Apr 25, 2025

hebasto commented at 9:19 am on April 27, 2025: member

Closing in favour of #32351.

hebasto closed this on Apr 27, 2025

l0rinc referenced this in commit 28f7f3f58d on Apr 27, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts