Merge #19083: test: msg_mempool, fRelay, and other bloomfilter tests

dca73941eb0f0a4c9b68efed3870b536f7dd6cfe scripted-diff: rename node to peer for mininodes (gzhao408)
0474ea25afc65546cbfe5f822c0212bf3e211023 [test] fix race conditions and test in p2p_filter (gzhao408)
4ef80f0827392a1310ca5a29cc1f8f5ca5d16f95 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408)
497a619386008dfaec0db15ecaebcdfaf75f5011 [test] add BIP 37 test for node with fRelay=false (gzhao408)
e8acc6015695c8439fc971a12709468995b96dcf [test] add mempool msg test for node with bloomfilter enabled (gzhao408)

Pull request description:

  This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_:
  1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)).
  2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`.
  3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled.
  4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py.
  5. Fixes race conditions in p2p_filter.py

ACKs for top commit:
  MarcoFalke:
    ACK dca73941eb only changes is restoring accidentally deleted test 🍮
  jonatack:
    ACK dca73941eb0f0a4c9b68efed3870b536f7dd6cfe modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to.

Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3

4 files changed, 165 insertions, 80 deletions

diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
index 15955a938c..5726a73e40 100755
--- a/test/functional/p2p_filter.py
+++ b/test/functional/p2p_filter.py
@@ -16,13 +16,15 @@ from test_framework.messages import (
     msg_filterclear,
     msg_filterload,
     msg_getdata,
+    msg_mempool,
+    msg_version,
 )
-from test_framework.mininode import P2PInterface
+from test_framework.mininode import P2PInterface, mininode_lock
 from test_framework.script import MAX_SCRIPT_ELEMENT_SIZE
 from test_framework.test_framework import BitcoinTestFramework
 
 
-class FilterNode(P2PInterface):
+class P2PBloomFilter(P2PInterface):
     # This is a P2SH watch-only wallet
     watch_script_pubkey = 'a914ffffffffffffffffffffffffffffffffffffffff87'
     # The initial filter (n=10, fp=0.000001) with just the above scriptPubKey added
@@ -34,6 +36,11 @@ class FilterNode(P2PInterface):
         nFlags=1,
     )
 
+    def __init__(self):
+        super().__init__()
+        self._tx_received = False
+        self._merkleblock_received = False
+
     def on_inv(self, message):
         want = msg_getdata()
         for i in message.inv:
@@ -46,10 +53,30 @@ class FilterNode(P2PInterface):
             self.send_message(want)
 
     def on_merkleblock(self, message):
-        self.merkleblock_received = True
+        self._merkleblock_received = True
 
     def on_tx(self, message):
-        self.tx_received = True
+        self._tx_received = True
+
+    @property
+    def tx_received(self):
+        with mininode_lock:
+            return self._tx_received
+
+    @tx_received.setter
+    def tx_received(self, value):
+        with mininode_lock:
+            self._tx_received = value
+
+    @property
+    def merkleblock_received(self):
+        with mininode_lock:
+            return self._merkleblock_received
+
+    @merkleblock_received.setter
+    def merkleblock_received(self, value):
+        with mininode_lock:
+            self._merkleblock_received = value
 
 
 class FilterTest(BitcoinTestFramework):
@@ -64,95 +91,144 @@ class FilterTest(BitcoinTestFramework):
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
-    def test_size_limits(self, filter_node):
+    def test_size_limits(self, filter_peer):
         self.log.info('Check that too large filter is rejected')
         with self.nodes[0].assert_debug_log(['Misbehaving']):
-            filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1)))
+            filter_peer.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1)))
 
         self.log.info('Check that max size filter is accepted')
         with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']):
-            filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE)))
-        filter_node.send_and_ping(msg_filterclear())
+            filter_peer.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE)))
+        filter_peer.send_and_ping(msg_filterclear())
 
         self.log.info('Check that filter with too many hash functions is rejected')
         with self.nodes[0].assert_debug_log(['Misbehaving']):
-            filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1))
+            filter_peer.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1))
 
         self.log.info('Check that filter with max hash functions is accepted')
         with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']):
-            filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS))
+            filter_peer.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS))
         # Don't send filterclear until next two filteradd checks are done
 
         self.log.info('Check that max size data element to add to the filter is accepted')
         with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']):
-            filter_node.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE)))
+            filter_peer.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE)))
 
         self.log.info('Check that too large data element to add to the filter is rejected')
         with self.nodes[0].assert_debug_log(['Misbehaving']):
-            filter_node.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1)))
+            filter_peer.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1)))
 
-        filter_node.send_and_ping(msg_filterclear())
+        filter_peer.send_and_ping(msg_filterclear())
 
-    def run_test(self):
-        filter_node = self.nodes[0].add_p2p_connection(FilterNode())
+    def test_msg_mempool(self):
+        self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
+        filter_peer = P2PBloomFilter()
 
-        self.test_size_limits(filter_node)
+        self.log.info("Create a tx relevant to the peer before connecting")
+        filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0]
+        txid = self.nodes[0].sendtoaddress(filter_address, 90)
 
-        self.log.info('Add filtered P2P connection to the node')
-        filter_node.send_and_ping(filter_node.watch_filter_init)
-        filter_address = self.nodes[0].decodescript(filter_node.watch_script_pubkey)['addresses'][0]
+        self.log.info("Send a mempool msg after connecting and check that the tx is received")
+        self.nodes[0].add_p2p_connection(filter_peer)
+        filter_peer.send_and_ping(filter_peer.watch_filter_init)
+        self.nodes[0].p2p.send_message(msg_mempool())
+        filter_peer.wait_for_tx(txid)
+
+    def test_frelay_false(self, filter_peer):
+        self.log.info("Check that a node with fRelay set to false does not receive invs until the filter is set")
+        filter_peer.tx_received = False
+        filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0]
+        self.nodes[0].sendtoaddress(filter_address, 90)
+        # Sync to make sure the reason filter_peer doesn't receive the tx is not p2p delays
+        filter_peer.sync_with_ping()
+        assert not filter_peer.tx_received
+
+        # Clear the mempool so that this transaction does not impact subsequent tests
+        self.nodes[0].generate(1)
+
+    def test_filter(self, filter_peer):
+        # Set the bloomfilter using filterload
+        filter_peer.send_and_ping(filter_peer.watch_filter_init)
+        # If fRelay is not already True, sending filterload sets it to True
+        assert self.nodes[0].getpeerinfo()[0]['relaytxes']
+        filter_address = self.nodes[0].decodescript(filter_peer.watch_script_pubkey)['addresses'][0]
 
         self.log.info('Check that we receive merkleblock and tx if the filter matches a tx in a block')
         block_hash = self.nodes[0].generatetoaddress(1, filter_address)[0]
         txid = self.nodes[0].getblock(block_hash)['tx'][0]
-        filter_node.wait_for_merkleblock(block_hash)
-        filter_node.wait_for_tx(txid)
+        filter_peer.wait_for_merkleblock(block_hash)
+        filter_peer.wait_for_tx(txid)
 
         self.log.info('Check that we only receive a merkleblock if the filter does not match a tx in a block')
-        filter_node.tx_received = False
+        filter_peer.tx_received = False
         block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
-        filter_node.wait_for_merkleblock(block_hash)
-        assert not filter_node.tx_received
+        filter_peer.wait_for_merkleblock(block_hash)
+        assert not filter_peer.tx_received
 
         self.log.info('Check that we not receive a tx if the filter does not match a mempool tx')
-        filter_node.merkleblock_received = False
-        filter_node.tx_received = False
+        filter_peer.merkleblock_received = False
+        filter_peer.tx_received = False
         self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 90)
-        filter_node.sync_with_ping()
-        filter_node.sync_with_ping()
-        assert not filter_node.merkleblock_received
-        assert not filter_node.tx_received
+        filter_peer.sync_with_ping()
+        filter_peer.sync_with_ping()
+        assert not filter_peer.merkleblock_received
+        assert not filter_peer.tx_received
 
-        self.log.info('Check that we receive a tx in reply to a mempool msg if the filter matches a mempool tx')
-        filter_node.merkleblock_received = False
+        self.log.info('Check that we receive a tx if the filter matches a mempool tx')
+        filter_peer.merkleblock_received = False
         txid = self.nodes[0].sendtoaddress(filter_address, 90)
-        filter_node.wait_for_tx(txid)
-        assert not filter_node.merkleblock_received
+        filter_peer.wait_for_tx(txid)
+        assert not filter_peer.merkleblock_received
 
         self.log.info('Check that after deleting filter all txs get relayed again')
-        filter_node.send_and_ping(msg_filterclear())
+        filter_peer.send_and_ping(msg_filterclear())
         for _ in range(5):
             txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
-            filter_node.wait_for_tx(txid)
+            filter_peer.wait_for_tx(txid)
 
         self.log.info('Check that request for filtered blocks is ignored if no filter is set')
-        filter_node.merkleblock_received = False
-        filter_node.tx_received = False
+        filter_peer.merkleblock_received = False
+        filter_peer.tx_received = False
         with self.nodes[0].assert_debug_log(expected_msgs=['received getdata']):
             block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
-            filter_node.wait_for_inv([CInv(MSG_BLOCK, int(block_hash, 16))])
-            filter_node.sync_with_ping()
-            assert not filter_node.merkleblock_received
-            assert not filter_node.tx_received
+            filter_peer.wait_for_inv([CInv(MSG_BLOCK, int(block_hash, 16))])
+            filter_peer.sync_with_ping()
+            assert not filter_peer.merkleblock_received
+            assert not filter_peer.tx_received
 
         self.log.info('Check that sending "filteradd" if no filter is set is treated as misbehavior')
         with self.nodes[0].assert_debug_log(['Misbehaving']):
-            filter_node.send_and_ping(msg_filteradd(data=b'letsmisbehave'))
+            filter_peer.send_and_ping(msg_filteradd(data=b'letsmisbehave'))
 
         self.log.info("Check that division-by-zero remote crash bug [CVE-2013-5700] is fixed")
-        filter_node.send_and_ping(msg_filterload(data=b'', nHashFuncs=1))
-        filter_node.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode'))
+        filter_peer.send_and_ping(msg_filterload(data=b'', nHashFuncs=1))
+        filter_peer.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode'))
+        self.nodes[0].disconnect_p2ps()
 
+    def run_test(self):
+        filter_peer = self.nodes[0].add_p2p_connection(P2PBloomFilter())
+        self.log.info('Test filter size limits')
+        self.test_size_limits(filter_peer)
+
+        self.log.info('Test BIP 37 for a node with fRelay = True (default)')
+        self.test_filter(filter_peer)
+        self.nodes[0].disconnect_p2ps()
+
+        self.log.info('Test BIP 37 for a node with fRelay = False')
+        # Add peer but do not send version yet
+        filter_peer_without_nrelay = self.nodes[0].add_p2p_connection(P2PBloomFilter(), send_version=False, wait_for_verack=False)
+        # Send version with fRelay=False
+        filter_peer_without_nrelay.wait_until(lambda: filter_peer_without_nrelay.is_connected, timeout=10)
+        version_without_fRelay = msg_version()
+        version_without_fRelay.nRelay = 0
+        filter_peer_without_nrelay.send_message(version_without_fRelay)
+        filter_peer_without_nrelay.wait_for_verack()
+        assert not self.nodes[0].getpeerinfo()[0]['relaytxes']
+        self.test_frelay_false(filter_peer_without_nrelay)
+        self.test_filter(filter_peer_without_nrelay)
+
+        self.log.info('Test msg_mempool')
+        self.test_msg_mempool()
 
 if __name__ == '__main__':
     FilterTest().main()
diff --git a/test/functional/p2p_mempool.py b/test/functional/p2p_mempool.py
deleted file mode 100755
index a8fcb181e6..0000000000
--- a/test/functional/p2p_mempool.py
+++ /dev/null
@@ -1,34 +0,0 @@
-#!/usr/bin/env python3
-# Copyright (c) 2015-2018 The Bitcoin Core developers
-# Distributed under the MIT software license, see the accompanying
-# file COPYING or http://www.opensource.org/licenses/mit-license.php.
-"""Test p2p mempool message.
-
-Test that nodes are disconnected if they send mempool messages when bloom
-filters are not enabled.
-"""
-
-from test_framework.messages import msg_mempool
-from test_framework.mininode import P2PInterface
-from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal
-
-class P2PMempoolTests(BitcoinTestFramework):
-    def set_test_params(self):
-        self.setup_clean_chain = True
-        self.num_nodes = 1
-        self.extra_args = [["-peerbloomfilters=0"]]
-
-    def run_test(self):
-        # Add a p2p connection
-        self.nodes[0].add_p2p_connection(P2PInterface())
-
-        #request mempool
-        self.nodes[0].p2p.send_message(msg_mempool())
-        self.nodes[0].p2p.wait_for_disconnect()
-
-        #mininode must be disconnected at this point
-        assert_equal(len(self.nodes[0].getpeerinfo()), 0)
-
-if __name__ == '__main__':
-    P2PMempoolTests().main()
diff --git a/test/functional/p2p_nobloomfilter_messages.py b/test/functional/p2p_nobloomfilter_messages.py
new file mode 100755
index 0000000000..41af74ebb8
--- /dev/null
+++ b/test/functional/p2p_nobloomfilter_messages.py
@@ -0,0 +1,43 @@
+#!/usr/bin/env python3
+# Copyright (c) 2015-2018 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test invalid p2p messages for nodes with bloom filters disabled.
+
+Test that, when bloom filters are not enabled, nodes are disconnected if:
+1. They send a p2p mempool message
+2. They send a p2p filterload message
+3. They send a p2p filteradd message
+"""
+
+from test_framework.messages import msg_mempool, msg_filteradd, msg_filterload
+from test_framework.mininode import P2PInterface
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+
+class P2PNobloomfilterMessages(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.num_nodes = 1
+        self.extra_args = [["-peerbloomfilters=0"]]
+
+    def test_message_causes_disconnect(self, message):
+        # Add a p2p connection that sends a message and check that it disconnects
+        peer = self.nodes[0].add_p2p_connection(P2PInterface())
+        peer.send_message(message)
+        peer.wait_for_disconnect()
+        assert_equal(len(self.nodes[0].getpeerinfo()), 0)
+
+    def run_test(self):
+        self.log.info("Test that node is disconnected if it sends mempool message")
+        self.test_message_causes_disconnect(msg_mempool())
+
+        self.log.info("Test that node is disconnected if it sends filterload message")
+        self.test_message_causes_disconnect(msg_filterload())
+
+        self.log.info("Test that node is disconnected if it sends filteradd message")
+        self.test_message_causes_disconnect(msg_filteradd(data=b'\xcc'))
+
+if __name__ == '__main__':
+    P2PNobloomfilterMessages().main()
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 0ea65c68b8..c0bc471d5a 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -164,7 +164,7 @@ BASE_SCRIPTS = [
     'wallet_keypool.py',
     'wallet_keypool.py --descriptors',
     'wallet_descriptor.py',
-    'p2p_mempool.py',
+    'p2p_nobloomfilter_messages.py',
     'p2p_filter.py',
     'rpc_setban.py',
     'p2p_blocksonly.py',