test: use wait_for_{block,header} helpers in p2p_fingerprint.py #20047

This small PR takes use of the message receiving helper functions wait_for_block() and wait_for_header() (from module test_framework.p2p) in the test p2p_fingerprint.py. It also simplifies the checks for very old stale blocks/headers requests by getting rid of the functions last_block_equals() and last_header_equals() and rather only testing that not any blocks/headers message is received at all. Unneeded sending of requests are also removed and calls to time.sleep(...) substituted by ping syncs.

Note that the test methods last_block_equals() and last_header_equals() are still needed for checking that the concerned message didn't arrive within a certain time window: https://github.com/bitcoin/bitcoin/blob/72affcb16cad45bd9e08ca163b2147fd01b84b70/test/functional/p2p_fingerprint.py#L123-L131 Unfortunately I couldn't figure out how to use the wait_for_{block,header} functions for that -- using something like

assert_raises(AssertionError, lambda: node0.wait_for_block(stale_hash, timeout=3))

works, but still prints out the assertion error, clogging up the log:

2020-09-30T15:00:20.316000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_hh8usu8u
2020-09-30T15:00:27.134000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        def test_function():
            if check_connected:
                assert self.is_connected
            return test_function_in()
'''
2020-09-30T15:00:30.150000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
        def test_function():
            if check_connected:
                assert self.is_connected
            return test_function_in()
'''
2020-09-30T15:00:30.252000Z TestFramework (INFO): Stopping nodes
2020-09-30T15:00:30.354000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_hh8usu8u on exit
2020-09-30T15:00:30.355000Z TestFramework (INFO): Tests successful

Concept ACK 136d96b7.

Maybe we should inline the two methods last_block_equals and last_header_equals because they are only used with a negation currently. That means the test would still pass if for some reason node.last_message.get("block") is falsey. What should be tested in the assert not self.last_block_equals() is that the last message's block was available but the hash didn't match.

@guggero: Thanks a lot for reviewing and your inputs, good points! I think that actually we can get rid of the functions last_{block,header}_equals completely (i.e. not even needing to inline them), as a check ensuring that no blocks or header message actually arrives after requesting very old blocks/headers should be sufficient. That is, removing "blocks"/"headers" from the last_message dictionary before the request, and after the waiting time, checking that it hasn't been added in-between. That should do it in my opinion.

tACK 521b833a.

Nice change with this additional commit, I agree that this makes the test even more precise while removing more code than it adds.

ACK

Force-pushed with the suggestions from MarcoFalke (https://github.com/bitcoin/bitcoin/pull/20047#discussion_r499482654, #20047 (review)).

ACK 6b56c1f4

pm ACK 6b56c1f4d0d5857d9d61a81dc96db1b603c368b5