Skip to content

[wallet] sendtoaddress output type argument #11489

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.

Sign up for GitHub

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

Jump to bottom
Closed
kallewoof wants to merge 1 commit into from
Closed

[wallet] sendtoaddress output type argument #11489

kallewoof wants to merge 1 commit into from

Conversation

@kallewoof
Copy link
Contributor

@kallewoof kallewoof commented Oct 12, 2017 •
edited
Loading

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 kallewoof force-pushed the 201709_segwitwallet2_sendtoaddress branch from b3cebac to a742bcb Compare October 12, 2017 04:45
promag
promag reviewed Oct 12, 2017
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@promag promag Oct 12, 2017

Choose a reason for hiding this comment

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

Should be change_style?

Copy link
Contributor

@promag promag Oct 12, 2017

Choose a reason for hiding this comment

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

Should default to GetArg("-changestyle")?

Copy link
Contributor Author

@kallewoof kallewoof Oct 13, 2017

Choose a reason for hiding this comment

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

Makes sense.

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@promag promag Oct 12, 2017

Choose a reason for hiding this comment

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

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:

bitcoin/src/wallet/wallet.cpp

Line 2679 in f74459d

scriptChange = GetScriptForDestination(vchPubKey.GetID());

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@promag promag Oct 12, 2017

Choose a reason for hiding this comment

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

Nit, replace ' with \". See:

bitcoin/src/wallet/rpcdump.cpp

Line 87 in f74459d

"2. \"label\" (string, optional, default=\"\") An optional label\n"

@kallewoof kallewoof force-pushed the 201709_segwitwallet2_sendtoaddress branch from a742bcb to 56a0314 Compare October 13, 2017 06:22
@kallewoof
Copy link
Contributor Author

kallewoof commented Oct 13, 2017

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

@kallewoof kallewoof force-pushed the 201709_segwitwallet2_sendtoaddress branch 3 times, most recently from 480a988 to 3e0e821 Compare October 13, 2017 06:28
@kallewoof
Copy link
Contributor Author

kallewoof commented Dec 5, 2017 •
edited
Loading

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 kallewoof closed this Dec 5, 2017
@kallewoof kallewoof reopened this Jan 18, 2018
@kallewoof kallewoof force-pushed the 201709_segwitwallet2_sendtoaddress branch 2 times, most recently from e82ae08 to 6ac76db Compare January 18, 2018 04:04
@kallewoof kallewoof force-pushed the 201709_segwitwallet2_sendtoaddress branch from 6ac76db to 676b58d Compare January 18, 2018 04:09
@kallewoof kallewoof mentioned this pull request Jan 18, 2018
Merged
@kallewoof kallewoof changed the title [wallet] sendtoaddress style argument [wallet] sendtoaddress output type argument Jan 18, 2018
@laanwj laanwj added this to the 0.16.0 milestone Jan 18, 2018
@maflcko
Copy link
Member

maflcko commented Jan 18, 2018

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

jonasschnelli commented Jan 18, 2018 •
edited
Loading

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 jonasschnelli removed this from the 0.16.0 milestone Jan 18, 2018
@maflcko maflcko mentioned this pull request Jan 18, 2018
Closed
@kallewoof
Copy link
Contributor Author

kallewoof commented Jan 18, 2018 •
edited
Loading

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

@maflcko
Copy link
Member

maflcko commented Jan 19, 2018

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 kallewoof closed this Jan 19, 2018
@kallewoof kallewoof deleted the 201709_segwitwallet2_sendtoaddress branch October 17, 2019 08:56
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kallewoof @maflcko @jonasschnelli @promag @laanwj @fanquake