fix: use std::atomic to protect connected manager/signer pointers, check before queueman access, update P2P message map in tests
#6842
Conversation
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughMultiple subsystems replace raw manager/signer pointers with std::atomic pointers and switch to explicit acquire/release loads/stores with local loaded copies before use. Files changed include chainlock, instantsend, evo/mnhftx (adds DisconnectManagers and moves includes), txmempool (adds DisconnectManagers, changes addUncheckedProTx signature), and txmempool.h/mnhftx.h/chainlock.h/instantsend.h type updates. net_processing.cpp and rpc/coinjoin.cpp add null checks around CoinJoin queue manager under ENABLE_WALLET. Tests: test/functional/test_framework/p2p.py reorders and expands MESSAGEMAP entries (adds many Dash/Q placeholders and restores getheaders2/headers2 handlers). Public APIs largely unchanged except for added DisconnectManagers declarations and the updated addUncheckedProTx signature. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/txmempool.cpp (1)
935-958: Guard against nullptr after disconnect in removeProTxSpentCollateralConflicts()This function re-loads with Assert and can abort if
DisconnectManagers()races. Prefer a tolerant early-return.- auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire)); + auto dmnman = m_dmnman.load(std::memory_order_acquire); + if (!dmnman) return;
🧹 Nitpick comments (13)
src/instantsend/instantsend.h (1)
100-104: Use CAS in ConnectSigner to prevent double-init racesReplace the current assert+store with a compare_exchange_strong to guard against concurrent ConnectSigner calls:
void ConnectSigner(gsl::not_null<instantsend::InstantSendSigner*> signer) { - // Prohibit double initialization - assert(m_signer.load(std::memory_order_acquire) == nullptr); - m_signer.store(signer, std::memory_order_release); + // Prohibit double initialization (thread-safe) + auto* expected = static_cast<instantsend::InstantSendSigner*>(nullptr); + const bool ok = m_signer.compare_exchange_strong( + expected, signer, std::memory_order_release, std::memory_order_relaxed); + assert(ok && "InstantSend signer double-initialization"); }Verified that all m_signer.load calls already use std::memory_order_acquire.
src/chainlock/chainlock.h (1)
76-80: Use compare_exchange_strong in ConnectSigner for thread safety
All m_signer.load calls use memory_order_acquire, so switching to a CAS in ConnectSigner is safe and prevents concurrent double-initialization.src/chainlock/chainlock.cpp (2)
222-228: Same race-narrowing suggestion in UpdatedBlockTip’s immediate task.Reload right before
TrySignChainTipto minimize stale-pointer exposure.- auto signer = m_signer.load(std::memory_order_acquire); CheckActiveState(); EnforceBestChainLock(); Cleanup(); - if (signer) { - signer->TrySignChainTip(isman); + if (auto signer = m_signer.load(std::memory_order_acquire)) { + signer->TrySignChainTip(isman); }
67-69: Reloadm_signerimmediately beforeTrySignChainTipin the scheduled lambda
Allm_signer.storecalls usememory_order_release(in bothsrc/chainlock/chainlock.handsrc/instantsend/instantsend.h), and the signer is owned byActiveContext’sunique_ptr, withDisconnectSigner()invoked before it’s destroyed. Apply:- [&]() { - auto signer = m_signer.load(std::memory_order_acquire); + [&]() { CheckActiveState(); EnforceBestChainLock(); Cleanup(); // regularly retry signing the current chaintip as it might have failed before due to missing islocks - if (signer) { - signer->TrySignChainTip(isman); + if (auto signer = m_signer.load(std::memory_order_acquire)) { + signer->TrySignChainTip(isman); } },src/instantsend/instantsend.cpp (1)
928-947: Reloadm_signerat the call site to reduce race window in the worker loop.
ProcessPendingInstantSendLocks()and the tx collection can take time; loading the signer up front increases the chance of a stale pointer beforeProcessPendingRetryLockTxs.Confirm release stores on connects/disconnects to pair with these acquire loads.
- auto signer = m_signer.load(std::memory_order_acquire); - if (!signer) return more_work; // Construct set of non-locked transactions that are pending to retry std::vector<CTransactionRef> txns{}; { LOCK2(cs_nonLocked, cs_pendingRetry); if (pendingRetryTxs.empty()) return more_work; txns.reserve(pendingRetryTxs.size()); for (const auto& txid : pendingRetryTxs) { if (auto it = nonLockedTxs.find(txid); it != nonLockedTxs.end()) { const auto& [_, tx_info] = *it; if (tx_info.tx) { txns.push_back(tx_info.tx); } } } } // Retry processing them - signer->ProcessPendingRetryLockTxs(txns); + if (auto signer = m_signer.load(std::memory_order_acquire)) { + signer->ProcessPendingRetryLockTxs(txns); + } else { + return more_work; + }Optional: factor a small helper to reduce repetition:
template<typename F> void WithSigner(F&& f) { if (auto s = m_signer.load(std::memory_order_acquire)) f(*s); }src/evo/mnhftx.h (2)
97-99: Atomic pointers: add explicit lifetime/ordering contract.The move to
std::atomic<T*>is fine, but please document:
- Stores must use
memory_order_release; loads here and in .cpp should usememory_order_acquire.- The pointed-to objects must outlive any in-flight calls after a successful load, or disconnection must be sequenced so that no threads can still observe/use the old pointer.
Optionally, publish both manager pointers consistently:
- If callers need both non-null simultaneously, consider an atomic of a small struct holding both pointers (or an atomic
shared_ptr<Managers>), to avoid observing one updated and the other stale.If helpful, I can draft the struct approach; let me know.
146-151: Doc nit: update comment to reflect both pointers.The comment still says “Reset llmq::CQuorumManager pointer,” but
DisconnectManagers()now resets bothChainstateManagerandllmq::CQuorumManager.- * Reset llmq::CQuorumManager pointer. + * Reset ChainstateManager and llmq::CQuorumManager pointers.src/txmempool.h (2)
593-593: Out-of-line DisconnectManagers(): LGTM; consider noexcept and lock-annotationThe declaration looks good. Since the implementation just clears atomics, mark it noexcept and (optionally) add a LOCKS_EXCLUDED(cs) annotation for consistency with nearby APIs.
- void DisconnectManagers(); + void DisconnectManagers() noexcept; // LOCKS_EXCLUDED(cs)
438-440: Document atomic manager pointer concurrency contract
Add a brief comment above the declarations of m_dmnman and m_isman stating they are non-owning pointers, that reads must use .load(std::memory_order_acquire), writes must use .store(..., std::memory_order_release), and that DisconnectManagers() must be called during a quiescent shutdown phase to avoid UAF. For example:// Non-owning. Concurrency contract: // - Reads: .load(std::memory_order_acquire) // - Writes: .store(..., std::memory_order_release) // - Lifetime: DisconnectManagers() must be called before destruction when no readers remain. std::atomic<CDeterministicMNManager*> m_dmnman{nullptr}; std::atomic<llmq::CInstantSendManager*> m_isman{nullptr};src/net_processing.cpp (1)
2279-2284: DSQ: added nullptr-check for CJ queue manager — good defensive fixPrevents dereferencing a null
queuemanwhen wallets are enabled; fallback to server path remains intact.You can extract this DSQ “have” check into a tiny helper to avoid duplicating the wallet/server logic in multiple places (see also Lines 2888-2891).
src/evo/mnhftx.cpp (1)
377-381: Harden double-initialization check with CAS (optional)
assert(m_chainman.load(...) == nullptr)is fine under the init contract, but a compare_exchange makes the guard robust if this ever gets called twice.Apply:
-assert(m_chainman.load(std::memory_order_acquire) == nullptr); -m_chainman.store(chainman, std::memory_order_release); -assert(m_qman.load(std::memory_order_acquire) == nullptr); -m_qman.store(qman, std::memory_order_release); +ChainstateManager* expected_cm{nullptr}; +bool cm_set = m_chainman.compare_exchange_strong(expected_cm, chainman, + std::memory_order_release, std::memory_order_relaxed); +assert(cm_set); +llmq::CQuorumManager* expected_qm{nullptr}; +bool qm_set = m_qman.compare_exchange_strong(expected_qm, qman, + std::memory_order_release, std::memory_order_relaxed); +assert(qm_set);src/txmempool.cpp (2)
443-447: Make initialization atomic and race-safe (optional)Two concurrent
ConnectManagers()calls could both pass the asserts. Use CAS to enforce single-initialization per pointer.- assert(m_dmnman.load(std::memory_order_acquire) == nullptr); - m_dmnman.store(dmnman, std::memory_order_release); - assert(m_isman.load(std::memory_order_acquire) == nullptr); - m_isman.store(isman, std::memory_order_release); + CDeterministicMNManager* expected_dmn{nullptr}; + bool ok1 = m_dmnman.compare_exchange_strong(expected_dmn, dmnman, std::memory_order_acq_rel); + assert(ok1); + llmq::CInstantSendManager* expected_is{nullptr}; + bool ok2 = m_isman.compare_exchange_strong(expected_is, isman, std::memory_order_acq_rel); + assert(ok2);
449-453: Optional: use exchange(acq_rel) in DisconnectManagers and document lifetime
ConnectManagers/DisconnectManagers are only invoked in node/chainstate.cpp during startup/shutdown—serialized by the node init/shutdown barrier, so no in-flight mempool ops can race with DisconnectManagers.- m_dmnman.store(nullptr, std::memory_order_release); - m_isman.store(nullptr, std::memory_order_release); + m_dmnman.exchange(nullptr, std::memory_order_acq_rel); + m_isman.exchange(nullptr, std::memory_order_acq_rel);Add a comment in txmempool.h next to DisconnectManagers stating that the managers must outlive any mempool operations to avoid UAF.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/chainlock/chainlock.cpp(5 hunks)src/chainlock/chainlock.h(2 hunks)src/evo/mnhftx.cpp(3 hunks)src/evo/mnhftx.h(3 hunks)src/instantsend/instantsend.cpp(7 hunks)src/instantsend/instantsend.h(2 hunks)src/net_processing.cpp(3 hunks)src/rpc/coinjoin.cpp(1 hunks)src/txmempool.cpp(12 hunks)src/txmempool.h(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: knst
PR: dashpay/dash#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.
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/rpc/coinjoin.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/instantsend/instantsend.cppsrc/chainlock/chainlock.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/txmempool.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/mnhftx.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/chainlock/chainlock.h
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
PR: dashpay/dash#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
⏰ 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). (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (22)
src/instantsend/instantsend.h (1)
62-63: Atomic pointer introduction looks goodThe atomic pointer with nullptr init is appropriate for cross-thread visibility.
src/chainlock/chainlock.h (1)
49-50: Atomic signer pointer change LGTMMatches the pattern used elsewhere; nullptr init is correct.
src/chainlock/chainlock.cpp (4)
87-89: Start/Stop sequencing looks correct.Stopping the scheduler before calling
signer->Stop()prevents new callbacks from racing with shutdown.
279-281: LGTM: guarded access to signer in BlockConnected.Uses an acquired local and checks for null before use.
287-289: LGTM: guarded access to signer in BlockDisconnected.Null-checked atomic load is correct.
456-458: Nice follow-up: safeCleanup()call on signer.This aligns with the previously deferred null-check concern for
ChainLockSigner::Cleanup()and prevents crashes when disconnected. Thanks for addressing it.src/instantsend/instantsend.cpp (7)
78-80: LGTM: start signer via atomic-loaded local.Pattern matches the new atomic ownership model.
85-87: LGTM: guardedStop()call.No issues.
351-353: LGTM: clear lock from queue only when signer present.Prevents spurious derefs.
452-454: LGTM: process mempool tx through signer when available.Reasonable fallback to non-locked tracking when absent.
493-496: LGTM: guardedProcessTxon block connect.Consistent with new pattern.
601-602: LGTM: clear inputs from queue on conflict.Correct null-checked usage.
609-610: LGTM: truncate inputs after clearing signer queue.Order makes sense.
src/evo/mnhftx.h (1)
8-9: LGTM: include adjustments for new atomic/deps.No issues spotted.
Also applies to: 11-17, 19-19
src/net_processing.cpp (2)
5276-5278: Message dispatch: gate CoinJoin queue manager calls on presence — LGTMAvoids calling
ProcessMessageon a nullqueueman.
2888-2891: No unguarded m_cj_ctx->queueman dereferences detected. All occurrences ofm_cj_ctx->queuemanare preceded by null checks.src/evo/mnhftx.cpp (3)
204-206: Atomic loads for manager pointers use acquire — correct pairing with release storesLocal copies via
Assert(m_chainman.load(std::memory_order_acquire))and same form_qmanare appropriate; usage via references below is safe assuming lifetime is managed externally.Please confirm
ConnectManagers()is always called before any ofProcessBlock/UndoBlock, and thatDisconnectManagers()is only called during a quiescent phase (no concurrent readers), since atomics alone don’t provide object lifetime management.Also applies to: 256-258
391-399: ForceSignalDBUpdate: atomic load and local use — LGTMAcquired
chainmanis used via local pointer; no direct member derefs remain. Looks good.
383-387: DisconnectManagers precondition documented and scoped to teardown: @pre annotations already specify the required order in both headers, and all DisconnectManagers calls are confined to init/shutdown in chainstate.cpp. No further changes needed.src/txmempool.cpp (3)
680-684: Use the loaded dmnman; avoid hidden re-loads laterNow that
dmnmanis passed in, continue to use it here instead of any future global loads. Current lines look fine; no changes needed beyond prior refactor.Also applies to: 686-693
1079-1082: Nice: outer null-check before removeProTxConflicts()Given the above defensive changes in sub-calls, this guard remains a good fast-path.
730-733: LGTM: guarded ProTx cleanup on removalThe null-check prevents unnecessary work when managers are not connected.
| if (node.cj_ctx->queueman) { | ||
| obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Also guard node.cj_ctx and keep queue_size stable to avoid NPE and API drift
node.cj_ctx itself can be nullptr in some runtime configurations; dereferencing it before checking risks a crash. Additionally, omitting queue_size changes the response schema. Push a default 0 when unavailable.
Apply this diff:
- if (node.cj_ctx->queueman) {
- obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize());
- }
+ if (node.cj_ctx && node.cj_ctx->queueman) {
+ obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize());
+ } else {
+ // Preserve schema stability even if queue manager isn't initialized
+ obj.pushKV("queue_size", 0);
+ }Run to find any other unguarded dereferences:
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -nP -C2 '\bcj_ctx->queueman\b' --glob 'src/**'Length of output: 815
Guard node.cj_ctx dereferences in both RPC and init paths
-
In
src/rpc/coinjoin.cpp(lines 477–479), apply:- if (node.cj_ctx->queueman) { - obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); - } + if (node.cj_ctx && node.cj_ctx->queueman) { + obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); + } else { + obj.pushKV("queue_size", 0); + }
-
In
src/init.cpp(line 2290), wrap the scheduler call to avoid dereferencing a null context:- node.scheduler->scheduleEvery( - std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*node.cj_ctx->queueman)), - std::chrono::seconds{1}); + if (node.cj_ctx && node.cj_ctx->queueman) { + node.scheduler->scheduleEvery( + std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*node.cj_ctx->queueman)), + std::chrono::seconds{1}); + }
📝 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.
| if (node.cj_ctx->queueman) { | |
| obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); | |
| } | |
| // File: src/rpc/coinjoin.cpp (around lines 477–479) | |
| - if (node.cj_ctx->queueman) { | |
| - obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); | |
| if (node.cj_ctx && node.cj_ctx->queueman) { | |
| obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); | |
| } else { | |
| obj.pushKV("queue_size", 0); | |
| } |
| if (node.cj_ctx->queueman) { | |
| obj.pushKV("queue_size", node.cj_ctx->queueman->GetQueueSize()); | |
| } | |
| // File: src/init.cpp (around line 2290) | |
| - node.scheduler->scheduleEvery( | |
| - std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*node.cj_ctx->queueman)), | |
| if (node.cj_ctx && node.cj_ctx->queueman) { | |
| node.scheduler->scheduleEvery( | |
| std::bind(&CCoinJoinClientQueueManager::DoMaintenance, std::ref(*node.cj_ctx->queueman)), | |
| std::chrono::seconds{1}); | |
| } |
🤖 Prompt for AI Agents
In src/rpc/coinjoin.cpp around lines 477-479 and src/init.cpp around line 2290,
the code dereferences node.cj_ctx without null-checks; update both locations to
first check that node.cj_ctx is non-null before accessing its members: in
coinjoin.cpp, only call node.cj_ctx->queueman and pushKV("queue_size", ...) if
node.cj_ctx != nullptr (and ensure queueman itself is checked as already done),
and in init.cpp wrap the scheduler call at line 2290 with a guard that verifies
cj_ctx is non-null before invoking the scheduler to avoid null dereference
during initialization.
| bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { | ||
| assert(m_dmnman); | ||
| auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire)); | ||
|
|
||
| LOCK(cs); | ||
|
|
There was a problem hiding this comment.
existsProviderTxConflict(): avoid Assert on m_dmnman; return conservative result when unavailable
This path can run while managers are disconnected; aborting is worse than conservatively signaling a conflict.
- auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire));
+ auto dmnman = m_dmnman.load(std::memory_order_acquire);
+ if (!dmnman) return true; // be conservative(Use the already-loaded dmnman below; current references are fine.)
Also applies to: 1404-1416, 1425-1436
🤖 Prompt for AI Agents
In src/txmempool.cpp around lines 1336-1340 (and similarly at 1404-1416 and
1425-1436), the code uses Assert(m_dmnman.load(...)) causing an abort when the
manager is unavailable; change this to use the already-loaded dmnman variable
and handle a null pointer conservatively by returning true (indicating a
conflict) instead of asserting/crashing, i.e., load m_dmnman once, check if
dmnman is null and if so return true, otherwise proceed to use dmnman as before.
| int CTxMemPool::Expire(std::chrono::seconds time) | ||
| { | ||
| AssertLockHeld(cs); | ||
| assert(m_isman); | ||
| auto isman = Assert(m_isman.load(std::memory_order_acquire)); | ||
| indexed_transaction_set::index<entry_time>::type::iterator it = mapTx.get<entry_time>().begin(); | ||
| setEntries toremove; | ||
| while (it != mapTx.get<entry_time>().end() && it->GetTime() < time) { | ||
| // locked txes do not expire until mined and have sufficient confirmations | ||
| if (m_isman->IsLocked(it->GetTx().GetHash())) { | ||
| if (isman->IsLocked(it->GetTx().GetHash())) { | ||
| it++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Expire(): avoid Assert on m_isman before first ConnectManagers() or after disconnect
Expire() can run early/late in lifecycle; aborting on null is risky. Make lock-check conditional.
- auto isman = Assert(m_isman.load(std::memory_order_acquire));
+ auto isman = m_isman.load(std::memory_order_acquire);
@@
- if (isman->IsLocked(it->GetTx().GetHash())) {
+ if (isman && isman->IsLocked(it->GetTx().GetHash())) {📝 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.
| int CTxMemPool::Expire(std::chrono::seconds time) | |
| { | |
| AssertLockHeld(cs); | |
| assert(m_isman); | |
| auto isman = Assert(m_isman.load(std::memory_order_acquire)); | |
| indexed_transaction_set::index<entry_time>::type::iterator it = mapTx.get<entry_time>().begin(); | |
| setEntries toremove; | |
| while (it != mapTx.get<entry_time>().end() && it->GetTime() < time) { | |
| // locked txes do not expire until mined and have sufficient confirmations | |
| if (m_isman->IsLocked(it->GetTx().GetHash())) { | |
| if (isman->IsLocked(it->GetTx().GetHash())) { | |
| it++; | |
| continue; | |
| } | |
| int CTxMemPool::Expire(std::chrono::seconds time) | |
| { | |
| AssertLockHeld(cs); | |
| // Load manager pointer but don’t assert—it may legitimately be null early/late in the lifecycle | |
| auto isman = m_isman.load(std::memory_order_acquire); | |
| indexed_transaction_set::index<entry_time>::type::iterator it = mapTx.get<entry_time>().begin(); | |
| setEntries toremove; | |
| while (it != mapTx.get<entry_time>().end() && it->GetTime() < time) { | |
| // locked txes do not expire until mined and have sufficient confirmations | |
| if (isman && isman->IsLocked(it->GetTx().GetHash())) { | |
| it++; | |
| continue; | |
| } | |
| // …rest of expiration logic… | |
| } | |
| // … | |
| } |
🤖 Prompt for AI Agents
In src/txmempool.cpp around lines 1572-1583, remove the unconditional
Assert(m_isman.load(...)) and make the m_isman check conditional so Expire()
does not abort if m_isman is null; load m_isman with memory_order_acquire into a
local variable, and only call IsLocked(...) when that pointer is non-null (i.e.
if (isman) { if (isman->IsLocked(...)) ... } ), leaving behavior unchanged when
managers are present.
CCoinJoinClientQueueManager is conditionally initialized, we will be deref'ing a nullptr if relay_txes is false.
kwvg
changed the title
std::atomic to protect connected manager/signer pointers, add check before CCoinJoinClientQueueManager access std::atomic to protect connected manager/signer pointers, check before queueman access, update P2P message map in tests
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/txmempool.cpp (2)
1335-1337: existsProviderTxConflict(): avoid Assert; return conservative outcome when managers unavailableDo not abort if managers are disconnected. Load once, early-return true on null to be safe. Use the loaded pointer below.
Apply:
-bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { - auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire)); +bool CTxMemPool::existsProviderTxConflict(const CTransaction &tx) const { + auto dmnman = m_dmnman.load(std::memory_order_acquire); @@ - auto dmn = dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); + if (!dmnman) return true; + auto dmn = dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); @@ - auto dmn = dmnman->GetListAtChainTip().GetMN(proTx.proTxHash); + if (!dmnman) return true; + auto dmn = dmnman->GetListAtChainTip().GetMN(proTx.proTxHash);Also applies to: 1401-1416, 1424-1436
1571-1582: Expire(): avoid Assert on m_isman; check conditionallyLet mempool expire when InstantSend manager is absent. Only gate IsLocked() call.
Apply:
- auto isman = Assert(m_isman.load(std::memory_order_acquire)); + auto isman = m_isman.load(std::memory_order_acquire); @@ - if (isman->IsLocked(it->GetTx().GetHash())) { + if (isman && isman->IsLocked(it->GetTx().GetHash())) {
🧹 Nitpick comments (3)
src/txmempool.h (1)
438-440: Good: managers made atomicUsing std::atomic pointers here is appropriate for cross-thread visibility. Consider a short comment noting acquire/release usage expectations.
test/functional/test_framework/p2p.py (1)
148-188: MESSAGEMAP updates LGTM; fixes handler gaps
- getheaders2/headers2/sendheaders2 now mapped to handlers; Dash/Q messages acknowledged with None to avoid unexpected failures.
Optional: keep Dash-specific keys alphabetized to ease future diffs.
src/txmempool.cpp (1)
443-447: Optional: relax memory orders where appropriateFor init-time asserts, memory_order_relaxed would suffice; not blocking, just noting.
Also applies to: 525-527, 729-731, 934-957, 1078-1081, 1335-1337, 1571-1582
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/evo/mnhftx.cpp(3 hunks)src/evo/mnhftx.h(3 hunks)src/net_processing.cpp(3 hunks)src/rpc/coinjoin.cpp(1 hunks)src/txmempool.cpp(12 hunks)src/txmempool.h(3 hunks)test/functional/test_framework/p2p.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/rpc/coinjoin.cpp
- src/net_processing.cpp
- src/evo/mnhftx.h
- src/evo/mnhftx.cpp
🧰 Additional context used
📓 Path-based instructions (2)
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/txmempool.hsrc/txmempool.cpp
test/functional/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/test_framework/p2p.py
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/txmempool.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
PR: dashpay/dash#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/txmempool.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
PR: dashpay/dash#6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/txmempool.cpp
🧬 Code graph analysis (3)
src/txmempool.h (3)
src/coinjoin/context.h (1)
llmq(21-23)src/evo/mnhftx.cpp (2)
DisconnectManagers(383-387)DisconnectManagers(383-383)src/txmempool.cpp (4)
DisconnectManagers(449-453)DisconnectManagers(449-449)addUncheckedProTx(650-698)addUncheckedProTx(650-651)
src/txmempool.cpp (1)
src/evo/mnhftx.cpp (2)
DisconnectManagers(383-387)DisconnectManagers(383-383)
test/functional/test_framework/p2p.py (1)
test/functional/test_framework/messages.py (8)
msg_getheaders2(2010-2031)msg_getmnlistd(2247-2266)msg_headers2(2059-2079)msg_mnlistdiff(2270-2322)msg_notfound(1930-1944)msg_qdata(2450-2491)msg_qgetdata(2418-2447)msg_sendheaders2(1964-1978)
⏰ 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: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- 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: x86_64-w64-mingw32 / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (9)
src/txmempool.h (2)
593-593: API surface looks rightDisconnectManagers() declaration aligns with the implementation and documented lifecycle.
890-891: Passing dmnman by reference is the right directionSignature change eliminates reloading atomics inside and clarifies dependencies.
src/txmempool.cpp (7)
443-447: ConnectManagers: sane acquire/release initializationAsserts against double-init and release stores are fine.
449-453: DisconnectManagers: correct release storesNulling atomics with release is consistent with loads.
525-527: Nice: load once, pass ref (avoids TOCTOU)This fixes the prior load-then-Assert race.
650-652: Good refactor: dmnman passed explicitlyKeeps atomic concerns out of business logic.
679-680: Assert on MN lookup is acceptable post-validationGiven ATMP validation, Assert(GetMN(...)) here is fine.
Also applies to: 687-688
934-957: Remove Assert on m_dmnman in removeProTxSpentCollateralConflicts()Avoid aborting when managers are disconnected. Make dmnman optional: only the MN list query needs it; the rest of the cleanup is safe without.
[ suggest_essential_refactor ]
Apply:- auto dmnman = Assert(m_dmnman.load(std::memory_order_acquire)); + auto dmnman = m_dmnman.load(std::memory_order_acquire); @@ - auto mnList = dmnman->GetListAtChainTip(); + auto mnList = dmnman ? dmnman->GetListAtChainTip() : CDeterministicMNList{}; @@ - auto dmn = mnList.GetMNByCollateral(in.prevout); + auto dmn = dmnman ? mnList.GetMNByCollateral(in.prevout) : nullptr;
1078-1081: Don’t gate removeProTxConflicts() on m_dmnman after making spent-collateral path tolerantOnce removeProTxSpentCollateralConflicts() handles null dmnman, this guard can be dropped to ensure consistent cleanup.
Apply:
- if (m_dmnman.load(std::memory_order_acquire)) { - removeProTxConflicts(*tx); - } + removeProTxConflicts(*tx);Likely an incorrect or invalid review comment.
| if (m_dmnman.load(std::memory_order_acquire)) { | ||
| removeUncheckedProTx(it->GetTx()); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
*Quick grep to ensure no remaining Assert(m_man.load(...)) sites
Run to verify all manager/signer asserts are removed or guarded.
Also applies to: 1078-1081, 934-957, 1335-1337, 1571-1582
🏁 Script executed:
#!/bin/bash
rg -nP 'Assert\s*\(\s*m_([a-z]+)man\.load' -C2 --type=cpp
rg -nP '\bload\s*\(\s*std::memory_order_acquire\s*\)' -C1 --type=cpp src/txmempool.cppLength of output: 3247
Remove or guard all remaining Assert(m_*man.load(...)) calls
- src/txmempool.cpp at lines 934, 1336 (dmnman) and 1574 (isman)
- src/evo/mnhftx.cpp at lines 204–205, 256–257, 391 (chainman/qman)
These must be either removed or wrapped in the same null‐check pattern used elsewhere before dereferencing.
🤖 Prompt for AI Agents
In src/txmempool.cpp around lines 729–731, replace any direct
Assert(m_dmnman.load(...)) usages with a null-check guard or remove them;
specifically, ensure you do not dereference m_dmnman unless
m_dmnman.load(std::memory_order_acquire) is true — wrap the dereference in an if
(m_dmnman.load(std::memory_order_acquire)) { ... } block (or remove the Assert
entirely). Apply the same fix pattern to the other reported locations in this
file (lines ~934, ~1336 for dmnman and ~1574 for isman) and to the other files
mentioned so no dereference occurs without first checking the atomic pointer is
non-null.
Unconditionally run ProTx cleanup in removeUnchecked()
Gating cleanup on m_dmnman risks stale mapProTx* entries if managers disconnect after insertion. removeUncheckedProTx() doesn’t depend on managers; always call it.
Apply:
- if (m_dmnman.load(std::memory_order_acquire)) {
- removeUncheckedProTx(it->GetTx());
- }
+ removeUncheckedProTx(it->GetTx());📝 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.
| if (m_dmnman.load(std::memory_order_acquire)) { | |
| removeUncheckedProTx(it->GetTx()); | |
| } | |
| - if (m_dmnman.load(std::memory_order_acquire)) { | |
| - removeUncheckedProTx(it->GetTx()); | |
| removeUncheckedProTx(it->GetTx()); |
🤖 Prompt for AI Agents
In src/txmempool.cpp around lines 729 to 731, the current code only calls
removeUncheckedProTx(it->GetTx()) when m_dmnman.load(std::memory_order_acquire)
is true; this can leave stale mapProTx* entries if managers disconnect after
insertion. Remove the conditional and always call
removeUncheckedProTx(it->GetTx()) unconditionally in removeUnchecked(), ensuring
ProTx cleanup runs regardless of m_dmnman state.
Additional Information
GovernanceSigner, dropRelay()s and use periodic relay instead, minor cleanup #6838This pull request contains the following:
nullptrcheck afterCCoinJoinClientQueueManagerwas conditionally initialized in dash#5163 (commit)Breaking Changes
None expected.
Checklist