Merge bitcoin/bitcoin#32191: Make TxGraph fuzz tests more deterministic

2835216ec0 txgraph: make GroupClusters use partition numbers directly (optimization) (Pieter Wuille)
c72c8d5d45 txgraph: compare sequence numbers instead of Cluster* (bugfix) (Pieter Wuille)

Pull request description:

  Part of cluster mempool: #30289

  The implicit transaction ordering for transactions in a TxGraphImpl is defined by:
  1. higher chunk feerate first
  2. lower Cluster* object pointer first
  3. lower position within cluster linearization first.

  Number (2) is not deterministic, as it intricately depends on the heap allocation algorithm. Fix this by giving each Cluster a unique `uint64_t m_sequence` value, and sorting by those instead.

  The second commit then uses this new approach to optimize GroupClusters a bit more, avoiding some repeated checks and dereferences, by making a local copy of the involved sequence numbers.

  Thanks to @dergoegge for pointing this out.

ACKs for top commit:
  instagibbs:
    reACK 2835216ec0
  marcofleon:
    ACK 2835216ec0
  glozow:
    utACK 2835216ec0

Tree-SHA512: d772a55b9ed620159b934a42a39fca7f900d4aa89c099a280a0c61ea0bd7c4fc39b388281ffc775064ea77b0b17263871b4c9763aa71c710a79287d5eb2cd4b4

src/test/fuzz/txgraph.cpp

@@ -206,11 +206,14 @@ struct SimTxGraph
                 ret.push_back(ptr);
             }
         }
-        // Deduplicate.
+        // Construct deduplicated version in input (do not use std::sort/std::unique for
-        std::sort(ret.begin(), ret.end());
+        // deduplication as it'd rely on non-deterministic pointer comparison).
-        ret.erase(std::unique(ret.begin(), ret.end()), ret.end());
+        arg.clear();
-        // Replace input.
+        for (auto ptr : ret) {
-        arg = std::move(ret);
+            if (std::find(arg.begin(), arg.end(), ptr) == arg.end()) {
+                arg.push_back(ptr);
+            }
+        }
     }
 };
 

src/txgraph.cpp

@@ -70,12 +70,16 @@ class Cluster
     ClusterSetIndex m_setindex{ClusterSetIndex(-1)};
     /** Which level this Cluster is at in the graph (-1=not inserted, 0=main, 1=staging). */
     int m_level{-1};
+    /** Sequence number for this Cluster (for tie-breaking comparison between equal-chunk-feerate
+        transactions in distinct clusters). */
+    uint64_t m_sequence;
 
 public:
+    Cluster() noexcept = delete;
     /** Construct an empty Cluster. */
-    Cluster() noexcept = default;
+    explicit Cluster(uint64_t sequence) noexcept;
     /** Construct a singleton Cluster. */
-    explicit Cluster(TxGraphImpl& graph, const FeePerWeight& feerate, GraphIndex graph_index) noexcept;
+    explicit Cluster(uint64_t sequence, TxGraphImpl& graph, const FeePerWeight& feerate, GraphIndex graph_index) noexcept;
 
     // Cannot move or copy (would invalidate Cluster* in Locator and ClusterSet). */
     Cluster(const Cluster&) = delete;
@@ -157,6 +161,7 @@ public:
     void SanityCheck(const TxGraphImpl& graph, int level) const;
 };
 
+
 /** The transaction graph, including staged changes.
  *
  * The overall design of the data structure consists of 3 interlinked representations:
@@ -244,6 +249,8 @@ private:
     ClusterSet m_main_clusterset;
     /** The staging ClusterSet, if any. */
     std::optional<ClusterSet> m_staging_clusterset;
+    /** Next sequence number to assign to created Clusters. */
+    uint64_t m_next_sequence_counter{0};
 
     /** A Locator that describes whether, where, and in which Cluster an Entry appears.
      *  Every Entry has MAX_LEVELS locators, as it may appear in one Cluster per level.
@@ -315,6 +322,18 @@ private:
     /** Set of Entries which have no linked Ref anymore. */
     std::vector<GraphIndex> m_unlinked;
 
+    /** Compare two Cluster* by their m_sequence value (while supporting nullptr). */
+    static std::strong_ordering CompareClusters(Cluster* a, Cluster* b) noexcept
+    {
+        // The nullptr pointer compares before everything else.
+        if (a == nullptr || b == nullptr) {
+            return (a != nullptr) <=> (b != nullptr);
+        }
+        // If neither pointer is nullptr, compare the Clusters' sequence numbers.
+        Assume(a == b || a->m_sequence != b->m_sequence);
+        return a->m_sequence <=> b->m_sequence;
+    }
+
 public:
     /** Construct a new TxGraphImpl with the specified maximum cluster count. */
     explicit TxGraphImpl(DepGraphIndex max_cluster_count) noexcept :
