diff options
| -rw-r--r-- | src/qt/paymentserver.cpp | 5 | ||||
| -rw-r--r-- | src/wallet/init.cpp | 8 | ||||
| -rw-r--r-- | src/wallet/rpcwallet.cpp | 4 | ||||
| -rw-r--r-- | src/wallet/wallet.cpp | 34 | ||||
| -rw-r--r-- | src/wallet/wallet.h | 2 | ||||
| -rwxr-xr-x | test/functional/address_types.py | 125 |
6 files changed, 154 insertions, 24 deletions
diff --git a/src/qt/paymentserver.cpp b/src/qt/paymentserver.cpp index dc729649b8..bc69d4f945 100644 --- a/src/qt/paymentserver.cpp +++ b/src/qt/paymentserver.cpp @@ -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); diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index 0f453f154f..7d61d708a1 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -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() 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", ""))); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index e307623fd5..59653721c3 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -256,9 +256,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())); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index da721ea008..c804338952 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2644,6 +2644,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) { @@ -2739,8 +2767,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); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 53a2c6b9d5..cc350853ea 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -961,6 +961,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(); diff --git a/test/functional/address_types.py b/test/functional/address_types.py index cb119c04b0..32c561aae3 100755 --- a/test/functional/address_types.py +++ b/test/functional/address_types.py @@ -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: - 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): # 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): 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): 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() |