From 2613c545f529f7c91462c12831d41dcb164bd0e2 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 16 Nov 2017 09:53:35 -0500 Subject: [PATCH 1/5] [tests] fix flake8 warnings in sendheaders.py --- test/functional/sendheaders.py | 92 ++++++++++++++++++++-------------- 1 file changed, 54 insertions(+), 38 deletions(-) diff --git a/test/functional/sendheaders.py b/test/functional/sendheaders.py index 55bb80ea0..8fdbcc6c0 100755 --- a/test/functional/sendheaders.py +++ b/test/functional/sendheaders.py @@ -4,7 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test behavior of headers messages to announce blocks. -Setup: +Setup: - Two nodes, two p2p connections to node0. One p2p connection should only ever receive inv's (omitted from testing description below, this is our control). @@ -83,16 +83,32 @@ d. Announce 49 headers that don't connect. e. Announce one more that doesn't connect. Expect: disconnect. """ - -from test_framework.mininode import * -from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import * from test_framework.blocktools import create_block, create_coinbase +from test_framework.mininode import ( + CBlockHeader, + CInv, + NODE_WITNESS, + NetworkThread, + NodeConnCB, + mininode_lock, + msg_block, + msg_getblocks, + msg_getdata, + msg_getheaders, + msg_headers, + msg_inv, + msg_sendheaders, +) +from test_framework.test_framework import BitcoinTestFramework +from test_framework.util import ( + assert_equal, + sync_blocks, + wait_until, +) +DIRECT_FETCH_RESPONSE_TIME = 0.05 -direct_fetch_response_time = 0.05 - -class TestNode(NodeConnCB): +class BaseNode(NodeConnCB): def __init__(self): super().__init__() self.block_announced = False @@ -136,8 +152,8 @@ class TestNode(NodeConnCB): # right header or the right inv # inv and headers should be lists of block hashes def check_last_announcement(self, headers=None, inv=None): - expect_headers = headers if headers != None else [] - expect_inv = inv if inv != None else [] + expect_headers = headers if headers is not None else [] + expect_inv = inv if inv is not None else [] test_function = lambda: self.block_announced wait_until(test_function, timeout=60, lock=mininode_lock) with mininode_lock: @@ -153,7 +169,7 @@ class TestNode(NodeConnCB): hash_headers = [] if "headers" in self.last_message: # treat headers as a list of block hashes - hash_headers = [ x.sha256 for x in self.last_message["headers"].headers ] + hash_headers = [x.sha256 for x in self.last_message["headers"].headers] if hash_headers != expect_headers: success = False @@ -176,7 +192,7 @@ class TestNode(NodeConnCB): def send_header_for_blocks(self, new_blocks): headers_message = msg_headers() - headers_message.headers = [ CBlockHeader(b) for b in new_blocks ] + headers_message.headers = [CBlockHeader(b) for b in new_blocks] self.send_message(headers_message) def send_getblocks(self, locator): @@ -202,27 +218,27 @@ class SendHeadersTest(BitcoinTestFramework): # to-be-reorged-out blocks are mined, so that we don't break later tests. # return the list of block hashes newly mined def mine_reorg(self, length): - self.nodes[0].generate(length) # make sure all invalidated blocks are node0's + self.nodes[0].generate(length) # make sure all invalidated blocks are node0's sync_blocks(self.nodes, wait=0.1) for x in self.nodes[0].p2ps: x.wait_for_block_announcement(int(self.nodes[0].getbestblockhash(), 16)) x.clear_last_announcement() tip_height = self.nodes[1].getblockcount() - hash_to_invalidate = self.nodes[1].getblockhash(tip_height-(length-1)) + hash_to_invalidate = self.nodes[1].getblockhash(tip_height - (length - 1)) self.nodes[1].invalidateblock(hash_to_invalidate) - all_hashes = self.nodes[1].generate(length+1) # Must be longer than the orig chain + all_hashes = self.nodes[1].generate(length + 1) # Must be longer than the orig chain sync_blocks(self.nodes, wait=0.1) return [int(x, 16) for x in all_hashes] def run_test(self): # Setup the p2p connections and start up the network thread. - inv_node = self.nodes[0].add_p2p_connection(TestNode()) + inv_node = self.nodes[0].add_p2p_connection(BaseNode()) # Set nServices to 0 for test_node, so no block download will occur outside of # direct fetching - test_node = self.nodes[0].add_p2p_connection(TestNode(), services=NODE_WITNESS) + test_node = self.nodes[0].add_p2p_connection(BaseNode(), services=NODE_WITNESS) - NetworkThread().start() # Start up network handling in another thread + NetworkThread().start() # Start up network handling in another thread # Test logic begins here inv_node.wait_for_verack() @@ -275,18 +291,18 @@ class SendHeadersTest(BitcoinTestFramework): test_node.get_headers(locator=[old_tip], hashstop=tip) test_node.get_data([tip]) test_node.wait_for_block(tip) - test_node.clear_last_announcement() # since we requested headers... + test_node.clear_last_announcement() # since we requested headers... elif i == 2: # this time announce own block via headers height = self.nodes[0].getblockcount() last_time = self.nodes[0].getblock(self.nodes[0].getbestblockhash())['time'] block_time = last_time + 1 - new_block = create_block(tip, create_coinbase(height+1), block_time) + new_block = create_block(tip, create_coinbase(height + 1), block_time) new_block.solve() test_node.send_header_for_blocks([new_block]) test_node.wait_for_getdata([new_block.sha256]) test_node.send_message(msg_block(new_block)) - test_node.sync_with_ping() # make sure this block is processed + test_node.sync_with_ping() # make sure this block is processed inv_node.clear_last_announcement() test_node.clear_last_announcement() @@ -305,7 +321,7 @@ class SendHeadersTest(BitcoinTestFramework): assert_equal(inv_node.check_last_announcement(inv=[tip]), True) assert_equal(test_node.check_last_announcement(headers=[tip]), True) - height = self.nodes[0].getblockcount()+1 + height = self.nodes[0].getblockcount() + 1 block_time += 10 # Advance far enough ahead for i in range(10): # Mine i blocks, and alternate announcing either via @@ -314,7 +330,7 @@ class SendHeadersTest(BitcoinTestFramework): # with block header, even though the blocks are never requested for j in range(2): blocks = [] - for b in range(i+1): + for b in range(i + 1): blocks.append(create_block(tip, create_coinbase(height), block_time)) blocks[-1].solve() tip = blocks[-1].sha256 @@ -328,7 +344,7 @@ class SendHeadersTest(BitcoinTestFramework): test_node.send_header_for_blocks(blocks) # Test that duplicate inv's won't result in duplicate # getdata requests, or duplicate headers announcements - [ inv_node.send_block_inv(x.sha256) for x in blocks ] + [inv_node.send_block_inv(x.sha256) for x in blocks] test_node.wait_for_getdata([x.sha256 for x in blocks]) inv_node.sync_with_ping() else: @@ -339,7 +355,7 @@ class SendHeadersTest(BitcoinTestFramework): # getdata requests (the check is further down) inv_node.send_header_for_blocks(blocks) inv_node.sync_with_ping() - [ test_node.send_message(msg_block(x)) for x in blocks ] + [test_node.send_message(msg_block(x)) for x in blocks] test_node.sync_with_ping() inv_node.sync_with_ping() # This block should not be announced to the inv node (since it also @@ -365,7 +381,7 @@ class SendHeadersTest(BitcoinTestFramework): assert_equal(inv_node.check_last_announcement(inv=[tip]), True) assert_equal(test_node.check_last_announcement(headers=new_block_hashes), True) - block_time += 8 + block_time += 8 # Mine a too-large reorg, which should be announced with a single inv new_block_hashes = self.mine_reorg(length=8) @@ -379,7 +395,7 @@ class SendHeadersTest(BitcoinTestFramework): fork_point = int(fork_point, 16) # Use getblocks/getdata - test_node.send_getblocks(locator = [fork_point]) + test_node.send_getblocks(locator=[fork_point]) assert_equal(test_node.check_last_announcement(inv=new_block_hashes), True) test_node.get_data(new_block_hashes) test_node.wait_for_block(new_block_hashes[-1]) @@ -403,7 +419,7 @@ class SendHeadersTest(BitcoinTestFramework): test_node.get_data([tip]) test_node.wait_for_block(tip) # This time, try sending either a getheaders to trigger resumption - # of headers announcements, or mine a new block and inv it, also + # of headers announcements, or mine a new block and inv it, also # triggering resumption of headers announcements. if j == 0: test_node.get_headers(locator=[tip], hashstop=0) @@ -434,7 +450,7 @@ class SendHeadersTest(BitcoinTestFramework): height += 1 inv_node.send_message(msg_block(blocks[-1])) - inv_node.sync_with_ping() # Make sure blocks are processed + inv_node.sync_with_ping() # Make sure blocks are processed test_node.last_message.pop("getdata", None) test_node.send_header_for_blocks(blocks) test_node.sync_with_ping() @@ -453,9 +469,9 @@ class SendHeadersTest(BitcoinTestFramework): test_node.send_header_for_blocks(blocks) test_node.sync_with_ping() - test_node.wait_for_getdata([x.sha256 for x in blocks], timeout=direct_fetch_response_time) + test_node.wait_for_getdata([x.sha256 for x in blocks], timeout=DIRECT_FETCH_RESPONSE_TIME) - [ test_node.send_message(msg_block(x)) for x in blocks ] + [test_node.send_message(msg_block(x)) for x in blocks] test_node.sync_with_ping() @@ -484,13 +500,13 @@ class SendHeadersTest(BitcoinTestFramework): # both blocks (same work as tip) test_node.send_header_for_blocks(blocks[1:2]) test_node.sync_with_ping() - test_node.wait_for_getdata([x.sha256 for x in blocks[0:2]], timeout=direct_fetch_response_time) + test_node.wait_for_getdata([x.sha256 for x in blocks[0:2]], timeout=DIRECT_FETCH_RESPONSE_TIME) # Announcing 16 more headers should trigger direct fetch for 14 more # blocks test_node.send_header_for_blocks(blocks[2:18]) test_node.sync_with_ping() - test_node.wait_for_getdata([x.sha256 for x in blocks[2:16]], timeout=direct_fetch_response_time) + test_node.wait_for_getdata([x.sha256 for x in blocks[2:16]], timeout=DIRECT_FETCH_RESPONSE_TIME) # Announcing 1 more header should not trigger any response test_node.last_message.pop("getdata", None) @@ -502,7 +518,7 @@ class SendHeadersTest(BitcoinTestFramework): self.log.info("Part 4: success!") # Now deliver all those blocks we announced. - [ test_node.send_message(msg_block(x)) for x in blocks ] + [test_node.send_message(msg_block(x)) for x in blocks] self.log.info("Part 5: Testing handling of unconnecting headers") # First we test that receipt of an unconnecting header doesn't prevent @@ -524,7 +540,7 @@ class SendHeadersTest(BitcoinTestFramework): test_node.wait_for_getheaders() test_node.send_header_for_blocks(blocks) test_node.wait_for_getdata([x.sha256 for x in blocks]) - [ test_node.send_message(msg_block(x)) for x in blocks ] + [test_node.send_message(msg_block(x)) for x in blocks] test_node.sync_with_ping() assert_equal(int(self.nodes[0].getbestblockhash(), 16), blocks[1].sha256) @@ -532,7 +548,7 @@ class SendHeadersTest(BitcoinTestFramework): # Now we test that if we repeatedly don't send connecting headers, we # don't go into an infinite loop trying to get them to connect. MAX_UNCONNECTING_HEADERS = 10 - for j in range(MAX_UNCONNECTING_HEADERS+1): + for j in range(MAX_UNCONNECTING_HEADERS + 1): blocks.append(create_block(tip, create_coinbase(height), block_time)) blocks[-1].solve() tip = blocks[-1].sha256 @@ -554,11 +570,11 @@ class SendHeadersTest(BitcoinTestFramework): # Now try to see how many unconnecting headers we can send # before we get disconnected. Should be 5*MAX_UNCONNECTING_HEADERS - for i in range(5*MAX_UNCONNECTING_HEADERS - 1): + for i in range(5 * MAX_UNCONNECTING_HEADERS - 1): # Send a header that doesn't connect, check that we get a getheaders. with mininode_lock: test_node.last_message.pop("getheaders", None) - test_node.send_header_for_blocks([blocks[i%len(blocks)]]) + test_node.send_header_for_blocks([blocks[i % len(blocks)]]) test_node.wait_for_getheaders() # Eventually this stops working. From f39d4bbd1e328cb04a6ddb133511385491a90d84 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 16 Nov 2017 11:07:40 -0500 Subject: [PATCH 2/5] [tests] tidy up BaseNode in sendheaders.py --- test/functional/sendheaders.py | 104 ++++++++++++++++----------------- 1 file changed, 51 insertions(+), 53 deletions(-) diff --git a/test/functional/sendheaders.py b/test/functional/sendheaders.py index 8fdbcc6c0..5c8b70a70 100755 --- a/test/functional/sendheaders.py +++ b/test/functional/sendheaders.py @@ -114,20 +114,14 @@ class BaseNode(NodeConnCB): self.block_announced = False self.last_blockhash_announced = None - def clear_last_announcement(self): - with mininode_lock: - self.block_announced = False - self.last_message.pop("inv", None) - self.last_message.pop("headers", None) - - # Request data for a list of block hashes - def get_data(self, block_hashes): + def send_get_data(self, block_hashes): + """Request data for a list of block hashes.""" msg = msg_getdata() for x in block_hashes: msg.inv.append(CInv(2, x)) self.connection.send_message(msg) - def get_headers(self, locator, hashstop): + def send_get_headers(self, locator, hashstop): msg = msg_getheaders() msg.locator.vHave = locator msg.hashstop = hashstop @@ -138,6 +132,25 @@ class BaseNode(NodeConnCB): msg.inv = [CInv(2, blockhash)] self.connection.send_message(msg) + def send_header_for_blocks(self, new_blocks): + headers_message = msg_headers() + headers_message.headers = [CBlockHeader(b) for b in new_blocks] + self.send_message(headers_message) + + def send_getblocks(self, locator): + getblocks_message = msg_getblocks() + getblocks_message.locator.vHave = locator + self.send_message(getblocks_message) + + def wait_for_getdata(self, hash_list, timeout=60): + if hash_list != []: + test_function = lambda: "getdata" in self.last_message and [x.hash for x in self.last_message["getdata"].inv] == hash_list + wait_until(test_function, timeout=timeout, lock=mininode_lock) + + def wait_for_block_announcement(self, block_hash, timeout=60): + test_function = lambda: self.last_blockhash_announced == block_hash + wait_until(test_function, timeout=timeout, lock=mininode_lock) + def on_inv(self, conn, message): self.block_announced = True self.last_blockhash_announced = message.inv[-1].hash @@ -148,10 +161,16 @@ class BaseNode(NodeConnCB): message.headers[-1].calc_sha256() self.last_blockhash_announced = message.headers[-1].sha256 - # Test whether the last announcement we received had the - # right header or the right inv - # inv and headers should be lists of block hashes + def clear_last_announcement(self): + with mininode_lock: + self.block_announced = False + self.last_message.pop("inv", None) + self.last_message.pop("headers", None) + def check_last_announcement(self, headers=None, inv=None): + """Test whether the last announcement received had the right header or the right inv. + + inv and headers should be lists of block hashes.""" expect_headers = headers if headers is not None else [] expect_inv = inv if inv is not None else [] test_function = lambda: self.block_announced @@ -177,47 +196,26 @@ class BaseNode(NodeConnCB): self.last_message.pop("headers", None) return success - def wait_for_getdata(self, hash_list, timeout=60): - if hash_list == []: - return - - test_function = lambda: "getdata" in self.last_message and [x.hash for x in self.last_message["getdata"].inv] == hash_list - wait_until(test_function, timeout=timeout, lock=mininode_lock) - return - - def wait_for_block_announcement(self, block_hash, timeout=60): - test_function = lambda: self.last_blockhash_announced == block_hash - wait_until(test_function, timeout=timeout, lock=mininode_lock) - return - - def send_header_for_blocks(self, new_blocks): - headers_message = msg_headers() - headers_message.headers = [CBlockHeader(b) for b in new_blocks] - self.send_message(headers_message) - - def send_getblocks(self, locator): - getblocks_message = msg_getblocks() - getblocks_message.locator.vHave = locator - self.send_message(getblocks_message) - class SendHeadersTest(BitcoinTestFramework): def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 2 - # mine count blocks and return the new tip def mine_blocks(self, count): + """Mine count blocks and return the new tip.""" + # Clear out last block announcement from each p2p listener [x.clear_last_announcement() for x in self.nodes[0].p2ps] self.nodes[0].generate(count) return int(self.nodes[0].getbestblockhash(), 16) - # mine a reorg that invalidates length blocks (replacing them with - # length+1 blocks). - # Note: we clear the state of our p2p connections after the - # to-be-reorged-out blocks are mined, so that we don't break later tests. - # return the list of block hashes newly mined def mine_reorg(self, length): + """Mine a reorg that invalidates length blocks (replacing them with # length+1 blocks). + + Note: we clear the state of our p2p connections after the + to-be-reorged-out blocks are mined, so that we don't break later tests. + return the list of block hashes newly mined.""" + self.nodes[0].generate(length) # make sure all invalidated blocks are node0's sync_blocks(self.nodes, wait=0.1) for x in self.nodes[0].p2ps: @@ -257,7 +255,7 @@ class SendHeadersTest(BitcoinTestFramework): self.log.info("Verify getheaders with null locator and valid hashstop returns headers.") test_node.clear_last_announcement() - test_node.get_headers(locator=[], hashstop=tip_hash) + test_node.send_get_headers(locator=[], hashstop=tip_hash) assert_equal(test_node.check_last_announcement(headers=[tip_hash]), True) self.log.info("Verify getheaders with null locator and invalid hashstop does not return headers.") @@ -265,7 +263,7 @@ class SendHeadersTest(BitcoinTestFramework): block.solve() test_node.send_header_for_blocks([block]) test_node.clear_last_announcement() - test_node.get_headers(locator=[], hashstop=int(block.hash, 16)) + test_node.send_get_headers(locator=[], hashstop=int(block.hash, 16)) test_node.sync_with_ping() assert_equal(test_node.block_announced, False) test_node.send_message(msg_block(block)) @@ -284,12 +282,12 @@ class SendHeadersTest(BitcoinTestFramework): # Try a few different responses; none should affect next announcement if i == 0: # first request the block - test_node.get_data([tip]) + test_node.send_get_data([tip]) test_node.wait_for_block(tip) elif i == 1: # next try requesting header and block - test_node.get_headers(locator=[old_tip], hashstop=tip) - test_node.get_data([tip]) + test_node.send_get_headers(locator=[old_tip], hashstop=tip) + test_node.send_get_data([tip]) test_node.wait_for_block(tip) test_node.clear_last_announcement() # since we requested headers... elif i == 2: @@ -313,7 +311,7 @@ class SendHeadersTest(BitcoinTestFramework): # commence and keep working. test_node.send_message(msg_sendheaders()) prev_tip = int(self.nodes[0].getbestblockhash(), 16) - test_node.get_headers(locator=[prev_tip], hashstop=0) + test_node.send_get_headers(locator=[prev_tip], hashstop=0) test_node.sync_with_ping() # Now that we've synced headers, headers announcements should work @@ -397,7 +395,7 @@ class SendHeadersTest(BitcoinTestFramework): # Use getblocks/getdata test_node.send_getblocks(locator=[fork_point]) assert_equal(test_node.check_last_announcement(inv=new_block_hashes), True) - test_node.get_data(new_block_hashes) + test_node.send_get_data(new_block_hashes) test_node.wait_for_block(new_block_hashes[-1]) for i in range(3): @@ -407,22 +405,22 @@ class SendHeadersTest(BitcoinTestFramework): assert_equal(test_node.check_last_announcement(inv=[tip]), True) if i == 0: # Just get the data -- shouldn't cause headers announcements to resume - test_node.get_data([tip]) + test_node.send_get_data([tip]) test_node.wait_for_block(tip) elif i == 1: # Send a getheaders message that shouldn't trigger headers announcements # to resume (best header sent will be too old) - test_node.get_headers(locator=[fork_point], hashstop=new_block_hashes[1]) - test_node.get_data([tip]) + test_node.send_get_headers(locator=[fork_point], hashstop=new_block_hashes[1]) + test_node.send_get_data([tip]) test_node.wait_for_block(tip) elif i == 2: - test_node.get_data([tip]) + test_node.send_get_data([tip]) test_node.wait_for_block(tip) # This time, try sending either a getheaders to trigger resumption # of headers announcements, or mine a new block and inv it, also # triggering resumption of headers announcements. if j == 0: - test_node.get_headers(locator=[tip], hashstop=0) + test_node.send_get_headers(locator=[tip], hashstop=0) test_node.sync_with_ping() else: test_node.send_block_inv(tip) From 25fd6e2c202c113a2413ba9df76fe0ea0e4cbe5c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 16 Nov 2017 11:52:45 -0500 Subject: [PATCH 3/5] [tests] refactor check_last_announcement() in sendheaders.py 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. --- test/functional/sendheaders.py | 51 ++++++++++++++++------------------ 1 file changed, 24 insertions(+), 27 deletions(-) diff --git a/test/functional/sendheaders.py b/test/functional/sendheaders.py index 5c8b70a70..1a585dd11 100755 --- a/test/functional/sendheaders.py +++ b/test/functional/sendheaders.py @@ -171,30 +171,27 @@ class BaseNode(NodeConnCB): """Test whether the last announcement received had the right header or the right inv. inv and headers should be lists of block hashes.""" - expect_headers = headers if headers is not None else [] - expect_inv = inv if inv is not None else [] + test_function = lambda: self.block_announced wait_until(test_function, timeout=60, lock=mininode_lock) + with mininode_lock: self.block_announced = False - success = True compare_inv = [] if "inv" in self.last_message: compare_inv = [x.hash for x in self.last_message["inv"].inv] - if compare_inv != expect_inv: - success = False + if inv is not None: + assert_equal(compare_inv, inv) - hash_headers = [] + compare_headers = [] if "headers" in self.last_message: - # treat headers as a list of block hashes - hash_headers = [x.sha256 for x in self.last_message["headers"].headers] - if hash_headers != expect_headers: - success = False + compare_headers = [x.sha256 for x in self.last_message["headers"].headers] + if headers is not None: + assert_equal(compare_headers, headers) self.last_message.pop("inv", None) self.last_message.pop("headers", None) - return success class SendHeadersTest(BitcoinTestFramework): def set_test_params(self): @@ -256,7 +253,7 @@ class SendHeadersTest(BitcoinTestFramework): self.log.info("Verify getheaders with null locator and valid hashstop returns headers.") test_node.clear_last_announcement() test_node.send_get_headers(locator=[], hashstop=tip_hash) - assert_equal(test_node.check_last_announcement(headers=[tip_hash]), True) + test_node.check_last_announcement(headers=[tip_hash]) self.log.info("Verify getheaders with null locator and invalid hashstop does not return headers.") block = create_block(int(tip["hash"], 16), create_coinbase(tip["height"] + 1), tip["mediantime"] + 1) @@ -277,8 +274,8 @@ class SendHeadersTest(BitcoinTestFramework): for i in range(4): old_tip = tip tip = self.mine_blocks(1) - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(inv=[tip]), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(inv=[tip], headers=[]) # Try a few different responses; none should affect next announcement if i == 0: # first request the block @@ -316,8 +313,8 @@ class SendHeadersTest(BitcoinTestFramework): # Now that we've synced headers, headers announcements should work tip = self.mine_blocks(1) - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(headers=[tip]), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(headers=[tip]) height = self.nodes[0].getblockcount() + 1 block_time += 10 # Advance far enough ahead @@ -361,8 +358,8 @@ class SendHeadersTest(BitcoinTestFramework): assert "inv" not in inv_node.last_message assert "headers" not in inv_node.last_message tip = self.mine_blocks(1) - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(headers=[tip]), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(headers=[tip]) height += 1 block_time += 1 @@ -376,16 +373,16 @@ class SendHeadersTest(BitcoinTestFramework): # First try mining a reorg that can propagate with header announcement new_block_hashes = self.mine_reorg(length=7) tip = new_block_hashes[-1] - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(headers=new_block_hashes), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(headers=new_block_hashes) block_time += 8 # Mine a too-large reorg, which should be announced with a single inv new_block_hashes = self.mine_reorg(length=8) tip = new_block_hashes[-1] - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(inv=[tip]), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(inv=[tip], headers=[]) block_time += 9 @@ -394,15 +391,15 @@ class SendHeadersTest(BitcoinTestFramework): # Use getblocks/getdata test_node.send_getblocks(locator=[fork_point]) - assert_equal(test_node.check_last_announcement(inv=new_block_hashes), True) + test_node.check_last_announcement(inv=new_block_hashes, headers=[]) test_node.send_get_data(new_block_hashes) test_node.wait_for_block(new_block_hashes[-1]) for i in range(3): # Mine another block, still should get only an inv tip = self.mine_blocks(1) - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(inv=[tip]), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(inv=[tip], headers=[]) if i == 0: # Just get the data -- shouldn't cause headers announcements to resume test_node.send_get_data([tip]) @@ -427,8 +424,8 @@ class SendHeadersTest(BitcoinTestFramework): test_node.sync_with_ping() # New blocks should now be announced with header tip = self.mine_blocks(1) - assert_equal(inv_node.check_last_announcement(inv=[tip]), True) - assert_equal(test_node.check_last_announcement(headers=[tip]), True) + inv_node.check_last_announcement(inv=[tip], headers=[]) + test_node.check_last_announcement(headers=[tip]) self.log.info("Part 3: success!") From f0c4ab9a7034aca6be83fcb6cd8479cd19a196a2 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 16 Nov 2017 11:52:59 -0500 Subject: [PATCH 4/5] [tests] fix flakiness in sendheaders.py 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. --- test/functional/sendheaders.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/test/functional/sendheaders.py b/test/functional/sendheaders.py index 1a585dd11..056471370 100755 --- a/test/functional/sendheaders.py +++ b/test/functional/sendheaders.py @@ -243,13 +243,16 @@ class SendHeadersTest(BitcoinTestFramework): inv_node.sync_with_ping() test_node.sync_with_ping() - self.test_null_locators(test_node) + self.test_null_locators(test_node, inv_node) self.test_nonnull_locators(test_node, inv_node) - def test_null_locators(self, test_node): + def test_null_locators(self, test_node, inv_node): tip = self.nodes[0].getblockheader(self.nodes[0].generate(1)[0]) tip_hash = int(tip["hash"], 16) + inv_node.check_last_announcement(inv=[tip_hash], headers=[]) + test_node.check_last_announcement(inv=[tip_hash], headers=[]) + self.log.info("Verify getheaders with null locator and valid hashstop returns headers.") test_node.clear_last_announcement() test_node.send_get_headers(locator=[], hashstop=tip_hash) @@ -263,7 +266,10 @@ class SendHeadersTest(BitcoinTestFramework): test_node.send_get_headers(locator=[], hashstop=int(block.hash, 16)) test_node.sync_with_ping() assert_equal(test_node.block_announced, False) + inv_node.clear_last_announcement() test_node.send_message(msg_block(block)) + inv_node.check_last_announcement(inv=[int(block.hash, 16)], headers=[]) + inv_node.clear_last_announcement() def test_nonnull_locators(self, test_node, inv_node): tip = int(self.nodes[0].getbestblockhash(), 16) From 9d42cc333139d7101a9223421d9eabcddfd0b025 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 17 Nov 2017 17:15:28 -0500 Subject: [PATCH 5/5] [tests] address review comments --- test/functional/sendheaders.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/test/functional/sendheaders.py b/test/functional/sendheaders.py index 056471370..68c0d95b4 100755 --- a/test/functional/sendheaders.py +++ b/test/functional/sendheaders.py @@ -6,9 +6,11 @@ Setup: -- Two nodes, two p2p connections to node0. One p2p connection should only ever - receive inv's (omitted from testing description below, this is our control). - Second node is used for creating reorgs. +- Two nodes: + - node0 is the node-under-test. We create two p2p connections to it. The + first p2p connection is a control and should only ever receive inv's. The + second p2p connection tests the headers sending logic. + - node1 is used to create reorgs. test_null_locators ================== @@ -143,9 +145,11 @@ class BaseNode(NodeConnCB): self.send_message(getblocks_message) def wait_for_getdata(self, hash_list, timeout=60): - if hash_list != []: - test_function = lambda: "getdata" in self.last_message and [x.hash for x in self.last_message["getdata"].inv] == hash_list - wait_until(test_function, timeout=timeout, lock=mininode_lock) + if hash_list == []: + return + + test_function = lambda: "getdata" in self.last_message and [x.hash for x in self.last_message["getdata"].inv] == hash_list + wait_until(test_function, timeout=timeout, lock=mininode_lock) def wait_for_block_announcement(self, block_hash, timeout=60): test_function = lambda: self.last_blockhash_announced == block_hash @@ -229,8 +233,8 @@ class SendHeadersTest(BitcoinTestFramework): def run_test(self): # Setup the p2p connections and start up the network thread. inv_node = self.nodes[0].add_p2p_connection(BaseNode()) - # Set nServices to 0 for test_node, so no block download will occur outside of - # direct fetching + # Make sure NODE_NETWORK is not set for test_node, so no block download + # will occur outside of direct fetching test_node = self.nodes[0].add_p2p_connection(BaseNode(), services=NODE_WITNESS) NetworkThread().start() # Start up network handling in another thread @@ -269,7 +273,6 @@ class SendHeadersTest(BitcoinTestFramework): inv_node.clear_last_announcement() test_node.send_message(msg_block(block)) inv_node.check_last_announcement(inv=[int(block.hash, 16)], headers=[]) - inv_node.clear_last_announcement() def test_nonnull_locators(self, test_node, inv_node): tip = int(self.nodes[0].getbestblockhash(), 16)