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.
127@@ -128,7 +128,7 @@ def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
128 def test_nonrbf_bumpfee_fails(peer_node, dest_address):
129 # cannot replace a non RBF transaction (from node which did not enable RBF)
130 not_rbfid = create_fund_sign_send(peer_node, {dest_address: 0.00090000})
131- assert_raises_message(JSONRPCException, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
132+ assert_raises_jsonrpc(-4, "not BIP 125 replaceable", peer_node.bumpfee, not_rbfid)
RPC_WALLET_ERROR instead of -4.
2752@@ -2753,33 +2753,33 @@ UniValue bumpfee(const JSONRPCRequest& request)
2753 CWalletTx& wtx = pwalletMain->mapWallet[hash];
2754
2755 if (pwalletMain->HasWalletSpend(hash)) {
2756- throw JSONRPCError(RPC_MISC_ERROR, "Transaction has descendants in the wallet");
2757+ 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
342+ throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete the transaction.");
343 }
344
345 if(vHashOut.empty()) {
346- throw JSONRPCError(RPC_INTERNAL_ERROR, "Transaction does not exist in wallet.");
347+ throw JSONRPCError(RPC_WALLET_ERROR, "Transaction does not exist in wallet.");
337@@ -338,11 +338,11 @@ UniValue removeprunedfunds(const JSONRPCRequest& request)
338 vector<uint256> vHashOut;
339
340 if(pwalletMain->ZapSelectTx(vHash, vHashOut) != DB_LOAD_OK) {
341- throw JSONRPCError(RPC_INTERNAL_ERROR, "Could not properly delete the transaction.");
342+ throw JSONRPCError(RPC_WALLET_ERROR, "Could not properly delete the transaction.");
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.
833@@ -829,7 +834,7 @@ UniValue pruneblockchain(const JSONRPCRequest& request)
834 + HelpExampleRpc("pruneblockchain", "1000"));
835
836 if (!fPruneMode)
837- throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Cannot prune blocks because node is not in prune mode.");
838+ throw JSONRPCError(RPC_MISC_ERROR, "Cannot prune blocks because node is not in prune mode.");
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.
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…
Also, I was wondering if this requires a mention in the release notes…
It does.
utACK 3ff485e32f7a1d068e799d323d3531ede22873f1
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.