Merge bitcoin#24410: [kernel 2a/n] Split hashing/index GetUTXOStats codepaths, decouple from coinstatsindex

664a14b coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f100687 kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa5238 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a92 scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
8097098 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e41 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463d coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f coinstats: Extract hash_type in-member to in-param (Carl Dong)
1022948 includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939 kernel: Remove unnecessary blockfilter{index,}.cpp (Carl Dong)

Pull request description:

  Part of: bitcoin#24303
  Depends on: bitcoin#24322

  The `GetUTXOStats` function has 2 codepaths:
    - One which queries the `CoinStatsIndex` for the UTXO hash
    - One which actually performs the hashing

  For `libbitcoinkernel`, the only place where we call `GetUTXOStats` is in `PopulateAndValidateSnapshots`, which uses the `SHA256D` hash, and is therefore unable to use the `CoinStatsIndex` since that only provides `MuHash` hashes. Not that I think indices necessarily belong in `libbitcoinkernel` anyway.

  This PR separates these 2 aforementioned codepaths of `GetUTXOStats`, uses the hashing codepath in `PopulateAndValidateSnapshots`, and removes the need to link in `index/coinstatsindex.cpp` and `node/coinstats.cpp`.

  -----

  Logistically, this PR:
  - Extracts out the `index_requested` and `hash_type` members of `CoinStats`, which served as "in-params" to `GetUTXOStats` embedded within the `CoinStats` struct. This allows `CoinStats` to only consist of "out-param" members, and be returned by `GetUTXOStats` without needing to be an "in-out" param
  - Introduce the purely virtual `UTXOHashers` class, with 3 implementations: `SHA256DHasher`, `MuHashHasher`, and `NullHasher`. These replace the existing template-based polymorphism.
  - Split `GetUTXOStats` into:
      - `CalculateUTXOStatsWithHasher(UTXOHasher&, ...)`, and
      - `LookupUTXOStatsWithIndex(CoinStatsIndex&, ...)`
  - Use `CalculateUTXOStatsWithHasher` directly where appropriate (`src/validation.cpp` and `src/fuzz`)
  - Move `GetUTXOStats` to `rpc/blockchain`, which is the only place that depends on `GetUTXOStats`'s weird fallback behaviour
  - Move `LookupUTXOStatsWithIndex` to `index/coinstatsindex`

  Code organization:
  - `src/`
    - `kernel/` → only contains the hashing codepath
      - `coinstats.cpp` → hashing codepath implementations
      - `coinstats.h` → header for `kernel/coinstats.cpp`
    - `index/` → only contains the index codepath
      - `coinstatsindex.cpp` → index codepath implementations
      - `coinstatsindex.h`
    - `validation.cpp` → only uses the hashing codepath
    - `rpc/blockchain.cpp` → uses both the hashing and index codepath, old `GetUTXOStats` fallback logic moved here as static
    - `test/fuzz/coins_view.cpp` → only uses the hashing codepath

  TODOs:
  - [x] Commit messages could be fleshed out more

  Would love any feedback!

ACKs for top commit:
  laanwj:
    Code review ACK 664a14b

Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73

Author: laanwj

src/Makefile.am

@@ -172,6 +172,7 @@ BITCOIN_CORE_H = \
   interfaces/node.h \
   interfaces/wallet.h \
   kernel/chainstatemanager_opts.h \
+  kernel/coinstats.h \
   key.h \
   key_io.h \
   logging.h \
@@ -191,7 +192,6 @@ BITCOIN_CORE_H = \
   node/caches.h \
   node/chainstate.h \
   node/coin.h \
-  node/coinstats.h \
   node/context.h \
   node/miner.h \
   node/minisketchwrapper.h \
@@ -357,6 +357,7 @@ libbitcoin_node_a_SOURCES = \
   index/coinstatsindex.cpp \
   index/txindex.cpp \
   init.cpp \
+  kernel/coinstats.cpp \
   mapport.cpp \
   net.cpp \
   netgroup.cpp \
@@ -365,7 +366,6 @@ libbitcoin_node_a_SOURCES = \
   node/caches.cpp \
   node/chainstate.cpp \
   node/coin.cpp \
-  node/coinstats.cpp \
   node/context.cpp \
   node/interfaces.cpp \
   node/miner.cpp \
@@ -853,7 +853,6 @@ endif
 libbitcoinkernel_la_SOURCES = \
   kernel/bitcoinkernel.cpp \
   arith_uint256.cpp \
-  blockfilter.cpp \
   chain.cpp \
   chainparamsbase.cpp \
   chainparams.cpp \
@@ -871,15 +870,12 @@ libbitcoinkernel_la_SOURCES = \
   flatfile.cpp \
   fs.cpp \
   hash.cpp \
