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

pull ishaanam wants to merge 2 commits into bitcoin:master from ishaanam:fundrawtransaction_minconf changing 6 files +261 −2
  1. ishaanam commented at 12:08 AM on June 15, 2022: contributor

    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

  2. DrahtBot added the label RPC/REST/ZMQ on Jun 15, 2022
  3. DrahtBot added the label Wallet on Jun 15, 2022
  4. DrahtBot commented at 8:28 AM on June 15, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK Xekyo, achow101, furszy
    Stale ACK w0xlt, 1440000bytes

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26732 (wallet: tx creation, don't select outputs from txes that are being replaced by furszy)

    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.

  5. unknown approved
  6. unknown commented at 7:28 AM on June 17, 2022: none

    ACK https://github.com/bitcoin/bitcoin/pull/25375/commits/0f023da016b2230af2d86b9461bfe5218716fdb5

    Tested minconf argument on regtest with:

    $ bitcoin-cli generatetoaddress 102 bcrt1qjjdvpcz3l2p4q2y7zc6ztkk3rvagke248y6ckm

$ bitcoin-cli listunspent
[
  {
    "txid": "56a91239cfcff53da811ae1969bb6e8e288f15ab58cac8fe455ea906a3b51ecb",
    "vout": 0,
    "address": "bcrt1qjjdvpcz3l2p4q2y7zc6ztkk3rvagke248y6ckm",
    "label": "",
    "scriptPubKey": "0014949ac0e051fa8350289e163425dad11b3a8b6555",
    "amount": 50.00000000,
    "confirmations": 101,
    "spendable": true,
    "solvable": true,
    "desc": "wpkh([ae30e814/84'/1'/0'/0/0]020a3db12de02b1f4bbb5e5016f7cebaa427e0c60d20b95ebc552ceb97c86f3722)#n2vukszk",
    "safe": true
  },
  {
    "txid": "f1257f379856d91401d27229dbdf797802a521edb6f958bc884e316265a48cfc",
    "vout": 0,
    "address": "bcrt1qjjdvpcz3l2p4q2y7zc6ztkk3rvagke248y6ckm",
    "label": "",
    "scriptPubKey": "0014949ac0e051fa8350289e163425dad11b3a8b6555",
    "amount": 50.00000000,
    "confirmations": 102,
    "spendable": true,
    "solvable": true,
    "desc": "wpkh([ae30e814/84'/1'/0'/0/0]020a3db12de02b1f4bbb5e5016f7cebaa427e0c60d20b95ebc552ceb97c86f3722)#n2vukszk",
    "safe": true
  }
]


    $ bitcoin-cli createrawtransaction '[]' '{"data":"505220233235333735"}'

02000000000100000000000000000b6a0950522023323533373500000000

    

$ bitcoin-cli fundrawtransaction "02000000000100000000000000000b6a0950522023323533373500000000" "{\"minconf\": 102}"
{
  "hex": "0200000001fc8ca46562314e88bc58f9b6ed21a5027879dfdb2972d20114d95698377f25f10000000000feffffff0272f1052a010000002251209b08765d5b558137195f9b3b5b4f5ceaffddcac91590e76f904fdde08cc033bb00000000000000000b6a0950522023323533373500000000",
  "fee": 0.00000142,
  "changepos": 0
}

$ bitcoin-cli decoderawtransaction 0200000001fc8ca46562314e88bc58f9b6ed21a5027879dfdb2972d20114d95698377f25f10000000000feffffff0272f1052a010000002251209b08765d5b558137195f9b3b5b4f5ceaffddcac91590e76f904fdde08cc033bb00000000000000000b6a0950522023323533373500000000
{
  "txid": "4974082891c14f96170f2af7f381e3a6f9510cb9e42841b1e12463f09b164ad5",
  "hash": "4974082891c14f96170f2af7f381e3a6f9510cb9e42841b1e12463f09b164ad5",
  "version": 2,
  "size": 114,
  "vsize": 114,
  "weight": 456,
  "locktime": 0,
  "vin": [
    {
      "txid": "f1257f379856d91401d27229dbdf797802a521edb6f958bc884e316265a48cfc",
      "vout": 0,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "sequence": 4294967294
    }
  ],
  "vout": [
    {
      "value": 49.99999858,
      "n": 0,
      "scriptPubKey": {
        "asm": "1 9b08765d5b558137195f9b3b5b4f5ceaffddcac91590e76f904fdde08cc033bb",
        "desc": "addr(bcrt1pnvy8vh2m2kqnwx2lnva4kn6uatlamjkfzkgwwmusflw7prxqxwas8y08ed)#4mpq27kr",
        "hex": "51209b08765d5b558137195f9b3b5b4f5ceaffddcac91590e76f904fdde08cc033bb",
        "address": "bcrt1pnvy8vh2m2kqnwx2lnva4kn6uatlamjkfzkgwwmusflw7prxqxwas8y08ed",
        "type": "witness_v1_taproot"
      }
    },
    {
      "value": 0.00000000,
      "n": 1,
      "scriptPubKey": {
        "asm": "OP_RETURN 505220233235333735",
        "desc": "raw(6a09505220233235333735)#wxsnvgu2",
        "hex": "6a09505220233235333735",
        "type": "nulldata"
      }
    }
  ]
}

    $ bitcoin-cli fundrawtransaction "02000000000100000000000000000b6a0950522023323533373500000000" "{\"minconf\": 103}"
error code: -4
error message:
Insufficient funds

    nit: If no UTXO available in wallet with minconf confirmations, the error could be specific to help the user. Example: Insufficient funds with required confirmations

  7. achow101 commented at 2:19 PM on June 18, 2022: member

    nit: If no UTXO available in wallet with minconf confirmations, the error could be specific to help the user. Example: Insufficient funds with required confirmations

    It's actually quite difficult currently to get more specific errors out of coin selection. There's quite a bit of plumbing needed that does not currently exist. #25218 and #24845 add a lot of what is needed to get such errors, but there's still an additional issue where it is hard to tell whether the insufficient funds is due to this particular filter or something else in coin selection.

  8. w0xlt approved
  9. w0xlt commented at 4:30 PM on June 18, 2022: contributor

    Code Review ACK https://github.com/bitcoin/bitcoin/pull/25375/commits/0f023da016b2230af2d86b9461bfe5218716fdb5

  10. w0xlt commented at 4:41 PM on June 18, 2022: contributor

    Perhaps this same option could be included in send() RPC ? This also creates a CCoinControl object and passes it to FundTransaction(), just like the other methods covered in this PR.

  11. luke-jr commented at 9:44 PM on June 18, 2022: member

    This seems like a subset of #22049

  12. ishaanam force-pushed on Jun 20, 2022
  13. ishaanam commented at 5:15 PM on June 20, 2022: contributor

    Changes made since last push:

    • refactored sendall tests based on feedback from @Xekyo
    • moved minconf help text to FundTxDoc
    • added minconf option to send as suggested by @w0xlt and added a test for that
    • added check that minconf is not negative and added tests to ensure that an error is raised when minconf is negative
  14. ishaanam commented at 8:50 AM on July 11, 2022: contributor

    This seems like a subset of #22049

    Yes, I didn't see that one since it didn't mention the issue I was looking at. I took a look at the other PR and it looks like the main differences, disregarding the tests, are that this one doesn't implement maxconf, but does implement the minconf option for sendall as well.

  15. ishaanam commented at 8:58 PM on August 15, 2022: contributor

    I'm closing this PR as it is quite similar to #22049

  16. ishaanam closed this on Aug 15, 2022
  17. luke-jr commented at 6:23 PM on September 28, 2022: member

    @ishaanam As #22049 has been abandoned, perhaps copy and rebase it here?

    P.S. I still have a rebased version at https://github.com/bitcoin/bitcoin/compare/master...luke-jr:rpc_fundtx_minmaxconf

  18. ghost commented at 5:44 PM on September 29, 2022: none

    @ishaanam As #22049 has been abandoned, perhaps copy and rebase it here?

    P.S. I still have a rebased version at master...luke-jr:rpc_fundtx_minmaxconf

    I agree with @luke-jr , its important to fix the issue not the author or pr and #22049 is abandoned

  19. ishaanam reopened this on Sep 29, 2022
  20. ishaanam force-pushed on Sep 29, 2022
  21. ishaanam force-pushed on Sep 29, 2022
  22. ishaanam force-pushed on Sep 29, 2022
  23. ishaanam commented at 8:47 PM on September 29, 2022: contributor

    @ishaanam As #22049 has been abandoned, perhaps copy and rebase it here?

    P.S. I still have a rebased version at master...luke-jr:rpc_fundtx_minmaxconf

    I have reopened this PR, thanks for rebasing #22049!

  25. ishaanam renamed this:
    rpc: add minconf option to sendall and fund transaction calls
    rpc: add minconf/maxconf options to sendall and fund transaction calls
    on Oct 1, 2022
  26. glozow requested review from achow101 on Oct 12, 2022
  27. glozow requested review from murchandamus on Oct 12, 2022
  28. DrahtBot added the label Needs rebase on Oct 21, 2022
  29. ishaanam force-pushed on Oct 21, 2022
  30. ishaanam commented at 6:07 PM on October 21, 2022: contributor

    Rebased

  31. DrahtBot removed the label Needs rebase on Oct 21, 2022
  32. in test/functional/rpc_psbt.py:119 in bc7378b351 outdated 
     114 | +        self.nodes[1].sendtoaddress(wallet.getnewaddress(), 1)
 115 | +        self.nodes[1].sendtoaddress(wallet.getnewaddress(), 1)
 116 | +        self.generate(self.nodes[1], 1)
 117 | +        self.nodes[1].sendtoaddress(wallet.getnewaddress(), 1)
 118 | +        self.nodes[1].sendtoaddress(wallet.getnewaddress(), 1)
 119 | +        self.generate(self.nodes[1], 1)
    furszy commented at 7:36 PM on October 21, 2022:

    Maybe:

    for _ in range(2):
    self.nodes[1].sendmany("", {self.nodes[1].getnewaddress():1, self.nodes[1].getnewaddress():1})
    self.generate(self.nodes[1], 1)

    (same for the other one)

    ishaanam commented at 9:54 PM on October 25, 2022:

    Done

  33. unknown approved
  35. in test/functional/rpc_fundrawtransaction.py:1378 in bc7378b351 outdated 
    1373 | +        mempool = self.nodes[0].getrawmempool()
1374 | +        assert txid1 in mempool
1375 | +
1376 | +        self.log.info("Fail to craft a new TX that sends more funds with add_inputs = False")
1377 | +        raw_tx2 = wallet.createrawtransaction([{'txid': utxo1['txid'], 'vout': utxo1['vout']}], {target_address: 1})
1378 | +        assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx2, {'fee_rate': 1, 'add_inputs': False})
    furszy commented at 2:38 PM on October 22, 2022:

    Any reason for adding this case here?

    Seems to not be needed as it's already covered inside the test_add_inputs_default_value function and we here want to test the min/max conf params.

    ishaanam commented at 9:54 PM on October 25, 2022:

    This commit was authored by @champo before test_add_inputs_default_value was added. I have removed this test.

  36. furszy commented at 2:43 PM on October 22, 2022: member

    Concept ACK

    Left few nits, nothing important. Will do a deeper round in the coming days.

  37. in src/wallet/rpc/spend.cpp:1303 in a7ab0960ab outdated 
    1299 | @@ -1300,6 +1300,8 @@ RPCHelpMan sendall()
