Skip to content
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.

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

cli: display multiwallet balances in -getinfo #18594

Merged

Conversation

jonatack
Copy link
Member

@jonatack jonatack commented Apr 11, 2020

This PR is a client-side version of #18453, per review feedback there and review club discussions. It updates bitcoin-cli -getinfo on the client side to display wallet name and balance for the loaded wallets when more than one is loaded (e.g. you are in "multiwallet mode") and -rpcwallet= is not passed; otherwise, behavior is unchanged.

before

$ bitcoin-cli -getinfo -regtest
{
  "version": 199900,
  "blocks": 15599,
  "headers": 15599,
  "verificationprogress": 1,
  "timeoffset": 0,
  "connections": 0,
  "proxy": "",
  "difficulty": 4.656542373906925e-10,
  "chain": "regtest",
  "balance": 0.00001000,
  "relayfee": 0.00001000
}

after

$ bitcoin-cli -getinfo -regtest
{
  "version": 199900,
  "blocks": 15599,
  "headers": 15599,
  "verificationprogress": 1,
  "timeoffset": 0,
  "connections": 0,
  "proxy": "",
  "difficulty": 4.656542373906925e-10,
  "chain": "regtest",
  "balances": {
    "": 0.00001000,
    "Encrypted": 0.00003500,
    "day-to-day": 0.00000120,
    "side project": 0.00000094
  }
}

Review club discussion about this PR is here: https://bitcoincore.reviews/18453

This PR can be manually tested by building, creating/loading/unloading several wallets with bitcoin-cli createwallet/loadwallet/unloadwallet and running bitcoin-cli -getinfo and bitcoin-cli -rpcwallet=<wallet-name> -getinfo.

wallet_multiwallet.py --usecli provides regression test coverage on this change, along with interface_bitcoin_cli.py where this PR adds test coverage.

Credit to Wladimir J. van der Laan for the idea in #17314 and #18453 (comment).

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-balances branch from 9139f33 to 0e90970 Compare April 11, 2020 14:19
@DrahtBot DrahtBot added the Tests label Apr 11, 2020
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-balances branch 2 times, most recently from 2c78f4c to a9476fc Compare April 11, 2020 16:59
@promag
Copy link
Contributor

promag commented Apr 12, 2020

Concept ACK

interface_bitcoin_cli.py fails in no wallet job.

@fanquake
Copy link
Member

It seems this is in part reverting changes that were just merged in #18574 (split out of #18453), and refactoring to handle multiwallet.

I understand splitting up changes, and maybe it doesn't matter so much in this instance, but it's a bit of code/review churn if we're PR'ing and merging refactors only to essentially undo the changes and do something else in a different PR a few hours later.

@jonatack
Copy link
Member Author

jonatack commented Apr 12, 2020

I empathise. See #18453 (comment). #18574 was the only change that seemed to have consensus, so it seemed best to split it out for merge and propose client-side code in this PR to test and compare with the server-side code in #18453.

EDIT: This PR no longer touches the changes merged in #18574.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 12, 2020

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.

@jonatack
Copy link
Member Author

Updated with the following:

src/bitcoin-cli.cpp Outdated Show resolved Hide resolved
@jnewbery
Copy link
Contributor

Concept and approach ACK.

@jonatack jonatack force-pushed the cli-getinfo-multiwallet-balances branch 2 times, most recently from ed06899 to 12297df Compare April 14, 2020 17:45
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-balances branch 2 times, most recently from 2baa28a to 50e44c9 Compare April 16, 2020 18:10
@jonatack jonatack force-pushed the cli-getinfo-multiwallet-balances branch from 50e44c9 to c2d63a8 Compare April 16, 2020 23:09
src/rpc/request.cpp Outdated Show resolved Hide resolved
src/bitcoin-cli.cpp Outdated Show resolved Hide resolved
@michaelfolkson
Copy link
Contributor

ACK ea59b18

Built, ran tests, tested manually by creating new wallet, transferring funds to it and unloading. Light code review.

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 14, 2020
to ConnectAndCallRPC() to be callable for individual connections.

This is needed for RPCs that need to be called and handled sequentially, rather
than alone or in a batch.

For example, when fetching the balances for each loaded wallet, -getinfo will
call RPC listwallets, and then, depending on the result, RCP getbalances.

Best to review this commit with `git show -w`.

Github-Pull: bitcoin#18594
Rebased-From: 29f2cbd
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 14, 2020
to allow passing rpcwallet independently from the -rpcwallet user option, and to
move the logic to the top-level layer where most of the other option args are
handled.

Github-Pull: bitcoin#18594
Rebased-From: 7430775
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 14, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 14, 2020
and replace GetBoolArg with IsArgSet as we only want
to know if the arg is passed; we do not need the value.

Github-Pull: bitcoin#18594
Rebased-From: afce85e
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 14, 2020
and improve the existing -getinfo -rpcwallet tests.

Thanks to MarcoFalke for the review feedback to fix the
CLI command vs option settings.

Github-Pull: bitcoin#18594
Rebased-From: 5edad5c
maflcko pushed a commit that referenced this pull request Jun 27, 2020
…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
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 28, 2020
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 18, 2021
…ndCallRPC()

Summary:
> This is needed for RPCs that need to be called and handled sequentially, rather
> than alone or in a batch.
>
> For example, when fetching the balances for each loaded wallet, -getinfo will
> call RPC listwallets, and then, depending on the result, RPC getbalances.
>

This is a backport of Core [[bitcoin/bitcoin#18594 | PR18594]] [1/6]
bitcoin/bitcoin@29f2cbd

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9237
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 18, 2021
Summary:
> to allow passing rpcwallet independently from the -rpcwallet user option, and to
> move the logic to the top-level layer where most of the other option args are
> handled.

This is a backport of Core [[bitcoin/bitcoin#18594 | PR18594]] [2/6]
bitcoin/bitcoin@7430775

Depends on D9237

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9238
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 18, 2021
Summary:
This is a backport of Core [[bitcoin/bitcoin#18594 | PR18594]] [3/6]
bitcoin/bitcoin@9f01849

Depends on D9238

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9239
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 18, 2021
Summary:
and replace GetBoolArg with IsArgSet as we only want
to know if the arg is passed; we do not need the value.

This is a backport of Core [[bitcoin/bitcoin#18594 | PR18594]] [4/6]
bitcoin/bitcoin@afce85e

Depends on D9239

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9240
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 18, 2021
Summary:
This is a backport of Core [[bitcoin/bitcoin#18594 | PR18594]] [5/6]
bitcoin/bitcoin@903b6c1

Depends on D9240

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9241
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 18, 2021
Summary:
and improve the existing -getinfo -rpcwallet tests.

This concludes the backport of Core [[bitcoin/bitcoin#18594 | PR18594]] [6/6]
bitcoin/bitcoin@5edad5c

Depends on D9241

Test Plan: `ninja check-functional`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9242
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 12, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 12, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 12, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 12, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 16, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Jun 16, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…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
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 15, 2021
…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
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 30, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 30, 2021
Treat specifying -norpcwallet exactly the same as not specifying any -rpcwallet
option, instead of treating it like -rpcwallet=0 with 0 as the wallet name.

This restores previous behavior before 7430775
from bitcoin#18594, which inadvertently changed
it.
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.