descriptor: Add proper Clone function to miniscript::Node #30866

achow101 commented at 7:52 PM on September 10, 2024: member

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.

Fixes #30864

DrahtBot commented at 7:52 PM on September 10, 2024: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	darosior, hodlinator, brunoerg
Stale ACK	sipa, davidgumberg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label CI failed on Sep 11, 2024

DrahtBot commented at 9:28 AM on September 11, 2024: contributor

🚧 At least one of the CI tasks failed. <sub>Debug: https://github.com/bitcoin/bitcoin/runs/29955732210</sub>

<details><summary>Hints</summary>

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

fanquake requested review from darosior on Sep 12, 2024

fanquake commented at 3:12 PM on September 12, 2024: member

https://github.com/bitcoin/bitcoin/pull/30866/checks?check_run_id=29955732214:

In file included from /ci_container_base/src/script/descriptor.cpp:10:
/ci_container_base/src/script/miniscript.h: In instantiation of ‘miniscript::Node<Key> miniscript::Node<Key>::Clone() const [with Key = unsigned int]’:
/ci_container_base/src/script/descriptor.cpp:1363:124:   required from here
/ci_container_base/src/script/miniscript.h:535:33: error: moving a local object in a return statement prevents copy elision [-Werror=pessimizing-move]
  535 |             return std::move(ret);
      |                                 ^
/ci_container_base/src/script/miniscript.h:535:33: note: remove ‘std::move’ call
cc1plus: all warnings being treated as errors

achow101 force-pushed on Sep 12, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

in src/script/descriptor.cpp:1363 in c343af67a8 outdated

1359 | @@ -1360,7 +1360,7 @@ class MiniscriptDescriptor final : public DescriptorImpl
1360 |          for (const auto& arg : m_pubkey_args) {
1361 |              providers.push_back(arg->Clone());
1362 |          }
1363 | -        return std::make_unique<MiniscriptDescriptor>(std::move(providers), miniscript::MakeNodeRef<uint32_t>(*m_node));
1364 | +        return std::make_unique<MiniscriptDescriptor>(std::move(providers), miniscript::MakeNodeRef<uint32_t>(m_node->Clone()));

hodlinator commented at 10:11 PM on October 4, 2024:

The PR description asserts:

Multipath descriptors requires performing a deep copy

Would be happy if you cared to add an elaboration on why that is.

It seemed to me like it should be safe to just have another shared_ptr point to the same const Node. Unless something on the outside could own a non-const reference into the node hierarchy and mutate it that way? In that case maybe it would be more robust for the MiniscriptDescriptor-ctor to be the one ensuring it holds a unique reference by doing the node->Clone() there instead (if .use_count() > 1).

Was able to avoid the crash in #30864 using only this on top of the parent commit of this PR:

        return std::make_unique<MiniscriptDescriptor>(std::move(providers), m_node);

Probably causes other bugs that are obvious for those who understand more of the context. Change passes both unit and non-extended functional tests though.

achow101 commented at 11:53 PM on October 4, 2024:

It's not strictly necessary but I decided to do it this way to follow the pattern used for all other descriptors. The multipath expanded descriptors are treated as separate descriptors everywhere, and doing a deep copy retains that behavior which will allow for future changes that may modify specific descriptors.

hodlinator commented at 7:51 PM on October 5, 2024:

How come the shallow copy in the version before the PR is causing a problem in this case though?

It seems the default-generated copy-ctor for Node used before was somehow ending up with corrupt/leaked data, but I've been unable to spot what it is. Can't see any slicing going on. Is something funky being done to the shared_ptrs?

Regardless, it might be worth adding:

    Node(const Node&) = delete;
    Node(Node&&) = delete;
    Node& operator=(const Node&) = delete;
    Node& operator=(Node&&) = delete;

Edit: Later versions of my change make the move-operators = default.

achow101 commented at 10:05 AM on October 16, 2024:

Not entirely sure either.

darosior commented at 5:01 PM on November 6, 2024:

The shallow copy in the version before this PR is causing a problem because we mess with a Node's subs in the destructor to avoid a recursion stack overflow: https://github.com/bitcoin/bitcoin/blob/2c90f8e08c4cf44d4c1ef3dde0e7f7991b8b9390/src/script/miniscript.h#L513-L524

