RPC getinfo,getnetworkinfo: export local node's client sub-version string #4914

pull jgarzik wants to merge 1 commits into bitcoin:master from jgarzik:2014_rpc_subver changing 2 files +6 −0

jgarzik commented at 1:56 AM on September 15, 2014: contributor

Alternative to #4829
RPC getinfo,getnetworkinfo: export local node's client sub-version string eadbedfc90
BitcoinPullTester commented at 2:11 AM on September 15, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4914_eadbedfc902305b9ac845f89e15e97742f04b1e2/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
sipa commented at 5:15 AM on September 15, 2014: member

I would rather not extend getinfo anymore.
Diapolo commented at 6:06 AM on September 15, 2014: none

Yeah, as we deprecated it, we shouldn't extend it. Perhaps somehow flag it deprecated?
in src/rpcnet.cpp:None in eadbedfc90
```
 392 | @@ -392,6 +393,8 @@ Value getnetworkinfo(const Array& params, bool fHelp)
 393 |  
 394 |      Object obj;
 395 |      obj.push_back(Pair("version",       (int)CLIENT_VERSION));
```
Diapolo commented at 6:08 AM on September 15, 2014:

While you are on getnetworkinfo, the int casts before CLIENT_VERSION and PROTOCOL_VERSION can be removed :).
laanwj commented at 6:46 AM on September 15, 2014: member

ACK on the getnetworkinfo change, NACK on getinfo change. Getinfo only exists for backwards compatibility, please don't change it anymore.
jgarzik commented at 12:56 PM on September 15, 2014: contributor

It is inconsistent to export version/clientversion in one place, and version/clientversion/subversion in a second place.
laanwj commented at 1:24 PM on September 15, 2014: member

It is, but we've NACKed many changes that added or changed info in getinfo, merging this would also be inconsistent. There are many more of those inconsistencies building up in getinfo output (see for example the testnet boolean). They'll just accrue until we remove the command at some point...

laanwj commented at 1:26 PM on September 15, 2014: member

@diapolo as for flagging it deprecated, I'd say this monster note at the top is quite clear:

 * [@note](/bitcoin-bitcoin/contributor/note/) Do not add or change anything in the information returned by this
 * method. `getinfo` exists for backwards-compatibilty only. It combines
 * information from wildly different sources in the program, which is a mess,
 * and is thus planned to be deprecated eventually.
 *
 * Based on the source of the information, new information should be added to:
 * - `getblockchaininfo`,
 * - `getnetworkinfo` or
 * - `getwalletinfo`
 *
 * Or alternatively, create a specific query method for the information.

laanwj referenced this in commit 58d4b23284 on Sep 25, 2014
laanwj commented at 8:18 AM on September 25, 2014: member

Merged via 58d4b23 (sans getinfo changes)
laanwj closed this on Sep 25, 2014
MarcoFalke locked this on Sep 8, 2021

Contributors

Linked (view graph)

#4829 Include version string of version message we send in the debug log