Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
Fix error codes from various RPCs #9853
Conversation
This was referenced Feb 24, 2017
| @@ -128,7 +128,7 @@ def test_segwit_bumpfee_succeeds(rbf_node, dest_address): | ||
| def test_nonrbf_bumpfee_fails(peer_node, dest_address): | ||
| # cannot replace a non RBF transaction (from node which did not enable RBF) | ||
| not_rbfid = create_fund_sign_send(peer_node, {dest_address: 0.00090000}) | ||
| - assert_raises_message(JSONRPCException, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid) | ||
| + assert_raises_jsonrpc(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid) |
ryanofsky
Feb 24, 2017
Contributor
Is there a plan to switch from integer literals to constants in a later PR? Seems like it would be nice to say RPC_WALLET_ERROR instead of -4.
laanwj
Feb 25, 2017
Owner
Yes that would be nice, but is orthogonal to this change, so if you want to do that it'd indeed be a different PR.
jnewbery
Feb 28, 2017
Member
yep, I'll put that in the todo list. I'm working on a primitives.py for bitcoin messages, datastructures and constants. Seems like a good place for the RPC error constants to live.
fanquake
added
the
RPC/REST/ZMQ
label
Feb 25, 2017
| @@ -829,7 +834,7 @@ UniValue pruneblockchain(const JSONRPCRequest& request) | ||
| + HelpExampleRpc("pruneblockchain", "1000")); | ||
| if (!fPruneMode) | ||
| - throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Cannot prune blocks because node is not in prune mode."); | ||
| + throw JSONRPCError(RPC_MISC_ERROR, "Cannot prune blocks because node is not in prune mode."); |
ryanofsky
Feb 28, 2017
Contributor
Some of these MISC_ERROR cases make it seem like we could benefit from an new RPC_INVALID_STATE value. (Blockchain too short is another case below.)
laanwj
Mar 2, 2017
Owner
Yes, there is no reason why we couldn't add new status values. There's no need to make everything fit in the arbitrary bag of codes that exists now, which has been there unchanged for a long while.
| @@ -338,11 +338,11 @@ UniValue removeprunedfunds(const JSONRPCRequest& request) | ||
| vector<uint256> vHashOut; | ||
| if(pwalletMain->ZapSelectTx(vHash, vHashOut) != DB_LOAD_OK) { | ||
| - throw JSONRPCError(RPC_INTERNAL_ERROR, "Could not properly delete the transaction."); | ||
| + throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete the transaction."); |
ryanofsky
Feb 28, 2017
Contributor
ZapSelect returns a DBErrors enum value which seems like it could be mapped to an RPC error (would also be nice to include in the error string).
jnewbery
Feb 28, 2017
Member
I agree we could do better, but let's leave that for a future PR.
I think this is strictly an improvement because currently the API is returning RPC_INTERNAL_ERROR even in cases where there's no internal error.
| } | ||
| if(vHashOut.empty()) { | ||
| - throw JSONRPCError(RPC_INTERNAL_ERROR, "Transaction does not exist in wallet."); | ||
| + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction does not exist in wallet."); |
| @@ -2753,33 +2753,33 @@ UniValue bumpfee(const JSONRPCRequest& request) | ||
| CWalletTx& wtx = pwalletMain->mapWallet[hash]; | ||
| if (pwalletMain->HasWalletSpend(hash)) { | ||
| - throw JSONRPCError(RPC_MISC_ERROR, "Transaction has descendants in the wallet"); | ||
| + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction has descendants in the wallet"); |
ryanofsky
Feb 28, 2017
Contributor
In cases where bumpfee is called erroneously with a transaction that shouldn't be bumped, invalid parameter seems more apt than wallet error.
Probably the most important thing you want to know when you get an error message from an API is whether the error is caused by something that you control and need to fix, or whether it's caused by something out of your control (like an implementation bug, or corrupt data, or a temporary failure). RPC_INVALID_PARAMETER clearly tells you that you need to fix something, while RPC_WALLET_ERROR is vague but suggests that something's wrong with the wallet.
jnewbery
Feb 28, 2017
Member
Probably the most important thing you want to know when you get an error message from an API is whether the error is caused by something that you control and need to fix, or whether it's caused by something out of your control
Nicely put. I'll update this to RPC_INVALID_PARAMETER
jnewbery
added some commits
Feb 7, 2017
|
Squashed @ryanofsky's nits and rebased. |
laanwj
self-assigned this
Mar 8, 2017
|
utACK 3ff485e |
|
Concept ACK.
Style Nit: In blockchain.cpp you are missing braces around the
multiline if block
Also, I was wondering if this requires a mention in the release notes...
|
It does. |
|
utACK 3ff485e Confirmed only minor changes since previous ACK. |
jnewbery
added some commits
Feb 24, 2017
|
Release notes updated. Otherwise no change. |
jnewbery commentedFeb 24, 2017
(Completes #9707 and #9842)
I've changed the qa test code to properly assert error codes and messages for failed RPC calls. That's revealed a few RPCs which were returning incorrect or misleading error codes. This PR fixes all of those cases.
It also adds commenting around the definition of the JSON RPC error constants to warn against using RPC_INVALID_REQUEST and RPC_METHOD_NOT_FOUND for application-layer errors.