Skip to content

Commit

Permalink
rpc: include_unsafe option for fundrawtransaction
Browse files Browse the repository at this point in the history
Allow RPC users to opt-in to unsafe inputs when funding a raw transaction.

Applications that need to manage a complex RBF flow (such as lightning
nodes using anchor outputs) are very limited if they can only use safe inputs.

Fixes #21299
  • Loading branch information
t-bast committed Apr 30, 2021
1 parent 480bf01 commit 11d6459
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 10 deletions.
5 changes: 5 additions & 0 deletions doc/release-notes.md
Expand Up @@ -152,6 +152,11 @@ Wallet
- The `bumpfee` RPC is not available with wallets that have private keys
disabled. `psbtbumpfee` can be used instead. (#20891)

- 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. (#21359)

GUI changes
-----------

Expand Down
2 changes: 2 additions & 0 deletions src/wallet/coincontrol.h
Expand Up @@ -29,6 +29,8 @@ class CCoinControl
std::optional<OutputType> m_change_type;
//! 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
bool fAllowOtherInputs = false;
//! Includes watch only addresses which are solvable
Expand Down
3 changes: 1 addition & 2 deletions src/wallet/coinselection.h
Expand Up @@ -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;
Expand Down
14 changes: 14 additions & 0 deletions src/wallet/rpcwallet.cpp
Expand Up @@ -3075,6 +3075,7 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
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)},
Expand Down Expand Up @@ -3135,6 +3136,10 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
lockUnspents = (options.exists("lock_unspents") ? options["lock_unspents"] : options["lockUnspents"]).get_bool();
}

if (options.exists("include_unsafe")) {
coinControl.m_include_unsafe_inputs = options["include_unsafe"].get_bool();
}

if (options.exists("feeRate")) {
if (options.exists("fee_rate")) {
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot specify both fee_rate (" + CURRENCY_ATOM + "/vB) and feeRate (" + CURRENCY_UNIT + "/kvB)");
Expand Down Expand Up @@ -3205,6 +3210,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, RPCArg::Default{true}, "For a transaction with existing inputs, automatically include more if they are not enough."},
{"include_unsafe", RPCArg::Type::BOOL, RPCArg::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, RPCArg::DefaultHint{"pool address"}, "The bitcoin address to receive the change"},
{"changePosition", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"},
{"change_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -changetype"}, "The output type to use. Only valid if changeAddress is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
Expand Down Expand Up @@ -4030,6 +4038,9 @@ static RPCHelpMan send()
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
{
{"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."},
{"include_unsafe", RPCArg::Type::BOOL, RPCArg::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, RPCArg::Default{true}, "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},
{"change_address", RPCArg::Type::STR_HEX, RPCArg::DefaultHint{"pool address"}, "The bitcoin address to receive the change"},
{"change_position", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"},
Expand Down Expand Up @@ -4373,6 +4384,9 @@ static RPCHelpMan walletcreatefundedpsbt()
{"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
{
{"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."},
{"include_unsafe", RPCArg::Type::BOOL, RPCArg::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, RPCArg::DefaultHint{"pool address"}, "The bitcoin address to receive the change"},
{"changePosition", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"},
{"change_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -changetype"}, "The output type to use. Only valid if changeAddress is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
Expand Down
13 changes: 10 additions & 3 deletions src/wallet/wallet.cpp
Expand Up @@ -2516,8 +2516,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)) 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)) 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)),
Expand All @@ -2535,6 +2534,14 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
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.
Expand Down Expand Up @@ -2836,7 +2843,7 @@ bool CWallet::CreateTransactionInternal(
txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight());
{
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.m_avoid_partial_spends = coin_control.m_avoid_partial_spends;

Expand Down
35 changes: 35 additions & 0 deletions test/functional/rpc_fundrawtransaction.py
Expand Up @@ -96,6 +96,7 @@ def run_test(self):
self.test_option_subtract_fee_from_outputs()
self.test_subtract_fee_with_presets()
self.test_transaction_too_large()
self.test_include_unsafe()

def test_change_position(self):
"""Ensure setting changePosition in fundraw with an exact match is handled properly."""
Expand Down Expand Up @@ -928,6 +929,40 @@ def test_transaction_too_large(self):
self.nodes[0].generate(10)
assert_raises_rpc_error(-4, "Transaction too large", recipient.fundrawtransaction, rawtx)

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()
8 changes: 8 additions & 0 deletions test/functional/rpc_psbt.py
Expand Up @@ -394,6 +394,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
Expand Down
27 changes: 22 additions & 5 deletions test/functional/wallet_send.py
Expand Up @@ -33,12 +33,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, arg_fee_rate=None,
conf_target=None, estimate_mode=None, fee_rate=None, add_to_wallet=None, psbt=None,
inputs=None, add_inputs=None, change_address=None, change_position=None, change_type=None,
inputs=None, add_inputs=None, include_unsafe=None, change_address=None, change_position=None, change_type=None,
include_watching=None, locktime=None, lock_unspents=None, replaceable=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:
Expand Down Expand Up @@ -71,6 +74,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:
Expand Down Expand Up @@ -133,6 +138,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"])
Expand All @@ -143,13 +152,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()
Expand Down Expand Up @@ -440,6 +449,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()

0 comments on commit 11d6459

Please sign in to comment.