src/protocol.h (2)

19-19: Include set is incomplete for using std::optional directly in this header

Good call adding . This header also uses std::optional in MessageProcessingResult but doesn't include itself, relying on transitive includes. Make the header self-contained.

Apply this diff near the other standard headers:

 #include <limits>
 #include <string>
+#include <optional>
 #include <variant>

---

603-608: Shape of m_inv_filter is terse; consider a named struct for readability and future extension

The optional pair<inv, variant<txref, txid>> works, but it's opaque at call sites and in PostProcessMessage. A small named type improves clarity and avoids nested templates in signatures.

For example:

+struct FilteredInvAction {
+    CInv inv;
+    std::variant<CTransactionRef, uint256> related; // tx or just txid
+};
 ...
-    std::optional<std::pair<CInv, std::variant<CTransactionRef, uint256>>> m_inv_filter;
+    std::optional<FilteredInvAction> m_inv_filter;

Follow-ups in users: ret.m_inv_filter = FilteredInvAction{inv, tx}; or {inv, islock->txid}.

src/instantsend/instantsend.h (1)

38-44: Give names to per-peer actions and make PendingState a bit more self-documenting

The vector<pair<NodeId, MessageProcessingResult>> type repeats and is hard to scan. A small alias (or struct) clarifies intent and reduces template noise.

