test: ensure group data cluster pointers are live #35506

instagibbs commented at 4:33 PM on June 10, 2026: member

Belt-and-suspenders check inside SanityCheck to avoid dangling pointers.

DrahtBot added the label Tests on Jun 10, 2026

DrahtBot commented at 4:33 PM on June 10, 2026: 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/35506.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label CI failed on Jun 10, 2026

DrahtBot commented at 5:56 PM on June 10, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/27290635825/job/80609942701</sub> <sub>LLM reason (✨ experimental): CI failed because clang-tidy reported a readability-container-contains issue treated as an error in txgraph.cpp (line 3072: using count(...) > 0 instead of contains).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

test: ensure group data cluster pointers are live 065ba41700

instagibbs force-pushed on Jun 10, 2026

DrahtBot removed the label CI failed on Jun 10, 2026

in src/txgraph.cpp:3071 in 065ba41700

3066 | +        // level or below (staging group data may reference not-yet-pulled-in
3067 | +        // main clusters).
3068 | +        if (clusterset.m_group_data.has_value()) {
3069 | +            for (const Cluster* cl : clusterset.m_group_data->m_group_clusters) {
3070 | +                bool found{false};
3071 | +                for (int l = 0; l <= level; ++l) {

l0rinc commented at 7:36 AM on June 11, 2026:

Instead of looping over the levels again, we could record the live clusters as SanityCheck() encounters them and check group-data liveness directly (while preserving the original non-empty implication):

diff --git a/src/txgraph.cpp b/src/txgraph.cpp
--- a/src/txgraph.cpp	(revision 065ba41700d7716870e3dcbb5b3756e0b7919b20)
+++ b/src/txgraph.cpp	(revision b1396ef91ff83a6999be0c7b278acf9c16235a9f)
@@ -2939,6 +2939,8 @@
     std::set<GraphIndex> expected_removed[MAX_LEVELS];
     /** Which Cluster::m_sequence values have been encountered. */
     std::set<uint64_t> sequences;
+    /** Which Clusters are live in ClusterSet::m_clusters up to the level being checked. */
+    std::set<const Cluster*> live_clusters;
     /** Which GraphIndexes ought to occur in m_main_chunkindex, based on m_entries. */
     std::set<GraphIndex> expected_chunkindex;
     /** Whether compaction is possible in the current state. */
@@ -3000,6 +3002,7 @@
             // ... for all clusters in them ...
             for (ClusterSetIndex setindex = 0; setindex < quality_clusters.size(); ++setindex) {
                 const auto& cluster = *quality_clusters[setindex];
+                live_clusters.insert(&cluster);
                 // The number of transactions in a Cluster cannot exceed m_max_cluster_count.
                 assert(cluster.GetTxCount() <= m_max_cluster_count);
                 // The level must match the Cluster's own idea of what level it is in (but GetLevel
@@ -3067,11 +3070,8 @@
         // main clusters).
         if (clusterset.m_group_data.has_value()) {
             for (const Cluster* cl : clusterset.m_group_data->m_group_clusters) {
-                bool found{false};
-                for (int l = 0; l <= level; ++l) {
-                    found |= expected_clusters[l].contains(cl);
-                }
-                assert(found);
+                assert(live_clusters.contains(cl));
+                assert(cl->GetTxCount() > 0);
             }
         }
         // Verify that the contents of m_removed matches what was expected based on the Entry vector.

in src/txgraph.cpp:3074 in 065ba41700

3069 | +            for (const Cluster* cl : clusterset.m_group_data->m_group_clusters) {
3070 | +                bool found{false};
3071 | +                for (int l = 0; l <= level; ++l) {
3072 | +                    found |= expected_clusters[l].contains(cl);
3073 | +                }
3074 | +                assert(found);

l0rinc commented at 7:41 AM on June 11, 2026:

After the liveness and non-empty checks, would it make sense to use one entry from the cluster to confirm that FindCluster(..., level) resolves back to the same cluster?

diff --git a/src/txgraph.cpp b/src/txgraph.cpp
--- a/src/txgraph.cpp	(revision ed9f1eb9b2d8123a7062ab9f969a3b61ffb223b8)
+++ b/src/txgraph.cpp	(revision a3c551eb5722d5f1ed5d82a5564e7938d288a89a)
@@ -3072,6 +3072,7 @@
             for (const Cluster* cl : clusterset.m_group_data->m_group_clusters) {
                 assert(live_clusters.contains(cl));
                 assert(cl->GetTxCount() > 0);
+                assert(FindCluster(cl->GetClusterEntry(0), level) == cl);
             }
         }
         // Verify that the contents of m_removed matches what was expected based on the Entry vector.

l0rinc changes_requested