From f86263b1807e9b2d6b519bfac22847681db432fe Mon Sep 17 00:00:00 2001 From: Samuel Dobson Date: Sun, 12 Jul 2020 14:31:46 +1200 Subject: [PATCH] Merge #18202: refactor: consolidate sendmany and sendtoaddress code 08fc6f6cfc3b06fd170452a766696d7b833113fa [rpc] refactor: consolidate sendmany and sendtoaddress code (Sjors Provoost) Pull request description: I consolidated code between these two RPC calls, since `sendtoaddress` is essentially `sendmany` with 1 destination. Unless I overlooked something, the only behaviour change is that some `sendtoaddress` error codes changed from `-4` to `-6`. The release note mentions this. Salvaged from #18201. ACKs for top commit: fjahr: Code review ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa jonatack: ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa meshcollider: Code review & functional test run ACK 08fc6f6cfc3b06fd170452a766696d7b833113fa Tree-SHA512: 7b66c52fa0444a4d02fc3f81d9c2a386794d447616026a30111eda35fb46510475eea6506a9ceda00bb4e0230ebb758da5d236b3ac05c954c044fa68a1e3e909 --- doc/release-notes-18202.md | 8 ++ src/wallet/rpcwallet.cpp | 140 +++++++++++--------------- test/functional/wallet_basic.py | 7 +- test/functional/wallet_fallbackfee.py | 2 +- 4 files changed, 70 insertions(+), 87 deletions(-) create mode 100644 doc/release-notes-18202.md 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 9b551a3bcc..476b799ca3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -352,40 +352,55 @@ static RPCHelpMan setlabel() }; } +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 Dash address: ") + address); + } -static CTransactionRef SendMoney(CWallet* const pwallet, const CTxDestination& address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue) + if (destinations.count(dest)) { + throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address); + } + destinations.insert(dest); + + CScript script_pub_key = GetScriptForDestination(dest); + CAmount amount = AmountFromValue(address_amounts[i++]); + + 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; + } + } + + 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) { - CAmount curBalance = pwallet->GetBalance(0, coin_control.m_avoid_address_reuse).m_mine_trusted; - - // Check amount - if (nValue <= 0) - throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid amount"); - - if (nValue > curBalance) - throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Insufficient funds"); + EnsureWalletIsUnlocked(pwallet); if (coin_control.IsUsingCoinJoin()) { - mapValue["DS"] = "1"; + map_value["DS"] = "1"; } - // Parse Dash address - CScript scriptPubKey = GetScriptForDestination(address); - - // Create and send the transaction + // 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 RPCHelpMan sendtoaddress() @@ -434,16 +449,6 @@ static RPCHelpMan sendtoaddress() 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 +476,18 @@ static RPCHelpMan sendtoaddress() 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); }, }; } @@ -935,9 +950,9 @@ static RPCHelpMan sendmany() if (!request.params[4].isNull() && !request.params[4].get_str().empty()) mapValue["comment"] = request.params[4].get_str(); - UniValue subtractFeeFrom(UniValue::VARR); + UniValue subtractFeeFromAmount(UniValue::VARR); if (!request.params[5].isNull()) - subtractFeeFrom = request.params[5].get_array(); + subtractFeeFromAmount = request.params[5].get_array(); // request.params[6] ("use_is") is deprecated and not used here @@ -949,53 +964,10 @@ static RPCHelpMan sendmany() SetFeeEstimateMode(pwallet, coin_control, request.params[9], request.params[8]); - if (coin_control.IsUsingCoinJoin()) { - mapValue["DS"] = "1"; - } + std::vector recipients; + ParseRecipients(sendTo, subtractFeeFromAmount, recipients); - std::set destinations; - std::vector vecSend; - - 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 Dash 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 < subtractFeeFrom.size(); idx++) { - const UniValue& addr = subtractFeeFrom[idx]; - if (addr.get_str() == name_) - fSubtractFeeFromAmount = true; - } - - CRecipient recipient = {scriptPubKey, nAmount, fSubtractFeeFromAmount}; - vecSend.push_back(recipient); - } - - EnsureWalletIsUnlocked(pwallet); - - // 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)); }, }; } diff --git a/test/functional/wallet_basic.py b/test/functional/wallet_basic.py index ad80f4d94c..9020a72329 100755 --- a/test/functional/wallet_basic.py +++ b/test/functional/wallet_basic.py @@ -126,7 +126,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(), 200) + assert_raises_rpc_error(-6, "Insufficient funds", self.nodes[2].sendtoaddress, self.nodes[2].getnewaddress(), 200) assert_equal([unspent_0], self.nodes[2].listlockunspent()) self.nodes[2].lockunspent(True, [unspent_0]) assert_equal(len(self.nodes[2].listlockunspent()), 0) @@ -380,6 +380,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") @@ -608,7 +611,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 c481c6492b..b28f3ecebc 100755 --- a/test/functional/wallet_fallbackfee.py +++ b/test/functional/wallet_fallbackfee.py @@ -24,7 +24,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}))