test: msg_mempool, fRelay, and other bloomfilter tests

glozow commented at 10:03 pm on May 27, 2020: member

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 added the label Tests on May 27, 2020

DrahtBot commented at 5:00 am on May 28, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#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.

in test/functional/p2p_filter.py:60 in e692c3cdd1 outdated

55@@ -51,6 +56,21 @@ def on_merkleblock(self, message):
56     def on_tx(self, message):
57         self.tx_received = True
58 
59+class FilterNodeFRelay(FilterNode):
60+    def peer_connect(self, *args, services=NODE_NETWORK|NODE_WITNESS, send_version=True, **kwargs):

jnewbery commented at 7:52 pm on May 31, 2020:

I think you can remove the services and send_version arguments, and just set them directly below (this function is only ever called once, and you know what services and send_version should be set to.

jnewbery commented at 7:53 pm on May 31, 2020: member

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

in test/functional/p2p_filter.py:73 in e692c3cdd1 outdated

68+            vt.addrTo.port = self.dstport
69+            vt.addrFrom.ip = "0.0.0.0"
70+            vt.addrFrom.port = 0
71+            vt.nRelay = 0 # default = 1
72+            self.on_connection_send_msg = vt  # Will be sent soon after connection_made
73+        return create_conn

MarcoFalke commented at 9:32 pm on May 31, 2020:

Is there any reason to overwrite this at all? The whole overwrite with the on_connection_send_msg magic is a bit of a mental load. I believe this can simply be a FilterNode, where you set send_version=False and then send_message(version_without_nRelay) and then wait_for_verack().

Overall it should be less complex and less lines of code

glozow commented at 7:37 pm on June 2, 2020:

Oops - had a big hole in my knowledge. You’re right that was way too complicated 🤦‍♀️ . How does this look:

0class FilterNode(P2PInterface):
1    def __init__(self, fRelay=True):
2        super().__init__()
3        if fRelay == False:
4            version_without_nRelay = msg_version()
5            version_without_nRelay.nRelay = 0
6            self.on_connection_send_msg = version_without_nRelay

I’m still not 100% clear on how connection works under the hood, but setting self.on_connection_send_msg feels correct since it’s handled in the connection_made callback - otherwise I need to somehow wait for connection to be established before using send_message directly.

MarcoFalke commented at 8:36 pm on June 2, 2020:

When I added on_connection_send_msg it was meant to be a “private” member. Obviously in python there is no “private”, so discount my point by that.

I think either is fine. Your solution with the on_connection_send_msg or something along the lines of:

0p2p = create_connection(bla)
1p2p.wait_until(p2p.is_connected)
2p2p.send_message(ver)
3p2p.wait_for_verack()

glozow force-pushed on Jun 3, 2020

in test/functional/p2p_filter.py:54 in 951442335b outdated

60@@ -51,7 +61,6 @@ def on_merkleblock(self, message):
61     def on_tx(self, message):
62         self.tx_received = True
63 
64-

MarcoFalke commented at 12:50 pm on June 4, 2020:

style-nit (feel free to ignore): I think pep-8 says two new lines before class

in test/functional/p2p_filter.py:45 in 951442335b outdated

36@@ -34,6 +37,13 @@ class FilterNode(P2PInterface):
37         nFlags=1,
38     )
39 
40+    def __init__(self, fRelay=True):
41+        super().__init__()
42+        if fRelay == False:
43+            version_without_nRelay = msg_version()
44+            version_without_nRelay.nRelay = 0
45+            self.on_connection_send_msg = version_without_nRelay

MarcoFalke commented at 12:55 pm on June 4, 2020:

How does this work? Isn’t peer_connect called after init, in which case on_connection_send_msg would be overwritten?

glozow commented at 3:38 pm on June 4, 2020:

Aw shoot, just fixed using send_version=False and then manually sending it after connection is established. Asserting relaytxes=False so it should be set correctly now.

MarcoFalke commented at 12:58 pm on June 4, 2020: member

Concept ACK

glozow force-pushed on Jun 4, 2020

in test/functional/p2p_filter.py:156 in 72672bc5a3 outdated

152@@ -125,8 +153,11 @@ def run_test(self):
153 
154         self.log.info('Check that we receive a tx in reply to a mempool msg if the filter matches a mempool tx')
155         filter_node.merkleblock_received = False
156+        filter_node.tx_received = False

