rpc: introduce getversion RPC #30112
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
Concept ACK Should we eventually deprecate |
| # The functional tests don't have access to the version number and it | ||
| # can't be hardcoded. Only doing sanity and consistency checks in this | ||
| # test. |
There was a problem hiding this comment.
You could do this by inspecting the logs?
git diff on 54cb1ab
diff --git a/test/functional/rpc_getversion.py b/test/functional/rpc_getversion.py
index 712c1f76fc..16ec046078 100755
--- a/test/functional/rpc_getversion.py
+++ b/test/functional/rpc_getversion.py
@@ -4,6 +4,8 @@
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test the getversion RPC."""
+import re
+
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal
@@ -12,11 +14,23 @@ class GetVersionTest (BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
+ def parse_version_from_log(self):
+ version_pattern = re.compile(r"Bitcoin Core version (\S+)")
+
+ with open(self.nodes[0].debug_log_path, 'r') as file:
+ for line in file:
+ match = version_pattern.search(line)
+ if match:
+ return match.group(1)
+
+ assert False, "No version found in log file."
+
def run_test(self):
+ log_version = self.parse_version_from_log()
version = self.nodes[0].getversion()
- # The functional tests don't have access to the version number and it
- # can't be hardcoded. Only doing sanity and consistency checks in this
- # test.
+
+ # compare log output and RPC output
+ assert_equal(log_version, version["long"])
# numerical: e.g. 279900
assert_equal(type(version["numeric"]), int)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
tdb3@1dc6469
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
diff --git a/test/config.ini.in b/test/config.ini.in
index 291599da45..49bf203cf8 100644
--- a/test/config.ini.in
+++ b/test/config.ini.in
@@ -8,6 +8,9 @@
[environment]
PACKAGE_NAME=@PACKAGE_NAME@
PACKAGE_BUGREPORT=@PACKAGE_BUGREPORT@
+CLIENT_VERSION_MAJOR=@CLIENT_VERSION_MAJOR@
+CLIENT_VERSION_MINOR=@CLIENT_VERSION_MINOR@
+CLIENT_VERSION_BUILD=@CLIENT_VERSION_BUILD@
SRCDIR=@abs_top_srcdir@
BUILDDIR=@abs_top_builddir@
EXEEXT=@EXEEXT@
diff --git a/test/functional/rpc_getversion.py b/test/functional/rpc_getversion.py
index 712c1f76fc..b01a1245a2 100755
--- a/test/functional/rpc_getversion.py
+++ b/test/functional/rpc_getversion.py
@@ -26,6 +26,8 @@ class GetVersionTest (BitcoinTestFramework):
# short: e.g. "27.99.0"
assert_equal(version["short"], f"{major}.{minor}.{build}")
+ client_version = '.'.join(self.config["environment"][f"CLIENT_VERSION_{x}"] for x in ['MAJOR', 'MINOR', 'BUILD'])
+ assert_equal(version["short"], client_version)
# long: e.g. "v27.99.0-b10ca895160a-dirty"
assert version["long"].startswith(f"v{major}.{minor}.{build}")
|
The windows failure is: |
Should fixed with: --- a/build_msvc/bitcoin_config.h.in
+++ b/build_msvc/bitcoin_config.h.in
@@ -11,6 +11,9 @@
/* Version is release */
#define CLIENT_VERSION_IS_RELEASE $
+/* Release candidate */
+#define CLIENT_VERSION_RC $
+
/* Major version */
#define CLIENT_VERSION_MAJOR $
|
|
Definite concept ACK from me. |
|
Concept ACK |
0xB10C
force-pushed
the
2024-05-getversion-rpc
branch
from
54cb1ab
to
d83c12d
Compare
Thanks, very helpful! I didn't spot the windows failure and wasn't sure why CI is failing. Included this in the first commit. |
I'm not too sure about removing |
|
Concept ACK, would need a release note.
Note that removing |
|
|
|
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. |
There was a problem hiding this comment.
Concept ACK.
Great work. This seems like an improvement over whitelisting getnetworkinfo and reduces exposed info.
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.
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)
$ src/bitcoin-cli getwalletinfo
error code: -32601
error message:
Method 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:
$ src/bitcoin-cli getversion
{
"short": "27.99.0",
"long": "v27.99.0-d83c12d63526",
"numeric": 279900,
"client": "Satoshi",
"release_candidate": 0,
"is_release": false
}
$ curl --user $(cat ~/.bitcoin/signet/.cookie) --data-binary '{"method":"getversion","id":"tester"}' -H 'conten
t-type: text/plain;' http://127.0.0.1:38332/
{"result":{"short":"27.99.0","long":"v27.99.0-d83c12d63526","numeric":279900,"client":"Satoshi","release_candidate":0,"is_release":false},"error":null,"id":"tester"}
| {RPCResult::Type::NUM, "numeric", "Node version as integer: '10000 * MAJOR + 100 * MINOR + 1 * BUILD'."}, | ||
| {RPCResult::Type::STR, "client", "The nodes client name."}, | ||
| {RPCResult::Type::NUM, "release_candidate", "The release candidate. 0 means this version is not a release candidate."}, | ||
| {RPCResult::Type::BOOL, "is_release", "True for release versions and false for development versions."}, |
There was a problem hiding this comment.
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.
The problem with those, imo, is that
Sure, i thought this was the beginning of/preparation for such a potential decoupling? If not, using the information on |
|
Would this make sense as part of |
Yeah, good point. 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). |
| # The functional tests don't have access to the version number and it | ||
| # can't be hardcoded. Only doing sanity and consistency checks in this | ||
| # test. |
There was a problem hiding this comment.
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:
diff --git a/test/config.ini.in b/test/config.ini.in
index 291599da45..49bf203cf8 100644
--- a/test/config.ini.in
+++ b/test/config.ini.in
@@ -8,6 +8,9 @@
[environment]
PACKAGE_NAME=@PACKAGE_NAME@
PACKAGE_BUGREPORT=@PACKAGE_BUGREPORT@
+CLIENT_VERSION_MAJOR=@CLIENT_VERSION_MAJOR@
+CLIENT_VERSION_MINOR=@CLIENT_VERSION_MINOR@
+CLIENT_VERSION_BUILD=@CLIENT_VERSION_BUILD@
SRCDIR=@abs_top_srcdir@
BUILDDIR=@abs_top_builddir@
EXEEXT=@EXEEXT@
diff --git a/test/functional/rpc_getversion.py b/test/functional/rpc_getversion.py
index 712c1f76fc..b01a1245a2 100755
--- a/test/functional/rpc_getversion.py
+++ b/test/functional/rpc_getversion.py
@@ -26,6 +26,8 @@ class GetVersionTest (BitcoinTestFramework):
# short: e.g. "27.99.0"
assert_equal(version["short"], f"{major}.{minor}.{build}")
+ client_version = '.'.join(self.config["environment"][f"CLIENT_VERSION_{x}"] for x in ['MAJOR', 'MINOR', 'BUILD'])
+ assert_equal(version["short"], client_version)
# long: e.g. "v27.99.0-b10ca895160a-dirty"
assert version["long"].startswith(f"v{major}.{minor}.{build}")
|
Concept NACK, consumers should just check if features are available |
|
@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? |
|
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). |
|
In theory the consumer could call the Concept ACK |
The current recommendation is already to use the 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 Maybe a path forward for this PR is:
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? |
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. rust-bitcoin/rust-bitcoincore-rpc#355).
The current recommendation is to use the
versionfield from thegetnetworkinfoRPC 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,getnetworkinfoneeds to be whitelisted for all RPC users.To allow RPC consumers to determine the node version and the available RPC commands and fields, a
getversionRPC is introduced.with v27.0
with v27.0rc1