Fix error codes from various RPCs #9853

jnewbery · 2017-02-24T16:28:48Z

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

ryanofsky · 2017-02-24T16:35:28Z

qa/rpc-tests/bumpfee.py

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


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.

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.

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.

ryanofsky

utACK b34faf156d49705faa335641cfbfc0efd60bb781, definitely an improvement.

ryanofsky · 2017-02-28T19:21:05Z

src/wallet/rpcwallet.cpp

@@ -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");


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.

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

ryanofsky · 2017-02-28T19:29:39Z

src/wallet/rpcdump.cpp

    }

    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.");


Maybe invalid parameter here as well.

ryanofsky · 2017-02-28T19:47:36Z

src/wallet/rpcdump.cpp

@@ -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.");


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

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.

ryanofsky · 2017-02-28T19:50:56Z

src/rpc/blockchain.cpp

@@ -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.");


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

Sounds sensible, but again let's leave that for a future PR.

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.

The bumpfee() RPC was returning misleading or incorrect error codes (for example RPC_INVALID_ADDRESS_OR_KEY when the transaction was not BIP125 replacable). This commit fixes those error codes: - RPC_INVALID_ADDRESS_OR_KEY if an invalid address was provided: - Invalid change address given - RPC_INVALID_PARAMETER if a single (non-address/key) parameter is incorrect - confTarget and totalFee options should not both be set. - Invalid confTarget - Insufficient totalFee (cannot be less than required fee) - RPC_WALLET_ERROR for any other error - Transaction has descendants in the wallet - Transaction has descendants in the mempool - Transaction has been mined, or is conflicted with a mined transaction - Transaction is not BIP 125 replaceable - Transaction has already been bumped - Transaction contains inputs that don't belong to the wallet - Transaction has multiple change outputs - Transaction does not have a change output - Fee is higher than maxTxFee - New fee rate is less than the minimum fee rate - Change output is too small. This commit also updates the test cases to explicitly test the error code.

RPCs in blockchain.cpp were returning misleading or incorrect error codes (for example getblock() returning RPC_INTERNAL_ERROR when the block had been pruned). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. - RPC_METHOD_NOT_FOUND should not be returned in response to a JSON request for an existing method. Those error codes have been replaced with RPC_MISC_ERROR or RPC_INVALID_PARAMETER as appropriate.

The removeprunedfunds() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when the transaction was not found in the wallet). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. This error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code.

The setban() RPC was returning misleading or incorrect error codes (for example RPC_CLIENT_NODE_ALREADY_ADDED when an invalid IP address was entered). This commit fixes those error codes: - RPC_CLIENT_INVALID_IP_OR_SUBNET should be returned if the client enters an invalid IP address or subnet. This commit also updates the test cases to explicitly test the error code. This commit also adds a testcase for trying to setban on an invalid subnet.

The fundrawtransaction() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when funding the transaction failed). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. That error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code.

jnewbery · 2017-03-07T21:49:06Z

Squashed @ryanofsky's nits and rebased.
Old tip: 5158a1e / https://github.com/jnewbery/bitcoin/tree/pr9853.1
New tip: 3ff485e / https://github.com/jnewbery/bitcoin/tree/pr9853.2

laanwj · 2017-03-08T14:51:07Z

utACK 3ff485e

maflcko · 2017-03-08T15:07:50Z

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

laanwj · 2017-03-08T15:15:28Z

Also, I was wondering if this requires a mention in the release notes...

It does.

ryanofsky · 2017-03-08T18:34:43Z

utACK 3ff485e

Confirmed only minor changes since previous ACK.

RPC_INVALID_REQUEST and RPC_METHOD_NOT_FOUND are mapped internally to HTTP error codes and should not be used for application-layer errors. This commit adds commenting around those definitions to warn not to use them for application errors.

jnewbery · 2017-03-08T19:24:41Z

Release notes updated. Otherwise no change.

adaa281 Update release notes to include RPC error code changes. (John Newbery) 338bf06 Add commenting around JSON error codes (John Newbery) dab804c Return correct error codes in fundrawtransaction(). (John Newbery) a012087 Return correct error codes in setban(). (John Newbery) 960bc7f Return correct error codes in removeprunedfunds(). (John Newbery) c119096 Return correct error codes in blockchain.cpp. (John Newbery) 6d07c62 Return correct error codes in bumpfee(). (John Newbery) Tree-SHA512: 4bb39ad221cd8c83d98ac5d7ad642f3a8c265522720dc86b2eebc70e74439a85b06d6ddcd6a874e879d986511de3ab0878bb7fe58b50cb0546b78913632ea809

