Skip to content

Commit 6713f0f

Browse files
sipasipa
committed
Make FillBlock consume txn_available to avoid shared_ptr copies
1 parent 62607d7 commit 6713f0f

File tree

3 files changed

+40
-21
lines changed

3 files changed

+40
-21
lines changed

‎src/blockencodings.cpp‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,9 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const {
142142
return txn_available[index] ? true : false;
143143
}
144144

145-
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) const {
145+
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) {
146146
assert(!header.IsNull());
147+
uint256 hash = header.GetHash();
147148
block = header;
148149
block.vtx.resize(txn_available.size());
149150

@@ -154,8 +155,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
154155
return READ_STATUS_INVALID;
155156
block.vtx[i] = vtx_missing[tx_missing_offset++];
156157
} else
157-
block.vtx[i] = txn_available[i];
158+
block.vtx[i] = std::move(txn_available[i]);
158159
}
160+
161+
// Make sure we can't call FillBlock again.
162+
header.SetNull();
163+
txn_available.clear();
164+
159165
if (vtx_missing.size() != tx_missing_offset)
160166
return READ_STATUS_INVALID;
161167

@@ -170,10 +176,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
170176
return READ_STATUS_CHECKBLOCK_FAILED;
171177
}
172178

173-
LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", header.GetHash().ToString(), prefilled_count, mempool_count, vtx_missing.size());
179+
LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", hash.ToString(), prefilled_count, mempool_count, vtx_missing.size());
174180
if (vtx_missing.size() < 5) {
175181
for (const auto& tx : vtx_missing)
176-
LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", header.GetHash().ToString(), tx->GetHash().ToString());
182+
LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString());
177183
}
178184

179185
return READ_STATUS_OK;

‎src/blockencodings.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ class PartiallyDownloadedBlock {
202202

203203
ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock);
204204
bool IsTxAvailable(size_t index) const;
205-
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing) const;
205+
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing);
206206
};
207207

208208
#endif

‎src/test/blockencodings_tests.cpp‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -85,17 +85,23 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
8585
BOOST_CHECK_EQUAL(pool.size(), poolSize - 1);
8686

8787
CBlock block2;
88-
std::vector<CTransactionRef> vtx_missing;
89-
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions
88+
{
89+
PartiallyDownloadedBlock tmp = partialBlock;
90+
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
91+
partialBlock = tmp;
92+
}
9093

91-
vtx_missing.push_back(block.vtx[2]); // Wrong transaction
92-
partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that
94+
// Wrong transaction
95+
{
96+
PartiallyDownloadedBlock tmp = partialBlock;
97+
partialBlock.FillBlock(block2, {block.vtx[2]}); // Current implementation doesn't check txn here, but don't require that
98+
partialBlock = tmp;
99+
}
93100
bool mutated;
94101
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));
95102

96-
vtx_missing[0] = block.vtx[1];
97103
CBlock block3;
98-
BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK);
104+
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}) == READ_STATUS_OK);
99105
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
100106
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
101107
BOOST_CHECK(!mutated);
@@ -181,17 +187,24 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
181187
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
182188

183189
CBlock block2;
184-
std::vector<CTransactionRef> vtx_missing;
185-
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions
190+
{
191+
PartiallyDownloadedBlock tmp = partialBlock;
192+
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
193+
partialBlock = tmp;
194+
}
186195

187-
vtx_missing.push_back(block.vtx[1]); // Wrong transaction
188-
partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that
196+
// Wrong transaction
197+
{
198+
PartiallyDownloadedBlock tmp = partialBlock;
199+
partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that
200+
partialBlock = tmp;
201+
}
189202
bool mutated;
190203
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));
191204

192-
vtx_missing[0] = block.vtx[0];
193205
CBlock block3;
194-
BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK);
206+
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
207+
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}) == READ_STATUS_OK);
195208
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
196209
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
197210
BOOST_CHECK(!mutated);
@@ -200,7 +213,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
200213
block.vtx.clear();
201214
block2.vtx.clear();
202215
block3.vtx.clear();
203-
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
216+
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy.
204217
}
205218
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
206219
}
@@ -240,8 +253,8 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
240253
BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
241254

242255
CBlock block2;
243-
std::vector<CTransactionRef> vtx_missing;
244-
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_OK);
256+
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
257+
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_OK);
245258
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString());
246259
bool mutated;
247260
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString());
@@ -250,7 +263,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
250263
txhash = block.vtx[1]->GetHash();
251264
block.vtx.clear();
252265
block2.vtx.clear();
253-
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1);
266+
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy.
254267
}
255268
BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0);
256269
}

0 commit comments

Comments
 (0)