Skip to content

Commit 596c446

Browse files
SjorsSjors
committed
[wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH
Only if -changetype is not set and -addresstype is not "legacy".
1 parent 44080a9 commit 596c446

6 files changed

Lines changed: 154 additions & 24 deletions

File tree

‎src/qt/paymentserver.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -643,8 +643,9 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
643643
// use for change. Despite an actual payment and not change, this is a close match:
644644
// it's the output type we use subject to privacy issues, but not restricted by what
645645
// other software supports.
646-
wallet->LearnRelatedScripts(newKey, g_change_type);
647-
CTxDestination dest = GetDestinationForKey(newKey, g_change_type);
646+
const OutputType change_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
647+
wallet->LearnRelatedScripts(newKey, change_type);
648+
CTxDestination dest = GetDestinationForKey(newKey, change_type);
648649
wallet->SetAddressBook(dest, strAccount, "refund");
649650

650651
CScript s = GetScriptForDestination(dest);

‎src/wallet/init.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ std::string GetWalletHelpString(bool showDebug)
1717
{
1818
std::string strUsage = HelpMessageGroup(_("Wallet options:"));
1919
strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
20-
strUsage += HelpMessageOpt("-changetype", _("What type of change to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default is same as -addresstype)"));
20+
strUsage += HelpMessageOpt("-changetype", _("What type of change to use (\"legacy\", \"p2sh-segwit\", or \"bech32\"). Default is same as -addresstype, except when -addresstype=p2sh-segwit a native segwit output is used when sending to a native segwit address)"));
2121
strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
2222
strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), DEFAULT_KEYPOOL_SIZE));
2323
strUsage += HelpMessageOpt("-fallbackfee=<amt>", strprintf(_("A fee rate (in %s/kB) that will be used when fee estimation has insufficient data (default: %s)"),
@@ -182,8 +182,10 @@ bool WalletParameterInteraction()
182182
return InitError(strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", "")));
183183
}
184184

185-
g_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), g_address_type);
186-
if (g_change_type == OUTPUT_TYPE_NONE) {
185+
// If changetype is set in config file or parameter, check that it's valid.
186+
// Default to OUTPUT_TYPE_NONE if not set.
187+
g_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), OUTPUT_TYPE_NONE);
188+
if (g_change_type == OUTPUT_TYPE_NONE && !gArgs.GetArg("-changetype", "").empty()) {
187189
return InitError(strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", "")));
188190
}
189191

‎src/wallet/rpcwallet.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -256,9 +256,9 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
256256
pwallet->TopUpKeyPool();
257257
}
258258

259-
OutputType output_type = g_change_type;
259+
OutputType output_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
260260
if (!request.params[0].isNull()) {
261-
output_type = ParseOutputType(request.params[0].get_str(), g_change_type);
261+
output_type = ParseOutputType(request.params[0].get_str(), output_type);
262262
if (output_type == OUTPUT_TYPE_NONE) {
263263
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
264264
}

‎src/wallet/wallet.cpp‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2644,6 +2644,34 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
26442644
return true;
26452645
}
26462646

2647+
OutputType CWallet::TransactionChangeType(const std::vector<CRecipient>& vecSend)
2648+
{
2649+
// If -changetype is specified, always use that change type.
2650+
if (g_change_type != OUTPUT_TYPE_NONE) {
2651+
return g_change_type;
2652+
}
2653+
2654+
// if g_address_type is legacy, use legacy address as change (even
2655+
// if some of the outputs are P2WPKH or P2WSH).
2656+
if (g_address_type == OUTPUT_TYPE_LEGACY) {
2657+
return OUTPUT_TYPE_LEGACY;
2658+
}
2659+
2660+
// if any destination is P2WPKH or P2WSH, use P2WPKH for the change
2661+
// output.
2662+
for (const auto& recipient : vecSend) {
2663+
// Check if any destination contains a witness program:
2664+
int witnessversion = 0;
2665+
std::vector<unsigned char> witnessprogram;
2666+
if (recipient.scriptPubKey.IsWitnessProgram(witnessversion, witnessprogram)) {
2667+
return OUTPUT_TYPE_BECH32;
2668+
}
2669+
}
2670+
2671+
// else use g_address_type for change
2672+
return g_address_type;
2673+
}
2674+
26472675
bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet,
26482676
int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
26492677
{
@@ -2739,8 +2767,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
27392767
return false;
27402768
}
27412769

2742-
LearnRelatedScripts(vchPubKey, g_change_type);
2743-
scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, g_change_type));
2770+
const OutputType change_type = TransactionChangeType(vecSend);
2771+
2772+
LearnRelatedScripts(vchPubKey, change_type);
2773+
scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, change_type));
27442774
}
27452775
CTxOut change_prototype_txout(0, scriptChange);
27462776
size_t change_prototype_size = GetSerializeSize(change_prototype_txout, SER_DISK, 0);

