#36486 p2p: keep orphan resolution requests for non-witness 1p1c parents
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36486 p2p: keep orphan resolution requests for non-witness 1p1c parents | Ready | 1 | Positive | 5 + 39 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
Preserves queued txid orphan resolution requests when a non-witness parent transaction is rejected as TX_RECONSIDERABLE. The download manager skips calling ForgetTxHash on the witness hash if a reconsiderable transaction lacks witness data. This ensures peers that announced an orphan child are still asked for the legacy parent by txid when that parent arrives first from a different peer.
Problem: For non-witness transactions, the wtxid equals the txid. When a low-feerate legacy parent is rejected with TX_RECONSIDERABLE, forgetting its wtxid in the transaction request tracker also wipes txid requests queued to resolve orphan children. If that parent arrived from a peer that did not announce the orphan child, opportunistic 1P1C relay fails to pair them, the child's announcers are never queried for the parent, and the child remains stuck in the orphanage.
Evidence of importance:
- Author's functional test demonstration: in p2p_opportunistic_1p1c.py, the new test_parent_from_other_peer times out on master when using a legacy parent (wallet_nonsegwit) waiting for getdata from the child announcer, but passes with this change.
Risk of a mistake: medium (p2p). Mistakes in transaction download tracking can cause duplicate transaction downloads or lingering state in the request tracker.
Changed since the previous assessment: None; the previous description was recorded before the patch was available.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- decision: whether to fold this into #36437 or rebase once #36437 lands 2026-10-13
Agreement
Positive
support without stated reasons, no open objection (Yudis-bit)
Thread notes: Added author's reply agreeing to coordinate with #36437 and offering to rebase or fold.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| Yudis-bit (none) | suggestion, correctness | Insert ptx->GetHash() into RecentRejectsReconsiderableFilter if witness to address the mirror case where segwit reconsiderable parents are missed. | answered · author explained caching txid would allow a mutated witness to cause an honest parent to be ignored | answered | yes | 2026-10-09: 'Line 461 `RecentRejectsReconsiderableFilter` only caches `wtxid`, so line 382 `contains(parent_txid)` misses segwit reconsiderable parents. Inserting `ptx->GetHash()` if witness addresses the mirror c' Status from: 2026-10-12: FlashWayne: 'I'd keep the txid of a segwit tx out of the reconsiderable filter: a low-feerate copy with a mutated witness also fails ' |
| instagibbs (member) | suggestion, approach | Catalog all known cases, put them under test, and address them as a group rather than fixing piecemeal. | answered · author demonstrated fuzz test results on top of #36437 and instagibbs moved review to #36437 | answered | yes | 2026-10-11: 'I'd rather catalog all the cases we know and care about, put them under test, and then attack them as a group to ensure we're not doing whackamole forever.' Status from: 2026-10-12: instagibbs: 'tagged you on #36437 I think review should move there for now' |
Support:
- Yudis-bit (none) (Tested ACK, 2026-10-09) [tested]: Tested on Windows x86_64, Clang 22 [not substantive]
Participants:
- Yudis-bit (none, support, 1 statements 2026-10-09 to 2026-10-09): Tested ACK; suggested caching txid in RecentRejectsReconsiderableFilter for segwit parents
- instagibbs (member, neutral, 2 statements 2026-10-11 to 2026-10-12): suggested addressing cases as a group in #36437 and moved review there
Files
39 lines under test/bench/ci.
- test/functional/p2p_opportunistic_1p1c.py +39/-0
- src/node/txdownloadman_impl.cpp +5/-1
Card
The short text the ranking pass compares across PRs.
Preserves orphan resolution requests for non-witness transactions during opportunistic 1P1C package relay. For legacy transactions, wtxid equals txid, so calling ForgetTxHash when a parent is rejected as TX_RECONSIDERABLE inadvertently deletes queued txid requests intended to fetch the parent from peers that announced the child. If the low-feerate parent arrived from a peer other than the child's announcer, 1P1C cannot pair them, and the child remains orphaned until re-announced. This change skips ForgetTxHash for non-witness reconsiderable transactions, resolving a test timeout bug and bringing legacy 1P1C handling in line with segwit behavior.
Data
thread read through 2026-10-13 (5 statements), head 8859ada450
labels: P2P
Categories
P2P
P3 · bug fix
Fixes a relay liveness bug where non-witness transactions can remain stuck in the orphanage during opportunistic 1P1C relay. Because txid equals wtxid for non-segwit transactions, 'forgetting its wtxid in the transaction request tracker also wipes txid requests queued to resolve orphan children', causing orphan resolution to stall when the parent arrives from a different peer than the child announcer.
Membership: Matches the P2P label and modifies transaction download and orphan resolution logic in src/node/txdownloadman_impl.cpp.
Factors: security/stability 1, bug 1, performance 0, user value 1, leverage 0