feat: coinjoin promotion / demotion #7052
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis pull request introduces Post-V24 promotion and demotion capabilities to the CoinJoin client protocol. The changes add session-level state tracking for promotion/demotion operations, extend queue joining and starting logic to accept denomination targets, implement denomination validation helpers for adjacency and navigation, introduce deployment-aware validation that distinguishes standard (1:1) entries from promotion (10→1) and demotion (1→10) entries, add dynamic input limits based on V24 activation status, implement wallet-level denomination selection and fully-mixed coin tracking methods, and provide comprehensive unit test coverage for pre- and post-fork scenarios. Sequence DiagramsequenceDiagram
participant ClientMgr as CoinJoinClientManager
participant ClientSess as CoinJoinClientSession
participant Wallet as CWallet
participant Server as CoinJoinServer
participant Chain as Chain/BlockIndex
rect rgb(240, 248, 255)
Note over ClientMgr,Chain: Promotion Opportunity Detected
ClientMgr->>ClientMgr: DoAutomaticDenominating()
ClientMgr->>ClientMgr: Iterate adjacent denom pairs
ClientMgr->>ClientMgr: ShouldPromote(smallDenom, largeDenom)?
end
rect rgb(230, 250, 230)
Note over ClientMgr,Chain: Queue Selection/Creation
alt Join Existing Queue
ClientMgr->>ClientSess: JoinExistingQueue(nBalance, connman,<br/>nTargetDenom, fPromotion, fDemotion)
ClientSess->>ClientSess: Filter queues by target denomination
ClientSess->>Wallet: GetTxDSInsByDenomination(nTargetDenom)
else Start New Queue
ClientMgr->>ClientSess: StartNewQueue(nBalance, connman,<br/>nTargetDenom, fPromotion, fDemotion)
ClientSess->>Wallet: SelectFullyMixedForPromotion(smallDenom, 10)
ClientSess->>ClientSess: Store m_vecPromotionInputs
end
end
rect rgb(255, 250, 240)
Note over ClientMgr,Chain: Prepare & Submit Entry
ClientSess->>ClientSess: SubmitDenominate(connman)
alt Promotion Path
ClientSess->>ClientSess: PreparePromotionEntry()
Note over ClientSess: Lock 10 inputs (smallDenom)
Note over ClientSess: Build output (largeDenom)
else Demotion Path
ClientSess->>ClientSess: PrepareDemotionEntry()
Note over ClientSess: Lock input (largeDenom)
Note over ClientSess: Build 10 outputs (smallDenom)
end
ClientSess->>ClientSess: SendDenominate(vecPSInOutPairs)
end
rect rgb(255, 240, 245)
Note over ClientMgr,Chain: Validation & Acceptance
ClientSess->>Server: Submit entry via broadcast
Server->>Chain: Retrieve block index for V24 check
Server->>Server: AddEntry() checks nMaxEntryInputs
Note over Server: Post-V24: allow 10 inputs (PROMOTION_RATIO)<br/>Pre-V24: allow 9 inputs (COINJOIN_ENTRY_MAX_SIZE)
Server->>Server: IsValidInOuts() validates denomination structure
Note over Server: Classify: PROMOTION/DEMOTION/STANDARD<br/>Validate input/output denomination pairs<br/>Verify total in == total out
Server-->>ClientSess: Entry accepted or rejected
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
1 participant
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.