feat: coinjoin promotion / demotion - #7052
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Post-V24 CoinJoin promotion and demotion between adjacent denominations. The client selects and locks specialized inputs, joins target-denomination queues, and prepares rebalance entries. Chain-aware validation, DSTX handling, server pool rules, wallet helpers, protocol negotiation, tests, and release notes support the new entry shapes. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client as CCoinJoinClientManager
participant Session as CCoinJoinClientSession
participant Wallet as CWallet
participant Server as CCoinJoinServer
participant Network as ValidateDSTX
Client->>Client: DoAutomaticDenominating()
Client->>Session: JoinExistingQueue() or StartNewQueue()
Session->>Wallet: Select and lock rebalance inputs
Session->>Server: SendDenominate()
Server->>Server: Validate entry and update pool
Network->>Server: Validate DSTX structure with chain context
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)
b70ccef to
d140732
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/coinjoin/client.cpp (1)
1789-1800: Optional: Simplify promotion output pairing for clarity.The current approach creates 10 input-output pairs with empty outputs (line 1793), then replaces the last pair's output (line 1800). While correct, this is less clear than creating the pairs with the appropriate output from the start.
🔎 Alternative approach
- // For promotion, outputs are created but only 1 matters (the larger denom) - // We'll use empty CTxOut for all but the first to signal "no output for this input" - // Actually, for promotion entry: 10 inputs, 1 output - // We need to pair each input with an "empty" output, except the last gets the real output - vecPSInOutPairsRet.emplace_back(txdsin, CTxOut()); -} - -// Now set the single output (larger denomination) on the last entry -CScript scriptDenom = keyHolderStorage.AddKey(m_wallet.get()); -if (!vecPSInOutPairsRet.empty()) { - // Replace the last output with the actual promotion output - vecPSInOutPairsRet.back().second = CTxOut(nLargerAmount, scriptDenom); + // For promotion: 10 inputs, 1 output (on last pair) + if (i == static_cast<int>(m_vecPromotionInputs.size()) - 1) { + // Last input gets the real promotion output + CScript scriptDenom = keyHolderStorage.AddKey(m_wallet.get()); + vecPSInOutPairsRet.emplace_back(txdsin, CTxOut(nLargerAmount, scriptDenom)); + } else { + // Other inputs get empty outputs + vecPSInOutPairsRet.emplace_back(txdsin, CTxOut()); + } + }This makes the intent clearer without changing behavior.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between ffb2e13 and d140732ef87897c32385398fa7dca34fa4784091.
📒 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.
|
This pull request has conflicts, please rebase. |
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
📥 Commits
Reviewing files that changed from the base of the PR and between d140732ef87897c32385398fa7dca34fa4784091 and bc4b034.
📒 Files selected for processing (6)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/server.cpp
| [[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
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. |
|
⛔ Blockers found — Sonnet deferred (commit 992e764) |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/coinjoin/client.cpp`:
- Around line 1449-1468: Update the failure branches in SubmitDenominate for
PreparePromotionEntry and PrepareDemotionEntry to release rebalance resources
before returning: call UnlockCoins(), keyHolderStorage.ReturnAll(), and
SetNull() using the same cleanup order and session state handling as the
established failure path. Preserve the existing error logging and
strAutoDenomResult assignment, and ensure both preparation-failure branches
leave the session out of the queue state immediately.
- Around line 1032-1089: Hold m_wallet->cs_wallet once across the entire
SelectRebalanceInputs selection-and-locking flow, including calls to
SelectFullyMixedForPromotion and SelectTxDSInsByDenomination, rather than
locking only the promotion conversion and final LockCoin loop. Keep the existing
validation and selection behavior unchanged, and ensure every selected input is
locked and recorded in m_vecRebalanceInputs while the outer wallet lock remains
held.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e2451b3-ebd1-432a-97b0-b12672ca51b6
📒 Files selected for processing (11)
doc/release-notes-7052.mdsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/test/coinjoin_inouts_tests.cppsrc/wallet/coinjoin.cppsrc/wallet/wallet.h
🚧 Files skipped from review as they are similar to previous changes (10)
- doc/release-notes-7052.md
- src/coinjoin/client.h
- src/coinjoin/common.h
- src/coinjoin/server.cpp
- src/net_processing.cpp
- src/wallet/coinjoin.cpp
- src/coinjoin/coinjoin.cpp
- src/test/coinjoin_inouts_tests.cpp
- src/wallet/wallet.h
- src/coinjoin/coinjoin.h
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
All seven carried-forward findings remain valid at exact head 8a93d93: four CoinJoin defects and one non-buildable intermediate-history issue are blocking, while two commit-organization issues remain suggestions. No new delta findings were discovered, so changes are still required.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 2 suggestion(s)
3 additional finding(s) omitted (not in diff).
4 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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 `<commit:ff5b0a4>`:
- [BLOCKING] <commit:ff5b0a4>:1: Fold the build-restoring API adaptation into the feature
The feature commit `4594bf45329`, whose parent is the exact PR base `2e52d33fa3b`, introduces `llmq::CChainLocksHandler`, the obsolete consensus-parameter call form for the versionbits `DEPLOYMENT_V24`, and removed `GetValidMNsCount()`/`GetValidWeightedMNsCount()` methods. The base instead provides `chainlock::Chainlocks`, requires `ChainstateManager` or a versionbits cache for V24, and exposes deterministic-masternode counts through `GetCounts()`. These obsolete references remain through `a3b21ffc824`; `ff5b0a421b9` repairs them only as commit 9 of 12. Fold the API adaptations into the commits that introduced the affected code so every retained commit builds, as claimed in the PR description.
In `<commit:f9bcf7e>`:
- [SUGGESTION] <commit:f9bcf7e>:1: Squash the initial lint and self-correction commits
The stack still retains `f9bcf7ecb6f`, `43aead97474`, and `1a80e93511b` as separate commits immediately following the feature they correct. They respectively clean logging introduced by `4594bf45329`, repair race, resource-lifecycle, and UTXO-validation defects in that implementation, and apply a trivial conditional refactor. Fold them into the feature commit so blame and bisect do not preserve lint cleanup or known-defective intermediate implementations.
In `<commit:a3b21ff>`:
- [SUGGESTION] <commit:a3b21ff>:1: Fold the review-fix chain into its originating changes
The exact PR range still contains 12 commits despite the current PR description claiming three atomic commits with all self-corrections folded. Commits `a3b21ffc824`, `9197036a087`, and `c8404127b1f` are broad review-fix commits for behavior introduced earlier in this PR, while `8a93d9379bb` explicitly corrects the immediately preceding commit's overly broad DSTX exemption. Fold these corrections into their originating feature and test changes, retaining the coherent feature, test, and release-note history described by the PR.
8a93d93 to
a44e4e9
Compare
|
Force-pushed to restore lost history and re-address the outstanding findings. What happened: the July 10 cleanup pass (commit This push rebases the July 10 head onto current
Verified: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/coinjoin/common.h (1)
161-184: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider aligning the parameter order of the two decision helpers.
ShouldPromoteDenomstakes the smaller-denomination count first.ShouldDemoteDenomstakes the larger-denomination count first. Both takeintparameters, so a swapped call compiles and silently inverts the decision. A consistent order, or a small struct/named arguments, removes that risk.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coinjoin/common.h` around lines 161 - 184, Align the parameter order of ShouldDemoteDenoms with ShouldPromoteDenoms by accepting the smaller-denomination count before the larger-denomination count, and update its internal references and all call sites accordingly. Preserve the existing demotion decision behavior while making accidental argument swaps distinguishable by position.src/test/coinjoin_inouts_tests.cpp (1)
886-925: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test re-implements the production classification logic.
detectEntryTypeis a local copy of the branch structure inCCoinJoinBaseSession::IsValidInOuts. The test therefore validates the copy, not the production code, and it will keep passing if the production branches change. Exercise the production path instead, or drop the case becauseis_standard_mixing_entryand theValidate*Entrytests already cover the shapes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/coinjoin_inouts_tests.cpp` around lines 886 - 925, Remove the local detectEntryType lambda and its assertions from entry_type_detection_logic, since it duplicates CCoinJoinBaseSession::IsValidInOuts. Either delete this redundant test case or replace it with inputs that exercise the production IsValidInOuts path directly, reusing the existing is_standard_mixing_entry and Validate*Entry coverage where appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/coinjoin/common.h`:
- Around line 161-184: Align the parameter order of ShouldDemoteDenoms with
ShouldPromoteDenoms by accepting the smaller-denomination count before the
larger-denomination count, and update its internal references and all call sites
accordingly. Preserve the existing demotion decision behavior while making
accidental argument swaps distinguishable by position.
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 886-925: Remove the local detectEntryType lambda and its
assertions from entry_type_detection_logic, since it duplicates
CCoinJoinBaseSession::IsValidInOuts. Either delete this redundant test case or
replace it with inputs that exercise the production IsValidInOuts path directly,
reusing the existing is_standard_mixing_entry and Validate*Entry coverage where
appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7750bc0-f0ad-4729-890f-dfcfcb4f43b6
📒 Files selected for processing (11)
doc/release-notes-7052.mdsrc/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/server.cppsrc/net_processing.cppsrc/test/coinjoin_inouts_tests.cppsrc/wallet/coinjoin.cppsrc/wallet/wallet.h
🚧 Files skipped from review as they are similar to previous changes (6)
- doc/release-notes-7052.md
- src/net_processing.cpp
- src/coinjoin/client.h
- src/wallet/wallet.h
- src/coinjoin/coinjoin.cpp
- src/coinjoin/client.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/coinjoin_inouts_tests.cpp (1)
1406-1416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
rebalance_session_slot_capdoes not exercise the reservation logic it claims to test.
nStandardAtFullreduces algebraically toCoinJoin::GetMinPoolParticipants()(Max - (Max - Min) = Min), so the checknStandardAtFull >= CoinJoin::GetMinPoolParticipants()is always true, independent of the actual configured values. The test never callsCreateNewSession,AddUserToExistingSession, orIsSessionReady, so it gives no regression protection for the reservation-cap logic added insrc/coinjoin/server.cpp.Exercise the real enforcement path instead, for example by constructing a
CCoinJoinAcceptwithFLAG_REBALANCEand drivingAddUserToExistingSessionpast the cap to confirm it returnsERR_QUEUE_FULLoncem_setRebalanceCollaterals.size()reachesMax - Min.
[recommended_refactor]As per coding guidelines, "For Dash-specific review hotspots, prefer small tests that prove the invariant being changed."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/coinjoin_inouts_tests.cpp` around lines 1406 - 1416, Replace the algebraic checks in rebalance_session_slot_cap with a behavioral test of the reservation cap using CCoinJoinAccept with FLAG_REBALANCE. Create a session and drive AddUserToExistingSession beyond MaxPoolParticipants minus GetMinPoolParticipants, asserting ERR_QUEUE_FULL once the rebalance collateral set reaches that limit; cover the relevant session-readiness path as needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/coinjoin/client.cpp`:
- Around line 550-569: Update the V24 cover validation in CCoinJoinClientSession
so promotion/demotion entries cannot satisfy the cover requirement by
themselves. Count foreign inputs contributed by standard mixing entries only, or
otherwise enforce that at least one standard mixer is present, while preserving
the existing minimum participant thresholds and refusal path.
---
Nitpick comments:
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 1406-1416: Replace the algebraic checks in
rebalance_session_slot_cap with a behavioral test of the reservation cap using
CCoinJoinAccept with FLAG_REBALANCE. Create a session and drive
AddUserToExistingSession beyond MaxPoolParticipants minus
GetMinPoolParticipants, asserting ERR_QUEUE_FULL once the rebalance collateral
set reaches that limit; cover the relevant session-readiness path as needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 488386b1-ad58-4260-9a0c-2dd7dd54749d
📒 Files selected for processing (9)
doc/release-notes-7052.mdsrc/coinjoin/client.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/server.cppsrc/coinjoin/server.hsrc/test/coinjoin_inouts_tests.cppsrc/test/net_tests.cppsrc/version.htest/functional/test_framework/p2p.py
2092ff6 to
1dade96
Compare
3129c7e to
a3f00f0
Compare
AddUserToExistingSession never checked whether the incoming dsa.txCollateral was already accepted into the current session. A client (or an attacker replaying a client's dsa) could therefore be added to vecSessionCollaterals multiple times and be counted as multiple participants, inflating the count IsSessionReady compares against the min/max pool participant thresholds. A session could then look ready with fewer real participants than required; each phantom slot expects a DSVIN entry that never arrives, so such sessions stall until timeout and ChargeFees may consume the replayed collateral. Comparing collateral txids is not sufficient: collaterals are only ever test-accepted, never added to the mempool, so a malicious client can re-sign the same collateral UTXO with a varied change output or signature and produce arbitrarily many distinct txids for what is economically the same collateral, bypassing any hash comparison. Instead, maintain a set mirroring the input prevouts of every accepted session collateral - populated when a collateral enters the session in CreateNewSession or AddUserToExistingSession, cleared alongside vecSessionCollaterals in SetNull - and reject a dsa whose collateral spends any prevout in it; identical transactions necessarily share prevouts, so this subsumes the txid check. Honest participants can never collide on a prevout: distinct wallets cannot spend the same UTXO, and a wallet mixing in multiple sessions locks its collateral inputs per session. Rejected with ERR_ALREADY_HAVE, an existing pool message, so no protocol change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add post-V24 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: - promotion combines PROMOTION_RATIO (10) fully-mixed inputs of one denomination into 1 output of the next larger denomination - demotion splits 1 input into PROMOTION_RATIO outputs of the next smaller denomination Client changes: - ShouldPromote()/ShouldDemote() decide when to rebalance based on per-denomination deficits vs -coinjoindenomsgoal, with a gap threshold to prevent oscillation; promotion additionally requires PROMOTION_RATIO fully-mixed coins so mixed coins are never wasted - DoAutomaticDenominating() checks rebalance opportunities even after the anonymization target is reached and drives promotion/demotion queues via new JoinExistingQueue()/StartNewQueue() overloads - SelectRebalanceInputs() selects and locks rebalance inputs up front, recording them in vecOutPointLocked so UnlockCoins() releases them on every failure path (queue-join failure, connect failure, prepare failure, session reset) - PreparePromotionEntry()/PrepareDemotionEntry() build the unbalanced entries submitted through SendDenominate() Server changes: - AddEntry() accepts up to PROMOTION_RATIO inputs post-V24 and caps non-standard entries so a session can always reach the GetMinPoolParticipants() standard-mixer minimum - CheckPool() only counts standard 1:1 entries toward the privacy threshold and resets immediately when a full session can no longer satisfy it, instead of stalling everyone until timeout - pre-V24, unbalanced entries keep flowing to AddEntry -> IsValidInOuts so their collateral is consumed as before Validation changes: - IsValidInOuts() classifies entries (standard / promotion / demotion) and dispatches to the ValidatePromotionEntry()/ValidateDemotionEntry() helpers; with fFinalTx=true it validates the aggregated final transaction via ValidateFinalTxComposition() plus the zero-fee check since per-entry shape rules don't apply to the concatenation - DSTX IsValidStructure() is now deployment-aware: pre-V24 it keeps the balanced 1:1 rules, post-V24 it allows unbalanced counts with independent input and output caps of GetMaxPoolParticipants() * PROMOTION_RATIO - ValidateDSTX() drops a DSTX that is only valid under post-V24 rules with a reduced penalty around the activation boundary so honest relayers whose tip is ahead of ours aren't discouraged, while structurally malformed transactions keep the full penalty Wallet changes: - GetDenominationCounts() tallies total and fully-mixed coins per denomination in a single scan, CountCoinsByDenomination() and SelectFullyMixedForPromotion() back the rebalance decision and input selection, and SelectTxDSInsByDenomination() gains a CoinType parameter so demotion can prefer fully-mixed coins All new behavior is gated on DEPLOYMENT_V24 activation; pre-V24 behavior is unchanged.
Extend coinjoin_inouts_tests with coverage for the post-V24 promotion/demotion feature: - ValidatePromotionEntry()/ValidateDemotionEntry() happy paths, wrong input/output counts, non-adjacent denominations and edge cases - deployment-aware IsValidStructure(): pre-V24 rejection of unbalanced transactions, the fPossiblyValidPostV24 fork-boundary reporting, and the independent post-V24 input/output caps - ValidateFinalTxComposition() aggregate arithmetic for mixed standard/promotion/demotion final transactions - denomination adjacency helpers, value preservation across all adjacent pairs (10 * smaller == larger), entry-type detection and standard-entry counting - ShouldPromote()/ShouldDemote() decision logic incl. mutual exclusivity and gap-threshold hysteresis - DSTX expiry height/chainlock logic via the extracted IsExpired() Exercising a genuinely V24-active CBlockIndex needs EHF activation machinery that unit tests can't set up; those acceptance paths are covered indirectly through the extracted helpers and still need functional-test coverage.
Bump PROTOCOL_VERSION to 70241 and fix each mixing session's rebalance capability at creation time from the creator's negotiated protocol version. The dsa message gains a version-gated flags field declaring which direction a participant will mix; peers below COINJOIN_REBALANCE_VERSION never see the new field, so their wire format is unchanged and DSQ relay is untouched. Without the version gate, a wallet running older software could be mixed into a session containing an unbalanced promotion/demotion entry post-V24 activation. It would fail IsValidInOuts on the final transaction, refuse to sign and risk having its collateral consumed by ChargeFees despite behaving honestly. Old clients are now rejected from rebalance-capable sessions at dsa time with ERR_VERSION - a message id they understand, received before any collateral is committed - and keep mixing normally in sessions without such entries. Mixing only conceals a participant when somebody else holds coins of the same size on the same side, so a session may finalize only when each side of the session denomination is occupied by nobody or by at least two participants; exactly one is the only forbidden count. Standard entries occupy both sides, promotions only the input side and demotions only the output side, and nothing is required at the larger adjacent denomination because a lone coin there has no group to hide. This replaces the earlier minimum-standard-entries proxy, which both rejected sessions that are perfectly private (two promoters and two demoters with no 1:1 mixer) and admitted ones that are not. Clients apply the same rule to the final transaction before signing, counting only coins at the session denomination. Counting foreign coins regardless of denomination would let a malicious masternode pad a transaction with larger-denomination coins that conceal nothing and still publish one linking a promotion's ten inputs, or a demotion's ten outputs, in plain sight. Declaring the direction up front also lets the masternode judge session coverage before taking collateral, so no participant is admitted and then refused at entry submission. Entries that occupy neither side, including empty ones, are now rejected outright rather than counting as standard mixers and diluting the coverage every participant relies on. Pre-V24 behavior is unchanged, including collateral consumption for unbalanced DSVIN spam. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lared direction Session admission decides whether both sides of the session denomination are covered based on the shapes participants declare in their dsa, but AddEntry only checked promotion/demotion entries against the declaration. A participant declaring a promotion and then submitting a valid standard entry could strip a side of its declared cover and force a fee-free session reset for everyone, repeatedly and at no cost. Every entry must now match its declared direction exactly; deviating consumes the collateral.
…e final tx Version gating keeps pre-70241 software out of rebalance sessions, but a 70241 client whose tip is one block behind the masternode at the V24 activation boundary would still validate the final transaction under pre-V24 rules, refuse to sign it and be charged its collateral. Final-tx validation (and the client-side cover check) now also accepts V24 activating in the block following our tip, so a masternode one block ahead cannot cost an honest client its collateral. Per-entry validation on the masternode side keeps using the strict tip state.
Older clients keep mixing in sessions created by older peers, and newer clients join those sessions for standard mixing; the previous wording suggested admission depended on a session's actual entries.
…them Pre-70241 software treats a promotion/demotion DSTX as structurally invalid, drops it and penalizes the relaying peer by 10 - enough repeated rebalance mixes would get honest relayers (including the originating masternode) discouraged by old peers, and the zero-fee transaction would never enter old mempools anyway. Both inventory drain sites now skip announcing an unbalanced DSTX to peers below COINJOIN_REBALANCE_VERSION; such peers see the transaction on block inclusion. Balanced DSTXes keep relaying to everyone so standard mixes retain their zero-fee propagation.
…ctor SelectFullyMixedForPromotion hand-rolled its scan over setWalletUTXO: deterministic outpoint order, no spendability filter, and no per-transaction dedup. The last one matters most - a demotion leaves 10 sibling outputs of the target denomination in a single transaction, and the sorted scan would later promote all 10 together, making them identifiable as one group in the final tx despite the cover coins. Select through AvailableCoinsListUnspent with ONLY_FULLY_MIXED like SelectTxDSInsByDenomination does, shuffled and capped at one coin per parent transaction.
CCoinJoinAccept::operator== is used via CPendingDsaRequest::operator== and the flags field is part of the message's identity.
a3f00f0 to
e585712
Compare
A conversion's 10:1 shape publicly clusters one participant's coins even inside a mixing transaction: an observer of the final tx knows ten of the session-denomination inputs (or outputs) belong to a single wallet. Since promotion inputs are fully mixed and the promoted output previously inherited their rounds, that cluster exited mixing permanently linked and never dispersed. Instead, treat converted coins as unmixed: GetRealOutpointCoinJoinRounds returns 0 rounds for an output whose own inputs in the same tx are at a different denomination, so promoted and demoted coins re-enter mixing at their new denomination and disperse normally. The rule is inert pre-V24 (standard mixing always matches denominations and CreateDenominated already hits the reset paths) and needs no activation gating. In exchange, both directions now require fully-mixed inputs: the demotion selector's ready-to-mix fallback is removed and ShouldDemoteDenoms gains a fully-mixed gate mirroring promotion, so only coins with protected histories are worth converting.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Six of the seven indexed prior findings are fixed in the rewritten stack; the carried-forward commit-history suggestion remains because several follow-up commits directly repair behavior introduced earlier in this same PR. Two new blocking issues remain: DSVIN entries are not bound one-to-one to admitted collaterals, and the reduced pre-activation DSTX penalty is applied throughout the entire pre-V24 period rather than only at the activation boundary.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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] src/coinjoin/server.cpp:629-632: Entries are not bound one-to-one to accepted collaterals
`AddEntry()` uses `vecSessionCollaterals.size()` only as an entry-count ceiling. It never requires `entry.txCollateral` to match an accepted session collateral or rejects a collateral already represented in `vecEntries`; the post-V24 declaration lookup at lines 690-692 even defaults an unrecognized collateral to `STANDARD`. Once the ready queue is public, any peer with a valid collateral can therefore submit a standard DSVIN without DSA admission, and an admitted participant can submit multiple entries under the same collateral using disjoint inputs. Those entries can fill all accepted slots and finalize without an admitted participant, while that omitted participant's collateral remains eligible for `ChargeRandomFees()`. This also defeats the PR's declared-shape participant accounting. Require every DSVIN collateral to match exactly one accepted session collateral and permit exactly one entry for it.
In `src/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:3558-3560: Premature DSTX tolerance applies far before V24 activation
`IsValidStructure()` marks any transaction that fails the pre-V24 count rules but fits the post-V24 caps as `fPossiblyValidPostV24`, regardless of whether V24 can activate in the next block. `ValidateDSTX()` then returns `PREMATURE` before masternode lookup and signature verification. As a result, throughout the entire pre-V24 period a peer can send structurally post-V24 DSTXes with arbitrary masternode identities or signatures and receive a misbehavior score of 1 instead of the normal 10. Restrict this tip-skew tolerance to a local tip for which V24 is active in the following block.
In `<commit:f363dde>`:
- [SUGGESTION] <commit:f363dde>:1: Fold the review-fix chain into its originating changes
The rewritten history removes the old generic review-fix commits, but it still retains direct same-PR corrections. Commit `f363dde5798` fixes the declared-direction behavior introduced by `93eaee83de1`; `789a2794d6e` replaces the promotion selector introduced by `259069c2094` because it omitted spendability filtering, shuffling, and parent-transaction deduplication; `e585712faba` adds the new `nFlags` field from `93eaee83de1` to equality; and `5fe7893f786` corrects wording introduced by `93eaee83de1`. Fold `789a2794d6e` into `259069c2094`, and fold `f363dde5798`, `e585712faba`, and `5fe7893f786` into `93eaee83de1`, so retained commits do not contain known-defective or misleading intermediate states. The independently motivated activation-tip-skew, relay-compatibility, and rounds-reset commits can remain separate.
| @@ -611,10 +632,15 @@ bool CCoinJoinServer::AddEntry(const CCoinJoinEntry& entry, PoolMessage& nMessag | |||
| return false; | |||
There was a problem hiding this comment.
🔴 Blocking: Entries are not bound one-to-one to accepted collaterals
AddEntry() uses vecSessionCollaterals.size() only as an entry-count ceiling. It never requires entry.txCollateral to match an accepted session collateral or rejects a collateral already represented in vecEntries; the post-V24 declaration lookup at lines 690-692 even defaults an unrecognized collateral to STANDARD. Once the ready queue is public, any peer with a valid collateral can therefore submit a standard DSVIN without DSA admission, and an admitted participant can submit multiple entries under the same collateral using disjoint inputs. Those entries can fill all accepted slots and finalize without an admitted participant, while that omitted participant's collateral remains eligible for ChargeRandomFees(). This also defeats the PR's declared-shape participant accounting. Require every DSVIN collateral to match exactly one accepted session collateral and permit exactly one entry for it.
source: ['codex']
| if (fPossiblyValidPostV24) { | ||
| return {DSTXValidationScore::PREMATURE, true}; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Premature DSTX tolerance applies far before V24 activation
IsValidStructure() marks any transaction that fails the pre-V24 count rules but fits the post-V24 caps as fPossiblyValidPostV24, regardless of whether V24 can activate in the next block. ValidateDSTX() then returns PREMATURE before masternode lookup and signature verification. As a result, throughout the entire pre-V24 period a peer can send structurally post-V24 DSTXes with arbitrary masternode identities or signatures and receive a misbehavior score of 1 instead of the normal 10. Restrict this tip-skew tolerance to a local tip for which V24 is active in the following block.
| if (fPossiblyValidPostV24) { | |
| return {DSTXValidationScore::PREMATURE, true}; | |
| } | |
| if (fPossiblyValidPostV24 && | |
| CoinJoin::IsPromotionDemotionActive(chainman, /*fNextBlock=*/true)) { | |
| return {DSTXValidationScore::PREMATURE, true}; | |
| } |
source: ['codex']
3 participants
Depends on #7507 — the duplicate-collateral fix this branch is rebased on.
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?
AddEntry→IsValidInOutsso their collateral is consumed as before (anti-spam behavior preserved)GetRealOutpointCoinJoinRounds(0 rounds for an output whose own inputs in the same tx are at a different denomination); the rule is inert pre-V24 and needs no activation gatingCoinJoin::ValidateFinalTxComposition()and covered it directly in unit testsProtocol-version gating of rebalance sessions
PROTOCOL_VERSIONto 70241 and addedCOINJOIN_REBALANCE_VERSION; thedsamessage gained a flags field declaring promotion/demotion intent, serialized only between peers that both negotiated ≥ 70241, so the wire format toward older peers is byte-identical and DSQ messages are untoucheddsatime withERR_VERSION(a message ID old releases already understand, delivered before any collateral is committed) — this prevents pre-70241 wallets from ever facing an unbalanced final transaction they cannot validate, which they would refuse to sign at the risk of losing their collateral toChargeFees. Old clients keep mixing in sessions created by old peers, which new clients still join for standard mixingdsa, andIsSessionReadyholds the session in queue until the declared shapes cover both sides of the session denomination; a session that never attracts the missing counterparty times out fee-free in queue state. Because admission relies on the declarations, every entry must match its participant's declared direction exactly — a deviating entry (e.g. declaring a promotion, then submitting a standard entry, which could strip a side of its declared cover and force a fee-free reset for everyone) has its collateral consumedHistory
The branch is rebased onto current
develop. Commits: feature, tests, release notes, and protocol-version gating from earlier review rounds, plus follow-ups from review: declared-direction enforcement with collateral consumption, activation-boundary tip-skew tolerance, release-note clarifications, version-gated announcement of unbalanced DSTXes, promotion input selection aligned with the standard selector (spendable-only, shuffled, at most one coin per parent transaction — so a demotion's 10 sibling outputs are never promoted together as an identifiable group),nFlagsinCCoinJoinAcceptequality, and the rounds reset for conversion outputs described above.How Has This Been Tested?
./src/test/test_dash --run_test=coinjoin_inouts_tests(including coverage for version-gateddsaserialization, mix-shape classification, and the side-coverage invariant),--run_test=coinjoin_tests, and--run_test=net_testspasscoinjoin_rebalance_rounds_reset_testsin the wallet suite exercisesGetRealOutpointCoinJoinRoundsdirectly: standard 1:1 mixing advances rounds, promotion/demotion-shaped transactions reset their outputs to 0, and a promoted coin advances normally when re-mixeddashdandtest_dashbuild cleanlyP2P_VERSIONin the functional-test framework is bumped in lockstep withPROTOCOL_VERSION, so functional tests can exercise the new gating; an old client is now cleanly emulatable with a 70240-advertisingP2PInterfacePost-V24 activation behavior (including the server-side admission/entry-enforcement paths) 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. The protocol version bump to 70241 is backward compatible: older peers keep the previous
dsawire format and are only excluded from sessions that could contain entries they cannot validate.