rpc: Add min_conf option to fund transaction calls

promag commented at 2:31 pm on November 2, 2018: member

This PR adds min_conf option to fundrawtransaction and walletcreatefundedpsbt RPC.

Fixes #14542.

MarcoFalke added the label Feature on Nov 2, 2018

MarcoFalke added the label Wallet on Nov 2, 2018

MarcoFalke added the label RPC/REST/ZMQ on Nov 2, 2018

DrahtBot commented at 2:38 pm on November 2, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

promag commented at 2:47 pm on November 2, 2018: member

Will add release notes and tests after some concept ACK.

gmaxwell commented at 9:17 pm on November 3, 2018: contributor

What is the max_conf for?

promag commented at 9:54 pm on November 3, 2018: member

@gmaxwell just followed listunspent options. https://github.com/bitcoin/bitcoin/blob/742ee213499194f97e59dae4971f1474ae7d57ad/src/wallet/rpcwallet.cpp#L2592-L2598

jonasschnelli commented at 1:53 pm on November 4, 2018: contributor

Concept ACK. The max_conf usefulness ist doubtable,… but maybe acceptable to be consistent with listunspent (and it’s a argument based option-object even in non-argument based RPC calls).

promag commented at 6:04 pm on November 4, 2018: member

Is it reasonable to add these options to coin control?

conscott commented at 11:29 am on November 6, 2018: contributor

Concept ACK

~~Maybe you can add some test cases to the rpc_fundrawtransaction.py as well to make sure coin selection fails if conditions are not met?~~

Nevermind, I see the note about tests.

gmaxwell commented at 7:01 pm on November 6, 2018: contributor

I believe the max exists for listunspent so that with the other flags you can see only unconfirmed outputs, which wouldn’t apply here, I think.

The maximum argument turns out to be a severe nuisance on listunspent because 99.99999% of the time you want it set to maximum and ‘maximum’ is some arbitrary big number but too big and the rpc is rejected (and that threshold even changed once, breaking all my spending automation). … but there it’s not an optional argument, so I think the same nuisance wouldn’t apply here.

Still, we’re left with functionality being added where no one can articulate a reason. I don’t think that is acceptable, esp where if it ever became useful it could be added without breaking compatibility.

promag commented at 11:48 am on November 7, 2018: member

@gmaxwell I’m happy to remove max_conf here, as it isn’t required for #14542. However it can be acceptable to create a transaction with fresh inputs, especially with 0 confirmations?

meshcollider commented at 11:13 pm on November 8, 2018: contributor

@promag I think in such a specific case they can manually pick the inputs to fund the transaction with, I agree with the comments above and think max_conf is probably not the most useful

DrahtBot added the label Needs rebase on Nov 9, 2018

promag force-pushed on Nov 12, 2018

promag renamed this:
~~RPC: Add min/max confirmation options to fund transaction calls~~
rpc: Add min_conf option to fund transaction calls
on Nov 12, 2018

promag commented at 1:24 pm on November 12, 2018: member

Rebased and removed max_conf option. Please concept ACK to add release note and tests.

DrahtBot removed the label Needs rebase on Nov 12, 2018

promag force-pushed on Nov 12, 2018

in src/wallet/rpcwallet.cpp:2944 in 235bd748ab outdated

2940@@ -2936,6 +2941,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
2941                             "     \"replaceable\"            (boolean, optional) Marks this transaction as BIP125 replaceable.\n"
2942                             "                              Allows this transaction to be replaced by a transaction with higher fees\n"
2943                             "     \"conf_target\"            (numeric, optional) Confirmation target (in blocks)\n"
2944+                            "     \"min_conf\"               (numeric, optional, default=1) The minimum confirmations to filter\n"

promag commented at 3:15 pm on November 21, 2018:

0                            "     \"min_conf\"               (numeric, optional, default=0) The minimum confirmations to filter\n"

in src/wallet/rpcwallet.cpp:3954 in 235bd748ab outdated

3950@@ -3945,6 +3951,7 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
3951                             "     \"replaceable\"            (boolean, optional) Marks this transaction as BIP125 replaceable.\n"
3952                             "                              Allows this transaction to be replaced by a transaction with higher fees\n"
3953                             "     \"conf_target\"            (numeric, optional) Confirmation target (in blocks)\n"
3954+                            "     \"min_conf\"               (numeric, optional, default=1) The minimum confirmations to filter\n"

