Allow simple multiwallet rpc calls #18734

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2020/03/multiwallet_rpc changing 3 files +223 −1

jonasschnelli commented at 2:05 PM on April 22, 2020: contributor
Would fix #18715. See discussion in #18453.

Currently, batching RPC requests across wallets does not work due to the fact that each wallet has its own endpoint.

This PR allows using a simple wildcard approach for the wallet endpoint (/wallet/*/). Using bitcoin-cli -rpcwallet="*" or -rpcwallet="pattern* works with this PR.

This PR is a simple approach that allows detailed control on a per call basis. Some calls probably need special logic (rescanblockchain). Some calls make not much sense across wallets (dumpwallet or backupwallet as example).

The output (or error/s) will be bundles as json array with a dictionary for each wallet:
```
[
  {
    "walletname": "test",
    "result": 0.00000000
  },
  {
    "walletname": "wallet",
    "result": 0.00000000
  },
  {
    "walletname": "test2",
    "result": 0.00000000
  }
]
```
Alternatives:
- implementing a generic approach on our httpserver/rpcserver layer (complicates layering)
- try to add it to the CRPCTable (overhead for non wallet calls)
- a dedicated "foreachwallet" RPC meta call
Todo:
- tests
- documentation
Allow simple multiwallet rpc calls 57b449dc25
jonasschnelli added the label Wallet on Apr 22, 2020
jonasschnelli added the label RPC/REST/ZMQ on Apr 22, 2020

in src/wallet/rpcwallet.cpp:415 in 57b449dc25

 410 | @@ -355,6 +411,10 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet
 411 |  
 412 |  static UniValue sendtoaddress(const JSONRPCRequest& request)
 413 |  {
 414 | +    if (IsMultiwalletJSONRPCRequest(request)) {
 415 | +        return ExecForeachWalletJSONRPCRequest(request, &sendtoaddress);

promag commented at 2:14 PM on April 22, 2020:

Do we want to incur in double payments?

promag commented at 2:18 PM on April 22, 2020: member

I still don't get what are the use cases beside the obvious getbalances. Not sure about mutation calls, like send.

Does it make sense to have one unified view in qt instead? That view could show all loaded wallets + their balances, flags etc.
MarcoFalke commented at 2:40 PM on April 22, 2020: member

I think @promag has a good point. Apart from get*, list* and keypoolrefill there is not really a use case for this endpoint.
jonasschnelli commented at 2:46 PM on April 22, 2020: contributor

I think @promag has a good point. Apart from get, list and keypoolrefill there is not really a use case for this endpoint.

Fair point. I think this is why I would favour this approach which can be implemented per call. I guess there are some edge cases for other calls like set* (like settxfee), import*. But even only supporting the list* and get* calls,... it would be around half of the wallet calls.
MarcoFalke commented at 2:51 PM on April 22, 2020: member

Instead of the not-quite-regex-but-still-a-pattern I'd prefer the caller to explicitly list the wallets. I think requiring that the user calls listtransactions once to get the wallet names is not too much to ask.

Also, the returned array could be a json dict with keys being the wallet name. That is easier to parse at the client.
jonasschnelli commented at 3:21 PM on April 22, 2020: contributor

Instead of the not-quite-regex-but-still-a-pattern I'd prefer the caller to explicitly list the wallets. I think requiring that the user calls listtransactions once to get the wallet names is not too much to ask.

Probably meh to not support a pure "*" (all wallets). Maybe "*" or a cs-list of wallets?

Also, the returned array could be a json dict with keys being the wallet name. That is easier to parse at the client.

Good point. A downside could be the identifiability of an error (you already don't get the http response code).
DrahtBot commented at 7:45 PM on April 22, 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:
- #18946 (rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} by MarcoFalke)
- #18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
- #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
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.
jb55 commented at 8:44 PM on April 22, 2020: member

@promag:

I still don't get what are the use cases beside the obvious getbalances.

listing transactions, coins. This what I currently do in scripts like btc-coins, but the round-trips are a bit slow with many wallets

Not sure about mutation calls, like send.

good point, that seems like a footgun. seems like this is more useful for not-mutating calls

Does it make sense to have one unified view in qt instead? That view could show all loaded wallets + their balances, flags etc.

I think that would be useful, but runs into similar issues when creating transactions where you probably want to know what wallet you're creating from. So maybe it would make sense to limit it to balances and maybe transactions in a unified qt view? The logic for this could be tricky. @MarcoFalke:

Instead of the not-quite-regex-but-still-a-pattern I'd prefer the caller to explicitly list the wallets. I think requiring that the user calls listtransactions once to get the wallet names is not too much to ask.

:+1:

Also, the returned array could be a json dict with keys being the wallet name. That is easier to parse at the client.

:+1:

in src/wallet/rpcdump.cpp:97 in 57b449dc25

  91 | @@ -92,6 +92,10 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
  92 |  
  93 |  UniValue importprivkey(const JSONRPCRequest& request)
  94 |  {
  95 | +    if (IsMultiwalletJSONRPCRequest(request)) {
  96 | +        return ExecForeachWalletJSONRPCRequest(request, &importprivkey);
  97 | +    }

luke-jr commented at 9:21 PM on April 24, 2020:

Can we avoid adding this to every wallet function?

in src/wallet/rpcdump.cpp:705 in 57b449dc25

 699 | @@ -676,6 +700,10 @@ UniValue importwallet(const JSONRPCRequest& request)
 700 |  
 701 |  UniValue dumpprivkey(const JSONRPCRequest& request)
 702 |  {
 703 | +    if (IsMultiwalletJSONRPCRequest(request)) {
 704 | +        return ExecForeachWalletJSONRPCRequest(request, &dumpprivkey);
 705 | +    }

luke-jr commented at 9:23 PM on April 24, 2020:

Something like dumpprivkey should probably just return the first result it gets (and ignore all the failures)?

luke-jr commented at 9:26 PM on April 24, 2020: member
Any reason not to return
```
{
    "walletname": result,
    ...
}
```?
```

in src/wallet/rpcwallet.cpp:120 in 57b449dc25

 115 | +}
 116 | +
 117 | +UniValue ExecForeachWalletJSONRPCRequest(JSONRPCRequest mutable_request, const rpcfn_type& func) {
 118 | +    std::string search_string = URL_DECODE(mutable_request.URI.substr(WALLET_ENDPOINT_BASE.size()));
 119 | +    const size_t pos = search_string.find('*');
 120 | +    assert(pos != std::string::npos); // we must have checked earlier for an asterisk

jonatack commented at 10:58 AM on May 26, 2020:

Needed to appease test/lint/lint-assertions.sh

-    assert(pos != std::string::npos); // we must have checked earlier for an asterisk
+    CHECK_NONFATAL(pos != std::string::npos); // we must have checked earlier for an asterisk

jonatack commented at 11:20 AM on May 26, 2020: member
Tested Concept ACK 57b449d rebased on current master
- I really like the rpcwallet=* wildcard feature; it would be my default use as a human user.
- The ability to specify multiple wallets with rpcwallet= would be good also, especially for RPC clients/scripts, as a logical follow-up.
- Agree with the idea of initially adding this only for get, list, and maybe keypoolrefill calls.
- Agree with the suggestions to try returning this as a JSON dict of objects with wallet names as keys.
fanquake commented at 12:52 PM on June 9, 2021: member

Going to close this as "Up for Grabs" for now.
fanquake closed this on Jun 9, 2021
fanquake added the label Up for grabs on Jun 9, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Wallet RPC/REST/ZMQ Up for grabs

Linked (view graph)

#18453 rpc, cli: add multiwallet balances rpc and use it in -getinfo #18715 Allow JSON RPC "batches" for multiwallet

Allow simple multiwallet rpc calls #18734

Conflicts