Add gettxoutsetinfo hash_type option #19328

fjahr · 2020-06-19T14:01:39Z

This is another intermediate part of the Coinstats Index (tracked in #18000).

Sjors suggested here that the part of the changes in #19145 that don't rely on the new hash_type muhash, i.e. that are for hash_type=none, could be merged separately from everything involving muhash. So these changes are extracted from #19145 here and can be merged without any other requirements.

Building the index with no UTXO set hash is still valuable because gettxoutsetinfo can still be used to audit the total_amount for example. By itself this PR is not a huge improvement, hash_type=none is speeding up gettxoutsetinfo by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer.

fjahr · 2020-06-19T14:01:53Z

cc @Sjors

maflcko

review ACK e884c98 🚩

Show signature and timestamp

Signature:

Timestamp of file with hash 23084cd1f58a12c1391d066fc4f3cc18831bc7705d21198fe3a30252ff1f9f0c -

src/node/coinstats.cpp

src/rpc/client.cpp

src/rpc/blockchain.cpp

src/node/coinstats.cpp

Sjors

Tested e884c98 on macOS 10.15.5

src/rpc/client.cpp

src/rpc/blockchain.cpp

Sjors · 2020-06-19T17:18:33Z

src/rpc/blockchain.cpp

@@ -981,7 +983,7 @@ static UniValue gettxoutsetinfo(const JSONRPCRequest& request)
                        {RPCResult::Type::NUM, "transactions", "The number of transactions with unspent outputs"},
                        {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs"},
                        {RPCResult::Type::NUM, "bogosize", "A meaningless metric for UTXO set size"},
-                        {RPCResult::Type::STR_HEX, "hash_serialized_2", "The serialized hash"},
+                        {RPCResult::Type::STR_HEX, "hash_serialized_2", "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"},


Maybe open a separate PR to see if there's support for deprecating hash_serialized_2 and switch the default to NONE (pre-ACK, unless we find someone who actually uses this)

Hm, at least BTCPayServer uses it for FastSync: https://github.com/btcpayserver/btcpayserver-docker/tree/master/contrib/FastSync. They say in the docs to compare the whole gettxoutsetinfo but the hash is really what ensures there is nothing malicious going on. But I am not sure how much that feature is actually used. I will reach out to them to get an opinion.

Is there any benefit of aggressively deprecating this? With the option to select no hash there should be no downside to just leaving the default as is

Most people won't realise how slow this command is by default until they already issued it (though without an index is slow anyway).

DrahtBot · 2020-06-19T19:59:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

gui: Fix regression in *txoutset* in GUI console #19323 (gui: Fix regression in txoutset in GUI console by hebasto)
Add hash_type MUHASH for gettxoutsetinfo #19145 (Add hash_type MUHASH for gettxoutsetinfo by fjahr)
Make SER_GETHASH implicit for CHashWriter and SerializeHash #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.

src/node/coinstats.cpp

maflcko · 2020-06-20T13:48:00Z

test/fuzz/coins_view.cpp:281:23: error: no matching function for call to 'GetUTXOStats'

                (void)GetUTXOStats(&coins_view_cache, stats);

                      ^~~~~~~~~~~~

./node/coinstats.h:39:6: note: candidate function not viable: requires at least 3 arguments, but 2 were provided

bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, const CoinStatsHashType hash_type, const std::function<void()>& interruption_point = {});

     ^

1 error generated.

src/node/coinstats.cpp

fjahr · 2020-06-21T11:32:34Z

Fixed review comments plus some more formatting improvements

src/rpc/blockchain.cpp

src/node/coinstats.cpp

fjahr · 2020-06-21T23:12:00Z

Addressed comments and CI failure.

Sjors

tACK 40506bf

Sjors · 2020-07-02T18:55:37Z

src/node/coinstats.cpp

+static void ApplyStats(CCoinsStats& stats, std::nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
+{
+    assert(!outputs.empty());
+    stats.nTransactions++;


I'd like to avoid this duplication, but it can wait.

I now remove it in the follow-up when adding hash_type MUHASH https://github.com/bitcoin/bitcoin/pull/19145/files#diff-0ab7b981bd35c1a1f45b5ed75013a698

maflcko · 2020-07-02T20:20:21Z

Checked that 40506bf is faster with the none option on my arm machine. Haven't yet reviewed the code of that commit

maflcko · 2020-07-06T12:06:23Z

ACK 40506bf 🖨

Show signature and timestamp

Signature:

Timestamp of file with hash 4ce439fee8be758f1a7c060666d9c236d07dd151ec5a09fed5df26d9df431ae9 -

maflcko · 2020-07-06T12:08:01Z

src/rpc/util.cpp

+{
+    if (param.isNull()) {
+        return default_type;
+    } else {


style-nit: No need for the else and all the whitespace padding below

Suggested change

} else {

}

const std::str...

Thanks, now fixed in #19145.

40506bf test: Test gettxouttsetinfo hash_type option (Fabian Jahr) f17a4d1 rpc: Add hash_type NONE to gettxoutsetinfo (Fabian Jahr) a712cf6 rpc: gettxoutsetinfo can specify hash_type (only legacy option for now) (Fabian Jahr) 605884e refactor: Extract GetBogoSize function (Fabian Jahr) Pull request description: This is another intermediate part of the Coinstats Index (tracked in bitcoin#18000). Sjors suggested [here](bitcoin#18000 (comment)) that the part of the changes in bitcoin#19145 that don't rely on the new `hash_type` muhash, i.e. that are for `hash_type=none`, could be merged separately from everything involving muhash. So these changes are extracted from bitcoin#19145 here and can be merged without any other requirements. Building the index with no UTXO set hash is still valuable because `gettxoutsetinfo` can still be used to audit the `total_amount` for example. By itself this PR is not a huge improvement, `hash_type=none` is speeding up `gettxoutsetinfo` by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer. ACKs for top commit: MarcoFalke: ACK 40506bf 🖨 Sjors: tACK 40506bf Tree-SHA512: 3964c2b8eed427511b1aa9b2ef285dff27dc4d1537d72c3911e435b6e6b40912232da4acb3a09bd19a0372ddffa44103388d8a650169d95a4a727b970d210add

Github-Pull: bitcoin#19328 Rebased-From: 605884e

Github-Pull: bitcoin#19328 Rebased-From: a712cf6

Github-Pull: bitcoin#19328 Rebased-From: f17a4d1

Github-Pull: bitcoin#19328 Rebased-From: 40506bf

Github-Pull: bitcoin#19328 Rebased-From: f17a4d1

Github-Pull: bitcoin#19328 Rebased-From: 40506bf

5f96d7d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr) 23fe504 test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr) 90c966b rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr) b936239 index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr) bb7788b test: Test coinstatsindex robustness across restarts (Fabian Jahr) e0938c2 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr) 2501576 rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr) 655d929 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack) ca01bb8 rpc: Add Coinstats index to getindexinfo (Fabian Jahr) 57a026c test: Add unit test for Coinstats index (Fabian Jahr) 6a4c0c0 test: Add functional test for Coinstats index (Fabian Jahr) 3f166ec rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr) 3c914d5 index: Coinstats index can be activated with command line flag (Fabian Jahr) dd58a4d index: Add Coinstats index (Fabian Jahr) a8a46c4 refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr) 9c8a265 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr) 2e2648a crypto: Make MuHash Remove method efficient (Fabian Jahr) Pull request description: 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 ACKs for top commit: Sjors: re-tACK 5f96d7d jonatack: Code review re-ACK 5f96d7d per `git range-diff 13d27b4 07201d3 5f96d7d` promag: Tested ACK 5f96d7d. Light code review ACK 5f96d7d. Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2