promag commented at 3:16 pm on November 21, 2018:

0                            "     \"min_conf\"               (numeric, optional, default=0) The minimum confirmations to filter\n"

promag commented at 3:16 pm on November 21, 2018: member

Current default for min_conf is 0.

meshcollider commented at 2:30 am on November 22, 2018: contributor

Concept ACK

laanwj commented at 9:18 am on November 23, 2018: member

Concept ACK

promag force-pushed on Dec 4, 2018

promag commented at 11:25 pm on December 4, 2018: member

Added tests and release notes. Is this useful for 0.17 branch (after 0.17.1)?

DrahtBot added the label Needs rebase on Dec 5, 2018

promag force-pushed on Dec 5, 2018

in src/wallet/rpcwallet.cpp:3025 in 493047a221 outdated

3021@@ -3017,6 +3022,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
3022                             {"replaceable", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "", "Marks this transaction as BIP125 replaceable.\n"
3023                             "                              Allows this transaction to be replaced by a transaction with higher fees"},
3024                             {"conf_target", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "", "Confirmation target (in blocks)"},
3025+                            {"min_conf", RPCArg::Type::NUM, /* opt */ true, /* default_val */ 0, "The minimum confirmations to filter"},

MarcoFalke commented at 10:56 pm on December 5, 2018:

Whoopsie, default_value should be string. Should fix this by disallowing something else.

DrahtBot removed the label Needs rebase on Dec 5, 2018

promag force-pushed on Dec 6, 2018

DrahtBot added the label Needs rebase on Dec 10, 2018

promag force-pushed on Dec 11, 2018

promag commented at 12:30 pm on December 11, 2018: member

Rebased.

DrahtBot removed the label Needs rebase on Dec 11, 2018

gmaxwell commented at 11:31 pm on December 12, 2018: contributor