jnewbery commented at 3:50 pm on June 4, 2020:

Any time we access members of a P2PInterface or derived class, we should be taking the mininode_lock, otherwise there’s the possibility of a data race between the main test logic thread and the network event loop. I think the easiest would be to define some getters on the FilterNode class that take the lock and then return the attribute value.

jnewbery commented at 3:58 pm on June 4, 2020:

In fact, I think this whole part of the test is not actually testing the mempool message (which is obvious, since it was passing even before you added the line to send the mempool message). Since you’re now testing the mempool message correctly in test_msg_mempool(), I think you can just erase this block.

glozow commented at 1:14 am on June 5, 2020:

True, I had considered just changing the log message (since it didn’t seem entirely correct) but I wasn’t really sure what it should be. It looks like @MarcoFalke wrote this test, I’m wondering what he would have in mind?

glozow commented at 1:18 am on June 5, 2020:

Good point. I’m thinking a getter and a setter: get_tx_received, reset_tx_received (to False), and for merkleblock_received as well? Shall I change the rest of the test too since it uses these throughout?

jnewbery commented at 3:19 pm on June 5, 2020:

Shall I change the rest of the test too since it uses these throughout?

If you want to, that’d be great! There are lots of data races in this test where the test logic thread and the p2p handler thread can be accessing the same memory.

Take a look at Python properties: https://docs.python.org/3/library/functions.html#property. By implementing a getter/setter using properties, you don’t need to change the client code from assert not filter_node.tx_received to assert not filter_node.get_tx_received().

Be careful not to use the setter for the on_xxxx(), methods, which already have the lock when called. It’s better to make the locks non-reentrant if possible (see #19178)

in test/functional/p2p_filter.py:188 in 72672bc5a3 outdated

183@@ -153,6 +184,33 @@ def run_test(self):
184         filter_node.send_and_ping(msg_filterload(data=b'', nHashFuncs=1))
185         filter_node.send_and_ping(msg_filteradd(data=b'letstrytocrashthisnode'))
186 
187+    def run_test(self):
188+        filter_node = self.nodes[0].add_p2p_connection(FilterNode())

jnewbery commented at 0:52 am on June 5, 2020:

nit: one style point that I’ve tried to move the tests away from is referring to connections as “nodes”. A node is a bitcoind instance that we’re testing. A p2p connection is a test structure used to interact with the node. Using the word node for both the node and connections to it is confusing.

glozow commented at 10:38 pm on June 5, 2020:

Noted! Renaming all of them increases the number of lines in the diff by a lot, so I put renames for existing code in a separate commit.

in test/functional/p2p_filter.py:129 in 72672bc5a3 outdated

127+
128+        # Clear the mempool so that this transaction does not impact subsequent tests
129+        self.nodes[0].generate(1)
130 
131+    def test_filter(self, filter_node):
132         self.log.info('Add filtered P2P connection to the node')

jnewbery commented at 0:57 am on June 5, 2020:

This log seems in the wrong place. The connection has already been created earlier.

jnewbery commented at 0:59 am on June 5, 2020: member

Looks good. A few comments inline.

glozow force-pushed on Jun 5, 2020

in test/functional/p2p_filter.py:121 in f78730d582 outdated

190-        filter_node.merkleblock_received = False
191-        filter_node.tx_received = False
192+        filter_peer.merkleblock_received = False
193+        filter_peer.tx_received = False
194         self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 90)
195-        filter_node.sync_with_ping()

glozow commented at 11:03 pm on June 5, 2020:

Weird that there are two sync_with_pings here. @MarcoFalke would it be correct for me to remove one (since I’m touching the lines anyway)? From what I understand, one sync_with_ping is enough to ensure that the messages were sent and processed, and we’ll be grabbing mininode_lock when we check the *_received values.

MarcoFalke commented at 0:02 am on June 6, 2020:

There are no messages sent at all, so using sync_with_ping is odd, but I didn’t know what else to use.

The first sync_with_ping is needed for any imaginary invs on the wire (from SendMessages) to be flushed to the filter peer
The seconds one is needed for our imaginary getdata (in reply to the imaginary inv) to be flushed to the node (and to be processed)

