[rpc] don't automatically append inputs in walletcreatefundedpsbt by Sjors · Pull Request #16377 · bitcoin/bitcoin

Sjors · 2019-07-12T11:40:44Z

When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, walletcreatefundedpsbt now fails if the amount is insufficient, unless addInputs is set to true.

Similarly for fundrawtransaction if the original transaction already specified inputs, we only add more if addInputs is set to true.

This protects against fat finger mistakes in the amount or fee rate (see also #16257). The behavior is also more similar to GUI coin selection.

DrahtBot · 2019-07-12T13:26:28Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #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.

instagibbs · 2019-07-12T15:06:57Z

concept ACK

promag

Concept ACK.

src/wallet/coincontrol.h

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

src/wallet/rpcwallet.cpp

Sjors · 2019-07-29T08:15:24Z

Instead of bool default_add_inputs into FundTransaction I'm now passing a CCoinControl object. Also addressed nits.

achow101 · 2019-07-30T20:43:30Z

This effects fundrawtransaction too, so documentation should be updated to indicate that.

Sjors · 2019-08-01T16:34:37Z

@achow101 I didn't change this behavior in fundrawtransaction.

achow101 · 2019-08-01T17:20:54Z

@achow101 I didn't change this behavior in fundrawtransaction.

Yes, but FundTransaction is used by both fundrawtransaction and walletcreatefundedpsbt. The add_inputs option is now something that both can have, but is not documented for fundrawtransaction. Furthermore, both of these do pretty much the same thing, just return different formats. So I think their behavior with this option should be the same.

Sjors · 2019-08-02T09:53:43Z

I changed fundrawtransaction to have the same behavior.

luke-jr · 2019-08-20T00:01:22Z

src/wallet/rpcwallet.cpp

This is backward-incompatible, and it makes no sense for this RPC which does exactly only that? Confused.

See @achow101's comment above: #16377 (comment)

I tend to agree that walletcreatefundedpspbt is different from fundrawtransaction in this aspect, because walletcreatefundedpspbt also sets the destination.

Would like to hear some more opinions, otherwise I'll drop the last commit (where this is added). Or I can change the default for fundrawtransaction but keep the option, in case it's useful.

I think both walletcreatefundedpsbt and fundrawtransaction are both most commonly used with no inputs. As I understand it, with add_inputs defaulting to false, this behavior doesn't change. Even so, the option is useful in fundrawtransaction as sometimes you just want it to set the change output and fee for a transaction after you've already selected the inputs.

Fonta1n3 · 2019-08-21T09:29:34Z

I have built an app on top of bitcoin-cli and definitely would prefer no inputs to be added at all by default when using walletcreatefundedpsbt and fundrawtransaction if a user specifies their own inputs. If a user is specifying inputs then it would be better to force them to recreate a transaction with the necessary inputs rather than automatically adding additional inputs the user has not specified.

It makes fee optimization a lot easier in this scenario where a user wants to specify inputs as one can not take advantage of the conf_target argument or other Bitcoin Core fee optimization arguments when building custom transactions with createpsbt and createrawtransaction.

Sjors · 2019-08-21T09:52:05Z

@FontaineDenton does your application use walletcreatefundedpsbt or fundrawtransaction? And can you elaborate on fee optimization?

Fonta1n3 · 2019-08-21T11:31:27Z

@FontaineDenton does your application use walletcreatefundedpsbt or fundrawtransaction? And can you elaborate on fee optimization?

I just updated my original comment so it hopefully makes more sense.

promag · 2020-03-11T19:23:36Z

src/wallet/rpcwallet.cpp

Makes sense to allow add_inputs = false when no input is manually selected?

It'll just fail with insufficient funds. I don't think it's necessary to special-case that.

Sjors · 2020-03-11T20:27:14Z

Rebased, slightly tweaked one test and added a new one.

promag

Looks good, just some small suggestions.

promag · 2020-03-11T22:39:52Z

doc/release-notes-16377.md

Maybe also add something like

The `fundrawtransaction` RPC now supports `add_inputs` option that when `false` prevents adding more inputs if necessary and consequently the RPC fails.

promag · 2020-03-11T22:42:16Z

src/wallet/rpcwallet.cpp

This is already the default value in coin_control so maybe remove this?

I think it adds clarity that this (default) value matters for this caller.

e5327f9

nit

// By default new inputs are added automatically. Can be overriden by options.add_inputs. CHECK_NONFATAL(coin_control.m_add_inputs);

promag · 2020-03-11T22:43:13Z

src/wallet/rpcwallet.cpp

nit, maybe add See add_inputs option.

When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true.

…t adding

sipa · 2020-03-25T20:39:41Z

With add_inputs set to false, how does walletcreatefundedpsbt differ from createpsbt? If there is no difference, it seems the default should always be true.

Sjors · 2020-03-26T10:33:52Z

@sipa the difference is when when you don't specify inputs. walletcreatefundedpsbt will pick inputs from your wallet, createpsbt won't.

If you do specificy inputs, then by default (as of this PR) neither command will add them. But they're still different, because createpsbt doesn't have wallet context to fill out redeem/witness scripts.

I didn't even realise createpsbt existed up to now :-)

achow101 · 2020-06-11T17:24:12Z

ACK e5327f9

promag

Code review ACK but left some comments. This looks great and gives more control to the caller.

The RPC bumpfee also adds inputs (or reduces change output) so maybe we could add this option there too in a follow-up.

promag · 2020-06-12T10:36:07Z

src/wallet/rpcwallet.cpp

e5327f9

nit

// By default new inputs are added automatically. Can be overriden by options.add_inputs. CHECK_NONFATAL(coin_control.m_add_inputs);

promag · 2020-06-12T10:38:43Z

src/wallet/rpcwallet.cpp

    RPCHelpMan{"fundrawtransaction",
-                "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n"
-                "This will not modify existing inputs, and will add at most one change output to the outputs.\n"
+                "\nIf the transaction has no inputs, they will be automatically selected to meet its out value.\n"


e5327f9

Previous text was fine?

No, the previous text suggests that it always adds inputs. I reworded that to make it clear we only do automatic coin selection if there is no manual coin selection.

promag · 2020-06-12T10:44:19Z

src/wallet/coincontrol.cpp

 {
    destChange = CNoDestination();
    m_change_type.reset();
+    m_add_inputs = true;


Haven't tested and looks like nobody asked, but can't we use fAllowOtherInputs instead?

That won't work. I think fAllowOtherInputs was designed with RBF in mind. It permits adding coins, as long as all the currently selected coins are included. But this PR tries to prevent adding coins.

promag · 2020-06-12T10:44:59Z

src/wallet/rpcwallet.cpp

 }

-void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options)
+void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl)


