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.

Type	Reviewers
ACK	l0rinc, marcofleon, sipa, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

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>

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.

instagibbs commented at 2:07 PM on June 11, 2026:

taken. I think the TxCount thing is redundant since expected_clusters would only track non-empty ones, but it's fine to have for clarity for the FindCluster call

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.

instagibbs commented at 2:06 PM on June 11, 2026:

I think so, it shows that it's not only pointing to a live cluster, but the cluster materializing the txn at this level. Taken.

l0rinc changes_requested

test: ensure group data cluster pointers are live df9eb72b12

instagibbs force-pushed on Jun 11, 2026

fanquake commented at 12:36 PM on June 12, 2026: member

cc @marcofleon @sipa

l0rinc commented at 8:17 PM on June 12, 2026: contributor

code review ACK df9eb72b129b999eb5e327c51fd580ba9910a778

Someone with more experience in this area should also review.

sedited requested review from ismaelsadeeq on Jun 16, 2026

marcofleon approved

marcofleon commented at 12:00 PM on June 26, 2026: contributor

lgtm ACK df9eb72b129b999eb5e327c51fd580ba9910a778

Pretty sure this is the one remaining Cluster* storage worth checking. Entry locator cluster pointers are already checked earlier in SanityCheck(), and I don’t think any of the other Cluster* uses persist long enough.

sipa commented at 12:07 PM on June 26, 2026: member

Cannot-hurt ACK df9eb72b129b999eb5e327c51fd580ba9910a778

marcofleon commented at 12:10 PM on June 26, 2026: contributor

Cannot-hurt ACK

This should be a new standard.

sedited approved

sedited commented at 12:21 PM on June 26, 2026: contributor

ACK df9eb72b129b999eb5e327c51fd580ba9910a778

sedited merged this on Jun 26, 2026

sedited closed this on Jun 26, 2026