Merge bitcoin/bitcoin#22047: index, rpc: Coinstatsindex follow-ups

779e638 coinstats: Add comments for new coinstatsindex values (Fabian Jahr)
5b3d4e7 Index: Improve logging in coinstatsindex (Fabian Jahr)
d4356d4 rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo (Fabian Jahr)
a5f6791 rpc: Add missing gettxoutsetinfo help docs (Fabian Jahr)
01386bf Index: Return early from failed coinstatsindex init (Fabian Jahr)
1e38423 index: Use batch writing in coinstatsindex WriteBlock (Fabian Jahr)
fb65dde scripted-diff: Fix coinstats data member names (Fabian Jahr)
8ea8c92 index: Avoid unnecessary type casts in coinstatsindex (Fabian Jahr)

Pull request description:

  This is a collection of smaller follow-ups to #19521, addressing several post-merge review comments.

ACKs for top commit:
  Sjors:
    re-utACK 779e638
  jonatack:
    re-ACK 779e638 diff since last review involves doc changes only; rebased to current master and verified clean debug build/no silent conflicts, unit tests, and feature_coinstatsindex functional test
  laanwj:
    Code review ACK 779e638
  Talkless:
    re-utACK 779e638 after cosmetic changes.

Tree-SHA512: cb0d038d230c582d7fe3041c89b1e04d39971fab3739d540c609cf826754c6c513b12ded08ac92180aec7a9d7a70114ece50357bd1a902de4adaae9f30b8d699

Author: laanwj

src/index/coinstatsindex.cpp

@@ -24,14 +24,14 @@ struct DBVal {
     uint64_t bogo_size;
     CAmount total_amount;
     CAmount total_subsidy;
-    CAmount block_unspendable_amount;
-    CAmount block_prevout_spent_amount;
-    CAmount block_new_outputs_ex_coinbase_amount;
-    CAmount block_coinbase_amount;
-    CAmount unspendables_genesis_block;
-    CAmount unspendables_bip30;
-    CAmount unspendables_scripts;
-    CAmount unspendables_unclaimed_rewards;
+    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;
 
     SERIALIZE_METHODS(DBVal, obj)
     {
@@ -40,14 +40,14 @@ struct DBVal {
         READWRITE(obj.bogo_size);
         READWRITE(obj.total_amount);
         READWRITE(obj.total_subsidy);
-        READWRITE(obj.block_unspendable_amount);
-        READWRITE(obj.block_prevout_spent_amount);
-        READWRITE(obj.block_new_outputs_ex_coinbase_amount);
-        READWRITE(obj.block_coinbase_amount);
-        READWRITE(obj.unspendables_genesis_block);
-        READWRITE(obj.unspendables_bip30);
-        READWRITE(obj.unspendables_scripts);
-        READWRITE(obj.unspendables_unclaimed_rewards);
+        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(obj.total_unspendables_genesis_block);
+        READWRITE(obj.total_unspendables_bip30);
+        READWRITE(obj.total_unspendables_scripts);
+        READWRITE(obj.total_unspendables_unclaimed_rewards);
     }
 };
 
@@ -122,9 +122,12 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
 
         uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
         if (read_out.first != expected_block_hash) {
+            LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
+                      read_out.first.ToString(), expected_block_hash.ToString());
+
             if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
-                return error("%s: previous block header belongs to unexpected block %s; expected %s",
-                             __func__, read_out.first.ToString(), expected_block_hash.ToString());
+                return error("%s: previous block header not found; expected %s",
+                             __func__, expected_block_hash.ToString());
             }
         }
 
@@ -138,29 +141,29 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
 
             // Skip duplicate txid coinbase transactions (BIP30).
             if (is_bip30_block && tx->IsCoinBase()) {
-                m_block_unspendable_amount += block_subsidy;
-                m_unspendables_bip30 += block_subsidy;
+                m_total_unspendable_amount += block_subsidy;
+                m_total_unspendables_bip30 += block_subsidy;
                 continue;
             }
 
