From f5e9a019a4bceff6819a10696f0ea8ec5c98a49e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 20 Jan 2017 18:27:13 -0500 Subject: [PATCH 01/12] Include missing #include in zmqnotificationinterface.h --- src/zmq/zmqnotificationinterface.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index beabb78da6..f22a539a3e 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -8,6 +8,7 @@ #include "validationinterface.h" #include #include +#include class CBlockIndex; class CZMQAbstractNotifier; From 822000cf82ce78954209df0bcf56b90c0f42e9b4 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Mar 2017 17:11:33 -0500 Subject: [PATCH 02/12] Add pblock to connectTrace at the end of ConnectTip, not start This makes ConnectTip responsible for the ConnectTrace instead of splitting the logic between ActivateBestChainStep and ConnectTip --- src/validation.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 239893dc0b..ff38f60813 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2219,24 +2219,23 @@ 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. + * The block is added to connectTrace if connection succeeds. */ 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. int64_t nTime1 = GetTimeMicros(); + std::shared_ptr pthisBlock; if (!pblock) { 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"); + pthisBlock = pblockNew; } else { - connectTrace.blocksConnected.emplace_back(pindexNew, pblock); + pthisBlock = pblock; } - const CBlock& blockConnecting = *connectTrace.blocksConnected.back().second; + const CBlock& blockConnecting = *pthisBlock; // Apply the block atomically to the chain state. int64_t nTime2 = GetTimeMicros(); nTimeReadFromDisk += nTime2 - nTime1; int64_t nTime3; @@ -2270,6 +2269,8 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1; LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); + + connectTrace.blocksConnected.emplace_back(pindexNew, std::move(pthisBlock)); return true; } @@ -2388,8 +2389,6 @@ 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, ...). From 29e6e231c88904d0e17187b116db5a958d952bcf Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Mar 2017 17:14:53 -0500 Subject: [PATCH 03/12] Make ConnectTrace::blocksConnected private, hide behind accessors --- src/validation.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index ff38f60813..f2c90e028b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2212,7 +2212,17 @@ static int64_t nTimePostConnect = 0; * part of a single ActivateBestChainStep call. */ struct ConnectTrace { +private: std::vector > > blocksConnected; + +public: + void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { + blocksConnected.emplace_back(pindex, std::move(pblock)); + } + + std::vector > >& GetBlocksConnected() { + return blocksConnected; + } }; /** @@ -2270,7 +2280,7 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, LogPrint(BCLog::BENCH, " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001); LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001); - connectTrace.blocksConnected.emplace_back(pindexNew, std::move(pthisBlock)); + connectTrace.BlockConnected(pindexNew, std::move(pthisBlock)); return true; } @@ -2499,7 +2509,7 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified // Transactions in the connected block are notified - for (const auto& pair : connectTrace.blocksConnected) { + for (const auto& pair : connectTrace.GetBlocksConnected()) { assert(pair.second); const CBlock& block = *(pair.second); for (unsigned int i = 0; i < block.vtx.size(); i++) From d3167ba9bbefc2e5b7062f81c481547f21c5e44b Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Mar 2017 17:19:22 -0500 Subject: [PATCH 04/12] Handle conflicted transactions directly in ConnectTrace --- src/validation.cpp | 84 +++++++++++++++++++++------------------------- 1 file changed, 39 insertions(+), 45 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index f2c90e028b..d91afbb714 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -154,39 +154,6 @@ namespace { std::set setDirtyFileInfo; } // anon namespace -/* Use this class to start tracking transactions that are removed from the - * mempool and pass all those transactions through SyncTransaction when the - * object goes out of scope. This is currently only used to call SyncTransaction - * on conflicts removed from the mempool during block connection. Applied in - * ActivateBestChain around ActivateBestStep which in turn calls: - * ConnectTip->removeForBlock->removeConflicts - */ -class MemPoolConflictRemovalTracker -{ -private: - std::vector conflictedTxs; - CTxMemPool &pool; - -public: - MemPoolConflictRemovalTracker(CTxMemPool &_pool) : pool(_pool) { - pool.NotifyEntryRemoved.connect(boost::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, _1, _2)); - } - - void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { - if (reason == MemPoolRemovalReason::CONFLICT) { - conflictedTxs.push_back(txRemoved); - } - } - - ~MemPoolConflictRemovalTracker() { - pool.NotifyEntryRemoved.disconnect(boost::bind(&MemPoolConflictRemovalTracker::NotifyEntryRemoved, this, _1, _2)); - for (const auto& tx : conflictedTxs) { - GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - conflictedTxs.clear(); - } -}; - CBlockIndex* FindForkInGlobalIndex(const CChain& chain, const CBlockLocator& locator) { // Find the first block the caller has in the main chain @@ -2210,12 +2177,26 @@ static int64_t nTimePostConnect = 0; /** * Used to track blocks whose transactions were applied to the UTXO state as a * part of a single ActivateBestChainStep call. + * + * This class also tracks transactions that are removed from the mempool as + * conflicts and can be used to pass all those transactions through + * SyncTransaction. */ -struct ConnectTrace { +class ConnectTrace { private: std::vector > > blocksConnected; + std::vector conflictedTxs; + CTxMemPool &pool; public: + ConnectTrace(CTxMemPool &_pool) : pool(_pool) { + pool.NotifyEntryRemoved.connect(boost::bind(&ConnectTrace::NotifyEntryRemoved, this, _1, _2)); + } + + ~ConnectTrace() { + pool.NotifyEntryRemoved.disconnect(boost::bind(&ConnectTrace::NotifyEntryRemoved, this, _1, _2)); + } + void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { blocksConnected.emplace_back(pindex, std::move(pblock)); } @@ -2223,6 +2204,19 @@ public: std::vector > >& GetBlocksConnected() { return blocksConnected; } + + void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { + if (reason == MemPoolRemovalReason::CONFLICT) { + conflictedTxs.push_back(txRemoved); + } + } + + void CallSyncTransactionOnConflictedTransactions() { + for (const auto& tx : conflictedTxs) { + GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } + conflictedTxs.clear(); + } }; /** @@ -2470,18 +2464,11 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, break; const CBlockIndex *pindexFork; - ConnectTrace connectTrace; bool fInitialDownload; { LOCK(cs_main); - { // TODO: Temporarily ensure that mempool removals are notified before - // connected transactions. This shouldn't matter, but the abandoned - // state of transactions in our wallet is currently cleared when we - // receive another notification and there is a race condition where - // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertently cleared by - // the notification that the conflicted transaction was evicted. - MemPoolConflictRemovalTracker mrt(mempool); + ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked + CBlockIndex *pindexOldTip = chainActive.Tip(); if (pindexMostWork == NULL) { pindexMostWork = FindMostWorkChain(); @@ -2505,8 +2492,15 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, fInitialDownload = IsInitialBlockDownload(); // throw all transactions though the signal-interface - - } // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified + connectTrace.CallSyncTransactionOnConflictedTransactions(); + + // TODO: Temporarily ensure that mempool removals are notified before + // connected transactions. This shouldn't matter, but the abandoned + // state of transactions in our wallet is currently cleared when we + // receive another notification and there is a race condition where + // notification of a connected conflict might cause an outside process + // to abandon a transaction and then have it inadvertently cleared by + // the notification that the conflicted transaction was evicted. // Transactions in the connected block are notified for (const auto& pair : connectTrace.GetBlocksConnected()) { From a1476877ce7b0614a93b7ba48ebbe71075c0f27c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Mar 2017 17:22:50 -0500 Subject: [PATCH 05/12] Keep conflictedTxs in ConnectTrace per-block --- src/validation.cpp | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d91afbb714..cd9d941939 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2179,17 +2179,17 @@ static int64_t nTimePostConnect = 0; * part of a single ActivateBestChainStep call. * * This class also tracks transactions that are removed from the mempool as - * conflicts and can be used to pass all those transactions through - * SyncTransaction. + * conflicts (per block) and can be used to pass all those transactions + * through SyncTransaction. */ class ConnectTrace { private: std::vector > > blocksConnected; - std::vector conflictedTxs; + std::vector > conflictedTxs; CTxMemPool &pool; public: - ConnectTrace(CTxMemPool &_pool) : pool(_pool) { + ConnectTrace(CTxMemPool &_pool) : conflictedTxs(1), pool(_pool) { pool.NotifyEntryRemoved.connect(boost::bind(&ConnectTrace::NotifyEntryRemoved, this, _1, _2)); } @@ -2199,6 +2199,7 @@ public: void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { blocksConnected.emplace_back(pindex, std::move(pblock)); + conflictedTxs.emplace_back(); } std::vector > >& GetBlocksConnected() { @@ -2207,15 +2208,18 @@ public: void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { if (reason == MemPoolRemovalReason::CONFLICT) { - conflictedTxs.push_back(txRemoved); + conflictedTxs.back().push_back(txRemoved); } } void CallSyncTransactionOnConflictedTransactions() { - for (const auto& tx : conflictedTxs) { - GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + for (const auto& txRemovedForBlock : conflictedTxs) { + for (const auto& tx : txRemovedForBlock) { + GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } } conflictedTxs.clear(); + conflictedTxs.emplace_back(); } }; From f4043349106067de66a3312ed3485149bdb71247 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 7 Mar 2017 14:43:35 -0500 Subject: [PATCH 06/12] Handle SyncTransaction in ActivateBestChain instead of ConnectTrace This makes a later change to move it all into one per-block callback simpler. --- src/validation.cpp | 74 ++++++++++++++++++++++++++++++---------------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index cd9d941939..36b0b4ed58 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2174,6 +2174,12 @@ static int64_t nTimeFlush = 0; static int64_t nTimeChainState = 0; static int64_t nTimePostConnect = 0; +struct PerBlockConnectTrace { + CBlockIndex* pindex = NULL; + std::shared_ptr pblock; + std::shared_ptr> conflictedTxs; + PerBlockConnectTrace() : conflictedTxs(std::make_shared>()) {} +}; /** * Used to track blocks whose transactions were applied to the UTXO state as a * part of a single ActivateBestChainStep call. @@ -2181,15 +2187,22 @@ static int64_t nTimePostConnect = 0; * This class also tracks transactions that are removed from the mempool as * conflicts (per block) and can be used to pass all those transactions * through SyncTransaction. + * + * This class assumes (and asserts) that the conflicted transactions for a given + * block are added via mempool callbacks prior to the BlockConnected() associated + * with those transactions. If any transactions are marked conflicted, it is + * assumed that an associated block will always be added. + * + * This class is single-use, once you call GetBlocksConnected() you have to throw + * it away and make a new one. */ class ConnectTrace { private: - std::vector > > blocksConnected; - std::vector > conflictedTxs; + std::vector blocksConnected; CTxMemPool &pool; public: - ConnectTrace(CTxMemPool &_pool) : conflictedTxs(1), pool(_pool) { + ConnectTrace(CTxMemPool &_pool) : blocksConnected(1), pool(_pool) { pool.NotifyEntryRemoved.connect(boost::bind(&ConnectTrace::NotifyEntryRemoved, this, _1, _2)); } @@ -2198,29 +2211,32 @@ public: } void BlockConnected(CBlockIndex* pindex, std::shared_ptr pblock) { - blocksConnected.emplace_back(pindex, std::move(pblock)); - conflictedTxs.emplace_back(); - } - - std::vector > >& GetBlocksConnected() { + assert(!blocksConnected.back().pindex); + assert(pindex); + assert(pblock); + blocksConnected.back().pindex = pindex; + blocksConnected.back().pblock = std::move(pblock); + blocksConnected.emplace_back(); + } + + std::vector& GetBlocksConnected() { + // We always keep one extra block at the end of our list because + // blocks are added after all the conflicted transactions have + // been filled in. Thus, the last entry should always be an empty + // one waiting for the transactions from the next block. We pop + // the last entry here to make sure the list we return is sane. + assert(!blocksConnected.back().pindex); + assert(blocksConnected.back().conflictedTxs->empty()); + blocksConnected.pop_back(); return blocksConnected; } void NotifyEntryRemoved(CTransactionRef txRemoved, MemPoolRemovalReason reason) { + assert(!blocksConnected.back().pindex); if (reason == MemPoolRemovalReason::CONFLICT) { - conflictedTxs.back().push_back(txRemoved); + blocksConnected.back().conflictedTxs->emplace_back(std::move(txRemoved)); } } - - void CallSyncTransactionOnConflictedTransactions() { - for (const auto& txRemovedForBlock : conflictedTxs) { - for (const auto& tx : txRemovedForBlock) { - GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - } - conflictedTxs.clear(); - conflictedTxs.emplace_back(); - } }; /** @@ -2495,9 +2511,6 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, pindexFork = chainActive.FindFork(pindexOldTip); fInitialDownload = IsInitialBlockDownload(); - // throw all transactions though the signal-interface - connectTrace.CallSyncTransactionOnConflictedTransactions(); - // TODO: Temporarily ensure that mempool removals are notified before // connected transactions. This shouldn't matter, but the abandoned // state of transactions in our wallet is currently cleared when we @@ -2506,12 +2519,21 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, // to abandon a transaction and then have it inadvertently cleared by // the notification that the conflicted transaction was evicted. + // throw all transactions though the signal-interface + auto blocksConnected = connectTrace.GetBlocksConnected(); + for (const PerBlockConnectTrace& trace : blocksConnected) { + assert(trace.conflictedTxs); + for (const auto& tx : *trace.conflictedTxs) { + GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + } + } + // Transactions in the connected block are notified - for (const auto& pair : connectTrace.GetBlocksConnected()) { - assert(pair.second); - const CBlock& block = *(pair.second); + for (const PerBlockConnectTrace& trace : blocksConnected) { + assert(trace.pblock && trace.pindex); + const CBlock& block = *(trace.pblock); for (unsigned int i = 0; i < block.vtx.size(); i++) - GetMainSignals().SyncTransaction(*block.vtx[i], pair.first, i); + GetMainSignals().SyncTransaction(*block.vtx[i], trace.pindex, i); } } // When we reach this point, we switched to a new tip (stored in pindexNewTip). From 461e49fee2935b1eb4d4ea7bae3023e655c0a6d8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 29 Mar 2017 21:12:42 -0400 Subject: [PATCH 07/12] SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected This simplifies fixing the wallet-returns-stale-info issue as we can now hold cs_wallet across an entire block instead of only per-tx (though we only actually do so in the next commit). This change also removes the NOT_IN_BLOCK constant in favor of only passing the CBlockIndex* parameter to SyncTransactions when a new block is being connected, instead of also when a block is being disconnected. This change adds a parameter to BlockConnectedDisconnected which lists the transactions which were removed from mempool due to confliction as a result of this operation. While its somewhat of a shame to make block-validation-logic generate a list of mempool changes to be included in its generated callbacks, fixing this isnt too hard. Further in this change-set, CValidationInterface starts listening to mempool directly, placing it in the middle and giving it a bit of logic to know how to route notifications from block-validation, mempool, etc (though not listening for conflicted-removals yet). --- src/net_processing.cpp | 26 ++++++++++--------- src/net_processing.h | 2 +- src/validation.cpp | 33 +++++------------------ src/validationinterface.cpp | 12 ++++++--- src/validationinterface.h | 27 +++++++++---------- src/wallet/wallet.cpp | 39 +++++++++++++++++++++++++--- src/wallet/wallet.h | 7 ++++- src/zmq/zmqnotificationinterface.cpp | 22 +++++++++++++++- src/zmq/zmqnotificationinterface.h | 4 ++- 9 files changed, 109 insertions(+), 63 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 17653f542d..f838250379 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -744,21 +744,23 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn) : connman(connmanI recentRejects.reset(new CRollingBloomFilter(120000, 0.000001)); } -void PeerLogicValidation::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock) { - if (nPosInBlock == CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK) - return; - +void PeerLogicValidation::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex, const std::vector& vtxConflicted) { LOCK(cs_main); std::vector vOrphanErase; - // Which orphan pool entries must we evict? - for (size_t j = 0; j < tx.vin.size(); j++) { - auto itByPrev = mapOrphanTransactionsByPrev.find(tx.vin[j].prevout); - if (itByPrev == mapOrphanTransactionsByPrev.end()) continue; - for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) { - const CTransaction& orphanTx = *(*mi)->second.tx; - const uint256& orphanHash = orphanTx.GetHash(); - vOrphanErase.push_back(orphanHash); + + for (const CTransactionRef& ptx : pblock->vtx) { + const CTransaction& tx = *ptx; + + // Which orphan pool entries must we evict? + for (size_t j = 0; j < tx.vin.size(); j++) { + auto itByPrev = mapOrphanTransactionsByPrev.find(tx.vin[j].prevout); + if (itByPrev == mapOrphanTransactionsByPrev.end()) continue; + for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) { + const CTransaction& orphanTx = *(*mi)->second.tx; + const uint256& orphanHash = orphanTx.GetHash(); + vOrphanErase.push_back(orphanHash); + } } } diff --git a/src/net_processing.h b/src/net_processing.h index 9e3f1b7156..4b7e5b0757 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -30,7 +30,7 @@ private: public: PeerLogicValidation(CConnman* connmanIn); - virtual void SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock); + virtual void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted); virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); virtual void BlockChecked(const CBlock& block, const CValidationState& state); virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock); diff --git a/src/validation.cpp b/src/validation.cpp index 36b0b4ed58..c32cdf9dfc 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -949,7 +949,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C } } - GetMainSignals().SyncTransaction(tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); + GetMainSignals().TransactionAddedToMempool(ptx); return true; } @@ -2120,7 +2120,8 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara CBlockIndex *pindexDelete = chainActive.Tip(); assert(pindexDelete); // Read block from disk. - CBlock block; + std::shared_ptr pblock = std::make_shared(); + CBlock& block = *pblock; if (!ReadBlockFromDisk(block, pindexDelete, chainparams.GetConsensus())) return AbortNode(state, "Failed to read block"); // Apply the block atomically to the chain state. @@ -2162,9 +2163,7 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara UpdateTip(pindexDelete->pprev, chainparams); // Let wallets know transactions went from 1-confirmed to // 0-confirmed or conflicted: - for (const auto& tx : block.vtx) { - GetMainSignals().SyncTransaction(*tx, pindexDelete->pprev, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } + GetMainSignals().BlockDisconnected(pblock); return true; } @@ -2511,29 +2510,9 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, pindexFork = chainActive.FindFork(pindexOldTip); fInitialDownload = IsInitialBlockDownload(); - // TODO: Temporarily ensure that mempool removals are notified before - // connected transactions. This shouldn't matter, but the abandoned - // state of transactions in our wallet is currently cleared when we - // receive another notification and there is a race condition where - // notification of a connected conflict might cause an outside process - // to abandon a transaction and then have it inadvertently cleared by - // the notification that the conflicted transaction was evicted. - - // throw all transactions though the signal-interface - auto blocksConnected = connectTrace.GetBlocksConnected(); - for (const PerBlockConnectTrace& trace : blocksConnected) { - assert(trace.conflictedTxs); - for (const auto& tx : *trace.conflictedTxs) { - GetMainSignals().SyncTransaction(*tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK); - } - } - - // Transactions in the connected block are notified - for (const PerBlockConnectTrace& trace : blocksConnected) { + for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { assert(trace.pblock && trace.pindex); - const CBlock& block = *(trace.pblock); - for (unsigned int i = 0; i < block.vtx.size(); i++) - GetMainSignals().SyncTransaction(*block.vtx[i], trace.pindex, i); + GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs); } } // When we reach this point, we switched to a new tip (stored in pindexNewTip). diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index d4121a28bc..4d8d7d3678 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -14,7 +14,9 @@ CMainSignals& GetMainSignals() void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.UpdatedBlockTip.connect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); - g_signals.SyncTransaction.connect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2, _3)); + g_signals.TransactionAddedToMempool.connect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1)); + g_signals.BlockConnected.connect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); + g_signals.BlockDisconnected.connect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); g_signals.UpdatedTransaction.connect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); g_signals.SetBestChain.connect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.Inventory.connect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); @@ -33,7 +35,9 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.Inventory.disconnect(boost::bind(&CValidationInterface::Inventory, pwalletIn, _1)); g_signals.SetBestChain.disconnect(boost::bind(&CValidationInterface::SetBestChain, pwalletIn, _1)); g_signals.UpdatedTransaction.disconnect(boost::bind(&CValidationInterface::UpdatedTransaction, pwalletIn, _1)); - g_signals.SyncTransaction.disconnect(boost::bind(&CValidationInterface::SyncTransaction, pwalletIn, _1, _2, _3)); + g_signals.TransactionAddedToMempool.disconnect(boost::bind(&CValidationInterface::TransactionAddedToMempool, pwalletIn, _1)); + g_signals.BlockConnected.disconnect(boost::bind(&CValidationInterface::BlockConnected, pwalletIn, _1, _2, _3)); + g_signals.BlockDisconnected.disconnect(boost::bind(&CValidationInterface::BlockDisconnected, pwalletIn, _1)); g_signals.UpdatedBlockTip.disconnect(boost::bind(&CValidationInterface::UpdatedBlockTip, pwalletIn, _1, _2, _3)); g_signals.NewPoWValidBlock.disconnect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); } @@ -46,7 +50,9 @@ void UnregisterAllValidationInterfaces() { g_signals.Inventory.disconnect_all_slots(); g_signals.SetBestChain.disconnect_all_slots(); g_signals.UpdatedTransaction.disconnect_all_slots(); - g_signals.SyncTransaction.disconnect_all_slots(); + g_signals.TransactionAddedToMempool.disconnect_all_slots(); + g_signals.BlockConnected.disconnect_all_slots(); + g_signals.BlockDisconnected.disconnect_all_slots(); g_signals.UpdatedBlockTip.disconnect_all_slots(); g_signals.NewPoWValidBlock.disconnect_all_slots(); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 7f13a29d23..ddb0afbb4d 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -10,13 +10,14 @@ #include #include +#include "primitives/transaction.h" // CTransaction(Ref) + class CBlock; class CBlockIndex; struct CBlockLocator; class CBlockIndex; class CConnman; class CReserveScript; -class CTransaction; class CValidationInterface; class CValidationState; class uint256; @@ -33,7 +34,9 @@ void UnregisterAllValidationInterfaces(); class CValidationInterface { protected: virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} - virtual void SyncTransaction(const CTransaction &tx, const CBlockIndex *pindex, int posInBlock) {} + virtual void TransactionAddedToMempool(const CTransactionRef &ptxn) {} + virtual void BlockConnected(const std::shared_ptr &block, const CBlockIndex *pindex, const std::vector &txnConflicted) {} + virtual void BlockDisconnected(const std::shared_ptr &block) {} virtual void SetBestChain(const CBlockLocator &locator) {} virtual void UpdatedTransaction(const uint256 &hash) {} virtual void Inventory(const uint256 &hash) {} @@ -50,17 +53,15 @@ protected: struct CMainSignals { /** Notifies listeners of updated block chain tip */ boost::signals2::signal UpdatedBlockTip; - /** A posInBlock value for SyncTransaction calls for transactions not - * included in connected blocks such as transactions removed from mempool, - * accepted to mempool or appearing in disconnected blocks.*/ - static const int SYNC_TRANSACTION_NOT_IN_BLOCK = -1; - /** Notifies listeners of updated transaction data (transaction, and - * optionally the block it is found in). Called with block data when - * transaction is included in a connected block, and without block data when - * transaction was accepted to mempool, removed from mempool (only when - * removal was due to conflict from connected block), or appeared in a - * disconnected block.*/ - boost::signals2::signal SyncTransaction; + /** Notifies listeners of a transaction having been added to mempool. */ + boost::signals2::signal TransactionAddedToMempool; + /** + * Notifies listeners of a block being connected. + * Provides a vector of transactions evicted from the mempool as a result. + */ + boost::signals2::signal &, const CBlockIndex *pindex, const std::vector &)> BlockConnected; + /** Notifies listeners of a block being disconnected */ + boost::signals2::signal &)> BlockDisconnected; /** Notifies listeners of an updated transaction without new data (for now: a coinbase potentially becoming visible). */ boost::signals2::signal UpdatedTransaction; /** Notifies listeners of a new active block chain. */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 55d81daab4..2b66faf687 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1116,11 +1116,10 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) } } -void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock) -{ - LOCK2(cs_main, cs_wallet); +void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) { + const CTransaction& tx = *ptx; - if (!AddToWalletIfInvolvingMe(tx, pindex, posInBlock, true)) + if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true)) return; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance @@ -1133,6 +1132,38 @@ void CWallet::SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, } } +void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(ptx, NULL, -1); +} + +void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { + // TODO: Tempoarily ensure that mempool removals are notified before + // connected transactions. This shouldn't matter, but the abandoned + // state of transactions in our wallet is currently cleared when we + // receive another notification and there is a race condition where + // notification of a connected conflict might cause an outside process + // to abandon a transaction and then have it inadvertantly cleared by + // the notification that the conflicted transaction was evicted. + + for (const CTransactionRef& ptx : vtxConflicted) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(ptx, NULL, -1); + } + for (size_t i = 0; i < pblock->vtx.size(); i++) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(pblock->vtx[i], pindex, i); + } +} + +void CWallet::BlockDisconnected(const std::shared_ptr& pblock) { + for (const CTransactionRef& ptx : pblock->vtx) { + LOCK2(cs_main, cs_wallet); + SyncTransaction(ptx, NULL, -1); + } +} + + isminetype CWallet::IsMine(const CTxIn &txin) const { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ccede60097..d7890ba0ca 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -661,6 +661,9 @@ private: void SyncMetaData(std::pair); + /* Used by TransactionAddedToMemorypool/BlockConnected/Disconnected */ + void SyncTransaction(const CTransactionRef& tx, const CBlockIndex *pindexBlockConnected, int posInBlock); + /* the HD chain data model (external chain counters) */ CHDChain hdChain; @@ -849,7 +852,9 @@ public: void MarkDirty(); bool AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose=true); bool LoadToWallet(const CWalletTx& wtxIn); - void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock) override; + void TransactionAddedToMempool(const CTransactionRef& tx) override; + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; + void BlockDisconnected(const std::shared_ptr& pblock) override; bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate); CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false); void ReacceptWalletTransactions(); diff --git a/src/zmq/zmqnotificationinterface.cpp b/src/zmq/zmqnotificationinterface.cpp index fac2a3c57a..c063898056 100644 --- a/src/zmq/zmqnotificationinterface.cpp +++ b/src/zmq/zmqnotificationinterface.cpp @@ -144,8 +144,12 @@ void CZMQNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, co } } -void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int posInBlock) +void CZMQNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx) { + // Used by BlockConnected and BlockDisconnected as well, because they're + // all the same external callback. + const CTransaction& tx = *ptx; + for (std::list::iterator i = notifiers.begin(); i!=notifiers.end(); ) { CZMQAbstractNotifier *notifier = *i; @@ -160,3 +164,19 @@ void CZMQNotificationInterface::SyncTransaction(const CTransaction& tx, const CB } } } + +void CZMQNotificationInterface::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) +{ + for (const CTransactionRef& ptx : pblock->vtx) { + // Do a normal notify for each transaction added in the block + TransactionAddedToMempool(ptx); + } +} + +void CZMQNotificationInterface::BlockDisconnected(const std::shared_ptr& pblock) +{ + for (const CTransactionRef& ptx : pblock->vtx) { + // Do a normal notify for each transaction removed in block disconnection + TransactionAddedToMempool(ptx); + } +} diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index f22a539a3e..7d765d4090 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -25,7 +25,9 @@ protected: void Shutdown(); // CValidationInterface - void SyncTransaction(const CTransaction& tx, const CBlockIndex *pindex, int posInBlock); + void TransactionAddedToMempool(const CTransactionRef& tx); + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted); + void BlockDisconnected(const std::shared_ptr& pblock); void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); private: From e6d5e6cbbef3dca1bc9dce095124f7b79d01a54a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 8 Mar 2017 12:55:33 -0500 Subject: [PATCH 08/12] Hold cs_wallet for whole block [dis]connection processing This simplifies fixing the wallet-returns-stale-info issue as we now hold cs_wallet across an entire block instead of only per-tx. --- src/wallet/wallet.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2b66faf687..b04037774d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1138,6 +1138,7 @@ void CWallet::TransactionAddedToMempool(const CTransactionRef& ptx) { } void CWallet::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) { + LOCK2(cs_main, cs_wallet); // TODO: Tempoarily ensure that mempool removals are notified before // connected transactions. This shouldn't matter, but the abandoned // state of transactions in our wallet is currently cleared when we @@ -1147,18 +1148,17 @@ void CWallet::BlockConnected(const std::shared_ptr& pblock, const // the notification that the conflicted transaction was evicted. for (const CTransactionRef& ptx : vtxConflicted) { - LOCK2(cs_main, cs_wallet); SyncTransaction(ptx, NULL, -1); } for (size_t i = 0; i < pblock->vtx.size(); i++) { - LOCK2(cs_main, cs_wallet); SyncTransaction(pblock->vtx[i], pindex, i); } } void CWallet::BlockDisconnected(const std::shared_ptr& pblock) { + LOCK2(cs_main, cs_wallet); + for (const CTransactionRef& ptx : pblock->vtx) { - LOCK2(cs_main, cs_wallet); SyncTransaction(ptx, NULL, -1); } } From acad82f375615d8d238c7d819a48acd7900329f2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 8 Feb 2017 14:00:14 -0500 Subject: [PATCH 09/12] Add override to functions using CValidationInterface methods --- src/net_processing.h | 8 ++++---- src/rpc/mining.cpp | 2 +- src/zmq/zmqnotificationinterface.h | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/net_processing.h b/src/net_processing.h index 4b7e5b0757..f460595bc1 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -30,10 +30,10 @@ private: public: PeerLogicValidation(CConnman* connmanIn); - virtual void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted); - virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); - virtual void BlockChecked(const CBlock& block, const CValidationState& state); - virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock); + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) override; + void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; + void BlockChecked(const CBlock& block, const CValidationState& state) override; + void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& pblock) override; }; struct CNodeStateStats { diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 7e5f0d608e..cf7ca650b9 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -710,7 +710,7 @@ public: submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), found(false), state() {} protected: - virtual void BlockChecked(const CBlock& block, const CValidationState& stateIn) { + void BlockChecked(const CBlock& block, const CValidationState& stateIn) override { if (block.GetHash() != hash) return; found = true; diff --git a/src/zmq/zmqnotificationinterface.h b/src/zmq/zmqnotificationinterface.h index 7d765d4090..eec6f7bc64 100644 --- a/src/zmq/zmqnotificationinterface.h +++ b/src/zmq/zmqnotificationinterface.h @@ -25,10 +25,10 @@ protected: void Shutdown(); // CValidationInterface - void TransactionAddedToMempool(const CTransactionRef& tx); - void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted); - void BlockDisconnected(const std::shared_ptr& pblock); - void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload); + void TransactionAddedToMempool(const CTransactionRef& tx) override; + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindexConnected, const std::vector& vtxConflicted) override; + void BlockDisconnected(const std::shared_ptr& pblock) override; + void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override; private: CZMQNotificationInterface(); From 91f1e6ce5e7854435196464aace0dcf7ce21dd5a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Jan 2017 15:47:03 -0500 Subject: [PATCH 10/12] Remove dead-code tracking of requests for blocks we generated --- src/validationinterface.cpp | 3 --- src/validationinterface.h | 3 --- src/wallet/wallet.h | 5 ----- 3 files changed, 11 deletions(-) diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 4d8d7d3678..0f699328c7 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -23,12 +23,10 @@ void RegisterValidationInterface(CValidationInterface* pwalletIn) { g_signals.Broadcast.connect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); g_signals.BlockChecked.connect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.ScriptForMining.connect(boost::bind(&CValidationInterface::GetScriptForMining, pwalletIn, _1)); - g_signals.BlockFound.connect(boost::bind(&CValidationInterface::ResetRequestCount, pwalletIn, _1)); g_signals.NewPoWValidBlock.connect(boost::bind(&CValidationInterface::NewPoWValidBlock, pwalletIn, _1, _2)); } void UnregisterValidationInterface(CValidationInterface* pwalletIn) { - g_signals.BlockFound.disconnect(boost::bind(&CValidationInterface::ResetRequestCount, pwalletIn, _1)); g_signals.ScriptForMining.disconnect(boost::bind(&CValidationInterface::GetScriptForMining, pwalletIn, _1)); g_signals.BlockChecked.disconnect(boost::bind(&CValidationInterface::BlockChecked, pwalletIn, _1, _2)); g_signals.Broadcast.disconnect(boost::bind(&CValidationInterface::ResendWalletTransactions, pwalletIn, _1, _2)); @@ -43,7 +41,6 @@ void UnregisterValidationInterface(CValidationInterface* pwalletIn) { } void UnregisterAllValidationInterfaces() { - g_signals.BlockFound.disconnect_all_slots(); g_signals.ScriptForMining.disconnect_all_slots(); g_signals.BlockChecked.disconnect_all_slots(); g_signals.Broadcast.disconnect_all_slots(); diff --git a/src/validationinterface.h b/src/validationinterface.h index ddb0afbb4d..baa04fe312 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -43,7 +43,6 @@ protected: virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {} virtual void BlockChecked(const CBlock&, const CValidationState&) {} virtual void GetScriptForMining(boost::shared_ptr&) {}; - virtual void ResetRequestCount(const uint256 &hash) {}; virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); @@ -79,8 +78,6 @@ struct CMainSignals { boost::signals2::signal BlockChecked; /** Notifies listeners that a key for mining is required (coinbase) */ boost::signals2::signal&)> ScriptForMining; - /** Notifies listeners that a block has been successfully mined */ - boost::signals2::signal BlockFound; /** * Notifies listeners that a block which builds directly on our current tip * has been received and connected to the headers tree, though not validated yet */ diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d7890ba0ca..127ffd6cf3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -964,11 +964,6 @@ public: } void GetScriptForMining(boost::shared_ptr &script) override; - void ResetRequestCount(const uint256 &hash) override - { - LOCK(cs_wallet); - mapRequestCount[hash] = 0; - }; unsigned int GetKeyPoolSize() { From 1c95e2f9c94f172ddddedeb1358953992f39f8bd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Jan 2017 16:15:41 -0500 Subject: [PATCH 11/12] Use std::shared_ptr instead of boost::shared_ptr in ScriptForMining --- src/rpc/mining.cpp | 7 +++---- src/validationinterface.h | 5 ++--- src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 4 +--- 4 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index cf7ca650b9..d234bb69ae 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -27,7 +27,6 @@ #include #include -#include #include @@ -95,7 +94,7 @@ UniValue getnetworkhashps(const JSONRPCRequest& request) return GetNetworkHashPS(request.params.size() > 0 ? request.params[0].get_int() : 120, request.params.size() > 1 ? request.params[1].get_int() : -1); } -UniValue generateBlocks(boost::shared_ptr coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript) +UniValue generateBlocks(std::shared_ptr coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript) { static const int nInnerLoopCount = 0x10000; int nHeightStart = 0; @@ -167,7 +166,7 @@ UniValue generate(const JSONRPCRequest& request) nMaxTries = request.params[1].get_int(); } - boost::shared_ptr coinbaseScript; + std::shared_ptr coinbaseScript; GetMainSignals().ScriptForMining(coinbaseScript); // If the keypool is exhausted, no script is returned at all. Catch this. @@ -208,7 +207,7 @@ UniValue generatetoaddress(const JSONRPCRequest& request) if (!address.IsValid()) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Error: Invalid address"); - boost::shared_ptr coinbaseScript(new CReserveScript()); + std::shared_ptr coinbaseScript = std::make_shared(); coinbaseScript->reserveScript = GetScriptForDestination(address.Get()); return generateBlocks(coinbaseScript, nGenerate, nMaxTries, false); diff --git a/src/validationinterface.h b/src/validationinterface.h index baa04fe312..083c136f2c 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -7,7 +7,6 @@ #define BITCOIN_VALIDATIONINTERFACE_H #include -#include #include #include "primitives/transaction.h" // CTransaction(Ref) @@ -42,7 +41,7 @@ protected: virtual void Inventory(const uint256 &hash) {} virtual void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) {} virtual void BlockChecked(const CBlock&, const CValidationState&) {} - virtual void GetScriptForMining(boost::shared_ptr&) {}; + virtual void GetScriptForMining(std::shared_ptr&) {}; virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr& block) {}; friend void ::RegisterValidationInterface(CValidationInterface*); friend void ::UnregisterValidationInterface(CValidationInterface*); @@ -77,7 +76,7 @@ struct CMainSignals { */ boost::signals2::signal BlockChecked; /** Notifies listeners that a key for mining is required (coinbase) */ - boost::signals2::signal&)> ScriptForMining; + boost::signals2::signal&)> ScriptForMining; /** * Notifies listeners that a block which builds directly on our current tip * has been received and connected to the headers tree, though not validated yet */ diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b04037774d..dc145fd3d2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3376,9 +3376,9 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx) } } -void CWallet::GetScriptForMining(boost::shared_ptr &script) +void CWallet::GetScriptForMining(std::shared_ptr &script) { - boost::shared_ptr rKey(new CReserveKey(this)); + std::shared_ptr rKey = std::make_shared(this); CPubKey pubkey; if (!rKey->GetReservedKey(pubkey)) return; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 127ffd6cf3..daae930399 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -28,8 +28,6 @@ #include #include -#include - extern CWallet* pwalletMain; /** @@ -963,7 +961,7 @@ public: } } - void GetScriptForMining(boost::shared_ptr &script) override; + void GetScriptForMining(std::shared_ptr &script) override; unsigned int GetKeyPoolSize() { From b1a6d4cd560fbdb66506841860db03c08ea4bbbc Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 6 Mar 2017 18:21:27 -0500 Subject: [PATCH 12/12] Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy --- src/wallet/wallet.cpp | 9 +++++---- src/wallet/wallet.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index dc145fd3d2..1b92c5e747 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -966,8 +966,9 @@ bool CWallet::LoadToWallet(const CWalletTx& wtxIn) * Abandoned state should probably be more carefully tracked via different * posInBlock signals or by checking mempool presence when necessary. */ -bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate) +bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate) { + const CTransaction& tx = *ptx; { AssertLockHeld(cs_wallet); @@ -988,7 +989,7 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex if (fExisted && !fUpdate) return false; if (fExisted || IsMine(tx) || IsFromMe(tx)) { - CWalletTx wtx(this, MakeTransactionRef(tx)); + CWalletTx wtx(this, ptx); // Get merkle branch if transaction was found in a block if (posInBlock != -1) @@ -1119,7 +1120,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) void CWallet::SyncTransaction(const CTransactionRef& ptx, const CBlockIndex *pindexBlockConnected, int posInBlock) { const CTransaction& tx = *ptx; - if (!AddToWalletIfInvolvingMe(tx, pindexBlockConnected, posInBlock, true)) + if (!AddToWalletIfInvolvingMe(ptx, pindexBlockConnected, posInBlock, true)) return; // Not one of ours // If a transaction changes 'conflicted' state, that changes the balance @@ -1542,7 +1543,7 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f CBlock block; if (ReadBlockFromDisk(block, pindex, Params().GetConsensus())) { for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) { - AddToWalletIfInvolvingMe(*block.vtx[posInBlock], pindex, posInBlock, fUpdate); + AddToWalletIfInvolvingMe(block.vtx[posInBlock], pindex, posInBlock, fUpdate); } if (!ret) { ret = pindex; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index daae930399..05c00326ec 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -853,7 +853,7 @@ public: void TransactionAddedToMempool(const CTransactionRef& tx) override; void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex *pindex, const std::vector& vtxConflicted) override; void BlockDisconnected(const std::shared_ptr& pblock) override; - bool AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate); + bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, const CBlockIndex* pIndex, int posInBlock, bool fUpdate); CBlockIndex* ScanForWalletTransactions(CBlockIndex* pindexStart, bool fUpdate = false); void ReacceptWalletTransactions(); void ResendWalletTransactions(int64_t nBestBlockTime, CConnman* connman) override;