rpc: Add min_conf option to fund transaction calls #14641

promag · 2018-11-02T14:31:34Z

This PR adds min_conf option to fundrawtransaction and walletcreatefundedpsbt RPC.

DrahtBot · 2018-11-02T14:38:35Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #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 · 2018-11-02T14:47:16Z

Will add release notes and tests after some concept ACK.

gmaxwell · 2018-11-03T21:17:31Z

What is the max_conf for?

promag · 2018-11-03T21:54:44Z

@gmaxwell just followed listunspent options.

bitcoin/src/wallet/rpcwallet.cpp

Lines 2592 to 2598 in 742ee21

    
           "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 · 2018-11-04T13:53:10Z

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 · 2018-11-04T18:04:17Z

Is it reasonable to add these options to coin control?

conscott

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 · 2018-11-06T19:01:18Z

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 · 2018-11-07T11:48:34Z

@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 · 2018-11-08T23:13:02Z

@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 · 2018-11-12T13:24:49Z

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

promag

Current default for min_conf is 0.

src/wallet/rpcwallet.cpp

meshcollider · 2018-11-22T02:30:24Z

Concept ACK

laanwj · 2018-11-23T09:18:38Z

Concept ACK

promag · 2018-12-04T23:25:33Z

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

src/wallet/rpcwallet.cpp

luke-jr · 2019-04-16T22:43:12Z

.gitignore

 *.orig
 *.pyc
 *.o
+*.o.*


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.

maflcko · 2019-07-02T13:55:52Z

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 rpc: Add min_conf option to fund transaction calls #14641
rpc: Raise error in getbalance if minconf is not zero rpc: Raise error in getbalance if minconf is not zero #15729

promag · 2019-07-02T14:03:05Z

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

…psbt

luke-jr · 2020-04-01T14:36:28Z

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

promag · 2020-04-01T15:00:11Z

Thanks @luke-jr!

DrahtBot · 2020-06-25T18:44:48Z

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

ejose19 · 2020-11-27T21:09:02Z

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

bitcoin/src/wallet/coinselection.cpp

Lines 331 to 336 in e2ff5e7

    
           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.

DrahtBot · 2021-12-15T11:22:45Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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 · 2022-03-21T13:08:00Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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.

maflcko · 2022-03-21T13:45:30Z

Marked "up for grabs"

…psbt [as deprecated] Github-Pull: bitcoin#14641 Rebased-From: 965628a [partial]

Github-Pull: bitcoin#14641 Rebased-From: 4138cb3

…ansaction calls cfe5aeb rpc: add minconf and maxconf options to sendall (ishaanam) a07a413 Wallet/RPC: Allow specifying min & max chain depth for inputs used by fund calls (Juan Pablo Civile) Pull request description: This PR adds a "minconf" option to `fundrawtransaction`, `walletcreatefundedpsbt`, and `sendall`. Alternative implementation of #14641 Fixes #14542 Edit: This PR now also adds this option to `send` ACKs for top commit: achow101: ACK cfe5aeb Xekyo: ACK cfe5aeb furszy: diff ACK cfe5aeb, only a non-blocking nit. Tree-SHA512: 836e610926eec3a62308fba88ddbd6a13d8f4dac37352d0309599f893cde9c1df5e9c298fda6e076493068e4d213e4afa7290a9e3bdb5a95a5d507da3f7b59e8

maflcko added Feature Wallet RPC/REST/ZMQ labels Nov 2, 2018

conscott reviewed Nov 6, 2018

View reviewed changes

DrahtBot added the Needs rebase label Nov 9, 2018

promag force-pushed the 2018-11-fundrawtransaction branch from d5e1b6c to d430696 Compare November 12, 2018 13:22

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

DrahtBot removed the Needs rebase label Nov 12, 2018

promag force-pushed the 2018-11-fundrawtransaction branch from d430696 to 235bd74 Compare November 12, 2018 14:18

promag commented Nov 21, 2018

View reviewed changes

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

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

promag force-pushed the 2018-11-fundrawtransaction branch 2 times, most recently from 0c328a4 to 2b11f02 Compare December 4, 2018 23:21