-            for (size_t j = 0; j < tx->vout.size(); ++j) {
+            for (uint32_t j = 0; j < tx->vout.size(); ++j) {
                 const CTxOut& out{tx->vout[j]};
                 Coin coin{out, pindex->nHeight, tx->IsCoinBase()};
-                COutPoint outpoint{tx->GetHash(), static_cast<uint32_t>(j)};
+                COutPoint outpoint{tx->GetHash(), j};
 
                 // Skip unspendable coins
                 if (coin.out.scriptPubKey.IsUnspendable()) {
-                    m_block_unspendable_amount += coin.out.nValue;
-                    m_unspendables_scripts += coin.out.nValue;
+                    m_total_unspendable_amount += coin.out.nValue;
+                    m_total_unspendables_scripts += coin.out.nValue;
                     continue;
                 }
 
                 m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
 
                 if (tx->IsCoinBase()) {
-                    m_block_coinbase_amount += coin.out.nValue;
+                    m_total_coinbase_amount += coin.out.nValue;
                 } else {
-                    m_block_new_outputs_ex_coinbase_amount += coin.out.nValue;
+                    m_total_new_outputs_ex_coinbase_amount += coin.out.nValue;
                 }
 
                 ++m_transaction_output_count;
@@ -178,7 +181,7 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
 
                     m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));
 
-                    m_block_prevout_spent_amount += coin.out.nValue;
+                    m_total_prevout_spent_amount += coin.out.nValue;
 
                     --m_transaction_output_count;
                     m_total_amount -= coin.out.nValue;
@@ -188,38 +191,41 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
         }
     } else {
         // genesis block
-        m_block_unspendable_amount += block_subsidy;
-        m_unspendables_genesis_block += block_subsidy;
+        m_total_unspendable_amount += block_subsidy;
+        m_total_unspendables_genesis_block += block_subsidy;
     }
 
     // If spent prevouts + block subsidy are still a higher amount than
     // 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_block_prevout_spent_amount + m_total_subsidy) - (m_block_new_outputs_ex_coinbase_amount + m_block_coinbase_amount + m_block_unspendable_amount)};
-    m_block_unspendable_amount += unclaimed_rewards;
-    m_unspendables_unclaimed_rewards += unclaimed_rewards;
+    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;
 
     std::pair<uint256, DBVal> value;
     value.first = pindex->GetBlockHash();
     value.second.transaction_output_count = m_transaction_output_count;
     value.second.bogo_size = m_bogo_size;
     value.second.total_amount = m_total_amount;
     value.second.total_subsidy = m_total_subsidy;
-    value.second.block_unspendable_amount = m_block_unspendable_amount;
-    value.second.block_prevout_spent_amount = m_block_prevout_spent_amount;
-    value.second.block_new_outputs_ex_coinbase_amount = m_block_new_outputs_ex_coinbase_amount;
-    value.second.block_coinbase_amount = m_block_coinbase_amount;
-    value.second.unspendables_genesis_block = m_unspendables_genesis_block;
-    value.second.unspendables_bip30 = m_unspendables_bip30;
-    value.second.unspendables_scripts = m_unspendables_scripts;
-    value.second.unspendables_unclaimed_rewards = m_unspendables_unclaimed_rewards;
+    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;
+    value.second.total_unspendables_genesis_block = m_total_unspendables_genesis_block;
+    value.second.total_unspendables_bip30 = m_total_unspendables_bip30;
+    value.second.total_unspendables_scripts = m_total_unspendables_scripts;
+    value.second.total_unspendables_unclaimed_rewards = m_total_unspendables_unclaimed_rewards;
 
     uint256 out;
     m_muhash.Finalize(out);
     value.second.muhash = out;
 
-    return m_db->Write(DBHeightKey(pindex->nHeight), value) && m_db->Write(DB_MUHASH, m_muhash);
+    CDBBatch batch(*m_db);
+    batch.Write(DBHeightKey(pindex->nHeight), value);
+    batch.Write(DB_MUHASH, m_muhash);
+    return m_db->WriteBatch(batch);
 }
 
 static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