@@ -569,7 +588,7 @@ std::vector<Cluster*> TxGraphImpl::GetConflicts() const noexcept
         }
     }
     // Deduplicate the result (the same Cluster may appear multiple times).
-    std::sort(ret.begin(), ret.end());
+    std::sort(ret.begin(), ret.end(), [](Cluster* a, Cluster* b) noexcept { return CompareClusters(a, b) < 0; });
     ret.erase(std::unique(ret.begin(), ret.end()), ret.end());
     return ret;
 }
@@ -577,7 +596,7 @@ std::vector<Cluster*> TxGraphImpl::GetConflicts() const noexcept
 Cluster* Cluster::CopyToStaging(TxGraphImpl& graph) const noexcept
 {
     // Construct an empty Cluster.
-    auto ret = std::make_unique<Cluster>();
+    auto ret = std::make_unique<Cluster>(graph.m_next_sequence_counter++);
     auto ptr = ret.get();
     // Copy depgraph, mapping, and linearization/
     ptr->m_depgraph = m_depgraph;
@@ -710,7 +729,7 @@ bool Cluster::Split(TxGraphImpl& graph) noexcept
         }
         first = false;
         // Construct a new Cluster to hold the found component.
-        auto new_cluster = std::make_unique<Cluster>();
+        auto new_cluster = std::make_unique<Cluster>(graph.m_next_sequence_counter++);
         new_clusters.push_back(new_cluster.get());
         // Remember that all the component's transactions go to this new Cluster. The positions
         // will be determined below, so use -1 for now.
@@ -956,9 +975,11 @@ void TxGraphImpl::ApplyRemovals(int up_to_level) noexcept
                 if (cluster != nullptr) PullIn(cluster);
             }
         }
-        // Group the set of to-be-removed entries by Cluster*.
+        // Group the set of to-be-removed entries by Cluster::m_sequence.
         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);
+            Cluster* cluster_a = m_entries[a].m_locator[level].cluster;
+            Cluster* cluster_b = m_entries[b].m_locator[level].cluster;
+            return CompareClusters(cluster_a, cluster_b) < 0;
         });
         // Process per Cluster.
         std::span to_remove_span{to_remove};
@@ -1082,38 +1103,37 @@ void TxGraphImpl::GroupClusters(int level) noexcept
     // with inefficient and/or oversized Clusters which just end up being split again anyway.
     SplitAll(level);
 
-    /** Annotated clusters: an entry for each Cluster, together with the representative for the
+    /** Annotated clusters: an entry for each Cluster, together with the sequence number for the
-     *  partition it is in if known, or with nullptr if not yet known. */
+     *  representative for the partition it is in (initially its own, later that of the
-    std::vector<std::pair<Cluster*, Cluster*>> an_clusters;
+     *  to-be-merged group). */
+    std::vector<std::pair<Cluster*, uint64_t>> an_clusters;
     /** Annotated dependencies: an entry for each m_deps_to_add entry (excluding ones that apply
-     *  to removed transactions), together with the representative root of the partition of
+     *  to removed transactions), together with the sequence number of the representative root of
-     *  Clusters it applies to. */
+     *  Clusters it applies to (initially that of the child Cluster, later that of the
-    std::vector<std::pair<std::pair<GraphIndex, GraphIndex>, Cluster*>> an_deps;
+     *  to-be-merged group). */
+    std::vector<std::pair<std::pair<GraphIndex, GraphIndex>, uint64_t>> an_deps;
 
-    // Construct a an_clusters entry for every parent and child in the to-be-applied dependencies.
+    // Construct a an_clusters entry for every parent and child in the to-be-applied dependencies,
+    // and an an_deps entry for each dependency to be applied.
+    an_deps.reserve(clusterset.m_deps_to_add.size());
     for (const auto& [par, chl] : clusterset.m_deps_to_add) {
         auto par_cluster = FindCluster(par, level);
         auto chl_cluster = FindCluster(chl, level);
         // Skip dependencies for which the parent or child transaction is removed.
         if (par_cluster == nullptr || chl_cluster == nullptr) continue;
-        an_clusters.emplace_back(par_cluster, nullptr);
+        an_clusters.emplace_back(par_cluster, par_cluster->m_sequence);
         // Do not include a duplicate when parent and child are identical, as it'll be removed
         // below anyway.
-        if (chl_cluster != par_cluster) an_clusters.emplace_back(chl_cluster, nullptr);
+        if (chl_cluster != par_cluster) an_clusters.emplace_back(chl_cluster, chl_cluster->m_sequence);
+        // Add entry to an_deps, using the child sequence number.
+        an_deps.emplace_back(std::pair{par, chl}, chl_cluster->m_sequence);
     }
     // Sort and deduplicate an_clusters, so we end up with a sorted list of all involved Clusters
     // to which dependencies apply.
