txgraph: Destroying Ref means removing transaction (feature)

Before this commit, if a TxGraph::Ref object is destroyed, it becomes impossible
to refer to, but the actual corresponding transaction node in the TxGraph remains,
and remains indefinitely as there is no way to remove it.

Fix this by making the destruction of TxGraph::Ref trigger immediate removal of
the corresponding transaction in TxGraph, both in main and staging if it exists.

src/test/fuzz/txgraph.cpp

@@ -146,6 +146,30 @@ struct SimTxGraph
         if (oversized.has_value() && *oversized) oversized = std::nullopt;
     }
 
+    /** Destroy the transaction from the graph, including from the removed set. This will
+     *  trigger TxGraph::Ref::~Ref. reset_oversize controls whether the cached oversized
+     *  value is cleared (destroying does not clear oversizedness in TxGraph of the main
+     *  graph while staging exists). */
+    void DestroyTransaction(TxGraph::Ref* ref, bool reset_oversize)
+    {
+        auto pos = Find(ref);
+        if (pos == MISSING) {
+            // Wipe the ref, if it exists, from the removed vector. Use std::partition rather
+            // than std::erase because we don't care about the order of the entries that
+            // remain.
+            auto remove = std::partition(removed.begin(), removed.end(), [&](auto& arg) { return arg.get() != ref; });
+            removed.erase(remove, removed.end());
+        } else {
+            graph.RemoveTransactions(SetType::Singleton(pos));
+            simrevmap.erase(simmap[pos].get());
+            simmap[pos].reset();
+            // This may invalidate our cached oversized value.
+            if (reset_oversize && oversized.has_value() && *oversized) {
+                oversized = std::nullopt;
+            }
+        }
+    }
+
     /** Construct the set with all positions in this graph corresponding to the specified
      *  TxGraph::Refs. All of them must occur in this graph and not be removed. */
     SetType MakeSet(std::span<TxGraph::Ref* const> arg)
@@ -327,9 +351,9 @@ FUZZ_TARGET(txgraph)
                 }
                 break;
             } else if (sel_sim.removed.size() > 0 && command-- == 0) {
-                // ~Ref. Destroying a TxGraph::Ref has an observable effect on the TxGraph it
+                // ~Ref (of an already-removed transaction). Destroying a TxGraph::Ref has an
-                // refers to, so this simulation permits doing so separately from other actions on
+                // observable effect on the TxGraph it refers to, so this simulation permits doing
-                // TxGraph.
+                // so separately from other actions on TxGraph.
 
                 // Pick a Ref of sel_sim.removed to destroy. Note that the same Ref may still occur
                 // in the other graph, and thus not actually trigger ~Ref yet (which is exactly
@@ -341,6 +365,28 @@ FUZZ_TARGET(txgraph)
                 }
                 sel_sim.removed.pop_back();
                 break;
+            } else if (command-- == 0) {
+                // ~Ref (of any transaction).
+                std::vector<TxGraph::Ref*> to_destroy;
+                to_destroy.push_back(pick_fn());
+                while (true) {
+                    // Keep adding either the ancestors or descendants the already picked
+                    // transactions have in both graphs (main and staging) combined. Destroying
+                    // will trigger deletions in both, so to have consistent TxGraph behavior, the
+                    // set must be closed under ancestors, or descendants, in both graphs.
+                    auto old_size = to_destroy.size();
+                    for (auto& sim : sims) sim.IncludeAncDesc(to_destroy, alt);
+                    if (to_destroy.size() == old_size) break;
+                }
+                // The order in which these ancestors/descendants are destroyed should not matter;
+                // randomly shuffle them.
+                std::shuffle(to_destroy.begin(), to_destroy.end(), rng);
+                for (TxGraph::Ref* ptr : to_destroy) {
+                    for (size_t level = 0; level < sims.size(); ++level) {
+                        sims[level].DestroyTransaction(ptr, level == sims.size() - 1);
+                    }
+                }
+                break;
             } else if (command-- == 0) {
                 // SetTransactionFee.
                 int64_t fee;
@@ -457,6 +503,10 @@ FUZZ_TARGET(txgraph)
                 // AbortStaging.
                 real->AbortStaging();
                 sims.pop_back();
+                // Reset the cached oversized value (if TxGraph::Ref destructions triggered
+                // removals of main transactions while staging was active, then aborting will
+                // cause it to be re-evaluated in TxGraph).
+                sims.back().oversized = std::nullopt;
                 break;
             }
         }
@@ -537,13 +587,4 @@ FUZZ_TARGET(txgraph)
 
     // Sanity check again (because invoking inspectors may modify internal unobservable state).
     real->SanityCheck();
