Merge #19804: test/refactor: reference p2p objects explicitly and rem…

…ove confusing Test_Node.p2p property

10d6150 [test] remove confusing p2p property (gzhao408)
549d30f scripted-diff: replace p2p with p2ps[0] in p2p_invalid_tx (gzhao408)
7a0de46 [doc] sample code for test framework p2p objects (gzhao408)
784f757 [refactor] clarify tests by referencing p2p objects directly (gzhao408)

Pull request description:

  The `TestNode` has a `p2p` property which is an alias for `p2ps[0]`.

  I think this should be removed because it can be confusing and misleading (to both the test writer and reviewer), especially if a TestNode has multiple p2ps connected (which is the case for many tests).
  Another example is when a test has multiple subtests that connect 1 p2p and use the `p2p` property to reference it. If the subtests don't completely clean up after themselves, the subtests may affect one another.

  The best way to refer to a connected p2p is use the object returned by `add_p2p_connection` like this:
  ```py
  p2p_conn = node.add_p2p_connection(P2PInterface())
  ```
  A good example is [p2p_invalid_locator.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_invalid_locator.py), which cleans up after itself (waits in both `wait_for_disconnect` and in `disconnect_p2ps`) but wouldn't need so much complexity if it just referenced the connections directly.

  If there is only one connected, it's not really that tedious to just use `node.p2ps[0]` instead of `node.p2p` (and it can always be aliased inside the test itself).

ACKs for top commit:
  robot-dreams:
    utACK 10d6150
  jnewbery:
    utACK 10d6150
  guggero:
    Concept ACK 10d6150.

Tree-SHA512: 5965548929794ec660dae03467640cb2156d7d826cefd26d3a126472cbc2494b855c1d26bbb7b412281fbdc92b9798b9765a85c27bc1a97f7798f27f64db6f13

test/functional/README.md

@@ -87,7 +87,9 @@ P2P messages. These can be found in the following source files:
 
 #### Using the P2P interface
 
-- [messages.py](test_framework/messages.py) contains all the definitions for objects that pass
+- `P2P`s can be used to test specific P2P protocol behavior.
+[p2p.py](test_framework/p2p.py) contains test framework p2p objects and
+[messages.py](test_framework/messages.py) contains all the definitions for objects passed
 over the network (`CBlock`, `CTransaction`, etc, along with the network-level
 wrappers for them, `msg_block`, `msg_tx`, etc).
 
@@ -100,8 +102,22 @@ contains the higher level logic for processing P2P payloads and connecting to
 the Bitcoin Core node application logic. For custom behaviour, subclass the
 P2PInterface object and override the callback methods.
 
-- Can be used to write tests where specific P2P protocol behavior is tested.
-Examples tests are [p2p_unrequested_blocks.py](p2p_unrequested_blocks.py),
+`P2PConnection`s can be used as such:
+
+```python
+p2p_conn = node.add_p2p_connection(P2PInterface())
+p2p_conn.send_and_ping(msg)
+```
+
+They can also be referenced by indexing into a `TestNode`'s `p2ps` list, which
+contains the list of test framework `p2p` objects connected to itself
+(it does not include any `TestNode`s):
+
+```python
+node.p2ps[0].sync_with_ping()
+```
+
+More examples can be found in [p2p_unrequested_blocks.py](p2p_unrequested_blocks.py),
 [p2p_compactblocks.py](p2p_compactblocks.py).
 
 #### Prototyping tests
