Merge bitcoin/bitcoin#33354: txgraph: use enum Level instead of bool main_only

d45f371 txgraph: use enum Level instead of bool main_only (Pieter Wuille)

Pull request description:

  Part of #30289. Inspired by bitcoin/bitcoin#28676 (comment).

  Since there has been more than one case in the development of #28676 of calling a `TxGraph` function without correctly setting the `bool main_only` argument that many of its interface functions have, make these mandatory and explicit, using an `enum class Level`:

  ```c++
  enum class Level {
      TOP, //!< Refers to staging if it exists, main otherwise.
      MAIN //!< Always refers to the main graph, whether staging is present or not.
  };
  ```

ACKs for top commit:
  instagibbs:
    ACK d45f371
  vasild:
    ACK d45f371
  glozow:
    code review ACK d45f371

Tree-SHA512: d1c4b37e8ab3ec91b414df8970cb47aa080803f68da5881c8e1cbdc6939dea7851e0f715192cf3edd44b7f328cd6b678474d41f9cd9da8cb68f6c5fd78cb71b1

Author: glozow

src/bench/txgraph.cpp

@@ -114,9 +114,9 @@ void BenchTxGraphTrim(benchmark::Bench& bench)
         graph->GetBlockBuilder();
     });
 
-    assert(!graph->IsOversized());
+    assert(!graph->IsOversized(TxGraph::Level::TOP));
     // At least 99% of chains must survive.
-    assert(graph->GetTransactionCount() >= (NUM_TOP_CHAINS * NUM_TX_PER_TOP_CHAIN * 99) / 100);
+    assert(graph->GetTransactionCount(TxGraph::Level::TOP) >= (NUM_TOP_CHAINS * NUM_TX_PER_TOP_CHAIN * 99) / 100);
 }
 
 } // namespace

src/test/fuzz/txgraph.cpp

@@ -383,14 +383,14 @@ FUZZ_TARGET(txgraph)
 
     /** Function to construct the correct fee-size diagram a real graph has based on its graph
      *  order (as reported by GetCluster(), so it works for both main and staging). */