79804fe

nit, unrelated, could receive options by reference.

Sjors · 2020-06-12T12:59:43Z

I'll leave the CHECK_NONFATAL in case I need to rebase or change something.

promag · 2020-06-15T11:42:37Z

src/wallet/wallet.cpp


        for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
+            // Only consider selected coins if add_inputs is false
+            if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(entry.first, i))) {


ultra nit, cache IsSelected() by adding above:

const bool is_selected = coinControl && coinControl->IsSelected(COutPoint(entry.first, i));

then use this in L2160 below.

meshcollider

utACK e5327f9

…tcreatefundedpsbt e5327f9 [rpc] fundrawtransaction: add_inputs option to control automatic input adding (Sjors Provoost) 79804fe [rpc] walletcreatefundedpsbt: don't automatically append inputs (Sjors Provoost) Pull request description: When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, `walletcreatefundedpsbt` now fails if the amount is insufficient, unless `addInputs` is set to `true`. Similarly for `fundrawtransaction` if the original transaction already specified inputs, we only add more if `addInputs` is set to `true`. This protects against fat finger mistakes in the amount or fee rate (see also bitcoin#16257). The behavior is also more similar to GUI coin selection. ACKs for top commit: achow101: ACK e5327f9 meshcollider: utACK e5327f9 Tree-SHA512: d8653b820914396c7c25b0d0a2b7e92de214aa023bc1aa085feb37d3b20fab361ebea90416a7db989f19bdc37e26cf0adfbcb712c80985c87afa67a9bd44fecb

Fonta1n3 · 2020-09-07T05:26:44Z

Which version of Bitcoin Core will this get released on?

sipa · 2020-09-07T05:27:35Z

Current master branch will become 0.21.

92326d8 [rpc] add send method (Sjors Provoost) 2c2a144 [rpc] add snake case aliases for transaction methods (Sjors Provoost) 1bc8d0f [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost) Pull request description: `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have: * manual coin selection * outputting a PSBT (it was controversial to add this, see #18201) * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`) At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases. This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format. Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see #16546). For `bitcoin-cli` users, it tries to keep the simplest use case easy to use: ```sh bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b ``` This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address. Depends on: - [x] #16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`) - [x] #11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`) - [x] #18244 (`[rpc] have lockUnspents also lock manually selected coins`) ACKs for top commit: meshcollider: Light re-utACK 92326d8 achow101: ACK 92326d8 Reviewed code and test, ran tests. kallewoof: utACK 92326d8 Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f

