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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33134.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Review ACK a53063a6d93103926509d3f95dc791140aae58c6
Nice cleanup.
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;
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.
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;
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)
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).
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)};
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)
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;
true, the function can now be void.
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()) {
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)) {
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()};
nit: these can also be a const now:
0const COutPoint outpoint{tx->GetHash(), j};
1const Coin coin{out, block.height, is_coinbase};
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
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}
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.
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.
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.