-    std::sort(an_clusters.begin(), an_clusters.end());
+    std::sort(an_clusters.begin(), an_clusters.end(), [](auto& a, auto& b) noexcept { return a.second < b.second; });
     an_clusters.erase(std::unique(an_clusters.begin(), an_clusters.end()), an_clusters.end());
-
+    // Sort an_deps by applying the same order to the involved child cluster.
-    // Sort the dependencies by child Cluster.
+    std::sort(an_deps.begin(), an_deps.end(), [&](auto& a, auto& b) noexcept { return a.second < b.second; });
-    std::sort(clusterset.m_deps_to_add.begin(), clusterset.m_deps_to_add.end(), [&](auto& a, auto& b) noexcept {
-        auto [_a_par, a_chl] = a;
-        auto [_b_par, b_chl] = b;
-        auto a_chl_cluster = FindCluster(a_chl, level);
-        auto b_chl_cluster = FindCluster(b_chl, level);
-        return std::less{}(a_chl_cluster, b_chl_cluster);
-    });
 
     // Run the union-find algorithm to to find partitions of the input Clusters which need to be
     // grouped together. See https://en.wikipedia.org/wiki/Disjoint-set_data_structure.
@@ -1121,8 +1141,8 @@ void TxGraphImpl::GroupClusters(int level) noexcept
         /** Each PartitionData entry contains information about a single input Cluster. */
         struct PartitionData
         {
-            /** The cluster this holds information for. */
+            /** The sequence number of the cluster this holds information for. */
-            Cluster* cluster;
+            uint64_t sequence;
             /** All PartitionData entries belonging to the same partition are organized in a tree.
              *  Each element points to its parent, or to itself if it is the root. The root is then
              *  a representative for the entire tree, and can be found by walking upwards from any
@@ -1132,15 +1152,15 @@ void TxGraphImpl::GroupClusters(int level) noexcept
              *  tree for this partition. */
             unsigned rank;
         };
-        /** Information about each input Cluster. Sorted by Cluster* pointer. */
+        /** Information about each input Cluster. Sorted by Cluster::m_sequence. */
         std::vector<PartitionData> partition_data;
 
         /** Given a Cluster, find its corresponding PartitionData. */