-
-    // Remove all remaining transactions, because Refs cannot be destroyed otherwise (this will be
-    // addressed in a follow-up commit).
-    for (auto& sim : sims) {
-        for (auto i : sim.graph.Positions()) {
-            auto ref = sim.GetRef(i);
-            real->RemoveTransaction(*ref);
-        }
-    }
 }

src/txgraph.cpp

@@ -225,7 +225,8 @@ private:
         /** Information about the merges to be performed, if known. */
         std::optional<GroupData> m_group_data = GroupData{};
         /** Which entries were removed in this ClusterSet (so they can be wiped on abort). This
-         *  includes all entries which have an (R) removed locator at this level (staging only). */
+         *  includes all entries which have an (R) removed locator at this level (staging only),
+         *  plus optionally any transaction in m_unlinked. */
         std::vector<GraphIndex> m_removed;
         /** Total number of transactions in this graph (sum of all transaction counts in all
          *  Clusters, and for staging also those inherited from the main ClusterSet). */
@@ -368,8 +369,34 @@ public:
         auto& entry = m_entries[idx];
         Assume(entry.m_ref != nullptr);
         entry.m_ref = nullptr;
+        // Mark the transaction as to be removed in all levels where it explicitly or implicitly
+        // exists.
+        bool exists_anywhere{false};
+        bool exists{false};
+        for (int level = 0; level <= GetTopLevel(); ++level) {
+            if (entry.m_locator[level].IsPresent()) {
+                exists_anywhere = true;
+                exists = true;
+            } else if (entry.m_locator[level].IsRemoved()) {
+                exists = false;
+            }
+            if (exists) {
+                auto& clusterset = GetClusterSet(level);
+                clusterset.m_to_remove.push_back(idx);
+                // Force recomputation of grouping data.
+                clusterset.m_group_data = std::nullopt;
+                // Do not wipe the oversized state of main if staging exists. The reason for this
+                // is that the alternative would mean that cluster merges may need to be applied to
+                // a formerly-oversized main graph while staging exists (to satisfy chunk feerate
+                // queries into main, for example), and such merges could conflict with pulls of
+                // some of their constituents into staging.
+                if (level == GetTopLevel() && clusterset.m_oversized == true) {
+                    clusterset.m_oversized = std::nullopt;
+                }
+            }
+        }
         m_unlinked.push_back(idx);
-        Compact();
+        if (!exists_anywhere) Compact();
     }
 
     // Functions related to various normalization/application steps.
@@ -382,12 +409,12 @@ public:
     *   transactions from inherited (P,M) to explicit (P,P). */
     Cluster* PullIn(Cluster* cluster) noexcept;
     /** Apply all removals queued up in m_to_remove to the relevant Clusters (which get a
-     *  NEEDS_SPLIT* QualityLevel) in the specified level. */
+     *  NEEDS_SPLIT* QualityLevel) up to the specified level. */
-    void ApplyRemovals(int level) noexcept;
+    void ApplyRemovals(int up_to_level) noexcept;
-    /** Split an individual cluster (which must be in the top-level ClusterSet). */
+    /** Split an individual cluster. */
     void Split(Cluster& cluster) noexcept;
-    /** Split all clusters that need splitting in the specified level. */
+    /** Split all clusters that need splitting up to the specified level. */
-    void SplitAll(int level) noexcept;
+    void SplitAll(int up_to_level) noexcept;
     /** Populate m_group_data based on m_deps_to_add in the specified level. */
     void GroupClusters(int level) noexcept;
     /** Merge the specified clusters. */
@@ -451,6 +478,14 @@ void TxGraphImpl::ClearLocator(int level, GraphIndex idx) noexcept
     }
     // Update the transaction count.
     --clusterset.m_txcount;
+    // If clearing main, adjust the status of Locators of this transaction in staging, if it exists.
+    if (level == 0 && GetTopLevel() == 1) {
+        if (entry.m_locator[1].IsRemoved()) {
+            entry.m_locator[1].SetMissing();
+        } else if (!entry.m_locator[1].IsPresent()) {
+            --m_staging_clusterset->m_txcount;
+        }
+    }
 }
 
 void Cluster::Updated(TxGraphImpl& graph) noexcept
