From 52b1939993771d0a8a718ca1667241872de8241a Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 3 May 2022 18:14:03 -0400 Subject: [PATCH 01/15] kernel: Remove unnecessary blockfilter{index,}.cpp It is no longer necessary to link in blockfilter.cpp and index/blockfilterindex.cpp after merge of PR#21726 since validation has been decouple from the blockfilterindex. --- src/Makefile.am | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index a0e793e42a0..0fd15baed1c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -853,7 +853,6 @@ endif libbitcoinkernel_la_SOURCES = \ kernel/bitcoinkernel.cpp \ arith_uint256.cpp \ - blockfilter.cpp \ chain.cpp \ chainparamsbase.cpp \ chainparams.cpp \ @@ -872,7 +871,6 @@ libbitcoinkernel_la_SOURCES = \ fs.cpp \ hash.cpp \ index/base.cpp \ - index/blockfilterindex.cpp \ index/coinstatsindex.cpp \ init/common.cpp \ key.cpp \ From 0848db9c35d9eae4d68cbdbef68c337656f3c906 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Fri, 29 Apr 2022 13:30:27 -0400 Subject: [PATCH 02/15] fuzz: Remove useless GetUTXOStats fuzz case In the GetUTXOStats fuzz case, GetUTXOStats is always called with a CCoinsViewCache. Which is guaranteed to throw a std::logic_error when its ::Cursor() method is called on the first line of GetUTXOStats. In the fuzz case, we basically catch this logic error and declare victory if we caught it. There is no point to fuzzing this deterministic logic. Confirmed with IWYU that the node/coinstats.h #include is no longer necessary. --- src/test/fuzz/coins_view.cpp | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/test/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp index 360dc003072..6c96702f1e7 100644 --- a/src/test/fuzz/coins_view.cpp +++ b/src/test/fuzz/coins_view.cpp @@ -10,7 +10,6 @@ #include #include #include -#include #include #include #include @@ -26,10 +25,6 @@ #include #include -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); }); From 102294898d708b7adc0150aba8e500a4aa19bc1c Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 2 May 2022 14:27:38 -0400 Subject: [PATCH 03/15] includes: Remove rpc/util.h -> node/coinstats.h Confirmed with IWYU that this is unnecessary. --- src/rpc/util.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/rpc/util.h b/src/rpc/util.h index abbc4c66fef..e883dc008e3 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 #include #include #include From a789f3f2b878e1236f8e043a8bb1ffb1afc1b673 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Sun, 13 Feb 2022 19:27:36 -0500 Subject: [PATCH 04/15] coinstats: Extract hash_type in-member to in-param Currently, CCoinsStats is a struct with both in-params and out-params where the hash_type and index_requested members are the only in-params. This change removes CCoinsStats' hash_type in-param member and adds it to the relevant functions instead. [META] In subsequent commits, all of CCoinsStats' members which serve as in-params will be moved out so as to make CCoinsStats a pure out-param struct. --- src/node/coinstats.cpp | 14 +++++++------- src/node/coinstats.h | 7 +------ src/rpc/blockchain.cpp | 14 +++++++------- src/test/coinstatsindex_tests.cpp | 4 ++-- src/validation.cpp | 4 ++-- 5 files changed, 19 insertions(+), 24 deletions(-) diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index eed43a1bc72..8d94e1bd369 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -93,7 +93,7 @@ static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map -static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, T hash_obj, const std::function& interruption_point, const CBlockIndex* pindex) +static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, T hash_obj, const std::function& interruption_point, const CBlockIndex* pindex, CoinStatsHashType& hash_type) { std::unique_ptr pcursor(view->Cursor()); assert(pcursor); @@ -106,7 +106,7 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& 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) { + if ((hash_type == CoinStatsHashType::MUHASH || hash_type == CoinStatsHashType::NONE) && g_coin_stats_index && stats.index_requested) { stats.index_used = true; return g_coin_stats_index->LookUpStats(pindex, stats); } @@ -144,19 +144,19 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& return true; } -bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, const std::function& interruption_point, const CBlockIndex* pindex) +bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex) { - switch (stats.m_hash_type) { + switch (hash_type) { case(CoinStatsHashType::HASH_SERIALIZED): { CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION); - return GetUTXOStats(view, blockman, stats, ss, interruption_point, pindex); + return GetUTXOStats(view, blockman, stats, ss, interruption_point, pindex, hash_type); } case(CoinStatsHashType::MUHASH): { MuHash3072 muhash; - return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex); + return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex, hash_type); } case(CoinStatsHashType::NONE): { - return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex); + return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex, hash_type); } } // no default case, so the compiler can warn about missing cases assert(false); diff --git a/src/node/coinstats.h b/src/node/coinstats.h index aa771b18b06..ee3f3b00306 100644 --- a/src/node/coinstats.h +++ b/src/node/coinstats.h @@ -28,9 +28,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}; @@ -69,12 +66,10 @@ 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(CoinStatsHashType hash_type) : m_hash_type(hash_type) {} }; //! Calculate statistics about the unspent transaction output set -bool GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, CCoinsStats& stats, const std::function& interruption_point = {}, const CBlockIndex* pindex = nullptr); +bool GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function& interruption_point = {}, const CBlockIndex* pindex = nullptr); uint64_t GetBogoSize(const CScript& script_pub_key); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 0a9458c2765..3411f72a2cb 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -862,7 +862,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}; + CCoinsStats stats{}; stats.index_requested = request.params[2].isNull() || request.params[2].get_bool(); NodeContext& node = EnsureAnyNodeContext(request.context); @@ -884,7 +884,7 @@ 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"); } @@ -903,7 +903,7 @@ static RPCHelpMan gettxoutsetinfo() } } - if (GetUTXOStats(coins_view, *blockman, stats, node.rpc_interruption_point, pindex)) { + if (GetUTXOStats(coins_view, *blockman, stats, hash_type, node.rpc_interruption_point, pindex)) { ret.pushKV("height", (int64_t)stats.nHeight); ret.pushKV("bestblock", stats.hashBlock.GetHex()); ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs); @@ -922,10 +922,10 @@ 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); + GetUTXOStats(coins_view, *blockman, prev_stats, hash_type, node.rpc_interruption_point, pindex->pprev); } UniValue block_info(UniValue::VOBJ); @@ -2285,7 +2285,7 @@ UniValue CreateUTXOSnapshot( const fs::path& temppath) { std::unique_ptr pcursor; - CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED}; + CCoinsStats stats{}; const CBlockIndex* tip; { @@ -2305,7 +2305,7 @@ UniValue CreateUTXOSnapshot( chainstate.ForceFlushStateToDisk(); - if (!GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, stats, node.rpc_interruption_point)) { + if (!GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, stats, CoinStatsHashType::HASH_SERIALIZED, node.rpc_interruption_point)) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set"); } diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index 5b73481bc1e..d7d09fa168e 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -33,7 +33,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) { CoinStatsIndex coin_stats_index{1 << 20, true}; - CCoinsStats coin_stats{CoinStatsHashType::MUHASH}; + CCoinsStats coin_stats{}; const CBlockIndex* block_index; { LOCK(cs_main); @@ -69,7 +69,7 @@ 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}; + CCoinsStats new_coin_stats{}; const CBlockIndex* new_block_index; { LOCK(cs_main); diff --git a/src/validation.cpp b/src/validation.cpp index a54ec8269ea..5fdf0398df7 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5095,14 +5095,14 @@ bool ChainstateManager::PopulateAndValidateSnapshot( assert(coins_cache.GetBestBlock() == base_blockhash); - CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED}; + CCoinsStats stats{}; 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)) { + if (!GetUTXOStats(snapshot_coinsdb, m_blockman, stats, CoinStatsHashType::HASH_SERIALIZED, breakpoint_fnc)) { LogPrintf("[snapshot] failed to generate coins stats\n"); return false; } From 46eb9fc56a296a2acea10ec7e5bf7b1827f73c45 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Sun, 13 Feb 2022 19:34:17 -0500 Subject: [PATCH 05/15] coinstats: Extract index_requested in-member to in-param This change removes CCoinsStats' index_requested in-param member and adds it to the relevant functions instead. --- src/node/coinstats.cpp | 12 ++++++------ src/node/coinstats.h | 14 ++++++++++---- src/rpc/blockchain.cpp | 8 ++++---- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 8d94e1bd369..13f1041a516 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -93,7 +93,7 @@ static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map -static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, T hash_obj, const std::function& interruption_point, const CBlockIndex* pindex, CoinStatsHashType& hash_type) +static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, T hash_obj, const std::function& interruption_point, const CBlockIndex* pindex, CoinStatsHashType& hash_type, bool index_requested) { std::unique_ptr pcursor(view->Cursor()); assert(pcursor); @@ -106,7 +106,7 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& 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 && stats.index_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); } @@ -144,19 +144,19 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& return true; } -bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex) +bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex, bool index_requested) { switch (hash_type) { case(CoinStatsHashType::HASH_SERIALIZED): { CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION); - return GetUTXOStats(view, blockman, stats, ss, interruption_point, pindex, hash_type); + return GetUTXOStats(view, blockman, stats, ss, interruption_point, pindex, hash_type, index_requested); } case(CoinStatsHashType::MUHASH): { MuHash3072 muhash; - return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex, hash_type); + return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex, hash_type, index_requested); } case(CoinStatsHashType::NONE): { - return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex, hash_type); + return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex, hash_type, index_requested); } } // no default case, so the compiler can warn about missing cases assert(false); diff --git a/src/node/coinstats.h b/src/node/coinstats.h index ee3f3b00306..adc67eb0a60 100644 --- a/src/node/coinstats.h +++ b/src/node/coinstats.h @@ -41,8 +41,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}; @@ -68,8 +66,16 @@ struct CCoinsStats { CAmount total_unspendables_unclaimed_rewards{0}; }; -//! Calculate statistics about the unspent transaction output set -bool GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function& interruption_point = {}, const CBlockIndex* pindex = nullptr); +/** + * Calculate statistics about the unspent transaction output set + * + * @param[in] index_requested Signals if the coinstatsindex should be used (when available). + */ +bool GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, + CCoinsStats& stats, CoinStatsHashType hash_type, + const std::function& interruption_point = {}, + const CBlockIndex* pindex = nullptr, + bool index_requested = true); uint64_t GetBogoSize(const CScript& script_pub_key); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 3411f72a2cb..98c2deb4bf0 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -863,7 +863,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{}; - 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); @@ -891,7 +891,7 @@ static RPCHelpMan gettxoutsetinfo() 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 +903,7 @@ static RPCHelpMan gettxoutsetinfo() } } - if (GetUTXOStats(coins_view, *blockman, stats, hash_type, node.rpc_interruption_point, pindex)) { + if (GetUTXOStats(coins_view, *blockman, stats, hash_type, node.rpc_interruption_point, pindex, index_requested)) { ret.pushKV("height", (int64_t)stats.nHeight); ret.pushKV("bestblock", stats.hashBlock.GetHex()); ret.pushKV("txouts", (int64_t)stats.nTransactionOutputs); @@ -925,7 +925,7 @@ static RPCHelpMan gettxoutsetinfo() CCoinsStats prev_stats{}; if (pindex->nHeight > 0) { - GetUTXOStats(coins_view, *blockman, prev_stats, hash_type, node.rpc_interruption_point, pindex->pprev); + GetUTXOStats(coins_view, *blockman, prev_stats, hash_type, node.rpc_interruption_point, pindex->pprev, index_requested); } UniValue block_info(UniValue::VOBJ); From 524463daf6a10b20a4e20116a68101a684929eda Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Tue, 15 Feb 2022 18:37:32 -0500 Subject: [PATCH 06/15] coinstats: Return purely out-param CCoinsStats In previous commits in this patchset, we removed all in-param members of CCoinsStats. Now that that's done, we can modify GetUTXOStats to return an optional CCoinsStats instead of a status bool. Callers are modified accordingly. In rpc/blockchain.cpp, we discover that GetUTXOStats' status bool when getting UTXO stats for pprev was not checked for error. We fix this as well. --- src/node/coinstats.cpp | 38 ++++++++++++++++++++++++-------------- src/node/coinstats.h | 10 +++++----- src/rpc/blockchain.cpp | 25 +++++++++++++++---------- src/validation.cpp | 8 ++++---- 4 files changed, 48 insertions(+), 33 deletions(-) diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 13f1041a516..8fc53fe1725 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -144,22 +145,31 @@ static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& return true; } -bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex, bool index_requested) +std::optional GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex, bool index_requested) { - 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); + CCoinsStats stats{}; + + 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); + } + case(CoinStatsHashType::MUHASH): { + MuHash3072 muhash; + return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex, hash_type, index_requested); + } + case(CoinStatsHashType::NONE): { + return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex, hash_type, index_requested); + } + } // no default case, so the compiler can warn about missing cases + assert(false); + }(); + + if (!success) { + return std::nullopt; } - case(CoinStatsHashType::MUHASH): { - MuHash3072 muhash; - return GetUTXOStats(view, blockman, stats, muhash, interruption_point, pindex, hash_type, index_requested); - } - case(CoinStatsHashType::NONE): { - return GetUTXOStats(view, blockman, stats, nullptr, interruption_point, pindex, hash_type, index_requested); - } - } // no default case, so the compiler can warn about missing cases - assert(false); + return stats; } // The legacy hash serializes the hashBlock diff --git a/src/node/coinstats.h b/src/node/coinstats.h index adc67eb0a60..36cbf19f295 100644 --- a/src/node/coinstats.h +++ b/src/node/coinstats.h @@ -71,11 +71,11 @@ struct CCoinsStats { * * @param[in] index_requested Signals if the coinstatsindex should be used (when available). */ -bool GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, - CCoinsStats& stats, CoinStatsHashType hash_type, - const std::function& interruption_point = {}, - const CBlockIndex* pindex = nullptr, - bool index_requested = true); +std::optional GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, + CoinStatsHashType hash_type, + const std::function& interruption_point = {}, + const CBlockIndex* pindex = nullptr, + bool index_requested = true); uint64_t GetBogoSize(const CScript& script_pub_key); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 98c2deb4bf0..97983cea5a5 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -862,7 +862,6 @@ 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{}; bool index_requested = request.params[2].isNull() || request.params[2].get_bool(); NodeContext& node = EnsureAnyNodeContext(request.context); @@ -903,7 +902,9 @@ static RPCHelpMan gettxoutsetinfo() } } - if (GetUTXOStats(coins_view, *blockman, stats, hash_type, node.rpc_interruption_point, pindex, index_requested)) { + const std::optional 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); @@ -923,9 +924,12 @@ static RPCHelpMan gettxoutsetinfo() ret.pushKV("total_unspendable_amount", ValueFromAmount(stats.total_unspendable_amount)); CCoinsStats prev_stats{}; - if (pindex->nHeight > 0) { - GetUTXOStats(coins_view, *blockman, prev_stats, hash_type, node.rpc_interruption_point, pindex->pprev, index_requested); + const std::optional 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 +2289,7 @@ UniValue CreateUTXOSnapshot( const fs::path& temppath) { std::unique_ptr pcursor; - CCoinsStats stats{}; + std::optional maybe_stats; const CBlockIndex* tip; { @@ -2305,19 +2309,20 @@ UniValue CreateUTXOSnapshot( chainstate.ForceFlushStateToDisk(); - if (!GetUTXOStats(&chainstate.CoinsDB(), chainstate.m_blockman, stats, CoinStatsHashType::HASH_SERIALIZED, 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 +2344,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/validation.cpp b/src/validation.cpp index 5fdf0398df7..40c2618db9d 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -5095,22 +5095,22 @@ bool ChainstateManager::PopulateAndValidateSnapshot( assert(coins_cache.GetBestBlock() == base_blockhash); - CCoinsStats stats{}; 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, CoinStatsHashType::HASH_SERIALIZED, breakpoint_fnc)) { + const std::optional maybe_stats = GetUTXOStats(snapshot_coinsdb, m_blockman, CoinStatsHashType::HASH_SERIALIZED, 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; } From 1352e410a5b84070279ff28399083cb3ab278593 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 16 Feb 2022 14:11:19 -0500 Subject: [PATCH 07/15] 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. --- src/node/coinstats.cpp | 71 +++++++++++++++++++++++++++++++----------- src/node/coinstats.h | 3 ++ 2 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 8fc53fe1725..26badd6eb29 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -19,6 +19,11 @@ #include 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 -static bool GetUTXOStats(CCoinsView* view, BlockManager& blockman, CCoinsStats& stats, T hash_obj, const std::function& interruption_point, const CBlockIndex* pindex, CoinStatsHashType& hash_type, bool index_requested) +static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function& interruption_point) { std::unique_ptr 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 GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex, bool index_requested) +std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function& 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 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 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 GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function& 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 diff --git a/src/node/coinstats.h b/src/node/coinstats.h index 36cbf19f295..d74d70deb23 100644 --- a/src/node/coinstats.h +++ b/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); }; /** From b7634fe02b6b030f5d62502c73db84ba9a276640 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 2 May 2022 13:44:19 -0400 Subject: [PATCH 08/15] Move logic from LookupUTXOStatsWithIndex to CoinStatsIndex::LookUpStats The indexing codepath logic in node/coinstats.cpp is simple enough to be moved into CoinStatsIndex::LookUpStats, avoiding an additional layer of function calls. Callers are modified accordingly. Also, add 2 missed BOOST_CHECKs to the coinstatsindex_initial_sync unit test. --- src/index/coinstatsindex.cpp | 35 +++++++++++++++++-------------- src/index/coinstatsindex.h | 2 +- src/node/coinstats.cpp | 24 +++------------------ src/test/coinstatsindex_tests.cpp | 10 ++++----- 4 files changed, 27 insertions(+), 44 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 69078708f90..c4bc47e538d 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -316,28 +316,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 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; + 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 true; + return stats; } bool CoinStatsIndex::Init() diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index 6f53bb74fbe..a6c935c25e9 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -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 LookUpStats(const CBlockIndex* block_index) const; }; /// The global UTXO set hash object. diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 26badd6eb29..8851eb2c2d0 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -187,33 +187,15 @@ static void FinalizeHash(MuHash3072& muhash, CCoinsStats& stats) } static void FinalizeHash(std::nullptr_t, CCoinsStats& stats) {} -std::optional 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 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 GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function& 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); + return g_coin_stats_index->LookUpStats(pindex); } else { - return LookupUTXOStatsWithIndex(*g_coin_stats_index, view, blockman); + CBlockIndex* block_index = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(view->GetBestBlock())); + return g_coin_stats_index->LookUpStats(block_index); } } diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index d7d09fa168e..c3b6870cbd1 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -33,7 +33,6 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup) { CoinStatsIndex coin_stats_index{1 << 20, true}; - CCoinsStats coin_stats{}; 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 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{}; 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); From 35f73ce4b2efd7341fe55f77b334f27ad8aad090 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 16 Feb 2022 15:31:02 -0500 Subject: [PATCH 09/15] coinstats: Move hasher codepath to kernel/coinstats As mentioned in a previous commit, the hashing codepath can now be moved to a separate file. This decouples callers that only rely on the hashing codepath from the indexing one. This is key for libbitcoinkernel, which needs to have the CoinsStats hashing codepath for AssumeUTXO, but does not wish to be coupled with indexes. Note that only the .cpp file is split in this commit, the header files will be split in a subsequent commit and the #includes to node/coinstats.h will be adjusted to only #include the necessary headers. --- src/Makefile.am | 2 + src/kernel/coinstats.cpp | 187 +++++++++++++++++++++++++++++++++++++++ src/node/coinstats.cpp | 176 ------------------------------------ src/node/coinstats.h | 2 + 4 files changed, 191 insertions(+), 176 deletions(-) create mode 100644 src/kernel/coinstats.cpp diff --git a/src/Makefile.am b/src/Makefile.am index 0fd15baed1c..005a91912c0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -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 \ @@ -873,6 +874,7 @@ libbitcoinkernel_la_SOURCES = \ index/base.cpp \ index/coinstatsindex.cpp \ init/common.cpp \ + kernel/coinstats.cpp \ key.cpp \ logging.cpp \ node/blockstorage.cpp \ diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp new file mode 100644 index 00000000000..15d5c3fbe6e --- /dev/null +++ b/src/kernel/coinstats.cpp @@ -0,0 +1,187 @@ +// 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 + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +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) +{ + return 32 /* txid */ + + 4 /* vout index */ + + 4 /* height + coinbase */ + + 8 /* amount */ + + 2 /* scriptPubKey len */ + + script_pub_key.size() /* scriptPubKey */; +} + +CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin) { + CDataStream ss(SER_DISK, PROTOCOL_VERSION); + ss << outpoint; + ss << static_cast(coin.nHeight * 2 + coin.fCoinBase); + ss << coin.out; + return ss; +} + +//! Warning: be very careful when changing this! assumeutxo and UTXO snapshot +//! validation commitments are reliant on the hash constructed by this +//! function. +//! +//! If the construction of this hash is changed, it will invalidate +//! existing UTXO snapshots. This will not result in any kind of consensus +//! failure, but it will force clients that were expecting to make use of +//! assumeutxo to do traditional IBD instead. +//! +//! It is also possible, though very unlikely, that a change in this +//! construction could cause a previously invalid (and potentially malicious) +//! UTXO snapshot to be considered valid. +static void ApplyHash(CHashWriter& ss, const uint256& hash, const std::map& outputs) +{ + for (auto it = outputs.begin(); it != outputs.end(); ++it) { + if (it == outputs.begin()) { + ss << hash; + ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); + } + + ss << VARINT(it->first + 1); + ss << it->second.out.scriptPubKey; + ss << VARINT_MODE(it->second.out.nValue, VarIntMode::NONNEGATIVE_SIGNED); + + if (it == std::prev(outputs.end())) { + ss << VARINT(0u); + } + } +} + +static void ApplyHash(std::nullptr_t, const uint256& hash, const std::map& outputs) {} + +static void ApplyHash(MuHash3072& muhash, const uint256& hash, const std::map& outputs) +{ + for (auto it = outputs.begin(); it != outputs.end(); ++it) { + COutPoint outpoint = COutPoint(hash, it->first); + Coin coin = it->second; + muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin))); + } +} + +static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map& outputs) +{ + assert(!outputs.empty()); + stats.nTransactions++; + for (auto it = outputs.begin(); it != outputs.end(); ++it) { + stats.nTransactionOutputs++; + if (stats.total_amount.has_value()) { + stats.total_amount = CheckedAdd(*stats.total_amount, it->second.out.nValue); + } + stats.nBogoSize += GetBogoSize(it->second.out.scriptPubKey); + } +} + +//! Calculate statistics about the unspent transaction output set +template +static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function& interruption_point) +{ + std::unique_ptr pcursor(view->Cursor()); + assert(pcursor); + + PrepareHash(hash_obj, stats); + + uint256 prevkey; + std::map outputs; + while (pcursor->Valid()) { + interruption_point(); + COutPoint key; + Coin coin; + if (pcursor->GetKey(key) && pcursor->GetValue(coin)) { + if (!outputs.empty() && key.hash != prevkey) { + ApplyStats(stats, prevkey, outputs); + ApplyHash(hash_obj, prevkey, outputs); + outputs.clear(); + } + prevkey = key.hash; + outputs[key.n] = std::move(coin); + stats.coins_count++; + } else { + return error("%s: unable to read value", __func__); + } + pcursor->Next(); + } + if (!outputs.empty()) { + ApplyStats(stats, prevkey, outputs); + ApplyHash(hash_obj, prevkey, outputs); + } + + FinalizeHash(hash_obj, stats); + + stats.nDiskSize = view->EstimateSize(); + + return true; +} + +std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function& interruption_point) +{ + 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; + } + return stats; +} + +// The legacy hash serializes the hashBlock +static void PrepareHash(CHashWriter& ss, const CCoinsStats& stats) +{ + ss << stats.hashBlock; +} +// MuHash does not need the prepare step +static void PrepareHash(MuHash3072& muhash, CCoinsStats& stats) {} +static void PrepareHash(std::nullptr_t, CCoinsStats& stats) {} + +static void FinalizeHash(CHashWriter& ss, CCoinsStats& stats) +{ + stats.hashSerialized = ss.GetHash(); +} +static void FinalizeHash(MuHash3072& muhash, CCoinsStats& stats) +{ + uint256 out; + muhash.Finalize(out); + stats.hashSerialized = out; +} +static void FinalizeHash(std::nullptr_t, CCoinsStats& stats) {} + +} // namespace node diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 8851eb2c2d0..12c8e7b0dac 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -6,187 +6,11 @@ #include #include -#include -#include #include #include -#include -#include -#include -#include #include -#include - 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) -{ - return 32 /* txid */ + - 4 /* vout index */ + - 4 /* height + coinbase */ + - 8 /* amount */ + - 2 /* scriptPubKey len */ + - script_pub_key.size() /* scriptPubKey */; -} - -CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin) { - CDataStream ss(SER_DISK, PROTOCOL_VERSION); - ss << outpoint; - ss << static_cast(coin.nHeight * 2 + coin.fCoinBase); - ss << coin.out; - return ss; -} - -//! Warning: be very careful when changing this! assumeutxo and UTXO snapshot -//! validation commitments are reliant on the hash constructed by this -//! function. -//! -//! If the construction of this hash is changed, it will invalidate -//! existing UTXO snapshots. This will not result in any kind of consensus -//! failure, but it will force clients that were expecting to make use of -//! assumeutxo to do traditional IBD instead. -//! -//! It is also possible, though very unlikely, that a change in this -//! construction could cause a previously invalid (and potentially malicious) -//! UTXO snapshot to be considered valid. -static void ApplyHash(CHashWriter& ss, const uint256& hash, const std::map& outputs) -{ - for (auto it = outputs.begin(); it != outputs.end(); ++it) { - if (it == outputs.begin()) { - ss << hash; - ss << VARINT(it->second.nHeight * 2 + it->second.fCoinBase ? 1u : 0u); - } - - ss << VARINT(it->first + 1); - ss << it->second.out.scriptPubKey; - ss << VARINT_MODE(it->second.out.nValue, VarIntMode::NONNEGATIVE_SIGNED); - - if (it == std::prev(outputs.end())) { - ss << VARINT(0u); - } - } -} - -static void ApplyHash(std::nullptr_t, const uint256& hash, const std::map& outputs) {} - -static void ApplyHash(MuHash3072& muhash, const uint256& hash, const std::map& outputs) -{ - for (auto it = outputs.begin(); it != outputs.end(); ++it) { - COutPoint outpoint = COutPoint(hash, it->first); - Coin coin = it->second; - muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin))); - } -} - -static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map& outputs) -{ - assert(!outputs.empty()); - stats.nTransactions++; - for (auto it = outputs.begin(); it != outputs.end(); ++it) { - stats.nTransactionOutputs++; - if (stats.total_amount.has_value()) { - stats.total_amount = CheckedAdd(*stats.total_amount, it->second.out.nValue); - } - stats.nBogoSize += GetBogoSize(it->second.out.scriptPubKey); - } -} - -//! Calculate statistics about the unspent transaction output set -template -static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function& interruption_point) -{ - std::unique_ptr pcursor(view->Cursor()); - assert(pcursor); - - PrepareHash(hash_obj, stats); - - uint256 prevkey; - std::map outputs; - while (pcursor->Valid()) { - interruption_point(); - COutPoint key; - Coin coin; - if (pcursor->GetKey(key) && pcursor->GetValue(coin)) { - if (!outputs.empty() && key.hash != prevkey) { - ApplyStats(stats, prevkey, outputs); - ApplyHash(hash_obj, prevkey, outputs); - outputs.clear(); - } - prevkey = key.hash; - outputs[key.n] = std::move(coin); - stats.coins_count++; - } else { - return error("%s: unable to read value", __func__); - } - pcursor->Next(); - } - if (!outputs.empty()) { - ApplyStats(stats, prevkey, outputs); - ApplyHash(hash_obj, prevkey, outputs); - } - - FinalizeHash(hash_obj, stats); - - stats.nDiskSize = view->EstimateSize(); - - return true; -} - -std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function& interruption_point) -{ - 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; - } - return stats; -} - -// The legacy hash serializes the hashBlock -static void PrepareHash(CHashWriter& ss, const CCoinsStats& stats) -{ - ss << stats.hashBlock; -} -// MuHash does not need the prepare step -static void PrepareHash(MuHash3072& muhash, CCoinsStats& stats) {} -static void PrepareHash(std::nullptr_t, CCoinsStats& stats) {} - -static void FinalizeHash(CHashWriter& ss, CCoinsStats& stats) -{ - stats.hashSerialized = ss.GetHash(); -} -static void FinalizeHash(MuHash3072& muhash, CCoinsStats& stats) -{ - uint256 out; - muhash.Finalize(out); - stats.hashSerialized = out; -} -static void FinalizeHash(std::nullptr_t, CCoinsStats& stats) {} - std::optional GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function& 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 diff --git a/src/node/coinstats.h b/src/node/coinstats.h index d74d70deb23..8abf1e31e80 100644 --- a/src/node/coinstats.h +++ b/src/node/coinstats.h @@ -83,6 +83,8 @@ std::optional GetUTXOStats(CCoinsView* view, node::BlockManager& bl uint64_t GetBogoSize(const CScript& script_pub_key); CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin); + +std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function& interruption_point = {}); } // namespace node #endif // BITCOIN_NODE_COINSTATS_H From 80970985c965f79b8c376c8a922497e385445dd8 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 16 Feb 2022 17:51:57 -0500 Subject: [PATCH 10/15] coinstats: Split node/coinstats.h to kernel/coinstats.h Most of this commit is pure-move. After this change: - kernel/coinstats.h -> Contains declarations for: - enum class CoinStatsHashType - struct CCoinsStats - GetBogoSize(...) - TxOutSer(...) - ComputeUTXOStats(...) - node/coinstats.h -> Just GetUTXOStats, which will be removed as we change callers to directly use the hashing/indexing codepaths in future commits. --- src/Makefile.am | 1 + src/kernel/coinstats.cpp | 2 +- src/kernel/coinstats.h | 78 ++++++++++++++++++++++++++++++++++++++++ src/node/coinstats.h | 56 ++--------------------------- 4 files changed, 82 insertions(+), 55 deletions(-) create mode 100644 src/kernel/coinstats.h diff --git a/src/Makefile.am b/src/Makefile.am index 005a91912c0..ffbd5e93762 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 \ diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp index 15d5c3fbe6e..49db98d6634 100644 --- a/src/kernel/coinstats.cpp +++ b/src/kernel/coinstats.cpp @@ -2,7 +2,7 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. -#include +#include #include #include diff --git a/src/kernel/coinstats.h b/src/kernel/coinstats.h new file mode 100644 index 00000000000..d470ea715ab --- /dev/null +++ b/src/kernel/coinstats.h @@ -0,0 +1,78 @@ +// 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_KERNEL_COINSTATS_H +#define BITCOIN_KERNEL_COINSTATS_H + +#include +#include +#include +#include +#include + +#include +#include + +class CCoinsView; +namespace node { +class BlockManager; +} // namespace node + +namespace node { +enum class CoinStatsHashType { + HASH_SERIALIZED, + MUHASH, + NONE, +}; + +struct CCoinsStats { + int nHeight{0}; + uint256 hashBlock{}; + uint64_t nTransactions{0}; + uint64_t nTransactionOutputs{0}; + uint64_t nBogoSize{0}; + uint256 hashSerialized{}; + uint64_t nDiskSize{0}; + //! The total amount, or nullopt if an overflow occurred calculating it + std::optional total_amount{0}; + + //! The number of coins contained. + uint64_t coins_count{0}; + + //! Signals if the coinstatsindex was used to retrieve the statistics. + bool index_used{false}; + + // Following values are only available from coinstats index + + //! Total cumulative amount of block subsidies up to and including this block + CAmount total_subsidy{0}; + //! Total cumulative amount of unspendable coins up to and including this block + CAmount total_unspendable_amount{0}; + //! Total cumulative amount of prevouts spent up to and including this block + CAmount total_prevout_spent_amount{0}; + //! Total cumulative amount of outputs created up to and including this block + CAmount total_new_outputs_ex_coinbase_amount{0}; + //! Total cumulative amount of coinbase outputs up to and including this block + CAmount total_coinbase_amount{0}; + //! The unspendable coinbase amount from the genesis block + CAmount total_unspendables_genesis_block{0}; + //! The two unspendable coinbase outputs total amount caused by BIP30 + CAmount total_unspendables_bip30{0}; + //! Total cumulative amount of outputs sent to unspendable scripts (OP_RETURN for example) up to and including this block + 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); +}; + +uint64_t GetBogoSize(const CScript& script_pub_key); + +CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin); + +std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function& interruption_point = {}); +} // namespace node + +#endif // BITCOIN_KERNEL_COINSTATS_H diff --git a/src/node/coinstats.h b/src/node/coinstats.h index 8abf1e31e80..fbd5fd40679 100644 --- a/src/node/coinstats.h +++ b/src/node/coinstats.h @@ -6,6 +6,8 @@ #ifndef BITCOIN_NODE_COINSTATS_H #define BITCOIN_NODE_COINSTATS_H +#include + #include #include #include @@ -21,54 +23,6 @@ class BlockManager; } // namespace node namespace node { -enum class CoinStatsHashType { - HASH_SERIALIZED, - MUHASH, - NONE, -}; - -struct CCoinsStats { - int nHeight{0}; - uint256 hashBlock{}; - uint64_t nTransactions{0}; - uint64_t nTransactionOutputs{0}; - uint64_t nBogoSize{0}; - uint256 hashSerialized{}; - uint64_t nDiskSize{0}; - //! The total amount, or nullopt if an overflow occurred calculating it - std::optional total_amount{0}; - - //! The number of coins contained. - uint64_t coins_count{0}; - - //! Signals if the coinstatsindex was used to retrieve the statistics. - bool index_used{false}; - - // Following values are only available from coinstats index - - //! Total cumulative amount of block subsidies up to and including this block - CAmount total_subsidy{0}; - //! Total cumulative amount of unspendable coins up to and including this block - CAmount total_unspendable_amount{0}; - //! Total cumulative amount of prevouts spent up to and including this block - CAmount total_prevout_spent_amount{0}; - //! Total cumulative amount of outputs created up to and including this block - CAmount total_new_outputs_ex_coinbase_amount{0}; - //! Total cumulative amount of coinbase outputs up to and including this block - CAmount total_coinbase_amount{0}; - //! The unspendable coinbase amount from the genesis block - CAmount total_unspendables_genesis_block{0}; - //! The two unspendable coinbase outputs total amount caused by BIP30 - CAmount total_unspendables_bip30{0}; - //! Total cumulative amount of outputs sent to unspendable scripts (OP_RETURN for example) up to and including this block - 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); -}; - /** * Calculate statistics about the unspent transaction output set * @@ -79,12 +33,6 @@ std::optional GetUTXOStats(CCoinsView* view, node::BlockManager& bl const std::function& interruption_point = {}, const CBlockIndex* pindex = nullptr, bool index_requested = true); - -uint64_t GetBogoSize(const CScript& script_pub_key); - -CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin); - -std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function& interruption_point = {}); } // namespace node #endif // BITCOIN_NODE_COINSTATS_H From 0e54456f0498e52131f8ae0c76b4dfe25f45b076 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Mon, 2 May 2022 14:25:05 -0400 Subject: [PATCH 11/15] Use only kernel/coinstats.h in index/coinstatsindex.h Removes a circular dependency, horray! --- src/index/coinstatsindex.h | 2 +- test/lint/lint-circular-dependencies.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index a6c935c25e9..315f1fa14aa 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -9,7 +9,7 @@ #include #include #include -#include +#include /** * CoinStatsIndex maintains statistics on the UTXO set. diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index 7ca2ec994b9..3b328501cfd 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", From f329a9298c06ffe74b9e9fbc07bfe6d282fef9cb Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 16 Feb 2022 17:58:41 -0500 Subject: [PATCH 12/15] scripted-diff: Move src/kernel/coinstats to kernel:: Introduces a new kernel:: namespace and move all of src/kernel/coinstats under it. In the verify script, lines like: line="$(grep -n 'namespace node {' -- src/kernel/coinstats.h | tail -n1 | cut -d: -f1)" sed -i -e "${line}s@namespace node {@namespace kernel {@" -- src/kernel/coinstats.h Are intended to replace only the last instance of "namespace node" with "namespace kernel", this is to avoid replacing forward declarations of things inside the node:: namespace. -BEGIN VERIFY SCRIPT- sed -E -i 's@namespace node@namespace kernel@g' -- src/kernel/coinstats.cpp line="$(grep -n 'namespace node {' -- src/kernel/coinstats.h | tail -n1 | cut -d: -f1)" sed -i -e "${line}s@namespace node {@namespace kernel {@" -- src/kernel/coinstats.h line="$(grep -n '// namespace node' -- src/kernel/coinstats.h | tail -n1 | cut -d: -f1)" sed -i -e "${line}s@// namespace node@// namespace kernel@" -- src/kernel/coinstats.h things='(CCoinsStats|CoinStatsHashType|GetBogoSize|TxOutSer|ComputeUTXOStats)' git grep -lE 'node::'"$things" | xargs sed -E -i 's@node::'"$things"'@kernel::\1@g' sed -E -i 's@'"$things"'@kernel::\1@g' -- src/node/coinstats.cpp src/node/coinstats.h sed -E -i 's@BlockManager@node::\0@g' -- src/kernel/coinstats.cpp -END VERIFY SCRIPT- --- src/index/coinstatsindex.cpp | 6 +++--- src/index/coinstatsindex.h | 2 +- src/kernel/coinstats.cpp | 6 +++--- src/kernel/coinstats.h | 4 ++-- src/node/coinstats.cpp | 6 +++--- src/node/coinstats.h | 4 ++-- src/rpc/blockchain.cpp | 4 ++-- src/test/coinstatsindex_tests.cpp | 4 ++-- src/validation.cpp | 4 ++-- 9 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index c4bc47e538d..666eb4fdb05 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -12,10 +12,10 @@ #include #include -using node::CCoinsStats; -using node::GetBogoSize; +using kernel::CCoinsStats; +using kernel::GetBogoSize; using node::ReadBlockFromDisk; -using node::TxOutSer; +using kernel::TxOutSer; using node::UndoReadFromDisk; static constexpr uint8_t DB_BLOCK_HASH{'s'}; diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index 315f1fa14aa..cae052d9133 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -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 - std::optional LookUpStats(const CBlockIndex* block_index) const; + std::optional LookUpStats(const CBlockIndex* block_index) const; }; /// The global UTXO set hash object. diff --git a/src/kernel/coinstats.cpp b/src/kernel/coinstats.cpp index 49db98d6634..f3808716276 100644 --- a/src/kernel/coinstats.cpp +++ b/src/kernel/coinstats.cpp @@ -15,7 +15,7 @@ #include -namespace node { +namespace kernel { CCoinsStats::CCoinsStats(int block_height, const uint256& block_hash) : nHeight(block_height), @@ -135,7 +135,7 @@ static bool ComputeUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, c return true; } -std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, BlockManager& blockman, const std::function& interruption_point) +std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function& interruption_point) { CBlockIndex* pindex = WITH_LOCK(::cs_main, return blockman.LookupBlockIndex(view->GetBestBlock())); CCoinsStats stats{Assert(pindex)->nHeight, pindex->GetBlockHash()}; @@ -184,4 +184,4 @@ static void FinalizeHash(MuHash3072& muhash, CCoinsStats& stats) } static void FinalizeHash(std::nullptr_t, CCoinsStats& stats) {} -} // namespace node +} // namespace kernel diff --git a/src/kernel/coinstats.h b/src/kernel/coinstats.h index d470ea715ab..a15957233fa 100644 --- a/src/kernel/coinstats.h +++ b/src/kernel/coinstats.h @@ -19,7 +19,7 @@ namespace node { class BlockManager; } // namespace node -namespace node { +namespace kernel { enum class CoinStatsHashType { HASH_SERIALIZED, MUHASH, @@ -73,6 +73,6 @@ uint64_t GetBogoSize(const CScript& script_pub_key); CDataStream TxOutSer(const COutPoint& outpoint, const Coin& coin); std::optional ComputeUTXOStats(CoinStatsHashType hash_type, CCoinsView* view, node::BlockManager& blockman, const std::function& interruption_point = {}); -} // namespace node +} // namespace kernel #endif // BITCOIN_KERNEL_COINSTATS_H diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp index 12c8e7b0dac..784d33d698d 100644 --- a/src/node/coinstats.cpp +++ b/src/node/coinstats.cpp @@ -11,10 +11,10 @@ #include namespace node { -std::optional GetUTXOStats(CCoinsView* view, BlockManager& blockman, CoinStatsHashType hash_type, const std::function& interruption_point, const CBlockIndex* pindex, bool index_requested) +std::optional GetUTXOStats(CCoinsView* view, BlockManager& blockman, kernel::CoinStatsHashType hash_type, const std::function& 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 ((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 { @@ -29,6 +29,6 @@ std::optional GetUTXOStats(CCoinsView* view, BlockManager& blockman // best block. assert(!pindex || pindex->GetBlockHash() == view->GetBestBlock()); - return ComputeUTXOStats(hash_type, view, blockman, interruption_point); + return kernel::ComputeUTXOStats(hash_type, view, blockman, interruption_point); } } // namespace node diff --git a/src/node/coinstats.h b/src/node/coinstats.h index fbd5fd40679..b1d1384bb3a 100644 --- a/src/node/coinstats.h +++ b/src/node/coinstats.h @@ -28,8 +28,8 @@ namespace node { * * @param[in] index_requested Signals if the coinstatsindex should be used (when available). */ -std::optional GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, - CoinStatsHashType hash_type, +std::optional GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, + kernel::CoinStatsHashType hash_type, const std::function& interruption_point = {}, const CBlockIndex* pindex = nullptr, bool index_requested = true); diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 97983cea5a5..8789d76f3cb 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -52,8 +52,8 @@ #include using node::BlockManager; -using node::CCoinsStats; -using node::CoinStatsHashType; +using kernel::CCoinsStats; +using kernel::CoinStatsHashType; using node::GetUTXOStats; using node::NodeContext; using node::ReadBlockFromDisk; diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp index c3b6870cbd1..50eb4790351 100644 --- a/src/test/coinstatsindex_tests.cpp +++ b/src/test/coinstatsindex_tests.cpp @@ -13,8 +13,8 @@ #include -using node::CCoinsStats; -using node::CoinStatsHashType; +using kernel::CCoinsStats; +using kernel::CoinStatsHashType; BOOST_AUTO_TEST_SUITE(coinstatsindex_tests) diff --git a/src/validation.cpp b/src/validation.cpp index 40c2618db9d..afaea9bb785 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -63,8 +63,8 @@ using node::BlockManager; using node::BlockMap; using node::CBlockIndexHeightOnlyComparator; using node::CBlockIndexWorkComparator; -using node::CCoinsStats; -using node::CoinStatsHashType; +using kernel::CCoinsStats; +using kernel::CoinStatsHashType; using node::fImporting; using node::fPruneMode; using node::fReindex; From faa52387e8e4856445b1cfc9b5e072ce8f690f36 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 17 Feb 2022 22:02:48 -0500 Subject: [PATCH 13/15] style-only: Rearrange using decls after scripted-diff --- src/index/coinstatsindex.cpp | 3 ++- src/rpc/blockchain.cpp | 3 ++- src/validation.cpp | 5 +++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 666eb4fdb05..687e330fe0b 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -14,8 +14,9 @@ using kernel::CCoinsStats; using kernel::GetBogoSize; -using node::ReadBlockFromDisk; using kernel::TxOutSer; + +using node::ReadBlockFromDisk; using node::UndoReadFromDisk; static constexpr uint8_t DB_BLOCK_HASH{'s'}; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 8789d76f3cb..968485661f0 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -51,9 +51,10 @@ #include #include -using node::BlockManager; using kernel::CCoinsStats; using kernel::CoinStatsHashType; + +using node::BlockManager; using node::GetUTXOStats; using node::NodeContext; using node::ReadBlockFromDisk; diff --git a/src/validation.cpp b/src/validation.cpp index afaea9bb785..8c807adc703 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -58,13 +58,14 @@ #include #include +using kernel::CCoinsStats; +using kernel::CoinStatsHashType; + using node::BLOCKFILE_CHUNK_SIZE; using node::BlockManager; using node::BlockMap; using node::CBlockIndexHeightOnlyComparator; using node::CBlockIndexWorkComparator; -using kernel::CCoinsStats; -using kernel::CoinStatsHashType; using node::fImporting; using node::fPruneMode; using node::fReindex; From f1006875665ffe8ff5da8185effe25b860743b4e Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Wed, 16 Feb 2022 15:23:11 -0500 Subject: [PATCH 14/15] kernel: Use ComputeUTXOStats in validation This is the "fruit of our labor" for this patchset. ChainstateManager::PopulateAndValidateSnapshot can now directly call ComputeUTXOStats(...). Our consensus engine is now fully decoupled from all indices. See the src/Makefile.am for some satisfying removals. --- src/Makefile.am | 3 --- src/validation.cpp | 9 +++++---- test/lint/lint-circular-dependencies.py | 2 +- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index ffbd5e93762..9b061996760 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -872,15 +872,12 @@ libbitcoinkernel_la_SOURCES = \ flatfile.cpp \ fs.cpp \ hash.cpp \ - index/base.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/validation.cpp b/src/validation.cpp index 8c807adc703..44e908152c2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -18,10 +18,10 @@ #include #include #include +#include #include #include #include -#include #include #include #include @@ -60,6 +60,7 @@ using kernel::CCoinsStats; using kernel::CoinStatsHashType; +using kernel::ComputeUTXOStats; using node::BLOCKFILE_CHUNK_SIZE; using node::BlockManager; @@ -69,7 +70,6 @@ using node::CBlockIndexWorkComparator; using node::fImporting; using node::fPruneMode; using node::fReindex; -using node::GetUTXOStats; using node::nPruneTarget; using node::OpenBlockFile; using node::ReadBlockFromDisk; @@ -4988,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; @@ -5102,7 +5103,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( // about the snapshot_chainstate. CCoinsViewDB* snapshot_coinsdb = WITH_LOCK(::cs_main, return &snapshot_chainstate.CoinsDB()); - const std::optional maybe_stats = GetUTXOStats(snapshot_coinsdb, m_blockman, CoinStatsHashType::HASH_SERIALIZED, breakpoint_fnc); + const std::optional 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; diff --git a/test/lint/lint-circular-dependencies.py b/test/lint/lint-circular-dependencies.py index 3b328501cfd..5d157eb4b1b 100755 --- a/test/lint/lint-circular-dependencies.py +++ b/test/lint/lint-circular-dependencies.py @@ -21,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" From 664a14ba7ccb40aa82d35a59831acd35db1897a6 Mon Sep 17 00:00:00 2001 From: Carl Dong Date: Thu, 17 Feb 2022 17:07:48 -0500 Subject: [PATCH 15/15] coinstats: Move GetUTXOStats to rpc/blockchain rpc/blockchain.cpp is now the only user of the vestigial GetUTXOStats(...). And since GetUTXOStats(...)'s special fallback logic was only really relevant/meant for rpc/blockchain.cpp, we can just move it there. --- src/Makefile.am | 2 -- src/node/coinstats.cpp | 34 ---------------------------------- src/node/coinstats.h | 38 -------------------------------------- src/rpc/blockchain.cpp | 33 +++++++++++++++++++++++++++++++-- 4 files changed, 31 insertions(+), 76 deletions(-) delete mode 100644 src/node/coinstats.cpp delete mode 100644 src/node/coinstats.h diff --git a/src/Makefile.am b/src/Makefile.am index 9b061996760..7f82d188f0c 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -192,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 \ @@ -367,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 \ diff --git a/src/node/coinstats.cpp b/src/node/coinstats.cpp deleted file mode 100644 index 784d33d698d..00000000000 --- a/src/node/coinstats.cpp +++ /dev/null @@ -1,34 +0,0 @@ -// Copyright (c) 2010 Satoshi Nakamoto -// Copyright (c) 2009-2021 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 - -#include -#include -#include -#include - -namespace node { -std::optional GetUTXOStats(CCoinsView* view, BlockManager& blockman, kernel::CoinStatsHashType hash_type, const std::function& 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 == 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. - assert(!pindex || pindex->GetBlockHash() == view->GetBestBlock()); - - return kernel::ComputeUTXOStats(hash_type, view, blockman, interruption_point); -} -} // namespace node diff --git a/src/node/coinstats.h b/src/node/coinstats.h deleted file mode 100644 index b1d1384bb3a..00000000000 --- a/src/node/coinstats.h +++ /dev/null @@ -1,38 +0,0 @@ -// Copyright (c) 2010 Satoshi Nakamoto -// Copyright (c) 2009-2021 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 - -#include - -#include -#include -#include -#include -#include - -#include -#include - -class CCoinsView; -namespace node { -class BlockManager; -} // namespace node - -namespace node { -/** - * Calculate statistics about the unspent transaction output set - * - * @param[in] index_requested Signals if the coinstatsindex should be used (when available). - */ -std::optional GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, - kernel::CoinStatsHashType hash_type, - const std::function& interruption_point = {}, - const CBlockIndex* pindex = nullptr, - bool index_requested = true); -} // namespace node - -#endif // BITCOIN_NODE_COINSTATS_H diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 968485661f0..03354f39f61 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -19,11 +19,11 @@ #include #include #include +#include #include #include #include #include -#include #include #include #include @@ -55,7 +55,6 @@ using kernel::CCoinsStats; using kernel::CoinStatsHashType; using node::BlockManager; -using node::GetUTXOStats; using node::NodeContext; using node::ReadBlockFromDisk; using node::SnapshotMetadata; @@ -809,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 GetUTXOStats(CCoinsView* view, node::BlockManager& blockman, + kernel::CoinStatsHashType hash_type, + const std::function& 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",