From d8d8b7bc6698d0657521754e0dc4ee3f303d8e6d Mon Sep 17 00:00:00 2001
From: Suhas Daftuar <sdaftuar@gmail.com>
Date: Sat, 23 Mar 2019 14:55:51 -0400
Subject: [PATCH 01/22] [qa] Overhaul p2p_compactblocks.py

Remove tests of:
 - compactblock behavior in a simulated pre-segwit version of bitcoind
   This should have been removed a long time ago, as it is not generally
   necessary for us to test the behavior of old nodes (except perhaps if we
   want to test that upgrading from an old node to a new one behaves properly)

 - compactblock behavior during segwit upgrade (ie verifying that network
   behavior before and after activation was as expected)
   This is unnecessary to test now that segwit activation has already happened.
---
 test/functional/p2p_compactblocks.py | 163 +++++++--------------------
 1 file changed, 43 insertions(+), 120 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 82a14af9da8c..ae71513d549b 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -15,7 +15,7 @@
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.script import CScript, OP_TRUE, OP_DROP
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, get_bip9_status, satoshi_round, sync_blocks, wait_until
+from test_framework.util import assert_equal, get_bip9_status, satoshi_round, wait_until
 
 # TestP2PConn: A peer we use to send messages to bitcoind, and store responses.
 class TestP2PConn(P2PInterface):
@@ -94,11 +94,9 @@ def send_await_disconnect(self, message, timeout=30):
 class CompactBlocksTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
-        # Node0 = pre-segwit, node1 = segwit-aware
-        self.num_nodes = 2
+        self.num_nodes = 1
         # This test was written assuming SegWit is activated using BIP9 at height 432 (3x confirmation window).
         # TODO: Rewrite this test to support SegWit being always active.
-        self.extra_args = [["-vbparams=segwit:0:0"], ["-vbparams=segwit:0:999999999999", "-txindex"]]
         self.utxos = []
 
     def skip_test_if_missing_module(self):
@@ -117,9 +115,8 @@ def build_block_on_tip(self, node, segwit=False):
 
     # Create 10 more anyone-can-spend utxo's for testing.
     def make_utxos(self):
-        # Doesn't matter which node we use, just use node0.
         block = self.build_block_on_tip(self.nodes[0])
-        self.test_node.send_and_ping(msg_block(block))
+        self.segwit_node.send_and_ping(msg_block(block))
         assert int(self.nodes[0].getbestblockhash(), 16) == block.sha256
         self.nodes[0].generate(100)
 
@@ -135,11 +132,18 @@ def make_utxos(self):
         block2.vtx.append(tx)
         block2.hashMerkleRoot = block2.calc_merkle_root()
         block2.solve()
-        self.test_node.send_and_ping(msg_block(block2))
+        self.segwit_node.send_and_ping(msg_block(block2))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
         self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
         return
 
+    # Send balance to a segwit output
+    def make_segwit_output(self, node):
+        if node.getbalance() < 2:
+            node.generate(101)
+        address = node.getnewaddress("bech32")
+        node.sendtoaddress(address, node.getbalance()-1)
+
     # Test "sendcmpct" (between peers preferring the same version):
     # - No compact block announcements unless sendcmpct is sent.
     # - If sendcmpct is sent with version > preferred_version, the message is ignored.
@@ -251,23 +255,16 @@ def test_invalid_cmpctblock_message(self):
         # This index will be too high
         prefilled_txn = PrefilledTransaction(1, block.vtx[0])
         cmpct_block.prefilled_txn = [prefilled_txn]
-        self.test_node.send_await_disconnect(msg_cmpctblock(cmpct_block))
+        self.segwit_node.send_await_disconnect(msg_cmpctblock(cmpct_block))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
 
     # Compare the generated shortids to what we expect based on BIP 152, given
     # bitcoind's choice of nonce.
-    def test_compactblock_construction(self, node, test_node, version, use_witness_address):
+    def test_compactblock_construction(self, node, test_node, version, use_witness_address=True):
         # Generate a bunch of transactions.
         node.generate(101)
         num_transactions = 25
         address = node.getnewaddress()
-        if use_witness_address:
-            # Want at least one segwit spend, so move all funds to
-            # a witness address.
-            address = node.getnewaddress(address_type='bech32')
-            value_to_send = node.getbalance()
-            node.sendtoaddress(address, satoshi_round(value_to_send - Decimal(0.1)))
-            node.generate(1)
 
         segwit_tx_generated = False
         for i in range(num_transactions):
@@ -375,7 +372,7 @@ def check_compactblock_construction_from_block(self, version, header_and_shortid
     # Post-segwit: upgraded nodes would only make this request of cb-version-2,
     # NODE_WITNESS peers.  Unupgraded nodes would still make this request of
     # any cb-version-1-supporting peer.
-    def test_compactblock_requests(self, node, test_node, version, segwit):
+    def test_compactblock_requests(self, node, test_node, version, segwit=True):
         # Try announcing a block with an inv or header, expect a compactblock
         # request
         for announce in ["inv", "header"]:
@@ -681,10 +678,6 @@ def test_compactblocks_not_at_tip(self, node, test_node):
         with mininode_lock:
             assert "blocktxn" not in test_node.last_message
 
-    def activate_segwit(self, node):
-        node.generate(144 * 3)
-        assert_equal(get_bip9_status(node, "segwit")["status"], 'active')
-
     def test_end_to_end_block_relay(self, node, listeners):
         utxo = self.utxos.pop(0)
 
@@ -706,7 +699,7 @@ def test_end_to_end_block_relay(self, node, listeners):
 
     # Test that we don't get disconnected if we relay a compact block with valid header,
     # but invalid transactions.
-    def test_invalid_tx_in_compactblock(self, node, test_node, use_segwit):
+    def test_invalid_tx_in_compactblock(self, node, test_node, use_segwit=True):
         assert len(self.utxos)
         utxo = self.utxos[0]
 
@@ -793,126 +786,56 @@ def announce_cmpct_block(node, peer):
 
     def run_test(self):
         # Setup the p2p connections
-        self.test_node = self.nodes[0].add_p2p_connection(TestP2PConn())
-        self.segwit_node = self.nodes[1].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
-        self.old_node = self.nodes[1].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
+        self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+        self.old_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
+        self.additional_segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
 
         # We will need UTXOs to construct transactions in later tests.
         self.make_utxos()
+        self.make_segwit_output(self.nodes[0])
 
-        self.log.info("Running tests, pre-segwit activation:")
+        assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
 
         self.log.info("Testing SENDCMPCT p2p message... ")
-        self.test_sendcmpct(self.nodes[0], self.test_node, 1)
-        sync_blocks(self.nodes)
-        self.test_sendcmpct(self.nodes[1], self.segwit_node, 2, old_node=self.old_node)
-        sync_blocks(self.nodes)
+        self.test_sendcmpct(self.nodes[0], self.segwit_node, 2, old_node=self.old_node)
+        self.test_sendcmpct(self.nodes[0], self.additional_segwit_node, 2)
 
         self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.nodes[0], self.test_node, 1, False)
-        sync_blocks(self.nodes)
-        self.test_compactblock_construction(self.nodes[1], self.segwit_node, 2, False)
-        sync_blocks(self.nodes)
-
-        self.log.info("Testing compactblock requests... ")
-        self.test_compactblock_requests(self.nodes[0], self.test_node, 1, False)
-        sync_blocks(self.nodes)
-        self.test_compactblock_requests(self.nodes[1], self.segwit_node, 2, False)
-        sync_blocks(self.nodes)
-
-        self.log.info("Testing getblocktxn requests...")
-        self.test_getblocktxn_requests(self.nodes[0], self.test_node, 1)
-        sync_blocks(self.nodes)
-        self.test_getblocktxn_requests(self.nodes[1], self.segwit_node, 2)
-        sync_blocks(self.nodes)
-
-        self.log.info("Testing getblocktxn handler...")
-        self.test_getblocktxn_handler(self.nodes[0], self.test_node, 1)
-        sync_blocks(self.nodes)
-        self.test_getblocktxn_handler(self.nodes[1], self.segwit_node, 2)
-        self.test_getblocktxn_handler(self.nodes[1], self.old_node, 1)
-        sync_blocks(self.nodes)
-
-        self.log.info("Testing compactblock requests/announcements not at chain tip...")
-        self.test_compactblocks_not_at_tip(self.nodes[0], self.test_node)
-        sync_blocks(self.nodes)
-        self.test_compactblocks_not_at_tip(self.nodes[1], self.segwit_node)
-        self.test_compactblocks_not_at_tip(self.nodes[1], self.old_node)
-        sync_blocks(self.nodes)
-
-        self.log.info("Testing handling of incorrect blocktxn responses...")
-        self.test_incorrect_blocktxn_response(self.nodes[0], self.test_node, 1)
-        sync_blocks(self.nodes)
-        self.test_incorrect_blocktxn_response(self.nodes[1], self.segwit_node, 2)
-        sync_blocks(self.nodes)
-
-        # End-to-end block relay tests
-        self.log.info("Testing end-to-end block relay...")
-        self.request_cb_announcements(self.test_node, self.nodes[0], 1)
-        self.request_cb_announcements(self.old_node, self.nodes[1], 1)
-        self.request_cb_announcements(self.segwit_node, self.nodes[1], 2)
-        self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.test_node, self.old_node])
-        self.test_end_to_end_block_relay(self.nodes[1], [self.segwit_node, self.test_node, self.old_node])
-
-        self.log.info("Testing handling of invalid compact blocks...")
-        self.test_invalid_tx_in_compactblock(self.nodes[0], self.test_node, False)
-        self.test_invalid_tx_in_compactblock(self.nodes[1], self.segwit_node, False)
-        self.test_invalid_tx_in_compactblock(self.nodes[1], self.old_node, False)
-
-        self.log.info("Testing reconstructing compact blocks from all peers...")
-        self.test_compactblock_reconstruction_multiple_peers(self.nodes[1], self.segwit_node, self.old_node)
-        sync_blocks(self.nodes)
-
-        # Advance to segwit activation
-        self.log.info("Advancing to segwit activation")
-        self.activate_segwit(self.nodes[1])
-        self.log.info("Running tests, post-segwit activation...")
-
-        self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.nodes[1], self.old_node, 1, True)
-        self.test_compactblock_construction(self.nodes[1], self.segwit_node, 2, True)
-        sync_blocks(self.nodes)
-
-        self.log.info("Testing compactblock requests (unupgraded node)... ")
-        self.test_compactblock_requests(self.nodes[0], self.test_node, 1, True)
-
-        self.log.info("Testing getblocktxn requests (unupgraded node)...")
-        self.test_getblocktxn_requests(self.nodes[0], self.test_node, 1)
-
-        # Need to manually sync node0 and node1, because post-segwit activation,
-        # node1 will not download blocks from node0.
-        self.log.info("Syncing nodes...")
-        assert self.nodes[0].getbestblockhash() != self.nodes[1].getbestblockhash()
-        while (self.nodes[0].getblockcount() > self.nodes[1].getblockcount()):
-            block_hash = self.nodes[0].getblockhash(self.nodes[1].getblockcount() + 1)
-            self.nodes[1].submitblock(self.nodes[0].getblock(block_hash, False))
-        assert_equal(self.nodes[0].getbestblockhash(), self.nodes[1].getbestblockhash())
+        self.test_compactblock_construction(self.nodes[0], self.old_node, 1)
+        self.test_compactblock_construction(self.nodes[0], self.segwit_node, 2)
 
         self.log.info("Testing compactblock requests (segwit node)... ")
-        self.test_compactblock_requests(self.nodes[1], self.segwit_node, 2, True)
+        self.test_compactblock_requests(self.nodes[0], self.segwit_node, 2)
 
         self.log.info("Testing getblocktxn requests (segwit node)...")
-        self.test_getblocktxn_requests(self.nodes[1], self.segwit_node, 2)
-        sync_blocks(self.nodes)
+        self.test_getblocktxn_requests(self.nodes[0], self.segwit_node, 2)
 
         self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
-        self.test_getblocktxn_handler(self.nodes[1], self.segwit_node, 2)
-        self.test_getblocktxn_handler(self.nodes[1], self.old_node, 1)
+        self.test_getblocktxn_handler(self.nodes[0], self.segwit_node, 2)
+        self.test_getblocktxn_handler(self.nodes[0], self.old_node, 1)
+
+        self.log.info("Testing compactblock requests/announcements not at chain tip...")
+        self.test_compactblocks_not_at_tip(self.nodes[0], self.segwit_node)
+        self.test_compactblocks_not_at_tip(self.nodes[0], self.old_node)
+
+        self.log.info("Testing handling of incorrect blocktxn responses...")
+        self.test_incorrect_blocktxn_response(self.nodes[0], self.segwit_node, 2)
+
+        self.log.info("Testing reconstructing compact blocks from all peers...")
+        self.test_compactblock_reconstruction_multiple_peers(self.nodes[0], self.segwit_node, self.additional_segwit_node)
 
         # Test that if we submitblock to node1, we'll get a compact block
         # announcement to all peers.
         # (Post-segwit activation, blocks won't propagate from node0 to node1
         # automatically, so don't bother testing a block announced to node0.)
         self.log.info("Testing end-to-end block relay...")
-        self.request_cb_announcements(self.test_node, self.nodes[0], 1)
-        self.request_cb_announcements(self.old_node, self.nodes[1], 1)
-        self.request_cb_announcements(self.segwit_node, self.nodes[1], 2)
-        self.test_end_to_end_block_relay(self.nodes[1], [self.segwit_node, self.test_node, self.old_node])
+        self.request_cb_announcements(self.old_node, self.nodes[0], 1)
+        self.request_cb_announcements(self.segwit_node, self.nodes[0], 2)
+        self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.old_node])
 
         self.log.info("Testing handling of invalid compact blocks...")
-        self.test_invalid_tx_in_compactblock(self.nodes[0], self.test_node, False)
-        self.test_invalid_tx_in_compactblock(self.nodes[1], self.segwit_node, True)
-        self.test_invalid_tx_in_compactblock(self.nodes[1], self.old_node, True)
+        self.test_invalid_tx_in_compactblock(self.nodes[0], self.segwit_node)
+        self.test_invalid_tx_in_compactblock(self.nodes[0], self.old_node)
 
         self.log.info("Testing invalid index in cmpctblock message...")
         self.test_invalid_cmpctblock_message()

From 5bc755fdd166e84d8606a613c379705b362f0bf2 Mon Sep 17 00:00:00 2001
From: Suhas Daftuar <sdaftuar@gmail.com>
Date: Sun, 24 Mar 2019 18:25:28 -0400
Subject: [PATCH 02/22] fixup! [qa] Overhaul p2p_compactblocks.py

