[rpc] deprecate estimatefee #11031

jnewbery · 2017-08-11T15:33:41Z

Deprecates estimatefee in v0.16, for final removal in v0.17.

This commit introduces a phased removal of RPC methods. RPC method is
disabled by default in version x, but can be enabled by using the
-deprecatedrpc=<methodname> argument. RPC method is removed entirely in version
(x+1).

This gives users fair warning that an RPC is to be removed, and time to change client software if necessary. Deprecation warnings in RPC return values or release notes are easily ignored.

This is a more generic version of the approach I tried to use in #10841, which too late to make it into v0.15.

jnewbery · 2017-08-11T17:50:34Z

This could be made even more generic by adding additional deprecated field to the CRPCTable. That way the IsDeprecatedRPCEnabled() call could be added to the dispatcher instead of the individual RPC functions, and the help text could show methods as DEPRECATED. I didn't think that was worth doing for just this one RPC method.

morcos · 2017-08-11T19:27:06Z

Concept ACK

promag

Mixed feelings about adding a option to enable individual methods. Why not one option to enable all existing deprecated methods?

In any case, I would split the code in 2 commits: 1st add support for the new argument; 2nd make estimatefee deprecated. In the 2nd commit there should be a functional test for the new RPC error.

promag · 2017-08-11T23:03:24Z

src/init.cpp

@@ -441,6 +441,7 @@ std::string HelpMessage(HelpMessageMode mode)
        strUsage += HelpMessageOpt("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u)", defaultChainParams->DefaultConsistencyChecks()));
        strUsage += HelpMessageOpt("-checkpoints", strprintf("Disable expensive verification for known chain history (default: %u)", DEFAULT_CHECKPOINTS_ENABLED));
        strUsage += HelpMessageOpt("-disablesafemode", strprintf("Disable safemode, override a real safe mode event (default: %u)", DEFAULT_DISABLE_SAFEMODE));
+        strUsage += HelpMessageOpt("-enablerpcmethod", "Enables a deprecated RPC method");


Missing validation for this argument?

Argument should mention it is a deprecated method? Like -enabledeprecated=...

I think enablerpcmethod is already clear, especially since this is a hidden option and the help text explains that it's used for enabling a deprecated method.

I don't think there's validation for any of our arguments.

Yes there is, don't recall exactly which.

Would be nice to have a list of deprecated RPC methods with enabled flag

With validation you force to clean outdated enablerpcmethod from config file.

I don't think validating arguments is the norm. If I'm wrong, please point out where in the code this happens.

I'd definitely concept ACK a PR for better validating of command line arguments in general (for example, warning if a user tries setting an argument that doesn't exist, to catch mis-spellings).

This particular argument enablerpcmethod seems to be one where validation is least useful. Users will find out instantly if they didn't enable the RPC method. Outdated enablerpcmethod arguments are a no-op so are harmless.

For reference, one arg validation http://github.com/bitcoin/bitcoin/blame/master/src/init.cpp#L1019.

promag · 2017-08-11T23:04:57Z

src/rpc/mining.cpp

@@ -790,6 +790,12 @@ UniValue estimatefee(const JSONRPCRequest& request)
            + HelpExampleCli("estimatefee", "6")
            );

