This PR should fix the intermittent failure of sendheaders.py described in #11673. The first three commits are tidying up and refactoring the file. The final commit fix flakiness in sendheaders.py fixes the intermittent failures. The commit message for that commit describes the problems that are being fixed.
I think @laanwj @MeshCollider @MarcoFalke have seen these failures.
fixes #11673
All calls of check_last_announcement() asserted that the return
value was True. Just assert inside the function instead. This
gives better debug information if the assert fails.
Also only check the contents of the most recent inv and header if
check_last_announcement() is called with the relevant argument.
Fixes to sources of intermittent failure in sendheaders.py
- at the start of test_null_locators(), a new block is generated and
then a getheaders is sent. check_last_accouncement() is called to assert
that the headers message is received. However, the new block triggers an
inv to be sent over both P2P connections, so there's a race. If the inv
is received at the wrong time, the test fails.
- test_null_locators() ends by sending a block to the node under test.
At the start of test_nonnull_locators(), a block is mined and
check_last_announcement() is called to assert that the inv received is
for the same block. That means there's a race: if the inv from the block
sent in test_null_locators() is received at the wrong time, the test
fails.
~Concept ACK, quick read looks good~ 👍 utACK https://github.com/bitcoin/bitcoin/pull/11707/commits/f0c4ab9a7034aca6be83fcb6cd8479cd19a196a2
utACK f0c4ab9
141 | + getblocks_message = msg_getblocks() 142 | + getblocks_message.locator.vHave = locator 143 | + self.send_message(getblocks_message) 144 | + 145 | + def wait_for_getdata(self, hash_list, timeout=60): 146 | + if hash_list != []:
Nit: I generally prefer the "early-return-syntax" (like it was before). This saves some white space when indenting.
sure, reverted
237 | - # length+1 blocks). 238 | - # Note: we clear the state of our p2p connections after the 239 | - # to-be-reorged-out blocks are mined, so that we don't break later tests. 240 | - # return the list of block hashes newly mined 241 | def mine_reorg(self, length): 242 | + """Mine a reorg that invalidates length blocks (replacing them with # length+1 blocks).
nit: loose hashtag from transforming comment into docstring.
3 | @@ -4,7 +4,7 @@ 4 | # file COPYING or http://www.opensource.org/licenses/mit-license.php. 5 | """Test behavior of headers messages to announce blocks. 6 | 7 | -Setup: 8 | +Setup: 9 | 10 | - Two nodes, two p2p connections to node0. One p2p connection should only ever 11 | receive inv's (omitted from testing description below, this is our control).
nit: Not sure what "omitted from testing description below, this is our control" means. Mind to clarify or remove?
I've clarified the language slightly
274 | 275 | def run_test(self): 276 | # Setup the p2p connections and start up the network thread. 277 | - inv_node = self.nodes[0].add_p2p_connection(TestNode()) 278 | + inv_node = self.nodes[0].add_p2p_connection(BaseNode()) 279 | # Set nServices to 0 for test_node, so no block download will occur outside of
nit: Mind to fixup the comment, since segwit is now always active in regtest?
fixed
168 | + with mininode_lock: 169 | + self.block_announced = False 170 | + self.last_message.pop("inv", None) 171 | + self.last_message.pop("headers", None) 172 | + 173 | def check_last_announcement(self, headers=None, inv=None):
nit: Are the default values required for headers and inv? For the case of inv_node, we must only ever send invs and headers==[]. For the case of test_node, we should send headers and inv==[]s or inv and headers==[]. Though, it is not required for the protocol, so feel free to leave as is.
Right, having the default arguments like this allows looser or stricter testing. If the function is called without both arguments, then the testing is looser. We can make it tighter by explicitly calling with one of the arguments set to an empty list.
I think it's ok as is, but I'm also happy to change if you have a better suggestion.
355 | @@ -339,16 +356,16 @@ def test_nonnull_locators(self, test_node, inv_node): 356 | # getdata requests (the check is further down) 357 | inv_node.send_header_for_blocks(blocks) 358 | inv_node.sync_with_ping() 359 | - [ test_node.send_message(msg_block(x)) for x in blocks ] 360 | + [test_node.send_message(msg_block(x)) for x in blocks] 361 | test_node.sync_with_ping() 362 | inv_node.sync_with_ping() 363 | # This block should not be announced to the inv node (since it also 364 | # broadcast it)
nit: test node, not inv node, broadcasts the block.
In this section of the test, both test_node and inv_node announce the new block.
265 | @@ -263,7 +266,10 @@ def test_null_locators(self, test_node): 266 | test_node.send_get_headers(locator=[], hashstop=int(block.hash, 16)) 267 | test_node.sync_with_ping() 268 | assert_equal(test_node.block_announced, False) 269 | + inv_node.clear_last_announcement() 270 | test_node.send_message(msg_block(block)) 271 | + inv_node.check_last_announcement(inv=[int(block.hash, 16)], headers=[]) 272 | + inv_node.clear_last_announcement()
nit: is this required? check_last_announcement already includes the code of clear_last_announcement
not required. Now removed
utACK f0c4ab9a7034aca6be83fcb6cd8479cd19a196a2 on the fix. Left some style comments, since you touched the file anyway. Feel free to leave.
Thanks for the comments @MarcoFalke . I've added a new commit to address them. Let me know if you'd like me to squash commits (it'd be slightly tedious to get the fixups into their appropriate commits, but I'm happy to do it if you want a cleaner git history).
re-utACK 9d42cc333139d7101a9223421d9eabcddfd0b025. I think it is best to leave the fixup commit on its own, so that the previous review is not invalidated by changing the commit hashes.