refactor, index: Remove member variables in coinstatsindex #33134

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.

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?

Even if just one value is initialized it is still initialized, so I don't see why this should be inaccurate. Also this is a function from the base index that is overridden so not using it just because of semantics seems overkill.

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.

I am not sure what inlining means to you in this context, this change makes member variables to local variables and because of that the variables don't need to be initialized in a separate function anymore. They can't even be initialized in a separate function because they are local so I don't see how this could be split into a separate commit. But maybe I misunderstand what you are suggesting.

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)

I took the part of this suggestion to use the intermittend DBVal but when taking everything together in #30469 many of the other changes don't apply anymore I think

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.

The member variables are removed and since they are removed we don't need to roll back there values here either. I don't see how this could be split up into separate commits.

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.

Not directly related to the change but also a very small change, I just renamed the function to RevertBlock in #30469

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

Considered this but with all the latest changes in #30469 this is no longer relevant

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:

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

done in #30469

nit: these can also be a const now:

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

done in #30469

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]);
}

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]?

vprevout contains Coins, not COutPoints so I am not sure I can follow this suggestion.