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

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 16076bb2a8517c6a8b101ad04e220ad3b3f71bcf

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 4e4d7c01b4134ea3b920345ee2a62e2b06cd458e

Some comments below.

test/functional/rpc_psbt.py

test/functional/wallet_send.py

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

ACK 0709545365d045b24563b4683881f65a2b34afdd

jonatack

ACK a5c3d9cc4cafef3434c97656ab0d4b4fe4727d51

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

src/wallet/wallet.cpp

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

re-ACK 1b1dad3efbbdf86f5426bf9c43902f40cb9a06c0

test/functional/rpc_fundrawtransaction.py

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

src/wallet/rpcwallet.cpp

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

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 Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Apr 26, 2021

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

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

rpc: include_unsafe option for fundrawtransaction #21359

rpc: include_unsafe option for fundrawtransaction #21359

Uh oh!

Conversation

t-bast commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

t-bast Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

t-bast commented Mar 17, 2021

Uh oh!

achow101 commented Mar 17, 2021

Uh oh!

t-bast commented Mar 25, 2021

Uh oh!

renepickhardt commented Mar 29, 2021

Uh oh!

t-bast commented Mar 29, 2021

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 26, 2021

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 30, 2021

Uh oh!

Uh oh!

ariard Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

t-bast Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ariard Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

t-bast Apr 30, 2021

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 3, 2021

Uh oh!

laanwj commented May 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

t-bast commented Mar 4, 2021 •

edited

Loading

DrahtBot commented Mar 4, 2021 •

edited

Loading

jonatack left a comment •

edited

Loading