Summary: > This is another intermediate part of the Coinstats Index (tracked in [[bitcoin/bitcoin#18000 | core#18000]]). > > Sjors suggested here that the part of the changes in #19145 that don't rely on the new hash_type muhash, i.e. that are for hash_type=none, could be merged separately from everything involving muhash. So these changes are extracted from #19145 here and can be merged without any other requirements. > > Building the index with no UTXO set hash is still valuable because gettxoutsetinfo can still be used to audit the total_amount for example. By itself this PR is not a huge improvement, hash_type=none is speeding up gettxoutsetinfo by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer. This is a backport of [[bitcoin/bitcoin#19328 | core#19328]] [1/4] bitcoin/bitcoin@605884e Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9978

Summary: This is a backport of [[bitcoin/bitcoin#19328 | core#19328]] [2/4] bitcoin/bitcoin@a712cf6 Backport note: D507 kept the name `hash_serialized` instead of using `hash_serialized_2` which was introduced in [[bitcoin/bitcoin#10426 | core#10426]] Depends on D9978 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9979

Summary: This is a backport of [[bitcoin/bitcoin#19328 | core#19328]] [3/4] bitcoin/bitcoin@f17a4d1 Depends on D9979 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9980

Summary: This is a backport of [[bitcoin/bitcoin#19328 | core#19328]] [4/4] bitcoin/bitcoin@40506bf Depends on D9980 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9981

refactor: Extract GetBogoSize function

~~605884e~~

fjahr mentioned this pull request Jun 19, 2020

[WIP] Index for UTXO Set Statistics #18000

Closed

maflcko added RPC/REST/ZMQ Consensus labels Jun 19, 2020

maflcko reviewed Jun 19, 2020

View reviewed changes

Sjors suggested changes Jun 19, 2020

View reviewed changes

This was referenced Jun 19, 2020

gui: Fix regression in *txoutset* in GUI console #19323

Merged

Add hash_type MUHASH for gettxoutsetinfo #19145

Merged

fjahr force-pushed the csi-5-hash_type-none branch from e884c98 to 0ca3ad1 Compare June 19, 2020 22:29

maflcko reviewed Jun 19, 2020

View reviewed changes

src/node/coinstats.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the csi-5-hash_type-none branch from 0ca3ad1 to 199b5a4 Compare June 19, 2020 22:56

DrahtBot mentioned this pull request Jun 20, 2020

Make SER_GETHASH implicit for CHashWriter and SerializeHash #13462

Closed

promag reviewed Jun 20, 2020

View reviewed changes

src/node/coinstats.cpp Outdated Show resolved Hide resolved

src/node/coinstats.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the csi-5-hash_type-none branch from 199b5a4 to 0c90ff2 Compare June 21, 2020 11:31

promag reviewed Jun 21, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

promag reviewed Jun 21, 2020

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

maflcko reviewed Jun 21, 2020

View reviewed changes

src/node/coinstats.cpp Outdated Show resolved Hide resolved

rpc: gettxoutsetinfo can specify hash_type (only legacy option for now)

a712cf6

fjahr force-pushed the csi-5-hash_type-none branch from 0c90ff2 to bd062da Compare June 21, 2020 23:09

fjahr added 2 commits June 22, 2020 01:55

rpc: Add hash_type NONE to gettxoutsetinfo

f17a4d1

test: Test gettxouttsetinfo hash_type option

40506bf

fjahr force-pushed the csi-5-hash_type-none branch from bd062da to 40506bf Compare June 21, 2020 23:57

fjahr mentioned this pull request Jun 27, 2020

Future PRs bitcoin-core-review-club/website#14

Closed

Sjors approved these changes Jul 2, 2020

View reviewed changes

maflcko reviewed Jul 6, 2020

View reviewed changes

maflcko merged commit b52e25c into bitcoin:master Jul 6, 2020

fjahr mentioned this pull request Jul 14, 2020

Coinstats Index #19521

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020

refactor: Extract GetBogoSize function

086eefe

Github-Pull: bitcoin#19328 Rebased-From: 605884e

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020

rpc: gettxoutsetinfo can specify hash_type (only legacy option for now)

39960cc

Github-Pull: bitcoin#19328 Rebased-From: a712cf6

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020

rpc: Add hash_type NONE to gettxoutsetinfo

ffeef4c

Github-Pull: bitcoin#19328 Rebased-From: f17a4d1

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020

test: Test gettxouttsetinfo hash_type option

7b948bd

Github-Pull: bitcoin#19328 Rebased-From: 40506bf

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 16, 2020

rpc: Add hash_type NONE to gettxoutsetinfo

5982699

Github-Pull: bitcoin#19328 Rebased-From: f17a4d1

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 16, 2020

test: Test gettxouttsetinfo hash_type option

43a907a

Github-Pull: bitcoin#19328 Rebased-From: 40506bf

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add gettxoutsetinfo hash_type option #19328

Add gettxoutsetinfo hash_type option #19328

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add gettxoutsetinfo hash_type option #19328

Add gettxoutsetinfo hash_type option #19328

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!