test: Add basic test for BIP 37

MarcoFalke commented at 5:28 pm on March 12, 2020: member

This does not add full coverage, but should be a good start and can be extended in the future. Currently, none of the BIP 37 p2p code has test coverage.

DrahtBot added the label Tests on Mar 12, 2020

MarcoFalke force-pushed on Mar 12, 2020

DrahtBot commented at 8:53 pm on March 12, 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:

#18446 (test: Add test for wtxid transaction relay by fjahr)
#18261 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

Coverage

Coverage	Change (pull 18334, 70f8cf0ac76a483f4e5784bb303e081688206e40)	Reference (master, e2d36639ca87b980b147047e4e47cf160b2c9406)
Lines	+0.1081 %	90.0780 %
Functions	+0.2643 %	85.8297 %
Branches	+0.0528 %	51.6976 %

Updated at: 2020-03-12T20:53:06.964444.

in test/functional/p2p_filter.py:32 in fabf37381c outdated

26+        data=
27+        b'@\x00\x08\x00\x80\x00\x00 \x00\xc0\x00 \x04\x00\x08$\x00\x04\x80\x00\x00 \x00\x00\x00\x00\x80\x00\x00@\x00\x02@ \x00',
28+        nHashFuncs=19,
29+        nTweak=0,
30+        nFlags=1,
31+    )

theStack commented at 8:03 pm on March 22, 2020:

Could add a short comment on what this filter’s dimensions are based upon (w.r.t. target parameters number of elements N, false positive rate P)?

MarcoFalke commented at 1:36 pm on March 24, 2020:

Thanks, extended the comment to state the filter parameters.

in test/functional/p2p_filter.py:74 in fabf37381c outdated

69+        self.log.info('Check that we receive merkleblock and tx if the filter matches a tx in a block')
70+        filter_node.merkleblock_received = False
71+        filter_node.tx_received = False
72+        self.nodes[0].generatetoaddress(1, filter_address)
73+        filter_node.sync_with_ping()  # wait for inv
74+        filter_node.sync_with_ping()  # wait for merkleblock

theStack commented at 8:48 pm on March 22, 2020:

This looks fishy, can two consecutive sync_with_ping() calls actually help more for synchronization than just one? To my understanding, sync_with_ping is only helpful whenever a node is already processing a received message and we want to guarantee that the processing is finished. I don’t see how you could “wait for messages” with that. We neither know that at the start of the first sync_with_ping() call the expected inv message from node0 has already arrived, nor that at the second sync_with_ping() call merkleblock has arrived (or even getdata was received from node0 before). I had a similar problem while working on #17461 (see #17461 (comment)) with an adequate but still not 100% satisfactory waiting solution.

To prove my point, the test fails without the immediate tx relay enabled (which should only serve as speed-up, but not as synchronization mechanism, in my humble opinion).

MarcoFalke commented at 1:37 pm on March 24, 2020:

Thanks, replaced with filter_node.wait_for_tx(txid), which is a stricter check that also passes when immediate tx relay is disabled.

theStack commented at 8:50 pm on March 22, 2020: member

Concept ACK (obviously, more tests are always good), left two code-review comments

MarcoFalke force-pushed on Mar 24, 2020

MarcoFalke commented at 1:38 pm on March 24, 2020: member

Addressed feedback by @theStack

in test/functional/p2p_filter.py:83 in fa5dbcf25b outdated

78+        filter_node.tx_received = False
79+        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
80+        filter_node.sync_with_ping()
81+        filter_node.sync_with_ping()
82+        assert filter_node.merkleblock_received
83+        assert not filter_node.tx_received

theStack commented at 9:04 pm on March 24, 2020:

I think sync_with_ping() doesn’t help here for reliable synchronization as well, as in theory there could still arrive a new message at the filter node after the asserts (same argumentation as my code-review comment a few lines above). Maybe using wait_until on filter_node.merkleblock_received would make it more robust?

MarcoFalke commented at 11:47 pm on March 24, 2020:

A wait_until on filter_node.merkleblock_received does return immediately when the assert filter_node.merkleblock_received has passed. I don’t see how this could help anything.

theStack commented at 10:05 am on March 25, 2020:

What I meant was removing the sync_with_ping()s completely (as they effectively only serve as hidden delays, they don’t do anything for reliable synchronization – the expected messages could in theory still appear after) and replacing the assert filter_node.merkleblock_received with a wait_until-pendant, similar like you did on the subtests 1 and 4 on the latest force-push (https://github.com/bitcoin/bitcoin/compare/fabf37381c4b62fc057ca0df4e77f2f2deb0e428..fa64441bd7a284a99c0e637e33e6aff690718d75).

Or did I overlook something and the situation for subtests 2 and 3 is different?

MarcoFalke commented at 10:47 am on March 25, 2020:

Replaced with filter_node.wait_for_merkleblock().

in test/functional/p2p_filter.py:90 in fa5dbcf25b outdated

85+        self.log.info('Check that we not receive a tx if the filter does not match a mempool tx')
86+        filter_node.merkleblock_received = False
87+        filter_node.tx_received = False
88+        self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 90)
89+        filter_node.sync_with_ping()
90+        filter_node.sync_with_ping()

