Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/rpc/mining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ static RPCHelpMan getnetworkhashps()

ChainstateManager& chainman = EnsureAnyChainman(request.context);
LOCK(cs_main);
return GetNetworkHashPS(!request.params[0].isNull() ? request.params[0].getInt<int>() : 120, !request.params[1].isNull() ? request.params[1].getInt<int>() : -1, chainman.ActiveChain());
return GetNetworkHashPS(self.Arg<int>(0), self.Arg<int>(1), chainman.ActiveChain());

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note for reviewers:

28230: is NOT partial due to diversity of implementation of prioritizetransaction, because bitcoin#10252 has never been backported.

},
};
}
Expand Down Expand Up @@ -230,12 +230,12 @@ static RPCHelpMan generatetodescriptor()
"\nGenerate 11 blocks to mydesc\n" + HelpExampleCli("generatetodescriptor", "11 \"mydesc\"")},
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
{
const int num_blocks{request.params[0].getInt<int>()};
const uint64_t max_tries{request.params[2].isNull() ? DEFAULT_MAX_TRIES : request.params[2].getInt<int>()};
const auto num_blocks{self.Arg<int>(0)};
const auto max_tries{self.Arg<uint64_t>(2)};

CScript coinbase_script;
std::string error;
if (!getScriptFromDescriptor(request.params[1].get_str(), coinbase_script, error)) {
if (!getScriptFromDescriptor(self.Arg<std::string>(1), coinbase_script, error)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, error);
}

Expand Down
46 changes: 46 additions & 0 deletions src/rpc/util.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -525,13 +525,59 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
throw std::runtime_error(ToString());
}
CHECK_NONFATAL(m_req == nullptr);
m_req = &request;
UniValue ret = m_fun(*this, request);
m_req = nullptr;
if (gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK)) {
CHECK_NONFATAL(std::any_of(m_results.m_results.begin(), m_results.m_results.end(), [&ret](const RPCResult& res) { return res.MatchesType(ret); }));
}
return ret;
}

using CheckFn = void(const RPCArg&);
static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
{
CHECK_NONFATAL(i < params.size());
const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
const RPCArg& param{params.at(i)};
if (check) check(param);

if (!arg.isNull()) return &arg;
if (!std::holds_alternative<RPCArg::Default>(param.m_fallback)) return nullptr;
return &std::get<RPCArg::Default>(param.m_fallback);
}
Comment on lines +539 to +549

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep MaybeArg() from materializing documented defaults.

DetailMaybeArg() feeds RPCArg::Default back to both Arg() and MaybeArg(). For MaybeArg<T>(), that makes an omitted parameter with a documented default indistinguishable from an explicitly provided value, which defeats callers that need to detect omission separately.

🛠️ One way to preserve omission for MaybeArg()
-static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
+static const UniValue* DetailMaybeArg(CheckFn* check, bool use_default, const std::vector<RPCArg>& params, const JSONRPCRequest* req, size_t i)
 {
     CHECK_NONFATAL(i < params.size());
     const UniValue& arg{CHECK_NONFATAL(req)->params[i]};
     const RPCArg& param{params.at(i)};
     if (check) check(param);

     if (!arg.isNull()) return &arg;
-    if (!std::holds_alternative<RPCArg::Default>(param.m_fallback)) return nullptr;
+    if (!use_default || !std::holds_alternative<RPCArg::Default>(param.m_fallback)) return nullptr;
     return &std::get<RPCArg::Default>(param.m_fallback);
 }

-#define TMPL_INST(check_param, ret_type, return_code)       \
+#define TMPL_INST(check_param, use_default, ret_type, return_code) \
     template <>                                             \
     ret_type RPCHelpMan::ArgValue<ret_type>(size_t i) const \
     {                                                       \
         const UniValue* maybe_arg{                          \
-            DetailMaybeArg(check_param, m_args, m_req, i),  \
+            DetailMaybeArg(check_param, use_default, m_args, m_req, i), \
         };                                                  \
         return return_code                                  \
     }                                                       \
     void force_semicolon(ret_type)

-TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
-TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
-TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
+TMPL_INST(nullptr, false, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
+TMPL_INST(nullptr, false, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
+TMPL_INST(nullptr, false, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);

-TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
-TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
-TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););
+TMPL_INST(CheckRequiredOrDefault, true, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););
+TMPL_INST(CheckRequiredOrDefault, true, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
+TMPL_INST(CheckRequiredOrDefault, true, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););

