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?
Yeah, good point - thought I had done it for consistency with something else but I guess not.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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')
looks like path is already being tested above on line 36
Fixed, thanks.
./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}]’
./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>]’
./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>]’
rpc/blockchain.cpp:2623:5: required from here
./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>]’
formatValue(out, fmtBegin, fmtEnd, ntrunc, *static_cast<const T*>(value));
~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./rpc/blockchain.h:10,
from rpc/blockchain.cpp:6:
./fs.h:233:24: note: declared here
template<> inline void formatValue(std::ostream&, const char*, const char*, int, const fs::path&) = delete;
^~~~~~~~~~~
needs to be updated for #22937
This change is functionally tested, should be uncontroversial, easy to review/merge, etc. if anyone has spare cycles.
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:
{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));
pretty sure this will overflow soon, if it doesn't already
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.
Code review ACK b637ffaad40ea8707bdb27334990d2bcac28cf53. Seems like there are some small fixes that could be made here, but this already does seem like an improvement.
Addressed all feedback - thanks for the good reviews @MarcoFalke @ryanofsky.
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.
result.pushKV("nchaintx", int64_t{tip->nChainTx});
rpc/blockchain.cpp:2654:12: error: call to member function 'pushKV' is ambiguous
result.pushKV("nchaintx", tip->nChainTx);
~~~~~~~^~~~~~
./univalue/include/univalue.h:125:10: note: candidate function
bool pushKV(const std::string& key, int64_t val_) {
^
./univalue/include/univalue.h:129:10: note: candidate function
bool pushKV(const std::string& key, uint64_t val_) {
^
./univalue/include/univalue.h:133:10: note: candidate function
bool pushKV(const std::string& key, bool val_) {
^
./univalue/include/univalue.h:137:10: note: candidate function
bool pushKV(const std::string& key, int val_) {
^
./univalue/include/univalue.h:141:10: note: candidate function
bool pushKV(const std::string& key, double val_) {
^
1 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"
Fixed, thanks
Code review ACK 7c27d38ccc89d49a7271b3d9a43159b34f8baed1. Just a few suggested tweaks since last review for casting and argument passing and RPC documentation.
- 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.
Code review ACK ffd09281fe26446fcefa0627c220a52706e35227