Merge #18637: coins: allow cache resize after init

f19fdd4 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne)
8ac3ef4 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne)
f36aaa6 Add CChainState::ResizeCoinsCaches (James O'Beirne)
b223111 txdb: add CCoinsViewDB::ChangeCacheSize (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

  ---

  In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate.

  Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.

  This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`).

  `ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.

ACKs for top commit:
  ajtowns:
    weak utACK f19fdd4 -- didn't find any major problems, but not super confident that I didn't miss anything
  fjahr:
    Code review ACK f19fdd4
  ryanofsky:
    Code review ACK f19fdd4. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)

Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269

Author: MarcoFalke

src/Makefile.test.include

@@ -275,6 +275,7 @@ BITCOIN_TESTS =\
   test/uint256_tests.cpp \
   test/util_tests.cpp \
   test/validation_block_tests.cpp \
+  test/validation_chainstate_tests.cpp \
   test/validation_chainstatemanager_tests.cpp \
   test/validation_flush_tests.cpp \
   test/validationinterface_tests.cpp \

src/coins.cpp

@@ -245,6 +245,14 @@ bool CCoinsViewCache::HaveInputs(const CTransaction& tx) const
     return true;
 }
 
+void CCoinsViewCache::ReallocateCache()
+{
+    // Cache should be empty when we're calling this.
+    assert(cacheCoins.size() == 0);
+    cacheCoins.~CCoinsMap();
+    ::new (&cacheCoins) CCoinsMap();
+}
+
 static const size_t MIN_TRANSACTION_OUTPUT_WEIGHT = WITNESS_SCALE_FACTOR * ::GetSerializeSize(CTxOut(), PROTOCOL_VERSION);
 static const size_t MAX_OUTPUTS_PER_BLOCK = MAX_BLOCK_WEIGHT / MIN_TRANSACTION_OUTPUT_WEIGHT;
 

src/coins.h

@@ -318,6 +318,13 @@ class CCoinsViewCache : public CCoinsViewBacked
     //! Check whether all prevouts of the transaction are present in the UTXO set represented by this view
     bool HaveInputs(const CTransaction& tx) const;
 
