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)

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.

Type	Reviewers
ACK	l0rinc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

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

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.

hodlinator force-pushed on Feb 9, 2026

hodlinator marked this as a draft on Feb 9, 2026

hodlinator commented at 9:50 pm on February 9, 2026: contributor

I’m currently evaluating a simpler approach to replace the previous 2 last commits (7e1d80f8eceb2db0c0461f3f1335e99b51c267c1 and b13c6ff9919e0f3a7fb117d85f93fb678cc01306) which introduced threads. Was planning to run it on a separate branch but accidentally pushed some test commits here. Moving to draft for now.

DrahtBot added the label CI failed on Feb 9, 2026

DrahtBot commented at 10:52 pm on February 9, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/21841872166/job/63028023404 LLM reason (✨ experimental): Miniscript tests segfaulted (SEGFAULT) causing the CI to fail.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

hodlinator force-pushed on Feb 10, 2026

hodlinator marked this as ready for review on Feb 10, 2026

hodlinator commented at 10:20 am on February 10, 2026: contributor

The current push (7a886f6b2a777df581b89f2fea803d315fe955b0) is ready for review despite the unrelated Windows CI issue. Compared to the last intentional push (b13c6ff9919e0f3a7fb117d85f93fb678cc01306 / #34499#pullrequestreview-3750813111):

Dropped the POSIX+Windows threading commits (see #34499 (comment)).
Embedded a new comment in the currently last commit regarding the tests dependence on the stack being small enough relative the depth.
Also amended the commit message to describe CI failures when running with naive ~Node().

This came after additional out-of-band feedback from @l0rinc and re-reading #31713 (review) by @darosior, especially:

I also think it’s unnecessary to introduce more complexity here and it would be better to just document this test is only meant to reliably fail on CI, and it passing locally is not a guarantee that the author of a change did not introduce a bug there (unless they ran it under a low ulimit like CI does).

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>

39e3295c71

hodlinator force-pushed on Feb 10, 2026

hodlinator commented at 10:28 am on February 10, 2026: contributor

Latest push improves wording in new comment based in part on @DrahtBot feedback.

in src/test/miniscript_tests.cpp:737 in 39e3295c71

734     using miniscript::internal::NoDupCheck;
735     using miniscript::Fragment;
736     using NodeU32 = miniscript::Node<uint32_t>;
737 
738-    constexpr auto ctx{miniscript::MiniscriptContext::P2WSH};
739+    const auto compute_depth{[] (const NodeU32& node) -> size_t {

l0rinc commented at 10:48 am on February 10, 2026:

nit: auto + -> size_t is a bit unorthodox, consider:

0    const auto compute_depth{[] (const NodeU32& node) {

or

0    const size_t compute_depth{[] (const NodeU32& node) {

hodlinator commented at 11:04 am on February 10, 2026:

Agree it’s not common to specify the return type for such short lambdas, will try to remember to remove if I re-touch. (The type of compute_depth is not size_t, it’s a lambda, 2nd suggestion does not compile).

l0rinc approved

l0rinc commented at 10:56 am on February 10, 2026: contributor

ACK 39e3295c71035b960bc9c5d0eeeaed3e06e9d1a6

rebased+tested locally, left a tiny nit, just close it if you disagree, it’s not important.

DrahtBot removed the label CI failed on Feb 10, 2026

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

Reviews