net_processing: Don't process tx after processing orphans

If we made progress on orphans, consider that enough work for this peer
for this round of ProcessMessages. This also allows cleaning up the api
for TxOrphange:GetTxToReconsider().

src/net_processing.cpp

@@ -584,14 +584,17 @@ private:
     /**
      * Reconsider orphan transactions after a parent has been accepted to the mempool.
      *
-     * @peer[in]  peer     The peer whose orphan transactions we will reconsider. Generally only one
+     * @peer[in]  peer     The peer whose orphan transactions we will reconsider. Generally only
-     *                     orphan will be reconsidered on each call of this function. This set
+     *                     one orphan will be reconsidered on each call of this function. If an
-     *                     may be added to if accepting an orphan causes its children to be
+     *                     accepted orphan has orphaned children, those will need to be
-     *                     reconsidered.
+     *                     reconsidered, creating more work, possibly for other peers.
-     * @return             True if there are still orphans in this peer's work set.
+     * @return             True if meaningful work was done (an orphan was accepted/rejected).
+     *                     If no meaningful work was done, then the work set for this peer
+     *                     will be empty.
      */
     bool ProcessOrphanTx(Peer& peer)
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex);
+
     /** Process a single headers message from a peer.
      *
      * @param[in]   pfrom     CNode of the peer
@@ -2892,9 +2895,8 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
     LOCK(cs_main);
 
     CTransactionRef porphanTx = nullptr;
-    bool more = false;
 
-    while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id, more)) {
+    while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id)) {
         const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx);
         const TxValidationState& state = result.m_state;
         const uint256& orphanHash = porphanTx->GetHash();
@@ -2907,7 +2909,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
             for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) {
                 AddToCompactExtraTransactions(removedTx);
             }
-            break;
+            return true;
         } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) {
             if (state.IsInvalid()) {
                 LogPrint(BCLog::MEMPOOL, "   invalid orphan tx %s from peer=%d. %s\n",
@@ -2950,11 +2952,11 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer)
                 }
             }
             m_orphanage.EraseTx(orphanHash);
-            break;
+            return true;
         }
     }
 
-    return more;
+    return false;
 }
 
 bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer,
@@ -4849,12 +4851,12 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
         }
     }
 
-    const bool has_more_orphans = ProcessOrphanTx(*peer);
+    const bool processed_orphan = ProcessOrphanTx(*peer);
 
     if (pfrom->fDisconnect)
         return false;
 
-    if (has_more_orphans) return true;
+    if (processed_orphan) return true;
 
     // this maintains the order of responses
     // and prevents m_getdata_requests to grow unbounded

src/test/fuzz/txorphan.cpp

@@ -89,11 +89,8 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
                 },
                 [&] {
                     {
-                        bool more = true;
+                        CTransactionRef ref = orphanage.GetTxToReconsider(peer_id);
-                        CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, more);
+                        if (ref) {
-                        if (!ref) {
-                            Assert(!more);
-                        } else {
                             bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash()));
                             Assert(have_tx);
                         }

src/txorphanage.cpp

@@ -174,7 +174,7 @@ bool TxOrphanage::HaveTx(const GenTxid& gtxid) const
     }
 }
 
-CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, bool& more)
+CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer)
 {
     LOCK(m_mutex);
 
@@ -187,12 +187,10 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, bool& more)
 
             const auto orphan_it = m_orphans.find(txid);
             if (orphan_it != m_orphans.end()) {
-                more = !work_set.empty();
                 return orphan_it->second.tx;
             }
         }
     }
-    more = false;
     return nullptr;
 }
 

src/txorphanage.h

@@ -27,13 +27,11 @@ public:
     bool HaveTx(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /** Extract a transaction from a peer's work set
-     *  Returns nullptr and sets more to false if there are no transactions
+     *  Returns nullptr if there are no transactions to work on.
-     *  to work on. Otherwise returns the transaction reference, removes
+     *  Otherwise returns the transaction reference, and removes
-     *  the transaction from the work set, and sets "more" to indicate
+     *  it from the work set.
-     *  if there are more orphans for this peer
-     *  to work on after this tx.
      */
-    CTransactionRef GetTxToReconsider(NodeId peer, bool& more) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+    CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /** Erase an orphan by txid */
     int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);