---
 test/functional/p2p_compactblocks.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index ae71513d549b..d748ff78079c 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -7,7 +7,6 @@
 Version 1 compact blocks are pre-segwit (txids)
 Version 2 compact blocks are post-segwit (wtxids)
 """
-from decimal import Decimal
 import random
 
 from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment
@@ -15,7 +14,7 @@
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.script import CScript, OP_TRUE, OP_DROP
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal, get_bip9_status, satoshi_round, wait_until
+from test_framework.util import assert_equal, get_bip9_status, wait_until
 
 # TestP2PConn: A peer we use to send messages to bitcoind, and store responses.
 class TestP2PConn(P2PInterface):

From 3973ef1a933cb89d4b5998bf68d890417d192816 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 09:58:13 -0400
Subject: [PATCH 03/22] nit fixups for 15660

---
 test/functional/p2p_compactblocks.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index d748ff78079c..f95632eb9a21 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -94,8 +94,6 @@ class CompactBlocksTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
-        # This test was written assuming SegWit is activated using BIP9 at height 432 (3x confirmation window).
-        # TODO: Rewrite this test to support SegWit being always active.
         self.utxos = []
 
     def skip_test_if_missing_module(self):
@@ -134,14 +132,13 @@ def make_utxos(self):
         self.segwit_node.send_and_ping(msg_block(block2))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
         self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
-        return
 
     # Send balance to a segwit output
     def make_segwit_output(self, node):
         if node.getbalance() < 2:
             node.generate(101)
         address = node.getnewaddress("bech32")
-        node.sendtoaddress(address, node.getbalance()-1)
+        node.sendtoaddress(address, node.getbalance() - 1)
 
     # Test "sendcmpct" (between peers preferring the same version):
     # - No compact block announcements unless sendcmpct is sent.

From 825bdd7acc54bcff963e5505d63d6d2e06fa6389 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 09:58:30 -0400
Subject: [PATCH 04/22] Remove unused call to self.make_utxos()

---
 test/functional/p2p_compactblocks.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index f95632eb9a21..39fafc8a9ee7 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -517,8 +517,6 @@ def test_tip_after_message(node, peer, msg, tip):
     # Incorrectly responding to a getblocktxn shouldn't cause the block to be
     # permanently failed.
     def test_incorrect_blocktxn_response(self, node, test_node, version):
-        if (len(self.utxos) == 0):
-            self.make_utxos()
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(node, utxo, 10)

From a5b4e45a2163e6d7bd5dab2d30d70dbe37a3bf4a Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 09:59:55 -0400
Subject: [PATCH 05/22] Move run_test to top of class

---
 test/functional/p2p_compactblocks.py | 113 +++++++++++++--------------
 1 file changed, 56 insertions(+), 57 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 39fafc8a9ee7..7713f80c3cdd 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -110,6 +110,62 @@ def build_block_on_tip(self, node, segwit=False):
         block.solve()
         return block
 
+    def run_test(self):
+        # Setup the p2p connections
+        self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+        self.old_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
+        self.additional_segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+
+        # We will need UTXOs to construct transactions in later tests.
+        self.make_utxos()
+        self.make_segwit_output(self.nodes[0])
+
+        assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
+
+        self.log.info("Testing SENDCMPCT p2p message... ")
+        self.test_sendcmpct(self.nodes[0], self.segwit_node, 2, old_node=self.old_node)
+        self.test_sendcmpct(self.nodes[0], self.additional_segwit_node, 2)
+
+        self.log.info("Testing compactblock construction...")
+        self.test_compactblock_construction(self.nodes[0], self.old_node, 1)
+        self.test_compactblock_construction(self.nodes[0], self.segwit_node, 2)
+
+        self.log.info("Testing compactblock requests (segwit node)... ")
+        self.test_compactblock_requests(self.nodes[0], self.segwit_node, 2)
+
+        self.log.info("Testing getblocktxn requests (segwit node)...")
+        self.test_getblocktxn_requests(self.nodes[0], self.segwit_node, 2)
+
+        self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
+        self.test_getblocktxn_handler(self.nodes[0], self.segwit_node, 2)
+        self.test_getblocktxn_handler(self.nodes[0], self.old_node, 1)
+
+        self.log.info("Testing compactblock requests/announcements not at chain tip...")
+        self.test_compactblocks_not_at_tip(self.nodes[0], self.segwit_node)
+        self.test_compactblocks_not_at_tip(self.nodes[0], self.old_node)
+
+        self.log.info("Testing handling of incorrect blocktxn responses...")
+        self.test_incorrect_blocktxn_response(self.nodes[0], self.segwit_node, 2)
+
+        self.log.info("Testing reconstructing compact blocks from all peers...")
+        self.test_compactblock_reconstruction_multiple_peers(self.nodes[0], self.segwit_node, self.additional_segwit_node)
+
+        # Test that if we submitblock to node1, we'll get a compact block
+        # announcement to all peers.
+        # (Post-segwit activation, blocks won't propagate from node0 to node1
+        # automatically, so don't bother testing a block announced to node0.)
+        self.log.info("Testing end-to-end block relay...")
+        self.request_cb_announcements(self.old_node, self.nodes[0], 1)
+        self.request_cb_announcements(self.segwit_node, self.nodes[0], 2)
+        self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.old_node])
+
+        self.log.info("Testing handling of invalid compact blocks...")
+        self.test_invalid_tx_in_compactblock(self.nodes[0], self.segwit_node)
+        self.test_invalid_tx_in_compactblock(self.nodes[0], self.old_node)
+
+        self.log.info("Testing invalid index in cmpctblock message...")
+        self.test_invalid_cmpctblock_message()
+
     # Create 10 more anyone-can-spend utxo's for testing.
     def make_utxos(self):
         block = self.build_block_on_tip(self.nodes[0])
@@ -778,62 +834,5 @@ def announce_cmpct_block(node, peer):
         stalling_peer.send_and_ping(msg)
         assert_equal(int(node.getbestblockhash(), 16), block.sha256)
 
-    def run_test(self):
-        # Setup the p2p connections
-        self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
-        self.old_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
-        self.additional_segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
-
-        # We will need UTXOs to construct transactions in later tests.
-        self.make_utxos()
-        self.make_segwit_output(self.nodes[0])
-
-        assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
-
-        self.log.info("Testing SENDCMPCT p2p message... ")
-        self.test_sendcmpct(self.nodes[0], self.segwit_node, 2, old_node=self.old_node)
-        self.test_sendcmpct(self.nodes[0], self.additional_segwit_node, 2)
-
-        self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.nodes[0], self.old_node, 1)
-        self.test_compactblock_construction(self.nodes[0], self.segwit_node, 2)
-
-        self.log.info("Testing compactblock requests (segwit node)... ")
-        self.test_compactblock_requests(self.nodes[0], self.segwit_node, 2)
-
-        self.log.info("Testing getblocktxn requests (segwit node)...")
-        self.test_getblocktxn_requests(self.nodes[0], self.segwit_node, 2)
-
-        self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
-        self.test_getblocktxn_handler(self.nodes[0], self.segwit_node, 2)
-        self.test_getblocktxn_handler(self.nodes[0], self.old_node, 1)
-
-        self.log.info("Testing compactblock requests/announcements not at chain tip...")
-        self.test_compactblocks_not_at_tip(self.nodes[0], self.segwit_node)
-        self.test_compactblocks_not_at_tip(self.nodes[0], self.old_node)
-
-        self.log.info("Testing handling of incorrect blocktxn responses...")
-        self.test_incorrect_blocktxn_response(self.nodes[0], self.segwit_node, 2)
-
-        self.log.info("Testing reconstructing compact blocks from all peers...")
-        self.test_compactblock_reconstruction_multiple_peers(self.nodes[0], self.segwit_node, self.additional_segwit_node)
-
-        # Test that if we submitblock to node1, we'll get a compact block
-        # announcement to all peers.
-        # (Post-segwit activation, blocks won't propagate from node0 to node1
-        # automatically, so don't bother testing a block announced to node0.)
-        self.log.info("Testing end-to-end block relay...")
-        self.request_cb_announcements(self.old_node, self.nodes[0], 1)
-        self.request_cb_announcements(self.segwit_node, self.nodes[0], 2)
-        self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.old_node])
-
-        self.log.info("Testing handling of invalid compact blocks...")
-        self.test_invalid_tx_in_compactblock(self.nodes[0], self.segwit_node)
-        self.test_invalid_tx_in_compactblock(self.nodes[0], self.old_node)
-
-        self.log.info("Testing invalid index in cmpctblock message...")
-        self.test_invalid_cmpctblock_message()
-
-
 if __name__ == '__main__':
     CompactBlocksTest().main()

From 393910330ff79360d7c31cbb1d254d51060f9fd3 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 10:16:16 -0400
Subject: [PATCH 06/22] combine make_utxos and make_segwit_output

---
 test/functional/p2p_compactblocks.py | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 7713f80c3cdd..9aace3aa20a7 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -116,9 +116,8 @@ def run_test(self):
         self.old_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
         self.additional_segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
 
-        # We will need UTXOs to construct transactions in later tests.
+        # Construct UTXOs for use in later tests.
         self.make_utxos()
-        self.make_segwit_output(self.nodes[0])
 
         assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
 
@@ -166,12 +165,13 @@ def run_test(self):
         self.log.info("Testing invalid index in cmpctblock message...")
         self.test_invalid_cmpctblock_message()
 
-    # Create 10 more anyone-can-spend utxo's for testing.
     def make_utxos(self):
+        """ Create 10 anyone-can-spend UTXOs for testing and send balance to bech32 output."""
         block = self.build_block_on_tip(self.nodes[0])
         self.segwit_node.send_and_ping(msg_block(block))
         assert int(self.nodes[0].getbestblockhash(), 16) == block.sha256
-        self.nodes[0].generate(100)
+        address = self.nodes[0].getnewaddress(address_type="bech32")
+        self.nodes[0].generatetoaddress(100, address)
 
         total_value = block.vtx[0].vout[0].nValue
         out_value = total_value // 10
@@ -189,13 +189,6 @@ def make_utxos(self):
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
         self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
 
-    # Send balance to a segwit output
-    def make_segwit_output(self, node):
-        if node.getbalance() < 2:
-            node.generate(101)
-        address = node.getnewaddress("bech32")
-        node.sendtoaddress(address, node.getbalance() - 1)
-
     # Test "sendcmpct" (between peers preferring the same version):
     # - No compact block announcements unless sendcmpct is sent.
     # - If sendcmpct is sent with version > preferred_version, the message is ignored.

From a1e7be86dcd3989c9bc6d03496bcfef86b2c8769 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 10:26:36 -0400
Subject: [PATCH 07/22] Remove unnecessary node argument from subtests

---
 test/functional/p2p_compactblocks.py | 170 +++++++++++++--------------
 1 file changed, 85 insertions(+), 85 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 9aace3aa20a7..62ee9801650f 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -122,45 +122,45 @@ def run_test(self):
         assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
 
         self.log.info("Testing SENDCMPCT p2p message... ")
-        self.test_sendcmpct(self.nodes[0], self.segwit_node, 2, old_node=self.old_node)
-        self.test_sendcmpct(self.nodes[0], self.additional_segwit_node, 2)
+        self.test_sendcmpct(self.segwit_node, 2, old_node=self.old_node)
+        self.test_sendcmpct(self.additional_segwit_node, 2)
 
         self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.nodes[0], self.old_node, 1)
-        self.test_compactblock_construction(self.nodes[0], self.segwit_node, 2)
+        self.test_compactblock_construction(self.old_node, 1)
+        self.test_compactblock_construction(self.segwit_node, 2)
 
         self.log.info("Testing compactblock requests (segwit node)... ")
-        self.test_compactblock_requests(self.nodes[0], self.segwit_node, 2)
+        self.test_compactblock_requests(self.segwit_node, 2)
 
         self.log.info("Testing getblocktxn requests (segwit node)...")
-        self.test_getblocktxn_requests(self.nodes[0], self.segwit_node, 2)
+        self.test_getblocktxn_requests(self.segwit_node, 2)
 
         self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
-        self.test_getblocktxn_handler(self.nodes[0], self.segwit_node, 2)
-        self.test_getblocktxn_handler(self.nodes[0], self.old_node, 1)
+        self.test_getblocktxn_handler(self.segwit_node, 2)
+        self.test_getblocktxn_handler(self.old_node, 1)
 
         self.log.info("Testing compactblock requests/announcements not at chain tip...")
-        self.test_compactblocks_not_at_tip(self.nodes[0], self.segwit_node)
-        self.test_compactblocks_not_at_tip(self.nodes[0], self.old_node)
+        self.test_compactblocks_not_at_tip(self.segwit_node)
+        self.test_compactblocks_not_at_tip(self.old_node)
 
         self.log.info("Testing handling of incorrect blocktxn responses...")
-        self.test_incorrect_blocktxn_response(self.nodes[0], self.segwit_node, 2)
+        self.test_incorrect_blocktxn_response(self.segwit_node, 2)
 
         self.log.info("Testing reconstructing compact blocks from all peers...")
-        self.test_compactblock_reconstruction_multiple_peers(self.nodes[0], self.segwit_node, self.additional_segwit_node)
+        self.test_compactblock_reconstruction_multiple_peers(self.segwit_node, self.additional_segwit_node)
 
         # Test that if we submitblock to node1, we'll get a compact block
         # announcement to all peers.
         # (Post-segwit activation, blocks won't propagate from node0 to node1
         # automatically, so don't bother testing a block announced to node0.)
         self.log.info("Testing end-to-end block relay...")
-        self.request_cb_announcements(self.old_node, self.nodes[0], 1)
-        self.request_cb_announcements(self.segwit_node, self.nodes[0], 2)
-        self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.old_node])
+        self.request_cb_announcements(self.old_node, 1)
+        self.request_cb_announcements(self.segwit_node, 2)
+        self.test_end_to_end_block_relay([self.segwit_node, self.old_node])
 
         self.log.info("Testing handling of invalid compact blocks...")
-        self.test_invalid_tx_in_compactblock(self.nodes[0], self.segwit_node)
-        self.test_invalid_tx_in_compactblock(self.nodes[0], self.old_node)
+        self.test_invalid_tx_in_compactblock(self.segwit_node)
+        self.test_invalid_tx_in_compactblock(self.old_node)
 
         self.log.info("Testing invalid index in cmpctblock message...")
         self.test_invalid_cmpctblock_message()
@@ -198,7 +198,7 @@ def make_utxos(self):
     #   are made with compact blocks.
     # If old_node is passed in, request compact blocks with version=preferred-1
     # and verify that it receives block announcements via compact block.
-    def test_sendcmpct(self, node, test_node, preferred_version, old_node=None):
+    def test_sendcmpct(self, test_node, preferred_version, old_node=None):
         # Make sure we get a SENDCMPCT message from our peer
         def received_sendcmpct():
             return (len(test_node.last_sendcmpct) > 0)
@@ -210,7 +210,7 @@ def received_sendcmpct():
             assert_equal(test_node.last_sendcmpct[-1].version, 1)
             test_node.last_sendcmpct = []
 
-        tip = int(node.getbestblockhash(), 16)
+        tip = int(self.nodes[0].getbestblockhash(), 16)
 
         def check_announcement_of_new_block(node, peer, predicate):
             peer.clear_block_announcement()
@@ -224,11 +224,11 @@ def check_announcement_of_new_block(node, peer, predicate):
                         block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
 
         # We shouldn't get any block announcements via cmpctblock yet.
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
 
         # Try one more time, this time after requesting headers.
         test_node.request_headers_and_sync(locator=[tip])
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message and "inv" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message and "inv" in p.last_message)
 
         # Test a few ways of using sendcmpct that should NOT
         # result in compact block announcements.
@@ -240,7 +240,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         sendcmpct.version = preferred_version + 1
         sendcmpct.announce = True
         test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
         test_node.request_headers_and_sync(locator=[tip])
@@ -249,7 +249,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         sendcmpct.version = preferred_version
         sendcmpct.announce = False
         test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
         test_node.request_headers_and_sync(locator=[tip])
@@ -258,26 +258,26 @@ def check_announcement_of_new_block(node, peer, predicate):
         sendcmpct.version = preferred_version
         sendcmpct.announce = True
         test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time (no headers sync should be needed!)
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after turning on sendheaders
         test_node.send_and_ping(msg_sendheaders())
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after sending a version-1, announce=false message.
         sendcmpct.version = preferred_version - 1
         sendcmpct.announce = False
         test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Now turn off announcements
         sendcmpct.version = preferred_version
         sendcmpct.announce = False
         test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
 
         if old_node is not None:
             # Verify that a peer using an older protocol version can receive
@@ -287,7 +287,7 @@ def check_announcement_of_new_block(node, peer, predicate):
             old_node.send_and_ping(sendcmpct)
             # Header sync
             old_node.request_headers_and_sync(locator=[tip])
-            check_announcement_of_new_block(node, old_node, lambda p: "cmpctblock" in p.last_message)
+            check_announcement_of_new_block(self.nodes[0], old_node, lambda p: "cmpctblock" in p.last_message)
 
     # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
     def test_invalid_cmpctblock_message(self):
@@ -305,16 +305,16 @@ def test_invalid_cmpctblock_message(self):
 
     # Compare the generated shortids to what we expect based on BIP 152, given
     # bitcoind's choice of nonce.
-    def test_compactblock_construction(self, node, test_node, version, use_witness_address=True):
+    def test_compactblock_construction(self, test_node, version, use_witness_address=True):
         # Generate a bunch of transactions.
-        node.generate(101)
+        self.nodes[0].generate(101)
         num_transactions = 25
-        address = node.getnewaddress()
+        address = self.nodes[0].getnewaddress()
 
         segwit_tx_generated = False
         for i in range(num_transactions):
-            txid = node.sendtoaddress(address, 0.1)
-            hex_tx = node.gettransaction(txid)["hex"]
+            txid = self.nodes[0].sendtoaddress(address, 0.1)
+            hex_tx = self.nodes[0].gettransaction(txid)["hex"]
             tx = FromHex(CTransaction(), hex_tx)
             if not tx.wit.is_null():
                 segwit_tx_generated = True
@@ -323,18 +323,18 @@ def test_compactblock_construction(self, node, test_node, version, use_witness_a
             assert segwit_tx_generated  # check that our test is not broken
 
         # Wait until we've seen the block announcement for the resulting tip
-        tip = int(node.getbestblockhash(), 16)
+        tip = int(self.nodes[0].getbestblockhash(), 16)
         test_node.wait_for_block_announcement(tip)
 
         # Make sure we will receive a fast-announce compact block
-        self.request_cb_announcements(test_node, node, version)
+        self.request_cb_announcements(test_node, version)
 
         # Now mine a block, and look at the resulting compact block.
         test_node.clear_block_announcement()
-        block_hash = int(node.generate(1)[0], 16)
+        block_hash = int(self.nodes[0].generate(1)[0], 16)
 
         # Store the raw block in our internal format.
-        block = FromHex(CBlock(), node.getblock("%064x" % block_hash, False))
+        block = FromHex(CBlock(), self.nodes[0].getblock("%064x" % block_hash, False))
         for tx in block.vtx:
             tx.calc_sha256()
         block.rehash()
@@ -417,11 +417,11 @@ def check_compactblock_construction_from_block(self, version, header_and_shortid
     # Post-segwit: upgraded nodes would only make this request of cb-version-2,
     # NODE_WITNESS peers.  Unupgraded nodes would still make this request of
     # any cb-version-1-supporting peer.
-    def test_compactblock_requests(self, node, test_node, version, segwit=True):
+    def test_compactblock_requests(self, test_node, version, segwit=True):
         # Try announcing a block with an inv or header, expect a compactblock
         # request
         for announce in ["inv", "header"]:
-            block = self.build_block_on_tip(node, segwit=segwit)
+            block = self.build_block_on_tip(self.nodes[0], segwit=segwit)
             with mininode_lock:
                 test_node.last_message.pop("getdata", None)
 
@@ -446,7 +446,7 @@ def test_compactblock_requests(self, node, test_node, version, segwit=True):
                 coinbase_hash = block.vtx[0].calc_sha256(True)
             comp_block.shortids = [calculate_shortid(k0, k1, coinbase_hash)]
             test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
-            assert_equal(int(node.getbestblockhash(), 16), block.hashPrevBlock)
+            assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
             # Expect a getblocktxn message.
             with mininode_lock:
                 assert "getblocktxn" in test_node.last_message
@@ -461,7 +461,7 @@ def test_compactblock_requests(self, node, test_node, version, segwit=True):
             msg.block_transactions.blockhash = block.sha256
             msg.block_transactions.transactions = [block.vtx[0]]
             test_node.send_and_ping(msg)
-            assert_equal(int(node.getbestblockhash(), 16), block.sha256)
+            assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     # Create a chain of transactions from given utxo, and add to a new block.
     def build_block_with_transactions(self, node, utxo, num_transactions):
@@ -482,7 +482,7 @@ def build_block_with_transactions(self, node, utxo, num_transactions):
     # Test that we only receive getblocktxn requests for transactions that the
     # node needs, and that responding to them causes the block to be
     # reconstructed.
-    def test_getblocktxn_requests(self, node, test_node, version):
+    def test_getblocktxn_requests(self, test_node, version):
         with_witness = (version == 2)
 
         def test_getblocktxn_response(compact_block, peer, expected_result):
@@ -501,7 +501,7 @@ def test_tip_after_message(node, peer, msg, tip):
         # that we receive getblocktxn messages back.
         utxo = self.utxos.pop(0)
 
-        block = self.build_block_with_transactions(node, utxo, 5)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block, use_witness=with_witness)
@@ -512,24 +512,24 @@ def test_tip_after_message(node, peer, msg, tip):
         if with_witness:
             msg_bt = msg_witness_blocktxn()  # serialize with witnesses
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[1:])
-        test_tip_after_message(node, test_node, msg_bt, block.sha256)
+        test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
 
         utxo = self.utxos.pop(0)
-        block = self.build_block_with_transactions(node, utxo, 5)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
 
         # Now try interspersing the prefilled transactions
         comp_block.initialize_from_block(block, prefill_list=[0, 1, 5], use_witness=with_witness)
         test_getblocktxn_response(comp_block, test_node, [2, 3, 4])
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[2:5])
-        test_tip_after_message(node, test_node, msg_bt, block.sha256)
+        test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
 
         # Now try giving one transaction ahead of time.
         utxo = self.utxos.pop(0)
-        block = self.build_block_with_transactions(node, utxo, 5)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         test_node.send_and_ping(msg_tx(block.vtx[1]))
-        assert block.vtx[1].hash in node.getrawmempool()
+        assert block.vtx[1].hash in self.nodes[0].getrawmempool()
 
         # Prefill 4 out of the 6 transactions, and verify that only the one
         # that was not in the mempool is requested.
@@ -537,18 +537,18 @@ def test_tip_after_message(node, peer, msg, tip):
         test_getblocktxn_response(comp_block, test_node, [5])
 
         msg_bt.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]])
-        test_tip_after_message(node, test_node, msg_bt, block.sha256)
+        test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
 
         # Now provide all transactions to the node before the block is
         # announced and verify reconstruction happens immediately.
         utxo = self.utxos.pop(0)
-        block = self.build_block_with_transactions(node, utxo, 10)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         for tx in block.vtx[1:]:
             test_node.send_message(msg_tx(tx))
         test_node.sync_with_ping()
         # Make sure all transactions were accepted.
-        mempool = node.getrawmempool()
+        mempool = self.nodes[0].getrawmempool()
         for tx in block.vtx[1:]:
             assert tx.hash in mempool
 
@@ -558,24 +558,24 @@ def test_tip_after_message(node, peer, msg, tip):
 
         # Send compact block
         comp_block.initialize_from_block(block, prefill_list=[0], use_witness=with_witness)
-        test_tip_after_message(node, test_node, msg_cmpctblock(comp_block.to_p2p()), block.sha256)
+        test_tip_after_message(self.nodes[0], test_node, msg_cmpctblock(comp_block.to_p2p()), block.sha256)
         with mininode_lock:
             # Shouldn't have gotten a request for any transaction
             assert "getblocktxn" not in test_node.last_message
 
     # Incorrectly responding to a getblocktxn shouldn't cause the block to be
     # permanently failed.
-    def test_incorrect_blocktxn_response(self, node, test_node, version):
+    def test_incorrect_blocktxn_response(self, test_node, version):
         utxo = self.utxos.pop(0)
 
-        block = self.build_block_with_transactions(node, utxo, 10)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         # Relay the first 5 transactions from the block in advance
         for tx in block.vtx[1:6]:
             test_node.send_message(msg_tx(tx))
         test_node.sync_with_ping()
         # Make sure all transactions were accepted.
-        mempool = node.getrawmempool()
+        mempool = self.nodes[0].getrawmempool()
         for tx in block.vtx[1:6]:
             assert tx.hash in mempool
 
@@ -604,7 +604,7 @@ def test_incorrect_blocktxn_response(self, node, test_node, version):
         test_node.send_and_ping(msg)
 
         # Tip should not have updated
-        assert_equal(int(node.getbestblockhash(), 16), block.hashPrevBlock)
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
 
         # We should receive a getdata request
         wait_until(lambda: "getdata" in test_node.last_message, timeout=10, lock=mininode_lock)
@@ -617,17 +617,17 @@ def test_incorrect_blocktxn_response(self, node, test_node, version):
             test_node.send_and_ping(msg_witness_block(block))
         else:
             test_node.send_and_ping(msg_block(block))
-        assert_equal(int(node.getbestblockhash(), 16), block.sha256)
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
-    def test_getblocktxn_handler(self, node, test_node, version):
+    def test_getblocktxn_handler(self, test_node, version):
         # bitcoind will not send blocktxn responses for blocks whose height is
         # more than 10 blocks deep.
         MAX_GETBLOCKTXN_DEPTH = 10
-        chain_height = node.getblockcount()
+        chain_height = self.nodes[0].getblockcount()
         current_height = chain_height
         while (current_height >= chain_height - MAX_GETBLOCKTXN_DEPTH):
-            block_hash = node.getblockhash(current_height)
-            block = FromHex(CBlock(), node.getblock(block_hash, False))
+            block_hash = self.nodes[0].getblockhash(current_height)
+            block = FromHex(CBlock(), self.nodes[0].getblock(block_hash, False))
 
             msg = msg_getblocktxn()
             msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [])
@@ -655,7 +655,7 @@ def test_getblocktxn_handler(self, node, test_node, version):
 
         # Next request should send a full block response, as we're past the
         # allowed depth for a blocktxn response.
-        block_hash = node.getblockhash(current_height)
+        block_hash = self.nodes[0].getblockhash(current_height)
         msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [0])
         with mininode_lock:
             test_node.last_message.pop("block", None)
@@ -666,13 +666,13 @@ def test_getblocktxn_handler(self, node, test_node, version):
             assert_equal(test_node.last_message["block"].block.sha256, int(block_hash, 16))
             assert "blocktxn" not in test_node.last_message
 
-    def test_compactblocks_not_at_tip(self, node, test_node):
+    def test_compactblocks_not_at_tip(self, test_node):
         # Test that requesting old compactblocks doesn't work.
         MAX_CMPCTBLOCK_DEPTH = 5
         new_blocks = []
         for i in range(MAX_CMPCTBLOCK_DEPTH + 1):
             test_node.clear_block_announcement()
-            new_blocks.append(node.generate(1)[0])
+            new_blocks.append(self.nodes[0].generate(1)[0])
             wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
 
         test_node.clear_block_announcement()
@@ -680,7 +680,7 @@ def test_compactblocks_not_at_tip(self, node, test_node):
         wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30, lock=mininode_lock)
 
         test_node.clear_block_announcement()
-        node.generate(1)
+        self.nodes[0].generate(1)
         wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
         test_node.clear_block_announcement()
         with mininode_lock:
@@ -692,9 +692,9 @@ def test_compactblocks_not_at_tip(self, node, test_node):
             assert_equal(test_node.last_message["block"].block.sha256, int(new_blocks[0], 16))
 
         # Generate an old compactblock, and verify that it's not accepted.
-        cur_height = node.getblockcount()
-        hashPrevBlock = int(node.getblockhash(cur_height - 5), 16)
-        block = self.build_block_on_tip(node)
+        cur_height = self.nodes[0].getblockcount()
+        hashPrevBlock = int(self.nodes[0].getblockhash(cur_height - 5), 16)
+        block = self.build_block_on_tip(self.nodes[0])
         block.hashPrevBlock = hashPrevBlock
         block.solve()
 
@@ -702,7 +702,7 @@ def test_compactblocks_not_at_tip(self, node, test_node):
         comp_block.initialize_from_block(block)
         test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
 
-        tips = node.getchaintips()
+        tips = self.nodes[0].getchaintips()
         found = False
         for x in tips:
             if x["hash"] == block.hash:
@@ -721,16 +721,16 @@ def test_compactblocks_not_at_tip(self, node, test_node):
         with mininode_lock:
             assert "blocktxn" not in test_node.last_message
 
-    def test_end_to_end_block_relay(self, node, listeners):
+    def test_end_to_end_block_relay(self, listeners):
         utxo = self.utxos.pop(0)
 
-        block = self.build_block_with_transactions(node, utxo, 10)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
 
         [l.clear_block_announcement() for l in listeners]
 
         # ToHex() won't serialize with witness, but this block has no witnesses
         # anyway. TODO: repeat this test with witness tx's to a segwit node.
-        node.submitblock(ToHex(block))
+        self.nodes[0].submitblock(ToHex(block))
 
         for l in listeners:
             wait_until(lambda: l.received_block_announcement(), timeout=30, lock=mininode_lock)
@@ -742,11 +742,11 @@ def test_end_to_end_block_relay(self, node, listeners):
 
     # Test that we don't get disconnected if we relay a compact block with valid header,
     # but invalid transactions.
-    def test_invalid_tx_in_compactblock(self, node, test_node, use_segwit=True):
+    def test_invalid_tx_in_compactblock(self, test_node, use_segwit=True):
         assert len(self.utxos)
         utxo = self.utxos[0]
 
-        block = self.build_block_with_transactions(node, utxo, 5)
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         del block.vtx[3]
         block.hashMerkleRoot = block.calc_merkle_root()
         if use_segwit:
@@ -764,13 +764,13 @@ def test_invalid_tx_in_compactblock(self, node, test_node, use_segwit=True):
         test_node.send_and_ping(msg)
 
         # Check that the tip didn't advance
-        assert int(node.getbestblockhash(), 16) is not block.sha256
+        assert int(self.nodes[0].getbestblockhash(), 16) is not block.sha256
         test_node.sync_with_ping()
 
     # Helper for enabling cb announcements
     # Send the sendcmpct request and sync headers
-    def request_cb_announcements(self, peer, node, version):
-        tip = node.getbestblockhash()
+    def request_cb_announcements(self, peer, version):
+        tip = self.nodes[0].getbestblockhash()
         peer.get_headers(locator=[int(tip, 16)], hashstop=0)
 
         msg = msg_sendcmpct()
@@ -778,7 +778,7 @@ def request_cb_announcements(self, peer, node, version):
         msg.announce = True
         peer.send_and_ping(msg)
 
-    def test_compactblock_reconstruction_multiple_peers(self, node, stalling_peer, delivery_peer):
+    def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer):
         assert len(self.utxos)
 
         def announce_cmpct_block(node, peer):
@@ -793,23 +793,23 @@ def announce_cmpct_block(node, peer):
                 assert "getblocktxn" in peer.last_message
             return block, cmpct_block
 
-        block, cmpct_block = announce_cmpct_block(node, stalling_peer)
+        block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
 
         for tx in block.vtx[1:]:
             delivery_peer.send_message(msg_tx(tx))
         delivery_peer.sync_with_ping()
-        mempool = node.getrawmempool()
+        mempool = self.nodes[0].getrawmempool()
         for tx in block.vtx[1:]:
             assert tx.hash in mempool
 
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
-        assert_equal(int(node.getbestblockhash(), 16), block.sha256)
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
 
         # Now test that delivering an invalid compact block won't break relay
 
-        block, cmpct_block = announce_cmpct_block(node, stalling_peer)
+        block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
         for tx in block.vtx[1:]:
             delivery_peer.send_message(msg_tx(tx))
         delivery_peer.sync_with_ping()
@@ -819,13 +819,13 @@ def announce_cmpct_block(node, peer):
 
         cmpct_block.use_witness = True
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
-        assert int(node.getbestblockhash(), 16) != block.sha256
+        assert int(self.nodes[0].getbestblockhash(), 16) != block.sha256
 
         msg = msg_blocktxn()
         msg.block_transactions.blockhash = block.sha256
         msg.block_transactions.transactions = block.vtx[1:]
         stalling_peer.send_and_ping(msg)
-        assert_equal(int(node.getbestblockhash(), 16), block.sha256)
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
 if __name__ == '__main__':
     CompactBlocksTest().main()

From e862a2bc49c6840646aeaffce9ebdff70581f3da Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 10:43:34 -0400
Subject: [PATCH 08/22] Remove unnecessary version argument from
 test_sendcmpct()

---
 test/functional/p2p_compactblocks.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 62ee9801650f..4d871504afb2 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -122,8 +122,8 @@ def run_test(self):
         assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
 
         self.log.info("Testing SENDCMPCT p2p message... ")
-        self.test_sendcmpct(self.segwit_node, 2, old_node=self.old_node)
-        self.test_sendcmpct(self.additional_segwit_node, 2)
+        self.test_sendcmpct(self.segwit_node, old_node=self.old_node)
+        self.test_sendcmpct(self.additional_segwit_node)
 
         self.log.info("Testing compactblock construction...")
         self.test_compactblock_construction(self.old_node, 1)
@@ -198,15 +198,15 @@ def make_utxos(self):
     #   are made with compact blocks.
     # If old_node is passed in, request compact blocks with version=preferred-1
     # and verify that it receives block announcements via compact block.
-    def test_sendcmpct(self, test_node, preferred_version, old_node=None):
+    def test_sendcmpct(self, test_node, old_node=None):
         # Make sure we get a SENDCMPCT message from our peer
         def received_sendcmpct():
             return (len(test_node.last_sendcmpct) > 0)
         wait_until(received_sendcmpct, timeout=30, lock=mininode_lock)
         with mininode_lock:
-            # Check that the first version received is the preferred one
-            assert_equal(test_node.last_sendcmpct[0].version, preferred_version)
-            # And that we receive versions down to 1.
+            # Check that we receive version 2 first.
+            assert_equal(test_node.last_sendcmpct[0].version, 2)
+            # And that we receive version 1.
             assert_equal(test_node.last_sendcmpct[-1].version, 1)
             test_node.last_sendcmpct = []
 
@@ -237,7 +237,7 @@ def check_announcement_of_new_block(node, peer, predicate):
 
         # Now try a SENDCMPCT message with too-high version
         sendcmpct = msg_sendcmpct()
-        sendcmpct.version = preferred_version + 1
+        sendcmpct.version = 3
         sendcmpct.announce = True
         test_node.send_and_ping(sendcmpct)
         check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
@@ -246,7 +246,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         test_node.request_headers_and_sync(locator=[tip])
 
         # Now try a SENDCMPCT message with valid version, but announce=False
-        sendcmpct.version = preferred_version
+        sendcmpct.version = 2
         sendcmpct.announce = False
         test_node.send_and_ping(sendcmpct)
         check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
@@ -255,7 +255,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         test_node.request_headers_and_sync(locator=[tip])
 
         # Finally, try a SENDCMPCT message with announce=True
-        sendcmpct.version = preferred_version
+        sendcmpct.version = 2
         sendcmpct.announce = True
         test_node.send_and_ping(sendcmpct)
         check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
@@ -267,14 +267,14 @@ def check_announcement_of_new_block(node, peer, predicate):
         test_node.send_and_ping(msg_sendheaders())
         check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
 
-        # Try one more time, after sending a version-1, announce=false message.
-        sendcmpct.version = preferred_version - 1
+        # Try one more time, after sending version 1, announce=false message.
+        sendcmpct.version = 1
         sendcmpct.announce = False
         test_node.send_and_ping(sendcmpct)
         check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Now turn off announcements
-        sendcmpct.version = preferred_version
+        sendcmpct.version = 2
         sendcmpct.announce = False
         test_node.send_and_ping(sendcmpct)
         check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
@@ -282,7 +282,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         if old_node is not None:
             # Verify that a peer using an older protocol version can receive
             # announcements from this node.
-            sendcmpct.version = preferred_version - 1
+            sendcmpct.version = 1
             sendcmpct.announce = True
             old_node.send_and_ping(sendcmpct)
             # Header sync

From a46737d37fd1637dddd4500a010bd25bbdf41a09 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 10:51:27 -0400
Subject: [PATCH 09/22] Remove unnecessary use_witness_address from
 test_compactblock_construction

---
 test/functional/p2p_compactblocks.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 4d871504afb2..0f687e072a2c 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -126,8 +126,8 @@ def run_test(self):
         self.test_sendcmpct(self.additional_segwit_node)
 
         self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.old_node, 1)
-        self.test_compactblock_construction(self.segwit_node, 2)
+        self.test_compactblock_construction(self.old_node, version=1)
+        self.test_compactblock_construction(self.segwit_node, version=2)
 
         self.log.info("Testing compactblock requests (segwit node)... ")
         self.test_compactblock_requests(self.segwit_node, 2)
@@ -305,7 +305,7 @@ def test_invalid_cmpctblock_message(self):
 
     # Compare the generated shortids to what we expect based on BIP 152, given
     # bitcoind's choice of nonce.
-    def test_compactblock_construction(self, test_node, version, use_witness_address=True):
+    def test_compactblock_construction(self, test_node, version):
         # Generate a bunch of transactions.
         self.nodes[0].generate(101)
         num_transactions = 25
@@ -319,8 +319,8 @@ def test_compactblock_construction(self, test_node, version, use_witness_address
             if not tx.wit.is_null():
                 segwit_tx_generated = True
 
-        if use_witness_address:
-            assert segwit_tx_generated  # check that our test is not broken
+        # check that our test is not broken
+        assert segwit_tx_generated
 
         # Wait until we've seen the block announcement for the resulting tip
         tip = int(self.nodes[0].getbestblockhash(), 16)

From 5f4b12b475b02dca2a10069beacfb774e974100e Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 11:01:29 -0400
Subject: [PATCH 10/22] Remove unnecessary arguments from
 test_compactblock_requests

---
 test/functional/p2p_compactblocks.py | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 0f687e072a2c..e9e3b8e43a7e 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -130,7 +130,7 @@ def run_test(self):
         self.test_compactblock_construction(self.segwit_node, version=2)
 
         self.log.info("Testing compactblock requests (segwit node)... ")
-        self.test_compactblock_requests(self.segwit_node, 2)
+        self.test_compactblock_requests(self.segwit_node)
 
         self.log.info("Testing getblocktxn requests (segwit node)...")
         self.test_getblocktxn_requests(self.segwit_node, 2)
@@ -414,14 +414,11 @@ def check_compactblock_construction_from_block(self, version, header_and_shortid
     # Test that bitcoind requests compact blocks when we announce new blocks
     # via header or inv, and that responding to getblocktxn causes the block
     # to be successfully reconstructed.
-    # Post-segwit: upgraded nodes would only make this request of cb-version-2,
-    # NODE_WITNESS peers.  Unupgraded nodes would still make this request of
-    # any cb-version-1-supporting peer.
-    def test_compactblock_requests(self, test_node, version, segwit=True):
+    def test_compactblock_requests(self, test_node):
         # Try announcing a block with an inv or header, expect a compactblock
         # request
         for announce in ["inv", "header"]:
-            block = self.build_block_on_tip(self.nodes[0], segwit=segwit)
+            block = self.build_block_on_tip(self.nodes[0], True)
             with mininode_lock:
                 test_node.last_message.pop("getdata", None)
 
@@ -442,8 +439,7 @@ def test_compactblock_requests(self, test_node, version, segwit=True):
             comp_block.nonce = 0
             [k0, k1] = comp_block.get_siphash_keys()
             coinbase_hash = block.vtx[0].sha256
-            if version == 2:
-                coinbase_hash = block.vtx[0].calc_sha256(True)
+            coinbase_hash = block.vtx[0].calc_sha256(True)
             comp_block.shortids = [calculate_shortid(k0, k1, coinbase_hash)]
             test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
@@ -454,10 +450,7 @@ def test_compactblock_requests(self, test_node, version, segwit=True):
             assert_equal(absolute_indexes, [0])  # should be a coinbase request
 
             # Send the coinbase, and verify that the tip advances.
-            if version == 2:
-                msg = msg_witness_blocktxn()
-            else:
-                msg = msg_blocktxn()
+            msg = msg_witness_blocktxn()
             msg.block_transactions.blockhash = block.sha256
             msg.block_transactions.transactions = [block.vtx[0]]
             test_node.send_and_ping(msg)

From 8cb08e5644fdb105e1d6afce693f25ddc04d0e85 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:01:00 -0400
Subject: [PATCH 11/22] Always construct segwit blocks in p2p_compactblocks.py

---
 test/functional/p2p_compactblocks.py | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index e9e3b8e43a7e..b786f2f70fd0 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -10,7 +10,7 @@
 import random
 
 from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment
-from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_block, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, NODE_WITNESS, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
+from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_block, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, NODE_WITNESS, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.script import CScript, OP_TRUE, OP_DROP
 from test_framework.test_framework import BitcoinTestFramework
@@ -99,14 +99,13 @@ def set_test_params(self):
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
-    def build_block_on_tip(self, node, segwit=False):
+    def build_block_on_tip(self, node):
         height = node.getblockcount()
         tip = node.getbestblockhash()
         mtp = node.getblockheader(tip)['mediantime']
         block = create_block(int(tip, 16), create_coinbase(height + 1), mtp + 1)
         block.nVersion = 4
-        if segwit:
-            add_witness_commitment(block)
+        add_witness_commitment(block)
         block.solve()
         return block
 
@@ -168,7 +167,7 @@ def run_test(self):
     def make_utxos(self):
         """ Create 10 anyone-can-spend UTXOs for testing and send balance to bech32 output."""
         block = self.build_block_on_tip(self.nodes[0])
-        self.segwit_node.send_and_ping(msg_block(block))
+        self.segwit_node.send_and_ping(msg_witness_block(block))
         assert int(self.nodes[0].getbestblockhash(), 16) == block.sha256
         address = self.nodes[0].getnewaddress(address_type="bech32")
         self.nodes[0].generatetoaddress(100, address)
@@ -184,8 +183,9 @@ def make_utxos(self):
         block2 = self.build_block_on_tip(self.nodes[0])
         block2.vtx.append(tx)
         block2.hashMerkleRoot = block2.calc_merkle_root()
+        add_witness_commitment(block2)
         block2.solve()
-        self.segwit_node.send_and_ping(msg_block(block2))
+        self.segwit_node.send_and_ping(msg_witness_block(block2))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
         self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
 
@@ -418,7 +418,7 @@ def test_compactblock_requests(self, test_node):
         # Try announcing a block with an inv or header, expect a compactblock
         # request
         for announce in ["inv", "header"]:
-            block = self.build_block_on_tip(self.nodes[0], True)
+            block = self.build_block_on_tip(self.nodes[0])
             with mininode_lock:
                 test_node.last_message.pop("getdata", None)
 
@@ -469,6 +469,7 @@ def build_block_with_transactions(self, node, utxo, num_transactions):
             block.vtx.append(tx)
 
         block.hashMerkleRoot = block.calc_merkle_root()
+        add_witness_commitment(block)
         block.solve()
         return block
 
@@ -779,7 +780,7 @@ def announce_cmpct_block(node, peer):
             block = self.build_block_with_transactions(node, utxo, 5)
 
             cmpct_block = HeaderAndShortIDs()
-            cmpct_block.initialize_from_block(block)
+            cmpct_block.initialize_from_block(block, use_witness=True)
             msg = msg_cmpctblock(cmpct_block.to_p2p())
             peer.send_and_ping(msg)
             with mininode_lock:
@@ -789,7 +790,7 @@ def announce_cmpct_block(node, peer):
         block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
 
         for tx in block.vtx[1:]:
-            delivery_peer.send_message(msg_tx(tx))
+            delivery_peer.send_message(msg_witness_tx(tx))
         delivery_peer.sync_with_ping()
         mempool = self.nodes[0].getrawmempool()
         for tx in block.vtx[1:]:
@@ -798,17 +799,17 @@ def announce_cmpct_block(node, peer):
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
+        # self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
 
         # Now test that delivering an invalid compact block won't break relay
 
         block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
         for tx in block.vtx[1:]:
-            delivery_peer.send_message(msg_tx(tx))
+            delivery_peer.send_message(msg_witness_tx(tx))
         delivery_peer.sync_with_ping()
 
         cmpct_block.prefilled_txn[0].tx.wit.vtxinwit = [CTxInWitness()]
-        cmpct_block.prefilled_txn[0].tx.wit.vtxinwit[0].scriptWitness.stack = [ser_uint256(0)]
+        cmpct_block.prefilled_txn[0].tx.wit.vtxinwit[0].scriptWitness.stack = [ser_uint256(1)]
 
         cmpct_block.use_witness = True
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))

From 38e4ec12871a4a063396e0cf649631e6e02de7ae Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:02:46 -0400
Subject: [PATCH 12/22] Remove unused version argument from
 test_getblocktxn_requests

---
 test/functional/p2p_compactblocks.py | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index b786f2f70fd0..98756bfe13c0 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -132,7 +132,7 @@ def run_test(self):
         self.test_compactblock_requests(self.segwit_node)
 
         self.log.info("Testing getblocktxn requests (segwit node)...")
-        self.test_getblocktxn_requests(self.segwit_node, 2)
+        self.test_getblocktxn_requests(self.segwit_node)
 
         self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
         self.test_getblocktxn_handler(self.segwit_node, 2)
@@ -476,8 +476,7 @@ def build_block_with_transactions(self, node, utxo, num_transactions):
     # Test that we only receive getblocktxn requests for transactions that the
     # node needs, and that responding to them causes the block to be
     # reconstructed.
-    def test_getblocktxn_requests(self, test_node, version):
-        with_witness = (version == 2)
+    def test_getblocktxn_requests(self, test_node):
 
         def test_getblocktxn_response(compact_block, peer, expected_result):
             msg = msg_cmpctblock(compact_block.to_p2p())
@@ -498,13 +497,11 @@ def test_tip_after_message(node, peer, msg, tip):
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, use_witness=with_witness)
+        comp_block.initialize_from_block(block, use_witness=True)
 
         test_getblocktxn_response(comp_block, test_node, [1, 2, 3, 4, 5])
 
-        msg_bt = msg_blocktxn()
-        if with_witness:
-            msg_bt = msg_witness_blocktxn()  # serialize with witnesses
+        msg_bt = msg_witness_blocktxn()  # serialize with witnesses
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[1:])
         test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
 
@@ -513,7 +510,7 @@ def test_tip_after_message(node, peer, msg, tip):
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
 
         # Now try interspersing the prefilled transactions
-        comp_block.initialize_from_block(block, prefill_list=[0, 1, 5], use_witness=with_witness)
+        comp_block.initialize_from_block(block, prefill_list=[0, 1, 5], use_witness=True)
         test_getblocktxn_response(comp_block, test_node, [2, 3, 4])
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[2:5])
         test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
@@ -527,7 +524,7 @@ def test_tip_after_message(node, peer, msg, tip):
 
         # Prefill 4 out of the 6 transactions, and verify that only the one
         # that was not in the mempool is requested.
-        comp_block.initialize_from_block(block, prefill_list=[0, 2, 3, 4], use_witness=with_witness)
+        comp_block.initialize_from_block(block, prefill_list=[0, 2, 3, 4], use_witness=True)
         test_getblocktxn_response(comp_block, test_node, [5])
 
         msg_bt.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]])
@@ -551,7 +548,7 @@ def test_tip_after_message(node, peer, msg, tip):
             test_node.last_message.pop("getblocktxn", None)
 
         # Send compact block
-        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=with_witness)
+        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
         test_tip_after_message(self.nodes[0], test_node, msg_cmpctblock(comp_block.to_p2p()), block.sha256)
         with mininode_lock:
             # Shouldn't have gotten a request for any transaction

From d941c9b118978682b86aeb32024699991e661cac Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:05:22 -0400
Subject: [PATCH 13/22] Remove unused args from
 test_incorrect_blocktxn_response

---
 test/functional/p2p_compactblocks.py | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 98756bfe13c0..058f4935fa90 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -10,7 +10,7 @@
 import random
 
 from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment
-from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_block, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, NODE_WITNESS, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
+from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, NODE_WITNESS, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.script import CScript, OP_TRUE, OP_DROP
 from test_framework.test_framework import BitcoinTestFramework
@@ -143,7 +143,7 @@ def run_test(self):
         self.test_compactblocks_not_at_tip(self.old_node)
 
         self.log.info("Testing handling of incorrect blocktxn responses...")
-        self.test_incorrect_blocktxn_response(self.segwit_node, 2)
+        self.test_incorrect_blocktxn_response(self.segwit_node)
 
         self.log.info("Testing reconstructing compact blocks from all peers...")
         self.test_compactblock_reconstruction_multiple_peers(self.segwit_node, self.additional_segwit_node)
@@ -556,7 +556,7 @@ def test_tip_after_message(node, peer, msg, tip):
 
     # Incorrectly responding to a getblocktxn shouldn't cause the block to be
     # permanently failed.
-    def test_incorrect_blocktxn_response(self, test_node, version):
+    def test_incorrect_blocktxn_response(self, test_node):
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
@@ -572,7 +572,7 @@ def test_incorrect_blocktxn_response(self, test_node, version):
 
         # Send compact block
         comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=(version == 2))
+        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
         test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
         absolute_indexes = []
         with mininode_lock:
@@ -588,9 +588,7 @@ def test_incorrect_blocktxn_response(self, test_node, version):
         # different peer provide the block further down, so that we're still
         # verifying that the block isn't marked bad permanently. This is good
         # enough for now.
-        msg = msg_blocktxn()
-        if version == 2:
-            msg = msg_witness_blocktxn()
+        msg = msg_witness_blocktxn()
         msg.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]] + block.vtx[7:])
         test_node.send_and_ping(msg)
 
@@ -604,10 +602,7 @@ def test_incorrect_blocktxn_response(self, test_node, version):
         assert_equal(test_node.last_message["getdata"].inv[0].hash, block.sha256)
 
         # Deliver the block
-        if version == 2:
-            test_node.send_and_ping(msg_witness_block(block))
-        else:
-            test_node.send_and_ping(msg_block(block))
+        test_node.send_and_ping(msg_witness_block(block))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     def test_getblocktxn_handler(self, test_node, version):

From 843cc290e05a1a04143475d3fa8a6c8973d6373b Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:15:01 -0400
Subject: [PATCH 14/22] Rename 'node' to 'peer'

The word node should be reserved for the actual bitcoind nodes
under test. P2P connections can be called peers.
---
 test/functional/p2p_compactblocks.py | 286 +++++++++++++--------------
 1 file changed, 143 insertions(+), 143 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 058f4935fa90..53597a258b19 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -111,63 +111,63 @@ def build_block_on_tip(self, node):
 
     def run_test(self):
         # Setup the p2p connections
-        self.segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
-        self.old_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
-        self.additional_segwit_node = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+        self.segwit_peer_1 = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+        self.segwit_peer_2 = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+        self.legacy_peer = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
 
         # Construct UTXOs for use in later tests.
-        self.make_utxos()
+        self.make_utxos(self.segwit_peer_1)
 
         assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
 
         self.log.info("Testing SENDCMPCT p2p message... ")
-        self.test_sendcmpct(self.segwit_node, old_node=self.old_node)
-        self.test_sendcmpct(self.additional_segwit_node)
+        self.test_sendcmpct(self.segwit_peer_1, legacy_peer=self.legacy_peer)
+        self.test_sendcmpct(self.segwit_peer_2)
 
         self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.old_node, version=1)
-        self.test_compactblock_construction(self.segwit_node, version=2)
+        self.test_compactblock_construction(self.legacy_peer, version=1)
+        self.test_compactblock_construction(self.segwit_peer_1, version=2)
 
         self.log.info("Testing compactblock requests (segwit node)... ")
-        self.test_compactblock_requests(self.segwit_node)
+        self.test_compactblock_requests(self.segwit_peer_1)
 
         self.log.info("Testing getblocktxn requests (segwit node)...")
-        self.test_getblocktxn_requests(self.segwit_node)
+        self.test_getblocktxn_requests(self.segwit_peer_1)
 
         self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
-        self.test_getblocktxn_handler(self.segwit_node, 2)
-        self.test_getblocktxn_handler(self.old_node, 1)
+        self.test_getblocktxn_handler(self.segwit_peer_1, 2)
+        self.test_getblocktxn_handler(self.legacy_peer, 1)
 
         self.log.info("Testing compactblock requests/announcements not at chain tip...")
-        self.test_compactblocks_not_at_tip(self.segwit_node)
-        self.test_compactblocks_not_at_tip(self.old_node)
+        self.test_compactblocks_not_at_tip(self.segwit_peer_1)
+        self.test_compactblocks_not_at_tip(self.legacy_peer)
 
         self.log.info("Testing handling of incorrect blocktxn responses...")
-        self.test_incorrect_blocktxn_response(self.segwit_node)
+        self.test_incorrect_blocktxn_response(self.segwit_peer_1)
 
         self.log.info("Testing reconstructing compact blocks from all peers...")
-        self.test_compactblock_reconstruction_multiple_peers(self.segwit_node, self.additional_segwit_node)
+        self.test_compactblock_reconstruction_multiple_peers(self.segwit_peer_1, self.segwit_peer_2)
 
         # Test that if we submitblock to node1, we'll get a compact block
         # announcement to all peers.
         # (Post-segwit activation, blocks won't propagate from node0 to node1
         # automatically, so don't bother testing a block announced to node0.)
         self.log.info("Testing end-to-end block relay...")
-        self.request_cb_announcements(self.old_node, 1)
-        self.request_cb_announcements(self.segwit_node, 2)
-        self.test_end_to_end_block_relay([self.segwit_node, self.old_node])
+        self.request_cb_announcements(self.legacy_peer, 1)
+        self.request_cb_announcements(self.segwit_peer_1, 2)
+        self.test_end_to_end_block_relay([self.segwit_peer_1, self.legacy_peer])
 
         self.log.info("Testing handling of invalid compact blocks...")
-        self.test_invalid_tx_in_compactblock(self.segwit_node)
-        self.test_invalid_tx_in_compactblock(self.old_node)
+        self.test_invalid_tx_in_compactblock(self.segwit_peer_1)
+        self.test_invalid_tx_in_compactblock(self.legacy_peer)
 
         self.log.info("Testing invalid index in cmpctblock message...")
-        self.test_invalid_cmpctblock_message()
+        self.test_invalid_cmpctblock_message(self.segwit_peer_1)
 
-    def make_utxos(self):
+    def make_utxos(self, peer):
         """ Create 10 anyone-can-spend UTXOs for testing and send balance to bech32 output."""
         block = self.build_block_on_tip(self.nodes[0])
-        self.segwit_node.send_and_ping(msg_witness_block(block))
+        peer.send_and_ping(msg_witness_block(block))
         assert int(self.nodes[0].getbestblockhash(), 16) == block.sha256
         address = self.nodes[0].getnewaddress(address_type="bech32")
         self.nodes[0].generatetoaddress(100, address)
@@ -185,7 +185,7 @@ def make_utxos(self):
         block2.hashMerkleRoot = block2.calc_merkle_root()
         add_witness_commitment(block2)
         block2.solve()
-        self.segwit_node.send_and_ping(msg_witness_block(block2))
+        peer.send_and_ping(msg_witness_block(block2))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
         self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
 
@@ -198,17 +198,17 @@ def make_utxos(self):
     #   are made with compact blocks.
     # If old_node is passed in, request compact blocks with version=preferred-1
     # and verify that it receives block announcements via compact block.
-    def test_sendcmpct(self, test_node, old_node=None):
+    def test_sendcmpct(self, peer, legacy_peer=None):
         # Make sure we get a SENDCMPCT message from our peer
         def received_sendcmpct():
-            return (len(test_node.last_sendcmpct) > 0)
+            return (len(peer.last_sendcmpct) > 0)
         wait_until(received_sendcmpct, timeout=30, lock=mininode_lock)
         with mininode_lock:
             # Check that we receive version 2 first.
-            assert_equal(test_node.last_sendcmpct[0].version, 2)
+            assert_equal(peer.last_sendcmpct[0].version, 2)
             # And that we receive version 1.
-            assert_equal(test_node.last_sendcmpct[-1].version, 1)
-            test_node.last_sendcmpct = []
+            assert_equal(peer.last_sendcmpct[-1].version, 1)
+            peer.last_sendcmpct = []
 
         tip = int(self.nodes[0].getbestblockhash(), 16)
 
@@ -224,73 +224,73 @@ def check_announcement_of_new_block(node, peer, predicate):
                         block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
 
         # We shouldn't get any block announcements via cmpctblock yet.
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message)
 
         # Try one more time, this time after requesting headers.
-        test_node.request_headers_and_sync(locator=[tip])
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message and "inv" in p.last_message)
+        peer.request_headers_and_sync(locator=[tip])
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message and "inv" in p.last_message)
 
         # Test a few ways of using sendcmpct that should NOT
         # result in compact block announcements.
         # Before each test, sync the headers chain.
-        test_node.request_headers_and_sync(locator=[tip])
+        peer.request_headers_and_sync(locator=[tip])
 
         # Now try a SENDCMPCT message with too-high version
         sendcmpct = msg_sendcmpct()
         sendcmpct.version = 3
         sendcmpct.announce = True
-        test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
+        peer.send_and_ping(sendcmpct)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
-        test_node.request_headers_and_sync(locator=[tip])
+        peer.request_headers_and_sync(locator=[tip])
 
         # Now try a SENDCMPCT message with valid version, but announce=False
         sendcmpct.version = 2
         sendcmpct.announce = False
-        test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message)
+        peer.send_and_ping(sendcmpct)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
-        test_node.request_headers_and_sync(locator=[tip])
+        peer.request_headers_and_sync(locator=[tip])
 
         # Finally, try a SENDCMPCT message with announce=True
         sendcmpct.version = 2
         sendcmpct.announce = True
-        test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
+        peer.send_and_ping(sendcmpct)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time (no headers sync should be needed!)
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after turning on sendheaders
-        test_node.send_and_ping(msg_sendheaders())
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
+        peer.send_and_ping(msg_sendheaders())
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after sending version 1, announce=false message.
         sendcmpct.version = 1
         sendcmpct.announce = False
-        test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" in p.last_message)
+        peer.send_and_ping(sendcmpct)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
 
         # Now turn off announcements
         sendcmpct.version = 2
         sendcmpct.announce = False
-        test_node.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
+        peer.send_and_ping(sendcmpct)
+        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
 
-        if old_node is not None:
+        if legacy_peer is not None:
             # Verify that a peer using an older protocol version can receive
             # announcements from this node.
             sendcmpct.version = 1
             sendcmpct.announce = True
-            old_node.send_and_ping(sendcmpct)
+            legacy_peer.send_and_ping(sendcmpct)
             # Header sync
-            old_node.request_headers_and_sync(locator=[tip])
-            check_announcement_of_new_block(self.nodes[0], old_node, lambda p: "cmpctblock" in p.last_message)
+            legacy_peer.request_headers_and_sync(locator=[tip])
+            check_announcement_of_new_block(self.nodes[0], legacy_peer, lambda p: "cmpctblock" in p.last_message)
 
     # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
-    def test_invalid_cmpctblock_message(self):
+    def test_invalid_cmpctblock_message(self, peer):
         self.nodes[0].generate(101)
         block = self.build_block_on_tip(self.nodes[0])
 
@@ -300,12 +300,12 @@ def test_invalid_cmpctblock_message(self):
         # This index will be too high
         prefilled_txn = PrefilledTransaction(1, block.vtx[0])
         cmpct_block.prefilled_txn = [prefilled_txn]
-        self.segwit_node.send_await_disconnect(msg_cmpctblock(cmpct_block))
+        peer.send_await_disconnect(msg_cmpctblock(cmpct_block))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
 
     # Compare the generated shortids to what we expect based on BIP 152, given
     # bitcoind's choice of nonce.
-    def test_compactblock_construction(self, test_node, version):
+    def test_compactblock_construction(self, peer, version):
         # Generate a bunch of transactions.
         self.nodes[0].generate(101)
         num_transactions = 25
@@ -324,13 +324,13 @@ def test_compactblock_construction(self, test_node, version):
 
         # Wait until we've seen the block announcement for the resulting tip
         tip = int(self.nodes[0].getbestblockhash(), 16)
-        test_node.wait_for_block_announcement(tip)
+        peer.wait_for_block_announcement(tip)
 
         # Make sure we will receive a fast-announce compact block
-        self.request_cb_announcements(test_node, version)
+        self.request_cb_announcements(peer, version)
 
         # Now mine a block, and look at the resulting compact block.
-        test_node.clear_block_announcement()
+        peer.clear_block_announcement()
         block_hash = int(self.nodes[0].generate(1)[0], 16)
 
         # Store the raw block in our internal format.
@@ -340,30 +340,30 @@ def test_compactblock_construction(self, test_node, version):
         block.rehash()
 
         # Wait until the block was announced (via compact blocks)
-        wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
+        wait_until(peer.received_block_announcement, timeout=30, lock=mininode_lock)
 
         # Now fetch and check the compact block
         header_and_shortids = None
         with mininode_lock:
-            assert "cmpctblock" in test_node.last_message
+            assert "cmpctblock" in peer.last_message
             # Convert the on-the-wire representation to absolute indexes
-            header_and_shortids = HeaderAndShortIDs(test_node.last_message["cmpctblock"].header_and_shortids)
+            header_and_shortids = HeaderAndShortIDs(peer.last_message["cmpctblock"].header_and_shortids)
         self.check_compactblock_construction_from_block(version, header_and_shortids, block_hash, block)
 
         # Now fetch the compact block using a normal non-announce getdata
         with mininode_lock:
-            test_node.clear_block_announcement()
+            peer.clear_block_announcement()
             inv = CInv(4, block_hash)  # 4 == "CompactBlock"
-            test_node.send_message(msg_getdata([inv]))
+            peer.send_message(msg_getdata([inv]))
 
-        wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
+        wait_until(peer.received_block_announcement, timeout=30, lock=mininode_lock)
 
         # Now fetch and check the compact block
         header_and_shortids = None
         with mininode_lock:
-            assert "cmpctblock" in test_node.last_message
+            assert "cmpctblock" in peer.last_message
             # Convert the on-the-wire representation to absolute indexes
-            header_and_shortids = HeaderAndShortIDs(test_node.last_message["cmpctblock"].header_and_shortids)
+            header_and_shortids = HeaderAndShortIDs(peer.last_message["cmpctblock"].header_and_shortids)
         self.check_compactblock_construction_from_block(version, header_and_shortids, block_hash, block)
 
     def check_compactblock_construction_from_block(self, version, header_and_shortids, block_hash, block):
@@ -414,24 +414,24 @@ def check_compactblock_construction_from_block(self, version, header_and_shortid
     # Test that bitcoind requests compact blocks when we announce new blocks
     # via header or inv, and that responding to getblocktxn causes the block
     # to be successfully reconstructed.
-    def test_compactblock_requests(self, test_node):
+    def test_compactblock_requests(self, peer):
         # Try announcing a block with an inv or header, expect a compactblock
         # request
         for announce in ["inv", "header"]:
             block = self.build_block_on_tip(self.nodes[0])
             with mininode_lock:
-                test_node.last_message.pop("getdata", None)
+                peer.last_message.pop("getdata", None)
 
             if announce == "inv":
-                test_node.send_message(msg_inv([CInv(2, block.sha256)]))
-                wait_until(lambda: "getheaders" in test_node.last_message, timeout=30, lock=mininode_lock)
-                test_node.send_header_for_blocks([block])
+                peer.send_message(msg_inv([CInv(2, block.sha256)]))
+                wait_until(lambda: "getheaders" in peer.last_message, timeout=30, lock=mininode_lock)
+                peer.send_header_for_blocks([block])
             else:
-                test_node.send_header_for_blocks([block])
-            wait_until(lambda: "getdata" in test_node.last_message, timeout=30, lock=mininode_lock)
-            assert_equal(len(test_node.last_message["getdata"].inv), 1)
-            assert_equal(test_node.last_message["getdata"].inv[0].type, 4)
-            assert_equal(test_node.last_message["getdata"].inv[0].hash, block.sha256)
+                peer.send_header_for_blocks([block])
+            wait_until(lambda: "getdata" in peer.last_message, timeout=30, lock=mininode_lock)
+            assert_equal(len(peer.last_message["getdata"].inv), 1)
+            assert_equal(peer.last_message["getdata"].inv[0].type, 4)
+            assert_equal(peer.last_message["getdata"].inv[0].hash, block.sha256)
 
             # Send back a compactblock message that omits the coinbase
             comp_block = HeaderAndShortIDs()
@@ -441,19 +441,19 @@ def test_compactblock_requests(self, test_node):
             coinbase_hash = block.vtx[0].sha256
             coinbase_hash = block.vtx[0].calc_sha256(True)
             comp_block.shortids = [calculate_shortid(k0, k1, coinbase_hash)]
-            test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
+            peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
             # Expect a getblocktxn message.
             with mininode_lock:
-                assert "getblocktxn" in test_node.last_message
-                absolute_indexes = test_node.last_message["getblocktxn"].block_txn_request.to_absolute()
+                assert "getblocktxn" in peer.last_message
+                absolute_indexes = peer.last_message["getblocktxn"].block_txn_request.to_absolute()
             assert_equal(absolute_indexes, [0])  # should be a coinbase request
 
             # Send the coinbase, and verify that the tip advances.
             msg = msg_witness_blocktxn()
             msg.block_transactions.blockhash = block.sha256
             msg.block_transactions.transactions = [block.vtx[0]]
-            test_node.send_and_ping(msg)
+            peer.send_and_ping(msg)
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     # Create a chain of transactions from given utxo, and add to a new block.
@@ -476,7 +476,7 @@ def build_block_with_transactions(self, node, utxo, num_transactions):
     # Test that we only receive getblocktxn requests for transactions that the
     # node needs, and that responding to them causes the block to be
     # reconstructed.
-    def test_getblocktxn_requests(self, test_node):
+    def test_getblocktxn_requests(self, peer):
 
         def test_getblocktxn_response(compact_block, peer, expected_result):
             msg = msg_cmpctblock(compact_block.to_p2p())
@@ -499,11 +499,11 @@ def test_tip_after_message(node, peer, msg, tip):
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block, use_witness=True)
 
-        test_getblocktxn_response(comp_block, test_node, [1, 2, 3, 4, 5])
+        test_getblocktxn_response(comp_block, peer, [1, 2, 3, 4, 5])
 
         msg_bt = msg_witness_blocktxn()  # serialize with witnesses
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[1:])
-        test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
+        test_tip_after_message(self.nodes[0], peer, msg_bt, block.sha256)
 
         utxo = self.utxos.pop(0)
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
@@ -511,24 +511,24 @@ def test_tip_after_message(node, peer, msg, tip):
 
         # Now try interspersing the prefilled transactions
         comp_block.initialize_from_block(block, prefill_list=[0, 1, 5], use_witness=True)
-        test_getblocktxn_response(comp_block, test_node, [2, 3, 4])
+        test_getblocktxn_response(comp_block, peer, [2, 3, 4])
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[2:5])
-        test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
+        test_tip_after_message(self.nodes[0], peer, msg_bt, block.sha256)
 
         # Now try giving one transaction ahead of time.
         utxo = self.utxos.pop(0)
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
-        test_node.send_and_ping(msg_tx(block.vtx[1]))
+        peer.send_and_ping(msg_tx(block.vtx[1]))
         assert block.vtx[1].hash in self.nodes[0].getrawmempool()
 
         # Prefill 4 out of the 6 transactions, and verify that only the one
         # that was not in the mempool is requested.
         comp_block.initialize_from_block(block, prefill_list=[0, 2, 3, 4], use_witness=True)
-        test_getblocktxn_response(comp_block, test_node, [5])
+        test_getblocktxn_response(comp_block, peer, [5])
 
         msg_bt.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]])
-        test_tip_after_message(self.nodes[0], test_node, msg_bt, block.sha256)
+        test_tip_after_message(self.nodes[0], peer, msg_bt, block.sha256)
 
         # Now provide all transactions to the node before the block is
         # announced and verify reconstruction happens immediately.
@@ -536,8 +536,8 @@ def test_tip_after_message(node, peer, msg, tip):
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         for tx in block.vtx[1:]:
-            test_node.send_message(msg_tx(tx))
-        test_node.sync_with_ping()
+            peer.send_message(msg_tx(tx))
+        peer.sync_with_ping()
         # Make sure all transactions were accepted.
         mempool = self.nodes[0].getrawmempool()
         for tx in block.vtx[1:]:
@@ -545,26 +545,26 @@ def test_tip_after_message(node, peer, msg, tip):
 
         # Clear out last request.
         with mininode_lock:
-            test_node.last_message.pop("getblocktxn", None)
+            peer.last_message.pop("getblocktxn", None)
 
         # Send compact block
         comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
-        test_tip_after_message(self.nodes[0], test_node, msg_cmpctblock(comp_block.to_p2p()), block.sha256)
+        test_tip_after_message(self.nodes[0], peer, msg_cmpctblock(comp_block.to_p2p()), block.sha256)
         with mininode_lock:
             # Shouldn't have gotten a request for any transaction
-            assert "getblocktxn" not in test_node.last_message
+            assert "getblocktxn" not in peer.last_message
 
     # Incorrectly responding to a getblocktxn shouldn't cause the block to be
     # permanently failed.
-    def test_incorrect_blocktxn_response(self, test_node):
+    def test_incorrect_blocktxn_response(self, peer):
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
         self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         # Relay the first 5 transactions from the block in advance
         for tx in block.vtx[1:6]:
-            test_node.send_message(msg_tx(tx))
-        test_node.sync_with_ping()
+            peer.send_message(msg_tx(tx))
+        peer.sync_with_ping()
         # Make sure all transactions were accepted.
         mempool = self.nodes[0].getrawmempool()
         for tx in block.vtx[1:6]:
@@ -573,11 +573,11 @@ def test_incorrect_blocktxn_response(self, test_node):
         # Send compact block
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
-        test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
+        peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
         absolute_indexes = []
         with mininode_lock:
-            assert "getblocktxn" in test_node.last_message
-            absolute_indexes = test_node.last_message["getblocktxn"].block_txn_request.to_absolute()
+            assert "getblocktxn" in peer.last_message
+            absolute_indexes = peer.last_message["getblocktxn"].block_txn_request.to_absolute()
         assert_equal(absolute_indexes, [6, 7, 8, 9, 10])
 
         # Now give an incorrect response.
@@ -590,22 +590,22 @@ def test_incorrect_blocktxn_response(self, test_node):
         # enough for now.
         msg = msg_witness_blocktxn()
         msg.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]] + block.vtx[7:])
-        test_node.send_and_ping(msg)
+        peer.send_and_ping(msg)
 
         # Tip should not have updated
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
 
         # We should receive a getdata request
-        wait_until(lambda: "getdata" in test_node.last_message, timeout=10, lock=mininode_lock)
-        assert_equal(len(test_node.last_message["getdata"].inv), 1)
-        assert test_node.last_message["getdata"].inv[0].type == 2 or test_node.last_message["getdata"].inv[0].type == 2 | MSG_WITNESS_FLAG
-        assert_equal(test_node.last_message["getdata"].inv[0].hash, block.sha256)
+        wait_until(lambda: "getdata" in peer.last_message, timeout=10, lock=mininode_lock)
+        assert_equal(len(peer.last_message["getdata"].inv), 1)
+        assert peer.last_message["getdata"].inv[0].type == 2 or peer.last_message["getdata"].inv[0].type == 2 | MSG_WITNESS_FLAG
+        assert_equal(peer.last_message["getdata"].inv[0].hash, block.sha256)
 
         # Deliver the block
-        test_node.send_and_ping(msg_witness_block(block))
+        peer.send_and_ping(msg_witness_block(block))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
-    def test_getblocktxn_handler(self, test_node, version):
+    def test_getblocktxn_handler(self, peer, version):
         # bitcoind will not send blocktxn responses for blocks whose height is
         # more than 10 blocks deep.
         MAX_GETBLOCKTXN_DEPTH = 10
@@ -619,15 +619,15 @@ def test_getblocktxn_handler(self, test_node, version):
             msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [])
             num_to_request = random.randint(1, len(block.vtx))
             msg.block_txn_request.from_absolute(sorted(random.sample(range(len(block.vtx)), num_to_request)))
-            test_node.send_message(msg)
-            wait_until(lambda: "blocktxn" in test_node.last_message, timeout=10, lock=mininode_lock)
+            peer.send_message(msg)
+            wait_until(lambda: "blocktxn" in peer.last_message, timeout=10, lock=mininode_lock)
 
             [tx.calc_sha256() for tx in block.vtx]
             with mininode_lock:
-                assert_equal(test_node.last_message["blocktxn"].block_transactions.blockhash, int(block_hash, 16))
+                assert_equal(peer.last_message["blocktxn"].block_transactions.blockhash, int(block_hash, 16))
                 all_indices = msg.block_txn_request.to_absolute()
                 for index in all_indices:
-                    tx = test_node.last_message["blocktxn"].block_transactions.transactions.pop(0)
+                    tx = peer.last_message["blocktxn"].block_transactions.transactions.pop(0)
                     tx.calc_sha256()
                     assert_equal(tx.sha256, block.vtx[index].sha256)
                     if version == 1:
@@ -636,7 +636,7 @@ def test_getblocktxn_handler(self, test_node, version):
                     else:
                         # Check that the witness matches
                         assert_equal(tx.calc_sha256(True), block.vtx[index].calc_sha256(True))
-                test_node.last_message.pop("blocktxn", None)
+                peer.last_message.pop("blocktxn", None)
             current_height -= 1
 
         # Next request should send a full block response, as we're past the
@@ -644,38 +644,38 @@ def test_getblocktxn_handler(self, test_node, version):
         block_hash = self.nodes[0].getblockhash(current_height)
         msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [0])
         with mininode_lock:
-            test_node.last_message.pop("block", None)
-            test_node.last_message.pop("blocktxn", None)
-        test_node.send_and_ping(msg)
+            peer.last_message.pop("block", None)
+            peer.last_message.pop("blocktxn", None)
+        peer.send_and_ping(msg)
         with mininode_lock:
-            test_node.last_message["block"].block.calc_sha256()
-            assert_equal(test_node.last_message["block"].block.sha256, int(block_hash, 16))
-            assert "blocktxn" not in test_node.last_message
+            peer.last_message["block"].block.calc_sha256()
+            assert_equal(peer.last_message["block"].block.sha256, int(block_hash, 16))
+            assert "blocktxn" not in peer.last_message
 
-    def test_compactblocks_not_at_tip(self, test_node):
+    def test_compactblocks_not_at_tip(self, peer):
         # Test that requesting old compactblocks doesn't work.
         MAX_CMPCTBLOCK_DEPTH = 5
         new_blocks = []
         for i in range(MAX_CMPCTBLOCK_DEPTH + 1):
-            test_node.clear_block_announcement()
+            peer.clear_block_announcement()
             new_blocks.append(self.nodes[0].generate(1)[0])
-            wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
+            wait_until(peer.received_block_announcement, timeout=30, lock=mininode_lock)
 
-        test_node.clear_block_announcement()
-        test_node.send_message(msg_getdata([CInv(4, int(new_blocks[0], 16))]))
-        wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30, lock=mininode_lock)
+        peer.clear_block_announcement()
+        peer.send_message(msg_getdata([CInv(4, int(new_blocks[0], 16))]))
+        wait_until(lambda: "cmpctblock" in peer.last_message, timeout=30, lock=mininode_lock)
 
-        test_node.clear_block_announcement()
+        peer.clear_block_announcement()
         self.nodes[0].generate(1)
-        wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
-        test_node.clear_block_announcement()
+        wait_until(peer.received_block_announcement, timeout=30, lock=mininode_lock)
+        peer.clear_block_announcement()
         with mininode_lock:
-            test_node.last_message.pop("block", None)
-        test_node.send_message(msg_getdata([CInv(4, int(new_blocks[0], 16))]))
-        wait_until(lambda: "block" in test_node.last_message, timeout=30, lock=mininode_lock)
+            peer.last_message.pop("block", None)
+        peer.send_message(msg_getdata([CInv(4, int(new_blocks[0], 16))]))
+        wait_until(lambda: "block" in peer.last_message, timeout=30, lock=mininode_lock)
         with mininode_lock:
-            test_node.last_message["block"].block.calc_sha256()
-            assert_equal(test_node.last_message["block"].block.sha256, int(new_blocks[0], 16))
+            peer.last_message["block"].block.calc_sha256()
+            assert_equal(peer.last_message["block"].block.sha256, int(new_blocks[0], 16))
 
         # Generate an old compactblock, and verify that it's not accepted.
         cur_height = self.nodes[0].getblockcount()
@@ -686,7 +686,7 @@ def test_compactblocks_not_at_tip(self, test_node):
 
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block)
-        test_node.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
+        peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
 
         tips = self.nodes[0].getchaintips()
         found = False
@@ -702,33 +702,33 @@ def test_compactblocks_not_at_tip(self, test_node):
         msg = msg_getblocktxn()
         msg.block_txn_request = BlockTransactionsRequest(block.sha256, [0])
         with mininode_lock:
-            test_node.last_message.pop("blocktxn", None)
-        test_node.send_and_ping(msg)
+            peer.last_message.pop("blocktxn", None)
+        peer.send_and_ping(msg)
         with mininode_lock:
-            assert "blocktxn" not in test_node.last_message
+            assert "blocktxn" not in peer.last_message
 
-    def test_end_to_end_block_relay(self, listeners):
+    def test_end_to_end_block_relay(self, peers):
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
 
-        [l.clear_block_announcement() for l in listeners]
+        [l.clear_block_announcement() for l in peers]
 
         # ToHex() won't serialize with witness, but this block has no witnesses
         # anyway. TODO: repeat this test with witness tx's to a segwit node.
         self.nodes[0].submitblock(ToHex(block))
 
-        for l in listeners:
+        for l in peers:
             wait_until(lambda: l.received_block_announcement(), timeout=30, lock=mininode_lock)
         with mininode_lock:
-            for l in listeners:
+            for l in peers:
                 assert "cmpctblock" in l.last_message
                 l.last_message["cmpctblock"].header_and_shortids.header.calc_sha256()
                 assert_equal(l.last_message["cmpctblock"].header_and_shortids.header.sha256, block.sha256)
 
     # Test that we don't get disconnected if we relay a compact block with valid header,
     # but invalid transactions.
-    def test_invalid_tx_in_compactblock(self, test_node, use_segwit=True):
+    def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
         assert len(self.utxos)
         utxo = self.utxos[0]
 
@@ -747,11 +747,11 @@ def test_invalid_tx_in_compactblock(self, test_node, use_segwit=True):
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=use_segwit)
         msg = msg_cmpctblock(comp_block.to_p2p())
-        test_node.send_and_ping(msg)
+        peer.send_and_ping(msg)
 
         # Check that the tip didn't advance
         assert int(self.nodes[0].getbestblockhash(), 16) is not block.sha256
-        test_node.sync_with_ping()
+        peer.sync_with_ping()
 
     # Helper for enabling cb announcements
     # Send the sendcmpct request and sync headers

From f651e6341ac61da931b4dffd99e78c4cbc762595 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:23:27 -0400
Subject: [PATCH 15/22] Nit fixups

---
 test/functional/p2p_compactblocks.py | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 53597a258b19..00a50963f91c 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -10,7 +10,7 @@
 import random
 
 from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment
-from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, NODE_WITNESS, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
+from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.script import CScript, OP_TRUE, OP_DROP
 from test_framework.test_framework import BitcoinTestFramework
@@ -111,8 +111,8 @@ def build_block_on_tip(self, node):
 
     def run_test(self):
         # Setup the p2p connections
-        self.segwit_peer_1 = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
-        self.segwit_peer_2 = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK | NODE_WITNESS)
+        self.segwit_peer_1 = self.nodes[0].add_p2p_connection(TestP2PConn())
+        self.segwit_peer_2 = self.nodes[0].add_p2p_connection(TestP2PConn())
         self.legacy_peer = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
 
         # Construct UTXOs for use in later tests.
@@ -128,13 +128,13 @@ def run_test(self):
         self.test_compactblock_construction(self.legacy_peer, version=1)
         self.test_compactblock_construction(self.segwit_peer_1, version=2)
 
-        self.log.info("Testing compactblock requests (segwit node)... ")
+        self.log.info("Testing compactblock requests...")
         self.test_compactblock_requests(self.segwit_peer_1)
 
-        self.log.info("Testing getblocktxn requests (segwit node)...")
+        self.log.info("Testing getblocktxn requests...")
         self.test_getblocktxn_requests(self.segwit_peer_1)
 
-        self.log.info("Testing getblocktxn handler (segwit node should return witnesses)...")
+        self.log.info("Testing getblocktxn handler...")
         self.test_getblocktxn_handler(self.segwit_peer_1, 2)
         self.test_getblocktxn_handler(self.legacy_peer, 1)
 
@@ -150,8 +150,6 @@ def run_test(self):
 
         # Test that if we submitblock to node1, we'll get a compact block
         # announcement to all peers.
-        # (Post-segwit activation, blocks won't propagate from node0 to node1
-        # automatically, so don't bother testing a block announced to node0.)
         self.log.info("Testing end-to-end block relay...")
         self.request_cb_announcements(self.legacy_peer, 1)
         self.request_cb_announcements(self.segwit_peer_1, 2)

From 6f5d080d2b37e7359d6623fceb22fa46a5ad18c5 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:36:39 -0400
Subject: [PATCH 16/22] Split test_sendcmpct into test_sendcmpct and
 test_sendcmpct_legacy

---
 test/functional/p2p_compactblocks.py | 67 ++++++++++++++--------------
 1 file changed, 34 insertions(+), 33 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 00a50963f91c..6bfa0eb9e176 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -109,6 +109,17 @@ def build_block_on_tip(self, node):
         block.solve()
         return block
 
+    def check_announcement_of_new_block(self, peer, predicate):
+        peer.clear_block_announcement()
+        block_hash = int(self.nodes[0].generate(1)[0], 16)
+        peer.wait_for_block_announcement(block_hash, timeout=30)
+        assert peer.block_announced
+
+        with mininode_lock:
+            assert predicate(peer), (
+                "block_hash={!r}, cmpctblock={!r}, inv={!r}".format(
+                    block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
+
     def run_test(self):
         # Setup the p2p connections
         self.segwit_peer_1 = self.nodes[0].add_p2p_connection(TestP2PConn())
@@ -121,8 +132,9 @@ def run_test(self):
         assert_equal(get_bip9_status(self.nodes[0], "segwit")["status"], 'active')
 
         self.log.info("Testing SENDCMPCT p2p message... ")
-        self.test_sendcmpct(self.segwit_peer_1, legacy_peer=self.legacy_peer)
+        self.test_sendcmpct(self.segwit_peer_1)
         self.test_sendcmpct(self.segwit_peer_2)
+        self.test_sendcmpct_legacy(self.legacy_peer)
 
         self.log.info("Testing compactblock construction...")
         self.test_compactblock_construction(self.legacy_peer, version=1)
@@ -194,8 +206,6 @@ def make_utxos(self, peer):
     #   made with compact blocks.
     # - If sendcmpct is then sent with boolean 1, then new block announcements
     #   are made with compact blocks.
-    # If old_node is passed in, request compact blocks with version=preferred-1
-    # and verify that it receives block announcements via compact block.
     def test_sendcmpct(self, peer, legacy_peer=None):
         # Make sure we get a SENDCMPCT message from our peer
         def received_sendcmpct():
@@ -210,23 +220,12 @@ def received_sendcmpct():
 
         tip = int(self.nodes[0].getbestblockhash(), 16)
 
-        def check_announcement_of_new_block(node, peer, predicate):
-            peer.clear_block_announcement()
-            block_hash = int(node.generate(1)[0], 16)
-            peer.wait_for_block_announcement(block_hash, timeout=30)
-            assert peer.block_announced
-
-            with mininode_lock:
-                assert predicate(peer), (
-                    "block_hash={!r}, cmpctblock={!r}, inv={!r}".format(
-                        block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
-
         # We shouldn't get any block announcements via cmpctblock yet.
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" not in p.last_message)
 
         # Try one more time, this time after requesting headers.
         peer.request_headers_and_sync(locator=[tip])
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message and "inv" in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" not in p.last_message and "inv" in p.last_message)
 
         # Test a few ways of using sendcmpct that should NOT
         # result in compact block announcements.
@@ -238,7 +237,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         sendcmpct.version = 3
         sendcmpct.announce = True
         peer.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
         peer.request_headers_and_sync(locator=[tip])
@@ -247,7 +246,7 @@ def check_announcement_of_new_block(node, peer, predicate):
         sendcmpct.version = 2
         sendcmpct.announce = False
         peer.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
         peer.request_headers_and_sync(locator=[tip])
@@ -256,36 +255,38 @@ def check_announcement_of_new_block(node, peer, predicate):
         sendcmpct.version = 2
         sendcmpct.announce = True
         peer.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time (no headers sync should be needed!)
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after turning on sendheaders
         peer.send_and_ping(msg_sendheaders())
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after sending version 1, announce=false message.
         sendcmpct.version = 1
         sendcmpct.announce = False
         peer.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
         # Now turn off announcements
         sendcmpct.version = 2
         sendcmpct.announce = False
         peer.send_and_ping(sendcmpct)
-        check_announcement_of_new_block(self.nodes[0], peer, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
-
-        if legacy_peer is not None:
-            # Verify that a peer using an older protocol version can receive
-            # announcements from this node.
-            sendcmpct.version = 1
-            sendcmpct.announce = True
-            legacy_peer.send_and_ping(sendcmpct)
-            # Header sync
-            legacy_peer.request_headers_and_sync(locator=[tip])
-            check_announcement_of_new_block(self.nodes[0], legacy_peer, lambda p: "cmpctblock" in p.last_message)
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
+
+    def test_sendcmpct_legacy(self, peer):
+        # Verify that a peer using an older protocol version can receive
+        # announcements from this node.
+        sendcmpct = msg_sendcmpct()
+        sendcmpct.version = 1
+        sendcmpct.announce = True
+        peer.send_and_ping(sendcmpct)
+        # Header sync
+        tip = int(self.nodes[0].getbestblockhash(), 16)
+        peer.request_headers_and_sync(locator=[tip])
+        self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
     # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
     def test_invalid_cmpctblock_message(self, peer):

From 52187a5655e4897c6f3fa4f6ccb231c507f4578c Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:42:54 -0400
Subject: [PATCH 17/22] Reorder p2p_compactblocks.py

Reorder the tests so the subtests are defined in the same order that
they are called. Also move helper methods to the top.
---
 test/functional/p2p_compactblocks.py | 258 +++++++++++++--------------
 1 file changed, 129 insertions(+), 129 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 6bfa0eb9e176..9724bf989f74 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -109,6 +109,23 @@ def build_block_on_tip(self, node):
         block.solve()
         return block
 
+    # Create a chain of transactions from given utxo, and add to a new block.
+    def build_block_with_transactions(self, node, utxo, num_transactions):
+        block = self.build_block_on_tip(node)
+
+        for i in range(num_transactions):
+            tx = CTransaction()
+            tx.vin.append(CTxIn(COutPoint(utxo[0], utxo[1]), b''))
+            tx.vout.append(CTxOut(utxo[2] - 1000, CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])))
+            tx.rehash()
+            utxo = [tx.sha256, 0, tx.vout[0].nValue]
+            block.vtx.append(tx)
+
+        block.hashMerkleRoot = block.calc_merkle_root()
+        add_witness_commitment(block)
+        block.solve()
+        return block
+
     def check_announcement_of_new_block(self, peer, predicate):
         peer.clear_block_announcement()
         block_hash = int(self.nodes[0].generate(1)[0], 16)
@@ -120,6 +137,17 @@ def check_announcement_of_new_block(self, peer, predicate):
                 "block_hash={!r}, cmpctblock={!r}, inv={!r}".format(
                     block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
 
+    # Helper for enabling cb announcements
+    # Send the sendcmpct request and sync headers
+    def request_cb_announcements(self, peer, version):
+        tip = self.nodes[0].getbestblockhash()
+        peer.get_headers(locator=[int(tip, 16)], hashstop=0)
+
+        msg = msg_sendcmpct()
+        msg.version = version
+        msg.announce = True
+        peer.send_and_ping(msg)
+
     def run_test(self):
         # Setup the p2p connections
         self.segwit_peer_1 = self.nodes[0].add_p2p_connection(TestP2PConn())
@@ -160,7 +188,7 @@ def run_test(self):
         self.log.info("Testing reconstructing compact blocks from all peers...")
         self.test_compactblock_reconstruction_multiple_peers(self.segwit_peer_1, self.segwit_peer_2)
 
-        # Test that if we submitblock to node1, we'll get a compact block
+        # Test that if we submitblock to the node, we'll get a compact block
         # announcement to all peers.
         self.log.info("Testing end-to-end block relay...")
         self.request_cb_announcements(self.legacy_peer, 1)
@@ -288,20 +316,6 @@ def test_sendcmpct_legacy(self, peer):
         peer.request_headers_and_sync(locator=[tip])
         self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
-    # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
-    def test_invalid_cmpctblock_message(self, peer):
-        self.nodes[0].generate(101)
-        block = self.build_block_on_tip(self.nodes[0])
-
-        cmpct_block = P2PHeaderAndShortIDs()
-        cmpct_block.header = CBlockHeader(block)
-        cmpct_block.prefilled_txn_length = 1
-        # This index will be too high
-        prefilled_txn = PrefilledTransaction(1, block.vtx[0])
-        cmpct_block.prefilled_txn = [prefilled_txn]
-        peer.send_await_disconnect(msg_cmpctblock(cmpct_block))
-        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
-
     # Compare the generated shortids to what we expect based on BIP 152, given
     # bitcoind's choice of nonce.
     def test_compactblock_construction(self, peer, version):
@@ -455,23 +469,6 @@ def test_compactblock_requests(self, peer):
             peer.send_and_ping(msg)
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
-    # Create a chain of transactions from given utxo, and add to a new block.
-    def build_block_with_transactions(self, node, utxo, num_transactions):
-        block = self.build_block_on_tip(node)
-
-        for i in range(num_transactions):
-            tx = CTransaction()
-            tx.vin.append(CTxIn(COutPoint(utxo[0], utxo[1]), b''))
-            tx.vout.append(CTxOut(utxo[2] - 1000, CScript([OP_TRUE, OP_DROP] * 15 + [OP_TRUE])))
-            tx.rehash()
-            utxo = [tx.sha256, 0, tx.vout[0].nValue]
-            block.vtx.append(tx)
-
-        block.hashMerkleRoot = block.calc_merkle_root()
-        add_witness_commitment(block)
-        block.solve()
-        return block
-
     # Test that we only receive getblocktxn requests for transactions that the
     # node needs, and that responding to them causes the block to be
     # reconstructed.
@@ -553,57 +550,6 @@ def test_tip_after_message(node, peer, msg, tip):
             # Shouldn't have gotten a request for any transaction
             assert "getblocktxn" not in peer.last_message
 
-    # Incorrectly responding to a getblocktxn shouldn't cause the block to be
-    # permanently failed.
-    def test_incorrect_blocktxn_response(self, peer):
-        utxo = self.utxos.pop(0)
-
-        block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
-        # Relay the first 5 transactions from the block in advance
-        for tx in block.vtx[1:6]:
-            peer.send_message(msg_tx(tx))
-        peer.sync_with_ping()
-        # Make sure all transactions were accepted.
-        mempool = self.nodes[0].getrawmempool()
-        for tx in block.vtx[1:6]:
-            assert tx.hash in mempool
-
-        # Send compact block
-        comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
-        peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
-        absolute_indexes = []
-        with mininode_lock:
-            assert "getblocktxn" in peer.last_message
-            absolute_indexes = peer.last_message["getblocktxn"].block_txn_request.to_absolute()
-        assert_equal(absolute_indexes, [6, 7, 8, 9, 10])
-
-        # Now give an incorrect response.
-        # Note that it's possible for bitcoind to be smart enough to know we're
-        # lying, since it could check to see if the shortid matches what we're
-        # sending, and eg disconnect us for misbehavior.  If that behavior
-        # change was made, we could just modify this test by having a
-        # different peer provide the block further down, so that we're still
-        # verifying that the block isn't marked bad permanently. This is good
-        # enough for now.
-        msg = msg_witness_blocktxn()
-        msg.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]] + block.vtx[7:])
-        peer.send_and_ping(msg)
-
-        # Tip should not have updated
-        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
-
-        # We should receive a getdata request
-        wait_until(lambda: "getdata" in peer.last_message, timeout=10, lock=mininode_lock)
-        assert_equal(len(peer.last_message["getdata"].inv), 1)
-        assert peer.last_message["getdata"].inv[0].type == 2 or peer.last_message["getdata"].inv[0].type == 2 | MSG_WITNESS_FLAG
-        assert_equal(peer.last_message["getdata"].inv[0].hash, block.sha256)
-
-        # Deliver the block
-        peer.send_and_ping(msg_witness_block(block))
-        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
-
     def test_getblocktxn_handler(self, peer, version):
         # bitcoind will not send blocktxn responses for blocks whose height is
         # more than 10 blocks deep.
@@ -706,62 +652,56 @@ def test_compactblocks_not_at_tip(self, peer):
         with mininode_lock:
             assert "blocktxn" not in peer.last_message
 
-    def test_end_to_end_block_relay(self, peers):
+    # Incorrectly responding to a getblocktxn shouldn't cause the block to be
+    # permanently failed.
+    def test_incorrect_blocktxn_response(self, peer):
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
+        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
+        # Relay the first 5 transactions from the block in advance
+        for tx in block.vtx[1:6]:
+            peer.send_message(msg_tx(tx))
+        peer.sync_with_ping()
+        # Make sure all transactions were accepted.
+        mempool = self.nodes[0].getrawmempool()
+        for tx in block.vtx[1:6]:
+            assert tx.hash in mempool
 
-        [l.clear_block_announcement() for l in peers]
-
-        # ToHex() won't serialize with witness, but this block has no witnesses
-        # anyway. TODO: repeat this test with witness tx's to a segwit node.
-        self.nodes[0].submitblock(ToHex(block))
-
-        for l in peers:
-            wait_until(lambda: l.received_block_announcement(), timeout=30, lock=mininode_lock)
+        # Send compact block
+        comp_block = HeaderAndShortIDs()
+        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
+        peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
+        absolute_indexes = []
         with mininode_lock:
-            for l in peers:
-                assert "cmpctblock" in l.last_message
-                l.last_message["cmpctblock"].header_and_shortids.header.calc_sha256()
-                assert_equal(l.last_message["cmpctblock"].header_and_shortids.header.sha256, block.sha256)
-
-    # Test that we don't get disconnected if we relay a compact block with valid header,
-    # but invalid transactions.
-    def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
-        assert len(self.utxos)
-        utxo = self.utxos[0]
-
-        block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
-        del block.vtx[3]
-        block.hashMerkleRoot = block.calc_merkle_root()
-        if use_segwit:
-            # If we're testing with segwit, also drop the coinbase witness,
-            # but include the witness commitment.
-            add_witness_commitment(block)
-            block.vtx[0].wit.vtxinwit = []
-        block.solve()
+            assert "getblocktxn" in peer.last_message
+            absolute_indexes = peer.last_message["getblocktxn"].block_txn_request.to_absolute()
+        assert_equal(absolute_indexes, [6, 7, 8, 9, 10])
 
-        # Now send the compact block with all transactions prefilled, and
-        # verify that we don't get disconnected.
-        comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=use_segwit)
-        msg = msg_cmpctblock(comp_block.to_p2p())
+        # Now give an incorrect response.
+        # Note that it's possible for bitcoind to be smart enough to know we're
+        # lying, since it could check to see if the shortid matches what we're
+        # sending, and eg disconnect us for misbehavior.  If that behavior
+        # change was made, we could just modify this test by having a
+        # different peer provide the block further down, so that we're still
+        # verifying that the block isn't marked bad permanently. This is good
+        # enough for now.
+        msg = msg_witness_blocktxn()
+        msg.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]] + block.vtx[7:])
         peer.send_and_ping(msg)
 
-        # Check that the tip didn't advance
-        assert int(self.nodes[0].getbestblockhash(), 16) is not block.sha256
-        peer.sync_with_ping()
+        # Tip should not have updated
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
 
-    # Helper for enabling cb announcements
-    # Send the sendcmpct request and sync headers
-    def request_cb_announcements(self, peer, version):
-        tip = self.nodes[0].getbestblockhash()
-        peer.get_headers(locator=[int(tip, 16)], hashstop=0)
+        # We should receive a getdata request
+        wait_until(lambda: "getdata" in peer.last_message, timeout=10, lock=mininode_lock)
+        assert_equal(len(peer.last_message["getdata"].inv), 1)
+        assert peer.last_message["getdata"].inv[0].type == 2 or peer.last_message["getdata"].inv[0].type == 2 | MSG_WITNESS_FLAG
+        assert_equal(peer.last_message["getdata"].inv[0].hash, block.sha256)
 
-        msg = msg_sendcmpct()
-        msg.version = version
-        msg.announce = True
-        peer.send_and_ping(msg)
+        # Deliver the block
+        peer.send_and_ping(msg_witness_block(block))
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer):
         assert len(self.utxos)
@@ -812,5 +752,65 @@ def announce_cmpct_block(node, peer):
         stalling_peer.send_and_ping(msg)
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
+    def test_end_to_end_block_relay(self, peers):
+        utxo = self.utxos.pop(0)
+
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
+
+        [l.clear_block_announcement() for l in peers]
+
+        # ToHex() won't serialize with witness, but this block has no witnesses
+        # anyway. TODO: repeat this test with witness tx's to a segwit node.
+        self.nodes[0].submitblock(ToHex(block))
+
+        for l in peers:
+            wait_until(lambda: l.received_block_announcement(), timeout=30, lock=mininode_lock)
+        with mininode_lock:
+            for l in peers:
+                assert "cmpctblock" in l.last_message
+                l.last_message["cmpctblock"].header_and_shortids.header.calc_sha256()
+                assert_equal(l.last_message["cmpctblock"].header_and_shortids.header.sha256, block.sha256)
+
+    # Test that we don't get disconnected if we relay a compact block with valid header,
+    # but invalid transactions.
+    def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
+        assert len(self.utxos)
+        utxo = self.utxos[0]
+
+        block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
+        del block.vtx[3]
+        block.hashMerkleRoot = block.calc_merkle_root()
+        if use_segwit:
+            # If we're testing with segwit, also drop the coinbase witness,
+            # but include the witness commitment.
+            add_witness_commitment(block)
+            block.vtx[0].wit.vtxinwit = []
+        block.solve()
+
+        # Now send the compact block with all transactions prefilled, and
+        # verify that we don't get disconnected.
+        comp_block = HeaderAndShortIDs()
+        comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=use_segwit)
+        msg = msg_cmpctblock(comp_block.to_p2p())
+        peer.send_and_ping(msg)
+
+        # Check that the tip didn't advance
+        assert int(self.nodes[0].getbestblockhash(), 16) is not block.sha256
+        peer.sync_with_ping()
+
+    # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
+    def test_invalid_cmpctblock_message(self, peer):
+        self.nodes[0].generate(101)
+        block = self.build_block_on_tip(self.nodes[0])
+
+        cmpct_block = P2PHeaderAndShortIDs()
+        cmpct_block.header = CBlockHeader(block)
+        cmpct_block.prefilled_txn_length = 1
+        # This index will be too high
+        prefilled_txn = PrefilledTransaction(1, block.vtx[0])
+        cmpct_block.prefilled_txn = [prefilled_txn]
+        peer.send_await_disconnect(msg_cmpctblock(cmpct_block))
+        assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock)
+
 if __name__ == '__main__':
     CompactBlocksTest().main()

From 64969432c9696f485f3a2bc100d40cdb0165f11a Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 13:53:07 -0400
Subject: [PATCH 18/22] Add version to p2p connections and remove as argument
 for subtests

---
 test/functional/p2p_compactblocks.py | 31 +++++++++++++++-------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 9724bf989f74..1eb69aceb2f5 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -139,20 +139,23 @@ def check_announcement_of_new_block(self, peer, predicate):
 
     # Helper for enabling cb announcements
     # Send the sendcmpct request and sync headers
-    def request_cb_announcements(self, peer, version):
+    def request_cb_announcements(self, peer):
         tip = self.nodes[0].getbestblockhash()
         peer.get_headers(locator=[int(tip, 16)], hashstop=0)
 
         msg = msg_sendcmpct()
-        msg.version = version
+        msg.version = peer.version
         msg.announce = True
         peer.send_and_ping(msg)
 
     def run_test(self):
         # Setup the p2p connections
         self.segwit_peer_1 = self.nodes[0].add_p2p_connection(TestP2PConn())
+        self.segwit_peer_1.version = 2
         self.segwit_peer_2 = self.nodes[0].add_p2p_connection(TestP2PConn())
+        self.segwit_peer_2.version = 2
         self.legacy_peer = self.nodes[0].add_p2p_connection(TestP2PConn(), services=NODE_NETWORK)
+        self.legacy_peer.version = 1
 
         # Construct UTXOs for use in later tests.
         self.make_utxos(self.segwit_peer_1)
@@ -165,8 +168,8 @@ def run_test(self):
         self.test_sendcmpct_legacy(self.legacy_peer)
 
         self.log.info("Testing compactblock construction...")
-        self.test_compactblock_construction(self.legacy_peer, version=1)
-        self.test_compactblock_construction(self.segwit_peer_1, version=2)
+        self.test_compactblock_construction(self.legacy_peer)
+        self.test_compactblock_construction(self.segwit_peer_1)
 
         self.log.info("Testing compactblock requests...")
         self.test_compactblock_requests(self.segwit_peer_1)
@@ -175,8 +178,8 @@ def run_test(self):
         self.test_getblocktxn_requests(self.segwit_peer_1)
 
         self.log.info("Testing getblocktxn handler...")
-        self.test_getblocktxn_handler(self.segwit_peer_1, 2)
-        self.test_getblocktxn_handler(self.legacy_peer, 1)
+        self.test_getblocktxn_handler(self.segwit_peer_1)
+        self.test_getblocktxn_handler(self.legacy_peer)
 
         self.log.info("Testing compactblock requests/announcements not at chain tip...")
         self.test_compactblocks_not_at_tip(self.segwit_peer_1)
@@ -191,8 +194,6 @@ def run_test(self):
         # Test that if we submitblock to the node, we'll get a compact block
         # announcement to all peers.
         self.log.info("Testing end-to-end block relay...")
-        self.request_cb_announcements(self.legacy_peer, 1)
-        self.request_cb_announcements(self.segwit_peer_1, 2)
         self.test_end_to_end_block_relay([self.segwit_peer_1, self.legacy_peer])
 
         self.log.info("Testing handling of invalid compact blocks...")
@@ -318,7 +319,7 @@ def test_sendcmpct_legacy(self, peer):
 
     # Compare the generated shortids to what we expect based on BIP 152, given
     # bitcoind's choice of nonce.
-    def test_compactblock_construction(self, peer, version):
+    def test_compactblock_construction(self, peer):
         # Generate a bunch of transactions.
         self.nodes[0].generate(101)
         num_transactions = 25
@@ -340,7 +341,7 @@ def test_compactblock_construction(self, peer, version):
         peer.wait_for_block_announcement(tip)
 
         # Make sure we will receive a fast-announce compact block
-        self.request_cb_announcements(peer, version)
+        self.request_cb_announcements(peer)
 
         # Now mine a block, and look at the resulting compact block.
         peer.clear_block_announcement()
@@ -361,7 +362,7 @@ def test_compactblock_construction(self, peer, version):
             assert "cmpctblock" in peer.last_message
             # Convert the on-the-wire representation to absolute indexes
             header_and_shortids = HeaderAndShortIDs(peer.last_message["cmpctblock"].header_and_shortids)
-        self.check_compactblock_construction_from_block(version, header_and_shortids, block_hash, block)
+        self.check_compactblock_construction_from_block(peer.version, header_and_shortids, block_hash, block)
 
         # Now fetch the compact block using a normal non-announce getdata
         with mininode_lock:
@@ -377,7 +378,7 @@ def test_compactblock_construction(self, peer, version):
             assert "cmpctblock" in peer.last_message
             # Convert the on-the-wire representation to absolute indexes
             header_and_shortids = HeaderAndShortIDs(peer.last_message["cmpctblock"].header_and_shortids)
-        self.check_compactblock_construction_from_block(version, header_and_shortids, block_hash, block)
+        self.check_compactblock_construction_from_block(peer.version, header_and_shortids, block_hash, block)
 
     def check_compactblock_construction_from_block(self, version, header_and_shortids, block_hash, block):
         # Check that we got the right block!
@@ -550,7 +551,7 @@ def test_tip_after_message(node, peer, msg, tip):
             # Shouldn't have gotten a request for any transaction
             assert "getblocktxn" not in peer.last_message
 
-    def test_getblocktxn_handler(self, peer, version):
+    def test_getblocktxn_handler(self, peer):
         # bitcoind will not send blocktxn responses for blocks whose height is
         # more than 10 blocks deep.
         MAX_GETBLOCKTXN_DEPTH = 10
@@ -575,7 +576,7 @@ def test_getblocktxn_handler(self, peer, version):
                     tx = peer.last_message["blocktxn"].block_transactions.transactions.pop(0)
                     tx.calc_sha256()
                     assert_equal(tx.sha256, block.vtx[index].sha256)
-                    if version == 1:
+                    if peer.version == 1:
                         # Witnesses should have been stripped
                         assert tx.wit.is_null()
                     else:
@@ -753,6 +754,8 @@ def announce_cmpct_block(node, peer):
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     def test_end_to_end_block_relay(self, peers):
+        for peer in peers:
+            self.request_cb_announcements(peer)
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)

From 19f5f6411ebd615388f6123401f3f7b2f614e298 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 15:30:21 -0400
Subject: [PATCH 19/22] Tidy up method comments in p2p_compactblocks.py

---
 test/functional/p2p_compactblocks.py | 72 +++++++++++++++-------------
 1 file changed, 39 insertions(+), 33 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 1eb69aceb2f5..3d67a762183c 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -75,8 +75,6 @@ def request_headers_and_sync(self, locator, hashstop=0):
         wait_until(self.received_block_announcement, timeout=30, lock=mininode_lock)
         self.clear_block_announcement()
 
-    # Block until a block announcement for a particular block hash is
-    # received.
     def wait_for_block_announcement(self, block_hash, timeout=30):
         def received_hash():
             return (block_hash in self.announced_blockhashes)
@@ -100,6 +98,7 @@ def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def build_block_on_tip(self, node):
+        """Builds a witness block on the tip."""
         height = node.getblockcount()
         tip = node.getbestblockhash()
         mtp = node.getblockheader(tip)['mediantime']
@@ -109,8 +108,10 @@ def build_block_on_tip(self, node):
         block.solve()
         return block
 
-    # Create a chain of transactions from given utxo, and add to a new block.
     def build_block_with_transactions(self, node, utxo, num_transactions):
+        """Builds a witness block with num_transactions on the tip.
+
+        The transactions are built as a chain based on utxo."""
         block = self.build_block_on_tip(node)
 
         for i in range(num_transactions):
@@ -127,6 +128,7 @@ def build_block_with_transactions(self, node, utxo, num_transactions):
         return block
 
     def check_announcement_of_new_block(self, peer, predicate):
+        """Generate a block on the node and check that it is announced to peer."""
         peer.clear_block_announcement()
         block_hash = int(self.nodes[0].generate(1)[0], 16)
         peer.wait_for_block_announcement(block_hash, timeout=30)
@@ -137,9 +139,8 @@ def check_announcement_of_new_block(self, peer, predicate):
                 "block_hash={!r}, cmpctblock={!r}, inv={!r}".format(
                     block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.get("inv", None)))
 
-    # Helper for enabling cb announcements
-    # Send the sendcmpct request and sync headers
     def request_cb_announcements(self, peer):
+        """Send a getheaders and a sendcmpct to the node."""
         tip = self.nodes[0].getbestblockhash()
         peer.get_headers(locator=[int(tip, 16)], hashstop=0)
 
@@ -191,8 +192,6 @@ def run_test(self):
         self.log.info("Testing reconstructing compact blocks from all peers...")
         self.test_compactblock_reconstruction_multiple_peers(self.segwit_peer_1, self.segwit_peer_2)
 
-        # Test that if we submitblock to the node, we'll get a compact block
-        # announcement to all peers.
         self.log.info("Testing end-to-end block relay...")
         self.test_end_to_end_block_relay([self.segwit_peer_1, self.legacy_peer])
 
@@ -228,14 +227,16 @@ def make_utxos(self, peer):
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
         self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
 
-    # Test "sendcmpct" (between peers preferring the same version):
-    # - No compact block announcements unless sendcmpct is sent.
-    # - If sendcmpct is sent with version > preferred_version, the message is ignored.
-    # - If sendcmpct is sent with boolean 0, then block announcements are not
-    #   made with compact blocks.
-    # - If sendcmpct is then sent with boolean 1, then new block announcements
-    #   are made with compact blocks.
     def test_sendcmpct(self, peer, legacy_peer=None):
+        """Test sendcmpct between peers preferring the same version
+
+        - No compact block announcements unless sendcmpct is sent.
+        - If sendcmpct is sent with version > preferred_version, the message is ignored.
+        - If sendcmpct is sent with boolean 0, then block announcements are not
+          made with compact blocks.
+        - If sendcmpct is then sent with boolean 1, then new block announcements
+          are made with compact blocks."""
+
         # Make sure we get a SENDCMPCT message from our peer
         def received_sendcmpct():
             return (len(peer.last_sendcmpct) > 0)
@@ -306,8 +307,7 @@ def received_sendcmpct():
         self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
 
     def test_sendcmpct_legacy(self, peer):
-        # Verify that a peer using an older protocol version can receive
-        # announcements from this node.
+        """Verify that a peer using an older protocol version can receive announcements from this node."""
         sendcmpct = msg_sendcmpct()
         sendcmpct.version = 1
         sendcmpct.announce = True
@@ -317,9 +317,9 @@ def test_sendcmpct_legacy(self, peer):
         peer.request_headers_and_sync(locator=[tip])
         self.check_announcement_of_new_block(peer, lambda p: "cmpctblock" in p.last_message)
 
-    # Compare the generated shortids to what we expect based on BIP 152, given
-    # bitcoind's choice of nonce.
     def test_compactblock_construction(self, peer):
+        """Compare the generated shortids to what we expect based on BIP 152, given bitcoind's choice of nonce."""
+
         # Generate a bunch of transactions.
         self.nodes[0].generate(101)
         num_transactions = 25
