Skip to content

Conversation

Sjors
Copy link
Member

@Sjors Sjors commented Jul 12, 2019

walletcreatefundedpsbt has some interesting features that sendtoaddress and sendmany don't have:

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:

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 12, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Member

luke-jr commented Aug 20, 2019

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
Copy link
Member Author

Sjors commented Aug 21, 2019

@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
Copy link
Member Author

Sjors commented Aug 26, 2019

Closing in favor of tweaking sendmany / sendtoaddress:

@Sjors
Copy link
Member Author

Sjors commented Feb 24, 2020

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:

  1. 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)
  2. concept ACKs and/or suggestions for the RPC interface

@luke-jr suggested:

Suggest moving "inputs" to a field on "options"

Done

Sjors added a commit to Sjors/bitcoin that referenced this pull request Aug 28, 2020
…e keys

TODO: replace with bitcoin#16378 once its prerequisites are merged.

In addition we check to see if the PSBT is complete, in preperation for a ScriptPubKeyMan that can use an external signer.
@Sjors
Copy link
Member Author

Sjors commented Aug 31, 2020

Rebased after #18244 landed.

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 27c07f0

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

Comment on lines 3914 to 3915
{"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)"},
{"estimate_mode", RPCArg::Type::STR, RPCArg::Optional::OMITTED, /* default */ "unset", std::string() + "The fee estimate mode, must be one of (case insensitive):\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Sjors added a commit to Sjors/bitcoin that referenced this pull request Sep 7, 2020
…e keys

TODO: replace with bitcoin#16378 once its prerequisites are merged.

In addition we check to see if the PSBT is complete, in preperation for a ScriptPubKeyMan that can use an external signer.
This is of neglible use here, but it allows new RPC methods to take outputs as their first argument and make inputs optional.
const UniValue &replaceable_arg = options["replaceable"];
if (!replaceable_arg.isNull()) {
RPCTypeCheckArgument(replaceable_arg, UniValue::VBOOL);
rbf = replaceable_arg.isTrue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I blindly copied it from walletcreatefundedpsbt :-)

Replaced with simpler version.

@ShaunApps
Copy link

ACK.

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

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light re-utACK 92326d8

(still haven't read the test carefully)

Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 92326d8 Reviewed code and test, ran tests.

Just some nits and non-blocking comments.

{"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"},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but this also works when walletbroadcast is off

Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 92326d8

@@ -21,10 +21,15 @@

CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf)
{
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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[rpc] walletcreatefundedpsbt: allow inputs to be null:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
{"", 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 + ""},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
{"", 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"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{"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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are there two conf_target and estimate_mode parameters?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@kallewoof kallewoof Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative:

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave this to your #19957

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@meshcollider
Copy link
Contributor

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

@meshcollider meshcollider merged commit ffaac6e into bitcoin:master Sep 15, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2020
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
@kallewoof kallewoof mentioned this pull request Sep 15, 2020
@Sjors Sjors deleted the 2019/07/send branch September 15, 2020 15:38
Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

post-merge Concept ACK

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

@@ -2972,22 +2977,24 @@ void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& f
coinControl.m_add_inputs = options["add_inputs"].get_bool();
}

if (options.exists("changeAddress")) {
CTxDestination dest = DecodeDestination(options["changeAddress"].get_str());
if (options.exists("changeAddress") || options.exists("change_address")) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that's covered in #19957

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully addressed in #19957?

Copy link
Contributor

@fjahr fjahr Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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...")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Sjors
Copy link
Member Author

Sjors commented Sep 17, 2020

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

fanquake added a commit that referenced this pull request Sep 29, 2020
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 29, 2020
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 7, 2021
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 7, 2021
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 7, 2021
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.