wallet, rpc: Remove deprecated balances from getwalletinfo and getunconfirmedbalance #32721

achow101 commented at 7:15 PM on June 10, 2025: member

getwalletinfo result fields balance, immature_balance, and unconfirmed_balance, and the getunconfirmedbalance RPC have all been deprecated since 0.19.0. It's been long enough that they should either be removed or undeprecated. The functionality provided by these RPCs is provided by getbalances.

DrahtBot commented at 7:15 PM on June 10, 2025: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32721.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, BrandonOdiwuor, davidgumberg, w0xlt
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

pablomartin4btc commented at 9:17 PM on June 10, 2025: member

Concept ACK

There are still functional test references to the fields removed in the first commit (also in wallet_multiwallet.py, wallet_reorgsrestore.py). Should references in the wallet interface be updated as well?

This would need release notes at some point, no?

DrahtBot added the label CI failed on Jun 10, 2025

wallet, rpc: Remove deprecated balances from getwalletinfo 0ec255139b

achow101 force-pushed on Jun 10, 2025

w0xlt commented at 10:05 PM on June 10, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32721/commits/19a2f0ca09b9b56485d21e309486906c26e8afcf

DrahtBot requested review from pablomartin4btc on Jun 10, 2025

achow101 force-pushed on Jun 10, 2025

wallet, rpc, test: Remove deprecated getunconfirmedbalance c3fe85e2d6

DrahtBot removed the label CI failed on Jun 10, 2025

in src/wallet/rpc/coins.cpp:217 in c3fe85e2d6

 213 | @@ -214,29 +214,6 @@ RPCHelpMan getbalance()
 214 |      };
 215 |  }
 216 |  
 217 | -RPCHelpMan getunconfirmedbalance()

maflcko commented at 9:46 AM on June 11, 2025:

lgtm. Seems confusing to have a single inconsistent RPC that mirrors a single field of another RPC

in src/wallet/rpc/wallet.cpp:47 in c3fe85e2d6

  41 | @@ -42,9 +42,6 @@ static RPCHelpMan getwalletinfo()
  42 |                          {RPCResult::Type::STR, "walletname", "the wallet name"},
  43 |                          {RPCResult::Type::NUM, "walletversion", "the wallet version"},
  44 |                          {RPCResult::Type::STR, "format", "the database format (only sqlite)"},
  45 | -                        {RPCResult::Type::STR_AMOUNT, "balance", "DEPRECATED. Identical to getbalances().mine.trusted"},
  46 | -                        {RPCResult::Type::STR_AMOUNT, "unconfirmed_balance", "DEPRECATED. Identical to getbalances().mine.untrusted_pending"},
  47 | -                        {RPCResult::Type::STR_AMOUNT, "immature_balance", "DEPRECATED. Identical to getbalances().mine.immature"},

maflcko commented at 9:50 AM on June 11, 2025:

I think back when I deprecated this, the legacy wallet allowed for mine and watch_only dicts in the getbalances. so listing one but not the other here was confusing. however, now that descriptor wallets are required, this is no longer the case. Seems fine to remove or un-deprecate. No strong opinion.

rkrux commented at 3:27 PM on June 12, 2025: contributor

ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712

I'm in favour of this removal as it gets rid of duplication in the RPCs and leads to fewer sources of balance in the RPCs and to lesser code to maintain as well, which I find quite desirable.

I am not sure if the removal of the long deprecated items constitute to a breaking change as we would expect the users to not have been using these items already. Besides adding in the release notes, I like how the conventional commits suggest to mention breaking changes in the commit description: https://www.conventionalcommits.org/en/v1.0.0/.

I don't know if this has been brought up already in the repo before.

DrahtBot requested review from w0xlt on Jun 12, 2025

BrandonOdiwuor commented at 4:42 PM on June 12, 2025: contributor

ACK c3fe85e2d6dd4f251a62a99fd891b0fa370f9712 removing the deprecated balance, unconfirmed_balance, immature_balance fields from getwalletinfo and getunconfirmedbalance RPCs, as this infomation can be found on the getbalances RPC

Master	c3fe85e2d6dd4f251a62a99fd891b0fa370f9712
<img width="650" alt="Screenshot 2025-06-12 at 15 12 00" src="https://github.com/user-attachments/assets/2d0e3394-3354-4a96-9e23-de52727926e0" />	<img width="654" alt="Screenshot 2025-06-12 at 15 08 29" src="https://github.com/user-attachments/assets/ebc37a6c-7c59-4431-a1b9-57adaa14d18f" />

davidgumberg commented at 6:33 PM on June 12, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/commit/c3fe85e2d6dd4f251a62a99fd891b0fa370f9712

Good enough as-is, but I would personally prefer un-deprecating the balance fields in getwalletinfo to removing them, as the only good reason for removing them seems to be that another RPC does that, but that hasn't stopped overlapping functionality in RPC's elsewhere, and showing balances seems pretty reasonably in-scope for getwalletinfo.

w0xlt commented at 6:44 PM on June 12, 2025: contributor

reACK https://github.com/bitcoin/bitcoin/pull/32721/commits/c3fe85e2d6dd4f251a62a99fd891b0fa370f9712

glozow commented at 8:37 PM on June 25, 2025: member

Not required, but I think it would be nice to write a release note mentioning "getunconfirmedbalance and these fields from getwalletinfo which were deprecated in 0.19 are now unavailable. Here are the equivalent fields from getbalances: etc etc etc"

glozow merged this on Jun 25, 2025

glozow closed this on Jun 25, 2025