Merge #19806: validation: UTXO snapshot activation

1afc0e4 doc: remove potentially confusing ChainstateManager comment (James O'Beirne)
769a1ef test: Add tests with maleated snapshot data (Fabian Jahr)
4d8de04 tests: add snapshot activation test (James O'Beirne)
31d2252 tests: add deterministic chain generation unittest fixture (James O'Beirne)
6606a4f move-onlyish: break out CreateUTXOSnapshot from dumptxoutset (James O'Beirne)
ad949ba txdb: don't reset during in-memory cache resize (James O'Beirne)
f6e2da5 simplify ChainstateManager::SnapshotBlockhash() return semantics (James O'Beirne)
7a6c46b chainparams: add allowed assumeutxo values (James O'Beirne)

Pull request description:

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

  Parent PR: #15606
  Issue: #15605
  Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal

  ---

  This change proposes logic for activating UTXO snapshots, which is unused at the moment aside from an included unittest. There are a few moveonyish/refactoring commits to allow for halfway decent unittests.

  Basic structure is included for specifying and checking the assumeutxo hash values used to validate activated snapshots. Initially I had specified a few height/hash pairs for mainnet in this change, but because of the security-critical nature of those parameters, I figured it was better to leave their inclusion to a future PR that includes only that change - my intent being that reviewers will be more likely to verify those parameters firsthand in a dedicated PR.

  Aside from that and the snapshot activation logic, there are a few related changes:

  - ~~allow caching the `nChainTx` value in the CCoinsViewDB; this is set during snapshot activation. Because we don't necessarily have access to the full chain at the time of snapshot load, this value is communicated through the snapshot metadata and must be cached within the chainstate to survive restarts.~~
  - break out `CreateUTXOSnapshot()` from dumptxoutset. This is essentially a move-only change to allow the reuse of snapshot creation logic from within unittests.
  - ...and a few other misc. changes that are solely related to unittests.

  The move-onlyish commit is most easily reviewed with `--color-moved=zebra`.

ACKs for top commit:
  fjahr:
    Code review ACK 1afc0e4
  laanwj:
    Code review ACK 1afc0e4

Tree-SHA512: a4e4f0698f00a53ec298b5e8b7ef1c9fdf0185f95139d1b1f63cfdf6cbbd6d17b8c6e51bbf1de2e5f1a946bf49f8466232698ef55acce5a012c80b067da366ea

Author: laanwj

src/chain.h

@@ -163,14 +163,27 @@ class CBlockIndex
 
     //! Number of transactions in this block.
     //! Note: in a potential headers-first mode, this number cannot be relied upon
+    //! Note: this value is faked during UTXO snapshot load to ensure that
+    //! LoadBlockIndex() will load index entries for blocks that we lack data for.
+    //! @sa ActivateSnapshot
     unsigned int nTx{0};
 
     //! (memory only) Number of transactions in the chain up to and including this block.
     //! This value will be non-zero only if and only if transactions for this block and all its parents are available.
     //! Change to 64-bit type when necessary; won't happen before 2030
+    //!
+    //! Note: this value is faked during use of a UTXO snapshot because we don't
+    //! have the underlying block data available during snapshot load.
+    //! @sa AssumeutxoData
+    //! @sa ActivateSnapshot
     unsigned int nChainTx{0};
 
     //! Verification status of this block. See enum BlockStatus
+    //!
+    //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
+    //! load to avoid the block index being spuriously rewound.
+    //! @sa RewindBlockIndex
+    //! @sa ActivateSnapshot
     uint32_t nStatus{0};
 
     //! block header

src/chainparams.cpp

@@ -8,7 +8,6 @@
 #include <chainparamsseeds.h>
 #include <consensus/merkle.h>
 #include <hash.h> // for signet block challenge hash
-#include <tinyformat.h>
 #include <util/system.h>
 #include <util/strencodings.h>
 #include <versionbitsinfo.h>
@@ -161,6 +160,10 @@ class CMainParams : public CChainParams {
             }
         };
 
+        m_assumeutxo_data = MapAssumeutxo{
+         // TODO to be specified in a future patch.
+        };
+
         chainTxData = ChainTxData{
             // Data from RPC: getchaintxstats 4096 0000000000000000000b9d2ec5a352ecba0592946514a92f14319dc2b367fc72
             /* nTime    */ 1603995752,
@@ -250,6 +253,10 @@ class CTestNetParams : public CChainParams {
             }
         };
 
+        m_assumeutxo_data = MapAssumeutxo{
+            // TODO to be specified in a future patch.
+        };
+
         chainTxData = ChainTxData{
             // Data from RPC: getchaintxstats 4096 000000000000006433d1efec504c53ca332b64963c425395515b01977bd7b3b0
             /* nTime    */ 1603359686,
@@ -431,6 +438,17 @@ class CRegTestParams : public CChainParams {
             }
         };
 
+        m_assumeutxo_data = MapAssumeutxo{
+            {
+                110,
+                {uint256S("0x76fd7334ac7c1baf57ddc0c626f073a655a35d98a4258cd1382c8cc2b8392e10"), 110},
+            },
+            {
+                210,
+                {uint256S("0x9c5ed99ef98544b34f8920b6d1802f72ac28ae6e2bd2bd4c316ff10c230df3f2"), 210},
+            },
+        };
+
         chainTxData = ChainTxData{
             0,
             0,
@@ -526,3 +544,9 @@ void SelectParams(const std::string& network)
     SelectBaseParams(network);
     globalChainParams = CreateChainParams(gArgs, network);
 }
+
+std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud)
+{
+    o << strprintf("AssumeutxoData(%s, %s)", aud.hash_serialized.ToString(), aud.nChainTx);
+    return o;
+}

src/chainparams.h

@@ -30,6 +30,26 @@ struct CCheckpointData {
     }
 };
 
+/**
+ * Holds configuration for use during UTXO snapshot load and validation. The contents
+ * here are security critical, since they dictate which UTXO snapshots are recognized
+ * as valid.
+ */
+struct AssumeutxoData {
+    //! The expected hash of the deserialized UTXO set.
+    const uint256 hash_serialized;
+
+    //! Used to populate the nChainTx value, which is used during BlockManager::LoadBlockIndex().
+    //!
+    //! We need to hardcode the value here because this is computed cumulatively using block data,
+    //! which we do not necessarily have at the time of snapshot load.
+    const unsigned int nChainTx;
+};
+
+std::ostream& operator<<(std::ostream& o, const AssumeutxoData& aud);
+
+using MapAssumeutxo = std::map<int, const AssumeutxoData>;
+
 /**
  * Holds various statistics on transactions within a chain. Used to estimate
  * verification progress during chain sync.
@@ -90,6 +110,11 @@ class CChainParams
     const std::string& Bech32HRP() const { return bech32_hrp; }
     const std::vector<SeedSpec6>& FixedSeeds() const { return vFixedSeeds; }
     const CCheckpointData& Checkpoints() const { return checkpointData; }
+
+    //! Get allowed assumeutxo configuration.
+    //! @see ChainstateManager
+    const MapAssumeutxo& Assumeutxo() const { return m_assumeutxo_data; }
+
     const ChainTxData& TxData() const { return chainTxData; }
 protected:
     CChainParams() {}
@@ -111,6 +136,7 @@ class CChainParams
     bool m_is_test_chain;
     bool m_is_mockable_chain;
     CCheckpointData checkpointData;
+    MapAssumeutxo m_assumeutxo_data;
     ChainTxData chainTxData;
 };
 

src/coins.cpp

@@ -97,6 +97,14 @@ void CCoinsViewCache::AddCoin(const COutPoint &outpoint, Coin&& coin, bool possi
     cachedCoinsUsage += it->second.coin.DynamicMemoryUsage();
 }
 
+void CCoinsViewCache::EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin) {
+    cachedCoinsUsage += coin.DynamicMemoryUsage();
+    cacheCoins.emplace(
+        std::piecewise_construct,
+        std::forward_as_tuple(std::move(outpoint)),
+        std::forward_as_tuple(std::move(coin), CCoinsCacheEntry::DIRTY));
+}
+
 void AddCoins(CCoinsViewCache& cache, const CTransaction &tx, int nHeight, bool check_for_overwrite) {
     bool fCoinbase = tx.IsCoinBase();
     const uint256& txid = tx.GetHash();

src/coins.h

@@ -20,6 +20,8 @@
 #include <functional>
 #include <unordered_map>
 
+class ChainstateManager;
+
 /**
  * A UTXO entry.
  *
@@ -125,6 +127,7 @@ struct CCoinsCacheEntry
 
     CCoinsCacheEntry() : flags(0) {}
     explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {}
+    CCoinsCacheEntry(Coin&& coin_, unsigned char flag) : coin(std::move(coin_)), flags(flag) {}
 };
 
 typedef std::unordered_map<COutPoint, CCoinsCacheEntry, SaltedOutpointHasher> CCoinsMap;
@@ -262,6 +265,15 @@ class CCoinsViewCache : public CCoinsViewBacked
      */
     void AddCoin(const COutPoint& outpoint, Coin&& coin, bool possible_overwrite);
 
+    /**
+     * Emplace a coin into cacheCoins without performing any checks, marking
+     * the emplaced coin as dirty.
+     *
+     * NOT FOR GENERAL USE. Used only when loading coins from a UTXO snapshot.
+     * @sa ChainstateManager::PopulateAndValidateSnapshot()
+     */
+    void EmplaceCoinInternalDANGER(COutPoint&& outpoint, Coin&& coin);
+
     /**
      * Spend a coin. Pass moveto in order to get the deleted data.
      * If no unspent output exists for the passed outpoint, this call

src/node/coinstats.cpp

@@ -55,6 +55,18 @@ static void ApplyHash(CCoinsStats& stats, MuHash3072& muhash, const uint256& has
     muhash.Insert(MakeUCharSpan(ss));
 }
 
+//! Warning: be very careful when changing this! assumeutxo and UTXO snapshot
+//! validation commitments are reliant on the hash constructed by this
+//! function.
+//!
+//! If the construction of this hash is changed, it will invalidate
+//! existing UTXO snapshots. This will not result in any kind of consensus
+//! failure, but it will force clients that were expecting to make use of
+//! assumeutxo to do traditional IBD instead.
+//!
+//! It is also possible, though very unlikely, that a change in this
+//! construction could cause a previously invalid (and potentially malicious)
+//! UTXO snapshot to be considered valid.
 template <typename T>
 static void ApplyStats(CCoinsStats& stats, T& hash_obj, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
 {

src/rpc/blockchain.cpp

@@ -2411,10 +2411,21 @@ static RPCHelpMan dumptxoutset()
 
     FILE* file{fsbridge::fopen(temppath, "wb")};
     CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
+    NodeContext& node = EnsureNodeContext(request.context);
+    UniValue result = CreateUTXOSnapshot(node, node.chainman->ActiveChainstate(), afile);
+    fs::rename(temppath, path);
+
+    result.pushKV("path", path.string());
+    return result;
+},
+    };
+}
+
+UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile)
+{
     std::unique_ptr<CCoinsViewCursor> pcursor;
     CCoinsStats stats;
     CBlockIndex* tip;
-    NodeContext& node = EnsureNodeContext(request.context);
 
     {
         // We need to lock cs_main to ensure that the coinsdb isn't written to
@@ -2431,13 +2442,13 @@ static RPCHelpMan dumptxoutset()
         //
         LOCK(::cs_main);
 
-        ::ChainstateActive().ForceFlushStateToDisk();
+        chainstate.ForceFlushStateToDisk();
 
-        if (!GetUTXOStats(&::ChainstateActive().CoinsDB(), stats, CoinStatsHashType::NONE, node.rpc_interruption_point)) {
+        if (!GetUTXOStats(&chainstate.CoinsDB(), stats, CoinStatsHashType::NONE, node.rpc_interruption_point)) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
         }
 
-        pcursor = std::unique_ptr<CCoinsViewCursor>(::ChainstateActive().CoinsDB().Cursor());
+        pcursor = std::unique_ptr<CCoinsViewCursor>(chainstate.CoinsDB().Cursor());
         tip = g_chainman.m_blockman.LookupBlockIndex(stats.hashBlock);
         CHECK_NONFATAL(tip);
     }
@@ -2462,16 +2473,13 @@ static RPCHelpMan dumptxoutset()
     }
 
     afile.fclose();
-    fs::rename(temppath, path);
 
     UniValue result(UniValue::VOBJ);
     result.pushKV("coins_written", stats.coins_count);
     result.pushKV("base_hash", tip->GetBlockHash().ToString());
     result.pushKV("base_height", tip->nHeight);
-    result.pushKV("path", path.string());
+
     return result;
-},
-    };
 }
 
 void RegisterBlockchainRPCCommands(CRPCTable &t)

src/rpc/blockchain.h

@@ -6,6 +6,7 @@
 #define BITCOIN_RPC_BLOCKCHAIN_H
 
 #include <amount.h>
+#include <streams.h>
 #include <sync.h>
 
 #include <stdint.h>
@@ -16,6 +17,7 @@ extern RecursiveMutex cs_main;
 class CBlock;
 class CBlockIndex;
 class CBlockPolicyEstimator;
+class CChainState;
 class CTxMemPool;
 class ChainstateManager;
 class UniValue;
@@ -57,4 +59,10 @@ CTxMemPool& EnsureMemPool(const util::Ref& context);
 ChainstateManager& EnsureChainman(const util::Ref& context);
 CBlockPolicyEstimator& EnsureFeeEstimator(const util::Ref& context);
 
+/**
+ * Helper to create UTXO snapshots given a chainstate and a file handle.
+ * @return a UniValue map containing metadata about the snapshot.
+ */
+UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFile& afile);
+
 #endif

src/test/util/setup_common.cpp

@@ -199,14 +199,43 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
     }
 }
 
