refactor: separate network and consensus logic 1/N [instantsend] #6959
Conversation
|
WalkthroughThis change factors InstantSend networking into a new NetInstantSend NetHandler and removes worker-thread responsibilities from CInstantSendManager. PeerManager gains a handler infrastructure (AddExtraHandler/RemoveHandlers/StartHandlers/StopHandlers/InterruptHandlers) to host external NetHandler instances. InstantSend APIs were simplified: worker lifecycle methods and CQuorumManager dependency removed; pending-lock enqueueing, fetching, and processing APIs were added and ProcessInstantSendLock now returns a variant. MessageProcessingResult had two fields removed. Build and init were updated to include and wire net_instantsend, and PeerManager lifecycle calls were extended to manage handlers. Sequence Diagram(s)sequenceDiagram
participant Init as Initialization
participant PM as PeerManager
participant NIS as NetInstantSend
participant ISM as CInstantSendManager
participant Signer as Signer
Init->>PM: AddExtraHandler(NetInstantSend)
PM->>NIS: (store in m_handlers)
Init->>PM: StartHandlers()
PM->>NIS: Start()
NIS->>NIS: Start WorkThread
NIS->>Signer: Start signer if available
Note over PM,NIS: Message Processing
PM->>PM: Core ProcessMessage
PM->>NIS: Delegate ProcessMessage
NIS->>ISM: EnqueueInstantSendLock(from, txid, islock)
ISM->>ISM: Store in m_pending_is
Note over NIS: Worker loop
NIS->>ISM: FetchPendingLocks()
ISM-->>NIS: PendingState
NIS->>NIS: ProcessPendingISLocks()
NIS->>ISM: ProcessInstantSendLock(...)
ISM-->>NIS: variant<txid, tx, monostate>
Init->>PM: StopHandlers()
PM->>NIS: Stop()
NIS->>Signer: Stop signer
NIS->>NIS: Join WorkThread
Init->>PM: RemoveHandlers()
sequenceDiagram
participant App as Application
participant Init as init.cpp
participant PM as PeerManager
participant ISM as CInstantSendManager
participant NIS as NetInstantSend
App->>Init: AppInitMain()
Note over Init,ISM: Old Flow (internal thread)
Init->>ISM: Create CInstantSendManager
Note over ISM: Start/Stop methods existed (removed)
Note over Init,NIS: New Flow (handler)
Init->>ISM: Create CInstantSendManager (no Start)
Init->>NIS: Create NetInstantSend(ISM)
Init->>PM: AddExtraHandler(NetInstantSend)
Init->>PM: StartHandlers()
PM->>NIS: Start()
NIS->>NIS: Start worker thread internally
App->>Init: Shutdown
Init->>PM: StopHandlers()
PM->>NIS: Stop()
NIS->>NIS: Join worker thread
Init->>PM: RemoveHandlers()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (2)
src/net_processing.cpp (2)
5441-5443: Let external handlers see unknown messages, too.Currently handlers are only invoked in the “known message” path. If an extension defines a new msg type not present in getAllNetMessageTypes(), it will be logged as unknown and never delivered.
Call handlers before logging in the unknown branch.
- for (const auto& handler : m_handlers) { - handler->ProcessMessage(pfrom, msg_type, vRecv); - } + for (const auto& handler : m_handlers) handler->ProcessMessage(pfrom, msg_type, vRecv); return; } // Ignore unknown commands for extensibility - LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId()); + for (const auto& handler : m_handlers) handler->ProcessMessage(pfrom, msg_type, vRecv); + LogPrint(BCLog::NET, "Unknown command \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
6520-6548: Misc: add missing cs_main lock; align wrapper semantics.As noted above, PeerEraseObjectRequest must lock cs_main internally. Also consider adding brief comments to wrappers clarifying locking expectations for external callers.
void PeerManagerImpl::PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message) { Misbehaving(pnode, howmuch, message); } void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) { - EraseObjectRequest(nodeid, inv); + WITH_LOCK(::cs_main, EraseObjectRequest(nodeid, inv)); } void PeerManagerImpl::PeerRelayInv(const CInv& inv) { RelayInv(inv); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/Makefile.am(2 hunks)src/init.cpp(5 hunks)src/instantsend/instantsend.cpp(8 hunks)src/instantsend/instantsend.h(5 hunks)src/instantsend/net_instantsend.cpp(1 hunks)src/instantsend/net_instantsend.h(1 hunks)src/llmq/context.cpp(1 hunks)src/msg_result.h(0 hunks)src/net_processing.cpp(5 hunks)src/net_processing.h(2 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
💤 Files with no reviewable changes (1)
- src/msg_result.h
🧰 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/llmq/context.cppsrc/net_processing.cppsrc/init.cppsrc/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/net_processing.hsrc/instantsend/instantsend.h
🧠 Learnings (8)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Learnt from: kwvg
Repo: dashpay/dash PR: 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: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/llmq/context.cppsrc/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/instantsend.h
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.cppsrc/init.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.cpp
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/init.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/init.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.h
🧬 Code graph analysis (6)
src/net_processing.cpp (2)
src/validationinterface.cpp (4)
RegisterValidationInterface(133-138)RegisterValidationInterface(133-133)UnregisterValidationInterface(145-150)UnregisterValidationInterface(145-145)src/llmq/dkgsessionhandler.cpp (2)
Misbehaving(112-116)Misbehaving(112-112)
src/instantsend/instantsend.cpp (3)
src/logging.h (1)
LogAcceptDebug(235-238)src/net_processing.cpp (4)
hash(1019-1019)hash(1025-1025)ret(676-676)tx(1077-1077)src/node/interfaces.cpp (18)
hash(893-910)hash(893-893)hash(932-936)hash(932-932)hash(958-963)hash(958-958)tx(649-652)tx(649-649)tx(772-775)tx(772-772)tx(776-779)tx(776-776)tx(797-800)tx(797-797)tx(1042-1049)tx(1042-1042)tx(1061-1076)tx(1061-1061)
src/instantsend/net_instantsend.cpp (3)
src/net_processing.cpp (24)
ProcessMessage(3660-5451)ProcessMessage(3660-3665)pfrom(613-613)pfrom(634-635)pfrom(744-746)pfrom(755-755)pfrom(763-763)pfrom(766-766)pfrom(768-768)pfrom(770-770)pfrom(866-866)pfrom(1048-1048)id(694-694)id(698-698)ret(676-676)inv(627-627)inv(628-628)inv(651-651)inv(652-652)inv(653-653)inv(662-662)inv(668-668)inv(956-956)inv(2378-2378)src/instantsend/net_instantsend.h (1)
Start(35-37)src/llmq/context.cpp (4)
Start(50-55)Start(50-50)Stop(57-62)Stop(57-57)
src/instantsend/net_instantsend.h (4)
src/instantsend/instantsend.h (1)
instantsend(38-44)src/instantsend/instantsend.cpp (2)
CInstantSendManager(51-62)CInstantSendManager(64-64)src/net_processing.h (1)
PeerManagerInternal(58-67)src/instantsend/net_instantsend.cpp (12)
ProcessMessage(17-65)ProcessMessage(17-17)Start(67-79)Start(67-67)Stop(81-95)Stop(81-81)WorkThreadMain(248-268)WorkThreadMain(248-248)ProcessPendingISLocks(221-246)ProcessPendingISLocks(221-221)ProcessPendingInstantSendLocks(97-218)ProcessPendingInstantSendLocks(97-99)
src/net_processing.h (1)
src/net_processing.cpp (67)
PeerMisbehaving(6520-6523)PeerMisbehaving(6520-6520)pnode(633-633)pnode(638-638)pnode(649-649)pnode(730-730)pnode(774-774)pnode(846-846)pnode(848-848)PeerEraseObjectRequest(6525-6528)PeerEraseObjectRequest(6525-6525)nodeid(623-623)nodeid(626-626)nodeid(639-639)nodeid(650-650)nodeid(670-670)nodeid(672-672)nodeid(710-711)nodeid(721-721)nodeid(853-853)nodeid(855-855)nodeid(1031-1031)nodeid(1066-1066)inv(627-627)inv(628-628)inv(651-651)inv(652-652)inv(653-653)inv(662-662)inv(668-668)inv(956-956)inv(2378-2378)PeerRelayInv(6530-6533)PeerRelayInv(6530-6530)PeerRelayInvFiltered(6535-6538)PeerRelayInvFiltered(6535-6535)PeerRelayInvFiltered(6540-6543)PeerRelayInvFiltered(6540-6540)PeerAskPeersForTransaction(6545-6548)PeerAskPeersForTransaction(6545-6545)txid(629-629)txid(654-654)txid(656-656)txid(659-659)ProcessMessage(3660-5451)ProcessMessage(3660-3665)pfrom(613-613)pfrom(634-635)pfrom(744-746)pfrom(755-755)pfrom(763-763)pfrom(766-766)pfrom(768-768)pfrom(770-770)pfrom(866-866)pfrom(1048-1048)AddExtraHandler(1645-1652)AddExtraHandler(1645-1645)handler(642-642)RemoveHandlers(1654-1659)RemoveHandlers(1654-1654)StartHandlers(1661-1666)StartHandlers(1661-1661)StopHandlers(1668-1676)StopHandlers(1668-1668)InterruptHandlers(1678-1683)InterruptHandlers(1678-1678)
src/instantsend/instantsend.h (3)
src/instantsend/net_instantsend.h (1)
instantsend(12-15)src/instantsend/lock.h (1)
instantsend(18-41)src/instantsend/instantsend.cpp (10)
CInstantSendManager(51-62)CInstantSendManager(64-64)FetchPendingLocks(92-119)FetchPendingLocks(92-92)EnqueueInstantSendLock(68-90)EnqueueInstantSendLock(68-69)PrepareTxToRetry(367-383)PrepareTxToRetry(367-367)ProcessInstantSendLock(121-195)ProcessInstantSendLock(121-122)
⏰ 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). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: Lint / Run linters
| void Start() override; | ||
| void Stop() override; | ||
| void Interrupt() override { workInterrupt(); }; |
There was a problem hiding this comment.
Stop() must self-interrupt before joining the worker
PeerManagerImpl::StopHandlers() (see src/net_processing.cpp Line 1667) calls handler->Stop() without guaranteeing that Interrupt() ran first. As implemented, NetInstantSend::Stop() asserts on !workInterrupt and then waits on workThread.join(). If Interrupt() was skipped—e.g. in an early shutdown/error path—the worker loop never exits (it keeps waiting on workInterrupt), so release builds hang indefinitely and debug builds hit the assert. Please make Stop() idempotent by signalling the interrupt internally (or otherwise ensuring the worker stops) before calling join().
void NetInstantSend::Stop()
{
- if (!workInterrupt) {
- assert(false);
- }
+ if (!workInterrupt) {
+ workInterrupt();
+ }
if (workThread.joinable()) {
workThread.join();
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/instantsend/net_instantsend.h around lines 35 to 37, Stop() currently
asserts that workInterrupt was already signalled and then joins the worker,
which can hang if Interrupt() wasn't called; modify Stop() to be idempotent by
signalling the interrupt internally (call or set workInterrupt) before waiting,
avoid asserting on workInterrupt, and only join the worker if the thread is
joinable; this ensures Stop() self-interrupts the worker loop and safely joins
without relying on an external Interrupt() call.
| /** Implements external handlers logic */ | ||
| void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) override; | ||
| void RemoveHandlers() override; | ||
| void StartHandlers() override; | ||
| void StopHandlers() override; | ||
| void InterruptHandlers() override; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
External handler API lacks synchronization contract; concurrent mutation risks UAF.
AddExtraHandler/RemoveHandlers/StartHandlers/StopHandlers/InterruptHandlers can run concurrently with ProcessMessage iterating m_handlers (under g_msgproc_mutex). Without a lock requirement, RemoveHandlers() can clear while iterating, causing a data race/UAF.
Either:
- Require callers to hold g_msgproc_mutex (with annotations), or
- Internally lock around m_handlers with g_msgproc_mutex (preferred for safety).
Suggested minimal contract via annotations (plus guarding usage in impl below).
- void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) override;
- void RemoveHandlers() override;
- void StartHandlers() override;
- void StopHandlers() override;
- void InterruptHandlers() override;
+ void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) override EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
+ void RemoveHandlers() override EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
+ void StartHandlers() override EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
+ void StopHandlers() override EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
+ void InterruptHandlers() override EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 641 to 647, the external handler API
methods
(AddExtraHandler/RemoveHandlers/StartHandlers/StopHandlers/InterruptHandlers)
mutate m_handlers while ProcessMessage iterates it under g_msgproc_mutex,
risking data races/UAF; to fix, protect all accesses and mutations of m_handlers
by taking g_msgproc_mutex inside these methods (preferred) or add thread-safety
annotations requiring callers to hold g_msgproc_mutex; update each method
implementation to lock g_msgproc_mutex (std::lock_guard or equivalent) around
operations on m_handlers and add a comment/annotation on the declaration
indicating the mutex that guards m_handlers so callers/linters are aware.
| /** Implement PeerManagerInternal */ | ||
| void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override; | ||
| void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override; | ||
| void PeerRelayInv(const CInv& inv) override; | ||
| void PeerRelayInvFiltered(const CInv& inv, const CTransaction& relatedTx) override; | ||
| void PeerRelayInvFiltered(const CInv& inv, const uint256& relatedTxHash) override; | ||
| void PeerAskPeersForTransaction(const uint256& txid) override; | ||
| private: |
There was a problem hiding this comment.
Wrapper methods must honor underlying lock requirements.
PeerEraseObjectRequest forwards to EraseObjectRequest which requires cs_main. The wrapper does not lock, inviting races if handlers call it directly.
Lock inside the wrapper to keep external API simple and safe.
- void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override;
+ void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override;And in the definition (see lines 6525-6529 below):
-void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv)
-{
- EraseObjectRequest(nodeid, inv);
-}
+void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv)
+{
+ WITH_LOCK(::cs_main, EraseObjectRequest(nodeid, inv));
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 648-655, the PeerEraseObjectRequest
wrapper must acquire cs_main before forwarding to EraseObjectRequest because the
underlying function requires cs_main; modify the wrapper to lock cs_main (e.g.
use LOCK(cs_main) or the project’s scoped lock pattern) for the duration of the
call to EraseObjectRequest so callers of the wrapper cannot race, and ensure the
lock covers any access needed by the underlying call (no other API changes
required).
| std::vector<std::unique_ptr<NetHandler>> m_handlers; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard m_handlers with a mutex (or document it).
m_handlers is accessed from ProcessMessage under g_msgproc_mutex, but lifecycle methods don’t lock. Mark it as guarded to aid static analysis and clarity.
- std::vector<std::unique_ptr<NetHandler>> m_handlers;
+ std::vector<std::unique_ptr<NetHandler>> m_handlers GUARDED_BY(g_msgproc_mutex);📝 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.
| std::vector<std::unique_ptr<NetHandler>> m_handlers; | |
| }; | |
| std::vector<std::unique_ptr<NetHandler>> m_handlers GUARDED_BY(g_msgproc_mutex); | |
| }; |
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 1086-1087, the member m_handlers is
accessed under g_msgproc_mutex in ProcessMessage but its declaration and
lifecycle methods aren’t marked or protected; either add an explicit mutex to
guard m_handlers (e.g., std::mutex m_handlers_mutex) and acquire it in all
lifecycle methods that mutate or access m_handlers, or annotate the existing
member with the appropriate thread-safety annotation/comment (e.g., // Guarded
by g_msgproc_mutex or GUARDED_BY(g_msgproc_mutex)) and ensure all code paths
consistently lock g_msgproc_mutex when touching m_handlers.
| void PeerManagerImpl::RemoveHandlers() | ||
| { | ||
| InterruptHandlers(); | ||
| StopHandlers(); | ||
| m_handlers.clear(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Clear handlers safely under lock; avoid nested locks.
Take g_msgproc_mutex once, interrupt/stop/unregister, then clear, to prevent concurrent iteration and to avoid nesting Stop/Interrupt that could re-lock.
void PeerManagerImpl::RemoveHandlers()
{
- InterruptHandlers();
- StopHandlers();
- m_handlers.clear();
+ LOCK(g_msgproc_mutex);
+ for (auto& h : m_handlers) h->Interrupt();
+ for (auto& h : m_handlers) {
+ if (auto i = dynamic_cast<CValidationInterface*>(h.get()); i != nullptr) {
+ UnregisterValidationInterface(i);
+ }
+ h->Stop();
+ }
+ m_handlers.clear();
}🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 1654 to 1660, calling
InterruptHandlers(), StopHandlers(), and then m_handlers.clear() risks nested
locking and concurrent iteration; acquire g_msgproc_mutex once at the start,
perform the interrupt/stop/unregister operations while holding that lock, then
clear m_handlers while still holding it; if InterruptHandlers/StopHandlers
currently re-acquire g_msgproc_mutex, refactor them to expose non-locking
internal helpers (or inline their core actions here) so the sequence run under a
single lock and avoid nested locks and race conditions.
| void PeerManagerImpl::StartHandlers() | ||
| { | ||
| for (auto& handler : m_handlers) { | ||
| handler->Start(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
StartHandlers should lock around traversal.
Protect iteration with g_msgproc_mutex to avoid races with Remove/Add.
void PeerManagerImpl::StartHandlers()
{
- for (auto& handler : m_handlers) {
- handler->Start();
- }
+ LOCK(g_msgproc_mutex);
+ for (auto& handler : m_handlers) handler->Start();
}🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 1661 to 1667, the StartHandlers() method
currently iterates m_handlers without synchronization; acquire g_msgproc_mutex
before the for-loop and release it after the traversal so the iteration is
protected from concurrent Remove/Add calls, i.e. lock g_msgproc_mutex at the
start of StartHandlers(), iterate calling handler->Start(), then unlock when
done to avoid races.
| void PeerManagerImpl::StopHandlers() | ||
| { | ||
| for (auto& handler : m_handlers) { | ||
| if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { | ||
| UnregisterValidationInterface(i); | ||
| } | ||
| handler->Stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
StopHandlers should lock and unregister before stopping (you already do).
Also guard traversal with g_msgproc_mutex for safety.
void PeerManagerImpl::StopHandlers()
{
- for (auto& handler : m_handlers) {
- if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) {
- UnregisterValidationInterface(i);
- }
- handler->Stop();
- }
+ LOCK(g_msgproc_mutex);
+ for (auto& handler : m_handlers) {
+ if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) {
+ UnregisterValidationInterface(i);
+ }
+ handler->Stop();
+ }
}📝 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.
| void PeerManagerImpl::StopHandlers() | |
| { | |
| for (auto& handler : m_handlers) { | |
| if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { | |
| UnregisterValidationInterface(i); | |
| } | |
| handler->Stop(); | |
| } | |
| } | |
| void PeerManagerImpl::StopHandlers() | |
| { | |
| LOCK(g_msgproc_mutex); | |
| for (auto& handler : m_handlers) { | |
| if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { | |
| UnregisterValidationInterface(i); | |
| } | |
| handler->Stop(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 1668 to 1676, protect traversal of
m_handlers with g_msgproc_mutex, perform the dynamic_cast and call
UnregisterValidationInterface while holding that lock, then release the lock and
call handler->Stop() (do not call Stop while holding g_msgproc_mutex). Ensure
the lock covers the iteration/unregistration only and that Stop() is invoked
afterwards to avoid deadlocks.
| void PeerManagerImpl::InterruptHandlers() | ||
| { | ||
| for (auto& handler : m_handlers) { | ||
| handler->Interrupt(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
InterruptHandlers should lock during traversal.
Same race consideration.
void PeerManagerImpl::InterruptHandlers()
{
- for (auto& handler : m_handlers) {
- handler->Interrupt();
- }
+ LOCK(g_msgproc_mutex);
+ for (auto& handler : m_handlers) handler->Interrupt();
}📝 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.
| void PeerManagerImpl::InterruptHandlers() | |
| { | |
| for (auto& handler : m_handlers) { | |
| handler->Interrupt(); | |
| } | |
| } | |
| void PeerManagerImpl::InterruptHandlers() | |
| { | |
| LOCK(g_msgproc_mutex); | |
| for (auto& handler : m_handlers) handler->Interrupt(); | |
| } |
🤖 Prompt for AI Agents
In src/net_processing.cpp around lines 1678 to 1684, InterruptHandlers iterates
m_handlers without holding the mutex protecting that container; acquire the
appropriate lock (the mutex that guards m_handlers, e.g. m_mutex or
m_handlers_mutex) before traversing and calling handler->Interrupt(). Use a
scoped lock (std::lock_guard or std::scoped_lock) to protect the loop, or
alternatively copy m_handlers under the lock and then call Interrupt() outside
the lock if handler callbacks may reenter to avoid deadlocks.
| "chainlock/chainlock -> chainlock/signing -> llmq/signing -> net_processing -> chainlock/chainlock", | ||
| "chainlock/chainlock -> chainlock/signing -> llmq/signing -> net_processing -> masternode/active/context -> chainlock/chainlock", | ||
| "chainlock/chainlock -> instantsend/instantsend -> instantsend/signing -> chainlock/chainlock", | ||
| "chainlock/chainlock -> instantsend/instantsend -> net_processing -> chainlock/chainlock", | ||
| "chainlock/chainlock -> instantsend/instantsend -> net_processing -> masternode/active/context -> chainlock/chainlock", | ||
| "chainlock/chainlock -> llmq/quorums -> msg_result -> coinjoin/coinjoin -> chainlock/chainlock", | ||
| "chainlock/chainlock -> validation -> chainlock/chainlock", | ||
| "chainlock/chainlock -> validation -> evo/chainhelper -> chainlock/chainlock", | ||
| "chainlock/signing -> instantsend/instantsend -> net_processing -> masternode/active/context -> chainlock/signing", | ||
| "coinjoin/client -> coinjoin/coinjoin -> instantsend/instantsend -> net_processing -> coinjoin/walletman -> coinjoin/client", | ||
| "coinjoin/coinjoin -> instantsend/instantsend -> llmq/quorums -> msg_result -> coinjoin/coinjoin", | ||
| "chainlock/signing -> llmq/signing -> net_processing -> masternode/active/context -> chainlock/signing", | ||
| "coinjoin/coinjoin -> instantsend/instantsend -> spork -> msg_result -> coinjoin/coinjoin", | ||
| "coinjoin/client -> core_io -> evo/mnhftx -> llmq/signing -> net_processing -> coinjoin/walletman -> coinjoin/client", | ||
| "coinjoin/server -> net_processing -> coinjoin/server", |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure updated cycles exactly match the tool’s output format.
The new chains you added look consistent with the refactor (routing via llmq/signing and net_processing), but this list is string-compared against contrib/devtools/circular-dependencies.py output. A single component name mismatch or order variance will break the lint.
Recommend verifying against the script output and trimming any obsolete entries to keep the list minimal.
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# From repo root
python3 test/lint/lint-circular-dependencies.py || true
# Also show raw discovered cycles to compare visually
pushd src >/dev/null
python3 ../contrib/devtools/circular-dependencies.py $(git ls-files '*.h' '*.cpp') | sed 's/^Circular dependency: //'
popd >/dev/nullLength of output: 20887
Remove all added entries—they're resolved, not new dependencies.
The lint test output confirms all the entries you added (lines 26-35) have actually been resolved by your refactoring, not introduced as new cycles needing exceptions. The script output explicitly states each one "is no longer present" and requests removal from EXPECTED_CIRCULAR_DEPENDENCIES. Your changes successfully eliminated these cycles rather than requiring new exception entries—remove all of them from the file.
🤖 Prompt for AI Agents
In test/lint/lint-circular-dependencies.py around lines 26 to 35, you added
several dependency-cycle entries that the lint output says are "no longer
present"; remove all the entries you added in that range from the
EXPECTED_CIRCULAR_DEPENDENCIES list so the file matches the linter output and no
longer documents resolved cycles.
There was a problem hiding this comment.
The lint test output confirms all the entries you added (lines 26-35) have actually been resolved by your refactoring, not introduced as new cycles needing exceptions
incorrect; if linter is quiet it means that added entities must be added; linter warns about both : extra and missing records.
There was a problem hiding this comment.
@knst, you're absolutely right, and I apologize for the confusion. I misread the linter behavior.
You've correctly pointed out that when the linter is quiet (no errors), it means:
- The added entries are new circular dependencies that were introduced by this refactoring and must be documented in the expected list
- The removed entries are circular dependencies that were eliminated by the refactoring
The changes you made properly reflect the actual state:
- ✅ New cycles added through
chainlock/signing -> llmq/signing -> net_processingpaths - ✅ Old cycles removed that went through
instantsend/instantsend -> net_processingpaths
This is the correct approach for dependency restructuring - you've successfully rerouted the dependencies through different components, which naturally resolves some cycles while introducing new ones through the alternate paths. The lint file correctly documents this new reality.
Thank you for the clarification!
/\_/\
( o.o )
> ^ < *learns something new*
🧠 Learnings used
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Learnt from: kwvg
Repo: dashpay/dash PR: 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: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
knst
force-pushed
the
refactor-peermanager-handlers-is
branch
2 times, most recently
from
71f8cbd to
030539a
Compare
How to disable this feature? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@knst, let me fetch the current CodeRabbit configuration to provide you with accurate guidance on how to disable the automatic review request feature. 🛠 Debug Information Gathered @knst, based on your current CodeRabbit configuration, I can see that automatic reviewer assignment is actually already disabled. Here are the two relevant settings: Current Configuration: reviews:
suggested_reviewers: true # Suggests reviewers in the walkthrough comment
auto_assign_reviewers: false # Actually requests reviews from suggested reviewersSince To completely disable reviewer-related features, add or update these settings in your reviews:
suggested_reviewers: false
auto_assign_reviewers: falseThis will:
Note: If you're seeing CodeRabbit requesting reviews despite
You can place the 🧠 Learnings used |
…ogic and consensus
…nstead and tidy up them a bit
knst
force-pushed
the
refactor-peermanager-handlers-is
branch
from
030539a to
cf2d6a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
src/instantsend/net_instantsend.cpp (1)
136-143: Take cs_main before reading the active chain.
m_chainstate.m_chain.Height()and the subsequentSelectQuorumForSigning(...)call both requirecs_main. Doing them without the lock lets this worker race the validation threads, which is undefined behaviour (and it’s the same race we flagged previously). Please grabcs_mainwhile computing the height and picking the quorum.- if (blockIndex->nHeight + dkgInterval < m_chainstate.m_chain.Height()) { - nSignHeight = blockIndex->nHeight + dkgInterval - 1; - } - // For RegTest non-rotating quorum cycleHash has directly quorum hash - auto quorum = llmq_params.useRotation ? llmq::SelectQuorumForSigning(llmq_params, m_chainstate.m_chain, m_qman, - id, nSignHeight, signOffset) - : m_qman.GetQuorum(llmq_params.type, islock->cycleHash); + std::shared_ptr<const llmq::CQuorum> quorum; + { + LOCK(::cs_main); + if (blockIndex->nHeight + dkgInterval < m_chainstate.m_chain.Height()) { + nSignHeight = blockIndex->nHeight + dkgInterval - 1; + } + // For RegTest non-rotating quorum cycleHash has directly quorum hash + quorum = llmq_params.useRotation ? llmq::SelectQuorumForSigning( + llmq_params, m_chainstate.m_chain, m_qman, id, nSignHeight, signOffset) + : m_qman.GetQuorum(llmq_params.type, islock->cycleHash); + }src/net_processing.cpp (10)
641-647: External handler API: define a clear locking contract (protect m_handlers).These lifecycle methods mutate/iterate m_handlers but have no declared synchronization. ProcessMessage iterates m_handlers under g_msgproc_mutex, so unsynchronized mutation here risks data races/UAF. Declare that these methods lock internally (LOCKS_EXCLUDED) and then implement locking around mutations/reads.
Apply annotations to match the locking intent:
- void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) override; - void RemoveHandlers() override; - void StartHandlers() override; - void StopHandlers() override; - void InterruptHandlers() override; + void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) override LOCKS_EXCLUDED(g_msgproc_mutex); + void RemoveHandlers() override LOCKS_EXCLUDED(g_msgproc_mutex); + void StartHandlers() override LOCKS_EXCLUDED(g_msgproc_mutex); + void StopHandlers() override LOCKS_EXCLUDED(g_msgproc_mutex); + void InterruptHandlers() override LOCKS_EXCLUDED(g_msgproc_mutex);
648-655: Wrapper API: PeerEraseObjectRequest must lock cs_main internally (and annotation should reflect it).The declaration requires cs_main, but the definition doesn’t lock, inviting misuse by external handlers. Make the wrapper self‑contained: acquire cs_main inside and mark the declaration LOCKS_EXCLUDED(cs_main).
- void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override LOCKS_EXCLUDED(::cs_main);
1086-1086: Guard m_handlers with g_msgproc_mutex.m_handlers is traversed under g_msgproc_mutex in ProcessMessage but declared unguarded. Add annotation to aid static analysis and document the contract.
- std::vector<std::unique_ptr<NetHandler>> m_handlers; + std::vector<std::unique_ptr<NetHandler>> m_handlers GUARDED_BY(g_msgproc_mutex);
1668-1676: StopHandlers: guard traversal; unregister before stop; call outside lock.Mirror the snapshot pattern to avoid concurrent mutation and reduce deadlock potential.
-void PeerManagerImpl::StopHandlers() -{ - for (auto& handler : m_handlers) { - if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { - UnregisterValidationInterface(i); - } - handler->Stop(); - } -} +void PeerManagerImpl::StopHandlers() +{ + std::vector<NetHandler*> handlers; + { + LOCK(g_msgproc_mutex); + handlers.reserve(m_handlers.size()); + for (auto& h : m_handlers) handlers.push_back(h.get()); + } + for (auto* h : handlers) { + if (auto i = dynamic_cast<CValidationInterface*>(h); i != nullptr) { + UnregisterValidationInterface(i); + } + h->Stop(); + } +}
1678-1684: InterruptHandlers: guard traversal with g_msgproc_mutex; call outside lock.Same race consideration.
-void PeerManagerImpl::InterruptHandlers() -{ - for (auto& handler : m_handlers) { - handler->Interrupt(); - } -} +void PeerManagerImpl::InterruptHandlers() +{ + std::vector<NetHandler*> handlers; + { + LOCK(g_msgproc_mutex); + handlers.reserve(m_handlers.size()); + for (auto& h : m_handlers) handlers.push_back(h.get()); + } + for (auto* h : handlers) h->Interrupt(); +}
6525-6529: Lock cs_main inside PeerEraseObjectRequest wrapper.Acquire cs_main within the wrapper to match the safe external API. Adjusted declaration is in an earlier comment.
-void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) -{ - EraseObjectRequest(nodeid, inv); -} +void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) +{ + WITH_LOCK(::cs_main, EraseObjectRequest(nodeid, inv)); +}
1654-1660: RemoveHandlers can race with ProcessMessage; clear under lock, stop outside.Swap m_handlers into a local vector under g_msgproc_mutex, then interrupt/stop/unregister outside the lock to avoid holding the global message lock while invoking external code. This preserves safety and reduces deadlock risk.
-void PeerManagerImpl::RemoveHandlers() -{ - InterruptHandlers(); - StopHandlers(); - m_handlers.clear(); -} +void PeerManagerImpl::RemoveHandlers() +{ + std::vector<std::unique_ptr<NetHandler>> handlers; + { + LOCK(g_msgproc_mutex); + handlers.swap(m_handlers); + } + for (auto& h : handlers) h->Interrupt(); + for (auto& h : handlers) { + if (auto i = dynamic_cast<CValidationInterface*>(h.get()); i != nullptr) { + UnregisterValidationInterface(i); + } + h->Stop(); + } + // handlers goes out of scope here +}
1661-1666: StartHandlers should not iterate m_handlers unsynchronized.Take a snapshot under g_msgproc_mutex, then start outside the lock.
-void PeerManagerImpl::StartHandlers() -{ - for (auto& handler : m_handlers) { - handler->Start(); - } -} +void PeerManagerImpl::StartHandlers() +{ + std::vector<NetHandler*> handlers; + { + LOCK(g_msgproc_mutex); + handlers.reserve(m_handlers.size()); + for (auto& h : m_handlers) handlers.push_back(h.get()); + } + for (auto* h : handlers) h->Start(); +}
1645-1652: AddExtraHandler needs synchronization and consistent assertion macro.
- Use Assert (project style) and guard the container mutation with g_msgproc_mutex.
- RegisterValidationInterface should not run while holding g_msgproc_mutex; snapshot the raw pointer, add under lock, then register out of lock.
-void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler) -{ - assert(handler != nullptr); - if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { - RegisterValidationInterface(i); - } - m_handlers.emplace_back(std::move(handler)); -} +void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler) +{ + Assert(handler != nullptr); + CValidationInterface* val_if = dynamic_cast<CValidationInterface*>(handler.get()); + { + LOCK(g_msgproc_mutex); + m_handlers.emplace_back(std::move(handler)); + } + if (val_if != nullptr) { + RegisterValidationInterface(val_if); + } +}
641-647: m_handlers requires GUARDED_BY annotation and lock acquisition in accessor functions.The verification confirms an actual thread-safety issue:
m_handlers(line 1086) lacks synchronization despite similar members in the same class being protected withGUARDED_BY(g_msgproc_mutex). All six access points (AddExtraHandler, RemoveHandlers, StartHandlers, StopHandlers, InterruptHandlers, and line 5441) operate without visible lock acquisition. The inconsistency with guarded siblings and lack of mutex protection in call contexts (init.cpp lines 255, 292, 2201, 2297) indicates m_handlers should either be annotated withGUARDED_BY(g_msgproc_mutex)or have explicitLOCK(g_msgproc_mutex)guards in each function.
🧹 Nitpick comments (2)
src/instantsend/instantsend.cpp (1)
92-119: Consider extracting magic number to named constant.The method logic is correct and thread-safe. However, the value
32on line 99 could be extracted as a named constant for better maintainability, despite the explanatory comment.Apply this diff:
+static constexpr size_t MAX_PENDING_LOCKS_PER_BATCH = 32; + instantsend::PendingState CInstantSendManager::FetchPendingLocks() { instantsend::PendingState ret; LOCK(cs_pendingLocks); // only process a max 32 locks at a time to avoid duplicate verification of recovered signatures which have been // verified by CSigningManager in parallel - const size_t maxCount = 32; + const size_t maxCount = MAX_PENDING_LOCKS_PER_BATCH;src/net_processing.cpp (1)
1647-1647: Use Assert instead of assert for consistency.Project code predominantly uses Assert/Assume. Replace the plain assert.
- assert(handler != nullptr); + Assert(handler != nullptr);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
-
src/Makefile.am(2 hunks) -
src/init.cpp(5 hunks) -
src/instantsend/instantsend.cpp(7 hunks) -
src/instantsend/instantsend.h(5 hunks) -
src/instantsend/net_instantsend.cpp(1 hunks) -
src/instantsend/net_instantsend.h(1 hunks) -
src/llmq/context.cpp(1 hunks) -
src/msg_result.h(0 hunks) -
src/net_processing.cpp(5 hunks) -
src/net_processing.h(2 hunks) -
test/lint/lint-circular-dependencies.py(2 hunks)
💤 Files with no reviewable changes (1)
- src/msg_result.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Makefile.am
- test/lint/lint-circular-dependencies.py
🧰 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/init.cppsrc/llmq/context.cppsrc/net_processing.cppsrc/instantsend/net_instantsend.hsrc/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/net_processing.hsrc/instantsend/instantsend.h
🧠 Learnings (20)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 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
Repo: dashpay/dash PR: 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.
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/init.cppsrc/llmq/context.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 Learning: 2025-10-03T11:20:37.475Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6838
File: src/active/context.cpp:29-33
Timestamp: 2025-10-03T11:20:37.475Z
Learning: In Dash codebase, NodeContext (src/node/context.h) serves only as a container with trivial c/d-tors; member lifetime is explicitly managed via reset() calls in the shutdown sequence (src/init.cpp), not by declaration order. For example, active_ctx.reset() is called before DashChainstateSetupClose handles llmq_ctx teardown, ensuring proper destruction order regardless of declaration order in the struct.
Applied to files:
src/init.cpp
📚 Learning: 2025-10-28T08:54:00.392Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6926
File: test/functional/feature_governance_cl.py:0-0
Timestamp: 2025-10-28T08:54:00.392Z
Learning: In Dash tests, the scheduler (mockscheduler) operates independently of network state. Isolated nodes should still run scheduler-based cleanup processes like governance cleanup, even if they have different state due to network isolation.
Applied to files:
src/init.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/llmq/context.cppsrc/instantsend/net_instantsend.hsrc/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.cpp
📚 Learning: 2025-09-09T21:36:58.969Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:223-232
Timestamp: 2025-09-09T21:36:58.969Z
Learning: In RawSender class (src/stats/rawsender.cpp), socket access operations (m_sock and m_server) should be protected by the cs_net mutex, not avoiding synchronization entirely. While lock overhead concerns are valid in general, socket operations require proper synchronization via cs_net.
Applied to files:
src/net_processing.cppsrc/instantsend/net_instantsend.cpp
📚 Learning: 2025-09-09T21:36:11.833Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:11.833Z
Learning: In RawSender class (src/stats/rawsender.cpp), cs_net is the appropriate mutex for protecting socket access (m_sock) and network operations, not additional custom locks. The implementation correctly uses cs_net with GUARDED_BY annotations and EXCLUSIVE_LOCKS_REQUIRED to synchronize socket access between SendDirectly() and ReconnectThread().
Applied to files:
src/net_processing.cppsrc/instantsend/net_instantsend.hsrc/instantsend/net_instantsend.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/net_processing.cppsrc/instantsend/net_instantsend.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-08-01T07:46:37.840Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Applied to files:
src/instantsend/net_instantsend.cpp
🧬 Code graph analysis (7)
src/init.cpp (1)
src/interfaces/node.h (1)
node(42-44)
src/net_processing.cpp (2)
src/validationinterface.cpp (4)
RegisterValidationInterface(133-138)RegisterValidationInterface(133-133)UnregisterValidationInterface(145-150)UnregisterValidationInterface(145-145)src/llmq/dkgsessionhandler.cpp (2)
Misbehaving(112-116)Misbehaving(112-112)
src/instantsend/net_instantsend.h (3)
src/instantsend/instantsend.h (1)
instantsend(38-44)src/net_processing.h (1)
PeerManagerInternal(58-67)src/instantsend/net_instantsend.cpp (12)
ProcessMessage(17-65)ProcessMessage(17-17)Start(67-79)Start(67-67)Stop(81-95)Stop(81-81)WorkThreadMain(247-267)WorkThreadMain(247-247)ProcessPendingISLocks(220-245)ProcessPendingISLocks(220-220)ProcessPendingInstantSendLocks(97-217)ProcessPendingInstantSendLocks(97-99)
src/instantsend/instantsend.cpp (3)
src/logging.h (1)
LogAcceptDebug(235-238)src/net_processing.cpp (4)
hash(1019-1019)hash(1025-1025)ret(676-676)tx(1077-1077)src/node/interfaces.cpp (18)
hash(893-910)hash(893-893)hash(932-936)hash(932-932)hash(958-963)hash(958-958)tx(649-652)tx(649-649)tx(772-775)tx(772-772)tx(776-779)tx(776-776)tx(797-800)tx(797-797)tx(1042-1049)tx(1042-1042)tx(1061-1076)tx(1061-1061)
src/instantsend/net_instantsend.cpp (2)
src/net_processing.cpp (15)
ProcessMessage(3660-5451)ProcessMessage(3660-3665)pfrom(613-613)pfrom(634-635)pfrom(744-746)pfrom(755-755)pfrom(763-763)pfrom(766-766)pfrom(768-768)pfrom(770-770)pfrom(866-866)pfrom(1048-1048)id(694-694)id(698-698)ret(676-676)src/instantsend/net_instantsend.h (1)
Start(35-37)
src/net_processing.h (1)
src/net_processing.cpp (67)
PeerMisbehaving(6520-6523)PeerMisbehaving(6520-6520)pnode(633-633)pnode(638-638)pnode(649-649)pnode(730-730)pnode(774-774)pnode(846-846)pnode(848-848)PeerEraseObjectRequest(6525-6528)PeerEraseObjectRequest(6525-6525)nodeid(623-623)nodeid(626-626)nodeid(639-639)nodeid(650-650)nodeid(670-670)nodeid(672-672)nodeid(710-711)nodeid(721-721)nodeid(853-853)nodeid(855-855)nodeid(1031-1031)nodeid(1066-1066)inv(627-627)inv(628-628)inv(651-651)inv(652-652)inv(653-653)inv(662-662)inv(668-668)inv(956-956)inv(2378-2378)PeerRelayInv(6530-6533)PeerRelayInv(6530-6530)PeerRelayInvFiltered(6535-6538)PeerRelayInvFiltered(6535-6535)PeerRelayInvFiltered(6540-6543)PeerRelayInvFiltered(6540-6540)PeerAskPeersForTransaction(6545-6548)PeerAskPeersForTransaction(6545-6545)txid(629-629)txid(654-654)txid(656-656)txid(659-659)ProcessMessage(3660-5451)ProcessMessage(3660-3665)pfrom(613-613)pfrom(634-635)pfrom(744-746)pfrom(755-755)pfrom(763-763)pfrom(766-766)pfrom(768-768)pfrom(770-770)pfrom(866-866)pfrom(1048-1048)AddExtraHandler(1645-1652)AddExtraHandler(1645-1645)handler(642-642)RemoveHandlers(1654-1659)RemoveHandlers(1654-1654)StartHandlers(1661-1666)StartHandlers(1661-1661)StopHandlers(1668-1676)StopHandlers(1668-1668)InterruptHandlers(1678-1683)InterruptHandlers(1678-1678)
src/instantsend/instantsend.h (3)
src/instantsend/net_instantsend.h (1)
instantsend(12-15)src/instantsend/lock.h (1)
instantsend(18-41)src/instantsend/instantsend.cpp (10)
CInstantSendManager(51-62)CInstantSendManager(64-64)FetchPendingLocks(92-119)FetchPendingLocks(92-92)EnqueueInstantSendLock(68-90)EnqueueInstantSendLock(68-69)PrepareTxToRetry(366-382)PrepareTxToRetry(366-366)ProcessInstantSendLock(121-194)ProcessInstantSendLock(121-122)
🪛 GitHub Actions: Clang Diff Format Check
src/instantsend/instantsend.h
[error] 133-136: Clang format differences found in the header. The diff shows formatting change for PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED attribute. The step '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). (9)
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (11)
src/instantsend/instantsend.cpp (5)
51-62: Constructor refactoring looks good.The removal of the
CQuorumManager¶meter aligns with the PR objective to decouple consensus logic from network logic. The remaining initialization is correct.
66-66: LGTM.Simple helper function for conditional timing metrics.
68-90: LGTM.The method correctly enqueues InstantSend locks with proper locking and timing metrics. Thread safety is maintained with
cs_pendingLocks.
366-382: LGTM.The method correctly collects non-locked transactions for retry with proper thread safety using
LOCK2to acquire bothcs_nonLockedandcs_pendingRetry. The early return and reserve optimizations are good practices.
121-194: All callers correctly handle the variant return type.Verification complete: The single call site in
src/instantsend/net_instantsend.cpp:191properly handles all three variant alternatives with an exhaustive if/else if/else chain. The method declaration includes[[nodiscard]]to enforce caller responsibility. No issues found.src/net_processing.cpp (1)
5441-5443: Iteration is under g_msgproc_mutex; ensure all mutations are too.This loop is safe only if every mutation of m_handlers is performed under the same mutex. After applying the locking fixes above, this is fine. Otherwise, UAF is possible.
Would you confirm no other code mutates m_handlers without taking g_msgproc_mutex (outside the methods fixed above)?
src/instantsend/instantsend.h (5)
8-11: LGTM! Includes support the refactoring.The added includes are necessary to support the new public API methods and the private
dbmember.
22-23: LGTM! Standard library includes for new API.The
<variant>and<vector>includes are required for the new method signatures.
43-43: LGTM! Structure updated for new pending lock handling.The
m_pending_isfield with explicit type aligns with the refactoredFetchPendingLocks()API.
93-95: LGTM! Constructor refactored to remove CQuorumManager dependency.This change aligns with the PR objective to decouple consensus logic from network dependencies.
106-106: LGTM! Signer accessor follows atomic access pattern.The method correctly returns the atomic pointer, and callers are expected to handle potential
nullptr.
| /* Helpers for communications between CInstantSendManager & NetInstantSend */ | ||
| // This helper returns up to 32 pending locks and remove them from queue of pending | ||
| [[nodiscard]] instantsend::PendingState FetchPendingLocks() EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); | ||
| void EnqueueInstantSendLock(NodeId from, const uint256& hash, const std::shared_ptr<instantsend::InstantSendLock>& islock) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks); | ||
| [[nodiscard]] std::vector<CTransactionRef> PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry); | ||
| CSigningManager& Sigman() { return sigman; } | ||
| [[nodiscard]] std::variant<uint256, CTransactionRef, std::monostate> ProcessInstantSendLock( | ||
| NodeId from, const uint256& hash, const instantsend::InstantSendLockPtr& islock) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix clang-format issue flagged by CI.
The new public API methods properly expose the InstantSend functionality for the NetHandler bridge and use appropriate annotations ([[nodiscard]], EXCLUSIVE_LOCKS_REQUIRED). However, the pipeline reports a clang-format violation on lines 133-136.
Please run clang-format on this file to resolve the formatting issue:
🏁 Script executed:
#!/bin/bash
# Show the formatting difference
git diff -U0 origin/develop -- src/instantsend/instantsend.h | ./contrib/devtools/clang-format-diff.py -p1Length of output: 632
🏁 Script executed:
#!/bin/bash
# View the actual file content around lines 131-140
sed -n '130,141p' src/instantsend/instantsend.h | cat -ALength of output: 915
🏁 Script executed:
#!/bin/bash
# Find similar multi-line function declarations to check formatting patterns
rg -A2 'EXCLUSIVE_LOCKS_REQUIRED' src/instantsend/instantsend.h -n | head -40Length of output: 2593
🏁 Script executed:
#!/bin/bash
# Check if clang-format config exists and what it specifies
find . -name '.clang-format' -o -name 'clang-format.cfg' | head -5Length of output: 128
🏁 Script executed:
#!/bin/bash
# Look for formatting guidelines in the codebase
find . -name 'CODING_GUIDELINES*' -o -name 'STYLE*' -o -name 'CONTRIBUTING*' | head -10Length of output: 76
🏁 Script executed:
#!/bin/bash
# Show the .clang-format configuration
cat src/.clang-formatLength of output: 1725
🏁 Script executed:
#!/bin/bash
# Get line lengths of the questionable lines to check column limits
sed -n '133,140p' src/instantsend/instantsend.h | while IFS= read -r line; do echo "${#line}: $line"; doneLength of output: 766
Wrap long lines to comply with clang-format column limit (120 characters).
Lines 134 and 136 exceed the 120-character column limit specified in .clang-format:
- Line 134:
EnqueueInstantSendLockdeclaration is 126 characters - Line 136:
PrepareTxToRetrydeclaration is 124 characters
Wrap these declarations to stay within the limit, following the pattern used for ProcessInstantSendLock (lines 138-140) where parameters are moved to continuation lines.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 133-136: Clang format differences found in the header. The diff shows formatting change for PrepareTxToRetry() EXCLUSIVE_LOCKS_REQUIRED attribute. The step 'git diff -U0 origin/develop --
🤖 Prompt for AI Agents
In src/instantsend/instantsend.h around lines 131 to 140, two declarations
exceed the 120-character clang-format limit: the EnqueueInstantSendLock
prototype on line 134 (126 chars) and PrepareTxToRetry on line 136 (124 chars).
Split their parameter lists and attributes onto continuation lines like
ProcessInstantSendLock does: move parameters (NodeId/from, const uint256&/hash,
const std::shared_ptr<...>/islock for EnqueueInstantSendLock) and the
EXCLUSIVE_LOCKS_REQUIRED(...) annotation to following indented lines so each
line stays ≤120 chars; preserve types, names, return types, and const/attribute
order exactly. Ensure indentation and line breaks follow the existing style used
for ProcessInstantSendLock so clang-format and static checks pass.
| if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { | ||
| RegisterValidationInterface(i); | ||
| } | ||
| m_handlers.emplace_back(std::move(handler)); |
There was a problem hiding this comment.
@coderabbitai it's already a &&; isn't the move redundant?
There was a problem hiding this comment.
@PastaPastaPasta Not redundant. Although the parameter type is std::unique_ptr&&, the named variable handler is an lvalue inside the function. We need std::move to cast it to an rvalue and transfer ownership into m_handlers. Without std::move, this would try to copy a std::unique_ptr and fail to compile.
Either of these is correct:
- m_handlers.emplace_back(std::move(handler));
- m_handlers.push_back(std::move(handler));
Dropping std::move here would be incorrect.
🧠 Learnings used
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 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.
| } | ||
|
|
||
| instantsend::PendingState CInstantSendManager::ProcessPendingInstantSendLocks() | ||
| instantsend::PendingState CInstantSendManager::FetchPendingLocks() |
There was a problem hiding this comment.
why are we changing logic here such as removal of pend?
There was a problem hiding this comment.
I believe the most of the logic is the same; use "hide withespaces difference" to review this function.
Only changes is
-removed.reserve(maxCount);
+removed.reserve(std::min(maxCount, pendingInstantSendLocks.size()));
so we don't reserve too much memory.
Though, there are 2 function ProcessPendingInstantSendLocks inside CInstantSendManager and both of them have been refactored. They have different arguments and purpose but confusing same name.
First one is refactored to 2 functions:
CInstantSendManager::FetchPendingLockswhich do return only list of pending locks and a flag "more work"NetInstantSend::ProcessPendingISLockswhich is outside of CInstantSendManager. We don't use any internal structures to process "pending is locks", see further explanation below for details.
see a caller in the worker thread:
auto [more_work, locks] = m_is_manager.FetchPendingLocks();
if (!locks.empty()) {
ProcessPendingISLocks(std::move(locks));
}Second function with the same name CInstantSendManager::ProcessPendingInstantSendLocks also doesn't belong to CInstantSendManager anymore, it's refactored to NetInstantSend::ProcessPendingInstantSendLocks. It includes bls verification here; check recovered signatures, and if everything is well, it does:
- call CInstantSendManager to process specific single is-lock.
m_is_manager.ProcessInstantSendLock; it's result is std::variant, see https://github.com/dashpay/dash/pull/6959/files?diff=split&w=1#r2516601290 - do peer-manager jobs here (relay inventory, asks for transaction).
- notify sig recovery manager about new recovered signature (do avoid duplicated check for per)
See key piece of this function simplified [it's not a real compilable code from PR]:
for (const auto& [hash, [nodeId, islock]] : pend) {
CInv inv(MSG_ISDLOCK, hash);
auto ret = m_is_manager.ProcessInstantSendLock(nodeId, hash, islock);
if (std::holds_alternative<uint256>(ret)) {
m_peer_manager->PeerRelayInvFiltered(inv, std::get<uint256>(ret));
m_peer_manager->PeerAskPeersForTransaction(islock->txid);
} else if (std::holds_alternative<CTransactionRef>(ret)) {
m_peer_manager->PeerRelayInvFiltered(inv, *std::get<CTransactionRef>(ret));
} else {
assert(std::holds_alternative<std::monostate>(ret));
}
if (!m_is_manager.Sigman().HasRecoveredSigForId(llmq_params.type, recSig->getId()))
m_is_manager.Sigman().PushReconstructedRecoveredSig(recSig);
}So, logic is preserved; just code is a bit moved between modules.
you may find useful to use git show --color-moved=dimmed-zebra -w -b to review this commit.
| [[nodiscard]] std::variant<uint256, CTransactionRef, std::monostate> ProcessInstantSendLock( | ||
| NodeId from, const uint256& hash, const instantsend::InstantSendLockPtr& islock) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry); |
There was a problem hiding this comment.
why are we returning a variant
There was a problem hiding this comment.
because we need to return CTransaction OR uint256 OR nothing.
If it will be (CTransaction OR nothing) I will use std::optional
But there's 3 options. So, variant is a choice.
|
|
||
| if (!m_is_manager.IsInstantSendEnabled()) return; | ||
|
|
||
| const auto islock = std::make_shared<instantsend::InstantSendLock>(); |
There was a problem hiding this comment.
make it non-const and move it at the end of this function
There was a problem hiding this comment.
sorry, how?
const auto islock = std::make_shared<instantsend::InstantSendLock>();
vRecv >> *islock;
I need to use it right here on the next line;
| if (!m_is_manager.AlreadyHave(CInv{MSG_ISDLOCK, hash})) { | ||
| LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: received islock, peer=%d\n", | ||
| __func__, islock->txid.ToString(), hash.ToString(), pfrom.GetId()); | ||
| LogPrint(BCLog::INSTANTSEND, "-- ISDLOCK txid=%s, islock=%s: received islock, peer=%d\n", |
There was a problem hiding this comment.
-
ISDLOCKis the name of network message. It doesn't actually matter which exactly function it is processed, because process handler for ISDLOCK messages is only one; I changed log intentionally. -
'--' is used intentionally, to mark that this log belongs to dash's implementation.
-
this is how it looks currently on regtest:
node1 2025-11-12T08:31:01.788533Z (mocktime: 1417714007) [ msghand] [instantsend/net_instantsend.cpp:60] [ProcessMessage] [instantsend] -- ISDLOCK txid=14aca557b41a3ffc3f17fc08eb7ffbcf907ae5b1de7dcbb67985487c66f8a023, islock=821466cfff26da1fbf7890e13b85be9d9dbc1103cce87212b23a8a1df3a7878e: received islock, peer=2
on mainnet with default options + debug=instantsend it will look like:
2025-11-12T08:34:47Z [instantsend] -- ISDLOCK txid=6b6462e4c61e46a9d995453185314616a8d10dacf3b9463edb3981071e06f41b, islock=6faf787949762e4eb15d03587868b137a727510da726ed9f12d2ec43309f7016: received islock, peer=7
I think the current log string is good enough. Make a suggestion which information is missing in your opinion
2 participants
Issue being fixed or feature implemented
Separation of consensus & chain code and network & node in Dash Core is blocked by tight connection of network and consensus code; in components such as
llmq::CSigningManager,llmq::CSigSharesManager,coinjoin::client,coinjoin::server,governance/,llmq::CInstantSendManager, etc.It blocks backport's of bitcoin related to 'kernel' project.
This PR addresses 1st dependency of
llmq::CInstantSendManageronPeerManager.It is a split from proof-of-concept PR #6934
What was done?
This PR introduces
NetHandlerwhich is a bridge interface between node/network related code and between consensus/chain code. In case if there is no need any network, objects that inherited byNetHandlerare not created.How Has This Been Tested?
Run unit & functional tests.
Run a linter
test/lint/lint-circular-dependencies.pyOverall conception is tested in #6934 as potentially successful to finish separation of network & consensus code.
Breaking Changes
N/A
Checklist: