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

champo commented at 5:53 PM on May 24, 2021: contributor

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

rpc: allow specifying min chain depth for inputs in 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.

6086c58d27

DrahtBot added the label RPC/REST/ZMQ on May 24, 2021

DrahtBot added the label Wallet on May 24, 2021

in test/functional/rpc_fundrawtransaction.py:981 in 6086c58d27 outdated

 976 | +        unconfirmedTxid = self.nodes[0].sendtoaddress(unconfirmedAddress, 1)
 977 | +
 978 | +        utxo1 = self.nodes[0].listunspent(0, 1, [unconfirmedAddress])[0]
 979 | +        assert unconfirmedTxid == utxo1['txid']
 980 | +
 981 | +        self.log.info("Crafting PSBT using an unconfirmed input")

achow101 commented at 8:04 PM on May 24, 2021:

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

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

Here and elsewhere in this file too.

in src/wallet/rpcwallet.cpp:3227 in 6086c58d27 outdated

3223 | @@ -3215,6 +3224,7 @@ static RPCHelpMan fundrawtransaction()
3224 |                              {"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"
3225 |                                                            "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
3226 |                                                            "If that happens, you will need to fund the transaction with different inputs and republish it."},
3227 | +                            {"inputs_min_depth", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this chain depth."},

promag commented at 8:06 PM on May 24, 2021:

IMO should be minconf, see listunspent RPC.

achow101 commented at 8:06 PM on May 24, 2021: member

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?

promag commented at 8:07 PM on May 24, 2021: member

Concept ACK.

champo commented at 8:14 PM on May 24, 2021: contributor

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!

test: fix inputs_min_depth test cases after rebase 91c0a3af93

rpc: rename inputs_min_depth to minconfs for consistency with listunspent 87371bf119

DrahtBot commented at 9:53 PM on May 24, 2021: contributor

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.

rpc: add maxconfs parameter in fund calls 290f9dad44

champo commented at 10:13 PM on May 24, 2021: contributor

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.

scripted-diff: drop trailing s from maxconfs and minconfs

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

7f4c9039f7

in src/wallet/rpcwallet.cpp:4066 in 290f9dad44 outdated

4062 | @@ -4043,6 +4063,8 @@ static RPCHelpMan send()
4063 |                      {"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"
4064 |                                                            "Warning: the resulting transaction may become invalid if one of the unsafe inputs disappears.\n"
4065 |                                                            "If that happens, you will need to fund the transaction with different inputs and republish it."},
4066 | +                    {"minconfs", RPCArg::Type::NUM, RPCArg::Default{0}, "If add_inputs is specified, require inputs with at least this confirmations."},

promag commented at 10:59 PM on May 24, 2021:

It's singular: https://github.com/bitcoin/bitcoin/blob/b295395664bd37e26d168c329f238237b34aef8c/src/wallet/rpcwallet.cpp#L2844-L2845

luke-jr commented at 4:32 AM on June 25, 2021: member

Duplicate of #14641 ?

DrahtBot commented at 8:26 AM on August 16, 2021: contributor

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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

DrahtBot added the label Needs rebase on Aug 16, 2021

DrahtBot commented at 12:28 PM on December 22, 2021: contributor

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 commented at 11:08 PM on February 28, 2022: member

Rebased & squashed here: https://github.com/bitcoin/bitcoin/compare/master...luke-jr:rpc_fundtx_minmaxconf

laanwj commented at 10:00 AM on June 22, 2022: member

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 commented at 9:21 PM on August 15, 2022: member

@champo would you like to follow up here?

glozow commented at 10:01 AM on September 26, 2022: member

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!

glozow closed this on Sep 26, 2022

bitcoin locked this on Sep 26, 2023