test: avoid stack overflow in FindChallenges via manual iteration #32351

pull l0rinc wants to merge 4 commits into bitcoin:master from l0rinc:l0rinc/FindChallenges changing 1 files +23 −22
  1. l0rinc commented at 2:15 pm on April 25, 2025: contributor

    FindChallenges explores the Miniscript node tree by going deep into the first child’s subtree, then the second, and so on - effectively performing a pre-order Traversal (Depth-First Search) recursively, using the call stack which can result in stack overflows on Windows debug builds.

    This change replaces the recursive implementation with an iterative version using an explicit stack. The new implementation also performs a pre-order depth-first traversal, though it processes children in right-to-left order (rather than left-to-right) due to the LIFO nature of the stack. Since both versions store results in a std::set, which automatically sorts and deduplicates elements, the exact traversal order doesn’t affect the final result.

    It is an alternative to increasing the Windows stack size, as proposed in #32349, and addresses the issue raised in #32341 by avoiding deep recursion altogether.

    The change is done in two commits:

    • add a new iterative FindChallenges method and rename the old method to *_recursive (to simplify the next commit where we remove it), asserting that its result matches the original;
    • remove the original recursive implementation.

    This approach avoids ignoring the misc-no-recursion warning as well.

    I tried modifying the new method to store results in a vector instead, but it demonstrated that the deduplication provided by std::set was necessary. One example showing the need for deduplication:

    Recursive (using set):

    0  (6, 9070746)
    1  (6, 19532513)
    2  (6, 3343376967)
    

    Iterative (using vector attempt):

    0  (6, 19532513)
    1  (6, 9070746)
    2  (6, 3343376967)
    3  (6, 9070746)  // Duplicate entry
    

    The performance of the test is the same as before, with the recursive method.

    Fixes https://github.com/bitcoin/bitcoin/issues/32341

  2. DrahtBot commented at 2:15 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/32351.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK sipa, hodlinator, achow101
    Concept ACK darosior
    Stale ACK hebasto

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31713 (miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up) by hodlinator)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Apr 25, 2025
  4. hebasto commented at 2:30 pm on April 25, 2025: member
  5. hebasto commented at 3:15 pm on April 25, 2025: member
    Here is a “Windows, Debug” CI job for this branch in https://github.com/hebasto/bitcoin-core-nightly.
  6. darosior commented at 6:20 pm on April 25, 2025: member
    Concept ACK. Should this marked as Fixes #32341?
  7. l0rinc commented at 8:16 pm on April 25, 2025: contributor

    miniscript_tests (SEGFAULT)

    This was the original failure in debug mode

    Here is a “Windows, Debug” CI job

    While the build is till running, this test passed already:

    089/140 Test  # 60: miniscript_tests .....................   Passed  126.23 sec
    

    Should this marked as Fixes #32341?

    Added it to the description

  8. in src/test/miniscript_tests.cpp:382 in bd89f7eff0 outdated
    376@@ -347,7 +377,9 @@ struct MiniScriptTest : BasicTestingSetup {
    377 /** Run random satisfaction tests. */
    378 void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
    379     auto script = node->ToScript(converter);
    380-    auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
    381+    const auto challenges_recursive{FindChallenges_recursive(node)};
    382+    const auto challenges{FindChallenges(node)}; // Find all challenges in the generated miniscript.
    383+    BOOST_CHECK(std::vector{challenges_recursive} == std::vector{challenges});
    


    hodlinator commented at 9:40 pm on April 26, 2025:

    In bd89f7eff0dc6c44c6a0b96883c3816fe0919f95, why not just:

    0    BOOST_CHECK(challenges_recursive == challenges);
    

    ? https://en.cppreference.com/w/cpp/container/set/operator_cmp claims the complexity is linear with the size.


    l0rinc commented at 9:26 am on April 27, 2025:
    This helped me debug the mentioned vector conversion, but it’s not important anymore, so your suggestion also works.
  9. hodlinator approved
  10. hodlinator commented at 9:55 pm on April 26, 2025: contributor

    ACK 2e533cf86a42b3416c1164d04089e80ffe622dd6

    Prefer this alternative of switching to an iterative algorithm (instead of bumping the stack space #32349).

    On master we do a depth-first gathering of challenges, the new stack based version {Edit: does breadth-first changes the traversal order somewhat}, but result is put in a std::set so traversal order does not matter.

  11. DrahtBot requested review from darosior on Apr 26, 2025
  12. hebasto approved
  13. hebasto commented at 9:15 am on April 27, 2025: member

    ACK 2e533cf86a42b3416c1164d04089e80ffe622dd6, I have reviewed the code and it looks OK.

    If we put refactoring aside, the changes in this PR could be much smaller and easier to review with git diff --ignore-all-space:

     0--- a/src/test/miniscript_tests.cpp
     1+++ b/src/test/miniscript_tests.cpp
     2@@ -297,9 +297,13 @@ using miniscript::operator""_mst;
     3 using Node = miniscript::Node<CPubKey>;
     4 
     5 /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
     6-// NOLINTNEXTLINE(misc-no-recursion)
     7-std::set<Challenge> FindChallenges(const NodeRef& ref) {
     8+std::set<Challenge> FindChallenges(const NodeRef& root) {
     9     std::set<Challenge> chal;
    10+    std::vector<const Node*> stack;
    11+    stack.push_back(root.get());
    12+    while (!stack.empty()) {
    13+        const Node* ref{stack.back()};
    14+        stack.pop_back();
    15         for (const auto& key : ref->keys) {
    16             chal.emplace(ChallengeType::PK, ChallengeNumber(key));
    17         }
    18@@ -317,8 +321,8 @@ std::set<Challenge> FindChallenges(const NodeRef& ref) {
    19             chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data));
    20         }
    21         for (const auto& sub : ref->subs) {
    22-        auto sub_chal = FindChallenges(sub);
    23-        chal.insert(sub_chal.begin(), sub_chal.end());
    24+            stack.push_back(sub.get());
    25+        }
    26     }
    27     return chal;
    28 }
    
  14. l0rinc commented at 9:26 am on April 27, 2025: contributor

    On master we do a depth-first gathering of challenges, the new stack based version does breadth-first, but result is put in a std::set so traversal order does not matter.

    I’ve reworded the commit message and PR description to make this clearer. The original method is pre-order depth-first, where the children are processed in the same order they appear (Left-to-right). The new method is still pre-order depth-first, but children are inserted in the reverse order (Right-to-left). But as you mention, it’s in a set, so the iteration order will still be the same. Also, narrowed the scope of stack by encapsulating it into a for loop, making chal the only variable in-scope when we get to the return statement.

    the changes in this PR could be much smaller and easier to review

    You mean to avoid the use of the switch statement? If you feel strongly about it I can revert that, but the code seemed easy to review in either way.

  15. l0rinc force-pushed on Apr 27, 2025
  16. l0rinc requested review from hodlinator on Apr 27, 2025
  17. l0rinc requested review from hebasto on Apr 27, 2025
  18. hebasto commented at 12:11 pm on April 27, 2025: member

    the changes in this PR could be much smaller and easier to review

    You mean to avoid the use of the switch statement? If you feel strongly about it I can revert that, but the code seemed easy to review in either way.

    I mean to squash all changes into a single commit with functionality changes only. Refactoring changes, such as introducing the switch statement, could be added in a separate commit.

    But not a strong opinion, though.

  19. l0rinc commented at 12:14 pm on April 27, 2025: contributor
    Since CI is checking every commit, I have split out the introduction of the new algorithm, demonstrating that it results in the same output as the original. I’m not sure how to do that in a single commit - unless I add it as a test, which didn’t seem necessary here.
  20. hebasto commented at 9:32 am on April 28, 2025: member

    @theuni

    Due to your #31367 (comment), you might b e interested in reviewing this PR.

  21. 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>
    b80d0bdee4
  22. test: remove old recursive `FindChallenges_recursive` implementation
    The performance of the test is the same as before, with the recursive method.
    f670836112
  23. refactor: simplify repeated comparisons in `FindChallenges`
    This obviates that the LHS of the comparison is always the same
    e400ac5352
  24. 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
    7e8ef959d0
  25. l0rinc force-pushed on Apr 28, 2025
  26. l0rinc commented at 2:12 pm on April 28, 2025: contributor

    Refactoring changes, such as introducing the switch statement, could be added in a separate commit

    Good idea, done in latest push. I have also checked https://corecheck.dev/bitcoin/bitcoin/pulls/32351 and saw that Sonar was also compaining about https://sonarcloud.io/organizations/aureleoules/rules?open=cpp%3AS4998&rule_key=cpp%3AS4998, addressed it also in a separate commit.

  27. sipa commented at 2:19 pm on April 28, 2025: member
    utACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
  28. in src/test/miniscript_tests.cpp:351 in 7e8ef959d0
    347@@ -347,7 +348,7 @@ struct MiniScriptTest : BasicTestingSetup {
    348 /** Run random satisfaction tests. */
    349 void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
    350     auto script = node->ToScript(converter);
    351-    auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
    352+    const auto challenges{FindChallenges(node.get())}; // Find all challenges in the generated miniscript.
    


    hodlinator commented at 3:46 pm on April 28, 2025:

    nit: If you re-touch, it would be nicer to abide by the Sonar suggestion “…or a reference” with:

    0    const auto challenges{FindChallenges(*node)}; // Find all challenges in the generated miniscript.
    

    and

    0std::set<Challenge> FindChallenges(const Node& root)
    

    to signify nullptr being unsupported.

  29. hodlinator approved
  30. hodlinator commented at 3:58 pm on April 28, 2025: contributor

    re-ACK 7e8ef959d0637ca5543ed33d3919937e0d053e70

    nit: Would switch order of:

    • e400ac53524d143467740e2f59698a7c94644c21 refactor: simplify repeated comparisons in FindChallenges
    • f670836112c01feb3cb71618192e9c0c2e55767f test: remove old recursive FindChallenges_recursive implementation

    This way CI should compare the switch-statement iterative version against the recursive one before it is removed. (Something I liked about what I last reviewed).

  31. darosior commented at 6:44 pm on April 28, 2025: member
    My review was requested but since Pieter and Hodlinator beat me to it i think this had enough review already?
  32. l0rinc commented at 6:48 pm on April 28, 2025: contributor
    If you don’t have time to review, a concept ack would still be useful edit: seems your #32351#pullrequestreview-2794937685 is still active
  33. achow101 commented at 10:23 pm on April 29, 2025: member
    ACK 7e8ef959d0637ca5543ed33d3919937e0d053e70
  34. achow101 merged this on Apr 29, 2025
  35. achow101 closed this on Apr 29, 2025

  36. jonatack commented at 10:40 pm on April 29, 2025: member
    Post-merge ACK
  37. l0rinc deleted the branch on Apr 30, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-05 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me