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

Code before:
    //! What node type this node is.
    const Fragment fragment;
    //! The k parameter (time for OLDER/AFTER, threshold for THRESH(_M))
    const uint32_t k = 0;
    //! The keys used by this expression (only for PK_K/PK_H/MULTI)
    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();
        }

Review comment (suggestion):
Is there a reasonable way to do this without using the default copy constructor? Since `miniscript::Node`'s destructor means using the implicit copy constructor is always dangerous without modifying the subs as is done here, wouldn't it be best to delete it (if possible) as another reviewer suggested? (https://github.com/bitcoin/bitcoin/pull/30866#discussion_r1788683926)

Code after:
    //! What node type this node is.
    const Fragment fragment;
    //! The k parameter (time for OLDER/AFTER, threshold for THRESH(_M))
    const uint32_t k = 0;
    //! The keys used by this expression (only for PK_K/PK_H/MULTI)
    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;