test: add further BIP37 size limit checks to p2p_filter.py #18672

pull theStack wants to merge 1 commits into bitcoin:master from theStack:20200416-test-add-further-bloom-filter-size-limit-checks changing 2 files +15 −5

theStack commented at 5:07 PM on April 16, 2020: member

This is a follow-up PR to #18628. In addition to the hash-functions limit test introduced with commit https://github.com/bitcoin/bitcoin/pull/18628/commits/fa4c29bc1d2425f861845bae4f3816d9817e622a, 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 added the label Tests on Apr 16, 2020
DrahtBot commented at 7:58 PM on April 16, 2020: member



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



Conflicts

No conflicts as of last run.
in test/functional/p2p_filter.py:75 in 1a58bd41f7 outdated
```
  66 | @@ -64,7 +67,13 @@ def run_test(self):
  67 |  
  68 |          self.log.info('Check that too large filter is rejected')
  69 |          with self.nodes[0].assert_debug_log(['Misbehaving']):
  70 | -            filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=51, nTweak=0, nFlags=1))
  71 | +            filter_node.send_and_ping(msg_filterload(data=b'\xaa', nHashFuncs=MAX_BLOOM_HASH_FUNCS+1, nTweak=0, nFlags=1))
  72 | +        with self.nodes[0].assert_debug_log(['Misbehaving']):
  73 | +            filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1)))
```
robot-visions commented at 5:23 PM on April 18, 2020:

Optional nit (please feel free to ignore): Would it make sense to either pass in nHashFuncs=1 or change the default nHashFuncs from 0 to 1?

theStack commented at 4:25 PM on April 20, 2020:

Thanks for your review! I think it's fine to keep the default parameters of the msg_filterload constructor as they are -- they are in line with the equivalent of the CBloomFilter() constructor:

https://github.com/bitcoin/bitcoin/blob/fc00e651e407200da1eb0ae61f6448e22d1b6d8d/src/bloom.h#L67

What I did though was removing the explicit parameters for the first test case, as nTweak and nFlags are irrelevant for filters that won't even get accepted by the node, hence setting them doesn't improve the test.

in test/functional/p2p_filter.py:71 in 1a58bd41f7 outdated

  66 | @@ -64,7 +67,13 @@ def run_test(self):
  67 |  
  68 |          self.log.info('Check that too large filter is rejected')

robot-visions commented at 5:32 PM on April 18, 2020:

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

robot-visions commented at 5:34 PM on April 18, 2020: contributor

Ack 1a58bd41f708dcfe9a21bc1aedab5daac629db88

Thanks for adding this! p2p_filter.py passes for me locally with your changes.
DrahtBot added the label Needs rebase on Apr 20, 2020

test: add further BIP37 size limit checks to p2p_filter.py

also unified method of detecting misbehaviour
(using assert_debug_log instead of checking peer's banscore)

c743718558

theStack force-pushed on Apr 20, 2020

theStack commented at 4:26 PM on April 20, 2020: member

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

MarcoFalke commented at 6:20 PM on April 20, 2020: member

ACK c7437185589926ec8def2af6bede6a407b3d2e4a
DrahtBot removed the label Needs rebase on Apr 20, 2020
robot-visions commented at 9:15 PM on April 20, 2020: contributor

ACK c7437185589926ec8def2af6bede6a407b3d2e4a

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 commented at 8:29 AM on April 21, 2020: contributor

utACK c7437185589926ec8def2af6bede6a407b3d2e4a. Seems to fix it: https://bitcoinbuilds.org/index.php?build=2524
in test/functional/p2p_filter.py:79 in c743718558
```
  75 | +        with self.nodes[0].assert_debug_log(['Misbehaving']):
  76 | +            filter_node.send_and_ping(msg_filterload(data=b'\xbb'*(MAX_BLOOM_FILTER_SIZE+1)))
  77 | +
  78 | +        self.log.info('Check that too large data element to add to the filter is rejected')
  79 | +        with self.nodes[0].assert_debug_log(['Misbehaving']):
  80 | +            filter_node.send_and_ping(msg_filteradd(data=b'\xcc'*(MAX_SCRIPT_ELEMENT_SIZE+1)))
```
MarcoFalke commented at 11:27 AM on April 21, 2020:

I think you need to successfully load a filter first to test this condition?

robot-visions commented at 6:38 PM on April 21, 2020:

Good point! Sorry I missed this when reviewing. I added this in #18726

theStack commented at 7:55 AM on April 22, 2020:

@MarcoFalke: You are right, the misbehaviour is caused for another reasons here, the test is basically equivalent to "'Check that sending "filteradd" if no filter is set is treated as misbehavior'" below. @robot-visions: Thanks for adding the fix!
MarcoFalke merged this on Apr 21, 2020
MarcoFalke closed this on Apr 21, 2020
sidhujag referenced this in commit f398e24e39 on Apr 23, 2020
MarcoFalke referenced this in commit 36c0abd8f6 on Apr 29, 2020
sidhujag referenced this in commit 44a764d706 on May 2, 2020
theStack deleted the branch on Dec 1, 2020
Fabcien referenced this in commit 2e6177c0ef on Jan 19, 2021
deadalnix referenced this in commit 197d07192c on Jan 19, 2021
Fabcien referenced this in commit a2a9c5790d on Jan 28, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18544 net: limit BIP37 filter lifespan (active between 'filterload'..'filterclear')#18628 test: Add various low-level p2p tests #18711 Regression introduced today with p2p_filter.py?#18726 test: check misbehavior more independently in p2p_filter.py

test: add further BIP37 size limit checks to p2p_filter.py #18672

Conflicts