From fab842b3248744fb0030486f64d3febe815f8377 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 15 Aug 2025 17:43:23 +0200 Subject: [PATCH 1/9] index, refactor: Rename ReverseBlock to RevertBlock Semantically this is the correct name for what the function is doing. --- src/index/coinstatsindex.cpp | 6 +++--- src/index/coinstatsindex.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 96693f7f48..ce3f566d9e 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -265,7 +265,7 @@ bool CoinStatsIndex::CustomRemove(const interfaces::BlockInfo& block) if (!m_db->WriteBatch(batch)) return false; - if (!ReverseBlock(block)) { + if (!RevertBlock(block)) { return false; // failure cause logged internally } @@ -381,8 +381,8 @@ interfaces::Chain::NotifyOptions CoinStatsIndex::CustomOptions() return options; } -// Reverse a single block as part of a reorg -bool CoinStatsIndex::ReverseBlock(const interfaces::BlockInfo& block) +// Revert a single block as part of a reorg +bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) { std::pair read_out; diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index 6e2743688a..f2e95b449e 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -38,7 +38,7 @@ private: CAmount m_total_unspendables_scripts{0}; CAmount m_total_unspendables_unclaimed_rewards{0}; - [[nodiscard]] bool ReverseBlock(const interfaces::BlockInfo& block); + [[nodiscard]] bool RevertBlock(const interfaces::BlockInfo& block); bool AllowPrune() const override { return true; } From 84e813a02bb7b3c735ae413f06c0fc156bfeb7ac Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Fri, 15 Aug 2025 18:09:07 +0200 Subject: [PATCH 2/9] index, refactor: DRY coinbase check Also marks a few additional variables const. --- src/index/coinstatsindex.cpp | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index ce3f566d9e..48d4c74961 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -140,9 +140,10 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) assert(block.data); for (size_t i = 0; i < block.data->vtx.size(); ++i) { const auto& tx{block.data->vtx.at(i)}; + const bool is_coinbase{tx->IsCoinBase()}; // Skip duplicate txid coinbase transactions (BIP30). - if (IsBIP30Unspendable(block.hash, block.height) && tx->IsCoinBase()) { + if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) { m_total_unspendable_amount += block_subsidy; m_total_unspendables_bip30 += block_subsidy; continue; @@ -150,8 +151,8 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) for (uint32_t j = 0; j < tx->vout.size(); ++j) { const CTxOut& out{tx->vout[j]}; - Coin coin{out, block.height, tx->IsCoinBase()}; - COutPoint outpoint{tx->GetHash(), j}; + const Coin coin{out, block.height, is_coinbase}; + const COutPoint outpoint{tx->GetHash(), j}; // Skip unspendable coins if (coin.out.scriptPubKey.IsUnspendable()) { @@ -162,7 +163,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) ApplyCoinHash(m_muhash, outpoint, coin); - if (tx->IsCoinBase()) { + if (is_coinbase) { m_total_coinbase_amount += coin.out.nValue; } else { m_total_new_outputs_ex_coinbase_amount += coin.out.nValue; @@ -174,12 +175,12 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) } // The coinbase tx has no undo data since no former output is spent - if (!tx->IsCoinBase()) { + if (!is_coinbase) { const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)}; for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) { - Coin coin{tx_undo.vprevout[j]}; - COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; + const Coin coin{tx_undo.vprevout[j]}; + const COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; RemoveCoinHash(m_muhash, outpoint, coin); @@ -413,11 +414,12 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) assert(block.undo_data); for (size_t i = 0; i < block.data->vtx.size(); ++i) { const auto& tx{block.data->vtx.at(i)}; + const bool is_coinbase{tx->IsCoinBase()}; for (uint32_t j = 0; j < tx->vout.size(); ++j) { const CTxOut& out{tx->vout[j]}; - COutPoint outpoint{tx->GetHash(), j}; - Coin coin{out, block.height, tx->IsCoinBase()}; + const COutPoint outpoint{tx->GetHash(), j}; + const Coin coin{out, block.height, is_coinbase}; // Skip unspendable coins if (coin.out.scriptPubKey.IsUnspendable()) { @@ -440,13 +442,12 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) } // The coinbase tx has no undo data since no former output is spent - if (!tx->IsCoinBase()) { + if (!is_coinbase) { const auto& tx_undo{block.undo_data->vtxundo.at(i - 1)}; for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) { - Coin coin{tx_undo.vprevout[j]}; - COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; - + const Coin coin{tx_undo.vprevout[j]}; + const COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; ApplyCoinHash(m_muhash, outpoint, coin); m_total_prevout_spent_amount -= coin.out.nValue; From 431a076ae6e3cc32a8725d4a01483d27c5081a34 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Mon, 4 Aug 2025 15:25:15 +0200 Subject: [PATCH 3/9] index: Fix coinstatsindex overflow issue The index originally stored cumulative values in a CAmount type but this allowed for potential overflow issues which were observed on Signet. Fix this by storing the values that are in danger of overflowing in a arith_uint256. Also turns an unnecessary copy into a reference in RevertBlock and CustomAppend and gets rid of the explicit total unspendable tracking which can be calculated by adding the four categories of unspendables together. --- doc/files.md | 2 +- src/index/coinstatsindex.cpp | 132 ++++++++++++++------------------ src/index/coinstatsindex.h | 8 +- src/kernel/coinstats.h | 18 +++-- src/rpc/blockchain.cpp | 28 +++++-- test/functional/feature_init.py | 4 +- 6 files changed, 98 insertions(+), 94 deletions(-) diff --git a/doc/files.md b/doc/files.md index 0bf115e1c1..c8d8aab3a7 100644 --- a/doc/files.md +++ b/doc/files.md @@ -57,7 +57,7 @@ Subdirectory | File(s) | Description `indexes/txindex/` | LevelDB database | Transaction index; *optional*, used if `-txindex=1` `indexes/blockfilter/basic/db/` | LevelDB database | Blockfilter index LevelDB database for the basic filtertype; *optional*, used if `-blockfilterindex=basic` `indexes/blockfilter/basic/` | `fltrNNNNN.dat`[\[2\]](#note2) | Blockfilter index filters for the basic filtertype; *optional*, used if `-blockfilterindex=basic` -`indexes/coinstats/db/` | LevelDB database | Coinstats index; *optional*, used if `-coinstatsindex=1` +`indexes/coinstatsindex/db/` | LevelDB database | Coinstats index; *optional*, used if `-coinstatsindex=1` `wallets/` | | [Contains wallets](#multi-wallet-environment); can be specified by `-walletdir` option; if `wallets/` subdirectory does not exist, wallets reside in the [data directory](#data-directory-location) `./` | `anchors.dat` | Anchor IP address database, created on shutdown and deleted at startup. Anchors are last known outgoing block-relay-only peers that are tried to re-connect to on startup `./` | `banlist.json` | Stores the addresses/subnets of banned nodes. diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 48d4c74961..bb4be90bf4 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -2,6 +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 @@ -27,35 +28,42 @@ static constexpr uint8_t DB_MUHASH{'M'}; namespace { struct DBVal { - uint256 muhash; - uint64_t transaction_output_count; - uint64_t bogo_size; - CAmount total_amount; - CAmount total_subsidy; - CAmount total_unspendable_amount; - CAmount total_prevout_spent_amount; - CAmount total_new_outputs_ex_coinbase_amount; - CAmount total_coinbase_amount; - CAmount total_unspendables_genesis_block; - CAmount total_unspendables_bip30; - CAmount total_unspendables_scripts; - CAmount total_unspendables_unclaimed_rewards; + uint256 muhash{uint256::ZERO}; + uint64_t transaction_output_count{0}; + uint64_t bogo_size{0}; + CAmount total_amount{0}; + CAmount total_subsidy{0}; + arith_uint256 total_prevout_spent_amount{0}; + arith_uint256 total_new_outputs_ex_coinbase_amount{0}; + arith_uint256 total_coinbase_amount{0}; + CAmount total_unspendables_genesis_block{0}; + CAmount total_unspendables_bip30{0}; + CAmount total_unspendables_scripts{0}; + CAmount total_unspendables_unclaimed_rewards{0}; SERIALIZE_METHODS(DBVal, obj) { + uint256 prevout_spent, new_outputs, coinbase; + SER_WRITE(obj, prevout_spent = ArithToUint256(obj.total_prevout_spent_amount)); + SER_WRITE(obj, new_outputs = ArithToUint256(obj.total_new_outputs_ex_coinbase_amount)); + SER_WRITE(obj, coinbase = ArithToUint256(obj.total_coinbase_amount)); + READWRITE(obj.muhash); READWRITE(obj.transaction_output_count); READWRITE(obj.bogo_size); READWRITE(obj.total_amount); READWRITE(obj.total_subsidy); - READWRITE(obj.total_unspendable_amount); - READWRITE(obj.total_prevout_spent_amount); - READWRITE(obj.total_new_outputs_ex_coinbase_amount); - READWRITE(obj.total_coinbase_amount); + READWRITE(prevout_spent); + READWRITE(new_outputs); + READWRITE(coinbase); READWRITE(obj.total_unspendables_genesis_block); READWRITE(obj.total_unspendables_bip30); READWRITE(obj.total_unspendables_scripts); READWRITE(obj.total_unspendables_unclaimed_rewards); + + SER_READ(obj, obj.total_prevout_spent_amount = UintToArith256(prevout_spent)); + SER_READ(obj, obj.total_new_outputs_ex_coinbase_amount = UintToArith256(new_outputs)); + SER_READ(obj, obj.total_coinbase_amount = UintToArith256(coinbase)); } }; @@ -106,7 +114,17 @@ std::unique_ptr g_coin_stats_index; CoinStatsIndex::CoinStatsIndex(std::unique_ptr chain, size_t n_cache_size, bool f_memory, bool f_wipe) : BaseIndex(std::move(chain), "coinstatsindex") { - fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstats"}; + // An earlier version of the index used "indexes/coinstats" but it contained + // a bug and is superseded by a fixed version at "indexes/coinstatsindex". + // The original index is kept around until the next release in case users + // decide to downgrade their node. + auto old_path = gArgs.GetDataDirNet() / "indexes" / "coinstats"; + if (fs::exists(old_path)) { + // TODO: Change this to deleting the old index with v31. + LogWarning("Old version of coinstatsindex found at %s. This folder can be safely deleted unless you " \ + "plan to downgrade your node to version 29 or lower.", fs::PathToString(old_path)); + } + fs::path path{gArgs.GetDataDirNet() / "indexes" / "coinstatsindex"}; fs::create_directories(path); m_db = std::make_unique(path / "db", n_cache_size, f_memory, f_wipe); @@ -144,7 +162,6 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // Skip duplicate txid coinbase transactions (BIP30). if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) { - m_total_unspendable_amount += block_subsidy; m_total_unspendables_bip30 += block_subsidy; continue; } @@ -156,7 +173,6 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // Skip unspendable coins if (coin.out.scriptPubKey.IsUnspendable()) { - m_total_unspendable_amount += coin.out.nValue; m_total_unspendables_scripts += coin.out.nValue; continue; } @@ -179,7 +195,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) const auto& tx_undo{Assert(block.undo_data)->vtxundo.at(i - 1)}; for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) { - const Coin coin{tx_undo.vprevout[j]}; + const Coin& coin{tx_undo.vprevout[j]}; const COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; RemoveCoinHash(m_muhash, outpoint, coin); @@ -194,7 +210,6 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) } } else { // genesis block - m_total_unspendable_amount += block_subsidy; m_total_unspendables_genesis_block += block_subsidy; } @@ -202,9 +217,10 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // new outputs + coinbase + current unspendable amount this means // the miner did not claim the full block reward. Unclaimed block // rewards are also unspendable. - const CAmount unclaimed_rewards{(m_total_prevout_spent_amount + m_total_subsidy) - (m_total_new_outputs_ex_coinbase_amount + m_total_coinbase_amount + m_total_unspendable_amount)}; - m_total_unspendable_amount += unclaimed_rewards; - m_total_unspendables_unclaimed_rewards += unclaimed_rewards; + const CAmount temp_total_unspendable_amount{m_total_unspendables_genesis_block + m_total_unspendables_bip30 + m_total_unspendables_scripts + m_total_unspendables_unclaimed_rewards}; + const arith_uint256 unclaimed_rewards{(m_total_prevout_spent_amount + m_total_subsidy) - (m_total_new_outputs_ex_coinbase_amount + m_total_coinbase_amount + temp_total_unspendable_amount)}; + assert(unclaimed_rewards <= arith_uint256(std::numeric_limits::max())); + m_total_unspendables_unclaimed_rewards += static_cast(unclaimed_rewards.GetLow64()); std::pair value; value.first = block.hash; @@ -212,7 +228,6 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) value.second.bogo_size = m_bogo_size; value.second.total_amount = m_total_amount; value.second.total_subsidy = m_total_subsidy; - value.second.total_unspendable_amount = m_total_unspendable_amount; value.second.total_prevout_spent_amount = m_total_prevout_spent_amount; value.second.total_new_outputs_ex_coinbase_amount = m_total_new_outputs_ex_coinbase_amount; value.second.total_coinbase_amount = m_total_coinbase_amount; @@ -307,7 +322,6 @@ std::optional CoinStatsIndex::LookUpStats(const CBlockIndex& block_ 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; @@ -352,7 +366,6 @@ bool CoinStatsIndex::CustomInit(const std::optional& block m_bogo_size = entry.bogo_size; m_total_amount = entry.total_amount; m_total_subsidy = entry.total_subsidy; - m_total_unspendable_amount = entry.total_unspendable_amount; m_total_prevout_spent_amount = entry.total_prevout_spent_amount; m_total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount; m_total_coinbase_amount = entry.total_coinbase_amount; @@ -387,9 +400,6 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) { std::pair read_out; - const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())}; - m_total_subsidy -= block_subsidy; - // Ignore genesis block if (block.height > 0) { if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) { @@ -409,7 +419,8 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) } } - // Remove the new UTXOs that were created from the block + // Roll back muhash by removing the new UTXOs that were created by the + // block and reapplying the old UTXOs that were spent by the block assert(block.data); assert(block.undo_data); for (size_t i = 0; i < block.data->vtx.size(); ++i) { @@ -421,24 +432,9 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) const COutPoint outpoint{tx->GetHash(), j}; const Coin coin{out, block.height, is_coinbase}; - // Skip unspendable coins - if (coin.out.scriptPubKey.IsUnspendable()) { - m_total_unspendable_amount -= coin.out.nValue; - m_total_unspendables_scripts -= coin.out.nValue; - continue; + if (!coin.out.scriptPubKey.IsUnspendable()) { + RemoveCoinHash(m_muhash, outpoint, coin); } - - RemoveCoinHash(m_muhash, outpoint, coin); - - if (tx->IsCoinBase()) { - m_total_coinbase_amount -= coin.out.nValue; - } else { - m_total_new_outputs_ex_coinbase_amount -= coin.out.nValue; - } - - --m_transaction_output_count; - m_total_amount -= coin.out.nValue; - m_bogo_size -= GetBogoSize(coin.out.scriptPubKey); } // The coinbase tx has no undo data since no former output is spent @@ -446,40 +442,30 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) const auto& tx_undo{block.undo_data->vtxundo.at(i - 1)}; for (size_t j = 0; j < tx_undo.vprevout.size(); ++j) { - const Coin coin{tx_undo.vprevout[j]}; + const Coin& coin{tx_undo.vprevout[j]}; const COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; ApplyCoinHash(m_muhash, outpoint, coin); - - m_total_prevout_spent_amount -= coin.out.nValue; - - m_transaction_output_count++; - m_total_amount += coin.out.nValue; - m_bogo_size += GetBogoSize(coin.out.scriptPubKey); } } } - const CAmount unclaimed_rewards{(m_total_new_outputs_ex_coinbase_amount + m_total_coinbase_amount + m_total_unspendable_amount) - (m_total_prevout_spent_amount + m_total_subsidy)}; - m_total_unspendable_amount -= unclaimed_rewards; - m_total_unspendables_unclaimed_rewards -= unclaimed_rewards; - - // Check that the rolled back internal values are consistent with the DB read out + // Check that the rolled back muhash is consistent with the DB read out uint256 out; m_muhash.Finalize(out); Assert(read_out.second.muhash == out); - Assert(m_transaction_output_count == read_out.second.transaction_output_count); - Assert(m_total_amount == read_out.second.total_amount); - Assert(m_bogo_size == read_out.second.bogo_size); - Assert(m_total_subsidy == read_out.second.total_subsidy); - Assert(m_total_unspendable_amount == read_out.second.total_unspendable_amount); - Assert(m_total_prevout_spent_amount == read_out.second.total_prevout_spent_amount); - Assert(m_total_new_outputs_ex_coinbase_amount == read_out.second.total_new_outputs_ex_coinbase_amount); - Assert(m_total_coinbase_amount == read_out.second.total_coinbase_amount); - Assert(m_total_unspendables_genesis_block == read_out.second.total_unspendables_genesis_block); - Assert(m_total_unspendables_bip30 == read_out.second.total_unspendables_bip30); - Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts); - Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards); + // Apply the other values from the DB to the member variables + m_transaction_output_count = read_out.second.transaction_output_count; + m_total_amount = read_out.second.total_amount; + m_bogo_size = read_out.second.bogo_size; + m_total_subsidy = read_out.second.total_subsidy; + m_total_prevout_spent_amount = read_out.second.total_prevout_spent_amount; + m_total_new_outputs_ex_coinbase_amount = read_out.second.total_new_outputs_ex_coinbase_amount; + m_total_coinbase_amount = read_out.second.total_coinbase_amount; + m_total_unspendables_genesis_block = read_out.second.total_unspendables_genesis_block; + m_total_unspendables_bip30 = read_out.second.total_unspendables_bip30; + m_total_unspendables_scripts = read_out.second.total_unspendables_scripts; + m_total_unspendables_unclaimed_rewards = read_out.second.total_unspendables_unclaimed_rewards; return true; } diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index f2e95b449e..5dcbc18641 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_INDEX_COINSTATSINDEX_H #define BITCOIN_INDEX_COINSTATSINDEX_H +#include #include #include @@ -29,10 +30,9 @@ private: uint64_t m_bogo_size{0}; CAmount m_total_amount{0}; CAmount m_total_subsidy{0}; - CAmount m_total_unspendable_amount{0}; - CAmount m_total_prevout_spent_amount{0}; - CAmount m_total_new_outputs_ex_coinbase_amount{0}; - CAmount m_total_coinbase_amount{0}; + arith_uint256 m_total_prevout_spent_amount{0}; + arith_uint256 m_total_new_outputs_ex_coinbase_amount{0}; + arith_uint256 m_total_coinbase_amount{0}; CAmount m_total_unspendables_genesis_block{0}; CAmount m_total_unspendables_bip30{0}; CAmount m_total_unspendables_scripts{0}; diff --git a/src/kernel/coinstats.h b/src/kernel/coinstats.h index c0c363a842..8a782ed5af 100644 --- a/src/kernel/coinstats.h +++ b/src/kernel/coinstats.h @@ -5,6 +5,7 @@ #ifndef BITCOIN_KERNEL_COINSTATS_H #define BITCOIN_KERNEL_COINSTATS_H +#include #include #include #include @@ -50,14 +51,6 @@ struct CCoinsStats { //! 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 @@ -67,6 +60,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}; + // Despite containing amounts the following values use a uint256 type to prevent overflowing + + //! Total cumulative amount of prevouts spent up to and including this block + arith_uint256 total_prevout_spent_amount{0}; + //! Total cumulative amount of outputs created up to and including this block + arith_uint256 total_new_outputs_ex_coinbase_amount{0}; + //! Total cumulative amount of coinbase outputs up to and including this block + arith_uint256 total_coinbase_amount{0}; + CCoinsStats() = default; CCoinsStats(int block_height, const uint256& block_hash); }; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index cfc0379f68..bd5deedf6e 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1101,8 +1101,6 @@ static RPCHelpMan gettxoutsetinfo() ret.pushKV("transactions", static_cast(stats.nTransactions)); ret.pushKV("disk_size", stats.nDiskSize); } else { - ret.pushKV("total_unspendable_amount", ValueFromAmount(stats.total_unspendable_amount)); - CCoinsStats prev_stats{}; if (pindex->nHeight > 0) { const std::optional maybe_prev_stats = GetUTXOStats(coins_view, *blockman, hash_type, node.rpc_interruption_point, pindex->pprev, index_requested); @@ -1112,11 +1110,29 @@ static RPCHelpMan gettxoutsetinfo() prev_stats = maybe_prev_stats.value(); } + CAmount block_total_unspendable_amount = stats.total_unspendables_genesis_block + + stats.total_unspendables_bip30 + + stats.total_unspendables_scripts + + stats.total_unspendables_unclaimed_rewards; + CAmount prev_block_total_unspendable_amount = prev_stats.total_unspendables_genesis_block + + prev_stats.total_unspendables_bip30 + + prev_stats.total_unspendables_scripts + + prev_stats.total_unspendables_unclaimed_rewards; + + ret.pushKV("total_unspendable_amount", ValueFromAmount(block_total_unspendable_amount)); + UniValue block_info(UniValue::VOBJ); - block_info.pushKV("prevout_spent", ValueFromAmount(stats.total_prevout_spent_amount - prev_stats.total_prevout_spent_amount)); - block_info.pushKV("coinbase", ValueFromAmount(stats.total_coinbase_amount - prev_stats.total_coinbase_amount)); - block_info.pushKV("new_outputs_ex_coinbase", ValueFromAmount(stats.total_new_outputs_ex_coinbase_amount - prev_stats.total_new_outputs_ex_coinbase_amount)); - block_info.pushKV("unspendable", ValueFromAmount(stats.total_unspendable_amount - prev_stats.total_unspendable_amount)); + // These per-block values should fit uint64 under normal circumstances + arith_uint256 diff_prevout = stats.total_prevout_spent_amount - prev_stats.total_prevout_spent_amount; + arith_uint256 diff_coinbase = stats.total_coinbase_amount - prev_stats.total_coinbase_amount; + arith_uint256 diff_outputs = stats.total_new_outputs_ex_coinbase_amount - prev_stats.total_new_outputs_ex_coinbase_amount; + CAmount prevout_amount = static_cast(diff_prevout.GetLow64()); + CAmount coinbase_amount = static_cast(diff_coinbase.GetLow64()); + CAmount outputs_amount = static_cast(diff_outputs.GetLow64()); + block_info.pushKV("prevout_spent", ValueFromAmount(prevout_amount)); + block_info.pushKV("coinbase", ValueFromAmount(coinbase_amount)); + block_info.pushKV("new_outputs_ex_coinbase", ValueFromAmount(outputs_amount)); + block_info.pushKV("unspendable", ValueFromAmount(block_total_unspendable_amount - prev_block_total_unspendable_amount)); UniValue unspendables(UniValue::VOBJ); unspendables.pushKV("genesis_block", ValueFromAmount(stats.total_unspendables_genesis_block - prev_stats.total_unspendables_genesis_block)); diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py index a7b7e0c676..b9d41a9713 100755 --- a/test/functional/feature_init.py +++ b/test/functional/feature_init.py @@ -128,7 +128,7 @@ class InitTest(BitcoinTestFramework): 'startup_args': ['-txindex=1'], }, # Removing these files does not result in a startup error: - # 'indexes/blockfilter/basic/*.dat', 'indexes/blockfilter/basic/db/*.*', 'indexes/coinstats/db/*.*', + # 'indexes/blockfilter/basic/*.dat', 'indexes/blockfilter/basic/db/*.*', 'indexes/coinstatsindex/db/*.*', # 'indexes/txindex/*.log', 'indexes/txindex/CURRENT', 'indexes/txindex/LOCK' ] @@ -154,7 +154,7 @@ class InitTest(BitcoinTestFramework): 'startup_args': ['-blockfilterindex=1'], }, { - 'filepath_glob': 'indexes/coinstats/db/*.*', + 'filepath_glob': 'indexes/coinstatsindex/db/*.*', 'error_message': 'LevelDB error: Corruption', 'startup_args': ['-coinstatsindex=1'], }, From b2e8b64ddc351124ac1390ee906a8fcd2781ca50 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Sat, 16 Aug 2025 00:20:29 +0200 Subject: [PATCH 4/9] index, refactor: Append blocks to coinstatsindex without db read --- src/index/coinstatsindex.cpp | 14 +++++++------- src/index/coinstatsindex.h | 2 ++ 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index bb4be90bf4..e4daf9ef42 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -137,16 +137,12 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) // Ignore genesis block if (block.height > 0) { - std::pair read_out; - if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) { - return false; - } - uint256 expected_block_hash{*Assert(block.prev_hash)}; - if (read_out.first != expected_block_hash) { + if (m_current_block_hash != expected_block_hash) { LogWarning("previous block header belongs to unexpected block %s; expected %s", - read_out.first.ToString(), expected_block_hash.ToString()); + m_current_block_hash.ToString(), expected_block_hash.ToString()); + std::pair read_out; if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) { LogError("previous block header not found; expected %s", expected_block_hash.ToString()); @@ -240,6 +236,8 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) m_muhash.Finalize(out); value.second.muhash = out; + m_current_block_hash = block.hash; + // Intentionally do not update DB_MUHASH here so it stays in sync with // DB_BEST_BLOCK, and the index is not corrupted if there is an unclean shutdown. return m_db->Write(DBHeightKey(block.height), value); @@ -373,6 +371,7 @@ bool CoinStatsIndex::CustomInit(const std::optional& block m_total_unspendables_bip30 = entry.total_unspendables_bip30; m_total_unspendables_scripts = entry.total_unspendables_scripts; m_total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards; + m_current_block_hash = block->hash; } return true; @@ -466,6 +465,7 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) m_total_unspendables_bip30 = read_out.second.total_unspendables_bip30; m_total_unspendables_scripts = read_out.second.total_unspendables_scripts; m_total_unspendables_unclaimed_rewards = read_out.second.total_unspendables_unclaimed_rewards; + m_current_block_hash = *block.prev_hash; return true; } diff --git a/src/index/coinstatsindex.h b/src/index/coinstatsindex.h index 5dcbc18641..7e48f4c4ee 100644 --- a/src/index/coinstatsindex.h +++ b/src/index/coinstatsindex.h @@ -38,6 +38,8 @@ private: CAmount m_total_unspendables_scripts{0}; CAmount m_total_unspendables_unclaimed_rewards{0}; + uint256 m_current_block_hash{}; + [[nodiscard]] bool RevertBlock(const interfaces::BlockInfo& block); bool AllowPrune() const override { return true; } From bb8d673183294a43c716ff8738da2492f3d7a94b Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Mon, 2 Jun 2025 22:40:14 +0200 Subject: [PATCH 5/9] test: Add coinstatsindex compatibility test --- .../feature_coinstatsindex_compatibility.py | 64 +++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 65 insertions(+) create mode 100755 test/functional/feature_coinstatsindex_compatibility.py diff --git a/test/functional/feature_coinstatsindex_compatibility.py b/test/functional/feature_coinstatsindex_compatibility.py new file mode 100755 index 0000000000..357700a713 --- /dev/null +++ b/test/functional/feature_coinstatsindex_compatibility.py @@ -0,0 +1,64 @@ +#!/usr/bin/env python3 +# Copyright (c) 2025 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""Test coinstatsindex across node versions. + +This test may be removed some time after v29 has reached end of life. +""" + +import shutil + +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import assert_equal + + +class CoinStatsIndexTest(BitcoinTestFramework): + def set_test_params(self): + self.num_nodes = 2 + self.supports_cli = False + self.extra_args = [["-coinstatsindex"],["-coinstatsindex"]] + + def skip_test_if_missing_module(self): + self.skip_if_no_previous_releases() + + def setup_nodes(self): + self.add_nodes( + self.num_nodes, + extra_args=self.extra_args, + versions=[ + None, + 280200, + ], + ) + self.start_nodes() + + def run_test(self): + self._test_coin_stats_index_compatibility() + + def _test_coin_stats_index_compatibility(self): + node = self.nodes[0] + legacy_node = self.nodes[1] + for n in self.nodes: + self.wait_until(lambda: n.getindexinfo()['coinstatsindex']['synced'] is True) + + self.log.info("Test that gettxoutsetinfo() output is consistent between the different index versions") + res0 = node.gettxoutsetinfo('muhash') + res1 = legacy_node.gettxoutsetinfo('muhash') + assert_equal(res1, res0) + + self.log.info("Test that gettxoutsetinfo() output is consistent for the new index running on a datadir with the old version") + self.stop_nodes() + shutil.rmtree(node.chain_path / "indexes" / "coinstatsindex") + shutil.copytree(legacy_node.chain_path / "indexes" / "coinstats", node.chain_path / "indexes" / "coinstats") + old_version_path = node.chain_path / "indexes" / "coinstats" + msg = f'[warning] Old version of coinstatsindex found at {old_version_path}. This folder can be safely deleted unless you plan to downgrade your node to version 29 or lower.' + with node.assert_debug_log(expected_msgs=[msg]): + self.start_node(0, ['-coinstatsindex']) + self.wait_until(lambda: node.getindexinfo()['coinstatsindex']['synced'] is True) + res2 = node.gettxoutsetinfo('muhash') + assert_equal(res2, res0) + + +if __name__ == '__main__': + CoinStatsIndexTest(__file__).main() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index f2e514a7a4..94965618cd 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -342,6 +342,7 @@ BASE_SCRIPTS = [ 'feature_anchors.py', 'mempool_datacarrier.py', 'feature_coinstatsindex.py', + 'feature_coinstatsindex_compatibility.py', 'wallet_orphanedreward.py', 'wallet_timelock.py', 'p2p_permissions.py', From 51df9de8e5b9c8ecd8339d95b630f312fcb9414e Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Mon, 14 Jul 2025 16:36:01 +0200 Subject: [PATCH 6/9] doc: Add release note for 30469 --- doc/release-notes-30469.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 doc/release-notes-30469.md diff --git a/doc/release-notes-30469.md b/doc/release-notes-30469.md new file mode 100644 index 0000000000..a1f6045960 --- /dev/null +++ b/doc/release-notes-30469.md @@ -0,0 +1,4 @@ +Indexes +------- + +- The implementation of coinstatsindex was changed to prevent an overflow bug that could already be observed on the default Signet. The new version of the index will need to be synced from scratch when starting the upgraded node for the first time. The new version is stored in `/indexes/coinstatsindex/` in contrast to the old version which was stored at `/indexes/coinstats/`. The old version of the index is not deleted by the upgraded node in case the user chooses to downgrade their node in the future. If the user does not plan to downgrade it is safe for them to remove `/indexes/coinstats/` from their datadir. A future release of Bitcoin Core may remove the old version of the index automatically. From 37c4fba1f4c18632bceb16f41f5a8f1a61fb9096 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Tue, 26 Aug 2025 00:21:24 +0100 Subject: [PATCH 7/9] index: Check BIP30 blocks when rewinding Coinstatsindex This is practically irrelevant due to the unlikeliness of a re-org reaching so deep that it would drop the BIP30 blocks from the chain (91842 and 91880). However this serves as documentation and ensures that the functions RevertBlock and CustomAppend are consistent. --- src/index/coinstatsindex.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index e4daf9ef42..24b1837bb0 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -426,6 +426,10 @@ bool CoinStatsIndex::RevertBlock(const interfaces::BlockInfo& block) const auto& tx{block.data->vtx.at(i)}; const bool is_coinbase{tx->IsCoinBase()}; + if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) { + continue; + } + for (uint32_t j = 0; j < tx->vout.size(); ++j) { const CTxOut& out{tx->vout[j]}; const COutPoint outpoint{tx->GetHash(), j}; From 54dc34ec2279378c78fa2d9155668e39e20decda Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Mon, 1 Sep 2025 23:26:44 +0200 Subject: [PATCH 8/9] index: Remove unused coinstatsindex recovery code The coinstatsindex currently looks for block data at a hash key if the prev block in CustomAppend is different than expected. This is not needed since base index should always prevent us ending up in this scenario since it should rewind the index before calling CustomAppend in this case. But even if we run into this and our belt-and-suspenders code is getting hit, the index could not recover properly from the hash key index data so it can be removed without any real impact. --- src/index/coinstatsindex.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 24b1837bb0..3d09225489 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -139,15 +139,9 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) if (block.height > 0) { uint256 expected_block_hash{*Assert(block.prev_hash)}; if (m_current_block_hash != expected_block_hash) { - LogWarning("previous block header belongs to unexpected block %s; expected %s", + LogError("previous block header belongs to unexpected block %s; expected %s", m_current_block_hash.ToString(), expected_block_hash.ToString()); - - std::pair read_out; - if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) { - LogError("previous block header not found; expected %s", - expected_block_hash.ToString()); - return false; - } + return false; } // Add the new utxos created from the block From c76797481155754329ec6a6f58e8402569043944 Mon Sep 17 00:00:00 2001 From: Fabian Jahr Date: Thu, 4 Sep 2025 18:34:58 +0200 Subject: [PATCH 9/9] clang-tidy: Fix critical warnings The std::move in coinstatsindex was not necessary since it was passed as a const reference argument. The other change in the utxo supply fuzz test changes a line that seems to have triggered a false alarm. --- src/index/coinstatsindex.cpp | 2 +- src/test/fuzz/utxo_total_supply.cpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp index 3d09225489..af798e2913 100644 --- a/src/index/coinstatsindex.cpp +++ b/src/index/coinstatsindex.cpp @@ -256,7 +256,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block) return false; } - batch.Write(DBHashKey(value.first), std::move(value.second)); + batch.Write(DBHashKey(value.first), value.second); return true; } diff --git a/src/test/fuzz/utxo_total_supply.cpp b/src/test/fuzz/utxo_total_supply.cpp index d9382ca831..1348962ad5 100644 --- a/src/test/fuzz/utxo_total_supply.cpp +++ b/src/test/fuzz/utxo_total_supply.cpp @@ -153,7 +153,7 @@ FUZZ_TARGET(utxo_total_supply) node::RegenerateCommitments(*current_block, chainman); const bool was_valid = !MineBlock(node, current_block).IsNull(); - const auto prev_utxo_stats = utxo_stats; + const uint256 prev_hash_serialized{utxo_stats.hashSerialized}; if (was_valid) { if (duplicate_coinbase_height == ActiveHeight()) { // we mined the duplicate coinbase @@ -167,7 +167,7 @@ FUZZ_TARGET(utxo_total_supply) if (!was_valid) { // utxo stats must not change - assert(prev_utxo_stats.hashSerialized == utxo_stats.hashSerialized); + assert(prev_hash_serialized == utxo_stats.hashSerialized); } current_block = PrepareNextBlock();