index, rpc: Coinstatsindex follow-ups #22047

fjahr · 2021-05-24T17:34:43Z

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

Talkless · 2021-05-24T18:43:21Z

src/rpc/blockchain.cpp

-                            {RPCResult::Type::STR_AMOUNT, "coinbase", ""},
-                            {RPCResult::Type::STR_AMOUNT, "new_outputs_ex_coinbase", ""},
-                            {RPCResult::Type::STR_AMOUNT, "unspendable", ""},
+                            {RPCResult::Type::STR_AMOUNT, "prevout_spent", "Total value of all prevouts spent in this block"},


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

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.

DrahtBot · 2021-05-24T21:58:34Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567 by MarcoFalke
test: Implicitly sync after generate* to preempt races and intermittent test failures #20362 by MarcoFalke
rpc: Add dumpcoinstats #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.

maflcko · 2021-05-25T08:31:26Z

src/index/coinstatsindex.cpp

+    CAmount total_unspendable_amount;
+    CAmount total_prevout_spent_amount;
+    CAmount total_new_outputs_ex_coinbase_amount;
+    CAmount total_coinbase_amount;


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

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.

src/index/coinstatsindex.cpp

Sjors

Mostly happy with f3cf029

Sjors · 2021-05-26T12:30:02Z

src/index/coinstatsindex.cpp

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


b778ac8: 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.

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

src/rpc/blockchain.cpp

Talkless · 2021-06-02T18:36:52Z

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

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-

Sjors · 2021-06-29T09:37:54Z

ACK d6dbb17

fjahr · 2021-07-18T13:40:50Z

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

jonatack

ACK d6dbb17 tested on signet after rebase to current master

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

src/rpc/blockchain.cpp

src/node/coinstats.h

jonatack · 2021-07-20T09:44:54Z

src/index/coinstatsindex.cpp

-                return error("%s: previous block header belongs to unexpected block %s; expected %s",
-                             __func__, read_out.first.ToString(), expected_block_hash.ToString());
+                return error("%s: previous block header not found; expected %s",
+                             __func__, expected_block_hash.ToString());


18aa58b 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

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.

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.

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

fjahr · 2021-07-25T20:39:04Z

Addressed @jonatack 's comments and fixed a typo.

Sjors · 2021-07-27T11:32:21Z

re-utACK 779e638

jonatack · 2021-07-27T16:32:43Z

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

re-utACK 779e638 after cosmetic changes.

laanwj · 2021-07-28T12:16:09Z

src/index/coinstatsindex.cpp

