Merge bitcoin/bitcoin#33253: Revert compact block cache inefficiencies

b7b249d3adfbd3c7b0c4d0499a86300f57982972 Revert "[refactor] rewrite vTxHashes as a vector of CTransactionRef" (Anthony Towns)
b9300d8d0a74b15a220a2ce529d5157d109c7ed3 Revert "refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef>" (Anthony Towns)
df5a50e5de204013ba56e1c4967a249ca07ba086 bench/blockencodings: add compact block reconstruction benchmark (Anthony Towns)

Pull request description:

  Reconstructing compact blocks is on the hot path for block relay, so revert changes from #28391 and #29752 that made it slower. Also add a benchmark to validate reconstruction performance, and a comment giving some background as to the approach.

ACKs for top commit:
  achow101:
    ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972
  polespinasa:
    lgtm code review and tested ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972
  cedwies:
    code-review ACK b7b249d
  davidgumberg:
    crACK b7b249d3adfbd3c
  instagibbs:
    ACK b7b249d3adfbd3c7b0c4d0499a86300f57982972

Tree-SHA512: dc266e7ac08281a5899fb1d8d0ad43eb4085f8ec42606833832800a568f4a43e3931f942d4dc53cf680af620b7e893e80c9fe9220f83894b4609184b1b3b3b42

src/bench/CMakeLists.txt

@@ -12,6 +12,7 @@ add_executable(bench_bitcoin
   bech32.cpp
   bip324_ecdh.cpp
   block_assemble.cpp
+  blockencodings.cpp
   ccoins_caching.cpp
   chacha20.cpp
   checkblock.cpp

src/bench/blockencodings.cpp

@@ -0,0 +1,131 @@
+// Copyright (c) 2025-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <bench/bench.h>
+#include <blockencodings.h>
+#include <consensus/amount.h>
+#include <kernel/cs_main.h>
+#include <net_processing.h>
+#include <primitives/transaction.h>
+#include <script/script.h>
+#include <sync.h>
+#include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
+#include <txmempool.h>
+#include <util/check.h>
+
+#include <memory>
+#include <vector>
+
+
+static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
+{
+    LockPoints lp;
+    AddToMempool(pool, CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
+}
+
+namespace {
+class BenchCBHAST : public CBlockHeaderAndShortTxIDs
+{
+private:
+    static CBlock DummyBlock()
+    {
+        CBlock block;
+        block.nVersion = 5;
+        block.hashPrevBlock.SetNull();
+        block.hashMerkleRoot.SetNull();
+        block.nTime = 1231006505;
+        block.nBits = 0x1d00ffff;
+        block.nNonce = 2083236893;
+        block.fChecked = false;
+        CMutableTransaction tx;
+        tx.vin.resize(1);
+        tx.vout.resize(1);
+        block.vtx.emplace_back(MakeTransactionRef(tx)); // dummy coinbase
+        return block;
+    }
+
+public:
+    BenchCBHAST(InsecureRandomContext& rng, int txs) : CBlockHeaderAndShortTxIDs(DummyBlock(), rng.rand64())
+    {
+        shorttxids.reserve(txs);
+        while (txs-- > 0) {
+            shorttxids.push_back(rng.randbits<SHORTTXIDS_LENGTH*8>());
+        }
+    }
+};
+} // anon namespace
+
+static void BlockEncodingBench(benchmark::Bench& bench, size_t n_pool, size_t n_extra)
+{
+    const auto testing_setup = MakeNoLogFileContext<const ChainTestingSetup>(ChainType::MAIN);
+    CTxMemPool& pool = *Assert(testing_setup->m_node.mempool);
+    InsecureRandomContext rng(11);
+
+    LOCK2(cs_main, pool.cs);
+
+    std::vector<std::pair<Wtxid, CTransactionRef>> extratxn;
+    extratxn.reserve(n_extra);
+
+    // bump up the size of txs
+    std::array<std::byte,200> sigspam;
+    sigspam.fill(std::byte(42));
+
+    // a reasonably large mempool of 50k txs, ~10MB total
+    std::vector<CTransactionRef> refs;
+    refs.reserve(n_pool + n_extra);
+    for (size_t i = 0; i < n_pool + n_extra; ++i) {
+        CMutableTransaction tx = CMutableTransaction();
+        tx.vin.resize(1);
+        tx.vin[0].scriptSig = CScript() << sigspam;
+        tx.vin[0].scriptWitness.stack.push_back({1});
+        tx.vout.resize(1);
+        tx.vout[0].scriptPubKey = CScript() << OP_1 << OP_EQUAL;
+        tx.vout[0].nValue = i;
+        refs.push_back(MakeTransactionRef(tx));
+    }
+
+    // ensure mempool ordering is different to memory ordering of transactions,
+    // to simulate a mempool that has changed over time
+    std::shuffle(refs.begin(), refs.end(), rng);
+
+    for (size_t i = 0; i < n_pool; ++i) {
+        AddTx(refs[i], /*fee=*/refs[i]->vout[0].nValue, pool);
+    }
+    for (size_t i = n_pool; i < n_pool + n_extra; ++i) {
+        extratxn.emplace_back(refs[i]->GetWitnessHash(), refs[i]);
+    }
+
+    BenchCBHAST cmpctblock{rng, 3000};
+
+    bench.run([&] {
+        PartiallyDownloadedBlock pdb{&pool};
+        auto res = pdb.InitData(cmpctblock, extratxn);
+
+        // if there were duplicates the benchmark will be invalid
+        // (eg, extra txns will be skipped) and we will receive
+        // READ_STATUS_FAILED
+        assert(res == READ_STATUS_OK);
+    });
+}
+
+static void BlockEncodingNoExtra(benchmark::Bench& bench)
+{
+    BlockEncodingBench(bench, 50000, 0);
+}
+
+static void BlockEncodingStdExtra(benchmark::Bench& bench)
+{
+    static_assert(DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN == 100);
+    BlockEncodingBench(bench, 50000, 100);
+}
+
+static void BlockEncodingLargeExtra(benchmark::Bench& bench)
+{
+    BlockEncodingBench(bench, 50000, 5000);
+}
+
+BENCHMARK(BlockEncodingNoExtra, benchmark::PriorityLevel::HIGH);
+BENCHMARK(BlockEncodingStdExtra, benchmark::PriorityLevel::HIGH);
+BENCHMARK(BlockEncodingLargeExtra, benchmark::PriorityLevel::HIGH);

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;
@@ -106,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 {
@@ -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 @@ 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);

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

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)
 
@@ -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)
 {
@@ -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,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);
         }
 

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

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;