test: tolerate limited peer disconnect in p2p test

[thepastaclaw]

test: tolerate limited peer disconnect in p2p test

Summary

• Tolerate the expected connect_nodes() fast-disconnect race in
  p2p_node_network_limited.py --v2transport.
• Keep the behavior assertion by waiting for node 0 to log the
  NODE_NETWORK_LIMITED old-block disconnect.
• Preserve the final check that unsynced node 2 remains at height 0.

Fixes #7288.

Validation

• ./test/functional/p2p_node_network_limited.py --v2transport \ --configfile=/Users/claw/Projects/dash/test/config.ini
• git diff --check
• Pre-PR review gate: code-review dashpay/dash upstream/develop \ fix-7288-node-network-limited-disconnect returned ship.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

✅ Review complete (commit af8fc66)

[coderabbitai]

Walkthrough

A test case for p2p network limited node behavior was modified to improve verification logic. The test connecting node 0 to node 2 (a pruned NODE_NETWORK_LIMITED peer) now wraps the connection and block synchronization operations within a debug log monitor. The test waits for a specific debug message indicating block request rejection below the NODE_NETWORK_LIMITED threshold. Error handling was added to gracefully handle AssertionError during connection attempts when the peer disconnects, while the sync_blocks operation remains optional with broad exception suppression.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: making the p2p test tolerate a peer disconnect scenario with NODE_NETWORK_LIMITED.
Description check	✅ Passed	The description is well-detailed and directly related to the changeset, explaining the disconnect race condition and the approach to harden the test.
Linked Issues check	✅ Passed	The changes directly address issue #7288 by implementing the suggested hardening: tolerating the expected connect_nodes() disconnect while preserving behavior assertions and postconditions.
Out of Scope Changes check	✅ Passed	All changes are within scope, limited to p2p_node_network_limited.py test modifications to handle the identified disconnect race condition.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/functional/p2p_node_network_limited.py (1)

83-90: ⚡ Quick win

LGTM — wait_for_debug_log wrapping correctly anchors the assertion.

Wrapping both operations inside wait_for_debug_log before asserting the postcondition is the right pattern: it guarantees the NODE_NETWORK_LIMITED rejection actually occurred before proceeding, rather than silently passing if the test skips the scenario. The AssertionError catch correctly targets the "Error: peer disconnected" message raised by check_bytesrecv inside connect_nodes, and re-raises unexpected errors. The byte-string prefix b"Ignore block request below NODE_NETWORK_LIMITED threshold" matches the C++ log output in ProcessGetBlockData at net_processing.cpp.

One robustness note: wait_for_debug_log records time_end before the yield (before the with body executes), then checks it only after the body exits. The inner sync_blocks(timeout=5) can consume most of that 5-second budget. In practice this is safe because the log message is written in C++ before fDisconnect is set (i.e. it will already be present when the post-body while-loop runs its first iteration, which checks found before checking the clock), but bumping the outer timeout to e.g. 15 would give more headroom under load.

🛡️ Optional: increase outer timeout

-        with self.nodes[0].wait_for_debug_log(
-                [b"Ignore block request below NODE_NETWORK_LIMITED threshold"], timeout=5):
+        with self.nodes[0].wait_for_debug_log(
+                [b"Ignore block request below NODE_NETWORK_LIMITED threshold"], timeout=15):

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/functional/p2p_node_network_limited.py` around lines 83 - 90, The test
wraps connect_nodes in wait_for_debug_log which is correct, but to avoid flaky
failures under heavy load increase the outer timeout from 5 to a larger value
(e.g. 15); update the wait_for_debug_log call that surrounds connect_nodes (and
related sync_blocks) in p2p_node_network_limited.py so the first argument
timeout is bumped (reference wait_for_debug_log, connect_nodes, sync_blocks) to
give more headroom for the C++ log to appear before the with-block post-check
runs.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/functional/p2p_node_network_limited.py`:
- Around line 91-94: The current broad except in the block calling
sync_blocks([self.nodes[0], self.nodes[2]], timeout=5) should be narrowed: catch
AssertionError (the expected wait_until timeout) instead of Exception, and for
any other unexpected exception either re-raise or log it so failures aren’t
silently swallowed; update the try/except to except AssertionError: pass and add
an except Exception as e: raise (or processLogger.error/fail) to surface
unexpected errors from sync_blocks/wait_until.

---

Nitpick comments:
In `@test/functional/p2p_node_network_limited.py`:
- Around line 83-90: The test wraps connect_nodes in wait_for_debug_log which is
correct, but to avoid flaky failures under heavy load increase the outer timeout
from 5 to a larger value (e.g. 15); update the wait_for_debug_log call that
surrounds connect_nodes (and related sync_blocks) in p2p_node_network_limited.py
so the first argument timeout is bumped (reference wait_for_debug_log,
connect_nodes, sync_blocks) to give more headroom for the C++ log to appear
before the with-block post-check runs.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2812ab35-574b-43bd-a426-fc0c7af55113

📥 Commits

Reviewing files that changed from the base of the PR and between 4a0fd85 and 7ba9817.

📒 Files selected for processing (1)

• test/functional/p2p_node_network_limited.py

[thepastaclaw]

Addressed CodeRabbit feedback in af8fc66: narrowed the expected sync_blocks() failure to AssertionError and bumped the debug-log wait timeout to 15s for more CI headroom.

Validation:

• git diff --check
• ./test/functional/p2p_node_network_limited.py --v2transport --configfile=/Users/claw/Projects/dash/test/config.ini

[thepastaclaw]

Code Review

Test-only change that hardens p2p_node_network_limited.py against a connect_nodes() race under --v2transport, where the NODE_NETWORK_LIMITED peer can disconnect the unsynced peer before the version handshake check completes. The fix narrows exception handling, asserts the expected debug log, and preserves the height-0 invariant on node 2. Approach is sound and minimal. One nitpick about implicit coupling to upstream framework error text.

Reviewed commit: af8fc66

💬 1 nitpick(s)