Merge #11403: SegWit wallet support

b224a47a1 Add address_types test (Pieter Wuille)
7ee54fd7c Support downgrading after recovered keypool witness keys (Pieter Wuille)
940a21932 SegWit wallet support (Pieter Wuille)
f37c64e47 Implicitly know about P2WPKH redeemscripts (Pieter Wuille)
57273f2b3 [test] Serialize CTransaction with witness by default (Pieter Wuille)
cf2c0b6f5 Support P2WPKH and P2SH-P2WPKH in dumpprivkey (Pieter Wuille)
37c03d3e0 Support P2WPKH addresses in create/addmultisig (Pieter Wuille)
3eaa003c8 Extend validateaddress information for P2SH-embedded witness (Pieter Wuille)
30a27dc5b Expose method to find key for a single-key destination (Pieter Wuille)
985c79552 Improve witness destination types and use them more (Pieter Wuille)
cbe197470 [refactor] GetAccount{PubKey,Address} -> GetAccountDestination (Pieter Wuille)
0c8ea6380 Abstract out IsSolvable from Witnessifier (Pieter Wuille)

Pull request description:

  This implements a minimum viable implementation of SegWit wallet support, based on top of #11389, and includes part of the functionality from #11089.

  Two new configuration options are added:
  * `-addresstype`, with options `legacy`, `p2sh`, and `bech32`. It controls what kind of addresses are produced by `getnewaddress`, `getaccountaddress`, and `createmultisigaddress`.
  * `-changetype`, with the same options, and by default equal to `-addresstype`, that controls what kind of change is used.

  All wallet private and public keys can be used for any type of address. Support for address types dependent on different derivation paths will need a major overhaul of how our internal detection of outputs work. I expect that that will happen for a next major version.

  The above also applies to imported keys, as having a distinction there but not for normal operations is a disaster for testing, and probably for comprehension of users. This has some ugly effects, like needing to associate the provided label to `importprivkey` with each style address for the corresponding key.

  To deal with witness outputs requiring a corresponding redeemscript in wallet, three approaches are used:
  * All SegWit addresses created through `getnewaddress` or multisig RPCs explicitly get their redeemscripts added to the wallet file. This means that downgrading after creating a witness address will work, as long as the wallet file is up to date.
  * All SegWit keys in the wallet get an _implicit_ redeemscript added, without it being written to the file. This means recovery of an old backup will work, as long as you use new software.
  * All keypool keys that are seen used in transactions explicitly get their redeemscripts added to the wallet files. This means that downgrading after recovering from a backup that includes a witness address will work.

  These approaches correspond to solutions 3a, 1a, and 5a respectively from https://gist.github.com/sipa/125cfa1615946d0c3f3eec2ad7f250a2. As argued there, there is no full solution for dealing with the case where you both downgrade and restore a backup, so that's also not implemented.

  `dumpwallet`, `importwallet`, `importmulti`, `signmessage` and `verifymessage` don't work with SegWit addresses yet. They're remaining TODOs, for this PR or a follow-up. Because of that, several tests unexpectedly run with `-addresstype=legacy` for now.

Tree-SHA512: d425dbe517c0422061ab8dacdc3a6ae47da071450932ed992c79559d922dff7b2574a31a8c94feccd3761c1dffb6422c50055e6dca8e3cf94a169bc95e39e959

18 files changed, 280 insertions, 50 deletions