-        auto locate_fn = [&](Cluster* arg) noexcept -> PartitionData* {
+        auto locate_fn = [&](uint64_t sequence) noexcept -> PartitionData* {
-            auto it = std::lower_bound(partition_data.begin(), partition_data.end(), arg,
+            auto it = std::lower_bound(partition_data.begin(), partition_data.end(), sequence,
-                                       [](auto& a, Cluster* ptr) noexcept { return a.cluster < ptr; });
+                                       [](auto& a, uint64_t seq) noexcept { return a.sequence < seq; });
             Assume(it != partition_data.end());
-            Assume(it->cluster == arg);
+            Assume(it->sequence == sequence);
             return &*it;
         };
 
@@ -1175,65 +1195,57 @@ void TxGraphImpl::GroupClusters(int level) noexcept
         // Start by initializing every Cluster as its own singleton partition.
         partition_data.resize(an_clusters.size());
         for (size_t i = 0; i < an_clusters.size(); ++i) {
-            partition_data[i].cluster = an_clusters[i].first;
+            partition_data[i].sequence = an_clusters[i].first->m_sequence;
             partition_data[i].parent = &partition_data[i];
             partition_data[i].rank = 0;
         }
 
-        // Run through all parent/child pairs in m_deps_to_add, and union the
+        // Run through all parent/child pairs in an_deps, and union the partitions their Clusters
-        // the partitions their Clusters are in.
+        // are in.
         Cluster* last_chl_cluster{nullptr};
         PartitionData* last_partition{nullptr};
-        for (const auto& [par, chl] : clusterset.m_deps_to_add) {
+        for (const auto& [dep, _] : an_deps) {
+            auto [par, chl] = dep;
             auto par_cluster = FindCluster(par, level);
             auto chl_cluster = FindCluster(chl, level);
+            Assume(chl_cluster != nullptr && par_cluster != nullptr);
             // Nothing to do if parent and child are in the same Cluster.
             if (par_cluster == chl_cluster) continue;
-            // Nothing to do if either parent or child transaction is removed already.
-            if (par_cluster == nullptr || chl_cluster == nullptr) continue;
             Assume(par != chl);
             if (chl_cluster == last_chl_cluster) {
                 // If the child Clusters is the same as the previous iteration, union with the
-                // tree they were in, avoiding the need for another lookup. Note that m_deps_to_add
+                // tree they were in, avoiding the need for another lookup. Note that an_deps
                 // is sorted by child Cluster, so batches with the same child are expected.
-                last_partition = union_fn(locate_fn(par_cluster), last_partition);
+                last_partition = union_fn(locate_fn(par_cluster->m_sequence), last_partition);
             } else {
                 last_chl_cluster = chl_cluster;
-                last_partition = union_fn(locate_fn(par_cluster), locate_fn(chl_cluster));
+                last_partition = union_fn(locate_fn(par_cluster->m_sequence), locate_fn(chl_cluster->m_sequence));
             }
         }
 
-        // Populate the an_clusters and an_deps data structures with the list of input Clusters,
+        // Update the sequence numbers in an_clusters and an_deps to be those of the partition
-        // and the input dependencies, annotated with the representative of the Cluster partition
+        // representative.
-        // it applies to.
+        auto deps_it = an_deps.begin();
-        an_deps.reserve(clusterset.m_deps_to_add.size());
-        auto deps_it = clusterset.m_deps_to_add.begin();
         for (size_t i = 0; i < partition_data.size(); ++i) {
             auto& data = partition_data[i];
-            // Find the representative of the partition Cluster i is in, and store it with the
+            // Find the sequence of the representative of the partition Cluster i is in, and store
-            // Cluster.
+            // it with the Cluster.
-            auto rep = find_root_fn(&data)->cluster;
+            auto rep_seq = find_root_fn(&data)->sequence;
-            Assume(an_clusters[i].second == nullptr);
+            an_clusters[i].second = rep_seq;
-            an_clusters[i].second = rep;
             // Find all dependencies whose child Cluster is Cluster i, and annotate them with rep.
-            while (deps_it != clusterset.m_deps_to_add.end()) {
+            while (deps_it != an_deps.end()) {
-                auto [par, chl] = *deps_it;
+                auto [par, chl] = deps_it->first;
                 auto chl_cluster = FindCluster(chl, level);
-                if (std::greater{}(chl_cluster, data.cluster)) break;
+                Assume(chl_cluster != nullptr);
-                // Skip dependencies that apply to earlier Clusters (those necessary are for
+                if (chl_cluster->m_sequence > data.sequence) break;
-                // deleted transactions, as otherwise we'd have processed them already).
+                deps_it->second = rep_seq;
-                if (chl_cluster == data.cluster) {
-                    auto par_cluster = FindCluster(par, level);
-                    // Also filter out dependencies applying to a removed parent.
-                    if (par_cluster != nullptr) an_deps.emplace_back(*deps_it, rep);
-                }
                 ++deps_it;
             }
         }
     }
 
-    // Sort both an_clusters and an_deps by representative of the partition they are in, grouping
+    // Sort both an_clusters and an_deps by sequence number of the representative of the
-    // all those applying to the same partition together.
+    // partition they are in, grouping all those applying to the same partition together.
     std::sort(an_deps.begin(), an_deps.end(), [](auto& a, auto& b) noexcept { return a.second < b.second; });
     std::sort(an_clusters.begin(), an_clusters.end(), [](auto& a, auto& b) noexcept { return a.second < b.second; });
 
@@ -1390,7 +1402,10 @@ void TxGraphImpl::MakeAllAcceptable(int level) noexcept
     }
 }
 
