Browse Source

Merge #12119: [wallet] use P2WPKH change output if any destination is 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
0.16
Wladimir J. van der Laan 7 years ago
parent
commit
95941396ff
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
  1. 5
      src/qt/paymentserver.cpp
  2. 8
      src/wallet/init.cpp
  3. 4
      src/wallet/rpcwallet.cpp
  4. 34
      src/wallet/wallet.cpp
  5. 2
      src/wallet/wallet.h
  6. 125
      test/functional/address_types.py

5
src/qt/paymentserver.cpp

@ -643,8 +643,9 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r @@ -643,8 +643,9 @@ void PaymentServer::fetchPaymentACK(CWallet* wallet, const SendCoinsRecipient& r
// use for change. Despite an actual payment and not change, this is a close match:
// it's the output type we use subject to privacy issues, but not restricted by what
// other software supports.
wallet->LearnRelatedScripts(newKey, g_change_type);
CTxDestination dest = GetDestinationForKey(newKey, g_change_type);
const OutputType change_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
wallet->LearnRelatedScripts(newKey, change_type);
CTxDestination dest = GetDestinationForKey(newKey, change_type);
wallet->SetAddressBook(dest, strAccount, "refund");
CScript s = GetScriptForDestination(dest);

8
src/wallet/init.cpp

@ -17,7 +17,7 @@ std::string GetWalletHelpString(bool showDebug) @@ -17,7 +17,7 @@ std::string GetWalletHelpString(bool showDebug)
{
std::string strUsage = HelpMessageGroup(_("Wallet options:"));
strUsage += HelpMessageOpt("-addresstype", strprintf(_("What type of addresses to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default: \"%s\")"), FormatOutputType(OUTPUT_TYPE_DEFAULT)));
strUsage += HelpMessageOpt("-changetype", _("What type of change to use (\"legacy\", \"p2sh-segwit\", or \"bech32\", default is same as -addresstype)"));
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)"));
strUsage += HelpMessageOpt("-disablewallet", _("Do not load the wallet and disable wallet RPC calls"));
strUsage += HelpMessageOpt("-keypool=<n>", strprintf(_("Set key pool size to <n> (default: %u)"), DEFAULT_KEYPOOL_SIZE));
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() @@ -182,8 +182,10 @@ bool WalletParameterInteraction()
return InitError(strprintf(_("Unknown address type '%s'"), gArgs.GetArg("-addresstype", "")));
}
g_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), g_address_type);
if (g_change_type == OUTPUT_TYPE_NONE) {
// If changetype is set in config file or parameter, check that it's valid.
// Default to OUTPUT_TYPE_NONE if not set.
g_change_type = ParseOutputType(gArgs.GetArg("-changetype", ""), OUTPUT_TYPE_NONE);
if (g_change_type == OUTPUT_TYPE_NONE && !gArgs.GetArg("-changetype", "").empty()) {
return InitError(strprintf(_("Unknown change type '%s'"), gArgs.GetArg("-changetype", "")));
}

4
src/wallet/rpcwallet.cpp

@ -257,9 +257,9 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request) @@ -257,9 +257,9 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request)
pwallet->TopUpKeyPool();
}
OutputType output_type = g_change_type;
OutputType output_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type;
if (!request.params[0].isNull()) {
output_type = ParseOutputType(request.params[0].get_str(), g_change_type);
output_type = ParseOutputType(request.params[0].get_str(), output_type);
if (output_type == OUTPUT_TYPE_NONE) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("Unknown address type '%s'", request.params[0].get_str()));
}

34
src/wallet/wallet.cpp

