cli: display multiwallet balances in -getinfo

jonatack commented at 2:03 pm on April 11, 2020: member

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

 0$ bitcoin-cli -getinfo -regtest
 1{
 2  "version": 199900,
 3  "blocks": 15599,
 4  "headers": 15599,
 5  "verificationprogress": 1,
 6  "timeoffset": 0,
 7  "connections": 0,
 8  "proxy": "",
 9  "difficulty": 4.656542373906925e-10,
10  "chain": "regtest",
11  "balance": 0.00001000,
12  "relayfee": 0.00001000
13}

after

 0$ bitcoin-cli -getinfo -regtest
 1{
 2  "version": 199900,
 3  "blocks": 15599,
 4  "headers": 15599,
 5  "verificationprogress": 1,
 6  "timeoffset": 0,
 7  "connections": 0,
 8  "proxy": "",
 9  "difficulty": 4.656542373906925e-10,
10  "chain": "regtest",
11  "balances": {
12    "": 0.00001000,
13    "Encrypted": 0.00003500,
14    "day-to-day": 0.00000120,
15    "side project": 0.00000094
16  }
17}

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).

jonatack force-pushed on Apr 11, 2020

DrahtBot added the label Tests on Apr 11, 2020

jonatack force-pushed on Apr 11, 2020

promag commented at 0:18 am on April 12, 2020: member

Concept ACK

interface_bitcoin_cli.py fails in no wallet job.

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.

jonatack commented at 10:01 am on April 12, 2020: member

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.

DrahtBot commented at 1:17 pm on April 12, 2020: member

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

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.

jonatack force-pushed on Apr 12, 2020

jonatack commented at 9:35 pm on April 12, 2020: member

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)

