Merge bitcoin#31859: test: Rename send_message to send_without_ping

fa9cf38 scripted-diff: test: Rename send_message to send_without_ping (MarcoFalke)
fa43567 test: Prefer send_and_ping over send_message+sync_with_ping (MarcoFalke)

Pull request description:

  `send_message` is problematic, because it is easy to forget a `sync_with_ping` (or other `wait_until`), leading to intermittent test failures. (Example: bitcoin#31837 (comment))

  There are more uses of `send_and_ping` in the codebase than `send_message`, so in most cases `send_and_ping` is needed anyway.

  For the remaining cases, clearly document that no sync happens by renaming `send_message` to `send_without_ping`.

ACKs for top commit:
  instagibbs:
    ACK bitcoin@fa9cf38

Tree-SHA512: 31caa6568d292ae3d3dda931a94aaa30cc1205ec2ef537a484393eb55687f86c212f1e751ac4a7636610bdf591502a50995dc63bf02f97be9fdc482072160b07

Author: fanquake

test/functional/README.md

@@ -183,7 +183,7 @@ way is the use the `profile_with_perf` context manager, e.g.
 with node.profile_with_perf("send-big-msgs"):
     # Perform activity on the node you're interested in profiling, e.g.:
     for _ in range(10000):
-        node.p2ps[0].send_message(some_large_message)
+        node.p2ps[0].send_without_ping(some_large_message)
 ```
 
 To see useful textual output, run

test/functional/example_test.py

@@ -184,7 +184,7 @@ def run_test(self):
             block.solve()
             block_message = msg_block(block)
             # Send message is used to send a P2P message to the node over our P2PInterface
-            peer_messaging.send_message(block_message)
+            peer_messaging.send_without_ping(block_message)
             self.tip = block.sha256
             blocks.append(self.tip)
             self.block_time += 1
@@ -209,7 +209,7 @@ def run_test(self):
         getdata_request = msg_getdata()
         for block in blocks:
             getdata_request.inv.append(CInv(MSG_BLOCK, block))
-        peer_receiving.send_message(getdata_request)
+        peer_receiving.send_without_ping(getdata_request)
 
         # wait_until() will loop until a predicate condition is met. Use it to test properties of the
         # P2PInterface objects.

test/functional/feature_assumeutxo.py

@@ -309,7 +309,7 @@ def test_sync_from_assumeutxo_node(self, snapshot):
         msg = msg_headers()
         for block_num in range(1, miner.getblockcount()+1):
             msg.headers.append(from_hex(CBlockHeader(), miner.getblockheader(miner.getblockhash(block_num), verbose=False)))
-        headers_provider_conn.send_message(msg)
+        headers_provider_conn.send_without_ping(msg)
 
         # Ensure headers arrived
         default_value = {'status': ''}  # No status

test/functional/feature_assumevalid.py

@@ -58,7 +58,7 @@ class BaseNode(P2PInterface):
     def send_header_for_blocks(self, new_blocks):
         headers_message = msg_headers()
         headers_message.headers = [CBlockHeader(b) for b in new_blocks]
-        self.send_message(headers_message)
+        self.send_without_ping(headers_message)
 
 
 class AssumeValidTest(BitcoinTestFramework):
@@ -80,7 +80,7 @@ def send_blocks_until_disconnected(self, p2p_conn):
             if not p2p_conn.is_connected:
                 break
             try:
-                p2p_conn.send_message(msg_block(self.blocks[i]))
+                p2p_conn.send_without_ping(msg_block(self.blocks[i]))
             except IOError:
                 assert not p2p_conn.is_connected
                 break
@@ -157,7 +157,7 @@ def run_test(self):
 
         # Send all blocks to node1. All blocks will be accepted.
         for i in range(2202):
-            p2p1.send_message(msg_block(self.blocks[i]))
+            p2p1.send_without_ping(msg_block(self.blocks[i]))
         # Syncing 2200 blocks can take a while on slow systems. Give it plenty of time to sync.
         p2p1.sync_with_ping(timeout=960)
         assert_equal(self.nodes[1].getblock(self.nodes[1].getbestblockhash())['height'], 2202)

test/functional/feature_maxuploadtarget.py

@@ -124,7 +124,7 @@ def run_test(self):
         # the test has been running so far).
         with self.nodes[0].assert_debug_log(expected_msgs=["historical block serving limit reached, disconnecting peer=0"]):
             for _ in range(3):
-                p2p_conns[0].send_message(getdata_request)
+                p2p_conns[0].send_without_ping(getdata_request)
             p2p_conns[0].wait_for_disconnect()
         assert_equal(len(self.nodes[0].getpeerinfo()), 2)
         self.log.info("Peer 0 disconnected after downloading old block too many times")
@@ -148,7 +148,7 @@ def run_test(self):
         # But if p2p_conns[1] tries for an old block, it gets disconnected too.
         getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)]
         with self.nodes[0].assert_debug_log(expected_msgs=["historical block serving limit reached, disconnecting peer=1"]):
-            p2p_conns[1].send_message(getdata_request)
+            p2p_conns[1].send_without_ping(getdata_request)
             p2p_conns[1].wait_for_disconnect()
         assert_equal(len(self.nodes[0].getpeerinfo()), 1)
 
@@ -198,7 +198,7 @@ def run_test(self):
 
         self.log.info("Peer gets disconnected for a mempool request after limit is reached")
         with self.nodes[0].assert_debug_log(expected_msgs=["mempool request with bandwidth limit reached, disconnecting peer=0"]):
-            peer.send_message(msg_mempool())
+            peer.send_without_ping(msg_mempool())
             peer.wait_for_disconnect()
 
         self.log.info("Test passing an unparsable value to -maxuploadtarget throws an error")

test/functional/feature_versionbits_warning.py

@@ -47,7 +47,7 @@ def send_blocks_with_version(self, peer, numblocks, version):
         for _ in range(numblocks):
             block = create_block(tip, create_coinbase(height + 1), block_time, version=version)
             block.solve()
-            peer.send_message(msg_block(block))
+            peer.send_without_ping(msg_block(block))
             block_time += 1
             height += 1
             tip = block.sha256

test/functional/interface_usdt_net.py

@@ -473,7 +473,7 @@ def handle_misbehaving_connection(_, data, __):
         for _ in range(EXPECTED_MISBEHAVING_CONNECTIONS):
             testnode = P2PInterface()
             self.nodes[0].add_p2p_connection(testnode)
-            testnode.send_message(msg)
+            testnode.send_without_ping(msg)
             bpf.perf_buffer_poll(timeout=500)
             testnode.peer_disconnect()
 

test/functional/p2p_addr_relay.py

@@ -76,9 +76,9 @@ def addr_received(self):
 
     def on_version(self, message):
         self.send_version()
-        self.send_message(msg_verack())
+        self.send_without_ping(msg_verack())
         if (self.send_getaddr):
-            self.send_message(msg_getaddr())
+            self.send_without_ping(msg_getaddr())
 
     def getaddr_received(self):
         return self.message_count['getaddr'] > 0
@@ -142,7 +142,7 @@ def oversized_addr_test(self):
 
         msg = self.setup_addr_msg(1010)
         with self.nodes[0].assert_debug_log(['addr message size = 1010']):
-            addr_source.send_message(msg)
+            addr_source.send_without_ping(msg)
             addr_source.wait_for_disconnect()
 
         self.nodes[0].disconnect_p2ps()

test/functional/p2p_addrfetch.py

@@ -62,7 +62,7 @@ def run_test(self):
 
         self.log.info("Check that answering with larger addr messages leads to disconnect")
         msg.addrs = [ADDR] * 2
-        peer.send_message(msg)
+        peer.send_without_ping(msg)
         peer.wait_for_disconnect(timeout=5)
 
         self.log.info("Check timeout for addr-fetch peer that does not send addrs")

test/functional/p2p_addrv2_relay.py

@@ -79,7 +79,7 @@ def run_test(self):
         self.log.info('Check disconnection when sending sendaddrv2 after verack')
         conn = self.nodes[0].add_p2p_connection(P2PInterface())
         with self.nodes[0].assert_debug_log(['sendaddrv2 received after verack, disconnecting peer=0']):
-            conn.send_message(msg_sendaddrv2())
+            conn.send_without_ping(msg_sendaddrv2())
             conn.wait_for_disconnect()
 
         self.log.info('Create connection that sends addrv2 messages')
@@ -104,7 +104,7 @@ def run_test(self):
         self.log.info('Send too-large addrv2 message')
         msg.addrs = ADDRS * 101
         with self.nodes[0].assert_debug_log(['addrv2 message size = 1010']):
-            addr_source.send_message(msg)
+            addr_source.send_without_ping(msg)
             addr_source.wait_for_disconnect()
 
 

test/functional/p2p_blockfilters.py

@@ -212,7 +212,7 @@ def run_test(self):
         for request in requests:
             peer_1 = self.nodes[1].add_p2p_connection(P2PInterface())
             with self.nodes[1].assert_debug_log(expected_msgs=["requested unsupported block filter type"]):
-                peer_1.send_message(request)
+                peer_1.send_without_ping(request)
                 peer_1.wait_for_disconnect()
 
         self.log.info("Check that invalid requests result in disconnection.")
@@ -259,7 +259,7 @@ def run_test(self):
         for request, expected_log_msg in requests:
             peer_0 = self.nodes[0].add_p2p_connection(P2PInterface())
             with self.nodes[0].assert_debug_log(expected_msgs=[expected_log_msg]):
-                peer_0.send_message(request)
+                peer_0.send_without_ping(request)
                 peer_0.wait_for_disconnect()
 
         self.log.info("Test -peerblockfilters without -blockfilterindex raises an error")

test/functional/p2p_blocksonly.py

@@ -34,7 +34,7 @@ def blocksonly_mode_tests(self):
         self.log.info('Check that tx invs also violate the protocol')
         self.nodes[0].add_p2p_connection(P2PInterface())
         with self.nodes[0].assert_debug_log(['transaction (0000000000000000000000000000000000000000000000000000000000001234) inv sent in violation of protocol, disconnecting peer']):
-            self.nodes[0].p2ps[0].send_message(msg_inv([CInv(t=MSG_WTX, h=0x1234)]))
+            self.nodes[0].p2ps[0].send_without_ping(msg_inv([CInv(t=MSG_WTX, h=0x1234)]))
             self.nodes[0].p2ps[0].wait_for_disconnect()
             del self.nodes[0].p2ps[0]
 
@@ -68,7 +68,7 @@ def blocksonly_mode_tests(self):
             # But if, for some reason, first_peer decides to relay transactions to us anyway, we should relay them to
             # second_peer since we gave relay permission to first_peer.
             # See https://github.com/bitcoin/bitcoin/issues/19943 for details.
-            first_peer.send_message(msg_tx(tx))
+            first_peer.send_without_ping(msg_tx(tx))
             self.log.info('Check that the peer with relay-permission is still connected after sending the transaction')
             assert_equal(first_peer.is_connected, True)
             second_peer.wait_for_tx(txid)
@@ -107,7 +107,7 @@ def blocks_relay_conn_tests(self):
     def check_p2p_inv_violation(self, peer):
         self.log.info("Check that tx-invs from P2P are rejected and result in disconnect")
         with self.nodes[0].assert_debug_log(["inv sent in violation of protocol, disconnecting peer"]):
-            peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0x12345)]))
+            peer.send_without_ping(msg_inv([CInv(t=MSG_WTX, h=0x12345)]))
             peer.wait_for_disconnect()
         self.nodes[0].disconnect_p2ps()
 
@@ -116,7 +116,7 @@ def check_p2p_tx_violation(self):
         spendtx = self.miniwallet.create_self_transfer()
 
         with self.nodes[0].assert_debug_log(['transaction sent in violation of protocol, disconnecting peer=0']):
-            self.nodes[0].p2ps[0].send_message(msg_tx(spendtx['tx']))
+            self.nodes[0].p2ps[0].send_without_ping(msg_tx(spendtx['tx']))
             self.nodes[0].p2ps[0].wait_for_disconnect()
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 0)
         self.nodes[0].disconnect_p2ps()

test/functional/p2p_compactblocks.py

@@ -113,12 +113,12 @@ def get_headers(self, locator, hashstop):
         msg = msg_getheaders()
         msg.locator.vHave = locator
         msg.hashstop = hashstop
-        self.send_message(msg)
+        self.send_without_ping(msg)
 
     def send_header_for_blocks(self, new_blocks):
         headers_message = msg_headers()
         headers_message.headers = [CBlockHeader(b) for b in new_blocks]
-        self.send_message(headers_message)
+        self.send_without_ping(headers_message)
 
     def request_headers_and_sync(self, locator, hashstop=0):
         self.clear_block_announcement()
@@ -138,7 +138,7 @@ def send_await_disconnect(self, message, timeout=30):
 
         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)
+        self.send_without_ping(message)
         self.wait_for_disconnect(timeout=timeout)
 
 class CompactBlocksTest(BitcoinTestFramework):
@@ -325,7 +325,7 @@ def test_compactblock_construction(self, test_node):
         # Now fetch the compact block using a normal non-announce getdata
         test_node.clear_block_announcement()
         inv = CInv(MSG_CMPCT_BLOCK, block_hash)
-        test_node.send_message(msg_getdata([inv]))
+        test_node.send_without_ping(msg_getdata([inv]))
 
         test_node.wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30)
 
@@ -386,7 +386,7 @@ def test_compactblock_requests(self, test_node):
             block = self.build_block_on_tip(node)
 
             if announce == "inv":
-                test_node.send_message(msg_inv([CInv(MSG_BLOCK, block.sha256)]))
+                test_node.send_without_ping(msg_inv([CInv(MSG_BLOCK, block.sha256)]))
                 test_node.wait_for_getheaders(timeout=30)
                 test_node.send_header_for_blocks([block])
             else:
@@ -497,7 +497,7 @@ def test_tip_after_message(node, peer, msg, tip):
         block = self.build_block_with_transactions(node, 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.send_without_ping(msg_tx(tx))
         test_node.sync_with_ping()
         # Make sure all transactions were accepted.
         mempool = node.getrawmempool()
@@ -525,7 +525,7 @@ def test_incorrect_blocktxn_response(self, test_node):
         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.send_without_ping(msg_tx(tx))
         test_node.sync_with_ping()
         # Make sure all transactions were accepted.
         mempool = node.getrawmempool()
@@ -581,7 +581,7 @@ def test_getblocktxn_handler(self, test_node):
             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)
+            test_node.send_without_ping(msg)
             test_node.wait_until(lambda: "blocktxn" in test_node.last_message, timeout=10)
 
             [tx.calc_sha256() for tx in block.vtx]
@@ -616,7 +616,7 @@ def test_getblocktxn_handler(self, test_node):
         block = from_hex(CBlock(), node.getblock(block_hash, False))
         msg.block_txn_request = BlockTransactionsRequest(int(block_hash, 16), [len(block.vtx)])
         with node.assert_debug_log(['getblocktxn with out-of-bounds tx indices']):
-            bad_peer.send_message(msg)
+            bad_peer.send_without_ping(msg)
         bad_peer.wait_for_disconnect()
 
     def test_low_work_compactblocks(self, test_node):
@@ -651,7 +651,7 @@ def test_compactblocks_not_at_tip(self, test_node):
             test_node.wait_until(test_node.received_block_announcement, timeout=30)
 
         test_node.clear_block_announcement()
-        test_node.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, int(new_blocks[0], 16))]))
+        test_node.send_without_ping(msg_getdata([CInv(MSG_CMPCT_BLOCK, int(new_blocks[0], 16))]))
         test_node.wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30)
 
         test_node.clear_block_announcement()
@@ -660,7 +660,7 @@ def test_compactblocks_not_at_tip(self, test_node):
         test_node.clear_block_announcement()
         with p2p_lock:
             test_node.last_message.pop("block", None)
-        test_node.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, int(new_blocks[0], 16))]))
+        test_node.send_without_ping(msg_getdata([CInv(MSG_CMPCT_BLOCK, int(new_blocks[0], 16))]))
         test_node.wait_until(lambda: "block" in test_node.last_message, timeout=30)
         with p2p_lock:
             test_node.last_message["block"].block.calc_sha256()
@@ -768,7 +768,7 @@ def announce_cmpct_block(node, peer):
         block, cmpct_block = announce_cmpct_block(node, stalling_peer)
 
         for tx in block.vtx[1:]:
-            delivery_peer.send_message(msg_tx(tx))
+            delivery_peer.send_without_ping(msg_tx(tx))
         delivery_peer.sync_with_ping()
         mempool = node.getrawmempool()
         for tx in block.vtx[1:]:
@@ -783,7 +783,7 @@ def announce_cmpct_block(node, peer):
 
         block, cmpct_block = announce_cmpct_block(node, stalling_peer)
         for tx in block.vtx[1:]:
-            delivery_peer.send_message(msg_tx(tx))
+            delivery_peer.send_without_ping(msg_tx(tx))
         delivery_peer.sync_with_ping()
 
         cmpct_block.prefilled_txn[0].tx.wit.vtxinwit = [CTxInWitness()]

test/functional/p2p_compactblocks_blocksonly.py

@@ -93,19 +93,16 @@ def run_test(self):
 
         block1 = self.build_block_on_tip()
 
-        p2p_conn_blocksonly.send_message(msg_headers(headers=[CBlockHeader(block1)]))
-        p2p_conn_blocksonly.sync_with_ping()
+        p2p_conn_blocksonly.send_and_ping(msg_headers(headers=[CBlockHeader(block1)]))
         assert_equal(p2p_conn_blocksonly.last_message['getdata'].inv, [CInv(MSG_BLOCK | MSG_WITNESS_FLAG, block1.sha256)])
 
-        p2p_conn_high_bw.send_message(msg_headers(headers=[CBlockHeader(block1)]))
-        p2p_conn_high_bw.sync_with_ping()
+        p2p_conn_high_bw.send_and_ping(msg_headers(headers=[CBlockHeader(block1)]))
         assert_equal(p2p_conn_high_bw.last_message['getdata'].inv, [CInv(MSG_CMPCT_BLOCK, block1.sha256)])
 
         self.log.info("Test that getdata(CMPCT) is still sent on BIP152 low bandwidth connections"
                       " when no -blocksonly nodes are involved")
 
         p2p_conn_low_bw.send_and_ping(msg_headers(headers=[CBlockHeader(block1)]))
-        p2p_conn_low_bw.sync_with_ping()
         assert_equal(p2p_conn_low_bw.last_message['getdata'].inv, [CInv(MSG_CMPCT_BLOCK, block1.sha256)])
 
         self.log.info("Test that -blocksonly nodes still serve compact blocks")
@@ -115,7 +112,7 @@ def test_for_cmpctblock(block):
                 return False
             return p2p_conn_blocksonly.last_message['cmpctblock'].header_and_shortids.header.rehash() == block.sha256
 
-        p2p_conn_blocksonly.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, block0.sha256)]))
+        p2p_conn_blocksonly.send_without_ping(msg_getdata([CInv(MSG_CMPCT_BLOCK, block0.sha256)]))
         p2p_conn_blocksonly.wait_until(lambda: test_for_cmpctblock(block0))
 
         # Request BIP152 high bandwidth mode from the -blocksonly node.

test/functional/p2p_eviction.py

@@ -34,13 +34,13 @@
 class SlowP2PDataStore(P2PDataStore):
     def on_ping(self, message):
         time.sleep(0.1)
-        self.send_message(msg_pong(message.nonce))
+        self.send_without_ping(msg_pong(message.nonce))
 
 
 class SlowP2PInterface(P2PInterface):
     def on_ping(self, message):
         time.sleep(0.1)
-        self.send_message(msg_pong(message.nonce))
+        self.send_without_ping(msg_pong(message.nonce))
 
 
 class P2PEvict(BitcoinTestFramework):
@@ -82,7 +82,7 @@ def run_test(self):
             txpeer.sync_with_ping()
 
             tx = self.wallet.create_self_transfer()['tx']
-            txpeer.send_message(msg_tx(tx))
+            txpeer.send_without_ping(msg_tx(tx))
             protected_peers.add(current_peer)
 
         self.log.info("Create 8 peers and protect them from eviction by having faster pings")