Adds string overloads for the RPCHelpMan::Arg and RPCHelpMan::MaybeArg helpers to be able to access RPC arguments by name instead of index number. Especially in RPCs with a large number of parameters, this can be quite helpful.
Example usage:
0const auto action{self.Arg<std::string>("action")};
Most of the LoC is adding test coverage and documentation updates. No behaviour change.
An alternative approach to #27788 with significantly less overhaul.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fjahr, maflcko, ryanofsky |
| Concept ACK | ismaelsadeeq, tdb3 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
Force pushed to make CI happy:
@params since it didn’t pick up the overloads (and it’s already documented in plain-text anyway)RPCHelpMan716@@ -715,6 +717,16 @@ std::vector<std::pair<std::string, bool>> RPCHelpMan::GetArgNames() const
717 return ret;
718 }
719
720+ size_t RPCHelpMan::GetParamIndex(std::string_view key) const
@stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?
I doubt this will have any impact. It could even perform worse, and require more code? Also, maps aren’t constexpr, so if this is moved toward constexpr/consteval, it will have to be re-written again.
Force pushed to address indentation issue (and removed the unintuitive “developer mistake” comment in docstring)
@stickies-v have you considered preparing a map once when an RPC is called rather than doing the search for the index every time an arg is accessed?
I agree with @maflcko’s reasoning, so going to leave this as is.
580@@ -581,4 +581,58 @@ BOOST_AUTO_TEST_CASE(help_example)
581 BOOST_CHECK_NE(HelpExampleRpcNamed("foo", {{"arg", true}}), HelpExampleRpcNamed("foo", {{"arg", "true"}}));
582 }
583
584+static void CheckRpc(const std::vector<RPCArg>& params, const UniValue& args, RPCHelpMan::RPCMethodImpl test_impl)
585+{
586+ auto null_result{RPCResult{RPCResult::Type::NONE, "", "None"}};
587+ const RPCHelpMan man{"dummy", "dummy description", params, null_result, RPCExamples{""}, test_impl};
s/man/rpc/g?
592+}
593+
594+BOOST_AUTO_TEST_CASE(rpc_arg_helper)
595+{
596+ constexpr bool DEFAULT_BOOL = true;
597+ constexpr char DEFAULT_STRING[] = "default";
char[]? Could use auto?
420+ * There are two overloads of this function:
421+ * - Use Arg<Type>(size_t i) to get the argument by index or its default value.
422+ * - Use Arg<Type>(const std::string& key) to get the argument by key or its default value.
423+ *
424+ * @return The value of the RPC argument as type R. If the argument is not provided
425+ * and has no default, a falsy value is returned.
R with Type.
409- * use MaybeArg<Type>(i) to get the optional argument or a falsy value.
410+ * Use this function when the argument is required or when it has a default value. If the
411+ * argument is optional and may not be provided, use MaybeArg instead.
412 *
413- * The Type passed to this helper must match the corresponding
414- * RPCArg::Type.
ACK 391873694137f241dfe1f57ed9058a438e147218 🍔
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 391873694137f241dfe1f57ed9058a438e147218 🍔
30mHHsiB/JX9sv6SH8E4hfbfpMPUipMUdvJ7diZjq+LEVBYg0u48Usa9OFR7sl0OdlDEJ3c64WImMfBoc80W4Bg==
I doubt this will have any impact. It could even perform worse, and require more code?
Same amount of code and I don’t see how this would perform worse: https://github.com/fjahr/bitcoin/commit/cbda2cf2c012030544ffc03620cbfcb91b1984be I also find this easier to reason about.
Also, maps aren’t constexpr, so if this is moved toward constexpr/consteval, it will have to be re-written again.
I guess I don’t have enough insight into future development plans of the RPC to judge that if is. But either way it seems weird to me that the best solution should be a find_if each time any argument is accessed (which comes out to O(n^2)), especially when we are already iterating over that information once before in the RPCHelperMan constructor. So it feels to me like this could be rewritten in the future just as likely.
Same amount of code and I don’t see how this would perform worse: fjahr@cbda2cf I also find this easier to reason about.
You removed the CHECK_NONFATAL to mark the bug as an internal bug.
which comes out to O(n^2)
The O notation only makes sense for large n. Is there more than one RPC with more than n=3? Again, if you measure it, I wouldn’t be surprised if the runtime is slower for a map. Though, you likely won’t be able to find an end-to-end performance difference, regardless of what is used.
In any case, it shouldn’t matter much, since apparently it can easily be reverted if the switch to constexpr is done.
2148@@ -2149,15 +2149,16 @@ static RPCHelpMan scantxoutset()
2149 [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2150 {
2151 UniValue result(UniValue::VOBJ);
2152- if (request.params[0].get_str() == "status") {
2153+ const auto action{self.Arg<std::string>("action")};
2154+ if (action == "status") {
nit: What is the rationale for using the new Arg() overload in some locations but not others? Increasing consistency of usage consistency may increase readability.
For example, action == <string literal> replaces request.params[0].get_str() in:
https://github.com/bitcoin/bitcoin/blob/391873694137f241dfe1f57ed9058a438e147218/src/rpc/blockchain.cpp#L2153
but not later in:
Thanks for catching, I missed that. Updated. This is the only such inconsistency I could find in the PR - did you see any others?
note:
request.params[1] in scantxoutset is not yet updated because Arg doesn’t support arrays yet, which is unrelated to this PR but absolutely warrants a follow-uprequest.params[2] in getbalance is not updated because ParseIncludeWatchonly requires a UniValue, and I don’t want to scope creep this PR too much (same for a couple other args)
Throughout the files modified for the PR, looks like there might be other opportunities to use the new overloading (e.g. other instances of params[0].get_str()). Some examples are referenced below. It would seem reasonable to scope this PR for the overloading and initial usage, and batch more updates in a subsequent PR (helps keep this PR small/digestible). It’s your call as the PR author. Great PR, and I would support either approach.
In scanblocks():
https://github.com/bitcoin/bitcoin/blob/30a6c999351041d6a1e8712a9659be1296a1b46a/src/rpc/blockchain.cpp#L2344
In generateblock():
https://github.com/bitcoin/bitcoin/blob/30a6c999351041d6a1e8712a9659be1296a1b46a/src/rpc/mining.cpp#L326
In addconnection():
https://github.com/bitcoin/bitcoin/blob/30a6c999351041d6a1e8712a9659be1296a1b46a/src/rpc/net.cpp#L392
Nice addition. Increases readability.
Concept ACK.
Inline comment provided.
Built and exercised unit tests and rpc* functional tests. All passed.
Compare the results of self.Arg with the request.params accessors
to ensure they behave the same way.
Overload the Arg and MaybeArg helpers to allow accessing arguments
by name as well.
Also update the docs to document Arg and MaybeArg separately
Use the new key-based Arg helper in a few locations to show how
it is used.
Force-pushed to:
action variable in scantxoutset as per this commentThank you all for the review, apologies for the slow follow-up while I was on holidays.
Same amount of code and I don’t see how this would perform worse: fjahr@cbda2cf I also find this easier to reason about.
Thank for writing out the code, I had assumed it’d be a bit more overhead (even after adding the CHECK_NONFATAL which can be done in just one line). I would still prefer to stick with the current approach. As maflcko suggests, the number of parameters is always going to be very small, so given that we don’t have to allocate a map I’m unsure how this would perform differently but I don’t think it’s worth investigating given that both will be very fast. I do prefer this approach being more self-contained inside GetParamIndex, as opposed to adding another member variable and adding to the already verbose RPCHelpMan constructor, so will keep as is.
OBJ_NAMED_PARAMS?
Does it work with
OBJ_NAMED_PARAMS?
Nope. I think we should just address the general use case of getting OBJ/ARR args with the Arg helpers, and then that should help with OBJ_NAMED_PARAMS too?
121@@ -122,7 +122,7 @@ static RPCHelpMan getnetworkhashps()
122 {
123 ChainstateManager& chainman = EnsureAnyChainman(request.context);
124 LOCK(cs_main);
125- return GetNetworkHashPS(self.Arg<int>(0), self.Arg<int>(1), chainman.ActiveChain());
126+ return GetNetworkHashPS(self.Arg<int>("nblocks"), self.Arg<int>("height"), chainman.ActiveChain());
Code review ACK 30a6c999351041d6a1e8712a9659be1296a1b46a
I still prefer my suggested approach but it’s still an improvement and I don’t want to block this.
ACK 30a6c999351041d6a1e8712a9659be1296a1b46a 🥑
Signature:
0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: ACK 30a6c999351041d6a1e8712a9659be1296a1b46a 🥑
33lgAk6/J8fpwQT5TCz4m/ixg4qM63zVPfUgyz+Lr5wGYqRByfvE0Ol9M1LF33YNs0KZW2MwO0Ic4dnbvd2Q6BQ==
Code review ACK 30a6c999351041d6a1e8712a9659be1296a1b46a. Nice change! Implementation is surprisingly simple and additional unit test coverage is welcome, too.
Does it work with
OBJ_NAMED_PARAMS?Nope. I think we should just address the general use case of getting
OBJ/ARRargs with theArghelpers, and then that should help withOBJ_NAMED_PARAMStoo?
It would be nice to support this for consistency, but benefit is probably more minor since these arguments are already accessed by name, and omitting this keeps the code very simple.