highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-12 16:45:40 +01:00
Code Issues Packages Projects Releases Wiki Activity

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

Browse Source 
7e8ef959d0 refactor: Fix Sonar rule `cpp:S4998` - avoid unique_ptr const& as parameter (Lőrinc)
e400ac5352 refactor: simplify repeated comparisons in `FindChallenges` (Lőrinc)
f670836112 test: remove old recursive `FindChallenges_recursive` implementation (Lőrinc)
b80d0bdee4 test: avoid stack overflow in `FindChallenges` via manual iteration (Lőrinc)

Pull request description:

  `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):
  ```
    (6, 9070746)
    (6, 19532513)
    (6, 3343376967)
  ```
  Iterative (using vector attempt):
  ```
    (6, 19532513)
    (6, 9070746)
    (6, 3343376967)
    (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

ACKs for top commit:
  achow101:
    ACK 7e8ef959d0
  sipa:
    utACK 7e8ef959d0
  hodlinator:
    re-ACK 7e8ef959d0

Tree-SHA512: 9e52eff82a7d76f5d37e3b74c508f08e5fced5386dad504bed111b27ed2b529008a6dd12a5116f009609a94c7ee7ebe3e80a759dda55dd1cb3ae52078f65ec71
This commit is contained in:
Ava Chow
2025-04-29 15:29:50 -07:00
parent 0ed5f37afe 7e8ef959d0
commit 2d5b424414
1 changed files with 23 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
src/test/miniscript_tests.cpp
View File

@@ -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);