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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32721.
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.
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?
213@@ -214,29 +214,6 @@ RPCHelpMan getbalance()
214 };
215 }
216
217-RPCHelpMan getunconfirmedbalance()
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"},
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.
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 |
|---|---|
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.