@@ -381,7 +381,7 @@ def test_compactblock_construction(self, peer):
         self.check_compactblock_construction_from_block(peer.version, header_and_shortids, block_hash, block)
 
     def check_compactblock_construction_from_block(self, version, header_and_shortids, block_hash, block):
-        # Check that we got the right block!
+        """Check that the compact block sent to the peer is correct."""
         header_and_shortids.header.calc_sha256()
         assert_equal(header_and_shortids.header.sha256, block_hash)
 
@@ -425,10 +425,12 @@ def check_compactblock_construction_from_block(self, version, header_and_shortid
                 header_and_shortids.shortids.pop(0)
             index += 1
 
-    # Test that bitcoind requests compact blocks when we announce new blocks
-    # via header or inv, and that responding to getblocktxn causes the block
-    # to be successfully reconstructed.
     def test_compactblock_requests(self, peer):
+        """Test compactblock requests:
+
+        - bitcoind requests compact blocks when we announce new blocks via header or inv
+        - responding to getblocktxn causes the block to be successfully reconstructed."""
+
         # Try announcing a block with an inv or header, expect a compactblock
         # request
         for announce in ["inv", "header"]:
@@ -470,10 +472,11 @@ def test_compactblock_requests(self, peer):
             peer.send_and_ping(msg)
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
-    # Test that we only receive getblocktxn requests for transactions that the
-    # node needs, and that responding to them causes the block to be
-    # reconstructed.
     def test_getblocktxn_requests(self, peer):
+        """Test getblocktxs requests
+
+        The peer should only receive getblocktxn requests for transactions that the
+        node needs. Responding to them causes the block to be reconstructed."""
 
         def test_getblocktxn_response(compact_block, peer, expected_result):
             msg = msg_cmpctblock(compact_block.to_p2p())
@@ -552,8 +555,9 @@ def test_tip_after_message(node, peer, msg, tip):
             assert "getblocktxn" not in peer.last_message
 
     def test_getblocktxn_handler(self, peer):
-        # bitcoind will not send blocktxn responses for blocks whose height is
-        # more than 10 blocks deep.
+        """Test that bitcoind will not send blocktxn responses for old blocks.
+
+        10 blocks is the max depth for requesting blocktxns."""
         MAX_GETBLOCKTXN_DEPTH = 10
         chain_height = self.nodes[0].getblockcount()
         current_height = chain_height
@@ -599,7 +603,9 @@ def test_getblocktxn_handler(self, peer):
             assert "blocktxn" not in peer.last_message
 
     def test_compactblocks_not_at_tip(self, peer):
-        # Test that requesting old compactblocks doesn't work.
+        """Test that bitcoind will not send compact blocks for old blocks.
+
+        5 blocks is the max depth for requesting compact blocks."""
         MAX_CMPCTBLOCK_DEPTH = 5
         new_blocks = []
         for i in range(MAX_CMPCTBLOCK_DEPTH + 1):
@@ -653,9 +659,8 @@ def test_compactblocks_not_at_tip(self, peer):
         with mininode_lock:
             assert "blocktxn" not in peer.last_message
 
-    # Incorrectly responding to a getblocktxn shouldn't cause the block to be
-    # permanently failed.
     def test_incorrect_blocktxn_response(self, peer):
+        """Test that blocks aren't permanently failed if an incorrect response to a getblocktxn is received."""
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
@@ -705,6 +710,7 @@ def test_incorrect_blocktxn_response(self, peer):
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer):
+        """Test that a compact block can be reconstructed from multiple peers."""
         assert len(self.utxos)
 
         def announce_cmpct_block(node, peer):