jonatack force-pushed on Apr 12, 2020

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:

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1index 53c6f3a655..18317574ad 100644
 2--- a/src/bitcoin-cli.cpp
 3+++ b/src/bitcoin-cli.cpp
 4@@ -419,6 +420,13 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
 5    const bool fWait = gArgs.GetBoolArg("-rpcwait", false);
 6     do {
 7         try {
 8             response = CallRPC(rh, strMethod, args, rpcwallet);
 9+
10+            if (fWait) {
11+                const UniValue& error = find_value(response, "error");
12+                if (!error.isNull() && error["code"].get_int() == RPC_IN_WARMUP) {
13+                    throw CConnectionFailed("server in warmup");
14+                }
15+            }
16             break; // Connection succeeded, no need to retry.
17         }
18         catch (const CConnectionFailed&) {
19@@ -507,12 +514,8 @@ static int CommandLineRPC(int argc, char *argv[])
20         const UniValue& error = find_value(reply, "error");
21         if (!error.isNull()) {
22             // Error
23-            int code = error["code"].get_int();
24-            if (code == RPC_IN_WARMUP && gArgs.GetBoolArg("-rpcwait", false)) {
25-                throw CConnectionFailed("server in warmup");
26-            }
27             strPrint = "error: " + error.write();
28-            nRet = abs(code);
29+            nRet = abs(error["code"].get_int());
30             if (error.isObject()) {
31                 UniValue errCode = find_value(error, "code");
32                 UniValue errMsg  = find_value(error, "message");

jnewbery commented at 7:27 pm on April 13, 2020: member

Concept and approach ACK.

jonatack force-pushed on Apr 13, 2020

jonatack force-pushed on Apr 14, 2020

DrahtBot added the label Needs rebase on Apr 16, 2020

jonatack force-pushed on Apr 16, 2020

DrahtBot removed the label Needs rebase on Apr 16, 2020

jonatack force-pushed on Apr 16, 2020

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 0:28 am on April 17, 2020:

c2d63a8c05e14cdfdaae2ebca7a6b38907c31f1c

0     const size_t num {in.size()};
1     std::vector<UniValue> batch(num);
2-    for (size_t i=0; i<num; ++i) {
3-        const UniValue &rec = in[i];
4+    for (const UniValue &rec : in.getValues()) {
5         if (!rec.isObject()) {
6             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.

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 0: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 0: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

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?)
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

jonatack force-pushed on Apr 17, 2020

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.

DrahtBot added the label Needs rebase on Apr 20, 2020

jonatack force-pushed on Apr 20, 2020

DrahtBot removed the label Needs rebase on Apr 20, 2020

jonatack force-pushed on Apr 20, 2020

jonatack commented at 6:01 pm on April 20, 2020: member

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?

jonatack force-pushed on Apr 20, 2020

jb55 commented at 10:08 pm on April 20, 2020: member

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.

jonatack commented at 10:20 pm on April 20, 2020: member

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!

jonatack force-pushed on Apr 21, 2020

jonatack force-pushed on Apr 24, 2020

MarcoFalke referenced this in commit 5f19155e5b on Apr 24, 2020

DrahtBot added the label Needs rebase on Apr 25, 2020

sidhujag referenced this in commit a1abc553ae on Apr 25, 2020

jonatack force-pushed on Apr 25, 2020

DrahtBot removed the label Needs rebase on Apr 25, 2020

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:

 0./src/bitcoin-cli -regtest -getinfo
 1{
 2  "version": 209900,
 3  "blocks": 205,
 4  "headers": 205,
 5  "verificationprogress": 1,
 6  "timeoffset": 0,
 7  "connections": 0,
 8  "proxy": "",
 9  "difficulty": 4.656542373906925e-10,
10  "chain": "regtest",
11  "relayfee": 0.00001000,
12  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications",
13  "balances": {
14    "": 5000.00000000,
15    "other": 250.00000000,
16    "secret_wallet": 0.00000000
17  }
18}

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

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.

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.

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.

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.

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!

jnewbery commented at 11:42 pm on May 1, 2020: member

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

jonatack force-pushed on May 4, 2020

jonatack commented at 11:40 am on May 4, 2020: member

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.

  0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
  1index 86271c4271..a3302835c3 100644
  2--- a/src/bitcoin-cli.cpp
  3+++ b/src/bitcoin-cli.cpp
  4@@ -17,6 +17,7 @@
  5 #include <util/translation.h>
  6 #include <util/url.h>
  7 
  8+#include <cstring>
  9 #include <functional>
 10 #include <memory>
 11 #include <stdio.h>
 12@@ -304,7 +305,7 @@ public:
 13     }
 14 };
 15 
 16-static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char *rpcwallet = nullptr)
 17+static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char * const rpcwallet = nullptr)
 18 {
 19     std::string host;
 20     // In preference order, we choose the following for the port:
 21@@ -369,9 +370,8 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 22 
 23     // check if we should use a special wallet endpoint
 24     std::string endpoint = "/";
 25-    if (rpcwallet || !gArgs.GetArgs("-rpcwallet").empty()) {
 26-        std::string walletName = rpcwallet ? rpcwallet : gArgs.GetArg("-rpcwallet", "");
 27-        char *encodedURI = evhttp_uriencode(walletName.data(), walletName.size(), false);
 28+    if (rpcwallet) {
 29+        char *encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
 30         if (encodedURI) {
 31             endpoint = "/wallet/"+ std::string(encodedURI);
 32             free(encodedURI);
 33@@ -417,7 +417,16 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 34     return reply;
 35 }
 36 
 37-static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char *rpcwallet = nullptr)
 38+/**
 39+ * ConnectAndCallRPC wraps CallRPC with -rpcwait and an exception handler.
 40+ *
 41+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] rh         Pointer to RequestHandler.
 42+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] strMethod  Reference to const string method to forward to CallRPC.
 43+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] rpcwallet  Pointer to const c-string rpcwallet arg to forward to CallRPC.
 44+ * [@returns](/bitcoin-bitcoin/contributor/returns/) the RPC response as a UniValue object.
 45+ * [@throws](/bitcoin-bitcoin/contributor/throws/) a CConnectionFailed std::runtime_error if connection failed or RPC server still in warmup.
 46+ */
 47+static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char * const rpcwallet = nullptr)
 48 {
 49     UniValue response(UniValue::VOBJ);
 50     // Execute and handle connection failures with -rpcwait.
 51@@ -425,7 +434,6 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
 52     do {
 53         try {
 54             response = CallRPC(rh, strMethod, args, rpcwallet);
 55-
 56             if (fWait) {
 57                 const UniValue& error = find_value(response, "error");
 58                 if (!error.isNull() && error["code"].get_int() == RPC_IN_WARMUP) {
 59@@ -442,10 +450,35 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
 60             }
 61         }
 62     } while (fWait);
 63-
 64     return response;
 65 }
 66 
 67+/**
 68+ * GetWalletBalances calls listwallets; if more than one wallet is loaded, it then
 69+ * fetches mine.trusted balances for each loaded wallet and pushes them to `result`.
 70+ *
 71+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] result  Reference to UniValue object the wallet names and balances are pushed to.
 72+ */
 73+static void GetWalletBalances(UniValue& result)
 74+{
 75+    std::unique_ptr<BaseRequestHandler> rh;
 76+    rh.reset(new DefaultRequestHandler());
 77+    const std::vector<std::string> args;
 78+    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
 79+    const UniValue& wallets = find_value(listwallets, "result");
 80+
 81+    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
 82+        UniValue balances(UniValue::VOBJ);
 83+        for (const UniValue& wallet : wallets.getValues()) {
 84+            const char * const wallet_name = wallet.get_str().c_str();
 85+            const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", args, wallet_name);
 86+            const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
 87+            balances.pushKV(wallet_name, balance);
 88+        }
 89+        result.pushKV("balances", balances);
 90+    }
 91+}
 92+
 93 static int CommandLineRPC(int argc, char *argv[])
 94 {
 95     std::string strPrint;
 96@@ -502,7 +535,7 @@ static int CommandLineRPC(int argc, char *argv[])
 97         }
 98         std::unique_ptr<BaseRequestHandler> rh;
 99         std::string method;
100-        if (gArgs.GetBoolArg("-getinfo", false)) {
101+        if (gArgs.IsArgSet("-getinfo")) {
102             rh.reset(new GetinfoRequestHandler());
103         } else {
104             rh.reset(new DefaultRequestHandler());
105@@ -512,7 +545,13 @@ static int CommandLineRPC(int argc, char *argv[])
106             method = args[0];
107             args.erase(args.begin()); // Remove trailing method name from arguments vector
108         }
109-        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args);
110+
111+        UniValue reply;
112+        if (gArgs.IsArgSet("-rpcwallet")) {
113+            reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.GetArg("-rpcwallet", "").c_str());
114+        } else {
115+            reply = ConnectAndCallRPC(rh.get(), method, args);
116+        }
117 
118         // Parse reply
119         UniValue result = find_value(reply, "result");
120@@ -534,22 +573,8 @@ static int CommandLineRPC(int argc, char *argv[])
121                 }
122             }
123         } else {
124-            // Display mine.trusted balances for all loaded wallets if -getinfo
125-            // was called without -rpcwallet and more than one wallet is loaded.
126-            if (gArgs.GetBoolArg("-getinfo", false) && gArgs.GetArgs("-rpcwallet").empty()) {
127-                rh.reset(new DefaultRequestHandler());
128-                const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
129-                const UniValue& wallets = find_value(listwallets, "result");
130-                    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
131-                    UniValue balances(UniValue::VOBJ);
132-                    for (const UniValue& wallet : wallets.getValues()) {
133-                        const char *wallet_name = wallet.get_str().c_str();
134-                        const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", args, wallet_name);
135-                        const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
136-                        balances.pushKV(wallet_name, balance);
137-                    }
138-                    result.pushKV("balances", balances);
139-                }
140+            if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
141+                GetWalletBalances(result); // fetch multiwallet balances and append to result
142             }
143             // Result
144             if (result.isNull()) {
145diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
146index 66bbd05d0d..7530e7daf6 100755
147--- a/test/functional/interface_bitcoin_cli.py
148+++ b/test/functional/interface_bitcoin_cli.py
149@@ -77,7 +77,7 @@ class TestBitcoinCli(BitcoinTestFramework):
150 
151             # Setup to test -getinfo and -rpcwallet= with multiple wallets.
152             wallets = ['', 'Encrypted', 'secret']
153-            amounts = [Decimal('59.999928'), Decimal(9), Decimal(31)]
154+            amounts = [BALANCE + Decimal('9.999928'), Decimal(9), Decimal(31)]
155             self.nodes[0].createwallet(wallet_name=wallets[1])
156             self.nodes[0].createwallet(wallet_name=wallets[2])

jonatack force-pushed on May 4, 2020

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.

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:

0        const char* wallet_name = nullptr;
1        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "").c_str();
2
3        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name);

