[refactor] put peerman tasks at the end of ProcessInvalidTx

Author: glozow

src/net_processing.cpp

@@ -573,7 +573,7 @@ class PeerManagerImpl final : public PeerManager
                                    NodeId parent_sender,
                                    NodeId child_sender) :
             m_txns{parent, child},
-            m_senders {parent_sender, child_sender}
+            m_senders{parent_sender, child_sender}
         {}
 
         // Move ctor
@@ -3033,6 +3033,11 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::ProcessInvali
         ptx->GetWitnessHash().ToString(),
         nodeid,
         state.ToString());
+
+    // Whether we should call AddToCompactExtraTransactions at the end
+    bool add_extra_compact_tx{first_time_failure};
+    // Hashes to pass to AddKnownTx later
+    std::vector<uint256> unique_parents;
     // Populated if failure is reconsiderable and eligible package is found.
     std::optional<PackageToValidate> package_to_validate;
 
@@ -3044,7 +3049,6 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::ProcessInvali
 
             // Deduplicate parent txids, so that we don't have to loop over
             // the same parent txid more than once down below.
-            std::vector<uint256> unique_parents;
             unique_parents.reserve(tx.vin.size());
             for (const CTxIn& txin : tx.vin) {
                 // We start with all parents, and then remove duplicates below.
@@ -3081,17 +3085,15 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::ProcessInvali
                     // Eventually we should replace this with an improved
                     // protocol for getting all unconfirmed parents.
                     const auto gtxid{GenTxid::Txid(parent_txid)};
-                    if (peer) AddKnownTx(*peer, parent_txid);
                     // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been
                     // previously rejected for being too low feerate. This orphan might CPFP it.
                     if (!m_txdownloadman.AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) {
                         m_txdownloadman.AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false);
                     }
                 }
 
-                if (m_orphanage.AddTx(ptx, nodeid) && RecursiveDynamicUsage(*ptx) < 100000) {
-                    AddToCompactExtraTransactions(ptx);
-                }
+                // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there
+                add_extra_compact_tx &= m_orphanage.AddTx(ptx, nodeid);
 
                 // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore.
                 m_txrequest.ForgetTxHash(tx.GetHash());
@@ -3100,6 +3102,7 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::ProcessInvali
                 // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789)
                 m_orphanage.LimitOrphans(m_opts.max_orphan_txs, m_rng);
             } else {
+                unique_parents.clear();
                 LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n",
                          tx.GetHash().ToString(),
                          tx.GetWitnessHash().ToString());
@@ -3115,8 +3118,9 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::ProcessInvali
                 m_txrequest.ForgetTxHash(tx.GetWitnessHash());
             }
         }
-        return std::nullopt;
-    } else if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
+    } else if (state.GetResult() == TxValidationResult::TX_WITNESS_STRIPPED) {
+        add_extra_compact_tx = false;
+    } else {
         // We can add the wtxid of this transaction to our reject filter.
         // Do not add txids of witness transactions or witness-stripped
         // transactions to the filter, as they can have been malleated;
@@ -3161,16 +3165,19 @@ std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::ProcessInvali
             RecentRejectsFilter().insert(ptx->GetHash().ToUint256());
             m_txrequest.ForgetTxHash(ptx->GetHash());
         }
-        if (first_time_failure && RecursiveDynamicUsage(*ptx) < 100000) {
-            AddToCompactExtraTransactions(ptx);
-        }
+    }
+    if (add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) {
+        AddToCompactExtraTransactions(ptx);
+    }
+    for (const uint256& parent_txid : unique_parents) {
+        if (peer) AddKnownTx(*peer, parent_txid);
     }
 
     MaybePunishNodeForTx(nodeid, state);
 
     // If the tx failed in ProcessOrphanTx, it should be removed from the orphanage unless the
     // tx was still missing inputs. If the tx was not in the orphanage, EraseTx does nothing and returns 0.
-    if (Assume(state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) {
+    if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS && m_orphanage.EraseTx(ptx->GetWitnessHash()) > 0) {
         LogDebug(BCLog::TXPACKAGES, "   removed orphan tx %s (wtxid=%s)\n", ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());
     }