From 5c6e98e66b699ad4556e37fe2c05c26627ce1acf Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 17 Oct 2025 17:40:54 -0500 Subject: [PATCH 1/3] refactor: replace pair with structured PendingISLockFromPeer in CInstantSendManager Updated the CInstantSendManager to use a new struct, PendingISLockFromPeer, for better clarity and type safety. This change replaces the use of std::pair for storing node ID and InstantSendLockPtr, enhancing code readability and maintainability across multiple functions handling instant send locks. --- src/instantsend/instantsend.cpp | 28 ++++++++++++++-------------- src/instantsend/instantsend.h | 11 ++++++++--- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 2ddb6e80083e..c64c357ed59c 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -168,7 +168,7 @@ MessageProcessingResult CInstantSendManager::ProcessMessage(NodeId from, std::st } LOCK(cs_pendingLocks); - pendingInstantSendLocks.emplace(hash, std::make_pair(from, islock)); + pendingInstantSendLocks.emplace(hash, instantsend::PendingISLockFromPeer{from, islock}); return ret; } @@ -239,7 +239,7 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks( const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, - const Uint256HashMap>& pend, + const Uint256HashMap& pend, std::vector>& peer_activity) { CBLSBatchVerifier batchVerifier(false, true, 8); @@ -249,8 +249,8 @@ Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks( size_t alreadyVerified = 0; for (const auto& p : pend) { const auto& hash = p.first; - auto nodeId = p.second.first; - const auto& islock = p.second.second; + auto nodeId = p.second.node_id; + const auto& islock = p.second.islock; if (batchVerifier.badSources.count(nodeId)) { continue; @@ -321,8 +321,8 @@ Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks( } for (const auto& p : pend) { const auto& hash = p.first; - auto nodeId = p.second.first; - const auto& islock = p.second.second; + auto nodeId = p.second.node_id; + const auto& islock = p.second.islock; if (batchVerifier.badMessages.count(hash)) { LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: invalid sig in islock, peer=%d\n", @@ -399,7 +399,7 @@ MessageProcessingResult CInstantSendManager::ProcessInstantSendLock(NodeId from, } else { // put it in a separate pending map and try again later LOCK(cs_pendingLocks); - pendingNoTxInstantSendLocks.try_emplace(hash, std::make_pair(from, islock)); + pendingNoTxInstantSendLocks.try_emplace(hash, instantsend::PendingISLockFromPeer{from, islock}); } // This will also add children TXs to pendingRetryTxs @@ -442,11 +442,11 @@ void CInstantSendManager::TransactionAddedToMempool(const CTransactionRef& tx) LOCK(cs_pendingLocks); auto it = pendingNoTxInstantSendLocks.begin(); while (it != pendingNoTxInstantSendLocks.end()) { - if (it->second.second->txid == tx->GetHash()) { + if (it->second.islock->txid == tx->GetHash()) { // we received an islock earlier LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, tx->GetHash().ToString(), it->first.ToString()); - islock = it->second.second; + islock = it->second.islock; pendingInstantSendLocks.try_emplace(it->first, it->second); pendingNoTxInstantSendLocks.erase(it); break; @@ -539,7 +539,7 @@ void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx, const CBlock LOCK(cs_pendingLocks); auto it = pendingNoTxInstantSendLocks.begin(); while (it != pendingNoTxInstantSendLocks.end()) { - if (it->second.second->txid == tx->GetHash()) { + if (it->second.islock->txid == tx->GetHash()) { // we received an islock earlier, let's put it back into pending and verify/lock LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, tx->GetHash().ToString(), it->first.ToString()); @@ -626,7 +626,7 @@ void CInstantSendManager::TryEmplacePendingLock(const uint256& hash, const NodeI if (db.KnownInstantSendLock(hash)) return; LOCK(cs_pendingLocks); if (!pendingInstantSendLocks.count(hash)) { - pendingInstantSendLocks.emplace(hash, std::make_pair(id, islock)); + pendingInstantSendLocks.emplace(hash, instantsend::PendingISLockFromPeer{id, islock}); } } @@ -848,11 +848,11 @@ bool CInstantSendManager::GetInstantSendLockByHash(const uint256& hash, instants LOCK(cs_pendingLocks); auto it = pendingInstantSendLocks.find(hash); if (it != pendingInstantSendLocks.end()) { - islock = it->second.second; + islock = it->second.islock; } else { auto itNoTx = pendingNoTxInstantSendLocks.find(hash); if (itNoTx != pendingNoTxInstantSendLocks.end()) { - islock = itNoTx->second.second; + islock = itNoTx->second.islock; } else { return false; } @@ -889,7 +889,7 @@ bool CInstantSendManager::IsWaitingForTx(const uint256& txHash) const LOCK(cs_pendingLocks); auto it = pendingNoTxInstantSendLocks.begin(); while (it != pendingNoTxInstantSendLocks.end()) { - if (it->second.second->txid == txHash) { + if (it->second.islock->txid == txHash) { LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__, txHash.ToString(), it->first.ToString()); return true; diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index d9343e9cf418..13e86704ecff 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -38,6 +38,11 @@ struct DbWrapperParams; namespace instantsend { class InstantSendSigner; +struct PendingISLockFromPeer { + NodeId node_id; + InstantSendLockPtr islock; +}; + struct PendingState { bool m_pending_work{false}; std::vector> m_peer_activity{}; @@ -69,9 +74,9 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent mutable Mutex cs_pendingLocks; // Incoming and not verified yet - Uint256HashMap> pendingInstantSendLocks GUARDED_BY(cs_pendingLocks); + Uint256HashMap pendingInstantSendLocks GUARDED_BY(cs_pendingLocks); // Tried to verify but there is no tx yet - Uint256HashMap> pendingNoTxInstantSendLocks GUARDED_BY(cs_pendingLocks); + Uint256HashMap pendingNoTxInstantSendLocks GUARDED_BY(cs_pendingLocks); // TXs which are neither IS locked nor ChainLocked. We use this to determine for which TXs we need to retry IS // locking of child TXs @@ -117,7 +122,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); Uint256HashSet ProcessPendingInstantSendLocks(const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, - const Uint256HashMap>& pend, + const Uint256HashMap& pend, std::vector>& peer_activity) EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); MessageProcessingResult ProcessInstantSendLock(NodeId from, const uint256& hash, From 2980e60c08ea28c0f8f242e4c5c91dcb22e71777 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 17 Oct 2025 17:54:17 -0500 Subject: [PATCH 2/3] perf: use vector instead of hash map for ProcessPendingInstantSendLocks The 'pend' local variable in ProcessPendingInstantSendLocks was previously using the same data structure as pendingInstantSendLocks (a hash map). However, once we're in the processing step, we only iterate sequentially through the locks - there are no hash-based lookups. This commit changes 'pend' to use std::vector for better performance: - Improved cache locality with contiguous memory layout - Better CPU prefetching during iteration (3x through the data) - Eliminates hash map overhead (bucket allocation, pointer chasing) - Filtering step uses build-new-vector approach to maintain O(n) The typical case processes up to 32 locks, making the vector's sequential access pattern ideal for modern CPU cache hierarchies. --- src/instantsend/instantsend.cpp | 22 ++++++++++++---------- src/instantsend/instantsend.h | 3 ++- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index c64c357ed59c..675b90bcfaf0 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -174,7 +174,7 @@ MessageProcessingResult CInstantSendManager::ProcessMessage(NodeId from, std::st instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() { - decltype(pendingInstantSendLocks) pend; + std::vector> pend; instantsend::PendingState ret; if (!IsInstantSendEnabled()) { @@ -189,6 +189,7 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() // The keys of the removed values are temporaily stored here to avoid invalidating an iterator std::vector removed; removed.reserve(maxCount); + pend.reserve(maxCount); for (const auto& [islockHash, nodeid_islptr_pair] : pendingInstantSendLocks) { // Check if we've reached max count @@ -196,7 +197,7 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() ret.m_pending_work = true; break; } - pend.emplace(islockHash, std::move(nodeid_islptr_pair)); + pend.emplace_back(islockHash, std::move(nodeid_islptr_pair)); removed.emplace_back(islockHash); } @@ -222,16 +223,17 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() if (!badISLocks.empty()) { LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- doing verification on old active set\n", __func__); - // filter out valid IS locks from "pend" - for (auto it = pend.begin(); it != pend.end();) { - if (!badISLocks.count(it->first)) { - it = pend.erase(it); - } else { - ++it; + // filter out valid IS locks from "pend" - keep only bad ones + std::vector> filteredPend; + filteredPend.reserve(badISLocks.size()); + for (auto& p : pend) { + if (badISLocks.contains(p.first)) { + filteredPend.push_back(std::move(p)); } } + // Now check against the previous active set and perform banning if this fails - ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, pend, ret.m_peer_activity); + ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, filteredPend, ret.m_peer_activity); } return ret; @@ -239,7 +241,7 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() Uint256HashSet CInstantSendManager::ProcessPendingInstantSendLocks( const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, - const Uint256HashMap& pend, + const std::vector>& pend, std::vector>& peer_activity) { CBLSBatchVerifier batchVerifier(false, true, 8); diff --git a/src/instantsend/instantsend.h b/src/instantsend/instantsend.h index 13e86704ecff..ebe6b4cb48b2 100644 --- a/src/instantsend/instantsend.h +++ b/src/instantsend/instantsend.h @@ -20,6 +20,7 @@ #include #include #include +#include class CBlockIndex; class CChainState; @@ -122,7 +123,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); Uint256HashSet ProcessPendingInstantSendLocks(const Consensus::LLMQParams& llmq_params, int signOffset, bool ban, - const Uint256HashMap& pend, + const std::vector>& pend, std::vector>& peer_activity) EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); MessageProcessingResult ProcessInstantSendLock(NodeId from, const uint256& hash, From 1b2bab48ad600c4acfc19758e0bf51e84244cc16 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 10 Nov 2025 20:23:20 -0600 Subject: [PATCH 3/3] chore: whitespace fix --- src/instantsend/instantsend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/instantsend/instantsend.cpp b/src/instantsend/instantsend.cpp index 675b90bcfaf0..293d35928e3c 100644 --- a/src/instantsend/instantsend.cpp +++ b/src/instantsend/instantsend.cpp @@ -231,7 +231,7 @@ instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() filteredPend.push_back(std::move(p)); } } - + // Now check against the previous active set and perform banning if this fails ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, filteredPend, ret.m_peer_activity); }