refactor: separate network and consensus logic 1/N [instantsend] #6959
Conversation
|
WalkthroughInstantSend networking was extracted into a new NetInstantSend NetHandler and integrated into the PeerManager handler lifecycle via AddExtraHandler / StartHandlers / StopHandlers / InterruptHandlers / RemoveHandlers. CInstantSendManager was refactored to remove its CQuorumManager dependency and its worker thread, exposing EnqueueInstantSendLock, FetchPendingLocks, PrepareTxToRetry, Sigman, and ProcessInstantSendLock (returns std::variant<uint256, CTransactionRef, std::monostate>). A new NetInstantSend implementation handles ISDLOCK message parsing, batching verification, recovered-sig reconstruction, and runs a background work thread. PeerManager gained internal forwarding APIs (PeerMisbehaving, PeerRelayInv, etc.) and now delegates message handling to registered NetHandler instances. MessageProcessingResult had two public members removed. Build files were updated to expose net_instantsend.*. Sequence Diagram(s)sequenceDiagram
participant AppInit as AppInitMain
participant PM as PeerManager
participant NIS as NetInstantSend
participant ISM as CInstantSendManager
participant Sig as CSigningManager
AppInit->>ISM: construct CInstantSendManager (qman removed, no worker)
AppInit->>NIS: construct NetInstantSend(ISM, qman, chainstate)
AppInit->>PM: AddExtraHandler(move NIS)
AppInit->>PM: StartHandlers()
PM->>NIS: Start()
NIS->>Sig: start signer if available
NIS->>NIS: spawn work thread (WorkThreadMain)
Note over PM,NIS: runtime message handling
PM->>NIS: ProcessMessage(pfrom,"ISDLOCK",vRecv)
NIS->>ISM: EnqueueInstantSendLock(from, hash, islock)
NIS->>ISM: FetchPendingLocks()
ISM-->>NIS: PendingState
NIS->>NIS: ProcessPendingISLocks -> batch verify -> call ISM.ProcessInstantSendLock(...)
sequenceDiagram
participant Shutdown as Shutdown
participant PM as PeerManager
participant NIS as NetInstantSend
participant ISM as CInstantSendManager
Shutdown->>PM: InterruptHandlers()
PM->>NIS: Interrupt()
Shutdown->>PM: StopHandlers()
PM->>NIS: Stop()
NIS->>NIS: stop signer, join work thread
Shutdown->>PM: RemoveHandlers()
PM->>NIS: clear handler list
Shutdown->>ISM: continue shutdown (no worker thread)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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.
There was a problem hiding this comment.
@UdjinM6 Thanks! I went ahead and provided a minimal, safe patch that:
- Guards m_handlers with g_msgproc_mutex.
- Internally synchronizes AddExtraHandler/RemoveHandlers/StartHandlers/StopHandlers/InterruptHandlers.
- Documents the contract with annotations.
Diffs:
src/net_processing.h
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ class PeerManager : public CValidationInterface, public NetEventsInterface, public PeerManagerInternal
- virtual void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) = 0;
- virtual void RemoveHandlers() = 0;
- virtual void StartHandlers() = 0;
- virtual void StopHandlers() = 0;
- virtual void InterruptHandlers() = 0;
+ // Lifecycle of external NetHandlers is synchronized internally on g_msgproc_mutex.
+ // These methods may be called without holding g_msgproc_mutex.
+ virtual void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) = 0 LOCKS_EXCLUDED(g_msgproc_mutex);
+ virtual void RemoveHandlers() = 0 LOCKS_EXCLUDED(g_msgproc_mutex);
+ virtual void StartHandlers() = 0 LOCKS_EXCLUDED(g_msgproc_mutex);
+ virtual void StopHandlers() = 0 LOCKS_EXCLUDED(g_msgproc_mutex);
+ virtual void InterruptHandlers() = 0 LOCKS_EXCLUDED(g_msgproc_mutex);src/net_processing.cpp
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ class PeerManagerImpl final : public PeerManager
- 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);
@@
- std::vector<std::unique_ptr<NetHandler>> m_handlers;
+ // Accessed/iterated under g_msgproc_mutex together with ProcessMessage().
+ std::vector<std::unique_ptr<NetHandler>> m_handlers GUARDED_BY(NetEventsInterface::g_msgproc_mutex);Implementations (internal locking + safe snapshot on removal)
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@
-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);
+ // Register first, then publish under the message-processing lock.
+ if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) {
+ RegisterValidationInterface(i);
+ }
+ LOCK(g_msgproc_mutex);
+ m_handlers.emplace_back(std::move(handler));
+}
@@
-void PeerManagerImpl::RemoveHandlers()
-{
- InterruptHandlers();
- StopHandlers();
- m_handlers.clear();
-}
+void PeerManagerImpl::RemoveHandlers()
+{
+ // Take ownership of handlers under lock to avoid racing with ProcessMessage().
+ std::vector<std::unique_ptr<NetHandler>> handlers;
+ {
+ LOCK(g_msgproc_mutex);
+ handlers.swap(m_handlers);
+ }
+ // Gracefully shut down outside of g_msgproc_mutex to avoid lock-order issues.
+ 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' now goes out of scope and deletes safely.
+}
@@
-void PeerManagerImpl::StartHandlers()
-{
- for (auto& handler : m_handlers) {
- handler->Start();
- }
-}
+void PeerManagerImpl::StartHandlers()
+{
+ LOCK(g_msgproc_mutex);
+ for (auto& handler : m_handlers) {
+ handler->Start();
+ }
+}
@@
-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();
+ }
+}
@@
-void PeerManagerImpl::InterruptHandlers()
-{
- for (auto& handler : m_handlers) {
- handler->Interrupt();
- }
-}
+void PeerManagerImpl::InterruptHandlers()
+{
+ LOCK(g_msgproc_mutex);
+ for (auto& handler : m_handlers) {
+ handler->Interrupt();
+ }
+}Notes
- ProcessMessage already requires g_msgproc_mutex, so iterating m_handlers remains serialized with these operations.
- RemoveHandlers() uses a swap-out pattern to guarantee no UAF even if a ProcessMessage loop was about to read the vector.
- Keeping UnregisterValidationInterface in Stop/Remove preserves existing behavior. Calling StopHandlers() after RemoveHandlers() will be a no-op.
If you’d like, I can push an updated patchset or open a small follow-up PR with these changes.
🧠 Learnings used
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.
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().
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.
There was a problem hiding this comment.
Sounds reasonable
I would not agree; because init code is single-threaded under cs_main mutex and it assume some sequence of object initialization.
For instance, we don't have locks to initialize NodeContext, such as:
node.chainman = std::make_unique<ChainstateManager>();
node.mn_sync = std::make_unique<CMasternodeSync>(*node.connman, *node.netfulfilledman);
node.govman = std::make_unique<CGovernanceManager>(*node.mn_metaman, *node.netfulfilledman, *node.chainman, node.dmnman, *node.mn_sync);
node.dstxman = std::make_unique<CDSTXManager>();
node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync,
.... and many others
PeerManager's handlers are also single-point initialized and single-point destroyed.
@UdjinM6 Could you point out any possible case, when we may indeed need extra lock beside cs_main here?
@coderabbitai, please look to initialization of node's members in init.cpp and compare with Peer-handlers; is there any special case when we could indeed need a lock?
There was a problem hiding this comment.
🧩 Analysis chain
@knst Thanks for the nudge to double‑check the initialization/shutdown path. Short answer: initialization itself is single‑threaded, but shutdown isn’t guaranteed to be quiescent at the exact moment InterruptHandlers()/RemoveHandlers() run. Because ProcessMessage iterates m_handlers under g_msgproc_mutex, any concurrent mutation of m_handlers without the same lock risks a race/UAF. Concretely:
- Where iteration happens
- In src/net_processing.cpp, ProcessMessage holds g_msgproc_mutex (AssertLockHeld(g_msgproc_mutex)) and does:
for (const auto& handler : m_handlers) { handler->ProcessMessage(...); }
- In src/net_processing.cpp, ProcessMessage holds g_msgproc_mutex (AssertLockHeld(g_msgproc_mutex)) and does:
- Where mutation can happen
- init.cpp calls InterruptHandlers() during Interrupt(NodeContext&).
- init.cpp calls RemoveHandlers() during PrepareShutdown(NodeContext&), and existing RemoveHandlers() iterates m_handlers (Interrupt/Stop) and then clears the vector.
Unless the shutdown sequence first stops the message handling loop and ensures no more ProcessMessage invocations will occur, RemoveHandlers() can run concurrently with that loop.
If your current sequence is:
- InterruptHandlers() (to prompt their worker threads),
- Stop network/message threads,
- RemoveHandlers()/StopHandlers(),
then we’re safe even without extra locking.
But the AI summary shows PrepareShutdown calling RemoveHandlers() before stopping peerman, and ProcessMessage can still be mid-iteration at that time. That’s the only “special case” I was guarding against.
I’m happy to align with your preference. Two low-friction options:
-
Option A: Keep API light, document/annotate the contract.
- Add EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) to AddExtraHandler/RemoveHandlers/StartHandlers/StopHandlers/InterruptHandlers, and take g_msgproc_mutex at the init.cpp call sites. That makes the sequencing explicit and matches how ProcessMessage already guards reads.
-
Option B (no call-site changes): Internalize the lock in the 5 methods.
- LOCK(g_msgproc_mutex) around any access to m_handlers.
- For RemoveHandlers(), swap m_handlers into a local vector under the lock, then Stop/Interrupt/clear outside the lock to avoid lock-order issues. This is robust even if shutdown calls arrive while the message loop is still draining.
Given your single-point init/dtor intent, Option A is probably sufficient if we also ensure, in init.cpp, that:
- InterruptHandlers() is called while message processing may still be running (that’s fine),
- but RemoveHandlers() only after the message handler thread has stopped (no further ProcessMessage possible).
If you confirm that order, I’ll drop the patch and just add a small comment near m_handlers to state “Accessed under g_msgproc_mutex; RemoveHandlers() must run after message thread shutdown.” Otherwise, I can provide the minimal internal-locking version to make it future-proof without touching init.cpp.
Let me know which route you prefer and I’ll update the diff accordingly.
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== Call sites in init.cpp =="
fd init.cpp -a
rg -n -C4 --line-number 'AddExtraHandler\(|StartHandlers\(|InterruptHandlers\(|RemoveHandlers\(|StopHandlers\(' -- src/init.cpp || true
echo
echo "== Net thread lifecycle clues in init.cpp =="
rg -n -C4 --line-number 'StartNode|StopNode|Interrupt\(|PrepareShutdown\(|StartShutdown|ShutdownRequested' -- src/init.cpp || true
rg -n -C4 --line-number 'connman\.(Start|Stop)|StartMapPort|StopMapPort|ThreadMessageHandler|StartExtraBlockRelayPeers' -- src/init.cpp src/net.cpp src/net_processing.cpp || true
echo
echo "== ProcessMessage iterates handlers under g_msgproc_mutex =="
rg -n -C2 --line-number 'AssertLockHeld\(g_msgproc_mutex\)|for\s*\(const auto& handler\s*:\s*m_handlers' -- src/net_processing.cpp
echo
echo "== Definitions of handler API methods (current locking) =="
rg -n -C3 --line-number 'PeerManagerImpl::(AddExtraHandler|RemoveHandlers|StartHandlers|StopHandlers|InterruptHandlers)\b' -- src/net_processing.cpp
echo
echo "== Any other mutations of m_handlers =="
rg -n -C2 --line-number 'm_handlers\.' -- src/net_processing.cpp
Length of output: 15388
@knst Thanks for the prompt to verify. I checked the actual call sites and the message-thread lifecycle in this branch:
- AppInitMain:
- AddExtraHandler(...) at Line 2201.
- StartHandlers() at Line 2297.
- Interrupt(NodeContext&):
- InterruptHandlers() at Line 255, then node.connman->Interrupt() at Line 262.
- PrepareShutdown(NodeContext&):
- RemoveHandlers() at Line 292, then StopHandlers() at Line 295.
And in net.cpp the msghand thread (CConnman::ThreadMessageHandler) is created inside CConnman and takes g_msgproc_mutex for processing. ProcessMessage/ProcessMessages/SendMessages assert that g_msgproc_mutex is held and iterate m_handlers under it.
Putting this together:
- AddExtraHandler runs in AppInitMain before StartHandlers and before the networking threads are started, so no concurrency with ProcessMessage.
- InterruptHandlers is invoked while msghand may still be running, but it only iterates m_handlers; it does not mutate the vector, so it’s safe versus concurrent iteration in ProcessMessage.
- Per the comments in init.cpp (Lines 210–222), WaitForShutdown interrupts and joins the thread group before Shutdown()/PrepareShutdown() runs. That means RemoveHandlers (which clears m_handlers) executes after msghand has exited. No overlap with ProcessMessage’s iteration, so no race/UAF there either.
Conclusion: there isn’t a “special case” in the current init/shutdown sequencing that requires taking a lock in these APIs, provided the existing contract holds:
- AddExtraHandler happens before the message thread starts, and
- RemoveHandlers happens after the message thread is joined.
If you’d like a small belt-and-suspenders, we can:
- Add a brief comment near m_handlers documenting that it’s only mutated pre-start and post-join of msghand, or
- add a lightweight runtime assert (e.g., assert(connman->IsMessageHandlerStopped()) inside RemoveHandlers in non-release builds).
I’m happy to leave the code as-is and add the comment, or wire in the assert—whichever you prefer.
🧬 Code Graph Analysis Results
[relevant_code_snippets]
-
File: src/net_processing.cpp
- Summary: Node/peer initialization and locking context
- Description: The InitializeNode implementation shows that node-member initialization occurs with two separate mutex protections:
- First, lock cs_main to insert a new CNodeState for the node, and initialize per-node state.
- Then, lock m_peer_mutex to insert/update the corresponding Peer object in m_peer_map.
- Finally, if the connection is not inbound, PushNodeVersion is invoked to send version info.
- This pattern indicates that initialization of node/peer state is guarded by two distinct locks (cs_main for global validation state and m_peer_mutex for per-peer state), which is relevant to discussion about whether any additional locking is needed when creating or destroying peer-handlers or node contexts.
- Context in net_processing.cpp (conceptual):
- Acquire cs_main to create the per-node CNodeState in m_node_states.
- Acquire m_peer_mutex to create/attach the Peer in m_peer_map.
- Optionally perform actions (e.g., PushNodeVersion) under appropriate locks.
-
File: src/net_processing.cpp
- Summary: SetupAddressRelay and member initialization guarded by a separate mutex
- Description: The code path for establishing address-relay capabilities initializes per-peer address relay state lazily and creates/uses m_addr_known BloomFilter under the NetEvents/peer context. This demonstrates a pattern where per-peer data structures are initialized under a specific lock (e.g., m_peer_mutex) and further guarded by other mutexes when accessed (e.g., m_addr_send_times_mutex, m_block_inv_mutex, m_tx_inventory_mutex). This is relevant to understanding whether additional locking would be required if PeerManagerImpl::InitializeNode or similar initialization paths are extended.
- Context in net_processing.cpp (conceptual):
- Access to peer.m_addr_known and related relay state is performed with m_peer_mutex protection, and related per-peer fields are guarded by their own internal mutexes.
-
File: src/net_processing.cpp
- Summary: Notable locking patterns in per-peer state management
- Description: The file uses a pattern of per-peer data guarded by a dedicated mutex inside Peer::TxRelay and other nested structures. Accessors like SetTxRelay/GetTxRelay are designed with exclusive/shared locking annotations, illustrating how initialization and destruction should coordinate with existing locks. This is pertinent when evaluating whether a lock is needed for specific initialization order or for single-point initialization/destroy operations mentioned in the user comment.
- Context in net_processing.cpp (conceptual):
- Per-peer TxRelay and related state are protected by internal m_tx_relay_mutex, m_bloom_filter_mutex, etc., with function-level locking requirements guiding safe initialization/destruction.
Note: The user asked to identify potentially relevant snippets to understand the initialization of a node’s members in init.cpp and compare with Peer-handlers; the above captures patterns in net_processing.cpp that relate to how node/peer state is created and guarded with locks, which is relevant to assessing whether locks are needed in single-threaded init vs. multi-threaded access. If you want exact code excerpts pasted verbatim, I can include the specific blocks from net_processing.cpp ( InitializeNode, related Peer initialization, and SetupAddressRelay usage ) with precise line ranges.
🧠 Learnings used
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.
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.
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().
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: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
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.
There was a problem hiding this comment.
@knst the race is in ProcessMessage vs Shutdown
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.
- Shutdown. Before call of shutdown there's call of PrepareShutdown.
PrepareShutdown does:
if (node.peerman) node.peerman->StopHandlers();
...
if (node.connman) node.connman->Stop();
...
node.peerman.reset()
Some semi-relevant doc:
/**
* Shutdown is split into 2 parts:
* Part 1: shut down everything but the main wallet instance (done in PrepareShutdown() )
* Part 2: delete wallet instance
*
* In case of a restart PrepareShutdown() was already called before, but this method here gets
* called implicitly when the parent object is deleted. In this case we have to skip the
* PrepareShutdown() part because it was already executed and just delete the wallet instance.
*/
- ProcessMessage()
ProcessMessage() for handlers are called inside PeerManagerImpl byPeerManagerImpl::ProcessMessage:
for (const auto& handler : m_handlers) {
handler->ProcessMessage(pfrom, msg_type, vRecv);
}
which is called by PeerManagerImpl::ProcessMessages which is called from CConnman::ThreadMessageHandler.
It means, that internal threads of handlers will be stopped -> after that stopped processing of p2p messages -> after that removed all handlers and PeerManager (in this order).
So, I believe there's no race @UdjinM6
There was a problem hiding this comment.
Hmm.. Indeed, the current shutdown sequence appears safe. But it's also fragile. Guess we can refactor it later if needed.
| /** 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 |
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.
| 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.
src/instantsend/net_instantsend.cpp
Outdated
|
|
||
| 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;
There was a problem hiding this comment.
make it non-const; move it on this line: https://github.com/dashpay/dash/pull/6959/files#diff-53f9df4bc25173e6698b6a6858b4248e55cbab98e711952b8f20404ca95f3687R63
This way we avoid a copy of the shared_ptr
There was a problem hiding this comment.
ah, std::move it. I got! not move a line
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-peermanager-handlers-is
branch
from
cf2d6a9 to
a53d5c1
Compare
Rebased due to conflict with merged #6956 Fixed log name |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (10)
src/instantsend/net_instantsend.cpp (2)
82-94: Stop must self-interrupt before joining.
IfStopHandlers()is ever invoked without an earlierInterruptHandlers()(the new lifecycle APIs allow that),workInterruptremains unset. In release builds the assert disappears, the worker keeps running, andjoin()blocks forever. Please haveStop()signal the interrupt itself before joining so we can’t deadlock.void NetInstantSend::Stop() { if (auto signer = m_is_manager.Signer(); signer) { signer->Stop(); } - // make sure to call Interrupt() first - if (!workInterrupt) { - assert(false); - } + if (!workInterrupt) { + workInterrupt(); + } if (workThread.joinable()) { workThread.join(); } }
134-142: Guard chain queries withcs_maininside the handler thread.
ProcessPendingInstantSendLocksruns on the NetInstantSend worker, som_chainstate.m_chain.Height()(and the rotation quorum selection) must holdcs_main. Right now the height read races against validation threads mutating the chain. Please wrap both the height lookup andSelectQuorumForSigninginWITH_LOCK(::cs_main, …)so we stay within the project’s locking discipline.int nSignHeight{-1}; const auto dkgInterval = llmq_params.dkgInterval; - if (blockIndex->nHeight + dkgInterval < m_chainstate.m_chain.Height()) { + const int chain_height = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Height()); + if (blockIndex->nHeight + dkgInterval < 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); + auto quorum = llmq_params.useRotation + ? WITH_LOCK(::cs_main, return 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 (8)
1086-1087: Annotate m_handlers with its guarding mutex.Mark m_handlers as guarded by g_msgproc_mutex to document/thread‑safe access and help static analysis.
- std::vector<std::unique_ptr<NetHandler>> m_handlers; + std::vector<std::unique_ptr<NetHandler>> m_handlers GUARDED_BY(NetEventsInterface::g_msgproc_mutex);
641-647: Define a clear synchronization contract for handler API.Add/Remove/Start/Stop/Interrupt currently have no lock requirement and their bodies don’t lock, yet ProcessMessage iterates m_handlers under g_msgproc_mutex. This can race and UAF if lifecycle calls run concurrently.
Either require callers to hold g_msgproc_mutex (annotations), or internally lock. Recommend internal locking (safer for API consumers); see diffs below for impls.
1645-1653: AddExtraHandler must lock around container mutation (and registration).Mutates m_handlers without synchronization; can race with ProcessMessage traversal.
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)); + Assert(handler != nullptr); + LOCK(g_msgproc_mutex); + NetHandler* raw = handler.get(); + m_handlers.emplace_back(std::move(handler)); + if (auto i = dynamic_cast<CValidationInterface*>(raw); i != nullptr) { + RegisterValidationInterface(i); + } }
1654-1660: RemoveHandlers must not race or call into handlers while holding the lock.Make a snapshot under g_msgproc_mutex, then interrupt/stop/unregister outside to avoid deadlocks/reentrancy while ProcessMessage holds the same lock.
void PeerManagerImpl::RemoveHandlers() { - InterruptHandlers(); - StopHandlers(); - m_handlers.clear(); + 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(); + } }
1661-1667: StartHandlers: snapshot under lock, start outside.Prevents traversal races and avoids calling handler code while holding g_msgproc_mutex.
void PeerManagerImpl::StartHandlers() { - for (auto& handler : m_handlers) { - handler->Start(); - } + std::vector<NetHandler*> to_start; + { + LOCK(g_msgproc_mutex); + to_start.reserve(m_handlers.size()); + for (auto& h : m_handlers) to_start.push_back(h.get()); + } + for (auto* h : to_start) h->Start(); }
1668-1676: StopHandlers: snapshot under lock; unregister then stop outside.Avoids races and potential deadlocks if Stop() re-enters net processing.
void PeerManagerImpl::StopHandlers() { - for (auto& handler : m_handlers) { - if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { - UnregisterValidationInterface(i); - } - handler->Stop(); - } + std::vector<NetHandler*> to_stop; + { + LOCK(g_msgproc_mutex); + to_stop.reserve(m_handlers.size()); + for (auto& h : m_handlers) to_stop.push_back(h.get()); + } + for (auto* h : to_stop) { + if (auto i = dynamic_cast<CValidationInterface*>(h); i != nullptr) { + UnregisterValidationInterface(i); + } + } + for (auto* h : to_stop) h->Stop(); }
1678-1684: InterruptHandlers: snapshot under lock, then interrupt outside.void PeerManagerImpl::InterruptHandlers() { - for (auto& handler : m_handlers) { - handler->Interrupt(); - } + std::vector<NetHandler*> to_interrupt; + { + LOCK(g_msgproc_mutex); + to_interrupt.reserve(m_handlers.size()); + for (auto& h : m_handlers) to_interrupt.push_back(h.get()); + } + for (auto* h : to_interrupt) h->Interrupt(); }
6525-6529: PeerEraseObjectRequest must honor cs_main requirement.Underlying EraseObjectRequest requires cs_main. Acquire it here to keep API safe for handlers; if you choose this, drop the EXCLUSIVE_LOCKS_REQUIRED(::cs_main) on the declaration.
void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) { - EraseObjectRequest(nodeid, inv); + WITH_LOCK(::cs_main, EraseObjectRequest(nodeid, inv)); }Please align the header declaration accordingly (remove the cs_main requirement if locking internally).
🧹 Nitpick comments (1)
src/net_processing.cpp (1)
5441-5443: Avoid reentrancy pitfalls: consider snapshotting before calling external handlers.You call handler->ProcessMessage() while holding g_msgproc_mutex. If a handler tries to add/remove handlers, it will contend on the same mutex and can deadlock. Either:
- Document “no lifecycle calls from ProcessMessage” and add debug assertions, or
- Snapshot m_handlers under the lock and invoke ProcessMessage on the snapshot after releasing it (heavier change).
Recommend at least asserting no lifecycle calls occur during handler->ProcessMessage.
📜 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
- src/llmq/context.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/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/init.cppsrc/instantsend/net_instantsend.hsrc/instantsend/instantsend.hsrc/net_processing.cppsrc/net_processing.h
🧠 Learnings (24)
📓 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-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/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/instantsend.h
📚 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/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.hsrc/net_processing.cpptest/lint/lint-circular-dependencies.py
📚 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/instantsend.cppsrc/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/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.htest/lint/lint-circular-dependencies.py
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Applied to files:
src/instantsend/instantsend.cpptest/lint/lint-circular-dependencies.py
📚 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/instantsend.cppsrc/instantsend/net_instantsend.cpptest/lint/lint-circular-dependencies.py
📚 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/instantsend.cppsrc/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/instantsend.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-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/net_processing.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-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/instantsend/net_instantsend.cppsrc/net_processing.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-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-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.cpptest/lint/lint-circular-dependencies.py
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 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/instantsend/net_instantsend.cppsrc/init.cppsrc/instantsend/instantsend.h
📚 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 operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/instantsend/net_instantsend.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-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/instantsend/instantsend.h
🧬 Code graph analysis (6)
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 (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(36-38)
src/instantsend/net_instantsend.h (3)
src/instantsend/instantsend.h (1)
instantsend(38-49)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.h (3)
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)src/instantsend/net_instantsend.h (1)
instantsend(12-16)src/instantsend/lock.h (1)
instantsend(18-41)
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/net_processing.h (2)
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/net.h (1)
NetEventsInterface(1128-1164)
⏰ 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: mac-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/instantsend/instantsend.cpp (6)
51-62: LGTM - Constructor refactored to remove CQuorumManager dependency.The removal of the
CQuorumManager¶meter aligns with the PR objective to decouple network and consensus logic dependencies.
66-66: LGTM - ShouldReportISLockTiming extracted as free function.The extraction to a free function with
LogAcceptDebugis appropriate for the refactoring.
68-90: LGTM - New public API for enqueueing InstantSend locks.The
EnqueueInstantSendLockmethod correctly handles timing statistics and thread-safe enqueueing of pending locks. The timing logic appropriately handles both the normal case (tx before islock) and the edge case (islock before tx).
92-119: LGTM - FetchPendingLocks refactored with improved reserve optimization.The refactoring correctly extracts pending lock fetching logic. The reserve optimization at line 102 using
std::min(maxCount, pendingInstantSendLocks.size())prevents over-allocation when the pending set is large. The max-32 processing limit and thread safety are preserved.
121-194: LGTM - ProcessInstantSendLock return type refactored to explicit variant.The change to
std::variant<uint256, CTransactionRef, std::monostate>makes the processing result explicit:
std::monostate: lock already known/chainlocked (no relay needed)uint256: transaction not yet available (relay lock inv, request tx)CTransactionRef: transaction available (relay lock inv with tx)This improves clarity over the previous struct-based result and enables the caller (NetInstantSend) to handle network operations appropriately. The processing logic is preserved.
366-382: LGTM - PrepareTxToRetry provides thread-safe retry candidate collection.The new public API correctly collects non-locked transactions eligible for retry. Thread safety is maintained via
LOCK2(cs_nonLocked, cs_pendingRetry), and the method appropriately returns a vector for external coordination without modifying internal state.src/instantsend/instantsend.h (1)
135-145: Net/consensus helper bridge is tidy.Nice job carving out the helper surface; the interface mirrors the implementations in instantsend.cpp and keeps the lock annotations consistent, so it plugs cleanly into the new NetHandler flow.
| struct PendingState { | ||
| bool m_pending_work{false}; | ||
| std::vector<std::pair<NodeId, MessageProcessingResult>> m_peer_activity{}; | ||
| std::vector<std::pair<uint256, PendingISLockFromPeer>> m_pending_is; |
There was a problem hiding this comment.
why not just std::vector<PendingISLockFromPeer>
There was a problem hiding this comment.
Firstly, there're 3 possible cases:
- vector has < 32 elements - no more works
- vector has = 32 elements - no more works
- vector has = 32 elements - more work
So, need to return also flag if there's any pending tasks left and keep the behaviour after refactoring the same.
Secondly, if instead returning flag 'm_pending_work' just compare <32 or ==32; it will create less robust code; because 2 components will use the same constant "32": to determine if there's enough elements in a batch; to determine if there's any work leftover. This check is not bug-prune and easy to mistake in the future when edit this code.
There was a problem hiding this comment.
Not the more work flag, the hash in the vector.
There was a problem hiding this comment.
Because it is used here:
auto [more_work, locks] = m_is_manager.FetchPendingLocks();
ProcessPendingISLocks(std::move(locks));
->
void NetInstantSend::ProcessPendingISLocks(std::vector<std::pair<uint256, instantsend::PendingISLockFromPeer>>&& locks_to_process)
{
...
for (auto& p : locks_to_process) {
if (bad_is_locks.contains(p.first)) { <---- here
still_pending.push_back(std::move(p));
}
}
Otherwise need to recalculate hash here; and one more time inside NetInstantSend::ProcessPendingInstantSendLocks
There was a problem hiding this comment.
Also notice, that it's a moved code; original is inside CInstantSendManager::ProcessPendingInstantSendLocks:
- // filter out valid IS locks from "pend" - keep only bad ones
- std::vector<std::pair<uint256, instantsend::PendingISLockFromPeer>> filteredPend;
- filteredPend.reserve(badISLocks.size());
- for (auto& p : pend) {
- if (badISLocks.contains(p.first)) {
- filteredPend.push_back(std::move(p));
- }
- }
-
src/instantsend/instantsend.h
Outdated
| /* 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) |
There was a problem hiding this comment.
take by value; and move it later
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/instantsend/net_instantsend.cpp (1)
97-217: Batch verification + relay logic is consistent; note existingm_chainstatelocking concern.
ProcessPendingInstantSendLockscorrectly:
- Skips bad sources and pre-verified locks.
- Resolves
cycleHashto a block index undercs_main.- Uses the LLMQ params to compute
nSignHeight, select a quorum, and derive the sign hash.- Runs a BLS batch verification and tracks
badMessagesandbadSources.- Delegates to
CInstantSendManager::ProcessInstantSendLockand relays viaPeerRelayInvFiltered, using the new variant to distinguish between known txs, unknown txids, and “no-op” cases.- Optionally reconstructs and forwards recovered signatures to the signing manager to avoid double verification.
The access to
m_chainstate.m_chain.Height()and the call toSelectQuorumForSigningwithout holdingcs_mainmirror the pre-existing pattern that was previously discussed; it’s an existing locking concern rather than introduced by this refactor.
🧹 Nitpick comments (1)
src/instantsend/instantsend.cpp (1)
68-90: Consider actually moving theInstantSendLockshared_ptr into the pending map.
EnqueueInstantSendLocktakesstd::shared_ptr<InstantSendLock>&&but currently copies it intopendingInstantSendLocks. Usingstd::move(islock)would avoid an extra atomic reference-count bump while preserving semantics.- LOCK(cs_pendingLocks); - pendingInstantSendLocks.emplace(hash, instantsend::PendingISLockFromPeer{from, islock}); + LOCK(cs_pendingLocks); + pendingInstantSendLocks.emplace(hash, instantsend::PendingISLockFromPeer{from, std::move(islock)});Not critical, but more consistent with the rvalue-qualifier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/instantsend/instantsend.cpp(7 hunks)src/instantsend/instantsend.h(5 hunks)src/instantsend/net_instantsend.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (22)
📓 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-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/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 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/instantsend/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 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/instantsend.cppsrc/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/instantsend.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Applied to files:
src/instantsend/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/instantsend.cppsrc/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/instantsend.cppsrc/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/instantsend.cppsrc/instantsend/net_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-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/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-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-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-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.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access is protected by the cs_net mutex. This mutex coordinates between the TCP send path in SendDirectly() and the reconnection operations in Connect()/Reconnect() methods, ensuring proper synchronization of socket state.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-05-10T00:54:30.887Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.887Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 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/instantsend/net_instantsend.cppsrc/instantsend/instantsend.h
📚 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 operations are properly synchronized using the cs_net mutex. The m_sock and m_server members are GUARDED_BY(cs_net), and methods like Connect(), SendDirectly(), and ReconnectThread() use appropriate locking with cs_net to prevent race conditions on socket access.
Applied to files:
src/instantsend/net_instantsend.cpp
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/instantsend/instantsend.h
🧬 Code graph analysis (3)
src/instantsend/instantsend.cpp (3)
src/logging.h (1)
LogAcceptDebug(235-238)src/net_processing.cpp (2)
ret(676-676)tx(1077-1077)src/node/interfaces.cpp (12)
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/llmq/context.cpp (4)
Start(50-55)Start(50-50)Stop(57-62)Stop(57-57)src/instantsend/net_instantsend.h (1)
Start(36-38)
src/instantsend/instantsend.h (1)
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)
⏰ 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). (8)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (13)
src/instantsend/instantsend.cpp (5)
7-17: Includes and constructor wiring look consistent with new dependencies.The added includes and updated constructor cleanly reflect the removal of the CQuorumManager dependency and the new use of chainlock, blockstorage, stats, and validation/mempool APIs. Member initialization order matches declaration order, so no subtle lifetime bugs here.
Also applies to: 51-61
66-66: Centralized timing helper is straightforward and used safely.
ShouldReportISLockTiming()cleanly gates stats/logging, and both uses (enqueue and AddNonLockedTx) correctly guard access totimingsTxSeenundercs_timingsTxSeen. No behavioral changes beyond the refactor.Also applies to: 316-320
92-119: FetchPendingLocks batching preserves behavior; only minor copy overhead.The function keeps the 32-lock batch limit and
m_pending_workflag semantics while avoiding iterator invalidation by storing keys to erase later. The only cost vs an iterator-based move is copyingPendingISLockFromPeer(including a shared_ptr) intom_pending_is, which is acceptable in this refactor-focused PR.
121-194: Variant-basedProcessInstantSendLockresult cleanly encodes relay decisions.The new return type maps cleanly to the NetInstantSend handling:
std::monostate→ no relay (already known, duplicate/chainlocked, etc.).CTransactionRef→ relay using full transaction context and send notifications.uint256(txid) → relay and request the transaction from peers when it’s missing locally.Internal behavior (db writes, conflict resolution, mempool updates, notifications) matches the previous flow; only the result packaging changed. This looks sound for the NetHandler bridge.
366-382:PrepareTxToRetrycorrectly snapshots retry candidates without mutating state.Locking
cs_nonLockedandcs_pendingRetrytogether and collecting existingCTransactionRefs into a vector for the signer preserves prior behavior; it doesn’t touchpendingRetryTxs, so retry bookkeeping remains centralized inRemoveNonLockedTxand related code. This fits well with the new worker-thread orchestration.src/instantsend/instantsend.h (3)
8-11: New includes andPendingStatelayout are appropriate for the NetHandler bridge.Pulling in
instantsend/db.h,lock.h,signing.h, and<variant>here matches the new public API surface.PendingState’sm_pending_isas a vector of(hash, PendingISLockFromPeer)aligns withFetchPendingLocksand NetInstantSend’s batching needs without changing observable behavior.Also applies to: 19-23, 41-49
98-101: Ctor and signer accessor changes are consistent with the new ownership model.Dropping the
CQuorumManager¶meter from the constructor and keeping only the chainlock/chainstate/signing/spork/mempool/mn_sync/db wiring matches the fact that quorum-specific work has moved into NetInstantSend. TheSigner()accessor is a thin, thread-safe wrapper over the atomic pointer and is used appropriately by the NetHandler.Also applies to: 111-111
135-145: NetInstantSend helper APIs are well-shaped and lock annotations look correct.The new public helpers (
FetchPendingLocks,EnqueueInstantSendLock,PrepareTxToRetry,Sigman,ProcessInstantSendLock) cleanly separate network-facing orchestration (NetInstantSend) from stateful consensus logic (CInstantSendManager). Each method acquires its own mutexes and theEXCLUSIVE_LOCKS_REQUIRED(!...)annotations match the implementations ininstantsend.cpp, which should keep ThreadSanitizer/static thread-safety checks happy.src/instantsend/net_instantsend.cpp (5)
17-65: ISDLOCK message handling and enqueue path look correct.
ProcessMessagecleanly filters onISDLOCK, checks InstantSend is enabled, deserializes the lock, computes its hash, clears the outstanding object request, runs trivial validation and cycleHash/height checks, and only enqueues ifAlreadyHavesays it’s new. The log line matches the new NetInstantSend-centric flow, and the move intoEnqueueInstantSendLockavoids extra shared_ptr copies at the call site.
67-95: Worker thread lifecycle and signer coordination are sound.
Start()asserts there’s no existing worker, spawns theTraceThread("isman", ...), and then starts the signer if present.Stop()stops the signer first, asserts thatInterrupt()(workInterrupt) was called, and joins the worker if joinable, which matches the PeerManager handler lifecycle expectations and avoids dangling threads on shutdown.
220-245: Two-pass processing against current and previous LLMQ active sets is preserved.
ProcessPendingISLocksfirst verifies against the current active set without banning, collects only the failing locks, and then re-verifies those against the previous active set withban=true. The filtering viabad_is_locksandstd::moveintostill_pendingmaintains the original behavior while keeping the interface toProcessPendingInstantSendLockssimple.
247-267: Worker loop correctly coordinates pending locks and retry txs.
WorkThreadMain:
- Exits promptly when
workInterruptis set.- Uses
FetchPendingLocksto get batched pending IS locks and hands them toProcessPendingISLocks.- Invokes
signer->ProcessPendingRetryLockTxs(m_is_manager.PrepareTxToRetry())to handle child retries.- Sleeps for 100 ms only when there’s no immediate more_work flag.
This matches the intention of moving InstantSend’s background work out of CInstantSendManager and into the NetHandler without changing behavior.
1-267: No action required—net_instantsend files are already marked as non-backported.The file
src/instantsend/net_instantsend.cppand its header are already covered by the existing wildcard patternssrc/instantsend/*.cppandsrc/instantsend/*.hintest/util/data/non-backported.txt. No additional entries are needed.
src/instantsend/instantsend.h
Outdated
| // 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) | ||
| void EnqueueInstantSendLock(NodeId from, const uint256& hash, std::shared_ptr<instantsend::InstantSendLock>&& islock) |
There was a problem hiding this comment.
take it by value; not by &&, imo. then on cab3b27#diff-ad0e0b034e03426cef39d5c87fbe3fde42546c13072c6d89040dbb6fc7f45ff1R89 move it again
There was a problem hiding this comment.
@coderabbitai justify both options: pass std::shared_ptr as && and as a value; benefits and downsides if any.
There was a problem hiding this comment.
@knst, here's a detailed comparison of both approaches:
Passing by rvalue reference (std::shared_ptr<T>&&)
Benefits:
- Explicitly signals at the signature level that the function takes ownership
- Slightly more explicit about move semantics being required
- Can avoid ambiguity about the function's intent
Downsides:
- Less flexible - requires callers to explicitly use
std::move()or pass a temporary - Cannot accept lvalues without
std::move()at the call site - Still requires moving from the parameter inside the function body
- More verbose at call sites
Passing by value (std::shared_ptr<T>)
Benefits:
- More flexible - accepts both rvalues and lvalues naturally
- When caller passes an rvalue (via
std::move()or temporary): uses move constructor (cheap - just pointer copy) - When caller passes an lvalue: uses copy constructor (reference count increment)
- Allows moving from the parameter inside the function without additional syntax
- Aligns with modern C++ best practices for "sink" parameters
- Cleaner, more idiomatic code
Downsides:
- Less explicit about ownership transfer at the signature level
- If caller passes an lvalue, incurs a reference count increment/decrement cycle
Recommendation
Pass by value is the modern C++ idiom for "sink" parameters (parameters where the function takes ownership). This is the approach recommended by C++ experts like Herb Sutter in his "Back to Basics" talks.
Why pass-by-value wins for sink parameters:
- Optimal performance: When passed an rvalue, only one move occurs. When passed an lvalue, one copy occurs (which is necessary anyway to preserve the source).
- Single implementation handles both move and copy semantics correctly
- More maintainable and flexible for callers
In your case, since the call site already uses std::move(islock), both approaches perform identically (one move operation). However, pass-by-value would also support callers who want to retain their copy of the shared_ptr, making the API more versatile.
Bottom line: PastaPastaPasta's suggestion to take by value is aligned with modern C++ best practices and provides better API flexibility with no performance penalty.
🧠 Learnings used
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.
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.887Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
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.
There was a problem hiding this comment.
I love that you referenced "This is the approach recommended by C++ experts like Herb Sutter in his "Back to Basics" talks." :D
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!
knst
force-pushed
the
refactor-peermanager-handlers-is
branch
from
cab3b27 to
43ef719
Compare
| /** 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.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/net_processing.cpp (7)
646-652: Document the lock contract on handler lifecycle API.These mutate/iterate m_handlers but lack annotations; add LOCKS_EXCLUDED(g_msgproc_mutex) (or EXCLUSIVE_LOCKS_REQUIRED if you want callers to hold it) to make the contract explicit.
Apply minimal docs-only diff:
- void AddExtraHandler(std::unique_ptr<NetHandler>&& handler) override; - void RemoveHandlers() override; - void StartHandlers() override; - void StopHandlers() override; - void InterruptHandlers() override; + // Handler lifecycle may be called without g_msgproc_mutex; ProcessMessage iterates under it. + 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);Based on learnings.
1091-1092: Mark m_handlers as guarded to aid static analysis.Align with usage under g_msgproc_mutex.
- std::vector<std::unique_ptr<NetHandler>> m_handlers; + std::vector<std::unique_ptr<NetHandler>> m_handlers GUARDED_BY(NetEventsInterface::g_msgproc_mutex);Based on learnings.
1650-1657: Consider guarding AddExtraHandler with g_msgproc_mutex (or assert pre-start).Currently unsynchronized vs ProcessMessage iteration. If you rely on init-time only, add a debug assert/comment; otherwise lock.
Option A (docs-only):
void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler) { - assert(handler != nullptr); + // Added before message thread starts; see init.cpp sequencing. + assert(handler != nullptr);Option B (internal lock):
void PeerManagerImpl::AddExtraHandler(std::unique_ptr<NetHandler>&& handler) { assert(handler != nullptr); - if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { + LOCK(g_msgproc_mutex); + if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { RegisterValidationInterface(i); } m_handlers.emplace_back(std::move(handler)); }Based on learnings.
1659-1665: Clear handlers without racing iteration: swap-under-lock pattern.Swap m_handlers under g_msgproc_mutex, then Interrupt/Stop outside the lock to avoid UAF and lock-order issues.
void PeerManagerImpl::RemoveHandlers() { - InterruptHandlers(); - StopHandlers(); - m_handlers.clear(); + 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(); + } }Based on learnings.
1666-1671: StartHandlers: optional protection while iterating.Lock g_msgproc_mutex or add a comment asserting single-threaded startup to future-proof against races.
void PeerManagerImpl::StartHandlers() { - for (auto& handler : m_handlers) { + LOCK(g_msgproc_mutex); + for (auto& handler : m_handlers) { handler->Start(); } }
1673-1681: StopHandlers: optional lock while traversing.Same rationale as StartHandlers. If you keep it unlocked, add a note about shutdown sequencing.
void PeerManagerImpl::StopHandlers() { - for (auto& handler : m_handlers) { + LOCK(g_msgproc_mutex); + for (auto& handler : m_handlers) { if (auto i = dynamic_cast<CValidationInterface*>(handler.get()); i != nullptr) { UnregisterValidationInterface(i); } handler->Stop(); } }
1683-1688: InterruptHandlers: optional lock while traversing.Protect against concurrent Remove/Add, or document that it’s single-threaded in shutdown.
void PeerManagerImpl::InterruptHandlers() { - for (auto& handler : m_handlers) { + LOCK(g_msgproc_mutex); + for (auto& handler : m_handlers) { handler->Interrupt(); } }
🧹 Nitpick comments (2)
src/net_processing.cpp (2)
598-601: Destructor ordering: assert/document quiescence before RemoveHandlers.Safe if msghand is already stopped; otherwise potential UAF vs ProcessMessage iterating m_handlers. Add a brief comment (or a debug assert) to encode the ordering guarantee.
Would you confirm RemoveHandlers() only runs after ThreadMessageHandler has been joined in PrepareShutdown()? A short comment here would help future readers. Based on learnings.
6525-6553: Wrappers: fine, but align impl annotations or lock where required.
- PeerMisbehaving/Relay* wrappers are straightforward.
- PeerEraseObjectRequest impl doesn’t lock; acceptable since the declaration requires cs_main. For consistency, consider adding the same annotation on the definition, or wrap with WITH_LOCK(::cs_main, ...) to harden the API.
If you keep the current contract, please confirm all callers hold cs_main (script in earlier comment). Based on learnings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/init.cpp(5 hunks)src/net_processing.cpp(6 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 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/net_processing.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.cppsrc/net_processing.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-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.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.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.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/net_processing.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/net_processing.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/net_processing.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/net_processing.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/net_processing.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/net_processing.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/net_processing.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.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/net_processing.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/net_processing.cpp
🧬 Code graph analysis (2)
src/init.cpp (2)
src/interfaces/node.h (2)
node(42-44)node(404-404)src/init.h (1)
node(23-25)
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)
⏰ 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). (7)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (6)
src/init.cpp (4)
90-90: LGTM! Include properly placed among related headers.The new include for NetInstantSend is appropriately positioned with other InstantSend and LLMQ headers.
254-256: LGTM! InterruptHandlers correctly placed in Interrupt().The handler interrupt is properly sequenced in the shutdown flow and includes appropriate null checking.
294-294: LGTM! StopHandlers appropriately positioned in shutdown sequence.The handler stop is correctly ordered between active context and LLMQ context stops, with proper null checking.
2296-2296: LGTM! StartHandlers correctly ordered after llmq_ctx initialization.The handler start is properly sequenced after
llmq_ctx->Start()(line 2295), ensuring dependencies are initialized before handlers that use them begin processing.src/net_processing.cpp (2)
5446-5448: LGTM: handler dispatch is serialized.Invoking external handlers under g_msgproc_mutex matches the existing message-processing concurrency model.
653-659: All call sites comply with cs_main lock requirement.Verification found only one call site in
src/instantsend/net_instantsend.cpp:30, which correctly acquires::cs_mainviaWITH_LOCK(::cs_main, ...)before invokingPeerEraseObjectRequest(). No violations detected.
| g_active_notification_interface = std::make_unique<ActiveNotificationInterface>(*node.active_ctx, *node.mn_activeman); | ||
| RegisterValidationInterface(g_active_notification_interface.get()); | ||
| } | ||
| node.peerman->AddExtraHandler(std::make_unique<NetInstantSend>(node.peerman.get(), *node.llmq_ctx->isman, *node.llmq_ctx->qman, chainman.ActiveChainstate())); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify llmq_ctx initialization before dereferencing.
The code dereferences node.llmq_ctx->isman and node.llmq_ctx->qman without checking if llmq_ctx is null. While the pattern throughout this section suggests llmq_ctx is expected to be initialized by this point, consider adding an assertion to make the assumption explicit:
assert(node.llmq_ctx);
node.peerman->AddExtraHandler(std::make_unique<NetInstantSend>(node.peerman.get(), *node.llmq_ctx->isman, *node.llmq_ctx->qman, chainman.ActiveChainstate()));This would make the code's expectations clear and provide better diagnostics if the assumption is violated.
Based on learnings
🏁 Script executed:
#!/bin/bash
# Description: Verify llmq_ctx initialization before line 2200 in init.cpp
# Expected: Find where llmq_ctx is initialized or asserted before use
# Search for llmq_ctx initialization/creation in the initialization flow
rg -n -B5 -A2 'llmq_ctx\s*=' src/init.cpp
# Search for assertions on llmq_ctx
rg -n 'Assert.*llmq_ctx|assert.*llmq_ctx' src/init.cpp
# Check how llmq_ctx is used elsewhere for patterns
rg -n -C3 'node\.llmq_ctx->' src/init.cpp | head -50Length of output: 2417
Looking at the script output, I need to understand the Node structure definition and verify where llmq_ctx is initialized before it's dereferenced. Let me search for more context.
🏁 Script executed:
#!/bin/bash
# Find Node structure definition to see how llmq_ctx is declared
fd -e h -e hpp | xargs rg -l 'struct Node|class Node' | head -5
# Search for where llmq_ctx is assigned in init.cpp
rg -n 'node\.llmq_ctx\s*=' src/init.cpp
# Find assertions or checks at start of InitializeNodeContext function
rg -n -A10 'void InitializeNodeContext' src/init.cpp | head -30Length of output: 107
🏁 Script executed:
#!/bin/bash
# Get context around line 2100 to understand the function structure before line 2172
rg -n -B50 'node\.cj_walletman = CJWalletManager' src/init.cpp | tail -60Length of output: 2444
🏁 Script executed:
#!/bin/bash
# Find Node structure definition
fd 'node\.h' | xargs cat 2>/dev/null | grep -A50 'struct Node\|llmq_ctx' | head -80Length of output: 1995
🏁 Script executed:
#!/bin/bash
# Check context around line 2100-2200 to see if there's an early guard or assertion
rg -n -B80 'node\.cj_walletman = CJWalletManager::make' src/init.cpp | head -100Length of output: 4889
🏁 Script executed:
#!/bin/bash
# Look for the start of InitializeNodeContext function to understand preconditions
rg -n 'void InitializeNodeContext\(' src/init.cpp -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the actual Node struct definition
rg -n 'struct NodeContext\s*{' src/node/context.h -A100 | head -120Length of output: 107
🏁 Script executed:
#!/bin/bash
# Look for where llmq_ctx is created/initialized in init.cpp
rg -n 'llmq_ctx.*make_unique|llmq_ctx.*std::' src/init.cppLength of output: 165
🏁 Script executed:
#!/bin/bash
# Check what happens before line 2164 to see initialization flow
rg -n -B100 'ChainstateManager& chainman = \*Assert' src/init.cpp | grep -A100 'Step 4\|Step 5\|Step 6' | head -80Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find Node struct with simpler pattern
grep -n 'struct Node' src/node/context.h | head -5
# Look for llmq_ctx member declaration
grep -n 'llmq_ctx' src/node/context.h
# Find the function containing line 2200 and its start
rg -n '^bool AppInitializeNodeContext' src/init.cppLength of output: 69
🏁 Script executed:
#!/bin/bash
# Look for early checks before line 2172 in the function
sed -n '1,2172p' src/init.cpp | tail -100 | head -60Length of output: 3909
🏁 Script executed:
#!/bin/bash
# Look for where llmq_ctx is initialized/created prior to this function
rg -n 'node.llmq_ctx = ' src/init.cppLength of output: 0
🏁 Script executed:
#!/bin/bash
# Find which function contains line 2200
sed -n '1900,2200p' src/init.cpp | grep -n 'bool \|void ' | tail -3Length of output: 189
🏁 Script executed:
#!/bin/bash
# Look for LLMQContext creation/initialization
rg -n 'LLMQContext\(' src/ -g '*.cpp' -g '*.h'Length of output: 4254
🏁 Script executed:
#!/bin/bash
# Check if there's any Assert on llmq_ctx before line 2172
rg -n -B200 'CJWalletManager::make.*llmq_ctx' src/init.cpp | grep -E 'Assert.*llmq_ctx|assert.*llmq_ctx|llmq_ctx.*Start|Step [0-9]'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the function containing line 2200 by searching backwards from that line
sed -n '1,2200p' src/init.cpp | grep -n '^bool \|^void ' | tail -1Length of output: 85
🏁 Script executed:
#!/bin/bash
# Look for where LLMQContext is instantiated/created in init.cpp
rg -n 'make_unique<LLMQContext>|std::make_unique.*LLMQContext' src/init.cppLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check LoadChainstate signature to see if llmq_ctx is a parameter
rg -n 'ChainstateLoadingError LoadChainstate' src/init.cpp -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for any setup/creation of llmq_ctx before the main initialization function
rg -n 'llmq_ctx' src/init.cpp | head -20Length of output: 1240
🏁 Script executed:
#!/bin/bash
# Search within AppInitMain function (lines 1485-2200+) for llmq_ctx setup
sed -n '1485,2010p' src/init.cpp | grep -n 'llmq_ctx'Length of output: 64
🏁 Script executed:
#!/bin/bash
# Look for where NodeContext is populated or llmq_ctx is created outside init.cpp
rg -n 'node\.llmq_ctx.*=' src/ -g '*.cpp' | grep -v test | grep -v rpcLength of output: 241
🏁 Script executed:
#!/bin/bash
# Check if llmq_ctx is created in AppInitializeNodeContext or similar
rg -n 'AppInitialize' src/init.cpp | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for Assert calls with llmq_ctx anywhere in init.cpp
rg -n 'Assert.*llmq_ctx' src/init.cppLength of output: 79
Add explicit assertion for llmq_ctx to document precondition.
The code at line 2200 dereferences node.llmq_ctx->isman and node.llmq_ctx->qman without checking if llmq_ctx is null. While the initialization flow ensures llmq_ctx is created before AppInitMain is called and remains non-null throughout this phase (evidenced by similar dereferences at lines 2172, 2186, 2195, 2295 without checks), there is no explicit assertion to document this precondition.
Adding an assertion makes the assumption clear and provides better diagnostics:
assert(node.llmq_ctx);
node.peerman->AddExtraHandler(std::make_unique<NetInstantSend>(node.peerman.get(), *node.llmq_ctx->isman, *node.llmq_ctx->qman, chainman.ActiveChainstate()));This assertion should be placed before the first dereference of llmq_ctx members in AppInitMain (e.g., before line 2172).
🤖 Prompt for AI Agents
In src/init.cpp around lines 2172–2200, the code dereferences node.llmq_ctx
(e.g., node.llmq_ctx->isman and ->qman) without an explicit check; add an
assertion documenting the precondition by inserting assert(node.llmq_ctx);
before the first dereference in AppInitMain (before line 2172) so subsequent
uses (including the call at line 2200) are guarded by a clear diagnostic.
…ogic and consensus
…nstead and tidy up them a bit
knst
force-pushed
the
refactor-peermanager-handlers-is
branch
from
abed72d to
d4d64f6
Compare
I rebased on top of develop to fix CI issue |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/instantsend/net_instantsend.h (1)
36-38: Stop() must self-interrupt before joining the worker.This issue was previously flagged:
Stop()can hang indefinitely ifInterrupt()wasn't called before it, because the worker loop waits onworkInterruptto exit. The current implementation doesn't makeStop()idempotent or self-sufficient.The past review comment correctly identifies this as a critical lifecycle issue and proposes calling
workInterrupt()insideStop()before joining to ensure the worker exits safely.
🧹 Nitpick comments (6)
src/net_processing.h (1)
58-68: NetHandler interface is useful, but its destructor calling virtuals is misleadingThe
PeerManagerInternal+NetHandlersplit gives external handlers a clean, minimal surface (misbehavior, inv relay, tx fetch) and the new handler lifecycle methods onPeerManagerare a good fit.However, this destructor:
virtual ~NetHandler() { Interrupt(); Stop(); }won’t dispatch to derived overrides (C++ only calls base implementations inside base dtors), and
PeerManagerImpl::RemoveHandlers()already callsInterruptHandlers()/StopHandlers()before clearing the handlers. To avoid confusion and duplicate shutdown paths, consider simplifying:- virtual ~NetHandler() { - Interrupt(); - Stop(); - } + virtual ~NetHandler() = default;and rely on the owning
PeerManagerto manage the lifecycle explicitly.Also applies to: 69-85, 87-87, 166-170
src/instantsend/instantsend.h (2)
46-49: PendingState payload aligns with FetchPendingLocks batchingUsing
std::vector<std::pair<uint256, PendingISLockFromPeer>> m_pending_ismatches the implementation inFetchPendingLocks()where(islockHash, PendingISLockFromPeer)pairs are accumulated and later consumed byNetInstantSend, preserving the existing “up to 32 locks + more_work flag” batching semantics.If you want to tighten readability a bit, consider a more descriptive member name like
m_pending_locksto reflect the contents more clearly, but this is purely cosmetic.
111-111: Consider making Signer()’s atomic load use an explicit memory order
ConnectSigner/DisconnectSignerusestd::memory_order_acquire/std::memory_order_releaseonm_signer, whileSigner()callsm_signer.load()with the defaultmemory_order_seq_cst. This is functionally fine, but you might want to make the ordering explicit here for consistency and documentation of the intended acquire‑release pattern, e.g.:- instantsend::InstantSendSigner* Signer() const { return m_signer.load(); } + instantsend::InstantSendSigner* Signer() const + { + return m_signer.load(std::memory_order_acquire); + }Not required, but it would make the synchronization discipline clearer to readers and tools.
src/net_processing.cpp (3)
598-602: Destructor-based handler cleanup is reasonable but document ordering assumptionsCalling
RemoveHandlers()in~PeerManagerImpl()is a good safety net if init/shutdown paths ever miss explicit cleanup. This relies on the existing guarantee that all message-handling threads are stopped and no furtherProcessMessagecalls can happen before thePeerManagerImpldestructor runs. To avoid future refactors accidentally violating that assumption, consider adding a brief comment near the destructor (or wherenode.peermanis reset in init.cpp) stating that the message handler thread must be fully joined beforePeerManagerImpldestruction.
646-651: NetHandler container and API shape look good; consider clarifying lifetime/usage contractThe
AddExtraHandler/RemoveHandlers/StartHandlers/StopHandlers/InterruptHandlersAPI plus thestd::vector<std::unique_ptr<NetHandler>> m_handlersstorage is a clean way to host external network handlers. Givenm_handlersis iterated underg_msgproc_mutexinProcessMessagebut mutated only from init/shutdown paths, it would help future readers if you briefly document thatm_handlersis only modified before the message handler thread starts and after it is joined (i.e., these methods are not meant to be called concurrently with normal message processing). A short comment onm_handlersor on the method declarations would be enough.Also applies to: 1091-1091
5446-5448: External NetHandlers sharevRecv; ensure interface expectations are clearDispatching to each
NetHandlerviahandler->ProcessMessage(pfrom, msg_type, vRecv)integrates well with the existing extension pattern (spork/governance/LLMQ modules). Because all handlers share the sameCDataStream& vRecv, it’s important that NetHandler implementations follow the same convention (only reading whenmsg_typematches and behaving correctly given the current stream position). If not already spelled out inNetHandler’s declaration, consider documenting this expectation there.
📜 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(6 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 (1)
- src/instantsend/net_instantsend.cpp
🧰 Additional context used
🧠 Learnings (27)
📓 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-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/instantsend/net_instantsend.hsrc/instantsend/instantsend.cppsrc/llmq/context.cppsrc/instantsend/instantsend.h
📚 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/instantsend/instantsend.cppsrc/net_processing.cpptest/lint/lint-circular-dependencies.pysrc/instantsend/instantsend.h
📚 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/instantsend.cppsrc/instantsend/instantsend.h
📚 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/instantsend.cppsrc/net_processing.cpptest/lint/lint-circular-dependencies.pysrc/instantsend/instantsend.h
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/instantsend/instantsend.cpp
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Applied to files:
src/instantsend/instantsend.cpptest/lint/lint-circular-dependencies.py
📚 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/instantsend.cppsrc/net_processing.cpptest/lint/lint-circular-dependencies.py
📚 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/instantsend.cppsrc/net_processing.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/instantsend.cppsrc/net_processing.cpp
📚 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-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/net_processing.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.cppsrc/net_processing.cpp
📚 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: In src/net_processing.cpp, if ActiveContext (m_active_ctx) is non-null, its members (including cj_server) are guaranteed to be initialized; call sites can safely dereference m_active_ctx->cj_server without an additional null-check.
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-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.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.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/net_processing.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/net_processing.cpptest/lint/lint-circular-dependencies.py
📚 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/net_processing.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.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-09-09T21:36:31.504Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6837
File: src/stats/rawsender.cpp:0-0
Timestamp: 2025-09-09T21:36:31.504Z
Learning: In RawSender class (src/stats/rawsender.cpp), m_sock access requires synchronization using the cs_net mutex. The race condition exists between SendDirectly() (callable from any thread) and Connect() (called by ReconnectThread), where both read and write m_sock concurrently. The cs_net mutex properly coordinates these network operations.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/net_processing.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/net_processing.cppsrc/instantsend/instantsend.h
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-05-10T00:54:30.887Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/netinfo.h:248-256
Timestamp: 2025-05-10T00:54:30.887Z
Learning: The Dash codebase includes a specialization for serializing std::shared_ptr objects in src/serialize.h, which allows direct serialization of shared_ptr instances with stream operators.
Applied to files:
src/instantsend/instantsend.h
🧬 Code graph analysis (5)
src/net_processing.h (2)
src/net_processing.cpp (67)
PeerMisbehaving(6525-6528)PeerMisbehaving(6525-6525)pnode(638-638)pnode(643-643)pnode(654-654)pnode(735-735)pnode(779-779)pnode(851-851)pnode(853-853)PeerEraseObjectRequest(6530-6533)PeerEraseObjectRequest(6530-6530)nodeid(628-628)nodeid(631-631)nodeid(644-644)nodeid(655-655)nodeid(675-675)nodeid(677-677)nodeid(715-716)nodeid(726-726)nodeid(858-858)nodeid(860-860)nodeid(1036-1036)nodeid(1071-1071)inv(632-632)inv(633-633)inv(656-656)inv(657-657)inv(658-658)inv(667-667)inv(673-673)inv(961-961)inv(2383-2383)PeerRelayInv(6535-6538)PeerRelayInv(6535-6535)PeerRelayInvFiltered(6540-6543)PeerRelayInvFiltered(6540-6540)PeerRelayInvFiltered(6545-6548)PeerRelayInvFiltered(6545-6545)PeerAskPeersForTransaction(6550-6553)PeerAskPeersForTransaction(6550-6550)txid(634-634)txid(659-659)txid(661-661)txid(664-664)ProcessMessage(3665-5456)ProcessMessage(3665-3670)pfrom(618-618)pfrom(639-640)pfrom(749-751)pfrom(760-760)pfrom(768-768)pfrom(771-771)pfrom(773-773)pfrom(775-775)pfrom(871-871)pfrom(1053-1053)AddExtraHandler(1650-1657)AddExtraHandler(1650-1650)handler(647-647)RemoveHandlers(1659-1664)RemoveHandlers(1659-1659)StartHandlers(1666-1671)StartHandlers(1666-1666)StopHandlers(1673-1681)StopHandlers(1673-1673)InterruptHandlers(1683-1688)InterruptHandlers(1683-1683)src/init.cpp (3)
Interrupt(241-270)Interrupt(241-241)handler(492-492)
src/instantsend/net_instantsend.h (2)
src/instantsend/instantsend.h (1)
instantsend(38-49)src/net_processing.h (1)
PeerManagerInternal(58-67)
src/instantsend/instantsend.cpp (3)
src/logging.h (1)
LogAcceptDebug(235-238)src/net_processing.cpp (4)
hash(1024-1024)hash(1030-1030)ret(681-681)tx(1082-1082)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/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.h (4)
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)src/instantsend/net_instantsend.h (1)
instantsend(12-16)src/instantsend/lock.h (1)
instantsend(18-41)src/llmq/signing.cpp (2)
CSigningManager(335-341)CSigningManager(343-343)
⏰ 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: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (19)
src/Makefile.am (1)
255-255: NetInstantSend build wiring looks correct
instantsend/net_instantsend.handinstantsend/net_instantsend.cppare added consistently alongside other InstantSend headers/sources inBITCOIN_CORE_Handlibbitcoin_node_a_SOURCES, so the new component is properly exposed to the node library.Also applies to: 527-527
test/lint/lint-circular-dependencies.py (1)
26-27: Updated expected circular dependencies align with new routingThe added cycles (chainlock/chainlock and instantsend/instantsend paths via
chainlock/signing/instantsend/signing→llmq/signing→net_processing) and the new coinjoin-related routes match the refactored dependency graph. As long ascircular-dependencies.pyruns clean on this branch, this list is consistent with the tooling.Also applies to: 32-34, 56-57
src/llmq/context.cpp (1)
35-36: InstantSendManager dependency change is consistent with LLMQContext wiringConstructing
CInstantSendManagerwith*sigmaninstead of*qmancleanly drops the direct quorum manager dependency while preserving the overall initialization order (qman→sigman→clhandler→isman). This fits the goal of separating consensus from networking without altering the existing start/stop ordering.src/init.cpp (1)
90-90: NetInstantSend registration and handler lifecycle are ordered sensibly
- Including
<instantsend/net_instantsend.h>and registeringNetInstantSendviaAddExtraHandlerafterpeerman/llmq_ctxconstruction but beforellmq_ctx->Start()/StartHandlers()ensures the handler is fully wired before network threads start.- The shutdown flow (
InterruptHandlers()inInterrupt(),StopHandlers()inPrepareShutdown()beforellmq_ctx->Stop()and beforepeerman.reset()) gives NetHandlers a chance to quiesce their own threads/work before their dependencies are torn down.- This keeps InstantSend networking cleanly separated while matching the existing LLMQ/ChainLocks lifecycle.
Also applies to: 254-256, 293-295, 2022-2022, 2298-2298
src/instantsend/net_instantsend.h (2)
25-33: Constructor initialization order matches declaration order.The member initialization list correctly follows the declaration order in the class body (lines 48-53), avoiding undefined behavior from out-of-order initialization.
38-38: Interrupt() implementation is concise and correct.The inline implementation of
Interrupt()correctly signals theworkInterruptto wake the worker thread.src/instantsend/instantsend.cpp (7)
7-17: New includes support the refactored interface.The added includes (
chainlock/chainlock.h,node/blockstorage.h,txmempool.h,validation.h,validationinterface.h) are necessary to support the refactoredCInstantSendManagerthat now directly manages chain state interactions previously handled through other managers.
51-62: Constructor correctly removes CQuorumManager dependency.The refactored constructor signature drops
CQuorumManager&and usesCChainLocksHandler&instead, aligning with the decoupling objectives. The initialization list correctly initializes all members.
66-66: Free function conversion is appropriate.Converting
ShouldReportISLockTiming()to a free function is correct since it doesn't access member state. The implementation logic is preserved.
68-90: EnqueueInstantSendLock correctly extracts network enqueuing logic.The new method properly:
- Maintains timing measurements for performance tracking
- Uses
LOCK(cs_pendingLocks)for thread-safe access- Efficiently emplaces into
pendingInstantSendLocksThis extraction is consistent with the PR's goal of separating network handling from consensus logic.
92-119: FetchPendingLocks correctly batches and extracts pending locks.The refactored method:
- Properly limits batch size to 32 to avoid duplicate BLS verification overhead
- Optimizes memory allocation with
std::min(maxCount, pendingInstantSendLocks.size())(line 102)- Uses
LOCK(cs_pendingLocks)for thread safety- Returns both the work indicator and the batch of locks
The logic is preserved as confirmed in the past review discussion.
121-194: ProcessInstantSendLock correctly uses std::variant for cleaner return semantics.The refactored return type
std::variant<uint256, CTransactionRef, std::monostate>provides clear semantics:
std::monostate: Lock already known or chainlocked (no relay needed)CTransactionRef: Transaction found, relay with full txuint256: Transaction pending, relay with txid onlyThe internal logic is preserved and the variant usage is correct throughout.
366-382: PrepareTxToRetry correctly extracts retry preparation logic.The new method properly:
- Uses
LOCK2(cs_nonLocked, cs_pendingRetry)to safely access both data structures- Pre-reserves vector capacity for efficiency
- Checks
tx_info.txexistence before collectingThis extraction is consistent with removing the worker thread from
CInstantSendManager.src/instantsend/instantsend.h (4)
8-11: InstantSend includes now match header’s direct type usagePulling in
instantsend/db.h,instantsend/lock.h, andinstantsend/signing.hhere is appropriate given the by‑valueinstantsend::CInstantSendDbmember and the use ofInstantSendLock/InstantSendSignertypes in this header. This keeps the header self‑contained and consistent with the new design.
22-22: Correctly add<variant>for ProcessInstantSendLock’s return typeIncluding
<variant>in the header matches the newstd::variant<uint256, CTransactionRef, std::monostate>return type and removes reliance on transitive includes; this is the right place for it.
98-100: Ctor signature change cleanly removes direct CQuorumManager dependencyThe updated constructor
explicit CInstantSendManager(CChainLocksHandler& _clhandler, CChainState& chainstate, CSigningManager& _sigman, CSporkManager& sporkman, CTxMemPool& _mempool, const CMasternodeSync& mn_sync, const util::DbWrapperParams& db_params);matches the definition in
instantsend.cppand the wayCInstantSendManageris now wired viaCSigningManagerrather thanCQuorumManager, in line with the PR’s decoupling goal. Parameter passing by (non‑const) reference andconst&forDbWrapperParamslook correct for ownership and lifetime.
135-145: NetInstantSend bridge helpers are well‑factored and annotatedThe new helper group (
FetchPendingLocks,EnqueueInstantSendLock,PrepareTxToRetry,Sigman,ProcessInstantSendLock) provides a clean, minimal surface forNetInstantSendto interact withCInstantSendManager:
FetchPendingLocks()andPrepareTxToRetry()are marked[[nodiscard]]and return value types that match their implementations ininstantsend.cpp.EnqueueInstantSendLock()takesstd::shared_ptr<InstantSendLock>by value, which is idiomatic for sink parameters and aligns with prior review discussion.Sigman()exposes the existingCSigningManager&dependency needed on the networking side without changing ownership.ProcessInstantSendLock()’sstd::variant<uint256, CTransactionRef, std::monostate>return type allows the caller to distinguish “have tx”, “only txid”, and “no further action” cleanly, and the thread‑safety annotations match the internal locking behavior shown in the .cpp.Overall this section aligns with the PR’s goal of separating network and consensus/chain logic without touching InstantSend’s consensus behavior. Based on learnings.
src/net_processing.cpp (2)
1650-1688: Handler lifecycle covers validation registration correctly; idempotence looks safe
AddExtraHandlerregistering aCValidationInterface(if present) andStopHandlersunregistering it before stopping the handler gives a clean lifecycle, andRemoveHandlers()(Interrupt → Stop → clear) is idempotent and safe even if called multiple times (e.g., from shutdown and the destructor). No changes needed from a correctness perspective.
6525-6553: PeerManagerInternal forwarding wrappers correctly preserve locking contractsThe new
PeerMisbehaving,PeerEraseObjectRequest,PeerRelayInv*, andPeerAskPeersForTransactionwrappers are straightforward and preserve the underlying methods’ lock requirements (includingEXCLUSIVE_LOCKS_REQUIRED(::cs_main)forPeerEraseObjectRequest). This gives external handlers a clean, internal API without duplicating logic. No further changes needed.
3 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: