This is part of the assumeutxo project (parent PR: #15606)
A few fixes to make this RPC actually useful when generating snapshots.
2531@@ -2530,6 +2532,8 @@ static RPCHelpMan dumptxoutset()
2532 {RPCResult::Type::STR_HEX, "base_hash", "the hash of the base of the snapshot"},
2533 {RPCResult::Type::NUM, "base_height", "the height of the base of the snapshot"},
2534 {RPCResult::Type::STR, "path", "the absolute path that the snapshot was written to"},
2535+ {RPCResult::Type::STR_HEX, "assumeutxo", "the hash of the UTXO set contents"},
assumeutxo seems like a weird name for this result, why not something like txoutset_hash?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)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.
44@@ -45,6 +45,11 @@ def run_test(self):
45 assert_equal(
46 digest, '7ae82c986fa5445678d2a21453bb1c86d39e47af13da137640c2b1cf8093691c')
47
48+ assert_equal(
49+ out['txoutset_hash'], 'd4b614f476b99a6e569973bf1c0120d88b1a168076f8ce25691fb41dd1cef149')
50+ assert_equal(out['nchaintx'], 101)
51+ assert out['path'].endswith('txoutset.dat')
path is already being tested above on line 36
0./tinyformat.h:1028:20: required from ‘tinyformat::detail::FormatListN<sizeof... (Args)> tinyformat::makeFormatList(const Args& ...) [with Args = {int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fs::path, fs::path}]’
1./tinyformat.h:1064:37: required from ‘void tinyformat::format(std::ostream&, const char*, const Args& ...) [with Args = {int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fs::path, fs::path}; std::ostream = std::basic_ostream<char>]’
2./tinyformat.h:1073:11: required from ‘std::__cxx11::string tinyformat::format(const char*, const Args& ...) [with Args = {int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fs::path, fs::path}; std::__cxx11::string = std::__cxx11::basic_string<char>]’
3rpc/blockchain.cpp:2623:5: required from here
4./tinyformat.h:543:24: error: use of deleted function ‘void tinyformat::formatValue(std::ostream&, const char*, const char*, int, const T&) [with T = fs::path; std::ostream = std::basic_ostream<char>]’
5 formatValue(out, fmtBegin, fmtEnd, ntrunc, *static_cast<const T*>(value));
6 ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
7In file included from ./rpc/blockchain.h:10,
8 from rpc/blockchain.cpp:6:
9./fs.h:233:24: note: declared here
10 template<> inline void formatValue(std::ostream&, const char*, const char*, int, const fs::path&) = delete;
11 ^~~~~~~~~~~
needs to be updated for #22937
2548@@ -2547,6 +2549,8 @@ static RPCHelpMan dumptxoutset()
2549 {RPCResult::Type::STR_HEX, "base_hash", "the hash of the base of the snapshot"},
2550 {RPCResult::Type::NUM, "base_height", "the height of the base of the snapshot"},
2551 {RPCResult::Type::STR, "path", "the absolute path that the snapshot was written to"},
2552+ {RPCResult::Type::STR_HEX, "txoutset_hash", "the hash of the UTXO set contents"},
2553+ {RPCResult::Type::NUM, "nchaintx", "the nchaintx value for the base block"},
nit, could explain what “nchaintx value” means instead:
0 {RPCResult::Type::NUM, "num_chain_txs", "the number of transactions in the chain of the base block"},
2648@@ -2635,7 +2649,9 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil
2649 result.pushKV("coins_written", stats.coins_count);
2650 result.pushKV("base_hash", tip->GetBlockHash().ToString());
2651 result.pushKV("base_height", tip->nHeight);
2652-
2653+ result.pushKV("path", fs::PathToString(path));
2654+ result.pushKV("txoutset_hash", stats.hashSerialized.ToString());
2655+ result.pushKV("nchaintx", static_cast<int>(tip->nChainTx));
re: #23155 (review)
pretty sure this will overflow soon, if it doesn’t already
Seems worth addressing. Should this just cast to int64 or is there a more ideal fix?
2648@@ -2635,7 +2649,9 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil
2649 result.pushKV("coins_written", stats.coins_count);
2650 result.pushKV("base_hash", tip->GetBlockHash().ToString());
2651 result.pushKV("base_height", tip->nHeight);
2652-
2653+ result.pushKV("path", fs::PathToString(path));
In commit “rpc: various fixups for dumptxoutset” (b637ffaad40ea8707bdb27334990d2bcac28cf53)
Should use path.u8string() here not fs::PathToString(path) because JSON strings (unlike log strings) need to be UTF-8
70+UniValue CreateUTXOSnapshot(
71+ NodeContext& node,
72+ CChainState& chainstate,
73+ CAutoFile& afile,
74+ const fs::path path,
75+ const fs::path tmppath);
In commit “rpc: various fixups for dumptxoutset” (b637ffaad40ea8707bdb27334990d2bcac28cf53)
Minor: Should use either const fs::path & or non-const fs::path with std::move to avoid creating unneeded temporary copies. const path just combines worst of both approaches.
2648@@ -2635,7 +2649,9 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil
2649 result.pushKV("coins_written", stats.coins_count);
2650 result.pushKV("base_hash", tip->GetBlockHash().ToString());
2651 result.pushKV("base_height", tip->nHeight);
2652-
2653+ result.pushKV("path", path.u8string());
2654+ result.pushKV("txoutset_hash", stats.hashSerialized.ToString());
2655+ result.pushKV("nchaintx", static_cast<int64_t>(tip->nChainTx));
Can you explain why a cast is needed? If one is needed, it might be better to use a non-narrowing.
0 result.pushKV("nchaintx", int64_t{tip->nChainTx});
0rpc/blockchain.cpp:2654:12: error: call to member function 'pushKV' is ambiguous
1 result.pushKV("nchaintx", tip->nChainTx);
2 ~~~~~~~^~~~~~
3./univalue/include/univalue.h:125:10: note: candidate function
4 bool pushKV(const std::string& key, int64_t val_) {
5 ^
6./univalue/include/univalue.h:129:10: note: candidate function
7 bool pushKV(const std::string& key, uint64_t val_) {
8 ^
9./univalue/include/univalue.h:133:10: note: candidate function
10 bool pushKV(const std::string& key, bool val_) {
11 ^
12./univalue/include/univalue.h:137:10: note: candidate function
13 bool pushKV(const std::string& key, int val_) {
14 ^
15./univalue/include/univalue.h:141:10: note: candidate function
16 bool pushKV(const std::string& key, double val_) {
17 ^
181 error generated.
I’ll change the cast as you specify.
2548@@ -2547,6 +2549,8 @@ static RPCHelpMan dumptxoutset()
2549 {RPCResult::Type::STR_HEX, "base_hash", "the hash of the base of the snapshot"},
2550 {RPCResult::Type::NUM, "base_height", "the height of the base of the snapshot"},
2551 {RPCResult::Type::STR, "path", "the absolute path that the snapshot was written to"},
2552+ {RPCResult::Type::STR_HEX, "txoutset_hash", "the hash of the UTXO set contents"},
2553+ {RPCResult::Type::NUM, "nchaintx", "the number of transactions in the chain up to the base block"},
In commit “rpc: various fixups for dumptxoutset” (7c27d38ccc89d49a7271b3d9a43159b34f8baed1)
This is a little different than what marco suggested #23155 (review) because it is keeping the “nchaintx” name and saying “chain up to the base block” instead of “chain of the base block”. I think “up to” seems a little misleading or ambiguous because it sounds like it excludes the number of transactions actually in the block. It might be better to use Marco’s wording or say explicitly “up to and including” or “up to but not including”
- Actually generate an assumeutxo hash and display it
- Add nchaintx to output (necessary for use in chainparams entry)
- Add path of serialized UTXO file to output
Pushed a small doc fix based on Marco, Russ’ feedback.
I think this small change is easily reviewable and could be merged without much risk.