[p2p] allow entries with the same txid in TxOrphanage

Index by wtxid instead of txid to allow entries with the same txid but
different witnesses in orphanage. This prevents an attacker from
blocking a transaction from entering the orphanage by sending a mutated
version of it.

4 files changed, 23 insertions, 30 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5ae76e01bf..73c15d76b7 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2302,7 +2302,10 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside
         // Never query by txid: it is possible that the transaction in the orphanage has the same
         // txid but a different witness, which would give us a false positive result. If we decided
         // not to request the transaction based on this result, an attacker could prevent us from
-        // downloading a transaction by intentionally creating a malleated version of it.
+        // downloading a transaction by intentionally creating a malleated version of it.  While
+        // only one (or none!) of these transactions can ultimately be confirmed, we have no way of
+        // discerning which one that is, so the orphanage can store multiple transactions with the
+        // same txid.
         //
         // While we won't query by txid, we can try to "guess" what the wtxid is based on the txid.
         // A non-segwit transaction's txid == wtxid. Query this txid "casted" to a wtxid. This will
diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp
index b2643cf678..a93f7a4ef4 100644
--- a/src/test/orphanage_tests.cpp
+++ b/src/test/orphanage_tests.cpp
@@ -30,8 +30,8 @@ public:
     CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
     {
         LOCK(m_mutex);
-        std::map<Txid, OrphanTx>::iterator it;
-        it = m_orphans.lower_bound(Txid::FromUint256(InsecureRand256()));
+        std::map<Wtxid, OrphanTx>::iterator it;
+        it = m_orphans.lower_bound(Wtxid::FromUint256(InsecureRand256()));
         if (it == m_orphans.end())
             it = m_orphans.begin();
         return it->second.tx;
diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
index ca8a0e3a92..4a1d48a466 100644
--- a/src/txorphanage.cpp
+++ b/src/txorphanage.cpp
@@ -23,7 +23,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
 
     const Txid& hash = tx->GetHash();
     const Wtxid& wtxid = tx->GetWitnessHash();
-    if (m_orphans.count(hash))
+    if (m_orphans.count(wtxid))
         return false;
 
     // Ignore big transactions, to avoid a
@@ -40,11 +40,9 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
         return false;
     }
 
-    auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()});
+    auto ret = m_orphans.emplace(wtxid, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()});
     assert(ret.second);
     m_orphan_list.push_back(ret.first);
-    // Allow for lookups in the orphan pool by wtxid, as well as txid
-    m_wtxid_to_orphan_it.emplace(tx->GetWitnessHash(), ret.first);
     for (const CTxIn& txin : tx->vin) {
         m_outpoint_to_orphan_it[txin.prevout].insert(ret.first);
     }
@@ -63,10 +61,7 @@ int TxOrphanage::EraseTx(const Wtxid& wtxid)
 int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid)
 {
     AssertLockHeld(m_mutex);
-    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;
+    std::map<Wtxid, OrphanTx>::iterator it = m_orphans.find(wtxid);
     if (it == m_orphans.end())
         return 0;
     for (const CTxIn& txin : it->second.tx->vin)
@@ -91,7 +86,6 @@ int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid)
     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(wtxid);
 
     m_orphans.erase(it);
     return 1;
@@ -104,13 +98,13 @@ void TxOrphanage::EraseForPeer(NodeId peer)
     m_peer_work_set.erase(peer);
 
     int nErased = 0;
-    std::map<Txid, OrphanTx>::iterator iter = m_orphans.begin();
+    std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin();
     while (iter != m_orphans.end())
     {
-        std::map<Txid, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
-        if (maybeErase->second.fromPeer == peer)
-        {
-            nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
+        // increment to avoid iterator becoming invalid after erasure
+        const auto& [wtxid, orphan] = *iter++;
+        if (orphan.fromPeer == peer) {
+            nErased += EraseTxNoLock(wtxid);
         }
     }
     if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);
@@ -127,10 +121,10 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng)
         // Sweep out expired orphan pool entries:
         int nErased = 0;
         int64_t nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL;
-        std::map<Txid, OrphanTx>::iterator iter = m_orphans.begin();
+        std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin();
         while (iter != m_orphans.end())
         {
-            std::map<Txid, OrphanTx>::iterator maybeErase = iter++;
+            std::map<Wtxid, OrphanTx>::iterator maybeErase = iter++;
             if (maybeErase->second.nTimeExpire <= nNow) {
                 nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
             } else {
@@ -162,7 +156,7 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
             for (const auto& elem : it_by_prev->second) {
                 // Get this source peer's work set, emplacing an empty set if it didn't exist
                 // (note: if this peer wasn't still connected, we would have removed the orphan tx already)
-                std::set<Txid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
+                std::set<Wtxid>& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second;
                 // Add this tx to the work set
                 orphan_work_set.insert(elem->first);
                 LogPrint(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n",
@@ -175,7 +169,7 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
 bool TxOrphanage::HaveTx(const Wtxid& wtxid) const
 {
     LOCK(m_mutex);
-    return m_wtxid_to_orphan_it.count(wtxid);
+    return m_orphans.count(wtxid);
 }
 
 CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer)
@@ -186,10 +180,10 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer)
     if (work_set_it != m_peer_work_set.end()) {
         auto& work_set = work_set_it->second;
         while (!work_set.empty()) {
-            Txid txid = *work_set.begin();
+            Wtxid wtxid = *work_set.begin();
             work_set.erase(work_set.begin());
 
-            const auto orphan_it = m_orphans.find(txid);
+            const auto orphan_it = m_orphans.find(wtxid);
             if (orphan_it != m_orphans.end()) {
                 return orphan_it->second.tx;
             }
diff --git a/src/txorphanage.h b/src/txorphanage.h
index a549fc7f9b..5f9888c969 100644
--- a/src/txorphanage.h
+++ b/src/txorphanage.h
@@ -77,12 +77,12 @@ protected:
         size_t list_pos;
     };
 
-    /** Map from txid to orphan transaction record. Limited by
+    /** Map from wtxid to orphan transaction record. Limited by
      *  -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */
-    std::map<Txid, OrphanTx> m_orphans GUARDED_BY(m_mutex);
+    std::map<Wtxid, OrphanTx> m_orphans GUARDED_BY(m_mutex);
 
     /** Which peer provided the orphans that need to be reconsidered */
-    std::map<NodeId, std::set<Txid>> m_peer_work_set GUARDED_BY(m_mutex);
+    std::map<NodeId, std::set<Wtxid>> m_peer_work_set GUARDED_BY(m_mutex);
 
     using OrphanMap = decltype(m_orphans);
 
@@ -102,10 +102,6 @@ protected:
     /** Orphan transactions in vector for quick random eviction */
     std::vector<OrphanMap::iterator> m_orphan_list GUARDED_BY(m_mutex);
 
-    /** Index from wtxid into the m_orphans to lookup orphan
-     *  transactions using their witness ids. */
-    std::map<Wtxid, OrphanMap::iterator> m_wtxid_to_orphan_it GUARDED_BY(m_mutex);
-
     /** Erase an orphan by wtxid */
     int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);
 };