refactor: follow-up dash#6761, implement review suggestions, more InstantSend cleanup #6815
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
dash#6425 moved AskNodesForLockedTx to PeerManager as AskPeersForTransaction, which removed the only usage of IsInvInFilter. We can safely remove it from the PeerManager interface as its usage is now purely internal.
PushInv takes a `const CInv&` and we don't expect to modify the payload
So far we cannot stop using `PeerManager` entirely the triggers are internal (i.e. not coming from P2P interactions) but we can extend and embrace MessageProcessingResult, to at least isolate its usage as best we can.
With their usage being brokered by `MessageProcessingResult`, we can remove them from the interface and keep them as implementation details.
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/chainlock/chainlock.h (1)
61-63: Nit: Prefer consistent initialization syntax for pointer membersStyle-only: prefer “= nullptr” for pointer members to match the rest of the codebase and improve readability. Also likely satisfies clang-format’s expectations.
Apply this diff:
- const CBlockIndex* bestChainLockBlockIndex GUARDED_BY(cs){nullptr}; - const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs){nullptr}; + const CBlockIndex* bestChainLockBlockIndex GUARDED_BY(cs) = nullptr; + const CBlockIndex* lastNotifyChainLockBlockIndex GUARDED_BY(cs) = nullptr;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
-
src/chainlock/chainlock.cpp(11 hunks) -
src/chainlock/chainlock.h(3 hunks) -
src/chainlock/clsig.cpp(2 hunks) -
src/chainlock/clsig.h(1 hunks) -
src/chainlock/signing.cpp(8 hunks) -
src/chainlock/signing.h(1 hunks) -
src/instantsend/instantsend.cpp(13 hunks) -
src/instantsend/instantsend.h(2 hunks) -
src/instantsend/lock.cpp(2 hunks) -
src/instantsend/lock.h(1 hunks) -
src/instantsend/signing.cpp(3 hunks) -
src/net_processing.cpp(9 hunks) -
src/net_processing.h(2 hunks) -
src/protocol.h(2 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- src/chainlock/chainlock.cpp
- src/instantsend/instantsend.h
- src/net_processing.cpp
- src/instantsend/instantsend.cpp
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/instantsend/lock.hsrc/chainlock/signing.hsrc/instantsend/lock.cppsrc/instantsend/signing.cppsrc/chainlock/clsig.hsrc/net_processing.hsrc/protocol.hsrc/chainlock/clsig.cppsrc/chainlock/signing.cppsrc/instantsend/instantsend.cppsrc/chainlock/chainlock.cppsrc/instantsend/instantsend.hsrc/net_processing.cppsrc/chainlock/chainlock.h
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/chainlock/signing.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
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.
Applied to files:
src/chainlock/chainlock.h
🧬 Code Graph Analysis (9)
src/instantsend/lock.h (1)
src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/instantsend/signing.cpp (1)
src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/chainlock/clsig.h (1)
src/chainlock/clsig.cpp (2)
GenSigRequestId(27-30)GenSigRequestId(27-27)
src/net_processing.h (1)
src/net_processing.cpp (23)
RelayInv(2346-2357)RelayInv(2346-2346)RelayInv(2359-2366)RelayInv(2359-2359)inv(630-630)inv(631-631)inv(653-653)inv(659-659)inv(937-937)inv(2316-2316)PostProcessMessage(3500-3525)PostProcessMessage(3500-3500)ret(662-662)ret(663-663)node(614-614)node(615-615)node(640-640)node(767-767)node(864-868)node(879-879)node(890-890)node(901-901)node(909-909)
src/chainlock/signing.cpp (2)
src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)src/chainlock/clsig.cpp (2)
GenSigRequestId(27-30)GenSigRequestId(27-27)
src/instantsend/instantsend.cpp (2)
src/net_processing.cpp (16)
ret(662-662)ret(663-663)ProcessMessage(3527-5311)ProcessMessage(3527-3532)pfrom(616-616)pfrom(637-638)pfrom(731-733)pfrom(739-739)pfrom(747-747)pfrom(750-750)pfrom(752-752)pfrom(754-754)pfrom(847-847)pfrom(1029-1029)from(1032-1032)node_id(728-728)src/instantsend/lock.cpp (4)
GenInputLockRequestId(42-45)GenInputLockRequestId(42-42)GenInputLockRequestId(46-46)GenInputLockRequestId(47-47)
src/chainlock/chainlock.cpp (4)
src/chainlock/chainlock.h (2)
cs(61-124)cs(62-121)src/chainlock/clsig.cpp (2)
GenSigRequestId(27-30)GenSigRequestId(27-27)src/instantsend/signing.cpp (1)
GetTransaction(21-22)src/chainlock/signing.h (1)
TX_CONFIRM_THRESHOLD(24-24)
src/instantsend/instantsend.h (1)
src/protocol.h (2)
MessageProcessingResult(622-624)MessageProcessingResult(625-628)
src/chainlock/chainlock.h (5)
src/llmq/signing.h (1)
lastCleanupTime(173-173)src/chainlock/chainlock.cpp (20)
AlreadyHave(95-99)AlreadyHave(95-95)GetChainLockByHash(101-112)GetChainLockByHash(101-101)GetBestChainLock(114-118)GetBestChainLock(114-114)UpdateTxFirstSeenMap(120-127)UpdateTxFirstSeenMap(120-120)ProcessNewChainLock(129-192)ProcessNewChainLock(129-130)AcceptedBlockHeader(194-215)AcceptedBlockHeader(194-194)UpdatedBlockTip(217-236)UpdatedBlockTip(217-217)CheckActiveState(238-252)CheckActiveState(238-238)EnforceBestChainLock(318-375)EnforceBestChainLock(318-318)Cleanup(437-490)Cleanup(437-437)src/chainlock/clsig.h (2)
chainlock(15-38)nodiscard(29-31)src/validationinterface.h (2)
chainlock(28-30)llmq(37-39)src/chainlock/signing.h (1)
llmq(14-20)
🪛 GitHub Actions: Clang Diff Format Check
src/chainlock/chainlock.cpp
[error] 165-171: Clang-format differences found in src/chainlock/chainlock.cpp. Run './contrib/devtools/clang-format-diff.py -p1' (or clang-format) to fix formatting. Failing command: git diff -U0 origin/develop --
src/chainlock/chainlock.h
[error] 76-123: Clang-format differences found in src/chainlock/chainlock.h. Run 'clang-format' to fix formatting. Failing command: git diff -U0 origin/develop --
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linux64_multiprocess-test / Test source
🔇 Additional comments (18)
src/chainlock/clsig.h (1)
39-41: LGTM! Clean API refactor for centralized CLSIG request ID generation.This change properly encapsulates the CLSIG request ID generation by removing the exported constant and replacing it with a centralized function. The approach aligns well with the similar refactoring done for InstantSend input locks.
src/chainlock/signing.h (1)
23-24: LGTM! Well-documented compile-time constant for transaction confirmation threshold.The addition of
TX_CONFIRM_THRESHOLDwith clear documentation improves code maintainability by replacing magic numbers throughout the codebase. The value of 5 blocks is appropriate for the safety window.src/instantsend/lock.h (1)
43-44: LGTM! Template design enables centralized input lock ID generation.The template declaration provides a clean, type-safe interface for generating input lock request IDs. The generic design allows for future extensibility while maintaining type safety through explicit instantiations in the implementation file.
src/instantsend/lock.cpp (2)
14-14: LGTM! Consistent prefix naming convention.The
INPUTLOCK_REQUESTID_PREFIXfollows the same naming pattern established byISLOCK_REQUESTID_PREFIXand uses the modernstd::string_viewtype for efficiency.
41-47: LGTM! Proper template implementation with explicit instantiations.The template implementation correctly uses
SerializeHashwith the prefix-value pair, maintaining consistency with the existing ISLOCK approach. The explicit instantiations forCOutPointandCTxInensure proper linkage while preventing template bloat.src/instantsend/signing.cpp (3)
114-114: LGTM! Centralized ID generation usage.The code correctly uses the new
GenInputLockRequestIdfunction instead of direct serialization, improving maintainability and consistency across the codebase.
298-298: LGTM! Consistent ID generation in signing flow.The usage of
GenInputLockRequestId(in)aligns with the centralized approach and correctly generates IDs for the entireCTxInobject rather than just theprevout.
347-347: LGTM! Proper ID generation for InstantSend lock creation.The code consistently uses
GenInputLockRequestId(in)when checking for recovered signatures, maintaining the same ID generation approach used throughout the signing process.src/protocol.h (1)
20-20: LGTM: variant-based filter and richer MessageProcessingResult payload
- include is correct.
- m_is_masternode, m_inv_filter, and m_request_tx additions align with the new PostProcessMessage flow.
- The constructor with the is_masternode flag is fine and preserves default initialization.
Also applies to: 601-614, 621-629
src/chainlock/chainlock.h (2)
64-67: Chrono migration: verify callers supply seconds (or convert appropriately)txFirstSeenTime and seenChainLocks now store std::chrono::seconds. Ensure all write sites (e.g., UpdateTxFirstSeenMap, ProcessNewChainLock cleanup timers) pass seconds or values convertible to seconds. Header still declares UpdateTxFirstSeenMap taking int64_t; that’s acceptable if the implementation constructs seconds from it, but consistency with seconds at the API boundary would be cleaner in a follow-up.
Would you like me to generate a follow-up patch to change UpdateTxFirstSeenMap to accept std::chrono::seconds across the interface (if it doesn’t break overrides)?
76-123: No clang-format issues detected inchainlock.h
Running clang-format onsrc/chainlock/chainlock.hproduced no changes—no formatting fixes are needed.src/net_processing.h (1)
95-97: LGTM: const-correct RelayInv overloadsSwitching RelayInv to take const CInv& is correct and aligns with the implementations in net_processing.cpp.
src/chainlock/signing.cpp (6)
18-27: LGTM: constructor now stores managers (shareman/spork/mn_sync)Wiring m_shareman, m_sporkman, and m_mn_sync via the ctor is correct and aligns with the broader refactor.
59-61: LGTM: additional null-check for pindexGuarding against null pindex and missing pprev prevents deref and aligns with prior review feedback about null safety.
98-103: LGTM: TX_CONFIRM_THRESHOLD applied; message aligned with logicThe loop now cleanly limits checks to tip + previous TX_CONFIRM_THRESHOLD blocks; the log message reflects the constant.
129-144: LGTM: centralized CLSIG request ID generationSwitching to GenSigRequestId(pindex->nHeight) removes duplication and enforces a single request ID scheme.
195-198: LGTM: GetBlockTxs null-check for LookupBlockIndexReturning early if pindex is missing addresses the null lookup issue noted in prior reviews.
251-263: LGTM: Cleanup handles missing pindex and conflicting/best CLSIGsErasing entries when the index is gone and pruning on ChainLock state prevents stale growth and matches the concurrency/safety expectations.
| @@ -7,7 +7,7 @@ | |||
| #include <tinyformat.h> | |||
|
|
|||
| namespace chainlock { | |||
| const std::string CLSIG_REQUESTID_PREFIX = "clsig"; | |||
| static constexpr std::string_view CLSIG_REQUESTID_PREFIX{"clsig"}; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include <string_view> for CLSIG_REQUESTID_PREFIX
This file uses std::string_view but does not include <string_view>, which will cause a compile error depending on indirect includes.
Add this include (outside the shown range):
#include <string_view>🤖 Prompt for AI Agents
In src/chainlock/clsig.cpp around line 10, the file declares a static constexpr
std::string_view CLSIG_REQUESTID_PREFIX but does not include the <string_view>
header; add the missing include line `#include <string_view>` near the top of
the file (with the other standard headers) so std::string_view is defined and
the file compiles reliably without relying on indirect includes.
| virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Don’t take const rvalue; use rvalue (or const lvalue) to enable/clarify moves
Using const MessageProcessingResult&& prevents moving out of the result and is atypical. Prefer either:
- MessageProcessingResult&& for move semantics, or
- const MessageProcessingResult& if you only read the result.
Given you may want to move vectors (e.g., m_transactions) later, rvalue without const is future-proof.
Apply one of the diffs below (Option A preferred):
Option A (enable moves):
- virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0;
+ virtual void PostProcessMessage(MessageProcessingResult&& ret, NodeId node = -1) = 0;Option B (explicitly no moves):
- virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0;
+ virtual void PostProcessMessage(const MessageProcessingResult& ret, NodeId node = -1) = 0;To verify consistency across declarations/definitions, run:
🏁 Script executed:
#!/bin/bash
# Find all declarations/definitions of PostProcessMessage
rg -n --context 2 $'PostProcessMessage\\(' src | sed -n '1,200p'Length of output: 3922
Enable move semantics by taking a non-const rvalue reference
To allow moving out of the MessageProcessingResult and follow C++ conventions, replace all occurrences of const MessageProcessingResult&& with MessageProcessingResult&&.
Files to update:
- src/net_processing.h (declaration)
- src/net_processing.cpp (override in the class declaration)
- src/net_processing.cpp (definition in
PeerManagerImpl)
Diffs:
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -123,1 +123,1 @@
- virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0;
+ virtual void PostProcessMessage(MessageProcessingResult&& ret, NodeId node = -1) = 0;--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -663,1 +663,1 @@
- void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+ void PostProcessMessage(MessageProcessingResult&& ret, NodeId node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -3500,1 +3500,1 @@
-void PeerManagerImpl::PostProcessMessage(const MessageProcessingResult&& result, NodeId node)
+void PeerManagerImpl::PostProcessMessage(MessageProcessingResult&& result, NodeId node)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0; | |
| - virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0; | |
| + virtual void PostProcessMessage(MessageProcessingResult&& ret, NodeId node = -1) = 0; |
| virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0; | |
| - void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); | |
| + void PostProcessMessage(MessageProcessingResult&& ret, NodeId node) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); |
| virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0; | |
| -void PeerManagerImpl::PostProcessMessage(const MessageProcessingResult&& result, NodeId node) | |
| +void PeerManagerImpl::PostProcessMessage(MessageProcessingResult&& result, NodeId node) |
🤖 Prompt for AI Agents
In src/net_processing.h around lines 123-124, replace the virtual method
signature taking a const rvalue reference with a non-const rvalue reference:
change "const MessageProcessingResult&&" to "MessageProcessingResult&&". Also
update the corresponding override declaration and the implementation in
src/net_processing.cpp (both the class override and the
PeerManagerImpl::PostProcessMessage definition) to use MessageProcessingResult&&
(remove the const), and ensure parameter usage in the implementation permits
moving from the argument where intended.
There was a problem hiding this comment.
@knst, thoughts? Nevermind, diff introduced by this PR. Reverting.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
should revert this line change, see coderabbitai comment for clarification
|
Portions of this PR to be spun off, marking as draft. |
| // Note: not relaying clsig here | ||
| return {}; | ||
| } | ||
|
|
There was a problem hiding this comment.
Just to clarify - it seems as there's possible race introduced, but I don't see any downsides of it.
1st thread: lock cs, update bestChainLockHash to hash_1, unlock cs.
2nd thread lock cs, update bestChainLockHash to hash_2, unlock cs
1st thread checks pindex, updates bestChainLockWithKnownBlock and bestChainLockBlockIndex.
There's no direct dependencies between (bestChainLockHash, bestChainLock) and (bestChainLockWithKnownBlock, bestChainLockBlockIndex), they could be different, right?
| m_chainstate.m_chain.Height() - pindex->nHeight >= 6) { | ||
| // tx got confirmed >= 6 times, so we can stop keeping track of it | ||
| m_chainstate.m_chain.Height() - pindex->nHeight > chainlock::TX_CONFIRM_THRESHOLD) { | ||
| // tx sufficiently deep, we can stop tracking it |
There was a problem hiding this comment.
nit: typo tx IS sufficiently deep
| { | ||
| auto hash = ::SerializeHash(*islock); | ||
|
|
||
| WITH_LOCK(::cs_main, peerman.EraseObjectRequest(pfrom.GetId(), CInv(MSG_ISDLOCK, hash))); |
There was a problem hiding this comment.
yeah, one more usage of PeerManager is gone!
| @@ -597,19 +598,27 @@ using PeerMsgRet = tl::expected<void, MisbehavingError>; | |||
| */ | |||
| struct MessageProcessingResult | |||
| { | |||
| bool m_is_masternode{false}; | |||
There was a problem hiding this comment.
missing const here; this flag is not meant to be changed
| @@ -174,7 +174,7 @@ MessageProcessingResult CInstantSendManager::ProcessMessageInstantSendLock(const | |||
| return ret; | |||
| } | |||
|
|
|||
| bool CInstantSendManager::ProcessPendingInstantSendLocks(PeerManager& peerman) | |||
| bool CInstantSendManager::ProcessPendingInstantSendLocks(std::vector<std::pair<NodeId, MessageProcessingResult>>& peer_activity) | |||
There was a problem hiding this comment.
consider creating peer_activity to be a returned value.
struct InstantSendManagerPendingHelper
{
std::vector<std::pair<NodeId, MessageProcessingResult>> m_peer_activity;
bool m_more_work{false};
};
it should replace fMoreWork flag usages.
Motivation: functions preferrable should return value by return rather than arguments
There was a problem hiding this comment.
IMO, commit bc9e919 refactor: reduce PeerManager usage in InstantSend worker functions is a bit controversial, I am not sure if the implementation will get easier like that or better.
I can't suggest right now any better solution, but it's a bit messy as it is right now. maybe this particular change worth own PR
| virtual void PostProcessMessage(const MessageProcessingResult&& ret, NodeId node = -1) = 0; | ||
|
|
There was a problem hiding this comment.
should revert this line change, see coderabbitai comment for clarification
| @@ -419,15 +419,17 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm | |||
| mempool.AddTransactionsUpdated(1); | |||
| } | |||
|
|
|||
| MessageProcessingResult ret{/*is_masternode=*/m_signer != nullptr}; | |||
There was a problem hiding this comment.
PeerManager knows already about is_masternode or not; see implementation:
const bool is_masternode = m_mn_activeman != nullptr;
Consider refactoring RequestObject, AskPeersForTransaction instead to drop this flag completely and simplify logic. Consider to move this commit and next a7f0a76 and related refactorings to new PR, they seems a bit out-of-scope
Additional Information
Depends on refactor: move ChainLocks impl. to
src/chainlock/and split masternode logic out of manager source #6761Assumptions surrounding
pindexusage have been documented in response to reviewer comments in dash#6761 (comment, comment).The internal structures of
CChainLocksHandler(txFirstSeenTime,seenChainLocks,lastCleanupTime) have their time resolution reduced from milliseconds to seconds while migrating tostd::chrono.CInstantSendManager::AskNodesForLockedTx()was moved asPeerManagerImpl::AskPeersForTransaction()in dash#6425 and with that, the sole external usage ofPeerManagerImpl::IsInvInFilter()was moved internally, we can therefore safely remove it from thePeerManagerinterface.While this pull request doesn't entirely decouple
PeerManagerfromCInstantSendManageras the worker thread isn't reactive (i.e. triggered byProcessMessage()), we can reduce its usage by folding all expected network activity into aMessageProcessingResultbatch that can be consumed byPostProcessMessage.To achieve this,
MessageProcessingResulthas been extended to allowm_request_tx)m_inv_filter)This has the additional effect of removing external uses of
RelayInvFiltered()andAskPeersForTransaction(), which can now be safely removed from thePeerManagerinterface as it is now brokered throughMessageProcessingResult.Note that while the old
RelayInvFiltered()allowed specifying any minimum protocol version,PostProcessMessage()will assume thatminProtoVersionisISDLOCK_PROTO_VERSION. This is acceptable asRelayInvFiltered()isn't used anywhere else and deterministic InstantSend locks were introduced in v0.18 (see dash#4381).If there is any foreseeable use needing to relay messages to even older versions of Dash Core,
m_inv_filtercan be modified to accommodate that.Breaking Changes
None expected.
Checklist