-TestChain100Setup::TestChain100Setup()
+TestChain100Setup::TestChain100Setup(bool deterministic)
 {
+    m_deterministic = deterministic;
+
+    if (m_deterministic) {
+        SetMockTime(1598887952);
+        constexpr std::array<unsigned char, 32> vchKey = {
+            {
+                0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1
+            }
+        };
+        coinbaseKey.Set(vchKey.begin(), vchKey.end(), false);
+    } else {
+        coinbaseKey.MakeNewKey(true);
+    }
+
     // Generate a 100-block chain:
-    coinbaseKey.MakeNewKey(true);
+    this->mineBlocks(COINBASE_MATURITY);
+
+    if (m_deterministic) {
+        LOCK(::cs_main);
+        assert(
+            m_node.chainman->ActiveChain().Tip()->GetBlockHash().ToString() ==
+            "49c95db1e470fed04496d801c9d8fbb78155d2c7f855232c918823d2c17d0cf6");
+    }
+}
+
+void TestChain100Setup::mineBlocks(int num_blocks)
+{
     CScript scriptPubKey = CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG;
-    for (int i = 0; i < COINBASE_MATURITY; i++) {
+    for (int i = 0; i < num_blocks; i++)
+    {
         std::vector<CMutableTransaction> noTxns;
         CBlock b = CreateAndProcessBlock(noTxns, scriptPubKey);
+        if (m_deterministic) {
+            SetMockTime(GetTime() + 1);
+        }
         m_coinbase_txns.push_back(b.vtx[0]);
     }
 }
@@ -234,6 +263,9 @@ CBlock TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransa
 TestChain100Setup::~TestChain100Setup()
 {
     gArgs.ForceSetArg("-segwitheight", "0");
+    if (m_deterministic) {
+        SetMockTime(0);
+    }
 }
 
 CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) const

