Merge bitcoin#18544: net: limit BIP37 filter lifespan (active between…

… 'filterload'..'filterclear')

a9ecbdf test: add more inactive filter tests to p2p_filter.py (Sebastian Falbesoner)
5eae034 net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear') (Sebastian Falbesoner)

Pull request description:

  This PR fixes bitcoin#18483. On the master branch, there is currently _always_ a BIP37 filter set for every peer: if not a specific filter is set through a `filterload` message, a default match-everything filter is instanciated and pointed to via the `CBloomFilter` default constructor; that happens both initially, when the containing structure `TxRelay` is constructed:

  https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net.h#L812

  and after a loaded filter is removed again through a `filterclear` message:

  https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3201

  The behaviour was introduced by commit bitcoin@37c6389 (an intentional covert fix for [CVE-2013-5700](bitcoin#18515), according to gmaxwell).

  This default match-everything filter leads to some unintended side-effects:
  1. `getdata` request for filtered blocks (i.e. type `MSG_FILTERED_BLOCK`) are always responded to with `merkleblock`s, even if no filter was set by the peer, see issue bitcoin#18483 (strictly speaking, this is a violation of BIP37) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L1504-L1507
  2. if a peer sends a `filteradd` message without having loaded a filter via `filterload` before, the intended increasing of the banscore never happens (triggered if `bad` is set to true, a few lines below) https://github.com/bitcoin/bitcoin/blob/c0b389b33516fb3eaaad7c30bd11dba768882a7e/src/net_processing.cpp#L3182-L3186

  This PR basically activates the `else`-branch code paths for all checks of `pfilter` again (on the master branch, they are dead code) by limiting the pointer's lifespan: instead of always having a filter set, the `pfilter` is only pointing to a `CBloomFilter`-instance after receiving a `filterload` message and the instance is destroyed again (and the pointer nullified) after receiving a `filterclear` message.

  Here is a before/after comparison in behaviour:
  | code part / scenario                          |    master branch                   |   PR branch                                          |
  | --------------------------------------------- | ---------------------------------- | ---------------------------------------------------- |
  | `getdata` processing for `MSG_FILTERED_BLOCK` | always responds with `merkleblock` | only responds if filter was set via `filterload`     |
  | `filteradd` processing, no filter was loaded  | nothing                            | peer's banscore increases by 100 (i.e. disconnect)   |

  On the other code parts where `pfilter` is checked there is no change in the logic behaviour (except that `CBloomFilter::IsRelevantAndUpdate()` is unnecessarily called and immediately returned in the master branch).
  Note that the default constructor of `CBloomFilter` is only used for deserializing the received `filterload` message and nowhere else. The PR also contains a functional test checking that sending `getdata` for filtered blocks is ignored by the node if no bloom filter is set.

ACKs for top commit:
  MarcoFalke:
    re-ACK a9ecbdf, only change is in test code 🕙

Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6

src/net.h

@@ -1158,7 +1158,6 @@ class CNode
     std::atomic<int64_t> nLastTXTime{0};
 
     struct TxRelay {
-        TxRelay() { }
         mutable RecursiveMutex cs_filter;
         // We use fRelayTxes for two purposes -
         // a) it allows us to not relay tx invs before receiving the peer's version message

test/functional/p2p_filter.py

@@ -7,15 +7,17 @@
 """
 
 from test_framework.messages import (
+    CInv,
     MSG_BLOCK,
     MSG_FILTERED_BLOCK,
-    msg_getdata,
-    msg_filterload,
     msg_filteradd,
     msg_filterclear,
+    msg_filterload,
+    msg_getdata,
 )
 from test_framework.mininode import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
 
 
 class FilterNode(P2PInterface):
@@ -35,8 +37,9 @@ def on_inv(self, message):
         for i in message.inv:
             # inv messages can only contain TX or BLOCK, so translate BLOCK to FILTERED_BLOCK
             if i.type == MSG_BLOCK:
-                i.type = MSG_FILTERED_BLOCK
-            want.inv.append(i)
+                want.inv.append(CInv(MSG_FILTERED_BLOCK, i.hash))
+            else:
+                want.inv.append(i)
         if len(want.inv):
             self.send_message(want)
 
@@ -136,6 +139,21 @@ def run_test(self):
             txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 7)
             filter_node.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
+        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
+
+        self.log.info('Check that sending "filteradd" if no filter is set is treated as misbehavior (+100)')
+        assert_equal(self.nodes[0].getpeerinfo()[0]['banscore'], 0)
+        filter_node.send_and_ping(msg_filteradd(data=b'letsmisbehave'))
+        assert_equal(self.nodes[0].getpeerinfo()[0]['banscore'], 100)
+
         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'))

test/functional/test_framework/mininode.py

@@ -523,19 +523,18 @@ def test_function():
 
         self.wait_until(test_function, timeout=timeout)
 
-    # TODO: enable when p2p_filter.py is backported
-    # def wait_for_inv(self, expected_inv, timeout=60):
-    #     """Waits for an INV message and checks that the first inv object in the message was as expected."""
-    #     if len(expected_inv) > 1:
-    #         raise NotImplementedError("wait_for_inv() will only verify the first inv object")
-
-    #     def test_function():
-    #         assert self.is_connected
-    #         return self.last_message.get("inv") and \
-    #                             self.last_message["inv"].inv[0].type == expected_inv[0].type and \
-    #                             self.last_message["inv"].inv[0].hash == expected_inv[0].hash
-
-    #    self.wait_until(test_function, timeout=timeout)
+    def wait_for_inv(self, expected_inv, timeout=60):
+        """Waits for an INV message and checks that the first inv object in the message was as expected."""
+        if len(expected_inv) > 1:
+            raise NotImplementedError("wait_for_inv() will only verify the first inv object")
+
+        def test_function():
+            assert self.is_connected
+            return self.last_message.get("inv") and \
+                                self.last_message["inv"].inv[0].type == expected_inv[0].type and \
+                                self.last_message["inv"].inv[0].hash == expected_inv[0].hash
+
+        self.wait_until(test_function, timeout=timeout)
 
     def wait_for_verack(self, timeout=60):
         def test_function():