Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: include_unsafe option for fundrawtransaction #21359

Merged
merged 1 commit into from May 10, 2021

Conversation

t-bast
Copy link
Contributor

@t-bast t-bast commented Mar 4, 2021

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
Copy link
Contributor

DrahtBot commented Mar 4, 2021

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

Conflicts

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.

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@t-bast t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 92e5b3a to 0dc8808 Compare March 10, 2021 14:37
@@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

t-bast commented Mar 17, 2021

@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
Copy link
Member

ACK 16076bb

@t-bast
Copy link
Contributor Author

t-bast commented Mar 25, 2021

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
Copy link

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
Copy link
Contributor Author

t-bast commented Mar 29, 2021

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

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

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4e4d7c0

Some comments below.

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
@jonatack
Copy link
Contributor

ACK 0709545

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

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a5c3d9c

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

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@t-bast t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from a5c3d9c to 2bf0b1e Compare April 30, 2021 08:16
src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@t-bast t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from ce70f32 to 1b1dad3 Compare April 30, 2021 09:33
@jonatack
Copy link
Contributor

re-ACK 1b1dad3

@maflcko maflcko requested a review from instagibbs April 30, 2021 11:19
test/functional/rpc_fundrawtransaction.py Show resolved Hide resolved
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took the definition of fSafe here:

bool fSafe;

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

src/wallet/wallet.cpp Outdated Show resolved Hide resolved
@@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@t-bast t-bast force-pushed the fund-raw-transaction-allow-unsafe branch from 1b1dad3 to 11d6459 Compare April 30, 2021 16:53
@DrahtBot
Copy link
Contributor

DrahtBot commented May 3, 2021

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

@laanwj
Copy link
Member

laanwj commented May 10, 2021

Code review ACK 11d6459

@laanwj 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
t-bast added a commit to t-bast/bitcoin that referenced this pull request May 11, 2021
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.
meshcollider added a commit that referenced this pull request May 13, 2021
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
@t-bast t-bast deleted the fund-raw-transaction-allow-unsafe branch May 13, 2021 18:12
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021
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
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
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.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add include_unsafe option to fundrawtransaction
10 participants