test: Check object hashes in wait_for_getheaders #29318

pull BrandonOdiwuor wants to merge 1 commits into bitcoin:master from BrandonOdiwuor:wait-for-getheaders-hash changing 6 files +17 −17

BrandonOdiwuor commented at 3:41 PM on January 25, 2024: contributor

Fixes #18614

Previously, wait_for_getheaders only looked for the presence of a recent "getheaders" message. Additionally checking the hashstop inside the message should make tests involving wait_for_getheaders more robust.

This PR is progress towards closing issue: #18614 after PR: #18690 (merged) which introduced this check for wait_for_getdata
test: check for matching object hashes in wait_for_getheaders d249092265
DrahtBot commented at 3:41 PM on January 25, 2024: contributor


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



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #29575 (net_processing: make any misbehavior trigger immediate discouragement by sipa)
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.
DrahtBot added the label Tests on Jan 25, 2024

in test/functional/p2p_initial_headers_sync.py:70 in d249092265

  66 | @@ -67,7 +67,7 @@ def run_test(self):
  67 |          self.announce_random_block(all_peers)
  68 |  
  69 |          self.log.info("Check that peer1 receives a getheaders in response")
  70 | -        peer1.wait_for_getheaders()
  71 | +        peer1.wait_for_getheaders(hash_stop=0)

maflcko commented at 9:17 AM on January 26, 2024:

I think the block hash can be returned from announce_random_block and then checked here against the first item in the block locator hashes?

in test/functional/p2p_initial_headers_sync.py:92 in d249092265

  88 | @@ -89,14 +89,14 @@ def run_test(self):
  89 |          self.announce_random_block(all_peers)
  90 |  
  91 |          self.log.info("Check that peer1 receives a getheaders in response")
  92 | -        peer1.wait_for_getheaders()
  93 | +        peer1.wait_for_getheaders(hash_stop=0)

maflcko commented at 9:17 AM on January 26, 2024:

same?

in test/functional/p2p_initial_headers_sync.py:99 in d249092265

  96 |          expected_peer = peer2
  97 |          if peer2 == peer_receiving_getheaders:
  98 |              expected_peer = peer3
  99 |  
 100 | -        expected_peer.wait_for_getheaders()
 101 | +        expected_peer.wait_for_getheaders(hash_stop=0)

maflcko commented at 9:17 AM on January 26, 2024:

same?

in test/functional/p2p_segwit.py:201 in d249092265

 197 | @@ -198,7 +198,7 @@ def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60):
 198 |              self.send_message(msg)
 199 |          else:
 200 |              self.send_message(msg_inv(inv=[CInv(MSG_BLOCK, block.sha256)]))
 201 | -            self.wait_for_getheaders()
 202 | +            self.wait_for_getheaders(hash_stop=0)

maflcko commented at 9:18 AM on January 26, 2024:

same?

maflcko commented at 9:21 AM on January 26, 2024: member

In Bitcoin Core, the stop hash will always be zero, so not sure if it is worth it to require passing it every time?

It may be better to instead pass the top hash and check that, if provided?
stratospher commented at 12:31 PM on March 25, 2024: contributor

this would be useful to have in #29122. are you planning to address the reviews and undraft the PR?
sr-gi commented at 12:53 PM on March 25, 2024: member

this would be useful to have in #29122. are you planning to address the reviews and undraft the PR?

I wouldn't mind taking over this otherwise
maflcko commented at 12:54 PM on March 25, 2024: member

Closing as up-for-grabs for now. There hasn't been any activity or progress at all, other than it being opened.
maflcko closed this on Mar 25, 2024
bitcoin locked this on Mar 25, 2025

Contributors

Labels

Linked (view graph)

#18614 test: Check hash in wait_for_get* helpers #18690 test: Check object hashes in wait_for_getdata

test: Check object hashes in wait_for_getheaders #29318

Code Coverage

Reviews

Conflicts