miniscript: Use valid script in test, etc (#31713 follow-ups)

hodlinator commented at 9:09 am on February 4, 2026: contributor

Add missing move #31713 (review)
Document reason behind moving entire vectors #31713 (review)
Make miniscript valid and improve test name #31713 (review)
Check for miniscript node depth rather than script size #31713 (review)
Use POSIX threads to reliably provoke stack overflow #31713 (review)

refactor(miniscript): Move keys to avoid copy

As done in other ctors.

Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

da51b5e4d2

DrahtBot added the label Descriptors on Feb 4, 2026

DrahtBot commented at 9:10 am on February 4, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

CreateThread(nullptr, /dwStackSize=/requested_stack_size, [] (LPVOID param) -> DWORD { … }, &win32_param, /dwCreationFlags=/STACK_SIZE_PARAM_IS_A_RESERVATION, nullptr) in src/test/miniscript_tests.cpp

2026-02-05 19:06:25

in src/script/miniscript.h:557 in ff28ecf97d

551@@ -552,6 +552,9 @@ class Node
552         // Destroy the subexpressions iteratively after moving out their
553         // subexpressions to avoid a stack-overflow due to recursive calls to
554         // the subs' destructors.
555+        // We move vectors rather than Node-instances in order to only have to
556+        // update array-pointers inside the vectors rather than copying fields
557+        // inside each node.

l0rinc commented at 9:26 am on February 4, 2026:

0        // update array-pointers inside the vectors rather than moving each node individually

hodlinator commented at 11:21 am on February 5, 2026:

Hopefully this is clearer:

0        // We move vectors in order to only update array-pointers inside them
1        // rather than moving individual Node instances which would involve
2        // moving/copying each Node field.

in src/test/miniscript_tests.cpp:752 in d1587724f9

748@@ -737,14 +749,15 @@ BOOST_AUTO_TEST_CASE(node_stress_stack)
749     constexpr auto ctx{miniscript::MiniscriptContext::TAPSCRIPT};
750 
751     NodeU32 root{NoDupCheck{}, ctx, Fragment::JUST_1};
752-    for (uint32_t i{0}; i < 200'000; ++i) {
753+    constexpr uint32_t depth{200'000};

l0rinc commented at 9:32 am on February 4, 2026:

why not just size_t, like ComputeDepth?

hodlinator commented at 11:22 am on February 5, 2026:

Unified on size_t.

in src/test/miniscript_tests.cpp:736 in 0fcdd0d822

730@@ -727,6 +731,7 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
731     g_testdata.reset();
732 }
733 
734+#ifndef WIN32
735 template <typename T>
736 size_t ComputeDepth(miniscript::Node<T>& node)

l0rinc commented at 9:32 am on February 4, 2026:

I don’t like the lingering (and overly general) method, what if we added it inside the test to minimize scope as a const lambda instead? And the while loop is basically a for loop in disguise:

0const auto compute_depth{[](const NodeU32& node) {
1    size_t depth{0};
2    for (const auto* n{&node}; !n->Subs().empty(); n = &n->Subs().front()) {
3        ++depth;
4    }
5    return depth;
6}};

hodlinator commented at 12:43 pm on February 4, 2026:

Taken, although I had to wrestle with the thread-lambda.

in src/test/miniscript_tests.cpp:758 in 0fcdd0d822

756+    // Set a reasonable but small stack size for the new thread below.
757+    const long requested_stack_size{std::max<long>(PTHREAD_STACK_MIN, 4096)};
758+    BOOST_REQUIRE_EQUAL(pthread_attr_setstacksize(&attr, requested_stack_size), EXIT_SUCCESS);
759+
760+    struct Param {
761+        long requested_depth;

l0rinc commented at 9:34 am on February 4, 2026:

why is one depth a long and the other a size_t?

hodlinator commented at 12:44 pm on February 4, 2026:

PTHREAD_STACK_MIN returns long, but I agree on striving towards size_t for more of the code.

in src/test/miniscript_tests.cpp:734 in 0fcdd0d822

730@@ -727,6 +731,7 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
731     g_testdata.reset();
732 }
733 
734+#ifndef WIN32

l0rinc commented at 9:36 am on February 4, 2026:

I’m not sure I understand this one. Instead of checking that the current code is working correctly on Windows, we exclude it completely because it wouldn’t reliably fail for an incorrect implementation?

hodlinator commented at 11:24 am on February 5, 2026:

~~Windows doesn’t respect the requested minimal stack space. I’ve implemented a workaround of munching up stack space using alloca() in latest push, let me know what you think.~~

Edit: I wasn’t using the right flag, the push contains a working reduced stack thread without alloca(). Above comment corresponded to a version I didn’t push here.

Edit2: To be clear, I implemented Windows support in a separate commit. Let me know if you prefer it to be squashed together with POSIX.

l0rinc changes_requested

l0rinc commented at 9:41 am on February 4, 2026: contributor

I’m mostly okay with most of the changes, I don’t like 0fcdd0d8221441fb2dd598a1a2a2b4378848c8e1, it kinda’ throws out the baby with the bathwater. The old test ran everywhere and would still catch stack overflows - it just didn’t guarantee it would fail for a broken implementation reliably.

Also, not sure when it’s uint32_t, when size_t and when long - it should be unified.

doc(miniscript): Explain why we operate on vectors

Explains the reason behind 198bbaee4959119a63b4038cd0dbb519f4daf6f0 where we had earlier switched from operating on unique_ptr<Node> to plain Node.

fd7c494c6b

test(miniscript): Make tested script valid

Also give more appropriate name to test.

Co-authored-by: Antoine Poinsot <darosior@protonmail.com>

5af5e87646

test(miniscript): Check for depth rather than script size

Co-authored-by: Antoine Poinsot <darosior@protonmail.com>
Co-authored-by: Lőrinc <pap.lorinc@gmail.com>

24cd733825

test(miniscript): Use POSIX threads to reliably provoke stack overflow

Moves code being tested to run in a thread with minimal stack space, and attempt to provoke exhaustion.

Removes IsValid() check since the generated script may exceed the maximum size.

Fails if switching ~Node() to default-generated implementation, etc.

7e1d80f8ec

test(miniscript): Implement Windows support for provoking stack overflow b13c6ff991

hodlinator force-pushed on Feb 5, 2026

hodlinator commented at 7:35 pm on February 5, 2026: contributor

Latest push was based on feedback from #34499#pullrequestreview-3749938660.

miniscript: Use valid script in test, etc (#31713 follow-ups) #34499

Reviews

LLM Linter (✨ experimental)