Skip to content

Commit

Permalink
Merge #11050: Avoid treating null RPC arguments different from missin…
Browse files Browse the repository at this point in the history 
…g arguments

Summary:
745d2e3 Clean up getbalance RPC parameter handling (Russell Yanofsky)
fd5d71e Update developer notes after params.size() cleanup (Russell Yanofsky)
e067673 Avoid treating null RPC arguments different from missing arguments (Russell Yanofsky)
e666efc Get rid of redundant RPC params.size() checks (Russell Yanofsky)

Pull request description:

  This is a followup to #10783.

  - The first commit doesn't change behavior at all, just simplifies code.
  - The second commit just changes RPC methods to treat null arguments the same as missing arguments instead of throwing type errors.
  - The third commit updates developer notes after the cleanup.
  - The forth commit does some additional code cleanup in `getbalance`.

  Followup changes that should happen in future PRs:

  - [ ] Replace uses of `.isTrue()` with calls to `.get_bool()` so numbers, objects, and strings cause type errors instead of being interpreted as false. bitcoin/bitcoin#11050 (comment)
  - [ ] Add braces around if statements. bitcoin/bitcoin#11050 (comment)
  - [ ] Maybe improve UniValue type error exceptions and eliminate RPCTypeCheck and RPCTypeCheckArgument functions. bitcoin/bitcoin#11050 (comment)

Tree-SHA512: e72f696011d20acc0778e996659e41f9426bffce387b29ff63bf59ad1163d5146761e4445b2b9b9e069a80596a57c7f4402b75a15d5d20f69f775ae558cf67e9

Backport of Core PR11050
bitcoin/bitcoin#11050

Depends on D3464

Test Plan:
  make check
  test_runner.py

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3997
  • Loading branch information
@laanwj @jonspock
laanwj authored and jonspock committed Oct 5, 2020
1 parent 5f4085e commit 17ee020
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 46 deletions.
9 changes: 4 additions & 5 deletions src/rpc/misc.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -405,9 +405,8 @@ static UniValue getmemoryinfo(const Config &config,
HelpExampleRpc("getmemoryinfo", ""));
}

