test(miniscript): Check for depth rather than script size
What the developer wrote
Authored by Hodlinator
test(miniscript): Check for depth rather than script size
CI failure due to default ~Node() implementation has been confirmed on Windows native, MacOS native, 32-bit ARM, ASan+LSan+UBSan+integer, i686, TSan, MSan. (Not on Alpine as that runs without CI_LIMIT_STACK_SIZE).
Co-authored-by: Antoine Poinsot <darosior@protonmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>
What changed, and why it matters
This is a test-only change in Bitcoin Core's miniscript unit tests. It swaps one internal test assertion (checking script size) for another (checking tree depth) so that the test can reliably detect stack-unsafe destructor implementations under CI's reduced stack size. It does not change production code, consensus rules, network behavior, or wallet operations.
No security action required. Treat as normal test maintenance. If reviewing related destructor changes, ensure Node destructors are stack-safe under deep recursion, but that concern is outside this commit's scope.
Security signals we found
No production code changed
Test-only modification
No cryptographic, consensus, P2P, or wallet changes
Comment references stack-unsafe destructor behavior in test environment only
Evidence from the diff
The commit modifies src/test/miniscript_tests.cpp’s node_stress_stack test. Previously the test built a 200,000-deep miniscript tree and asserted on ScriptSize(). The patch adds a compute_depth helper and asserts on tree depth instead. The stated reason is that CI jobs using CI_LIMIT_STACK_SIZE (512 kB stack) were failing when Node’s destructor was stack-unsafe (~Node()=default causing deep recursion). By checking depth, the test still exercises deep trees and stack-safe destruction without depending on script-size logic that was unrelated to the stack-safety concern.
Changed components
src/test/miniscript_tests.cppInspect captured patch +18 / −3
diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
index 2bb96144..b1cfad94 100644
--- a/src/test/miniscript_tests.cpp
+++ b/src/test/miniscript_tests.cpp
@@ -734,18 +734,33 @@ BOOST_AUTO_TEST_CASE(node_stress_stack)
using miniscript::Fragment;
using NodeU32 = miniscript::Node<uint32_t>;
+ const auto compute_depth{[] (const NodeU32& node) -> size_t {
+ size_t depth{0};
+ for (const auto* n{&node}; !n->Subs().empty(); n = &n->Subs().front()) {
+ ++depth;
+ }
+ return depth;
+ }};
+
constexpr auto ctx{miniscript::MiniscriptContext::TAPSCRIPT};
NodeU32 root{NoDupCheck{}, ctx, Fragment::JUST_1};
- for (uint32_t i{0}; i < 200'000; ++i) {
+ // Some CI jobs run with CI_LIMIT_STACK_SIZE which reduces the stack size
+ // via ulimit to 512 kbytes. When tested with ~Node()=default (stack-unsafe)
+ // implementations the test has been shown to fail for the below depth.
+ // The test may pass locally despite stack-unsafe implementations unless the
+ // stack is reduced in a similar way or the depth is temporarily increased.
+ constexpr size_t depth{200'000};
+ for (size_t i{0}; i < depth; ++i) {
root = NodeU32{NoDupCheck{}, ctx, Fragment::WRAP_N, Vector(std::move(root))};
}
BOOST_CHECK(root.IsValid());
- BOOST_CHECK_EQUAL(root.ScriptSize(), 200'001);
+ BOOST_CHECK_EQUAL(compute_depth(root), depth);
auto clone{root.Clone()};
- BOOST_CHECK_EQUAL(clone.ScriptSize(), root.ScriptSize());
+ BOOST_CHECK_EQUAL(compute_depth(clone), depth);
clone = std::move(root);
+ BOOST_CHECK_EQUAL(compute_depth(clone), depth);
}
BOOST_AUTO_TEST_SUITE_END()
Why this scored 12/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.