Merge #18764: refactor: test: replace inv type magic numbers by constants

4a614ff test: explicit imports from test_framework.messages in p2p_invalid_messages.py (Sebastian Falbesoner)
b35e1d2 test: add inventory type constant MSG_CMPCT_BLOCK (Sebastian Falbesoner)
eeaaa58 test: replace inv type magic numbers by constants (Sebastian Falbesoner)

Pull request description:

  Many functional tests still use magic numbers for inventory types, either passed to the `CInv` constructor or for comparing the `type` member of `CInv`. This PR replaces all of those by constants in the module `test_framework.messages` that have been introduced in commit c32cf9f: `MSG_TX` (1) or `MSG_BLOCK` (2).

  It also introduces a new constant `MSG_CMPCT_BLOCK` (naming as in `src/protocol.h`) and uses it to replace the remaining magic numbers.

  The occurences of the magic numbers were identified through `grep`ing for `CInv(` and `type ==`. The idea was first to create a scripted-diff, but since also adding missing `import`s is needed, this would be non-trivial. Besides, also some unneeded comments like `# 2 == "Block"` could be removed.

ACKs for top commit:
  gzhao408:
    ACK [`4a614ff`](bitcoin/bitcoin@4a614ff)

Tree-SHA512: 4ba4fdef9f3eef7fd5ac72cb03ca3524863d1ae292161c550424a4c1047283fa2d2e7e03017d1fbae3652b3cb14f08b8d4b368403f3f209993aef3f2e2b22784

Author: MarcoFalke

test/functional/example_test.py

@@ -15,7 +15,7 @@
 
 # Avoid wildcard * imports
 from test_framework.blocktools import (create_block, create_coinbase)
-from test_framework.messages import CInv
+from test_framework.messages import CInv, MSG_BLOCK
 from test_framework.mininode import (
     P2PInterface,
     mininode_lock,
@@ -198,7 +198,7 @@ def run_test(self):
 
         getdata_request = msg_getdata()
         for block in blocks:
-            getdata_request.inv.append(CInv(2, block))
+            getdata_request.inv.append(CInv(MSG_BLOCK, block))
         self.nodes[2].p2p.send_message(getdata_request)
 
         # wait_until() will loop until a predicate condition is met. Use it to test properties of the

test/functional/feature_maxuploadtarget.py

@@ -13,7 +13,7 @@
 from collections import defaultdict
 import time
 
-from test_framework.messages import CInv, msg_getdata
+from test_framework.messages import CInv, MSG_BLOCK, msg_getdata
 from test_framework.mininode import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal, mine_large_block
@@ -84,7 +84,7 @@ def run_test(self):
         # the same big old block too many times (expect: disconnect)
 
         getdata_request = msg_getdata()
-        getdata_request.inv.append(CInv(2, big_old_block))
+        getdata_request.inv.append(CInv(MSG_BLOCK, big_old_block))
 
         max_bytes_per_day = 800*1024*1024
         daily_buffer = 144 * 4000000
@@ -109,15 +109,15 @@ def run_test(self):
         # Requesting the current block on p2p_conns[1] should succeed indefinitely,
         # even when over the max upload target.
         # We'll try 800 times
-        getdata_request.inv = [CInv(2, big_new_block)]
+        getdata_request.inv = [CInv(MSG_BLOCK, big_new_block)]
         for i in range(800):
             p2p_conns[1].send_and_ping(getdata_request)
             assert_equal(p2p_conns[1].block_receive_map[big_new_block], i+1)
 
         self.log.info("Peer 1 able to repeatedly download new block")
 
         # But if p2p_conns[1] tries for an old block, it gets disconnected too.
-        getdata_request.inv = [CInv(2, big_old_block)]
+        getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)]
         p2p_conns[1].send_message(getdata_request)
         p2p_conns[1].wait_for_disconnect()
         assert_equal(len(self.nodes[0].getpeerinfo()), 1)
@@ -145,12 +145,12 @@ def run_test(self):
         self.nodes[0].add_p2p_connection(TestP2PConn())
 
         #retrieve 20 blocks which should be enough to break the 1MB limit
-        getdata_request.inv = [CInv(2, big_new_block)]
+        getdata_request.inv = [CInv(MSG_BLOCK, big_new_block)]
         for i in range(20):
             self.nodes[0].p2p.send_and_ping(getdata_request)
             assert_equal(self.nodes[0].p2p.block_receive_map[big_new_block], i+1)
 
