The ultimate send RPC

Sjors commented at 1:56 pm on July 12, 2019: member

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:

0bitcoin-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:

#16377 ([rpc] don't automatically append inputs in walletcreatefundedpsbt)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option)
#18244 ([rpc] have lockUnspents also lock manually selected coins)

Sjors force-pushed on Jul 12, 2019

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

in src/wallet/rpcwallet.cpp:4345 in 5136b7316e outdated

4193+        },
4194+        RPCResult{
4195+                    "{\n"
4196+                    "  \"complete\" : true|false, (boolean) If the transaction has a complete set of signatures\n"
4197+                    "  \"txid\" :                 (string) The transaction id for the send. Only 1 transaction is created regardless of \n"
4198+                    "                                      the number of addresses.\n",

instagibbs commented at 2:15 pm on July 12, 2019:

as noted in my other PR, I think broadcast is the wrong word here. It’s moreso that we do not commit it to the wallet, which also broadcasts it

Sjors commented at 2:28 pm on July 12, 2019:

I’m open to other terms, maybe commit? But that seems less clear.

The background motivation here is that an RPC consumer may need certainty about whether the transaction failed or succeeded. For example this should even work if the RPC connection gets cut before the call returns.

Just retrying is unsafe. So right now the only option is to check if the wallet contains transactions to the expected destination(s). With this flag, the RPC consumer gets full control and can for example commit the full transaction to some redundant database before broadcasting it.

(and maybe in future we support wallet-less spend based on just a descriptor with private keys, where there’s no way to know if a previous attempt failed, since you can’t rely on the local mempool for that)

instagibbs commented at 2:49 pm on July 12, 2019:

“broadcast” already has meaning to users unfortunately e.g., walletbroadcast startup arg

Sjors commented at 3:25 pm on July 13, 2019:

That’s a rather scary startup option imo; forget it once and everything gets broadcast?

laanwj commented at 12:51 pm on August 1, 2019:

That’s a rather scary startup option imo; forget it once and everything gets broadcast?

This is definitely an option, like the proxy configuration, one’d want to have in the bitcoin.conf and not on the command line!

But also, not necessarily: the idea is that when you have it enabled, you broadcast wallet transactions manually (e.g. by pasting them into the transaction broadcast of a block explorer, or a script such as bitcoin-submittx over tor) so they end up in the mempool or block chain through some other route. There’s no reason for the wallet to broadcast them then.

Sjors commented at 5:04 pm on August 1, 2019:

The option I introduce here doesn’t even add the transaction to the wallet; it only returns the hex. So it goes beyond walletbroadcast=0 afaik.

Sjors commented at 4:34 pm on August 2, 2019:

I renamed broadcast to add_to_wallet and explained it a bit better in the docs.

DrahtBot commented at 4:18 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:

#18788 (tests: Update more tests to work with descriptor wallets 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.

laanwj added the label Wallet on Jul 15, 2019

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

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

Suggest moving “inputs” to a field on “options”, and removing PSBT output from send (there is already a RPC for it, as you note).

Sjors commented at 8:56 am on August 21, 2019: member

@luke-jr agree about moving the inputs. That also makes it easier to rebase on #11413 (explicit fee rate) and have a syntax like send 0.001 BTC bc1... 1 sat/B.

The reason I added PSBT output is to make multisig easier. When used with a descriptor wallet #15764 you simply call send and it will sign its own part and give you a PSBT to give to the other party (haven’t tested this yet though). With the existing psbt methods that’s more tedious.

Sjors commented at 10:34 am on August 26, 2019: member

Closing in favor of tweaking sendmany / sendtoaddress:

#11413 adds fee rate support (1 sat/b) without the need for an options JSON
@luke-jr suggested moving coin selection into an options JSON (it’s a more advanced feature than fee selection, so that’s fine by me)
PSBT output can be added when we need it, same for the option to avoid committing transaction to wallet

Sjors closed this on Aug 26, 2019

Sjors reopened this on Feb 24, 2020

Sjors force-pushed on Feb 24, 2020

Sjors commented at 9:28 pm on February 24, 2020: member

And we’re back! I tried to make the simplest use case, sending coins to a single address with a manual fee in satoshi per byte, easy for bitcoin-cli users. This means you can set conf_target and estimate_mode either in the options dictionary or as a positional argument.

0bitcoin-cli -regtest send '{"ADDRESS": 0.1}' 1 sat/b

Ideally I’d also get rid of the JSON encoding, but at least this RPC example is easy enough for a new bitcoin-cli user to copy-paste and tweak. And meanwhile GUI PSBT support has made enough progress.

This needs some cleanup, but I suggest focussing initial review on:

the dependencies: #16377 and #11413 (I’ll have to adjust some things depending on those)
concept ACKs and/or suggestions for the RPC interface @luke-jr suggested:

Suggest moving “inputs” to a field on “options”

Done

Sjors force-pushed on Feb 25, 2020

Sjors referenced this in commit aee8d81378 on Feb 25, 2020

Sjors force-pushed on Mar 2, 2020

Sjors renamed this:
~~[WIP] The ultimate send RPC~~
The ultimate send RPC
on Mar 2, 2020

Sjors commented at 5:37 pm on March 2, 2020: member

This now has enough test coverage that I consider it review worthy, but note that it depends on 3 other pull requests.

Sjors force-pushed on Mar 4, 2020

DrahtBot added the label Needs rebase on Mar 7, 2020

promag commented at 3:07 pm on March 15, 2020: member

Concept ACK.

Sjors referenced this in commit 24cba79301 on Mar 23, 2020

Sjors referenced this in commit f3450a0186 on Mar 27, 2020

Sjors referenced this in commit 7669405234 on Apr 1, 2020

Sjors referenced this in commit 0ec75f1b32 on Apr 22, 2020

Sjors referenced this in commit 192d1203f8 on Apr 22, 2020

Sjors referenced this in commit 45839a57e5 on Apr 23, 2020

Sjors referenced this in commit 2313f9ab09 on Apr 27, 2020

Sjors referenced this in commit f94e94c37c on Apr 27, 2020

Sjors referenced this in commit 372eef41f0 on Apr 27, 2020

Sjors referenced this in commit 09d61da0fc on Apr 27, 2020

Sjors referenced this in commit 2988c2fda1 on May 7, 2020

Sjors referenced this in commit e8fc838858 on May 18, 2020

Sjors referenced this in commit 87a29a1e68 on May 22, 2020

Sjors referenced this in commit 4b6fb767b2 on Jun 10, 2020

Sjors referenced this in commit fc46eb2202 on Jun 16, 2020

Sjors referenced this in commit 4bb5361ae7 on Jun 18, 2020

Sjors referenced this in commit 29f6508c28 on Jun 19, 2020

Sjors referenced this in commit 42cc844f65 on Jun 19, 2020

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

Sjors referenced this in commit b5e02cd2f3 on Jun 25, 2020

Sjors force-pushed on Jun 25, 2020

Sjors commented at 7:01 pm on June 25, 2020: member

Rebased after #16377 and #11413, it should be easier to review now.

DrahtBot removed the label Needs rebase on Jun 25, 2020

Sjors referenced this in commit 53f13c8467 on Jun 29, 2020

Sjors referenced this in commit 4b02413733 on Jul 2, 2020

Sjors referenced this in commit 6d7475cffa on Jul 6, 2020

Sjors referenced this in commit 9b8ddbcc1c on Jul 14, 2020

Sjors referenced this in commit bef5f38822 on Jul 18, 2020

Sjors referenced this in commit 70b728651d on Jul 30, 2020

Sjors referenced this in commit 7ec42c1748 on Jul 31, 2020

Sjors referenced this in commit f324534295 on Aug 6, 2020

Sjors force-pushed on Aug 7, 2020

Sjors commented at 3:37 pm on August 7, 2020: member

Rebased and tests should pass now. Some things that I think are worth discussing:

CommitTransaction() happily accepts transactions with nLockTime in the future. It’s not clear to me if those eventually get broadcast; in the functional test I had to re-submit it manually. This could be tackled in another PR though.
We return a transaction hash or a PSBT, in part depending on magic. You can opt out of this magic by setting the psbt option to true (which implies add_to_wallet=false, but in addition to the serialised hex it returns a PSBT even for a fully signed transaction)
A hex serialized transaction is only returned when setting add_to_wallet to false, so there’s no magic there. But depending on how (1) is handled, this could change, e.g. it could return a hex for transaction with (far) future nLockTime (not my preference though).

in src/wallet/rpcwallet.cpp:3836 in 15ba29aafe outdated

3832@@ -3818,6 +3833,186 @@ static UniValue listlabels(const JSONRPCRequest& request)
3833     return ret;
3834 }
3835 
3836+UniValue send(const JSONRPCRequest& request)

