test: shift coverage from getunconfirmedbalance to getbalances

jonatack commented at 5:14 pm on March 27, 2020: member

This PR updates several tests and then removes the getunconfirmedbalance RPC which was deprecated in facfb4111d14a3b06c46690a2cca7ca91cea8a96 a year ago.

Next steps: remove the deprecated getwalletinfo fields and the getbalance RPC in follow-ups, if there seems to be consensus on those removals.

Update:

getunconfirmedbalance RPC was deprecated in facfb4111d14a3b06c46690a2cca7ca91cea8a96 a year ago, but following the review comments below, 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.

DrahtBot added the label RPC/REST/ZMQ on Mar 27, 2020

DrahtBot added the label Tests on Mar 27, 2020

DrahtBot added the label Wallet on Mar 27, 2020

MarcoFalke commented at 5:49 pm on March 27, 2020: member

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

jonatack commented at 6:06 pm on March 27, 2020: member

These were deprecated in the 0.19.0.1 release notes.

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

Some previous comments on this change:

#15930 (comment): I agree about depracating / removing getunconfirmedbalance
#15930#issue-275044019: 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.

MarcoFalke commented at 6:13 pm on March 27, 2020: member

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.

jonatack commented at 6:19 pm on March 27, 2020: member

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.

MarcoFalke commented at 6:31 pm on March 27, 2020: member

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

MarcoFalke commented at 6:34 pm on March 27, 2020: member

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.

jonatack commented at 6:46 pm on March 27, 2020: member

Agreed.

DrahtBot commented at 11:43 pm on March 27, 2020: member

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

Conflicts

No conflicts as of last run.

sipa commented at 1:24 am on March 28, 2020: member

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.

promag commented at 0:53 am on March 30, 2020: member

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.

Improve getbalances coverage in wallet_balance tests

and shift some getunconfirmedbalance calls to getbalances, as the former is
deprecated, while leaving essential coverage for it in test_balances().

3e6f7377f6

Shift coverage from getunconfirmedbalance to getbalances in wallet_abandonconflict tests 7eacdc5167

Use getbalances in wallet_address_types tests 0306d78cb4

jonatack force-pushed on Apr 5, 2020

jonatack commented at 5:55 pm on April 5, 2020: member

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.

jonatack renamed this:
~~rpc: remove deprecated getunconfirmedbalance~~
test: shift coverage from getunconfirmedbalance to getbalances
on Apr 5, 2020

jnewbery commented at 2:29 pm on April 6, 2020: member

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).

jonatack commented at 4:07 pm on April 7, 2020: member

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).

jnewbery commented at 7:36 pm on April 13, 2020: member

utACK 0306d78cb

MarcoFalke merged this on Apr 13, 2020

MarcoFalke closed this on Apr 13, 2020

jonatack deleted the branch on Apr 13, 2020

sidhujag referenced this in commit c5f6ac6251 on Apr 13, 2020

Fabcien referenced this in commit 19d226a189 on Jan 15, 2021

DrahtBot locked this on Feb 15, 2022

test: shift coverage from getunconfirmedbalance to getbalances #18451

Conflicts