src/test/util/setup_common.h

@@ -78,7 +78,6 @@ struct BasicTestingSetup {
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();
 
-private:
     const fs::path m_path_root;
 };
 
@@ -112,7 +111,7 @@ class CScript;
  * Testing fixture that pre-creates a 100-block REGTEST-mode block chain
  */
 struct TestChain100Setup : public RegTestingSetup {
-    TestChain100Setup();
+    TestChain100Setup(bool deterministic = false);
 
     /**
      * Create a new block with just given transactions, coinbase paying to
@@ -121,12 +120,21 @@ struct TestChain100Setup : public RegTestingSetup {
     CBlock CreateAndProcessBlock(const std::vector<CMutableTransaction>& txns,
                                  const CScript& scriptPubKey);
 
+    //! Mine a series of new blocks on the active chain.
+    void mineBlocks(int num_blocks);
+
     ~TestChain100Setup();
 
+    bool m_deterministic;
     std::vector<CTransactionRef> m_coinbase_txns; // For convenience, coinbase transactions
     CKey coinbaseKey; // private/public key needed to spend coinbase transactions
 };
 
+
+struct TestChain100DeterministicSetup : public TestChain100Setup {
+    TestChain100DeterministicSetup() : TestChain100Setup(true) { }
+};
+
 class CTxMemPoolEntry;
 
 struct TestMemPoolEntryHelper