glozow commented at 4:44 pm on June 7, 2020:

Ohh I see what you mean. If only 1 is sent, the order could be:

send ping
receive inv, send getdata
receive pong, can move on
assert not received
receive data (too late)

With 2, it has to be:

send ping1
receive inv, send getdata
receive pong1, send ping2
any response to getdata must be received before pong2
assert

Is this correct?

MarcoFalke commented at 4:47 pm on June 7, 2020:

Jup, that’s the idea

MarcoFalke commented at 2:09 am on June 6, 2020: member

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

glozow force-pushed on Jun 7, 2020

glozow commented at 5:44 pm on June 7, 2020: member

Resolves #18473 . Ready for Review again!

in test/functional/p2p_filter.py:126 in 75865b28a0 outdated

 93@@ -93,6 +94,23 @@ def test_size_limits(self, filter_node):
 94 
 95         filter_node.send_and_ping(msg_filterclear())
 96 
 97+    def test_msg_mempool(self):
 98+        self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
 99+        self.nodes[0].disconnect_p2ps()
100+        filter_peer = FilterNode()
101+        filter_peer.tx_received = False

MarcoFalke commented at 12:30 pm on June 9, 2020:

in commit 75865b28a0:

I think tx_received can be removed from this test, as it is redundant to wait_for_tx, no?

glozow commented at 1:06 am on June 10, 2020:

Er, do you mean because it’s already set to False in the constructor? I removed it for this reason, thanks for bringing it to my attention. I don’t think we can remove tx_received because we sometimes want to assert that we didn’t receive a tx with it.

in test/functional/p2p_filter.py:144 in 75865b28a0 outdated

142@@ -125,8 +143,11 @@ def run_test(self):
143 
144         self.log.info('Check that we receive a tx in reply to a mempool msg if the filter matches a mempool tx')

MarcoFalke commented at 12:36 pm on June 9, 2020:

same commit:

This comment is obviously wrong (git blame myself)

0        self.log.info('Check that we receive a tx if the filter matches a mempool tx')

MarcoFalke commented at 11:23 am on June 10, 2020:

unrelated nit in commit facd5f9765:

Has this comment been fixed?

in test/functional/p2p_filter.py:148 in 75865b28a0 outdated

142@@ -125,8 +143,11 @@ def run_test(self):
143 
144         self.log.info('Check that we receive a tx in reply to a mempool msg if the filter matches a mempool tx')
145         filter_node.merkleblock_received = False
146+        filter_node.tx_received = False
147         txid = self.nodes[0].sendtoaddress(filter_address, 90)
148+        self.nodes[0].p2p.send_message(msg_mempool())

MarcoFalke commented at 12:37 pm on June 9, 2020:

So this change needs to go

glozow commented at 1:07 am on June 10, 2020:

I removed it altogether, since receiving tx behavior for this reason is already tested and msg_mempool is added.

in test/functional/p2p_filter.py:99 in 75865b28a0 outdated

93@@ -93,6 +94,23 @@ def test_size_limits(self, filter_node):
94 
95         filter_node.send_and_ping(msg_filterclear())
96 
97+    def test_msg_mempool(self):
98+        self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
99+        self.nodes[0].disconnect_p2ps()

MarcoFalke commented at 12:38 pm on June 9, 2020:

Same commit:

I have a slight preference that tests clean up after themselves, and not after other tests. So this should be moved to the end of the subtest that created the peer connection.

in test/functional/p2p_filter.py:195 in 5f0dd129a0 outdated

190+        self.test_size_limits(filter_peer)
191+
192+        self.log.info('Test BIP 37 for a node with fRelay = True (default)')
193+        self.test_filter(filter_peer)
194+        self.nodes[0].disconnect_p2ps()
195+        wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)

MarcoFalke commented at 12:46 pm on June 9, 2020:

in commit 5f0dd129a0:

It seems a burden on tests to call this to avoid races. It might be easy to forget and races might only appear long after the test has been written and merged. It seems less fragile to move this wait into the disconnect helper.

glozow commented at 2:24 am on June 10, 2020:

Uh oh… I naively added a wait_until(lambda: len(self.getpeerinfo()) == 0) but getpeerinfo gets both p2p connections and connected nodes. I need to figure out a better way to do this.

