rpc: support a formal description of our JSON-RPC interface#34683
rpc: support a formal description of our JSON-RPC interface#34683willcl-ark wants to merge 9 commits into
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34683. ReviewsSee the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK |
janb84
left a comment
There was a problem hiding this comment.
Concept ACK.
This was already useful for bitcoin-tui
| committed = json.loads(openrpc_path.read_text(encoding="utf-8")) | ||
|
|
||
| if generated != committed: | ||
| self.log.info("Generated doc/openrpc.json:\n%s", |
There was a problem hiding this comment.
I guess it could be distracting to have to see every rpc change twice. Maybe the test could just check that generating the json does not crash?
In theory, there could be a diff output like #26706 (comment) somewhere for each pull request, but that seems better to be optional and hidden.
the doc/openrpc.json could have the same This is a placeholder file. content like the manpages.
(Of course it is fine to keep both commits in this pull for now, so that the json is visible here, and only drop them before merge)
There was a problem hiding this comment.
Thank you for this comment.
Having the json checked in as a skeleton (like manpages) is a nice approach. It's kind of the opposite to stickies thoughts, so I guess I'll see which folks seem to generally prefer (and will leave the file in this PR for now for reference).
I guess it could be distracting to have to see every rpc change twice. Maybe the test could just check that generating the json does not crash?
This is also a nice idea/possibility. If we are not worrying about having a live artifact in this repo, then I agree this is almost certainly how it should work.
There was a problem hiding this comment.
I am also thinking about devs on platforms that do not support a full build (IIRC external signer was not present on Windows for some time?). I guess they could manually undo the hunk that removes the $feature-related RPC from the json, or copy-paste the full json output from the CI log somehow.
In any case, either way seems fine and should be trivial to adjust post-merge. This is just a nit.
|
Concept ACK |
|
Big concept ACK, very nice work. If we want to cram less functionality in Bitcoin Core, we need to make it easier for people to build on top of it. A change like this will make it much easier for consumers to use the RPC. In addition, I think it will also help with our RPC stability guarantees and testing, and to make it easier for consumers to quickly find and incorporate any changes that have been made to the interface.
This seems like a feature, not a bug? As long as devs can compile the binaries they need to test/dev, I think this doesn't impose any unnecessary overhead?
If OpenRPC is the best choice for our repo (from quick glance, it seems to work well), and we have enough support to commit to keeping our RPC OpenRPC compatible, I think it absolutely should live in this repo, so that we can quickly and strictly enforce continued compatibility with the spec. I'm less opinionated on whether the output json should live in this repo. I think it's fine to do so - the changes should be a LOT less frequent than e.g. doxygen changes, and it's nice having it easily accessible. However, I think it's also acceptable for users to generate this on-the-fly, or use any other hosted mirror of this documentation, which should be trivial to do. |
I don't really see why it wouldn't be better to have a If people want to grab the artifact from the web instead of their node, then providing a url on bitcoincore.org (like https://bitcoincore.org/en/doc/30.0.0/rpc/network/disconnectnode/ eg) seems plausible enough. Having the json output in the repo just seems annoying to me; more data, more conflicts, more diffs to check, more things you have to manually mess around with that could be automated. For the test suite, picking a rarely updated but somewhat interesting RPC, and checking that the "getopenrpcinfo" result for that command matches a fixed value, and that the commands listed match the output of "help" is probably fine, or at least seems like it would be to me. OpenRPC seems fine to me; doesn't seem like there's much benefit in worrying about the details, beyond getting something that's moderately machine readable. |
Good question, sorry I didn't answer in OP initially, I had meant to provide a comparison of sorts... I tried this approach, and basically my reasoning was that you end up with a much larger c++ change (which I thought might be less desirable), although you get the nice benefits you note. I think it could be useful to codify the possible approaches I see, and their impact on this codebase. So roughly in ascending order of LoC needed in this repo:
I am going to rework my branch which outputs the json directly from RPC, and will post an update here with a comparison.
agree, this would be fine.
Yeah I think I agree here too. I did test breaking an RPC to see how annoying it was to detect and fix. Quite annoying, was the answer.
👍🏼 Thank you for articulating this, as I did spend a fair bit of time "worrying" about which format to select, but I see now you're correct; as long as it's structured (i.e valid json) and machine readable, it should be OK, and people can easily transform it into their own formats. |
|
Hey @willcl-ark, have you checked my comment on corepc? Is something like that you wanted to build? |
2846fab to
a3d6298
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
5f58a98 to
0857f2b
Compare
@ajtowns I have updated the draft here now to work like this. It was not as much extra c++ code in sever.cpp as I thought, which seems nice, and I agree it's a better approach. Comparing the python output to the c++ output only differed on formatting. New output file from a
@natobritto I had not heard of this one, no. I knew there were one or two? unmaintained rust bitcoin core json rpc libs, but pretty sure neither was this one. I will check it out. |
|
I like the new C++ approach! I found one conundrum: As per openRPC output the command Details {
"name": "getopenrpcinfo",
"description": "Returns an OpenRPC document for currently available RPC commands.",
"params": [
],
"result": {
"name": "result",
"schema": {
"type": "object",
"properties": {
"openrpc": {
"type": "string",
"description": "OpenRPC specification version."
},
"info": {
"type": "object",
"properties": {
},
"additionalProperties": false,
"description": "Metadata about this JSON-RPC interface."
},
"methods": {
"type": "array",
"items": {
"type": "object",
"properties": {
},
"additionalProperties": false
},
"description": "Documented RPC methods."
}
},
"additionalProperties": false,
"required": [
"openrpc",
"info",
"methods"
]
}
},
"x-bitcoin-category": "control"
},The output is enriched with an info object filled (so it has additional properties) : "info": {
"title": "Bitcoin Core JSON-RPC",
"version": "v30.99.0-dev",
"description": "Autogenerated from Bitcoin Core RPC metadata."
},This invalidates the generated openRPC spec |
0857f2b to
726260c
Compare
Doh! You're right that the getopenrpcinfo schema was indeed self-invalidating! I've updated the RPCResult to fully describe the info fields (title, version, description) and the method object structure (name, description, params, result, x-bitcoin-category), including the param and result Content Descriptor shapes. Dynamic JSON Schema fields within those use unconstrained schemas since their structure varies per method. I don't think there's anything we can do about that. There are also a few bits related to optional args before required, but those are legacy RPC artefacts. I also added a sample helper script I used to validate that it's now actually valid, per the meta-schema (you can run this yourself with |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
726260c to
d4a7546
Compare
hodlinator
left a comment
There was a problem hiding this comment.
Concept ACK
My gut says we should be maintaining a shared spec and generate stub C++ binding code from it, as is done for .capnp files. However, the current reverse approach of generating the spec from the C++ declarations has the following benefits:
- Doesn't add more codegen complexity to the build.
- Doesn't add complexity of keeping generated C++ code and version controlled code in sync.
- Doesn't touch existing C++ RPC declarations.
- If we were to decide we want to switch to non-C++ spec + codegen, the current approach gives us the raw material for that spec.
| "info": { | ||
| "title": "Bitcoin Core JSON-RPC", | ||
| "version": "v30.99.0-dev", | ||
| "description": "Placeholder OpenRPC skeleton. Regenerate this file for release candidates using `bitcoin-cli getopenrpcinfo`." |
There was a problem hiding this comment.
Not clear to me if this should be in this repo, or done like the rpc help at https://github.com/bitcoin-core/bitcoincore.org/blob/master/contrib/doc-gen/generate.go
There was a problem hiding this comment.
Yes, I think the final 4 commits here are up for conceptual-inclusion review; we could... drop them all. The other extreme is to vendor the schema into this repo and use that in the functional test. Or we could keep something like the basic sanity test I have here and drop the final 3.
IMO if we want to distribute in guix tarballs keeping the skeleton file checked in isn't too bad. But I'm not strongly wedded to the idea either. If we didn't want to distribute a static file, we could drop both the skeleton file and guix commits.
There was a problem hiding this comment.
I think it's very useful to be able to interrogate bitcoind for its schema, it makes the schema much more discoverable, and makes it much more likely that the code will be kept up to date. It's also probably the path to integrate it with the least code, since there's minimal additional code for a new API call, and the C++ objects which represent the RPC API are all available to the schema generator.
| for (const auto& [name, cmds] : mapCommands) { | ||
| if (cmds.empty()) continue; | ||
| const CRPCCommand* cmd{cmds.front()}; | ||
| if (cmd->category == "hidden" || !cmd->metadata_fn) continue; |
There was a problem hiding this comment.
Just encountered a problem with codegen because we omit hidden RPCs and arguments. I believe we should include them in getopenrpcinfo. Or at least provide an optional argument show_hidden in the RPC for completeness and to enable codegen testing on regtest.
| if (cmd->category == "hidden" || !cmd->metadata_fn) continue; | |
| if ((!include_hidden && cmd->category == "hidden") || !cmd->metadata_fn) continue; |
There was a problem hiding this comment.
Added show_hidden. Kept hidden args omitted by default but they can be included when getopenrpcinfo(true) is called.
|
|
||
| UniValue params{UniValue::VARR}; | ||
| for (const auto& arg : helpman.GetArgs()) { | ||
| if (arg.m_opts.hidden) continue; |
There was a problem hiding this comment.
This fixes the hidden fields.
| if (arg.m_opts.hidden) continue; | |
| if (!include_hidden && arg.m_opts.hidden) continue; |
…elds 2447385 rpc: remove unused RPCResult::Type::ELISION (satsfy (Renato Britto)) 7a85118 rpc: expand decodepsbt output script with explicit fields (satsfy (Renato Britto)) 88e2a6a rpc: expand getaddressinfo embedded with explicit fields (Renato Britto) a9f9e7d rpc: extract fee estimate result helpers (Renato Britto) 8a615a8 rpc: extract ListSinceBlockTxFields() helper (Renato Britto) 372ac28 rpc: extend TxDoc() for getblock verbosity 2/3 (Renato Britto) 0380a1c rpc: extend TxDoc() for getrawtransaction verbosity 2 (Renato Britto) 44fc3a2 rpc: introduce HelpElision variant and ElideGroup helper (Renato Britto) Pull request description: Partially addresses #29912. Motivated by #34683, which exports OpenRPC from existing `RPCHelpMan` metadata. [Sample OpenRPC](https://gist.github.com/natobritto/8c4a1da04968d2325082ac4bca7d2408). Some RPC help definitions rely on `RPCResult::Type::ELISION` entries whose structure is only described in prose. This keeps human-readable help concise, but leaves parts of the result layout implicit and prevents tools from deriving complete machine-readable schemas from `RPCHelpMan` metadata. This PR replaces ELISION-based reuse with shared structured definitions, so result layouts are represented directly in metadata rather than only in text. At the same time, human-readable help remains compact via explicit help-rendering elision using `HelpElision`, so previously elided sections stay abbreviated without losing schema completeness. Affected RPCs: `getrawtransaction`, `getblock`, `listsinceblock`, `estimaterawfee`, `getaddressinfo`. RPC return values are unchanged. Human-readable help remains compact, while structured result metadata becomes explicit enough to derive complete machine-readable schemas. A related `RPCResult::Type::ELISION` use in `importdescriptors` was split out into the follow-up PR #34867 because it changes the generated help output, per [this review comment](#34764 (comment)). Changes: - Introduce `HelpElision` (`NONE`, `START`, `SKIP`) and `ElideGroup()`, replacing the tri-state `print_elision` - Add an `RPCResult` copy-with-replacement-options constructor to support applying elision while keeping `m_opts` const - Extend `TxDoc()` / `TxDocOptions` to support reusable transaction layouts with optional `prevout`, `fee`, `hex`, and elision behavior - Replace ELISION-based reuse in `getrawtransaction` and `getblock` with explicit structured definitions - Factor shared result layouts into `GetBlockFields()`, `ListSinceBlockTxFields()`, `FeeRateBucketDoc()`, `GetAddressInfoEmbeddedFields()` and `FeeEstimateHorizonDoc()` - Expand `listsinceblock.removed`, `estimaterawfee` horizons/buckets and `getaddressinfo.embedded` into explicit metadata while preserving concise help output ACKs for top commit: achow101: ACK 2447385 w0xlt: reACK 2447385 janb84: re ACK 2447385 willcl-ark: ACK 2447385 Tree-SHA512: 8dc03c45c388ebdb4f8f1613af2576fc127a8d5425efe518cd0d0ed2439a38b2ed1236413471672c85f52ee22c4a17677c18fd4689bc6570496fc3af4cd4112f
CRPCTable::help() takes std::string_view but server.h relies on transitive includes for it. Add the direct include (probably makes iwyu happier too?)
After removing the last CRPCCommand pointer for a given name, erase the now-empty vector from mapCommands. Without this, listCommands() returns the name of a fully removed command because it iterates mapCommands keys unconditionally. For example, when unloading the wallet the RPCs are deregistered, and this prevents getopenrpcinfo from returning non-existant RPCs.
RPCResult::Type::ANY triggers NONFATAL_UNREACHABLE() in ToSections(), which crashes the help() RPC when a command uses Type::ANY in a nested result field. Previously this was never hit because Type::ANY was only used as a top-level alternate result type, filtered out before ToSections() is called. getopenrpcinfo() will use this result type, so render it like other types allowing it to be used in nested result definitions like schema.
bbd7e55 to
dc99429
Compare
|
Thank for your review and comments @satsfy. I took many of your suggestions in the latest push, which also now includes the merged ELISION changes. |
| case RPCResult::Type::STR: | ||
| return MakeObject({{"type", "string"}}); | ||
| case RPCResult::Type::STR_AMOUNT: | ||
| return MakeObject({{"type", "string"}, {"x-bitcoin-unit", "amount"}}); |
There was a problem hiding this comment.
RPCResult::Type::STR_AMOUNT is represented as a JSON number, not a JSON string. The result type checker expects it to be UniValue::VNUM:
static std::optional<UniValue::VType> ExpectedType(RPCResult::Type type)
{
...
case Type::NUM:
case Type::STR_AMOUNT:
case Type::NUM_TIME: {
return UniValue::VNUM;
}
}So the OpenRPC schema should use "type": "number" while keeping the amount annotation:
case RPCResult::Type::STR_AMOUNT:
return MakeObject({{"type", "number"}, {"x-bitcoin-unit", "amount"}});Suggestion:
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index ca383a065a..8f62948fbd 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -411,7 +411,7 @@ UniValue OpenRPCResultSchema(const RPCResult& result)
case RPCResult::Type::STR:
return MakeObject({{"type", "string"}});
case RPCResult::Type::STR_AMOUNT:
- return MakeObject({{"type", "string"}, {"x-bitcoin-unit", "amount"}});
+ return MakeObject({{"type", "number"}, {"x-bitcoin-unit", "amount"}});
case RPCResult::Type::STR_HEX:
return MakeObject({{"type", "string"}, {"pattern", "^[0-9a-fA-F]+$"}});
case RPCResult::Type::NUM:
diff --git a/test/functional/rpc_openrpc.py b/test/functional/rpc_openrpc.py
index 6d48470578..c041162a0f 100755
--- a/test/functional/rpc_openrpc.py
+++ b/test/functional/rpc_openrpc.py
@@ -55,7 +55,7 @@ class OpenRPCDocTest(BitcoinTestFramework):
analyzepsbt = find_method(openrpc, "analyzepsbt")
result_schema = analyzepsbt["result"]["schema"]
estimated_feerate = result_schema["properties"]["estimated_feerate"]
- assert_equal(estimated_feerate["type"], "string")
+ assert_equal(estimated_feerate["type"], "number")
assert_equal(estimated_feerate["x-bitcoin-unit"], "amount")| UniValue OpenRPCArgSchema(const RPCArg& arg, bool include_hidden) | ||
| { | ||
| UniValue schema{UniValue::VOBJ}; | ||
| switch (arg.m_type) { |
There was a problem hiding this comment.
It looks like getopenrpcinfo does not account for skip_type_check.
For example, createrawtransaction.outputs is declared as RPCArg::Type::ARR with skip_type_check = true. Runtime accepts both an array and a direct object, but OpenRPC still emits an array-only schema.
Similarly, getwalletinfo.scanning is declared as RPCResult::Type::OBJ with skip_type_check = true. It can also return false when no scan is active, but OpenRPC still emits an object-only schema.
So the generated schema can be stricter than the actual RPC behavior. Maybe skip_type_check fields should either emit a less restrictive schema, like {}, or explicit alternatives with oneOf where the accepted shapes are known.
Suggestion:
diff
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index ca383a065a..a1a927fa8c 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -329,6 +329,18 @@ void ApplyArgFallback(UniValue& schema, const RPCArg& arg)
UniValue OpenRPCArgSchema(const RPCArg& arg, bool include_hidden)
{
UniValue schema{UniValue::VOBJ};
+ if (arg.m_opts.skip_type_check) {
+ ApplyTypeStrOverride(schema, arg);
+ if (schema.empty() && arg.m_type == RPCArg::Type::ARR) {
+ UniValue one_of{UniValue::VARR};
+ one_of.push_back(MakeObject({{"type", "array"}}));
+ one_of.push_back(MakeObject({{"type", "object"}}));
+ schema.pushKV("oneOf", std::move(one_of));
+ }
+ ApplyArgFallback(schema, arg);
+ return schema;
+ }
+
switch (arg.m_type) {
case RPCArg::Type::STR:
schema = MakeObject({{"type", "string"}});
@@ -407,6 +419,24 @@ UniValue OpenRPCArgSchema(const RPCArg& arg, bool include_hidden)
// NOLINTNEXTLINE(misc-no-recursion)
UniValue OpenRPCResultSchema(const RPCResult& result)
{
+ if (result.m_opts.skip_type_check) {
+ RPCResultOptions opts{result.m_opts};
+ opts.skip_type_check = false;
+ if (result.m_type == RPCResult::Type::OBJ) {
+ UniValue obj_schema{OpenRPCResultSchema(RPCResult{result, std::move(opts)})};
+ if (result.m_key_name.empty()) return obj_schema;
+
+ UniValue one_of{UniValue::VARR};
+ one_of.push_back(std::move(obj_schema));
+ one_of.push_back(MakeObject({{"const", false}}));
+ UniValue schema{UniValue::VOBJ};
+ schema.pushKV("oneOf", std::move(one_of));
+ return schema;
+ }
+ if (result.m_type == RPCResult::Type::ARR) return OpenRPCResultSchema(RPCResult{result, std::move(opts)});
+ return UniValue{UniValue::VOBJ};
+ }
+
switch (result.m_type) {
case RPCResult::Type::STR:
return MakeObject({{"type", "string"}});
diff --git a/test/functional/rpc_openrpc.py b/test/functional/rpc_openrpc.py
index 6d48470578..51d3e97ca3 100755
--- a/test/functional/rpc_openrpc.py
+++ b/test/functional/rpc_openrpc.py
@@ -44,6 +44,21 @@ class OpenRPCDocTest(BitcoinTestFramework):
hash_or_height = find_param(getblockstats, "hash_or_height")
assert_equal(hash_or_height["schema"], {"oneOf": [{"type": "number"}, {"type": "string"}]})
+ self.log.info("Checking skipped type check schemas")
+ createrawtransaction = find_method(openrpc, "createrawtransaction")
+ outputs = find_param(createrawtransaction, "outputs")
+ assert_equal(outputs["schema"], {"oneOf": [{"type": "array"}, {"type": "object"}]})
+
+ getdescriptoractivity = find_method(openrpc, "getdescriptoractivity")
+ activity = getdescriptoractivity["result"]["schema"]["properties"]["activity"]
+ assert_equal(activity["type"], "array")
+
+ if "getwalletinfo" in [method["name"] for method in openrpc["methods"]]:
+ getwalletinfo = find_method(openrpc, "getwalletinfo")
+ scanning = getwalletinfo["result"]["schema"]["properties"]["scanning"]
+ assert_equal(scanning["oneOf"][0]["type"], "object")
+ assert_equal(scanning["oneOf"][1], {"const": False})
+
self.log.info("Checking argument fallback annotations")
getblock = find_method(openrpc, "getblock")
verbosity = find_param(getblock, "verbosity")| break; | ||
| } | ||
| case RPCArg::Type::RANGE: { | ||
| UniValue prefix_items{UniValue::VARR}; |
There was a problem hiding this comment.
If I understand the specification correctly, OpenRPC Schema Objects are based on JSON Schema Draft 7, where tuple validation uses array-valued items plus additionalItems, not prefixItems.
https://spec.open-rpc.org/#schema-object
https://json-schema.org/draft-07/draft-handrews-json-schema-validation-01#rfc.section.6.4.1
Suggestion:
diff
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index ca383a065a..5394da5bab 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -350,12 +350,13 @@ UniValue OpenRPCArgSchema(const RPCArg& arg, bool include_hidden)
break;
}
case RPCArg::Type::RANGE: {
- UniValue prefix_items{UniValue::VARR};
- prefix_items.push_back(MakeObject({{"type", "number"}}));
- prefix_items.push_back(MakeObject({{"type", "number"}}));
+ UniValue items{UniValue::VARR};
+ items.push_back(MakeObject({{"type", "number"}}));
+ items.push_back(MakeObject({{"type", "number"}}));
UniValue range_schema{UniValue::VOBJ};
range_schema.pushKV("type", "array");
- range_schema.pushKV("prefixItems", std::move(prefix_items));
+ range_schema.pushKV("items", std::move(items));
+ range_schema.pushKV("additionalItems", false);
range_schema.pushKV("minItems", 2);
range_schema.pushKV("maxItems", 2);
UniValue one_of{UniValue::VARR};
@@ -434,13 +435,14 @@ UniValue OpenRPCResultSchema(const RPCResult& result)
return schema;
}
case RPCResult::Type::ARR_FIXED: {
- UniValue prefix_items{UniValue::VARR};
+ UniValue items{UniValue::VARR};
for (const auto& inner : result.m_inner) {
- prefix_items.push_back(OpenRPCResultSchema(inner));
+ items.push_back(OpenRPCResultSchema(inner));
}
UniValue schema{UniValue::VOBJ};
schema.pushKV("type", "array");
- schema.pushKV("prefixItems", std::move(prefix_items));
+ schema.pushKV("items", std::move(items));
+ schema.pushKV("additionalItems", false);
schema.pushKV("minItems", uint64_t(result.m_inner.size()));
schema.pushKV("maxItems", uint64_t(result.m_inner.size()));
return schema;
diff --git a/test/functional/rpc_openrpc.py b/test/functional/rpc_openrpc.py
index 6d48470578..3568d47498 100755
--- a/test/functional/rpc_openrpc.py
+++ b/test/functional/rpc_openrpc.py
@@ -44,6 +44,26 @@ class OpenRPCDocTest(BitcoinTestFramework):
hash_or_height = find_param(getblockstats, "hash_or_height")
assert_equal(hash_or_height["schema"], {"oneOf": [{"type": "number"}, {"type": "string"}]})
+ self.log.info("Checking Draft 7 tuple schemas")
+ deriveaddresses = find_method(openrpc, "deriveaddresses")
+ range_array = find_param(deriveaddresses, "range")["schema"]["oneOf"][1]
+ assert "prefixItems" not in range_array
+ assert_equal(range_array, {
+ "type": "array",
+ "items": [{"type": "number"}, {"type": "number"}],
+ "additionalItems": False,
+ "minItems": 2,
+ "maxItems": 2,
+ })
+
+ feerate_percentiles = getblockstats["result"]["schema"]["properties"]["feerate_percentiles"]
+ assert "prefixItems" not in feerate_percentiles
+ assert_equal(feerate_percentiles["type"], "array")
+ assert_equal(feerate_percentiles["items"], [{"type": "number"}] * 5)
+ assert_equal(feerate_percentiles["additionalItems"], False)
+ assert_equal(feerate_percentiles["minItems"], 5)
+ assert_equal(feerate_percentiles["maxItems"], 5)
+
self.log.info("Checking argument fallback annotations")
getblock = find_method(openrpc, "getblock")
verbosity = find_param(getblock, "verbosity")Expose the generated OpenRPC document through RPC so clients can inspect the interface supported by the running node. The command can optionally include hidden RPC commands and arguments for complete code generation and regtest coverage.
Cover the new RPC with a lightweight functional test that verifies the result is JSON serializable and exposes the expected top-level OpenRPC document fields.
Keep the repeated embedded address documentation in one helper so the OpenRPC metadata and help text stay consistent without duplicating the same field list.
|
Thanks for your review @w0xlt (and @satsfy!) In making amendments here to @w0xlt's comments I re-read the spec again and noticed two more issues I am considering fixing up:
Regarding 1. As far as I can tell this just relaxes the versionning requirements a little (no code changes needed our side), so I will update to the current latest version. On 2., I have a --fixup which adds |
bump it :D there is no code currently that depends on 1.3.2-specific semantics.
By that, if you mean, they both call the primitive something like this? : diffdiff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index ca383a065a..63d610d64a 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -479,6 +479,67 @@ UniValue OpenRPCResultSchema(const RPCResult& result)
}
} // namespace
+static RPCResult OpenRPCDocResult()
+{
+ return RPCResult{
+ RPCResult::Type::OBJ, "", "",
+ {
+ {RPCResult::Type::STR, "openrpc", "OpenRPC specification version."},
+ {RPCResult::Type::OBJ, "info", "Metadata about this JSON-RPC interface.",
+ {
+ {RPCResult::Type::STR, "title", "API title."},
+ {RPCResult::Type::STR, "version", "Bitcoin Core version string."},
+ {RPCResult::Type::STR, "description", "API description."},
+ }},
+ {RPCResult::Type::ARR, "methods", "Documented RPC methods.",
+ {{RPCResult::Type::OBJ, "", "An RPC method description object.",
+ {
+ {RPCResult::Type::STR, "name", "Method name."},
+ {RPCResult::Type::STR, "description", "Method description."},
+ {RPCResult::Type::ARR, "params", "Method parameters.",
+ {{RPCResult::Type::OBJ, "", "A parameter.",
+ {
+ {RPCResult::Type::STR, "name", "Parameter name."},
+ {RPCResult::Type::BOOL, "required", "Whether the parameter is required."},
+ {RPCResult::Type::ANY, "schema", "JSON Schema for the parameter."},
+ {RPCResult::Type::STR, "description", /*optional=*/true, "Parameter description."},
+ {RPCResult::Type::ARR, "x-bitcoin-aliases", /*optional=*/true, "Alternative parameter names.",
+ {{RPCResult::Type::STR, "", "An alias."}}},
+ {RPCResult::Type::BOOL, "x-bitcoin-placeholder", /*optional=*/true, "Whether the parameter is retained only for compatibility."},
+ {RPCResult::Type::BOOL, "x-bitcoin-also-positional", /*optional=*/true, "Whether the parameter can also be passed positionally."},
+ }}}},
+ {RPCResult::Type::OBJ, "result", "Method result.",
+ {
+ {RPCResult::Type::STR, "name", "Result name."},
+ {RPCResult::Type::ANY, "schema", "JSON Schema for the result."},
+ }},
+ {RPCResult::Type::STR, "x-bitcoin-category", "RPC category."},
+ }}}},
+ },
+ {.skip_type_check = true}};
+}
+
+static RPCMethod rpc_discover()
+{
+ return RPCMethod{
+ "rpc.discover",
+ "Returns the OpenRPC document describing this JSON-RPC service.\n"
+ "This is the OpenRPC service discovery method; it describes the public\n"
+ "interface only. Use getopenrpcinfo to additionally include hidden RPC\n"
+ "commands and arguments.\n",
+ {},
+ OpenRPCDocResult(),
+ RPCExamples{
+ HelpExampleCli("rpc.discover", "")
+ + HelpExampleRpc("rpc.discover", "")
+ },
+ [](const RPCMethod& self, const JSONRPCRequest& request) -> UniValue
+{
+ return tableRPC.buildOpenRPCDoc(/*include_hidden=*/false);
+},
+ };
+}
+
static RPCMethod getopenrpcinfo()
{
return RPCMethod{
@@ -487,42 +548,7 @@ static RPCMethod getopenrpcinfo()
{
{"show_hidden", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also include hidden RPC commands and arguments."},
},
- RPCResult{
- RPCResult::Type::OBJ, "", "",
- {
- {RPCResult::Type::STR, "openrpc", "OpenRPC specification version."},
- {RPCResult::Type::OBJ, "info", "Metadata about this JSON-RPC interface.",
- {
- {RPCResult::Type::STR, "title", "API title."},
- {RPCResult::Type::STR, "version", "Bitcoin Core version string."},
- {RPCResult::Type::STR, "description", "API description."},
- }},
- {RPCResult::Type::ARR, "methods", "Documented RPC methods.",
- {{RPCResult::Type::OBJ, "", "An RPC method description object.",
- {
- {RPCResult::Type::STR, "name", "Method name."},
- {RPCResult::Type::STR, "description", "Method description."},
- {RPCResult::Type::ARR, "params", "Method parameters.",
- {{RPCResult::Type::OBJ, "", "A parameter.",
- {
- {RPCResult::Type::STR, "name", "Parameter name."},
- {RPCResult::Type::BOOL, "required", "Whether the parameter is required."},
- {RPCResult::Type::ANY, "schema", "JSON Schema for the parameter."},
- {RPCResult::Type::STR, "description", /*optional=*/true, "Parameter description."},
- {RPCResult::Type::ARR, "x-bitcoin-aliases", /*optional=*/true, "Alternative parameter names.",
- {{RPCResult::Type::STR, "", "An alias."}}},
- {RPCResult::Type::BOOL, "x-bitcoin-placeholder", /*optional=*/true, "Whether the parameter is retained only for compatibility."},
- {RPCResult::Type::BOOL, "x-bitcoin-also-positional", /*optional=*/true, "Whether the parameter can also be passed positionally."},
- }}}},
- {RPCResult::Type::OBJ, "result", "Method result.",
- {
- {RPCResult::Type::STR, "name", "Result name."},
- {RPCResult::Type::ANY, "schema", "JSON Schema for the result."},
- }},
- {RPCResult::Type::STR, "x-bitcoin-category", "RPC category."},
- }}}},
- },
- {.skip_type_check = true}},
+ OpenRPCDocResult(),
RPCExamples{
HelpExampleCli("getopenrpcinfo", "")
+ HelpExampleRpc("getopenrpcinfo", "")
@@ -537,6 +563,7 @@ static RPCMethod getopenrpcinfo()
static const CRPCCommand vRPCCommands[]{
/* Overall control/query calls */
+ {"control", &rpc_discover},
{"control", &getopenrpcinfo},
{"control", &getrpcinfo},
{"control", &help}, |
yes pretty much! I will push something up shortly, as I also think we should make both of those changes. |
In the pull description: I think that should be Fixes ...? Also, could remove all the |
dc99429 to
bcfebec
Compare
|
Sorry for the delay. Pushed updates addressing the review feedback including:
|
OpenRPC service discovery specifies `rpc.discover` as the discovery method name. Expose `rpc.discover` as an alias for `getopenrpcinfo`, so clients that expect the standard OpenRPC discovery method can retrieve the same generated document without changing the existing Bitcoin Core RPC.
bcfebec to
eab59e2
Compare
Fixes #29912
This PR adds a machine-readable OpenRPC 1.4.1 specification of out JSON-RPC interface, auto-generated from existing
RPCHelpManmetadata.There is currently no formal, machine-readable specification of the RPC API. As discussed in #29912, this has knock-on consequences:
This draft builds on prior art by casey and the observations by laanwj, stickies-v, kilianmh, hodlinator, and cdecker in #29912. Casey's work demonstrated that RPCHelpMan already contains all the structured information needed, which makes this feasible without duplicating any API definitions.
This differs from Casey's branches in that it uses the OpenRPC standard rather than an ad-hoc format or raw JSON Schema.
Why OpenRPC
I seletced OpenRPC for a number of reasons:
Approach
RPCHelpManmetadata →getopenrpcinfo/rpc.discover→ OpenRPC JSONTradeoffs
vs an ad-hoc format OpenRPC gives us interoperability with the (admittedly surprisingly limited) tooling, documentation generators, code generators, and validators, at the cost of needing x-bitcoin-* extensions for Bitcoin-specific concepts. As Casey noted after trying both approaches, JSON Schema "is probably not a great fit". OpenRPC's method-level framing on top of JSON Schema addresses the ergonomic issues while keeping the schema benefits. After testing both, I think I agree.
Types: JSON Schema cannot natively express all Bitcoin-specific semantics. Amount result fields are represented as JSON numbers with
x-bitcoin-unit: amount; other Bitcoin-specific distinctions remain in descriptions orx-bitcoin-*extensions. More structured unit metadata and stronger constraints can be added in follow-up work.Some RPCs return different types depending on argument values (e.g. verbosity levels). These are represented as
oneOfin the result schema with free-text condition descriptions. This is accurate but not fully machine-parseable — a code generator cannot automatically determine which result variant corresponds to which argument value without parsing the description. I still we have enough information to satisfy humans an agents alike though.Regenerating the spec
The functional test invokes both RPCs, verifies valid JSON, checks public and hidden RPC handling, and covers representative generated schemas. It does not compare a committed generated artifact.
getopenrpcinfoomits hidden RPCs and arguments by default;getopenrpcinfo(true)includes them. The standard parameterlessrpc.discovermethod returns the public document.The RPC output documents which RPCs are available for any given built binary.
Discussion questions
My personal thoughts are that this is very nice to have.