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
Stale ACK	Yudis-bit

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

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](/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