merge bitcoin#21359: include_unsafe option for fundrawtransaction

Author: kwvg

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.

src/wallet/coincontrol.h

@@ -40,6 +40,8 @@ class CCoinControl
     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

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;

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"},

src/wallet/wallet.cpp

@@ -2932,8 +2932,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& 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<COutput>& 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<COutput> 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
 

test/functional/rpc_fundrawtransaction.py

@@ -97,6 +97,7 @@ def run_test(self):
         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 @@ def test_subtract_fee_with_presets(self):
         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()

test/functional/rpc_psbt.py

@@ -202,6 +202,14 @@ def run_test(self):
         # 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

test/functional/wallet_send.py

@@ -31,12 +31,15 @@ def skip_test_if_missing_module(self):
     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 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
             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 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
             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 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
             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 @@ def run_test(self):
         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()