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 f73ace0

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 👍

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 a5ea571

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.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• test: add coverage for -rpcwallet cli option #18724 (test: add coverage for -rpcwallet cli option by jonatack)
• cli: display multiwallet balances in -getinfo #18594 (cli: display multiwallet balances in -getinfo by jonatack)
• [RPC] Include coinbase transactions in receivedby RPCs #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.

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.

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

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