@@ -754,6 +760,7 @@ def announce_cmpct_block(node, peer):
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
     def test_end_to_end_block_relay(self, peers):
+        """Test that if we submitblock to the node, we'll get a compact block announcement to all peers."""
         for peer in peers:
             self.request_cb_announcements(peer)
         utxo = self.utxos.pop(0)
@@ -774,9 +781,8 @@ def test_end_to_end_block_relay(self, peers):
                 l.last_message["cmpctblock"].header_and_shortids.header.calc_sha256()
                 assert_equal(l.last_message["cmpctblock"].header_and_shortids.header.sha256, block.sha256)
 
-    # Test that we don't get disconnected if we relay a compact block with valid header,
-    # but invalid transactions.
     def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
+        """Test that we don't get disconnected if we relay a compact block with valid header and invalid transactions."""
         assert len(self.utxos)
         utxo = self.utxos[0]
 
@@ -801,8 +807,8 @@ def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
         assert int(self.nodes[0].getbestblockhash(), 16) is not block.sha256
         peer.sync_with_ping()
 
-    # This test actually causes bitcoind to (reasonably!) disconnect us, so do this last.
     def test_invalid_cmpctblock_message(self, peer):
+        """Test that the node disconnects a peer that sends an invalid compact block."""
         self.nodes[0].generate(101)
         block = self.build_block_on_tip(self.nodes[0])
 

