feat: coinjoin promotion / demotion#7052
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
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, 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
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/net_processing.cpp (1)
3516-3526: Tip-aware DSTX structure validation looks correct, minor clarity tweak possibleUsing
chainman.ActiveChain().Tip()undercs_mainand passingpindexintodstx.IsValidStructure(pindex)is the right direction for fork‑/deployment‑aware DSTX checks and matches existing patterns in this file that rely on a non‑null active tip.Two small nits you may consider for clarity (not blockers):
- Use a separate local (e.g.
const CBlockIndex* pindex_for_validation = pindex;) before the laterforloops that walkpindexback 24 blocks, so it’s obvious that the structure check uses the current tip and is independent from the masternode lookup iteration.- Optionally add a brief comment above the
LOCK(cs_main)explaining that we intentionally snapshot the current tip for tip‑dependent CoinJoin structure rules.Otherwise this change looks consistent with the new CoinJoin promotion/demotion and V24‑aware validation flow.
src/wallet/coinjoin.cpp (1)
443-471: Consider early termination placement for clarity.The early termination check at line 453 occurs after the lock is acquired but before any significant work. While functionally correct, this could be slightly more efficient if moved before the wallet lookup, though the impact is negligible.
The function correctly:
- Guards on enabled state and valid denomination
- Filters for confirmed depth (
< 1)- Requires fully mixed status for promotion candidates
Minor readability improvement - the early break is fine, but consider:
LOCK(cs_wallet); for (const auto& outpoint : setWalletUTXO) { - if (static_cast<int>(vecRet.size()) >= nCount) break; + if (vecRet.size() >= static_cast<size_t>(nCount)) break;This avoids a sign conversion and is slightly more idiomatic.
src/coinjoin/coinjoin.cpp (1)
289-313: Minor: Consider extracting checkTxOut to reduce closure complexity.The
checkTxOutlambda captures multiple variables and handles denomination validation, script checks, and duplicate detection. While functional, extracting this as a private member function could improve testability.The logic is correct:
- Validates denomination against expected
- Ensures P2PKH script type
- Prevents duplicate scriptPubKeys (privacy requirement)
src/coinjoin/client.cpp (1)
1375-1494: Consider extracting common masternode selection logic.The masternode selection loop (lines 1435-1491) is nearly identical to the standard
StartNewQueue(lines 1316-1372). The differences are:
- Input validation at the start
- Denomination is fixed rather than selected from
setAmounts- Promotion/demotion state is stored
This duplication could lead to maintenance issues if the masternode selection logic needs updates.
Consider extracting the common masternode selection and connection logic into a private helper method that both
StartNewQueueoverloads can call, passing in the denomination and a callback for post-connection setup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/coinjoin/client.cpp(12 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/coinjoin.cpp(6 hunks)src/coinjoin/coinjoin.h(2 hunks)src/coinjoin/common.h(1 hunks)src/coinjoin/server.cpp(3 hunks)src/net_processing.cpp(1 hunks)src/test/coinjoin_inouts_tests.cpp(4 hunks)src/wallet/coinjoin.cpp(3 hunks)src/wallet/wallet.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,h,hpp,cc}
📄 CodeRabbit inference engine (CLAUDE.md)
Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1
Files:
src/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/coinjoin.hsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/common.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cppsrc/wallet/coinjoin.cpp
src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cpp
src/coinjoin/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cpp
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures
Files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.hsrc/coinjoin/common.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cpp
src/{test,wallet/test}/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Files:
src/test/coinjoin_inouts_tests.cpp
src/wallet/**/*.{cpp,h}
📄 CodeRabbit inference engine (CLAUDE.md)
Wallet implementation must use Berkeley DB and SQLite
Files:
src/wallet/wallet.hsrc/wallet/coinjoin.cpp
🧠 Learnings (21)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/coinjoin/**/*.{cpp,h} : CoinJoin implementation must use masternode-coordinated mixing sessions with uniform denomination outputs
Applied to files:
src/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/coinjoin.hsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/common.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.hsrc/coinjoin/client.cppsrc/wallet/coinjoin.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/coinjoin/server.cppsrc/net_processing.cppsrc/coinjoin/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq}/**/*.{cpp,h} : BLS integration must be used for cryptographic foundation of advanced masternode features
Applied to files:
src/coinjoin/server.cppsrc/wallet/wallet.hsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/node/chainstate.{cpp,h} : Chainstate initialization must be separated into dedicated src/node/chainstate.* files
Applied to files:
src/coinjoin/server.cppsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data
Applied to files:
src/coinjoin/server.cppsrc/wallet/wallet.hsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,txmempool}/**/*.{cpp,h} : Block validation and mempool handling must use extensions to Bitcoin Core mechanisms for special transaction validation and enhanced transaction relay
Applied to files:
src/net_processing.cppsrc/coinjoin/coinjoin.hsrc/test/coinjoin_inouts_tests.cppsrc/coinjoin/common.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cppsrc/wallet/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{validation,consensus,net_processing}/**/*.{cpp,h} : ValidationInterface callbacks must be used for event-driven architecture to coordinate subsystems during block/transaction processing
Applied to files:
src/net_processing.cppsrc/coinjoin/coinjoin.hsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/client.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/net_processing.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/coinjoin/coinjoin.h
📚 Learning: 2025-05-05T12:45:44.781Z
Learnt from: knst
Repo: dashpay/dash PR: 6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
Applied to files:
src/coinjoin/coinjoin.h
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{test,wallet/test}/**/*.{cpp,h} : Unit tests in src/test/ and src/wallet/test/ must use Boost::Test framework
Applied to files:
src/test/coinjoin_inouts_tests.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/test/coinjoin_inouts_tests.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/test/coinjoin_inouts_tests.cppsrc/wallet/wallet.hsrc/coinjoin/coinjoin.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/wallet/**/*.{cpp,h} : Wallet implementation must use Berkeley DB and SQLite
Applied to files:
src/wallet/wallet.hsrc/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-25T10:53:37.523Z
Learnt from: knst
Repo: dashpay/dash PR: 7008
File: src/masternode/sync.h:17-18
Timestamp: 2025-11-25T10:53:37.523Z
Learning: The file src/masternode/sync.h containing `CMasternodeSync` is misnamed and misplaced—it has nothing to do with "masternode" functionality. It should eventually be renamed to `NodeSyncing` or `NodeSyncStatus` and moved to src/node/sync.h as a future refactoring.
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-11-24T16:41:22.457Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/**/*.{cpp,h} : Special transactions use payload serialization routines defined in src/evo/specialtx.h and must include appropriate special transaction types (ProRegTx, ProUpServTx, ProUpRegTx, ProUpRevTx)
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-02-14T15:19:17.218Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/coinjoin/client.cpp
📚 Learning: 2025-09-02T07:34:28.226Z
Learnt from: knst
Repo: dashpay/dash PR: 6834
File: test/functional/wallet_mnemonicbits.py:50-51
Timestamp: 2025-09-02T07:34:28.226Z
Learning: CJ (CoinJoin) descriptors with derivation path "9'/1" are intentionally inactive in descriptor wallets, while regular internal/external descriptors with different derivation paths remain active.
Applied to files:
src/wallet/coinjoin.cpp
🧬 Code graph analysis (6)
src/coinjoin/coinjoin.h (1)
src/coinjoin/coinjoin.cpp (6)
IsValidStructure(93-136)IsValidStructure(93-93)ValidatePromotionEntry(584-633)ValidatePromotionEntry(584-585)ValidateDemotionEntry(635-675)ValidateDemotionEntry(635-636)
src/wallet/wallet.h (1)
src/wallet/coinjoin.cpp (8)
SelectTxDSInsByDenomination(48-51)SelectTxDSInsByDenomination(48-48)SelectTxDSInsByDenomination(53-93)SelectTxDSInsByDenomination(53-53)CountCoinsByDenomination(414-441)CountCoinsByDenomination(414-414)SelectFullyMixedForPromotion(443-471)SelectFullyMixedForPromotion(443-443)
src/coinjoin/coinjoin.cpp (1)
src/coinjoin/common.h (3)
IsDenominatedAmount(101-101)GetLargerAdjacentDenom(146-151)AmountToDenomination(53-61)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (10)
JoinExistingQueue(1149-1292)JoinExistingQueue(1149-1150)PreparePromotionEntry(1725-1784)PreparePromotionEntry(1725-1725)PrepareDemotionEntry(1786-1845)PrepareDemotionEntry(1786-1786)ShouldPromote(2316-2349)ShouldPromote(2316-2316)ShouldDemote(2351-2378)ShouldDemote(2351-2351)
src/coinjoin/client.cpp (1)
src/coinjoin/common.h (3)
GetLargerAdjacentDenom(146-151)DenominationToAmount(69-184)AreAdjacentDenominations(135-141)
src/wallet/coinjoin.cpp (2)
src/coinjoin/common.h (1)
DenominationToAmount(69-184)src/wallet/wallet.cpp (2)
GetTxDepthInMainChain(3561-3571)GetTxDepthInMainChain(3561-3561)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: win64-build / Build source
🔇 Additional comments (46)
src/wallet/wallet.h (3)
28-28: LGTM: Include addition is appropriate.The inclusion of
wallet/coincontrol.his necessary to support the newSelectTxDSInsByDenominationoverload at line 547, which usesCoinTypeas a parameter. The existing forward declaration ofCCoinControlat line 124 is insufficient for the enum type.
547-547: LGTM: Overload supports denomination-based selection with coin type filtering.The new overload extends the existing
SelectTxDSInsByDenominationto accept aCoinTypeparameter, enabling more granular control over input selection for promotion/demotion workflows.
565-580: LGTM: Well-documented wallet APIs for promotion/demotion support.The new methods provide essential functionality for the post-V24 promotion/demotion feature:
CountCoinsByDenominationcounts coins by denomination with optional fully-mixed filteringSelectFullyMixedForPromotionselects fully-mixed coins suitable for promotionBoth methods are correctly marked
constand have clear documentation.src/coinjoin/common.h (5)
112-114: LGTM: Clear promotion/demotion constants.The constants are well-defined:
PROMOTION_RATIO = 10reflects the 10:1 ratio between adjacent denominationsGAP_THRESHOLD = 10defines the deficit threshold for triggering promotion/demotion
120-129: LGTM: Correct denomination index lookup.The function correctly maps a bitshifted denomination to its index in
vecStandardDenominations, returning -1 for invalid denominations. The logic and bounds checking are sound.
135-141: LGTM: Correct adjacency check for denominations.The function correctly validates that two denominations are adjacent in the standard denomination list, handling invalid inputs appropriately. This is essential for promotion/demotion validation.
146-151: LGTM: Correct larger adjacent denomination lookup.The function correctly navigates to the larger adjacent denomination, returning 0 when the input is already the largest or invalid. The bitshift calculation is accurate.
156-161: LGTM: Correct smaller adjacent denomination lookup.The function correctly navigates to the smaller adjacent denomination, returning 0 when the input is already the smallest or invalid. The bounds checking and bitshift calculation are accurate.
src/coinjoin/server.cpp (4)
7-9: LGTM: Necessary includes for V24 deployment checks.The includes are required:
chainparams.hfor accessing consensus parameters viaParams()deploymentstatus.hfor theDeploymentActiveAtfunction used in V24 activation checks
228-238: LGTM: Correct V24 gate for promotion/demotion entries.The deployment gate correctly rejects unbalanced (promotion/demotion) entries when V24 is not active. The logic appropriately:
- Detects unbalanced entries by comparing input/output counts
- Checks V24 activation status at the current chain tip
- Rejects with
ERR_MODEwhen the feature is not yet active
607-618: LGTM: Dynamic max inputs based on V24 activation.The code correctly adjusts the maximum entry input count based on V24 activation:
- Pre-V24: limits to
COINJOIN_ENTRY_MAX_SIZE(9)- Post-V24: allows up to
PROMOTION_RATIO(10) for promotion entriesThe logging and error handling are appropriate. This change works in conjunction with the ProcessDSVIN gate to properly support promotion/demotion entries.
228-238: Note: V24 activation checks are performed separately.The code checks V24 activation in both
ProcessDSVIN(line 232) andAddEntry(line 610). While these are called sequentially and the risk is minimal, be aware that the chain tip could theoretically advance between checks. In practice, this is acceptable as it would only result in entry rejection, which is safe.This is an informational note for awareness; verification via testing would confirm the behavior is correct under block transitions.
Also applies to: 607-618
src/coinjoin/coinjoin.h (2)
284-284: LGTM: Signature update for deployment-aware validation.The updated
IsValidStructure(const CBlockIndex* pindex)signature correctly enables V24 deployment detection for promotion/demotion validation. Usingnullptrto indicate pre-fork behavior is a reasonable pattern.
367-388: LGTM: Well-documented validation helper declarations.The new
ValidatePromotionEntryandValidateDemotionEntrydeclarations are properly documented with clear parameter semantics. The API correctly identifies the session denomination role (smaller denom for both cases) and returns validation status via the out-parameter.src/wallet/coinjoin.cpp (3)
48-51: LGTM: Clean delegation for backward compatibility.The existing 3-parameter overload now delegates to the new 4-parameter version with
CoinType::ONLY_READY_TO_MIXas default. This preserves existing behavior while enabling the new coin type flexibility.
53-68: LGTM: New overload with configurable coin type.The new overload correctly accepts
CoinTypeand passes it toCCoinControl. The enhanced logging at line 68 is helpful for debugging coin selection with different coin types.
414-441: LGTM: CountCoinsByDenomination implementation is correct.The function properly:
- Guards on
CCoinJoinClientOptions::IsEnabled()- Validates denomination via
DenominationToAmount- Uses depth check
< 1to skip unconfirmed/conflicted (consistent with line 462)- Correctly handles the
fFullyMixedOnlyfiltersrc/coinjoin/client.h (4)
146-150: LGTM: Promotion/demotion session state additions.The new session state members are properly initialized:
m_fPromotion{false}andm_fDemotion{false}with brace initializersm_vecPromotionInputsas an empty vectorThese flags enable session-level tracking of the entry type being prepared.
176-183: LGTM: Entry preparation methods with proper lock annotations.Both
PreparePromotionEntryandPrepareDemotionEntrycorrectly requirem_wallet->cs_walletlock, consistent withPrepareDenominateabove. The separation of promotion (10→1) and demotion (1→10) preparation is clean.
335-350: LGTM: Manager-level decision helpers with clear documentation.The
ShouldPromoteandShouldDemotemethods are properly documented with parameter semantics. These provide the decision logic for when to trigger promotion/demotion based on denomination counts and goals.
164-168: Verify SetNull() clears new state members.The new parameters to
JoinExistingQueueandStartNewQueuecorrectly extend the API with defaults for backward compatibility. EnsureSetNull()in the session class properly resetsm_fPromotion,m_fDemotion, andm_vecPromotionInputsbefore merging.src/test/coinjoin_inouts_tests.cpp (7)
52-86: LGTM: Updated tests for pindex-aware IsValidStructure.The tests correctly pass
nullptrtoIsValidStructure()to test pre-V24 behavior, matching the updated signature. The test cases cover:
- Valid structure
- Invalid identifiers
- Size mismatch (pre-V24 rejection)
- Invalid scripts and amounts
Good coverage of pre-fork validation paths.
137-149: LGTM: Clean test helper functions.
MakeDenomInputandMakeDenomOutputprovide reusable helpers for constructing test inputs/outputs. The helpers correctly useP2PKHScriptfor valid script generation.
151-175: LGTM: Valid promotion entry test.The test correctly validates a promotion entry with:
- 10 inputs (
PROMOTION_RATIO)- 1 output at the larger adjacent denomination
- Proper denomination adjacency (0.1 → 1.0 DASH)
477-500: Critical test for value preservation across denominations.This test validates a key invariant:
nSmaller * PROMOTION_RATIO == nLargerfor all adjacent denomination pairs. This ensures promotion/demotion preserves exact value.
722-756: Test helper mirrors implementation logic correctly.The
TestShouldPromoteandTestShouldDemotehelpers correctly implement the decision algorithm with:
- Half-goal threshold check
- Deficit calculation (
max(0, goal - count))- Gap threshold comparison
This enables testing the algorithm logic without wallet dependencies.
827-861: Thorough mutual exclusivity testing.The
promote_demote_mutually_exclusivetest correctly validates that for any count distribution, at most one of promote/demote can be true. The use of structured bindings (auto [p, d] = ...) is clean C++17 style.
1019-1039: Good documentation of functional test requirements.The comment block properly documents that post-V24 behaviors requiring EHF activation cannot be unit tested and require functional tests. This sets clear expectations for test coverage boundaries.
src/coinjoin/coinjoin.cpp (7)
9-9: LGTM: Required include for deployment status checks.The
deploymentstatus.hinclude is necessary for theDeploymentActiveAtcalls used in V24 activation checks.
93-136: LGTM: IsValidStructure correctly handles V24 deployment.The implementation correctly:
- Uses
DeploymentActiveAtwith pindex for V24 detection- Pre-V24: Requires balanced vin/vout counts
- Post-V24: Allows unbalanced for promotion/demotion
- Dynamically adjusts max inputs (180 pre-fork, 200 post-fork)
- Validates all outputs are denominated and P2PKH
The comment about value sum validation being deferred to
IsValidInOutsis accurate since it requires UTXO access.
221-260: LGTM: Entry type detection with proper V24 gating.The entry type detection logic correctly:
- Identifies STANDARD (balanced) entries for all versions
- Only allows PROMOTION/DEMOTION post-V24
- Properly rejects invalid structures with ERR_SIZE_MISMATCH
The local
EntryTypeenum provides clear semantics.
261-288: LGTM: Denomination expectations set correctly per entry type.For each entry type:
- STANDARD: inputs and outputs at session denom
- PROMOTION: inputs at session denom, output at larger adjacent
- DEMOTION: input at larger adjacent, outputs at session denom
The early failure when
nLargerAdjacentDenom == 0prevents invalid promotion/demotion from the largest/smallest denominations.
351-363: LGTM: Value preservation check applies to all entry types.The
nFees != 0check correctly ensures total input value equals total output value. This is essential for promotion/demotion where 10 × smaller_denom == larger_denom must hold exactly.The logging now includes entry type, which aids debugging.
583-633: LGTM: ValidatePromotionEntry implementation is correct.The function properly validates:
- Exactly
PROMOTION_RATIOinputs- Exactly 1 output
- Larger adjacent denomination exists
- Output matches larger denomination
- Output is P2PKH
Note: Input denomination validation is not performed here as this function validates structure, not UTXO values (done in
IsValidInOuts).
635-675: LGTM: ValidateDemotionEntry correctly validates structure.The function properly validates:
- Exactly 1 input
- Exactly
PROMOTION_RATIOoutputs- All outputs at session denomination
- All outputs are P2PKH
Similar to promotion, input denomination validation is deferred to
IsValidInOutswhere UTXO access is available.src/coinjoin/client.cpp (11)
11-11: LGTM!The new includes for
deploymentstatus.handvalidation.hare appropriate for the V24 deployment detection logic added in this PR.Also applies to: 25-25
295-299: LGTM!Proper cleanup of the new promotion/demotion session state fields, with the lock assertion already in place from the existing code.
979-1035: Clarify target denomination semantics for queues.The V24 detection and iteration logic looks correct. However, I want to verify the target denomination semantics:
- For promotion (lines 1007, 1011):
nSmallerDenomis passed as target - this makes sense as the queue is for the smaller denomination that gets promoted to larger.- For demotion (lines 1026, 1030):
nSmallerDenomis also passed - this means the queue denomination is the output denomination, not the input.This is consistent with the queue matching in
JoinExistingQueueat line 1185 which checksdsq.nDenom != nTargetDenom. The session denomination represents what the mixing session produces, which for demotion is the smaller denomination.The logic is correct but the naming could be clearer. The
nTargetDenomeffectively means "the denomination this session is operating on" which works for both promotion inputs and demotion outputs.
1575-1594: LGTM!Clean separation of promotion/demotion submission paths from standard mixing. The error handling properly logs failures and sets the status message.
1725-1784: LGTM!The promotion entry preparation correctly implements the 10:1 ratio:
- 10 inputs from fully-mixed smaller denomination coins
- 1 output of the larger adjacent denomination
- Empty
CTxOut()placeholders are properly filtered out inSendDenominate- Input locking prevents double-spending during the session
1786-1845: LGTM with a note on denomination semantics.The demotion entry preparation correctly implements the 1:10 ratio:
- 1 input from the larger denomination coin
- 10 outputs of the smaller denomination
- Empty
CTxDSIn()placeholders are properly filtered inSendDenominateThe denomination logic is correct:
nSessionDenomis the target denomination (smaller, for outputs), andGetLargerAdjacentDenom(nSessionDenom)correctly identifies the input denomination for validation.
2316-2349: Well-designed promotion decision logic.The algorithm correctly:
- Validates denomination adjacency
- Protects denominations still being built (< half goal)
- Uses a gap threshold to prevent oscillation
- Requires fully-mixed coins to maintain anonymity guarantees
The hysteresis via
GAP_THRESHOLDis a good design choice to prevent thrashing between promotion and demotion.
2351-2378: LGTM!The demotion decision logic correctly mirrors promotion but without the fully-mixed requirement. This asymmetry is intentional and makes sense:
- Promotion requires fully-mixed coins to preserve anonymity (10 mixed → 1 mixed)
- Demotion can use any denominated coin since splitting doesn't reduce anonymity (1 → 10 new outputs)
The gap threshold hysteresis is shared, preventing oscillation.
1192-1241: Verify wallet lock consistency between promotion and demotion paths.For the promotion path (lines 1192-1213), you explicitly acquire
m_wallet->cs_walletat line 1200 to safely accessmapWallet. However, for the demotion path (lines 1214-1234), you callSelectTxDSInsByDenominationwithout an explicit lock in this scope. Confirm whetherSelectTxDSInsByDenominationacquires the wallet lock internally or if explicit locking is needed here to match the promotion path's pattern.
1576-1594: Resource cleanup on preparation failure follows existing timeout-based pattern.In the promotion/demotion paths (lines 1577–1594), if
PreparePromotionEntryorPrepareDemotionEntryfails, locked coins remain locked until the session times out and callsSetNull() → UnlockCoins(). This matches the pattern used in standard mixing paths. Consider whether immediate cleanup would improve user experience by freeing coins sooner, though the current approach is consistent with the codebase design.
499-510: Remove references to non-existent preparation functions and correct CTxOut behavior.The filtering logic uses
prevout.IsNull()andnValue > 0to distinguish placeholder entries from real transaction data. However, the original review contains factual errors:
- CTxOut's default constructor calls
SetNull(), settingnValueto-1(not0), so the checkif (txOut.nValue > 0)correctly filters out both default (-1) and zero-value outputs.- The referenced functions
PreparePromotionEntryandPrepareDemotionEntrydo not exist in the Dash Core repository and cannot be verified against this filtering logic.The actual filtering appears sound for handling asymmetric input/output counts in promotion/demotion scenarios, but the justification in the original comment is incorrect.
Likely an incorrect or invalid review comment.
Fixes for PR dashpay#7052: Issue #1 (CRITICAL): Fix race condition in coin selection - Lock coins immediately after selection in JoinExistingQueue/StartNewQueue - Prevents concurrent sessions from selecting the same coins - Add defensive IsLockedCoin check in Prepare functions Issue #2 (CRITICAL): Fix resource leak on session failure - Add promotion inputs to vecOutPointLocked in SetNull() - Ensures coins locked early are properly unlocked if session fails - Leverages existing UnlockCoins() retry mechanism Issue #3: Add UTXO validation before use - Check IsSpent() before using promotion/demotion inputs - Prevents using externally spent or transferred coins 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.
| bool CCoinJoinClientManager::ShouldPromote(int nSmallerDenom, int nLargerDenom) const | ||
| { | ||
| // Validate denominations are adjacent | ||
| if (!CoinJoin::AreAdjacentDenominations(nSmallerDenom, nLargerDenom)) { | ||
| return false; | ||
| } | ||
|
|
||
| const int nGoal = CCoinJoinClientOptions::GetDenomsGoal(); | ||
| const int nHalfGoal = nGoal / 2; | ||
|
|
||
| const int nSmallerCount = m_wallet->CountCoinsByDenomination(nSmallerDenom, /*fFullyMixedOnly=*/false); | ||
| const int nLargerCount = m_wallet->CountCoinsByDenomination(nLargerDenom, /*fFullyMixedOnly=*/false); | ||
|
|
||
| // Don't sacrifice a denomination that's still being built up | ||
| if (nSmallerCount < nHalfGoal) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate how far each is from goal (0 if at or above goal) | ||
| const int nSmallerDeficit = std::max(0, nGoal - nSmallerCount); | ||
| const int nLargerDeficit = std::max(0, nGoal - nLargerCount); | ||
|
|
||
| // Promote if: | ||
| // 1. Smaller denom has at least half the goal (above check) | ||
| // 2. Larger denomination is further from goal (needs more help) | ||
| // 3. Gap exceeds threshold to prevent oscillation | ||
| // 4. Have 10 fully-mixed coins to promote | ||
| const int nFullyMixedCount = m_wallet->CountCoinsByDenomination(nSmallerDenom, /*fFullyMixedOnly=*/true); | ||
| if (nFullyMixedCount < CoinJoin::PROMOTION_RATIO) { | ||
| return false; | ||
| } | ||
|
|
||
| return (nLargerDeficit > nSmallerDeficit + CoinJoin::GAP_THRESHOLD); | ||
| } | ||
|
|
||
| bool CCoinJoinClientManager::ShouldDemote(int nLargerDenom, int nSmallerDenom) const | ||
| { | ||
| // Validate denominations are adjacent | ||
| if (!CoinJoin::AreAdjacentDenominations(nLargerDenom, nSmallerDenom)) { | ||
| return false; | ||
| } | ||
|
|
||
| const int nGoal = CCoinJoinClientOptions::GetDenomsGoal(); | ||
| const int nHalfGoal = nGoal / 2; | ||
|
|
||
| const int nLargerCount = m_wallet->CountCoinsByDenomination(nLargerDenom, /*fFullyMixedOnly=*/false); | ||
| const int nSmallerCount = m_wallet->CountCoinsByDenomination(nSmallerDenom, /*fFullyMixedOnly=*/false); | ||
|
|
||
| // Don't sacrifice a denomination that's still being built up | ||
| if (nLargerCount < nHalfGoal) { | ||
| return false; | ||
| } | ||
|
|
||
| // Calculate how far each is from goal (0 if at or above goal) | ||
| const int nSmallerDeficit = std::max(0, nGoal - nSmallerCount); | ||
| const int nLargerDeficit = std::max(0, nGoal - nLargerCount); | ||
|
|
||
| // Demote if: | ||
| // 1. Larger denom has at least half the goal (above check) | ||
| // 2. Smaller denomination is further from goal (needs more help) | ||
| // 3. Gap exceeds threshold to prevent oscillation | ||
| return (nSmallerDeficit > nLargerDeficit + CoinJoin::GAP_THRESHOLD); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for GAP_THRESHOLD definition in the codebase
rg "GAP_THRESHOLD" --type h --type cpp -A 2 -B 2Repository: dashpay/dash
Length of output: 3792
🏁 Script executed:
#!/bin/bash
# Find default nCoinJoinDenomsGoal and understand its usage
rg "nCoinJoinDenomsGoal|GetDenomsGoal" --type cpp --type h -B 2 -A 2 | head -100Repository: dashpay/dash
Length of output: 7609
🏁 Script executed:
#!/bin/bash
# Find the default and min/max values for denoms goal
rg "DEFAULT_COINJOIN_DENOMS_GOAL|MIN_COINJOIN_DENOMS_GOAL|MAX_COINJOIN_DENOMS_GOAL" --type cpp --type hRepository: dashpay/dash
Length of output: 1301
GAP_THRESHOLD is fixed at 10 and may not scale appropriately for extreme goal values.
The promotion/demotion logic uses a hardcoded GAP_THRESHOLD = 10 (in src/coinjoin/common.h). For the default goal of 50, this represents a 20% deficit gap threshold, which is reasonable. However, the fixed constant doesn't scale with nCoinJoinDenomsGoal:
- At minimum goal (10): threshold is 100% of goal—overly restrictive for rebalancing
- At maximum goal (100,000): threshold is 0.01% of goal—insufficient to prevent oscillation
The logic itself is correct (proper adjacency validation, half-goal checks, fully-mixed coin requirements). Consider making GAP_THRESHOLD adaptive (e.g., goal / 5) to ensure consistent rebalancing behavior across all valid goal configurations (10–100,000).
|
This pull request has conflicts, please rebase. |
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
| // Post-V24: Unlock promotion/demotion inputs before clearing state | ||
| // These coins were locked in JoinExistingQueue/StartNewQueue but may not | ||
| // have been added to vecOutPointLocked yet if the session failed early | ||
| if (!m_vecPromotionInputs.empty()) { | ||
| // Add to vecOutPointLocked so UnlockCoins() will handle them properly | ||
| // with its retry mechanism if the wallet is locked | ||
| for (const auto& outpoint : m_vecPromotionInputs) { | ||
| // Only add if not already in the list (avoid duplicates) | ||
| if (std::find(vecOutPointLocked.begin(), vecOutPointLocked.end(), outpoint) == vecOutPointLocked.end()) { | ||
| vecOutPointLocked.push_back(outpoint); | ||
| } | ||
| } | ||
| } | ||
| m_fPromotion = false; | ||
| m_fDemotion = false; | ||
| m_vecPromotionInputs.clear(); |
There was a problem hiding this comment.
This fallback runs after most unlock paths.
The code only appends m_vecPromotionInputs into vecOutPointLocked; it never unlocks them. Callers like ResetPool(), CompletedTransaction(), and the POOL_STATE_ERROR timeout branch already call UnlockCoins() before SetNull(), so these outpoints are either re-queued after they were unlocked or remain locked indefinitely if they were never tracked earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/client.cpp` around lines 296 - 311, The fallback only appends
m_vecPromotionInputs into vecOutPointLocked but never unlocks them, leaving some
coins locked indefinitely; call UnlockCoins(...) on the promotion inputs before
clearing/resetting to ensure they are released. Specifically, in the same
cleanup path that touches m_vecPromotionInputs and flags (m_fPromotion,
m_fDemotion), invoke UnlockCoins(m_vecPromotionInputs) (or UnlockCoins with the
appropriate overload) so those outpoints are unlocked even if they were never
added earlier to vecOutPointLocked; keep this change consistent with other
callers like ResetPool(), CompletedTransaction(), and the POOL_STATE_ERROR
timeout branch that expect UnlockCoins() to run before SetNull()/clearing state.
| [[nodiscard]] bool IsExpired(const CBlockIndex* pindex, const llmq::CChainLocksHandler& clhandler) const; | ||
| [[nodiscard]] bool IsValidStructure(const CBlockIndex* pindex) const; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '34,40p' src/coinjoin/coinjoin.h
sed -n '292,297p' src/coinjoin/coinjoin.h
sed -n '420,445p' src/coinjoin/coinjoin.h
rg -n -C2 '\b(CChainLocksHandler|Chainlocks)\b' srcRepository: dashpay/dash
Length of output: 33868
Fix incorrect type name in IsExpired() signature.
Line 296 declares IsExpired with parameter const llmq::CChainLocksHandler& clhandler, but CChainLocksHandler does not exist in the llmq namespace. The correct type is chainlock::ChainlockHandler. Update the signature to use the correct type and add the necessary forward declaration if not already present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/coinjoin/coinjoin.h` around lines 296 - 297, Change the IsExpired method
signature to accept the correct Chainlock handler type: replace the parameter
type llmq::CChainLocksHandler with chainlock::ChainlockHandler in the IsExpired
declaration and any corresponding definition; if chainlock::ChainlockHandler
isn't visible here, add or update a forward declaration for class
ChainlockHandler inside the chainlock namespace (or include the appropriate
header) so the compiler can resolve chainlock::ChainlockHandler when compiling
the IsExpired(const CBlockIndex* pindex, const chainlock::ChainlockHandler&
clhandler) method.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR #7052 adds CoinJoin promotion (10 smaller → 1 larger) and demotion (1 larger → 10 smaller) gated by V24 activation. The core validation logic in IsValidInOuts is sound. Two blocking issues remain: (1) the full-session finalization path bypasses the new minimum standard mixer requirement, allowing sessions with zero privacy cover; (2) the new coin selection function skips the IsLockedCoin check, enabling concurrent sessions to double-spend the same inputs.
Reviewed commit: c2089b6
🔴 2 blocking | 🟡 3 suggestion(s) | 💬 1 nitpick(s)
1 additional finding
🔴 blocking: Full-session finalization bypasses minimum standard mixer check
src/coinjoin/server.cpp (lines 296-299)
CheckPool() has two finalization paths. The timeout path (line 306-307) correctly enforces GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants(). But the full-session path (line 296) only checks GetEntriesCount() == vecSessionCollaterals.size() — it does not verify that enough standard (1:1) entries exist. If all participants submit promotion/demotion entries, the session finalizes with zero standard mixers, providing no privacy cover.
Both review agents independently flagged this as the highest-priority issue.
💡 Suggested change
if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) {
if (GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()) {
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n");
CreateFinalTransaction();
return;
}
LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- all entries received but insufficient standard mixers (%d), waiting for timeout\n", GetStandardEntriesCount());
}
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [BLOCKING] lines 296-299: Full-session finalization bypasses minimum standard mixer check
CheckPool() has two finalization paths. The timeout path (line 306-307) correctly enforces `GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()`. But the full-session path (line 296) only checks `GetEntriesCount() == vecSessionCollaterals.size()` — it does not verify that enough standard (1:1) entries exist. If all participants submit promotion/demotion entries, the session finalizes with zero standard mixers, providing no privacy cover.
Both review agents independently flagged this as the highest-priority issue.
- [SUGGESTION] lines 230-232: V24 activation checks access ActiveChain().Tip() without cs_main
Two locations access `m_chainman.ActiveChain().Tip()` without holding cs_main:
- ProcessDSVIN (line 231): checks V24 activation for unbalanced entry validation
- AddEntry (line 620): checks V24 activation to determine nMaxEntryInputs
The tip pointer could become stale between read and use. The downstream IsValidInOuts() at coinjoin.cpp:222-225 does acquire cs_main properly, creating an inconsistency. These are race windows that could allow a brief period of incorrect accept/reject behavior during V24 activation.
In `src/wallet/coinjoin.cpp`:
- [BLOCKING] lines 443-471: SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation
SelectFullyMixedForPromotion() (line 443-471) iterates setWalletUTXO without checking IsLockedCoin(). The existing denomination selector at line 181 correctly filters with `IsSpent(target_outpoint) || IsLockedCoin(target_outpoint)`. Without this check, concurrent CoinJoin sessions can select and reserve the same UTXOs for promotion, leading to signing failures or double-spend attempts.
CountCoinsByDenomination() (line 414-441) has the same omission but is less critical since it's only used for counting.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 586-677: ValidatePromotionEntry/ValidateDemotionEntry are dead code in production
ValidatePromotionEntry() and ValidateDemotionEntry() are declared in coinjoin.h, implemented in coinjoin.cpp (lines 586-677), and extensively tested in coinjoin_inouts_tests.cpp. However, they are never called from production code — the actual validation is performed inline within IsValidInOuts(). This creates dead code and a maintenance risk: the tested functions could diverge from the actual validation path without detection.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths lack unit test coverage
The test at line 382 confirms pre-V24 rejection of unbalanced entries but explicitly notes (line 384) that post-V24 acceptance requires a CBlockIndex with V24 active, which is not set up. The post-V24 code paths — where promotion and demotion entries are actually accepted and validated — are not exercised in unit tests. This means the core new functionality is only covered by the standalone Validate*Entry tests (which are dead code in production, per finding above).
| std::vector<COutPoint> CWallet::SelectFullyMixedForPromotion(int nDenom, int nCount) const | ||
| { | ||
| std::vector<COutPoint> vecRet; | ||
| if (!CCoinJoinClientOptions::IsEnabled()) return vecRet; | ||
|
|
||
| const CAmount nDenomAmount = CoinJoin::DenominationToAmount(nDenom); | ||
| if (nDenomAmount <= 0) return vecRet; | ||
|
|
||
| LOCK(cs_wallet); | ||
| for (const auto& outpoint : setWalletUTXO) { | ||
| if (static_cast<int>(vecRet.size()) >= nCount) break; | ||
|
|
||
| const auto it{mapWallet.find(outpoint.hash)}; | ||
| if (it == mapWallet.end()) continue; | ||
|
|
||
| const CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | ||
| if (nValue != nDenomAmount) continue; | ||
|
|
||
| // Skip unconfirmed or conflicted | ||
| if (GetTxDepthInMainChain(it->second) < 1) continue; | ||
|
|
||
| // Must be fully mixed for promotion | ||
| if (!IsFullyMixed(outpoint)) continue; | ||
|
|
||
| vecRet.push_back(outpoint); | ||
| } | ||
|
|
||
| return vecRet; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation
SelectFullyMixedForPromotion() (line 443-471) iterates setWalletUTXO without checking IsLockedCoin(). The existing denomination selector at line 181 correctly filters with IsSpent(target_outpoint) || IsLockedCoin(target_outpoint). Without this check, concurrent CoinJoin sessions can select and reserve the same UTXOs for promotion, leading to signing failures or double-spend attempts.
CountCoinsByDenomination() (line 414-441) has the same omission but is less critical since it's only used for counting.
💡 Suggested change
| std::vector<COutPoint> CWallet::SelectFullyMixedForPromotion(int nDenom, int nCount) const | |
| { | |
| std::vector<COutPoint> vecRet; | |
| if (!CCoinJoinClientOptions::IsEnabled()) return vecRet; | |
| const CAmount nDenomAmount = CoinJoin::DenominationToAmount(nDenom); | |
| if (nDenomAmount <= 0) return vecRet; | |
| LOCK(cs_wallet); | |
| for (const auto& outpoint : setWalletUTXO) { | |
| if (static_cast<int>(vecRet.size()) >= nCount) break; | |
| const auto it{mapWallet.find(outpoint.hash)}; | |
| if (it == mapWallet.end()) continue; | |
| const CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | |
| if (nValue != nDenomAmount) continue; | |
| // Skip unconfirmed or conflicted | |
| if (GetTxDepthInMainChain(it->second) < 1) continue; | |
| // Must be fully mixed for promotion | |
| if (!IsFullyMixed(outpoint)) continue; | |
| vecRet.push_back(outpoint); | |
| } | |
| return vecRet; | |
| } | |
| Add `if (IsLockedCoin(outpoint)) continue;` after the `if (it == mapWallet.end()) continue;` check in SelectFullyMixedForPromotion(). Consider adding the same to CountCoinsByDenomination() for consistency. |
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/wallet/coinjoin.cpp`:
- [BLOCKING] lines 443-471: SelectFullyMixedForPromotion ignores wallet coin locks, enabling double-reservation
SelectFullyMixedForPromotion() (line 443-471) iterates setWalletUTXO without checking IsLockedCoin(). The existing denomination selector at line 181 correctly filters with `IsSpent(target_outpoint) || IsLockedCoin(target_outpoint)`. Without this check, concurrent CoinJoin sessions can select and reserve the same UTXOs for promotion, leading to signing failures or double-spend attempts.
CountCoinsByDenomination() (line 414-441) has the same omission but is less critical since it's only used for counting.
| // This is a promotion or demotion entry - requires V24 activation | ||
| const CBlockIndex* pindex = m_chainman.ActiveChain().Tip(); | ||
| const bool fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24); |
There was a problem hiding this comment.
🟡 Suggestion: V24 activation checks access ActiveChain().Tip() without cs_main
Two locations access m_chainman.ActiveChain().Tip() without holding cs_main:
- ProcessDSVIN (line 231): checks V24 activation for unbalanced entry validation
- AddEntry (line 620): checks V24 activation to determine nMaxEntryInputs
The tip pointer could become stale between read and use. The downstream IsValidInOuts() at coinjoin.cpp:222-225 does acquire cs_main properly, creating an inconsistency. These are race windows that could allow a brief period of incorrect accept/reject behavior during V24 activation.
💡 Suggested change
| // This is a promotion or demotion entry - requires V24 activation | |
| const CBlockIndex* pindex = m_chainman.ActiveChain().Tip(); | |
| const bool fV24Active = pindex && DeploymentActiveAt(*pindex, Params().GetConsensus(), Consensus::DEPLOYMENT_V24); | |
| Wrap both V24 checks in `LOCK(cs_main)` or hoist the activation check into a helper that acquires the lock. |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 230-232: V24 activation checks access ActiveChain().Tip() without cs_main
Two locations access `m_chainman.ActiveChain().Tip()` without holding cs_main:
- ProcessDSVIN (line 231): checks V24 activation for unbalanced entry validation
- AddEntry (line 620): checks V24 activation to determine nMaxEntryInputs
The tip pointer could become stale between read and use. The downstream IsValidInOuts() at coinjoin.cpp:222-225 does acquire cs_main properly, creating an inconsistency. These are race windows that could allow a brief period of incorrect accept/reject behavior during V24 activation.
| bool CoinJoin::ValidatePromotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | ||
| int nSessionDenom, PoolMessage& nMessageIDRet) | ||
| { | ||
| // Promotion: 10 inputs of smaller denom → 1 output of larger denom | ||
| // Session denom is the smaller denom (inputs) | ||
| nMessageIDRet = MSG_NOERR; | ||
|
|
||
| // Check input count | ||
| if (vecTxIn.size() != static_cast<size_t>(PROMOTION_RATIO)) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong input count %zu, expected %d\n", | ||
| vecTxIn.size(), PROMOTION_RATIO); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Check output count | ||
| if (vecTxOut.size() != 1) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong output count %zu, expected 1\n", | ||
| vecTxOut.size()); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Get the larger adjacent denomination | ||
| const int nLargerDenom = GetLargerAdjacentDenom(nSessionDenom); | ||
| if (nLargerDenom == 0) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: no larger adjacent denom for %s\n", | ||
| DenominationToString(nSessionDenom)); | ||
| nMessageIDRet = ERR_DENOM; | ||
| return false; | ||
| } | ||
|
|
||
| // Validate output is at larger denomination | ||
| const int nOutputDenom = AmountToDenomination(vecTxOut[0].nValue); | ||
| if (nOutputDenom != nLargerDenom) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output denom %s != expected %s\n", | ||
| DenominationToString(nOutputDenom), DenominationToString(nLargerDenom)); | ||
| nMessageIDRet = ERR_DENOM; | ||
| return false; | ||
| } | ||
|
|
||
| // Validate output is P2PKH | ||
| if (!vecTxOut[0].scriptPubKey.IsPayToPublicKeyHash()) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output is not P2PKH\n"); | ||
| nMessageIDRet = ERR_INVALID_SCRIPT; | ||
| return false; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| bool CoinJoin::ValidateDemotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | ||
| int nSessionDenom, PoolMessage& nMessageIDRet) | ||
| { | ||
| // Demotion: 1 input of larger denom → 10 outputs of smaller denom | ||
| // Session denom is the smaller denom (outputs) | ||
| nMessageIDRet = MSG_NOERR; | ||
|
|
||
| // Check input count | ||
| if (vecTxIn.size() != 1) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong input count %zu, expected 1\n", | ||
| vecTxIn.size()); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Check output count | ||
| if (vecTxOut.size() != static_cast<size_t>(PROMOTION_RATIO)) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong output count %zu, expected %d\n", | ||
| vecTxOut.size(), PROMOTION_RATIO); | ||
| nMessageIDRet = ERR_SIZE_MISMATCH; | ||
| return false; | ||
| } | ||
|
|
||
| // Validate all outputs are at session denomination and P2PKH | ||
| for (const auto& txout : vecTxOut) { | ||
| const int nDenom = AmountToDenomination(txout.nValue); | ||
| if (nDenom != nSessionDenom) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output denom %s != session denom %s\n", | ||
| DenominationToString(nDenom), DenominationToString(nSessionDenom)); | ||
| nMessageIDRet = ERR_DENOM; | ||
| return false; | ||
| } | ||
| if (!txout.scriptPubKey.IsPayToPublicKeyHash()) { | ||
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output is not P2PKH\n"); | ||
| nMessageIDRet = ERR_INVALID_SCRIPT; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: ValidatePromotionEntry/ValidateDemotionEntry are dead code in production
ValidatePromotionEntry() and ValidateDemotionEntry() are declared in coinjoin.h, implemented in coinjoin.cpp (lines 586-677), and extensively tested in coinjoin_inouts_tests.cpp. However, they are never called from production code — the actual validation is performed inline within IsValidInOuts(). This creates dead code and a maintenance risk: the tested functions could diverge from the actual validation path without detection.
💡 Suggested change
| bool CoinJoin::ValidatePromotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | |
| int nSessionDenom, PoolMessage& nMessageIDRet) | |
| { | |
| // Promotion: 10 inputs of smaller denom → 1 output of larger denom | |
| // Session denom is the smaller denom (inputs) | |
| nMessageIDRet = MSG_NOERR; | |
| // Check input count | |
| if (vecTxIn.size() != static_cast<size_t>(PROMOTION_RATIO)) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong input count %zu, expected %d\n", | |
| vecTxIn.size(), PROMOTION_RATIO); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Check output count | |
| if (vecTxOut.size() != 1) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: wrong output count %zu, expected 1\n", | |
| vecTxOut.size()); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Get the larger adjacent denomination | |
| const int nLargerDenom = GetLargerAdjacentDenom(nSessionDenom); | |
| if (nLargerDenom == 0) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: no larger adjacent denom for %s\n", | |
| DenominationToString(nSessionDenom)); | |
| nMessageIDRet = ERR_DENOM; | |
| return false; | |
| } | |
| // Validate output is at larger denomination | |
| const int nOutputDenom = AmountToDenomination(vecTxOut[0].nValue); | |
| if (nOutputDenom != nLargerDenom) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output denom %s != expected %s\n", | |
| DenominationToString(nOutputDenom), DenominationToString(nLargerDenom)); | |
| nMessageIDRet = ERR_DENOM; | |
| return false; | |
| } | |
| // Validate output is P2PKH | |
| if (!vecTxOut[0].scriptPubKey.IsPayToPublicKeyHash()) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidatePromotionEntry -- ERROR: output is not P2PKH\n"); | |
| nMessageIDRet = ERR_INVALID_SCRIPT; | |
| return false; | |
| } | |
| return true; | |
| } | |
| bool CoinJoin::ValidateDemotionEntry(const std::vector<CTxIn>& vecTxIn, const std::vector<CTxOut>& vecTxOut, | |
| int nSessionDenom, PoolMessage& nMessageIDRet) | |
| { | |
| // Demotion: 1 input of larger denom → 10 outputs of smaller denom | |
| // Session denom is the smaller denom (outputs) | |
| nMessageIDRet = MSG_NOERR; | |
| // Check input count | |
| if (vecTxIn.size() != 1) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong input count %zu, expected 1\n", | |
| vecTxIn.size()); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Check output count | |
| if (vecTxOut.size() != static_cast<size_t>(PROMOTION_RATIO)) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: wrong output count %zu, expected %d\n", | |
| vecTxOut.size(), PROMOTION_RATIO); | |
| nMessageIDRet = ERR_SIZE_MISMATCH; | |
| return false; | |
| } | |
| // Validate all outputs are at session denomination and P2PKH | |
| for (const auto& txout : vecTxOut) { | |
| const int nDenom = AmountToDenomination(txout.nValue); | |
| if (nDenom != nSessionDenom) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output denom %s != session denom %s\n", | |
| DenominationToString(nDenom), DenominationToString(nSessionDenom)); | |
| nMessageIDRet = ERR_DENOM; | |
| return false; | |
| } | |
| if (!txout.scriptPubKey.IsPayToPublicKeyHash()) { | |
| LogPrint(BCLog::COINJOIN, "CoinJoin::ValidateDemotionEntry -- ERROR: output is not P2PKH\n"); | |
| nMessageIDRet = ERR_INVALID_SCRIPT; | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| Either refactor IsValidInOuts() to delegate to these functions, or remove them and test IsValidInOuts() directly. |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/coinjoin.cpp`:
- [SUGGESTION] lines 586-677: ValidatePromotionEntry/ValidateDemotionEntry are dead code in production
ValidatePromotionEntry() and ValidateDemotionEntry() are declared in coinjoin.h, implemented in coinjoin.cpp (lines 586-677), and extensively tested in coinjoin_inouts_tests.cpp. However, they are never called from production code — the actual validation is performed inline within IsValidInOuts(). This creates dead code and a maintenance risk: the tested functions could diverge from the actual validation path without detection.
| BOOST_AUTO_TEST_CASE(isvalidstructure_postfork_unbalanced_valid) | ||
| { | ||
| // Post-V24: Unbalanced vin/vout (promotion: 10 inputs, 1 output) should be valid | ||
| // We need a mock pindex that signals V24 active - for this test we use nullptr which means pre-fork | ||
| // This test validates that the structure check correctly identifies promotion structure | ||
|
|
||
| CCoinJoinBroadcastTx promo; | ||
| { | ||
| CMutableTransaction mtx; | ||
| // Promotion: 10 inputs of smaller denom -> 1 output of larger denom | ||
| const int nInputCount = CoinJoin::PROMOTION_RATIO; | ||
| const CAmount nLargerAmount = CoinJoin::DenominationToAmount(1 << 1); // 1.0 DASH | ||
|
|
||
| for (int i = 0; i < nInputCount; ++i) { | ||
| CTxIn in; | ||
| in.prevout = COutPoint(uint256::ONE, static_cast<uint32_t>(i)); | ||
| mtx.vin.push_back(in); | ||
| } | ||
| // 1 output of larger denom | ||
| CTxOut out{nLargerAmount, P2PKHScript(0x01)}; | ||
| mtx.vout.push_back(out); | ||
|
|
||
| promo.tx = MakeTransactionRef(mtx); | ||
| promo.m_protxHash = uint256::ONE; | ||
| } | ||
|
|
||
| // Pre-V24 (nullptr): unbalanced should fail | ||
| BOOST_CHECK(!promo.IsValidStructure(nullptr)); | ||
|
|
||
| // Note: Post-V24 test would require a valid CBlockIndex with V24 deployment active | ||
| // which requires more setup. The above confirms pre-fork rejection works. | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Post-V24 acceptance paths lack unit test coverage
The test at line 382 confirms pre-V24 rejection of unbalanced entries but explicitly notes (line 384) that post-V24 acceptance requires a CBlockIndex with V24 active, which is not set up. The post-V24 code paths — where promotion and demotion entries are actually accepted and validated — are not exercised in unit tests. This means the core new functionality is only covered by the standalone Validate*Entry tests (which are dead code in production, per finding above).
💡 Suggested change
| BOOST_AUTO_TEST_CASE(isvalidstructure_postfork_unbalanced_valid) | |
| { | |
| // Post-V24: Unbalanced vin/vout (promotion: 10 inputs, 1 output) should be valid | |
| // We need a mock pindex that signals V24 active - for this test we use nullptr which means pre-fork | |
| // This test validates that the structure check correctly identifies promotion structure | |
| CCoinJoinBroadcastTx promo; | |
| { | |
| CMutableTransaction mtx; | |
| // Promotion: 10 inputs of smaller denom -> 1 output of larger denom | |
| const int nInputCount = CoinJoin::PROMOTION_RATIO; | |
| const CAmount nLargerAmount = CoinJoin::DenominationToAmount(1 << 1); // 1.0 DASH | |
| for (int i = 0; i < nInputCount; ++i) { | |
| CTxIn in; | |
| in.prevout = COutPoint(uint256::ONE, static_cast<uint32_t>(i)); | |
| mtx.vin.push_back(in); | |
| } | |
| // 1 output of larger denom | |
| CTxOut out{nLargerAmount, P2PKHScript(0x01)}; | |
| mtx.vout.push_back(out); | |
| promo.tx = MakeTransactionRef(mtx); | |
| promo.m_protxHash = uint256::ONE; | |
| } | |
| // Pre-V24 (nullptr): unbalanced should fail | |
| BOOST_CHECK(!promo.IsValidStructure(nullptr)); | |
| // Note: Post-V24 test would require a valid CBlockIndex with V24 deployment active | |
| // which requires more setup. The above confirms pre-fork rejection works. | |
| } | |
| Add test fixtures that construct a CBlockIndex with V24 deployment active to exercise the acceptance paths in IsValidStructure() and IsValidInOuts(). |
source: ['claude']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths lack unit test coverage
The test at line 382 confirms pre-V24 rejection of unbalanced entries but explicitly notes (line 384) that post-V24 acceptance requires a CBlockIndex with V24 active, which is not set up. The post-V24 code paths — where promotion and demotion entries are actually accepted and validated — are not exercised in unit tests. This means the core new functionality is only covered by the standalone Validate*Entry tests (which are dead code in production, per finding above).
| // Post-V24: Promotion/demotion session state | ||
| bool m_fPromotion{false}; // True if this session is promoting smaller -> larger denom | ||
| bool m_fDemotion{false}; // True if this session is demoting larger -> smaller denom | ||
| std::vector<COutPoint> m_vecPromotionInputs; // Selected inputs for promotion (10 coins) |
There was a problem hiding this comment.
💬 Nitpick: m_vecPromotionInputs is reused for demotion inputs, naming is misleading
m_vecPromotionInputs stores inputs for both promotion (10 coins) and demotion (1 coin), as seen at client.cpp:1290-1292 and 1825-1839. The name and comment ("Selected inputs for promotion (10 coins)") imply promotion-only usage. Consider renaming to m_vecMixingInputs or m_vecSessionInputs.
source: ['claude']
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/coinjoin/client.cpp (3)
1225-1255:⚠️ Potential issue | 🔴 CriticalDelay rebalance locking until the queue join is definitely kept.
Promotion/demotion inputs are locked before the
IsMasternodeOrDisconnectRequested()continuepath and before the successful session state is recorded. If that check trips, the outpoints are neither invecOutPointLockednor inm_vecRebalanceInputs, so they leak out of cleanup entirely.Also applies to: 1266-1269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 1225 - 1255, The code currently locks promotion/demotion inputs (m_wallet->LockCoin calls for vecCoins and vecTxDSInTmp) before the IsMasternodeOrDisconnectRequested() continuation and before recording the session state, causing leaked locked outpoints if the join is aborted; move the LockCoin calls so they occur only after the join is definitively kept (i.e., after the IsMasternodeOrDisconnectRequested() check passes and after you have pushed the outpoints into vecOutPointLocked and/or m_vecRebalanceInputs), and wrap the LockCoin calls with LOCK(m_wallet->cs_wallet) as done elsewhere; apply the same change for the demotion path (the block that currently locks vecTxDSInTmp[0].prevout) and the equivalent locking near the later duplicate section so locks are only taken once the session state/ownership vectors are updated.
296-311:⚠️ Potential issue | 🟠 Major
SetNull()does not actually release rebalance locks.Most cleanup paths already call
UnlockCoins()beforeSetNull()(for example Lines 273, 449, and 728). Appendingm_vecRebalanceInputshere only repopulatesvecOutPointLockedafter the unlock pass, so sessions that fail beforePreparePromotionEntry()orPrepareDemotionEntry()can leave those coins locked indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 296 - 311, SetNull() currently appends m_vecRebalanceInputs to vecOutPointLocked after other UnlockCoins() calls, which re-locks coins; instead ensure rebalance inputs are explicitly unlocked before clearing: call UnlockCoins(m_vecRebalanceInputs) (or add them to vecOutPointLocked before the existing UnlockCoins() pass) and only then clear m_vecRebalanceInputs and reset m_fPromotion/m_fDemotion; reference SetNull(), UnlockCoins(), m_vecRebalanceInputs, vecOutPointLocked, PreparePromotionEntry(), and PrepareDemotionEntry() when making the change.
1431-1456:⚠️ Potential issue | 🔴 Critical
StartNewQueue()has the same pre-selection lock leak.These coins are locked before the masternode retry loop starts, but
m_vecRebalanceInputsis only populated on the success path at Lines 1508-1514. Anycontinueorreturn falsebefore then leaves the rebalance inputs locked with no owner to unlock them.Also applies to: 1467-1514
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/coinjoin/client.cpp` around lines 1431 - 1456, StartNewQueue() locks demotion inputs (m_wallet->LockCoin(vecTxDSInTmp[0].prevout)) before the masternode retry/selection path completes, but m_vecRebalanceInputs is only populated on the success path, causing locked coins to be leaked on early returns; fix by deferring the LockCoin call until after the masternode selection succeeds and m_vecRebalanceInputs will be set, or immediately add the locked prevout into m_vecRebalanceInputs (so ownership/cleanup is tracked) and ensure any early return paths unlock those entries; update the CCoinJoinClientSession::StartNewQueue logic around vecTxDSInTmp/prevout locking to either move the LOCK(m_wallet->cs_wallet)/LockCoin(...) to after the success branch or push_back the prevout into m_vecRebalanceInputs right when locking so cleanup/unlock is guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/coinjoin/coinjoin.cpp`:
- Around line 227-276: IsValidInOuts() now treats the whole vin/vout as one
EntryType (EntryType enum) and rejects mixed-session final transactions (e.g.
multiple STANDARD entries plus a PROMOTION), causing clients/servers (client.cpp
and server.cpp callers) to fail signing; fix by restoring per-entry validation
or adding a dedicated final-transaction validator: either (A) change the callers
in client.cpp/server.cpp to call the existing per-entry validators
(CoinJoin::ValidatePromotionEntry, CoinJoin::ValidateDemotionEntry, or the
STANDARD path) for each logical entry instead of passing the whole tx into
IsValidInOuts(), or (B) implement a new function (e.g.,
CoinJoin::ValidateFinalTransaction) that parses the combined vin/vout into
constituent entries, validates each entry with the existing
ValidatePromotionEntry/ValidateDemotionEntry logic, and determines expected
denominations using CoinJoin::GetLargerAdjacentDenom per entry; update
IsValidInOuts() to only validate single entries or delegate to the new
ValidateFinalTransaction when given a full-final-tx context so mixed entries are
accepted.
- Around line 103-127: Add an explicit post‑V24 cap on outputs: after computing
nMaxInputs (which uses fV24Active, CoinJoin::GetMaxPoolParticipants() and
CoinJoin::PROMOTION_RATIO), add a check that tx->vout.size() <= nMaxInputs and
return false if it exceeds it; keep the existing denominated P2PKH validation
unchanged so IsValidStructure()/the surrounding logic enforces both input and
output caps.
In `@src/wallet/coinjoin.cpp`:
- Around line 424-431: The loop over setWalletUTXO accesses
it->second.tx->vout[outpoint.n] without verifying outpoint.n is a valid index;
add a guard before any vout indexing in both loops (the one around
mapWallet.find and the one at 454-463) to check that it->second.tx is non-null
and outpoint.n < it->second.tx->vout.size(), and continue if not; keep existing
checks (IsSpent, IsLockedCoin, nValue == nDenomAmount) unchanged but perform
them only after this bounds/null check to avoid out-of-bounds reads when
handling stale or inconsistent outpoints (symbols: setWalletUTXO, mapWallet,
it->second.tx, vout, outpoint.n, IsSpent, IsLockedCoin, nDenomAmount).
---
Duplicate comments:
In `@src/coinjoin/client.cpp`:
- Around line 1225-1255: The code currently locks promotion/demotion inputs
(m_wallet->LockCoin calls for vecCoins and vecTxDSInTmp) before the
IsMasternodeOrDisconnectRequested() continuation and before recording the
session state, causing leaked locked outpoints if the join is aborted; move the
LockCoin calls so they occur only after the join is definitively kept (i.e.,
after the IsMasternodeOrDisconnectRequested() check passes and after you have
pushed the outpoints into vecOutPointLocked and/or m_vecRebalanceInputs), and
wrap the LockCoin calls with LOCK(m_wallet->cs_wallet) as done elsewhere; apply
the same change for the demotion path (the block that currently locks
vecTxDSInTmp[0].prevout) and the equivalent locking near the later duplicate
section so locks are only taken once the session state/ownership vectors are
updated.
- Around line 296-311: SetNull() currently appends m_vecRebalanceInputs to
vecOutPointLocked after other UnlockCoins() calls, which re-locks coins; instead
ensure rebalance inputs are explicitly unlocked before clearing: call
UnlockCoins(m_vecRebalanceInputs) (or add them to vecOutPointLocked before the
existing UnlockCoins() pass) and only then clear m_vecRebalanceInputs and reset
m_fPromotion/m_fDemotion; reference SetNull(), UnlockCoins(),
m_vecRebalanceInputs, vecOutPointLocked, PreparePromotionEntry(), and
PrepareDemotionEntry() when making the change.
- Around line 1431-1456: StartNewQueue() locks demotion inputs
(m_wallet->LockCoin(vecTxDSInTmp[0].prevout)) before the masternode
retry/selection path completes, but m_vecRebalanceInputs is only populated on
the success path, causing locked coins to be leaked on early returns; fix by
deferring the LockCoin call until after the masternode selection succeeds and
m_vecRebalanceInputs will be set, or immediately add the locked prevout into
m_vecRebalanceInputs (so ownership/cleanup is tracked) and ensure any early
return paths unlock those entries; update the
CCoinJoinClientSession::StartNewQueue logic around vecTxDSInTmp/prevout locking
to either move the LOCK(m_wallet->cs_wallet)/LockCoin(...) to after the success
branch or push_back the prevout into m_vecRebalanceInputs right when locking so
cleanup/unlock is guaranteed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ccc108f2-8300-41e8-b074-26afc4c78c13
📒 Files selected for processing (5)
src/coinjoin/client.cppsrc/coinjoin/client.hsrc/coinjoin/coinjoin.cppsrc/coinjoin/server.cppsrc/wallet/coinjoin.cpp
| for (const auto& outpoint : setWalletUTXO) { | ||
| const auto it{mapWallet.find(outpoint.hash)}; | ||
| if (it == mapWallet.end()) continue; | ||
|
|
||
| if (IsSpent(outpoint) || IsLockedCoin(outpoint)) continue; | ||
|
|
||
| const CAmount nValue = it->second.tx->vout[outpoint.n].nValue; | ||
| if (nValue != nDenomAmount) continue; |
There was a problem hiding this comment.
Guard outpoint.n before indexing vout.
Both new helpers dereference it->second.tx->vout[outpoint.n] straight from setWalletUTXO. A stale or inconsistent outpoint now turns the background promotion logic into an out-of-bounds read instead of just skipping the entry.
🐛 Minimal guard
const auto it{mapWallet.find(outpoint.hash)};
if (it == mapWallet.end()) continue;
+ if (outpoint.n >= it->second.tx->vout.size()) continue;
if (IsSpent(outpoint) || IsLockedCoin(outpoint)) continue;Apply the same guard in both loops.
Also applies to: 454-463
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/coinjoin.cpp` around lines 424 - 431, The loop over setWalletUTXO
accesses it->second.tx->vout[outpoint.n] without verifying outpoint.n is a valid
index; add a guard before any vout indexing in both loops (the one around
mapWallet.find and the one at 454-463) to check that it->second.tx is non-null
and outpoint.n < it->second.tx->vout.size(), and continue if not; keep existing
checks (IsSpent, IsLockedCoin, nValue == nDenomAmount) unchanged but perform
them only after this bounds/null check to avoid out-of-bounds reads when
handling stale or inconsistent outpoints (symbols: setWalletUTXO, mapWallet,
it->second.tx, vout, outpoint.n, IsSpent, IsLockedCoin, nDenomAmount).
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
This push addresses 4 of 5 prior review findings cleanly: the blocking coin-lock double-reservation bug is fixed, V24 activation checks now hold cs_main, the dead-code validators are integrated into the production path, and the misleading m_vecPromotionInputs name is corrected to m_vecRebalanceInputs. The new outpoint index bounds checks and IsSpent/IsLockedCoin filters are correct. One design concern: sessions composed entirely of promotion/demotion entries (insufficient standard mixers) will accept all entries then stall until timeout rather than failing fast.
Reviewed commit: 332186c
🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 300-307: Sessions with insufficient standard mixers stall until timeout instead of failing fast
Once `GetEntriesCount() == vecSessionCollaterals.size()`, the pool is full — `AddEntry()` rejects further entries and the server is past `POOL_STATE_QUEUE`. If `GetStandardEntriesCount() < GetMinPoolParticipants()` at this point, the session is deterministically unsatisfiable: it will never gain more standard mixers. Lines 300-306 log and wait, then `CheckTimeout()` eventually resets via `SetNull()`. This means all participants (including legitimate standard mixers) wait the full timeout window before the session is recycled.
Consider resetting immediately when the pool is full but the standard-mixer threshold is unachievable, so participants can re-queue without the timeout delay.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths still lack integration test coverage
The incremental diff changes live server behavior (IsValidInOuts now dispatches through the dedicated validators; CheckPool enforces minimum standard mixers) but adds no test updates. The standalone `ValidatePromotionEntry`/`ValidateDemotionEntry` tests are now exercising live code (good), but the full server-side path — `ProcessDSVIN` → `AddEntry` → `IsValidInOuts` → `CheckPool` with V24 active — remains untested. The new insufficient-standard-mixer branch is also uncovered.
| if (nState == POOL_STATE_ACCEPTING_ENTRIES && size_t(GetEntriesCount()) == vecSessionCollaterals.size()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n"); | ||
| CreateFinalTransaction(); | ||
| return; | ||
| if (GetStandardEntriesCount() >= CoinJoin::GetMinPoolParticipants()) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- FINALIZE TRANSACTIONS\n"); | ||
| CreateFinalTransaction(); | ||
| return; | ||
| } | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- all entries received but insufficient standard mixers (%d), waiting for timeout\n", GetStandardEntriesCount()); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Sessions with insufficient standard mixers stall until timeout instead of failing fast
Once GetEntriesCount() == vecSessionCollaterals.size(), the pool is full — AddEntry() rejects further entries and the server is past POOL_STATE_QUEUE. If GetStandardEntriesCount() < GetMinPoolParticipants() at this point, the session is deterministically unsatisfiable: it will never gain more standard mixers. Lines 300-306 log and wait, then CheckTimeout() eventually resets via SetNull(). This means all participants (including legitimate standard mixers) wait the full timeout window before the session is recycled.
Consider resetting immediately when the pool is full but the standard-mixer threshold is unachievable, so participants can re-queue without the timeout delay.
source: ['codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/coinjoin/server.cpp`:
- [SUGGESTION] lines 300-307: Sessions with insufficient standard mixers stall until timeout instead of failing fast
Once `GetEntriesCount() == vecSessionCollaterals.size()`, the pool is full — `AddEntry()` rejects further entries and the server is past `POOL_STATE_QUEUE`. If `GetStandardEntriesCount() < GetMinPoolParticipants()` at this point, the session is deterministically unsatisfiable: it will never gain more standard mixers. Lines 300-306 log and wait, then `CheckTimeout()` eventually resets via `SetNull()`. This means all participants (including legitimate standard mixers) wait the full timeout window before the session is recycled.
Consider resetting immediately when the pool is full but the standard-mixer threshold is unachievable, so participants can re-queue without the timeout delay.
| BOOST_AUTO_TEST_CASE(isvalidstructure_postfork_unbalanced_valid) | ||
| { | ||
| // Post-V24: Unbalanced vin/vout (promotion: 10 inputs, 1 output) should be valid | ||
| // We need a mock pindex that signals V24 active - for this test we use nullptr which means pre-fork | ||
| // This test validates that the structure check correctly identifies promotion structure | ||
|
|
||
| CCoinJoinBroadcastTx promo; | ||
| { | ||
| CMutableTransaction mtx; | ||
| // Promotion: 10 inputs of smaller denom -> 1 output of larger denom | ||
| const int nInputCount = CoinJoin::PROMOTION_RATIO; | ||
| const CAmount nLargerAmount = CoinJoin::DenominationToAmount(1 << 1); // 1.0 DASH | ||
|
|
||
| for (int i = 0; i < nInputCount; ++i) { | ||
| CTxIn in; | ||
| in.prevout = COutPoint(uint256::ONE, static_cast<uint32_t>(i)); | ||
| mtx.vin.push_back(in); | ||
| } | ||
| // 1 output of larger denom | ||
| CTxOut out{nLargerAmount, P2PKHScript(0x01)}; | ||
| mtx.vout.push_back(out); | ||
|
|
||
| promo.tx = MakeTransactionRef(mtx); | ||
| promo.m_protxHash = uint256::ONE; | ||
| } | ||
|
|
||
| // Pre-V24 (nullptr): unbalanced should fail | ||
| BOOST_CHECK(!promo.IsValidStructure(nullptr)); | ||
|
|
||
| // Note: Post-V24 test would require a valid CBlockIndex with V24 deployment active | ||
| // which requires more setup. The above confirms pre-fork rejection works. | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Post-V24 acceptance paths still lack integration test coverage
The incremental diff changes live server behavior (IsValidInOuts now dispatches through the dedicated validators; CheckPool enforces minimum standard mixers) but adds no test updates. The standalone ValidatePromotionEntry/ValidateDemotionEntry tests are now exercising live code (good), but the full server-side path — ProcessDSVIN → AddEntry → IsValidInOuts → CheckPool with V24 active — remains untested. The new insufficient-standard-mixer branch is also uncovered.
source: ['claude', 'codex']
🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/test/coinjoin_inouts_tests.cpp`:
- [SUGGESTION] lines 355-386: Post-V24 acceptance paths still lack integration test coverage
The incremental diff changes live server behavior (IsValidInOuts now dispatches through the dedicated validators; CheckPool enforces minimum standard mixers) but adds no test updates. The standalone `ValidatePromotionEntry`/`ValidateDemotionEntry` tests are now exercising live code (good), but the full server-side path — `ProcessDSVIN` → `AddEntry` → `IsValidInOuts` → `CheckPool` with V24 active — remains untested. The new insufficient-standard-mixer branch is also uncovered.
|
This pull request has conflicts, please rebase. |
|
⛔ Blockers found — Sonnet deferred (commit 5294f2c) |
9f03035 ci: harden conflict advisory comments (PastaClaw) a506e22 ci: make conflict prediction advisory (PastaClaw) Pull request description: # ci: make conflict prediction advisory ## Issue being fixed or feature implemented The existing potential-conflicts workflow is hard to act on because it can fail CI and only reports the conflict from the triggering PR's side. For review and merge planning, maintainers need an advisory comment on both affected PRs that explains which PR would force the other to rebase if merged first. ## What was done? Reworked the potential-conflicts workflow to: - Replace the external conflict-checker and sticky-comment actions with a repository-local Python handler. - Discover same-base open PRs with overlapping changed files through the GitHub API. - Validate overlap candidates with GitHub's pre-mergeability page before reporting them. - Maintain one GitHub Actions-owned advisory comment per affected PR. - Show both directions in that comment: - "If this PR merges first" - "If these PRs merge first" - Update reciprocal comments on the conflicting PRs so both sides show the merge-order risk. - Keep the workflow advisory-only with `continue-on-error`, so conflicts do not make CI red. - Clean stale reciprocal state when conflicts disappear, PRs close, or PRs are converted to draft. - Ignore user-authored comments that forge the managed marker; only `github-actions[bot]` comments are treated as workflow-owned state. - Serialize the workflow globally to avoid reciprocal comment write races. ## How Has This Been Tested? Validated locally on macOS with: ```bash python3 .github/workflows/test_handle_potential_conflicts.py python3 -m py_compile \ .github/workflows/handle_potential_conflicts.py \ .github/workflows/test_handle_potential_conflicts.py python3 test/lint/lint-python.py \ .github/workflows/handle_potential_conflicts.py \ .github/workflows/test_handle_potential_conflicts.py git diff --check 25aeb5f..HEAD ``` Ran a live read-only dry-run against `#7419`: ```bash GITHUB_TOKEN=$(gh auth token) \ GITHUB_REPOSITORY=dashpay/dash \ .github/workflows/handle_potential_conflicts.py \ --pr-number 7419 \ --dry-run ``` - The dry-run found `#7052` as the validated advisory conflict and rendered the managed comment body without writing comments. Also ran an Opus sidecar design pass and multiple Codex review-gate passes. The final blocker-only review returned `No issues found`. ## Breaking Changes None. The workflow remains advisory-only and does not fail CI when conflicts are found or when comment updates fail. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 94b9b8b6b9623795e5da1843b5dc7e6192d17dbacaaef26a6f54e0a0c032d283fd3d2ae4358d275c5c5d4ed579c70977b42241a91bf53c2e1ec66e95e307bdcf
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If these PRs merge firstThis PR will likely need a rebase:
|
3e7e959 to
1fde12a
Compare
|
Pushed a cleanup pass addressing the outstanding review findings and restructuring history: Code changes
Deliberately not changed
History |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Cumulative new-push review at head 1fde12a. Verified all five prior findings against the worktree: prior-1 (collateral-bypass) fixed by removing the ProcessDSVIN pre-check so unbalanced entries reach AddEntry->IsValidInOuts and consume collateral; prior-3 (rebalance-lock leak) fixed with UnlockCoins/ReturnAll/SetNull on the Prepare-failure branch; prior-4/prior-5 (commit hygiene) fixed by squashing to three atomic commits. prior-2 is intentionally deferred: the FINAL_TX aggregate arithmetic was extracted into ValidateFinalTxComposition() with direct unit coverage, while V24-active server acceptance-flow coverage is deferred to functional tests per the PR description. No new blocking or suggestion-level defects found in the coinjoin promotion/demotion logic or the DSTX fork-boundary penalty narrowing.
Source: reviewers — claude/opus [general: ok], codex/gpt-5.5 [general: FAILED (failed)], claude/opus [dash-core-commit-history: ok], codex/gpt-5.5 [dash-core-commit-history: FAILED (failed)]; verifier — claude/opus
Prior findings reconciliation
prior-1[suggestion]src/coinjoin/server.cpp:213-218— Pre-V24 DSVIN pre-check bypasses ConsumeCollateral anti-spam path: FIXED — The early ERR_MODE pre-check in ProcessDSVIN is gone (server.cpp:213-218 now documents that unbalanced entries flow into AddEntry->IsValidInOuts, which rejects them pre-V24 and consumes collateral).prior-2[suggestion]src/coinjoin/coinjoin.cpp:394-409— New FINAL_TX aggregate validation and post-V24 server acceptance paths lack unit coverage: INTENTIONALLY DEFERRED — The aggregate arithmetic is extracted into CoinJoin::ValidateFinalTxComposition() and directly unit-tested (coinjoin_inouts_tests.cpp:603-635); residual V24-active server acceptance-flow coverage is explicitly deferred to functional tests per the PR description.prior-3[suggestion]src/coinjoin/client.cpp:1468-1486— Promotion/demotion SubmitDenominate failures leave rebalance inputs locked until timeout: FIXED — SubmitDenominate's promotion/demotion Prepare-failure branch (client.cpp:1481-1483) now calls UnlockCoins(); keyHolderStorage.ReturnAll(); WITH_LOCK(cs_coinjoin, SetNull()), releasing rebalance inputs immediately instead of waiting for CheckTimeout.prior-4[suggestion]<commit:3e7e9591>— 3e7e9591 explicitly walks back f6e3cb49 within the same push — squash the pair: FIXED — The branch is now a three-commit stack (feat/test/docs); f6e3cb49 and 3e7e9591 are no longer ancestors of HEAD, so the walk-back pair is gone.prior-5[suggestion]<commit:1244b53047>— Same-push initial-feature bugfixes should be squashed into 313f643223: FIXED — 313f643223 and its same-push fixups (3d3d5210f6, 1244b53047) are folded into the single feat commit 15eeeb9; none are reachable from HEAD.
New findings in latest delta
(none)
Additional cumulative findings
(none)
|
This pull request has conflicts, please rebase. |
Fixes for PR dashpay#7052: Issue #1 (CRITICAL): Fix race condition in coin selection - Lock coins immediately after selection in JoinExistingQueue/StartNewQueue - Prevents concurrent sessions from selecting the same coins - Add defensive IsLockedCoin check in Prepare functions Issue #2 (CRITICAL): Fix resource leak on session failure - Add promotion inputs to vecOutPointLocked in SetNull() - Ensures coins locked early are properly unlocked if session fails - Leverages existing UnlockCoins() retry mechanism Issue #3: Add UTXO validation before use - Check IsSpent() before using promotion/demotion inputs - Prevents using externally spent or transferred coins 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ensure promotion/demotion sessions have adequate privacy by requiring the minimum participant threshold to be met by STANDARD mixing entries only. Implementation: - Add IsStandardMixingEntry() method to CCoinJoinEntry Detects entry type: standard (equal in/out), promotion (10:1), demotion (1:10) - Add GetStandardEntriesCount() to CCoinJoinBaseSession Counts only standard mixing entries for threshold checks - Modify server CheckPool timeout logic to use GetStandardEntriesCount() Only standard entries count toward minimum; promotion/demotion get free cover Why this matters: - Promotion/demotion creates unique patterns (10→1 or 1→10) - Small sessions where you're the only rebalancer = weak anonymity (33%) - Requiring minimum standard mixers provides cover traffic Benefits: - Promotion/demotion can still start queues (better UX, doesn't block feature) - Privacy protected by requiring standard mixer cover traffic - Session won't finalize until adequate anonymity set exists Example: Before: 1 promotion + 2 standard = proceeds (weak: 33% chance you're identified) After: 1 promotion + 2 standard = times out (needs 3 standard minimum) After: 2 promotion + 4 standard = proceeds (good: 6 participants, better cover!) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Remove 4 redundant/low-value tests: - promotion_ratio_constant (trivial constant check) - gap_threshold_constant (trivial constant check) - decision_logic_mutual_exclusivity (documentation only) - adjacent_denomination_helpers (duplicate) Add 2 critical tests for privacy protection: - is_standard_mixing_entry (validates IsStandardMixingEntry method) - get_standard_entries_count (validates threshold enforcement) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
* fix: add IsLockedCoin/IsSpent check in SelectFullyMixedForPromotion Add IsSpent/IsLockedCoin filtering to both CountCoinsByDenomination() and SelectFullyMixedForPromotion() to prevent concurrent CoinJoin sessions from selecting the same UTXOs. This matches the existing filtering pattern used in the denomination selector. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add bounds checking for outpoint.n in coinjoin client Add bounds check for outpoint.n before accessing wtx.tx->vout in JoinExistingQueue and StartNewQueue promotion paths. This prevents potential out-of-bounds access if wallet state changes between UTXO selection and input construction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: acquire cs_main for V24 activation checks in server Wrap m_chainman.ActiveChain().Tip() accesses in LOCK(::cs_main) in ProcessDSVIN and AddEntry to prevent race conditions during V24 activation state checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: enforce standard mixer minimum on full-session finalization The full-session finalization path in CheckPool was creating the final transaction without verifying the minimum standard mixer count. This could allow a session with only promotion/demotion entries (which don't provide privacy) to finalize. Add the same GetStandardEntriesCount() check that the timeout path already uses. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: use ValidatePromotionEntry/ValidateDemotionEntry in IsValidInOuts Replace the inline promotion/demotion validation in IsValidInOuts() with calls to the dedicated CoinJoin::ValidatePromotionEntry() and CoinJoin::ValidateDemotionEntry() functions. These functions were already declared, implemented, and tested but never called from production code. This ensures the tested code IS the production code. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: rename m_vecPromotionInputs to m_vecRebalanceInputs The member stores inputs for both promotion AND demotion operations, so the name m_vecPromotionInputs was misleading. Rename to m_vecRebalanceInputs to better reflect its dual purpose. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: PastaClaw <thepastaclaw@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Post-rebase fixups: llmq::CChainLocksHandler was replaced by chainlock::Chainlocks, DEPLOYMENT_V24 became a versionbits deployment so DeploymentActiveAt needs the ChainstateManager, CDeterministicMNList count getters moved to GetCounts(), and the chainlocks handler moved from LLMQContext to NodeContext. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- unlock promotion/demotion inputs when a pending masternode connection expires or the masternode can't be found, instead of leaving them permanently locked - replace repeated per-denomination wallet scans in ShouldPromote/ShouldDemote with a single CWallet::GetDenominationCounts() scan shared across all four adjacent-pair checks per DoAutomaticDenominating pass - consolidate the duplicated V24-activation check into CoinJoin::IsPromotionDemotionActive(), used at all call sites in coinjoin.cpp/server.cpp/client.cpp - make DenominationToAmount reuse GetDenominationIndex instead of re-scanning vecStandardDenominations with its own loop Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- validate the aggregated final transaction with new aggregate rules (allowed denominations, value balance, promotion/demotion input/output consistency) instead of per-entry shape classification, which rejected every final tx containing a promotion/demotion entry and exposed honest participants to collateral charges - consolidate promotion/demotion input selection into SelectRebalanceInputs(), record locked outpoints in vecOutPointLocked at lock time and unlock on all JoinExistingQueue/StartNewQueue failure paths, fixing coin-lock leaks and the UnlockCoins-before-SetNull ordering bug that left rebalance inputs locked until restart - check promotion/demotion opportunities before the 'nothing to do' / 'not enough funds' early exits so rebalancing works for wallets that reached their anonymization target (the feature's primary use case) - cap promotion/demotion entries per session on the server so the frozen participant set can always reach the standard-mixer minimum, and reset a stuck session immediately instead of stalling everyone until timeout - drop unbalanced DSTXes without a Misbehaving penalty when V24 is not active at the local tip, so honest relayers are not punished around the activation boundary - implement CDSTXManager::IsTxExpired via CCoinJoinBroadcastTx::IsExpired so the unit test covers the production expiry rule Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit exempted every unbalanced DSTX from the Misbehaving penalty while V24 is inactive at the local tip, which also forgave structurally malformed transactions and weakened the anti-spam pairing with the recent-rejects exemption for DSTX messages. IsValidStructure now reports whether a failing tx would be structurally valid under post-V24 rules (activation-independent checks still enforced), and ValidateDSTX applies a small score-1 penalty (same rationale as UNKNOWN_MASTERNODE) only in that case, keeping the full penalty for garbage. Adds unit coverage for the new flag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1fde12a to
5294f2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/coinjoin/server.cpp (1)
621-668: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical: the blanket size cap at line 631 rejects every legitimate promotion/demotion entry before the V24-aware cap is ever checked.
COINJOIN_ENTRY_MAX_SIZEis 9.CoinJoin::PROMOTION_RATIOis 10 (confirmed by this PR's own tests, e.g.nMaxPostFork = GetMaxPoolParticipants() * PROMOTION_RATIO == 200). The very first check —if (entry.vecTxDSIn.size() > COINJOIN_ENTRY_MAX_SIZE || entry.vecTxOut.size() > COINJOIN_ENTRY_MAX_SIZE) { ... return false; }— unconditionally rejects (with
ERR_MAXIMUMand collateral consumption) any entry withvecTxDSIn.size() == 10(a valid promotion) orvecTxOut.size() == 10(a valid demotion), since10 > 9. The subsequent V24-awarenMaxEntryInputscheck (10 when promotion/demotion is active) at line 663 is therefore unreachable for the exact shapes it was written to allow. As written, post-V24 promotion and demotion entries submitted viaDSVINcan never be accepted by the server — the core feature of this PR is broken at the acceptance layer, and legitimate users attempting to promote/demote will additionally lose their collateral. This matches the gap already noted by past reviewers ("Post-V24 acceptance paths lack unit test coverage") — a live AddEntry test with V24 active would have caught this immediately.🐛 Proposed fix: use the V24-aware bound for the first check too
- if (entry.vecTxDSIn.size() > COINJOIN_ENTRY_MAX_SIZE || entry.vecTxOut.size() > COINJOIN_ENTRY_MAX_SIZE) { + // Post-V24: allow up to PROMOTION_RATIO (10) inputs/outputs for promotion/demotion entries + // Pre-V24: max COINJOIN_ENTRY_MAX_SIZE (9) inputs/outputs + const size_t nMaxEntryInputs = CoinJoin::IsPromotionDemotionActive(m_chainman) ? CoinJoin::PROMOTION_RATIO : COINJOIN_ENTRY_MAX_SIZE; + if (entry.vecTxDSIn.size() > nMaxEntryInputs || entry.vecTxOut.size() > nMaxEntryInputs) { LogPrint(BCLog::COINJOIN, /* Continued */ "CCoinJoinServer::%s -- ERROR: too many inputs or outputs! inputs=%s/%s, outputs=%s/%s\n", __func__, - entry.vecTxDSIn.size(), COINJOIN_ENTRY_MAX_SIZE, entry.vecTxOut.size(), COINJOIN_ENTRY_MAX_SIZE); + entry.vecTxDSIn.size(), nMaxEntryInputs, entry.vecTxOut.size(), nMaxEntryInputs); nMessageIDRet = ERR_MAXIMUM; ... return false; } - - if (!CoinJoin::IsCollateralValid(...)) { ... } - - // Post-V24: allow up to PROMOTION_RATIO (10) inputs for promotion entries - // Pre-V24: max COINJOIN_ENTRY_MAX_SIZE (9) inputs - const size_t nMaxEntryInputs = CoinJoin::IsPromotionDemotionActive(m_chainman) ? CoinJoin::PROMOTION_RATIO : COINJOIN_ENTRY_MAX_SIZE; - - if (entry.vecTxDSIn.size() > nMaxEntryInputs) { - ... - return false; - }🤖 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/server.cpp` around lines 621 - 668, Update CCoinJoinServer::AddEntry so the initial input-size validation uses the same V24-aware nMaxEntryInputs bound as the later input check, while retaining COINJOIN_ENTRY_MAX_SIZE for outputs. Ensure promotion/demotion entries with PROMOTION_RATIO inputs are not rejected or have collateral consumed before the feature-specific validation runs.
🧹 Nitpick comments (1)
src/test/coinjoin_inouts_tests.cpp (1)
815-854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror-logic tests risk silent divergence from production code.
entry_type_detection_logicre-implements the STANDARD/PROMOTION/DEMOTION/INVALID classification as a local lambda instead of exercising the real classification embedded inCCoinJoinBaseSession::IsValidInOuts(coinjoin.cpp), andTestShouldPromote/TestShouldDemoteare explicitly commented as mirroring client.cpp's decision logic rather than calling it. If the production logic changes, these tests keep passing against the stale mirror while the real behavior silently diverges — the same risk class as the earlierValidatePromotionEntry/ValidateDemotionEntry"dead code" finding, which was resolved precisely by having production call the tested functions directly.Consider extracting the entry-type classification (and the client-side promote/demote decision) into small named, testable functions that both production code and these tests call directly.
Also applies to: 957-1131
🤖 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 815 - 854, Replace the local entry-type lambda in entry_type_detection_logic with a named production classification function used directly by CCoinJoinBaseSession::IsValidInOuts and these tests, preserving the STANDARD/PROMOTION/DEMOTION/INVALID outcomes. Similarly, extract the promote/demote decision logic currently mirrored by TestShouldPromote and TestShouldDemote in client.cpp into callable functions, then update production and tests to use them so behavior cannot diverge.
🤖 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/coinjoin.cpp`:
- Around line 95-137: Update CCoinJoinBroadcastTx::IsValidStructure so the
post-V24 structural validity check caps both tx->vin.size() and tx->vout.size()
at nMaxInputsPostV24. Preserve the existing pre-V24 balanced-count validation
and activation-boundary reporting behavior.
In `@src/coinjoin/server.cpp`:
- Around line 214-219: Remove the pre-V24 unbalanced-entry early return in the
DSVIN handling flow around IsPromotionDemotionActive, so invalid entries reach
AddEntry and IsValidInOuts and consume collateral through the existing
validation path. Do not retain a rejection before AddEntry that bypasses
ConsumeCollateral.
---
Outside diff comments:
In `@src/coinjoin/server.cpp`:
- Around line 621-668: Update CCoinJoinServer::AddEntry so the initial
input-size validation uses the same V24-aware nMaxEntryInputs bound as the later
input check, while retaining COINJOIN_ENTRY_MAX_SIZE for outputs. Ensure
promotion/demotion entries with PROMOTION_RATIO inputs are not rejected or have
collateral consumed before the feature-specific validation runs.
---
Nitpick comments:
In `@src/test/coinjoin_inouts_tests.cpp`:
- Around line 815-854: Replace the local entry-type lambda in
entry_type_detection_logic with a named production classification function used
directly by CCoinJoinBaseSession::IsValidInOuts and these tests, preserving the
STANDARD/PROMOTION/DEMOTION/INVALID outcomes. Similarly, extract the
promote/demote decision logic currently mirrored by TestShouldPromote and
TestShouldDemote in client.cpp into callable functions, then update production
and tests to use them so behavior cannot diverge.
🪄 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
Run ID: 3998d447-b7d6-4cb2-a133-a0563d791fa8
📒 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 (7)
- doc/release-notes-7052.md
- src/coinjoin/common.h
- src/wallet/coinjoin.cpp
- src/wallet/wallet.h
- src/coinjoin/client.h
- src/net_processing.cpp
- src/coinjoin/client.cpp
| bool CCoinJoinBroadcastTx::IsValidStructure(const CBlockIndex* pindex, const ChainstateManager& chainman, | ||
| bool* fPossiblyValidPostV24Ret) const | ||
| { | ||
| if (fPossiblyValidPostV24Ret) *fPossiblyValidPostV24Ret = false; | ||
|
|
||
| // some trivial checks only, activation-independent ones first | ||
| if (masternodeOutpoint.IsNull() && m_protxHash.IsNull()) { | ||
| return false; | ||
| } | ||
| if (tx->vin.size() != tx->vout.size()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (tx->vin.size() < size_t(CoinJoin::GetMinPoolParticipants())) { | ||
| return false; | ||
| } | ||
| if (tx->vin.size() > CoinJoin::GetMaxPoolInputOutputCount()) { | ||
| if (!std::ranges::all_of(tx->vout, [](const auto& txOut) { | ||
| return CoinJoin::IsDenominatedAmount(txOut.nValue) && txOut.scriptPubKey.IsPayToPublicKeyHash(); | ||
| })) { | ||
| return false; | ||
| } | ||
| return std::ranges::all_of(tx->vout, [](const auto& txOut) { | ||
| return CoinJoin::IsDenominatedAmount(txOut.nValue) && txOut.scriptPubKey.IsPayToPublicKeyHash(); | ||
| }); | ||
|
|
||
| // Post-V24: allow unbalanced counts (promotion/demotion) and up to 200 inputs | ||
| // (20 participants * 10 inputs for promotions) | ||
| // Pre-V24: require balanced input/output counts (1:1 mixing only), max 180 inputs | ||
| // (20 participants * 9 entries) | ||
| // Note: For post-V24 unbalanced transactions (promotion/demotion), value sum validation | ||
| // (inputs == outputs) requires UTXO access and is performed in IsValidInOuts() when the | ||
| // transaction is processed. | ||
| const size_t nMaxInputsPreV24 = CoinJoin::GetMaxPoolParticipants() * COINJOIN_ENTRY_MAX_SIZE; | ||
| const size_t nMaxInputsPostV24 = CoinJoin::GetMaxPoolParticipants() * CoinJoin::PROMOTION_RATIO; | ||
| const bool fValidPreV24 = tx->vin.size() == tx->vout.size() && tx->vin.size() <= nMaxInputsPreV24; | ||
| const bool fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24; | ||
|
|
||
| const bool fV24Active = pindex && DeploymentActiveAt(*pindex, chainman, Consensus::DEPLOYMENT_V24); | ||
| if (fV24Active) { | ||
| return fValidPostV24; | ||
| } | ||
|
|
||
| // Pre-V24: report whether the tx would be valid on a post-V24 tip so callers can avoid | ||
| // fully punishing relayers whose tip is ahead of ours around the activation boundary | ||
| if (!fValidPreV24 && fValidPostV24 && fPossiblyValidPostV24Ret) { | ||
| *fPossiblyValidPostV24Ret = true; | ||
| } | ||
| return fValidPreV24; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Post-V24 IsValidStructure still lacks an explicit vout cap — re-opens a previously "addressed" finding.
fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24; only bounds vin.size(). There is no corresponding bound on tx->vout.size(). A demotion-heavy structure can legitimately have up to nMaxInputsPostV24 (200) outputs from as few as GetMinPoolParticipants() inputs, so a malformed DSTX with a huge output count (but a small/valid input count) currently passes this structural pre-check and could be relayed, only to be rejected much later (if at all, for non-mixing-session broadcast paths). This is the same concern raised previously on this function ("Restore an explicit post-V24 vout cap here"), which was marked as addressed in earlier commits but does not appear in the current code — likely lost during the rebase/squash described in the PR's cleanup-pass summary.
🧱 Restore the structural cap
- const bool fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24;
+ const bool fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24 && tx->vout.size() <= nMaxInputsPostV24;🤖 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/coinjoin.cpp` around lines 95 - 137, Update
CCoinJoinBroadcastTx::IsValidStructure so the post-V24 structural validity check
caps both tx->vin.size() and tx->vout.size() at nMaxInputsPostV24. Preserve the
existing pre-V24 balanced-count validation and activation-boundary reporting
behavior.
| // Post-V24: Check if unbalanced entries (promotion/demotion) are allowed | ||
| if (entry.vecTxDSIn.size() != entry.vecTxOut.size() && !CoinJoin::IsPromotionDemotionActive(m_chainman)) { | ||
| LogPrint(BCLog::COINJOIN, "DSVIN -- promotion/demotion entry rejected: V24 not active\n"); | ||
| PushStatus(peer, STATUS_REJECTED, ERR_MODE); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The pre-V24 DSVIN unbalanced-entry rejection is still present, contradicting the PR's own cleanup-pass description and bypassing anti-spam collateral consumption.
The PR's comments summary states this cleanup pass "Removed the pre-V24 DSVIN pre-check so invalid unbalanced entries reach AddEntry and consume collateral," but this exact early-return is still here. Because it returns before AddEntry/IsValidInOuts runs, the pre-V24 branch of IsValidInOuts (which sets fConsumeCollateralRet = true for size mismatches) never executes, so ConsumeCollateral is never called. A peer can submit unbalanced DSVIN entries pre-V24 repeatedly with no economic cost — the same anti-spam gap flagged in a prior review round on this file.
Either drop this pre-check (letting AddEntry handle rejection and collateral consumption), or call ConsumeCollateral(entry.txCollateral) before returning here.
🤖 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/server.cpp` around lines 214 - 219, Remove the pre-V24
unbalanced-entry early return in the DSVIN handling flow around
IsPromotionDemotionActive, so invalid entries reach AddEntry and IsValidInOuts
and consume collateral through the existing validation path. Do not retain a
rejection before AddEntry that bypasses ConsumeCollateral.
|
This pull request has conflicts, please rebase. |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The current head contains four confirmed CoinJoin regressions: the server rejects every valid promotion or demotion entry, pre-V24 unbalanced submissions bypass collateral consumption, rebalance preparation failures retain wallet locks until timeout, and post-V24 DSTX outputs are not structurally capped. The commit stack also contains a build-restoring API adaptation that leaves earlier commits uncompilable, plus same-PR corrective commits that should be folded into their originating changes. Cumulative prior reconciliation is complete with no carried-forward findings.
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).
🤖 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:631-668: The legacy entry-size guard rejects every promotion and demotion
COINJOIN_ENTRY_MAX_SIZE is 9, while a valid promotion has 10 inputs and a valid demotion has 10 outputs. The unconditional guard at lines 631-651 therefore rejects both new entry shapes before reaching the V24-aware input limit or IsValidInOuts(), and may consume the participant's collateral. As a result, no promotion or demotion produced by the client can be accepted by the server after V24 activation. Replace the legacy limit with an activation-aware limit that permits PROMOTION_RATIO items on either side post-V24, and remove the redundant input-only check.
- [BLOCKING] src/coinjoin/server.cpp:214-219: Pre-V24 rejection bypasses collateral consumption
This early return rejects unbalanced DSVIN messages before AddEntry() can apply the existing anti-spam penalty. Without the pre-check, oversized promotion/demotion shapes are rejected by AddEntry's size guard and other unbalanced shapes reach IsValidInOuts(), whose pre-V24 branch sets fConsumeCollateralRet; both paths consume valid session collateral. Returning here instead allows repeated pre-V24 unbalanced submissions without the intended economic cost. Remove this redundant pre-check and let AddEntry perform validation and collateral handling.
In `src/coinjoin/client.cpp`:
- [BLOCKING] src/coinjoin/client.cpp:1467-1485: Entry-preparation failures leave rebalance inputs locked
SelectRebalanceInputs() locks the selected promotion or demotion inputs and records them in vecOutPointLocked before joining the queue. If PreparePromotionEntry() or PrepareDemotionEntry() later fails because an input disappeared, became spent, or is otherwise unusable, these branches return without unlocking the inputs or resetting the session. TrySubmitDenominate() ignores the return value, leaving the session in POOL_STATE_QUEUE and the coins unavailable until the queue timeout. Release the locks, return reserved keys, and reset the session immediately on either preparation failure.
In `src/coinjoin/coinjoin.cpp`:
- [BLOCKING] src/coinjoin/coinjoin.cpp:121-124: Post-V24 DSTX validation does not cap outputs
Post-V24 input and output counts no longer constrain one another, but fValidPostV24 applies the 200-item session limit only to vin. A DSTX with a valid small input count and any output count permitted by general transaction limits therefore passes this structural check as long as every output is denominated P2PKH. Before activation, the same malformed shape can also be classified as possibly valid post-V24 and receive only the reduced fork-boundary penalty. A valid session can produce at most GetMaxPoolParticipants() * PROMOTION_RATIO outputs, so vout must be capped independently.
In `<commit:23a55bc>`:
- [BLOCKING] <commit:23a55bc>:1: Fold the build-restoring API adaptation into the feature
Commit 66a7db4337a introduces references to llmq::CChainLocksHandler, the three-argument DeploymentActiveAt(..., Params().GetConsensus(), DEPLOYMENT_V24) form, GetValidMNsCount(), and GetValidWeightedMNsCount(). Those APIs do not exist on the authoritative base: the base uses chainlock::Chainlocks, requires ChainstateManager for the V24 versionbits deployment, and exposes masternode counts through GetCounts(). Commit 23a55bc8cf7 repairs these references only after eight earlier PR commits, so every intermediate tree from 66a7db4337a through 1141bf49cbe fails to compile. Fold each API adaptation into the commit that introduced the affected code so every retained commit builds against the PR base.
In `<commit:40d75da>`:
- [SUGGESTION] <commit:40d75da>:1: Squash the initial lint and self-correction commits
Commits 40d75da7ffd, cbd56b59538, and 1f015efdab8 only clean up code introduced by the feature commit in this same stack. They respectively reformat new log statements, repair race/resource/UTXO-validation defects in the new implementation, and apply a cosmetic conditional refactor. Fold these changes into 66a7db4337a so blame points to the feature implementation and bisect does not expose known-broken intermediate states.
In `<commit:1141bf4>`:
- [SUGGESTION] <commit:1141bf4>:1: Fold the review-fix chain into its originating changes
Commits 1141bf49cbe, a424f84e6c1, and ff95d445a9e are broad review-fix commits that repair or refactor behavior introduced earlier in this PR, rather than independent logical changes. Commit 5294f2c859c is a direct same-stack correction whose message states that the preceding commit's DSTX exemption also forgave malformed transactions. Fold these corrections into their originating feature and test commits, retaining a coherent feature commit, related test commit, and release-note commit instead of preserving known-defective intermediate implementations.
| return false; | ||
| } | ||
|
|
||
| // Post-V24: allow up to PROMOTION_RATIO (10) inputs for promotion entries | ||
| // Pre-V24: max COINJOIN_ENTRY_MAX_SIZE (9) inputs | ||
| const size_t nMaxEntryInputs = CoinJoin::IsPromotionDemotionActive(m_chainman) ? CoinJoin::PROMOTION_RATIO : COINJOIN_ENTRY_MAX_SIZE; | ||
|
|
||
| if (entry.vecTxDSIn.size() > nMaxEntryInputs) { | ||
| LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- ERROR: too many inputs! %d/%d\n", __func__, entry.vecTxDSIn.size(), nMaxEntryInputs); | ||
| nMessageIDRet = ERR_MAXIMUM; | ||
| ConsumeCollateral(entry.txCollateral); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: The legacy entry-size guard rejects every promotion and demotion
COINJOIN_ENTRY_MAX_SIZE is 9, while a valid promotion has 10 inputs and a valid demotion has 10 outputs. The unconditional guard at lines 631-651 therefore rejects both new entry shapes before reaching the V24-aware input limit or IsValidInOuts(), and may consume the participant's collateral. As a result, no promotion or demotion produced by the client can be accepted by the server after V24 activation. Replace the legacy limit with an activation-aware limit that permits PROMOTION_RATIO items on either side post-V24, and remove the redundant input-only check.
source: ['codex']
| // Post-V24: Check if unbalanced entries (promotion/demotion) are allowed | ||
| if (entry.vecTxDSIn.size() != entry.vecTxOut.size() && !CoinJoin::IsPromotionDemotionActive(m_chainman)) { | ||
| LogPrint(BCLog::COINJOIN, "DSVIN -- promotion/demotion entry rejected: V24 not active\n"); | ||
| PushStatus(peer, STATUS_REJECTED, ERR_MODE); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Pre-V24 rejection bypasses collateral consumption
This early return rejects unbalanced DSVIN messages before AddEntry() can apply the existing anti-spam penalty. Without the pre-check, oversized promotion/demotion shapes are rejected by AddEntry's size guard and other unbalanced shapes reach IsValidInOuts(), whose pre-V24 branch sets fConsumeCollateralRet; both paths consume valid session collateral. Returning here instead allows repeated pre-V24 unbalanced submissions without the intended economic cost. Remove this redundant pre-check and let AddEntry perform validation and collateral handling.
source: ['codex']
| if (m_fPromotion) { | ||
| if (PreparePromotionEntry(strError, vecPSInOutPairsTmp)) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Promotion entry prepared, sending\n"); | ||
| return SendDenominate(vecPSInOutPairsTmp, connman); | ||
| } | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- PreparePromotionEntry failed: %s\n", strError); | ||
| strAutoDenomResult = Untranslated(strError); | ||
| return false; | ||
| } | ||
|
|
||
| if (m_fDemotion) { | ||
| if (PrepareDemotionEntry(strError, vecPSInOutPairsTmp)) { | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Demotion entry prepared, sending\n"); | ||
| return SendDenominate(vecPSInOutPairsTmp, connman); | ||
| } | ||
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- PrepareDemotionEntry failed: %s\n", strError); | ||
| strAutoDenomResult = Untranslated(strError); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Entry-preparation failures leave rebalance inputs locked
SelectRebalanceInputs() locks the selected promotion or demotion inputs and records them in vecOutPointLocked before joining the queue. If PreparePromotionEntry() or PrepareDemotionEntry() later fails because an input disappeared, became spent, or is otherwise unusable, these branches return without unlocking the inputs or resetting the session. TrySubmitDenominate() ignores the return value, leaving the session in POOL_STATE_QUEUE and the coins unavailable until the queue timeout. Release the locks, return reserved keys, and reset the session immediately on either preparation failure.
| if (m_fPromotion) { | |
| if (PreparePromotionEntry(strError, vecPSInOutPairsTmp)) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Promotion entry prepared, sending\n"); | |
| return SendDenominate(vecPSInOutPairsTmp, connman); | |
| } | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- PreparePromotionEntry failed: %s\n", strError); | |
| strAutoDenomResult = Untranslated(strError); | |
| return false; | |
| } | |
| if (m_fDemotion) { | |
| if (PrepareDemotionEntry(strError, vecPSInOutPairsTmp)) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Demotion entry prepared, sending\n"); | |
| return SendDenominate(vecPSInOutPairsTmp, connman); | |
| } | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- PrepareDemotionEntry failed: %s\n", strError); | |
| strAutoDenomResult = Untranslated(strError); | |
| return false; | |
| } | |
| if (m_fPromotion || m_fDemotion) { | |
| const bool fPrepared = m_fPromotion ? PreparePromotionEntry(strError, vecPSInOutPairsTmp) | |
| : PrepareDemotionEntry(strError, vecPSInOutPairsTmp); | |
| if (fPrepared) { | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- %s entry prepared, sending\n", | |
| m_fPromotion ? "Promotion" : "Demotion"); | |
| return SendDenominate(vecPSInOutPairsTmp, connman); | |
| } | |
| WalletCJLogPrint(m_wallet, "CCoinJoinClientSession::SubmitDenominate -- Prepare%sEntry failed: %s\n", | |
| m_fPromotion ? "Promotion" : "Demotion", strError); | |
| strAutoDenomResult = Untranslated(strError); | |
| UnlockCoins(); | |
| keyHolderStorage.ReturnAll(); | |
| WITH_LOCK(cs_coinjoin, SetNull()); | |
| return false; | |
| } |
source: ['codex']
| const size_t nMaxInputsPreV24 = CoinJoin::GetMaxPoolParticipants() * COINJOIN_ENTRY_MAX_SIZE; | ||
| const size_t nMaxInputsPostV24 = CoinJoin::GetMaxPoolParticipants() * CoinJoin::PROMOTION_RATIO; | ||
| const bool fValidPreV24 = tx->vin.size() == tx->vout.size() && tx->vin.size() <= nMaxInputsPreV24; | ||
| const bool fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24; |
There was a problem hiding this comment.
🔴 Blocking: Post-V24 DSTX validation does not cap outputs
Post-V24 input and output counts no longer constrain one another, but fValidPostV24 applies the 200-item session limit only to vin. A DSTX with a valid small input count and any output count permitted by general transaction limits therefore passes this structural check as long as every output is denominated P2PKH. Before activation, the same malformed shape can also be classified as possibly valid post-V24 and receive only the reduced fork-boundary penalty. A valid session can produce at most GetMaxPoolParticipants() * PROMOTION_RATIO outputs, so vout must be capped independently.
| const size_t nMaxInputsPreV24 = CoinJoin::GetMaxPoolParticipants() * COINJOIN_ENTRY_MAX_SIZE; | |
| const size_t nMaxInputsPostV24 = CoinJoin::GetMaxPoolParticipants() * CoinJoin::PROMOTION_RATIO; | |
| const bool fValidPreV24 = tx->vin.size() == tx->vout.size() && tx->vin.size() <= nMaxInputsPreV24; | |
| const bool fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24; | |
| const bool fValidPostV24 = tx->vin.size() <= nMaxInputsPostV24 && | |
| tx->vout.size() <= nMaxInputsPostV24; |
source: ['codex']
3 participants
Summary
This PR adds CoinJoin denomination promotion and demotion so participants can convert between adjacent standard denominations inside a mixing session instead of being limited to strict 1:1 denomination mixes.
The new behavior is gated by V24 activation. Pre-V24 behavior remains unchanged.
What was done?
AddEntry→IsValidInOutsso their collateral is consumed as before (anti-spam behavior preserved)CoinJoin::ValidateFinalTxComposition()and covered it directly in unit testsHistory
The branch is rebased onto current
developand squashed into three atomic commits (feature, tests, release notes); all intermediate self-correction commits from earlier review rounds are folded into the feature commit per review feedback.How Has This Been Tested?
./src/test/test_dash --run_test=coinjoin_inouts_tests(35 cases) and--run_test=coinjoin_testspassdashdandtest_dashbuild cleanly at each commitPost-V24 activation behavior still needs functional coverage because EHF activation paths cannot be fully exercised in these unit tests alone.
Breaking Changes
None. The feature is activation-gated and preserves existing pre-V24 behavior.