DrahtBot added the Needs rebase label Dec 5, 2018

promag force-pushed the 2018-11-fundrawtransaction branch from 2b11f02 to 493047a Compare December 5, 2018 22:09

maflcko reviewed Dec 5, 2018

View reviewed changes

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

DrahtBot removed the Needs rebase label Dec 5, 2018

promag force-pushed the 2018-11-fundrawtransaction branch from 493047a to 3831c98 Compare December 6, 2018 00:56

DrahtBot added the Needs rebase label Apr 15, 2019

promag force-pushed the 2018-11-fundrawtransaction branch from 5bb1356 to a3991b7 Compare April 16, 2019 14:37

luke-jr reviewed Apr 16, 2019

View reviewed changes

DrahtBot removed the Needs rebase label Apr 17, 2019

maflcko mentioned this pull request Jul 2, 2019

rpc: Raise error in getbalance if minconf is not zero #15729

Closed

DrahtBot added the Needs rebase label Aug 2, 2019

promag added 2 commits April 1, 2020 13:24

rpc: Add min_conf option to fundrawtransaction and walletcreatefunded…

965628a

…psbt

qa: Test fundrawtransaction and walletcreatefundedpsbt with min_conf

55a0b4c

promag force-pushed the 2018-11-fundrawtransaction branch from a3991b7 to 55a0b4c Compare April 2, 2020 02:33

DrahtBot removed the Needs rebase label Apr 2, 2020

DrahtBot mentioned this pull request Apr 2, 2020

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #11413

Merged

DrahtBot added the Needs rebase label Jun 25, 2020

luke-jr mentioned this pull request Jun 25, 2021

rpc: allow specifying min chain depth for inputs in fund calls #22049

Closed

maflcko added the Up for grabs label Mar 21, 2022

fanquake closed this Apr 26, 2022

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

rpc: Add min_conf option to fundrawtransaction and walletcreatefunded…

637fede

…psbt [as deprecated] Github-Pull: bitcoin#14641 Rebased-From: 965628a [partial]

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

qa: Test fundrawtransaction and walletcreatefundedpsbt with min_conf

972a1fe

Github-Pull: bitcoin#14641 Rebased-From: 4138cb3

ishaanam mentioned this pull request Jun 15, 2022

rpc: add minconf/maxconf options to sendall and fund transaction calls #25375

Merged

fanquake removed the Up for grabs label Jun 15, 2022

bitcoin locked and limited conversation to collaborators Jun 15, 2023

rpc: Add min_conf option to fund transaction calls #14641

rpc: Add min_conf option to fund transaction calls #14641

Uh oh!

Conversation

promag commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag commented Nov 2, 2018

Uh oh!

gmaxwell commented Nov 3, 2018

Uh oh!

promag commented Nov 3, 2018

Uh oh!

jonasschnelli commented Nov 4, 2018

Uh oh!

promag commented Nov 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

conscott 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!

gmaxwell commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 7, 2018

Uh oh!

meshcollider commented Nov 8, 2018

Uh oh!

promag commented Nov 12, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

meshcollider commented Nov 22, 2018

Uh oh!

laanwj commented Nov 23, 2018

Uh oh!

promag commented Dec 4, 2018

Uh oh!

Uh oh!

luke-jr Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

promag Apr 16, 2019 • 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!

maflcko commented Jul 2, 2019

Uh oh!

promag commented Jul 2, 2019

Uh oh!

luke-jr commented Apr 1, 2020

Uh oh!

promag commented Apr 1, 2020

Uh oh!

DrahtBot commented Jun 25, 2020

Uh oh!

ejose19 commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

maflcko commented Mar 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

promag commented Nov 2, 2018 •

edited

Loading

DrahtBot commented Nov 2, 2018 •

edited

Loading

promag commented Nov 4, 2018 •

edited

Loading

conscott left a comment •

edited

Loading

gmaxwell commented Nov 6, 2018 •

edited

Loading

promag Apr 16, 2019 •

edited

Loading

ejose19 commented Nov 27, 2020 •

edited

Loading