+    if (!IsDeprecatedRPCEnabled(std::string("estimatefee"))){


Should be before help? Why show the help if it's disabled? Why the explicit std::string()?

I initially had this before the help if statement, but the way that help is implemented in server.cpp means that calling help without any arguments calls into every RPC method. Adding the if (!IsDeprecatedRPCEnabled statement before the help if statement breaks that.

You're right that no explicit std::string() is required. Removing.

promag · 2017-08-11T23:05:48Z

src/rpc/server.cpp

@@ -387,6 +387,16 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
        throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
 }

+bool IsDeprecatedRPCEnabled(std::string rpc_name)


const std::string& method?

promag · 2017-08-11T23:07:21Z

src/rpc/server.cpp

+{
+    const std::vector<std::string> enabled_rpcs = gArgs.GetArgs("-enablerpcmethod");
+
+    if (find(enabled_rpcs.begin(), enabled_rpcs.end(), rpc_name) != enabled_rpcs.end()) {


No need to if:

return find(enabled_rpcs.begin(), enabled_rpcs.end(), rpc_name) != enabled_rpcs.end();

jnewbery · 2017-08-12T17:45:23Z

Thanks for the thoughtful review @promag!

Mixed feelings about adding a option to enable individual methods. Why not one option to enable all existing deprecated methods?

The point of this is that it forces action on the user if they want to continue using a deprecated method. In version x they need to explicitly update their config to include -enablerpcmethod=<method>, so they have fair warning of 1 release to update their client software before upgrading to version x+1. If there was a generic enabledeprecatedmethods that they could set and forget, then they might not realise that one of the methods they're using is deprecated, upgrade to version x+1 and find that they have no way to use the method.

In any case, I would split the code in 2 commits: 1st add support for the new argument; 2nd make estimatefee deprecated. In the 2nd commit there should be a functional test for the new RPC error.

Seemed to me like this is a small enough change to not need multiple commits. But I'm happy to split if you think it'll help future reviewers.

Agree that there should be a new functional test.

jnewbery · 2017-08-23T15:06:09Z

Thanks to @mess110 for contributing a functional test to cover the new command line option.

jnewbery · 2017-09-12T18:48:22Z

rebased

jnewbery · 2017-09-14T13:54:58Z

rebased and fixed conflict

TheBlueMatt · 2017-09-15T19:09:50Z

utACK dc163bc

jonasschnelli · 2017-09-17T03:57:58Z

utACK dc163bc

promag

utACK.

promag · 2017-09-17T08:24:41Z

src/rpc/mining.cpp

@@ -789,6 +789,12 @@ UniValue estimatefee(const JSONRPCRequest& request)
            + HelpExampleCli("estimatefee", "6")
            );

+    if (!IsDeprecatedRPCEnabled("estimatefee")){


Missing space before {.

maflcko · 2017-09-17T15:08:02Z

utACK dc163bc

instagibbs

utACK

instagibbs · 2017-09-18T19:15:13Z

test/functional/deprecated_rpc.py

@@ -0,0 +1,23 @@
+#!/usr/bin/env python3
+# Copyright (c) 2014-2016 The Bitcoin Core developers


micronit: 2017

Will be fixed by the "bump copyright headers" script ;)

I was pushing anyone so I fixed this

jnewbery · 2017-09-18T20:14:47Z

Updated:

fixed nits
fixed help text

Old HEAD for this branch is here: https://github.com/jnewbery/bitcoin/releases/tag/pr11031.1

meshcollider · 2017-09-21T19:14:38Z

utACK 4efc831 modulo the rename of the parameter being discussed on IRC

luke-jr

utACK

maflcko · 2017-09-27T12:13:52Z

re-utACK 048e0c3 (verified that only the option name changed)

Going to merge this, since some other pulls build on top of this.

laanwj · 2017-09-27T12:29:31Z

@MarcoFalke thanks

048e0c3 [rpc] [tests] Add deprecated RPC test (Cristian Mircea Messel) d4cdbd6 [rpc] Deprecate estimatefee RPC (John Newbery) Pull request description: Deprecates estimatefee in v0.16, for final removal in v0.17. This commit introduces a phased removal of RPC methods. RPC method is disabled by default in version x, but can be enabled by using the `-deprecatedrpc=<methodname>` argument. RPC method is removed entirely in version (x+1). This gives users fair warning that an RPC is to be removed, and time to change client software if necessary. Deprecation warnings in RPC return values or release notes are easily ignored. This is a more generic version of the approach I tried to use in #10841, which too late to make it into v0.15. Tree-SHA512: 9695a600e84b812974387333e4a6805d18972da30befb754e9e4da77cd9815d00c5cc2ee0b0350bdbbdb5fdc6ba47789f8b2c6f5b15c8cd5a1deefcc4832da30

jtimon · 2017-09-27T23:48:59Z

posthumous utACK

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

048e0c3 [rpc] [tests] Add deprecated RPC test (Cristian Mircea Messel) d4cdbd6 [rpc] Deprecate estimatefee RPC (John Newbery) Pull request description: Deprecates estimatefee in v0.16, for final removal in v0.17. This commit introduces a phased removal of RPC methods. RPC method is disabled by default in version x, but can be enabled by using the `-deprecatedrpc=<methodname>` argument. RPC method is removed entirely in version (x+1). This gives users fair warning that an RPC is to be removed, and time to change client software if necessary. Deprecation warnings in RPC return values or release notes are easily ignored. This is a more generic version of the approach I tried to use in bitcoin#10841, which too late to make it into v0.15. Tree-SHA512: 9695a600e84b812974387333e4a6805d18972da30befb754e9e4da77cd9815d00c5cc2ee0b0350bdbbdb5fdc6ba47789f8b2c6f5b15c8cd5a1deefcc4832da30

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

048e0c3 [rpc] [tests] Add deprecated RPC test (Cristian Mircea Messel) d4cdbd6 [rpc] Deprecate estimatefee RPC (John Newbery) Pull request description: Deprecates estimatefee in v0.16, for final removal in v0.17. This commit introduces a phased removal of RPC methods. RPC method is disabled by default in version x, but can be enabled by using the `-deprecatedrpc=<methodname>` argument. RPC method is removed entirely in version (x+1). This gives users fair warning that an RPC is to be removed, and time to change client software if necessary. Deprecation warnings in RPC return values or release notes are easily ignored. This is a more generic version of the approach I tried to use in bitcoin#10841, which too late to make it into v0.15. Tree-SHA512: 9695a600e84b812974387333e4a6805d18972da30befb754e9e4da77cd9815d00c5cc2ee0b0350bdbbdb5fdc6ba47789f8b2c6f5b15c8cd5a1deefcc4832da30

jnewbery force-pushed the deprecate_estimatefee branch from 052a98c to d301afe Compare August 11, 2017 17:48

promag reviewed Aug 11, 2017

View reviewed changes

fanquake added the RPC/REST/ZMQ label Aug 12, 2017

fanquake modified the milestone: 0.16.0 Aug 12, 2017

jnewbery force-pushed the deprecate_estimatefee branch from d301afe to 9a1c908 Compare August 12, 2017 17:39

mess110 mentioned this pull request Aug 22, 2017

add DeprecatedRpcTest jnewbery/bitcoin#3

Merged

jnewbery force-pushed the deprecate_estimatefee branch from 78738ee to cab9682 Compare August 23, 2017 15:05

This was referenced Aug 29, 2017

[RPC] Split signrawtransaction into wallet and non-wallet RPC command #10579

Merged

[RPC] Split part of validateaddress into getaddressinfo #10583

Merged

jnewbery force-pushed the deprecate_estimatefee branch from cab9682 to 744b99b Compare September 12, 2017 18:47

jnewbery force-pushed the deprecate_estimatefee branch from 744b99b to dc163bc Compare September 14, 2017 13:54

promag reviewed Sep 17, 2017

View reviewed changes

instagibbs approved these changes Sep 18, 2017

View reviewed changes

jnewbery force-pushed the deprecate_estimatefee branch from dc163bc to 4876eea Compare September 18, 2017 20:13

jnewbery force-pushed the deprecate_estimatefee branch from 4876eea to 4efc831 Compare September 18, 2017 21:54

jnewbery mentioned this pull request Sep 19, 2017

[RPC] Add "errors" field to getblockchaininfo and unify "errors" field in get*info RPCs #10858

Merged

luke-jr approved these changes Sep 21, 2017

View reviewed changes

jnewbery force-pushed the deprecate_estimatefee branch from 4efc831 to 822ada4 Compare September 21, 2017 19:39

promag approved these changes Sep 26, 2017

View reviewed changes

maflcko merged commit 048e0c3 into bitcoin:master Sep 27, 2017

jnewbery deleted the deprecate_estimatefee branch September 27, 2017 13:53

ryanofsky mentioned this pull request Oct 21, 2017

Rename account to label where appropriate #11536

Merged

achow101 mentioned this pull request Oct 23, 2017

Use estimatesmartfee instead of estimatefee goatpig/BitcoinArmory#341

Closed

SomberNight mentioned this pull request Oct 29, 2017

use estimatesmartfee for fee estimation kyuupichan/electrumx#300

Merged

meshcollider mentioned this pull request Jan 17, 2018

TODO for release notes 0.16.0 #11054

Closed

jnewbery mentioned this pull request Feb 2, 2018

Remove deprecated rpc options #12336

Merged

jnewbery mentioned this pull request Oct 3, 2018

Breaking User Space #14363

Closed

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

		@@ -0,0 +1,23 @@
		#!/usr/bin/env python3
		# Copyright (c) 2014-2016 The Bitcoin Core developers

[rpc] deprecate estimatefee #11031

[rpc] deprecate estimatefee #11031

Uh oh!

Conversation

jnewbery commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Aug 11, 2017

Uh oh!

morcos commented Aug 11, 2017

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!

promag Aug 11, 2017 • 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!

dsirotkin256 Aug 12, 2017 • 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!

promag Aug 11, 2017 • 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!

jnewbery commented Aug 12, 2017

Uh oh!

jnewbery commented Aug 23, 2017

Uh oh!

jnewbery commented Sep 12, 2017

Uh oh!

jnewbery commented Sep 14, 2017

Uh oh!

TheBlueMatt commented Sep 15, 2017

Uh oh!

jonasschnelli commented Sep 17, 2017

Uh oh!

promag 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!

maflcko commented Sep 17, 2017

Uh oh!

instagibbs 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!

jnewbery commented Sep 18, 2017

jnewbery commented Aug 11, 2017 •

edited

Loading

promag left a comment •

edited

Loading

promag Aug 11, 2017 •

edited

Loading

dsirotkin256 Aug 12, 2017 •

edited

Loading

promag Aug 11, 2017 •

edited

Loading

meshcollider commented Sep 21, 2017 •

edited

Loading