Merge #11867: Improve node network test

ee5efad6cf [tests] refactor node_network_limited (John Newbery)
b425131f5a [tests] remove redundant duplicate tests from node_network_limited (John Newbery)
2e02984591 [tests] node_network_limited - remove race condition (John Newbery)
dbfe294805 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery)
1285312048 [tests] fix flake8 warnings in node_network_limited.py (John Newbery)

Pull request description:

  Fixes race condition in the node_network_limited test case introduced in #11740. Also tidies up the test and removes redundant duplicate tests.

Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011

2 files changed, 34 insertions, 56 deletions

diff --git a/test/functional/node_network_limited.py b/test/functional/node_network_limited.py
index 40b876243b..70415e0168 100755
--- a/test/functional/node_network_limited.py
+++ b/test/functional/node_network_limited.py
@@ -2,14 +2,26 @@
 # Copyright (c) 2017 The Bitcoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Tests NODE_NETWORK_LIMITED.
+
+Tests that a node configured with -prune=550 signals NODE_NETWORK_LIMITED correctly
+and that it responds to getdata requests for blocks correctly:
+    - send a block within 288 + 2 of the tip
+    - disconnect peers who request blocks older than that."""
+from test_framework.messages import CInv, msg_getdata
+from test_framework.mininode import NODE_BLOOM, NODE_NETWORK_LIMITED, NODE_WITNESS, NetworkThread, P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import *
-from test_framework.mininode import *
+from test_framework.util import assert_equal
 
-class BaseNode(P2PInterface):
-    nServices = 0
-    def on_version(self, message):
-        self.nServices = message.nServices
+class P2PIgnoreInv(P2PInterface):
+    def on_inv(self, message):
+        # The node will send us invs for other blocks. Ignore them.
+        pass
+
+    def send_getdata_for_block(self, blockhash):
+        getdata_request = msg_getdata()
+        getdata_request.inv.append(CInv(2, int(blockhash, 16)))
+        self.send_message(getdata_request)
 
 class NodeNetworkLimitedTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -17,64 +29,29 @@ class NodeNetworkLimitedTest(BitcoinTestFramework):
         self.num_nodes = 1
         self.extra_args = [['-prune=550']]
 
-    def getSignaledServiceFlags(self):
-        node = self.nodes[0].add_p2p_connection(BaseNode())
+    def run_test(self):
+        node = self.nodes[0].add_p2p_connection(P2PIgnoreInv())
         NetworkThread().start()
         node.wait_for_verack()
-        services = node.nServices
-        self.nodes[0].disconnect_p2ps()
-        node.wait_for_disconnect()
-        return services
 
-    def tryGetBlockViaGetData(self, blockhash, must_disconnect):
-        node = self.nodes[0].add_p2p_connection(BaseNode())
-        NetworkThread().start()
-        node.wait_for_verack()
-        node.send_message(msg_verack())
-        getdata_request = msg_getdata()
-        getdata_request.inv.append(CInv(2, int(blockhash, 16)))
-        node.send_message(getdata_request)
+        expected_services = NODE_BLOOM | NODE_WITNESS | NODE_NETWORK_LIMITED
 
-        if (must_disconnect):
-            #ensure we get disconnected
-            node.wait_for_disconnect(5)
-        else:
-            # check if the peer sends us the requested block
-            node.wait_for_block(int(blockhash, 16), 3)
-            self.nodes[0].disconnect_p2ps()
-            node.wait_for_disconnect()
+        self.log.info("Check that node has signalled expected services.")
+        assert_equal(node.nServices, expected_services)
 
-    def run_test(self):
-        #NODE_BLOOM & NODE_WITNESS & NODE_NETWORK_LIMITED must now be signaled
-        assert_equal(self.getSignaledServiceFlags(), 1036) #1036 == 0x40C == 0100 0000 1100
-#                                                                              |        ||
-#                                                                              |        |^--- NODE_BLOOM
-#                                                                              |        ^---- NODE_WITNESS
-#                                                                              ^-- NODE_NETWORK_LIMITED
+        self.log.info("Check that the localservices is as expected.")
+        assert_equal(int(self.nodes[0].getnetworkinfo()['localservices'], 16), expected_services)
 
-        #now mine some blocks over the NODE_NETWORK_LIMITED + 2(racy buffer ext.) target
-        firstblock = self.nodes[0].generate(1)[0]
+        self.log.info("Mine enough blocks to reach the NODE_NETWORK_LIMITED range.")
         blocks = self.nodes[0].generate(292)
-        blockWithinLimitedRange = blocks[-1]
-
-        #make sure we can max retrive block at tip-288
-        #requesting block at height 2 (tip-289) must fail (ignored)
-        self.tryGetBlockViaGetData(firstblock, True) #first block must lead to disconnect
-        self.tryGetBlockViaGetData(blocks[1], False) #last block in valid range
-        self.tryGetBlockViaGetData(blocks[0], True) #first block outside of the 288+2 limit
-
-        #NODE_NETWORK_LIMITED must still be signaled after restart
-        self.restart_node(0)
-        assert_equal(self.getSignaledServiceFlags(), 1036)
-
-        #test the RPC service flags
-        assert_equal(self.nodes[0].getnetworkinfo()['localservices'], "000000000000040c")
 
-        # getdata a block above the NODE_NETWORK_LIMITED threshold must be possible
-        self.tryGetBlockViaGetData(blockWithinLimitedRange, False)
+        self.log.info("Make sure we can max retrive block at tip-288.")
+        node.send_getdata_for_block(blocks[1])  # last block in valid range
+        node.wait_for_block(int(blocks[1], 16), timeout=3)
 
-        # getdata a block below the NODE_NETWORK_LIMITED threshold must be ignored
-        self.tryGetBlockViaGetData(firstblock, True)
+        self.log.info("Requesting block at height 2 (tip-289) must fail (ignored).")
+        node.send_getdata_for_block(blocks[0])  # first block outside of the 288+2 limit
+        node.wait_for_disconnect(5)
 
 if __name__ == '__main__':
     NodeNetworkLimitedTest().main()
diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py
index 2ab1bdac0f..d8032e4430 100644
--- a/test/functional/test_framework/messages.py
+++ b/test/functional/test_framework/messages.py
@@ -38,10 +38,11 @@ COIN = 100000000 # 1 btc in satoshis
 
 NODE_NETWORK = (1 << 0)
 # NODE_GETUTXO = (1 << 1)
-# NODE_BLOOM = (1 << 2)
+NODE_BLOOM = (1 << 2)
 NODE_WITNESS = (1 << 3)
 NODE_UNSUPPORTED_SERVICE_BIT_5 = (1 << 5)
 NODE_UNSUPPORTED_SERVICE_BIT_7 = (1 << 7)
+NODE_NETWORK_LIMITED = (1 << 10)
 
 # Serialization/deserialization tools
 def sha256(s):