-
Notifications
You must be signed in to change notification settings - Fork 38.7k
test: add further BIP37 size limit checks to p2p_filter.py #18672
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: add further BIP37 size limit checks to p2p_filter.py #18672
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
robot-visions
left a comment
There was a problem hiding this 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.
test/functional/p2p_filter.py
Outdated
There was a problem hiding this comment.
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)
1a58bd4 to
c743718
Compare
|
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: |
|
ACK c743718 |
|
ACK c743718 Thanks for updating! This still passes locally for me, and I confirmed that |
…_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
…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
…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
…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
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
…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
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:ad message type
filteradd:Also introduces new constants for the limits (or reuses the max script size constant in case for the
filteraddlimit).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_logmethod.