In the scriptpubkeyman fuzz target using the seed from #30864 we would Parse the tr(%17/<2;3>,l:pk(%08)) multipath descriptor into a parsed_descs vector of 2 Descriptor pointers. We would use the first of those descriptors (tr(xpub..../2,l:pk(4bdf....))#jmgpm0u8) and move it into a WalletDescriptor. When exiting the CreateWalletDescriptor function the parsed_descs vector would be destructed, along with the second Descriptor pointer it contains. The destructor of the Node (l:pk(xpub...)) it contains would in turn be called, moving out all its subs that are also pointed to by the first Descriptor's Node. As soon as you try to perform an operation on it it will try to access moved structures.

DrahtBot added the label CI failed on Oct 22, 2024

achow101 force-pushed on Oct 24, 2024

DrahtBot removed the label CI failed on Oct 25, 2024

darosior commented at 5:02 PM on November 6, 2024: member

Concept ACK.

darosior commented at 4:12 PM on November 7, 2024: member

I wrote a regression unit test for this: https://github.com/darosior/bitcoin/commit/d23ffe08ef42475dcd40a13857cd775ac98e0270. It's failing on master but also on this PR. Curiously i had to introduce a new test case with a descriptor close to the one from the fuzzer input as the test is not failing on the existing multipath miniscript descriptor.

in src/script/miniscript.h:543 in 12e2550622 outdated

 533 | +                ret.subs.push_back(MakeNodeRef<Key>(sub));
 534 | +            }
 535 | +            return ret;
 536 | +        };
 537 | +        return TreeEval<Node<Key>>(upfn);
 538 | +    }

darosior commented at 10:14 PM on November 7, 2024:

More smartpointy less segfaulty:

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:

achow101 commented at 10:20 PM on November 7, 2024:

Thanks, taken.

achow101 force-pushed on Nov 7, 2024

achow101 commented at 10:20 PM on November 7, 2024: member

I wrote a regression unit test for this: darosior@d23ffe0.

Added the test commit as well.

in src/script/miniscript.h:650 in 9178f8cf58 outdated

 643 | @@ -630,7 +644,10 @@ struct Node {
 644 |              // If evaluation returns std::nullopt, abort immediately.
 645 |              if (!result) return {};
 646 |              // Replace the last node.subs.size() elements of results with the new result.
 647 | -            results.erase(results.end() - node.subs.size(), results.end());
 648 | +            // Use pop_back to truncate results to avoid MoveAssignable requirement of erase().
 649 | +            for (size_t i = 0; i < node.subs.size(); ++i) {
 650 | +                results.pop_back();
 651 | +            }

darosior commented at 2:32 PM on November 8, 2024:

This is not necessary anymore.

achow101 commented at 4:50 PM on November 8, 2024:

Removed

achow101 force-pushed on Nov 8, 2024

darosior approved

darosior commented at 9:22 PM on November 8, 2024: member

ACK 6a00ea17c136e67a66f3558dd2d02a07860b0afe

fanquake added this to the milestone 29.0 on Nov 11, 2024

hodlinator commented at 11:53 PM on November 13, 2024: contributor

I've experimented with a change on top of this PR that removes shared_ptr usage from src/script/ altogether in order to clarify ownership.

My change is in 80fca845b5f28677207a8fea4a173baaef23036f. It helped uncover another place where the node needs to be cloned (at the bottom of the outer loop):

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index b099fdfc3f..f0294df72a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -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;
         }

It would be preferable to add that one-line fix to this PR, but I'm also interested to hear if there is any chance of my shared_ptr-removal getting any review as a separate PR. I understand Wallet needs more review of existing PRs, so I'm open to leaving it as draft for a while.

achow101 commented at 12:37 AM on November 14, 2024: member

My change is in 80fca84. It helped uncover another place where the node needs to be cloned (at the bottom of the outer loop):

Good catch, added that.

Do you have a particular test case that triggered there?

hodlinator commented at 1:42 PM on November 14, 2024: contributor

Do you have a particular test case that triggered there?

Worked through several iterations of the shared_ptr-removal over a couple of days, at some point I had:

ret.emplace_back(std::make_unique<MiniscriptDescriptor>(std::move(pubs), std::move(*node)));

'cause I wasn't paying attention to the statement being inside a loop. The second descriptor created in the loop ended up with already-moved-from values, which resulted in an this test failing:

https://github.com/bitcoin/bitcoin/blob/6a00ea17c136e67a66f3558dd2d02a07860b0afe/src/test/descriptor_tests.cpp#L799-L823

