highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-21 07:39:08 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge #13023: Fix some concurrency issues in ActivateBestChain()

Browse Source 
dd435ad Add unit tests for signals generated by ProcessNewBlock() (Jesse Cohen)
a3ae8e6 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen)
ecc3c4a Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo)

Pull request description:

  Originally this PR was just to add tests around concurrency in block validation - those tests seem to have uncovered another bug in ActivateBestChain - this now fixes that bug and adds tests.

  ActivateBestChain (invoked after a new block is validated) proceeds in steps - acquiring and releasing cs_main while incrementally disconnecting and connecting blocks to sync to the most work chain known (FindMostWorkChain()). Every time cs_main is released the result of FindMostWorkChain() can change - but currently that value is cached across acquisitions of cs_main and only refreshed when an invalid chain is explored. It needs to be refreshed every time cs_main is reacquired. The test added in 6094ce7304 will occasionally fail without the commit fixing this issue 26bfdbaddb

  Original description below
  --

  After a bug discovered where UpdatedBlockTip() notifications could be triggered out of order (#12978), these unit tests check certain invariants about these signals.

  The scheduler test asserts that a SingleThreadedSchedulerClient processes callbacks fully and sequentially.

  The block validation test generates a random chain and calls ProcessNewBlock from multiple threads at random and in parallel. ValidationInterface callbacks verify that the ordering of BlockConnected BlockDisconnected and UpdatedBlockTip events occur as expected.

Tree-SHA512: 4102423a03d2ea28580c7a70add8a6bdb22ef9e33b107c3aadef80d5af02644cdfaae516c44933924717599c81701e0b96fbf9cf38696e9e41372401a5ee1f3c
This commit is contained in:
Wladimir J. van der Laan
2018-05-16 17:49:29 +02:00
parent 40c34a0a29 dd435ad402
commit 11e7bdfd90
6 changed files with 251 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

78
src/validation.cpp
View File

@@ -145,6 +145,12 @@ private:
*/
std::set<CBlockIndex*> m_failed_blocks;
/**
* the ChainState CriticalSection
* A lock that must be held when modifying this ChainState - held in ActivateBestChain()
*/
CCriticalSection m_cs_chainstate;
public:
CChain chainActive;
BlockMap mapBlockIndex;
@@ -2519,6 +2525,7 @@ void CChainState::PruneBlockIndexCandidates() {
bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
{
AssertLockHeld(cs_main);
const CBlockIndex *pindexOldTip = chainActive.Tip();
const CBlockIndex *pindexFork = chainActive.FindFork(pindexMostWork);
@@ -2635,6 +2642,12 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
// sanely for performance or correctness!
AssertLockNotHeld(cs_main);
// ABC maintains a fair degree of expensive-to-calculate internal state
// because this function periodically releases cs_main so that it does not lock up other threads for too long
// during large connects - and to allow for e.g. the callback queue to drain
// we use m_cs_chainstate to enforce mutual exclusion so that only one caller may execute this function at a time
LOCK(m_cs_chainstate);
CBlockIndex *pindexMostWork = nullptr;
CBlockIndex *pindexNewTip = nullptr;
int nStopAtHeight = gArgs.GetArg("-stopatheight", DEFAULT_STOPATHEIGHT);
@@ -2648,45 +2661,53 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
SyncWithValidationInterfaceQueue();
}
const CBlockIndex *pindexFork;
bool fInitialDownload;
{
LOCK(cs_main);
ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked
CBlockIndex* starting_tip = chainActive.Tip();
bool blocks_connected = false;
do {
// We absolutely may not unlock cs_main until we've made forward progress
// (with the exception of shutdown due to hardware issues, low disk space, etc).
ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked
CBlockIndex *pindexOldTip = chainActive.Tip();
if (pindexMostWork == nullptr) {
pindexMostWork = FindMostWorkChain();
}
if (pindexMostWork == nullptr) {
pindexMostWork = FindMostWorkChain();
}
// Whether we have anything to do at all.
if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip())
return true;
// Whether we have anything to do at all.
if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) {
break;
}
bool fInvalidFound = false;
std::shared_ptr<const CBlock> nullBlockPtr;
if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace))
return false;
bool fInvalidFound = false;
std::shared_ptr<const CBlock> nullBlockPtr;
if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace))
return false;
blocks_connected = true;
if (fInvalidFound) {
// Wipe cache, we may need another branch now.
pindexMostWork = nullptr;
}
pindexNewTip = chainActive.Tip();
pindexFork = chainActive.FindFork(pindexOldTip);
fInitialDownload = IsInitialBlockDownload();
if (fInvalidFound) {
// Wipe cache, we may need another branch now.
pindexMostWork = nullptr;
}
pindexNewTip = chainActive.Tip();
for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
assert(trace.pblock && trace.pindex);
GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs);
}
for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
assert(trace.pblock && trace.pindex);
GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs);
}
} while (!chainActive.Tip() || (starting_tip && CBlockIndexWorkComparator()(chainActive.Tip(), starting_tip)));
if (!blocks_connected) return true;
const CBlockIndex* pindexFork = chainActive.FindFork(starting_tip);
bool fInitialDownload = IsInitialBlockDownload();
// Notify external listeners about the new tip.
// Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected
GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
// Always notify the UI if a new block tip was connected
if (pindexFork != pindexNewTip) {
// Notify ValidationInterface subscribers
GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
// Always notify the UI if a new block tip was connected
uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
}
}
@@ -2710,6 +2731,7 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
return true;
}
bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock) {
return g_chainstate.ActivateBestChain(state, chainparams, std::move(pblock));
}