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

664a14ba7c coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f100687566 kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f04 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c9 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6 coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b8 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993 kernel: Remove unnecessary blockfilter{index,}.cpp (Carl Dong)

Pull request description:

  Part of: #24303
  Depends on: #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 664a14ba7c

Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73

src/test/coinstatsindex_tests.cpp

@@ -13,8 +13,8 @@
 
 #include <chrono>
 
-using node::CCoinsStats;
-using node::CoinStatsHashType;
+using kernel::CCoinsStats;
+using kernel::CoinStatsHashType;
 
 BOOST_AUTO_TEST_SUITE(coinstatsindex_tests)
 
@@ -33,7 +33,6 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
 {
     CoinStatsIndex coin_stats_index{1 << 20, true};
 
-    CCoinsStats coin_stats{CoinStatsHashType::MUHASH};
     const CBlockIndex* block_index;
     {
         LOCK(cs_main);
@@ -41,7 +40,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
     }
 
     // CoinStatsIndex should not be found before it is started.
-    BOOST_CHECK(!coin_stats_index.LookUpStats(block_index, coin_stats));
+    BOOST_CHECK(!coin_stats_index.LookUpStats(block_index));
 
     // BlockUntilSyncedToCurrentChain should return false before CoinStatsIndex
     // is started.
@@ -57,10 +56,10 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
         LOCK(cs_main);
         genesis_block_index = m_node.chainman->ActiveChain().Genesis();
     }
-    BOOST_CHECK(coin_stats_index.LookUpStats(genesis_block_index, coin_stats));
+    BOOST_CHECK(coin_stats_index.LookUpStats(genesis_block_index));
 
     // Check that CoinStatsIndex updates with new blocks.
-    coin_stats_index.LookUpStats(block_index, coin_stats);
+    BOOST_CHECK(coin_stats_index.LookUpStats(block_index));
 
     const CScript script_pub_key{CScript() << ToByteVector(coinbaseKey.GetPubKey()) << OP_CHECKSIG};
     std::vector<CMutableTransaction> noTxns;
@@ -69,13 +68,12 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
     // Let the CoinStatsIndex to catch up again.
     BOOST_CHECK(coin_stats_index.BlockUntilSyncedToCurrentChain());
 
-    CCoinsStats new_coin_stats{CoinStatsHashType::MUHASH};
     const CBlockIndex* new_block_index;
     {
         LOCK(cs_main);
         new_block_index = m_node.chainman->ActiveChain().Tip();
     }
-    coin_stats_index.LookUpStats(new_block_index, new_coin_stats);
+    BOOST_CHECK(coin_stats_index.LookUpStats(new_block_index));
 
     BOOST_CHECK(block_index != new_block_index);
 

src/test/fuzz/coins_view.cpp

@@ -10,7 +10,6 @@
 #include <consensus/tx_verify.h>
 #include <consensus/validation.h>
 #include <key.h>
-#include <node/coinstats.h>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <pubkey.h>
@@ -26,10 +25,6 @@
 #include <string>
 #include <vector>
 
-using node::CCoinsStats;
-using node::CoinStatsHashType;
-using node::GetUTXOStats;
-
 namespace {
 const TestingSetup* g_setup;
 const Coin EMPTY_COIN{};
@@ -269,16 +264,6 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
                 }
                 (void)GetTransactionSigOpCost(transaction, coins_view_cache, flags);
             },
-            [&] {
-                CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED};
-                bool expected_code_path = false;
-                try {
-                    (void)GetUTXOStats(&coins_view_cache, g_setup->m_node.chainman->m_blockman, stats);
-                } catch (const std::logic_error&) {
-                    expected_code_path = true;
-                }
-                assert(expected_code_path);
-            },
             [&] {
                 (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache);
             });