rpc: include_unsafe option for fundrawtransaction #21359

t-bast · 2021-03-04T10:05:40Z

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

DrahtBot · 2021-03-04T15:49:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

MOVEONLY: CWallet transaction code out of wallet.cpp/.h #21207 (MOVEONLY: CWallet transaction code out of wallet.cpp/.h by ryanofsky)
refactor: Make CWalletTx sync state type-safe #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

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.

src/wallet/wallet.cpp

t-bast · 2021-03-10T14:41:47Z

src/wallet/wallet.cpp

@@ -2797,7 +2798,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);


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?

t-bast · 2021-03-17T18:14:18Z

@achow101 @luke-jr would you have some time to give a quick pass to this hopefully very simple PR and let me know if there's a concept ACK and it's worth pursuing? 🙏

I know, Taproot is happening (is it?), I understand if you don't have much time available 😄

achow101 · 2021-03-17T22:24:02Z

ACK 16076bb

t-bast · 2021-03-25T11:03:50Z

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).

renepickhardt · 2021-03-29T08:36:23Z

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)

t-bast · 2021-03-29T08:59:12Z

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 ¯_(ツ)_/¯

test/functional/rpc_fundrawtransaction.py

jonatack

ACK 4e4d7c0

Some comments below.

test/functional/rpc_psbt.py

test/functional/wallet_send.py

jonatack · 2021-04-26T13:29:54Z

ACK 0709545

jonatack

ACK a5c3d9c

Happy to re-ack for #21359 (comment) and #21359 (comment).

src/wallet/wallet.cpp

jonatack · 2021-04-30T09:42:03Z

re-ACK 1b1dad3

test/functional/rpc_fundrawtransaction.py

ariard · 2021-04-30T14:21:32Z

src/wallet/rpcwallet.cpp

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


I think "opt-in RBF" is better than "replacement" to qualify transactions which may be evicted of the mempool due to bip 125 rules.

It took the definition of fSafe here:

bitcoin/src/wallet/wallet.h

Line 595 in 2b45cf0

bool fSafe;

Is there consensus on updating this in every place where it's used?

src/wallet/wallet.cpp

ariard · 2021-04-30T15:07:32Z

src/wallet/rpcwallet.cpp

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


Did we already consider for spending replacement transactions from our wallets before this PR ? isRBFOptIndoesn't seem used in FundTransaction path. Unsafe inputs definition doesn't seem to be tied to replaceability only key externality ?

I'm not 100% I understand your comment. Are you saying that 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 bitcoin#21299

DrahtBot · 2021-05-03T09:33:36Z

🕵️ @harding has been requested to review this pull request as specified in the REVIEWERS file.

laanwj · 2021-05-10T14:01:30Z

Code review ACK 11d6459

The fOnlySafe argument to AvailableCoins is now redundant, since bitcoin#21359 added a similar field inside the CCoinControl struct. Not all code paths set a CCoinControl instance, but when it's missing we can default to using only safe inputs which is backwards-compatible.

c30dd02 refactor: remove redundant fOnlySafe argument (t-bast) Pull request description: The `fOnlySafe` argument to `AvailableCoins` is now redundant, since #21359 added a similar field inside the `CCoinControl` struct (see #21359 (comment)). Not all code paths create a `CCoinControl` instance, but when it's missing we can default to using only safe inputs which is backwards-compatible. ACKs for top commit: instagibbs: utACK c30dd02 promag: Code review ACK c30dd02. achow101: ACK c30dd02 meshcollider: Code review + test run ACK c30dd02 Tree-SHA512: af3cb598d06f233fc48a7c9c45bb14da92b5cf4168b8dbd4f134dc3e0c2b615c6590238ddb1eaf380aea5bbdd3386d2ac8ecd7d22dfc93579adc39248542839b

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 bitcoin#21299 Github-Pull: bitcoin#21359 Rebased-From: 11d6459

