Merge 11bae1d375 into c5e44a0435

32d55e28af test: Use the correct node for doubled keypath test (Ava Chow)

Pull request description:

  #29124 had a silent merge conflict with #32350 which resulted in it using the wrong node. Fix the test to use the correct v22 node.

ACKs for top commit:
  maflcko:
    lgtm ACK 32d55e28af
  rkrux:
    ACK 32d55e28af
  BrandonOdiwuor:
    Code Review ACK 32d55e28af

Tree-SHA512: 1e0231985beb382b16e1d608c874750423d0502388db0c8ad450b22d17f9d96f5e16a6b44948ebda5efc750f62b60d0de8dd20131f449427426a36caf374af92

src/script/descriptor.cpp

@@ -1307,13 +1307,13 @@ public:
 class MiniscriptDescriptor final : public DescriptorImpl
 {
 private:
-    miniscript::NodeRef<uint32_t> m_node;
+    miniscript::Node<uint32_t> m_node;
 
 protected:
     std::vector<CScript> MakeScripts(const std::vector<CPubKey>& keys, std::span<const CScript> scripts,
                                      FlatSigningProvider& provider) const override
     {
-        const auto script_ctx{m_node->GetMsCtx()};
+        const auto script_ctx{m_node.GetMsCtx()};
         for (const auto& key : keys) {
             if (miniscript::IsTapscript(script_ctx)) {
                 provider.pubkeys.emplace(Hash160(XOnlyPubKey{key}), key);
@@ -1321,17 +1321,17 @@ protected:
                 provider.pubkeys.emplace(key.GetID(), key);
             }
         }
-        return Vector(m_node->ToScript(ScriptMaker(keys, script_ctx)));
+        return Vector(m_node.ToScript(ScriptMaker(keys, script_ctx)));
     }
 
 public:
-    MiniscriptDescriptor(std::vector<std::unique_ptr<PubkeyProvider>> providers, miniscript::NodeRef<uint32_t> node)
+    MiniscriptDescriptor(std::vector<std::unique_ptr<PubkeyProvider>> providers, miniscript::Node<uint32_t>&& node)
         : DescriptorImpl(std::move(providers), "?"), m_node(std::move(node)) {}
 
     bool ToStringHelper(const SigningProvider* arg, std::string& out, const StringType type,
                         const DescriptorCache* cache = nullptr) const override
     {
-        if (const auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type == StringType::PRIVATE))) {
+        if (const auto res = m_node.ToString(StringMaker(arg, m_pubkey_args, type == StringType::PRIVATE))) {
             out = *res;
             return true;
         }
@@ -1342,15 +1342,15 @@ public:
     bool IsSingleType() const final { return true; }
     bool IsSingleKey() const final { return false; }
 
-    std::optional<int64_t> ScriptSize() const override { return m_node->ScriptSize(); }
+    std::optional<int64_t> ScriptSize() const override { return m_node.ScriptSize(); }
 
     std::optional<int64_t> MaxSatSize(bool) const override {
         // For Miniscript we always assume high-R ECDSA signatures.
-        return m_node->GetWitnessSize();
+        return m_node.GetWitnessSize();
     }
 
     std::optional<int64_t> MaxSatisfactionElems() const override {
-        return m_node->GetStackSize();
+        return m_node.GetStackSize();
     }
 
     std::unique_ptr<DescriptorImpl> Clone() const override
@@ -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), m_node->Clone());
+        return std::make_unique<MiniscriptDescriptor>(std::move(providers), m_node.Clone());
     }
 };
 
