Coinstats Index

fjahr commented at 11:58 pm on July 14, 2020: member

This is part of the coinstats index project tracked in #18000

While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run gettxoutsetinfo with a specific hash type. As the first type it added hash_type=none which skips the hashing of the UTXO set altogether. This alone did not make gettxoutsetinfo much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.

Features summary:

Users can start their node with the option -coinstatsindex which syncs the index in the background
After the index is synced the user can use gettxoutsetinfo with hash_type=none or hash_type=muhash and will get the response instantly out of the index
The user can specify a height or block hash when calling gettxoutsetinfo to see coin statistics at a specific block height

fanquake added the label UTXO Db and Indexes on Jul 14, 2020

DrahtBot commented at 5:44 am on July 15, 2020: 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:

#21726 (Add prune blockers to BlockManager by fjahr)
#21584 (Fix assumeutxo crash due to invalid base_blockhash by MarcoFalke)
#20295 (rpc: getblockfrompeer by Sjors)
#17167 (Allow whitelisting outgoing connections by luke-jr)
#13462 (Make SER_GETHASH implicit for CHashWriter and SerializeHash by Empact)

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 Jul 15, 2020

in src/init.cpp:964 in 0269ff80aa outdated

1016+    // if using block pruning, then disallow txindex, coinstatsindex and blockfilterindex
1017     if (gArgs.GetArg("-prune", 0)) {
1018         if (gArgs.GetBoolArg("-txindex", DEFAULT_TXINDEX))
1019             return InitError(_("Prune mode is incompatible with -txindex."));
1020+        if (gArgs.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX))
1021+            return InitError(_("Prune mode is incompatible with -coinstatsindex."));

luke-jr commented at 11:37 pm on July 30, 2020:

Why?

fjahr commented at 7:41 pm on August 1, 2020:

It syncs over the whole blockchain, processing every block and saving state on each. That allows users to query the stats for every block height (a nice-to-have feature). It’s the standard way BaseIndex currently works, although changing that would probably only require minimal effort. Potential users who I have talked to didn’t really care much about this. But I will look into it if this again since progress is slow anyway currently.

Sjors commented at 2:25 pm on December 9, 2020:

As a potential user, it would be nice if I can at least turn on pruning after the index is generated, but I think that’s for a separate PR.

achow101 commented at 8:56 pm on March 29, 2021:

I don’t think it is necessary to disallow the coin stats index when pruning is enabled. The only reason that txindex disallows it is because txindex gets the transaction data by reading it off disk from the block files, so that is incompatible with pruning.

The coin stats index doesn’t have the same problem. The only issue would be if there were a large reorg and it needed the undo data that has already been deleted. However this is an issue overall of pruned nodes so it shouldn’t be a concern for the coin stats index.

in src/index/coinstatsindex.cpp:24 in 0269ff80aa outdated

