rpc: introduce getversion RPC

0xB10C commented at 3:07 pm on May 15, 2024: contributor

The Bitcoin Core RPC interface is implicitly versioned on the major version of Bitcoin Core. Some downstream RPC consumers and clients, for example rust-bitcoincore-rpc, need to know about the underlying node version to determine the available RPC calls and how to interpret the RPC responses (e.g. https://github.com/rust-bitcoin/rust-bitcoincore-rpc/issues/355).

The current recommendation is to use the version field from the getnetworkinfo RPC call. However, this RPC call also returns, for example, the IP addresses of the node, which makes it unsuitable for ‘public’ access to a semi-trusted RPC consumer. With the current recommendation, getnetworkinfo needs to be whitelisted for all RPC users.

To allow RPC consumers to determine the node version and the available RPC commands and fields, a getversion RPC is introduced.

0$ bitcoin-cli getversion
1{
2  "short": "27.99.0",
3  "long": "v27.99.0-7adfdfca190b",
4  "numeric": 279900,
5  "client": "Satoshi",
6  "release_candidate": 0,
7  "is_release": false
8}

0{
1  "short": "27.0.0",
2  "long": "v27.0.0",
3  "numeric": 270000,
4  "client": "Satoshi",
5  "release_candidate": 0,
6  "is_release": true
7}

0{
1  "short": "27.0.0",
2  "long": "v27.0.0rc1",
3  "numeric": 270000,
4  "client": "Satoshi",
5  "release_candidate": 1,
6  "is_release": true
7}

DrahtBot commented at 3:07 pm on May 15, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Concept ACK	stickies-v, apoelstra, BrandonOdiwuor, brunoerg, jonatack, tdb3, theStack, maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot added the label RPC/REST/ZMQ on May 15, 2024

stickies-v commented at 3:29 pm on May 15, 2024: contributor

Concept ACK

Should we eventually deprecate version and subversion fields from getnetworkinfo? If so, updating the getnetworkinfo docs is probably in order? No need to rush deprecating anything just yet, but just to indicate the direction?

in test/functional/rpc_getversion.py:19 in 54cb1abffa outdated

14+
15+    def run_test(self):
16+        version = self.nodes[0].getversion()
17+        # The functional tests don't have access to the version number and it
18+        # can't be hardcoded. Only doing sanity and consistency checks in this
19+        # test.

stickies-v commented at 4:01 pm on May 15, 2024:

You could do this by inspecting the logs?

 0diff --git a/test/functional/rpc_getversion.py b/test/functional/rpc_getversion.py
 1index 712c1f76fc..16ec046078 100755
 2--- a/test/functional/rpc_getversion.py
 3+++ b/test/functional/rpc_getversion.py
 4@@ -4,6 +4,8 @@
 5 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 6 """Test the getversion RPC."""
 7 
 8+import re
 9+
10 from test_framework.test_framework import BitcoinTestFramework
11 from test_framework.util import assert_equal
12 
13@@ -12,11 +14,23 @@ class GetVersionTest (BitcoinTestFramework):
14     def set_test_params(self):
15         self.num_nodes = 1
16 
17+    def parse_version_from_log(self):
18+        version_pattern = re.compile(r"Bitcoin Core version (\S+)")
19+
20+        with open(self.nodes[0].debug_log_path, 'r') as file:
21+            for line in file:
22+                match = version_pattern.search(line)
23+                if match:
24+                    return match.group(1)
25+
26+        assert False, "No version found in log file."
27+
28     def run_test(self):
29+        log_version = self.parse_version_from_log()
30         version = self.nodes[0].getversion()
31-        # The functional tests don't have access to the version number and it
32-        # can't be hardcoded. Only doing sanity and consistency checks in this
33-        # test.
34+
35+        # compare log output and RPC output
36+        assert_equal(log_version, version["long"])
37 
38         # numerical: e.g. 279900
39         assert_equal(type(version["numeric"]), int)

0xB10C commented at 7:18 pm on May 15, 2024:

I agree that this would work. I’m a bit reluctant to introduce more log parsing for tests. I’m leaving unresolved: If someone feels strongly about doing this, let me know.

An alternative I have though about is adding the required version information to test/config.ini. Again, also open to do this if someone thinks it’s important to do it.

tdb3 commented at 3:37 pm on May 17, 2024:

In general, for tests I think the inclination should be to get data from direct sources (e.g. RPC) rather than from the debug log, but this instance seems like an edge case where searching the debug log might make sense (e.g. could match what’s in the log with what the RPC call response contains).

I briefly looked for this capability, but didn’t see it (I was surprised, so maybe I’m overlooking something simple). If others would like this capability, I’m thinking it’s probably something to have at a higher/general level than for just this functional test. Something like the following: https://github.com/tdb3/bitcoin/commit/1dc646979788105edc0083f8cf4d2760c97cadf2

If others like the concept/approach, I could submit a PR (and would make @stickies-v a coauthor since his suggestion above inspired it). I don’t want to pollute this PR, just adding for awareness.

tdb3 commented at 5:10 pm on May 17, 2024:

Was talking with @pinheadmz about this. Maybe as an alternative, the test could fetch the getversion RPC response (e.g. with “27.0…”), then use assert_debug_log() to check that the RPC response matches what’s in the log.

theStack commented at 5:20 pm on May 22, 2024:

I think the cleanest approach to access the version info from a functional test is via test/config.ini, which is generated by the build system based on substitutions in test/config.ini.in. The values are available as (nested) dictionary via self.config. Simple example:

 0diff --git a/test/config.ini.in b/test/config.ini.in
 1index 291599da45..49bf203cf8 100644
 2--- a/test/config.ini.in
 3+++ b/test/config.ini.in
 4@@ -8,6 +8,9 @@
 5 [environment]
 6 PACKAGE_NAME=@PACKAGE_NAME@
 7 PACKAGE_BUGREPORT=@PACKAGE_BUGREPORT@
 8+CLIENT_VERSION_MAJOR=@CLIENT_VERSION_MAJOR@
 9+CLIENT_VERSION_MINOR=@CLIENT_VERSION_MINOR@
10+CLIENT_VERSION_BUILD=@CLIENT_VERSION_BUILD@
11 SRCDIR=@abs_top_srcdir@
12 BUILDDIR=@abs_top_builddir@
13 EXEEXT=@EXEEXT@
14diff --git a/test/functional/rpc_getversion.py b/test/functional/rpc_getversion.py
15index 712c1f76fc..b01a1245a2 100755
16--- a/test/functional/rpc_getversion.py
17+++ b/test/functional/rpc_getversion.py
18@@ -26,6 +26,8 @@ class GetVersionTest (BitcoinTestFramework):
19 
20         # short: e.g. "27.99.0"
21         assert_equal(version["short"], f"{major}.{minor}.{build}")
22+        client_version = '.'.join(self.config["environment"][f"CLIENT_VERSION_{x}"] for x in ['MAJOR', 'MINOR', 'BUILD'])
23+        assert_equal(version["short"], client_version)
24 
25         # long: e.g. "v27.99.0-b10ca895160a-dirty"
26         assert version["long"].startswith(f"v{major}.{minor}.{build}")

maflcko commented at 4:13 pm on May 15, 2024: member

The windows failure is:

0D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]

