rpc: Add MaybeArg() and Arg() default helper #28230

maflcko · 2023-08-07T11:14:00Z

Currently the RPC method implementations have many issues:

Default RPC argument values (and their optionality state) are duplicated in the documentation and the C++ code, with no checks to prevent them from going out of sync.
Getting an optional RPC argument is verbose, using a ternary operator, or worse, a multi-line if.

Fix all issues by adding default helper that can be called via self.Arg<int>(0). The helper needs a few lines of code in the src/rpc/util.h header file. Everything else will be implemented in the cpp file once and if an RPC method needs it.

There is also an self.MaybeArg<int>(0) helper that works on any arg to return the argument, the default, or a falsy value.

DrahtBot · 2023-08-07T11:14:03Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, TheCharlatan, ajtowns
Approach ACK	ryanofsky
Stale ACK	achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28172 (refactor: use string_view for passing string literals to Parse{Hash,Hex} by jonatack)
#27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#24963 (RPC/Wallet: Convert walletprocesspsbt to use options parameter by luke-jr)

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.

stickies-v · 2023-08-07T12:02:04Z

Concept ACK

src/rpc/util.h

src/rpc/util.cpp

src/rpc/util.h

stickies-v

Approach ACK fa6b2da57ef7ff125a493c7835cb15935255ff8c

This makes for a much more elegant interface throughout the RPC methods, with minimal overhead.

src/rpc/util.h

src/rpc/util.cpp

src/rpc/util.h

src/rpc/util.cpp

src/rpc/util.h

stickies-v

ACK fa5468dcf0109d11ddaeb4b3591e6e04b4ea6125, really like this!

src/rpc/util.cpp

sedited · 2023-08-08T20:07:12Z

Concept ACK

ryanofsky · 2023-08-09T14:18:41Z

src/rpc/util.h

+     * Helper to get a request argument or its default value.
+     * This function only works during m_fun(), i.e. it should only be used in RPC method implementations.
+     * Use Arg<Type> to get the argument or its default value.
+     * Use Arg<Type*> to get the argument or a falsy value.


In commit "rpc: Add Arg() default helper" (fa56ad7)

Haven't really looked at implementation yet, but can documentation be updated to say what happens with type mismatches? E.g. if arg is an int and you request a bool? Is the value just ignored and is a default returned instead? Is there an exception?

Also might be nice to add a short examples here showing what equivalent low level code is so developers can know how to use this helper and interpret it. Like Arg<int>(3) is equivalent to request.params[3].isNumber() ? request.params[3].getInt<int>() : default_value (or whatever the equivalent is)

Overall this seems like a nice improvement that should cut down on boilerplate. I am wondering if it might be possible later to extend this:

to support arrays/objects

to enforces only expected types are passed at compile time

to enforce default not requested when there is no default at compile time

to somehow be compatible with luke's RPC/Wallet: Convert walletprocesspsbt to use options parameter #24963 which allows multiple types per parameter (if that is a good idea)

to enforces only expected types are passed at compile time

Do we want to revisit #22978 first before doing compile time type checking for RPC?

I think compile time RPC type checks would require templatising RPCArg by what is currently m_type (and m_inner perhaps), changing RPCHelpMan to be templatised based on a std::tuple<...> of RPCArg<X>s, and likewise for RPCMethodImpl which then gives the actual RPC implementation compile time access to its expected argument types.

I don't think you could reasonably access params by name and get compile time type checking; for the named-only params via options, you could turn options into an ordered tuple and access named-only params by their position from when you defined the options object.

For multitype parameters, I think it'd be best to just have a Type::ANY placeholder.

Haven't really looked at implementation yet, but can documentation be updated to say what happens with type mismatches?

This sound like a bug in the source code, so I don't think it makes sense to document unreachable or unexpected code paths. Generally, in the RPC server non-fatal bugs are expected to throw an internal bug message.

