From 6426716a9940eea0e4d6e53c55282de5de473784 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 29 Mar 2017 14:16:42 -0400 Subject: Add send_await_disconnect() method to p2p-compactblocks.py p2p-compactblocks was incorrectly using sync_with_ping() when sending in invalid block. The node would disconnect us and never respond to the ping, so the sync_with_ping would just time out after 30 seconds and continue with the test. This commit adds a send_await_disconnect() method that sends the message, and then waits for the node to disconnect us. In this commit I've added the method to p2p-compactblocks.py, but a future commit could move it to mininode since it could be useful more generally. This commit reduces the p2p-compactblock runtime by 30 seconds. --- test/functional/p2p-compactblocks.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p-compactblocks.py b/test/functional/p2p-compactblocks.py index 86b767b98a..bf8d113767 100755 --- a/test/functional/p2p-compactblocks.py +++ b/test/functional/p2p-compactblocks.py @@ -32,6 +32,13 @@ class TestNode(NodeConnCB): # This is for synchronizing the p2p message traffic, # so we can eg wait until a particular block is announced. self.set_announced_blockhashes = set() + self.connected = False + + def on_open(self, conn): + self.connected = True + + def on_close(self, conn): + self.connected = False def on_sendcmpct(self, conn, message): self.last_sendcmpct.append(message) @@ -107,6 +114,18 @@ class TestNode(NodeConnCB): return (block_hash in self.set_announced_blockhashes) return wait_until(received_hash, timeout=timeout) + def send_await_disconnect(self, message, timeout=30): + """Sends a message to the node and wait for disconnect. + + This is used when we want to send a message into the node that we expect + will get us disconnected, eg an invalid block.""" + self.send_message(message) + success = wait_until(lambda: not self.connected, timeout=timeout) + if not success: + logger.error("send_await_disconnect failed!") + raise AssertionError("send_await_disconnect failed!") + return success + class CompactBlocksTest(BitcoinTestFramework): def __init__(self): super().__init__() @@ -274,8 +293,8 @@ class CompactBlocksTest(BitcoinTestFramework): # This index will be too high prefilled_txn = PrefilledTransaction(1, block.vtx[0]) cmpct_block.prefilled_txn = [prefilled_txn] - self.test_node.send_and_ping(msg_cmpctblock(cmpct_block)) - assert(int(self.nodes[0].getbestblockhash(), 16) == block.hashPrevBlock) + self.test_node.send_await_disconnect(msg_cmpctblock(cmpct_block)) + assert_equal(int(self.nodes[0].getbestblockhash(), 16), block.hashPrevBlock) # Compare the generated shortids to what we expect based on BIP 152, given # bitcoind's choice of nonce. -- cgit v1.2.3 From 6a18bb9a3603839160dd77b671d5f59d12bd2666 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 29 Mar 2017 11:37:00 -0400 Subject: [tests] sync_with_ping should assert that ping hasn't timed out sync_with_ping currently returns false if the timeout expires, and it is the caller's responsibility to fail the test. However, none of the tests currently assert on sync_with_ping()'s return code. This commit adds an assert to sync_with_ping so the test will fail if the timeout expires. This commit also removes all the duplicate implementations of sync_with_ping() from the individual tests. --- test/functional/assumevalid.py | 3 ++- test/functional/maxuploadtarget.py | 9 --------- test/functional/p2p-acceptblock.py | 15 --------------- test/functional/p2p-mempool.py | 9 --------- test/functional/p2p-segwit.py | 21 +++++++-------------- test/functional/p2p-versionbits-warning.py | 15 --------------- test/functional/test_framework/mininode.py | 5 ++++- 7 files changed, 13 insertions(+), 64 deletions(-) diff --git a/test/functional/assumevalid.py b/test/functional/assumevalid.py index a9c2b17b4a..8e301c4379 100755 --- a/test/functional/assumevalid.py +++ b/test/functional/assumevalid.py @@ -190,7 +190,8 @@ class AssumeValidTest(BitcoinTestFramework): # Send all blocks to node1. All blocks will be accepted. for i in range(2202): node1.send_message(msg_block(self.blocks[i])) - node1.sync_with_ping() # make sure the most recent block is synced + # Syncing 2200 blocks can take a while on slow systems. Give it plenty of time to sync. + node1.sync_with_ping(120) assert_equal(self.nodes[1].getblock(self.nodes[1].getbestblockhash())['height'], 2202) # Send blocks to node2. Block 102 will be rejected. diff --git a/test/functional/maxuploadtarget.py b/test/functional/maxuploadtarget.py index b26c10796d..9b42bf276c 100755 --- a/test/functional/maxuploadtarget.py +++ b/test/functional/maxuploadtarget.py @@ -68,15 +68,6 @@ class TestNode(NodeConnCB): def on_close(self, conn): self.peer_disconnected = True - # Sync up with the node after delivery of a block - def sync_with_ping(self, timeout=30): - def received_pong(): - return (self.last_pong.nonce == self.ping_counter) - self.connection.send_message(msg_ping(nonce=self.ping_counter)) - success = wait_until(received_pong, timeout=timeout) - self.ping_counter += 1 - return success - class MaxUploadTest(BitcoinTestFramework): def __init__(self): diff --git a/test/functional/p2p-acceptblock.py b/test/functional/p2p-acceptblock.py index 7bdbe2bb12..c09945baa6 100755 --- a/test/functional/p2p-acceptblock.py +++ b/test/functional/p2p-acceptblock.py @@ -88,21 +88,6 @@ class TestNode(NodeConnCB): def on_pong(self, conn, message): self.last_pong = message - # Sync up with the node after delivery of a block - def sync_with_ping(self, timeout=30): - self.connection.send_message(msg_ping(nonce=self.ping_counter)) - received_pong = False - sleep_time = 0.05 - while not received_pong and timeout > 0: - time.sleep(sleep_time) - timeout -= sleep_time - with mininode_lock: - if self.last_pong.nonce == self.ping_counter: - received_pong = True - self.ping_counter += 1 - return received_pong - - class AcceptBlockTest(BitcoinTestFramework): def add_options(self, parser): parser.add_option("--testbinary", dest="testbinary", diff --git a/test/functional/p2p-mempool.py b/test/functional/p2p-mempool.py index 3d3a9939d4..5064ce74aa 100755 --- a/test/functional/p2p-mempool.py +++ b/test/functional/p2p-mempool.py @@ -62,15 +62,6 @@ class TestNode(NodeConnCB): def on_close(self, conn): self.peer_disconnected = True - # Sync up with the node after delivery of a block - def sync_with_ping(self, timeout=30): - def received_pong(): - return (self.last_pong.nonce == self.ping_counter) - self.connection.send_message(msg_ping(nonce=self.ping_counter)) - success = wait_until(received_pong, timeout=timeout) - self.ping_counter += 1 - return success - def send_mempool(self): self.lastInv = [] self.send_message(msg_mempool()) diff --git a/test/functional/p2p-segwit.py b/test/functional/p2p-segwit.py index 69f829279c..cd7b788eb4 100755 --- a/test/functional/p2p-segwit.py +++ b/test/functional/p2p-segwit.py @@ -80,13 +80,6 @@ class TestNode(NodeConnCB): timeout -= self.sleep_time raise AssertionError("Sync failed to complete") - def sync_with_ping(self, timeout=60): - self.send_message(msg_ping(nonce=self.ping_counter)) - test_function = lambda: self.last_pong.nonce == self.ping_counter - self.sync(test_function, timeout) - self.ping_counter += 1 - return - def wait_for_block(self, blockhash, timeout=60): test_function = lambda: self.last_block != None and self.last_block.sha256 == blockhash self.sync(test_function, timeout) @@ -148,7 +141,7 @@ class TestNode(NodeConnCB): if with_witness: tx_message = msg_witness_tx(tx) self.send_message(tx_message) - self.sync_with_ping() + self.sync_with_ping(60) assert_equal(tx.hash in self.connection.rpc.getrawmempool(), accepted) if (reason != None and not accepted): # Check the rejection reason as well. @@ -161,7 +154,7 @@ class TestNode(NodeConnCB): self.send_message(msg_witness_block(block)) else: self.send_message(msg_block(block)) - self.sync_with_ping() + self.sync_with_ping(60) assert_equal(self.connection.rpc.getbestblockhash() == block.hash, accepted) @@ -235,7 +228,7 @@ class SegWitTest(BitcoinTestFramework): block = self.build_next_block(nVersion=1) block.solve() self.test_node.send_message(msg_block(block)) - self.test_node.sync_with_ping() # make sure the block was processed + self.test_node.sync_with_ping(60) # make sure the block was processed txid = block.vtx[0].sha256 self.nodes[0].generate(99) # let the block mature @@ -251,7 +244,7 @@ class SegWitTest(BitcoinTestFramework): assert_equal(msg_tx(tx).serialize(), msg_witness_tx(tx).serialize()) self.test_node.send_message(msg_witness_tx(tx)) - self.test_node.sync_with_ping() # make sure the tx was processed + self.test_node.sync_with_ping(60) # make sure the tx was processed assert(tx.hash in self.nodes[0].getrawmempool()) # Save this transaction for later self.utxo.append(UTXO(tx.sha256, 0, 49*100000000)) @@ -291,7 +284,7 @@ class SegWitTest(BitcoinTestFramework): # But it should not be permanently marked bad... # Resend without witness information. self.test_node.send_message(msg_block(block)) - self.test_node.sync_with_ping() + self.test_node.sync_with_ping(60) assert_equal(self.nodes[0].getbestblockhash(), block.hash) sync_blocks(self.nodes) @@ -1257,7 +1250,7 @@ class SegWitTest(BitcoinTestFramework): # Spending a higher version witness output is not allowed by policy, # even with fRequireStandard=false. self.test_node.test_transaction_acceptance(tx3, with_witness=True, accepted=False) - self.test_node.sync_with_ping() + self.test_node.sync_with_ping(60) with mininode_lock: assert(b"reserved for soft-fork upgrades" in self.test_node.last_reject.reason) @@ -1387,7 +1380,7 @@ class SegWitTest(BitcoinTestFramework): for i in range(NUM_TESTS): # Ping regularly to keep the connection alive if (not i % 100): - self.test_node.sync_with_ping() + self.test_node.sync_with_ping(60) # Choose random number of inputs to use. num_inputs = random.randint(1, 10) # Create a slight bias for producing more utxos diff --git a/test/functional/p2p-versionbits-warning.py b/test/functional/p2p-versionbits-warning.py index d4d03861a4..da960e2d80 100755 --- a/test/functional/p2p-versionbits-warning.py +++ b/test/functional/p2p-versionbits-warning.py @@ -46,21 +46,6 @@ class TestNode(NodeConnCB): def on_pong(self, conn, message): self.last_pong = message - # Sync up with the node after delivery of a block - def sync_with_ping(self, timeout=30): - self.connection.send_message(msg_ping(nonce=self.ping_counter)) - received_pong = False - sleep_time = 0.05 - while not received_pong and timeout > 0: - time.sleep(sleep_time) - timeout -= sleep_time - with mininode_lock: - if self.last_pong.nonce == self.ping_counter: - received_pong = True - self.ping_counter += 1 - return received_pong - - class VersionBitsWarningTest(BitcoinTestFramework): def __init__(self): super().__init__() diff --git a/test/functional/test_framework/mininode.py b/test/functional/test_framework/mininode.py index b02ffbe14a..ebb846a237 100755 --- a/test/functional/test_framework/mininode.py +++ b/test/functional/test_framework/mininode.py @@ -1563,11 +1563,14 @@ class NodeConnCB(object): self.sync_with_ping() # Sync up with the node - def sync_with_ping(self, timeout=30): + def sync_with_ping(self, timeout=60): def received_pong(): return (self.last_pong.nonce == self.ping_counter) self.send_message(msg_ping(nonce=self.ping_counter)) success = wait_until(received_pong, timeout=timeout) + if not success: + logger.error("sync_with_ping failed!") + raise AssertionError("sync_with_ping failed!") self.ping_counter += 1 return success -- cgit v1.2.3