The setban() RPC was returning misleading or incorrect error codes (for example RPC_CLIENT_NODE_ALREADY_ADDED when an invalid IP address was entered). This commit fixes those error codes: - RPC_CLIENT_INVALID_IP_OR_SUBNET should be returned if the client enters an invalid IP address or subnet. This commit also updates the test cases to explicitly test the error code. This commit also adds a testcase for trying to setban on an invalid subnet. Github-Pull: bitcoin#9853 Rebased-From: a012087

The fundrawtransaction() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when funding the transaction failed). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. That error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: dab804c

Github-Pull: bitcoin#9853 Rebased-From: adaa281

The bumpfee() RPC was returning misleading or incorrect error codes (for example RPC_INVALID_ADDRESS_OR_KEY when the transaction was not BIP125 replacable). This commit fixes those error codes: - RPC_INVALID_ADDRESS_OR_KEY if an invalid address was provided: - Invalid change address given - RPC_INVALID_PARAMETER if a single (non-address/key) parameter is incorrect - confTarget and totalFee options should not both be set. - Invalid confTarget - Insufficient totalFee (cannot be less than required fee) - RPC_WALLET_ERROR for any other error - Transaction has descendants in the wallet - Transaction has descendants in the mempool - Transaction has been mined, or is conflicted with a mined transaction - Transaction is not BIP 125 replaceable - Transaction has already been bumped - Transaction contains inputs that don't belong to the wallet - Transaction has multiple change outputs - Transaction does not have a change output - Fee is higher than maxTxFee - New fee rate is less than the minimum fee rate - Change output is too small. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: 6d07c62

RPCs in blockchain.cpp were returning misleading or incorrect error codes (for example getblock() returning RPC_INTERNAL_ERROR when the block had been pruned). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. - RPC_METHOD_NOT_FOUND should not be returned in response to a JSON request for an existing method. Those error codes have been replaced with RPC_MISC_ERROR or RPC_INVALID_PARAMETER as appropriate. Github-Pull: bitcoin#9853 Rebased-From: c119096

The removeprunedfunds() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when the transaction was not found in the wallet). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. This error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: 960bc7f

The setban() RPC was returning misleading or incorrect error codes (for example RPC_CLIENT_NODE_ALREADY_ADDED when an invalid IP address was entered). This commit fixes those error codes: - RPC_CLIENT_INVALID_IP_OR_SUBNET should be returned if the client enters an invalid IP address or subnet. This commit also updates the test cases to explicitly test the error code. This commit also adds a testcase for trying to setban on an invalid subnet. Github-Pull: bitcoin#9853 Rebased-From: a012087

The fundrawtransaction() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when funding the transaction failed). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. That error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: dab804c

Github-Pull: bitcoin#9853 Rebased-From: adaa281

… branch 'disconnect_ban_fixes-0.14' into 0.14.2_fixes

The bumpfee() RPC was returning misleading or incorrect error codes (for example RPC_INVALID_ADDRESS_OR_KEY when the transaction was not BIP125 replacable). This commit fixes those error codes: - RPC_INVALID_ADDRESS_OR_KEY if an invalid address was provided: - Invalid change address given - RPC_INVALID_PARAMETER if a single (non-address/key) parameter is incorrect - confTarget and totalFee options should not both be set. - Invalid confTarget - Insufficient totalFee (cannot be less than required fee) - RPC_WALLET_ERROR for any other error - Transaction has descendants in the wallet - Transaction has descendants in the mempool - Transaction has been mined, or is conflicted with a mined transaction - Transaction is not BIP 125 replaceable - Transaction has already been bumped - Transaction contains inputs that don't belong to the wallet - Transaction has multiple change outputs - Transaction does not have a change output - Fee is higher than maxTxFee - New fee rate is less than the minimum fee rate - Change output is too small. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: 6d07c62

RPCs in blockchain.cpp were returning misleading or incorrect error codes (for example getblock() returning RPC_INTERNAL_ERROR when the block had been pruned). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. - RPC_METHOD_NOT_FOUND should not be returned in response to a JSON request for an existing method. Those error codes have been replaced with RPC_MISC_ERROR or RPC_INVALID_PARAMETER as appropriate. Github-Pull: bitcoin#9853 Rebased-From: c119096

The removeprunedfunds() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when the transaction was not found in the wallet). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. This error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: 960bc7f

