Revert "refactor: Simplify extra_txn to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef>"

This reverts commit a8203e9.

Author: ajtowns

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};

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--;

src/blockencodings.h

@@ -144,8 +144,8 @@ class PartiallyDownloadedBlock {
 
     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);

src/net_processing.cpp

@@ -974,7 +974,7 @@ class PeerManagerImpl final : public PeerManager
     /** 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;
 }
 

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));

src/test/fuzz/partially_downloaded_block.cpp

@@ -67,15 +67,15 @@ 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]};
 
         bool add_to_extra_txn{fuzzed_data_provider.ConsumeBool()};
         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);
         }