(finally) remove getinfo

[TheBlueMatt]

I see no reason not to have done this in 0.13, let alone for 0.15.

[achow101]

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.

[TheBlueMatt]

Really? Didn't realize we only marked it deprecated in 0.14. Ugh, OK :(.

[jnewbery]

This should be rebased on #10841 and merged to master as soon as v0.15 has been cut.

[TheBlueMatt]

OK, rebased on #10841, this should be un-tagged 15 and tagged 16.

[laanwj]

This is way too late for 0.15, it's never a good idea to remove functionality last minute.

[sipa]

I think it was reopened intended for 0.16

[jnewbery]

#10841 was rejected in favour of #10857, which has now been merged. This should be rebased on master.

[TheBlueMatt]

Rebased on master.

[laanwj]

0.15 has been split off, I suppose we can merge this now.
Needs rebase again though.

[TheBlueMatt]

Rebased.

[maflcko]

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

The following patch should do:

diff --git a/doc/release-notes.md b/doc/release-notes.md
index aa1d1bea1..197a3aadc 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -56,6 +56,14 @@ frequently tested on them.
 Notable changes
 ===============
 
+RPC changes
+-----------
+
+* The deprecated RPC `getinfo` was removed. It is recommended that the more specific RPCs are used:
+   - `getblockchaininfo`
+   - `getnetworkinfo`
+   - `getwalletinfo`
+
 Credits
 =======
 
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 73ad3bdec..51abe2b25 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -87,7 +87,7 @@ enum class FeeEstimateMode;
 /** (client) version numbers for particular wallet features */
 enum WalletFeature
 {
-    FEATURE_BASE = 10500, // the earliest version new wallets supports (only useful for getinfo's clientversion output)
+    FEATURE_BASE = 10500, // the earliest version new wallets supports (only useful for getwalletinfo's clientversion output)
 
     FEATURE_WALLETCRYPT = 40000, // wallet encryption
     FEATURE_COMPRPUBKEY = 60000, // compressed public keys
diff --git a/test/functional/bitcoin_cli.py b/test/functional/bitcoin_cli.py
index 103320209..ccae7c9a6 100755
--- a/test/functional/bitcoin_cli.py
+++ b/test/functional/bitcoin_cli.py
@@ -16,11 +16,11 @@ class TestBitcoinCli(BitcoinTestFramework):
     def run_test(self):
         """Main test logic"""
 
-        self.log.info("Compare responses from getinfo RPC and `bitcoin-cli getinfo`")
-        cli_get_info = self.nodes[0].cli.getinfo()
-        rpc_get_info = self.nodes[0].getinfo()
+        self.log.info("Compare responses from getwalletinfo RPC and `bitcoin-cli getwalletinfo`")
+        cli_get_wallet_info = self.nodes[0].cli.getwalletinfo()
+        rpc_get_wallet_info = self.nodes[0].getwalletinfo()
 
-        assert_equal(cli_get_info, rpc_get_info)
+        assert_equal(cli_get_wallet_info, rpc_get_wallet_info)
 
 if __name__ == '__main__':
     TestBitcoinCli().main()

[TheBlueMatt]

Fixed.

[jnewbery]

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: jnewbery@10da6df

[maflcko]

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

[maflcko]

nit: Wallet with one t.

[TheBlueMatt]

Heh, oops, missed that. Fixed now.

[laanwj]

Heh, oops, missed that. Fixed now.

@TheBlueMatt you still didn't get the developer-notes change from jnewbery@10da6df

[TheBlueMatt]

Grrr, fixed now.

[promag]

Sorry, missing patch from @MarcoFalke?

[maflcko]

(finally) re-utACK aece8a4.

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

[laanwj]

@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 :)

[jnewbery]

Grrr, fixed now.

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

[TheBlueMatt]

No? The merged commit (aece8a4) has getwalletinfo and getblockchaininfo in it?

[jnewbery]

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.

[maflcko]

@jnewbery If it is worth it, just submit a pull for it.

[jnewbery]

@MarcoFalke it's not worth a PR

[TheBlueMatt]

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

[promag]

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

[maflcko]

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.