[rpc] deprecate estimatefee #11031

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:deprecate_estimatefee changing 8 files +42 −1
  1. jnewbery commented at 3:33 pm on August 11, 2017: member

    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.

  2. jnewbery force-pushed on Aug 11, 2017
  3. jnewbery commented at 5:50 pm on August 11, 2017: member
    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.
  4. morcos commented at 7:27 pm on August 11, 2017: member
    Concept ACK
  5. in src/init.cpp:433 in d301afead9 outdated 
    440@@ -441,6 +441,7 @@ std::string HelpMessage(HelpMessageMode mode)
441         strUsage += HelpMessageOpt("-checkmempool=<n>", strprintf("Run checks every <n> transactions (default: %u)", defaultChainParams->DefaultConsistencyChecks()));
442         strUsage += HelpMessageOpt("-checkpoints", strprintf("Disable expensive verification for known chain history (default: %u)", DEFAULT_CHECKPOINTS_ENABLED));
443         strUsage += HelpMessageOpt("-disablesafemode", strprintf("Disable safemode, override a real safe mode event (default: %u)", DEFAULT_DISABLE_SAFEMODE));
444+        strUsage += HelpMessageOpt("-enablerpcmethod", "Enables a deprecated RPC method");
    promag commented at 11:03 pm on August 11, 2017:
    Missing validation for this argument?
    promag commented at 11:03 pm on August 11, 2017:
    Argument should mention it is a deprecated method? Like -enabledeprecated=...
    jnewbery commented at 5:19 pm on August 12, 2017:

    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.

    promag commented at 5:33 pm on August 12, 2017:
    Yes there is, don’t recall exactly which.
    hodlbirb commented at 5:48 pm on August 12, 2017:
    Would be nice to have a list of deprecated RPC methods with enabled flag
    promag commented at 10:05 pm on August 12, 2017:
    With validation you force to clean outdated enablerpcmethod from config file.
    jnewbery commented at 1:22 pm on August 14, 2017:

    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.

    promag commented at 8:24 am on September 17, 2017:
    For reference, one arg validation http://github.com/bitcoin/bitcoin/blame/master/src/init.cpp#L1019.
  6. in src/rpc/mining.cpp:793 in d301afead9 outdated 
    789@@ -790,6 +790,12 @@ UniValue estimatefee(const JSONRPCRequest& request)
790             + HelpExampleCli("estimatefee", "6")
791             );
792 
793+    if (!IsDeprecatedRPCEnabled(std::string("estimatefee"))){
    promag commented at 11:04 pm on August 11, 2017:
    Should be before help? Why show the help if it’s disabled? Why the explicit std::string()?
    jnewbery commented at 5:30 pm on August 12, 2017:

    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.

  7. in src/rpc/server.cpp:390 in d301afead9 outdated 
    386@@ -387,6 +387,16 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
387         throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
388 }
389 
390+bool IsDeprecatedRPCEnabled(std::string rpc_name)
    promag commented at 11:05 pm on August 11, 2017:
    const std::string& method?
    jnewbery commented at 5:39 pm on August 12, 2017:
    changed
  8. in src/rpc/server.cpp:394 in d301afead9 outdated 
    386@@ -387,6 +387,16 @@ void JSONRPCRequest::parse(const UniValue& valRequest)