Also applies to: 560-579

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/rpc/util.cpp` around lines 539 - 549, DetailMaybeArg() currently returns
an RPCArg::Default as if it were an actual UniValue param, causing MaybeArg<T>()
to be unable to distinguish an omitted argument from an explicitly provided
default; modify DetailMaybeArg() (and the similar block at 560-579) so that it
returns nullptr when the request omits the parameter even if param.m_fallback
holds RPCArg::Default, and only return a pointer to the documented default for
callers that expect a materialized default (e.g., Arg()); adjust callers of
DetailMaybeArg()/MaybeArg<T>() accordingly so MaybeArg<T>() treats nullptr as
"omitted" while Arg() still receives the fallback value when necessary.


static void CheckRequiredOrDefault(const RPCArg& param)
{
// Must use `Arg<Type>(i)` to get the argument or its default value.
const bool required{
std::holds_alternative<RPCArg::Optional>(param.m_fallback) && RPCArg::Optional::NO == std::get<RPCArg::Optional>(param.m_fallback),
};
CHECK_NONFATAL(required || std::holds_alternative<RPCArg::Default>(param.m_fallback));
}

#define TMPL_INST(check_param, ret_type, return_code) \
template <> \
ret_type RPCHelpMan::ArgValue<ret_type>(size_t i) const \
{ \
const UniValue* maybe_arg{ \
DetailMaybeArg(check_param, m_args, m_req, i), \
}; \
return return_code \
} \
void force_semicolon(ret_type)

// Optional arg (without default). Can also be called on required args, if needed.
TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);

// Required arg or optional arg with default value.
TMPL_INST(CheckRequiredOrDefault, int, CHECK_NONFATAL(maybe_arg)->getInt<int>(););

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return type errors for null required RPC args

ArgValue<int>/ArgValue<const std::string&> now does CHECK_NONFATAL(maybe_arg); when a required parameter is explicitly passed as null, DetailMaybeArg() returns nullptr and this path raises NonFatalCheckError instead of a UniValue::type_error. For RPCs switched to self.Arg<...>() (e.g. generatetodescriptor), malformed input now surfaces as internal RPC_MISC_ERROR rather than a normal type error, which changes API behavior and leaks internal-bug messaging for user input.

Useful? React with 👍 / 👎.

TMPL_INST(CheckRequiredOrDefault, uint64_t, CHECK_NONFATAL(maybe_arg)->getInt<uint64_t>(););
TMPL_INST(CheckRequiredOrDefault, const std::string&, CHECK_NONFATAL(maybe_arg)->get_str(););

bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
{
size_t num_required_args = 0;
Expand Down
58 changes: 58 additions & 0 deletions src/rpc/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef BITCOIN_RPC_UTIL_H
#define BITCOIN_RPC_UTIL_H

#include <consensus/amount.h>
#include <node/transaction.h>
#include <pubkey.h>
#include <rpc/protocol.h>
Expand All @@ -13,14 +14,30 @@
#include <script/sign.h>
#include <script/signingprovider.h>
#include <script/standard.h>
#include <uint256.h>
#include <univalue.h>
#include <util/check.h>
#include <util/strencodings.h>

#include <cstddef>
#include <cstdint>
#include <functional>
#include <initializer_list>
#include <map>
#include <optional>
#include <string>
#include <type_traits>
#include <utility>
#include <variant>
#include <vector>

class JSONRPCRequest;
enum ServiceFlags : uint64_t;
enum class OutputType;
enum class TransactionError;
struct FlatSigningProvider;
struct bilingual_str;

static constexpr bool DEFAULT_RPC_DOC_CHECK{
#ifdef RPC_DOC_CHECK
true
Expand Down Expand Up @@ -365,6 +382,44 @@ class RPCHelpMan
RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);

UniValue HandleRequest(const JSONRPCRequest& request) const;
/**
* Helper to get a request argument.
* This function only works during m_fun(), i.e. it should only be used in
* RPC method implementations. The helper internally checks whether the
* user-passed argument isNull() and parses (from JSON) and returns the
* user-passed argument, or the default value derived from the RPCArg
* documention, or a falsy value if no default was given.
*
* Use Arg<Type>(i) to get the argument or its default value. Otherwise,
* use MaybeArg<Type>(i) to get the optional argument or a falsy value.
*
* The Type passed to this helper must match the corresponding
* RPCArg::Type.
*/
template <typename R>
auto Arg(size_t i) const
{
// Return argument (required or with default value).
if constexpr (std::is_integral_v<R> || std::is_floating_point_v<R>) {
// Return numbers by value.
return ArgValue<R>(i);
} else {
// Return everything else by reference.
return ArgValue<const R&>(i);
}
}
template <typename R>
auto MaybeArg(size_t i) const
{
// Return optional argument (without default).
if constexpr (std::is_integral_v<R> || std::is_floating_point_v<R>) {
// Return numbers by value, wrapped in optional.
return ArgValue<std::optional<R>>(i);
} else {
// Return other types by pointer.
return ArgValue<const R*>(i);
}
}
std::string ToString() const;
/** Return the named args that need to be converted from string to another JSON type */
UniValue GetArgMap() const;
Expand All @@ -381,6 +436,9 @@ class RPCHelpMan
const std::vector<RPCArg> m_args;
const RPCResults m_results;
const RPCExamples m_examples;
mutable const JSONRPCRequest* m_req{nullptr}; // A pointer to the request for the duration of m_fun()
template <typename R>
R ArgValue(size_t i) const;
};

RPCErrorCode RPCErrorFromTransactionError(TransactionError terr);
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/rpc/coins.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,8 @@ RPCHelpMan getbalance()

LOCK(pwallet->cs_wallet);

const UniValue& dummy_value = request.params[0];
if (!dummy_value.isNull() && dummy_value.get_str() != "*") {
const auto dummy_value{self.MaybeArg<std::string>(0)};
if (dummy_value && *dummy_value != "*") {
throw JSONRPCError(RPC_METHOD_DEPRECATED, "dummy first argument must be excluded or set to \"*\".");
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/rpc/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -765,7 +765,7 @@ static RPCHelpMan unloadwallet()
// Release the "main" shared pointer and prevent further notifications.
// Note that any attempt to load the same wallet would fail until the wallet
// is destroyed (see CheckUniqueFileid).
std::optional<bool> load_on_start = request.params[1].isNull() ? std::nullopt : std::optional<bool>(request.params[1].get_bool());
std::optional<bool> load_on_start{self.MaybeArg<bool>(1)};
if (!RemoveWallet(context, wallet, load_on_start, warnings)) {
throw JSONRPCError(RPC_MISC_ERROR, "Requested wallet already unloaded");
}
Expand Down