The setban() RPC was returning misleading or incorrect error codes (for example RPC_CLIENT_NODE_ALREADY_ADDED when an invalid IP address was entered). This commit fixes those error codes: - RPC_CLIENT_INVALID_IP_OR_SUBNET should be returned if the client enters an invalid IP address or subnet. This commit also updates the test cases to explicitly test the error code. This commit also adds a testcase for trying to setban on an invalid subnet. Github-Pull: bitcoin#9853 Rebased-From: a012087

The fundrawtransaction() RPC was returning misleading or incorrect error codes (for example RPC_INTERNAL_ERROR when funding the transaction failed). This commit fixes those error codes: - RPC_INTERNAL_ERROR should not be returned for application-level errors, only for genuine internal errors such as corrupted data. That error code has been replaced with RPC_WALLET_ERROR. This commit also updates the test cases to explicitly test the error code. Github-Pull: bitcoin#9853 Rebased-From: dab804c

Github-Pull: bitcoin#9853 Rebased-From: adaa281

adaa281 Update release notes to include RPC error code changes. (John Newbery) 338bf06 Add commenting around JSON error codes (John Newbery) dab804c Return correct error codes in fundrawtransaction(). (John Newbery) a012087 Return correct error codes in setban(). (John Newbery) 960bc7f Return correct error codes in removeprunedfunds(). (John Newbery) c119096 Return correct error codes in blockchain.cpp. (John Newbery) 6d07c62 Return correct error codes in bumpfee(). (John Newbery) Tree-SHA512: 4bb39ad221cd8c83d98ac5d7ad642f3a8c265522720dc86b2eebc70e74439a85b06d6ddcd6a874e879d986511de3ab0878bb7fe58b50cb0546b78913632ea809

This was referenced Feb 24, 2017

Fix error causes and messages in rpc/net.cpp #9713

Closed

Return correct error codes from bumpfee() #9714

Closed

ryanofsky reviewed Feb 24, 2017

View reviewed changes

fanquake added the RPC/REST/ZMQ label Feb 25, 2017

ryanofsky reviewed Feb 28, 2017

View reviewed changes

jnewbery force-pushed the fixerrorcodes branch from c994421 to 4c17b4b Compare March 1, 2017 19:28

jnewbery added 5 commits March 7, 2017 16:45

jnewbery force-pushed the fixerrorcodes branch from 4c17b4b to 3ff485e Compare March 7, 2017 21:45

laanwj self-assigned this Mar 8, 2017

jnewbery added 2 commits March 8, 2017 14:22

Add commenting around JSON error codes

338bf06

RPC_INVALID_REQUEST and RPC_METHOD_NOT_FOUND are mapped internally to HTTP error codes and should not be used for application-layer errors. This commit adds commenting around those definitions to warn not to use them for application errors.

Update release notes to include RPC error code changes.

adaa281

jnewbery force-pushed the fixerrorcodes branch from 3ff485e to adaa281 Compare March 8, 2017 19:23

laanwj merged commit adaa281 into bitcoin:master Mar 9, 2017

ryanofsky mentioned this pull request Mar 9, 2017

Refactor Bumpfee, move core functionality to CWallet #9681

Merged

jnewbery deleted the fixerrorcodes branch March 9, 2017 14:37

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jun 3, 2017

Update release notes to include RPC error code changes.

a44f684

Github-Pull: bitcoin#9853 Rebased-From: adaa281

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

Update release notes to include RPC error code changes.

c25d0a8

Github-Pull: bitcoin#9853 Rebased-From: adaa281

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

Merge bitcoin#9853 via branch 'fixerrorcodes-0.14' into 0.14.2_fixes

99e5dbd

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

Merge bitcoin#10234, bitcoin#10143 (fixes only), and bitcoin#9853 via…

e05799a

… branch 'disconnect_ban_fixes-0.14' into 0.14.2_fixes

nomnombtc pushed a commit to nomnombtc/bitcoin that referenced this pull request Jul 17, 2017

Update release notes to include RPC error code changes.

fc1aa80

Github-Pull: bitcoin#9853 Rebased-From: adaa281

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

Fix error codes from various RPCs #9853

Fix error codes from various RPCs #9853

Uh oh!

Conversation

jnewbery commented Feb 24, 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!

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!

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!

jnewbery commented Mar 7, 2017

Uh oh!

laanwj commented Mar 8, 2017

Uh oh!

maflcko commented Mar 8, 2017 via email

Uh oh!

laanwj commented Mar 8, 2017

Uh oh!

ryanofsky commented Mar 8, 2017

Uh oh!

jnewbery commented Mar 8, 2017

Uh oh!

Uh oh!