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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process. A summary of reviews will appear here.
🚧 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.
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
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()));
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.
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_ptr
s?
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;
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.