or even:

0        const char * const wallet_name = gArgs.IsArgSet("-rpcwallet") ?
1                                         gArgs.GetArg("-rpcwallet", "").c_str() :
2                                         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:

0        Optional<std::string> wallet_name {};
1
2        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");

and the code in CallRPC() changes to:

0    // check if we should use a special wallet endpoint
1    std::string endpoint = "/";
2    if (rpcwallet) {
3        char* encodedURI = evhttp_uriencode(rpcwallet->data(), rpcwallet->size(), false);
4...

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!)

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.

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:

0    std::unique_ptr<BaseRequestHandler> rh{MakeUnique<DefaultRequestHandler>()};

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

TIL, thank you! Updated.

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:

0    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.

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:

0if (!find_value(listwallets, "error").isNull() || wallets.size() <= 1) return;
1Univalue balances(UniValue::VIBJ);
2...

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.

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!)

jonatack force-pushed on May 20, 2020

jonatack commented at 8:33 am on May 20, 2020: member

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:

  0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
  1index a3302835c3..feea064be9 100644
  2--- a/src/bitcoin-cli.cpp
  3+++ b/src/bitcoin-cli.cpp
  4@@ -305,7 +305,7 @@ public:
  5     }
  6 };
  7 
  8-static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char * const rpcwallet = nullptr)
  9+static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
 10 {
 11     std::string host;
 12     // In preference order, we choose the following for the port:
 13@@ -371,9 +371,9 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 14     // check if we should use a special wallet endpoint
 15     std::string endpoint = "/";
 16     if (rpcwallet) {
 17-        char *encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
 18+        char* encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
 19         if (encodedURI) {
 20-            endpoint = "/wallet/"+ std::string(encodedURI);
 21+            endpoint = "/wallet/" + std::string(encodedURI);
 22             free(encodedURI);
 23         } else {
 24             throw CConnectionFailed("uri-encode failed");
 25@@ -426,7 +426,7 @@ static UniValue CallRPC(BaseRequestHandler *rh, const std::string& strMethod, co
 26  * [@returns](/bitcoin-bitcoin/contributor/returns/) the RPC response as a UniValue object.
 27  * [@throws](/bitcoin-bitcoin/contributor/throws/) a CConnectionFailed std::runtime_error if connection failed or RPC server still in warmup.
 28  */
 29-static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& strMethod, const std::vector<std::string>& args, const char * const rpcwallet = nullptr)
 30+static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
 31 {
 32     UniValue response(UniValue::VOBJ);
 33     // Execute and handle connection failures with -rpcwait.
 34@@ -441,8 +441,7 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
 35                 }
 36             }
 37             break; // Connection succeeded, no need to retry.
 38-        }
 39-        catch (const CConnectionFailed&) {
 40+        } catch (const CConnectionFailed&) {
 41             if (fWait) {
 42                 UninterruptibleSleep(std::chrono::milliseconds{1000});
 43             } else {
 44@@ -457,26 +456,24 @@ static UniValue ConnectAndCallRPC(BaseRequestHandler *rh, const std::string& str
 45  * GetWalletBalances calls listwallets; if more than one wallet is loaded, it then
 46  * fetches mine.trusted balances for each loaded wallet and pushes them to `result`.
 47  *
 48- * [@param](/bitcoin-bitcoin/contributor/param/)[in] result  Reference to UniValue object the wallet names and balances are pushed to.
 49+ * [@param](/bitcoin-bitcoin/contributor/param/) result  Reference to UniValue object the wallet names and balances are pushed to.
 50  */
 51 static void GetWalletBalances(UniValue& result)
 52 {
 53-    std::unique_ptr<BaseRequestHandler> rh;
 54-    rh.reset(new DefaultRequestHandler());
 55-    const std::vector<std::string> args;
 56-    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", args);
 57+    std::unique_ptr<BaseRequestHandler> rh{MakeUnique<DefaultRequestHandler>()};
 58+    const UniValue listwallets = ConnectAndCallRPC(rh.get(), "listwallets", /* args=*/{});
 59+    if (!find_value(listwallets, "error").isNull()) return;
 60     const UniValue& wallets = find_value(listwallets, "result");
 61+    if (wallets.size() <= 1) return;
 62
 63-    if (find_value(listwallets, "error").isNull() && wallets.size() > 1) {
 64-        UniValue balances(UniValue::VOBJ);
 65-        for (const UniValue& wallet : wallets.getValues()) {
 66-            const char * const wallet_name = wallet.get_str().c_str();
 67-            const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", args, wallet_name);
 68-            const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
 69-            balances.pushKV(wallet_name, balance);
 70-        }
 71-        result.pushKV("balances", balances);
 72
 73+    UniValue balances(UniValue::VOBJ);
 74+    for (const UniValue& wallet : wallets.getValues()) {
 75+        const char* const wallet_name = wallet.get_str().c_str();
 76+        const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", /* args=*/{}, wallet_name);
 77+        const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
 78+        balances.pushKV(wallet_name, balance);
 79     }
 80+    result.pushKV("balances", balances);
 81 }
 82 
 83 static int CommandLineRPC(int argc, char *argv[])
 84@@ -545,13 +542,7 @@ static int CommandLineRPC(int argc, char *argv[])
 85             method = args[0];
 86             args.erase(args.begin()); // Remove trailing method name from arguments vector
 87         }
 88-
 89-        UniValue reply;
 90-        if (gArgs.IsArgSet("-rpcwallet")) {
 91-            reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.GetArg("-rpcwallet", "").c_str());
 92-        } else {
 93-            reply = ConnectAndCallRPC(rh.get(), method, args);
 94-        }
 95+        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.IsArgSet("-rpcwallet") ? gArgs.GetArg("-rpcwallet", "").c_str() : nullptr);
 96 
 97@@ -585,12 +576,10 @@ static int CommandLineRPC(int argc, char *argv[])
 98                 strPrint = result.write(2);
 99             }
100         }
101-    }
102-    catch (const std::exception& e) {
103+    } catch (const std::exception& e) {
104         strPrint = std::string("error: ") + e.what();
105         nRet = EXIT_FAILURE;
106-    }
107-    catch (...) {
108+    } catch (...) {
109         PrintExceptionContinue(nullptr, "CommandLineRPC()");
110         throw;
111     }

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.

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.

 0--- a/src/bitcoin-cli.cpp
 1+++ b/src/bitcoin-cli.cpp
 2@@ -532,7 +532,7 @@ static int CommandLineRPC(int argc, char *argv[])
 3         }
 4         std::unique_ptr<BaseRequestHandler> rh;
 5         std::string method;
 6-        if (gArgs.IsArgSet("-getinfo")) {
 7+        if (gArgs.GetBoolArg("-getinfo", false)) {
 8             rh.reset(new GetinfoRequestHandler());
 9@@ -543,7 +543,7 @@ static int CommandLineRPC(int argc, char *argv[])
10             args.erase(args.begin()); // Remove trailing method name from arguments vector
11         }
12         Optional<std::string> wallet_name{};
13-        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");
14+        if (!gArgs.GetArgs("-rpcwallet").empty()) wallet_name = gArgs.GetArg("-rpcwallet", "");
15         const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name);
16@@ -566,7 +566,7 @@ static int CommandLineRPC(int argc, char *argv[])
17         } else {
18-            if (gArgs.IsArgSet("-getinfo") && !gArgs.IsArgSet("-rpcwallet")) {
19+            if (gArgs.GetBoolArg("-getinfo", false) && gArgs.GetArgs("-rpcwallet").empty()) {
20                 GetWalletBalances(result); // fetch multiwallet balances and append to result
21             }
22diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py
23index 7530e7daf6..10ac82b107 100755
24--- a/test/functional/interface_bitcoin_cli.py
25+++ b/test/functional/interface_bitcoin_cli.py
26@@ -49,6 +49,10 @@ class TestBitcoinCli(BitcoinTestFramework):
27         self.log.info("Test -getinfo with arguments fails")
28         assert_raises_process_error(1, "-getinfo takes no arguments", self.nodes[0].cli('-getinfo').help)
29 
30+        self.log.info("Test -getinfo=0 and -nogetinfo fail")
31+        for command in ['-getinfo=0', '-nogetinfo']:
32+            assert_raises_process_error(1, "error: too few parameters (need at least command)", self.nodes[0].cli(command).send_cli)
33+
34         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

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.

0    for (const UniValue& wallet : wallets.getValues()) {
1        const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", /* args=*/{}, wallet.get_str().c_str());
2        const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
3        balances.pushKV(wallet, balance);
4    }

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.

jonatack force-pushed on May 21, 2020

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

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

cli: create GetWalletBalances() to fetch multiwallet balances 9f01849a49

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

rpc: drop unused JSONRPCProcessBatchReply size arg, refactor 903b6c117f

test: add -getinfo multiwallet functional tests

and improve the existing -getinfo -rpcwallet tests.

5edad5ce5d

jonatack force-pushed on May 21, 2020

jonatack commented at 8:41 am on May 21, 2020: member

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.

 0diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 1 #include <clientversion.h>
 2+#include <optional.h>
 3 #include <rpc/client.h>
 4@@ -17,7 +18,6 @@
 5 #include <util/url.h>
 6 
 7-#include <cstring>
 8 #include <functional>
 9@@ -305,7 +305,7 @@ public:
10     }
11 };
12 
13-static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
14+static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const Optional<std::string>& rpcwallet = {})
15 {
16     std::string host;
17     // In preference order, we choose the following for the port:
18@@ -371,7 +371,7 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
19     // check if we should use a special wallet endpoint
20     std::string endpoint = "/";
21     if (rpcwallet) {
22-        char* encodedURI = evhttp_uriencode(rpcwallet, strlen(rpcwallet), false);
23+        char* encodedURI = evhttp_uriencode(rpcwallet->data(), rpcwallet->size(), false);
24         if (encodedURI) {
25             endpoint = "/wallet/" + std::string(encodedURI);
26             free(encodedURI);
27@@ -422,11 +422,11 @@ static UniValue CallRPC(BaseRequestHandler* rh, const std::string& strMethod, co
28  *
29  * [@param](/bitcoin-bitcoin/contributor/param/)[in] rh         Pointer to RequestHandler.
30  * [@param](/bitcoin-bitcoin/contributor/param/)[in] strMethod  Reference to const string method to forward to CallRPC.
31- * [@param](/bitcoin-bitcoin/contributor/param/)[in] rpcwallet  Pointer to const c-string rpcwallet arg to forward to CallRPC.
32+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] rpcwallet  Reference to const optional string wallet name to forward to CallRPC.
33  * [@returns](/bitcoin-bitcoin/contributor/returns/) the RPC response as a UniValue object.
34  * [@throws](/bitcoin-bitcoin/contributor/throws/) a CConnectionFailed std::runtime_error if connection failed or RPC server still in warmup.
35  */
36-static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const char* const rpcwallet = nullptr)
37+static UniValue ConnectAndCallRPC(BaseRequestHandler* rh, const std::string& strMethod, const std::vector<std::string>& args, const Optional<std::string>& rpcwallet = {})
38 {
39     UniValue response(UniValue::VOBJ);
40     // Execute and handle connection failures with -rpcwait.
41@@ -468,7 +468,7 @@ static void GetWalletBalances(UniValue& result)
42 
43     UniValue balances(UniValue::VOBJ);
44     for (const UniValue& wallet : wallets.getValues()) {
45-        const char* const wallet_name = wallet.get_str().c_str();
46+        const std::string wallet_name = wallet.get_str();
47         const UniValue getbalances = ConnectAndCallRPC(rh.get(), "getbalances", /* args=*/{}, wallet_name);
48         const UniValue& balance = find_value(getbalances, "result")["mine"]["trusted"];
49         balances.pushKV(wallet_name, balance);
50@@ -542,7 +542,9 @@ static int CommandLineRPC(int argc, char *argv[])
51             method = args[0];
52             args.erase(args.begin()); // Remove trailing method name from arguments vector
53         }
54-        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, gArgs.IsArgSet("-rpcwallet") ? gArgs.GetArg("-rpcwallet", "").c_str() : nullptr);
55+        Optional<std::string> wallet_name{};
56+        if (gArgs.IsArgSet("-rpcwallet")) wallet_name = gArgs.GetArg("-rpcwallet", "");
57+        const UniValue reply = ConnectAndCallRPC(rh.get(), method, args, wallet_name);

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?

jonatack commented at 1:50 pm on May 21, 2020: member

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

0  "balance": 0.00003714,
1  "balances": {
2    "": 0.00001000,
3    "Encrypted": 0.00003500,
4    "day-to-day": 0.00000120,
5    "side project": 0.00000094
6  }
7}

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.

jnewbery commented at 3:33 pm on May 21, 2020: member

utACK 5edad5ce5d3f15b694bf3fad0300c6446674b554

Thanks for being patient with my iterative reviews!

jonatack commented at 8:31 am on May 22, 2020: member

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).

