diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 6374cb52c1873..a60c759997926 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -492,10 +492,12 @@ class PeerManagerImpl final : public PeerManager CTxMemPool& pool, Options opts); /** Overridden from CValidationInterface. */ + void UpdatedBlockTipSync(const CBlockIndex* pindexNew) override + EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); void BlockConnected(ChainstateRole role, const std::shared_ptr& pblock, const CBlockIndex* pindexConnected) override - EXCLUSIVE_LOCKS_REQUIRED(!m_recent_confirmed_transactions_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); void BlockDisconnected(const std::shared_ptr &block, const CBlockIndex* pindex) override - EXCLUSIVE_LOCKS_REQUIRED(!m_recent_confirmed_transactions_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void BlockChecked(const CBlock& block, const BlockValidationState& state) override @@ -504,13 +506,13 @@ class PeerManagerImpl final : public PeerManager EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex); /** Implement NetEventsInterface */ - void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex); + void InitializeNode(CNode& node, ServiceFlags our_services) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_tx_download_mutex); + void FinalizeNode(const CNode& node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex, !m_tx_download_mutex); bool HasAllDesirableServiceFlags(ServiceFlags services) const override; bool ProcessMessages(CNode* pfrom, std::atomic& interrupt) override - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex, !m_tx_download_mutex); bool SendMessages(CNode* pto) override - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, g_msgproc_mutex, !m_tx_download_mutex); /** Implement PeerManager */ void StartScheduledTasks(CScheduler& scheduler) override; @@ -529,7 +531,7 @@ class PeerManagerImpl final : public PeerManager void UnitTestMisbehaving(NodeId peer_id, int howmuch) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), howmuch, ""); }; void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv, const std::chrono::microseconds time_received, const std::atomic& interruptMsgProc) override - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_recent_confirmed_transactions_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_most_recent_block_mutex, !m_headers_presync_mutex, g_msgproc_mutex, !m_tx_download_mutex); void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) override; ServiceFlags GetDesirableServiceFlags(ServiceFlags services) const override; @@ -594,12 +596,12 @@ class PeerManagerImpl final : public PeerManager * Updates m_txrequest, m_recent_rejects, m_recent_rejects_reconsiderable, m_orphanage, and vExtraTxnForCompact. */ void ProcessInvalidTx(NodeId nodeid, const CTransactionRef& tx, const TxValidationState& result, bool maybe_add_extra_compact_tx) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); /** Handle a transaction whose result was MempoolAcceptResult::ResultType::VALID. * Updates m_txrequest, m_orphanage, and vExtraTxnForCompact. Also queues the tx for relay. */ void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list& replaced_transactions) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); struct PackageToValidate { const Package m_txns; @@ -629,13 +631,13 @@ class PeerManagerImpl final : public PeerManager * individual transactions, and caches rejection for the package as a group. */ void ProcessPackageResult(const PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package, * skipping any combinations that have already been tried. Return the resulting package along with * the senders of its respective transactions, or std::nullopt if no package is found. */ std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, m_tx_download_mutex); /** * Reconsider orphan transactions after a parent has been accepted to the mempool. @@ -649,7 +651,7 @@ class PeerManagerImpl final : public PeerManager * will be empty. */ bool ProcessOrphanTx(Peer& peer) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex); + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, !m_tx_download_mutex); /** Process a single headers message from a peer. * @@ -731,7 +733,7 @@ class PeerManagerImpl final : public PeerManager * peer. The announcement parameters are decided in PeerManager and then * passed to TxRequestTracker. */ void AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_tx_download_mutex); /** Send a message to a peer */ void PushMessage(CNode& node, CSerializedNetMsg&& msg) const { m_connman.PushMessage(&node, std::move(msg)); } @@ -779,7 +781,10 @@ class PeerManagerImpl final : public PeerManager BanMan* const m_banman; ChainstateManager& m_chainman; CTxMemPool& m_mempool; - TxRequestTracker m_txrequest GUARDED_BY(::cs_main); + + /** Protects tx download including TxRequest, rejection filter. */ + Mutex m_tx_download_mutex; + TxRequestTracker m_txrequest GUARDED_BY(m_tx_download_mutex); std::unique_ptr m_txreconciliation; /** The height of the best chain */ @@ -855,11 +860,9 @@ class PeerManagerImpl final : public PeerManager * - m_recent_rejects * - m_recent_rejects_reconsiderable (if include_reconsiderable = true) * - m_recent_confirmed_transactions - * Also responsible for resetting m_recent_rejects and m_recent_rejects_reconsiderable if the - * chain tip has changed. * */ bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) - EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_recent_confirmed_transactions_mutex); + EXCLUSIVE_LOCKS_REQUIRED(m_tx_download_mutex); /** * Filter for transactions that were recently rejected by the mempool. @@ -895,10 +898,7 @@ class PeerManagerImpl final : public PeerManager * * Memory used: 1.3 MB */ - CRollingBloomFilter m_recent_rejects GUARDED_BY(::cs_main){120'000, 0.000'001}; - /** Block hash of chain tip the last time we reset m_recent_rejects and - * m_recent_rejects_reconsiderable. */ - uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main); + CRollingBloomFilter m_recent_rejects GUARDED_BY(m_tx_download_mutex){120'000, 0.000'001}; /** * Filter for: @@ -920,7 +920,7 @@ class PeerManagerImpl final : public PeerManager * * Parameters are picked to be the same as m_recent_rejects, with the same rationale. */ - CRollingBloomFilter m_recent_rejects_reconsiderable GUARDED_BY(::cs_main){120'000, 0.000'001}; + CRollingBloomFilter m_recent_rejects_reconsiderable GUARDED_BY(m_tx_download_mutex){120'000, 0.000'001}; /* * Filter for transactions that have been recently confirmed. @@ -937,8 +937,7 @@ class PeerManagerImpl final : public PeerManager * transaction per day that would be inadvertently ignored (which is the * same probability that we have in the reject filter). */ - Mutex m_recent_confirmed_transactions_mutex; - CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_recent_confirmed_transactions_mutex){48'000, 0.000'001}; + CRollingBloomFilter m_recent_confirmed_transactions GUARDED_BY(m_tx_download_mutex){48'000, 0.000'001}; /** * For sending `inv`s to inbound peers, we use a single (exponentially @@ -1075,7 +1074,7 @@ class PeerManagerImpl final : public PeerManager int m_peers_downloading_from GUARDED_BY(cs_main) = 0; /** Storage for orphan information */ - TxOrphanage m_orphanage; + TxOrphanage m_orphanage GUARDED_BY(m_tx_download_mutex); void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); @@ -1638,7 +1637,8 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, const Peer& peer) void PeerManagerImpl::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid, std::chrono::microseconds current_time) { - AssertLockHeld(::cs_main); // For m_txrequest + AssertLockHeld(::cs_main); // for State + AssertLockHeld(m_tx_download_mutex); // For m_txrequest NodeId nodeid = node.GetId(); if (!node.HasPermission(NetPermissionFlags::Relay) && m_txrequest.Count(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) { // Too many queued announcements from this peer @@ -1674,8 +1674,9 @@ void PeerManagerImpl::InitializeNode(CNode& node, ServiceFlags our_services) { NodeId nodeid = node.GetId(); { - LOCK(cs_main); + LOCK(cs_main); // For m_node_states m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn())); + LOCK(m_tx_download_mutex); assert(m_txrequest.Count(nodeid) == 0); } @@ -1748,8 +1749,11 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) } } } - m_orphanage.EraseForPeer(nodeid); - m_txrequest.DisconnectedPeer(nodeid); + { + LOCK(m_tx_download_mutex); + m_orphanage.EraseForPeer(nodeid); + m_txrequest.DisconnectedPeer(nodeid); + } if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); m_num_preferred_download_peers -= state->fPreferredDownload; m_peers_downloading_from -= (!state->vBlocksInFlight.empty()); @@ -1766,6 +1770,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) assert(m_peers_downloading_from == 0); assert(m_outbound_peers_with_protect_from_disconnect == 0); assert(m_wtxid_relay_peers == 0); + LOCK(m_tx_download_mutex); assert(m_txrequest.Size() == 0); assert(m_orphanage.Size() == 0); } @@ -2081,6 +2086,17 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) scheduler.scheduleFromNow([&] { ReattemptInitialBroadcast(scheduler); }, delta); } +void PeerManagerImpl::UpdatedBlockTipSync(const CBlockIndex* pindexNew) +{ + AssertLockNotHeld(m_mempool.cs); + LOCK(m_tx_download_mutex); + // If the chain tip has changed, previously rejected transactions might now be invalid, e.g. due + // to a timelock. Reset the rejection filters to give those transactions another chance if we + // see them again. + m_recent_rejects.reset(); + m_recent_rejects_reconsiderable.reset(); +} + /** * Evict orphan txn pool entries based on a newly connected * block, remember the recently confirmed transactions, and delete tracked @@ -2111,23 +2127,18 @@ void PeerManagerImpl::BlockConnected( if (role == ChainstateRole::BACKGROUND) { return; } + LOCK(m_tx_download_mutex); m_orphanage.EraseForBlock(*pblock); - { - LOCK(m_recent_confirmed_transactions_mutex); - for (const auto& ptx : pblock->vtx) { - m_recent_confirmed_transactions.insert(ptx->GetHash().ToUint256()); - if (ptx->HasWitness()) { - m_recent_confirmed_transactions.insert(ptx->GetWitnessHash().ToUint256()); - } + for (const auto& ptx : pblock->vtx) { + m_recent_confirmed_transactions.insert(ptx->GetHash().ToUint256()); + if (ptx->HasWitness()) { + m_recent_confirmed_transactions.insert(ptx->GetWitnessHash().ToUint256()); } } - { - LOCK(cs_main); - for (const auto& ptx : pblock->vtx) { - m_txrequest.ForgetTxHash(ptx->GetHash()); - m_txrequest.ForgetTxHash(ptx->GetWitnessHash()); - } + for (const auto& ptx : pblock->vtx) { + m_txrequest.ForgetTxHash(ptx->GetHash()); + m_txrequest.ForgetTxHash(ptx->GetWitnessHash()); } } @@ -2141,7 +2152,7 @@ void PeerManagerImpl::BlockDisconnected(const std::shared_ptr &blo // block's worth of transactions in it, but that should be fine, since // presumably the most common case of relaying a confirmed transaction // should be just after a new block containing it is found. - LOCK(m_recent_confirmed_transactions_mutex); + LOCK(m_tx_download_mutex); m_recent_confirmed_transactions.reset(); } @@ -2281,15 +2292,7 @@ void PeerManagerImpl::BlockChecked(const CBlock& block, const BlockValidationSta bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable) { - if (m_chainman.ActiveChain().Tip()->GetBlockHash() != hashRecentRejectsChainTip) { - // If the chain tip has changed previously rejected transactions - // might be now valid, e.g. due to a nLockTime'd tx becoming valid, - // or a double-spend. Reset the rejects filter and give those - // txs a second chance. - hashRecentRejectsChainTip = m_chainman.ActiveChain().Tip()->GetBlockHash(); - m_recent_rejects.reset(); - m_recent_rejects_reconsiderable.reset(); - } + AssertLockHeld(m_tx_download_mutex); const uint256& hash = gtxid.GetHash(); @@ -2313,10 +2316,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true; - { - LOCK(m_recent_confirmed_transactions_mutex); - if (m_recent_confirmed_transactions.contains(hash)) return true; - } + if (m_recent_confirmed_transactions.contains(hash)) return true; return m_recent_rejects.contains(hash) || m_mempool.exists(gtxid); } @@ -3199,7 +3199,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); - AssertLockHeld(cs_main); + AssertLockHeld(m_tx_download_mutex); LogDebug(BCLog::MEMPOOLREJ, "%s (wtxid=%s) from peer=%d was not accepted: %s\n", ptx->GetHash().ToString(), @@ -3264,7 +3264,7 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); - AssertLockHeld(cs_main); + AssertLockHeld(m_tx_download_mutex); // As this version of the transaction was acceptable, we can forget about any requests for it. // No-op if the tx is not in txrequest. @@ -3292,7 +3292,7 @@ void PeerManagerImpl::ProcessPackageResult(const PackageToValidate& package_to_v { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); - AssertLockHeld(cs_main); + AssertLockHeld(m_tx_download_mutex); const auto& package = package_to_validate.m_txns; const auto& senders = package_to_validate.m_senders; @@ -3348,7 +3348,7 @@ std::optional PeerManagerImpl::Find1P1CPacka { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); - AssertLockHeld(cs_main); + AssertLockHeld(m_tx_download_mutex); const auto& parent_wtxid{ptx->GetWitnessHash()}; @@ -3401,7 +3401,7 @@ std::optional PeerManagerImpl::Find1P1CPacka bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) { AssertLockHeld(g_msgproc_mutex); - LOCK(cs_main); + LOCK2(::cs_main, m_tx_download_mutex); CTransactionRef porphanTx = nullptr; @@ -4218,7 +4218,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const bool reject_tx_invs{RejectIncomingTxs(pfrom)}; - LOCK(cs_main); + LOCK2(cs_main, m_tx_download_mutex); const auto current_time{GetTime()}; uint256* best_block{nullptr}; @@ -4551,7 +4551,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const uint256& hash = peer->m_wtxid_relay ? wtxid : txid; AddKnownTx(*peer, hash); - LOCK(cs_main); + LOCK2(cs_main, m_tx_download_mutex); m_txrequest.ReceivedResponse(pfrom.GetId(), txid); if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid); @@ -5312,7 +5312,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, std::vector vInv; vRecv >> vInv; if (vInv.size() <= MAX_PEER_TX_ANNOUNCEMENTS + MAX_BLOCKS_IN_TRANSIT_PER_PEER) { - LOCK(::cs_main); + LOCK(m_tx_download_mutex); for (CInv &inv : vInv) { if (inv.IsGenTxMsg()) { // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as @@ -5433,6 +5433,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt // by another peer that was already processed; in that case, // the extra work may not be noticed, possibly resulting in an // unnecessary 100ms delay) + LOCK(m_tx_download_mutex); 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()); @@ -6317,6 +6318,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto) // // Message: getdata (transactions) // + LOCK(m_tx_download_mutex); std::vector> expired; auto requestable = m_txrequest.GetRequestable(pto->GetId(), current_time, &expired); for (const auto& entry : expired) { diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 450bf6a4fc733..1582c2424ad9c 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -21,15 +21,13 @@ BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup) class TxOrphanageTest : public TxOrphanage { public: - inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + inline size_t CountOrphans() const { - LOCK(m_mutex); return m_orphans.size(); } - CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + CTransactionRef RandomOrphan() { - LOCK(m_mutex); std::map::iterator it; it = m_orphans.lower_bound(Wtxid::FromUint256(InsecureRand256())); if (it == m_orphans.end()) diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 13ef96f3be29f..6fa1408d0faf6 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -19,8 +19,6 @@ static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) { - LOCK(m_mutex); - const Txid& hash = tx->GetHash(); const Wtxid& wtxid = tx->GetWitnessHash(); if (m_orphans.count(wtxid)) @@ -54,13 +52,11 @@ bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer) int TxOrphanage::EraseTx(const Wtxid& wtxid) { - LOCK(m_mutex); - return EraseTxNoLock(wtxid); + return EraseTxInternal(wtxid); } -int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid) +int TxOrphanage::EraseTxInternal(const Wtxid& wtxid) { - AssertLockHeld(m_mutex); std::map::iterator it = m_orphans.find(wtxid); if (it == m_orphans.end()) return 0; @@ -96,8 +92,6 @@ int TxOrphanage::EraseTxNoLock(const Wtxid& wtxid) void TxOrphanage::EraseForPeer(NodeId peer) { - LOCK(m_mutex); - m_peer_work_set.erase(peer); int nErased = 0; @@ -107,7 +101,7 @@ void TxOrphanage::EraseForPeer(NodeId peer) // increment to avoid iterator becoming invalid after erasure const auto& [wtxid, orphan] = *iter++; if (orphan.fromPeer == peer) { - nErased += EraseTxNoLock(wtxid); + nErased += EraseTxInternal(wtxid); } } if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", nErased, peer); @@ -115,8 +109,6 @@ void TxOrphanage::EraseForPeer(NodeId peer) void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) { - LOCK(m_mutex); - unsigned int nEvicted = 0; static int64_t nNextSweep; int64_t nNow = GetTime(); @@ -129,7 +121,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->GetWitnessHash()); + nErased += EraseTxInternal(maybeErase->second.tx->GetWitnessHash()); } else { nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime); } @@ -142,7 +134,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]->second.tx->GetWitnessHash()); + EraseTxInternal(m_orphan_list[randompos]->second.tx->GetWitnessHash()); ++nEvicted; } if (nEvicted > 0) LogPrint(BCLog::TXPACKAGES, "orphanage overflow, removed %u tx\n", nEvicted); @@ -150,9 +142,6 @@ void TxOrphanage::LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) { - LOCK(m_mutex); - - 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()) { @@ -171,14 +160,11 @@ void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx) bool TxOrphanage::HaveTx(const Wtxid& wtxid) const { - LOCK(m_mutex); return m_orphans.count(wtxid); } CTransactionRef TxOrphanage::GetTxToReconsider(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; @@ -197,8 +183,6 @@ CTransactionRef TxOrphanage::GetTxToReconsider(NodeId peer) 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; @@ -209,8 +193,6 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer) void TxOrphanage::EraseForBlock(const CBlock& block) { - LOCK(m_mutex); - std::vector vOrphanErase; for (const CTransactionRef& ptx : block.vtx) { @@ -231,7 +213,7 @@ void TxOrphanage::EraseForBlock(const CBlock& block) if (vOrphanErase.size()) { int nErased = 0; for (const auto& orphanHash : vOrphanErase) { - nErased += EraseTxNoLock(orphanHash); + nErased += EraseTxInternal(orphanHash); } LogPrint(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) included or conflicted by block\n", nErased); } @@ -239,8 +221,6 @@ void TxOrphanage::EraseForBlock(const CBlock& block) std::vector TxOrphanage::GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const { - LOCK(m_mutex); - // First construct a vector of iterators to ensure we do not return duplicates of the same tx // and so we can sort by nTimeExpire. std::vector iters; @@ -281,8 +261,6 @@ std::vector TxOrphanage::GetChildrenFromSamePeer(const CTransac std::vector> TxOrphanage::GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const { - LOCK(m_mutex); - // First construct vector of iterators to ensure we do not return duplicates of the same tx. std::vector iters; diff --git a/src/txorphanage.h b/src/txorphanage.h index 5f9888c969e07..8d81adb0ad37e 100644 --- a/src/txorphanage.h +++ b/src/txorphanage.h @@ -21,55 +21,51 @@ class TxOrphanage { public: /** Add a new orphan transaction */ - bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + bool AddTx(const CTransactionRef& tx, NodeId peer); /** Check if we already have an orphan transaction (by wtxid only) */ - bool HaveTx(const Wtxid& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + bool HaveTx(const Wtxid& wtxid) 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 * it from the work set. */ - CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + CTransactionRef GetTxToReconsider(NodeId peer); /** Erase an orphan by wtxid */ - int EraseTx(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + int EraseTx(const Wtxid& wtxid); /** Erase all orphans announced by a peer (eg, after that peer disconnects) */ - void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + void EraseForPeer(NodeId peer); /** Erase all orphans included in or invalidated by a new block */ - void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + void EraseForBlock(const CBlock& block); /** Limit the orphanage to the given maximum */ - void LimitOrphans(unsigned int max_orphans, FastRandomContext& rng) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + void LimitOrphans(unsigned int max_orphans, FastRandomContext& rng); /** 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);; + void AddChildrenToWorkSet(const CTransaction& tx); /** Does this peer have any work to do? */ - bool HaveTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);; + bool HaveTxToReconsider(NodeId peer); /** Get all children that spend from this tx and were received from nodeid. Sorted from most * recent to least recent. */ - std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const; /** Get all children that spend from this tx but were not received from nodeid. Also return * which peer provided each tx. */ - std::vector> GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + std::vector> GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const; /** Return how many entries exist in the orphange */ - size_t Size() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + size_t Size() { - LOCK(m_mutex); return m_orphans.size(); } protected: - /** Guards orphan transactions */ - mutable Mutex m_mutex; - struct OrphanTx { CTransactionRef tx; NodeId fromPeer; @@ -79,10 +75,10 @@ class TxOrphanage { /** 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; /** 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; using OrphanMap = decltype(m_orphans); @@ -97,13 +93,13 @@ class TxOrphanage { /** Index from the parents' COutPoint into the m_orphans. Used * to remove orphan transactions from the m_orphans */ - std::map> m_outpoint_to_orphan_it GUARDED_BY(m_mutex); + std::map> m_outpoint_to_orphan_it; /** Orphan transactions in vector for quick random eviction */ - std::vector m_orphan_list GUARDED_BY(m_mutex); + std::vector m_orphan_list; /** Erase an orphan by wtxid */ - int EraseTxNoLock(const Wtxid& wtxid) EXCLUSIVE_LOCKS_REQUIRED(m_mutex); + int EraseTxInternal(const Wtxid& wtxid); }; #endif // BITCOIN_TXORPHANAGE_H diff --git a/src/validation.cpp b/src/validation.cpp index d398ec74061c9..6fa28b62e4b20 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -3330,6 +3330,8 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< { LOCK(cs_main); + bool notify_updated_tip = false; + { // Lock transaction pool for at least as long as it takes for connectTrace to be consumed LOCK(MempoolMutex()); const bool was_in_ibd = m_chainman.IsInitialBlockDownload(); @@ -3394,6 +3396,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< if (this == &m_chainman.ActiveChainstate() && pindexFork != pindexNewTip) { // Notify ValidationInterface subscribers if (m_chainman.m_options.signals) { + notify_updated_tip = true; m_chainman.m_options.signals->UpdatedBlockTip(pindexNewTip, pindexFork, still_in_ibd); } @@ -3406,7 +3409,11 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr< break; } } - } + } // release MempoolMutex + if (notify_updated_tip) { + m_chainman.m_options.signals->UpdatedBlockTipSync(pindexNewTip); + } + } // release cs_main // When we reach this point, we switched to a new tip (stored in pindexNewTip). if (exited_ibd) { @@ -3625,6 +3632,9 @@ bool Chainstate::InvalidateBlock(BlockValidationState& state, CBlockIndex* pinde // distinguish user-initiated invalidateblock changes from other // changes. (void)m_chainman.GetNotifications().blockTip(GetSynchronizationState(m_chainman.IsInitialBlockDownload(), m_chainman.m_blockman.m_reindexing), *to_mark_failed->pprev); + if (m_chainman.m_options.signals) { + m_chainman.m_options.signals->UpdatedBlockTipSync(m_chain.Tip()); + } } return true; } diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp index 813fde109c5fc..1f736bec21b8f 100644 --- a/src/validationinterface.cpp +++ b/src/validationinterface.cpp @@ -183,6 +183,12 @@ void ValidationSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlo fInitialDownload); } +void ValidationSignals::UpdatedBlockTipSync(const CBlockIndex *pindexNew) +{ + LOG_EVENT("%s: new block hash=%s block height=%d", __func__, pindexNew->GetBlockHash().ToString(), pindexNew->nHeight); + m_internals->Iterate([&](CValidationInterface& callbacks) { callbacks.UpdatedBlockTipSync(pindexNew); }); +} + void ValidationSignals::TransactionAddedToMempool(const NewMempoolTransactionInfo& tx, uint64_t mempool_sequence) { auto event = [tx, mempool_sequence, this] { diff --git a/src/validationinterface.h b/src/validationinterface.h index 6f49a73c93c0d..36a74c35622d1 100644 --- a/src/validationinterface.h +++ b/src/validationinterface.h @@ -61,6 +61,10 @@ class CValidationInterface { * Called on a background thread. Only called for the active chainstate. */ virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {} + /** + * Notifies listeners when the block chain tip advances synchronously. + */ + virtual void UpdatedBlockTipSync(const CBlockIndex* pindexNew) {}; /** * Notifies listeners of a transaction having been added to mempool. * @@ -214,6 +218,7 @@ class ValidationSignals { void SyncWithValidationInterfaceQueue() LOCKS_EXCLUDED(cs_main); void UpdatedBlockTip(const CBlockIndex *, const CBlockIndex *, bool fInitialDownload); + void UpdatedBlockTipSync(const CBlockIndex*); void TransactionAddedToMempool(const NewMempoolTransactionInfo&, uint64_t mempool_sequence); void TransactionRemovedFromMempool(const CTransactionRef&, MemPoolRemovalReason, uint64_t mempool_sequence); void MempoolTransactionsRemovedForBlock(const std::vector&, unsigned int nBlockHeight);