From 6fdd43b968f984ef92ca4576872dc65462ba7312 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 25 Oct 2016 13:20:45 -0400 Subject: Add struct to track block-connect-time-generated info for callbacks --- src/validation.cpp | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) (limited to 'src/validation.cpp') 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); -- cgit v1.2.3 From fd9d89070a70a7333d8ffe944cc53f8fd4122548 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Oct 2016 12:12:50 -0400 Subject: Keep blocks as shared_ptrs, instead of copying txn in ConnectTip --- src/validation.cpp | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) (limited to 'src/validation.cpp') 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); -- cgit v1.2.3 From ae4db44d0341b7517d02bdc74d4b69d0cd2f6778 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 25 Oct 2016 14:22:41 -0400 Subject: Create a shared_ptr for the block we're connecting in ActivateBCS --- src/validation.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'src/validation.cpp') 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()) -- cgit v1.2.3 From 2736c44c8edea5ce6a502a04269926fecda27301 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 15 Oct 2016 14:01:31 -0400 Subject: Make the optional pblock in ActivateBestChain a shared_ptr --- src/validation.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) (limited to 'src/validation.cpp') 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; -- cgit v1.2.3 From 2d6e5619afa2d43a37a0a38daf33f58965ddfa80 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 4 Dec 2016 00:17:30 -0800 Subject: Switch pblock in ProcessNewBlock to a shared_ptr This (finally) fixes a performance regression in b3b3c2a5623d5c942d2b3565cc2d833c65105555 --- src/validation.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) (limited to 'src/validation.cpp') 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; -- cgit v1.2.3 From dd0df81ebdbf705f7ad386c7229bf1bbc3125f62 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 29 Nov 2016 21:25:39 -0800 Subject: Document ConnectBlock connectTrace postconditions --- src/validation.cpp | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/validation.cpp') 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) { -- cgit v1.2.3