refactor: remove dead branches in `SingletonClusterImpl`
What the developer wrote
Authored by Lőrinc
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
What changed, and why it matters
This is a code cleanup change in Bitcoin Core's internal transaction graph logic. It removes branches of code that the author proved can never actually run, replacing them with explicit 'this should never happen' assumptions. There is no security bug being fixed; it is purely a refactoring to simplify the code and document invariants.
No security action required. Treat as a normal refactoring review; verify the author's invariants hold and that the new Assume assertions are acceptable for the project's debug/assumption policy.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The commit refactors SingletonClusterImpl::Split, Merge, and ApplyDependencies in src/txgraph.cpp. The author argues that prior callers always leave a singleton cluster empty before Split, never dispatch Merge to a singleton (because Merge upgrades the target to GenericClusterImpl when parent != child), and never dispatch ApplyDependencies to a singleton (because dependency groups are merged first and have at least one dependency). The dead branches are removed and replaced with Assume(false) or Assume(!GetTxCount()). Coverage links are provided showing the removed lines were never executed.
Changed components
src/txgraph.cppSingletonClusterImpl::SplitSingletonClusterImpl::MergeSingletonClusterImpl::ApplyDependenciesInspect captured patch +7 / −18
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index 3a18f60c..ae85c546 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -1437,16 +1437,9 @@ bool GenericClusterImpl::Split(TxGraphImpl& graph, int level) noexcept
bool SingletonClusterImpl::Split(TxGraphImpl& graph, int level) noexcept
{
Assume(NeedsSplitting());
- if (GetTxCount() == 0) {
- // The cluster is now empty.
- graph.GetClusterSet(level).m_cluster_usage -= TotalMemoryUsage();
- return true;
- } else {
- // Nothing changed.
- graph.SetClusterQuality(level, m_quality, m_setindex, QualityLevel::OPTIMAL);
- Updated(graph, level);
- return false;
- }
+ Assume(!GetTxCount());
+ graph.GetClusterSet(level).m_cluster_usage -= TotalMemoryUsage();
+ return true;
}
void GenericClusterImpl::Merge(TxGraphImpl& graph, int level, Cluster& other) noexcept
@@ -1482,10 +1475,9 @@ void GenericClusterImpl::Merge(TxGraphImpl& graph, int level, Cluster& other) no
});
}
-void SingletonClusterImpl::Merge(TxGraphImpl& graph, int level, Cluster& other_abstract) noexcept
+void SingletonClusterImpl::Merge(TxGraphImpl&, int, Cluster&) noexcept
{
- // Nothing can be merged into a singleton; it should have been converted to GenericClusterImpl
- // first.
+ // Nothing can be merged into a singleton; it should have been converted to GenericClusterImpl first.
Assume(false);
}
@@ -1537,13 +1529,10 @@ void GenericClusterImpl::ApplyDependencies(TxGraphImpl& graph, int level, std::s
Updated(graph, level);
}
-void SingletonClusterImpl::ApplyDependencies(TxGraphImpl& graph, int level, std::span<std::pair<GraphIndex, GraphIndex>> to_apply) noexcept
+void SingletonClusterImpl::ApplyDependencies(TxGraphImpl&, int, std::span<std::pair<GraphIndex, GraphIndex>>) noexcept
{
// Nothing can actually be applied.
- for (auto& [par, chl] : to_apply) {
- Assume(par == m_graph_index);
- Assume(chl == m_graph_index);
- }
+ Assume(false);
}
TxGraphImpl::~TxGraphImpl() noexcept
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.