An OR_D node we were trying to convert into a string was failing because subs was empty (already moved away from).

sipa commented at 10:21 PM on December 12, 2024: member

utACK 6a00ea17c136e67a66f3558dd2d02a07860b0afe

darosior commented at 10:38 PM on December 12, 2024: member

@achow101

Good catch, added that.

Looks like you didn't.

It helped uncover another place where the node needs to be cloned

Ugh. I should have caught that. Good catch @hodlinator.

sipa commented at 10:42 PM on December 12, 2024: member

I don't think anything in the Bitcoin Core codebase actually needs the std::shared_ptr way of storing miniscript subnodes, and std::unique_ptr would suffice. The std::shared_ptrs were inherited from the miniscript codebase, where the shared_ptrs matter for the policy compiler, but I could understand that Bitcoin Core doesn't want that burden. Using std::unique_ptr instead would not leave any chance for shallow duplication.

in src/script/miniscript.h:530 in 6a00ea17c1 outdated

 522 | @@ -523,6 +523,20 @@ struct Node {
 523 |          }
 524 |      }
 525 |  
 526 | +    NodeRef<Key> Clone() const
 527 | +    {
 528 | +        // Use TreeEval() to avoid a stack-overflow due to recursion
 529 | +        auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
 530 | +            NodeRef<Key> ret(MakeNodeRef<Key>(node));

davidgumberg commented at 1:29 AM on December 18, 2024:

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)

achow101 commented at 9:29 PM on December 30, 2024:

Yes, done and deleted the copy constructor.

achow101 force-pushed on Dec 30, 2024

achow101 commented at 5:47 PM on December 30, 2024: member

Looks like you didn't.

Oops, pushed it.

furszy commented at 6:47 PM on December 30, 2024: member

Any way to add a test that fails/crashes without the last push?

DrahtBot added the label CI failed on Dec 30, 2024

achow101 force-pushed on Dec 30, 2024

achow101 commented at 9:31 PM on December 30, 2024: member

I don't think anything in the Bitcoin Core codebase actually needs the std::shared_ptr way of storing miniscript subnodes, and std::unique_ptr would suffice. The std::shared_ptrs were inherited from the miniscript codebase, where the shared_ptrs matter for the policy compiler, but I could understand that Bitcoin Core doesn't want that burden. Using std::unique_ptr instead would not leave any chance for shallow duplication.

Indeed, the std:;shared_ptr wasn't really needed and I've changed NodeRef to be a std::unique_ptr. This appears to also catch the case found by @hodlinator earlier.

Any way to add a test that fails/crashes without the last push?

I believe changing to std::unique_ptr and removing the copy constructor covers that case.

DrahtBot removed the label CI failed on Dec 31, 2024

