… '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

c743718 test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to #18628. In addition to the hash-functions limit test introduced with commit fa4c29b, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki):

  ad message type `filterload`:
  > The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**.

  ad message type `filteradd`:
  > The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object).

  Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit).

  Also fixes #18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with #18544) below to also use the more commonly used `assert_debug_log` method.

ACKs for top commit:
  MarcoFalke:
    ACK c743718
  robot-visions:
    ACK c743718
  jonasschnelli:
    utACK c743718. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524

Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be

…_filter.py

c743718 test: add further BIP37 size limit checks to p2p_filter.py (Sebastian Falbesoner)

Pull request description:

  This is a follow-up PR to bitcoin#18628. In addition to the hash-functions limit test introduced with commit bitcoin@fa4c29b, it adds checks for the following size limits as defined in [BIP37](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki):

  ad message type `filterload`:
  > The filter itself is simply a bit field of arbitrary byte-aligned size. The maximum size is **36,000 bytes**.

  ad message type `filteradd`:
  > The data field must be smaller than or equal to **520 bytes** in size (the maximum size of any potentially matched object).

  Also introduces new constants for the limits (or reuses the max script size constant in case for the `filteradd` limit).

  Also fixes bitcoin#18711 by changing the misbehaviour check on "filteradd without filterset" (introduced with bitcoin#18544) below to also use the more commonly used `assert_debug_log` method.

ACKs for top commit:
  MarcoFalke:
    ACK c743718
  robot-visions:
    ACK c743718
  jonasschnelli:
    utACK c743718. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524

Tree-SHA512: a03e7639263eb36a381922afb4e1d0ed2ae286f2ad2e7bbd922509a043ddf6cfd08747e01d54d29bfb8f54b66908f653974b9c347e4ca4f43332b586778893be

…rclear')

Summary:
> net: limit BIP37 filter lifespan (active between 'filterload' and 'filterclear')
> Previously, a default match-everything bloom filter was set for every peer,
> i.e. even before receiving a 'filterload' message and after receiving a
> 'filterclear' message code branches checking for the existence of the filter
> by testing the pointer "pfilter" were _always_ executed.

> test: add more inactive filter tests to p2p_filter.py
> check the following expected behaviors if no filter is set:
> -> filtered block requests are ignored by the node
> -> sending a 'filteradd' message is treated as misbehavior
>
> also fixes a bug in the on_inv() callback method, which
> directly modified the type from BLOCK to FILTERED_BLOCK
> in the received 'inv' message rather than just for the reply
>
> Co-authored-by: MarcoFalke <[email protected]>

This is a backport of Core [[bitcoin/bitcoin#18544 | PR18544]]

I had to change the last test to use a different way to detect misbehavior, because banscore was deprecated in D8798 (this would have been applied in [[bitcoin/bitcoin#18672 | PR18672]], if the backports had been done in sequential order)

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8949

…n (active between 'filterload'..'filterclear') (#4043)

* partial backport 18544: net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear')

Previously, a default match-everything bloom filter was set for every peer,
i.e. even before receiving a 'filterload' message and after receiving a
'filterclear' message code branches checking for the existence of the filter
by testing the pointer "pfilter" were _always_ executed.

* net: Match the backport PR a bit more

Co-authored-by: xdustinface <[email protected]>