@@ -317,14 +323,14 @@ bool CoinStatsIndex::LookUpStats(const CBlockIndex* block_index, CCoinsStats& co
     coins_stats.nBogoSize = entry.bogo_size;
     coins_stats.nTotalAmount = entry.total_amount;
     coins_stats.total_subsidy = entry.total_subsidy;
-    coins_stats.block_unspendable_amount = entry.block_unspendable_amount;
-    coins_stats.block_prevout_spent_amount = entry.block_prevout_spent_amount;
-    coins_stats.block_new_outputs_ex_coinbase_amount = entry.block_new_outputs_ex_coinbase_amount;
-    coins_stats.block_coinbase_amount = entry.block_coinbase_amount;
-    coins_stats.unspendables_genesis_block = entry.unspendables_genesis_block;
-    coins_stats.unspendables_bip30 = entry.unspendables_bip30;
-    coins_stats.unspendables_scripts = entry.unspendables_scripts;
-    coins_stats.unspendables_unclaimed_rewards = entry.unspendables_unclaimed_rewards;
+    coins_stats.total_unspendable_amount = entry.total_unspendable_amount;
+    coins_stats.total_prevout_spent_amount = entry.total_prevout_spent_amount;
+    coins_stats.total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount;
+    coins_stats.total_coinbase_amount = entry.total_coinbase_amount;
+    coins_stats.total_unspendables_genesis_block = entry.total_unspendables_genesis_block;
+    coins_stats.total_unspendables_bip30 = entry.total_unspendables_bip30;
+    coins_stats.total_unspendables_scripts = entry.total_unspendables_scripts;
+    coins_stats.total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards;
 
     return true;
 }
@@ -341,33 +347,31 @@ bool CoinStatsIndex::Init()
         }
     }
 
-    if (BaseIndex::Init()) {
-        const CBlockIndex* pindex{CurrentIndex()};
+    if (!BaseIndex::Init()) return false;
 
-        if (pindex) {
-            DBVal entry;
-            if (!LookUpOne(*m_db, pindex, entry)) {
-                return false;
-            }
+    const CBlockIndex* pindex{CurrentIndex()};
 
-            m_transaction_output_count = entry.transaction_output_count;
-            m_bogo_size = entry.bogo_size;
-            m_total_amount = entry.total_amount;
-            m_total_subsidy = entry.total_subsidy;
-            m_block_unspendable_amount = entry.block_unspendable_amount;
-            m_block_prevout_spent_amount = entry.block_prevout_spent_amount;
-            m_block_new_outputs_ex_coinbase_amount = entry.block_new_outputs_ex_coinbase_amount;
-            m_block_coinbase_amount = entry.block_coinbase_amount;
-            m_unspendables_genesis_block = entry.unspendables_genesis_block;
-            m_unspendables_bip30 = entry.unspendables_bip30;
-            m_unspendables_scripts = entry.unspendables_scripts;
-            m_unspendables_unclaimed_rewards = entry.unspendables_unclaimed_rewards;
+    if (pindex) {
+        DBVal entry;
+        if (!LookUpOne(*m_db, pindex, entry)) {
+            return false;
         }
 
-        return true;
+        m_transaction_output_count = entry.transaction_output_count;
+        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;
+        m_total_unspendables_genesis_block = entry.total_unspendables_genesis_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;
     }
 
-    return false;
+    return true;
 }
 
 // Reverse a single block as part of a reorg
@@ -391,9 +395,12 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
 
         uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
         if (read_out.first != expected_block_hash) {
+            LogPrintf("WARNING: previous block header belongs to unexpected block %s; expected %s\n",
+                      read_out.first.ToString(), expected_block_hash.ToString());
+
             if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
-                return error("%s: previous block header belongs to unexpected block %s; expected %s",
-                             __func__, read_out.first.ToString(), expected_block_hash.ToString());
+                return error("%s: previous block header not found; expected %s",
+                             __func__, expected_block_hash.ToString());
             }
         }
     }
