test: msg_mempool, fRelay, and other bloomfilter tests by glozow · Pull Request #19083 · bitcoin/bitcoin

glozow · 2020-05-27T22:03:50Z

This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned off:

Tests p2p message msg_mempool: a node that has peerbloomfilters enabled should send its mempool (disabled behavior already tested here).
Tests that bloomfilter peers with fRelay=False in the version message should not receive any invs until they set the filter. The rest is the same as what’s already tested in p2p_filter.py.
Tests that peers get disconnected if they send filterload or filteradd p2p messages to a node with bloom filters disabled.
Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py.
Fixes race conditions in p2p_filter.py

DrahtBot · 2020-05-28T05:00:51Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
tests: Update more tests to work with descriptor wallets #18788 (wallet: tests: Update more tests to work with descriptor wallets by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery

Concept ACK. I think you can probably merge all the tests for bloom filters enabled and disabled into a single test file.

test/functional/p2p_filter.py

maflcko

Concept ACK

test/functional/p2p_filter.py

jnewbery

Looks good. A few comments inline.

test/functional/p2p_filter.py

maflcko · 2020-06-06T02:09:34Z

nit: commit f78730d582c605882e5a3bf554aefb1a92942b0e can be done as a scripted diff, but no big deal

glozow · 2020-06-07T17:44:55Z

Resolves #18473 . Ready for Review again!

maflcko

ACK 1e64440490 looks like an improvement, but I have some nits to consider 🍬

Show signature and timestamp

Signature:

Timestamp of file with hash 88b89b80c24192ef59fea53c027a40ed6e6fe7c8d6795036ca7991f3fb4d7971 -

test/functional/p2p_filter.py

maflcko

Concept ACK b758749173

test/functional/p2p_filter.py

-msg_mempool is currently only tested with bloomfilter disabled (node is disconnected) in p2p_mempool.py -msg_mempool should get mempool txns in response when bloomfilter is enabled -edit test that doesn't test msg_mempool as intended

A node with fRelay=False should not receive any invs until filter is set using filterload; all other behavior should be identical.

…nect -A node with bloomfilters disabled should disconnect peers that send msg_mempool, msg_filterload, or msg_filteradd. -Renamed the test because it now has a wider scope and msg_mempool's actual functionality makes more sense for p2p_filter.py.

-grab mininode_lock for every access to mininode attributes, otherwise there are race conditions

-BEGIN VERIFY SCRIPT- sed -i 's/FilterNode/P2PBloomFilter/g' test/functional/p2p_filter.py; sed -i 's/filter_node/filter_peer/g' test/functional/p2p_filter.py; -END VERIFY SCRIPT-

maflcko · 2020-06-10T17:34:59Z

ACK dca7394 only changes is restoring accidentally deleted test 🍮

Show signature and timestamp

Signature:

Timestamp of file with hash 70bc0be0f61503b76003fe460f747227c9eb065aff2dd334d2640b7467d69650 -

glozow · 2020-06-10T17:40:14Z

Thanks for the thorough review @MarcoFalke 🙏 , greatly appreciated.

jonatack

ACK dca7394 modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to.

test/functional/p2p_filter.py

jonatack · 2020-06-11T14:25:43Z

test/functional/p2p_filter.py

+        self.test_filter(filter_peer)
+        self.nodes[0].disconnect_p2ps()
+
+        self.log.info('Test BIP 37 for a node with fRelay = False')


497a619 perhaps put the BIP37 tests into their own function

They're mostly in their own function test_filter, so I left this one as is.

test/functional/p2p_nobloomfilter_messages.py

maflcko · 2020-06-11T14:44:05Z

With two ACKs, this test change seems ready to merge. @gzhao408 Let me know if you want to address the feedback or want to leave it as is for now (for potential follow-ups) and have the pull request merged.

glozow · 2020-06-11T18:05:59Z

Thank you for the review @jonatack 🙏 !
I've made a followup #19252 for disconnect_p2ps to wait for disconnect to complete anyway, so I can easily incorporate the feedback there. @MarcoFalke green light to merge :) thanks