I haven't looked at the code, but types of arguments passed in by the user should be checked before m_fun is run. m_fun, and thus, Arg() isn't run when the types passed in by the user mismatch the ones in the documentation. If Arg() specifies the wrong type, the getter will throw. Currently all getters are univalue getters, so they'll throw the univalue internal exception. (This is the current behavior and preserved)

I think this is fine as-is, but I am happy to change it to something else, for example:

Re-implement the type check and throw a new, special exception. (This will be a change in "behavior", or rather "bug behavior")

Implement a compile-time json and use the compiler to avoid the case altogether, or alternatively, use clang-tidy to do it?

Also might be nice to add a short examples here showing what equivalent low level code is so developers can know how to use this helper and interpret it.

The diff in this commit should explain it. I've also added docs about the internals (isNull check and fallback to RPCArg::m_fallback)

to support arrays/objects

Yeah, it should be possible to return an object whose operator[] is defined. Would it be fine to do this in a follow-up?

to enforces only expected types are passed at compile time

Maybe as follow-up, unless the patch is trivial?

to enforce default not requested when there is no default at compile time

This is already done, no?

to somehow be compatible with luke's

Not sure about adding dead code, but once and if it is needed, it should be trivial to implement by mapping the types to a Arg<std::variant<int, bool>>(1) in C++ code. However, I am not sure if this is worth it, when it is only used once. Might as well use the "legacy" method for now?

I think compile time RPC type checks would require templatising RPCArg by what is currently m_type (and m_inner perhaps), changing RPCHelpMan to be templatised based on a std::tuple<...> of RPCArg<X>s, and likewise for RPCMethodImpl which then gives the actual RPC implementation compile time access to its expected argument types.

I haven't looked at this, but some RPCHelpMan have run-time inputs, so I am not sure if they can be trivially converted to compile-time inputs. Maybe this can be done in a follow-up? The diff should be a trivial change from Arg<int>(0) to Arg<int, 0>(), right? Or with future C++ versions no change in m_fun implementations is needed at all?

This sound like a bug in the source code, so I don't think it makes sense to document unreachable or unexpected code paths.

If it's a bug to to call the the Arg function with an unexpected type, it's important for the documentation to at say what types it is allowed to be called with, because I don't think there's another way of knowing this without a having a pretty deep understanding of the RPCHelpMan framework and digging into template and macro code.

If you just want the documentation to say "It is a bug to pass X type" instead of "Passing X type will throw an exception" that is fine of course.

it's important for the documentation to at say what types it is allowed to be called with

I guess long term it could make sense to make RPCArg::Type an enum of C++ types and then have the documentation say that the type passed to Arg<Type>(i) must match exactly the corresponding RPCArg::Type.

For now, my recommendation would be to call Arg<std::string>(i) for STR* args, Arg<bool>(i) for BOOL args, and Arg<double/(u)int${n}_t>(i) for NUM args.

Would it be fine to add "The Type passed to this helper must match the corresponding RPCArg::Type" to the doxygen of Arg?

Edit: Done.

src/rpc/util.cpp

src/rpc/util.h

stickies-v · 2023-08-10T21:31:31Z

src/rpc/util.cpp

+    return &std::get<RPCArg::Default>(fallback);
+}
+
+#define TMPL_INST(no_default, ret_type, get_arg, return_code) \


We could remove no_default from the function signature here, and deduce it from the ret_type being a std::optional or pointer type? I think both approaches have merit, so no very strong preference either way atm, but since we're already doing those deductions in Arg I think it would make sense here, too?

Sure, mind providing a patch that compiles, which I can take?

src/rpc/util.cpp

stickies-v

Thanks for extending this to cover all kinds of argument types, I think this will make it much more straightforward for users to add in their own template instantiations where needed.

I still don't really have a clear preference for std::optional vs pointer, and I think it's not that important, so ACK fa56ad7

maflcko · 2023-08-18T14:52:56Z

Addressed all (style) feedback so far, I think 😅

ajtowns · 2023-08-19T02:24:21Z

utACK faac216d48b81a07c85c6185af237d7bfd57e23c

