clusterlin: keep FIFO queue of improvable chunks (preparation)
What changed, and why it matters
This commit is a routine internal refactoring of Bitcoin Core's transaction-cluster linearization algorithm. It replaces a loop that scans all transactions with a FIFO queue of 'chunks' that might still be improvable. The change is described by the author as preparation for future randomization, a fairness improvement, and a performance optimization. There is no indication in the commit or supplied references that this fixes a security bug or is security-relevant.
No security action required. Review as normal code-quality/algorithmic change.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch modifies src/cluster_linearize.h to introduce a VecDeque
Changed components
src/cluster_linearize.hsrc/test/fuzz/cluster_linearize.cppsrc/test/util/cluster_linearize.hInspect captured patch +73 / −37
diff --git a/src/cluster_linearize.h b/src/cluster_linearize.h
index 64610ee7..4d0d7c55 100644
--- a/src/cluster_linearize.h
+++ b/src/cluster_linearize.h
@@ -690,8 +690,7 @@ public:
* transaction in the other chunk is activated (this will be changed in a later commit).
*
* - How to decide which chunk to find a dependency to split in:
- * - The chunk with the lowest-index representative (an implementation detail) that can be split
- * is picked (this will be changed in a later commit).
+ * - A round-robin queue of chunks to improve is maintained.
*
* - How to decide what dependency to deactivate (when splitting chunks):
* - Inside the selected chunk (see above), among the dependencies whose top feerate is strictly
@@ -742,6 +741,8 @@ private:
std::vector<TxData> m_tx_data;
/** Information about each dependency. Indexed by DepIdx. */
std::vector<DepData> m_dep_data;
+ /** A FIFO of chunk representatives of chunks that may be improved still. */
+ VecDeque<TxIdx> m_suboptimal_chunks;
/** The number of updated transactions in activations/deactivations. */
uint64_t m_cost{0};
@@ -959,6 +960,8 @@ private:
if (merged_rep == TxIdx(-1)) break;
chunk_rep = merged_rep;
}
+ // Add the chunk to the queue of improvable chunks.
+ m_suboptimal_chunks.push_back(chunk_rep);
}
/** Split a chunk, and then merge the resulting two chunks to make the graph topological
@@ -1043,38 +1046,58 @@ public:
/** Make state topological. Can be called after constructing, or after LoadLinearization. */
void MakeTopological() noexcept
{
- while (true) {
- bool done = true;
- // Iterate over all transactions (only processing those which are chunk representatives).
- for (auto chunk : m_transaction_idxs) {
- auto& chunk_data = m_tx_data[chunk];
- // If this is not a chunk representative, skip.
- if (chunk_data.chunk_rep != chunk) continue;
- // Attempt to merge the chunk upwards.
- auto result_up = MergeStep<false>(chunk);
- if (result_up != TxIdx(-1)) {
- done = false;
- continue;
- }
- // Attempt to merge the chunk downwards.
- auto result_down = MergeStep<true>(chunk);
- if (result_down != TxIdx(-1)) {
- done = false;
- continue;
- }
+ for (auto tx : m_transaction_idxs) {
+ auto& tx_data = m_tx_data[tx];
+ if (tx_data.chunk_rep == tx) {
+ m_suboptimal_chunks.emplace_back(tx);
+ }
+ }
+ while (!m_suboptimal_chunks.empty()) {
+ // Pop an entry from the potentially-suboptimal chunk queue.
+ TxIdx chunk = m_suboptimal_chunks.front();
+ m_suboptimal_chunks.pop_front();
+ auto& chunk_data = m_tx_data[chunk];
+ // If what was popped is not currently a chunk representative, continue. This may
+ // happen when it was merged with something else since being added.
+ if (chunk_data.chunk_rep != chunk) continue;
+ // Attempt to merge the chunk upwards.
+ auto result_up = MergeStep<false>(chunk);
+ if (result_up != TxIdx(-1)) {
+ m_suboptimal_chunks.push_back(result_up);
+ continue;
+ }
+ // Attempt to merge the chunk downwards.
+ auto result_down = MergeStep<true>(chunk);
+ if (result_down != TxIdx(-1)) {
+ m_suboptimal_chunks.push_back(result_down);
+ continue;
+ }
+ }
+ }
+
+ /** Initialize the data structure for optimization. It must be topological already. */
+ void StartOptimizing() noexcept
+ {
+ // Mark chunks suboptimal.
+ for (auto tx : m_transaction_idxs) {
+ auto& tx_data = m_tx_data[tx];
+ if (tx_data.chunk_rep == tx) {
+ m_suboptimal_chunks.push_back(tx);
}
- // Stop if no changes were made anymore.
- if (done) break;
}
}
/** Try to improve the forest. Returns false if it is optimal, true otherwise. */
bool OptimizeStep() noexcept
{
- // Iterate over all transactions (only processing those which are chunk representatives).
- for (auto chunk : m_transaction_idxs) {
+ while (!m_suboptimal_chunks.empty()) {
+ // Pop an entry from the potentially-suboptimal chunk queue.
+ TxIdx chunk = m_suboptimal_chunks.front();
+ m_suboptimal_chunks.pop_front();
auto& chunk_data = m_tx_data[chunk];
- // If this is not a chunk representative, skip.
+ // If what was popped is not currently a chunk representative, continue. This may
+ // happen when a split chunk merges in Improve() with one or more existing chunks that
+ // are themselves on the suboptimal queue already.
if (chunk_data.chunk_rep != chunk) continue;
// Iterate over all transactions of the chunk.
for (auto tx : chunk_data.chunk_setinfo.transactions) {
@@ -1344,6 +1367,14 @@ public:
assert(dep_data.top_setinfo.feerate ==
depgraph.FeeRate(dep_data.top_setinfo.transactions));
}
+
+ //
+ // Verify m_suboptimal_chunks.
+ //
+ for (size_t i = 0; i < m_suboptimal_chunks.size(); ++i) {
+ auto tx_idx = m_suboptimal_chunks[i];
+ assert(m_transaction_idxs[tx_idx]);
+ }
}
};
@@ -1377,11 +1408,14 @@ std::tuple<std::vector<DepGraphIndex>, bool, uint64_t> Linearize(const DepGraph<
// Make improvement steps to it until we hit the max_iterations limit, or an optimal result
// is found.
bool optimal = false;
- while (forest.GetCost() < max_iterations) {
- if (!forest.OptimizeStep()) {
- optimal = true;
- break;
- }
+ if (forest.GetCost() < max_iterations) {
+ forest.StartOptimizing();
+ do {
+ if (!forest.OptimizeStep()) {
+ optimal = true;
+ break;
+ }
+ } while (forest.GetCost() < max_iterations);
}
return {forest.GetLinearization(), optimal, forest.GetCost()};
}
diff --git a/src/test/fuzz/cluster_linearize.cpp b/src/test/fuzz/cluster_linearize.cpp
index b4d52344..fe18407d 100644
--- a/src/test/fuzz/cluster_linearize.cpp
+++ b/src/test/fuzz/cluster_linearize.cpp
@@ -1068,6 +1068,8 @@ FUZZ_TARGET(clusterlin_sfl)
}
// Loop until optimal.
+ test_fn();
+ sfl.StartOptimizing();
while (true) {
test_fn();
if (!sfl.OptimizeStep()) break;
diff --git a/src/test/util/cluster_linearize.h b/src/test/util/cluster_linearize.h
index d4126063..2e73032e 100644
--- a/src/test/util/cluster_linearize.h
+++ b/src/test/util/cluster_linearize.h
@@ -404,12 +404,12 @@ inline uint64_t MaxOptimalLinearizationIters(DepGraphIndex cluster_count)
0,
0, 2, 8, 21, 51, 99, 162, 208,
300, 349, 489, 627, 776, 867, 982, 1204,
- 1414, 1473, 1770, 2045, 2391, 2417, 3669, 3953,
- 3816, 5717, 4096, 5933, 5225, 5684, 6205, 6407,
- 7671, 12044, 11799, 9577, 9631, 10819, 12277, 15250,
- 18609, 14439, 22283, 16461, 22887, 20641, 22009, 22053,
- 27068, 22173, 31066, 30848, 31841, 37174, 39701, 35666,
- 42728, 43679, 45719, 40217, 51395, 57796, 72739, 60079
+ 1414, 1473, 1770, 2045, 2285, 2417, 3669, 3953,
+ 3816, 5720, 4103, 5934, 5443, 5323, 6338, 6407,
+ 7671, 11625, 11799, 10104, 9631, 11203, 12487, 15262,
+ 17800, 14132, 21915, 16495, 23350, 21304, 22221, 22230,
+ 26119, 22182, 31118, 30848, 32166, 37174, 39708, 36189,
+ 42747, 43689, 46555, 39818, 51077, 58489, 72633, 59756
};
assert(cluster_count < std::size(ITERS));
// Multiply the table number by two, to account for the fact that they are not absolutes.
Why this scored 14/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.