Merge #16945: refactor: introduce CChainState::GetCoinsCacheSizeState

laanwj · laanwj · commit 2ed74a43a05a · 2020-01-13T12:42:38.000+01:00
02b9511 tests: add tests for GetCoinsCacheSizeState (James O'Beirne) b17e91d refactoring: introduce CChainState::GetCoinsCacheSizeState (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 pulls out the routine for detection of how full the coins cache is from FlushStateToDisk. We use this logic independently when deciding when to flush the coins cache during UTXO snapshot activation ([see here](bitcoin/bitcoin@231fb5f#diff-24efdb00bfbe56b140fb006b562cc70bR5275)). ACKs for top commit: ariard: Code review ACK 02b9511. ryanofsky: Code review ACK 02b9511. Just rebase, new COIN_SIZE comment, and new test message since last review Tree-SHA512: 8bdd78bf68a4a5d33a776e73fcc2857f050d6d102caa4997ed19ca25468c1358e6e728199d61b423033c02e6bc8f00a1d9da52cf17a2d37d70860fca9237ea7c
diff --git a/src/Makefile.test.include b/src/Makefile.test.include
@@ -185,6 +185,7 @@ BITCOIN_TESTS =\
   test/uint256_tests.cpp \
   test/util_tests.cpp \
   test/validation_block_tests.cpp \
+  test/validation_flush_tests.cpp \
   test/versionbits_tests.cpp
 
 if ENABLE_PROPERTY_TESTS
diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp
@@ -0,0 +1,174 @@
+// Copyright (c) 2019 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 <txmempool.h>
+#include <validation.h>
+#include <sync.h>
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(validation_flush_tests, BasicTestingSetup)
+
+//! Test utilities for detecting when we need to flush the coins cache based
+//! on estimated memory usage.
+//!
+//! @sa CChainState::GetCoinsCacheSizeState()
+//!
+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());
+    CTxMemPool tx_pool{};
+
+    constexpr bool is_64_bit = sizeof(void*) == 8;
+
+    LOCK(::cs_main);
+    auto& view = chainstate.CoinsTip();
+
+    //! 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;
+    };
+
+    // The number of bytes consumed by coin's heap data, i.e. CScript
+    // (prevector<28, unsigned char>) when assigned 56 bytes of data per above.
+    //
+    // See also: Coin::DynamicMemoryUsage().
+    constexpr int COIN_SIZE = is_64_bit ? 80 : 64;
+
+    auto print_view_mem_usage = [](CCoinsViewCache& view) {
+        BOOST_TEST_MESSAGE("CCoinsViewCache memory usage: " << view.DynamicMemoryUsage());
+    };
+
+    constexpr size_t MAX_COINS_CACHE_BYTES = 1024;
+
+    // Without any coins in the cache, we shouldn't need to flush.
+    BOOST_CHECK_EQUAL(
+        chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
+        CoinsCacheSizeState::OK);
+
+    // If the initial memory allocations of cacheCoins don't match these common
+    // cases, we can't really continue to make assertions about memory usage.
+    // End the test early.
+    if (view.DynamicMemoryUsage() != 32 && view.DynamicMemoryUsage() != 16) {
+        // Add a bunch of coins to see that we at least flip over to CRITICAL.
+
+        for (int i{0}; i < 1000; ++i) {
+            COutPoint res = add_coin(view);
+            BOOST_CHECK_EQUAL(view.AccessCoin(res).DynamicMemoryUsage(), COIN_SIZE);
+        }
+
+        BOOST_CHECK_EQUAL(
+            chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
+            CoinsCacheSizeState::CRITICAL);
+
+        BOOST_TEST_MESSAGE("Exiting cache flush tests early due to unsupported arch");
+        return;
+    }
+
+    print_view_mem_usage(view);
+    BOOST_CHECK_EQUAL(view.DynamicMemoryUsage(), is_64_bit ? 32 : 16);
+
+    // We should be able to add COINS_UNTIL_CRITICAL coins to the cache before going CRITICAL.
+    // This is contingent not only on the dynamic memory usage of the Coins
+    // that we're adding (COIN_SIZE bytes per), but also on how much memory the
+    // cacheCoins (unordered_map) preallocates.
+    //
+    // I came up with the count by examining the printed memory usage of the
+    // CCoinsCacheView, so it's sort of arbitrary - but it shouldn't change
+    // unless we somehow change the way the cacheCoins map allocates memory.
+    //
+    constexpr int COINS_UNTIL_CRITICAL = is_64_bit ? 4 : 5;
+
+    for (int i{0}; i < COINS_UNTIL_CRITICAL; ++i) {
+        COutPoint res = add_coin(view);
+        print_view_mem_usage(view);
+        BOOST_CHECK_EQUAL(view.AccessCoin(res).DynamicMemoryUsage(), COIN_SIZE);
+        BOOST_CHECK_EQUAL(
+            chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
+            CoinsCacheSizeState::OK);
+    }
+
+    // Adding an additional coin will push us over the edge to CRITICAL.
+    add_coin(view);
+    print_view_mem_usage(view);
+
+    auto size_state = chainstate.GetCoinsCacheSizeState(
+        tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0);
+
+    if (!is_64_bit && size_state == CoinsCacheSizeState::LARGE) {
+        // On 32 bit hosts, we may hit LARGE before CRITICAL.
+        add_coin(view);
+        print_view_mem_usage(view);
+    }
+
+    BOOST_CHECK_EQUAL(
+        chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 0),
+        CoinsCacheSizeState::CRITICAL);
+
+    // Passing non-zero max mempool usage should allow us more headroom.
+    BOOST_CHECK_EQUAL(
+        chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
+        CoinsCacheSizeState::OK);
+
+    for (int i{0}; i < 3; ++i) {
+        add_coin(view);
+        print_view_mem_usage(view);
+        BOOST_CHECK_EQUAL(
+            chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, /*max_mempool_size_bytes*/ 1 << 10),
+            CoinsCacheSizeState::OK);
+    }
+
+    // Adding another coin with the additional mempool room will put us >90%
+    // but not yet critical.
+    add_coin(view);
+    print_view_mem_usage(view);
+
+    // Only perform these checks on 64 bit hosts; I haven't done the math for 32.
+    if (is_64_bit) {
+        float usage_percentage = (float)view.DynamicMemoryUsage() / (MAX_COINS_CACHE_BYTES + (1 << 10));
+        BOOST_TEST_MESSAGE("CoinsTip usage percentage: " << usage_percentage);
+        BOOST_CHECK(usage_percentage >= 0.9);
+        BOOST_CHECK(usage_percentage < 1);
+        BOOST_CHECK_EQUAL(
+            chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, 1 << 10),
+            CoinsCacheSizeState::LARGE);
+    }
+
+    // Using the default max_* values permits way more coins to be added.
+    for (int i{0}; i < 1000; ++i) {
+        add_coin(view);
+        BOOST_CHECK_EQUAL(
+            chainstate.GetCoinsCacheSizeState(tx_pool),
+            CoinsCacheSizeState::OK);
+    }
+
+    // Flushing the view doesn't take us back to OK because cacheCoins has
+    // preallocated memory that doesn't get reclaimed even after flush.
+
+    BOOST_CHECK_EQUAL(
+        chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, 0),
+        CoinsCacheSizeState::CRITICAL);
+
+    view.SetBestBlock(InsecureRand256());
+    BOOST_CHECK(view.Flush());
+    print_view_mem_usage(view);
+
+    BOOST_CHECK_EQUAL(
+        chainstate.GetCoinsCacheSizeState(tx_pool, MAX_COINS_CACHE_BYTES, 0),
+        CoinsCacheSizeState::CRITICAL);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
diff --git a/src/txdb.h b/src/txdb.h
@@ -20,8 +20,6 @@ class CBlockIndex;
 class CCoinsViewDBCursor;
 class uint256;
 