‎src/wallet/wallet.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -961,6 +961,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface
961961
CAmount GetLegacyBalance(const isminefilter& filter, int minDepth, const std::string* account) const;
962962
CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const;
963963

964+
OutputType TransactionChangeType(const std::vector<CRecipient>& vecSend);
965+
964966
/**
965967
* Insert additional inputs into the transaction by
966968
* calling CreateTransaction();

‎test/functional/address_types.py‎

Lines changed: 110 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,38 +4,76 @@
44
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
55
"""Test that the wallet can send and receive using all combinations of address types.
66
7-
There are 4 nodes-under-test:
7+
There are 5 nodes-under-test:
88
- node0 uses legacy addresses
99
- node1 uses p2sh/segwit addresses
1010
- node2 uses p2sh/segwit addresses and bech32 addresses for change
1111
- node3 uses bech32 addresses
12+
- node4 uses a p2sh/segwit addresses for change
1213
13-
node4 exists to generate new blocks.
14+
node5 exists to generate new blocks.
1415
15-
The script is a series of tests, iterating over the 4 nodes. In each iteration
16-
of the test, one node sends:
16+
## Multisig address test
17+
18+
Test that adding a multisig address with:
19+
- an uncompressed pubkey always gives a legacy address
20+
- only compressed pubkeys gives the an `-addresstype` address
21+
22+
## Sending to address types test
23+
24+
A series of tests, iterating over node0-node4. In each iteration of the test, one node sends:
1725
- 10/101th of its balance to itself (using getrawchangeaddress for single key addresses)
1826
- 20/101th to the next node
1927
- 30/101th to the node after that
2028
- 40/101th to the remaining node
2129
- 1/101th remains as fee+change
2230
2331
Iterate over each node for single key addresses, and then over each node for
24-
multisig addresses. In a second iteration, the same is done, but with explicit address_type
25-
parameters passed to getnewaddress and getrawchangeaddress. Node0 and node3 send to p2sh,
26-
node 1 sends to bech32, and node2 sends to legacy. As every node sends coins after receiving,
27-
this also verifies that spending coins sent to all these address types works."""
32+
multisig addresses.
33+
34+
Repeat test, but with explicit address_type parameters passed to getnewaddress
35+
and getrawchangeaddress:
36+
- node0 and node3 send to p2sh.
37+
- node1 sends to bech32.
38+
- node2 sends to legacy.
39+
40+
As every node sends coins after receiving, this also
41+
verifies that spending coins sent to all these address types works.
42+
43+
## Change type test
44+
45+
Test that the nodes generate the correct change address type:
46+
- node0 always uses a legacy change address.
47+
- node1 uses a bech32 addresses for change if any destination address is bech32.
48+
- node2 always uses a bech32 address for change
49+
- node3 always uses a bech32 address for change
50+
- node4 always uses p2sh/segwit output for change.
51+
"""
2852

2953
from decimal import Decimal
3054
import itertools
3155

3256
from test_framework.test_framework import BitcoinTestFramework
33-
from test_framework.util import assert_equal, assert_greater_than, connect_nodes_bi, sync_blocks, sync_mempools
57+
from test_framework.util import (
58+
assert_equal,
59+
assert_greater_than,
60+
assert_raises_rpc_error,
61+
connect_nodes_bi,
62+
sync_blocks,
63+
sync_mempools,
64+
)
3465

3566
class AddressTypeTest(BitcoinTestFramework):
3667
def set_test_params(self):
37-
self.num_nodes = 5
38-
self.extra_args = [["-addresstype=legacy"], ["-addresstype=p2sh-segwit"], ["-addresstype=p2sh-segwit", "-changetype=bech32"], ["-addresstype=bech32"], []]
68+
self.num_nodes = 6
69+
self.extra_args = [
70+
["-addresstype=legacy"],
71+
["-addresstype=p2sh-segwit"],
72+
["-addresstype=p2sh-segwit", "-changetype=bech32"],
73+
["-addresstype=bech32"],
74+
["-changetype=p2sh-segwit"],
75+
[]
76+
]
3977

4078
def setup_network(self):
4179
self.setup_nodes()
@@ -104,10 +142,26 @@ def test_address(self, node, address, multisig, typ):
104142
# Unknown type
105143
assert(False)
106144

145+
def test_change_output_type(self, node_sender, destinations, expected_type):
146+
txid = self.nodes[node_sender].sendmany(fromaccount="", amounts=dict.fromkeys(destinations, 0.001))
147+
raw_tx = self.nodes[node_sender].getrawtransaction(txid)
148+
tx = self.nodes[node_sender].decoderawtransaction(raw_tx)
149+
150+
# Make sure the transaction has change:
151+
assert_equal(len(tx["vout"]), len(destinations) + 1)
152+
153+
# Make sure the destinations are included, and remove them:
154+
output_addresses = [vout['scriptPubKey']['addresses'][0] for vout in tx["vout"]]
155+
change_addresses = [d for d in output_addresses if d not in destinations]
156+
assert_equal(len(change_addresses), 1)
157+
158+
self.log.debug("Check if change address " + change_addresses[0] + " is " + expected_type)
159+
self.test_address(node_sender, change_addresses[0], multisig=False, typ=expected_type)
160+
107161
def run_test(self):
108-
# Mine 101 blocks on node4 to bring nodes out of IBD and make sure that
162+
# Mine 101 blocks on node5 to bring nodes out of IBD and make sure that
109163
# no coinbases are maturing for the nodes-under-test during the test
110-
self.nodes[4].generate(101)
164+
self.nodes[5].generate(101)
111165
sync_blocks(self.nodes)
112166

113167
uncompressed_1 = "0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee"
@@ -182,8 +236,8 @@ def run_test(self):
182236
to_node %= 4
183237
assert_equal(unconf_balances[to_node], to_send * 10 * (2 + n))
184238

185-
# node4 collects fee and block subsidy to keep accounting simple
186-
self.nodes[4].generate(1)
239+
# node5 collects fee and block subsidy to keep accounting simple
240+
self.nodes[5].generate(1)
187241
sync_blocks(self.nodes)
188242

189243
new_balances = self.get_balances()
@@ -195,5 +249,46 @@ def run_test(self):
195249
to_node %= 4
196250
assert_equal(new_balances[to_node], old_balances[to_node] + to_send * 10 * (2 + n))
197251

252+
# Get one p2sh/segwit address from node2 and two bech32 addresses from node3:
253+
to_address_p2sh = self.nodes[2].getnewaddress()
254+
to_address_bech32_1 = self.nodes[3].getnewaddress()
255+
to_address_bech32_2 = self.nodes[3].getnewaddress()
256+
257+
# Fund node 4:
258+
self.nodes[5].sendtoaddress(self.nodes[4].getnewaddress(), Decimal("1"))
259+
self.nodes[5].generate(1)
260+
sync_blocks(self.nodes)
261+
assert_equal(self.nodes[4].getbalance(), 1)
262+
263+
self.log.info("Nodes with addresstype=legacy never use a P2WPKH change output")
264+
self.test_change_output_type(0, [to_address_bech32_1], 'legacy')
265+
266+
self.log.info("Nodes with addresstype=p2sh-segwit only use a P2WPKH change output if any destination address is bech32:")
267+
self.test_change_output_type(1, [to_address_p2sh], 'p2sh-segwit')
268+
self.test_change_output_type(1, [to_address_bech32_1], 'bech32')
269+
self.test_change_output_type(1, [to_address_p2sh, to_address_bech32_1], 'bech32')
270+
self.test_change_output_type(1, [to_address_bech32_1, to_address_bech32_2], 'bech32')
271+
272+
self.log.info("Nodes with change_type=bech32 always use a P2WPKH change output:")
273+
self.test_change_output_type(2, [to_address_bech32_1], 'bech32')
274+
self.test_change_output_type(2, [to_address_p2sh], 'bech32')
275+
276+
self.log.info("Nodes with addresstype=bech32 always use a P2WPKH change output (unless changetype is set otherwise):")
277+
self.test_change_output_type(3, [to_address_bech32_1], 'bech32')
278+
self.test_change_output_type(3, [to_address_p2sh], 'bech32')
279+
280+
self.log.info('getrawchangeaddress defaults to addresstype if -changetype is not set and argument is absent')
281+
self.test_address(3, self.nodes[3].getrawchangeaddress(), multisig=False, typ='bech32')
282+
283+
self.log.info('getrawchangeaddress fails with invalid changetype argument')
284+
assert_raises_rpc_error(-5, "Unknown address type 'bech23'", self.nodes[3].getrawchangeaddress, 'bech23')
285+
286+
self.log.info("Nodes with changetype=p2sh-segwit never use a P2WPKH change output")
287+
self.test_change_output_type(4, [to_address_bech32_1], 'p2sh-segwit')
288+
self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
289+
self.log.info("Except for getrawchangeaddress if specified:")
290+
self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
291+
self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')
292+
198293
if __name__ == '__main__':
199294
AddressTypeTest().main()

0 commit comments

Comments
 (0)