#36406 test: make p2p_orphan_handling's never-requested checks able to fail
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36406 test: make p2p_orphan_handling's never-requested checks able to fail | Ready | 1 | Strong | 0 + 12 tests | |||
|
|
|
|
| |||
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
Fixes `test_orphan_inherit_rejection` in `p2p_orphan_handling.py` so getdata checks actually test node behavior. It enforces integer types in `PeerTxRelayer.assert_never_requested`, passes integer transaction hashes from the test, advances mocktime so delayed orphan parent requests can arrive, and switches the parent-relaying peer.
Problem: `assert_never_requested` compares its argument to getdata inv hashes, which are Python integers. The test passed hex strings, causing the equality check `int == str` to always evaluate to `False` and assertions to pass unconditionally even if the node erroneously requested the transactions.
Evidence of importance:
- Commit a3c4ad12e9: 'PeerTxRelayer.assert_never_requested compares the hash of every request received, an int, with its argument, and test_orphan_inherit_rejection passed it hex strings. An int never equals a str, so these checks passed whatever the node requested.'
- Author verified that mutating the test scenario to trigger requests caused the old string checks to still pass while the new integer checks properly failed.
Risk of a mistake: low (tests). Touches only functional test assertion logic; test failures would affect CI but cannot impact production node behavior.
Changed since the previous assessment: Adds an explicit `isinstance(txhash, int)` type assertion inside `PeerTxRelayer.assert_never_requested` to prevent caller type mismatches.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing
Agreement
Strong
substantive support, no open objection (instagibbs)
Thread notes: Initial record. Reviewer instagibbs supported the PR with suggestions (an explanatory comment, an isinstance assertion, and a minor syntax simplification), all of which were implemented and ACKed. / Initial review by instagibbs provided suggestions that the author promptly addressed, followed by a commit ACK.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| instagibbs (member) | suggestion, safety | Add an assertion in assert_never_requested ensuring txhash is an int to avoid future misuse | accepted · fix pushed · author added the assertion in a separate commit and reviewer ACKed | answered | yes | 2026-10-02: 'Could we add this to `assert_never_requested` to remove the footgun and find misuse elsewhere going forward' Status from: 2026-10-02: instagibbs: 'ACK https://github.com/bitcoin/bitcoin/pull/36406/commits/5f22014aca2d071edfea95e8d3d4ba0bebbc57ca' |
| instagibbs (member) | suggestion, style | Use txid_int directly to be less verbose | accepted · fix pushed · author adopted the suggestion using x['tx'].txid_int and reviewer ACKed | answered | yes | 2026-10-02: 'think this works here and elsewhere and is less verbose' Status from: 2026-10-02: instagibbs: 'ACK https://github.com/bitcoin/bitcoin/pull/36406/commits/5f22014aca2d071edfea95e8d3d4ba0bebbc57ca' |
| instagibbs (member) | suggestion, maintenance | Add a brief comment explaining why relaying from peer2 | accepted · fix pushed · author added the explanatory comment and reviewer ACKed | answered | yes | 2026-10-02: 'since we're changing this to peer2, and it's non-obvious, very brief comment why?' Status from: 2026-10-02: instagibbs: 'ACK https://github.com/bitcoin/bitcoin/pull/36406/commits/5f22014aca2d071edfea95e8d3d4ba0bebbc57ca' |
Support:
- instagibbs (member) (ACK, 2026-10-02): ACK https://github.com/bitcoin/bitcoin/pull/36406/commits/5f22014aca2d071edfea95e8d3d4ba0bebbc57ca [not substantive]
- instagibbs (member) (Concept ACK, 2026-10-02) [read: test/functional/p2p_orphan_handling.py]: good catches
Participants:
- instagibbs (member, support, 4 statements 2026-10-02 to 2026-10-02): concept ACKed, suggested an assertion, code comment, and syntax tweak, then ACKed the push
Files
18 lines under test/bench/ci.
- test/functional/p2p_orphan_handling.py +12/-6
Card
The short text the ranking pass compares across PRs.
Fixes a bug in the p2p_orphan_handling functional test where assertions passed hex strings instead of integers to assert_never_requested. Because an integer never equals a string in Python, these checks passed vacuously regardless of what the node actually requested. The PR passes integers, enforces the integer type in the helper, switches the initial relaying peer, and advances mocktime so requests can trigger and regressions are caught. This is test-only code fixing non-functional test assertions with low risk.
Data
thread read through 2026-10-02 (5 statements), head 5f22014aca
labels: Tests
Categories
P2P
P3 · test coverage (ranking pass: Test-only repair of a vacuous assertion; ordered inside the test cluster.)
Fixes broken `assert_never_requested` checks in `p2p_orphan_handling.py` that were comparing integers to hex strings and therefore unconditionally passing. Commit a3c4ad12e9 explains that because `int never equals a str`, 'these checks passed whatever the node requested.' Enforcing integer types and advancing mocktime restores meaningful test coverage for orphan request rejection without modifying node behavior.
Membership: Touches test/functional/p2p_orphan_handling.py, testing p2p transaction relay and orphan request handling.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1