From 6e8dd99ef1c147898bd06fee7014afdff6618f18 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 25 Jun 2020 15:34:56 -0400 Subject: [PATCH 1/6] [net processing] Add doxygen comments for orphan data and function --- src/net_processing.cpp | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 05f51882663..c932062a285 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -153,8 +153,14 @@ struct COrphanTx { int64_t nTimeExpire; size_t list_pos; }; + +/** Guards orphan transactions and extra txs for compact blocks */ RecursiveMutex g_cs_orphans; +/** Map from txid to orphan transaction record. Limited by + * -maxorphantx/DEFAULT_MAX_ORPHAN_TRANSACTIONS */ std::map mapOrphanTransactions GUARDED_BY(g_cs_orphans); +/** Index from wtxid into the mapOrphanTransactions to lookup orphan + * transactions using their witness ids. */ std::map::iterator> g_orphans_by_wtxid GUARDED_BY(g_cs_orphans); void EraseOrphansFor(NodeId peer); @@ -258,12 +264,19 @@ namespace { return &(*a) < &(*b); } }; + + /** Index from the parents' COutPoint into the mapOrphanTransactions. Used + * to remove orphan transactions from the mapOrphanTransactions */ std::map::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans); + /** Orphan transactions in vector for quick random eviction */ + std::vector::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); - std::vector::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); //! For random eviction - - static size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0; + /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction. + * The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of + * these are kept in a ring buffer */ static std::vector> vExtraTxnForCompact GUARDED_BY(g_cs_orphans); + /** Offset into vExtraTxnForCompact to insert the next tx */ + static size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0; } // namespace namespace { @@ -2021,6 +2034,16 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector& orphan_work_set, std::list& removed_txn) { AssertLockHeld(cs_main); From 55c79a9cefb6c83cdebbf6c538c471607695b457 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 25 Jun 2020 15:37:51 -0400 Subject: [PATCH 2/6] ProcessOrphanTx: remove useless done variable There is a keyword that allows us to break out of loops. Use it. There's a small change in behaviour here: if we process multiple orphans that are still orphans, then we'll only call mempool.check() once at the end, instead of after processing each tx. --- src/net_processing.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c932062a285..45cb29065eb 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2049,8 +2049,8 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< AssertLockHeld(cs_main); AssertLockHeld(g_cs_orphans); std::set setMisbehaving; - bool done = false; - while (!done && !orphan_work_set.empty()) { + + while (!orphan_work_set.empty()) { const uint256 orphanHash = *orphan_work_set.begin(); orphan_work_set.erase(orphan_work_set.begin()); @@ -2078,7 +2078,7 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< } } EraseOrphanTx(orphanHash); - done = true; + break; } else if (orphan_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (orphan_state.IsInvalid()) { // Punish peer that gave us an invalid orphan tx @@ -2124,10 +2124,10 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< } } EraseOrphanTx(orphanHash); - done = true; + break; } - m_mempool.check(&::ChainstateActive().CoinsTip()); } + m_mempool.check(&::ChainstateActive().CoinsTip()); } /** From 4763b51bca86fb9e49175619a47cdbef34feaf99 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 25 Jun 2020 15:39:32 -0400 Subject: [PATCH 3/6] ProcessOrphanTx: remove useless setMisbehaving set This starts empty, and is only added to if we're about to exit the function (so we never read from it). --- src/net_processing.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 45cb29065eb..40783e210ce 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2048,7 +2048,6 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< { AssertLockHeld(cs_main); AssertLockHeld(g_cs_orphans); - std::set setMisbehaving; while (!orphan_work_set.empty()) { const uint256 orphanHash = *orphan_work_set.begin(); @@ -2065,7 +2064,6 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< // that relayed the previous transaction). TxValidationState orphan_state; - if (setMisbehaving.count(fromPeer)) continue; if (AcceptToMemoryPool(m_mempool, orphan_state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman); @@ -2081,14 +2079,12 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< break; } else if (orphan_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (orphan_state.IsInvalid()) { - // Punish peer that gave us an invalid orphan tx - if (MaybePunishNodeForTx(fromPeer, orphan_state)) { - setMisbehaving.insert(fromPeer); - } LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n", orphanHash.ToString(), fromPeer, orphan_state.ToString()); + // Maybe punish peer that gave us an invalid orphan tx + MaybePunishNodeForTx(fromPeer, orphan_state); } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee From e07c5d94231cefb748f9534ab8ff0b3e2b04c4d8 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 25 Jun 2020 15:47:54 -0400 Subject: [PATCH 4/6] ProcessOrphanTx: Remove outdated commented Also rename orphan_state to state. Both the comment and the variable name are leftover from when this logic was part of ProcessMessage(). --- src/net_processing.cpp | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 40783e210ce..e28775d84c4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2059,12 +2059,9 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< const CTransactionRef porphanTx = orphan_it->second.tx; const CTransaction& orphanTx = *porphanTx; NodeId fromPeer = orphan_it->second.fromPeer; - // Use a new TxValidationState because orphans come from different peers (and we call - // MaybePunishNodeForTx based on the source peer from the orphan map, not based on the peer - // that relayed the previous transaction). - TxValidationState orphan_state; + TxValidationState state; - if (AcceptToMemoryPool(m_mempool, orphan_state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { + if (AcceptToMemoryPool(m_mempool, state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman); for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { @@ -2077,19 +2074,19 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< } EraseOrphanTx(orphanHash); break; - } else if (orphan_state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { - if (orphan_state.IsInvalid()) { + } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { + if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n", orphanHash.ToString(), fromPeer, - orphan_state.ToString()); + state.ToString()); // Maybe punish peer that gave us an invalid orphan tx - MaybePunishNodeForTx(fromPeer, orphan_state); + MaybePunishNodeForTx(fromPeer, state); } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); - if (orphan_state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { + if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) { // We can add the wtxid of this transaction to our reject filter. // Do not add txids of witness transactions or witness-stripped // transactions to the filter, as they can have been malleated; @@ -2113,7 +2110,7 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< // processing of this transaction in the event that child // transactions are later received (resulting in // parent-fetching by txid via the orphan-handling logic). - if (orphan_state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) { + if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) { // We only add the txid if it differs from the wtxid, to // avoid wasting entries in the rolling bloom filter. recentRejects->insert(orphanTx.GetHash()); From 4fce726bd1e35a686cd9d48add5da22b1b5e25e1 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 25 Jun 2020 16:15:14 -0400 Subject: [PATCH 5/6] ProcessOrphanTx: Remove aliases --- src/net_processing.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e28775d84c4..5c3b71faa2c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2057,14 +2057,12 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< if (orphan_it == mapOrphanTransactions.end()) continue; const CTransactionRef porphanTx = orphan_it->second.tx; - const CTransaction& orphanTx = *porphanTx; - NodeId fromPeer = orphan_it->second.fromPeer; TxValidationState state; if (AcceptToMemoryPool(m_mempool, state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanHash, porphanTx->GetWitnessHash(), m_connman); - for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { + for (unsigned int i = 0; i < porphanTx->vout.size(); i++) { auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(orphanHash, i)); if (it_by_prev != mapOrphanTransactionsByPrev.end()) { for (const auto& elem : it_by_prev->second) { @@ -2078,10 +2076,10 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s from peer=%d. %s\n", orphanHash.ToString(), - fromPeer, + orphan_it->second.fromPeer, state.ToString()); // Maybe punish peer that gave us an invalid orphan tx - MaybePunishNodeForTx(fromPeer, state); + MaybePunishNodeForTx(orphan_it->second.fromPeer, state); } // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee @@ -2101,7 +2099,7 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< // for concerns around weakening security of unupgraded nodes // if we start doing this too early. assert(recentRejects); - recentRejects->insert(orphanTx.GetWitnessHash()); + recentRejects->insert(porphanTx->GetWitnessHash()); // If the transaction failed for TX_INPUTS_NOT_STANDARD, // then we know that the witness was irrelevant to the policy // failure, since this check depends only on the txid @@ -2110,10 +2108,10 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< // processing of this transaction in the event that child // transactions are later received (resulting in // parent-fetching by txid via the orphan-handling logic). - if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && orphanTx.GetWitnessHash() != orphanTx.GetHash()) { + if (state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD && porphanTx->GetWitnessHash() != porphanTx->GetHash()) { // We only add the txid if it differs from the wtxid, to // avoid wasting entries in the rolling bloom filter. - recentRejects->insert(orphanTx.GetHash()); + recentRejects->insert(porphanTx->GetHash()); } } EraseOrphanTx(orphanHash); From 001343f4bc8b22fa9e313bd2867756eb9d614fa3 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 25 Jun 2020 17:26:55 -0400 Subject: [PATCH 6/6] ProcessOrphanTx: Move AddToCompactExtraTransactions call into ProcessOrphanTx --- src/net_processing.cpp | 23 +++++++++++------------ src/net_processing.h | 3 +-- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5c3b71faa2c..c82574c4f64 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2041,10 +2041,8 @@ void PeerManager::ProcessHeadersMessage(CNode& pfrom, const std::vector& orphan_work_set, std::list& removed_txn) +void PeerManager::ProcessOrphanTx(std::set& orphan_work_set) { AssertLockHeld(cs_main); AssertLockHeld(g_cs_orphans); @@ -2058,6 +2056,7 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< const CTransactionRef porphanTx = orphan_it->second.tx; TxValidationState state; + std::list removed_txn; if (AcceptToMemoryPool(m_mempool, state, porphanTx, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); @@ -2071,6 +2070,9 @@ void PeerManager::ProcessOrphanTx(std::set& orphan_work_set, std::list< } } EraseOrphanTx(orphanHash); + for (const CTransactionRef& removedTx : removed_txn) { + AddToCompactExtraTransactions(removedTx); + } break; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { if (state.IsInvalid()) { @@ -3034,8 +3036,12 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat tx.GetHash().ToString(), m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000); + for (const CTransactionRef& removedTx : lRemovedTxn) { + AddToCompactExtraTransactions(removedTx); + } + // Recursively process any orphan transactions that depended on this one - ProcessOrphanTx(pfrom.orphan_work_set, lRemovedTxn); + ProcessOrphanTx(pfrom.orphan_work_set); } else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { @@ -3138,9 +3144,6 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat } } - for (const CTransactionRef& removedTx : lRemovedTxn) - AddToCompactExtraTransactions(removedTx); - // If a tx has been detected by recentRejects, we will have reached // this point and the tx will have been ignored. Because we haven't run // the tx through AcceptToMemoryPool, we won't have computed a DoS @@ -3853,12 +3856,8 @@ bool PeerManager::ProcessMessages(CNode* pfrom, std::atomic& interruptMsgP ProcessGetData(*pfrom, m_chainparams, m_connman, m_mempool, interruptMsgProc); if (!pfrom->orphan_work_set.empty()) { - std::list removed_txn; LOCK2(cs_main, g_cs_orphans); - ProcessOrphanTx(pfrom->orphan_work_set, removed_txn); - for (const CTransactionRef& removedTx : removed_txn) { - AddToCompactExtraTransactions(removedTx); - } + ProcessOrphanTx(pfrom->orphan_work_set); } if (pfrom->fDisconnect) diff --git a/src/net_processing.h b/src/net_processing.h index 520f7489e8f..f7afddda23a 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -121,8 +121,7 @@ private: */ bool MaybeDiscourageAndDisconnect(CNode& pnode); - void ProcessOrphanTx(std::set& orphan_work_set, std::list& removed_txn) - EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans); + void ProcessOrphanTx(std::set& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans); /** Process a single headers message from a peer. */ void ProcessHeadersMessage(CNode& pfrom, const std::vector& headers, bool via_compact_block);