Add gettxoutsetinfo hash

fjahr commented at 2:01 pm on June 19, 2020: member

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.

refactor: Extract GetBogoSize function 605884ef21

fjahr commented at 2:01 pm on June 19, 2020: member

cc @Sjors

MarcoFalke added the label RPC/REST/ZMQ on Jun 19, 2020

MarcoFalke added the label Consensus on Jun 19, 2020

in src/node/coinstats.cpp:46 in 87dd07e038 outdated

41@@ -42,19 +42,21 @@ static void ApplyStats(CCoinsStats &stats, CHashWriter& ss, const uint256& hash,
42 }
43 
44 //! Calculate statistics about the unspent transaction output set
45-bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, const std::function<void()>& interruption_point)
46+template <typename T>
47+bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point)

MarcoFalke commented at 4:43 pm on June 19, 2020:

nit in commit 87dd07e0382c6398d118d88c88b0c9ef24251e37

0static bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, T hash_obj, const std::function<void()>& interruption_point)

fjahr commented at 10:29 pm on June 19, 2020:

done

in src/node/coinstats.cpp:97 in 87dd07e038 outdated

94+        case(CoinStatsHashType::HASH_SERIALIZED): {
95+            CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION);
96+            return GetUTXOStats(view, stats, ss, interruption_point);
97+        }
98+    }
99+    return false; // Unreachable code

MarcoFalke commented at 4:44 pm on June 19, 2020:

87dd07e0382c6398d118d88c88b0c9ef24251e37:

According to the developer notes, assert(false) and with the appropriate comment

fjahr commented at 10:29 pm on June 19, 2020:

done

in src/rpc/client.cpp:126 in 87dd07e038 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", 0, "hash_type" },

MarcoFalke commented at 4:45 pm on June 19, 2020:

87dd07e0382c6398d118d88c88b0c9ef24251e37

why is this needed? Looks like a str to me

Sjors commented at 5:04 pm on June 19, 2020:

Indeed, this should be dropped (try: bitcoin-cli gettxoutsetinfo none, it works without this line).

fjahr commented at 10:30 pm on June 19, 2020:

done

in src/rpc/blockchain.cpp:2334 in 87dd07e038 outdated

