refactor: remove dead branches in SingletonClusterImpl #33768

1. 
   l0rinc commented at 11:20 am on November 3, 2025: contributor

   Found during review: cluster mempool: control/optimize TxGraph memory usage

   Fixes

   SplitAll() always calls ApplyRemovals() first, for a singleton, it empties the cluster, therefore any SingletonClusterImpl passed to Split() must be empty.

   TxGraphImpl::ApplyDependencies() first merges each dependency group and asserts the group has at least one dependency. Since parent != child, TxGraphImpl::Merge() upgrades the merge target to GenericClusterImpl, therefore the ApplyDependencies() is never dispatched to SingletonClusterImpl.

   Coverage proof:

   • https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446
   • https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20251103/linux/src/bitcoin-core/src/txgraph.cpp.html#L1446
2. refactor: remove dead branches in `SingletonClusterImpl`

   `SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
   
   `TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency.
   Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
   
   Found during review: https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423058928
   Coverage evidence:
   * https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446
   * https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20251103/linux/src/bitcoin-core/src/txgraph.cpp.html#L1446

   2d23820ee1
3. DrahtBot added the label Refactoring on Nov 3, 2025
4. 
   DrahtBot commented at 11:20 am on November 3, 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/33768.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	sipa, instagibbs

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

5. l0rinc marked this as ready for review on Nov 3, 2025
6. 
   sipa commented at 12:53 pm on November 3, 2025: member

   ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
7. fanquake requested review from instagibbs on Nov 3, 2025
8. 
   instagibbs commented at 11:57 am on November 5, 2025: member

   ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
9. in src/txgraph.cpp:1442 in 2d23820ee1

   1447-        Updated(graph, level);
   1448-        return false;
   1449-    }
   1450+    Assume(!GetTxCount());
   1451+    graph.GetClusterSet(level).m_cluster_usage -= TotalMemoryUsage();
   1452+    return true;
   

   ---

   ---

   

   flack commented at 8:12 pm on November 6, 2025:

   this now always returns true. Is that needed for anything?

   ---

   

   l0rinc commented at 8:31 pm on November 6, 2025:

   Good question, I have already checked that, the sibling GenericClusterImpl::Split can return false which would trigger deletion.

   ---

   

   sipa commented at 8:36 pm on November 6, 2025:

   Yeah. It’s indeed the case that within SingletonClusterImpl it’ll always return true, but this isn’t the case for all possible Cluster implementations.
10. fanquake merged this on Nov 7, 2025
11. fanquake closed this on Nov 7, 2025

    ---

12. l0rinc deleted the branch on Nov 7, 2025

Contributors
l0rinc DrahtBot sipa instagibbs flack

Review Requested
instagibbs

Labels
Refactoring