cli: display multiwallet balances in -getinfo #18594

pull jonatack wants to merge 6 commits into bitcoin:master from jonatack:cli-getinfo-multiwallet-balances changing 4 files +143 −83
  1. jonatack commented at 2:03 PM on April 11, 2020: contributor

    This PR is a client-side version of #18453, per review feedback there and review club discussions. 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

    $ 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

    $ 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 #17314 and #18453 (comment).

  2. jonatack force-pushed on Apr 11, 2020
  3. DrahtBot added the label Tests on Apr 11, 2020
  4. jonatack force-pushed on Apr 11, 2020
  5. jonatack force-pushed on Apr 11, 2020
  6. promag commented at 12:18 AM on April 12, 2020: member

    Concept ACK

    interface_bitcoin_cli.py fails in no wallet job.

  7. fanquake commented at 2:49 AM on April 12, 2020: member

    It seems this is in part reverting changes that were just merged in #18574 (split out of #18453), and refactoring to handle multiwallet.

    I understand splitting up changes, and maybe it doesn't matter so much in this instance, but it's a bit of code/review churn if we're PR'ing and merging refactors only to essentially undo the changes and do something else in a different PR a few hours later.

  8. jonatack commented at 10:01 AM on April 12, 2020: contributor

    I empathise. See #18453 (comment). #18574 was the only change that seemed to have consensus, so it seemed best to split it out for merge and propose client-side code in this PR to test and compare with the server-side code in #18453.

    EDIT: This PR no longer touches the changes merged in #18574.

  9. DrahtBot commented at 1:17 PM on April 12, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #16439 (RPC: support "@height" in place of blockhash for getblock etc by ajtowns)

    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.

  10. DrahtBot cross-referenced this on Apr 12, 2020 from issue rpc, cli: add multiwallet balances rpc and use it in -getinfo by jonatack
  11. jonatack force-pushed on Apr 12, 2020
  12. jonatack commented at 9:35 PM on April 12, 2020: contributor

    Updated with the following:

    • ensure it works when built without the wallet
    • extract the connection try/wait/failure logic to be callable for each connection, so we can use this for calling listwallets and getbalances to be more robust
    • drop an unused arg in JSONRPCProcessBatchReply as requested in #18574 (review)
  13. jonatack force-pushed on Apr 12, 2020
  14. DrahtBot cross-referenced this on Apr 13, 2020 from issue cli: add -generate option by brakmic
  15. DrahtBot cross-referenced this on Apr 13, 2020 from issue cli/gui: support "@height" in place of blockhash for getblock on client side by ajtowns
  16. in src/bitcoin-cli.cpp:512 in e9a271b158 outdated 
     536 | +        const UniValue& error = find_value(reply, "error");
 537 | +        if (!error.isNull()) {
 538 | +            // Error
 539 | +            int code = error["code"].get_int();
 540 | +            if (code == RPC_IN_WARMUP && gArgs.GetBoolArg("-rpcwait", false)) {
 541 | +                throw CConnectionFailed("server in warmup");
    jnewbery commented at 7:27 PM on April 13, 2020:

    After commit cli: extract connection try/wait/failure logic, this throw no longer gets caught.

    jonatack commented at 9:24 PM on April 13, 2020:

    Thanks @jnewbery, good catch on the throw! Updated that commit as per:

    <details><summary>git diff e9a271b ed06899</summary> <p>

    diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 53c6f3a655..18317574ad 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -419,6 +420,13 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
    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&) {
@@ -507,12 +514,8 @@ static int CommandLineRPC(int argc, char *argv[])
         const UniValue& error = find_value(reply, "error");
         if (!error.isNull()) {
             // Error
-            int code = error["code"].get_int();
-            if (code == RPC_IN_WARMUP && gArgs.GetBoolArg("-rpcwait", false)) {
-                throw CConnectionFailed("server in warmup");
-            }
             strPrint = "error: " + error.write();
-            nRet = abs(code);
+            nRet = abs(error["code"].get_int());
             if (error.isObject()) {
                 UniValue errCode = find_value(error, "code");
                 UniValue errMsg  = find_value(error, "message");

    </p> </details>

  17. jnewbery commented at 7:27 PM on April 13, 2020: member

    Concept and approach ACK.

  18. jonatack force-pushed on Apr 13, 2020
  19. jonatack force-pushed on Apr 14, 2020
  20. DrahtBot cross-referenced this on Apr 15, 2020 from issue test: add coverage for bitcoin-cli -rpcwait by jonatack
  21. DrahtBot added the label Needs rebase on Apr 16, 2020
  22. jonatack force-pushed on Apr 16, 2020
  23. jonatack force-pushed on Apr 16, 2020
  24. DrahtBot removed the label Needs rebase on Apr 16, 2020
  25. jonatack force-pushed on Apr 16, 2020
  26. in src/rpc/request.cpp:140 in c2d63a8c05 outdated 
     137 |          throw std::runtime_error("Batch must be an array");
 138 |      }
 139 | +    const size_t num {in.size()};
 140 |      std::vector<UniValue> batch(num);
 141 | -    for (size_t i=0; i<in.size(); ++i) {
 142 | +    for (size_t i=0; i<num; ++i) {
    promag commented at 12:28 AM on April 17, 2020:

    c2d63a8c05e14cdfdaae2ebca7a6b38907c31f1c

         const size_t num {in.size()};
     std::vector<UniValue> batch(num);
-    for (size_t i=0; i<num; ++i) {
-        const UniValue &rec = in[i];
+    for (const UniValue &rec : in.getValues()) {
         if (!rec.isObject()) {
             throw std::runtime_error("Batch member must be an object");
    jonatack commented at 1:09 AM on April 17, 2020:

    That's better -- thanks! Done.

  27. in src/bitcoin-cli.cpp:278 in c2d63a8c05 outdated 
     272 | @@ -275,9 +273,6 @@ class GetinfoRequestHandler: public BaseRequestHandler
 273 |              }
 274 |              result.pushKV("paytxfee", batch[ID_WALLETINFO]["result"]["paytxfee"]);
 275 |          }
 276 | -        if (!batch[ID_BALANCES]["result"].isNull()) {
    promag commented at 12:42 AM on April 17, 2020:

    Any reason to drop this (and break existing scripts)? Especially when -rpcwallet is set - and in this case all balances aren't even necessary. And if -rpcwallet is not set but listwallets gives just one wallet then it could still show "balance": after all, the server is defaulting to the unique one.

    jonatack commented at 12:59 AM on April 17, 2020:

    Thanks for reviewing @promag. I don't know if API stability with -getinfo is an issue if it's intended for human use. ISTM it's better to display the balances consistently, whether one wallet or several?

    promag commented at 1:10 AM on April 17, 2020:

    I mean that you could leave "balance" untouched and just add balances. Later we could drop it. Only a suggestion, but

    1. if -rpcwallet is set why show all balances? Sounds conceptually wrong. Note that getwalletinfo uses that (or are you planning to call getwalletinfo for all wallets?)
    2. this is not an API but it's a command people can use in scripts so why break it for no good reason?
    promag commented at 10:32 AM on April 17, 2020:

    Note that if multiple wallets are loaded and if -rpcwallet is not set then getwalletinfo fails, which results in not displaying keypoolsize and paytxfee.

    So I think if -rpcwallet is set (or just one wallet is loaded), balances shouldn't be displayed.

    jonatack commented at 5:55 PM on April 20, 2020:

    Done

  28. jonatack force-pushed on Apr 17, 2020
  29. jonatack cross-referenced this on Apr 17, 2020 from issue interface_bitcoin_cli.py fails intermittently by MarcoFalke
  30. DrahtBot cross-referenced this on Apr 17, 2020 from issue test: add wait_for_cookie_credentials() to framework for rpcwait tests by jonatack
  31. michaelfolkson commented at 2:09 PM on April 18, 2020: contributor

    ACK ea59b18387c4b26240e1f087c568408922b3c940

    Built, ran tests, tested manually by creating new wallet, transferring funds to it and unloading. Light code review.

  32. DrahtBot added the label Needs rebase on Apr 20, 2020
  33. jonatack force-pushed on Apr 20, 2020
  34. DrahtBot removed the label Needs rebase on Apr 20, 2020
  35. jonatack force-pushed on Apr 20, 2020
  36. jonatack commented at 6:01 PM on April 20, 2020: contributor

    PR updated per @promag's review feedback to display wallet names and balances for all loaded wallets when more than one wallet is loaded (e.g. you are in "multiwallet mode") and -rpcwallet= is not passed; otherwise, behavior is unchanged. @fanquake, this PR no longer touches the changes in #18574. @jnewbery, @promag, @michaelfolkson, can you please re-review?

  37. jonatack force-pushed on Apr 20, 2020
  38. jonatack force-pushed on Apr 20, 2020
  39. jonatack force-pushed on Apr 20, 2020
  40. jb55 commented at 10:08 PM on April 20, 2020: contributor

    Concept ACK, but I wonder why in 208be5a811eee4f4d4d336a9e47c2b36760621b5 this is hardcoded to getinfo? Couldn't this be generalized to any command with almost no extra work (return results grouped by wallet name), or am I missing something?

    edit: hmm after reading some of the linked issues it looks like this is trying to achieve something very specific, but my suggestion could be a future improvement on top of this.

  41. jonatack commented at 10:20 PM on April 20, 2020: contributor

    Thanks @jb55, I agree, but there isn't currently consensus on what the server-side API interface should be and if one should be done at all. I learned from the first PR that a client-side version embedded into -getinfo as proposed here will be easier to merge due to more clear consensus and because it isn't subject to API constraints. I see this as a solution that can be merged now, and maybe a server-side one might emerge (or not) later, and could be called from -getinfo as well to batch the calls to getbalances. Don't hesitate to ACK if you'd like to see it move forward!

  42. jonatack force-pushed on Apr 21, 2020
  43. jonatack cross-referenced this on Apr 21, 2020 from issue Allow JSON RPC "batches" for multiwallet by jonasschnelli
  44. jonatack cross-referenced this on Apr 21, 2020 from issue test: add coverage for -rpcwallet cli option by jonatack
  45. jonatack force-pushed on Apr 24, 2020
  46. MarcoFalke referenced this in commit 5f19155e5b on Apr 24, 2020
  47. DrahtBot added the label Needs rebase on Apr 25, 2020
  48. sidhujag referenced this in commit a1abc553ae on Apr 25, 2020
  49. jonatack force-pushed on Apr 25, 2020
  50. DrahtBot removed the label Needs rebase on Apr 25, 2020
  51. jonatack cross-referenced this on Apr 25, 2020 from issue Re-thinking `bitcoin-cli -getinfo` by laanwj
  52. brakmic commented at 3:46 PM on April 30, 2020: contributor

    ACK https://github.com/bitcoin/bitcoin/commit/9fa9882fbbf4324f36cbf1008642adf43436be53

    Built, run and tested on macOS Catalina 10.15.4 Manual testing with several wallets successful:

    ./src/bitcoin-cli -regtest -getinfo
{
  "version": 209900,
  "blocks": 205,
  "headers": 205,
  "verificationprogress": 1,
  "timeoffset": 0,
  "connections": 0,
  "proxy": "",
  "difficulty": 4.656542373906925e-10,
  "chain": "regtest",
  "relayfee": 0.00001000,
  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications",
  "balances": {
    "": 5000.00000000,
    "other": 250.00000000,
    "secret_wallet": 0.00000000
  }
}

    Execution of ./test/functional/wallet_multiwallet.py --usecli was successful.

  53. in src/bitcoin-cli.cpp:372 in 9fa9882fbb outdated 
     368 | @@ -369,14 +369,13 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 369 |  
 370 |      // check if we should use a special wallet endpoint
 371 |      std::string endpoint = "/";
 372 | -    if (!gArgs.GetArgs("-rpcwallet").empty()) {
 373 | -        std::string walletName = gArgs.GetArg("-rpcwallet", "");
 374 | +    if (rpcwallet || !gArgs.GetArgs("-rpcwallet").empty()) {
    jnewbery commented at 9:58 PM on May 1, 2020:

    It seems a bit confusing to have two different ways to specify the wallet here (an entry in the global gArgs key-value store and a function parameter). Can you make the outer CommandLineRPC() function parse the command line -rpcwallet argument and pass it into ConnectAndCallRPC() function?

    jonatack commented at 11:36 AM on May 4, 2020:

    Great point. Done; much better.

  54. in src/bitcoin-cli.cpp:440 in 9fa9882fbb outdated 
     427 | +            response = CallRPC(rh, strMethod, args, rpcwallet);
 428 | +
 429 | +            if (fWait) {
 430 | +                const UniValue& error = find_value(response, "error");
 431 | +                if (!error.isNull() && error["code"].get_int() == RPC_IN_WARMUP) {
 432 | +                    throw CConnectionFailed("server in warmup");
    jnewbery commented at 10:06 PM on May 1, 2020:

    observation: using throw/catch for control flow is generally considered an anti-pattern. This whole function could be tidied up by changing this block to UninterruptibleSleep(std::chrono::milliseconds{1000}); continue;.

    (This PR simply moves this code to its own function, so don't feel obligated to change this. It could be done as a follow-up, or not at all!)

    jonatack commented at 11:32 AM on May 4, 2020:

    Thanks, John. I tried but it wasn't working out in a way that was tidier, so leaving it be for now.

  55. in src/bitcoin-cli.cpp:543 in 9fa9882fbb outdated 
     568 | +            // was called without -rpcwallet and more than one wallet is loaded.
 569 | +            if (gArgs.GetBoolArg("-getinfo", false) && gArgs.GetArgs("-rpcwallet").empty()) {
 570 | +                rh.reset(new DefaultRequestHandler());
 571 | +                const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
 572 | +                const UniValue& wallets = find_value(listwallets, "result");
 573 | +                    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
    jnewbery commented at 10:12 PM on May 1, 2020:

    This line is over-indented. Should be aligned with the line above.

    jonatack commented at 11:34 AM on May 4, 2020:

    Oops, thanks John! Fixed.

  56. in src/bitcoin-cli.cpp:541 in 9fa9882fbb outdated 
     566 | +        } else {
 567 | +            // Display mine.trusted balances for all loaded wallets if -getinfo
 568 | +            // was called without -rpcwallet and more than one wallet is loaded.
 569 | +            if (gArgs.GetBoolArg("-getinfo", false) && gArgs.GetArgs("-rpcwallet").empty()) {
 570 | +                rh.reset(new DefaultRequestHandler());
 571 | +                const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
    jnewbery commented at 11:20 PM on May 1, 2020:

    args is always going to be empty here (see L239). Can you change this to an empty vector? (same for ConnectAndCallRPRC() call below)

    jonatack commented at 11:35 AM on May 4, 2020:

    Done.

  57. in src/rpc/request.cpp:133 in 9fa9882fbb outdated 
     129 | @@ -130,20 +130,20 @@ void DeleteAuthCookie()
 130 |      }
 131 |  }
 132 |  
 133 | -std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue &in, size_t num)
 134 | +std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue& in)
    jnewbery commented at 11:37 PM on May 1, 2020:

    Nice tidy up!

  58. jnewbery commented at 11:42 PM on May 1, 2020: member

    Good stuff Jon. A few style nits, but otherwise looks great

  59. jonatack force-pushed on May 4, 2020
  60. jonatack commented at 11:40 AM on May 4, 2020: contributor

    Thanks everyone for the reviews and @jnewbery for the excellent suggestions -- done and also moved the GetWalletBalances code to its own function and added Doxygen documentation for it and ConnectAndCallRPC.

    <details> <summary>Changes: git diff 9fa9882 2e7d8b9</summary> <p>

    diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 86271c4271..a3302835c3 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -17,6 +17,7 @@
 #include <util/translation.h>
 #include <util/url.h>
 
+#include <cstring>
 #include <functional>
 #include <memory>
 #include <stdio.h>
@@ -304,7 +305,7 @@ public:
     }
 };
 
-static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char *rpcwallet = nullptr)
+static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char * const rpcwallet = nullptr)
 {
     std::string host;
     // In preference order, we choose the following for the port:
@@ -369,9 +370,8 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 
     // check if we should use a special wallet endpoint
     std::string endpoint = "/";
-    if (rpcwallet || !gArgs.GetArgs("-rpcwallet").empty()) {
-        std::string walletName = rpcwallet ? rpcwallet : gArgs.GetArg("-rpcwallet", "");
-        char *encodedURI = evhttp_uriencode(walletName.data(), walletName.size(), false);
+    if (rpcwallet) {
+        char *encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
         if (encodedURI) {
             endpoint = "/wallet/"+ std::string(encodedURI);
             free(encodedURI);
@@ -417,7 +417,16 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
     return reply;
 }
 
-static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char *rpcwallet = nullptr)
+/**
+ * ConnectAndCallRPC wraps CallRPC with -rpcwait and an exception handler.
+ *
+ * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] rh         Pointer to RequestHandler.
+ * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] strMethod  Reference to const string method to forward to CallRPC.
+ * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] rpcwallet  Pointer to const c-string rpcwallet arg to forward to CallRPC.
+ * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/) the RPC response as a UniValue object.
+ * [@throws](/github-metadata-backup-bitcoin-bitcoin/contributor/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 char * const rpcwallet = nullptr)
 {
     UniValue response(UniValue::VOBJ);
     // Execute and handle connection failures with -rpcwait.
@@ -425,7 +434,6 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
     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) {
@@ -442,10 +450,35 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
             }
         }
     } 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](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] result  Reference to UniValue object the wallet names and balances are pushed to.
+ */
+static void GetWalletBalances(UniValue& result)
+{
+    std::unique_ptr<BaseRequestHandler> rh;
+    rh.reset(new DefaultRequestHandler());
+    const std::vector<std::string> args;
+    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
+    const UniValue& wallets = find_value(listwallets, "result");
+
+    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
+        UniValue balances(UniValue::VOBJ);
+        for (const UniValue& wallet : wallets.getValues()) {
+            const char * const wallet_name = wallet.get_str().c_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;
@@ -502,7 +535,7 @@ 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());
         } else {
             rh.reset(new DefaultRequestHandler());
@@ -512,7 +545,13 @@ 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);
+
+        UniValue reply;
+        if (gArgs.IsArgSet("-rpcwallet")) {
+            reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.GetArg("-rpcwallet", "").c_str());
+        } else {
+            reply = ConnectAndCallRPC(rh.get(), method, args);
+        }
 
         // Parse reply
         UniValue result = find_value(reply, "result");
@@ -534,22 +573,8 @@ static int CommandLineRPC(int argc, char *argv[])
                 }
             }
         } else {
-            // Display mine.trusted balances for all loaded wallets if -getinfo
-            // was called without -rpcwallet and more than one wallet is loaded.
-            if (gArgs.GetBoolArg("-getinfo", false) && gArgs.GetArgs("-rpcwallet").empty()) {
-                rh.reset(new DefaultRequestHandler());
-                const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
-                const UniValue& wallets = find_value(listwallets, "result");
-                    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
-                    UniValue balances(UniValue::VOBJ);
-                    for (const UniValue& wallet : wallets.getValues()) {
-                        const char *wallet_name = wallet.get_str().c_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);
-                }
+            if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
+                GetWalletBalances(result); // fetch multiwallet balances and append to result
             }
             // Result
             if (result.isNull()) {
diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
index 66bbd05d0d..7530e7daf6 100755
--- a/test/functional/interface_bitcoin_cli.py
+++ b/test/functional/interface_bitcoin_cli.py
@@ -77,7 +77,7 @@ 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])

    </p> </details>

  61. jonatack force-pushed on May 4, 2020
  62. in src/bitcoin-cli.cpp:425 in 2e7d8b9bf5 outdated 
     416 | @@ -418,6 +417,68 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 417 |      return reply;
 418 |  }
 419 |  
 420 | +/**
 421 | + * ConnectAndCallRPC wraps CallRPC with -rpcwait and an exception handler.
 422 | + *
 423 | + * @param[in] rh         Pointer to RequestHandler.
 424 | + * @param[in] strMethod  Reference to const string method to forward to CallRPC.
 425 | + * @param[in] rpcwallet  Pointer to const c-string rpcwallet arg to forward to CallRPC.
    jnewbery commented at 5:36 PM on May 19, 2020:

    nit: this comment line for rpcwallet appears one commit too early. It should be added in cli: lift -rpcwallet logic up to CommandLineRPC()

    jonatack commented at 7:57 AM on May 20, 2020:

    Good catch! Fixed.

  63. in src/bitcoin-cli.cpp:554 in 2e7d8b9bf5 outdated 
     587 | +        UniValue reply;
 588 | +        if (gArgs.IsArgSet("-rpcwallet")) {
 589 | +            reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.GetArg("-rpcwallet", "").c_str());
 590 | +        } else {
 591 | +            reply = ConnectAndCallRPC(rh.get(), method, args);
 592 | +        }
    jnewbery commented at 6:31 PM on May 19, 2020:

    Calling the same function with mostly the same params in the if and else branch seems redundant. How about:

            const char* wallet_name = nullptr;
        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "").c_str();

        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name);

    or even:

            const char * const wallet_name = gArgs.IsArgSet("-rpcwallet") ?
                                         gArgs.GetArg("-rpcwallet", "").c_str() :
                                         nullptr;

    (but generally I prefer to be a bit more verbose and avoid ternary operators).

    jonatack commented at 8:18 AM on May 20, 2020:

    I wanted to do this as well, but with C-style strings it causes issues (unless wrapped in the same block scope IIRC) and test/functional/wallet_multiwallet.py --usecli fails.

    jonatack commented at 8:26 AM on May 20, 2020:

    Inlined the wallet_name logic as a ternary in one call.

    jnewbery commented at 9:48 PM on May 20, 2020:

    My suggestion has a horrible bug, which only revealed itself when the wallet name is long enough. c_str() returns a char* that's only valid as long as the underlying std::string is in scope. In gArgs.GetArg("-rpcwallet", "").c_str(), the gArgs.GetArg("-rpcwallet", "") is a temporary that goes out of scope as soon as the statement ends, so wallet_name is pointing to freed memory. For short wallet names, that memory might not get reused, and the code that dereferences it still works, but for longer names, it fails.

    I'm sorry to keep suggesting you move this around, but how about using a Optional<std::string> as the optional wallet name? The ConnectAndCallRPC() function signatures becomes:

    static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, Optional<std::string> rpcwallet = {})

    the wallet parsing code becomes:

            Optional<std::string> wallet_name {};

        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");

    and the code in CallRPC() changes to:

        // check if we should use a special wallet endpoint
    std::string endpoint = "/";
    if (rpcwallet) {
        char* encodedURI = evhttp_uriencode(rpcwallet->data(), rpcwallet->size(), false);
...
    jonatack commented at 8:06 AM on May 21, 2020:

    Thanks, John! Much better. Suggestions gratefully taken, passed as const reference (per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in), added the explicit #include <optional.h> header, and changed GetWalletBalances to pass const std::string wallet_name.

    Edit: since much of the time rpcwallet is nullptr, I'm not sure whether it's better in this case to pass by value for the nullptr case or by const reference for the string case. Likely little difference and by value is simpler and more straightforward.

    jonatack commented at 8:08 AM on May 21, 2020:

    (I had run into this c_str() issue 3 weeks ago and wondered what the better solution was. Thanks again!)

  64. in src/bitcoin-cli.cpp:460 in 2e7d8b9bf5 outdated 
     455 | +
 456 | +/**
 457 | + * GetWalletBalances calls listwallets; if more than one wallet is loaded, it then
 458 | + * fetches mine.trusted balances for each loaded wallet and pushes them to `result`.
 459 | + *
 460 | + * @param[in] result  Reference to UniValue object the wallet names and balances are pushed to.
    jnewbery commented at 6:35 PM on May 19, 2020:

    nit: this is an in/out parameter, since it gets appended to by the function.

    jonatack commented at 7:58 AM on May 20, 2020:

    Thanks. Replaced with just @param.

  65. in src/bitcoin-cli.cpp:465 in 2e7d8b9bf5 outdated 
     460 | + * @param[in] result  Reference to UniValue object the wallet names and balances are pushed to.
 461 | + */
 462 | +static void GetWalletBalances(UniValue& result)
 463 | +{
 464 | +    std::unique_ptr<BaseRequestHandler> rh;
 465 | +    rh.reset(new DefaultRequestHandler());
    jnewbery commented at 6:39 PM on May 19, 2020:

    prefer using MakeUnique to construct objects owned by unique pointers:

        std::unique_ptr<BaseRequestHandler> rh{MakeUnique<DefaultRequestHandler>()};
    jonatack commented at 7:58 AM on May 20, 2020:

    TIL, thank you! Updated.

  66. in src/bitcoin-cli.cpp:467 in 2e7d8b9bf5 outdated 
     462 | +static void GetWalletBalances(UniValue& result)
 463 | +{
 464 | +    std::unique_ptr<BaseRequestHandler> rh;
 465 | +    rh.reset(new DefaultRequestHandler());
 466 | +    const std::vector<std::string> args;
 467 | +    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
    jnewbery commented at 6:42 PM on May 19, 2020:

    nit: no need to construct a local variable. Use a temporary:

        const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", /* args=*/{});

    Same in the call to ConnectAndCallRPC() below.

    jonatack commented at 7:59 AM on May 20, 2020:

    Done.

  67. in src/bitcoin-cli.cpp:470 in 2e7d8b9bf5 outdated 
     465 | +    rh.reset(new DefaultRequestHandler());
 466 | +    const std::vector<std::string> args;
 467 | +    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
 468 | +    const UniValue& wallets = find_value(listwallets, "result");
 469 | +
 470 | +    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
    jnewbery commented at 6:54 PM on May 19, 2020:

    nit: I prefer to exit early rather than put the mainline case inside an if block. That helps avoid overnesting (not an issue here), and I think it also communicates the intent a bit better:

    if (!find_value(listwallets, "error").isNull() || wallets.size() <= 1) return;
Univalue balances(UniValue::VIBJ);
...
    jonatack commented at 8:03 AM on May 20, 2020:

    I agree (fan of guard clauses myself) but the conditional becomes a bit less readable to my eye going from an and to a negative or, so separated it into two guard conditionals (I'm admittedly not sure it's actually more readable that way.

    jnewbery commented at 9:50 PM on May 20, 2020:

    I think it's fine like that, but this is really a matter of personal taste. You should go with whichever way you prefer.

  68. jnewbery commented at 7:02 PM on May 19, 2020: member

    utACK 2e7d8b9bf584ab61295795f0521537fc3f1057df. I've left some more style nits inline, but feel free to ignore. I think this is ready for merge as-is.

    There's no need to credit reviewers in the commit logs (if we did that everywhere, the commit logs would get very long!)

  69. jonatack force-pushed on May 20, 2020
  70. jonatack commented at 8:33 AM on May 20, 2020: contributor

    Thank you @jnewbery, I appreciate your outstanding reviewing and applied your suggestions. I also applied Clang formatting to the PR changeset. Here are the changes since the last push:

    <details><summary><code>git diff 2e7d8b9 0af7d3a</code></summary><p>

    diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index a3302835c3..feea064be9 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -305,7 +305,7 @@ public:
     }
 };
 
-static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char * const rpcwallet = nullptr)
+static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
 {
     std::string host;
     // In preference order, we choose the following for the port:
@@ -371,9 +371,9 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
     // check if we should use a special wallet endpoint
     std::string endpoint = "/";
     if (rpcwallet) {
-        char *encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
+        char* encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
         if (encodedURI) {
-            endpoint = "/wallet/"+ std::string(encodedURI);
+            endpoint = "/wallet/" + std::string(encodedURI);
             free(encodedURI);
         } else {
             throw CConnectionFailed("uri-encode failed");
@@ -426,7 +426,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
  * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/) the RPC response as a UniValue object.
  * [@throws](/github-metadata-backup-bitcoin-bitcoin/contributor/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 char * const rpcwallet = nullptr)
+static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
 {
     UniValue response(UniValue::VOBJ);
     // Execute and handle connection failures with -rpcwait.
@@ -441,8 +441,7 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
                 }
             }
             break; // Connection succeeded, no need to retry.
-        }
-        catch (const CConnectionFailed&) {
+        } catch (const CConnectionFailed&) {
             if (fWait) {
                 UninterruptibleSleep(std::chrono::milliseconds{1000});
             } else {
@@ -457,26 +456,24 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
  * 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](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] result  Reference to UniValue object the wallet names and balances are pushed to.
+ * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/) result  Reference to UniValue object the wallet names and balances are pushed to.
  */
 static void GetWalletBalances(UniValue& result)
 {
-    std::unique_ptr<BaseRequestHandler> rh;
-    rh.reset(new DefaultRequestHandler());
-    const std::vector<std::string> args;
-    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
+    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;

-    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
-        UniValue balances(UniValue::VOBJ);
-        for (const UniValue& wallet : wallets.getValues()) {
-            const char * const wallet_name = wallet.get_str().c_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);

+    UniValue balances(UniValue::VOBJ);
+    for (const UniValue& wallet : wallets.getValues()) {
+        const char* const wallet_name = wallet.get_str().c_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[])
@@ -545,13 +542,7 @@ static int CommandLineRPC(int argc, char *argv[])
             method = args[0];
             args.erase(args.begin()); // Remove trailing method name from arguments vector
         }
-
-        UniValue reply;
-        if (gArgs.IsArgSet("-rpcwallet")) {
-            reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.GetArg("-rpcwallet", "").c_str());
-        } else {
-            reply = ConnectAndCallRPC(rh.get(), method, args);
-        }
+        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.IsArgSet("-rpcwallet") ? gArgs.GetArg("-rpcwallet", "").c_str() : nullptr);
 
