Merge bitcoin/bitcoin#31713: miniscript refactor: Remove unique_ptr-indirection

964c44c test(miniscript): Prove avoidance of stack overflow (Hodlinator)
198bbae refactor(miniscript): Destroy nodes one full subs-vector at a time (Hodlinator)
50cab85 refactor(miniscript): Remove NodeRef & MakeNodeRef() (Hodlinator)
15fb34d refactor(miniscript): Remove superfluous unique_ptr-indirection (Hodlinator)
e55b23c refactor(miniscript): Remove Node::subs mutability (Hodlinator)
c6f798b refactor(miniscript): Make fields non-const & private (Hodlinator)
22e4115 doc(miniscript): Remove mention of shared pointers (Hodlinator)

Pull request description:

  Removes one level of unnecessary indirection, which was a change that originally [aided in finding one issue](bitcoin/bitcoin#30866 (review)) in #30866. Simplifies the code one step further than 09a1875 belonging to aforementioned PR.

  Also adds test which verifies resistance to stack overflow when it comes to `~Node()` and `Node::Clone()`.

  No observed difference when running benchmarks: ExpandDescriptor/WalletIsMineDescriptors/WalletIsMineMigratedDescriptors/WalletLoadingDescriptors.

  Followup to #30866.

ACKs for top commit:
  achow101:
    ACK 964c44c
  darosior:
    Code review ACK 964c44c
  l0rinc:
    ACK 964c44c

Tree-SHA512: 32927e8f0f916fb70372ffd110f7ec7207d9e7a099c21c0a7482a12e96593b673c339719f4ab166ad7c086dc43767315fc1742c5b236a3facc45c4cfeb5872e9

Author: achow101

src/script/descriptor.cpp

@@ -1584,31 +1584,31 @@ class StringMaker {
 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);
             } else {
                 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))
     {
         // Traverse miniscript tree for unsafe use of older()
-        miniscript::ForEachNode(*m_node, [&](const miniscript::Node<uint32_t>& node) {
-            if (node.fragment == miniscript::Fragment::OLDER) {
-                const uint32_t raw = node.k;
+        miniscript::ForEachNode(m_node, [&](const miniscript::Node<uint32_t>& node) {
+            if (node.Fragment() == miniscript::Fragment::OLDER) {
+                const uint32_t raw = node.K();
                 const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
                 if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
                     const bool is_time_based = (raw & CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG) != 0;
@@ -1626,7 +1626,7 @@ class MiniscriptDescriptor final : public DescriptorImpl
                         const DescriptorCache* cache = nullptr) const override
     {
         bool has_priv_key{false};
-        auto res = m_node->ToString(StringMaker(arg, m_pubkey_args, type, cache), has_priv_key);
+        auto res = m_node.ToString(StringMaker(arg, m_pubkey_args, type, cache), has_priv_key);
         if (res) out = *res;
         if (type == StringType::PRIVATE) {
             Assume(res.has_value());
@@ -1639,15 +1639,17 @@ class MiniscriptDescriptor final : public DescriptorImpl
     bool IsSolvable() const override { return true; }
     bool IsSingleType() const final { return true; }
 
-    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 {
+    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();
+    std::optional<int64_t> MaxSatisfactionElems() const override
+    {
+        return m_node.GetStackSize();
     }
 
     std::unique_ptr<DescriptorImpl> Clone() const override
@@ -1657,7 +1659,7 @@ class MiniscriptDescriptor final : public DescriptorImpl
         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());
     }
 };
 
@@ -2566,7 +2568,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.value();
                 if (const auto sub = node->FindInsaneSub()) insane_node = sub;
                 error = *insane_node->ToString(parser);
                 if (!insane_node->IsValid()) {
@@ -2575,7 +2577,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.value() && !insane_node->NeedsSignature()) {
                         error += ": witnesses without signature exist";
                     } else if (!insane_node->CheckTimeLocksMix()) {
                         error += ": contains mixes of timelocks expressed in blocks and seconds";
@@ -2775,7 +2777,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));
         }
     }