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: Add min_conf option to fund transaction calls #14641

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

@promag
Copy link
Member

@promag promag commented Nov 2, 2018

This PR adds min_conf option to fundrawtransaction and walletcreatefundedpsbt RPC.

Fixes #14542.

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 2, 2018

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

@promag promag commented Nov 2, 2018

Will add release notes and tests after some concept ACK.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Nov 3, 2018

What is the max_conf for?

@promag
Copy link
Member Author

@promag promag commented Nov 3, 2018

@gmaxwell just followed listunspent options.

"listunspent ( minconf maxconf [\"addresses\",...] [include_unsafe] [query_options])\n"
"\nReturns array of unspent transaction outputs\n"
"with between minconf and maxconf (inclusive) confirmations.\n"
"Optionally filter to only include txouts paid to specified addresses.\n"
"\nArguments:\n"
"1. minconf (numeric, optional, default=1) The minimum confirmations to filter\n"
"2. maxconf (numeric, optional, default=9999999) The maximum confirmations to filter\n"

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Nov 4, 2018

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

@promag promag commented Nov 4, 2018

Is it reasonable to add these options to coin control?

Copy link
Contributor

@conscott conscott left a comment

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

@gmaxwell gmaxwell commented Nov 6, 2018

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

@promag promag commented Nov 7, 2018

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

@meshcollider meshcollider commented Nov 8, 2018

@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

@promag promag force-pushed the promag:2018-11-fundrawtransaction branch Nov 12, 2018
@promag promag changed the title RPC: Add min/max confirmation options to fund transaction calls rpc: Add min_conf option to fund transaction calls Nov 12, 2018
@promag
Copy link
Member Author

@promag promag commented Nov 12, 2018

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

@promag promag force-pushed the promag:2018-11-fundrawtransaction branch Nov 12, 2018
Copy link
Member Author

@promag promag left a comment

Current default for min_conf is 0.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Nov 22, 2018

Concept ACK

1 similar comment
@laanwj
Copy link
Member

@laanwj laanwj commented Nov 23, 2018

Concept ACK

@promag promag force-pushed the promag:2018-11-fundrawtransaction branch 2 times, most recently Dec 4, 2018
@promag
Copy link
Member Author

@promag promag commented Dec 4, 2018

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

@promag promag force-pushed the promag:2018-11-fundrawtransaction branch Dec 5, 2018
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@promag promag force-pushed the promag:2018-11-fundrawtransaction branch Dec 6, 2018
@promag
Copy link
Member Author

@promag promag commented Jan 18, 2019

Ping @gmaxwell.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@promag promag force-pushed the promag:2018-11-fundrawtransaction branch from 78c9eef to 6ef4b82 Mar 3, 2019
@promag
Copy link
Member Author

@promag promag commented Mar 3, 2019

Rebased.

@promag promag force-pushed the promag:2018-11-fundrawtransaction branch from 6ef4b82 to 5bb1356 Mar 11, 2019
@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 8, 2019

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Apr 8, 2019

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.

@@ -3024,7 +3029,7 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f

std::string strFailReason;

if (!pwallet->FundTransaction(tx, fee_out, change_position, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl)) {
if (!pwallet->FundTransaction(tx, fee_out, change_position, strFailReason, lockUnspents, setSubtractFeeFromOutputs, coinControl, min_depth)) {

This comment has been minimized.

@MarcoFalke

MarcoFalke Apr 8, 2019
Member

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

@promag promag force-pushed the promag:2018-11-fundrawtransaction branch from 5bb1356 to a3991b7 Apr 16, 2019
.gitignore Outdated
@@ -61,6 +61,7 @@ src/qt/bitcoin-qt.includes
*.orig
*.pyc
*.o
*.o.*

This comment has been minimized.

This comment has been minimized.

@promag

promag Apr 16, 2019
Author Member

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.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jul 2, 2019

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

@promag promag commented Jul 2, 2019

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

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Apr 1, 2020

@promag here's a rebase you can use master...luke-jr:fundraw_minconf

@promag
Copy link
Member Author

@promag promag commented Apr 1, 2020

Thanks @luke-jr!

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jun 25, 2020

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

@ejose19
Copy link

@ejose19 ejose19 commented Nov 27, 2020

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

bool OutputGroup::EligibleForSpending(const CoinEligibilityFilter& eligibility_filter) const
{
return m_depth >= (m_from_me ? eligibility_filter.conf_mine : eligibility_filter.conf_theirs)
&& m_ancestors <= eligibility_filter.max_ancestors
&& m_descendants <= eligibility_filter.max_descendants;
}

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

bitcoin/src/wallet/wallet.cpp

Lines 2484 to 2491 in e2ff5e7

bool res = value_to_select <= 0 ||
SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used) ||
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
(m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used)) ||
(m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), groups, setCoinsRet, nValueRet, coin_selection_params, bnb_used));

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

10 participants