hebasto commented at 4:22 pm on May 15, 2024: member

The windows failure is:

0D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]

Should fixed with:

 0--- a/build_msvc/bitcoin_config.h.in
 1+++ b/build_msvc/bitcoin_config.h.in
 2@@ -11,6 +11,9 @@
 3 /* Version is release */
 4 #define CLIENT_VERSION_IS_RELEASE $
 5 
 6+/* Release candidate */
 7+#define CLIENT_VERSION_RC $
 8+
 9 /* Major version */
10 #define CLIENT_VERSION_MAJOR $
11

maflcko added the label Needs CMake port on May 15, 2024

apoelstra commented at 4:29 pm on May 15, 2024: contributor

Definite concept ACK from me.

BrandonOdiwuor commented at 4:55 pm on May 15, 2024: contributor

Concept ACK

brunoerg commented at 5:06 pm on May 15, 2024: contributor

Concept ACK

bitcoin deleted a comment on May 15, 2024

build: expose CLIENT_VERSION_RC a5b1ab7217

refactor: expose FormatVersion 258ae3bc71

rpc: introduce getversion RPC 26b2724dd3

test: functional test for getversion RPC b262b2a1dc

doc: use getversion for version information d83c12d635

0xB10C force-pushed on May 15, 2024

0xB10C commented at 7:20 pm on May 15, 2024: contributor

