Add change type option to fundrawtransaction by promag · Pull Request #12194 · bitcoin/bitcoin

promag · 2018-01-15T16:36:41Z

Adds a new option change_type to fundrawtransaction RPC. This is useful to override the node -changetype argument.

The new option is exclusive to changeAddress option, setting both raises a RPC error.

See also #11403, #12119.

promag · 2018-01-15T16:38:56Z

Missing tests for the new option.

instagibbs · 2018-01-15T16:48:18Z

might be a good candidate for another coincontrol field?

concept ACK

promag · 2018-01-15T17:10:29Z

might be a good candidate for another coincontrol field?

I thought the same, but coin control doesn't sound the best place to put it. Correct me if I'm wrong though.

jonasschnelli · 2018-01-15T19:22:00Z

src/wallet/rpcwallet.cpp

nit: missing , after )

Not sure what you mean.

jonasschnelli · 2018-01-15T19:22:31Z

Concept ACK.
I agree with @instagibbs that it could/should be a CoinControl member.

promag · 2018-01-16T00:21:30Z

Thanks @instagibbs and @jonasschnelli, changed as requested.

promag · 2018-01-16T01:59:40Z

Reference #12177.

kallewoof · 2018-01-18T04:11:51Z

Also see #11489

jonasschnelli · 2018-01-18T19:40:53Z

Needs tests.

maflcko · 2018-01-18T20:22:53Z

I am -0 on this one, I tend to like #12119 more.

promag · 2018-01-18T20:45:06Z

If change type is not explicit defined then it should follow #12119 logic for change type. I can rebase with that pull. WDYT?

ryanofsky

utACK 4c74a8ef93d75e605feece08b150cdbd7260ec82. I think it's nice to be adding the coincontrol/fundraw option, if only to make it explicit that unless you override it (or changeAddress), then the type of change output will depend on global options the wallet was started up with. Notes:

This PR will conflict with #12119, but the changes should be compatible (as promag pointed out). I don't have an opinion on which PR should be merged first or rebased on the other.
It would be good to add a simple test to exercise the new code.
It could be good if other RPCs calling CreateTransaction/SendMoney got a change_type option.

promag · 2018-01-20T14:33:31Z

I don't have an opinion on which PR should be merged first or rebased on the other.

From what I can tell #12119 changes the OUTPUT_TYPE_NONE meaning. For that reason I would merge this one first.

It would be good to add a simple test to exercise the new code.

Will do.

It could be good if other RPCs calling CreateTransaction/SendMoney got a change_type option

Not sure about that though. I think this is useful for a migration period and targets expert users.

promag · 2018-01-24T15:32:49Z

Rebase and added test.

instagibbs · 2018-01-24T15:40:02Z

src/wallet/coincontrol.h

Probably should add some comments distinguishing use between this and destChange right above.

Had to look up why both are required. (this being useful for non-prescribed destination, but prescribed destination type only)

instagibbs · 2018-01-24T15:46:26Z

src/wallet/rpcwallet.cpp

nit: maybe try to avoid additional "address" language for change stuff. "The output type to use for change"

Also s/unspecified/not specified.

ryanofsky · 2018-01-24T15:56:26Z

Slightly tested (running & tweaking fundraw test script) ACK 79985b052e8846c11a165ac9eabcc8767c32cbb6. Changes since last review were rebasing after #12119, adding test, adding change_type RPCTypeCheck, adding coincontrol comments.

instagibbs · 2018-01-24T15:58:25Z

utACK 79985b0

non-blocking requests

jonasschnelli · 2018-01-24T19:07:58Z

src/wallet/coincontrol.h

 public:
+    //! Custom change destination, if not set an address is generated
    CTxDestination destChange;
+    //! Custom change type, ignored if destChange is set, defaults to g_change_type


nit: the term "change type" seems not precise enough... I would have used change address type.

jonasschnelli · 2018-01-24T19:08:23Z

src/wallet/coincontrol.h

+    //! Custom change destination, if not set an address is generated
    CTxDestination destChange;
+    //! Custom change type, ignored if destChange is set, defaults to g_change_type
+    OutputType change_type;


nit: I guess this should haven been m_change_type?

jonasschnelli · 2018-01-24T19:10:03Z

utACK 16f6f59

16f6f59 [qa] Test fundrawtransaction with change_type option (João Barbosa) 536ddeb [rpc] Add change_type option to fundrawtransaction (João Barbosa) 31dbd5a [wallet] Add change type to CCoinControl (João Barbosa) Pull request description: Adds a new option `change_type` to `fundrawtransaction` RPC. This is useful to override the node `-changetype` argument. The new option is exclusive to `changeAddress` option, setting both raises a RPC error. See also #11403, #12119. Tree-SHA512: 654686444f6125e37015a62f167064d54ec335701534988447be4687fa5ef9c7980a8a07cc0a03fff6ea6c4c1abf0f77a8843d535c4f3fe0bf93f968a4e676e6

promag force-pushed the 2018-01-fundrawtransaction-changetype branch from a5e687c to 6596d32 Compare January 15, 2018 17:12

jonasschnelli reviewed Jan 15, 2018

View reviewed changes

promag force-pushed the 2018-01-fundrawtransaction-changetype branch 3 times, most recently from c0e5b8e to f46f06f Compare January 16, 2018 00:20

promag force-pushed the 2018-01-fundrawtransaction-changetype branch from f46f06f to 4c74a8e Compare January 16, 2018 01:58

promag mentioned this pull request Jan 17, 2018

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

Closed

laanwj added this to the 0.16.0 milestone Jan 18, 2018

fanquake added Wallet RPC/REST/ZMQ labels Jan 19, 2018

ryanofsky reviewed Jan 19, 2018

View reviewed changes

promag force-pushed the 2018-01-fundrawtransaction-changetype branch from 4c74a8e to bceb5da Compare January 24, 2018 15:31

promag mentioned this pull request Jan 24, 2018

[wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH #12119

Merged

instagibbs reviewed Jan 24, 2018

View reviewed changes

[wallet] Add change type to CCoinControl

31dbd5a

promag force-pushed the 2018-01-fundrawtransaction-changetype branch from bceb5da to 79985b0 Compare January 24, 2018 15:52

promag added 2 commits January 24, 2018 16:00

[rpc] Add change_type option to fundrawtransaction

536ddeb

[qa] Test fundrawtransaction with change_type option

16f6f59

promag force-pushed the 2018-01-fundrawtransaction-changetype branch from 79985b0 to 16f6f59 Compare January 24, 2018 16:01

jonasschnelli reviewed Jan 24, 2018

View reviewed changes

jonasschnelli merged commit 16f6f59 into bitcoin:master Jan 24, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

promag commented Jan 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 15, 2018

Uh oh!

instagibbs commented Jan 15, 2018

Uh oh!

promag commented Jan 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 15, 2018

Uh oh!

promag commented Jan 16, 2018

Uh oh!

promag commented Jan 16, 2018

Uh oh!

kallewoof commented Jan 18, 2018

Uh oh!

jonasschnelli commented Jan 18, 2018

Uh oh!

maflcko commented Jan 18, 2018

Uh oh!

promag commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

promag commented Jan 15, 2018 •

edited

Loading

promag commented Jan 18, 2018 •

edited

Loading

promag commented Jan 20, 2018 •

edited

Loading

instagibbs Jan 24, 2018 •

edited

Loading

ryanofsky commented Jan 24, 2018 •

edited

Loading