glozow commented at 3:54 pm on June 10, 2020:

An idea is to wait until len(self.getpeerinfo()) decreases by len(self.p2ps) but that doesn’t seem completely safe

MarcoFalke commented at 5:37 pm on June 10, 2020:

mininodes have a unique user agent, so the assert could be that no peers of this user agent are connected

in test/functional/p2p_filter.py:210 in 5f0dd129a0 outdated

205+        filter_peer_without_nrelay.wait_for_verack()
206+        assert not self.nodes[0].getpeerinfo()[0]['relaytxes']
207+        self.test_frelay_false(filter_peer_without_nrelay)
208+        self.test_filter(filter_peer_without_nrelay)
209+        self.nodes[0].disconnect_p2ps()
210+        wait_until(lambda: len(self.nodes[0].getpeerinfo()) == 0)

MarcoFalke commented at 12:47 pm on June 9, 2020:

same

in test/functional/p2p_filter.py:228 in 5f0dd129a0 outdated

203+        version_without_fRelay.nRelay = 0
204+        filter_peer_without_nrelay.send_message(version_without_fRelay)
205+        filter_peer_without_nrelay.wait_for_verack()
206+        assert not self.nodes[0].getpeerinfo()[0]['relaytxes']
207+        self.test_frelay_false(filter_peer_without_nrelay)
208+        self.test_filter(filter_peer_without_nrelay)

MarcoFalke commented at 12:49 pm on June 9, 2020:

Same commit: Could add assert self.nodes[0].getpeerinfo()[0]['relaytxes'] after this line?

glozow commented at 1:08 am on June 10, 2020:

Added it right after it should be set to True after the filterload message in test_filter.

MarcoFalke commented at 12:53 pm on June 9, 2020: member

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

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 1e64440490 looks like an improvement, but I have some nits to consider 🍬
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUiqqQv9HPT78+tE2ecU5neOV9/iItDomZZ3+dJ/+1clX3pZNzJ7k0Qh/Cr3DBU+
 8Eh4zIS0D1ooVyxknFMhGziukNAmlzYAFW1lSytCwEMCxZhJ4Nujlq3dp/aaQ5FoH
 9j1D2zvTe4QEKvysqTqHafJQ0iuxYoXzO2TXs5io6923GpxLHwndOiozkNUkLd9TA
102W2XwhO/k7Uu0r0SCBFmSyZmb93Jus8h118q0Dskg7uM8FBJ/FuPS6xvIUWpn21h
113etYU0+9E8ncRJ6SXTU4bKcPsHbJrpjo3hzGghhJLlz1+ywJzSiF2TVumFilgMyU
12+MwS/Yvm+vwuTQwFOzIzAQfQYdm1Dh0JOcy52Uob1keytZV/K3opDS3MrlvlPxIC
134ZKMuT/JclX9UkKzIu/ppQ6O57PbhJAcZyXBs4JrQ896rLccsYSGO3K1jQgijrEA
14lZQhSVPm2Y0/cHMrgr9/QcZKk+Ffyvz89HgThyrvmNJNdGCLhuPnoWlQMwjhpviq
15akjJhYf+
16=Pmtq
17-----END PGP SIGNATURE-----

Timestamp of file with hash 88b89b80c24192ef59fea53c027a40ed6e6fe7c8d6795036ca7991f3fb4d7971 -

glozow force-pushed on Jun 10, 2020

in test/functional/p2p_filter.py:110 in facd5f9765 outdated

105+        self.log.info("Send a mempool msg after connecting and check that the tx is received")
106+        self.nodes[0].add_p2p_connection(filter_peer)
107+        filter_peer.send_and_ping(filter_peer.watch_filter_init)
108+        self.nodes[0].p2p.send_message(msg_mempool())
109+        filter_peer.wait_for_tx(txid)
110+        assert filter_peer.tx_received

MarcoFalke commented at 11:22 am on June 10, 2020:

nit in commit facd5f9765:

This assert is not needed. A stronger assert was checked in the previous line, no?

glozow commented at 3:45 pm on June 10, 2020:

Ah yes! All of them should be either wait_for_* or assert not *_received now. And every *_received = False is accompanied by an assert not *_received later.

MarcoFalke commented at 5:36 pm on June 10, 2020:

