From a4fe09973aa82210b98dcb4e4e9f11ef59780f9b Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 25 Mar 2021 14:13:45 +1000 Subject: [PATCH 1/5] txorphanage: index workset by originating peer --- src/net_processing.cpp | 4 ++-- src/test/fuzz/txorphan.cpp | 2 +- src/txorphanage.cpp | 8 +++++--- src/txorphanage.h | 6 +++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 346600efd00..5001358c59e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2903,7 +2903,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash()); - m_orphanage.AddChildrenToWorkSet(*porphanTx, peer.m_id); + m_orphanage.AddChildrenToWorkSet(*porphanTx); m_orphanage.EraseTx(orphanHash); for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { AddToCompactExtraTransactions(removedTx); @@ -4030,7 +4030,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); RelayTransaction(tx.GetHash(), tx.GetWitnessHash()); - m_orphanage.AddChildrenToWorkSet(tx, peer->m_id); + m_orphanage.AddChildrenToWorkSet(tx); pfrom.m_last_tx_time = GetTime(); diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 02743051e81..dff29bcd6e5 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -85,7 +85,7 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage) CallOneOf( fuzzed_data_provider, [&] { - orphanage.AddChildrenToWorkSet(*tx, peer_id); + orphanage.AddChildrenToWorkSet(*tx); }, [&] { { diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index b0b71e135c5..2dbc73cbcaf 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -145,17 +145,19 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans) if (nEvicted > 0) LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); } -void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, NodeId peer) +void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) { LOCK(m_mutex); - // Get this peer's work set, emplacing an empty set it didn't exist - std::set& orphan_work_set = m_peer_work_set.try_emplace(peer).first->second; for (unsigned int i = 0; i < tx.vout.size(); i++) { 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); } } diff --git a/src/txorphanage.h b/src/txorphanage.h index 551502d3250..e8767fddc59 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -47,8 +47,8 @@ public: /** Limit the orphanage to the given maximum */ void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); - /** Add any orphans that list a particular tx as a parent into a peer's work set */ - void AddChildrenToWorkSet(const CTransaction& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Add any orphans that list a particular tx as a parent into the from peer's work set */ + void AddChildrenToWorkSet(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);; /** Return how many entries exist in the orphange */ size_t Size() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) @@ -72,7 +72,7 @@ protected: * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ std::map m_orphans GUARDED_BY(m_mutex); - /** Which peer provided a parent tx of orphans that need to be reconsidered */ + /** Which peer provided the orphans that need to be reconsidered */ std::map> m_peer_work_set GUARDED_BY(m_mutex); using OrphanMap = decltype(m_orphans); From be2304676bedcd15debcdc694549fdd2b255ba62 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Tue, 22 Nov 2022 01:39:32 +1000 Subject: [PATCH 2/5] txorphange: Drop redundant originator arg from GetTxToReconsider --- src/net_processing.cpp | 7 +++---- src/test/fuzz/txorphan.cpp | 3 +-- src/txorphanage.cpp | 3 +-- src/txorphanage.h | 6 +++--- 4 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5001358c59e..9b9ef400014 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2892,10 +2892,9 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) AssertLockHeld(cs_main); CTransactionRef porphanTx = nullptr; - NodeId from_peer = -1; bool more = false; - while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id, from_peer, more)) { + while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id, more)) { const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; const uint256& orphanHash = porphanTx->GetHash(); @@ -2913,10 +2912,10 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n", orphanHash.ToString(), - from_peer, + peer.m_id, state.ToString()); // Maybe punish peer that gave us an invalid orphan tx - MaybePunishNodeForTx(from_peer, state); + MaybePunishNodeForTx(peer.m_id, state); } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index dff29bcd6e5..4673b884dcd 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -89,9 +89,8 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage) }, [&] { { - NodeId originator; bool more = true; - CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, originator, more); + CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, more); if (!ref) { Assert(!more); } else { diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 2dbc73cbcaf..31c6ff71062 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -174,7 +174,7 @@ bool TxOrphanage::HaveTx(const GenTxid& gtxid) const } } -CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, NodeId& originator, bool& more) +CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, bool& more) { LOCK(m_mutex); @@ -188,7 +188,6 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, NodeId& originator, const auto orphan_it = m_orphans.find(txid); if (orphan_it != m_orphans.end()) { more = !work_set.empty(); - originator = orphan_it->second.fromPeer; return orphan_it->second.tx; } } diff --git a/src/txorphanage.h b/src/txorphanage.h index e8767fddc59..55c02976dd8 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -29,11 +29,11 @@ public: /** Extract a transaction from a peer's work set * Returns nullptr and sets more to false if there are no transactions * to work on. Otherwise returns the transaction reference, removes - * the transaction from the work set, and populates its arguments with - * the originating peer, and whether there are more orphans for this peer + * the transaction from the work set, and sets "more" to indicate + * if there are more orphans for this peer * to work on after this tx. */ - CTransactionRef GetTxToReconsider(NodeId peer, NodeId& originator, bool& more) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + CTransactionRef GetTxToReconsider(NodeId peer, bool& more) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Erase an orphan by txid */ int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); From c5837757068bf8ea3e5b6fdad82f69d1deb81545 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Thu, 22 Apr 2021 19:24:27 +1000 Subject: [PATCH 3/5] net_processing: only process orphans before messages Previously, when we processed a new tx we would attempt to ATMP any orphans that considered the new tx a parent immediately, but would only accept at most one such tx, leaving any others to be considered on a future run of ProcessMessages(). With this patch, we don't attempt any orphan processing immediately after receiving a tx, instead deferring all of them until the next call to ProcessMessages(). --- src/net_processing.cpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9b9ef400014..25d9e754255 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -591,7 +591,7 @@ private: * @return True if there are still orphans in this peer's work set. */ bool ProcessOrphanTx(Peer& peer) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex); /** Process a single headers message from a peer. * * @param[in] pfrom CNode of the peer @@ -2889,7 +2889,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) { AssertLockHeld(g_msgproc_mutex); - AssertLockHeld(cs_main); + LOCK(cs_main); CTransactionRef porphanTx = nullptr; bool more = false; @@ -4041,9 +4041,6 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { AddToCompactExtraTransactions(removedTx); } - - // Recursively process any orphan transactions that depended on this one - ProcessOrphanTx(*peer); } else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { @@ -4852,11 +4849,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt } } - bool has_more_orphans; - { - LOCK(cs_main); - has_more_orphans = ProcessOrphanTx(*peer); - } + const bool has_more_orphans = ProcessOrphanTx(*peer); if (pfrom->fDisconnect) return false; From ecb0a3e4259b81d6bb74d59a58eb65552c17d8d8 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 25 Jan 2023 18:13:00 +1000 Subject: [PATCH 4/5] net_processing: Don't process tx after processing orphans If we made progress on orphans, consider that enough work for this peer for this round of ProcessMessages. This also allows cleaning up the api for TxOrphange:GetTxToReconsider(). --- src/net_processing.cpp | 26 ++++++++++++++------------ src/test/fuzz/txorphan.cpp | 7 ++----- src/txorphanage.cpp | 4 +--- src/txorphanage.h | 10 ++++------ 4 files changed, 21 insertions(+), 26 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 25d9e754255..13c412842dd 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -584,14 +584,17 @@ private: /** * Reconsider orphan transactions after a parent has been accepted to the mempool. * - * @peer[in] peer The peer whose orphan transactions we will reconsider. Generally only one - * orphan will be reconsidered on each call of this function. This set - * may be added to if accepting an orphan causes its children to be - * reconsidered. - * @return True if there are still orphans in this peer's work set. + * @peer[in] peer The peer whose orphan transactions we will reconsider. Generally only + * one orphan will be reconsidered on each call of this function. If an + * accepted orphan has orphaned children, those will need to be + * reconsidered, creating more work, possibly for other peers. + * @return True if meaningful work was done (an orphan was accepted/rejected). + * If no meaningful work was done, then the work set for this peer + * will be empty. */ bool ProcessOrphanTx(Peer& peer) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex); + /** Process a single headers message from a peer. * * @param[in] pfrom CNode of the peer @@ -2892,9 +2895,8 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) LOCK(cs_main); CTransactionRef porphanTx = nullptr; - bool more = false; - while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id, more)) { + while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(peer.m_id)) { const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; const uint256& orphanHash = porphanTx->GetHash(); @@ -2907,7 +2909,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) { AddToCompactExtraTransactions(removedTx); } - break; + return true; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n", @@ -2950,11 +2952,11 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) } } m_orphanage.EraseTx(orphanHash); - break; + return true; } } - return more; + return false; } bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, @@ -4849,12 +4851,12 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt } } - const bool has_more_orphans = ProcessOrphanTx(*peer); + const bool processed_orphan = ProcessOrphanTx(*peer); if (pfrom->fDisconnect) return false; - if (has_more_orphans) return true; + if (processed_orphan) return true; // this maintains the order of responses // and prevents m_getdata_requests to grow unbounded diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 4673b884dcd..f9d1bb85cb8 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -89,11 +89,8 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage) }, [&] { { - bool more = true; - CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, more); - if (!ref) { - Assert(!more); - } else { + CTransactionRef ref = orphanage.GetTxToReconsider(peer_id); + if (ref) { bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash())); Assert(have_tx); } diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 31c6ff71062..f82cd886f95 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -174,7 +174,7 @@ bool TxOrphanage::HaveTx(const GenTxid& gtxid) const } } -CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, bool& more) +CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) { LOCK(m_mutex); @@ -187,12 +187,10 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer, bool& more) const auto orphan_it = m_orphans.find(txid); if (orphan_it != m_orphans.end()) { - more = !work_set.empty(); return orphan_it->second.tx; } } } - more = false; return nullptr; } diff --git a/src/txorphanage.h b/src/txorphanage.h index 55c02976dd8..f886e07b12c 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -27,13 +27,11 @@ public: bool HaveTx(const GenTxid& gtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Extract a transaction from a peer's work set - * Returns nullptr and sets more to false if there are no transactions - * to work on. Otherwise returns the transaction reference, removes - * the transaction from the work set, and sets "more" to indicate - * if there are more orphans for this peer - * to work on after this tx. + * Returns nullptr if there are no transactions to work on. + * Otherwise returns the transaction reference, and removes + * it from the work set. */ - CTransactionRef GetTxToReconsider(NodeId peer, bool& more) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); /** Erase an orphan by txid */ int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); From c58c249a5b694c88122589fedbef4e2f13f08bb4 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 23 Dec 2022 00:21:10 +1000 Subject: [PATCH 5/5] net_processing: indicate more work to do when orphans are ready to reconsider When PR#15644 made orphan processing interruptible, it also introduced a potential 100ms delay between processing of the first and second newly reconsiderable orphan, because it didn't check if the orphan work set was non-empty after invoking ProcessMessage(). This adds that check, so that ProcessMessages() will return true if there are orphans to process, usually avoiding the 100ms delay in CConnman::ThreadMessageHandler(). --- src/net_processing.cpp | 6 ++++++ src/txorphanage.cpp | 12 ++++++++++++ src/txorphanage.h | 3 +++ 3 files changed, 21 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 13c412842dd..368e81944d2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4902,6 +4902,12 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt LOCK(peer->m_getdata_requests_mutex); if (!peer->m_getdata_requests.empty()) fMoreWork = true; } + // Does this peer has an orphan ready to reconsider? + // (Note: we may have provided a parent for an orphan provided + // by another peer that was already processed; in that case, + // the extra work may not be noticed, possibly resulting in an + // unnecessary 100ms delay) + if (m_orphanage.HaveTxToReconsider(peer->m_id)) fMoreWork = true; } catch (const std::exception& e) { LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(msg.m_type), msg.m_message_size, e.what(), typeid(e).name()); } catch (...) { diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index f82cd886f95..eb74d3451e4 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -194,6 +194,18 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) return nullptr; } +bool TxOrphanage::HaveTxToReconsider(NodeId peer) +{ + LOCK(m_mutex); + + auto work_set_it = m_peer_work_set.find(peer); + if (work_set_it != m_peer_work_set.end()) { + auto& work_set = work_set_it->second; + return !work_set.empty(); + } + return false; +} + void TxOrphanage::EraseForBlock(const CBlock& block) { LOCK(m_mutex); diff --git a/src/txorphanage.h b/src/txorphanage.h index f886e07b12c..08cfd4e79c0 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -48,6 +48,9 @@ public: /** Add any orphans that list a particular tx as a parent into the from peer's work set */ void AddChildrenToWorkSet(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);; + /** Does this peer have any work to do? */ + bool HaveTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);; + /** Return how many entries exist in the orphange */ size_t Size() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) {