LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

Return correct error codes in bumpfee().

Browse source 
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.
This commit is contained in:
John Newbery 2017-02-07 14:27:57 -05:00
parent 47510ad3dd
commit 6d07c62322
2 changed files with 20 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

18
qa/rpc-tests/bumpfee.py
View file

@ -128,7 +128,7 @@ def test_segwit_bumpfee_succeeds(rbf_node, dest_address):
def test_nonrbf_bumpfee_fails(peer_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) # 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}) 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): 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 = rbf_node.signrawtransaction(rawtx)
signedtx = peer_node.signrawtransaction(signedtx["hex"]) signedtx = peer_node.signrawtransaction(signedtx["hex"])
rbfid = rbf_node.sendrawtransaction(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) 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.createrawtransaction([{"txid": parent_id, "vout": 0}], {dest_address: 0.00020000})
tx = rbf_node.signrawtransaction(tx) tx = rbf_node.signrawtransaction(tx)
txid = rbf_node.sendrawtransaction(tx["hex"]) 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): 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"), Decimal("0.00100000"),
{dest_address: 0.00080000, {dest_address: 0.00080000,
get_change_address(rbf_node): Decimal("0.00010000")}) 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): 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")) rbf_node.settxfee(Decimal("0.00001000"))
rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000}) rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000})
bumped = rbf_node.bumpfee(rbfid, {"totalFee": 1000}) 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}) 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 # check that re-bumping a non-replaceable bump tx fails
rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000}) rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000})
bumped = rbf_node.bumpfee(rbfid, {"totalFee": 10000, "replaceable": False}) 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}) {"totalFee": 20000})
@ -268,7 +268,7 @@ def test_bumpfee_metadata(rbf_node, dest_address):
def test_locked_wallet_fails(rbf_node, dest_address): def test_locked_wallet_fails(rbf_node, dest_address):
rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000}) rbfid = create_fund_sign_send(rbf_node, {dest_address: 0.00090000})
rbf_node.walletlock() 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) rbf_node.bumpfee, rbfid)
@ -315,9 +315,7 @@ def submit_block_with_tx(node, tx):
block.rehash() block.rehash()
block.hashMerkleRoot = block.calc_merkle_root() block.hashMerkleRoot = block.calc_merkle_root()
block.solve() block.solve()
error = node.submitblock(bytes_to_hex_str(block.serialize(True))) node.submitblock(bytes_to_hex_str(block.serialize(True)))
if error is not None:
raise Exception(error)
return block return block

24
src/wallet/rpcwallet.cpp
View file

@ -2707,7 +2707,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
CBitcoinAddress address(options["changeAddress"].get_str()); CBitcoinAddress address(options["changeAddress"].get_str());
if (!address.IsValid()) 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(); changeAddress = address.Get();
} }
@ -2862,33 +2862,33 @@ UniValue bumpfee(const JSONRPCRequest& request)
CWalletTx& wtx = pwallet->mapWallet[hash]; CWalletTx& wtx = pwallet->mapWallet[hash];
if (pwallet->HasWalletSpend(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); LOCK(mempool.cs);
auto it = mempool.mapTx.find(hash); auto it = mempool.mapTx.find(hash);
if (it != mempool.mapTx.end() && it->GetCountWithDescendants() > 1) { 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) { 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)) { 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")) { 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 // 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 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)) { 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 // 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) { for (size_t i = 0; i < wtx.tx->vout.size(); ++i) {
if (pwallet->IsChange(wtx.tx->vout[i])) { if (pwallet->IsChange(wtx.tx->vout[i])) {
if (nOutput != -1) { 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; nOutput = i;
} }
} }
if (nOutput == -1) { 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. // 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 // Check that in all cases the new fee doesn't violate maxTxFee
if (nNewFee > 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)", strprintf("Specified or calculated fee %s is too high (cannot be higher than maxTxFee %s)",
FormatMoney(nNewFee), FormatMoney(maxTxFee))); 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. // 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); CFeeRate minMempoolFeeRate = mempool.GetMinFee(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
if (nNewFeeRate.GetFeePerK() < minMempoolFeeRate.GetFeePerK()) { 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. // Now modify the output to increase the fee.
@ -3016,7 +3016,7 @@ UniValue bumpfee(const JSONRPCRequest& request)
CMutableTransaction tx(*(wtx.tx)); CMutableTransaction tx(*(wtx.tx));
CTxOut* poutput = &(tx.vout[nOutput]); CTxOut* poutput = &(tx.vout[nOutput]);
if (poutput->nValue < nDelta) { 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) // If the output would become dust, discard it (converting the dust to fee)