foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-08 10:31:50 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#21359: rpc: include_unsafe option for fundrawtransaction

Browse source 
11d6459b6e rpc: include_unsafe option for fundrawtransaction (t-bast)

Pull request description:

  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.

  I also added this option to `send` and `walletcreatefundedpsbt` who internally delegate to `fundrawtransaction`.

  Fixes #21299

ACKs for top commit:
  laanwj:
    Code review ACK 11d6459b6e

Tree-SHA512: 5e542a4febcfd6f41cf784678ff02ec9282eae2082c274983f72c5ea87b7ebbe1bd5fdc6a020d7a9d5996157754eb4966b8aeb6c1ceebf0b1519f735579b8bac
This commit is contained in:
W. J. van der Laan 2021-05-10 15:55:22 +02:00
commit 32692d2681
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
8 changed files with 97 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
doc/release-notes.md
View file

@ -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
-----------

2
src/wallet/coincontrol.h
View file

@ -29,6 +29,8 @@ public:
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

3
src/wallet/coinselection.h
View file

@ -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;

14
src/wallet/rpcwallet.cpp
View file

@ -3076,6 +3076,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)},
@ -3136,6 +3137,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)");
@ -3206,6 +3211,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\"."},
@ -4031,6 +4039,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"},
@ -4374,6 +4385,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\"."},

13
src/wallet/wallet.cpp
View file

@ -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)),
@ -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.
@ -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;

35
test/functional/rpc_fundrawtransaction.py
View file

@ -98,6 +98,7 @@ class RawTransactionsTest(BitcoinTestFramework):
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."""
@ -934,6 +935,40 @@ class RawTransactionsTest(BitcoinTestFramework):
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
test/functional/rpc_psbt.py
View file

@ -402,6 +402,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

27
test/functional/wallet_send.py
View file

@ -33,12 +33,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, 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:
@ -71,6 +74,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:
@ -133,6 +138,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"])
@ -143,13 +152,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()
@ -459,6 +468,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()