p2p: don't let one peer's same-txid variant cancel another peer's 1p1c or orphan resolution #36369

pull instagibbs wants to merge 9 commits into bitcoin:master from instagibbs:2026-09-1p1c-preserve-orphan changing 12 files +728 −55

instagibbs commented at 4:09 PM on September 28, 2026: member

Opportunistic 1p1c relay and orphan resolution fetch a missing parent by txid, but several outcomes are recorded by wtxid, or against a single package member, in ways another peer could exploit. A peer sending a variant of a parent with the same txid and a different or missing witness could make the node drop the honest child, cancel the honest peer's request for the parent, or reject later children of that parent. Each case delays a CPFP package until a new announcer appears or the parent confirms.

This PR fixes four such cases deemed the easiest to accomplish by a direct node peer trying to frustrate 1P1C relay at each individual hop.

There are remaining residuals that are cataloged here in sloppy form: https://gist.github.com/instagibbs/880a76535fd802fced9f9a5253cdbb40

To close these classes of issues by construction, we would have to replace our opportunistic protocol with a sender-initiated one, but that requires a network wide protocol update. This is beyond the scope of this PR and effort, for now.

Follow-up PR enforcing invariants we want to see post-fixes: #36371

Given these are 4 essentially individual fixes, I can also split off fixes to make review easier, but I will keep this PR open in some form to show the scope of the desired changes.

Related minor PR: #36328

Investigation kicked off by a Project Loupe report

p2p: keep orphan after a package feerate failure

When a 1p1c package fails the package feerate check, the child gets a
TX_RECONSIDERABLE result, and ProcessPackageResult passed it to
ProcessInvalidTx, which erased the child from the orphanage for all of
its announcers.

The failure is a property of that pair of wtxids, which
MempoolRejectedPackage already caches, not of the child. The child can
still succeed with another version of the parent, such as the one an
honest announcer holds, so it should stay available.

Skip ProcessInvalidTx for package members with a TX_RECONSIDERABLE
result. A child that is too low feerate on its own is still rejected
when its parent's work set is processed.

c0d03dddb7

DrahtBot added the label P2P on Sep 28, 2026
DrahtBot commented at 4:10 PM on September 28, 2026: contributor


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



Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36369.



Reviews

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

Type Reviewers

Concept ACK darosior

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:
- #36318 (validation: Ensure Invalid ValidationState has result by optout21)
- #36015 (txorphanage: bound orphan memory by storing transactions serialized by brunoerg)
- #35713 (Remove boost as a unit test runner by rustaceanrob)
- #35569 (Encapsulation for CTransaction by purpleKarrot)
- #35502 (refactor: extract per-message helpers from ProcessMessage (move-only) by w0xlt)
- #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
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.
darosior commented at 4:13 PM on September 28, 2026: member

Concept ACK
instagibbs force-pushed on Sep 28, 2026
DrahtBot added the label CI failed on Sep 28, 2026
DrahtBot commented at 4:22 PM on September 28, 2026: contributor


🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/36449058662/job/109018784014</sub> <sub>LLM reason (✨ experimental): CI failed because ruff linting (py_lint) reported an unused import (test_framework.messages.msg_notfound) in test/functional/p2p_orphan_handling.py.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
- Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
- A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
- An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.

</details>
instagibbs force-pushed on Sep 28, 2026

Type	Reviewers
Concept ACK	darosior

test: 1p1c child survives package feerate failure from another announcer

Check that when another announcer of the child delivers a witness-padded
parent that fails the package feerate check, the child stays in the
orphanage and the honest peer's package is still accepted, for both v2
and TRUC packages.

3be618c556

p2p: don't cancel parent requests when a witnessless parent is rejected

A TX_RECONSIDERABLE rejection forgets the transaction's wtxid in
TxRequestTracker for all peers. For a transaction without witness data
the wtxid equals the txid, which is the hash orphan resolution uses to
request a missing parent. Such a rejection therefore cancelled every
announcer's request for the parent, and the orphan stayed unresolved
until a new announcer appeared or the parent confirmed.