92326d8 [rpc] add send method (Sjors Provoost) 2c2a144 [rpc] add snake case aliases for transaction methods (Sjors Provoost) 1bc8d0f [rpc] walletcreatefundedpsbt: allow inputs to be null (Sjors Provoost) Pull request description: `walletcreatefundedpsbt` has some interesting features that `sendtoaddress` and `sendmany` don't have: * manual coin selection * outputting a PSBT (it was controversial to add this, see bitcoin#18201) * create a transaction without adding to wallet (which leads to broadcasting, unless `-walletbroadcast=0`) At the same time `walletcreatefundedpsbt` can't broadcast a transaction, which is inconvenient for simple use cases. This PR introduces a new `send` RPC method which creates a PSBT, signs it if possible and adds it to the wallet by default. If it can't sign all inputs, it outputs a PSBT. If `add_to_wallet` is set to `false` it will return the transaction in both PSBT and hex format. Because it uses a PSBT internally, it will much easier to add hardware wallet support to this method (see bitcoin#16546). For `bitcoin-cli` users, it tries to keep the simplest use case easy to use: ```sh bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b ``` This paves the way for deprecating `sendtoaddress` and `sendmany` though there's no rush. The only missing feature compared to these older methods is adding labels to a destination address. Depends on: - [x] bitcoin#16377 (`[rpc] don't automatically append inputs in walletcreatefundedpsbt`) - [x] bitcoin#11413 (`[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option`) - [x] bitcoin#18244 (`[rpc] have lockUnspents also lock manually selected coins`) ACKs for top commit: meshcollider: Light re-utACK 92326d8 achow101: ACK 92326d8 Reviewed code and test, ran tests. kallewoof: utACK 92326d8 Tree-SHA512: 7552ef1b193d4c06e381c44932fdb0d54f64383e4c7d6b988f49d059c7d4bba45ce6aa7813e03df86360ad9dad6f3010eb76ee7da480551742d5fd98c2251c0f

