Revert "[refactor] rewrite vTxHashes as a vector of CTransactionRef"
What the developer wrote
Authored by Anthony Towns
Revert "[refactor] rewrite vTxHashes as a vector of CTransactionRef"
This reverts commit a03aef9cec35b0d03aa63d7e8093f0420cd4b40b.
What changed, and why it matters
This commit undoes a recent code cleanup that changed how Bitcoin Core's memory pool tracks transactions for compact block reconstruction. The reversion restores the previous design where the mempool keeps pairs of transaction IDs and iterators, rather than storing direct shared references to transactions. There is no indication in the commit itself that this fixes a security bug; it appears to be a design or correctness revert, possibly due to bugs introduced by the original refactor.
No immediate security action is required based on this commit alone. If the revert was triggered by a latent bug in the original refactor, monitor follow-up commits, release notes, and the Bitcoin Core issue tracker for additional context or a more targeted fix.
Security signals we found
No security-relevant language in commit title or message
Change is a pure revert of a prior refactor
No mention of vulnerability, bug, exploit, DoS, consensus, or privacy
No CVE, advisory, or researcher attribution in commit metadata
Evidence from the diff
The patch reverts commit a03aef9cec35b0d03aa63d7e8093f0420cd4b40b, which had rewritten CTxMemPool::txns_randomized from std::vector<std::pair<Wtxid, txiter>> to std::vector<CTransactionRef>. The revert restores the pair-based structure, updates addNewTransaction and removeUnchecked accordingly, and adjusts blockencodings.cpp to use the stored wtxid and to call GetSharedTx() via the iterator. The test file’s expected shared-transaction reference count is lowered from 4 to 3, matching the removal of one extra CTransactionRef holder in the randomized vector. The commit message gives only ‘Revert’ with no security framing.
Changed components
src/txmempool.hsrc/txmempool.cppsrc/blockencodings.cppsrc/test/blockencodings_tests.cppInspect captured patch +12 / −11
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index f0984fa0..cf6da55a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -114,12 +114,12 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
std::vector<bool> have_txn(txn_available.size());
{
LOCK(pool->cs);
- for (const auto& tx : pool->txns_randomized) {
- uint64_t shortid = cmpctblock.GetShortID(tx->GetWitnessHash());
+ for (const auto& [wtxid, txit] : pool->txns_randomized) {
+ uint64_t shortid = cmpctblock.GetShortID(wtxid);
std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
if (idit != shorttxids.end()) {
if (!have_txn[idit->second]) {
- txn_available[idit->second] = tx;
+ txn_available[idit->second] = txit->GetSharedTx();
have_txn[idit->second] = true;
mempool_count++;
} else {
diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp
index 82293fd5..641617b7 100644
--- a/src/test/blockencodings_tests.cpp
+++ b/src/test/blockencodings_tests.cpp
@@ -56,8 +56,8 @@ static CBlock BuildBlockTestCase(FastRandomContext& ctx) {
}
// Number of shared use_counts we expect for a tx we haven't touched
-// (block + mempool entry + mempool txns_randomized + our copy from the GetSharedTx call)
-constexpr long SHARED_TX_OFFSET{4};
+// (block + mempool entry + our copy from the GetSharedTx call)
+constexpr long SHARED_TX_OFFSET{3};
BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
{
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 6bb910e3..cae16a67 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -507,7 +507,7 @@ void CTxMemPool::addNewTransaction(CTxMemPool::txiter newit, CTxMemPool::setEntr
totalTxSize += entry.GetTxSize();
m_total_fee += entry.GetFee();
- txns_randomized.emplace_back(newit->GetSharedTx());
+ txns_randomized.emplace_back(tx.GetWitnessHash(), newit);
newit->idx_randomized = txns_randomized.size() - 1;
TRACEPOINT(mempool, added,
@@ -544,15 +544,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
RemoveUnbroadcastTx(it->GetTx().GetHash(), true /* add logging because unchecked */);
if (txns_randomized.size() > 1) {
- // Update idx_randomized of the to-be-moved entry.
- Assert(GetEntry(txns_randomized.back()->GetHash()))->idx_randomized = it->idx_randomized;
// Remove entry from txns_randomized by replacing it with the back and deleting the back.
txns_randomized[it->idx_randomized] = std::move(txns_randomized.back());
+ txns_randomized[it->idx_randomized].second->idx_randomized = it->idx_randomized;
txns_randomized.pop_back();
- if (txns_randomized.size() * 2 < txns_randomized.capacity())
+ if (txns_randomized.size() * 2 < txns_randomized.capacity()) {
txns_randomized.shrink_to_fit();
- } else
+ }
+ } else {
txns_randomized.clear();
+ }
totalTxSize -= it->GetTxSize();
m_total_fee -= it->GetFee();
diff --git a/src/txmempool.h b/src/txmempool.h
index f471726c..a3675481 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -368,7 +368,7 @@ public:
indexed_transaction_set mapTx GUARDED_BY(cs);
using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
- std::vector<CTransactionRef> txns_randomized GUARDED_BY(cs); //!< All transactions in mapTx, in random order
+ std::vector<std::pair<Wtxid, txiter>> txns_randomized GUARDED_BY(cs); //!< All transactions in mapTx with their wtxids, in arbitrary order
typedef std::set<txiter, CompareIteratorByHash> setEntries;
Why this scored 12/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.