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

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

$ bitcoin-cli -getinfo -regtest
{
  "version": 199900,
  "blocks": 15599,
  "headers": 15599,
  "verificationprogress": 1,
  "timeoffset": 0,
  "connections": 0,
  "proxy": "",
  "difficulty": 4.656542373906925e-10,
  "chain": "regtest",
  "keypoolsize": 1000,
  "paytxfee": 0.00000000,
  "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
  },
  "relayfee": 0.00001000
}

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.

Concept ACK

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

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.

Concept ACK.

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

Good idea, will do.

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.

Something like this? (getbalances version)

$ bitcoin-cli -getinfo -regtest
{
  "version": 199900,
  "blocks": 15599,
  "headers": 15599,
  "verificationprogress": 1,
  "timeoffset": 0,
  "connections": 0,
  "proxy": "",
  "difficulty": 4.656542373906925e-10,
  "chain": "regtest",
  "balances": [
    {
      "": {
        "mine": {
          "trusted": 0.00001000,
          "untrusted_pending": 0.00000000,
          "immature": 0.00000000
        }
      }
    },
    {
      "encrypted": {
        "mine": {
          "trusted": 0.00003500,
          "untrusted_pending": 0.00000000,
          "immature": 0.00000000
        }
      }
    },
  ],
  "relayfee": 0.00001000
}

or like? (mine.trusted balances only)

$ bitcoin-cli -getinfo -regtest
{
  "version": 199900,
  "blocks": 15599,
  "headers": 15599,
  "verificationprogress": 1,
  "timeoffset": 0,
  "connections": 0,
  "proxy": "",
  "difficulty": 4.656542373906925e-10,
  "chain": "regtest",
  "balances": [
    {
      "name": "",
      "amount": 0.00001000
    },
    {
      "name": "encrypted",
      "amount": 0.00003500
    }
  ],
  "relayfee": 0.00001000
}

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

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

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

Perfect -- thanks :+1:

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

Why not iterate on the cli?

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.

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.

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

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

Yes. I was debating internally about "listbalances"...

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.

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.

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

• #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.

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.

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:

bitcoin-cli -allwallets getbalances
Output:
[
  {"name": "mywallet",
  {"output": 
		{
		  "mine": {
		    "trusted": 0.00000000,
		    "untrusted_pending": 0.00000000,
		    "immature": 0.00000000
		  }
		}
	}
,
  {"name": "encryptedwallet",
  {"output": 
		{
		  "mine": {
		    "trusted": 1.00000000,
		    "untrusted_pending": 0.00000000,
		    "immature": 0.00000000
		  }
		}
	}
,
...
]

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.

Updated the PR name and description.

Concept ACK

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

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.

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

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.

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

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

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.

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.

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

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.

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.

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

Rebased on master for the CI fix.

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

<!--cf906140f33d8803c4a75a2196329ecb-->

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

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.