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::iterator it; - it = m_orphans.lower_bound(Txid::FromUint256(InsecureRand256())); + std::map::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::iterator it = it_by_wtxid->second; + std::map::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::iterator iter = m_orphans.begin(); + std::map::iterator iter = m_orphans.begin(); while (iter != m_orphans.end()) { - std::map::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::iterator iter = m_orphans.begin(); + std::map::iterator iter = m_orphans.begin(); while (iter != m_orphans.end()) { - std::map::iterator maybeErase = iter++; + std::map::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& orphan_work_set = m_peer_work_set.try_emplace(elem->second.fromPeer).first->second; + std::set& 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 m_orphans GUARDED_BY(m_mutex); + std::map m_orphans GUARDED_BY(m_mutex); /** Which peer provided the orphans that need to be reconsidered */ - std::map> m_peer_work_set GUARDED_BY(m_mutex); + std::map> 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 m_orphan_list GUARDED_BY(m_mutex); - /** Index from wtxid into the m_orphans to lookup orphan - * transactions using their witness ids. */ - std::map m_wtxid_to_orphan_it GUARDED_BY(m_mutex); - /** Erase an orphan by wtxid */ int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); };