Repository: bitcoin/bitcoin
File: src/script/miniscript.h (C/C++)

Code before:
            subs.pop_back();
            while (!node->subs.empty()) {
                subs.push_back(std::move(node->subs.back()));
                node->subs.pop_back();
            }
        }
    }

    Node<Key> Clone() const
    {
        // Use TreeEval() to avoid a stack-overflow due to recursion
        auto upfn = [](const Node& node, Span<Node> subs) {
            Node<Key> ret(node);
            ret.subs.clear();
            for (const auto& sub : subs) {
                ret.subs.push_back(MakeNodeRef<Key>(sub));
            }
            return ret;
        };
        return TreeEval<Node<Key>>(upfn);
    }

private:
    //! Cached ops counts.
    const internal::Ops ops;
    //! Cached stack size bounds.
    const internal::StackSize ss;
    //! Cached witness size bounds.
    const internal::WitnessSize ws;
    //! Cached expression type (computed by CalcType and fed through SanitizeType).
    const Type typ;
    //! Cached script length (computed by CalcScriptLen).
    const size_t scriptlen;
    //! Whether a public key appears more than once in this node. This value is initialized
    //! by all constructors except the NoDupCheck ones. The NoDupCheck ones skip the
    //! computation, requiring it to be done manually by invoking DuplicateKeyCheck().
    //! DuplicateKeyCheck(), or a non-NoDupCheck constructor, will compute has_duplicate_keys
    //! for all subnodes as well.
    mutable std::optional<bool> has_duplicate_keys;


    //! Compute the length of the script for this miniscript (including children).
    size_t CalcScriptLen() const {
        size_t subsize = 0;
        for (const auto& sub : subs) {
            subsize += sub->ScriptSize();
        }
        static constexpr auto NONE_MST{""_mst};
        Type sub0type = subs.size() > 0 ? subs[0]->GetType() : NONE_MST;
        return internal::ComputeScriptLen(fragment, sub0type, subsize, k, subs.size(), keys.size(), m_script_ctx);
    }

Review comment (bug):
More smartpointy less segfaulty:
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 6053c853bb..b099fdfc3f 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1360,7 +1360,7 @@ public:
         for (const auto& arg : m_pubkey_args) {
             providers.push_back(arg->Clone());
         }
-        return std::make_unique<MiniscriptDescriptor>(std::move(providers), miniscript::MakeNodeRef<uint32_t>(m_node->Clone()));
+        return std::make_unique<MiniscriptDescriptor>(std::move(providers), m_node->Clone());
     }
 };
 
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index a74e312e79..1472c9ac4f 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -523,18 +523,18 @@ struct Node {
         }
     }
 
-    Node<Key> Clone() const
+    NodeRef<Key> Clone() const
     {
         // Use TreeEval() to avoid a stack-overflow due to recursion
-        auto upfn = [](const Node& node, Span<Node> subs) {
-            Node<Key> ret(node);
-            ret.subs.clear();
+        auto upfn = [](const Node& node, Span<NodeRef<Key>> subs) {
+            auto ret(MakeNodeRef<Key>(node));
+            ret->subs.clear();
             for (const auto& sub : subs) {
-                ret.subs.push_back(MakeNodeRef<Key>(sub));
+                ret->subs.push_back(sub);
             }
             return ret;
         };
-        return TreeEval<Node<Key>>(upfn);
+        return TreeEval<NodeRef<Key>>(upfn);
     }
 
 private:
```

Code after:
    /* Destroy the shared pointers iteratively to avoid a stack-overflow due to recursive calls
     * to the subs' destructors. */
    ~Node() {
        while (!subs.empty()) {
            auto node = std::move(subs.back());
            subs.pop_back();
            while (!node->subs.empty()) {
                subs.push_back(std::move(node->subs.back()));
                node->subs.pop_back();
            }
        }
    }

    NodeRef<Key> Clone() const
    {
        // Use TreeEval() to avoid a stack-overflow due to recursion
        auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
            std::vector<NodeRef<Key>> new_subs;
            for (auto child = children.begin(); child != children.end(); ++child) {
                new_subs.emplace_back(std::move(*child));
            }
            // std::make_unique (and therefore MakeNodeRef) doesn't work on private constructors
            return std::unique_ptr<Node>{new Node{internal::NoDupCheck{}, node.m_script_ctx, node.fragment, std::move(new_subs), node.keys, node.data, node.k}};
        };
        return TreeEval<NodeRef<Key>>(upfn);
    }

private:
    //! Cached ops counts.
    const internal::Ops ops;
    //! Cached stack size bounds.
    const internal::StackSize ss;
    //! Cached witness size bounds.
    const internal::WitnessSize ws;
    //! Cached expression type (computed by CalcType and fed through SanitizeType).
    const Type typ;
    //! Cached script length (computed by CalcScriptLen).
    const size_t scriptlen;
    //! Whether a public key appears more than once in this node. This value is initialized
    //! by all constructors except the NoDupCheck ones. The NoDupCheck ones skip the
    //! computation, requiring it to be done manually by invoking DuplicateKeyCheck().
    //! DuplicateKeyCheck(), or a non-NoDupCheck constructor, will compute has_duplicate_keys
    //! for all subnodes as well.
    mutable std::optional<bool> has_duplicate_keys;

    // Constructor which takes all of the data that a Node could possibly contain.
    // This is kept private as no valid fragment has all of these arguments.
    // Only used by Clone()
    Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>> sub, std::vector<Key> key, std::vector<unsigned char> 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()) {}