From 69c5aa89479af20c2bdb0c02191617140efdae5e Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 23 Jun 2024 12:37:45 +0000 Subject: [PATCH] merge bitcoin#21359: include_unsafe option for fundrawtransaction --- doc/release-notes-21359.md | 7 +++++ src/wallet/coincontrol.h | 2 ++ src/wallet/coinselection.h | 3 +- src/wallet/rpcwallet.cpp | 17 +++++++++-- src/wallet/wallet.cpp | 13 ++++++-- test/functional/rpc_fundrawtransaction.py | 36 +++++++++++++++++++++++ test/functional/rpc_psbt.py | 8 +++++ test/functional/wallet_send.py | 27 +++++++++++++---- 8 files changed, 101 insertions(+), 12 deletions(-) create mode 100644 doc/release-notes-21359.md diff --git a/doc/release-notes-21359.md b/doc/release-notes-21359.md new file mode 100644 index 0000000000..f86c72f5c4 --- /dev/null +++ b/doc/release-notes-21359.md @@ -0,0 +1,7 @@ +Wallet +------ + +- The `fundrawtransaction`, `send` and `walletcreatefundedpsbt` RPCs now support an `include_unsafe` option + that when `true` allows using unsafe inputs to fund the transaction. + Note that the resulting transaction may become invalid if one of the unsafe inputs disappears. + If that happens, the transaction must be funded with different inputs and republished. diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index aa8e2440ed..b7a71fe4b7 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -40,6 +40,8 @@ public: CTxDestination destChange = CNoDestination(); //! If false, only selected inputs are used bool m_add_inputs = true; + //! If false, only safe inputs will be used + bool m_include_unsafe_inputs = false; //! If false, allows unselected inputs, but requires all selected inputs be used if fAllowOtherInputs is true (default) bool fAllowOtherInputs = false; //! 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/coinselection.h b/src/wallet/coinselection.h index 6070a43b8e..0734927ad2 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -65,8 +65,7 @@ struct CoinEligibilityFilter /** Minimum number of confirmations for outputs that we sent to ourselves. * We may use unconfirmed UTXOs sent from ourselves, e.g. change outputs. */ const int conf_mine; - /** Minimum number of confirmations for outputs received from a different - * wallet. We never spend unconfirmed foreign outputs as we cannot rely on these funds yet. */ + /** Minimum number of confirmations for outputs received from a different wallet. */ const int conf_theirs; /** Maximum number of unconfirmed ancestors aggregated across all UTXOs in an OutputGroup. */ const uint64_t max_ancestors; diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 22c7d48e72..8e1aee2b36 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3418,6 +3418,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f RPCTypeCheckObj(options, { {"add_inputs", UniValueType(UniValue::VBOOL)}, + {"include_unsafe", UniValueType(UniValue::VBOOL)}, {"add_to_wallet", UniValueType(UniValue::VBOOL)}, {"changeAddress", UniValueType(UniValue::VSTR)}, {"change_address", UniValueType(UniValue::VSTR)}, @@ -3464,8 +3465,11 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f lockUnspents = (options.exists("lock_unspents") ? options["lock_unspents"] : options["lockUnspents"]).get_bool(); } - if (options.exists("feeRate")) - { + if (options.exists("include_unsafe")) { + coinControl.m_include_unsafe_inputs = options["include_unsafe"].get_bool(); + } + + if (options.exists("feeRate")) { if (options.exists("conf_target")) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both conf_target and feeRate"); } @@ -3530,6 +3534,9 @@ static RPCHelpMan fundrawtransaction() {"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."}, + {"include_unsafe", RPCArg::Type::BOOL, /* default */ "false", "Include inputs that are not safe to spend (unconfirmed transactions from outside keys and unconfirmed replacement transactions).\n" + "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n" + "If that happens, you will need to fund the transaction with different inputs and republish it."}, {"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" @@ -4199,6 +4206,9 @@ static RPCHelpMan send() {"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."}, + {"include_unsafe", RPCArg::Type::BOOL, /* default */ "false", "Include inputs that are not safe to spend (unconfirmed transactions from outside keys and unconfirmed replacement transactions).\n" + "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n" + "If that happens, you will need to fund the transaction with different inputs and republish it."}, {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"}, {"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The Dash address to receive the change"}, {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"}, @@ -4533,6 +4543,9 @@ static RPCHelpMan walletcreatefundedpsbt() {"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."}, + {"include_unsafe", RPCArg::Type::BOOL, /* default */ "false", "Include inputs that are not safe to spend (unconfirmed transactions from outside keys and unconfirmed replacement transactions).\n" + "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n" + "If that happens, you will need to fund the transaction with different inputs and republish it."}, {"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"}, diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7ca352bf9c..e8237c777e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2932,8 +2932,7 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; // Fall back to using zero confirmation change (but with as few ancestors in the mempool as - // possible) if we cannot fund the transaction otherwise. We never spend unconfirmed - // outputs received from other wallets. + // possible) if we cannot fund the transaction otherwise. if (m_spend_zero_conf_change) { if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) return true; if (SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), @@ -2951,6 +2950,14 @@ bool CWallet::SelectCoins(const std::vector& vAvailableCoins, const CAm vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used, nCoinType)) { return true; } + // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs + // received from other wallets. + if (coin_control.m_include_unsafe_inputs + && SelectCoinsMinConf(value_to_select, + CoinEligibilityFilter(0 /* conf_mine */, 0 /* conf_theirs */, max_ancestors-1, max_descendants-1, true /* include_partial_groups */), + vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) { + return true; + } // Try with unlimited ancestors/descendants. The transaction will still need to meet // mempool ancestor/descendant policy to be accepted to mempool and broadcasted, but // OutputGroups use heuristics that may overestimate ancestor/descendant counts. @@ -3473,7 +3480,7 @@ bool CWallet::CreateTransactionInternal( { CAmount nAmountAvailable{0}; std::vector vAvailableCoins; - AvailableCoins(vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); + AvailableCoins(vAvailableCoins, !coin_control.m_include_unsafe_inputs, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy coin_selection_params.use_bnb = false; // never use BnB diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 43a7b2f3c1..300f3d7fcf 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -97,6 +97,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.test_address_reuse() self.test_option_subtract_fee_from_outputs() self.test_subtract_fee_with_presets() + self.test_include_unsafe() def test_change_position(self): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" @@ -815,5 +816,40 @@ class RawTransactionsTest(BitcoinTestFramework): signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex']) self.nodes[0].sendrawtransaction(signedtx['hex']) + def test_include_unsafe(self): + self.log.info("Test fundrawtxn with unsafe inputs") + + self.nodes[0].createwallet("unsafe") + wallet = self.nodes[0].get_wallet_rpc("unsafe") + + # We receive unconfirmed funds from external keys (unsafe outputs). + addr = wallet.getnewaddress() + txid1 = self.nodes[2].sendtoaddress(addr, 6) + txid2 = self.nodes[2].sendtoaddress(addr, 4) + self.sync_all() + vout1 = find_vout_for_address(wallet, txid1, addr) + vout2 = find_vout_for_address(wallet, txid2, addr) + + # Unsafe inputs are ignored by default. + rawtx = wallet.createrawtransaction([], [{self.nodes[2].getnewaddress(): 5}]) + assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, rawtx) + + # But we can opt-in to use them for funding. + fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True}) + tx_dec = wallet.decoderawtransaction(fundedtx['hex']) + assert any([txin['txid'] == txid1 and txin['vout'] == vout1 for txin in tx_dec['vin']]) + signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) + wallet.sendrawtransaction(signedtx['hex']) + + # And we can also use them once they're confirmed. + self.nodes[0].generate(1) + rawtx = wallet.createrawtransaction([], [{self.nodes[2].getnewaddress(): 3}]) + fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True}) + tx_dec = wallet.decoderawtransaction(fundedtx['hex']) + assert any([txin['txid'] == txid2 and txin['vout'] == vout2 for txin in tx_dec['vin']]) + signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) + wallet.sendrawtransaction(signedtx['hex']) + + if __name__ == '__main__': RawTransactionsTest().main() diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 2cf63da9a6..cb5bd668a8 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -202,6 +202,14 @@ class PSBTTest(BitcoinTestFramework): # We don't care about the decode result, but decoding must succeed. self.nodes[0].decodepsbt(double_processed_psbt["psbt"]) + # Make sure unsafe inputs are included if specified + self.nodes[2].createwallet(wallet_name="unsafe") + wunsafe = self.nodes[2].get_wallet_rpc("unsafe") + self.nodes[0].sendtoaddress(wunsafe.getnewaddress(), 2) + self.sync_mempools() + assert_raises_rpc_error(-4, "Insufficient funds", wunsafe.walletcreatefundedpsbt, [], [{self.nodes[0].getnewaddress(): 1}]) + wunsafe.walletcreatefundedpsbt([], [{self.nodes[0].getnewaddress(): 1}], 0, {"include_unsafe": True}) + # BIP 174 Test Vectors # Check that unknown values are just passed through diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py index f9e9c1e5a4..78ffc4bb33 100755 --- a/test/functional/wallet_send.py +++ b/test/functional/wallet_send.py @@ -31,12 +31,15 @@ class WalletSendTest(BitcoinTestFramework): def test_send(self, from_wallet, to_wallet=None, amount=None, data=None, arg_conf_target=None, arg_estimate_mode=None, conf_target=None, estimate_mode=None, add_to_wallet=None, psbt=None, - inputs=None, add_inputs=None, change_address=None, change_position=None, + inputs=None, add_inputs=None, include_unsafe=None, change_address=None, change_position=None, include_watching=None, locktime=None, lock_unspents=None, subtract_fee_from_outputs=None, expect_error=None): assert (amount is None) != (data is None) - from_balance_before = from_wallet.getbalance() + from_balance_before = from_wallet.getbalances()["mine"]["trusted"] + if include_unsafe: + from_balance_before += from_wallet.getbalances()["mine"]["untrusted_pending"] + if to_wallet is None: assert amount is None else: @@ -67,6 +70,8 @@ class WalletSendTest(BitcoinTestFramework): options["inputs"] = inputs if add_inputs is not None: options["add_inputs"] = add_inputs + if include_unsafe is not None: + options["include_unsafe"] = include_unsafe if change_address is not None: options["change_address"] = change_address if change_position is not None: @@ -120,6 +125,10 @@ class WalletSendTest(BitcoinTestFramework): assert not "txid" in res assert "psbt" in res + from_balance = from_wallet.getbalances()["mine"]["trusted"] + if include_unsafe: + from_balance += from_wallet.getbalances()["mine"]["untrusted_pending"] + if add_to_wallet and not include_watching: # Ensure transaction exists in the wallet: tx = from_wallet.gettransaction(res["txid"]) @@ -129,13 +138,13 @@ class WalletSendTest(BitcoinTestFramework): assert tx if amount: if subtract_fee_from_outputs: - assert_equal(from_balance_before - from_wallet.getbalance(), amount) + assert_equal(from_balance_before - from_balance, amount) else: - assert_greater_than(from_balance_before - from_wallet.getbalance(), amount) + assert_greater_than(from_balance_before - from_balance, amount) else: assert next((out for out in tx["vout"] if out["scriptPubKey"]["asm"] == "OP_RETURN 35"), None) else: - assert_equal(from_balance_before, from_wallet.getbalance()) + assert_equal(from_balance_before, from_balance) if to_wallet: self.sync_mempools() @@ -367,6 +376,14 @@ class WalletSendTest(BitcoinTestFramework): self.log.info("Subtract fee from output") self.test_send(from_wallet=w0, to_wallet=w1, amount=1, subtract_fee_from_outputs=[0]) + self.log.info("Include unsafe inputs") + self.nodes[1].createwallet(wallet_name="w5") + w5 = self.nodes[1].get_wallet_rpc("w5") + self.test_send(from_wallet=w0, to_wallet=w5, amount=2) + self.test_send(from_wallet=w5, to_wallet=w0, amount=1, expect_error=(-4, "Insufficient funds")) + res = self.test_send(from_wallet=w5, to_wallet=w0, amount=1, include_unsafe=True) + assert res["complete"] + if __name__ == '__main__': WalletSendTest().main()