1300 |                          {"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"},
1301 |                          {"psbt", RPCArg::Type::BOOL,  RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."},
1302 |                          {"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."},
1303 | +                        {"minconf", RPCArg::Type::NUM, RPCArg::Default{1}, "If add_inputs is specified, require inputs with at least this confirmations."},
    furszy commented at 2:46 PM on October 22, 2022:

    I maybe missed it but where the minconf is set to 1 inside sendall? (default value is 0)

    ishaanam commented at 2:56 PM on October 22, 2022:

    The default minconf in sendall is 1 because sendall does not spend unconfirmed inputs.

    furszy commented at 3:17 PM on October 22, 2022:

    where is that established?

    let's say that the user does not pass minconf, so the default min_conf value inside coin control is 0. So, the AvailableCoins call will return outputs with depth 0 as well, which are added to the raw tx.

    ishaanam commented at 9:59 PM on October 25, 2022:

    AvailableCoins won't return unconfirmed utxos that aren't change unless include_unsafe is set to true. However, because this means that sendall can still spend unconfirmed change, which means that the default minconf is really 0. I have updated the docs to reflect this.

  38. in test/functional/rpc_psbt.py:134 in a7ab0960ab outdated 
     129 | +        # Make sure we only had the one input
 130 | +        tx1_inputs = self.nodes[0].decodepsbt(psbtx1)['tx']['vin']
 131 | +        assert_equal(len(tx1_inputs), 1)
 132 | +
 133 | +        utxo1 = tx1_inputs[0]
 134 | +        assert unconfirmed_txid == utxo1['txid']
    brunoerg commented at 2:53 PM on October 22, 2022: 
            assert_equal(unconfirmed_txid, utxo1['txid'])
    ishaanam commented at 9:59 PM on October 25, 2022:

    Done

  39. in test/functional/rpc_psbt.py:114 in a7ab0960ab outdated 
     105 | @@ -105,6 +106,70 @@ def test_utxo_conversion(self):
 106 |          self.connect_nodes(0, 1)
 107 |          self.connect_nodes(0, 2)
 108 |  
 109 | +    def test_input_confs_control(self):
 110 | +        self.nodes[0].createwallet("minconf")
 111 | +        wallet = self.nodes[0].get_wallet_rpc("minconf")
 112 | +
 113 | +        # Fund the wallet with different chain heights
 114 | +        self.nodes[1].sendtoaddress(wallet.getnewaddress(), 1)
    brunoerg commented at 2:55 PM on October 22, 2022:

    maybe you could use a for loop here to avoid repeat same code?

    ishaanam commented at 10:00 PM on October 25, 2022:

    Done

  40. ishaanam force-pushed on Oct 25, 2022
  41. ishaanam commented at 11:10 PM on October 25, 2022: contributor

    @furszy, @brunoerg, Thanks for the review, I have implemented all of your suggestions.

  42. in test/functional/rpc_fundrawtransaction.py:1351 in 0e1f99c4c3 outdated 
    1346 | +        # Fund the wallet with different chain heights
1347 | +        for _ in range(2):
1348 | +            self.nodes[2].sendmany("", {wallet.getnewaddress():1, wallet.getnewaddress():1})
1349 | +            self.generate(self.nodes[2], 1)
1350 | +
1351 | +        self.sync_blocks()
    furszy commented at 7:23 PM on October 26, 2022:

    in 0e1f99c4:

    nit: can remove the sync_blocks call. self.generate calls sync_all() internally if no sync function is provided.

    same in the other test

    ishaanam commented at 4:16 PM on October 29, 2022:

    Done

  43. furszy commented at 7:26 PM on October 26, 2022: member

    The RPC API changes will need release-notes too.

  44. ishaanam force-pushed on Oct 29, 2022
  45. ishaanam commented at 4:16 PM on October 29, 2022: contributor

    Added release notes.

  46. unknown approved
  48. in doc/release-notes-25375.md:4 in 4d7a1b6d72 outdated 
       0 | @@ -0,0 +1,11 @@
   1 | +Updated RPCs
   2 | +--------
   3 | +
   4 | +The `minconf`option, which allows a user to specify the minimum number
    brunoerg commented at 1:11 PM on October 31, 2022:

    nit:

    The `minconf` option, which allows a user to specify the minimum number
    ishaanam commented at 1:34 AM on November 6, 2022:

    Fixed

  49. ishaanam force-pushed on Nov 5, 2022
  50. in test/functional/rpc_fundrawtransaction.py:1384 in 44eb56e74e outdated 
    1373 | +        assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx2, {'add_inputs': True, 'minconf': 3, 'fee_rate': 10})
