mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-04-29 14:59:39 -04:00
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>
This commit is contained in:
parent
80e6ad9e30
commit
b80d0bdee4
1 changed files with 37 additions and 3 deletions
|
@ -298,7 +298,7 @@ using Node = miniscript::Node<CPubKey>;
|
||||||
|
|
||||||
/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
|
/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
|
||||||
// NOLINTNEXTLINE(misc-no-recursion)
|
// NOLINTNEXTLINE(misc-no-recursion)
|
||||||
std::set<Challenge> FindChallenges(const NodeRef& ref) {
|
std::set<Challenge> FindChallenges_recursive(const NodeRef& ref) {
|
||||||
std::set<Challenge> chal;
|
std::set<Challenge> chal;
|
||||||
for (const auto& key : ref->keys) {
|
for (const auto& key : ref->keys) {
|
||||||
chal.emplace(ChallengeType::PK, ChallengeNumber(key));
|
chal.emplace(ChallengeType::PK, ChallengeNumber(key));
|
||||||
|
@ -317,12 +317,44 @@ std::set<Challenge> FindChallenges(const NodeRef& ref) {
|
||||||
chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data));
|
chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data));
|
||||||
}
|
}
|
||||||
for (const auto& sub : ref->subs) {
|
for (const auto& sub : ref->subs) {
|
||||||
auto sub_chal = FindChallenges(sub);
|
auto sub_chal = FindChallenges_recursive(sub);
|
||||||
chal.insert(sub_chal.begin(), sub_chal.end());
|
chal.insert(sub_chal.begin(), sub_chal.end());
|
||||||
}
|
}
|
||||||
return chal;
|
return chal;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
|
||||||
|
std::set<Challenge> FindChallenges(const NodeRef& root)
|
||||||
|
{
|
||||||
|
std::set<Challenge> chal;
|
||||||
|
|
||||||
|
for (std::vector stack{root.get()}; !stack.empty();) {
|
||||||
|
const Node* ref{stack.back()};
|
||||||
|
stack.pop_back();
|
||||||
|
|
||||||
|
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) {
|
||||||
|
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.
|
//! 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)
|
CScript ScriptPubKey(miniscript::MiniscriptContext ctx, const CScript& script, TaprootBuilder& builder)
|
||||||
{
|
{
|
||||||
|
@ -347,7 +379,9 @@ struct MiniScriptTest : BasicTestingSetup {
|
||||||
/** Run random satisfaction tests. */
|
/** Run random satisfaction tests. */
|
||||||
void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
|
void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
|
||||||
auto script = node->ToScript(converter);
|
auto script = node->ToScript(converter);
|
||||||
auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
|
const auto challenges_recursive{FindChallenges_recursive(node)};
|
||||||
|
const auto challenges{FindChallenges(node)}; // Find all challenges in the generated miniscript.
|
||||||
|
BOOST_CHECK(challenges_recursive == challenges);
|
||||||
std::vector<Challenge> challist(challenges.begin(), challenges.end());
|
std::vector<Challenge> challist(challenges.begin(), challenges.end());
|
||||||
for (int iter = 0; iter < 3; ++iter) {
|
for (int iter = 0; iter < 3; ++iter) {
|
||||||
std::shuffle(challist.begin(), challist.end(), m_rng);
|
std::shuffle(challist.begin(), challist.end(), m_rng);
|
||||||
|
|
Loading…
Add table
Reference in a new issue