@@ -402,24 +409,24 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
     for (size_t i = 0; i < block.vtx.size(); ++i) {
         const auto& tx{block.vtx.at(i)};
 
-        for (size_t j = 0; j < tx->vout.size(); ++j) {
+        for (uint32_t j = 0; j < tx->vout.size(); ++j) {
             const CTxOut& out{tx->vout[j]};
-            COutPoint outpoint{tx->GetHash(), static_cast<uint32_t>(j)};
+            COutPoint outpoint{tx->GetHash(), j};
             Coin coin{out, pindex->nHeight, tx->IsCoinBase()};
 
             // Skip unspendable coins
             if (coin.out.scriptPubKey.IsUnspendable()) {
-                m_block_unspendable_amount -= coin.out.nValue;
-                m_unspendables_scripts -= coin.out.nValue;
+                m_total_unspendable_amount -= coin.out.nValue;
+                m_total_unspendables_scripts -= coin.out.nValue;
                 continue;
             }
 
             m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));
 
             if (tx->IsCoinBase()) {
-                m_block_coinbase_amount -= coin.out.nValue;
+                m_total_coinbase_amount -= coin.out.nValue;
             } else {
-                m_block_new_outputs_ex_coinbase_amount -= coin.out.nValue;
+                m_total_new_outputs_ex_coinbase_amount -= coin.out.nValue;
             }
 
             --m_transaction_output_count;
@@ -437,7 +444,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
 
                 m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
 
-                m_block_prevout_spent_amount -= coin.out.nValue;
+                m_total_prevout_spent_amount -= coin.out.nValue;
 
                 m_transaction_output_count++;
                 m_total_amount += coin.out.nValue;
@@ -446,9 +453,9 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
         }
     }
 
-    const CAmount unclaimed_rewards{(m_block_new_outputs_ex_coinbase_amount + m_block_coinbase_amount + m_block_unspendable_amount) - (m_block_prevout_spent_amount + m_total_subsidy)};
-    m_block_unspendable_amount -= unclaimed_rewards;
-    m_unspendables_unclaimed_rewards -= unclaimed_rewards;
+    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
     uint256 out;
@@ -459,14 +466,14 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
     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_block_unspendable_amount == read_out.second.block_unspendable_amount);
-    Assert(m_block_prevout_spent_amount == read_out.second.block_prevout_spent_amount);
-    Assert(m_block_new_outputs_ex_coinbase_amount == read_out.second.block_new_outputs_ex_coinbase_amount);
-    Assert(m_block_coinbase_amount == read_out.second.block_coinbase_amount);
-    Assert(m_unspendables_genesis_block == read_out.second.unspendables_genesis_block);
-    Assert(m_unspendables_bip30 == read_out.second.unspendables_bip30);
-    Assert(m_unspendables_scripts == read_out.second.unspendables_scripts);
-    Assert(m_unspendables_unclaimed_rewards == read_out.second.unspendables_unclaimed_rewards);
+    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);
 
     return m_db->Write(DB_MUHASH, m_muhash);
 }

src/index/coinstatsindex.h

@@ -25,14 +25,14 @@ class CoinStatsIndex final : public BaseIndex
     uint64_t m_bogo_size{0};
     CAmount m_total_amount{0};
     CAmount m_total_subsidy{0};
-    CAmount m_block_unspendable_amount{0};
-    CAmount m_block_prevout_spent_amount{0};
-    CAmount m_block_new_outputs_ex_coinbase_amount{0};
-    CAmount m_block_coinbase_amount{0};
-    CAmount m_unspendables_genesis_block{0};
-    CAmount m_unspendables_bip30{0};
-    CAmount m_unspendables_scripts{0};
-    CAmount m_unspendables_unclaimed_rewards{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};
+    CAmount m_total_unspendables_genesis_block{0};
+    CAmount m_total_unspendables_bip30{0};
+    CAmount m_total_unspendables_scripts{0};
+    CAmount m_total_unspendables_unclaimed_rewards{0};
 
     bool ReverseBlock(const CBlock& block, const CBlockIndex* pindex);
 

src/node/coinstats.h

@@ -45,15 +45,25 @@ struct CCoinsStats
     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};
-    CAmount block_unspendable_amount{0};
-    CAmount block_prevout_spent_amount{0};
-    CAmount block_new_outputs_ex_coinbase_amount{0};
-    CAmount block_coinbase_amount{0};
-    CAmount unspendables_genesis_block{0};
-    CAmount unspendables_bip30{0};
-    CAmount unspendables_scripts{0};
-    CAmount unspendables_unclaimed_rewards{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(CoinStatsHashType hash_type) : m_hash_type(hash_type) {}
 };