@@ -2106,7 +2106,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
             }
             if (!node->IsSane() || node->IsNotSatisfiable()) {
                 // Try to find the first insane sub for better error reporting.
-                auto insane_node = node.get();
+                const auto* insane_node = &*node;
                 if (const auto sub = node->FindInsaneSub()) insane_node = sub;
                 if (const auto str = insane_node->ToString(parser)) error = *str;
                 if (!insane_node->IsValid()) {
@@ -2115,7 +2115,7 @@ std::vector<std::unique_ptr<DescriptorImpl>> ParseScript(uint32_t& key_exp_index
                     error += " is not sane";
                     if (!insane_node->IsNonMalleable()) {
                         error += ": malleable witnesses exist";
-                    } else if (insane_node == node.get() && !insane_node->NeedsSignature()) {
+                    } else if (insane_node == &*node && !insane_node->NeedsSignature()) {
                         error += ": witnesses without signature exist";
                     } else if (!insane_node->CheckTimeLocksMix()) {
                         error += ": contains mixes of timelocks expressed in blocks and seconds";
@@ -2315,7 +2315,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
             for (auto& key : parser.m_keys) {
                 keys.emplace_back(std::move(key.at(0)));
             }
-            return std::make_unique<MiniscriptDescriptor>(std::move(keys), std::move(node));
+            return std::make_unique<MiniscriptDescriptor>(std::move(keys), std::move(*node));
         }
     }
 

src/script/miniscript.h

@@ -188,13 +188,6 @@ inline consteval Type operator""_mst(const char* c, size_t l)
 
 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 unique_ptr.
-template<typename Key, typename... Args>
-NodeRef<Key> MakeNodeRef(Args&&... args) { return std::make_unique<const Node<Key>>(std::forward<Args>(args)...); }
-
 //! The different node types in miniscript.
 enum class Fragment {
     JUST_0,    //!< OP_0
@@ -349,8 +342,8 @@ struct InputResult {
 //! Class whose objects represent the maximum of a list of integers.
 template<typename I>
 struct MaxInt {
-    const bool valid;
-    const I value;
+    bool valid;
+    I value;
 
     MaxInt() : valid(false), value(0) {}
     MaxInt(I val) : valid(true), value(val) {}
@@ -421,11 +414,11 @@ struct Ops {
  */
 struct SatInfo {
     //! Whether a canonical satisfaction/dissatisfaction is possible at all.
-    const bool valid;
+    bool valid;
     //! How much higher the stack size at start of execution can be compared to at the end.
-    const int32_t netdiff;
+    int32_t netdiff;
     //! Mow much higher the stack size can be during execution compared to at the end.
-    const int32_t exec;
+    int32_t exec;
 
     /** Empty script set. */
     constexpr SatInfo() noexcept : valid(false), netdiff(0), exec(0) {}
@@ -480,7 +473,7 @@ struct SatInfo {
 };
 
 struct StackSize {
-    const SatInfo sat, dsat;
+    SatInfo sat, dsat;
 
     constexpr StackSize(SatInfo in_sat, SatInfo in_dsat) noexcept : sat(in_sat), dsat(in_dsat) {};
     constexpr StackSize(SatInfo in_both) noexcept : sat(in_both), dsat(in_both) {};
@@ -503,56 +496,59 @@ struct NoDupCheck {};
 template<typename Key>
 struct Node {
     //! What node type this node is.
-    const Fragment fragment;
+    Fragment fragment;
     //! The k parameter (time for OLDER/AFTER, threshold for THRESH(_M))
-    const uint32_t k = 0;
+    uint32_t k = 0;
     //! The keys used by this expression (only for PK_K/PK_H/MULTI)
-    const std::vector<Key> keys;
+    std::vector<Key> keys;
     //! The data bytes in this expression (only for HASH160/HASH256/SHA256/RIPEMD10).
-    const std::vector<unsigned char> data;
+    std::vector<unsigned char> data;
     //! Subexpressions (for WRAP_*/AND_*/OR_*/ANDOR/THRESH)
-    mutable std::vector<NodeRef<Key>> subs;
+    std::vector<Node> subs;
     //! The Script context for this node. Either P2WSH or Tapscript.
-    const MiniscriptContext m_script_ctx;
+    MiniscriptContext m_script_ctx;
 
-    /* Destroy the shared pointers iteratively to avoid a stack-overflow due to recursive calls
-     * to the subs' destructors. */
-    ~Node() {
-        while (!subs.empty()) {
-            auto node = std::move(subs.back());
-            subs.pop_back();
-            while (!node->subs.empty()) {
-                subs.push_back(std::move(node->subs.back()));
-                node->subs.pop_back();
+    ~Node()
+    {
+        // Destroy the subexpressions iteratively to avoid a stack-overflow due
+        // to recursive calls to the subs' destructors.
+        std::vector<std::vector<Node<Key>>> to_be_stripped;
+        to_be_stripped.emplace_back(std::move(subs));
+        while (!to_be_stripped.empty()) {
+            auto stripping{std::move(to_be_stripped.back())};
+            to_be_stripped.pop_back();
+            for (auto& i : stripping) {
+                to_be_stripped.emplace_back(std::move(i.subs));
             }
         }
     }
 
-    NodeRef<Key> Clone() const
+    Node<Key> Clone() const
     {
         // Use TreeEval() to avoid a stack-overflow due to recursion
-        auto upfn = [](const Node& node, std::span<NodeRef<Key>> children) {
-            std::vector<NodeRef<Key>> new_subs;
-            for (auto child = children.begin(); child != children.end(); ++child) {
-                new_subs.emplace_back(std::move(*child));
+        auto upfn = [](const Node& node, std::span<Node> children) {
+            std::vector<Node> new_subs;
+            for (auto& child : children) {
+                // It's fine to move from children as they are new nodes having
+                // been produced by calling this function one level down.
+                new_subs.emplace_back(std::move(child));
             }
-            // std::make_unique (and therefore MakeNodeRef) doesn't work on private constructors
-            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}};
+            return Node{internal::NoDupCheck{}, node.m_script_ctx, node.fragment, std::move(new_subs), node.keys, node.data, node.k};
         };
-        return TreeEval<NodeRef<Key>>(upfn);
+        return TreeEval<Node>(upfn);
     }
 
 private:
     //! Cached ops counts.
-    const internal::Ops ops;
+    internal::Ops ops;
     //! Cached stack size bounds.
-    const internal::StackSize ss;
+    internal::StackSize ss;
     //! Cached witness size bounds.
-    const internal::WitnessSize ws;
+    internal::WitnessSize ws;
     //! Cached expression type (computed by CalcType and fed through SanitizeType).
-    const Type typ;
+    Type typ;
     //! Cached script length (computed by CalcScriptLen).
-    const size_t scriptlen;
+    size_t scriptlen;
     //! Whether a public key appears more than once in this node. This value is initialized
     //! by all constructors except the NoDupCheck ones. The NoDupCheck ones skip the
     //! computation, requiring it to be done manually by invoking DuplicateKeyCheck().
@@ -563,17 +559,17 @@ private:
     // Constructor which takes all of the data that a Node could possibly contain.
     // This is kept private as no valid fragment has all of these arguments.
     // Only used by Clone()
-    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)
+    Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<Node<Key>> sub, std::vector<Key> key, std::vector<unsigned char> arg, uint32_t val)
         : fragment(nt), k(val), keys(key), data(std::move(arg)), subs(std::move(sub)), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
 
     //! Compute the length of the script for this miniscript (including children).
     size_t CalcScriptLen() const {
         size_t subsize = 0;
         for (const auto& sub : subs) {
-            subsize += sub->ScriptSize();
+            subsize += sub.ScriptSize();
         }
         static constexpr auto NONE_MST{""_mst};
-        Type sub0type = subs.size() > 0 ? subs[0]->GetType() : NONE_MST;
+        Type sub0type = subs.size() > 0 ? subs[0].GetType() : NONE_MST;
         return internal::ComputeScriptLen(fragment, sub0type, subsize, k, subs.size(), keys.size(), m_script_ctx);
     }
 
@@ -644,7 +640,7 @@ private:
                  * that child (and all earlier children) will be at the end of `results`. */
                 size_t child_index = stack.back().expanded++;
                 State child_state = downfn(stack.back().state, node, child_index);
-                stack.emplace_back(*node.subs[child_index], 0, std::move(child_state));
+                stack.emplace_back(node.subs[child_index], 0, std::move(child_state));
                 continue;
             }
             // Invoke upfn with the last node.subs.size() elements of results as input.
@@ -722,7 +718,7 @@ private:
             if (b.subs.size() < a.subs.size()) return 1;
             size_t n = a.subs.size();
             for (size_t i = 0; i < n; ++i) {
-                queue.emplace_back(*a.subs[n - 1 - i], *b.subs[n - 1 - i]);
+                queue.emplace_back(a.subs[n - 1 - i], b.subs[n - 1 - i]);
             }
         }
         return 0;
@@ -735,13 +731,13 @@ private:
         // THRESH has a variable number of subexpressions
         std::vector<Type> sub_types;
         if (fragment == Fragment::THRESH) {
-            for (const auto& sub : subs) sub_types.push_back(sub->GetType());
+            for (const auto& sub : subs) sub_types.push_back(sub.GetType());
         }
         // All other nodes than THRESH can be computed just from the types of the 0-3 subexpressions.
         static constexpr auto NONE_MST{""_mst};
-        Type x = subs.size() > 0 ? subs[0]->GetType() : NONE_MST;
-        Type y = subs.size() > 1 ? subs[1]->GetType() : NONE_MST;
-        Type z = subs.size() > 2 ? subs[2]->GetType() : NONE_MST;
+        Type x = subs.size() > 0 ? subs[0].GetType() : NONE_MST;
+        Type y = subs.size() > 1 ? subs[1].GetType() : NONE_MST;
+        Type z = subs.size() > 2 ? subs[2].GetType() : NONE_MST;
 
         return SanitizeType(ComputeType(fragment, x, y, z, sub_types, k, data.size(), subs.size(), keys.size(), m_script_ctx));
     }
@@ -780,7 +776,7 @@ public:
                 case Fragment::WRAP_C: return BuildScript(std::move(subs[0]), verify ? OP_CHECKSIGVERIFY : OP_CHECKSIG);
                 case Fragment::WRAP_D: return BuildScript(OP_DUP, OP_IF, subs[0], OP_ENDIF);
                 case Fragment::WRAP_V: {
-                    if (node.subs[0]->GetType() << "x"_mst) {
+                    if (node.subs[0].GetType() << "x"_mst) {
                         return BuildScript(std::move(subs[0]), OP_VERIFY);
                     } else {
                         return std::move(subs[0]);
@@ -836,9 +832,9 @@ public:
                     node.fragment == Fragment::WRAP_D || node.fragment == Fragment::WRAP_V ||
                     node.fragment == Fragment::WRAP_J || node.fragment == Fragment::WRAP_N ||
                     node.fragment == Fragment::WRAP_C ||
-                    (node.fragment == Fragment::AND_V && node.subs[1]->fragment == Fragment::JUST_1) ||
-                    (node.fragment == Fragment::OR_I && node.subs[0]->fragment == Fragment::JUST_0) ||
-                    (node.fragment == Fragment::OR_I && node.subs[1]->fragment == Fragment::JUST_0));
+                    (node.fragment == Fragment::AND_V && node.subs[1].fragment == Fragment::JUST_1) ||
+                    (node.fragment == Fragment::OR_I && node.subs[0].fragment == Fragment::JUST_0) ||
+                    (node.fragment == Fragment::OR_I && node.subs[1].fragment == Fragment::JUST_0));
         };
         // The upward function computes for a node, given whether its parent is a wrapper,
         // and the string representations of its child nodes, the string representation of the node.
@@ -850,15 +846,15 @@ public:
                 case Fragment::WRAP_A: return "a" + std::move(subs[0]);
                 case Fragment::WRAP_S: return "s" + std::move(subs[0]);
                 case Fragment::WRAP_C:
-                    if (node.subs[0]->fragment == Fragment::PK_K) {
+                    if (node.subs[0].fragment == Fragment::PK_K) {
                         // pk(K) is syntactic sugar for c:pk_k(K)
-                        auto key_str = ctx.ToString(node.subs[0]->keys[0]);
+                        auto key_str = ctx.ToString(node.subs[0].keys[0]);
                         if (!key_str) return {};
                         return std::move(ret) + "pk(" + std::move(*key_str) + ")";
                     }
-                    if (node.subs[0]->fragment == Fragment::PK_H) {
+                    if (node.subs[0].fragment == Fragment::PK_H) {
                         // pkh(K) is syntactic sugar for c:pk_h(K)
-                        auto key_str = ctx.ToString(node.subs[0]->keys[0]);
+                        auto key_str = ctx.ToString(node.subs[0].keys[0]);
                         if (!key_str) return {};
                         return std::move(ret) + "pkh(" + std::move(*key_str) + ")";
                     }
@@ -869,11 +865,11 @@ public:
                 case Fragment::WRAP_N: return "n" + std::move(subs[0]);
                 case Fragment::AND_V:
                     // t:X is syntactic sugar for and_v(X,1).
-                    if (node.subs[1]->fragment == Fragment::JUST_1) return "t" + std::move(subs[0]);
+                    if (node.subs[1].fragment == Fragment::JUST_1) return "t" + std::move(subs[0]);
                     break;
                 case Fragment::OR_I:
-                    if (node.subs[0]->fragment == Fragment::JUST_0) return "l" + std::move(subs[1]);
-                    if (node.subs[1]->fragment == Fragment::JUST_0) return "u" + std::move(subs[0]);
+                    if (node.subs[0].fragment == Fragment::JUST_0) return "l" + std::move(subs[1]);
+                    if (node.subs[1].fragment == Fragment::JUST_0) return "u" + std::move(subs[0]);
                     break;
                 default: break;
             }
@@ -904,7 +900,7 @@ public:
                 case Fragment::OR_I: return std::move(ret) + "or_i(" + std::move(subs[0]) + "," + std::move(subs[1]) + ")";
                 case Fragment::ANDOR:
                     // and_n(X,Y) is syntactic sugar for andor(X,Y,0).
-                    if (node.subs[2]->fragment == Fragment::JUST_0) return std::move(ret) + "and_n(" + std::move(subs[0]) + "," + std::move(subs[1]) + ")";
+                    if (node.subs[2].fragment == Fragment::JUST_0) return std::move(ret) + "and_n(" + std::move(subs[0]) + "," + std::move(subs[1]) + ")";
                     return std::move(ret) + "andor(" + std::move(subs[0]) + "," + std::move(subs[1]) + "," + std::move(subs[2]) + ")";
                 case Fragment::MULTI: {
                     CHECK_NONFATAL(!is_tapscript);
@@ -954,59 +950,59 @@ private:
             case Fragment::RIPEMD160:
             case Fragment::HASH256:
             case Fragment::HASH160: return {4, 0, {}};
-            case Fragment::AND_V: return {subs[0]->ops.count + subs[1]->ops.count, subs[0]->ops.sat + subs[1]->ops.sat, {}};
+            case Fragment::AND_V: return {subs[0].ops.count + subs[1].ops.count, subs[0].ops.sat + subs[1].ops.sat, {}};
             case Fragment::AND_B: {
-                const auto count{1 + subs[0]->ops.count + subs[1]->ops.count};
-                const auto sat{subs[0]->ops.sat + subs[1]->ops.sat};
-                const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
+                const auto count{1 + subs[0].ops.count + subs[1].ops.count};
+                const auto sat{subs[0].ops.sat + subs[1].ops.sat};
+                const auto dsat{subs[0].ops.dsat + subs[1].ops.dsat};
                 return {count, sat, dsat};
             }
             case Fragment::OR_B: {
-                const auto count{1 + subs[0]->ops.count + subs[1]->ops.count};
-                const auto sat{(subs[0]->ops.sat + subs[1]->ops.dsat) | (subs[1]->ops.sat + subs[0]->ops.dsat)};
-                const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
+                const auto count{1 + subs[0].ops.count + subs[1].ops.count};
+                const auto sat{(subs[0].ops.sat + subs[1].ops.dsat) | (subs[1].ops.sat + subs[0].ops.dsat)};
+                const auto dsat{subs[0].ops.dsat + subs[1].ops.dsat};
                 return {count, sat, dsat};
             }
             case Fragment::OR_D: {
-                const auto count{3 + subs[0]->ops.count + subs[1]->ops.count};
-                const auto sat{subs[0]->ops.sat | (subs[1]->ops.sat + subs[0]->ops.dsat)};
-                const auto dsat{subs[0]->ops.dsat + subs[1]->ops.dsat};
+                const auto count{3 + subs[0].ops.count + subs[1].ops.count};
+                const auto sat{subs[0].ops.sat | (subs[1].ops.sat + subs[0].ops.dsat)};
+                const auto dsat{subs[0].ops.dsat + subs[1].ops.dsat};
                 return {count, sat, dsat};
             }
             case Fragment::OR_C: {
-                const auto count{2 + subs[0]->ops.count + subs[1]->ops.count};
-                const auto sat{subs[0]->ops.sat | (subs[1]->ops.sat + subs[0]->ops.dsat)};
+                const auto count{2 + subs[0].ops.count + subs[1].ops.count};
+                const auto sat{subs[0].ops.sat | (subs[1].ops.sat + subs[0].ops.dsat)};
                 return {count, sat, {}};
             }
             case Fragment::OR_I: {
-                const auto count{3 + subs[0]->ops.count + subs[1]->ops.count};
-                const auto sat{subs[0]->ops.sat | subs[1]->ops.sat};
-                const auto dsat{subs[0]->ops.dsat | subs[1]->ops.dsat};
+                const auto count{3 + subs[0].ops.count + subs[1].ops.count};
+                const auto sat{subs[0].ops.sat | subs[1].ops.sat};
+                const auto dsat{subs[0].ops.dsat | subs[1].ops.dsat};
                 return {count, sat, dsat};
             }
             case Fragment::ANDOR: {
-                const auto count{3 + subs[0]->ops.count + subs[1]->ops.count + subs[2]->ops.count};
-                const auto sat{(subs[1]->ops.sat + subs[0]->ops.sat) | (subs[0]->ops.dsat + subs[2]->ops.sat)};
-                const auto dsat{subs[0]->ops.dsat + subs[2]->ops.dsat};
+                const auto count{3 + subs[0].ops.count + subs[1].ops.count + subs[2].ops.count};
+                const auto sat{(subs[1].ops.sat + subs[0].ops.sat) | (subs[0].ops.dsat + subs[2].ops.sat)};
+                const auto dsat{subs[0].ops.dsat + subs[2].ops.dsat};
                 return {count, sat, dsat};
             }
             case Fragment::MULTI: return {1, (uint32_t)keys.size(), (uint32_t)keys.size()};
             case Fragment::MULTI_A: return {(uint32_t)keys.size() + 1, 0, 0};
             case Fragment::WRAP_S:
             case Fragment::WRAP_C:
-            case Fragment::WRAP_N: return {1 + subs[0]->ops.count, subs[0]->ops.sat, subs[0]->ops.dsat};
-            case Fragment::WRAP_A: return {2 + subs[0]->ops.count, subs[0]->ops.sat, subs[0]->ops.dsat};
-            case Fragment::WRAP_D: return {3 + subs[0]->ops.count, subs[0]->ops.sat, 0};
-            case Fragment::WRAP_J: return {4 + subs[0]->ops.count, subs[0]->ops.sat, 0};
-            case Fragment::WRAP_V: return {subs[0]->ops.count + (subs[0]->GetType() << "x"_mst), subs[0]->ops.sat, {}};
+            case Fragment::WRAP_N: return {1 + subs[0].ops.count, subs[0].ops.sat, subs[0].ops.dsat};
+            case Fragment::WRAP_A: return {2 + subs[0].ops.count, subs[0].ops.sat, subs[0].ops.dsat};
+            case Fragment::WRAP_D: return {3 + subs[0].ops.count, subs[0].ops.sat, 0};
+            case Fragment::WRAP_J: return {4 + subs[0].ops.count, subs[0].ops.sat, 0};
+            case Fragment::WRAP_V: return {subs[0].ops.count + (subs[0].GetType() << "x"_mst), subs[0].ops.sat, {}};
             case Fragment::THRESH: {
                 uint32_t count = 0;
                 auto sats = Vector(internal::MaxInt<uint32_t>(0));
                 for (const auto& sub : subs) {
-                    count += sub->ops.count + 1;
-                    auto next_sats = Vector(sats[0] + sub->ops.dsat);
-                    for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + sub->ops.dsat) | (sats[j - 1] + sub->ops.sat));
-                    next_sats.push_back(sats[sats.size() - 1] + sub->ops.sat);
+                    count += sub.ops.count + 1;
+                    auto next_sats = Vector(sats[0] + sub.ops.dsat);
+                    for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + sub.ops.dsat) | (sats[j - 1] + sub.ops.sat));
+                    next_sats.push_back(sats[sats.size() - 1] + sub.ops.sat);
                     sats = std::move(next_sats);
                 }
                 assert(k < sats.size());
@@ -1033,48 +1029,48 @@ private:
                 {}
             };
             case Fragment::ANDOR: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
-                const auto& z{subs[2]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
+                const auto& z{subs[2].ss};
                 return {
                     (x.sat + SatInfo::If() + y.sat) | (x.dsat + SatInfo::If() + z.sat),
                     x.dsat + SatInfo::If() + z.dsat
                 };
             }
             case Fragment::AND_V: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
                 return {x.sat + y.sat, {}};
             }
             case Fragment::AND_B: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
                 return {x.sat + y.sat + SatInfo::BinaryOp(), x.dsat + y.dsat + SatInfo::BinaryOp()};
             }
             case Fragment::OR_B: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
                 return {
                     ((x.sat + y.dsat) | (x.dsat + y.sat)) + SatInfo::BinaryOp(),
                     x.dsat + y.dsat + SatInfo::BinaryOp()
                 };
             }
             case Fragment::OR_C: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
                 return {(x.sat + SatInfo::If()) | (x.dsat + SatInfo::If() + y.sat), {}};
             }
             case Fragment::OR_D: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
                 return {
                     (x.sat + SatInfo::OP_IFDUP(true) + SatInfo::If()) | (x.dsat + SatInfo::OP_IFDUP(false) + SatInfo::If() + y.sat),
                     x.dsat + SatInfo::OP_IFDUP(false) + SatInfo::If() + y.dsat
                 };
             }
             case Fragment::OR_I: {
-                const auto& x{subs[0]->ss};
-                const auto& y{subs[1]->ss};
+                const auto& x{subs[0].ss};
+                const auto& y{subs[1].ss};
                 return {SatInfo::If() + (x.sat | y.sat), SatInfo::If() + (x.dsat | y.dsat)};
             }
             // multi(k, key1, key2, ..., key_n) starts off with k+1 stack elements (a 0, plus k