-//! No need to periodic flush if at least this much space still available.
-static constexpr int MAX_BLOCK_COINSDB_USAGE = 10;
 //! -dbcache default (MiB)
 static const int64_t nDefaultDbCache = 450;
 //! -dbbatchsize default (bytes)
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -2187,13 +2187,44 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
     return true;
 }
 
+CoinsCacheSizeState CChainState::GetCoinsCacheSizeState(const CTxMemPool& tx_pool)
+{
+    return this->GetCoinsCacheSizeState(
+        tx_pool,
+        nCoinCacheUsage,
+        gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
+}
+
+CoinsCacheSizeState CChainState::GetCoinsCacheSizeState(
+    const CTxMemPool& tx_pool,
+    size_t max_coins_cache_size_bytes,
+    size_t max_mempool_size_bytes)
+{
+    int64_t nMempoolUsage = tx_pool.DynamicMemoryUsage();
+    int64_t cacheSize = CoinsTip().DynamicMemoryUsage();
+    int64_t nTotalSpace =
+        max_coins_cache_size_bytes + std::max<int64_t>(max_mempool_size_bytes - nMempoolUsage, 0);
+
+    //! No need to periodic flush if at least this much space still available.
+    static constexpr int64_t MAX_BLOCK_COINSDB_USAGE_BYTES = 10 * 1024 * 1024;  // 10MB
+    int64_t large_threshold =
+        std::max((9 * nTotalSpace) / 10, nTotalSpace - MAX_BLOCK_COINSDB_USAGE_BYTES);
+
+    if (cacheSize > nTotalSpace) {
+        LogPrintf("Cache size (%s) exceeds total space (%s)\n", cacheSize, nTotalSpace);
+        return CoinsCacheSizeState::CRITICAL;
+    } else if (cacheSize > large_threshold) {
+        return CoinsCacheSizeState::LARGE;
+    }
+    return CoinsCacheSizeState::OK;
+}
+
 bool CChainState::FlushStateToDisk(
     const CChainParams& chainparams,
     BlockValidationState &state,
     FlushStateMode mode,
     int nManualPruneHeight)
 {
-    int64_t nMempoolUsage = mempool.DynamicMemoryUsage();
     LOCK(cs_main);
     assert(this->CanFlushToDisk());
     static int64_t nLastWrite = 0;
@@ -2208,6 +2239,7 @@ bool CChainState::FlushStateToDisk(
     {
         bool fFlushForPrune = false;
         bool fDoFullFlush = false;
+        CoinsCacheSizeState cache_state = GetCoinsCacheSizeState(::mempool);
         LOCK(cs_LastBlockFile);
         if (fPruneMode && (fCheckForPruning || nManualPruneHeight > 0) && !fReindex) {
             if (nManualPruneHeight > 0) {
@@ -2236,13 +2268,10 @@ bool CChainState::FlushStateToDisk(
         if (nLastFlush == 0) {
             nLastFlush = nNow;
         }
-        int64_t nMempoolSizeMax = gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000;
-        int64_t cacheSize = CoinsTip().DynamicMemoryUsage();
-        int64_t nTotalSpace = nCoinCacheUsage + std::max<int64_t>(nMempoolSizeMax - nMempoolUsage, 0);
         // The cache is large and we're within 10% and 10 MiB of the limit, but we have time now (not in the middle of a block processing).
-        bool fCacheLarge = mode == FlushStateMode::PERIODIC && cacheSize > std::max((9 * nTotalSpace) / 10, nTotalSpace - MAX_BLOCK_COINSDB_USAGE * 1024 * 1024);
+        bool fCacheLarge = mode == FlushStateMode::PERIODIC && cache_state >= CoinsCacheSizeState::LARGE;
         // The cache is over the limit, we have to write now.
-        bool fCacheCritical = mode == FlushStateMode::IF_NEEDED && cacheSize > nTotalSpace;
+        bool fCacheCritical = mode == FlushStateMode::IF_NEEDED && cache_state >= CoinsCacheSizeState::CRITICAL;
         // It's been a while since we wrote the block index to disk. Do this frequently, so we don't need to redownload after a crash.
         bool fPeriodicWrite = mode == FlushStateMode::PERIODIC && nNow > nLastWrite + (int64_t)DATABASE_WRITE_INTERVAL * 1000000;
         // It's been very long since we flushed the cache. Do this infrequently, to optimize cache usage.
diff --git a/src/validation.h b/src/validation.h
@@ -530,6 +530,15 @@ class CoinsViews {
     void InitCache() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 };
 
+enum class CoinsCacheSizeState
+{
+    //! The coins cache is in immediate need of a flush.
+    CRITICAL = 2,
+    //! The cache is at >= 90% capacity.
+    LARGE = 1,
+    OK = 0
+};
+
 /**
  * CChainState stores and provides an API to update our local knowledge of the
  * current best chain.
@@ -721,6 +730,17 @@ class CChainState {
     /** Update the chain tip based on database information, i.e. CoinsTip()'s best block. */
     bool LoadChainTip(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
+    //! Dictates whether we need to flush the cache to disk or not.
+    //!
+    //! @return the state of the size of the coins cache.
+    CoinsCacheSizeState GetCoinsCacheSizeState(const CTxMemPool& tx_pool)
+        EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
+    CoinsCacheSizeState GetCoinsCacheSizeState(
+        const CTxMemPool& tx_pool,
+        size_t max_coins_cache_size_bytes,
+        size_t max_mempool_size_bytes) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+
 private:
     bool ActivateBestChainStep(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexMostWork, const std::shared_ptr<const CBlock>& pblock, bool& fInvalidFound, ConnectTrace& connectTrace) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
     bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, ::mempool.cs);