Apply:

 namespace instantsend {
-struct PendingState {
+using PeerAction = std::pair<NodeId, MessageProcessingResult>;
+
+struct PendingState {
     bool m_pending_work{false};
-    std::vector<std::pair<NodeId, MessageProcessingResult>> m_peer_activity{};
+    std::vector<PeerAction> m_peer_activity{};
 };
 } // namespace instantsend

Optionally mark PendingState-returning helpers [[nodiscard]] to discourage accidental result drops.

src/instantsend/instantsend.cpp (4)

172-208: Micro-optimizations and a typo in batching pending locks

• The for-loop iterates with const auto&… then std::move on a const ref, which defeats moving. Use non-const ref if you want to move from the map’s value. Shared_ptr copies are cheap, but this is easy to make correct.
• Reserve buckets in pend to avoid rehashing.
• Typo: “temporaily” -> “temporarily”.

Apply:

 instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks()
 {
     decltype(pendingInstantSendLocks) pend;
     instantsend::PendingState ret;
@@
-        // The keys of the removed values are temporaily stored here to avoid invalidating an iterator
+        // The keys of the removed values are temporarily stored here to avoid invalidating an iterator
         std::vector<uint256> removed;
         removed.reserve(maxCount);
+        pend.reserve(maxCount);
 
-        for (const auto& [islockHash, nodeid_islptr_pair] : pendingInstantSendLocks) {
+        for (auto& [islockHash, nodeid_islptr_pair] : pendingInstantSendLocks) {
             // Check if we've reached max count
             if (pend.size() >= maxCount) {
                 ret.m_pending_work = true;
                 break;
             }
             pend.emplace(islockHash, std::move(nodeid_islptr_pair));
             removed.emplace_back(islockHash);
         }

---

238-347: Pre-size peer_activity to reduce reallocations during batch processing

You append once per pending lock and once per bad source. Reserving amortizes vector growth, especially under high load.

Add just before the ban handling:

 std::unordered_set<uint256, StaticSaltedHasher> badISLocks;
 
+// Reserve space for per-peer work results to avoid reallocations
+peer_activity.reserve(peer_activity.size() + pend.size() + batchVerifier.badSources.size());
+
 if (ban && !batchVerifier.badSources.empty()) {

---

349-429: Returning MessageProcessingResult with filtered INV and optional tx request — LGTM

The split between “announce with tx-aware filtering” vs “announce with txid and request tx” is clear and keeps I/O outside the worker.

Minor nits:

• Consider making inv const: const CInv inv(MSG_ISDLOCK, hash);
• Comments: “we don’t have the TX yet, so we only filter based on txid” — perfect context, keep this style in future touch-ups.

---

923-931: Avoid copying m_peer_activity with structured binding; move from the returned state instead

Structured binding from the prvalue PendingState will copy the vector into peer_activity before you move individual elements. Bind the whole struct and move from its member.

Apply:

-            auto [more_work, peer_activity] = ProcessPendingInstantSendLocks();
-            for (auto& [node_id, mpr] : peer_activity) {
+            auto st = ProcessPendingInstantSendLocks();
+            for (auto& [node_id, mpr] : st.m_peer_activity) {
                 peerman.PostProcessMessage(std::move(mpr), node_id);
             }
-            if (!m_signer) return more_work;
+            if (!m_signer) return st.m_pending_work;

src/net_processing.cpp (3)

1579-1611: RequestObject: scheduling logic is correct; fix unsafe logging format and consider small clarity nits

• Using m_mn_activeman != nullptr as the “this node is MN” signal passed into CalculateObjectGetDataTime preserves the previous behavior without threading a flag through APIs. Looks good.
• The LogPrint format uses %d for microseconds (int) but count() returns a 64-bit integer. That’s undefined behavior on LLP64/LP64. Cast or use a 64-bit specifier.

Apply this logging fix:

-    LogPrint(BCLog::NET, "%s -- inv=(%s), current_time=%d, process_time=%d, delta=%d\n", __func__, inv.ToString(), current_time.count(), process_time.count(), (process_time - current_time).count());
+    LogPrint(BCLog::NET, "%s -- inv=(%s), current_time=%lld, process_time=%lld, delta=%lld\n",
+            __func__, inv.ToString(),
+            static_cast<long long>(current_time.count()),
+            static_cast<long long>(process_time.count()),
+            static_cast<long long>((process_time - current_time).count()));

Optional clarity: assign a local const bool is_masternode = (m_mn_activeman != nullptr) before calling CalculateObjectGetDataTime to make intent explicit.

---

2296-2323: AskPeersForTransaction: add fairness (randomization) when selecting up to 4 peers

Current code picks the first 4 peers from m_peer_map with a positive filter hit, which can bias requests toward stable insertion order. Use reservoir sampling (or shuffle candidates) to fairly choose up to 4 peers.

Proposed change using reservoir sampling and preserving existing locks:

 void PeerManagerImpl::AskPeersForTransaction(const uint256& txid)
 {
-    std::vector<PeerRef> peersToAsk;
-    peersToAsk.reserve(4);
+    std::vector<PeerRef> peersToAsk;
+    peersToAsk.reserve(4);
 
     {
         LOCK(m_peer_mutex);
-        // TODO consider prioritizing MNs again, once that flag is moved into Peer
-        for (const auto& [_, peer] : m_peer_map) {
-            if (peersToAsk.size() >= 4) {
-                break;
-            }
-            if (IsInvInFilter(*peer, txid)) {
-                peersToAsk.emplace_back(peer);
-            }
-        }
+        // TODO consider prioritizing MNs again, once that flag is moved into Peer
+        size_t seen = 0;
+        for (const auto& [_, peer] : m_peer_map) {
+            if (!IsInvInFilter(*peer, txid)) continue;
+            ++seen;
+            if (peersToAsk.size() < 4) {
+                peersToAsk.emplace_back(peer);
+            } else {
+                // Replace existing with decreasing probability
+                const size_t j = static_cast<size_t>(GetRand(seen));
+                if (j < 4) peersToAsk[j] = peer;
+            }
+        }
     }
     {
         CInv inv(MSG_TX, txid);
         LOCK(cs_main);
         for (PeerRef& peer : peersToAsk) {
             LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__,
                       txid.ToString(), peer->m_id);
 
-            RequestObject(peer->m_id, inv, GetTime<std::chrono::microseconds>(), /*fForce=*/true);
+            RequestObject(peer->m_id, inv, GetTime<std::chrono::microseconds>(), /*fForce=*/true);
         }
     }
 }

---

646-663: All RequestObject and RelayInvFiltered call sites updated to new signatures

Verified via a repository-wide search that there are no remaining uses of the old RequestObject(is_masternode) overload or RelayInvFiltered/AskPeersForTransaction variants—every invocation now uses the 4-argument RequestObject (with fForce defaulted where omitted), and both RelayInvFiltered overloads are called with the appropriate minProtoVersion.

• No calls to an old RequestObject signature were found outside net_processing.cpp; all RequestObject( usages accept (NodeId, const CInv&, microseconds, bool) only.
• All RelayInvFiltered( and AskPeersForTransaction( invocations pass the correct parameters and versions.

Next steps:

• Add a brief comment adjacent to these private declarations in src/net_processing.cpp (lines 646–663) noting that PostProcessMessage leverages ISDLOCK_PROTO_VERSION as the relay gate when m_inv_filter is set, and that this is intentional for InstantSend behavior.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.

src/net_processing.cpp (1)

3506-3518: Variant handling for m_inv_filter: nit addressed.

The explicit holds_alternative branches with a defensive assert cover all variant cases cleanly.

src/instantsend/instantsend.cpp (4)

186-188: Spelling nit: “temporaily” → “temporarily”

Minor doc polish.

-        // The keys of the removed values are temporaily stored here to avoid invalidating an iterator
+        // The keys of the removed values are temporarily stored here to avoid invalidating an iterator

---

218-218: Fix clang-format diff failure on wrapped call

CI reports clang-format differences for the call sites. Reflow the arguments to match the project style.

-auto badISLocks = ProcessPendingInstantSendLocks(llmq_params, /*signOffset=*/0, /*ban=*/false, pend, ret.m_peer_activity);
+auto badISLocks = ProcessPendingInstantSendLocks(
+    llmq_params, /*signOffset=*/0, /*ban=*/false, pend, ret.m_peer_activity);
@@
-ProcessPendingInstantSendLocks(llmq_params, dkgInterval, /*ban=*/true, pend, ret.m_peer_activity);
+ProcessPendingInstantSendLocks(
+    llmq_params, dkgInterval, /*ban=*/true, pend, ret.m_peer_activity);

Also applies to: 231-231

---

311-318: Remove unnecessary cs_main lock in ban path

peer_activity.emplace_back(nodeId, MisbehavingError{20}); doesn’t touch chainstate; taking ::cs_main here adds contention without need.

-    if (ban && !batchVerifier.badSources.empty()) {
-        LOCK(::cs_main);
-        for (const auto& nodeId : batchVerifier.badSources) {
+    if (ban && !batchVerifier.badSources.empty()) {
+        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
             peer_activity.emplace_back(nodeId, MisbehavingError{20});
         }
     }

---

923-932: Avoid copying peer_activity with structured bindings

auto [more_work, peer_activity] = ...; copies the vector. Move out of the returned struct to avoid an extra allocation/copy.

-            auto [more_work, peer_activity] = ProcessPendingInstantSendLocks();
-            for (auto& [node_id, mpr] : peer_activity) {
-                peerman.PostProcessMessage(std::move(mpr), node_id);
-            }
+            auto pending = ProcessPendingInstantSendLocks();
+            const bool more_work = pending.m_pending_work;
+            for (auto& [node_id, mpr] : pending.m_peer_activity) {
+                peerman.PostProcessMessage(std::move(mpr), node_id);
+            }

src/net_processing.cpp (3)

649-657: RelayInvFiltered helpers moved private: OK; minProtoVersion assumption is acceptable here.

Given current usage (InstantSend), ISDLOCK_PROTO_VERSION as a floor is fine. If future callers want broader relays, consider threading minProtoVersion through MessageProcessingResult.

---

1579-1611: RequestObject rewrite: semantics and timing look correct.

• Inferring masternode role via m_mn_activeman in CalculateObjectGetDataTime matches prior intent (skip inbound delay for MNs).
• Capacity/dup guards on m_object_announced/process_time are preserved.

Minor: the LogPrint uses %d for microsecond counts; prefer 64-bit formatting to avoid truncation.

-    LogPrint(BCLog::NET, "%s -- inv=(%s), current_time=%d, process_time=%d, delta=%d\n", __func__, inv.ToString(), current_time.count(), process_time.count(), (process_time - current_time).count());
+    LogPrint(BCLog::NET, "%s -- inv=(%s), current_time=%lld, process_time=%lld, delta=%lld\n",
+             __func__, inv.ToString(),
+             (long long)current_time.count(),
+             (long long)process_time.count(),
+             (long long)(process_time - current_time).count());

---

2296-2321: Avoid deterministic peer bias when asking for TX.

We always pick the first 4 peers in m_peer_map. Randomize the choice to spread load and reduce adversarial influence.

-    std::vector<PeerRef> peersToAsk;
-    peersToAsk.reserve(4);
-    {
-        LOCK(m_peer_mutex);
-        // TODO consider prioritizing MNs again, once that flag is moved into Peer
-        for (const auto& [_, peer] : m_peer_map) {
-            if (peersToAsk.size() >= 4) {
-                break;
-            }
-            if (IsInvInFilter(*peer, txid)) {
-                peersToAsk.emplace_back(peer);
-            }
-        }
-    }
+    std::vector<PeerRef> candidates;
+    {
+        LOCK(m_peer_mutex);
+        for (const auto& [_, peer] : m_peer_map) {
+            if (IsInvInFilter(*peer, txid)) candidates.emplace_back(peer);
+        }
+    }
+    // Shuffle and take up to 4
+    Shuffle(candidates.begin(), candidates.end(), FastRandomContext());
+    std::vector<PeerRef> peersToAsk;
+    peersToAsk.reserve(std::min<size_t>(4, candidates.size()));
+    for (size_t i = 0; i < std::min<size_t>(4, candidates.size()); ++i) {
+        peersToAsk.emplace_back(candidates[i]);
+    }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.