Add gettxoutsetinfo hash_type option #19328

pull fjahr wants to merge 4 commits into bitcoin:master from fjahr:csi-5-hash_type-none changing 7 files +111 −18
  1. 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.

  2. refactor: Extract GetBogoSize function 605884ef21
  3. fjahr commented at 2:01 pm on June 19, 2020: member
    cc @Sjors
  4. MarcoFalke added the label RPC/REST/ZMQ on Jun 19, 2020
  5. MarcoFalke added the label Consensus on Jun 19, 2020
  6. 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
  7. 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
  8. 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
  9. 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
  10. 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
  11. 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 -

  12. 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).
  13. Sjors changes_requested
  14. Sjors commented at 5:33 pm on June 19, 2020: member
    Tested e884c98996b135eee51e5d3994ae604110c797fd on macOS 10.15.5
  15. 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.

  16. fjahr force-pushed on Jun 19, 2020
  17. 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.
  18. fjahr force-pushed on Jun 19, 2020
  19. 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.

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



    fjahr commented at 11:31 am on June 21, 2020:
    done
  21. 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.
    
  22. fjahr force-pushed on Jun 21, 2020
  23. fjahr commented at 11:32 am on June 21, 2020: member
    Fixed review comments plus some more formatting improvements
  24. 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
  25. 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
  26. rpc: gettxoutsetinfo can specify hash_type (only legacy option for now) a712cf6f68
  27. 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
  28. fjahr force-pushed on Jun 21, 2020
  29. fjahr commented at 11:12 pm on June 21, 2020: member
    Addressed comments and CI failure.
  30. rpc: Add hash_type NONE to gettxoutsetinfo f17a4d1c4d
  31. test: Test gettxouttsetinfo hash_type option 40506bf93f
  32. fjahr force-pushed on Jun 21, 2020
  33. 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
  34. Sjors approved
  35. Sjors commented at 6:56 pm on July 2, 2020: member
    tACK 40506bf93f955adfbc446c4d5fee4fa8bcfd7d9a
  36. 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
  37. 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 -

  38. 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.
  39. MarcoFalke merged this on Jul 6, 2020
  40. MarcoFalke closed this on Jul 6, 2020

  41. sidhujag referenced this in commit e98a4e94d1 on Jul 8, 2020
  42. luke-jr referenced this in commit 086eefe23d on Aug 15, 2020
  43. luke-jr referenced this in commit 39960cc054 on Aug 15, 2020
  44. luke-jr referenced this in commit ffeef4cb55 on Aug 15, 2020
  45. luke-jr referenced this in commit 7b948bd8aa on Aug 15, 2020
  46. luke-jr referenced this in commit 5982699246 on Aug 16, 2020
  47. luke-jr referenced this in commit 43a907a4c7 on Aug 16, 2020
  48. laanwj referenced this in commit 2b45cf0bcd on Apr 30, 2021
  49. Fabcien referenced this in commit 8ae513e386 on Aug 31, 2021
  50. deadalnix referenced this in commit 3d81fdd40f on Aug 31, 2021
  51. Fabcien referenced this in commit 352cf5ad6e on Aug 31, 2021
  52. Fabcien referenced this in commit c501ed2fa6 on Aug 31, 2021
  53. DrahtBot locked this on Feb 15, 2022

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-23 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me