[refactor] TxOrphanage::EraseTx by wtxid

No behavior change right now, as transactions in the orphanage are
unique by txid. This makes the next commit easier to review.

Author: glozow

src/net_processing.cpp

@@ -3232,7 +3232,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx
 
     // 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->GetHash()) > 0) {
+    if (Assume(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());
     }
 }
@@ -3250,7 +3250,7 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c
 
     m_orphanage.AddChildrenToWorkSet(*tx);
     // If it came from the orphanage, remove it. No-op if the tx is not in txorphanage.
-    m_orphanage.EraseTx(tx->GetHash());
+    m_orphanage.EraseTx(tx->GetWitnessHash());
 
     LogDebug(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (wtxid=%s) (poolsz %u txn, %u kB)\n",
              nodeid,

src/test/fuzz/txorphan.cpp

@@ -137,12 +137,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
                     bool have_tx = orphanage.HaveTx(tx->GetWitnessHash());
                     // EraseTx should return 0 if m_orphans doesn't have the tx
                     {
-                        Assert(have_tx == orphanage.EraseTx(tx->GetHash()));
+                        Assert(have_tx == orphanage.EraseTx(tx->GetWitnessHash()));
                     }
                     have_tx = orphanage.HaveTx(tx->GetWitnessHash());
                     // have_tx should be false and EraseTx should fail
                     {
-                        Assert(!have_tx && !orphanage.EraseTx(tx->GetHash()));
+                        Assert(!have_tx && !orphanage.EraseTx(tx->GetWitnessHash()));
                     }
                 },
                 [&] {

src/txorphanage.cpp

@@ -54,16 +54,19 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
     return true;
 }
 
-int TxOrphanage::EraseTx(const Txid& txid)
+int TxOrphanage::EraseTx(const Wtxid& wtxid)
 {
     LOCK(m_mutex);
-    return EraseTxNoLock(txid);
+    return EraseTxNoLock(wtxid);
 }
 
-int TxOrphanage::EraseTxNoLock(const Txid& txid)
+int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid)
 {
     AssertLockHeld(m_mutex);
-    std::map<Txid, OrphanTx>::iterator it = m_orphans.find(txid);
+    auto it_by_wtxid = m_wtxid_to_orphan_it.find(wtxid);
+    if (it_by_wtxid == m_wtxid_to_orphan_it.end()) return 0;
+
+    std::map<Txid, OrphanTx>::iterator it = it_by_wtxid->second;
     if (it == m_orphans.end())
         return 0;
     for (const CTxIn& txin : it->second.tx->vin)
@@ -85,10 +88,10 @@ int TxOrphanage::EraseTxNoLock(const Txid& txid)
         m_orphan_list[old_pos] = it_last;
         it_last->second.list_pos = old_pos;
     }
-    const auto& wtxid = it->second.tx->GetWitnessHash();
+    const auto& txid = it->second.tx->GetHash();
     LogPrint(BCLog::TXPACKAGES, "   removed orphan tx %s (wtxid=%s)\n", txid.ToString(), wtxid.ToString());
     m_orphan_list.pop_back();
-    m_wtxid_to_orphan_it.erase(it->second.tx->GetWitnessHash());
+    m_wtxid_to_orphan_it.erase(wtxid);
 
     m_orphans.erase(it);
     return 1;
@@ -107,7 +110,7 @@ void TxOrphanage::EraseForPeer(NodeId peer)
         std::map<Txid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
         if (maybeErase->second.fromPeer == peer)
         {
-            nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
+            nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
         }
     }
     if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);
@@ -129,7 +132,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng)
         {
             std::map<Txid, OrphanTx>::iterator maybeErase = iter++;
             if (maybeErase->second.nTimeExpire <= nNow) {
-                nErased += EraseTxNoLock(maybeErase->second.tx->GetHash());
+                nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
             } else {
                 nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime);
             }
@@ -142,7 +145,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng)
     {
         // Evict a random orphan:
         size_t randompos = rng.randrange(m_orphan_list.size());
-        EraseTxNoLock(m_orphan_list[randompos]->first);
+        EraseTxNoLock(m_orphan_list[randompos]->second.tx->GetWitnessHash());
         ++nEvicted;
     }
     if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted);
@@ -211,7 +214,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
 {
     LOCK(m_mutex);
 
-    std::vector<Txid> vOrphanErase;
+    std::vector<Wtxid> vOrphanErase;
 
     for (const CTransactionRef& ptx : block.vtx) {
         const CTransaction& tx = *ptx;
@@ -222,8 +225,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
             if (itByPrev == m_outpoint_to_orphan_it.end()) continue;
             for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
                 const CTransaction& orphanTx = *(*mi)->second.tx;
-                const auto& orphanHash = orphanTx.GetHash();
-                vOrphanErase.push_back(orphanHash);
+                vOrphanErase.push_back(orphanTx.GetWitnessHash());
             }
         }
     }

src/txorphanage.h

@@ -33,8 +33,8 @@ class TxOrphanage {
      */
     CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
-    /** Erase an orphan by txid */
-    int EraseTx(const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+    /** Erase an orphan by wtxid */
+    int EraseTx(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /** Erase all orphans announced by a peer (eg, after that peer disconnects) */
     void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
@@ -106,8 +106,8 @@ class TxOrphanage {
      *  transactions using their witness ids. */
     std::map<Wtxid, OrphanMap::iterator> m_wtxid_to_orphan_it GUARDED_BY(m_mutex);
 
-    /** Erase an orphan by txid */
-    int EraseTxNoLock(const Txid& txid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
+    /** Erase an orphan by wtxid */
+    int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
 };
 
 #endif // BITCOIN_TXORPHANAGE_H