Revert "refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef>"
What changed, and why it matters
This commit undoes a recent code cleanup in Bitcoin Core's compact block reconstruction feature. The original cleanup changed an internal list of spare transactions from storing pairs of 'transaction ID + transaction pointer' to storing only the transaction pointer. This revert restores the paired format and adds a comment explaining that keeping both values together improves CPU cache performance during block reconstruction. There is no direct evidence in the commit that this fixes an active security bug, but it is a defensive revert of a performance refactor in a consensus-adjacent code path.
Treat as a low-signal defensive revert. Review the original refactor for any subtle correctness or performance issues in compact block reconstruction, especially around witness hash computation and cache behavior. No immediate emergency action is indicated by this commit alone.
Security signals we found
Revert of a recent refactor in compact block reconstruction code
Change removes repeated witness-hash computation from a hot path
Touches consensus-adjacent block reconstruction logic
No explicit bug, CVE, or vulnerability description in commit message
Evidence from the diff
The commit reverts a refactor (a8203e94) that simplified extra_txn from std::vector<std::pair<Wtxid, CTransactionRef>> to std::vector<CTransactionRef>. It restores the paired vector across blockencodings.cpp/h, net_processing.cpp, benchmarks, unit tests, and fuzz tests. A new comment in PartiallyDownloadedBlock::InitData justifies the paired format for CPU cache locality during compact block reconstruction. The functional change is that InitData no longer calls GetWitnessHash() on each extra transaction during reconstruction; instead it uses the precomputed Wtxid.
Changed components
src/blockencodings.cppsrc/blockencodings.hsrc/net_processing.cppsrc/bench/blockencodings.cppsrc/test/blockencodings_tests.cppsrc/test/fuzz/partially_downloaded_block.cppInspect captured patch +24 / −19
diff --git a/src/bench/blockencodings.cpp b/src/bench/blockencodings.cpp
index 7e942395..8f665991 100644
--- a/src/bench/blockencodings.cpp
+++ b/src/bench/blockencodings.cpp
@@ -65,7 +65,7 @@ static void BlockEncodingBench(benchmark::Bench& bench, size_t n_pool, size_t n_
LOCK2(cs_main, pool.cs);
- std::vector<CTransactionRef> extratxn;
+ std::vector<std::pair<Wtxid, CTransactionRef>> extratxn;
extratxn.reserve(n_extra);
// bump up the size of txs
@@ -94,7 +94,7 @@ static void BlockEncodingBench(benchmark::Bench& bench, size_t n_pool, size_t n_
AddTx(refs[i], /*fee=*/refs[i]->vout[0].nValue, pool);
}
for (size_t i = n_pool; i < n_pool + n_extra; ++i) {
- extratxn.push_back(refs[i]);
+ extratxn.emplace_back(refs[i]->GetWitnessHash(), refs[i]);
}
BenchCBHAST cmpctblock{rng, 3000};
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 3f61a122..f0984fa0 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -45,7 +45,15 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
return SipHashUint256(shorttxidk0, shorttxidk1, wtxid.ToUint256()) & 0xffffffffffffL;
}
-ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<CTransactionRef>& extra_txn) {
+/* Reconstructing a compact block is in the hot-path for block relay,
+ * so we want to do it as quickly as possible. Because this often
+ * involves iterating over the entire mempool, we put all the data we
+ * need (ie the wtxid and a reference to the actual transaction data)
+ * in a vector and iterate over the vector directly. This allows optimal
+ * CPU caching behaviour, at a cost of only 40 bytes per transaction.
+ */
+ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<Wtxid, CTransactionRef>>& extra_txn)
+{
LogDebug(BCLog::CMPCTBLOCK, "Initializing PartiallyDownloadedBlock for block %s using a cmpctblock of %u bytes\n", cmpctblock.header.GetHash().ToString(), GetSerializeSize(cmpctblock));
if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty()))
return READ_STATUS_INVALID;
@@ -133,14 +141,11 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
}
for (size_t i = 0; i < extra_txn.size(); i++) {
- if (extra_txn[i] == nullptr) {
- continue;
- }
- uint64_t shortid = cmpctblock.GetShortID(extra_txn[i]->GetWitnessHash());
+ uint64_t shortid = cmpctblock.GetShortID(extra_txn[i].first);
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] = extra_txn[i];
+ txn_available[idit->second] = extra_txn[i].second;
have_txn[idit->second] = true;
mempool_count++;
extra_count++;
@@ -152,7 +157,7 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
// Note that we don't want duplication between extra_txn and mempool to
// trigger this case, so we compare witness hashes first
if (txn_available[idit->second] &&
- txn_available[idit->second]->GetWitnessHash() != extra_txn[i]->GetWitnessHash()) {
+ txn_available[idit->second]->GetWitnessHash() != extra_txn[i].second->GetWitnessHash()) {
txn_available[idit->second].reset();
mempool_count--;
extra_count--;
diff --git a/src/blockencodings.h b/src/blockencodings.h
index fce59bc5..133724b6 100644
--- a/src/blockencodings.h
+++ b/src/blockencodings.h
@@ -144,8 +144,8 @@ public:
explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
- // extra_txn is a list of extra orphan/conflicted/etc transactions to look at
- ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<CTransactionRef>& extra_txn);
+ // extra_txn is a list of extra transactions to look at, in <witness hash, reference> form
+ ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<Wtxid, CTransactionRef>>& extra_txn);
bool IsTxAvailable(size_t index) const;
// segwit_active enforces witness mutation checks just before reporting a healthy status
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active);
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index f2c65391..e1674b6d 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -974,7 +974,7 @@ private:
/** Orphan/conflicted/etc transactions that are kept for compact block reconstruction.
* The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of
* these are kept in a ring buffer */
- std::vector<CTransactionRef> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
+ std::vector<std::pair<Wtxid, CTransactionRef>> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
/** Offset into vExtraTxnForCompact to insert the next tx */
size_t vExtraTxnForCompactIt GUARDED_BY(g_msgproc_mutex) = 0;
@@ -1768,7 +1768,7 @@ void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
return;
if (!vExtraTxnForCompact.size())
vExtraTxnForCompact.resize(m_opts.max_extra_txs);
- vExtraTxnForCompact[vExtraTxnForCompactIt] = tx;
+ vExtraTxnForCompact[vExtraTxnForCompactIt] = std::make_pair(tx->GetWitnessHash(), tx);
vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs;
}
diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp
index b3d21096..82293fd5 100644
--- a/src/test/blockencodings_tests.cpp
+++ b/src/test/blockencodings_tests.cpp
@@ -14,7 +14,7 @@
#include <boost/test/unit_test.hpp>
-const std::vector<CTransactionRef> empty_extra_txn;
+const std::vector<std::pair<Wtxid, CTransactionRef>> empty_extra_txn;
BOOST_FIXTURE_TEST_SUITE(blockencodings_tests, RegTestingSetup)
@@ -318,7 +318,7 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) {
const CTransactionRef non_block_tx = MakeTransactionRef(std::move(mtx));
CBlock block(BuildBlockTestCase(rand_ctx));
- std::vector<CTransactionRef> extra_txn;
+ std::vector<std::pair<Wtxid, CTransactionRef>> extra_txn;
extra_txn.resize(10);
LOCK2(cs_main, pool.cs);
@@ -342,9 +342,9 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) {
BOOST_CHECK( partial_block.IsTxAvailable(2));
// Add an unrelated tx to extra_txn:
- extra_txn[0] = non_block_tx;
+ extra_txn[0] = {non_block_tx->GetWitnessHash(), non_block_tx};
// and a tx from the block that's not in the mempool:
- extra_txn[1] = block.vtx[1];
+ extra_txn[1] = {block.vtx[1]->GetWitnessHash(), block.vtx[1]};
BOOST_CHECK(partial_block_with_extra.InitData(cmpctblock, extra_txn) == READ_STATUS_OK);
BOOST_CHECK(partial_block_with_extra.IsTxAvailable(0));
diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp
index e0c99749..a39b51d7 100644
--- a/src/test/fuzz/partially_downloaded_block.cpp
+++ b/src/test/fuzz/partially_downloaded_block.cpp
@@ -67,7 +67,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
// The coinbase is always available
available.insert(0);
- std::vector<CTransactionRef> extra_txn;
+ std::vector<std::pair<Wtxid, CTransactionRef>> extra_txn;
for (size_t i = 1; i < block->vtx.size(); ++i) {
auto tx{block->vtx[i]};
@@ -75,7 +75,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
bool add_to_mempool{fuzzed_data_provider.ConsumeBool()};
if (add_to_extra_txn) {
- extra_txn.emplace_back(tx);
+ extra_txn.emplace_back(tx->GetWitnessHash(), tx);
available.insert(i);
}
Why this scored 27/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.