p2p: keep orphan resolution requests for non-witness 1p1c parents #36486

FlashWayne commented at 8:05 AM on October 9, 2026: contributor

When a low-feerate parent is rejected as TX_RECONSIDERABLE, MempoolRejectedTx calls m_txrequest.ForgetTxHash(ptx->GetWitnessHash()) (txdownloadman_impl.cpp:473). For a transaction without witness, wtxid == txid, so this also deletes the txid requests that MaybeAddOrphanResolutionCandidate queued for every peer that announced the orphan child.

If the parent then comes from a peer that did not announce the child:

Find1P1CPackage only pairs the parent with children from the same peer, so no package is formed
the child's announcers are never asked for the parent
the child stays in the orphanage until some new peer announces it

With a segwit parent the txid differs from the wtxid, so the txid requests survive and the package gets accepted. A legacy (P2PK) parent is affected.

This skips ForgetTxHash when the result is TX_RECONSIDERABLE and the tx has no witness. New wtxid announcements of that parent are still ignored through AlreadyHaveTx(include_reconsiderable=true). The cost is that announcements already queued may download the parent once more.

The new test_parent_from_other_peer in p2p_opportunistic_1p1c.py runs for a segwit and a legacy parent: peer A sends the child (orphan), peer B sends the low-fee parent, then it checks that A is asked for the parent by txid and the package is accepted. On master the legacy case times out waiting for that request; with the change it passes. p2p_orphan_handling.py and p2p_tx_download.py still pass.

p2p: keep orphan resolution requests for non-witness 1p1c parents

When a low-feerate parent is rejected as TX_RECONSIDERABLE, the tx
download manager calls m_txrequest.ForgetTxHash(wtxid). For a tx without
witness, wtxid == txid, so this also deletes the txid requests that were
queued to resolve an orphan child from the peers that announced it.

If the parent arrives from a peer that didn't announce the child,
Find1P1CPackage doesn't pair them (it only considers children from the
same peer), and the child's announcers are never asked for the parent. The
child stays in the orphanage until some new peer announces it. With a
segwit parent the txid differs from the wtxid, so the requests survive.

Skip ForgetTxHash for a reconsiderable tx that has no witness. New wtxid
announcements are still ignored through AlreadyHaveTx.

8859ada450

DrahtBot added the label P2P on Oct 9, 2026

DrahtBot commented at 8:05 AM on October 9, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36486
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36486.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36486#author-ralph

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	Yudis-bit

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36369 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36369.svg"></sub> (p2p: don't let one peer's same-txid variant cancel another peer's 1p1c or orphan resolution by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Yudis-bit commented at 1:08 PM on October 9, 2026: none

Tested ACK 8859ada450 (Windows x86_64, Clang 22).

Line 461 RecentRejectsReconsiderableFilter only caches wtxid, so line 382 contains(parent_txid) misses segwit reconsiderable parents. Inserting ptx->GetHash() if witness addresses the mirror case (here or follow-up).

instagibbs commented at 6:33 PM on October 11, 2026: member

#36437 introduces this case as parent_stripped, where a maliciously stripped variant causes the cancellation. IIUC this is the same code paths in the end.

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.

(added an explanatory note for the fuzz case)

FlashWayne commented at 9:28 AM on October 12, 2026: contributor

Thanks, agreed it's the same path: the stripped parent in parent_stripped and an honest non-segwit parent both reach ForgetTxHash with wtxid == txid. The non-segwit case doesn't need an adversary though, a parent relayed by a peer that didn't announce the child is enough.

In case it helps with the catalog: I applied this commit on top of #36437 and stopped setting holes.parent_stripped. Without the commit, p2p_1p1c_liveness hits the liveness assert within seconds from an empty corpus (964 crashing inputs in 10 minutes); with it, all of those inputs pass, and 5 workers ran for an hour (~13M executions) without a failure. So it looks like it closes that hole. Happy to rebase onto #36437 and drop the flag here, or for you to fold it into #36369/#36437 if you'd rather land them as a group. @Yudis-bit thanks for testing. I'd keep the txid of a segwit tx out of the reconsiderable filter: a low-feerate copy with a mutated witness also fails as TX_RECONSIDERABLE, so it would put the honest parent's txid there, and AlreadyHaveTx(..., include_reconsiderable=true) would then ignore txid announcements of the honest parent (the #8279 concern in the comment above that branch). At the line you point to it only affects the early give-up for a child with two reconsiderable parents.

instagibbs commented at 9:25 PM on October 12, 2026: member

tagged you on #36437 I think review should move there for now

FlashWayne commented at 8:18 AM on October 13, 2026: contributor

Thanks, makes sense to move review to #36437. I re-checked against its current head (65b48ba1f1): with this commit applied and holes.parent_stripped no longer set, p2p_1p1c_liveness ran 10 minutes with 5 workers from an empty corpus (~2.4M execs) with no failures. So this closes the parent_stripped hole. Happy to rebase on top of #36437 once it lands and drop the flag in this PR, or to have this folded into #36437/#36369 if you'd rather land the holes as a group.