[wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH #12119
Conversation
|
Does it forces bech32 addresses in the wallet without consent? |
|
@promag for change addresses yes, so that could be an objection. More conservative approaches could be:
|
|
Creating a change output AFAIK does not add any address to the wallet. |
|
@sipa they are listed in |
|
@Sjors IMO the |
|
General concept ACK-- but I would suggest that it only do this if the change type is at least p2sh segwit: There might be some compatibility reason that someone is avoiding creating segwit outputs for some reason. That doesn't hold if they're already asking for p2sh output... privacy was the major reason the sw wallet support didn't go 100% native change from the start... but that is even more an argument for this. In fact, I think it's an argument that we should use native change if any (or no more strict than 'most') outputs are native and not just require all. |
d3d59af to
ef621fe
Compare
|
@gmaxwell it now uses a bech32 change address if change type is p2sh-segwit and any of the outputs are bech32. |
b0ac236 to
c3bc3f6
Compare
|
Seems to me if the user is explicitly setting a change type, we should respect that. Instead, I suggest turning OUTPUT_TYPE_DEFAULT into a real value, and handling it specially for change here. Eg, https://github.com/luke-jr/bitcoin/commits/bech32-change |
|
It seems a bit pointless to stand on its own in a PR. It would be better to just include it as a separate commit here (and in another PR I'm about to open). To get it in your own branch as-is, switch to it and do: |
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
Wrong indentation. Also any_destination_bech32?
There was a problem hiding this comment.
Will fix indentation and check variable name, thanks.
src/wallet/wallet.cpp
Outdated
TheBlueMatt
left a comment
There was a problem hiding this comment.
Concept ACK. Somewhat agree with @luke-jr, though you can do it simpler - just check if the change address type option has been set - if it has, use it, if it hasn't turn on this new behavior.
Or even check if any input or any output is bech32? |
|
agreed with @TheBlueMatt just check if it's set |
|
I can't reproduce this Travis failure on MacOS or Ubuntu 14.04 (I didn't use the depends system). |
|
It seemed like people in the IRC meeting liked: "If the configured change type is not legacy, use p2wpkh if any of the outputs are p2wpkh". |
|
@gmaxwell by "legacy" you mean |
|
By legacy I mean p2pkh. We should not create native segwit change if the user has specifically asked for non-segwit change. If a user is setting legacy then it's likely that they really don't want segwit outputs for some reason (for example, compatibility with older software). That argument doesn't apply for p2sh-segwit. The only reason to ever use p2sh-segwit change is for change distinguishability privacy, which, as you note-- doesn't apply when there are segwit outputs. |
|
As a pedantic aside, it's a mistake to say "bech32" -- there is no address involved with change, so no bech32 involved here at all. The question here is to use p2sh-p2wpkh or p2wpkh, which would exist no less even if bech32 had never been specified. |
|
Concept ACK |
|
utACK dc2812f. The only thing that bothers me is the fact that |
jnewbery
left a comment
There was a problem hiding this comment.
Tested ACK dc2812f3e40dbbc7533a2d2e15cf4dfb3ae079af.
A bunch of style nits inline. Otherwise looks great.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
This is perhaps just personal taste, but I prefer early function return if we already know what change address we'll use without having to look at the outputs. That also means we can remove the local variables any_destination_native_segwit and change_type. It'd look something like:
// If -changetype is specified, always use that change type.
if (g_change_type != OUTPUT_TYPE_NONE) {
return g_change_type;
}
// if g_address_type is legacy, use legacy address as change (even
// if some of the outputs are P2WPKH or P2WSH).
if (g_address_type == OUTPUT_TYPE_LEGACY) {
return OUTPUT_TYPE_LEGACY;
}
// if any destination is P2WPKH or P2WSH, use P2WPKH for the change
// output.
for (const auto& recipient : vecSend) {
// Check if any destination contains a witness program:
int witnessversion = 0;
std::vector<unsigned char> witnessprogram;
if (recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
return OUTPUT_TYPE_BECH32;
}
}
// else use g_address_type for change
return g_address_type;
There was a problem hiding this comment.
I find your version more readable as well, so switched to that.
test/functional/address_types.py
Outdated
There was a problem hiding this comment.
please update the lines above to say there are 5 nodes under test, and describe what node4 is for.
test/functional/address_types.py
Outdated
test/functional/address_types.py
Outdated
test/functional/address_types.py
Outdated
test/functional/address_types.py
Outdated
There was a problem hiding this comment.
I think the whole docstring could be updated to reflect better what the test is actually doing:
"""Test that the wallet can send and receive using all combinations of address types.
There are 5 nodes-under-test:
- node0 uses legacy addresses
- node1 uses p2sh/segwit addresses
- node2 uses p2sh/segwit addresses and bech32 addresses for change
- node3 uses bech32 addresses
- node4 uses p2sh/segwit addresses for change
node5 exists to generate new blocks.
## Multisig address test
Test that adding a multisig address with:
- an uncompressed pubkey always gives a legacy address
- only compressed pubkeys gives the an `-addresstype` address
## Sending to address types test
A series of tests, iterating over node0-node4. In each iteration of the test, one node sends:
- 10/101th of its balance to itself (using getrawchangeaddress for single key addresses)
- 20/101th to the next node
- 30/101th to the node after that
- 40/101th to the remaining node
- 1/101th remains as fee+change
Iterate over each node for single key addresses, and then over each node for
multisig addresses.
Repeat test, but with explicit address_type parameters passed to getnewaddress
and getrawchangeaddress:
- node0 and node3 send to p2sh.
- node1 sends to bech32.
- node2 sends to legacy.
As every node sends coins after receiving, this also
verifies that spending coins sent to all these address types works.
## Change type test
Test that the nodes generate the correct change address type:
- node0 always uses a legacy change address.
- node1 uses a bech32 addresses for change if any destination address is bech32.
- node4 always uses p2sh/segwit output for change.
"""
There was a problem hiding this comment.
That's much clearer, thanks. I also ordered the change address tests by node id added node 2 and 3 to the description.
test/functional/address_types.py
Outdated
There was a problem hiding this comment.
Suggest you make all of these logs INFO level
test/functional/address_types.py
Outdated
There was a problem hiding this comment.
suggest you make these named arguments so it's clear what the "" is for.
test/functional/address_types.py
Outdated
There was a problem hiding this comment.
Perhaps clearer as:
self.extra_args = [["-addresstype=legacy"], # node0
["-addresstype=p2sh-segwit"], # node1
["-addresstype=p2sh-segwit", "-changetype=bech32"], # node2
["-addresstype=bech32"], # node3
["-changetype=p2sh-segwit"], # node4
[]] # node5
test/functional/address_types.py
Outdated
There was a problem hiding this comment.
I think it's fine to join these lines
Only if -changetype is not set and -addresstype is not "legacy".
| return true; | ||
| } | ||
|
|
||
| OutputType CWallet::TransactionChangeType(const std::vector<CRecipient>& vecSend) |
There was a problem hiding this comment.
TransactionChangeType() doesn't really need to be a class member function on CWallet, and could be a utility function in walletutil.cpp. In general, it'd be nice to start making the wallet class and wallet.cpp a bit smaller, but that's probably a tidy-up for another day.
There was a problem hiding this comment.
Agree, this should be moved at some point.
|
utACK 596c446 |
… P2WPKH or P2WSH 596c446 [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH (Sjors Provoost) Pull request description: If `-changetype` is not explicitly set, then regardless of `-addresstype`, the wallet will use a ~`bech32` change address~ `P2WPKH` change output if any destination is `P2WPKH` or `P2WSH`. This seems more intuitive to me and more in line with the spirit of [BIP-69](https://github.com/bitcoin/bips/blob/master/bip-0069.mediawiki). When combined with #11991 a QT user could opt to use `bech32` exclusively without having to figure out how to launch with `-changetype=bech32`, although so would #11937. Tree-SHA512: 9238d3ccd1f3be8dfdd43444ccf45d6bdc6584ced3172a3045f3ecfec4a7cc8999db0cdb76ae49236492a84e6dbf3a1fdf18544d3eaf6d518e1f8bd241db33e7
| OutputType CWallet::TransactionChangeType(const std::vector<CRecipient>& vecSend) | ||
| { | ||
| // If -changetype is specified, always use that change type. | ||
| if (g_change_type != OUTPUT_TYPE_NONE) { |
There was a problem hiding this comment.
For the future, it might be a good idea add change_type and address_type fields to CoinControl class, and have this method accept a CoinControl argument, instead of relying on the globals. This would make it easier to ensure that when an RPC is being called that has address type options, that they will take precedence over the globals.
Pieter's suggestion of adding a separate P2SH_SEGWIT_BUT_NATIVE_WHEN_NATIVE_DESTINATION enum value instead of assigning multiple meanings to OUTPUT_TYPE_NONE is also a good idea #12119 (comment).
Other cleanups are also possible. I think a nice refactoring PR that didn't change behavior could be submitted that:
- Changed OutputType from enum to enum class.
- Got rid of
DEFAULTenum value and just set P2SH_SEGWIT as default during initialization. - Got rid of
NONEenum value, replacing it withPARSE_ERRORandP2SH_SEGWIT_BUT_NATIVE_WHEN_NATIVE_DESTINATION(or something shorter likeMATCH_OTHER_OUTPUTS).
There was a problem hiding this comment.
Nice wrap up @ryanofsky. In #12194 I'm adding change type to coin control, however I'm not passing coin control here.
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
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky) Pull request description: Based on suggestion by @sipa #12119 (comment) After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes. Follows up #12408 by @MarcoFalke Followups for future PRs: - [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: #12729 (comment) and #12729 (comment) - [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: #12729 (comment). Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin/bitcoin#12119 (comment) After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes.
Summary: Based on suggestion by Pieter Wuille <pieter.wuille@gmail.com> at bitcoin/bitcoin#12119 (comment) After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes. This is a backprt of Core PR12729 Test Plan: make check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D4423
…WPKH or P2WSH Summary: Only if -changetype is not set and -addresstype is not "legacy". Implement some of the test changes from [[bitcoin/bitcoin#12119 | PR12119]] . Basically stripped everything that is segwit realted, which is almost everything. The 5th node is now redundant. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6134
…WPKH or P2WSH Summary: Only if -changetype is not set and -addresstype is not "legacy". Implement some of the test changes from [[bitcoin/bitcoin#12119 | PR12119]] . Basically stripped everything that is segwit realted, which is almost everything. The 5th node is now redundant. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6134
1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky) Pull request description: Based on suggestion by @sipa bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes. Follows up bitcoin#12408 by @MarcoFalke Followups for future PRs: - [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: bitcoin#12729 (comment) and bitcoin#12729 (comment) - [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: bitcoin#12729 (comment). Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48
If
-changetypeis not explicitly set, then regardless of-addresstype, the wallet will use abech32change addressP2WPKHchange output if any destination isP2WPKHorP2WSH.This seems more intuitive to me and more in line with the spirit of BIP-69.
When combined with #11991 a QT user could opt to use
bech32exclusively without having to figure out how to launch with-changetype=bech32, although so would #11937.