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 https://git.fish.foo/bitcoin/bitcoin/pulls/36578#author-ralph

Reviews

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

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #36015 (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.

@ismaelsadeeq

Tested ACK b0dda32 on Windows.

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
     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()

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.

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?

@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

tACK a93bdc8

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 a773e1e, 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.