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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
2613@@ -2613,7 +2614,7 @@ SigningResult CWallet::SignMessage(const std::string& message, const PKHash& pkh
2614 return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;
2615 }
2616
2617-bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
2618+bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, bilingual_str& error, bool includeUnsafe, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
includeUnsafe into CCoinControl instead.
CCoinControl would be a good place to store that bool. Do you prefer includeUnsafe (more consistent with the RPC) or fSafeOnly (more consistent with the inner codebase)?
2797@@ -2797,7 +2798,7 @@ bool CWallet::CreateTransactionInternal(
2798 txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight());
2799 {
2800 std::vector<COutput> vAvailableCoins;
2801- AvailableCoins(vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
2802+ AvailableCoins(vAvailableCoins, !coin_control.m_include_unsafe_inputs, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
We could remove the fOnlySafe argument to AvailableCoins now that this is provided by coin_control.
The only gotcha is that AvailableCoins can be called without a coin_control, but I checked the codebase and in all the places where that happens fOnlySafe is set to true, so we could set it inside AvailableCoins to const bool only_safe = {coinControl ? !coinControl->m_include_unsafe_inputs : true};.
Do you want me to apply that change in this PR?
Is there something I can do to help this PR move forward?
We really need it to unblock anchor outputs in lightning (we can run a modified bitcoind with this patch applied for our own node, but we can’t ask our users to run a non-release bitcoind version).
Looks good to me.
Could there go anything other wrong than a transaction becoming invalid because an input disappeared? (this of course would not be really wrong)
Could there go anything other wrong than a transaction becoming invalid because an input disappeared?
It’s a very good question, it’s part of my “unknown unknowns” and why I’m curious to get feedback from more people here. It seems to me that there isn’t anything else, but there are many things I don’t know so I wouldn’t rely on my instincts only ¯_(ツ)_/¯
928@@ -928,6 +929,29 @@ def test_transaction_too_large(self):
929 self.nodes[0].generate(10)
930 assert_raises_rpc_error(-4, "Transaction too large", recipient.fundrawtransaction, rawtx)
931
932+ def test_include_unsafe(self):
FundTransaction() has other callers like the walletcreatefundedpsbt and send RPCs, might be a good idea to have test_include_unsafe tests that guard against present/future regressions in rpc_test_psbt.py and wallet_send.py.
3207@@ -3203,6 +3208,9 @@ static RPCHelpMan fundrawtransaction()
3208 {"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}",
3209 {
3210 {"add_inputs", RPCArg::Type::BOOL, /* default */ "true", "For a transaction with existing inputs, automatically include more if they are not enough."},
3211+ {"include_unsafe", RPCArg::Type::BOOL, /* default */ "false", "Include inputs that are not safe to spend.\n"
3212+ "Beware that the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
This would need a release note.
Added in https://github.com/bitcoin/bitcoin/pull/21359/commits/c3eb1994c24167bedb114e34b53dc7cbeb9ff403, let me know if that’s how it should be done.
138@@ -133,6 +139,11 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
139 assert not "txid" in res
140 assert "psbt" in res
141
142+ if include_unsafe:
143+ from_balance = from_wallet.getbalances()["mine"]["untrusted_pending"]
144+ else:
145+ from_balance = from_wallet.getbalance()
will review soon, the release note LGTM
0 from_balance = from_wallet.getbalances()["mine"]["trusted"]
as IIRC we are moving away from getbalance in favor of getbalances… so it’s good that you have abstracted it out to this one place (edit: it’s not just one place, but could make it one in a function)
should from_balance when include_unsafe is true not also include “trusted”?
getbalance() to replace them with getbalances()["mine"]["trusted"] in a separate PR, don’t you think?
I volunteer to make that follow-up PR right after this one if that sounds good to you.
3207@@ -3203,6 +3208,9 @@ static RPCHelpMan fundrawtransaction()
3208 {"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}",
3209 {
3210 {"add_inputs", RPCArg::Type::BOOL, /* default */ "true", "For a transaction with existing inputs, automatically include more if they are not enough."},
3211+ {"include_unsafe", RPCArg::Type::BOOL, /* default */ "false", "Include inputs that are not safe to spend.\n"
safe flag in https://github.com/bitcoin/bitcoin/pull/21359/commits/1bb252669dc5e8feb41e4975d609108eb28c8667, let me know if you think the wording should be updated to be clearer.
398+ self.nodes[2].createwallet(wallet_name="unsafe")
399+ wunsafe = self.nodes[2].get_wallet_rpc("unsafe")
400+ self.nodes[0].sendtoaddress(wunsafe.getnewaddress(), 2)
401+ self.sync_mempools()
402+ assert_raises_rpc_error(-4, "Insufficient funds", wunsafe.walletcreatefundedpsbt, [], [{self.nodes[0].getnewaddress():1}])
403+ wunsafe.walletcreatefundedpsbt([], [{self.nodes[0].getnewaddress():1}], 0, {"include_unsafe": True})
style nit if you need to retouch (same on previous line)
0 wunsafe.walletcreatefundedpsbt([], [{self.nodes[0].getnewaddress(): 1}], 0, {"include_unsafe": True})
41
42- from_balance_before = from_wallet.getbalance()
43+ if include_unsafe:
44+ from_balance_before = from_wallet.getbalances()["mine"]["untrusted_pending"]
45+ else:
46+ from_balance_before = from_wallet.getbalances()["mine"]["trusted"]
While this works for the purposes of the test, should from_balance_before always include “trusted”:
0 from_balance_before = from_wallet.getbalances()["mine"]["trusted"]
1 if include_unsafe:
2 from_balance_before += from_wallet.getbalances()["mine"]["untrusted_pending"]
138@@ -133,6 +139,11 @@ def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
139 assert not "txid" in res
140 assert "psbt" in res
141
142+ if include_unsafe:
143+ from_balance = from_wallet.getbalances()["mine"]["untrusted_pending"]
144+ else:
145+ from_balance = from_wallet.getbalances()["mine"]["trusted"]
same here
0 from_balance = from_wallet.getbalances()["mine"]["trusted"]
1 if include_unsafe:
2 from_balance += from_wallet.getbalances()["mine"]["untrusted_pending"]
ACK 4e4d7c01b4134ea3b920345ee2a62e2b06cd458e
Some comments below.
2533@@ -2535,6 +2534,13 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2534 vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
2535 return true;
2536 }
2537+ // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs
2538+ // received from other wallets.
2539+ if (coin_control.m_include_unsafe_inputs && SelectCoinsMinConf(value_to_select,
2540+ CoinEligibilityFilter(0, 0, max_ancestors-1, max_descendants-1, true /* include_partial_groups */),
2541+ vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
nit, I think it’s best to not indent arguments the same as the function they are a part of. Maybe something like
0- if (coin_control.m_include_unsafe_inputs && SelectCoinsMinConf(value_to_select,
1- CoinEligibilityFilter(0, 0, max_ancestors-1, max_descendants-1, true /* include_partial_groups */),
2- vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
3+ if (coin_control.m_include_unsafe_inputs
4+ && SelectCoinsMinConf(
5+ value_to_select,
6+ CoinEligibilityFilter(0, 0, max_ancestors-1, max_descendants-1, true /* include_partial_groups */),
7+ vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
8 return true;
ACK a5c3d9cc4cafef3434c97656ab0d4b4fe4727d51
Happy to re-ack for #21359 (review) and #21359 (review).
2533@@ -2535,6 +2534,14 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2534 vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
2535 return true;
2536 }
2537+ // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs
2538+ // received from other wallets.
2539+ if (coin_control.m_include_unsafe_inputs
linter appeasement time (sorry about that if it’s from my suggestion)
0 if (coin_control.m_include_unsafe_inputs
948+ # But we can opt-in to use them for funding.
949+ fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True})
950+ tx_dec = wallet.decoderawtransaction(fundedtx['hex'])
951+ assert any([txin['txid'] == txid and txin['vout'] == vout for txin in tx_dec['vin']])
952+ signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex'])
953+ wallet.sendrawtransaction(signedtx['hex'])
3209@@ -3205,6 +3210,9 @@ static RPCHelpMan fundrawtransaction()
3210 {"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}",
3211 {
3212 {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{true}, "For a transaction with existing inputs, automatically include more if they are not enough."},
3213+ {"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"
fSafe here: https://github.com/bitcoin/bitcoin/blob/2b45cf0bcdb3d2c1de46899e30885c953b57b475/src/wallet/wallet.h#L595
Is there consensus on updating this in every place where it’s used?
2533@@ -2535,6 +2534,14 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
2534 vCoins, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) {
2535 return true;
2536 }
2537+ // Try with unsafe inputs if they are allowed. This may spend unconfirmed outputs
2538+ // received from other wallets.
2539+ if (coin_control.m_include_unsafe_inputs
2540+ && SelectCoinsMinConf(
2541+ value_to_select, CoinEligibilityFilter(0, 0, max_ancestors-1, max_descendants-1, true /* include_partial_groups */),
Have a look on CoinEligibilityFilter, it says “We never spend of unconfirmed foreign outputs as we cannot rely on it on these funds yet”. I don’t think it’s true anymore ?
Further, maybe add in-line comments about CoinEligibilityFilter parameters “/* conf_mine / 0, / conf_theirs */ 1”, good for readabilty
4037@@ -4030,6 +4038,9 @@ static RPCHelpMan send()
4038 {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
4039 {
4040 {"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{false}, "If inputs are specified, automatically include more if they are not enough."},
4041+ {"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"
isRBFOptIndoesn’t seem used in FundTransaction path. Unsafe inputs definition doesn’t seem to be tied to replaceability only key externality ?
FundTransaction doesn’t check isRBFOptIn, which means that the wallet may currently use unconfirmed outputs from replaceable txs if they’re our own txs (which could be an issue because we could replace that tx and invalidate its children)?
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