Keep those requests when the rejected transaction has no witness and an
orphan spends it.

d8dc948e13

test: orphan resolution survives rejection of a witnessless parent

Check that after another peer delivers a witnessless parent that is
rejected as reconsiderable, the child's announcer is still asked for the
parent and the package is accepted, for both a witness-stripped and a
nonsegwit parent.

ffc79605db

fuzz: check that one peer's outcome doesn't cancel others' requests

Add invariants to the txdownloadman_impl target: an event about one
transaction or peer must not remove other peers' transaction requests or
orphan announcements beyond that transaction's own hashes and entries.

Uniform random operations almost never reach the states where this
matters, so bias the harness towards orphans with missing parents and
towards TX_MISSING_INPUTS and TX_RECONSIDERABLE results. Without the
bias, reverting the witnessless-parent fix survived 57k runs.

3b16af7e7a

p2p: handle orphans by their actually missing parents

Orphan handling treated every input's parent as missing: each was
checked against the reject filters and requested from the orphan's
announcers. A confirmed parent whose txid had entered the reject filter,
which a rejected witnessless copy of it can cause, made the node drop
every orphan spending one of its outputs.

Have validation report which parents it found missing, and use only
those when storing a new orphan. Parents that are present are no longer
held against the orphan or requested.

Update the orphan handling functional test, which expected present
parents to be requested.

c2d498bab6

test: orphans are judged only by their actually missing parents

Check that a child spending a zero-fee parent and a confirmed output is
stored and resolved even after the confirmed transaction's txid enters
the reject filter, and that only the missing parent is requested.

Have the txdownloadman_impl fuzz target pass a random non-empty subset
of parents as missing.

bf6bbf5bde

p2p: don't add already-known transactions to the reject filter

TX_CONFLICT means the transaction is already known, in the mempool or
confirmed, not that it is invalid. Its wtxid still went into the reject
filter, and for a witnessless copy that is also the txid. If the known
parent then leaves the mempool before the next block, through eviction
or replacement, a child arriving afterwards is dropped as having a
rejected parent.

Don't add TX_CONFLICT results to the reject filter. The entry only saved
re-downloading and re-checking a transaction we already have.

Update the rejection-type table for TX_CONFLICT.

c8e49749af

test: known parent's stripped replay does not poison it across eviction

Check that after a stripped copy of a mempool parent is rejected as a
conflict and the parent is evicted, a child spending it is still stored
as an orphan.

32cf270494

instagibbs force-pushed on Sep 28, 2026
DrahtBot removed the label CI failed on Sep 28, 2026
fanquake added this to the milestone 33.0 on Sep 29, 2026
instagibbs commented at 9:13 PM on September 29, 2026: member

note: I was considering the "average case" cost of https://github.com/bitcoin/bitcoin/pull/36369/commits/c2d498bab6f165a3523af647bfb72e3d33c04678 since theoretically it could be a common pattern to be missing an early input and we could skip more validation, but it turns out there is essentially no meaningful load like this, after analyzing over 36k orphans I could reconstruct history of and do some qualatative benchmarks, it adds maybe 10ms of verification in a day spread over up to ~75k orphans. I think the clarity of reporting what we're actually missing is better.

The worst case is unchanged, where the entire tx is citing real inputs but they're all not the transaction maker's, so they fail script validation much later.
instagibbs commented at 7:14 PM on October 1, 2026: member
ah, #33066 probably needs ot be directly addressed for F3+F4

from a local skill I'm tweaking, from which I got an Approach NACK:

<details> <summary>Click to expand additional details</summary> A. Working notes