@@ -1088,18 +1084,18 @@ private:
             case Fragment::MULTI_A: return {SatInfo(keys.size() - 1, keys.size())};
             case Fragment::WRAP_A:
             case Fragment::WRAP_N:
-            case Fragment::WRAP_S: return subs[0]->ss;
+            case Fragment::WRAP_S: return subs[0].ss;
             case Fragment::WRAP_C: return {
-                subs[0]->ss.sat + SatInfo::OP_CHECKSIG(),
-                subs[0]->ss.dsat + SatInfo::OP_CHECKSIG()
+                subs[0].ss.sat + SatInfo::OP_CHECKSIG(),
+                subs[0].ss.dsat + SatInfo::OP_CHECKSIG()
             };
             case Fragment::WRAP_D: return {
-                SatInfo::OP_DUP() + SatInfo::If() + subs[0]->ss.sat,
+                SatInfo::OP_DUP() + SatInfo::If() + subs[0].ss.sat,
                 SatInfo::OP_DUP() + SatInfo::If()
             };
-            case Fragment::WRAP_V: return {subs[0]->ss.sat + SatInfo::OP_VERIFY(), {}};
+            case Fragment::WRAP_V: return {subs[0].ss.sat + SatInfo::OP_VERIFY(), {}};
             case Fragment::WRAP_J: return {
-                SatInfo::OP_SIZE() + SatInfo::OP_0NOTEQUAL() + SatInfo::If() + subs[0]->ss.sat,
+                SatInfo::OP_SIZE() + SatInfo::OP_0NOTEQUAL() + SatInfo::If() + subs[0].ss.sat,
                 SatInfo::OP_SIZE() + SatInfo::OP_0NOTEQUAL() + SatInfo::If()
             };
             case Fragment::THRESH: {
@@ -1110,13 +1106,13 @@ private:
                     // element i we need to add OP_ADD (if i>0).
                     auto add = i ? SatInfo::BinaryOp() : SatInfo::Empty();
                     // Construct a variable that will become the next sats, starting with index 0.
-                    auto next_sats = Vector(sats[0] + subs[i]->ss.dsat + add);
+                    auto next_sats = Vector(sats[0] + subs[i].ss.dsat + add);
                     // Then loop to construct next_sats[1..i].
                     for (size_t j = 1; j < sats.size(); ++j) {
-                        next_sats.push_back(((sats[j] + subs[i]->ss.dsat) | (sats[j - 1] + subs[i]->ss.sat)) + add);
+                        next_sats.push_back(((sats[j] + subs[i].ss.dsat) | (sats[j - 1] + subs[i].ss.sat)) + add);
                     }
                     // Finally construct next_sats[i+1].
-                    next_sats.push_back(sats[sats.size() - 1] + subs[i]->ss.sat + add);
+                    next_sats.push_back(sats[sats.size() - 1] + subs[i].ss.sat + add);
                     // Switch over.
                     sats = std::move(next_sats);
                 }
