diff options
author | laanwj <126646+laanwj@users.noreply.github.com> | 2022-05-24 13:55:57 +0200 |
---|---|---|
committer | laanwj <126646+laanwj@users.noreply.github.com> | 2022-05-24 14:43:00 +0200 |
commit | 700808754884919916a5518e7ecfdabadef956d8 (patch) | |
tree | fbcd2f27b0d5be31741d1d9e3fdd15966a8afdbe | |
parent | 88989063705b122a8c119f0f7023cb0db14c89a2 (diff) | |
parent | 664a14ba7ccb40aa82d35a59831acd35db1897a6 (diff) |
Merge bitcoin/bitcoin#24410: [kernel 2a/n] Split hashing/index `GetUTXOStats` codepaths, decouple from `coinstatsindex`
664a14ba7ccb40aa82d35a59831acd35db1897a6 coinstats: Move GetUTXOStats to rpc/blockchain (Carl Dong)
f1006875665ffe8ff5da8185effe25b860743b4e kernel: Use ComputeUTXOStats in validation (Carl Dong)
faa52387e8e4856445b1cfc9b5e072ce8f690f36 style-only: Rearrange using decls after scripted-diff (Carl Dong)
f329a9298c06ffe74b9e9fbc07bfe6d282fef9cb scripted-diff: Move src/kernel/coinstats to kernel:: (Carl Dong)
0e54456f0498e52131f8ae0c76b4dfe25f45b076 Use only kernel/coinstats.h in index/coinstatsindex.h (Carl Dong)
80970985c965f79b8c376c8a922497e385445dd8 coinstats: Split node/coinstats.h to kernel/coinstats.h (Carl Dong)
35f73ce4b2efd7341fe55f77b334f27ad8aad090 coinstats: Move hasher codepath to kernel/coinstats (Carl Dong)
b7634fe02b6b030f5d62502c73db84ba9a276640 Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats (Carl Dong)
1352e410a5b84070279ff28399083cb3ab278593 coinstats: Separate hasher/index lookup codepaths (Carl Dong)
524463daf6a10b20a4e20116a68101a684929eda coinstats: Return purely out-param CCoinsStats (Carl Dong)
46eb9fc56a296a2acea10ec7e5bf7b1827f73c45 coinstats: Extract index_requested in-member to in-param (Carl Dong)
a789f3f2b878e1236f8e043a8bb1ffb1afc1b673 coinstats: Extract hash_type in-member to in-param (Carl Dong)
102294898d708b7adc0150aba8e500a4aa19bc1c includes: Remove rpc/util.h -> node/coinstats.h (Carl Dong)
0848db9c35d9eae4d68cbdbef68c337656f3c906 fuzz: Remove useless GetUTXOStats fuzz case (Carl Dong)
52b1939993771d0a8a718ca1667241872de8241a 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 664a14ba7ccb40aa82d35a59831acd35db1897a6
Tree-SHA512: 18722c7bd279174d2d1881fec33ea04a9b261aae1c12e998cf434ef297d8ded47de69c526c8033a2ba7abc93ba3d2ff5faf4ce05e8888c725c31cf885ce3ef73
-rw-r--r-- | src/Makefile.am | 10 | ||||
-rw-r--r-- | src/index/coinstatsindex.cpp | 44 | ||||
-rw-r--r-- | src/index/coinstatsindex.h | 4 | ||||
-rw-r--r-- | src/kernel/coinstats.cpp (renamed from src/node/coinstats.cpp) | 70 | ||||
-rw-r--r-- | src/kernel/coinstats.h (renamed from src/node/coinstats.h) | 26 | ||||
-rw-r--r-- | src/rpc/blockchain.cpp | 71 | ||||
-rw-r--r-- | src/rpc/util.h | 1 | ||||
-rw-r--r-- | src/test/coinstatsindex_tests.cpp | 14 | ||||
-rw-r--r-- | src/test/fuzz/coins_view.cpp | 15 | ||||
-rw-r--r-- | src/validation.cpp | 20 | ||||
-rwxr-xr-x | test/lint/lint-circular-dependencies.py | 3 |
11 files changed, 146 insertions, 132 deletions
diff --git a/src/Makefile.am b/src/Makefile.am index a0e793e42a..7f82d188f0 100644 --- a/src/Makefile.am +++ b/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 \ diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 69078708f9..687e330fe0 100644 --- a/src/index/coinstatsindex.cpp +++ b/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() diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index 6f53bb74fb..cae052d913 100644 --- a/src/index/coinstatsindex.h +++ b/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 @@ public: 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. diff --git a/src/node/coinstats.cpp b/src/kernel/coinstats.cpp index eed43a1bc7..f380871627 100644 --- a/src/node/coinstats.cpp +++ b/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 diff --git a/src/node/coinstats.h b/src/kernel/coinstats.h index aa771b18b0..a15957233f 100644 --- a/src/node/coinstats.h +++ b/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,7 +19,7 @@ namespace node { class BlockManager; } // namespace node -namespace node { +namespace kernel { enum class CoinStatsHashType { HASH_SERIALIZED, MUHASH, @@ -28,9 +27,6 @@ enum class CoinStatsHashType { }; 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 diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 0a9458c276..03354f39f6 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -19,11 +19,11 @@ #include <hash.h> #include <index/blockfilterindex.h> #include <index/coinstatsindex.h> +#include <kernel/coinstats.h> #include <logging/timer.h> #include <net.h> #include <net_processing.h> #include <node/blockstorage.h> -#include <node/coinstats.h> #include <node/context.h> #include <node/utxo_snapshot.h> #include <primitives/transaction.h> @@ -51,10 +51,10 @@ #include <memory> #include <mutex> +using kernel::CCoinsStats; +using kernel::CoinStatsHashType; + using node::BlockManager; -using node::CCoinsStats; -using node::CoinStatsHashType; -using node::GetUTXOStats; using node::NodeContext; using node::ReadBlockFromDisk; using node::SnapshotMetadata; @@ -808,6 +808,36 @@ CoinStatsHashType ParseHashType(const std::string& hash_type_input) } } +/** + * Calculate statistics about the unspent transaction output set + * + * @param[in] index_requested Signals if the coinstatsindex should be used (when available). + */ +static std::optional<kernel::CCoinsStats> GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, + kernel::CoinStatsHashType hash_type, + const std::function<void()>& interruption_point = {}, + const CBlockIndex* pindex = nullptr, + bool index_requested = true) +{ + // Use CoinStatsIndex if it is requested and available and a hash_type of Muhash or None was requested + if ((hash_type == kernel::CoinStatsHashType::MUHASH || hash_type == kernel::CoinStatsHashType::NONE) && g_coin_stats_index && index_requested) { + if (pindex) { + return g_coin_stats_index->LookUpStats(pindex); + } else { + CBlockIndex* block_index = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(view->GetBestBlock())); + return g_coin_stats_index->LookUpStats(block_index); + } + } + + // 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. + CHECK_NONFATAL(!pindex || pindex->GetBlockHash() == view->GetBestBlock()); + + return kernel::ComputeUTXOStats(hash_type, view, blockman, interruption_point); +} + static RPCHelpMan gettxoutsetinfo() { return RPCHelpMan{"gettxoutsetinfo", @@ -862,8 +892,7 @@ static RPCHelpMan gettxoutsetinfo() const CBlockIndex* pindex{nullptr}; const CoinStatsHashType hash_type{request.params[0].isNull() ? CoinStatsHashType::HASH_SERIALIZED : ParseHashType(request.params[0].get_str())}; - CCoinsStats stats{hash_type}; - stats.index_requested = request.params[2].isNull() || request.params[2].get_bool(); + bool index_requested = request.params[2].isNull() || request.params[2].get_bool(); NodeContext& node = EnsureAnyNodeContext(request.context); ChainstateManager& chainman = EnsureChainman(node); @@ -884,14 +913,14 @@ static RPCHelpMan gettxoutsetinfo() throw JSONRPCError(RPC_INVALID_PARAMETER, "Querying specific block heights requires coinstatsindex"); } - if (stats.m_hash_type == CoinStatsHashType::HASH_SERIALIZED) { + if (hash_type == CoinStatsHashType::HASH_SERIALIZED) { throw JSONRPCError(RPC_INVALID_PARAMETER, "hash_serialized_2 hash type cannot be queried for a specific block"); } pindex = ParseHashOrHeight(request.params[1], chainman); } - if (stats.index_requested && g_coin_stats_index) { + if (index_requested && g_coin_stats_index) { if (!g_coin_stats_index->BlockUntilSyncedToCurrentChain()) { const IndexSummary summary{g_coin_stats_index->GetSummary()}; @@ -903,7 +932,9 @@ static RPCHelpMan gettxoutsetinfo() } } - if (GetUTXOStats(coins_view, *blockman, stats, node.rpc_interruption_point, pindex)) { + const std::optional<CCoinsStats> maybe_stats = GetUTXOStats(coins_view, *blockman, hash_type, node.rpc_interruption_point, pindex, index_requested); + if (maybe_stats.has_value()) { + const CCoinsStats& stats = maybe_stats.value(); ret.pushKV("height", (int64_t)stats.nHeight); ret.pushKV("bestblock", stats.hashBlock.GetHex()); ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs); @@ -922,10 +953,13 @@ static RPCHelpMan gettxoutsetinfo() } else { ret.pushKV("total_unspendable_amount", ValueFromAmount(stats.total_unspendable_amount)); - CCoinsStats prev_stats{hash_type}; - + CCoinsStats prev_stats{}; if (pindex->nHeight > 0) { - GetUTXOStats(coins_view, *blockman, prev_stats, node.rpc_interruption_point, pindex->pprev); + const std::optional<CCoinsStats> maybe_prev_stats = GetUTXOStats(coins_view, *blockman, hash_type, node.rpc_interruption_point, pindex->pprev, index_requested); + if (!maybe_prev_stats) { + throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set"); + } + prev_stats = maybe_prev_stats.value(); } UniValue block_info(UniValue::VOBJ); @@ -2285,7 +2319,7 @@ UniValue CreateUTXOSnapshot( const fs::path& temppath) { std::unique_ptr<CCoinsViewCursor> pcursor; - CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED}; + std::optional<CCoinsStats> maybe_stats; const CBlockIndex* tip; { @@ -2305,19 +2339,20 @@ UniValue CreateUTXOSnapshot( chainstate.ForceFlushStateToDisk(); - if (!GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, stats, node.rpc_interruption_point)) { + maybe_stats = GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, CoinStatsHashType::HASH_SERIALIZED, node.rpc_interruption_point); + if (!maybe_stats) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set"); } pcursor = chainstate.CoinsDB().Cursor(); - tip = CHECK_NONFATAL(chainstate.m_blockman.LookupBlockIndex(stats.hashBlock)); + tip = CHECK_NONFATAL(chainstate.m_blockman.LookupBlockIndex(maybe_stats->hashBlock)); } LOG_TIME_SECONDS(strprintf("writing UTXO snapshot at height %s (%s) to file %s (via %s)", tip->nHeight, tip->GetBlockHash().ToString(), fs::PathToString(path), fs::PathToString(temppath))); - SnapshotMetadata metadata{tip->GetBlockHash(), stats.coins_count, tip->nChainTx}; + SnapshotMetadata metadata{tip->GetBlockHash(), maybe_stats->coins_count, tip->nChainTx}; afile << metadata; @@ -2339,11 +2374,11 @@ UniValue CreateUTXOSnapshot( afile.fclose(); UniValue result(UniValue::VOBJ); - result.pushKV("coins_written", stats.coins_count); + result.pushKV("coins_written", maybe_stats->coins_count); result.pushKV("base_hash", tip->GetBlockHash().ToString()); result.pushKV("base_height", tip->nHeight); result.pushKV("path", path.u8string()); - result.pushKV("txoutset_hash", stats.hashSerialized.ToString()); + result.pushKV("txoutset_hash", maybe_stats->hashSerialized.ToString()); // Cast required because univalue doesn't have serialization specified for // `unsigned int`, nChainTx's type. result.pushKV("nchaintx", uint64_t{tip->nChainTx}); diff --git a/src/rpc/util.h b/src/rpc/util.h index abbc4c66fe..e883dc008e 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -5,7 +5,6 @@ #ifndef BITCOIN_RPC_UTIL_H #define BITCOIN_RPC_UTIL_H -#include <node/coinstats.h> #include <node/transaction.h> #include <outputtype.h> #include <protocol.h> diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 5b73481bc1..50eb479035 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/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); diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 360dc00307..6c96702f1e 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/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{}; @@ -270,16 +265,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); }); } diff --git a/src/validation.cpp b/src/validation.cpp index a54ec8269e..44e908152c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -18,10 +18,10 @@ #include <cuckoocache.h> #include <flatfile.h> #include <hash.h> +#include <kernel/coinstats.h> #include <logging.h> #include <logging/timer.h> #include <node/blockstorage.h> -#include <node/coinstats.h> #include <node/ui_interface.h> #include <node/utxo_snapshot.h> #include <policy/policy.h> @@ -58,17 +58,18 @@ #include <optional> #include <string> +using kernel::CCoinsStats; +using kernel::CoinStatsHashType; +using kernel::ComputeUTXOStats; + using node::BLOCKFILE_CHUNK_SIZE; using node::BlockManager; using node::BlockMap; using node::CBlockIndexHeightOnlyComparator; using node::CBlockIndexWorkComparator; -using node::CCoinsStats; -using node::CoinStatsHashType; using node::fImporting; using node::fPruneMode; using node::fReindex; -using node::GetUTXOStats; using node::nPruneTarget; using node::OpenBlockFile; using node::ReadBlockFromDisk; @@ -4987,7 +4988,8 @@ bool ChainstateManager::PopulateAndValidateSnapshot( CBlockIndex* snapshot_start_block = WITH_LOCK(::cs_main, return m_blockman.LookupBlockIndex(base_blockhash)); if (!snapshot_start_block) { - // Needed for GetUTXOStats and ExpectedAssumeutxo to determine the height and to avoid a crash when base_blockhash.IsNull() + // Needed for ComputeUTXOStats and ExpectedAssumeutxo to determine the + // height and to avoid a crash when base_blockhash.IsNull() LogPrintf("[snapshot] Did not find snapshot start blockheader %s\n", base_blockhash.ToString()); return false; @@ -5095,22 +5097,22 @@ bool ChainstateManager::PopulateAndValidateSnapshot( assert(coins_cache.GetBestBlock() == base_blockhash); - CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED}; auto breakpoint_fnc = [] { /* TODO insert breakpoint here? */ }; // As above, okay to immediately release cs_main here since no other context knows // about the snapshot_chainstate. CCoinsViewDB* snapshot_coinsdb = WITH_LOCK(::cs_main, return &snapshot_chainstate.CoinsDB()); - if (!GetUTXOStats(snapshot_coinsdb, m_blockman, stats, breakpoint_fnc)) { + const std::optional<CCoinsStats> maybe_stats = ComputeUTXOStats(CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, breakpoint_fnc); + if (!maybe_stats.has_value()) { LogPrintf("[snapshot] failed to generate coins stats\n"); return false; } // Assert that the deserialized chainstate contents match the expected assumeutxo value. - if (AssumeutxoHash{stats.hashSerialized} != au_data.hash_serialized) { + if (AssumeutxoHash{maybe_stats->hashSerialized} != au_data.hash_serialized) { LogPrintf("[snapshot] bad snapshot content hash: expected %s, got %s\n", - au_data.hash_serialized.ToString(), stats.hashSerialized.ToString()); + au_data.hash_serialized.ToString(), maybe_stats->hashSerialized.ToString()); return false; } diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index 7ca2ec994b..5d157eb4b1 100755 --- a/test/lint/lint-circular-dependencies.py +++ b/test/lint/lint-circular-dependencies.py @@ -14,7 +14,6 @@ import sys EXPECTED_CIRCULAR_DEPENDENCIES = ( "chainparamsbase -> util/system -> chainparamsbase", "node/blockstorage -> validation -> node/blockstorage", - "index/coinstatsindex -> node/coinstats -> index/coinstatsindex", "policy/fees -> txmempool -> policy/fees", "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel", "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel", @@ -22,7 +21,7 @@ EXPECTED_CIRCULAR_DEPENDENCIES = ( "qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel", "wallet/fees -> wallet/wallet -> wallet/fees", "wallet/wallet -> wallet/walletdb -> wallet/wallet", - "node/coinstats -> validation -> node/coinstats", + "kernel/coinstats -> validation -> kernel/coinstats", ) CODE_DIR = "src" |