-
Notifications
You must be signed in to change notification settings - Fork 37.7k
The ultimate send RPC #16378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
The ultimate send RPC #16378
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
Suggest moving "inputs" to a field on "options", and removing PSBT output from |
@luke-jr agree about moving the inputs. That also makes it easier to rebase on #11413 (explicit fee rate) and have a syntax like The reason I added PSBT output is to make multisig easier. When used with a descriptor wallet #15764 you simply call |
Closing in favor of tweaking
|
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
Ideally I'd also get rid of the JSON encoding, but at least this RPC example is easy enough for a new This needs some cleanup, but I suggest focussing initial review on:
@luke-jr suggested:
Done |
bab395a
to
528be83
Compare
…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.
Rebased after #18244 landed. |
There was a problem hiding this 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!
src/wallet/rpcwallet.cpp
Outdated
{"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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
…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.
src/wallet/rpcwallet.cpp
Outdated
const UniValue &replaceable_arg = options["replaceable"]; | ||
if (!replaceable_arg.isNull()) { | ||
RPCTypeCheckArgument(replaceable_arg, UniValue::VBOOL); | ||
rbf = replaceable_arg.isTrue(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ACK. Ran functional tests. Played w/ command in cli. Left a nit |
There was a problem hiding this 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)
There was a problem hiding this 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"}, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this 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()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 + ""}, |
There was a problem hiding this comment.
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"}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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(); | ||
} |
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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
There was a problem hiding this 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")) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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"/
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully addressed in #19957?
There was a problem hiding this comment.
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":
rbf = options["add_to_wallet"].get_bool(); | |
rbf = options["replaceable"].get_bool(); |
There was a problem hiding this comment.
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...") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
walletcreatefundedpsbt
has some interesting features thatsendtoaddress
andsendmany
don't have:-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. Ifadd_to_wallet
is set tofalse
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
andsendmany
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
)[wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option
)[rpc] have lockUnspents also lock manually selected coins
)