@@ -1146,35 +1142,35 @@ private:
             case Fragment::HASH256:
             case Fragment::HASH160: return {1 + 32, {}};
             case Fragment::ANDOR: {
-                const auto sat{(subs[0]->ws.sat + subs[1]->ws.sat) | (subs[0]->ws.dsat + subs[2]->ws.sat)};
-                const auto dsat{subs[0]->ws.dsat + subs[2]->ws.dsat};
+                const auto sat{(subs[0].ws.sat + subs[1].ws.sat) | (subs[0].ws.dsat + subs[2].ws.sat)};
+                const auto dsat{subs[0].ws.dsat + subs[2].ws.dsat};
                 return {sat, dsat};
             }
-            case Fragment::AND_V: return {subs[0]->ws.sat + subs[1]->ws.sat, {}};
-            case Fragment::AND_B: return {subs[0]->ws.sat + subs[1]->ws.sat, subs[0]->ws.dsat + subs[1]->ws.dsat};
+            case Fragment::AND_V: return {subs[0].ws.sat + subs[1].ws.sat, {}};
+            case Fragment::AND_B: return {subs[0].ws.sat + subs[1].ws.sat, subs[0].ws.dsat + subs[1].ws.dsat};
             case Fragment::OR_B: {
-                const auto sat{(subs[0]->ws.dsat + subs[1]->ws.sat) | (subs[0]->ws.sat + subs[1]->ws.dsat)};
-                const auto dsat{subs[0]->ws.dsat + subs[1]->ws.dsat};
+                const auto sat{(subs[0].ws.dsat + subs[1].ws.sat) | (subs[0].ws.sat + subs[1].ws.dsat)};
+                const auto dsat{subs[0].ws.dsat + subs[1].ws.dsat};
                 return {sat, dsat};
             }
-            case Fragment::OR_C: return {subs[0]->ws.sat | (subs[0]->ws.dsat + subs[1]->ws.sat), {}};
-            case Fragment::OR_D: return {subs[0]->ws.sat | (subs[0]->ws.dsat + subs[1]->ws.sat), subs[0]->ws.dsat + subs[1]->ws.dsat};
-            case Fragment::OR_I: return {(subs[0]->ws.sat + 1 + 1) | (subs[1]->ws.sat + 1), (subs[0]->ws.dsat + 1 + 1) | (subs[1]->ws.dsat + 1)};
+            case Fragment::OR_C: return {subs[0].ws.sat | (subs[0].ws.dsat + subs[1].ws.sat), {}};
+            case Fragment::OR_D: return {subs[0].ws.sat | (subs[0].ws.dsat + subs[1].ws.sat), subs[0].ws.dsat + subs[1].ws.dsat};
+            case Fragment::OR_I: return {(subs[0].ws.sat + 1 + 1) | (subs[1].ws.sat + 1), (subs[0].ws.dsat + 1 + 1) | (subs[1].ws.dsat + 1)};
             case Fragment::MULTI: return {k * sig_size + 1, k + 1};
             case Fragment::MULTI_A: return {k * sig_size + static_cast<uint32_t>(keys.size()) - k, static_cast<uint32_t>(keys.size())};
             case Fragment::WRAP_A:
             case Fragment::WRAP_N:
             case Fragment::WRAP_S:
-            case Fragment::WRAP_C: return subs[0]->ws;
-            case Fragment::WRAP_D: return {1 + 1 + subs[0]->ws.sat, 1};
-            case Fragment::WRAP_V: return {subs[0]->ws.sat, {}};
-            case Fragment::WRAP_J: return {subs[0]->ws.sat, 1};
+            case Fragment::WRAP_C: return subs[0].ws;
+            case Fragment::WRAP_D: return {1 + 1 + subs[0].ws.sat, 1};
+            case Fragment::WRAP_V: return {subs[0].ws.sat, {}};
+            case Fragment::WRAP_J: return {subs[0].ws.sat, 1};
             case Fragment::THRESH: {
                 auto sats = Vector(internal::MaxInt<uint32_t>(0));
                 for (const auto& sub : subs) {
-                    auto next_sats = Vector(sats[0] + sub->ws.dsat);
-                    for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + sub->ws.dsat) | (sats[j - 1] + sub->ws.sat));
-                    next_sats.push_back(sats[sats.size() - 1] + sub->ws.sat);
+                    auto next_sats = Vector(sats[0] + sub.ws.dsat);
+                    for (size_t j = 1; j < sats.size(); ++j) next_sats.push_back((sats[j] + sub.ws.dsat) | (sats[j - 1] + sub.ws.sat));
+                    next_sats.push_back(sats[sats.size() - 1] + sub.ws.sat);
                     sats = std::move(next_sats);
                 }
                 assert(k < sats.size());
@@ -1658,29 +1654,29 @@ 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<unsigned char> arg, uint32_t val = 0)
+    Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<Node<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)
         : fragment(nt), k(val), data(std::move(arg)), 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<Key> key, uint32_t val = 0)
+    Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<Node<Key>> sub, std::vector<Key> key, uint32_t val = 0)
         : fragment(nt), k(val), keys(std::move(key)), m_script_ctx{script_ctx}, subs(std::move(sub)), ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
     Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<Key> key, uint32_t val = 0)
         : fragment(nt), k(val), keys(std::move(key)), 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, uint32_t val = 0)
+    Node(internal::NoDupCheck, MiniscriptContext script_ctx, Fragment nt, std::vector<Node<Key>> sub, uint32_t val = 0)
         : fragment(nt), k(val), 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, uint32_t val = 0)
         : fragment(nt), k(val), m_script_ctx{script_ctx}, ops(CalcOps()), ss(CalcStackSize()), ws(CalcWitnessSize()), typ(CalcType()), scriptlen(CalcScriptLen()) {}
 
     // Constructors with various argument combinations, which do perform the duplicate key check.
-    template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<NodeRef<Key>> sub, std::vector<unsigned char> arg, uint32_t val = 0)
+    template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<Node<Key>> sub, std::vector<unsigned char> arg, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(sub), std::move(arg), val) { DuplicateKeyCheck(ctx); }
     template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<unsigned char> arg, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(arg), val) { DuplicateKeyCheck(ctx);}
-    template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<NodeRef<Key>> sub, std::vector<Key> key, uint32_t val = 0)
+    template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<Node<Key>> sub, std::vector<Key> key, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(sub), std::move(key), val) { DuplicateKeyCheck(ctx); }
     template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<Key> key, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(key), val) { DuplicateKeyCheck(ctx); }
-    template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<NodeRef<Key>> sub, uint32_t val = 0)
+    template <typename Ctx> Node(const Ctx& ctx, Fragment nt, std::vector<Node<Key>> sub, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, std::move(sub), val) { DuplicateKeyCheck(ctx); }
     template <typename Ctx> Node(const Ctx& ctx, Fragment nt, uint32_t val = 0)
         : Node(internal::NoDupCheck{}, ctx.MsContext(), nt, val) { DuplicateKeyCheck(ctx); }
@@ -1688,6 +1684,10 @@ public:
     // Delete copy constructor and assignment operator, use Clone() instead
     Node(const Node&) = delete;
     Node& operator=(const Node&) = delete;