instagibbs commented at 7:06 pm on May 22, 2020: member

concept ACK

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.

meshcollider added the label Utils/log/libs on May 23, 2020

meshcollider merged this on May 23, 2020

meshcollider closed this on May 23, 2020

fanquake added the label Needs release note on May 23, 2020

MarcoFalke removed the label Tests on May 23, 2020

jonatack deleted the branch on May 23, 2020

sidhujag referenced this in commit f17ae817ed on May 25, 2020

jonatack referenced this in commit 4d3c9af7fc on May 28, 2020

jonatack referenced this in commit 92cebdd74a on May 28, 2020

jonatack referenced this in commit fb72200e01 on May 28, 2020

jonatack referenced this in commit b782daafee on May 28, 2020

jonatack referenced this in commit 3f194ae2a4 on May 28, 2020

jonatack referenced this in commit 5af36ba875 on May 28, 2020

jonatack commented at 12:03 pm on May 28, 2020: member

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

Thanks – done in #19089 and #19354.

luke-jr referenced this in commit c00c055ae8 on Jun 9, 2020

luke-jr referenced this in commit 79ab751e40 on Jun 9, 2020

luke-jr referenced this in commit 0455407e48 on Jun 9, 2020

luke-jr referenced this in commit c46c5c1c97 on Jun 9, 2020

