fix(coinjoin): only report a promotion when its inputs can be selected

GetDenominationCounts() counted every fully-mixed coin, but
SelectFullyMixedForPromotion() takes at most one coin per parent
transaction. Standard mixing clusters up to 9 of a wallet's coins in each
mixing transaction, so wallets routinely held PROMOTION_RATIO fully-mixed
coins spread over fewer than PROMOTION_RATIO transactions. Such a wallet
reported a promotion opportunity on every attempt, failed selection every
time, never promoted, and skipped the "nothing to do" exits it would
otherwise take.

Count fully-mixed coins at most once per parent transaction, which is what
selection can return. ShouldDemote() only needs one such coin and is
unaffected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Author: PastaPastaPasta

src/wallet/coinjoin.cpp

@@ -431,6 +431,7 @@ CoinJoinDenomCounts CWallet::GetDenominationCounts() const
     if (!CCoinJoinClientOptions::IsEnabled()) return counts;
 
     const auto& denoms = CoinJoin::GetStandardDenominations();
+    std::array<std::set<uint256>, std::tuple_size_v<decltype(counts.fully_mixed)>> fully_mixed_parents;
 
     LOCK(cs_wallet);
     for (const auto& outpoint : setWalletUTXO) {
@@ -448,7 +449,9 @@ CoinJoinDenomCounts CWallet::GetDenominationCounts() const
 
         const size_t idx = static_cast<size_t>(denom_it - denoms.begin());
         counts.total[idx]++;
-        if (IsFullyMixed(outpoint)) counts.fully_mixed[idx]++;
+        if (IsFullyMixed(outpoint) && fully_mixed_parents[idx].insert(outpoint.hash).second) {
+            counts.fully_mixed[idx]++;
+        }
     }
 
     return counts;

src/wallet/test/coinjoin_tests.cpp

@@ -597,6 +597,71 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_rebalance_rounds_reset_tests, CTransactionBuild
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(coinjoin_promotion_needs_coins_from_distinct_txs_tests, CTransactionBuilderTestSetup)
+{
+    constexpr CAmount nSmallerAmount{10000100};
+    constexpr CAmount nLargerAmount{nSmallerAmount * CoinJoin::PROMOTION_RATIO};
+    const int nSmallerDenom{CoinJoin::AmountToDenomination(nSmallerAmount)};
+    const int nLargerDenom{CoinJoin::AmountToDenomination(nLargerAmount)};
+    const size_t idxSmaller = CoinJoin::GetDenominationIndex(nSmallerDenom);
+    const CScript scriptOurs = GetScriptForRawPubKey(coinbaseKey.GetPubKey());
+
+    const bool fEnabledPrev{CCoinJoinClientOptions::IsEnabled()};
+    const int nRoundsPrev{CCoinJoinClientOptions::GetRounds()};
+    const int nGoalPrev{CCoinJoinClientOptions::GetDenomsGoal()};
+    CCoinJoinClientOptions::SetEnabled(true);
+    CCoinJoinClientOptions::SetRounds(MIN_COINJOIN_ROUNDS);
+    CCoinJoinClientOptions::SetDenomsGoal(MIN_COINJOIN_DENOMS_GOAL);
+
+    // Mines a zero-fee self-mixing tx spending vecIn, one output per input
+    const auto mix = [&](const std::vector<COutPoint>& vecIn) {
+        CMutableTransaction mtx;
+        for (const auto& outpoint : vecIn) {
+            mtx.vin.emplace_back(outpoint);
+            mtx.vout.emplace_back(nSmallerAmount, scriptOurs);
+        }
+        BOOST_REQUIRE(WITH_LOCK(wallet->cs_wallet, return wallet->SignTransaction(mtx)));
+        BOOST_REQUIRE(wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}));
+        AddTxToChain(mtx.GetHash());
+        std::vector<COutPoint> vecOut;
+        for (uint32_t n = 0; n < mtx.vout.size(); ++n) {
+            vecOut.emplace_back(mtx.GetHash(), n);
+        }
+        return vecOut;
+    };
+
+    // PROMOTION_RATIO coins mixed together until fully mixed whatever the salt decides
+    std::vector<COutPoint> vecCoins = GetTallyItem(std::vector<CAmount>(CoinJoin::PROMOTION_RATIO, nSmallerAmount)).outpoints;
+    for (int i = 0; i < CCoinJoinClientOptions::GetRounds() + CCoinJoinClientOptions::GetRandomRounds(); ++i) {
+        vecCoins = mix(vecCoins);
+    }
+    for (const auto& outpoint : vecCoins) {
+        BOOST_REQUIRE(wallet->IsFullyMixed(outpoint));
+    }
+
+    // All of them share one parent tx, so a promotion can only ever select one of them: no
+    // promotion opportunity may be reported, or the client would chase it on every attempt
+    auto counts = wallet->GetDenominationCounts();
+    BOOST_CHECK_EQUAL(counts.total[idxSmaller], CoinJoin::PROMOTION_RATIO);
+    BOOST_CHECK_EQUAL(counts.fully_mixed[idxSmaller], 1);
+    BOOST_CHECK(!CCoinJoinClientManager::ShouldPromote(nSmallerDenom, nLargerDenom, counts));
+    BOOST_CHECK_EQUAL(wallet->SelectFullyMixedForPromotion(nSmallerDenom, CoinJoin::PROMOTION_RATIO).size(), 1U);
+
+    // One more round, each coin in its own tx: now the promotion is both reported and selectable
+    for (const auto& outpoint : vecCoins) {
+        mix({outpoint});
+    }
+    counts = wallet->GetDenominationCounts();
+    BOOST_CHECK_EQUAL(counts.fully_mixed[idxSmaller], CoinJoin::PROMOTION_RATIO);
+    BOOST_CHECK(CCoinJoinClientManager::ShouldPromote(nSmallerDenom, nLargerDenom, counts));
+    BOOST_CHECK_EQUAL(wallet->SelectFullyMixedForPromotion(nSmallerDenom, CoinJoin::PROMOTION_RATIO).size(),
+                      static_cast<size_t>(CoinJoin::PROMOTION_RATIO));
+
+    CCoinJoinClientOptions::SetDenomsGoal(nGoalPrev);
+    CCoinJoinClientOptions::SetRounds(nRoundsPrev);
+    CCoinJoinClientOptions::SetEnabled(fEnabledPrev);
+}
+
 BOOST_FIXTURE_TEST_CASE(coinjoin_rounds_cache_unknown_tx_tests, CTransactionBuilderTestSetup)
 {
     constexpr CAmount nDenomAmount{10000100}; // 0.100001 DASH

src/wallet/wallet.h

@@ -311,6 +311,8 @@ class WalletRescanReserver; //forward declarations for ScanForWalletTransactions
 //! CoinJoin::vecStandardDenominations (index 0 = largest denomination).
 struct CoinJoinDenomCounts {
     std::array<int, 5> total{};
+    //! Fully-mixed coins that can be spent together, i.e. at most one per transaction: a
+    //! conversion never takes two coins with the same parent (see SelectFullyMixedForPromotion)
     std::array<int, 5> fully_mixed{};
 };