Status: PR #36369 by instagibbs is open and not a draft. It has 5 commits of fixes plus tests and fuzz coverage, 728+/55−, touching validation.cpp, net_processing.cpp, txdownloadman and txorphanage. The thread has one Concept ACK (darosior) and no pushback. The only author follow-up is a perf note on the validation change. The description matches the diff.
1. Problem. A peer sends a same-txid, different-witness copy of a CPFP parent. The honest package is then delayed at that hop "until a new announcer appears or the parent confirms." Anyone using 1p1c can hit this, for example LN anchor or TRUC closes. The evidence is a Project Loupe report plus functional tests and fuzz reproductions. There are no incidents in the wild. Goal one level up: fee-bumped, time-sensitive transactions should reach miners even when cheap peer-level griefing is attempted.
2. Baseline. An attacker who is directly connected to a node can stall a chosen package at that node. Stalling it network-wide needs connections to many nodes on the path to miners. Nothing has been observed in practice, but the attack is cheap and targeted.
3. Payoff. The PR closes four reproduced vectors (F1–F4 in the gist), and each one has a test showing the honest package now gets through. The author's own gist lists 11 residual issues of the same class (R0–R10), so the PR gives a partial liveness improvement, not a guarantee. #36371 (the liveness fuzz target) is follow-up work and doesn't count toward this PR's payoff.
4. Risks.
- Invariant enforced by discipline. The property the PR wants is "one peer's variant of a tx must not affect other peers' requests or orphan state." The code enforces it with special cases inside the shared handler, MempoolRejectedTx:
  
  !(RECONSIDERABLE && !HasWitness() && HaveChildren()) around ForgetTxHash;
  
  a TX_CONFLICT exclusion from the reject filter;
  
  a TX_RECONSIDERABLE skip in ProcessPackageResult;
  
  a new exception to the m_tx_download_mutex lock invariant.
  
  R1 in the gist shows a further hole in the same shared ForgetTxHash path, so the failure rate of this approach is already visible.
