TxGraph: Increase fuzz coverage #32177

1. 
   instagibbs commented at 5:17 pm on March 31, 2025: member

   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
2. TxGraphImpl::PullIn: only allowed when staging exists 2c5cf987e9
3. TxGraphImpl::Compact: m_main_clusterset.m_removed is always empty 8bca0d325a
4. CountDistinctClusters: nullptrs disallowed 57433502e6
5. Get*Union: disallow nulltpr Refs a40bd374aa
6. 
   DrahtBot commented at 5:17 pm on March 31, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32177.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sipa, glozow

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

7. 
   sipa commented at 6:17 pm on March 31, 2025: member

   utACK a40bd374aaf8e10116fa664fa7b480d85ee2fe59
8. fanquake requested review from glozow on Apr 1, 2025
9. in src/txgraph.cpp:1885 in a40bd374aa

   1881@@ -1880,7 +1882,7 @@ TxGraph::GraphIndex TxGraphImpl::CountDistinctClusters(std::span<const Ref* cons
   1882     std::vector<Cluster*> clusters;
   1883     clusters.reserve(refs.size());
   1884     for (const Ref* ref : refs) {
   1885-        if (ref == nullptr) continue;
   1886+        Assume(ref);
   

   ---

   ---

   

   glozow commented at 4:23 pm on April 1, 2025:

   Do you want to retain the safe exit if this isn’t true? if (!Assume(ref)) continue; ?

   ---

   

   instagibbs commented at 5:08 pm on April 1, 2025:

   Current thinking and elsewhere in TxGraph we don’t take the cost of a conditional if we’re confident that the code cannot be reached by a valid invocation by the caller.

   See #31363 (review) , #31363 (review) , and #32100 (comment) for more discussion

10. 
    glozow commented at 6:48 pm on April 2, 2025: member

    utACK a40bd374aaf8e10116fa664fa7b480d85ee2fe59
11. fanquake merged this on Apr 3, 2025
12. fanquake closed this on Apr 3, 2025

    ---

Contributors
instagibbs DrahtBot sipa glozow