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

maflcko commented at 11:14 am on August 7, 2023: member

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 commented at 11:14 am on August 7, 2023: contributor

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.

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

stickies-v commented at 12:02 pm on August 7, 2023: contributor

Concept ACK

maflcko force-pushed on Aug 7, 2023

in src/rpc/util.h:388 in fa8cb8bfd9 outdated

382@@ -383,6 +383,13 @@ class RPCHelpMan
383     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
384 
385     UniValue HandleRequest(const JSONRPCRequest& request) const;
386+    /**
387+     * Helper to get a request param or the default value.
388+     * Throws if the fallback has no default value, or if the return type can

stickies-v commented at 12:38 pm on August 7, 2023:

nit: would be helpful to specify what it throws, and perhaps use @throws to make it structured?

maflcko commented at 1:29 pm on August 7, 2023:

I am leaning toward removing the “throws”. This only happens on internal bugs. For example if a dev writes self.Param<int>(99999999) (or switches int to bool by accident).

It is the requirement of the dev to pass the correct index and the tests will check it (if they call the RPC at least once).

maflcko commented at 2:03 pm on August 7, 2023:

The only check it does is, that if the default value is used, the return type must be able to represent it. (The other checks were already done before.

However, I think it is an implementation detail of RPCHelpMan when to check for internal bugs. Developers should just avoid writing bugs in the first place. And if they do, they’ll get a helpful error message (in the tests) and they shouldn’t care where it is from.

Thus, I’ve removed this from the docs completely.

stickies-v commented at 6:35 pm on August 7, 2023:

Makes sense, thanks for the explanation. Resolved.

in src/rpc/util.h:392 in fa8cb8bfd9 outdated

387+     * Helper to get a request param or the default value.
388+     * Throws if the fallback has no default value, or if the return type can
389+     * not represent the default value.
390+     */
391+    template <typename R>
392+    R Param(size_t i) const;

stickies-v commented at 12:57 pm on August 7, 2023:

nit: as we’re getting the parameter value, would Arg be a better name? Perhaps it’s too similar to RPCArg though (which unfortunately would have been more appropriately named RPCParam, I think, but that’s not relevant to this PR).

maflcko commented at 1:47 pm on August 7, 2023:

Thx, done

in src/rpc/util.cpp:635 in fa8cb8bfd9 outdated

631@@ -631,9 +632,25 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
632                           PACKAGE_BUGREPORT)};
633         }
634     }
635+    m_req = nullptr;

stickies-v commented at 1:02 pm on August 7, 2023:

I think we also want to set this to nullptr in the many cases where we throw early in this function?

maflcko commented at 1:25 pm on August 7, 2023:

Why? throw can only happen on internal bugs. Not sure if it makes sense to write additional code to cover unreachable code.

Also, how? This can be done in the destructor, but then the point of setting it is moot.

stickies-v commented at 4:37 pm on August 7, 2023:

I like how you changed this in your latest force push, can be marked as resolved

in src/rpc/util.h:409 in fa8cb8bfd9 outdated

405@@ -399,6 +406,7 @@ class RPCHelpMan
406     const std::vector<RPCArg> m_args;
407     const RPCResults m_results;
408     const RPCExamples m_examples;
409+    mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of HandleRequest()

stickies-v commented at 1:13 pm on August 7, 2023:

I’m not sure this is threadsafe with our g_work_queue. Will we not have race conditions here when multiple requests on the same RPCHelpMan are in the queue at the same time?

maflcko commented at 1:22 pm on August 7, 2023:

RPCHelpMan are constructed and allocated fresh for each RPC call.

stickies-v commented at 4:33 pm on August 7, 2023:

Thanks, you’re right. I always assumed they were instantiated along with CRPCTable, but as you say they’re freshly created on every CRPCTable::execute() call because of this line. Can be marked as resolved.

maflcko renamed this:
~~rpc: Add Param() default helper~~
rpc: Add Arg() default helper
on Aug 7, 2023

maflcko force-pushed on Aug 7, 2023

in src/rpc/util.h:388 in fa6b2da57e outdated

