Skip to content

Commit

Permalink
Merge bitcoin#9725: CValidationInterface Cleanups
Browse files Browse the repository at this point in the history 
b1a6d4c Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo)
1c95e2f Use std::shared_ptr instead of boost::shared_ptr in ScriptForMining (Matt Corallo)
91f1e6c Remove dead-code tracking of requests for blocks we generated (Matt Corallo)
acad82f Add override to functions using CValidationInterface methods (Matt Corallo)
e6d5e6c Hold cs_wallet for whole block [dis]connection processing (Matt Corallo)
461e49f SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected (Matt Corallo)
f404334 Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo)
a147687 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo)
d3167ba Handle conflicted transactions directly in ConnectTrace (Matt Corallo)
29e6e23 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo)
822000c Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo)
f5e9a01 Include missing #include in zmqnotificationinterface.h (Matt Corallo)

Tree-SHA512: 8893d47559da3b28d2ef7359768547cba8a4b43b6f891d80f5848f995a84b1517bfb0f706fdc8cd43f09a1350349eb440d9724a59363ab517dfcc4fcb31b2018
  • Loading branch information
@laanwj @UdjinM6
laanwj authored and UdjinM6 committed May 23, 2019
1 parent 7ea034c commit 321a936
Show file tree
Hide file tree
Showing 10 changed files with 196 additions and 134 deletions.
26 changes: 14 additions & 12 deletions src/net_processing.cpp
View file
Expand Up @@ -806,21 +806,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<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted) {
LOCK(g_cs_orphans);

std::vector<uint256> 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);
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions src/net_processing.h
View file
Expand Up @@ -36,10 +36,10 @@ class PeerLogicValidation : public CValidationInterface {
public:
PeerLogicValidation(CConnman* connmanIn);

virtual void SyncTransaction(const CTransaction& tx, const CBlockIndex* pindex, int nPosInBlock) override;
virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
virtual void BlockChecked(const CBlock& block, const CValidationState& state) override;
virtual void NewPoWValidBlock(const CBlockIndex *pindex, const std::shared_ptr<const CBlock>& pblock) override;
void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected, const std::vector<CTransactionRef>& 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<const CBlock>& pblock) override;
};

struct CNodeStateStats {
Expand Down
9 changes: 4 additions & 5 deletions src/rpc/mining.cpp
View file
Expand Up @@ -37,7 +37,6 @@
#include <stdint.h>

#include <boost/assign/list_of.hpp>
#include <boost/shared_ptr.hpp>

#include <univalue.h>

Expand Down Expand Up @@ -106,7 +105,7 @@ UniValue getnetworkhashps(const JSONRPCRequest& request)
}

#if ENABLE_MINER
UniValue generateBlocks(boost::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript)
UniValue generateBlocks(std::shared_ptr<CReserveScript> coinbaseScript, int nGenerate, uint64_t nMaxTries, bool keepScript)
{
static const int nInnerLoopCount = 0x10000;
int nHeightStart = 0;
Expand Down Expand Up @@ -178,7 +177,7 @@ UniValue generate(const JSONRPCRequest& request)
nMaxTries = request.params[1].get_int();
}

boost::shared_ptr<CReserveScript> coinbaseScript;
std::shared_ptr<CReserveScript> coinbaseScript;
GetMainSignals().ScriptForMining(coinbaseScript);

// If the keypool is exhausted, no script is returned at all. Catch this.
Expand Down Expand Up @@ -219,7 +218,7 @@ UniValue generatetoaddress(const JSONRPCRequest& request)
if (!address.IsValid())
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Error: Invalid address");

boost::shared_ptr<CReserveScript> coinbaseScript(new CReserveScript());
std::shared_ptr<CReserveScript> coinbaseScript = std::make_shared<CReserveScript>();
coinbaseScript->reserveScript = GetScriptForDestination(address.Get());

return generateBlocks(coinbaseScript, nGenerate, nMaxTries, false);
Expand Down Expand Up @@ -752,7 +751,7 @@ class submitblock_StateCatcher : public CValidationInterface
submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), found(false), state() {}

