index, rpc: Coinstatsindex follow-ups

fjahr commented at 5:34 pm on May 24, 2021: member

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

index: Avoid unnecessary type casts in coinstatsindex 8ea8c927ac

DrahtBot added the label RPC/REST/ZMQ on May 24, 2021

DrahtBot added the label UTXO Db and Indexes on May 24, 2021

fjahr force-pushed on May 24, 2021

in src/rpc/blockchain.cpp:1120 in 482e546254 outdated

1116@@ -1117,13 +1117,13 @@ static RPCHelpMan gettxoutsetinfo()
1117                         {RPCResult::Type::STR_AMOUNT, "total_unspendable_amount", "The total amount of coins permanently excluded from the UTXO set (only available if coinstatsindex is used)"},
1118                         {RPCResult::Type::OBJ, "block_info", "Info on amounts in the block at this block height (only available if coinstatsindex is used)",
1119                         {
1120-                            {RPCResult::Type::STR_AMOUNT, "prevout_spent", ""},
1121-                            {RPCResult::Type::STR_AMOUNT, "coinbase", ""},
1122-                            {RPCResult::Type::STR_AMOUNT, "new_outputs_ex_coinbase", ""},
1123-                            {RPCResult::Type::STR_AMOUNT, "unspendable", ""},
1124+                            {RPCResult::Type::STR_AMOUNT, "prevout_spent", "Total value of all prevouts spent in this block"},

Talkless commented at 6:43 pm on May 24, 2021:

Not sure if “value” is the best wording choice here. I see “amount” used much more in the codebase, and even variables/enums used here in PR uses “amount”, not “value”.

fjahr commented at 8:37 pm on May 24, 2021:

Yeah, I see what you mean and I was on the fence there. I felt like amount could be easier mistaken to mean ‘count’ in this context but I am not a native speaker so maybe that feeling is just off. I have changed it to ‘amount’ since, as you say, consistency is better, all other things being equal.

fjahr force-pushed on May 24, 2021

DrahtBot commented at 9:58 pm on May 24, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22567 by MarcoFalke
#20362 by MarcoFalke
#19792 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.

in src/index/coinstatsindex.cpp:30 in 5535567f3d outdated

29-    CAmount block_new_outputs_ex_coinbase_amount;
30-    CAmount block_coinbase_amount;
31+    CAmount total_unspendable_amount;
32+    CAmount total_prevout_spent_amount;
33+    CAmount total_new_outputs_ex_coinbase_amount;
34+    CAmount total_coinbase_amount;

MarcoFalke commented at 8:31 am on May 25, 2021:

unspendables are also “total”, but don’t mention it. Might be good to remove the “total_” here, or add it to unspendables?

fjahr commented at 10:30 pm on May 25, 2021:

Dropped the total_ everywhere except for the m_total_amount, it seemed right to keep it in this case since it’s closed to the corresponding legacy name nTotalAmount that is also still in use.

in src/index/coinstatsindex.cpp:126 in 5535567f3d outdated

121@@ -122,9 +122,12 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
122 
123         uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
124         if (read_out.first != expected_block_hash) {
125+            LogPrintf("%s: WARNING: previous block header belongs to unexpected block %s; expected %s\n",
126+                      __func__, read_out.first.ToString(), expected_block_hash.ToString());

MarcoFalke commented at 8:32 am on May 25, 2021:

0                      read_out.first.ToString(), expected_block_hash.ToString());

This is toggled with -logsourcelocations, so can skip here.

fjahr commented at 10:29 pm on May 25, 2021:

done

in src/index/coinstatsindex.cpp:399 in 5535567f3d outdated

394@@ -391,9 +395,12 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
395 
396         uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
397         if (read_out.first != expected_block_hash) {
398+            LogPrintf("%s: WARNING: previous block header belongs to unexpected block %s; expected %s\n",
399+                      __func__, read_out.first.ToString(), expected_block_hash.ToString());

MarcoFalke commented at 8:32 am on May 25, 2021:

same

fjahr commented at 10:28 pm on May 25, 2021:

done

fjahr force-pushed on May 25, 2021

in src/index/coinstatsindex.cpp:26 in b778ac8f2b outdated

22@@ -23,11 +23,11 @@ struct DBVal {
23     uint64_t transaction_output_count;
24     uint64_t bogo_size;
25     CAmount total_amount;
26-    CAmount total_subsidy;

Sjors commented at 12:30 pm on May 26, 2021:

b778ac8f2b12f771e5b8ddcadc35ee267e57cfb0: I think the total_ prefix is more clear (and consistent with total_amount which you wanted to keep). So maybe use the second approach @MarcoFalke suggested:

unspendables are also “total”, but don’t mention it. Might be good to remove the “total_” here, or add it to unspendables?

More importantly, can you add comments explaining how these values are used? Their cumulative nature is a bit counter intuitive, though it makes sense for rollbacks.

fjahr commented at 0:15 am on June 3, 2021:

Done, changed the names to all use total_ and added comments explaining each of the new values in coinstats.h a bit more clearly.

in src/rpc/blockchain.cpp:1123 in 0f9e936e52 outdated

1122-                            {RPCResult::Type::STR_AMOUNT, "new_outputs_ex_coinbase", ""},
1123-                            {RPCResult::Type::STR_AMOUNT, "unspendable", ""},
1124+                            {RPCResult::Type::STR_AMOUNT, "prevout_spent", "Total amount of all prevouts spent in this block"},
1125+                            {RPCResult::Type::STR_AMOUNT, "coinbase", "Coinbase subsidy amount of this block"},
1126+                            {RPCResult::Type::STR_AMOUNT, "new_outputs_ex_coinbase", "Total amount of new outputs created by this block"},
1127+                            {RPCResult::Type::STR_AMOUNT, "unspendable", "Total amount of unspendable outputs in this block"},

Sjors commented at 12:46 pm on May 26, 2021:

0f9e936e521d7090c876d0c181ca241c56e387ab: created in this block

fjahr commented at 0:16 am on June 3, 2021:

Done

Sjors commented at 12:55 pm on May 26, 2021: member

Mostly happy with f3cf0298180680900eabf1cd2827bd854d4cfeda

Talkless commented at 6:36 pm on June 2, 2021: none

tACK f3cf0298180680900eabf1cd2827bd854d4cfeda. Built on Debian Sid, ran -regtest with -coinstatsindex, mined some blocks and checked gettxoutsetinfo output and executed feature_coinstatsindex functional test.

scripted-diff: Fix coinstats data member names

Initially these values were 'per block' in an earlier version but were then changed to total values. The names were not updated to reflect that.

-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }

s 'm_block_unspendable_amount'              'm_total_unspendable_amount'
s 'm_block_prevout_spent_amount'            'm_total_prevout_spent_amount'
s 'm_block_new_outputs_ex_coinbase_amount'  'm_total_new_outputs_ex_coinbase_amount'
s 'm_block_coinbase_amount'                 'm_total_coinbase_amount'
s 'block_unspendable_amount'                'total_unspendable_amount'
s 'block_prevout_spent_amount'              'total_prevout_spent_amount'
s 'block_new_outputs_ex_coinbase_amount'    'total_new_outputs_ex_coinbase_amount'
s 'block_coinbase_amount'                   'total_coinbase_amount'

s 'unspendables_genesis_block'              'total_unspendables_genesis_block'
s 'unspendables_bip30'                      'total_unspendables_bip30'
s 'unspendables_scripts'                    'total_unspendables_scripts'
s 'unspendables_unclaimed_rewards'          'total_unspendables_unclaimed_rewards'
s 'm_unspendables_genesis_block'            'm_total_unspendables_genesis_block'
s 'm_unspendables_bip30'                    'm_total_unspendables_bip30'
s 'm_unspendables_scripts'                  'm_total_unspendables_scripts'
s 'm_unspendables_unclaimed_rewards'        'm_total_unspendables_unclaimed_rewards'
-END VERIFY SCRIPT-

fb65dde147

index: Use batch writing in coinstatsindex WriteBlock 1e3842385b

Index: Return early from failed coinstatsindex init 01386bfd88

fjahr force-pushed on Jun 3, 2021

Sjors commented at 9:37 am on June 29, 2021: member

ACK d6dbb17

fjahr commented at 1:40 pm on July 18, 2021: member

@MarcoFalke could you check if you are happy with this? :)

in src/rpc/blockchain.cpp:1126 in d6dbb17b16 outdated

1126+                            {RPCResult::Type::STR_AMOUNT, "new_outputs_ex_coinbase", "Total amount of new outputs created by this block"},
1127+                            {RPCResult::Type::STR_AMOUNT, "unspendable", "Total amount of unspendable outputs created in this block"},
1128                             {RPCResult::Type::OBJ, "unspendables", "Detailed view of the unspendable categories",
1129                             {
1130-                                {RPCResult::Type::STR_AMOUNT, "genesis_block", ""},
1131+                                {RPCResult::Type::STR_AMOUNT, "genesis_block", "The Genesis block subsidy is not spendable"},

jonatack commented at 9:34 am on July 20, 2021:

ac2a8b51

0                                {RPCResult::Type::STR_AMOUNT, "genesis_block", "The unspendable amount of the Genesis block subsidy"},

fjahr commented at 8:38 pm on July 25, 2021:

done

in src/node/coinstats.h:65 in d6dbb17b16 outdated

68+    CAmount total_unspendables_genesis_block{0};
69+    //! The two unspendable coinbase outputs total amount caused by BIP30
70+    CAmount total_unspendables_bip30{0};
71+    //! Total cumulative amount of outputs sent to unspendable scripts (OP_RETURN for example) up until and including this block
72+    CAmount total_unspendables_scripts{0};
73+    //! Total cumulative amount coins lost due to unclaimed miner rewards up until and including this block

jonatack commented at 9:41 am on July 20, 2021:

d6dbb17 nit, s/up until/up to/ for each of these

fjahr commented at 8:38 pm on July 25, 2021:

done

in src/index/coinstatsindex.cpp:403 in d6dbb17b16 outdated

400+
401             if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {
402-                return error("%s: previous block header belongs to unexpected block %s; expected %s",
403-                             __func__, read_out.first.ToString(), expected_block_hash.ToString());
404+                return error("%s: previous block header not found; expected %s",
405+                             __func__, expected_block_hash.ToString());

jonatack commented at 9:44 am on July 20, 2021:

18aa58b4 I did not test this change; it may be good to add a test here and for the same logging change above lines 125-130

fjahr commented at 8:37 pm on July 25, 2021:

I think we can really only hit this in cases of data corruption (or a bug somewhere else in the code). So that makes it really hard to test. I have made a note to look into it some more and think about how to add coverage without too much effort.

jonatack commented at 10:01 am on July 20, 2021: member

ACK d6dbb17b1608502850e9e5c6ec29569cfcb23237 tested on signet after rebase to current master

Happy to re-ACK for any of the suggestions below, if you’re so inclined.

rpc: Add missing gettxoutsetinfo help docs a5f6791139

rpc: Block until synced if coinstatsindex is used in gettxoutsetinfo

During initial sync after startup the gettxoutsetinfo RPC will still return an error while catching up. However, after the initial sync the index will not error immediately anymore when it's in the process of syncing to the tip while being called. Instead it will block until synced and then return the response.

d4356d4e48

Index: Improve logging in coinstatsindex

More accurate logging of a warning should make clear if the recovery condition was hit while catching the results of the previous block.

5b3d4e724f

coinstats: Add comments for new coinstatsindex values 779e638ca9

fjahr force-pushed on Jul 25, 2021

fjahr commented at 8:39 pm on July 25, 2021: member

Addressed @jonatack ’s comments and fixed a typo.

Sjors commented at 11:32 am on July 27, 2021: member

re-utACK 779e638ca9b2b37c247577d225b93ac762b0602f

jonatack commented at 4:32 pm on July 27, 2021: member

re-ACK 779e638ca9b2b37c247577d225b93ac762b0602f 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

Talkless approved

Talkless commented at 5:36 pm on July 27, 2021: none

re-utACK 779e638ca9b2b37c247577d225b93ac762b0602f after cosmetic changes.

in src/index/coinstatsindex.cpp:412 in 779e638ca9

408@@ -402,24 +409,24 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
409     for (size_t i = 0; i < block.vtx.size(); ++i) {
410         const auto& tx{block.vtx.at(i)};
411 
412-        for (size_t j = 0; j < tx->vout.size(); ++j) {
413+        for (uint32_t j = 0; j < tx->vout.size(); ++j) {

laanwj commented at 12:16 pm on July 28, 2021:

Doesn’t vector<…>::size() return a size_t, making this effectively a cast? (apparently not necessarily: which size_type it returns is C++ library dependent)

laanwj commented at 12:16 pm on July 28, 2021: member

Code review ACK 779e638ca9b2b37c247577d225b93ac762b0602f

laanwj merged this on Jul 28, 2021

laanwj closed this on Jul 28, 2021

sidhujag referenced this in commit cec2338136 on Jul 28, 2021

luke-jr referenced this in commit 4ee6ddd887 on Oct 10, 2021

luke-jr referenced this in commit 9a4bf4d70a on Oct 10, 2021

luke-jr referenced this in commit 19a40fd663 on Oct 10, 2021

luke-jr referenced this in commit fbc290d6b2 on Oct 10, 2021

Fabcien referenced this in commit 9239362cf1 on Jun 13, 2022

Fabcien referenced this in commit 13d1d58b39 on Jun 13, 2022

DrahtBot locked this on Aug 18, 2022

index, rpc: Coinstatsindex follow-ups #22047

Conflicts