luke-jr referenced this in commit 5d58a18287 on Jun 9, 2020

luke-jr referenced this in commit b1e3a87abd on Jun 13, 2020

luke-jr referenced this in commit 2c1d0933b0 on Jun 13, 2020

luke-jr referenced this in commit b51bd52d29 on Jun 13, 2020

luke-jr referenced this in commit 4fb54cc38f on Jun 13, 2020

luke-jr referenced this in commit d080d1c762 on Jun 13, 2020

luke-jr referenced this in commit 9940129cdd on Jun 14, 2020

luke-jr referenced this in commit d6439cc938 on Jun 14, 2020

luke-jr referenced this in commit 3f491eb3e4 on Jun 14, 2020

luke-jr referenced this in commit 9d20020c77 on Jun 14, 2020

luke-jr referenced this in commit 57f92a81dd on Jun 14, 2020

MarcoFalke removed the label Needs release note on Jun 27, 2020

MarcoFalke referenced this in commit d342a45ca7 on Jun 27, 2020

ryanofsky referenced this in commit b65072798c on Sep 28, 2020

Fabcien referenced this in commit 61bb2abbe0 on Feb 18, 2021

Fabcien referenced this in commit 9aa48e14e2 on Feb 18, 2021

Fabcien referenced this in commit 01f2e8d875 on Feb 18, 2021

