From 922241c7eec66d56c59db33a9223f26ac0121be5 Mon Sep 17 00:00:00 2001
From: Ava Chow <github@achow101.com>
Date: Tue, 10 Sep 2024 15:50:08 -0400
Subject: [PATCH] descriptor: Add proper Clone function to miniscript::Node

Multipath descriptors requires performing a deep copy, so a Clone
function that does that is added to miniscript::Node instead of the
current shallow copy.

Co-Authored-By: Antoine Poinsot <darosior@protonmail.com>
---
 src/script/descriptor.cpp |  4 ++--
 src/script/miniscript.h   | 29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 5026470edc..f0294df72a 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));
+        return std::make_unique<MiniscriptDescriptor>(std::move(providers), m_node->Clone());
     }
 };
 
@@ -2154,7 +2154,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
                 for (auto& pub : parser.m_keys) {
                     pubs.emplace_back(std::move(pub.at(i)));
                 }
-                ret.emplace_back(std::make_unique<MiniscriptDescriptor>(std::move(pubs), node));
+                ret.emplace_back(std::make_unique<MiniscriptDescriptor>(std::move(pubs), node->Clone()));
             }
             return ret;
         }
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 58f24434f0..ac0d046942 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -523,6 +523,35 @@ struct Node {
         }
     }
 
+    NodeRef<Key> Clone() const
+    {
+        // 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);
+            }
+            return ret;
+        };
+        return TreeEval<NodeRef<Key>>(upfn);
+    }
+
 private:
     //! Cached ops counts.
     const internal::Ops ops;