Merge bitcoin/bitcoin#32177: TxGraph: Increase fuzz coverage

a40bd37 Get*Union: disallow nulltpr Refs (Greg Sanders)
5743350 CountDistinctClusters: nullptrs disallowed (Greg Sanders)
8bca0d3 TxGraphImpl::Compact: m_main_clusterset.m_removed is always empty (Greg Sanders)
2c5cf98 TxGraphImpl::PullIn: only allowed when staging exists (Greg Sanders)

Pull request description:

  Was looking at my local coverage report, and noticed a few spots that will not or cannot be hit.

  CountDistinctClusters, GetAncestorsUnion, and GetDescendantsUnion accept nullptrs, but the test harness never employs them. Disallow them.

  We never call PullIn whenever there isn't staging, so just enforce that invariant via assertion.

  Remaining places that are not covered:

  1) Relinearize: Currently we seem to always start with a cold (not known to be optimal) cluster, and after one attempt at linearization result into something optimal. This means we never shortcircuit, nor run PostLinearization, nor store the quality as ACCEPTABLE. Reducing iterations causes these lines to be hit. sipa says he will take this on as varying the amount of iterations was meant to be done eventually anyways.
  2) We never do a move assignment operator when the lvalue already has a `m_graph` (so we never call UnlinkRef) https://github.com/bitcoin/bitcoin/blob/3358b1d105196647230e6f828b8ec820426b96a0/src/txgraph.cpp#L2097
  3) We never use the move constructor: https://github.com/bitcoin/bitcoin/blob/3358b1d105196647230e6f828b8ec820426b96a0/src/txgraph.cpp#L2108

ACKs for top commit:
  sipa:
    utACK a40bd37
  glozow:
    utACK a40bd37

Tree-SHA512: ca88297222e80e0d590889698899f892b9335cfa587a76a6c6ca62c8d846f208b6b0b9a9b1829bafabdb929a1a0c3a75f23edf7dd2b4f5e2dad0235e5bc68ba3

Author: fanquake

src/txgraph.cpp

@@ -411,8 +411,8 @@ class TxGraphImpl final : public TxGraph
      *  values for remaining Entry objects, so this only does something when no to-be-applied
      *  operations or staged removals referring to GraphIndexes remain). */
     void Compact() noexcept;
-    /** If cluster is not in staging, copy it there, and return a pointer to it. This has no
-    *   effect if only a main graph exists, but if staging exists this modifies the locators of its
+    /** If cluster is not in staging, copy it there, and return a pointer to it.
+    *   Staging must exist, and this modifies the locators of its
     *   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
@@ -928,7 +928,7 @@ Cluster* TxGraphImpl::FindCluster(GraphIndex idx, int level) const noexcept
 Cluster* TxGraphImpl::PullIn(Cluster* cluster) noexcept
 {
     int to_level = GetTopLevel();
-    if (to_level == 0) return cluster;
+    Assume(to_level == 1);
     int level = cluster->m_level;
     Assume(level <= to_level);
     // Copy the Cluster from main to staging, if it's not already there.
@@ -1008,7 +1008,7 @@ void TxGraphImpl::Compact() noexcept
     // to rewrite them. It is easier to delay the compaction until they have been applied.
     if (!m_main_clusterset.m_deps_to_add.empty()) return;
     if (!m_main_clusterset.m_to_remove.empty()) return;
-    if (!m_main_clusterset.m_removed.empty()) return;
+    Assume(m_main_clusterset.m_removed.empty()); // non-staging m_removed is always empty
     if (m_staging_clusterset.has_value()) {
         if (!m_staging_clusterset->m_deps_to_add.empty()) return;
         if (!m_staging_clusterset->m_to_remove.empty()) return;
@@ -1595,6 +1595,7 @@ std::vector<TxGraph::Ref*> TxGraphImpl::GetAncestorsUnion(std::span<const Ref* c
     std::vector<std::pair<Cluster*, DepGraphIndex>> matches;
     matches.reserve(args.size());
     for (auto arg : args) {
+        Assume(arg);
         // Skip empty Refs.
         if (GetRefGraph(*arg) == nullptr) continue;
         Assume(GetRefGraph(*arg) == this);
@@ -1627,6 +1628,7 @@ std::vector<TxGraph::Ref*> TxGraphImpl::GetDescendantsUnion(std::span<const Ref*
     std::vector<std::pair<Cluster*, DepGraphIndex>> matches;
     matches.reserve(args.size());
     for (auto arg : args) {
+        Assume(arg);
         // Skip empty Refs.
         if (GetRefGraph(*arg) == nullptr) continue;
         Assume(GetRefGraph(*arg) == this);
@@ -1880,7 +1882,7 @@ TxGraph::GraphIndex TxGraphImpl::CountDistinctClusters(std::span<const Ref* cons
     std::vector<Cluster*> clusters;
     clusters.reserve(refs.size());
     for (const Ref* ref : refs) {
-        if (ref == nullptr) continue;
+        Assume(ref);
         if (GetRefGraph(*ref) == nullptr) continue;
         Assume(GetRefGraph(*ref) == this);
         auto cluster = FindCluster(GetRefIndex(*ref), level);

src/txgraph.h

@@ -144,11 +144,11 @@ class TxGraph
     virtual std::vector<Ref*> GetDescendants(const Ref& arg, bool main_only = false) noexcept = 0;
     /** Like GetAncestors, but return the Refs for all transactions in the union of the provided
      *  arguments' ancestors (each transaction is only reported once). Refs that do not exist in
-     *  the queried graph are ignored. */
+     *  the queried graph are ignored. Null refs are not allowed. */
     virtual std::vector<Ref*> GetAncestorsUnion(std::span<const Ref* const> args, bool main_only = false) noexcept = 0;
     /** Like GetDescendants, but return the Refs for all transactions in the union of the provided
      *  arguments' descendants (each transaction is only reported once). Refs that do not exist in
-     *  the queried graph are ignored. */
+     *  the queried graph are ignored. Null refs are not allowed. */
     virtual std::vector<Ref*> GetDescendantsUnion(std::span<const Ref* const> args, bool main_only = false) noexcept = 0;
     /** Get the total number of transactions in the graph. If main_only is false and a staging
      *  graph exists, it is queried; otherwise the main graph is queried. This is available even
@@ -159,8 +159,8 @@ class TxGraph
     virtual std::strong_ordering CompareMainOrder(const Ref& a, const Ref& b) noexcept = 0;
     /** Count the number of distinct clusters that the specified transactions belong to. If
      *  main_only is false and a staging graph exists, staging clusters are counted. Otherwise,
-     *  main clusters are counted. Refs that do not exist in the queried graph are ignored. The
-     *  queried graph must not be oversized. */
+     *  main clusters are counted. Refs that do not exist in the queried graph are ignored. Refs
+     *  can not be null. The queried graph must not be oversized. */
     virtual GraphIndex CountDistinctClusters(std::span<const Ref* const>, bool main_only = false) noexcept = 0;
 
     /** Perform an internal consistency check on this object. */