test: improve coverage for a resolved stalling situation
What changed, and why it matters
This commit only changes a test file. It adds a new test scenario to make sure a previously fixed bug in block-download stalling logic stays fixed. There is no change to the actual Bitcoin Core node software, so it cannot directly affect live network security.
No security action required. Review the preceding commit to confirm the actual stalling fix is sound, since this commit only adds test coverage for it.
Security signals we found
Regression test for stalling logic
No production code changes
Commit message references a resolved stalling situation
Evidence from the diff
The commit modifies test/functional/p2p_ibd_stalling.py to extend the existing IBD stalling functional test. It introduces a second stalled block and an extra peer, then verifies that after the first stalled block is provided, the node does not immediately mark another peer as a staller. The commit message explicitly states the test would fail without the previous commit, indicating it is regression coverage for a bug already fixed in the preceding change.
Changed components
test/functional/p2p_ibd_stalling.pyInspect captured patch +20 / −17
diff --git a/test/functional/p2p_ibd_stalling.py b/test/functional/p2p_ibd_stalling.py
index e7066ea7..c09b2e50 100755
--- a/test/functional/p2p_ibd_stalling.py
+++ b/test/functional/p2p_ibd_stalling.py
@@ -29,15 +29,15 @@ from test_framework.util import (
class P2PStaller(P2PDataStore):
- def __init__(self, stall_block):
- self.stall_block = stall_block
+ def __init__(self, stall_blocks):
+ self.stall_blocks = stall_blocks
super().__init__()
def on_getdata(self, message):
for inv in message.inv:
self.getdata_requests.append(inv.hash)
if (inv.type & MSG_TYPE_MASK) == MSG_BLOCK:
- if (inv.hash != self.stall_block):
+ if (inv.hash not in self.stall_blocks):
self.send_without_ping(msg_block(self.block_store[inv.hash]))
def on_getheaders(self, message):
@@ -51,7 +51,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
def run_test(self):
NUM_BLOCKS = 1025
- NUM_PEERS = 4
+ NUM_PEERS = 5
node = self.nodes[0]
tip = int(node.getbestblockhash(), 16)
blocks = []
@@ -66,7 +66,9 @@ class P2PIBDStallingTest(BitcoinTestFramework):
block_time += 1
height += 1
block_dict[blocks[-1].hash_int] = blocks[-1]
- stall_block = blocks[0].hash_int
+ stall_index = 0
+ second_stall_index = 500
+ stall_blocks = [blocks[stall_index].hash_int, blocks[second_stall_index].hash_int]
headers_message = msg_headers()
headers_message.headers = [CBlockHeader(b) for b in blocks[:NUM_BLOCKS-1]]
@@ -76,12 +78,12 @@ class P2PIBDStallingTest(BitcoinTestFramework):
self.mocktime = int(time.time()) + 1
node.setmocktime(self.mocktime)
for id in range(NUM_PEERS):
- peers.append(node.add_outbound_p2p_connection(P2PStaller(stall_block), p2p_idx=id, connection_type="outbound-full-relay"))
+ peers.append(node.add_outbound_p2p_connection(P2PStaller(stall_blocks), p2p_idx=id, connection_type="outbound-full-relay"))
peers[-1].block_store = block_dict
peers[-1].send_and_ping(headers_message)
- # Wait until all blocks are received (except for stall_block), so that no other blocks are in flight.
- self.wait_until(lambda: sum(len(peer['inflight']) for peer in node.getpeerinfo()) == 1)
+ # Wait until all blocks are received (except for the stall blocks), so that no other blocks are in flight.
+ self.wait_until(lambda: sum(len(peer['inflight']) for peer in node.getpeerinfo()) == len(stall_blocks))
self.all_sync_send_with_ping(peers)
# If there was a peer marked for stalling, it would get disconnected
@@ -102,7 +104,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
node.setmocktime(self.mocktime)
peers[0].wait_for_disconnect()
assert_equal(node.num_test_p2p_connections(), NUM_PEERS - 1)
- self.wait_until(lambda: self.is_block_requested(peers, stall_block))
+ self.wait_until(lambda: self.is_block_requested(peers, stall_blocks[0]))
# Make sure that SendMessages() is invoked, which assigns the missing block
# to another peer and starts the stalling logic for them
self.all_sync_send_with_ping(peers)
@@ -117,7 +119,7 @@ class P2PIBDStallingTest(BitcoinTestFramework):
self.mocktime += 2
node.setmocktime(self.mocktime)
self.wait_until(lambda: sum(x.is_connected for x in node.p2ps) == NUM_PEERS - 2)
- self.wait_until(lambda: self.is_block_requested(peers, stall_block))
+ self.wait_until(lambda: self.is_block_requested(peers, stall_blocks[0]))
self.all_sync_send_with_ping(peers)
self.log.info("Check that the stalling timeout gets doubled to 8 seconds for the next staller")
@@ -130,17 +132,18 @@ class P2PIBDStallingTest(BitcoinTestFramework):
self.mocktime += 2
node.setmocktime(self.mocktime)
self.wait_until(lambda: sum(x.is_connected for x in node.p2ps) == NUM_PEERS - 3)
- self.wait_until(lambda: self.is_block_requested(peers, stall_block))
+ self.wait_until(lambda: self.is_block_requested(peers, stall_blocks[0]))
self.all_sync_send_with_ping(peers)
- self.log.info("Provide the withheld block and check that stalling timeout gets reduced back to 2 seconds")
- with node.assert_debug_log(expected_msgs=['Decreased stalling timeout to 2 seconds']):
+ self.log.info("Provide the first withheld block and check that stalling timeout gets reduced back to 2 seconds")
+ with node.assert_debug_log(expected_msgs=['Decreased stalling timeout to 2 seconds'], unexpected_msgs=['Stall started']):
for p in peers:
- if p.is_connected and (stall_block in p.getdata_requests):
- p.send_without_ping(msg_block(block_dict[stall_block]))
+ if p.is_connected and (stall_blocks[0] in p.getdata_requests):
+ p.send_without_ping(msg_block(block_dict[stall_blocks[0]]))
+ self.all_sync_send_with_ping(peers)
- self.log.info("Check that all outstanding blocks get connected")
- self.wait_until(lambda: node.getblockcount() == NUM_BLOCKS)
+ self.log.info("Check that all outstanding blocks up to the second stall block get connected")
+ self.wait_until(lambda: node.getblockcount() == second_stall_index)
def all_sync_send_with_ping(self, peers):
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.