diff --git a/test/functional/address_types.py b/test/functional/address_types.py
new file mode 100755
index 0000000000..cb119c04b0
--- /dev/null
+++ b/test/functional/address_types.py
@@ -0,0 +1,199 @@
+#!/usr/bin/env python3
+# Copyright (c) 2017 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# 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:
+    - node0 uses legacy addresses
+    - node1 uses p2sh/segwit addresses
+    - node2 uses p2sh/segwit addresses and bech32 addresses for change
+    - node3 uses bech32 addresses
+
+node4 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:
+    - 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. 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."""
+
+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
+
+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"], []]
+
+    def setup_network(self):
+        self.setup_nodes()
+
+        # Fully mesh-connect nodes for faster mempool sync
+        for i, j in itertools.product(range(self.num_nodes), repeat=2):
+            if i > j:
+                connect_nodes_bi(self.nodes, i, j)
+        self.sync_all()
+
+    def get_balances(self, confirmed=True):
+        """Return a list of confirmed or unconfirmed balances."""
+        if confirmed:
+            return [self.nodes[i].getbalance() for i in range(4)]
+        else:
+            return [self.nodes[i].getunconfirmedbalance() for i in range(4)]
+
+    def test_address(self, node, address, multisig, typ):
+        """Run sanity checks on an address."""
+        info = self.nodes[node].validateaddress(address)
+        assert(info['isvalid'])
+        if not multisig and typ == 'legacy':
+            # P2PKH
+            assert(not info['isscript'])
+            assert(not info['iswitness'])
+            assert('pubkey' in info)
+        elif not multisig and typ == 'p2sh-segwit':
+            # P2SH-P2WPKH
+            assert(info['isscript'])
+            assert(not info['iswitness'])
+            assert_equal(info['script'], 'witness_v0_keyhash')
+            assert('pubkey' in info)
+        elif not multisig and typ == 'bech32':
+            # P2WPKH
+            assert(not info['isscript'])
+            assert(info['iswitness'])
+            assert_equal(info['witness_version'], 0)
+            assert_equal(len(info['witness_program']), 40)
+            assert('pubkey' in info)
+        elif typ == 'legacy':
+            # P2SH-multisig
+            assert(info['isscript'])
+            assert_equal(info['script'], 'multisig')
+            assert(not info['iswitness'])
+            assert('pubkeys' in info)
+        elif typ == 'p2sh-segwit':
+            # P2SH-P2WSH-multisig
+            assert(info['isscript'])
+            assert_equal(info['script'], 'witness_v0_scripthash')
+            assert(not info['iswitness'])
+            assert(info['embedded']['isscript'])
+            assert_equal(info['embedded']['script'], 'multisig')
+            assert(info['embedded']['iswitness'])
+            assert_equal(info['embedded']['witness_version'], 0)
+            assert_equal(len(info['embedded']['witness_program']), 64)
+            assert('pubkeys' in info['embedded'])
+        elif typ == 'bech32':
+            # P2WSH-multisig
+            assert(info['isscript'])
+            assert_equal(info['script'], 'multisig')
+            assert(info['iswitness'])
+            assert_equal(info['witness_version'], 0)
+            assert_equal(len(info['witness_program']), 64)
+            assert('pubkeys' in info)
+        else:
+            # Unknown type
+            assert(False)
+
+    def run_test(self):
+        # Mine 101 blocks on node4 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)
+        sync_blocks(self.nodes)
+
+        uncompressed_1 = "0496b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52da7589379515d4e0a604f8141781e62294721166bf621e73a82cbf2342c858ee"
+        uncompressed_2 = "047211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073dee6c89064984f03385237d92167c13e236446b417ab79a0fcae412ae3316b77"
+        compressed_1 = "0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52"
+        compressed_2 = "037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073"
+
+        # addmultisigaddress with at least 1 uncompressed key should return a legacy address.
+        for node in range(4):
+            self.test_address(node, self.nodes[node].addmultisigaddress(2, [uncompressed_1, uncompressed_2]), True, 'legacy')
+            self.test_address(node, self.nodes[node].addmultisigaddress(2, [compressed_1, uncompressed_2]), True, 'legacy')
+            self.test_address(node, self.nodes[node].addmultisigaddress(2, [uncompressed_1, compressed_2]), True, 'legacy')
+        # addmultisigaddress with all compressed keys should return the appropriate address type (even when the keys are not ours).
+        self.test_address(0, self.nodes[0].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'legacy')
+        self.test_address(1, self.nodes[1].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'p2sh-segwit')
+        self.test_address(2, self.nodes[2].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'p2sh-segwit')
+        self.test_address(3, self.nodes[3].addmultisigaddress(2, [compressed_1, compressed_2]), True, 'bech32')
+
+        for explicit_type, multisig, from_node in itertools.product([False, True], [False, True], range(4)):
+            address_type = None
+            if explicit_type and not multisig:
+                if from_node == 1:
+                    address_type = 'bech32'
+                elif from_node == 0 or from_node == 3:
+                    address_type = 'p2sh-segwit'
+                else:
+                    address_type = 'legacy'
+            self.log.info("Sending from node {} ({}) with{} multisig using {}".format(from_node, self.extra_args[from_node], "" if multisig else "out", "default" if address_type is None else address_type))
+            old_balances = self.get_balances()
+            self.log.debug("Old balances are {}".format(old_balances))
+            to_send = (old_balances[from_node] / 101).quantize(Decimal("0.00000001"))
+            sends = {}
+
+            self.log.debug("Prepare sends")
+            for n, to_node in enumerate(range(from_node, from_node + 4)):
+                to_node %= 4
+                change = False
+                if not multisig:
+                    if from_node == to_node:
+                        # When sending non-multisig to self, use getrawchangeaddress
+                        address = self.nodes[to_node].getrawchangeaddress(address_type=address_type)
+                        change = True
+                    else:
+                        address = self.nodes[to_node].getnewaddress(address_type=address_type)
+                else:
+                    addr1 = self.nodes[to_node].getnewaddress()
+                    addr2 = self.nodes[to_node].getnewaddress()
+                    address = self.nodes[to_node].addmultisigaddress(2, [addr1, addr2])
+
+                # Do some sanity checking on the created address
+                if address_type is not None:
+                    typ = address_type
+                elif to_node == 0:
+                    typ = 'legacy'
+                elif to_node == 1 or (to_node == 2 and not change):
+                    typ = 'p2sh-segwit'
+                else:
+                    typ = 'bech32'
+                self.test_address(to_node, address, multisig, typ)
+
+                # Output entry
+                sends[address] = to_send * 10 * (1 + n)
+
+            self.log.debug("Sending: {}".format(sends))
+            self.nodes[from_node].sendmany("", sends)
+            sync_mempools(self.nodes)
+
+            unconf_balances = self.get_balances(False)
+            self.log.debug("Check unconfirmed balances: {}".format(unconf_balances))
+            assert_equal(unconf_balances[from_node], 0)
+            for n, to_node in enumerate(range(from_node + 1, from_node + 4)):
+                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)
+            sync_blocks(self.nodes)
+
+            new_balances = self.get_balances()
+            self.log.debug("Check new balances: {}".format(new_balances))
+            # We don't know what fee was set, so we can only check bounds on the balance of the sending node
+            assert_greater_than(new_balances[from_node], to_send * 10)
+            assert_greater_than(to_send * 11, new_balances[from_node])
+            for n, to_node in enumerate(range(from_node + 1, from_node + 4)):
+                to_node %= 4
+                assert_equal(new_balances[to_node], old_balances[to_node] + to_send * 10 * (2 + n))
+
+if __name__ == '__main__':
+    AddressTypeTest().main()
diff --git a/test/functional/bip68-112-113-p2p.py b/test/functional/bip68-112-113-p2p.py
index 30ceed3be7..82aa0ff891 100755
--- a/test/functional/bip68-112-113-p2p.py
+++ b/test/functional/bip68-112-113-p2p.py
@@ -95,7 +95,7 @@ class BIP68_112_113Test(ComparisonTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         self.setup_clean_chain = True
-        self.extra_args = [['-whitelist=127.0.0.1', '-blockversion=4']]
+        self.extra_args = [['-whitelist=127.0.0.1', '-blockversion=4', '-addresstype=legacy']]
 
     def run_test(self):
         test = TestManager(self, self.options.tmpdir)
diff --git a/test/functional/bip68-sequence.py b/test/functional/bip68-sequence.py
index 2acdd72aff..94b13653b9 100755
--- a/test/functional/bip68-sequence.py
+++ b/test/functional/bip68-sequence.py
@@ -362,9 +362,10 @@ class BIP68Test(BitcoinTestFramework):
         block.vtx.extend([tx1, tx2, tx3])
         block.hashMerkleRoot = block.calc_merkle_root()
         block.rehash()
+        add_witness_commitment(block)
         block.solve()
 
-        self.nodes[0].submitblock(ToHex(block))
+        self.nodes[0].submitblock(bytes_to_hex_str(block.serialize(True)))
         assert_equal(self.nodes[0].getbestblockhash(), block.hash)
 
     def activateCSV(self):
diff --git a/test/functional/bumpfee.py b/test/functional/bumpfee.py
index ffd5ac197b..5cbd9f5cf7 100755
--- a/test/functional/bumpfee.py
+++ b/test/functional/bumpfee.py
@@ -194,7 +194,7 @@ def test_settxfee(rbf_node, dest_address):
     requested_feerate = Decimal("0.00025000")
     rbf_node.settxfee(requested_feerate)
     bumped_tx = rbf_node.bumpfee(rbfid)
-    actual_feerate = bumped_tx["fee"] * 1000 / rbf_node.getrawtransaction(bumped_tx["txid"], True)["size"]
+    actual_feerate = bumped_tx["fee"] * 1000 / rbf_node.getrawtransaction(bumped_tx["txid"], True)["vsize"]
     # Assert that the difference between the requested feerate and the actual
     # feerate of the bumped transaction is small.
     assert_greater_than(Decimal("0.00001000"), abs(requested_feerate - actual_feerate))
@@ -290,6 +290,7 @@ def submit_block_with_tx(node, tx):
     block.vtx.append(ctx)
     block.rehash()
     block.hashMerkleRoot = block.calc_merkle_root()
+    blocktools.add_witness_commitment(block)
     block.solve()
     node.submitblock(bytes_to_hex_str(block.serialize(True)))
     return block
diff --git a/test/functional/import-rescan.py b/test/functional/import-rescan.py
index 7d5fbb8d65..d193a99d5b 100755
--- a/test/functional/import-rescan.py
+++ b/test/functional/import-rescan.py
@@ -119,7 +119,7 @@ class ImportRescanTest(BitcoinTestFramework):
         self.num_nodes = 2 + len(IMPORT_NODES)
 
     def setup_network(self):
-        extra_args = [[] for _ in range(self.num_nodes)]
+        extra_args = [["-addresstype=legacy"] for _ in range(self.num_nodes)]
         for i, import_node in enumerate(IMPORT_NODES, 2):
             if import_node.prune:
                 extra_args[i] += ["-prune=1"]
diff --git a/test/functional/importmulti.py b/test/functional/importmulti.py
index 35cb5b6768..ab4ca48e48 100755
--- a/test/functional/importmulti.py
+++ b/test/functional/importmulti.py
@@ -9,6 +9,7 @@ from test_framework.util import *
 class ImportMultiTest (BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 2
+        self.extra_args = [["-addresstype=legacy"], ["-addresstype=legacy"]]
         self.setup_clean_chain = True
 
     def setup_network(self):
diff --git a/test/functional/nulldummy.py b/test/functional/nulldummy.py
index a94fe8204f..110b7a655e 100755
--- a/test/functional/nulldummy.py
+++ b/test/functional/nulldummy.py
@@ -42,7 +42,7 @@ class NULLDUMMYTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         # This script tests NULLDUMMY activation, which is part of the 'segwit' deployment, so we go through
         # normal segwit activation here (and don't use the default always-on behaviour).
-        self.extra_args = [['-whitelist=127.0.0.1', '-walletprematurewitness', '-vbparams=segwit:0:999999999999']]
+        self.extra_args = [['-whitelist=127.0.0.1', '-walletprematurewitness', '-vbparams=segwit:0:999999999999', '-addresstype=legacy']]
 
     def run_test(self):
         self.address = self.nodes[0].getnewaddress()
diff --git a/test/functional/p2p-fullblocktest.py b/test/functional/p2p-fullblocktest.py
index d08318a96b..fe9bbda14b 100755
--- a/test/functional/p2p-fullblocktest.py
+++ b/test/functional/p2p-fullblocktest.py
@@ -36,12 +36,15 @@ class CBrokenBlock(CBlock):
         self.vtx = copy.deepcopy(base_block.vtx)
         self.hashMerkleRoot = self.calc_merkle_root()
 
-    def serialize(self):
+    def serialize(self, with_witness=False):
         r = b""
         r += super(CBlock, self).serialize()
         r += struct.pack("<BQ", 255, len(self.vtx))
         for tx in self.vtx:
-            r += tx.serialize()
+            if with_witness:
+                r += tx.serialize_with_witness()
+            else:
+                r += tx.serialize_without_witness()
         return r
 
     def normal_serialize(self):
diff --git a/test/functional/p2p-segwit.py b/test/functional/p2p-segwit.py
index 2487fdc239..5f7cc05413 100755
--- a/test/functional/p2p-segwit.py
+++ b/test/functional/p2p-segwit.py
@@ -25,7 +25,7 @@ MAX_SIGOP_COST = 80000
 # Calculate the virtual size of a witness block:
 # (base + witness/4)
 def get_virtual_size(witness_block):
-    base_size = len(witness_block.serialize())
+    base_size = len(witness_block.serialize(with_witness=False))
     total_size = len(witness_block.serialize(with_witness=True))
     # the "+3" is so we round up
     vsize = int((3*base_size + total_size + 3)/4)
diff --git a/test/functional/rawtransactions.py b/test/functional/rawtransactions.py
index 073ed394c1..fe749adb49 100755
--- a/test/functional/rawtransactions.py
+++ b/test/functional/rawtransactions.py
@@ -39,6 +39,7 @@ class RawTransactionsTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 3
+        self.extra_args = [["-addresstype=legacy"], ["-addresstype=legacy"], ["-addresstype=legacy"]]
 
     def setup_network(self, split=False):
         super().setup_network()
@@ -189,7 +190,7 @@ class RawTransactionsTest(BitcoinTestFramework):
                 break
 
         bal = self.nodes[0].getbalance()
-        inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex']}]
+        inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex'], "amount" : vout['value']}]
         outputs = { self.nodes[0].getnewaddress() : 2.19 }
         rawTx = self.nodes[2].createrawtransaction(inputs, outputs)
         rawTxPartialSigned = self.nodes[1].signrawtransaction(rawTx, inputs)
@@ -234,7 +235,7 @@ class RawTransactionsTest(BitcoinTestFramework):
                 break
 
         bal = self.nodes[0].getbalance()
-        inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex'], "redeemScript" : mSigObjValid['hex']}]
+        inputs = [{ "txid" : txId, "vout" : vout['n'], "scriptPubKey" : vout['scriptPubKey']['hex'], "redeemScript" : mSigObjValid['hex'], "amount" : vout['value']}]
         outputs = { self.nodes[0].getnewaddress() : 2.19 }
         rawTx2 = self.nodes[2].createrawtransaction(inputs, outputs)
         rawTxPartialSigned1 = self.nodes[1].signrawtransaction(rawTx2, inputs)
diff --git a/test/functional/segwit.py b/test/functional/segwit.py
index dbf4a86e38..5a4a7468e5 100755
--- a/test/functional/segwit.py
+++ b/test/functional/segwit.py
@@ -78,9 +78,9 @@ class SegWitTest(BitcoinTestFramework):
         self.setup_clean_chain = True
         self.num_nodes = 3
         # This test tests SegWit both pre and post-activation, so use the normal BIP9 activation.
-        self.extra_args = [["-walletprematurewitness", "-rpcserialversion=0", "-vbparams=segwit:0:999999999999"],
-                           ["-blockversion=4", "-promiscuousmempoolflags=517", "-prematurewitness", "-walletprematurewitness", "-rpcserialversion=1", "-vbparams=segwit:0:999999999999"],
-                           ["-blockversion=536870915", "-promiscuousmempoolflags=517", "-prematurewitness", "-walletprematurewitness", "-vbparams=segwit:0:999999999999"]]
+        self.extra_args = [["-walletprematurewitness", "-rpcserialversion=0", "-vbparams=segwit:0:999999999999", "-addresstype=legacy"],
+                           ["-blockversion=4", "-promiscuousmempoolflags=517", "-prematurewitness", "-walletprematurewitness", "-rpcserialversion=1", "-vbparams=segwit:0:999999999999", "-addresstype=legacy"],
+                           ["-blockversion=536870915", "-promiscuousmempoolflags=517", "-prematurewitness", "-walletprematurewitness", "-vbparams=segwit:0:999999999999", "-addresstype=legacy"]]
 
     def setup_network(self):
         super().setup_network()
@@ -135,9 +135,9 @@ class SegWitTest(BitcoinTestFramework):
             self.pubkey.append(self.nodes[i].validateaddress(newaddress)["pubkey"])
             multiaddress = self.nodes[i].addmultisigaddress(1, [self.pubkey[-1]])
             multiscript = CScript([OP_1, hex_str_to_bytes(self.pubkey[-1]), OP_1, OP_CHECKMULTISIG])
-            p2sh_addr = self.nodes[i].addwitnessaddress(newaddress, True)
+            p2sh_addr = self.nodes[i].addwitnessaddress(newaddress)
             bip173_addr = self.nodes[i].addwitnessaddress(newaddress, False)
-            p2sh_ms_addr = self.nodes[i].addwitnessaddress(multiaddress, True)
+            p2sh_ms_addr = self.nodes[i].addwitnessaddress(multiaddress)
             bip173_ms_addr = self.nodes[i].addwitnessaddress(multiaddress, False)
             assert_equal(p2sh_addr, key_to_p2sh_p2wpkh(self.pubkey[-1]))
             assert_equal(bip173_addr, key_to_p2wpkh(self.pubkey[-1]))
@@ -356,8 +356,10 @@ class SegWitTest(BitcoinTestFramework):
                 [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh] = self.p2pkh_address_to_script(v)
                 # normal P2PKH and P2PK with compressed keys should always be spendable
                 spendable_anytime.extend([p2pkh, p2pk])
-                # P2SH_P2PK, P2SH_P2PKH, and witness with compressed keys are spendable after direct importaddress
-                spendable_after_importaddress.extend([p2wpkh, p2sh_p2wpkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
+                # P2SH_P2PK, P2SH_P2PKH with compressed keys are spendable after direct importaddress
+                spendable_after_importaddress.extend([p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
+                # P2WPKH and P2SH_P2WPKH with compressed keys should always be spendable
+                spendable_anytime.extend([p2wpkh, p2sh_p2wpkh])
 
         for i in uncompressed_spendable_address:
             v = self.nodes[0].validateaddress(i)
@@ -373,7 +375,7 @@ class SegWitTest(BitcoinTestFramework):
                 spendable_anytime.extend([p2pkh, p2pk])
                 # P2SH_P2PK and P2SH_P2PKH are spendable after direct importaddress
                 spendable_after_importaddress.extend([p2sh_p2pk, p2sh_p2pkh])
-                # witness with uncompressed keys are never seen
+                # Witness output types with uncompressed keys are never seen
                 unseen_anytime.extend([p2wpkh, p2sh_p2wpkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
 
         for i in compressed_solvable_address:
@@ -384,10 +386,10 @@ class SegWitTest(BitcoinTestFramework):
                 solvable_after_importaddress.extend([bare, p2sh, p2wsh, p2sh_p2wsh])
             else:
                 [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh] = self.p2pkh_address_to_script(v)
-                # normal P2PKH and P2PK with compressed keys should always be seen
-                solvable_anytime.extend([p2pkh, p2pk])
-                # P2SH_P2PK, P2SH_P2PKH, and witness with compressed keys are seen after direct importaddress
-                solvable_after_importaddress.extend([p2wpkh, p2sh_p2wpkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
+                # normal P2PKH, P2PK, P2WPKH and P2SH_P2WPKH with compressed keys should always be seen
+                solvable_anytime.extend([p2pkh, p2pk, p2wpkh, p2sh_p2wpkh])
+                # P2SH_P2PK, P2SH_P2PKH with compressed keys are seen after direct importaddress
+                solvable_after_importaddress.extend([p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
 
         for i in uncompressed_solvable_address:
             v = self.nodes[0].validateaddress(i)
@@ -403,7 +405,7 @@ class SegWitTest(BitcoinTestFramework):
                 solvable_anytime.extend([p2pkh, p2pk])
                 # P2SH_P2PK, P2SH_P2PKH with uncompressed keys are seen after direct importaddress
                 solvable_after_importaddress.extend([p2sh_p2pk, p2sh_p2pkh])
-                # witness with uncompressed keys are never seen
+                # Witness output types with uncompressed keys are never seen
                 unseen_anytime.extend([p2wpkh, p2sh_p2wpkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh])
 
         op1 = CScript([OP_1])
@@ -496,6 +498,8 @@ class SegWitTest(BitcoinTestFramework):
         spendable_after_addwitnessaddress = []      # These outputs should be seen after importaddress
         solvable_after_addwitnessaddress=[]         # These outputs should be seen after importaddress but not spendable
         unseen_anytime = []                         # These outputs should never be seen
+        solvable_anytime = []                       # These outputs should be solvable after importpubkey
+        unseen_anytime = []                         # These outputs should never be seen
 
         uncompressed_spendable_address.append(self.nodes[0].addmultisigaddress(2, [uncompressed_spendable_address[0], compressed_spendable_address[0]]))
         uncompressed_spendable_address.append(self.nodes[0].addmultisigaddress(2, [uncompressed_spendable_address[0], uncompressed_spendable_address[0]]))
@@ -514,9 +518,8 @@ class SegWitTest(BitcoinTestFramework):
                 premature_witaddress.append(script_to_p2sh(p2wsh))
             else:
                 [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh] = self.p2pkh_address_to_script(v)
-                # P2WPKH, P2SH_P2WPKH are spendable after addwitnessaddress
-                spendable_after_addwitnessaddress.extend([p2wpkh, p2sh_p2wpkh])
-                premature_witaddress.append(script_to_p2sh(p2wpkh))
+                # P2WPKH, P2SH_P2WPKH are always spendable
+                spendable_anytime.extend([p2wpkh, p2sh_p2wpkh])
 
         for i in uncompressed_spendable_address + uncompressed_solvable_address:
             v = self.nodes[0].validateaddress(i)
@@ -538,10 +541,11 @@ class SegWitTest(BitcoinTestFramework):
                 premature_witaddress.append(script_to_p2sh(p2wsh))
             else:
                 [p2wpkh, p2sh_p2wpkh, p2pk, p2pkh, p2sh_p2pk, p2sh_p2pkh, p2wsh_p2pk, p2wsh_p2pkh, p2sh_p2wsh_p2pk, p2sh_p2wsh_p2pkh] = self.p2pkh_address_to_script(v)
-                # P2SH_P2PK, P2SH_P2PKH with compressed keys are seen after addwitnessaddress
-                solvable_after_addwitnessaddress.extend([p2wpkh, p2sh_p2wpkh])
-                premature_witaddress.append(script_to_p2sh(p2wpkh))
+                # P2SH_P2PK, P2SH_P2PKH with compressed keys are always solvable
+                solvable_anytime.extend([p2wpkh, p2sh_p2wpkh])
 
+        self.mine_and_test_listunspent(spendable_anytime, 2)
+        self.mine_and_test_listunspent(solvable_anytime, 1)
         self.mine_and_test_listunspent(spendable_after_addwitnessaddress + solvable_after_addwitnessaddress + unseen_anytime, 0)
 
         # addwitnessaddress should refuse to return a witness address if an uncompressed key is used
@@ -558,8 +562,8 @@ class SegWitTest(BitcoinTestFramework):
             witaddress = self.nodes[0].addwitnessaddress(i)
             assert_equal(witaddress, self.nodes[0].addwitnessaddress(witaddress))
 
-        spendable_txid.append(self.mine_and_test_listunspent(spendable_after_addwitnessaddress, 2))
-        solvable_txid.append(self.mine_and_test_listunspent(solvable_after_addwitnessaddress, 1))
+        spendable_txid.append(self.mine_and_test_listunspent(spendable_after_addwitnessaddress + spendable_anytime, 2))
+        solvable_txid.append(self.mine_and_test_listunspent(solvable_after_addwitnessaddress + solvable_anytime, 1))
         self.mine_and_test_listunspent(unseen_anytime, 0)
 
         # Check that createrawtransaction/decoderawtransaction with non-v0 Bech32 works
diff --git a/test/functional/signmessages.py b/test/functional/signmessages.py
index c1939b432b..5b6935ceea 100755
--- a/test/functional/signmessages.py
+++ b/test/functional/signmessages.py
@@ -11,6 +11,7 @@ class SignMessagesTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
+        self.extra_args = [["-addresstype=legacy"]]
 
     def run_test(self):
         message = 'This is just a test message'
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index d8032e4430..a54a0299c7 100644
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -453,10 +453,10 @@ class CTransaction():
         r += struct.pack("<I", self.nLockTime)
         return r
 
-    # Regular serialization is without witness -- must explicitly
-    # call serialize_with_witness to include witness data.
+    # Regular serialization is with witness -- must explicitly
+    # call serialize_without_witness to exclude witness data.
     def serialize(self):
-        return self.serialize_without_witness()
+        return self.serialize_with_witness()
 
     # Recalculate the txid (transaction hash without witness)
     def rehash(self):
@@ -472,7 +472,7 @@ class CTransaction():
 
         if self.sha256 is None:
             self.sha256 = uint256_from_str(hash256(self.serialize_without_witness()))
-        self.hash = encode(hash256(self.serialize())[::-1], 'hex_codec').decode('ascii')
+        self.hash = encode(hash256(self.serialize_without_witness())[::-1], 'hex_codec').decode('ascii')
 
     def is_valid(self):
         self.calc_sha256()
@@ -569,7 +569,7 @@ class CBlock(CBlockHeader):
         if with_witness:
             r += ser_vector(self.vtx, "serialize_with_witness")
         else:
-            r += ser_vector(self.vtx)
+            r += ser_vector(self.vtx, "serialize_without_witness")
         return r
 
     # Calculate the merkle root given a vector of transaction hashes
@@ -636,7 +636,7 @@ class PrefilledTransaction():
         self.tx = CTransaction()
         self.tx.deserialize(f)
 
-    def serialize(self, with_witness=False):
+    def serialize(self, with_witness=True):
         r = b""
         r += ser_compact_size(self.index)
         if with_witness:
@@ -645,6 +645,9 @@ class PrefilledTransaction():
             r += self.tx.serialize_without_witness()
         return r
 
+    def serialize_without_witness(self):
+        return self.serialize(with_witness=False)
+
     def serialize_with_witness(self):
         return self.serialize(with_witness=True)
 
@@ -684,7 +687,7 @@ class P2PHeaderAndShortIDs():
         if with_witness:
             r += ser_vector(self.prefilled_txn, "serialize_with_witness")
         else:
-            r += ser_vector(self.prefilled_txn)
+            r += ser_vector(self.prefilled_txn, "serialize_without_witness")
         return r
 
     def __repr__(self):
@@ -815,13 +818,13 @@ class BlockTransactions():
         self.blockhash = deser_uint256(f)
         self.transactions = deser_vector(f, CTransaction)
 
-    def serialize(self, with_witness=False):
+    def serialize(self, with_witness=True):
         r = b""
         r += ser_uint256(self.blockhash)
         if with_witness:
             r += ser_vector(self.transactions, "serialize_with_witness")
         else:
-            r += ser_vector(self.transactions)
+            r += ser_vector(self.transactions, "serialize_without_witness")
         return r
 
     def __repr__(self):
@@ -1021,7 +1024,7 @@ class msg_block():
         self.block.deserialize(f)
 
     def serialize(self):
-        return self.block.serialize()
+        return self.block.serialize(with_witness=False)
 
     def __repr__(self):
         return "msg_block(block=%s)" % (repr(self.block))
@@ -1292,7 +1295,7 @@ class msg_blocktxn():
 
     def serialize(self):
         r = b""
-        r += self.block_transactions.serialize()
+        r += self.block_transactions.serialize(with_witness=False)
         return r
 
     def __repr__(self):
diff --git a/test/functional/test_framework/script.py b/test/functional/test_framework/script.py
index 8429f34c41..dae8a4e569 100644
--- a/test/functional/test_framework/script.py
+++ b/test/functional/test_framework/script.py
@@ -641,7 +641,7 @@ def SignatureHash(script, txTo, inIdx, hashtype):
         txtmp.vin = []
         txtmp.vin.append(tmp)
 
-    s = txtmp.serialize()
+    s = txtmp.serialize_without_witness()
     s += struct.pack(b"<I", hashtype)
 
     hash = hash256(s)
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 428a18be86..2d5ea84814 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -78,6 +78,7 @@ BASE_SCRIPTS= [
     'abandonconflict.py',
     'bip68-112-113-p2p.py',
     'rawtransactions.py',
+    'address_types.py',
     'reindex.py',
     # vv Tests less than 30s vv
     'keypool-topup.py',
@@ -86,6 +87,7 @@ BASE_SCRIPTS= [
     'mempool_resurrect_test.py',
     'txn_doublespend.py --mineblock',
     'txn_clone.py',
+    'txn_clone.py --segwit',
     'getchaintips.py',
     'rest.py',
     'mempool_spendcoinbase.py',
diff --git a/test/functional/txn_clone.py b/test/functional/txn_clone.py
index c50f86341f..ce26d6e0ee 100755
--- a/test/functional/txn_clone.py
+++ b/test/functional/txn_clone.py
@@ -14,6 +14,8 @@ class TxnMallTest(BitcoinTestFramework):
     def add_options(self, parser):
         parser.add_option("--mineblock", dest="mine_block", default=False, action="store_true",
                           help="Test double-spend of 1-confirmed transaction")
+        parser.add_option("--segwit", dest="segwit", default=False, action="store_true",
+                          help="Test behaviour with SegWit txn (which should fail")
 
     def setup_network(self):
         # Start with split network:
@@ -22,6 +24,11 @@ class TxnMallTest(BitcoinTestFramework):
         disconnect_nodes(self.nodes[2], 1)
 
     def run_test(self):
+        if self.options.segwit:
+            output_type="p2sh-segwit"
+        else:
+            output_type="legacy"
+
         # All nodes should start with 1,250 BTC:
         starting_balance = 1250
         for i in range(4):
@@ -31,11 +38,11 @@ class TxnMallTest(BitcoinTestFramework):
         # Assign coins to foo and bar accounts:
         self.nodes[0].settxfee(.001)
 
-        node0_address_foo = self.nodes[0].getnewaddress("foo")
+        node0_address_foo = self.nodes[0].getnewaddress("foo", output_type)
         fund_foo_txid = self.nodes[0].sendfrom("", node0_address_foo, 1219)
         fund_foo_tx = self.nodes[0].gettransaction(fund_foo_txid)
 
-        node0_address_bar = self.nodes[0].getnewaddress("bar")
+        node0_address_bar = self.nodes[0].getnewaddress("bar", output_type)
         fund_bar_txid = self.nodes[0].sendfrom("", node0_address_bar, 29)
         fund_bar_tx = self.nodes[0].gettransaction(fund_bar_txid)
 
@@ -106,6 +113,10 @@ class TxnMallTest(BitcoinTestFramework):
         # Send clone and its parent to miner
         self.nodes[2].sendrawtransaction(fund_foo_tx["hex"])
         txid1_clone = self.nodes[2].sendrawtransaction(tx1_clone["hex"])
+        if self.options.segwit:
+            assert_equal(txid1, txid1_clone)
+            return
+
         # ... mine a block...
         self.nodes[2].generate(1)
 
diff --git a/test/functional/wallet-dump.py b/test/functional/wallet-dump.py
index fa51a2e9d7..8b81c81ba8 100755
--- a/test/functional/wallet-dump.py
+++ b/test/functional/wallet-dump.py
@@ -69,7 +69,7 @@ def read_dump(file_name, addrs, script_addrs, hd_master_addr_old):
 class WalletDumpTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
-        self.extra_args = [["-keypool=90"]]
+        self.extra_args = [["-keypool=90", "-addresstype=legacy"]]
 
     def setup_network(self, split=False):
         # Use 1 minute timeout because the initial getnewaddress RPC can take
diff --git a/test/functional/wallet.py b/test/functional/wallet.py
index 8cd059c618..a90dbc8adf 100755
--- a/test/functional/wallet.py
+++ b/test/functional/wallet.py
@@ -27,6 +27,9 @@ class WalletTest(BitcoinTestFramework):
         assert_fee_amount(fee, tx_size, fee_per_byte * 1000)
         return curr_balance
 
+    def get_vsize(self, txn):
+        return self.nodes[0].decoderawtransaction(txn)['vsize']
+
     def run_test(self):
         # Check that there's no UTXO on none of the nodes
         assert_equal(len(self.nodes[0].listunspent()), 0)
@@ -162,7 +165,7 @@ class WalletTest(BitcoinTestFramework):
         txid = self.nodes[2].sendtoaddress(address, 10, "", "", False)
         self.nodes[2].generate(1)
         self.sync_all([self.nodes[0:3]])
-        node_2_bal = self.check_fee_amount(self.nodes[2].getbalance(), Decimal('84'), fee_per_byte, count_bytes(self.nodes[2].getrawtransaction(txid)))
+        node_2_bal = self.check_fee_amount(self.nodes[2].getbalance(), Decimal('84'), fee_per_byte, self.get_vsize(self.nodes[2].getrawtransaction(txid)))
         assert_equal(self.nodes[0].getbalance(), Decimal('10'))
 
         # Send 10 BTC with subtract fee from amount
@@ -171,14 +174,14 @@ class WalletTest(BitcoinTestFramework):
         self.sync_all([self.nodes[0:3]])
         node_2_bal -= Decimal('10')
         assert_equal(self.nodes[2].getbalance(), node_2_bal)
-        node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), Decimal('20'), fee_per_byte, count_bytes(self.nodes[2].getrawtransaction(txid)))
+        node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), Decimal('20'), fee_per_byte, self.get_vsize(self.nodes[2].getrawtransaction(txid)))
 
         # Sendmany 10 BTC
         txid = self.nodes[2].sendmany('from1', {address: 10}, 0, "", [])
         self.nodes[2].generate(1)
         self.sync_all([self.nodes[0:3]])
         node_0_bal += Decimal('10')
-        node_2_bal = self.check_fee_amount(self.nodes[2].getbalance(), node_2_bal - Decimal('10'), fee_per_byte, count_bytes(self.nodes[2].getrawtransaction(txid)))
+        node_2_bal = self.check_fee_amount(self.nodes[2].getbalance(), node_2_bal - Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].getrawtransaction(txid)))
         assert_equal(self.nodes[0].getbalance(), node_0_bal)
 
         # Sendmany 10 BTC with subtract fee from amount
@@ -187,7 +190,7 @@ class WalletTest(BitcoinTestFramework):
         self.sync_all([self.nodes[0:3]])
         node_2_bal -= Decimal('10')
         assert_equal(self.nodes[2].getbalance(), node_2_bal)
-        node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, count_bytes(self.nodes[2].getrawtransaction(txid)))
+        node_0_bal = self.check_fee_amount(self.nodes[0].getbalance(), node_0_bal + Decimal('10'), fee_per_byte, self.get_vsize(self.nodes[2].getrawtransaction(txid)))
 
         # Test ResendWalletTransactions:
         # Create a couple of transactions, then start up a fourth