387         throw JSONRPCError(RPC_INVALID_REQUEST, "Params must be an array or object");
388 }
389 
390+bool IsDeprecatedRPCEnabled(std::string rpc_name)
391+{
392+    const std::vector<std::string> enabled_rpcs = gArgs.GetArgs("-enablerpcmethod");
393+
394+    if (find(enabled_rpcs.begin(), enabled_rpcs.end(), rpc_name) != enabled_rpcs.end()) {
    promag commented at 11:07 pm on August 11, 2017:

    No need to if:

    0return find(enabled_rpcs.begin(), enabled_rpcs.end(), rpc_name) != enabled_rpcs.end();
    jnewbery commented at 5:39 pm on August 12, 2017:
    yep, fixed
  9. promag commented at 11:11 pm on August 11, 2017: member

    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.

  10. fanquake added the label RPC/REST/ZMQ on Aug 12, 2017
  11. fanquake added this to the milestone 0.16.0 on Aug 12, 2017
  12. fanquake removed this from the milestone 0.16.0 on Aug 12, 2017
  13. jnewbery force-pushed on Aug 12, 2017
  14. jnewbery commented at 5:45 pm on August 12, 2017: member

    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.

  15. jnewbery force-pushed on Aug 23, 2017
  16. jnewbery commented at 3:06 pm on August 23, 2017: member
    Thanks to @mess110 for contributing a functional test to cover the new command line option.
  17. jnewbery force-pushed on Sep 12, 2017
  18. jnewbery commented at 6:48 pm on September 12, 2017: member
    rebased
  19. jnewbery force-pushed on Sep 14, 2017
  20. jnewbery commented at 1:54 pm on September 14, 2017: member
    rebased and fixed conflict
  21. TheBlueMatt commented at 7:09 pm on September 15, 2017: member
    utACK dc163bca56fa10ae06874e56a2ee08af44b4be8a
  22. jonasschnelli commented at 3:57 am on September 17, 2017: contributor
    utACK dc163bca56fa10ae06874e56a2ee08af44b4be8a
  23. in src/rpc/mining.cpp:792 in dc163bca56 outdated 
    788@@ -789,6 +789,12 @@ UniValue estimatefee(const JSONRPCRequest& request)
789             + HelpExampleCli("estimatefee", "6")
790             );
791 
792+    if (!IsDeprecatedRPCEnabled("estimatefee")){
    promag commented at 8:24 am on September 17, 2017:
    Missing space before {.
    jnewbery commented at 8:13 pm on September 18, 2017:
    fixed
  24. promag commented at 8:26 am on September 17, 2017: member
    utACK.
  25. MarcoFalke commented at 3:08 pm on September 17, 2017: member
    utACK dc163bca56fa10ae06874e56a2ee08af44b4be8a
  26. in test/functional/deprecated_rpc.py:2 in dc163bca56 outdated 
    0@@ -0,0 +1,23 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2014-2016 The Bitcoin Core developers
    instagibbs commented at 7:15 pm on September 18, 2017:
    micronit: 2017
    MarcoFalke commented at 7:26 pm on September 18, 2017:
    Will be fixed by the “bump copyright headers” script ;)
    jnewbery commented at 8:14 pm on September 18, 2017:
    I was pushing anyone so I fixed this
  27. instagibbs approved
  28. instagibbs commented at 7:17 pm on September 18, 2017: member
    utACK
  29. jnewbery force-pushed on Sep 18, 2017
  30. jnewbery commented at 8:14 pm on September 18, 2017: member

    Updated:

    • fixed nits
    • fixed help text

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

  31. jnewbery force-pushed on Sep 18, 2017
  32. meshcollider commented at 7:14 pm on September 21, 2017: contributor
    utACK https://github.com/bitcoin/bitcoin/pull/11031/commits/4efc8317c2d665d910374aff6e40989ae9917617 modulo the rename of the parameter being discussed on IRC
  33. luke-jr approved
  34. luke-jr commented at 7:17 pm on September 21, 2017: member
    utACK
  35. jnewbery force-pushed on Sep 21, 2017
  36. jnewbery commented at 7:41 pm on September 21, 2017: member
    Changed argument name from -enablerpcmethod to -deprecatedrpc as discussed in the IRC meeting: https://botbot.me/freenode/bitcoin-core-dev/2017-09-21/?msg=91401205&page=3
  37. in test/functional/deprecated_rpc.py:13 in 822ada4867 outdated 
     8+
 9+class DeprecatedRpcTest(BitcoinTestFramework):
10+    def set_test_params(self):
11+        self.num_nodes = 2
12+        self.setup_clean_chain = True
13+        self.extra_args = [[], ["-deprecatedrpc=estimatefee"]]
    achow101 commented at 7:50 pm on September 21, 2017:
    I think it would be better to have a dummy RPC that is marked deprecated since estimatefee will be removed in the future.
    jnewbery commented at 3:06 pm on September 22, 2017:

    hmm, I’m not sure we should add more product code just so the test code can test something else.

    We should just change this test to use a different rpc when estimatefee is removed (or skip the test entirely if there are currently no deprecated RPCs).

  38. sipa commented at 7:06 pm on September 22, 2017: member

    utACK 822ada4867a046bd5dbbdaacdef71999664c9704

    I really like this approach for deprecation.

  39. fanquake added this to the milestone 0.16.0 on Sep 25, 2017
  40. achow101 commented at 3:56 pm on September 26, 2017: member
    utACK 822ada4867a046bd5dbbdaacdef71999664c9704
  41. promag changes_requested
  42. promag commented at 4:02 pm on September 26, 2017: member
    Fix a6919d9 commit description.
  43. [rpc] Deprecate estimatefee RPC 
    Deprecate 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=<method>` argument. RPC method is removed entirely in
version (x+1).
    d4cdbd6fb6
  44. [rpc] [tests] Add deprecated RPC test 048e0c3e26
  45. jnewbery force-pushed on Sep 26, 2017
  46. jnewbery commented at 4:18 pm on September 26, 2017: member
    fixed commit comment. @laanwj - time for merge? This PR has 9 utACKs
  47. promag approved
  48. MarcoFalke commented at 12:13 pm on September 27, 2017: member

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

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

  49. laanwj commented at 12:29 pm on September 27, 2017: member
    @MarcoFalke thanks
  50. MarcoFalke merged this on Sep 27, 2017
  51. MarcoFalke closed this on Sep 27, 2017
  52. MarcoFalke referenced this in commit ef8340d25f on Sep 27, 2017
  53. jnewbery deleted the branch on Sep 27, 2017
  54. jtimon commented at 11:48 pm on September 27, 2017: contributor
    posthumous utACK
  55. laanwj referenced this in commit 3843780fd8 on Feb 8, 2018
  56. PastaPastaPasta referenced this in commit ce4a9b49ff on Dec 22, 2019
  57. PastaPastaPasta referenced this in commit 817e8ecdf6 on Jan 2, 2020
  58. PastaPastaPasta referenced this in commit 63390920d5 on Jan 4, 2020
  59. PastaPastaPasta referenced this in commit 4d597601ba on Jan 12, 2020
  60. PastaPastaPasta referenced this in commit 5eee9c3946 on Jan 12, 2020
  61. PastaPastaPasta referenced this in commit 58303cd55e on Jan 12, 2020
  62. PastaPastaPasta referenced this in commit 56780e9e43 on Jan 12, 2020
  63. PastaPastaPasta referenced this in commit 4af8db6d33 on Jan 12, 2020
  64. PastaPastaPasta referenced this in commit 168e93162f on Jun 9, 2020
  65. PastaPastaPasta referenced this in commit 7e04a9a3b1 on Jun 9, 2020
  66. PastaPastaPasta referenced this in commit 8254d30d21 on Jun 10, 2020
  67. PastaPastaPasta referenced this in commit 55cc7d8f89 on Jun 10, 2020
  68. PastaPastaPasta referenced this in commit da0042972b on Jun 11, 2020
  69. PastaPastaPasta referenced this in commit 1b7e6b45af on Jun 11, 2020
  70. ckti referenced this in commit 78eb635f66 on Mar 28, 2021
  71. gades referenced this in commit 12199bee20 on Jun 26, 2021
  72. DrahtBot locked this on Sep 8, 2021


jnewbery morcos promag hodlbirb TheBlueMatt jonasschnelli MarcoFalke instagibbs meshcollider luke-jr achow101 sipa laanwj jtimon

Labels
RPC/REST/ZMQ

Milestone
0.16.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me