Merge #19596: Deduplicate parent txid loop of requested transactions and missing parents of orphan transactions

4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar)
8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar)

Pull request description:

  I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs.  There may be thousands of inputs in a transaction, and the same txid may appear many times.  In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter.

  This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan.

  This addresses a couple of [related](bitcoin/bitcoin#19109 (comment)) [comments](bitcoin/bitcoin#19109 (comment)) left in #19109.

ACKs for top commit:
  laanwj:
    Code review ACK 4c0731f
  jonatack:
    ACK 4c0731f
  ajtowns:
    ACK 4c0731f

Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d

Author: laanwj

src/net_processing.cpp

@@ -1741,16 +1741,27 @@ void static ProcessGetData(CNode& pfrom, const CChainParams& chainparams, CConnm
             connman.PushMessage(&pfrom, msgMaker.Make(nSendFlags, NetMsgType::TX, *tx));
             mempool.RemoveUnbroadcastTx(tx->GetHash());
             // As we're going to send tx, make sure its unconfirmed parents are made requestable.
-            for (const auto& txin : tx->vin) {
-                auto txinfo = mempool.info(txin.prevout.hash);
-                if (txinfo.tx && txinfo.m_time > now - UNCONDITIONAL_RELAY_DELAY) {
-                    // Relaying a transaction with a recent but unconfirmed parent.
-                    if (WITH_LOCK(pfrom.m_tx_relay->cs_tx_inventory, return !pfrom.m_tx_relay->filterInventoryKnown.contains(txin.prevout.hash))) {
-                        LOCK(cs_main);
-                        State(pfrom.GetId())->m_recently_announced_invs.insert(txin.prevout.hash);
+            std::vector<uint256> parent_ids_to_add;
+            {
+                LOCK(mempool.cs);
+                auto txiter = mempool.GetIter(tx->GetHash());
+                if (txiter) {
+                    const CTxMemPool::setEntries& parents = mempool.GetMemPoolParents(*txiter);
+                    parent_ids_to_add.reserve(parents.size());
+                    for (CTxMemPool::txiter parent_iter : parents) {
+                        if (parent_iter->GetTime() > now - UNCONDITIONAL_RELAY_DELAY) {
+                            parent_ids_to_add.push_back(parent_iter->GetTx().GetHash());
+                        }
                     }
                 }
             }
+            for (const uint256& parent_txid : parent_ids_to_add) {
+                // Relaying a transaction with a recent but unconfirmed parent.
+                if (WITH_LOCK(pfrom.m_tx_relay->cs_tx_inventory, return !pfrom.m_tx_relay->filterInventoryKnown.contains(parent_txid))) {
+                    LOCK(cs_main);
+                    State(pfrom.GetId())->m_recently_announced_invs.insert(parent_txid);
+                }
+            }
         } else {
             vNotFound.push_back(inv);
         }
@@ -2999,8 +3010,19 @@ void ProcessMessage(
         else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS)
         {
             bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected
+
+            // 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) {
-                if (recentRejects->contains(txin.prevout.hash)) {
+                // We start with all parents, and then remove duplicates below.
+                unique_parents.push_back(txin.prevout.hash);
+            }
+            std::sort(unique_parents.begin(), unique_parents.end());
+            unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end());
+            for (const uint256& parent_txid : unique_parents) {
+                if (recentRejects->contains(parent_txid)) {
                     fRejectedParents = true;
                     break;
                 }
@@ -3009,14 +3031,14 @@ void ProcessMessage(
                 uint32_t nFetchFlags = GetFetchFlags(pfrom);
                 const auto current_time = GetTime<std::chrono::microseconds>();
 
-                for (const CTxIn& txin : tx.vin) {
+                for (const uint256& parent_txid : unique_parents) {
                     // Here, we only have the txid (and not wtxid) of the
                     // inputs, so we only request in txid mode, even for
                     // wtxidrelay peers.
                     // Eventually we should replace this with an improved
                     // protocol for getting all unconfirmed parents.
-                    CInv _inv(MSG_TX | nFetchFlags, txin.prevout.hash);
-                    pfrom.AddKnownTx(txin.prevout.hash);
+                    CInv _inv(MSG_TX | nFetchFlags, parent_txid);
+                    pfrom.AddKnownTx(parent_txid);
                     if (!AlreadyHave(_inv, mempool)) RequestTx(State(pfrom.GetId()), ToGenTxid(_inv), current_time);
                 }
                 AddOrphanTx(ptx, pfrom.GetId());