From ad67c17765a1f1bc6939d1470b12f26ab18300f4 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 15:41:42 -0400
Subject: [PATCH 20/22] Generate all required utxos at start of test.

This simplifies the test by not requiring additional utxos to be
created during the test.
---
 test/functional/p2p_compactblocks.py | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 3d67a762183c..db99a195948a 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -92,7 +92,6 @@ class CompactBlocksTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
-        self.utxos = []
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
@@ -225,7 +224,7 @@ def make_utxos(self, peer):
         block2.solve()
         peer.send_and_ping(msg_witness_block(block2))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block2.sha256)
-        self.utxos.extend([[tx.sha256, i, out_value] for i in range(10)])
+        self.utxos = [[tx.sha256, i, out_value] for i in range(10)]
 
     def test_sendcmpct(self, peer, legacy_peer=None):
         """Test sendcmpct between peers preferring the same version
@@ -495,7 +494,6 @@ def test_tip_after_message(node, peer, msg, tip):
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block, use_witness=True)
 
@@ -507,7 +505,6 @@ def test_tip_after_message(node, peer, msg, tip):
 
         utxo = self.utxos.pop(0)
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
 
         # Now try interspersing the prefilled transactions
         comp_block.initialize_from_block(block, prefill_list=[0, 1, 5], use_witness=True)
@@ -518,7 +515,6 @@ def test_tip_after_message(node, peer, msg, tip):
         # Now try giving one transaction ahead of time.
         utxo = self.utxos.pop(0)
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         peer.send_and_ping(msg_tx(block.vtx[1]))
         assert block.vtx[1].hash in self.nodes[0].getrawmempool()
 
@@ -534,7 +530,6 @@ def test_tip_after_message(node, peer, msg, tip):
         # announced and verify reconstruction happens immediately.
         utxo = self.utxos.pop(0)
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         for tx in block.vtx[1:]:
             peer.send_message(msg_tx(tx))
         peer.sync_with_ping()
@@ -664,7 +659,6 @@ def test_incorrect_blocktxn_response(self, peer):
         utxo = self.utxos.pop(0)
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 10)
-        self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
         # Relay the first 5 transactions from the block in advance
         for tx in block.vtx[1:6]:
             peer.send_message(msg_tx(tx))
@@ -711,7 +705,6 @@ def test_incorrect_blocktxn_response(self, peer):
 
     def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer):
         """Test that a compact block can be reconstructed from multiple peers."""
-        assert len(self.utxos)
 
         def announce_cmpct_block(node, peer):
             utxo = self.utxos.pop(0)
@@ -737,8 +730,6 @@ def announce_cmpct_block(node, peer):
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
-        # self.utxos.append([block.vtx[-1].sha256, 0, block.vtx[-1].vout[0].nValue])
-
         # Now test that delivering an invalid compact block won't break relay
 
         block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
@@ -783,7 +774,6 @@ def test_end_to_end_block_relay(self, peers):
 
     def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
         """Test that we don't get disconnected if we relay a compact block with valid header and invalid transactions."""