std::string mode = (request.params.size() < 1 || request.params[0].isNull())
? "stats"
: request.params[0].get_str();
std::string mode =
request.params[0].isNull() ? "stats" : request.params[0].get_str();
if (mode == "stats") {
UniValue obj(UniValue::VOBJ);
obj.pushKV("locked", RPCLockedMemoryInfo());
Expand Down Expand Up @@ -490,11 +489,11 @@ static UniValue logging(const Config &config, const JSONRPCRequest &request) {
}

uint32_t original_log_categories = GetLogger().GetCategoryMask();
if (request.params.size() > 0 && request.params[0].isArray()) {
if (request.params[0].isArray()) {
EnableOrDisableLogCategories(request.params[0], true);
}

if (request.params.size() > 1 && request.params[1].isArray()) {
if (request.params[1].isArray()) {
EnableOrDisableLogCategories(request.params[1], false);
}

Expand Down
13 changes: 6 additions & 7 deletions src/rpc/net.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ static UniValue getpeerinfo(const Config &config,

static UniValue addnode(const Config &config, const JSONRPCRequest &request) {
std::string strCommand;
if (request.params.size() == 2) {
if (!request.params[1].isNull()) {
strCommand = request.params[1].get_str();
}

Expand Down Expand Up @@ -323,8 +323,7 @@ static UniValue disconnectnode(const Config &config,

bool success;
const UniValue &address_arg = request.params[0];
const UniValue &id_arg =
request.params.size() < 2 ? NullUniValue : request.params[1];
const UniValue &id_arg = request.params[1];

if (!address_arg.isNull() && id_arg.isNull()) {
/* handle disconnect-by-address */
Expand Down Expand Up @@ -391,7 +390,7 @@ static UniValue getaddednodeinfo(const Config &config,

std::vector<AddedNodeInfo> vInfo = g_connman->GetAddedNodeInfo();

if (request.params.size() == 1 && !request.params[0].isNull()) {
if (!request.params[0].isNull()) {
bool found = false;
for (const AddedNodeInfo &info : vInfo) {
if (info.strAddedNode == request.params[0].get_str()) {
Expand Down Expand Up @@ -616,7 +615,7 @@ static UniValue getnetworkinfo(const Config &config,

static UniValue setban(const Config &config, const JSONRPCRequest &request) {
std::string strCommand;
if (request.params.size() >= 2) {
if (!request.params[1].isNull()) {
strCommand = request.params[1].get_str();
}

Expand Down Expand Up @@ -678,12 +677,12 @@ static UniValue setban(const Config &config, const JSONRPCRequest &request) {

// Use standard bantime if not specified.
int64_t banTime = 0;
if (request.params.size() >= 3 && !request.params[2].isNull()) {
if (!request.params[2].isNull()) {
banTime = request.params[2].get_int64();
}

bool absolute = false;
if (request.params.size() == 4 && request.params[3].isTrue()) {
if (request.params[3].isTrue()) {
absolute = true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/rpc/rawtransaction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1422,7 +1422,7 @@ static UniValue sendrawtransaction(const Config &config,
const TxId &txid = tx->GetId();

Amount nMaxRawTxFee = maxTxFee;
if (request.params.size() > 1 && request.params[1].get_bool()) {
if (!request.params[1].isNull() && request.params[1].get_bool()) {
nMaxRawTxFee = Amount::zero();
}

Expand Down
74 changes: 41 additions & 33 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -617,17 +617,15 @@ static UniValue sendtoaddress(const Config &config,

// Wallet comments
mapValue_t mapValue;
if (request.params.size() > 2 && !request.params[2].isNull() &&
!request.params[2].get_str().empty()) {
if (!request.params[2].isNull() && !request.params[2].get_str().empty()) {
mapValue["comment"] = request.params[2].get_str();
}
if (request.params.size() > 3 && !request.params[3].isNull() &&
!request.params[3].get_str().empty()) {
if (!request.params[3].isNull() && !request.params[3].get_str().empty()) {
mapValue["to"] = request.params[3].get_str();
}

bool fSubtractFeeFromAmount = false;
if (request.params.size() > 4) {
if (!request.params[4].isNull()) {
fSubtractFeeFromAmount = request.params[4].get_bool();
}

Expand Down Expand Up @@ -1199,21 +1197,36 @@ static UniValue getbalance(const Config &config,
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);

if (request.params.size() == 0) {
const UniValue &account_value = request.params[0];
const UniValue &minconf = request.params[1];
const UniValue &include_watchonly = request.params[2];

if (account_value.isNull()) {
if (!minconf.isNull()) {
throw JSONRPCError(RPC_INVALID_PARAMETER,
"getbalance minconf option is only currently "
"supported if an account is specified");
}
if (!include_watchonly.isNull()) {
throw JSONRPCError(
RPC_INVALID_PARAMETER,
"getbalance include_watchonly option is only currently "
"supported if an account is specified");
}
return ValueFromAmount(pwallet->GetBalance());
}

const std::string &account_param = request.params[0].get_str();
const std::string &account_param = account_value.get_str();
const std::string *account =
account_param != "*" ? &account_param : nullptr;

int nMinDepth = 1;
if (!request.params[1].isNull()) {
nMinDepth = request.params[1].get_int();
if (!minconf.isNull()) {
nMinDepth = minconf.get_int();
}

isminefilter filter = ISMINE_SPENDABLE;
if (!request.params[2].isNull() && request.params[2].get_bool()) {
if (!include_watchonly.isNull() && include_watchonly.get_bool()) {
filter = filter | ISMINE_WATCH_ONLY;
}

Expand Down Expand Up @@ -1312,13 +1325,13 @@ static UniValue movecmd(const Config &config, const JSONRPCRequest &request) {
if (nAmount <= Amount::zero()) {
throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send");
}
if (request.params.size() > 3) {
if (!request.params[3].isNull()) {
// Unused parameter, used to be nMinDepth, keep type-checking it though.
(void)request.params[3].get_int();
}

std::string strComment;
if (request.params.size() > 4) {
if (!request.params[4].isNull()) {
strComment = request.params[4].get_str();
}

Expand Down Expand Up @@ -1414,18 +1427,16 @@ static UniValue sendfrom(const Config &config, const JSONRPCRequest &request) {
}

int nMinDepth = 1;
if (request.params.size() > 3) {
if (!request.params[3].isNull()) {
nMinDepth = request.params[3].get_int();
}

mapValue_t mapValue;
if (request.params.size() > 4 && !request.params[4].isNull() &&
!request.params[4].get_str().empty()) {
if (!request.params[4].isNull() && !request.params[4].get_str().empty()) {
mapValue["comment"] = request.params[4].get_str();
}

if (request.params.size() > 5 && !request.params[5].isNull() &&
!request.params[5].get_str().empty()) {
if (!request.params[5].isNull() && !request.params[5].get_str().empty()) {
mapValue["to"] = request.params[5].get_str();
}

Expand Down Expand Up @@ -1546,13 +1557,12 @@ static UniValue sendmany(const Config &config, const JSONRPCRequest &request) {
}

mapValue_t mapValue;
if (request.params.size() > 3 && !request.params[3].isNull() &&
!request.params[3].get_str().empty()) {
if (!request.params[3].isNull() && !request.params[3].get_str().empty()) {
mapValue["comment"] = request.params[3].get_str();
}

UniValue subtractFeeFromAmount(UniValue::VARR);
if (request.params.size() > 4) {
if (!request.params[4].isNull()) {
subtractFeeFromAmount = request.params[4].get_array();
}

Expand Down Expand Up @@ -2393,12 +2403,12 @@ static UniValue listaccounts(const Config &config,
LOCK(pwallet->cs_wallet);

int nMinDepth = 1;
if (request.params.size() > 0) {
if (!request.params[0].isNull()) {
nMinDepth = request.params[0].get_int();
}

isminefilter includeWatchonly = ISMINE_SPENDABLE;
if (request.params.size() > 1 && request.params[1].get_bool()) {
if (!request.params[1].isNull() && request.params[1].get_bool()) {
includeWatchonly = includeWatchonly | ISMINE_WATCH_ONLY;
}

Expand Down Expand Up @@ -3213,21 +3223,19 @@ static UniValue lockunspent(const Config &config,
auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);

if (request.params.size() == 1) {
RPCTypeCheck(request.params, {UniValue::VBOOL});
} else {
RPCTypeCheck(request.params, {UniValue::VBOOL, UniValue::VARR});
}
RPCTypeCheckArgument(request.params[0], UniValue::VBOOL);

bool fUnlock = request.params[0].get_bool();

if (request.params.size() == 1) {
if (request.params[1].isNull()) {
if (fUnlock) {
pwallet->UnlockAllCoins();
}
return true;
}

RPCTypeCheckArgument(request.params[1], UniValue::VARR);

const UniValue &output_params = request.params[1];

// Create and validate the COutPoints first.
Expand Down Expand Up @@ -3899,19 +3907,19 @@ static UniValue listunspent(const Config &config,
}

int nMinDepth = 1;
if (request.params.size() > 0 && !request.params[0].isNull()) {
if (!request.params[0].isNull()) {
RPCTypeCheckArgument(request.params[0], UniValue::VNUM);
nMinDepth = request.params[0].get_int();
}

int nMaxDepth = 9999999;
if (request.params.size() > 1 && !request.params[1].isNull()) {
if (!request.params[1].isNull()) {
RPCTypeCheckArgument(request.params[1], UniValue::VNUM);
nMaxDepth = request.params[1].get_int();
}

std::set<CTxDestination> destinations;
if (request.params.size() > 2 && !request.params[2].isNull()) {
if (!request.params[2].isNull()) {
RPCTypeCheckArgument(request.params[2], UniValue::VARR);
UniValue inputs = request.params[2].get_array();
for (size_t idx = 0; idx < inputs.size(); idx++) {
Expand All @@ -3933,7 +3941,7 @@ static UniValue listunspent(const Config &config,
}

bool include_unsafe = true;
if (request.params.size() > 3 && !request.params[3].isNull()) {
if (!request.params[3].isNull()) {
RPCTypeCheckArgument(request.params[3], UniValue::VBOOL);
include_unsafe = request.params[3].get_bool();
}
Expand Down Expand Up @@ -4370,7 +4378,7 @@ UniValue generate(const Config &config, const JSONRPCRequest &request) {

int num_generate = request.params[0].get_int();
uint64_t max_tries = 1000000;
if (request.params.size() > 1 && !request.params[1].isNull()) {
if (!request.params[1].isNull()) {
max_tries = request.params[1].get_int();
}

Expand Down

0 comments on commit 17ee020

Please sign in to comment.