The ultimate send RPC by Sjors · Pull Request #16378 · bitcoin/bitcoin

Sjors · 2019-07-12T13:56:59Z

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 rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #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:

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:

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

src/wallet/rpcwallet.cpp

DrahtBot · 2019-07-12T16:18:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

luke-jr · 2019-08-20T00:05:04Z

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 · 2019-08-21T08:56:06Z

@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 · 2019-08-26T10:34:14Z

Closing in favor of tweaking sendmany / sendtoaddress:

[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #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 · 2020-02-24T21:28:48Z

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.

bitcoin-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: [rpc] don't automatically append inputs in walletcreatefundedpsbt #16377 and [wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option #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 · 2020-08-31T11:56:06Z

Rebased after #18244 landed.

meshcollider

utACK 27c07f08662a8044c729d0adead0511984c40cef

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

meshcollider · 2020-09-07T00:33:53Z

src/wallet/rpcwallet.cpp

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

2. conf_target (numeric) wallet default 3. estimate_mode (string) unset

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

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

src/wallet/rpcwallet.cpp

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

meshcollider · 2020-09-10T02:41:18Z

src/wallet/rpcwallet.cpp

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

Because I blindly copied it from walletcreatefundedpsbt :-)

Replaced with simpler version.

src/wallet/rpcwallet.cpp

ShaunApps · 2020-09-10T13:14:42Z

ACK.

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

meshcollider

Light re-utACK 92326d8