-        assert len(self.utxos)
         utxo = self.utxos[0]
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)

From 3898a5446167c8bfec0cc79229ad5cd73be6e1f4 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 16:13:16 -0400
Subject: [PATCH 21/22] Always serialize compact blocks as witness blocks

---
 test/functional/p2p_compactblocks.py       | 33 ++++++++++------------
 test/functional/test_framework/messages.py | 15 +++-------
 2 files changed, 19 insertions(+), 29 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index db99a195948a..e988764bceee 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -495,7 +495,7 @@ def test_tip_after_message(node, peer, msg, tip):
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
         comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, use_witness=True)
+        comp_block.initialize_from_block(block)
 
         test_getblocktxn_response(comp_block, peer, [1, 2, 3, 4, 5])
 
@@ -507,7 +507,7 @@ def test_tip_after_message(node, peer, msg, tip):
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
 
         # Now try interspersing the prefilled transactions
-        comp_block.initialize_from_block(block, prefill_list=[0, 1, 5], use_witness=True)
+        comp_block.initialize_from_block(block, prefill_list=[0, 1, 5])
         test_getblocktxn_response(comp_block, peer, [2, 3, 4])
         msg_bt.block_transactions = BlockTransactions(block.sha256, block.vtx[2:5])
         test_tip_after_message(self.nodes[0], peer, msg_bt, block.sha256)