+    //! Force a reallocation of the cache map. This is required when downsizing
+    //! the cache because the map's allocator may be hanging onto a lot of
+    //! memory despite having called .clear().
+    //!
+    //! See: https://stackoverflow.com/questions/42114044/how-to-release-unordered-map-memory
+    void ReallocateCache();
+
 private:
     /**
      * @note this is marked const, but may actually append to `cacheCoins`, increasing

src/init.cpp

@@ -1534,7 +1534,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
     int64_t nCoinDBCache = std::min(nTotalCache / 2, (nTotalCache / 4) + (1 << 23)); // use 25%-50% of the remainder for disk cache
     nCoinDBCache = std::min(nCoinDBCache, nMaxCoinsDBCache << 20); // cap total coins db cache
     nTotalCache -= nCoinDBCache;
-    nCoinCacheUsage = nTotalCache; // the rest goes to in-memory cache
+    int64_t nCoinCacheUsage = nTotalCache; // the rest goes to in-memory cache
     int64_t nMempoolSizeMax = gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000;
     LogPrintf("Cache configuration:\n");
     LogPrintf("* Using %.1f MiB for block index database\n", nBlockTreeDBCache * (1.0 / 1024 / 1024));
@@ -1563,6 +1563,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
             try {
                 LOCK(cs_main);
                 chainman.InitializeChainstate();
+                chainman.m_total_coinstip_cache = nCoinCacheUsage;
+                chainman.m_total_coinsdb_cache = nCoinDBCache;
+
                 UnloadBlockIndex();
 
                 // new CBlockTreeDB tries to delete the existing file, which
@@ -1646,7 +1649,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
                     }
 
                     // The on-disk coinsdb is now in a good state, create the cache
-                    chainstate->InitCoinsCache();
+                    chainstate->InitCoinsCache(nCoinCacheUsage);
                     assert(chainstate->CanFlushToDisk());
 
                     if (!is_coinsview_empty(chainstate)) {

src/test/util/setup_common.cpp

@@ -142,7 +142,7 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
     ::ChainstateActive().InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
     assert(!::ChainstateActive().CanFlushToDisk());
-    ::ChainstateActive().InitCoinsCache();
+    ::ChainstateActive().InitCoinsCache(1 << 23);
     assert(::ChainstateActive().CanFlushToDisk());
     if (!LoadGenesisBlock(chainparams)) {
         throw std::runtime_error("LoadGenesisBlock failed.");

src/test/validation_chainstate_tests.cpp

@@ -0,0 +1,76 @@
+// Copyright (c) 2020 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 <random.h>
+#include <uint256.h>
+#include <consensus/validation.h>
+#include <sync.h>
+#include <test/util/setup_common.h>
+#include <validation.h>
+
+#include <vector>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(validation_chainstate_tests, TestingSetup)
+
+//! Test resizing coins-related CChainState caches during runtime.
+//!
+BOOST_AUTO_TEST_CASE(validation_chainstate_resize_caches)
+{
+    ChainstateManager manager;
+
+    //! Create and add a Coin with DynamicMemoryUsage of 80 bytes to the given view.
+    auto add_coin = [](CCoinsViewCache& coins_view) -> COutPoint {
+        Coin newcoin;
+        uint256 txid = InsecureRand256();
+        COutPoint outp{txid, 0};
+        newcoin.nHeight = 1;
+        newcoin.out.nValue = InsecureRand32();
+        newcoin.out.scriptPubKey.assign((uint32_t)56, 1);
+        coins_view.AddCoin(outp, std::move(newcoin), false);
+
+        return outp;
+    };
+
+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c1 = manager.InitializeChainstate();
+    LEAVE_CRITICAL_SECTION(cs_main);
+    c1.InitCoinsDB(
+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
+    WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));
+
+    // Add a coin to the in-memory cache, upsize once, then downsize.
+    {
+        LOCK(::cs_main);
+        auto outpoint = add_coin(c1.CoinsTip());
+
+        // Set a meaningless bestblock value in the coinsview cache - otherwise we won't
+        // flush during ResizecoinsCaches() and will subsequently hit an assertion.
+        c1.CoinsTip().SetBestBlock(InsecureRand256());
+
+        BOOST_CHECK(c1.CoinsTip().HaveCoinInCache(outpoint));
+
+        c1.ResizeCoinsCaches(
+            1 << 24,  // upsizing the coinsview cache
+            1 << 22  // downsizing the coinsdb cache
+        );
+
+        // View should still have the coin cached, since we haven't destructed the cache on upsize.
+        BOOST_CHECK(c1.CoinsTip().HaveCoinInCache(outpoint));
+
+        c1.ResizeCoinsCaches(
+            1 << 22,  // downsizing the coinsview cache
+            1 << 23  // upsizing the coinsdb cache
+        );
+
+        // The view cache should be empty since we had to destruct to downsize.
+        BOOST_CHECK(!c1.CoinsTip().HaveCoinInCache(outpoint));
+    }
+
+    // Avoid triggering the address sanitizer.
+    WITH_LOCK(::cs_main, manager.Unload());
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/validation_chainstatemanager_tests.cpp

@@ -28,13 +28,11 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
 
     // Create a legacy (IBD) chainstate.
     //
-    ENTER_CRITICAL_SECTION(cs_main);
-    CChainState& c1 = manager.InitializeChainstate();
-    LEAVE_CRITICAL_SECTION(cs_main);
+    CChainState& c1 = *WITH_LOCK(::cs_main, return &manager.InitializeChainstate());
     chainstates.push_back(&c1);
     c1.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
-    WITH_LOCK(::cs_main, c1.InitCoinsCache());
+    WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));
 
     BOOST_CHECK(!manager.IsSnapshotActive());
     BOOST_CHECK(!manager.IsSnapshotValidated());
@@ -57,12 +55,13 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
     // Create a snapshot-based chainstate.
     //
     ENTER_CRITICAL_SECTION(cs_main);
-    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
+    CChainState& c2 = *WITH_LOCK(::cs_main,
+        return &manager.InitializeChainstate(GetRandHash()));
     LEAVE_CRITICAL_SECTION(cs_main);
     chainstates.push_back(&c2);
     c2.InitCoinsDB(
         /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
-    WITH_LOCK(::cs_main, c2.InitCoinsCache());
+    WITH_LOCK(::cs_main, c2.InitCoinsCache(1 << 23));
     // Unlike c1, which doesn't have any blocks. Gets us different tip, height.
     c2.LoadGenesisBlock(chainparams);
     BlockValidationState _;
@@ -104,4 +103,58 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
     WITH_LOCK(::cs_main, manager.Unload());
 }
 
+//! Test rebalancing the caches associated with each chainstate.
+BOOST_AUTO_TEST_CASE(chainstatemanager_rebalance_caches)
+{
+    ChainstateManager manager;
+    size_t max_cache = 10000;
+    manager.m_total_coinsdb_cache = max_cache;
+    manager.m_total_coinstip_cache = max_cache;
+
+    std::vector<CChainState*> chainstates;
+
+    // Create a legacy (IBD) chainstate.
+    //
+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c1 = manager.InitializeChainstate();
+    LEAVE_CRITICAL_SECTION(cs_main);
+    chainstates.push_back(&c1);
+    c1.InitCoinsDB(
+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
+
+    {
+        LOCK(::cs_main);
+        c1.InitCoinsCache(1 << 23);
+        c1.CoinsTip().SetBestBlock(InsecureRand256());
+        manager.MaybeRebalanceCaches();
+    }
+
+    BOOST_CHECK_EQUAL(c1.m_coinstip_cache_size_bytes, max_cache);
+    BOOST_CHECK_EQUAL(c1.m_coinsdb_cache_size_bytes, max_cache);
+
+    // Create a snapshot-based chainstate.
+    //
+    ENTER_CRITICAL_SECTION(cs_main);
+    CChainState& c2 = manager.InitializeChainstate(GetRandHash());
+    LEAVE_CRITICAL_SECTION(cs_main);
+    chainstates.push_back(&c2);
+    c2.InitCoinsDB(
+        /* cache_size_bytes */ 1 << 23, /* in_memory */ true, /* should_wipe */ false);
+
+    {
+        LOCK(::cs_main);
+        c2.InitCoinsCache(1 << 23);
+        c2.CoinsTip().SetBestBlock(InsecureRand256());
+        manager.MaybeRebalanceCaches();
+    }
+
+    // Since both chainstates are considered to be in initial block download,
+    // the snapshot chainstate should take priority.
+    BOOST_CHECK_CLOSE(c1.m_coinstip_cache_size_bytes, max_cache * 0.05, 1);
+    BOOST_CHECK_CLOSE(c1.m_coinsdb_cache_size_bytes, max_cache * 0.05, 1);
+    BOOST_CHECK_CLOSE(c2.m_coinstip_cache_size_bytes, max_cache * 0.95, 1);
+    BOOST_CHECK_CLOSE(c2.m_coinsdb_cache_size_bytes, max_cache * 0.95, 1);
+
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/test/validation_flush_tests.cpp