1374 | +
1375 | +        self.log.info("Fail to broadcast a new TX with maxconf 0 due to BIP125 rules to verify it actually chose unconfirmed outputs")
1376 | +        funded_invalid = wallet.fundrawtransaction(raw_tx2, {'add_inputs': True, 'maxconf': 0, 'fee_rate': 10})['hex']
1377 | +        final_invalid = wallet.signrawtransactionwithwallet(funded_invalid)['hex']
1378 | +        assert_raises_rpc_error(-26, "bad-txns-spends-conflicting-tx", self.nodes[0].sendrawtransaction, final_invalid)
    furszy commented at 3:04 PM on November 7, 2022:

    This test wasn't clear to me. It lacks of context description.

    At least for myself, will surely forget what it does and why it does it if we don't add some comments around.

    Maybe something like this:

    # Create a replacement tx to 'final_tx1' that has 1 BTC target instead of 0.1.
raw_tx2 = wallet.createrawtransaction([{'txid': utxo1['txid'], 'vout': utxo1['vout']}], {target_address: 1})
assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx2, {'add_inputs': True, 'minconf': 3, 'fee_rate': 10})

self.log.info("Fail to broadcast a new TX with maxconf 0 due to BIP125 rules to verify it actually chose unconfirmed outputs")
# Now fund 'raw_tx2' to fulfill the total target (1 BTC) by using all the wallet unconfirmed outputs.
# As it was created with the first unconfirmed output, 'raw_tx2' only has 0.1 BTC covered (need to fund 0.9 BTC more).
# So, the selection process, to cover the amount, will pick up the 'final_tx1' output as well, which is an output of the tx that this
# new tx is replacing!. So, once we send it to the mempool, it will return a "bad-txns-spends-conflicting-tx"
# because the input will no longer exist once the first tx gets replaced by this new one).
funded_invalid = wallet.fundrawtransaction(raw_tx2, {'add_inputs': True, 'maxconf': 0, 'fee_rate': 10})['hex']
final_invalid = wallet.signrawtransactionwithwallet(funded_invalid)['hex']
assert_raises_rpc_error(-26, "bad-txns-spends-conflicting-tx", self.nodes[0].sendrawtransaction, final_invalid)
    ishaanam commented at 8:01 PM on December 4, 2022:

    I have added this comment.

  51. ishaanam force-pushed on Dec 4, 2022
  52. DrahtBot added the label Needs rebase on Dec 9, 2022
  53. ishaanam force-pushed on Dec 10, 2022
  54. DrahtBot removed the label Needs rebase on Dec 10, 2022
  55. in src/wallet/rpc/spend.cpp:1304 in b3a3cb14bf outdated 
    1299 | @@ -1300,6 +1300,8 @@ RPCHelpMan sendall()
