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

ishaanam · 2022-06-15T00:08:34Z

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

DrahtBot · 2022-06-15T08:28:24Z

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

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.

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.

ghost

ACK 0f023da

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

achow101 · 2022-06-18T14:19:45Z

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.

w0xlt

Code Review ACK 0f023da

w0xlt · 2022-06-18T16:41:57Z

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.

luke-jr · 2022-06-18T21:44:14Z

This seems like a subset of #22049

ishaanam · 2022-06-20T17:15:39Z

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

ishaanam · 2022-07-11T08:50:05Z

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.

ishaanam · 2022-08-15T20:58:28Z

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

luke-jr · 2022-09-28T18:23:39Z

@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

ghost · 2022-09-29T17:44:03Z

@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

ishaanam · 2022-09-29T20:47:33Z

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

ghost

reACK 3f0f67c

ishaanam · 2022-10-21T18:07:10Z

Rebased

test/functional/rpc_fundrawtransaction.py

furszy

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

src/wallet/rpc/spend.cpp

furszy

ACK 24c906b

achow101 · 2022-12-20T22:40:58Z

ACK 24c906b3311336ccaf5ea9c70da5ebde6f1cd041

murchandamus

I noticed that 1e14aea 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?

ishaanam · 2023-01-11T16:37:30Z

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

murchandamus

ACK 41ebc2b0294941d12980e19307bd991f9ff85301

src/wallet/rpc/spend.cpp

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

ishaanam · 2023-01-11T22:31:19Z

Rebased for silent merge conflict.

murchandamus

ACK cfe5aeb

murchandamus · 2023-01-12T14:28:42Z

src/wallet/rpc/spend.cpp

+                if (coin_control.m_max_depth < coin_control.m_min_depth) {
+                    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));


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.

achow101 · 2023-01-13T00:06:12Z

ACK cfe5aeb

furszy

diff ACK cfe5aeb, only a non-blocking nit.

furszy · 2023-01-13T22:23:35Z

test/functional/wallet_send.py


+        self.log.info("Minconf")
+        self.nodes[1].createwallet(wallet_name="minconfw")
+        minconfw= self.nodes[1].get_wallet_rpc("minconfw")


nit:
missing space

Suggested change

minconfw= self.nodes[1].get_wallet_rpc("minconfw")

minconfw = self.nodes[1].get_wallet_rpc("minconfw")

…fund tr…

maflcko · 2023-01-17T16:08:25Z

src/wallet/rpc/spend.cpp

                                                          "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
                                                          "If that happens, you will need to fund the transaction with different inputs and republish it."},
+                            {"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."},


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)

harding · 2023-01-23T03:24:56Z

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.

ishaanam · 2023-01-23T04:49:58Z

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

… 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. Github-Pull: bitcoin#25375 Rebased-From: 3e14de57e0c1bd7692deadfde9f99b1683e5becd

Github-Pull: bitcoin#25375 Rebased-From: 18e277fcf261ebb66cf7e8a25b380e4d6da9111f

DrahtBot added RPC/REST/ZMQ Wallet labels Jun 15, 2022

DrahtBot mentioned this pull request Jun 15, 2022

rpc: Filter inputs by type during CoinSelection #25183

Closed

ghost approved these changes Jun 17, 2022

View reviewed changes

w0xlt approved these changes Jun 18, 2022

View reviewed changes

ishaanam force-pushed the fundrawtransaction_minconf branch from 0f023da to 1e14aea Compare June 20, 2022 16:47

ishaanam closed this Aug 15, 2022

ishaanam reopened this Sep 29, 2022

ishaanam force-pushed the fundrawtransaction_minconf branch 3 times, most recently from 7e24831 to 3f0f67c Compare September 29, 2022 20:47

ghost reviewed Sep 29, 2022

View reviewed changes

ishaanam changed the title ~~rpc: add minconf option to sendall and fund transaction calls~~ rpc: add minconf/maxconf options to sendall and fund transaction calls Oct 1, 2022

glozow requested review from achow101 and murchandamus October 12, 2022 19:35

