From e810842acda6fe56e0536ebecfbb9d17d26e1513 Mon Sep 17 00:00:00 2001 From: glozow Date: Tue, 18 Apr 2023 16:02:58 +0100 Subject: [PATCH] [txorphanage] support multiple announcers Add ability to add and track multiple announcers per orphan transaction, erasing announcers but not the entire orphan. The tx creation code in orphanage_tests needs to be updated so that each tx is unique, because the CountOrphans() check assumes that calling EraseForPeer necessarily means its orphans are deleted. Unused for now. --- src/rpc/mempool.cpp | 4 +- src/test/orphanage_tests.cpp | 4 +- src/txorphanage.cpp | 71 +++++++++++++++++++++++++++--------- src/txorphanage.h | 12 +++++- 4 files changed, 68 insertions(+), 23 deletions(-) diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 6d462a7e851..5239886fafd 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -843,7 +843,9 @@ static UniValue OrphanToJSON(const TxOrphanage::OrphanTxBase& orphan) o.pushKV("entry", int64_t{TicksSinceEpoch(orphan.nTimeExpire - ORPHAN_TX_EXPIRE_TIME)}); o.pushKV("expiration", int64_t{TicksSinceEpoch(orphan.nTimeExpire)}); UniValue from(UniValue::VARR); - from.push_back(orphan.fromPeer); // only one fromPeer for now + for (const auto fromPeer: orphan.announcers) { + from.push_back(fromPeer); + } o.pushKV("from", from); return o; } diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 799f2c0fec4..07e7002cadc 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -132,7 +132,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vin[0].prevout.hash = Txid::FromUint256(m_rng.rand256()); tx.vin[0].scriptSig << OP_1; tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; + tx.vout[0].nValue = i*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); orphanage.AddTx(MakeTransactionRef(tx), i); @@ -148,7 +148,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) tx.vin[0].prevout.n = 0; tx.vin[0].prevout.hash = txPrev->GetHash(); tx.vout.resize(1); - tx.vout[0].nValue = 1*CENT; + tx.vout[0].nValue = i*CENT; tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); SignatureData empty; BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL, empty)); diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index ba4ba6c3b62..a108ce94370 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -16,8 +16,11 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) { const Txid& hash = tx->GetHash(); const Wtxid& wtxid = tx->GetWitnessHash(); - if (m_orphans.count(wtxid)) + if (auto it{m_orphans.find(wtxid)}; it != m_orphans.end()) { + AddAnnouncer(wtxid, peer); + // No new orphan entry was created. An announcer may have been added. return false; + } // Ignore big transactions, to avoid a // send-big-orphans memory exhaustion attack. If a peer has a legitimate @@ -33,7 +36,7 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return false; } - auto ret = m_orphans.emplace(wtxid, OrphanTx{{tx, peer, Now() + ORPHAN_TX_EXPIRE_TIME}, m_orphan_list.size()}); + auto ret = m_orphans.emplace(wtxid, OrphanTx{{tx, {peer}, Now() + ORPHAN_TX_EXPIRE_TIME}, m_orphan_list.size()}); assert(ret.second); m_orphan_list.push_back(ret.first); for (const CTxIn& txin : tx->vin) { @@ -45,6 +48,20 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) return true; } +bool TxOrphanage::AddAnnouncer(const Wtxid& wtxid, NodeId peer) +{ + const auto it = m_orphans.find(wtxid); + if (it != m_orphans.end()) { + Assume(!it->second.announcers.empty()); + const auto ret = it->second.announcers.insert(peer); + if (ret.second) { + LogDebug(BCLog::TXPACKAGES, "added peer=%d as announcer of orphan tx %s\n", peer, wtxid.ToString()); + return true; + } + } + return false; +} + int TxOrphanage::EraseTx(const Wtxid& wtxid) { std::map::iterator it = m_orphans.find(wtxid); @@ -89,9 +106,15 @@ void TxOrphanage::EraseForPeer(NodeId peer) while (iter != m_orphans.end()) { // increment to avoid iterator becoming invalid after erasure - const auto& [wtxid, orphan] = *iter++; - if (orphan.fromPeer == peer) { - nErased += EraseTx(wtxid); + auto& [wtxid, orphan] = *iter++; + auto orphan_it = orphan.announcers.find(peer); + if (orphan_it != orphan.announcers.end()) { + orphan.announcers.erase(peer); + + // No remaining annnouncers: clean up entry + if (orphan.announcers.empty()) { + nErased += EraseTx(orphan.tx->GetWitnessHash()); + } } } if (nErased > 0) LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); @@ -110,7 +133,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { std::map::iterator maybeErase = iter++; if (maybeErase->second.nTimeExpire <= nNow) { - nErased += EraseTx(maybeErase->second.tx->GetWitnessHash()); + nErased += EraseTx(maybeErase->first); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -123,7 +146,7 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { // Evict a random orphan: size_t randompos = rng.randrange(m_orphan_list.size()); - EraseTx(m_orphan_list[randompos]->second.tx->GetWitnessHash()); + EraseTx(m_orphan_list[randompos]->first); ++nEvicted; } if (nEvicted > 0) LogDebug(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); @@ -135,13 +158,17 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i)); if (it_by_prev != m_outpoint_to_orphan_it.end()) { 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; - // Add this tx to the work set - orphan_work_set.insert(elem->first); - LogDebug(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", - tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), elem->second.fromPeer); + // Belt and suspenders, each orphan should always have at least 1 announcer. + if (!Assume(!elem->second.announcers.empty())) continue; + for (const auto announcer: elem->second.announcers) { + // 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(announcer).first->second; + // Add this tx to the work set + orphan_work_set.insert(elem->first); + LogDebug(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", + tx.GetHash().ToString(), tx.GetWitnessHash().ToString(), announcer); + } } } } @@ -152,6 +179,12 @@ bool TxOrphanage::HaveTx(const Wtxid& wtxid) const return m_orphans.count(wtxid); } +bool TxOrphanage::HaveTxFromPeer(const Wtxid& wtxid, NodeId peer) const +{ + auto it = m_orphans.find(wtxid); + return (it != m_orphans.end() && it->second.announcers.contains(peer)); +} + CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) { auto work_set_it = m_peer_work_set.find(peer); @@ -219,7 +252,7 @@ std::vector TxOrphanage::GetChildrenFromSamePeer(const CTransac const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i)); if (it_by_prev != m_outpoint_to_orphan_it.end()) { for (const auto& elem : it_by_prev->second) { - if (elem->second.fromPeer == nodeid) { + if (elem->second.announcers.contains(nodeid)) { iters.emplace_back(elem); } } @@ -258,7 +291,7 @@ std::vector> TxOrphanage::GetChildrenFromDiff const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i)); if (it_by_prev != m_outpoint_to_orphan_it.end()) { for (const auto& elem : it_by_prev->second) { - if (elem->second.fromPeer != nodeid) { + if (!elem->second.announcers.contains(nodeid)) { iters.emplace_back(elem); } } @@ -273,7 +306,9 @@ std::vector> TxOrphanage::GetChildrenFromDiff std::vector> children_found; children_found.reserve(iters.size()); for (const auto& child_iter : iters) { - children_found.emplace_back(child_iter->second.tx, child_iter->second.fromPeer); + // Use first peer in announcers list + auto peer = *(child_iter->second.announcers.begin()); + children_found.emplace_back(child_iter->second.tx, peer); } return children_found; } @@ -283,7 +318,7 @@ std::vector TxOrphanage::GetOrphanTransactions() cons std::vector ret; ret.reserve(m_orphans.size()); for (auto const& o : m_orphans) { - ret.push_back({o.second.tx, o.second.fromPeer, o.second.nTimeExpire}); + ret.push_back({o.second.tx, o.second.announcers, o.second.nTimeExpire}); } return ret; } diff --git a/src/txorphanage.h b/src/txorphanage.h index b1fae3fa008..76ff8313a5a 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -30,9 +30,15 @@ public: /** Add a new orphan transaction */ bool AddTx(const CTransactionRef& tx, NodeId peer); + /** Add an additional announcer to an orphan if it exists. Otherwise, do nothing. */ + bool AddAnnouncer(const Wtxid& wtxid, NodeId peer); + /** Check if we already have an orphan transaction (by wtxid only) */ bool HaveTx(const Wtxid& wtxid) const; + /** Check if a {tx, peer} exists in the orphanage.*/ + bool HaveTxFromPeer(const Wtxid& wtxid, NodeId peer) const; + /** Extract a transaction from a peer's work set * Returns nullptr if there are no transactions to work on. * Otherwise returns the transaction reference, and removes @@ -43,7 +49,8 @@ public: /** Erase an orphan by wtxid */ int EraseTx(const Wtxid& wtxid); - /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ + /** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan + * has been announced by another peer, don't erase, just remove this peer from the list of announcers. */ void EraseForPeer(NodeId peer); /** Erase all orphans included in or invalidated by a new block */ @@ -75,7 +82,8 @@ public: /** Allows providing orphan information externally */ struct OrphanTxBase { CTransactionRef tx; - NodeId fromPeer; + /** Peers added with AddTx or AddAnnouncer. */ + std::set announcers; NodeSeconds nTimeExpire; };