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 1, 2024
1 parent 842f7fd commit 646aa4d
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 51 deletions.
8 changes: 5 additions & 3 deletions src/net_processing.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2295,7 +2295,9 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside

const uint256& hash = gtxid.GetHash();

if (m_orphanage.HaveTx(gtxid)) return true;
// Orphanage is checked by wtxid. However, even if this is a txid, look up the same hash in
// case this is a non-segwit transaction in the orphanage.
if (m_orphanage.HaveTx(Wtxid::FromUint256(gtxid.GetHash()))) return true;

if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;

Expand Down Expand Up @@ -3219,7 +3221,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());
}
}
Expand All @@ -3237,7 +3239,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,
Expand Down
14 changes: 7 additions & 7 deletions src/test/fuzz/txorphan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,21 +112,21 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
{
CTransactionRef ref = orphanage.GetTxToReconsider(peer_id);
if (ref) {
bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetWitnessHash()));
bool have_tx = orphanage.HaveTx(ref->GetWitnessHash());
Assert(have_tx);
}
}
},
[&] {
bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash()));
bool have_tx = orphanage.HaveTx(tx->GetWitnessHash());
// AddTx should return false if tx is too big or already have it
// tx weight is unknown, we only check when tx is already in orphanage
{
bool add_tx = orphanage.AddTx(tx, peer_id);
// have_tx == true -> add_tx == false
Assert(!have_tx || !add_tx);
}
have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash()));
have_tx = orphanage.HaveTx(tx->GetWitnessHash());
{
bool add_tx = orphanage.AddTx(tx, peer_id);
// if have_tx is still false, it must be too big
Expand All @@ -135,15 +135,15 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
}
},
[&] {
bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetWitnessHash()));
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(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(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()));
}
},
[&] {
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
47 changes: 20 additions & 27 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 @@ -54,18 +52,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);
std::map<Wtxid, OrphanTx>::iterator it = m_orphans.find(wtxid);
if (it == m_orphans.end())
return 0;
const auto& txid = it->second.tx->GetHash();
for (const CTxIn& txin : it->second.tx->vin)
{
auto itPrev = m_outpoint_to_orphan_it.find(txin.prevout);
Expand All @@ -85,10 +84,8 @@ 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();
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_orphans.erase(it);
return 1;
Expand All @@ -101,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
std::map<Wtxid, 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);
Expand All @@ -124,12 +121,12 @@ 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->GetHash());
nErased += EraseTxNoLock(maybeErase->second.tx->GetWitnessHash());
} else {
nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime);
}
Expand Down Expand Up @@ -159,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 @@ -169,14 +166,10 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx)
}
}

bool TxOrphanage::HaveTx(const GenTxid& gtxid) const
bool TxOrphanage::HaveTx(const Wtxid& wtxid) const
{
LOCK(m_mutex);
if (gtxid.IsWtxid()) {
return m_wtxid_to_orphan_it.count(Wtxid::FromUint256(gtxid.GetHash()));
} else {
return m_orphans.count(Txid::FromUint256(gtxid.GetHash()));
}
return m_orphans.count(wtxid);
}

CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer)
Expand All @@ -187,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 All @@ -215,7 +208,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;
Expand All @@ -226,7 +219,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();
const auto& orphanHash = orphanTx.GetWitnessHash();
vOrphanErase.push_back(orphanHash);
}
}
Expand Down
20 changes: 8 additions & 12 deletions src/txorphanage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class TxOrphanage {
bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

/** Check if we already have an orphan transaction (by txid or wtxid) */
bool HaveTx(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
bool HaveTx(const Wtxid& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

/** Extract a transaction from a peer's work set
* Returns nullptr if there are no transactions to work on.
Expand All @@ -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);
Expand Down 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,12 +102,8 @@ 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 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

0 comments on commit 646aa4d

Please sign in to comment.