Skip to content

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Apr 9, 2020

Extracted from #18453 to preserve that PR as a discussion on multiwallet RPC/CLI.

This PR updates bitcoin-cli -getinfo to fetch the wallet balance from getbalances in order to no longer depend on getwalletinfo.balance which was deprecated a year ago in facfb41.

I found this when removing the getwalletinfo() balance, unconfirmed_balance, and immature_balance fields to see what broke from depending on them.

I didn't see any perceivable change in -getinfo run time from the change.

Test coverage for this change is provided by test/functional/interface_bitcoin_cli.py, which the second commit updates to (a) no longer depend on getwalletinfo.balances and (b) test the -getinfo blockcount and balance fields against non-default, non-zero values.

@jonatack
Copy link
Member Author

jonatack commented Apr 9, 2020

This change was ACKed in the original PR by @jonasschnelli.

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

utACK baa069151971f137782e3bd998f07f9f16185637

@maflcko maflcko changed the title getinfo: call getbalances.ismine.trusted instead of getwalletinfo.balance cli: call getbalances.ismine.trusted instead of getwalletinfo.balance Apr 9, 2020
@jonatack jonatack force-pushed the getinfo-call-getbalances-instead-of-getwalletinfo-balances branch from baa0691 to 7dd939a Compare April 9, 2020 14:28
@jonatack
Copy link
Member Author

jonatack commented Apr 9, 2020

Thanks for reviewing @vasild! Can you re-ACK?

@jonatack jonatack force-pushed the getinfo-call-getbalances-instead-of-getwalletinfo-balances branch 2 times, most recently from 085d332 to 130abeb Compare April 9, 2020 15:28
@jonatack
Copy link
Member Author

jonatack commented Apr 9, 2020

Force-pushed for good catch by @vasild (thanks!)

@vasild
Copy link
Contributor

vasild commented Apr 9, 2020

ACK 130abeb93

Compiles and test/functional/interface_bitcoin_cli.py passes on my end.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 9, 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.

- no longer depend on the deprecated balance field of getwalletinfo

- test blockcount and balance against non-default, non-zero expected values
@jonatack jonatack force-pushed the getinfo-call-getbalances-instead-of-getwalletinfo-balances branch from 130abeb to 5df0877 Compare April 9, 2020 15:35
@jonatack
Copy link
Member Author

jonatack commented Apr 9, 2020

One (hopefully) last fixup 🤦

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

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

re-ACK 5df0877

test/lint/lint-python.sh passes locally

@robot-visions
Copy link
Contributor

ACK 5df0877

  • unit tests pass
  • functional tests pass both with and without wallets compiled
  • manual test passes: no user-facing differences to -getinfo behavior for (i) wallet functionality not compiled, (ii) single wallet loaded, (iii) multiple wallets loaded

And thanks for updating the ID_ naming!

@maflcko
Copy link
Member

maflcko commented Apr 9, 2020

ACK 5df0877

Copy link
Contributor

@promag promag 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 5df0877.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 5df0877

@maflcko maflcko merged commit 6ab96ec into bitcoin:master Apr 10, 2020
@jonatack jonatack deleted the getinfo-call-getbalances-instead-of-getwalletinfo-balances branch April 10, 2020 17:14
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
- no longer depend on the deprecated balance field of getwalletinfo

- test blockcount and balance against non-default, non-zero expected values

Github-Pull: bitcoin#18574
Rebased-From: 5df0877
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 14, 2021
…ance

Summary:
> This PR updates bitcoin-cli -getinfo to fetch the wallet balance from getbalances in order to no longer depend on getwalletinfo.balance which was deprecated a year ago in D6447
>
> test: update and harden interface_bitcoin_cli tests
>
> - no longer depend on the deprecated balance field of getwalletinfo
> - test blockcount and balance against non-default, non-zero expected values

This is a backport of Core [[bitcoin/bitcoin#18574 | PR18574]]

Test Plan:
`ninja all check-all`
`src/bitcoin-cli -getinfo`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Subscribers: majcosta

Differential Revision: https://reviews.bitcoinabc.org/D8912
PhotoshiNakamoto added a commit to PhotonicBitcoin/pBTC-core that referenced this pull request Dec 11, 2021
Bitcoin-Core PR: bitcoin/bitcoin#18574

Pull request description:

  Extracted from #18453 to preserve that PR as a discussion on multiwallet RPC/CLI.

  This PR updates `bitcoin-cli -getinfo` to fetch the wallet balance from `getbalances` in order to no longer depend on `getwalletinfo.balance` which was deprecated a year ago in facfb41.

  I found this when removing the getwalletinfo() `balance`, `unconfirmed_balance`, and `immature_balance` fields to see what broke from depending on them.

  I didn't see any perceivable change in `-getinfo` run time from the change.

  Test coverage for this change is provided by `test/functional/interface_bitcoin_cli.py`, which the second commit updates to (a) no longer depend on getwalletinfo.balances and (b) test the -getinfo blockcount and balance fields against non-default, non-zero values.
@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.

8 participants