diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 8733d347136..f2b9cdc4576 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -552,6 +552,9 @@ public: // Destroy the subexpressions iteratively after moving out their // subexpressions to avoid a stack-overflow due to recursive calls to // the subs' destructors. + // We move vectors in order to only update array-pointers inside them + // rather than moving individual Node instances which would involve + // moving/copying each Node field. std::vector> queue; queue.push_back(std::move(subs)); do { @@ -607,7 +610,7 @@ private: // This is kept private as no valid fragment has all of these arguments. // Only used by Clone() Node(internal::NoDupCheck, MiniscriptContext script_ctx, enum Fragment nt, std::vector sub, std::vector key, std::vector arg, uint32_t val) - : fragment(nt), k(val), keys(key), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} + : fragment(nt), k(val), keys(std::move(key)), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {} //! Compute the length of the script for this miniscript (including children). size_t CalcScriptLen() const diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp index 9a3e9ea0789..6dac2aa907f 100644 --- a/src/test/miniscript_tests.cpp +++ b/src/test/miniscript_tests.cpp @@ -727,24 +727,39 @@ BOOST_AUTO_TEST_CASE(fixed_tests) } // Confirm that ~Node(), Node::Clone() and operator=(Node&&) are stack-safe. -BOOST_AUTO_TEST_CASE(node_deep_destruct) +BOOST_AUTO_TEST_CASE(node_stress_stack) { using miniscript::internal::NoDupCheck; using miniscript::Fragment; using NodeU32 = miniscript::Node; - constexpr auto ctx{miniscript::MiniscriptContext::P2WSH}; + 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) { - root = NodeU32{NoDupCheck{}, ctx, Fragment::WRAP_S, Vector(std::move(root))}; + // 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_EQUAL(root.ScriptSize(), 200'001); + BOOST_CHECK(root.IsValid()); + 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()