stickies-v

ACK faac216d48b81a07c85c6185af237d7bfd57e23c

nit: pull description & title have gotten a bit outdated as the PR changed quite a bit since its initial version. I think a brief mention of MaybeArg and the optionality of arguments could be helpful to include here?

As a nice bonus, it seems adding the RPCHelpMan::m_req pointer also makes it quite trivial to get an argument by name and builds nicely on this pull. I quickly pulled something together in stickies-v@c19d186 just as a PoC that passes functional tests, and (the delta to this PR) seems significantly less work than #27788 (but I've not properly tested/dug into this, just something I thought of).

maflcko · 2023-08-22T07:09:25Z

Thanks, edited subject line and description.

sedited

ACK faac216d48b81a07c85c6185af237d7bfd57e23c

src/rpc/util.cpp

achow101 · 2023-08-23T22:20:28Z

ACK faac216d48b81a07c85c6185af237d7bfd57e23c

achow101 · 2023-08-23T22:23:19Z

src/rpc/util.cpp

../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls] 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:675:1: note: in expansion of macro ‘TMPL_INST’ 675 | TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’ 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:674:1: note: in expansion of macro ‘TMPL_INST’ 674 | TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls] 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:676:1: note: in expansion of macro ‘TMPL_INST’ 676 | TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’ 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:675:1: note: in expansion of macro ‘TMPL_INST’ 675 | TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls] 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:679:1: note: in expansion of macro ‘TMPL_INST’ 679 | TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>();); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’ 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:676:1: note: in expansion of macro ‘TMPL_INST’ 676 | TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls] 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:680:1: note: in expansion of macro ‘TMPL_INST’ 680 | TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>();); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’ 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:679:1: note: in expansion of macro ‘TMPL_INST’ 679 | TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>();); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls] 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:681:1: note: in expansion of macro ‘TMPL_INST’ 681 | TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str();); | ^~~~~~~~~ ../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’ 671 | void force_semicolon() | ^~~~~~~~~~~~~~~ ../../../src/rpc/util.cpp:680:1: note: in expansion of macro ‘TMPL_INST’ 680 | TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>();); | ^~~~~~~~~ cc1plus: all warnings being treated as errors

Seems to work fine without this line.

Could drop it and just not have the semi-colons after the macro invocations too.

Could also change it to void force_semicolon(ret_type dummy) to avoid the warning.

clang accepts but ignores -Wredundant-decls, clang-tidy defaults to ignoring the warning in macros (yay!), but seems like the current code will be annoying for compiling with gcc.

Any reason why CI didn't fail? I guess this is #25972

maflcko · 2023-08-24T08:45:41Z

Force pushed to fix the two nits, should be trivial to re-ACK

stickies-v

re-ACK c00000d

Verified that this:

updates commit msg to PR title
implements #28230 (comment)
fixes #28230 (comment)

sedited

re-ACK c00000d

ajtowns · 2023-08-24T10:33:27Z

reACK c00000d

…ring literals to Parse{Hash,Hex} bb91131 doc: remove out-of-date external link in src/util/strencodings.h (Jon Atack) 7d494a4 refactor: use string_view to pass string literals to Parse{Hash,Hex} (Jon Atack) Pull request description: as `string_view` is optimized to be trivially copiable, whereas the current code creates a `std::string` copy at each call. These utility methods are called by quite a few RPCs and tests, as well as by each other. ``` $ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l 61 ``` Also remove an out-of-date external link. ACKs for top commit: jonatack: Rebased per `git range-diff c9273f6 b94581a bb91131` for an include header from the merge of bitcoin/bitcoin#28230. Should be trivial to re-ACK. maflcko: lgtm ACK bb91131 ns-xvrn: ACK bitcoin/bitcoin@bb91131 achow101: ACK bb91131 brunoerg: crACK bb91131 Tree-SHA512: 9734fe022c9e43fd93c23a917770d332dbbd3132c80a234059714c32faa6469391e59349954749fc86c4ef0b18d5fd99bf8f4b7b82d9f799943799c1253272ae

