Skip to content

Commit 11e7bdf

Browse files
committed
Merge #13023: Fix some concurrency issues in ActivateBestChain()
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 bitcoin/bitcoin@6094ce7 will occasionally fail without the commit fixing this issue bitcoin/bitcoin@26bfdba 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
2 parents 40c34a0 + dd435ad commit 11e7bdf

File tree

6 files changed

+251
-31
lines changed

6 files changed

+251
-31
lines changed

src/Makefile.test.include

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,10 @@ BITCOIN_TESTS =\
8686
test/txindex_tests.cpp \
8787
test/txvalidation_tests.cpp \
8888
test/txvalidationcache_tests.cpp \
89-
test/versionbits_tests.cpp \
9089
test/uint256_tests.cpp \
91-
test/util_tests.cpp
90+
test/util_tests.cpp \
91+
test/validation_block_tests.cpp \
92+
test/versionbits_tests.cpp
9293

9394
if ENABLE_WALLET
9495
BITCOIN_TESTS += \

src/test/test_bitcoin.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@ FastRandomContext insecure_rand_ctx(insecure_rand_seed);
3939
extern bool fPrintToConsole;
4040
extern void noui_connect();
4141

42+
std::ostream& operator<<(std::ostream& os, const uint256& num)
43+
{
44+
os << num.ToString();
45+
return os;
46+
}
47+
4248
BasicTestingSetup::BasicTestingSetup(const std::string& chainName)
4349
{
4450
SHA256AutoDetect();

src/test/test_bitcoin.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,4 +120,7 @@ struct TestMemPoolEntryHelper
120120

121121
CBlock getBlock13b8a();
122122

123+
// define an implicit conversion here so that uint256 may be used directly in BOOST_CHECK_*
124+
std::ostream& operator<<(std::ostream& os, const uint256& num);
125+
123126
#endif

src/test/validation_block_tests.cpp

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
// Copyright (c) 2018 The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <boost/test/unit_test.hpp>
6+
7+
#include <chainparams.h>
8+
#include <consensus/merkle.h>
9+
#include <consensus/validation.h>
10+
#include <miner.h>
11+
#include <pow.h>
12+
#include <random.h>
13+
#include <test/test_bitcoin.h>
14+
#include <validation.h>
15+
#include <validationinterface.h>
16+
17+
struct RegtestingSetup : public TestingSetup {
18+
RegtestingSetup() : TestingSetup(CBaseChainParams::REGTEST) {}
19+
};
20+
21+
BOOST_FIXTURE_TEST_SUITE(validation_block_tests, RegtestingSetup)
22+
23+
struct TestSubscriber : public CValidationInterface {
24+
uint256 m_expected_tip;
25+
26+
TestSubscriber(uint256 tip) : m_expected_tip(tip) {}
27+
28+
void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload)
29+
{
30+
BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash());
31+
}
32+
33+
void BlockConnected(const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex, const std::vector<CTransactionRef>& txnConflicted)
34+
{
35+
BOOST_CHECK_EQUAL(m_expected_tip, block->hashPrevBlock);
36+
BOOST_CHECK_EQUAL(m_expected_tip, pindex->pprev->GetBlockHash());
37+
38+
m_expected_tip = block->GetHash();
39+
}
40+
41+
void BlockDisconnected(const std::shared_ptr<const CBlock>& block)
42+
{
43+
BOOST_CHECK_EQUAL(m_expected_tip, block->GetHash());
44+
45+
m_expected_tip = block->hashPrevBlock;
46+
}
47+
};
48+
49+
std::shared_ptr<CBlock> Block(const uint256& prev_hash)
50+
{
51+
static int i = 0;
52+
static uint64_t time = Params().GenesisBlock().nTime;
53+
54+
CScript pubKey;
55+
pubKey << i++ << OP_TRUE;
56+
57+
auto ptemplate = BlockAssembler(Params()).CreateNewBlock(pubKey, false);
58+
auto pblock = std::make_shared<CBlock>(ptemplate->block);
59+
pblock->hashPrevBlock = prev_hash;
60+
pblock->nTime = ++time;
61+
62+
CMutableTransaction txCoinbase(*pblock->vtx[0]);
63+
txCoinbase.vout.resize(1);
64+
txCoinbase.vin[0].scriptWitness.SetNull();
65+
pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase));
66+
67+
return pblock;
68+
}
69+
70+
std::shared_ptr<CBlock> FinalizeBlock(std::shared_ptr<CBlock> pblock)
71+
{
72+
pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
73+
74+
while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits, Params().GetConsensus())) {
75+
++(pblock->nNonce);
76+
}
77+
78+
return pblock;
79+
}
80+
81+
// construct a valid block
82+
const std::shared_ptr<const CBlock> GoodBlock(const uint256& prev_hash)
83+
{
84+
return FinalizeBlock(Block(prev_hash));
85+
}
86+
87+
// construct an invalid block (but with a valid header)
88+
const std::shared_ptr<const CBlock> BadBlock(const uint256& prev_hash)
89+
{
90+
auto pblock = Block(prev_hash);
91+
92+
CMutableTransaction coinbase_spend;
93+
coinbase_spend.vin.push_back(CTxIn(COutPoint(pblock->vtx[0]->GetHash(), 0), CScript(), 0));
94+
coinbase_spend.vout.push_back(pblock->vtx[0]->vout[0]);
95+
96+
CTransactionRef tx = MakeTransactionRef(coinbase_spend);
97+
pblock->vtx.push_back(tx);
98+
99+
auto ret = FinalizeBlock(pblock);
100+
return ret;
101+
}
102+
103+
void BuildChain(const uint256& root, int height, const unsigned int invalid_rate, const unsigned int branch_rate, const unsigned int max_size, std::vector<std::shared_ptr<const CBlock>>& blocks)
104+
{
105+
if (height <= 0 || blocks.size() >= max_size) return;
106+
107+
bool gen_invalid = GetRand(100) < invalid_rate;
108+
bool gen_fork = GetRand(100) < branch_rate;
109+
110+
const std::shared_ptr<const CBlock> pblock = gen_invalid ? BadBlock(root) : GoodBlock(root);
111+
blocks.push_back(pblock);
112+
if (!gen_invalid) {
113+
BuildChain(pblock->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks);
114+
}
115+
116+
if (gen_fork) {
117+
blocks.push_back(GoodBlock(root));
118+
BuildChain(blocks.back()->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks);
119+
}
120+
}
121+
122+
BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
123+
{
124+
// build a large-ish chain that's likely to have some forks
125+
std::vector<std::shared_ptr<const CBlock>> blocks;
126+
while (blocks.size() < 50) {
127+
blocks.clear();
128+
BuildChain(Params().GenesisBlock().GetHash(), 100, 15, 10, 500, blocks);
129+
}
130+
131+
bool ignored;
132+
CValidationState state;
133+
std::vector<CBlockHeader> headers;
134+
std::transform(blocks.begin(), blocks.end(), std::back_inserter(headers), [](std::shared_ptr<const CBlock> b) { return b->GetBlockHeader(); });
135+
136+
// Process all the headers so we understand the toplogy of the chain
137+
BOOST_CHECK(ProcessNewBlockHeaders(headers, state, Params()));
138+
139+
// Connect the genesis block and drain any outstanding events
140+
ProcessNewBlock(Params(), std::make_shared<CBlock>(Params().GenesisBlock()), true, &ignored);
141+
SyncWithValidationInterfaceQueue();
142+
143+
// subscribe to events (this subscriber will validate event ordering)
144+
const CBlockIndex* initial_tip = nullptr;
145+
{
146+
LOCK(cs_main);
147+
initial_tip = chainActive.Tip();
148+
}
149+
TestSubscriber sub(initial_tip->GetBlockHash());
150+
RegisterValidationInterface(&sub);
151+
152+
// create a bunch of threads that repeatedly process a block generated above at random
153+
// this will create parallelism and randomness inside validation - the ValidationInterface
154+
// will subscribe to events generated during block validation and assert on ordering invariance
155+
boost::thread_group threads;
156+
for (int i = 0; i < 10; i++) {
157+
threads.create_thread([&blocks]() {
158+
bool ignored;
159+
for (int i = 0; i < 1000; i++) {
160+
auto block = blocks[GetRand(blocks.size() - 1)];
161+
ProcessNewBlock(Params(), block, true, &ignored);
162+
}
163+
164+
// to make sure that eventually we process the full chain - do it here
165+
for (auto block : blocks) {
166+
if (block->vtx.size() == 1) {
167+
bool processed = ProcessNewBlock(Params(), block, true, &ignored);
168+
assert(processed);
169+
}
170+
}
171+
});
172+
}
173+
174+
threads.join_all();
175+
while (GetMainSignals().CallbacksPending() > 0) {
176+
MilliSleep(100);
177+
}
178+
179+
UnregisterValidationInterface(&sub);
180+
181+
BOOST_CHECK_EQUAL(sub.m_expected_tip, chainActive.Tip()->GetBlockHash());
182+
}
183+
184+
BOOST_AUTO_TEST_SUITE_END()

