(finally) remove getinfo #10838

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2017-07-seriously-fuck-getinfo changing 6 files +15 −101
  1. TheBlueMatt commented at 11:48 pm on July 15, 2017: member
    I see no reason not to have done this in 0.13, let alone for 0.15.
  2. fanquake added the label RPC/REST/ZMQ on Jul 15, 2017
  3. TheBlueMatt force-pushed on Jul 15, 2017
  4. achow101 commented at 4:40 am on July 16, 2017: member

    NACK for 0.15

    Although getinfo has been known among Bitcoin Core developers as being deprecated for a long time, it was only documented and marked as deprecated in 0.14. Furthermore, a large number of projects which use the RPC commands rely on getinfo to make sure that the RPC server is available. Thus removing getinfo now would break a lot of things. I think that before we remove getinfo, we need to inform projects that use getinfo that it will be removed and that they need to change to using a different RPC command.

  5. fanquake added this to the milestone 0.15.0 on Jul 16, 2017
  6. TheBlueMatt commented at 4:49 pm on July 16, 2017: member
    Really? Didn’t realize we only marked it deprecated in 0.14. Ugh, OK :(.
  7. TheBlueMatt closed this on Jul 16, 2017
  8. jnewbery commented at 11:19 pm on July 16, 2017: member
    This should be rebased on #10841 and merged to master as soon as v0.15 has been cut.
  9. TheBlueMatt reopened this on Jul 16, 2017
  10. TheBlueMatt force-pushed on Jul 16, 2017
  11. TheBlueMatt commented at 11:30 pm on July 16, 2017: member
    OK, rebased on #10841, this should be un-tagged 15 and tagged 16.
  12. fanquake added this to the milestone 0.16.0 on Jul 17, 2017
  13. fanquake removed this from the milestone 0.15.0 on Jul 17, 2017
  14. laanwj commented at 6:09 am on July 17, 2017: member
    This is way too late for 0.15, it’s never a good idea to remove functionality last minute.
  15. sipa commented at 6:10 am on July 17, 2017: member
    I think it was reopened intended for 0.16
  16. jnewbery commented at 2:16 pm on July 21, 2017: member
    #10841 was rejected in favour of #10857, which has now been merged. This should be rebased on master.
  17. TheBlueMatt force-pushed on Jul 24, 2017
  18. TheBlueMatt renamed this:
    (finally) remove the longest-ever-deprecated RPC call getinfo
    (finally) remove getinfo
    on Jul 24, 2017
  19. TheBlueMatt commented at 3:49 pm on July 24, 2017: member
    Rebased on master.
  20. TheBlueMatt force-pushed on Jul 25, 2017
  21. laanwj commented at 10:07 am on August 28, 2017: member
    0.15 has been split off, I suppose we can merge this now. Needs rebase again though.
  22. TheBlueMatt commented at 1:14 pm on August 30, 2017: member
    Rebased.
  23. TheBlueMatt force-pushed on Aug 30, 2017
  24. MarcoFalke commented at 1:53 pm on August 30, 2017: member

    Needs silent merge conflict resolved. Also needs mention in the release notes.

    The following patch should do:

     0diff --git a/doc/release-notes.md b/doc/release-notes.md
 1index aa1d1bea1..197a3aadc 100644
 2--- a/doc/release-notes.md
 3+++ b/doc/release-notes.md
 4@@ -56,6 +56,14 @@ frequently tested on them.
 5 Notable changes
 6 ===============
 7 
 8+RPC changes
 9+-----------
10+
11+* The deprecated RPC `getinfo` was removed. It is recommended that the more specific RPCs are used:
12+   - `getblockchaininfo`
13+   - `getnetworkinfo`
14+   - `getwalletinfo`
15+
16 Credits
17 =======
18 
19diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
20index 73ad3bdec..51abe2b25 100644
21--- a/src/wallet/wallet.h
22+++ b/src/wallet/wallet.h
23@@ -87,7 +87,7 @@ enum class FeeEstimateMode;
24 /** (client) version numbers for particular wallet features */
25 enum WalletFeature
26 {
27-    FEATURE_BASE = 10500, // the earliest version new wallets supports (only useful for getinfo's clientversion output)
28+    FEATURE_BASE = 10500, // the earliest version new wallets supports (only useful for getwalletinfo's clientversion output)
29 
30     FEATURE_WALLETCRYPT = 40000, // wallet encryption
31     FEATURE_COMPRPUBKEY = 60000, // compressed public keys
32diff --git a/test/functional/bitcoin_cli.py b/test/functional/bitcoin_cli.py
33index 103320209..ccae7c9a6 100755
34--- a/test/functional/bitcoin_cli.py
35+++ b/test/functional/bitcoin_cli.py
36@@ -16,11 +16,11 @@ class TestBitcoinCli(BitcoinTestFramework):
37     def run_test(self):
38         """Main test logic"""
39 
40-        self.log.info("Compare responses from getinfo RPC and `bitcoin-cli getinfo`")
41-        cli_get_info = self.nodes[0].cli.getinfo()
42-        rpc_get_info = self.nodes[0].getinfo()
43+        self.log.info("Compare responses from getwalletinfo RPC and `bitcoin-cli getwalletinfo`")
44+        cli_get_wallet_info = self.nodes[0].cli.getwalletinfo()
45+        rpc_get_wallet_info = self.nodes[0].getwalletinfo()
46 
47-        assert_equal(cli_get_info, rpc_get_info)
48+        assert_equal(cli_get_wallet_info, rpc_get_wallet_info)
49 
50 if __name__ == '__main__':
51     TestBitcoinCli().main()
  25. TheBlueMatt force-pushed on Sep 4, 2017
  26. TheBlueMatt commented at 8:56 pm on September 4, 2017: member
    Fixed.
  27. jnewbery commented at 5:14 pm on September 5, 2017: member

    Needs rebase. A couple comments:

    • why not test all get*info RPC methods in the bitcoin_cli.py test?
    • Need to remove reference to getinfo in the developer notes.

    Feel free to take and squash fixup commit here: https://github.com/jnewbery/bitcoin/commit/10da6dfc8615abdddd7121af1d5381926efa2826

  28. in test/functional/bitcoin_cli.py:18 in f54f200b42 outdated 
    15@@ -16,9 +16,15 @@ def __init__(self):
16     def run_test(self):
17         """Main test logic"""
18 
19-        self.log.info("Compare responses from getinfo RPC and `bitcoin-cli getinfo`")
20-        cli_get_info = self.nodes[0].cli.getinfo()
21-        rpc_get_info = self.nodes[0].getinfo()
22+        self.log.info("Compare responses from gewallettinfo RPC and `bitcoin-cli getwalletinfo`")
    MarcoFalke commented at 5:19 pm on September 5, 2017:
    nit: Wallet with one t.
  29. MarcoFalke commented at 5:21 pm on September 5, 2017: member
    reACK f54f200b4263b884c86dd1a8644015ca75de4e35. I propose to merge this after rebase. No need to have this sitting around and get moldy.
  30. laanwj commented at 5:44 pm on September 6, 2017: member

    I propose to merge this after rebase. No need to have this sitting around and get moldy.

    Agree, though it needs rebase again.

    utACK https://github.com/bitcoin/bitcoin/pull/10838/commits/f54f200b4263b884c86dd1a8644015ca75de4e35

  31. achow101 commented at 5:49 pm on September 6, 2017: member
    utACK f54f200b4263b884c86dd1a8644015ca75de4e35
  32. TheBlueMatt force-pushed on Sep 6, 2017
  33. TheBlueMatt commented at 8:44 pm on September 6, 2017: member
    Rebased.
  34. jnewbery commented at 8:56 pm on September 6, 2017: member
    Please address comment #10838 (comment) :)
  35. TheBlueMatt force-pushed on Sep 6, 2017
  36. TheBlueMatt commented at 9:03 pm on September 6, 2017: member
    Heh, oops, missed that. Fixed now.
  37. laanwj commented at 9:48 pm on September 6, 2017: member

    Heh, oops, missed that. Fixed now. @TheBlueMatt you still didn’t get the developer-notes change from https://github.com/jnewbery/bitcoin/commit/10da6dfc8615abdddd7121af1d5381926efa2826

  38. laanwj assigned laanwj on Sep 6, 2017
  39. (finally) remove getinfo in favor of more module-specific infos aece8a4637
  40. TheBlueMatt force-pushed on Sep 6, 2017
  41. TheBlueMatt commented at 11:10 pm on September 6, 2017: member
    Grrr, fixed now.
  42. promag commented at 11:21 pm on September 6, 2017: member
    Sorry, missing patch from @MarcoFalke?
  43. MarcoFalke commented at 11:25 pm on September 6, 2017: member
    (finally) re-utACK aece8a4637f0d097e4be497bc82d59b37244d245. @promag No one writes release notes anyway. I stopped caring.
  44. laanwj commented at 11:28 pm on September 6, 2017: member

    @promag No one writes release notes anyway. I stopped caring.

    Well as you wrote them already it’d make sense to include them.

    But yes, people are really lazy with regard to writing release notes. Maybe the reminder issue was a bad idea as it encourages people to delay writing them :)

  45. laanwj merged this on Sep 6, 2017
  46. laanwj closed this on Sep 6, 2017
  47. laanwj referenced this in commit 66a5b419ef on Sep 6, 2017
  48. jnewbery commented at 11:42 pm on September 6, 2017: member

    Grrr, fixed now.

    Huh? Now you’ve removed the changes to bitcoin_cli.py to test all get*info RPCs. Was that intentional?

  49. TheBlueMatt commented at 0:49 am on September 7, 2017: member
    No? The merged commit (https://github.com/bitcoin/bitcoin/commit/aece8a4637f0d097e4be497bc82d59b37244d245) has getwalletinfo and getblockchaininfo in it?
  50. jnewbery commented at 6:02 pm on September 7, 2017: member

    My suggestion in #10838 (comment) was to improve coverage by testing all get*info RPCs and also update the developer notes. I included a link to a commit that improved test coverage and made the test code more compact and readable.

    No big deal. It was a suggestion, but this PR is fine as is. Post-commit ACK.

  51. MarcoFalke commented at 6:23 pm on September 7, 2017: member
    @jnewbery If it is worth it, just submit a pull for it.
  52. jnewbery commented at 6:34 pm on September 7, 2017: member
    @MarcoFalke it’s not worth a PR
  53. TheBlueMatt commented at 6:44 pm on September 7, 2017: member
    @jnewbery ah, ok, someone linked toa different version of that that only tested 2 of the get*infos and I took that one instead, sorry I missed the full version.
  54. promag commented at 12:36 pm on September 8, 2017: member

    Well as you wrote them already it’d make sense to include them.

    But yes, people are really lazy with regard to writing release notes. Maybe the reminder issue was a bad idea as it encourages people to delay writing them :)

    IMO release notes should be on the same level as source code. If a patch raises releases notes then those belong to that patch and should be reviewed, fixed, etc..

  55. MarcoFalke commented at 3:47 am on September 10, 2017: member

    If a patch raises releases notes then those belong to that patch and should be reviewed, fixed, etc..

    Agree. I’ve been mentioning that in pulls and tagged some with the ‘Needs release notes’ badge. Usually it has been left ignored.

  56. PastaPastaPasta referenced this in commit 1be95963e8 on Sep 23, 2019
  57. PastaPastaPasta referenced this in commit 829fb5a9e3 on Sep 24, 2019
  58. codablock referenced this in commit b9599c297f on Sep 24, 2019
  59. barrystyle referenced this in commit 8f2dd3acc7 on Jan 22, 2020
  60. PastaPastaPasta referenced this in commit 81915c9862 on Feb 13, 2020
  61. PastaPastaPasta referenced this in commit 0c6ce19048 on Feb 27, 2020
  62. PastaPastaPasta referenced this in commit f7f33091e1 on Feb 27, 2020
  63. ckti referenced this in commit 19b390adec on Mar 28, 2021
  64. DrahtBot locked this on Sep 8, 2021


TheBlueMatt achow101 jnewbery laanwj sipa MarcoFalke promag

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