Fabcien referenced this in commit 984a96569b on Feb 18, 2021

Fabcien referenced this in commit b5e2564186 on Feb 18, 2021

deadalnix referenced this in commit 08c813ab96 on Feb 18, 2021

ryanofsky referenced this in commit dcbbb75e2c on Apr 12, 2021

ryanofsky referenced this in commit 4223415ff6 on Apr 12, 2021

ryanofsky referenced this in commit fba5a00126 on Apr 12, 2021

ryanofsky referenced this in commit 6b8276e8f5 on Apr 12, 2021

ryanofsky referenced this in commit 848b05e776 on Jun 16, 2021

ryanofsky referenced this in commit 048a4ee606 on Jun 16, 2021

PastaPastaPasta referenced this in commit 688eab9354 on Jun 27, 2021

PastaPastaPasta referenced this in commit 318011721f on Jun 28, 2021

PastaPastaPasta referenced this in commit c46412ae2c on Jun 29, 2021

PastaPastaPasta referenced this in commit 0175e1e613 on Jul 1, 2021

PastaPastaPasta referenced this in commit cece4977cd on Jul 1, 2021

PastaPastaPasta referenced this in commit da41cd4a77 on Jul 15, 2021

ryanofsky referenced this in commit c90d00e875 on Dec 30, 2021

ryanofsky referenced this in commit 98ebf0b32a on Dec 30, 2021

DrahtBot locked this on Feb 15, 2022

cli: display multiwallet balances in -getinfo #18594

Conflicts