1300 |                          {"lock_unspents", RPCArg::Type::BOOL, RPCArg::Default{false}, "Lock selected unspent outputs"},
1301 |                          {"psbt", RPCArg::Type::BOOL,  RPCArg::DefaultHint{"automatic"}, "Always return a PSBT, implies add_to_wallet=false."},
1302 |                          {"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."},
1303 | +                        {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this confirmations."},
1304 | +                        {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this confirmations."},
    furszy commented at 12:50 PM on December 20, 2022:

    In b3a3cb14:

    sendall has no add_inputs argument. If inputs are specified, the command will only select them.

    ishaanam commented at 7:49 PM on December 20, 2022:

    Good catch, I have changed the docs to reflect this.

  56. furszy commented at 12:55 PM on December 20, 2022: member

    Almost there, code reviewed b3a3cb14. Left a last finding and it's ready to go.

  57. ishaanam force-pushed on Dec 20, 2022
  58. furszy approved
  59. furszy commented at 8:41 PM on December 20, 2022: member

    ACK 24c906b

  60. achow101 commented at 10:40 PM on December 20, 2022: member

    ACK 24c906b3311336ccaf5ea9c70da5ebde6f1cd041

  61. murchandamus commented at 9:28 PM on January 10, 2023: contributor

    I noticed that https://github.com/bitcoin/bitcoin/commit/1e14aeacd3ae86deb23ad4255cafbd0a3d9a894a added a test for send in test/functional/wallet_send.py, but that test does not seem to be part of the latest change set. Is it possible that the send test got lost accidentally or did I miss the reasoning why it got removed?

  62. ishaanam commented at 4:37 PM on January 11, 2023: contributor

    @Xekyo good catch. I replaced some of my commits with those from #22049 and it looks like that didn't include a test for send. I'll add that back soon.

  63. ishaanam force-pushed on Jan 11, 2023
  64. in src/wallet/rpc/spend.cpp:766 in 41ebc2b029 outdated 
     761 | @@ -744,6 +762,8 @@ RPCHelpMan fundrawtransaction()
 762 |                              {"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"
 763 |                                                            "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
 764 |                                                            "If that happens, you will need to fund the transaction with different inputs and republish it."},
 765 | +                            {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this confirmations."},
 766 | +                            {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this confirmations."},
    murchandamus commented at 9:13 PM on January 11, 2023:

    Nit if you need to update anything else. Here and in the other manual entries, this reads a bit odd to me. Maybe:

                                {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this many confirmations."},
                            {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this many confirmations."},
    ishaanam commented at 10:31 PM on January 11, 2023:

    I've changed all of them to this.

  65. in src/wallet/rpc/spend.cpp:1164 in 41ebc2b029 outdated 
    1159 | @@ -1140,6 +1160,8 @@ RPCHelpMan send()
1160 |                      {"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"
1161 |                                                            "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
1162 |                                                            "If that happens, you will need to fund the transaction with different inputs and republish it."},
1163 | +                    {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this confirmations."},
1164 | +                    {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this confirmations."},
    murchandamus commented at 9:17 PM on January 11, 2023:

    Nit as above:

                        {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this many confirmations."},
                    {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this many confirmations."},
    ishaanam commented at 10:31 PM on January 11, 2023:

    Done

  66. in src/wallet/rpc/spend.cpp:1643 in 41ebc2b029 outdated 
    1638 | @@ -1596,6 +1639,8 @@ RPCHelpMan walletcreatefundedpsbt()
1639 |                              {"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"
1640 |                                                            "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
1641 |                                                            "If that happens, you will need to fund the transaction with different inputs and republish it."},
1642 | +                            {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this confirmations."},
1643 | +                            {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this confirmations."},
    murchandamus commented at 9:18 PM on January 11, 2023:

    As above:

                                {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this many confirmations."},
                            {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this many confirmations."},
    ishaanam commented at 10:31 PM on January 11, 2023:

    Done

  67. murchandamus commented at 9:26 PM on January 11, 2023: contributor

    ACK 41ebc2b0294941d12980e19307bd991f9ff85301

  68. Wallet/RPC: Allow specifying min & max chain depth for inputs used by fund calls
    Enables users to craft BIP-125 replacements with changes to the output
list, ensuring that if additional funds are needed they will be added.
    a07a413466
  69. rpc: add minconf and maxconf options to sendall cfe5aebc79
  70. ishaanam force-pushed on Jan 11, 2023
  71. ishaanam commented at 10:31 PM on January 11, 2023: contributor

    Rebased for silent merge conflict.

  72. in src/wallet/rpc/spend.cpp:1399 in cfe5aebc79 
    1394 | +
1395 | +            if (options.exists("maxconf")) {
1396 | +                coin_control.m_max_depth = options["maxconf"].getInt<int>();
1397 | +
1398 | +                if (coin_control.m_max_depth < coin_control.m_min_depth) {
1399 | +                    throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("maxconf can't be lower than minconf: %d < %d", coin_control.m_max_depth, coin_control.m_min_depth));
    murchandamus commented at 2:28 PM on January 12, 2023:

    Note to myself/other reviewers . m_min_depth is initialized to DEFAULT_MIN_DEPTH = 0 in coincontrol.h, which is why this check of m_max_depth will always be defined and weed out negative numbers.

  73. murchandamus approved
  74. murchandamus commented at 4:39 PM on January 12, 2023: contributor

    ACK cfe5aebc79c510bd2156e199c3324d7ee1f8d2ad

  75. achow101 commented at 12:06 AM on January 13, 2023: member

    ACK cfe5aebc79c510bd2156e199c3324d7ee1f8d2ad

  76. in test/functional/wallet_send.py:494 in a07a413466 outdated 
     488 | @@ -487,6 +489,16 @@ def run_test(self):
 489 |          res = self.test_send(from_wallet=w5, to_wallet=w0, amount=1, include_unsafe=True)
 490 |          assert res["complete"]
 491 |  
 492 | +        self.log.info("Minconf")
 493 | +        self.nodes[1].createwallet(wallet_name="minconfw")
 494 | +        minconfw= self.nodes[1].get_wallet_rpc("minconfw")
    furszy commented at 10:23 PM on January 13, 2023:

    nit: missing space

            minconfw = self.nodes[1].get_wallet_rpc("minconfw")
  77. furszy approved
  78. furszy commented at 10:26 PM on January 13, 2023: member

    diff ACK cfe5aebc, only a non-blocking nit.

  79. achow101 merged this on Jan 16, 2023
  80. achow101 closed this on Jan 16, 2023
  81. sidhujag referenced this in commit 3961da0c3f on Jan 17, 2023
  82. in src/wallet/rpc/spend.cpp:766 in cfe5aebc79 
     761 | @@ -744,6 +762,8 @@ RPCHelpMan fundrawtransaction()
 762 |                              {"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"
 763 |                                                            "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
 764 |                                                            "If that happens, you will need to fund the transaction with different inputs and republish it."},
 765 | +                            {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this many confirmations."},
 766 | +                            {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "If add_inputs is specified, require inputs with at most this many confirmations."},
    maflcko commented at 4:08 PM on January 17, 2023:

    nit: Looks like this should be documented as (optional) in the RPC doc output. In theory this instance can be fixed by replacing OMITTED with OMITTED_NAMED_ARG, however a more complete fix is in #26706

    (Same for other "maxconf" in this pull)

  83. harding commented at 3:24 AM on January 23, 2023: contributor

    What was the motivation for the maxconf parameter? I'm guessing it's was the result of @achow101's comment, but I'm having trouble imagining a situation where someone might want to skip using UTXOs older than x blocks.

  84. ishaanam commented at 4:49 AM on January 23, 2023: contributor

    @harding It could be useful in the scenario in which someone doesn't want to reveal the age of their wallet (Eg. specify maxconf as 2016 confirmations so that the recipient doesn't know that the wallet dates all the way back to 10 years ago).

  85. luke-jr referenced this in commit ed41a10e83 on Jul 5, 2023
  86. luke-jr referenced this in commit 25f0423a27 on Jul 5, 2023
  87. ishaanam deleted the branch on Nov 30, 2023
  88. bitcoin locked this on Nov 29, 2024
Contributors
ishaanamDrahtBotachow101w0xltluke-jrghostfurszybrunoergmurchandamusmaflckoharding


achow101

Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#14542 fundrawtransaction - select input transaction with minimum confirmations#14641 rpc: Add min_conf option to 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-04-13 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me