Fix misleading "Method not found" multiwallet errors #10931

ryanofsky · 2017-07-26T08:29:54Z

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.

ryanofsky · 2017-07-26T08:32:16Z

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

sipa · 2017-07-26T09:17:29Z

utACK 5bb4fe6

jnewbery

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

I'll test tomorrow.

jnewbery · 2017-07-26T09:27:24Z

src/wallet/rpcwallet.cpp

+    if (avoidException) return false;
+    if (::vpwallets.empty()) {
+        // Wallet RPC methods are disabled if no wallets are loaded.
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");


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

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

Thread #10931 (comment)

Ok, added in 4cfa444

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:

bitcoin/src/rpc/server.cpp

Line 495 in 5c8eb79

throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");

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

Thread #10931 (comment)

Interesting, added comment in 0ed0352.

jonasschnelli · 2017-07-26T09:44:04Z

src/wallet/rpcwallet.cpp

+        // Wallet RPC methods are disabled if no wallets are loaded.
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");
+    }
+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "


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

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.

Thread #10931 (comment)

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.

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.

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.

Thread continues here: #10931 (comment)

jonasschnelli · 2017-07-26T09:44:45Z

utACK 5bb4fe6

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

ryanofsky

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

ryanofsky · 2017-07-26T12:59:46Z

src/wallet/rpcwallet.cpp

+        // Wallet RPC methods are disabled if no wallets are loaded.
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");
+    }
+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "


Thread #10931 (comment)

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.

ryanofsky · 2017-07-26T13:42:54Z

src/wallet/rpcwallet.cpp

+    if (avoidException) return false;
+    if (::vpwallets.empty()) {
+        // Wallet RPC methods are disabled if no wallets are loaded.
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");


Thread #10931 (comment)

Ok, added in 4cfa444

gmaxwell

utACK

achow101 · 2017-07-26T18:46:10Z

utACK 4cfa444

jnewbery · 2017-07-27T01:47:48Z

Tested ACK 4cfa444

laanwj · 2017-07-27T09:37:38Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(
+            RPC_METHOD_NOT_FOUND, "Method not found (wallet method is disabled because no wallet is loaded)");
+    }
+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "


Somehow this got hidden: #10931 (comment)

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.

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.

Thread #10931 (comment)

Ok @laanwj, thanks for providing an explanation. I implemented your suggestion in 6b5c343. 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.

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

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

Squashed 6b5c343 -> df389bc (pr/multierr.3 -> pr/multierr.4)

ryanofsky · 2017-07-27T14:19:08Z

src/wallet/rpcwallet.cpp

+    if (avoidException) return false;
+    if (::vpwallets.empty()) {
+        // Wallet RPC methods are disabled if no wallets are loaded.
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");


Thread #10931 (comment)

Interesting, added comment in 0ed0352.

ryanofsky · 2017-07-27T14:22:54Z

src/wallet/rpcwallet.cpp

+        // Wallet RPC methods are disabled if no wallets are loaded.
+        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found (disabled)");
+    }
+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "


Thread continues here: #10931 (comment)

ryanofsky · 2017-07-27T14:23:06Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(
+            RPC_METHOD_NOT_FOUND, "Method not found (wallet method is disabled because no wallet is loaded)");
+    }
+    throw JSONRPCError(RPC_WALLET_NOT_SPECIFIED, "Wallet file not specified (please provide bitcoin-cli "


Thread #10931 (comment)

Ok @laanwj, thanks for providing an explanation. I implemented your suggestion in 6b5c343. 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 · 2017-07-27T15:53:49Z

utACK df389bc

df389bc Change wallet method disabled error text (Russell Yanofsky) e526b3d Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) Pull request description: 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. Tree-SHA512: 6a8d885283f69bcfc28f2e08ac03eff02f9f8160a312ce2a90d868aa52533434fc0b4c4ab86547c2f09392338956df915637eaf7136a4fc105e6c8179f2d0ac8

…rors df389bc Change wallet method disabled error text (Russell Yanofsky) e526b3d Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) Pull request description: 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. Tree-SHA512: 6a8d885283f69bcfc28f2e08ac03eff02f9f8160a312ce2a90d868aa52533434fc0b4c4ab86547c2f09392338956df915637eaf7136a4fc105e6c8179f2d0ac8

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

fanquake added RPC/REST/ZMQ Wallet labels Jul 26, 2017

laanwj added this to the 0.15.0 milestone Jul 26, 2017

jnewbery reviewed Jul 26, 2017

View reviewed changes

jonasschnelli reviewed Jul 26, 2017

View reviewed changes

Change wallet method disabled error text

df389bc

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

ryanofsky commented Jul 26, 2017

View reviewed changes

gmaxwell approved these changes Jul 26, 2017

View reviewed changes

laanwj reviewed Jul 27, 2017

View reviewed changes

ryanofsky force-pushed the pr/multierr branch from 4cfa444 to df389bc Compare July 27, 2017 15:26

ryanofsky commented Jul 27, 2017

View reviewed changes

laanwj merged commit df389bc into bitcoin:master Jul 27, 2017

ryanofsky mentioned this pull request Jan 2, 2018

Rename account to label where appropriate #11536

Merged

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix misleading "Method not found" multiwallet errors #10931

Fix misleading "Method not found" multiwallet errors #10931

Uh oh!

Conversation

ryanofsky commented Jul 26, 2017

Uh oh!

ryanofsky commented Jul 26, 2017

Uh oh!

sipa commented Jul 26, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jul 26, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 26, 2017

Uh oh!

jnewbery commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 27, 2017

Uh oh!

Uh oh!

laanwj Jul 27, 2017 •

edited

Loading