From 6713f0f142d97b4608c95a3ea03b4b670fceab2b Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 11 Nov 2016 13:01:27 -0800 Subject: [PATCH] Make FillBlock consume txn_available to avoid shared_ptr copies --- src/blockencodings.cpp | 14 +++++++--- src/blockencodings.h | 2 +- src/test/blockencodings_tests.cpp | 45 ++++++++++++++++++++----------- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 914af0c670f3e..72fe17bdc7ef8 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -142,8 +142,9 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const { return txn_available[index] ? true : false; } -ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector& vtx_missing) const { +ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector& vtx_missing) { assert(!header.IsNull()); + uint256 hash = header.GetHash(); block = header; block.vtx.resize(txn_available.size()); @@ -154,8 +155,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< return READ_STATUS_INVALID; block.vtx[i] = vtx_missing[tx_missing_offset++]; } else - block.vtx[i] = txn_available[i]; + block.vtx[i] = std::move(txn_available[i]); } + + // Make sure we can't call FillBlock again. + header.SetNull(); + txn_available.clear(); + if (vtx_missing.size() != tx_missing_offset) return READ_STATUS_INVALID; @@ -170,10 +176,10 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< return READ_STATUS_CHECKBLOCK_FAILED; } - 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()); + 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()); if (vtx_missing.size() < 5) { for (const auto& tx : vtx_missing) - LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", header.GetHash().ToString(), tx->GetHash().ToString()); + LogPrint("cmpctblock", "Reconstructed block %s required tx %s\n", hash.ToString(), tx->GetHash().ToString()); } return READ_STATUS_OK; diff --git a/src/blockencodings.h b/src/blockencodings.h index 27baf1f8f8263..809ccbf9363fa 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -202,7 +202,7 @@ class PartiallyDownloadedBlock { ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock); bool IsTxAvailable(size_t index) const; - ReadStatus FillBlock(CBlock& block, const std::vector& vtx_missing) const; + ReadStatus FillBlock(CBlock& block, const std::vector& vtx_missing); }; #endif diff --git a/src/test/blockencodings_tests.cpp b/src/test/blockencodings_tests.cpp index 7478758f73f0b..90f273e731cb3 100644 --- a/src/test/blockencodings_tests.cpp +++ b/src/test/blockencodings_tests.cpp @@ -85,17 +85,23 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest) BOOST_CHECK_EQUAL(pool.size(), poolSize - 1); CBlock block2; - std::vector vtx_missing; - BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions + { + PartiallyDownloadedBlock tmp = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions + partialBlock = tmp; + } - vtx_missing.push_back(block.vtx[2]); // Wrong transaction - partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that + // Wrong transaction + { + PartiallyDownloadedBlock tmp = partialBlock; + partialBlock.FillBlock(block2, {block.vtx[2]}); // Current implementation doesn't check txn here, but don't require that + partialBlock = tmp; + } bool mutated; BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated)); - vtx_missing[0] = block.vtx[1]; CBlock block3; - BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK); + BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}) == READ_STATUS_OK); BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString()); BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString()); BOOST_CHECK(!mutated); @@ -181,17 +187,24 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[2]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); CBlock block2; - std::vector vtx_missing; - BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_INVALID); // No transactions + { + PartiallyDownloadedBlock tmp = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions + partialBlock = tmp; + } - vtx_missing.push_back(block.vtx[1]); // Wrong transaction - partialBlock.FillBlock(block2, vtx_missing); // Current implementation doesn't check txn here, but don't require that + // Wrong transaction + { + PartiallyDownloadedBlock tmp = partialBlock; + partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that + partialBlock = tmp; + } bool mutated; BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated)); - vtx_missing[0] = block.vtx[0]; CBlock block3; - BOOST_CHECK(partialBlock.FillBlock(block3, vtx_missing) == READ_STATUS_OK); + PartiallyDownloadedBlock partialBlockCopy = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}) == READ_STATUS_OK); BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString()); BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString()); BOOST_CHECK(!mutated); @@ -200,7 +213,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest) block.vtx.clear(); block2.vtx.clear(); block3.vtx.clear(); - BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); + BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy. } BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); } @@ -240,8 +253,8 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) BOOST_CHECK_EQUAL(pool.mapTx.find(block.vtx[1]->GetHash())->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); CBlock block2; - std::vector vtx_missing; - BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_OK); + PartiallyDownloadedBlock partialBlockCopy = partialBlock; + BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_OK); BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString()); bool mutated; BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString()); @@ -250,7 +263,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest) txhash = block.vtx[1]->GetHash(); block.vtx.clear(); block2.vtx.clear(); - BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); + BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 1); // + 1 because of partialBlockCopy. } BOOST_CHECK_EQUAL(pool.mapTx.find(txhash)->GetSharedTx().use_count(), SHARED_TX_OFFSET + 0); }