Add change type option to fundrawtransaction #12194

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

@@ -3015,6 +3015,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
                            "   {\n"
                            "     \"changeAddress\"          (string, optional, default pool address) The bitcoin address to receive the change\n"
                            "     \"changePosition\"         (numeric, optional, default random) The index of the change output\n"
+                            "     \"change_type\"            (string, optional) The address type to use. Only valid if changeAddress is unspecified. Options are \"legacy\", \"p2sh\", and \"bech32\". Default is set by -changetype.\n"


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 [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH #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

@@ -17,6 +17,7 @@ class CCoinControl
 {
 public:
    CTxDestination destChange;
+    OutputType change_type;


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

@@ -3057,6 +3057,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
                            "   {\n"
                            "     \"changeAddress\"          (string, optional, default pool address) The bitcoin address to receive the change\n"
                            "     \"changePosition\"         (numeric, optional, default random) The index of the change output\n"
+                            "     \"change_type\"            (string, optional) The address type to use. Only valid if changeAddress is unspecified. Options are \"legacy\", \"p2sh-segwit\", and \"bech32\". Default is set by -changetype.\n"


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

    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

    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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add change type option to fundrawtransaction #12194

Add change type option to fundrawtransaction #12194

promag commented Jan 15, 2018 •

edited

promag commented Jan 15, 2018

instagibbs commented Jan 15, 2018

promag commented Jan 15, 2018

jonasschnelli Jan 15, 2018

promag Jan 16, 2018

jonasschnelli commented Jan 15, 2018

promag commented Jan 16, 2018

promag commented Jan 16, 2018

kallewoof commented Jan 18, 2018

jonasschnelli commented Jan 18, 2018

maflcko commented Jan 18, 2018

promag commented Jan 18, 2018 •

edited

ryanofsky left a comment

promag commented Jan 20, 2018 •

edited

promag commented Jan 24, 2018

instagibbs Jan 24, 2018

promag Jan 24, 2018

instagibbs Jan 24, 2018 •

edited

promag Jan 24, 2018

promag Jan 24, 2018

ryanofsky commented Jan 24, 2018 •

edited

instagibbs commented Jan 24, 2018

jonasschnelli Jan 24, 2018

jonasschnelli Jan 24, 2018

jonasschnelli commented Jan 24, 2018

		CTxDestination destChange;
		//! Custom change type, ignored if destChange is set, defaults to g_change_type

Add change type option to fundrawtransaction #12194

Add change type option to fundrawtransaction #12194

Conversation

promag commented Jan 15, 2018 • edited

promag commented Jan 15, 2018

instagibbs commented Jan 15, 2018

promag commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Jan 15, 2018

promag commented Jan 16, 2018

promag commented Jan 16, 2018

kallewoof commented Jan 18, 2018

jonasschnelli commented Jan 18, 2018

maflcko commented Jan 18, 2018

promag commented Jan 18, 2018 • edited

ryanofsky left a comment

Choose a reason for hiding this comment

promag commented Jan 20, 2018 • edited

promag commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs Jan 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky commented Jan 24, 2018 • edited

instagibbs commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonasschnelli commented Jan 24, 2018

promag commented Jan 15, 2018 •

edited

promag commented Jan 18, 2018 •

edited

promag commented Jan 20, 2018 •

edited

instagibbs Jan 24, 2018 •

edited

ryanofsky commented Jan 24, 2018 •

edited