Thanks, that makes the tests similarly structured

in test/functional/p2p_filter.py:130 in facd5f9765 outdated

138@@ -123,12 +139,6 @@ def run_test(self):
139         assert not filter_node.merkleblock_received
140         assert not filter_node.tx_received
141 
142-        self.log.info('Check that we receive a tx in reply to a mempool msg if the filter matches a mempool tx')
143-        filter_node.merkleblock_received = False
144-        txid = self.nodes[0].sendtoaddress(filter_address, 90)
145-        filter_node.wait_for_tx(txid)
146-        assert not filter_node.merkleblock_received

MarcoFalke commented at 11:25 am on June 10, 2020:

facd5f9765 why is this test removed?

glozow commented at 3:43 pm on June 10, 2020:

I had thought it was already tested, my apologies. I just changed the log message and left the test as-is.

MarcoFalke commented at 4:55 pm on June 10, 2020:

Maybe I am missing something obvious, but I couldn’t find where this is tested already.

glozow commented at 5:38 pm on June 10, 2020:

It wasn’t. I had thought it was tested with the first generatetoaddress test, but that’s a tx in a block, not a tx in mempool.

in test/functional/p2p_filter.py:127 in 27f3cac34e outdated

126 
127-        self.log.info('Add filtered P2P connection to the node')
128+    def test_filter(self, filter_node, frelay=False):
129+        # Set the bloomfilter using filterload
130         filter_node.send_and_ping(filter_node.watch_filter_init)
131+        if frelay == False:

MarcoFalke commented at 11:29 am on June 10, 2020:

in 27f3cac34e

why is this guard needed?

glozow commented at 1:58 pm on June 10, 2020:

Not really needed, but I didn’t think the assertion was relevant to the case where fRelay=True because it doesn’t change after filterload. I could take it out.

glozow commented at 3:41 pm on June 10, 2020:

Removed the conditional and replaced log with comment.

MarcoFalke commented at 5:36 pm on June 10, 2020:

Thanks. This is a now a stronger assertion with less code.

MarcoFalke commented at 11:30 am on June 10, 2020: member

Concept ACK b758749173

[test] add mempool msg test for node with bloomfilter enabled

-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

e8acc60156

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

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

497a619386

[test] sending invalid msgs to node with bloomfilters=0 causes disconnect

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

4ef80f0827

[test] fix race conditions and test in p2p_filter

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

0474ea25af

scripted-diff: rename node to peer for mininodes

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

dca73941eb

glozow force-pushed on Jun 10, 2020

MarcoFalke commented at 5:34 pm on June 10, 2020: member

ACK dca73941eb only changes is restoring accidentally deleted test 🍮

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK dca73941eb only changes is restoring accidentally deleted test 🍮
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUg6PAv/ROMdaabZoFQqsrL/+8J/dZZQXJsUGSiG2TpTQibWFyPSlvgYeDe5raf6
 8yTa139/BqIY8ZsoiMsW7dwHtx3sJQ4bEkL53nECP5OaFS3wdK+6DoddSW4n/Vdm0
 9F3/Wz01/x2tYg8hXP4jpoaBN1KcJpbDtrp+hk4UuOu2Yrwxm7sT1uJi8Gl5g8cRc
10wKPFldxOR/NvB2WjDWGHwda3WSN9ylEa66XYjXCm3dVi0cL4rfsVy6pudL4yVmh8
11suygyKj15vWTmJz4NRsWnTisIRBXcIen/gNvNKl8bEfNU1BUeThziey/6i/zszq6
12DP51JOUSOGCEZqZsK6vcaflUz6GRIGkBQXQ/mC4DrtNJHakn0wSLZ+gO2UKuIKdU
13r6NnqvNSrNFWkc2l1tfmxovGnkZQ2iwu98Mo/X8hs7SCshvcVsDLvhhDecxB61Z6
14tsnwdSR8EBZrMkGWUYjzG6BAsnV/hQpLErFuthJTzEDdmyoO2eWAYk+R3+5Uszrl
15euwfkY0A
16=debF
17-----END PGP SIGNATURE-----

Timestamp of file with hash 70bc0be0f61503b76003fe460f747227c9eb065aff2dd334d2640b7467d69650 -

glozow commented at 5:40 pm on June 10, 2020: member

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