… tests dca7394 scripted-diff: rename node to peer for mininodes (gzhao408) 0474ea2 [test] fix race conditions and test in p2p_filter (gzhao408) 4ef80f0 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408) 497a619 [test] add BIP 37 test for node with fRelay=false (gzhao408) e8acc60 [test] add mempool msg test for node with bloomfilter enabled (gzhao408) Pull request description: This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_: 1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)). 2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`. 3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled. 4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py. 5. Fixes race conditions in p2p_filter.py ACKs for top commit: MarcoFalke: ACK dca7394 only changes is restoring accidentally deleted test 🍮 jonatack: ACK dca7394 modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to. Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3

…ter test followups 9a40cfc [refactor] use waiting inside disconnect_p2ps (gzhao408) aeb9fb4 [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408) e81942d [test] logging and style followups for bloomfilter tests (gzhao408) Pull request description: Followup to #19083 which adds bloomfilter-related tests. 1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](#19083 (comment)). And clean up any redundant `wait_until`s in the functional tests. 2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](#19083 (review)) ACKs for top commit: jonatack: Code review ACK 9a40cfc from re-reviewing the diff and `git range-diff 5cafb46 8386ad5 9a40cfc` MarcoFalke: ACK 9a40cfc 🐂 Tree-SHA512: 2e14b1c12fc08a355bd5ccad7a2a734a4ccda4bc7dc7bac171cb57359819fc1599d764290729af74832fac3e2be258c5d406c701e78ab6d7262835859b9a7d87

Summary: dca73941eb0f0a4c9b68efed3870b536f7dd6cfe scripted-diff: rename node to peer for mininodes (gzhao408) 0474ea25afc65546cbfe5f822c0212bf3e211023 [test] fix race conditions and test in p2p_filter (gzhao408) 4ef80f0827392a1310ca5a29cc1f8f5ca5d16f95 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408) 497a619386008dfaec0db15ecaebcdfaf75f5011 [test] add BIP 37 test for node with fRelay=false (gzhao408) e8acc6015695c8439fc971a12709468995b96dcf [test] add mempool msg test for node with bloomfilter enabled (gzhao408) Pull request description: This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_: 1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)). 2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`. 3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled. 4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py. 5. Fixes race conditions in p2p_filter.py --- Backport of [[bitcoin/bitcoin#19083 | core#19083]] Test Plan: ninja all ./test/test_runner.py p2p_filter p2p_nobloomfilter_messages Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9511

…filter test followups Summary: 9a40cfc558b3f7fa4fff1270f969582af17479a5 [refactor] use waiting inside disconnect_p2ps (gzhao408) aeb9fb414e2d000830287d9dd3fed7fc2eb570d2 [test] wait for disconnect_p2ps to be reflected in getpeerinfo (gzhao408) e81942d2e1288367e8da94adb2b2a88be99e4751 [test] logging and style followups for bloomfilter tests (gzhao408) Pull request description: Followup to #19083 which adds bloomfilter-related tests. 1. Make test_node `disconnect_p2ps` wait until disconnection is complete to avoid race conditions (and not place the burden on tests) from MarcoFalke's [comment](bitcoin/bitcoin#19083 (comment)). And clean up any redundant `wait_until`s in the functional tests. 2. Clean up style + logging in p2p_filter.py and p2p_nobloomfilter_messages.py and jonatack's other [comments](bitcoin/bitcoin#19083 (review)) --- Backport of [[bitcoin/bitcoin#19252 | core#19252]] Depends on D9512 Test Plan: ninja all check check-functional Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Differential Revision: https://reviews.bitcoinabc.org/D9513

glozow mentioned this pull request May 27, 2020

write tests for bloomfiltered connection #18473

Closed

DrahtBot added the Tests label May 27, 2020

DrahtBot mentioned this pull request May 28, 2020

tests: Update more tests to work with descriptor wallets #18788

Merged

jnewbery reviewed May 31, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

maflcko reviewed May 31, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

glozow force-pushed the test-bloomfilter branch from e692c3c to 9514423 Compare June 3, 2020 22:51

maflcko reviewed Jun 4, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

glozow force-pushed the test-bloomfilter branch from 9514423 to 72672bc Compare June 4, 2020 15:33

DrahtBot mentioned this pull request Jun 4, 2020

Refactor: Improve setup_clean_chain semantics #19168

Closed

jnewbery reviewed Jun 5, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

glozow force-pushed the test-bloomfilter branch from 72672bc to f78730d Compare June 5, 2020 22:37

glozow commented Jun 5, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

glozow force-pushed the test-bloomfilter branch from f78730d to 1e64440 Compare June 7, 2020 16:28

maflcko reviewed Jun 9, 2020

View reviewed changes

glozow force-pushed the test-bloomfilter branch 3 times, most recently from f709a14 to b758749 Compare June 10, 2020 03:24

maflcko reviewed Jun 10, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

maflcko reviewed Jun 10, 2020

View reviewed changes

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

test/functional/p2p_filter.py Outdated Show resolved Hide resolved

glozow added 5 commits June 10, 2020 07:27

[test] add BIP 37 test for node with fRelay=false

497a619

A node with fRelay=False should not receive any invs until filter is set using filterload; all other behavior should be identical.

[test] fix race conditions and test in p2p_filter

0474ea2

-grab mininode_lock for every access to mininode attributes, otherwise there are race conditions

scripted-diff: rename node to peer for mininodes

dca7394

-BEGIN VERIFY SCRIPT- sed -i 's/FilterNode/P2PBloomFilter/g' test/functional/p2p_filter.py; sed -i 's/filter_node/filter_peer/g' test/functional/p2p_filter.py; -END VERIFY SCRIPT-

glozow force-pushed the test-bloomfilter branch from b758749 to dca7394 Compare June 10, 2020 15:41

jonatack reviewed Jun 11, 2020

View reviewed changes

glozow mentioned this pull request Jun 11, 2020

test: wait for disconnect in disconnect_p2ps + bloomfilter test followups #19252

Merged

maflcko merged commit b33136b into bitcoin:master Jun 11, 2020

glozow deleted the test-bloomfilter branch July 25, 2020 14:47

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

glozow commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 6, 2020

Uh oh!

glozow commented Jun 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jun 10, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

glozow Jun 13, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 11, 2020

Uh oh!

glozow commented Jun 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

glozow commented May 27, 2020 •

edited

Loading

DrahtBot commented May 28, 2020 •

edited

Loading

glozow commented Jun 7, 2020 •

edited

Loading

maflcko commented Jun 10, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading