Merge #18594: cli: display multiwallet balances in -getinfo

5edad5c test: add -getinfo multiwallet functional tests (Jon Atack)
903b6c1 rpc: drop unused JSONRPCProcessBatchReply size arg, refactor (Jon Atack)
afce85e cli: use GetWalletBalances() functionality for -getinfo (Jon Atack)
9f01849 cli: create GetWalletBalances() to fetch multiwallet balances (Jon Atack)
7430775 cli: lift -rpcwallet logic up to CommandLineRPC() (Jon Atack)
29f2cbd cli: extract connection exception handler, -rpcwait logic (Jon Atack)

Pull request description:

  This PR is a client-side version of #18453, per review feedback there and [review club discussions](https://bitcoincore.reviews/18453#meeting-log). It updates `bitcoin-cli -getinfo` on the client side to display wallet name and balance for the loaded wallets when more than one is loaded (e.g. you are in "multiwallet mode") and `-rpcwallet=` is not passed; otherwise, behavior is unchanged.

  before
  ```json
  $ bitcoin-cli -getinfo -regtest
  {
    "version": 199900,
    "blocks": 15599,
    "headers": 15599,
    "verificationprogress": 1,
    "timeoffset": 0,
    "connections": 0,
    "proxy": "",
    "difficulty": 4.656542373906925e-10,
    "chain": "regtest",
    "balance": 0.00001000,
    "relayfee": 0.00001000
  }

  ```
  after
  ```json
  $ bitcoin-cli -getinfo -regtest
  {
    "version": 199900,
    "blocks": 15599,
    "headers": 15599,
    "verificationprogress": 1,
    "timeoffset": 0,
    "connections": 0,
    "proxy": "",
    "difficulty": 4.656542373906925e-10,
    "chain": "regtest",
    "balances": {
      "": 0.00001000,
      "Encrypted": 0.00003500,
      "day-to-day": 0.00000120,
      "side project": 0.00000094
    }
  }
  ```
  -----

  `Review club` discussion about this PR is here: https://bitcoincore.reviews/18453

  This PR can be manually tested by building, creating/loading/unloading several wallets with `bitcoin-cli createwallet/loadwallet/unloadwallet` and running `bitcoin-cli -getinfo` and `bitcoin-cli -rpcwallet=<wallet-name> -getinfo`.

  `wallet_multiwallet.py --usecli` provides regression test coverage on this change, along with `interface_bitcoin_cli.py` where this PR adds test coverage.

  Credit to Wladimir J. van der Laan for the idea in bitcoin/bitcoin#17314 and bitcoin/bitcoin#18453 (comment).

ACKs for top commit:
  promag:
    Tested ACK 5edad5c.
  jnewbery:
    utACK 5edad5c
  meshcollider:
    Code review ACK 5edad5c

Tree-SHA512: 4ca36c5f6c49936b40afb605c44459c1d5b80b5bd84df634007ca276b3f6c102a0cb382f9d528370363ee32c94b0d7ffa15184578eaf8de74179e566c5c5cee5

Author: meshcollider

src/bitcoin-cli.cpp

@@ -9,6 +9,7 @@
 
 #include <chainparamsbase.h>
 #include <clientversion.h>
+#include <optional.h>
 #include <rpc/client.h>
 #include <rpc/protocol.h>
 #include <rpc/request.h>
@@ -250,7 +251,7 @@ class GetinfoRequestHandler: public BaseRequestHandler
     UniValue ProcessReply(const UniValue &batch_in) override
     {
         UniValue result(UniValue::VOBJ);
-        std::vector<UniValue> batch = JSONRPCProcessBatchReply(batch_in, batch_in.size());
+        const std::vector<UniValue> 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()) {
@@ -304,7 +305,7 @@ class DefaultRequestHandler: public BaseRequestHandler {
     }
 };
 
-static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args)
+static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const Optional<std::string>& 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");
         }
     }
@@ -418,6 +417,65 @@ 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.
+ * @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<std::string>& args, const Optional<std::string>& 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, rpcwallet);
+            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;
+}
+
+/**
+ * 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<BaseRequestHandler> rh{MakeUnique<DefaultRequestHandler>()};
+    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;
@@ -474,9 +532,8 @@ static int CommandLineRPC(int argc, char *argv[])
         }
         std::unique_ptr<BaseRequestHandler> 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) {
@@ -485,62 +542,46 @@ static int CommandLineRPC(int argc, char *argv[])
             method = args[0];
             args.erase(args.begin()); // Remove trailing method name from arguments vector
         }
-
-        // 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
-                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=<filename>\" 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);
+        Optional<std::string> 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");
+        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");
+
+                if (errMsg.isStr()) {
+                    strPrint += ("error message:\n" + errMsg.get_str());
+                }
+                if (errCode.isNum() && errCode.get_int() == RPC_WALLET_NOT_SPECIFIED) {
+                    strPrint += "\nTry adding \"-rpcwallet=<filename>\" 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 {
+            if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
+                GetWalletBalances(result); // fetch multiwallet balances and append to result
             }
-        } while (fWait);
-    }
-    catch (const std::exception& e) {
+            // Result
+            if (result.isNull()) {
+                strPrint = "";
+            } else if (result.isStr()) {
+                strPrint = result.get_str();
+            } else {
+                strPrint = result.write(2);
+            }
+        }
+    } catch (const std::exception& e) {
         strPrint = std::string("error: ") + e.what();
         nRet = EXIT_FAILURE;
-    }
-    catch (...) {
+    } catch (...) {
         PrintExceptionContinue(nullptr, "CommandLineRPC()");
         throw;
     }

src/rpc/request.cpp

@@ -130,20 +130,20 @@ void DeleteAuthCookie()
     }
 }
 
-std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue &in, size_t num)
+std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in)
 {
     if (!in.isArray()) {
         throw std::runtime_error("Batch must be an array");
     }
+    const size_t num {in.size()};
     std::vector<UniValue> batch(num);
-    for (size_t i=0; i<in.size(); ++i) {
-        const UniValue &rec = in[i];
+    for (const UniValue& rec : in.getValues()) {
         if (!rec.isObject()) {
-            throw std::runtime_error("Batch member must be object");
+            throw std::runtime_error("Batch member must be an object");
         }
         size_t id = rec["id"].get_int();
         if (id >= 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;
     }

src/rpc/request.h

@@ -26,7 +26,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<UniValue> JSONRPCProcessBatchReply(const UniValue &in, size_t num);
+std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in);
 
 class JSONRPCRequest
 {

test/functional/interface_bitcoin_cli.py

@@ -67,6 +67,7 @@ def run_test(self):
         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 @@ def run_test(self):
 
             # 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 after unloading all wallets except a non-default one returns its balance")
+            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[2])
             assert_equal(self.nodes[0].listwallets(), [wallets[1]])
-            assert_equal(self.nodes[0].cli('-getinfo').send_cli()['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=unloaded wallet returns no balance")
-            assert 'balance' not in self.nodes[0].cli('-getinfo').send_cli('-rpcwallet={}'.format(wallets[2])).keys()
+            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 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 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