src/validation.cpp

Lines changed: 50 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,12 @@ class CChainState {
145145
*/
146146
std::set<CBlockIndex*> m_failed_blocks;
147147

148+
/**
149+
* the ChainState CriticalSection
150+
* A lock that must be held when modifying this ChainState - held in ActivateBestChain()
151+
*/
152+
CCriticalSection m_cs_chainstate;
153+
148154
public:
149155
CChain chainActive;
150156
BlockMap mapBlockIndex;
@@ -2519,6 +2525,7 @@ void CChainState::PruneBlockIndexCandidates() {
25192525
bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace)
25202526
{
25212527
AssertLockHeld(cs_main);
2528+
25222529
const CBlockIndex *pindexOldTip = chainActive.Tip();
25232530
const CBlockIndex *pindexFork = chainActive.FindFork(pindexMostWork);
25242531

@@ -2635,6 +2642,12 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
26352642
// sanely for performance or correctness!
26362643
AssertLockNotHeld(cs_main);
26372644

2645+
// ABC maintains a fair degree of expensive-to-calculate internal state
2646+
// because this function periodically releases cs_main so that it does not lock up other threads for too long
2647+
// during large connects - and to allow for e.g. the callback queue to drain
2648+
// we use m_cs_chainstate to enforce mutual exclusion so that only one caller may execute this function at a time
2649+
LOCK(m_cs_chainstate);
2650+
26382651
CBlockIndex *pindexMostWork = nullptr;
26392652
CBlockIndex *pindexNewTip = nullptr;
26402653
int nStopAtHeight = gArgs.GetArg("-stopatheight", DEFAULT_STOPATHEIGHT);
@@ -2648,45 +2661,53 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
26482661
SyncWithValidationInterfaceQueue();
26492662
}
26502663

2651-
const CBlockIndex *pindexFork;
2652-
bool fInitialDownload;
26532664
{
26542665
LOCK(cs_main);
2655-
ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked
2666+
CBlockIndex* starting_tip = chainActive.Tip();
2667+
bool blocks_connected = false;
2668+
do {
2669+
// We absolutely may not unlock cs_main until we've made forward progress
2670+
// (with the exception of shutdown due to hardware issues, low disk space, etc).
2671+
ConnectTrace connectTrace(mempool); // Destructed before cs_main is unlocked
2672+
2673+
if (pindexMostWork == nullptr) {
2674+
pindexMostWork = FindMostWorkChain();
2675+
}
26562676

2657-
CBlockIndex *pindexOldTip = chainActive.Tip();
2658-
if (pindexMostWork == nullptr) {
2659-
pindexMostWork = FindMostWorkChain();
2660-
}
2677+
// Whether we have anything to do at all.
2678+
if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) {
2679+
break;
2680+
}
26612681

2662-
// Whether we have anything to do at all.
2663-
if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip())
2664-
return true;
2682+
bool fInvalidFound = false;
2683+
std::shared_ptr<const CBlock> nullBlockPtr;
2684+
if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace))
2685+
return false;
2686+
blocks_connected = true;
26652687