+
+    // subs is movable, circumventing recursion, so these are permitted.
+    Node(Node&&) = default;
+    Node& operator=(Node&&) = default;
 };
 
 namespace internal {
@@ -1775,14 +1775,14 @@ std::optional<std::pair<std::vector<unsigned char>, int>> ParseHexStrEnd(std::sp
 
 /** BuildBack pops the last two elements off `constructed` and wraps them in the specified Fragment */
 template<typename Key>
-void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<NodeRef<Key>>& constructed, const bool reverse = false)
+void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<Node<Key>>& constructed, const bool reverse = false)
 {
-    NodeRef<Key> child = std::move(constructed.back());
+    Node<Key> child{std::move(constructed.back())};
     constructed.pop_back();
     if (reverse) {
-        constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, script_ctx, nt, Vector(std::move(child), std::move(constructed.back())));
+        constructed.back() = Node<Key>{internal::NoDupCheck{}, script_ctx, nt, Vector(std::move(child), std::move(constructed.back()))};
     } else {
-        constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, script_ctx, nt, Vector(std::move(constructed.back()), std::move(child)));
+        constructed.back() = Node<Key>{internal::NoDupCheck{}, script_ctx, nt, Vector(std::move(constructed.back()), std::move(child))};
     }
 }
 
@@ -1792,7 +1792,7 @@ void BuildBack(const MiniscriptContext script_ctx, Fragment nt, std::vector<Node
  * the `IsValidTopLevel()` and `IsSaneTopLevel()` to check for these properties on the node.
  */
 template<typename Key, typename Ctx>
-inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
+inline std::optional<Node<Key>> Parse(std::span<const char> in, const Ctx& ctx)
 {
     using namespace script;
 
@@ -1811,7 +1811,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
 
     // The two integers are used to hold state for thresh()
     std::vector<std::tuple<ParseContext, int64_t, int64_t>> to_parse;
-    std::vector<NodeRef<Key>> constructed;
+    std::vector<Node<Key>> constructed;
 
     to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1);
 
@@ -1843,10 +1843,10 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
         if (is_multi_a) {
             // (push + xonly-key + CHECKSIG[ADD]) * n + k + OP_NUMEQUAL(VERIFY), minus one.
             script_size += (1 + 32 + 1) * keys.size() + BuildScript(k).size();
-            constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI_A, std::move(keys), k));
+            constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI_A, std::move(keys), k);
         } else {
             script_size += 2 + (keys.size() > 16) + (k > 16) + 34 * keys.size();
-            constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI, std::move(keys), k));
+            constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI, std::move(keys), k);
         }
         return true;
     };
@@ -1904,7 +1904,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
                 } else if (in[j] == 'l') {
                     // The l: wrapper is equivalent to or_i(0,X)
                     script_size += 4;
-                    constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0));
+                    constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0);
                     to_parse.emplace_back(ParseContext::OR_I, -1, -1);
                 } else {
                     return {};
@@ -1917,63 +1917,63 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
         }
         case ParseContext::EXPR: {
             if (Const("0", in)) {
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0);
             } else if (Const("1", in)) {
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1);
             } else if (Const("pk(", in)) {
                 auto res = ParseKeyEnd<Key, Ctx>(in, ctx);
                 if (!res) return {};
                 auto& [key, key_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key))))));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key)))));
                 in = in.subspan(key_size + 1);
                 script_size += IsTapscript(ctx.MsContext()) ? 33 : 34;
             } else if (Const("pkh(", in)) {
                 auto res = ParseKeyEnd<Key>(in, ctx);
                 if (!res) return {};
                 auto& [key, key_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key))))));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key)))));
                 in = in.subspan(key_size + 1);
                 script_size += 24;
             } else if (Const("pk_k(", in)) {
                 auto res = ParseKeyEnd<Key>(in, ctx);
                 if (!res) return {};
                 auto& [key, key_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key))));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(key)));
                 in = in.subspan(key_size + 1);
                 script_size += IsTapscript(ctx.MsContext()) ? 32 : 33;
             } else if (Const("pk_h(", in)) {
                 auto res = ParseKeyEnd<Key>(in, ctx);
                 if (!res) return {};
                 auto& [key, key_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key))));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(key)));
                 in = in.subspan(key_size + 1);
                 script_size += 23;
             } else if (Const("sha256(", in)) {
                 auto res = ParseHexStrEnd(in, 32, ctx);
                 if (!res) return {};
                 auto& [hash, hash_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(hash)));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, std::move(hash));
                 in = in.subspan(hash_size + 1);
                 script_size += 38;
             } else if (Const("ripemd160(", in)) {
                 auto res = ParseHexStrEnd(in, 20, ctx);
                 if (!res) return {};
                 auto& [hash, hash_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(hash)));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, std::move(hash));
                 in = in.subspan(hash_size + 1);
                 script_size += 26;
             } else if (Const("hash256(", in)) {
                 auto res = ParseHexStrEnd(in, 32, ctx);
                 if (!res) return {};
                 auto& [hash, hash_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(hash)));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, std::move(hash));
                 in = in.subspan(hash_size + 1);
                 script_size += 38;
             } else if (Const("hash160(", in)) {
                 auto res = ParseHexStrEnd(in, 20, ctx);
                 if (!res) return {};
                 auto& [hash, hash_size] = *res;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(hash)));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, std::move(hash));
                 in = in.subspan(hash_size + 1);
                 script_size += 26;
             } else if (Const("after(", in)) {
@@ -1981,7 +1981,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
                 if (arg_size < 1) return {};
                 const auto num{ToIntegral<int64_t>(std::string_view(in.data(), arg_size))};
                 if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num);
                 in = in.subspan(arg_size + 1);
                 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
             } else if (Const("older(", in)) {
@@ -1989,7 +1989,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
                 if (arg_size < 1) return {};
                 const auto num{ToIntegral<int64_t>(std::string_view(in.data(), arg_size))};
                 if (!num.has_value() || *num < 1 || *num >= 0x80000000L) return {};
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num);
                 in = in.subspan(arg_size + 1);
                 script_size += 1 + (*num > 16) + (*num > 0x7f) + (*num > 0x7fff) + (*num > 0x7fffff);
             } else if (Const("multi(", in)) {
@@ -2048,40 +2048,40 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
             break;
         }
         case ParseContext::ALT: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_A, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_A, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::SWAP: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_S, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_S, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::CHECK: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::DUP_IF: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_D, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_D, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::NON_ZERO: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_J, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_J, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::ZERO_NOTEQUAL: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_N, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_N, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::VERIFY: {
-            script_size += (constructed.back()->GetType() << "x"_mst);
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_V, Vector(std::move(constructed.back())));
+            script_size += (constructed.back().GetType() << "x"_mst);
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_V, Vector(std::move(constructed.back()))};
             break;
         }
         case ParseContext::WRAP_U: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OR_I, Vector(std::move(constructed.back()), MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0)));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::OR_I, Vector(std::move(constructed.back()), Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0))};
             break;
         }
         case ParseContext::WRAP_T: {
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AND_V, Vector(std::move(constructed.back()), MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1)));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::AND_V, Vector(std::move(constructed.back()), Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1))};
             break;
         }
         case ParseContext::AND_B: {
@@ -2091,7 +2091,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
         case ParseContext::AND_N: {
             auto mid = std::move(constructed.back());
             constructed.pop_back();
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0)));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), Node<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0))};
             break;
         }
         case ParseContext::AND_V: {
@@ -2119,7 +2119,7 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
             constructed.pop_back();
             auto mid = std::move(constructed.back());
             constructed.pop_back();
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), std::move(right)));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::ANDOR, Vector(std::move(constructed.back()), std::move(mid), std::move(right))};
             break;
         }
         case ParseContext::THRESH: {
@@ -2133,13 +2133,13 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
                 if (k > n) return {};
                 in = in.subspan(1);
                 // Children are constructed in reverse order, so iterate from end to beginning
-                std::vector<NodeRef<Key>> subs;
+                std::vector<Node<Key>> subs;
                 for (int i = 0; i < n; ++i) {
                     subs.push_back(std::move(constructed.back()));
                     constructed.pop_back();
                 }
                 std::reverse(subs.begin(), subs.end());
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::THRESH, std::move(subs), k));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::THRESH, std::move(subs), k);
             } else {
                 return {};
             }
@@ -2161,10 +2161,10 @@ inline NodeRef<Key> Parse(std::span<const char> in, const Ctx& ctx)
     // Sanity checks on the produced miniscript
     assert(constructed.size() >= 1);
     CHECK_NONFATAL(constructed.size() == 1);
-    assert(constructed[0]->ScriptSize() == script_size);
+    assert(constructed[0].ScriptSize() == script_size);
     if (in.size() > 0) return {};
-    NodeRef<Key> tl_node = std::move(constructed.front());
-    tl_node->DuplicateKeyCheck(ctx);
+    Node<Key> tl_node{std::move(constructed.front())};
+    tl_node.DuplicateKeyCheck(ctx);
     return tl_node;
 }
 
