p2p: clean up a 1p1c package child that is already in the mempool #36578

instagibbs commented at 9:08 PM on October 19, 2026: member

An orphan that enters the mempool other than through peer relay (e.g. by RPC) stays in the orphanage, so Find1P1CPackage can pick it as the child of a reconsiderable parent, and package evaluation then reports it as MEMPOOL_ENTRY, which hits Assume(false). Have Find1P1CPackage erase such children from the orphanage and skip them.

DrahtBot added the label P2P on Oct 19, 2026

DrahtBot commented at 9:08 PM on October 19, 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/36578
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36578.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36578#author-ralph

Reviews

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

Type	Reviewers
ACK	fametrano
Stale 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:

#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.

instagibbs commented at 9:09 PM on October 19, 2026: member

@ismaelsadeeq

Yudis-bit commented at 8:39 AM on October 20, 2026: none

Tested ACK b0dda32aff on Windows.

ismaelsadeeq commented at 10:14 AM on October 21, 2026: member

Not sure, the Assume(false) removed is a no-op in release, and the replacement cleans the orphan only reactively when it's later re-selected as a 1p1c child. If that never happens, the orphan dangles until it expires. I'd rather we clean the orphanage proactively on mempool entry, via TransactionAddedToMempool? so an orphan that enters by any path is removed regardless. This is redundant ofc with the existing p2p cleanup for relayed txs, but we need both: the notification is async and can be processed before the orphan is inserted (it's EraseTx then no-ops), so the synchronous p2p path still does useful work.

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index ae329489e6..f23303605f 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -576,6 +576,8 @@ public:
     /** Overridden from CValidationInterface. */
     void ActiveTipChange(const CBlockIndex& new_tip, bool) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
+    void TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) override
+        EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
     void BlockConnected(const ChainstateRole& role, const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
     void BlockDisconnected(const std::shared_ptr<const CBlock> &block, const CBlockIndex* pindex) override
@@ -2190,6 +2192,12 @@ void PeerManagerImpl::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd)
     }
 }

+void PeerManagerImpl::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence)
+{
+    LOCK(m_tx_download_mutex);
+    m_txdownloadman.MempoolAcceptedTx(tx.info.m_tx);
+}
+
 /**
  * Evict orphan txn pool entries based on a newly connected
  * block, remember the recently confirmed transactions, and delete tracked
diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py
index b6e521dbe4..056bd72823 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -841,7 +841,7 @@ class OrphanHandlingTest(BitcoinTestFramework): [@cleanup](/github-metadata-backup-bitcoin-bitcoin/contributor/cleanup/)
     def test_orphan_in_mempool(self):
-        self.log.info("Test that an orphan in the mempool is removed from the orphanage when found as the child of a 1p1c package")
+        self.log.info("Test that an orphan is removed from the orphanage when it enters the mempool other than through peer relay")
         node = self.nodes[0]
         parent = self.wallet.create_self_transfer(fee_rate=0)
         child = self.wallet.create_self_transfer(utxo_to_spend=parent["new_utxo"])
@@ -854,12 +854,10 @@ class OrphanHandlingTest(BitcoinTestFramework):
         peer.send_and_ping(msg_tx(child["tx"]))
         assert tx_in_orphanage(node, child["tx"])

-        # The child enters the mempool by RPC, which does not remove it from the orphanage.
+        # The child enters the mempool by RPC. Even though this is not through peer relay, it is
+        # still removed from the orphanage.
         assert_equal(node.submitpackage([parent["hex"], child["hex"]])["package_msg"], "success")
-        assert tx_in_orphanage(node, child["tx"])
-
-        # The same parent version is paired with the orphan, which turns out to be in the mempool.
-        peer.send_and_ping(msg_tx(parent_bad_wit))
+        node.syncwithvalidationinterfacequeue()
         assert not tx_in_orphanage(node, child["tx"])
         assert child["txid"] in node.getrawmempool()

instagibbs commented at 1:08 PM on October 21, 2026: member

Not sure, the Assume(false) removed is a no-op in release

This would not have been an open issue otherwise :)

I'd rather we clean the orphanage proactively on mempool entry

I think if we're going to do that, we should simply have it call EraseTx to avoid additional AddChildrenToWorkSet invocations.

Another alternative to the synchronous piece in this PR would to have Find1P1CPackage detect children in the mempool as well as orphanage and clean it up there.

ismaelsadeeq commented at 1:33 PM on October 21, 2026: member

I think if we're going to do that, we should simply have it call EraseTx to avoid additional AddChildrenToWorkSet invocations.

Seems better to me; is there something I am missing about why you take this approach in this PR?

p2p: clean up a 1p1c package child that is already in the mempool

An orphan that enters the mempool other than through peer relay (e.g. by
RPC) stays in the orphanage, so Find1P1CPackage can pick it as the child
of a reconsiderable parent, and package evaluation then reports it as
MEMPOOL_ENTRY, which hits Assume(false). Have Find1P1CPackage erase such
children from the orphanage and skip them.

a93bdc8320

instagibbs commented at 1:56 PM on October 21, 2026: member

@ismaelsadeeq swtiched to the Find1P1CPackage strategy for now; can still be convinced the extra machinery to async sweep them and taking lock is worth it, but keeping strategy as is for now

instagibbs force-pushed on Oct 21, 2026

fanquake added this to the milestone 33.0 on Oct 21, 2026

fametrano commented at 7:50 AM on October 25, 2026: none

tACK a93bdc83202e00375b643e6df3b9daa801f7de23

The Assume(false) is also reached through sendrawtransaction, not only through submitpackage as in the new test. I checked on master in a Debug build. The parent pays the minimum relay fee for its own size. A peer sends a copy with a padded witness, which has the same txid but falls below the minimum relay feerate, so it is rejected as reconsiderable. The same peer's child is then kept as an orphan. Two sendrawtransaction calls put the real parent and the child in the mempool. When the peer sends the padded parent again, the node aborts at net_processing.cpp:3477. With this PR the child leaves the orphanage at that point and stays in the mempool.

In the same scenario the child stays in the orphanage after both sendrawtransaction calls, with or without this PR, until the padded parent pairs with it. That is the reactive cleanup ismaelsadeeq pointed out, and I agree with keeping a mempool hook out of this PR.

I tested the head merged into master a773e1e33b, in a Debug build, with p2p_orphan_handling (the new case aborts the node on master), p2p_opportunistic_1p1c and the txdownload and txpackage unit tests. I ran them again with #36594 and #36437 merged on top. I can share the functional test as a gist.