Not sure if we should break RPC users because it is convenient for us to delete code

These were deprecated in the 0.19.0.1 release notes.

- `getbalances` returns an object with all balances (`mine`,
  `untrusted_pending` and `immature`). Please refer to the RPC help of
  `getbalances` for details. The new RPC is intended to replace
  `getbalance`, `getunconfirmedbalance`, and the balance fields in
  `getwalletinfo`.  These old calls and fields may be removed in a
  future version. (#15930, #16239)

Some previous comments on this change:

• rpc: Add balances RPC #15930 (comment): I agree about depracating / removing getunconfirmedbalance

• rpc: Add balances RPC #15930 (comment): incorrectly named rpcs such as getunconfirmedbalance or rpcs redundant to this such as getbalance could be removed

In any case, it may be a good idea to update the tests.

Yes, they could be (not: must be) removed in the future, but we don't need to be overly aggressive about it when there is no reason to be so. Every API change is going to cause pain for users, encouraging them to not upgrade to the latest version and thus causing harm to everyone involved.

I think they can be removed when 0.19.0 is EOL or at least no longer actively maintained, since at that point we assume that no one is depending on the deprecated API.

I understand an argument for aggressive deprecation and removal when there is a massive maintenance burden (such as wallet accounts) or danger in using the deprecated API, but I don't see any of this apply here.

Much of the value of this PR (as well as the proposed follow-ups) is in updating the tests, so depending on consensus it can be oriented in that direction as well.

I don't mind updating tests as long as test coverage is not decreased https://marcofalke.github.io/btc_cov/total.coverage/src/wallet/rpcwallet.cpp.gcov.html#775

And before removing, it might have to go through a deprecation cycle with -deprecatedrpc according to https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning . I am not going to advocate to start this deprecation cycle now, but I wanted to mention it for completeness.

Agreed.

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

Conflicts

No conflicts as of last run.

I agree we should do a deprecation cycle first (with -deprecatedrpc). I'm neutral on whether we do that now or later. Once we've been through that cycle we should feel fine deleting.

And before removing, it might have to go through a deprecation cycle with -deprecatedrpc according to https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#versioning . I am not going to advocate to start this deprecation cycle now, but I wanted to mention it for completeness.

Agree, no need to rush here.

As per the review comments, this PR now only updates the test coverage to use getbalances while still leaving basic coverage for getunconfirmedbalance in wallet_balance.py.

That said, I've seen 3 regular contributors confused in the past 10 days by "DEPRECATED" warnings in the code that are not following the deprecation policy in JSON-RPC-interface.md#versioning.

ISTM these warnings should either be removed, or the calls deprecated (-deprecatedrpc), or the policy updated to describe these warnings as a pre-deprecation practice.

I think the right thing to do here is to go through a -deprecatedrpc cycle.

@MarcoFalke

I think they can be removed when 0.19.0 is EOL or at least no longer actively maintained, since at that point we assume that no one is depending on the deprecated API.

We can't assume that. Simply writing 'DEPRECATED' in RPC documentation doesn't do anything. No-one who is using that API is going to see that documentation, so they'll continue to use the interface exactly as they have done in the past (and in my experience, even people who do see it don't make any changes to their client behaviour if they don't have to).

I don't think it matters which release we start the -deprecatedrpc cycle in. It's a forcing function to make clients update their code (and gives them at least 6 months to do that).

I think the right thing to do here is to go through a -deprecatedrpc cycle.

Simply writing 'DEPRECATED' in RPC documentation doesn't do anything. No-one who is using that API is going to see that documentation, so they'll continue to use the interface exactly as they have done in the past (and in my experience, even people who do see it don't make any changes to their client behaviour if they don't have to).

I don't think it matters which release we start the -deprecatedrpc cycle in. It's a forcing function to make clients update their code (and gives them at least 6 months to do that).

I agree. The test updates in this PR should be done at any rate. Once they are merged, a PR can be opened to begin the deprecation process (or remove the warnings, or update the deprecation documentation if 2-stage soft-then-hard deprecation is the consensus preference).

utACK 0306d78