in test/functional/p2p_filter.py:127 in dca73941eb

134+    def test_msg_mempool(self):
135+        self.log.info("Check that a node with bloom filters enabled services p2p mempool messages")
136+        filter_peer = P2PBloomFilter()
137 
138-        self.test_size_limits(filter_node)
139+        self.log.info("Create a tx relevant to the peer before connecting")

jonatack commented at 2:14 pm on June 11, 2020:

e8acc60 It looks like these logs lines 127 and 131 are related to the same test as the logging at line 124. Perhaps make them debug logs instead of info to declutter the ample logging output. (Thanks for putting the test in a separate function as you did.)

in test/functional/p2p_filter.py:230 in dca73941eb

266+        filter_peer_without_nrelay.wait_for_verack()
267+        assert not self.nodes[0].getpeerinfo()[0]['relaytxes']
268+        self.test_frelay_false(filter_peer_without_nrelay)
269+        self.test_filter(filter_peer_without_nrelay)
270+
271+        self.log.info('Test msg_mempool')

jonatack commented at 2:23 pm on June 11, 2020:

497a6193 Is this logging (or the one at the start of test_msg_mempool()) redundant?

in test/functional/p2p_filter.py:217 in dca73941eb

253+
254+        self.log.info('Test BIP 37 for a node with fRelay = True (default)')
255+        self.test_filter(filter_peer)
256+        self.nodes[0].disconnect_p2ps()
257+
258+        self.log.info('Test BIP 37 for a node with fRelay = False')

jonatack commented at 2:25 pm on June 11, 2020:

497a6193 perhaps put the BIP37 tests into their own function

glozow commented at 5:08 pm on June 13, 2020:

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

in test/functional/p2p_nobloomfilter_messages.py:19 in dca73941eb

14+from test_framework.mininode import P2PInterface
15+from test_framework.test_framework import BitcoinTestFramework
16+from test_framework.util import assert_equal
17+
18+
19+class P2PNobloomfilterMessages(BitcoinTestFramework):

jonatack commented at 2:29 pm on June 11, 2020:

4ef80f0 nit: P2PNoBloomFilterMessages (and at the last line)

in test/functional/p2p_nobloomfilter_messages.py:26 in dca73941eb

21+        self.setup_clean_chain = True
22+        self.num_nodes = 1
23+        self.extra_args = [["-peerbloomfilters=0"]]
24+
25+    def test_message_causes_disconnect(self, message):
26+        # Add a p2p connection that sends a message and check that it disconnects

jonatack commented at 2:31 pm on June 11, 2020:

4ef80f0 nit: this could be a """docstring."""

in test/functional/p2p_nobloomfilter_messages.py:41 in dca73941eb

36+        self.log.info("Test that node is disconnected if it sends filterload message")
37+        self.test_message_causes_disconnect(msg_filterload())
38+
39+        self.log.info("Test that node is disconnected if it sends filteradd message")
40+        self.test_message_causes_disconnect(msg_filteradd(data=b'\xcc'))
41+

jonatack commented at 2:33 pm on June 11, 2020:

4ef80f0 nit: I think PEP8 (or this codebase, not sure which) prefers two lines here at the end before if __name__ == '__main__':

Same in p2p_filter.py in dca73941.

jonatack commented at 2:40 pm on June 11, 2020: member

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

MarcoFalke commented at 2:44 pm on June 11, 2020: member

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 commented at 6:05 pm on June 11, 2020: member

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

MarcoFalke merged this on Jun 11, 2020

MarcoFalke closed this on Jun 11, 2020

sidhujag referenced this in commit 29ca92f693 on Jun 11, 2020

glozow referenced this in commit 731fb4e10b on Jun 12, 2020

glozow referenced this in commit afad631b1c on Jun 12, 2020

glozow referenced this in commit 81e82070b7 on Jun 15, 2020

MarcoFalke referenced this in commit 38389dd3a0 on Jun 17, 2020

glozow deleted the branch on Jul 25, 2020

Fabcien referenced this in commit f5b30883d7 on May 13, 2021

deadalnix referenced this in commit f1c2bb088d on May 13, 2021

DrahtBot locked this on Feb 15, 2022

test: msg_mempool, fRelay, and other bloomfilter tests #19083

Conflicts