@@ -21,7 +21,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
     BlockManager blockman{};
     CChainState chainstate{blockman};
     chainstate.InitCoinsDB(/*cache_size_bytes*/ 1 << 10, /*in_memory*/ true, /*should_wipe*/ false);
-    WITH_LOCK(::cs_main, chainstate.InitCoinsCache());
+    WITH_LOCK(::cs_main, chainstate.InitCoinsCache(1 << 10));
     CTxMemPool tx_pool{};
 
     constexpr bool is_64_bit = sizeof(void*) == 8;

src/txdb.cpp

@@ -10,6 +10,7 @@
 #include <random.h>
 #include <shutdown.h>
 #include <uint256.h>
+#include <util/memory.h>
 #include <util/system.h>
 #include <util/translation.h>
 #include <util/vector.h>
@@ -39,35 +40,45 @@ struct CoinEntry {
 
 }
 
-CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) : db(ldb_path, nCacheSize, fMemory, fWipe, true)
+CCoinsViewDB::CCoinsViewDB(fs::path ldb_path, size_t nCacheSize, bool fMemory, bool fWipe) :
+    m_db(MakeUnique<CDBWrapper>(ldb_path, nCacheSize, fMemory, fWipe, true)),
+    m_ldb_path(ldb_path),
+    m_is_memory(fMemory) { }
+
+void CCoinsViewDB::ResizeCache(size_t new_cache_size)
 {
+    // Have to do a reset first to get the original `m_db` state to release its
+    // filesystem lock.
+    m_db.reset();
+    m_db = MakeUnique<CDBWrapper>(
+        m_ldb_path, new_cache_size, m_is_memory, /*fWipe*/ false, /*obfuscate*/ true);
 }
 
 bool CCoinsViewDB::GetCoin(const COutPoint &outpoint, Coin &coin) const {
-    return db.Read(CoinEntry(&outpoint), coin);
+    return m_db->Read(CoinEntry(&outpoint), coin);
 }
 
 bool CCoinsViewDB::HaveCoin(const COutPoint &outpoint) const {
-    return db.Exists(CoinEntry(&outpoint));
+    return m_db->Exists(CoinEntry(&outpoint));
 }
 
 uint256 CCoinsViewDB::GetBestBlock() const {
     uint256 hashBestChain;
-    if (!db.Read(DB_BEST_BLOCK, hashBestChain))
+    if (!m_db->Read(DB_BEST_BLOCK, hashBestChain))
         return uint256();
     return hashBestChain;
 }
 
 std::vector<uint256> CCoinsViewDB::GetHeadBlocks() const {
     std::vector<uint256> vhashHeadBlocks;
-    if (!db.Read(DB_HEAD_BLOCKS, vhashHeadBlocks)) {
+    if (!m_db->Read(DB_HEAD_BLOCKS, vhashHeadBlocks)) {
         return std::vector<uint256>();
     }
     return vhashHeadBlocks;
 }
 
 bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
-    CDBBatch batch(db);
+    CDBBatch batch(*m_db);
     size_t count = 0;
     size_t changed = 0;
     size_t batch_size = (size_t)gArgs.GetArg("-dbbatchsize", nDefaultDbBatchSize);
@@ -105,7 +116,7 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
         mapCoins.erase(itOld);
         if (batch.SizeEstimate() > batch_size) {
             LogPrint(BCLog::COINDB, "Writing partial batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0));
-            db.WriteBatch(batch);
+            m_db->WriteBatch(batch);
             batch.Clear();
             if (crash_simulate) {
                 static FastRandomContext rng;
@@ -122,14 +133,14 @@ bool CCoinsViewDB::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlock) {
     batch.Write(DB_BEST_BLOCK, hashBlock);
 
     LogPrint(BCLog::COINDB, "Writing final batch of %.2f MiB\n", batch.SizeEstimate() * (1.0 / 1048576.0));
-    bool ret = db.WriteBatch(batch);
+    bool ret = m_db->WriteBatch(batch);
     LogPrint(BCLog::COINDB, "Committed %u changed transaction outputs (out of %u) to coin database...\n", (unsigned int)changed, (unsigned int)count);
     return ret;
 }
 
 size_t CCoinsViewDB::EstimateSize() const
 {
-    return db.EstimateSize(DB_COIN, (char)(DB_COIN+1));
+    return m_db->EstimateSize(DB_COIN, (char)(DB_COIN+1));
 }
 
 CBlockTreeDB::CBlockTreeDB(size_t nCacheSize, bool fMemory, bool fWipe) : CDBWrapper(GetDataDir() / "blocks" / "index", nCacheSize, fMemory, fWipe) {
@@ -156,7 +167,7 @@ bool CBlockTreeDB::ReadLastBlockFile(int &nFile) {
 
 CCoinsViewCursor *CCoinsViewDB::Cursor() const
 {
-    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(db).NewIterator(), GetBestBlock());
+    CCoinsViewDBCursor *i = new CCoinsViewDBCursor(const_cast<CDBWrapper&>(*m_db).NewIterator(), GetBestBlock());
     /* It seems that there are no "const iterators" for LevelDB.  Since we
        only need read operations on it, use a const-cast to get around
        that restriction.  */
@@ -335,7 +346,7 @@ class CCoins
  * Currently implemented: from the per-tx utxo model (0.8..0.14.x) to per-txout.
  */
 bool CCoinsViewDB::Upgrade() {
-    std::unique_ptr<CDBIterator> pcursor(db.NewIterator());
+    std::unique_ptr<CDBIterator> pcursor(m_db->NewIterator());
     pcursor->Seek(std::make_pair(DB_COINS, uint256()));
     if (!pcursor->Valid()) {
         return true;
@@ -346,7 +357,7 @@ bool CCoinsViewDB::Upgrade() {
     LogPrintf("[0%%]..."); /* Continued */
     uiInterface.ShowProgress(_("Upgrading UTXO database").translated, 0, true);
     size_t batch_size = 1 << 24;
-    CDBBatch batch(db);
+    CDBBatch batch(*m_db);
     int reportDone = 0;
     std::pair<unsigned char, uint256> key;
     std::pair<unsigned char, uint256> prev_key = {DB_COINS, uint256()};
@@ -380,18 +391,18 @@ bool CCoinsViewDB::Upgrade() {
             }
             batch.Erase(key);
             if (batch.SizeEstimate() > batch_size) {
-                db.WriteBatch(batch);
+                m_db->WriteBatch(batch);
                 batch.Clear();
-                db.CompactRange(prev_key, key);
+                m_db->CompactRange(prev_key, key);
                 prev_key = key;
             }
             pcursor->Next();
         } else {
             break;
         }
     }
-    db.WriteBatch(batch);
-    db.CompactRange({DB_COINS, uint256()}, key);
+    m_db->WriteBatch(batch);
+    m_db->CompactRange({DB_COINS, uint256()}, key);
     uiInterface.ShowProgress("", 100, false);
     LogPrintf("[%s].\n", ShutdownRequested() ? "CANCELLED" : "DONE");
     return !ShutdownRequested();