Remove deprecated rpc options by jnewbery · Pull Request #12336 · bitcoin/bitcoin

jnewbery · 2018-02-02T19:45:56Z

There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched.

estimatefee RPC has been removed. The feature_fee_estimation.py test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC estimatesmartfee. Improving the test coverage should be done in a new PR. ([rpc] deprecate estimatefee #11031)
the errors field returned by getmininginfo has been deprecated and replaced by a warning field. ([RPC] Add "errors" field to getblockchaininfo and unify "errors" field in get*info RPCs #10858)
providing addresses as inputs to createmultisig has been deprecated. Users should use addmultisigaddress instead ([RPC] Disallow using addresses in createmultisig #11415)
The return format from addmultisigaddress has changed ([RPC] Disallow using addresses in createmultisig #11415)

getwitnessaddress was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up)

promag · 2018-02-02T19:50:24Z

src/rpc/mining.cpp

Why not remove?

Because there's no harm in leaving the warning in for one additional release. Output is now:

→ bcli estimatefee 4 error code: -32 error message: estimatefee was removed in v0.17. Clients should use estimatesmartfee.

Alright, in the future we should do the same for others. Not sure if this is documented (or should be).

~~Nit: No need for \n at the end of exception messages.~~
Edit: never mind, I didn't see it was continued on the next line due to the comment interrupting it, sorry.

I guess there is no need for \n. There are other multiple sentence errors without it.

ugh. This error message was split over two lines before the commit. Please can we focus on reviewing the code changes and not how many new lines there should be in error messages?

promag · 2018-02-02T19:51:50Z

src/rpc/mining.cpp

Nit, to align or not to align 👀 IMO one space.

Fine. Done.

maflcko · 2018-02-03T13:24:57Z

src/rpc/mining.cpp

Should be hidden, I guess.

Yes. Fixed.

sipa · 2018-02-05T21:00:33Z

Concept ACK

laanwj

Looks good to me, a few small nits.
Would make sense to mention final removal of these in doc/release-notes.md (the one on the master branch).

laanwj · 2018-02-06T09:41:35Z

test/functional/rpc_deprecated.py

Please add a comment here that newly deprecated functionality should be tested here, otherwise this empty function looks really strange.
Alternatively, maybe it's better to remove this test entirely if it's empty, It can always be brought back.

IMO should stay otherwise it's easy to add deprecated tests elsewhere. A comment would be nice.

Comment added.

laanwj · 2018-02-06T10:02:25Z

src/rpc/mining.cpp

~~Nit: No need for \n at the end of exception messages.~~
Edit: never mind, I didn't see it was continued on the next line due to the comment interrupting it, sorry.

promag

Tested ACK.

Would make sense to mention final removal of these in doc/release-notes.md

Or needs release notes tag.

I would reword commit "[tests] Remove tests for deprecated estimatefee RPC" to "[qa] Replace deprecated estimatefee with estimatesmartfee".

Also remove:?

bitcoin/src/wallet/rpcwallet.cpp

Line 3140 in c9ca4f6

    
           {"reserveChangeKey", UniValueType(UniValue::VBOOL)}, // DEPRECATED (and ignored), should be removed in 0.16 or so.

promag · 2018-02-08T11:32:51Z

test/functional/rpc_deprecated.py

IMO should stay otherwise it's easy to add deprecated tests elsewhere. A comment would be nice.

promag · 2018-02-08T11:35:15Z

src/rpc/misc.cpp

I believe there is no test for this error?

bitcoin/test/functional/rpc_rawtransaction.py

Line 155 in b264528

assert_raises_rpc_error(-5, "Invalid public key", self.nodes[0].createmultisig, 2, [addr1Obj['pubkey'], addr1]) # addmultisigaddress can take both pubkeys and addresses so long as they are in the wallet, which is tested here.

promag · 2018-02-08T11:39:15Z

src/rpc/mining.cpp

I guess there is no need for \n. There are other multiple sentence errors without it.

jnewbery · 2018-02-08T14:12:24Z

I would reword commit "[tests] Remove tests for deprecated estimatefee RPC" to "[qa] Replace deprecated estimatefee with estimatesmartfee".

That's not what the commit does. It removes the tests for estimatefee and leaves the (minimal) checking of estimatesmartfee in place.

Please see original comment:

The feature_fee_estimation.py test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC estimatesmartfee. Improving the test coverage should be done in a new PR.

Feel free to contribute better testing for estimatesmartfee.

laanwj · 2018-02-08T14:25:37Z

test/functional/rpc_deprecated.py

+        # self.nodes[1].createmultisig(1, [self.nodes[1].getnewaddress()])
+        #
+        # There are currently no deprecated RPC methods in master, so this
+        # test is currently empty.


db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (#11415) - The return format from `addmultisigaddress` has changed (#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (bitcoin#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (bitcoin#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (bitcoin#11415) - The return format from `addmultisigaddress` has changed (bitcoin#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after bitcoin#11739 and bitcoin#11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

jnewbery added 3 commits February 2, 2018 10:08

[tests] Fix style warnings in feature_fee_estimation.py

d119f2e

[tests] Remove tests for deprecated estimatefee RPC

a5623b1

[tests] Remove estimatefee from rpc_deprecated.py test

a8e437a

promag reviewed Feb 2, 2018

View reviewed changes

jnewbery force-pushed the remove_deprecated_rpcs branch from a5f1c8e to 7c5aede Compare February 2, 2018 20:36

fanquake added the RPC/REST/ZMQ label Feb 2, 2018

maflcko reviewed Feb 3, 2018

View reviewed changes

jnewbery added 2 commits February 5, 2018 15:17

[rpc] remove deprecated estimatefee RPC

c6f09c2

[rpc] Remove deprecated getmininginfo RPC option

d066a1c

jnewbery force-pushed the remove_deprecated_rpcs branch from 7c5aede to ba8bac6 Compare February 5, 2018 20:18

laanwj reviewed Feb 6, 2018

View reviewed changes

laanwj self-assigned this Feb 6, 2018

promag reviewed Feb 8, 2018

View reviewed changes

jnewbery added 3 commits February 8, 2018 08:59

[tests] Remove test for deprecated createmultsig option

ed45c82

[RPC] Remove deprecated createmultisig object

cb28a0b

[RPC] Remove deprecated addmultisigaddress return format

db1cbcc

jnewbery force-pushed the remove_deprecated_rpcs branch from ba8bac6 to db1cbcc Compare February 8, 2018 14:12

laanwj reviewed Feb 8, 2018

View reviewed changes

laanwj merged commit db1cbcc into bitcoin:master Feb 8, 2018

laanwj removed their assignment Feb 8, 2018

jnewbery deleted the remove_deprecated_rpcs branch February 8, 2018 16:53

laanwj mentioned this pull request Feb 13, 2018

rpc: Reject deprecated reserveChangeKey in fundrawtransaction #12409

Merged

jnewbery mentioned this pull request Mar 21, 2018

tests: Remove unused argument max_invalid from check_estimates(...) #12746

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

jnewbery commented Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Feb 5, 2018

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jnewbery commented Feb 2, 2018 •

edited

Loading

laanwj Feb 6, 2018 •

edited

Loading

laanwj Feb 6, 2018 •

edited

Loading

promag left a comment •

edited

Loading