[rpc] don't automatically append inputs in walletcreatefundedpsbt

Sjors commented at 11:40 am on July 12, 2019: member

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.

Sjors force-pushed on Jul 12, 2019

fanquake added the label RPC/REST/ZMQ on Jul 12, 2019

fanquake added the label Wallet on Jul 12, 2019

Sjors force-pushed on Jul 12, 2019

DrahtBot commented at 1:26 pm on July 12, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
#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 commented at 3:06 pm on July 12, 2019: member

concept ACK

in src/wallet/coincontrol.h:24 in 4bf5d6abc9 outdated

19@@ -20,6 +20,8 @@ class CCoinControl
20     CTxDestination destChange;
21     //! Override the default change type if set, ignored if destChange is set
22     boost::optional<OutputType> m_change_type;
23+    //! If false, only selected inputs are used
24+    bool fAddInputs;

promag commented at 10:30 pm on July 14, 2019:

nit, bool m_add_inputs.

in src/wallet/rpcwallet.cpp:2996 in 4bf5d6abc9 outdated

2992@@ -2991,6 +2993,9 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
2993             },
2994             true, true);
2995 
2996+        if (options.exists("addInputs") )

promag commented at 10:31 pm on July 14, 2019:

nits, “add_inputs”. Remove whitespace at the end. Add { }.

in src/wallet/wallet.cpp:2499 in 4bf5d6abc9 outdated

2494@@ -2495,6 +2495,11 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
2495             continue;
2496 
2497         for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
2498+            // Only consider selected coins if addInputs is false
2499+            if(coinControl && !coinControl->fAddInputs && !coinControl->IsSelected(COutPoint(entry.first, i))) {

promag commented at 10:32 pm on July 14, 2019:

nit, space after if

in src/wallet/rpcwallet.cpp:2960 in 4bf5d6abc9 outdated

2956@@ -2957,13 +2957,14 @@ static UniValue listunspent(const JSONRPCRequest& request)
2957     return results;
2958 }
2959 
2960-void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options)
2961+void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, bool default_add_inputs)

promag commented at 10:44 pm on July 14, 2019:

Instead of adding parameters why not receive the coin control?

promag commented at 10:44 pm on July 14, 2019: member

Concept ACK.

Sjors commented at 8:15 am on July 29, 2019: member

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

Sjors force-pushed on Jul 29, 2019

achow101 commented at 8:43 pm on July 30, 2019: member

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

Sjors commented at 4:34 pm on August 1, 2019: member

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

achow101 commented at 5:20 pm on August 1, 2019: member

@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 force-pushed on Aug 2, 2019

Sjors renamed this:
~~[rpc] don't automatically append inputs in walletcreatefundedpsbt~~
[rpc] don't automatically append inputs in walletcreatefundedpsbt & fundrawtransaction
on Aug 2, 2019

Sjors commented at 9:53 am on August 2, 2019: member

I changed fundrawtransaction to have the same behavior.

DrahtBot added the label Needs rebase on Aug 2, 2019

Sjors force-pushed on Aug 2, 2019

DrahtBot removed the label Needs rebase on Aug 2, 2019

in src/wallet/rpcwallet.cpp:3132 in f82a770152 outdated

3110@@ -3107,6 +3111,7 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
3111                     {"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
3112                     {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}",
3113                         {
3114+                            {"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "For a transaction with existing inputs, automatically include more if they are not enough."},

luke-jr commented at 0:01 am on August 20, 2019:

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

Sjors commented at 8:43 am on August 21, 2019:

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.

achow101 commented at 5:09 pm on June 11, 2020:

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 commented at 9:29 am on August 21, 2019: none

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 commented at 9:52 am on August 21, 2019: member

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

Fonta1n3 commented at 11:31 am on August 21, 2019: none

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

DrahtBot added the label Needs rebase on Oct 30, 2019

Sjors force-pushed on Oct 30, 2019

DrahtBot removed the label Needs rebase on Oct 30, 2019

Sjors renamed this:
~~[rpc] don't automatically append inputs in walletcreatefundedpsbt & fundrawtransaction~~
[rpc] don't automatically append inputs in walletcreatefundedpsbt
on Feb 20, 2020

Sjors force-pushed on Feb 20, 2020

Sjors commented at 3:20 pm on February 20, 2020: member

I switched the default for fundrawtransaction to false, to keep it backwards compatible; I’m not familiar with how people use that RPC in practice. For walletcreatefundedpsbt I do think this change makes it safer.

Sjors force-pushed on Feb 20, 2020

in doc/release-notes-16377.md:3 in 5fc79ef2ff outdated

0@@ -0,0 +1,6 @@
1+RPC changes
2+-----------
3+The `walletcreatefundedpsbt` RPC call will fail with

kallewoof commented at 5:39 am on March 6, 2020:

The phrasing is vague on whether this is talking about previous or new behavior. Maybe RPC call will now fail with?

in doc/release-notes-16377.md:5 in 5fc79ef2ff outdated

0@@ -0,0 +1,6 @@
1+RPC changes
2+-----------
3+The `walletcreatefundedpsbt` RPC call will fail with
4+`Insufficient funds` when inputs are manually selected but are not enough to cover
5+the outputs and fee. Additional inputs can automatically be added through a

kallewoof commented at 5:40 am on March 6, 2020:

a -> the

in test/functional/rpc_psbt.py:148 in 5fc79ef2ff outdated

143@@ -137,13 +144,13 @@ def run_test(self):
144         self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex'])
145 
146         # feeRate of 0.1 BTC / KB produces a total fee slightly below -maxtxfee (~0.05280000):
147-        res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1})
148+        res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1, "add_inputs": True})

