From 29f2cbdeb7afdde87d108adf80cffad17d112632 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 12 Apr 2020 21:01:29 +0200 Subject: [PATCH 1/6] cli: extract connection exception handler, -rpcwait logic to ConnectAndCallRPC() to be callable for individual connections. This is needed for RPCs that need to be called and handled sequentially, rather than alone or in a batch. For example, when fetching the balances for each loaded wallet, -getinfo will call RPC listwallets, and then, depending on the result, RPC getbalances. It may be somewhat helpful to review this commit with `git show -w`. --- src/bitcoin-cli.cpp | 111 +++++++++++++++++++++++++------------------- 1 file changed, 62 insertions(+), 49 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index cdaabd6fab1..fd3a007dbb1 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -418,6 +418,40 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co return reply; } +/** + * ConnectAndCallRPC wraps CallRPC with -rpcwait and an exception handler. + * + * @param[in] rh Pointer to RequestHandler. + * @param[in] strMethod Reference to const string method to forward to CallRPC. + * @returns the RPC response as a UniValue object. + * @throws a CConnectionFailed std::runtime_error if connection failed or RPC server still in warmup. + */ +static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector& args) +{ + UniValue response(UniValue::VOBJ); + // Execute and handle connection failures with -rpcwait. + const bool fWait = gArgs.GetBoolArg("-rpcwait", false); + do { + try { + response = CallRPC(rh, strMethod, args); + if (fWait) { + const UniValue& error = find_value(response, "error"); + if (!error.isNull() && error["code"].get_int() == RPC_IN_WARMUP) { + throw CConnectionFailed("server in warmup"); + } + } + break; // Connection succeeded, no need to retry. + } catch (const CConnectionFailed&) { + if (fWait) { + UninterruptibleSleep(std::chrono::milliseconds{1000}); + } else { + throw; + } + } + } while (fWait); + return response; +} + static int CommandLineRPC(int argc, char *argv[]) { std::string strPrint; @@ -485,62 +519,41 @@ static int CommandLineRPC(int argc, char *argv[]) method = args[0]; args.erase(args.begin()); // Remove trailing method name from arguments vector } + const UniValue reply = ConnectAndCallRPC(rh.get(), method, args); - // Execute and handle connection failures with -rpcwait - const bool fWait = gArgs.GetBoolArg("-rpcwait", false); - do { - try { - const UniValue reply = CallRPC(rh.get(), method, args); + // Parse reply + UniValue result = find_value(reply, "result"); + const UniValue& error = find_value(reply, "error"); + if (!error.isNull()) { + // Error + strPrint = "error: " + error.write(); + nRet = abs(error["code"].get_int()); + if (error.isObject()) { + const UniValue& errCode = find_value(error, "code"); + const UniValue& errMsg = find_value(error, "message"); + strPrint = errCode.isNull() ? "" : ("error code: " + errCode.getValStr() + "\n"); - // Parse reply - const UniValue& result = find_value(reply, "result"); - const UniValue& error = find_value(reply, "error"); - - if (!error.isNull()) { - // Error - int code = error["code"].get_int(); - if (fWait && code == RPC_IN_WARMUP) - throw CConnectionFailed("server in warmup"); - strPrint = "error: " + error.write(); - nRet = abs(code); - if (error.isObject()) - { - UniValue errCode = find_value(error, "code"); - UniValue errMsg = find_value(error, "message"); - strPrint = errCode.isNull() ? "" : "error code: "+errCode.getValStr()+"\n"; - - if (errMsg.isStr()) - strPrint += "error message:\n"+errMsg.get_str(); - - if (errCode.isNum() && errCode.get_int() == RPC_WALLET_NOT_SPECIFIED) { - strPrint += "\nTry adding \"-rpcwallet=\" option to bitcoin-cli command line."; - } - } - } else { - // Result - if (result.isNull()) - strPrint = ""; - else if (result.isStr()) - strPrint = result.get_str(); - else - strPrint = result.write(2); + if (errMsg.isStr()) { + strPrint += ("error message:\n" + errMsg.get_str()); + } + if (errCode.isNum() && errCode.get_int() == RPC_WALLET_NOT_SPECIFIED) { + strPrint += "\nTry adding \"-rpcwallet=\" option to bitcoin-cli command line."; } - // Connection succeeded, no need to retry. - break; } - catch (const CConnectionFailed&) { - if (fWait) - UninterruptibleSleep(std::chrono::milliseconds{1000}); - else - throw; + } else { + // Result + if (result.isNull()) { + strPrint = ""; + } else if (result.isStr()) { + strPrint = result.get_str(); + } else { + strPrint = result.write(2); } - } while (fWait); - } - catch (const std::exception& e) { + } + } catch (const std::exception& e) { strPrint = std::string("error: ") + e.what(); nRet = EXIT_FAILURE; - } - catch (...) { + } catch (...) { PrintExceptionContinue(nullptr, "CommandLineRPC()"); throw; } From 743077544b5420246ef29e0b708c90e3a8dfeeb6 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Fri, 10 Apr 2020 22:34:43 +0200 Subject: [PATCH 2/6] cli: lift -rpcwallet logic up to CommandLineRPC() to allow passing rpcwallet independently from the -rpcwallet user option, and to move the logic to the top-level layer where most of the other option args are handled. --- src/bitcoin-cli.cpp | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index fd3a007dbb1..3a9e93f51f7 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -304,7 +305,7 @@ public: } }; -static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector& args) +static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector& args, const Optional& rpcwallet = {}) { std::string host; // In preference order, we choose the following for the port: @@ -369,14 +370,12 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co // check if we should use a special wallet endpoint std::string endpoint = "/"; - if (!gArgs.GetArgs("-rpcwallet").empty()) { - std::string walletName = gArgs.GetArg("-rpcwallet", ""); - char *encodedURI = evhttp_uriencode(walletName.data(), walletName.size(), false); + if (rpcwallet) { + char* encodedURI = evhttp_uriencode(rpcwallet->data(), rpcwallet->size(), false); if (encodedURI) { - endpoint = "/wallet/"+ std::string(encodedURI); + endpoint = "/wallet/" + std::string(encodedURI); free(encodedURI); - } - else { + } else { throw CConnectionFailed("uri-encode failed"); } } @@ -423,17 +422,18 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co * * @param[in] rh Pointer to RequestHandler. * @param[in] strMethod Reference to const string method to forward to CallRPC. + * @param[in] rpcwallet Reference to const optional string wallet name to forward to CallRPC. * @returns the RPC response as a UniValue object. * @throws a CConnectionFailed std::runtime_error if connection failed or RPC server still in warmup. */ -static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector& args) +static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector& args, const Optional& rpcwallet = {}) { UniValue response(UniValue::VOBJ); // Execute and handle connection failures with -rpcwait. const bool fWait = gArgs.GetBoolArg("-rpcwait", false); do { try { - response = CallRPC(rh, strMethod, args); + response = CallRPC(rh, strMethod, args, rpcwallet); if (fWait) { const UniValue& error = find_value(response, "error"); if (!error.isNull() && error["code"].get_int() == RPC_IN_WARMUP) { @@ -519,7 +519,9 @@ static int CommandLineRPC(int argc, char *argv[]) method = args[0]; args.erase(args.begin()); // Remove trailing method name from arguments vector } - const UniValue reply = ConnectAndCallRPC(rh.get(), method, args); + Optional wallet_name{}; + if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", ""); + const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name); // Parse reply UniValue result = find_value(reply, "result"); From 9f01849a498a70616506bdcda8ce6897aa29e664 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 12 Apr 2020 21:28:19 +0200 Subject: [PATCH 3/6] cli: create GetWalletBalances() to fetch multiwallet balances --- src/bitcoin-cli.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 3a9e93f51f7..4d5b8f847be 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -452,6 +452,30 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& str return response; } +/** + * GetWalletBalances calls listwallets; if more than one wallet is loaded, it then + * fetches mine.trusted balances for each loaded wallet and pushes them to `result`. + * + * @param result Reference to UniValue object the wallet names and balances are pushed to. + */ +static void GetWalletBalances(UniValue& result) +{ + std::unique_ptr rh{MakeUnique()}; + const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", /* args=*/{}); + if (!find_value(listwallets, "error").isNull()) return; + const UniValue& wallets = find_value(listwallets, "result"); + if (wallets.size() <= 1) return; + + UniValue balances(UniValue::VOBJ); + for (const UniValue& wallet : wallets.getValues()) { + const std::string wallet_name = wallet.get_str(); + const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", /* args=*/{}, wallet_name); + const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"]; + balances.pushKV(wallet_name, balance); + } + result.pushKV("balances", balances); +} + static int CommandLineRPC(int argc, char *argv[]) { std::string strPrint; From afce85eb994384246e455b766549c3206cb059e0 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 3 May 2020 11:10:24 +0200 Subject: [PATCH 4/6] cli: use GetWalletBalances() functionality for -getinfo and replace GetBoolArg with IsArgSet as we only want to know if the arg is passed; we do not need the value. --- src/bitcoin-cli.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 4d5b8f847be..045442c9ff3 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -532,9 +532,8 @@ static int CommandLineRPC(int argc, char *argv[]) } std::unique_ptr rh; std::string method; - if (gArgs.GetBoolArg("-getinfo", false)) { + if (gArgs.IsArgSet("-getinfo")) { rh.reset(new GetinfoRequestHandler()); - method = ""; } else { rh.reset(new DefaultRequestHandler()); if (args.size() < 1) { @@ -567,6 +566,9 @@ static int CommandLineRPC(int argc, char *argv[]) } } } else { + if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) { + GetWalletBalances(result); // fetch multiwallet balances and append to result + } // Result if (result.isNull()) { strPrint = ""; From 903b6c117f541ea9258d3234ffcf59427344e668 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sun, 12 Apr 2020 21:46:16 +0200 Subject: [PATCH 5/6] rpc: drop unused JSONRPCProcessBatchReply size arg, refactor --- src/bitcoin-cli.cpp | 2 +- src/rpc/request.cpp | 10 +++++----- src/rpc/request.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp index 045442c9ff3..45a586cd120 100644 --- a/src/bitcoin-cli.cpp +++ b/src/bitcoin-cli.cpp @@ -251,7 +251,7 @@ public: UniValue ProcessReply(const UniValue &batch_in) override { UniValue result(UniValue::VOBJ); - std::vector batch = JSONRPCProcessBatchReply(batch_in, batch_in.size()); + const std::vector batch = JSONRPCProcessBatchReply(batch_in); // Errors in getnetworkinfo() and getblockchaininfo() are fatal, pass them on; // getwalletinfo() and getbalances() are allowed to fail if there is no wallet. if (!batch[ID_NETWORKINFO]["error"].isNull()) { diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp index 56cac6661ec..7fef45f50ef 100644 --- a/src/rpc/request.cpp +++ b/src/rpc/request.cpp @@ -130,20 +130,20 @@ void DeleteAuthCookie() } } -std::vector JSONRPCProcessBatchReply(const UniValue &in, size_t num) +std::vector JSONRPCProcessBatchReply(const UniValue& in) { if (!in.isArray()) { throw std::runtime_error("Batch must be an array"); } + const size_t num {in.size()}; std::vector batch(num); - for (size_t i=0; i= num) { - throw std::runtime_error("Batch member id larger than size"); + throw std::runtime_error("Batch member id is larger than batch size"); } batch[id] = rec; } diff --git a/src/rpc/request.h b/src/rpc/request.h index 99eb4f9354a..d6080c3cf30 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -22,7 +22,7 @@ bool GetAuthCookie(std::string *cookie_out); /** Delete RPC authentication cookie from disk */ void DeleteAuthCookie(); /** Parse JSON-RPC batch reply into a vector */ -std::vector JSONRPCProcessBatchReply(const UniValue &in, size_t num); +std::vector JSONRPCProcessBatchReply(const UniValue& in); class JSONRPCRequest { From 5edad5ce5d3f15b694bf3fad0300c6446674b554 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Sat, 11 Apr 2020 03:46:07 +0200 Subject: [PATCH 6/6] test: add -getinfo multiwallet functional tests and improve the existing -getinfo -rpcwallet tests. --- test/functional/interface_bitcoin_cli.py | 47 +++++++++++++++++------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index 1c943052208..7530e7daf68 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -67,6 +67,7 @@ class TestBitcoinCli(BitcoinTestFramework): if self.is_wallet_compiled(): self.log.info("Test -getinfo and bitcoin-cli getwalletinfo return expected wallet info") assert_equal(cli_get_info['balance'], BALANCE) + assert 'balances' not in cli_get_info.keys() wallet_info = self.nodes[0].getwalletinfo() assert_equal(cli_get_info['keypoolsize'], wallet_info['keypoolsize']) assert_equal(cli_get_info['unlocked_until'], wallet_info['unlocked_until']) @@ -76,42 +77,60 @@ class TestBitcoinCli(BitcoinTestFramework): # Setup to test -getinfo and -rpcwallet= with multiple wallets. wallets = ['', 'Encrypted', 'secret'] - amounts = [Decimal('59.999928'), Decimal(9), Decimal(31)] + amounts = [BALANCE + Decimal('9.999928'), Decimal(9), Decimal(31)] self.nodes[0].createwallet(wallet_name=wallets[1]) self.nodes[0].createwallet(wallet_name=wallets[2]) w1 = self.nodes[0].get_wallet_rpc(wallets[0]) w2 = self.nodes[0].get_wallet_rpc(wallets[1]) w3 = self.nodes[0].get_wallet_rpc(wallets[2]) w1.walletpassphrase(password, self.rpc_timeout) + w2.encryptwallet(password) w1.sendtoaddress(w2.getnewaddress(), amounts[1]) w1.sendtoaddress(w3.getnewaddress(), amounts[2]) # Mine a block to confirm; adds a block reward (50 BTC) to the default wallet. self.nodes[0].generate(1) - self.log.info("Test -getinfo with multiple wallets loaded returns no balance") - assert_equal(set(self.nodes[0].listwallets()), set(wallets)) - assert 'balance' not in self.nodes[0].cli('-getinfo').send_cli().keys() - self.log.info("Test -getinfo with multiple wallets and -rpcwallet returns specified wallet balance") for i in range(len(wallets)): - cli_get_info = self.nodes[0].cli('-getinfo').send_cli('-rpcwallet={}'.format(wallets[i])) + cli_get_info = self.nodes[0].cli('-getinfo', '-rpcwallet={}'.format(wallets[i])).send_cli() + assert 'balances' not in cli_get_info.keys() assert_equal(cli_get_info['balance'], amounts[i]) - self.log.info("Test -getinfo with multiple wallets and -rpcwallet=non-existing-wallet returns no balance") - assert 'balance' not in self.nodes[0].cli('-getinfo').send_cli('-rpcwallet=does-not-exist').keys() + self.log.info("Test -getinfo with multiple wallets and -rpcwallet=non-existing-wallet returns no balances") + cli_get_info_keys = self.nodes[0].cli('-getinfo', '-rpcwallet=does-not-exist').send_cli().keys() + assert 'balance' not in cli_get_info_keys + assert 'balances' not in cli_get_info_keys + + self.log.info("Test -getinfo with multiple wallets returns all loaded wallet names and balances") + assert_equal(set(self.nodes[0].listwallets()), set(wallets)) + cli_get_info = self.nodes[0].cli('-getinfo').send_cli() + assert 'balance' not in cli_get_info.keys() + assert_equal(cli_get_info['balances'], {k: v for k, v in zip(wallets, amounts)}) + + # Unload the default wallet and re-verify. + self.nodes[0].unloadwallet(wallets[0]) + assert wallets[0] not in self.nodes[0].listwallets() + cli_get_info = self.nodes[0].cli('-getinfo').send_cli() + assert 'balance' not in cli_get_info.keys() + assert_equal(cli_get_info['balances'], {k: v for k, v in zip(wallets[1:], amounts[1:])}) self.log.info("Test -getinfo after unloading all wallets except a non-default one returns its balance") - self.nodes[0].unloadwallet(wallets[0]) self.nodes[0].unloadwallet(wallets[2]) assert_equal(self.nodes[0].listwallets(), [wallets[1]]) - assert_equal(self.nodes[0].cli('-getinfo').send_cli()['balance'], amounts[1]) + cli_get_info = self.nodes[0].cli('-getinfo').send_cli() + assert 'balances' not in cli_get_info.keys() + assert_equal(cli_get_info['balance'], amounts[1]) - self.log.info("Test -getinfo -rpcwallet=remaining-non-default-wallet returns its balance") - assert_equal(self.nodes[0].cli('-getinfo').send_cli('-rpcwallet={}'.format(wallets[1]))['balance'], amounts[1]) + self.log.info("Test -getinfo with -rpcwallet=remaining-non-default-wallet returns only its balance") + cli_get_info = self.nodes[0].cli('-getinfo', '-rpcwallet={}'.format(wallets[1])).send_cli() + assert 'balances' not in cli_get_info.keys() + assert_equal(cli_get_info['balance'], amounts[1]) - self.log.info("Test -getinfo with -rpcwallet=unloaded wallet returns no balance") - assert 'balance' not in self.nodes[0].cli('-getinfo').send_cli('-rpcwallet={}'.format(wallets[2])).keys() + self.log.info("Test -getinfo with -rpcwallet=unloaded wallet returns no balances") + cli_get_info = self.nodes[0].cli('-getinfo', '-rpcwallet={}'.format(wallets[2])).send_cli() + assert 'balance' not in cli_get_info_keys + assert 'balances' not in cli_get_info_keys else: self.log.info("*** Wallet not compiled; cli getwalletinfo and -getinfo wallet tests skipped") self.nodes[0].generate(1) # maintain block parity with the wallet_compiled conditional branch