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 https://github.com/bitcoin/bitcoin/pull/13023/commits/6094ce73045fe0b4654ff94327c2059512af88fb will occasionally fail without the commit fixing this issue https://github.com/bitcoin/bitcoin/pull/13023/commits/26bfdbaddbb9f13864deb7241c6d513f22c5ab62

  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

6 files changed, 251 insertions, 31 deletions

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 91d3a3d47c..7174b3e8df 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -86,9 +86,10 @@ BITCOIN_TESTS =\
   test/txindex_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 9857827f05..e9814edc23 100644
--- a/src/test/test_bitcoin.cpp
+++ b/src/test/test_bitcoin.cpp
@@ -39,6 +39,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 <boost/test/unit_test.hpp>
+
+#include <chainparams.h>
+#include <consensus/merkle.h>
+#include <consensus/validation.h>
+#include <miner.h>
+#include <pow.h>
+#include <random.h>
+#include <test/test_bitcoin.h>
+#include <validation.h>
+#include <validationinterface.h>
+
+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<const CBlock>& block, const CBlockIndex* pindex, const std::vector<CTransactionRef>& 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<const CBlock>& block)
+    {
+        BOOST_CHECK_EQUAL(m_expected_tip, block->GetHash());
+
+        m_expected_tip = block->hashPrevBlock;
+    }
+};
+
+std::shared_ptr<CBlock> 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<CBlock>(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<CBlock> FinalizeBlock(std::shared_ptr<CBlock> 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<const CBlock> GoodBlock(const uint256& prev_hash)
+{
+    return FinalizeBlock(Block(prev_hash));
+}
+
+// construct an invalid block (but with a valid header)
+const std::shared_ptr<const CBlock> 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<std::shared_ptr<const CBlock>>& 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<const CBlock> 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<std::shared_ptr<const CBlock>> blocks;
+    while (blocks.size() < 50) {
+        blocks.clear();
+        BuildChain(Params().GenesisBlock().GetHash(), 100, 15, 10, 500, blocks);
+    }
+
+    bool ignored;
+    CValidationState state;
+    std::vector<CBlockHeader> headers;
+    std::transform(blocks.begin(), blocks.end(), std::back_inserter(headers), [](std::shared_ptr<const CBlock> 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<CBlock>(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()
diff --git a/src/validation.cpp b/src/validation.cpp
index f22ae7f9be..3971906424 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -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
+
+                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<const CBlock> 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<const CBlock> 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);
             }
         }
@@ -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));
 }
diff --git a/src/validationinterface.h b/src/validationinterface.h
index 3a5fed0106..0ca82235da 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.
      */