… append inputs Summary: When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true. --- Backport of Core [[bitcoin/bitcoin#16377 | PR16377]] [1/2] Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8659

…ntrol automatic input adding Summary: --- Backport of Core [[bitcoin/bitcoin#16377 | PR16377]] Depends on D8659 Note: replaced a couple unused `i` for-loop variables in functional tests that the linter complained about with `_`'s Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D8660

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from a74855c to e87e089 Compare July 12, 2019 11:41

fanquake added RPC/REST/ZMQ Wallet labels Jul 12, 2019

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from e87e089 to 4bf5d6a Compare July 12, 2019 12:30

Sjors mentioned this pull request Jul 12, 2019

The ultimate send RPC #16378

Merged

3 tasks

promag reviewed Jul 14, 2019

View reviewed changes

src/wallet/coincontrol.h Outdated Show resolved Hide resolved

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

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

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from 4bf5d6a to d8e51a2 Compare July 29, 2019 08:15

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from d8e51a2 to 6769883 Compare August 2, 2019 09:51

Sjors changed the title ~~[rpc] don't automatically append inputs in walletcreatefundedpsbt~~ [rpc] don't automatically append inputs in walletcreatefundedpsbt & fundrawtransaction Aug 2, 2019

DrahtBot added the Needs rebase label Aug 2, 2019

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from 6769883 to f82a770 Compare August 2, 2019 15:50

DrahtBot removed the Needs rebase label Aug 2, 2019

luke-jr reviewed Aug 20, 2019

View reviewed changes

DrahtBot added the Needs rebase label Oct 30, 2019

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from f82a770 to 21af29a Compare October 30, 2019 19:34

DrahtBot removed the Needs rebase label Oct 30, 2019

Sjors mentioned this pull request Nov 4, 2019

Enable BnB coin selection for preset inputs and subtract fee from outputs #17290

Merged

DrahtBot mentioned this pull request Feb 11, 2020

Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211

Merged

Sjors changed the title ~~[rpc] don't automatically append inputs in walletcreatefundedpsbt & fundrawtransaction~~ [rpc] don't automatically append inputs in walletcreatefundedpsbt Feb 20, 2020

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from 21af29a to 914009d Compare February 20, 2020 15:18

promag reviewed Mar 11, 2020

View reviewed changes

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from 5fc79ef to 9d0ef2a Compare March 11, 2020 20:27

DrahtBot removed the Needs rebase label Mar 11, 2020

promag reviewed Mar 11, 2020

View reviewed changes

Sjors added 2 commits March 12, 2020 13:07

[rpc] walletcreatefundedpsbt: don't automatically append inputs

79804fe

When the user doesn't specificy inputs, it makes sense to automatically select them. But when the user does specify inputs, we now fail if the amount is insufficient, unless addInputs is set to true.

[rpc] fundrawtransaction: add_inputs option to control automatic inpu…

e5327f9

…t adding

Sjors force-pushed the 2019/07/walletcreatefundedpsbt_addinputs branch from 9d0ef2a to e5327f9 Compare March 12, 2020 12:07

DrahtBot mentioned this pull request Mar 25, 2020

Use shared pointers only in validation interface #18354

Closed

This was referenced Apr 5, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

rpc: replace raw pointers with shared_ptrs #18592

Closed

Sjors requested a review from promag June 12, 2020 09:07

promag reviewed Jun 12, 2020

View reviewed changes

promag reviewed Jun 15, 2020

View reviewed changes

meshcollider reviewed Jun 21, 2020

View reviewed changes

meshcollider merged commit 6bb5f6d into bitcoin:master Jun 21, 2020

Sjors deleted the 2019/07/walletcreatefundedpsbt_addinputs branch June 21, 2020 15:04

ben-kaufman mentioned this pull request Jan 21, 2021

Coin selection for unconfirmed utxos doesn't work cryptoadvance/specter-desktop#886

Open

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

Sjors commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

instagibbs commented Jul 12, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Jul 29, 2019

Uh oh!

achow101 commented Jul 30, 2019

Uh oh!

Sjors commented Aug 1, 2019

Uh oh!

achow101 commented Aug 1, 2019

Uh oh!

Sjors commented Aug 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fonta1n3 commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Aug 21, 2019

Uh oh!

Fonta1n3 commented Aug 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Mar 11, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Mar 25, 2020

Uh oh!

Sjors commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jun 11, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Sjors commented Jul 12, 2019 •

edited

Loading

DrahtBot commented Jul 12, 2019 •

edited

Loading

Sjors Aug 21, 2019 •

edited

Loading

Fonta1n3 commented Aug 21, 2019 •

edited

Loading

promag Mar 11, 2020 •

edited

Loading

Sjors commented Mar 26, 2020 •

edited

Loading

promag Jun 12, 2020 •

edited

Loading