@@ -157,7 +173,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.p2p.send_message(some_large_message)
+        node.p2ps[0].send_message(some_large_message)
 ```
 
 To see useful textual output, run

test/functional/example_test.py

@@ -136,7 +136,7 @@ def run_test(self):
         """Main test logic"""
 
         # Create P2P connections will wait for a verack to make sure the connection is fully up
-        self.nodes[0].add_p2p_connection(BaseNode())
+        peer_messaging = self.nodes[0].add_p2p_connection(BaseNode())
 
         # Generating a block on one of the nodes will get us out of IBD
         blocks = [int(self.nodes[0].generate(nblocks=1)[0], 16)]
@@ -173,7 +173,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
-            self.nodes[0].p2p.send_message(block_message)
+            peer_messaging.send_message(block_message)
             self.tip = block.sha256
             blocks.append(self.tip)
             self.block_time += 1
@@ -191,25 +191,25 @@ def run_test(self):
         self.log.info("Add P2P connection to node2")
         self.nodes[0].disconnect_p2ps()
 
-        self.nodes[2].add_p2p_connection(BaseNode())
+        peer_receiving = self.nodes[2].add_p2p_connection(BaseNode())
 
         self.log.info("Test that node2 propagates all the blocks to us")
 
         getdata_request = msg_getdata()
         for block in blocks:
             getdata_request.inv.append(CInv(MSG_BLOCK, block))
-        self.nodes[2].p2p.send_message(getdata_request)
+        peer_receiving.send_message(getdata_request)
 
         # wait_until() will loop until a predicate condition is met. Use it to test properties of the
         # P2PInterface objects.
-        self.nodes[2].p2p.wait_until(lambda: sorted(blocks) == sorted(list(self.nodes[2].p2p.block_receive_map.keys())), timeout=5)
+        peer_receiving.wait_until(lambda: sorted(blocks) == sorted(list(peer_receiving.block_receive_map.keys())), timeout=5)
 
         self.log.info("Check that each block was received only once")
         # The network thread uses a global lock on data access to the P2PConnection objects when sending and receiving
         # messages. The test thread should acquire the global lock before accessing any P2PConnection data to avoid locking
         # and synchronization issues. Note p2p.wait_until() acquires this global lock internally when testing the predicate.
         with p2p_lock:
-            for block in self.nodes[2].p2p.block_receive_map.values():
+            for block in peer_receiving.block_receive_map.values():
                 assert_equal(block, 1)
 
 

test/functional/feature_block.py

@@ -1386,14 +1386,14 @@ def bootstrap_p2p(self, timeout=10):
         """Add a P2P connection to the node.
 
         Helper to connect and wait for version handshake."""
-        self.nodes[0].add_p2p_connection(P2PDataStore())
+        self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore())
         # We need to wait for the initial getheaders from the peer before we
         # start populating our blockstore. If we don't, then we may run ahead
         # to the next subtest before we receive the getheaders. We'd then send
         # an INV for the next block and receive two getheaders - one for the
         # IBD and one for the INV. We'd respond to both and could get
         # unexpectedly disconnected if the DoS score for that error is 50.
-        self.nodes[0].p2p.wait_for_getheaders(timeout=timeout)
+        self.helper_peer.wait_for_getheaders(timeout=timeout)
 
     def reconnect_p2p(self, timeout=60):
         """Tear down and bootstrap the P2P connection to the node.
@@ -1407,7 +1407,7 @@ def send_blocks(self, blocks, success=True, reject_reason=None, force_send=False
         """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block.
 
         Call with success = False if the tip shouldn't advance to the most recent block."""
-        self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, force_send=force_send, timeout=timeout, expect_disconnect=reconnect)
+        self.helper_peer.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason, force_send=force_send, timeout=timeout, expect_disconnect=reconnect)
 
         if reconnect:
             self.reconnect_p2p(timeout=timeout)

test/functional/feature_cltv.py

@@ -75,7 +75,7 @@ def test_cltv_info(self, *, is_active):
         )
 
     def run_test(self):
-        self.nodes[0].add_p2p_connection(P2PInterface())
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
 
         self.test_cltv_info(is_active=False)
 
@@ -99,7 +99,7 @@ def run_test(self):
         block.solve()
 
         self.test_cltv_info(is_active=False)  # Not active as of current tip and next block does not need to obey rules
-        self.nodes[0].p2p.send_and_ping(msg_block(block))
+        peer.send_and_ping(msg_block(block))
         self.test_cltv_info(is_active=True)  # Not active as of current tip, but next block must obey rules
         assert_equal(self.nodes[0].getbestblockhash(), block.hash)
 
@@ -111,9 +111,9 @@ def run_test(self):
         block.solve()
 
         with self.nodes[0].assert_debug_log(expected_msgs=['{}, bad-version(0x00000003)'.format(block.hash)]):
-            self.nodes[0].p2p.send_and_ping(msg_block(block))
+            peer.send_and_ping(msg_block(block))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
-            self.nodes[0].p2p.sync_with_ping()
+            peer.sync_with_ping()
 
         self.log.info("Test that invalid-according-to-cltv transactions cannot appear in a block")
         block.nVersion = 4
@@ -136,9 +136,9 @@ def run_test(self):
         block.solve()
 
         with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Negative locktime)'.format(block.vtx[-1].hash)]):
-            self.nodes[0].p2p.send_and_ping(msg_block(block))
+            peer.send_and_ping(msg_block(block))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
-            self.nodes[0].p2p.sync_with_ping()
+            peer.sync_with_ping()
 
         self.log.info("Test that a version 4 block with a valid-according-to-CLTV transaction is accepted")
         spendtx = cltv_validate(self.nodes[0], spendtx, CLTV_HEIGHT - 1)
@@ -150,7 +150,7 @@ def run_test(self):
         block.solve()
 
         self.test_cltv_info(is_active=True)  # Not active as of current tip, but next block must obey rules
-        self.nodes[0].p2p.send_and_ping(msg_block(block))
+        peer.send_and_ping(msg_block(block))
         self.test_cltv_info(is_active=True)  # Active as of current tip
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 

test/functional/feature_csv_activation.py

@@ -182,10 +182,10 @@ def send_blocks(self, blocks, success=True, reject_reason=None):
         """Sends blocks to test node. Syncs and verifies that tip has advanced to most recent block.
 
         Call with success = False if the tip shouldn't advance to the most recent block."""
-        self.nodes[0].p2p.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason)
+        self.helper_peer.send_blocks_and_test(blocks, self.nodes[0], success=success, reject_reason=reject_reason)
 
     def run_test(self):
-        self.nodes[0].add_p2p_connection(P2PDataStore())
+        self.helper_peer = self.nodes[0].add_p2p_connection(P2PDataStore())
 
         self.log.info("Generate blocks in the past for coinbase outputs.")
         long_past_time = int(time.time()) - 600 * 1000  # enough to build up to 1000 blocks 10 minutes apart without worrying about getting into the future

test/functional/feature_dersig.py

@@ -59,7 +59,7 @@ def test_dersig_info(self, *, is_active):
         )
 
     def run_test(self):
-        self.nodes[0].add_p2p_connection(P2PInterface())
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
 
         self.test_dersig_info(is_active=False)
 
@@ -84,7 +84,7 @@ def run_test(self):
         block.solve()
 
         self.test_dersig_info(is_active=False)  # Not active as of current tip and next block does not need to obey rules
-        self.nodes[0].p2p.send_and_ping(msg_block(block))
+        peer.send_and_ping(msg_block(block))
         self.test_dersig_info(is_active=True)  # Not active as of current tip, but next block must obey rules
         assert_equal(self.nodes[0].getbestblockhash(), block.hash)
 
@@ -97,9 +97,9 @@ def run_test(self):
         block.solve()
 
         with self.nodes[0].assert_debug_log(expected_msgs=['{}, bad-version(0x00000002)'.format(block.hash)]):
-            self.nodes[0].p2p.send_and_ping(msg_block(block))
+            peer.send_and_ping(msg_block(block))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
-            self.nodes[0].p2p.sync_with_ping()
+            peer.sync_with_ping()
 
         self.log.info("Test that transactions with non-DER signatures cannot appear in a block")
         block.nVersion = 3
@@ -123,9 +123,9 @@ def run_test(self):
         block.solve()
 
         with self.nodes[0].assert_debug_log(expected_msgs=['CheckInputScripts on {} failed with non-mandatory-script-verify-flag (Non-canonical DER signature)'.format(block.vtx[-1].hash)]):
-            self.nodes[0].p2p.send_and_ping(msg_block(block))
+            peer.send_and_ping(msg_block(block))
             assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
-            self.nodes[0].p2p.sync_with_ping()
+            peer.sync_with_ping()
 
         self.log.info("Test that a version 3 block with a DERSIG-compliant transaction is accepted")
         block.vtx[1] = create_transaction(self.nodes[0], self.coinbase_txids[1], self.nodeaddress, amount=1.0)
@@ -134,7 +134,7 @@ def run_test(self):
         block.solve()
 
         self.test_dersig_info(is_active=True)  # Not active as of current tip, but next block must obey rules
-        self.nodes[0].p2p.send_and_ping(msg_block(block))
+        peer.send_and_ping(msg_block(block))
         self.test_dersig_info(is_active=True)  # Active as of current tip
         assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.sha256)
 

test/functional/feature_maxuploadtarget.py

@@ -145,16 +145,16 @@ def run_test(self):
         self.restart_node(0, ["-whitelist=download@127.0.0.1", "-maxuploadtarget=1"])
 
         # Reconnect to self.nodes[0]
-        self.nodes[0].add_p2p_connection(TestP2PConn())
+        peer = self.nodes[0].add_p2p_connection(TestP2PConn())
 
         #retrieve 20 blocks which should be enough to break the 1MB limit
         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)
+            peer.send_and_ping(getdata_request)
+            assert_equal(peer.block_receive_map[big_new_block], i+1)
 
         getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)]
-        self.nodes[0].p2p.send_and_ping(getdata_request)
+        peer.send_and_ping(getdata_request)
 
         self.log.info("Peer still connected after trying to download old block (download permission)")
         peer_info = self.nodes[0].getpeerinfo()

test/functional/feature_versionbits_warning.py

@@ -61,23 +61,23 @@ def versionbits_in_alert_file(self):
 
     def run_test(self):
         node = self.nodes[0]
-        node.add_p2p_connection(P2PInterface())
+        peer = node.add_p2p_connection(P2PInterface())
 
         node_deterministic_address = node.get_deterministic_priv_key().address
         # Mine one period worth of blocks
         node.generatetoaddress(VB_PERIOD, node_deterministic_address)
 
         self.log.info("Check that there is no warning if previous VB_BLOCKS have <VB_THRESHOLD blocks with unknown versionbits version.")
         # Build one period of blocks with < VB_THRESHOLD blocks signaling some unknown bit
-        self.send_blocks_with_version(node.p2p, VB_THRESHOLD - 1, VB_UNKNOWN_VERSION)
+        self.send_blocks_with_version(peer, VB_THRESHOLD - 1, VB_UNKNOWN_VERSION)
         node.generatetoaddress(VB_PERIOD - VB_THRESHOLD + 1, node_deterministic_address)
 
         # Check that we're not getting any versionbit-related errors in get*info()
         assert not VB_PATTERN.match(node.getmininginfo()["warnings"])
         assert not VB_PATTERN.match(node.getnetworkinfo()["warnings"])
 
         # Build one period of blocks with VB_THRESHOLD blocks signaling some unknown bit
-        self.send_blocks_with_version(node.p2p, VB_THRESHOLD, VB_UNKNOWN_VERSION)
+        self.send_blocks_with_version(peer, VB_THRESHOLD, VB_UNKNOWN_VERSION)
         node.generatetoaddress(VB_PERIOD - VB_THRESHOLD, node_deterministic_address)
 
         self.log.info("Check that there is a warning if previous VB_BLOCKS have >=VB_THRESHOLD blocks with unknown versionbits version.")

test/functional/mempool_packages.py

@@ -58,7 +58,7 @@ def chain_transaction(self, node, parent_txid, vout, value, fee, num_outputs):
 
     def run_test(self):
         # Mine some blocks and have them mature.
-        self.nodes[0].add_p2p_connection(P2PTxInvStore()) # keep track of invs
+        peer_inv_store = self.nodes[0].add_p2p_connection(P2PTxInvStore()) # keep track of invs
         self.nodes[0].generate(101)
         utxo = self.nodes[0].listunspent(10)
         txid = utxo[0]['txid']
@@ -80,7 +80,7 @@ def run_test(self):
 
         # Wait until mempool transactions have passed initial broadcast (sent inv and received getdata)
         # Otherwise, getrawmempool may be inconsistent with getmempoolentry if unbroadcast changes in between
-        self.nodes[0].p2p.wait_for_broadcast(witness_chain)
+        peer_inv_store.wait_for_broadcast(witness_chain)
 
         # Check mempool has MAX_ANCESTORS transactions in it, and descendant and ancestor
         # count and fees should look correct

test/functional/mining_basic.py

@@ -234,9 +234,9 @@ def chain_tip(b_hash, *, status='headers-only', branchlen=1):
         assert_raises_rpc_error(-25, 'time-too-old', lambda: node.submitheader(hexdata=CBlockHeader(bad_block_time).serialize().hex()))
 
         # Should ask for the block from a p2p node, if they announce the header as well:
-        node.add_p2p_connection(P2PDataStore())
-        node.p2p.wait_for_getheaders(timeout=5)  # Drop the first getheaders
-        node.p2p.send_blocks_and_test(blocks=[block], node=node)
+        peer = node.add_p2p_connection(P2PDataStore())
+        peer.wait_for_getheaders(timeout=5)  # Drop the first getheaders
+        peer.send_blocks_and_test(blocks=[block], node=node)
         # Must be active now:
         assert chain_tip(block.hash, status='active', branchlen=0) in node.getchaintips()
 

test/functional/p2p_blocksonly.py

@@ -17,7 +17,7 @@ def set_test_params(self):
         self.extra_args = [["-blocksonly"]]
 
     def run_test(self):
-        self.nodes[0].add_p2p_connection(P2PInterface())
+        block_relay_peer = self.nodes[0].add_p2p_connection(P2PInterface())
 
         self.log.info('Check that txs from p2p are rejected and result in disconnect')
         prevtx = self.nodes[0].getblock(self.nodes[0].getblockhash(1), 2)['tx'][0]
@@ -41,20 +41,20 @@ def run_test(self):
         )['hex']
         assert_equal(self.nodes[0].getnetworkinfo()['localrelay'], False)
         with self.nodes[0].assert_debug_log(['transaction sent in violation of protocol peer=0']):
-            self.nodes[0].p2p.send_message(msg_tx(FromHex(CTransaction(), sigtx)))
-            self.nodes[0].p2p.wait_for_disconnect()
+            block_relay_peer.send_message(msg_tx(FromHex(CTransaction(), sigtx)))
+            block_relay_peer.wait_for_disconnect()
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 0)
 
         # Remove the disconnected peer and add a new one.
         del self.nodes[0].p2ps[0]
-        self.nodes[0].add_p2p_connection(P2PInterface())
+        tx_relay_peer = self.nodes[0].add_p2p_connection(P2PInterface())
 
         self.log.info('Check that txs from rpc are not rejected and relayed to other peers')
         assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], True)
         txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid']
         with self.nodes[0].assert_debug_log(['received getdata for: wtx {} peer=1'.format(txid)]):
             self.nodes[0].sendrawtransaction(sigtx)
-            self.nodes[0].p2p.wait_for_tx(txid)
+            tx_relay_peer.wait_for_tx(txid)
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)
 
         self.log.info('Check that txs from peers with relay-permission are not rejected and relayed to others')