in src/script/miniscript.h:540 in 922241c7ee outdated

 535 | +                Assert(node.data.empty() && node.subs.empty());
 536 | +                ret = MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, node.keys, node.k);
 537 | +            } else if (!node.data.empty()) {
 538 | +                Assert(node.keys.empty() && node.subs.empty());
 539 | +                ret = MakeNodeRef<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, node.data, node.k);
 540 | +            } else if (!node.subs.empty()) {

davidgumberg commented at 11:14 PM on January 6, 2025:

just an observation: in all branches other than this one children.empty() == true since node.subs.empty() == true, and more generally children.size() == node.subs.size()

Verified with the following diff:

diff --git a/src/script/miniscript.h b/src/script/miniscript.h
@@ -527,6 +527,7 @@ struct Node {
     {
         // Use TreeEval() to avoid a stack-overflow due to recursion
         auto upfn = [](const Node& node, Span<NodeRef<Key>> children) {
+            Assert(node.subs.size() == children.size());
             NodeRef<Key> ret;

davidgumberg commented at 11:29 PM on January 6, 2025: contributor

crACK https://github.com/bitcoin/bitcoin/pull/30866/commits/815467f46f47df6ff52686b0144430c14a31f4a8

This resolves #30864.

$ echo "dHIoJTE3LzwyOzM+LGw6cGsoJTA4KSk=" | base64 --decode > scriptpubkeyman.crash
$ FUZZ=scriptpubkeyman ./build/src/test/fuzz/fuzz scriptpubkeyman.crash
scriptpubkeyman: succeeded against 1 files in 0s.

The default copy constructor of miniscript::Node can't be safe because of the custom iterative destructor miniscript::~Node and deleting it is an improvement.

DrahtBot requested review from sipa on Jan 6, 2025

DrahtBot requested review from darosior on Jan 6, 2025

in src/script/miniscript.h:1691 in 815467f46f outdated

1686 | @@ -1658,6 +1687,10 @@ struct Node {
1687 |          : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(sub), val) { DuplicateKeyCheck(ctx); }
1688 |      template <typename Ctx> Node(const Ctx& ctx, Fragment nt, uint32_t val = 0)
1689 |          : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, val) { DuplicateKeyCheck(ctx); }
1690 | +
1691 | +    // Delete copy constructor and assignment operator

hodlinator commented at 10:12 AM on January 7, 2025:

nit: Could be more helpful?

    // Delete copy constructor and assignment operator, use Clone() instead

achow101 commented at 8:16 PM on January 7, 2025:

Done

in src/script/miniscript.h:189 in 815467f46f outdated

 183 | @@ -184,11 +184,11 @@ inline consteval Type operator"" _mst(const char* c, size_t l) {
 184 |  using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
 185 |  
 186 |  template<typename Key> struct Node;
 187 | -template<typename Key> using NodeRef = std::shared_ptr<const Node<Key>>;
 188 | +template<typename Key> using NodeRef = std::unique_ptr<const Node<Key>>;
 189 |  
 190 |  //! Construct a miniscript node as a shared_ptr.

hodlinator commented at 11:12 AM on January 7, 2025:

Still mentions shared_ptr.

achow101 commented at 8:16 PM on January 7, 2025:

Removed

hodlinator approved

hodlinator commented at 11:18 AM on January 7, 2025: contributor

ACK 815467f46f47df6ff52686b0144430c14a31f4a8

Amazed by how frictionless it was to switch from shared_ptr -> unique_ptr. unique_ptr still is an extra level of unnecessary indirection compared to my 80fca845b5f28677207a8fea4a173baaef23036f, but the latter is a much more disruptive change. Should probably fix remaining mention of shared_ptr, see inline comment.

Thanks for deleting the copy-ctor & assignment operator as I (imprecisely) suggested, making this kind of mistake no longer compile.

Good to see you could avoid adding a new Node-constructor just to be used in Clone().

Testing

Re-verified that fuzz failure is fixed by the first commit (and that it still works by the last one).

Passed modified unit tests.

achow101 force-pushed on Jan 7, 2025

davidgumberg commented at 8:26 PM on January 7, 2025: contributor

trivial reACK https://github.com/bitcoin/bitcoin/commit/352391c2cf1a45231ae92ca92d2415b3786ab9ad

Recent push only changes comments.

$ git diff 815467f..352391c
diff --git a/src/script/miniscript.h b/src/script/miniscript.h
index 04e487f884..4f5c38bb7b 100644
--- a/src/script/miniscript.h
+++ b/src/script/miniscript.h
@@ -186,7 +186,7 @@ using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
 template<typename Key> struct Node;
 template<typename Key> using NodeRef = std::unique_ptr<const Node<Key>>;

-//! Construct a miniscript node as a shared_ptr.
+//! Construct a miniscript node as a unique_ptr.
 template<typename Key, typename... Args>
 NodeRef<Key> MakeNodeRef(Args&&... args) { return std::make_unique<const Node<Key>>(std::forward<Args>(args)...); }

@@ -1688,7 +1688,7 @@ public:
     template <typename Ctx> Node(const Ctx& ctx, Fragment nt, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, val) { DuplicateKeyCheck(ctx); }

-    // Delete copy constructor and assignment operator
+    // Delete copy constructor and assignment operator, use Clone() instead
     Node(const Node&) = delete;
     Node& operator=(const Node&) = delete;

DrahtBot requested review from hodlinator on Jan 7, 2025

hodlinator approved

hodlinator commented at 9:11 AM on January 8, 2025: contributor

re-ACK 352391c2cf1a45231ae92ca92d2415b3786ab9ad

Confirmed through range-diff to only be comment changes following my previous feedback.

brunoerg approved

brunoerg commented at 2:55 PM on January 10, 2025: contributor

code review ACK 352391c2cf1a45231ae92ca92d2415b3786ab9ad

in src/script/miniscript.h:535 in 922241c7ee outdated

 530 | +            NodeRef<Key> ret;
 531 | +            // As all members of Node are const, except for subs, we need to construct the cloned node with all of these members.
 532 | +            // However, there is no constructor that takes all three of data, keys, and subs.
 533 | +            // But, they are mutually exclusive, so we can use the appropriate constructor depending on what is available.
 534 | +            if (!node.keys.empty()) {
 535 | +                Assert(node.data.empty() && node.subs.empty());

darosior commented at 9:07 PM on January 10, 2025:

It seems much simpler and robust to just add the missing constructor than matching on available data and asserting?

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)

hodlinator commented at 9:17 PM on January 10, 2025:

That simplifies Clone() a lot, only thing I would add is make the new constructor private if we want to discourage outside uses.

achow101 commented at 10:48 PM on January 20, 2025:

Done

DrahtBot requested review from darosior on Jan 10, 2025

achow101 force-pushed on Jan 20, 2025

in src/script/miniscript.h:535 in 17e59eccec outdated

 530 | +            std::vector<NodeRef<Key>> new_subs;
 531 | +            for (auto child = children.begin(); child != children.end(); ++child) {
 532 | +                new_subs.emplace_back(std::move(*child));
 533 | +            }
 534 | +            // std::make_unique (and therefore MakeNodeRef) doesn't work on private constructors
 535 | +            return std::unique_ptr<const Node<Key>>(new Node<Key>(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, std::move(new_subs), node.keys, node.data, node.k));

hodlinator commented at 8:32 AM on January 21, 2025:

nit: Could make less verbose:

            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}};

Good call on adding the make_unique comment.

achow101 commented at 6:27 PM on January 21, 2025:

Done

hodlinator approved

hodlinator commented at 8:48 AM on January 21, 2025: contributor

re-ACK 17e59eccec2a94417fe932f8bfcecee46c85d9a3

git range-diff master 352391c 17e59ec

Added private (cheers!) constructor, simplifying Clone().

DrahtBot requested review from davidgumberg on Jan 21, 2025

DrahtBot requested review from brunoerg on Jan 21, 2025

brunoerg approved

brunoerg commented at 9:56 AM on January 21, 2025: contributor

reACK 17e59eccec2a94417fe932f8bfcecee46c85d9a3

352391c2cf1a45231ae92ca92d2415b3786ab9ad..17e59eccec2a94417fe932f8bfcecee46c85d9a3: nice simplification!

DrahtBot added the label Needs rebase on Jan 21, 2025

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>

6d11c9c60b

miniscript: Ensure there is no NodeRef copy constructor or assignment operator 9ccb46f91a

miniscript: Make NodeRef a unique_ptr

There's no need for it to be a shared_ptr.

09a1875ad8

qa: check parsed multipath descriptors dont share references 66d21d0eb6

achow101 force-pushed on Jan 21, 2025

DrahtBot removed the label Needs rebase on Jan 21, 2025

darosior approved

darosior commented at 7:07 PM on January 21, 2025: member

re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc

DrahtBot requested review from hodlinator on Jan 21, 2025

DrahtBot requested review from brunoerg on Jan 21, 2025

hodlinator approved

hodlinator commented at 7:22 PM on January 21, 2025: contributor

re-ACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc :rocket:

brunoerg approved

brunoerg commented at 7:25 PM on January 21, 2025: contributor

reACK 66d21d0eb6517e04ebfb9fad4085e788de51b4dc

fanquake merged this on Jan 22, 2025

fanquake closed this on Jan 22, 2025

sedited referenced this in commit e1405325c9 on Feb 3, 2025

stickies-v referenced this in commit d760fd3dda on Mar 17, 2025

stickies-v referenced this in commit cc83553352 on Mar 17, 2025

stickies-v referenced this in commit 2614933f06 on Mar 17, 2025

stickies-v referenced this in commit b70418c5fc on Mar 17, 2025

stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025

hodlinator referenced this in commit b24cdc1cb2 on Jun 24, 2025

hodlinator referenced this in commit b0dd90ab07 on Jul 2, 2025

bug-castercv502 referenced this in commit bf2e6853b0 on Sep 28, 2025

hodlinator referenced this in commit b806ced214 on Nov 19, 2025

hodlinator referenced this in commit 6e0e5ff973 on Dec 18, 2025

hodlinator referenced this in commit b9f52e6081 on Jan 3, 2026

hodlinator referenced this in commit 22e4115312 on Jan 20, 2026

bitcoin locked this on Jan 22, 2026