Skip to content

cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups #19089

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
Closed
jonatack wants to merge 7 commits into from
Closed

cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups #19089

jonatack wants to merge 7 commits into from

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented May 28, 2020 •
edited
Loading

These are follow-ups to #18594:

  • release note for -getinfo multiwallet balances

  • update the -getinfo help:

$ bitcoin-cli -h | grep -A3 getinfo
  -getinfo
       Get general information from the remote server, including the balances
       of each loaded wallet when in multiwallet mode. Note that
       -getinfo is the combined result of several RPCs (getnetworkinfo,
       getblockchaininfo, getwalletinfo, getbalances, and in multiwallet
       mode, listwallets), each with potentially different state.

@jonatack jonatack mentioned this pull request May 28, 2020
Merged
maflcko
maflcko reviewed May 28, 2020
View reviewed changes
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from daab3ce to 8ab19a3 Compare May 28, 2020 12:40
luke-jr
luke-jr reviewed May 28, 2020
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request May 28, 2020
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented May 28, 2020 •
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:

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.

This was referenced May 29, 2020
Closed
Merged
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from 8ab19a3 to 7f0d45c Compare June 5, 2020 09:57
@hebasto
Copy link
Member

hebasto commented Jun 5, 2020

Concept ACK.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from a53a8c9 to 8019cf2 Compare June 5, 2020 12:43
hebasto
hebasto reviewed Jun 5, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approach ACK 8019cf2dc7c4cf59ece71a6e7143b3f054a2243e.

@jonatack
Could you consider to replace s/ArgsManager::ALLOW_ANY/ArgsManager::ALLOW_BOOL/ for -getinfo command-line argument?

There is a general direction to move from the ArgsManager::ALLOW_ANY flag to more specific ones:

If you decide to implement this suggestion, the first commit and the last one could be combined.

@jonatack
Copy link
Member Author

jonatack commented Jun 5, 2020

Thanks for reviewing, @hebasto! From what I can tell, it would be best to not change this (yet):

  • ALLOW_BOOL appears to be unused in the codebase, apart from -rpcwhitelistdefault
  • the change would have no effect on -getinfo command parsing behavior and tests
  • in the event that we move to using more specific flags and that this one needs to be changed, many commands will need to be updated and ISTM it would be most coherent in terms of git history that they be changed together in the same commit or PR.

@hebasto
Copy link
Member

hebasto commented Jun 5, 2020

Thanks for reviewing, @hebasto! From what I can tell, it would be best to not change this (yet):

  • ALLOW_BOOL appears to be unused in the codebase, apart from -rpcwhitelistdefault

  • the change would have no effect on -getinfo command parsing behavior and tests

  • in the event that we move to using more specific flags and that this one needs to be changed, many commands will need to be updated and ISTM it would be most coherent in terms of git history that they be changed together in the same commit or PR.

FWIW, ArgsManager::ALLOW_BOOL is designed to use along with ArgsManager::GetBoolArg(). Additional error checking has not been implemented yet, unfortunately.

In any case, will continue my review :)

hebasto
hebasto reviewed Jun 6, 2020
View reviewed changes
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch 2 times, most recently from 68e4a1e to 320b510 Compare June 6, 2020 09:33
hebasto
hebasto reviewed Jun 6, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

Thanks for a separate commit that introduces the assert_scale() finction.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch 2 times, most recently from 059f317 to 9785b10 Compare June 6, 2020 15:19
hebasto
hebasto reviewed Jun 6, 2020
View reviewed changes
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from 9785b10 to 0215c23 Compare June 6, 2020 16:38
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from 0215c23 to 02c4b86 Compare June 17, 2020 07:23
@jonatack jonatack mentioned this pull request Jun 21, 2020
Merged
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-follow-ups branch from 02c4b86 to 865d2c3 Compare June 21, 2020 16:31
@jonatack jonatack mentioned this pull request Jun 22, 2020
Merged
@jonatack
Copy link
Member Author

jonatack commented Jun 22, 2020

Closing, will propose the changes one by one.

@jonatack jonatack closed this Jun 22, 2020
maflcko pushed a commit that referenced this pull request Jun 27, 2020
Merge #19354: doc: add release note for -getinfo displaying multiwall…
d342a45 
…et balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for #18594. This is one of the commits from #19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
@jonatack @luke-jr
cli: more robust bitcoin-cli -getinfo command parsing
e203242 
per Luke Dashjr review feedback in PR 18594

Github-Pull: bitcoin#19089
Rebased-From: d83ae3ed8fd83ed146d457314667656dc3ba57b5
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
@jonatack @luke-jr
test: add -getinfo command parsing regression tests
4de07b3 
These tests fail without the changes in the first commit of this PR.

Github-Pull: bitcoin#19089
Rebased-From: 198943934543a85a07bb50cf585adc0042280819
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
@jonatack @luke-jr
test: add assert_scale assertion to test framework
1da782b 
Github-Pull: bitcoin#19089
Rebased-From: 067e5dcbed4ba19b69d881e0b627f1d51478b678
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
@jonatack @luke-jr
test: add coverage for scale in -getinfo amount values
27104bb 
Github-Pull: bitcoin#19089
Rebased-From: 17f0b1e229f4ed60ab6b8e549ef67768b9aca29b
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 9, 2020
@jonatack @luke-jr
cli: update bitcoin-cli -getinfo help
483d014 
Github-Pull: bitcoin#19089
Rebased-From: 0215c23a956970e43d2418f6c6d0d1b1c8a2d8be
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
@PastaPastaPasta
Merge bitcoin#19354: doc: add release note for -getinfo displaying mu…
cece497 
…ltiwallet balances

6d35d0d doc: add release note for -getinfo displaying multiwallet balances (Jon Atack)

Pull request description:

  Release note for bitcoin#18594. This is one of the commits from bitcoin#19089, which had one concept ACK and approach ACK since late May. It seems better to submit the changes atomically.

Top commit has no ACKs.

Tree-SHA512: 38616d14b02c39f4ee4b93bf14f72043423cef177b595e85181bc9dc610fbe19d8271f2d2c9e5e17bb46423ffe27746e8e510b13a23ae6fd0e5bc4418a00dafa
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto left review comments

+2 more reviewers

@luke-jr luke-jr luke-jr left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests Utils/log/libs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jonatack @DrahtBot @hebasto @luke-jr @maflcko @fanquake