rpc, cli: add multiwallet balances rpc and use it in -getinfo

jonatack commented at 11:23 pm on March 27, 2020: member

This PR implements point 2 from #17314 (show balance per wallet):

create an RPC that returns the ismine.trusted balance for all loaded wallets
update bitcoin-cli -getinfo to use the RPC

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  "keypoolsize": 1000,
12  "paytxfee": 0.00000000,
13  "balance": 0.00001000,
14  "relayfee": 0.00001000
15}

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  "relayfee": 0.00001000
18}

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

This approach is simpler, adds no additional rpc calls in -getinfo and runs faster than doing it on the client side inside bitcoin-cli.cpp, but it also adds the first multiwallet RPC and would need to be well-considered in terms of the API.

fanquake added the label Utils/log/libs on Mar 27, 2020

laanwj commented at 10:03 am on March 28, 2020: member

Concept ACK

jonasschnelli commented at 10:09 am on March 28, 2020: contributor

Adds one more call to getinfo. Under the hood, it loads the same structures GetBalance().m_mine_trusted. Not efficient, but since performance is acceptable, I think it’s worth the change to prevent from future issues and allows stripping down getwalletinfo

utACK f73ace05f2e67917e12026f0c69aec0e5b043366

laanwj commented at 11:05 am on March 28, 2020: member

What would make me really happy, when you’re making changes here anyway, is implementing point 2 from #17314: show balance per wallet. This is much more useful in the multi-wallet case.

promag commented at 11:14 am on March 28, 2020: member

Concept ACK.

Agree in making @laanwj happy. The change requires a listwallets call before the batch.

jonatack commented at 11:14 am on March 28, 2020: member

Good idea, will do.

laanwj commented at 11:19 am on March 28, 2020: member

The change requires a listwallets call before the batch

~~I don’t think that’s necessary, getbalances will already retrieve the balances for all currently active wallets, which is what I mean, It doesn’t need to list inactive ones in the datadir IMO.~~ Oh this is not true, I’m confused apparently.

jonatack commented at 9:27 pm on March 28, 2020: member

Something like this? (getbalances version)

 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    {
13      "": {
14        "mine": {
15          "trusted": 0.00001000,
16          "untrusted_pending": 0.00000000,
17          "immature": 0.00000000
18        }
19      }
20    },
21    {
22      "encrypted": {
23        "mine": {
24          "trusted": 0.00003500,
25          "untrusted_pending": 0.00000000,
26          "immature": 0.00000000
27        }
28      }
29    },
30  ],
31  "relayfee": 0.00001000
32}

or like? (mine.trusted balances only)

 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    {
13      "name": "",
14      "amount": 0.00001000
15    },
16    {
17      "name": "encrypted",
18      "amount": 0.00003500
19    }
20  ],
21  "relayfee": 0.00001000
22}

laanwj commented at 11:05 pm on March 28, 2020: member

It’s best if -getinfo fits on a single screen, so I’d prefer a really condensed form, for ex.

 0{
 1  "version": 199900,
 2  "blocks": 15599,
 3  "headers": 15599,
 4  "verificationprogress": 1,
 5  "timeoffset": 0,
 6  "connections": 0,
 7  "proxy": "",
 8  "difficulty": 4.656542373906925e-10,
 9  "chain": "regtest",
10  "balances": {
11    "": 0.00001000,
12    "encrypted": 0.00003500,
13  },
14  "relayfee": 0.00001000
15}

jonatack commented at 11:24 pm on March 28, 2020: member

It’s best if -getinfo fits on a single screen, so I’d prefer a really condensed form

Perfect – thanks :+1:

jonatack force-pushed on Mar 29, 2020

jonatack renamed this:
~~cli: enable -getinfo to fetch wallet balance from getbalances()~~
cli -getinfo: enable multiwallet balances and no longer depend on getwalletinfo balance
on Mar 29, 2020

jonatack force-pushed on Mar 29, 2020

jonatack commented at 1:37 am on March 29, 2020: member

Done. If this approach is ok, I’ll add tests for the new call.

promag commented at 1:41 am on March 29, 2020: member

