rpc: Add dumpcoinstats

fjahr commented at 7:01 pm on August 24, 2020: contributor

Requires #19521 (this PR only adds the last commit).

The rpc dumpcoinstats dumps the content of the coinstats index into a CSV file for auditing purposes.

DrahtBot added the label Build system on Aug 24, 2020

DrahtBot added the label RPC/REST/ZMQ on Aug 24, 2020

DrahtBot added the label UTXO Db and Indexes on Aug 24, 2020

DrahtBot added the label Validation on Aug 24, 2020

DrahtBot commented at 9:25 pm on August 24, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	laanwj, promag
Approach ACK	Zero-1729
Stale ACK	PierreRochard, w0xlt, Sjors

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:

#27607 (init: verify blocks data existence only once for all the indexers by furszy)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

fjahr force-pushed on Aug 25, 2020

fjahr marked this as ready for review on Aug 25, 2020

fjahr commented at 9:15 pm on August 25, 2020: contributor

Taking this out of draft status since I am interested in conceptual feedback on something like this but of course, #19521 still has a long way to go before this can be seriously considered.

DrahtBot added the label Needs rebase on Aug 26, 2020

fjahr force-pushed on Aug 26, 2020

DrahtBot removed the label Needs rebase on Aug 26, 2020

PierreRochard commented at 2:54 am on August 27, 2020: contributor

Tested ACK 21c42c22c9b50f60a4b4a0ee84c45f47df98752f

laanwj commented at 12:00 pm on August 27, 2020: member

