From a5da10e29b8a6399332d9ab751bc00a9f12ef8e9 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Tue, 6 Feb 2024 15:41:26 +0000 Subject: [PATCH] merge bitcoin#16377: don't automatically append inputs in walletcreatefundedpsbt Co-authored-by: Konstantin Akimov --- doc/release-notes-5861.md | 9 +++++++ src/wallet/coincontrol.cpp | 2 +- src/wallet/coincontrol.h | 2 ++ src/wallet/rpcwallet.cpp | 29 ++++++++++++++++------- src/wallet/wallet.cpp | 5 ++++ test/functional/rpc_fundrawtransaction.py | 14 +++++++++-- test/functional/rpc_psbt.py | 19 ++++++++++----- 7 files changed, 63 insertions(+), 17 deletions(-) create mode 100644 doc/release-notes-5861.md diff --git a/doc/release-notes-5861.md b/doc/release-notes-5861.md new file mode 100644 index 0000000000..3442fa451b --- /dev/null +++ b/doc/release-notes-5861.md @@ -0,0 +1,9 @@ +RPC changes +----------- +- The `walletcreatefundedpsbt` RPC call will now fail with + `Insufficient funds` when inputs are manually selected but are not enough to cover + the outputs and fee. Additional inputs can automatically be added through the + new `add_inputs` option. + +- The `fundrawtransaction` RPC now supports `add_inputs` option that when `false` + prevents adding more inputs if necessary and consequently the RPC fails. diff --git a/src/wallet/coincontrol.cpp b/src/wallet/coincontrol.cpp index 2778a566a3..f21ff8d3bf 100644 --- a/src/wallet/coincontrol.cpp +++ b/src/wallet/coincontrol.cpp @@ -9,6 +9,7 @@ void CCoinControl::SetNull(bool fResetCoinType) { destChange = CNoDestination(); + m_add_inputs = true; fAllowOtherInputs = false; fAllowWatchOnly = false; m_avoid_partial_spends = gArgs.GetBoolArg("-avoidpartialspends", DEFAULT_AVOIDPARTIALSPENDS); @@ -26,4 +27,3 @@ void CCoinControl::SetNull(bool fResetCoinType) nCoinType = CoinType::ALL_COINS; } } - diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index 221f5a8e39..920e9d606b 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -37,6 +37,8 @@ class CCoinControl { public: CTxDestination destChange; + //! If false, only selected inputs are used + bool m_add_inputs; //! If false, allows unselected inputs, but requires all selected inputs be used if fAllowOtherInputs is true (default) bool fAllowOtherInputs; //! If false, only include as many inputs as necessary to fulfill a coin selection request. Only usable together with fAllowOtherInputs diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b438ddb27b..307e07adc5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3234,13 +3234,12 @@ static UniValue listunspent(const JSONRPCRequest& request) return results; } -void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options) +void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl) { // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - CCoinControl coinControl; change_position = -1; bool lockUnspents = false; UniValue subtractFeeFromOutputs; @@ -3255,6 +3254,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f RPCTypeCheckArgument(options, UniValue::VOBJ); RPCTypeCheckObj(options, { + {"add_inputs", UniValueType(UniValue::VBOOL)}, {"changeAddress", UniValueType(UniValue::VSTR)}, {"changePosition", UniValueType(UniValue::VNUM)}, {"includeWatching", UniValueType(UniValue::VBOOL)}, @@ -3266,6 +3266,10 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f }, true, true); + if (options.exists("add_inputs") ) { + coinControl.m_add_inputs = options["add_inputs"].get_bool(); + } + if (options.exists("changeAddress")) { CTxDestination dest = DecodeDestination(options["changeAddress"].get_str()); @@ -3334,8 +3338,8 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f static UniValue fundrawtransaction(const JSONRPCRequest& request) { RPCHelpMan{"fundrawtransaction", - "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n" - "This will not modify existing inputs, and will add at most one change output to the outputs.\n" + "\nIf the transaction has no inputs, they will be automatically selected to meet its out value.\n" + "It will add at most one change output to the outputs.\n" "No existing outputs will be modified unless \"subtractFeeFromOutputs\" is specified.\n" "Note that inputs which were signed may need to be resigned after completion since in/outputs have been added.\n" "The inputs added will not be signed, use signrawtransactionwithkey\n" @@ -3349,6 +3353,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request) {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"}, {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}", { + {"add_inputs", RPCArg::Type::BOOL, /* default */ "true", "For a transaction with existing inputs, automatically include more if they are not enough."}, {"changeAddress", RPCArg::Type::STR, /* default */ "pool address", "The Dash address to receive the change"}, {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, {"includeWatching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n" @@ -3404,7 +3409,10 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request) CAmount fee; int change_position; - FundTransaction(pwallet, tx, fee, change_position, request.params[1]); + CCoinControl coin_control; + // Automatically select (additional) coins. Can be overriden by options.add_inputs. + coin_control.m_add_inputs = true; + FundTransaction(pwallet, tx, fee, change_position, request.params[1], coin_control); UniValue result(UniValue::VOBJ); result.pushKV("hex", EncodeHexTx(CTransaction(tx))); @@ -4066,10 +4074,10 @@ UniValue walletprocesspsbt(const JSONRPCRequest& request) UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) { RPCHelpMan{"walletcreatefundedpsbt", - "\nCreates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n" + "\nCreates and funds a transaction in the Partially Signed Transaction format.\n" "Implements the Creator and Updater roles.\n", { - {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs", + {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs. Leave empty to add inputs automatically. See add_inputs option.", { {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", { @@ -4100,6 +4108,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"}, {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "", { + {"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "If inputs are specified, automatically include more if they are not enough."}, {"changeAddress", RPCArg::Type::STR_HEX, /* default */ "pool address", "The Dash address to receive the change"}, {"changePosition", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, {"includeWatching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only"}, @@ -4150,7 +4159,11 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request) CAmount fee; int change_position; CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2]); - FundTransaction(pwallet, rawTx, fee, change_position, request.params[3]); + CCoinControl coin_control; + // Automatically select coins, unless at least one is manually selected. Can + // be overriden by options.add_inputs. + coin_control.m_add_inputs = rawTx.vin.size() == 0; + FundTransaction(pwallet, rawTx, fee, change_position, request.params[3], coin_control); // Make a blank psbt PartiallySignedTransaction psbtx{rawTx}; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6974dc3b4c..d56ac2adf7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2625,6 +2625,11 @@ void CWallet::AvailableCoins(std::vector &vCoins, bool fOnlySafe, const } if(!found) continue; + // Only consider selected coins if add_inputs is false + if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(wtxid, i))) { + continue; + } + if (pcoin->tx->vout[i].nValue < nMinimumAmount || pcoin->tx->vout[i].nValue > nMaximumAmount) continue; diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 7e32d1cba9..c68f1b2906 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -264,7 +264,11 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) + # Should fail without add_inputs: + assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + # add_inputs is enabled by default rawtxfund = self.nodes[2].fundrawtransaction(rawtx) + dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) totalOut = 0 matchingOuts = 0 @@ -292,7 +296,10 @@ class RawTransactionsTest(BitcoinTestFramework): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) + # Should fail without add_inputs: + assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) + dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) totalOut = 0 matchingOuts = 0 @@ -323,7 +330,10 @@ class RawTransactionsTest(BitcoinTestFramework): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx) + # Should fail without add_inputs: + assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) + dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) totalOut = 0 matchingOuts = 0 diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 188f9e7dd6..8bf22b0c0a 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -8,8 +8,8 @@ from decimal import Decimal from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( + assert_approx, assert_equal, - assert_greater_than, assert_raises_rpc_error, find_output ) @@ -31,6 +31,13 @@ class PSBTTest(BitcoinTestFramework): # Create and fund a raw tx for sending 10 DASH psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt'] + # If inputs are specified, do not automatically add more: + utxo1 = self.nodes[0].listunspent()[0] + assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[0].walletcreatefundedpsbt, [{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():900}) + + psbtx1 = self.nodes[0].walletcreatefundedpsbt([{"txid": utxo1['txid'], "vout": utxo1['vout']}], {self.nodes[2].getnewaddress():900}, 0, {"add_inputs": True})['psbt'] + assert_equal(len(self.nodes[0].decodepsbt(psbtx1)['tx']['vin']), 2) + # Node 1 should not be able to add anything to it but still return the psbtx same as before psbtx = self.nodes[1].walletprocesspsbt(psbtx1)['psbt'] assert_equal(psbtx1, psbtx) @@ -96,16 +103,16 @@ class PSBTTest(BitcoinTestFramework): self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex']) # feeRate of 0.1 DASH / KB produces a total fee slightly below -maxtxfee (~0.06650000): - res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}, 0, {"feeRate": 0.1}, False) - assert_greater_than(res["fee"], 0.03) - assert_greater_than(0.04, res["fee"]) + res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}, 0, {"feeRate": 0.1, "add_inputs": True}, False) + assert_approx(res["fee"], 0.04, 0.03) decoded_psbt = self.nodes[0].decodepsbt(res['psbt']) for psbt_in in decoded_psbt["inputs"]: assert "bip32_derivs" not in psbt_in # feeRate of 10 DASH / KB produces a total fee well above -maxtxfee # previously this was silently capped at -maxtxfee - assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}, 0, {"feeRate": 10}) + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():9.99}, 0, {"feeRate": 10, "add_inputs": True}) + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():1}, 0, {"feeRate": 10, "add_inputs": True}) # partially sign multisig things with node 1 psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2sh_pos}], outputs={self.nodes[1].getnewaddress():9.99}, options={'changeAddress': self.nodes[1].getrawchangeaddress()})['psbt'] @@ -185,7 +192,7 @@ class PSBTTest(BitcoinTestFramework): # Regression test for 14473 (mishandling of already-signed witness transaction): unspent = self.nodes[0].listunspent()[0] - psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}]) + psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], 0, {"add_inputs": True}) complete_psbt = self.nodes[0].walletprocesspsbt(psbtx_info["psbt"]) double_processed_psbt = self.nodes[0].walletprocesspsbt(complete_psbt["psbt"]) assert_equal(complete_psbt, double_processed_psbt)