From b9300d8d0a74b15a220a2ce529d5157d109c7ed3 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Mon, 25 Aug 2025 15:58:59 +1000 Subject: [PATCH] Revert "refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair" This reverts commit a8203e94123b6ea6e4f4a6320e3ad20457f44a28. --- src/bench/blockencodings.cpp | 4 ++-- src/blockencodings.cpp | 19 ++++++++++++------- src/blockencodings.h | 4 ++-- src/net_processing.cpp | 4 ++-- src/test/blockencodings_tests.cpp | 8 ++++---- src/test/fuzz/partially_downloaded_block.cpp | 4 ++-- 6 files changed, 24 insertions(+), 19 deletions(-) diff --git a/src/bench/blockencodings.cpp b/src/bench/blockencodings.cpp index 7e9423953c9..8f6659919c6 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 extratxn; + std::vector> 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 3f61a122e67..f0984fa09e1 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& 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>& 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::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 fce59bc5614..133724b64e8 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& extra_txn); + // extra_txn is a list of extra transactions to look at, in form + ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector>& 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& vtx_missing, bool segwit_active); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f2c6539132d..e1674b6d514 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 vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex); + std::vector> 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 b3d21096879..82293fd5207 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -14,7 +14,7 @@ #include -const std::vector empty_extra_txn; +const std::vector> 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 extra_txn; + std::vector> 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 e0c997494ce..a39b51d71f9 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 extra_txn; + std::vector> 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); }