:( I hate to say it, but I think this runs into the problems resolving #14602. In short, you almost never want to spend coins that come from third parties without confirmation (as they could be doublespent, causing you to double spend someone) but you almost always do want to be willing to use your own change unconfirmed if its required to get enough funds– otherwise you’ll have seemingly inexplicable failures due to coins being tied up by other unconfirmed transactions.

promag commented at 10:52 am on December 17, 2018: member

@gmaxwell I think it’s reasonable to have both ways to filter unspents?

luke-jr commented at 5:12 am on December 20, 2018: member

utACK (despite the issue @gmaxwell points out, I think this is still an improvement..)

promag commented at 8:20 am on January 18, 2019: member

Ping @gmaxwell.

MarcoFalke added the label Needs rebase on Feb 12, 2019

in src/wallet/rpcwallet.cpp:3024 in 78c9eef211 outdated

3020@@ -3016,6 +3021,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
3021                             {"replaceable", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "fallback to wallet's default", "Marks this transaction as BIP125 replaceable.\n"
3022                             "                              Allows this transaction to be replaced by a transaction with higher fees"},
3023                             {"conf_target", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "fallback to wallet's default", "Confirmation target (in blocks)"},
3024+                            {"min_conf", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "0", "The minimum confirmations to filter"},

MarcoFalke commented at 11:53 pm on February 12, 2019:

0                            {"min_conf", RPCArg::Type::NUM, /* default */ "0", "The minimum confirmations to filter"},

promag commented at 4:28 pm on March 3, 2019:

Done.

promag force-pushed on Mar 3, 2019

promag commented at 4:29 pm on March 3, 2019: member

Rebased.

DrahtBot removed the label Needs rebase on Mar 3, 2019

DrahtBot added the label Needs rebase on Mar 4, 2019

promag force-pushed on Mar 11, 2019

DrahtBot removed the label Needs rebase on Mar 11, 2019

luke-jr commented at 9:22 pm on April 8, 2019: member

In light of removal of minconf from sendmany, are we still moving forward on this?

MarcoFalke commented at 10:22 pm on April 8, 2019: member

sendmany::minconf was 1 (by default and in many scripts), so it couldn’t be used in the same fashion as min_conf in this pull request. Otherwise it would break existing behaviour to spend own mempool-change #15595 (comment).

I wouldn’t object adding min_conf with default=0 to other wallet calls as well.

in src/wallet/rpcwallet.cpp:3032 in 5bb1356e57 outdated

3028@@ -3024,7 +3029,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3029 
3030     std::string strFailReason;
3031 
3032-    if (!pwallet->FundTransaction(tx, fee_out, change_position, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {
3033+    if (!pwallet->FundTransaction(tx, fee_out, change_position, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl, min_depth)) {

MarcoFalke commented at 10:29 pm on April 8, 2019:

Could pass down via coin control to avoid having to adjust all function signatures?

See e.g. https://github.com/bitcoin/bitcoin/pull/15595/files#diff-5694bdba4c3d58805c185a3fbced2868R36

DrahtBot added the label Needs rebase on Apr 15, 2019

promag force-pushed on Apr 16, 2019

in .gitignore:64 in a3991b7c0b outdated

60@@ -61,6 +61,7 @@ src/qt/bitcoin-qt.includes
61 *.orig
62 *.pyc
63 *.o
64+*.o.*

luke-jr commented at 10:43 pm on April 16, 2019:

Why?

promag commented at 10:48 pm on April 16, 2019:

I’m glad you ask 😄try running make -j10 and in parallel run git status. Some editors will refresh showing these temporary files (mostly *.o.tmp).

Edit 2: at least on macos. Edit: happy to submit this in a separate PR.

DrahtBot removed the label Needs rebase on Apr 17, 2019

MarcoFalke commented at 1:55 pm on July 2, 2019: member

On pull request is removing min conf, the other is adding it. Would be nice if there was a uniform general direction.

rpc: Add min_conf option to fund transaction calls #14641
rpc: Raise error in getbalance if minconf is not zero #15729

promag commented at 2:03 pm on July 2, 2019: member

This is similar to listunspent and handles all utxo the same, regardless if change or not.

DrahtBot added the label Needs rebase on Aug 2, 2019

rpc: Add min_conf option to fundrawtransaction and walletcreatefundedpsbt 965628a5c0

qa: Test fundrawtransaction and walletcreatefundedpsbt with min_conf 55a0b4c0f9

luke-jr commented at 2:36 pm on April 1, 2020: member

@promag here’s a rebase you can use https://github.com/bitcoin/bitcoin/compare/master...luke-jr:fundraw_minconf

promag commented at 3:00 pm on April 1, 2020: member

Thanks @luke-jr!

promag force-pushed on Apr 2, 2020

DrahtBot removed the label Needs rebase on Apr 2, 2020

DrahtBot added the label Needs rebase on Jun 25, 2020

DrahtBot commented at 6:44 pm on June 25, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

ejose19 commented at 9:09 pm on November 27, 2020: none

While looking for this functionability I found this PR, and I also had the same concerns as @gmaxwell regarding the trusted/untrusted. However could we use instead m_min_depth_mine & m_min_depth_theirs similar to

https://github.com/bitcoin/bitcoin/blob/e2ff5e7b35d71195278d2a2ed9485f141de33d7a/src/wallet/coinselection.cpp#L331-L336

and implement it at either AvailableCoins or construct the CoinEligibilityFilter with the passed arguments at

https://github.com/bitcoin/bitcoin/blob/e2ff5e7b35d71195278d2a2ed9485f141de33d7a/src/wallet/wallet.cpp#L2484-L2491

?. This way we can set 0 || 1 for own transactions, and 3+ for untrusted txs (since node already marks eligible with 1 confirmation, but that’s not enough for a third party tx)

I’m interested in submitting a PR in case the proposed resolution is correct.

DrahtBot commented at 11:22 am on December 15, 2021: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 1:08 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

MarcoFalke added the label Up for grabs on Mar 21, 2022

MarcoFalke commented at 1:45 pm on March 21, 2022: member

Marked “up for grabs”

fanquake closed this on Apr 26, 2022

luke-jr referenced this in commit 637fede758 on May 21, 2022

luke-jr referenced this in commit 972a1feefa on May 21, 2022

fanquake removed the label Up for grabs on Jun 15, 2022

achow101 referenced this in commit b55b11f92a on Jan 16, 2023

DrahtBot locked this on Jun 15, 2023

rpc: Add min_conf option to fund transaction calls #14641

Conflicts