[wallet] sendtoaddress output type argument

kallewoof commented at 4:44 am on October 12, 2017: member

This is an attempt at fixing #11134.

It basically adds the output type flag as is, which is then used to generate the change destination.

kallewoof force-pushed on Oct 12, 2017

fanquake added the label Wallet on Oct 12, 2017

in src/wallet/rpcwallet.cpp:448 in a742bcb132 outdated

442@@ -443,9 +443,9 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
443         return NullUniValue;
444     }
445 
446-    if (request.fHelp || request.params.size() < 2 || request.params.size() > 8)
447+    if (request.fHelp || request.params.size() < 2 || request.params.size() > 9)
448         throw std::runtime_error(
449-            "sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount replaceable conf_target \"estimate_mode\")\n"
450+            "sendtoaddress \"address\" amount ( \"comment\" \"comment_to\" subtractfeefromamount replaceable conf_target \"estimate_mode\" \"style\" )\n"

promag commented at 1:39 pm on October 12, 2017:

Should be change_style?

promag commented at 1:46 pm on October 12, 2017:

Should default to GetArg("-changestyle")?

kallewoof commented at 1:40 am on October 13, 2017:

Makes sense.

in src/wallet/rpcwallet.cpp:527 in a742bcb132 outdated

522+        // Generate a new key that is added to wallet
523+        CPubKey newKey;
524+        if (!pwallet->GetKeyFromPool(newKey)) {
525+            throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
526+        }
527+        coin_control.destChange = pwallet->GetDestinationForKey(newKey, style);

promag commented at 1:41 pm on October 12, 2017:

I think it is preferable to add m_change_style to coin control so that a new key is consumed only when necessary. It can also helps in other send/fund calls. This should be used in: https://github.com/bitcoin/bitcoin/blob/f74459dba6de4d4462860318f6ee5bda8522e07b/src/wallet/wallet.cpp#L2679

in src/wallet/rpcwallet.cpp:467 in a742bcb132 outdated

463@@ -464,6 +464,7 @@ UniValue sendtoaddress(const JSONRPCRequest& request)
464             "       \"UNSET\"\n"
465             "       \"ECONOMICAL\"\n"
466             "       \"CONSERVATIVE\"\n"
467+            "9. \"style\"              (string, optional, default='p2pkh') The address style to use for the change output. Valid options are 'p2pkh', 'p2sh_p2wpkh', and 'p2wpkh'"

promag commented at 1:56 pm on October 12, 2017:

Nit, replace ' with \". See: https://github.com/bitcoin/bitcoin/blob/f74459dba6de4d4462860318f6ee5bda8522e07b/src/wallet/rpcdump.cpp#L87

promag commented at 1:57 pm on October 12, 2017: member

Concept ACK.

kallewoof force-pushed on Oct 13, 2017

kallewoof commented at 6:22 am on October 13, 2017: member

@promag Thanks for the review! Very good points. I’ve updated the code according to your suggestions.

kallewoof force-pushed on Oct 13, 2017

kallewoof commented at 4:43 am on December 5, 2017: member

Closing this as I’m not sure of a good way to stay up to date with the other PR. Will reopen once merged instead.

Edit: Reopened post-merge of dependent PR.

kallewoof closed this on Dec 5, 2017

kallewoof reopened this on Jan 18, 2018

kallewoof force-pushed on Jan 18, 2018

Support output type argument in sendtoaddress for change address (closes #11134) 676b58d825

kallewoof force-pushed on Jan 18, 2018

kallewoof renamed this:
~~[wallet] sendtoaddress style argument~~
[wallet] sendtoaddress output type argument
on Jan 18, 2018

laanwj added this to the milestone 0.16.0 on Jan 18, 2018

MarcoFalke commented at 7:14 pm on January 18, 2018: member

Tend to NACK here. If you really want to control the change type on a per-call basis, maybe you should use createrawtransaction. It is not our goal to support every edge use-case. I doubt the option in sendtoaddress will still be useful two releases in the future and we’d have to deprecate it with way too much effort.

Other than that, you could set the change type on the command line or use a more intelligent solution such as #12119 (not yet merged).

jonasschnelli commented at 7:35 pm on January 18, 2018: contributor

I agree with @MarcoFalke. sendtoaddress is a simple send command where you don’t even have control over how much fee you have to pay (single shot) and I don’t think we should add more “expert” functions there.

jonasschnelli removed this from the milestone 0.16.0 on Jan 18, 2018

kallewoof commented at 11:46 pm on January 18, 2018: member

The next step “down” from sendtoaddress is createrawtransaction which is way overkill 99% of the time. This desire to keep sendtoaddress dumb to “not confuse users” is itself confusing to me. See also #11413 which invalidates the argument about fees above (unless it’s NAK’d and not merged).

MarcoFalke commented at 0:41 am on January 19, 2018: member

I can understand the motivation to add an option to set the fee explicitly, but I fail to see any reason to support setting the change type.

kallewoof closed this on Jan 19, 2018

kallewoof deleted the branch on Oct 17, 2019

DrahtBot locked this on Dec 16, 2021

[wallet] sendtoaddress output type argument #11489