From 73d737211536de5b834f21016c5549e52de11374 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Mon, 4 Dec 2023 18:21:26 +0100 Subject: [PATCH 1/3] test: verify `-maxuploadtarget` limit state via `getnettotals` RPC result --- test/functional/feature_maxuploadtarget.py | 28 ++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/test/functional/feature_maxuploadtarget.py b/test/functional/feature_maxuploadtarget.py index c551c0b449f..83c168404e2 100755 --- a/test/functional/feature_maxuploadtarget.py +++ b/test/functional/feature_maxuploadtarget.py @@ -27,6 +27,9 @@ from test_framework.util import ( from test_framework.wallet import MiniWallet +UPLOAD_TARGET_MB = 800 + + class TestP2PConn(P2PInterface): def __init__(self): super().__init__() @@ -45,12 +48,21 @@ class MaxUploadTest(BitcoinTestFramework): self.setup_clean_chain = True self.num_nodes = 1 self.extra_args = [[ - "-maxuploadtarget=800M", + f"-maxuploadtarget={UPLOAD_TARGET_MB}M", "-datacarriersize=100000", ]] self.supports_cli = False + def assert_uploadtarget_state(self, *, target_reached, serve_historical_blocks): + """Verify the node's current upload target state via the `getnettotals` RPC call.""" + uploadtarget = self.nodes[0].getnettotals()["uploadtarget"] + assert_equal(uploadtarget["target_reached"], target_reached) + assert_equal(uploadtarget["serve_historical_blocks"], serve_historical_blocks) + def run_test(self): + # Initially, neither historical blocks serving limit nor total limit are reached + self.assert_uploadtarget_state(target_reached=False, serve_historical_blocks=True) + # Before we connect anything, we first set the time on the node # to be in the past, otherwise things break because the CNode # time counters can't be reset backward after initialization @@ -93,7 +105,7 @@ class MaxUploadTest(BitcoinTestFramework): getdata_request = msg_getdata() getdata_request.inv.append(CInv(MSG_BLOCK, big_old_block)) - max_bytes_per_day = 800*1024*1024 + max_bytes_per_day = UPLOAD_TARGET_MB * 1024 *1024 daily_buffer = 144 * 4000000 max_bytes_available = max_bytes_per_day - daily_buffer success_count = max_bytes_available // old_block_size @@ -113,6 +125,9 @@ class MaxUploadTest(BitcoinTestFramework): assert_equal(len(self.nodes[0].getpeerinfo()), 2) self.log.info("Peer 0 disconnected after downloading old block too many times") + # Historical blocks serving limit is reached by now, but total limit still isn't + self.assert_uploadtarget_state(target_reached=False, serve_historical_blocks=False) + # Requesting the current block on p2p_conns[1] should succeed indefinitely, # even when over the max upload target. # We'll try 800 times @@ -121,6 +136,9 @@ class MaxUploadTest(BitcoinTestFramework): p2p_conns[1].send_and_ping(getdata_request) assert_equal(p2p_conns[1].block_receive_map[big_new_block], i+1) + # Both historical blocks serving limit and total limit are reached + self.assert_uploadtarget_state(target_reached=True, serve_historical_blocks=False) + self.log.info("Peer 1 able to repeatedly download new block") # But if p2p_conns[1] tries for an old block, it gets disconnected too. @@ -139,6 +157,7 @@ class MaxUploadTest(BitcoinTestFramework): p2p_conns[2].sync_with_ping() p2p_conns[2].send_and_ping(getdata_request) assert_equal(p2p_conns[2].block_receive_map[big_old_block], 1) + self.assert_uploadtarget_state(target_reached=False, serve_historical_blocks=True) self.log.info("Peer 2 able to download old block") @@ -146,6 +165,8 @@ class MaxUploadTest(BitcoinTestFramework): self.log.info("Restarting node 0 with download permission and 1MB maxuploadtarget") self.restart_node(0, ["-whitelist=download@127.0.0.1", "-maxuploadtarget=1"]) + # Total limit isn't reached after restart, but 1 MB is too small to serve historical blocks + self.assert_uploadtarget_state(target_reached=False, serve_historical_blocks=False) # Reconnect to self.nodes[0] peer = self.nodes[0].add_p2p_connection(TestP2PConn()) @@ -156,6 +177,9 @@ class MaxUploadTest(BitcoinTestFramework): peer.send_and_ping(getdata_request) assert_equal(peer.block_receive_map[big_new_block], i+1) + # Total limit is exceeded + self.assert_uploadtarget_state(target_reached=True, serve_historical_blocks=False) + getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)] peer.send_and_ping(getdata_request) From dd5cf38818d1e3f6cf583e2b242afd0da68b290a Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Mon, 4 Dec 2023 18:31:36 +0100 Subject: [PATCH 2/3] test: check for specific disconnect reasons in feature_maxuploadtarget.py This ensures that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect?" (from an implementation reader's perspective) and "where is the code handling this disconnect?" (from a test reader's perspective) can be answered simply by grep-ping the corresponding debug message. --- test/functional/feature_maxuploadtarget.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/functional/feature_maxuploadtarget.py b/test/functional/feature_maxuploadtarget.py index 83c168404e2..9c66b67c5ba 100755 --- a/test/functional/feature_maxuploadtarget.py +++ b/test/functional/feature_maxuploadtarget.py @@ -119,9 +119,10 @@ class MaxUploadTest(BitcoinTestFramework): assert_equal(len(self.nodes[0].getpeerinfo()), 3) # At most a couple more tries should succeed (depending on how long # the test has been running so far). - for _ in range(3): - p2p_conns[0].send_message(getdata_request) - p2p_conns[0].wait_for_disconnect() + with self.nodes[0].assert_debug_log(expected_msgs=["historical block serving limit reached, disconnect peer"]): + for _ in range(3): + p2p_conns[0].send_message(getdata_request) + p2p_conns[0].wait_for_disconnect() assert_equal(len(self.nodes[0].getpeerinfo()), 2) self.log.info("Peer 0 disconnected after downloading old block too many times") @@ -143,8 +144,9 @@ class MaxUploadTest(BitcoinTestFramework): # But if p2p_conns[1] tries for an old block, it gets disconnected too. getdata_request.inv = [CInv(MSG_BLOCK, big_old_block)] - p2p_conns[1].send_message(getdata_request) - p2p_conns[1].wait_for_disconnect() + with self.nodes[0].assert_debug_log(expected_msgs=["historical block serving limit reached, disconnect peer"]): + p2p_conns[1].send_message(getdata_request) + p2p_conns[1].wait_for_disconnect() assert_equal(len(self.nodes[0].getpeerinfo()), 1) self.log.info("Peer 1 disconnected after trying to download old block") From b58f009d9585aab775998644de07e27e2a4a8045 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Mon, 4 Dec 2023 18:50:27 +0100 Subject: [PATCH 3/3] test: check that mempool msgs lead to disconnect if uploadtarget is reached Note that another reason for disconnect after receiving a MEMPOOL msg of a peer is if bloom filters are disabled on the node. This case is covered in the functional test `p2p_nobloomfilter_messages.py`. --- test/functional/feature_maxuploadtarget.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/functional/feature_maxuploadtarget.py b/test/functional/feature_maxuploadtarget.py index 9c66b67c5ba..814eb21e6fe 100755 --- a/test/functional/feature_maxuploadtarget.py +++ b/test/functional/feature_maxuploadtarget.py @@ -8,6 +8,7 @@ if uploadtarget has been reached. * Verify that getdata requests for recent blocks are respected even if uploadtarget has been reached. +* Verify that mempool requests lead to a disconnect if uploadtarget has been reached. * Verify that the upload counters are reset after 24 hours. """ from collections import defaultdict @@ -17,6 +18,7 @@ from test_framework.messages import ( CInv, MSG_BLOCK, msg_getdata, + msg_mempool, ) from test_framework.p2p import P2PInterface from test_framework.test_framework import BitcoinTestFramework @@ -165,14 +167,17 @@ class MaxUploadTest(BitcoinTestFramework): self.nodes[0].disconnect_p2ps() - self.log.info("Restarting node 0 with download permission and 1MB maxuploadtarget") - self.restart_node(0, ["-whitelist=download@127.0.0.1", "-maxuploadtarget=1"]) + self.log.info("Restarting node 0 with download permission, bloom filter support and 1MB maxuploadtarget") + self.restart_node(0, ["-whitelist=download@127.0.0.1", "-peerbloomfilters", "-maxuploadtarget=1"]) # Total limit isn't reached after restart, but 1 MB is too small to serve historical blocks self.assert_uploadtarget_state(target_reached=False, serve_historical_blocks=False) # Reconnect to self.nodes[0] peer = self.nodes[0].add_p2p_connection(TestP2PConn()) + # Sending mempool message shouldn't disconnect peer, as total limit isn't reached yet + peer.send_and_ping(msg_mempool()) + #retrieve 20 blocks which should be enough to break the 1MB limit getdata_request.inv = [CInv(MSG_BLOCK, big_new_block)] for i in range(20): @@ -190,6 +195,11 @@ class MaxUploadTest(BitcoinTestFramework): assert_equal(len(peer_info), 1) # node is still connected assert_equal(peer_info[0]['permissions'], ['download']) + self.log.info("Peer gets disconnected for a mempool request after limit is reached") + with self.nodes[0].assert_debug_log(expected_msgs=["mempool request with bandwidth limit reached, disconnect peer"]): + peer.send_message(msg_mempool()) + peer.wait_for_disconnect() + self.log.info("Test passing an unparsable value to -maxuploadtarget throws an error") self.stop_node(0) self.nodes[0].assert_start_raises_init_error(extra_args=["-maxuploadtarget=abc"], expected_msg="Error: Unable to parse -maxuploadtarget: 'abc'")