-  index/base.cpp \
-  index/blockfilterindex.cpp \
-  index/coinstatsindex.cpp \
   init/common.cpp \
+  kernel/coinstats.cpp \
   key.cpp \
   logging.cpp \
   node/blockstorage.cpp \
   node/chainstate.cpp \
-  node/coinstats.cpp \
   node/ui_interface.cpp \
   policy/feerate.cpp \
   policy/fees.cpp \

src/index/coinstatsindex.cpp

@@ -12,10 +12,11 @@
 #include <undo.h>
 #include <validation.h>
 
-using node::CCoinsStats;
-using node::GetBogoSize;
+using kernel::CCoinsStats;
+using kernel::GetBogoSize;
+using kernel::TxOutSer;
+
 using node::ReadBlockFromDisk;
-using node::TxOutSer;
 using node::UndoReadFromDisk;
 
 static constexpr uint8_t DB_BLOCK_HASH{'s'};
@@ -316,28 +317,31 @@ static bool LookUpOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVa
     return db.Read(DBHashKey(block_index->GetBlockHash()), result);
 }
 
-bool CoinStatsIndex::LookUpStats(const CBlockIndex* block_index, CCoinsStats& coins_stats) const
+std::optional<CCoinsStats> CoinStatsIndex::LookUpStats(const CBlockIndex* block_index) const
 {
+    CCoinsStats stats{Assert(block_index)->nHeight, block_index->GetBlockHash()};
+    stats.index_used = true;
+
     DBVal entry;
     if (!LookUpOne(*m_db, block_index, entry)) {
-        return false;
+        return std::nullopt;
     }
 
-    coins_stats.hashSerialized = entry.muhash;
-    coins_stats.nTransactionOutputs = entry.transaction_output_count;
-    coins_stats.nBogoSize = entry.bogo_size;
-    coins_stats.total_amount = entry.total_amount;
-    coins_stats.total_subsidy = entry.total_subsidy;
-    coins_stats.total_unspendable_amount = entry.total_unspendable_amount;
-    coins_stats.total_prevout_spent_amount = entry.total_prevout_spent_amount;
-    coins_stats.total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount;
-    coins_stats.total_coinbase_amount = entry.total_coinbase_amount;
-    coins_stats.total_unspendables_genesis_block = entry.total_unspendables_genesis_block;
-    coins_stats.total_unspendables_bip30 = entry.total_unspendables_bip30;
-    coins_stats.total_unspendables_scripts = entry.total_unspendables_scripts;
-    coins_stats.total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards;
-
-    return true;
+    stats.hashSerialized = entry.muhash;
+    stats.nTransactionOutputs = entry.transaction_output_count;
+    stats.nBogoSize = entry.bogo_size;
+    stats.total_amount = entry.total_amount;
+    stats.total_subsidy = entry.total_subsidy;
+    stats.total_unspendable_amount = entry.total_unspendable_amount;
+    stats.total_prevout_spent_amount = entry.total_prevout_spent_amount;
+    stats.total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount;
+    stats.total_coinbase_amount = entry.total_coinbase_amount;
+    stats.total_unspendables_genesis_block = entry.total_unspendables_genesis_block;
+    stats.total_unspendables_bip30 = entry.total_unspendables_bip30;
+    stats.total_unspendables_scripts = entry.total_unspendables_scripts;
+    stats.total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards;
+
+    return stats;
 }
 
 bool CoinStatsIndex::Init()

src/index/coinstatsindex.h

@@ -9,7 +9,7 @@
 #include <crypto/muhash.h>
 #include <flatfile.h>
 #include <index/base.h>
