From 922241c7eec66d56c59db33a9223f26ac0121be5 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 10 Sep 2024 15:50:08 -0400 Subject: [PATCH 1/4] 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 --- 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 5026470edcf..f0294df72a9 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(std::move(providers), miniscript::MakeNodeRef(*m_node)); + return std::make_unique(std::move(providers), m_node->Clone()); } }; @@ -2154,7 +2154,7 @@ std::vector> 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(std::move(pubs), node)); + ret.emplace_back(std::make_unique(std::move(pubs), node->Clone())); } return ret; } diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 58f24434f06..ac0d0469422 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -523,6 +523,35 @@ struct Node { } } + NodeRef Clone() const + { + // Use TreeEval() to avoid a stack-overflow due to recursion + auto upfn = [](const Node& node, Span> children) { + NodeRef 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(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(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> new_subs; + for (auto child = children.begin(); child != children.end(); ++child) { + new_subs.emplace_back(std::move(*child)); + } + ret = MakeNodeRef(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, std::move(new_subs), node.k); + } else { + ret = MakeNodeRef(internal::NoDupCheck{}, node.m_script_ctx, node.fragment, node.k); + } + return ret; + }; + return TreeEval>(upfn); + } + private: //! Cached ops counts. const internal::Ops ops; From 3f10ee4fc429adee70a31bd01ed1f6c357877595 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 30 Dec 2024 16:15:13 -0500 Subject: [PATCH 2/4] miniscript: Ensure there is no NodeRef copy constructor or assignment operator --- src/script/miniscript.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index ac0d0469422..80c12ee1f53 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -1687,6 +1687,10 @@ public: : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(sub), val) { DuplicateKeyCheck(ctx); } template 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, use Clone() instead + Node(const Node&) = delete; + Node& operator=(const Node&) = delete; }; namespace internal { From 8d12b599517c3f42daa54fdd8b3f20f92e5cff98 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 30 Dec 2024 16:14:39 -0500 Subject: [PATCH 3/4] miniscript: Make NodeRef a unique_ptr There's no need for it to be a shared_ptr. --- src/script/miniscript.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/script/miniscript.h b/src/script/miniscript.h index 80c12ee1f53..4f5c38bb7b6 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -184,11 +184,11 @@ inline consteval Type operator"" _mst(const char* c, size_t l) { using Opcode = std::pair>; template struct Node; -template using NodeRef = std::shared_ptr>; +template using NodeRef = std::unique_ptr>; -//! Construct a miniscript node as a shared_ptr. +//! Construct a miniscript node as a unique_ptr. template -NodeRef MakeNodeRef(Args&&... args) { return std::make_shared>(std::forward(args)...); } +NodeRef MakeNodeRef(Args&&... args) { return std::make_unique>(std::forward(args)...); } //! The different node types in miniscript. enum class Fragment { From 352391c2cf1a45231ae92ca92d2415b3786ab9ad Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Thu, 7 Nov 2024 10:52:45 -0500 Subject: [PATCH 4/4] qa: check parsed multipath descriptors dont share references --- src/test/descriptor_tests.cpp | 39 +++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp index 24b8f2f793a..d76efc448f5 100644 --- a/src/test/descriptor_tests.cpp +++ b/src/test/descriptor_tests.cpp @@ -429,6 +429,20 @@ void CheckMultipath(const std::string& prv, /*spender_nlocktime=*/0, /*spender_nsequence=*/CTxIn::SEQUENCE_FINAL, /*preimages=*/{}, expanded_prvs.at(i), expanded_pubs.at(i), i); } + + // The descriptor for each path must be standalone. They should not share common references. Test this + // by parsing a multipath descriptor expression, deallocating all but one of the descriptors and making + // sure we can perform operations on it. + FlatSigningProvider prov, out; + std::string error; + const auto desc{[&](){ + auto parsed{Parse(pub, prov, error)}; + assert(parsed.size() > 1); + return std::move(parsed.at(0)); + }()}; + desc->ToString(); + std::vector out_scripts; + desc->Expand(0, prov, out_scripts, out); } void CheckInferDescriptor(const std::string& script_hex, const std::string& expected_desc, const std::vector& hex_scripts, const std::vector>& origin_pubkeys) @@ -807,6 +821,31 @@ BOOST_AUTO_TEST_CASE(descriptor_test) {{0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 0}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 1}, {0x80000000UL + 48, 0x80000000UL + 1, 0x80000000UL, 0x80000000UL + 2, 1, 2}}, } ); + CheckMultipath("tr(xprv9yYge4PS54XkYT9KiLfCRwc8Jeuz8DucxQGtuEecJZYhKNiqbPxYHTPzXtskmzWBqdqkRAGsghNmZzNsfU2wstaB3XjDQFPv567aQSSuPyo/<2;3>,l:pk(xprvA1ADjaN8H3HGnZSmt4VF7YdWoV9oNq8jhqhurxsrYycBAFK555cECoaY22KWt6BTRNLuvobW5VQTF89PN3iA485LAg7epazevPyjCa4xTzd))", + "tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK/<2;3>,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ))", + { + "tr(xprv9yYge4PS54XkYT9KiLfCRwc8Jeuz8DucxQGtuEecJZYhKNiqbPxYHTPzXtskmzWBqdqkRAGsghNmZzNsfU2wstaB3XjDQFPv567aQSSuPyo/2,l:pk(xprvA1ADjaN8H3HGnZSmt4VF7YdWoV9oNq8jhqhurxsrYycBAFK555cECoaY22KWt6BTRNLuvobW5VQTF89PN3iA485LAg7epazevPyjCa4xTzd))", + "tr(xprv9yYge4PS54XkYT9KiLfCRwc8Jeuz8DucxQGtuEecJZYhKNiqbPxYHTPzXtskmzWBqdqkRAGsghNmZzNsfU2wstaB3XjDQFPv567aQSSuPyo/3,l:pk(xprvA1ADjaN8H3HGnZSmt4VF7YdWoV9oNq8jhqhurxsrYycBAFK555cECoaY22KWt6BTRNLuvobW5VQTF89PN3iA485LAg7epazevPyjCa4xTzd))", + }, + { + "tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK/2,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ))", + "tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK/3,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ))", + }, + { + "tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK/2,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ))", + "tr(xpub6CY33ZvKuS63kwDnpNCCo5YrrgkUXgdUKdCVhd4Dru5gCB3z8wGnqFiUP98Za5pYSYF5KmvBHTY3Ra8FAJGggzBjuHS69WzN8gscPupuZwK/3,l:pk(xpub6E9a95u27Qqa13XEz62FUgaFMWzHnHrb54dWfMHU7K9A33eDccvUkbu1sHYoByHAgJdR326rWqn9pGZgZHz1afDprW5gGwS4gUX8Ri6aGPZ))", + }, + XONLY_KEYS, + { + {{"51200e3c14456bfa30f9f0bed6e55f35e1e9ca17c835e9f71b25bac0dfaab38ff2cd"}}, + {{"51202bdda29337ecaf8fcd5aa395febac6f99b8a866a0e8fb3f7bde2e24b1a7df2ba"}}, + }, + OutputType::BECH32M, + { + {{2}, {}}, + {{3}, {}}, + } + ); CheckUnparsable("pkh(xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/<0;1>/<2;3>)", "pkh(xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/<0;1>/<2;3>)", "pkh(): Multiple multipath key path specifiers found"); CheckUnparsable("pkh([deadbeef/<0;1>]xprv9s21ZrQH143K31xYSDQpPDxsXRTUcvj2iNHm5NUtrGiGG5e2DtALGdso3pGz6ssrdK4PFmM8NSpSBHNqPqm55Qn3LqFtT2emdEXVYsCzC2U/0)", "pkh([deadbeef/<0;1>]xpub661MyMwAqRbcFW31YEwpkMuc5THy2PSt5bDMsktWQcFF8syAmRUapSCGu8ED9W6oDMSgv6Zz8idoc4a6mr8BDzTJY47LJhkJ8UB7WEGuduB/0)", "pkh(): Key path value \'<0;1>\' specifies multipath in a section where multipath is not allowed"); CheckUnparsable("tr(xprv9s21ZrQH143K2Zu2kTVKcQi9nKhfgJUkYqG73wXsHuhATm1wkt6kcSZeTYEw2PL7krZtJopEYDvBdYWdAai3n3TWUTCVfHvPHqTYJv7smYe/6/*,{pk(xprvA1RpRA33e1JQ7ifknakTFpgNXPmW2YvmhqLQYMmrj4xJXXWYpDPS3xz7iAxn8L39njGVyuoseXzU6rcxFLJ8HFsTjSyQbLYnMpCqE2VbFWc/<1;2;3>/0/*),pk(xprv9s21ZrQH143K3jUwNHoqQNrtzJnJmx4Yup8NkNLdVQCymYbPbJXnPhwkfTfxZfptcs3rLAPUXS39oDLgrNKQGwbGsEmJJ8BU3RzQuvShEG4/0/0/<3;4>/*)})", "tr(xpub6B4sSbNr8XFYXqqKB7PeUemqgEaVtCLjgd5Lf2VYtezSHozC7ffCvVNCyu9TCgHntRQdimjV3tHbxmNfocxtuh6saNtZEw91gjXLRhQ3Yar/6/*,{pk(xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/<1;2;3>/0/*),pk(xpub6AhFhZJJGt9YB8i85RfrJ8jT3T2FF5EejDCXqXfm1DAczFEXkk8HD3CXTg2TmKM8wTbSnSw3wPg5JuyLitUrpRmkjn2BQXyZnqJx16AGy94/0/0/<3;4>/*)})", "tr(): Multipath subscripts have mismatched lengths");