-    auto get_diagram_fn = [&](bool main_only) -> std::vector<FeeFrac> {
-        int level = main_only ? 0 : sims.size() - 1;
+    auto get_diagram_fn = [&](TxGraph::Level level_select) -> std::vector<FeeFrac> {
+        int level = level_select == TxGraph::Level::MAIN ? 0 : sims.size() - 1;
         auto& sim = sims[level];
         // For every transaction in the graph, request its cluster, and throw them into a set.
         std::set<std::vector<TxGraph::Ref*>> clusters;
         for (auto i : sim.graph.Positions()) {
             auto ref = sim.GetRef(i);
-            clusters.insert(real->GetCluster(*ref, main_only));
+            clusters.insert(real->GetCluster(*ref, level_select));
         }
         // Compute the chunkings of each (deduplicated) cluster.
         size_t num_tx{0};
@@ -423,19 +423,19 @@ FUZZ_TARGET(txgraph)
         // operations), and the second-lowest bit as a way of selecting main vs. staging, and leave
         // the rest of the bits in command.
         bool alt = command & 1;
-        bool use_main = command & 2;
+        TxGraph::Level level_select = (command & 2) ? TxGraph::Level::MAIN : TxGraph::Level::TOP;
         command >>= 2;
 
         /** Use the bottom 2 bits of command to select an entry in the block_builders vector (if
-         *  any). These use the same bits as alt/use_main, so don't use those in actions below
+         *  any). These use the same bits as alt/level_select, so don't use those in actions below
          *  where builder_idx is used as well. */
         int builder_idx = block_builders.empty() ? -1 : int((orig_command & 3) % block_builders.size());
 
         // Provide convenient aliases for the top simulated graph (main, or staging if it exists),
-        // one for the simulated graph selected based on use_main (for operations that can operate
+        // one for the simulated graph selected based on level_select (for operations that can operate
         // on both graphs), and one that always refers to the main graph.
         auto& top_sim = sims.back();
-        auto& sel_sim = use_main ? sims[0] : top_sim;
+        auto& sel_sim = level_select == TxGraph::Level::MAIN ? sims[0] : top_sim;
         auto& main_sim = sims[0];
 
         // Keep decrementing command for each applicable operation, until one is hit. Multiple
@@ -546,18 +546,18 @@ FUZZ_TARGET(txgraph)
                 break;
             } else if (command-- == 0) {
                 // GetTransactionCount.
-                assert(real->GetTransactionCount(use_main) == sel_sim.GetTransactionCount());
+                assert(real->GetTransactionCount(level_select) == sel_sim.GetTransactionCount());
                 break;
             } else if (command-- == 0) {
                 // Exists.
                 auto ref = pick_fn();
-                bool exists = real->Exists(*ref, use_main);
+                bool exists = real->Exists(*ref, level_select);
                 bool should_exist = sel_sim.Find(ref) != SimTxGraph::MISSING;
                 assert(exists == should_exist);
                 break;
             } else if (command-- == 0) {
                 // IsOversized.
-                assert(sel_sim.IsOversized() == real->IsOversized(use_main));
+                assert(sel_sim.IsOversized() == real->IsOversized(level_select));
                 break;
             } else if (command-- == 0) {
                 // GetIndividualFeerate.
@@ -590,8 +590,8 @@ FUZZ_TARGET(txgraph)
             } else if (!sel_sim.IsOversized() && command-- == 0) {
                 // GetAncestors/GetDescendants.
                 auto ref = pick_fn();
-                auto result = alt ? real->GetDescendants(*ref, use_main)
-                                  : real->GetAncestors(*ref, use_main);
+                auto result = alt ? real->GetDescendants(*ref, level_select)
+                                  : real->GetAncestors(*ref, level_select);
                 assert(result.size() <= max_cluster_count);
                 auto result_set = sel_sim.MakeSet(result);
                 assert(result.size() == result_set.Count());
@@ -610,8 +610,8 @@ FUZZ_TARGET(txgraph)
                 // Their order should not matter, shuffle them.
                 std::shuffle(refs.begin(), refs.end(), rng);
                 // Invoke the real function, and convert to SimPos set.
-                auto result = alt ? real->GetDescendantsUnion(refs, use_main)
-                                  : real->GetAncestorsUnion(refs, use_main);
+                auto result = alt ? real->GetDescendantsUnion(refs, level_select)
+                                  : real->GetAncestorsUnion(refs, level_select);
                 auto result_set = sel_sim.MakeSet(result);
                 assert(result.size() == result_set.Count());
                 // Compute the expected result.
@@ -623,7 +623,7 @@ FUZZ_TARGET(txgraph)
             } else if (!sel_sim.IsOversized() && command-- == 0) {
                 // GetCluster.
                 auto ref = pick_fn();
-                auto result = real->GetCluster(*ref, use_main);
+                auto result = real->GetCluster(*ref, level_select);
                 // Check cluster count limit.
                 assert(result.size() <= max_cluster_count);
                 // Require the result to be topologically valid and not contain duplicates.
@@ -712,7 +712,7 @@ FUZZ_TARGET(txgraph)
                 // Their order should not matter, shuffle them.
                 std::shuffle(refs.begin(), refs.end(), rng);
                 // Invoke the real function.
-                auto result = real->CountDistinctClusters(refs, use_main);
+                auto result = real->CountDistinctClusters(refs, level_select);
                 // Build a set with representatives of the clusters the Refs occur in in the
                 // simulated graph. For each, remember the lowest-index transaction SimPos in the
                 // cluster.
@@ -905,7 +905,7 @@ FUZZ_TARGET(txgraph)
 
                 // First, we need to have dependencies applied and linearizations fixed to avoid
                 // circular dependencies in implied graph; trigger it via whatever means.
-                real->CountDistinctClusters({}, false);
+                real->CountDistinctClusters({}, TxGraph::Level::TOP);
 
                 // Gather the current clusters.
                 auto clusters = top_sim.GetComponents();
@@ -1110,7 +1110,7 @@ FUZZ_TARGET(txgraph)
 
         // Check that the implied ordering gives rise to a combined diagram that matches the
         // diagram constructed from the individual cluster linearization chunkings.
-        auto main_real_diagram = get_diagram_fn(/*main_only=*/true);
+        auto main_real_diagram = get_diagram_fn(TxGraph::Level::MAIN);
         auto main_implied_diagram = ChunkLinearization(sims[0].graph, vec1);
         assert(CompareChunks(main_real_diagram, main_implied_diagram) == 0);
 
@@ -1141,7 +1141,7 @@ FUZZ_TARGET(txgraph)
                                 std::greater{});
             assert(main_cmp_diagram.size() + missing_main_cmp.size() == main_real_diagram.size());
             // Do the same for chunks in stage_diagram missing from stage_cmp_diagram.
-            auto stage_real_diagram = get_diagram_fn(/*main_only=*/false);
+            auto stage_real_diagram = get_diagram_fn(TxGraph::Level::TOP);
             std::vector<FeeFrac> missing_stage_cmp;
             std::set_difference(stage_real_diagram.begin(), stage_real_diagram.end(),
                                 stage_cmp_diagram.begin(), stage_cmp_diagram.end(),
@@ -1167,13 +1167,13 @@ FUZZ_TARGET(txgraph)
 
     assert(real->HaveStaging() == (sims.size() > 1));
 
-    // Try to run a full comparison, for both main_only=false and main_only=true in TxGraph
-    // inspector functions that support both.
-    for (int main_only = 0; main_only < 2; ++main_only) {
-        auto& sim = main_only ? sims[0] : sims.back();
+    // Try to run a full comparison, for both TxGraph::Level::MAIN and TxGraph::Level::TOP in
+    // TxGraph inspector functions that support both.
+    for (auto level : {TxGraph::Level::TOP, TxGraph::Level::MAIN}) {
+        auto& sim = level == TxGraph::Level::TOP ? sims.back() : sims.front();
         // Compare simple properties of the graph with the simulation.
-        assert(real->IsOversized(main_only) == sim.IsOversized());
-        assert(real->GetTransactionCount(main_only) == sim.GetTransactionCount());
+        assert(real->IsOversized(level) == sim.IsOversized());
+        assert(real->GetTransactionCount(level) == sim.GetTransactionCount());
         // If the graph (and the simulation) are not oversized, perform a full comparison.
         if (!sim.IsOversized()) {
             auto todo = sim.graph.Positions();
@@ -1188,16 +1188,16 @@ FUZZ_TARGET(txgraph)
                     assert(sim.graph.FeeRate(i) == real->GetIndividualFeerate(*sim.GetRef(i)));
                     // Check its ancestors against simulation.
                     auto expect_anc = sim.graph.Ancestors(i);
-                    auto anc = sim.MakeSet(real->GetAncestors(*sim.GetRef(i), main_only));
+                    auto anc = sim.MakeSet(real->GetAncestors(*sim.GetRef(i), level));
                     assert(anc.Count() <= max_cluster_count);
                     assert(anc == expect_anc);
                     // Check its descendants against simulation.
                     auto expect_desc = sim.graph.Descendants(i);
-                    auto desc = sim.MakeSet(real->GetDescendants(*sim.GetRef(i), main_only));
+                    auto desc = sim.MakeSet(real->GetDescendants(*sim.GetRef(i), level));
                     assert(desc.Count() <= max_cluster_count);
                     assert(desc == expect_desc);
                     // Check the cluster the transaction is part of.
-                    auto cluster = real->GetCluster(*sim.GetRef(i), main_only);
+                    auto cluster = real->GetCluster(*sim.GetRef(i), level);
                     assert(cluster.size() <= max_cluster_count);
                     assert(sim.MakeSet(cluster) == component);
                     // Check that the cluster is reported in a valid topological order (its
@@ -1217,7 +1217,7 @@ FUZZ_TARGET(txgraph)
                     assert(total_size <= max_cluster_size);
                     // Construct a chunking object for the simulated graph, using the reported cluster
                     // linearization as ordering, and compare it against the reported chunk feerates.
-                    if (sims.size() == 1 || main_only) {
+                    if (sims.size() == 1 || level == TxGraph::Level::MAIN) {
                         cluster_linearize::LinearizationChunking simlinchunk(sim.graph, simlin);
                         DepGraphIndex idx{0};
                         for (unsigned chunknum = 0; chunknum < simlinchunk.NumChunksLeft(); ++chunknum) {

src/test/txgraph_tests.cpp

@@ -64,19 +64,19 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_zigzag)
     // Check that the graph is now oversized. This also forces the graph to
     // group clusters and compute the oversized status.
     graph->SanityCheck();
-    BOOST_CHECK_EQUAL(graph->GetTransactionCount(), NUM_TOTAL_TX);
-    BOOST_CHECK(graph->IsOversized(/*main_only=*/false));
+    BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), NUM_TOTAL_TX);
+    BOOST_CHECK(graph->IsOversized(TxGraph::Level::TOP));
 
     // Call Trim() to remove transactions and bring the cluster back within limits.
     auto removed_refs = graph->Trim();
     graph->SanityCheck();
-    BOOST_CHECK(!graph->IsOversized(/*main_only=*/false));
+    BOOST_CHECK(!graph->IsOversized(TxGraph::Level::TOP));
 
     // We only need to trim the middle bottom transaction to end up with 2 clusters each within cluster limits.
     BOOST_CHECK_EQUAL(removed_refs.size(), 1);
-    BOOST_CHECK_EQUAL(graph->GetTransactionCount(), MAX_CLUSTER_COUNT * 2 - 2);
+    BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), MAX_CLUSTER_COUNT * 2 - 2);
     for (unsigned int i = 0; i < refs.size(); ++i) {
-        BOOST_CHECK_EQUAL(graph->Exists(refs[i]), i != (NUM_BOTTOM_TX / 2));
+        BOOST_CHECK_EQUAL(graph->Exists(refs[i], TxGraph::Level::TOP), i != (NUM_BOTTOM_TX / 2));
     }
 }
 
@@ -123,19 +123,19 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_flower)
 
     // Check that the graph is now oversized. This also forces the graph to
     // group clusters and compute the oversized status.
-    BOOST_CHECK(graph->IsOversized(/*main_only=*/false));
+    BOOST_CHECK(graph->IsOversized(TxGraph::Level::TOP));
 
     // Call Trim() to remove transactions and bring the cluster back within limits.
     auto removed_refs = graph->Trim();
     graph->SanityCheck();
-    BOOST_CHECK(!graph->IsOversized(/*main_only=*/false));
+    BOOST_CHECK(!graph->IsOversized(TxGraph::Level::TOP));
 
     // Since only the bottom transaction connects these clusters, we only need to remove it.
     BOOST_CHECK_EQUAL(removed_refs.size(), 1);
-    BOOST_CHECK_EQUAL(graph->GetTransactionCount(false), MAX_CLUSTER_COUNT * 2);
-    BOOST_CHECK(!graph->Exists(refs[0]));
+    BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), MAX_CLUSTER_COUNT * 2);
+    BOOST_CHECK(!graph->Exists(refs[0], TxGraph::Level::TOP));
     for (unsigned int i = 1; i < refs.size(); ++i) {
-        BOOST_CHECK(graph->Exists(refs[i]));
+        BOOST_CHECK(graph->Exists(refs[i], TxGraph::Level::TOP));
     }
 }
 
