test: complete impl. of msg_merkleblock and wait_for

theStack commented at 12:59 pm on April 11, 2020: member

Implements the missing initialization/serialization methods for msg_merkleblock, based on the already present class CMerkleBlock. Also changes the method wait_for_merkleblock() to be more precise by waiting for a merkleblock with a specified blockhash instead of an arbitrary one.

In the BIP37 test p2p_filter.py, this new method is used to make the test of receiving merkleblock and tx if a filter is set to be more precise, by checking if they also arrive in the right order.

In the course of this PR, also the interface for the methods wait_for_merkleblock() and wait_for_header() are improved to take a hex string instead of an integer, which is more typesafe and less of a burden to the caller.

fanquake added the label Tests on Apr 11, 2020

in test/functional/p2p_filter.py:80 in afb20f7fac outdated

81-            filter_node.last_message.pop("merkleblock", None)
82         filter_node.tx_received = False
83-        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
84-        filter_node.wait_for_merkleblock()
85+        block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
86+        filter_node.wait_for_merkleblock(int(block_hash, 16))

MarcoFalke commented at 1:10 pm on April 11, 2020:

style-nit: I slightly prefer the interface where the hash is passed in as a hex string, as opposed to a plain integer. The integer is less type safe and could be confused with other args such as timeout. Also, putting the burden on the caller to always call int() makes tests slightly harder to write.

theStack commented at 1:21 pm on April 11, 2020:

I fully agree, but then for consistency reasons all three methods wait_for_{header,block,merkleblock} should have the same interface and be changed to take a hex string accordingly? Maybe a good candidate for a follow-up PR (good first issue).

MarcoFalke commented at 1:30 pm on April 11, 2020:

At least wait_for_header is only used in one place:

0$ git grep wait_for_header
1test/functional/p2p_invalid_locator.py:                node.p2p.wait_for_header(int(node.getbestblockhash(), 16))

so this could be sneaked into this pull (as a separate commit) or left for later. Whatever you prefer.

MarcoFalke approved

MarcoFalke commented at 1:11 pm on April 11, 2020: member

ACK, nice I didn’t know CMerkleBlock was already implemented

theStack referenced this in commit de91a4165c on Apr 11, 2020

theStack commented at 2:12 pm on April 11, 2020: member

Pushed another commit that improves the interface of wait_for_merkleblock and wait_for_header by taking a hex string instead of an integer, as suggested.

MarcoFalke commented at 3:43 pm on April 11, 2020: member

0# ./test/lint/lint-python.sh 
1test/functional/p2p_filter.py:17:1: F401 'test_framework.mininode.mininode_lock' imported but unused

test: complete impl. of msg_merkleblock and wait_for_merkleblock

Implements the missing initialization/serialization methods for
msg_merkleblock, based on the already present class CMerkleBlock.
Also changes the method wait_for_merkleblock() to be more precise by waiting
for a merkleblock with a specified blockhash instead of an arbitrary one.

In the BIP37 test p2p_filter.py, this new method is used to make the test of
receiving merkleblock and tx if a filter is set to be more precise, by checking
if they also arrive in the right order.

1356a45ef0

refactor: test: improve wait_for{header,merkleblock} interface

The interfaces for the methods wait_for_header() and wait_for_merkleblock() are
changed to take a hex string instead of an integer, improving type safety and
removing the burden from the caller to always do the transformation via
`int(...)`. As suggested by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/18593#discussion_r407062253

854382885f

theStack force-pushed on Apr 11, 2020

theStack commented at 4:54 pm on April 11, 2020: member

@MarcoFalke: Thanks for the hint, fixed. (Should also form the habit of running the linters locally before pushing…)

MarcoFalke commented at 5:08 pm on April 11, 2020: member

No worries. The linter should run on travis in the first couple of seconds and then either fail or pass. I don’t even have flake8 installed on my workstation.

DrahtBot commented at 10:18 pm on April 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:

#18610 (scripted-diff: test: replace command with msgtype (naming) by theStack)

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.

MarcoFalke commented at 11:33 pm on April 12, 2020: member

ACK 854382885f18aa9a95cdde3d11591b05c305ad3f

MarcoFalke merged this on Apr 12, 2020

MarcoFalke closed this on Apr 12, 2020

sidhujag referenced this in commit a142bdcaf0 on Apr 13, 2020

jasonbcox referenced this in commit 171445f9f2 on Sep 24, 2020

janus referenced this in commit d0f854bf3c on Nov 8, 2020

theStack deleted the branch on Dec 1, 2020

DrahtBot locked this on Feb 15, 2022

test: complete impl. of msg_merkleblock and wait_for_merkleblock #18593

Conflicts