Skip to content

rpc: various fixups for dumptxoutset #23155

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 2, 2021
Merged

rpc: various fixups for dumptxoutset #23155

merged 1 commit into from
Dec 2, 2021

Conversation

jamesob
Copy link
Contributor

@jamesob jamesob commented Oct 1, 2021

This is part of the assumeutxo project (parent PR: #15606)

A few fixes to make this RPC actually useful when generating snapshots.

  • Generate an assumeutxo hash and display it (sort of a bugfix)
  • Add nchaintx to output (necessary for use in chainparams entry)
  • Add path of serialized UTXO file to output

benthecarman
benthecarman reviewed Oct 1, 2021
View reviewed changes
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 1, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23497 (Add 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.

@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch from 0b3ed75 to 1e161db Compare October 1, 2021 18:27
This was referenced Oct 1, 2021
Merged
Merged
benthecarman
benthecarman reviewed Oct 1, 2021
View reviewed changes
This was referenced Oct 2, 2021
Merged
Closed
@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch from 1e161db to 9b0c162 Compare October 26, 2021 13:11
@laanwj
Copy link
Member

laanwj commented Oct 27, 2021 

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

@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch 2 times, most recently from bda4d42 to 7d6a994 Compare October 27, 2021 14:46
@jamesob
Copy link
Contributor Author

jamesob commented Oct 29, 2021

needs to be updated for #22937

Fixed, thanks.

@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch 2 times, most recently from 5b542e5 to b637ffa Compare October 29, 2021 13:59
@DrahtBot DrahtBot mentioned this pull request Nov 1, 2021
Merged
@jamesob
Copy link
Contributor Author

jamesob commented Nov 3, 2021 •
edited
Loading

This change is functionally tested, should be uncontroversial, easy to review/merge, etc. if anyone has spare cycles.

maflcko
maflcko reviewed Nov 3, 2021
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Nov 13, 2021
Merged
ryanofsky
ryanofsky approved these changes Nov 16, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK b637ffa. Seems like there are some small fixes that could be made here, but this already does seem like an improvement.

@ryanofsky ryanofsky mentioned this pull request Nov 16, 2021
Merged
@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch from b637ffa to 1299ae9 Compare November 16, 2021 15:09
@jamesob
Copy link
Contributor Author

jamesob commented Nov 16, 2021

Addressed all feedback - thanks for the good reviews @MarcoFalke @ryanofsky.

maflcko
maflcko reviewed Nov 16, 2021
View reviewed changes
@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch 2 times, most recently from 6bf0fd2 to 7c27d38 Compare November 16, 2021 15:22
fanquake added a commit to bitcoin-core/gui that referenced this pull request Nov 17, 2021
@fanquake
Merge bitcoin/bitcoin#23522: Improve fs::PathToString documentation
b869a78 
9b575f1 Improve fs::PathToString documentation (Russell Yanofsky)

Pull request description:

  Add a developer note about avoiding `fs::PathToString` in RPCs, and improve some other `fs::PathToString` comments.

  Developer note might have been useful in two recent review comments:

  - bitcoin/bitcoin#23398 (comment)
  - bitcoin/bitcoin#23155 (comment)

ACKs for top commit:
  laanwj:
    Documentation review ACK 9b575f1
  jamesob:
    ACK bitcoin/bitcoin@9b575f1
  prayank23:
    ACK bitcoin/bitcoin@9b575f1
  hebasto:
    ACK 9b575f1
  shaavan:
    ACK 9b575f1

Tree-SHA512: b8b3ecb6208c3897241e4f24dcec64fe7cf091bc79388862cf5f4b315cb8e804939981c4bed4c81dbff99ec9f750bad99015d0f04890704ac9df63c2a6719b6d
ryanofsky
ryanofsky approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 7c27d38. Just a few suggested tweaks since last review for casting and argument passing and RPC documentation.

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 17, 2021
@fanquake
Merge bitcoin#23522: Improve fs::PathToString documentation
867659e 
9b575f1 Improve fs::PathToString documentation (Russell Yanofsky)

Pull request description:

  Add a developer note about avoiding `fs::PathToString` in RPCs, and improve some other `fs::PathToString` comments.

  Developer note might have been useful in two recent review comments:

  - bitcoin#23398 (comment)
  - bitcoin#23155 (comment)

ACKs for top commit:
  laanwj:
    Documentation review ACK 9b575f1
  jamesob:
    ACK bitcoin@9b575f1
  prayank23:
    ACK bitcoin@9b575f1
  hebasto:
    ACK 9b575f1
  shaavan:
    ACK 9b575f1

Tree-SHA512: b8b3ecb6208c3897241e4f24dcec64fe7cf091bc79388862cf5f4b315cb8e804939981c4bed4c81dbff99ec9f750bad99015d0f04890704ac9df63c2a6719b6d
@jamesob
rpc: various fixups for dumptxoutset
ffd0928 
- 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
@jamesob jamesob force-pushed the 2021-10-au-rpc-fixes branch from 7c27d38 to ffd0928 Compare November 30, 2021 16:19
@jamesob
Copy link
Contributor Author

jamesob commented Nov 30, 2021

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.

@laanwj
Copy link
Member

laanwj commented Dec 2, 2021

Code review ACK ffd0928

@laanwj laanwj merged commit 6acda4b into bitcoin:master Dec 2, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 2, 2021
@laanwj
Merge bitcoin#23155: rpc: various fixups for dumptxoutset
4a27cee
RandyMcMillan pushed a commit to RandyMcMillan/mempool-tab that referenced this pull request Dec 23, 2021
@laanwj
Merge bitcoin/bitcoin#23155: rpc: various fixups for dumptxoutset
2352ef8 
026443b rpc: various fixups for dumptxoutset (James O'Beirne)

Pull request description:

  This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606)

  ---

  A few fixes to make this RPC actually useful when generating snapshots.

  - Generate an assumeutxo hash and display it (sort of a bugfix)
  - Add nchaintx to output (necessary for use in chainparams entry)
  - Add path of serialized UTXO file to output

ACKs for top commit:
  laanwj:
    Code review ACK 026443b

Tree-SHA512: b0b5fd5138dea0e21258b1b18ab75bf3fd1628522cc1dbafa81af9cb9fa96562a1c39124fdb31057f256bfc560f462f907e9fe5e209b577b3f57afae2b7be826
@bitcoin bitcoin locked and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@maflcko maflcko maflcko left review comments

@benthecarman benthecarman benthecarman left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jamesob @DrahtBot @laanwj @maflcko @ryanofsky @benthecarman