Merge bitcoin/bitcoin#21526: validation: UpdateTip/CheckBlockIndex assumeutxo support

673a5bd3377929a0a6a62eda8b560e47bc2cca0c test: validation: add unittest for UpdateTip behavior (James O'Beirne)
2705570109a2a90ecfd3f4180944498626fc2707 test: refactor: separate CreateBlock in TestChain100Setup (James O'Beirne)
298bf5d563cc740c6ae71750d86942e0278b22d6 test: refactor: declare NoMalleation const auto (James O'Beirne)
071200993f3a9412821ce5387851d659baf85327 move-only: unittest: add test/util/chainstate.h (James O'Beirne)
8f5710fd0ac5173b577e5d00708485170b321bcc validation: fix CheckBlockIndex for multiple chainstates (James O'Beirne)
5a807736dacfc3e6fa57231219336acf08be38fb validation: insert assumed-valid block index entries into candidates (James O'Beirne)
01a9b8fe719efab2c268dc738bc93cfbdf92edb7 validation: set BLOCK_ASSUMED_VALID during snapshot load (James O'Beirne)
42b2520db93fd9feb3df4101654391fa7d3e2140 chain: add BLOCK_ASSUMED_VALID for use with assumeutxo (James O'Beirne)
b217020df78bc981d221fe04497c831120ef969f validation: change UpdateTip for multiple chainstates (James O'Beirne)
665072a36df2e4c88705fedd4ac7c955d7f6a488 doc: add comment for g_best_block (James O'Beirne)
ac4051d891e2d5c8ac130da16b85b9d880b44720 refactor: remove unused assumeutxo methods (James O'Beirne)
9f6bb539359b98d5b39482ab8a28a68608f0c645 validation: add chainman ref to CChainState (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)

  ---

  Modify UpdateTip and CheckBlockIndex for use with multiple chainstates. Includes a new unittest verifying `g_best_block` behavior (previously untested at the unit level) and various changes necessary for running and testing `ProcessNewBlock()`-like behavior on the background validation chainstate.

  This changeset introduces a new block index `nStatus` flag called `BLOCK_ASSUMED_VALID`, and it is applied to block index entries that are beneath the UTXO snapshot base block upon snapshot load. Once each block is validated (during async background validation), the flag is removed. This allows us to avoid (ab)using `BLOCK_VALID_*` flags for snapshot chain block entries, and preserves the original meaning of those flags.

  Note: this PR previously incorporated changes to `LoadBlockIndex()` and `RewindBlockIndex()` as noted in Russ' comments below, but once I generated the changes necessary to test the UpdateTip change, I decided to split this changes out into another PR due to the size of this one.

ACKs for top commit:
  achow101:
    ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
  jonatack:
    Code-review re-ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c reviewed diff, rebased to master/debug build/ran unit+functional tests
  naumenkogs:
    ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
  fjahr:
    Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c
  ariard:
    utACK 673a5bd3
  ryanofsky:
    Code review ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c. Just linker fix and split commit changes mentioned https://github.com/bitcoin/bitcoin/pull/21526#issuecomment-921064563 since last review
  benthecarman:
    ACK 673a5bd3377929a0a6a62eda8b560e47bc2cca0c

Tree-SHA512: 0a6dc23d041b27ed9fd0ee1f3e5971b92fb1d2df2fc9b655d5dc48594235321ab1798d06de2ec55482ac3966a9ed56de8d56e9e29cae75bbe8690bafc2dda383

6 files changed, 174 insertions, 60 deletions

diff --git a/src/test/util/chainstate.h b/src/test/util/chainstate.h
new file mode 100644
index 0000000000..81ea4c38f5
--- /dev/null
+++ b/src/test/util/chainstate.h
@@ -0,0 +1,54 @@
+// Copyright (c) 2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+//
+#ifndef BITCOIN_TEST_UTIL_CHAINSTATE_H
+#define BITCOIN_TEST_UTIL_CHAINSTATE_H
+
+#include <clientversion.h>
+#include <fs.h>
+#include <node/context.h>
+#include <node/utxo_snapshot.h>
+#include <rpc/blockchain.h>
+#include <validation.h>
+
+#include <univalue.h>
+
+#include <boost/test/unit_test.hpp>
+
+const auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){};
+
+/**
+ * Create and activate a UTXO snapshot, optionally providing a function to
+ * malleate the snapshot.
+ */
+template<typename F = decltype(NoMalleation)>
+static bool
+CreateAndActivateUTXOSnapshot(NodeContext& node, const fs::path root, F malleation = NoMalleation)
+{
+    // Write out a snapshot to the test's tempdir.
+    //
+    int height;
+    WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight());
+    fs::path snapshot_path = root / tfm::format("test_snapshot.%d.dat", height);
+    FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
+    CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION};
+
+    UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), auto_outfile);
+    BOOST_TEST_MESSAGE(
+        "Wrote UTXO snapshot to " << snapshot_path.make_preferred().string() << ": " << result.write());
+
+    // Read the written snapshot in and then activate it.
+    //
+    FILE* infile{fsbridge::fopen(snapshot_path, "rb")};
+    CAutoFile auto_infile{infile, SER_DISK, CLIENT_VERSION};
+    SnapshotMetadata metadata;
+    auto_infile >> metadata;
+
+    malleation(auto_infile, metadata);
+
+    return node.chainman->ActivateSnapshot(auto_infile, metadata, /*in_memory*/ true);
+}
+
+
+#endif // BITCOIN_TEST_UTIL_CHAINSTATE_H
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index cabc4b3b49..df4f003227 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -234,11 +234,14 @@ void TestChain100Setup::mineBlocks(int num_blocks)
     }
 }
 
-CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns, const CScript& scriptPubKey)
+CBlock TestChain100Setup::CreateBlock(
+    const std::vector<CMutableTransaction>& txns,
+    const CScript& scriptPubKey,
+    CChainState& chainstate)
 {
     const CChainParams& chainparams = Params();
     CTxMemPool empty_pool;
-    CBlock block = BlockAssembler(m_node.chainman->ActiveChainstate(), empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
+    CBlock block = BlockAssembler(chainstate, empty_pool, chainparams).CreateNewBlock(scriptPubKey)->block;
 
     Assert(block.vtx.size() == 1);
     for (const CMutableTransaction& tx : txns) {
@@ -248,6 +251,20 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
 
     while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
 
+    return block;
+}
+
+CBlock TestChain100Setup::CreateAndProcessBlock(
+    const std::vector<CMutableTransaction>& txns,
+    const CScript& scriptPubKey,
+    CChainState* chainstate)
+{
+    if (!chainstate) {
+        chainstate = &Assert(m_node.chainman)->ActiveChainstate();
+    }
+
+    const CChainParams& chainparams = Params();
+    const CBlock block = this->CreateBlock(txns, scriptPubKey, *chainstate);
     std::shared_ptr<const CBlock> shared_pblock = std::make_shared<const CBlock>(block);
     Assert(m_node.chainman)->ProcessNewBlock(chainparams, shared_pblock, true, nullptr);
 
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 5d12dc2323..acb96e54df 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -119,9 +119,20 @@ struct TestChain100Setup : public RegTestingSetup {
     /**
      * Create a new block with just given transactions, coinbase paying to
      * scriptPubKey, and try to add it to the current chain.
+     * If no chainstate is specified, default to the active.
      */
     CBlock CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns,
-                                 const CScript& scriptPubKey);
+                                 const CScript& scriptPubKey,
+                                 CChainState* chainstate = nullptr);
+
+    /**
+     * Create a new block with just given transactions, coinbase paying to
+     * scriptPubKey.
+     */
+    CBlock CreateBlock(
+        const std::vector<CMutableTransaction>& txns,
+        const CScript& scriptPubKey,
+        CChainState& chainstate);
 
     //! Mine a series of new blocks on the active chain.
     void mineBlocks(int num_blocks);
diff --git a/src/test/validation_chainstate_tests.cpp b/src/test/validation_chainstate_tests.cpp
index 315ef22599..726c9ebbb8 100644
--- a/src/test/validation_chainstate_tests.cpp
+++ b/src/test/validation_chainstate_tests.cpp
@@ -2,10 +2,13 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 //
+#include <chainparams.h>
 #include <random.h>
 #include <uint256.h>
 #include <consensus/validation.h>
 #include <sync.h>
+#include <rpc/blockchain.h>
+#include <test/util/chainstate.h>
 #include <test/util/setup_common.h>
 #include <validation.h>
 
@@ -73,4 +76,77 @@ BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
     WITH_LOCK(::cs_main, manager.Unload());
 }
 
+//! Test UpdateTip behavior for both active and background chainstates.
+//!
+//! When run on the background chainstate, UpdateTip should do a subset
+//! of what it does for the active chainstate.
+BOOST_FIXTURE_TEST_CASE(chainstate_update_tip, TestChain100Setup)
+{
+    ChainstateManager& chainman = *Assert(m_node.chainman);
+    uint256 curr_tip = ::g_best_block;
+
+    // Mine 10 more blocks, putting at us height 110 where a valid assumeutxo value can
+    // be found.
+    mineBlocks(10);
+
+    // After adding some blocks to the tip, best block should have changed.
+    BOOST_CHECK(::g_best_block != curr_tip);
+
+    BOOST_REQUIRE(CreateAndActivateUTXOSnapshot(m_node, m_path_root));
+
+    // Ensure our active chain is the snapshot chainstate.
+    BOOST_CHECK(chainman.IsSnapshotActive());
+
+    curr_tip = ::g_best_block;
+
+    // Mine a new block on top of the activated snapshot chainstate.
+    mineBlocks(1);  // Defined in TestChain100Setup.
+
+    // After adding some blocks to the snapshot tip, best block should have changed.
+    BOOST_CHECK(::g_best_block != curr_tip);
+
+    curr_tip = ::g_best_block;
+
+    CChainState* background_cs;
+
+    BOOST_CHECK_EQUAL(chainman.GetAll().size(), 2);
+    for (CChainState* cs : chainman.GetAll()) {
+        if (cs != &chainman.ActiveChainstate()) {
+            background_cs = cs;
+        }
+    }
+    BOOST_CHECK(background_cs);
+
+    // Create a block to append to the validation chain.
+    std::vector<CMutableTransaction> noTxns;
+    CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
+    CBlock validation_block = this->CreateBlock(noTxns, scriptPubKey, *background_cs);
+    auto pblock = std::make_shared<const CBlock>(validation_block);
+    BlockValidationState state;
+    CBlockIndex* pindex = nullptr;
+    const CChainParams& chainparams = Params();
+    bool newblock = false;
+
+    // TODO: much of this is inlined from ProcessNewBlock(); just reuse PNB()
+    // once it is changed to support multiple chainstates.
+    {
+        LOCK(::cs_main);
+        bool checked = CheckBlock(*pblock, state, chainparams.GetConsensus());
+        BOOST_CHECK(checked);
+        bool accepted = background_cs->AcceptBlock(
+            pblock, state, &pindex, true, nullptr, &newblock);
+        BOOST_CHECK(accepted);
+    }
+    // UpdateTip is called here
+    bool block_added = background_cs->ActivateBestChain(state, pblock);
+
+    // Ensure tip is as expected
+    BOOST_CHECK_EQUAL(background_cs->m_chain.Tip()->GetBlockHash(), validation_block.GetHash());
+
+    // g_best_block should be unchanged after adding a block to the background
+    // validation chain.
+    BOOST_CHECK(block_added);
+    BOOST_CHECK_EQUAL(curr_tip, ::g_best_block);
+}
+
 BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 0bd378631b..be9e05a65e 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@ -8,13 +8,13 @@
 #include <random.h>
 #include <rpc/blockchain.h>
 #include <sync.h>
+#include <test/util/chainstate.h>
 #include <test/util/setup_common.h>
 #include <uint256.h>
 #include <validation.h>
 #include <validationinterface.h>
 
 #include <tinyformat.h>
-#include <univalue.h>
 
 #include <vector>
 
@@ -44,7 +44,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 
     BOOST_CHECK(!manager.IsSnapshotActive());
     BOOST_CHECK(!manager.IsSnapshotValidated());
-    BOOST_CHECK(!manager.IsBackgroundIBD(&c1));
     auto all = manager.GetAll();
     BOOST_CHECK_EQUAL_COLLECTIONS(all.begin(), all.end(), chainstates.begin(), chainstates.end());
 
@@ -57,9 +56,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
     auto exp_tip = c1.m_chain.Tip();
     BOOST_CHECK_EQUAL(active_tip, exp_tip);
 
-    auto& validated_cs = manager.ValidatedChainstate();
-    BOOST_CHECK_EQUAL(&validated_cs, &c1);
-
     BOOST_CHECK(!manager.SnapshotBlockhash().has_value());
 
     // Create a snapshot-based chainstate.
@@ -81,8 +77,8 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 
     BOOST_CHECK(manager.IsSnapshotActive());
     BOOST_CHECK(!manager.IsSnapshotValidated());
-    BOOST_CHECK(manager.IsBackgroundIBD(&c1));
-    BOOST_CHECK(!manager.IsBackgroundIBD(&c2));
+    BOOST_CHECK_EQUAL(&c2, &manager.ActiveChainstate());
+    BOOST_CHECK(&c1 != &manager.ActiveChainstate());
     auto all2 = manager.GetAll();
     BOOST_CHECK_EQUAL_COLLECTIONS(all2.begin(), all2.end(), chainstates.begin(), chainstates.end());
 
@@ -99,16 +95,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
     // CCoinsViewCache instances.
     BOOST_CHECK(exp_tip != exp_tip2);
 
-    auto& validated_cs2 = manager.ValidatedChainstate();
-    BOOST_CHECK_EQUAL(&validated_cs2, &c1);
-
-    auto& validated_chain = manager.ValidatedChain();
-    BOOST_CHECK_EQUAL(&validated_chain, &c1.m_chain);
-
-    auto validated_tip = manager.ValidatedTip();
-    exp_tip = c1.m_chain.Tip();
-    BOOST_CHECK_EQUAL(validated_tip, exp_tip);
-
     // Let scheduler events finish running to avoid accessing memory that is going to be unloaded
     SyncWithValidationInterfaceQueue();
 
@@ -168,36 +154,6 @@ BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
     BOOST_CHECK_CLOSE(c2.m_coinsdb_cache_size_bytes, max_cache * 0.95, 1);
 }
 
-auto NoMalleation = [](CAutoFile& file, SnapshotMetadata& meta){};
-
-template<typename F = decltype(NoMalleation)>
-static bool
-CreateAndActivateUTXOSnapshot(NodeContext& node, const fs::path root, F malleation = NoMalleation)
-{
-    // Write out a snapshot to the test's tempdir.
-    //
-    int height;
-    WITH_LOCK(::cs_main, height = node.chainman->ActiveHeight());
-    fs::path snapshot_path = root / tfm::format("test_snapshot.%d.dat", height);
-    FILE* outfile{fsbridge::fopen(snapshot_path, "wb")};
-    CAutoFile auto_outfile{outfile, SER_DISK, CLIENT_VERSION};
-
-    UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), auto_outfile);
-    BOOST_TEST_MESSAGE(
-        "Wrote UTXO snapshot to " << snapshot_path.make_preferred().string() << ": " << result.write());
-
-    // Read the written snapshot in and then activate it.
-    //
-    FILE* infile{fsbridge::fopen(snapshot_path, "rb")};
-    CAutoFile auto_infile{infile, SER_DISK, CLIENT_VERSION};
-    SnapshotMetadata metadata;
-    auto_infile >> metadata;
-
-    malleation(auto_infile, metadata);
-
-    return node.chainman->ActivateSnapshot(auto_infile, metadata, /*in_memory*/ true);
-}
-
 //! Test basic snapshot activation.
 BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
 {
@@ -321,27 +277,27 @@ BOOST_FIXTURE_TEST_CASE(chainstatemanager_activate_snapshot, TestChain100Setup)
     {
         LOCK(::cs_main);
         size_t coins_in_active{0};
-        size_t coins_in_ibd{0};
-        size_t coins_missing_ibd{0};
+        size_t coins_in_background{0};
+        size_t coins_missing_from_background{0};
 
         for (CChainState* chainstate : chainman.GetAll()) {
             BOOST_TEST_MESSAGE("Checking coins in " << chainstate->ToString());
             CCoinsViewCache& coinscache = chainstate->CoinsTip();
-            bool is_ibd = chainman.IsBackgroundIBD(chainstate);
+            bool is_background = chainstate != &chainman.ActiveChainstate();
 
             for (CTransactionRef& txn : m_coinbase_txns) {
                 COutPoint op{txn->GetHash(), 0};
                 if (coinscache.HaveCoin(op)) {
-                    (is_ibd ? coins_in_ibd : coins_in_active)++;
-                } else if (is_ibd) {
-                    coins_missing_ibd++;
+                    (is_background ? coins_in_background : coins_in_active)++;
+                } else if (is_background) {
+                    coins_missing_from_background++;
                 }
             }
         }
 
         BOOST_CHECK_EQUAL(coins_in_active, initial_total_coins + new_coins);
-        BOOST_CHECK_EQUAL(coins_in_ibd, initial_total_coins);
-        BOOST_CHECK_EQUAL(coins_missing_ibd, new_coins);
+        BOOST_CHECK_EQUAL(coins_in_background, initial_total_coins);
+        BOOST_CHECK_EQUAL(coins_missing_from_background, new_coins);
     }
 
     // Snapshot should refuse to load after one has already loaded.
diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp
index 22aafcaa6c..9136c497ea 100644
--- a/src/test/validation_flush_tests.cpp
+++ b/src/test/validation_flush_tests.cpp
@@ -9,7 +9,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, BasicTestingSetup)
+BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, ChainTestingSetup)
 
 //! Test utilities for detecting when we need to flush the coins cache based
 //! on estimated memory usage.
@@ -20,7 +20,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
 {
     CTxMemPool mempool;
     BlockManager blockman{};
-    CChainState chainstate{&mempool, blockman};
+    CChainState chainstate{&mempool, blockman, *Assert(m_node.chainman)};
     chainstate.InitCoinsDB(/*cache_size_bytes*/ 1 << 10, /*in_memory*/ true, /*should_wipe*/ false);
     WITH_LOCK(::cs_main, chainstate.InitCoinsCache(1 << 10));