2666-
bool fInvalidFound = false;
2667-
std::shared_ptr<const CBlock> nullBlockPtr;
2668-
if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace))
2669-
return false;
2688+
if (fInvalidFound) {
2689+
// Wipe cache, we may need another branch now.
2690+
pindexMostWork = nullptr;
2691+
}
2692+
pindexNewTip = chainActive.Tip();
26702693

2671-
if (fInvalidFound) {
2672-
// Wipe cache, we may need another branch now.
2673-
pindexMostWork = nullptr;
2674-
}
2675-
pindexNewTip = chainActive.Tip();
2676-
pindexFork = chainActive.FindFork(pindexOldTip);
2677-
fInitialDownload = IsInitialBlockDownload();
2694+
for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2695+
assert(trace.pblock && trace.pindex);
2696+
GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs);
2697+
}
2698+
} while (!chainActive.Tip() || (starting_tip && CBlockIndexWorkComparator()(chainActive.Tip(), starting_tip)));
2699+
if (!blocks_connected) return true;
26782700

2679-
for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2680-
assert(trace.pblock && trace.pindex);
2681-
GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs);
2682-
}
2701+
const CBlockIndex* pindexFork = chainActive.FindFork(starting_tip);
2702+
bool fInitialDownload = IsInitialBlockDownload();
26832703

26842704
// Notify external listeners about the new tip.
26852705
// Enqueue while holding cs_main to ensure that UpdatedBlockTip is called in the order in which blocks are connected
2686-
GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
2687-
2688-
// Always notify the UI if a new block tip was connected
26892706
if (pindexFork != pindexNewTip) {
2707+
// Notify ValidationInterface subscribers
2708+
GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload);
2709+
2710+
// Always notify the UI if a new block tip was connected
26902711
uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip);
26912712
}
26922713
}
@@ -2710,6 +2731,7 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams&
27102731

27112732
return true;
27122733
}
2734+
27132735
bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock) {
27142736
return g_chainstate.ActivateBestChain(state, chainparams, std::move(pblock));
27152737
}

src/validationinterface.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,11 @@ class CValidationInterface {
6161
*/
6262
~CValidationInterface() = default;
6363
/**
64-
* Notifies listeners of updated block chain tip
64+
* Notifies listeners when the block chain tip advances.
65+
*
66+
* When multiple blocks are connected at once, UpdatedBlockTip will be called on the final tip
67+
* but may not be called on every intermediate tip. If the latter behavior is desired,
68+
* subscribe to BlockConnected() instead.
6569
*
6670
* Called on a background thread.
6771
*/

0 commit comments

Comments
 (0)