test: tolerate limited peer disconnect in p2p test#7305
test: tolerate limited peer disconnect in p2p test#7305thepastaclaw wants to merge 1 commit intodashpay:developfrom
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit af8fc66) |
WalkthroughA 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)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/functional/p2p_node_network_limited.py (1)
83-90: ⚡ Quick winLGTM —
wait_for_debug_logwrapping correctly anchors the assertion.Wrapping both operations inside
wait_for_debug_logbefore 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. TheAssertionErrorcatch correctly targets the"Error: peer disconnected"message raised bycheck_bytesrecvinsideconnect_nodes, and re-raises unexpected errors. The byte-string prefixb"Ignore block request below NODE_NETWORK_LIMITED threshold"matches the C++ log output inProcessGetBlockDataatnet_processing.cpp.One robustness note:
wait_for_debug_logrecordstime_endbefore theyield(before thewithbody executes), then checks it only after the body exits. The innersync_blocks(timeout=5)can consume most of that 5-second budget. In practice this is safe because the log message is written in C++ beforefDisconnectis set (i.e. it will already be present when the post-body while-loop runs its first iteration, which checksfoundbefore checking the clock), but bumping the outer timeout to e.g.15would 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
📒 Files selected for processing (1)
test/functional/p2p_node_network_limited.py
| try: | ||
| self.sync_blocks([self.nodes[0], self.nodes[2]], timeout=5) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Narrow the broad except Exception: pass (Ruff S110/BLE001).
sync_blocks is expected to raise AssertionError (from an internal wait_until timeout). Catching the bare Exception silently swallows any other unexpected failure, making future debugging harder. At minimum, restrict to AssertionError, or log the exception:
🐛 Proposed fix
try:
self.sync_blocks([self.nodes[0], self.nodes[2]], timeout=5)
- except Exception:
- pass
+ except AssertionError:
+ pass # expected: NODE_NETWORK_LIMITED peer cannot serve IBD blocks🧰 Tools
🪛 Ruff (0.15.12)
[error] 93-94: try-except-pass detected, consider logging the exception
(S110)
[warning] 93-93: Do not catch blind exception: Exception
(BLE001)
🤖 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 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.
thepastaclaw
force-pushed
the
fix-7288-node-network-limited-disconnect
branch
from
7ba9817 to
af8fc66
Compare
|
Addressed CodeRabbit feedback in Validation:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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)
| except AssertionError as e: | ||
| if "peer disconnected" not in str(e): | ||
| raise |
There was a problem hiding this comment.
💬 Nitpick: Brittle substring match on connect_nodes() AssertionError text
The tolerated race is detected via if "peer disconnected" not in str(e). That string originates in test/functional/test_framework/test_framework.py:732 (assert peer is not None, "Error: peer disconnected"). If a future Bitcoin Core backport rewords that assertion message, this catch will silently start re-raising and the test will look flaky again for the same underlying race. The actual signal is the debug-log assertion inside the wait_for_debug_log block, so risk is low, but the implicit coupling deserves either an explanatory comment pointing at the framework source, or a looser match (e.g. on "disconnect").
source: ['claude']
1 participant
test: tolerate limited peer disconnect in p2p test
Summary
connect_nodes()fast-disconnect race inp2p_node_network_limited.py --v2transport.NODE_NETWORK_LIMITEDold-block disconnect.Fixes #7288.
Validation
./test/functional/p2p_node_network_limited.py --v2transport \ --configfile=/Users/claw/Projects/dash/test/config.inigit diff --checkcode-review dashpay/dash upstream/develop \ fix-7288-node-network-limited-disconnectreturnedship.