The windows failure is:

0D:\a\bitcoin\bitcoin\src\rpc\node.cpp(223,45): error C2065: 'CLIENT_VERSION_RC': undeclared identifier [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_node\libbitcoin_node.vcxproj]

Should fixed with:

 0--- a/build_msvc/bitcoin_config.h.in
 1+++ b/build_msvc/bitcoin_config.h.in
 2@@ -11,6 +11,9 @@
 3 /* Version is release */
 4 #define CLIENT_VERSION_IS_RELEASE $
 5 
 6+/* Release candidate */
 7+#define CLIENT_VERSION_RC $
 8+
 9 /* Major version */
10 #define CLIENT_VERSION_MAJOR $
11

Thanks, very helpful! I didn’t spot the windows failure and wasn’t sure why CI is failing. Included this in the first commit.

0xB10C commented at 7:27 pm on May 15, 2024: contributor

Should we eventually deprecate version and subversion fields from getnetworkinfo? If so, updating the getnetworkinfo docs is probably in order? No need to rush deprecating anything just yet, but just to indicate the direction?

I’m not too sure about removing subversion, but I think we can deprecate version at some point as it’s identical to numeric in getversion. If I understand you correctly, you mean adding a note to the version docs that users should use getversion going forward?

jonatack commented at 7:49 pm on May 15, 2024: contributor

Concept ACK, would need a release note.

I’m not too sure about removing subversion, but I think we can deprecate version

Note that removing version from getnetworkinfo would break some bitcoin-cli calls to long-running nodes, unless handled with an IsNull() check and would also add an additional RPC call there. Client software of our RPC API are probably using it as well, so maybe not worth it to remove anything.

laanwj commented at 9:14 am on May 16, 2024: member

getrpcversion maybe? The reason i bring it up is that it needs to be restrictive enough in scope to just the RPC interface version, not wallet version, not P2P versions, etc.

apoelstra commented at 1:02 pm on May 16, 2024: contributor

I’d be fine with that, though I’m not sure that there’s a privacy loss to just throwing that extra info into the RPC.

At the very least, the RPC ought to indicate whether the wallet is compiled since that affects which RPC calls are available.

in src/rpc/node.cpp:212 in d83c12d635

207+                {RPCResult::Type::STR, "short", "Node version formatted as 'MAJOR.MINOR.BUILD'."},
208+                {RPCResult::Type::STR, "long", "Detailed node version. Optionally with git commit hash information."},
209+                {RPCResult::Type::NUM, "numeric", "Node version as integer: '10000 * MAJOR + 100 * MINOR + 1 * BUILD'."},
210+                {RPCResult::Type::STR, "client", "The nodes client name."},
211+                {RPCResult::Type::NUM, "release_candidate", "The release candidate. 0 means this version is not a release candidate."},
212+                {RPCResult::Type::BOOL, "is_release", "True for release versions and false for development versions."},

tdb3 commented at 1:45 am on May 17, 2024:

Thinking out loud about this one: Are there instances where long would be insufficient for determining if the client/node is a release? In the examples provided in the opening post, it seems like this could be determined by lack of commit hash and rc designation. It definitely would be convenient for the RPC user to have a bool provided (and prevents burdening the user with extra parsing or interpretation), so I see value in keeping this field.