2330@@ -2316,7 +2331,7 @@ UniValue dumptxoutset(const JSONRPCRequest& request)
2331 
2332         ::ChainstateActive().ForceFlushStateToDisk();
2333 
2334-        if (!GetUTXOStats(&::ChainstateActive().CoinsDB(), stats, RpcInterruptionPoint)) {
2335+        if (!GetUTXOStats(&::ChainstateActive().CoinsDB(), stats, CoinStatsHashType::HASH_SERIALIZED, RpcInterruptionPoint)) {

MarcoFalke commented at 4:47 pm on June 19, 2020:

in commit ea41a5bdcc49566be0c23ef9362781e70f557bad:

why is this not changed to NONE?

Sjors commented at 5:15 pm on June 19, 2020:

Agreed, this hash is unused in UTXO snapshots, as judged by the code as well as the hardcoded UTXO set has in rpc_dumptxoutset.py.

fjahr commented at 10:30 pm on June 19, 2020:

done

in src/node/coinstats.cpp:44 in ea41a5bdcc outdated

40@@ -41,6 +41,17 @@ static void ApplyStats(CCoinsStats &stats, CHashWriter& ss, const uint256& hash,
41     ss << VARINT(0u);
42 }
43 
44+static void ApplyStats(CCoinsStats &stats, nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)

MarcoFalke commented at 4:51 pm on June 19, 2020:

nit ea41a5bdcc49566be0c23ef9362781e70f557bad

0static void ApplyStats(CCoinsStats& stats, nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)

fjahr commented at 10:30 pm on June 19, 2020:

done

MarcoFalke commented at 4:51 pm on June 19, 2020: member

review ACK e884c98996b135eee51e5d3994ae604110c797fd 🚩

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK e884c98996b135eee51e5d3994ae604110c797fd 🚩
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhpPgv/XaXojJucDooSWoRbsa/hlk3xG9yYOVK6BisILFXd1xNoo2pBnGwuSX2L
 8lq2h19aOP4Zk/iYOzPwdP8UAOaDBoFJ9pAQwo0YYuHu2lIFLlmjFScJMd1Y3OUhl
 9+d6SAYSsc6lQ9QxpaMwocZ4V6X3JrFcXBK0vCscF2wm/bBVCOwBo/x0sSrPFyO9M
10dAnaJIbth+P6NVu/jNKtigPK0hTal2rTqfxB1/RZbnoJw3yugWLPDsF+/FiBp5+Y
11g2Y/uSTBp25RS/vKN6+L0qJfuiJGS/ixwUMpD/kDiY7r3hqIlpn8ZHdyxCRRYZgp
12OfOC9ZicTgO2EdNrOepxrIwWbnrtWXyPo/AJlMqadk/8MtqNzQ+tivMfapu8gPUi
13WzI7W5nYv8ZFTX85rz8nwhDtaxKliIgkkui8rEhpgMESpPYWx5X6443C9ZmRYV0X
14XDEXWaLnBNaX5a8dE+uTpzlODjiY8PIgBTpoAk+7qfzO8YpfGKiOZP7++XISEEwf
15ZBDVe5go
16=SITy
17-----END PGP SIGNATURE-----

Timestamp of file with hash 23084cd1f58a12c1391d066fc4f3cc18831bc7705d21198fe3a30252ff1f9f0c -

in src/rpc/blockchain.cpp:986 in e884c98996 outdated

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

Sjors commented at 5:18 pm on June 19, 2020:

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)

fjahr commented at 10:41 pm on June 19, 2020:

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.

MarcoFalke commented at 10:53 pm on June 19, 2020:

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

Sjors commented at 10:03 am on June 30, 2020:

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

Sjors changes_requested

Sjors commented at 5:33 pm on June 19, 2020: member

Tested e884c98996b135eee51e5d3994ae604110c797fd on macOS 10.15.5

DrahtBot commented at 7:59 pm on June 19, 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:

#19323 (gui: Fix regression in txoutset in GUI console by hebasto)
#19145 (Add hash_type MUHASH for gettxoutsetinfo by fjahr)
#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 Jun 19, 2020

in src/node/coinstats.cpp:111 in 0ca3ad17f3 outdated

108+        }
109+        case(CoinStatsHashType::NONE): {
110+            return GetUTXOStats(view, stats, nullptr, interruption_point);
111+        }
112+    }
113+    assert(false); // Unreachable code

MarcoFalke commented at 10:33 pm on June 19, 2020:

Should be identical to the other cases to make it grep-able. See dev-notes example:

 0enum class Tabs {
 1    INFO,
 2    CONSOLE,
 3    GRAPH,
 4    PEERS
 5};
 6
 7int GetInt(Tabs tab)
 8{
 9    switch (tab) {
10    case Tabs::INFO: return 0;
11    case Tabs::CONSOLE: return 1;
12    case Tabs::GRAPH: return 2;
13    case Tabs::PEERS: return 3;
14    } // no default case, so the compiler can warn about missing cases
15    assert(false);
16}

0    assert(false);

fjahr commented at 10:56 pm on June 19, 2020:

Ok, thanks. Fixed.

fjahr force-pushed on Jun 19, 2020

in src/node/coinstats.cpp:89 in ed4c730229 outdated

86+
87     stats.nDiskSize = view->EstimateSize();
88     return true;
89 }
90+
91+bool GetUTXOStats(CCoinsView *view, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function<void()>& interruption_point)

promag commented at 1:40 pm on June 20, 2020:

ed4c7302298385b797000740960b06ff91082ce8

nit * view.

in src/node/coinstats.cpp:92 in ed4c730229 outdated