Why not iterate on the cli?

jonatack commented at 1:54 am on March 29, 2020: member

I could be wrong, but this seemed cleaner, and I think we’ll ultimately want either an rpc like this or allow passing a multi bool to getbalance or getbalances to do the same.

jonatack force-pushed on Mar 29, 2020

promag commented at 2:49 pm on March 29, 2020: member

Not sure about the approach TBH. I’d rather see the cli use existing calls.

The new RPC getwalletbalances is the first to use multiple wallets and could lead to more of multiwallet RPC. For instance, getaddressesbylabel could return all address of all loaded wallets.

jonatack force-pushed on Mar 29, 2020

jonatack commented at 3:10 pm on March 29, 2020: member

This works, but I understand. Interested in feedback. Note that for now getwalletbalances doesn’t show up in the help. Rebased to add tests for getwalletbalances in the wallet_multiwallet tests (which also improves the existing tests).

sipa commented at 11:15 pm on April 1, 2020: member

I’m not opposed to having “across-all-wallets” RPCs, but perhaps it’s worth making them visually distinct? How about “getallwalletbalances” or “allwalletsgetbalance”?

jonatack commented at 11:23 pm on April 1, 2020: member

Yes. I was debating internally about “listbalances”…

fjahr commented at 5:36 pm on April 3, 2020: member

ACK a5ea5717400d8f31ba210df5705eb6c1470da307

Reviewed code and did some light testing locally.

I think multi wallet RPCs will be useful for a lot of users. For naming I don’t have strong feelings but since “multi wallet” is already the term most frequently used for this scenario verbally, I think I would prefer if that gets used if people think the distinction is necessary. So I would prefer something like “(get)multiwalletbalances” to something like listbalances or something with allwallets.

in src/bitcoin-cli.cpp:243 in a5ea571740 outdated

239@@ -239,16 +240,17 @@ class GetinfoRequestHandler: public BaseRequestHandler
240         result.push_back(JSONRPCRequestObj("getnetworkinfo", NullUniValue, ID_NETWORKINFO));
241         result.push_back(JSONRPCRequestObj("getblockchaininfo", NullUniValue, ID_BLOCKCHAININFO));
242         result.push_back(JSONRPCRequestObj("getwalletinfo", NullUniValue, ID_WALLETINFO));
243+        result.push_back(JSONRPCRequestObj("getwalletbalances", NullUniValue, ID_BALANCESINFO));

robot-visions commented at 10:41 pm on April 4, 2020:

Super nit (please feel free to ignore): Would it make sense to use ID_WALLETBALANCES in place of ID_BALANCESINFO to preserve the getXXX -> ID_XXX naming scheme?

jonatack commented at 4:26 pm on April 7, 2020:

Thanks for the feedback. The naming of getwalletbalances may change but will keep this in mind.

jonatack commented at 5:00 pm on April 9, 2020:

Thanks – done in #18574 (please review :))

in test/functional/wallet_multiwallet.py:179 in a5ea571740 outdated

175@@ -173,17 +176,34 @@ def wallet_file(name):
176 
177         w1, w2, w3, w4, *_ = wallets
178         node.generatetoaddress(nblocks=101, address=w1.getnewaddress())
179-        assert_equal(w1.getbalance(), 100)
180-        assert_equal(w2.getbalance(), 0)
181-        assert_equal(w3.getbalance(), 0)
182+        wallet_name = sorted(wallet_names)[1]

robot-visions commented at 10:42 pm on April 4, 2020:

Super nit (please feel free to ignore): Would it make sense to define a variable like extern_wallet next to where wallet_names is defined, instead of relying on the sort order of the path names?

jonatack commented at 4:28 pm on April 7, 2020:

Not a bad idea; will look at this if I have to retouch the PR.

jonatack commented at 6:50 pm on April 16, 2020:

Done in the latest rebase; it’s much better with your suggestion.

Mind re-ACKing here and/or #18594 (the client-side version)?

robot-visions commented at 10:43 pm on April 4, 2020: contributor

ACK. Code change looks great.

Local testing:

