From 6d07c62322f60eb2702c6654e994fc353bcfcf8c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Tue, 7 Feb 2017 14:27:57 -0500 Subject: [PATCH] Return correct error codes in bumpfee(). 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. --- qa/rpc-tests/bumpfee.py | 18 ++++++++---------- src/wallet/rpcwallet.cpp | 24 ++++++++++++------------ 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/qa/rpc-tests/bumpfee.py b/qa/rpc-tests/bumpfee.py index cc897a32c7..68e9808ea5 100755 --- a/qa/rpc-tests/bumpfee.py +++ b/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) def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address): @@ -148,7 +148,7 @@ def test_notmine_bumpfee_fails(rbf_node, peer_node, dest_address): signedtx = rbf_node.signrawtransaction(rawtx) signedtx = peer_node.signrawtransaction(signedtx["hex"]) rbfid = rbf_node.sendrawtransaction(signedtx["hex"]) - assert_raises_message(JSONRPCException, "Transaction contains inputs that don't belong to this wallet", + assert_raises_jsonrpc(-4, "Transaction contains inputs that don't belong to this wallet", rbf_node.bumpfee, rbfid) @@ -159,7 +159,7 @@ def test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address) tx = rbf_node.createrawtransaction([{"txid": parent_id, "vout": 0}], {dest_address: 0.00020000}) tx = rbf_node.signrawtransaction(tx) txid = rbf_node.sendrawtransaction(tx["hex"]) - assert_raises_message(JSONRPCException, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id) + assert_raises_jsonrpc(-8, "Transaction has descendants in the wallet", rbf_node.bumpfee, parent_id) def test_small_output_fails(rbf_node, dest_address): @@ -174,7 +174,7 @@ def test_small_output_fails(rbf_node, dest_address): Decimal("0.00100000"), {dest_address: 0.00080000, get_change_address(rbf_node): Decimal("0.00010000")}) - assert_raises_message(JSONRPCException, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 20001}) + assert_raises_jsonrpc(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 20001}) def test_dust_to_fee(rbf_node, dest_address): @@ -209,7 +209,7 @@ def test_rebumping(rbf_node, dest_address): rbf_node.settxfee(Decimal("0.00001000")) rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000}) bumped = rbf_node.bumpfee(rbfid, {"totalFee": 1000}) - assert_raises_message(JSONRPCException, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 2000}) + assert_raises_jsonrpc(-4, "already bumped", rbf_node.bumpfee, rbfid, {"totalFee": 2000}) rbf_node.bumpfee(bumped["txid"], {"totalFee": 2000}) @@ -217,7 +217,7 @@ def test_rebumping_not_replaceable(rbf_node, dest_address): # check that re-bumping a non-replaceable bump tx fails rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000}) bumped = rbf_node.bumpfee(rbfid, {"totalFee": 10000, "replaceable": False}) - assert_raises_message(JSONRPCException, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"], + assert_raises_jsonrpc(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"], {"totalFee": 20000}) @@ -268,7 +268,7 @@ def test_bumpfee_metadata(rbf_node, dest_address): def test_locked_wallet_fails(rbf_node, dest_address): rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000}) rbf_node.walletlock() - assert_raises_message(JSONRPCException, "Please enter the wallet passphrase with walletpassphrase first.", + assert_raises_jsonrpc(-13, "Please enter the wallet passphrase with walletpassphrase first.", rbf_node.bumpfee, rbfid) @@ -315,9 +315,7 @@ def submit_block_with_tx(node, tx): block.rehash() block.hashMerkleRoot = block.calc_merkle_root() block.solve() - error = node.submitblock(bytes_to_hex_str(block.serialize(True))) - if error is not None: - raise Exception(error) + node.submitblock(bytes_to_hex_str(block.serialize(True))) return block diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 5ef93ac532..ea6795aeaa 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2707,7 +2707,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request) CBitcoinAddress address(options["changeAddress"].get_str()); if (!address.IsValid()) - throw JSONRPCError(RPC_INVALID_PARAMETER, "changeAddress must be a valid bitcoin address"); + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "changeAddress must be a valid bitcoin address"); changeAddress = address.Get(); } @@ -2862,33 +2862,33 @@ UniValue bumpfee(const JSONRPCRequest& request) CWalletTx& wtx = pwallet->mapWallet[hash]; if (pwallet->HasWalletSpend(hash)) { - throw JSONRPCError(RPC_MISC_ERROR, "Transaction has descendants in the wallet"); + throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction has descendants in the wallet"); } { LOCK(mempool.cs); auto it = mempool.mapTx.find(hash); if (it != mempool.mapTx.end() && it->GetCountWithDescendants() > 1) { - throw JSONRPCError(RPC_MISC_ERROR, "Transaction has descendants in the mempool"); + throw JSONRPCError(RPC_INVALID_PARAMETER, "Transaction has descendants in the mempool"); } } if (wtx.GetDepthInMainChain() != 0) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction has been mined, or is conflicted with a mined transaction"); + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction has been mined, or is conflicted with a mined transaction"); } if (!SignalsOptInRBF(wtx)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction is not BIP 125 replaceable"); + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction is not BIP 125 replaceable"); } if (wtx.mapValue.count("replaced_by_txid")) { - throw JSONRPCError(RPC_INVALID_REQUEST, strprintf("Cannot bump transaction %s which was already bumped by transaction %s", hash.ToString(), wtx.mapValue.at("replaced_by_txid"))); + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Cannot bump transaction %s which was already bumped by transaction %s", hash.ToString(), wtx.mapValue.at("replaced_by_txid"))); } // check that original tx consists entirely of our inputs // if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee) if (!pwallet->IsAllFromMe(wtx, ISMINE_SPENDABLE)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Transaction contains inputs that don't belong to this wallet"); + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction contains inputs that don't belong to this wallet"); } // figure out which output was change @@ -2897,13 +2897,13 @@ UniValue bumpfee(const JSONRPCRequest& request) for (size_t i = 0; i < wtx.tx->vout.size(); ++i) { if (pwallet->IsChange(wtx.tx->vout[i])) { if (nOutput != -1) { - throw JSONRPCError(RPC_MISC_ERROR, "Transaction has multiple change outputs"); + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction has multiple change outputs"); } nOutput = i; } } if (nOutput == -1) { - throw JSONRPCError(RPC_MISC_ERROR, "Transaction does not have a change output"); + throw JSONRPCError(RPC_WALLET_ERROR, "Transaction does not have a change output"); } // Calculate the expected size of the new transaction. @@ -2994,7 +2994,7 @@ UniValue bumpfee(const JSONRPCRequest& request) // Check that in all cases the new fee doesn't violate maxTxFee if (nNewFee > maxTxFee) { - throw JSONRPCError(RPC_MISC_ERROR, + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)", FormatMoney(nNewFee), FormatMoney(maxTxFee))); } @@ -3006,7 +3006,7 @@ UniValue bumpfee(const JSONRPCRequest& request) // moment earlier. In this case, we report an error to the user, who may use totalFee to make an adjustment. CFeeRate minMempoolFeeRate = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) { - throw JSONRPCError(RPC_MISC_ERROR, strprintf("New fee rate (%s) is less than the minimum fee rate (%s) to get into the mempool. totalFee value should to be at least %s or settxfee value should be at least %s to add transaction.", FormatMoney(nNewFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)), FormatMoney(minMempoolFeeRate.GetFeePerK()))); + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("New fee rate (%s) is less than the minimum fee rate (%s) to get into the mempool. totalFee value should to be at least %s or settxfee value should be at least %s to add transaction.", FormatMoney(nNewFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)), FormatMoney(minMempoolFeeRate.GetFeePerK()))); } // Now modify the output to increase the fee. @@ -3016,7 +3016,7 @@ UniValue bumpfee(const JSONRPCRequest& request) CMutableTransaction tx(*(wtx.tx)); CTxOut* poutput = &(tx.vout[nOutput]); if (poutput->nValue < nDelta) { - throw JSONRPCError(RPC_MISC_ERROR, "Change output is too small to bump the fee"); + throw JSONRPCError(RPC_WALLET_ERROR, "Change output is too small to bump the fee"); } // If the output would become dust, discard it (converting the dust to fee)