@@ -520,7 +520,7 @@ def test_tip_after_message(node, peer, msg, tip):
 
         # Prefill 4 out of the 6 transactions, and verify that only the one
         # that was not in the mempool is requested.
-        comp_block.initialize_from_block(block, prefill_list=[0, 2, 3, 4], use_witness=True)
+        comp_block.initialize_from_block(block, prefill_list=[0, 2, 3, 4])
         test_getblocktxn_response(comp_block, peer, [5])
 
         msg_bt.block_transactions = BlockTransactions(block.sha256, [block.vtx[5]])
@@ -543,7 +543,7 @@ def test_tip_after_message(node, peer, msg, tip):
             peer.last_message.pop("getblocktxn", None)
 
         # Send compact block
-        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
+        comp_block.initialize_from_block(block, prefill_list=[0])
         test_tip_after_message(self.nodes[0], peer, msg_cmpctblock(comp_block.to_p2p()), block.sha256)
         with mininode_lock:
             # Shouldn't have gotten a request for any transaction
@@ -670,7 +670,7 @@ def test_incorrect_blocktxn_response(self, peer):
 
         # Send compact block
         comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, prefill_list=[0], use_witness=True)
+        comp_block.initialize_from_block(block, prefill_list=[0])
         peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
         absolute_indexes = []
         with mininode_lock:
