Skip to content

Commit 7cc9a08

Browse files
committed
Merge bitcoin/bitcoin#33253: Revert compact block cache inefficiencies
b7b249d Revert "[refactor] rewrite vTxHashes as a vector of CTransactionRef" (Anthony Towns) b9300d8 Revert "refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef>" (Anthony Towns) df5a50e 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 b7b249d polespinasa: lgtm code review and tested ACK b7b249d cedwies: code-review ACK b7b249d davidgumberg: crACK b7b249d instagibbs: ACK b7b249d Tree-SHA512: dc266e7ac08281a5899fb1d8d0ad43eb4085f8ec42606833832800a568f4a43e3931f942d4dc53cf680af620b7e893e80c9fe9220f83894b4609184b1b3b3b42
2 parents 084fd68 + b7b249d commit 7cc9a08

File tree

9 files changed

+166
-28
lines changed

9 files changed

+166
-28
lines changed

src/bench/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ add_executable(bench_bitcoin
1212
bech32.cpp
1313
bip324_ecdh.cpp
1414
block_assemble.cpp
15+
blockencodings.cpp
1516
ccoins_caching.cpp
1617
chacha20.cpp
1718
checkblock.cpp

src/bench/blockencodings.cpp

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
// Copyright (c) 2025-present The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <bench/bench.h>
6+
#include <blockencodings.h>
7+
#include <consensus/amount.h>
8+
#include <kernel/cs_main.h>
9+
#include <net_processing.h>
10+
#include <primitives/transaction.h>
11+
#include <script/script.h>
12+
#include <sync.h>
13+
#include <test/util/setup_common.h>
14+
#include <test/util/txmempool.h>
15+
#include <txmempool.h>
16+
#include <util/check.h>
17+
18+
#include <memory>
19+
#include <vector>
20+
21+
22+
static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
23+
{
24+
LockPoints lp;
25+
AddToMempool(pool, CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
26+
}
27+
28+
namespace {
29+
class BenchCBHAST : public CBlockHeaderAndShortTxIDs
30+
{
31+
private:
32+
static CBlock DummyBlock()
33+
{
34+
CBlock block;
35+
block.nVersion = 5;
36+
block.hashPrevBlock.SetNull();
37+
block.hashMerkleRoot.SetNull();
38+
block.nTime = 1231006505;
39+
block.nBits = 0x1d00ffff;
40+
block.nNonce = 2083236893;
41+
block.fChecked = false;
42+
CMutableTransaction tx;
43+
tx.vin.resize(1);
44+
tx.vout.resize(1);
45+
block.vtx.emplace_back(MakeTransactionRef(tx)); // dummy coinbase
46+
return block;
47+
}
48+
49+
public:
50+
BenchCBHAST(InsecureRandomContext& rng, int txs) : CBlockHeaderAndShortTxIDs(DummyBlock(), rng.rand64())
51+
{
52+
shorttxids.reserve(txs);
53+
while (txs-- > 0) {
54+
shorttxids.push_back(rng.randbits<SHORTTXIDS_LENGTH*8>());
55+
}
56+
}
57+
};
58+
} // anon namespace
59+
60+
static void BlockEncodingBench(benchmark::Bench& bench, size_t n_pool, size_t n_extra)
61+
{
62+
const auto testing_setup = MakeNoLogFileContext<const ChainTestingSetup>(ChainType::MAIN);
63+
CTxMemPool& pool = *Assert(testing_setup->m_node.mempool);
64+
InsecureRandomContext rng(11);
65+
66+
LOCK2(cs_main, pool.cs);
67+
68+
std::vector<std::pair<Wtxid, CTransactionRef>> extratxn;
69+
extratxn.reserve(n_extra);
70+
71+
// bump up the size of txs
72+
std::array<std::byte,200> sigspam;
73+
sigspam.fill(std::byte(42));
74+
75+
// a reasonably large mempool of 50k txs, ~10MB total
76+
std::vector<CTransactionRef> refs;
77+
refs.reserve(n_pool + n_extra);
78+
for (size_t i = 0; i < n_pool + n_extra; ++i) {
79+
CMutableTransaction tx = CMutableTransaction();
80+
tx.vin.resize(1);
81+
tx.vin[0].scriptSig = CScript() << sigspam;
82+
tx.vin[0].scriptWitness.stack.push_back({1});
83+
tx.vout.resize(1);
84+
tx.vout[0].scriptPubKey = CScript() << OP_1 << OP_EQUAL;
85+
tx.vout[0].nValue = i;
86+
refs.push_back(MakeTransactionRef(tx));
87+
}
88+
89+
// ensure mempool ordering is different to memory ordering of transactions,
90+
// to simulate a mempool that has changed over time
91+
std::shuffle(refs.begin(), refs.end(), rng);
92+
93+
for (size_t i = 0; i < n_pool; ++i) {
94+
AddTx(refs[i], /*fee=*/refs[i]->vout[0].nValue, pool);
95+
}
96+
for (size_t i = n_pool; i < n_pool + n_extra; ++i) {
97+
extratxn.emplace_back(refs[i]->GetWitnessHash(), refs[i]);
98+
}
99+
100+
BenchCBHAST cmpctblock{rng, 3000};
101+
102+
bench.run([&] {
103+
PartiallyDownloadedBlock pdb{&pool};
104+
auto res = pdb.InitData(cmpctblock, extratxn);
105+
106+
// if there were duplicates the benchmark will be invalid
107+
// (eg, extra txns will be skipped) and we will receive
108+
// READ_STATUS_FAILED
109+
assert(res == READ_STATUS_OK);
110+
});
111+
}
112+
113+
static void BlockEncodingNoExtra(benchmark::Bench& bench)
114+
{
115+
BlockEncodingBench(bench, 50000, 0);
116+
}
117+
118+
static void BlockEncodingStdExtra(benchmark::Bench& bench)
119+
{
120+
static_assert(DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN == 100);
121+
BlockEncodingBench(bench, 50000, 100);
122+
}
123+
124+
static void BlockEncodingLargeExtra(benchmark::Bench& bench)
125+
{
126+
BlockEncodingBench(bench, 50000, 5000);
127+
}
128+
129+
BENCHMARK(BlockEncodingNoExtra, benchmark::PriorityLevel::HIGH);
130+
BENCHMARK(BlockEncodingStdExtra, benchmark::PriorityLevel::HIGH);
131+
BENCHMARK(BlockEncodingLargeExtra, benchmark::PriorityLevel::HIGH);

src/blockencodings.cpp

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,15 @@ uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
4545
return SipHashUint256(shorttxidk0, shorttxidk1, wtxid.ToUint256()) & 0xffffffffffffL;
4646
}
4747

48-
ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<CTransactionRef>& extra_txn) {
48+
/* Reconstructing a compact block is in the hot-path for block relay,
49+
* so we want to do it as quickly as possible. Because this often
50+
* involves iterating over the entire mempool, we put all the data we
51+
* need (ie the wtxid and a reference to the actual transaction data)
52+
* in a vector and iterate over the vector directly. This allows optimal
53+
* CPU caching behaviour, at a cost of only 40 bytes per transaction.
54+
*/
55+
ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<Wtxid, CTransactionRef>>& extra_txn)
56+
{
4957
LogDebug(BCLog::CMPCTBLOCK, "Initializing PartiallyDownloadedBlock for block %s using a cmpctblock of %u bytes\n", cmpctblock.header.GetHash().ToString(), GetSerializeSize(cmpctblock));
5058
if (cmpctblock.header.IsNull() || (cmpctblock.shorttxids.empty() && cmpctblock.prefilledtxn.empty()))
5159
return READ_STATUS_INVALID;
@@ -106,12 +114,12 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
106114
std::vector<bool> have_txn(txn_available.size());
107115
{
108116
LOCK(pool->cs);
109-
for (const auto& tx : pool->txns_randomized) {
110-
uint64_t shortid = cmpctblock.GetShortID(tx->GetWitnessHash());
117+
for (const auto& [wtxid, txit] : pool->txns_randomized) {
118+
uint64_t shortid = cmpctblock.GetShortID(wtxid);
111119
std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
112120
if (idit != shorttxids.end()) {
113121
if (!have_txn[idit->second]) {
114-
txn_available[idit->second] = tx;
122+
txn_available[idit->second] = txit->GetSharedTx();
115123
have_txn[idit->second] = true;
116124
mempool_count++;
117125
} else {
@@ -133,14 +141,11 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
133141
}
134142

135143
for (size_t i = 0; i < extra_txn.size(); i++) {
136-
if (extra_txn[i] == nullptr) {
137-
continue;
138-
}
139-
uint64_t shortid = cmpctblock.GetShortID(extra_txn[i]->GetWitnessHash());
144+
uint64_t shortid = cmpctblock.GetShortID(extra_txn[i].first);
140145
std::unordered_map<uint64_t, uint16_t>::iterator idit = shorttxids.find(shortid);
141146
if (idit != shorttxids.end()) {
142147
if (!have_txn[idit->second]) {
143-
txn_available[idit->second] = extra_txn[i];
148+
txn_available[idit->second] = extra_txn[i].second;
144149
have_txn[idit->second] = true;
145150
mempool_count++;
146151
extra_count++;
@@ -152,7 +157,7 @@ ReadStatus PartiallyDownloadedBlock::InitData(const CBlockHeaderAndShortTxIDs& c
152157
// Note that we don't want duplication between extra_txn and mempool to
153158
// trigger this case, so we compare witness hashes first
154159
if (txn_available[idit->second] &&
155-
txn_available[idit->second]->GetWitnessHash() != extra_txn[i]->GetWitnessHash()) {
160+
txn_available[idit->second]->GetWitnessHash() != extra_txn[i].second->GetWitnessHash()) {
156161
txn_available[idit->second].reset();
157162
mempool_count--;
158163
extra_count--;

src/blockencodings.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,8 @@ class PartiallyDownloadedBlock {
144144

145145
explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
146146

147-
// extra_txn is a list of extra orphan/conflicted/etc transactions to look at
148-
ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<CTransactionRef>& extra_txn);
147+
// extra_txn is a list of extra transactions to look at, in <witness hash, reference> form
148+
ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<std::pair<Wtxid, CTransactionRef>>& extra_txn);
149149
bool IsTxAvailable(size_t index) const;
150150
// segwit_active enforces witness mutation checks just before reporting a healthy status
151151
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active);

src/net_processing.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -974,7 +974,7 @@ class PeerManagerImpl final : public PeerManager
974974
/** Orphan/conflicted/etc transactions that are kept for compact block reconstruction.
975975
* The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of
976976
* these are kept in a ring buffer */
977-
std::vector<CTransactionRef> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
977+
std::vector<std::pair<Wtxid, CTransactionRef>> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
978978
/** Offset into vExtraTxnForCompact to insert the next tx */
979979
size_t vExtraTxnForCompactIt GUARDED_BY(g_msgproc_mutex) = 0;
980980

@@ -1768,7 +1768,7 @@ void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
17681768
return;
17691769
if (!vExtraTxnForCompact.size())
17701770
vExtraTxnForCompact.resize(m_opts.max_extra_txs);
1771-
vExtraTxnForCompact[vExtraTxnForCompactIt] = tx;
1771+
vExtraTxnForCompact[vExtraTxnForCompactIt] = std::make_pair(tx->GetWitnessHash(), tx);
17721772
vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs;
17731773
}
17741774

src/test/blockencodings_tests.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
#include <boost/test/unit_test.hpp>
1616

17-
const std::vector<CTransactionRef> empty_extra_txn;
17+
const std::vector<std::pair<Wtxid, CTransactionRef>> empty_extra_txn;
1818

1919
BOOST_FIXTURE_TEST_SUITE(blockencodings_tests, RegTestingSetup)
2020

@@ -56,8 +56,8 @@ static CBlock BuildBlockTestCase(FastRandomContext& ctx) {
5656
}
5757

5858
// Number of shared use_counts we expect for a tx we haven't touched
59-
// (block + mempool entry + mempool txns_randomized + our copy from the GetSharedTx call)
60-
constexpr long SHARED_TX_OFFSET{4};
59+
// (block + mempool entry + our copy from the GetSharedTx call)
60+
constexpr long SHARED_TX_OFFSET{3};
6161

6262
BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
6363
{
@@ -318,7 +318,7 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) {
318318
const CTransactionRef non_block_tx = MakeTransactionRef(std::move(mtx));
319319

320320
CBlock block(BuildBlockTestCase(rand_ctx));
321-
std::vector<CTransactionRef> extra_txn;
321+
std::vector<std::pair<Wtxid, CTransactionRef>> extra_txn;
322322
extra_txn.resize(10);
323323

324324
LOCK2(cs_main, pool.cs);
@@ -342,9 +342,9 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) {
342342
BOOST_CHECK( partial_block.IsTxAvailable(2));
343343

344344
// Add an unrelated tx to extra_txn:
345-
extra_txn[0] = non_block_tx;
345+
extra_txn[0] = {non_block_tx->GetWitnessHash(), non_block_tx};
346346
// and a tx from the block that's not in the mempool:
347-
extra_txn[1] = block.vtx[1];
347+
extra_txn[1] = {block.vtx[1]->GetWitnessHash(), block.vtx[1]};
348348

349349
BOOST_CHECK(partial_block_with_extra.InitData(cmpctblock, extra_txn) == READ_STATUS_OK);
350350
BOOST_CHECK(partial_block_with_extra.IsTxAvailable(0));

src/test/fuzz/partially_downloaded_block.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,15 +67,15 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
6767
// The coinbase is always available
6868
available.insert(0);
6969

70-
std::vector<CTransactionRef> extra_txn;
70+
std::vector<std::pair<Wtxid, CTransactionRef>> extra_txn;
7171
for (size_t i = 1; i < block->vtx.size(); ++i) {
7272
auto tx{block->vtx[i]};
7373

7474
bool add_to_extra_txn{fuzzed_data_provider.ConsumeBool()};
7575
bool add_to_mempool{fuzzed_data_provider.ConsumeBool()};
7676

7777
if (add_to_extra_txn) {
78-
extra_txn.emplace_back(tx);
78+
extra_txn.emplace_back(tx->GetWitnessHash(), tx);
7979
available.insert(i);
8080
}
8181

src/txmempool.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -507,7 +507,7 @@ void CTxMemPool::addNewTransaction(CTxMemPool::txiter newit, CTxMemPool::setEntr
507507
totalTxSize += entry.GetTxSize();
508508
m_total_fee += entry.GetFee();
509509

510-
txns_randomized.emplace_back(newit->GetSharedTx());
510+
txns_randomized.emplace_back(tx.GetWitnessHash(), newit);
511511
newit->idx_randomized = txns_randomized.size() - 1;
512512

513513
TRACEPOINT(mempool, added,
@@ -544,15 +544,16 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
544544
RemoveUnbroadcastTx(it->GetTx().GetHash(), true /* add logging because unchecked */);
545545

546546
if (txns_randomized.size() > 1) {
547-
// Update idx_randomized of the to-be-moved entry.
548-
Assert(GetEntry(txns_randomized.back()->GetHash()))->idx_randomized = it->idx_randomized;
549547
// Remove entry from txns_randomized by replacing it with the back and deleting the back.
550548
txns_randomized[it->idx_randomized] = std::move(txns_randomized.back());
549+
txns_randomized[it->idx_randomized].second->idx_randomized = it->idx_randomized;
551550
txns_randomized.pop_back();
552-
if (txns_randomized.size() * 2 < txns_randomized.capacity())
551+
if (txns_randomized.size() * 2 < txns_randomized.capacity()) {
553552
txns_randomized.shrink_to_fit();
554-
} else
553+
}
554+
} else {
555555
txns_randomized.clear();
556+
}
556557

557558
totalTxSize -= it->GetTxSize();
558559
m_total_fee -= it->GetFee();

src/txmempool.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,7 @@ class CTxMemPool
368368
indexed_transaction_set mapTx GUARDED_BY(cs);
369369

370370
using txiter = indexed_transaction_set::nth_index<0>::type::const_iterator;
371-
std::vector<CTransactionRef> txns_randomized GUARDED_BY(cs); //!< All transactions in mapTx, in random order
371+
std::vector<std::pair<Wtxid, txiter>> txns_randomized GUARDED_BY(cs); //!< All transactions in mapTx with their wtxids, in arbitrary order
372372

373373
typedef std::set<txiter, CompareIteratorByHash> setEntries;
374374

0 commit comments

Comments
 (0)