@@ -2251,11 +2251,11 @@ enum class DecodeContext {
 
 //! Parse a miniscript from a bitcoin script
 template<typename Key, typename Ctx, typename I>
-inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
+inline std::optional<Node<Key>> DecodeScript(I& in, I last, const Ctx& ctx)
 {
     // The two integers are used to hold state for thresh()
     std::vector<std::tuple<DecodeContext, int64_t, int64_t>> to_parse;
-    std::vector<NodeRef<Key>> constructed;
+    std::vector<Node<Key>> constructed;
 
     // This is the top level, so we assume the type is B
     // (in particular, disallowing top level W expressions)
@@ -2263,7 +2263,7 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
 
     while (!to_parse.empty()) {
         // Exit early if the Miniscript is not going to be valid.
-        if (!constructed.empty() && !constructed.back()->IsValid()) return {};
+        if (!constructed.empty() && !constructed.back().IsValid()) return {};
 
         // Get the current context we are decoding within
         auto [cur_context, n, k] = to_parse.back();
@@ -2276,12 +2276,12 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
             // Constants
             if (in[0].first == OP_1) {
                 ++in;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_1);
                 break;
             }
             if (in[0].first == OP_0) {
                 ++in;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::JUST_0);
                 break;
             }
             // Public keys
@@ -2289,14 +2289,14 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
                 auto key = ctx.FromPKBytes(in[0].second.begin(), in[0].second.end());
                 if (!key) return {};
                 ++in;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(*key))));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_K, Vector(std::move(*key)));
                 break;
             }
             if (last - in >= 5 && in[0].first == OP_VERIFY && in[1].first == OP_EQUAL && in[3].first == OP_HASH160 && in[4].first == OP_DUP && in[2].second.size() == 20) {
                 auto key = ctx.FromPKHBytes(in[2].second.begin(), in[2].second.end());
                 if (!key) return {};
                 in += 5;
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key))));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::PK_H, Vector(std::move(*key)));
                 break;
             }
             // Time locks
@@ -2304,31 +2304,31 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
             if (last - in >= 2 && in[0].first == OP_CHECKSEQUENCEVERIFY && (num = ParseScriptNumber(in[1]))) {
                 in += 2;
                 if (*num < 1 || *num > 0x7FFFFFFFL) return {};
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::OLDER, *num);
                 break;
             }
             if (last - in >= 2 && in[0].first == OP_CHECKLOCKTIMEVERIFY && (num = ParseScriptNumber(in[1]))) {
                 in += 2;
                 if (num < 1 || num > 0x7FFFFFFFL) return {};
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::AFTER, *num);
                 break;
             }
             // Hashes
             if (last - in >= 7 && in[0].first == OP_EQUAL && in[3].first == OP_VERIFY && in[4].first == OP_EQUAL && (num = ParseScriptNumber(in[5])) && num == 32 && in[6].first == OP_SIZE) {
                 if (in[2].first == OP_SHA256 && in[1].second.size() == 32) {
-                    constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, in[1].second));
+                    constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::SHA256, in[1].second);
                     in += 7;
                     break;
                 } else if (in[2].first == OP_RIPEMD160 && in[1].second.size() == 20) {
-                    constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, in[1].second));
+                    constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::RIPEMD160, in[1].second);
                     in += 7;
                     break;
                 } else if (in[2].first == OP_HASH256 && in[1].second.size() == 32) {
-                    constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, in[1].second));
+                    constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH256, in[1].second);
                     in += 7;
                     break;
                 } else if (in[2].first == OP_HASH160 && in[1].second.size() == 20) {
-                    constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, in[1].second));
+                    constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::HASH160, in[1].second);
                     in += 7;
                     break;
                 }
@@ -2350,7 +2350,7 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
                 if (!k || *k < 1 || *k > *n) return {};
                 in += 3 + *n;
                 std::reverse(keys.begin(), keys.end());
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI, std::move(keys), *k));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI, std::move(keys), *k);
                 break;
             }
             // Tapscript's equivalent of multi
@@ -2380,7 +2380,7 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
                 if (keys.size() < (size_t)*k) return {};
                 in += 2 + keys.size() * 2;
                 std::reverse(keys.begin(), keys.end());