@@ -505,8 +540,9 @@ std::vector<Cluster*> TxGraphImpl::GetConflicts() const noexcept
     // All main Clusters containing transactions in m_removed (so (P,R) ones) are conflicts.
     for (auto i : clusterset.m_removed) {
         auto& entry = m_entries[i];
-        Assume(entry.m_locator[0].IsPresent());
+        if (entry.m_locator[0].IsPresent()) {
-        ret.push_back(entry.m_locator[0].cluster);
+            ret.push_back(entry.m_locator[0].cluster);
+        }
     }
     // Then go over all Clusters at this level, and find their conflicts (the (P,P) ones).
     for (int quality = 0; quality < int(QualityLevel::NONE); ++quality) {
@@ -873,39 +909,41 @@ Cluster* TxGraphImpl::PullIn(Cluster* cluster) noexcept
     return cluster;
 }
 
-void TxGraphImpl::ApplyRemovals(int level) noexcept
+void TxGraphImpl::ApplyRemovals(int up_to_level) noexcept
 {
-    auto& clusterset = GetClusterSet(level);
+    Assume(up_to_level >= 0 && up_to_level <= GetTopLevel());
-    auto& to_remove = clusterset.m_to_remove;
+    for (int level = 0; level <= up_to_level; ++level) {
-    // Skip if there is nothing to remove.
+        auto& clusterset = GetClusterSet(level);
-    if (to_remove.empty()) return;
+        auto& to_remove = clusterset.m_to_remove;
-    // There cannot be removals to be applied in main when staging exists (they should have been
+        // Skip if there is nothing to remove in this level.
-    // applied in StartStaging already, and none can be added to main while staging exists).
+        if (to_remove.empty()) continue;
-    Assume(level == GetTopLevel());
+        // Pull in all Clusters that are not in staging.
-    // Pull in all Clusters that are not in staging.
+        if (level == 1) {
-    for (GraphIndex index : to_remove) {
+            for (GraphIndex index : to_remove) {
-        auto cluster = FindCluster(index, level);
+                auto cluster = FindCluster(index, level);
-        PullIn(cluster);
+                if (cluster != nullptr) PullIn(cluster);
-    }
+            }
-    // Group the set of to-be-removed entries by Cluster*.
-    std::sort(to_remove.begin(), to_remove.end(), [&](GraphIndex a, GraphIndex b) noexcept {
-        return std::less{}(m_entries[a].m_locator[level].cluster, m_entries[b].m_locator[level].cluster);
-    });
-    // Process per Cluster.
-    std::span to_remove_span{to_remove};
-    while (!to_remove_span.empty()) {
-        Cluster* cluster = m_entries[to_remove_span.front()].m_locator[level].cluster;
-        if (cluster != nullptr) {
-            // If the first to_remove_span entry's Cluster exists, hand to_remove_span to it, so it
-            // can pop off whatever applies to it.
-            cluster->ApplyRemovals(*this, to_remove_span);
-        } else {
-            // Otherwise, skip this already-removed entry. This may happen when RemoveTransaction
-            // was called twice on the same Ref.
-            to_remove_span = to_remove_span.subspan(1);
         }
+        // Group the set of to-be-removed entries by Cluster*.
+        std::sort(to_remove.begin(), to_remove.end(), [&](GraphIndex a, GraphIndex b) noexcept {
+            return std::less{}(m_entries[a].m_locator[level].cluster, m_entries[b].m_locator[level].cluster);
+        });
+        // Process per Cluster.
+        std::span to_remove_span{to_remove};
+        while (!to_remove_span.empty()) {
+            Cluster* cluster = m_entries[to_remove_span.front()].m_locator[level].cluster;
+            if (cluster != nullptr) {
+                // If the first to_remove_span entry's Cluster exists, hand to_remove_span to it, so it
+                // can pop off whatever applies to it.
+                cluster->ApplyRemovals(*this, to_remove_span);
+            } else {
+                // Otherwise, skip this already-removed entry. This may happen when
+                // RemoveTransaction was called twice on the same Ref, for example.
+                to_remove_span = to_remove_span.subspan(1);
+            }
+        }
+        to_remove.clear();
     }
-    to_remove.clear();
     Compact();
 }
 
@@ -919,7 +957,7 @@ void TxGraphImpl::SwapIndexes(GraphIndex a, GraphIndex b) noexcept
     for (int i = 0; i < 2; ++i) {
         GraphIndex idx = i ? b : a;
         Entry& entry = m_entries[idx];
-        // Update linked Ref.
+        // Update linked Ref, if any exists.
         if (entry.m_ref) GetRefIndex(*entry.m_ref) = idx;
         // Update the locators for both levels. The rest of the Entry information will not change,
         // so no need to invoke Cluster::Updated().
@@ -986,14 +1024,17 @@ void TxGraphImpl::Split(Cluster& cluster) noexcept
     }
 }
 
-void TxGraphImpl::SplitAll(int level) noexcept
+void TxGraphImpl::SplitAll(int up_to_level) noexcept
 {
+    Assume(up_to_level >= 0 && up_to_level <= GetTopLevel());
     // Before splitting all Cluster, first make sure all removals are applied.
-    ApplyRemovals(level);
+    ApplyRemovals(up_to_level);
-    for (auto quality : {QualityLevel::NEEDS_SPLIT, QualityLevel::NEEDS_SPLIT_ACCEPTABLE}) {
+    for (int level = 0; level <= up_to_level; ++level) {
-        auto& queue = GetClusterSet(level).m_clusters[int(quality)];
+        for (auto quality : {QualityLevel::NEEDS_SPLIT, QualityLevel::NEEDS_SPLIT_ACCEPTABLE}) {
-        while (!queue.empty()) {
+            auto& queue = GetClusterSet(level).m_clusters[int(quality)];
-            Split(*queue.back().get());
+            while (!queue.empty()) {
+                Split(*queue.back().get());
+            }
         }
     }
 }
@@ -1003,10 +1044,6 @@ void TxGraphImpl::GroupClusters(int level) noexcept
     auto& clusterset = GetClusterSet(level);
     // If the groupings have been computed already, nothing is left to be done.
     if (clusterset.m_group_data.has_value()) return;
-    // We should never need to compute main grouping while staging exists (it should have already
-    // been computing in StartStaging, and no modifications that invalidate it can be made while
-    // staging exists).
-    Assume(level == GetTopLevel());
 
     // Before computing which Clusters need to be merged together, first apply all removals and
     // split the Clusters into connected components. If we would group first, we might end up
@@ -1256,8 +1293,10 @@ void TxGraphImpl::ApplyDependencies(int level) noexcept
         auto cluster_span = std::span{clusterset.m_group_data->m_group_clusters}
                                 .subspan(group_data.m_cluster_offset, group_data.m_cluster_count);
         // Pull in all the Clusters that contain dependencies.
-        for (Cluster*& cluster : cluster_span) {
+        if (level == 1) {
-            cluster = PullIn(cluster);
+            for (Cluster*& cluster : cluster_span) {
+                cluster = PullIn(cluster);
+            }
         }
         // Invoke Merge() to merge them into a single Cluster.
         Merge(cluster_span);
@@ -1398,6 +1437,7 @@ std::vector<TxGraph::Ref*> Cluster::GetAncestorRefs(const TxGraphImpl& graph, De
     // Translate all ancestors (in arbitrary order) to Refs (if they have any), and return them.
     for (auto idx : m_depgraph.Ancestors(idx)) {
         const auto& entry = graph.m_entries[m_mapping[idx]];
+        Assume(entry.m_ref != nullptr);
         ret.push_back(entry.m_ref);
     }
     return ret;
@@ -1410,6 +1450,7 @@ std::vector<TxGraph::Ref*> Cluster::GetDescendantRefs(const TxGraphImpl& graph,
     // Translate all descendants (in arbitrary order) to Refs (if they have any), and return them.
     for (auto idx : m_depgraph.Descendants(idx)) {
         const auto& entry = graph.m_entries[m_mapping[idx]];
+        Assume(entry.m_ref != nullptr);
         ret.push_back(entry.m_ref);
     }
     return ret;
@@ -1422,6 +1463,7 @@ std::vector<TxGraph::Ref*> Cluster::GetClusterRefs(const TxGraphImpl& graph) noe
     // Translate all transactions in the Cluster (in linearization order) to Refs.
     for (auto idx : m_linearization) {
         const auto& entry = graph.m_entries[m_mapping[idx]];
+        Assume(entry.m_ref != nullptr);
         ret.push_back(entry.m_ref);
     }
     return ret;
@@ -1513,7 +1555,8 @@ FeePerWeight TxGraphImpl::GetIndividualFeerate(const Ref& arg) noexcept
     // be identical if it occurs in both), and return the empty FeePerWeight if it isn't in any.
     Cluster* cluster{nullptr};
     for (int level = 0; level <= GetTopLevel(); ++level) {
-        // Apply removals, so that we can correctly report FeePerWeight{} for non-existing transaction.
+        // Apply removals, so that we can correctly report FeePerWeight{} for non-existing
+        // transactions.
         ApplyRemovals(level);
         if (m_entries[GetRefIndex(arg)].m_locator[level].IsPresent()) {
             cluster = m_entries[GetRefIndex(arg)].m_locator[level].cluster;
@@ -1600,6 +1643,11 @@ void TxGraphImpl::AbortStaging() noexcept
     // Destroy the staging ClusterSet.
     m_staging_clusterset.reset();
     Compact();
+    if (!m_main_clusterset.m_group_data.has_value()) {
+        // In case m_oversized in main was kept after a Ref destruction while staging exists, we
+        // need to re-evaluate m_oversized now.
+        m_main_clusterset.m_oversized = std::nullopt;
+    }
 }
 
 void TxGraphImpl::CommitStaging() noexcept
@@ -1783,10 +1831,13 @@ void TxGraphImpl::SanityCheck() const
             }
         }
 
-        // Verify that all to-be-removed transactions have valid identifiers, and aren't removed yet.
+        // Verify that all to-be-removed transactions have valid identifiers.
         for (GraphIndex idx : clusterset.m_to_remove) {
             assert(idx < m_entries.size());
-            assert(FindCluster(idx, level) != nullptr);
+            // We cannot assert that all m_to_remove transactions are still present: ~Ref on a
+            // (P,M) transaction (present in main, inherited in staging) will cause an m_to_remove
+            // addition in both main and staging, but a subsequence ApplyRemovals in main will
+            // cause it to disappear from staging too, leaving the m_to_remove in place.
         }
 
         // Verify that all to-be-added dependencies have valid identifiers.
@@ -1801,6 +1852,15 @@ void TxGraphImpl::SanityCheck() const
 
         // Verify that the contents of m_removed matches what was expected based on the Entry vector.
         std::set<GraphIndex> actual_removed(clusterset.m_removed.begin(), clusterset.m_removed.end());
+        for (auto i : expected_unlinked) {
+            // If a transaction exists in both main and staging, and is removed from staging (adding
+            // it to m_removed there), and consequently destroyed (wiping the locator completely),
+            // it can remain in m_removed despite not having an IsRemoved() locator. Exclude those
+            // transactions from the comparison here.
+            actual_removed.erase(i);
+            expected_removed[level].erase(i);
+        }
+
         assert(actual_removed == expected_removed[level]);
 
         // If any GraphIndex entries remain in this ClusterSet, compact is not possible.
@@ -1812,6 +1872,10 @@ void TxGraphImpl::SanityCheck() const
         if (clusterset.m_group_data.has_value()) {
             assert(clusterset.m_oversized == clusterset.m_group_data->m_group_oversized);
         }
+
+        // For non-top levels, m_oversized must be known (as it cannot change until the level
+        // on top is gone).
+        if (level < GetTopLevel()) assert(clusterset.m_oversized.has_value());
     }
 
     // Verify that the contents of m_unlinked matches what was expected based on the Entry vector.

src/txgraph.h

@@ -50,7 +50,8 @@ public:
     /** Data type used to reference transactions within a TxGraph.
      *
      * Every transaction within a TxGraph has exactly one corresponding TxGraph::Ref, held by users
-     * of the class. Refs can only be destroyed after the transaction is removed from the graph.
+     * of the class. Destroying the TxGraph::Ref removes the corresponding transaction (in both the
+     * main and staging graphs).
      *
      * Users of the class can inherit from TxGraph::Ref. If all Refs are inherited this way, the
      * Ref* pointers returned by TxGraph functions can be cast to, and used as, this inherited type.
@@ -104,7 +105,9 @@ public:
     /** Determine whether the graph is oversized (contains a connected component of more than the
      *  configured maximum cluster count). If main_only is false and a staging graph exists, it is
      *  queried; otherwise the main graph is queried. Some of the functions below are not available
-     *  for oversized graphs. The mutators above are always available. */
+     *  for oversized graphs. The mutators above are always available. Removing a transaction by
+     *  destroying its Ref while staging exists will not clear main's oversizedness until staging
+     *  is aborted or committed. */
     virtual bool IsOversized(bool main_only = false) noexcept = 0;
     /** Determine whether arg exists in the graph (i.e., was not removed). If main_only is false
      *  and a staging graph exists, it is queried; otherwise the main graph is queried. This is
@@ -167,8 +170,8 @@ public:
         /** Construct an empty Ref. Non-empty Refs can only be created using
          *  TxGraph::AddTransaction. */
         Ref() noexcept = default;
-        /** Destroy this Ref. This is only allowed when it is empty, or the transaction it refers
+        /** Destroy this Ref. If it is not empty, the corresponding transaction is removed (in both
-         *  to does not exist in the graph (in main nor staging). */
+         *  main and staging, if it exists). */
         virtual ~Ref();
         // Support moving a Ref.
         Ref& operator=(Ref&& other) noexcept;