I hacked up rpc_fundrawtransaction.py a little bit because our `get_address` method here depends on nobody having called `createwallet` on a particular node ... the "correct" fix for this would be to redo the Elements changes to this functional test for the 0.20+ createwallet changes, but because there will be big future changes to this test (in Elements #900 (PSET)) I did the quick/hacky thing, and will defer cleanups to a separate post-rebase PR.

fanquake added the RPC/REST/ZMQ label Mar 4, 2021

DrahtBot added the Needs rebase label Mar 8, 2021

DrahtBot mentioned this pull request Mar 8, 2021

refactor: split CWallet::Create #20773

Merged

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 54bd3a1 to 92e5b3a Compare March 9, 2021 13:16

DrahtBot removed the Needs rebase label Mar 9, 2021

luke-jr reviewed Mar 9, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Mar 10, 2021

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 92e5b3a to 0dc8808 Compare March 10, 2021 14:37

t-bast commented Mar 10, 2021

View reviewed changes

DrahtBot removed the Needs rebase label Mar 10, 2021

DrahtBot mentioned this pull request Mar 11, 2021

test: increase rpc_timeout for fundrawtx test_transaction_too_large #21410

Merged

DrahtBot added the Needs rebase label Mar 16, 2021

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 0dc8808 to 16076bb Compare March 17, 2021 18:01

DrahtBot removed the Needs rebase label Mar 17, 2021

jonatack reviewed Mar 29, 2021

View reviewed changes

test/functional/rpc_fundrawtransaction.py Show resolved Hide resolved

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 1bb2526 to 4e4d7c0 Compare April 26, 2021 10:20

jonatack reviewed Apr 26, 2021

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

test/functional/wallet_send.py Outdated Show resolved Hide resolved

test/functional/wallet_send.py Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Apr 26, 2021

DrahtBot added the Needs rebase label Apr 29, 2021

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 0709545 to a5c3d9c Compare April 30, 2021 07:23

jonatack reviewed Apr 30, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from a5c3d9c to 2bf0b1e Compare April 30, 2021 08:16

DrahtBot removed the Needs rebase label Apr 30, 2021

jonatack reviewed Apr 30, 2021

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from ce70f32 to 1b1dad3 Compare April 30, 2021 09:33

maflcko requested a review from instagibbs April 30, 2021 11:19

ariard reviewed Apr 30, 2021

View reviewed changes

t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 1b1dad3 to 11d6459 Compare April 30, 2021 16:53

laanwj merged commit 32692d2 into bitcoin:master May 10, 2021

laanwj removed this from Blockers in High-priority for review May 10, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2021

Merge bitcoin#21359: rpc: include_unsafe option for fundrawtransaction

5f93fe8

t-bast mentioned this pull request May 11, 2021

refactor: remove redundant fOnlySafe argument #21910

Merged

t-bast deleted the fund-raw-transaction-allow-unsafe branch May 13, 2021 18:12

t-bast mentioned this pull request Jan 27, 2022

Add support for funding txs with unsafe inputs ACINQ/eclair#2150

Closed

gwillen mentioned this pull request Apr 6, 2022

Bring Elements up to date with Bitcoin Core 22.0 ElementsProject/elements#1058

Closed

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: include_unsafe option for fundrawtransaction #21359

rpc: include_unsafe option for fundrawtransaction #21359

t-bast commented Mar 4, 2021 •

edited

DrahtBot commented Mar 4, 2021 •

edited

t-bast Mar 10, 2021

t-bast commented Mar 17, 2021

achow101 commented Mar 17, 2021

t-bast commented Mar 25, 2021

renepickhardt commented Mar 29, 2021

t-bast commented Mar 29, 2021

jonatack left a comment

jonatack commented Apr 26, 2021

jonatack left a comment •

edited

jonatack commented Apr 30, 2021

ariard Apr 30, 2021

t-bast Apr 30, 2021

ariard Apr 30, 2021

t-bast Apr 30, 2021

DrahtBot commented May 3, 2021

laanwj commented May 10, 2021

Navigation Menu

rpc: include_unsafe option for fundrawtransaction #21359

rpc: include_unsafe option for fundrawtransaction #21359

Conversation

t-bast commented Mar 4, 2021 • edited

DrahtBot commented Mar 4, 2021 • edited

Conflicts

t-bast Mar 10, 2021

Choose a reason for hiding this comment

t-bast commented Mar 17, 2021

achow101 commented Mar 17, 2021

t-bast commented Mar 25, 2021

renepickhardt commented Mar 29, 2021

t-bast commented Mar 29, 2021

jonatack left a comment

Choose a reason for hiding this comment

jonatack commented Apr 26, 2021

jonatack left a comment • edited

Choose a reason for hiding this comment

jonatack commented Apr 30, 2021

ariard Apr 30, 2021

Choose a reason for hiding this comment

t-bast Apr 30, 2021

Choose a reason for hiding this comment

ariard Apr 30, 2021

Choose a reason for hiding this comment

t-bast Apr 30, 2021

Choose a reason for hiding this comment

DrahtBot commented May 3, 2021

laanwj commented May 10, 2021

t-bast commented Mar 4, 2021 •

edited

DrahtBot commented Mar 4, 2021 •

edited

jonatack left a comment •

edited