@@ -208,7 +208,7 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_huge)
     graph->SanityCheck();
 
     // Not oversized so far (just 1000 clusters of 64).
-    BOOST_CHECK(!graph->IsOversized());
+    BOOST_CHECK(!graph->IsOversized(TxGraph::Level::TOP));
 
     // Construct the bottom transactions, and dependencies to the top chains.
     while (top_components.size() > 1) {
@@ -237,19 +237,19 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_huge)
     graph->SanityCheck();
 
     // Now we are oversized (one cluster of 64011).
-    BOOST_CHECK(graph->IsOversized());
-    const auto total_tx_count = graph->GetTransactionCount();
+    BOOST_CHECK(graph->IsOversized(TxGraph::Level::TOP));
+    const auto total_tx_count = graph->GetTransactionCount(TxGraph::Level::TOP);
     BOOST_CHECK(total_tx_count == top_refs.size() + bottom_refs.size());
     BOOST_CHECK(total_tx_count == NUM_TOTAL_TX);
 
     // Call Trim() to remove transactions and bring the cluster back within limits.
     auto removed_refs = graph->Trim();
-    BOOST_CHECK(!graph->IsOversized());
-    BOOST_CHECK(removed_refs.size() == total_tx_count - graph->GetTransactionCount());
+    BOOST_CHECK(!graph->IsOversized(TxGraph::Level::TOP));
+    BOOST_CHECK(removed_refs.size() == total_tx_count - graph->GetTransactionCount(TxGraph::Level::TOP));
     graph->SanityCheck();
 
     // At least 99% of chains must survive.