- Coupling into validation. F3 changes MemPoolAccept::PreChecks: it now scans every input, and it adds m_missing_parents to MempoolAcceptResult. That pushes an orphan-handling concern into validation, which is the costliest place to put it.
- Conflicts. DrahtBot lists 5 conflicting PRs, including #36015 (orphanage) and #36318 (validation).
- Reversibility. All changes are internal or policy, so they are easy to reverse.
1. Prior attempts.
- #33066 "p2p: never check tx rejections by txid" (glozow, 2025). It removed reject-filter lookups by parent txid, which is the read at txdownloadman_impl.cpp:379 and the txid path in AlreadyHaveTx. It got Concept ACKs from sipa and darosior. The author closed it "for now" after #33105 solved the triple-validation cost. Cause of death: deprioritized, not rejected. ajtowns's objection was narrow: keep the cache for INPUTS_NOT_STANDARD. Addressed here: not mentioned at all.
- #32379 (darosior, closed draft). It proposed dropping the WITNESS_STRIPPED special case. ajtowns argued it is still needed while orphan resolution goes by txid. That argument still holds and is consistent with this PR.
- #27742 BIP331 (closed). This is the sender-initiated design the author names as the fix-by-construction. The author scopes it out because it needs a reject-filter lookups by parent txid, which is the read at txdownloadman_impl.cpp:379 and the txid path in AlreadyHaveTx. It got Concept ACKs from sipa and darosior. The author closed it "for now" after #33105 solved the triple-validation cost. Cause of death: deprioritized, not rejected. ajtowns's objection was narrow: keep the cache for INPUTS_NOT_STANDARD. Addressed here: not mentioned at all.
- #32379 (darosior, closed draft). It proposed dropping the WITNESS_STRIPPED special case. ajtowns argued it is still needed while orphan resolution goes by txid. That argument still holds and is consistent with this PR.
- #27742 BIP331 (closed). This is the sender-initiated design the author names as the fix-by-construction. The author scopes it out because it needs a network-wide protocol upgrade, and that argument holds.
1. Alternatives.
Kept:
- Never check the reject filter by txid for orphan parents (prior: #33066). This removes the R7 class (F3 and F4) by construction, since no witness-dependent outcome can be read through the txid at all. It needs no PreChecks or MempoolAcceptResult change and no TX_CONFLICT carve-out. Its cost: a little bandwidth when re-fetching truly bad parents, and glozow measured zero txid hits over 2 weeks. Neither the PR nor its thread raises it.
- Type-aware TxRequestTracker forgetting (blind; also the author's own proposed fix for R1 in the gist). A wtxid-keyed failure would forget only wtxid announcements and leave txid orphan-resolution requests alone. That covers F2 and R1 with one rule instead of the HasWitness/HaveChildren special case. Its cost: txrequest has to track announcement type, and accept/block paths still forget by hash. Not raised in the thread.
Rejected:
- Document only (blind): fixes nothing; equivalent to the baseline.
- Detect and punish same-txid variants (blind): legitimate witness variants exist, and punishing the sender doesn't recover the honest package. (My argument.)
- Retry the parent from other announcers (blind): this is what F2 already does, so it isn't a different design.
- L2 multi-node submitpackage or a contrib watcher script (blind): only helps the submitter's own node, not later hops. (My argument.)
- BIP331 sender-initiated relay (prior/PR): needs a network-wide upgrade. (The author's argument, and it holds.)
F1 (keep the orphan after a package-feerate failure) has no competing design and looks correct as it is. 4. Prior attempts.
- #33066 "p2p: never check tx rejections by txid" (glozow, 2025). It removed reject-filter lookups by parent txid, which is the read at txdownloadman_impl.cpp:379 and the txid path in AlreadyHaveTx. It got Concept ACKs from sipa and darosior. The author closed it "for now" after #33105 solved the triple-validation cost. Cause of death: deprioritized, not rejected. ajtowns's objection was narrow: keep the cache for INPUTS_NOT_STANDARD. Addressed here: not mentioned at all.
- #32379 (darosior, closed draft). It proposed dropping the WITNESS_STRIPPED special case. ajtowns argued it is still needed while orphan resolution removed reject-filter lookups by parent txid, which is the read at txdownloadman_impl.cpp:379 and the txid path in AlreadyHaveTx. It got Concept ACKs from sipa and darosior. The author closed it "for now" after
  
  a TX_CONFLICT exclusion from the reject filter;
  
  a TX_RECONSIDERABLE skip in ProcessPackageResult;
  
  a new exception to the m_tx_download_mutex lock invariant.
  
  R1 in the gist shows a further hole in the same shared ForgetTxHash path, so the failure rate of this approach is already visible.
- Coupling into validation. F3 changes MemPoolAccept::PreChecks: it now scans every input, and it adds m_missing_parents to MempoolAcceptResult. That pushes an orphan-handling concern into validation, which is the costliest place to put it.
- Conflicts. DrahtBot lists 5 conflicting PRs, including #36015 (orphanage) and #36318 (validation).
- Reversibility. All changes are internal or policy, so they are easy to reverse.
1. Prior attempts.
- #33066 "p2p: never check tx rejections by txid" (glozow, 2025). It removed reject-filter lookups by parent txid, which is the read at txdownloadman_impl.cpp:379 and the txid path in AlreadyHaveTx. It got Concept ACKs from sipa and darosior. The author closed it "for now" after #33105 solved the triple-validation cost. Cause of death: deprioritized, not rejected. ajtowns's objection was narrow: keep the cache for INPUTS_NOT_STANDARD. Addressed here: not mentioned at all.
- #32379 (darosior, closed draft). It proposed dropping the WITNESS_STRIPPED special case. ajtowns argued it is still needed while orphan resolution goes by txid. That argument still holds and is consistent with this PR.
- #27742 BIP331 (closed). This is the sender-initiated design the author names as the fix-by-construction. The author scopes it out because it needs a network-wide protocol upgrade, and that argument holds.
1. Alternatives.
Kept:
- Never check the reject filter by txid for orphan parents (prior: #33066). This removes the R7 class (F3 and F4) by construction, since no witness-dependent outcome can be read through the txid at all. It needs no PreChecks or MempoolAcceptResult change and no TX_CONFLICT carve-out. Its cost: a little bandwidth when re-fetching truly bad parents, and glozow measured zero txid hits over 2 weeks. Neither the PR nor its thread raises it.
- Type-aware TxRequestTracker forgetting (blind; also the author's own proposed fix for R1 in the gist). A wtxid-keyed failure would forget only wtxid announcements and leave txid orphan-resolution requests alone. That covers F2 and R1 with one rule instead of the HasWitness/HaveChildren special case. Its cost: txrequest has to track announcement type, and accept/block paths still forget by hash. Not raised in the thread.
Rejected:
- Document only (blind): fixes nothing; equivalent to the baseline.
- Detect and punish same-txid variants (blind): legitimate witness variants exist, and punishing the sender doesn't recover the honest package. (My argument.)
- Retry the parent from other announcers (blind): this is what F2 already does, so it isn't a different design.
- L2 multi-node submitpackage or a contrib watcher script (blind): only helps the submitter's own node, not later hops. (My argument.)
- BIP331 sender-initiated relay (prior/PR): needs a network-wide upgrade. (The author's argument, and it holds.)
F1 (keep the orphan after a package-feerate failure) has no competing design and looks correct as it is.

Dominance: #33066's approach dominates F3+F4. It has the same payoff, removes the discipline risk by construction, and avoids the validation.cpp change. Type-aware forgetting likely dominates F2 and also closes R1.
1. Verdict. Concept ACK, Approach NACK. An alternative dominates per (5), and (0) and (2) are earned.
</details>
fametrano commented at 12:04 PM on October 2, 2026: contributor

I checked that the new tests catch what they fix, at 32cf270494. I reverted each of the four fixes on its own and ran txdownload_tests, p2p_opportunistic_1p1c.py and p2p_orphan_handling.py. Each revert makes at least one new test fail. F4 is caught only by the unit tests; no functional test covers it. I did not review the C++ changes.

One nit inline, in test_orphan_multiple_parents.

Happy to redo this check once F3 and F4 are reworked around #33066.

in test/functional/p2p_orphan_handling.py:295 in 32cf270494

 288 | @@ -291,13 +289,12 @@ def test_orphan_multiple_parents(self):
 289 |          self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY)
 290 |          peer.sync_with_ping()
 291 |          assert tx_in_orphanage(node, orphan["tx"])
 292 | -        assert_equal(len(peer.last_message["getdata"].inv), 2)
 293 | -        peer.wait_for_parent_requests([int(txid_conf_old, 16), int(missing_tx["txid"], 16)])
 294 | +        # Only the parent that is actually missing is requested. The confirmed parents, whether or not
 295 | +        # they are still in the recently-confirmed filter, and the in-mempool parent are present.
 296 | +        assert_equal(len(peer.last_message["getdata"].inv), 1)
 297 | +        peer.wait_for_parent_requests([int(missing_tx["txid"], 16)])

fametrano commented at 12:04 PM on October 2, 2026:

nit: assert_equal(len(peer.last_message["getdata"].inv), 1) can pass on the earlier getdata for the orphan, which also has one item. wait_for_parent_requests already checks the count. assert_never_requested checks directly that the long-confirmed parent is never requested. With this change the test passes, and with F3 reverted it fails at wait_for_parent_requests.

        peer.wait_for_parent_requests([int(missing_tx["txid"], 16)])
        peer.assert_never_requested(int(utxo_conf_old["txid"], 16))

Contributors

Labels

Milestone
33.0

Linked (view graph)

#36328 p2p: Reconsider package transactions that are immediately trimmed #36371 fuzz: add end-to-end liveness target for opportunistic 1p1c relay

p2p: don't let one peer's same-txid variant cancel another peer's 1p1c or orphan resolution #36369

Code Coverage & Benchmarks

Reviews

Conflicts