p2p: index TxOrphanage by wtxid, allow entries with same txid

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

src/test/orphanage_tests.cpp

@@ -30,8 +30,8 @@ class TxOrphanageTest : public TxOrphanage
  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;

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;
  }

src/txorphanage.h

@@ -77,12 +77,12 @@ class TxOrphanage {
  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 @@ class TxOrphanage {
  /** 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);
 };