RPC: access RPC arguments by name #29277

stickies-v · 2024-01-18T20:50:25Z

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:

const 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.

DrahtBot · 2024-01-18T20:50:28Z

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
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.

ismaelsadeeq · 2024-01-18T21:13:53Z

Concept ACK

DrahtBot · 2024-01-18T21:56:11Z

🚧 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.

_{Debug: https://github.com/bitcoin/bitcoin/runs/20633278087}

stickies-v · 2024-01-19T00:09:05Z

Force pushed to make CI happy:

removed doxygen @params since it didn't pick up the overloads (and it's already documented in plain-text anyway)
added a null return type to the testing RPCHelpMan

fjahr

Concept ACK, certainly makes the RPC code more readable.

@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?

src/rpc/util.cpp

maflcko · 2024-02-19T11:02:04Z

@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.

stickies-v · 2024-02-21T12:53:35Z

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.

maflcko

ACK 3918736 🍔

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 391873694137f241dfe1f57ed9058a438e147218 🍔
0mHHsiB/JX9sv6SH8E4hfbfpMPUipMUdvJ7diZjq+LEVBYg0u48Usa9OFR7sl0OdlDEJ3c64WImMfBoc80W4Bg==

src/test/rpc_tests.cpp

src/rpc/util.h

fjahr · 2024-02-21T16:19:58Z

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: fjahr@cbda2cf 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.

maflcko · 2024-02-21T16:42:06Z

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.

tdb3

Nice addition. Increases readability.
Concept ACK.

Inline comment provided.

Built and exercised unit tests and rpc* functional tests. All passed.

src/rpc/blockchain.cpp

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.

stickies-v

Force-pushed to:

rebase to latest master to address CI failure (?)
fix some documentation inaccuracies (1, 2)
address some test nits (1, 2)
consistently use action variable in scantxoutset as per this comment

Thank 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.

src/rpc/blockchain.cpp

src/rpc/util.h

src/test/rpc_tests.cpp

maflcko · 2024-04-10T09:59:42Z

Does it work with OBJ_NAMED_PARAMS?

stickies-v · 2024-04-16T15:08:01Z

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?

maflcko · 2024-04-16T15:11:08Z

src/rpc/mining.cpp

    ChainstateManager& chainman = EnsureAnyChainman(request.context);
    LOCK(cs_main);
-    return GetNetworkHashPS(self.Arg<int>(0), self.Arg<int>(1), chainman.ActiveChain());
+    return GetNetworkHashPS(self.Arg<int>("nblocks"), self.Arg<int>("height"), chainman.ActiveChain());


It could make sense to remove the index-based access and require the name-based access?

Done in #29997

fjahr · 2024-04-28T19:49:03Z

Code review ACK 30a6c99

I still prefer my suggested approach but it's still an improvement and I don't want to block this.

maflcko · 2024-04-29T10:38:06Z

ACK 30a6c99 🥑

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 30a6c999351041d6a1e8712a9659be1296a1b46a 🥑
3lgAk6/J8fpwQT5TCz4m/ixg4qM63zVPfUgyz+Lr5wGYqRByfvE0Ol9M1LF33YNs0KZW2MwO0Ic4dnbvd2Q6BQ==

ryanofsky

Code review ACK 30a6c99. 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/ARR args with the Arg helpers, and then that should help with OBJ_NAMED_PARAMS too?

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.

Summary: Add arg helper unit test. 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. This is a backport of [[bitcoin/bitcoin#29277 | core#29277]] Depends on D17946 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D17947

DrahtBot added the RPC/REST/ZMQ label Jan 18, 2024

maflcko mentioned this pull request Jan 18, 2024

rpc: Be able to access RPC parameters by name #27788

Closed

DrahtBot added the CI failed label Jan 18, 2024

stickies-v force-pushed the 2023-10/named-args-on-arg-helper branch from 2c222ea to d2369cb Compare January 18, 2024 22:41

DrahtBot removed the CI failed label Jan 19, 2024

fjahr reviewed Feb 18, 2024

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

stickies-v force-pushed the 2023-10/named-args-on-arg-helper branch from d2369cb to 3918736 Compare February 21, 2024 12:49

maflcko approved these changes Feb 21, 2024

View reviewed changes

src/test/rpc_tests.cpp Outdated Show resolved Hide resolved

src/test/rpc_tests.cpp Outdated Show resolved Hide resolved

src/rpc/util.h Outdated Show resolved Hide resolved

src/rpc/util.h Show resolved Hide resolved

DrahtBot requested review from fjahr and ismaelsadeeq February 21, 2024 13:44

tdb3 reviewed Feb 29, 2024

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

stickies-v added 3 commits March 1, 2024 13:51

rpc: add arg helper unit test

13525e0

Compare the results of self.Arg with the request.params accessors to ensure they behave the same way.

rpc: add named arg helper

bbb3126

Overload the Arg and MaybeArg helpers to allow accessing arguments by name as well. Also update the docs to document Arg and MaybeArg separately

rpc: access some args by name

30a6c99

Use the new key-based Arg helper in a few locations to show how it is used.

stickies-v force-pushed the 2023-10/named-args-on-arg-helper branch from 3918736 to 30a6c99 Compare March 1, 2024 14:15

stickies-v commented Mar 1, 2024

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

src/rpc/util.h Show resolved Hide resolved

src/rpc/util.h Outdated Show resolved Hide resolved

src/test/rpc_tests.cpp Outdated Show resolved Hide resolved

src/test/rpc_tests.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 16, 2024

View reviewed changes

DrahtBot requested review from maflcko and tdb3 April 28, 2024 19:49

ryanofsky approved these changes Apr 29, 2024

View reviewed changes

ryanofsky merged commit 19865a8 into bitcoin:master Apr 29, 2024

stickies-v deleted the 2023-10/named-args-on-arg-helper branch April 29, 2024 14:50

kwvg mentioned this pull request Dec 15, 2024

backport: merge bitcoin#27452, #29347, #29356, #29353, #29452, #29483, #30545, #31383 (BIP324 backports: part 5) dashpay/dash#6488

Merged

5 tasks

bitcoin locked and limited conversation to collaborators Apr 29, 2025

RPC: access RPC arguments by name #29277

RPC: access RPC arguments by name #29277

Uh oh!

Conversation

stickies-v commented Jan 18, 2024

Uh oh!

DrahtBot commented Jan 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Uh oh!

ismaelsadeeq commented Jan 18, 2024

Uh oh!

DrahtBot commented Jan 18, 2024

Uh oh!

stickies-v commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Feb 19, 2024

Uh oh!

stickies-v commented Feb 21, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr commented Feb 21, 2024

Uh oh!

maflcko commented Feb 21, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 10, 2024

Uh oh!

stickies-v commented Apr 16, 2024

Uh oh!

maflcko Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Apr 29, 2024

Choose a reason for hiding this comment

Uh oh!

fjahr commented Apr 28, 2024

Uh oh!

maflcko commented Apr 29, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

DrahtBot commented Jan 18, 2024 •

edited

Loading

stickies-v commented Jan 19, 2024 •

edited

Loading