DrahtBot added the RPC/REST/ZMQ label Aug 7, 2023

This was referenced Aug 7, 2023

rpc: Add RPCContext #20017

Closed

rpc: Add importmempool RPC #27460

Merged

maflcko force-pushed the 2308-rpc-default- branch 2 times, most recently from fa13ff4 to fa8cb8b Compare August 7, 2023 12:22

stickies-v reviewed Aug 7, 2023

View reviewed changes

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

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

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

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

maflcko changed the title ~~rpc: Add Param() default helper~~ rpc: Add Arg() default helper Aug 7, 2023

maflcko force-pushed the 2308-rpc-default- branch 3 times, most recently from fac0196 to fa6b2da Compare August 7, 2023 14:02

stickies-v reviewed Aug 7, 2023

View reviewed changes

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

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

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

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

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

ryanofsky reviewed Aug 7, 2023

View reviewed changes

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

maflcko force-pushed the 2308-rpc-default- branch from fa6b2da to fa5468d Compare August 8, 2023 08:47

stickies-v approved these changes Aug 8, 2023

View reviewed changes

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

maflcko force-pushed the 2308-rpc-default- branch 3 times, most recently from faa8309 to fa56ad7 Compare August 9, 2023 08:09

ryanofsky reviewed Aug 9, 2023

View reviewed changes

stickies-v reviewed Aug 10, 2023

View reviewed changes

stickies-v approved these changes Aug 10, 2023

View reviewed changes

DrahtBot mentioned this pull request Aug 11, 2023

Silent Payments: Implement BIP352 #28122

Draft

DrahtBot added CI failed and removed Needs rebase labels Aug 17, 2023

maflcko force-pushed the 2308-rpc-default- branch from fa448d5 to faac216 Compare August 18, 2023 08:44

DrahtBot removed the CI failed label Aug 18, 2023

stickies-v approved these changes Aug 21, 2023

View reviewed changes

maflcko changed the title ~~rpc: Add Arg() default helper~~ rpc: Add MaybeArg() and Arg() default helper Aug 22, 2023

sedited approved these changes Aug 22, 2023

View reviewed changes

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

achow101 reviewed Aug 23, 2023

View reviewed changes

rpc: Add MaybeArg() and Arg() default helper

c00000d

maflcko force-pushed the 2308-rpc-default- branch from faac216 to c00000d Compare August 24, 2023 08:44

stickies-v approved these changes Aug 24, 2023

View reviewed changes

DrahtBot requested review from achow101, ajtowns and sedited August 24, 2023 09:12

sedited approved these changes Aug 24, 2023

View reviewed changes

DrahtBot removed the request for review from ajtowns August 24, 2023 10:33

fanquake merged commit 083316c into bitcoin:master Aug 24, 2023

maflcko deleted the 2308-rpc-default- branch August 24, 2023 12:22

stickies-v mentioned this pull request Sep 1, 2023

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

Closed

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023

Merge bitcoin#28230: rpc: Add MaybeArg() and Arg() default helper

4631bd4

maflcko mentioned this pull request Sep 10, 2023

build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set #25972

Merged

ajtowns mentioned this pull request Sep 28, 2023

rpc: add hidden getrawaddrman RPC to list addrman table entries #28523

Merged

bitcoin locked and limited conversation to collaborators Aug 23, 2024

rpc: Add MaybeArg() and Arg() default helper #28230

rpc: Add MaybeArg() and Arg() default helper #28230

Uh oh!

Conversation

maflcko commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

stickies-v commented Aug 7, 2023

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sedited commented Aug 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

maflcko commented Aug 18, 2023

Uh oh!

ajtowns commented Aug 19, 2023

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 22, 2023

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Aug 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 7, 2023 •

edited

Loading

DrahtBot commented Aug 7, 2023 •

edited

Loading

maflcko Aug 14, 2023 •

edited

Loading

stickies-v left a comment •

edited

Loading