-Cluster::Cluster(TxGraphImpl& graph, const FeePerWeight& feerate, GraphIndex graph_index) noexcept
+Cluster::Cluster(uint64_t sequence) noexcept : m_sequence{sequence} {}
+
+Cluster::Cluster(uint64_t sequence, TxGraphImpl& graph, const FeePerWeight& feerate, GraphIndex graph_index) noexcept :
+    m_sequence{sequence}
 {
     // Create a new transaction in the DepGraph, and remember its position in m_mapping.
     auto cluster_idx = m_depgraph.AddTransaction(feerate);
@@ -1410,7 +1425,7 @@ TxGraph::Ref TxGraphImpl::AddTransaction(const FeePerWeight& feerate) noexcept
     GetRefGraph(ret) = this;
     GetRefIndex(ret) = idx;
     // Construct a new singleton Cluster (which is necessarily optimally linearized).
-    auto cluster = std::make_unique<Cluster>(*this, feerate, idx);
+    auto cluster = std::make_unique<Cluster>(m_next_sequence_counter++, *this, feerate, idx);
     auto cluster_ptr = cluster.get();
     int level = GetTopLevel();
     auto& clusterset = GetClusterSet(level);
@@ -1606,7 +1621,7 @@ std::vector<TxGraph::Ref*> TxGraphImpl::GetAncestorsUnion(std::span<const Ref* c
         matches.emplace_back(cluster, m_entries[GetRefIndex(*arg)].m_locator[cluster->m_level].index);
     }
     // Group by Cluster.
-    std::sort(matches.begin(), matches.end(), [](auto& a, auto& b) noexcept { return std::less{}(a.first, b.first); });
+    std::sort(matches.begin(), matches.end(), [](auto& a, auto& b) noexcept { return CompareClusters(a.first, b.first) < 0; });
     // Dispatch to the Clusters.
     std::span match_span(matches);
     std::vector<TxGraph::Ref*> ret;
@@ -1639,7 +1654,7 @@ std::vector<TxGraph::Ref*> TxGraphImpl::GetDescendantsUnion(std::span<const Ref*
         matches.emplace_back(cluster, m_entries[GetRefIndex(*arg)].m_locator[cluster->m_level].index);
     }
     // Group by Cluster.
-    std::sort(matches.begin(), matches.end(), [](auto& a, auto& b) noexcept { return std::less{}(a.first, b.first); });
+    std::sort(matches.begin(), matches.end(), [](auto& a, auto& b) noexcept { return CompareClusters(a.first, b.first) < 0; });
     // Dispatch to the Clusters.
     std::span match_span(matches);
     std::vector<TxGraph::Ref*> ret;
@@ -1867,7 +1882,9 @@ std::strong_ordering TxGraphImpl::CompareMainOrder(const Ref& a, const Ref& b) n
     if (feerate_cmp < 0) return std::strong_ordering::less;
     if (feerate_cmp > 0) return std::strong_ordering::greater;
     // Compare Cluster* as tie-break for equal chunk feerates.
-    if (locator_a.cluster != locator_b.cluster) return locator_a.cluster <=> locator_b.cluster;
+    if (locator_a.cluster != locator_b.cluster) {
+        return CompareClusters(locator_a.cluster, locator_b.cluster);
+    }
     // As final tie-break, compare position within cluster linearization.
     return entry_a.m_main_lin_index <=> entry_b.m_main_lin_index;
 }
@@ -1889,7 +1906,7 @@ TxGraph::GraphIndex TxGraphImpl::CountDistinctClusters(std::span<const Ref* cons
         if (cluster != nullptr) clusters.push_back(cluster);
     }
     // Count the number of distinct elements in clusters.
-    std::sort(clusters.begin(), clusters.end());
+    std::sort(clusters.begin(), clusters.end(), [](Cluster* a, Cluster* b) noexcept { return CompareClusters(a, b) < 0; });
     Cluster* last{nullptr};
     GraphIndex ret{0};
     for (Cluster* cluster : clusters) {
@@ -1951,6 +1968,8 @@ void TxGraphImpl::SanityCheck() const
     std::set<const Cluster*> expected_clusters[MAX_LEVELS];
     /** Which GraphIndexes ought to occur in ClusterSet::m_removed, based on m_entries. */
     std::set<GraphIndex> expected_removed[MAX_LEVELS];
+    /** Which Cluster::m_sequence values have been encountered. */
+    std::set<uint64_t> sequences;
     /** Whether compaction is possible in the current state. */
     bool compact_possible{true};
 
@@ -2004,6 +2023,10 @@ void TxGraphImpl::SanityCheck() const
             // ... for all clusters in them ...
             for (ClusterSetIndex setindex = 0; setindex < quality_clusters.size(); ++setindex) {
                 const auto& cluster = *quality_clusters[setindex];
+                // Check the sequence number.
+                assert(cluster.m_sequence < m_next_sequence_counter);
+                assert(sequences.count(cluster.m_sequence) == 0);
+                sequences.insert(cluster.m_sequence);
                 // Remember we saw this Cluster (only if it is non-empty; empty Clusters aren't
                 // expected to be referenced by the Entry vector).
                 if (cluster.GetTxCount() != 0) {