instagibbs
commented at 12:58 PM on October 5, 2026:
member
The first commit adds p2p_1p1c_liveness, an end-to-end fuzz target for parent and child relay. It
drives a real PeerManager with real messages and validation, and checks that if an honest peer announces
the child of an acceptable parent and child, the child reaches the mempool within a bounded time,
whatever other peers do. Known liveness holes are marked in the target as flags that turn the check off
once opened, including this one. With that flag turned off, the target finds this bug on master from an
empty corpus in under an hour on one core; an earlier version found it first
(https://github.com/bitcoin/bitcoin/pull/36371#issuecomment-5937814761).
The second commit hands reconsideration over to a remaining announcer when the marked announcement is
erased, and removes the flag.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
#36015 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36015.svg"></sub> (txorphanage: bound orphan memory by storing transactions serialized by brunoerg)
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
instagibbs marked this as ready for review on Oct 5, 2026
instagibbs
commented at 1:12 PM on October 5, 2026:
member
Prior harness found this PR's issue that is fixed, and this harness development found "R11", so I think it's already paid for itself
<details>
<summary>Concept review notes</summary>
Diff. Two commits.
294d6ffcd0 changes TxOrphanageImpl::Erase in about 20 lines. When the erased announcement was the
one marked for reconsideration, the mark moves to the remaining announcer with the lowest NodeId. The
comment in AddChildrenToWorkSet and the doc comment for EraseForPeer are updated, and
txorphanage_sim and orphanage_tests are updated to match.
345497515a adds the p2p_1p1c_liveness fuzz target (547 lines, test-only).
The description matches the diff.
Problem. The node is sent a parent and a child (for example a CPFP), and the child does not reach the
mempool until the orphan expires or is evicted. The trigger is one announcer leaving at the wrong
moment, and an adversary can try to cause it. The evidence is a fuzzer reproduction on master. There is
no reported incident. The goal one level up is that 1p1c relay gets a child accepted in bounded time
whatever adversarial peers do.
Baseline. The child is delayed until orphan expiry or eviction. The attacker needs to be the randomly
chosen announcer, which is likelier with many sybil connections, and it must disconnect before its next
ProcessMessages turn. In that turn ProcessOrphanTx runs first, and the turn is skipped once
fDisconnect is set. If the attacker also supplies the parent and then closes the socket, the race is
plausible. The cost is delay, not loss.
Payoff. The fix closes the hole for both the disconnect and the eviction path. The new unit and sim
tests demonstrate it, and the liveness harness checks it end to end. The payoff is modest but real.
Risks.
The invariant: "an orphan whose inputs became available keeps a reconsiderable announcement while any
announcement exists." It is enforced by construction, because every removal of an announcement goes
through Erase<Tag>.
The change stays inside txorphanage. net_processing and txdownloadman are untouched.
Blast radius is relay liveness and, at worst, extra orphan-validation work. No consensus or fund
exposure, and no new Asserts.
The change is internal and easy to reverse.
Review cost is mostly the harness. It drives a real PeerManager, so it is a possible source of
maintenance work and flakiness.
Prior discussion. This came up in #31666, which introduced the single random announcer:
That proposal was deferred, not rejected. This PR implements it, and the fuzzer reproduction is new
evidence against the impracticality point. No closed PRs attempt this fix.
Alternatives.
Kept, both trades rather than improvements:
Wtxid-keyed reconsider queue that any announcer drains. Removes per-peer ownership structurally, but
changes the TxOrphanage interface and moves the cost of other peers' work into each peer's processing
turn.
Detect-and-heal sweep for orphans with available inputs and no reconsider mark. Catches causes not
yet known, but adds a second mechanism and CPU cost.
Rejected:
Mark every announcer reconsiderable. It undoes ed24e01696 (at most one reconsiderable announcement
per wtxid) and wastes work. This is glozow's argument from #31666.
Process one workset item at disconnect. An attacker defeats it by putting two items in the workset.
Also glozow's argument from #31666.
Process children synchronously after the parent is accepted. It changes net_processing's per-peer
processing budget, a larger change in a costlier place.
Docs-only or tests-only. They leave the hole open.
Re-request the child via txrequest. Adds a new bandwidth path an attacker can trigger.
The PR takes the shallowest placement of the candidates.
Dominance: none.
Verdict:Concept ACK. The fix delivers the payoff without the harness. The harness is test-only and
has already found bugs, so splitting it out would not change the verdict.
instagibbs force-pushed on Oct 5, 2026
DrahtBot added the label CI failed on Oct 5, 2026
instagibbs
commented at 2:25 PM on October 5, 2026:
member
DrahtBot removed the label CI failed on Oct 5, 2026
w0xlt
commented at 1:34 AM on October 8, 2026:
contributor
Concept ACK
instagibbs force-pushed on Oct 11, 2026
fuzz: add end-to-end liveness target for parent and child relay
Check that if an honest peer announces the child of an acceptable
parent and child, the child ends up in the mempool within a bounded
time, whatever other peers do. The target drives a real PeerManager
with real messages and validation, which the existing targets cannot,
as they model validation results.
After an adversarial prefix the honest peer only answers requests. With
an outbound honest peer the adversaries keep acting; with an inbound
one they go quiet. Adversary connections are limited, and adversaries
do not refill outbound slots.
Known liveness holes are marked in the target and turn the check off
once opened, including the lost orphan reconsideration fixed in the
next commit.
47776a909e
txorphanage: hand reconsideration over when its announcement is erased
A child that arrives before its parent is kept as an orphan. Once the
parent is accepted on its own, the orphan is put in one randomly chosen
announcer's work set to be retried. If that announcer disconnects or is
evicted first, the orphan is never retried, and re-announcements by the
other announcers don't fetch it again since it is already held. This
keeps e.g. a CPFP child of a parent that pays for itself out of the
mempool until the orphan is evicted, and an announcer can cause it on
purpose.
Hand reconsideration over to the remaining announcer with the lowest
NodeId instead.
Remove its hole from the liveness target.
65b48ba1f1
instagibbs force-pushed on Oct 12, 2026
instagibbs
commented at 9:24 PM on October 12, 2026:
member
added coinbase-based censorship cast stuff to the PR. Does not add any new holes per se, but covers more ground.
@FlashWayne this PR and the follow-up in the OP should cover known holes. If we can get this in first, then subtracting holes just becomes a matter of implementation debate, and which are worth filling. parent_stripped does seem worth filling, for sure.
fametrano
commented at 7:48 AM on October 25, 2026:
contributor
tACK65b48ba1f1f4c6c04a0d1bea9f7774091ec4e130
I reproduced the hole end to end with a Boost case of my own. Peer A sends twelve children and peer B announces each of them by wtxid, so each orphan has two announcers. A then sends the parent and disconnects before its next turn, and B runs until it has no more work. On master 6 of the 12 children reach the mempool, and the other 6 stay in the orphanage with nobody to reconsider them. With this PR all 12 are accepted in 13 turns of B, and the orphanage is empty. The updated peer_worksets case fails on master at its new hand-over checks and passes here.
The hand-over keeps the callers' iterators valid. EraseForPeer, EraseTxInternal and LimitOrphans advance their iterator before each erase. In Erase(), lower_bound can land on the announcement being erased, and the ++ skips it. modify() only changes the other announcement's m_reconsider, which moves it within ByPeer alone, and that key stays unique through m_entry_sequence.
I tested the head merged into master a773e1e33b, in a Debug build, with the orphanage, denialofservice and txdownload unit tests, p2p_orphan_handling and p2p_opportunistic_1p1c. Under libFuzzer, p2p_1p1c_liveness ran for 10 minutes and txorphanage_sim for 3, with no crash. I repeated all of it with #36594 and #36578 merged on top, with the liveness target for 5 minutes and txorphanage_sim for 1. I can share the test case as a gist.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-10-30 09:40 UTC
