feat: coinjoin promotion / demotion#7052
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds post-V24 CoinJoin promotion and demotion between adjacent denominations. The client now tracks rebalance state, selects and locks special inputs, and prepares promotion/demotion entries for submission. CoinJoin validation, DSTX structure checks, server acceptance rules, and wallet denomination helpers were updated for the new entry shapes. Tests and release notes were expanded to cover the new flows. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClientMgr as CCoinJoinClientManager
participant ClientSess as CCoinJoinClientSession
participant Wallet as CWallet
participant Server as CCoinJoinServer
participant Net as ValidateDSTX
ClientMgr->>ClientMgr: DoAutomaticDenominating()
ClientMgr->>ClientMgr: ShouldPromote / ShouldDemote
ClientMgr->>ClientSess: JoinExistingQueue() or StartNewQueue()
ClientSess->>Wallet: SelectTxDSInsByDenomination / SelectFullyMixedForPromotion
ClientSess->>ClientSess: SubmitDenominate()
ClientSess->>Server: SendDenominate()
Server->>Server: ProcessDSVIN / AddEntry / CheckPool
Server->>Net: dstx.IsValidStructure(pindex, chainman, ...)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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)
PastaPastaPasta
force-pushed
the
feat/coinjoin-promotion
branch
from
b70ccef to
d140732
Compare
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)
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.
| 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. |
PastaPastaPasta
force-pushed
the
feat/coinjoin-promotion
branch
from
d140732 to
bc4b034
Compare
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)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/coinjoin/coinjoin.cpp (1)
219-360:⚠️ Potential issue | 🟠 MajorThis validator now accepts non-uniform CoinJoin entries.
The new
PROMOTION/DEMOTIONbranches explicitly accept 10:1 and 1:10 shapes, so CoinJoin validation no longer enforces uniform denomination outputs inside a session. If this protocol change is intentional, the repository rule needs to be updated alongside it.As per coding guidelines,
src/coinjoin/**/*.{cpp,h}: CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs.Also applies to: 586-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/coinjoin.cpp` around lines 219 - 360, The validator now permits non-uniform PROMOTION/DEMOTION entries (10:1 and 1:10) which breaks the rule that CoinJoin sessions must have uniform denomination outputs; revert this by disabling the PROMOTION/DEMOTION branches so only STANDARD entries are allowed: change the EntryType detection (symbols: EntryType, fV24Active, CoinJoin::PROMOTION_RATIO) to treat any inputs/outputs size mismatch as an error (preserve the existing pre-V24 error path), and ensure checkTxOut and denomination expectations always use nSessionDenom (symbol: nSessionDenom, checkTxOut) so all inputs/outputs are validated against the session denom only.
♻️ Duplicate comments (2)
src/coinjoin/client.cpp (2)
1208-1214:⚠️ Potential issue | 🟠 MajorBounds-check
outpoint.nbefore indexingvout.Both loops trust
outpoint.nand readwtx.tx->vout[outpoint.n]unchecked. A stale or inconsistent outpoint here becomes undefined behaviour instead of a logged skip.Also applies to: 1410-1416
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 1208 - 1214, The code indexes wtx.tx->vout with outpoint.n without bounds checking which can cause UB for stale/inconsistent outpoints; before constructing CTxDSIn in the loop over vecCoins (and the similar loop around the CTxDSIn creation at the 1410-1416 region), verify that outpoint.n is less than wtx.tx->vout.size() and if not, log a warning (including the outpoint/hash and n) and continue to the next outpoint instead of indexing; apply the same guard around every use of wtx.tx->vout[outpoint.n] (e.g., where CTxDSIn is built) to prevent unsafe access.
1221-1225:⚠️ Potential issue | 🔴 CriticalEarly exits still leak locked promotion/demotion coins.
These
LockCoin()calls happen before the later winner/rate-limit/connection rejection branches, butm_vecPromotionInputsis not populated until Line 1279 / Line 1500 and nothing adds these outpoints tovecOutPointLockedon the early exits. The newSetNull()fallback therefore cannot recover them, so a failed attempt can leave user funds locked indefinitely.Also applies to: 1247-1251, 1262-1265, 1423-1426, 1444-1448, 1461-1488, 1515-1516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 1221 - 1225, The loop that calls m_wallet->LockCoin(outpoint) on vecCoins can leak locks on early exits because vecOutPointLocked isn't populated and SetNull() can't recover them; either move the locking until after winner/rate-limit/connection checks (so promotions in m_vecPromotionInputs are set) or, simpler, immediately record each locked outpoint by pushing it into vecOutPointLocked inside the same loop (for each outpoint in vecCoins) so any subsequent early-exit cleanup (SetNull()/unlock logic) can find and release them; update all analogous locking sites (the other ranges mentioned) to follow the same pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/coinjoin/client.cpp`:
- Around line 296-311: The fallback only appends m_vecPromotionInputs into
vecOutPointLocked but never unlocks them, leaving some coins locked
indefinitely; call UnlockCoins(...) on the promotion inputs before
clearing/resetting to ensure they are released. Specifically, in the same
cleanup path that touches m_vecPromotionInputs and flags (m_fPromotion,
m_fDemotion), invoke UnlockCoins(m_vecPromotionInputs) (or UnlockCoins with the
appropriate overload) so those outpoints are unlocked even if they were never
added earlier to vecOutPointLocked; keep this change consistent with other
callers like ResetPool(), CompletedTransaction(), and the POOL_STATE_ERROR
timeout branch that expect UnlockCoins() to run before SetNull()/clearing state.
In `@src/coinjoin/coinjoin.h`:
- Around line 296-297: Change the IsExpired method signature to accept the
correct Chainlock handler type: replace the parameter type
llmq::CChainLocksHandler with chainlock::ChainlockHandler in the IsExpired
declaration and any corresponding definition; if chainlock::ChainlockHandler
isn't visible here, add or update a forward declaration for class
ChainlockHandler inside the chainlock namespace (or include the appropriate
header) so the compiler can resolve chainlock::ChainlockHandler when compiling
the IsExpired(const CBlockIndex* pindex, const chainlock::ChainlockHandler&
clhandler) method.
In `@src/coinjoin/server.cpp`:
- Around line 231-233: The tip lookups calling m_chainman.ActiveChain().Tip()
and the subsequent DeploymentActiveAt check (using pindex,
Params().GetConsensus(), Consensus::DEPLOYMENT_V24) must be performed while
holding the chainstate lock to avoid races; wrap the code that obtains pindex
and evaluates fV24Active with LOCK(::cs_main) (the same pattern used in
CCoinJoinBaseSession::IsValidInOuts()) so the activation gate cannot change
mid-request. Apply the same LOCK(::cs_main) guarding to the analogous checks
around lines 620-622 as well.
- Around line 304-307: The finalize path that calls CreateFinalTransaction()
when all accepted participants have submitted must also enforce the minimum
standard-mixer count; update the branch that transitions from
POOL_STATE_ACCEPTING_ENTRIES to the finalize step (the code that currently calls
CreateFinalTransaction() when all submissions are present) to include the same
guard used in the timeout path: check GetStandardEntriesCount() >=
CoinJoin::GetMinPoolParticipants() (and CCoinJoinServer::HasTimedOut() logic as
appropriate) before invoking CreateFinalTransaction(), and abort/skip
finalization if the condition is not met so promotion/demotion-only sessions
cannot complete.
---
Outside diff comments:
In `@src/coinjoin/coinjoin.cpp`:
- Around line 219-360: The validator now permits non-uniform PROMOTION/DEMOTION
entries (10:1 and 1:10) which breaks the rule that CoinJoin sessions must have
uniform denomination outputs; revert this by disabling the PROMOTION/DEMOTION
branches so only STANDARD entries are allowed: change the EntryType detection
(symbols: EntryType, fV24Active, CoinJoin::PROMOTION_RATIO) to treat any
inputs/outputs size mismatch as an error (preserve the existing pre-V24 error
path), and ensure checkTxOut and denomination expectations always use
nSessionDenom (symbol: nSessionDenom, checkTxOut) so all inputs/outputs are
validated against the session denom only.
---
Duplicate comments:
In `@src/coinjoin/client.cpp`:
- Around line 1208-1214: The code indexes wtx.tx->vout with outpoint.n without
bounds checking which can cause UB for stale/inconsistent outpoints; before
constructing CTxDSIn in the loop over vecCoins (and the similar loop around the
CTxDSIn creation at the 1410-1416 region), verify that outpoint.n is less than
wtx.tx->vout.size() and if not, log a warning (including the outpoint/hash and
n) and continue to the next outpoint instead of indexing; apply the same guard
around every use of wtx.tx->vout[outpoint.n] (e.g., where CTxDSIn is built) to
prevent unsafe access.
- Around line 1221-1225: The loop that calls m_wallet->LockCoin(outpoint) on
vecCoins can leak locks on early exits because vecOutPointLocked isn't populated
and SetNull() can't recover them; either move the locking until after
winner/rate-limit/connection checks (so promotions in m_vecPromotionInputs are
set) or, simpler, immediately record each locked outpoint by pushing it into
vecOutPointLocked inside the same loop (for each outpoint in vecCoins) so any
subsequent early-exit cleanup (SetNull()/unlock logic) can find and release
them; update all analogous locking sites (the other ranges mentioned) to follow
the same pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1f8ea791-13b5-43f4-875c-8f47542e04b0
📒 Files selected for processing (6)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/server.cpp
| // Post-V24: Unlock promotion/demotion inputs before clearing state | ||
| // These coins were locked in JoinExistingQueue/StartNewQueue but may not | ||
| // have been added to vecOutPointLocked yet if the session failed early | ||
| if (!m_vecPromotionInputs.empty()) { | ||
| // Add to vecOutPointLocked so UnlockCoins() will handle them properly | ||
| // with its retry mechanism if the wallet is locked | ||
| for (const auto& outpoint : m_vecPromotionInputs) { | ||
| // Only add if not already in the list (avoid duplicates) | ||
| if (std::find(vecOutPointLocked.begin(), vecOutPointLocked.end(), outpoint) == vecOutPointLocked.end()) { | ||
| vecOutPointLocked.push_back(outpoint); | ||
| } | ||
| } | ||
| } | ||
| m_fPromotion = false; | ||
| m_fDemotion = false; | ||
| m_vecPromotionInputs.clear(); |
There was a problem hiding this comment.
This fallback runs after most unlock paths.
The code only appends m_vecPromotionInputs into vecOutPointLocked; it never unlocks them. Callers like ResetPool(), CompletedTransaction(), and the POOL_STATE_ERROR timeout branch already call UnlockCoins() before SetNull(), so these outpoints are either re-queued after they were unlocked or remain locked indefinitely if they were never tracked earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/client.cpp` around lines 296 - 311, The fallback only appends
m_vecPromotionInputs into vecOutPointLocked but never unlocks them, leaving some
coins locked indefinitely; call UnlockCoins(...) on the promotion inputs before
clearing/resetting to ensure they are released. Specifically, in the same
cleanup path that touches m_vecPromotionInputs and flags (m_fPromotion,
m_fDemotion), invoke UnlockCoins(m_vecPromotionInputs) (or UnlockCoins with the
appropriate overload) so those outpoints are unlocked even if they were never
added earlier to vecOutPointLocked; keep this change consistent with other
callers like ResetPool(), CompletedTransaction(), and the POOL_STATE_ERROR
timeout branch that expect UnlockCoins() to run before SetNull()/clearing state.
| [[nodiscard]] bool IsExpired(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler) const; | ||
| [[nodiscard]] bool IsValidStructure(const CBlockIndex* pindex) const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '34,40p' src/coinjoin/coinjoin.h
sed -n '292,297p' src/coinjoin/coinjoin.h
sed -n '420,445p' src/coinjoin/coinjoin.h
rg -n -C2 '\b(CChainLocksHandler|Chainlocks)\b' srcRepository: dashpay/dash
Length of output: 33868
Fix incorrect type name in IsExpired() signature.
Line 296 declares IsExpired with parameter const llmq::CChainLocksHandler& clhandler, but CChainLocksHandler does not exist in the llmq namespace. The correct type is chainlock::ChainlockHandler. Update the signature to use the correct type and add the necessary forward declaration if not already present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/coinjoin.h` around lines 296 - 297, Change the IsExpired method
signature to accept the correct Chainlock handler type: replace the parameter
type llmq::CChainLocksHandler with chainlock::ChainlockHandler in the IsExpired
declaration and any corresponding definition; if chainlock::ChainlockHandler
isn't visible here, add or update a forward declaration for class
ChainlockHandler inside the chainlock namespace (or include the appropriate
header) so the compiler can resolve chainlock::ChainlockHandler when compiling
the IsExpired(const CBlockIndex* pindex, const chainlock::ChainlockHandler&
clhandler) method.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7052 adds CoinJoin promotion (10 smaller → 1 larger) and demotion (1 larger → 10 smaller) gated by V24 activation. The core validation logic in IsValidInOuts is sound. Two blocking issues remain: (1) the full-session finalization path bypasses the new minimum standard mixer requirement, allowing sessions with zero privacy cover; (2) the new coin selection function skips the IsLockedCoin check, enabling concurrent sessions to double-spend the same inputs.
Reviewed commit: c2089b6
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: Full-session finalization bypasses minimum standard mixer check
src/coinjoin/server.cpp (lines 296-299)
CheckPool() has two finalization paths. The timeout path (line 306-307) correctly enforces GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants(). But the full-session path (line 296) only checks GetEntriesCount() == vecSessionCollaterals.size() — it does not verify that enough standard (1:1) entries exist. If all participants submit promotion/demotion entries, the session finalizes with zero standard mixers, providing no privacy cover.
Both review agents independently flagged this as the highest-priority issue.
💡 Suggested change
if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) {
if (GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n");
CreateFinalTransaction();
return;
}
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- all entries received but insufficient standard mixers (%d), waiting for timeout\n", GetStandardEntriesCount());
}
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [BLOCKING] lines 296-299: Full-session finalization bypasses minimum standard mixer check
CheckPool() has two finalization paths. The timeout path (line 306-307) correctly enforces `GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()`. But the full-session path (line 296) only checks `GetEntriesCount() == vecSessionCollaterals.size()` — it does not verify that enough standard (1:1) entries exist. If all participants submit promotion/demotion entries, the session finalizes with zero standard mixers, providing no privacy cover.
Both review agents independently flagged this as the highest-priority issue.
- [SUGGESTION] lines 230-232: V24 activation checks access ActiveChain().Tip() without cs_main
Two locations access `m_chainman.ActiveChain().Tip()` without holding cs_main:
- ProcessDSVIN (line 231): checks V24 activation for unbalanced entry validation
- AddEntry (line 620): checks V24 activation to determine nMaxEntryInputs
The tip pointer could become stale between read and use. The downstream IsValidInOuts() at coinjoin.cpp:222-225 does acquire cs_main properly, creating an inconsistency. These are race windows that could allow a brief period of incorrect accept/reject behavior during V24 activation.
In `src/wallet/coinjoin.cpp`:
- [BLOCKING] lines 443-471: SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation
SelectFullyMixedForPromotion() (line 443-471) iterates setWalletUTXO without checking IsLockedCoin(). The existing denomination selector at line 181 correctly filters with `IsSpent(target_outpoint) || IsLockedCoin(target_outpoint)`. Without this check, concurrent CoinJoin sessions can select and reserve the same UTXOs for promotion, leading to signing failures or double-spend attempts.
CountCoinsByDenomination() (line 414-441) has the same omission but is less critical since it's only used for counting.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 586-677: ValidatePromotionEntry/ValidateDemotionEntry are dead code in production
ValidatePromotionEntry() and ValidateDemotionEntry() are declared in coinjoin.h, implemented in coinjoin.cpp (lines 586-677), and extensively tested in coinjoin_inouts_tests.cpp. However, they are never called from production code — the actual validation is performed inline within IsValidInOuts(). This creates dead code and a maintenance risk: the tested functions could diverge from the actual validation path without detection.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths lack unit test coverage
The test at line 382 confirms pre-V24 rejection of unbalanced entries but explicitly notes (line 384) that post-V24 acceptance requires a CBlockIndex with V24 active, which is not set up. The post-V24 code paths — where promotion and demotion entries are actually accepted and validated — are not exercised in unit tests. This means the core new functionality is only covered by the standalone Validate*Entry tests (which are dead code in production, per finding above).
| std::vector<COutPoint> CWallet::SelectFullyMixedForPromotion(int nDenom, int nCount) const | ||
| { | ||
| std::vector<COutPoint> vecRet; | ||
| if (!CCoinJoinClientOptions::IsEnabled()) return vecRet; | ||
|
|
||
| const CAmount nDenomAmount = CoinJoin::DenominationToAmount(nDenom); | ||
| if (nDenomAmount <= 0) return vecRet; | ||
|
|
||
| LOCK(cs_wallet); | ||
| for (const auto& outpoint : setWalletUTXO) { | ||
| if (static_cast<int>(vecRet.size()) >= nCount) break; | ||
|
|
||
| const auto it{mapWallet.find(outpoint.hash)}; | ||
| if (it == mapWallet.end()) continue; | ||
|
|
||
| const CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | ||
| if (nValue != nDenomAmount) continue; | ||
|
|
||
| // Skip unconfirmed or conflicted | ||
| if (GetTxDepthInMainChain(it->second) < 1) continue; | ||
|
|
||
| // Must be fully mixed for promotion | ||
| if (!IsFullyMixed(outpoint)) continue; | ||
|
|
||
| vecRet.push_back(outpoint); | ||
| } | ||
|
|
||
| return vecRet; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation
SelectFullyMixedForPromotion() (line 443-471) iterates setWalletUTXO without checking IsLockedCoin(). The existing denomination selector at line 181 correctly filters with IsSpent(target_outpoint) || IsLockedCoin(target_outpoint). Without this check, concurrent CoinJoin sessions can select and reserve the same UTXOs for promotion, leading to signing failures or double-spend attempts.
CountCoinsByDenomination() (line 414-441) has the same omission but is less critical since it's only used for counting.
💡 Suggested change
| std::vector<COutPoint> CWallet::SelectFullyMixedForPromotion(int nDenom, int nCount) const | |
| { | |
| std::vector<COutPoint> vecRet; | |
| if (!CCoinJoinClientOptions::IsEnabled()) return vecRet; | |
| const CAmount nDenomAmount = CoinJoin::DenominationToAmount(nDenom); | |
| if (nDenomAmount <= 0) return vecRet; | |
| LOCK(cs_wallet); | |
| for (const auto& outpoint : setWalletUTXO) { | |
| if (static_cast<int>(vecRet.size()) >= nCount) break; | |
| const auto it{mapWallet.find(outpoint.hash)}; | |
| if (it == mapWallet.end()) continue; | |
| const CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | |
| if (nValue != nDenomAmount) continue; | |
| // Skip unconfirmed or conflicted | |
| if (GetTxDepthInMainChain(it->second) < 1) continue; | |
| // Must be fully mixed for promotion | |
| if (!IsFullyMixed(outpoint)) continue; | |
| vecRet.push_back(outpoint); | |
| } | |
| return vecRet; | |
| } | |
| Add `if (IsLockedCoin(outpoint)) continue;` after the `if (it == mapWallet.end()) continue;` check in SelectFullyMixedForPromotion(). Consider adding the same to CountCoinsByDenomination() for consistency. |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/coinjoin.cpp`:
- [BLOCKING] lines 443-471: SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation
SelectFullyMixedForPromotion() (line 443-471) iterates setWalletUTXO without checking IsLockedCoin(). The existing denomination selector at line 181 correctly filters with `IsSpent(target_outpoint) || IsLockedCoin(target_outpoint)`. Without this check, concurrent CoinJoin sessions can select and reserve the same UTXOs for promotion, leading to signing failures or double-spend attempts.
CountCoinsByDenomination() (line 414-441) has the same omission but is less critical since it's only used for counting.
| // This is a promotion or demotion entry - requires V24 activation | ||
| const CBlockIndex* pindex = m_chainman.ActiveChain().Tip(); | ||
| const bool fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24); |
There was a problem hiding this comment.
🟡 Suggestion: V24 activation checks access ActiveChain().Tip() without cs_main
Two locations access m_chainman.ActiveChain().Tip() without holding cs_main:
- ProcessDSVIN (line 231): checks V24 activation for unbalanced entry validation
- AddEntry (line 620): checks V24 activation to determine nMaxEntryInputs
The tip pointer could become stale between read and use. The downstream IsValidInOuts() at coinjoin.cpp:222-225 does acquire cs_main properly, creating an inconsistency. These are race windows that could allow a brief period of incorrect accept/reject behavior during V24 activation.
💡 Suggested change
| // This is a promotion or demotion entry - requires V24 activation | |
| const CBlockIndex* pindex = m_chainman.ActiveChain().Tip(); | |
| const bool fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24); | |
| Wrap both V24 checks in `LOCK(cs_main)` or hoist the activation check into a helper that acquires the lock. |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 230-232: V24 activation checks access ActiveChain().Tip() without cs_main
Two locations access `m_chainman.ActiveChain().Tip()` without holding cs_main:
- ProcessDSVIN (line 231): checks V24 activation for unbalanced entry validation
- AddEntry (line 620): checks V24 activation to determine nMaxEntryInputs
The tip pointer could become stale between read and use. The downstream IsValidInOuts() at coinjoin.cpp:222-225 does acquire cs_main properly, creating an inconsistency. These are race windows that could allow a brief period of incorrect accept/reject behavior during V24 activation.
| bool CoinJoin::ValidatePromotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | ||
| int nSessionDenom, PoolMessage& nMessageIDRet) | ||
| { | ||
| // Promotion: 10 inputs of smaller denom → 1 output of larger denom | ||
| // Session denom is the smaller denom (inputs) | ||
| nMessageIDRet = MSG_NOERR; | ||
|
|
||
| // Check input count | ||
| if (vecTxIn.size() != static_cast<size_t>(PROMOTION_RATIO)) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong input count %zu, expected %d\n", | ||
| vecTxIn.size(), PROMOTION_RATIO); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Check output count | ||
| if (vecTxOut.size() != 1) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong output count %zu, expected 1\n", | ||
| vecTxOut.size()); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Get the larger adjacent denomination | ||
| const int nLargerDenom = GetLargerAdjacentDenom(nSessionDenom); | ||
| if (nLargerDenom == 0) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: no larger adjacent denom for %s\n", | ||
| DenominationToString(nSessionDenom)); | ||
| nMessageIDRet = ERR_DENOM; | ||
| return false; | ||
| } | ||
|
|
||
| // Validate output is at larger denomination | ||
| const int nOutputDenom = AmountToDenomination(vecTxOut[0].nValue); | ||
| if (nOutputDenom != nLargerDenom) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output denom %s != expected %s\n", | ||
| DenominationToString(nOutputDenom), DenominationToString(nLargerDenom)); | ||
| nMessageIDRet = ERR_DENOM; | ||
| return false; | ||
| } | ||
|
|
||
| // Validate output is P2PKH | ||
| if (!vecTxOut[0].scriptPubKey.IsPayToPublicKeyHash()) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output is not P2PKH\n"); | ||
| nMessageIDRet = ERR_INVALID_SCRIPT; | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool CoinJoin::ValidateDemotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | ||
| int nSessionDenom, PoolMessage& nMessageIDRet) | ||
| { | ||
| // Demotion: 1 input of larger denom → 10 outputs of smaller denom | ||
| // Session denom is the smaller denom (outputs) | ||
| nMessageIDRet = MSG_NOERR; | ||
|
|
||
| // Check input count | ||
| if (vecTxIn.size() != 1) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong input count %zu, expected 1\n", | ||
| vecTxIn.size()); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Check output count | ||
| if (vecTxOut.size() != static_cast<size_t>(PROMOTION_RATIO)) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong output count %zu, expected %d\n", | ||
| vecTxOut.size(), PROMOTION_RATIO); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Validate all outputs are at session denomination and P2PKH | ||
| for (const auto& txout : vecTxOut) { | ||
| const int nDenom = AmountToDenomination(txout.nValue); | ||
| if (nDenom != nSessionDenom) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output denom %s != session denom %s\n", | ||
| DenominationToString(nDenom), DenominationToString(nSessionDenom)); | ||
| nMessageIDRet = ERR_DENOM; | ||
| return false; | ||
| } | ||
| if (!txout.scriptPubKey.IsPayToPublicKeyHash()) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output is not P2PKH\n"); | ||
| nMessageIDRet = ERR_INVALID_SCRIPT; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: ValidatePromotionEntry/ValidateDemotionEntry are dead code in production
ValidatePromotionEntry() and ValidateDemotionEntry() are declared in coinjoin.h, implemented in coinjoin.cpp (lines 586-677), and extensively tested in coinjoin_inouts_tests.cpp. However, they are never called from production code — the actual validation is performed inline within IsValidInOuts(). This creates dead code and a maintenance risk: the tested functions could diverge from the actual validation path without detection.
💡 Suggested change
| bool CoinJoin::ValidatePromotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | |
| int nSessionDenom, PoolMessage& nMessageIDRet) | |
| { | |
| // Promotion: 10 inputs of smaller denom → 1 output of larger denom | |
| // Session denom is the smaller denom (inputs) | |
| nMessageIDRet = MSG_NOERR; | |
| // Check input count | |
| if (vecTxIn.size() != static_cast<size_t>(PROMOTION_RATIO)) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong input count %zu, expected %d\n", | |
| vecTxIn.size(), PROMOTION_RATIO); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Check output count | |
| if (vecTxOut.size() != 1) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong output count %zu, expected 1\n", | |
| vecTxOut.size()); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Get the larger adjacent denomination | |
| const int nLargerDenom = GetLargerAdjacentDenom(nSessionDenom); | |
| if (nLargerDenom == 0) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: no larger adjacent denom for %s\n", | |
| DenominationToString(nSessionDenom)); | |
| nMessageIDRet = ERR_DENOM; | |
| return false; | |
| } | |
| // Validate output is at larger denomination | |
| const int nOutputDenom = AmountToDenomination(vecTxOut[0].nValue); | |
| if (nOutputDenom != nLargerDenom) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output denom %s != expected %s\n", | |
| DenominationToString(nOutputDenom), DenominationToString(nLargerDenom)); | |
| nMessageIDRet = ERR_DENOM; | |
| return false; | |
| } | |
| // Validate output is P2PKH | |
| if (!vecTxOut[0].scriptPubKey.IsPayToPublicKeyHash()) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output is not P2PKH\n"); | |
| nMessageIDRet = ERR_INVALID_SCRIPT; | |
| return false; | |
| } | |
| return true; | |
| } | |
| bool CoinJoin::ValidateDemotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | |
| int nSessionDenom, PoolMessage& nMessageIDRet) | |
| { | |
| // Demotion: 1 input of larger denom → 10 outputs of smaller denom | |
| // Session denom is the smaller denom (outputs) | |
| nMessageIDRet = MSG_NOERR; | |
| // Check input count | |
| if (vecTxIn.size() != 1) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong input count %zu, expected 1\n", | |
| vecTxIn.size()); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Check output count | |
| if (vecTxOut.size() != static_cast<size_t>(PROMOTION_RATIO)) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong output count %zu, expected %d\n", | |
| vecTxOut.size(), PROMOTION_RATIO); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Validate all outputs are at session denomination and P2PKH | |
| for (const auto& txout : vecTxOut) { | |
| const int nDenom = AmountToDenomination(txout.nValue); | |
| if (nDenom != nSessionDenom) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output denom %s != session denom %s\n", | |
| DenominationToString(nDenom), DenominationToString(nSessionDenom)); | |
| nMessageIDRet = ERR_DENOM; | |
| return false; | |
| } | |
| if (!txout.scriptPubKey.IsPayToPublicKeyHash()) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output is not P2PKH\n"); | |
| nMessageIDRet = ERR_INVALID_SCRIPT; | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| Either refactor IsValidInOuts() to delegate to these functions, or remove them and test IsValidInOuts() directly. |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 586-677: ValidatePromotionEntry/ValidateDemotionEntry are dead code in production
ValidatePromotionEntry() and ValidateDemotionEntry() are declared in coinjoin.h, implemented in coinjoin.cpp (lines 586-677), and extensively tested in coinjoin_inouts_tests.cpp. However, they are never called from production code — the actual validation is performed inline within IsValidInOuts(). This creates dead code and a maintenance risk: the tested functions could diverge from the actual validation path without detection.
| BOOST_AUTO_TEST_CASE(isvalidstructure_postfork_unbalanced_valid) | ||
| { | ||
| // Post-V24: Unbalanced vin/vout (promotion: 10 inputs, 1 output) should be valid | ||
| // We need a mock pindex that signals V24 active - for this test we use nullptr which means pre-fork | ||
| // This test validates that the structure check correctly identifies promotion structure | ||
|
|
||
| CCoinJoinBroadcastTx promo; | ||
| { | ||
| CMutableTransaction mtx; | ||
| // Promotion: 10 inputs of smaller denom -> 1 output of larger denom | ||
| const int nInputCount = CoinJoin::PROMOTION_RATIO; | ||
| const CAmount nLargerAmount = CoinJoin::DenominationToAmount(1 << 1); // 1.0 DASH | ||
|
|
||
| for (int i = 0; i < nInputCount; ++i) { | ||
| CTxIn in; | ||
| in.prevout = COutPoint(uint256::ONE, static_cast<uint32_t>(i)); | ||
| mtx.vin.push_back(in); | ||
| } | ||
| // 1 output of larger denom | ||
| CTxOut out{nLargerAmount, P2PKHScript(0x01)}; | ||
| mtx.vout.push_back(out); | ||
|
|
||
| promo.tx = MakeTransactionRef(mtx); | ||
| promo.m_protxHash = uint256::ONE; | ||
| } | ||
|
|
||
| // Pre-V24 (nullptr): unbalanced should fail | ||
| BOOST_CHECK(!promo.IsValidStructure(nullptr)); | ||
|
|
||
| // Note: Post-V24 test would require a valid CBlockIndex with V24 deployment active | ||
| // which requires more setup. The above confirms pre-fork rejection works. | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Post-V24 acceptance paths lack unit test coverage
The test at line 382 confirms pre-V24 rejection of unbalanced entries but explicitly notes (line 384) that post-V24 acceptance requires a CBlockIndex with V24 active, which is not set up. The post-V24 code paths — where promotion and demotion entries are actually accepted and validated — are not exercised in unit tests. This means the core new functionality is only covered by the standalone Validate*Entry tests (which are dead code in production, per finding above).
💡 Suggested change
| BOOST_AUTO_TEST_CASE(isvalidstructure_postfork_unbalanced_valid) | |
| { | |
| // Post-V24: Unbalanced vin/vout (promotion: 10 inputs, 1 output) should be valid | |
| // We need a mock pindex that signals V24 active - for this test we use nullptr which means pre-fork | |
| // This test validates that the structure check correctly identifies promotion structure | |
| CCoinJoinBroadcastTx promo; | |
| { | |
| CMutableTransaction mtx; | |
| // Promotion: 10 inputs of smaller denom -> 1 output of larger denom | |
| const int nInputCount = CoinJoin::PROMOTION_RATIO; | |
| const CAmount nLargerAmount = CoinJoin::DenominationToAmount(1 << 1); // 1.0 DASH | |
| for (int i = 0; i < nInputCount; ++i) { | |
| CTxIn in; | |
| in.prevout = COutPoint(uint256::ONE, static_cast<uint32_t>(i)); | |
| mtx.vin.push_back(in); | |
| } | |
| // 1 output of larger denom | |
| CTxOut out{nLargerAmount, P2PKHScript(0x01)}; | |
| mtx.vout.push_back(out); | |
| promo.tx = MakeTransactionRef(mtx); | |
| promo.m_protxHash = uint256::ONE; | |
| } | |
| // Pre-V24 (nullptr): unbalanced should fail | |
| BOOST_CHECK(!promo.IsValidStructure(nullptr)); | |
| // Note: Post-V24 test would require a valid CBlockIndex with V24 deployment active | |
| // which requires more setup. The above confirms pre-fork rejection works. | |
| } | |
| Add test fixtures that construct a CBlockIndex with V24 deployment active to exercise the acceptance paths in IsValidStructure() and IsValidInOuts(). |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths lack unit test coverage
The test at line 382 confirms pre-V24 rejection of unbalanced entries but explicitly notes (line 384) that post-V24 acceptance requires a CBlockIndex with V24 active, which is not set up. The post-V24 code paths — where promotion and demotion entries are actually accepted and validated — are not exercised in unit tests. This means the core new functionality is only covered by the standalone Validate*Entry tests (which are dead code in production, per finding above).
| // Post-V24: Promotion/demotion session state | ||
| bool m_fPromotion{false}; // True if this session is promoting smaller -> larger denom | ||
| bool m_fDemotion{false}; // True if this session is demoting larger -> smaller denom | ||
| std::vector<COutPoint> m_vecPromotionInputs; // Selected inputs for promotion (10 coins) |
There was a problem hiding this comment.
💬 Nitpick: m_vecPromotionInputs is reused for demotion inputs, naming is misleading
m_vecPromotionInputs stores inputs for both promotion (10 coins) and demotion (1 coin), as seen at client.cpp:1290-1292 and 1825-1839. The name and comment ("Selected inputs for promotion (10 coins)") imply promotion-only usage. Consider renaming to m_vecMixingInputs or m_vecSessionInputs.
source: ['claude']
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/coinjoin/client.cpp (3)
1225-1255:⚠️ Potential issue | 🔴 CriticalDelay rebalance locking until the queue join is definitely kept.
Promotion/demotion inputs are locked before the
IsMasternodeOrDisconnectRequested()continuepath and before the successful session state is recorded. If that check trips, the outpoints are neither invecOutPointLockednor inm_vecRebalanceInputs, so they leak out of cleanup entirely.Also applies to: 1266-1269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 1225 - 1255, The code currently locks promotion/demotion inputs (m_wallet->LockCoin calls for vecCoins and vecTxDSInTmp) before the IsMasternodeOrDisconnectRequested() continuation and before recording the session state, causing leaked locked outpoints if the join is aborted; move the LockCoin calls so they occur only after the join is definitively kept (i.e., after the IsMasternodeOrDisconnectRequested() check passes and after you have pushed the outpoints into vecOutPointLocked and/or m_vecRebalanceInputs), and wrap the LockCoin calls with LOCK(m_wallet->cs_wallet) as done elsewhere; apply the same change for the demotion path (the block that currently locks vecTxDSInTmp[0].prevout) and the equivalent locking near the later duplicate section so locks are only taken once the session state/ownership vectors are updated.
296-311:⚠️ Potential issue | 🟠 Major
SetNull()does not actually release rebalance locks.Most cleanup paths already call
UnlockCoins()beforeSetNull()(for example Lines 273, 449, and 728). Appendingm_vecRebalanceInputshere only repopulatesvecOutPointLockedafter the unlock pass, so sessions that fail beforePreparePromotionEntry()orPrepareDemotionEntry()can leave those coins locked indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 296 - 311, SetNull() currently appends m_vecRebalanceInputs to vecOutPointLocked after other UnlockCoins() calls, which re-locks coins; instead ensure rebalance inputs are explicitly unlocked before clearing: call UnlockCoins(m_vecRebalanceInputs) (or add them to vecOutPointLocked before the existing UnlockCoins() pass) and only then clear m_vecRebalanceInputs and reset m_fPromotion/m_fDemotion; reference SetNull(), UnlockCoins(), m_vecRebalanceInputs, vecOutPointLocked, PreparePromotionEntry(), and PrepareDemotionEntry() when making the change.
1431-1456:⚠️ Potential issue | 🔴 Critical
StartNewQueue()has the same pre-selection lock leak.These coins are locked before the masternode retry loop starts, but
m_vecRebalanceInputsis only populated on the success path at Lines 1508-1514. Anycontinueorreturn falsebefore then leaves the rebalance inputs locked with no owner to unlock them.Also applies to: 1467-1514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 1431 - 1456, StartNewQueue() locks demotion inputs (m_wallet->LockCoin(vecTxDSInTmp[0].prevout)) before the masternode retry/selection path completes, but m_vecRebalanceInputs is only populated on the success path, causing locked coins to be leaked on early returns; fix by deferring the LockCoin call until after the masternode selection succeeds and m_vecRebalanceInputs will be set, or immediately add the locked prevout into m_vecRebalanceInputs (so ownership/cleanup is tracked) and ensure any early return paths unlock those entries; update the CCoinJoinClientSession::StartNewQueue logic around vecTxDSInTmp/prevout locking to either move the LOCK(m_wallet->cs_wallet)/LockCoin(...) to after the success branch or push_back the prevout into m_vecRebalanceInputs right when locking so cleanup/unlock is guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/coinjoin/coinjoin.cpp`:
- Around line 227-276: IsValidInOuts() now treats the whole vin/vout as one
EntryType (EntryType enum) and rejects mixed-session final transactions (e.g.
multiple STANDARD entries plus a PROMOTION), causing clients/servers (client.cpp
and server.cpp callers) to fail signing; fix by restoring per-entry validation
or adding a dedicated final-transaction validator: either (A) change the callers
in client.cpp/server.cpp to call the existing per-entry validators
(CoinJoin::ValidatePromotionEntry, CoinJoin::ValidateDemotionEntry, or the
STANDARD path) for each logical entry instead of passing the whole tx into
IsValidInOuts(), or (B) implement a new function (e.g.,
CoinJoin::ValidateFinalTransaction) that parses the combined vin/vout into
constituent entries, validates each entry with the existing
ValidatePromotionEntry/ValidateDemotionEntry logic, and determines expected
denominations using CoinJoin::GetLargerAdjacentDenom per entry; update
IsValidInOuts() to only validate single entries or delegate to the new
ValidateFinalTransaction when given a full-final-tx context so mixed entries are
accepted.
- Around line 103-127: Add an explicit post‑V24 cap on outputs: after computing
nMaxInputs (which uses fV24Active, CoinJoin::GetMaxPoolParticipants() and
CoinJoin::PROMOTION_RATIO), add a check that tx->vout.size() <= nMaxInputs and
return false if it exceeds it; keep the existing denominated P2PKH validation
unchanged so IsValidStructure()/the surrounding logic enforces both input and
output caps.
In `@src/wallet/coinjoin.cpp`:
- Around line 424-431: The loop over setWalletUTXO accesses
it->second.tx->vout[outpoint.n] without verifying outpoint.n is a valid index;
add a guard before any vout indexing in both loops (the one around
mapWallet.find and the one at 454-463) to check that it->second.tx is non-null
and outpoint.n < it->second.tx->vout.size(), and continue if not; keep existing
checks (IsSpent, IsLockedCoin, nValue == nDenomAmount) unchanged but perform
them only after this bounds/null check to avoid out-of-bounds reads when
handling stale or inconsistent outpoints (symbols: setWalletUTXO, mapWallet,
it->second.tx, vout, outpoint.n, IsSpent, IsLockedCoin, nDenomAmount).
---
Duplicate comments:
In `@src/coinjoin/client.cpp`:
- Around line 1225-1255: The code currently locks promotion/demotion inputs
(m_wallet->LockCoin calls for vecCoins and vecTxDSInTmp) before the
IsMasternodeOrDisconnectRequested() continuation and before recording the
session state, causing leaked locked outpoints if the join is aborted; move the
LockCoin calls so they occur only after the join is definitively kept (i.e.,
after the IsMasternodeOrDisconnectRequested() check passes and after you have
pushed the outpoints into vecOutPointLocked and/or m_vecRebalanceInputs), and
wrap the LockCoin calls with LOCK(m_wallet->cs_wallet) as done elsewhere; apply
the same change for the demotion path (the block that currently locks
vecTxDSInTmp[0].prevout) and the equivalent locking near the later duplicate
section so locks are only taken once the session state/ownership vectors are
updated.
- Around line 296-311: SetNull() currently appends m_vecRebalanceInputs to
vecOutPointLocked after other UnlockCoins() calls, which re-locks coins; instead
ensure rebalance inputs are explicitly unlocked before clearing: call
UnlockCoins(m_vecRebalanceInputs) (or add them to vecOutPointLocked before the
existing UnlockCoins() pass) and only then clear m_vecRebalanceInputs and reset
m_fPromotion/m_fDemotion; reference SetNull(), UnlockCoins(),
m_vecRebalanceInputs, vecOutPointLocked, PreparePromotionEntry(), and
PrepareDemotionEntry() when making the change.
- Around line 1431-1456: StartNewQueue() locks demotion inputs
(m_wallet->LockCoin(vecTxDSInTmp[0].prevout)) before the masternode
retry/selection path completes, but m_vecRebalanceInputs is only populated on
the success path, causing locked coins to be leaked on early returns; fix by
deferring the LockCoin call until after the masternode selection succeeds and
m_vecRebalanceInputs will be set, or immediately add the locked prevout into
m_vecRebalanceInputs (so ownership/cleanup is tracked) and ensure any early
return paths unlock those entries; update the
CCoinJoinClientSession::StartNewQueue logic around vecTxDSInTmp/prevout locking
to either move the LOCK(m_wallet->cs_wallet)/LockCoin(...) to after the success
branch or push_back the prevout into m_vecRebalanceInputs right when locking so
cleanup/unlock is guaranteed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ccc108f2-8300-41e8-b074-26afc4c78c13
📒 Files selected for processing (5)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/server.cppsrc/wallet/coinjoin.cpp
| // Pre-V24: require balanced input/output counts (1:1 mixing only) | ||
| // Post-V24: allow unbalanced counts (promotion/demotion) | ||
| if (!fV24Active && tx->vin.size() != tx->vout.size()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (tx->vin.size() < size_t(CoinJoin::GetMinPoolParticipants())) { | ||
| return false; | ||
| } | ||
| if (tx->vin.size() > CoinJoin::GetMaxPoolParticipants() * COINJOIN_ENTRY_MAX_SIZE) { | ||
|
|
||
| // Post-V24: allow up to 200 inputs (20 participants * 10 inputs for promotions) | ||
| // Pre-V24: max 180 inputs (20 participants * 9 entries) | ||
| const size_t nMaxInputs = fV24Active | ||
| ? CoinJoin::GetMaxPoolParticipants() * CoinJoin::PROMOTION_RATIO | ||
| : CoinJoin::GetMaxPoolParticipants() * COINJOIN_ENTRY_MAX_SIZE; | ||
|
|
||
| if (tx->vin.size() > nMaxInputs) { | ||
| return false; | ||
| } | ||
| return std::ranges::all_of(tx->vout, [](const auto& txOut) { | ||
|
|
||
| if (!std::ranges::all_of(tx->vout, [](const auto& txOut) { | ||
| return CoinJoin::IsDenominatedAmount(txOut.nValue) && txOut.scriptPubKey.IsPayToPublicKeyHash(); | ||
| }); | ||
| })) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Restore an explicit post-V24 vout cap here.
Pre-V24 the vin.size() == vout.size() rule bounded outputs implicitly. After this change, any number of denominated P2PKH outputs passes IsValidStructure() as long as the input count is within nMaxInputs, even though valid post-V24 sessions still top out at GetMaxPoolParticipants() * CoinJoin::PROMOTION_RATIO outputs.
🧱 Restore the structural cap
- if (tx->vin.size() > nMaxInputs) {
+ if (tx->vin.size() > nMaxInputs || tx->vout.size() > nMaxInputs) {
return false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/coinjoin.cpp` around lines 103 - 127, Add an explicit post‑V24
cap on outputs: after computing nMaxInputs (which uses fV24Active,
CoinJoin::GetMaxPoolParticipants() and CoinJoin::PROMOTION_RATIO), add a check
that tx->vout.size() <= nMaxInputs and return false if it exceeds it; keep the
existing denominated P2PKH validation unchanged so IsValidStructure()/the
surrounding logic enforces both input and output caps.
| for (const auto& outpoint : setWalletUTXO) { | ||
| const auto it{mapWallet.find(outpoint.hash)}; | ||
| if (it == mapWallet.end()) continue; | ||
|
|
||
| if (IsSpent(outpoint) || IsLockedCoin(outpoint)) continue; | ||
|
|
||
| const CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | ||
| if (nValue != nDenomAmount) continue; |
There was a problem hiding this comment.
Guard outpoint.n before indexing vout.
Both new helpers dereference it->second.tx->vout[outpoint.n] straight from setWalletUTXO. A stale or inconsistent outpoint now turns the background promotion logic into an out-of-bounds read instead of just skipping the entry.
🐛 Minimal guard
const auto it{mapWallet.find(outpoint.hash)};
if (it == mapWallet.end()) continue;
+ if (outpoint.n >= it->second.tx->vout.size()) continue;
if (IsSpent(outpoint) || IsLockedCoin(outpoint)) continue;Apply the same guard in both loops.
Also applies to: 454-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/coinjoin.cpp` around lines 424 - 431, The loop over setWalletUTXO
accesses it->second.tx->vout[outpoint.n] without verifying outpoint.n is a valid
index; add a guard before any vout indexing in both loops (the one around
mapWallet.find and the one at 454-463) to check that it->second.tx is non-null
and outpoint.n < it->second.tx->vout.size(), and continue if not; keep existing
checks (IsSpent, IsLockedCoin, nValue == nDenomAmount) unchanged but perform
them only after this bounds/null check to avoid out-of-bounds reads when
handling stale or inconsistent outpoints (symbols: setWalletUTXO, mapWallet,
it->second.tx, vout, outpoint.n, IsSpent, IsLockedCoin, nDenomAmount).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push addresses 4 of 5 prior review findings cleanly: the blocking coin-lock double-reservation bug is fixed, V24 activation checks now hold cs_main, the dead-code validators are integrated into the production path, and the misleading m_vecPromotionInputs name is corrected to m_vecRebalanceInputs. The new outpoint index bounds checks and IsSpent/IsLockedCoin filters are correct. One design concern: sessions composed entirely of promotion/demotion entries (insufficient standard mixers) will accept all entries then stall until timeout rather than failing fast.
Reviewed commit: 332186c
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 300-307: Sessions with insufficient standard mixers stall until timeout instead of failing fast
Once `GetEntriesCount() == vecSessionCollaterals.size()`, the pool is full — `AddEntry()` rejects further entries and the server is past `POOL_STATE_QUEUE`. If `GetStandardEntriesCount() < GetMinPoolParticipants()` at this point, the session is deterministically unsatisfiable: it will never gain more standard mixers. Lines 300-306 log and wait, then `CheckTimeout()` eventually resets via `SetNull()`. This means all participants (including legitimate standard mixers) wait the full timeout window before the session is recycled.
Consider resetting immediately when the pool is full but the standard-mixer threshold is unachievable, so participants can re-queue without the timeout delay.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths still lack integration test coverage
The incremental diff changes live server behavior (IsValidInOuts now dispatches through the dedicated validators; CheckPool enforces minimum standard mixers) but adds no test updates. The standalone `ValidatePromotionEntry`/`ValidateDemotionEntry` tests are now exercising live code (good), but the full server-side path — `ProcessDSVIN` → `AddEntry` → `IsValidInOuts` → `CheckPool` with V24 active — remains untested. The new insufficient-standard-mixer branch is also uncovered.
| if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n"); | ||
| CreateFinalTransaction(); | ||
| return; | ||
| if (GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n"); | ||
| CreateFinalTransaction(); | ||
| return; | ||
| } | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- all entries received but insufficient standard mixers (%d), waiting for timeout\n", GetStandardEntriesCount()); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Sessions with insufficient standard mixers stall until timeout instead of failing fast
Once GetEntriesCount() == vecSessionCollaterals.size(), the pool is full — AddEntry() rejects further entries and the server is past POOL_STATE_QUEUE. If GetStandardEntriesCount() < GetMinPoolParticipants() at this point, the session is deterministically unsatisfiable: it will never gain more standard mixers. Lines 300-306 log and wait, then CheckTimeout() eventually resets via SetNull(). This means all participants (including legitimate standard mixers) wait the full timeout window before the session is recycled.
Consider resetting immediately when the pool is full but the standard-mixer threshold is unachievable, so participants can re-queue without the timeout delay.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 300-307: Sessions with insufficient standard mixers stall until timeout instead of failing fast
Once `GetEntriesCount() == vecSessionCollaterals.size()`, the pool is full — `AddEntry()` rejects further entries and the server is past `POOL_STATE_QUEUE`. If `GetStandardEntriesCount() < GetMinPoolParticipants()` at this point, the session is deterministically unsatisfiable: it will never gain more standard mixers. Lines 300-306 log and wait, then `CheckTimeout()` eventually resets via `SetNull()`. This means all participants (including legitimate standard mixers) wait the full timeout window before the session is recycled.
Consider resetting immediately when the pool is full but the standard-mixer threshold is unachievable, so participants can re-queue without the timeout delay.
| BOOST_AUTO_TEST_CASE(isvalidstructure_postfork_unbalanced_valid) | ||
| { | ||
| // Post-V24: Unbalanced vin/vout (promotion: 10 inputs, 1 output) should be valid | ||
| // We need a mock pindex that signals V24 active - for this test we use nullptr which means pre-fork | ||
| // This test validates that the structure check correctly identifies promotion structure | ||
|
|
||
| CCoinJoinBroadcastTx promo; | ||
| { | ||
| CMutableTransaction mtx; | ||
| // Promotion: 10 inputs of smaller denom -> 1 output of larger denom | ||
| const int nInputCount = CoinJoin::PROMOTION_RATIO; | ||
| const CAmount nLargerAmount = CoinJoin::DenominationToAmount(1 << 1); // 1.0 DASH | ||
|
|
||
| for (int i = 0; i < nInputCount; ++i) { | ||
| CTxIn in; | ||
| in.prevout = COutPoint(uint256::ONE, static_cast<uint32_t>(i)); | ||
| mtx.vin.push_back(in); | ||
| } | ||
| // 1 output of larger denom | ||
| CTxOut out{nLargerAmount, P2PKHScript(0x01)}; | ||
| mtx.vout.push_back(out); | ||
|
|
||
| promo.tx = MakeTransactionRef(mtx); | ||
| promo.m_protxHash = uint256::ONE; | ||
| } | ||
|
|
||
| // Pre-V24 (nullptr): unbalanced should fail | ||
| BOOST_CHECK(!promo.IsValidStructure(nullptr)); | ||
|
|
||
| // Note: Post-V24 test would require a valid CBlockIndex with V24 deployment active | ||
| // which requires more setup. The above confirms pre-fork rejection works. | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Post-V24 acceptance paths still lack integration test coverage
The incremental diff changes live server behavior (IsValidInOuts now dispatches through the dedicated validators; CheckPool enforces minimum standard mixers) but adds no test updates. The standalone ValidatePromotionEntry/ValidateDemotionEntry tests are now exercising live code (good), but the full server-side path — ProcessDSVIN → AddEntry → IsValidInOuts → CheckPool with V24 active — remains untested. The new insufficient-standard-mixer branch is also uncovered.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths still lack integration test coverage
The incremental diff changes live server behavior (IsValidInOuts now dispatches through the dedicated validators; CheckPool enforces minimum standard mixers) but adds no test updates. The standalone `ValidatePromotionEntry`/`ValidateDemotionEntry` tests are now exercising live code (good), but the full server-side path — `ProcessDSVIN` → `AddEntry` → `IsValidInOuts` → `CheckPool` with V24 active — remains untested. The new insufficient-standard-mixer branch is also uncovered.
|
This pull request has conflicts, please rebase. |
|
✅ Review complete (commit 3e7e959) |
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>
* fix: add IsLockedCoin/IsSpent check in SelectFullyMixedForPromotion Add IsSpent/IsLockedCoin filtering to both CountCoinsByDenomination() and SelectFullyMixedForPromotion() to prevent concurrent CoinJoin sessions from selecting the same UTXOs. This matches the existing filtering pattern used in the denomination selector. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add bounds checking for outpoint.n in coinjoin client Add bounds check for outpoint.n before accessing wtx.tx->vout in JoinExistingQueue and StartNewQueue promotion paths. This prevents potential out-of-bounds access if wallet state changes between UTXO selection and input construction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: acquire cs_main for V24 activation checks in server Wrap m_chainman.ActiveChain().Tip() accesses in LOCK(::cs_main) in ProcessDSVIN and AddEntry to prevent race conditions during V24 activation state checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: enforce standard mixer minimum on full-session finalization The full-session finalization path in CheckPool was creating the final transaction without verifying the minimum standard mixer count. This could allow a session with only promotion/demotion entries (which don't provide privacy) to finalize. Add the same GetStandardEntriesCount() check that the timeout path already uses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: use ValidatePromotionEntry/ValidateDemotionEntry in IsValidInOuts Replace the inline promotion/demotion validation in IsValidInOuts() with calls to the dedicated CoinJoin::ValidatePromotionEntry() and CoinJoin::ValidateDemotionEntry() functions. These functions were already declared, implemented, and tested but never called from production code. This ensures the tested code IS the production code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: rename m_vecPromotionInputs to m_vecRebalanceInputs The member stores inputs for both promotion AND demotion operations, so the name m_vecPromotionInputs was misleading. Rename to m_vecRebalanceInputs to better reflect its dual purpose. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Post-rebase fixups: llmq::CChainLocksHandler was replaced by chainlock::Chainlocks, DEPLOYMENT_V24 became a versionbits deployment so DeploymentActiveAt needs the ChainstateManager, CDeterministicMNList count getters moved to GetCounts(), and the chainlocks handler moved from LLMQContext to NodeContext. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PastaPastaPasta
force-pushed
the
feat/coinjoin-promotion
branch
from
332186c to
f3c87a9
Compare
There was a problem hiding this comment.
Code Review
This push is a rebase-fixup that adapts CoinJoin promotion/demotion to develop's current API surface (Chainlocks handler location, DeploymentActiveAt, GetCounts()). Four of the prior blocking issues (standard-mixer minimum on full-session finalization, cs_main around V24 tip access, IsLockedCoin filtering in SelectFullyMixedForPromotion, and wiring ValidatePromotion/DemotionEntry into the live IsValidInOuts path) are resolved. Three blocking issues remain: (a) SignFinalTransaction validates the aggregate final transaction with a shape-classifier that only recognizes a single entry, so any V24 session containing promotion/demotion entries is rejected client-side; (b) JoinExistingQueue and (c) the StartNewQueue promotion/demotion overload lock coins before the outer retry/masternode-check loops but never register them in m_vecRebalanceInputs on the continue/return paths, leaking wallet locks per failed attempt.
🔴 3 blocking | 🟡 4 suggestion(s)
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.
Findings
-
🔴 blocking: SignFinalTransaction rejects every mixed post-V24 session via aggregate IsValidInOuts
src/coinjoin/client.cpp:501This calls
IsValidInOuts(...finalMutableTransaction.vin, finalMutableTransaction.vout...)on the fully-aggregated final transaction.CCoinJoinBaseSession::IsValidInOuts(coinjoin.cpp:257-266) classifies the entire (vin,vout) tuple as exactly one entry type — STANDARD (vin.size() == vout.size()), PROMOTION (vin.size() == PROMOTION_RATIO && vout.size() == 1), DEMOTION (vin.size() == 1 && vout.size() == PROMOTION_RATIO), otherwise INVALID. OnceCreateFinalTransactionconcatenates entries (server.cpp:326-336), any session containing at least one promotion or demotion entry mixed with standard entries produces counts that match none of the three shapes. Example: 3 standard participants + 1 promotion entry ⇒ 13 vin / 4 vout ⇒ INVALID ⇒ client refuses to sign at STEP 1, and the same rejection fires on every participant. Even if counts happen to balance to STANDARD by coincidence, the per-outputcheckTxOutinvocation compares each output denom tonSessionDenom, so any promotion output at the larger adjacent denom fails ERR_DENOM. Net effect: the entire post-V24 mixed-session code path — the point of this PR — cannot complete, because standard entries are still required byCheckPool(server.cpp:287-292) yet their presence guarantees the aggregate shape is rejected client-side. The client validation for the aggregate needs to iterate per-entry (using each participant's vecTxDSIn / vecTxOut) or otherwise account for combined shapes, rather than applying the single-entry classifier to the concatenated transaction.Source: codex
-
🔴 blocking: JoinExistingQueue leaks wallet coin locks when the masternode check triggers
continuesrc/coinjoin/client.cpp:1110In the promotion branch,
m_wallet->LockCoin(outpoint)runs at lines 1112-1114 for each of the 10 selected inputs; in the demotion branch,LockCoinruns at line 1139 for the single selected input. Both branches then fall through to the masternode gate at line 1151. IfIsMasternodeOrDisconnectRequestedis true, the loopcontinues at line 1154 without ever populatingm_vecRebalanceInputs— that only happens on the success path at lines 1170-1182.SetNull()(lines 182-194) reconcilesm_vecRebalanceInputsintovecOutPointLockedsoUnlockCoins()can drain them, but it has nothing to drain because those outpoints were never recorded. The next iteration ofwhile (GetQueueItemAndTry(dsq))callsSelectFullyMixedForPromotion/SelectTxDSInsByDenominationagain, which now correctly skipIsLockedCoinoutpoints and therefore pick a fresh, disjoint batch — locking and orphaning those as well. When the outer loop finally exits withreturn falseat line 1194, every leaked batch remains locked until the wallet is restarted (or the user manually unlocks). SinceDoAutomaticDenominatingcalls this repeatedly, the wallet can be steadily drained of usable denominated UTXOs. Move theLockCoincalls behind the masternode check (or record each locked outpoint intom_vecRebalanceInputs/vecOutPointLockedimmediately after locking) so the SetNull/UnlockCoins cleanup path handles both success and failure exits.Source: opus, sonnet5, codex
-
🔴 blocking: StartNewQueue promotion/demotion overload strands its coin locks on every retry-exhaustion path
src/coinjoin/client.cpp:1279The promotion branch at lines 1316-1319 and the demotion branch at lines 1338-1340 lock coins once, before the
while (nTries < 10)retry loop at line 1353. Every failure exit —dmn == nullptrreturning at line 1358, or the retry loop exhaustingwinner-skip/threshold-exceeded/disconnect-requestedcontinues and falling through toreturn falseat line 1410 — bypasses lines 1394-1400 wherem_vecRebalanceInputsis populated. Withm_vecRebalanceInputsempty,SetNull()has nothing to move intovecOutPointLocked, soUnlockCoins()never releases the coins locked at 1318/1340. Every failed StartNewQueue attempt therefore strands 10 promotion coins (or 1 demotion coin) permanently for the wallet's lifetime. Combined with the JoinExistingQueue leak above, an automatic-denominating wallet can lose access to a large fraction of its denominated balance under normal masternode churn. Same fix pattern: register outpoints for cleanup at the point of locking, or defer locking until the success path.Source: opus, sonnet5, codex
-
🟡 suggestion: DSVIN pre-check regresses pre-V24 collateral consumption for unbalanced entries
src/coinjoin/server.cpp:214The new early-return in
ProcessDSVINrejects any unbalanced entry pre-V24 withPushStatus(STATUS_REJECTED, ERR_MODE)and returns beforeAddEntryruns. Previously the same unbalanced input would have flowed toAddEntry → IsValidInOuts, which setsfConsumeCollateralRet = truein the pre-V24 branch (coinjoin.cpp:270-272), causingConsumeCollateral(entry.txCollateral)to fire at server.cpp:651. With the pre-check, that anti-abuse path is silently skipped, letting a peer spam unbalanced DSVIN messages without cost. The pre-check is also redundant with the existing IsValidInOuts pre-V24 rejection. Either drop the pre-check and let AddEntry handle it (preserving ConsumeCollateral), or callConsumeCollateral(entry.txCollateral)before returning in this branch.Source: opus
-
🟡 suggestion: Full sessions with too few standard mixers stall until timeout instead of failing fast (carry-forward prior-6)
src/coinjoin/server.cpp:286Once
GetEntriesCount() == vecSessionCollaterals.size()the pool is full andAddEntrywill reject any subsequent entries, so ifGetStandardEntriesCount() < CoinJoin::GetMinPoolParticipants()the session is deterministically unsatisfiable — no additional standard mixer can join. The current code just logs and waits untilCheckTimeoutfires (server.cpp:519) afterCOINJOIN_QUEUE_TIMEOUT, during which every accepted participant sits idle with collateral reserved. Reset the session (or relay a distinct STATUS_REJECTED with a specific PoolMessage) immediately when the full-but-under-threshold branch is detected so clients can requeue without waiting the full timeout window.Source: opus, sonnet5, codex
-
🟡 suggestion: Post-V24 acceptance paths still lack unit coverage (carry-forward prior-7)
src/test/coinjoin_inouts_tests.cpp:426isvalidstructure_postfork_unbalanced_valid(line 426),isvalidstructure_demotion_structure(line 459), and the mixed-entry post-V24 test (line 566+) each still explicitly note that exercising a genuine post-V24 acceptance requires aCBlockIndexwith V24 active, and only assert the pre-V24 nullptr rejection path. The individualValidatePromotionEntry/ValidateDemotionEntryhelpers are covered, but the actual gating inCCoinJoinBroadcastTx::IsValidStructureandCCoinJoinBaseSession::IsValidInOuts— the consensus-observable behavior — has no unit-level positive case. A minimal test harness using-testactivationheight=v24@Nor a mock CBlockIndex positioned past the V24 threshold would let these tests assert both branches. As-is, the very issue in finding #1 (aggregate IsValidInOuts on final tx) and the server-side accepting flow have no automated coverage.Source: opus, sonnet5, codex
-
🟡 suggestion: Fold fixup/review-feedback/post-rebase commits before merge
<commit-history>dashpay/dash merges without squashing, so intermediate fixup commits land in
develophistory. This stack contains several such commits that patch code introduced earlier in the same PR:3d3d5210f6(fix: lint-logs issues, formatting-only for the feature's log statements),1244b53047(fix: address critical issues— a race condition, resource leak, and missing UTXO check on brand-new code),862df05ec0(trivial refactor— no area prefix, no body, cosmetic conditional flattening),b92f57fdc9(fix: address review feedback— bundles six independent sub-fixes: IsLockedCoin filter, bounds check, cs_main lock, minimum-mixer enforcement on the full-session path, wiring the validators, and a member rename), andf3c87a9ad1(fix: adapt … to develop API changes— post-rebase edits that leave earlier commits unable to build against current develop). Squash each fix into the commit that introduced the affected code (or split b92f57f by subsystem and squash accordingly) sogit bisectdoesn't land on the feature commit with a documented race and resource leak,git blamepoints at the feature rather than a lint pass, and each commit in the stack builds cleanly on top of develop.Source: opus, sonnet5, codex
Prior Findings Resolved
prior-1Full-session finalization bypasses minimum standard mixer check: server.cpp:286-293 now gatesCreateFinalTransaction()onGetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()in the full-collaterals branch and defers to timeout otherwise.prior-2V24 activation checks access ActiveChain().Tip() without cs_main:ProcessDSVIN(server.cpp:217-222),AddEntry(server.cpp:613-618), andIsValidInOuts(coinjoin.cpp:244-248) now acquireLOCK(::cs_main)before reading the active-chain tip.prior-3SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation: The selection path in wallet/coinjoin.cpp now short-circuits onIsSpent(outpoint) || IsLockedCoin(outpoint), matching the pattern used by the standard selection helpers.prior-4ValidatePromotionEntry/ValidateDemotionEntry are dead code in production:CCoinJoinBaseSession::IsValidInOuts(coinjoin.cpp:287-299) invokesCoinJoin::ValidatePromotionEntryandCoinJoin::ValidateDemotionEntrydirectly, so the validators are now exercised on the live server AddEntry path.prior-5Missing production wiring / partial coverage of new validators: Superseded by prior-4 for the wiring aspect; the remaining test-coverage aspect is carried forward as prior-7.
| bool CCoinJoinClientSession::StartNewQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman, | ||
| int nTargetDenom, bool fPromotion, bool fDemotion) | ||
| { | ||
| assert(m_mn_metaman.IsValid()); | ||
|
|
||
| if (!CCoinJoinClientOptions::IsEnabled()) return false; | ||
| if (nTargetDenom == 0) return false; | ||
|
|
||
| // For promotion/demotion, verify we have the required coins before starting a queue | ||
| std::vector<CTxDSIn> vecTxDSInTmp; | ||
|
|
||
| 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; | ||
| if (outpoint.n >= wtx.tx->vout.size()) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- invalid outpoint index %u for tx %s\n", outpoint.n, outpoint.hash.ToString()); | ||
| 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::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) { | ||
| // Demotion: need 1 coin of the larger denomination | ||
| const int nLargerDenom = CoinJoin::GetLargerAdjacentDenom(nTargetDenom); | ||
| if (nLargerDenom == 0) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- No larger adjacent denom for demotion\n"); | ||
| return false; | ||
| } | ||
| // Prefer fully-mixed coins first, then fall back to ready-to-mix | ||
| if (!m_wallet->SelectTxDSInsByDenomination(nLargerDenom, CoinJoin::DenominationToAmount(nLargerDenom), vecTxDSInTmp, CoinType::ONLY_FULLY_MIXED)) { | ||
| if (!m_wallet->SelectTxDSInsByDenomination(nLargerDenom, CoinJoin::DenominationToAmount(nLargerDenom), vecTxDSInTmp, CoinType::ONLY_READY_TO_MIX)) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- Couldn't find coin for demotion\n"); | ||
| return false; | ||
| } | ||
| } | ||
| if (vecTxDSInTmp.size() > 1) { | ||
| vecTxDSInTmp.resize(1); | ||
| } | ||
| // Lock demotion input immediately to prevent race conditions | ||
| if (!vecTxDSInTmp.empty()) { | ||
| LOCK(m_wallet->cs_wallet); | ||
| m_wallet->LockCoin(vecTxDSInTmp[0].prevout); | ||
| } | ||
| } else { | ||
| // Neither promotion nor demotion - shouldn't use this overload | ||
| return false; | ||
| } | ||
|
|
||
| int nTries = 0; | ||
| const auto mnList = m_dmnman.GetListAtChainTip(); | ||
| const auto mnCounts = mnList.GetCounts(); | ||
| const int nMnCount = mnCounts.enabled(); | ||
| const int nWeightedMnCount = mnCounts.m_valid_weighted; | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| m_clientman.AddUsedMasternode(dmn->proTxHash); | ||
|
|
||
| // skip next mn payments winners | ||
| if (dmn->pdmnState->nLastPaidHeight + nWeightedMnCount < mnList.GetHeight() + WinnersToSkip()) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- skipping winner, masternode=%s\n", dmn->proTxHash.ToString()); | ||
| nTries++; | ||
| continue; | ||
| } | ||
|
|
||
| if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, nMnCount)) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- too early to mix with node masternode=%s\n", | ||
| dmn->proTxHash.ToString()); | ||
| nTries++; | ||
| continue; | ||
| } | ||
|
|
||
| if (connman.IsMasternodeOrDisconnectRequested(dmn->pdmnState->netInfo->GetPrimary())) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- skipping connection, masternode=%s\n", | ||
| dmn->proTxHash.ToString()); | ||
| nTries++; | ||
| continue; | ||
| } | ||
|
|
||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- attempting %s connection, masternode=%s, tries=%d\n", | ||
| fPromotion ? "PROMOTION" : "DEMOTION", dmn->proTxHash.ToString(), nTries); | ||
|
|
||
| nSessionDenom = nTargetDenom; | ||
| mixingMasternode = dmn; | ||
| connman.AddPendingMasternode(dmn->proTxHash); | ||
| pendingDsaRequest = CPendingDsaRequest(dmn->proTxHash, CCoinJoinAccept(nSessionDenom, txMyCollateral)); | ||
| SetState(POOL_STATE_QUEUE); | ||
| nTimeLastSuccessfulStep = GetTime(); | ||
|
|
||
| // Store promotion/demotion state and inputs | ||
| m_fPromotion = fPromotion; | ||
| m_fDemotion = fDemotion; | ||
| m_vecRebalanceInputs.clear(); | ||
| for (const auto& txdsin : vecTxDSInTmp) { | ||
| m_vecRebalanceInputs.push_back(txdsin.prevout); | ||
| } | ||
|
|
||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::StartNewQueue -- pending %s connection, masternode=%s, nSessionDenom=%d (%s), %zu inputs\n", | ||
| fPromotion ? "PROMOTION" : "DEMOTION", | ||
| dmn->proTxHash.ToString(), nSessionDenom, CoinJoin::DenominationToString(nSessionDenom), | ||
| m_vecRebalanceInputs.size()); | ||
| strAutoDenomResult = _("Trying to connect…"); | ||
| return true; | ||
| } | ||
| strAutoDenomResult = _("Failed to start a new mixing queue"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: StartNewQueue promotion/demotion overload strands its coin locks on every retry-exhaustion path
The promotion branch at lines 1316-1319 and the demotion branch at lines 1338-1340 lock coins once, before the while (nTries < 10) retry loop at line 1353. Every failure exit — dmn == nullptr returning at line 1358, or the retry loop exhausting winner-skip/threshold-exceeded/disconnect-requested continues and falling through to return false at line 1410 — bypasses lines 1394-1400 where m_vecRebalanceInputs is populated. With m_vecRebalanceInputs empty, SetNull() has nothing to move into vecOutPointLocked, so UnlockCoins() never releases the coins locked at 1318/1340. Every failed StartNewQueue attempt therefore strands 10 promotion coins (or 1 demotion coin) permanently for the wallet's lifetime. Combined with the JoinExistingQueue leak above, an automatic-denominating wallet can lose access to a large fraction of its denominated balance under normal masternode churn. Same fix pattern: register outpoints for cleanup at the point of locking, or defer locking until the success path.
source: ['opus', 'sonnet5', 'codex']
There was a problem hiding this comment.
Resolved in 2be0623 — StartNewQueue promotion/demotion overload strands its coin locks on every retry-exhaustion path no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Post-V24: Check if unbalanced entries (promotion/demotion) are allowed | ||
| if (entry.vecTxDSIn.size() != entry.vecTxOut.size()) { | ||
| // This is a promotion or demotion entry - requires V24 activation | ||
| bool fV24Active{false}; | ||
| { | ||
| LOCK(::cs_main); | ||
| const CBlockIndex* pindex = m_chainman.ActiveChain().Tip(); | ||
| fV24Active = pindex && DeploymentActiveAt(*pindex, m_chainman, Consensus::DEPLOYMENT_V24); | ||
| } | ||
| if (!fV24Active) { | ||
| LogPrint(BCLog::COINJOIN, "DSVIN -- promotion/demotion entry rejected: V24 not active\n"); | ||
| PushStatus(peer, STATUS_REJECTED, ERR_MODE); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: DSVIN pre-check regresses pre-V24 collateral consumption for unbalanced entries
The new early-return in ProcessDSVIN rejects any unbalanced entry pre-V24 with PushStatus(STATUS_REJECTED, ERR_MODE) and returns before AddEntry runs. Previously the same unbalanced input would have flowed to AddEntry → IsValidInOuts, which sets fConsumeCollateralRet = true in the pre-V24 branch (coinjoin.cpp:270-272), causing ConsumeCollateral(entry.txCollateral) to fire at server.cpp:651. With the pre-check, that anti-abuse path is silently skipped, letting a peer spam unbalanced DSVIN messages without cost. The pre-check is also redundant with the existing IsValidInOuts pre-V24 rejection. Either drop the pre-check and let AddEntry handle it (preserving ConsumeCollateral), or call ConsumeCollateral(entry.txCollateral) before returning in this branch.
source: ['opus']
There was a problem hiding this comment.
Correction: this finding remains valid at 3e7e9591. The current review carries it forward as Pre-V24 DSVIN pre-check bypasses ConsumeCollateral anti-spam path. The earlier auto-resolved wording was caused by reconciliation matching the wrong prior state; no thread resolution was applied.
- unlock promotion/demotion inputs when a pending masternode connection expires or the masternode can't be found, instead of leaving them permanently locked - replace repeated per-denomination wallet scans in ShouldPromote/ShouldDemote with a single CWallet::GetDenominationCounts() scan shared across all four adjacent-pair checks per DoAutomaticDenominating pass - consolidate the duplicated V24-activation check into CoinJoin::IsPromotionDemotionActive(), used at all call sites in coinjoin.cpp/server.cpp/client.cpp - make DenominationToAmount reuse GetDenominationIndex instead of re-scanning vecStandardDenominations with its own loop Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- validate the aggregated final transaction with new aggregate rules (allowed denominations, value balance, promotion/demotion input/output consistency) instead of per-entry shape classification, which rejected every final tx containing a promotion/demotion entry and exposed honest participants to collateral charges - consolidate promotion/demotion input selection into SelectRebalanceInputs(), record locked outpoints in vecOutPointLocked at lock time and unlock on all JoinExistingQueue/StartNewQueue failure paths, fixing coin-lock leaks and the UnlockCoins-before-SetNull ordering bug that left rebalance inputs locked until restart - check promotion/demotion opportunities before the 'nothing to do' / 'not enough funds' early exits so rebalancing works for wallets that reached their anonymization target (the feature's primary use case) - cap promotion/demotion entries per session on the server so the frozen participant set can always reach the standard-mixer minimum, and reset a stuck session immediately instead of stalling everyone until timeout - drop unbalanced DSTXes without a Misbehaving penalty when V24 is not active at the local tip, so honest relayers are not punished around the activation boundary - implement CDSTXManager::IsTxExpired via CCoinJoinBroadcastTx::IsExpired so the unit test covers the production expiry rule Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit exempted every unbalanced DSTX from the Misbehaving penalty while V24 is inactive at the local tip, which also forgave structurally malformed transactions and weakened the anti-spam pairing with the recent-rejects exemption for DSTX messages. IsValidStructure now reports whether a failing tx would be structurally valid under post-V24 rules (activation-independent checks still enforced), and ValidateDSTX applies a small score-1 penalty (same rationale as UNKNOWN_MASTERNODE) only in that case, keeping the full penalty for garbage. Adds unit coverage for the new flag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This push resolves the three blocking issues from the f3c87a9 review (aggregate FINAL_TX validation for SignFinalTransaction, coin-lock cleanup in JoinExistingQueue/StartNewQueue via SelectRebalanceInputs, and the full-session-under-threshold stall). The DSTX fork-boundary penalty narrowing in 3e7e959 is well-scoped and unit-covered. Two prior suggestions remain valid: the pre-V24 DSVIN pre-check silently drops ConsumeCollateral, and the new FINAL_TX and post-V24 server acceptance paths still have no direct unit coverage. One new suggestion: PreparePromotionEntry/PrepareDemotionEntry failures in SubmitDenominate leave rebalance inputs locked until CheckTimeout expires the session.
🟡 5 suggestion(s)
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history.
Findings
-
🟡 suggestion: Pre-V24 DSVIN pre-check bypasses ConsumeCollateral anti-spam path
src/coinjoin/server.cpp:213-218ProcessDSVIN rejects unbalanced entries pre-V24 with
PushStatus(STATUS_REJECTED, ERR_MODE)and returns before AddEntry runs. Without this pre-check, the same unbalanced entry would flow to AddEntry -> IsValidInOuts, which setsfConsumeCollateralRet = truein the pre-V24 branch (coinjoin.cpp:275-281) and causesConsumeCollateral(entry.txCollateral)to fire at server.cpp:655-657. With the pre-check, that anti-abuse cost is silently skipped, letting a peer that reached POOL_STATE_ACCEPTING_ENTRIES spam malformed DSVIN messages pre-V24 without paying collateral. The pre-check is also redundant with the pre-V24 size-mismatch rejection in IsValidInOuts. Either drop the pre-check and let AddEntry handle it (preserving ConsumeCollateral), or callConsumeCollateral(entry.txCollateral)before returning here.Source: claude, codex
-
🟡 suggestion: New FINAL_TX aggregate validation and post-V24 server acceptance paths lack unit coverage
src/coinjoin/coinjoin.cpp:394-409The FINAL_TX branch of IsValidInOuts (coinjoin.cpp:263-266, 394-408) is the fix for the previously-blocking SignFinalTransaction rejection bug — the core correctness fix for this PR — but no test in
src/test/coinjoin_inouts_tests.cppcalls IsValidInOuts withfFinalTx=true. The aggregate arithmetic here is subtle (relies on value-conservation plus two one-sided inequalities againstnLargerInputs * PROMOTION_RATIO/nLargerOutputs * PROMOTION_RATIOto guarantee composability from valid entries), so it warrants direct coverage. Similarly,isvalidstructure_postfork_unbalanced_validandisvalidstructure_demotion_structureexplicitly note (line 458) that post-V24 activation is not exercised, leaving the CheckPool insufficient-standard-mixer reset (server.cpp:285-287) and the AddEntry non-standard-slot cap (server.cpp:621-631) untested. Add at minimum a unit-level test that constructs (or stubs) a V24-active CBlockIndex to drive the FINAL_TX branch and the acceptance flow.Source: claude, codex
-
🟡 suggestion: Promotion/demotion SubmitDenominate failures leave rebalance inputs locked until timeout
src/coinjoin/client.cpp:1468-1486SelectRebalanceInputs() (called from StartNewQueue/JoinExistingQueue) locks promotion/demotion coins and records them in
vecOutPointLocked/m_vecRebalanceInputsbefore the session is queued. IfPreparePromotionEntry()orPrepareDemotionEntry()then fails in SubmitDenominate (lines 1473-1475, 1483-1485), the function only returns false — it does not callUnlockRebalanceInputs(),UnlockCoins(), or reset the session. The callerTrySubmitDenominate(line 1434) returns true regardless, so the session sits inPOOL_STATE_QUEUEwith the rebalance inputs held untilCheckTimeout()eventually fires afterCOINJOIN_QUEUE_TIMEOUT + 10sand unwinds viaUnlockCoins() → SetNull(). The standard-mixing path in the same function doesn't lock coins ahead of Prepare, so this is a promotion/demotion-specific delay. Consider callingUnlockCoins(); WITH_LOCK(cs_coinjoin, SetNull());(matching the pattern used elsewhere in this file) on the Prepare failure branches so the user can requeue immediately.Source: codex
-
🟡 suggestion: 3e7e959 explicitly walks back f6e3cb4 within the same push — squash the pair
<commit:3e7e9591>:13e7e959's commit message states: 'The previous commit exempted every unbalanced DSTX from the Misbehaving penalty ... which also forgave structurally malformed transactions.' The 'previous commit' is f6e3cb4 in this same push, so this is a same-author, same-push correction to a change that never shipped or received external review in its intermediate form. Since dashpay/dash merges without squashing, leaving these two commits means
git blameon ValidateDSTX's fork-boundary exemption lands on the intermediate f6e3cb4 (which forgave structurally malformed transactions) rather than on the final intended behavior, and a bisect on any future DSTX-scoring regression would flag f6e3cb4 as introducing a bug that was actually fixed two commits later in the same push. Squash 3e7e959 into f6e3cb4 so the DSTX exemption lands in develop in its final form as a single commit.Source: claude, codex
-
🟡 suggestion: Same-push initial-feature bugfixes should be squashed into 313f643
<commit:1244b53047>:1The two commits immediately following the base feature commit 313f643 (
feat: coinjoin promotion / demotion) are same-push corrections that never received external review in their broken state: 3d3d521 (fix: lint-logs issues) only reformats log statements just added by the feature commit, and 1244b53 (fix: address critical issues in CoinJoin promotion/demotion) — per its own message — fixes a race condition, coin-lock resource leak, and missing UTXO checks in functions the feature commit introduced. None of the three external review rounds captured in this PR's history reviewed 313f643 in isolation, so these are pure self-corrections. Left unsquashed,git blameon JoinExistingQueue's coin-locking logic lands on 313f643 (which contains the race and the leak) rather than the corrected version, and a bisect run would flag the broken intermediate as the introducing commit. Squash 3d3d521 and 1244b53 into 313f643 before merge. The review-round commits following (b92f56fd, 2be0623, f6e3cb4) legitimately map to external review cycles and can stay if you prefer to preserve that history.Source: claude, codex
Prior Findings Resolved
-
c2089b6c-1Full-session finalization bypasses minimum standard mixer check: Fixed at server.cpp:277-287 — CheckPool now requires GetStandardEntriesCount() >= GetMinPoolParticipants() on the full-session path and calls SetNull() immediately when the threshold is unreachable instead of stalling until timeout. -
c2089b6c-2SelectFullyMixedForPromotion ignores wallet coin locks: Fixed — SelectRebalanceInputs (client.cpp:1048) now filters via IsSpent/IsLockedCoin and records outpoints in vecOutPointLocked so UnlockCoins() releases them on failure paths. -
c2089b6c-3V24 activation checks access ActiveChain().Tip() without cs_main: Fixed — IsPromotionDemotionActive callers now hold cs_main (verified in ProcessDSVIN, AddEntry, IsValidInOuts). -
c2089b6c-4ValidatePromotionEntry/ValidateDemotionEntry are dead code: Fixed — coinjoin.cpp:299-310 now dispatches to ValidatePromotionEntry/ValidateDemotionEntry from the live IsValidInOuts path. -
332186c2-1Sessions with insufficient standard mixers stall until timeout: Fixed at server.cpp:282-287 — CheckPool now resets the session immediately with SetNull() when the pool fills but standard-mixer minimum is unreachable, and AddEntry (server.cpp:621-631) caps non-standard entries so the state should not be reachable in the first place. -
f3c87a9a-1SignFinalTransaction rejects every mixed post-V24 session via aggregate IsValidInOuts: Fixed — IsValidInOuts now has a FINAL_TX entry type (coinjoin.cpp:263-266) that permits the larger adjacent denomination and validates aggregate composition consistency (lines 394-408) instead of applying the single-entry shape classifier to the concatenated transaction. -
f3c87a9a-2JoinExistingQueue leaks wallet coin locks on masternode-check continue: Fixed — SelectRebalanceInputs (client.cpp:1048, called at 1137) locks and records rebalance inputs into vecOutPointLocked up front, and UnlockRebalanceInputs is called on every failure exit (client.cpp:1211). -
f3c87a9a-3StartNewQueue promotion/demotion overload strands coin locks on retry exhaustion: Fixed — StartNewQueue now calls SelectRebalanceInputs once up front (client.cpp:1312) and UnlockRebalanceInputs on both the dmn==nullptr and the retry-exhaustion exits (client.cpp:1327, 1377).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 213-218: Pre-V24 DSVIN pre-check bypasses ConsumeCollateral anti-spam path
ProcessDSVIN rejects unbalanced entries pre-V24 with `PushStatus(STATUS_REJECTED, ERR_MODE)` and returns before AddEntry runs. Without this pre-check, the same unbalanced entry would flow to AddEntry -> IsValidInOuts, which sets `fConsumeCollateralRet = true` in the pre-V24 branch (coinjoin.cpp:275-281) and causes `ConsumeCollateral(entry.txCollateral)` to fire at server.cpp:655-657. With the pre-check, that anti-abuse cost is silently skipped, letting a peer that reached POOL_STATE_ACCEPTING_ENTRIES spam malformed DSVIN messages pre-V24 without paying collateral. The pre-check is also redundant with the pre-V24 size-mismatch rejection in IsValidInOuts. Either drop the pre-check and let AddEntry handle it (preserving ConsumeCollateral), or call `ConsumeCollateral(entry.txCollateral)` before returning here.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 394-409: New FINAL_TX aggregate validation and post-V24 server acceptance paths lack unit coverage
The FINAL_TX branch of IsValidInOuts (coinjoin.cpp:263-266, 394-408) is the fix for the previously-blocking SignFinalTransaction rejection bug — the core correctness fix for this PR — but no test in `src/test/coinjoin_inouts_tests.cpp` calls IsValidInOuts with `fFinalTx=true`. The aggregate arithmetic here is subtle (relies on value-conservation plus two one-sided inequalities against `nLargerInputs * PROMOTION_RATIO` / `nLargerOutputs * PROMOTION_RATIO` to guarantee composability from valid entries), so it warrants direct coverage. Similarly, `isvalidstructure_postfork_unbalanced_valid` and `isvalidstructure_demotion_structure` explicitly note (line 458) that post-V24 activation is not exercised, leaving the CheckPool insufficient-standard-mixer reset (server.cpp:285-287) and the AddEntry non-standard-slot cap (server.cpp:621-631) untested. Add at minimum a unit-level test that constructs (or stubs) a V24-active CBlockIndex to drive the FINAL_TX branch and the acceptance flow.
In `src/coinjoin/client.cpp`:
- [SUGGESTION] lines 1468-1486: Promotion/demotion SubmitDenominate failures leave rebalance inputs locked until timeout
SelectRebalanceInputs() (called from StartNewQueue/JoinExistingQueue) locks promotion/demotion coins and records them in `vecOutPointLocked`/`m_vecRebalanceInputs` before the session is queued. If `PreparePromotionEntry()` or `PrepareDemotionEntry()` then fails in SubmitDenominate (lines 1473-1475, 1483-1485), the function only returns false — it does not call `UnlockRebalanceInputs()`, `UnlockCoins()`, or reset the session. The caller `TrySubmitDenominate` (line 1434) returns true regardless, so the session sits in `POOL_STATE_QUEUE` with the rebalance inputs held until `CheckTimeout()` eventually fires after `COINJOIN_QUEUE_TIMEOUT + 10s` and unwinds via `UnlockCoins() → SetNull()`. The standard-mixing path in the same function doesn't lock coins ahead of Prepare, so this is a promotion/demotion-specific delay. Consider calling `UnlockCoins(); WITH_LOCK(cs_coinjoin, SetNull());` (matching the pattern used elsewhere in this file) on the Prepare failure branches so the user can requeue immediately.
In `<commit:3e7e9591>`:
- [SUGGESTION] lines 1: 3e7e9591 explicitly walks back f6e3cb49 within the same push — squash the pair
3e7e9591's commit message states: 'The previous commit exempted every unbalanced DSTX from the Misbehaving penalty ... which also forgave structurally malformed transactions.' The 'previous commit' is f6e3cb49 in this same push, so this is a same-author, same-push correction to a change that never shipped or received external review in its intermediate form. Since dashpay/dash merges without squashing, leaving these two commits means `git blame` on ValidateDSTX's fork-boundary exemption lands on the intermediate f6e3cb49 (which forgave structurally malformed transactions) rather than on the final intended behavior, and a bisect on any future DSTX-scoring regression would flag f6e3cb49 as introducing a bug that was actually fixed two commits later in the same push. Squash 3e7e9591 into f6e3cb49 so the DSTX exemption lands in develop in its final form as a single commit.
In `<commit:1244b53047>`:
- [SUGGESTION] lines 1: Same-push initial-feature bugfixes should be squashed into 313f643223
The two commits immediately following the base feature commit 313f643223 (`feat: coinjoin promotion / demotion`) are same-push corrections that never received external review in their broken state: 3d3d5210f6 (`fix: lint-logs issues`) only reformats log statements just added by the feature commit, and 1244b53047 (`fix: address critical issues in CoinJoin promotion/demotion`) — per its own message — fixes a race condition, coin-lock resource leak, and missing UTXO checks in functions the feature commit introduced. None of the three external review rounds captured in this PR's history reviewed 313f643223 in isolation, so these are pure self-corrections. Left unsquashed, `git blame` on JoinExistingQueue's coin-locking logic lands on 313f643223 (which contains the race and the leak) rather than the corrected version, and a bisect run would flag the broken intermediate as the introducing commit. Squash 3d3d5210f6 and 1244b53047 into 313f643223 before merge. The review-round commits following (b92f56fd, 2be0623d, f6e3cb49) legitimately map to external review cycles and can stay if you prefer to preserve that history.
| if (entryType == EntryType::FINAL_TX) { | ||
| // Aggregate consistency: every larger-denom output consumes PROMOTION_RATIO session-denom | ||
| // inputs (promotion) and every larger-denom input produces PROMOTION_RATIO session-denom | ||
| // outputs (demotion); the remainder on both sides are standard 1:1 inputs/outputs and | ||
| // must not be negative. Together with the zero-fee check above this guarantees the final | ||
| // tx is composable from valid standard/promotion/demotion entries. | ||
| const size_t nSessionInputs = vin.size() - nLargerInputs; | ||
| const size_t nSessionOutputs = vout.size() - nLargerOutputs; | ||
| if (nSessionInputs < nLargerOutputs * size_t(CoinJoin::PROMOTION_RATIO) || | ||
| nSessionOutputs < nLargerInputs * size_t(CoinJoin::PROMOTION_RATIO)) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinBaseSession::%s -- ERROR: inconsistent final tx composition! %d/%d session denom, %d/%d larger denom inputs/outputs\n", | ||
| __func__, nSessionInputs, nSessionOutputs, nLargerInputs, nLargerOutputs); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: New FINAL_TX aggregate validation and post-V24 server acceptance paths lack unit coverage
The FINAL_TX branch of IsValidInOuts (coinjoin.cpp:263-266, 394-408) is the fix for the previously-blocking SignFinalTransaction rejection bug — the core correctness fix for this PR — but no test in src/test/coinjoin_inouts_tests.cpp calls IsValidInOuts with fFinalTx=true. The aggregate arithmetic here is subtle (relies on value-conservation plus two one-sided inequalities against nLargerInputs * PROMOTION_RATIO / nLargerOutputs * PROMOTION_RATIO to guarantee composability from valid entries), so it warrants direct coverage. Similarly, isvalidstructure_postfork_unbalanced_valid and isvalidstructure_demotion_structure explicitly note (line 458) that post-V24 activation is not exercised, leaving the CheckPool insufficient-standard-mixer reset (server.cpp:285-287) and the AddEntry non-standard-slot cap (server.cpp:621-631) untested. Add at minimum a unit-level test that constructs (or stubs) a V24-active CBlockIndex to drive the FINAL_TX branch and the acceptance flow.
source: ['claude', 'codex']
| if (m_fPromotion) { | ||
| if (PreparePromotionEntry(strError, vecPSInOutPairsTmp)) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Promotion entry prepared, sending\n"); | ||
| return SendDenominate(vecPSInOutPairsTmp, connman); | ||
| } | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- PreparePromotionEntry failed: %s\n", strError); | ||
| strAutoDenomResult = Untranslated(strError); | ||
| return false; | ||
| } | ||
|
|
||
| if (m_fDemotion) { | ||
| if (PrepareDemotionEntry(strError, vecPSInOutPairsTmp)) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Demotion entry prepared, sending\n"); | ||
| return SendDenominate(vecPSInOutPairsTmp, connman); | ||
| } | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- PrepareDemotionEntry failed: %s\n", strError); | ||
| strAutoDenomResult = Untranslated(strError); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Promotion/demotion SubmitDenominate failures leave rebalance inputs locked until timeout
SelectRebalanceInputs() (called from StartNewQueue/JoinExistingQueue) locks promotion/demotion coins and records them in vecOutPointLocked/m_vecRebalanceInputs before the session is queued. If PreparePromotionEntry() or PrepareDemotionEntry() then fails in SubmitDenominate (lines 1473-1475, 1483-1485), the function only returns false — it does not call UnlockRebalanceInputs(), UnlockCoins(), or reset the session. The caller TrySubmitDenominate (line 1434) returns true regardless, so the session sits in POOL_STATE_QUEUE with the rebalance inputs held until CheckTimeout() eventually fires after COINJOIN_QUEUE_TIMEOUT + 10s and unwinds via UnlockCoins() → SetNull(). The standard-mixing path in the same function doesn't lock coins ahead of Prepare, so this is a promotion/demotion-specific delay. Consider calling UnlockCoins(); WITH_LOCK(cs_coinjoin, SetNull()); (matching the pattern used elsewhere in this file) on the Prepare failure branches so the user can requeue immediately.
source: ['codex']
3 participants
Summary
This PR adds CoinJoin denomination promotion and demotion so participants can convert between adjacent standard denominations inside a mixing session instead of being limited to strict 1:1 denomination mixes.
The new behavior is gated by V24 activation. Pre-V24 behavior remains unchanged.
What was done?
How Has This Been Tested?
src/test/coinjoin_inouts_tests.cppdevelopgit diff --checkpasses after conflict resolutionPost-V24 activation behavior still needs functional coverage because EHF activation paths cannot be fully exercised in these unit tests alone.
Breaking Changes
None. The feature is activation-gated and preserves existing pre-V24 behavior.