Skip to content

Commit

Permalink
[p2p] allow entries with the same txid in TxOrphanage
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@glozow
glozow committed May 14, 2024
1 parent c31f148 commit 8923edf
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 30 deletions.
5 changes: 4 additions & 1 deletion src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/test/orphanage_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
34 changes: 14 additions & 20 deletions src/txorphanage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
Expand All @@ -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)
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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 {
Expand Down Expand Up @@ -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",
Expand All @@ -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)
Expand All @@ -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;
}
Expand Down
10 changes: 3 additions & 7 deletions src/txorphanage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);
};
Expand Down

0 comments on commit 8923edf

Please sign in to comment.