From 7e475b9648bbee04f5825b922ba0399373eaa5a9 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 10 May 2024 09:23:18 +0100 Subject: [PATCH 1/7] [p2p] don't query orphanage by txid --- src/net_processing.cpp | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bdbf077ab5..985b41b160 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2295,7 +2295,20 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside const uint256& hash = gtxid.GetHash(); - if (m_orphanage.HaveTx(gtxid)) return true; + if (gtxid.IsWtxid()) { + // Normal query by wtxid. + if (m_orphanage.HaveTx(gtxid)) return true; + } else { + // 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. + // + // 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 + // help us find non-segwit transactions, saving bandwidth, and should have no false positives. + if (m_orphanage.HaveTx(GenTxid::Wtxid(hash))) return true; + } if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true; From efcc5930175f31b685adb4627a038d9f0848eb1f Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 10 May 2024 10:55:39 +0100 Subject: [PATCH 2/7] [refactor] TxOrphanage::HaveTx only by wtxid --- src/net_processing.cpp | 4 ++-- src/test/fuzz/txorphan.cpp | 11 +++++------ src/txorphanage.cpp | 8 ++------ src/txorphanage.h | 4 ++-- 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 985b41b160..0b8b896987 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2297,7 +2297,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside if (gtxid.IsWtxid()) { // Normal query by wtxid. - if (m_orphanage.HaveTx(gtxid)) return true; + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; } else { // 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 @@ -2307,7 +2307,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside // 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 // help us find non-segwit transactions, saving bandwidth, and should have no false positives. - if (m_orphanage.HaveTx(GenTxid::Wtxid(hash))) return true; + if (m_orphanage.HaveTx(Wtxid::FromUint256(hash))) return true; } if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true; diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index a44f47b00d..5e9034e7f7 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -112,13 +112,12 @@ 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())); - Assert(have_tx); + Assert(orphanage.HaveTx(ref->GetWitnessHash())); } } }, [&] { - 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 { @@ -126,7 +125,7 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // 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 @@ -135,12 +134,12 @@ 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())); } - 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())); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 82206e56c3..e10f01c693 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -169,14 +169,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_wtxid_to_orphan_it.count(wtxid); } CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) diff --git a/src/txorphanage.h b/src/txorphanage.h index a3c8edaa2a..33e41ff5b7 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -23,8 +23,8 @@ public: /** Add a new orphan transaction */ 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); + /** Check if we already have an orphan transaction (by wtxid only) */ + 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. From c31f148166f01a9167d82501a77823785d28a841 Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 10 May 2024 12:04:50 +0100 Subject: [PATCH 3/7] [refactor] TxOrphanage::EraseTx by wtxid No behavior change right now, as transactions in the orphanage are unique by txid. This makes the next commit easier to review. --- src/net_processing.cpp | 4 ++-- src/test/fuzz/txorphan.cpp | 4 ++-- src/txorphanage.cpp | 26 ++++++++++++++------------ src/txorphanage.h | 8 ++++---- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0b8b896987..5ae76e01bf 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3232,7 +3232,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()); } } @@ -3250,7 +3250,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, diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 5e9034e7f7..64f6ccfcda 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -137,12 +137,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) 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(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())); } }, [&] { diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index e10f01c693..ca8a0e3a92 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -54,16 +54,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::iterator it = m_orphans.find(txid); + 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; if (it == m_orphans.end()) return 0; for (const CTxIn& txin : it->second.tx->vin) @@ -85,10 +88,10 @@ 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(); + 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(it->second.tx->GetWitnessHash()); + m_wtxid_to_orphan_it.erase(wtxid); m_orphans.erase(it); return 1; @@ -107,7 +110,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) std::map::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); @@ -129,7 +132,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { std::map::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); } @@ -142,7 +145,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size()); - EraseTxNoLock(m_orphan_list[randompos]->first); + EraseTxNoLock(m_orphan_list[randompos]->second.tx->GetWitnessHash()); ++nEvicted; } if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); @@ -211,7 +214,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) { LOCK(m_mutex); - std::vector vOrphanErase; + std::vector vOrphanErase; for (const CTransactionRef& ptx : block.vtx) { const CTransaction& tx = *ptx; @@ -222,8 +225,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(); - vOrphanErase.push_back(orphanHash); + vOrphanErase.push_back(orphanTx.GetWitnessHash()); } } } diff --git a/src/txorphanage.h b/src/txorphanage.h index 33e41ff5b7..a549fc7f9b 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -33,8 +33,8 @@ public: */ 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); @@ -106,8 +106,8 @@ protected: * transactions using their witness ids. */ std::map 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 From 8923edfc1f12ebc6a074651c084ba7d249074799 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 29 Apr 2024 16:40:28 +0100 Subject: [PATCH 4/7] [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. --- src/net_processing.cpp | 5 ++++- src/test/orphanage_tests.cpp | 4 ++-- src/txorphanage.cpp | 34 ++++++++++++++-------------------- src/txorphanage.h | 10 +++------- 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::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); }; From 6675f6428d653bf7a53537bd773114f4fb5ba53f Mon Sep 17 00:00:00 2001 From: glozow Date: Fri, 10 May 2024 12:38:03 +0100 Subject: [PATCH 5/7] [unit test] TxOrphanage handling of same-txid-different-witness txns --- src/test/orphanage_tests.cpp | 53 ++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index a93f7a4ef4..450bf6a4fc 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -70,6 +70,16 @@ static CTransactionRef MakeTransactionSpending(const std::vector& out return MakeTransactionRef(tx); } +// Make another (not necessarily valid) tx with the same txid but different wtxid. +static CTransactionRef MakeMutation(const CTransactionRef& ptx) +{ + CMutableTransaction tx(*ptx); + tx.vin[0].scriptWitness.stack.push_back({5}); + auto mutated_tx = MakeTransactionRef(tx); + assert(ptx->GetHash() == mutated_tx->GetHash()); + return mutated_tx; +} + static bool EqualTxns(const std::set& set_txns, const std::vector& vec_txns) { if (vec_txns.size() != set_txns.size()) return false; @@ -180,6 +190,49 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) BOOST_CHECK(orphanage.CountOrphans() == 0); } +BOOST_AUTO_TEST_CASE(same_txid_diff_witness) +{ + FastRandomContext det_rand{true}; + TxOrphanage orphanage; + NodeId peer{0}; + + std::vector empty_outpoints; + auto parent = MakeTransactionSpending(empty_outpoints, det_rand); + + // Create children to go into orphanage. + auto child_normal = MakeTransactionSpending({{parent->GetHash(), 0}}, det_rand); + auto child_mutated = MakeMutation(child_normal); + + const auto& normal_wtxid = child_normal->GetWitnessHash(); + const auto& mutated_wtxid = child_mutated->GetWitnessHash(); + BOOST_CHECK(normal_wtxid != mutated_wtxid); + + BOOST_CHECK(orphanage.AddTx(child_normal, peer)); + // EraseTx fails as transaction by this wtxid doesn't exist. + BOOST_CHECK_EQUAL(orphanage.EraseTx(mutated_wtxid), 0); + BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); + + // Must succeed. Both transactions should be present in orphanage. + BOOST_CHECK(orphanage.AddTx(child_mutated, peer)); + BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(orphanage.HaveTx(mutated_wtxid)); + + // Outpoints map should track all entries: check that both are returned as children of the parent. + std::set expected_children{child_normal, child_mutated}; + BOOST_CHECK(EqualTxns(expected_children, orphanage.GetChildrenFromSamePeer(parent, peer))); + + // Erase by wtxid: mutated first + BOOST_CHECK_EQUAL(orphanage.EraseTx(mutated_wtxid), 1); + BOOST_CHECK(orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); + + BOOST_CHECK_EQUAL(orphanage.EraseTx(normal_wtxid), 1); + BOOST_CHECK(!orphanage.HaveTx(normal_wtxid)); + BOOST_CHECK(!orphanage.HaveTx(mutated_wtxid)); +} + + BOOST_AUTO_TEST_CASE(get_children) { FastRandomContext det_rand{true}; From b16da7eda76944719713be68b61f03d4acdd3e16 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 29 Apr 2024 16:06:17 +0100 Subject: [PATCH 6/7] [functional test] attackers sending mutated orphans --- test/functional/p2p_orphan_handling.py | 174 ++++++++++++++++++++++++- 1 file changed, 173 insertions(+), 1 deletion(-) diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index 6166c62aa2..f525d87cca 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -7,6 +7,7 @@ import time from test_framework.messages import ( CInv, + CTxInWitness, MSG_TX, MSG_WITNESS_TX, MSG_WTX, @@ -21,6 +22,7 @@ from test_framework.p2p import ( NONPREF_PEER_TX_DELAY, OVERLOADED_PEER_TX_DELAY, p2p_lock, + P2PInterface, P2PTxInvStore, TXID_RELAY_DELAY, ) @@ -127,6 +129,22 @@ class OrphanHandlingTest(BitcoinTestFramework): peer.wait_for_getdata([wtxid]) peer.send_and_ping(msg_tx(tx)) + def create_malleated_version(self, tx): + """ + Create a malleated version of the tx where the witness is replaced with garbage data. + Returns a CTransaction object. + """ + tx_bad_wit = tx_from_hex(tx["hex"]) + tx_bad_wit.wit.vtxinwit = [CTxInWitness()] + # Add garbage data to witness 0. We cannot simply strip the witness, as the node would + # classify it as a transaction in which the witness was missing rather than wrong. + tx_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage'] + + assert_equal(tx["txid"], tx_bad_wit.rehash()) + assert tx["wtxid"] != tx_bad_wit.getwtxid() + + return tx_bad_wit + @cleanup def test_arrival_timing_orphan(self): self.log.info("Test missing parents that arrive during delay are not requested") @@ -284,8 +302,8 @@ class OrphanHandlingTest(BitcoinTestFramework): # doesn't give up on the orphan. Once all of the missing parents are received, it should be # submitted to mempool. peer.send_message(msg_notfound(vec=[CInv(MSG_WITNESS_TX, int(txid_conf_old, 16))])) + # Sync with ping to ensure orphans are reconsidered peer.send_and_ping(msg_tx(missing_tx["tx"])) - peer.sync_with_ping() assert_equal(node.getmempoolentry(orphan["txid"])["ancestorcount"], 3) @cleanup @@ -394,10 +412,161 @@ class OrphanHandlingTest(BitcoinTestFramework): peer2.assert_never_requested(child["tx"].getwtxid()) # The child should never be requested, even if announced again with potentially different witness. + # Sync with ping to ensure orphans are reconsidered peer3.send_and_ping(msg_inv([CInv(t=MSG_TX, h=int(child["txid"], 16))])) self.nodes[0].bumpmocktime(TXREQUEST_TIME_SKIP) peer3.assert_never_requested(child["txid"]) + @cleanup + def test_same_txid_orphan(self): + self.log.info("Check what happens when orphan with same txid is already in orphanage") + node = self.nodes[0] + + tx_parent = self.wallet.create_self_transfer() + + # Create the real child + tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"]) + + # Create a fake version of the child + tx_orphan_bad_wit = self.create_malleated_version(tx_child) + + bad_peer = node.add_p2p_connection(P2PInterface()) + honest_peer = node.add_p2p_connection(P2PInterface()) + + # 1. Fake orphan is received first. It is missing an input. + bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + + # 2. Node requests the missing parent by txid. + parent_txid_int = int(tx_parent["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + bad_peer.wait_for_getdata([parent_txid_int]) + + # 3. Honest peer relays the real child, which is also missing parents and should be placed + # in the orphanage. + with node.assert_debug_log(["missingorspent", "stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + + # Time out the previous request for the parent (node will not request the same transaction + # from multiple nodes at the same time) + node.bumpmocktime(GETDATA_TX_INTERVAL) + + # 4. The parent is requested. Honest peer sends it. + honest_peer.wait_for_getdata([parent_txid_int]) + # Sync with ping to ensure orphans are reconsidered + honest_peer.send_and_ping(msg_tx(tx_parent["tx"])) + + # 5. After parent is accepted, orphans should be reconsidered. + # The real child should be accepted and the fake one rejected. + node_mempool = node.getrawmempool() + assert tx_parent["txid"] in node_mempool + assert tx_child["txid"] in node_mempool + assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) + + @cleanup + def test_same_txid_orphan_of_orphan(self): + self.log.info("Check what happens when orphan's parent with same txid is already in orphanage") + node = self.nodes[0] + + tx_grandparent = self.wallet.create_self_transfer() + + # Create middle tx (both parent and child) which will be in orphanage. + tx_middle = self.wallet.create_self_transfer(utxo_to_spend=tx_grandparent["new_utxo"]) + + # Create a fake version of the middle tx + tx_orphan_bad_wit = self.create_malleated_version(tx_middle) + + # Create grandchild spending from tx_middle (and spending from tx_orphan_bad_wit since they + # have the same txid). + tx_grandchild = self.wallet.create_self_transfer(utxo_to_spend=tx_middle["new_utxo"]) + + bad_peer = node.add_p2p_connection(P2PInterface()) + honest_peer = node.add_p2p_connection(P2PInterface()) + + # 1. Fake orphan is received first. It is missing an input. + bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + + # 2. Node requests missing tx_grandparent by txid. + grandparent_txid_int = int(tx_grandparent["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + bad_peer.wait_for_getdata([grandparent_txid_int]) + + # 3. Honest peer relays the grandchild, which is missing a parent. The parent by txid already + # exists in orphanage, but should be re-requested because the node shouldn't assume that the + # witness data is the same. In this case, a same-txid-different-witness transaction exists! + with node.assert_debug_log(["stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_grandchild["tx"])) + middle_txid_int = int(tx_middle["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + honest_peer.wait_for_getdata([middle_txid_int]) + + # 4. Honest peer relays the real child, which is also missing parents and should be placed + # in the orphanage. + with node.assert_debug_log(["stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_middle["tx"])) + assert_equal(len(node.getrawmempool()), 0) + + # 5. Honest peer sends tx_grandparent + honest_peer.send_and_ping(msg_tx(tx_grandparent["tx"])) + + # 6. After parent is accepted, orphans should be reconsidered. + # The real child should be accepted and the fake one rejected. + node_mempool = node.getrawmempool() + assert tx_grandparent["txid"] in node_mempool + assert tx_middle["txid"] in node_mempool + assert tx_grandchild["txid"] in node_mempool + assert_equal(node.getmempoolentry(tx_middle["txid"])["wtxid"], tx_middle["wtxid"]) + + @cleanup + def test_orphan_txid_inv(self): + self.log.info("Check node does not ignore announcement with same txid as tx in orphanage") + node = self.nodes[0] + + tx_parent = self.wallet.create_self_transfer() + + # Create the real child and fake version + tx_child = self.wallet.create_self_transfer(utxo_to_spend=tx_parent["new_utxo"]) + tx_orphan_bad_wit = self.create_malleated_version(tx_child) + + bad_peer = node.add_p2p_connection(PeerTxRelayer()) + # Must not send wtxidrelay because otherwise the inv(TX) will be ignored later + honest_peer = node.add_p2p_connection(P2PInterface(wtxidrelay=False)) + + # 1. Fake orphan is received first. It is missing an input. + bad_peer.send_and_ping(msg_tx(tx_orphan_bad_wit)) + + # 2. Node requests the missing parent by txid. + parent_txid_int = int(tx_parent["txid"], 16) + node.bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY) + bad_peer.wait_for_getdata([parent_txid_int]) + + # 3. Honest peer announces the real child, by txid (this isn't common but the node should + # still keep track of it). + child_txid_int = int(tx_child["txid"], 16) + honest_peer.send_and_ping(msg_inv([CInv(t=MSG_TX, h=child_txid_int)])) + + # 4. The child is requested. Honest peer sends it. + node.bumpmocktime(TXREQUEST_TIME_SKIP) + honest_peer.wait_for_getdata([child_txid_int]) + with node.assert_debug_log(["stored orphan tx"]): + honest_peer.send_and_ping(msg_tx(tx_child["tx"])) + + # 5. After first parent request times out, the node sends another one for the missing parent + # of the real orphan child. + node.bumpmocktime(GETDATA_TX_INTERVAL) + honest_peer.wait_for_getdata([parent_txid_int]) + honest_peer.send_and_ping(msg_tx(tx_parent["tx"])) + + # 6. After parent is accepted, orphans should be reconsidered. + # The real child should be accepted and the fake one rejected. This may happen in either + # order since the message-processing is randomized. If tx_orphan_bad_wit is validated first, + # its consensus error leads to disconnection of bad_peer. If tx_child is validated first, + # tx_orphan_bad_wit is rejected for txn-same-nonwitness-data-in-mempool (no punishment). + node_mempool = node.getrawmempool() + assert tx_parent["txid"] in node_mempool + assert tx_child["txid"] in node_mempool + assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"]) + + def run_test(self): self.nodes[0].setmocktime(int(time.time())) self.wallet_nonsegwit = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK) @@ -410,6 +579,9 @@ class OrphanHandlingTest(BitcoinTestFramework): self.test_orphans_overlapping_parents() self.test_orphan_of_orphan() self.test_orphan_inherit_rejection() + self.test_same_txid_orphan() + self.test_same_txid_orphan_of_orphan() + self.test_orphan_txid_inv() if __name__ == '__main__': From 0fb17bf61a40b73a2b81a18e70b3de180c917f22 Mon Sep 17 00:00:00 2001 From: glozow Date: Mon, 13 May 2024 14:54:13 +0100 Subject: [PATCH 7/7] [log] updates in TxOrphanage - Add elapsed time in "remove orphan" log - Add size in "stored orphan" log - grammar edit --- src/txorphanage.cpp | 11 +++++++---- test/functional/p2p_invalid_tx.py | 6 +++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 4a1d48a466..692dc0461f 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -47,7 +47,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) m_outpoint_to_orphan_it[txin.prevout].insert(ret.first); } - LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s) (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), + LogPrint(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s), weight: %u (mapsz %u outsz %u)\n", hash.ToString(), wtxid.ToString(), sz, m_orphans.size(), m_outpoint_to_orphan_it.size()); return true; } @@ -84,7 +84,10 @@ int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid) it_last->second.list_pos = old_pos; } const auto& txid = it->second.tx->GetHash(); - LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s)\n", txid.ToString(), wtxid.ToString()); + // Time spent in orphanage = difference between current and entry time. + // Entry time is equal to ORPHAN_TX_EXPIRE_TIME earlier than entry's expiry. + LogPrint(BCLog::TXPACKAGES, " removed orphan tx %s (wtxid=%s) after %ds\n", txid.ToString(), wtxid.ToString(), + GetTime() + ORPHAN_TX_EXPIRE_TIME - it->second.nTimeExpire); m_orphan_list.pop_back(); m_orphans.erase(it); @@ -107,7 +110,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) nErased += EraseTxNoLock(wtxid); } } - if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer); + if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); } void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) @@ -230,7 +233,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) for (const auto& orphanHash : vOrphanErase) { nErased += EraseTxNoLock(orphanHash); } - LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased); + LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) included or conflicted by block\n", nErased); } } diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index ae9dc816ab..0ae05d4b0b 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -165,7 +165,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False) self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool') - with node.assert_debug_log(['Erased 100 orphan tx from peer=25']): + with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=25']): self.reconnect_p2p(num_connections=1) self.log.info('Test that a transaction in the orphan pool is included in a new tip block causes erase this transaction from the orphan pool') @@ -190,7 +190,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): block_A.solve() self.log.info('Send the block that includes the previous orphan ... ') - with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + with node.assert_debug_log(["Erased 1 orphan transaction(s) included or conflicted by block"]): node.p2ps[0].send_blocks_and_test([block_A], node, success=True) self.log.info('Test that a transaction in the orphan pool conflicts with a new tip block causes erase this transaction from the orphan pool') @@ -219,7 +219,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): block_B.solve() self.log.info('Send the block that includes a transaction which conflicts with the previous orphan ... ') - with node.assert_debug_log(["Erased 1 orphan tx included or conflicted by block"]): + with node.assert_debug_log(["Erased 1 orphan transaction(s) included or conflicted by block"]): node.p2ps[0].send_blocks_and_test([block_B], node, success=True)