-#include <node/coinstats.h>
+#include <kernel/coinstats.h>
 
 /**
  * CoinStatsIndex maintains statistics on the UTXO set.
@@ -56,7 +56,7 @@ class CoinStatsIndex final : public BaseIndex
     explicit CoinStatsIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
 
     // Look up stats for a specific block using CBlockIndex
-    bool LookUpStats(const CBlockIndex* block_index, node::CCoinsStats& coins_stats) const;
+    std::optional<kernel::CCoinsStats> LookUpStats(const CBlockIndex* block_index) const;
 };
 
 /// The global UTXO set hash object.

src/node/coinstats.cpp renamed to src/kernel/coinstats.cpp

@@ -1,14 +1,12 @@
-// Copyright (c) 2010 Satoshi Nakamoto
-// Copyright (c) 2009-2021 The Bitcoin Core developers
+// Copyright (c) 2022 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 <node/coinstats.h>
+#include <kernel/coinstats.h>
 
 #include <coins.h>
 #include <crypto/muhash.h>
 #include <hash.h>
-#include <index/coinstatsindex.h>
 #include <serialize.h>
 #include <uint256.h>
 #include <util/overflow.h>
@@ -17,7 +15,12 @@
 
 #include <map>
 
-namespace node {
+namespace kernel {
+
+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)
 {
@@ -93,24 +96,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)
+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 ((stats.m_hash_type == CoinStatsHashType::MUHASH || stats.m_hash_type == CoinStatsHashType::NONE) && g_coin_stats_index && stats.index_requested) {
-        stats.index_used = true;
-        return g_coin_stats_index->LookUpStats(pindex, stats);
-    }
-
     PrepareHash(hash_obj, stats);
 
     uint256 prevkey;
@@ -141,25 +131,36 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats&
     FinalizeHash(hash_obj, stats);
 
     stats.nDiskSize = view->EstimateSize();
+
     return true;
 }
 
-bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, const std::function<void()>& interruption_point, const CBlockIndex* pindex)
+std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point)
 {
-    switch (stats.m_hash_type) {
-    case(CoinStatsHashType::HASH_SERIALIZED): {
-        CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION);
-        return GetUTXOStats(view, blockman, stats, ss, interruption_point, pindex);
-    }
-    case(CoinStatsHashType::MUHASH): {
-        MuHash3072 muhash;
-        return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex);
-    }
-    case(CoinStatsHashType::NONE): {
-        return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex);
+    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 ComputeUTXOStats(view, stats, ss, interruption_point);
+        }
+        case(CoinStatsHashType::MUHASH): {
+            MuHash3072 muhash;
+            return ComputeUTXOStats(view, stats, muhash, interruption_point);
+        }
+        case(CoinStatsHashType::NONE): {
+            return ComputeUTXOStats(view, stats, nullptr, interruption_point);
+        }
+        } // no default case, so the compiler can warn about missing cases
+        assert(false);
+    }();
+
+    if (!success) {
+        return std::nullopt;
     }
-    } // no default case, so the compiler can warn about missing cases
-    assert(false);
+    return stats;
 }
 
 // The legacy hash serializes the hashBlock
@@ -182,4 +183,5 @@ static void FinalizeHash(MuHash3072& muhash, CCoinsStats& stats)
     stats.hashSerialized = out;
 }
 static void FinalizeHash(std::nullptr_t, CCoinsStats& stats) {}
-} // namespace node
+
+} // namespace kernel

src/node/coinstats.h renamed to src/kernel/coinstats.h

@@ -1,10 +1,9 @@
-// Copyright (c) 2010 Satoshi Nakamoto
-// Copyright (c) 2009-2021 The Bitcoin Core developers
+// Copyright (c) 2022 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_NODE_COINSTATS_H
-#define BITCOIN_NODE_COINSTATS_H
+#ifndef BITCOIN_KERNEL_COINSTATS_H
+#define BITCOIN_KERNEL_COINSTATS_H
 
 #include <chain.h>
 #include <coins.h>
@@ -20,17 +19,14 @@ namespace node {
 class BlockManager;
 } // namespace node
 
-namespace node {
+namespace kernel {
 enum class CoinStatsHashType {
     HASH_SERIALIZED,
     MUHASH,
     NONE,
 };
 
 struct CCoinsStats {
-    //! Which hash type to use
-    const CoinStatsHashType m_hash_type;
-
     int nHeight{0};
     uint256 hashBlock{};
     uint64_t nTransactions{0};
@@ -44,8 +40,6 @@ struct CCoinsStats {
     //! The number of coins contained.
     uint64_t coins_count{0};
 
-    //! Signals if the coinstatsindex should be used (when available).
-    bool index_requested{true};
     //! Signals if the coinstatsindex was used to retrieve the statistics.
     bool index_used{false};
 
@@ -70,15 +64,15 @@ struct CCoinsStats {
     //! Total cumulative amount of coins lost due to unclaimed miner rewards up to and including this block
     CAmount total_unspendables_unclaimed_rewards{0};
 
-    CCoinsStats(CoinStatsHashType hash_type) : m_hash_type(hash_type) {}
+    CCoinsStats() = default;
+    CCoinsStats(int block_height, const uint256& block_hash);
 };
 
-//! Calculate statistics about the unspent transaction output set
-bool GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, CCoinsStats& stats, const std::function<void()>& interruption_point = {}, const CBlockIndex* pindex = nullptr);
-
 uint64_t GetBogoSize(const CScript& script_pub_key);
 
 CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin);
-} // namespace node
 
-#endif // BITCOIN_NODE_COINSTATS_H
+std::optional<CCoinsStats> ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function<void()>& interruption_point = {});
+} // namespace kernel
+
+#endif // BITCOIN_KERNEL_COINSTATS_H