@ -2674,6 +2674,34 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC @@ -2674,6 +2674,34 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
return true;
}
OutputType CWallet::TransactionChangeType(const std::vector<CRecipient>& vecSend)
{
// 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;
}
bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletTx& wtxNew, CReserveKey& reservekey, CAmount& nFeeRet,
int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign)
{
@ -2769,8 +2797,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT @@ -2769,8 +2797,10 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CWalletT
return false;
}
LearnRelatedScripts(vchPubKey, g_change_type);
scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, g_change_type));
const OutputType change_type = TransactionChangeType(vecSend);
LearnRelatedScripts(vchPubKey, change_type);
scriptChange = GetScriptForDestination(GetDestinationForKey(vchPubKey, change_type));
}
CTxOut change_prototype_txout(0, scriptChange);
size_t change_prototype_size = GetSerializeSize(change_prototype_txout, SER_DISK, 0);

2
src/wallet/wallet.h

@ -965,6 +965,8 @@ public: @@ -965,6 +965,8 @@ public:
CAmount GetLegacyBalance(const isminefilter& filter, int minDepth, const std::string* account) const;
CAmount GetAvailableBalance(const CCoinControl* coinControl = nullptr) const;
OutputType TransactionChangeType(const std::vector<CRecipient>& vecSend);
/**
* Insert additional inputs into the transaction by
* calling CreateTransaction();

125
test/functional/address_types.py

@ -4,16 +4,24 @@ @@ -4,16 +4,24 @@
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test that the wallet can send and receive using all combinations of address types.
There are 4 nodes-under-test:
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 a p2sh/segwit addresses for change
node4 exists to generate new blocks.
node5 exists to generate new blocks.
The script is a series of tests, iterating over the 4 nodes. In each iteration
of the test, one node sends:
## 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
@ -21,21 +29,51 @@ of the test, one node sends: @@ -21,21 +29,51 @@ of the test, one node sends:
- 1/101th remains as fee+change
Iterate over each node for single key addresses, and then over each node for
multisig addresses. In a second iteration, the same is done, but with explicit address_type
parameters passed to getnewaddress and getrawchangeaddress. Node0 and node3 send to p2sh,
node 1 sends to bech32, and node2 sends to legacy. As every node sends coins after receiving,
this also verifies that spending coins sent to all these address types works."""
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.
- node2 always uses a bech32 address for change
- node3 always uses a bech32 address for change
- node4 always uses p2sh/segwit output for change.
"""
from decimal import Decimal
import itertools
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import assert_equal, assert_greater_than, connect_nodes_bi, sync_blocks, sync_mempools
from test_framework.util import (
assert_equal,
assert_greater_than,
assert_raises_rpc_error,
connect_nodes_bi,
sync_blocks,
sync_mempools,
)
class AddressTypeTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 5
self.extra_args = [["-addresstype=legacy"], ["-addresstype=p2sh-segwit"], ["-addresstype=p2sh-segwit", "-changetype=bech32"], ["-addresstype=bech32"], []]
self.num_nodes = 6
self.extra_args = [
["-addresstype=legacy"],
["-addresstype=p2sh-segwit"],
["-addresstype=p2sh-segwit", "-changetype=bech32"],
["-addresstype=bech32"],
["-changetype=p2sh-segwit"],
[]
]
def setup_network(self):
self.setup_nodes()
@ -104,10 +142,26 @@ class AddressTypeTest(BitcoinTestFramework): @@ -104,10 +142,26 @@ class AddressTypeTest(BitcoinTestFramework):
# Unknown type
assert(False)
def test_change_output_type(self, node_sender, destinations, expected_type):
txid = self.nodes[node_sender].sendmany(fromaccount="", amounts=dict.fromkeys(destinations, 0.001))
raw_tx = self.nodes[node_sender].getrawtransaction(txid)
tx = self.nodes[node_sender].decoderawtransaction(raw_tx)
# Make sure the transaction has change:
assert_equal(len(tx["vout"]), len(destinations) + 1)
# Make sure the destinations are included, and remove them:
output_addresses = [vout['scriptPubKey']['addresses'][0] for vout in tx["vout"]]
change_addresses = [d for d in output_addresses if d not in destinations]
assert_equal(len(change_addresses), 1)
self.log.debug("Check if change address " + change_addresses[0] + " is " + expected_type)
self.test_address(node_sender, change_addresses[0], multisig=False, typ=expected_type)
def run_test(self):
# Mine 101 blocks on node4 to bring nodes out of IBD and make sure that
# Mine 101 blocks on node5 to bring nodes out of IBD and make sure that
# no coinbases are maturing for the nodes-under-test during the test
self.nodes[4].generate(101)
self.nodes[5].generate(101)
sync_blocks(self.nodes)
uncompressed_1 = "0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee"
@ -182,8 +236,8 @@ class AddressTypeTest(BitcoinTestFramework): @@ -182,8 +236,8 @@ class AddressTypeTest(BitcoinTestFramework):
to_node %= 4
assert_equal(unconf_balances[to_node], to_send * 10 * (2 + n))
# node4 collects fee and block subsidy to keep accounting simple
self.nodes[4].generate(1)
# node5 collects fee and block subsidy to keep accounting simple
self.nodes[5].generate(1)
sync_blocks(self.nodes)
new_balances = self.get_balances()
@ -195,5 +249,46 @@ class AddressTypeTest(BitcoinTestFramework): @@ -195,5 +249,46 @@ class AddressTypeTest(BitcoinTestFramework):
to_node %= 4
assert_equal(new_balances[to_node], old_balances[to_node] + to_send * 10 * (2 + n))
# Get one p2sh/segwit address from node2 and two bech32 addresses from node3:
to_address_p2sh = self.nodes[2].getnewaddress()
to_address_bech32_1 = self.nodes[3].getnewaddress()
to_address_bech32_2 = self.nodes[3].getnewaddress()
# Fund node 4:
self.nodes[5].sendtoaddress(self.nodes[4].getnewaddress(), Decimal("1"))
self.nodes[5].generate(1)
sync_blocks(self.nodes)
assert_equal(self.nodes[4].getbalance(), 1)
self.log.info("Nodes with addresstype=legacy never use a P2WPKH change output")
self.test_change_output_type(0, [to_address_bech32_1], 'legacy')
self.log.info("Nodes with addresstype=p2sh-segwit only use a P2WPKH change output if any destination address is bech32:")
self.test_change_output_type(1, [to_address_p2sh], 'p2sh-segwit')
self.test_change_output_type(1, [to_address_bech32_1], 'bech32')
self.test_change_output_type(1, [to_address_p2sh, to_address_bech32_1], 'bech32')
self.test_change_output_type(1, [to_address_bech32_1, to_address_bech32_2], 'bech32')
self.log.info("Nodes with change_type=bech32 always use a P2WPKH change output:")
self.test_change_output_type(2, [to_address_bech32_1], 'bech32')
self.test_change_output_type(2, [to_address_p2sh], 'bech32')
self.log.info("Nodes with addresstype=bech32 always use a P2WPKH change output (unless changetype is set otherwise):")
self.test_change_output_type(3, [to_address_bech32_1], 'bech32')
self.test_change_output_type(3, [to_address_p2sh], 'bech32')
self.log.info('getrawchangeaddress defaults to addresstype if -changetype is not set and argument is absent')
self.test_address(3, self.nodes[3].getrawchangeaddress(), multisig=False, typ='bech32')
self.log.info('getrawchangeaddress fails with invalid changetype argument')
assert_raises_rpc_error(-5, "Unknown address type 'bech23'", self.nodes[3].getrawchangeaddress, 'bech23')
self.log.info("Nodes with changetype=p2sh-segwit never use a P2WPKH change output")
self.test_change_output_type(4, [to_address_bech32_1], 'p2sh-segwit')
self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
self.log.info("Except for getrawchangeaddress if specified:")
self.test_address(4, self.nodes[4].getrawchangeaddress(), multisig=False, typ='p2sh-segwit')
self.test_address(4, self.nodes[4].getrawchangeaddress('bech32'), multisig=False, typ='bech32')
if __name__ == '__main__':
AddressTypeTest().main()

Loading…
Cancel
Save