Allow simple multiwallet rpc calls #18734

jonasschnelli · 2020-04-22T14:05:46Z

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

promag

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.

promag · 2020-04-22T14:14:06Z

src/wallet/rpcwallet.cpp

@@ -355,6 +411,10 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet

 static UniValue sendtoaddress(const JSONRPCRequest& request)
 {
+    if (IsMultiwalletJSONRPCRequest(request)) {
+        return ExecForeachWalletJSONRPCRequest(request, &sendtoaddress);


Do we want to incur in double payments?

maflcko · 2020-04-22T14:40:47Z

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

jonasschnelli · 2020-04-22T14:46:08Z

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.

maflcko · 2020-04-22T14:51:32Z

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 · 2020-04-22T15:21:32Z

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 · 2020-04-22T19:45:10Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} #18946 (rpcwallet: Replace boost::optional::emplace with simple assignment of T{} by MarcoFalke)
rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
rpc: remove deprecated CRPCCommand constructor #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 · 2020-04-22T20:44:41Z

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

👍

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

👍

luke-jr · 2020-04-24T21:21:38Z

src/wallet/rpcdump.cpp

+    if (IsMultiwalletJSONRPCRequest(request)) {
+        return ExecForeachWalletJSONRPCRequest(request, &importprivkey);
+    }


Can we avoid adding this to every wallet function?

luke-jr · 2020-04-24T21:23:04Z

src/wallet/rpcdump.cpp

+    if (IsMultiwalletJSONRPCRequest(request)) {
+        return ExecForeachWalletJSONRPCRequest(request, &dumpprivkey);
+    }


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

luke-jr · 2020-04-24T21:26:06Z

Any reason not to return

{
    "walletname": result,
    ...
}
```?

jonatack

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.

jonatack · 2020-05-26T10:58:28Z

src/wallet/rpcwallet.cpp

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


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

fanquake · 2021-06-09T12:52:57Z

Going to close this as "Up for Grabs" for now.

Allow simple multiwallet rpc calls

57b449d

jonasschnelli added Wallet RPC/REST/ZMQ labels Apr 22, 2020

promag reviewed Apr 22, 2020

View reviewed changes

DrahtBot mentioned this pull request Apr 22, 2020

rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654

Merged

DrahtBot mentioned this pull request Apr 22, 2020

rpc: replace raw pointers with shared_ptrs #18592

Closed

luke-jr reviewed Apr 24, 2020

View reviewed changes

This was referenced Apr 28, 2020

Use shared pointers only in validation interface #18354

Closed

rpc: remove deprecated CRPCCommand constructor #18531

Merged

DrahtBot mentioned this pull request May 11, 2020

rpcwallet: Replace boost::optional<T>::emplace with simple assignment of T{} #18946

Merged

jonatack mentioned this pull request May 26, 2020

Newsletters: add 99 (2020-05-27) bitcoinops/bitcoinops.github.io#405

Merged

3 tasks

jonatack reviewed May 26, 2020

View reviewed changes

fanquake closed this Jun 9, 2021

fanquake added the Up for grabs label Jun 9, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow simple multiwallet rpc calls #18734

Allow simple multiwallet rpc calls #18734

Uh oh!

jonasschnelli commented Apr 22, 2020

Uh oh!

promag left a comment

Uh oh!

promag Apr 22, 2020

Uh oh!

maflcko commented Apr 22, 2020

Uh oh!

jonasschnelli commented Apr 22, 2020

Uh oh!

maflcko commented Apr 22, 2020

Uh oh!

jonasschnelli commented Apr 22, 2020 •

edited

Loading

Uh oh!

DrahtBot commented Apr 22, 2020 •

edited

Loading

Uh oh!

jb55 commented Apr 22, 2020

Uh oh!

luke-jr Apr 24, 2020

Uh oh!

luke-jr Apr 24, 2020

Uh oh!

luke-jr commented Apr 24, 2020

Uh oh!

jonatack left a comment

Uh oh!

jonatack May 26, 2020

Uh oh!

fanquake commented Jun 9, 2021

Uh oh!

Uh oh!

Allow simple multiwallet rpc calls #18734

Allow simple multiwallet rpc calls #18734

Uh oh!

Conversation

jonasschnelli commented Apr 22, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 22, 2020

Uh oh!

jonasschnelli commented Apr 22, 2020

Uh oh!

maflcko commented Apr 22, 2020

Uh oh!

jonasschnelli commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jb55 commented Apr 22, 2020

Uh oh!

luke-jr Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr Apr 24, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Apr 24, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack May 26, 2020

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jun 9, 2021

Uh oh!

Uh oh!

jonasschnelli commented Apr 22, 2020 •

edited

Loading

DrahtBot commented Apr 22, 2020 •

edited

Loading