Unit tests passed
Functional tests passed (interface_bitcoin_cli.py and wallet_multiwallet.py)
Manual smoke test passed (created multiple wallets, generated coins, made sure bitcoin-cli -regtest -getinfo and bitcoin-cli -regtest getwalletbalances behaved as expected)

Thanks for hosting the next PR Review Club! During the session, I’d also be curious to learn when it’s appropriate to add new RPCs to the bitcoin-cli documentation.

DrahtBot commented at 8:03 pm on April 5, 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:

#18724 (test: add coverage for -rpcwallet cli option by jonatack)
#18594 (cli: display multiwallet balances in -getinfo by jonatack)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

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.

in src/bitcoin-cli.cpp:251 in a5ea571740 outdated

249     {
250         UniValue result(UniValue::VOBJ);
251-        std::vector<UniValue> batch = JSONRPCProcessBatchReply(batch_in, 3);
252-        // Errors in getnetworkinfo() and getblockchaininfo() are fatal, pass them on
253-        // getwalletinfo() is allowed to fail in case there is no wallet.
254+        std::vector<UniValue> batch = JSONRPCProcessBatchReply(batch_in, 4);

vasild commented at 3:55 pm on April 7, 2020:

Maybe use batch_in.size() instead of 4?

jonatack commented at 9:12 am on April 9, 2020:

Thanks – done in #18574 (please ack :).

vasild commented at 9:38 am on April 9, 2020:

Good idea to split the PR in two.

in src/wallet/rpcwallet.cpp:717 in a5ea571740 outdated

726+        }
727+        CWallet& wallet = *rpc_wallet;
728+        wallet.BlockUntilSyncedToCurrentChain();
729+
730+        auto locked_chain = wallet.chain().lock();
731+        LOCK(wallet.cs_wallet);

vasild commented at 7:10 pm on April 7, 2020:

These locks are acquired inside GetBalance(), so they can be removed from here?

A wallet’s chain will never change and, I assume, all wallets’ chains are the same. That is - we are operating on one chain here, for all wallets. Can we / should we lock that chain before the for-loop in order to get a consistent view (i.e. avoid getting one wallet’s balance as of height H and another wallet’s balance as of height H+1)?

ryanofsky commented at 5:05 pm on April 8, 2020:

A wallet’s chain will never change and, I assume, all wallets’ chains are the same. That is - we are operating on one chain here, for all wallets. Can we / should we lock that chain before the for-loop in order to get a consistent view (i.e. avoid getting one wallet’s balance as of height H and another wallet’s balance as of height H+1)?

We generally don’t want wallet code to lock up the node, and after #16426 we are removing the ability of wallet code to be able to do this. It synchronization between different wallets is a concern, a solution could be to return tip hashes alongside balances. Or to restart the wallet loop if the tip changes midway.

vasild commented at 6:54 pm on April 8, 2020:

I see. So only the first question above remains (remove the locks acquisition).

in src/wallet/rpcwallet.cpp:719 in a5ea571740 outdated

728+        wallet.BlockUntilSyncedToCurrentChain();
729+
730+        auto locked_chain = wallet.chain().lock();
731+        LOCK(wallet.cs_wallet);
732+
733+        obj.pushKV(wallet.GetName(), ValueFromAmount(wallet.GetBalance().m_mine_trusted));

vasild commented at 7:22 pm on April 7, 2020:

The GetBalance() call without arguments will default to GetBalance(min_depth=0), i.e. also counting unconfirmed transactions. Is this intended? Answer: (probably) yes - from the unconfirmed transactions, it will actually count only those from us.

wallet.GetName() produces something like "": 0.00000000 for the default wallet. Would GetDisplayName() be more suitable here? It would produce "[default wallet]": 0.00000000.

jonatack commented at 4:33 pm on April 8, 2020:

GetDisplayName() seems to be mainly for the GUI wallet. The RPC is primarily designed to be consumed by applications, whose developers would likely find GetName() more practical, which to me explains why GetName is used.

CLI -getinfo is between the two and oriented toward users, so the new RPC could maybe return both fields, e.g. Name for RPC output and DisplayName for CLI output, but I’m -1 on it due to the additional complexity not being worth it (and admittedly also used to the RPC output as-is).

