Skip to content

Conversation

@theStack
Copy link
Contributor

@theStack theStack commented Apr 16, 2020

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:

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.

@laanwj laanwj added the Tests label Apr 16, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 16, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Copy link
Contributor

@robot-visions robot-visions left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack 1a58bd41f708dcfe9a21bc1aedab5daac629db88

Thanks for adding this! p2p_filter.py passes for me locally with your changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nit (please feel free to ignore): Would it make sense to also add checks like this?

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'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS, nTweak=0, nFlags=1))
with self.nodes[0].assert_debug_log([], unexpected_msgs=['Misbehaving']):
    filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE)))

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)))

also unified method of detecting misbehaviour
(using assert_debug_log instead of checking peer's banscore)
@theStack theStack force-pushed the 20200416-test-add-further-bloom-filter-size-limit-checks branch from 1a58bd4 to c743718 Compare April 20, 2020 16:23
@theStack
Copy link
Contributor Author

Rebased and changed the "filteradd if no filter is set" misbehaviour test to also use assert_debug_log, like the rest of the test. Fixes the following fail currently happening on master branch:

2020-04-20T16:24:43.175000Z TestFramework (INFO): Check that sending "filteradd" if no filter is set is treated as misbehavior (+100)
2020-04-20T16:24:43.178000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/test_framework.py", line 112, in main
    self.run_test()
  File "./p2p_filter.py", line 120, in run_test
    assert_equal(self.nodes[0].getpeerinfo()[0]['banscore'], 0)
  File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/util.py", line 46, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(100 == 0)
2020-04-20T16:24:43.230000Z TestFramework (INFO): Stopping nodes

@maflcko
Copy link
Member

maflcko commented Apr 20, 2020

ACK c743718

@robot-visions
Copy link
Contributor

ACK c743718

Thanks for updating! This still passes locally for me, and I confirmed that filter_node.send_and_ping(msg_filteradd(data=b'letsmisbehave')) does in fact still increase the banscore.

@jonasschnelli
Copy link
Contributor

@maflcko maflcko merged commit 4ad6144 into bitcoin:master Apr 21, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 23, 2020
…_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
maflcko pushed a commit that referenced this pull request Apr 29, 2020
…r.py

cd543d9 test: check misbehavior more independently in p2p_filter.py (Danny Lee)

Pull request description:

  This expands on #18672 in two ways:

  - Check positive cases (`filterload` accepted, `filteradd` accepted) in addition to the negative cases added in #18672
  - Address MarcoFalke 's [suggestion](#18672 (comment)) to successfully load a filter before testing `filteradd`

ACKs for top commit:
  theStack:
    re-ACK cd543d9

Tree-SHA512: f82402f6287ccddf08b38b6432d5e2b2b2ef528802a981d04c24bac459022f732d9090d4849d72d3d1eb2c757161dcb18c4c036b6e11dc80114e9cd49f21c3bd
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 2, 2020
…p_filter.py

cd543d9 test: check misbehavior more independently in p2p_filter.py (Danny Lee)

Pull request description:

  This expands on bitcoin#18672 in two ways:

  - Check positive cases (`filterload` accepted, `filteradd` accepted) in addition to the negative cases added in bitcoin#18672
  - Address MarcoFalke 's [suggestion](bitcoin#18672 (comment)) to successfully load a filter before testing `filteradd`

ACKs for top commit:
  theStack:
    re-ACK bitcoin@cd543d9

Tree-SHA512: f82402f6287ccddf08b38b6432d5e2b2b2ef528802a981d04c24bac459022f732d9090d4849d72d3d1eb2c757161dcb18c4c036b6e11dc80114e9cd49f21c3bd
@theStack theStack deleted the 20200416-test-add-further-bloom-filter-size-limit-checks branch December 1, 2020 09:55
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 19, 2021
…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
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 19, 2021
Summary:
> This is a follow-up PR to [[bitcoin/bitcoin#18628 | PR18628]] (D8929). In addition to the hash-functions limit test introduced with commit fa4c29b, it adds checks for the following size limits as defined in BIP37:
>
> 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).

This is a backport of Core [[bitcoin/bitcoin#18672 | PR18672]]
Depends on D8949

Test Plan: `ninja && test/functional/test_runner.py p2p_filter`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8950
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 28, 2021
…lter.py

Summary:
cd543d9193ac1882c1b4a8a84e3ac7356a8b7ce9 test: check misbehavior more independently in p2p_filter.py (Danny Lee)

Pull request description:

  This expands on #18672 in two ways:

  - Check positive cases (`filterload` accepted, `filteradd` accepted) in addition to the negative cases added in #18672
  - Address MarcoFalke 's [suggestion](bitcoin/bitcoin#18672 (comment)) to successfully load a filter before testing `filteradd`

---

Backport of Core [[bitcoin/bitcoin#18726 | PR18726]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9092
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Regression introduced today with p2p_filter.py?

6 participants