refactor, index: Remove member variables in coinstatsindex

[fjahr]

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.

[DrahtBot]

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.

[jonatack]

Review ACK a53063a

Nice cleanup.

[mzumsande]

Concept ACK

[furszy]

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).

[l0rinc]

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.

[l0rinc]

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.

[l0rinc]

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:

diff --git a/src/index/coinstatsindex.cpp b/src/index/coinstatsindex.cpp
--- a/src/index/coinstatsindex.cpp	(revision a53063a6d93103926509d3f95dc791140aae58c6)
+++ b/src/index/coinstatsindex.cpp	(date 1754333742030)
@@ -114,56 +114,27 @@
 
 bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
 {
-    uint64_t transaction_output_count{0};
-    uint64_t bogo_size{0};
-    CAmount total_amount{0};
-    CAmount total_subsidy{0};
-    CAmount total_unspendable_amount{0};
-    CAmount total_prevout_spent_amount{0};
-    CAmount total_new_outputs_ex_coinbase_amount{0};
-    CAmount 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};
+    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
+    std::pair<uint256, DBVal> val{};
 
-    // Ignore genesis block
     if (block.height > 0) {
-        std::pair<uint256, DBVal> read_out;
-        if (!m_db->Read(DBHeightKey(block.height - 1), read_out)) {
+        if (!m_db->Read(DBHeightKey(block.height - 1), val)) {
             return false;
         }
 
         uint256 expected_block_hash{*Assert(block.prev_hash)};
-        if (read_out.first != expected_block_hash) {
+        if (val.first != expected_block_hash) {
             LogWarning("previous block header belongs to unexpected block %s; expected %s",
-                      read_out.first.ToString(), expected_block_hash.ToString());
+                      val.first.ToString(), expected_block_hash.ToString());
 
-            if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
+            if (!m_db->Read(DBHashKey(expected_block_hash), val)) {
                 LogError("previous block header not found; expected %s",
                           expected_block_hash.ToString());
                 return false;
             }
         }
 
-        transaction_output_count = read_out.second.transaction_output_count;
-        bogo_size = read_out.second.bogo_size;
-        total_amount = read_out.second.total_amount;
-        total_subsidy = read_out.second.total_subsidy;
-        total_unspendable_amount = read_out.second.total_unspendable_amount;
-        total_prevout_spent_amount = read_out.second.total_prevout_spent_amount;
-        total_new_outputs_ex_coinbase_amount = read_out.second.total_new_outputs_ex_coinbase_amount;
-        total_coinbase_amount = read_out.second.total_coinbase_amount;
-        total_unspendables_genesis_block = read_out.second.total_unspendables_genesis_block;
-        total_unspendables_bip30 = read_out.second.total_unspendables_bip30;
-        total_unspendables_scripts = read_out.second.total_unspendables_scripts;
-        total_unspendables_unclaimed_rewards = read_out.second.total_unspendables_unclaimed_rewards;
-    }
-
-    const CAmount block_subsidy{GetBlockSubsidy(block.height, Params().GetConsensus())};
-    total_subsidy += block_subsidy;
-
-    if (block.height > 0) {
         // Add the new utxos created from the block
         assert(block.data);
         for (size_t i = 0; i < block.data->vtx.size(); ++i) {
@@ -171,8 +142,8 @@
 
             // Skip duplicate txid coinbase transactions (BIP30).
             if (IsBIP30Unspendable(block.hash, block.height) && tx->IsCoinBase()) {
-                total_unspendable_amount += block_subsidy;
-                total_unspendables_bip30 += block_subsidy;
+                val.second.total_unspendable_amount += block_subsidy;
+                val.second.total_unspendables_bip30 += block_subsidy;
                 continue;
             }
 
@@ -183,22 +154,22 @@
 
                 // Skip unspendable coins
                 if (coin.out.scriptPubKey.IsUnspendable()) {
-                    total_unspendable_amount += coin.out.nValue;
-                    total_unspendables_scripts += coin.out.nValue;
+                    val.second.total_unspendable_amount += coin.out.nValue;
+                    val.second.total_unspendables_scripts += coin.out.nValue;
                     continue;
                 }
 
                 ApplyCoinHash(m_muhash, outpoint, coin);
 
                 if (tx->IsCoinBase()) {
-                    total_coinbase_amount += coin.out.nValue;
+                    val.second.total_coinbase_amount += coin.out.nValue;
                 } else {
-                    total_new_outputs_ex_coinbase_amount += coin.out.nValue;
+                    val.second.total_new_outputs_ex_coinbase_amount += coin.out.nValue;
                 }
 
-                ++transaction_output_count;
-                total_amount += coin.out.nValue;
-                bogo_size += GetBogoSize(coin.out.scriptPubKey);
+                ++val.second.transaction_output_count;
+                val.second.total_amount += coin.out.nValue;
+                val.second.bogo_size += GetBogoSize(coin.out.scriptPubKey);
             }
 
             // The coinbase tx has no undo data since no former output is spent
@@ -211,50 +182,39 @@
 
                     RemoveCoinHash(m_muhash, outpoint, coin);
 
-                    total_prevout_spent_amount += coin.out.nValue;
+                    val.second.total_prevout_spent_amount += coin.out.nValue;
 
-                    --transaction_output_count;
-                    total_amount -= coin.out.nValue;
-                    bogo_size -= GetBogoSize(coin.out.scriptPubKey);
+                    --val.second.transaction_output_count;
+                    val.second.total_amount -= coin.out.nValue;
+                    val.second.bogo_size -= GetBogoSize(coin.out.scriptPubKey);
                 }
             }
         }
     } else {
         // genesis block
-        total_unspendable_amount += block_subsidy;
-        total_unspendables_genesis_block += block_subsidy;
+        val.second.total_unspendable_amount += block_subsidy;
+        val.second.total_unspendables_genesis_block += block_subsidy;
     }