89 }
90+
91+bool GetUTXOStats(CCoinsView *view, CCoinsStats& stats, CoinStatsHashType hash_type, const std::function<void()>& interruption_point)
92+{
93+    switch(hash_type) {
94+        case(CoinStatsHashType::HASH_SERIALIZED): {

promag commented at 1:46 pm on June 20, 2020:

ed4c7302298385b797000740960b06ff91082ce8

nit, this format is not used. Also add space after switch above.

MarcoFalke commented at 2:58 pm on June 20, 2020:

;) https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

fjahr commented at 11:31 am on June 21, 2020:

done

MarcoFalke commented at 1:48 pm on June 20, 2020: member

 0test/fuzz/coins_view.cpp:281:23: error: no matching function for call to 'GetUTXOStats'
 1
 2                (void)GetUTXOStats(&coins_view_cache, stats);
 3
 4                      ^~~~~~~~~~~~
 5
 6./node/coinstats.h:39:6: note: candidate function not viable: requires at least 3 arguments, but 2 were provided
 7
 8bool GetUTXOStats(CCoinsView* view, CCoinsStats& stats, const CoinStatsHashType hash_type, const std::function<void()>& interruption_point = {});
 9
10     ^
11
121 error generated.

fjahr force-pushed on Jun 21, 2020

fjahr commented at 11:32 am on June 21, 2020: member

Fixed review comments plus some more formatting improvements

in src/rpc/blockchain.cpp:1001 in 0c90ff28e7 outdated

 997@@ -996,14 +998,29 @@ static UniValue gettxoutsetinfo(const JSONRPCRequest& request)
 998     CCoinsStats stats;
 999     ::ChainstateActive().ForceFlushStateToDisk();
1000 
1001+    CoinStatsHashType hash_type = CoinStatsHashType::HASH_SERIALIZED;

promag commented at 11:49 am on June 21, 2020:

nit, move code below to new function and here make it const CoinStatsHashType hash_type = ParseHashType(request.params[0], CoinStatsHashType::HASH_SERIALIZED) (2nd is default value if 1st is null).

fjahr commented at 11:09 pm on June 21, 2020:

done

in src/rpc/blockchain.cpp:1006 in 0c90ff28e7 outdated

 997@@ -996,14 +998,29 @@ static UniValue gettxoutsetinfo(const JSONRPCRequest& request)
 998     CCoinsStats stats;
 999     ::ChainstateActive().ForceFlushStateToDisk();
1000 
1001+    CoinStatsHashType hash_type = CoinStatsHashType::HASH_SERIALIZED;
1002+    if (!request.params[0].isNull()) {
1003+        std::string hash_type_input = request.params[0].get_str();
1004+
1005+        if (hash_type_input == "hash_serialized_2") {
1006+            // keep default

promag commented at 11:52 am on June 21, 2020:

nit, I’d explicitly set hash_type = CoinStatsHashType::HASH_SERIALIZED.

fjahr commented at 11:09 pm on June 21, 2020:

done

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

in src/node/coinstats.cpp:44 in 0c90ff28e7 outdated

41+        stats.nBogoSize += GetBogoSize(output.second.out.scriptPubKey);
42     }
43     ss << VARINT(0u);
44 }
45 
46+static void ApplyStats(CCoinsStats& stats, nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)

MarcoFalke commented at 12:51 pm on June 21, 2020:

 0node/coinstats.cpp:44:44: error: unknown type name 'nullptr_t'; did you mean 'std::nullptr_t'?
 1
 2static void ApplyStats(CCoinsStats& stats, nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
 3
 4                                           ^~~~~~~~~
 5
 6                                           std::nullptr_t
 7
 8/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/x86_64-linux-gnu/c++/9/bits/c++config.h:258:29: note: 'std::nullptr_t' declared here
 9
10  typedef decltype(nullptr)     nullptr_t;
11
12                                ^
13
14node/coinstats.cpp:119:25: error: use of undeclared identifier 'nullptr_t'; did you mean 'nullptr'?
15
16static void PrepareHash(nullptr_t, CCoinsStats& stats) {}
17
18                        ^~~~~~~~~
19
20                        nullptr

fjahr commented at 11:12 pm on June 21, 2020:

fixed

fjahr force-pushed on Jun 21, 2020

fjahr commented at 11:12 pm on June 21, 2020: member

Addressed comments and CI failure.

rpc: Add hash_type NONE to gettxoutsetinfo f17a4d1c4d

test: Test gettxouttsetinfo hash_type option 40506bf93f

fjahr force-pushed on Jun 21, 2020

in src/node/coinstats.cpp:47 in f17a4d1c4d outdated

40@@ -41,6 +41,17 @@ static void ApplyStats(CCoinsStats& stats, CHashWriter& ss, const uint256& hash,
41     ss << VARINT(0u);
42 }
43 
44+static void ApplyStats(CCoinsStats& stats, std::nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
45+{
46+    assert(!outputs.empty());
47+    stats.nTransactions++;

Sjors commented at 6:55 pm on July 2, 2020:

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

fjahr commented at 11:51 am on July 5, 2020:

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

Sjors approved

Sjors commented at 6:56 pm on July 2, 2020: member

tACK 40506bf93f955adfbc446c4d5fee4fa8bcfd7d9a

MarcoFalke commented at 8:20 pm on July 2, 2020: member

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

MarcoFalke commented at 12:06 pm on July 6, 2020: member

ACK 40506bf93f955adfbc446c4d5fee4fa8bcfd7d9a 🖨

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 40506bf93f955adfbc446c4d5fee4fa8bcfd7d9a 🖨
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiC+Qv/UfSTrJYS3nGpNjOzD1xGJWB/hTnUlgwgMIm4UkrnJw5tpLgSxniPsa7J
 88sK5CaKfpLKpvXjW+sFMcadgHX8IeZlFfvlDev8Bc6T0RrGh/61qgjwKqQUKtAh+
 9zv+v+QnvkXInpgTYK3oLF7UvtBqEqwpRi04e6m23eLaDcdMC03iBAjYPd+/4WbFV
10+wQdWyZTn52Fe3IKR6+QcyQXXZS4lgUYd1K0N90w1nfB1kVx7C04KMn3Ubj6FxOb
11kZtIdYLciOCPodgy5idcZGHHEcEg88z3lhXp5JqQmh96dhDf9ehsclgl5ycFVl8I
12Iu8BXhHs/OwYg+sy/9IDvdN6uwzknwIv1E9ueSe5fS0Xh8W6KLOaEIa7iAz4OPlQ
13fvseGpqHfuuGv8QHK+3e4+GorghSLM2hOyUure5y+l64dWa5BQeubc2FXHFupyf8
14pAYWjmUjSnk240Fb4nqRqp3j6iu7joHWFKK5FSOCofZ6pLlZ7cIDluKXsYYqaDJq
15p1ypj/n5
16=eqXn
17-----END PGP SIGNATURE-----

Timestamp of file with hash 4ce439fee8be758f1a7c060666d9c236d07dd151ec5a09fed5df26d9df431ae9 -

in src/rpc/util.cpp:120 in 40506bf93f

112@@ -113,6 +113,23 @@ std::vector<unsigned char> ParseHexO(const UniValue& o, std::string strKey)
113     return ParseHexV(find_value(o, strKey), strKey);
114 }
115 
116+CoinStatsHashType ParseHashType(const UniValue& param, const CoinStatsHashType default_type)
117+{
118+    if (param.isNull()) {
119+        return default_type;
120+    } else {

MarcoFalke commented at 12:08 pm on July 6, 2020:

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

0    }
1    const std::str...

fjahr commented at 1:59 pm on July 6, 2020:

Thanks, now fixed in #19145.

MarcoFalke merged this on Jul 6, 2020

MarcoFalke closed this on Jul 6, 2020

sidhujag referenced this in commit e98a4e94d1 on Jul 8, 2020

luke-jr referenced this in commit 086eefe23d on Aug 15, 2020

luke-jr referenced this in commit 39960cc054 on Aug 15, 2020

luke-jr referenced this in commit ffeef4cb55 on Aug 15, 2020

luke-jr referenced this in commit 7b948bd8aa on Aug 15, 2020

luke-jr referenced this in commit 5982699246 on Aug 16, 2020

luke-jr referenced this in commit 43a907a4c7 on Aug 16, 2020

laanwj referenced this in commit 2b45cf0bcd on Apr 30, 2021

Fabcien referenced this in commit 8ae513e386 on Aug 31, 2021

deadalnix referenced this in commit 3d81fdd40f on Aug 31, 2021

Fabcien referenced this in commit 352cf5ad6e on Aug 31, 2021

Fabcien referenced this in commit c501ed2fa6 on Aug 31, 2021

DrahtBot locked this on Feb 15, 2022

Add gettxoutsetinfo hash_type option #19328

Conflicts