#36617 test: Fix intermittent issue in p2p_orphan_handling.py

ReviewabilityReviewsAgreementSize
#36617 maflcko test: Fix intermittent issue in p2p_orphan_handling.pyReady1Crickets0 + 2 tests
  • Prevent intermittent CI test failures in orphan transaction handling
  • Ensure peer disconnection state is fully cleaned up before mock time advances
  • Ready to review
  • Nothing known that would invalidate a review
  • No substantive feedback either way
  • Approval without stated rationale from an account with no prior project history (Conradgabe)
  • Resolved question regarding an existing test comment (Conradgabe)
  • 0 lines added or modified outside tests
  • 2 lines added or modified in tests
  • 2 lines removed in total
  • 1 files, 1 commits
P2P#63 of 71
P3 · bug fix
  • P3 because it resolves an intermittent test race condition without altering node p2p behavior
  • Improves CI reliability when testing orphan transaction relay logic
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Fix an intermittent failure in the functional test `p2p_orphan_handling.py` during peer disconnection. The test now waits for `getorphantxs()` to reflect peer cleanup before bumping mock time, ensuring orphan state and in-flight requests are removed before asserting new parent requests.

Problem: In `p2p_orphan_handling.py`, disconnecting a peer does not instantly invoke `TxDownloadManager::DisconnectedPeer` if the message handling thread still holds a reference to the disconnected `CNode`. Previously, the test only waited on the connection count and a ping before advancing mock time, which could cause a split `getdata` request and fail the assertion on peer parent requests.

Evidence of importance:

  • Author links to a reproducible CI failure in GitHub Actions (run 39809026827)
  • Author provides a diff adding sleeps in net.cpp to reproduce the test failure locally with ~10% failure rate

Scope: Small 2-line test-only change.

Risk of a mistake: low (tests). Touches only synchronization logic in a functional test without affecting node or consensus code.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • nothing

Agreement

Crickets

no substantive comment either way

Thread notes: Initial record for PR 36617. Conradgabe's crACK has no rationale and comes from an account with no prior project history, so it is omitted from support per policy. Their comment nit was addressed and accepted as accurate without changes. / First read of PR 36617. Conradgabe offered a crACK without rationale from an account with no history in the project, so it is omitted from support. Their suggestion regarding a code comment was resolved after author explanation.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
Conradgabe (none)suggestion, maintenanceUpdate the comment on peer2.sync_with_ping() to reflect that ping does not guarantee deferred deletion has occurredaccepted · author explained why the comment is still correct, and reviewer agreedansweredyes2026-10-23: 'nit: the comment on `peer2.sync_with_ping()` ("Sync with the 'net' thread which completes the disconnection fully") is no longer correct given this PR update'
Status from: 2026-10-23: Conradgabe: 'so I think the existing comment is still accurate.'

Participants:

  • Conradgabe (none, neutral, 2 statements 2026-10-23 to 2026-10-23): left a crACK without rationale and suggested clarifying a test comment, which they dropped after explanation

Files

4 lines under test/bench/ci.

  • test/functional/p2p_orphan_handling.py +2/-2

Card

The short text the ranking pass compares across PRs.

This PR fixes an intermittent race condition in the functional test p2p_orphan_handling.py during peer disconnection. Disconnecting a peer does not synchronously clear its orphan state in TxDownloadManager if the message handling thread still holds a CNode reference, causing subsequent parent request assertions to fail. The change waits for node.getorphantxs() to confirm orphan cleanup before advancing mock time. The author provided a CI failure log and a diff in net.cpp demonstrating a 10% reproduction rate.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-24T01:20, confidence high, input hash 0d7d8e97da4670a5
thread read through 2026-10-23 (4 statements), head faf819bf23
labels: Tests

Categories

P2P

P3 · bug fix

Fixes an intermittent race condition in the orphan handling test where disconnected peers still held references during mock time advancement. Addressing test flakiness improves CI signal for orphan handling relay behavior, though it does not change node P2P behavior directly.

Membership: Touches test/functional/p2p_orphan_handling.py, testing peer disconnection and orphan transaction handling.

Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 0