15+
16+namespace {
17+
18+struct DBVal {
19+    uint64_t transaction_output_count;
20+    uint64_t bogo_size;

PierreRochard commented at 2:17 am on August 17, 2020:

A comment explaining “bogo” would be helpful, I looked at the function that calculates it and it didn’t make intuitive sense to me.

fjahr commented at 10:18 pm on August 17, 2020:

I think bogo just stands for bogus, indicating that this metric is not meaningful in any way other than comparing it to itself, i.e. has the UTXO set grown or shrunk over the last x blocks. I added a comment on the calculation and improved the RPC help with the same text.

in src/index/coinstatsindex.cpp:112 in 0269ff80aa outdated

106+
107+bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
108+{
109+    CBlockUndo block_undo;
110+
111+    // Ignore genesis block

PierreRochard commented at 2:19 am on August 17, 2020:

Can we add unspendable_amount to the index data and include the genesis block in it?

in src/index/coinstatsindex.cpp:139 in 0269ff80aa outdated

130+
131+        // Add the new utxos created from the block
132+        for (size_t i = 0; i < block.vtx.size(); ++i) {
133+            const auto& tx = block.vtx.at(i);
134+
135+            // Skip duplicate txid coinbase transactions (BIP30).

PierreRochard commented at 2:20 am on August 17, 2020:

I’m assuming these would be added to unspendable_amount too

in src/index/coinstatsindex.cpp:145 in 0269ff80aa outdated

140+            for (size_t j = 0; j < tx->vout.size(); ++j) {
141+                const CTxOut& out = tx->vout[j];
142+                Coin coin = Coin(out, pindex->nHeight, tx->IsCoinBase());
143+
144+                // Skip unspendable coins
145+                if (coin.out.scriptPubKey.IsUnspendable()) continue;

PierreRochard commented at 2:21 am on August 17, 2020:

I think this would be 3/3 of unspendable_amount items.

fjahr commented at 10:18 pm on August 17, 2020:

Actually 3/4 :) The unclaimed block rewards need to be included as well. (See code and/or my blog post)

in src/index/coinstatsindex.cpp:25 in 0269ff80aa outdated

16+namespace {
17+
18+struct DBVal {
19+    uint64_t transaction_output_count;
20+    uint64_t bogo_size;
21+    CAmount total_amount;

PierreRochard commented at 2:38 am on August 17, 2020:

If I understand the logic, total_amount is the net increase of coins due to this block?

I don’t want to bloat your stats index, but I’d be interested in adding total_prevout_spent_amount, total_new_outputs_ex_coinbase_amount, and coinbase_amount to unpack total_amount.

fjahr commented at 10:18 pm on August 17, 2020:

It is the total amount of coins in the UTXO set at that block height. So it’s accumulative rather than for this specific block. I added the other values in a draft commit, needs some cleanup but ready to test.

PierreRochard changes_requested

PierreRochard commented at 2:44 am on August 17, 2020: contributor

To reconcile total issuance with the spendable UTXO set, it would be helpful to sum up unspendable amounts.

DrahtBot added the label Needs rebase on Aug 17, 2020

fjahr commented at 10:18 pm on August 17, 2020: member

@PierreRochard Thanks a lot for the comments! I am happy to include these values you requested and drafted that up in a new commit (will need another day for clean-up, docs, tests etc. but it should work already). I actually had done some similar work already for my blog post and was thinking about adding some more numbers myself, just opted to keep the changeset small at the end.

The RPC now has the values for every block by passing a verbose flag true after the block indicator (example is testnet):

 0$ src/bitcoin-cli gettxoutsetinfo 'none' 1800003 true
 1{
 2  "height": 1800003,
 3  "bestblock": "00000000000005359e00fe3d069b2999baaaf4d69a34492e543f9f82047ddd01",
 4  "txouts": 23990594,
 5  "bogosize": 1799579780,
 6  "disk_size": 1341237413,
 7  "total_amount": 20940531.15125810,
 8  "block_info": {
 9    "unspendable_amount": 0.00000000,
10    "total_prevout_spent_amount": 0.21428778,
11    "total_new_outputs_ex_coinbase_amount": 0.21428778,
12    "coinbase_amount": 0.19531250
13  }
14}

I just noticed when I was almost done, that I wasn’t sure if you would like these requested values to be cumulative or per-block. They are per-block now but it is trivial to change that. Actually, I could even do both. And another questions: should the unspendable output values be included or excluded from the other output values. Example: src/bitcoin-cli gettxoutsetinfo 'none' 0 true shows coinbase: 50.0 although that is unspendable. Same for the total_new_outputs_ex_coinbase_amount if there is an OP_RETURN for example. Just a question of the definition.

And I saw your request for the data dump on twitter. Will look into that as a follow-up :)

fjahr force-pushed on Aug 17, 2020

benthecarman commented at 11:02 pm on August 17, 2020: contributor

Concept ACK

Would be nice to be able to see the total_unspendable_amount up to that current block.

DrahtBot removed the label Needs rebase on Aug 18, 2020

jonatack commented at 7:52 am on August 18, 2020: member

Concept ACK

in src/index/coinstatsindex.cpp:203 in 7139c17988 outdated

198+    if (pindex->nHeight > 0) {
199+        m_total_new_outputs_ex_coinbase_amount = total_out - block_subsidy;
200+
201+        // Unclaimed block rewards
202+        if ((total_in + block_subsidy) > total_out) {
203+            m_unspendable_amount += (total_in + block_subsidy - total_out);

PierreRochard commented at 9:46 pm on August 20, 2020:

I think this is causing BIP30 blocks to be double counted as unspendable

bitcoin-cli gettxoutsetinfo 'none' 91722 true

"unspendable_amount": 100.00000000

PierreRochard commented at 11:39 pm on August 20, 2020: contributor

values to be cumulative or per-block

I find it easier to do the cumulative sum in pandas than to back in to the per-block, but the answer also depends on the performance of the underlying index for aggregate queries. If this behaves like a SQL database and we want it to be normalized, it should be per-block, but I’m not religious about it.

fjahr force-pushed on Aug 21, 2020

fjahr commented at 0:33 am on August 21, 2020: member

@PierreRochard Thanks for testing and further feedback. As I have written the tests today and did further cleanup I have found that I hadn’t been really doing what you were looking for in the coinbase_amount. I was simply returning the block subsidy there.

I am pushing my fixes and test as a separate commit for now since you were already testing with the first version:

Adds total_unspendable_amount, the cumulative number as requested
Tests for different unspendables (except BIP30 because that is pretty hard to test)
Cleans up internal naming and removes some indirections that caused bugs in some scenarios (removes total_in and total_out for example)

I think the formula in block_sanity_check in the test is what should hold for each block and that is how you intended it, correct?

0    assert_equal(
1        block_info['total_prevout_spent_amount'] + block_subsidy,
2        block_info['total_new_outputs_ex_coinbase_amount'] + block_info['coinbase_amount'] + block_info['unspendable_amount']
3    )

I think the new code also fixes the BIP30 issue you discovered but I am still syncing the new version of the index with not-super-fast hardware so I will be able to check tomorrow.

PierreRochard commented at 2:09 am on August 21, 2020: contributor

Yes, perfect, that equation is exactly right. I’ll just delete the coinstats index file for now but does this indexing system have versioning/migrations?

benthecarman commented at 5:19 am on August 21, 2020: contributor

From testnet:

 0gettxoutsetinfo  none 1807942 true
 1{
 2  "height": 1807942,
 3  "bestblock": "00000000374715d95667998b720a60bf8241182b279c913573f5e9e3fa54ba00",
 4  "txouts": 24100924,
 5  "bogosize": 1807675952,
 6  "disk_size": 1318392833,
 7  "total_amount": 20942081.72908999,
 8  "total_unspendable_amount": 875.88809751,
 9  "block_info": {
10    "unspendable_amount": 0.00000000,
11    "total_prevout_spent_amount": 16798.39641164,
12    "total_new_outputs_ex_coinbase_amount": 16798.36929066,
13    "coinbase_amount": 0.22243348
14  }
15}

Looking good!

fjahr commented at 12:26 pm on August 21, 2020: member

Yes, perfect, that equation is exactly right. I’ll just delete the coinstats index file for now but does this indexing system have versioning/migrations?

Great! There is only a different kind of migration for txindex because it was around before the whole BaseIndex class was introduced. It is something that I have been thinking about already because I will add the UTXO set hash at some point. I am still figuring out what would work best for Coinstats but also other indexes in general, so for now the index needs to be deleted and resynced, unfortunately.

fjahr force-pushed on Aug 22, 2020

fjahr commented at 7:51 pm on August 22, 2020: member

Cleaned up the latest changes and tests in more coherent commits. This is ready for review now.

in src/index/coinstatsindex.cpp:173 in 79535960d1 outdated

168+                if (coin.out.scriptPubKey.IsUnspendable()) {
169+                    m_block_unspendable_amount += coin.out.nValue;
170+                    continue;
171+                }
172+
173+                if (tx->IsCoinBase() && j == 0) {

PierreRochard commented at 2:38 am on August 23, 2020:

This only counts the first output of the coinbase tx as being the coinbase amount, which doesn’t necessarily hold true ( one example https://blockstream.info/block/0000000000000af7c56d09abdbce2a36ba9aebbd8559b3d0460defaeaf3c0f31 )

fjahr commented at 11:03 am on August 23, 2020:

You are right. I just pushed a fix for this and also improved the tests to account for this case.

fjahr force-pushed on Aug 23, 2020

kallewoof commented at 1:39 am on August 26, 2020: member

@fjahr

I’m late to the party, but:

I just noticed when I was almost done, that I wasn’t sure if you would like these requested values to be cumulative or per-block.

It is trivial to convert cumulative to per-block by simply taking value(block) - value(prevblock), but the other way requires iterating over all the blocks, so I think cumulative would be more useful.

DrahtBot added the label Needs rebase on Aug 26, 2020

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

Would it be too verbose to split out unspendables by the 4 categories? I realize it is granular, but it would remove any ambiguity in the reconciliation.

fjahr commented at 4:53 pm on August 28, 2020: member

Would it be too verbose to split out unspendables by the 4 categories? I realize it is granular, but it would remove any ambiguity in the reconciliation.

I think more verbose is not a problem itself unless it creates other problems, the only serious argument against it IMO would be if it ends up taking up too much disk space but the index is still pretty small and unless I change it to be accumulative numbers it should mostly be zeros anyway which should let the index only grow very little. I am just still thinking about if would really make sense to record all four categories because it is technically not possible that there will ever be another Genesis block or another BIP30 block, so recording these two values (even if it’s just 0s) for every block forever seems kind of pointless in a way. But then if I leave them out it’s annoying that it can’t be a clean sum of all the categories to get the total unspendable. So, happy to do it but still struggling a bit with finding the right approach.

fjahr commented at 5:14 pm on August 28, 2020: member

@fjahr

I’m late to the party, but:

I just noticed when I was almost done, that I wasn’t sure if you would like these requested values to be cumulative or per-block.

It is trivial to convert cumulative to per-block by simply taking value(block) - value(prevblock), but the other way requires iterating over all the blocks, so I think cumulative would be more useful.

Two concept ACKs, I think you are still early :) And that is a good point. I think I will do the following then: I will record cumulative numbers on the index but also return the per-block numbers in the rpc by default using the prevblock numbers. Since I have written that comment I have thought a bit more about the casual (non-auditor) user who might just want to check on a specific block height and for them the per-block numbers are much more intuitive and I would like to avoid another situation where the users see the very large cumulative numbers and ask questions about what it means, if it’s a bug etc.

fjahr force-pushed on Aug 28, 2020

fjahr commented at 5:16 pm on August 28, 2020: member

Only rebased for now, will work on the suggested changes shortly.

DrahtBot removed the label Needs rebase on Aug 28, 2020

in src/init.cpp:24 in fe86a280c3 outdated

22@@ -23,6 +23,7 @@
23 #include <httpserver.h>
24 #include <index/blockfilterindex.h>
25 #include <index/txindex.h>
26+#include <index/coinstatsindex.h>

jonatack commented at 4:03 pm on August 30, 2020:

9981a8f nit: sort

fjahr commented at 10:58 pm on September 14, 2020:

Fixed

in src/init.cpp:443 in fe86a280c3 outdated

439@@ -432,6 +440,7 @@ void SetupServerArgs(NodeContext& node)
440                  strprintf("Maintain an index of compact filters by block (default: %s, values: %s).", DEFAULT_BLOCKFILTERINDEX, ListBlockFilterTypes()) +
441                  " If <type> is not supplied or if <type> = 1, indexes for all known types are enabled.",
442                  ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
443+    argsman.AddArg("-coinstatsindex", strprintf("Maintain coin statistics index, used by the gettxoutset rpc call (default: %u)", DEFAULT_COINSTATSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

jonatack commented at 4:07 pm on August 30, 2020:

9981a8f

s/rpc/RPC/ (can also fix the one in txindex)
omit “call” (the C in RPC stands for “call”)
drop the comma at s/index, /index /
nit: sort for both -coinstatsindex and -blockfilterindex (they should be at line 411)

fjahr commented at 10:58 pm on September 14, 2020:

Fixed

jonatack commented at 4:40 pm on August 30, 2020: member

Checkpoint review/smoke test at fe86a280c3f49da8: debug build clean, tests clean, the index builds on testnet when launched with -coinstatsindex, and the updated rpc runs even while the index is still building (at a higher height):

0$ bitcoin-cli -testnet getindexinfo
1{
2  "coinstatsindex": {
3    "synced": false,
4    "best_block_height": 1355104
5  }
6}

 0$ bitcoin-cli -testnet gettxoutsetinfo none 1355000 true
 1{
 2  "height": 1355000,
 3  "bestblock": "00000000769e833f6293de2808438a4b507994bfe84d8ca040a515a878674c21",
 4  "txouts": 19494870,
 5  "bogosize": 1465308169,
 6  "disk_size": 1315794956,
 7  "total_amount": 20745258.46782398,
 8  "total_unspendable_amount": 836.06342602,
 9  "block_info": {
10    "unspendable_amount": 0.00000500,
11    "total_prevout_spent_amount": 17388.87556776,
12    "total_new_outputs_ex_coinbase_amount": 17387.97225198,
13    "coinbase_amount": 1.68456078
14  }
15}

OTOH on a mainnet node, which has been running txindex=1 peerbloomfilters=1 blockfilterindex=1 without issues, this PR crashed repeatedly:

02020-08-30T16:32:25Z coinstatsindex thread start
12020-08-30T16:32:25Z Syncing coinstatsindex with block chain from height 632589
22020-08-30T16:32:26Z *** ThreadSync: Failed to write block 000000000000000000017ff124286aa9a7c80a353c11bd7bd81e8a4b467c2e34 to index database
32020-08-30T16:32:26Z Error: A fatal internal error occurred, see debug.log for details
4Error: A fatal internal error occurred, see debug.log for details
52020-08-30T16:32:26Z coinstatsindex thread exit
6...
72020-08-30T16:32:26Z Shutdown: In progress...
8...
92020-08-30T16:32:26Z Shutdown: done

EDIT: I’m relieved to report that back on master, all is still ok

 0$ bitcoin-cli getindexinfo
 1{
 2  "txindex": {
 3    "synced": true,
 4    "best_block_height": 646005
 5  },
 6  "basic block filter index": {
 7    "synced": true,
 8    "best_block_height": 646005
 9  }
10}

Looking forward to re-testing when the latest changes are ready.

fjahr commented at 7:59 pm on August 30, 2020: member

OTOH on a mainnet node, which has been running txindex=1 peerbloomfilters=1 blockfilterindex=1 without issues, this PR crashed repeatedly:

02020-08-30T16:32:25Z coinstatsindex thread start
12020-08-30T16:32:25Z Syncing coinstatsindex with block chain from height 632589
22020-08-30T16:32:26Z *** ThreadSync: Failed to write block 000000000000000000017ff124286aa9a7c80a353c11bd7bd81e8a4b467c2e34 to index database
32020-08-30T16:32:26Z Error: A fatal internal error occurred, see debug.log for details
4Error: A fatal internal error occurred, see debug.log for details
52020-08-30T16:32:26Z coinstatsindex thread exit
6...
72020-08-30T16:32:26Z Shutdown: In progress...
8...
92020-08-30T16:32:26Z Shutdown: done

Thanks for taking another look @jonatack . Maybe you had an older version of coinstatsindex running on that node before? You need to delete the folder and fully resync in that case. I have not taken care of a migration mechanism yet. That could explain such crashes.

jonatack commented at 8:09 pm on August 30, 2020: member

Thanks for taking another look @jonatack . Maybe you had an older version of coinstatsindex running on that node before? You need to delete the folder and fully resync in that case. I have not taken care of a migration mechanism yet. That could explain such crashes.

Thanks @fjahr, that could explain it; I tested building the index with #18000 and only on mainnet.

jonatack commented at 8:23 pm on August 30, 2020: member

Yup, all good now.

0rm -rf ~/.bitcoin/indexes/coinstats

02020-08-30T20:17:32Z coinstatsindex thread start
12020-08-30T20:17:32Z Syncing coinstatsindex with block chain from height 0
2...
32020-08-30T20:21:09Z Syncing coinstatsindex with block chain from height 150291
42020-08-30T20:21:40Z Syncing coinstatsindex with block chain from height 172129
52020-08-30T20:22:11Z Syncing coinstatsindex with block chain from height 183549
62020-08-30T20:22:42Z Syncing coinstatsindex with block chain from height 190364

 0$ ./src/bitcoin-cli getindexinfo
 1{
 2  "txindex": {
 3    "synced": true,
 4    "best_block_height": 646031
 5  },
 6  "coinstatsindex": {
 7    "synced": false,
 8    "best_block_height": 203567
 9  },
10  "basic block filter index": {
11    "synced": true,
12    "best_block_height": 646031
13  }
14}

in src/index/coinstatsindex.h:43 in d28fed9b50 outdated

38+
39+public:
40+    // Constructs the index, which becomes available to be queried.
41+    explicit CoinStatsIndex(size_t n_cache_size, bool f_memory = false, bool f_wipe = false);
42+
43+    // Look up hash digest for a specific block using CBlockIndex

Sjors commented at 2:01 pm on September 9, 2020:

nit: look up stats

fjahr commented at 10:59 pm on September 14, 2020:

Fixed

in src/index/coinstatsindex.cpp:296 in d28fed9b50 outdated

249+{
250+    // First check if the result is stored under the height index and the value there matches the
251+    // block hash. This should be the case if the block is on the active chain.
252+    std::pair<uint256, DBVal> read_out;
253+    if (!db.Read(DBHeightKey(block_index->nHeight), read_out)) {
254+        return false;

Sjors commented at 2:15 pm on September 9, 2020:

This would prematurely return if the active chain is shorter? It currently doesn’t matter, because gettxoutsetinfo only takes a height argument.

fjahr commented at 11:00 pm on September 14, 2020:

I’m not sure what you mean here. Like, in case of a reorg?

Sjors commented at 2:27 pm on September 21, 2020:

A rather large reorg (with difficulty adjustments), where the new chain is shorter, but has more proof of work.

in src/index/coinstatsindex.cpp:98 in d28fed9b50 outdated

93+        if (m_db->Exists(DB_BLOCK_HASH)) {
94+            return error("%s: Cannot read current %s state; index may be corrupted",
95+                         __func__, GetName());
96+        }
97+
98+        m_transaction_output_count = 0;

Sjors commented at 2:34 pm on September 9, 2020:

Why are these values only initialised is no result is found? (and result is not used, so this just a sanity check?). Should this be initialised with the most recent block?

fjahr commented at 11:00 pm on September 14, 2020:

Yeah, this was just to initialize the values when the index is initialized itself. But initializing the values in the object is sufficient so I removed this.

in src/index/coinstatsindex.cpp:119 in d28fed9b50 outdated

113+        if (!UndoReadFromDisk(block_undo, pindex)) {
114+            return false;
115+        }
116+
117+        std::pair<uint256, DBVal> read_out;
118+        if (!m_db->Read(DBHeightKey(pindex->nHeight - 1), read_out)) {

Sjors commented at 2:40 pm on September 9, 2020:

Why doesn’t this fail for pindex->nHeight == 1 given that we don’t index block 0?

fjahr commented at 11:00 pm on September 14, 2020:

Hm, we do index block 0, we just don’t process the block data in the usual way but there is still an entry for it.

Sjors commented at 11:10 am on March 1, 2021:

How do we process the genesis block?

in src/index/coinstatsindex.cpp:168 in d28fed9b50 outdated

163+            }
164+        }
165+    }
166+
167+    CCoinsView* coins_view = WITH_LOCK(cs_main, return &ChainstateActive().CoinsDB());
168+    m_disk_size = coins_view->EstimateSize();

Sjors commented at 2:44 pm on September 9, 2020:

(Why) are we using the tip disk size here? The index now contains the disk size at the time the index was made.

fjahr commented at 11:00 pm on September 14, 2020:

You are right, that doesn’t make sense. I guess there is no reasonable way to get that number for every height so I removed it from the index.

Sjors commented at 2:54 pm on September 9, 2020: member

Concept ACK

An open question is how the UTXO set hash should be added after this is already merged. The hash could be added into the existing index which would need to resync or somehow be migrated.

With a bit of luck that happens before the next release, in which case developers using master can just delete the index and resync. Otherwise we could add a warning in the documentation that a future update may require a new sync (such as when adding MuHash, or some other stat). I expect only advanced users to need this.

Let’s make a separate PR for 2dbbbdd8a43f7b73e5dc3385545426021b9d5d84 Remove tracking of transaction count in UTXO set to maximise the opportunity for anyone who cares about this field to notice. It also needs a release note. From n=1 experience, ForkMonitor does not use this field.

21e42bb8143823c89021b757de0419ada2dda058 (Add verbose amounts tracking to Coinstats index) may be more suitable for a followup. The verbose argument is unnecessary imo; just show those fields when they’re available.

Partial code review done, will continue later (I’m also syncing the index).

fjahr commented at 11:03 pm on September 14, 2020: member

Thanks for all the feedback, reviewers! I pushed a big update which should address all of the feedback from the past couple of weeks. If I missed something, please let me know:

Now saving cumulative values in the index. The PRC still returns the per-block values by subtracting the values from the prev block.
Unspendables are tracked in the four separate categories.
Addressed feedback by @jonatack
Addressed feedback by @Sjors . Most importantly removed disk_size from the index and only returning it if the index is not used. This also led me to revert the change removing transactions because I could deal with it the exact same way and that meant keeping it in place now felt like the more sensible move.

fjahr force-pushed on Sep 14, 2020

in test/lint/lint-circular-dependencies.sh:17 in 1062318079 outdated

10@@ -11,6 +11,7 @@ export LC_ALL=C
11 EXPECTED_CIRCULAR_DEPENDENCIES=(
12     "chainparamsbase -> util/system -> chainparamsbase"
13     "index/txindex -> validation -> index/txindex"
14+    "index/coinstatsindex -> node/coinstats -> index/coinstatsindex"

promag commented at 11:26 pm on October 6, 2020:

Could move CCoinsStats and related to src/coinstats.h to avoid this.

fjahr commented at 2:17 pm on December 6, 2020:

Sorry for the late reply, I am not sure if I can follow this: you mean src/node/coinstats.h (file was moved not too long ago I think), right? But the struct CCoinsStats is already in there.

jonatack commented at 6:56 pm on March 26, 2021:

It would be nice indeed to avoid adding a circular dependency.

fjahr commented at 10:15 pm on April 6, 2021:

I agree but I don’t see how to achieve this with some refactor if it’s done right. It’s is on the top of my TODO list to resolve some circular dependencies but I think it’s out of the scope of this PR.

in src/rpc/blockchain.cpp:1116 in 1062318079 outdated

1022                         {RPCResult::Type::STR_HEX, "hash_serialized_2", "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"},
1023-                        {RPCResult::Type::NUM, "disk_size", "The estimated size of the chainstate on disk"},
1024-                        {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount"},
1025+                        {RPCResult::Type::NUM, "transactions", "The number of transactions with unspent outputs (not available when coinstatsindex is used)"},
1026+                        {RPCResult::Type::NUM, "disk_size", "The estimated size of the chainstate on disk (not available when coinstatsindex is used)"},
1027+                        {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount of coins in the UTXO set"},

Sjors commented at 10:04 am on October 9, 2020:

Maybe add: The genesis coinbase transaction is excluded from the UTXO set, so users understand the result of gettxoutsetinfo none 0 (total_amount: 0, total_unspendable_amount: 50, unspendable_amount: 50)

fjahr commented at 11:59 pm on November 25, 2020:

Done

in src/rpc/blockchain.cpp:1027 in 1062318079 outdated

1027+                        {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount of coins in the UTXO set"},
1028+                        {RPCResult::Type::STR_AMOUNT, "total_unspendable_amount", "The total amount of coins excluded from the UTXO set (only available if coinstatsindex is used)"},
1029+                        {RPCResult::Type::OBJ, "block_info", "Info on amounts in the block at this block height (only available if coinstatsindex is used)",
1030+                        {
1031+                            {RPCResult::Type::STR_AMOUNT, "unspendable_amount", ""},
1032+                            {RPCResult::Type::STR_AMOUNT, "total_prevout_spent_amount", ""},

Sjors commented at 10:06 am on October 9, 2020:

Suggest dropping total_ here and below, so it’s clear this is per block.

Alternatively, rename total_unspendable_amount to cumulative_unspendable_amount

I also don’t think the word “amount” is needed in the variable name. We already indicate count by using plural form, e.g. txouts. Finally I suggest putting total at the end, so variables with similar stuff are easier to spot, so:

0total
1total_unspendable
2block_info
3   prevout_spent
4   coinbase
5   new_outputs_ex_coinbase
6   unspendable
7   unspendables
8      genesis_block
9      ...

fjahr commented at 11:59 pm on November 25, 2020:

I dropped the totals and amounts in block_info, I think that made sense. I think we should keep total_amount as is to keep compatibility and I left total_unspendable_amount as is so it’s close to its pair on that level if that makes sense. I am not sure I understand the comment about putting total at the end. That was only in case I really want to keep the total somehow?

Sjors commented at 1:30 pm on December 9, 2020:

Not sure what I meant either :-) It’s fine now.

in src/rpc/client.cpp:130 in 1062318079 outdated

122@@ -123,6 +123,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
123     { "gettxout", 1, "n" },
124     { "gettxout", 2, "include_mempool" },
125     { "gettxoutproof", 0, "txids" },
126+    { "gettxoutsetinfo", 1, "hash_or_height" },

Sjors commented at 10:26 am on October 9, 2020:

For other reviewers, see #15448 and #19949 for solutions to avoid having to pass a hash with '"1abc...'"

jonatack commented at 7:47 pm on March 27, 2021:

590b887de0b5f6cb ISTM this line should be added in 6aedd7bb05 where the hash_or_height param is added

in src/rpc/blockchain.cpp:1023 in 1062318079 outdated

1023-                        {RPCResult::Type::NUM, "disk_size", "The estimated size of the chainstate on disk"},
1024-                        {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount"},
1025+                        {RPCResult::Type::NUM, "transactions", "The number of transactions with unspent outputs (not available when coinstatsindex is used)"},
1026+                        {RPCResult::Type::NUM, "disk_size", "The estimated size of the chainstate on disk (not available when coinstatsindex is used)"},
1027+                        {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount of coins in the UTXO set"},
1028+                        {RPCResult::Type::STR_AMOUNT, "total_unspendable_amount", "The total amount of coins excluded from the UTXO set (only available if coinstatsindex is used)"},

Sjors commented at 10:39 am on October 9, 2020:

“permanently excluded” makes it more clear this number does not include coinbase transactions.

fjahr commented at 0:00 am on November 26, 2020:

Done

in src/rpc/blockchain.cpp:1083 in 1062318079 outdated

1012                 },
1013                 RPCResult{
1014                     RPCResult::Type::OBJ, "", "",
1015                     {
1016                         {RPCResult::Type::NUM, "height", "The current block height (index)"},
1017                         {RPCResult::Type::STR_HEX, "bestblock", "The hash of the block at the tip of the chain"},

Sjors commented at 10:41 am on October 9, 2020:

The current block height -> The (current) block height
bestblock -> should probably be dropped if a hash argument is used, otherwise it’s confusing

fjahr commented at 0:02 am on November 26, 2020:

I think the bestblock help text was confusing as it was outdated so I changed it. Otherwise, I think it’s ok to repeat the hash that was passed to keep consistency. Made the other change.

in src/rpc/blockchain.cpp:1033 in 1062318079 outdated

1033+                            {RPCResult::Type::STR_AMOUNT, "total_new_outputs_ex_coinbase_amount", ""},
1034+                            {RPCResult::Type::STR_AMOUNT, "coinbase_amount", ""},
1035+                            {RPCResult::Type::OBJ, "unspendables", "Detailed view of the unspendable categories",
1036+                            {
1037+                                {RPCResult::Type::STR_AMOUNT, "genesis_block", ""},
1038+                                {RPCResult::Type::STR_AMOUNT, "bip30", ""},

Sjors commented at 10:45 am on October 9, 2020:

It’s not due to BIP30 that these coins are lost. BIP30 prevents it from ever happening again, so doc could e.g. say: “Transactions overridden by duplicates (no longer possible with BIP30)”

fjahr commented at 0:02 am on November 26, 2020:

Done

Sjors commented at 11:12 am on October 9, 2020: member

I like the separate categories for unspendables.

Building the index for mainnet, configured with --enable-debug, took about 3.5 hours.

I prefer flipping the hash_or_height and hash_type arguments.

When the index is being synced and you query for a recent block, the error could be less obscure: Unable to read UTXO set

Interesting blocks for other reviewers:

0
91722 (pre-BIP30 coinbase, made unspendable by a duplicate in a later block).
124724 unclaimed reward (miner left 1 satoshi subsidiy on the table, as well as 0.01 BTC in fees)
308570 OP_RETURN that didn’t burn coins
473281 OP_RETURN with 0.00000001 burn
645179 main chain block and (if your node has it) stale block 00000000000000000004c1baa2412fd31eb75bbe79def4f66ef97e0e15a20668 (throws Block is not in chain main; I thought we also indexed blocks outside the main chain?)
650000 nice round number to compare: txouts: 13304079, bogosize: 991494955, total_amount: 1601649.79154586, total_unspendable_amount: 6.45845414 (that last one looks wrong; does this reset when you restart the node during indexing?)

DrahtBot added the label Needs rebase on Oct 15, 2020

fjahr force-pushed on Nov 25, 2020

fjahr commented at 0:13 am on November 26, 2020: member

@Sjors Thanks for the review and sorry for the long wait. I improved the error message as well, as you suggested. But I am a bit hesitant about flipping the arguments because the hash_type is relevant to both users running the index and those that don’t run the index and hash_or_height is only for those that run the index. But maybe there is an elegant solution to that that I am not seeing :)

DrahtBot removed the label Needs rebase on Nov 26, 2020

in src/index/coinstatsindex.cpp:124 in 2f6f9a7ad1 outdated

112+            return false;
113+        }
114+
115+        uint256 expected_block_hash = pindex->pprev->GetBlockHash();
116+        if (read_out.first != expected_block_hash) {
117+            return error("%s: previous block header belongs to unexpected block %s; expected %s",

Sjors commented at 1:56 pm on December 9, 2020:

2f6f9a7ad11209744fb0fbf797a1a87d9127dd5e: I’m puzzled why we never hit this, maybe because I’ve only testsed on a node that saw very few reorgs.

If you have two blocks at height N, and you’re processing a block at N+1, you might not always get the right block at height N if you use DBHeightKey. IIUC this could be solved with an else branch that tries DBHashKey.

Might be good to have to test for that.

in src/init.cpp:1566 in 5479000c79 outdated

1562@@ -1552,6 +1563,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1563         filter_index_cache = max_cache / n_indexes;
1564         nTotalCache -= filter_index_cache * n_indexes;
1565     }
1566+    int64_t coin_stats_cache = 0;

Sjors commented at 2:27 pm on December 9, 2020:

5479000c79b254120b7535bf322869439ee33215 nit: coin_stats_cache_size

fjahr commented at 0:54 am on February 22, 2021:

Done

in src/node/coinstats.cpp:81 in 5479000c79 outdated

78+
79+    const CBlockIndex* block_index;
80     {
81         LOCK(cs_main);
82-        stats.nHeight = LookupBlockIndex(stats.hashBlock)->nHeight;
83+        block_index = LookupBlockIndex(pcursor->GetBestBlock());

Sjors commented at 2:30 pm on December 9, 2020:

5479000c79b254120b7535bf322869439ee33215: consider splitting the usage of the index away from enabling the index

fjahr commented at 0:04 am on March 1, 2021:

I added an option (no_index) for it in a new commit.

in src/node/coinstats.cpp:31 in 5479000c79 outdated

25@@ -25,6 +26,18 @@ uint64_t GetBogoSize(const CScript& scriptPubKey)
26            scriptPubKey.size() /* scriptPubKey */;
27 }
28 
29+static bool CanUseIndex(CHashWriter& ss)
30+{
31+    return false;

Sjors commented at 2:36 pm on December 9, 2020:

5479000: this seems overly abstract, maybe just check the hash type in GetUTXOStats? Unless it makes more sense once MuHash is added.

fjahr commented at 0:54 am on February 22, 2021:

Done

in src/index/coinstatsindex.cpp:152 in fc621a8fbd outdated

156@@ -135,7 +157,17 @@ bool CoinStatsIndex::WriteBlock(const CBlock& block, const CBlockIndex* pindex)
157                 Coin coin = Coin(out, pindex->nHeight, tx->IsCoinBase());
158 
159                 // Skip unspendable coins
160-                if (coin.out.scriptPubKey.IsUnspendable()) continue;
161+                if (coin.out.scriptPubKey.IsUnspendable()) {

Sjors commented at 2:43 pm on December 9, 2020:

fc621a8fbdb5ecad1866e754ad38c24bfedfe5c1 This can in theory also happen for other (future) reasons than OP_RETURN:

0    /**
1     * Returns whether the script is guaranteed to fail at execution,
2     * regardless of the initial stack. This allows outputs to be pruned
3     * instantly when entering the UTXO set.
4     */
5    bool IsUnspendable() const
6    {
7        return (size() > 0 && *begin() == OP_RETURN) || (size() > MAX_SCRIPT_SIZE);
8    }

fjahr commented at 0:58 am on February 22, 2021:

Yepp, to be honest I just didn’t come up with a better name in the moment and didn’t revisit it afterwards. Will give this some more thought of what a good name is. Maybe unspendable_script.

fjahr commented at 0:04 am on March 1, 2021:

Renamed

Sjors commented at 2:48 pm on December 9, 2020: member

355b91d454195e10289e34bdde755442d43bc30e looks good, but there’s clearly a bug when you restart the node during the indexing process. E.g. txouts and bogosize seem to get reset and can even turn negative.

I left a few additional questions / issues.

I’m syncing it again and still need to test if bogohash and other stats match with or without index for most recent block.

Sjors commented at 2:14 pm on January 23, 2021: member

Would love to see some progress here…

fjahr commented at 10:20 pm on January 25, 2021: member

Would love to see some progress here…

Yes, thanks for the nudge. However, since #19055 is merged now and reviewing power seems to be particularly scarce at the moment, I think it makes more sense to focus on getting #19145 in (which is comparatively small and easy-ish to review) and then reopen this here as a follow-up with the UTXO set hash included again also taking in your feedback @Sjors . I think this will be more intuitive for reviewers than keeping this and then adding the hash as a follow-up, the current approach made sense because it was unclear how long #19055 would take. I am working on getting that ready and will put this PR here in draft mode until it’s ready, to avoid confusion.

fjahr marked this as a draft on Jan 25, 2021

in src/rpc/blockchain.cpp:2556 in 355b91d454 outdated

2552@@ -2482,7 +2553,7 @@ static const CRPCCommand commands[] =
2553     { "blockchain",         "getmempoolinfo",         &getmempoolinfo,         {} },
2554     { "blockchain",         "getrawmempool",          &getrawmempool,          {"verbose", "mempool_sequence"} },
2555     { "blockchain",         "gettxout",               &gettxout,               {"txid","n","include_mempool"} },
2556-    { "blockchain",         "gettxoutsetinfo",        &gettxoutsetinfo,        {"hash_type"} },
2557+    { "blockchain",         "gettxoutsetinfo",        &gettxoutsetinfo,        {"hash_type", "hash_or_height"} },

MarcoFalke commented at 6:42 pm on January 28, 2021:

can remove this diff, and then rebase

Sjors commented at 1:03 pm on February 19, 2021: member

#19055 is merged, rebase time?

fjahr force-pushed on Feb 21, 2021

fjahr force-pushed on Feb 22, 2021

fjahr commented at 0:53 am on February 22, 2021: member

#19055 is merged, rebase time?

Done, and I have made several changes based on feedback. But there are still a few comments I haven’t addressed yet or still need to test.

fjahr marked this as ready for review on Feb 22, 2021

fjahr force-pushed on Feb 22, 2021

fjahr force-pushed on Feb 23, 2021

in src/index/coinstatsindex.cpp:56 in 847e8ba7e6 outdated

51+};
52+
53+struct DBHeightKey {
54+    int height;
55+
56+    DBHeightKey() : height(0) {}

Sjors commented at 1:22 pm on February 23, 2021:

clang is unhappy: unused member function 'DBHeightKey' [-Wunused-member-function]

fjahr commented at 4:14 pm on February 28, 2021:

fixed

Sjors commented at 1:30 pm on February 23, 2021: member

When I shutdown the node (gracefully) while it’s building the index, and then start it again, it fails with ERROR: Init: Cannot read current coinstatsindex state; index may be corrupted. That’s a new bug.

fjahr force-pushed on Feb 25, 2021

fjahr renamed this:
~~Coinstats Index (without UTXO set hash)~~
Coinstats Index
on Feb 25, 2021

laanwj added this to the "Blockers" column in a project

fjahr force-pushed on Mar 1, 2021

fjahr commented at 0:08 am on March 1, 2021: member

Thanks for your comments @Sjors ! I think I have addressed all except for the reorg test but I am working on that one.

in src/validation.cpp:5440 in 917e29a776 outdated

5436@@ -5437,7 +5437,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
5437     // about the snapshot_chainstate.
5438     CCoinsViewDB* snapshot_coinsdb = WITH_LOCK(::cs_main, return &snapshot_chainstate.CoinsDB());
5439 
5440-    if (!GetUTXOStats(snapshot_coinsdb, stats, CoinStatsHashType::HASH_SERIALIZED, breakpoint_fnc)) {
5441+    if (!GetUTXOStats(snapshot_coinsdb, stats, breakpoint_fnc)) {

Sjors commented at 10:16 am on March 1, 2021:

Because the snapshot uses CoinStatsHashType::HASH_SERIALIZED it might be better to initialise stats with that option explicitly, in case we ever change the default.

fjahr commented at 0:44 am on March 4, 2021:

Yepp, makes sense. Done.

in src/index/coinstatsindex.cpp:108 in 1fc3470c1b outdated

103+        if (read_out.first != expected_block_hash) {
104+            return error("%s: previous block header belongs to unexpected block %s; expected %s",
105+                         __func__, read_out.first.ToString(), expected_block_hash.ToString());
106+        }
107+
108+        // TODO

Sjors commented at 10:41 am on March 1, 2021:

TODO what?

fjahr commented at 0:17 am on March 2, 2021:

Sorry, I confused myself quite a bit because I thought deduplicating bip30 block stuff was part of a Coinstats PR discussion and I had made a commit in a local branch but after searching for it for a while I realized now that it wasn’t there, it was actually in #19888. So, this was actually a “I think I had something that I might want to use here” reminder for this: https://github.com/bitcoin/bitcoin/pull/19888/commits/b8797b064099aafadc0bbc9f152a962f76dd31e3. But I am not sure if it’s worth it to throw another consensus critical code refactoring commit in here.

Sjors commented at 7:51 am on March 2, 2021:

Definitely not. You could just make a separate PR for it later, and maybe write a comment to point out the duplication.

in src/index/coinstatsindex.cpp:168 in 1fc3470c1b outdated

128+                // Skip unspendable coins
129+                if (coin.out.scriptPubKey.IsUnspendable()) continue;
130+
131+                m_transaction_output_count++;
132+                m_total_amount += coin.out.nValue;
133+                m_bogo_size += GetBogoSize(coin.out.scriptPubKey);

Sjors commented at 10:56 am on March 1, 2021:

Are you sure bogo_size shouldn’t be counted for OP_RETURN? I guess not, because you’re testing that the result is consistent with a non-indexed node: https://github.com/bitcoin/bitcoin/pull/19521/commits/9c07684e444a5c8e1f81d19221c6217dada3da72#diff-a6434325d09a6df4b371513c837907dfc1a97cf540709def4bded9b2a17e3f49R61

fjahr commented at 10:32 pm on March 3, 2021:

Yeah, to be consistent we should only count stuff that actually ends up in the UTXO set.

in src/index/coinstatsindex.cpp:357 in 1fc3470c1b outdated

292+
293+            m_transaction_output_count = entry.transaction_output_count;
294+            m_bogo_size = entry.bogo_size;
295+            m_total_amount = entry.total_amount;
296+            m_total_subsidy = entry.total_subsidy;
297+            m_block_unspendable_amount = entry.block_unspendable_amount;

Sjors commented at 11:02 am on March 1, 2021:

1fc3470c1b03857245b94eebde5cd0f18805a884: I think these ended up in the wrong commit

fjahr commented at 0:44 am on March 4, 2021:

Yepp, fixed.

in src/index/coinstatsindex.cpp:417 in 1fc3470c1b outdated

344+            Coin coin = Coin(out, pindex->nHeight, tx->IsCoinBase());
345+
346+            // Skip unspendable coins
347+            if (coin.out.scriptPubKey.IsUnspendable()) continue;
348+
349+            m_muhash.Remove(MakeUCharSpan(TxOutSer(outpoint, coin)));

Sjors commented at 11:06 am on March 1, 2021:

1fc3470c1b03857245b94eebde5cd0f18805a884 if you’re checking MuHash for consistency, why not also check the other variables?

fjahr commented at 0:44 am on March 4, 2021:

Done

in test/functional/feature_coinstatsindex.py:95 in 9c07684e44 outdated

59+            res1.pop('muhash', None)
60+
61+            # Everything left should be the same
62+            assert_equal(res1, res0)
63+
64+        self.log.info("Test that gettxoutsetinfo() can get fetch data on specific heights with index")

Sjors commented at 11:17 am on March 1, 2021:

Would be nice to add a test here that adds a block and then rolls it back, to see if the result is untouched.

fjahr commented at 0:44 am on March 4, 2021:

I added the test, please let me know if you can think of other scenarios that I might have missed.

in src/rpc/client.cpp:132 in c3488a426f outdated

126@@ -127,6 +127,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
127     { "gettxout", 1, "n" },
128     { "gettxout", 2, "include_mempool" },
129     { "gettxoutproof", 0, "txids" },
130+    { "gettxoutsetinfo", 1, "hash_or_height" },
131     { "lockunspent", 0, "unlock" },

adamjonas commented at 3:49 pm on March 1, 2021:

rpc_help.py looking for { "gettxoutsetinfo", 2, "no_index"} here:

0File "/bitcoin/test/functional/rpc_help.py", line 51, in run_test
1    self.test_client_conversion_table()
2File "/bitcoin/test/functional/rpc_help.py", line 69, in test_client_conversion_table
3    raise AssertionError("RPC client conversion table ({}) and RPC server named arguments mismatch!\n{}".format(
4AssertionError: RPC client conversion table (/bitcoin/src/rpc/client.cpp) and RPC server named arguments mismatch!
5{('gettxoutsetinfo', 2, 'no_index')}

fjahr commented at 0:28 am on March 2, 2021:

Hm, strange I don’t see this fail when running rpc_help.py locally. Should be fixed now.

Sjors changes_requested

Sjors commented at 4:15 pm on March 1, 2021: member

c3488a426f7ef71e45143e4bd797d06332bd5287 is getting very close

rpc_help.py complains:

0RPC server named arguments mismatch!
1{('gettxoutsetinfo', 2, 'no_index')}

Probably because you need to add { "gettxoutsetinfo", 2, "no_index" } to client.cpp.

The error I found last week is gone now.

a bug when you restart the node during the indexing process. E.g. txouts and bogosize seem to get reset and can even turn negative.

This has also been fixed now.

The “To be discussed” section in the PR description can be dropped now.

MuHash at height 670,000: b1567b5e632bd67e72e056711fd644945ed9abcae5c28bf4750e4a10c567112a hash_serialized_2 at height 672702: 0aafad4ae3a9e8b9c9f97a6b9e16e19e0c24bfc5543470436f365e0b1edec222, but with no_index and on master it’s b5177c4bd0df0195c8e8877c33f973b740bf148301c8662eca94cf0b3385f210. bogo_size at height 672702: 5466711713 (matches no_index and master)

When I did src/bitcoin-cli gettxoutsetinfo muhash 670000 true, when my tip was further, it returned a muhash that appears to be nonsense. It should error instead.

Other reviewers, see also my list of interesting blocks above. When testing the tip, it helps to call setnetworkactive false so it doesn’t move between calls.

I’d still like to know what happens when a valid-fork chaintip has two or more blocks. Unfortunately I don’t have any on my node: bitcoin-cli getchaintips | jq '.[] | select(.status == "valid-fork")'. My node did see a 2 block fork at height 656,478, but it only has the headers. I could use #20295 to fetch the block if it’s still out there. In order for the indexer to pick it up, I guess it has to be validated? So that would require a long rollback. So anyway, a test would be easier.

Regarding the error I saw with 645179 when asking for the hash of a stale block, this makes sense now: you only create a block hash based entry during a reorg, but it’s not done for all the stale blocks the node already has. This is probably a general issue with the indexes system, and not worth addressing here imo.

Bonus tip for other reviewers, if you want to drink tea while the index builds, try @dongcarl’s yo: while (src/bitcoin-cli getindexinfo | jq '.coinstatsindex.synced' | grep "false"); do sleep 60; done; yo

fjahr force-pushed on Mar 2, 2021

in src/rpc/blockchain.cpp:1080 in 5491fd3886 outdated

1072@@ -1043,42 +1073,80 @@ static RPCHelpMan gettxoutsetinfo()
1073 {
1074     return RPCHelpMan{"gettxoutsetinfo",
1075                 "\nReturns statistics about the unspent transaction output set.\n"
1076-                "Note this call may take some time.\n",
1077+                "Note this call may take some time if you are not using coinstatsindex.\n",
1078                 {
1079                     {"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
1080+                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex)", "", {"", "string or numeric"}},
1081+                    {"no_index", RPCArg::Type::BOOL, /* default */ "false", "Don't use Coinstatsindex even when it is available."},

MarcoFalke commented at 9:45 am on March 2, 2021:

Instead of double-negation this could be named use_index (default: true)?

fjahr commented at 0:39 am on March 4, 2021:

Done

in test/functional/feature_utxo_set_hash.py:30 in 5491fd3886 outdated

31-        assert_equal(self.nodes[0].gettxoutsetinfo()['hash_serialized_2'], "b32ec1dda5a53cd025b95387aad344a801825fe46a60ff952ce26528f01d3be8")
32-        assert_equal(self.nodes[0].gettxoutsetinfo("muhash")['muhash'], "dd5ad2a105c2d29495f577245c357409002329b9f4d6182c0af3dc2f462555c8")
33+        node = self.nodes[0]
34+        mocktime = node.getblockheader(node.getblockhash(0))['time'] + 1
35+        node.setmocktime(mocktime)
36+        blocks = node.generate(10)

MarcoFalke commented at 9:47 am on March 2, 2021:

Would be nice to use a fixed address, instead of relying on the hardcoded one to not change. Also, could this be split up into a separate test-only pull?

fjahr commented at 0:42 am on March 4, 2021:

Yeah, I also still want to switch the test to use MiniWallet, that improvement is coming up. I could make a test only pull but personally, I find it hard to review a PR without some tests alongside it. But overall this has still grown quite a bit and there are probably still plenty of tests to be written, so I will think about where to make a good cut.

MarcoFalke commented at 1:16 pm on March 7, 2021:

Splitting up the tests makes it easier to check for accidental regressions (https://github.com/bitcoin/bitcoin/pull/19145#discussion_r575334728, #19145#pullrequestreview-579351730, …). If the tests are changed along with changes that should be refactor-only or new tests added for a different feature than the one being introduced, it may be easier to miss bugs.

Obviously a good first step is to touch the tests in separate commits, whenever possible, so the only burden to reviewers is to run the tests on each commit.

fjahr commented at 8:42 pm on March 8, 2021:

I think I got you now. Initially, I misunderstood that I should move all the tests in this PR into separate PRs but I think you were only referring to the #19145 follow-ups, correct? I have moved these into #21390 and use one of the deterministic privkeys there now.

jonatack commented at 10:38 pm on March 3, 2021: member

Thanks for your persistance on this @fjahr. Will review soon.

fjahr force-pushed on Mar 4, 2021

fjahr commented at 0:56 am on March 4, 2021: member

Addressed several comments by @Sjors and @MarcoFalke . Thanks for reviewing 🙏

How do we process the genesis block?

(for some reason GH doesn’t let me comment on this) The Genesis block is explicitly skipped in WriteBlock.

c3488a4 is getting very close

rpc_help.py complains:
0RPC server named arguments mismatch!
1{('gettxoutsetinfo', 2, 'no_index')}
Probably because you need to add { "gettxoutsetinfo", 2, "no_index" } to client.cpp.

Yepp, fixed.

The error I found last week is gone now.

a bug when you restart the node during the indexing process. E.g. txouts and bogosize seem to get reset and can even turn negative.

This has also been fixed now.

The “To be discussed” section in the PR description can be dropped now.

MuHash at height 670,000: b1567b5e632bd67e72e056711fd644945ed9abcae5c28bf4750e4a10c567112a hash_serialized_2 at height 672702: 0aafad4ae3a9e8b9c9f97a6b9e16e19e0c24bfc5543470436f365e0b1edec222, but with no_index and on master it’s b5177c4bd0df0195c8e8877c33f973b740bf148301c8662eca94cf0b3385f210. bogo_size at height 672702: 5466711713 (matches no_index and master)

I had inserted the MuHash insert line above the line that skips unspendable outputs, so they were added to the hash, that’s why they must have been inconsistent. I haven’t yet synced with the new code, will test tomorrow.

I think the hash_serialized call with a specific height shouldn’t work since it’s incompatible with the index, so I added that.

When I did src/bitcoin-cli gettxoutsetinfo muhash 670000 true, when my tip was further, it returned a muhash that appears to be nonsense. It should error instead.

Other reviewers, see also my list of interesting blocks above. When testing the tip, it helps to call setnetworkactive false so it doesn’t move between calls.

I’d still like to know what happens when a valid-fork chaintip has two or more blocks. Unfortunately I don’t have any on my node: bitcoin-cli getchaintips | jq '.[] | select(.status == "valid-fork")'. My node did see a 2 block fork at height 656,478, but it only has the headers. I could use #20295 to fetch the block if it’s still out there. In order for the indexer to pick it up, I guess it has to be validated? So that would require a long rollback. So anyway, a test would be easier.

Regarding the error I saw with 645179 when asking for the hash of a stale block, this makes sense now: you only create a block hash based entry during a reorg, but it’s not done for all the stale blocks the node already has. This is probably a general issue with the indexes system, and not worth addressing here imo.

Bonus tip for other reviewers, if you want to drink tea while the index builds, try @dongcarl’s yo: while (src/bitcoin-cli getindexinfo | jq '.coinstatsindex.synced' | grep "false"); do sleep 60; done; yo

Sjors commented at 12:18 pm on March 4, 2021: member

How do we process the genesis block?

The Genesis block is explicitly skipped in WriteBlock.

But then why isn’t the result of src/bitcoin-cli gettxoutsetinfo muhash 0 all zeros? Yet it contains a value for muhash and unspendables->genesis_block is 50. Where does that get set?

fjahr commented at 7:37 pm on March 4, 2021: member

How do we process the genesis block?

The Genesis block is explicitly skipped in WriteBlock.

But then why isn’t the result of src/bitcoin-cli gettxoutsetinfo muhash 0 all zeros? Yet it contains a value for muhash and unspendables->genesis_block is 50. Where does that get set?

We skip almost all the processing of the genesis block in WriteBlock() from this line: https://github.com/bitcoin/bitcoin/pull/19521/files#diff-f1b8261031c7f062025f97106824d80601e5247ba0b68ffe608285eeaecceed2R112

0if (pindex->nHeight > 0) {

But there is also an else block belonging to that if ~80 lines later, that’s where these values are set: https://github.com/bitcoin/bitcoin/pull/19521/files#diff-f1b8261031c7f062025f97106824d80601e5247ba0b68ffe608285eeaecceed2R188

0    } else {
1        m_block_unspendable_amount += block_subsidy;
2        m_unspendables_genesis_block += block_subsidy;
3    }

fjahr commented at 11:34 pm on March 6, 2021: member

But then why isn’t the result of src/bitcoin-cli gettxoutsetinfo muhash 0 all zeros?

I realized I didn’t answer that one yet. The internal state of the MuHash3072 object is all zeros but that state gets finalized by hashing it with SHA256 so the uint256 output in the gettxoutsetinfo call is not all zeros anymore.

in src/index/coinstatsindex.cpp:110 in d608c40041 outdated

105+                return error("%s: previous block header belongs to unexpected block %s; expected %s",
106+                             __func__, read_out.first.ToString(), expected_block_hash.ToString());
107+            }
108+        }
109+
110+        // TODOi: Deduplicate BIP30 related code

Sjors commented at 6:40 pm on March 7, 2021:

d608c400411f1451d6298efc1bf6f200a56aa136: no need to brag that you use vim

fjahr commented at 8:34 pm on March 8, 2021:

You caught me ;) Fixed.

in test/functional/feature_coinstatsindex.py:267 in 29ac833e8f outdated

262+        blocks = index_node.generate(2)
263+        self.wait_until(lambda: not try_rpc(-32603, "Unable to read UTXO set", index_node.gettxoutsetinfo, 'muhash'))
264+
265+        # Test that the result of the reorged block is not returned for its old block height
266+        res = index_node.gettxoutsetinfo('muhash', 111)
267+        assert_equal(res["bestblock"], blocks[0])

Sjors commented at 6:59 pm on March 7, 2021:

stats.hashBlock = pindex->GetBlockHash(); so this check doesn’t really do much. I believe what you want to (also) check here is that muhash for height 111 matches muhash for blocks[0].

For context, the bit I’m worried about is that CoinStatsIndex::WriteBlock first attempts to fetch by height, only then falls back to fetching by hash.

So in the case of a reorg, I think it will ignore the hash and give you the result form the active tip branch. But the reorg I had in mind would come from a temporarily disconnected node (with some unique transaction so it’s different).

In other words, try querying a reorged block by hash and see if it returns the right muhash, rather than the main chain one.

0        if (!m_db->Read(DBHeightKey(pindex->nHeight - 1), read_out)) {
1            return false;
2        }
3
4        uint256 expected_block_hash = pindex->pprev->GetBlockHash();
5        if (read_out.first != expected_block_hash) {
6            if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {

Another thing you can test is what happens when you activate the index after a reorg (but I think fixing issues there can wait).

fjahr commented at 10:11 pm on March 8, 2021:

Interesting. Thanks for the clearer description @Sjors. The reorg case that you describe here (querying a reorged out block by hash) would return an error until now because ParseHashOrHeight() is checking that the queried block is part of the active chain. But I think it could be interesting to have that functionality so I removed this check and added the test. It works fine. However, I am still a bit unsure if I should integrate this here or pull it out into a separate PR because ParseHashOrHeight() is also used by getblockstats so it leads to a behavior change there too. Tests are passing though and I tend to think it’s ok. But I have left it as a separate WIP commit for now for easier inspection.

Another thing you can test is what happens when you activate the index after a reorg (but I think fixing issues there can wait).

Not sure I fully understand this one. Do you mean just activating the index for the first time on a node that is aware of some stale blocks?

Sjors commented at 9:06 am on March 9, 2021:

But I think it could be interesting to have that functionality so I removed this check and added the test. It works fine.

Nice!

Do you mean just activating the index for the first time on a node that is aware of some stale blocks?

Correct. Do those stale blocks get indexed?

Sjors commented at 9:45 am on March 9, 2021:

I expanded the test to do a deeper reorg: https://github.com/Sjors/bitcoin/commit/24c37b2dc446e944176093f1cd47f32686cdd1aa

fjahr commented at 11:50 pm on March 10, 2021:

Cool, I pulled in your changes and made you co-author. The stale blocks don’t get indexed because the base index only follows the active chain when syncing. I have added some additional coverage for that in the reorg test part, mainly to document that behavior. I have pulled the change that allows querying stale blocks in its own commit so it’s more clear that this also touches getblockstats and that that gets the attention such a side effect deserves.

Sjors commented at 7:06 pm on March 7, 2021: member

tACK e2a554de71644ffeabd919ef25de522289d08196

MuHash at height 670,000: cdf7ed6a6b3d4ec8d8635811c44510a3903fbbc6d46bf6e549fca6cd80ccb66e muhash at height 673,681: 372d849e6a5734badcf022fbba337dffa648d2ee352127d51968f4366998ef99 (matches use_index=false and master) hash_serialized_2 at height 673,681: cb3bc45aa4553157768fdd968f1686069448720ab8cbeb0c75a3d4efa293db69 (matches master) bogo_size at height 673,681: 5487111539 (matches use_index=false and master)

fjahr force-pushed on Mar 8, 2021

Sjors commented at 9:52 am on March 9, 2021: member

I think you lost 6b21ff9bf7 in the rebase? I think ffeb0af71f and 98ec2e5170 were moved to #21390.

fjahr commented at 11:02 pm on March 10, 2021: member

I think you lost 6b21ff9 in the rebase? I think ffeb0af and 98ec2e5 were moved to #21390.

Sorry, yes, I moved all three of them to #21390 but only commented on it here #19521 (review) . The test changes were follow-ups to #19145 and the doc improvements could also stand alone after I made some light edits so I followed @MarcoFalke ’s recommendation there.

fjahr force-pushed on Mar 10, 2021

Sjors commented at 10:12 am on March 11, 2021: member

re-utACK fc3f9c0a0debe040971098c3edb1078c8679c54c

DrahtBot added the label Needs rebase on Mar 11, 2021

fjahr force-pushed on Mar 11, 2021

fjahr commented at 11:22 pm on March 11, 2021: member

rebased

in src/init.cpp:1825 in 28d2bc80d6 outdated

1820@@ -1809,6 +1821,11 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
1821         GetBlockFilterIndex(filter_type)->Start();
1822     }
1823 
1824+    if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
1825+        g_coin_stats_index = MakeUnique<CoinStatsIndex>(coin_stats_cache_size, false, fReindex);

fanquake commented at 0:45 am on March 12, 2021:

Please use std::make_unique in new code.

fjahr commented at 11:53 pm on March 12, 2021:

Done

DrahtBot removed the label Needs rebase on Mar 12, 2021

fjahr force-pushed on Mar 12, 2021

fjahr commented at 11:54 pm on March 12, 2021: member

Addressed @fanquake s comment and used std::make_unique.

in src/index/coinstatsindex.cpp:102 in 7e1382942f outdated

 97+CoinStatsIndex::CoinStatsIndex(size_t n_cache_size, bool f_memory, bool f_wipe)
 98+{
 99+    fs::path path = GetDataDir() / "indexes" / "coinstats";
100+    fs::create_directories(path);
101+
102+    m_db = MakeUnique<CoinStatsIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);

jonatack commented at 11:38 am on March 15, 2021:

I’m seeing build errors here (debian gcc 10.2.1)

0index/coinstatsindex.cpp: In constructor ‘CoinStatsIndex::CoinStatsIndex(size_t, bool, bool)’:
1index/coinstatsindex.cpp:102:12: error: ‘MakeUnique’ was not declared in this scope
2  102 |     m_db = MakeUnique<CoinStatsIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);
3      |            ^~~~~~~~~~

and with clang 11

0index/coinstatsindex.cpp:102:12: error: use of undeclared identifier 'MakeUnique'
1    m_db = MakeUnique<CoinStatsIndex::DB>(path / "db", n_cache_size, f_memory, f_wipe);
2           ^
31 error generated.

maybe need to use std::make_unique

jonatack commented at 12:26 pm on March 15, 2021:

Update: build, unit tests and test/functional/feature_coinstatsindex.py are green for me locally after replacing with std::make_unique.

fjahr commented at 9:33 pm on March 15, 2021:

Ugh, indeed, sloppily overlooked during the last update. Thanks @jonatack ! Fixed.

fjahr force-pushed on Mar 15, 2021

Sjors commented at 9:56 am on March 16, 2021: member

re-ACK 9e746e8 MuHash at height 674,863: eca35832ffdac43638e849e818ae10db7d2ea53746c406e386e8139e0583a990 (matches use_index=false)

jonatack commented at 5:30 pm on March 16, 2021: member

Thanks for re-pushing @fjahr. Reviewing.

jonatack commented at 6:15 pm on March 16, 2021: member

When building bf347106cd “index: Coinstats index can be activated with command line flag”

0rpc/blockchain.cpp: In lambda function:
1rpc/blockchain.cpp:1092:13: error: ‘g_coin_stats_index’ was not declared in this scope
2 1092 |         if (g_coin_stats_index && (g_coin_stats_index->GetSummary().synced == false)) {
3      |             ^~~~~~~~~~~~~~~~~~

fjahr force-pushed on Mar 16, 2021

fjahr commented at 11:23 pm on March 16, 2021: member

When building bf34710 “index: Coinstats index can be activated with command line flag”

0rpc/blockchain.cpp: In lambda function:
1rpc/blockchain.cpp:1092:13: error: ‘g_coin_stats_index’ was not declared in this scope
2 1092 |         if (g_coin_stats_index && (g_coin_stats_index->GetSummary().synced == false)) {
3      |             ^~~~~~~~~~~~~~~~~~

Ah, that include was a bit late to the party, fixed.

jonatack commented at 1:15 am on March 17, 2021: member

Thanks @fjahr! resolved in the latest push; moving forward, test/functional/feature_coinstatsindex.py fails at commit 540428a2c7

 0((HEAD detached at 540428a2c7))$ src/test/test_bitcoin -t coinstatsindex_tests ; test/functional/feature_coinstatsindex.py
 1Running 1 test case...
 2
 3*** No errors detected
 4
 52021-03-17T01:07:46.879000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_inxlkokp
 62021-03-17T01:07:56.135000Z TestFramework (INFO): Test that gettxoutsetinfo() output is consistent with or without coinstatsindex option
 72021-03-17T01:07:56.744000Z TestFramework (ERROR): Assertion failed
 8Traceback (most recent call last):
 9  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
10    self.run_test()
11  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_coinstatsindex.py", line 33, in run_test
12    self._test_coin_stats_index()
13  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_coinstatsindex.py", line 62, in _test_coin_stats_index
14    assert_equal(res1, res0)
15  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 50, in assert_equal
16    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
17AssertionError: not({'height': 102, 'bestblock': '7471357854133661aaa6d50eba11ec46c3322e299c5b6907885351551005f771', 'txouts': 103, 'bogosize': 7722, 'total_amount': Decimal('5100.00000000'), 'total_unspendable_amount': Decimal('50.00000000'), 'block_info': {'prevout_spent': Decimal('50.00000000'), 'coinbase': Decimal('50.00004440'), 'new_outputs_ex_coinbase': Decimal('49.99995560'), 'unspendable': Decimal('0E-8'), 'unspendables': {'genesis_block': Decimal('0E-8'), 'bip30': Decimal('0E-8'), 'scripts': Decimal('0E-8'), 'unclaimed_rewards': Decimal('0E-8')}}} == {'height': 102, 'bestblock': '7471357854133661aaa6d50eba11ec46c3322e299c5b6907885351551005f771', 'txouts': 103, 'bogosize': 7722, 'total_amount': Decimal('5100.00000000')})
182021-03-17T01:07:56.797000Z TestFramework (INFO): Stopping nodes
192021-03-17T01:07:57.000000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_inxlkokp
202021-03-17T01:07:57.000000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_inxlkokp/test_framework.log
212021-03-17T01:07:57.001000Z TestFramework (ERROR): 
222021-03-17T01:07:57.001000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_inxlkokp' to consolidate all logs
232021-03-17T01:07:57.001000Z TestFramework (ERROR): 
242021-03-17T01:07:57.001000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
252021-03-17T01:07:57.001000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
262021-03-17T01:07:57.001000Z TestFramework (ERROR): 
27
28
29((HEAD detached at 540428a2c7)) $ test/functional/feature_coinstatsindex.py
302021-03-17T01:08:09.775000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_rxs4xbm6
312021-03-17T01:08:20.052000Z TestFramework (INFO): Test that gettxoutsetinfo() output is consistent with or without coinstatsindex option
322021-03-17T01:08:20.215000Z TestFramework (ERROR): Assertion failed
33Traceback (most recent call last):
34  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
35    self.run_test()
36  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_coinstatsindex.py", line 33, in run_test
37    self._test_coin_stats_index()
38  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_coinstatsindex.py", line 62, in _test_coin_stats_index
39    assert_equal(res1, res0)
40  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 50, in assert_equal
41    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
42AssertionError: not({'height': 102, 'bestblock': '591e401cdaae94127f81f00ac25ec95edcdeac17c2031b772c71f92104091293', 'txouts': 103, 'bogosize': 7722, 'total_amount': Decimal('5100.00000000'), 'total_unspendable_amount': Decimal('50.00000000'), 'block_info': {'prevout_spent': Decimal('50.00000000'), 'coinbase': Decimal('50.00004440'), 'new_outputs_ex_coinbase': Decimal('49.99995560'), 'unspendable': Decimal('0E-8'), 'unspendables': {'genesis_block': Decimal('0E-8'), 'bip30': Decimal('0E-8'), 'scripts': Decimal('0E-8'), 'unclaimed_rewards': Decimal('0E-8')}}} == {'height': 102, 'bestblock': '591e401cdaae94127f81f00ac25ec95edcdeac17c2031b772c71f92104091293', 'txouts': 103, 'bogosize': 7722, 'total_amount': Decimal('5100.00000000')})
432021-03-17T01:08:20.267000Z TestFramework (INFO): Stopping nodes
442021-03-17T01:08:20.473000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_rxs4xbm6
452021-03-17T01:08:20.473000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_rxs4xbm6/test_framework.log
462021-03-17T01:08:20.473000Z TestFramework (ERROR): 
472021-03-17T01:08:20.474000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_rxs4xbm6' to consolidate all logs
482021-03-17T01:08:20.474000Z TestFramework (ERROR): 
492021-03-17T01:08:20.474000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
502021-03-17T01:08:20.474000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
512021-03-17T01:08:20.474000Z TestFramework (ERROR): 
52
53
54((HEAD detached at 540428a2c7)) $ test/functional/feature_coinstatsindex.py
552021-03-17T01:08:25.879000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_i2yd6ri_
562021-03-17T01:08:35.297000Z TestFramework (INFO): Test that gettxoutsetinfo() output is consistent with or without coinstatsindex option
572021-03-17T01:08:35.848000Z TestFramework (ERROR): Assertion failed
58Traceback (most recent call last):
59  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 128, in main
60    self.run_test()
61  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_coinstatsindex.py", line 33, in run_test
62    self._test_coin_stats_index()
63  File "/home/jon/projects/bitcoin/bitcoin/test/functional/feature_coinstatsindex.py", line 62, in _test_coin_stats_index
64    assert_equal(res1, res0)
65  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 50, in assert_equal
66    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
67AssertionError: not({'height': 102, 'bestblock': '3a98eacc26af554ce719e5f09257a9e8e535ee82fb363c79f4144b0100d26938', 'txouts': 103, 'bogosize': 7722, 'total_amount': Decimal('5100.00000000'), 'total_unspendable_amount': Decimal('50.00000000'), 'block_info': {'prevout_spent': Decimal('50.00000000'), 'coinbase': Decimal('50.00004440'), 'new_outputs_ex_coinbase': Decimal('49.99995560'), 'unspendable': Decimal('0E-8'), 'unspendables': {'genesis_block': Decimal('0E-8'), 'bip30': Decimal('0E-8'), 'scripts': Decimal('0E-8'), 'unclaimed_rewards': Decimal('0E-8')}}} == {'height': 102, 'bestblock': '3a98eacc26af554ce719e5f09257a9e8e535ee82fb363c79f4144b0100d26938', 'txouts': 103, 'bogosize': 7722, 'total_amount': Decimal('5100.00000000')})
682021-03-17T01:08:35.902000Z TestFramework (INFO): Stopping nodes
692021-03-17T01:08:36.111000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_i2yd6ri_
702021-03-17T01:08:36.112000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_i2yd6ri_/test_framework.log
712021-03-17T01:08:36.112000Z TestFramework (ERROR): 
722021-03-17T01:08:36.113000Z TestFramework (ERROR): Hint: Call /home/jon/projects/bitcoin/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_i2yd6ri_' to consolidate all logs
732021-03-17T01:08:36.113000Z TestFramework (ERROR): 
742021-03-17T01:08:36.113000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
752021-03-17T01:08:36.114000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
762021-03-17T01:08:36.114000Z TestFramework (ERROR):

fjahr force-pushed on Mar 17, 2021

fjahr commented at 9:38 pm on March 17, 2021: member

Thanks @fjahr! resolved in the latest push; moving forward, test/functional/feature_coinstatsindex.py fails at commit 540428a

test output x 3

Fixed and quick-checked the following commits. I think that should be the last of those. 🤞

jonatack commented at 11:58 am on March 23, 2021: member

Fixed and quick-checked the following commits. I think that should be the last of those. crossed_fingers

Yup, they check out for me too. Deleted my coinstats index files from last August and re-syncing under adversarial conditions (restarts, very frequent internet cuts) while trying the commands in mainnet, signet and testnet. The reviews by @Sjors are very helpful with ideas of issues to check / things to test.

in src/rpc/blockchain.cpp:1189 in b12f331cbf outdated

1185+
1186+            ret.pushKV("block_info", block_info);
1187+        }
1188     } else {
1189+        if (g_coin_stats_index && (g_coin_stats_index->GetSummary().synced == false)) {
1190+            throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set. Coinstatsindex is still syncing.");

jonatack commented at 12:05 pm on March 23, 2021:

Perhaps provide the best block height so the user doesn’t have to call getindexinfo to find out. Maybe a bit long, but something like:

Unable to read the UTXO set beyond the current best block height of 3190, as the coinstatsindex is still syncing.

leonardojobim commented at 8:52 pm on March 28, 2021:

Just a follow-up suggestion. It adds the current Coinstatsindex height in the error message, so the the user can see the progress of the synchronization.

0Unable to read UTXO set. Coinstatsindex is still syncing. Current height: 77329
1Unable to read UTXO set. Coinstatsindex is still syncing. Current height: 86827

0            throw JSONRPCError(RPC_INTERNAL_ERROR, "Unable to read UTXO set. Coinstatsindex is still syncing. Current height: " + 
1                std::to_string(g_coin_stats_index->GetSummary().best_block_height));

fjahr commented at 10:38 pm on April 5, 2021:

Thanks, I added this with some tweaks.

fjahr commented at 10:39 pm on April 5, 2021:

Done

in src/crypto/muhash.cpp:344 in b12f331cbf outdated

340@@ -341,6 +341,6 @@ MuHash3072& MuHash3072::Insert(Span<const unsigned char> in) noexcept {
341 }
342 
343 MuHash3072& MuHash3072::Remove(Span<const unsigned char> in) noexcept {
344-    m_numerator.Divide(ToNum3072(in));
345+    m_denominator.Multiply(ToNum3072(in));

jonatack commented at 2:13 pm on March 23, 2021:

9d4c991 if you retouch, it would be nice to provide some context in the commit message, point to a discussion or benchmark, etc.

jonatack commented at 8:58 am on April 11, 2021:

Thanks; done in latest update a4eaae4

in src/test/fuzz/coins_view.cpp:261 in b12f331cbf outdated

263@@ -264,7 +264,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
264                 CCoinsStats stats;

jonatack commented at 3:01 pm on March 23, 2021:

31d0b7f7 could make the same change here as in validation.cpp:L5421 in the same commit (or vice-versa)

Two possible simplifications:

a) pass the argument in the tests, drop unneeded ctor that is currently only used for tests

 0src/node/coinstats.h
 1@@ -56,7 +56,6 @@ struct CCoinsStats 
 2     CCoinsStats(CoinStatsHashType hash_type) : m_hash_type(hash_type) {}
 3-    CCoinsStats() : m_hash_type(CoinStatsHashType::HASH_SERIALIZED) {}
 4 
 5src/test/coinstatsindex_tests.cpp
 6@@ -14,7 +14,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
 7 { 
 8-    CCoinsStats coin_stats;
 9+    CCoinsStats coin_stats{CoinStatsHashType::HASH_SERIALIZED};
10     const CBlockIndex* block_index;
11@@ -57,7 +57,7 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
12 
13-    CCoinsStats new_coin_stats;
14+    CCoinsStats new_coin_stats{CoinStatsHashType::HASH_SERIALIZED};
15     const CBlockIndex* new_block_index;
16
17src/test/fuzz/coins_view.cpp
18@@ -261,7 +261,7 @@ FUZZ_TARGET_INIT(coins_view, initialize_coins_view)
19-                CCoinsStats stats;
20+                CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED};
21                 bool expected_code_path = false;

b) use the default member initializer as the general default, overridden by the specific one in ctor member initializer list

 0src/node/coinstats.h
 1@@ -26,7 +26,7 @@ enum class CoinStatsHashType { 
 2 struct CCoinsStats
 3 {
 4-    CoinStatsHashType m_hash_type;
 5+    CoinStatsHashType m_hash_type{CoinStatsHashType::HASH_SERIALIZED};
 6@@ -56,7 +56,7 @@ struct CCoinsStats 
 7-    CCoinsStats() : m_hash_type(CoinStatsHashType::HASH_SERIALIZED) {}
 8+    CCoinsStats() {}
 9 };
10
11src/validation.cpp
12@@ -5418,7 +5418,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot( 
13-    CCoinsStats stats{CoinStatsHashType::HASH_SERIALIZED};
14+    CCoinsStats stats;

fjahr commented at 11:01 pm on April 6, 2021:

I have dropped the ctor. We need to keep the explicit passing of the hash type in validation in case any default changes but we still need hash_serialized for assume_utxo.

DrahtBot added the label Needs rebase on Mar 24, 2021

MarcoFalke referenced this in commit 9b48b3ac42 on Mar 26, 2021

jonatack commented at 8:06 pm on March 26, 2021: member

WIP update: resynced coinstatsindex on mainnet and am part of the way through the commits. The debug builds are clean and tests pass at each commit.

Note to reviewers: you will probably sync coinstatsindex twice faster with a regular build, e.g. without --enable-debug.

A few comments below. After that, I began saving suggestions in an annotated branch at https://github.com/jonatack/bitcoin/commits/pr19521-review, which has the benefit that it takes less space than here on GitHub.

jonatack commented at 6:57 pm on March 27, 2021: member

Review update: reviewed up to 590b887 and added suggestions in separate annotated commits in the branch at https://github.com/jonatack/bitcoin/commits/pr19521-review

https://github.com/jonatack/bitcoin/commit/e9262d2b38ebfe134b42e44ff3b88cfb89f744cc : “PR19521 suggestions, commit e880a5e Add Coinstats index”
https://github.com/jonatack/bitcoin/commit/d4fb34de51a3d21ae68e8331822020e6fc2f06e6 : “PR19521 suggestions, commit 435ed5b Coinstats index can be activated with command line flag”
https://github.com/jonatack/bitcoin/commit/452a4fb5c03c024d4e98a38ef3806f921888d641 : “PR19521 suggestions, commit 6aedd7b gettxoutsetinfo can be requested for specific blockheights”
https://github.com/jonatack/bitcoin/commit/f6ebc75c78442cf0fde8c4330ebdda32a426c1a3 : “PR19521 suggestions, commit dc26799 Add unit test for Coinstats index” (among other things updated the tests to use std::chrono)
https://github.com/jonatack/bitcoin/commit/ae3d058394ad0268f491b240372e76b0a0457719 : “PR19521 suggestions, commit 590b887 Add verbose amounts tracking to Coinstats index”

jonatack commented at 6:58 pm on March 27, 2021: member

Here is a test commit you can pull in, if you like, to add coverage for c3cfc509 Add Coinstats index to getindexinfo

https://github.com/jonatack/bitcoin/commits/pr19521-coinstatsindex-getindexinfo-test-coverage

in src/index/coinstatsindex.cpp:143 in b12f331cbf outdated

137+
138+            // Skip duplicate txid coinbase transactions (BIP30).
139+            if (is_bip30_block && tx->IsCoinBase()) {
140+                m_block_unspendable_amount += block_subsidy;
141+                m_unspendables_bip30 += block_subsidy;
142+                continue;

jonatack commented at 7:52 pm on March 27, 2021:

590b887de0b5f6cb didn’t look deeply but does there need to be an equivalent inverse of this BIP30 duplicate transactions code in CoinStatsIndex::ReverseBlock()?

fjahr commented at 10:38 pm on April 5, 2021:

No, only if we would expect to reorg this deep :)

Sjors commented at 9:20 am on April 6, 2021:

Which (also) can’t happen because of the checkpoint at height 295,000.

leonardojobim approved

leonardojobim commented at 9:08 pm on March 28, 2021: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/19521/commits/b12f331cbf2544291693368f3600937965f05490 on the mainnet on Ubuntu 20.04.

Worked as expected with the tip of the chain and with the blocks mentioned in #19521#pullrequestreview-505535088 . Tested with hash_type=none, hash_type=muhash and with height and hash.

jonatack commented at 4:17 pm on March 29, 2021: member

Approach ACK. I finished this pass of reviewing and have pushed annotated comments/fixes/suggestions, per-commit, in https://github.com/jonatack/bitcoin/commits/pr19521-review (with a few initial comments in the first commit and in my comments here above).

in src/init.cpp:404 in 435ed5ba65 outdated

400@@ -393,6 +401,11 @@ void SetupServerArgs(NodeContext& node)
401 #endif
402     argsman.AddArg("-blockreconstructionextratxn=<n>", strprintf("Extra transactions to keep in memory for compact block reconstructions (default: %u)", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
403     argsman.AddArg("-blocksonly", strprintf("Whether to reject transactions from network peers. Automatic broadcast and rebroadcast of any transactions from inbound peers is disabled, unless the peer has the 'forcerelay' permission. RPC transactions are not affected. (default: %u)", DEFAULT_BLOCKSONLY), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
404+    argsman.AddArg("-blockfilterindex=<type>",

achow101 commented at 8:52 pm on March 29, 2021:

In 435ed5ba6589bf8391c3374ebf74bafed5f13237 “index: Coinstats index can be activated with command line flag”

This is an unrelated move. It isn’t necessary to have all of the arguments in alphabetical order, the formatter for output will do that automatically.

fjahr commented at 10:38 pm on April 5, 2021:

Undone

in src/init.cpp:1046 in 435ed5ba65 outdated

1042@@ -1034,10 +1043,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1043         nLocalServices = ServiceFlags(nLocalServices | NODE_COMPACT_FILTERS);
1044     }
1045 
1046-    // if using block pruning, then disallow txindex
1047+    // if using block pruning, then disallow txindex, coinstatsindex and blockfilterindex

achow101 commented at 8:53 pm on March 29, 2021:

In 435ed5ba6589bf8391c3374ebf74bafed5f13237 “index: Coinstats index can be activated with command line flag”

This comment is incorrect. The block filter index is not disallowed when pruning is enabled.

jonatack commented at 9:26 pm on March 29, 2021:

Agree: https://github.com/jonatack/bitcoin/commit/d4fb34de51a3d21ae68e8331822020e6fc2f06e6#r48780784

fjahr commented at 10:38 pm on April 5, 2021:

It was when I first wrote that commit ;) fixed

in src/node/coinstats.cpp:106 in 435ed5ba65 outdated

103+        pindex = blockman.LookupBlockIndex(stats.hashBlock);
104+    }
105+
106+    stats.nHeight = pindex->nHeight;
107+
108+    // Use CoinStatsIndex if it is available and hash_type none was requested

achow101 commented at 8:57 pm on March 29, 2021:

In 435ed5ba6589bf8391c3374ebf74bafed5f13237 “index: Coinstats index can be activated with command line flag”

hash_type muhash is also allowed.

jonatack commented at 9:25 pm on March 29, 2021:

agree, made a similar comment here: https://github.com/jonatack/bitcoin/commit/d4fb34de51a3d21ae68e8331822020e6fc2f06e6#r48780818

fjahr commented at 10:38 pm on April 5, 2021:

Done

in src/index/coinstatsindex.cpp:139 in e880a5e95b outdated

134+                m_total_amount += coin.out.nValue;
135+                m_bogo_size += GetBogoSize(coin.out.scriptPubKey);
136+            }
137+
138+            // The coinbase tx has no undo data since no former output is spent
139+            if (i > 0) {

achow101 commented at 9:12 pm on March 29, 2021:

In e880a5e95b999369bc277ed4d0ecbf0eaa758e88 “index: Add Coinstats index”

This would be easier to read as if (!tx->IsCoinbase())

fjahr commented at 10:38 pm on April 5, 2021:

Done

achow101 commented at 9:15 pm on March 29, 2021: member

Concept ACK

Needs rebase.

fjahr force-pushed on Apr 5, 2021

DrahtBot removed the label Needs rebase on Apr 5, 2021

Sjors commented at 9:20 am on April 6, 2021: member

re-tACK 5860532c4041bc3f90fc1f8919f56b6f3955ae9f

Muhash at height 677990: c49dc3e5ab445812a3eeb8460aa169b4b8900fe8e8c70b328bb7fa0edb0d4690

in src/index/coinstatsindex.cpp:191 in 5860532c40 outdated

184+                    m_bogo_size -= GetBogoSize(coin.out.scriptPubKey);
185+                }
186+            }
187+        }
188+    } else {
189+        m_block_unspendable_amount += block_subsidy;

jonatack commented at 12:56 pm on April 6, 2021:

5b683ab suggest adding a comment here, as this is quite far away from the parent if

0     } else {
1+        // genesis block
2         m_block_unspendable_amount += block_subsidy;
3         m_unspendables_genesis_block += block_subsidy;

fjahr commented at 11:01 pm on April 6, 2021:

Done

in src/rpc/blockchain.cpp:1077 in 5860532c40 outdated

1074+                "Note this call may take some time if you are not using coinstatsindex.\n",
1075                 {
1076-                    {"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
1077+                    {"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'"},
1078+                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex)", "", {"", "string or numeric"}},
1079+                    {"use_index", RPCArg::Type::BOOL, /* default */ "true", "Use Coinstatsindex even when it is available."},

jonatack commented at 1:25 pm on April 6, 2021:

7287e76f28

what does this mean: “Use Coinstatsindex even when it is available.” … I am #notsurehere but maybe it is supposed to say:

0                    {"use_index", RPCArg::Type::BOOL, /* default */ "true", "Use coinstatsindex, if available"},

s/Coinstatsindex/coinstatsindex/ for consistency
drop full stops for consistency

fjahr commented at 11:01 pm on April 6, 2021:

Hm, I already fixed this but must have undone it somehow in all the rebasing :-/ I hope it works now.

in src/rpc/blockchain.cpp:1192 in 5860532c40 outdated

1188     } else {
1189+        if (g_coin_stats_index) {
1190+            IndexSummary summary = g_coin_stats_index->GetSummary();
1191+
1192+            if (!summary.synced) {
1193+                throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to read UTXO set. coinstatsindex is still syncing. Current height: %d", summary.best_block_height));

jonatack commented at 2:02 pm on April 6, 2021:

13d2fc63ac

Verified this new error message (thanks for improving this):

0$ bitcoin-cli -signet gettxoutsetinfo muhash 32353
1error code: -32603
2error message:
3Unable to read UTXO set. coinstatsindex is still syncing. Current height: 31025

suggestion, as “coinstatsindex” after a full stop looks a little odd:

0                throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to read UTXO set because coinstatsindex is still syncing. Current height: %d", summary.best_block_height));

in src/rpc/blockchain.cpp:1189 in 5860532c40 outdated

1185+
1186+            ret.pushKV("block_info", block_info);
1187+        }
1188     } else {
1189+        if (g_coin_stats_index) {
1190+            IndexSummary summary = g_coin_stats_index->GetSummary();

jonatack commented at 2:06 pm on April 6, 2021:

13d2fc63ac

0            const IndexSummary summary{g_coin_stats_index->GetSummary()};

fjahr commented at 11:01 pm on April 6, 2021:

Done

in src/init.cpp:1576 in 5860532c40 outdated

1572@@ -1562,6 +1573,7 @@ bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAn
1573         filter_index_cache = max_cache / n_indexes;
1574         nTotalCache -= filter_index_cache * n_indexes;
1575     }
1576+    int64_t coin_stats_cache_size = 0;

jonatack commented at 2:09 pm on April 6, 2021:

13d2fc6 looks like this can be const (I’m not sure there is a reason to declare this variable instead of doing std::make_unique<CoinStatsIndex>(/* coin_stats_cache_size */ 0, false, fReindex); at line 1829 below)

fjahr commented at 11:02 pm on April 6, 2021:

Done (inlined)

jonatack commented at 2:39 pm on April 6, 2021: member

ACK 5860532c4041bc3f90fc1f8919f56b6f3955ae9f

Muhash at height 677990: c49dc3e5ab445812a3eeb8460aa169b4b8900fe8e8c70b328bb7fa0edb0d4690 (same as Sjors)

Thanks for pulling in the test and all the suggestions. A few minor comments below that can be ignored or done in a follow-up, plus these which I’m caching here:

fjahr force-pushed on Apr 6, 2021

fjahr commented at 11:08 pm on April 6, 2021: member

Sorry, I didn’t get around to write an update after I pushed yesterday. I further addressed comments from @jonatack and I think I should have addressed everything now :)

I don’t think it is necessary to disallow the coin stats index when pruning is enabled. The only reason that txindex disallows it is because txindex gets the transaction data by reading it off disk from the block files, so that is incompatible with pruning.

The coin stats index doesn’t have the same problem. The only issue would be if there were a large reorg and it needed the undo data that has already been deleted. However this is an issue overall of pruned nodes so it shouldn’t be a concern for the coin stats index.

Yes, but I would like to keep this for a follow-up because if I do it the same way as it is done for the blockfilterindexes it makes coinstatsindex a new dependency for validation and that would also be a new circular dependency. But I am working on a better solution, it just requires some refactoring that is beyond the scope of this PR IMO.

DrahtBot added the label Needs rebase on Apr 7, 2021

fjahr force-pushed on Apr 10, 2021

fjahr commented at 10:29 pm on April 10, 2021: member

Rebased

DrahtBot removed the label Needs rebase on Apr 10, 2021

jonatack commented at 9:12 am on April 11, 2021: member

re-ACK a4eaae4697d027f511ca2fa216b2ec0480b0be5d per git diff 5860532 074cc23 (before rebase) and git range-diff 1e3db68 5860532 a4eaae4 (after rebase), debug build is clean, lightly re-tested on signet

 0$ bitcoin-cli -signet gettxoutsetinfo "none" 90111
 1error code: -8
 2error message:
 3Target block height 90111 after current tip 33053
 4
 5$ bitcoin-cli -signet gettxoutsetinfo "muhash" 33053
 6error code: -32603
 7error message:
 8Unable to read UTXO set because coinstatsindex is still syncing. Current height: 32533
 9
10...a few seconds later...
11
12$ bitcoin-cli gettxoutsetinfo muhash 33053
13{
14  "height": 33053,
15  "bestblock": "000000ef50b4fd3572a61e461ea156a936906fcf9828c9f2cb305391bbb5078c",
16  "txouts": 48452,
17  "bogosize": 3545492,
18  "muhash": "8620187d4cb621cee5c409fc3809d43d9029557b6b07f646d8293156e69e6a8f",
19  "total_amount": 1652650.00000000,
20  "total_unspendable_amount": 50.00000000,
21  "block_info": {
22    "prevout_spent": 6.65019527,
23    "coinbase": 50.00000433,
24    "new_outputs_ex_coinbase": 6.65019094,
25    "unspendable": 0.00000000,
26    "unspendables": {
27      "genesis_block": 0.00000000,
28      "bip30": 0.00000000,
29      "scripts": 0.00000000,
30      "unclaimed_rewards": 0.00000000
31    }
32  }
33}
34
35$  bitcoin-cli -signet help gettxoutsetinfo
36gettxoutsetinfo ( "hash_type" hash_or_height use_index )
37
38Returns statistics about the unspent transaction output set.
39Note this call may take some time if you are not using coinstatsindex.
40
41Arguments:
421. hash_type         (string, optional, default=hash_serialized_2) Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'
432. hash_or_height    (string or numeric) The block hash or height of the target height (only available with coinstatsindex)
443. use_index         (boolean, optional, default=true) Use coinstatsindex, if available.
45
46Result:
47{                                     (json object)
48  "height" : n,                       (numeric) The block height (index) of the returned statistics
49  "bestblock" : "hex",                (string) The hash of the block at which these statistics are calculated
50  "txouts" : n,                       (numeric) The number of unspent transaction outputs
51  "bogosize" : n,                     (numeric) Database-independent, meaningless metric indicating the UTXO set size
52  "hash_serialized_2" : "hex",        (string, optional) The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)
53  "muhash" : "hex",                   (string, optional) The serialized hash (only present if 'muhash' hash_type is chosen)
54  "transactions" : n,                 (numeric) The number of transactions with unspent outputs (not available when coinstatsindex is used)
55  "disk_size" : n,                    (numeric) The estimated size of the chainstate on disk (not available when coinstatsindex is used)
56  "total_amount" : n,                 (numeric) The total amount of coins in the UTXO set
57  "total_unspendable_amount" : n,     (numeric) The total amount of coins permanently excluded from the UTXO set (only available if coinstatsindex is used)
58  "block_info" : {                    (json object) Info on amounts in the block at this block height (only available if coinstatsindex is used)
59    "prevout_spent" : n,              (numeric)
60    "coinbase" : n,                   (numeric)
61    "new_outputs_ex_coinbase" : n,    (numeric)
62    "unspendable" : n,                (numeric)
63    "unspendables" : {                (json object) Detailed view of the unspendable categories
64      "genesis_block" : n,            (numeric)
65      "bip30" : n,                    (numeric) Transactions overridden by duplicates (no longer possible with BIP30)
66      "scripts" : n,                  (numeric) Amounts sent to scripts that are unspendable (for example OP_RETURN outputs)
67      "unclaimed_rewards" : n         (numeric) Fee rewards that miners did not claim in their coinbase transaction
68    }
69  }
70}
71
72Examples:
73> bitcoin-cli gettxoutsetinfo 
74> bitcoin-cli gettxoutsetinfo "none"
75> bitcoin-cli gettxoutsetinfo "none" 1000
76> bitcoin-cli gettxoutsetinfo "none" '"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"'
77> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "gettxoutsetinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
78> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "gettxoutsetinfo", "params": ["none"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
79> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "gettxoutsetinfo", "params": ["none", 1000]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
80> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "gettxoutsetinfo", "params": ["none", "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

ivanacostarubio commented at 10:38 pm on April 12, 2021: none

Tested ACK. a4eaae4

Manual Testing:

 0./src/bitcoin-cli gettxoutsetinfo muhash 677990
 1{
 2  "height": 677990,
 3  "bestblock": "00000000000000000005e25f3d3e12bf69664dcfe78e2b90a58d5a9e596eef6b",
 4  "txouts": 74586452,
 5  "bogosize": 5588597888,
 6  "muhash": "0f829dbd43ecd72de16a8b906f287600a0426ea0bb6deb1bc15772af2e534628",
 7  "total_amount": 18674738.62932678,
 8  "total_unspendable_amount": 205.12067322,
 9  "block_info": {
10    "prevout_spent": 15754.91848125,
11    "coinbase": 7.03601537,
12    "new_outputs_ex_coinbase": 15754.13246588,
13    "unspendable": 0.00000000,
14    "unspendables": {
15      "genesis_block": 0.00000000,
16      "bip30": 0.00000000,
17      "scripts": 0.00000000,
18      "unclaimed_rewards": 0.00000000
19    }
20  }
21}

 0./src/bitcoin-cli  gettxoutsetinfo "none" 1000
 1{
 2  "height": 1000,
 3  "bestblock": "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09",
 4  "txouts": 998,
 5  "bogosize": 116724,
 6  "total_amount": 50000.00000000,
 7  "total_unspendable_amount": 50.00000000,
 8  "block_info": {
 9    "prevout_spent": 0.00000000,
10    "coinbase": 50.00000000,
11    "new_outputs_ex_coinbase": 0.00000000,
12    "unspendable": 0.00000000,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000000,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

 0./src/bitcoin-cli gettxoutsetinfo "none" '"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"'
 1{
 2  "height": 1000,
 3  "bestblock": "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09",
 4  "txouts": 998,
 5  "bogosize": 116724,
 6  "total_amount": 50000.00000000,
 7  "total_unspendable_amount": 50.00000000,
 8  "block_info": {
 9    "prevout_spent": 0.00000000,
10    "coinbase": 50.00000000,
11    "new_outputs_ex_coinbase": 0.00000000,
12    "unspendable": 0.00000000,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000000,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

I saw this on the logs:

02021-04-11T18:53:56Z Syncing coinstatsindex with block chain from height 617225
12021-04-11T18:54:33Z Syncing coinstatsindex with block chain from height 617231
22021-04-11T18:55:04Z Syncing coinstatsindex with block chain from height 617280

Without coinstatsindex enabled:

0./src/bitcoin-cli gettxoutsetinfo muhash 677990
1
2error code: -8
3error message:
4Querying specific block heights requires coinstatsindex

Sjors commented at 12:21 pm on April 13, 2021: member

re-tACK a4eaae4

Muhash at height 679,048: f5058d7485db847f189ad33c80ecdbb8ad0ceba3fe3e9e32a3ada9553dc52406

Of course it needs a rebase 2 hours after my re-ACK :-(

DrahtBot added the label Needs rebase on Apr 13, 2021

grunch commented at 3:54 pm on April 14, 2021: none

Tested ACK. a4eaae4

Manual Testing:

 0$ src/bitcoin-cli gettxoutsetinfo none '"00000000d1145790a8694403d4063f323d499e655c83426834d4ce2f8dd4a2ee"'
 1{
 2  "height": 170,
 3  "bestblock": "00000000d1145790a8694403d4063f323d499e655c83426834d4ce2f8dd4a2ee",
 4  "txouts": 171,
 5  "bogosize": 20007,
 6  "total_amount": 8500.00000000,
 7  "total_unspendable_amount": 50.00000000,
 8  "block_info": {
 9    "prevout_spent": 50.00000000,
10    "coinbase": 50.00000000,
11    "new_outputs_ex_coinbase": 50.00000000,
12    "unspendable": 0.00000000,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000000,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

0$ src/bitcoin-cli gettxoutsetinfo none 400000
1error code: -32603
2error message:
3Unable to read UTXO set because coinstatsindex is still syncing. Current height: 291230

 0$ src/bitcoin-cli gettxoutsetinfo none 200000
 1{
 2  "height": 200000,
 3  "bestblock": "000000000000034a7dedef4a161fa058a2d67a173a90155f3a2fe6fc132e0ebf",
 4  "txouts": 2318056,
 5  "bogosize": 175620421,
 6  "total_amount": 9999889.98361183,
 7  "total_unspendable_amount": 160.01638817,
 8  "block_info": {
 9    "prevout_spent": 211681.67936751,
10    "coinbase": 50.63517500,
11    "new_outputs_ex_coinbase": 211681.04419251,
12    "unspendable": 0.00000000,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000000,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

 0$ src/bitcoin-cli gettxoutsetinfo muhash 100000
 1{
 2  "height": 100000,
 3  "bestblock": "000000000003ba27aa200b1cecaad478d2b00432346c3f1f3986da1afd33e506",
 4  "txouts": 71888,
 5  "bogosize": 7606209,
 6  "muhash": "86aa993b32b9df2afa1a2c4855d34911146ad77328a82887e058d23f01831e3e",
 7  "total_amount": 4999900.00000000,
 8  "total_unspendable_amount": 150.00000000,
 9  "block_info": {
10    "prevout_spent": 53.01000000,
11    "coinbase": 50.00000000,
12    "new_outputs_ex_coinbase": 53.01000000,
13    "unspendable": 0.00000000,
14    "unspendables": {
15      "genesis_block": 0.00000000,
16      "bip30": 0.00000000,
17      "scripts": 0.00000000,
18      "unclaimed_rewards": 0.00000000
19    }
20  }
21}

0$ src/bitcoin-cli gettxoutsetinfo none
1error code: -32603
2error message:
3Unable to read UTXO set because coinstatsindex is still syncing. Current height: 299131

fjahr force-pushed on Apr 14, 2021

fjahr commented at 7:52 pm on April 14, 2021: member

Ain’t no party like a rebase party, ‘cause a rebase party don’t stop…

jonatack commented at 9:13 pm on April 14, 2021: member

Code review re-ACK 1be67944193241ff5aee60188f2730367a638500 per git range-diff 773f8c1 a4eaae4 1be6794, only changes since my last review are rebase due to added neighboring headers and circular dependencies from merged #21575

jonatack commented at 9:16 pm on April 14, 2021: member

(also verified that the unrelated-looking cirrus CI test failure in feature_notifications.py#L172 does not occur for me locally and opened issue #21683)

DrahtBot removed the label Needs rebase on Apr 14, 2021

Sjors commented at 7:20 am on April 15, 2021: member

re-utACK 1be67944193241ff5aee60188f2730367a638500: what jonatack said

ivanacostarubio commented at 8:07 pm on April 15, 2021: none

re-tACK 1be6794

After recompiling:

Right after starting bitcoind:

0 ./src/bitcoin-cli gettxoutsetinfo 'muhash' 677990
1error code: -28
2error message:
3Verifying blocks...

After verified the blocks:

 0src/bitcoin-cli gettxoutsetinfo 'muhash' 677990
 1{
 2  "height": 677990,
 3  "bestblock": "00000000000000000005e25f3d3e12bf69664dcfe78e2b90a58d5a9e596eef6b",
 4  "txouts": 74586452,
 5  "bogosize": 5588597888,
 6  "muhash": "0f829dbd43ecd72de16a8b906f287600a0426ea0bb6deb1bc15772af2e534628",
 7  "total_amount": 18674738.62932678,
 8  "total_unspendable_amount": 205.12067322,
 9  "block_info": {
10    "prevout_spent": 15754.91848125,
11    "coinbase": 7.03601537,
12    "new_outputs_ex_coinbase": 15754.13246588,
13    "unspendable": 0.00000000,
14    "unspendables": {
15      "genesis_block": 0.00000000,
16      "bip30": 0.00000000,
17      "scripts": 0.00000000,
18      "unclaimed_rewards": 0.00000000
19    }
20  }
21}

 0./src/bitcoin-cli  gettxoutsetinfo "none" 1000
 1{
 2 "height": 1000,
 3 "bestblock": "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09",
 4 "txouts": 998,
 5 "bogosize": 116724,
 6 "total_amount": 50000.00000000,
 7 "total_unspendable_amount": 50.00000000,
 8 "block_info": {
 9   "prevout_spent": 0.00000000,
10   "coinbase": 50.00000000,
11   "new_outputs_ex_coinbase": 0.00000000,
12   "unspendable": 0.00000000,
13   "unspendables": {
14     "genesis_block": 0.00000000,
15     "bip30": 0.00000000,
16     "scripts": 0.00000000,
17     "unclaimed_rewards": 0.00000000
18   }
19 }
20}

DrahtBot added the label Needs rebase on Apr 17, 2021

fjahr force-pushed on Apr 17, 2021

fjahr commented at 10:13 pm on April 17, 2021: member

Ain’t no party like a rebase party, ‘cause a rebase party don’t stop…

🕺

in src/rpc/blockchain.cpp:165 in 0123b7e4e2 outdated

160+        CBlockIndex* pindex = chainman.m_blockman.LookupBlockIndex(hash);
161+
162+        if (!pindex) {
163+            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
164+        }
165+        if (active_chain.Contains(pindex)) {

jonatack commented at 10:26 pm on April 17, 2021:

In 0123b7e4e2 “rpc: gettxoutsetinfo can be requested for specific blockheights” the range-diff seems to be:

0-        if (!::ChainActive().Contains(pindex)) {
1+        if (active_chain.Contains(pindex)) {

Should it be if (!active_chain.Contains(pindex)) { ? (it’s late so I may be misreading)

jonatack commented at 10:32 pm on April 17, 2021:

Looks like that commit and then f87cf789 are affected. Ugh, my sympathies for the rebase.

fjahr commented at 10:53 pm on April 17, 2021:

Yepp, thanks. Fixed!

DrahtBot removed the label Needs rebase on Apr 17, 2021

fjahr force-pushed on Apr 17, 2021

jonatack commented at 2:19 pm on April 18, 2021: member

Code review re-ACK 07201d39d1c10ddbf8a576915203bf3aa4be135c changes appear to be rebase-only per git range-diff 0dd7b23 1be6794 07201d3.

Debug build is clean; I didn’t build and test each commit like in my previous reviews above.

It may be good if someone familiar with the chainman changes in #21391 et al that caused the rebase had a look.

aitorjs commented at 5:38 pm on April 18, 2021: contributor

Tested ACK. 07201d3

Manual Testing on testnet:

 0$ src/bitcoind -coinstatsindex
 12021-04-18T20:07:19Z Syncing coinstatsindex with block chain from height 15716
 22021-04-18T20:07:50Z Syncing coinstatsindex with block chain from height 19420
 32021-04-18T20:07:57Z FlushStateToDisk: write coins cache to disk (1580417 coins, 218854kB) started
 42021-04-18T20:08:02Z FlushStateToDisk: write coins cache to disk (1580417 coins, 218854kB) completed (4.79s)
 5
 62021-04-18T22:43:40Z Syncing coinstatsindex with block chain from height 1958230
 72021-04-18T22:44:11Z Syncing coinstatsindex with block chain from height 1961375
 82021-04-18T22:44:42Z Syncing coinstatsindex with block chain from height 1964519
 92021-04-18T22:45:13Z Syncing coinstatsindex with block chain from height 1967320
102021-04-18T22:45:44Z Syncing coinstatsindex with block chain from height 1969832
112021-04-18T22:46:15Z Syncing coinstatsindex with block chain from height 1971923
122021-04-18T22:46:15Z coinstatsindex is enabled at height 1971953
132021-04-18T22:46:15Z coinstatsindex thread exit

 0$ src/bitcoin-cli gettxoutsetinfo 'none' 0 true
 1{
 2
 3  "height": 0,
 4  "bestblock": "000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943",
 5  "txouts": 0,
 6  "bogosize": 0,
 7  "total_amount": 0.00000000,
 8  "total_unspendable_amount": 50.00000000,
 9  "block_info": {
10    "prevout_spent": 0.00000000,
11    "coinbase": 0.00000000,
12    "new_outputs_ex_coinbase": 0.00000000,
13    "unspendable": 50.00000000,
14    "unspendables": {
15      "genesis_block": 50.00000000,
16      "bip30": 0.00000000,
17      "scripts": 0.00000000,
18      "unclaimed_rewards": 0.00000000
19    }
20  }
21}

 0$ src/bitcoin-cli gettxoutsetinfo muhash 0
 1{
 2  "height": 0,
 3  "bestblock": "000000000933ea01ad0ee984209779baaec3ced90fa3f408719526f8d77f4943",
 4  "txouts": 0,
 5  "bogosize": 0,
 6  "muhash": "dd5ad2a105c2d29495f577245c357409002329b9f4d6182c0af3dc2f462555c8",
 7  "total_amount": 0.00000000,
 8  "total_unspendable_amount": 50.00000000,
 9  "block_info": {
10    "prevout_spent": 0.00000000,
11    "coinbase": 0.00000000,
12    "new_outputs_ex_coinbase": 0.00000000,
13    "unspendable": 50.00000000,
14    "unspendables": {
15      "genesis_block": 50.00000000,
16      "bip30": 0.00000000,
17      "scripts": 0.00000000,
18      "unclaimed_rewards": 0.00000000
19    }
20  }
21}

0$ src/bitcoin-cli gettxoutsetinfo 'none' 1800003 true
1error code: -32603
2error message:
3Unable to read UTXO set because coinstatsindex is still syncing. Current height: 403170

0$ src/bitcoin-cli gettxoutsetinfo muhash 677990
1error code: -32603
2error message:
3Unable to read UTXO set because coinstatsindex is still syncing. Current height: 403357

 0$ src/bitcoin-cli gettxoutsetinfo 'none' 500000 true
 1{
 2  "height": 500000,
 3  "bestblock": "000000000001a7c0aaa2630fbb2c0e476aafffc60f82177375b2aaa22209f606",
 4  "txouts": 5447499,
 5  "bogosize": 413044654,
 6  "total_amount": 16749305.03353846,
 7  "total_unspendable_amount": 707.46646154,
 8  "block_info": {
 9    "prevout_spent": 0.00000000,
10    "coinbase": 12.50000000,
11    "new_outputs_ex_coinbase": 0.00000000,
12    "unspendable": 0.00000000,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000000,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

 0$ src/bitcoin-cli gettxoutsetinfo 'none' '"00000000009e2958c15ff9290d571bf9459e93b19765c6801ddeccadbb160a1e"' true
 1{
 2  "height": 100000,
 3  "bestblock": "00000000009e2958c15ff9290d571bf9459e93b19765c6801ddeccadbb160a1e",
 4  "txouts": 274250,
 5  "bogosize": 21344460,
 6  "total_amount": 4999849.36870600,
 7  "total_unspendable_amount": 200.63129400,
 8  "block_info": {
 9    "prevout_spent": 0.00000000,
10    "coinbase": 50.00000000,
11    "new_outputs_ex_coinbase": 0.00000000,
12    "unspendable": 0.00000000,
13    "unspendables": {
14      "genesis_block": 0.00000000,
15      "bip30": 0.00000000,
16      "scripts": 0.00000000,
17      "unclaimed_rewards": 0.00000000
18    }
19  }
20}

 0$ src/bitcoin-cli gettxoutsetinfo muhash '"00000000009e2958c15ff9290d571bf9459e93b19765c6801ddeccadbb160a1e"'
 1{
 2  "height": 100000,
 3  "bestblock": "00000000009e2958c15ff9290d571bf9459e93b19765c6801ddeccadbb160a1e",
 4  "txouts": 274250,
 5  "bogosize": 21344460,
 6  "muhash": "a014cef901c62e92a84004a0f77999062a44210329e3bc21e1b96d9d8a392fb1",
 7  "total_amount": 4999849.36870600,
 8  "total_unspendable_amount": 200.63129400,
 9  "block_info": {
10    "prevout_spent": 0.00000000,
11    "coinbase": 50.00000000,
12    "new_outputs_ex_coinbase": 0.00000000,
13    "unspendable": 0.00000000,
14    "unspendables": {
15      "genesis_block": 0.00000000,
16      "bip30": 0.00000000,
17      "scripts": 0.00000000,
18      "unclaimed_rewards": 0.00000000
19    }
20  }
21}

 0$ test/functional/feature_coinstatsindex.py
 12021-04-18T17:06:20.254000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_4klxhrpj
 22021-04-18T17:06:21.796000Z TestFramework (INFO): Test that gettxoutsetinfo() output is consistent with or without coinstatsindex option
 32021-04-18T17:06:21.862000Z TestFramework (INFO): Test that gettxoutsetinfo() can get fetch data on specific heights with index
 42021-04-18T17:06:22.013000Z TestFramework (INFO): Test gettxoutsetinfo() with index and verbose flag
 52021-04-18T17:06:22.178000Z TestFramework (INFO): Test that the index is robust across restarts
 62021-04-18T17:06:22.880000Z TestFramework (INFO): Test use_index option for nodes running the index
 72021-04-18T17:06:23.002000Z TestFramework (INFO): Test that index can handle reorgs
 82021-04-18T17:06:23.516000Z TestFramework (INFO): Test that a node aware of stale blocks syncs them as well
 92021-04-18T17:06:24.774000Z TestFramework (INFO): Test that the rpc raises if the legacy hash is passed with the index
102021-04-18T17:06:24.864000Z TestFramework (INFO): Stopping nodes
112021-04-18T17:06:25.017000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_4klxhrpj on exit
122021-04-18T17:06:25.018000Z TestFramework (INFO): Tests successful

0$ ~/.bitcoin/testnet3/indexes/coinstats$ du -h
1336M	./db
2336M	.

DrahtBot commented at 7:16 am on April 19, 2021: member

:partying_face: This pull request conflicts with the target branch and needs rebase party.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Apr 19, 2021

crypto: Make MuHash Remove method efficient

Division of MuHash objects are very expensive and multiplication relatively cheap. The whole idea of introducing and tracking numerator and denominators seperately as a representation of the internal state was so that divisions would be rare. So using divison in the Remove method did not make any sense and was just a silly mistake which is corrected here.

2e2648a902

refactor: Pass hash_type to CoinsStats in stats object 9c8a265fd2

refactor: Simplify ApplyStats and ApplyHash a8a46c4b3c

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.

dd58a4de21

index: Coinstats index can be activated with command line flag 3c914d58ff

rpc: gettxoutsetinfo can be requested for specific blockheights 3f166ecc12

test: Add functional test for Coinstats index 6a4c0c09ab

test: Add unit test for Coinstats index 57a026c30f

rpc: Add Coinstats index to getindexinfo ca01bb8d68

test: add coinstatsindex getindexinfo coverage, improve current tests 655d929836

rpc, index: Add verbose amounts tracking to Coinstats index 2501576ecc

test: Add tests for block_info in gettxoutsetinfo

This additional data will automatically be returned if the coinstats index is used.

e0938c2909

test: Test coinstatsindex robustness across restarts bb7788b121

index, rpc: Add use_index option for gettxoutsetinfo b9362392ae

rpc: Allow gettxoutsetinfo and getblockstats for stale blocks 90c966b0f3

test: Add test for coinstatsindex behavior in reorgs

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

23fe50436b

rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height 5f96d7d22d

fjahr force-pushed on Apr 19, 2021

fjahr commented at 7:28 pm on April 19, 2021: member

Rebased, review while it’s hot! 🔥

 0$ git range-diff master 07201d3 5f96d7d22
 1 1:  f8b5dd30f =  1:  2e2648a90 crypto: Make MuHash Remove method efficient
 2 2:  da85ed5a6 =  2:  9c8a265fd refactor: Pass hash_type to CoinsStats in stats object
 3 3:  85405cc2a =  3:  a8a46c4b3 refactor: Simplify ApplyStats and ApplyHash
 4 4:  873cd68fc =  4:  dd58a4de2 index: Add Coinstats index
 5 5:  e2d5eddfa =  5:  3c914d58f index: Coinstats index can be activated with command line flag
 6 6:  aeffcbdf5 !  6:  3f166ecc1 rpc: gettxoutsetinfo can be requested for specific blockheights
 7    @@ src/rpc/blockchain.cpp: static RPCHelpMan gettxoutsetinfo()
 8     -                "Note this call may take some time.\n",
 9     +                "Note this call may take some time if you are not using coinstatsindex.\n",
10                      {
11    --                    {"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
12    -+                    {"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'"},
13    +                     {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
14     +                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex)", "", {"", "string or numeric"}},
15                      },
16                      RPCResult{
17 7:  fa7a1f8ae =  7:  6a4c0c09a test: Add functional test for Coinstats index
18 8:  1edf2c26b =  8:  57a026c30 test: Add unit test for Coinstats index
19 9:  5194bf51d =  9:  ca01bb8d6 rpc: Add Coinstats index to getindexinfo
2010:  2214cca58 = 10:  655d92983 test: add coinstatsindex getindexinfo coverage, improve current tests
2111:  3b89ad85f = 11:  2501576ec rpc, index: Add verbose amounts tracking to Coinstats index
2212:  08952fb10 = 12:  e0938c290 test: Add tests for block_info in gettxoutsetinfo
2313:  71e136d5a = 13:  bb7788b12 test: Test coinstatsindex robustness across restarts
2414:  6c99446b4 ! 14:  b9362392a index, rpc: Add use_index option for gettxoutsetinfo
25    @@ src/node/coinstats.h: struct CCoinsStats
26      ## src/rpc/blockchain.cpp ##
27     @@ src/rpc/blockchain.cpp: static RPCHelpMan gettxoutsetinfo()
28                      {
29    -                     {"hash_type", RPCArg::Type::STR, /* default */ "hash_serialized_2", "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'"},
30    +                     {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
31                          {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex)", "", {"", "string or numeric"}},
32    -+                    {"use_index", RPCArg::Type::BOOL, /* default */ "true", "Use coinstatsindex, if available."},
33    ++                    {"use_index", RPCArg::Type::BOOL, RPCArg::Default{true}, "Use coinstatsindex, if available."},
34                      },
35                      RPCResult{
36                          RPCResult::Type::OBJ, "", "",
3715:  1c0ddc4ef = 15:  90c966b0f rpc: Allow gettxoutsetinfo and getblockstats for stale blocks
3816:  8e61ced8e = 16:  23fe50436 test: Add test for coinstatsindex behavior in reorgs
3917:  07201d39d = 17:  5f96d7d22 rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height

MarcoFalke removed the label Needs rebase on Apr 19, 2021

Sjors commented at 8:11 pm on April 19, 2021: member

re-tACK 5f96d7d22d8e05876c6fc014e70488699950fe38

Muhash at height for block 000000000000000000000f0d7742e6cd22da819b59af361224f452baee8d31c4 (which or may or may not win the race) is 1d324ac1c67e5317ab40d5c9e23b5aae6a0049fa5649795d32553cae008076fb.

Sjors commented at 8:21 pm on April 19, 2021: member

Narrator: it lost, which is a good opportunity to test a reorg in real life. For height 679824 I now get 157ec719407b64f9204f464fcc203b04d288ff837a3ce5ca63edeb20a2614903.

in src/rpc/blockchain.cpp:1102 in 5f96d7d22d

1095@@ -1068,50 +1096,88 @@ static RPCHelpMan gettxoutsetinfo()
1096 {
1097     return RPCHelpMan{"gettxoutsetinfo",
1098                 "\nReturns statistics about the unspent transaction output set.\n"
1099-                "Note this call may take some time.\n",
1100+                "Note this call may take some time if you are not using coinstatsindex.\n",
1101                 {
1102                     {"hash_type", RPCArg::Type::STR, RPCArg::Default{"hash_serialized_2"}, "Which UTXO set hash should be calculated. Options: 'hash_serialized_2' (the legacy algorithm), 'muhash', 'none'."},
1103+                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex)", "", {"", "string or numeric"}},

jonatack commented at 10:53 pm on April 19, 2021:

nit if you have to rebase, this is the only help argument of the three that doesn’t end with a period.

0                    {"hash_or_height", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The block hash or height of the target height (only available with coinstatsindex).", "", {"", "string or numeric"}},

jonatack commented at 12:03 pm on May 1, 2021:

done in #21818

jonatack commented at 10:55 pm on April 19, 2021: member

Code review re-ACK 5f96d7d22d8e05876c6fc014e70488699950fe38 per git range-diff 13d27b4 07201d3 5f96d7d

promag commented at 9:04 am on April 21, 2021: member

I think it should error when hash_or_height is set but index is not available or use_index=false otherwise the result is misleading.

in src/node/coinstats.h:26 in 9c8a265fd2 outdated

22@@ -23,6 +23,7 @@ enum class CoinStatsHashType {
23 
24 struct CCoinsStats
25 {
26+    CoinStatsHashType m_hash_type;

promag commented at 9:08 am on April 21, 2021:

9c8a265fd21a87228c18a1661df99fedc1866baf

const CoinStatsHashType m_hash_type and drop constructor.

in src/node/coinstats.cpp:98 in 3f166ecc12 outdated

102-        assert(std::addressof(g_chainman.m_blockman) == std::addressof(blockman));
103-        pindex = blockman.LookupBlockIndex(stats.hashBlock);
104-        stats.nHeight = Assert(pindex)->nHeight;
105+
106+    if (!pindex) {
107+        {

promag commented at 9:33 am on April 21, 2021:

3f166ecc125fce6ccd995687fa16572090a5d099

Remove unnecessary block.

in src/rpc/blockchain.cpp:143 in 3f166ecc12 outdated

139@@ -140,6 +140,35 @@ static int ComputeNextBlockAndDepth(const CBlockIndex* tip, const CBlockIndex* b
140     return blockindex == tip ? 1 : -1;
141 }
142 
143+CBlockIndex* ParseHashOrHeight(const UniValue& param, ChainstateManager& chainman) {

promag commented at 9:35 am on April 21, 2021:

3f166ecc125fce6ccd995687fa16572090a5d099

nit, can be static and move { to new line.

promag commented at 9:49 am on April 21, 2021: member

Tested ACK 5f96d7d22d8e05876c6fc014e70488699950fe38. Light code review ACK 5f96d7d22d8e05876c6fc014e70488699950fe38.

Feel free to ignore nits. These and the above comment can be addressed later.

fjahr commented at 9:38 pm on April 24, 2021: member

I think it should error when hash_or_height is set but index is not available or use_index=false otherwise the result is misleading.

There is already a check for that and it should error in the case you describe: https://github.com/bitcoin/bitcoin/pull/19521/files#diff-decae4be02fb8a47ab4557fe74a9cb853bdfa3ec0fa1b515c0a1e5de91f4ad0bR1168

If you found a scenario where it doesn’t work, please let me know. There is at least a basic test for it here but maybe I have overlooked something: https://github.com/bitcoin/bitcoin/pull/19521/files#diff-a6434325d09a6df4b371513c837907dfc1a97cf540709def4bded9b2a17e3f49R115

laanwj merged this on Apr 30, 2021

laanwj closed this on Apr 30, 2021

laanwj commented at 3:35 pm on April 30, 2021: member

Code review ACK 5f96d7d22d8e05876c6fc014e70488699950fe38

laanwj removed this from the "Blockers" column in a project

sidhujag referenced this in commit ddf3334252 on May 1, 2021

domob1812 referenced this in commit 1b051737fe on May 3, 2021

domob1812 referenced this in commit 184cf01ee1 on May 4, 2021

in src/index/coinstatsindex.cpp:149 in 5f96d7d22d

144+            }
145+
146+            for (size_t j = 0; j < tx->vout.size(); ++j) {
147+                const CTxOut& out{tx->vout[j]};
148+                Coin coin{out, pindex->nHeight, tx->IsCoinBase()};
149+                COutPoint outpoint{tx->GetHash(), static_cast<uint32_t>(j)};

MarcoFalke commented at 6:26 am on May 18, 2021:

could just make j of type uint32_t to avoid the cast?

in src/index/coinstatsindex.cpp:163 in 5f96d7d22d

158+                m_muhash.Insert(MakeUCharSpan(TxOutSer(outpoint, coin)));
159+
160+                if (tx->IsCoinBase()) {
161+                    m_block_coinbase_amount += coin.out.nValue;
162+                } else {
163+                    m_block_new_outputs_ex_coinbase_amount += coin.out.nValue;

MarcoFalke commented at 6:37 am on May 18, 2021:

why is this called m_block...? The value is not per block, but the total, like all other CAmount members!? It might be good to call all of them m_block, or m_total, or just skip the prefix for all of them.

fjahr commented at 6:10 pm on May 24, 2021:

Yepp, it used to be per block, but the names were not changed when the values were changed to track totals.

in src/index/coinstatsindex.cpp:222 in 5f96d7d22d

217+
218+    uint256 out;
219+    m_muhash.Finalize(out);
220+    value.second.muhash = out;
221+
222+    return m_db->Write(DBHeightKey(pindex->nHeight), value) && m_db->Write(DB_MUHASH, m_muhash);

MarcoFalke commented at 6:44 am on May 18, 2021:

Shouldn’t this be a batch to ensure an atomic operation? Otherwise you may end up with a corrupt muhash?

in src/index/coinstatsindex.cpp:344 in 5f96d7d22d

339+            return error("%s: Cannot read current %s state; index may be corrupted",
340+                         __func__, GetName());
341+        }
342+    }
343+
344+    if (BaseIndex::Init()) {

MarcoFalke commented at 6:54 am on May 18, 2021:

Could use early return if (!...) return false; to avoid large multi-line nesting

in src/index/coinstatsindex.cpp:125 in 5f96d7d22d

120+            return false;
121+        }
122+
123+        uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
124+        if (read_out.first != expected_block_hash) {
125+            if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {

MarcoFalke commented at 7:10 am on May 18, 2021:

Under what circumstance would this recovery condition be hit? Also, shouldn’t the error message mention that this condition failed as well?

fjahr commented at 6:13 pm on May 24, 2021:

This could happen if there is a reorg that the index has not picked up on yet, so the hash returned from the height index is not on the active chain anymore. I added a warning logging and changed the error() message to be more accurate.

in src/index/coinstatsindex.cpp:394 in 5f96d7d22d

389+            return false;
390+        }
391+
392+        uint256 expected_block_hash{pindex->pprev->GetBlockHash()};
393+        if (read_out.first != expected_block_hash) {
394+            if (!m_db->Read(DBHashKey(expected_block_hash), read_out)) {

MarcoFalke commented at 7:10 am on May 18, 2021:

Under what circumstance would this recovery condition be hit? Also, shouldn’t the error message mention that this condition failed as well?

in src/init.cpp:963 in 5f96d7d22d

955@@ -947,10 +956,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
956         nLocalServices = ServiceFlags(nLocalServices | NODE_COMPACT_FILTERS);
957     }
958 
959-    // if using block pruning, then disallow txindex
960+    // if using block pruning, then disallow txindex and coinstatsindex
961     if (args.GetArg("-prune", 0)) {
962         if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX))
963             return InitError(_("Prune mode is incompatible with -txindex."));
964+        if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX))

MarcoFalke commented at 7:12 am on May 18, 2021:

missing {} :sweat_smile:

fjahr commented at 6:08 pm on May 24, 2021:

Hopefully removed soon anyway with #21726 :)

in test/functional/feature_coinstatsindex.py:212 in 5f96d7d22d

207+        block = create_block(int(tip, 16), cb, block_time)
208+        block.solve()
209+        self.nodes[0].submitblock(ToHex(block))
210+        self.sync_all()
211+
212+        self.wait_until(lambda: not try_rpc(-32603, "Unable to read UTXO set", index_node.gettxoutsetinfo, 'muhash'))

MarcoFalke commented at 8:35 am on May 18, 2021:

why is this needed? (sync_all should call syncwithvalidationinterface). Also, if it didn’t, shouldn’t the index RPC call syncwithvalidationinterface internally by itself, similar to how the wallet does it?

fjahr commented at 6:11 pm on May 24, 2021:

True, I missed the option to use the BlockUntilSynced... function from BaseIndex.

in src/rpc/blockchain.cpp:1121 in 5f96d7d22d

1120                         {RPCResult::Type::STR_AMOUNT, "total_amount", "The total amount of coins in the UTXO set"},
1121+                        {RPCResult::Type::STR_AMOUNT, "total_unspendable_amount", "The total amount of coins permanently excluded from the UTXO set (only available if coinstatsindex is used)"},
1122+                        {RPCResult::Type::OBJ, "block_info", "Info on amounts in the block at this block height (only available if coinstatsindex is used)",
1123+                        {
1124+                            {RPCResult::Type::STR_AMOUNT, "prevout_spent", ""},
1125+                            {RPCResult::Type::STR_AMOUNT, "coinbase", ""},

MarcoFalke commented at 8:40 am on May 18, 2021:

Empty string as documentation? Would be nice to explain this a bit better. Does it include unspendable outputs …?

fjahr commented at 6:17 pm on May 24, 2021: member

Thanks @MarcoFalke , all comments should be addressed in #22047 aside from the init arg brackets which should be made redundant with #21726.

laanwj referenced this in commit 31fef69c03 on Jul 28, 2021

Fabcien referenced this in commit 7082554a0f on Mar 15, 2022

gwillen referenced this in commit c4182106af on Jun 1, 2022

Fabcien referenced this in commit 07d5e75fce on Jun 13, 2022

Fabcien referenced this in commit 6ac00d2ec5 on Jun 13, 2022

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

Fabcien referenced this in commit fab6ec9148 on Jun 13, 2022

Fabcien referenced this in commit 031b81d9f7 on Jun 13, 2022

Fabcien referenced this in commit 6b6d8c9d2c on Jun 13, 2022

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

Fabcien referenced this in commit a6842135a4 on Jun 13, 2022

Fabcien referenced this in commit 395ac40852 on Jun 13, 2022

Fabcien referenced this in commit 377ada0ea0 on Jun 13, 2022

Fabcien referenced this in commit ab529d9041 on Jun 13, 2022

Fabcien referenced this in commit 59770e7971 on Jun 13, 2022

DrahtBot locked this on Aug 16, 2022

Coinstats Index #19521

Conflicts