kallewoof commented at 5:52 am on March 6, 2020:

I know the two asserts below this are not yours, but they can use assert_approx to achieve the same result with less confusion.

0assert_approx(res["fee"], 0.055, 0.005)

Sjors commented at 8:16 pm on March 11, 2020:

I’ll use that.

in test/functional/rpc_psbt.py:153 in 5fc79ef2ff outdated

150         assert_greater_than(0.06, res["fee"])
151 
152         # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee
153         # previously this was silently capped at -maxtxfee
154-        assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10})
155+        assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10, "add_inputs": True})

kallewoof commented at 5:53 am on March 6, 2020:

Perhaps duplicate this with "add_inputs": False case as well.

Sjors commented at 8:22 pm on March 11, 2020:

Done

in test/functional/rpc_psbt.py:232 in 5fc79ef2ff outdated

219@@ -213,31 +220,31 @@ def run_test(self):
220         # replaceable arg
221         block_height = self.nodes[0].getblockcount()
222         unspent = self.nodes[0].listunspent()[0]
223-        psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable": False}, False)
224+        psbtx_info = self.nodes[0].walletcreatefundedpsbt([{"txid":unspent["txid"], "vout":unspent["vout"]}], [{self.nodes[2].getnewaddress():unspent["amount"]+1}], block_height+2, {"replaceable": False, "add_inputs": True}, False)

kallewoof commented at 5:54 am on March 6, 2020:

Can this be tested for the "add_inputs": False case as well?

Sjors commented at 8:23 pm on March 11, 2020:

I don’t think that’s very useful, because the goal of the test is to “Make sure both pre-included and funded inputs have the correct sequence numbers”, and "add_inputs": False would just test a subset of what we already check here.

kallewoof approved

kallewoof commented at 5:55 am on March 6, 2020: member

ACK 5fc79ef2ff8065d78de339f4b4e935f67ec3211d

DrahtBot added the label Needs rebase on Mar 11, 2020

in src/wallet/rpcwallet.cpp:3054 in 5fc79ef2ff outdated

