Merge #18544: net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear')

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

Pull request description:

  This PR fixes https://github.com/bitcoin/bitcoin/issues/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 https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 (an intentional covert fix for [CVE-2013-5700](https://github.com/bitcoin/bitcoin/pull/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 #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 a9ecbdfcaa, only change is in test code 🕙

Tree-SHA512: 1a656a6d74ccaf628e7fdca063ba63fbab2089e0b6d0a11be9bbd387c2ee6d3230706ff8ffc1a55711481df3d4547137dd7c9d9184d89eaa43ade4927792d0b6

1 files changed, 1 insertions, 2 deletions

diff --git a/src/net.h b/src/net.h
index 0dbd5e0549..cec31bfd80 100644
--- a/src/net.h
+++ b/src/net.h
@@ -809,14 +809,13 @@ public:
     RecursiveMutex cs_inventory;
 
     struct TxRelay {
-        TxRelay() { pfilter = MakeUnique<CBloomFilter>(); }
         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
         // b) the peer may tell us in its version message that we should not relay tx invs
         //    unless it loads a bloom filter.
         bool fRelayTxes GUARDED_BY(cs_filter){false};
-        std::unique_ptr<CBloomFilter> pfilter PT_GUARDED_BY(cs_filter) GUARDED_BY(cs_filter);
+        std::unique_ptr<CBloomFilter> pfilter PT_GUARDED_BY(cs_filter) GUARDED_BY(cs_filter){nullptr};
 
         mutable RecursiveMutex cs_tx_inventory;
         CRollingBloomFilter filterInventoryKnown GUARDED_BY(cs_tx_inventory){50000, 0.000001};