MarcoFalke commented at 12:21 pm on August 14, 2020:

Might use the non-deprecated constructor. E.g. #19528

Sjors commented at 4:19 pm on August 28, 2020:

I switched to the fancy new thing.

in src/wallet/rpcwallet.cpp:3884 in 15ba29aafe outdated

3879+                    },
3880+                    {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
3881+                    {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
3882+                    {"psbt", RPCArg::Type::BOOL,  /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."},
3883+                    {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n"
3884+                    "                              The fee will be equally deducted from the amount of each specified output.\n"

MarcoFalke commented at 12:22 pm on August 14, 2020:

0                    "The fee will be equally deducted from the amount of each specified output.\n"

No need for the whitespace

in src/wallet/rpcwallet.cpp:3924 in 15ba29aafe outdated

3919+        }, true
3920+    );
3921+
3922+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
3923+    if (!wallet) return NullUniValue;
3924+    CWallet* const pwallet = wallet.get();

MarcoFalke commented at 12:22 pm on August 14, 2020:

As the previous line already checks for nullptr, this can be a reference.

0    CWallet& wallet = pwallet.get();

Sjors commented at 4:06 pm on August 28, 2020:

Do you mean const CWallet& pwallet = wallet.get(); ? This creates a const mess when calling FundTransaction (which I’d rather not refactor in this PR).

MarcoFalke commented at 12:23 pm on August 14, 2020: member

some style-nits to consider next time you rebase

Sjors force-pushed on Aug 28, 2020

Sjors referenced this in commit 5dbd6d683b on Aug 28, 2020

Sjors force-pushed on Aug 31, 2020

Sjors commented at 11:56 am on August 31, 2020: member

Rebased after #18244 landed.

in src/wallet/rpcwallet.cpp:3915 in 27c07f0866 outdated

3910+                        },
3911+                    },
3912+                },
3913+            },
3914+            {"conf_target", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
3915+            {"estimate_mode", RPCArg::Type::STR, RPCArg::Optional::OMITTED, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"

meshcollider commented at 0:33 am on September 7, 2020:

Because these are Optional::OMITTED, the “default” is actually the description string, so the only output is:

02. conf_target                           (numeric) wallet default
13. estimate_mode                         (string) unset

Sjors commented at 6:57 pm on September 7, 2020:

I dropped the Optional::OMITTED bit, not super intuitive… (cc @MarcoFalke)

02. conf_target                           (numeric, optional, default=wallet default) Confirmation target (in blocks), or fee rate (for BTC/kB or sat/B estimate modes)
13. estimate_mode                         (string, optional, default=unset) The fee estimate mode, must be one of (case insensitive):
2                                         "unset"
3                                         "economical"
4                                         "conservative"
5                                         "BTC/kB"
6                                         "sat/B"
74. options                               (json object, optional)

in src/wallet/rpcwallet.cpp:3958 in 27c07f0866 outdated

3953+        RPCResult{
3954+            RPCResult::Type::OBJ, "", "",
3955+                {
3956+                    {RPCResult::Type::BOOL, "complete", "If the transaction has a complete set of signatures"},
3957+                    {RPCResult::Type::STR_HEX, "txid", "The transaction id for the send. Only 1 transaction is created regardless of \n"
3958+                                                       "the number of addresses.\n"},

meshcollider commented at 0:36 am on September 7, 2020:

nit: unnecessary newline

meshcollider commented at 0:52 am on September 7, 2020: contributor

utACK 27c07f08662a8044c729d0adead0511984c40cef

Only lightly read through the test. Ran the functional tests. LGTM. Just a couple of helptext comments inline. Thanks Sjors!

Sjors referenced this in commit 867727da91 on Sep 7, 2020

[rpc] walletcreatefundedpsbt: allow inputs to be null

This is of neglible use here, but it allows new RPC methods to take outputs as their first argument and make inputs optional.

1bc8d0fd59

[rpc] add snake case aliases for transaction methods 2c2a1445dc

Sjors force-pushed on Sep 7, 2020

in src/wallet/rpcwallet.cpp:3999 in 62d3523e21 outdated

3994+            int change_position;
3995+            bool rbf = pwallet->m_signal_rbf;
3996+            const UniValue &replaceable_arg = options["replaceable"];
3997+            if (!replaceable_arg.isNull()) {
3998+                RPCTypeCheckArgument(replaceable_arg, UniValue::VBOOL);
3999+                rbf = replaceable_arg.isTrue();

meshcollider commented at 2:41 am on September 10, 2020:

Why is this check so much more involved than just if exists() and get_bool() like you do with add_to_wallet below?

Sjors commented at 11:44 am on September 10, 2020:

Because I blindly copied it from walletcreatefundedpsbt :-)

Replaced with simpler version.

[rpc] add send method 92326d8976

Sjors force-pushed on Sep 10, 2020

in src/wallet/rpcwallet.cpp:3946 in 92326d8976

3941+                }
3942+        },
3943+        RPCExamples{""
3944+        "\nSend with a fee rate of 1 satoshi per byte\n"
3945+        + HelpExampleCli("send", "'{\"" + EXAMPLE_ADDRESS[0] + "\": 0.1}' 1 sat/b\n" +
3946+            "\nCreate a transaction that should confirm the next block, with a specific input, and return result without adding to wallet or broadcasting to the network\n")

ShaunApps commented at 1:13 pm on September 10, 2020:

nit: parenthesis ‘)’ in wrong place, enclosing HelpExampleCli and next example string

Sjors commented at 12:58 pm on September 17, 2020:

Thanks, adding fix to followup PR.

ShaunApps commented at 1:14 pm on September 10, 2020: none

ACK.

Ran functional tests. Played w/ command in cli. Left a nit

laanwj added this to the "Blockers" column in a project

meshcollider commented at 0:40 am on September 11, 2020: contributor

Light re-utACK 92326d89766155a792254d30a9962251b8fc7799

(still haven’t read the test carefully)

in src/wallet/rpcwallet.cpp:2971 in 92326d8976

2967+                {"inputs", UniValueType(UniValue::VARR)},
2968                 {"lockUnspents", UniValueType(UniValue::VBOOL)},
2969                 {"lock_unspents", UniValueType(UniValue::VBOOL)},
2970-                {"feeRate", UniValueType()}, // will be checked below
2971+                {"locktime", UniValueType(UniValue::VNUM)},
2972+                {"feeRate", UniValueType()}, // will be checked below,

achow101 commented at 0:44 am on September 15, 2020:

nit: Extra trailing comma

Sjors commented at 12:59 pm on September 17, 2020:

Thanks, fixing nits in followup.

in src/wallet/rpcwallet.cpp:4014 in 92326d8976

4009+            }
4010+
4011+            // Make a blank psbt
4012+            PartiallySignedTransaction psbtx(rawTx);
4013+
4014+            // Fill transaction with out data and sign

achow101 commented at 1:06 am on September 15, 2020:

nit: s/out/our

in src/wallet/rpcwallet.cpp:4027 in 92326d8976

4022+            complete = FinalizeAndExtractPSBT(psbtx, mtx);
4023+
4024+            UniValue result(UniValue::VOBJ);
4025+
4026+            // Serialize the PSBT
4027+            CDataStream ssTx(SER_NETWORK, PROTOCOL_VERSION);

achow101 commented at 1:08 am on September 15, 2020:

Seems like this serialization should only occur if we are actually going to return the psbt, so move it inside the next if block?

in test/functional/wallet_send.py:33 in 92326d8976

28+        self.skip_if_no_wallet()
29+
30+    def test_send(self, from_wallet, to_wallet=None, amount=None, data=None,
31+                  arg_conf_target=None, arg_estimate_mode=None,
32+                  conf_target=None, estimate_mode=None, add_to_wallet=None,psbt=None,
33+                  inputs=None,add_inputs=None,change_address=None,change_position=None,change_type=None,

achow101 commented at 1:10 am on September 15, 2020:

nit: spaces after each arg, here and below

Sjors commented at 1:13 pm on September 17, 2020:

,[a-zA-Z0-9]+ found a bunch more

in src/wallet/rpcwallet.cpp:3901 in 92326d8976

3896+            {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
3897+                        "       \"" + FeeModes("\"\n\"") + "\""},
3898+            {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
3899+                {
3900+                    {"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "If inputs are specified, automatically include more if they are not enough."},
3901+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},

achow101 commented at 1:15 am on September 15, 2020:

Seems like it would be better to call this broadcast rather than add_to_wallet.

Sjors commented at 1:14 pm on September 17, 2020:

Maybe, but this also works when walletbroadcast is off

in test/functional/wallet_send.py:256 in 92326d8976

251+        fee = self.nodes[1].decodepsbt(res["psbt"])["fee"]
252+        assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00002"))
253+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, conf_target=-1, estimate_mode="sat/b",
254+                       expect_error=(-3, "Amount out of range"))
255+        # Fee rate of 0.1 satoshi per byte should throw an error
256+        # TODO: error should say 1.000 sat/b

achow101 commented at 1:21 am on September 15, 2020:

I think that’s supposed to be 0.1, not 1.000

achow101 commented at 1:33 am on September 15, 2020: member

ACK 92326d89766155a792254d30a9962251b8fc7799 Reviewed code and test, ran tests.

Just some nits and non-blocking comments.

in src/rpc/rawtransaction_util.cpp:24 in 1bc8d0fd59 outdated

20@@ -21,10 +21,15 @@
21 
22 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf)
23 {
24-    if (inputs_in.isNull() || outputs_in.isNull())
25-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, arguments 1 and 2 must be non-null");
26+    if (outputs_in.isNull())

kallewoof commented at 2:19 am on September 15, 2020:

[rpc] walletcreatefundedpsbt: allow inputs to be null:

Style-nit: add {} while at it, since you’re touching this line.

Sjors commented at 1:17 pm on September 17, 2020:

Will change in the followup in a separate commit, in case people think it’s too late to touch.

in src/rpc/rawtransaction_util.cpp:28 in 1bc8d0fd59 outdated

25-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, arguments 1 and 2 must be non-null");
26+    if (outputs_in.isNull())
27+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null");
28+
29+    UniValue inputs;
30+    if (inputs_in.isNull())

kallewoof commented at 2:19 am on September 15, 2020:

[rpc] walletcreatefundedpsbt: allow inputs to be null:

Add {}s.

in src/wallet/rpcwallet.cpp:3885 in 92326d8976

3880+                    "That is, each address can only appear once and there can only be one 'data' object.\n"
3881+                    "For convenience, a dictionary, which holds the key-value pairs directly, is also accepted.",
3882+                {
3883+                    {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
3884+                        {
3885+                            {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""},

kallewoof commented at 2:36 am on September 15, 2020:

RPCArg::Type::AMOUNT is not a key-value pair. I think you mean RPCArg::Type::OBJ.

in src/wallet/rpcwallet.cpp:3890 in 92326d8976

3885+                            {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT + ""},
3886+                        },
3887+                        },
3888+                    {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
3889+                        {
3890+                            {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"},

kallewoof commented at 2:38 am on September 15, 2020:

RPCArg::Type::STR_HEX is not a key-value pair either. I think you mean RPCArg::Type::OBJ here too.

Sjors commented at 7:23 pm on September 17, 2020:

I think the type is correct, see also walletcreatefundedpsbt, but it’s confusing (in the code) that the wrapping object is described next to the data field. cc @achow101 / @MarcoFalke thoughts?

achow101 commented at 7:34 pm on September 17, 2020:

The type is correct because it is describing the type of the value in this key-value pair. The weirdness is that this output specification is the almost the only place where the key is not fixed but user defined.

in src/wallet/rpcwallet.cpp:3906 in 92326d8976

3901+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},
3902+                    {"change_address", RPCArg::Type::STR_HEX, /* default */ "pool address", "The bitcoin address to receive the change"},
3903+                    {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
3904+                    {"change_type", RPCArg::Type::STR, /* default */ "set by -changetype", "The output type to use. Only valid if change_address is not specified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\"."},
3905+                    {"conf_target", RPCArg::Type::NUM, /* default */ "wallet default", "Confirmation target (in blocks), or fee rate (for " + CURRENCY_UNIT + "/kB or " + CURRENCY_ATOM + "/B estimate modes)"},
3906+                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"

kallewoof commented at 2:40 am on September 15, 2020:

Why are there two conf_target and estimate_mode parameters?

Sjors commented at 1:20 pm on September 17, 2020:

For convenience; setting the fee rate is so common I’d rather not have to use the options field.

in src/wallet/rpcwallet.cpp:4009 in 92326d8976

4004+            FundTransaction(pwallet, rawTx, fee, change_position, options, coin_control);
4005+
4006+            bool add_to_wallet = true;
4007+            if (options.exists("add_to_wallet")) {
4008+                add_to_wallet = options["add_to_wallet"].get_bool();
4009+            }

kallewoof commented at 2:44 am on September 15, 2020:

Alternative:

0bool add_to_wallet = !options.exists("add_to_wallet") || options["add_to_wallet"].get_bool();

Sjors commented at 1:20 pm on September 17, 2020:

I’ll leave this to your #19957

kallewoof commented at 0:12 am on September 18, 2020:

Feels out of scope for #19957, and not a big deal anyway so leaving it be.

kallewoof commented at 2:47 am on September 15, 2020: member

utACK 92326d89766155a792254d30a9962251b8fc7799

meshcollider commented at 2:51 am on September 15, 2020: contributor

Nits can be left for followup, let’s get this in to move #16546 forward 🚀

meshcollider merged this on Sep 15, 2020

meshcollider closed this on Sep 15, 2020

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

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

meshcollider removed this from the "Blockers" column in a project

in src/wallet/rpcwallet.cpp:2980 in 2c2a1445dc outdated

2976@@ -2972,22 +2977,24 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
2977             coinControl.m_add_inputs = options["add_inputs"].get_bool();
2978         }
2979 
2980-        if (options.exists("changeAddress")) {
2981-            CTxDestination dest = DecodeDestination(options["changeAddress"].get_str());
2982+        if (options.exists("changeAddress") || options.exists("change_address")) {

fjahr commented at 1:58 pm on September 15, 2020:

Would be more robust to throw an error if both options are set.

Sjors commented at 1:21 pm on September 17, 2020:

I believe that’s covered in #19957

in src/wallet/rpcwallet.cpp:3879 in 92326d8976

3870@@ -3866,6 +3871,185 @@ static UniValue listlabels(const JSONRPCRequest& request)
3871     return ret;
3872 }
3873 
3874+static RPCHelpMan send()
3875+{
3876+    return RPCHelpMan{"send",
3877+        "\nSend a transaction.\n",
3878+        {
3879+            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "a json array with outputs (key-value pairs), where none of the keys are duplicated.\n"

fjahr commented at 2:01 pm on September 15, 2020:

nit: JSON, Capitalize A

Sjors commented at 1:22 pm on September 17, 2020:

I’ll address nits in the followup PR

in src/wallet/rpcwallet.cpp:3921 in 92326d8976

3916+                        },
3917+                    },
3918+                    {"locktime", RPCArg::Type::NUM, /* default */ "0", "Raw locktime. Non-0 value also locktime-activates inputs"},
3919+                    {"lock_unspents", RPCArg::Type::BOOL, /* default */ "false", "Lock selected unspent outputs"},
3920+                    {"psbt", RPCArg::Type::BOOL,  /* default */ "automatic", "Always return a PSBT, implies add_to_wallet=false."},
3921+                    {"subtract_fee_from_outputs", RPCArg::Type::ARR, /* default */ "empty array", "A json array of integers.\n"

fjahr commented at 2:04 pm on September 15, 2020:

nit: JSON

in src/wallet/rpcwallet.cpp:3911 in 92326d8976

3906+                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
3907+            "       \"" + FeeModes("\"\n\"") + "\""},
3908+                    {"include_watching", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Also select inputs which are watch only.\n"
3909+                                          "Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n"
3910+                                          "e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."},
3911+                    {"inputs", RPCArg::Type::ARR, /* default */ "empty array", "Specify inputs instead of adding them automatically. A json array of json objects",

fjahr commented at 2:05 pm on September 15, 2020:

nit: JSON

in src/wallet/rpcwallet.cpp:3997 in 92326d8976

3992+
3993+            CAmount fee;
3994+            int change_position;
3995+            bool rbf = pwallet->m_signal_rbf;
3996+            if (options.exists("replaceable")) {
3997+                rbf = options["add_to_wallet"].get_bool();

fjahr commented at 2:09 pm on September 15, 2020:

This seems like an bug? s/“add_to_wallet”/“replaceable”/

fjahr commented at 2:12 pm on September 15, 2020:

nit: Maybe this and comparable sections could be compressed with ternaries as well

0const bool rbf = options.exists("replaceable") ? options["add_to_wallet"].get_bool() : pwallet->m_signal_rbf;

EDIT: or see kalle’s suggestion…

Sjors commented at 1:23 pm on September 17, 2020:

Hopefully addressed in #19957?

fjahr commented at 4:32 pm on September 17, 2020:

Not that I see… I am not sure if I am missing something or if you missed the point of my initial (non-nit) comment :) But now I seem to be able to make code suggestions again (didn’t work before for some reason), this is what I meant with “this seems like a bug”:

0                rbf = options["replaceable"].get_bool();

Sjors commented at 6:42 pm on September 17, 2020:

I think I got distracted by your second comment. Will fix in #19969

in test/functional/wallet_send.py:330 in 92326d8976

325+        locked_coins = w0.listlockunspent()
326+        assert_equal(len(locked_coins), 1)
327+        # Locked coins are automatically unlocked when manually selected
328+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False)
329+
330+        self.log.info("Replaceable...")

fjahr commented at 2:26 pm on September 15, 2020:

The assert if the replaceable option was actually set is only done if the tx is added to the wallet, so I think these tests can not fail.

Sjors commented at 1:28 pm on September 17, 2020:

Added the check here in two other places just in case, though test_send should probably be refactored to check this unless it’s explicitly expected to fail.

in test/functional/wallet_send.py:328 in 92326d8976

323+        res = self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1], add_to_wallet=False, lock_unspents=True)
324+        assert res["complete"]
325+        locked_coins = w0.listlockunspent()
326+        assert_equal(len(locked_coins), 1)
327+        # Locked coins are automatically unlocked when manually selected
328+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False)

fjahr commented at 2:29 pm on September 15, 2020:

micro-nit: spacing before add_to_wallet

Sjors deleted the branch on Sep 15, 2020

fjahr commented at 4:03 pm on September 15, 2020: member

post-merge Concept ACK

Feel free to ignore my nits, even in a follow-up. But the replaceable issue needs to be addressed afaict.

Sjors commented at 1:31 pm on September 17, 2020: member

Nits addressed in #19969, where I’m also declaring this RPC experimental.

fanquake referenced this in commit e36aa351a3 on Sep 29, 2020

sidhujag referenced this in commit 4793d4ff1f on Sep 29, 2020

Fabcien referenced this in commit 9804792410 on Oct 7, 2021

Fabcien referenced this in commit 3ed98b036f on Oct 7, 2021

Fabcien referenced this in commit d044621ed4 on Oct 7, 2021

DrahtBot locked this on Feb 15, 2022

The ultimate send RPC #16378

Conflicts