+
+    val.second.total_subsidy += 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{(total_prevout_spent_amount + total_subsidy) - (total_new_outputs_ex_coinbase_amount + total_coinbase_amount + total_unspendable_amount)};
-    total_unspendable_amount += unclaimed_rewards;
-    total_unspendables_unclaimed_rewards += unclaimed_rewards;
+    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)};
+    val.second.total_unspendable_amount += unclaimed_rewards;
+    val.second.total_unspendables_unclaimed_rewards += unclaimed_rewards;
 
-    std::pair<uint256, DBVal> value;
-    value.first = block.hash;
-    value.second.transaction_output_count = transaction_output_count;
-    value.second.bogo_size = bogo_size;
-    value.second.total_amount = total_amount;
-    value.second.total_subsidy = total_subsidy;
-    value.second.total_unspendable_amount = total_unspendable_amount;
-    value.second.total_prevout_spent_amount = total_prevout_spent_amount;
-    value.second.total_new_outputs_ex_coinbase_amount = total_new_outputs_ex_coinbase_amount;
-    value.second.total_coinbase_amount = total_coinbase_amount;
-    value.second.total_unspendables_genesis_block = total_unspendables_genesis_block;
-    value.second.total_unspendables_bip30 = total_unspendables_bip30;
-    value.second.total_unspendables_scripts = total_unspendables_scripts;
-    value.second.total_unspendables_unclaimed_rewards = total_unspendables_unclaimed_rewards;
+    val.first = block.hash;
 
     uint256 out;
     m_muhash.Finalize(out);
-    value.second.muhash = out;
+    val.second.muhash = out;
 
     // 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);
+    return m_db->Write(DBHeightKey(block.height), val);
 }
 
 [[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)

[l0rinc]

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.

[l0rinc]

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.

[l0rinc]

After the refactor every path returns true, the function can now be void.

[l0rinc]

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:

Suggested change

	if (IsBIP30Unspendable(block.hash, block.height) && tx->IsCoinBase()) {
	const bool is_coinbase{tx->IsCoinBase()};
	if (is_coinbase && IsBIP30Unspendable(block.hash, block.height)) {

[l0rinc]

nit: these can also be a const now:

const COutPoint outpoint{tx->GetHash(), j};
const Coin coin{out, block.height, is_coinbase};

[l0rinc]

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:

if (is_coinbase) continue;

const auto& vprevout{block.undo_data->vtxundo.at(i - 1).vprevout};
for (size_t j{0}; j < vprevout.size(); ++j) {
    ApplyCoinHash(m_muhash, tx->vin[j].prevout, vprevout[j]);
}

[Sammie05]

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.