diff --git a/doc/release-notes-18202.md b/doc/release-notes-18202.md new file mode 100644 index 0000000000..f2bd870ac4 --- /dev/null +++ b/doc/release-notes-18202.md @@ -0,0 +1,8 @@ +Low-level RPC Changes +--------------------- + +- To make RPC `sendtoaddress` more consistent with `sendmany` the following error + `sendtoaddress` codes were changed from `-4` to `-6`: + - Insufficient funds + - Fee estimation failed + - Transaction has too long of a mempool chain diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 3304d4ad0b..9d334063c4 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -359,36 +359,54 @@ static UniValue setlabel(const JSONRPCRequest& request) return NullUniValue; } +void ParseRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs, std::vector &recipients) { + std::set destinations; + int i = 0; + for (const std::string& address: address_amounts.getKeys()) { + CTxDestination dest = DecodeDestination(address); + if (!IsValidDestination(dest)) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + address); + } -static CTransactionRef SendMoney(CWallet* const pwallet, const CTxDestination& address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue) -{ - CAmount curBalance = pwallet->GetBalance(0, coin_control.m_avoid_address_reuse).m_mine_trusted; + if (destinations.count(dest)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); + } + destinations.insert(dest); - // Check amount - if (nValue <= 0) - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid amount"); + CScript script_pub_key = GetScriptForDestination(dest); + CAmount amount = AmountFromValue(address_amounts[i++]); - if (nValue > curBalance) - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds"); + bool subtract_fee = false; + for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) { + const UniValue& addr = subtract_fee_outputs[idx]; + if (addr.get_str() == address) { + subtract_fee = true; + } + } - // Parse Bitcoin address - CScript scriptPubKey = GetScriptForDestination(address); + CRecipient recipient = {script_pub_key, amount, subtract_fee}; + recipients.push_back(recipient); + } +} + +UniValue SendMoney(CWallet* const pwallet, const CCoinControl &coin_control, std::vector &recipients, mapValue_t map_value) +{ + EnsureWalletIsUnlocked(pwallet); - // Create and send the transaction + // Shuffle recipient list + std::shuffle(recipients.begin(), recipients.end(), FastRandomContext()); + + // Send CAmount nFeeRequired = 0; - bilingual_str error; - std::vector vecSend; int nChangePosRet = -1; - CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount}; - vecSend.push_back(recipient); + bilingual_str error; CTransactionRef tx; - if (!pwallet->CreateTransaction(vecSend, tx, nFeeRequired, nChangePosRet, error, coin_control)) { - if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance) - error = strprintf(Untranslated("Error: This transaction requires a transaction fee of at least %s"), FormatMoney(nFeeRequired)); - throw JSONRPCError(RPC_WALLET_ERROR, error.original); + bool fCreated = pwallet->CreateTransaction(recipients, tx, nFeeRequired, nChangePosRet, error, coin_control, !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); + if (!fCreated) { + throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original); } - pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */); - return tx; + pwallet->CommitTransaction(tx, std::move(map_value), {} /* orderForm */); + return tx->GetHash().GetHex(); } static UniValue sendtoaddress(const JSONRPCRequest& request) @@ -436,16 +454,6 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) LOCK(pwallet->cs_wallet); - CTxDestination dest = DecodeDestination(request.params[0].get_str()); - if (!IsValidDestination(dest)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address"); - } - - // Amount - CAmount nAmount = AmountFromValue(request.params[1]); - if (nAmount <= 0) - throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); - // Wallet comments mapValue_t mapValue; if (!request.params[2].isNull() && !request.params[2].get_str().empty()) @@ -471,8 +479,18 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) EnsureWalletIsUnlocked(pwallet); - CTransactionRef tx = SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue)); - return tx->GetHash().GetHex(); + UniValue address_amounts(UniValue::VOBJ); + const std::string address = request.params[0].get_str(); + address_amounts.pushKV(address, request.params[1]); + UniValue subtractFeeFromAmount(UniValue::VARR); + if (fSubtractFeeFromAmount) { + subtractFeeFromAmount.push_back(address); + } + + std::vector recipients; + ParseRecipients(address_amounts, subtractFeeFromAmount, recipients); + + return SendMoney(pwallet, coin_control, recipients, mapValue); } static UniValue listaddressgroupings(const JSONRPCRequest& request) @@ -860,52 +878,10 @@ static UniValue sendmany(const JSONRPCRequest& request) SetFeeEstimateMode(pwallet, coin_control, request.params[7], request.params[6]); - std::set destinations; - std::vector vecSend; + std::vector recipients; + ParseRecipients(sendTo, subtractFeeFromAmount, recipients); - std::vector keys = sendTo.getKeys(); - for (const std::string& name_ : keys) { - CTxDestination dest = DecodeDestination(name_); - if (!IsValidDestination(dest)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + name_); - } - - if (destinations.count(dest)) { - throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + name_); - } - destinations.insert(dest); - - CScript scriptPubKey = GetScriptForDestination(dest); - CAmount nAmount = AmountFromValue(sendTo[name_]); - if (nAmount <= 0) - throw JSONRPCError(RPC_TYPE_ERROR, "Invalid amount for send"); - - bool fSubtractFeeFromAmount = false; - for (unsigned int idx = 0; idx < subtractFeeFromAmount.size(); idx++) { - const UniValue& addr = subtractFeeFromAmount[idx]; - if (addr.get_str() == name_) - fSubtractFeeFromAmount = true; - } - - CRecipient recipient = {scriptPubKey, nAmount, fSubtractFeeFromAmount}; - vecSend.push_back(recipient); - } - - EnsureWalletIsUnlocked(pwallet); - - // Shuffle recipient list - std::shuffle(vecSend.begin(), vecSend.end(), FastRandomContext()); - - // Send - CAmount nFeeRequired = 0; - int nChangePosRet = -1; - bilingual_str error; - CTransactionRef tx; - bool fCreated = pwallet->CreateTransaction(vecSend, tx, nFeeRequired, nChangePosRet, error, coin_control); - if (!fCreated) - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, error.original); - pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */); - return tx->GetHash().GetHex(); + return SendMoney(pwallet, coin_control, recipients, std::move(mapValue)); } static UniValue addmultisigaddress(const JSONRPCRequest& request) diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index 8962362276..81382d94ad 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -119,7 +119,7 @@ class WalletTest(BitcoinTestFramework): assert_raises_rpc_error(-8, "Invalid parameter, expected locked output", self.nodes[2].lockunspent, True, [unspent_0]) self.nodes[2].lockunspent(False, [unspent_0]) assert_raises_rpc_error(-8, "Invalid parameter, output already locked", self.nodes[2].lockunspent, False, [unspent_0]) - assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 20) + assert_raises_rpc_error(-6, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 20) assert_equal([unspent_0], self.nodes[2].listlockunspent()) self.nodes[2].lockunspent(True, [unspent_0]) assert_equal(len(self.nodes[2].listlockunspent()), 0) @@ -363,6 +363,9 @@ class WalletTest(BitcoinTestFramework): assert_equal(tx_obj['amount'], Decimal('-0.0001')) # General checks for errors from incorrect inputs + # This will raise an exception because the amount is negative + assert_raises_rpc_error(-3, "Amount out of range", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "-1") + # This will raise an exception because the amount type is wrong assert_raises_rpc_error(-3, "Invalid amount", self.nodes[0].sendtoaddress, self.nodes[2].getnewaddress(), "1f-4") @@ -590,7 +593,7 @@ class WalletTest(BitcoinTestFramework): node0_balance = self.nodes[0].getbalance() # With walletrejectlongchains we will not create the tx and store it in our wallet. - assert_raises_rpc_error(-4, "Transaction has too long of a mempool chain", self.nodes[0].sendtoaddress, sending_addr, node0_balance - Decimal('0.01')) + assert_raises_rpc_error(-6, "Transaction has too long of a mempool chain", self.nodes[0].sendtoaddress, sending_addr, node0_balance - Decimal('0.01')) # Verify nothing new in wallet assert_equal(total_txs, len(self.nodes[0].listtransactions("*", 99999))) diff --git a/test/functional/wallet_fallbackfee.py b/test/functional/wallet_fallbackfee.py index 0c67982bbe..dbf853b35c 100755 --- a/test/functional/wallet_fallbackfee.py +++ b/test/functional/wallet_fallbackfee.py @@ -22,7 +22,7 @@ class WalletRBFTest(BitcoinTestFramework): # test sending a tx with disabled fallback fee (must fail) self.restart_node(0, extra_args=["-fallbackfee=0"]) - assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)) + assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)) assert_raises_rpc_error(-4, "Fee estimation failed", lambda: self.nodes[0].fundrawtransaction(self.nodes[0].createrawtransaction([], {self.nodes[0].getnewaddress(): 1}))) assert_raises_rpc_error(-6, "Fee estimation failed", lambda: self.nodes[0].sendmany("", {self.nodes[0].getnewaddress(): 1}))