382@@ -383,6 +383,12 @@ class RPCHelpMan
383     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
384 
385     UniValue HandleRequest(const JSONRPCRequest& request) const;
386+    /**
387+     * Helper to get a request argument or its default value.
388+     * This function only works during m_fun().

stickies-v commented at 4:45 pm on August 7, 2023:

nit: While correct, I think this could benefit from a useability hint, e.g.:

0     * This function only works during m_fun(), i.e. it should only be used in RPC method implementations.

maflcko commented at 8:47 am on August 8, 2023:

Nice, used your doc string. Thanks

in src/rpc/util.cpp:649 in fa6b2da57e outdated

644+    CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));
645+    return arg.isNull() ? std::get<RPCArg::Default>(fallback) : arg;
646+}
647+
648+template <>
649+int RPCHelpMan::Arg<int>(size_t i) const

stickies-v commented at 6:29 pm on August 7, 2023:

This implementation only supports being called on parameters with a RPCArg::Default fallback. Do you envision this being extended to also support RPCArg::DefaultHint and RPCArg::Optional::OMITTED fallbacks, and if so, how?

I think one way is with a std::optional<int> RPCHelpMan::Arg<std::optional<int>>(size_t i) const specialization. I tinkered around with it a bit and this is what I came up with:

 0diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
 1index daf751111f..fd1d6c6a63 100644
 2--- a/src/rpc/server.cpp
 3+++ b/src/rpc/server.cpp
 4@@ -180,8 +180,8 @@ static RPCHelpMan stop()
 5     // Event loop will exit after current HTTP requests have been handled, so
 6     // this reply will get back to the client.
 7     StartShutdown();
 8-    if (jsonRequest.params[0].isNum()) {
 9-        UninterruptibleSleep(std::chrono::milliseconds{jsonRequest.params[0].getInt<int>()});
10+    if (const auto wait{self.Arg<std::optional<int>>(0)}) {
11+        UninterruptibleSleep(std::chrono::milliseconds{*wait});
12     }
13     return RESULT;
14 },
15diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
16index 4e0bace96f..c1e626eeaa 100644
17--- a/src/rpc/util.cpp
18+++ b/src/rpc/util.cpp
19@@ -636,19 +636,37 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
20     return ret;
21 }
22 
23-static const UniValue& ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
24+static const std::optional<UniValue> ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
25 {
26     CHECK_NONFATAL(i < params.size());
27     const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
28     const RPCArg::Fallback& fallback{params.at(i).m_fallback};
29-    CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));
30-    return arg.isNull() ? std::get<RPCArg::Default>(fallback) : arg;
31+    const bool has_default{std::holds_alternative<RPCArg::Default>(fallback)};
32+
33+    if (arg.isNull()) {
34+        if (has_default) {
35+            return std::get<RPCArg::Default>(fallback);
36+        } else { 
37+            return std::nullopt;
38+        }
39+    }
40+    return arg;
41 }
42 
43 template <>
44 int RPCHelpMan::Arg<int>(size_t i) const
45 {
46-    return ArgOrDefault(m_args, m_req, i).getInt<int>();
47+    return CHECK_NONFATAL(ArgOrDefault(m_args, m_req, i)).value().getInt<int>();
48+}
49+
50+template <>
51+std::optional<int> RPCHelpMan::Arg<std::optional<int>>(size_t i) const
52+{
53+    if (const auto result{ArgOrDefault(m_args, m_req, i)}) {
54+        return result.value().getInt<int>();
55+    } else {
56+        return std::nullopt;
57+    }
58 }
59 
60 bool RPCHelpMan::IsValidNumArgs(size_t num_args) const

I like the limited scope of this PR to implement just int types, but perhaps it would be useful to extend it a bit and implement the helper for non-default parameters too? At the very least we get a sense of what we want the interface to look like?

If we don’t extend it, perhaps the documentation for template <typename R> R Arg(size_t i) const; in rpc/util.h needs to be updated to reflect where it can safely be used?

maflcko commented at 8:48 am on August 8, 2023:

This implementation only supports being called on parameters with a RPCArg::Default fallback.

sorry, I was missing the early return if (!arg.isNull()) return arg;. Fixed now. Good catch!

maflcko commented at 8:52 am on August 8, 2023:

std::optional

Not sure. This requires all args to be copied again, which can be expensive, if the arg is a hex-encoded block. (https://github.com/bitcoin/bitcoin/pull/20017/files) had the same issue, btw.

I wrote some code to implement std::string as well (without a copy).

stickies-v commented at 5:16 pm on August 8, 2023:

Big fan of this rewrite, makes it pretty extensible without too much code duplication. Nice!

Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations. For example, I think these 2 lines: https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/wallet/rpc/spend.cpp#L274-L275

can be rewritten as:

0    if (auto comment{self.Arg<const std::string*>(2)}; comment && !(comment->empty())) {
1        mapValue["comment"] = *comment;
2    }

Not necessarily shorter, but much more idiomatic imo? I’ve implemented it like this - can probably be cleaned up quite a bit more:

  0diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
  1index 0db66ec1b1..1f7bdac609 100644
  2--- a/src/rpc/util.cpp
  3+++ b/src/rpc/util.cpp
  4@@ -636,27 +636,47 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
  5     return ret;
  6 }
  7 
  8-static const UniValue& ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
  9+static const UniValue* ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
 10 {
 11     CHECK_NONFATAL(i < params.size());
 12     const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
 13-    if (!arg.isNull()) return arg;
 14+    if (!arg.isNull()) return &arg;
 15     const RPCArg::Fallback& fallback{params.at(i).m_fallback};
 16-    CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));
 17-    return std::get<RPCArg::Default>(fallback);
 18+    if (std::holds_alternative<RPCArg::Default>(fallback)) {
 19+        return &(std::get<RPCArg::Default>(fallback));
 20+    }
 21+    return nullptr;
 22 }
 23 
 24-#define TMPL_INST(ret_type, get_arg, get_type)            \
 25-    template <>                                           \
 26-    ret_type RPCHelpMan::get_arg(size_t i) const          \
 27-    {                                                     \
 28-        return ArgOrDefault(m_args, m_req, i).get_type(); \
 29-    }                                                     \
 30+#define TMPL_INST(ret_type, get_arg, get_type)        \
 31+    template <>                                       \
 32+    ret_type RPCHelpMan::get_arg(size_t i) const      \
 33+    {                                                 \
 34+        return CHECK_NONFATAL(ArgOrDefault(m_args, m_req, i))->get_type(); \
 35+    }                                                 \
 36+    void force_semicolon()
 37+
 38+#define TMPL_INST_PTR(ret_type, get_arg, get_type)    \
 39+    template <>                                       \
 40+    std::shared_ptr<ret_type> RPCHelpMan::get_arg(size_t i) const \
 41+    {                                                 \
 42+        auto result {ArgOrDefault(m_args, m_req, i)}; \
 43+        if (!result) return nullptr;                  \
 44+        auto&& ref = result->get_type();               \
 45+        if constexpr (std::is_lvalue_reference_v<decltype(ref)>) { \
 46+            return std::shared_ptr<ret_type>(&ref, [](ret_type*) {}); \
 47+        } else { \
 48+            return std::make_shared<ret_type>(std::move(ref)); \
 49+        } \
 50+    }                                                 \
 51     void force_semicolon()
 52 
 53+
 54+
 55 TMPL_INST(int, ArgValue<int>, getInt<int>);
 56 TMPL_INST(uint64_t, ArgValue<uint64_t>, getInt<uint64_t>);
 57 TMPL_INST(const std::string&, ArgRef<std::string>, get_str);
 58+TMPL_INST_PTR(const std::string, ArgPtr<const std::string>, get_str);
 59 
 60 bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
 61 {
 62diff --git a/src/rpc/util.h b/src/rpc/util.h
 63index 0ef019a780..563a619208 100644
 64--- a/src/rpc/util.h
 65+++ b/src/rpc/util.h
 66@@ -18,6 +18,7 @@
 67 #include <util/check.h>
 68 
 69 #include <string>
 70+#include <type_traits>
 71 #include <variant>
 72 #include <vector>
 73 
 74@@ -392,6 +393,8 @@ public:
 75     {
 76         if constexpr (std::is_integral_v<R>) {
 77             return ArgValue<R>(i);
 78+        } else if constexpr (std::is_pointer_v<R>) {
 79+            return ArgPtr<std::remove_pointer_t<R>>(i);
 80         } else {
 81             return ArgRef<R>(i);
 82         }
 83@@ -417,6 +420,8 @@ private:
 84     R ArgValue(size_t i) const;
 85     template <typename R>
 86     const R& ArgRef(size_t i) const;
 87+    template <typename R>
 88+    std::shared_ptr<R> ArgPtr(size_t i) const;
 89 };
 90 
 91 /**
 92diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
 93index b695d4bed3..420807bf1d 100644
 94--- a/src/wallet/rpc/spend.cpp
 95+++ b/src/wallet/rpc/spend.cpp
 96@@ -271,8 +271,9 @@ RPCHelpMan sendtoaddress()
 97 
 98     // Wallet comments
 99     mapValue_t mapValue;
100-    if (!request.params[2].isNull() && !request.params[2].get_str().empty())
101-        mapValue["comment"] = request.params[2].get_str();
102+    if (auto comment{self.Arg<const std::string*>(2)}; comment && !(comment->empty())) {
103+        mapValue["comment"] = *comment;
104+    }
105     if (!request.params[3].isNull() && !request.params[3].get_str().empty())
106         mapValue["to"] = request.params[3].get_str();

I’m not married to this approach, and it doesn’t need to be added in this PR either. But I think being able to use a consistent interface for Arg for all types of parameters would be nice.

maflcko commented at 5:04 am on August 9, 2023:

Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations.

I don’t think your approach works. It is not possible to “move” a const reference into a shared_ptr. It will be a copy again.

Simply using a raw pointer in your approach seems fine, though.

maflcko commented at 7:01 am on August 9, 2023:

Or rather, std::optional<number> for stuff that is returned by value and const String* for stuff that is returned by reference?

stickies-v commented at 10:16 am on August 9, 2023:

It is not possible to “move” a const reference into a shared_ptr. It will be a copy again.

Sorry, I messed up my git diff a bit, posting a slightly older version, auto& ref = result->get_type(); has to be auto&& ref = result->get_type(); (updated in the above diff now). With that, I think std::move(ref) is only called in case of an rvalue, and we use the (from what I understand without copy) std::shared_ptr constructor in case of an lvalue ref. I think that avoids unnecessary copies?

Or rather, std::optional for stuff that is returned by value and const String* for stuff that is returned by reference?

That was my initial idea too, but I thought having a uniform interface (i.e. pointer for everything where it’s not guaranteed to get a value) is quite a bit nicer than having to switch between std::optional and pointer depending on how we’ve implemented UniValue to return the value? Although I suspect most callsites will just be using the boolean and dereference operators, which are the same for both (ignoring optional’s commonly used .value() and .value_or() operators).

Simply using a raw pointer in your approach seems fine, though.

But if get_type() returns by value, how would we use a raw pointer? Who owns the underlying? That’s why I went for shared_ptr, but I’m still easily confused by memory management so apologies if I’m being dim.

maflcko commented at 10:19 am on August 9, 2023:

Sorry, I messed up my git diff a bit, posting a slightly older version, auto& ref = result->get_type(); has to be auto&& ref = result->get_type(); (updated in the above diff now). With that, I think std::move(ref) is only called in case of an rvalue, and we use the (from what I understand without copy) std::shared_ptr constructor in case of an lvalue ref. I think that avoids unnecessary copies?

Pretty sure it doesn’t, when get_type() returns a const std::string&, both auto& and auto&& will just be a const std::string& as well. (It is not possible to remove const by using auto, auto&, or auto&&, or std::move())

maflcko commented at 10:19 am on August 9, 2023:

But if get_type() returns by value, how would we use a raw pointer? Who owns the underlying? That’s why I went for shared_ptr, but I’m still easily confused by memory management so apologies if I’m being dim.

std::optional can be used to own optional memory, see my previous reply to the thread and the current version of the code :)

stickies-v commented at 10:35 am on August 9, 2023:

Pretty sure it doesn’t, when get_type() returns a const std::string&, both auto& and auto&& will just be a const std::string& as well.

Oh yes, I agree. So in that case, ref will be const std::string& lvalue, and we’ll use std::shared_ptr<ret_type>(&ref, [](ret_type*) {}); where I don’t think any copies happen?

std::optional can be used to own optional memory

I know, I was answering your suggestion of using raw pointers instead of shared_ptr. std::optional solves the memory management issues, but I was just trying to avoid it to keep the interface consistent (i.e. not use optional for one thing and pointers for another) while avoiding unnecessary copies, which I think is only possible with pointers.

maflcko commented at 10:48 am on August 9, 2023:

Ok, I see you are only using shared_ptr to wrap a raw pointer without a deleter.

Happy to switch to that, but I think it may be unintuitive, if sometimes a dev is allowed to keep the shared pointer around after the RPC, and sometimes not.

maflcko commented at 10:57 am on August 9, 2023:

Enforcing/Documenting the lifetime behavior seems easier to understand with separate types (at least for me)

stickies-v commented at 11:24 am on August 9, 2023:

That’s a good point. To confirm my understanding: the lifecycles are identical for both the (1) hybrid std::optional/pointer and the (2) uniform pointer approach (i.e. in both cases, reference types can only be used during the RPC call, value types can be used whenever), but for (2) the developer (user) would have to dive into the implementation to figure out what the exact lifecycle is, whereas if we use different types it’s obvious from just looking at the return type.

I’m going to think about it a bit more, but currently I’m leaning towards agreeing that the interface “inconsistency” as per the current implementation then actually becomes a good thing. Thanks.

maflcko commented at 11:42 am on August 9, 2023:

I could also imagine a third alternative (3) std::shared_ptr/raw pointer, so that everything is a pointer, but there are still different types for each lifetime. (Basically only replacing std::optional in the current approach with std::shared_ptr)

But this also may be more confusing than it helping.

stickies-v commented at 10:03 pm on August 10, 2023:

But this also may be more confusing than it helping.

Yeah, I don’t really think that helps. Still no strong view, I like your current implementation, it’s less complex than what I suggested, so barring strong conviction that’s good enough reason for me to keep it as is. Perhaps useful to keep this convo open for future reference, but considered resolved from my end.

stickies-v commented at 6:37 pm on August 7, 2023: contributor

Approach ACK fa6b2da57ef7ff125a493c7835cb15935255ff8c

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

in src/rpc/util.h:457 in fa6b2da57e outdated

404@@ -399,6 +405,7 @@ class RPCHelpMan
405     const std::vector<RPCArg> m_args;
406     const RPCResults m_results;
407     const RPCExamples m_examples;
408+    mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of m_fun()

ryanofsky commented at 8:24 pm on August 7, 2023:

In commit “rpc: Add Arg() default helper” (fa6b2da57ef7ff125a493c7835cb15935255ff8c)

Any reason to not just drop const from HandleRequest instead of making this variable mutable? Keeping the const there seems a little misleading

maflcko commented at 8:47 am on August 8, 2023:

I thought it would be more fun for reviewers if they could ACK the first use of the mutable const keyword in this codebase. Also, the value will be nullptr before and after the call to HandleRequest, so it seems almost const to me, but happy to change.

TheCharlatan commented at 6:35 pm on August 9, 2023:

Also, the value will be nullptr before and after the call to HandleRequest

Does this also hold if m_fun throws an exception?

ajtowns commented at 4:22 am on August 11, 2023:

Isn’t this buggy? We can have multiple RPC threads, making multiple requests to the same function, but that function only has a single RPCHelpMan which will simultaneously have different things assigned to m_fun?

stickies-v commented at 9:40 am on August 11, 2023:

@ajtowns I raised a similar concern, but it looks like this is safe: #28230 (review)

maflcko commented at 1:23 pm on August 14, 2023:

Also, the value will be nullptr before and after the call to HandleRequest

Does this also hold if m_fun throws an exception?

The object does not exist after an exception. It will be destroyed/deallocated for each RPC call, see #28230 (review)

maflcko force-pushed on Aug 8, 2023

in src/rpc/util.cpp:643 in fa5468dcf0 outdated

635@@ -634,6 +636,28 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
636     return ret;
637 }
638 
639+static const UniValue& ArgOrDefault(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
640+{
641+    CHECK_NONFATAL(i < params.size());
642+    const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
643+    if (!arg.isNull()) return arg;
644+    const RPCArg::Fallback& fallback{params.at(i).m_fallback};

stickies-v commented at 5:31 pm on August 8, 2023:

nit: using .at(i) here and [i] just a few lines up, when I think they both have the same preconditions, so staying consistent is probably better

maflcko commented at 6:25 am on August 9, 2023:

One is a std::vector, the other is UniValue, no?

stickies-v commented at 7:36 am on August 9, 2023:

Whoops yes sorry.

stickies-v approved

stickies-v commented at 5:34 pm on August 8, 2023: contributor

ACK fa5468dcf0109d11ddaeb4b3591e6e04b4ea6125, really like this!

TheCharlatan commented at 8:07 pm on August 8, 2023: contributor

Concept ACK

maflcko force-pushed on Aug 9, 2023

in src/rpc/util.h:407 in fa56ad7416 outdated

399@@ -383,6 +400,36 @@ class RPCHelpMan
400     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
401 
402     UniValue HandleRequest(const JSONRPCRequest& request) const;
403+    /**
404+     * Helper to get a request argument or its default value.
405+     * This function only works during m_fun(), i.e. it should only be used in RPC method implementations.
406+     * Use Arg<Type> to get the argument or its default value.
407+     * Use Arg<Type*> to get the argument or a falsy value.

ryanofsky commented at 2:18 pm on August 9, 2023:

In commit “rpc: Add Arg() default helper” (fa56ad74160ba3c07a16a661fc14cded6deed9eb)

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 #24963 which allows multiple types per parameter (if that is a good idea)

stickies-v commented at 9:26 pm on August 10, 2023:

nit: This phrasing seems to suggest that the developer is free to choose which Arg call to use. I would change that to be more explicit, along the lines of (I think this can be improved):

0If the argument has a `RPCArg::Default` specified, you must use `Arg<Type>` to get the argument or its default value.
1Otherwise, you must use Arg<Type*> to get a pointer or std::optional to get the argument, or a falsy value if it was not provided. A pointer is returned if the underlying is a reference type (e.g. std::string), a std::optional is returned if the underlying is a value type (e.g. int).

ajtowns commented at 6:39 am on August 11, 2023:

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.

maflcko commented at 12:23 pm on August 14, 2023:

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?

maflcko commented at 12:25 pm on August 14, 2023:

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?

maflcko commented at 1:43 pm on August 14, 2023:

Left this as-is in the header, because the implementation already documents this :sweat_smile:

However, I’ve added your docs to the new Check* functions.

ryanofsky commented at 2:39 pm on August 14, 2023:

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.

maflcko commented at 3:11 pm on August 14, 2023:

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.

maflcko commented at 3:13 pm on August 14, 2023:

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.

in src/rpc/util.cpp:639 in fa56ad7416 outdated

635@@ -634,6 +636,39 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
636     return ret;
637 }
638 
639+static const UniValue* MaybeArg(bool no_default, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)

stickies-v commented at 9:19 pm on August 10, 2023:

nit: I find affirmative names much easier to understand than negative ones. Event though there’s no double negation used here (but it would in another suggestion I made), would you consider changing this to has_default? I find that much clearer on the template instantiation calls too.

maflcko commented at 1:36 pm on August 14, 2023:

I’ve made bool no_default a function CheckNoDefault, because this is exactly what needs to be checked. Let me know if you still disagree.

in src/rpc/util.cpp:644 in fa56ad7416 outdated

635@@ -634,6 +636,39 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
636     return ret;
637 }
638 
639+static const UniValue* MaybeArg(bool no_default, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
640+{
641+    CHECK_NONFATAL(i < params.size());
642+    const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
643+    const RPCArg::Fallback& fallback{params.at(i).m_fallback};
644+    if (no_default) CHECK_NONFATAL(!std::holds_alternative<RPCArg::Default>(fallback));

stickies-v commented at 9:20 pm on August 10, 2023:

Since we’re calling std::get<RPCArg::Default>(fallback); later on, wouldn’t it make more sense to call if (has_default) CHECK_NONFATAL(std::holds_alternative<RPCArg::Default>(fallback));?

maflcko commented at 1:36 pm on August 14, 2023:

Added more checks. (CheckRequiredOrDefault)

in src/rpc/util.cpp:651 in fa56ad7416 outdated

646+    if (!arg.isNull()) return &arg;
647+    if (no_default) return nullptr;
648+    return &std::get<RPCArg::Default>(fallback);
649+}
650+
651+#define TMPL_INST(no_default, ret_type, get_arg, return_code) \

stickies-v commented at 9:31 pm on August 10, 2023:

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?

maflcko commented at 1:44 pm on August 14, 2023:

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

stickies-v approved

stickies-v commented at 10:04 pm on August 10, 2023: contributor

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 fa56ad74160ba3c07a16a661fc14cded6deed9eb

in src/rpc/util.h:422 in fa56ad7416 outdated

412+        if constexpr (std::is_pointer_v<R>) {
413+            // Return optional argument (without default).
414+            using U = std::remove_pointer_t<R>;
415+            if constexpr (std::is_integral_v<U> || std::is_floating_point_v<U>) {
416+                // Return numbers by value, wrapped in optional.
417+                return ArgValue<std::optional<U>>(i);

ajtowns commented at 4:12 am on August 11, 2023:

Saying Arg<int*>(5) and getting back a std::optional<int> instead of an int* seems weird? Why not just have two functions: ArgOrNot<int>(5) and ArgOrDefault<int>(5) and drop the outer if constexpr ?

stickies-v commented at 10:15 am on August 11, 2023:

I’d be on board with that approach, too. A bit more explicit and reduces implementation complexity, while I think usability is as good, since we already require the user to distinguish between the arg having a default or not.

nit/bikeshed: I’d prefer Arg over ArgOrDefault.

maflcko commented at 2:05 pm on August 14, 2023:

nit/bikeshed: I’d prefer Arg over ArgOrDefault.

Agree. Also used MaybeArg over ArgOrNot.

ajtowns commented at 5:37 am on August 11, 2023: contributor

Concept ACK

Here’s a branch demoing alternatives for the comments below: https://github.com/ajtowns/bitcoin/commits/202308-rpchelpfulreq

maflcko force-pushed on Aug 14, 2023

maflcko commented at 2:08 pm on August 14, 2023: member

Sorry for the repeated force-pushes, but I think I’ve addressed/replied to all feedback for now.

ryanofsky approved

ryanofsky commented at 2:42 pm on August 14, 2023: contributor

Approach ACK. I wouldn’t say the API here is perfectly ideal because it doesn’t check everything at compile time that could theoretically be checked at compile time. But it is a strict improvement that cuts verbosity and will only make other improvements easier in the future.

maflcko force-pushed on Aug 15, 2023

maflcko commented at 6:39 pm on August 15, 2023: member

Approach ACK. I wouldn’t say the API here is perfectly ideal because it doesn’t check everything at compile time that could theoretically be checked at compile time.

I don’t think any API changes will be required, assuming C++20 const(expr/init/eval), to turn run-time checks into compile-time checks, but I haven’t confirmed this.

in src/rpc/util.cpp:639 in fab5d7358d outdated

635@@ -634,6 +636,56 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
636     return ret;
637 }
638 
639+namespace detail {

ajtowns commented at 3:09 am on August 17, 2023:

Is this only a named namespace because MaybeArg duplicates the function name in RPCHelpMan? I think either this should be an anonymous namespace (and use ::MaybeArg to reference this func, or rename it), or ::MaybeArg should be declared static.

maflcko commented at 6:32 am on August 17, 2023:

Style-wise, I don’t like to shadow global-namespace functions by name. I think it is better if every function has a unique name. I’ll change it to static DetailMaybeArg without a namespace.

ajtowns commented at 7:40 am on August 17, 2023:

static in a named namespace would be fine too :shrug:

in src/rpc/util.cpp:640 in fab5d7358d outdated

635@@ -634,6 +636,56 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
636     return ret;
637 }
638 
639+namespace detail {
640+const UniValue* MaybeArg(std::function<void(const RPCArg&)> check, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)

ajtowns commented at 3:58 am on August 17, 2023:

No need for the function wrapper – MaybeArg(void(&check)(const RPCArg&), ..)

maflcko commented at 6:32 am on August 17, 2023:

I don’t like the style of function pointers and I think <void(const RPCArg&)> check is easier to parse than void(&check)(const RPCArg&). But I may change this in the next push, if there is one.

maflcko commented at 6:54 am on August 17, 2023:

Ah I guess I can use using CheckFn = void(const RPCArg&); to make it as easy to parse.

in src/rpc/util.cpp:656 in fab5d7358d outdated

651+} // namespace detail
652+
653+static void CheckNoDefault(const RPCArg& param)
654+{
655+    // Must use `MaybeArg<Type>(i)` to get the argument.
656+    CHECK_NONFATAL(!std::holds_alternative<RPCArg::Default>(param.m_fallback));

ajtowns commented at 4:05 am on August 17, 2023:

Shouldn’t this be !required && !holds_alternative ? Why use MaybeArg(1) instead of Arg(1) for a required arg?

(One reason: maybe we want to be able to change the RPCHelpMan definition for a call at runtime, eg based on -deprecatedrpc – in that case if deprecatedrpc turns a required arg into an optional, being able to use MaybeArg might make the code simpler. But in that case MaybeArg should just always work, and not have a check, I think?)

maflcko commented at 6:37 am on August 17, 2023:

Ok, I’ll remove the check

maflcko force-pushed on Aug 17, 2023

maflcko commented at 7:18 am on August 17, 2023: member

Force pushed to allow MaybeArg in all contexts.

in src/rpc/util.cpp:652 in fa0cbfc445 outdated

645+    if (check) check(param);
646+
647+    if (!arg.isNull()) return &arg;
648+    if (!std::holds_alternative<RPCArg::Default>(param.m_fallback)) return nullptr;
649+    return &std::get<RPCArg::Default>(param.m_fallback);
650+}

ajtowns commented at 10:42 am on August 17, 2023:

With MaybeArg always working, could make this:

 0static const UniValue* DetailMaybeArg(const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i, bool null_ok)
 1 {
 2...
 3    if (!null_ok) CheckRequiredOrDefault(param);
 4...
 5}
 6
 7#define TMPL_INST_MAYBE(ret_type, get_arg, return_code, return_code_null) \
 8    template <>                                                \
 9    ret_type RPCHelpMan::get_arg(size_t i) const               \
10    {                                                          \
11        const UniValue* maybe_arg{                             \
12            DetailMaybeArg(m_args, m_req, i, true),            \
13        };                                                     \
14        return maybe_arg ? return_code : return_code_null;     \
15    }                                                          \
16    void force_semicolon()
17
18#define TMPL_INST_REQ(ret_type, get_arg, return_code)          \
19    template <>                                                \
20    ret_type RPCHelpMan::get_arg(size_t i) const               \
21    {                                                          \
22        const UniValue* maybe_arg{                             \
23            DetailMaybeArg(m_args, m_req, i, false),           \
24        };                                                     \
25        CHECK_NONFATAL(maybe_arg);                             \
26        return return_code;                                    \
27    }                                                          \
28    void force_semicolon()
29 
30TMPL_INST_MAYBE(std::optional<double>, ArgValue<std::optional<double>>, std::optional{maybe_arg->get_real()}, std::nullopt);
31TMPL_INST_REQ(int, ArgValue<int>, maybe_arg->getInt<int>());

maflcko commented at 11:16 am on August 17, 2023:

I don’t like macros, so I’ll leave as-is for now, because my version has less lines of code inside a macro

ajtowns commented at 0:11 am on August 18, 2023:

Okay, you nerdsniped me successfully. https://github.com/ajtowns/bitcoin/blob/9f332c93bcf7c2a1a4796dda4d2304014b10037e/src/rpc/util.cpp#L684-L705

maflcko commented at 8:44 am on August 18, 2023:

heh, but I don’t like templates either. My preference would be to use if constexpr (is_pointer||is_optional) { foo } else { bar } or C++20 concepts. See also #28230 (review)

However, I’ve taken your idea to remove ArgRef. Thanks!

ajtowns commented at 1:23 am on August 19, 2023:

Yeah, pretty rare that a nerdsnipe results in anything productive. But you’re not going to get me with C++20 features until #23363 is done…

in src/rpc/util.h:406 in fa0cbfc445 outdated

399@@ -383,6 +400,44 @@ class RPCHelpMan
400     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
401 
402     UniValue HandleRequest(const JSONRPCRequest& request) const;
403+    /**
404+     * Helper to get a request argument.
405+     * This function only works during m_fun(), i.e. it should only be used in
406+     * RPC method implementations. The helper internally checkes whether the

ajtowns commented at 10:43 am on August 17, 2023:

“checks” not “checkes”

maflcko commented at 11:28 am on August 17, 2023:

thx, fixed

in src/rpc/util.h:411 in fa0cbfc445 outdated

406+     * RPC method implementations. The helper internally checkes whether the
407+     * user-passed argument isNull() and parses (from JSON) and returns the
408+     * user-passed argument, or the default value derived from the RPCArg
409+     * documention, or a falsy value if no default was given.
410+     *
411+     * Use Arg<Type>(i) to get the argument or its default value.

ajtowns commented at 10:43 am on August 17, 2023:

Add “should not be used for optional arguments that do not have a default error” or similar?

maflcko commented at 11:29 am on August 17, 2023:

thx, I changed the doc a bit.

ajtowns commented at 10:46 am on August 17, 2023: contributor

utACK fa0cbfc445fbbad4009833ad9f13fb56886111a5

DrahtBot requested review from stickies-v on Aug 17, 2023

maflcko force-pushed on Aug 17, 2023

DrahtBot added the label Needs rebase on Aug 17, 2023

maflcko force-pushed on Aug 17, 2023

DrahtBot removed the label Needs rebase on Aug 17, 2023

DrahtBot added the label CI failed on Aug 18, 2023

maflcko force-pushed on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

maflcko commented at 2:52 pm on August 18, 2023: member

Addressed all (style) feedback so far, I think :sweat_smile:

ajtowns commented at 2:24 am on August 19, 2023: contributor

utACK faac216d48b81a07c85c6185af237d7bfd57e23c

stickies-v approved

stickies-v commented at 4:27 pm on August 21, 2023: contributor

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 https://github.com/stickies-v/bitcoin/commit/c19d1868bddcb2068dc96b75d70dbb3b6a942bfc 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 renamed this:
~~rpc: Add Arg() default helper~~
rpc: Add MaybeArg() and Arg() default helper
on Aug 22, 2023

maflcko commented at 7:09 am on August 22, 2023: member

Thanks, edited subject line and description.

in src/rpc/util.cpp:613 in faac216d48 outdated

608@@ -609,7 +609,9 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
609     if (!arg_mismatch.empty()) {
610         throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Wrong type passed:\n%s", arg_mismatch.write(4)));
611     }
612+    m_req = &request;

TheCharlatan commented at 12:48 pm on August 22, 2023:

Nit: Maybe add CHECK_NONFATAL(m_req == nullptr) to document that there should be no side effects here?

maflcko commented at 1:33 pm on August 22, 2023:

Will do in a follow-up

TheCharlatan approved

TheCharlatan commented at 1:05 pm on August 22, 2023: contributor

ACK faac216d48b81a07c85c6185af237d7bfd57e23c

achow101 commented at 10:20 pm on August 23, 2023: member

ACK faac216d48b81a07c85c6185af237d7bfd57e23c

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

in src/rpc/util.cpp:671 in faac216d48 outdated

666+        const UniValue* maybe_arg{                          \
667+            DetailMaybeArg(check_param, m_args, m_req, i),  \
668+        };                                                  \
669+        return return_code                                  \
670+    }                                                       \
671+    void force_semicolon()

achow101 commented at 10:23 pm on August 23, 2023:

 0../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls]
 1  671 |     void force_semicolon()
 2      |          ^~~~~~~~~~~~~~~
 3../../../src/rpc/util.cpp:675:1: note: in expansion of macro ‘TMPL_INST’
 4  675 | TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
 5      | ^~~~~~~~~
 6../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’
 7  671 |     void force_semicolon()
 8      |          ^~~~~~~~~~~~~~~
 9../../../src/rpc/util.cpp:674:1: note: in expansion of macro ‘TMPL_INST’
10  674 | TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
11      | ^~~~~~~~~
12../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls]
13  671 |     void force_semicolon()
14      |          ^~~~~~~~~~~~~~~
15../../../src/rpc/util.cpp:676:1: note: in expansion of macro ‘TMPL_INST’
16  676 | TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
17      | ^~~~~~~~~
18../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’
19  671 |     void force_semicolon()
20      |          ^~~~~~~~~~~~~~~
21../../../src/rpc/util.cpp:675:1: note: in expansion of macro ‘TMPL_INST’
22  675 | TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
23      | ^~~~~~~~~
24../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls]
25  671 |     void force_semicolon()
26      |          ^~~~~~~~~~~~~~~
27../../../src/rpc/util.cpp:679:1: note: in expansion of macro ‘TMPL_INST’
28  679 | TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
29      | ^~~~~~~~~
30../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’
31  671 |     void force_semicolon()
32      |          ^~~~~~~~~~~~~~~
33../../../src/rpc/util.cpp:676:1: note: in expansion of macro ‘TMPL_INST’
34  676 | TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
35      | ^~~~~~~~~
36../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls]
37  671 |     void force_semicolon()
38      |          ^~~~~~~~~~~~~~~
39../../../src/rpc/util.cpp:680:1: note: in expansion of macro ‘TMPL_INST’
40  680 | TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
41      | ^~~~~~~~~
42../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’
43  671 |     void force_semicolon()
44      |          ^~~~~~~~~~~~~~~
45../../../src/rpc/util.cpp:679:1: note: in expansion of macro ‘TMPL_INST’
46  679 | TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
47      | ^~~~~~~~~
48../../../src/rpc/util.cpp:671:10: error: redundant redeclaration of ‘void force_semicolon()’ in same scope [-Werror=redundant-decls]
49  671 |     void force_semicolon()
50      |          ^~~~~~~~~~~~~~~
51../../../src/rpc/util.cpp:681:1: note: in expansion of macro ‘TMPL_INST’
52  681 | TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););
53      | ^~~~~~~~~
54../../../src/rpc/util.cpp:671:10: note: previous declaration of ‘void force_semicolon()’
55  671 |     void force_semicolon()
56      |          ^~~~~~~~~~~~~~~
57../../../src/rpc/util.cpp:680:1: note: in expansion of macro ‘TMPL_INST’
58  680 | TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
59      | ^~~~~~~~~
60cc1plus: all warnings being treated as errors

achow101 commented at 10:31 pm on August 23, 2023:

Seems to work fine without this line.

ajtowns commented at 1:54 am on August 24, 2023:

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.

maflcko commented at 8:19 am on August 24, 2023:

Any reason why CI didn’t fail? I guess this is https://github.com/bitcoin/bitcoin/pull/25972

maflcko force-pushed on Aug 24, 2023

maflcko commented at 8:45 am on August 24, 2023: member

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

stickies-v approved

stickies-v commented at 9:12 am on August 24, 2023: contributor

re-ACK c00000df1605788acadceb90c22ae9f00db8a9dc

Verified that this:

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

DrahtBot requested review from ajtowns on Aug 24, 2023

DrahtBot requested review from achow101 on Aug 24, 2023

DrahtBot requested review from TheCharlatan on Aug 24, 2023

TheCharlatan approved

TheCharlatan commented at 9:13 am on August 24, 2023: contributor

re-ACK c00000df1605788acadceb90c22ae9f00db8a9dc

ajtowns commented at 10:33 am on August 24, 2023: contributor

reACK c00000df1605788acadceb90c22ae9f00db8a9dc

DrahtBot removed review request from ajtowns on Aug 24, 2023

fanquake merged this on Aug 24, 2023

fanquake closed this on Aug 24, 2023

maflcko deleted the branch on Aug 24, 2023

Frank-GER referenced this in commit 4631bd4442 on Sep 8, 2023

achow101 referenced this in commit 9b68c9b85e on Nov 2, 2023

bitcoin locked this on Aug 23, 2024

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

Reviews

Conflicts