-        getdata_request.inv = [CInv(2, big_old_block)]
+        getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)]
         self.nodes[0].p2p.send_and_ping(getdata_request)
         assert_equal(len(self.nodes[0].getpeerinfo()), 1) #node is still connected because of the whitelist
 

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_no_witness_block, msg_no_witness_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_block, msg_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_no_witness_block, msg_no_witness_blocktxn, msg_cmpctblock, msg_getblocktxn, msg_getdata, msg_getheaders, msg_headers, msg_inv, msg_sendcmpct, msg_sendheaders, msg_tx, msg_block, msg_blocktxn, MSG_BLOCK, MSG_CMPCT_BLOCK, 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
@@ -44,7 +44,7 @@ def on_headers(self, message):
 
     def on_inv(self, message):
         for x in self.last_message["inv"].inv:
-            if x.type == 2:
+            if x.type == MSG_BLOCK:
                 self.block_announced = True
                 self.announced_blockhashes.add(x.hash)
 
@@ -307,7 +307,7 @@ def test_compactblock_construction(self, test_node, use_witness_address=True):
         # Now fetch the compact block using a normal non-announce getdata
         with mininode_lock:
             test_node.clear_block_announcement()
-            inv = CInv(4, block_hash)  # 4 == "CompactBlock"
+            inv = CInv(MSG_CMPCT_BLOCK, block_hash)
             test_node.send_message(msg_getdata([inv]))
 
         wait_until(test_node.received_block_announcement, timeout=30, lock=mininode_lock)
@@ -380,7 +380,7 @@ def test_compactblock_requests(self, test_node, segwit=True):
             block = self.build_block_on_tip(node, segwit=segwit)
 
             if announce == "inv":
-                test_node.send_message(msg_inv([CInv(2, block.sha256)]))
+                test_node.send_message(msg_inv([CInv(MSG_BLOCK, block.sha256)]))
                 wait_until(lambda: "getheaders" in test_node.last_message, timeout=30, lock=mininode_lock)
                 test_node.send_header_for_blocks([block])
             else:
@@ -564,7 +564,8 @@ def test_incorrect_blocktxn_response(self, test_node):
 
         # We should receive a getdata request
         test_node.wait_for_getdata([block.sha256], timeout=10)
-        assert test_node.last_message["getdata"].inv[0].type == 2 or test_node.last_message["getdata"].inv[0].type == 2 | MSG_WITNESS_FLAG
+        assert test_node.last_message["getdata"].inv[0].type == MSG_BLOCK or \
+               test_node.last_message["getdata"].inv[0].type == MSG_BLOCK | MSG_WITNESS_FLAG
 
         # Deliver the block
         if version == 2:
@@ -633,7 +634,7 @@ def test_compactblocks_not_at_tip(self, test_node):
             wait_until(test_node.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))]))
+        test_node.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, int(new_blocks[0], 16))]))
         wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30, lock=mininode_lock)
 
         test_node.clear_block_announcement()
@@ -642,7 +643,7 @@ def test_compactblocks_not_at_tip(self, test_node):
         test_node.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))]))
+        test_node.send_message(msg_getdata([CInv(MSG_CMPCT_BLOCK, int(new_blocks[0], 16))]))
         wait_until(lambda: "block" in test_node.last_message, timeout=30, lock=mininode_lock)
         with mininode_lock:
             test_node.last_message["block"].block.calc_sha256()

test/functional/p2p_feefilter.py

@@ -7,7 +7,7 @@
 from decimal import Decimal
 import time
 
-from test_framework.messages import msg_feefilter
+from test_framework.messages import MSG_TX, msg_feefilter
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 
@@ -31,7 +31,7 @@ def __init__(self):
 
     def on_inv(self, message):
         for i in message.inv:
-            if (i.type == 1):
+            if (i.type == MSG_TX):
                 self.txinvs.append(hashToHex(i.hash))
 
     def clear_invs(self):

test/functional/p2p_fingerprint.py

@@ -11,7 +11,7 @@
 import time
 
 from test_framework.blocktools import (create_block, create_coinbase)
