refactor, index: Remove member variables in coinstatsindex #33134

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2025-08-csi-no-members changing 2 files +68 −128
  1. fjahr commented at 4:21 pm on August 4, 2025: contributor

    This picks up a review comment from #30469 which suggested to remove the member variables in coinstatsindex and replace them with local variables where needed.

    This is a pure refactor and performance should not be impacted because there is no increase in disk reads or writes.

  2. refactor, index: Remove member variables in coinstatsindex a53063a6d9
  3. DrahtBot commented at 4:21 pm on August 4, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33134.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK jonatack
    Concept ACK mzumsande, l0rinc, Sammie05

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30469 (index: Fix coinstats overflow by fjahr)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. jonatack commented at 5:14 pm on August 4, 2025: member

    Review ACK a53063a6d93103926509d3f95dc791140aae58c6

    Nice cleanup.

  5. in src/index/coinstatsindex.cpp:361 in a53063a6d9
    384-        m_total_new_outputs_ex_coinbase_amount = entry.total_new_outputs_ex_coinbase_amount;
    385-        m_total_coinbase_amount = entry.total_coinbase_amount;
    386-        m_total_unspendables_genesis_block = entry.total_unspendables_genesis_block;
    387-        m_total_unspendables_bip30 = entry.total_unspendables_bip30;
    388-        m_total_unspendables_scripts = entry.total_unspendables_scripts;
    389-        m_total_unspendables_unclaimed_rewards = entry.total_unspendables_unclaimed_rewards;
    


    l0rinc commented at 6:28 pm on August 4, 2025:

    Is it still an init now that it just updates m_muhash? The method doc claims that: “/// Initialize internal state from the database and block index” - is that still accurate?

    Since this change is not strictly inlining of fields anymore, please consider doing it in a separate commit where the commit message gives more context.

  6. in src/index/coinstatsindex.cpp:164 in a53063a6d9
    159+        total_unspendables_scripts = read_out.second.total_unspendables_scripts;
    160+        total_unspendables_unclaimed_rewards = read_out.second.total_unspendables_unclaimed_rewards;
    161+    }
    162+
    163+    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
    164+    total_subsidy += block_subsidy;
    


    l0rinc commented at 6:47 pm on August 4, 2025:

    What’s the reason for doing these calculations in the middle of the method - is it to avoid calculating the subsidy when there’s a read error? I wouldn’t optimize for that usecase: leaving the block_subsidy declaration at the beginning and only updating total_subsidy at the very end would avoid repeating the block.height > 0 condition and the diff would also be smaller.

    Also, since we’re reading and writing a DBVal anyway, could use that to hold the state instead of keeping track of the individual primitives and copying back and forth? I mean something like:

      0diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
      1--- a/src/index/coinstatsindex.cpp	(revision a53063a6d93103926509d3f95dc791140aae58c6)
      2+++ b/src/index/coinstatsindex.cpp	(date 1754333742030)
      3@@ -114,56 +114,27 @@
      4 
      5 bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
      6 {
      7-    uint64_t transaction_output_count{0};
      8-    uint64_t bogo_size{0};
      9-    CAmount total_amount{0};
     10-    CAmount total_subsidy{0};
     11-    CAmount total_unspendable_amount{0};
     12-    CAmount total_prevout_spent_amount{0};
     13-    CAmount total_new_outputs_ex_coinbase_amount{0};
     14-    CAmount total_coinbase_amount{0};
     15-    CAmount total_unspendables_genesis_block{0};
     16-    CAmount total_unspendables_bip30{0};
     17-    CAmount total_unspendables_scripts{0};
     18-    CAmount total_unspendables_unclaimed_rewards{0};
     19+    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
     20+    std::pair<uint256, DBVal> val{};
     21 
     22-    // Ignore genesis block
     23     if (block.height > 0) {
     24-        std::pair<uint256, DBVal> read_out;
     25-        if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) {
     26+        if (!m_db->Read(DBHeightKey(block.height - 1), val)) {
     27             return false;
     28         }
     29 
     30         uint256 expected_block_hash{*Assert(block.prev_hash)};
     31-        if (read_out.first != expected_block_hash) {
     32+        if (val.first != expected_block_hash) {
     33             LogWarning("previous block header belongs to unexpected block %s; expected %s",
     34-                      read_out.first.ToString(), expected_block_hash.ToString());
     35+                      val.first.ToString(), expected_block_hash.ToString());
     36 
     37-            if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
     38+            if (!m_db->Read(DBHashKey(expected_block_hash), val)) {
     39                 LogError("previous block header not found; expected %s",
     40                           expected_block_hash.ToString());
     41                 return false;
     42             }
     43         }
     44 
     45-        transaction_output_count = read_out.second.transaction_output_count;
     46-        bogo_size = read_out.second.bogo_size;
     47-        total_amount = read_out.second.total_amount;
     48-        total_subsidy = read_out.second.total_subsidy;
     49-        total_unspendable_amount = read_out.second.total_unspendable_amount;
     50-        total_prevout_spent_amount = read_out.second.total_prevout_spent_amount;
     51-        total_new_outputs_ex_coinbase_amount = read_out.second.total_new_outputs_ex_coinbase_amount;
     52-        total_coinbase_amount = read_out.second.total_coinbase_amount;
     53-        total_unspendables_genesis_block = read_out.second.total_unspendables_genesis_block;
     54-        total_unspendables_bip30 = read_out.second.total_unspendables_bip30;
     55-        total_unspendables_scripts = read_out.second.total_unspendables_scripts;
     56-        total_unspendables_unclaimed_rewards = read_out.second.total_unspendables_unclaimed_rewards;
     57-    }
     58-
     59-    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
     60-    total_subsidy += block_subsidy;
     61-
     62-    if (block.height > 0) {
     63         // Add the new utxos created from the block
     64         assert(block.data);
     65         for (size_t i = 0; i < block.data->vtx.size(); ++i) {
     66@@ -171,8 +142,8 @@
     67 
     68             // Skip duplicate txid coinbase transactions (BIP30).
     69             if (IsBIP30Unspendable(block.hash, block.height) && tx->IsCoinBase()) {
     70-                total_unspendable_amount += block_subsidy;
     71-                total_unspendables_bip30 += block_subsidy;
     72+                val.second.total_unspendable_amount += block_subsidy;
     73+                val.second.total_unspendables_bip30 += block_subsidy;
     74                 continue;
     75             }
     76 
     77@@ -183,22 +154,22 @@
     78 
     79                 // Skip unspendable coins
     80                 if (coin.out.scriptPubKey.IsUnspendable()) {
     81-                    total_unspendable_amount += coin.out.nValue;
     82-                    total_unspendables_scripts += coin.out.nValue;
     83+                    val.second.total_unspendable_amount += coin.out.nValue;
     84+                    val.second.total_unspendables_scripts += coin.out.nValue;
     85                     continue;
     86                 }
     87 
     88                 ApplyCoinHash(m_muhash, outpoint, coin);
     89 
     90                 if (tx->IsCoinBase()) {
     91-                    total_coinbase_amount += coin.out.nValue;
     92+                    val.second.total_coinbase_amount += coin.out.nValue;
     93                 } else {
     94-                    total_new_outputs_ex_coinbase_amount += coin.out.nValue;
     95+                    val.second.total_new_outputs_ex_coinbase_amount += coin.out.nValue;
     96                 }
     97 
     98-                ++transaction_output_count;
     99-                total_amount += coin.out.nValue;
    100-                bogo_size += GetBogoSize(coin.out.scriptPubKey);
    101+                ++val.second.transaction_output_count;
    102+                val.second.total_amount += coin.out.nValue;
    103+                val.second.bogo_size += GetBogoSize(coin.out.scriptPubKey);
    104             }
    105 
    106             // The coinbase tx has no undo data since no former output is spent
    107@@ -211,50 +182,39 @@
    108 
    109                     RemoveCoinHash(m_muhash, outpoint, coin);
    110 
    111-                    total_prevout_spent_amount += coin.out.nValue;
    112+                    val.second.total_prevout_spent_amount += coin.out.nValue;
    113 
    114-                    --transaction_output_count;
    115-                    total_amount -= coin.out.nValue;
    116-                    bogo_size -= GetBogoSize(coin.out.scriptPubKey);
    117+                    --val.second.transaction_output_count;
    118+                    val.second.total_amount -= coin.out.nValue;
    119+                    val.second.bogo_size -= GetBogoSize(coin.out.scriptPubKey);
    120                 }
    121             }
    122         }
    123     } else {
    124         // genesis block
    125-        total_unspendable_amount += block_subsidy;
    126-        total_unspendables_genesis_block += block_subsidy;
    127+        val.second.total_unspendable_amount += block_subsidy;
    128+        val.second.total_unspendables_genesis_block += block_subsidy;
    129     }
    130+
    131+    val.second.total_subsidy += block_subsidy;
    132 
    133     // If spent prevouts + block subsidy are still a higher amount than
    134     // new outputs + coinbase + current unspendable amount this means
    135     // the miner did not claim the full block reward. Unclaimed block
    136     // rewards are also unspendable.
    137-    const CAmount unclaimed_rewards{(total_prevout_spent_amount + total_subsidy) - (total_new_outputs_ex_coinbase_amount + total_coinbase_amount + total_unspendable_amount)};
    138-    total_unspendable_amount += unclaimed_rewards;
    139-    total_unspendables_unclaimed_rewards += unclaimed_rewards;
    140+    const CAmount unclaimed_rewards{(val.second.total_prevout_spent_amount + val.second.total_subsidy) - (val.second.total_new_outputs_ex_coinbase_amount + val.second.total_coinbase_amount + val.second.total_unspendable_amount)};
    141+    val.second.total_unspendable_amount += unclaimed_rewards;
    142+    val.second.total_unspendables_unclaimed_rewards += unclaimed_rewards;
    143 
    144-    std::pair<uint256, DBVal> value;
    145-    value.first = block.hash;
    146-    value.second.transaction_output_count = transaction_output_count;
    147-    value.second.bogo_size = bogo_size;
    148-    value.second.total_amount = total_amount;
    149-    value.second.total_subsidy = total_subsidy;
    150-    value.second.total_unspendable_amount = total_unspendable_amount;
    151-    value.second.total_prevout_spent_amount = total_prevout_spent_amount;
    152-    value.second.total_new_outputs_ex_coinbase_amount = total_new_outputs_ex_coinbase_amount;
    153-    value.second.total_coinbase_amount = total_coinbase_amount;
    154-    value.second.total_unspendables_genesis_block = total_unspendables_genesis_block;
    155-    value.second.total_unspendables_bip30 = total_unspendables_bip30;
    156-    value.second.total_unspendables_scripts = total_unspendables_scripts;
    157-    value.second.total_unspendables_unclaimed_rewards = total_unspendables_unclaimed_rewards;
    158+    val.first = block.hash;
    159 
    160     uint256 out;
    161     m_muhash.Finalize(out);
    162-    value.second.muhash = out;
    163+    val.second.muhash = out;
    164 
    165     // Intentionally do not update DB_MUHASH here so it stays in sync with
    166     // DB_BEST_BLOCK, and the index is not corrupted if there is an unclean shutdown.
    167-    return m_db->Write(DBHeightKey(block.height), value);
    168+    return m_db->Write(DBHeightKey(block.height), val);
    169 }
    170 
    171 [[nodiscard]] static bool CopyHeightIndexToHashIndex(CDBIterator& db_it, CDBBatch& batch,
    

    (val.second can of course be extracted if you find it harder to read)

    Also note that return m_db->Write(DBHeightKey(block.height), val); is always true currently, it’s not actually returning false on error, see: #33042)

  7. mzumsande commented at 6:57 pm on August 4, 2025: contributor
    Concept ACK
  8. furszy commented at 7:05 pm on August 4, 2025: member
    Based on #30469 code; I would only remove the per-block values from the class and leave the total_* ones there. Mainly because these values effectively cache the last state, which will lets us remove the need for reading the previous record from disk that we do on every connected block (reading from disk is slower than caching a few accumulators at the class level).
  9. in src/index/coinstatsindex.cpp:461 in a53063a6d9
    437-                m_bogo_size += GetBogoSize(coin.out.scriptPubKey);
    438             }
    439         }
    440     }
    441 
    442-    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)};
    


    l0rinc commented at 7:16 pm on August 4, 2025:
    it’s not immediately obvious how these changes are related to inlining of fields (and why we don’t need to keep the accounting at all) - could you please extract it to a separate commit which explains what’s happening here? Bundling low-risk changes in the same commit as high-risk changes makes review harder.
  10. in src/index/coinstatsindex.cpp:400 in a53063a6d9
    398@@ -384,59 +399,25 @@ interfaces::Chain::NotifyOptions CoinStatsIndex::CustomOptions()
    399 // Reverse a single block as part of a reorg
    400 bool CoinStatsIndex::ReverseBlock(const interfaces::BlockInfo& block)
    


    l0rinc commented at 7:28 pm on August 4, 2025:
    I know you didn’t write this, but maybe this is a good time to clarify that we’re not strictly changing the order of something (i.e. reversing), but undoing/rewinding/reverting/rolling back.
  11. in src/index/coinstatsindex.cpp:435 in a53063a6d9
    459-    Assert(m_total_unspendables_genesis_block == read_out.second.total_unspendables_genesis_block);
    460-    Assert(m_total_unspendables_bip30 == read_out.second.total_unspendables_bip30);
    461-    Assert(m_total_unspendables_scripts == read_out.second.total_unspendables_scripts);
    462-    Assert(m_total_unspendables_unclaimed_rewards == read_out.second.total_unspendables_unclaimed_rewards);
    463-
    464     return true;
    


    l0rinc commented at 7:31 pm on August 4, 2025:
    After the refactor every path returns true, the function can now be void.
  12. in src/index/coinstatsindex.cpp:409 in a53063a6d9
    429     assert(block.data);
    430     assert(block.undo_data);
    431     for (size_t i = 0; i < block.data->vtx.size(); ++i) {
    432         const auto& tx{block.data->vtx.at(i)};
    433 
    434+        if (IsBIP30Unspendable(block.hash, block.height) && tx->IsCoinBase()) {
    


    l0rinc commented at 7:44 pm on August 4, 2025:

    Please consider extracting the repeated coinbase check (since it’s not just a trivial getter), which would enable short-circuiting the other non-trivial-and-rare BIP30 check:

    0        const bool is_coinbase{tx->IsCoinBase()};
    1        if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) {
    
  13. in src/index/coinstatsindex.cpp:416 in a53063a6d9
    436+        }
    437+
    438         for (uint32_t j = 0; j < tx->vout.size(); ++j) {
    439             const CTxOut& out{tx->vout[j]};
    440             COutPoint outpoint{tx->GetHash(), j};
    441             Coin coin{out, block.height, tx->IsCoinBase()};
    


    l0rinc commented at 7:45 pm on August 4, 2025:

    nit: these can also be a const now:

    0const COutPoint outpoint{tx->GetHash(), j};
    1const Coin coin{out, block.height, is_coinbase};
    
  14. in src/index/coinstatsindex.cpp:423 in a53063a6d9
    460-            --m_transaction_output_count;
    461-            m_total_amount -= coin.out.nValue;
    462-            m_bogo_size -= GetBogoSize(coin.out.scriptPubKey);
    463         }
    464 
    465         // The coinbase tx has no undo data since no former output is spent
    


    l0rinc commented at 7:57 pm on August 4, 2025:

    Seems to me we don’t actually need the whole coin anymore, only its vprevout. And is there a particular reason for copying the outpoint via COutPoint outpoint{tx->vin[j].prevout.hash, tx->vin[j].prevout.n}; instead of just tx_undo.vprevout[j]? And a few lines above we used continue to avoid extra indentation, maybe we can apply that here as well:

    0if (is_coinbase) continue;
    1
    2const auto& vprevout{block.undo_data->vtxundo.at(i - 1).vprevout};
    3for (size_t j{0}; j < vprevout.size(); ++j) {
    4    ApplyCoinHash(m_muhash, tx->vin[j].prevout, vprevout[j]);
    5}
    
  15. l0rinc commented at 8:16 pm on August 4, 2025: contributor

    Concept ACK

    Left a few notes, I think we could split this into smaller, more focused changes, explaining the motivation and decisions in the commit messages and since we’re already cleaning up, there are a few more steps that we could do.

  16. Sammie05 commented at 2:08 am on August 5, 2025: none

    Concept ACK, I agree with the goal of removing unnecessary class members to simplify the code.

    But consider splitting the commit as suggested by @l0rinc to isolate the risky accounting changes.

    Reusing DBVal as a container simplifies the logic. Good improvement.

    Please update the docstring of the method (still says “initialize internal state”), which might no longer be accurate.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-05 06:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me