Fix misleading "Method not found" multiwallet errors

ryanofsky commented at 8:29 am on July 26, 2017: member

Raise RPC_WALLET_NOT_SPECIFIED instead of RPC_METHOD_NOT_FOUND when a required wallet filename was not specified in an RPC call.

Also raise more specific RPC_WALLET_NOT_FOUND error instead of RPC_INVALID_PARAMETER in case an invalid wallet was specified, for consistency.

Raise RPC_WALLET_NOT_SPECIFIED instead of RPC_METHOD_NOT_FOUND when a required
wallet filename was not specified in an RPC call.

Also raise more specific RPC_WALLET_NOT_FOUND error instead of
RPC_INVALID_PARAMETER in case an invalid wallet was specified, for consistency.

e526b3d34c

fanquake added the label RPC/REST/ZMQ on Jul 26, 2017

fanquake added the label Wallet on Jul 26, 2017

ryanofsky commented at 8:32 am on July 26, 2017: member

Should tag for 0.15 since this fixes a new error and wouldn’t be backwards compatible if it was in a later release.

laanwj added this to the milestone 0.15.0 on Jul 26, 2017

sipa commented at 9:17 am on July 26, 2017: member

utACK 5bb4fe6b3f7c8ced61902615edee708667cbae33

in src/wallet/rpcwallet.cpp:64 in 5bb4fe6b3f outdated