-from test_framework.messages import CInv
+from test_framework.messages import CInv, MSG_BLOCK
 from test_framework.mininode import (
     P2PInterface,
     msg_headers,
@@ -48,7 +48,7 @@ def build_chain(self, nblocks, prev_hash, prev_height, prev_median_time):
     # Send a getdata request for a given block hash
     def send_block_request(self, block_hash, node):
         msg = msg_getdata()
-        msg.inv.append(CInv(2, block_hash))  # 2 == "Block"
+        msg.inv.append(CInv(MSG_BLOCK, block_hash))
         node.send_message(msg)
 
     # Send a getheaders request for a given single block hash

test/functional/p2p_invalid_messages.py

@@ -7,7 +7,16 @@
 import struct
 import sys
 
-from test_framework import messages
+from test_framework.messages import (
+    CBlockHeader,
+    CInv,
+    msg_getdata,
+    msg_headers,
+    msg_inv,
+    msg_ping,
+    MSG_TX,
+    ser_string,
+)
 from test_framework.mininode import (
     NetworkThread,
     P2PDataStore,
@@ -25,7 +34,7 @@ def __init__(self, *, str_data):
         self.str_data = str_data.encode() if not isinstance(str_data, bytes) else str_data
 
     def serialize(self):
-        return messages.ser_string(self.str_data)
+        return ser_string(self.str_data)
 
     def __repr__(self):
         return "{}(data={})".format(self.msgtype, self.str_data)
@@ -135,7 +144,7 @@ def run_test(self):
             # For some reason unknown to me, we sometimes have to push additional data to the
             # peer in order for it to realize a disconnect.
             try:
-                node.p2p.send_message(messages.msg_ping(nonce=123123))
+                node.p2p.send_message(msg_ping(nonce=123123))
             except IOError:
                 pass
 
@@ -158,7 +167,7 @@ async def swap_magic_bytes():
         asyncio.run_coroutine_threadsafe(swap_magic_bytes(), NetworkThread.network_event_loop).result()
 
         with self.nodes[0].assert_debug_log(['PROCESSMESSAGE: INVALID MESSAGESTART ping']):
-            conn.send_message(messages.msg_ping(nonce=0xff))
+            conn.send_message(msg_ping(nonce=0xff))
             conn.wait_for_disconnect(timeout=1)
             self.nodes[0].disconnect_p2ps()
 
@@ -206,13 +215,13 @@ def test_msgtype(self):
     def test_large_inv(self):
         conn = self.nodes[0].add_p2p_connection(P2PInterface())
         with self.nodes[0].assert_debug_log(['Misbehaving', 'peer=4 (0 -> 20): message inv size() = 50001']):
-            msg = messages.msg_inv([messages.CInv(1, 1)] * 50001)
+            msg = msg_inv([CInv(MSG_TX, 1)] * 50001)
             conn.send_and_ping(msg)
         with self.nodes[0].assert_debug_log(['Misbehaving', 'peer=4 (20 -> 40): message getdata size() = 50001']):
-            msg = messages.msg_getdata([messages.CInv(1, 1)] * 50001)
+            msg = msg_getdata([CInv(MSG_TX, 1)] * 50001)
             conn.send_and_ping(msg)
         with self.nodes[0].assert_debug_log(['Misbehaving', 'peer=4 (40 -> 60): headers message size = 2001']):
-            msg = messages.msg_headers([messages.CBlockHeader()] * 2001)
+            msg = msg_headers([CBlockHeader()] * 2001)
             conn.send_and_ping(msg)
         self.nodes[0].disconnect_p2ps()
 

test/functional/p2p_leak_tx.py

@@ -4,7 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test that we don't leak txs to inbound peers that we haven't yet announced to"""
 
-from test_framework.messages import msg_getdata, CInv
+from test_framework.messages import msg_getdata, CInv, MSG_TX
 from test_framework.mininode import P2PDataStore
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -37,7 +37,7 @@ def run_test(self):
             txid = gen_node.sendtoaddress(gen_node.getnewaddress(), 0.01)
 
             want_tx = msg_getdata()
-            want_tx.inv.append(CInv(t=1, h=int(txid, 16)))
+            want_tx.inv.append(CInv(t=MSG_TX, h=int(txid, 16)))
             inbound_peer.last_message.pop('notfound', None)
             inbound_peer.send_and_ping(want_tx)
 

test/functional/p2p_node_network_limited.py

@@ -8,7 +8,7 @@
 and that it responds to getdata requests for blocks correctly:
     - send a block within 288 + 2 of the tip
     - disconnect peers who request blocks older than that."""
-from test_framework.messages import CInv, msg_getdata, msg_verack, NODE_NETWORK_LIMITED, NODE_WITNESS
+from test_framework.messages import CInv, MSG_BLOCK, msg_getdata, msg_verack, NODE_NETWORK_LIMITED, NODE_WITNESS
 from test_framework.mininode import P2PInterface, mininode_lock
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
@@ -31,7 +31,7 @@ def wait_for_addr(self, timeout=5):
         wait_until(test_function, timeout=timeout, lock=mininode_lock)
     def send_getdata_for_block(self, blockhash):
         getdata_request = msg_getdata()
-        getdata_request.inv.append(CInv(2, int(blockhash, 16)))
+        getdata_request.inv.append(CInv(MSG_BLOCK, int(blockhash, 16)))
         self.send_message(getdata_request)
 
 class NodeNetworkLimitedTest(BitcoinTestFramework):

test/functional/p2p_segwit.py

@@ -22,6 +22,8 @@
     CTxOut,
     CTxWitness,
     MAX_BLOCK_BASE_SIZE,
+    MSG_BLOCK,
+    MSG_TX,
     MSG_WITNESS_FLAG,
     NODE_NETWORK,
     NODE_WITNESS,
@@ -157,7 +159,7 @@ def on_getdata(self, message):
     def announce_tx_and_wait_for_getdata(self, tx, timeout=60, success=True):
         with mininode_lock:
             self.last_message.pop("getdata", None)
-        self.send_message(msg_inv(inv=[CInv(1, tx.sha256)]))
+        self.send_message(msg_inv(inv=[CInv(MSG_TX, tx.sha256)]))
         if success:
             self.wait_for_getdata([tx.sha256], timeout)
         else:
@@ -173,7 +175,7 @@ def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60):
         if use_header:
             self.send_message(msg)
         else:
-            self.send_message(msg_inv(inv=[CInv(2, block.sha256)]))
+            self.send_message(msg_inv(inv=[CInv(MSG_BLOCK, block.sha256)]))
             self.wait_for_getheaders()
             self.send_message(msg)
         self.wait_for_getdata([block.sha256])
@@ -576,7 +578,7 @@ def test_witness_tx_relay_before_segwit_activation(self):
         # Verify that if a peer doesn't set nServices to include NODE_WITNESS,
         # the getdata is just for the non-witness portion.
         self.old_node.announce_tx_and_wait_for_getdata(tx)
-        assert self.old_node.last_message["getdata"].inv[0].type == 1
+        assert self.old_node.last_message["getdata"].inv[0].type == MSG_TX
 
         # Since we haven't delivered the tx yet, inv'ing the same tx from
         # a witness transaction ought not result in a getdata.
@@ -1310,9 +1312,9 @@ def test_tx_relay_after_segwit_activation(self):
         tx3.wit.vtxinwit[0].scriptWitness.stack = [witness_program]
         # Also check that old_node gets a tx announcement, even though this is
         # a witness transaction.
-        self.old_node.wait_for_inv([CInv(1, tx2.sha256)])  # wait until tx2 was inv'ed
+        self.old_node.wait_for_inv([CInv(MSG_TX, tx2.sha256)])  # wait until tx2 was inv'ed
         test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=True, accepted=True)
-        self.old_node.wait_for_inv([CInv(1, tx3.sha256)])
+        self.old_node.wait_for_inv([CInv(MSG_TX, tx3.sha256)])
 
         # Test that getrawtransaction returns correct witness information
         # hash, size, vsize

test/functional/p2p_sendheaders.py

@@ -92,6 +92,7 @@
     NODE_WITNESS,
     P2PInterface,
     mininode_lock,
+    MSG_BLOCK,
     msg_block,
     msg_getblocks,
     msg_getdata,
@@ -120,7 +121,7 @@ def send_get_data(self, block_hashes):
         """Request data for a list of block hashes."""
         msg = msg_getdata()
         for x in block_hashes:
-            msg.inv.append(CInv(2, x))
+            msg.inv.append(CInv(MSG_BLOCK, x))
         self.send_message(msg)
 
     def send_get_headers(self, locator, hashstop):
@@ -131,7 +132,7 @@ def send_get_headers(self, locator, hashstop):
 
     def send_block_inv(self, blockhash):
         msg = msg_inv()
-        msg.inv = [CInv(2, blockhash)]
+        msg.inv = [CInv(MSG_BLOCK, blockhash)]
         self.send_message(msg)
 
     def send_header_for_blocks(self, new_blocks):