rpc: Add min_conf option to fund transaction calls #14641

pull promag wants to merge 2 commits into bitcoin:master from promag:2018-11-fundrawtransaction changing 3 files +15 −1
  1. promag commented at 2:31 PM on November 2, 2018: member

    This PR adds min_conf option to fundrawtransaction and walletcreatefundedpsbt RPC.

    Fixes #14542.

  2. MarcoFalke added the label Feature on Nov 2, 2018
  3. MarcoFalke added the label Wallet on Nov 2, 2018
  4. MarcoFalke added the label RPC/REST/ZMQ on Nov 2, 2018
  5. DrahtBot commented at 2:38 PM on November 2, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  6. promag commented at 2:47 PM on November 2, 2018: member

    Will add release notes and tests after some concept ACK.

  7. gmaxwell commented at 9:17 PM on November 3, 2018: contributor

    What is the max_conf for?

  9. 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).

  10. promag commented at 6:04 PM on November 4, 2018: member

    Is it reasonable to add these options to coin control?

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

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

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

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

  15. DrahtBot added the label Needs rebase on Nov 9, 2018
  16. promag force-pushed on Nov 12, 2018
  17. 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
  18. 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.

  19. DrahtBot removed the label Needs rebase on Nov 12, 2018
  20. promag force-pushed on Nov 12, 2018
  21. 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: 
                                "     \"min_conf\"               (numeric, optional, default=0) The minimum confirmations to filter\n"
  22. 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: 
                                "     \"min_conf\"               (numeric, optional, default=0) The minimum confirmations to filter\n"
  23. promag commented at 3:16 PM on November 21, 2018: member

    Current default for min_conf is 0.

  24. meshcollider commented at 2:30 AM on November 22, 2018: contributor

    Concept ACK

  25. laanwj commented at 9:18 AM on November 23, 2018: member

    Concept ACK

  26. promag force-pushed on Dec 4, 2018
  27. promag force-pushed on Dec 4, 2018
  28. 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)?

  29. DrahtBot added the label Needs rebase on Dec 5, 2018
  30. promag force-pushed on Dec 5, 2018
  31. 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.

  32. DrahtBot removed the label Needs rebase on Dec 5, 2018
  33. promag force-pushed on Dec 6, 2018
  34. DrahtBot added the label Needs rebase on Dec 10, 2018
  35. promag force-pushed on Dec 11, 2018
  36. promag commented at 12:30 PM on December 11, 2018: member

    Rebased.

  37. DrahtBot removed the label Needs rebase on Dec 11, 2018
  38. 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.

  39. promag commented at 10:52 AM on December 17, 2018: member

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

  40. 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..)

  41. promag commented at 8:20 AM on January 18, 2019: member

    Ping @gmaxwell.

  42. MarcoFalke added the label Needs rebase on Feb 12, 2019
  43. 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: 
                                {"min_conf", RPCArg::Type::NUM, /* default */ "0", "The minimum confirmations to filter"},
    promag commented at 4:28 PM on March 3, 2019:

    Done.

  44. promag force-pushed on Mar 3, 2019
  45. promag commented at 4:29 PM on March 3, 2019: member

    Rebased.

  46. DrahtBot removed the label Needs rebase on Mar 3, 2019
  47. DrahtBot added the label Needs rebase on Mar 4, 2019
  48. promag force-pushed on Mar 11, 2019
  49. DrahtBot removed the label Needs rebase on Mar 11, 2019
  50. 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?

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

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

  53. DrahtBot added the label Needs rebase on Apr 15, 2019
  54. promag force-pushed on Apr 16, 2019
  55. 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.

  56. DrahtBot removed the label Needs rebase on Apr 17, 2019
  57. 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
  58. 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.

  59. DrahtBot added the label Needs rebase on Aug 2, 2019
  60. rpc: Add min_conf option to fundrawtransaction and walletcreatefundedpsbt 965628a5c0
  61. qa: Test fundrawtransaction and walletcreatefundedpsbt with min_conf 55a0b4c0f9
  62. 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

  63. promag commented at 3:00 PM on April 1, 2020: member

    Thanks @luke-jr!

  64. promag force-pushed on Apr 2, 2020
  65. DrahtBot removed the label Needs rebase on Apr 2, 2020
  66. DrahtBot added the label Needs rebase on Jun 25, 2020
  67. DrahtBot commented at 6:44 PM on June 25, 2020: member

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

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

  69. DrahtBot commented at 11:22 AM on December 15, 2021: member

    <!--13523179cfe9479db18ec6c5d236f789-->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.
  70. DrahtBot commented at 1:08 PM on March 21, 2022: member

    <!--13523179cfe9479db18ec6c5d236f789-->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.
  71. MarcoFalke added the label Up for grabs on Mar 21, 2022
  72. MarcoFalke commented at 1:45 PM on March 21, 2022: member

    Marked "up for grabs"

  73. fanquake closed this on Apr 26, 2022
  74. luke-jr referenced this in commit 637fede758 on May 21, 2022
  75. luke-jr referenced this in commit 972a1feefa on May 21, 2022
  76. fanquake removed the label Up for grabs on Jun 15, 2022
  77. achow101 referenced this in commit b55b11f92a on Jan 16, 2023
  78. DrahtBot locked this on Jun 15, 2023
Contributors
promagDrahtBotgmaxwelljonasschnelliconscottmeshcolliderlaanwjMarcoFalkeluke-jrejose19
Labels
FeatureWalletRPC/REST/ZMQNeeds rebase
Linked (view graph)
#14542 fundrawtransaction - select input transaction with minimum confirmations#25375 rpc: add minconf/maxconf options to sendall and fund transaction calls
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-01 03:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me