@@ -585,12 +576,10 @@ static int CommandLineRPC(int argc, char *argv[])
                 strPrint = result.write(2);
             }
         }
-    }
-    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;
     }

    </p></details>

  71. jnewbery commented at 9:57 PM on May 20, 2020: member

    @jonatack thanks for being so responsive to review, and well done for catching the nasty bug in my previous suggestion! (https://github.com/bitcoin/bitcoin/pull/18594#discussion_r428327325).

    I have one final suggestion: switch out the c-style char * arguments for Optional<std::string>s. Using std::string seems better in almost all cases.

  72. in src/bitcoin-cli.cpp:535 in 0af7d3a5c2 outdated 
     531 | @@ -474,9 +532,8 @@ static int CommandLineRPC(int argc, char *argv[])
 532 |          }
 533 |          std::unique_ptr<BaseRequestHandler> rh;
 534 |          std::string method;
 535 | -        if (gArgs.GetBoolArg("-getinfo", false)) {
 536 | +        if (gArgs.IsArgSet("-getinfo")) {
    luke-jr commented at 3:48 AM on May 21, 2020:

    Why are you changing this?

    jonatack commented at 7:57 AM on May 21, 2020:

    IsArgSet() seemed more appropriate than GetBoolArg() as we don't need the value, only to know if the arg is set (and IsArgSet() is used elsewhere in this file for the same purpose). Updated the commit message to say why.

    luke-jr commented at 2:09 PM on May 21, 2020:

    Pretty sure this will do the wrong thing with -nogetinfo or -getinfo=0 ?

    jonatack commented at 2:28 PM on May 21, 2020:

    Thanks -- with your examples and IsArgSet, it runs as if -getinfo was passed. With GetBoolArg, it raises with "error: too few parameters (need at least command)". If that is the desired behavior, then ISTM I should not only revert this change but also use GetBoolArg at line 569 and update the tests to cover this. Confirm?

    jonatack commented at 5:08 PM on May 21, 2020:

    Added a regression test since this was failing silently and will parse the -getinfo and -rpcwallet command args as before. Thanks @luke-jr for the catch! Edit: to keep the reviews, will do in the follow-up to also add a release note.

    --- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -532,7 +532,7 @@ static int CommandLineRPC(int argc, char *argv[])
         }
         std::unique_ptr<BaseRequestHandler> rh;
         std::string method;
-        if (gArgs.IsArgSet("-getinfo")) {
+        if (gArgs.GetBoolArg("-getinfo", false)) {
             rh.reset(new GetinfoRequestHandler());
@@ -543,7 +543,7 @@ static int CommandLineRPC(int argc, char *argv[])
             args.erase(args.begin()); // Remove trailing method name from arguments vector
         }
         Optional<std::string> wallet_name{};
-        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");
+        if (!gArgs.GetArgs("-rpcwallet").empty()) wallet_name = gArgs.GetArg("-rpcwallet", "");
         const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name);
@@ -566,7 +566,7 @@ static int CommandLineRPC(int argc, char *argv[])
         } else {
-            if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
+            if (gArgs.GetBoolArg("-getinfo", false) && gArgs.GetArgs("-rpcwallet").empty()) {
                 GetWalletBalances(result); // fetch multiwallet balances and append to result
             }
diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
index 7530e7daf6..10ac82b107 100755
--- a/test/functional/interface_bitcoin_cli.py
+++ b/test/functional/interface_bitcoin_cli.py
@@ -49,6 +49,10 @@ class TestBitcoinCli(BitcoinTestFramework):
         self.log.info("Test -getinfo with arguments fails")
         assert_raises_process_error(1, "-getinfo takes no arguments", self.nodes[0].cli('-getinfo').help)
 
+        self.log.info("Test -getinfo=0 and -nogetinfo fail")
+        for command in ['-getinfo=0', '-nogetinfo']:
+            assert_raises_process_error(1, "error: too few parameters (need at least command)", self.nodes[0].cli(command).send_cli)
+
         self.log.info("Test -getinfo returns expected network and blockchain info")
    jnewbery commented at 3:42 PM on May 22, 2020:

    Seems fine, although this is a pretty pathological case. Why would anyone ever call bitcoin-cli -nogetinfo?

    jonatack commented at 12:02 PM on May 28, 2020:

    Fix and test coverage added in #19089

  73. in src/bitcoin-cli.cpp:471 in 0af7d3a5c2 outdated 
     466 | +    const UniValue& wallets = find_value(listwallets, "result");
 467 | +    if (wallets.size() <= 1) return;
 468 | +
 469 | +    UniValue balances(UniValue::VOBJ);
 470 | +    for (const UniValue& wallet : wallets.getValues()) {
 471 | +        const char* const wallet_name = wallet.get_str().c_str();
    luke-jr commented at 3:55 AM on May 21, 2020:

    Scoping issue. wallet_name will be an out-of-scope temporary here.

    Suggest just passing it as an arg, and using the UniValue in pushKV below.

        for (const UniValue& wallet : wallets.getValues()) {
        const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", /* args=*/{}, wallet.get_str().c_str());
        const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
        balances.pushKV(wallet, balance);
    }
    jonatack commented at 8:00 AM on May 21, 2020:

    Good catch, thanks! Switched to std::string after taking @jnewbery's suggestion to switch to using Optional<std::string> for the rpcwallet parameter.

  74. jonatack force-pushed on May 21, 2020
  75. 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`.
    29f2cbdeb7
  76. 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.
    743077544b
  77. cli: create GetWalletBalances() to fetch multiwallet balances 9f01849a49
  78. 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.
    afce85eb99
  79. rpc: drop unused JSONRPCProcessBatchReply size arg, refactor 903b6c117f
  80. test: add -getinfo multiwallet functional tests
    and improve the existing -getinfo -rpcwallet tests.
    5edad5ce5d
  81. jonatack force-pushed on May 21, 2020
  82. jonatack commented at 8:41 AM on May 21, 2020: contributor

    Thank you @jnewbery and @luke-jr for your excellent reviews and suggestions. I'll take more heed henceforth of the developer notes' warning about c_str(). Updated to use Optional<std::string> as per the following diff. Aside from a release note if merged, this PR should hopefully be ready.

    <details><summary><code>git diff 0af7d3a 5edad5c</code></summary><p>

    diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 #include <clientversion.h>
+#include <optional.h>
 #include <rpc/client.h>
@@ -17,7 +18,6 @@
 #include <util/url.h>
 
-#include <cstring>
 #include <functional>
@@ -305,7 +305,7 @@ public:
     }
 };
 
-static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
+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:
@@ -371,7 +371,7 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
     // check if we should use a special wallet endpoint
     std::string endpoint = "/";
     if (rpcwallet) {
-        char* encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
+        char* encodedURI = evhttp_uriencode(rpcwallet->data(), rpcwallet->size(), false);
         if (encodedURI) {
             endpoint = "/wallet/" + std::string(encodedURI);
             free(encodedURI);
@@ -422,11 +422,11 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
  *
  * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] rh         Pointer to RequestHandler.
  * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] strMethod  Reference to const string method to forward to CallRPC.
- * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] rpcwallet  Pointer to const c-string rpcwallet arg to forward to CallRPC.
+ * [@param](/github-metadata-backup-bitcoin-bitcoin/contributor/param/)[in] rpcwallet  Reference to const optional string wallet name to forward to CallRPC.
  * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/) the RPC response as a UniValue object.
  * [@throws](/github-metadata-backup-bitcoin-bitcoin/contributor/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 char* const rpcwallet = nullptr)
+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.
@@ -468,7 +468,7 @@ static void GetWalletBalances(UniValue& result)
 
     UniValue balances(UniValue::VOBJ);
     for (const UniValue& wallet : wallets.getValues()) {
-        const char* const wallet_name = wallet.get_str().c_str();
+        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);
@@ -542,7 +542,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, gArgs.IsArgSet("-rpcwallet") ? gArgs.GetArg("-rpcwallet", "").c_str() : nullptr);
+        Optional<std::string> wallet_name{};
+        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");
+        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name);

    </p></details>

  83. promag commented at 11:08 AM on May 21, 2020: member

    Tested ACK 5edad5ce5d3f15b694bf3fad0300c6446674b554.

    Does it make sense sum up balances and always have the balance?

  84. jonatack commented at 1:50 PM on May 21, 2020: contributor

    @promag why not (for a follow-up):

      "balance": 0.00003714,
  "balances": {
    "": 0.00001000,
    "Encrypted": 0.00003500,
    "day-to-day": 0.00000120,
    "side project": 0.00000094
  }
}

    What I miss with -getinfo in single-wallet mode is showing the wallet name. At some point a common interface like the above for both modes might be good, to always see the wallet name even when there is only one wallet loaded.

  85. jnewbery commented at 3:33 PM on May 21, 2020: member

    utACK 5edad5ce5d3f15b694bf3fad0300c6446674b554

    Thanks for being patient with my iterative reviews!

  86. jonatack commented at 8:31 AM on May 22, 2020: contributor

    Thanks @jnewbery, @promag and @luke-jr for reviewing. I'll add a commit to the release note follow-up that tightens up the -getinfo command parsing and adds a test as per #18594 (review).

  87. instagibbs commented at 7:06 PM on May 22, 2020: member

    concept ACK

  88. meshcollider commented at 11:44 AM on May 23, 2020: contributor

    Code review ACK 5edad5ce5d3f15b694bf3fad0300c6446674b554

    Agree that a release note followup which can include a few other things would be good, this is RTM as is.

  89. meshcollider added the label Utils/log/libs on May 23, 2020
  90. meshcollider merged this on May 23, 2020
  91. meshcollider closed this on May 23, 2020
  92. fanquake added the label Needs release note on May 23, 2020
  93. MarcoFalke removed the label Tests on May 23, 2020
  94. jonatack deleted the branch on May 23, 2020
  95. sidhujag referenced this in commit f17ae817ed on May 25, 2020
  96. jonatack referenced this in commit 4d3c9af7fc on May 28, 2020
  97. jonatack referenced this in commit 92cebdd74a on May 28, 2020
  98. jonatack referenced this in commit fb72200e01 on May 28, 2020
  99. jonatack referenced this in commit b782daafee on May 28, 2020
  100. jonatack referenced this in commit 3f194ae2a4 on May 28, 2020
  101. jonatack referenced this in commit 5af36ba875 on May 28, 2020
  102. jonatack cross-referenced this on May 28, 2020 from issue cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups by jonatack
  103. jonatack commented at 12:03 PM on May 28, 2020: contributor

    Agree that a release note followup which can include a few other things would be good

    Thanks -- done in #19089 and #19354.

  104. jonatack cross-referenced this on May 28, 2020 from issue cli: display multiwallet total balance in -getinfo by jonatack
  105. luke-jr referenced this in commit c00c055ae8 on Jun 9, 2020
  106. luke-jr referenced this in commit 79ab751e40 on Jun 9, 2020
  107. luke-jr referenced this in commit 0455407e48 on Jun 9, 2020
  108. luke-jr referenced this in commit c46c5c1c97 on Jun 9, 2020
  109. luke-jr referenced this in commit 5d58a18287 on Jun 9, 2020
  110. luke-jr referenced this in commit b1e3a87abd on Jun 13, 2020
  111. luke-jr referenced this in commit 2c1d0933b0 on Jun 13, 2020
  112. luke-jr referenced this in commit b51bd52d29 on Jun 13, 2020
  113. luke-jr referenced this in commit 4fb54cc38f on Jun 13, 2020
  114. luke-jr referenced this in commit d080d1c762 on Jun 13, 2020
  115. luke-jr referenced this in commit 9940129cdd on Jun 14, 2020
  116. luke-jr referenced this in commit d6439cc938 on Jun 14, 2020
  117. luke-jr referenced this in commit 3f491eb3e4 on Jun 14, 2020
  118. luke-jr referenced this in commit 9d20020c77 on Jun 14, 2020
  119. luke-jr referenced this in commit 57f92a81dd on Jun 14, 2020
  120. promag cross-referenced this on Jun 22, 2020 from issue doc: add release note for -getinfo displaying multiwallet balances by jonatack
  121. MarcoFalke removed the label Needs release note on Jun 27, 2020
  122. MarcoFalke referenced this in commit d342a45ca7 on Jun 27, 2020
  123. ryanofsky referenced this in commit b65072798c on Sep 28, 2020
  124. Fabcien referenced this in commit 61bb2abbe0 on Feb 18, 2021
  125. Fabcien referenced this in commit 9aa48e14e2 on Feb 18, 2021
  126. Fabcien referenced this in commit 01f2e8d875 on Feb 18, 2021
  127. Fabcien referenced this in commit 984a96569b on Feb 18, 2021
  128. Fabcien referenced this in commit b5e2564186 on Feb 18, 2021
  129. deadalnix referenced this in commit 08c813ab96 on Feb 18, 2021
  130. ryanofsky referenced this in commit dcbbb75e2c on Apr 12, 2021
  131. ryanofsky referenced this in commit 4223415ff6 on Apr 12, 2021
  132. ryanofsky referenced this in commit fba5a00126 on Apr 12, 2021
  133. ryanofsky referenced this in commit 6b8276e8f5 on Apr 12, 2021
  134. ryanofsky referenced this in commit 848b05e776 on Jun 16, 2021
  135. ryanofsky referenced this in commit 048a4ee606 on Jun 16, 2021
  136. PastaPastaPasta referenced this in commit 688eab9354 on Jun 27, 2021
  137. PastaPastaPasta referenced this in commit 318011721f on Jun 28, 2021
  138. PastaPastaPasta referenced this in commit c46412ae2c on Jun 29, 2021
  139. PastaPastaPasta referenced this in commit 0175e1e613 on Jul 1, 2021
  140. PastaPastaPasta referenced this in commit cece4977cd on Jul 1, 2021
  141. PastaPastaPasta referenced this in commit da41cd4a77 on Jul 15, 2021
  142. ryanofsky referenced this in commit c90d00e875 on Dec 30, 2021
  143. ryanofsky referenced this in commit 98ebf0b32a on Dec 30, 2021
  144. bitcoin locked this on Feb 15, 2022
Contributors
jonatackpromagfanquakeDrahtBotjnewberymichaelfolksonjb55brakmicluke-jrinstagibbsmeshcollider
Labels
Utils/log/libs
Linked (view graph)
#17314 Re-thinking `bitcoin-cli -getinfo`#18453 rpc, cli: add multiwallet balances rpc and use it in -getinfo#18574 cli: call getbalances.ismine.trusted instead of getwalletinfo.balance#18684 interface_bitcoin_cli.py fails intermittently #18715 Allow JSON RPC "batches" for multiwallet#18724 test: add coverage for -rpcwallet cli option#19089 cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups#19092 cli: display multiwallet total balance in -getinfo#19354 doc: add release note for -getinfo displaying multiwallet balances
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:54 UTC