From ecc3c4a019e6db30e208b8554b1a3658dcb9a80a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 9 Oct 2017 11:19:10 -0400 Subject: Do not unlock cs_main in ABC unless we've actually made progress. Technically, some internal datastructures may be in an inconsistent state if we do this, though there are no known bugs there. Still, for future safety, its much better to only unlock cs_main if we've made progress (not just tried a reorg which may make progress). --- src/validation.cpp | 65 +++++++++++++++++++++++++++-------------------- src/validationinterface.h | 6 ++++- 2 files changed, 42 insertions(+), 29 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index daa33d3f5a..477b3e1550 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2666,45 +2666,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 + + if (pindexMostWork == nullptr) { + pindexMostWork = FindMostWorkChain(); + } - CBlockIndex *pindexOldTip = chainActive.Tip(); - if (pindexMostWork == nullptr) { - pindexMostWork = FindMostWorkChain(); - } + // Whether we have anything to do at all. + if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) { + break; + } - // Whether we have anything to do at all. - if (pindexMostWork == nullptr || pindexMostWork == chainActive.Tip()) - return true; + bool fInvalidFound = false; + std::shared_ptr nullBlockPtr; + if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) + return false; + blocks_connected = true; - bool fInvalidFound = false; - std::shared_ptr nullBlockPtr; - if (!ActivateBestChainStep(state, chainparams, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fInvalidFound, connectTrace)) - return false; + if (fInvalidFound) { + // Wipe cache, we may need another branch now. + pindexMostWork = nullptr; + } + pindexNewTip = chainActive.Tip(); - if (fInvalidFound) { - // Wipe cache, we may need another branch now. - pindexMostWork = nullptr; - } - pindexNewTip = chainActive.Tip(); - pindexFork = chainActive.FindFork(pindexOldTip); - fInitialDownload = IsInitialBlockDownload(); + 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; - for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) { - assert(trace.pblock && trace.pindex); - GetMainSignals().BlockConnected(trace.pblock, trace.pindex, trace.conflictedTxs); - } + 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); } } @@ -2728,6 +2736,7 @@ bool CChainState::ActivateBestChain(CValidationState &state, const CChainParams& return true; } + bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr pblock) { return g_chainstate.ActivateBestChain(state, chainparams, std::move(pblock)); } diff --git a/src/validationinterface.h b/src/validationinterface.h index 63097166af..6bf0f9c39f 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -61,7 +61,11 @@ protected: */ ~CValidationInterface() = default; /** - * Notifies listeners of updated block chain tip + * Notifies listeners when the block chain tip advances. + * + * When multiple blocks are connected at once, UpdatedBlockTip will be called on the final tip + * but may not be called on every intermediate tip. If the latter behavior is desired, + * subscribe to BlockConnected() instead. * * Called on a background thread. */ -- cgit v1.2.3 From a3ae8e68739023e5dba9e5cb190e707ed4603316 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Thu, 10 May 2018 15:57:16 -0400 Subject: Fix concurrency-related bugs in ActivateBestChain If multiple threads are invoking ActivateBestChain, it was possible to have them working towards different tips, and we could arrive at a less work tip than we should. Fix this by introducing a ChainState lock which must be held for the entire duration of ActivateBestChain to enforce exclusion in ABC. --- src/validation.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/validation.cpp b/src/validation.cpp index 477b3e1550..405f21baa3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -144,6 +144,12 @@ private: */ std::set g_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; @@ -2542,6 +2548,7 @@ void CChainState::PruneBlockIndexCandidates() { bool CChainState::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(); const CBlockIndex *pindexFork = chainActive.FindFork(pindexMostWork); @@ -2653,6 +2660,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); -- cgit v1.2.3 From dd435ad40267f5c50ff17533c696f9302829a6a6 Mon Sep 17 00:00:00 2001 From: Jesse Cohen Date: Wed, 18 Apr 2018 08:01:48 -0400 Subject: Add unit tests for signals generated by ProcessNewBlock() After a recent bug discovered in callback ordering in MainSignals, this test checks invariants in ordering of BlockConnected / BlockDisconnected / UpdatedChainTip signals --- src/Makefile.test.include | 5 +- src/test/test_bitcoin.cpp | 6 ++ src/test/test_bitcoin.h | 3 + src/test/validation_block_tests.cpp | 184 ++++++++++++++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 2 deletions(-) create mode 100644 src/test/validation_block_tests.cpp diff --git a/src/Makefile.test.include b/src/Makefile.test.include index c4f18bb371..efdb39b648 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -85,9 +85,10 @@ BITCOIN_TESTS =\ test/transaction_tests.cpp \ test/txvalidation_tests.cpp \ test/txvalidationcache_tests.cpp \ - test/versionbits_tests.cpp \ test/uint256_tests.cpp \ - test/util_tests.cpp + test/util_tests.cpp \ + test/validation_block_tests.cpp \ + test/versionbits_tests.cpp if ENABLE_WALLET BITCOIN_TESTS += \ diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index b72df1604f..77d43890e4 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -38,6 +38,12 @@ FastRandomContext insecure_rand_ctx(insecure_rand_seed); extern bool fPrintToConsole; extern void noui_connect(); +std::ostream& operator<<(std::ostream& os, const uint256& num) +{ + os << num.ToString(); + return os; +} + BasicTestingSetup::BasicTestingSetup(const std::string& chainName) { SHA256AutoDetect(); diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h index 1f91eb622c..d013613de2 100644 --- a/src/test/test_bitcoin.h +++ b/src/test/test_bitcoin.h @@ -120,4 +120,7 @@ struct TestMemPoolEntryHelper CBlock getBlock13b8a(); +// define an implicit conversion here so that uint256 may be used directly in BOOST_CHECK_* +std::ostream& operator<<(std::ostream& os, const uint256& num); + #endif diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp new file mode 100644 index 0000000000..f835570168 --- /dev/null +++ b/src/test/validation_block_tests.cpp @@ -0,0 +1,184 @@ +// Copyright (c) 2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct RegtestingSetup : public TestingSetup { + RegtestingSetup() : TestingSetup(CBaseChainParams::REGTEST) {} +}; + +BOOST_FIXTURE_TEST_SUITE(validation_block_tests, RegtestingSetup) + +struct TestSubscriber : public CValidationInterface { + uint256 m_expected_tip; + + TestSubscriber(uint256 tip) : m_expected_tip(tip) {} + + void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) + { + BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash()); + } + + void BlockConnected(const std::shared_ptr& block, const CBlockIndex* pindex, const std::vector& txnConflicted) + { + BOOST_CHECK_EQUAL(m_expected_tip, block->hashPrevBlock); + BOOST_CHECK_EQUAL(m_expected_tip, pindex->pprev->GetBlockHash()); + + m_expected_tip = block->GetHash(); + } + + void BlockDisconnected(const std::shared_ptr& block) + { + BOOST_CHECK_EQUAL(m_expected_tip, block->GetHash()); + + m_expected_tip = block->hashPrevBlock; + } +}; + +std::shared_ptr Block(const uint256& prev_hash) +{ + static int i = 0; + static uint64_t time = Params().GenesisBlock().nTime; + + CScript pubKey; + pubKey << i++ << OP_TRUE; + + auto ptemplate = BlockAssembler(Params()).CreateNewBlock(pubKey, false); + auto pblock = std::make_shared(ptemplate->block); + pblock->hashPrevBlock = prev_hash; + pblock->nTime = ++time; + + CMutableTransaction txCoinbase(*pblock->vtx[0]); + txCoinbase.vout.resize(1); + txCoinbase.vin[0].scriptWitness.SetNull(); + pblock->vtx[0] = MakeTransactionRef(std::move(txCoinbase)); + + return pblock; +} + +std::shared_ptr FinalizeBlock(std::shared_ptr pblock) +{ + pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); + + while (!CheckProofOfWork(pblock->GetHash(), pblock->nBits, Params().GetConsensus())) { + ++(pblock->nNonce); + } + + return pblock; +} + +// construct a valid block +const std::shared_ptr GoodBlock(const uint256& prev_hash) +{ + return FinalizeBlock(Block(prev_hash)); +} + +// construct an invalid block (but with a valid header) +const std::shared_ptr BadBlock(const uint256& prev_hash) +{ + auto pblock = Block(prev_hash); + + CMutableTransaction coinbase_spend; + coinbase_spend.vin.push_back(CTxIn(COutPoint(pblock->vtx[0]->GetHash(), 0), CScript(), 0)); + coinbase_spend.vout.push_back(pblock->vtx[0]->vout[0]); + + CTransactionRef tx = MakeTransactionRef(coinbase_spend); + pblock->vtx.push_back(tx); + + auto ret = FinalizeBlock(pblock); + return ret; +} + +void BuildChain(const uint256& root, int height, const unsigned int invalid_rate, const unsigned int branch_rate, const unsigned int max_size, std::vector>& blocks) +{ + if (height <= 0 || blocks.size() >= max_size) return; + + bool gen_invalid = GetRand(100) < invalid_rate; + bool gen_fork = GetRand(100) < branch_rate; + + const std::shared_ptr pblock = gen_invalid ? BadBlock(root) : GoodBlock(root); + blocks.push_back(pblock); + if (!gen_invalid) { + BuildChain(pblock->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks); + } + + if (gen_fork) { + blocks.push_back(GoodBlock(root)); + BuildChain(blocks.back()->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks); + } +} + +BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering) +{ + // build a large-ish chain that's likely to have some forks + std::vector> blocks; + while (blocks.size() < 50) { + blocks.clear(); + BuildChain(Params().GenesisBlock().GetHash(), 100, 15, 10, 500, blocks); + } + + bool ignored; + CValidationState state; + std::vector headers; + std::transform(blocks.begin(), blocks.end(), std::back_inserter(headers), [](std::shared_ptr b) { return b->GetBlockHeader(); }); + + // Process all the headers so we understand the toplogy of the chain + BOOST_CHECK(ProcessNewBlockHeaders(headers, state, Params())); + + // Connect the genesis block and drain any outstanding events + ProcessNewBlock(Params(), std::make_shared(Params().GenesisBlock()), true, &ignored); + SyncWithValidationInterfaceQueue(); + + // subscribe to events (this subscriber will validate event ordering) + const CBlockIndex* initial_tip = nullptr; + { + LOCK(cs_main); + initial_tip = chainActive.Tip(); + } + TestSubscriber sub(initial_tip->GetBlockHash()); + RegisterValidationInterface(&sub); + + // create a bunch of threads that repeatedly process a block generated above at random + // this will create parallelism and randomness inside validation - the ValidationInterface + // will subscribe to events generated during block validation and assert on ordering invariance + boost::thread_group threads; + for (int i = 0; i < 10; i++) { + threads.create_thread([&blocks]() { + bool ignored; + for (int i = 0; i < 1000; i++) { + auto block = blocks[GetRand(blocks.size() - 1)]; + ProcessNewBlock(Params(), block, true, &ignored); + } + + // to make sure that eventually we process the full chain - do it here + for (auto block : blocks) { + if (block->vtx.size() == 1) { + bool processed = ProcessNewBlock(Params(), block, true, &ignored); + assert(processed); + } + } + }); + } + + threads.join_all(); + while (GetMainSignals().CallbacksPending() > 0) { + MilliSleep(100); + } + + UnregisterValidationInterface(&sub); + + BOOST_CHECK_EQUAL(sub.m_expected_tip, chainActive.Tip()->GetBlockHash()); +} + +BOOST_AUTO_TEST_SUITE_END() -- cgit v1.2.3