(still haven't read the test carefully)

achow101

ACK 92326d8 Reviewed code and test, ran tests.

Just some nits and non-blocking comments.

src/wallet/rpcwallet.cpp

test/functional/wallet_send.py

achow101 · 2020-09-15T01:15:11Z

src/wallet/rpcwallet.cpp

+            {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
+                {
+                    {"add_inputs", RPCArg::Type::BOOL, /* default */ "false", "If inputs are specified, automatically include more if they are not enough."},
+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns a serialized transaction which will not be added to the wallet or broadcast"},


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

Maybe, but this also works when walletbroadcast is off

test/functional/wallet_send.py

kallewoof

utACK 92326d8

kallewoof · 2020-09-15T02:19:01Z

src/rpc/rawtransaction_util.cpp

 {
-    if (inputs_in.isNull() || outputs_in.isNull())
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, arguments 1 and 2 must be non-null");
+    if (outputs_in.isNull())


[rpc] walletcreatefundedpsbt: allow inputs to be null:

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

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

src/rpc/rawtransaction_util.cpp

kallewoof · 2020-09-15T02:36:39Z

src/wallet/rpcwallet.cpp

+                {
+                    {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+                        {
+                            {"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 + ""},


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

kallewoof · 2020-09-15T02:38:27Z

src/wallet/rpcwallet.cpp

+                        },
+                    {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+                        {
+                            {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"},


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

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?

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.

kallewoof · 2020-09-15T02:40:23Z

src/wallet/rpcwallet.cpp

+                    {"change_position", RPCArg::Type::NUM, /* default */ "random", "The index of the change output"},
+                    {"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\"."},
+                    {"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)"},
+                    {"estimate_mode", RPCArg::Type::STR, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"


Why are there two conf_target and estimate_mode parameters?

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

kallewoof · 2020-09-15T02:44:23Z

src/wallet/rpcwallet.cpp

+            bool add_to_wallet = true;
+            if (options.exists("add_to_wallet")) {
+                add_to_wallet = options["add_to_wallet"].get_bool();
+            }


Alternative:

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

I'll leave this to your #19957

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

meshcollider · 2020-09-15T02:51:18Z

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

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

fjahr

post-merge Concept ACK

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

fjahr · 2020-09-15T13:58:26Z

src/wallet/rpcwallet.cpp


-        if (options.exists("changeAddress")) {
-            CTxDestination dest = DecodeDestination(options["changeAddress"].get_str());
+        if (options.exists("changeAddress") || options.exists("change_address")) {


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

I believe that's covered in #19957

src/wallet/rpcwallet.cpp

fjahr · 2020-09-15T14:09:12Z

src/wallet/rpcwallet.cpp

+            int change_position;
+            bool rbf = pwallet->m_signal_rbf;
+            if (options.exists("replaceable")) {
+                rbf = options["add_to_wallet"].get_bool();


This seems like an bug? s/"add_to_wallet"/"replaceable"/

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

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

EDIT: or see kalle's suggestion...

Hopefully addressed in #19957?

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

Suggested change

rbf = options["add_to_wallet"].get_bool();

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

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

fjahr · 2020-09-15T14:26:43Z

test/functional/wallet_send.py

+        # Locked coins are automatically unlocked when manually selected
+        self.test_send(from_wallet=w0, to_wallet=w1, amount=1, inputs=[utxo1],add_to_wallet=False)
+
+        self.log.info("Replaceable...")


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.

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.

test/functional/wallet_send.py

Sjors · 2020-09-17T13:31:41Z

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

f7b331e rpc: add brackets to ConstructTransaction (Sjors Provoost) d813d26 [rpc] send: various touch-ups (Sjors Provoost) 0fc1c68 [rpc] send: fix parsing replaceable option (Sjors Provoost) efc9b85 Mark send RPC experimental (Sjors Provoost) Pull request description: Followup based on #16378 nits. It also fixes an argument parsing error (uncaught because the test wasn't sufficiently thorough). I marked the RPC as experimental so we can tweak it a bit over the next release cycle. ACKs for top commit: meshcollider: utACK f7b331e fjahr: utACK f7b331e kallewoof: ACK f7b331e Tree-SHA512: 82dd8ac76a6558872db3f5249d4d6440469400aaa339153bc627d1ee673a91ecfadecb486bc1939ba87ebbd80e26ff29698e93e358599f3d26fde0e526892afe

f7b331e rpc: add brackets to ConstructTransaction (Sjors Provoost) d813d26 [rpc] send: various touch-ups (Sjors Provoost) 0fc1c68 [rpc] send: fix parsing replaceable option (Sjors Provoost) efc9b85 Mark send RPC experimental (Sjors Provoost) Pull request description: Followup based on bitcoin#16378 nits. It also fixes an argument parsing error (uncaught because the test wasn't sufficiently thorough). I marked the RPC as experimental so we can tweak it a bit over the next release cycle. ACKs for top commit: meshcollider: utACK f7b331e fjahr: utACK f7b331e kallewoof: ACK f7b331e Tree-SHA512: 82dd8ac76a6558872db3f5249d4d6440469400aaa339153bc627d1ee673a91ecfadecb486bc1939ba87ebbd80e26ff29698e93e358599f3d26fde0e526892afe

Summary: This is of neglible use here, but it allows new RPC methods to take outputs as their first argument and make inputs optional. PR description: > walletcreatefundedpsbt has some interesting features that sendtoaddress and sendmany don't have: > > - manual coin selection > - outputting a PSBT > - 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: > > `bitcoin-cli -regtest send '{"ADDRESS": 0.1}' ` > > 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. This is a backport of [[bitcoin/bitcoin#16378 | core#16378]] [1/3] bitcoin/bitcoin@1bc8d0f Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10263

Summary: This is a backport of [[bitcoin/bitcoin#16378 | core#16378]] [2/3] bitcoin/bitcoin@2c2a144 Depends on D10263 I also added "fee_rate", because I will use it in the next commit instead of `conf_target` and `estimate_mode` to set the fee. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10264

Summary: This is a backport of [[bitcoin/bitcoin#16378 | [[bitcoin/bitcoin#16378 | core#16378]]]] [3/3] bitcoin/bitcoin@92326d8 Depends on D10264 Backport differences: - Arguments `conf_target` and `estimate_mode`, as well as the options of the same name are removed. The option `fee_rate` can be used to set the fee. The `RPCExamples` are updated accordingly. - Options `change_type` and `replaceable` are removed. - The functional test sets the fee rate with `fee_rate` option, so the unit is XEC/kB instead of sat/B. - In the test, I did not set `decimal.getcontext().prec = 2`, because this causes a suprising loss of precision that makes somes checks fail: ``` >>> from decimal import getcontext, Decimal >>> getcontext().prec = 2 >>> Decimal("1329999995.50") - Decimal("1328999993.25") Decimal('1.0E+6') ``` Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10265

Sjors force-pushed the 2019/07/send branch from f3a46fd to ea13ea5 Compare July 12, 2019 13:57

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

instagibbs reviewed Jul 12, 2019

View reviewed changes

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

laanwj added the Wallet label Jul 15, 2019

DrahtBot added the Needs rebase label Aug 2, 2019

Sjors force-pushed the 2019/07/send branch from ea13ea5 to 2ca53d1 Compare August 2, 2019 16:34

DrahtBot removed the Needs rebase label Aug 2, 2019

This was referenced Aug 2, 2019

[rpc] signer send and fee bump convenience methods #15876

Closed

External signer support - Wallet Box edition #16546

Merged

Sjors mentioned this pull request Aug 15, 2019

bumpfee: Return PSBT when wallet has privkeys disabled #16373

Merged

Sjors closed this Aug 26, 2019

Sjors mentioned this pull request Feb 24, 2020

rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

Closed

Sjors reopened this Feb 24, 2020

Sjors force-pushed the 2019/07/send branch from 2ca53d1 to 06865d0 Compare February 24, 2020 21:22

Sjors force-pushed the 2019/07/send branch 2 times, most recently from bab395a to 528be83 Compare February 25, 2020 16:57

Sjors force-pushed the 2019/07/send branch from 15ba29a to 510ff41 Compare August 28, 2020 16:18

Sjors force-pushed the 2019/07/send branch from 510ff41 to 27c07f0 Compare August 31, 2020 11:56

meshcollider reviewed Sep 7, 2020

View reviewed changes

Sjors added 2 commits September 7, 2020 20:33

[rpc] walletcreatefundedpsbt: allow inputs to be null

1bc8d0f

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

[rpc] add snake case aliases for transaction methods

2c2a144

Sjors force-pushed the 2019/07/send branch from 27c07f0 to 62d3523 Compare September 7, 2020 18:59

meshcollider reviewed Sep 10, 2020

View reviewed changes

[rpc] add send method

92326d8

Sjors force-pushed the 2019/07/send branch from 62d3523 to 92326d8 Compare September 10, 2020 11:45

ShaunApps reviewed Sep 10, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

meshcollider reviewed Sep 11, 2020

View reviewed changes

achow101 reviewed Sep 15, 2020

View reviewed changes

kallewoof reviewed Sep 15, 2020

View reviewed changes

meshcollider merged commit ffaac6e into bitcoin:master Sep 15, 2020

kallewoof mentioned this pull request Sep 15, 2020

rpc: alias helper #19957

Closed

Sjors deleted the 2019/07/send branch September 15, 2020 15:38

fjahr reviewed Sep 15, 2020

View reviewed changes

Sjors mentioned this pull request Sep 17, 2020

Send RPC bug fix and touch-ups #19969

Merged

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

Conversation

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

Uh oh!

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!

luke-jr commented Aug 20, 2019

Uh oh!

Sjors commented Aug 21, 2019

Uh oh!

Sjors commented Aug 26, 2019

Uh oh!

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

Uh oh!

Uh oh!

Sjors commented Aug 31, 2020

Uh oh!

meshcollider left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShaunApps commented Sep 10, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kallewoof 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

Uh oh!

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!

kallewoof Sep 15, 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!

Sjors commented Jul 12, 2019 •

edited

Loading

DrahtBot commented Jul 12, 2019 •

edited

Loading

Sjors commented Feb 24, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading

achow101 left a comment •

edited

Loading

kallewoof Sep 15, 2020 •

edited

Loading

fjahr Sep 17, 2020 •

edited

Loading