From 3ddce9aaab8519e0fdebbe53352d634e0271a76d Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sat, 6 Apr 2024 16:02:31 +0000 Subject: [PATCH] llmq: pass PeerManager to llmq::CInstantSendManager constructor Required to avoid crashes when calling RelayInvFiltered in situations where the PeerManager* atomic hasn't been set (possible when ProcessMessage isn't called, leaving the value unset, while a separate thread traverses the ProcessPendingInstantSendLocks > ProcessPendingInstantSendLocks[1] > ProcessInstantSendLock > RelayInvFiltered call chain). [1] - There is a function with the exact same name but with multiple arguments --- src/llmq/context.cpp | 2 +- src/llmq/instantsend.cpp | 18 ++++++------------ src/llmq/instantsend.h | 10 +++++----- src/net_processing.cpp | 2 +- 4 files changed, 13 insertions(+), 19 deletions(-) diff --git a/src/llmq/context.cpp b/src/llmq/context.cpp index eb8b36c0c1e7fd..5f48356faff3ad 100644 --- a/src/llmq/context.cpp +++ b/src/llmq/context.cpp @@ -44,7 +44,7 @@ LLMQContext::LLMQContext(CChainState& chainstate, CConnman& connman, CDeterminis }()}, isman{[&]() -> llmq::CInstantSendManager* const { assert(llmq::quorumInstantSendManager == nullptr); - llmq::quorumInstantSendManager = std::make_unique(*llmq::chainLocksHandler, chainstate, connman, *llmq::quorumManager, *sigman, *shareman, sporkman, mempool, *::masternodeSync, unit_tests, wipe); + llmq::quorumInstantSendManager = std::make_unique(*llmq::chainLocksHandler, chainstate, connman, *llmq::quorumManager, *sigman, *shareman, sporkman, mempool, peerman, *::masternodeSync, unit_tests, wipe); return llmq::quorumInstantSendManager.get(); }()}, ehfSignalsHandler{std::make_unique(chainstate, mnhfman, *sigman, *shareman, peerman, sporkman, *llmq::quorumManager, mempool)} diff --git a/src/llmq/instantsend.cpp b/src/llmq/instantsend.cpp index 8278b156739e13..49a6ec860c68df 100644 --- a/src/llmq/instantsend.cpp +++ b/src/llmq/instantsend.cpp @@ -747,15 +747,9 @@ void CInstantSendManager::HandleNewInstantSendLockRecoveredSig(const llmq::CReco pendingInstantSendLocks.emplace(hash, std::make_pair(-1, islock)); } -PeerMsgRet CInstantSendManager::ProcessMessage(const CNode& pfrom, gsl::not_null peerman, std::string_view msg_type, CDataStream& vRecv) +PeerMsgRet CInstantSendManager::ProcessMessage(const CNode& pfrom, std::string_view msg_type, CDataStream& vRecv) { if (IsInstantSendEnabled() && msg_type == NetMsgType::ISDLOCK) { - if (m_peerman == nullptr) { - m_peerman = peerman; - } - // we should never use one CInstantSendManager with different PeerManager - assert(m_peerman == peerman); - const auto islock = std::make_shared(); vRecv >> *islock; return ProcessMessageInstantSendLock(pfrom, islock); @@ -957,7 +951,7 @@ std::unordered_set CInstantSendManager::ProcessPend for (const auto& nodeId : batchVerifier.badSources) { // Let's not be too harsh, as the peer might simply be unlucky and might have sent us an old lock which // does not validate anymore due to changed quorums - m_peerman.load()->Misbehaving(nodeId, 20); + m_peerman.Misbehaving(nodeId, 20); } } for (const auto& p : pend) { @@ -1051,11 +1045,11 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has CInv inv(MSG_ISDLOCK, hash); if (tx != nullptr) { - m_peerman.load()->RelayInvFiltered(inv, *tx, ISDLOCK_PROTO_VERSION); + m_peerman.RelayInvFiltered(inv, *tx, ISDLOCK_PROTO_VERSION); } else { // we don't have the TX yet, so we only filter based on txid. Later when that TX arrives, we will re-announce // with the TX taken into account. - m_peerman.load()->RelayInvFiltered(inv, islock->txid, ISDLOCK_PROTO_VERSION); + m_peerman.RelayInvFiltered(inv, islock->txid, ISDLOCK_PROTO_VERSION); } ResolveBlockConflicts(hash, *islock); @@ -1068,7 +1062,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has // bump mempool counter to make sure newly locked txes are picked up by getblocktemplate mempool.AddTransactionsUpdated(1); } else { - AskNodesForLockedTx(islock->txid, connman, *m_peerman.load()); + AskNodesForLockedTx(islock->txid, connman, m_peerman); } } @@ -1344,7 +1338,7 @@ void CInstantSendManager::RemoveMempoolConflictsForLock(const uint256& hash, con for (const auto& p : toDelete) { RemoveConflictedTx(*p.second); } - AskNodesForLockedTx(islock.txid, connman, *m_peerman.load()); + AskNodesForLockedTx(islock.txid, connman, m_peerman); } } diff --git a/src/llmq/instantsend.h b/src/llmq/instantsend.h index cd141b65b58670..e330290dca552a 100644 --- a/src/llmq/instantsend.h +++ b/src/llmq/instantsend.h @@ -205,8 +205,8 @@ class CInstantSendManager : public CRecoveredSigsListener CSigSharesManager& shareman; CSporkManager& spork_manager; CTxMemPool& mempool; + PeerManager& m_peerman; const CMasternodeSync& m_mn_sync; - std::atomic m_peerman{nullptr}; std::atomic fUpgradedDB{false}; @@ -256,11 +256,11 @@ class CInstantSendManager : public CRecoveredSigsListener public: explicit CInstantSendManager(CChainLocksHandler& _clhandler, CChainState& chainstate, CConnman& _connman, CQuorumManager& _qman, CSigningManager& _sigman, CSigSharesManager& _shareman, - CSporkManager& sporkman, CTxMemPool& _mempool, const CMasternodeSync& mn_sync, - bool unitTests, bool fWipe) : + CSporkManager& sporkman, CTxMemPool& _mempool, PeerManager& peerman, + const CMasternodeSync& mn_sync, bool unitTests, bool fWipe) : db(unitTests, fWipe), clhandler(_clhandler), m_chainstate(chainstate), connman(_connman), qman(_qman), sigman(_sigman), - shareman(_shareman), spork_manager(sporkman), mempool(_mempool), m_mn_sync(mn_sync) + shareman(_shareman), spork_manager(sporkman), mempool(_mempool), m_peerman(peerman), m_mn_sync(mn_sync) { workInterrupt.reset(); } @@ -313,7 +313,7 @@ class CInstantSendManager : public CRecoveredSigsListener void HandleNewRecoveredSig(const CRecoveredSig& recoveredSig) override LOCKS_EXCLUDED(cs_inputReqests, cs_creating); - PeerMsgRet ProcessMessage(const CNode& pfrom, gsl::not_null peerman, std::string_view msg_type, CDataStream& vRecv); + PeerMsgRet ProcessMessage(const CNode& pfrom, std::string_view msg_type, CDataStream& vRecv); void TransactionAddedToMempool(const CTransactionRef& tx) LOCKS_EXCLUDED(cs_pendingLocks); void TransactionRemovedFromMempool(const CTransactionRef& tx); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b23c11493d889f..2ce471c7d92241 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -4671,7 +4671,7 @@ void PeerManagerImpl::ProcessMessage( m_llmq_ctx->shareman->ProcessMessage(pfrom, m_sporkman, msg_type, vRecv); ProcessPeerMsgRet(m_llmq_ctx->sigman->ProcessMessage(pfrom, this, msg_type, vRecv), pfrom); ProcessPeerMsgRet(m_llmq_ctx->clhandler->ProcessMessage(pfrom, msg_type, vRecv), pfrom); - ProcessPeerMsgRet(m_llmq_ctx->isman->ProcessMessage(pfrom, this, msg_type, vRecv), pfrom); + ProcessPeerMsgRet(m_llmq_ctx->isman->ProcessMessage(pfrom, msg_type, vRecv), pfrom); return; }