tdb3 commented at 2:58 am on May 17, 2024: contributor

Concept ACK.

Great work. This seems like an improvement over whitelisting getnetworkinfo and reduces exposed info.

#30112 (comment)

getrpcversion maybe? The reason i bring it up is that it needs to be restrictive enough in scope to just the RPC interface version, not wallet version, not P2P versions, etc.

I generally agree with this comment that the scope of this new RPC should be constrained where it makes sense. A more specific RPC name could help to align with that goal (e.g. getrpcversion or perhaps getnodeversion, getclientversion, etc.). I’m partial to getnodeversion or getclientversion, since RPC is implicitly versioned by the major version of the client/node, and this RPC seems to generally be providing client/node versioning information. If in the future RPC version is no longer coupled to client/node version, then another RPC getrpcversion could still be implemented separately without having to change getnodeversion.

#30112 (comment)

the RPC ought to indicate whether the wallet is compiled since that affects which RPC calls are available.

Maybe I’m missing something, but in the spirit of minimizing info leaking, maybe this information shouldn’t be provided to an RPC user that is excluded from having access to wallet RPCs. If that user does have access to wallet RPCs (e.g. via rpcwhitelistdefault and rpcwhitelist configuration) and tries to access wallet RPCs, then if I’m not mistaken, the response would be 403 (regardless of whether wallet is built). If the user has access to wallet RPCs, but wallet isn’t built, the response would inform the user that the method isn’t available. Not as convenient for the user as having an RPC call tell the user directly, but leaks less info.

(404 for legacy jsonrpc (1.x), or 200 for jsonrpc 2.0)

0$ src/bitcoin-cli getwalletinfo
1error code: -32601
2error message:
3Method not found

Other than the thoughts above, reviewed the code, built and ran all functional tests (all passed). Calling the RPC with bitcoin-cli and curl worked well:

 0$ src/bitcoin-cli getversion
 1{
 2  "short": "27.99.0",
 3  "long": "v27.99.0-d83c12d63526",
 4  "numeric": 279900,
 5  "client": "Satoshi",
 6  "release_candidate": 0,
 7  "is_release": false
 8}
 9
10$ curl --user $(cat ~/.bitcoin/signet/.cookie) --data-binary '{"method":"getversion","id":"tester"}' -H 'conten
11t-type: text/plain;' http://127.0.0.1:38332/
12{"result":{"short":"27.99.0","long":"v27.99.0-d83c12d63526","numeric":279900,"client":"Satoshi","release_candidate":0,"is_release":false},"error":null,"id":"tester"}

laanwj commented at 9:01 am on May 17, 2024: member

I’m partial to getnodeversion or getclientversion, since RPC is implicitly versioned by the major version of the client/node

The problem with those, imo, is that client and node are both words associated with general networking. They might as well return P2P protocol version information. If this call is intended to return RPC versions and capabilities, then that should probably be explicit in the name and documentation. But maybe i’m misunderstanding the intent.

If in the future RPC version is no longer coupled to client/node version, then another RPC getrpcversion could still be implemented separately without having to change getnodeversion.

Sure, i thought this was the beginning of/preparation for such a potential decoupling? If not, using the information on getnetworkinfo already seems fine.

ajtowns commented at 1:22 pm on May 17, 2024: contributor

Would this make sense as part of getrpcinfo ? If the point of this is for RPC clients to check so they can be forwards/backwards compatible, then I’m not sure it really makes sense to look at anything other than the numeric field? Would the other fields make more sense as a response to a getnodeinfo command? (That would seem a more appropriate place for the logpath field getrpcinfo returns)

tdb3 commented at 1:27 pm on May 17, 2024: contributor

I’m partial to getnodeversion or getclientversion, since RPC is implicitly versioned by the major version of the client/node

The problem with those, imo, is that client and node are both words associated with general networking. They might as well return P2P protocol version information. If this call is intended to return RPC versions and capabilities, then that should probably be explicit in the name and documentation. But maybe i’m misunderstanding the intent.

