coinstats: Separate hasher/index lookup codepaths

Split out ComputeUTXOStats and LookupUTXOStatsWithIndex from
GetUTXOStats, since the hashing and indexing codepaths are quite
disparate in practice.

Also allow add a constructor to CCoinsStats for it to be constructed
from a a block height and hash. This is used in both codepaths.

Also add a note in GetUTXOStats documenting a behaviour quirk that
predates this patchset.

[META] This allows the hashing codepath to be moved to a separate file
       in a future commit, decoupling callers that only rely on the
       hashing codepath from the indexing one. This is key for
       libbitcoinkernel, which needs to have the hashing codepath for
       AssumeUTXO, but does not wish to be coupled with indexes.

Author: dongcarl

src/node/coinstats.cpp

@@ -19,6 +19,11 @@
 #include <map>
 
 namespace node {
+
+CCoinsStats::CCoinsStats(int block_height, const uint256& block_hash)
+    : nHeight(block_height),
+      hashBlock(block_hash) {}
+
 // Database-independent metric indicating the UTXO set size
 uint64_t GetBogoSize(const CScript& script_pub_key)
 {
@@ -94,24 +99,11 @@ static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map<u
 
 //! Calculate statistics about the unspent transaction output set
 template <typename T>
-static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point, const CBlockIndex* pindex, CoinStatsHashType& hash_type, bool index_requested)
+static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point)
 {
     std::unique_ptr<CCoinsViewCursor> pcursor(view->Cursor());
     assert(pcursor);
 
-    if (!pindex) {
-        LOCK(cs_main);
-        pindex = blockman.LookupBlockIndex(view->GetBestBlock());
-    }
-    stats.nHeight = Assert(pindex)->nHeight;
-    stats.hashBlock = pindex->GetBlockHash();
-
-    // Use CoinStatsIndex if it is requested and available and a hash_type of Muhash or None was requested
-    if ((hash_type == CoinStatsHashType::MUHASH || hash_type == CoinStatsHashType::NONE) && g_coin_stats_index && index_requested) {
-        stats.index_used = true;
-        return g_coin_stats_index->LookUpStats(pindex, stats);
-    }
-
     PrepareHash(hash_obj, stats);
 
     uint256 prevkey;
@@ -142,25 +134,27 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats&
     FinalizeHash(hash_obj, stats);
 
     stats.nDiskSize = view->EstimateSize();
+
     return true;
 }
 
-std::optional<CCoinsStats> GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function<void()>& interruption_point, const CBlockIndex* pindex, bool index_requested)
+std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function<void()>& interruption_point)
 {
-    CCoinsStats stats{};
+    CBlockIndex* pindex = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(view->GetBestBlock()));
+    CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
 
     bool success = [&]() -> bool {
         switch (hash_type) {
         case(CoinStatsHashType::HASH_SERIALIZED): {
             CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION);
-            return GetUTXOStats(view, blockman, stats, ss, interruption_point, pindex, hash_type, index_requested);
+            return ComputeUTXOStats(view, stats, ss, interruption_point);
         }
         case(CoinStatsHashType::MUHASH): {
             MuHash3072 muhash;
-            return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex, hash_type, index_requested);
+            return ComputeUTXOStats(view, stats, muhash, interruption_point);
         }
         case(CoinStatsHashType::NONE): {
-            return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex, hash_type, index_requested);
+            return ComputeUTXOStats(view, stats, nullptr, interruption_point);
         }
         } // no default case, so the compiler can warn about missing cases
         assert(false);
@@ -192,4 +186,43 @@ static void FinalizeHash(MuHash3072& muhash, CCoinsStats& stats)
     stats.hashSerialized = out;
 }
 static void FinalizeHash(std::nullptr_t, CCoinsStats& stats) {}
+
+std::optional<CCoinsStats> LookupUTXOStatsWithIndex(CoinStatsIndex& coin_stats_index, const CBlockIndex* pindex)
+{
+    CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()};
+
+    stats.index_used = true;
+    if (!coin_stats_index.LookUpStats(pindex, stats)) {
+        return std::nullopt;
+    }
+
+    return stats;
+}
+
+std::optional<CCoinsStats> LookupUTXOStatsWithIndex(CoinStatsIndex& coin_stats_index, CCoinsView* view, BlockManager& blockman)
+{
+    CBlockIndex* pindex = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(view->GetBestBlock()));
+
+    return LookupUTXOStatsWithIndex(coin_stats_index, pindex);
+}
+
+std::optional<CCoinsStats> GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function<void()>& interruption_point, const CBlockIndex* pindex, bool index_requested)
+{
+    // Use CoinStatsIndex if it is requested and available and a hash_type of Muhash or None was requested
+    if ((hash_type == CoinStatsHashType::MUHASH || hash_type == CoinStatsHashType::NONE) && g_coin_stats_index && index_requested) {
+        if (pindex) {
+            return LookupUTXOStatsWithIndex(*g_coin_stats_index, pindex);
+        } else {
+            return LookupUTXOStatsWithIndex(*g_coin_stats_index, view, blockman);
+        }
+    }
+
+    // If the coinstats index isn't requested or is otherwise not usable, the
+    // pindex should either be null or equal to the view's best block. This is
+    // because without the coinstats index we can only get coinstats about the
+    // best block.
+    assert(!pindex || pindex->GetBlockHash() == view->GetBestBlock());
+
+    return ComputeUTXOStats(hash_type, view, blockman, interruption_point);
+}
 } // namespace node

src/node/coinstats.h

@@ -64,6 +64,9 @@ struct CCoinsStats {
     CAmount total_unspendables_scripts{0};
     //! Total cumulative amount of coins lost due to unclaimed miner rewards up to and including this block
     CAmount total_unspendables_unclaimed_rewards{0};
+
+    CCoinsStats() = default;
+    CCoinsStats(int block_height, const uint256& block_hash);
 };
 
 /**