From 6fdd43b968f984ef92ca4576872dc65462ba7312 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 25 Oct 2016 13:20:45 -0400 Subject: [PATCH 1/6] Add struct to track block-connect-time-generated info for callbacks --- src/validation.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 9cfb5221a..e8fe8639f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2152,11 +2152,20 @@ static int64_t nTimeFlush = 0; static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; +/** + * Used to track conflicted transactions removed from mempool and transactions + * applied to the UTXO state as a part of a single ActivateBestChainStep call. + */ +struct ConnectTrace { + std::vector txConflicted; + std::vector> txChanged; +}; + /** * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. */ -bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock, std::vector &txConflicted, std::vector> &txChanged) +bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock, ConnectTrace& connectTrace) { assert(pindexNew->pprev == chainActive.Tip()); // Read block from disk. @@ -2192,12 +2201,12 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4; LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool.; - mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, &txConflicted, !IsInitialBlockDownload()); + mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, &connectTrace.txConflicted, !IsInitialBlockDownload()); // Update chainActive & related variables. UpdateTip(pindexNew, chainparams); for (unsigned int i=0; i < pblock->vtx.size(); i++) - txChanged.emplace_back(pblock->vtx[i], pindexNew, i); + connectTrace.txChanged.emplace_back(pblock->vtx[i], pindexNew, i); int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; LogPrint("bench", " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); @@ -2279,7 +2288,7 @@ static void PruneBlockIndexCandidates() { * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound, std::vector& txConflicted, std::vector>& txChanged) +static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); const CBlockIndex *pindexOldTip = chainActive.Tip(); @@ -2312,7 +2321,7 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c // Connect new blocks. BOOST_REVERSE_FOREACH(CBlockIndex *pindexConnect, vpindexToConnect) { - if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, txConflicted, txChanged)) { + if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) @@ -2380,17 +2389,13 @@ static void NotifyHeaderTip() { bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, const CBlock *pblock) { CBlockIndex *pindexMostWork = NULL; CBlockIndex *pindexNewTip = NULL; - std::vector> txChanged; - if (pblock) - txChanged.reserve(pblock->vtx.size()); do { - txChanged.clear(); boost::this_thread::interruption_point(); if (ShutdownRequested()) break; const CBlockIndex *pindexFork; - std::vector txConflicted; + ConnectTrace connectTrace; bool fInitialDownload; { LOCK(cs_main); @@ -2404,7 +2409,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, return true; bool fInvalidFound = false; - if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fInvalidFound, txConflicted, txChanged)) + if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fInvalidFound, connectTrace)) return false; if (fInvalidFound) { @@ -2421,13 +2426,13 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, // throw all transactions though the signal-interface // while _not_ holding the cs_main lock - for (const auto& tx : txConflicted) + for (const auto& tx : connectTrace.txConflicted) { GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); } // ... and about transactions that got confirmed: - for (unsigned int i = 0; i < txChanged.size(); i++) - GetMainSignals().SyncTransaction(*std::get<0>(txChanged[i]), std::get<1>(txChanged[i]), std::get<2>(txChanged[i])); + for (unsigned int i = 0; i < connectTrace.txChanged.size(); i++) + GetMainSignals().SyncTransaction(*std::get<0>(connectTrace.txChanged[i]), std::get<1>(connectTrace.txChanged[i]), std::get<2>(connectTrace.txChanged[i])); // Notify external listeners about the new tip. GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); From fd9d89070a70a7333d8ffe944cc53f8fd4122548 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Oct 2016 12:12:50 -0400 Subject: [PATCH 2/6] Keep blocks as shared_ptrs, instead of copying txn in ConnectTip --- src/validation.cpp | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index e8fe8639f..ffb473266 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2158,7 +2158,7 @@ static int64_t nTimePostConnect = 0; */ struct ConnectTrace { std::vector txConflicted; - std::vector> txChanged; + std::vector > > blocksConnected; }; /** @@ -2170,11 +2170,15 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, assert(pindexNew->pprev == chainActive.Tip()); // Read block from disk. int64_t nTime1 = GetTimeMicros(); - CBlock block; if (!pblock) { - if (!ReadBlockFromDisk(block, pindexNew, chainparams.GetConsensus())) + std::shared_ptr pblockNew = std::make_shared(); + connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew); + if (!ReadBlockFromDisk(*pblockNew, pindexNew, chainparams.GetConsensus())) return AbortNode(state, "Failed to read block"); - pblock = █ + pblock = pblockNew.get(); + } else { + //TODO: This copy is a major performance regression, but ProcessNewBlock callers need updated to fix this + connectTrace.blocksConnected.emplace_back(pindexNew, std::make_shared(*pblock)); } // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1; @@ -2205,9 +2209,6 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, // Update chainActive & related variables. UpdateTip(pindexNew, chainparams); - for (unsigned int i=0; i < pblock->vtx.size(); i++) - connectTrace.txChanged.emplace_back(pblock->vtx[i], pindexNew, i); - int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; LogPrint("bench", " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint("bench", "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); @@ -2329,6 +2330,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c state = CValidationState(); fInvalidFound = true; fContinue = false; + // If we didn't actually connect the block, don't notify listeners about it + connectTrace.blocksConnected.pop_back(); break; } else { // A system error occurred (disk space, database error, ...). @@ -2431,8 +2434,12 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, GetMainSignals().SyncTransaction(*tx, pindexNewTip, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); } // ... and about transactions that got confirmed: - for (unsigned int i = 0; i < connectTrace.txChanged.size(); i++) - GetMainSignals().SyncTransaction(*std::get<0>(connectTrace.txChanged[i]), std::get<1>(connectTrace.txChanged[i]), std::get<2>(connectTrace.txChanged[i])); + for (const auto& pair : connectTrace.blocksConnected) { + assert(pair.second); + const CBlock& block = *(pair.second); + for (unsigned int i = 0; i < block.vtx.size(); i++) + GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i); + } // Notify external listeners about the new tip. GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); From ae4db44d0341b7517d02bdc74d4b69d0cd2f6778 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 25 Oct 2016 14:22:41 -0400 Subject: [PATCH 3/6] Create a shared_ptr for the block we're connecting in ActivateBCS --- src/validation.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index ffb473266..3a9ad4a28 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2165,7 +2165,7 @@ struct ConnectTrace { * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. */ -bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock, ConnectTrace& connectTrace) +bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr& pblock, ConnectTrace& connectTrace) { assert(pindexNew->pprev == chainActive.Tip()); // Read block from disk. @@ -2175,19 +2175,18 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, connectTrace.blocksConnected.emplace_back(pindexNew, pblockNew); if (!ReadBlockFromDisk(*pblockNew, pindexNew, chainparams.GetConsensus())) return AbortNode(state, "Failed to read block"); - pblock = pblockNew.get(); } else { - //TODO: This copy is a major performance regression, but ProcessNewBlock callers need updated to fix this - connectTrace.blocksConnected.emplace_back(pindexNew, std::make_shared(*pblock)); + connectTrace.blocksConnected.emplace_back(pindexNew, pblock); } + const CBlock& blockConnecting = *connectTrace.blocksConnected.back().second; // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1; int64_t nTime3; LogPrint("bench", " - Load block from disk: %.2fms [%.2fs]\n", (nTime2 - nTime1) * 0.001, nTimeReadFromDisk * 0.000001); { CCoinsViewCache view(pcoinsTip); - bool rv = ConnectBlock(*pblock, state, pindexNew, view, chainparams); - GetMainSignals().BlockChecked(*pblock, state); + bool rv = ConnectBlock(blockConnecting, state, pindexNew, view, chainparams); + GetMainSignals().BlockChecked(blockConnecting, state); if (!rv) { if (state.IsInvalid()) InvalidBlockFound(pindexNew, state); @@ -2205,7 +2204,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, int64_t nTime5 = GetTimeMicros(); nTimeChainState += nTime5 - nTime4; LogPrint("bench", " - Writing chainstate: %.2fms [%.2fs]\n", (nTime5 - nTime4) * 0.001, nTimeChainState * 0.000001); // Remove conflicting transactions from the mempool.; - mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, &connectTrace.txConflicted, !IsInitialBlockDownload()); + mempool.removeForBlock(blockConnecting.vtx, pindexNew->nHeight, &connectTrace.txConflicted, !IsInitialBlockDownload()); // Update chainActive & related variables. UpdateTip(pindexNew, chainparams); @@ -2322,7 +2321,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c // Connect new blocks. BOOST_REVERSE_FOREACH(CBlockIndex *pindexConnect, vpindexToConnect) { - if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL, connectTrace)) { + //TODO: The pblock copy is a major performance regression, but callers need updated to fix this + if (!ConnectTip(state, chainparams, pindexConnect, (pblock && pindexConnect == pindexMostWork) ? std::make_shared(*pblock) : std::shared_ptr(), connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) From 2736c44c8edea5ce6a502a04269926fecda27301 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Oct 2016 14:01:31 -0400 Subject: [PATCH 4/6] Make the optional pblock in ActivateBestChain a shared_ptr --- src/rpc/blockchain.cpp | 4 ++-- src/validation.cpp | 17 +++++++++++------ src/validation.h | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index b9b68fc10..e6d80f06a 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1319,7 +1319,7 @@ UniValue invalidateblock(const JSONRPCRequest& request) } if (state.IsValid()) { - ActivateBestChain(state, Params(), NULL); + ActivateBestChain(state, Params()); } if (!state.IsValid()) { @@ -1357,7 +1357,7 @@ UniValue reconsiderblock(const JSONRPCRequest& request) } CValidationState state; - ActivateBestChain(state, Params(), NULL); + ActivateBestChain(state, Params()); if (!state.IsValid()) { throw JSONRPCError(RPC_DATABASE_ERROR, state.GetRejectReason()); diff --git a/src/validation.cpp b/src/validation.cpp index 3a9ad4a28..bd66c5679 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2288,7 +2288,7 @@ static void PruneBlockIndexCandidates() { * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const CBlock* pblock, bool& fInvalidFound, ConnectTrace& connectTrace) +static bool ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); const CBlockIndex *pindexOldTip = chainActive.Tip(); @@ -2321,8 +2321,7 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c // Connect new blocks. BOOST_REVERSE_FOREACH(CBlockIndex *pindexConnect, vpindexToConnect) { - //TODO: The pblock copy is a major performance regression, but callers need updated to fix this - if (!ConnectTip(state, chainparams, pindexConnect, (pblock && pindexConnect == pindexMostWork) ? std::make_shared(*pblock) : std::shared_ptr(), connectTrace)) { + if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr(), connectTrace)) { if (state.IsInvalid()) { // The block violates a consensus rule. if (!state.CorruptionPossible()) @@ -2389,7 +2388,7 @@ static void NotifyHeaderTip() { * or an activated best chain. pblock is either NULL or a pointer to a block * that is already loaded (to avoid loading it again from disk). */ -bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, const CBlock *pblock) { +bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr pblock) { CBlockIndex *pindexMostWork = NULL; CBlockIndex *pindexNewTip = NULL; do { @@ -2412,7 +2411,8 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, return true; bool fInvalidFound = false; - if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : NULL, fInvalidFound, connectTrace)) + std::shared_ptr nullBlockPtr; + if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) return false; if (fInvalidFound) { @@ -3142,8 +3142,13 @@ bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool NotifyHeaderTip(); + //TODO: This copy is a major performance regression, but callers need updated to fix this + std::shared_ptr block_ptr; + if (pblock) + block_ptr.reset(new CBlock(*pblock)); + CValidationState state; // Only used to report errors, not invalidity - ignore it - if (!ActivateBestChain(state, chainparams, pblock)) + if (!ActivateBestChain(state, chainparams, block_ptr)) return error("%s: ActivateBestChain failed", __func__); return true; diff --git a/src/validation.h b/src/validation.h index a798cf370..2d055d190 100644 --- a/src/validation.h +++ b/src/validation.h @@ -278,7 +278,7 @@ std::string GetWarnings(const std::string& strFor); /** Retrieve a transaction (from memory pool, or from disk, if possible) */ bool GetTransaction(const uint256 &hash, CTransaction &tx, const Consensus::Params& params, uint256 &hashBlock, bool fAllowSlow = false); /** Find the best known block, and make it the tip of the block chain */ -bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, const CBlock* pblock = NULL); +bool ActivateBestChain(CValidationState& state, const CChainParams& chainparams, std::shared_ptr pblock = std::shared_ptr()); CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams); /** From 2d6e5619afa2d43a37a0a38daf33f58965ddfa80 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 4 Dec 2016 00:17:30 -0800 Subject: [PATCH 5/6] Switch pblock in ProcessNewBlock to a shared_ptr This (finally) fixes a performance regression in b3b3c2a5623d5c942d2b3565cc2d833c65105555 --- src/net_processing.cpp | 16 ++++++++-------- src/rpc/mining.cpp | 8 +++++--- src/test/miner_tests.cpp | 3 ++- src/test/test_bitcoin.cpp | 3 ++- src/validation.cpp | 9 ++------- src/validation.h | 2 +- 6 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 747167264..e81332d61 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1895,7 +1895,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, BlockTransactions resp; vRecv >> resp; - CBlock block; + std::shared_ptr pblock = std::make_shared(); bool fBlockRead = false; { LOCK(cs_main); @@ -1908,7 +1908,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, } PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; - ReadStatus status = partialBlock.FillBlock(block, resp.txn); + ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn); if (status == READ_STATUS_INVALID) { MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case of whitelist Misbehaving(pfrom->GetId(), 100); @@ -1951,7 +1951,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, bool fNewBlock = false; // Since we requested this block (it was in mapBlocksInFlight), force it to be processed, // even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc) - ProcessNewBlock(chainparams, &block, true, NULL, &fNewBlock); + ProcessNewBlock(chainparams, pblock, true, NULL, &fNewBlock); if (fNewBlock) pfrom->nLastBlockTime = GetTime(); } @@ -2112,17 +2112,17 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, else if (strCommand == NetMsgType::BLOCK && !fImporting && !fReindex) // Ignore blocks received while importing { - CBlock block; - vRecv >> block; + std::shared_ptr pblock = std::make_shared(); + vRecv >> *pblock; - LogPrint("net", "received block %s peer=%d\n", block.GetHash().ToString(), pfrom->id); + LogPrint("net", "received block %s peer=%d\n", pblock->GetHash().ToString(), pfrom->id); // Process all blocks from whitelisted peers, even if not requested, // unless we're still syncing with the network. // Such an unrequested block may still be processed, subject to the // conditions in AcceptBlock(). bool forceProcessing = pfrom->fWhitelisted && !IsInitialBlockDownload(); - const uint256 hash(block.GetHash()); + const uint256 hash(pblock->GetHash()); { LOCK(cs_main); // Also always process if we requested the block explicitly, as we may @@ -2133,7 +2133,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, mapBlockSource.emplace(hash, std::make_pair(pfrom->GetId(), true)); } bool fNewBlock = false; - ProcessNewBlock(chainparams, &block, forceProcessing, NULL, &fNewBlock); + ProcessNewBlock(chainparams, pblock, forceProcessing, NULL, &fNewBlock); if (fNewBlock) pfrom->nLastBlockTime = GetTime(); } diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 61408b3b6..cb22dec34 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -131,7 +131,8 @@ UniValue generateBlocks(boost::shared_ptr coinbaseScript, int nG if (pblock->nNonce == nInnerLoopCount) { continue; } - if (!ProcessNewBlock(Params(), pblock, true, NULL, NULL)) + std::shared_ptr shared_pblock = std::make_shared(*pblock); + if (!ProcessNewBlock(Params(), shared_pblock, true, NULL, NULL)) throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); ++nHeight; blockHashes.push_back(pblock->GetHash().GetHex()); @@ -728,7 +729,8 @@ UniValue submitblock(const JSONRPCRequest& request) + HelpExampleRpc("submitblock", "\"mydata\"") ); - CBlock block; + std::shared_ptr blockptr = std::make_shared(); + CBlock& block = *blockptr; if (!DecodeHexBlk(block, request.params[0].get_str())) throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed"); @@ -758,7 +760,7 @@ UniValue submitblock(const JSONRPCRequest& request) submitblock_StateCatcher sc(block.GetHash()); RegisterValidationInterface(&sc); - bool fAccepted = ProcessNewBlock(Params(), &block, true, NULL, NULL); + bool fAccepted = ProcessNewBlock(Params(), blockptr, true, NULL, NULL); UnregisterValidationInterface(&sc); if (fBlockPresent) { diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index ca703841d..bc1bdd887 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -223,7 +223,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) txFirst.push_back(pblock->vtx[0]); pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); pblock->nNonce = blockinfo[i].nonce; - BOOST_CHECK(ProcessNewBlock(chainparams, pblock, true, NULL, NULL)); + std::shared_ptr shared_pblock = std::make_shared(*pblock); + BOOST_CHECK(ProcessNewBlock(chainparams, shared_pblock, true, NULL, NULL)); pblock->hashPrevBlock = pblock->GetHash(); } diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index f979d01a3..139389117 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -127,7 +127,8 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector& while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce; - ProcessNewBlock(chainparams, &block, true, NULL, NULL); + std::shared_ptr shared_pblock = std::make_shared(block); + ProcessNewBlock(chainparams, shared_pblock, true, NULL, NULL); CBlock result = block; return result; diff --git a/src/validation.cpp b/src/validation.cpp index bd66c5679..0983c1f76 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3123,7 +3123,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha return true; } -bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool *fNewBlock) +bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool *fNewBlock) { { LOCK(cs_main); @@ -3142,13 +3142,8 @@ bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool NotifyHeaderTip(); - //TODO: This copy is a major performance regression, but callers need updated to fix this - std::shared_ptr block_ptr; - if (pblock) - block_ptr.reset(new CBlock(*pblock)); - CValidationState state; // Only used to report errors, not invalidity - ignore it - if (!ActivateBestChain(state, chainparams, block_ptr)) + if (!ActivateBestChain(state, chainparams, pblock)) return error("%s: ActivateBestChain failed", __func__); return true; diff --git a/src/validation.h b/src/validation.h index 2d055d190..3a2b51bfe 100644 --- a/src/validation.h +++ b/src/validation.h @@ -233,7 +233,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; * @param[out] fNewBlock A boolean which is set to indicate if the block was first received via this call * @return True if state.IsValid() */ -bool ProcessNewBlock(const CChainParams& chainparams, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool* fNewBlock); +bool ProcessNewBlock(const CChainParams& chainparams, const std::shared_ptr pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool* fNewBlock); /** * Process incoming block headers. From dd0df81ebdbf705f7ad386c7229bf1bbc3125f62 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 29 Nov 2016 21:25:39 -0800 Subject: [PATCH 6/6] Document ConnectBlock connectTrace postconditions --- src/validation.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 0983c1f76..3bcbf33d6 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2164,6 +2164,10 @@ struct ConnectTrace { /** * Connect a new block to chainActive. pblock is either NULL or a pointer to a CBlock * corresponding to pindexNew, to bypass loading it again from disk. + * + * The block is always added to connectTrace (either after loading from disk or by copying + * pblock) - if that is not intended, care must be taken to remove the last entry in + * blocksConnected in case of failure. */ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr& pblock, ConnectTrace& connectTrace) {