If in the future RPC version is no longer coupled to client/node version, then another RPC getrpcversion could still be implemented separately without having to change getnodeversion.

Sure, i thought this was the beginning of/preparation for such a potential decoupling? If not, using the information on getnetworkinfo already seems fine.

Yeah, good point. getnodeversion or getclientversion are probably not the best names. I’m open to other naming.

Seems like the intent of this PR is to provide RPC version info for downstream applications, without having to leak extraneous info. That goal makes sense to me. The info provided by the RPC method seems to be the version of Core running rather than explicitly the RPC version (which makes sense since currently the RPC version is tied to Core).

The intent of the comment was to name the RPC method based on what it provides (Core version info), which happens to be useful for RPC. Decoupling RPC version from Core version doesn’t seem like something we should pursue now IMO. However, if that is ever done in the future, an RPC-version-specific RPC method could be added without having to change this one (this one provides Core version info). To me it seems that this scenario might cause less downstream app breaking in the future (but I’ve only thought briefly on it so far).

theStack commented at 5:21 pm on May 22, 2024: contributor

Concept ACK

luke-jr commented at 3:42 pm on May 23, 2024: member

Concept NACK, consumers should just check if features are available

apoelstra commented at 4:00 pm on May 23, 2024: contributor

@luke-jr are you suggesting that all consumers should call the entire RPC surface to confirm which calls are present, what the types of all inputs are, what the types of all outputs are, etc., in order to determine whether their software is compatible with the interface?

luke-jr commented at 4:10 pm on May 23, 2024: member

Consumers don’t need the entire RPC surface, only specific parts. If a compatibility check is desired, that’s indeed how it should be (and typically is) done - just like autotools/cmake checks for APIs. It could also be done at runtime in many cases: if a call fails, fallback to another one (and maybe flag so you don’t try the better call next time).

maflcko commented at 4:13 pm on May 23, 2024: member

In theory the consumer could call the help RPC (or a computer readable help, once there is one, see #29912) and use that to double check that the views match. However, this may be more work for consumers to implement, as opposed to a simple version check.

Concept ACK

0xB10C commented at 11:41 am on May 25, 2024: contributor

Concept NACK, consumers should just check if features are available

The current recommendation is already to use the version field from the getnetworkinfo RPC call. This PR doesn’t really add anything new that wasn’t already exposed and recommend for RPC clients to use.

Throwing this in for even more confusion about this new RPC and the software vs RPC interface version:

For my fork-observer project I currently use the version from the getnetworkinfo RPC to display the node version on the site. However, this feature can only be optional to avoid leaking IPs of only semi-trusted nodes (see e.g. the whitelisting recommendations here). I know that forkmonitor.info has an admin interface for setting and updating the versions shown to users. This seems brittle. Just showing the long version added in this PR would be easier.

Maybe a path forward for this PR is:

change getrpcinfo:
- add a version field to getrpcinfo:
  - this can either be numerical
  - or simply CLIENT_VERSION_MAJOR for easier decoupling of software and RPC version in the future
- optionally move the logpath from getrpcinfo to anther RPC call and deprecate it in getrpcinfo
expose logpath, long and/or short in e.g. getnodeinfo

In general, having a formal description of the RPC interface (i.e. #29912) for each version seems like a better and more long-term solution?

0xB10C commented at 3:37 pm on June 19, 2024: contributor

While this has a few Concept ACKs, I’m not sure the Approach (introducing a new RPC) is really the best route here. I’m closing this for now as I’ll be focusing on other things. Feel free to pick this up if you want.

0xB10C closed this on Jun 19, 2024

maflcko commented at 6:31 am on June 20, 2024: member

lgtm ACK d83c12d635263fda74392eff372792e983749adb

maflcko removed the label Needs CMake port on Jul 25, 2024

rpc: introduce getversion RPC #30112

Code Coverage

Reviews

Conflicts