sync_with_ping should assert that ping hasn't timed out

Summary:
This backport core's PR10114

Depends on D968

From 2d353cf194ab00a9e569a2067ddc141c816b4c2a :

Add send_await_disconnect() method to p2p-compactblocks.py

p2p-compactblocks was incorrectly using sync_with_ping() when sending in
invalid block. The node would disconnect us and never respond to the
ping, so the sync_with_ping would just time out after 30 seconds and
continue with the test.

This commit adds a send_await_disconnect() method that sends the
message, and then waits for the node to disconnect us. In this commit
I've added the method to p2p-compactblocks.py, but a future commit could
move it to mininode since it could be useful more generally.

This commit reduces the p2p-compactblock runtime by 30 seconds.

From 8d55c24eadab5033bf931b4e3b4c162e7c58990c :

[tests] sync_with_ping should assert that ping hasn't timed out

sync_with_ping currently returns false if the timeout expires, and it is
the caller's responsibility to fail the test. However, none of the tests
currently assert on sync_with_ping()'s return code. This commit adds an
assert to sync_with_ping so the test will fail if the timeout expires.

This commit also removes all the duplicate implementations of
sync_with_ping() from the individual tests.

Test Plan:
  make check
  ./test/functional/test_runner.py

Reviewers: #bitcoin_abc, schancel

Reviewed By: #bitcoin_abc, schancel

Differential Revision: https://reviews.bitcoinabc.org/D977

test/functional/assumevalid.py

@@ -201,7 +201,8 @@ def run_test(self):
         # Send all blocks to node1. All blocks will be accepted.
         for i in range(2202):
             node1.send_message(msg_block(self.blocks[i]))
-        node1.sync_with_ping()  # make sure the most recent block is synced
+        # Syncing 2200 blocks can take a while on slow systems. Give it plenty of time to sync.
+        node1.sync_with_ping(120)
         assert_equal(self.nodes[1].getblock(
             self.nodes[1].getbestblockhash())['height'], 2202)
 

test/functional/maxuploadtarget.py

@@ -74,16 +74,6 @@ def on_pong(self, conn, message):
     def on_close(self, conn):
         self.peer_disconnected = True
 
-    # Sync up with the node after delivery of a block
-    def sync_with_ping(self, timeout=30):
-        def received_pong():
-            return (self.last_pong.nonce == self.ping_counter)
-        self.connection.send_message(msg_ping(nonce=self.ping_counter))
-        success = wait_until(received_pong, timeout=timeout)
-        self.ping_counter += 1
-        return success
-
-
 class MaxUploadTest(BitcoinTestFramework):
 
     def __init__(self):

test/functional/p2p-acceptblock.py

@@ -92,21 +92,6 @@ def send_message(self, message):
     def on_pong(self, conn, message):
         self.last_pong = message
 
-    # Sync up with the node after delivery of a block
-    def sync_with_ping(self, timeout=30):
-        self.connection.send_message(msg_ping(nonce=self.ping_counter))
-        received_pong = False
-        sleep_time = 0.05
-        while not received_pong and timeout > 0:
-            time.sleep(sleep_time)
-            timeout -= sleep_time
-            with mininode_lock:
-                if self.last_pong.nonce == self.ping_counter:
-                    received_pong = True
-        self.ping_counter += 1
-        return received_pong
-
-
 class AcceptBlockTest(BitcoinTestFramework):
 
     def add_options(self, parser):

test/functional/p2p-compactblocks.py

@@ -37,6 +37,13 @@ def __init__(self):
         # This is for synchronizing the p2p message traffic,
         # so we can eg wait until a particular block is announced.
         self.set_announced_blockhashes = set()
+        self.connected = False
+
+    def on_open(self, conn):
+        self.connected = True
+
+    def on_close(self, conn):
+        self.connected = False
 
     def on_sendcmpct(self, conn, message):
         self.last_sendcmpct.append(message)
@@ -113,6 +120,18 @@ def received_hash():
             return (block_hash in self.set_announced_blockhashes)
         return wait_until(received_hash, timeout=timeout)
 
+    def send_await_disconnect(self, message, timeout=30):
+        """Sends a message to the node and wait for disconnect.
+
+        This is used when we want to send a message into the node that we expect
+        will get us disconnected, eg an invalid block."""
+        self.send_message(message)
+        success = wait_until(lambda: not self.connected, timeout=timeout)
+        if not success:
+            logger.error("send_await_disconnect failed!")
+            raise AssertionError("send_await_disconnect failed!")
+        return success
+
 
 class CompactBlocksTest(BitcoinTestFramework):
 
@@ -284,9 +303,9 @@ 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_and_ping(msg_cmpctblock(cmpct_block))
-        assert(
-            int(self.nodes[0].getbestblockhash(), 16) == block.hashPrevBlock)
+        self.test_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.

test/functional/p2p-mempool.py

@@ -59,15 +59,6 @@ def on_pong(self, conn, message):
     def on_close(self, conn):
         self.peer_disconnected = True
 
-    # Sync up with the node after delivery of a block
-    def sync_with_ping(self, timeout=30):
-        def received_pong():
-            return (self.last_pong.nonce == self.ping_counter)
-        self.connection.send_message(msg_ping(nonce=self.ping_counter))
-        success = wait_until(received_pong, timeout=timeout)
-        self.ping_counter += 1
-        return success
-
     def send_mempool(self):
         self.lastInv = []
         self.send_message(msg_mempool())

test/functional/p2p-versionbits-warning.py

@@ -52,21 +52,6 @@ def send_message(self, message):
     def on_pong(self, conn, message):
         self.last_pong = message
 
-    # Sync up with the node after delivery of a block
-    def sync_with_ping(self, timeout=30):
-        self.connection.send_message(msg_ping(nonce=self.ping_counter))
-        received_pong = False
-        sleep_time = 0.05
-        while not received_pong and timeout > 0:
-            time.sleep(sleep_time)
-            timeout -= sleep_time
-            with mininode_lock:
-                if self.last_pong.nonce == self.ping_counter:
-                    received_pong = True
-        self.ping_counter += 1
-        return received_pong
-
-
 class VersionBitsWarningTest(BitcoinTestFramework):
 
     def __init__(self):

test/functional/test_framework/mininode.py

@@ -1655,11 +1655,14 @@ def send_and_ping(self, message):
         self.sync_with_ping()
 
     # Sync up with the node
-    def sync_with_ping(self, timeout=30):
+    def sync_with_ping(self, timeout=60):
         def received_pong():
             return (self.last_pong.nonce == self.ping_counter)
         self.send_message(msg_ping(nonce=self.ping_counter))
         success = wait_until(received_pong, timeout=timeout)
+        if not success:
+            logger.error("sync_with_ping failed!")
+            raise AssertionError("sync_with_ping failed!")
         self.ping_counter += 1
 
         return success