theStack commented at 9:14 pm on March 24, 2020:

I don’t think this helps for synchronization here, see code-review comment above.

MarcoFalke commented at 10:47 am on March 25, 2020:

This can not be replaced with a wait_for unless I am mistaken

theStack commented at 11:34 am on March 25, 2020:

Agreed, wait_for is not an option here – it’s always problematic to assert that something was not received, as we never know for sure that it couldn’t still come in in the future. I don’t know a better way here than to just wait for some arbitrary time between the .sendtoaddress() call and the asserts :confused:

theStack commented at 9:20 pm on March 24, 2020: member

The synchronization approach is more robust now, though it is not used for all of your four subtests yet (see my code-review comment). Just out of curiousity, how did you create the bloom filter data in practice? Is there like a toolset available for playing around with bloom filters? Or did you go for the manual, programmatic way, i.e. creating CBloomFilter instance based on the bitcoind codebase, insert()ing the scriptPubKey and hex-dumping the filter’s vData contents? Right now, assuming scriptPubKey changes, I as reader of the test wouldn’t know a quick way of how to update the data field.

MarcoFalke commented at 11:49 pm on March 24, 2020: member

Just out of curiousity, how did you create the bloom filter data in practice? Is there like a toolset available for playing around with bloom filters? Or did you go for the manual, programmatic way, i.e. creating CBloomFilter instance based on the bitcoind codebase, insert()ing the scriptPubKey and hex-dumping the filter’s vData contents?

I did what you assumed. Instead of hexdump I sent it to a python mininode and made it print() the repr.

MarcoFalke commented at 11:50 pm on March 24, 2020: member

Just to be make sure: This test is not complete and can be extended/improved in the future. Though, I don’t have any interest in doing that myself atm. This task will be opened as a good first issue.

test: Add basic test for BIP 37 fa15699969

MarcoFalke force-pushed on Mar 25, 2020

MarcoFalke commented at 11:55 pm on March 27, 2020: member

Anything left to do here?

theStack commented at 10:51 am on March 29, 2020: member

Anything left to do here?

I am still sceptical about the point of the two remaining .sync_with_ping()s in the context of this test. Having skimmed through all of its uses in other functional tests, I identified three use-cases for .sync_with_ping():

ensuring that the version exchange with the test node already happened
ensuring that messages that have been sent from the test node itself to the peer have been processed, with the following pattern: peer.send_message(...), following peer.sync_with_ping()
checking if a peer is still alive or keeping connections alive, respectively

In this test none of this really applies, the .sync_with_ping()s just serve as implicit delay, so I’d prefer to make the delay explicit. Thats at least my undestanding of the situation, maybe someone with more experience with the P2P tests can comment here.

MarcoFalke commented at 11:50 am on March 29, 2020: member

The rationale is the same, I assume that an “imaginary” message (obviously it doesn’t exist on the wire, because that would be a bug) can at most take 3 ping-pong round trips. (Assuming also immediate tx relay). I am not sure what I could replace the delay with that would make more sense. If I use a plain time.sleep, I wouldn’t know what to pass as argument, because I don’t know the time it takes for a round trip. Also, with immediate tx relay disabled, I can not predict the poisson delay on top of that.

I think the best solution is to remove the test, if it is too controversial.

practicalswift commented at 12:31 pm on March 29, 2020: contributor

Code review ACK fa156999695ddaeb016d8320bee62f8d96679d55 – more testing coverage is better than less testing coverage

Improvements can be done in a followup.

MarcoFalke commented at 3:37 pm on March 29, 2020: member

@practicalswift I don’t want to just merge this as long as there are outstanding concerns from @theStack . I don’t know how to address them, so removing that test case temporarily and adding it back when they are figured out would be an option.

theStack commented at 4:17 pm on March 29, 2020: member

@MarcoFalke: It was not my intention to block this PR – indeed I think not merging this PR would be far worse than having a test with a small synchronization controversy (that only comes from one single person so far). The pattern of sync_with_ping()ing the same node more than once in a row for synchronization purposes has never been used so far in any functional tests, which raised my doubts if its the right tool. But with your description of “at most taking that many ping-pong roundtrips” it makes a bit more sense to me now.

(I still think that in an ideal world all those P2P tests should also work with immediate tx relay disabled, but on the other hand I also have no idea how to realize this.)

MarcoFalke merged this on Mar 30, 2020

MarcoFalke closed this on Mar 30, 2020

sidhujag referenced this in commit 7c914b20ab on Mar 30, 2020

MarcoFalke deleted the branch on Mar 30, 2020

jasonbcox referenced this in commit 887b712b17 on Sep 24, 2020

DrahtBot locked this on Feb 15, 2022

test: Add basic test for BIP 37 #18334

Conflicts

Coverage