ryanofsky commented at 4:58 pm on April 8, 2020:

To be consistent with other rpcs like listwallets it’s right for this to be using GetName not GetDisplayName. Other wallets have names, but the default wallet doesn’t have a name.

in src/wallet/rpcwallet.cpp:710 in a5ea571740 outdated

719+ */
720+static UniValue getwalletbalances(const JSONRPCRequest& request)
721+{
722+    UniValue obj{UniValue::VOBJ};
723+    for (const std::shared_ptr<CWallet>& rpc_wallet : GetWallets()) {
724+        if (!EnsureWalletIsAvailable(rpc_wallet.get(), request.fHelp)) {

vasild commented at 7:45 pm on April 7, 2020:

Should we be doing any of this if request.fHelp is true? It happens when bitcoin-cli help is executed even though none of its output contains getwalletbalances or any balances.

in src/wallet/rpcwallet.cpp:4324 in a5ea571740 outdated

4285@@ -4264,6 +4286,7 @@ static const CRPCCommand commands[] =
4286     { "wallet",             "encryptwallet",                    &encryptwallet,                 {"passphrase"} },
4287     { "wallet",             "getaddressesbylabel",              &getaddressesbylabel,           {"label"} },
4288     { "wallet",             "getaddressinfo",                   &getaddressinfo,                {"address"} },
4289+    { "wallet",             "getwalletbalances",                &getwalletbalances,             {} },

vasild commented at 7:49 pm on April 7, 2020:

Do you plan to document this before merge? If “no”, then are there other undocumented commands?

jonatack commented at 9:02 pm on April 7, 2020:

If the consensus is to add a public RPC, yes, it should have documentation.

vasild commented at 7:14 am on April 8, 2020:

Ok, makes sense. IMO the current approach where the iterate-over-wallets logic is inside bitcoind and exposed via RPC is better than having that logic inside bitcoin-cli because in the latter case it cannot be reused by RPC users - should they need that functionality they would have to replicate the logic inside their applications.

vasild commented at 8:12 pm on April 7, 2020: member

Approach ACK.

The modified tests test/functional/interface_bitcoin_cli.py and test/functional/wallet_multiwallet.py pass on my computer and the output of bitcoin-cli -getinfo -regtest looks as intended.

jonasschnelli added the label RPC/REST/ZMQ on Apr 8, 2020

jonasschnelli commented at 7:55 am on April 8, 2020: contributor

Concept ACK. I added the RPC label as this now adds a new PRC command (should probably be in the PR description).

I strongly agree with @sipa. Across-all-wallets calls should have a naming convention. IMO allwallet[s] should be the required string (works with get, list, execute, etc.). Once we have started adding across-all-wallets calls it will be difficult to change the concept (so its worth investing some brainpower in how that should work).

Another approach would be to add a special RPC endpoint like /allwallets that will combine the JSON output of each per-wallet-output (more a generic RPC multiwallet approach). From the users perspective that could look like:

 0bitcoin-cli -allwallets getbalances
 1Output:
 2[
 3  {"name": "mywallet",
 4  {"output": 
 5		{
 6		  "mine": {
 7		    "trusted": 0.00000000,
 8		    "untrusted_pending": 0.00000000,
 9		    "immature": 0.00000000
10		  }
11		}
12	}
13,
14  {"name": "encryptedwallet",
15  {"output": 
16		{
17		  "mine": {
18		    "trusted": 1.00000000,
19		    "untrusted_pending": 0.00000000,
20		    "immature": 0.00000000
21		  }
22		}
23	}
24,
25...
26]

I’m not convince that this is the better approach (has probably some performance and flexibility downsides), just wanted to highlight the importantness of defining the first across-all-wallets RPC.

jonatack renamed this:
~~cli -getinfo: enable multiwallet balances and no longer depend on getwalletinfo balance~~
rpc, cli: add multiwallet balances rpc, use it in -getinfo, no longer depend on getwalletinfo balance
on Apr 8, 2020

jonatack commented at 1:44 pm on April 8, 2020: member

Updated the PR name and description.

theStack commented at 4:57 pm on April 8, 2020: member

Concept ACK

ryanofsky commented at 5:25 pm on April 8, 2020: member

I strongly agree with @sipa. Across-all-wallets calls should have a naming convention. IMO allwallet[s] should be the required string (works with get, list, execute, etc.). Once we have started adding across-all-wallets calls it will be difficult to change the concept (so its worth investing some brainpower in how that should work).

Just using the word “wallets” seems pretty good as a naming convention here. And we already have a listwallets RPC method following this convention.

Another approach would be to add a special RPC endpoint like /allwallets that will combine the JSON output of each per-wallet-output (more a generic RPC multiwallet approach).

This is a neat idea. Another way to implement it without a new endpoint would be with foreachwallet forwarding method: bitcoin-cli foreachwallet getbalances

jonasschnelli commented at 6:10 pm on April 8, 2020: contributor

Just using the word “wallets” seems pretty good as a naming convention here. And we already have a listwallets RPC method following this convention.

Could that lead to non-ideal distinguishability for certain (future?) calls (as an example listwalletdir versus listwalletsdir a.s.o.)?

This is a neat idea. Another way to implement it without a new endpoint would be with foreachwallet forwarding method: bitcoin-cli foreachwallet getbalances

Nice idea to use a dedicated RPC method instead of an endpoint.

ryanofsky commented at 6:45 pm on April 8, 2020: member

Could that lead to non-ideal distinguishability for certain (future?) calls (as an example listwalletdir versus listwalletsdir a.s.o.)?

I think I’m not sure what the question is asking, but I’d rename listwalletdir to listwalletsdir (and rename -walletdir to -walletsdir, see #12221), with aliases for backwards compatibility. listwallets, listwalletsdir, and getwalletsbalances as a convention for cross-wallet calls does seems clear enough to me

jnewbery commented at 8:51 pm on April 8, 2020: member

Concept ACK, but approach NACK. I don’t think we should add cross-wallet RPC methods to the server. Currently we have the following convention:

For a multiwallet node:

node RPC methods are available on the / endpoint
wallet RPC methods are available on the /wallet/<wallet_name> endpoint (wallet admin methods like loadwallet, unloadwallet, listwallets are on the node / endpoint

For a single wallet node:

all node and RPC methods are available on the / endpoint

In future, we might want to add per-wallet permissions (#10615) or wallets in a separate process with its own RPC server (#10102). Currently, both of these things map nicely to the one-endpoint-one-wallet design we have. I think adding cross-wallet RPCs would make those things more difficult.

I think it shouldn’t be too difficult to add this functionality on the bitcoin-cli client side. That seems like a better approach to me.

promag commented at 9:11 pm on April 8, 2020: member

I also share @jnewbery opinion (https://github.com/bitcoin/bitcoin/pull/18453#issuecomment-605648343).

ryanofsky commented at 9:55 pm on April 8, 2020: member

@jnewbery, I don’t understand the distinction you’re making. What makes listwallets and unloadwallet “wallet admin” methods that are ok to have, and getwalletsbalances not a wallet admin method, or not a wallet admin method that’s ok to have?

I also don’t see this interfering in a significant way with #10102 or #10615. From perspective of #10102 at least, this isn’t different than the current listwallets method.

jnewbery commented at 11:04 pm on April 8, 2020: member

I don’t understand the distinction you’re making. What makes listwallets and unloadwallet “wallet admin” methods that are ok to have

I think it’s fine to allow the node administrator to see which wallets are loaded or to unload a wallet, but they shouldn’t be able to carry out any actions on that wallet. It would be unexpected to add wallet permissions to a wallet and then find out that a user without those permissions is able to see the balance. How would you expect this (or other cross-wallet RPC methods) to behave if some of the wallets had permissions?

I generally don’t like arguments that we shouldn’t add a new feature because it might interact badly with some theoretical future feature, but in this case I don’t see the need to add a whole new class of cross-wallet RPCs when the functionality can easily be added to the client.

jonasschnelli commented at 7:50 am on April 9, 2020: contributor

Concept ACK, but approach NACK. I don’t think we should add cross-wallet RPC methods to the server.

Thanks @jnewbery for sharing your thoughts. The advantage of having serverside across-all-wallets calls if probably only efficiency (performance, bandwidth) which might be not worth the effort. I think even with per wallet permissions serverside across-all-wallet calls would work as well (some elements may be omitted or a permission error is returned).

Our JSON RPC implementation supports batches. Though,… I guess since we are using different endpoints for the wallet selection, batching across wallets is not possible?

Therefore I think it would make sense to support some sort of batching option that allow across-wallets batches (for performance reasons). @ryanofsky foreachwallet idea seems to be a viable way. Maybe as an endpoint or a method that has sub-methods are parameter.

jonatack commented at 9:14 am on April 9, 2020: member

I’ve extracted the original PR to #18574 to preserve this one as a discussion on multiwallet RPC/CLI.

jonatack commented at 9:27 am on April 9, 2020: member

After looking at #10102 (and #10615 which is closed), I tend to agree with @ryanofsky and @jonasschnelli. Will propose multiwallet balances for -getinfo inside bitcoin-cli.cpp as a lower-scope solution for now in a separate PR, while this discussion continues in parallel for multiwallets on the server side.

MarcoFalke referenced this in commit 6ab96ec546 on Apr 10, 2020

DrahtBot added the label Needs rebase on Apr 10, 2020

jonatack renamed this:
~~rpc, cli: add multiwallet balances rpc, use it in -getinfo, no longer depend on getwalletinfo balance~~
rpc, cli: add multiwallet balances rpc and use it in -getinfo
on Apr 11, 2020

jonatack force-pushed on Apr 12, 2020

DrahtBot removed the label Needs rebase on Apr 12, 2020

jonatack commented at 4:40 pm on April 13, 2020: member

Rebased.

I implemented a client-side version of this PR at #18594 per the approach suggested by @laanwj, @promag, and @jnewbery. For now I think it’s the immediate solution, and once a server-side multiwallet RPC API is adopted, it can be used for -getinfo.

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

jonatack force-pushed on Apr 17, 2020

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

jb55 commented at 6:17 pm on April 20, 2020: member

@jonasschnelli:

Therefore I think it would make sense to support some sort of batching option that allow across-wallets batches (for performance reasons).

ACK this. Right now I have bash scripts for doing multi-wallet things like balances and coins but it’s pretty horrible and slow since I can’t batch requests. It would be so much nicer if there was a special wallet endpoint you could hit which signals that you want the result from all wallets, or at least a way to batch requests for different wallets.

rpc: create getwalletbalances rpc, initial test coverage fd5073a7c0

cli: display multiwallet balances in -getinfo (server side version) bc8ae3899d

test: add multiwallet -getinfo tests to interface_bitcoin_cli.py c414b42635

jonatack force-pushed on Apr 21, 2020

jonatack commented at 1:09 pm on April 21, 2020: member

Rebased on master for the CI fix.

luke-jr changes_requested

luke-jr commented at 0:15 am on April 23, 2020: member

Concept NACK specifically to enabling access to the wallet list (or non-selected wallets) for existing interfaces.

Up until now, RPCs would only ever access the wallet the request was sent to…

DrahtBot commented at 0:39 am on April 25, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Apr 25, 2020

jonatack commented at 3:07 pm on April 25, 2020: member

Thanks to everyone for reviewing.

Summary: 4 Concept ACKs 1 Approach ACK 1 Concept NACK 2 Approach NACKs 4 ACKs

To take the feedback into account, a client-side, smaller-scope version of this feature for -getinfo only, with Concept ACKs, ACKs, and several rounds of review, is ready now for final review at #18594. Edit: #18594 is merged, thanks everyone.

jonatack closed this on Apr 25, 2020

meshcollider referenced this in commit 24f7029064 on May 23, 2020

sidhujag referenced this in commit f17ae817ed on May 25, 2020

MarcoFalke locked this on Feb 15, 2022

rpc, cli: add multiwallet balances rpc and use it in -getinfo #18453

Conflicts