@@ -402,24 +409,24 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
    for (size_t i = 0; i < block.vtx.size(); ++i) {
        const auto& tx{block.vtx.at(i)};

-        for (size_t j = 0; j < tx->vout.size(); ++j) {
+        for (uint32_t j = 0; j < tx->vout.size(); ++j) {


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 · 2021-07-28T12:16:47Z

Code review ACK 779e638

Github-Pull: bitcoin#22047 Rebased-From: 1e38423

Github-Pull: bitcoin#22047 Rebased-From: a5f6791

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. Github-Pull: bitcoin#22047 Rebased-From: d4356d4

More accurate logging of a warning should make clear if the recovery condition was hit while catching the results of the previous block. Github-Pull: bitcoin#22047 Rebased-From: 5b3d4e7

Summary: Main commit bitcoin/bitcoin@dd58a4d > index: Add Coinstats index > > The index holds the values previously calculated in coinstats.cpp for each block, representing the state of the UTXO set at each height. ----- bitcoin/bitcoin@57a026c > test: Add unit test for Coinstats index ----- bitcoin/bitcoin@8ea8c92 > index: Avoid unnecessary type casts in coinstatsindex ----- Replace two database writes with a single atomic batch write, to avoid a risk of database corruption. See [[ bitcoin/bitcoin@dd58a4d#r634089179 | the corresponding review ]]. bitcoin/bitcoin@1e38423 > index: Use batch writing in coinstatsindex WriteBlock ----- This is a backport of [[bitcoin/bitcoin#19521 | core#19521]] [4 & 8/17] and [[bitcoin/bitcoin#22047 | core#22047]] [1 & 3/3] Depends on D11597 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D11598

Summary: This is a backport of [[bitcoin/bitcoin#19521 | core#19521]] [11/17] and [[bitcoin/bitcoin#22047 | core#22047]] [2/3] bitcoin/bitcoin@2501576 bitcoin/bitcoin@fb65dde Depends on D11604 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11605

index: Avoid unnecessary type casts in coinstatsindex

8ea8c92

DrahtBot added RPC/REST/ZMQ UTXO Db and Indexes labels May 24, 2021

fjahr force-pushed the 19521-followup branch from e7cb8e0 to 482e546 Compare May 24, 2021 17:59

fjahr mentioned this pull request May 24, 2021

Coinstats Index #19521

Merged

Talkless reviewed May 24, 2021

View reviewed changes

fjahr force-pushed the 19521-followup branch from 482e546 to 5535567 Compare May 24, 2021 20:37

DrahtBot mentioned this pull request May 25, 2021

rpc: Add dumpcoinstats #19792

Closed

maflcko reviewed May 25, 2021

View reviewed changes

fjahr force-pushed the 19521-followup branch 2 times, most recently from c6dfa1c to f3cf029 Compare May 25, 2021 22:34

Sjors reviewed May 26, 2021

View reviewed changes

fjahr added 3 commits June 3, 2021 01:48

index: Use batch writing in coinstatsindex WriteBlock

1e38423

Index: Return early from failed coinstatsindex init

01386bf

fjahr force-pushed the 19521-followup branch from f3cf029 to d6dbb17 Compare June 3, 2021 00:07

DrahtBot mentioned this pull request Jun 24, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #20362

Closed

jonatack reviewed Jul 20, 2021

View reviewed changes

fjahr added 4 commits July 25, 2021 20:59

rpc: Add missing gettxoutsetinfo help docs

a5f6791

Index: Improve logging in coinstatsindex

5b3d4e7

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

coinstats: Add comments for new coinstatsindex values

779e638

fjahr force-pushed the 19521-followup branch from d6dbb17 to 779e638 Compare July 25, 2021 20:38

Talkless approved these changes Jul 27, 2021

View reviewed changes

DrahtBot mentioned this pull request Jul 28, 2021

test: Implicitly sync after generate* to preempt races and intermittent test failures #22567

Closed

laanwj reviewed Jul 28, 2021

View reviewed changes

laanwj merged commit 31fef69 into bitcoin:master Jul 28, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 28, 2021

Merge bitcoin#22047: index, rpc: Coinstatsindex follow-ups

cec2338

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

index: Use batch writing in coinstatsindex WriteBlock

4ee6ddd

Github-Pull: bitcoin#22047 Rebased-From: 1e38423

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Oct 10, 2021

rpc: Add missing gettxoutsetinfo help docs

9a4bf4d

Github-Pull: bitcoin#22047 Rebased-From: a5f6791

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

index, rpc: Coinstatsindex follow-ups #22047

index, rpc: Coinstatsindex follow-ups #22047

fjahr commented May 24, 2021

Talkless May 24, 2021

fjahr May 24, 2021

DrahtBot commented May 24, 2021 •

edited

maflcko May 25, 2021

fjahr May 25, 2021

Sjors left a comment

Sjors May 26, 2021

fjahr Jun 3, 2021

Talkless commented Jun 2, 2021

Sjors commented Jun 29, 2021

fjahr commented Jul 18, 2021

jonatack left a comment

jonatack Jul 20, 2021 •

edited

fjahr Jul 25, 2021

fjahr commented Jul 25, 2021 •

edited

Sjors commented Jul 27, 2021

jonatack commented Jul 27, 2021 •

edited

Talkless left a comment

laanwj Jul 28, 2021

laanwj commented Jul 28, 2021

index, rpc: Coinstatsindex follow-ups #22047

index, rpc: Coinstatsindex follow-ups #22047

Conversation

fjahr commented May 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented May 24, 2021 • edited

Conflicts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Talkless commented Jun 2, 2021

Sjors commented Jun 29, 2021

fjahr commented Jul 18, 2021

jonatack left a comment

Choose a reason for hiding this comment

jonatack Jul 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjahr commented Jul 25, 2021 • edited

Sjors commented Jul 27, 2021

jonatack commented Jul 27, 2021 • edited

Talkless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Jul 28, 2021

DrahtBot commented May 24, 2021 •

edited

jonatack Jul 20, 2021 •

edited

fjahr commented Jul 25, 2021 •

edited

jonatack commented Jul 27, 2021 •

edited