diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 7f0c51f29148..d4f6a77eb346 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -101,7 +101,8 @@ BITCOIN_TESTS =\ test/univalue_tests.cpp \ test/util_tests.cpp \ test/sha256compress_tests.cpp \ - test/upgrades_tests.cpp + test/upgrades_tests.cpp \ + test/validation_block_tests.cpp SAPLING_TESTS =\ test/librust/libsapling_utils_tests.cpp \ diff --git a/src/blockassembler.h b/src/blockassembler.h index eaf183dec6a0..753b10048201 100644 --- a/src/blockassembler.h +++ b/src/blockassembler.h @@ -66,8 +66,8 @@ class BlockAssembler BlockAssembler(const CChainParams& chainparams, const bool defaultPrintPriority); /** Construct a new block template with coinbase to scriptPubKeyIn */ std::unique_ptr CreateNewBlock(const CScript& scriptPubKeyIn, - CWallet* pwallet, - bool fProofOfStake, + CWallet* pwallet = nullptr, + bool fProofOfStake = false, std::vector* availableCoins = nullptr); private: diff --git a/src/scheduler.h b/src/scheduler.h index 781bd52ea56f..6661c032c82f 100644 --- a/src/scheduler.h +++ b/src/scheduler.h @@ -87,9 +87,13 @@ class CScheduler /** * Class used by CScheduler clients which may schedule multiple jobs - * which are required to be run serially. Does not require such jobs - * to be executed on the same thread, but no two jobs will be executed - * at the same time. + * which are required to be run serially. Jobs may not be run on the + * same thread, but no two jobs will be executed + * at the same time and memory will be release-acquire consistent + * (the scheduler will internally do an acquire before invoking a callback + * as well as a release at the end). In practice this means that a callback + * B() will be able to observe all of the effects of callback A() which executed + * before it. */ class SingleThreadedSchedulerClient { private: @@ -104,6 +108,13 @@ class SingleThreadedSchedulerClient { public: explicit SingleThreadedSchedulerClient(CScheduler *pschedulerIn) : m_pscheduler(pschedulerIn) {} + + /** + * Add a callback to be executed. Callbacks are executed serially + * and memory is release-acquire consistent between callback executions. + * Practially, this means that callbacks can behave as if they are executed + * in order by a single thread. + */ void AddToProcessQueue(std::function func); // Processes all remaining queue members on the calling thread, blocking until queue is empty diff --git a/src/test/CMakeLists.txt b/src/test/CMakeLists.txt index 98d670fbc5e8..b80d39141f2a 100644 --- a/src/test/CMakeLists.txt +++ b/src/test/CMakeLists.txt @@ -133,6 +133,7 @@ set(BITCOIN_TESTS ${CMAKE_CURRENT_SOURCE_DIR}/validation_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/sha256compress_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/upgrades_tests.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/validation_block_tests.cpp ${CMAKE_CURRENT_SOURCE_DIR}/librust/sapling_rpc_wallet_tests.cpp ${CMAKE_SOURCE_DIR}/src/wallet/test/wallet_tests.cpp ${CMAKE_SOURCE_DIR}/src/wallet/test/crypto_tests.cpp diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp index ce46cbf5887f..3f3b0a660718 100644 --- a/src/test/scheduler_tests.cpp +++ b/src/test/scheduler_tests.cpp @@ -61,7 +61,7 @@ BOOST_AUTO_TEST_CASE(manythreads) size_t nTasks = microTasks.getQueueInfo(first, last); BOOST_CHECK(nTasks == 0); - for (int i = 0; i < 100; i++) { + for (int i = 0; i < 100; ++i) { boost::chrono::system_clock::time_point t = now + boost::chrono::microseconds(randomMsec(rng)); boost::chrono::system_clock::time_point tReschedule = now + boost::chrono::microseconds(500 + randomMsec(rng)); int whichCounter = zeroToNine(rng); @@ -108,4 +108,46 @@ BOOST_AUTO_TEST_CASE(manythreads) BOOST_CHECK_EQUAL(counterSum, 200); } +BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered) +{ + CScheduler scheduler; + + // each queue should be well ordered with respect to itself but not other queues + SingleThreadedSchedulerClient queue1(&scheduler); + SingleThreadedSchedulerClient queue2(&scheduler); + + // create more threads than queues + // if the queues only permit execution of one task at once then + // the extra threads should effectively be doing nothing + // if they don't we'll get out of order behaviour + boost::thread_group threads; + for (int i = 0; i < 5; ++i) { + threads.create_thread(boost::bind(&CScheduler::serviceQueue, &scheduler)); + } + + // these are not atomic, if SinglethreadedSchedulerClient prevents + // parallel execution at the queue level no synchronization should be required here + int counter1 = 0; + int counter2 = 0; + + // just simply count up on each queue - if execution is properly ordered then + // the callbacks should run in exactly the order in which they were enqueued + for (int i = 0; i < 100; ++i) { + queue1.AddToProcessQueue([i, &counter1]() { + BOOST_CHECK_EQUAL(i, counter1++); + }); + + queue2.AddToProcessQueue([i, &counter2]() { + BOOST_CHECK_EQUAL(i, counter2++); + }); + } + + // finish up + scheduler.stop(true); + threads.join_all(); + + BOOST_CHECK_EQUAL(counter1, 100); + BOOST_CHECK_EQUAL(counter2, 100); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/test_pivx.cpp b/src/test/test_pivx.cpp index 2d4b2c3b5423..adad49eb9192 100644 --- a/src/test/test_pivx.cpp +++ b/src/test/test_pivx.cpp @@ -32,6 +32,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() { RandomInit(); diff --git a/src/test/test_pivx.h b/src/test/test_pivx.h index d22da8ecd1bd..251c9ffb929d 100644 --- a/src/test/test_pivx.h +++ b/src/test/test_pivx.h @@ -117,4 +117,7 @@ struct TestMemPoolEntryHelper TestMemPoolEntryHelper &SigOps(unsigned int _sigops) { sigOpCount = _sigops; return *this; } }; +// 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 000000000000..dba3c04f1b63 --- /dev/null +++ b/src/test/validation_block_tests.cpp @@ -0,0 +1,178 @@ +// Copyright (c) 2018 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or https://www.opensource.org/licenses/mit-license.php. + +#include + +#include "blockassembler.h" +#include "chainparams.h" +#include "consensus/merkle.h" +#include "consensus/validation.h" +#include "pow.h" +#include "random.h" +#include "test/test_pivx.h" +#include "validation.h" +#include "validationinterface.h" + +struct RegtestingSetup : public TestingSetup { + RegtestingSetup() : TestingSetup() { + SelectParams(CBaseChainParams::REGTEST); + } +}; + +BOOST_FIXTURE_TEST_SUITE(validation_block_tests, RegtestingSetup) + +struct TestSubscriber : public CValidationInterface { + uint256 m_expected_tip; + + TestSubscriber(uint256 tip) : m_expected_tip(std::move(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, const uint256& blockHash, int nBlockHeight, int64_t blockTime) + { + 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(), false).CreateNewBlock(pubKey); + auto pblock = std::make_shared(ptemplate->block); + pblock->hashPrevBlock = prev_hash; + pblock->nTime = ++time; + + CMutableTransaction txCoinbase(*pblock->vtx[0]); + txCoinbase.vout.resize(1); + 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)) { + ++(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.emplace_back(CTxIn(COutPoint(pblock->vtx[0]->GetHash(), 0), CScript(), 0)); + coinbase_spend.vout.emplace_back(pblock->vtx[0]->vout[0]); + + CTransactionRef tx = MakeTransactionRef(coinbase_spend); + pblock->vtx.emplace_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.emplace_back(pblock); + if (!gen_invalid) { + BuildChain(pblock->GetHash(), height - 1, invalid_rate, branch_rate, max_size, blocks); + } + + if (gen_fork) { + blocks.emplace_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); + } + + CValidationState state; + // Connect the genesis block and drain any outstanding events + BOOST_CHECK_MESSAGE(ProcessNewBlock(state, nullptr, std::make_shared(Params().GenesisBlock()), nullptr), "Error: genesis not connected"); + SyncWithValidationInterfaceQueue(); + + // subscribe to events (this subscriber will validate event ordering) + const CBlockIndex* initial_tip = WITH_LOCK(cs_main, return 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]() { + CValidationState state; + for (int i = 0; i < 1000; i++) { + auto block = blocks[GetRand(blocks.size() - 1)]; + ProcessNewBlock(state, nullptr, block, nullptr); + } + + // to make sure that eventually we process the full chain - do it here + for (const auto& block : blocks) { + if (block->vtx.size() == 1) { + bool processed = ProcessNewBlock(state, nullptr, block, nullptr); + // Future to do: "prevblk-not-found" here is the only valid reason to not check processed flag. + if (state.GetRejectReason() == "duplicate" || + state.GetRejectReason() == "prevblk-not-found" || + state.GetRejectReason() == "bad-prevblk") continue; + BOOST_ASSERT_MSG(processed, ("Error: " + state.GetRejectReason()).c_str()); + } + } + }); + } + + threads.join_all(); + while (GetMainSignals().CallbacksPending() > 0) { + MilliSleep(100); + } + + UnregisterValidationInterface(&sub); + + BOOST_CHECK_EQUAL(sub.m_expected_tip, WITH_LOCK(cs_main, return chainActive.Tip()->GetBlockHash())); +} + +BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index c9e6380fa884..59ff2cc0d127 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -147,11 +147,17 @@ struct CBlockIndexWorkComparator { CBlockIndex* pindexBestInvalid; /** - * The set of all CBlockIndex entries with BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) and - * as good as our current tip or better. Entries may be failed, though. - */ + * The set of all CBlockIndex entries with BLOCK_VALID_TRANSACTIONS (for itself and all ancestors) and + * as good as our current tip or better. Entries may be failed, though. + */ std::set setBlockIndexCandidates; +/** + * the ChainState Mutex + * A lock that must be held when modifying this ChainState - held in ActivateBestChain() + */ +Mutex m_cs_chainstate; + /** All pairs A->B, where A (or one if its ancestors) misses transactions, but B has transactions. */ std::multimap mapBlocksUnlinked; @@ -2138,12 +2144,11 @@ static void PruneBlockIndexCandidates() * Try to make some progress towards making pindexMostWork the active block. * pblock is either NULL or a pointer to a CBlock corresponding to pindexMostWork. */ -static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool fAlreadyChecked, ConnectTrace& connectTrace) +static bool ActivateBestChainStep(CValidationState& state, CBlockIndex* pindexMostWork, const std::shared_ptr& pblock, bool fAlreadyChecked, bool& fInvalidFound, ConnectTrace& connectTrace) { AssertLockHeld(cs_main); if (pblock == NULL) fAlreadyChecked = false; - bool fInvalidFound = false; const CBlockIndex* pindexOldTip = chainActive.Tip(); const CBlockIndex* pindexFork = chainActive.FindFork(pindexMostWork); @@ -2227,6 +2232,12 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb // 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* pindexNewTip = nullptr; CBlockIndex* pindexMostWork = nullptr; do { @@ -2239,42 +2250,64 @@ bool ActivateBestChain(CValidationState& state, std::shared_ptr pb 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(); - 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 == NULL || pindexMostWork == chainActive.Tip()) - return true; + bool fInvalidFound = false; + std::shared_ptr nullBlockPtr; + if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, fInvalidFound, connectTrace)) + return false; + blocks_connected = true; - std::shared_ptr nullBlockPtr; - if (!ActivateBestChainStep(state, pindexMostWork, pblock && pblock->GetHash() == pindexMostWork->GetBlockHash() ? pblock : nullBlockPtr, fAlreadyChecked, connectTrace)) - return false; + if (fInvalidFound) { + // Wipe cache, we may need another branch now. + pindexMostWork = nullptr; + } + pindexNewTip = chainActive.Tip(); - 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. - GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); + // 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 + if (pindexFork != pindexNewTip) { + // Notify ValidationInterface subscribers + GetMainSignals().UpdatedBlockTip(pindexNewTip, pindexFork, fInitialDownload); - // Always notify the UI if a new block tip was connected - if (pindexFork != pindexNewTip) { - // Notify the UI - uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); + // Always notify the UI if a new block tip was connected + uiInterface.NotifyBlockTip(fInitialDownload, pindexNewTip); + } } + // When we reach this point, we switched to a new tip (stored in pindexNewTip). + // We check shutdown only after giving ActivateBestChainStep a chance to run once so that we + // never shutdown before connecting the genesis block during LoadChainTip(). Previously this + // caused an assert() failure during shutdown in such cases as the UTXO DB flushing checks + // that the best block hash is non-null. + if (ShutdownRequested()) + break; } while (pindexMostWork != chainActive.Tip()); CheckBlockIndex(); @@ -2916,8 +2949,10 @@ bool GetPrevIndex(const CBlock& block, CBlockIndex** pindexPrevRet, CValidationS pindexPrev = nullptr; if (block.GetHash() != Params().GetConsensus().hashGenesisBlock) { BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock); - if (mi == mapBlockIndex.end()) - return state.DoS(0, error("%s : prev block %s not found", __func__, block.hashPrevBlock.GetHex()), 0, "bad-prevblk"); + if (mi == mapBlockIndex.end()) { + return state.DoS(0, error("%s : prev block %s not found", __func__, block.hashPrevBlock.GetHex()), 0, + "prevblk-not-found"); + } pindexPrev = (*mi).second; if (pindexPrev->nStatus & BLOCK_FAILED_MASK) { //If this "invalid" block is an exact match from the checkpoints, then reconsider it diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index ab2b92527642..76a5ff27be39 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -146,6 +146,10 @@ void CMainSignals::MempoolEntryRemoved(CTransactionRef ptx, MemPoolRemovalReason } void CMainSignals::UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) { + // Dependencies exist that require UpdatedBlockTip events to be delivered in the order in which + // the chain actually updates. One way to ensure this is for the caller to invoke this signal + // in the same critical section where the chain is updated + m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] { m_internals->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); }); diff --git a/src/validationinterface.h b/src/validationinterface.h index 27bfd9d4ce17..91ec2ee5b46c 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -54,12 +54,31 @@ void CallFunctionInValidationInterfaceQueue(std::function func); */ void SyncWithValidationInterfaceQueue(); +/** + * Implement this to subscribe to events generated in validation + * + * Each CValidationInterface() subscriber will receive event callbacks + * in the order in which the events were generated by validation. + * Furthermore, each ValidationInterface() subscriber may assume that + * callbacks effectively run in a single thread with single-threaded + * memory consistency. That is, for a given ValidationInterface() + * instantiation, each callback will complete before the next one is + * invoked. This means, for example when a block is connected that the + * UpdatedBlockTip() callback may depend on an operation performed in + * the BlockConnected() callback without worrying about explicit + * synchronization. No ordering should be assumed across + * ValidationInterface() subscribers. + */ class CValidationInterface { public: virtual ~CValidationInterface() = default; protected: /** - * 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. */