feat: coinjoin promotion / demotion #7052
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
WalkthroughThis PR adds post‑V24 promotion and demotion flows to CoinJoin: new session flags and promotion input tracking, promotion/demotion-aware queue join/start APIs, wallet helpers to count/select fully‑mixed denomination UTXOs, deployment‑aware structure and in/out validation (classifying STANDARD/PROMOTION/DEMOTION), denomination adjacency helpers and constants (PROMOTION_RATIO, GAP_THRESHOLD), deferred DSTX structure validation using the current chain tip, server limits updated for post‑V24 entries, and extensive unit tests covering pre/post‑fork and promotion/demotion scenarios. Sequence DiagramsequenceDiagram
participant ClientMgr as CoinJoinClientManager
participant ClientSess as CCoinJoinClientSession
participant Wallet as CWallet
participant Server as CoinJoinServer
participant Chain as Chain/BlockIndex
rect rgb(240,248,255)
Note over ClientMgr: DoAutomaticDenominating() iterates adjacent denom pairs
ClientMgr->>ClientMgr: ShouldPromote/ShouldDemote checks
end
rect rgb(230,250,230)
Note over ClientMgr,ClientSess: Queue selection/creation with promotion/demotion context
alt Join existing queue
ClientMgr->>ClientSess: JoinExistingQueue(nBalance,nTargetDenom,fPromotion,fDemotion)
ClientSess->>Wallet: SelectTxDSInsByDenomination / SelectFullyMixedForPromotion
else Start new queue
ClientMgr->>ClientSess: StartNewQueue(nBalance,nTargetDenom,fPromotion,fDemotion)
ClientSess->>Wallet: SelectFullyMixedForPromotion (store m_vecPromotionInputs)
end
end
rect rgb(255,250,240)
Note over ClientSess: Prepare & submit entry
ClientSess->>ClientSess: SubmitDenominate()
alt Promotion flow
ClientSess->>ClientSess: PreparePromotionEntry() (10 small->1 large)
else Demotion flow
ClientSess->>ClientSess: PrepareDemotionEntry() (1 large->10 small)
else Standard flow
ClientSess->>ClientSess: Standard 1:1 Prepare
end
ClientSess->>Server: Send/submit entry (DSTX)
end
rect rgb(255,240,245)
Note over Server,Chain: Validation
Server->>Chain: Retrieve pindex for V24 check
Server->>Server: IsValidStructure(pindex) and IsValidInOuts() -> classify entry type
Server->>Server: Enforce input limits (pre-V24 vs PROMOTION_RATIO) and denomination rules
Server-->>ClientSess: Accept or reject entry
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 0
🧹 Nitpick comments (4)
src/net_processing.cpp (1)
3516-3526: Tip-aware DSTX structure validation looks correct, minor clarity tweak possibleUsing
chainman.ActiveChain().Tip()undercs_mainand passingpindexintodstx.IsValidStructure(pindex)is the right direction for fork‑/deployment‑aware DSTX checks and matches existing patterns in this file that rely on a non‑null active tip.Two small nits you may consider for clarity (not blockers):
- Use a separate local (e.g.
const CBlockIndex* pindex_for_validation = pindex;) before the laterforloops that walkpindexback 24 blocks, so it’s obvious that the structure check uses the current tip and is independent from the masternode lookup iteration.- Optionally add a brief comment above the
LOCK(cs_main)explaining that we intentionally snapshot the current tip for tip‑dependent CoinJoin structure rules.Otherwise this change looks consistent with the new CoinJoin promotion/demotion and V24‑aware validation flow.
src/wallet/coinjoin.cpp (1)
443-471: Consider early termination placement for clarity.The early termination check at line 453 occurs after the lock is acquired but before any significant work. While functionally correct, this could be slightly more efficient if moved before the wallet lookup, though the impact is negligible.
The function correctly:
- Guards on enabled state and valid denomination
- Filters for confirmed depth (
< 1)- Requires fully mixed status for promotion candidates
Minor readability improvement - the early break is fine, but consider:
LOCK(cs_wallet); for (const auto& outpoint : setWalletUTXO) { - if (static_cast<int>(vecRet.size()) >= nCount) break; + if (vecRet.size() >= static_cast<size_t>(nCount)) break;This avoids a sign conversion and is slightly more idiomatic.
src/coinjoin/coinjoin.cpp (1)
289-313: Minor: Consider extracting checkTxOut to reduce closure complexity.The
checkTxOutlambda captures multiple variables and handles denomination validation, script checks, and duplicate detection. While functional, extracting this as a private member function could improve testability.The logic is correct:
- Validates denomination against expected
- Ensures P2PKH script type
- Prevents duplicate scriptPubKeys (privacy requirement)
src/coinjoin/client.cpp (1)
1375-1494: Consider extracting common masternode selection logic.The masternode selection loop (lines 1435-1491) is nearly identical to the standard
StartNewQueue(lines 1316-1372). The differences are:
- Input validation at the start
- Denomination is fixed rather than selected from
setAmounts- Promotion/demotion state is stored
This duplication could lead to maintenance issues if the masternode selection logic needs updates.
Consider extracting the common masternode selection and connection logic into a private helper method that both
StartNewQueueoverloads can call, passing in the denomination and a callback for post-connection setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/coinjoin/client.cpp(12 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/coinjoin.cpp(6 hunks)src/coinjoin/coinjoin.h(2 hunks)src/coinjoin/common.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/net_processing.cpp(1 hunks)src/test/coinjoin_inouts_tests.cpp(4 hunks)src/wallet/coinjoin.cpp(3 hunks)src/wallet/wallet.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/coinjoin.hsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/common.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cppsrc/wallet/coinjoin.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cpp
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/coinjoin_inouts_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/wallet.hsrc/wallet/coinjoin.cpp
🧠 Learnings (21)
📓 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/coinjoin.hsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/common.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cppsrc/wallet/coinjoin.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/coinjoin/server.cppsrc/wallet/wallet.hsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/coinjoin/server.cppsrc/wallet/wallet.hsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/net_processing.cppsrc/coinjoin/coinjoin.hsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/common.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cppsrc/wallet/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/net_processing.cppsrc/coinjoin/coinjoin.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.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-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/coinjoin/coinjoin.h
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/coinjoin/coinjoin.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/test/coinjoin_inouts_tests.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/test/coinjoin_inouts_tests.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/test/coinjoin_inouts_tests.cppsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/wallet.hsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/coinjoin/client.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/coinjoin.cpp
🧬 Code graph analysis (6)
src/coinjoin/coinjoin.h (1)
src/coinjoin/coinjoin.cpp (6)
IsValidStructure(93-136)IsValidStructure(93-93)ValidatePromotionEntry(584-633)ValidatePromotionEntry(584-585)ValidateDemotionEntry(635-675)ValidateDemotionEntry(635-636)
src/wallet/wallet.h (1)
src/wallet/coinjoin.cpp (8)
SelectTxDSInsByDenomination(48-51)SelectTxDSInsByDenomination(48-48)SelectTxDSInsByDenomination(53-93)SelectTxDSInsByDenomination(53-53)CountCoinsByDenomination(414-441)CountCoinsByDenomination(414-414)SelectFullyMixedForPromotion(443-471)SelectFullyMixedForPromotion(443-443)
src/coinjoin/coinjoin.cpp (1)
src/coinjoin/common.h (3)
IsDenominatedAmount(101-101)GetLargerAdjacentDenom(146-151)AmountToDenomination(53-61)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (10)
JoinExistingQueue(1149-1292)JoinExistingQueue(1149-1150)PreparePromotionEntry(1725-1784)PreparePromotionEntry(1725-1725)PrepareDemotionEntry(1786-1845)PrepareDemotionEntry(1786-1786)ShouldPromote(2316-2349)ShouldPromote(2316-2316)ShouldDemote(2351-2378)ShouldDemote(2351-2351)
src/coinjoin/client.cpp (1)
src/coinjoin/common.h (3)
GetLargerAdjacentDenom(146-151)DenominationToAmount(69-184)AreAdjacentDenominations(135-141)
src/wallet/coinjoin.cpp (2)
src/coinjoin/common.h (1)
DenominationToAmount(69-184)src/wallet/wallet.cpp (2)
GetTxDepthInMainChain(3561-3571)GetTxDepthInMainChain(3561-3561)
⏰ 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: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-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_sqlite-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (46)
src/wallet/wallet.h (3)
28-28: LGTM: Include addition is appropriate.The inclusion of
wallet/coincontrol.his necessary to support the newSelectTxDSInsByDenominationoverload at line 547, which usesCoinTypeas a parameter. The existing forward declaration ofCCoinControlat line 124 is insufficient for the enum type.
547-547: LGTM: Overload supports denomination-based selection with coin type filtering.The new overload extends the existing
SelectTxDSInsByDenominationto accept aCoinTypeparameter, enabling more granular control over input selection for promotion/demotion workflows.
565-580: LGTM: Well-documented wallet APIs for promotion/demotion support.The new methods provide essential functionality for the post-V24 promotion/demotion feature:
CountCoinsByDenominationcounts coins by denomination with optional fully-mixed filteringSelectFullyMixedForPromotionselects fully-mixed coins suitable for promotionBoth methods are correctly marked
constand have clear documentation.src/coinjoin/common.h (5)
112-114: LGTM: Clear promotion/demotion constants.The constants are well-defined:
PROMOTION_RATIO = 10reflects the 10:1 ratio between adjacent denominationsGAP_THRESHOLD = 10defines the deficit threshold for triggering promotion/demotion
120-129: LGTM: Correct denomination index lookup.The function correctly maps a bitshifted denomination to its index in
vecStandardDenominations, returning -1 for invalid denominations. The logic and bounds checking are sound.
135-141: LGTM: Correct adjacency check for denominations.The function correctly validates that two denominations are adjacent in the standard denomination list, handling invalid inputs appropriately. This is essential for promotion/demotion validation.
146-151: LGTM: Correct larger adjacent denomination lookup.The function correctly navigates to the larger adjacent denomination, returning 0 when the input is already the largest or invalid. The bitshift calculation is accurate.
156-161: LGTM: Correct smaller adjacent denomination lookup.The function correctly navigates to the smaller adjacent denomination, returning 0 when the input is already the smallest or invalid. The bounds checking and bitshift calculation are accurate.
src/coinjoin/server.cpp (4)
7-9: LGTM: Necessary includes for V24 deployment checks.The includes are required:
chainparams.hfor accessing consensus parameters viaParams()deploymentstatus.hfor theDeploymentActiveAtfunction used in V24 activation checks
228-238: LGTM: Correct V24 gate for promotion/demotion entries.The deployment gate correctly rejects unbalanced (promotion/demotion) entries when V24 is not active. The logic appropriately:
- Detects unbalanced entries by comparing input/output counts
- Checks V24 activation status at the current chain tip
- Rejects with
ERR_MODEwhen the feature is not yet active
607-618: LGTM: Dynamic max inputs based on V24 activation.The code correctly adjusts the maximum entry input count based on V24 activation:
- Pre-V24: limits to
COINJOIN_ENTRY_MAX_SIZE(9)- Post-V24: allows up to
PROMOTION_RATIO(10) for promotion entriesThe logging and error handling are appropriate. This change works in conjunction with the ProcessDSVIN gate to properly support promotion/demotion entries.
228-238: Note: V24 activation checks are performed separately.The code checks V24 activation in both
ProcessDSVIN(line 232) andAddEntry(line 610). While these are called sequentially and the risk is minimal, be aware that the chain tip could theoretically advance between checks. In practice, this is acceptable as it would only result in entry rejection, which is safe.This is an informational note for awareness; verification via testing would confirm the behavior is correct under block transitions.
Also applies to: 607-618
src/coinjoin/coinjoin.h (2)
284-284: LGTM: Signature update for deployment-aware validation.The updated
IsValidStructure(const CBlockIndex* pindex)signature correctly enables V24 deployment detection for promotion/demotion validation. Usingnullptrto indicate pre-fork behavior is a reasonable pattern.
367-388: LGTM: Well-documented validation helper declarations.The new
ValidatePromotionEntryandValidateDemotionEntrydeclarations are properly documented with clear parameter semantics. The API correctly identifies the session denomination role (smaller denom for both cases) and returns validation status via the out-parameter.src/wallet/coinjoin.cpp (3)
48-51: LGTM: Clean delegation for backward compatibility.The existing 3-parameter overload now delegates to the new 4-parameter version with
CoinType::ONLY_READY_TO_MIXas default. This preserves existing behavior while enabling the new coin type flexibility.
53-68: LGTM: New overload with configurable coin type.The new overload correctly accepts
CoinTypeand passes it toCCoinControl. The enhanced logging at line 68 is helpful for debugging coin selection with different coin types.
414-441: LGTM: CountCoinsByDenomination implementation is correct.The function properly:
- Guards on
CCoinJoinClientOptions::IsEnabled()- Validates denomination via
DenominationToAmount- Uses depth check
< 1to skip unconfirmed/conflicted (consistent with line 462)- Correctly handles the
fFullyMixedOnlyfiltersrc/coinjoin/client.h (4)
146-150: LGTM: Promotion/demotion session state additions.The new session state members are properly initialized:
m_fPromotion{false}andm_fDemotion{false}with brace initializersm_vecPromotionInputsas an empty vectorThese flags enable session-level tracking of the entry type being prepared.
176-183: LGTM: Entry preparation methods with proper lock annotations.Both
PreparePromotionEntryandPrepareDemotionEntrycorrectly requirem_wallet->cs_walletlock, consistent withPrepareDenominateabove. The separation of promotion (10→1) and demotion (1→10) preparation is clean.
335-350: LGTM: Manager-level decision helpers with clear documentation.The
ShouldPromoteandShouldDemotemethods are properly documented with parameter semantics. These provide the decision logic for when to trigger promotion/demotion based on denomination counts and goals.
164-168: Verify SetNull() clears new state members.The new parameters to
JoinExistingQueueandStartNewQueuecorrectly extend the API with defaults for backward compatibility. EnsureSetNull()in the session class properly resetsm_fPromotion,m_fDemotion, andm_vecPromotionInputsbefore merging.src/test/coinjoin_inouts_tests.cpp (7)
52-86: LGTM: Updated tests for pindex-aware IsValidStructure.The tests correctly pass
nullptrtoIsValidStructure()to test pre-V24 behavior, matching the updated signature. The test cases cover:
- Valid structure
- Invalid identifiers
- Size mismatch (pre-V24 rejection)
- Invalid scripts and amounts
Good coverage of pre-fork validation paths.
137-149: LGTM: Clean test helper functions.
MakeDenomInputandMakeDenomOutputprovide reusable helpers for constructing test inputs/outputs. The helpers correctly useP2PKHScriptfor valid script generation.
151-175: LGTM: Valid promotion entry test.The test correctly validates a promotion entry with:
- 10 inputs (
PROMOTION_RATIO)- 1 output at the larger adjacent denomination
- Proper denomination adjacency (0.1 → 1.0 DASH)
477-500: Critical test for value preservation across denominations.This test validates a key invariant:
nSmaller * PROMOTION_RATIO == nLargerfor all adjacent denomination pairs. This ensures promotion/demotion preserves exact value.
722-756: Test helper mirrors implementation logic correctly.The
TestShouldPromoteandTestShouldDemotehelpers correctly implement the decision algorithm with:
- Half-goal threshold check
- Deficit calculation (
max(0, goal - count))- Gap threshold comparison
This enables testing the algorithm logic without wallet dependencies.
827-861: Thorough mutual exclusivity testing.The
promote_demote_mutually_exclusivetest correctly validates that for any count distribution, at most one of promote/demote can be true. The use of structured bindings (auto [p, d] = ...) is clean C++17 style.
1019-1039: Good documentation of functional test requirements.The comment block properly documents that post-V24 behaviors requiring EHF activation cannot be unit tested and require functional tests. This sets clear expectations for test coverage boundaries.
src/coinjoin/coinjoin.cpp (7)
9-9: LGTM: Required include for deployment status checks.The
deploymentstatus.hinclude is necessary for theDeploymentActiveAtcalls used in V24 activation checks.
93-136: LGTM: IsValidStructure correctly handles V24 deployment.The implementation correctly:
- Uses
DeploymentActiveAtwith pindex for V24 detection- Pre-V24: Requires balanced vin/vout counts
- Post-V24: Allows unbalanced for promotion/demotion
- Dynamically adjusts max inputs (180 pre-fork, 200 post-fork)
- Validates all outputs are denominated and P2PKH
The comment about value sum validation being deferred to
IsValidInOutsis accurate since it requires UTXO access.
221-260: LGTM: Entry type detection with proper V24 gating.The entry type detection logic correctly:
- Identifies STANDARD (balanced) entries for all versions
- Only allows PROMOTION/DEMOTION post-V24
- Properly rejects invalid structures with ERR_SIZE_MISMATCH
The local
EntryTypeenum provides clear semantics.
261-288: LGTM: Denomination expectations set correctly per entry type.For each entry type:
- STANDARD: inputs and outputs at session denom
- PROMOTION: inputs at session denom, output at larger adjacent
- DEMOTION: input at larger adjacent, outputs at session denom
The early failure when
nLargerAdjacentDenom == 0prevents invalid promotion/demotion from the largest/smallest denominations.
351-363: LGTM: Value preservation check applies to all entry types.The
nFees != 0check correctly ensures total input value equals total output value. This is essential for promotion/demotion where 10 × smaller_denom == larger_denom must hold exactly.The logging now includes entry type, which aids debugging.
583-633: LGTM: ValidatePromotionEntry implementation is correct.The function properly validates:
- Exactly
PROMOTION_RATIOinputs- Exactly 1 output
- Larger adjacent denomination exists
- Output matches larger denomination
- Output is P2PKH
Note: Input denomination validation is not performed here as this function validates structure, not UTXO values (done in
IsValidInOuts).
635-675: LGTM: ValidateDemotionEntry correctly validates structure.The function properly validates:
- Exactly 1 input
- Exactly
PROMOTION_RATIOoutputs- All outputs at session denomination
- All outputs are P2PKH
Similar to promotion, input denomination validation is deferred to
IsValidInOutswhere UTXO access is available.src/coinjoin/client.cpp (11)
11-11: LGTM!The new includes for
deploymentstatus.handvalidation.hare appropriate for the V24 deployment detection logic added in this PR.Also applies to: 25-25
295-299: LGTM!Proper cleanup of the new promotion/demotion session state fields, with the lock assertion already in place from the existing code.
979-1035: Clarify target denomination semantics for queues.The V24 detection and iteration logic looks correct. However, I want to verify the target denomination semantics:
- For promotion (lines 1007, 1011):
nSmallerDenomis passed as target - this makes sense as the queue is for the smaller denomination that gets promoted to larger.- For demotion (lines 1026, 1030):
nSmallerDenomis also passed - this means the queue denomination is the output denomination, not the input.This is consistent with the queue matching in
JoinExistingQueueat line 1185 which checksdsq.nDenom != nTargetDenom. The session denomination represents what the mixing session produces, which for demotion is the smaller denomination.The logic is correct but the naming could be clearer. The
nTargetDenomeffectively means "the denomination this session is operating on" which works for both promotion inputs and demotion outputs.
1575-1594: LGTM!Clean separation of promotion/demotion submission paths from standard mixing. The error handling properly logs failures and sets the status message.
1725-1784: LGTM!The promotion entry preparation correctly implements the 10:1 ratio:
- 10 inputs from fully-mixed smaller denomination coins
- 1 output of the larger adjacent denomination
- Empty
CTxOut()placeholders are properly filtered out inSendDenominate- Input locking prevents double-spending during the session
1786-1845: LGTM with a note on denomination semantics.The demotion entry preparation correctly implements the 1:10 ratio:
- 1 input from the larger denomination coin
- 10 outputs of the smaller denomination
- Empty
CTxDSIn()placeholders are properly filtered inSendDenominateThe denomination logic is correct:
nSessionDenomis the target denomination (smaller, for outputs), andGetLargerAdjacentDenom(nSessionDenom)correctly identifies the input denomination for validation.
2316-2349: Well-designed promotion decision logic.The algorithm correctly:
- Validates denomination adjacency
- Protects denominations still being built (< half goal)
- Uses a gap threshold to prevent oscillation
- Requires fully-mixed coins to maintain anonymity guarantees
The hysteresis via
GAP_THRESHOLDis a good design choice to prevent thrashing between promotion and demotion.
2351-2378: LGTM!The demotion decision logic correctly mirrors promotion but without the fully-mixed requirement. This asymmetry is intentional and makes sense:
- Promotion requires fully-mixed coins to preserve anonymity (10 mixed → 1 mixed)
- Demotion can use any denominated coin since splitting doesn't reduce anonymity (1 → 10 new outputs)
The gap threshold hysteresis is shared, preventing oscillation.
1192-1241: Verify wallet lock consistency between promotion and demotion paths.For the promotion path (lines 1192-1213), you explicitly acquire
m_wallet->cs_walletat line 1200 to safely accessmapWallet. However, for the demotion path (lines 1214-1234), you callSelectTxDSInsByDenominationwithout an explicit lock in this scope. Confirm whetherSelectTxDSInsByDenominationacquires the wallet lock internally or if explicit locking is needed here to match the promotion path's pattern.
1576-1594: Resource cleanup on preparation failure follows existing timeout-based pattern.In the promotion/demotion paths (lines 1577–1594), if
PreparePromotionEntryorPrepareDemotionEntryfails, locked coins remain locked until the session times out and callsSetNull() → UnlockCoins(). This matches the pattern used in standard mixing paths. Consider whether immediate cleanup would improve user experience by freeing coins sooner, though the current approach is consistent with the codebase design.
499-510: Remove references to non-existent preparation functions and correct CTxOut behavior.The filtering logic uses
prevout.IsNull()andnValue > 0to distinguish placeholder entries from real transaction data. However, the original review contains factual errors:
- CTxOut's default constructor calls
SetNull(), settingnValueto-1(not0), so the checkif (txOut.nValue > 0)correctly filters out both default (-1) and zero-value outputs.- The referenced functions
PreparePromotionEntryandPrepareDemotionEntrydo not exist in the Dash Core repository and cannot be verified against this filtering logic.The actual filtering appears sound for handling asymmetric input/output counts in promotion/demotion scenarios, but the justification in the original comment is incorrect.
Likely an incorrect or invalid review comment.
Fixes for PR dashpay#7052: Issue #1 (CRITICAL): Fix race condition in coin selection - Lock coins immediately after selection in JoinExistingQueue/StartNewQueue - Prevents concurrent sessions from selecting the same coins - Add defensive IsLockedCoin check in Prepare functions Issue #2 (CRITICAL): Fix resource leak on session failure - Add promotion inputs to vecOutPointLocked in SetNull() - Ensures coins locked early are properly unlocked if session fails - Leverages existing UnlockCoins() retry mechanism Issue #3: Add UTXO validation before use - Check IsSpent() before using promotion/demotion inputs - Prevents using externally spent or transferred coins 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fixes for PR dashpay#7052: Issue #1 (CRITICAL): Fix race condition in coin selection - Lock coins immediately after selection in JoinExistingQueue/StartNewQueue - Prevents concurrent sessions from selecting the same coins - Add defensive IsLockedCoin check in Prepare functions Issue #2 (CRITICAL): Fix resource leak on session failure - Add promotion inputs to vecOutPointLocked in SetNull() - Ensures coins locked early are properly unlocked if session fails - Leverages existing UnlockCoins() retry mechanism Issue #3: Add UTXO validation before use - Check IsSpent() before using promotion/demotion inputs - Prevents using externally spent or transferred coins 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ensure promotion/demotion sessions have adequate privacy by requiring the minimum participant threshold to be met by STANDARD mixing entries only. Implementation: - Add IsStandardMixingEntry() method to CCoinJoinEntry Detects entry type: standard (equal in/out), promotion (10:1), demotion (1:10) - Add GetStandardEntriesCount() to CCoinJoinBaseSession Counts only standard mixing entries for threshold checks - Modify server CheckPool timeout logic to use GetStandardEntriesCount() Only standard entries count toward minimum; promotion/demotion get free cover Why this matters: - Promotion/demotion creates unique patterns (10→1 or 1→10) - Small sessions where you're the only rebalancer = weak anonymity (33%) - Requiring minimum standard mixers provides cover traffic Benefits: - Promotion/demotion can still start queues (better UX, doesn't block feature) - Privacy protected by requiring standard mixer cover traffic - Session won't finalize until adequate anonymity set exists Example: Before: 1 promotion + 2 standard = proceeds (weak: 33% chance you're identified) After: 1 promotion + 2 standard = times out (needs 3 standard minimum) After: 2 promotion + 4 standard = proceeds (good: 6 participants, better cover!) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Remove 4 redundant/low-value tests: - promotion_ratio_constant (trivial constant check) - gap_threshold_constant (trivial constant check) - decision_logic_mutual_exclusivity (documentation only) - adjacent_denomination_helpers (duplicate) Add 2 critical tests for privacy protection: - is_standard_mixing_entry (validates IsStandardMixingEntry method) - get_standard_entries_count (validates threshold enforcement) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
PastaPastaPasta
force-pushed
the
feat/coinjoin-promotion
branch
from
b70ccef to
d140732
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/coinjoin/client.cpp (1)
1789-1800: Optional: Simplify promotion output pairing for clarity.The current approach creates 10 input-output pairs with empty outputs (line 1793), then replaces the last pair's output (line 1800). While correct, this is less clear than creating the pairs with the appropriate output from the start.
🔎 Alternative approach
- // For promotion, outputs are created but only 1 matters (the larger denom) - // We'll use empty CTxOut for all but the first to signal "no output for this input" - // Actually, for promotion entry: 10 inputs, 1 output - // We need to pair each input with an "empty" output, except the last gets the real output - vecPSInOutPairsRet.emplace_back(txdsin, CTxOut()); -} - -// Now set the single output (larger denomination) on the last entry -CScript scriptDenom = keyHolderStorage.AddKey(m_wallet.get()); -if (!vecPSInOutPairsRet.empty()) { - // Replace the last output with the actual promotion output - vecPSInOutPairsRet.back().second = CTxOut(nLargerAmount, scriptDenom); + // For promotion: 10 inputs, 1 output (on last pair) + if (i == static_cast<int>(m_vecPromotionInputs.size()) - 1) { + // Last input gets the real promotion output + CScript scriptDenom = keyHolderStorage.AddKey(m_wallet.get()); + vecPSInOutPairsRet.emplace_back(txdsin, CTxOut(nLargerAmount, scriptDenom)); + } else { + // Other inputs get empty outputs + vecPSInOutPairsRet.emplace_back(txdsin, CTxOut()); + } + }This makes the intent clearer without changing behavior.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/coinjoin/client.cpp(12 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/coinjoin.cpp(6 hunks)src/coinjoin/coinjoin.h(4 hunks)src/coinjoin/common.h(1 hunks)src/coinjoin/server.cpp(4 hunks)src/net_processing.cpp(1 hunks)src/test/coinjoin_inouts_tests.cpp(4 hunks)src/wallet/coinjoin.cpp(3 hunks)src/wallet/wallet.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/net_processing.cpp
- src/coinjoin/common.h
- src/wallet/wallet.h
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/coinjoin/server.cppsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.hsrc/wallet/coinjoin.cppsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/coinjoin/server.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/coinjoin/server.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/coinjoin_inouts_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/coinjoin.cpp
🧠 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: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/coinjoin/server.cppsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.hsrc/wallet/coinjoin.cppsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/coinjoin/server.cppsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/coinjoin/server.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/coinjoin/server.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/test/coinjoin_inouts_tests.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/test/coinjoin_inouts_tests.cpp
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/coinjoin/coinjoin.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/coinjoin/coinjoin.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/coinjoin/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/coinjoin/client.cpp
🧬 Code graph analysis (7)
src/coinjoin/server.cpp (2)
src/coinjoin/coinjoin.h (1)
GetStandardEntriesCount(330-341)src/coinjoin/coinjoin.cpp (2)
GetMinPoolParticipants(579-579)GetMinPoolParticipants(579-579)
src/test/coinjoin_inouts_tests.cpp (2)
src/coinjoin/common.h (6)
DenominationToAmount(69-184)IsValidDenomination(102-102)AreAdjacentDenominations(135-141)GetLargerAdjacentDenom(146-151)GetSmallerAdjacentDenom(156-161)GetSmallestDenomination(47-47)src/coinjoin/coinjoin.cpp (4)
ValidatePromotionEntry(582-631)ValidatePromotionEntry(582-583)ValidateDemotionEntry(633-673)ValidateDemotionEntry(633-634)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (14)
JoinExistingQueue(1159-1307)JoinExistingQueue(1159-1160)StartNewQueue(1309-1388)StartNewQueue(1309-1309)StartNewQueue(1390-1517)StartNewQueue(1390-1391)PreparePromotionEntry(1748-1817)PreparePromotionEntry(1748-1748)PrepareDemotionEntry(1819-1888)PrepareDemotionEntry(1819-1819)ShouldPromote(2359-2392)ShouldPromote(2359-2359)ShouldDemote(2394-2421)ShouldDemote(2394-2394)
src/coinjoin/coinjoin.h (1)
src/coinjoin/coinjoin.cpp (7)
nodiscard(64-68)IsValidStructure(93-134)IsValidStructure(93-93)ValidatePromotionEntry(582-631)ValidatePromotionEntry(582-583)ValidateDemotionEntry(633-673)ValidateDemotionEntry(633-634)
src/wallet/coinjoin.cpp (4)
src/coinjoin/client.cpp (1)
coin_control(2022-2022)src/rpc/masternode.cpp (1)
coin_control(163-163)src/coinjoin/common.h (2)
nDenomAmount(88-98)DenominationToAmount(69-184)src/wallet/wallet.cpp (2)
GetTxDepthInMainChain(3588-3598)GetTxDepthInMainChain(3588-3588)
src/coinjoin/coinjoin.cpp (1)
src/coinjoin/common.h (3)
IsDenominatedAmount(101-101)GetLargerAdjacentDenom(146-151)AmountToDenomination(53-61)
src/coinjoin/client.cpp (3)
src/wallet/coinjoin.cpp (2)
outpoint(574-574)outpoint(624-624)src/wallet/interfaces.cpp (4)
outpoint(399-399)outpoint(399-399)outpoint(400-400)outpoint(400-400)src/coinjoin/common.h (3)
GetLargerAdjacentDenom(146-151)DenominationToAmount(69-184)AreAdjacentDenominations(135-141)
⏰ 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). (5)
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
🔇 Additional comments (29)
src/wallet/coinjoin.cpp (3)
48-51: LGTM: Clean delegation pattern.The 4-parameter overload correctly delegates to the new 5-parameter version with the appropriate default
CoinType::ONLY_READY_TO_MIX, maintaining backward compatibility.
53-93: LGTM: CoinType parameterization enables promotion/demotion flows.The new 5-parameter overload correctly propagates the
CoinTypetoCCoinControland includes it in logging. The implementation maintains the original coin selection logic while adding flexibility for different coin type filtering.
414-441: Verify depth check semantics.The depth check
GetTxDepthInMainChain(it->second) < 1filters unconfirmed or conflicted transactions. According toGetTxDepthInMainChainsemantics, this function returns positive depth for confirmed transactions, 0 for unconfirmed, and negative depth for conflicted transactions. The< 1check correctly keeps only confirmed transactions with depth >= 1.src/coinjoin/server.cpp (3)
228-238: LGTM: Correct V24 deployment gating for promotion/demotion.The pre-check correctly identifies unbalanced entries (promotion/demotion) and rejects them when V24 is not active. The use of
ERR_MODEis appropriate for incompatible mode rejection, and the nullpindexcheck prevents potential crashes.
303-306: LGTM: Critical privacy protection for promotion/demotion flows.The change from
GetEntriesCount()toGetStandardEntriesCount()is essential for privacy. Promotion/demotion entries should not count toward the minimum participant threshold because they don't contribute to the anonymity set—they rely on standard mixing participants for privacy. The inline comment clearly explains this rationale.
609-620: LGTM: Deployment-aware entry input limits.The V24-aware maximum input calculation correctly increases the limit to
PROMOTION_RATIO(10) for post-V24 promotion entries while maintaining the pre-V24 limit ofCOINJOIN_ENTRY_MAX_SIZE(9). The error logging includes the actual limit for clarity.src/coinjoin/coinjoin.h (4)
172-179: LGTM: Clear entry type classification.The
IsStandardMixingEntry()method provides a simple and correct classification based on input/output balance. The inline implementation is appropriate for this trivial check, and the comment clearly explains the three entry types.
329-341: LGTM: Correct lock annotations and idiomatic implementation.The standard entry counting methods correctly use thread-safety annotations to prevent deadlocks. The locked variant (
GetStandardEntriesCountLocked) is provided for callers already holding the lock, and the implementation uses idiomaticstd::count_if. The comment clearly explains the privacy threshold purpose.
391-411: LGTM: Well-documented validation API.The promotion/demotion validation functions are cleanly declared in the
CoinJoinnamespace with comprehensive documentation. The parameter list is appropriate for the validation task, and the pattern of returningboolwith an out-parameternMessageIDRetis consistent with existing code.
293-293: All call sites have been properly updated for the signature change. The production code insrc/net_processing.cpp:3532passes the block index parameter, while test code appropriately passesnullptrfor pre-fork validation testing.src/test/coinjoin_inouts_tests.cpp (5)
52-87: LGTM: Correct pre-fork testing pattern.The tests correctly use
nullptrfor thepindexparameter to simulate pre-V24 behavior. This aligns with the implementation inIsValidStructurewhere!fV24Activeresults in pre-fork validation rules. The comments clearly document this pattern.
138-149: LGTM: Clean test helper utilities.The
MakeDenomInputandMakeDenomOutputhelper functions provide clean abstractions that reduce test boilerplate while ensuring unique prevouts and correct P2PKH scripts as required by CoinJoin.
469-492: LGTM: Critical value preservation invariant.This test verifies the fundamental invariant that
10 * smaller == largerexactly for all adjacent denomination pairs. This is essential for promotion/demotion to preserve value without fees. The test correctly covers all adjacent pairs in the denomination ladder.
684-856: LGTM: Comprehensive promotion/demotion decision algorithm tests.The test suite thoroughly covers the promotion/demotion decision algorithms:
- HalfGoal threshold prevents sacrificing denominations being built up
- GAP_THRESHOLD ensures sufficient deficit gap before action
- Mutual exclusivity tests confirm at most one action triggers
- Specific examples from the implementation plan are verified
These tests provide confidence in the correctness of the decision logic that will balance denomination distributions.
982-1021: LGTM: Privacy threshold counting verification.This test verifies that
GetStandardEntriesCountcorrectly excludes promotion/demotion entries from the count, returning only the standard mixing entries (3 of 5 total). This aligns with the privacy protection logic in the server where only standard entries count toward the minimum threshold.src/coinjoin/client.h (4)
164-168: LGTM: Clean API extension with backward compatibility.The queue method signatures are extended with default parameters (
nTargetDenom = 0, fPromotion = false, fDemotion = false), maintaining backward compatibility while enabling promotion/demotion flows. The overloading pattern keeps the API clean.
177-183: LGTM: Well-documented preparation methods with correct lock annotations.The
PreparePromotionEntryandPrepareDemotionEntrymethods are properly annotated withEXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet)to ensure thread safety. The API follows the existing pattern of returningboolwith a string error message and output parameters for the constructed entry pairs.
336-350: LGTM: Clean decision API with comprehensive documentation.The
ShouldPromoteandShouldDemotemethods provide a clean decision API. They are correctly markedconstsince they only inspect state without modification. The comprehensive documentation clearly explains the parameters and return values.
146-149: Mutual exclusivity of promotion/demotion flags is enforced.The implementation correctly ensures mutual exclusivity:
JoinExistingQueueincludes an explicit assertion:assert(!(fPromotion && fDemotion))at line 1166StartNewQueueenforces it through control flow withif (fPromotion) { ... } else if (fDemotion) { ... } else { return false; }at lines 1400-1444- All callers pass mutually exclusive values (one true, one false)
src/coinjoin/coinjoin.cpp (4)
93-134: LGTM: Correct V24-aware structure validation.The
IsValidStructureimplementation correctly:
- Computes V24 activation via
DeploymentActiveAtwith nullpindexhandling- Rejects unbalanced entries pre-V24 (line 104)
- Adjusts max inputs: 200 post-V24 vs 180 pre-V24 (lines 114-116)
- Validates all outputs are valid denominations and P2PKH (lines 123-127)
- Documents that value sum validation is deferred to
IsValidInOuts(lines 129-131)The implementation aligns with the test patterns that use
nullptrfor pre-fork scenarios.
210-363: LGTM: Comprehensive entry validation with proper V24 support.The extended
IsValidInOutsimplementation provides thorough validation:
- V24 activation check (lines 220-225): Correctly acquires
cs_mainto access chain tip- Entry classification (lines 227-257): Properly categorizes STANDARD/PROMOTION/DEMOTION/INVALID
- Denomination validation (lines 259-285): Correctly determines expected denoms based on entry type
- Script validation (lines 287-311): Validates P2PKH requirement and prevents duplicate scripts
- UTXO validation (lines 324-347): Ensures inputs exist and are spendable
- Fee enforcement (lines 349-355): Correctly requires zero fees for CoinJoin
- Logging (lines 357-362): Includes entry type for debugging
The implementation correctly handles the complexity of three entry types while maintaining the core CoinJoin guarantees.
582-631: LGTM: Thorough promotion entry validation.The
ValidatePromotionEntryimplementation correctly validates:
- Input count must be exactly
PROMOTION_RATIO(10)- Output count must be exactly 1
- Larger adjacent denomination must exist (not largest)
- Output denomination must match the larger adjacent denomination
- Output must be P2PKH script
Error messages are descriptive and use
LogPrintfor debugging. The validation prevents malformed promotion entries from entering the pool.
633-673: LGTM: Complete demotion entry validation.The
ValidateDemotionEntryimplementation correctly validates:
- Input count must be exactly 1
- Output count must be exactly
PROMOTION_RATIO(10)- All outputs must be at session denomination (smaller)
- All outputs must be P2PKH scripts
The loop at lines 657-670 correctly validates all outputs, not just the first, ensuring the entire demotion structure is valid.
src/coinjoin/client.cpp (6)
11-11: LGTM!The new includes support V24 deployment detection and chain state access required by the promotion/demotion logic.
Also applies to: 25-25
510-525: LGTM! Filtering approach correctly handles promotion/demotion entry structure.The filtering removes empty placeholder inputs/outputs from promotion (10 inputs → 1 output) and demotion (1 input → 10 outputs) entries before relay. The updated log reflects actual counts submitted.
991-1045: Verify denomination promotion/demotion priority order is intentional.The loop processes adjacent denomination pairs in order i=0 to i=3, which corresponds to:
- i=0: 10 DASH ↔ 1 DASH
- i=1: 1 DASH ↔ 0.1 DASH
- i=2: 0.1 DASH ↔ 0.01 DASH
- i=3: 0.01 DASH ↔ 0.001 DASH
Since the function returns on first successful promotion/demotion (lines 1019, 1023, 1037, 1041), larger denominations have priority. Confirm this matches the intended behavior, or consider processing in reverse order to prioritize completing smaller denominations first (which may be more common operations).
1598-1617: LGTM! Clear separation of promotion/demotion and standard mixing flows.The conditional logic correctly routes to
PreparePromotionEntry/PrepareDemotionEntryfor V24 flows and falls back to standard mixing otherwise. Error handling is appropriate.
1768-1794: Add bounds checking and remove redundant IsSpent check.Line 1775 accesses
wtx.tx->vout[outpoint.n]without validating bounds (same issue appears at line 1846 in PrepareDemotionEntry).Additionally, the
IsSpentcheck at lines 1781-1784 is redundant since coins inm_vecPromotionInputsare already locked (preventing spending). If a locked coin is spent, that indicates a critical wallet inconsistency that should be handled differently (assertion or stronger error).🔎 Add bounds check and consider removing redundant IsSpent
const wallet::CWalletTx& wtx = it->second; + if (outpoint.n >= wtx.tx->vout.size()) { + strErrorRet = "Invalid promotion input index"; + return false; + } - // Validate the UTXO is still spendable - if (m_wallet->IsSpent(outpoint)) { - strErrorRet = "Promotion input has been spent"; - return false; - } + // Locked coins should never be spent - assert if this occurs + assert(!m_wallet->IsSpent(outpoint));Apply similar changes to PrepareDemotionEntry at lines 1846-1855.
⛔ Skipped due to learnings
Learnt from: CR Repo: dashpay/dash PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T16:41:22.457Z Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
295-311: SetNull() properly handles promotion/demotion input unlocking with appropriate defensive checks.The code correctly identifies and addresses a real tracking gap: promotion/demotion inputs are locked via
m_wallet->LockCoin()inJoinExistingQueue/StartNewQueuebut are not added tovecOutPointLockeduntilPreparePromotionEntry/PrepareDemotionEntry. Early session failures (connection issues, timeouts, validation failures) that callSetNull()before these preparation methods would leave inputs locked in the wallet but missing fromvecOutPointLocked. The SetNull() code transfersm_vecPromotionInputstovecOutPointLockedbefore clearing state, ensuringUnlockCoins()will properly unlock them. The duplicate check is necessary and defensive—it prevents adding the same outpoint multiple times tovecOutPointLocked, which could occur if the code path executes multiple times or encounters edge cases where inputs exist in both sources. This implementation is sound.
| if (fPromotion && nTargetDenom != 0) { | ||
| // Promotion: select 10 fully-mixed coins of the smaller denomination | ||
| auto vecCoins = m_wallet->SelectFullyMixedForPromotion(nTargetDenom, CoinJoin::PROMOTION_RATIO); | ||
| if (static_cast<int>(vecCoins.size()) < CoinJoin::PROMOTION_RATIO) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Not enough fully-mixed coins for promotion\n"); | ||
| continue; | ||
| } | ||
| // Convert COutPoints to CTxDSIn | ||
| LOCK(m_wallet->cs_wallet); | ||
| for (const auto& outpoint : vecCoins) { | ||
| const auto it = m_wallet->mapWallet.find(outpoint.hash); | ||
| if (it != m_wallet->mapWallet.end()) { | ||
| const wallet::CWalletTx& wtx = it->second; | ||
| CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey, | ||
| m_wallet->GetRealOutpointCoinJoinRounds(outpoint)); | ||
| vecTxDSInTmp.push_back(txdsin); | ||
| } | ||
| } | ||
| if (static_cast<int>(vecTxDSInTmp.size()) < CoinJoin::PROMOTION_RATIO) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Failed to build promotion inputs\n"); | ||
| continue; | ||
| } | ||
| // Lock promotion inputs immediately to prevent race conditions | ||
| // with other concurrent CoinJoin sessions | ||
| for (const auto& outpoint : vecCoins) { | ||
| m_wallet->LockCoin(outpoint); | ||
| } | ||
| } else if (fDemotion && nTargetDenom != 0) { |
There was a problem hiding this comment.
Add bounds checking for outpoint index when accessing wallet transactions.
Lines 1210-1216 access wtx.tx->vout[outpoint.n] without validating outpoint.n < wtx.tx->vout.size(). Although unlikely, a corrupted outpoint could cause an out-of-bounds access.
🔎 Add bounds check
for (const auto& outpoint : vecCoins) {
const auto it = m_wallet->mapWallet.find(outpoint.hash);
if (it != m_wallet->mapWallet.end()) {
const wallet::CWalletTx& wtx = it->second;
+ if (outpoint.n >= wtx.tx->vout.size()) {
+ WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Invalid outpoint index for promotion\n");
+ continue;
+ }
CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey,
m_wallet->GetRealOutpointCoinJoinRounds(outpoint));
vecTxDSInTmp.push_back(txdsin);
}
}📝 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 (fPromotion && nTargetDenom != 0) { | |
| // Promotion: select 10 fully-mixed coins of the smaller denomination | |
| auto vecCoins = m_wallet->SelectFullyMixedForPromotion(nTargetDenom, CoinJoin::PROMOTION_RATIO); | |
| if (static_cast<int>(vecCoins.size()) < CoinJoin::PROMOTION_RATIO) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Not enough fully-mixed coins for promotion\n"); | |
| continue; | |
| } | |
| // Convert COutPoints to CTxDSIn | |
| LOCK(m_wallet->cs_wallet); | |
| for (const auto& outpoint : vecCoins) { | |
| const auto it = m_wallet->mapWallet.find(outpoint.hash); | |
| if (it != m_wallet->mapWallet.end()) { | |
| const wallet::CWalletTx& wtx = it->second; | |
| CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey, | |
| m_wallet->GetRealOutpointCoinJoinRounds(outpoint)); | |
| vecTxDSInTmp.push_back(txdsin); | |
| } | |
| } | |
| if (static_cast<int>(vecTxDSInTmp.size()) < CoinJoin::PROMOTION_RATIO) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Failed to build promotion inputs\n"); | |
| continue; | |
| } | |
| // Lock promotion inputs immediately to prevent race conditions | |
| // with other concurrent CoinJoin sessions | |
| for (const auto& outpoint : vecCoins) { | |
| m_wallet->LockCoin(outpoint); | |
| } | |
| } else if (fDemotion && nTargetDenom != 0) { | |
| if (fPromotion && nTargetDenom != 0) { | |
| // Promotion: select 10 fully-mixed coins of the smaller denomination | |
| auto vecCoins = m_wallet->SelectFullyMixedForPromotion(nTargetDenom, CoinJoin::PROMOTION_RATIO); | |
| if (static_cast<int>(vecCoins.size()) < CoinJoin::PROMOTION_RATIO) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Not enough fully-mixed coins for promotion\n"); | |
| continue; | |
| } | |
| // Convert COutPoints to CTxDSIn | |
| LOCK(m_wallet->cs_wallet); | |
| for (const auto& outpoint : vecCoins) { | |
| const auto it = m_wallet->mapWallet.find(outpoint.hash); | |
| if (it != m_wallet->mapWallet.end()) { | |
| const wallet::CWalletTx& wtx = it->second; | |
| if (outpoint.n >= wtx.tx->vout.size()) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Invalid outpoint index for promotion\n"); | |
| continue; | |
| } | |
| CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey, | |
| m_wallet->GetRealOutpointCoinJoinRounds(outpoint)); | |
| vecTxDSInTmp.push_back(txdsin); | |
| } | |
| } | |
| if (static_cast<int>(vecTxDSInTmp.size()) < CoinJoin::PROMOTION_RATIO) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::JoinExistingQueue -- Failed to build promotion inputs\n"); | |
| continue; | |
| } | |
| // Lock promotion inputs immediately to prevent race conditions | |
| // with other concurrent CoinJoin sessions | |
| for (const auto& outpoint : vecCoins) { | |
| m_wallet->LockCoin(outpoint); | |
| } | |
| } else if (fDemotion && nTargetDenom != 0) { |
🤖 Prompt for AI Agents
In src/coinjoin/client.cpp around lines 1200-1227, the code accesses
wtx.tx->vout[outpoint.n] without verifying outpoint.n is within bounds; add a
bounds check (outpoint.n < wtx.tx->vout.size()) before accessing the vout, skip
the outpoint if invalid and log a warning, and ensure the later locking loop
only locks the outpoints that were successfully converted (e.g., lock using the
outpoints corresponding to vecTxDSInTmp or maintain a parallel validOutPoints
list) so you don't attempt to lock or use invalid entries.
| for (const auto& outpoint : vecCoins) { | ||
| m_wallet->LockCoin(outpoint); | ||
| } |
There was a problem hiding this comment.
Critical: Coin locking leak when queue join attempt fails after locking.
Promotion coins are locked at lines 1224-1226 (demotion at 1249-1251), but if a subsequent check fails (e.g., IsMasternodeOrDisconnectRequested at line 1263), the continue statement leaves coins permanently locked without tracking them for unlock.
These locked coins are not added to m_vecPromotionInputs (only happens at 1282-1285 after successful join) and won't be unlocked until wallet restart or manual intervention.
🔎 Track locked coins immediately for cleanup
Move coin locking after the masternode validation checks, or unlock coins before continue:
// Lock promotion inputs immediately to prevent race conditions
// with other concurrent CoinJoin sessions
+ std::vector<COutPoint> vecLockedCoins;
for (const auto& outpoint : vecCoins) {
m_wallet->LockCoin(outpoint);
+ vecLockedCoins.push_back(outpoint);
}
} else if (fDemotion && nTargetDenom != 0) {
// ... demotion logic ...
} else {
// ... standard logic ...
}
m_clientman.AddUsedMasternode(dmn->proTxHash);
if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) {
+ // Unlock coins before continuing
+ if (fPromotion && !vecLockedCoins.empty()) {
+ for (const auto& outpoint : vecLockedCoins) {
+ m_wallet->UnlockCoin(outpoint);
+ }
+ }
WalletCJLogPrint(m_wallet, /* Continued */Apply similar pattern for demotion and add cleanup before all continue statements in the loop.
Also applies to: 1249-1251, 1276-1301
🤖 Prompt for AI Agents
In src/coinjoin/client.cpp around lines 1224-1226 (and similarly 1249-1251,
1276-1301), promotion/demotion coins are locked before validation checks and
early continue paths (e.g., IsMasternodeOrDisconnectRequested at ~1263) can
leave them permanently locked; either move the LockCoin() calls to after all
failure checks succeed, or immediately record each locked outpoint into a local
tracking container (e.g., vector<COutPoint> lockedThisAttempt) right when you
call LockCoin(), and ensure every early-return/continue path unlocks all entries
in that container before continuing; apply the same pattern to demotion code and
any other places in the loop that lock coins so all locked coins are always
unlocked on error paths or only locked after validation passes.
| if (fPromotion) { | ||
| // Promotion: need 10 fully-mixed coins of the target (smaller) denomination | ||
| auto vecCoins = m_wallet->SelectFullyMixedForPromotion(nTargetDenom, CoinJoin::PROMOTION_RATIO); | ||
| if (static_cast<int>(vecCoins.size()) < CoinJoin::PROMOTION_RATIO) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- Not enough fully-mixed coins for promotion\n"); | ||
| return false; | ||
| } | ||
| // Convert to CTxDSIn for storage | ||
| LOCK(m_wallet->cs_wallet); | ||
| for (const auto& outpoint : vecCoins) { | ||
| const auto it = m_wallet->mapWallet.find(outpoint.hash); | ||
| if (it != m_wallet->mapWallet.end()) { | ||
| const wallet::CWalletTx& wtx = it->second; | ||
| CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey, | ||
| m_wallet->GetRealOutpointCoinJoinRounds(outpoint)); | ||
| vecTxDSInTmp.push_back(txdsin); | ||
| } | ||
| } | ||
| if (static_cast<int>(vecTxDSInTmp.size()) < CoinJoin::PROMOTION_RATIO) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- Failed to build promotion inputs\n"); | ||
| return false; | ||
| } | ||
| // Lock promotion inputs immediately to prevent race conditions | ||
| for (const auto& outpoint : vecCoins) { | ||
| m_wallet->LockCoin(outpoint); | ||
| } | ||
| } else if (fDemotion) { |
There was a problem hiding this comment.
Critical: Same coin locking leak when masternode selection fails.
The same resource leak exists here: coins are locked at lines 1424-1426 (demotion at 1445-1447) before entering the masternode selection loop. If the loop exhausts all retries (line 1514-1516), coins remain locked but m_vecPromotionInputs is never populated, so they won't be unlocked.
Additionally, line 1410 accesses wtx.tx->vout[outpoint.n] without bounds checking (same issue as JoinExistingQueue).
🔎 Defer coin locking until after masternode selection
for (const auto& outpoint : vecCoins) {
const auto it = m_wallet->mapWallet.find(outpoint.hash);
if (it != m_wallet->mapWallet.end()) {
const wallet::CWalletTx& wtx = it->second;
+ if (outpoint.n >= wtx.tx->vout.size()) {
+ WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- Invalid outpoint index\n");
+ return false;
+ }
CTxDSIn txdsin(CTxIn(outpoint), wtx.tx->vout[outpoint.n].scriptPubKey,
m_wallet->GetRealOutpointCoinJoinRounds(outpoint));
vecTxDSInTmp.push_back(txdsin);
}
}
if (static_cast<int>(vecTxDSInTmp.size()) < CoinJoin::PROMOTION_RATIO) {
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- Failed to build promotion inputs\n");
return false;
}
- // Lock promotion inputs immediately to prevent race conditions
- for (const auto& outpoint : vecCoins) {
- m_wallet->LockCoin(outpoint);
- }
} else if (fDemotion) {
// ... similar changes for demotion ...
}
int nTries = 0;
const auto mnList = m_dmnman.GetListAtChainTip();
const int nMnCount = mnList.GetValidMNsCount();
const int nWeightedMnCount = mnList.GetValidWeightedMNsCount();
while (nTries < 10) {
auto dmn = m_clientman.GetRandomNotUsedMasternode();
if (!dmn) {
strAutoDenomResult = _("Can't find random Masternode.");
WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- %s\n", strAutoDenomResult.original);
return false;
}
// ... masternode checks ...
+ // Lock coins only after successful masternode selection
+ if (fPromotion) {
+ for (const auto& txdsin : vecTxDSInTmp) {
+ m_wallet->LockCoin(txdsin.prevout);
+ }
+ } else if (fDemotion) {
+ if (!vecTxDSInTmp.empty()) {
+ m_wallet->LockCoin(vecTxDSInTmp[0].prevout);
+ }
+ }
+
// Store promotion/demotion state and inputs
m_fPromotion = fPromotion;This ensures coins are only locked if we successfully found a masternode and are about to join the queue.
Also applies to: 1454-1517
🤖 Prompt for AI Agents
In src/coinjoin/client.cpp around lines 1401-1427 (and similarly 1454-1517 for
demotion), defer locking selected coins until after a masternode has been
successfully chosen; currently coins are locked immediately (lines ~1424-1426 /
1445-1447) and will remain locked if masternode selection exhausts retries. Also
add a bounds check before accessing wtx.tx->vout[outpoint.n] when building
vecTxDSInTmp: verify outpoint.n < wtx.tx->vout.size() and skip/log the outpoint
if invalid. Fix by (1) removing the immediate LockCoin loop and instead lock
coins only after masternode selection succeeds and you are about to populate
m_vecPromotionInputs/m_vecDemotionInputs, and (2) if you must lock earlier,
track locked outpoints and ensure you unlock them on all failure/early-return
paths; finally ensure the same changes are applied to both the promotion and
demotion blocks.
| bool CCoinJoinClientManager::ShouldPromote(int nSmallerDenom, int nLargerDenom) const | ||
| { | ||
| // Validate denominations are adjacent | ||
| if (!CoinJoin::AreAdjacentDenominations(nSmallerDenom, nLargerDenom)) { | ||
| return false; | ||
| } | ||
|
|
||
| const int nGoal = CCoinJoinClientOptions::GetDenomsGoal(); | ||
| const int nHalfGoal = nGoal / 2; | ||
|
|
||
| const int nSmallerCount = m_wallet->CountCoinsByDenomination(nSmallerDenom, /*fFullyMixedOnly=*/false); | ||
| const int nLargerCount = m_wallet->CountCoinsByDenomination(nLargerDenom, /*fFullyMixedOnly=*/false); | ||
|
|
||
| // Don't sacrifice a denomination that's still being built up | ||
| if (nSmallerCount < nHalfGoal) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate how far each is from goal (0 if at or above goal) | ||
| const int nSmallerDeficit = std::max(0, nGoal - nSmallerCount); | ||
| const int nLargerDeficit = std::max(0, nGoal - nLargerCount); | ||
|
|
||
| // Promote if: | ||
| // 1. Smaller denom has at least half the goal (above check) | ||
| // 2. Larger denomination is further from goal (needs more help) | ||
| // 3. Gap exceeds threshold to prevent oscillation | ||
| // 4. Have 10 fully-mixed coins to promote | ||
| const int nFullyMixedCount = m_wallet->CountCoinsByDenomination(nSmallerDenom, /*fFullyMixedOnly=*/true); | ||
| if (nFullyMixedCount < CoinJoin::PROMOTION_RATIO) { | ||
| return false; | ||
| } | ||
|
|
||
| return (nLargerDeficit > nSmallerDeficit + CoinJoin::GAP_THRESHOLD); | ||
| } | ||
|
|
||
| bool CCoinJoinClientManager::ShouldDemote(int nLargerDenom, int nSmallerDenom) const | ||
| { | ||
| // Validate denominations are adjacent | ||
| if (!CoinJoin::AreAdjacentDenominations(nLargerDenom, nSmallerDenom)) { | ||
| return false; | ||
| } | ||
|
|
||
| const int nGoal = CCoinJoinClientOptions::GetDenomsGoal(); | ||
| const int nHalfGoal = nGoal / 2; | ||
|
|
||
| const int nLargerCount = m_wallet->CountCoinsByDenomination(nLargerDenom, /*fFullyMixedOnly=*/false); | ||
| const int nSmallerCount = m_wallet->CountCoinsByDenomination(nSmallerDenom, /*fFullyMixedOnly=*/false); | ||
|
|
||
| // Don't sacrifice a denomination that's still being built up | ||
| if (nLargerCount < nHalfGoal) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate how far each is from goal (0 if at or above goal) | ||
| const int nSmallerDeficit = std::max(0, nGoal - nSmallerCount); | ||
| const int nLargerDeficit = std::max(0, nGoal - nLargerCount); | ||
|
|
||
| // Demote if: | ||
| // 1. Larger denom has at least half the goal (above check) | ||
| // 2. Smaller denomination is further from goal (needs more help) | ||
| // 3. Gap exceeds threshold to prevent oscillation | ||
| return (nSmallerDeficit > nLargerDeficit + CoinJoin::GAP_THRESHOLD); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for GAP_THRESHOLD definition in the codebase
rg "GAP_THRESHOLD" --type h --type cpp -A 2 -B 2Repository: dashpay/dash
Length of output: 3792
🏁 Script executed:
#!/bin/bash
# Find default nCoinJoinDenomsGoal and understand its usage
rg "nCoinJoinDenomsGoal|GetDenomsGoal" --type cpp --type h -B 2 -A 2 | head -100Repository: dashpay/dash
Length of output: 7609
🏁 Script executed:
#!/bin/bash
# Find the default and min/max values for denoms goal
rg "DEFAULT_COINJOIN_DENOMS_GOAL|MIN_COINJOIN_DENOMS_GOAL|MAX_COINJOIN_DENOMS_GOAL" --type cpp --type hRepository: dashpay/dash
Length of output: 1301
GAP_THRESHOLD is fixed at 10 and may not scale appropriately for extreme goal values.
The promotion/demotion logic uses a hardcoded GAP_THRESHOLD = 10 (in src/coinjoin/common.h). For the default goal of 50, this represents a 20% deficit gap threshold, which is reasonable. However, the fixed constant doesn't scale with nCoinJoinDenomsGoal:
- At minimum goal (10): threshold is 100% of goal—overly restrictive for rebalancing
- At maximum goal (100,000): threshold is 0.01% of goal—insufficient to prevent oscillation
The logic itself is correct (proper adjacency validation, half-goal checks, fully-mixed coin requirements). Consider making GAP_THRESHOLD adaptive (e.g., goal / 5) to ensure consistent rebalancing behavior across all valid goal configurations (10–100,000).
|
This pull request has conflicts, please rebase. |
2 participants
Please remove the italicized help prompts before submitting or merging
Provide a general summary of your changes in the Title above
Pull requests without a rationale and clear improvement may be closed
immediately.
Please provide clear motivation for your patch and explain how it improves
Dash Core user experience or Dash Core developer experience
significantly:
src/test/) orfunctional tests (see
test/). Contributors should note which tests covermodified code. If no tests exist for a region of modified code, new tests
should accompany the change.
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Dash Core, if possible.
Issue being fixed or feature implemented
What was done?
Describe your changes in detail
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.