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

champo · 2021-05-24T17:53:59Z

Enables users to craft BIP-125 replacements with changes to the output
list, ensuring that if additional funds are inputs they will be added.

Enables users to craft BIP-125 replacements with changes to the output list, ensuring that if additional funds are needed they will be added.

achow101

Seems reasonable, although the use case for this is not immediately obvious to me.

Perhaps it would also be useful to add inputs_max_depth too?

achow101 · 2021-05-24T20:04:07Z

test/functional/rpc_fundrawtransaction.py

+        utxo1 = self.nodes[0].listunspent(0, 1, [unconfirmedAddress])[0]
+        assert unconfirmedTxid == utxo1['txid']
+
+        self.log.info("Crafting PSBT using an unconfirmed input")


In 6086c58 "rpc: allow specifying min chain depth for inputs in fund calls"

Suggested change

self.log.info("Crafting PSBT using an unconfirmed input")

self.log.info("Crafting tx using an unconfirmed input")

Here and elsewhere in this file too.

promag

Concept ACK.

promag · 2021-05-24T20:06:54Z

src/wallet/rpcwallet.cpp

                            {"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"
                                                          "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."},
+                            {"inputs_min_depth", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this chain depth."},


IMO should be minconf, see listunspent RPC.

champo · 2021-05-24T20:14:31Z

Seems reasonable, although the use case for this is not immediately obvious to me.

The use case is for applications that are using RBF for something else than bumping fee (as implemented by bumpfee RPC). RBF requires new inputs added to a replacement to be confirmed. The functional tests are modeled after one of the use cases: increasing the value of an output beyond the current funding of the TX.

Perhaps it would also be useful to add inputs_max_depth too?

Will do!

…pent

DrahtBot · 2021-05-24T21:53:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22686 (wallet: Use GetSelectionAmount in ApproximateBestSubset by achow101)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)

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.

champo · 2021-05-24T22:13:55Z

Thanks for the quick look guys! I've updated the changset with your feedback. I'm not sure what the ettiquete is here regarding squashing mid-review, so I opted to add new commits. LMK if this is not what you expect.

promag · 2021-05-24T22:59:08Z

src/wallet/rpcwallet.cpp

                    {"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"
                                                          "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."},
+                    {"minconfs", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this confirmations."},


It's singular:

bitcoin/src/wallet/rpcwallet.cpp

Lines 2844 to 2845 in b295395

{"minconf", RPCArg::Type::NUM, RPCArg::Default{1}, "The minimum confirmations to filter"},

{"maxconf", RPCArg::Type::NUM, RPCArg::Default{9999999}, "The maximum confirmations to filter"},

-BEGIN VERIFY SCRIPT- sed -i"" -e "s/minconfs/minconf/" ./src/wallet/rpcwallet.cpp ./test/functional/rpc_fundrawtransaction.py ./test/functional/rpc_psbt.py sed -i"" -e "s/maxconfs/maxconf/" ./src/wallet/rpcwallet.cpp ./test/functional/rpc_fundrawtransaction.py ./test/functional/rpc_psbt.py -END VERIFY SCRIPT-

luke-jr · 2021-06-25T04:32:21Z

Duplicate of #14641 ?

DrahtBot · 2021-08-16T08:26:49Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

DrahtBot · 2021-12-22T12:28:48Z

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.

luke-jr · 2022-02-28T23:08:42Z

Rebased & squashed here: master...luke-jr:rpc_fundtx_minmaxconf

laanwj · 2022-06-22T10:00:30Z

Code review ACK. Needs rebase though.

It looks like some commits can be squashed? E.g. the scripted-diff commit seems to only change code that was introduced in this same PR.

fanquake · 2022-08-15T21:21:22Z

@champo would you like to follow up here?

glozow · 2022-09-26T10:01:20Z

Closing as this has needed rebase for more than 1 year. Feel free to reopen if you get a chance to work on this again in the future, thanks!

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

6086c58

Enables users to craft BIP-125 replacements with changes to the output list, ensuring that if additional funds are needed they will be added.

DrahtBot added RPC/REST/ZMQ Wallet labels May 24, 2021

achow101 reviewed May 24, 2021

View reviewed changes

promag reviewed May 24, 2021

View reviewed changes

champo added 2 commits May 24, 2021 17:45

test: fix inputs_min_depth test cases after rebase

91c0a3a

rpc: rename inputs_min_depth to minconfs for consistency with listuns…

87371bf

…pent

rpc: add maxconfs parameter in fund calls

290f9da

promag reviewed May 24, 2021

View reviewed changes

DrahtBot mentioned this pull request May 25, 2021

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

DrahtBot mentioned this pull request Aug 12, 2021

wallet: Use GetSelectionAmount in ApproximateBestSubset #22686

Merged

DrahtBot added the Needs rebase label Aug 16, 2021

luke-jr mentioned this pull request Jun 18, 2022

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

Merged

glozow closed this Sep 26, 2022

bitcoin locked and limited conversation to collaborators Sep 26, 2023

	self.log.info("Crafting PSBT using an unconfirmed input")
	self.log.info("Crafting tx using an unconfirmed input")

	{"minconf", RPCArg::Type::NUM, RPCArg::Default{1}, "The minimum confirmations to filter"},
	{"maxconf", RPCArg::Type::NUM, RPCArg::Default{9999999}, "The maximum confirmations to filter"},

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

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

Uh oh!

Conversation

champo commented May 24, 2021

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 May 24, 2021

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag May 24, 2021

Choose a reason for hiding this comment

Uh oh!

champo commented May 24, 2021

Uh oh!

DrahtBot commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

champo commented May 24, 2021

Uh oh!

promag May 24, 2021

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jun 25, 2021

Uh oh!

DrahtBot commented Aug 16, 2021

Uh oh!

DrahtBot commented Dec 22, 2021

Uh oh!

luke-jr commented Feb 28, 2022

Uh oh!

laanwj commented Jun 22, 2022

Uh oh!

fanquake commented Aug 15, 2022

Uh oh!

glozow commented Sep 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DrahtBot commented May 24, 2021 •

edited

Loading