DrahtBot mentioned this pull request Oct 19, 2022

wallet: Fix sendall with watchonly wallets and specified inputs #26344

Merged

DrahtBot added the Needs rebase label Oct 21, 2022

ishaanam force-pushed the fundrawtransaction_minconf branch from 3f0f67c to a7ab096 Compare October 21, 2022 17:17

DrahtBot removed the Needs rebase label Oct 21, 2022

furszy reviewed Nov 7, 2022

View reviewed changes

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

ishaanam force-pushed the fundrawtransaction_minconf branch from f38eb81 to 18e277f Compare December 4, 2022 20:01

DrahtBot added the Needs rebase label Dec 9, 2022

ishaanam force-pushed the fundrawtransaction_minconf branch from 18e277f to b3a3cb1 Compare December 10, 2022 16:10

DrahtBot removed the Needs rebase label Dec 10, 2022

furszy reviewed Dec 20, 2022

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

ishaanam force-pushed the fundrawtransaction_minconf branch from b3a3cb1 to 24c906b Compare December 20, 2022 19:31

furszy approved these changes Dec 20, 2022

View reviewed changes

DrahtBot mentioned this pull request Dec 21, 2022

[WIP] wallet: tx creation, don't select outputs from txes that are being replaced #26732

Closed

murchandamus reviewed Jan 10, 2023

View reviewed changes

ishaanam force-pushed the fundrawtransaction_minconf branch from 24c906b to 41ebc2b Compare January 11, 2023 20:37

murchandamus reviewed Jan 11, 2023

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

champo and others added 2 commits January 11, 2023 17:08

Wallet/RPC: Allow specifying min & max chain depth for inputs used by…

a07a413

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

rpc: add minconf and maxconf options to sendall

cfe5aeb

ishaanam force-pushed the fundrawtransaction_minconf branch from 41ebc2b to cfe5aeb Compare January 11, 2023 22:10

murchandamus approved these changes Jan 12, 2023

View reviewed changes

furszy approved these changes Jan 13, 2023

View reviewed changes

achow101 merged commit b55b11f into bitcoin:master Jan 16, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 17, 2023

Merge bitcoin#25375: rpc: add minconf/maxconf options to sendall and …

3961da0

…fund tr…

maflcko reviewed Jan 17, 2023

View reviewed changes

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jul 5, 2023

rpc: add minconf and maxconf options to sendall

25f0423

Github-Pull: bitcoin#25375 Rebased-From: 18e277fcf261ebb66cf7e8a25b380e4d6da9111f

ishaanam deleted the fundrawtransaction_minconf branch November 30, 2023 03:18

bitcoin locked and limited conversation to collaborators Nov 29, 2024

		if (coin_control.m_max_depth < coin_control.m_min_depth) {
		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));

	minconfw= self.nodes[1].get_wallet_rpc("minconfw")
	minconfw = self.nodes[1].get_wallet_rpc("minconfw")

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

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

Uh oh!

Conversation

ishaanam commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 18, 2022

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Jun 18, 2022

Uh oh!

luke-jr commented Jun 18, 2022

Uh oh!

ishaanam commented Jun 20, 2022

Uh oh!

ishaanam commented Jul 11, 2022

Uh oh!

ishaanam commented Aug 15, 2022

Uh oh!

luke-jr commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 29, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ishaanam commented Sep 29, 2022

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Oct 21, 2022

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 20, 2022

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

ishaanam commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ishaanam commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

murchandamus left a comment

Choose a reason for hiding this comment

Uh oh!

murchandamus Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jan 13, 2023

Uh oh!

furszy 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

ishaanam commented Jun 15, 2022 •

edited

Loading

DrahtBot commented Jun 15, 2022 •

edited

Loading

ghost left a comment •

edited by ghost

Loading

luke-jr commented Sep 28, 2022 •

edited

Loading

ghost commented Sep 29, 2022 •

edited by ghost

Loading

ishaanam commented Jan 11, 2023 •

edited

Loading

ishaanam commented Jan 11, 2023 •

edited

Loading

furszy left a comment •

edited

Loading