protected:
virtual void BlockChecked(const CBlock& block, const CValidationState& stateIn) override {
void BlockChecked(const CBlock& block, const CValidationState& stateIn) override {
if (block.GetHash() != hash)
return;
found = true;
Expand Down
142 changes: 75 additions & 67 deletions src/validation.cpp
View file
Expand Up @@ -176,39 +176,6 @@ namespace {
std::set<int> 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<CTransactionRef> 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
Expand Down Expand Up @@ -908,7 +875,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C
}

if(!fDryRun)
GetMainSignals().SyncTransaction(tx, NULL, CMainSignals::SYNC_TRANSACTION_NOT_IN_BLOCK);
GetMainSignals().TransactionAddedToMempool(ptx);

return true;
}
Expand Down Expand Up @@ -2534,7 +2501,8 @@ bool static DisconnectTip(CValidationState& state, const CChainParams& chainpara
CBlockIndex *pindexDelete = chainActive.Tip();
assert(pindexDelete);
// Read block from disk.
CBlock block;
std::shared_ptr<CBlock> pblock = std::make_shared<CBlock>();
CBlock& block = *pblock;
if (!ReadBlockFromDisk(block, pindexDelete, chainparams.GetConsensus()))
return AbortNode(state, "Failed to read block");
// Apply the block atomically to the chain state.
Expand Down Expand Up @@ -2575,9 +2543,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;
}

Expand All @@ -2587,36 +2553,92 @@ static int64_t nTimeFlush = 0;
static int64_t nTimeChainState = 0;
static int64_t nTimePostConnect = 0;

struct PerBlockConnectTrace {
CBlockIndex* pindex = NULL;
std::shared_ptr<const CBlock> pblock;
std::shared_ptr<std::vector<CTransactionRef>> conflictedTxs;
PerBlockConnectTrace() : conflictedTxs(std::make_shared<std::vector<CTransactionRef>>()) {}
};
/**
* 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 (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.
*/
struct ConnectTrace {
std::vector<std::pair<CBlockIndex*, std::shared_ptr<const CBlock> > > blocksConnected;
class ConnectTrace {
private:
std::vector<PerBlockConnectTrace> blocksConnected;
CTxMemPool &pool;

public:
ConnectTrace(CTxMemPool &_pool) : blocksConnected(1), 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<const CBlock> pblock) {
assert(!blocksConnected.back().pindex);
assert(pindex);
assert(pblock);
blocksConnected.back().pindex = pindex;
blocksConnected.back().pblock = std::move(pblock);
blocksConnected.emplace_back();
}

std::vector<PerBlockConnectTrace>& 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) {
blocksConnected.back().conflictedTxs->emplace_back(std::move(txRemoved));
}
}
};

/**
* 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<const CBlock>& pblock, ConnectTrace& connectTrace)
{
assert(pindexNew->pprev == chainActive.Tip());
// Read block from disk.
int64_t nTime1 = GetTimeMicros();
std::shared_ptr<const CBlock> pthisBlock;
if (!pblock) {
std::shared_ptr<CBlock> pblockNew = std::make_shared<CBlock>();
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;
Expand Down Expand Up @@ -2653,6 +2675,8 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams,
int64_t nTime6 = GetTimeMicros(); nTimePostConnect += nTime6 - nTime5; nTimeTotal += nTime6 - nTime1;
LogPrint(BCLog::BENCHMARK, " - Connect postprocess: %.2fms [%.2fs]\n", (nTime6 - nTime5) * 0.001, nTimePostConnect * 0.000001);
LogPrint(BCLog::BENCHMARK, "- Connect block: %.2fms [%.2fs]\n", (nTime6 - nTime1) * 0.001, nTimeTotal * 0.000001);

connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
return true;
}

Expand Down Expand Up @@ -2814,8 +2838,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, ...).
Expand Down Expand Up @@ -2894,18 +2916,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();
Expand All @@ -2928,16 +2943,9 @@ bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams,
pindexFork = chainActive.FindFork(pindexOldTip);
fInitialDownload = IsInitialBlockDownload();

// throw all transactions though the signal-interface

} // MemPoolConflictRemovalTracker destroyed and conflict evictions are notified

// Transactions in the connected block are notified
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);
for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
assert(trace.pblock && trace.pindex);
GetMainSignals().BlockConnected(trace.pblock, trace.pindex, *trace.conflictedTxs);
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
Expand Down

0 comments on commit 321a936

Please sign in to comment.