Add change type option to fundrawtransaction #12194

pull promag wants to merge 3 commits into bitcoin:master from promag:2018-01-fundrawtransaction-changetype changing 5 files +34 −5

promag commented at 4:36 PM on January 15, 2018: member

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 commented at 4:38 PM on January 15, 2018: member

Missing tests for the new option.
instagibbs commented at 4:48 PM on January 15, 2018: member

might be a good candidate for another coincontrol field?

concept ACK
promag commented at 5:10 PM on January 15, 2018: member

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.
promag force-pushed on Jan 15, 2018

in src/wallet/rpcwallet.cpp:3018 in 6596d32841 outdated

3014 | @@ -3015,6 +3015,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
3015 |                              "   {\n"
3016 |                              "     \"changeAddress\"          (string, optional, default pool address) The bitcoin address to receive the change\n"
3017 |                              "     \"changePosition\"         (numeric, optional, default random) The index of the change output\n"
3018 | +                            "     \"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"

jonasschnelli commented at 7:22 PM on January 15, 2018:

nit: missing , after )

promag commented at 12:17 AM on January 16, 2018:

Not sure what you mean.

jonasschnelli commented at 7:22 PM on January 15, 2018: contributor

Concept ACK. I agree with @instagibbs that it could/should be a CoinControl member.
promag force-pushed on Jan 16, 2018
promag force-pushed on Jan 16, 2018
promag force-pushed on Jan 16, 2018
promag commented at 12:21 AM on January 16, 2018: member

Thanks @instagibbs and @jonasschnelli, changed as requested.
promag force-pushed on Jan 16, 2018
promag commented at 1:59 AM on January 16, 2018: member

Reference #12177.
kallewoof commented at 4:11 AM on January 18, 2018: member

Also see #11489
laanwj added this to the milestone 0.16.0 on Jan 18, 2018
jonasschnelli commented at 7:40 PM on January 18, 2018: contributor

Needs tests.
MarcoFalke commented at 8:22 PM on January 18, 2018: member

I am -0 on this one, I tend to like #12119 more.
promag commented at 8:45 PM on January 18, 2018: member

If change type is not explicit defined then it should follow #12119 logic for change type. I can rebase with that pull. WDYT?
fanquake added the label Wallet on Jan 19, 2018
fanquake added the label RPC/REST/ZMQ on Jan 19, 2018
ryanofsky commented at 10:19 PM on January 19, 2018: member
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 commented at 2:33 PM on January 20, 2018: member

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 force-pushed on Jan 24, 2018
promag commented at 3:32 PM on January 24, 2018: member

Rebase and added test.
in src/wallet/coincontrol.h:22 in 3145336843 outdated
```
  16 | @@ -17,6 +17,7 @@ class CCoinControl
  17 |  {
  18 |  public:
  19 |      CTxDestination destChange;
  20 | +    OutputType change_type;
```
instagibbs commented at 3:40 PM on January 24, 2018:

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)

promag commented at 4:02 PM on January 24, 2018:

Done.
[wallet] Add change type to CCoinControl 31dbd5af48

in src/wallet/rpcwallet.cpp:3060 in 5a53388e6a outdated

3056 | @@ -3057,6 +3057,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
3057 |                              "   {\n"
3058 |                              "     \"changeAddress\"          (string, optional, default pool address) The bitcoin address to receive the change\n"
3059 |                              "     \"changePosition\"         (numeric, optional, default random) The index of the change output\n"
3060 | +                            "     \"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"

instagibbs commented at 3:46 PM on January 24, 2018:

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

promag commented at 4:02 PM on January 24, 2018:

Done.

promag commented at 4:02 PM on January 24, 2018:

Also s/unspecified/not specified.

promag force-pushed on Jan 24, 2018
ryanofsky commented at 3:56 PM on January 24, 2018: member

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 commented at 3:58 PM on January 24, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/12194/commits/79985b052e8846c11a165ac9eabcc8767c32cbb6

non-blocking requests
[rpc] Add change_type option to fundrawtransaction 536ddeb173
[qa] Test fundrawtransaction with change_type option 16f6f59dcf
promag force-pushed on Jan 24, 2018

in src/wallet/coincontrol.h:21 in 31dbd5af48 outdated

  15 | @@ -16,7 +16,10 @@
  16 |  class CCoinControl
  17 |  {
  18 |  public:
  19 | +    //! Custom change destination, if not set an address is generated
  20 |      CTxDestination destChange;
  21 | +    //! Custom change type, ignored if destChange is set, defaults to g_change_type

jonasschnelli commented at 7:07 PM on January 24, 2018:

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

in src/wallet/coincontrol.h:22 in 31dbd5af48 outdated

  15 | @@ -16,7 +16,10 @@
  16 |  class CCoinControl
  17 |  {
  18 |  public:
  19 | +    //! Custom change destination, if not set an address is generated
  20 |      CTxDestination destChange;
  21 | +    //! Custom change type, ignored if destChange is set, defaults to g_change_type
  22 | +    OutputType change_type;

jonasschnelli commented at 7:08 PM on January 24, 2018:

nit: I guess this should haven been m_change_type?

jonasschnelli commented at 7:10 PM on January 24, 2018: contributor

utACK 16f6f59dc
jonasschnelli merged this on Jan 24, 2018
jonasschnelli closed this on Jan 24, 2018
jonasschnelli referenced this in commit 7abb0f0929 on Jan 24, 2018
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.16.0

Linked (view graph)

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