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

1. 
   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

2. 
   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

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

3. DrahtBot added the label CI failed on Sep 11, 2024
4. 
   DrahtBot commented at 9:28 am on September 11, 2024: contributor

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

   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.

5. fanquake requested review from darosior on Sep 12, 2024
6. 
   fanquake commented at 3:12 pm on September 12, 2024: member

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

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

7. achow101 force-pushed on Sep 12, 2024
8. DrahtBot removed the label CI failed on Sep 15, 2024
9. 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:

   0        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:

   0    Node(const Node&) = delete;
   1    Node(Node&&) = delete;
   2    Node& operator=(const Node&) = delete;
   3    Node& operator=(Node&&) = delete;
   

   ---

   

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

   Not entirely sure either.
10. DrahtBot added the label CI failed on Oct 22, 2024
11. 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.

    12e2550622
12. achow101 force-pushed on Oct 24, 2024
13. DrahtBot removed the label CI failed on Oct 25, 2024

Contributors
achow101 DrahtBot fanquake hodlinator

Review Requested
darosior