66-    return true;
67+    if (pwallet) return true;
68+    if (avoidException) return false;
69+    if (::vpwallets.empty()) {
70+        // Wallet RPC methods are disabled if no wallets are loaded.
71+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");

jnewbery commented at 9:27 am on July 26, 2017:

Any chance of changing the error message to Method not found (wallet disabled)?

laanwj commented at 9:48 am on July 26, 2017:

Or “no wallets loaded”, I think that’s more relevant now, especially when runtime loading/unloading is added later

ryanofsky commented at 1:42 pm on July 26, 2017:

Thread #10931 (review)

Ok, added in 4cfa4442aa53d9c86d1596e05dcd1608d378120f

jnewbery commented at 1:39 am on July 27, 2017:

Actually it appears that it’s not currently possible to hit this error. If bitcoind is started with disablewallet then the wallet RPC commands aren’t registered and any call to a wallet RPC will be rejected here: https://github.com/bitcoin/bitcoin/blob/5c8eb7916de7030d6610df53977b3068342c8f24/src/rpc/server.cpp#L495. I don’t think there’s currently a way to start bitcoind with the wallet component loaded but no wallets loaded.

Code looks good and will be required when dynamic wallet loading is implemented, but is not currently testable. Suggest you leave the code in, but add a comment.

ryanofsky commented at 2:19 pm on July 27, 2017:

Thread #10931 (review)

Interesting, added comment in 0ed0352a643af237a6ce59bf74ca9edfadbb6331.

jnewbery commented at 9:27 am on July 26, 2017: member

Definite concept ACK, and agree that this should go into v0.15 as a bugfix.

I’ll test tomorrow.

in src/wallet/rpcwallet.cpp:66 in 5bb4fe6b3f outdated

68+    if (avoidException) return false;
69+    if (::vpwallets.empty()) {
70+        // Wallet RPC methods are disabled if no wallets are loaded.
71+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");
72+    }
73+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "

jonasschnelli commented at 9:44 am on July 26, 2017:

Not sure if we should mention a specific RPC client (bitcoin-cli) on the server side.

laanwj commented at 9:52 am on July 26, 2017:

Agree with @jonasschnelli, we shouldn’t mention bitcoin-cli in server errors. Nothing prevents you from catching this specific error code in the client and giving a specific error though, if you want to be user-friendly.

ryanofsky commented at 12:59 pm on July 26, 2017:

Thread #10931 (review)

Unclear why this would be an improvement, and how the server mentioning a bitcoin-cli option is different than server providing bitcoin-cli command lines in the help text of all rpc methods (which seems like a good thing to me). Generating the text in one place instead of two places means it can be more coherent. And maybe there is some maintainability argument on the side of having more ambiguous or fragmented text, but in this case adding code to bitcoin-cli to handle wallet not specified doesn’t seem like a clear win for maintainability, either.

jnewbery commented at 1:46 am on July 27, 2017:

I almost wrote the same comment as Jonas about server not having bitcoin-cli specific messages, but Russ has convinced me. I think it’s ok to have a bitcoin-cli message here.

laanwj commented at 9:35 am on July 27, 2017:

NACK. I still disagree. People use RPC from many other programs than just bitcoin-cli. Mentioning a -cli argument is just confusing. If you want to add documentation in an error message, you should document how people should change their API usage instead.

how the server mentioning a bitcoin-cli option is different than server providing bitcoin-cli command lines in the help text of all rpc methods

That’s IMO different because it’s a clearly isolated context that mentions use of bitcoin-cli, not a random error.

ryanofsky commented at 2:22 pm on July 27, 2017:

Thread continues here: #10931 (review)

jonasschnelli commented at 9:44 am on July 26, 2017: contributor

utACK 5bb4fe6b3f7c8ced61902615edee708667cbae33

Change wallet method disabled error text

Not strictly backwards compatible because the error is not new in this release.

df389bca20

ryanofsky commented at 1:49 pm on July 26, 2017: member

Added a commit 5bb4fe6b3f7c8ced61902615edee708667cbae33 -> 4cfa4442aa53d9c86d1596e05dcd1608d378120f (pr/multierr.1 -> pr/multierr.2, compare)

gmaxwell approved

gmaxwell commented at 3:15 pm on July 26, 2017: contributor

utACK

achow101 commented at 6:46 pm on July 26, 2017: member

utACK 4cfa4442aa53d9c86d1596e05dcd1608d378120f

jnewbery commented at 1:47 am on July 27, 2017: member

Tested ACK 4cfa4442aa53d9c86d1596e05dcd1608d378120f

in src/wallet/rpcwallet.cpp:67 in 4cfa4442aa outdated

69+    if (::vpwallets.empty()) {
70+        // Wallet RPC methods are disabled if no wallets are loaded.
71+        throw JSONRPCError(
72+            RPC_METHOD_NOT_FOUND, "Method not found (wallet method is disabled because no wallet is loaded)");
73+    }
74+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "

laanwj commented at 9:37 am on July 27, 2017:

Somehow this got hidden: #10931 (review)

I still disagree with mentioning -cli here. If there is to be extenxsive documentation here, this should mention how to change the API usage, as people will be using this API from many applications and not just bitcoin-cli.

jonasschnelli commented at 10:08 am on July 27, 2017:

Agree with @laanwj. Please remove the bitcoin-cli part. By design, it makes no sense to report client specific error message in a pure JSON RPC server implementation.

ryanofsky commented at 2:23 pm on July 27, 2017:

Thread #10931 (review)

Ok @laanwj, thanks for providing an explanation. I implemented your suggestion in 6b5c343a49a25085bf1d6300cea2d40cebc993b6. I think we’re basically talking about a tradeoff between more useful vs more maintainable help text, though differences ares very small in this case.

laanwj commented at 3:50 pm on July 27, 2017:

In a way, yes, though I don’t think this is much less maintainable; we have those error codes for a reason, might as well use them to print custom text at the client side. We could at some point provide a better help text for other codes as well, where it makes sense. Anoher advantage of generating messages at the client side is that they could be translated (though we currently don’t support translation in any way in non-qt). In any case thanks for implementing this.

ryanofsky force-pushed on Jul 27, 2017

ryanofsky commented at 3:27 pm on July 27, 2017: member

Added 2 commits 4cfa4442aa53d9c86d1596e05dcd1608d378120f -> 6b5c343a49a25085bf1d6300cea2d40cebc993b6 (pr/multierr.2 -> pr/multierr.3, compare)

Squashed 6b5c343a49a25085bf1d6300cea2d40cebc993b6 -> df389bca2048f5814387987463fe3298766bc6b6 (pr/multierr.3 -> pr/multierr.4)

laanwj commented at 3:53 pm on July 27, 2017: member

utACK df389bc

laanwj merged this on Jul 27, 2017

laanwj closed this on Jul 27, 2017

laanwj referenced this in commit 0b11a07848 on Jul 27, 2017

PastaPastaPasta referenced this in commit d717b92d64 on Aug 6, 2019

PastaPastaPasta referenced this in commit 8466085462 on Aug 6, 2019

PastaPastaPasta referenced this in commit acbe071f32 on Aug 6, 2019

PastaPastaPasta referenced this in commit c128c77af6 on Aug 7, 2019

PastaPastaPasta referenced this in commit a8eea6f633 on Aug 8, 2019

PastaPastaPasta referenced this in commit 417d95f4ea on Aug 12, 2019

barrystyle referenced this in commit 0a8921c361 on Jan 22, 2020

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

MarcoFalke locked this on Sep 8, 2021

Fix misleading “Method not found” multiwallet errors #10931