3029@@ -3030,6 +3030,10 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
3030             },
3031             true, true);
3032 
3033+        if (options.exists("add_inputs") ) {
3034+            coinControl.m_add_inputs = options["add_inputs"].get_bool();

promag commented at 7:23 pm on March 11, 2020:

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

Sjors commented at 8:11 pm on March 11, 2020:

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

Sjors commented at 8:27 pm on March 11, 2020: member

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

Sjors force-pushed on Mar 11, 2020

DrahtBot removed the label Needs rebase on Mar 11, 2020

in doc/release-notes-16377.md:6 in 9d0ef2a54e outdated

0@@ -0,0 +1,6 @@
1+RPC changes
2+-----------
3+The `walletcreatefundedpsbt` RPC call will now fail with
4+`Insufficient funds` when inputs are manually selected but are not enough to cover
5+the outputs and fee. Additional inputs can automatically be added through the
6+new `add_inputs` option.

promag commented at 10:39 pm on March 11, 2020:

Maybe also add something like

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

Sjors commented at 12:07 pm on March 12, 2020:

Done

in src/wallet/rpcwallet.cpp:3234 in 9d0ef2a54e outdated

3228@@ -3224,7 +3229,10 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
3229 
3230     CAmount fee;
3231     int change_position;
3232-    FundTransaction(pwallet, tx, fee, change_position, request.params[1]);
3233+    CCoinControl coin_control;
3234+    // Automatically select (additional) coins. Can be overriden by options.add_inputs.
3235+    coin_control.m_add_inputs = true;

promag commented at 10:42 pm on March 11, 2020:

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

Sjors commented at 12:04 pm on March 12, 2020:

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

promag commented at 10:36 am on June 12, 2020:

e5327f947c310849e1ddbb24321e4c9f85564549

nit

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

in src/wallet/rpcwallet.cpp:4160 in 9d0ef2a54e outdated

4153@@ -4146,10 +4154,10 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
4154     }
4155 
4156             RPCHelpMan{"walletcreatefundedpsbt",
4157-                "\nCreates and funds a transaction in the Partially Signed Transaction format. Inputs will be added if supplied inputs are not enough\n"
4158+                "\nCreates and funds a transaction in the Partially Signed Transaction format.\n"
4159                 "Implements the Creator and Updater roles.\n",
4160                 {
4161-                    {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs",
4162+                    {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs. Leave empty to add inputs automatically.",

promag commented at 10:43 pm on March 11, 2020:

nit, maybe add See add_inputs option.

Sjors commented at 12:07 pm on March 12, 2020:

Done

promag commented at 10:44 pm on March 11, 2020: member

Looks good, just some small suggestions.

[rpc] walletcreatefundedpsbt: don't automatically append inputs

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.

79804fe24b

[rpc] fundrawtransaction: add_inputs option to control automatic input adding e5327f947c

Sjors force-pushed on Mar 12, 2020

sipa commented at 8:39 pm on March 25, 2020: member

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 commented at 10:33 am on March 26, 2020: member

@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 commented at 5:24 pm on June 11, 2020: member

ACK e5327f947c310849e1ddbb24321e4c9f85564549

Sjors requested review from promag on Jun 12, 2020

in src/wallet/rpcwallet.cpp:3151 in e5327f947c

3147@@ -3148,8 +3148,8 @@ static UniValue fundrawtransaction(const JSONRPCRequest& request)
3148     }
3149 
3150     RPCHelpMan{"fundrawtransaction",
3151-                "\nAdd inputs to a transaction until it has enough in value to meet its out value.\n"
3152-                "This will not modify existing inputs, and will add at most one change output to the outputs.\n"
3153+                "\nIf the transaction has no inputs, they will be automatically selected to meet its out value.\n"

promag commented at 10:38 am on June 12, 2020:

e5327f947c310849e1ddbb24321e4c9f85564549

Previous text was fine?

Sjors commented at 12:34 pm on June 12, 2020:

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.

in src/wallet/coincontrol.cpp:13 in 79804fe24b outdated

 9@@ -10,6 +10,7 @@ void CCoinControl::SetNull()
10 {
11     destChange = CNoDestination();
12     m_change_type.reset();
13+    m_add_inputs = true;

promag commented at 10:44 am on June 12, 2020:

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

Sjors commented at 12:58 pm on June 12, 2020:

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.

in src/wallet/rpcwallet.cpp:3019 in 79804fe24b outdated

3015@@ -3016,13 +3016,12 @@ static UniValue listunspent(const JSONRPCRequest& request)
3016     return results;
3017 }
3018 
3019-void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options)
3020+void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options, CCoinControl& coinControl)

promag commented at 10:44 am on June 12, 2020:

79804fe24bd00e183382dfbcab9343960d158aa5

nit, unrelated, could receive options by reference.

promag commented at 10:48 am on June 12, 2020: member

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.

Sjors commented at 12:59 pm on June 12, 2020: member

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

in src/wallet/wallet.cpp:2153 in e5327f947c

2148@@ -2149,6 +2149,11 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<
2149         }
2150 
2151         for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) {
2152+            // Only consider selected coins if add_inputs is false
2153+            if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(COutPoint(entry.first, i))) {

promag commented at 11:42 am on June 15, 2020:

ultra nit, cache IsSelected() by adding above:

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

then use this in L2160 below.

meshcollider added this to the "PRs" column in a project

meshcollider commented at 8:37 am on June 21, 2020: contributor

utACK e5327f947c310849e1ddbb24321e4c9f85564549

meshcollider merged this on Jun 21, 2020

meshcollider closed this on Jun 21, 2020

Sjors deleted the branch on Jun 21, 2020

sidhujag referenced this in commit 8b7f46f92c on Jul 7, 2020

meshcollider moved this from the "PRs" to the "Done" column in a project

Fonta1n3 commented at 5:26 am on September 7, 2020: none

Which version of Bitcoin Core will this get released on?

sipa commented at 5:27 am on September 7, 2020: member

Current master branch will become 0.21.

meshcollider referenced this in commit ffaac6e614 on Sep 15, 2020

sidhujag referenced this in commit 2d9d86f6b8 on Sep 15, 2020

Fabcien referenced this in commit a3c6da56e3 on Dec 11, 2020

Fabcien referenced this in commit 1b8a6cbb05 on Dec 11, 2020

DrahtBot locked this on Feb 15, 2022

[rpc] don’t automatically append inputs in walletcreatefundedpsbt #16377

Conflicts