-    BOOST_CHECK(graph->GetTransactionCount() >= (NUM_TOP_CHAINS * NUM_TX_PER_TOP_CHAIN * 99) / 100);
+    BOOST_CHECK(graph->GetTransactionCount(TxGraph::Level::TOP) >= (NUM_TOP_CHAINS * NUM_TX_PER_TOP_CHAIN * 99) / 100);
 }
 
 BOOST_AUTO_TEST_CASE(txgraph_trim_big_singletons)
@@ -277,17 +277,17 @@ BOOST_AUTO_TEST_CASE(txgraph_trim_big_singletons)
 
     // Check that the graph is now oversized. This also forces the graph to
     // group clusters and compute the oversized status.
-    BOOST_CHECK(graph->IsOversized(/*main_only=*/false));
+    BOOST_CHECK(graph->IsOversized(TxGraph::Level::TOP));
 
     // Call Trim() to remove transactions and bring the cluster back within limits.
     auto removed_refs = graph->Trim();
     graph->SanityCheck();
-    BOOST_CHECK_EQUAL(graph->GetTransactionCount(), NUM_TOTAL_TX - 6);
-    BOOST_CHECK(!graph->IsOversized(/*main_only=*/false));
+    BOOST_CHECK_EQUAL(graph->GetTransactionCount(TxGraph::Level::TOP), NUM_TOTAL_TX - 6);
+    BOOST_CHECK(!graph->IsOversized(TxGraph::Level::TOP));
 
     // Check that all the oversized transactions were removed.
     for (unsigned int i = 0; i < refs.size(); ++i) {
-        BOOST_CHECK_EQUAL(graph->Exists(refs[i]), i != 88 && i % 20 != 0);
+        BOOST_CHECK_EQUAL(graph->Exists(refs[i], TxGraph::Level::TOP), i != 88 && i % 20 != 0);
     }
 }