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

Code before:
    const std::vector<Key> keys;
    //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10).
    const std::vector<unsigned char> data;
    //! Subexpressions (for WRAP_*/AND_*/OR_*/ANDOR/THRESH)
    mutable std::vector<NodeRef<Key>> subs;
    //! The Script context for this node. Either P2WSH or Tapscript.
    const MiniscriptContext m_script_ctx;

    /* 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();
            }
        }
    }

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):
It seems much simpler and robust to just add the missing constructor than matching on available data and asserting?
```diff
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f8845..07214bdf2a4 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -527,27 +527,11 @@ struct Node {
     {
         // Use TreeEval() to avoid a stack-overflow due to recursion
         auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
-            NodeRef<Key> ret;
-            // As all members of Node are const, except for subs, we need to construct the cloned node with all of these members.
-            // However, there is no constructor that takes all three of data, keys, and subs.
-            // But, they are mutually exclusive, so we can use the appropriate constructor depending on what is available.
-            if (!node.keys.empty()) {
-                Assert(node.data.empty() && node.subs.empty());
-                ret = MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, node.keys, node.k);
-            } else if (!node.data.empty()) {
-                Assert(node.keys.empty() && node.subs.empty());
-                ret = MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, node.data, node.k);
-            } else if (!node.subs.empty()) {
-                Assert(node.data.empty() && node.keys.empty());
-                std::vector<NodeRef<Key>> new_subs;
-                for (auto child = children.begin(); child != children.end(); ++child) {
-                    new_subs.emplace_back(std::move(*child));
-                }
-                ret = MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, std::move(new_subs), node.k);
-            } else {
-                ret = MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, node.k);
+            std::vector<NodeRef<Key>> new_subs;
+            for (auto child = children.begin(); child != children.end(); ++child) {
+                new_subs.emplace_back(std::move(*child));
             }
-            return ret;
+            return MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, std::move(new_subs), node.keys, node.data, node.k);
         };
         return TreeEval<NodeRef<Key>>(upfn);
     }
@@ -1661,6 +1645,8 @@ public:
     bool operator==(const Node<Key>& arg) const { return Compare(*this, arg) == 0; }
 
     // Constructors with various argument combinations, which bypass the duplicate key check.
+    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), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
     Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>> sub, std::vector<unsigned char> arg, uint32_t val = 0)
         : fragment(nt), k(val), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
     Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<unsigned char> arg, uint32_t val = 0)
```

Code after:
    const std::vector<Key> keys;
    //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10).
    const std::vector<unsigned char> data;
    //! Subexpressions (for WRAP_*/AND_*/OR_*/ANDOR/THRESH)
    mutable std::vector<NodeRef<Key>> subs;
    //! The Script context for this node. Either P2WSH or Tapscript.
    const MiniscriptContext m_script_ctx;

    /* 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.