[wallet] sendtoaddress output type argument #11489

pull kallewoof wants to merge 1 commits into bitcoin:master from kallewoof:201709_segwitwallet2_sendtoaddress changing 3 files +15 −4

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 force-pushed on Oct 13, 2017
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
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 12: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

Contributors

Labels

Linked (view graph)

#11134 Sendtoaddress flag for sending change to a segwit address?