-                constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI_A, std::move(keys), *k));
+                constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::MULTI_A, std::move(keys), *k);
                 break;
             }
             /** In the following wrappers, we only need to push SINGLE_BKV_EXPR rather
@@ -2475,38 +2475,38 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
         case DecodeContext::SWAP: {
             if (in >= last || in[0].first != OP_SWAP || constructed.empty()) return {};
             ++in;
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_S, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_S, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::ALT: {
             if (in >= last || in[0].first != OP_TOALTSTACK || constructed.empty()) return {};
             ++in;
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_A, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_A, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::CHECK: {
             if (constructed.empty()) return {};
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_C, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::DUP_IF: {
             if (constructed.empty()) return {};
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_D, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_D, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::VERIFY: {
             if (constructed.empty()) return {};
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_V, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_V, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::NON_ZERO: {
             if (constructed.empty()) return {};
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_J, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_J, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::ZERO_NOTEQUAL: {
             if (constructed.empty()) return {};
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_N, Vector(std::move(constructed.back())));
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::WRAP_N, Vector(std::move(constructed.back()))};
             break;
         }
         case DecodeContext::AND_V: {
@@ -2536,12 +2536,12 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
         }
         case DecodeContext::ANDOR: {
             if (constructed.size() < 3) return {};
-            NodeRef<Key> left = std::move(constructed.back());
+            Node left{std::move(constructed.back())};
             constructed.pop_back();
-            NodeRef<Key> right = std::move(constructed.back());
+            Node right{std::move(constructed.back())};
             constructed.pop_back();
-            NodeRef<Key> mid = std::move(constructed.back());
-            constructed.back() = MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::ANDOR, Vector(std::move(left), std::move(mid), std::move(right)));
+            Node mid{std::move(constructed.back())};
+            constructed.back() = Node{internal::NoDupCheck{}, ctx.MsContext(), Fragment::ANDOR, Vector(std::move(left), std::move(mid), std::move(right))};
             break;
         }
         case DecodeContext::THRESH_W: {
@@ -2559,13 +2559,13 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
         }
         case DecodeContext::THRESH_E: {
             if (k < 1 || k > n || constructed.size() < static_cast<size_t>(n)) return {};
-            std::vector<NodeRef<Key>> subs;
+            std::vector<Node<Key>> subs;
             for (int i = 0; i < n; ++i) {
-                NodeRef<Key> sub = std::move(constructed.back());
+                Node sub{std::move(constructed.back())};
                 constructed.pop_back();
                 subs.push_back(std::move(sub));
             }
-            constructed.push_back(MakeNodeRef<Key>(internal::NoDupCheck{}, ctx.MsContext(), Fragment::THRESH, std::move(subs), k));
+            constructed.emplace_back(internal::NoDupCheck{}, ctx.MsContext(), Fragment::THRESH, std::move(subs), k);
             break;
         }
         case DecodeContext::ENDIF: {
@@ -2629,23 +2629,25 @@ inline NodeRef<Key> DecodeScript(I& in, I last, const Ctx& ctx)
         }
     }
     if (constructed.size() != 1) return {};
-    NodeRef<Key> tl_node = std::move(constructed.front());
-    tl_node->DuplicateKeyCheck(ctx);
+    Node tl_node{std::move(constructed.front())};
+    tl_node.DuplicateKeyCheck(ctx);
     // Note that due to how ComputeType works (only assign the type to the node if the
     // subs' types are valid) this would fail if any node of tree is badly typed.
-    if (!tl_node->IsValidTopLevel()) return {};
+    if (!tl_node.IsValidTopLevel()) return {};
     return tl_node;
 }
 
 } // namespace internal
 
 template<typename Ctx>
-inline NodeRef<typename Ctx::Key> FromString(const std::string& str, const Ctx& ctx) {
+inline std::optional<Node<typename Ctx::Key>> FromString(const std::string& str, const Ctx& ctx)
+{
     return internal::Parse<typename Ctx::Key>(str, ctx);
 }
 
 template<typename Ctx>
-inline NodeRef<typename Ctx::Key> FromScript(const CScript& script, const Ctx& ctx) {
+inline std::optional<Node<typename Ctx::Key>> FromScript(const CScript& script, const Ctx& ctx)
+{
     using namespace internal;
     // A too large Script is necessarily invalid, don't bother parsing it.
     if (script.size() > MaxScriptSize(ctx.MsContext())) return {};

src/test/fuzz/miniscript.cpp

@@ -14,11 +14,11 @@
 #include <util/strencodings.h>
 
 #include <algorithm>
+#include <optional>
 
 namespace {
 
 using Fragment = miniscript::Fragment;
-using NodeRef = miniscript::NodeRef<CPubKey>;
 using Node = miniscript::Node<CPubKey>;
 using Type = miniscript::Type;
 using MsCtx = miniscript::MiniscriptContext;
@@ -311,11 +311,6 @@ const struct KeyComparator {
 // A dummy scriptsig to pass to VerifyScript (we always use Segwit v0).
 const CScript DUMMY_SCRIPTSIG;
 
-//! Construct a miniscript node as a shared_ptr.
-template<typename... Args> NodeRef MakeNodeRef(Args&&... args) {
-    return miniscript::MakeNodeRef<CPubKey>(miniscript::internal::NoDupCheck{}, std::forward<Args>(args)...);
-}
-
 /** Information about a yet to be constructed Miniscript node. */
 struct NodeInfo {
     //! The type of this node
@@ -851,14 +846,14 @@ std::optional<NodeInfo> ConsumeNodeSmart(MsCtx script_ctx, FuzzedDataProvider& p
  * Generate a Miniscript node based on the fuzzer's input.
  *
  * - ConsumeNode is a function object taking a Type, and returning an std::optional<NodeInfo>.
- * - root_type is the required type properties of the constructed NodeRef.
+ * - root_type is the required type properties of the constructed Node.
  * - strict_valid sets whether ConsumeNode is expected to guarantee a NodeInfo that results in
- *   a NodeRef whose Type() matches the type fed to ConsumeNode.
+ *   a Node whose Type() matches the type fed to ConsumeNode.
  */
 template<typename F>
-NodeRef GenNode(MsCtx script_ctx, F ConsumeNode, Type root_type, bool strict_valid = false) {
+std::optional<Node> GenNode(MsCtx script_ctx, F ConsumeNode, Type root_type, bool strict_valid = false) {
     /** A stack of miniscript Nodes being built up. */
-    std::vector<NodeRef> stack;
+    std::vector<Node> stack;
     /** The queue of instructions. */
     std::vector<std::pair<Type, std::optional<NodeInfo>>> todo{{root_type, {}}};
     /** Predict the number of (static) script ops. */
@@ -961,24 +956,24 @@ NodeRef GenNode(MsCtx script_ctx, F ConsumeNode, Type root_type, bool strict_val
         } else {
             // The back of todo has fragment and number of children decided, and
             // those children have been constructed at the back of stack. Pop
-            // that entry off todo, and use it to construct a new NodeRef on
+            // that entry off todo, and use it to construct a new Node on
             // stack.
             NodeInfo& info = *todo.back().second;
             // Gather children from the back of stack.
-            std::vector<NodeRef> sub;
+            std::vector<Node> sub;
             sub.reserve(info.subtypes.size());
             for (size_t i = 0; i < info.subtypes.size(); ++i) {
                 sub.push_back(std::move(*(stack.end() - info.subtypes.size() + i)));
             }
             stack.erase(stack.end() - info.subtypes.size(), stack.end());
-            // Construct new NodeRef.
-            NodeRef node;
+            // Construct new Node.
+            std::optional<Node> node;
             if (info.keys.empty()) {
-                node = MakeNodeRef(script_ctx, info.fragment, std::move(sub), std::move(info.hash), info.k);
+                node = Node{miniscript::internal::NoDupCheck{}, script_ctx, info.fragment, std::move(sub), std::move(info.hash), info.k};
             } else {
                 assert(sub.empty());
                 assert(info.hash.empty());
-                node = MakeNodeRef(script_ctx, info.fragment, std::move(info.keys), info.k);
+                node = Node{miniscript::internal::NoDupCheck{}, script_ctx, info.fragment, std::move(info.keys), info.k};
             }
             // Verify acceptability.
             if (!node || (node->GetType() & "KVWB"_mst) == ""_mst) {
@@ -990,7 +985,7 @@ NodeRef GenNode(MsCtx script_ctx, F ConsumeNode, Type root_type, bool strict_val
             }
             if (!node->IsValid()) return {};
             // Update resource predictions.
-            if (node->fragment == Fragment::WRAP_V && node->subs[0]->GetType() << "x"_mst) {
+            if (node->fragment == Fragment::WRAP_V && node->subs[0].GetType() << "x"_mst) {
                 ops += 1;
                 scriptsize += 1;
             }
@@ -999,14 +994,14 @@ NodeRef GenNode(MsCtx script_ctx, F ConsumeNode, Type root_type, bool strict_val
                 return {};
             }
             // Move it to the stack.
-            stack.push_back(std::move(node));
+            stack.push_back(std::move(*node));
             todo.pop_back();
         }
     }
     assert(stack.size() == 1);
-    assert(stack[0]->GetStaticOps() == ops);
-    assert(stack[0]->ScriptSize() == scriptsize);
-    stack[0]->DuplicateKeyCheck(KEY_COMP);
+    assert(stack[0].GetStaticOps() == ops);
+    assert(stack[0].ScriptSize() == scriptsize);
+    stack[0].DuplicateKeyCheck(KEY_COMP);
     return std::move(stack[0]);
 }
 
@@ -1031,7 +1026,7 @@ void SatisfactionToWitness(MsCtx ctx, CScriptWitness& witness, const CScript& sc
 }
 
 /** Perform various applicable tests on a miniscript Node. */
-void TestNode(const MsCtx script_ctx, const NodeRef& node, FuzzedDataProvider& provider)
+void TestNode(const MsCtx script_ctx, const std::optional<Node> node, FuzzedDataProvider& provider)
 {
     if (!node) return;
 

src/test/miniscript_tests.cpp

@@ -292,31 +292,31 @@ public:
 };
 
 using Fragment = miniscript::Fragment;
-using NodeRef = miniscript::NodeRef<CPubKey>;
 using miniscript::operator""_mst;
 using Node = miniscript::Node<CPubKey>;
 
 /** Compute all challenges (pubkeys, hashes, timelocks) that occur in a given Miniscript. */
 // NOLINTNEXTLINE(misc-no-recursion)
-std::set<Challenge> FindChallenges(const NodeRef& ref) {
+std::set<Challenge> FindChallenges(const Node& node)
+{
     std::set<Challenge> chal;
-    for (const auto& key : ref->keys) {
+    for (const auto& key : node.keys) {
         chal.emplace(ChallengeType::PK, ChallengeNumber(key));
     }
-    if (ref->fragment == miniscript::Fragment::OLDER) {
-        chal.emplace(ChallengeType::OLDER, ref->k);
-    } else if (ref->fragment == miniscript::Fragment::AFTER) {
-        chal.emplace(ChallengeType::AFTER, ref->k);
-    } else if (ref->fragment == miniscript::Fragment::SHA256) {
-        chal.emplace(ChallengeType::SHA256, ChallengeNumber(ref->data));
-    } else if (ref->fragment == miniscript::Fragment::RIPEMD160) {
-        chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(ref->data));
-    } else if (ref->fragment == miniscript::Fragment::HASH256) {
-        chal.emplace(ChallengeType::HASH256, ChallengeNumber(ref->data));
-    } else if (ref->fragment == miniscript::Fragment::HASH160) {
-        chal.emplace(ChallengeType::HASH160, ChallengeNumber(ref->data));
+    if (node.fragment == miniscript::Fragment::OLDER) {
+        chal.emplace(ChallengeType::OLDER, node.k);
+    } else if (node.fragment == miniscript::Fragment::AFTER) {
+        chal.emplace(ChallengeType::AFTER, node.k);
+    } else if (node.fragment == miniscript::Fragment::SHA256) {
+        chal.emplace(ChallengeType::SHA256, ChallengeNumber(node.data));
+    } else if (node.fragment == miniscript::Fragment::RIPEMD160) {
+        chal.emplace(ChallengeType::RIPEMD160, ChallengeNumber(node.data));
+    } else if (node.fragment == miniscript::Fragment::HASH256) {
+        chal.emplace(ChallengeType::HASH256, ChallengeNumber(node.data));
+    } else if (node.fragment == miniscript::Fragment::HASH160) {
+        chal.emplace(ChallengeType::HASH160, ChallengeNumber(node.data));
     }
-    for (const auto& sub : ref->subs) {
+    for (const auto& sub : node.subs) {
         auto sub_chal = FindChallenges(sub);
         chal.insert(sub_chal.begin(), sub_chal.end());
     }
@@ -345,8 +345,9 @@ void SatisfactionToWitness(miniscript::MiniscriptContext ctx, CScriptWitness& wi
 
 struct MiniScriptTest : BasicTestingSetup {
 /** Run random satisfaction tests. */
-void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const NodeRef& node) {
-    auto script = node->ToScript(converter);
+void TestSatisfy(const KeyConverter& converter, const std::string& testcase, const Node& node)
+{
+    auto script = node.ToScript(converter);
     auto challenges = FindChallenges(node); // Find all challenges in the generated miniscript.
     std::vector<Challenge> challist(challenges.begin(), challenges.end());
     for (int iter = 0; iter < 3; ++iter) {
@@ -364,12 +365,12 @@ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, con
 
             // Run malleable satisfaction algorithm.
             CScriptWitness witness_mal;
-            const bool mal_success = node->Satisfy(satisfier, witness_mal.stack, false) == miniscript::Availability::YES;
+            const bool mal_success = node.Satisfy(satisfier, witness_mal.stack, false) == miniscript::Availability::YES;
             SatisfactionToWitness(converter.MsContext(), witness_mal, script, builder);
 
             // Run non-malleable satisfaction algorithm.
             CScriptWitness witness_nonmal;
-            const bool nonmal_success = node->Satisfy(satisfier, witness_nonmal.stack, true) == miniscript::Availability::YES;
+            const bool nonmal_success = node.Satisfy(satisfier, witness_nonmal.stack, true) == miniscript::Availability::YES;
             // Compute witness size (excluding script push, control block, and witness count encoding).
             const size_t wit_size = GetSerializeSize(witness_nonmal.stack) - GetSizeOfCompactSize(witness_nonmal.stack.size());
             SatisfactionToWitness(converter.MsContext(), witness_nonmal, script, builder);
@@ -378,23 +379,23 @@ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, con
                 // Non-malleable satisfactions are bounded by the satisfaction size plus:
                 // - For P2WSH spends, the witness script
                 // - For Tapscript spends, both the witness script and the control block
-                const size_t max_stack_size{*node->GetStackSize() + 1 + miniscript::IsTapscript(converter.MsContext())};
+                const size_t max_stack_size{*node.GetStackSize() + 1 + miniscript::IsTapscript(converter.MsContext())};
                 BOOST_CHECK(witness_nonmal.stack.size() <= max_stack_size);
                 // If a non-malleable satisfaction exists, the malleable one must also exist, and be identical to it.
                 BOOST_CHECK(mal_success);
                 BOOST_CHECK(witness_nonmal.stack == witness_mal.stack);
-                assert(wit_size <= *node->GetWitnessSize());
+                assert(wit_size <= *node.GetWitnessSize());
 
                 // Test non-malleable satisfaction.
                 ScriptError serror;
                 bool res = VerifyScript(CScript(), script_pubkey, &witness_nonmal, STANDARD_SCRIPT_VERIFY_FLAGS, checker, &serror);
                 // Non-malleable satisfactions are guaranteed to be valid if ValidSatisfactions().
-                if (node->ValidSatisfactions()) BOOST_CHECK(res);
+                if (node.ValidSatisfactions()) BOOST_CHECK(res);
                 // More detailed: non-malleable satisfactions must be valid, or could fail with ops count error (if CheckOpsLimit failed),
                 // or with a stack size error (if CheckStackSize check fails).
                 BOOST_CHECK(res ||
-                            (!node->CheckOpsLimit() && serror == ScriptError::SCRIPT_ERR_OP_COUNT) ||
-                            (!node->CheckStackSize() && serror == ScriptError::SCRIPT_ERR_STACK_SIZE));
+                            (!node.CheckOpsLimit() && serror == ScriptError::SCRIPT_ERR_OP_COUNT) ||
+                            (!node.CheckStackSize() && serror == ScriptError::SCRIPT_ERR_STACK_SIZE));
             }
 
             if (mal_success && (!nonmal_success || witness_mal.stack != witness_nonmal.stack)) {
@@ -406,7 +407,7 @@ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, con
                 BOOST_CHECK(res || serror == ScriptError::SCRIPT_ERR_OP_COUNT || serror == ScriptError::SCRIPT_ERR_STACK_SIZE);
             }
 
-            if (node->IsSane()) {
+            if (node.IsSane()) {
                 // For sane nodes, the two algorithms behave identically.
                 BOOST_CHECK_EQUAL(mal_success, nonmal_success);
             }
@@ -416,7 +417,7 @@ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, con
             // For nonmalleable solutions this is only true if the added condition is PK;
             // for other conditions, adding one may make an valid satisfaction become malleable. If the script
             // is sane, this cannot happen however.
-            if (node->IsSane() || add < 0 || challist[add].first == ChallengeType::PK) {
+            if (node.IsSane() || add < 0 || challist[add].first == ChallengeType::PK) {
                 BOOST_CHECK(nonmal_success >= prev_nonmal_success);
             }
             // Remember results for the next added challenge.
@@ -424,11 +425,11 @@ void TestSatisfy(const KeyConverter& converter, const std::string& testcase, con
             prev_nonmal_success = nonmal_success;
         }
 
-        bool satisfiable = node->IsSatisfiable([](const Node&) { return true; });
+        bool satisfiable = node.IsSatisfiable([](const Node&) { return true; });
         // If the miniscript was satisfiable at all, a satisfaction must be found after all conditions are added.
         BOOST_CHECK_EQUAL(prev_mal_success, satisfiable);
         // If the miniscript is sane and satisfiable, a nonmalleable satisfaction must eventually be found.
-        if (node->IsSane()) BOOST_CHECK_EQUAL(prev_nonmal_success, satisfiable);
+        if (node.IsSane()) BOOST_CHECK_EQUAL(prev_nonmal_success, satisfiable);
     }
 }
 
@@ -471,7 +472,7 @@ void Test(const std::string& ms, const std::string& hexscript, int mode, const K
         if (stacklimit != -1) BOOST_CHECK_MESSAGE((int)*node->GetStackSize() == stacklimit, "Stack limit mismatch: " << ms << " (" << *node->GetStackSize() << " vs " << stacklimit << ")");
         if (max_wit_size) BOOST_CHECK_MESSAGE(*node->GetWitnessSize() == *max_wit_size, "Witness size limit mismatch: " << ms << " (" << *node->GetWitnessSize() << " vs " << *max_wit_size << ")");
         if (stack_exec) BOOST_CHECK_MESSAGE(*node->GetExecStackSize() == *stack_exec, "Stack execution limit mismatch: " << ms << " (" << *node->GetExecStackSize() << " vs " << *stack_exec << ")");
-        TestSatisfy(converter, ms, node);
+        TestSatisfy(converter, ms, *node);
     }
 }
 
@@ -599,11 +600,11 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
     constexpr KeyConverter tap_converter{miniscript::MiniscriptContext::TAPSCRIPT};
     constexpr KeyConverter wsh_converter{miniscript::MiniscriptContext::P2WSH};
     const auto no_pubkey{"ac519c"_hex_u8};
-    BOOST_CHECK(miniscript::FromScript({no_pubkey.begin(), no_pubkey.end()}, tap_converter) == nullptr);
+    BOOST_CHECK(!miniscript::FromScript({no_pubkey.begin(), no_pubkey.end()}, tap_converter));
     const auto incomplete_multi_a{"ba20c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5ba519c"_hex_u8};
-    BOOST_CHECK(miniscript::FromScript({incomplete_multi_a.begin(), incomplete_multi_a.end()}, tap_converter) == nullptr);
+    BOOST_CHECK(!miniscript::FromScript({incomplete_multi_a.begin(), incomplete_multi_a.end()}, tap_converter));
     const auto incomplete_multi_a_2{"ac2079be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798ac20c6047f9441ed7d6d3045406e95c07cd85c778e4b8cef3ca7abac09b95c709ee5ba519c"_hex_u8};
-    BOOST_CHECK(miniscript::FromScript({incomplete_multi_a_2.begin(), incomplete_multi_a_2.end()}, tap_converter) == nullptr);
+    BOOST_CHECK(!miniscript::FromScript({incomplete_multi_a_2.begin(), incomplete_multi_a_2.end()}, tap_converter));
     // Can use multi_a under Tapscript but not P2WSH.
     Test("and_v(v:multi_a(2,03d01115d548e7561b15c38f004d734633687cf4419620095bc5b0f47070afe85a,025601570cb47f238d2b0286db4a990fa0f3ba28d1a319f5e7cf55c2a2444da7cc),after(1231488000))", "?", "20d01115d548e7561b15c38f004d734633687cf4419620095bc5b0f47070afe85aac205601570cb47f238d2b0286db4a990fa0f3ba28d1a319f5e7cf55c2a2444da7ccba529d0400046749b1", TESTMODE_VALID | TESTMODE_NONMAL | TESTMODE_NEEDSIG | TESTMODE_P2WSH_INVALID, 4, 2, {}, {}, 3);
     // Can use more than 20 keys in a multi_a.
@@ -649,13 +650,13 @@ BOOST_AUTO_TEST_CASE(fixed_tests)
     // A Script with a non minimal push is invalid
     constexpr auto nonminpush{"0000210232780000feff00ffffffffffff21ff005f00ae21ae00000000060602060406564c2102320000060900fe00005f00ae21ae00100000060606060606000000000000000000000000000000000000000000000000000000000000000000"_hex_u8};
     const CScript nonminpush_script(nonminpush.begin(), nonminpush.end());
-    BOOST_CHECK(miniscript::FromScript(nonminpush_script, wsh_converter) == nullptr);
-    BOOST_CHECK(miniscript::FromScript(nonminpush_script, tap_converter) == nullptr);
+    BOOST_CHECK(!miniscript::FromScript(nonminpush_script, wsh_converter));
+    BOOST_CHECK(!miniscript::FromScript(nonminpush_script, tap_converter));
     // A non-minimal VERIFY (<key> CHECKSIG VERIFY 1)
     constexpr auto nonminverify{"2103a0434d9e47f3c86235477c7b1ae6ae5d3442d49b1943c2b752a68e2a47e247c7ac6951"_hex_u8};
     const CScript nonminverify_script(nonminverify.begin(), nonminverify.end());
-    BOOST_CHECK(miniscript::FromScript(nonminverify_script, wsh_converter) == nullptr);
-    BOOST_CHECK(miniscript::FromScript(nonminverify_script, tap_converter) == nullptr);
+    BOOST_CHECK(!miniscript::FromScript(nonminverify_script, wsh_converter));
+    BOOST_CHECK(!miniscript::FromScript(nonminverify_script, tap_converter));
     // A threshold as large as the number of subs is valid.
     Test("thresh(2,c:pk_k(03d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65),altv:after(100))", "2103d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65ac6b6300670164b16951686c935287", "20d30199d74fb5a22d47b6e054e2f378cedacffcb89904a61d75d0dbd407143e65ac6b6300670164b16951686c935287", TESTMODE_VALID | TESTMODE_NEEDSIG | TESTMODE_NONMAL);
     // A threshold of 1 is valid.

test/functional/wallet_backwards_compatibility.py

@@ -87,7 +87,7 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
         # 0.21.x and 22.x would both produce bad derivation paths when topping up an inactive hd chain
         # Make sure that this is being automatically cleaned up by migration
         node_master = self.nodes[1]
-        node_v22 = self.nodes[self.num_nodes - 5]
+        node_v22 = self.nodes[self.num_nodes - 3]
         wallet_name = "bad_deriv_path"
         node_v22.createwallet(wallet_name=wallet_name, descriptors=False)
         bad_deriv_wallet = node_v22.get_wallet_rpc(wallet_name)