@@ -711,7 +711,7 @@ def announce_cmpct_block(node, peer):
             block = self.build_block_with_transactions(node, utxo, 5)
 
             cmpct_block = HeaderAndShortIDs()
-            cmpct_block.initialize_from_block(block, use_witness=True)
+            cmpct_block.initialize_from_block(block)
             msg = msg_cmpctblock(cmpct_block.to_p2p())
             peer.send_and_ping(msg)
             with mininode_lock:
@@ -739,8 +739,6 @@ def announce_cmpct_block(node, peer):
 
         cmpct_block.prefilled_txn[0].tx.wit.vtxinwit = [CTxInWitness()]
         cmpct_block.prefilled_txn[0].tx.wit.vtxinwit[0].scriptWitness.stack = [ser_uint256(1)]
-
-        cmpct_block.use_witness = True
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
         assert int(self.nodes[0].getbestblockhash(), 16) != block.sha256
 
@@ -760,9 +758,7 @@ def test_end_to_end_block_relay(self, peers):
 
         [l.clear_block_announcement() for l in peers]
 
-        # ToHex() won't serialize with witness, but this block has no witnesses
-        # anyway. TODO: repeat this test with witness tx's to a segwit node.
-        self.nodes[0].submitblock(ToHex(block))
+        self.nodes[0].submitblock(block.serialize(with_witness=True).hex())
 
         for l in peers:
             wait_until(lambda: l.received_block_announcement(), timeout=30, lock=mininode_lock)
@@ -772,24 +768,25 @@ def test_end_to_end_block_relay(self, peers):
                 l.last_message["cmpctblock"].header_and_shortids.header.calc_sha256()
                 assert_equal(l.last_message["cmpctblock"].header_and_shortids.header.sha256, block.sha256)
 
-    def test_invalid_tx_in_compactblock(self, peer, use_segwit=True):
+    def test_invalid_tx_in_compactblock(self, peer):
         """Test that we don't get disconnected if we relay a compact block with valid header and invalid transactions."""
         utxo = self.utxos[0]
 
         block = self.build_block_with_transactions(self.nodes[0], utxo, 5)
+
+        # Delete an intermediate transaction
         del block.vtx[3]
         block.hashMerkleRoot = block.calc_merkle_root()
-        if use_segwit:
-            # If we're testing with segwit, also drop the coinbase witness,
-            # but include the witness commitment.
-            add_witness_commitment(block)
-            block.vtx[0].wit.vtxinwit = []
+
+        # Also drop the coinbase witness but include the witness commitment.
+        add_witness_commitment(block)
+        block.vtx[0].wit.vtxinwit = []
         block.solve()
 
         # Now send the compact block with all transactions prefilled, and
         # verify that we don't get disconnected.
         comp_block = HeaderAndShortIDs()
-        comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=use_segwit)
+        comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4])
         msg = msg_cmpctblock(comp_block.to_p2p())
         peer.send_and_ping(msg)
 
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 7cf51d9223ac..c8a9edf2b271 100755
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -759,14 +759,13 @@ def calculate_shortid(k0, k1, tx_hash):
 # This version gets rid of the array lengths, and reinterprets the differential
 # encoding into indices that can be used for lookup.
 class HeaderAndShortIDs:
-    __slots__ = ("header", "nonce", "prefilled_txn", "shortids", "use_witness")
+    __slots__ = ("header", "nonce", "prefilled_txn", "shortids")
 
     def __init__(self, p2pheaders_and_shortids = None):
         self.header = CBlockHeader()
         self.nonce = 0
         self.shortids = []
         self.prefilled_txn = []
-        self.use_witness = False
 
         if p2pheaders_and_shortids is not None:
             self.header = p2pheaders_and_shortids.header
@@ -778,10 +777,7 @@ def __init__(self, p2pheaders_and_shortids = None):
                 last_index = self.prefilled_txn[-1].index
 
     def to_p2p(self):
-        if self.use_witness:
-            ret = P2PHeaderAndShortWitnessIDs()
-        else:
-            ret = P2PHeaderAndShortIDs()
+        ret = P2PHeaderAndShortWitnessIDs()
         ret.header = self.header
         ret.nonce = self.nonce
         ret.shortids_length = len(self.shortids)
@@ -803,18 +799,15 @@ def get_siphash_keys(self):
         return [ key0, key1 ]
 
     # Version 2 compact blocks use wtxid in shortids (rather than txid)
-    def initialize_from_block(self, block, nonce=0, prefill_list = [0], use_witness = False):
+    def initialize_from_block(self, block, nonce=0, prefill_list = [0]):
         self.header = CBlockHeader(block)
         self.nonce = nonce
         self.prefilled_txn = [ PrefilledTransaction(i, block.vtx[i]) for i in prefill_list ]
         self.shortids = []
-        self.use_witness = use_witness
         [k0, k1] = self.get_siphash_keys()
         for i in range(len(block.vtx)):
             if i not in prefill_list:
-                tx_hash = block.vtx[i].sha256
-                if use_witness:
-                    tx_hash = block.vtx[i].calc_sha256(with_witness=True)
+                tx_hash = block.vtx[i].calc_sha256(with_witness=True)
                 self.shortids.append(calculate_shortid(k0, k1, tx_hash))
 
     def __repr__(self):

From c7da3ace656bd30b4875153d29704e7467fcd459 Mon Sep 17 00:00:00 2001
From: John Newbery <john@johnnewbery.com>
Date: Mon, 25 Mar 2019 16:17:19 -0400
Subject: [PATCH 22/22] General tidyup

---
 test/functional/p2p_compactblocks.py | 35 +++++++++++++++-------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index e988764bceee..87265fd23531 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -10,7 +10,7 @@
 import random
 
 from test_framework.blocktools import create_block, create_coinbase, add_witness_commitment
-from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256, ToHex
+from test_framework.messages import BlockTransactions, BlockTransactionsRequest, calculate_shortid, CBlock, CBlockHeader, CInv, COutPoint, CTransaction, CTxIn, CTxInWitness, CTxOut, FromHex, HeaderAndShortIDs, msg_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_witness_block, msg_witness_tx, msg_witness_blocktxn, MSG_WITNESS_FLAG, NODE_NETWORK, P2PHeaderAndShortIDs, PrefilledTransaction, ser_uint256
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.script import CScript, OP_TRUE, OP_DROP
 from test_framework.test_framework import BitcoinTestFramework
@@ -320,7 +320,6 @@ def test_compactblock_construction(self, peer):
         """Compare the generated shortids to what we expect based on BIP 152, given bitcoind's choice of nonce."""
 
         # Generate a bunch of transactions.
-        self.nodes[0].generate(101)
         num_transactions = 25
         address = self.nodes[0].getnewaddress()
 
@@ -384,11 +383,12 @@ def check_compactblock_construction_from_block(self, version, header_and_shortid
         header_and_shortids.header.calc_sha256()
         assert_equal(header_and_shortids.header.sha256, block_hash)
 
-        # Make sure the prefilled_txn appears to have included the coinbase
+        # Make sure the prefilled_txn includes the coinbase
         assert len(header_and_shortids.prefilled_txn) >= 1
         assert_equal(header_and_shortids.prefilled_txn[0].index, 0)
 
         # Check that all prefilled_txn entries match what's in the block.
+        # We expect only the coinbase to be prefilled since the node sent all txs to the peer earlier.
         for entry in header_and_shortids.prefilled_txn:
             entry.tx.calc_sha256()
             # This checks the non-witness parts of the tx agree
@@ -445,7 +445,7 @@ def test_compactblock_requests(self, peer):
                 peer.send_header_for_blocks([block])
             wait_until(lambda: "getdata" in peer.last_message, timeout=30, lock=mininode_lock)
             assert_equal(len(peer.last_message["getdata"].inv), 1)
-            assert_equal(peer.last_message["getdata"].inv[0].type, 4)
+            assert_equal(peer.last_message["getdata"].inv[0].type, 4)  # type 4 is compact block
             assert_equal(peer.last_message["getdata"].inv[0].hash, block.sha256)
 
             # Send back a compactblock message that omits the coinbase
@@ -629,20 +629,17 @@ def test_compactblocks_not_at_tip(self, peer):
         hashPrevBlock = int(self.nodes[0].getblockhash(cur_height - 5), 16)
         block = self.build_block_on_tip(self.nodes[0])
         block.hashPrevBlock = hashPrevBlock
+        add_witness_commitment(block)
         block.solve()
 
         comp_block = HeaderAndShortIDs()
         comp_block.initialize_from_block(block)
         peer.send_and_ping(msg_cmpctblock(comp_block.to_p2p()))
 
+        # The block header should be stored as a chain tip.
         tips = self.nodes[0].getchaintips()
-        found = False
-        for x in tips:
-            if x["hash"] == block.hash:
-                assert_equal(x["status"], "headers-only")
-                found = True
-                break
-        assert found
+        tip = next(x for x in tips if x['hash'] == block.hash)
+        assert_equal(tip['status'], 'headers-only')
 
         # Requesting this block via getblocktxn should silently fail
         # (to avoid fingerprinting attacks).
@@ -696,7 +693,7 @@ def test_incorrect_blocktxn_response(self, peer):
         # We should receive a getdata request
         wait_until(lambda: "getdata" in peer.last_message, timeout=10, lock=mininode_lock)
         assert_equal(len(peer.last_message["getdata"].inv), 1)
-        assert peer.last_message["getdata"].inv[0].type == 2 or peer.last_message["getdata"].inv[0].type == 2 | MSG_WITNESS_FLAG
+        assert_equal(peer.last_message["getdata"].inv[0].type, 2 | MSG_WITNESS_FLAG)
         assert_equal(peer.last_message["getdata"].inv[0].hash, block.sha256)
 
         # Deliver the block
@@ -720,6 +717,8 @@ def announce_cmpct_block(node, peer):
 
         block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
 
+        # stalling peer doesn't send the block txs. Instead, delivery peer sends
+        # them as txs.
         for tx in block.vtx[1:]:
             delivery_peer.send_message(msg_witness_tx(tx))
         delivery_peer.sync_with_ping()
@@ -727,21 +726,25 @@ def announce_cmpct_block(node, peer):
         for tx in block.vtx[1:]:
             assert tx.hash in mempool
 
+        # node can construct the block after delivery peer sends a compact block.
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 
         # Now test that delivering an invalid compact block won't break relay
-
         block, cmpct_block = announce_cmpct_block(self.nodes[0], stalling_peer)
         for tx in block.vtx[1:]:
             delivery_peer.send_message(msg_witness_tx(tx))
         delivery_peer.sync_with_ping()
 
+        # Malleate the coinbase witness before sending it to the node in a compact
+        # block from delivery node.
         cmpct_block.prefilled_txn[0].tx.wit.vtxinwit = [CTxInWitness()]
         cmpct_block.prefilled_txn[0].tx.wit.vtxinwit[0].scriptWitness.stack = [ser_uint256(1)]
         delivery_peer.send_and_ping(msg_cmpctblock(cmpct_block.to_p2p()))
         assert int(self.nodes[0].getbestblockhash(), 16) != block.sha256
 
+        # Send the coinbase to the node in a blocktxn message from the stalling peer.
+        # The node should still be able to reconstruct the block.
         msg = msg_blocktxn()
         msg.block_transactions.blockhash = block.sha256
         msg.block_transactions.transactions = block.vtx[1:]
@@ -791,18 +794,18 @@ def test_invalid_tx_in_compactblock(self, peer):
         peer.send_and_ping(msg)
 
         # Check that the tip didn't advance
-        assert int(self.nodes[0].getbestblockhash(), 16) is not block.sha256
+        assert int(self.nodes[0].getbestblockhash(), 16) != block.sha256
         peer.sync_with_ping()
 
     def test_invalid_cmpctblock_message(self, peer):
         """Test that the node disconnects a peer that sends an invalid compact block."""
-        self.nodes[0].generate(101)
         block = self.build_block_on_tip(self.nodes[0])
 
         cmpct_block = P2PHeaderAndShortIDs()
         cmpct_block.header = CBlockHeader(block)
         cmpct_block.prefilled_txn_length = 1
-        # This index will be too high
+
+        # This index is too high
         prefilled_txn = PrefilledTransaction(1, block.vtx[0])
         cmpct_block.prefilled_txn = [prefilled_txn]
         peer.send_await_disconnect(msg_cmpctblock(cmpct_block))