test: Check object hashes in wait_for_getdata #18690

pull robot-visions wants to merge 1 commits into bitcoin:master from robot-visions:wait-for-getdata changing 4 files +9 −16

robot-visions commented at 8:14 PM on April 17, 2020: contributor

Previously, wait_for_getdata only looked for the presence of a recent "getdata" message. Additionally checking the object hashes inside the message should make tests involving wait_for_getdata more robust.

p2p_sendheaders.py already overrides wait_for_getdata do this check; we can use the same approach consistently across all tests that call wait_for_getdata.

This PR is progress towards #18614 , but closing that issue would also involve some additional changes to wait_for_getheaders.
DrahtBot added the label Tests on Apr 17, 2020
in test/functional/test_framework/mininode.py:416 in 02d6d565d9 outdated
```
 417 | +        The object hashes in the inventory vector must match the provided hash_list."""
 418 |  
 419 |          def test_function():
 420 |              assert self.is_connected
 421 | -            return self.last_message.get("getdata")
 422 | +            return "getdata" in self.last_message and [x.hash for x in self.last_message["getdata"].inv] == hash_list
```
theStack commented at 6:00 PM on April 20, 2020:

nit: I'd prefer the self.last_message.get("data") syntax here to be consistent to other wait_for_ methods, see e.g.: https://github.com/bitcoin/bitcoin/blob/56d2ff8a8fc52676a22163f804a9f1a5415b4b71/test/functional/test_framework/mininode.py#L385 Or, as longer version (in terms of LOC) that only needs to access last_message once: https://github.com/bitcoin/bitcoin/blob/56d2ff8a8fc52676a22163f804a9f1a5415b4b71/test/functional/test_framework/mininode.py#L392-L395

robot-visions commented at 9:25 PM on April 20, 2020:

Good idea! Updated to use the longer version.
theStack approved
theStack commented at 6:01 PM on April 20, 2020: member

Concept ACK I think the two commits could be squashed into one
robot-visions force-pushed on Apr 20, 2020
robot-visions commented at 9:26 PM on April 20, 2020: contributor

Updated, thanks for the review!
robot-visions force-pushed on Apr 21, 2020
theStack commented at 11:27 AM on April 22, 2020: member

Could you rebase on master? If was short before ACKing this but then I saw that for the PR branch itself the functional tests fail due to https://github.com/bitcoin/bitcoin/issues/18711
test: check for matching object hashes in wait_for_getdata 9f5608c289
robot-visions force-pushed on Apr 22, 2020
robot-visions commented at 6:04 PM on April 22, 2020: contributor

@theStack Rebased!
theStack approved
theStack commented at 10:29 AM on April 23, 2020: member

ACK 9f5608c2893f89cd56c7c548b748996199e0da1d :beers:
MarcoFalke merged this on Apr 23, 2020
MarcoFalke closed this on Apr 23, 2020
robot-visions commented at 4:02 PM on April 23, 2020: contributor

Huge thanks @theStack @MarcoFalke for helping me to get my first PR merged :)
robot-visions deleted the branch on Apr 23, 2020
theStack commented at 4:09 PM on April 23, 2020: member

@robot-visions: Congratulations! :tada: Have fun going down further the rabbit hole, may many more PRs follow (WARNING: working on Bitcoin Core can be addictive ;-))
MarcoFalke referenced this in commit a215c61333 on Apr 24, 2020
sidhujag referenced this in commit 83fbffb5ee on Apr 24, 2020
Fabcien referenced this in commit b6aa3c347d on Jan 20, 2021
Fabcien referenced this in commit 9d4e2e9eed on Jan 20, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

robot-visions

theStack

Labels

Tests

Linked (view graph)

#18614 test: Check hash in wait_for_get* helpers #18756 refactor: test: use wait_for_getdata() in p2p_compactblocks.py #29318 test: Check object hashes in wait_for_getheaders #29748 test: Makes `wait_for_getdata` delete data on checks, plus allows to check the getdata message type