Concept ACK, I wish we had gotten streaming to work though so that the file can be downloaded (see my experiment in #7759). RPCs that creates a server-local file, although there are already some others, are not that great from a security and usability point of view.

DrahtBot added the label Needs rebase on Sep 22, 2020

fjahr force-pushed on Sep 22, 2020

fjahr commented at 9:28 pm on September 22, 2020: contributor

Rebased on top of the latest changes in #19521

DrahtBot removed the label Needs rebase on Sep 22, 2020

promag commented at 11:04 pm on October 6, 2020: member

Concept ACK, could have a functional test.

DrahtBot added the label Needs rebase on Oct 15, 2020

fanquake removed the label Build system on Mar 16, 2021

fjahr force-pushed on May 5, 2021

fjahr commented at 11:32 pm on May 5, 2021: contributor

Rebased now that #19521, will also push a functional test shortly.

DrahtBot removed the label Needs rebase on May 6, 2021

fjahr force-pushed on May 6, 2021

fjahr commented at 10:48 pm on May 6, 2021: contributor

Added functional test.

DrahtBot added the label Needs rebase on Jul 28, 2021

fjahr force-pushed on Aug 1, 2021

fjahr commented at 7:41 pm on August 1, 2021: contributor

Rebased

DrahtBot removed the label Needs rebase on Aug 1, 2021

MarcoFalke commented at 6:36 am on August 2, 2021: member

Error: RPC command “dumpcoinstats” not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.

fjahr force-pushed on Aug 2, 2021

fjahr commented at 10:05 pm on August 3, 2021: contributor

Thanks @MarcoFalke , fixed!

Zero-1729 commented at 1:16 pm on August 13, 2021: contributor

Approach ACK b85813a

MarcoFalke removed the label UTXO Db and Indexes on Dec 20, 2021

MarcoFalke removed the label RPC/REST/ZMQ on Dec 20, 2021

MarcoFalke removed the label Validation on Dec 20, 2021

DrahtBot added the label RPC/REST/ZMQ on Dec 20, 2021

fjahr force-pushed on Jan 23, 2022

fjahr commented at 5:50 pm on January 23, 2022: contributor

Rebased and fixed silent merge conflict.

in src/rpc/blockchain.cpp:1254 in cd87e8163a outdated

1249+        CHECK_NONFATAL(stats.total_amount.has_value());
1250+        file << strprintf("%s,", FormatMoney(stats.total_amount.value()));
1251+        file << strprintf("%s,", FormatMoney(stats.total_unspendable_amount));
1252+        file << strprintf("%s,", FormatMoney(stats.total_prevout_spent_amount));
1253+        file << strprintf("%s,", FormatMoney(stats.total_new_outputs_ex_coinbase_amount));
1254+        file << strprintf("%s,", FormatMoney(stats.total_coinbase_amount));

Sjors commented at 4:04 pm on January 26, 2022:

I would prefer these to be per-block amounts like in the RPC (obtaining the sum of a column is trivial in a spreadsheet).

fjahr commented at 0:14 am on January 31, 2022:

There seemed to be a few people that preferred this version back when I first built the coinstatsindex but we can just have both. So now there is a bool argument to switch between cummulative and per-block.

Sjors commented at 4:05 pm on January 26, 2022: member

Concept ACK. Tested and reviewed 87fd7b8, but see inline

Also, not to scope creep, but can you add a few goodies from CBlockIndex to the CSV?

the number of transactions per block (nTx)
timestamp (and maybe MTP, but that can be derived in a spreadsheet)
nChainWork (or per block)
nVersion

fjahr commented at 0:15 am on January 31, 2022: contributor

Also, not to scope creep, but can you add a few goodies from CBlockIndex to the CSV?

the number of transactions per block (nTx)

timestamp (and maybe MTP, but that can be derived in a spreadsheet)

nChainWork (or per block)

nVersion

Easy enough, if it enables more use cases we can easily add more. I have added all of these.

in src/rpc/blockchain.cpp:1177 in f73b68c262 outdated

1172+{
1173+    return RPCHelpMan{"dumpcoinstats",
1174+                "\nDumps content of coinstats index to a CSV file.\n",
1175+                {
1176+                    {"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The filename with path (absolute path recommended)"},
1177+                    {"cummulative", RPCArg::Type::BOOL, RPCArg::Default{false}, "The numbers coming out of coinstatsindex will be cummulative"},

Sjors commented at 4:19 pm on January 31, 2022:

cumulative

fjahr commented at 7:54 pm on February 6, 2022:

fixed

in src/rpc/blockchain.cpp:1274 in f73b68c262 outdated

1269+            file << strprintf("%s,", FormatMoney(stats.total_unspendables_genesis_block));
1270+            file << strprintf("%s,", FormatMoney(stats.total_unspendables_bip30));
1271+            file << strprintf("%s,", FormatMoney(stats.total_unspendables_scripts));
1272+            file << strprintf("%s,", FormatMoney(stats.total_unspendables_unclaimed_rewards));
1273+        } else {
1274+            file << strprintf("%s,", FormatMoney(stats.total_unspendable_amount - prev_stats.total_unspendable_amount));

Sjors commented at 4:27 pm on January 31, 2022:

Nit: maybe calculate these values before the if (cummulative) to remove the repetition here:

0const unspendable_amount = tats.total_unspendable_amount - cumulative ? 0 : prev_stats.total_prevout_spent_amount
1...
2file << strprintf("%s,", FormatMoney(unspendable_amount));

fjahr commented at 7:54 pm on February 6, 2022:

done

in src/rpc/blockchain.cpp:1213 in f73b68c262 outdated

1208+    }
1209+
1210+    const bool cummulative{request.params[1].isNull() ? false : request.params[1].get_bool()};
1211+
1212+    file << strprintf("height,"
1213+                      "bestblock,"

Sjors commented at 4:41 pm on January 31, 2022:

block_hash ?

fjahr commented at 7:54 pm on February 6, 2022:

done

in src/rpc/blockchain.cpp:1217 in f73b68c262 outdated

1212+    file << strprintf("height,"
1213+                      "bestblock,"
1214+                      "txouts,"
1215+                      "bogosize,"
1216+                      "muhash,"
1217+                      "total_amount,"

Sjors commented at 4:43 pm on January 31, 2022:

cumulative ? "total_amount" : "amount"

fjahr commented at 7:54 pm on February 6, 2022:

I was a bit hesitant about this because it might make some scripting harder if the header is different but I guess it makes sense to change it if the values are different. So done now.

Sjors commented at 4:54 pm on January 31, 2022: member

f73b68c262df256e08a32af4a2e8142b3f68dee6 almost there

fjahr force-pushed on Feb 6, 2022

fjahr commented at 7:55 pm on February 6, 2022: contributor

Addressed @Sjors ’ comments, thanks for reviewing! :)

in src/rpc/client.cpp:158 in ba1a08a3e1 outdated

154@@ -155,6 +155,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
155     { "getblockstats", 0, "hash_or_height" },
156     { "getblockstats", 1, "stats" },
157     { "pruneblockchain", 0, "height" },
158+    { "dumpcoinstats", 1, "cummulative"},

Sjors commented at 9:19 am on February 7, 2022:

Missed a spot

fjahr commented at 11:20 pm on February 7, 2022:

Gah, thought I had grepped for it. But there was another silent merge conflict anyway… :/

fjahr force-pushed on Feb 7, 2022

Sjors commented at 10:02 am on February 8, 2022: member

Windows CI seems unhappy:

PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: ‘C:\Users\ContainerAdministrator\AppData\Local\Temp\test_runner_₿_🏃_20220207_231436\feature_coinstatsindex_17\node1\test.csv’

in src/rpc/blockchain.cpp:1169 in e381d716b0 outdated

1162@@ -1161,6 +1163,162 @@ static RPCHelpMan pruneblockchain()
1163     };
1164 }
1165 
1166+static RPCHelpMan dumpcoinstats()
1167+{
1168+    return RPCHelpMan{"dumpcoinstats",
1169+                "\nDumps content of coinstats index to a CSV file.\n",

MarcoFalke commented at 10:08 am on February 8, 2022:

nit:

0        "Dumps content of coinstats index to a CSV file.",

Can remove some whitespace and \n.

fjahr commented at 5:17 pm on February 12, 2022:

done

in src/node/coinstats.h:32 in a287c74981 outdated

28@@ -29,7 +29,7 @@ enum class CoinStatsHashType {
29 
30 struct CCoinsStats {
31     //! Which hash type to use
32-    const CoinStatsHashType m_hash_type;
33+    CoinStatsHashType m_hash_type;

MarcoFalke commented at 10:11 am on February 8, 2022:

Wouldn’t you have to delete the default constructor now? Otherwise it seems possible to leave this uninitialized?

fjahr commented at 5:18 pm on February 12, 2022:

Hm, ok, didn’t think about that. I opted to set hash_serialized as a default value instead.

in src/rpc/blockchain.cpp:1186 in e381d716b0 outdated

1181+                    HelpExampleCli("dumpcoinstats", "\"test\"") +
1182+                    HelpExampleRpc("dumpcoinstats", "\"test\"")
1183+                },
1184+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1185+{
1186+    if (request.params[0].isNull())

MarcoFalke commented at 10:14 am on February 8, 2022:

Missing { here and elsewhere?

fjahr commented at 5:17 pm on February 12, 2022:

fixed

fjahr force-pushed on Feb 12, 2022

fjahr commented at 5:20 pm on February 12, 2022: contributor

Addressed comments by @Sjors and @MarcoFalke , thanks for reviewing!

in src/rpc/blockchain.cpp:1305 in 3772c09e16 outdated

1301+        file << strprintf("%s,", FormatMoney(unspendables_unclaimed_rewards));
1302+
1303+        file << strprintf("%d,", pindex->nTx);
1304+        file << strprintf("%d,", pindex->nTime);
1305+        file << strprintf("%d,", pindex->GetMedianTimePast());
1306+        file << strprintf("%s,", pindex->nChainWork.GetHex());

Sjors commented at 2:59 pm on February 18, 2022:

3772c09e16544c7876da13f33e6cb11401c9e4bd : this is a cumulative metric. Would be nice to get the per block work.

fjahr commented at 1:29 am on February 20, 2022:

done

in src/rpc/blockchain.cpp:1302 in 3772c09e16 outdated

1296@@ -1291,7 +1297,13 @@ static RPCHelpMan dumpcoinstats()
1297         file << strprintf("%s,", FormatMoney(unspendables_scripts));
1298 
1299         const CAmount unspendables_unclaimed_rewards = cumulative ? stats.total_unspendables_unclaimed_rewards : stats.total_unspendables_unclaimed_rewards - prev_stats.total_unspendables_unclaimed_rewards;
1300-        file << strprintf("%s\n", FormatMoney(unspendables_unclaimed_rewards));
1301+        file << strprintf("%s,", FormatMoney(unspendables_unclaimed_rewards));
1302+
1303+        file << strprintf("%d,", pindex->nTx);

Sjors commented at 3:03 pm on February 18, 2022:

3772c09e16544c7876da13f33e6cb11401c9e4bd : this is not cumulative, so would be nice to have cumulative version.

fjahr commented at 1:29 am on February 20, 2022:

done

Sjors approved

Sjors commented at 3:04 pm on February 18, 2022: member

tACK 3772c09e16544c7876da13f33e6cb11401c9e4bd

Remaining feedback can wait for a followup, but I’m also happy to review it again.

Maybe document the fields in the help, since some are not obvious. For each item, note if it has a cumulative version.

A slightly more logical ordering of columns would be:

height
block_hash
timestamp
mediantime
version
txs
maybe insert: txins to count the number of outputs spent. I think you can use coin_count between two blocks and txouts to calculate it.
txouts (document for cumulative: this is the total number of outputs ever created, not the total in existence at this height)
maybe insert: the number txouts in existence at this height (the same for cumulative). It may be useful to track its ratio to bogosize over time.
prevout_spent_amount (maybe rename to amount_spent)
amount (or amount_created)
coinbase_amount
new_outputs_ex_coinbase_amount
subsidy
unclaimed_rewards
unspendable_scripts (docs should clarify this is just OP_RETURN)
unspendable_amount (docs should clarify this includes both immature and OP_RETURN)
genesis_block
bip30
chainwork
muhash
bogosize: Database-independent approximate metric of UTXO set size. It counts every UTXO entry as 50 + the length of its scriptPubKey.

This puts the most useful fields first.

There may be some metrics that are useful for both cumulative and non-cumulative. In particular it’s both useful to know how many outputs were created and destroyed in any given block (and their value, and the delta UTXO set size), and how many outputs existed at any given block (and much value they represented and disk space they used).

in src/rpc/blockchain.cpp:1268 in 3772c09e16 outdated

1263+            throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");
1264+
1265+        file << strprintf("%d,", static_cast<int64_t>(stats.nHeight));
1266+        file << strprintf("%s,", stats.hashBlock.GetHex());
1267+        file << strprintf("%d,", static_cast<int64_t>(stats.nTransactionOutputs));
1268+        file << strprintf("%d,", static_cast<int64_t>(stats.nBogoSize));

Sjors commented at 3:40 pm on February 18, 2022:

The per-block delta in bogosize is useful too, for the non cumulative version.

fjahr commented at 1:29 am on February 20, 2022:

done

fjahr force-pushed on Feb 20, 2022

fjahr commented at 1:28 am on February 20, 2022: contributor

Addressed @Sjors feedback and also squashed some commits because it was just easier to do the reorganization this way.

Maybe document the fields in the help, since some are not obvious. For each item, note if it has a cumulative version.

I was a bit unsure where to put this and how to format it but I tried my best.

A slightly more logical ordering of columns would be:

I applied that order

maybe insert: txins to count the number of outputs spent. I think you can use coin_count between two blocks and txouts to calculate it.

I think it’s not possible without updating coinstatsindex. The coin_count isn’t included there, I am not sure why but probably just bad timing since it was merged while I was already working on the index. Leaving this as a follow-up since I first have to think about updating the index. I also thought about calculating it with txouts but I think it would be (txouts - prev_txouts) - coinbase_txouts - unspendable_txouts and from those I am missing the unspendable counts as well.

txouts (document for cumulative: this is the total number of outputs ever created, not the total in existence at this height)

I actually didn’t have this as non-cummulative yet, but done as well.

maybe insert: the number txouts in existence at this height (the same for cumulative). It may be useful to track its ratio to bogosize over time.

I think you mean UTXOs here which would basically be coin_count? Or should it also include unspendable outputs?

prevout_spent_amount (maybe rename to amount_spent)

amount (or amount_created)

renamed both

unspendable_amount (docs should clarify this includes both immature and OP_RETURN)

No, immature is not included here. This is the just the total of the other unspendable values (genesis, bip30, scripts and unclaimed rewards)

in src/rpc/blockchain.cpp:1297 in 87f0cc381c outdated

1292+        file << strprintf("%s,", stats.hashBlock.GetHex());
1293+        file << strprintf("%d,", pindex->nTime);
1294+        file << strprintf("%d,", pindex->GetMedianTimePast());
1295+        file << strprintf("%s,", pindex->nVersion);
1296+
1297+        const int num_tx = cumulative ? pindex->nTx : pindex->nTx - (pindex->pprev ? pindex->pprev->nTx : 0);

Sjors commented at 3:25 pm on February 21, 2022:

nTx is not cumulative

0        num_tx_sum += pindex->nTx;
1        const int num_tx = cumulative ? num_tx_sum : pindex->nTx;

fjahr commented at 7:27 pm on March 6, 2022:

Done

in src/rpc/blockchain.cpp:1080 in 87f0cc381c outdated

1281+        coins_view = &active_chainstate.CoinsDB();
1282+        blockman = &active_chainstate.m_blockman;
1283+    }
1284+
1285+    CCoinsStats prev_stats{CoinStatsHashType::MUHASH};
1286+

Sjors commented at 3:26 pm on February 21, 2022:

0      uint64_t num_tx_sum = 0;

fjahr commented at 7:27 pm on March 6, 2022:

Done

in src/rpc/blockchain.cpp:1176 in 87f0cc381c outdated

1171+        "- block_hash\n"
1172+        "- timestamp\n"
1173+        "- mediantime\n"
1174+        "- version\n"
1175+        "- (total_)txs - Number of transactions included\n"
1176+        "- (total_)txouts - Number of transaction outputs created\n"

Sjors commented at 3:35 pm on February 21, 2022:

“Nett number of” or “outputs added”

See e.g. block 170 that has a coinbase output and a regular transaction with one input and two outputs, yet txouts is 2. https://www.blockchain.com/btc/block/00000000d1145790a8694403d4063f323d499e655c83426834d4ce2f8dd4a2ee

(it’d be nice to actually know how many were created, but see discussion elsewhere)

fjahr commented at 7:27 pm on March 6, 2022:

Done

in src/rpc/blockchain.cpp:1178 in 87f0cc381c outdated

1173+        "- mediantime\n"
1174+        "- version\n"
1175+        "- (total_)txs - Number of transactions included\n"
1176+        "- (total_)txouts - Number of transaction outputs created\n"
1177+        "- (total_)amount_spent - Amount of inputs spent\n"
1178+        "- (total_)amount_created - Amount of coins added to the UTXO set\n"

Sjors commented at 3:40 pm on February 21, 2022:

Maybe call this amount_generated and add “(never more than the block subsidy)

fjahr commented at 7:27 pm on March 6, 2022:

Done

in src/rpc/blockchain.cpp:1181 in 87f0cc381c outdated

1176+        "- (total_)txouts - Number of transaction outputs created\n"
1177+        "- (total_)amount_spent - Amount of inputs spent\n"
1178+        "- (total_)amount_created - Amount of coins added to the UTXO set\n"
1179+        "- (total_)coinbase_amount - Coinbase subsidy amount\n"
1180+        "- (total_)new_outputs_ex_coinbase_amount - Amount of new outputs created by this block\n"
1181+        "- (total_)subsidy - Total subsidy paid out\n"

Sjors commented at 3:54 pm on February 21, 2022:

Should be “Maximum subsidy”

Block 501,726 (0000000000000000004b27f9ee7ba33d6f048f684aaeb0eea4befd80f1701126) claimed 0 rewards, yet its subsidycolumn says 12.5.

Alternatively we can keep subsidy what it is, but then amount_generated should be negative.

https://bitcoin.stackexchange.com/questions/38994/will-there-be-21-million-bitcoins-eventually

Unrelated bug: the unspendable_amount for this block is set to 12.5 but should be zero (also wrong in gettxoutsetinfo). Same issue for block 526,591 which claimed half its reward (the other half is not unspendable, it doesn’t exist). This is different than the situation with the genesis block: that block does exist and does claim a reward, but it just can’t be spent.

fjahr commented at 7:27 pm on March 6, 2022:

Hehe, we are really getting into the weeds here :)

Yeah, the “paid out” part is definitely wrong. It wasn’t paid out but also it can never be paid out in the the future, I still find this hard to express concisely so changed to “Maximum subsidy”.

For the unspendable_amount comment: that is not a bug by my definition. When I wrote the coinstatsindex one of my goals was to make these non-existent amounts visible, and I summed this up with the other categories of lost coins under the umbrella term unspendables . See this code block in coinstatsindex:

0    const CAmount unclaimed_rewards{(m_total_prevout_spent_amount + m_total_subsidy) - (m_total_new_outputs_ex_coinbase_amount + m_total_coinbase_amount + m_total_unspendable_amount)};
1    m_total_unspendable_amount += unclaimed_rewards;
2    m_total_unspendables_unclaimed_rewards += unclaimed_rewards;

If the definition for anything “unspendable” was that it needed have to existed before as an output in a tx, then yes, the unclaimed rewards should not be part of this category. But I think this is confusing to some without deep knowledge of how the subsidies work. They might think that these amounts could still be claimed some time in the future, since they haven’t existed yet and there will be 21M, right? So I was more looking at this from the side of the 21M: for example the 12.5 are part of the 21M total (and part of the >19M that are considered mined) even though they will never exist because the moment to claim them has passed. Thus, I consider them unspendable even though they never even existed in the first place, i.e. “unspendable” includes “never spendable in the future”. If we don’t keep track of these to some degree we can never really do the full accounting that this export was intended for.

Sjors commented at 8:59 am on March 11, 2022:

I see, in that case just document that “unspendable” means “unspendable coins plus unclaimed subsidies”.

Though I think it’s cleaner to track unspendable and unclaimed_rewards separately, and then explain in the docs that “unclaimed” means it can never be claimed. I suspect anyone using an RPC like this will understand that.

fjahr commented at 10:55 pm on March 13, 2022:

Done, amended the doc for the unspendable_amount.

I see your reasoning but at least for now let’s keep it consistent with the index code and gettxoutsetinfo where the unclaimed rewards are in the unspendable ‘category’.

in src/rpc/blockchain.cpp:1179 in 87f0cc381c outdated

1174+        "- version\n"
1175+        "- (total_)txs - Number of transactions included\n"
1176+        "- (total_)txouts - Number of transaction outputs created\n"
1177+        "- (total_)amount_spent - Amount of inputs spent\n"
1178+        "- (total_)amount_created - Amount of coins added to the UTXO set\n"
1179+        "- (total_)coinbase_amount - Coinbase subsidy amount\n"

Sjors commented at 4:13 pm on February 21, 2022:

“Amount claimed in coinbase transaction.” (see e.g. block 409,008 which claimed a whopping 17K BTC.

fjahr commented at 7:26 pm on March 6, 2022:

Done

Sjors commented at 4:21 pm on February 21, 2022: member

87f0cc381c77e7d321d9e01a11a4725878c81810 almost there

Leaving this as a follow-up since I first have to think about updating the index

Yeah, let’s not touch the index itself here.

I think you mean UTXOs here which would basically be coin_count? Or should it also include unspendable outputs?

Excluding unspendable sounds good to me. Other than the coinbase, they’re not really part of the UTXO set in practice, e.g. they don’t use RAM and their bogo_size is zero too.

Seeing this in a spreadsheet slightly changed my thoughts on the optimal order. After txouts let’s do:

coinbase_amount
subsidy (which represents the maximum allowed subsidy)
amount_generated (in theory this can be less than zero, but afaik there has never been a block that included transactions but forgot to claim both the subsidy and fees)
unclaimed_rewards
amount_spent
new_outputs_ex_coinbase_amount

(then continue as it is now, from unspendable_scripts)

fjahr force-pushed on Mar 6, 2022

fjahr commented at 7:29 pm on March 6, 2022: contributor

Seeing this in a spreadsheet slightly changed my thoughts on the optimal order. After txouts let’s do:

coinbase_amount

subsidy (which represents the maximum allowed subsidy)

amount_generated (in theory this can be less than zero, but afaik there has never been a block that included transactions but forgot to claim both the subsidy and fees)

unclaimed_rewards

amount_spent

new_outputs_ex_coinbase_amount

(then continue as it is now, from unspendable_scripts)

Done

fjahr force-pushed on Mar 13, 2022

Sjors commented at 10:00 am on March 16, 2022: member

tACK 418331f35424bdc95dedba6023bf0d8f7819fabe

TIL block 367,853 has the highest number of transactions ever: 12,239

Added a wish list for more stats: https://github.com/bitcoin/bitcoin/issues/24581

DrahtBot added the label Needs rebase on Mar 16, 2022

fjahr force-pushed on Mar 16, 2022

fjahr commented at 11:49 pm on March 16, 2022: contributor

Way to kill the mood @DrahtBot ! Rebased…

DrahtBot removed the label Needs rebase on Mar 17, 2022

Sjors commented at 10:20 am on March 17, 2022: member

re-utACK 843159e5df3dc0d1ca0341b22aaa9fe86160994a

DrahtBot added the label Needs rebase on Apr 24, 2022

fjahr force-pushed on Apr 24, 2022

fjahr commented at 1:14 pm on April 24, 2022: contributor

Rebased

DrahtBot removed the label Needs rebase on Apr 24, 2022

Sjors commented at 5:10 pm on April 24, 2022: member

Half of CI seems unhappy…

fjahr force-pushed on Apr 24, 2022

fjahr commented at 9:33 pm on April 24, 2022: contributor

@Sjors Thanks! Fixed the silent merge conflict.

Sjors commented at 6:13 pm on April 25, 2022: member

re-tACK 19b12fa07653cb11a6fd97566af0036241eaa8c9

w0xlt approved

w0xlt commented at 0:42 am on April 26, 2022: contributor

tACK https://github.com/bitcoin/bitcoin/pull/19792/commits/19b12fa07653cb11a6fd97566af0036241eaa8c9

DrahtBot added the label Needs rebase on Apr 26, 2022

in src/rpc/blockchain.cpp:841 in 19b12fa076 outdated

836+            HelpExampleCli("dumpcoinstats", "\"test\"") +
837+            HelpExampleRpc("dumpcoinstats", "\"test\"")
838+        },
839+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
840+{
841+    if (request.params[0].isNull()) {

MarcoFalke commented at 9:01 am on April 29, 2022:

nit: I think for new code it is fine to use the normal indent (8 spaces and 12 spaces for those lines, respectively)

fjahr commented at 1:24 pm on May 1, 2022:

done

fjahr force-pushed on May 1, 2022

fjahr commented at 1:24 pm on May 1, 2022: contributor

Rebased and changed indentation per @MarcoFalke ’s request.

in src/rpc/blockchain.cpp:924 in 9d45f7c455 outdated

919+            CCoinsStats prev_stats{CoinStatsHashType::MUHASH};
920+            uint64_t num_tx_sum = 0;
921+
922+            for (; pindex; pindex = active_chainstate.m_chain.Next(pindex)) {
923+                if (!GetUTXOStats(coins_view, *blockman, stats, node.rpc_interruption_point, pindex))
924+                    throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set");

w0xlt commented at 1:43 pm on May 1, 2022:

nit: Perhaps the error message could be more specific?

0                    throw JSONRPCError(RPC_INTERNAL_ERROR, "Failed to generate coins stats");

fjahr commented at 3:48 pm on May 1, 2022:

Thanks for the quick re-review! I adapted your suggestion by adding the height of the failure as well, which should make is more helpful in case of corruption in the index.

w0xlt approved

w0xlt commented at 1:43 pm on May 1, 2022: contributor

reACK https://github.com/bitcoin/bitcoin/pull/19792/commits/9d45f7c455318089fb73dc275efbf6e176d971e2

DrahtBot removed the label Needs rebase on May 1, 2022

fjahr force-pushed on May 1, 2022

DrahtBot added the label Needs rebase on May 24, 2022

fjahr force-pushed on May 27, 2022

fjahr commented at 11:55 pm on May 27, 2022: contributor

Rebased. The first commit has become obsolete, so it was removed.

DrahtBot removed the label Needs rebase on May 28, 2022

Sjors commented at 9:29 am on May 30, 2022: member

re-tACK 3b3e3d966ad44b37bab3659b0d840f8384da05be

in src/rpc/blockchain.cpp:993 in 3b3e3d966a outdated

986@@ -985,6 +987,202 @@ static RPCHelpMan gettxoutsetinfo()
987     };
988 }
989 
990+static RPCHelpMan dumpcoinstats()
991+{
992+    return RPCHelpMan{"dumpcoinstats",
993+        "Dumps content of coinstats index to a CSV file. The resulting file contains the following columns:\n"

MarcoFalke commented at 12:02 pm on May 30, 2022:

Could mention that this will have a line for each block?

fjahr commented at 4:02 pm on June 11, 2022:

done

in src/rpc/blockchain.cpp:1044 in 3b3e3d966a outdated

1039+                throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.u8string() + " already exists. If you are sure this is what you want, move it out of the way first");
1040+            }
1041+
1042+            std::ofstream file;
1043+            file.open(filepath);
1044+            if (!file.is_open())

MarcoFalke commented at 12:03 pm on May 30, 2022:

0            if (!file.is_open()) {

fjahr commented at 4:02 pm on June 11, 2022:

done

in src/rpc/blockchain.cpp:1097 in 3b3e3d966a outdated

1092+            }
1093+
1094+            NodeContext& node = EnsureAnyNodeContext(request.context);
1095+            ChainstateManager& chainman = EnsureChainman(node);
1096+            CChainState& active_chainstate = chainman.ActiveChainstate();
1097+            active_chainstate.ForceFlushStateToDisk();

MarcoFalke commented at 12:04 pm on May 30, 2022:

Pretty sure flushing and getting the cursor will need to happen in the same cs_main scope, otherwise the flush seems best-effort at best?

fjahr commented at 4:02 pm on June 11, 2022:

Yeah, I would have described it as best effort so I think it can be omitted here as well. And this made me realize that we don’t actually need coins_view and blockman, so I adapted this as well.

in src/rpc/blockchain.cpp:1114 in 3b3e3d966a outdated

1109+            uint64_t num_tx_sum = 0;
1110+
1111+            for (; pindex; pindex = active_chainstate.m_chain.Next(pindex)) {
1112+                const std::optional<CCoinsStats> maybe_stats{GetUTXOStats(coins_view, *blockman, CoinStatsHashType::MUHASH, node.rpc_interruption_point, pindex)};
1113+
1114+                if (!maybe_stats)

MarcoFalke commented at 12:05 pm on May 30, 2022:

0                if (!maybe_stats) {

Also, could mention that the file may have been created and may need to be manually cleared now?

fjahr commented at 4:01 pm on June 11, 2022:

done

in src/rpc/blockchain.cpp:1117 in 3b3e3d966a outdated

1112+                const std::optional<CCoinsStats> maybe_stats{GetUTXOStats(coins_view, *blockman, CoinStatsHashType::MUHASH, node.rpc_interruption_point, pindex)};
1113+
1114+                if (!maybe_stats)
1115+                    throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Failed to get coins stats at height %d", pindex->nHeight));
1116+
1117+                CCoinsStats stats{maybe_stats.value()};

MarcoFalke commented at 12:06 pm on May 30, 2022:

Any reason to create a copy when a reference should be enough?

fjahr commented at 4:01 pm on June 11, 2022:

done

in test/functional/feature_coinstatsindex.py:315 in 3b3e3d966a outdated

310+            if cumulative:
311+                file = "ctest.csv"
312+            else:
313+                file = "test.csv"
314+
315+            path = os.path.join(get_datadir_path(self.options.tmpdir, 1), file)

MarcoFalke commented at 12:10 pm on May 30, 2022:

Probably time to introduce a helper and use that:

 0diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
 1index 7d2db391b6..aa9ff9831f 100755
 2--- a/test/functional/test_framework/test_node.py
 3+++ b/test/functional/test_framework/test_node.py
 4@@ -381,9 +381,13 @@ class TestNode():
 5     def wait_until_stopped(self, timeout=BITCOIND_PROC_WAIT_TIMEOUT):
 6         wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
 7 
 8+ [@property](/bitcoin-bitcoin/contributor/property/)
 9+    def datadir_path(self) -> Path:
10+        return Path(self.datadir)
11+ [@property](/bitcoin-bitcoin/contributor/property/)
12     def chain_path(self) -> Path:
13-        return Path(self.datadir) / self.chain
14+        return self.datadir_path / self.chain [@property](/bitcoin-bitcoin/contributor/property/)
15     def debug_log_path(self) -> Path:

0            path = self.nodes[1].datadir_path /  file

fjahr commented at 4:01 pm on June 11, 2022:

I like the helper but using the / operator would mean file would also need to be a Path object iiuc. So, instead I opted for a slightly different approach that I liked a little better using another helper datadir_file_path. What do you think?

MarcoFalke commented at 4:11 pm on June 11, 2022:

but using the / operator would mean file would also need to be a Path object iiuc

Does it? (It doesn’t for me locally)

fjahr commented at 4:30 pm on June 11, 2022:

Hm, seems like I misinterpreted the error, actually the next line fails. And I am now also seeing it fail with the Path object but I am sure it previously passed at least twice with it. Very strange. Maybe a python version or platform issue, need to look deeper into this.

 02022-06-11T16:28:24.998000Z TestFramework (INFO): Test dumpcoinstats RPC
 12022-06-11T16:28:24.998000Z TestFramework (ERROR): Unexpected exception caught during testing
 2Traceback (most recent call last):
 3  File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
 4    self.run_test()
 5  File "/Users/FJ/projects/clones/bitcoin/test/functional/feature_coinstatsindex.py", line 58, in run_test
 6    self._test_dumpcoinstats_rpc()
 7  File "/Users/FJ/projects/clones/bitcoin/test/functional/feature_coinstatsindex.py", line 319, in _test_dumpcoinstats_rpc
 8    self.nodes[1].dumpcoinstats(path, cumulative)
 9  File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/coverage.py", line 49, in __call__
10    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
11  File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
12    postdata = json.dumps(self.get_request(*args, **argsn), default=EncodeDecimal, ensure_ascii=self.ensure_ascii)
13  File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/authproxy.py", line 131, in get_request
14    json.dumps(args or argsn, default=EncodeDecimal, ensure_ascii=self.ensure_ascii),
15  File "/opt/homebrew/Cellar/python@3.9/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/__init__.py", line 234, in dumps
16    return cls(
17  File "/opt/homebrew/Cellar/python@3.9/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/encoder.py", line 199, in encode
18    chunks = self.iterencode(o, _one_shot=True)
19  File "/opt/homebrew/Cellar/python@3.9/3.9.12/Frameworks/Python.framework/Versions/3.9/lib/python3.9/json/encoder.py", line 257, in iterencode
20    return _iterencode(o, 0)
21  File "/Users/FJ/projects/clones/bitcoin/test/functional/test_framework/authproxy.py", line 66, in EncodeDecimal
22    raise TypeError(repr(o) + " is not JSON serializable")
23TypeError: PosixPath('/var/folders/9z/n7rz_6cj3bq__11k5kcrsvvm0000gn/T/bitcoin_func_test_ru27w0qw/node1/ctest.csv') is not JSON serializable

MarcoFalke commented at 5:01 pm on June 11, 2022:

You could do str(path) before passing it as json?

fjahr commented at 6:51 pm on June 11, 2022:

Yeah, that works too and I just pushed a version with that. But I find it a little less elegant overall.

MarcoFalke approved

MarcoFalke commented at 12:11 pm on May 30, 2022: member

Not sure about the cs_main locking assumptions

fjahr force-pushed on Jun 11, 2022

in test/functional/rpc_users.py:115 in 42763a243a outdated

111@@ -112,7 +112,7 @@ def run_test(self):
112         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar$baz'])
113 
114         self.log.info('Check that failure to write cookie file will abort the node gracefully')
115-        cookie_file = os.path.join(get_datadir_path(self.options.tmpdir, 0), self.chain, '.cookie.tmp')
116+        cookie_file = str(self.nodes[0].datadir_path / self.chain / '.cookie.tmp')

MarcoFalke commented at 12:39 pm on June 12, 2022:

0        cookie_file = self.nodes[0].chain_path / '.cookie.tmp'

nit?

fjahr commented at 3:55 pm on June 12, 2022:

nit!

fjahr force-pushed on Jun 12, 2022

aureleoules commented at 3:06 pm on October 14, 2022: member

If the index has not fully been synced yet, the error thrown would be:

Failed to get coins stats at height X. You may have to remove an incomplete dump file before running this command again.

It’s not very clear that the index has not finished syncing. Also, the csv file would be created before this error is thrown, so the file would have to be removed manually.

I would suggest checking if the index is synced before dumping and create the file only if the index is ready+enabled, with something like this:

 0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
 1index 74877d7da..7bb6e2927 100644
 2--- a/src/rpc/blockchain.cpp
 3+++ b/src/rpc/blockchain.cpp
 4@@ -1039,16 +1039,21 @@ static RPCHelpMan dumpcoinstats()
 5                 throw JSONRPCError(RPC_INVALID_PARAMETER, filepath.u8string() + " already exists. If you are sure this is what you want, move it out of the way first");
 6             }
 7 
 8+            if (!g_coin_stats_index) {
 9+                throw JSONRPCError(RPC_MISC_ERROR, "Coinstats index is not enabled");
10+            }
11+
12+            const auto summary{g_coin_stats_index->GetSummary()};
13+            if (!summary.synced) {
14+                throw JSONRPCError(RPC_MISC_ERROR, strprintf("coinstatsindex is not synced. Current best block height: %d", summary.best_block_height));
15+            }
16+
17             std::ofstream file;
18             file.open(filepath);
19             if (!file.is_open()) {
20                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open dump file");
21             }
22 
23-            if (!g_coin_stats_index) {
24-                throw JSONRPCError(RPC_MISC_ERROR, "Coinstats index is not enabled");
25-            }
26-
27             const bool cumulative{request.params[1].isNull() ? false : request.params[1].get_bool()};
28 
29             file << strprintf("height,"

in src/rpc/blockchain.cpp:1171 in c8e63f8e7d outdated

1166+                const uint64_t bogo_size = cumulative ? stats.nBogoSize : stats.nBogoSize - prev_stats.nBogoSize;
1167+                file << strprintf("%d\n", static_cast<int64_t>(bogo_size));
1168+
1169+                prev_stats = stats;
1170+
1171+                if (pindex->nHeight >= active_chainstate.m_chain.Tip()->nHeight) break;

aureleoules commented at 3:48 pm on October 14, 2022:

Is it possible that the index has not synced up to active_chainstate.m_chain.Tip() if the tip moved during the dump? I don’t see any lock for that. Could be fixed with this?

0                if (pindex->nHeight >= g_coin_stats_index->GetSummary().best_block_height) break;

fjahr commented at 3:41 pm on December 22, 2022:

Yeah, I agree, there is probably a very small chance of this happening and it is avoided with this change.

fjahr force-pushed on Dec 22, 2022

fjahr commented at 3:44 pm on December 22, 2022: contributor

Addressed comments from @aureleoules , thanks for the review!

Further note: I can’t change the status back it seems, but please treat this as a draft for now. This PR will need to be updated and rebased soon when I have resolved the overflow issue noted in #26362.

EDIT: Rebased as well since there was a silent merge conflict.

fanquake marked this as a draft on Dec 22, 2022

fjahr force-pushed on Dec 22, 2022

fjahr commented at 3:51 pm on December 22, 2022: contributor

Thanks @fanquake , lightning speed :)

fjahr force-pushed on Dec 22, 2022

DrahtBot added the label Needs rebase on Apr 3, 2023

rpc: Add dumpcoinstats

This RPC dumps the full content of the coinstats index together with some other statistics into a CSV file.

906d54fc8c

test: Add dumpcoinstats RPC functional test 8094ab6486

test: Introduce datadir_path helper 9ec745cfdc

fjahr force-pushed on May 3, 2023

DrahtBot removed the label Needs rebase on May 3, 2023

DrahtBot added the label CI failed on May 14, 2023

DrahtBot added the label Needs rebase on May 17, 2023

DrahtBot commented at 7:26 pm on May 17, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot commented at 1:40 am on August 15, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

fjahr commented at 2:03 pm on August 15, 2023: contributor

There doesn’t seem to be much interest in this feature currently. If someone needs it, feel free to ping me here and I will reorg/rebase for you.

fjahr closed this on Aug 15, 2023

rpc: Add dumpcoinstats #19792

Reviews

Conflicts