txgraph: clear cluster's chunk index in ~Ref (preparation)
What the developer wrote
Authored by Pieter Wuille
txgraph: clear cluster's chunk index in ~Ref (preparation)
Whenever a TxGraph::Ref is destroyed, if it by then still appears inside
main-level clusters, wipe the chunk index entries for those clusters, to
prevent having lingering indexes for transactions without Ref.
This is preparation for enabling a callback being passed to MakeTxGraph
to define a fallback order on objects. Once the Ref for a transaction is
gone, it is not possible to invoke the callback anymore. To prevent the
index becoming inconsistent, we need to immediately get rid of the index
entries when the Ref disappears.
This is not a problem, because such destructions necessarily will
trigger a relinearization of the cluster (assuming there are
transactions in it left) before becoming acceptable again, and the chunk
ordering is not observable (through CompareMainOrder, or through the
BlockBuilder interface) until that point. However, the index itself
needs to remain consistent in the mean time, even if not meaningful.
What changed, and why it matters
This is a small internal cleanup in Bitcoin Core's new transaction graph code. It makes sure that when a transaction reference object is destroyed, any cached 'chunk index' bookkeeping for its cluster is wiped out. The commit message explicitly says this is preparation for a future feature and is not currently a problem, because the index is not observable by outside code until the cluster is reprocessed anyway. There is no direct evidence this fixes an exploitable security bug.
Treat as routine defensive maintenance. Review the follow-up change that enables the MakeTxGraph callback to confirm the preparation is correctly used. No urgent action required based on this commit alone.
Security signals we found
Internal consistency hardening for transaction graph chunk index
Destructor-time cleanup of dangling references
Assertion relaxation tied to removal-in-progress state
Preparatory change for future callback ordering feature
Evidence from the diff
The patch adds a RemoveChunkData() virtual method to Cluster implementations and calls it from TxGraphImpl::~Ref (the destructor path for Ref objects) when the Ref is still present in a main-level cluster. It also adjusts SanityCheck assertions to skip chunk-index consistency checks while m_to_remove is non-empty. The stated purpose is to keep the internal chunk index consistent after a Ref disappears, in preparation for a future callback-based fallback ordering. The commit message says the chunk ordering is not observable through CompareMainOrder or BlockBuilder until relinearization occurs, so this is defensive/preparatory rather than a fix for an active vulnerability.
Changed components
src/txgraph.cppGenericClusterImplSingletonClusterImplTxGraphImpl::Ref destructorInspect captured patch +39 / −11
diff --git a/src/txgraph.cpp b/src/txgraph.cpp
index 3e3e4a68..ac07579e 100644
--- a/src/txgraph.cpp
+++ b/src/txgraph.cpp
@@ -187,6 +187,8 @@ public:
* when called from Compact, to recompute after GraphIndexes may have changed; in this case,
* no chunk index objects are removed or created either. */
virtual void Updated(TxGraphImpl& graph, int level, bool rename) noexcept = 0;
+ /** Remove all chunk index entries for this cluster (level=0 only). */
+ virtual void RemoveChunkData(TxGraphImpl& graph) noexcept = 0;
/** Create a copy of this Cluster in staging, returning a pointer to it (used by PullIn). */
virtual Cluster* CopyToStaging(TxGraphImpl& graph) const noexcept = 0;
/** Get the list of Clusters in main that conflict with this one (which is assumed to be in staging). */
@@ -283,6 +285,7 @@ public:
int GetLevel(const TxGraphImpl& graph) const noexcept final;
void UpdateMapping(DepGraphIndex cluster_idx, GraphIndex graph_idx) noexcept final { m_mapping[cluster_idx] = graph_idx; }
void Updated(TxGraphImpl& graph, int level, bool rename) noexcept final;
+ void RemoveChunkData(TxGraphImpl& graph) noexcept final;
Cluster* CopyToStaging(TxGraphImpl& graph) const noexcept final;
void GetConflicts(const TxGraphImpl& graph, std::vector<Cluster*>& out) const noexcept final;
void MakeStagingTransactionsMissing(TxGraphImpl& graph) noexcept final;
@@ -339,6 +342,7 @@ public:
int GetLevel(const TxGraphImpl& graph) const noexcept final;
void UpdateMapping(DepGraphIndex cluster_idx, GraphIndex graph_idx) noexcept final { Assume(cluster_idx == 0); m_graph_index = graph_idx; }
void Updated(TxGraphImpl& graph, int level, bool rename) noexcept final;
+ void RemoveChunkData(TxGraphImpl& graph) noexcept final;
Cluster* CopyToStaging(TxGraphImpl& graph) const noexcept final;
void GetConflicts(const TxGraphImpl& graph, std::vector<Cluster*>& out) const noexcept final;
void MakeStagingTransactionsMissing(TxGraphImpl& graph) noexcept final;
@@ -697,6 +701,11 @@ public:
auto& entry = m_entries[idx];
Assume(entry.m_ref != nullptr);
Assume(m_main_chunkindex_observers == 0 || !entry.m_locator[0].IsPresent());
+ // Remove all chunk index entries for the affected cluster, to avoid any chunk indexes
+ // referencing unlinked/destroyed Refs.
+ if (entry.m_locator[0].IsPresent()) {
+ entry.m_locator[0].cluster->RemoveChunkData(*this);
+ }
entry.m_ref = nullptr;
// Mark the transaction as to be removed in all levels where it explicitly or implicitly
// exists.
@@ -1011,6 +1020,21 @@ void TxGraphImpl::ClearLocator(int level, GraphIndex idx, bool oversized_tx) noe
if (level == 0) ClearChunkData(entry);
}
+void GenericClusterImpl::RemoveChunkData(TxGraphImpl& graph) noexcept
+{
+ for (DepGraphIndex idx : m_linearization) {
+ auto& entry = graph.m_entries[m_mapping[idx]];
+ graph.ClearChunkData(entry);
+ }
+}
+
+void SingletonClusterImpl::RemoveChunkData(TxGraphImpl& graph) noexcept
+{
+ if (GetTxCount() == 0) return;
+ auto& entry = graph.m_entries[m_graph_index];
+ graph.ClearChunkData(entry);
+}
+
void GenericClusterImpl::Updated(TxGraphImpl& graph, int level, bool rename) noexcept
{
// Update all the Locators for this Cluster's Entry objects.
@@ -2777,14 +2801,16 @@ void GenericClusterImpl::SanityCheck(const TxGraphImpl& graph, int level) const
assert(entry.m_main_chunk_feerate == linchunking[chunk_num].feerate);
// Verify that an entry in the chunk index exists for every chunk-ending transaction.
++chunk_pos;
- bool is_chunk_end = (chunk_pos == linchunking[chunk_num].transactions.Count());
- assert((entry.m_main_chunkindex_iterator != graph.m_main_chunkindex.end()) == is_chunk_end);
- if (is_chunk_end) {
- auto& chunk_data = *entry.m_main_chunkindex_iterator;
- if (m_done == m_depgraph.Positions() && chunk_pos == 1) {
- assert(chunk_data.m_chunk_count == LinearizationIndex(-1));
- } else {
- assert(chunk_data.m_chunk_count == chunk_pos);
+ if (graph.m_main_clusterset.m_to_remove.empty()) {
+ bool is_chunk_end = (chunk_pos == linchunking[chunk_num].transactions.Count());
+ assert((entry.m_main_chunkindex_iterator != graph.m_main_chunkindex.end()) == is_chunk_end);
+ if (is_chunk_end) {
+ auto& chunk_data = *entry.m_main_chunkindex_iterator;
+ if (m_done == m_depgraph.Positions() && chunk_pos == 1) {
+ assert(chunk_data.m_chunk_count == LinearizationIndex(-1));
+ } else {
+ assert(chunk_data.m_chunk_count == chunk_pos);
+ }
}
}
// If this Cluster has an acceptable quality level, its chunks must be connected.
@@ -2808,9 +2834,11 @@ void SingletonClusterImpl::SanityCheck(const TxGraphImpl& graph, int level) cons
if (level == 0 && IsAcceptable()) {
assert(entry.m_main_lin_index == 0);
assert(entry.m_main_chunk_feerate == m_feerate);
- assert(entry.m_main_chunkindex_iterator != graph.m_main_chunkindex.end());
- auto& chunk_data = *entry.m_main_chunkindex_iterator;
- assert(chunk_data.m_chunk_count == LinearizationIndex(-1));
+ if (graph.m_main_clusterset.m_to_remove.empty()) {
+ assert(entry.m_main_chunkindex_iterator != graph.m_main_chunkindex.end());
+ auto& chunk_data = *entry.m_main_chunkindex_iterator;
+ assert(chunk_data.m_chunk_count == LinearizationIndex(-1));
+ }
}
}
}
Why this scored 18/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.