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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31713.

Reviews

See the guideline for information on the review process.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34141 (miniscript: Use Func and Expr when parsing keys, hashes, and locktimes by achow101)
• #32471 (wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key by Eunovo)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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):

• node.Satisfy(satisfier, witness_mal.stack, false) in src/test/fuzz/miniscript.cpp
• node.Satisfy(satisfier, witness_nonmal.stack, true) in src/test/fuzz/miniscript.cpp
• node.Satisfy(satisfier, witness_mal.stack, false) in src/test/miniscript_tests.cpp
• node.Satisfy(satisfier, witness_nonmal.stack, true) in src/test/miniscript_tests.cpp

^2026-01-03

CC @achow101 @darosior @brunoerg from parent PR to gauge their interest.

It's unfortunate that this PR touches many lines, so if there's little support for the refactoring I'm fine with closing, or letting it linger for a while (I understand this is super-low priority). At least it helped find the issue mentioned at the top of the summary.

CI failure looks unrelated. Could you s/script/miniscript/ in the PR title to make it clear to reviewer this does not touch consensus critical stuff?

It's unfortunate that this PR touches many lines

It's also not a part of the codebase where i expect much churn, so it probably lowers the bar for doing something like this.

CI failure looks unrelated.

I can't seem to find it. It would be good to always report or at least leave a comment with the log to any CI failure. Otherwise, it will remain unfixed and will just happen on another pull later on.

leave a comment with the log to any CI failure

I think they meant the p2p_1p1c_network failure at https://github.com/bitcoin/bitcoin/actions/runs/12917231673/job/36023158007?pr=31713

I think they meant the p2p_1p1c_network failure at https://github.com/bitcoin/bitcoin/actions/runs/12917231673/job/36023158007?pr=31713

Yes, that's the one, thanks for finding it @l0rinc!

Could you s/script/miniscript/ in the PR title to make it clear to reviewer this does not touch consensus critical stuff?

Updated PR title and commit message titles.

From a quick skim, this looks fine to me. I wonder if @sipa has any objection to not using pointers here.

Let me see if we can get the conflicts in first and then do this when nothing else is conflicting anymore.

Very rough-skim Concept ACK.

This will make the code diverge further from the c++ miniscript policy compiler code, but that's a bullet we already took with the shared_ptr to unique_ptr change, and it doesn't seem there is development happening on that anyway.

Redid the rebase locally, besides an import collision the following was added:

diff --git a/src/test/miniscript_tests.cpp b/src/test/miniscript_tests.cpp
--- a/src/test/miniscript_tests.cpp	(revision 31196b79cabb6e4519e8a27165e1be75c5044403)
+++ b/src/test/miniscript_tests.cpp	(date 1763547689484)
@@ -727,7 +727,7 @@
     g_testdata.reset();
 }
 
-// Confirm that ~Node() and Node::Clone() are stack-safe.
+// Confirm that ~Node(), Node::Clone() and operator=(Node&&) are stack-safe.
 BOOST_AUTO_TEST_CASE(node_deep_destruct)
 {
     using miniscript::internal::NoDupCheck;
@@ -744,6 +744,8 @@
 
     auto clone{root.Clone()};
     BOOST_CHECK_EQUAL(clone.ScriptSize(), root.ScriptSize());
+
+    clone = std::move(root);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

Code coverage indicates both move method are tested now:
https://github.com/bitcoin/bitcoin/blob/c6ac239124b3e66f2d5ff52edc6ff422926abead/src/script/miniscript.h#L1716-L1717

reACK c6ac239124b3e66f2d5ff52edc6ff422926abead

reACK d782bff

The only difference since last ACK was to omit collecting empty leaves in the destructor

@darosior conceptual / final look?

Looks good conceptually. I may get to it later but it's not a priority for me so i don't want to be a blocker.

Sorry for the conflict - happy to re-review after the rebase. Tried it locally, seems trivial.

Rebased to solve trivial conflict with #33192.

Rebased locally, soft reset against re-rebased PR HEAD is empty.

ACK 7909f9f

Rebased to resolve conflict with #33135.

Re-rebased and soft reset the results and the small remaining diff seem good to me.

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index df21f6cdb9..dedc35afc8 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1552,9 +1552,9 @@ public:
         : DescriptorImpl(std::move(providers), "?"), m_node(std::move(node))
     {
         // Traverse miniscript tree for unsafe use of older()
-        miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
-            if (node.fragment == miniscript::Fragment::OLDER) {
-                const uint32_t raw = node.k;
+        miniscript::ForEachNode(m_node, [&](const miniscript::Node<uint32_t>& node) {
+            if (node.Fragment() == miniscript::Fragment::OLDER) {
+                const uint32_t raw = node.K();
                 const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
                 if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
                     const bool is_time_based = (raw & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) != 0;
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 9d5ebcfd87..bd3a16f404 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -191,11 +191,6 @@ inline consteval Type operator""_mst(const char* c, size_t l)
 using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
 
 template<typename Key> class Node;
-template<typename Key> using NodeRef = Node<Key>; // <- TODO: Remove in next commit.
-
-//! Construct a miniscript node (TODO: remove in next commit).
-template<typename Key, typename... Args>
-Node<Key> MakeNodeRef(Args&&... args) { return Node<Key>(std::forward<Args>(args)...); }
 
 //! Unordered traversal of a miniscript node tree.
 template <typename Key, std::invocable<const Node<Key>&> Fn>
@@ -206,8 +201,8 @@ void ForEachNode(const Node<Key>& root, Fn&& fn)
         const Node<Key>& node = stack.back();
         std::invoke(fn, node);
         stack.pop_back();
-        for (const auto& sub : node.subs) {
-            stack.emplace_back(*sub);
+        for (const auto& sub : node.Subs()) {
+            stack.emplace_back(sub);
         }
     }
 }

Also verified with git range-diff -w -U0 master 7909f9f bdc8802 to be sure.
Someone with more expertise in this area should also check it.

code review ACK bdc8802